Repository navigation
Conversation
Instead, return an empty blocks array which the requesting peer will interpret as "the peer cannot provide the requested blocks".
…er can serve them
vicsn
left a comment
vicsn
left a comment
There was a problem hiding this comment.
Great to see!
The choice of shutdown over in-process CDN fetching was deliberate.
I think I agree, but can you check that the current CDN-fetching is robust enough? It should not fail and switch to P2P sync on the first fetching error.
Some on AI findings:
A single peer can force any client to shut down by advertising a fabricated tip
We should aim for the simplest fix for this. needed_blocks_below_all_peer_floors decides "no peer can serve me, this is unrecoverable" purely from locators.latest_locator_height(), which is an unverified peer claim:
if peer_tip > height {
any_peer_ahead = true;
if assumed_peer_floor(peer_tip) <= height.saturating_add(1) {
return false;
}
}
Block locators are only checked for structural validity — BlockLocators::ensure_is_valid requires checkpoints at multiples of CHECKPOINT_INTERVAL (10,000) and consistency between the last checkpoint and the last recent height. Nothing bounds the claimed height, and update_peer_locators explicitly documents that it does not cross-check against other peers. An attacker copies the honest checkpoint chain up to the real tip and appends ~10 fabricated checkpoints plus 100 fabricated recents.
The consequences chain together:
set_greatest_peer_height takes the max over all peers, and MAX_BLOCKS_BEHIND is 1, so one liar flips the node to Syncing — satisfying the !self.is_block_synced() guard.
The liar's assumed_peer_floor sits ~99,000 blocks above us, so it is excluded from candidate_locators; honest peers at exactly our height fail the peer_tip > current_height test. No candidates, no batches, no outstanding requests.
any_peer_ahead is true and no peer clears the floor test, so unservable_since is armed. After STUCK_SYNC_SHUTDOWN_THRESHOLD (5 min) the node calls stop_with_failure() and exits non-zero.
In a healthy network the timer resets whenever an honest peer advertises a tip strictly above ours, which happens every couple of seconds, so this is hard to sustain. The dangerous case is exactly the one you least want it in: a network halt or stall, when no honest peer is ever ahead. Then one connected peer can walk every client off the network in five minutes, and because the exit code is non-zero, supervisors restart into CDN sync and the attacker repeats it — a sustained crash loop. Clients accept inbound connections from anyone, so this needs no privileges. Validators are exposed too (they call prepare_block_requests through the BFT sync loop), but only to a Byzantine committee member, since gateway locators come from authorized validators.
The PR description acknowledges this ("a lying peer could push idle nodes to exit") and argues the network will never halt meaningfully. I don't think that's a safe assumption for a mechanism whose failure mode is mass shutdown. Concrete hardening options: require the stuck condition to be corroborated by a quorum or a majority of connected peers rather than any single one; derive the floor from the median or minimum of peer tips ahead of us instead of each peer's own claim; and require the node to have made no sync progress at all for the whole window, not merely that the flag stayed armed.
Shutdown-as-recovery has no backoff and no bound on repeats
Indeed instead of trying and waiting for 5 minutes could we fail immediately and loudly when:
- The node is far behind, and CDN sync failed.
- The node is far behind, and have --nocdn turned on.
Pin snarkVM to eranrund/snarkVM agent/authority-prune-v2 (76cc738a7), which is staging plus authority pruning. That rev pins proptest =1.12, so the lock bumps proptest and its transitive deps along with the snarkvm entries.
The constant moved from 100_000 to 500_000 in snarkVM; the literals no longer exercised the boundaries they described.
A peer that advanced past its advertised tip answers with an empty response, which is re-issued; the stuck detector's threshold covers the rest.
…fallback With --nocdn, or after a failed startup CDN sync, waiting out the shutdown threshold changes nothing, so shut down at once and name the recovery. The CDN worker now reports a sync that stopped short instead of masking it as success; the callers ignored that result before.
Only with the prune-authority-data feature; otherwise unchanged. Visible on /v2, since the v1 error middleware still turns every error into a 500.
Block bundles were already retried; the initial latest.json fetch was not, so a single transient failure skipped the CDN sync entirely.
A single peer advertising a fabricated far-ahead tip is a minority among honest peers at this node's height, so it no longer arms the detector. The threshold now measures a window without any ledger progress; progress restarts it.
The snarkVM pin forces proptest 1.12 (rand 0.10), where RngCore no longer provides next_u64.
Share the CDN wait and stuck shutdown between client and validator, compute the retention floor once in snarkos-node-sync, derive the stuck rule's preconditions instead of passing them in, check REST retention inside the blocking task and after the block cache, shorten the CDN height backoff, and cut the comments.
Evaluate the stuck condition on every query so a window armed earlier cannot outlive it; require the router's minimum peer count before a majority counts; treat a CDN behind the node as nothing to fetch rather than a failed sync; include heights above genesis when a /blocks range starts at 0; count the CDN height attempts as the constant says.
Check the re-issue path's peers against the first block it re-requests, not the one after; measure the stuck condition at the sync height that request generation uses; skip the REST retention check for an empty range; describe the empty response as it is today; do not rely on clock resolution in a test.
On a pruning node, requests below the retention window are expected.
| git = "https://github.com/ProvableHQ/snarkVM.git" | ||
| rev = "9796eb19dee4fd2c671be9b3a1322eda62586991" #staging | ||
| git = "https://github.com/eranrund/snarkVM.git" | ||
| rev = "fd64f0bbb76829918bb07cbaa2e004a345dc8cc7" # eran/authority-prune |
There was a problem hiding this comment.
Remember to undo once the snarkVM PR is merged
find_sync_peers reports the network tip to BFT sync and to provers, whose ledger height is 0; filtering it by retention left provers without puzzle requests and showed far-behind validators a tip of 0.
Treat a CDN with no blocks past the node as no fallback, so a node stuck below every peer's floor fails fast with the snapshot instruction; log the re-issue gap at debug and describe it accurately; name the actual CDN-disabled cause. Simplify: REST uses snarkVM's oldest_retained_authority_height and carries the parsed height; one helper answers block requests empty when the ledger cannot serve them; the stuck tests are table-driven and the lifecycle test checks a duration that can fail; connect_test_peer moves to tests/common; the mock ledger returns to staging and the malformed-request test is dropped.
Count only requests still awaiting a response, so answered blocks waiting on an unservable gap no longer keep the detector from arming; report an empty CDN range as nothing loaded so it is classified as Behind; note that the 410 shows only on /v2.
An answer for the next block means sync can still progress, so it keeps the detector from arming. Skip the hash-to-height lookup on nodes that retain every block, and keep the test that a malformed block request still disconnects.
…d tidy the REST check
…e-queue every unsent range
… height prepare_block_requests dropped peers assumed to have pruned retention_height()+1, but construct_requests starts at the first height not in the ledger and not already requested, which is far higher during catch-up. Compute that height once and use it for both.
The failure height is the start height when no block was loaded, for example when fetching the CDN height failed, so say the sync failed at or before it.
The early failures in load_blocks returned the start height, a block never fetched, while the others return the last completed height. Make them agree and log the failure as stopping after that block.
Every failure now carries the last completed height, so the old guard was always true.
The bundle closure owned a copy of current_height, so a failing insert reported the pre-bundle height, failed the integrity check in the sync worker, and masked the real error.
The sync worker warned and wait_for_cdn warned the same chain again; only wait_for_cdn consumes the outcome, so it keeps the one warning.
…ight The obsolete-pruning loop used the raw sync height while new requests start from retention_height(), so with the sync height behind the ledger a failed batch wholly below the ledger was kept and re-issued from above the ledger every round, bypassing the new-request caps. Both paths now use retention_height(), the re-issue is sized from its shifted start, a batch already requested in full is dropped, and the re-request path filters peers by the same tip height as the new-request path.
…tion height The new-request start passed retention_height()+1 and the helper maxed it with ledger+1 again. The helper now only skips outstanding requests; both call sites in prepare_block_requests pass a height above current_height (the retention height), which keeps the invariant visible where the start is chosen.
The integrity-check branches replaced the load error, which the worker no longer logs itself, so the one warning lost the cause.
The serve filter only checked a peer's floor, so a peer whose tip lies between the current height and the batch start joined the cohort and pulled the common ancestor below the start, stalling the batch.
A batch whose construct_requests returned nothing was still pushed as an empty request list, so a round could return only empty lists and skip the new-request path with capacity free. Its entries stay queued.
The heights before the re-issue start are already outstanding; leaving them queued made the peer's later timeout or disconnect re-insert them and warn "already exists in the failed requests map" once per height.
…batch The re-request path returned early when no connected peer could serve a batch, so a batch above every peer's tip blocked new requests for the blocks those peers do have, and the stuck detector never armed.
|
Closing in favor of #4596 |
This is a companion PR to aleo-labs-protocol/snarkVM#3329
The main benefits of the smaller state, is that initial participation and recovery will be much faster, making the network more robust.
It contains two major changes, presented as individual commits that hopefully make it easier to review/reason about.
Background
The companion snarkVM PR prunes authority data (
authority_map+certificate_map) for blocks older thanAUTHORITY_RETENTION_BLOCKS(100,000 blocks, roughly three days at 2.5s block times). Per review discussion there, peers do not advertise whether they keep full history. Instead the protocol contract becomes implicit and uniform: every peer serves[tip − AUTHORITY_RETENTION_BLOCKS, tip], and anything older is fetched from the trusted CDN (automatic on startup) or from a ledger snapshot (manual).That design needs two things from snarkOS, which today does neither: a node must be able to decline to serve blocks it no longer has without punishing the requester, and a node must avoid asking peers for blocks they cannot possibly still hold - and know what to do when nobody can serve it.
Commit 1:
Don't treat get_blocks as failure and penalize requesting peerToday, when a node fails to load the blocks in a
BlockRequest, the handler returnsfalse/Errand the inbound dispatcher turns that into "peer sent an invalid block request", disconnecting the requester for a protocol violation. The failure is attributed to the wrong side: with pruning, an honest node would disconnect every peer that legitimately asks for a range it has pruned (and even without pruning, it's not the requesters fault thatget_blocksfailed...).This commit makes an unavailable range a normal, benign outcome:
get_blocksfailure now logs a warning and replies with an emptyBlockResponserather than failing the message. An empty response is expressible in the existing wire format (DataBlocksserializes au8count), so there is no format change and no new message variant.DataBlocks::ensure_response_is_well_formednow accepts an empty response, documented as the peer's explicit "I cannot serve this range" signal. Ordering and range checks still apply to non-empty responses.InsertBlockResponseError::EmptyBlockResponseis now classified as benign. The existing machinery then does the right thing without further changes: the peer's outstanding requests are removed and marked for re-issue to other peers, so failover is immediate rather than waiting out the 60s request timeout.Compatibility. No wire format change. Against an un-upgraded requester, an empty response is rejected and that peer disconnects the server and retries elsewhere - noisier, but assumed to be better than today, where the server disconnects an innocent requester. Un-upgraded servers never emit empty responses, so their behavior is unchanged.
Commit 2:
Don't request blocks that peers have pruned, and shut down when no peer can serve themWith commit 1 alone, a node far behind the network would request pruned ranges, receive empty responses, re-issue to the same peers, and churn indefinitely. This commit "teaches" the requester the retention contract and gives it an exit.
find_sync_peers_innernow skips peers whose assumed pruning floor lies above the next block we need. The floor is derived from the peer's advertised tip:tip − (AUTHORITY_RETENTION_BLOCKS − RETENTION_SLACK). The slack (1,000 blocks, chosen arbitrarily) exists because an advertised tip is only a snapshot - by the time our request arrives the peer may have advanced and pruned correspondingly, so we cannot assume it serves exactlytip − retention.BlockSyncrecords when peers ahead of us exist but every one of them has pruned what we need, exposed assync_stuck_below_peer_floors(). This state cannot resolve itself - peers prune further with every block they produce, so the gap only widens. It is deliberately distinguished from simply having no useful peers (not yet connected, isolated), which is transient and does not count.SignalHandler::stop_with_failure(), which causes the process to exit non-zero so a supervisor restarts it - at which point the existing startup CDN sync closes the gap automatically. Restoring a snapshot is the manual alternative; nodes run with--nocdnmust use one.The choice of shutdown over in-process CDN fetching was deliberate. Mid-run CDN sync would require reworking the one-shot
CdnBlockSynclifecycle, re-routing its ledger writes through the advancement lock it currently bypasses, and adding a pause/resume surface toBlockSync- and it would be difficult to cover validators, where a mid-run CDN jump would strand BFT/DAG state. Restarting into the existing, well-tested startup path achieves the same outcome. This is a thing we can improve upon if we decide to invest resources in it, but in reality I think we are not going to experience nodes being down for almost 3 days and then suddenly coming back to life and trying to sync. If a node fell that far behind, it might as well restart. This keeps the code and potential edgecases smaller and easier to reason about. Its also worth emphasizing that CDN sync takes place every time a node restarts, so this is standard practice.Misc notes
.ci/test_devnet.sh). We'll need to make the 100k prune block window configurable (maybe via a build env var), and we'd need a way to trigger a node falling far behind and aborting.GET /block/{h}404s;GET /blocksfails the whole range) - a separate follow-up. I'm considering using HTTP code 410 Gone (Gone client error response status code indicates that the target resource is no longer available at the origin server) to differentiate between a block that never existed and a block that used to exist but are no longer available due to pruning.Another thing worth mentioning is that this shifts the entire network (after an upgrade) to assuming no node can actually provide post-prune-window block, even though some might (due to having the
historyfeature enabled). That was a deliberate decision that favors simplicity over having each peer return different ranges it can fulfill (explicitly suggested in the companion PR).