test: reproduce the reopened-rescan livelock in the pool-healing path - #2604
test: reproduce the reopened-rescan livelock in the pool-healing path#2604zancas wants to merge 12 commits into
Conversation
A pool whose history is reopened by reopen_pool_history keeps its condemned wallet blocks, and create_scan_task seams every rescan chunk above the lowest on one of them. InitialScanData::new trusts that seam's tree bounds over the chain metadata served with the compact blocks, so scanning the chunk reproduces the exact IncorrectTreeSize that caused the reopening. That error reaches the scan-result handler long before the lowest chunk's slower trial decryption completes, the wallet reopens the same history again, and the session ends with no progress: the healing loop never completes for a reopened history that spans more than one chunk. The new test drives that cycle deterministically. It builds the post-reopening sync state that reopen_scan_ranges_from produces, dispatches two chunks exactly as the two concurrent scan workers receive them, and asserts the property the healing needs in order to make progress: rescanning the second chunk must not reproduce the reopening error. The test fails on this branch by design and passes once the healing strips, revokes, or stops trusting the condemned blocks' testimony. The branch's end-to-end tests cannot observe this because their regtest histories fit a single chunk. MockWallet also gains a real transparent-address store, because create_scan_task reads it and the getters were todo stubs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A version-0 TreeBounds blob records no ironwood tree sizes; the reader manufactures zeroes and discards the version byte, so a block written by a pre-Ironwood scanner at or above Nu6_3 activation reads back indistinguishable from one that legitimately saw an empty ironwood tree. Those laundered zeroes are the same seam testimony the livelock test on this branch shows the healing path trusting. The first test asserts WalletBlock::read rejects a v0 TreeBounds above ironwood activation; it fails on this branch by design. The second asserts a v0 TreeBounds below activation still reads, so the coming fix cannot over-reject legitimately pre-Ironwood blocks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The qualified range is the span of blocks this scanner is designed to handle; it grows only when pepper-sync gains the capability to process a new epoch, never merely because upstream announces one. Without a guard, a stale binary on a chain that has activated an upgrade beyond its qualified range would scan blocks it cannot correctly process, recreating the Ironwood seam-poisoning bug one epoch later. Once per sync session, before any wallet mutation, the engine now fetches the server metadata through a new FetchRequest::LightdInfo variant and checks the tip consensus branch ID against the branches the compiled BranchId enum can name. An unknown or unqualified branch aborts the session with the typed SyncError::TipOutsideQualifiedRange, and a malformed branch string is ServerError::MalformedConsensusBranchId. Two canaries watch the upstream boundary. The exhaustive BranchId match breaks the build the moment upstream stabilises its cfg-gated Nu7 variant, forcing a qualification decision. A canary test fails the moment any upgrade beyond NU6.3 acquires a mainnet activation height, demanding the height-bounded scan planning for the Some case. The ratified vocabulary (Qualified Range, Unqualified, Valid Epochs, Seam Block, Quarantine, Tolerant Read, Live-Lock) is pinned in the zingolib domain glossary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The two senses of Unqualified now carry distinct names: an unqualified reading binary is the running software whose Qualified Range does not cover the wallet before it, refusing at wallet read before any scanning exists, and unqualified written data is an entry recorded outside its writer's Qualified Range. The entry also records the ratified heal boundary: truncation to the end of the poisoning writer's greatest understood epoch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feat: qualified-range guard and provenance red test atop the livelock reproduction
TreeBounds now carries its provenance: the greatest epoch understood by the software that wrote it. The reader maps the wire version to the new field instead of discarding it, and the writer round-trips it — a pre-Ironwood record saves back as a byte-identical v0 blob — so neither a read nor a save can launder writer information again. Given a valid indexer (the branch-ID guard has passed) whose chain height is at or above the wallet height, sync now heals unqualified written data before any other wallet mutation: if any block carries pre-Ironwood provenance at or above Nu6_3 activation, the wallet is truncated to the last qualified writer height — activation minus one — through the existing truncation machinery, rewinding all derived state, and the session rescans the span with qualified code. A shorter indexer defers healing untouched, so an offline or lagging session never destroys data it cannot yet replace. The truncation of stores and scan-range accounting is unified in the targeted_truncate_wallet_height wrapper, shared by healing, the wallet-above-chain reset, and the clear-all recovery; the reorg handler stays outside it because that flow re-prioritises ranges rather than truncating them. The serialization tests retarget from reject-at-read to the ratified quarantine semantics: a v0 blob reads successfully with PreIronwood provenance, and writes back byte-for-byte. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reopening a pool history now revokes that pool in every surviving wallet block: TreeBounds carries per-pool revoked-testimony flags, reopen_pool_history sets the condemned pool flag through revoke_pool_bounds_testimony, and the seam-trusting arm of InitialScanData::new falls back to chain-calculated bounds for any pool whose seam testimony is revoked, or whose seam predates Ironwood. The rescan therefore starts from the chain truth instead of the condemned record, and the reopened-rescan livelock closes: the previously red reproduction test now passes, making the pepper-sync suite fully green. The flags persist as TreeBounds serialization version 2, which appends three revocation bytes; version-1 blobs read with no revocations, and pre-Ironwood records still round-trip byte-for-byte in their version-0 layout. The livelock test fixture now fabricates the stripped post-reopening contract, since reopening strips as of this change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The truncation appliers move from sync.rs into sync/truncate.rs beside the pure planning rules, and the bare primitives become private to that module. The exposed surface names each legal composition: the paired targeted_truncate_wallet_height rewinds range accounting with the data, for every flow in which the claimed height itself is wrong; the data-only truncate_data_for_verification exists solely for reorg fork-finding, where the caller re-prioritises the affected span for verification and range coverage must survive; and clear_wallet_data is the clear-all recovery. An unpaired data truncation can no longer be written by accident, which is the composition hole that let the reopening path leave condemned testimony seam-eligible. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The glossary gains the judgment and its consequence as first-class terms. Condemnation is the finding that a record fails to account for the chain, by either channel: the load-time provenance verdict or a scan-time contradiction. Revoked Testimony is the standing of a condemned record — retained but never citable, surviving saves — of which Quarantine is the load-time form. The Seam Block entry now states that a revoked seam is refused rather than consulted, and the paired-truncation doc lists its three flows explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
truncate_scan_ranges moves from state.rs into sync/truncate.rs and becomes private, taking its unit test with it. The totality argument: the truncate module exposes every legal truncation as a named composition, and a composition surface is only total if neither half is reachable on its own. Data-alone truncation was already private; range- alone truncation remained writable from anywhere in the sync module, leaving open the inverse composition hole — accounting rewound while data stands, the mirror image of the defect that produced the reopened-rescan livelock. With both primitives private, the named compositions are exhaustively the truncations that can be written. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The module name carries the verb, so the functions no longer repeat it: targeted_wallet_height, data_for_verification, plan, plan_pool, and the private wallet_data, stores, and scan_ranges. Callers use partial imports and read as truncate::targeted_wallet_height and truncate::plan_pool, naming each operation exactly once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Oscar-Pepper
left a comment
There was a problem hiding this comment.
this seems like over-kill, I dont think the right solution is to add so much unecessary new fields and logic, especially adding to serialization. From what i understand, the issue this PR is trying to solve is the fact that the wallet block with the incorrect ironwood tree bound still persists in the wallet after the IncorrectTreeSize error is hit and the scan range is set back to historic for rescanning. It is true this error can be hit again as it wont necesarily be scanned in linear order and we can't use verify scan priorities for this purpose, meaning we cannot guarantee linear scanning. I believe the correct approach is to just remove any wallet data in the ranges that were reset and add the heights of all removed txs as scan targets so these txs are priorities and also definately not missed during rescan
|
Data written by a writer that doesn't know how to interpret the data (because it's from the writer's future) cannot be known to be correct. The specific instance is not the correct thing to fix. The thing to fix is to make the system aware of how the input data is evolving and make it impossible for this class of errors to ever recur. |
|
i propose we solve this issue with this #2607 instead |
This PR adds one deterministic unit test that demonstrates a livelock in the healing path introduced by the base branch, plus the two-line mock repair the test needs. The test fails on this branch by design; it passes once the healing is fixed.
The cycle it demonstrates
reopen_pool_historyclears the condemned pool's shard tree and returns its scanned ranges toHistoric, but it leaves the condemned wallet blocks in the block store, and theirtree_boundsstill carry the zeroes that triggered the reopening. The rescan then dispatches adjacentHistoricchunks to both scan workers (HistoricoutranksScanningin the priority order, so the second worker does not idle).create_scan_taskseams the second chunk on the surviving stale block below it, andInitialScanData::newtrusts that seam's bounds over the chain metadata served with the compact blocks. The second chunk therefore failscheck_tree_sizeon its first block, in milliseconds, with the exactIncorrectTreeSizethat caused the reopening — while the first chunk still has its full trial-decryption workload ahead. The fast failure is processed first: the wallet reopens the same history again and the session ends, discarding the slow chunk's work. The next session reconstructs the identical race, so the healing never completes for any reopened history that spans more than one chunk.The test drives one full turn of that cycle without timing or concurrency: it fabricates the post-reopening sync state that
reopen_scan_ranges_fromproduces, issues the twocreate_scan_taskdispatches the two workers would receive, scans the second chunk from its attached seam, and asserts that the scan does not reproduce the reopening error. On this branch it panics with:The branch's end-to-end tests cannot observe this because their regtest histories fit inside a single chunk, so no rescan chunk is ever seamed on a condemned block.
Candidate fixes
Either of these makes the test pass. First, have
reopen_pool_historyalso strip or revoke the reopened pool's bounds in the surviving wallet blocks, so no future seam can trust them — the branch's ownstrip_pool_tracking_for_testalready implements exactly this cut. Second, teach the seam-trusting arm ofInitialScanData::newto fall back tocalculate_block_tree_boundsfor a pool whose seam testimony is condemned.Incidental repair
MockWalletgains a real (empty) transparent-address store becausecreate_scan_taskreads it and both getters weretodo!()stubs.🤖 Generated with Claude Code