[Fix] Prune stale inbound rate-limit cache entries to bound memory growth - #4331
Open
thep2p wants to merge 3 commits into
Open
[Fix] Prune stale inbound rate-limit cache entries to bound memory growth#4331thep2p wants to merge 3 commits into
thep2p wants to merge 3 commits into
Conversation
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.
Motivation
The router's per-peer inbound rate-limit caches in Cache grow without bound. Each of these maps is only ever inserted into:
seen_inbound_connections(keyed by rawIpAddr, populated pre-handshake on every connection attempt)seen_inbound_messages,seen_inbound_puzzle_requests,seen_inbound_block_requests,seen_inbound_unconfirmed_solutions(keyed bySocketAddr)The per-key timestamp
VecDequesare trimmed on access, but an emptied key is never removed, andclear_peer_entries(the disconnect hook) pruned onlyseen_outbound_block_requests— despite a doc comment claiming it removed all per-peer entries. As a result a peer that cycles source IPs / advertised listener ports leaves a permanent entry per key, with no eviction and no size cap, i.e. a slow, remotely-driven memory-growth vector.seen_inbound_connectionsis reachable by unauthenticated peers since it is populated before the handshake completes.Fix: add
Cache::clear_stale_entries, a periodic sweep (called from the router heartbeat) that removes any key whose timestamps are all expired. The safety invariant: a fully-expired entry carries no rate-limit weight, so dropping it is observationally a no-op — it never resets a live peer's limit and never evicts fresh data, so it can't be abused to dodge rate limiting. To let the heartbeat reference the two windows it doesn't own,MESSAGE_LIMIT_TIME_FRAME_IN_SECSis moved ontoRouter<N>next toCONNECTION_ATTEMPTS_SINCE_SECS, and both are made available in all build configs.Why a sweep, not cleanup-on-disconnect? Extending
clear_peer_entriesto clear all per-peer maps on disconnect looks simpler, but it can't cover everything:seen_inbound_connectionsis populated before the handshake (and for peers that get banned or never fully connect), so those keys may never reach a disconnect event; and a peer thatdisconnects while its window is still hot leaves an entry that goes stale later but is never revisited. A periodic sweep visits every key on a schedule regardless of whether the peer returns, so it's the only mechanism that covers all cases — disconnect-time cleanup would at best be an optimization. The sweep also removes only fully-expired entries, so unlike wiping a peer's state on disconnect it can't be used to reset a live rate-limit budget by reconnecting.
Related:
Test Plan
Four unit tests added in node/router/src/helpers/cache.rs, one per guarantee:
test_clear_stale_entries_removes_expired_keys: expired keys are removed, map shrinks to 0 (regression)test_clear_stale_entries_preserves_fresh_entries: live entries survive; rate-limit count is not resettest_clear_stale_entries_trims_expired_but_keeps_active_keys: partial expiry: stale timestamps trimmed, active key kepttest_clear_stale_entries_sweeps_all_inbound_maps: all five inbound maps are sweptDocumentation
No external docs (AleoNet/welcome) require changes — this is internal node-local behavior.
clear_stale_entriescarries a doc comment explaining the "expired entry == no-op to remove" safety invariant, and the misleading clear_peer_entries comment is corrected to describe what it actually prunes.Backwards compatibility
No consensus behavior changes; nothing is gated by
ConsensusVersion, and the wire protocol is unchanged — this is purely node-local memory hygiene. Minor note:MESSAGE_LIMIT_TIME_FRAME_IN_SECSandCONNECTION_ATTEMPTS_SINCE_SECSare now defined in all build configs (previously#[cfg(not(feature = "test"))]); behavior is unchanged because the throttling logic that consumes them remains test-gated — only the constants are now always visible, so the heartbeat sweep can reference them.