reindex: replay the most-work branch, not every reachable block

[bkeroack]

The bug

-reindex rebuilds the block index and UTXO set by replaying blk*.dat. Those files are a block tree, not a chain — Core and satd both persist every block they fully receive, including ones a later reorg orphaned — so any datadir that has been live through a reorg has fork points on disk.

The replay (reindex_from_flat_files) walked that tree breadth-first and connected every genesis-reachable block as if it extended the tip, with no chainwork comparison and no reorg handling:

let height = parent.height + 1;
let batch = connect::connect_block(...)?;   // no best-chain selection
self.store.write_batch(batch)?;
{ let mut tip = self.tip.write(); tip.hash = hash; tip.height = height; }

At the first fork, two outcomes:

• Loud. Branches spending a coin in common: the losing block connects first (it is valid on top of its parent), the winning block is then handed an already-spent input, and the reindex aborts with bad-txns-inputs-missingorspent.
• Silent, and worse. Branches that don't conflict: the losing block connects successfully, applying its coinbase and spends on top of the winning chain, and the reindex runs to completion reporting success over a UTXO set that no longer matches consensus.

How it was found

A mainnet -reindex against a Bitcoin Core blocks/ dir (the migration path CORE_DIFFERENCES.md points operators at) ran for three days and died at height 916308 — the first fork point in the files, where stale …056ea and main-chain …fae7 share parent …f3222fa4ad88. A full scan of the 5661 copied files found 959,048 genesis-reachable blocks, zero corrupt records, and 17 fork points, all between 916308 and 956336 — i.e. from the height where that node finished IBD and went live onward. The block data was fine; the traversal was not.

The fix

Replace the tree BFS with an explicit plan (plan_reindex_chain):

• Select the branch with the most cumulative chainwork — not depth. Depth agrees with work on mainnet but is the wrong metric across a difficulty transition. Equal work keeps the branch seen first in flat-file order, the reindex analogue of the consensus first-seen rule.
• Order it genesis→tip and connect only that branch. connect_block now always receives a block whose parent is the current tip.
• Blocks off the branch are written to the block index as DataStored and never connected, so getblockheader on an orphaned hash keeps working across a reindex and the branch stays available to find_best_valid_tip if later extended. PoW is re-checked before indexing one; no height_hash entry is ever written for it (that's fix(chain): never let header acceptance clobber the active height index (951k root cause) #322's active-chain pollution reached through a different door).
• Duplicate records for the same block are collapsed at scan time. A repeated children edge re-walked that block's entire subtree once per copy — the production datadir had three duplicate genesis records.

reindex_chainstate (-reindex-chainstate) replays by height along the existing block index and was never affected.

Tests

reindex_from_flat_files had zero test coverage, which is why this shipped. Added:

Four end-to-end cases driving the real replay over real flat files:

• ..._ignores_non_conflicting_stale_sibling — the silent-corruption half
• ..._survives_conflicting_stale_sibling — the production abort, reproduced at height 102 with a mature-coinbase double spend
• ..._follows_the_reorg_winner — winning branch written to disk after the abandoned one
• ..._tolerates_duplicate_records — duplicate copies must not double-apply
• plus ..._honors_stop_at_height_without_indexing_side_chain

All four end-to-end cases were verified to fail against the old traversal — two with Connect(MissingOrSpentInput) (the production error), two on the corrupted UTXO set — so they're real regression tests.

Four planner unit tests replace the two reachable_tip_height tests: orphans/short forks, work-beats-depth, equal-work first-seen, genesis-only.

cargo test --workspace green; cargo clippy --workspace --all-targets clean.

Operator impact

Anyone who ran -reindex on 0.4.x or earlier against a datadir that had seen a reorg should re-run it. An aborted reindex left nothing usable; a "completed" one may have a UTXO set that silently disagrees with consensus. Noted in the 0.5.0 release notes and upgrade notes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GJqZbJtTUo7K7G9wJcrvHv

[bkeroack]

Follow-up: audit of every block-selection path (2nd commit)

Asked whether the first commit was a durable fix or a bandaid, so I audited every path that picks the next block to connect:

path	picks next block by	enforced prev == running tip?
IBD connect_stored_block	hash, caller-driven	yes, always has
accept_block	hash, real chainwork/reorg logic	yes
background.rs	own cursor, separate chain	n/a (own invariant)
reindex_from_flat_files	tree BFS → most-work path	fixed in commit 1, but nothing enforced it
reindex_replay (-reindex-chainstate)	height→hash index	no

The last row is the same corruption class and was still live. The height→hash index is derived state, and it has been observed polluted with a fork block in production (#322, and the bad-cb-height reindex loop that followed). Given such an index, reindex_replay connected the fork block, carried on with the main chain on top of it, and returned Ok — a successful -reindex-chainstate over a UTXO set assembled from two branches, with nothing in the log to indicate it.

That isn't hypothetical: the new test reindex_chainstate_refuses_a_spliced_branch was run against the unguarded code and the reindex completes:

panicked at: a spliced replay must fail, not complete: ()

Fix

require_extends_tip, called from all three reindex connect paths. Same invariant and same error as the IBD path — deliberately placed on the connect rather than inside any one selection strategy, so a future bug in either picker surfaces loudly before it can touch the UTXO set. On the flat-file path it now holds by construction (the plan walks parent pointers); the assert is there so a change to the planner can't quietly reintroduce a side-chain connect.

Explicitly not claimed

A height index naming a complete, self-consistent alternative branch would still be replayed faithfully — catching that needs chainwork-based selection inside reindex_replay, a larger change. The guard covers the shape actually observed: a fork block spliced into an otherwise-correct index.

Regression protection

Every new test was verified to fail against the pre-fix code, not just pass against the new code:

test	pre-fix failure
..._ignores_non_conflicting_stale_sibling	stale coinbase leaked into the UTXO set
..._survives_conflicting_stale_sibling	Connect(MissingOrSpentInput)
..._follows_the_reorg_winner	tip height 11 instead of 8
..._tolerates_duplicate_records	duplicate subtree walk
reindex_chainstate_refuses_a_spliced_branch	reindex returned Ok over a spliced UTXO set

cargo test --workspace green; cargo clippy --workspace --all-targets clean.

[bkeroack]

Third commit: chainwork selection inside reindex_replay — the gap from the last comment is now closed

The previous comment flagged what the guard did not cover:

A height index naming a complete, self-consistent alternative branch would still be replayed faithfully — catching that needs chainwork selection inside reindex_replay, a larger change.

That's now done, and it also upgrades the polluted-index case from refuses to start to rebuilds correctly.

What changed

New chain::replay_plan derives the replay chain from the block index rather than the height→hash index. The block index is self-authenticating where it counts: every entry carries the block header, and a header names its parent by hash. Selection picks the most-work fully-connectable branch and recomputes cumulative chainwork and height from those headers — so neither the height→hash index nor the entries' own chainwork/height fields can misdirect the replay.

Block status is honored during selection, so invalidateblock survives a chainstate reindex (invalid block + all descendants excluded), and a header-only hole in a branch's ancestry demotes that branch instead of wedging the replay.

Resolution is a memoized walk to genesis — linear in block count. find_best_valid_tip can't be reused: its per-candidate is_connectable walks back to the active chain, which during a reindex is genesis, making it quadratic.

The non-obvious part: the prefetcher's MTP

The plan is also threaded into the prefetcher. This is load-bearing, not tidiness. The prefetcher runs ahead of the connect cursor, so for the heights it's working on, the height index still holds the pre-reindex chain. Once the replayed chain can differ from what the index names — which is the entire point of this commit — leaving compute_mtp on the index would hand the connect thread an MTP computed over the wrong branch. MTP gates BIP113 locktimes.

The IBD caller passes None: there the index is authoritative, written forward as blocks connect and guarded by connect_stored_block.

Smaller items

• Refuse to resume a partial chainstate that isn't on the selected branch (rather than building one UTXO set from two chains).
• Flush before planning, so for_each_block_index — which reads through the coin cache to the backing store — can't miss an entry still in the dirty cache.
• max_indexed_height removed rather than left as a trap: a height→hash probe whose only remaining caller was the replay's progress total. The plan's tip height replaces it.

Tests

Seven planner unit tests: work-beats-depth, forged chainwork/height fields ignored, invalid subtree excluded, header-only gap demotes a branch, deterministic hash tie-break, orphan strand, genesis-only.

Two end-to-end changes:

• reindex_chainstate_ignores_a_polluted_height_index — replaces the previous "refuses a spliced branch" test. A polluted entry is now simply irrelevant: the replay completes on the correct branch, the stale coinbase never enters the UTXO set, the coin count matches the pre-reindex set, and the bad height entry is rewritten. Verified to fail against the height-index picker (BadPrevBlock).
• reindex_chainstate_refuses_to_resume_onto_a_different_branch — cross-branch resume fails closed.

cargo test --workspace green; cargo clippy --workspace --all-targets clean.

Remaining known limit

Selection needs the block index to be present and readable. If the index itself is missing entries (not merely wrong), the affected branches are excluded and a full -reindex from the block files is the recovery path — which is what the error message says.

[bkeroack]

Deep review — five independent reviewers

Lenses: selection algorithm / UTXO-consensus safety / side-chain indexing & regression / scale & ops / adversarial test review. Everything acted on below I verified myself first.

Two definite bugs, fixed in cedcc432

Both are in the guards this PR added, not the selection it rewrote. The convergence was unusual: 4 of 5 reviewers found the first independently, 3 of 5 the second.

1. The tie-break incumbent was read from the index this PR exists to distrust.
state.rs re-derived it with get_block_hash_by_height(required_height) — the height→hash index replay_plan's own module doc calls "observed polluted with a fork block in production (#322)" — at exactly the height where that pollution was seen. The incumbent wins an exact chainwork tie, so a polluted row handed the win to an equal-work stale sibling and rebuilt the entire chainstate onto the orphan. The inverse of this PR's purpose, through the one input that wasn't hardened.

The authoritative hash was already in hand and discarded: main.rs read store.get_tip() and kept only .height. Now threads (hash, height).

Reproduced before fixing — with the old lookup the replay lands on the sibling:

assertion `left == right` failed: the tie-break must follow the authoritative tip, not the polluted row
  left: 584274ea…  (the polluted row's sibling)
 right: 6f436e71…  (the authoritative tip)

New regression test reindex_chainstate_tie_break_ignores_a_polluted_height_index. The existing polluted-index test pollutes a mid-chain height, where the loser is excluded on work and the incumbent never gets a say — only a tie at the tip exercises this.

2. The fail-closed floor destroyed its own precondition, so it could only fire once.
required_height came from a tip that clear_chainstate() then dropped, and was never persisted. The retry — an operator re-running, or Restart=always in contrib/systemd/satd.service bringing it back within seconds — reads the genesis tip the failed run left behind, computes a floor of 0, and passes. Replay connects nothing, reports success, and the node serves height 0 with an empty UTXO set while clear_chainstate has already stamped the tx/address/filter indexes complete: Electrum and Esplora answer "no history" for every address. Precisely what the guard's comment says it prevents, one restart away.

The check now runs before the clear, on the intact block index. Nothing is destroyed until the replay is known possible, so a retry re-runs it identically. Costs one extra block-index scan on a job measured in minutes to hours.

Also fixed

• Verify the block read at flat_pos is the block the plan selected. A damaged block index — the thing -reindex-chainstate repairs — can point at a different well-formed record. Nothing noticed: connect_block would write that block's hash, height row and UTXO delta while the caller set the in-memory tip to the plan's hash, and the next require_extends_tip compares against the in-memory tip and passes. Split-brain to completion.
• Log which chain was selected, both paths. The failure this PR fixes was "it replayed the wrong chain", and diagnosing it took a three-day run plus a custom scan of 5661 files because nothing ever said which branch was chosen.
• Name block/height/flat-position on read failure.
• Two false doc claims corrected. The flat-file tie-break is not first-seen — BFS dequeues in height order, so on an exact tie the shallower tip wins and scan order only decides within a height (the two planners can therefore disagree on such a tie). And index_reindex_side_chain claimed "the reorg path validates before connecting"; it does not — reorg_to goes through connect_block, which checks neither PoW nor difficulty.

Filed rather than fixed here

• reindex: -reindex has no coverage floor, so a missing blk file silently truncates #504 — -reindex has no coverage floor; a missing blk000NN.dat silently truncates
• reindex: no check_block on the replay path, so a flipped bit in a tx payload is connected #505 — no check_block on the replay path (Core-parity gap: a flipped bit in a tx payload connects and reports success)
• reindex: side-chain entries are indexed on a PoW check alone, and the reorg that connects them re-checks nothing #506 — side-chain entries indexed on PoW alone, and the reorg that connects them re-checks nothing
• reindex: test gaps left open after #491 #507 — test gaps, including that tolerates_duplicate_records passes against the old traversal and so is not a regression test

Correction to the PR description: "All four end-to-end cases were verified to fail against the old traversal" is wrong for tolerates_duplicate_records. Master's loop already did header_by_hash.remove(&hash) then continue on the second copy. The other three do genuinely fail against the old code.

Confirmed clean

Chainwork accumulation matches connect_block exactly (so a reindexed node cannot pick a different tip than a synced one); no cycles, no recursion, no OOM — the scan dedups before inserting the child edge, making the genesis-reachable subgraph provably a tree; determinism holds on both paths; memory figures in the doc comments are arithmetically correct (~518 MB flat-file peak, and the new drop(children) is a net improvement for the connect phase); undo data is written for every replayed block; the batch is atomic across all CFs; P2P cannot start mid-replay; no path writes a height_hash row for an off-branch block (verified against the complete set of writers, not the PR description); DataStored is the correct status and find_best_valid_tip accepts these entries; the prefetcher's compute_mtp(plan) change is necessary and correctly scoped, and the asymmetry with the direct path is right rather than a bug.

cargo clippy --workspace --all-targets and cargo test --workspace clean.