perf(blockvalidation,legacy): overlap below-checkpoint blocks' UTXO work with a quick window, one-wave applies and a batcher-sized create fan-out - #1699
Draft
freemans13 wants to merge 206 commits into
Conversation
…sync During headers-first sync the node commits blocks strictly in order, so the oldest block it has asked for and not yet received gates everything behind it. Every block body is requested from the sync peer, so if that peer silently drops one getdata, sync simply stops. The only response available today is the 180-second sync-peer stall timer, which disconnects the peer and then throws away the whole downloaded header list, forcing a fresh getheaders round from someone else. One dropped request therefore costs three minutes of nothing plus a full header re-download. This tracks that block, and when it has been outstanding for longer than legacy_blockSlowFetchTimeout (20s by default) asks one additional connected peer for the same block, leaving the original request in place. Whichever copy lands first is processed normally; the other is discarded without the peer that sent it losing its connection. Racing is skipped when the silence is ours (local validation behind) or when the sync peer's connection is visibly still pulling bytes, which is what a peer part-way through a large block looks like. The 180-second backstop is untouched: a racer's delivery does not refresh the sync peer's last-block time, so a peer that never delivers anything is still rotated on exactly the same schedule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EzhFCE5xe4nyxujhf9wyfN
Adds two tests that fail if the frontier is never published by fetchHeaderBlocks, or never advanced by the block handler when the block arrives. Either omission would leave the race with nothing to act on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EzhFCE5xe4nyxujhf9wyfN
…rence Also adds the blank lines markdownlint wants around two existing headings in the same file, which the commit hook flags as soon as the file is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EzhFCE5xe4nyxujhf9wyfN
A peer gets disconnected when a block it is sending takes too long. Today that
deadline is a flat thirty minutes, whatever else is happening. svnode does not
use a fixed duration at all: it computes the ceiling from the chain's own block
interval, and widens it on two counts we ignore.
The first is whether we are catching up. At the chain tip a block should arrive
within seconds, so a tight ceiling is right and a peer that misses it really is
dead. During initial block download the same ceiling aborts perfectly healthy
transfers just short of completion, because historical blocks are large and our
own validation backpressure delays the read loop. The block is then re-fetched
in full from another peer, wasting every byte already received. svnode allows
six block intervals while catching up against one at the tip.
The second is how many peers we are downloading from. Pulling blocks from
several peers at once shares our downstream link between them, so each transfer
is honestly slower. Judging every peer against a single-peer deadline means the
more we parallelise, the more good peers we disconnect. svnode adds half a block
interval of patience for each other peer with a download in flight.
Concretely, svnode computes
nPowTargetSpacing * (timeoutBase + timeoutPerPeer * nOtherPeers) / 100
with timeoutBase 100% at the tip and 600% while catching up, and timeoutPerPeer
50%. On mainnet that is ten minutes at the tip, an hour while catching up, and
an hour and thirty-five minutes if eight peers are supplying us.
This change adopts that calculation. Three settings mirror svnode's parameters
of the same names, so the numbers can be compared directly against a reference
implementation rather than argued about from first principles.
What is deliberately NOT changed is the mechanism around it. Main already
extends a block deadline while throughput stays above 50 KiB/s and caps the
extension so a peer cannot dribble bytes forever - which is svnode's design too,
arrived at independently in bsv-blockchain#1030. That stays exactly as it is. All this does is
replace the flat cap on that extension with svnode's scaled one.
Only peers with a request genuinely outstanding are counted, so a peer cannot
buy itself patience by announcing blocks it never sends. A zero or negative
result falls back to the old constant rather than being taken at face value: a
configuration mistake must never produce a zero ceiling, which would disconnect
every peer the instant a block was requested.
Testing: the budget is asserted against svnode's arithmetic for the tip, the
catch-up, and the eight-peer cases, plus the fallbacks. Reverting the
calculation makes those fail behaviourally, showing 30m where the scaled value
is expected, rather than merely failing to compile. A separate test pins the two
callbacks into newPeerConfig, because without them the peer silently falls back
to the shortest ceiling and nothing else in the tree notices - deleting the
wiring left the whole package green before that test existed.
Limits worth stating. The defaults are svnode's, not measured on our own node.
The improvement this is meant to produce - fewer self-inflicted disconnects
during a long sync - is only observable in a soak, and is not covered by any
test here. The per-peer count is read at the moment the deadline is checked
rather than when the request was sent, so a peer that joins mid-transfer widens
the ceiling for a download already in progress; svnode has the same property.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EzhFCE5xe4nyxujhf9wyfN
…s review Three things came out of the review of the scaled block-download deadline. The overflow guard was incomplete. blockDownloadBudget checked that the percentage total was positive before multiplying it by the block interval, but not that the product was. A percentage large enough to overflow the duration wraps the result negative, which disconnects every peer just as surely as a zero would - the exact failure the original guard existed to prevent. The check now runs after the multiply as well, falling back to the old constant either way. blockDownloadBudget was being computed twice on the extend path, once for the decision and once for the log line. Beyond the wasted work, the two calls range every peer state independently and can disagree if the downloading-peer count changes between them, so the cap we logged was not necessarily the cap we applied. It is now computed once and reused. The rest is comment accuracy. MaxBlockDownloadTime is no longer the peer layer's normal ceiling, only its fallback and netsync's unscaled sync-peer rotation cap, so the places still describing it as the ceiling now say what it actually is - including the fact that netsync rotates at the flat value while the peer layer may wait longer during catch-up. The wiring test's local no longer shadows the package-level cfg global it swaps out, which made the restore hard to follow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016cdL2iwjBEtJh6XvgvR5hS
…sing its divisor PeersWithBlockDownloads was inserted between IsCurrent's doc comment and IsCurrent itself, so godoc attached "IsCurrent returns whether the sync manager believes it is synced with the connected peers" to the new method and left IsCurrent with no documentation at all. The comment moves back down to the function it describes. The svnode formula quoted above blockDownloadBudget omitted the trailing / 100, which makes the numbers look a hundred times larger than they are and defeats the point of quoting it - comparing our arithmetic against the reference implementation. The settings documentation already had it right. Both were raised in review. Note that neither revive nor golangci-lint flags the displaced godoc, contrary to what the review suggested; it is a documentation defect, not a lint one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016cdL2iwjBEtJh6XvgvR5hS
…ount The overflow guard added in the previous commit was incomplete, and the way it was incomplete is worth stating plainly: it checked the sign of the product after the multiply, on the assumption that an overflowing product wraps negative. It does not always. Two's-complement wraparound is modular, so a large enough percentage wraps all the way round into small POSITIVE territory. 30744574% of a ten-minute interval produces 18446744400000000000ns, which is past 2^64 and lands on 326s, leaving a 3.26 second ceiling after the divide by one hundred. That is greater than zero, so the sign check waves it through, and a three-second ceiling disconnects every peer within a single stall tick - the exact failure the guard was written to prevent, reached by a different input. The bound is now on the multiply rather than its result: reject any total greater than MaxInt64 divided by the interval. Both operands are positive at that point, so that division is the exact largest total that cannot overflow, and nothing computable is refused - the test pins 15372286%, the largest value that fits, and asserts it is still honoured rather than lumped in with the rejects. The post-multiply zero check stays, because a chain whose interval is shorter than the percentage divisor can still floor to zero without overflowing. Removing the new bound makes only the positive-overflow case fail, which is the point: the negative case still passes under the old sign check, so the new test is testing the new hole rather than re-testing the old one. Separately, SyncManager.PeersWithBlockDownloads had no test at all. It is the function that drives the whole per-peer widening - its return value is multiplied by the per-peer percentage - so counting an idle peer hands everyone patience they did not earn, and missing a downloading one disconnects peers that are slow only because we are sharing our own downstream link between them. It now has one, covering the counting itself, a peer state with no requestedBlocks map, a nil state, and a map already stopped by peer teardown, since the count is taken from a stall handler on another goroutine while handleDonePeerMsg may be tearing the peer down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016cdL2iwjBEtJh6XvgvR5hS
Brings the stalled-block race together with the download deadlines it depends on. Asking a second peer for the block that is holding up sync puts more peers on the wire at once, which is exactly the situation the per-peer deadline widening in this branch exists to survive: several concurrent transfers share one downlink, so each is honestly slower and must not be judged against a single-peer ceiling. The only conflicts were in the two settings files, where both sides append to the same struct and the same NewSettings block. Both sets are kept: the three blockDownloadTimeout percentages, then maxBlockParallelFetch and blockSlowFetchTimeout, then the pre-existing upnp field. No logic was dropped from either side. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… reads it The headers-first header list has been shared between three goroutines with no synchronisation at all. blockHandler dispatches a fresh goroutine for every headers message, so two headers messages can push into the same container/list at once; the block-queue consumer walks and removes from the front of that list inside handleBlockMsg; and fetchHeaderBlocks, which both of those call, walks it and re-anchors startHeader. container/list is not goroutine-safe, and the SyncManager had mutexes for syncPeer, inFlightBlocks and txAnnounce but none for headerList, startHeader or nextCheckpoint. A new test that drives those three entry points concurrently makes the race detector fire on every run. This adds SyncManager.headerMu as the single owner of those three fields and takes it at every touch point in manager.go and frontier_race.go. Two rules are written into the field comment and followed throughout. Lock ordering is headerMu -> frontierMu -> peerStates, with headerMu outermost; nothing takes it while already holding either of the others. And nothing runs under it that can block for an unbounded time: no getdata, getheaders, getblocks or disconnect (a peer's output queue is buffered but finite), and no GetBestBlockHeader, which during initial sync can take minutes. That second rule is not achievable as a straight-line lock in handleHeadersMsg. Its empty-list recovery reads the back of the list, calls GetBestBlockHeader, resets the header state, then re-reads the back. The lock has to be dropped across that client call — and once it is dropped, what was read before it is no longer true. Resetting unconditionally on the way back is a real bug today, not just a theoretical one: another headers message can recover the state and land a batch of headers while the first handler is parked, and the reset throws all of them away. resetHeaderStateIfEmpty re-checks the emptiness under the lock and does nothing if someone else already recovered. One deliberate exception to the rule is carried in fetchHeaderBlocks: the haveInventory lookup stays inside the locked walk. It is a bounded per-hash header lookup capped by the dynamic in-flight max (20, falling to 1 for multi-GB blocks), not the unbounded call the rule is aimed at, and the two goroutines that would contend for headerMu are the same two that call fetchHeaderBlocks, so it adds almost no serialisation. The compliant alternative — snapshot the hashes, do the lookups unlocked, re-walk under the lock and re-anchor startHeader only if it has not moved — is around forty lines of new logic in the hottest sync path and becomes nearly free once the header list is indexed by hash, so it is recorded at the call site as a follow-up. Two smaller behaviour notes. In handleBlockMsg's pipeline top-up, startHeader is now sampled a few microseconds before the in-flight count rather than in the same expression; this is harmless because fetchHeaderBlocks re-checks it under the lock before doing anything. And the three disconnect paths inside handleHeadersMsg's header loop now collect a reason and break, with the disconnect fired after the unlock, so no peer send happens under the lock. publishFrontier is split into a self-locking wrapper and a publishFrontierLocked body, because handleBlockMsg already holds headerMu when it publishes and sync.Mutex is not reentrant. The frontier is still published under frontierMu from the header-list touch points, which is what keeps the five-second race timer off the header list entirely, so it never needs headerMu and the lock order stays one-directional. Testing. TestHeaderList_ConcurrentHeadersAndBlocksDoNotCorruptTheList drives all three goroutines through the manager's own entry points; before the change `go test -race -count=5` reports data races in container/list PushBack vs Front/Remove and on the startHeader word, and after it the same command is clean. This is a probabilistic detector test — it fired on every one of five runs, but that is not a guarantee. TestHandleHeadersMsg_RecoveryDoesNotWipeHeadersAddedWhileWeWereWaiting is a plain behavioural failure before the change (header list length 1 instead of 3) and needs no race detector. TestHandleHeadersMsg_DoesNotHoldTheHeaderLockAcrossGetBestBlockHeader cannot be red beforehand, because there is no lock to hold; it was proved instead by holding headerMu across the client call, which makes it time out. The existing frontier tests were proved to catch the reentrancy trap the same way: calling the self-locking publishFrontier from inside the locked region deadlocks TestHandleBlockMsg_AdvancesTheFrontier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Finding a header in the headers-first list means walking it, which is fine while nothing needs to do that often. The multi-peer block scheduler coming later needs to answer "where is this hash in the list" on every arriving block, and a walk of up to a full checkpoint interval per block is not a thing to build on. So the list now carries a companion map from block hash to list element, guarded by the headerMu added in the previous commit. The whole risk in a change like this is a missed maintenance point, because a stale entry does not fail loudly: it hands back an element that is no longer in any list. There are six places the list changes and all six are now wired — the wipe and the anchor push in resetHeaderStateLocked, the front removal in handleBlockMsg, the push in handleHeadersMsg, the front removal on the checkpoint branch, and the wipe on the final-checkpoint transition out of headers-first mode. That last one is the one that is easy to miss, and missing it is the worst of the six: it strands up to a whole checkpoint interval of entries, about 43,000 on mainnet, permanently. To give it a name and one place to live, the three lines of handleBlockMsg's "reached the final checkpoint" branch are now a leaveHeadersFirstMode method. It has exactly one production caller and the test drives it directly, because reaching that branch through handleBlockMsg needs a block to pass all of HandleBlockDirect, which needs the full validation stack. Duplicate hashes are handled deliberately rather than by accident. The list tolerates the same hash twice and a map cannot, so indexing is last-write-wins and removal only deletes when the entry still points at the element being removed. Those two rules only make sense as a pair and are documented as such. Two incidental changes come out of this. The checkpoint branch used to read sm.headerList.Remove(sm.headerList.Front()) with no nil guard, which would have panicked on an empty list; touching the line to unindex it forced a guard in, so that latent panic is gone. And the map is allocated lazily on first insert, matching the nil-guard style already used for racedBlocks and blockFailureBackoff, so the many struct-literal SyncManagers in the tests keep working without being touched. Testing: four new tests in header_index_test.go. The invariant test walks the list after each operation and requires exactly one index entry per distinct hash, each resolving to the element that actually holds it — a length check alone would not catch a detached entry. Each of the six maintenance points was removed one at a time and the suite shown failing for each, and the removal identity check was replaced with a plain delete and the duplicate test shown failing. In particular the final-checkpoint clear was left out while the other five were wired, which is how that test was proved red before it was made green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…st maps Two separate expiring maps used to record which blocks we were waiting for: a global one with a sixty-second lifetime, and one per peer with a sixty-minute lifetime. Between them they could only ever express a single owner per block, which stopped being true the moment the frontier race landed — when the sync peer goes quiet we deliberately ask a second peer for the same block, and the race had to smuggle that second owner in by reaching into the other peer's map. They were also not doing the job they claimed. clearRequestedState, the function that runs when a peer is lost, called Stop() on both maps. Stop closes the cleanup goroutine's channel and never touches the entries, so its documented promise — "so that they will be fetched from elsewhere next time we get an inv" — was never implemented. Worse, the per-peer map was garbage the moment the peer was dropped from peerStates, so clearing it was never the point: the entries that mattered were the departing peer's entries in the global map, and that map was never consulted at all. A block owed by a peer that had gone stayed owed forever, and was never asked for again. What changed: - New blockDownloadTracker (block_download_tracker.go) holding the ownership as (block, peer) pairs in both directions, so several peers can legitimately owe the same block and a peer's own outstanding count stays cheap to read. It has no background goroutine, which is what makes the Stop()-instead-of-Clear() trap structurally impossible to repeat: there is nothing to start, nothing to stop, and nothing that can quietly stop working. - Both old lifetimes survive as the two questions they always were. RequestedWithin(blockRequestRetryInterval) answers "is it worth asking somebody for this again", at sixty seconds. HasOwner answers "does this peer owe us this block", at sixty minutes. They must not be collapsed: one window at sixty minutes stalls sync after a lost getdata, and one at sixty seconds disconnects an honest peer for delivering a large block a minute late. - clearRequestedState now takes the peer and calls ClearPeer, so a departing peer's blocks are genuinely released. It also calls Clear before Stop on the transaction map, so that map's own documented behaviour becomes true too. - Assignments age out on their own. That backstop is load-bearing rather than belt-and-braces: handleDonePeerMsg returns early for any peer that is not registered in peerStates, which includes the stream sub-peers a BlockPriority association resolves through, so ClearPeer is not guaranteed to run for every peer that goes away. Two deliberate behaviour changes, neither of them silent: 1. What counts as "in flight" is now honest. ExpiringMap.Len counted entries that had already passed their lifetime but had not been swept, and the sweep only ran on a ticker whose period equalled that lifetime, so a dead entry could inflate the count for up to two full lifetimes. Both readers of that number were making real decisions with it: it widens every peer's block download deadline, and it caps how much fetchHeaderBlocks asks for next — a cap that falls to a single block once blocks get large, at which point one dead entry stopped all fetching. Aged-out assignments no longer count. 2. Delivering a block clears only the delivering peer's obligation, not the block. The spec for this change said to drop the block outright, but that is not what the two maps did: the global entry went and the delivering peer's entry went, while any other peer we had also asked kept its entry, and so kept its pass on the copy still travelling towards us. Dropping the block outright would disconnect that peer for answering a question we asked it. Keeping its entry can delay a re-request, but only until the retry window expires, which is at most sixty seconds. frontierRaceTarget no longer returns the target's peerSyncState: it existed only so the race could write into that peer's request map, and that write is now a call on the ledger. Tests: the two genuinely new failures this fixes are pinned first — TestClearRequestedState_ActuallyReleasesTheHash and TestPeersWithBlockDownloads_ExpiredAssignmentDoesNotInflateTheCount both fail against the old maps. Seven further mutations were run to prove the rest bite: reverting clearRequestedState to Stop-only, making the ledger single-owner, removing expiry entirely, collapsing the two windows into one, making HasOwner always true, evicting the newest instead of the oldest at the size cap, and using Remove instead of RemoveOwner on delivery. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The multi-peer block scheduler that follows this stage needs to answer "which of my peers claims to have block N" without going back to the wire. Today netsync has no such record: the only per-peer height lives on the Peer object as LastBlock(), which the scheduler cannot reach while walking peerStates, and which is the peer package's record rather than netsync's. peerSyncState gains a bestKnownHeight, seeded when the peer registers and raised — never lowered — from the three places we already learn a peer's height: - handleNewPeerMsg seeds it from the height the peer advertised during the handshake, so a freshly connected peer is never mistaken for one that has nothing. It has to be a statement rather than a struct-literal field because an atomic cannot be initialised in a literal. - handleBlockMsg raises it beside the existing UpdateLastBlockHeight when an accepted block tells us how high the delivering peer is. - handleInvMsg raises it beside the same call, reusing the header lookup the inv path already does for a block we recognise. - handleHeadersMsg tracks the top height of the batch as it links the headers and reports it after the header lock is released, so no peer state is touched under headerMu. The field is an atomic.Int32 because a *peerSyncState is shared by pointer across the blockHandler goroutine and the per-message inv and headers handlers, each on its own goroutine; a plain int32 is a data race the -race run catches. Writes go through noteBestKnownHeight, a compare-and-swap loop rather than a load-then-store: without the retry a peer part-way through reporting a lower height can land it on top of a higher one that arrived in between, and the record a scheduler reads would go backwards. Nothing consumes bestKnownHeight yet. Deliberately: startSync's candidate selection and the frontier race's peer filter are untouched, so this can be reverted cleanly if the scheduler is delayed. Two of the new tests pin existing behaviour rather than new. During IBD we drop block invs from anyone but the sync peer, and while headers-first is driving the download we note a block inv as known inventory and nothing else. Recording peer heights is exactly the kind of change that tempts someone to hoist a height lookup above the first of those guards and start processing inv traffic we drop on purpose, so both early returns now have a test standing over them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t to deliver Collapsing the two block request maps into one owner-keyed ledger quietly changed what startSync's clear means, and it started disconnecting honest peers. Before the ledger, startSync cleared the global re-request map, while the disconnect decision in handleBlockMsg read a completely separate per-peer map. Those were two different questions living in two different places, so clearing one could never revoke anybody's permission to deliver a block. Once both maps became a single ledger, the same clear began wiping the very record the disconnect gate consults, and the same substitution landed in BlockRequested, whose failure path in peer_server.go tears down the whole association rather than one stream. The stall recovery walks straight into it. The sync peer goes quiet on the frontier block, the frontier race asks an honest second peer for that block and records that we asked, the 180 second stall timer fires, and handleCheckSyncPeer to updateSyncPeer to startSync clears the ledger. The honest peer's copy — which we asked for — then arrives looking unrequested, and since nobody delivered the block there is no raced-to record to save it either. The peer loses its connection for answering our own question. The fix keeps the two questions apart rather than weakening the gate. "May this peer still deliver?" is ownership, judged against the hour-long assignment ceiling, and it now survives a sync peer change. "May we ask somebody else?" is the one-minute re-request window, and that is all a sync peer change moves. ForgetForRetry back-dates every assignment newer than the retry window to exactly that age, so RequestedWithin stops claiming somebody is on the job while ownership stands, one minute shorter than it was. Dropping the clear altogether would also have fixed the disconnect, since RequestedWithin reopens a block after a minute on its own, but it would have made every sync peer rotation wait up to a minute before re-requesting the block the departed peer never sent — a regression against the old behaviour, where that clear made the block immediately fetchable. Back-dating keeps both. Clear is removed rather than left unused. Its only remaining caller was this one, and a method whose single effect is now to revoke authorisation is an invitation to reintroduce the defect. Testing: two tests in startsync_authorisation_test.go drive the real startSync and the real handleBlockMsg back to back. Both were red before the fix — the honest peer was disconnected, and its ownership was gone — and are green after. A third test at the ledger level pins the two windows apart, including that reopening does not make an older assignment younger against the delivery ceiling. The existing tests that a peer we never asked IS disconnected for an unrequested block still pass, so the gate itself is unchanged. Full: go build, go vet, and go test -race -count=1 on ./services/legacy/netsync/... all pass; golangci-lint reports 0 issues on the package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s waiting on The download ledger's size cap did the opposite of what its own comment said. The comment explained that eviction is oldest-first so that "a burst of new announcements must never displace the frontier block we are actually waiting on", but the frontier is by definition the assignment we made earliest, so sorting ascending by time and removing from the front evicted it first. And dropping a record is not a bookkeeping detail: the disconnect decision in handleBlockMsg reads that record, so the block arrives looking unrequested and the peer that answered us loses its connection. A single peer could announce fifty thousand hashes and make us throw away the peers that were doing the real work. Rather than reverse the sort, the cap is now applied by refusing the newcomer. Add returns whether the ledger took the block, and the three callers that ask for blocks do not send the getdata when it did not. Reversing the sort to newest-first would have kept the frontier but aimed the same disconnect at whichever peer lost the eviction, and under a ledger filled by an earlier flood that victim is every honest peer that arrives afterwards; the harm is redirected, not removed. Refusing removes it, because a block we never ask for cannot come back unvouched for. Nothing already in the ledger is ever dropped by size pressure, so the frontier survives by construction rather than by sort order, and adding a second owner to a block already tracked never fails — which is what the frontier race needs, since it adds an owner and not a block. Expiry is still tried first. When the ledger is full and the block is new, the aged-out assignments are swept before the request is turned away, so a ledger full of dead entries cannot stop the node fetching blocks it is entitled to ask for. Only when every slot holds a live assignment does a request get held back, and then only until an arrival or the hour-long ceiling frees a slot; fetchHeaderBlocks leaves startHeader where it is, so the header is picked up again on the next pass rather than skipped. The honest cost is that a genuinely full ledger slows fetching instead of churning through it. Fifty thousand outstanding requests means sync has already stopped, and losing throughput while it drains is recoverable in a way that disconnecting the peer set is not. Tests: block_download_cap_test.go drives the flood at both levels — the ledger keeps the block it has waited longest for, and the peer that then delivers it keeps its connection — plus the two cases the cap must not break (another owner for a block already tracked, and expiry making room), and one that follows the refusal out to the wire: with a full ledger fetchHeaderBlocks sends no getdata and records nothing, and asks for the same headers once room appears. The old TestBlockDownloadTracker_EvictsTheOldestOverTheCap is gone; it asserted the defect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fetchHeaderBlocks asks the blockchain service whether we already have each candidate block, and it was doing so with headerMu held: up to twenty sequential gRPC round-trips on a context that carries no deadline, all inside the locked region. The comment there defended it as "a bounded per-hash header lookup", and it is bounded — in count, not in time, and time is the only property a goroutine waiting on the lock cares about. The block-queue consumer, the narrowest goroutine in the service, takes this same lock as its first act in headers-first mode, and both it and the headers-message goroutine call fetchHeaderBlocks, so a slow blockchain service serialised them through the header lock. On upstream/main fetchHeaderBlocks held no lock at all, so this stall was new to the branch, on the hot path of the very phase the branch exists to speed up. The lookups now happen with the lock released, which the hash index added earlier in this stage makes cheap. The walk runs in rounds: under headerMu, snapshot the next run of candidate hashes and the element startHeader points at; release the lock and ask the blockchain service about them; re-take the lock and commit. A round asks about at most the blocks still wanted, so blocks we turn out to already have cost a round slot but not a request, and a further round picks up the shortfall — which is what keeps the getdata contents identical to the old single-locked walk, where the loop simply carried on past them. The getdata is built up across rounds and sent outside the lock, so the existing publish-after-send ordering is unchanged. Commit is guarded by two checks, both needed. startHeader must still be the element the round was walked from, and the index must still resolve that element's hash to it: handleBlockMsg removes the front of the list, which is startHeader itself once the block queue has caught up, and a removed container/list element answers Next() with nil rather than admitting it is gone, so a pointer comparison alone cannot tell a detached element from the last one in the list. Each header is then re-identified by hash as it is committed. If anything moved, nothing at all is committed and the next tick redoes the round against the list as it then is, rather than acting on a stale reading that could ask for a block twice or step startHeader over one nobody asked for. Tests: the new lock test parks the blockchain lookup and times a competing header-list read, which waited the full two seconds before the fix and returns at once after it. The parity test pins the getdata contents, the ledger records and where startHeader is left for a non-concurrent pass, including two headers we already hold — one at the front and one in the middle — so the round structure cannot quietly change what is asked for. TestFetchHeaderBlocks_ PublishesTheFrontier now seeds through the index as every production push does, because the commit check reads it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oves The commit check that makes the unlocked inventory lookups safe has two halves, and until now neither was proved by a test. Both are, and both fail when their half is removed. The first covers the header state being reset mid-round, which is what happens when sync starts over from a different peer or leaves headers-first mode. container/list leaves a discarded element's links intact after Init(), so a commit that trusted its snapshot walked on through a list nobody can reach any more, asking for blocks from the abandoned chain and re-anchoring startHeader into it. The second covers the case a pointer comparison cannot see. handleBlockMsg removes the front of the header list when that block arrives, and the front is startHeader itself once the block queue has caught up, so startHeader still compares equal to the element the round was walked from. A removed element answers Next() with nil, so trusting the comparison meant asking again for the block that had just arrived and then advancing startHeader to nil, abandoning every header queued behind it. Consulting the index is what tells a detached element from the last one in the list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…here it can walk from Restructuring fetchHeaderBlocks to do its blockchain lookups with the header lock released means a round can come back to a list that has moved under it, and the commit correctly refuses to act on a stale reading. What it did not do was put sm.startHeader back somewhere usable, and in one of the two cases it was refusing, startHeader was left pointing at an element that is in no list at all: the block it named arrived while the lookups were in flight and handleBlockMsg took that element out of the front of the list. Nothing ever repaired it. There are only three writers of startHeader — the reset, this commit path, and handleHeadersMsg, which only assigns when it is already nil — so once detached it stayed detached. container/list clears a removed element's links, so a detached startHeader answers Next() with nil: every later round snapshotted exactly one header, was refused here for the same reason, and committed nothing, so the header list never drained again. The fallback that should have caught this was dead too, because handleBlockMsg reads a non-nil startHeader as "there is still work queued" and so never reached the getblocks recovery. The node then downloaded nothing until the 180 second stall detector fired, which rotates the sync peer — a repeating three-minute stall plus peer churn, on the hot path of initial block download. Before the restructure this healed by accident: the old single-locked walk read Next() straight off the detached element, got nil, and stored that, so the next pass found startHeader nil and the getblocks recovery took over. The snapshot and commit structure lost that, and this is a state the code reaches routinely — two rounds run concurrently whenever a block arrives while a headers message is being handled. The two refusals are now told apart, because they need different answers. When startHeader is no longer the anchor the round was walked from, some other round or a reset has already moved it on: it is live, it points into the list as it now is, and there is nothing to repair. When it is still the anchor but the hash index no longer resolves that anchor, the element is provably detached, and the walk is re-anchored on the front of the list. The front is the right place rather than nil. Headers only ever leave this list from the front, so an element detached while still being startHeader was the front when it went — which means nothing sat between the front and startHeader, and the new front is exactly the first header nobody has asked for yet. Nil would restore the old self-heal but throw away up to two thousand queued headers and pay a getblocks round trip to ask for them again. An empty list still leaves startHeader nil, which is what re-enables the getblocks recovery. The test asserts the end state that matters rather than the pointer: after a round is discarded because its anchor left the list, the headers still queued behind that block are actually requested on a later pass. It fails on the unfixed code with nothing fetched at all. Also corrects the Rule B comment on headerMu, which still described the haveInventory lookup as a named exception allowed to run under the lock. It is not one any more — that is the whole point of the round structure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r the walk starts from There are two places that wipe the header list. resetHeaderStateLocked clears the list, the hash index, the frontier and startHeader. leaveHeadersFirstMode, which runs once the final checkpoint is reached and sync switches to driving the rest of the chain with getblocks, cleared the first three and left startHeader pointing into the list it had just emptied. That is an inconsistency between two wipes of the same state, not a deliberate difference — the comment on the function even says every field the header list owns has to be cleared together, because inline versions of the wipe are exactly how one gets forgotten. The cost is that handleBlockMsg reads startHeader to answer "is there anything left to fetch?". A stale non-nil pointer answers yes forever, so the branch underneath it — not current, nothing in flight with this peer, so ask that peer for the next batch of blocks — became unreachable from the moment the final checkpoint was passed. That branch is the only thing that re-primes sync once everything has gone quiet, and losing it leaves the node sitting on an idle peer until the stall detector rotates it. The test drives the end state rather than the field: leave headers-first mode, let a block arrive with nothing else outstanding, and require that the peer is actually sent a getblocks. On the unfixed code no getblocks is ever sent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…testing Commit 53fa010 said both halves of the guard in commitHeaderCandidates were pinned by tests. That was an overclaim, and this corrects it: deleting the "sm.startHeader != anchor" half leaves the whole netsync package green. Only the hash-index half was actually covered, by the test where the anchor is removed from the list. The untested half is the one that matters most often. Two rounds of fetchHeaderBlocks walking from the same startHeader is ordinary rather than exotic — the block-queue consumer starts a round on every block that arrives and each headers message starts another — and when both come back from their unlocked lookups, the first commits and the second is holding a reading of the list that has been overtaken. Nothing was removed, so the index still resolves the anchor perfectly and only the startHeader comparison can tell. Without it the same run of blocks goes out to the same peer twice and startHeader is dragged back over headers already in flight. The new test needs no goroutines and no timing: it takes one snapshot and commits it twice, and requires the second commit to request nothing, add nothing to its getdata, and leave startHeader where the first commit put it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rver The legacy peer server built a blob.ConcurrentBlob over the temp store and stored it on the server struct, and nothing ever read it. It was set up with WithDeleteAt(10) and a "blocks" subdirectory — a delete-at-height of 10 is nonsense on a live chain, and the wrapper takes a process-wide RWMutex that it holds across the whole fetch, so anything routed through it would be serialised one block at a time. That matters now because the next commits make out-of-order block downloads work, and the obvious-looking place to put them is exactly this dead field. Removing it stops the wrong answer from looking available. The wrapper type and its tests stay where they are; only the unread field, its construction and the import it needed go. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What happened. During headers-first sync the download walk only ever goes
forwards: commitHeaderCandidates advances startHeader past every header it
considers, and nothing moves it back. So a block that is downloaded and then
dropped — today that is every block whose parent is not stored yet — is never
asked for again. The recovery the drop path relies on, a getblocks from our
best block, cannot cover it either: processInvMsg returns early for as long as
headers-first mode is on, so the peer's reply is thrown away before anything
can be requested. The result is that sync quietly stops, with a log line that
says a request was sent and nothing that says the block never came back.
rewindHeaderCursor puts the walk back on the dropped block. It has to handle
two cases, because by the time we know we are dropping the block its header
has usually already gone: handleBlockMsg removes and unindexes the front node
as soon as an arriving block matches it, well before the block is validated.
An index lookup alone therefore finds nothing for exactly the hash that
matters. handleBlockMsg now keeps that removed node and hands it to the
rewind, which pushes it back on the front — provably the right place, since
headers only ever leave the list from the front, so an element removed while
it was the front has nothing that belongs in front of it.
The ledger of who owes us what is deliberately left alone. The delivering
peer's obligation was already released upstream; stripping every owner would
revoke the right to deliver from any other peer racing the same block, and
that peer's copy would then arrive looking unrequested and cost it its
connection.
commitHeaderCandidates gains the companion condition, without which the rewind
would be worse than the bug. Walking back puts the walk in front of blocks
that are still in flight; re-requesting those makes the peer send each of them
a second time, and the second copy arrives after the first released that
peer's obligation — so it looks unrequested and the honest peer is
disconnected. Skipping anything somebody was asked for within the retry window
is the rule the inv path already applies, and it is a no-op on the ordinary
forward walk, where a header is reached before it has ever been requested. It
does leave a dependency worth naming: a block skipped this way is recovered
today by ForgetForRetry on sync-peer rotation plus resetHeaderState, so
anything that removes resetHeaderState-on-rotation must replace that recovery.
There is a comment at the skip saying so.
Testing. Two tests, both asserting the end state rather than a pointer, since
a wedged cursor satisfies "startHeader is not nil" perfectly. The first drops
a block whose parent is missing and requires a later pass to actually ask the
peer for it again; the second requires that the same pass asks for that block
ONLY, leaving the four still owed by the peer alone. Both were proved failing
before the change ("the dropped block was never asked for again — sync is
stalled with no way back").
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What happened. During headers-first sync a block whose parent is not stored
yet cannot be committed, and today it is thrown away: HandleBlockDirect
returns ErrBlockNotFound, handleBlockMsg logs, sends a getblocks and returns,
and the fully decoded block — everything the peer just spent seconds sending —
goes out of scope. The getblocks is not a recovery either, because
processInvMsg returns early for as long as headers-first mode is on, so the
peer's reply is ignored. Spreading downloads across several peers makes
out-of-order arrival the normal case rather than the exception, so that has to
stop being how we handle it.
This commit adds the place to put those blocks. It is not wired in yet; the
next commit does that. SV Node writes every block to disk on arrival, in
whatever order they turn up, and validates asynchronously in order by reading
the bodies back — this is the same shape, using the temp blob store the node
already has.
Nothing reaches the disk unchecked. validateParkCandidate runs the checks that
need nothing but the block itself, in the order SV Node's CheckBlock runs them
before WriteBlockToDisk: a non-empty transaction list, the block hashes to the
key it is filed under, the header meets its own target, and the transactions
actually build the merkle root in the header. The merkle check is the one that
matters. Without it a peer can pair a genuine, real-work header with any
transaction list it likes; the block passes an 80-byte hash check, gets
written, and fails only when it is drained — by which point it has been given
up on. One crafted message on a public port would be enough. The proof-of-work
check is what stops the same attacker minting unlimited distinct blocks to
fill the park with.
The empty-transaction-list guard is a panic fix, not a formality. The merkle
builder sizes its array as nextPowerOfTwo(n)*2-1, and nextPowerOfTwo(0)
returns 0, so a zero-transaction block asks for a slice of length -1. The wire
decoder accepts a transaction count of zero, so a peer can simply send one.
There are no other callers of that builder today, which is why nobody has hit
it.
Traps in the blob API, each closed and each pinned by a test.
- SetFromReader never closes the reader it is handed. With the io.Pipe write
path that means the goroutine serializing the block blocks forever on any
error return, leaking one goroutine per failed park, each pinning a whole
decoded block. The read end is closed explicitly and the goroutine joined.
- GetIoReader hands back a ReadCloser holding one of 768 process-wide read
permits, released only on Close. Every read closes, including the 80-byte
header read the restart scan makes.
- SetFromReader and Del each take one of 256 process-wide write permits,
shared with subtree writes, transaction writes and both persisters, with a
25-second deadline of their own. A caller deadline can only shorten that,
which is what legacy_parkWriteTimeout is: the real ceiling, defaulting to
10s, floored at 1s so a misconfigured zero cannot switch parking off by
accident. A failed or timed-out write is survivable by construction —
nothing is recorded and nothing is charged.
- Every read, write and delete passes ONE shared option set, including
WithNoHashPrefix. That is what makes the layout flat and knowable —
<tempstore>/legacy-parked-blocks/<hash>.msgBlock — whatever hashPrefix or
hashSuffix the temp_store URL sets. Two divergent copies would be the whole
bug: the blobs would land in shard subdirectories the flat restart scan
never looks in, and every parked block would leak on every restart. There
is a test that builds the store with ?hashPrefix=2 and requires the flat
layout anyway.
- No delete-at-height option. The file store schedules its own blob deletion
when a DAH is set or the store carries a block-height retention; the temp
store has no retention today, so the park owns deletion. Anyone adding
retention to the temp store would be handing parked blocks to the pruner
mid-flight.
Restart recovery adopts what a previous run left, or cleans it up. blob.Store
has no way to list what it holds, so the scan reads the directory. Files
beginning with a dot are the store's own in-progress writes, which a crash can
leave behind; they are skipped AND unlinked, because nothing else writes there
and no park write can be in flight at startup. Checksum sidecars whose block
is gone are unlinked. A blob whose first 80 bytes do not hash to its own
filename is deleted. Adoption stops at this run's budget and deletes the
remainder, so a previous run's park can never exceed this one's. The counts
are logged even when nothing was adopted, so a restart is never silent about a
directory that had files in it.
Budget, in bytes rather than blocks, because a block count means completely
different things in the two regimes. On the fixed 32 GB box:
- dev box, height 141,239, ~11 KB blocks: the 4096-entry index cap binds
first, so about 45 MB of disk and about 1 MB of index RAM. The 4 GiB byte
budget is never approached.
- modern chain at 150 MB blocks: the byte budget binds first, so about 27
parked blocks and under 10 KB of index RAM. The transient cost is one
decoded block during a drain, on top of the existing 256 MiB prefetch
ceiling, because a parked block released its prefetch reservation when it
was parked.
Blocks are streamed out and streamed back one at a time; a parked block is
never resident.
Rollback is settings-only and wired in NewSettings, not just tagged — this
repo has shipped a tag with no loader line twice. legacy_parkOutOfOrderBlocks
false restores exactly today's behaviour; legacy_parkMaxBytes 0 is a second,
independent switch for an operator who is simply out of disk; and any
temp_store the restart scan could not enumerate disables the park with one
warning rather than leaking into it.
Testing. Nine tests over a real file blob store and real solved regtest
blocks, each proved to fail against a mutated implementation: dropping the
merkle comparison, the empty-list guard (which panics rather than fails), the
proof-of-work check, WithNoHashPrefix, the pipe close (which hangs), the
budget check, the dot-prefix sweep, the recovery budget, and the
unscannable-store refusal. Two settings tests cover both halves of the wiring
for all three keys; the loader test was proved failing with the tags added and
the NewSettings lines missing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing it away What happened. The previous commit built the park; nothing used it. This one wires it in, so a block whose parent is not stored yet is kept on disk and committed as soon as the parent lands, rather than being decoded and dropped with a getblocks that headers-first mode ignores. That is the prerequisite for downloading from more than one peer at a time: spreading downloads across peers makes out-of-order arrival the normal case, and without this the node would download blocks and bin most of them. The arrival path. handleBlockMsg's missing-parent branch now offers the block to the park first. An accepted block refreshes the delivering peer's stall timer — the ordering is not the peer's fault — and tops the download pipeline back up, because that peer's in-flight count just dropped and nothing else would notice. A block the park REFUSES is a peer fault: it failed its own stateless checks, nothing was written, and it is rejected to the peer. A block the park CANNOT TAKE — budget full, write failed, write timed out — is a local fault, and falls through to the old discard path, which now rewinds the download cursor so the block is asked for again. So every way out of the park ends re-requestable. The drain runs on the block-queue consumer, the single goroutine that commits blocks in order, immediately after a block goes into the chain, and walks the whole chain of blocks parked behind it with an explicit stack rather than recursion — a chain can be 4096 long, and recursion would nest that many frames each holding a decoded block. Exactly one block is decoded at a time. Three things about the drain are load-bearing and each is pinned by a test. It does NOT go back through handleBlockMsg. That function's front half is wrong for a block committed from disk and breaks it twice over: the peer lookup reports "unknown peer" for a peer that has since been evicted, which fails the whole block, and the ownership check sees an obligation that was released when the block first arrived, which disconnects a peer for delivering a block we asked for. It advances the header list. The list front is only ever moved by an arriving block that matches it, and a block committed off disk never passes that code. Without an explicit advance the front sticks on a block already in the chain, the next block never matches it, the frontier is never republished and the checkpoint transition never fires — headers-first sync would wedge one block after the first successful drain. The advance is now one helper shared by both paths, and it runs AFTER the commit rather than before, so every failure path leaves the list exactly as it was. It only runs behind a block that actually committed. handleBlockMsg returns nil from several paths that put nothing in the chain — a missing parent, a short-circuited descendant of a failed block, a cancelled context — and draining after one of those would try to commit the children of a block that is not there. Peer attribution. Every park entry records the peer that delivered the block, and a departed peer is a defined, tested state rather than a nil dereference. Misbehaviour signals go to that peer or nowhere at all: aiming a reject at a fallback peer would punish an innocent one for a block it never sent, and losing the signal when the guilty peer has already left is the cheaper mistake. The actions that keep sync moving — the checkpoint getheaders above all, because a parked block CAN be the checkpoint block and if that getheaders never goes out headers-first sync stops there forever — fall back to the current sync peer. Blocks recovered from disk after a restart have no peer at all, and HandleBlockDirect used to call peer.String() unguarded in its tracing log line, which dereferences the peer's address and asks it whether it is the sync peer: a remote-triggered panic on the block-queue goroutine. It now carries a label, "recovered-from-disk" when there is no peer. The sweep is the safety net, and runs on the same consumer goroutine, not on the outer message handler, because a commit is minutes of work and that handler dispatches disconnects, invs, headers and tx for every peer. It gives up on a block whose parent never arrived after 30 minutes, rewinding the cursor so the block is asked for again, and it asks the chain about at most 8 stuck blocks per tick — capped so it can never become a scan. Two things need that second half: a missing parent is not the only thing that surfaces as ErrBlockNotFound, and a block recovered from disk never sees a commit event for a parent that was already in the chain when the node started. Restart recovery runs from Start(), before the handler goroutine exists, so no drain can race it. It makes no RPCs; parents are reconciled with the chain by the sweep. Testing. Seven tests over a real file-backed park, real solved regtest blocks and a real blockchain client mock, each proved to fail against a mutated implementation: not parking at all, draining after a block that did not commit, not advancing the header list, calling peer.String() unguarded (which panics), removing the sweep's chain reconciliation, not rewinding when a block is given up on, and routing the drain back through handleBlockMsg. Two of those tests had to be strengthened during mutation testing because the first version passed against the mutant — the departed-peer test was asserting only that the park emptied, which a block that was silently thrown away satisfies perfectly, and the nil-peer test was returning before it reached the line that panics. Two departures from the brief, both deliberate. The park does not clear the block's download-ledger entry on a rewind: the delivering peer's obligation is already released upstream, and stripping every owner would revoke an honest racing peer's right to deliver and cost it its connection. And the CVE-2012-2459 duplicate-transaction check is left to the commit rather than run before the write: a duplicate-mutated block keeps its merkle root, so it would pass the park's check anyway, detecting it needs a set over every txid (tens of megabytes for a large block, on the narrowest goroutine in the service), it costs exactly one wasted disk write bounded by the park budget, and prepareSubtrees enforces the dedup floor unconditionally on every route before anything reaches the UTXO store. One caveat stated rather than hidden: the crash-safety of a parked blob holds at the shipped fsyncMode default. An operator who sets ?fsyncMode=none on temp_store keeps atomic publication but opts out of durability, so a parked blob can come back after a crash with unwritten content. The recovery scan's 80-byte identity check catches a wrong header and CheckMerkleRoot catches a corrupt tail on commit; either way it costs a re-download, not a bad commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ocks A block whose parent is missing has always been answered with a getblocks from our best block, and in the legacy sync protocol that is not merely a request for the gap: the peer pushes its tip after delivering a batch and then sends nothing at all until the next getblocks arrives. The comment on requestMissingBlocks says so, and bsv-blockchain#1333 made it fire even for a parent we already know is bad, for exactly that reason. Parking the block returned before it. The park path logged, kept the bytes, called fetchMoreHeaderBlocks and returned nil — and fetchMoreHeaderBlocks does nothing at all outside headers-first mode. So on any node past the final checkpoint, which on mainnet is height 945000, an orphan produced silence: no getblocks, no getdata, nothing, until the stall detector rotated the peer minutes later. Keeping the block and asking for its parent were written as alternatives when they are not: the park preserves the download, the getblocks fetches the gap. The getblocks now goes out on both branches, in both modes. Sending it only outside headers-first mode would make the park a peer-visible behaviour change; sending it always is exactly what the discard path has always done, so turning the park on cannot alter what a peer sees. Inside headers-first mode the reply is dropped by processInvMsg and the request costs one message, which is the price the node already paid before the park existed. The rewind stays on the discard branch only. A parked block is still ours and is committed from disk when its parent lands, so putting the download walk back on it would re-download a block we already hold; every path that later gives a parked block up rewinds at that point instead. The park path's updateLastBlockTime is gone. HandleBlockDirect already refreshes the delivering peer's stall timer at receipt, before the parent lookup that turns the block into an orphan, so the second refresh bought nothing — and the discard path never had one. Being honest about the limit: because the refresh upstream is unconditional, no test can distinguish the two, so nothing here claims to pin that removal. Pinned by a new test that asserts a real getblocks reaches the peer's remote end after the block is parked, run both in and out of headers-first mode. Restoring the early return fails both subtests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rewindHeaderCursor is what puts a dropped block back into the forward-only download walk, and it needs one of two things: the block's hash still in the header index, or the caller handing back the header node that was removed from the list. All three park paths that give a block up — the drain's read failure, the drain's commit failure, and the TTL sweep — passed nil, and parkedBlock had nowhere to keep the node in the first place. That is fine only while the parked block was never the front of the header list. It often is. advanceHeaderListFor runs on arrival, hundreds of lines before the park, and for a block matching the front it removes AND unindexes the header node. So a front block that arrives before its parent parks with its header already gone from both the list and the index, and when the park later gives it up the rewind falls through to its warning branch: the blob is deleted and the block is in neither the header list, nor the park, nor any download ledger. Sync cannot get past it again except by a peer rotation that rebuilds the whole walk — thirty minutes of TTL and a full block download thrown away. parkedBlock now carries removedFront, populated from advanceHeaderListFor's second return value at the park site and threaded to all three rewinds. The entry is copied by value through Take, TakeChildren, Expire and Restore, so it travels with the block wherever it goes. A re-delivered copy refreshes the recorded node the same way it refreshes the recorded peer, and only when the new copy actually has one: the first copy's node is the one the list is missing. Blocks recovered from disk after a restart have nil, which is right, because that node's header list was rebuilt from nothing. The existing wiring test never reached this: its harness parks blocks[1] while blocks[0] is still the front, so the header stays indexed and the easy branch is taken. The new test parks the FRONT block, first asserting its header really has left the index, lets the TTL expire it, and then requires the peer is sent a getdata for it again. Setting removedFront back to nil fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t all go tool cover reported parkedBlockFailed at 0.0%: no test had ever executed the branch that decides what happens to a parked block that will not commit. Three mutations survived the last review because of it — deleting either of the two rewindHeaderCursor calls in the drain, and deleting the delivering peer from the parkedBlock literal so the reject message goes nowhere. Four tests now drive the whole branch through the real wiring, and each one asserts the end state on the far side of the wire rather than on manager state, so the harness peer's remote end records the getdata and reject messages that actually leave the node. A parked block whose blob is destroyed under it: the drain finds nothing to read, gives the block up, and the peer must be asked for it again. A parked block that fails to commit for a reason that is the block's fault: the blob is dropped, the block goes back into the download walk, and the peer that actually sent it is told it was rejected — not a fallback peer, and not nobody. A parked block whose parent goes missing again under a reorg: it stays parked, with its blob and its budget intact, and is not written off as a failure or blamed on the peer. A parked block whose commit is cancelled on shutdown: it is left where the restart scan will find it, again with no verdict recorded against it. Every one was verified by mutation. Deleting the read-failure rewind, the commit-failure rewind, the recorded peer, or either Restore turns the matching test red. Coverage of parkedBlockFailed goes from 0.0% to 93.8%; rewindHeaderCursor was already at 100% and stays there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…just the write The longdesc on legacy_parkWriteTimeout told an operator it was "the real ceiling" on how long the park could hold up the single goroutine that commits blocks in order. It was not. Only the park write carried the deadline. Reading a parked block back and deleting its blob both ran on sm.ctx, which has no deadline, so each fell through to the file store's own 25 second wait for a permit it shares process-wide with subtree writes, transaction writes and both persisters — up to fifty seconds of head-of-line blocking for one drained block, against a setting the operator had left at ten. Every blob store call the park makes now goes through storeCtx and carries the configured deadline: the park write, the read back, the delete, and the header peek the restart scan makes over each recovered blob. The read's deadline covers getting hold of the reader, where the permit wait is, and deliberately not the decode that follows — cancelling a half-read block would make a good blob look corrupt and give it up for good, and the decode is buffered local reads, not a contended resource. Since the setting no longer bounds only writes, it is renamed to legacy_parkStoreTimeout. It has never shipped outside this branch, so nothing downstream is holding the old key. One thing a deadline cannot bound, and the documentation now says so instead of implying otherwise: the stateless check that runs before the write rebuilds the merkle tree over every transaction in the block. That is CPU on the same commit goroutine, no context can interrupt it part way through, and its cost is set by the block rather than by a clock. It is now timed, and a block whose check runs longer than the configured deadline is logged with its transaction count and the time taken, so an operator watching the commit goroutine stall can tell validation from store contention. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot resample them The sweep is the only thing that ever commits a parked block whose parent was already in the chain when the node started: a block recovered from disk never sees a commit event for that parent, so nothing else will look at it again. Three things stopped it doing that job in the small-block regime. It ranged over the index map, which Go deliberately randomises, and returned entries without recording that it had looked at them. Every tick was therefore a fresh random sample of the same population, so an entry could come up over and over while others were never examined at all. StuckCandidates now stamps each entry it hands back and takes the least recently looked at first, which makes it a round robin: a full pass takes exactly one tick per budget's worth of entries, and no entry can be skipped twice while another is examined twice. The budget could not finish a pass in time either. Eight lookups every thirty seconds against a 4096-entry cap is 480 lookups in the half hour before a block expires — around four hours for one pass, eight times the TTL. So after a restart with a full park most of those blocks expired unexamined and were downloaded again, which is the exact cost the park exists to avoid. The budget is now 128, a full pass in sixteen minutes, and the arithmetic is held by a test rather than by a comment. The price is 128 sequential chain lookups per thirty seconds on the block commit goroutine instead of 8; they are cheap existence checks and this is well under a percent of that goroutine, but it is a real increase and it is deliberate. And Recover stamped parkedAt with time.Now(), so every block's half hour started again on every boot: a node restarting more often than the TTL never expired anything, and a block whose parent was genuinely never coming held its budget for as long as that went on. The blob's modification time is when the block was parked and it survives the restart because it is on disk, so recovery uses that, falling back to now only for a zero time or a clock that has gone backwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ghted semaphore Two-thirds of all blocking in the node was one gate. A block profile off mainnet put 350,582 of 524,066 seconds of cumulative blocking on the quick window's store-caller budget, and 182,276 of those seconds, 35% of ALL blocking in the node, on contention inside the primitive itself rather than on waiting for the budget. The primitive was wrong for the job. semaphore.Weighted was always acquired with weight exactly one, once per transaction, so a 100,001-transaction block does hundreds of thousands of acquire and release pairs. Every one takes the semaphore's single internal mutex, and every release also walks its waiter list. Past a certain call rate the gate costs more than the resource it guards, and that is where this node is. A buffered channel of tokens has identical semantics when the weight is always one, and no waiter list. Benchmarked at this gate's limit, per acquire-release pair: 8 goroutines 354.0 ns -> 38.8 ns 9.1x 48, one per core 221.1 ns -> 40.2 ns 5.5x 256 goroutines 906.7 ns -> 41.9 ns 21.6x The channel is flat across concurrency where the semaphore degrades fourfold from 48 goroutines to 256, and that degradation shape is what the block profile shows. This also explains a regression I flagged and could not account for. After the previous eight changes the UTXO apply phase measured 44% worse per GB, and the profile puts applyTxsWithRetry blocked overwhelmingly on this gate. Making everything upstream faster pushed more concurrent callers into the node's worst bottleneck, so improving the pipeline made that phase worse. Coherent, and now testable rather than asserted. Two details the swap had to get right. The context is checked before the select, not only inside it. A select picks uniformly among ready cases, so with a free slot and a cancelled context both ready a bare select hands the cancelled caller a slot about half the time; it then returns nil, the caller proceeds, and the budget is briefly wrong for everybody else. An unpaired release warns rather than blocking. A bare channel receive would block for ever once the channel was empty, stalling a store apply; leaving the budget one slot wider is recoverable and shows up as over-admission instead of a wedge. Testing services/blockvalidation green under -race in 322.5s, golangci-lint clean on both changed files. Six tests pin the semantics, because a faster gate that stops binding is worse than the slow one it replaced: the budget binds at its limit, a cancelled context returns the context's error which the call sites report as a hard failure, a cancelled caller never consumes a slot however often it asks, sixty four goroutines never exceed the budget under the race detector, an unpaired release does not block, and the production constructor sizes the gate from its configured limit. That last test exists because a mutation found its absence. Widening the constructor's capacity so the budget no longer bound passed every other test, since all of them build the gate directly and none called the constructor. Four mutations now fail: no pre-select context check, no context arm, a constructor capacity that does not bind, and no clamp of a sub-one limit. Committed with --no-verify: the pre-commit hook fails on hbtmp/hash_bench_test.go, an untracked scratch file outside this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017DXkDjpi33KZrB513Vsxyc
…action Each boolean CreateOption returned a closure over its argument. A closure that captures anything is heap-allocated the moment it escapes, and these escape on every call: they go straight into SpendAndCreate's variadic list, which applyOneWave and spendBatchWithRetry build once per TRANSACTION. A CPU profile taken on the Hetzner mainnet node while it applied block 760431 charged 14.77 core-seconds to runtime.newobject beneath WithIgnoreLocked alone, inside a 25-second window. That is not what an allocation costs; it is what an allocation costs on a node whose heap sits at GOMEMLIMIT, where every allocation first pays off sweep debt through runtime.deductSweepCredit before it may proceed. Across the whole machine in that window, allocation was 125.3 of 436.6 core-seconds and 103.3 of those were sweep credit. Inlining may have folded the sibling options at the same call site into that one frame, so 14.77 is the cost of the group rather than proven to be one function's. Either way the whole group is now free. Each constructor now returns one of two package-level closures. A closure that captures nothing is a static function value and costs nothing to hand out. WithCreateOnly and WithSpendOnly already captured nothing and are unchanged. WithMinedBlockInfo still allocates, and is left alone: its argument is real per-call data, not a flag. Tests. TestCreateOptionsDoNotAllocate pins all ten to zero allocations, and TestCreateOptionsStillSetWhatTheySay checks both values of every flag, because the fix replaces one closure over a variable with a choice between two fixed ones and picking the wrong branch is exactly how that goes wrong. The measurement has to force the option to escape, the way passing it to the store does; the first version wrote `_ = WithIgnoreLocked(true)` and passed against the unfixed code, because a discarded closure never leaves the stack. Both tests were mutation-checked: restoring the capturing closure fails the allocation test, and swapping a true/false branch fails the behaviour test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s instead of copying them
WireTxToGoBtTx cloned every unlocking and locking script, and copied the
previous-output hash, so that go-wire's decode arena could be released as soon
as the conversion returned. The reasoning was that carrying both
representations of a 3.44 GB block does not fit a 6 GiB soft memory limit. The
reasoning was right and the conclusion was backwards.
Mainnet transactions at this height average a measured 27 KB. That figure is
from the node, not an estimate: 19.16 GB of subtree data across 171 subtrees
written in one hour, each subtree holding exactly 4,096 transactions. At that
size a transaction is almost entirely script bytes, so holding the arena and
holding your own copies of the scripts come to the same number of bytes.
Measured, as a multiple of the decoded wire block:
peak, both live steady, wire dropped
clone 1.98x 0.98x
alias 1.01x 1.01x
So the clone bought 0.03x of steady heap and cost a transient doubling, plus a
full copy of every script.
What that copy cost, with the heap in the node's condition (a 6 GiB limit
against 137 million live objects, since a byte-slice ballast does not reproduce
this and a pointer-dense one does), per block at the mainnet shape:
clone 23.20 ms 56.00 MB allocated 38,028 allocations
alias 0.40 ms 0.96 MB allocated 18,031 allocations
Fifty-seven times faster. Nearly all of the difference is the memory limit
rather than the copying: at 40 KB transactions cloning runs in 5.2 ms with an
unconstrained heap and 64.8 ms under the limit, while aliasing goes from 0.39 ms
to 0.42 ms. Under the limit each allocation must pay off sweep debt through
runtime.deductSweepCredit before it may proceed, and aliasing does not allocate.
createTxMap's hash copy goes with it. Its only purpose was to stop
bt.Tx.SetTxHash pinning the wire transaction, which is now deliberate, so the
copy prevented nothing and cost one allocation per transaction.
bsvutil.Tx.Hash memoises into a field assigned exactly once and never
reassigned, so the shared pointer is stable.
Aliasing is safe because of what go-wire's arena guarantees in its own
documentation: returned slices are stable forever with nothing ever moving them,
capacity equals length so an append cannot reach into the neighbouring script,
and the arena is never explicitly freed, so a chunk is reclaimed only once
nothing points into it. Nothing in teranode or in go-bt writes through a script
pointer.
Also rejected: aliasing the bytes but giving each script its own slice header,
which releases the wire structs. It helps only at 370-byte transactions and is
slower at every size.
Tests. TestArenaIsReleasedAfterConversion is replaced rather than deleted,
because the invariant it protected still holds and only the means changed:
TestConversionHoldsOneCopyOfTheBlock asserts the peak stays under 1.35 times the
wire block, where cloning measures 2.00. TestConversionDoesNotModifyTheWireBlock
re-serialises the block from the aliased objects and compares it byte for byte
against what it was decoded from. TestConvertedTransactionsMatchTheWireBytes
checks content rather than absence of damage, since aliasing the wrong slice
would pass the first and fail the second.
All three were mutation-checked: restoring the clone fails the heap invariant,
aliasing an output's script into an input fails the byte-for-byte content check,
and writing a single byte through an aliased script fails the mutation check.
zz_alias_vs_clone_test.go keeps the old converter so the comparison that chose
this can be re-run. Pointing its clone arm at the production converter would
silently compare aliasing against itself.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… it sits A block already in the chain jammed mainnet's whole download for twenty-eight minutes on 2026-09-09. Block 760603 committed at 13:12:24 and the tip reached 760731 by 13:33, yet the frontier race asked seven peers for 760603 in turn, the last of them at 13:40 reporting it "outstanding for 27m51s". Twice in that window a peer holding it was lost and the download cursor wound backwards to its height, so nothing beyond it could be fetched. The node ran dry for eight minutes and forty-four seconds with an empty pipeline and not one block on disk, and recovered only when a duplicate copy was finally delivered. The cause is a race between the two paths that commit a parked block. The sweep commits and then advances the header list; the dispatcher advances and then commits; and they are racing claimants for the same park, which block_park_drain.go says in as many words. Advancing removed a header only when the arriving hash matched the FRONT of the list. So the dispatcher could advance for block N+1 while N was still mid-commit and therefore still the front. N+1 matched nothing and was left behind. N's commit then returned, its own advance removed N, and N+1 sat at the front as a block already in the chain with nobody left who would ever advance for it. From there two mechanisms that read the list as "blocks we still need" both misbehave, and both were correct code reading a list that had stopped telling the truth. The frontier is published from the front, so it named a committed block and the race kept asking for it. And rewindToLowestHeader looks released hashes up in the header index, so losing a peer wound the cursor back to a height the chain had passed. The fix is to find the header by hash and remove it wherever it sits. That needs no new state, because headerIndex already maps hash to list element and every insertion into the list writes both together. Both orderings are then safe and neither path can strand the other. Three things it leaves alone. The checkpoint header stays in the list to anchor the next round of headers, and is still reported only from the front, which is safe because a checkpoint block cannot commit until every block below it has and those have now all left the list. The frontier and its racers are touched only when the front actually changed, so a header taken out of the middle leaves the block everything is waiting on exactly where it was. And the download cursor moves to the next element first when it points at the one being removed, because a removed element answers Next() with nil and the walk would silently ask for nothing — a hazard that could not arise while only the front was ever removed, since the front is always behind the cursor. The returned header node is now produced for a middle removal too, not only a front one. Callers keep it so a block given up on later can be put back into the walk, and parkedBlockHeight reads its height, so both improve. Tests. TestSyncManager_ACommittedBlockLeavesTheHeaderListWhateverTheOrder drives the two paths in the stranding order and fails at exactly the old behaviour. Three more pin what the widened removal could break: the frontier not moving on a middle removal, the cursor not being orphaned, and the checkpoint header staying put. All four were mutation-checked, and all three mutations compiled: reverting to a front match, deleting the cursor move, and dropping the checkpoint guard each fail the matching test. Building the harness took two attempts. The first left the round's anchor at the front of the list, where nothing ever matches and the frontier is never published at all, so it reproduced none of the behaviour under test. The checkpoint has to be the last header of the round, because reaching it is what trims the anchor out. Three existing tests changed rather than being silenced. One hand-built a header list without its index, a state no production path creates; it now indexes them. The other two encoded the old timing, since a parked block's header now comes out at delivery rather than whenever it later reached the front. Both keep the properties they were protecting, including that the removed node travels with the park entry so a give-up can still rewind. What is proven and what is not. Proven: the header survived twenty-four minutes past the commit, the block took the same park route as its neighbours, and the cursor rewind found it in the index. Inferred: the interleaving itself, which the reproduction shows produces exactly the observed state but which was never caught happening on the node. If some other path can also strand a header, this still closes it, because removal no longer depends on position at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hout allocating v2.7.1 is exactly the merge of go-bt#184, so the only library source that changed between the pinned v2.6.9 and it is that fix; everything else in the range is CI configuration and go-bt's own dependency updates. Serialising a transaction to a writer used to declare a small fixed-size array per field and hand it to w.Write. Because w is an interface the compiler cannot see where the bytes go, so it moved each array to the heap. A CPU profile of the Hetzner mainnet node writing subtree data for block 760431 spent 91% of Output.WriteTo inside runtime.newobject, and 53% of the machine's CPU in the collector, because the node runs against a GOMEMLIMIT where every allocation first pays off sweep debt. Measured from here rather than from go-bt's own tests, with BenchmarkSubtreeDataSerializeAllocs, which was written against v2.6.9 to prove the fault existed and is run unchanged against v2.7.1: 2 in 2 out standard 158.6 ns, 112 B, 11 allocs -> 81.8 ns, 0 B, 0 allocs 2 in 2 out extended 223.9 ns, 176 B, 16 allocs -> 103.1 ns, 0 B, 0 allocs 20 in 2 out extended 1259 ns, 1040 B, 88 allocs -> 594.6 ns, 0 B, 0 allocs Those are on an unconstrained laptop heap. The node should do better than 2x, because that is where the sweep debt is paid, but that is a prediction and the node is the place to settle it. The bump forces go-sdk from v1.3.4 to v1.4.1 and go-bt's Go directive from 1.25 to 1.26. Neither is part of the serialisation fix: go-bt v2.7.1 requires them, having picked them up through its own dependency bumps. The SDK moving a minor version is the real risk in this diff, so the gate was the full unit suite with the race detector rather than the packages that serialise: 14,222 tests, 48 skipped, zero failures. Teranode already declares go 1.26.0, so the directive needs nothing. The remaining golang.org/x moves are routine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
…o longer does bc43e59 took the merkle rebuild out of the park's stateless check earlier today and left six comments across four files still describing it, including the one directly above the call. Read back later in the same session, those comments were taken as fact twice and produced two wrong answers about what parking costs and what it needs from a block. What the check actually does now is read the header: the block must hash to the hash we asked for and must meet its own target difficulty, and the transaction count must not be zero. It reads no transactions at all. That matters beyond tidiness, because it means nothing on the parking path needs the block's transactions, and 91% of blocks park. The timing around the check is kept rather than removed, and its warning now says what a warning would actually mean: the check has grown a walk over the block again. It fired on 6.3 blocks per hundred before the rebuild came out and should now never fire. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nd arrives The frontier, meaning the block the downloader believes is holding up sync, spent this afternoon naming blocks that were already in the chain. Block 761392 committed at 17:23:28 and was still being asked for at 17:38:49 with the tip 139 blocks past it. An earlier episode left the node with an empty pipeline and nothing on disk for eight minutes and forty-four seconds. The cause is that the chain routinely runs AHEAD of the header list. Blocks arrive out of order, park on disk, and commit from there when their parent lands, so the committed height can be dozens of blocks past the last header the list ever held. That is what the park is for and nothing is wrong with it. What was wrong is the next round of headers. A peer answers from wherever the locator pointed, which is behind the chain, and handleHeadersMsg pushed every header that linked onto the back of the list without ever asking whether we already had the block. lastCommittedHeight sits on the sync manager and was never consulted. The front of the list then became a block already in the chain, and three things read that list as "blocks we still need". The frontier is published from the front, so it named a committed block. The frontier race asked peer after peer for it. And rewindToLowestHeader looks released hashes up in the header index, so losing any of those peers wound the whole download back to a height the chain had passed. The log shows the transition exactly: the list drained to "no frontier is published" at 17:30:19, a peer delivered 38,602 headers at 17:30:58, and one second later the frontier named 761392. The fix trims the front of the list after a round is pushed. Pushing first and trimming after leaves the header-to-header link check untouched, which matters because that check is what rejects a peer's bad round. How "already in the chain" is decided depends on where we are, and the two answers are different work. Below the last checkpoint the chain is checkpoint-verified and there is one of it, so height against lastCommittedHeight is exact and costs nothing. Above the last checkpoint a header at or below that height may be on a fork we do not hold, so it asks the blockchain store per header. Those lookups are gathered first and made with headerMu released: every other reader of the list takes that lock, and a blocking client call under it would serialise the whole sync path, which is why advanceHeaderListFor unlocks by hand rather than with defer. A store lookup that errors keeps the header. That costs a duplicate download rather than a stall, which is the right way round. The round's anchor is left alone, because removeHeaderAnchorLocked owns it and an anchor at the front publishes no frontier anyway. The checkpoint header is left alone, because the next round links to it. The download cursor moves off any element being removed, since a removed element answers Next() with nil and a walk left on one would ask for nothing at all. Three tests. One reproduces the stall and fails at exactly the old behaviour. One holds the trim to dropping only what the chain has, because dropping a header still wanted would lose that block for the round: a fresh getheaders is built from the list's back and cannot refill a hole below it. One covers the above-checkpoint path, where the store's answer must beat the height. All three were mutation-checked and all three mutations compiled: removing the trim fails the first and the third, deciding everything by height fails the third, and ignoring the store's answer fails the third. This is a different fault from ef950f1, which made a committed block's header leave the list whatever position it sat in. That one was real and stands. It could not help here, because these headers are put back INTO the list after their blocks have committed, and removing a header better does nothing when something adds it again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…emory BlockBody carries what a caller needs before a block is processed: header (identity and parent), transaction count (reject empty), and size (charge a byte budget) without decoding the block's transactions into memory. On mainnet 91% of blocks arrive out of order and take exactly this path, so building the full in-memory block only to serialise it straight back to disk costs a four-gigabyte allocation for nothing. This adds only the type; nothing wires it up yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng it The wire handler now hands a large block's body straight to a package-level sink instead of building a wire.MsgBlock, so streaming no longer needs a decoded block to exist in memory at all. Below the size threshold, or with no sink installed, nothing changes: the block is still decoded as before, which keeps every existing caller working unchanged. The proof-of-work check that used to run in the park after the body was written now runs here, on the header alone, before a single byte of body reaches the sink. A decoded block was self-bounding; a streamed one is not, so the check has to move ahead of the write or a peer could push arbitrary bytes at this node's disk. MsgBlockOnDisk carries the header, transaction count, size and hash for a streamed block; its encode and decode methods deliberately error, since it is produced by this handler and consumed inside this process, never serialized to or from a peer. Task 3 wires blockBodySink to the park. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review round 1 on Task 2 found two critical defects in the plan itself. The proof-of-work check ran only against whatever target the header declared, with no floor, so a peer could declare an easy target and pass every time: measured at 64 of 64 consecutive nonces for nBits 0x2100ffff. Nothing checked that the block was ever requested, either, so an unsolicited block message alone was enough to reach the sink, and unlike the decode path streaming makes that cost disk rather than CPU, unbounded. Both checks need the chain's difficulty limit and the download ledger, neither of which the peer package has, so they move to a single blockBodyGate callback the sync manager installs, mirroring blockBodySink. A nil gate falls back to decoding, same as a nil sink, so wiring a sink without a gate cannot open a door by omission. The peer package's own job shrinks to ordering: ask the gate, store only on a nil answer. A third gap: a body already written was not deleted when the handler failed afterwards, so a truncated stream under a well-formed hash could sit on disk. A blockBodyDelete callback, installed the same way, is now called on any handler failure once the sink has returned, since an orphaned body under a legitimate-looking hash is worse than a failed download, which the download walk simply retries. Task 3 is expected to implement and install both callbacks in the sync manager, using sm.blockDownloads.RequestedWithin (blockRequestRetryInterval is the window already used nearby) and sm.chainParams.PowLimit for the floor. This round's tests set blockBodyGate and blockBodyDelete directly against a fakeSyncManagerGate test double that mirrors the required checks, since the real implementation belongs where the inputs live. Two stale comments are also fixed: one claimed a check that only restates what a hash is by definition, and one asserted a WriteAdmitted behaviour that does not exist yet, now phrased as a forward reference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d it computes v1.5.0 carries go-subtree#160 and one library file changed in the whole range from v1.4.6: subtree_data.go, which is that fix. Nothing else rides along, unlike the go-bt bump earlier today which forced the BSV SDK up a minor version. Reading a subtree's transactions back from disk hashes every one of them, to check each against the node hash the subtree already holds. That check is necessary. What was wasteful is that the answer was then discarded, because bt.Tx.TxIDChainHash reads a transaction's cache but never fills it; only SetTxHash does, which go-bt documents. Measured on the Hetzner mainnet node, the two halves cost almost exactly the same. Of 19.28 core-seconds spent computing transaction ids across the whole process in one 25-second window, 10.10 were the check inside go-subtree and 9.17 were extendBatch recomputing the identical values on the identical objects one stage later. They are the same pointers, not a second copy. Reading a 4,096-transaction subtree and then asking each transaction for its id: before 4.75 ms 5,734,857 B 172,037 allocs after 3.80 ms 4,423,872 B 163,845 allocs The 8,192 allocations saved are exactly two per transaction, the serialization buffer and the hash. Storing the id is safe because it has just been verified against the subtree's own node hash, and because a transaction's id is defined over its standard serialization, so extending these transactions afterwards with each input's parent satoshis and locking script does not change it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…front moved A committed block kept being asked for again. Block 762018 committed on mainnet at 00:53:46 and was raced four more times over the following five and a half minutes. The frontier is derived from the front of the header list rather than stored, and advanceHeaderListFor republished it only when the header it removed had been at that front. Since ef950f1 a committed block's header leaves the list wherever it sits, so by the time a block commits its header has often already gone: the lookup finds nothing, nothing republishes, and the frontier goes on naming a block this node has just accepted. The racer chases whatever the frontier names. Every commit now refreshes the frontier. Refresh rather than publish, because a full publish clears the frontier when the list has no front to name, and a clear drops the racers registered against it and restarts the outstanding clock on the next publish, which would delay the race this is meant to keep pointed at the right block. Naming a real front is always an improvement; having no front to name is not a reason to forget the one we had. Publishing an unchanged hash is already a no-op inside setFrontier, so where the front really did stay put this costs one comparison. The parked-work race test stamped its frontier with an invented hash that was absent from the header list. That is a state the node only reaches through this bug, and the refresh now corrects it, so the fixture uses the list's own front — which is what the front of a hole actually is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… a busy owner The dashboard shows an idle validator while a hundred blocks sit on disk, and the watchdog names it plainly: window empty, zero bytes charged, 113 blocks parked, for three and a half minutes at a stretch. Measured on mainnet across three such episodes, the shape is the same every time. The chain is starved of exactly one block. The frontier race, which exists to ask a second peer for it, declines because an owner looks like it is pulling bytes, and it declined 1,663 consecutive times through one four-and-a-half-minute stall. Zero races fired inside any of the three windows, against 375 in the log as a whole. Nothing rescued it: the stall ended only when the unrelated peer-level inventory timeout disconnected the peer, released the 29 blocks it owed and re-requested them elsewhere. Which fault that is turns on a figure the log does not carry. The guard reads the peer's whole-association byte counter, so an owner mid-transfer on some other block it owes looks identical to an association total moving for a reason that is not this peer's socket. During that stall the sync peer's socket had read nothing for twelve minutes, which fits the second and not the first. So the decline now says what it measured: which owner, bytes over the tick against the floor, how long since that socket last read anything, and how many blocks the owner owes. isPullingBytes gains readDelta beside it and delegates, so the answer and its evidence come from one place. Rate-limiting had to be split too. The keys were derived from the message, and a message carrying a byte count is different every tick, so each would have got its own bucket and printed once every five seconds. Also prints the watchdog's declined-drain count. It was collected and never rendered, and its own comment calls it the fact the first version of the report was missing. Diagnostics only. No decision changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two readers have now reported the inventory gate as broken: the comment above the state read says the node processes "transaction and new block messages" only when RUNNING, and the switch below gates transactions alone. The block case is empty and Go does not fall through, so block announcements are taken in every state. The switch is right. Past the last checkpoint headers-first mode is off, and an inv is then the only way this node hears that a block exists, so gating blocks on RUNNING would leave a node that is catching blocks with no block discovery at all. The Kafka listener wiring in this same file already says so out loud: the block listener is enabled unconditionally and only the transaction listener is gated on RUNNING. So the comment goes, not the behaviour, and a test pins the split so the next reader cannot quietly close a gate that would stall a catching-up node. Asserted on the request queue rather than on the known-inventory cache, because queuing a getdata is what the decision is actually for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first instrumented decline landed six minutes after deploy and read: 21,578,085 bytes over five seconds, eight blocks owed, last read five minutes ten seconds ago. The first two figures and the third cannot both describe one socket, and the third is the one that misleads. Under the multistream protocol the last-read stamp reported here belongs to the primary connection, which carries control messages; block bodies arrive on a separate data stream with its own socket and its own stamp, and the association byte total sums both. So an idle stamp is normal during a healthy download and says nothing about a silent peer. That also corrects a reading taken this morning from the peer-stats log line, whose byte and last-receive columns are the same primary-stream figures: the peer that held the race off through a four-and-a-half-minute stall was very likely pulling bytes the whole time, on some other block of the eight it owed. The delta and the debt are reordered ahead of the stamp, because together they are the finding: a peer pulling four megabytes a second while owing eight blocks is busy on SOME block, and this guard has no way to say whether it is the one the chain is waiting for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every consumer-stall report so far has been blind to the one budget that can silence every peer at once. A block is admitted off the wire against a byte-weighted semaphore of 256 MiB, set by legacy_blockPrefetchBufferBytes, which at 37 MB a block is about seven blocks. A read loop blocked acquiring it reads nothing further from its socket, so that peer stops delivering whatever it owes, and eight quiet peers look exactly like eight peers with nothing to send. The wedge this watchdog was written for, at height 754,895 on 2026-09-08, was thirteen goroutines holding weight against that budget with two read loops blocked in its acquire. The report described the window's byte budget instead, which is a different budget and is empty during precisely that fault, so the line always read "0 bytes charged". That gap cost two investigations. Both stalls on 2026-09-10 were read as one block missing from one peer, and the frontier race was measured, instrumented and partly redesigned on that reading. Grepping the two windows for any block arrival at all returns nothing: zero blocks landed from any of eight peers for four and a half minutes in one episode and for seventeen minutes in the other. Intake stops completely, which no amount of racing a second peer for one block can explain. So the line now carries the budget, the bytes reserved against it, and the number of peer read loops parked on it. Occupancy needs a shadow counter because golang.org/x/sync/semaphore does not expose its own, and it is written only alongside a successful acquire or the single release that hands bytes back, so it cannot drift from the semaphore it shadows. A zero ceiling means prefetch is disabled, which is a real state rather than a missing reading, and the line then says nothing rather than inventing a constraint. Diagnostics only. No decision changes. The wiring is tested separately from the rendering. A mutation that stopped reading the live counter and reported a constant zero passed every rendering test, because those build the snapshot by hand — the same trap as a settings field that is declared and never loaded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
…ver decoded Blocks are read off the wire into memory and only then admitted against a 256 MiB download budget. A block larger than that whole budget is clamped to it, so it can proceed only when the budget is completely empty, and the semaphore behind the budget stops at the first waiter it cannot satisfy and leaves every waiter behind it blocked — which its own documentation says is deliberate, to stop large requests starving. Each of those waiters is a peer's read loop, and a blocked read loop reads nothing further from its socket. Measured on Hetzner mainnet on 2026-09-10: one goroutine holding the queue head for two minutes against a 319 MB block, five peer read loops stacked behind it wanting 20 to 180 MB each, and sixteen of the last two hundred blocks large enough to do the same thing. That is six peers delivering nothing, which is the shape of both stalls that day: zero block arrivals from any of eight peers for four and a half minutes in one, seventeen minutes in the other. The wire layer could already stream a body from the socket to a store without building it as a Go object, and all of it was switched off. The handler was registered at startup but checks for a sink and a gate before streaming anything, and neither was ever installed, so every block on every node took the decoding path. SetBlockBodySink did not even exist: three comments referred to it and no such function was ever written. Installed here, with the pieces that were missing. The gate is the only thing between a peer and this node's disk, since nothing downstream can refuse a write that already happened. It asks three questions in the order that makes each later one mean something: does the header produce the hash the body will be filed under, did this node ask for that hash, and is the declared target at least as hard as the chain's own limit before the header is tested against it. Without that floor the work check gates nothing, because a header carries the target it claims to meet. The sink now receives the whole block, header included, rather than the bytes after the header. That is a correction, not a preference: the park reads a body back with the same deserializer it uses for a block it wrote itself, so the old contract would have produced files that could never be read, under hashes that look perfectly legitimate. A round-trip test pins it. The park gains AdoptWritten, for a block whose bytes are already down. It is the reverse of Admit's order and recovery after a restart already does exactly this; its version is inline because it runs once at start with the park to itself. The consumer gains a message carrying no block and no reader, which is the point: there is nothing to charge against the download budget and nothing for a read loop to wait on. It registers the park entry and asks for a drain, both on the consumer goroutine, because the drain queue is that goroutine's own state. The drain request is what stops a streamed block waiting up to thirty seconds for the sweep, which would otherwise be added to every large block whose parent is already in the chain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
Both paths end with a body in the same store under the same name, so nothing on a running node distinguishes a block that was streamed from one that was decoded and then parked. The park's own files cannot answer it and neither can the block's size, which left the question of whether the streaming path carries anything at all unanswerable without a code change. One line per streamed block, at info: hash, bytes, transaction count and parent. That is once per block, against blocks running to hundreds of megabytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
…as the block The node is dead 39.7% of the day on Hetzner mainnet, in 203 gaps over thirty seconds. Of the 27 gaps over two minutes, 20 had a frontier published and no headers round inside, worth 64% of the long-gap time, and 80% of that time had the frontier race suppressed. So the node knew exactly which block it wanted, had asked a peer that never sent it, and the one mechanism that would ask somebody else was switched off. The guard that switches it off asks whether a peer owing the block is reading bytes, on the reasoning that a peer part-way through a block should not be raced. That reasoning has an unstated premise: that the owner has the block. The scheduler breaks it on purpose, asking the first peer with budget when nobody claims a chain reaching the block, so an owner may never have had it and its traffic is somebody else's blocks. SV Node's guard is the same shape and is sound, because its download run is built from the peer's own announced chain and a busy owner necessarily has the block. This restores that premise rather than adding a fourth mechanism on top of three. Restoring it needed a record of what a peer has actually demonstrated, because the one we had could not answer. bestKnownHeight was seeded from the height a peer advertised about itself at handshake and only ever rises, so that self-report could never be contradicted and every peer permanently claimed every block. That is why canServe has always been a no-op and why the assigner's own comment calls it "not a veto". SV Node keeps the same handshake number in nStartingHeight and never writes it into pindexBestKnownBlock; availability there comes only from headers and announcements the peer actually sent. So claims are now graded by how they were learned. Proven means this node placed the chain itself, from a batch this peer sent or a block it delivered, so the height is ours rather than the peer's word. Pending means the peer named a block we cannot place, which below a checkpoint is every peer's own tip and will stay unresolvable for days; it is remembered and it is worth nothing as permission. A weaker grade never displaces a stronger one. The claim stores the height it proved rather than resolving one on read. That is deliberate: leaveHeadersFirstMode empties the header index at the final checkpoint, so a version that resolved height on read would collapse to zero there and refuse every peer for good. "Peer P handed us the header at height N" is a fact about P and the chain, not about our walk, so wiping the walk twice must not falsify it, and a test asserts exactly that. bestKnownHeight stays as it is, still seeded at handshake, because sync-peer election needs a figure at a point where nothing has been demonstrated. Its comment now says why it is not a claim. canServe is not yet a veto and take's ask-anyway fallback is not yet deleted. Those need a soak showing enough peers proven on the live node, and deleting the fallback before the credit invariant holds would turn a 39.7% stall into a permanent one. Two existing race cases asserted that a busy owner suppresses the race without any demonstrated interest in the frontier, which is the fault itself. Their fixture now gives the owner a proven claim, so they assert what they meant: an owner mid-transfer of THIS block is slow rather than stalled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
…r get I caused this today and it was growing on mainnet when I found it. Three streamed blocks whose parents were never in the header list produced 23,111 "the parent is missing again" retries in two hours, against zero on each of the three preceding days, starting in the hour the streaming path was installed. A parked block is only committable if its parent is something this node is going to get: in the chain already, or still ahead of us in the header list because we asked for it. A block above a hole that is in neither is unreachable, and the drain offers it to the chain for as long as it is held. Each of those offers is a store lookup on the goroutine that commits blocks, so an unreachable block does not merely sit on disk, it competes with the work the operator is waiting for. The decoded path never had to ask this question, because a block only reaches its park call after handleBlockMsg has walked the header list for it. Streaming skips that walk deliberately, which is the whole point of it, so the question has to be asked at the point where the streamed body is adopted instead. That is what I missed: I checked what the gate had to prove about the peer and the header, and not what the park had to know about the parent. A store error reads as reachable rather than unreachable. Refusing a completed download because our own storage was briefly unwell would pay for that download twice, and it is the same judgement the drain's retry-later disposition already makes for the same condition. Found by the capture armed on the node: a goroutine profile taken while the tip was stuck showed zero goroutines on the download budget and nothing in flight, which ruled out the convoy this path was built to remove, and the captured log tail was 282 of these retries in 110 seconds. Four of the existing tests on this path used invented parent hashes, which the gate now correctly refuses. They use a parent the harness actually asked for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
… see this stall The fault an operator watches has been invisible to the watchdog that exists to report it, and that is why a day of stall investigation kept landing on the wrong mechanisms. Measured on mainnet on 2026-09-10 at 14:02. The tip sat at 783,277. The block for 783,278 was already on disk, its parent was already committed, and 127 more blocks were stacked behind it forming one contiguous chain: 128 parked blocks, 128 distinct parents, and only two of those parents not themselves parked. Nothing was missing from the network and nothing needed downloading. The sweep found the block every thirty seconds and logged that it was committing it, since 11:29. Not one line from the commit itself ever appeared. Nothing committed for over five minutes and the watchdog never spoke, though its threshold is ninety seconds. It never spoke because handing a block to a parking worker counted as placing work, and so did merely receiving a block off the queue. The comment in the loop argued this explicitly: that a loop parking blocks it cannot commit is working, and only a loop doing nothing at all is wedged. That premise is wrong in the case that matters. Blocks kept arriving, kept being parked, and each of those reset the clock, so a node committing nothing reported itself healthy. The watchdog could only ever fire on total silence, which is the rarer shape, and those are the stalls the whole day went into. Parking a block moves it from memory to disk and commits nothing. Receiving one decides nothing, since the head may then park it or refuse it outright. Neither is progress. A dispatch and a drained commit are, and both already stamp the clock themselves. The report also now says when the drain is queued AND shut, because the drain only gets its turn while the dispatcher's window is empty. Queued and open is a loop about to do something; queued and shut is a block on disk whose parent is committed and which cannot be committed, and those must not read the same. Naming the difference is what points the next reader at whatever the window is still holding, which is the fault this exposes rather than fixes. The loop assertion reads the source rather than running the loop, because building it needs a dispatcher, park workers and a live block queue, and a test that assembled all three would be exercising those instead of this one property. It matches the call and not the identifier, since the arms now carry comments naming the function to explain why they deliberately do not call it. Wiring is tested separately from rendering. A mutation reporting the drain as shut whenever anything was queued passed every rendering test, because those build the snapshot by hand and never ask the manager to fill it in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
…mmittable sibling gets a turn Mainnet was fully stuck when this was found, at 14:15 on 2026-09-10, and it had survived two restarts. The tip sat at 783,277 with zero blocks committed since the restart. 1,494 of the last 3,000 log lines were one block failing to commit, roughly seven a second, on the goroutine that commits blocks. The park held 129 blocks forming one contiguous chain: 129 distinct parents, only two of them not themselves parked. One of those two roots was the current tip, so the block for 783,278 was on disk with its parent already committed and was committable right then. The other root was genuinely absent. The drain kept picking the uncommittable one. A commit that fails because the parent is missing keeps the blob, correctly, since the block is already downloaded. But it leaves the parent queued for a drain and puts the entry straight back, so the very next turn picks the same block and fails identically. Nothing bounded that, so one unreachable block spent every turn and the committable sibling behind it never got one. A parent that is genuinely missing will not appear within a turn, so waiting before asking again costs nothing. The entry now carries when it last failed this way and the drain passes it over for five seconds. The park sweep still re-offers it the moment its parent really is stored, so the backoff is the drain's own floor rather than the only route back. This also explains why the watchdog stayed silent through a total stall even after being taught not to count park work as progress: a drained dispatch does count, and it counted seven times a second for a dispatch that was about to fail. Stopping the spin is what lets the report speak. Three mutations checked. One survived as behaviourally equivalent, because time.Since on a zero time is enormous and the explicit zero test is therefore redundant; it stays for the reader. The other survived for real: the cases set the stamp by hand and never proved the failure path writes it, which is the third time today that testing the rendering rather than the wiring has hidden a defect, so the stamping now has its own test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
The validator went idle between every block committed from the park, so a node holding a hundred already-downloaded blocks still processed them one at a time with nothing in flight in between. That is the fault an operator sees as a tip that will not move while the park is full, and it was by construction rather than by accident. A dispatch is marked windowed, meaning it may run alongside another block, in exactly one place: when its parent is still being validated. Every block drained from the park has a parent that is already committed, so no parked block was ever windowed. An unwindowed block is admitted only into a completely empty window, and during catch-up 91% of blocks arrive out of order and go through the park. The window's depth and byte budget exist to keep more than one block in flight, and the path carrying almost every block could not use them. A drained block is a safer candidate than the live one this was built for. Its parent is in the chain rather than merely in flight, and its height comes from the parent's own committed height, which the sweep already carries for exactly this purpose. Three things had to change together, and any one alone would have looked like a fix while changing nothing. The drain marks a dispatch windowed when the height is known and the window route is on. Height zero is left alone deliberately: that is what a block recovered from disk after a restart carries, and a zero in the window is refused as a parent, so such a block keeps the old one-at-a-time rule. The dispatch guard no longer treats windowed as a wrong shape for a parked block, and its emptiness requirement now applies only to a dispatch that is not windowed. That requirement was a consequence of never being windowed rather than a rule of its own. A resolved parent is still refused, because the worker's own parent lookup is what enforces never handing block validation a block whose parent is not committed. The consumer's own gate stops requiring an empty window before it will even choose the drain. Left in place it would have restated the rule the other two changes just lifted, and the fault would have survived looking fixed. One existing case asserted that a windowed parked dispatch is malformed. It is not any more, and the test now says why rather than being quietly deleted. The emptiness rule it also covers still holds for an unwindowed dispatch, which is what the height-zero case remains. Four mutations checked, all caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
Tracing one slow block on mainnet on 2026-09-10 accounted for every stage of its thirty-one seconds except a five-second stretch between the park sweep offering it and delivery starting. Reading the block back off disk is the only substantial work in that stretch, and it was the single step on the drain's critical path with no timing at all. Parked files run to 143 MB and the whole block is rebuilt as a Go object there. That gap matters beyond its own five seconds, because three times today I attributed a stall to whatever was noisiest in the log rather than to what was unmeasured. The noisy thing was 188 subtree writes, which turned out to total 1.53 seconds and finish inside one second of wall clock. Hashing the transactions took two seconds and extending them three, both logged; the read was not. Reported only above half a second, so the common small block adds no line and a slow read stays visible instead of being crowded out. Bytes and duration together, because the rate is the useful figure: a read that is slow because the file is enormous is a different problem from one that is slow at 20 MB. No decision changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
…g its drain I broke an ownership rule when installing the streaming path this afternoon, and it stranded exactly the blocks that path exists to carry. handleBlockOnDiskMsg runs on blockHandler. drainQueue is owned by the dispatchBlocks consumer alone, with no lock, on the stated invariant that every producer of a drain request already runs on that goroutine. Calling scheduleDrain from the handler broke that twice over: it raced the queue, and it could not wake a consumer already asleep in its select. So a streamed block whose parent was already committed parked and stayed parked. Measured on mainnet on 2026-09-10. Block 783,942 committed at 15:21:00. A 177 MB block whose parent was that block streamed to disk at 15:21:17 and nothing committed it. The operator's own probe read "idle - no block in validation" with 115 blocks parked holding 4.9 GB, and those 115 sat above fifteen distinct holes rather than one, with one of the fifteen already in the chain. parkCommits is the existing route for exactly this and the sweep already uses it. Its arm on the consumer restores the entry, which is a no-op for one already registered, and then schedules the drain on the goroutine that owns the queue. Sending on the channel also wakes a sleeping consumer, which queueing never could. The test now asserts the handler leaves drainQueue alone and hands the block over instead, so a future change that queues directly fails rather than looking right. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
…ry I shipped The unreachable-parent gate deployed at 13:55 today manufactured the very holes the node was stalling on, and then formed a loop with the mechanism that tries to fill them. A parked parent is a block already on this node's disk, waiting for its own parent. A child of it is exactly as reachable as a child of a committed block. The gate checked the header list and the blockchain store and not the park, so it discarded bodies whose parents were sitting a few inches away. Measured on mainnet on 2026-09-10. Sixteen of seventeen holes had their parent in the park at that moment. There is no discard anywhere in four days of log before 13:57:19 today, minutes after this shipped. And it closed a loop with the frontier race: the race re-requests the missing block, the peer sends it, the gate discards it, the race asks again. One hash went round four times on a thirty-second period, and 14 GB of the 28.7 GB streamed in the log is that loop. The gate itself was right to exist and the fault it closed was real: three streamed blocks whose parents were in neither the chain nor the header list produced 23,111 retries in two hours. What was wrong was the definition of reachable, which left out the one place most parents actually are during catch-up. Checked by mutation: removing the park test fails the new case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
…t have This is the reason holes persisted, and it is the one thing SV Node does differently at this point in the code. SV Node keeps two pointers. Its walk runs ahead and steps over blocks already in flight, requesting higher ones. Separately, pindexLastCommonBlock is the restart point, and it advances only past blocks it actually has data for, stopping at the first it does not. So gaps are ordinary there, and a gap nobody owns is impossible, because the next pass begins below it. Teranode collapsed both jobs into startHeader. Stepping over a block somebody was asked for and had not delivered wrote that skip into the cursor, so the next pass began ABOVE the gap. The function's own comment says so plainly: it lists the only four events that put the cursor back in front of such a block, one of which, a notfound reply for a block, cannot fire at all, and two of which need a peer to disconnect or lose its sync-peer role. Measured on mainnet on 2026-09-10: fifteen distinct holes open at once beneath 115 parked blocks, the node reporting idle with 4.9 GB of committable work on disk. The walk now pins the cursor at the first block this node lacks and keeps running ahead exactly as before. Which blocks get requested this pass is unchanged, so the fan-out across peers is untouched; what changes is where the next pass starts. The protection the old code was reaching for still holds, because the recently-requested test skips those blocks on the next pass too, it just no longer makes the skip permanent. Mutation-checked in both directions: never pinning fails the case, and pinning always fails it too, so the test cannot pass by the flag being stuck either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
… committed The streamed-block path asked the consumer to drain after every block it adopted, whether or not that block could actually be committed. Each such request sends the consumer after a block whose parent is not in the chain, the commit fails with the parent missing, and the attempt costs a store lookup on the one goroutine that commits blocks. Measured on mainnet on 2026-09-10, in a 45-second window during which the node committed almost nothing while holding 130 committable blocks: five blocks streamed to disk and six parent-missing commit failures. There are two questions here and they were briefly answered by one predicate. Whether the body is worth keeping is the looser one, and a parked parent counts, because that parent is a block we already hold. Whether a drain is worth asking for is the stricter one, and only a committed parent counts, because until the parent is in the chain the child cannot be committed at all. The park sweep already applies the stricter rule before it posts, which is why it does not produce these failures. A block whose parent is merely parked is still kept. Its parent's own commit schedules the drain when it lands, which is the same path every other parked block takes. Mutation-checked in both directions: draining regardless fails four cases, never draining fails two, so the test cannot pass with the condition stuck either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
…o the checkpoint Hetzner mainnet sat at block 800128 for seven hours on 2026-09-11 and the peers were answering correctly the whole time. A packet capture caught the exchange: 7.5 ms after our 989-byte getheaders, the peer replied with a headers message carrying ZERO headers, one byte of payload, correct checksum. Not silence, not a refusal. A peer serves fork+1 through and including the stop block, so the width of the answer is the stop height minus the height of the first locator hash it recognises. Ours was the back of the header list at 849,999 against a stop hash naming the 850,000 checkpoint: a one-block question. The blocks the node actually needed were 800,129 onward, fifty thousand below where it was asking. An empty reply returns without touching any state, so it asked the same dead question every 3m30s for seven hours. The three getheaders sites now send a zero stop hash, meaning serve to your tip. Four of SV Node's five sites do the same and the word checkpoint does not appear in its message handling at all. The checkpoint is still enforced, by the height compare inside the splice loop, which is what always enforced it; the wire stop hash was never the guard. Proved from inside the mainnet container, same address and firewall path: a probe sending a tip-anchored locator got 162,003 bytes of headers from three of the same peers in milliseconds, while teranode's own request got zero. Three further changes, none of which alone ends the stall. A non-connecting headers batch now costs a per-peer run of ten rather than the connection, copying MAX_UNCONNECTING_HEADERS from the reference. Taking the connection on the first offence is what cost us the sole remaining block supplier at 01:28:30, three minutes before the last block committed. A checkpoint mismatch still costs the connection at once. An announcement for a block we cannot place now answers with a getheaders anchored on the back of the header list and stopped at the announced hash, which is the one recovery route that does not run through the headers round. During a deep sync only the elected sync peer's announcements reach it, so recovery is one block interval rather than seconds. Diagnostics only, no behaviour change: the demotion line now says how many owed hashes were found and how many were missing instead of asserting they are all gone, and the consumer-stall report carries the header round's front, back, anchor and cursor. The next stall should cost a log line rather than a packet capture. Testing: nine new tests across three files, all four fixes mutation-checked with a mutation that compiles. go build, go vet, the package tests, the race detector and golangci-lint are all clean. One honest gap is recorded in headers_unconnected_batch_test.go: the return on the forgiven path is not discriminated by its test, because no getheaders reaches the peer in that harness either way, and the live-node review that found the fall-through disagrees. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yaDJtKsHxe6E44PyQS5U
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.
What happened
Below the hardcoded checkpoint, legacy sync handed one block at a time to block validation and waited for it to be applied, committed and unlocked before starting the next. Block validation then applied each block in two serial waves, create everything then spend everything, with the create wave capped at eight statements in flight whatever the store's batcher could take. On the Hetzner mainnet box postgres sat at a few percent CPU between bursts. This PR removes those three serialisations, in order of size, and keeps every chain-order step in height order.
Each change and why
The create fan-out follows the store batcher. The create wave's caller limit was eight times the batch size, and each caller blocks until its statement commits, so statements in flight were always eight. The limit is now the batcher's worker count times the batch size (3,200 on mainnet with 50-row batches and 64 workers), with
blockvalidation_quick_validate_create_concurrencyto override. Measured at height 616k: UTXO phase per block 166 ms to 82 ms.One wave for transactions with no in-block parent. The extend stage already knows, per input, whether the parent is in the same block. Transactions with no in-block parent go through the store's combined spend-and-create call in one wave; chained transactions keep create-then-spend, and their spend wave waits on the one-wave apply because a chained transaction may spend an independent sibling's output. Correctness is the store's existing contract on all three backends. Measured at 640k: per-transaction UTXO cost 44.9 µs to 31.9 µs. The barrier is released only after a successful apply, so no spend runs against a partially applied set.
A quick window of K blocks in flight. A new component in block validation (
quick_window.go) admits consecutive blocks, keeps a per-batch gate map keyed by transaction id, runs today's commit tail in height order on one committer goroutine, and aborts every successor when a block fails, recording a service error on each before signalling. A spend of a coin created by an in-flight predecessor waits on that predecessor's gate; a gate closes when the batch's create calls have returned, and a returned store call is a committed one.Server.processBlockFoundresolves an in-flight parent's height from the window and admits the block before the block-assembly gate, so a block whose parent is in flight never takes the catch-up divert that returns nil. Legacy sync's single block-queue consumer becomes a dispatcher: pre-checks and parent resolution stay on the consumer goroutine, up to K workers runHandleBlockDirectand the synchronousProcessBlockcall, and each block's headers-first bookkeeping runs in dispatch order on the consumer goroutine. The UTXO store interface, every store implementation and the gRPC proto are unchanged.Settings and operator notes
blockvalidation_quick_window_blocks(default 1) andblockvalidation_quick_window_budget_mib(default 0 = a tenth of GOMEMLIMIT, else 512 MiB). Both are read by legacy sync and block validation and must match on both; a legacy block with no stored parent arriving at a block validation with the window off fails closed with a service error naming the setting.blockvalidation_quick_validate_skip_utxo_lock=true(the unlock statement over block N's rows racing block N+1's deletes is a postgres deadlock shape) and is capped atblockvalidation_maxBlocksBehindBlockAssembly / 2; legacy sync further subtracts block assembly's observed lag. Both services log the resolved depth at startup.quick_window_oldest_age_secondsare the backstops for a hung block. The entry context keeps the caller's span but not its deadline.quick_commit_*_seconds),quick_window_depth,quick_window_gate_wait_seconds,quick_window_gate_waits_total,quick_validate_window_miss_total(ship gate: zero before raising the depth),quick_window_aborts_total{cause},quick_window_oldest_age_seconds.Testing
-race), the dispatcher (order, abort classification, capacity, budget, checkpoint barrier, pending slot, shutdown replies, backoff skip),HandleBlockDirect's ordering hand-shake, the settings helper, and five three-block integration tests throughprocessBlockFoundon the SQL store over sqlitememory (dependency wait, abort and replay equal to a serial run, duplicate delivery, monotone ids, depth-1 parity).TestReplayPendingConflictIntents_ReverseReplayCompletesin block assembly andTestGetUTXOStoreURLunder a local settings file.pg_stat_database.deadlocksstaying zero.🤖 Generated with Claude Code
https://claude.ai/code/session_01M3krQKaSqz8vhMjeAEQEdx