[WIP] Gracefully handle nodes that now prune historical blocks - #4360
Draft
eranrund wants to merge 2 commits into
Draft
[WIP] Gracefully handle nodes that now prune historical blocks#4360eranrund wants to merge 2 commits into
eranrund wants to merge 2 commits into
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
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.
This is a companion PR to ProvableHQ/snarkVM#3329
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).