Merge bitcoin/bitcoin#25193: indexes: Read the locator's top block during init, allow interaction with reindex-chainstate

[DashCoreAutoGuix]

Backports bitcoin#25193

Original Bitcoin commit: 4e8a765

Summary

This PR makes two improvements to the index init phase:

1. Prevent index corruption in case a reorg happens when the index was switched off: Read the top block from the locator instead of looking for fork point already in BaseIndex::Init(). The custom Rewind() method in ThreadSync() handles reverting to the fork point properly.

2. Allow using the -reindex-chainstate option without needing to disable indexes: With BaseIndex::Init() not calling FindForkInGlobalIndex() anymore, we can allow reindex-chainstate with active indexes.

Changes

• Added g_indexes_ready_to_sync flag to coordinate index sync with reindex-chainstate
• Modified BaseIndex::Init() to use LookupBlockIndex instead of FindForkInGlobalIndex
• Removed reindex-chainstate incompatibility checks with indexes
• Added wait loop in BaseIndex::ThreadSync() for reindex-chainstate to complete
• Updated functional tests accordingly

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Indexes can now be used concurrently with the -reindex-chainstate option, removing previous incompatibility restrictions.
  • Enhanced index synchronization logic to properly coordinate initialization during node startup and import processes.

• Chores

  • Refined help text for -prune option.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The PR introduces a synchronization mechanism via g_indexes_ready_to_sync to defer index initialization until after the initial chain import completes, allowing -reindex-chainstate to proceed without compatibility restrictions. It modifies index initialization logic, updates the startup sequence, and adjusts related tests accordingly.

Changes

Cohort / File(s)	Summary
Index synchronization mechanism src/node/blockstorage.h, src/node/blockstorage.cpp	Introduces new global atomic flag g_indexes_ready_to_sync initialized to false; flag is set to true at end of ThreadImport after mempool loading completes.
Index initialization logic src/index/base.cpp	Modifies best-block resolution in Init from FindForkInGlobalIndex to direct block lookup; adds guard around pruning/sanity checks; adds wait loop in ThreadSync that blocks until g_indexes_ready_to_sync becomes true.
Initialization flow updates src/init.cpp, src/test/util/setup_common.cpp	Adds using declaration for g_indexes_ready_to_sync; removes compatibility restrictions preventing -reindex-chainstate with -coinstatsindex, -blockfilterindex, and -txindex; updates pruning help text; sets flag to true during test setup.
Functional tests test/functional/feature_coinstatsindex.py, test/functional/p2p_blockfilters.py	Adds sync_index_node helper and _test_init_index_after_reorg test scenario; removes test assertion for init error on -blockfilterindex + -reindex-chainstate combination; updates reindex test flow to restart with both flags and validate consistency.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Attention areas:
  • Synchronization mechanism timing and correctness: verify g_indexes_ready_to_sync is set at appropriate point in ThreadImport
  • Impact of removing compatibility restrictions on -reindex-chainstate: ensure no unintended side effects from allowing previously-disallowed flag combinations
  • Index initialization delay logic in ThreadSync: confirm wait loop handles interruption correctly and doesn't introduce deadlocks
  • Test behavior changes: validate that new test scenarios properly exercise the deferred initialization path and that removed assertions no longer apply

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and specifically describes the main changes: improving index initialization to read the locator's top block during init and allowing interaction with reindex-chainstate, which directly reflects the primary objectives of preventing index corruption and enabling reindex-chainstate compatibility.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch backport-0.26-batch-477-pr-25193

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e63410e and a2d860f.

📒 Files selected for processing (7)

• src/index/base.cpp (3 hunks)
• src/init.cpp (4 hunks)
• src/node/blockstorage.cpp (2 hunks)
• src/node/blockstorage.h (1 hunks)
• src/test/util/setup_common.cpp (1 hunks)
• test/functional/feature_coinstatsindex.py (5 hunks)
• test/functional/p2p_blockfilters.py (0 hunks)

💤 Files with no reviewable changes (1)

• test/functional/p2p_blockfilters.py

🧰 Additional context used

📓 Path-based instructions (4)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

C++20 codebase should be placed under src/

Files:

• src/test/util/setup_common.cpp
• src/node/blockstorage.cpp
• src/index/base.cpp
• src/node/blockstorage.h
• src/init.cpp

src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

• src/test/util/setup_common.cpp

**

⚙️ CodeRabbit configuration file

