Repository navigation
cl: add per-peer rate limiting on P2P ReqResp handlers - #20146
Merged
Merged
Conversation
Implement per-peer, per-protocol token bucket rate limiting and per-peer concurrent stream caps on all incoming Caplin ReqResp handlers. Previously, checkRateLimit() was defined but never called, and no concurrency limits existed, allowing a single peer to flood unlimited requests. Rate limits are aligned with Lighthouse/Lodestar (e.g. 128 blocks/10s, 2 pings/10s, 1 goodbye/10s). A 30-second punishment is applied when a peer exceeds a protocol's rate limit. Concurrent in-flight requests per peer are capped at 5. Removes dead rate limiting code (RateLimits struct, checkRateLimit method, punishmentEndTimes sync.Map) that was never wired up. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move peerID extraction after defer recover() to avoid unrecoverable panic - Use Load before LoadOrStore to avoid allocating a tokenBucket on every request - Remove unused ErrRateLimited/ErrTooManyRequests exports and duplicate RateLimitedPrefix; use existing InvalidRequestPrefix constant instead - Fix copyright year (2024 → 2026) and add header to test file Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add periodic cleanup of stale sync.Map entries (expired punishments, idle token buckets, zero-count concurrency counters) to prevent unbounded memory growth as peers connect and disconnect. Write InvalidRequestPrefix on concurrency rejection to match rate-limit path. Remove redundant s.Close() after s.Reset(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
hashFiles() is not available in composite action outputs.value expressions. Use steps.restore.outputs.cache-primary-key instead, which reflects the exact key passed to the cache/restore step. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 task
Write a spec-compliant SSZ-snappy error message (via EncodeAndWrite) on rate-limit and concurrency rejections instead of a raw response-code byte. Remove concurrency-counter cleanup from the periodic sweep to eliminate a TOCTOU race between cleanup's Delete and acquireConcurrency's LoadOrStore. Each entry is a single *atomic.Int32 per peer (~40 bytes), so the memory cost of keeping them is negligible. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…omments Enable libp2p ResourceManager with per-peer inbound stream cap of 32 (matching Lighthouse's MAX_INBOUND_SUBSTREAMS), down from the default 256. This adds transport-level protection complementary to the application-level token bucket rate limiter. Remove dead comments: observeBandwidth() (function never existed) and rcmgrObs.MustRegisterWith() (variable never existed). Bandwidth monitoring is already enabled via metrics.BandwidthCounter in p2p.go. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Change rate limiting from per-request to per-response-item counting, aligned with Lighthouse's approach. The wrapper pre-consumes 1 token for admission; batch handlers (blocks-by-range, blobs-by-range, etc.) consume additional tokens proportional to the requested item count after decoding the request. This closes the ~96x effectiveness gap vs Lighthouse. For example, blocks-by-range now costs min(req.Count, 96) tokens per request instead of 1, giving an effective burst of ~128 blocks (matching Lighthouse's 128 blocks/10s). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ng punishment Use s.Close() instead of s.Reset() on rate-limit and concurrency rejections so the peer can read the SSZ error response before the stream is torn down. When punishment is triggered, drain the token bucket to 0 and advance lastRefill to the punishment expiry (minus a 1-token grace window). This suppresses token accumulation during the punishment period, preventing a repeat offender from getting a fresh full burst at expiry. The tryConsume refill is clamped to non-negative elapsed time so the advanced lastRefill does not subtract tokens. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add end-to-end tests over real libp2p streams that verify rate limiting works at the protocol level. TestPingRateLimit confirms burst=2 and punishment. TestBlocksByRangeRateLimit confirms per-item token costing rejects a second 96-block request after the first exhausts most of the 128-token burst. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
yperbasis
changed the title
yperbasis
changed the title
yperbasis
marked this pull request as ready for review
March 25, 2026 16:08
yperbasis
requested review from
AskAlexSharov,
Giulio2002 and
domiwei
as code owners
March 25, 2026 16:08
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Mar 25, 2026
## Summary - Use `steps.restore.outputs.cache-primary-key` instead of re-computing the inline `hashFiles` expression in the `restore-mod-cache` composite action's `primary-key` output. The inline expression was being evaluated at the wrong level (composite action output context rather than step context), causing a cache key mismatch between restore and save. Cherry-picked from #20146 (ae57f6f) to land independently. ## Test plan - [ ] Verify CI cache hit/miss behavior on a workflow run using this action 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Oleksandr Lystopad <oleksandr.lystopad@erigon.tech>
Use GetBlobParameters(epoch) instead of max(MaxBlobsPerBlock, MaxBlobsPerBlockElectra) so Deneb requests (max 6 blobs/block) are not overcharged at the Electra rate (max 9). Blob sidecars are deprecated at Fulu, so this only affects Deneb/Electra ranges. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
|
BlobSchedule-aware lookup |
domiwei
previously approved these changes
Mar 26, 2026
domiwei
reviewed
Mar 26, 2026
domiwei
left a comment
domiwei
left a comment
Member
There was a problem hiding this comment.
Overall: great PR — the two-phase token consumption (1 for admission in the wrapper, remainder after decoding) is clean, and the punishment drain + lastRefill advance to prevent burst regeneration is a nice detail. Tests are thorough (both unit and integration over real libp2p streams).
Two comments:
1. blobs.go — use GetBlobParameters() instead of max()
// current
maxBlobs := max(int(c.beaconConfig.MaxBlobsPerBlock), int(c.beaconConfig.MaxBlobsPerBlockElectra))
// suggested
startEpoch := req.StartSlot / c.beaconConfig.SlotsPerEpoch
maxBlobs := int(c.beaconConfig.GetBlobParameters(startEpoch).MaxBlobsPerBlock)max(MaxBlobsPerBlock, MaxBlobsPerBlockElectra) always charges at the Electra rate (9), which overcharges Deneb requests by 50% (actual max is 6). GetBlobParameters(epoch) is the canonical API used elsewhere (on_block.go, beacon_block.go, operations.go) and handles BlobSchedule correctly.
Blob sidecars are deprecated at Fulu so the effective max here is always ≤ Electra's, but using the schedule-aware lookup is more precise and consistent with the rest of the codebase.
2. Nit: typo
maxBlobsThroughoutputPerRequest → maxBlobsThroughputPerRequest (Throughoutput → Throughput)
domiwei
enabled auto-merge
March 26, 2026 06:09
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
domiwei
approved these changes
Mar 26, 2026
wmitsuda
pushed a commit
that referenced
this pull request
Apr 1, 2026
## Summary - Use `steps.restore.outputs.cache-primary-key` instead of re-computing the inline `hashFiles` expression in the `restore-mod-cache` composite action's `primary-key` output. The inline expression was being evaluated at the wrong level (composite action output context rather than step context), causing a cache key mismatch between restore and save. Cherry-picked from #20146 (ae57f6f) to land independently. ## Test plan - [ ] Verify CI cache hit/miss behavior on a workflow run using this action 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Oleksandr Lystopad <oleksandr.lystopad@erigon.tech>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RateLimitsstruct,checkRateLimit()method,punishmentEndTimessync.Map) and dead comments (observeBandwidth,rcmgrObs) that were never wired upFixes https://github.com/ethereum-bounty/erigon/issues/3
🤖 Generated with Claude Code