feat!: migrate additional indexes from synchronous to async operation #7101
Conversation
Migrates TimestampIndex to follow the BaseIndex async pattern (similar to TxIndex),
enabling background syncing without blocking validation.
Key changes:
- Move data structures from src/timestampindex.h to src/index/timestampindex.h
- Implement async TimestampIndex class inheriting from BaseIndex
- Create separate database at indexes/timestampindex/ (previously in blocks/index/)
- Remove synchronous writes from validation (ConnectBlock, DisconnectBlock, RollforwardBlock)
- Update getblockhashes RPC to use async index with BlockUntilSyncedToCurrentChain()
- Remove global fTimestampIndex flag and ERROR_TIMEIDX_NEEDS_REINDEX
- Add cache allocation in node/caches.{h,cpp}
- Update initialization/shutdown in init.cpp
Benefits:
- Index can be toggled on/off without requiring -reindex
- Background syncing doesn't block block validation
- Works with pruned nodes (only stores block metadata)
- Consistent with other async indexes (txindex)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Updates feature_timestampindex.py to validate async index capabilities: - Test that index can be disabled without -reindex - Test that index can be re-enabled without -reindex (syncs in background) - Use bump_mocktime() to ensure distinct timestamps for range queries - Verify correct RPC behavior after toggling These tests validate key advantages of the async migration over the old synchronous implementation, where changing the index required a full blockchain reindex. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
This commit migrates SpentIndex to use the async BaseIndex pattern,
similar to the TimestampIndex migration.
Key changes:
- Create src/index/spentindex.{h,cpp} with async SpentIndex class
- Use CBlockUndo to access historical spent output data
- Incompatible with pruned nodes (requires undo data)
- Database location: indexes/spentindex/ (128MB cache)
- Remove synchronous writes from validation.cpp
- Remove database methods from txdb.{h,cpp}
- Remove global fSpentIndex flag from blockstorage.{h,cpp}
- Remove validation check from chainstate.{h,cpp}
- Update RPC handlers (getspentinfo, getrawtransaction)
- Move CAddressUnspent* structures to addressindex.h
- Fix locking: query async index before acquiring cs_main in TxToJSON
Implementation details:
- Uses undo data via UndoReadFromDisk() for historical UTXO access
- Cache size: 128MB (~1.2M entries, proportional to TxIndex)
- Properly handles both mempool and confirmed transactions
- Background syncing with BlockUntilSyncedToCurrentChain()
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Update the spentindex functional test to reflect that SpentIndex now builds asynchronously and no longer requires -reindex to enable. Changes: - Remove test for ERROR_SPENTIDX_NEEDS_REINDEX (no longer applicable) - Remove ErrorMatch import (no longer needed) - Update test description to reflect async enable/disable behavior - SpentIndex can now be enabled without -reindex like other async indexes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Migrates AddressIndex from synchronous writes during block validation
to asynchronous operation using the BaseIndex framework.
Key changes:
- Create async AddressIndex class in src/index/addressindex.{h,cpp}
- Dual database architecture: transaction history + unspent outputs
- Implement Rewind() and BlockDisconnected() for proper reorg handling
- Update RPC handlers (getaddressutxos, getaddressdeltas, getaddressbalance,
getaddresstxids) to use g_addressindex global
- Remove synchronous writes from validation.cpp
- Remove database methods from txdb.{h,cpp}
- Remove global fAddressIndex flag from blockstorage
- Remove validation check from chainstate initialization
- Add 256MB cache allocation (larger than SpentIndex due to dual databases)
- Add to Makefile.am build system
Updates the AddressIndex functional test to reflect async operation: - AddressIndex can now be enabled/disabled without -reindex - Test already includes comprehensive reorg testing that verifies proper cleanup of both transaction history and unspent index
Add a template method to compact a specific key range instead of the entire database. This enables incremental compaction during batch operations to reclaim disk space progressively. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add CBlockTreeDB::MigrateOldIndexData() to migrate old synchronous index data from the block index database to new async index databases during node startup. This preserves existing index data so users don't need to rebuild indexes after upgrading. Key features: - Migrates data for each index independently (timestampindex, spentindex, addressindex) - Copies old entries from blocks/index to new databases under indexes/* - Uses batched writes to handle millions of entries efficiently - Writes DB_BEST_BLOCK metadata from chainstate so indexes know they're synced - Erases old data from blocks/index after successful migration - Compacts blocks/index database after each index to reclaim disk space progressively - Automatically triggered during LoadBlockIndexDB() - Only runs once - subsequent startups will find no old data This migration significantly improves upgrade experience by avoiding hours of index rebuilding on mainnet nodes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
|
WalkthroughThis pull request refactors address, spent, and timestamp indexing from synchronous to asynchronous operation within the BaseIndex framework. It introduces three new index classes ( Sequence DiagramssequenceDiagram
participant Block as Block Chain
participant Index as AsyncIndex<br/>(WriteBlock)
participant UndoData as Undo Data
participant DB as Index DB
participant Storage as Persistent Storage
Block->>Index: WriteBlock(block, blockindex)
Index->>UndoData: Read undo data for block
UndoData-->>Index: Undo entries
Index->>Index: Parse inputs (spends)
Index->>Index: Parse outputs (receipts)
Index->>DB: WriteBatch(entries)
DB->>Storage: Serialize & write keys/values
Storage-->>DB: Confirm write
DB-->>Index: Success
Index-->>Block: true
sequenceDiagram
participant Chain as Chain Manager
participant Index as AsyncIndex<br/>(Rewind)
participant Storage as Persistent Storage
participant DB as Index DB
Chain->>Index: Rewind(current_tip, new_tip)
Index->>Index: Validate ancestry
Index->>Storage: Read blocks being rewound
Storage-->>Index: Block data
Index->>Index: Iterate blocks in reverse<br/>read undo data
Index->>Index: Build erase/restore entries
Index->>DB: EraseAddressIndex(entries)
Index->>DB: UpdateUnspentIndex(entries)
DB->>Storage: Remove/update persisted state
Index->>Chain: BaseIndex::Rewind()<br/>(update best block)
Index-->>Chain: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The diff spans substantial architectural refactoring across 40+ files, introducing three complete asynchronous index implementations with database layers, migrating legacy index data, replacing RPC utilities, updating initialization flows, and modifying test expectations. While individual index implementations follow similar patterns (reducing review complexity), the heterogeneity of changes—new classes, removed classes, build updates, initialization changes, RPC integration changes, migration logic, and test updates—demands careful verification of data flow integrity, index consistency semantics, and proper initialization sequencing. The density of logic in DB batch operations, rewind paths, and block processing makes this a significant review undertaking. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/functional/feature_spentindex.py (1)
6-8: Incorrect file header comment.The docstring says "Test addressindex generation and fetching" but this file is
feature_spentindex.pywhich tests the spent index.# -# Test addressindex generation and fetching +# Test spentindex generation and fetching #
🤖 Fix all issues with AI agents
In @src/index/addressindex_types.h:
- Around line 8-19: The header src/index/addressindex_types.h is failing CI
formatting; run the repository's clang-format-diff.py flow to auto-apply the
project's clang-format style to this file (targeting the current patch/commit),
regenerate the diff, and commit the updated formatting so the
BITCOIN_INDEX_ADDRESSINDEX_TYPES_H header guard and the include block (e.g.,
consensus/amount.h, script/script.h, serialize.h, uint256.h, util/underlying.h
and the chrono/tuple/vector includes) conform to the project's style; ensure you
use the repo's specified clang-format binary/version when running the script
before pushing.
In @src/index/addressindex.cpp:
- Around line 61-77: The iterator loop over pcursor (checking DB_ADDRESSINDEX
and CAddressIndexKey::m_address_bytes == address_hash) lacks validation of the
address type and may include entries with the same hash but different
CAddressIndexKey::m_address_type; update the condition that currently checks
key.first == DB_ADDRESSINDEX && key.second.m_address_bytes == address_hash to
also require key.second.m_address_type == expected_type (or compare against the
provided address type variable), so only matching type+hash pairs are processed
before pushing into entries and advancing pcursor.
- Around line 90-103: The loop in the ReadAddressUnspentIndex-like logic only
checks key.first == DB_ADDRESSUNSPENTINDEX and key.second.m_address_bytes ==
address_hash, but never validates the address type; update the if condition to
also require key.second.type == type (or the actual function parameter name for
the requested address type) so only entries matching both address bytes and
address type are collected; ensure you still call pcursor->Next() on success and
return the same error on GetValue failure.
In @src/index/timestampindex.cpp:
- Around line 5-10: The file is missing the declaration for error(...), causing
build failures in TimestampIndex::Rewind and TimestampIndex::BlockDisconnected;
add the missing include by adding #include <util/error.h> alongside the existing
includes at the top of timestampindex.cpp so the calls to error(...) resolve
correctly.
- Around line 90-103: BlockDisconnected currently calls Rewind(best_block_index,
pindex->pprev) without ensuring pindex->pprev is non-null which can trigger an
assert in Rewind; add a defensive guard in TimestampIndex::BlockDisconnected to
check that pindex->pprev != nullptr before calling Rewind (if null, log an
error/warning with __func__ and GetName() and skip the Rewind), keeping existing
behavior when pprev is valid.
In @src/init.cpp:
- Around line 1213-1220: Add a runtime guard during index startup to catch
already-pruned datadir cases: when initializing address and spent indexes (the
code around the index start block at ~2277), check
chainman.m_blockman.m_have_pruned and if true return an InitError (or otherwise
abort with a clear message) indicating pruning is incompatible with enabling
-addressindex or -spentindex; ensure you reference the existing flag checks for
args.GetBoolArg("-addressindex") and args.GetBoolArg("-spentindex") so the new
check triggers even when -prune=0 but the on-disk datadir is already pruned.
In @src/rpc/node.cpp:
- Around line 814-843: Validate that outputIndex is non-negative before
constructing CSpentIndexKey: if outputIndex < 0, throw
JSONRPCError(RPC_INVALID_PARAMETER, "Invalid outpoint index"); then build
CSpentIndexKey(txid, outputIndex) and proceed with the existing async index
check (g_spentindex->GetSpentInfo) and mempool overlay (EnsureAnyMemPool +
mempool.getSpentIndex) logic unchanged; keep the current final error
throw(JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unable to get spent info")) when
neither source finds a value, and optionally add a distinct RPC error only if
GetSpentInfo can explicitly signal “not yet synced” (handle that case where
GetSpentInfo provides such a status).
In @src/txdb.h:
- Around line 93-98: Add the EXCLUSIVE_LOCKS_REQUIRED(::cs_main) annotation to
the public declaration of MigrateOldIndexData in src/txdb.h to enforce that
callers must hold ::cs_main (e.g., change the declaration of MigrateOldIndexData
to include EXCLUSIVE_LOCKS_REQUIRED(::cs_main)); this matches the existing
annotation on LoadBlockIndexGuts and reflects that MigrateOldIndexData reads the
coins DB (DB_BEST_BLOCK) and is only called from LoadBlockIndexDB which holds
cs_main.
In @test/util/data/non-backported.txt:
- Around line 21-23: The file lists header patterns for new index sources but
misses the corresponding source patterns; update the list (where you have
src/index/addres*.h, src/index/spent*.h, src/index/timestamp*.h) to also include
the matching .cpp patterns so clang-format covers source files too — add
src/index/addres*.cpp, src/index/spent*.cpp and src/index/timestamp*.cpp to the
same list.
🧹 Nitpick comments (13)
src/index/spentindex_types.h (1)
19-39: LGTM! Well-structured key type.The key design with
m_tx_hashandm_tx_indexcorrectly identifies spent outputs.Minor consideration: The codebase uses
VARINTfor similar index fields (e.g.,CoinEntryintxdb.cppline 65). UsingVARINTform_tx_indexcould reduce storage for typical low-value indices, though the current fixed-width approach is also valid.src/index/spentindex.h (1)
76-76: Consider usingconst CSpentIndexKey&for the lookup parameter.The
keyparameter appears to be input-only for the lookup operation. Usingconst CSpentIndexKey& keywould better express the intent and allow callers to pass temporaries or const objects.♻️ Suggested signature change
- bool GetSpentInfo(CSpentIndexKey& key, CSpentIndexValue& value) const; + bool GetSpentInfo(const CSpentIndexKey& key, CSpentIndexValue& value) const;src/index/spentindex.cpp (1)
148-161: Consider logging severity for rewind failures.When
Rewindfails inBlockDisconnected, the error is logged but the index may be left in an inconsistent state. This could lead to stale or incorrect data being served byGetSpentInfo.Consider whether a more severe action (e.g., marking the index as requiring rebuild) would be appropriate, or document that the index may become inconsistent after a failed rewind.
src/txdb.cpp (1)
492-502: Consider using a small cache for chainstate read.The
CDBWrapperfor reading the best block is created with cache size 0. While this works, a small cache (e.g., 8 MiB) could improve read performance, especially if the coins database needs to warm up.- CDBWrapper coins_db(chainstate_path, 0, false, false); + CDBWrapper coins_db(chainstate_path, 8 << 20 /* 8 MiB */, false, false);src/rpc/rawtransaction.cpp (2)
79-131: Consider consolidating duplicate loop logic.The code iterates over
tx.vinandtx.vouttwice—once for the index query and once for the mempool query. While functionally correct (mempool overwrites index entries), this could be simplified by querying both sources in a single pass.That said, the current structure clearly separates the two data sources, which aids readability and maintains the semantic that mempool data takes precedence.
86-94: Minor: Redundant coinbase check inside loop.
tx.IsCoinBase()is checked for every input iteration, but it only needs to be checked once. Consider hoisting the check outside the loop.+ const bool is_coinbase = tx.IsCoinBase(); // Collect spent info for inputs for (const auto& txin : tx.vin) { - if (!tx.IsCoinBase()) { + if (!is_coinbase) { CSpentIndexValue spentInfo;src/index/addressindex_types.h (1)
235-306: Unspent key/value types look consistent; consider movingCScriptto avoid copies
CAddressUnspentKeyfixed-size serialization andCAddressUnspentValuenull semantics look fine. Minor improvement: avoid copyingCScriptin the value ctor.Proposed diff (move CScript)
- CAddressUnspentValue(CAmount amount, CScript tx_script, int32_t block_height) : - m_amount{amount}, - m_tx_script{tx_script}, + CAddressUnspentValue(CAmount amount, CScript tx_script, int32_t block_height) : + m_amount{amount}, + m_tx_script{std::move(tx_script)}, m_block_height{block_height} {};src/rpc/node.cpp (3)
411-436: Consider loosening the hard dependency ong_addressindexfor mempool-only results, or explicitly justify it.At Line 413-415 you reject the RPC when
-addressindexis disabled, but the implementation is purelymempool.getAddressIndex(...)(Line 430) and doesn’t queryg_addressindex. If the intended policy is “these RPCs are only available when addressindex is enabled” then this is fine, but it would be good to make that explicit in a short comment because the guard otherwise looks accidental.
493-509: Prefer consistent “index not synced” error whenGetAddressUnspentIndex()returns false.With async indexes, a
falsereturn may mean “not synced yet / interrupted” rather than “no address info”. Right now you throw"No information available for address"(Line 507), which can be misleading during startup/IBD. Consider emitting a dedicated message (similar togetaddressbalance’s"Address index not yet synced").
586-595: Avoid mutatingstart/endinside the per-address loop (surprising behavior).At Line 590 you normalize
start/endfor every address iteration. For multi-address queries this can silently change query semantics after the first address. Better: normalize once before the loop (or keep per-address local copies).src/index/timestampindex.cpp (1)
25-44:ReadRange()should either clearhashesor document “appends to output”.Right now
ReadRange()always appends to the caller-provided vector. That’s fine if intended, but it’s easy to accidentally reuse a vector and get unexpected results. A one-line comment (orhashes.clear()if callers always expect a fresh result) would prevent surprises.src/index/addressindex.h (1)
8-14: Make the header self-contained and tighten encapsulation (final class).
- This header uses
std::unique_ptrbut doesn’t include<memory>. Relying on transitive includes is fragile.- Since
AddressIndexisfinal,m_dbdoesn’t need to beprotected.Proposed fix
#include <index/addressindex_types.h> #include <index/base.h> +#include <memory> #include <vector> @@ class AddressIndex final : public BaseIndex { protected: class DB; - const std::unique_ptr<DB> m_db; +private: + const std::unique_ptr<DB> m_db; +protected: class DB : public BaseIndex::DBAlso applies to: 23-28
src/index/addressindex.cpp (1)
115-124: Consider suppressing unused variable warning.The
valuein the structured binding at line 119 is unused. While this works, using a placeholder or attribute would be cleaner.-for (const auto& [key, value] : entries) { +for (const auto& [key, _] : entries) { batch.Erase(std::make_pair(DB_ADDRESSINDEX, key)); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
doc/release-notes-7101.mdsrc/Makefile.amsrc/addressindex.cppsrc/core_write.cppsrc/dbwrapper.hsrc/index/addressindex.cppsrc/index/addressindex.hsrc/index/addressindex_types.hsrc/index/addressindex_util.cppsrc/index/addressindex_util.hsrc/index/spentindex.cppsrc/index/spentindex.hsrc/index/spentindex_types.hsrc/index/timestampindex.cppsrc/index/timestampindex.hsrc/index/timestampindex_types.hsrc/init.cppsrc/node/blockstorage.cppsrc/node/blockstorage.hsrc/node/caches.cppsrc/node/caches.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/rpc/blockchain.cppsrc/rpc/index_util.cppsrc/rpc/index_util.hsrc/rpc/node.cppsrc/rpc/rawtransaction.cppsrc/spentindex.hsrc/test/util/setup_common.cppsrc/timestampindex.hsrc/txdb.cppsrc/txdb.hsrc/txmempool.cppsrc/txmempool.hsrc/validation.cppsrc/zmq/zmqpublishnotifier.cpptest/functional/feature_addressindex.pytest/functional/feature_spentindex.pytest/functional/feature_timestampindex.pytest/util/data/non-backported.txt
💤 Files with no reviewable changes (9)
- src/test/util/setup_common.cpp
- src/spentindex.h
- src/node/chainstate.cpp
- src/node/blockstorage.h
- src/addressindex.cpp
- src/timestampindex.h
- src/rpc/index_util.h
- src/node/chainstate.h
- src/rpc/index_util.cpp
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/core_write.cppsrc/index/addressindex_util.hsrc/index/timestampindex_types.hsrc/node/caches.cppsrc/node/blockstorage.cppsrc/dbwrapper.hsrc/validation.cppsrc/rpc/blockchain.cppsrc/txdb.cppsrc/txmempool.cppsrc/rpc/rawtransaction.cppsrc/node/caches.hsrc/index/spentindex_types.hsrc/txmempool.hsrc/index/spentindex.hsrc/index/addressindex.cppsrc/index/timestampindex.cppsrc/index/addressindex_types.hsrc/index/addressindex.hsrc/index/addressindex_util.cppsrc/rpc/node.cppsrc/zmq/zmqpublishnotifier.cppsrc/index/timestampindex.hsrc/index/spentindex.cppsrc/txdb.hsrc/init.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/feature_timestampindex.pytest/functional/feature_addressindex.pytest/functional/feature_spentindex.py
src/node/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
NodeContext must be extended with Dash-specific managers for each subsystem during initialization
Files:
src/node/caches.cppsrc/node/blockstorage.cppsrc/node/caches.h
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
doc/release-notes-7101.md
🧠 Learnings (30)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/core_write.cppsrc/node/caches.cppsrc/validation.cppsrc/rpc/blockchain.cppsrc/txdb.cppsrc/txmempool.cppsrc/rpc/rawtransaction.cpptest/util/data/non-backported.txtsrc/index/spentindex_types.hsrc/txmempool.hsrc/Makefile.amsrc/index/addressindex.cppsrc/rpc/node.cppsrc/index/spentindex.cppsrc/txdb.hsrc/init.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/**/*.{cpp,h} : Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Applied to files:
src/core_write.cppsrc/rpc/blockchain.cppsrc/rpc/rawtransaction.cpptest/util/data/non-backported.txtsrc/txmempool.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/core_write.cppsrc/node/caches.cppsrc/node/blockstorage.cppsrc/validation.cppsrc/txdb.cpptest/util/data/non-backported.txtsrc/txmempool.hsrc/index/spentindex.hsrc/Makefile.amsrc/index/addressindex.cppsrc/rpc/node.cppsrc/index/spentindex.cppsrc/txdb.hsrc/init.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/core_write.cppsrc/node/blockstorage.cppsrc/rpc/blockchain.cppsrc/txdb.cpptest/util/data/non-backported.txtsrc/Makefile.amsrc/txdb.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/core_write.cppsrc/node/caches.cppsrc/txdb.cppsrc/txmempool.hsrc/txdb.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/core_write.cppsrc/rpc/blockchain.cppsrc/rpc/rawtransaction.cpptest/util/data/non-backported.txtsrc/txmempool.hsrc/Makefile.amsrc/zmq/zmqpublishnotifier.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
test/functional/feature_timestampindex.pytest/functional/feature_spentindex.py
📚 Learning: 2025-12-22T15:42:48.595Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7068
File: src/qt/guiutil_font.h:68-77
Timestamp: 2025-12-22T15:42:48.595Z
Learning: In C++/Qt codebases, use fail-fast asserts in setters to enforce invariants (e.g., ensuring internal maps contain necessary keys). Prefer assert() for programmer errors that should be caught in development and debugging, rather than defensive runtime checks with fallbacks. This helps catch invariant violations early during development. Apply to header and source files where invariant-driven state mutations occur, especially in setters like SetWeightBold/SetWeightNormal that assume established relationships or preconditions.
Applied to files:
src/index/addressindex_util.hsrc/index/timestampindex_types.hsrc/dbwrapper.hsrc/node/caches.hsrc/index/spentindex_types.hsrc/txmempool.hsrc/index/spentindex.hsrc/index/addressindex_types.hsrc/index/addressindex.hsrc/index/timestampindex.hsrc/txdb.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h} : Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Applied to files:
src/node/caches.cpptest/util/data/non-backported.txtsrc/node/caches.hsrc/Makefile.am
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
Repo: dashpay/dash PR: 6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/node/blockstorage.cppsrc/txdb.cppsrc/txdb.h
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/node/blockstorage.cpp
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/validation.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/validation.cppsrc/rpc/blockchain.cppsrc/txmempool.cppsrc/rpc/rawtransaction.cpptest/util/data/non-backported.txtsrc/txmempool.hsrc/Makefile.am
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: In Dash Core, `DEFAULT_SYNC_MEMPOOL` belongs with node synchronization logic (currently in src/masternode/sync.h) rather than with validation logic in src/validation.h, even though other mempool-related constants are in validation.h. The constant is conceptually tied to sync functionality, not validation.
Applied to files:
src/validation.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/validation.cppsrc/rpc/blockchain.cppsrc/txdb.cppsrc/Makefile.amsrc/rpc/node.cppsrc/init.cpp
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/txdb.cppsrc/index/addressindex_types.hsrc/txdb.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Dash extends Bitcoin Core through composition with minimal changes to the Bitcoin Core foundation
Applied to files:
src/txdb.cppsrc/txdb.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/txmempool.cppsrc/rpc/rawtransaction.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/txmempool.cppsrc/rpc/rawtransaction.cpptest/util/data/non-backported.txtsrc/txmempool.h
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/rpc/rawtransaction.cppsrc/rpc/node.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Applied to files:
src/rpc/rawtransaction.cppsrc/Makefile.am
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/rpc/rawtransaction.cpp
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/rpc/rawtransaction.cpp
📚 Learning: 2025-12-01T18:13:21.314Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 7018
File: test/lint/lint-github-actions.py:1-9
Timestamp: 2025-12-01T18:13:21.314Z
Learning: In the Dash repository, the file test/util/data/non-backported.txt should only include C++ files (.cpp or .h) because it is used for running clang-format. Other file types (such as Python files, .ui files, etc.) should not be added to this list.
Applied to files:
test/util/data/non-backported.txt
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
test/util/data/non-backported.txt
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
test/util/data/non-backported.txtsrc/Makefile.am
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Applied to files:
test/util/data/non-backported.txt
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/txmempool.hsrc/Makefile.amsrc/txdb.h
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/txdb.h
🧬 Code graph analysis (14)
test/functional/feature_timestampindex.py (2)
test/functional/test_framework/test_framework.py (4)
start_node(635-644)no_op(789-790)sync_all(862-864)bump_mocktime(866-883)test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/index/addressindex_util.h (1)
src/index/addressindex_util.cpp (2)
AddressBytesFromScript(12-28)AddressBytesFromScript(12-12)
test/functional/feature_addressindex.py (1)
test/functional/test_framework/test_framework.py (1)
sync_all(862-864)
src/rpc/rawtransaction.cpp (2)
src/bitcoin-tx.cpp (1)
txin(263-263)src/core_write.cpp (1)
spentKey(269-269)
src/index/spentindex_types.h (1)
src/txdb.cpp (1)
SERIALIZE_METHODS(66-66)
src/txmempool.h (1)
src/txmempool.cpp (2)
getAddressIndex(583-595)getAddressIndex(583-584)
src/index/addressindex.cpp (1)
src/index/addressindex_util.cpp (2)
AddressBytesFromScript(12-28)AddressBytesFromScript(12-12)
test/functional/feature_spentindex.py (1)
src/rpc/blockchain.cpp (1)
unspent(2576-2576)
src/index/timestampindex.cpp (2)
src/index/timestampindex_types.h (1)
CTimestampIndexIteratorKey(18-19)src/index/timestampindex.h (1)
BlockDisconnected(49-52)
src/index/addressindex.h (1)
src/index/addressindex.cpp (25)
AddressIndex(141-144)AddressIndex(146-146)DB(21-24)WriteBatch(26-47)WriteBatch(26-27)ReadAddressIndex(49-80)ReadAddressIndex(49-51)ReadAddressUnspentIndex(82-113)ReadAddressUnspentIndex(82-84)EraseAddressIndex(115-124)EraseAddressIndex(115-115)UpdateAddressUnspentIndex(126-139)UpdateAddressUnspentIndex(126-126)WriteBlock(148-252)WriteBlock(148-148)Rewind(254-375)Rewind(254-254)BlockDisconnected(377-390)BlockDisconnected(377-377)GetDB(392-392)GetDB(392-392)GetAddressIndex(394-403)GetAddressIndex(394-396)GetAddressUnspentIndex(405-414)GetAddressUnspentIndex(405-407)
src/index/addressindex_util.cpp (1)
src/hash.h (1)
Hash160(94-99)
src/index/timestampindex.h (1)
src/index/timestampindex.cpp (19)
TimestampIndex(51-54)TimestampIndex(56-56)DB(15-18)Write(20-23)Write(20-20)ReadRange(25-44)ReadRange(25-25)EraseTimestampIndex(46-49)EraseTimestampIndex(46-46)WriteBlock(58-68)WriteBlock(58-58)Rewind(70-88)Rewind(70-70)BlockDisconnected(90-103)BlockDisconnected(90-90)GetDB(105-105)GetDB(105-105)GetBlockHashes(107-114)GetBlockHashes(107-107)
src/index/spentindex.cpp (3)
src/node/blockstorage.cpp (2)
UndoReadFromDisk(512-543)UndoReadFromDisk(512-512)src/index/addressindex_util.cpp (2)
AddressBytesFromScript(12-28)AddressBytesFromScript(12-12)src/index/spentindex.h (1)
BlockDisconnected(60-63)
src/txdb.h (1)
src/txdb.cpp (2)
MigrateOldIndexData(465-586)MigrateOldIndexData(465-465)
🪛 Cppcheck (2.19.0)
src/rpc/blockchain.cpp
[error] 22-22: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
src/txdb.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
src/index/addressindex.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
src/index/timestampindex.cpp
[error] 16-16: #error Client version information missing
(preprocessorErrorDirective)
src/index/addressindex_util.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
src/index/spentindex.cpp
[error] 16-16: #error Client version information missing
(preprocessorErrorDirective)
🪛 GitHub Actions: Clang Diff Format Check
src/index/addressindex_types.h
[error] 28-29: Clang format differences found between before/after formatting. Run 'git diff -U0 origin/develop --
⏰ 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). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
🔇 Additional comments (52)
src/zmq/zmqpublishnotifier.cpp (1)
21-21: LGTM!The include is correctly added to support the
ToUnderlyingcall used on line 468 for logging the governance object type. This makes the dependency explicit rather than relying on transitive includes.src/dbwrapper.h (1)
395-410: LGTM!The
CompactRangeimplementation correctly follows the established pattern used byEstimateSizefor key serialization. Memory safety is ensured since theCDataStreamobjects remain in scope for the duration of thepdb->CompactRangecall, and theconstqualifier is appropriate since compaction doesn't change the logical database contents.doc/release-notes-7101.md (1)
1-12: LGTM! Clear and informative release notes.The documentation clearly explains the migration behavior, expected duration, and breaking changes regarding pruned node incompatibility. The mention of
getindexinfoRPC for progress monitoring is helpful for users.src/index/addressindex_util.cpp (1)
12-28: LGTM! Address extraction logic is correct.The byte offsets correctly match standard script formats:
- P2SH: extracts the 20-byte hash at bytes [2, 22)
- P2PKH: extracts the 20-byte hash at bytes [3, 23)
- P2PK: hashes the pubkey (excluding length prefix and OP_CHECKSIG)
The static analysis hint about line 10 is a false positive from Cppcheck not recognizing the
<vector>system header.src/txmempool.h (2)
21-22: LGTM! Appropriate includes for the new index type headers.The new includes for
addressindex_types.handspentindex_types.halign with the PR's migration of index types to dedicated header files undersrc/index/.
614-615: LGTM! Return type change is consistent with implementation.The change from
booltovoidaligns with the implementation intxmempool.cppwhich simply populates the results vector without any failure path. This simplifies the API since the function always succeeds.src/index/spentindex_types.h (2)
41-79: LGTM! Comprehensive value structure.The value type captures all relevant spending information: the spending transaction, block height, amount, and address details. The
IsNull()check onm_tx_hashis a reasonable sentinel for validity.
81-89: LGTM! Clean comparison functor and type alias.Using
std::tiefor lexicographic comparison is idiomatic and correct. TheCSpentIndexEntryalias provides a convenient key-value pair type.src/index/spentindex.h (3)
13-17: LGTM! Reasonable defaults and helper struct.
DEFAULT_SPENTINDEX{false}is appropriate for an opt-in feature. TheCSpentIndexTxInfostruct provides a convenient container for collecting spent info during block processing.
19-29: LGTM! Clear and comprehensive documentation.The docstring clearly explains the index purpose, the undo data dependency, and why the index is incompatible with pruned nodes. The database location is also documented.
30-77: Overall class design follows BaseIndex pattern correctly.The
SpentIndexclass properly:
- Derives from
BaseIndexwith appropriate overrides- Uses a nested
DBclass for database operations- Implements
RewindandBlockDisconnectedfor reorg handling- Returns
falsefromAllowPrune()to enforce the undo data requirement- Exposes a global
g_spentindexpointer consistent with other indexes (g_addressindex,g_timestampindex)src/core_write.cpp (1)
22-22: LGTM!The include consolidation from
<addressindex.h>and<spentindex.h>to<index/spentindex.h>aligns with the new index structure. The types used in this file (CSpentIndexKey,CSpentIndexValue,CSpentIndexTxInfo) are correctly exposed through the new header path.src/index/spentindex.cpp (5)
1-3: LGTM!New SpentIndex implementation file with appropriate MIT license header.
22-53: LGTM!The DB class follows the BaseIndex::DB pattern correctly. The
WriteBatchmethod handles both writes and deletes cleanly using the null value convention (documented in line 32 comment), andEraseSpentIndexprovides efficient batch deletion.
62-108: LGTM!The
WriteBlockimplementation correctly:
- Skips the genesis block (no inputs to index)
- Reads undo data to extract spent output information
- Validates undo data size matches input count (preventing data corruption)
- Extracts address type/bytes using
AddressBytesFromScript- Batches all entries for efficient DB write
110-146: LGTM!The
Rewindimplementation correctly iterates backwards through the chain, reads blocks from disk, collects all spent index keys for each block's inputs, and erases them in batches. The final call toBaseIndex::Rewindensures the best block locator is updated consistently.
165-172: LGTM!The
GetSpentInfomethod correctly ensures the index is synced to the current chain tip before querying, providing consistent data to callers.src/node/blockstorage.cpp (1)
394-398: LGTM!The migration call is correctly placed in
LoadBlockIndexDBafter reindexing flag check. Returningfalseon migration failure ensures the node won't start with an inconsistent index state. This is the appropriate location since it runs during initial block database loading.test/functional/feature_addressindex.py (2)
49-56: LGTM!The test correctly validates the new async index behavior:
- Disables
-addressindexand restarts- Re-enables
-addressindexwithout-reindex- Continues with normal index operations
This confirms that the async AddressIndex can be toggled without requiring a full reindex, as expected from the migration to the BaseIndex framework.
58-342: Test coverage is comprehensive.The existing test cases thoroughly validate address index functionality including p2pkh/p2sh queries, balance tracking, height range filtering, reorg handling, and mempool indexing. The async index behavior is implicitly validated by the successful execution of these queries after the toggle-without-reindex flow.
src/rpc/blockchain.cpp (2)
22-22: LGTM!Correctly includes the new timestamp index header to access
g_timestampindexfor the async index migration.
555-565: LGTM! Good error handling for async index.The implementation correctly:
- Checks if the timestamp index is enabled before use
- Provides a distinct error when the index is still syncing vs. not enabled
- Uses the global
g_timestampindexpointer consistent with the async index patternsrc/node/caches.cpp (2)
7-9: LGTM!Correctly includes the new index headers to access their default flag constants (
DEFAULT_ADDRESSINDEX,DEFAULT_TIMESTAMPINDEX,DEFAULT_SPENTINDEX).
25-30: Cache allocation is well-proportioned.The cache allocation ratios make sense:
- Address index (256 MiB cap): Larger index with address-to-txid mappings
- Timestamp index (8 MiB cap): Simpler timestamp-to-blockhash mappings
- Spent index (128 MiB cap): Moderate size for spent output tracking
All three correctly reduce
nTotalCacheafter allocation to ensure subsequent allocations work from the remaining budget.src/node/caches.h (1)
20-22: LGTM!The new cache size fields are correctly added with consistent
int64_ttype matching the existing fields. These are properly populated byCalculateCacheSizes()in caches.cpp.src/Makefile.am (2)
248-258: LGTM!New index headers are correctly added to
BITCOIN_CORE_Hin proper alphabetical order within theindex/group.
541-547: LGTM!New index source files are correctly added to
libbitcoin_node_a_SOURCES:
index/addressindex.cppandindex/addressindex_util.cppfor address indexingindex/spentindex.cppfor spent output trackingindex/timestampindex.cppfor timestamp-based block lookupsThese are properly placed in the node library since indexes are node-specific functionality. Old root-level index files have been removed.
test/functional/feature_spentindex.py (3)
50-57: LGTM!The test correctly validates that the async spent index can be enabled without requiring
-reindex, which is the expected behavior after migrating to the async BaseIndex framework.
124-130: LGTM!Good addition to verify that
getspentinfocorrectly returns mempool-backed data withheight: -1for unconfirmed transactions.
140-168: LGTM!Comprehensive reorg handling test that validates the spent index correctly transitions between confirmed and mempool states during block invalidation and reconsideration.
test/functional/feature_timestampindex.py (3)
28-40: LGTM!The test correctly validates that the async timestamp index can be enabled and disabled without requiring
-reindex, consistent with the PR's migration to the BaseIndex framework.
42-52: LGTM!Good test coverage for the timestamp index RPC, using
bump_mocktimeto ensure distinct block timestamps for reliable verification.
54-72: LGTM!The reorg test correctly validates that the timestamp index removes invalidated blocks and restores them after reconsideration. The assertion checks are consistent with invalidating the block at index 3 (which invalidates both blocks 3 and 4).
src/txmempool.cpp (4)
36-40: LGTM!Forward declarations for the index classes and their global pointers avoid circular header dependencies while allowing the mempool to check index availability.
538-541: LGTM!The null guard correctly prevents address index updates when the index is not enabled, avoiding unnecessary work and potential null pointer issues.
608-611: LGTM!Consistent null guard pattern with
addAddressIndex, correctly preventing spent index updates when the index is disabled.
583-595: No changes required. The function currently returnsvoidwith no evidence in the git history of a previousboolreturn type. The single call site insrc/rpc/node.cpp:430correctly treats the function asvoid. The implementation has no failure paths and the out-parameter pattern for populating results is a valid C++ design.Likely an incorrect or invalid review comment.
src/txdb.cpp (4)
44-48: LGTM!Defining the deprecated key constants locally for migration purposes is appropriate. These are only needed during the one-time migration and don't pollute the public interface.
356-442: Well-structured migration template.The implementation correctly handles:
- Batched writes to avoid memory pressure
- Iterator reset before compaction (required by LevelDB)
- Periodic compaction to reclaim disk space during migration
- Final sync writes for durability
One minor observation: after compaction and iterator reset (lines 412-413), seeking to
startrelies on previously-erased keys being gone. This is correct sinceWriteBatchwith erase was called before compaction.
448-463: LGTM!The finalization correctly writes a minimal
CBlockLocatorcontaining just the best block hash, ensuring the migrated index knows its sync point.
542-575: LGTM!Correctly migrates both address index types (transaction history and unspent) to the same database, with a single best-block locator write covering both.
src/rpc/rawtransaction.cpp (1)
133-140: LGTM!The lock ordering is correct: spent index queries (which may call
BlockUntilSyncedToCurrentChain) must complete before acquiringcs_main. The comment at lines 80-81 explains this requirement.src/validation.cpp (1)
1057-1062: Mempool indexing calls are properly gated—no action neededThe unconditional calls to
addAddressIndex()andaddSpentIndex()insrc/validation.cppare safe. Both methods insrc/txmempool.cpp(lines 540 and 610) immediately return early if the corresponding global index (g_addressindexorg_spentindex) is null. These globals are only instantiated when-addressindexor-spentindexcommand-line flags are explicitly enabled; both default tofalse. The mempool methods have no performance or behavioral impact when indexes are disabled.src/rpc/node.cpp (1)
653-670: Good: using index best-block height to avoid chain-tip/index skew.Using
g_addressindex->GetSummary()and rejecting when not synced (Line 663-668) is the right direction for async index RPCs.src/init.cpp (1)
242-277: Index lifecycle wiring looks consistent (Interrupt/Stop/reset).The added
Interrupt()andStop()/reset()hooks forg_addressindex,g_timestampindex, andg_spentindex(Line 264-272 and Line 389-400) match the existingg_txindexpattern and are placed after background callback flushing.Also applies to: 384-406
src/index/timestampindex.h (1)
8-12: The timestampindex.h file does not currently contain theGetBlockHashesmethod at lines 58-66 that the review comment references, nor is there evidence of this method being called in the codebase. The review comment appears to be written for a future state of the code or for a different version of the file than currently exists in the repository.To verify this review comment properly, I would need:
- Confirmation of whether this is code being reviewed in a pull request that hasn't been applied yet
- The actual implementation of
GetBlockHashesmethod with the (high, low) parameter order mentioned- Verification that the parameter order confusion actually exists in the intended code state
- Confirmation of which call sites need to be audited for the parameter order issue
The missing includes (
<memory>,<vector>) and the parameter order confusion ([low, high] vs (high, low)) are valid concerns if the mentioned code exists, but they cannot be definitively verified against the current state of the repository.src/index/addressindex.cpp (6)
1-19: LGTM!The includes, constants, and global pointer declaration follow the established pattern for async indexes. The
constexprdatabase key prefixes are well-defined.
21-47: LGTM!The DB constructor and
WriteBatchimplementation are well-structured. The null-value convention for signaling deletion is applied consistently, and the batch operations ensure atomicity.
55-59: Height range filtering requires both bounds to be set.The condition
start > 0 && end > 0means specifying only a start height (e.g.,start=100, end=0for "from block 100 onwards") will not seek to the correct position. Consider whether this is intentional or if the logic should support one-sided ranges.If one-sided ranges should be supported:
-if (start > 0 && end > 0) { +if (start > 0) { pcursor->Seek(std::make_pair(DB_ADDRESSINDEX, CAddressIndexIteratorHeightKey(type, address_hash, start))); } else {
148-252: LGTM!The
WriteBlockimplementation correctly:
- Skips genesis block (no undo data available)
- Validates undo data alignment with transaction inputs
- Tracks spending (negative values) and receiving (positive values)
- Uses correct transaction indices (0 for coinbase, i+1 for others)
- Processes coinbase outputs separately since coinbase has no inputs
254-275: LGTM on Rewind structure.The rewind logic correctly:
- Asserts the ancestor relationship
- Processes blocks in reverse order from current_tip to new_tip
- Reads both block and undo data for each block being unwound
- Delegates to base class at the end to update the best block pointer
394-414: LGTM on query methods.Both
GetAddressIndexandGetAddressUnspentIndexcorrectly callBlockUntilSyncedToCurrentChain()before querying, ensuring callers receive consistent data relative to the current chain tip.
| #ifndef BITCOIN_INDEX_ADDRESSINDEX_TYPES_H | ||
| #define BITCOIN_INDEX_ADDRESSINDEX_TYPES_H | ||
|
|
||
| #include <consensus/amount.h> | ||
| #include <script/script.h> | ||
| #include <serialize.h> | ||
| #include <uint256.h> | ||
| #include <util/underlying.h> | ||
|
|
||
| #include <chrono> | ||
| #include <tuple> | ||
| #include <vector> |
There was a problem hiding this comment.
Fix CI clang-format failure in this header
The pipeline reports clang-format diffs for this file; please apply the repo’s clang-format-diff.py flow (as the CI error suggests) and re-run formatting.
🤖 Prompt for AI Agents
In @src/index/addressindex_types.h around lines 8 - 19, The header
src/index/addressindex_types.h is failing CI formatting; run the repository's
clang-format-diff.py flow to auto-apply the project's clang-format style to this
file (targeting the current patch/commit), regenerate the diff, and commit the
updated formatting so the BITCOIN_INDEX_ADDRESSINDEX_TYPES_H header guard and
the include block (e.g., consensus/amount.h, script/script.h, serialize.h,
uint256.h, util/underlying.h and the chrono/tuple/vector includes) conform to
the project's style; ensure you use the repo's specified clang-format
binary/version when running the script before pushing.
| while (pcursor->Valid()) { | ||
| std::pair<uint8_t, CAddressIndexKey> key; | ||
| if (pcursor->GetKey(key) && key.first == DB_ADDRESSINDEX && key.second.m_address_bytes == address_hash) { | ||
| if (end > 0 && key.second.m_block_height > end) { | ||
| break; | ||
| } | ||
| CAmount value; | ||
| if (pcursor->GetValue(value)) { | ||
| entries.emplace_back(key.second, value); | ||
| pcursor->Next(); | ||
| } else { | ||
| return error("failed to get address index value"); | ||
| } | ||
| } else { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing address type validation in iterator loop.
The loop condition at line 63 only verifies m_address_bytes but not the address type. If the key ordering allows entries with the same address_hash but different AddressType to be adjacent, this could return incorrect entries.
🐛 Proposed fix: Add type validation
while (pcursor->Valid()) {
std::pair<uint8_t, CAddressIndexKey> key;
- if (pcursor->GetKey(key) && key.first == DB_ADDRESSINDEX && key.second.m_address_bytes == address_hash) {
+ if (pcursor->GetKey(key) && key.first == DB_ADDRESSINDEX &&
+ key.second.m_type == type && key.second.m_address_bytes == address_hash) {
if (end > 0 && key.second.m_block_height > end) {
break;
}🤖 Prompt for AI Agents
In @src/index/addressindex.cpp around lines 61 - 77, The iterator loop over
pcursor (checking DB_ADDRESSINDEX and CAddressIndexKey::m_address_bytes ==
address_hash) lacks validation of the address type and may include entries with
the same hash but different CAddressIndexKey::m_address_type; update the
condition that currently checks key.first == DB_ADDRESSINDEX &&
key.second.m_address_bytes == address_hash to also require
key.second.m_address_type == expected_type (or compare against the provided
address type variable), so only matching type+hash pairs are processed before
pushing into entries and advancing pcursor.
| while (pcursor->Valid()) { | ||
| std::pair<uint8_t, CAddressUnspentKey> key; | ||
| if (pcursor->GetKey(key) && key.first == DB_ADDRESSUNSPENTINDEX && key.second.m_address_bytes == address_hash) { | ||
| CAddressUnspentValue value; | ||
| if (pcursor->GetValue(value)) { | ||
| entries.emplace_back(key.second, value); | ||
| pcursor->Next(); | ||
| } else { | ||
| return error("failed to get address unspent value"); | ||
| } | ||
| } else { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Same missing type validation as in ReadAddressIndex.
The loop condition at line 92 only verifies m_address_bytes without checking the address type, which could lead to returning entries of incorrect types.
🐛 Proposed fix
while (pcursor->Valid()) {
std::pair<uint8_t, CAddressUnspentKey> key;
- if (pcursor->GetKey(key) && key.first == DB_ADDRESSUNSPENTINDEX && key.second.m_address_bytes == address_hash) {
+ if (pcursor->GetKey(key) && key.first == DB_ADDRESSUNSPENTINDEX &&
+ key.second.m_type == type && key.second.m_address_bytes == address_hash) {🤖 Prompt for AI Agents
In @src/index/addressindex.cpp around lines 90 - 103, The loop in the
ReadAddressUnspentIndex-like logic only checks key.first ==
DB_ADDRESSUNSPENTINDEX and key.second.m_address_bytes == address_hash, but never
validates the address type; update the if condition to also require
key.second.type == type (or the actual function parameter name for the requested
address type) so only entries matching both address bytes and address type are
collected; ensure you still call pcursor->Next() on success and return the same
error on GetValue failure.
| void AddressIndex::BlockDisconnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex) | ||
| { | ||
| // When a block is disconnected (e.g., via invalidateblock), we need to rewind the index | ||
| // to remove this block's data | ||
| const CBlockIndex* best_block_index = CurrentIndex(); | ||
|
|
||
| // Only rewind if we have this block indexed | ||
| if (best_block_index && best_block_index->nHeight >= pindex->nHeight) { | ||
| if (!Rewind(best_block_index, pindex->pprev)) { | ||
| error("%s: Failed to rewind %s to previous block after disconnect", | ||
| __func__, GetName()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Error from Rewind is discarded, leaving index potentially inconsistent.
When Rewind fails at line 385-386, the error is logged but the failure is not propagated. This could leave the index in an inconsistent state where the actual data doesn't match what the index believes is synced.
Consider logging a more severe warning or tracking the failed state:
if (best_block_index && best_block_index->nHeight >= pindex->nHeight) {
if (!Rewind(best_block_index, pindex->pprev)) {
- error("%s: Failed to rewind %s to previous block after disconnect",
- __func__, GetName());
+ LogPrintf("%s: CRITICAL - Failed to rewind %s to previous block after disconnect. "
+ "Index may be inconsistent and require -reindex.\n",
+ __func__, GetName());
}
}| #include <index/timestampindex.h> | ||
|
|
||
| #include <chain.h> | ||
| #include <logging.h> | ||
| #include <util/system.h> | ||
|
|
There was a problem hiding this comment.
Build breaker: error(...) is used without including its declaration.
TimestampIndex::Rewind() and BlockDisconnected() call error(...) (Line 81 and Line 99), but this file doesn’t include util/error.h. Unless another include happens to pull it in transitively, this is likely a compile failure.
Proposed fix
#include <index/timestampindex.h>
#include <chain.h>
#include <logging.h>
+#include <util/error.h>
#include <util/system.h>Also applies to: 70-103
🤖 Prompt for AI Agents
In @src/index/timestampindex.cpp around lines 5 - 10, The file is missing the
declaration for error(...), causing build failures in TimestampIndex::Rewind and
TimestampIndex::BlockDisconnected; add the missing include by adding #include
<util/error.h> alongside the existing includes at the top of timestampindex.cpp
so the calls to error(...) resolve correctly.
| void TimestampIndex::BlockDisconnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex) | ||
| { | ||
| // When a block is disconnected (e.g., via invalidateblock), we need to rewind the index | ||
| // to remove this block's data | ||
| const CBlockIndex* best_block_index = CurrentIndex(); | ||
|
|
||
| // Only rewind if we have this block indexed | ||
| if (best_block_index && best_block_index->nHeight >= pindex->nHeight) { | ||
| if (!Rewind(best_block_index, pindex->pprev)) { | ||
| error("%s: Failed to rewind %s to previous block after disconnect", | ||
| __func__, GetName()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against pindex->pprev == nullptr in BlockDisconnected().
This matches the pattern used in other indexes, but it’s still safer to avoid calling Rewind(best, nullptr) (which will assert/crash in Rewind). Even if disconnecting genesis “shouldn’t happen”, a defensive guard here is cheap.
Proposed fix
const CBlockIndex* best_block_index = CurrentIndex();
// Only rewind if we have this block indexed
if (best_block_index && best_block_index->nHeight >= pindex->nHeight) {
+ if (pindex->pprev == nullptr) return;
if (!Rewind(best_block_index, pindex->pprev)) {
error("%s: Failed to rewind %s to previous block after disconnect",
__func__, GetName());
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void TimestampIndex::BlockDisconnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex) | |
| { | |
| // When a block is disconnected (e.g., via invalidateblock), we need to rewind the index | |
| // to remove this block's data | |
| const CBlockIndex* best_block_index = CurrentIndex(); | |
| // Only rewind if we have this block indexed | |
| if (best_block_index && best_block_index->nHeight >= pindex->nHeight) { | |
| if (!Rewind(best_block_index, pindex->pprev)) { | |
| error("%s: Failed to rewind %s to previous block after disconnect", | |
| __func__, GetName()); | |
| } | |
| } | |
| } | |
| void TimestampIndex::BlockDisconnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex) | |
| { | |
| // When a block is disconnected (e.g., via invalidateblock), we need to rewind the index | |
| // to remove this block's data | |
| const CBlockIndex* best_block_index = CurrentIndex(); | |
| // Only rewind if we have this block indexed | |
| if (best_block_index && best_block_index->nHeight >= pindex->nHeight) { | |
| if (pindex->pprev == nullptr) return; | |
| if (!Rewind(best_block_index, pindex->pprev)) { | |
| error("%s: Failed to rewind %s to previous block after disconnect", | |
| __func__, GetName()); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/index/timestampindex.cpp around lines 90 - 103, BlockDisconnected
currently calls Rewind(best_block_index, pindex->pprev) without ensuring
pindex->pprev is non-null which can trigger an assert in Rewind; add a defensive
guard in TimestampIndex::BlockDisconnected to check that pindex->pprev !=
nullptr before calling Rewind (if null, log an error/warning with __func__ and
GetName() and skip the Rewind), keeping existing behavior when pprev is valid.
| if (args.GetIntArg("-prune", 0)) { | ||
| if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) | ||
| return InitError(_("Prune mode is incompatible with -txindex.")); | ||
| if (args.GetBoolArg("-addressindex", DEFAULT_ADDRESSINDEX)) | ||
| return InitError(_("Prune mode is incompatible with -addressindex.")); | ||
| if (args.GetBoolArg("-spentindex", DEFAULT_SPENTINDEX)) | ||
| return InitError(_("Prune mode is incompatible with -spentindex.")); | ||
| if (args.GetBoolArg("-reindex-chainstate", false)) { |
There was a problem hiding this comment.
Address/spent index prune incompatibility should also catch “already-pruned datadir” cases.
You correctly reject -prune with -addressindex/-spentindex (Line 1216-1219). But a node can still have chainman.m_blockman.m_have_pruned == true due to historical pruning even if -prune=0, and then enabling address/spent indexing will likely fail later (or behave badly) because undo data is missing.
Consider adding an additional runtime guard at index start time (Line 2277+), since you already reference chainman.m_blockman.m_have_pruned elsewhere in this file.
Proposed fix
if (args.GetBoolArg("-addressindex", DEFAULT_ADDRESSINDEX)) {
+ if (chainman.m_blockman.m_have_pruned) {
+ return InitError(_("Address index is incompatible with a pruned blockstore. Re-download blocks/undo (or use a non-pruned datadir)."));
+ }
g_addressindex = std::make_unique<AddressIndex>(cache_sizes.address_index, false, fReindex);
if (!g_addressindex->Start(chainman.ActiveChainstate())) {
return false;
}
}
@@
if (args.GetBoolArg("-spentindex", DEFAULT_SPENTINDEX)) {
+ if (chainman.m_blockman.m_have_pruned) {
+ return InitError(_("Spent index is incompatible with a pruned blockstore. Re-download blocks/undo (or use a non-pruned datadir)."));
+ }
g_spentindex = std::make_unique<SpentIndex>(cache_sizes.spent_index, false, fReindex);
if (!g_spentindex->Start(chainman.ActiveChainstate())) {
return false;
}
}Also applies to: 2277-2297
🤖 Prompt for AI Agents
In @src/init.cpp around lines 1213 - 1220, Add a runtime guard during index
startup to catch already-pruned datadir cases: when initializing address and
spent indexes (the code around the index start block at ~2277), check
chainman.m_blockman.m_have_pruned and if true return an InitError (or otherwise
abort with a clear message) indicating pruning is incompatible with enabling
-addressindex or -spentindex; ensure you reference the existing flag checks for
args.GetBoolArg("-addressindex") and args.GetBoolArg("-spentindex") so the new
check triggers even when -prune=0 but the on-disk datadir is already pruned.
| if (!g_spentindex) { | ||
| throw JSONRPCError(RPC_MISC_ERROR, "Spent index is not enabled. Start with -spentindex to enable."); | ||
| } | ||
|
|
||
| CSpentIndexKey key(txid, outputIndex); | ||
| CSpentIndexValue value; | ||
| bool found = false; | ||
|
|
||
| // Check the async index first | ||
| if (g_spentindex->GetSpentInfo(key, value)) { | ||
| found = true; | ||
| } | ||
|
|
||
| ChainstateManager& chainman = EnsureAnyChainman(request.context); | ||
| // Check mempool for unconfirmed spends. | ||
| // | ||
| // Mempool entry overwrites index entry if present. | ||
| // During reorg, mempool may have newer state than index (which updates asynchronously). | ||
| CTxMemPool& mempool = EnsureAnyMemPool(request.context); | ||
| if (LOCK(::cs_main); !GetSpentIndex(*chainman.m_blockman.m_block_tree_db, mempool, key, value)) { | ||
| { | ||
| LOCK(mempool.cs); | ||
| CSpentIndexValue mempoolValue; | ||
| if (mempool.getSpentIndex(key, mempoolValue)) { | ||
| value = mempoolValue; // Overwrite with mempool entry (current state) | ||
| found = true; | ||
| } | ||
| } | ||
|
|
||
| if (!found) { | ||
| throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unable to get spent info"); | ||
| } |
There was a problem hiding this comment.
Validate outputIndex and keep error semantics crisp with async overlay.
Two nits in the new flow:
- Negative
outputIndexshould be rejected (invalid outpoint index) before buildingCSpentIndexKey. - When both index and mempool miss,
"Unable to get spent info"is fine; but ifGetSpentInfo()can fail due to “not yet synced”, consider distinguishing that (optional).
Proposed fix (input validation)
uint256 txid = ParseHashV(txidValue, "txid");
int outputIndex = indexValue.getInt<int>();
+ if (outputIndex < 0) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid index");
+ }
+
if (!g_spentindex) {
throw JSONRPCError(RPC_MISC_ERROR, "Spent index is not enabled. Start with -spentindex to enable.");
}🤖 Prompt for AI Agents
In @src/rpc/node.cpp around lines 814 - 843, Validate that outputIndex is
non-negative before constructing CSpentIndexKey: if outputIndex < 0, throw
JSONRPCError(RPC_INVALID_PARAMETER, "Invalid outpoint index"); then build
CSpentIndexKey(txid, outputIndex) and proceed with the existing async index
check (g_spentindex->GetSpentInfo) and mempool overlay (EnsureAnyMemPool +
mempool.getSpentIndex) logic unchanged; keep the current final error
throw(JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unable to get spent info")) when
neither source finds a value, and optionally add a distinct RPC error only if
GetSpentInfo can explicitly signal “not yet synced” (handle that case where
GetSpentInfo provides such a status).
| bool LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function<CBlockIndex*(const uint256&)> insertBlockIndex) | ||
| EXCLUSIVE_LOCKS_REQUIRED(::cs_main); | ||
|
|
||
| /// Migrate old synchronous index data to new async index databases | ||
| bool MigrateOldIndexData(); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find call sites of MigrateOldIndexData
echo "=== Call sites for MigrateOldIndexData ===" >&2
rg -n --type=cpp --type=h "MigrateOldIndexData\(" -B 3 -A 3
# Find the implementation
echo -e "\n=== Implementation location ===" >&2
rg -n --type=cpp "bool.*MigrateOldIndexData\(\)" -B 2 -A 20Repository: dashpay/dash
Length of output: 2902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the call site context in blockstorage.cpp (wider range)
echo "=== Call site context in blockstorage.cpp ===" >&2
sed -n '380,410p' src/node/blockstorage.cpp | cat -n
# Check for any lock acquisition around the call
echo -e "\n=== Check for locks held at call site ===" >&2
sed -n '370,410p' src/node/blockstorage.cpp | grep -i "lock\|cs_main" | cat -n
# Read the full implementation of MigrateOldIndexData
echo -e "\n=== Full implementation of MigrateOldIndexData ===" >&2
sed -n '465,550p' src/txdb.cpp | cat -nRepository: dashpay/dash
Length of output: 6771
🏁 Script executed:
#!/bin/bash
set -euoripefail
# Find the function containing the call at line 396
echo "=== Find function containing the call ===" >&2
sed -n '300,396p' src/node/blockstorage.cpp | grep -E "^[a-zA-Z_].*\(|^bool.*\(|^void.*\(" | tail -1
# Get the full function signature and context
echo -e "\n=== Function context ===" >&2
sed -n '350,400p' src/node/blockstorage.cpp | head -60 | cat -n
# Check where LoadBlockIndexDB is called from
echo -e "\n=== Call sites of LoadBlockIndexDB ===" >&2
rg -n "LoadBlockIndexDB\(" --type=cpp -B 5 -A 2Repository: dashpay/dash
Length of output: 730
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the function containing the call at line 396
echo "=== Find function containing the call ===" >&2
sed -n '300,396p' src/node/blockstorage.cpp | grep -E "^[a-zA-Z_].*\(|^bool.*\(|^void.*\(" | tail -1
# Get the full function signature and context
echo -e "\n=== Function context ===" >&2
sed -n '350,400p' src/node/blockstorage.cpp | cat -n
# Check where LoadBlockIndexDB is called from
echo -e "\n=== Call sites of LoadBlockIndexDB ===" >&2
rg -n "LoadBlockIndexDB\(" --type=cpp -B 5 -A 2Repository: dashpay/dash
Length of output: 4364
Add EXCLUSIVE_LOCKS_REQUIRED(::cs_main) annotation to MigrateOldIndexData() declaration
The method accesses the coins database (reading DB_BEST_BLOCK) and is only called from LoadBlockIndexDB(), which already requires cs_main. Add the lock annotation to the public method signature in src/txdb.h to enforce this contract and prevent future misuse:
bool MigrateOldIndexData() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
🤖 Prompt for AI Agents
In @src/txdb.h around lines 93 - 98, Add the EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
annotation to the public declaration of MigrateOldIndexData in src/txdb.h to
enforce that callers must hold ::cs_main (e.g., change the declaration of
MigrateOldIndexData to include EXCLUSIVE_LOCKS_REQUIRED(::cs_main)); this
matches the existing annotation on LoadBlockIndexGuts and reflects that
MigrateOldIndexData reads the coins DB (DB_BEST_BLOCK) and is only called from
LoadBlockIndexDB which holds cs_main.
| src/index/addres*.h | ||
| src/index/spent*.h | ||
| src/index/timestamp*.h |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for new index files matching the patterns
echo "=== Header files (.h) ==="
fd -t f -e h . src/index/ | grep -E 'addres|spent|timestamp'
echo ""
echo "=== Source files (.cpp) ==="
fd -t f -e cpp . src/index/ | grep -E 'addres|spent|timestamp'
echo ""
echo "=== All files in src/index/ ==="
fd -t f . src/index/ | head -30Repository: dashpay/dash
Length of output: 981
Add corresponding .cpp file patterns for new index sources.
The .h patterns (addres*.h, spent*.h, timestamp*.h) cover the header files, but the corresponding source files also exist and should be included per repository conventions for clang-format:
src/index/addressindex.cppsrc/index/addressindex_util.cppsrc/index/spentindex.cppsrc/index/timestampindex.cpp
Add patterns for these .cpp files to the list:
src/index/addres*.cppsrc/index/spent*.cpp
🤖 Prompt for AI Agents
In @test/util/data/non-backported.txt around lines 21 - 23, The file lists
header patterns for new index sources but misses the corresponding source
patterns; update the list (where you have src/index/addres*.h,
src/index/spent*.h, src/index/timestamp*.h) to also include the matching .cpp
patterns so clang-format covers source files too — add src/index/addres*.cpp,
src/index/spent*.cpp and src/index/timestamp*.cpp to the same list.
1 participant
Issue being fixed or feature implemented
Migrate three Dash-specific indexes (
AddressIndex,SpentIndex,TimestampIndex) from synchronous operation inCBlockTreeDBto async operation usingBaseIndexframework.When enabling an index for the first time, the node will build the index in the background while remaining fully operational. Progress can be monitored via the
getindexinfoRPC.Existing nodes with indexes enabled will automatically migrate data from the old location (block index database) to new separate databases on first startup. This migration may take 20-40 minutes or longer depending on hardware specifications and index sizes. The node will log progress during migration.
What was done?
Architecture Improvements:
BaseIndex, enabling async catch-up during IBDindexes/{addressindex,spentindex,timestampindex}/-reindexMigration System:
MigrateIndex<>()function handles data migrationHow Has This Been Tested?
Run tests. Run a testnet/mainnet node and confirm migration succeeds (backup old datadir cause otherwise you'd have to reindex to go back to develop/master binaries).
Breaking Changes
SpentIndexandAddressIndexare incompatible with pruned nodes as they require access to undo data nowChecklist: