Conversation
… sent pings Action item 2 from #4424. `Ping` had no per-type inbound counter, so its only bound was the generic `MESSAGE_LIMIT` of 500 messages per 5 seconds - 100/s. `Ping` is the most expensive message to parse per byte on the untrusted path: it is the only message whose size is unbounded by a fixed shape, and `BlockLocators::read_le` does a multi-pass structural validation of ~73 KiB at today's mainnet height. 100/s of that is ~7 MB/s of locator parsing per peer. Two halves, because the limit is not safe without the second. Receiver side: a `seen_inbound_pings` counter alongside the existing per-type counters (`insert_inbound_ping`, 60-second window), checked in the `Message::Ping` arm and swept by `clear_stale_entries` like its siblings. Sender side: `Ping::ping_all_peers` pings every peer on every batch of newly synced blocks. During catch-up those batches are paced only by how fast block responses arrive, and a peer becomes re-pingable as soon as its `Pong` lands, so the rate at which we ping a given peer is bounded by the round trip to it rather than by anything on this node. This is the spam-detection hazard the `PingInner` TODO flagged; `MIN_PING_INTERVAL` (1s) now floors it, and the TODO is resolved. Enforcing that needs the ping times themselves, so `PingInner` gains `last_ping`, pruned each pass of entries older than `MAX_PING_INTERVAL` - past which every peer satisfies the floor anyway, so a missing entry reads as "may be pinged now" and the map stays bounded by peer count without a disconnect hook. Deriving eligibility from the schedule key instead would be wrong: a deferred peer must be rescheduled at `last_ping + MIN_PING_INTERVAL` rather than left on its `pong + MAX_PING_INTERVAL` timer, because the announcement it would have carried is what peers most want when a burst of new blocks *ends*, and the old timer delays it ~19s instead of ~1s. `next_ping` is now keyed by peer (`HashMap<SocketAddr, Instant>`) rather than by instant. That fixes two things. An instant-keyed map silently dropped a peer's ping whenever two peers were scheduled for the same instant, with nothing to reschedule it. And nothing checks that a `Pong` was solicited - `inbound.rs` forwards every one to `on_pong_received` - so a peer could accumulate a schedule entry per `Pong` and collect one ~73 KiB `Ping` per entry on the next block, turning cheap inbound pongs into unbounded outbound bandwidth. One entry per peer is now structural rather than something the ping paths have to maintain. `MAXIMUM_PINGS_PER_INTERVAL` is 1200 per 60s, sized for the transition rather than for hardening: peers predating the floor can still legitimately burst, and dropping one mid-sync is worse than the looser bound - the more so because the window is keyed by listener address and outlives the disconnect, so such a peer re-trips on its first message back and stays unusable for the rest of the interval. Clearing it on disconnect would let a peer reset its budget by reconnecting, which is why no sibling counter is cleared there either. Once the floor is ubiquitous this drops to ~120, which is where it bounds anything. Also drops `Inbound::PING_SLEEP_IN_SECS`, which has had no references since the cadence moved to `Ping::MAX_PING_INTERVAL` and duplicated its value. Refs #4424. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ET44Km85Tt1Ar7SqeoQzN6
This branch has not been deployed
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
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.
WIP, I'm still iterating on this
Action item 2 from #4424.
Pinghad no per-type inbound counter, so its only bound was the genericMESSAGE_LIMITof 500 messages per 5 seconds - 100/s.Pingis the most expensive message to parse per byte on the untrusted path: it is the only message whose size is unbounded by a fixed shape, andBlockLocators::read_ledoes a multi-pass structural validation of ~73 KiB at today's mainnet height. 100/s of that is ~7 MB/s of locator parsing per peer.Two halves, because the limit is not safe without the second.
Receiver side: a
seen_inbound_pingscounter alongside the existing per-type counters (insert_inbound_ping, 60-second window), checked in theMessage::Pingarm and swept byclear_stale_entrieslike its siblings.Sender side:
Ping::ping_all_peerspings every peer on every batch of newly synced blocks. During catch-up those batches are paced only by how fast block responses arrive, and a peer becomes re-pingable as soon as itsPonglands, so the rate at which we ping a given peer is bounded by the round trip to it rather than by anything on this node. This is the spam-detection hazard thePingInnerTODO flagged;MIN_PING_INTERVAL(1s) now floors it, and the TODO is resolved.Enforcing that needs the ping times themselves, so
PingInnergainslast_ping, pruned each pass of entries older thanMAX_PING_INTERVAL- past which every peer satisfies the floor anyway, so a missing entry reads as "may be pinged now" and the map stays bounded by peer count without a disconnect hook. Deriving eligibility from the schedule key instead would be wrong: a deferred peer must be rescheduled atlast_ping + MIN_PING_INTERVALrather than left on itspong + MAX_PING_INTERVALtimer, because the announcement it would have carried is what peers most want when a burst of new blocks ends, and the old timer delays it ~19s instead of ~1s.next_pingis now keyed by peer (HashMap<SocketAddr, Instant>) rather than by instant. That fixes two things. An instant-keyed map silently dropped a peer's ping whenever two peers were scheduled for the same instant, with nothing to reschedule it. And nothing checks that aPongwas solicited -inbound.rsforwards every one toon_pong_received- so a peer could accumulate a schedule entry perPongand collect one ~73 KiBPingper entry on the next block, turning cheap inbound pongs into unbounded outbound bandwidth. One entry per peer is now structural rather than something the ping paths have to maintain.MAXIMUM_PINGS_PER_INTERVALis 1200 per 60s, sized for the transition rather than for hardening: peers predating the floor can still legitimately burst, and dropping one mid-sync is worse than the looser bound - the more so because the window is keyed by listener address and outlives the disconnect, so such a peer re-trips on its first message back and stays unusable for the rest of the interval. Clearing it on disconnect would let a peer reset its budget by reconnecting, which is why no sibling counter is cleared there either. Once the floor is ubiquitous this drops to ~120, which is where it bounds anything.Also drops
Inbound::PING_SLEEP_IN_SECS, which has had no references since the cadence moved toPing::MAX_PING_INTERVALand duplicated its value.Refs #4424.
Claude-Session: https://claude.ai/code/session_01ET44Km85Tt1Ar7SqeoQzN6
Motivation
Write your motivation here, make sure to link related issues and PRs.
Test Plan
If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!
Documentation
If this PR adds or changes functionality, consider clarifying which docs need to be updated, e.g. on AleoNet/welcome.
Backwards compatibility
Please review backwards compatibility. Does any functionality need to be guarded by an existing or new
ConsensusVersion?