#34489 index: batch db writes during initial sync
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #34489 index: batch db writes during initial sync | Waiting on author | 0 (+1) | Blocked | 119 + 181 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Batches index database writes into windows of up to 500 blocks during initial sync, passing an active CDBBatch to each index's CustomAppend method. Updates the sync loop to fetch block ranges via NextSyncRange under a single cs_main acquisition rather than locking cs_main on every block. Adds unit test coverage for batch sync windows, reorg handling mid-sync, and locator persistence across batch boundaries on shutdown.
Problem: During initial synchronization, node indexes write LevelDB records to disk block-by-block and acquire cs_main once per block. On spinning disks, this per-block I/O degrades sync performance and generates an excessive number of LevelDB files, while frequent cs_main contention slows down concurrent block validation and initial block download.
Evidence of importance:
- PR description: 'Batch DB writes instead of flushing per block, which improves sync time on HDD due to the reduced number of disk write operations.'
- PR description: 'Reduce cs_main lock contention, which improves the overall node responsiveness (and primarily IBD) while the indexes threads are syncing.'
- PR description cites reducing the number of generated LDB files and decoupling pre-work from #26966.
Scope: Refactors the virtual CustomAppend interface on BaseIndex and all four index implementations (coinstats, txindex, txospender, and blockfilter) to accept CDBBatch references.
Risk of a mistake: medium (other). Flawed batching or crash-recovery logic during initial index sync could lead to corrupted index databases or failed sync resumption after an unclean shutdown, requiring a complete reindex.
Reviewability
Paused · Waiting on author
Open objections from l0rinc with no author reply for 65 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: rebase and addressing corruption race on interrupt and flat file writes 2026-08-23
- author: rebase and addressing l0rinc's objections on interrupt state desync and loop bounds 2026-08-23
Agreement
Blocked
objection that needs a change, nobody has replied (l0rinc)
Thread notes: Re-read and updated all claims and questions per schema rules. Added non-objection claims (questions from fjahr, sedited, and suggestion from arejula27) that were missing in schema 1. Marked r:3744459738 answered after furszy's acknowledgment in c:5388740037, and updated rc:2905123979 to contested following l0rinc's response to sipa and furszy. / Re-evaluated all records under current schema. l0rinc's r:3744459738 objection was previously marked answered with a note that standing was not assesse
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| fjahr (contributor) | question, approach | Why keep ProcessBlock around instead of handling one or many blocks in one function? | answered · author explained specialization plans for parallelization in rc:2759661644 | answered | yes | 2026-02-03: 'Why keep `ProcessBlock` around? It should be simpler if we just have one function and it can be handed one or many blocks.' Status from: 2026-02-03: furszy: 'Because `ProcessBlocks` will be specialized in a follow-up commit (please see #26966) to enable additional speedups' |
| fjahr (contributor) | question, approach | Has the batch size been optimized with benchmarks or is that planned, possibly per-index? | accepted · author replied in rc:2759584026 to handle in follow-up; fjahr accepted in rc:2935249122 noting benchmark results showed no penalty | answered | yes | 2026-02-03: 'Has this been optimized with benchmarks or are you planning to do that? This might be something differs between the indexes so it might make sense to let each index define their own value.' Status from: 2026-03-14: fjahr: 'the benchmarks have been run now and quite extensively and I haven't seen any penalty from this change. I think the follow-up that tweaks the value... if that is the case then I don't think my comment should be considered a blocker.' |
| arejula27 (none) | suggestion, approach | Consider batching by transaction count rather than block count to make batch sizes and memory usage more uniform | answered · author explained the trade-offs regarding parallelization in c:3892133978 and previous comments | answered | yes | 2026-02-12: 'I suggest considering other approaches, such as a fixed transaction count (e.g., adding blocks until the number of transactions within them falls within a target range).' Status from: 2026-02-12: furszy: 'even if it runs at the same speed on an HDD (which I doubt), the other improvements this PR brings still make it worth it.' |
| arejula27 (none) | objection, correctness | Early return in ProcessBlocks bypassed interrupt handling, skipping the expected recovery and commit path | fixed · fix pushed · author replied in rc:2807692937; fixed by rc:2807692937 | answered | yes | 2026-02-14: 'Returning `false` here causes the interrupt to be handled incorrectly... As a result, the expected recovery path is skipped.' Status from: 2026-02-14: furszy: 'Nice catch. Pushed an update that fixes it and adds test coverage.' |
| arejula27 (none) | objection, interface | Removing final from BlockFilterIndex weakens class encapsulation solely to allow test subclassing | fixed · fix pushed · author replied in rc:2885860031; fixed by c:4000080225 | answered | yes | 2026-03-04: 'Im not sure if removing `final` from `BlockFilterIndex` only to allow `IndexBlockSim` to subclass it for **testing** is the better decision' Status from: 2026-03-04: furszy: 'Updated per feedback. Thanks @arejula27.' |
| sedited (contributor) | question, safety | What is the expected memory increase from batching and does it put undesirable memory pressure on the OS? | answered · author replied in c:4019669239 explaining why conservative batch size keeps memory overhead acceptable during initial sync | answered | yes | 2026-03-08: 'What is the expectation on the increase of memory usage here? I tried profiling it with massif... Can we really batch operations in this way without putting more memory pressure on the OS?' Status from: 2026-03-08: furszy: 'The batch size is intentionally small. I was very conservative with the chosen value to avoid having to worry about it here.' |
| l0rinc (contributor) | objection, correctness | In-memory index state advances ahead of persisted data during batching; an interrupt causes Commit() to write advanced state with a stale locator, corrupting the index on restart | open · author acknowledged the race condition but has not pushed a rebased fix | changed | yes | 2026-03-09: 'The most serious issue seems to me that `Commit()` during interrupt writes advanced `m_muhash` alongside a stale locator, which causes `CoinStatsIndex::CustomInit` to reject the index as corrupted on' |
| l0rinc (contributor) | objection, correctness | Commit claims to be a refactor with no behavior change but reorders iterator assignment before block processing without documenting the change or safety | contested · author replied in rc:2936981455 and sipa clarified in rc:2940970303, but l0rinc maintained in rc:2941160846 that commits should be squashed or documented | answered | yes | 2026-03-09: 'We're changing behavior in a commit that claims it's not a behavior change, without adding context for why this change was needed' Status from: 2026-03-16: l0rinc: 'I think those commits should be squashed and the behavior change either fixed or documented.' |
| l0rinc (contributor) | objection, safety | Unbounded backward loop in ProcessBlocks could loop infinitely or crash past genesis if start and end heights are invalid or on different forks | open | answered | no | 2026-03-09: 'Given how intertwined the code is here, a serious mess-up could cause an infinite loop and would be really hard to trace back to the source.' |
| l0rinc (contributor) | objection, correctness | BlockFilterIndex writes flat files to disk eagerly per block, which is misaligned with LevelDB batching and risks orphaned flat file data on interrupt | open · re-raised in rc:3882262247 asking if it can be fixed | answered | no | 2026-03-09: 'We seem to be writing to disk to eagerly here, this also is misaligned with the batching.' |
Support:
- l0rinc (contributor) (Concept ACK, 2026-02-03) [read]: I prefer this over https://github.com/bitcoin/bitcoin/pull/33306 - and will investigate if this solves that problem once my benchmarking servers free up.
- optout21 (contributor) (ACK, 2026-02-09) [read: src/index/base.cpp]: LGTM after clarifications and changes! Code review, no assessment on performance.
- arejula27 (none) (Concept ACK, 2026-02-12) [read]: I really like the idea and I think it will be a massive upgrade with your parallelisation proposal #26966
- polespinasa (member) (Concept ACK, 2026-05-27) [read: src/index/base.cpp, src/index/base.h]: swift code review for the Spanish PR review club; noted commit messages could explain refactoring steps better [not substantive]
Participants:
- fjahr (contributor, question, 5 statements 2026-02-03 to 2026-03-14): asked about retaining ProcessBlock, batch size tuning, and benchmarks; confirmed benchmark results show no penalty
- hebasto (member, neutral, 1 statements 2026-02-03 to 2026-02-03): noted IWYU include issue resolved in #34498
- l0rinc (contributor, objection, 43 statements 2026-02-03 to 2026-08-28): requested changes on locator/in-memory desync causing corruption on interrupt, loop bounds, eager flat file writing, and lack of explanation for refactors
- maflcko (member, question, 2 statements 2026-02-04 to 2026-03-03): asked about running tests across all commits and noted unrelated CI test failures
- Jhackman2019 (none, neutral, 1 statements 2026-02-08 to 2026-02-08): reported clean build and test pass on ARM64
- bvbfan (contributor, neutral, 1 statements 2026-02-08 to 2026-02-08): pointed out a non-existent function call in txindex.cpp
- optout21 (contributor, support, 10 statements 2026-02-09 to 2026-03-07): crACK after discussions on batching logic and lock contention reduction
- arejula27 (none, objection, 24 statements 2026-02-12 to 2026-08-23): Concept ACK; reported interrupt handling bug and removing final (both resolved); helping rebase and benchmarking
- sedited (contributor, question, 5 statements 2026-02-18 to 2026-03-08): asked about BlockBatch usage, NextSyncBlock design, and potential memory pressure
- sipa (member, neutral, 1 statements 2026-03-16 to 2026-03-16): clarified that the iterator reordering refactor does not alter execution behavior
- polespinasa (member, support, 4 statements 2026-05-27 to 2026-05-27): Concept ACK during PR review club; suggested clearer commit messages on refactors
Dependencies
Files
195 lines under test/bench/ci.
- src/test/blockfilter_index_tests.cpp +181/-14
- src/index/base.cpp +83/-33
- src/index/base.h +16/-2
- src/index/txindex.cpp +3/-8
- src/index/blockfilterindex.cpp +4/-4
- src/index/txospenderindex.cpp +3/-5
- src/index/blockfilterindex.h +3/-2
- src/index/txospenderindex.h +3/-2
- src/index/coinstatsindex.cpp +2/-2
- src/index/coinstatsindex.h +1/-1
- src/index/txindex.h +1/-1
Card
The short text the ranking pass compares across PRs.
This PR modifies index initial synchronization to process block ranges in batches of up to 500 blocks rather than flushing LevelDB writes on every block. Currently, writing per-block causes heavy disk I/O on HDDs and creates excessive LevelDB files, while locking cs_main per block degrades node responsiveness during validation and IBD. The author claims this significantly reduces disk write overhead and lock contention during initial sync, while extracting foundation work for parallelized indexing in #26966. Depends on merged PR #34897.
Data
thread read through 2026-08-28 (142 statements), head 1497086cdb
labels: UTXO Db and Indexes, Needs rebase · draft
Categories
Indexes
P2 · speedup (ranking pass: Placed ahead of #26966 because it extracts the batching foundation that the parallel-sync PR builds on.)
Batches LevelDB index writes into 500-block windows and retrieves block ranges under a single cs_main lock acquisition, significantly reducing I/O and lock contention during initial index synchronization. The author notes this 'improves sync time on HDD due to the reduced number of disk write operations' and reduces cs_main contention to improve responsiveness during initial sync, while also laying groundwork for parallel index sync (#26966).
Membership: Touches `src/index/base.{cpp,h}` and all concrete index implementations (`txindex`, `blockfilterindex`, `txospenderindex`, `coinstatsindex`) to batch database writes and update synchronization logic.
Factors: security/stability 0, bug 0, performance 2, user value 1, leverage 1