#24230 indexes: Stop using node internal types and locking cs_main, improve sync logic
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #24230 indexes: Stop using node internal types and locking cs_main, improve sync logic | Ready | 0 | Mild | 951 + 201 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
Decouples secondary indexes (txindex, blockfilterindex, coinstatsindex, and txospenderindex) from node internals by eliminating direct references to CBlockIndex, CChain, and CChainState. Extracts background index synchronization into a new node::SyncChain function and avoids holding cs_main during index notifications and sync. Consolidates block append and rewind logic into unified notification callbacks and avoids starting sync threads when indexes are already caught up.
Problem: Secondary indexes directly query internal node types and hold cs_main during notification dispatch and synchronization, causing lock contention with block validation and blocking indexes from running out-of-process. In addition, separate sync and notification execution paths caused startup races under -reindex-chainstate (triggering redundant block rewinds and reappends) and briefly served stale data for disconnected blocks during chain reorganizations.
Evidence of importance:
- Commit c00719c2d9: 'Avoid a race condition in index startup where indexes set m_synced = true too early... causing those blocks to trigger the Rewind() call in the BaseIndex::BlockConnected notification handler, and pointlessly remove and then re-append the same blocks. This race condition was easy to reproduce just by running the test/functional/feature_coinstatsindex.py...'
- Commit ce3f249009: 'Previously, during a reorg window, gettxspendingprevout would return stale (pre-reorg) index data until the new chain tip arrived; now the index reflects disconnections immediately.'
- Commit 84bb3a75ae: 'It is also a potential performance improvement since it avoids holding cs_main while processing all indexing notifications. Previously cs_main was held while sending some notifications in the attachChain sync thread.'
Scope: A substantial portion of the diff consists of moving sync and notification registration code from src/index/base.cpp to src/node/chain.cpp and src/node/interfaces.cpp, alongside mechanical conversions of CBlockIndex pointers to interfaces::BlockRef and interfaces::BlockInfo.
Risk of a mistake: medium (validation, other). Altering index notification synchronization and removing cs_main locks could introduce thread-safety issues, deadlocks, or index state corruption during chain reorgs or node shutdown.
Changed since the previous assessment: Rebased onto recent master changes, moving BlockInfo back from kernel to interfaces and MakeBlockInfo to node (reversing part of #34079), while updating txospenderindex wiring and tests.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: planned cleanups in later commits before ready for full review 2026-01-26
- author: answer furszy's question regarding block filter index copying extra entry 2024-04-01
Agreement
Mild
objection that needs a change was answered, no reply from the objector (furszy: swallowing ReverseBlock() return value could hide errors if something is wrong with the index)
Thread notes: Ryanofsky posted multiple rebase and update descriptions resolving conflicts and CI failures across iterations up to pr/indexy.92, noting the PR is mostly ready for review but still pending minor cleanups in later commits.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| mzumsande (member) | objection, correctness | bitcoind coredumps with leveldb assertion failure when invalidateblock is called during index sync | fixed · fix pushed · author fixed the bug in rebase 72c29827; mzumsande confirmed resolved | changed | yes | 2022-02-15: 'If, at this commit, I am in the middle of syncing an index and at the same time call `invalidateblock` for the tip, my node coredumps' Status from: 2022-02-18: mzumsande: 'Yes, I don't encounter this anymore now.' |
| mzumsande (member) | objection, correctness | index sync blocks node operations and prevents graceful abort with SIGINT | accepted · reviewer could not reproduce upon re-testing and dropped the objection | changed | yes | 2022-03-05: 'Index Sync now seems to block everything else and makes it impossible to abort gracefully' Status from: 2022-03-11: mzumsande: 'I tried to reproduce this again, but couldn't anymore. I must have made some error during my tests the last time.' |
| furszy (member) | objection, correctness | race condition allows block connection notifications while ThreadSync is running during startup | fixed · fix pushed · fixed in commit c553d9f0b0f; furszy confirmed | changed | yes | 2023-08-02: 'Which would enable the index new block connection signals reception at the same time that `ThreadSync()` is being executed.' Status from: 2023-08-04: furszy: 'Yeah, we are sync.' |
| furszy (member) | objection, correctness | swallowing ReverseBlock() return value could hide errors if something is wrong with the index | answered · Author acknowledged the catch and requested handling it in a follow-up PR to reduce complexity. | changed | yes | 2023-08-30: 'Seems that we are swallowing `ReverseBlock()` return value. When it could mean that there is something wrong with the index.' Status from: 2023-09-05: ryanofsky: 'Good catch. It would be great if you want to follow up in different PR to reduce complexity here.' |
| furszy (member) | question, correctness | why block.height - 1 is used when copying height index to hash index, which seems to copy an extra entry | open | answered | no | 2024-04-01: 'why block.height - 1? It seems that the block filter index CopyHeightIndexToHashIndex will copy an extra entry to the hash index.' |
| sedited (member) | suggestion, maintenance | move FindTx into the block manager so it can be called through the interface without being passed around | open | answered | no | 2024-08-31: 'I feel like the entire `FindTx` function could just be moved into the block manager. Then there is no need to pass it around here and it can instead be called through the interface just like elsewhere' |
| furszy (member) | objection, correctness | last cached header is incorrectly set to the disconnected block during rewind | accepted · reviewer pushed fix in follow-up PR #32694 | changed | no | 2025-06-06: 'This doesn't seem to be correct. The last cached header is being set to the block we just disconnected.' Status from: 2025-10-15: furszy: 'This can be closed. I pushed it in https://github.com/bitcoin/bitcoin/pull/32694.' |
Support:
- fjahr (member) (Concept ACK, 2022-02-13): Concept ACK, will start reviewing soon [not substantive]
- mzumsande (member) (Concept ACK, 2022-03-05) [read]: Concept ACK, made it through all commits [not substantive]
- aureleoules (contributor) (Concept ACK, 2023-01-17): Concept ACK [not substantive]
- sedited (member) (Concept ACK, 2023-12-22): Concept ACK [not substantive]
- sedited (member) (Approach ACK, 2024-01-03) [read]: This change looks good, albeit its daunting length.
- josibake (member) (Concept ACK, 2024-01-28) [read: src/index/base.cpp]: Found this from https://github.com/bitcoin/bitcoin/blob/5fbcc8f0560cce36abafb8467339276b7c0d62b6/src/index/base.cpp#L87 while working with the indexing code.
- sedited (member) (2026-01-21) [read]: I still think it would be a good idea.
Participants:
- Sjors (member, question, 19 statements 2022-02-08 to 2024-07-23): reviewed intermediate commits, asked design questions, and suggested reorganizations
- fjahr (member, support, 1 statements 2022-02-13 to 2022-02-13): Concept ACK
- mzumsande (member, objection, 24 statements 2022-02-15 to 2022-06-24): Concept ACK; reported crash on invalidateblock during sync and graceful abort issue (both resolved)
- jonatack (member, neutral, 1 statements 2022-03-09 to 2022-03-09): noted plan to review
- maxim85200 (none, neutral, 1 statements 2022-03-13 to 2022-03-13): commented
- jamesob (contributor, question, 32 statements 2022-03-18 to 2023-07-15): partial review; raised questions on intermediate commits and suggested review strategies
- maflcko (member, neutral, 6 statements 2022-06-15 to 2023-08-17): pointed out compiler warning, syntax error, and CI failures
- aureleoules (contributor, support, 1 statements 2023-01-17 to 2023-01-17): Concept ACK and refactoring suggestion
- furszy (member, objection, 23 statements 2023-08-02 to 2025-10-15): reviewed extensively; identified race condition at startup and swallowed return values (both resolved); asked question on block filter index
- sedited (member, support, 18 statements 2023-12-18 to 2026-01-21): Concept and Approach ACK; reaffirmed support in 2026; suggested moving FindTx into block manager
- josibake (member, support, 3 statements 2024-01-28 to 2024-08-07): Concept ACK with interest in kernel and multiprocess indexing improvements
Dependencies
Files
288 lines under test/bench/ci.
- src/index/base.cpp +305/-306
- src/node/interfaces.cpp +189/-11
- src/index/base.h +77/-52
- src/node/chain.cpp +124/-0
- src/test/baseindex_tests.cpp +54/-23
- src/node/chain.h +67/-0
- src/index/blockfilterindex.cpp +34/-25
- src/test/util/index.cpp +39/-0
- src/interfaces/types.h +36/-0
- src/index/blockfilterindex.h +18/-15
- src/test/util/index.h +32/-0
- src/interfaces/chain.h +31/-0
- src/kernel/chain.h +0/-28
- test/functional/feature_init.py +23/-4
- src/test/blockfilter_index_tests.cpp +12/-12
- src/test/txindex_tests.cpp +12/-10
- src/kernel/chain.cpp +0/-21
- src/interfaces/handler.h +16/-3
- src/index/txindex.cpp +10/-7
- src/test/coinstatsindex_tests.cpp +7/-5
- src/test/util/validation.cpp +2/-10
- src/test/util/validation.h +0/-11
- src/rpc/blockchain.cpp +5/-5
- src/index/coinstatsindex.cpp +4/-4
- src/index/txospenderindex.cpp +4/-4
- src/net_processing.cpp +4/-4
- src/index/txindex.h +6/-1
- src/wallet/test/rescan_tests.cpp +4/-3
- src/index/coinstatsindex.h +3/-3
- src/index/txospenderindex.h +5/-1
- src/test/txospenderindex_tests.cpp +4/-2
- src/bench/index_blockfilter.cpp +4/-1
- src/bench/wallet_create_tx.cpp +3/-2
- src/rest.cpp +3/-2
- src/util/btcsignals.h +5/-0
- src/init.cpp +2/-2
- test/functional/rpc_gettxspendingprevout.py +2/-2
- test/functional/test_framework/test_node.py +2/-2
- src/common/interfaces.cpp +2/-1
- src/CMakeLists.txt +1/-0
- src/test/util/CMakeLists.txt +1/-0
- src/validationinterface.h +0/-1
Card
The short text the ranking pass compares across PRs.
Decouples all secondary indexes from internal node types like CBlockIndex and CChainState, moving chain sync logic out of BaseIndex into node::SyncChain and dispatching notifications without holding cs_main. Consolidating the synchronization and notification paths fixes an index startup race under -reindex-chainstate that caused redundant rewinds, and eliminates a window where gettxspendingprevout served stale data during reorgs. Moving index processing behind interfaces is a prerequisite for running indexes in separate processes (#10102) and sharing the chain sync mechanism with wallet rescans (#15719).
Data
thread read through 2026-01-26 (274 statements), head 7e6c3fb04a
labels: UTXO Db and Indexes · draft
Categories
Indexes
P1 · cleanup (assessed alone as P2; Raised to P1: fixes a -reindex-chainstate startup race and stale-data-during-reorg window (category's top concern, correctness under reorgs) while being the stated prerequisite for out-of-process indexes (#10102) and shared chain sync (#15719); it cannot sit in the same band as the ordinary speedups it enables.)
Decouples secondary indexes from internal node types (CBlockIndex, CChain, CChainState) and drops cs_main locking during sync and notifications. The PR also directly targets correctness during reorgs and initial sync, fixing an index startup race under -reindex-chainstate and eliminating stale index data returns during reorg windows.
Membership: Matches the 'UTXO Db and Indexes' label and modifies BaseIndex, txindex, blockfilterindex, coinstatsindex, and txospenderindex across src/index/.
Factors: security/stability 1, bug 1, performance 1, user value 1, leverage 2