**: # CodeRabbit AI Review Instructions for Dash Backports

Your Role

You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.

Critical Validation Rules

1. File Operations Must Match (AUTO-REJECT if violated)

• If Bitcoin modifies an existing file → Dash MUST modify (not create new)
• If Bitcoin creates a new file → Dash creates
• If Bitcoin deletes a file → Dash deletes
• Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys

2. Size Ratio Check (80-150% of Bitcoin)

• Count functional lines changed (exclude comments/whitespace)
• Dash changes should be 80-150% of Bitcoin's size
• Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash

3. No Scope Creep

• Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
• No unrelated refactoring or style changes
• Only Bitcoin's intended changes + minimal Dash adaptations

4. Bitcoin-Specific Code Detection

• Auto-reject witness/segwit code: msg_wtxidrelay, MSG_WTX, witness imports
• Auto-reject RBF (replace-by-fee) functionality
• Note: PSBT is supported in Dash (don't flag)

5. Mandatory Adaptations Only

• bitcoin → dash in strings/paths
• Bitcoin → Dash in user-facing text
• Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
• Hardcoded test values specific to Dash
• No other changes unless absolutely required

6. Completeness Check

• All files changed in Bitcoin must be present
• Extra files need clear justification (Dash-specific compatibility)
• Missing files = incomplete backport

Review Process

1. First: Check file operations match exactly
2. Second: Calculate size ratio
3. Third: Scan for scope creep patterns
4. Fourth: Detect Bitcoin-specific code
5. Fifth: Verify all changes are minimal adaptations

Output Format

VALIDATION: [PASS/FAIL]

File Operatio...

Files:

• src/test/util/setup_common.cpp
• src/node/blockstorage.cpp
• src/index/base.cpp
• src/node/blockstorage.h
• src/init.cpp
• test/functional/feature_coinstatsindex.py

test/functional/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Functional tests should be placed in test/functional/ and written in Python

Files:

• test/functional/feature_coinstatsindex.py

🧠 Learnings (3)

📓 Common learnings

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T19:54:21.426Z
Learning: In Dash backports from Bitcoin Core, including necessary compilation fixes (such as API compatibility changes like UniValue get_int() → getInt<int>()) alongside the core backport is standard and expected practice. These compatibility fixes ensure the backported code compiles in Dash's evolved codebase while preserving Bitcoin's original functionality and intent.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.

📚 Learning: 2025-07-31T01:14:55.631Z

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.

Applied to files:

• src/index/base.cpp

📚 Learning: 2025-07-28T20:34:29.061Z

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.

Applied to files:

• src/index/base.cpp

🧬 Code graph analysis (2)

src/index/base.cpp (2)

src/util/translation.h (1)

• Untranslated (48-48)

src/index/coinstatsindex.h (1)

• GetName (54-54)

test/functional/feature_coinstatsindex.py (3)

test/functional/test_framework/util.py (1)

• assert_equal (69-74)

test/functional/test_framework/wallet.py (2)

• generate (158-169)
• getnewdestination (323-337)

test/functional/test_framework/test_framework.py (1)

• no_op (789-790)

🪛 Ruff (0.14.7)

test/functional/feature_coinstatsindex.py

319-319: Missing return type annotation for private function _test_init_index_after_reorg

Add return type annotation: None

(ANN202)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: linux64_nowallet-build / Build source
• GitHub Check: linux64-build / Build source
• GitHub Check: arm-linux-build / Build source
• GitHub Check: Lint / Run linters
• GitHub Check: x86_64-w64-mingw32 / Build depends

🔇 Additional comments (15)

src/node/blockstorage.h (1)

51-54: New global flag declaration matches upstream design

Declaring g_indexes_ready_to_sync alongside fImporting/fReindex under node cleanly exposes the shared readiness flag without any Dash-specific deviations.

src/test/util/setup_common.cpp (1)

227-235: Test harness correctly initializes index-sync flag

Setting node::g_indexes_ready_to_sync = true; in BasicTestingSetup ensures index threads in tests never deadlock waiting for ThreadImport, matching upstream behavior and introducing no extra Dash logic.

src/node/blockstorage.cpp (2)

26-32: Global definition of g_indexes_ready_to_sync is consistent with other node flags

Defining g_indexes_ready_to_sync{false} alongside fImporting/fReindex matches the intended global state tracking with no extra Dash-specific behavior.

---

828-903: ThreadImport correctly signals index-sync readiness

Flipping g_indexes_ready_to_sync = true; immediately after LoadMempool(args); ensures optional indexes only begin syncing after block import and mempool loading complete. All earlier return paths already trigger shutdown, so waiting index threads will exit via their interrupt without hanging.

src/init.cpp (5)

140-163: Expose g_indexes_ready_to_sync in init via using

Adding using node::g_indexes_ready_to_sync; alongside other node:: aliases is the minimal way to let the init code control the new sync flag, with no behavioral differences from upstream beyond the expected Dash context.

---

574-577: Prune help-string tweak is aligned with upstream wording

Changing the -prune help text to use “>=” better reflects the actual semantics and mirrors upstream, without altering behavior.

---

590-595: Updated -reindex-chainstate help text matches new semantics

The new description (“Rebuild chain state from the currently indexed blocks… use full -reindex instead”) removes now-obsolete incompatibility warnings and matches the behavior introduced by the index-sync gating.

---

1935-1952: Use of fReindexChainState is consistent with index-sync gating

Capturing fReindexChainState = args.GetBoolArg("-reindex-chainstate", false); here feeds the Step 8 logic that decides whether to set g_indexes_ready_to_sync early or wait for ThreadImport, mirroring upstream structure.

---

2225-2234: Index startup correctly coordinates with -reindex-chainstate

The new block

// If reindex-chainstate was specified, delay syncing indexes until ThreadImport has reindexed the chain
if (!fReindexChainState) g_indexes_ready_to_sync = true;

ensures:

• Normal startup: indexes can sync immediately (flag set here).
• With -reindex-chainstate: indexes wait on the flag until ThreadImport completes the chainstate rebuild and sets it, preventing corruption or wasted work.

This matches upstream PR bitcoin#25193 behavior with no Dash-specific additions.

src/index/base.cpp (4)

17-22: Importing g_indexes_ready_to_sync into index code is necessary plumbing

using node::g_indexes_ready_to_sync; is the minimal wiring needed so BaseIndex can honor the new global readiness flag; it matches upstream and doesn’t change semantics beyond that.

---

69-81: Init now uses locator’s top block and fails cleanly if missing

The updated Init() logic:

• Reads the persisted locator and looks up locator.vHave.at(0) via m_chainstate->m_blockman.LookupBlockIndex.
• If the block isn’t found, it returns an InitError("<index>: best block of the index not found. Please rebuild the index.") instead of silently rewinding from an unknown fork point.
• Otherwise, it sets the best block to that exact index entry.

This is exactly the behavior introduced upstream to avoid ambiguous fork resolution and to give clear guidance when an index must be rebuilt.

---

83-122: Pruning checks correctly gated on index readiness flag

Wrapping the existing prune-compatibility logic in:

if (!m_synced && g_indexes_ready_to_sync) {
    ...
}

prevents premature “best block goes beyond pruned data” failures while -reindex-chainstate is rebuilding chainstate and indexes are intentionally out of sync. Once the flag is true, the original safety checks run unchanged. This matches upstream and adds no Dash-only semantics.

---

142-148: ThreadSync waits for g_indexes_ready_to_sync before syncing

The new loop:

while (!g_indexes_ready_to_sync) {
    if (!m_interrupt.sleep_for(std::chrono::milliseconds(500))) return;
}

ensures index background sync does not start until either:

• -reindex-chainstate has finished and ThreadImport sets the flag, or
• shutdown interrupts the thread.

This is the intended upstream behavior to make indexes compatible with -reindex-chainstate without corruption.

test/functional/feature_coinstatsindex.py (2)

79-81: Index sync helper and reindex/reorg test adjustments faithfully match Bitcoin commit

sync_index_node() and its use after -reindex / -reindex-chainstate restarts and before the reorg invalidation ensure the coinstats index is fully synced before assertions, exactly as in Bitcoin’s 4e8a765 change set, with no Dash-specific deviations. This is the expected adaptation alongside the new g_indexes_ready_to_sync core behavior.

Also applies to: 247-258, 279-283

---

51-57: New _test_init_index_after_reorg test is a direct, scope‑accurate backport

Adding _test_init_index_after_reorg() to run_test and implementing it to compare muhash with the index disabled during a reorg versus re-enabled later mirrors the upstream test logic line‑for‑line, without extra assertions or behavior beyond Bitcoin’s commit. No further Dash-specific changes are needed here.

Also applies to: 319-337

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}