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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
|
WalkthroughThis PR converts address, spent, and timestamp indexes from synchronous block-tree storage to asynchronous BaseIndex-backed components. It adds AddressIndex, SpentIndex, and TimestampIndex under src/index/ with dedicated DB layers and types, exposes global pointers (g_addressindex, g_spentindex, g_timestampindex), updates build and init/shutdown flows to start/stop these indexes, removes legacy rpc/index_util and on-disk sync helpers, and adds CBlockTreeDB::MigrateOldIndexData() to migrate existing index data to the new async databases on first startup. RPCs, mempool, validation, and tests are updated to use the new async index APIs. Sequence Diagram(s)sequenceDiagram
participant Chain as Chain Manager
participant Index as Address/Spent/Timestamp Index
participant Undo as Undo/UTXO Data
participant DB as Index DB
participant Disk as Disk
Chain->>Index: WriteBlock(block, pindex)
Index->>Undo: Read undo / prevouts
Undo-->>Index: Undo / UTXO entries
Index->>Index: Build index entries (spends/receipts/timestamps)
Index->>DB: WriteBatch(entries)
DB->>Disk: Persist entries
Disk-->>DB: Ack
DB-->>Index: Success
Index-->>Chain: Return success
sequenceDiagram
participant Node as Node Startup
participant BlockDB as CBlockTreeDB
participant Migrator as Migration routine
participant NewDB as Async Index DBs
participant OldDB as Old Block Index DB
Node->>BlockDB: LoadBlockIndexDB()
BlockDB->>Migrator: MigrateOldIndexData()
Migrator->>OldDB: Read old index entries
OldDB-->>Migrator: Old entries batch
Migrator->>NewDB: WriteBatch to new index DBs
NewDB->>NewDB: FinalizeMigration (best-block)
Migrator->>OldDB: Compact/Erase migrated ranges
Migrator-->>BlockDB: Migration result
BlockDB-->>Node: Continue startup
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 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 |
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.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/index/addressindex.cpp`:
- Around line 361-373: AddressIndex::BlockDisconnected may call Rewind with
pindex->pprev which can be nullptr for the genesis block; add a null check for
pindex->pprev before invoking Rewind (use CurrentIndex() to decide whether to
rewind and only call Rewind(best_block_index, pindex->pprev) when pindex->pprev
!= nullptr), and if pindex->pprev is nullptr log an appropriate error/warning
(including GetName()) and skip the Rewind to avoid the assertion in Rewind.
In `@src/index/spentindex.cpp`:
- Around line 148-160: In SpentIndex::BlockDisconnected, add a defensive null
check for pindex->pprev before calling Rewind: if pindex->pprev is null (e.g.,
genesis), skip calling Rewind and log or handle gracefully (use process/error
logging similar to the existing error call using GetName()); keep the existing
checks using CurrentIndex() and only call Rewind(best_block_index,
pindex->pprev) when pindex->pprev != nullptr to avoid a potential null pointer
dereference.
♻️ Duplicate comments (1)
src/index/timestampindex.cpp (1)
5-9: Missing include forerror()function.The
error()function is called at lines 81 and 99, but<util/error.h>is not included. This may cause compilation failures unless transitively included.Proposed fix
`#include` <index/timestampindex.h> `#include` <chain.h> `#include` <logging.h> +#include <util/error.h> `#include` <util/system.h>
🧹 Nitpick comments (4)
src/index/addressindex_util.cpp (1)
12-28: Consider adding bounds checks before byte extraction.The
Is*methods should ensure valid script format, but defensive bounds checking before slice operations would guard against malformed scripts or future changes to these methods.For example,
script.begin() + 22on P2SH assumes at least 23 bytes. WhileIsPayToScriptHash()should only return true for valid P2SH scripts, adding explicit size checks would be more robust.🛡️ Optional defensive check example
bool AddressBytesFromScript(const CScript& script, AddressType& address_type, uint160& address_bytes) { if (script.IsPayToScriptHash()) { + if (script.size() < 23) return false; // P2SH is exactly 23 bytes address_type = AddressType::P2SH; address_bytes = uint160(std::vector<uint8_t>(script.begin() + 2, script.begin() + 22)); } else if (script.IsPayToPublicKeyHash()) { + if (script.size() < 25) return false; // P2PKH is exactly 25 bytes address_type = AddressType::P2PK_OR_P2PKH; address_bytes = uint160(std::vector<uint8_t>(script.begin() + 3, script.begin() + 23)); } else if (script.IsPayToPublicKey()) { + if (script.size() < 35) return false; // P2PK minimum is 35 bytes (compressed) address_type = AddressType::P2PK_OR_P2PKH; address_bytes = Hash160(std::vector<uint8_t>(script.begin() + 1, script.end() - 1)); } else {test/functional/feature_spentindex.py (1)
148-162: Consider addingsync_all()after invalidation to ensure consistent state before verification.The test invalidates blocks on all nodes in a loop but doesn't synchronize before checking
getspentinfoon node 1. While this likely works because node 1's mempool is populated independently, adding a sync would make the test more robust.♻️ Optional improvement
# Invalidate the block containing the spending transaction on all nodes for node in self.nodes: node.invalidateblock(block_hash) + self.sync_all() # After invalidation, transactions go back to mempool # getspentinfo should still find the spend in mempool with height -1src/index/addressindex.cpp (1)
266-318: Minor: Comment doesn't match implementation.The comment at line 266 says "Process transactions in reverse to undo them", but the loop actually processes transactions in forward order (index 0 to vtxundo.size()). The operations performed are still correct reversals, so this is just a documentation inconsistency.
Suggested fix
- // Process transactions in reverse to undo them + // Undo each transaction's index entries // blockundo.vtxundo[i] corresponds to block.vtx[i+1] (coinbase skipped)src/rpc/node.cpp (1)
589-594: Consider moving start/end validation outside the loop.The start/end reset at line 590 is evaluated on every iteration but only needs to happen once. Moving it before the loop would be cleaner.
Suggested refactor
if (!g_addressindex) { throw JSONRPCError(RPC_MISC_ERROR, "Address index is not enabled. Start with -addressindex to enable."); } + if (start <= 0 || end <= 0) { start = 0; end = 0; } for (const auto& address : addresses) { - if (start <= 0 || end <= 0) { start = 0; end = 0; } if (!g_addressindex->GetAddressIndex(address.first, address.second, addressIndex, start, end)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for address"); } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/index/addressindex.cppsrc/index/addressindex.hsrc/index/addressindex_util.cppsrc/index/spentindex.cppsrc/index/spentindex.hsrc/index/timestampindex.cppsrc/rpc/node.cppsrc/txdb.htest/functional/feature_spentindex.pytest/util/data/non-backported.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- test/util/data/non-backported.txt
🧰 Additional context used
📓 Path-based instructions (2)
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/txdb.hsrc/index/addressindex.hsrc/index/spentindex.hsrc/index/spentindex.cppsrc/index/addressindex.cppsrc/index/timestampindex.cppsrc/rpc/node.cppsrc/index/addressindex_util.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_spentindex.py
🧠 Learnings (19)
📓 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: 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/{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/txdb.hsrc/index/addressindex.hsrc/index/spentindex.hsrc/index/spentindex.cppsrc/index/addressindex.cppsrc/rpc/node.cpp
📚 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/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/{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/txdb.hsrc/index/spentindex.hsrc/index/spentindex.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/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/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/txdb.hsrc/index/spentindex.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} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/txdb.h
📚 Learning: 2025-07-22T14:38:45.606Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6738
File: src/evo/smldiff.h:91-94
Timestamp: 2025-07-22T14:38:45.606Z
Learning: In the Dash codebase, EXCLUSIVE_LOCKS_REQUIRED annotations are Clang thread safety annotations that are enforced at compile time on supported platforms like macOS. If callers don't properly hold the required locks, the build will fail with compile-time errors, not runtime issues.
Applied to files:
src/txdb.h
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/txdb.h
📚 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.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.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
📚 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/txdb.h
📚 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/txdb.hsrc/index/addressindex.hsrc/index/spentindex.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/index/addressindex.cppsrc/index/timestampindex.cppsrc/rpc/node.cpp
📚 Learning: 2025-05-05T12:45:44.781Z
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Applied to files:
src/index/timestampindex.cpp
📚 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/node.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/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/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/rpc/node.cpp
🧬 Code graph analysis (7)
src/txdb.h (1)
src/txdb.cpp (2)
MigrateOldIndexData(465-586)MigrateOldIndexData(465-465)
src/index/spentindex.h (1)
src/index/spentindex.cpp (9)
SpentIndex(55-58)SpentIndex(60-60)DB(22-25)WriteBatch(27-39)WriteBatch(27-27)ReadSpentIndex(41-44)ReadSpentIndex(41-41)EraseSpentIndex(46-53)EraseSpentIndex(46-46)
src/index/spentindex.cpp (4)
src/index/addressindex.cpp (7)
DB(21-24)Rewind(244-359)Rewind(244-244)BlockDisconnected(361-373)BlockDisconnected(361-361)GetDB(375-375)GetDB(375-375)src/node/blockstorage.cpp (2)
UndoReadFromDisk(512-543)UndoReadFromDisk(512-512)src/index/addressindex_util.cpp (2)
AddressBytesFromScript(12-29)AddressBytesFromScript(12-12)src/index/spentindex.h (1)
BlockDisconnected(60-63)
test/functional/feature_spentindex.py (3)
test/functional/test_framework/test_framework.py (1)
sync_all(862-864)src/rpc/node.cpp (2)
getspentinfo(780-857)getspentinfo(780-780)test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/index/addressindex.cpp (2)
src/node/blockstorage.cpp (6)
UndoReadFromDisk(512-543)UndoReadFromDisk(512-512)ReadBlockFromDisk(744-767)ReadBlockFromDisk(744-744)ReadBlockFromDisk(769-781)ReadBlockFromDisk(769-769)src/index/addressindex_util.cpp (2)
AddressBytesFromScript(12-29)AddressBytesFromScript(12-12)
src/index/timestampindex.cpp (5)
src/index/spentindex.cpp (5)
DB(22-25)BlockDisconnected(148-160)BlockDisconnected(148-148)GetDB(162-162)GetDB(162-162)src/index/timestampindex_types.h (1)
CTimestampIndexIteratorKey(18-19)src/index/addressindex.h (1)
BlockDisconnected(67-71)src/index/spentindex.h (1)
BlockDisconnected(60-63)src/index/timestampindex.h (1)
BlockDisconnected(49-52)
src/index/addressindex_util.cpp (1)
src/hash.h (1)
Hash160(94-99)
🪛 Cppcheck (2.19.0)
src/index/spentindex.cpp
[error] 16-16: #error Client version information missing
(preprocessorErrorDirective)
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)
⏰ 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_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (16)
src/txdb.h (1)
95-97: LGTM!The
MigrateOldIndexData()declaration is clean with properEXCLUSIVE_LOCKS_REQUIRED(::cs_main)annotation, matching the contract established byLoadBlockIndexGuts. The documentation comment clearly describes the method's purpose.src/index/spentindex.cpp (3)
62-108: LGTM!The
WriteBlockimplementation correctly:
- Skips the genesis block (no undo data available)
- Validates undo data alignment with transaction inputs
- Extracts address information via
AddressBytesFromScript- Batches entries efficiently before writing
110-146: LGTM!The
Rewindimplementation correctly handles chain reorganizations by:
- Validating the ancestor relationship via assertion
- Iterating backwards through disconnected blocks
- Reading blocks from disk to identify spent outputs to remove
- Delegating to
BaseIndex::Rewindfor best-block pointer updates
27-53: LGTM!The DB methods are well-implemented:
WriteBatchefficiently handles both writes and deletions using the null-value conventionEraseSpentIndexproperly batches erasure operations- Clear documentation of the null-value deletion pattern
src/index/spentindex.h (2)
30-77: LGTM!The
SpentIndexclass is well-designed:
- Proper documentation explaining pruning incompatibility and undo data requirement
- Class marked
finalas it's not intended for further derivationAllowPrune() = falsecorrectly enforces the undo data requirement- Clean separation between public API (
GetSpentInfo) and internal operations
15-17: LGTM!The
CSpentIndexTxInfostruct provides a clean container for tracking spent outputs in the mempool context.test/functional/feature_spentindex.py (2)
124-130: LGTM!Good test coverage for mempool integration. The test correctly verifies that
getspentinforeturnsheight=-1for unconfirmed transactions, which matches the RPC implementation that checks mempool after the async index.
49-57: LGTM!Good coverage for the async index behavior. This test verifies the PR's key feature that spentindex can be enabled/disabled without requiring
-reindex, which is a significant usability improvement.src/index/addressindex.h (1)
1-92: LGTM! Well-structured async index header.The header properly:
- Documents the dual-database design (transaction history + UTXO filtering)
- Correctly disables pruning via
AllowPrune() const override { return false; }- Exposes appropriate public query methods with
constqualifiers- Uses proper encapsulation with protected/private members
src/index/timestampindex.cpp (2)
90-102: LGTM! BlockDisconnected properly guards against nullpprev.The condition at line 97 correctly checks
pindex->pprevbefore callingRewind, preventing potential issues when disconnecting the genesis block.
25-44: LGTM! Timestamp range query implementation is correct.The
ReadRangemethod properly:
- Seeks to the lower timestamp bound
- Iterates while checking the upper bound (
m_block_time <= high)- Terminates cleanly when leaving the valid key range
src/index/addressindex.cpp (2)
49-80: LGTM! Type validation now correctly included.The iterator loop at line 62 now properly validates both
m_address_type == typeandm_address_bytes == address_hash, addressing the previous review concern about returning entries of incorrect types.
148-242: LGTM! WriteBlock implementation is well-structured.The implementation correctly:
- Skips genesis block (no undo data)
- Validates undo data matches transaction input count
- Processes coinbase outputs separately (no inputs)
- Uses proper index mapping (
i + 1to skip coinbase in vtx)- Gracefully skips unsupported script types via
AddressBytesFromScriptsrc/rpc/node.cpp (3)
413-415: LGTM! Address index availability check is correct.The RPC handler properly validates that
g_addressindexis enabled before proceeding with mempool queries.
663-669: LGTM! Proper handling of async index state.The implementation correctly:
- Uses the index's
best_block_heightrather than chain tip (avoiding inconsistency during sync)- Checks
syncedstatus before proceeding- Documents the rationale clearly in comments
814-856: LGTM! getspentinfo correctly layers async index with mempool.The implementation properly:
- Validates
outputIndex >= 0(addressing previous review)- Checks the async index first, then mempool
- Correctly overwrites with mempool state (may have newer data during reorg)
- Documents the rationale for mempool priority
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Add null check for pindex->pprev in BlockDisconnected - Add address type validation in AddressIndex iterator loops - Add outputIndex >= 0 validation in getspentinfo RPC - Add EXCLUSIVE_LOCKS_REQUIRED(::cs_main) to MigrateOldIndexData - Use const reference for SpentIndex::GetSpentInfo key parameter - Move m_db to private section in AddressIndex - Add missing <memory> include in addressindex.h - Fix unused variable warning (use _ placeholder) - Fix docstring in feature_spentindex.py - Add .cpp patterns to non-backported.txt 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
5e6534a to
f0a57f2
Compare
Add addressindex, timestampindex, and spentindex to getindexinfo RPC output. Update rpc_misc.py test to verify new indexes are reported. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Check sync status upfront for address, timestamp, and spent index RPCs. Include current sync height in error messages to help users understand progress, following the pattern used by coinstatsindex. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
721ff47 to
0a7a4f1
Compare
|
@coderabbitai pls review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 2525 🏁 Script executed: Length of output: 7264 🏁 Script executed: Length of output: 5305 🏁 Script executed: Length of output: 3828 🏁 Script executed: Length of output: 5229 🏁 Script executed: Length of output: 556 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 438 🏁 Script executed: Length of output: 928 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1536 🏁 Script executed: Length of output: 1928 🏁 Script executed: Length of output: 677 🏁 Script executed: Length of output: 2387 🏁 Script executed: Length of output: 2879 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 147 🏁 Script executed: Length of output: 138 🏁 Script executed: Length of output: 3065 🏁 Script executed: Length of output: 801 🏁 Script executed: Length of output: 2538 🏁 Script executed: Length of output: 1707 🏁 Script executed: Length of output: 306 🏁 Script executed: Length of output: 287 🏁 Script executed: Length of output: 1282
✅ Strengths1. Excellent Migration Logic
2. Robust Index Implementations
3. Proper Pruning Enforcement
4. Comprehensive RPC Guards
|
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: