warp sync hardening - #2014
Conversation
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e39b7dede5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Some(SeenRequestsValue::Fulfilled(requests)) => { | ||
| *requests = requests.saturating_add(1); | ||
| if *requests > MAX_NUMBER_OF_SAME_REQUESTS_PER_PEER { | ||
| return Err(HandleError::SameRequest); |
There was a problem hiding this comment.
When a transfer fails after the first chunk because of a lost response, later chunk timeout, or transient import failure, the client restarts that peer at offset 0 and the monitor retries recovery indefinitely. The server counts those restarts as replay abuse, so the third attempt returns SameRequest and applies the instant-ban i32::MIN penalty even though the peer is following this repository's client protocol; with only one snapshot server, this can permanently prevent warp recovery. Retry tracking needs to distinguish legitimate transfer restarts or the client needs resumable progress rather than banning repeated offset-zero requests.
Useful? React with 👍 / 👎.
| } | ||
| // Cap each range at the shorter of the per-chunk timeout and the remaining budget, so | ||
| // the last request cannot overrun the deadline it was checked against. | ||
| let chunk_timeout = CHUNK_TIMEOUT.min(deadline - Instant::now()); |
There was a problem hiding this comment.
If a slow transfer is still just under budget at the preceding check and the task is descheduled before this subtraction, Instant::now() can be later than deadline, causing deadline - Instant::now() to panic. Because the recovery monitor is non-essential while its gate remains armed, that panic can leave block import and authoring disabled indefinitely instead of reporting TransferTooSlow; re-check or use a saturating/checked remaining-duration calculation.
Useful? React with 👍 / 👎.
… server Serializing the ledger arena is the most expensive thing this node does for a remote peer, and the request that triggers it is ~44 bytes. The handler had no rate limiting and no reputation reporting, and memoized exactly one target block, so alternating two finalized hashes evicted the memo on every request and forced a full serialize + compress each time. One peer could pin the handler thread indefinitely and starve honest warp-syncing peers. Substrate's state_request_handler -- which this is patterned on -- already carries seen_requests plus rep::SAME_REQUEST for the same class of abuse, at a fraction of the per-request cost. Carry that over and add the bound the memo cannot provide on its own: - seen_requests LRU keyed (peer, target, offset), penalising a peer that replays a byte-identical range. An honest client pages each offset once. - snapshot memo widened to a 3-entry LRU, so alternation is a hit and nearby targets share work. This bounds memory, not CPU. - per-peer serialization budget, charged *before* the work: a peer cycling target blocks to defeat a memo of any fixed size is refused rather than served-then-penalised. This is the actual resource bound; the reputation change only accelerates eviction. Cheap rejections (unknown block, not finalized, undecodable request) stay unpenalised -- an honest peer racing finality or a reorg produces those, and banning for our own timing would cost us good peers. Assisted-by: Claude:claude-opus-5 claude-code
Non-validators served ledger snapshots unconditionally, with no way to turn it off. That is exactly backwards for the nodes whose exposure to arbitrary peers is highest -- public RPC endpoints and bootnodes -- which are non-validators and so had no opt-out at all. Add the counterpart to --serve-warp-ledger-sync. Passing both is rejected by clap rather than resolved by silent precedence, so an operator who sets both is told to pick one instead of quietly getting whichever the code happened to check first. Serving off still leaves the protocol registered as Outbound, so the node can warp-sync as a client. Assisted-by: Claude:claude-opus-5 claude-code
required_chunk_len bounds the *number* of requests spent on one peer, not the wall clock. With ~1 MiB ranges and the protocol's 40s request timeout, a peer answering each range just inside that timeout holds arena recovery open for roughly thirteen hours while remaining, technically, responsive. Recovery is sequential across peers and the block-import gate is held throughout, so this is a stall of the whole node, not just of one fetch. Add the time bound the size bound cannot give: - a per-chunk timeout well under the protocol timeout, so a peer that simply goes quiet is dropped in seconds rather than tying up a slot for 40s - a whole-transfer deadline derived from the advertised size and a minimum throughput, so a large arena over a slow link still completes but a peer that stops making real progress loses its turn The first range keeps the full protocol timeout: on a cold server memo it legitimately pays for the entire arena serialization. The budget is measured from the start of the transfer, so a peer cannot buy extra time by being slow to answer that first request. Assisted-by: Claude:claude-opus-5 claude-code
The change file described only --serve-warp-ledger-sync. Note the opt-out and the abuse bounds on both sides, since both are operator-visible: serving can now refuse a peer, and recovery can now give up on one. Assisted-by: Claude:claude-opus-5 claude-code
A warp proof is built from the blocks carrying GRANDPA justifications at authority-set changes. Under `--blocks-pruning <N>` those bodies are pruned like any other, so a block-pruned node silently stops being a viable warp-sync server -- it still advertises the protocol, it just can no longer answer. substrate provides `GrandpaPruningFilter` for exactly this, and `DatabaseSettings::pruning_filters` to install it. Midnight wired neither, so the field was left empty. Not a live fault today, because `--blocks-pruning` defaults to `archive-canonical` and nothing is pruned -- but this PR is what makes Midnight a warp-serving network, so the trap is newly reachable by any operator reclaiming disk. Installed only when this node serves, so a node that never serves does not retain blocks it has no use for. That makes the decision a `new_partial` input, which also removes the duplicated serve/no-serve expression: it is now computed once in `new_full` and shared with the protocol registration. Note the filter only protects blocks pruned from here on. Enabling serving later on a node that has already run block-pruned leaves holes in its justification history that this cannot repair -- see the comment at the call site. Assisted-by: Claude:claude-opus-5 claude-code
e39b7de to
b52d224
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b52d224f08
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let spent = self.serializations_per_peer.get(peer).copied().unwrap_or(0); | ||
| if spent >= MAX_SERIALIZATIONS_PER_PEER { | ||
| return Err(HandleError::TargetCycling); | ||
| } | ||
| self.serializations_per_peer.put(*peer, spent + 1); |
There was a problem hiding this comment.
When this server is handling more than SNAPSHOT_CACHE_ENTRIES distinct warp targets concurrently, an honest peer’s snapshot can be evicted between range requests. Its next range for the same target then falls through this path and increments serializations_per_peer; after four such cache misses the peer gets TargetCycling even though it never changed targets. That can make busy non-validator snapshot servers reject legitimate warp clients, so the budget should track distinct targets per peer or pin active transfers instead of charging every cache miss.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
PR Review Summary
PR: #2014 - warp sync hardening
Plan: work package README
Activities: Start Work Package, Lean-Coding Audit, Post-Implementation Review, Strategic Review
Reports: Prior feedback triage, Code review, Test suite review, Strategic review
Date: 2026-08-14
Executive Summary
The abuse-resistance design is carefully reasoned and its rationale is recorded to an unusually high standard, but two of its four bounds do not hold as written: the replay penalty fires against this PR's own retry path and permanently bans the peers a joining node depends on, and justification retention is gated on a flag that does not govern warp-proof serving. Two required checks are red at b52d224, and the ten added tests execute none of the changed behaviour.
Overall Rating: Request Changes
Prior Feedback Triage
Disposition of every prior comment and review on the PR, determined before independent analysis.
| # | Finding | Author | Disposition |
|---|---|---|---|
| 1 | Seven CI jobs fail on head e39b7de | datadog-official[bot] | Superseded |
| 2 | Server replay tracking bans legitimate recovery retries | chatgpt-codex-connector[bot] | Confirmed |
| 3 | Deadline subtraction re-reads the clock after the budget check | chatgpt-codex-connector[bot] | Confirmed |
| 4 | Codex review container listing automated suggestions | chatgpt-codex-connector[bot] | Refuted |
Code Review Findings
| # | @ | Finding | Severity |
|---|---|---|---|
| 1 | > | A joining node's third retry against a peer is refused and the peer banned | High |
| 2 | > | Justification retention gated on ledger-sync serving while warp-proof serving is unconditional | High |
| 3 | > | Replay LRU sized for substrate's key density, not this protocol's chunk count | Medium |
| 4 | > | Serve path never checks the raw-size ceiling every client enforces | Medium |
| 5 | > | Per-chunk timeout re-reads the clock after the budget guard | Medium |
| 6 | > | Per-peer serialization budget never decays and resets on eviction | Medium |
| 7 | > | Changelog fragment links PR 1650, so check-changes fails | High |
| 8 | > | A key whose serve always fails stays First, so replays go uncounted | Low |
Test Review Findings
| # | @ | Finding | Severity |
|---|---|---|---|
| 1 | > | No test drives a request through handle_request or blob_for | High |
| 2 | > | No test drives fetch_blob_from or either new client error | High |
| 3 | > | Three tests assert compile-time constants behind an allow attribute | Low |
| 4 | > | The three-boolean serve/no-serve policy expression has no test | Medium |
| 5 | > | A budget assertion restates the production formula it tests | Low |
| 6 | > | A test exercises hand-written trait impls rather than handler behaviour | Low |
| 7 | > | The round-trip harness cannot reach the retry path by construction | Medium |
Strategic Review
| # | @ | Finding | Severity |
|---|---|---|---|
| 1 | > | Submission checklist unfilled and its DCO item false, so DCO blocks merge | High |
| 2 | > | A CLI opt-out feature rides inside a hardening fix with no requirement | Medium |
| 3 | > | This PR's release note is appended to merged PR 1650's fragment | Medium |
| 4 | > | command.rs enters the diff only to pass a parameter CR-2 would delete | Low |
What This Change Gets Right
- The serialization budget is charged before the work, so an over-budget peer is refused rather than served and then penalised — >
- Cheap rejections are ordered first, so a header lookup for an unknown or unfinalized block never consumes budget — >
reputation_changematches exhaustively and returnsNonefor every failure an honest peer can reach, so a future variant must state which it is — >- The transfer budget is derived against the compressed ceiling and pinned there by a test, where the raw ceiling would have understated a peer's claim — >
Action Items
Must Address (Blocking):
- Point the changelog fragment's
PR:line at 2014 to clearcheck-changes(CR-7) - Sign off the commit range and fill the submission checklist to clear DCO (SR-1)
- Re-key replay detection on a value the client advances, so an honest retry is not banned (CR-1, PF-2)
- Read the transfer clock once and fail with
TransferTooSlowwhen no time remains (CR-5, PF-3)
Should Address (Recommended):
- Install the GRANDPA pruning filter wherever the warp-proof provider is registered (CR-2)
- Drive
handle_requestandblob_forin a test, including a repeated offset-0 request (TR-1) - Drive
fetch_blob_fromagainst a stub peer under a paused tokio clock (TR-2)
Could Address (Suggested):
- Size the replay LRU on this protocol's chunks-per-transfer, or track replay per transfer (CR-3)
- Refuse and warn when a serialized arena exceeds
MAX_LEDGER_SYNC_RAW_BYTES(CR-4) - Give the per-peer serialization budget a decay window or a reset on disconnect (CR-6)
- Lift the serve/no-serve expression to a named function and table-test all eight combinations (TR-4)
- Route the round-trip harness through
handle_requestandfetch_blob_from(TR-7) - State in the PR body why an opt-out flag is needed given the per-peer budget (SR-2)
- Give this PR its own release-note fragment under
changes/changed/(SR-3)
Nice to Have (Optional):
- Count attempts on the
Firstarm, or record why an unfulfilled key is not counted (CR-8) - Move the constant assertions to
const _: () = assert!(…);beside their constants (TR-3) - Replace the restated budget formula with a literal expectation (TR-5)
- Fold the key-distinctness intent into the
handle_requesttest (TR-6) - Drop
command.rsfrom the diff oncenew_partialno longer takes the flag (SR-4)
Severity Definitions
| Severity | Merge Blocker? | Expectation |
|---|---|---|
| Critical | Yes | Must fix before merge |
| High | Recommended | Should fix before merge |
| Medium | No | Can be follow-up PR |
| Low | No | Nice to have |
Posted by an automated review agent on behalf of @m2ux. The recommendation reflects an independent re-verification at head b52d224; the maintainers retain full discretion over disposition.
2 participants
Overview
Warp sync hardening. See individual commits.
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
Please describe any additional testing aside from CI:
🔱 Fork Strategy
Links