fix(finalised-state): guard against concurrent background backfills in sync_to_height#1275
Draft
emersonian wants to merge 1 commit into
Draft
fix(finalised-state): guard against concurrent background backfills in sync_to_height#1275emersonian wants to merge 1 commit into
emersonian wants to merge 1 commit into
Conversation
…n sync_to_height The ChainIndex sync loop calls FinalisedState::sync_to_height on every iteration. With LONG_RUNNING_SYNC_THRESHOLD = 10, any gap larger than ten blocks takes the background path, which spawns a detached task and returns immediately. begin_background_op() only bumps a counter for wait_until_synced and does not gate re-entry, so during a from-genesis / far-behind catch-up (where the gap stays large for the whole sync) every loop iteration spawns ANOTHER backfill task. The concurrent backfills race the single LMDB writer and the ephemeral-reference swap, producing out-of-order "cannot write block at height X; current tip is Y" (Y > X) errors, exhausting the retry budget into CriticalError, and ultimately a SIGSEGV inside LMDB (the environment is opened without WRITE_MAP and with MDB_NOTLS, so concurrent writers corrupt its dirty-page bookkeeping). Return early when a background op is already in flight so only one backfill runs at a time; the sync loop re-checks on its next iteration and the existing task keeps making progress. Near-tip syncs (gap <= threshold) use the inline path and are unaffected. Validated on Zcash mainnet from genesis: before, the node crashlooped (6 restarts, stuck at h~233k with no forward progress); after, 0 restarts, a single backfill, CPU ~30-40 cores -> <1, memory ~15 GiB -> ~1.3 GiB. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
idky137
requested changes
Jun 21, 2026
| // gate re-entry. Bail out early when a background op is already in flight — the sync | ||
| // loop simply re-checks on its next iteration and the existing task keeps making | ||
| // progress. Near-tip syncs (gap <= threshold) take the inline path and are unaffected. | ||
| if router.has_background_ops() { |
Contributor
There was a problem hiding this comment.
This must be called whether or not the sync is "long running", as there must only ever be a single active db writer. (see 82aa6d0)
Member
|
I don't see a specific ask from the reviewer. I am marking this draft. When the author decides they're ready for another review they can toggle back to review-ready. |
zancas
marked this pull request as draft
June 26, 2026 20:53
zancas
added a commit
that referenced
this pull request
Jul 3, 2026
Three moves toward the pilot's acceptance bar (ADR 0006: a mainnet benchmark of an optimally fast index sync), informed by emersonian's PR #1241, which measured the monolith's serial two-await-per-block fetch collapsing to ~1 blk/s in the sandblast band with both zaino and zebra CPU-idle: - Instrument the build. run() returns SpendIndexBuildStats — worker count, blocks, spends, and wall-clock per stage (stream/extract, collate, bulk-load). Collate and load are identical across build variants, so comparing runs isolates the streaming stage. spawn_build logs the stats and gains loud, ignorable-when-unset benchmark env knobs: ZAINO_SPEND_INDEX_START_HEIGHT / _END_HEIGHT bound the built range; ZAINO_SPEND_INDEX_WORKERS sets the fan-out. - Parallelize the streaming stage. Workers pull fixed-size 1000-block chunks from a shared atomic queue — chunk-pulling self-balances the block-weight skew (sandblast blocks dwarf 2016-era ones) — and extract into worker-local buffers; one global sort and one MDB_APPEND pass follow, so workers never touch the store (the single-writer discipline whose violation PR #1275 diagnosed as LMDB SIGSEGV). There is one code path, not two: workers = 1 IS the serial baseline, so serial-vs-parallel comparisons vary only the fan-out. The move-only single-build guarantee is untouched. - Make the fetch roots-free. Workers extract directly from the zebra block (extract_spends_from_zebra_block): one get_block per height, no get_commitment_tree_roots await, no compact conversion of the discarded shielded data. The compact-form extractor remains as the test oracle, so the existing sync-loop and presence tests now cross-check the two extraction paths over the same chains. SpendIndexSync drops its network field (it only fed activation heights the extractor no longer needs). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zancas
added a commit
that referenced
this pull request
Jul 3, 2026
Three moves toward the pilot's acceptance bar (ADR 0006: a mainnet benchmark of an optimally fast index sync), informed by emersonian's PR #1241, which measured the monolith's serial two-await-per-block fetch collapsing to ~1 blk/s in the sandblast band with both zaino and zebra CPU-idle: - Instrument the build. run() returns SpendIndexBuildStats — worker count, blocks, spends, and wall-clock per stage (stream/extract, collate, bulk-load). Collate and load are identical across build variants, so comparing runs isolates the streaming stage. spawn_build logs the stats and gains loud, ignorable-when-unset benchmark env knobs: ZAINO_SPEND_INDEX_START_HEIGHT / _END_HEIGHT bound the built range; ZAINO_SPEND_INDEX_WORKERS sets the fan-out. - Parallelize the streaming stage. Workers pull fixed-size 1000-block chunks from a shared atomic queue — chunk-pulling self-balances the block-weight skew (sandblast blocks dwarf 2016-era ones) — and extract into worker-local buffers; one global sort and one MDB_APPEND pass follow, so workers never touch the store (the single-writer discipline whose violation PR #1275 diagnosed as LMDB SIGSEGV). There is one code path, not two: workers = 1 IS the serial baseline, so serial-vs-parallel comparisons vary only the fan-out. The move-only single-build guarantee is untouched. - Make the fetch roots-free. Workers extract directly from the zebra block (extract_spends_from_zebra_block): one get_block per height, no get_commitment_tree_roots await, no compact conversion of the discarded shielded data. The compact-form extractor remains as the test oracle, so the existing sync-loop and presence tests now cross-check the two extraction paths over the same chains. SpendIndexSync drops its network field (it only fed activation heights the extractor no longer needs). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
(AI-authored - this allowed Zaino to deploy in my test environment without crashing, in addition to setting the config sync_write_batch_bytes to 128MiB)
Problem
A from-genesis (or far-behind) mainnet sync crashes repeatedly with SIGSEGV and
never cleanly catches up. Logs show, on a loop:
Root cause
ChainIndex::start_sync_loopcallsFinalisedState::sync_to_heighton everyiteration. With
LONG_RUNNING_SYNC_THRESHOLD = 10, any gap larger than ten blockstakes the background path, which
tokio::spawns a detached backfill task andreturns immediately.
begin_background_op()only increments a counter forwait_until_synced— it does not gate re-entry. During a large catch-up the gapstays huge for the whole sync, so each loop iteration spawns another backfill task.
The concurrent backfills then race the single LMDB writer and the
init_or_take_ephemeralreference swap, producing the out-of-order"cannot write block at height X; current tip is Y" (Y > X) errors, exhausting the
retry budget into
CriticalError, and ultimately a SIGSEGV inside LMDB — theenvironment is opened without
WRITE_MAPand withMDB_NOTLS, so concurrentwriters corrupt its dirty-page bookkeeping.
Near-tip operation (gap ≤ threshold) takes the inline path and never hits this,
which is why only large catch-ups are affected.
Fix
Bail out of the background path early when a background op is already in flight, so
only one backfill runs at a time. The sync loop re-checks on its next iteration and
the existing task keeps making progress. Inline (near-tip) syncs are unchanged.
Validation
Zcash mainnet, from genesis, k3s/LMDB:
Note for reviewers
has_background_ops()is a counter read, so the check-then-begin_background_opisn't atomic in general. It's safe here because the only production caller is the
single sequential
ChainIndexsync loop. If you'd prefer hardening against futureconcurrent callers, a dedicated
AtomicBoolclaimed viacompare_exchange(or atokio::sync::Mutex/TryLockaround the spawn) would make it race-proof.