Repository navigation
feat(p2p): per-peer fork download budget - #2133
nullPointerEnjoyer wants to merge 8 commits into
Conversation
Downloading and validating the blocks behind announced headers is an expensive operation. A peer could previously make us perform an unbounded amount of such work over time by repeatedly announcing distinct header chains that don't extend our tip. Introduce a token-bucket budget (capacity + refill interval, both configurable via the p2p protocol config) that every non-empty header list will be charged against; header lists received during the initial block download are exempt so that node bootstrap can't be stalled. No wire-level changes: the budget only gates our own block requests.
Charge every non-empty header list (including tip-anchored ones: a miner can produce an unlimited number of distinct valid children of our tip, so exempting them would leave the cost attack open) against the peer's budget before downloading the announced blocks. An exhausted budget defers the list: the peer is not punished (no ban score, no disconnect; it hasn't violated the protocol), the list is expected to arrive later via a scheduled header request retry that fires once per refill interval and is subject to the budget itself. Also stop requesting more headers from a peer that repeatedly sends already-known header lists while claiming it may have more of them, which would otherwise keep us issuing header requests forever at no cost to it.
Cover the three behaviors of the budget: deferral without punishment followed by recovery after a refill, budget-limited tip-anchored fork variants, and the initial block download exemption.
Addresses an OCR review finding: keep the pre-existing ProtocolConfig test literals stable so that adding a config field in the future doesn't require touching them again.
Addresses an OCR review finding: keep the helper consistent with the existing make_new_blocks helper style by holding the previous block as a borrow instead of cloning it on every iteration.
The budget retry timer runs on the real tokio clock while refills are driven by the mocked one; a long interval leaves the retry pending for a long real time, which can interleave with the sync loops when the test machine is under load.
Addresses OCR review findings: merge the duplicated refill-interval rationale into one accurate comment and describe the actual fork topology of the tip-anchored variants test, including the guarantee it pins down (the budget is charged before the anchoring check, so lists anchored at unknown blocks are deferred rather than punished).
The fork download budget retry had two weaknesses: - The retry slept inside a select! branch with a variable duration, so the sleep was recreated on every loop iteration and could be postponed indefinitely by message traffic. Move the retry deadline check into the loop body and add an unconditional fixed-interval tick arm to guarantee wakeups when the peer is idle. - A repeated deferral re-scheduled the retry further into the future, so announcements that arrive late could keep postponing the fetch. Keep the earliest scheduled retry instead, so a new deferral never postpones a retry the budget may already have recovered for.
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let headers_count = headers.len(); | ||
| if !self.fork_download_budget.try_take(headers_count, self.time_getter.get_time()) { |
There was a problem hiding this comment.
try_take is an all-or-nothing grant, and a header list can be larger than the budget capacity. Since ForkDownloadLimit (2000) equals the default HeaderLimit/msg_header_count_limit (2000), a peer may legitimately send a full-size (2000-header) list; if the bucket is not completely full at that moment, try_take fails, and because tokens only refill up to capacity, the same request can never succeed. The result is a permanent deferral of that list: every retry scheduled below re-requests headers, the peer re-sends the same oversized list, and it is deferred again — an infinite, self-sustaining loop that silently stalls synchronization from this peer with no ban score and no distinguishing diagnostic. Mitigate by capping the charged amount at min(headers_count, capacity) (granting partial progress per refill), or by granting/deferring in capacity-sized chunks, or at minimum ensure ForkDownloadLimit > HeaderLimit with a validation/debug assertion.
Suggestion:
| let headers_count = headers.len(); | |
| if !self.fork_download_budget.try_take(headers_count, self.time_getter.get_time()) { | |
| let headers_count = headers.len().min( | |
| *self.p2p_config.protocol_config.max_fork_downloads_per_peer, | |
| ); | |
| if !self.fork_download_budget.try_take(headers_count, self.time_getter.get_time()) { |
1 participant
Summary
Test plan