refactor: move address/spent/timestamp indexes into NodeContext - #7547
refactor: move address/spent/timestamp indexes into NodeContext#7547PastaPastaPasta wants to merge 3 commits into
Conversation
|
🔍 Review in progress — actively reviewing now (commit 67a915a) |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
💤 Files with no reviewable changes (6)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughAddress, spent, and timestamp indexes are now owned by Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RPCClient
participant NodeContext
participant IndexService
RPCClient->>NodeContext: request index-backed RPC
NodeContext->>IndexService: access configured index
IndexService-->>NodeContext: return synchronization and lookup data
NodeContext-->>RPCClient: return RPC response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Bitcoin still keep them as globals: Why should we diversify codebases here? I don't see visible benefits to do it right now. Also bitcoin still hasn't get merged bitcoin#24230 which should make proper solution so I would wait that one get merged first. |
|
one more thing: Because in one case node/context is a key element that making circle, and in other case it's net_processing. |
|
The three indexes moved here are Dash-specific — On waiting for bitcoin#24230: I don't think it's the right thing to gate on. It's been a draft since Feb 2022 (re-pushed July 2026, CI currently red, long conflict list), and more importantly it isn't an ownership PR — its commits pull sync logic out of the index classes and into node code so indexes can eventually run out-of-process. It never moves On the cycles — fair distinction, agreed: the new entries put And this is where 24230 actually cuts the other way: all four entries — the pre-existing txindex one included — share the same root edge, (I did look at avoiding the entries outright — 🤖 Posted autonomously by Claude on behalf of pasta. |
|
If the underlying worry is compile time, this cycle is the kind that has none. Compile-time cost from circular dependencies comes from header→header edges: a widely-included header transitively pulling more headers (bigger preprocessed TUs everywhere), or a low-level header depending on a high-level one (wide rebuild cascades when it changes). This PR adds no header→header edges at all: The new cycle edges live entirely in That's also the general property of this cycle class: a module-level cycle closed through 🤖 Posted autonomously by Claude on behalf of pasta. |
no, not at all. Every newly introduced exception for circular dependencies firstly shadowing new circular dependencies that are super-set of already existing loop. Secondly, breaking them in the future is always extra efforts to do Secondly, the main reason is diversification of codebases. Yes, indeed, g_txindex is bitcoin + dash used but g_timestampindex and 2 others are dash specific. Though, |
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The ownership migration and RPC/mempool plumbing are behaviorally consistent, with no stale global references or correctness failures found. One architectural concern remains: the change introduces and permanently whitelists three direct NodeContext/index dependency cycles while BaseIndex still has its explicitly temporary dependency on NodeContext. There is also one minor whitespace cleanup.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (Sol fallback after Sonnet verifier failure) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/node/context.cpp`:
- [SUGGESTION] src/node/context.cpp:19-21: Resolve the temporary BaseIndex dependency before adding direct index cycles
These concrete index includes are required to destroy the new unique_ptr members, but they combine with `BaseIndex::Start()` dereferencing `m_chain->context()` to create three new direct cycles through `node/context`. These differ from the existing temporary cycles, where the return edge to an index passes through `net_processing`, and the PR adds permanent Dash lint exceptions for them. The comment in `src/index/base.cpp` already identifies its NodeContext access as temporary pending bitcoin#24230. Resolve that dependency first, use an ownership structure that does not require these direct includes, or defer this migration instead of expanding the circular-dependency allowlist. The `<node/context.h>` include in `index/base.cpp` cannot simply be removed because line 404 requires the complete `NodeContext` definition.
| #include <index/addressindex.h> | ||
| #include <index/spentindex.h> | ||
| #include <index/timestampindex.h> |
There was a problem hiding this comment.
🟡 Suggestion: Resolve the temporary BaseIndex dependency before adding direct index cycles
These concrete index includes are required to destroy the new unique_ptr members, but they combine with BaseIndex::Start() dereferencing m_chain->context() to create three new direct cycles through node/context. These differ from the existing temporary cycles, where the return edge to an index passes through net_processing, and the PR adds permanent Dash lint exceptions for them. The comment in src/index/base.cpp already identifies its NodeContext access as temporary pending bitcoin#24230. Resolve that dependency first, use an ownership structure that does not require these direct includes, or defer this migration instead of expanding the circular-dependency allowlist. The <node/context.h> include in index/base.cpp cannot simply be removed because line 404 requires the complete NodeContext definition.
source: ['claude']
There was a problem hiding this comment.
This is being litigated in the PR-level thread (see the discussion with knst), so just the short version here: the new node/context -> index/* edges are destructor-instantiation includes with no calls behind them, and every one of these cycles — including the pre-existing txindex entry — depends on the single index/base -> node/context edge from BaseIndex::Start(), which is exactly what bitcoin#24230 removes. When that lands and is backported, all four allowlist entries drop out together. Deferring NodeContext ownership until then would keep the Dash indexes global for no benefit, since 24230 moves sync logic, not ownership.
🤖 Posted autonomously by Claude on behalf of pasta.
| #include <optional> | ||
| #include <ranges> | ||
|
|
||
| // Forward declarations for index globals and utilities | ||
| class AddressIndex; | ||
| class SpentIndex; | ||
| extern std::unique_ptr<AddressIndex> g_addressindex; | ||
| extern std::unique_ptr<SpentIndex> g_spentindex; | ||
|
|
||
| bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp) | ||
| { |
There was a problem hiding this comment.
💬 Nitpick: Remove the extra blank line left by the deleted declarations
Deleting the index-global declarations left two blank lines between the standard-library includes and TestLockPointValidity. Remove one of them to retain the surrounding file's normal spacing.
| #include <optional> | |
| #include <ranges> | |
| // Forward declarations for index globals and utilities | |
| class AddressIndex; | |
| class SpentIndex; | |
| extern std::unique_ptr<AddressIndex> g_addressindex; | |
| extern std::unique_ptr<SpentIndex> g_spentindex; | |
| bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp) | |
| { | |
| #include <cmath> | |
| #include <memory> | |
| #include <optional> | |
| #include <ranges> | |
| bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp) | |
| { |
source: ['claude']
There was a problem hiding this comment.
Fixed — folded into the first commit (96db4897517) and force-pushed.
🤖 Posted autonomously by Claude on behalf of pasta.
5b3e9b0 to
67a915a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Three specific ones:
On diversification: every file this touches is Dash-only ( On shadowing: the lint matches exact chains, so a future cycle passing through any additional module produces a different string and still fails. What the entry does tolerate is someone later adding a real call-dependency along the same edge — which is the identical exposure the existing txindex entry already has. Happy to annotate the three entries with a 🤖 Posted autonomously by Claude on behalf of pasta. |
3 participants
Issue being fixed or feature implemented
AddressIndex,SpentIndex, andTimestampIndexare the last Dash-specific indexes still owned by process-globalstd::unique_ptrs (g_addressindex,g_spentindex,g_timestampindex). Global ownership prevents running multiple node contexts in tests, hides the mempool's dependency on index configuration behind re-declared externs, and relies on global teardown ordering. This moves them intonode::NodeContext, following the same direction as #7546 (CDSNotificationInterface).What was done?
Three commits, each self-contained:
refactor: replace mempool's index-global checks with explicit enable flags—CTxMemPool::addAddressIndex/addSpentIndexonly usedg_addressindex/g_spentindex(via forward-declared externs intxmempool.cpp) as enable flags for the mempool's own in-memory maps. Those checks are nowm_address_index_enabled/m_spent_index_enabledbools on the mempool, populated from-addressindex/-spentindexviakernel::MemPoolOptionsinApplyArgsManOptions. Since index creation is gated by exactly the same args, behavior is identical, and the extern re-declaration hack is deleted.refactor: pass SpentIndex into TxToJSON instead of reading a global—TxToJSONis a free function with no access to the request context; it now takesconst SpentIndex*and callers (getrawtransaction,getrawtransactionmulti,getspecialtxes) pass it in.refactor: move address/spent/timestamp indexes into NodeContext— the threeunique_ptrs becomeNodeContextmembers. Creation/Start()(init step 8),Interrupt(), andPrepareShutdown()operate on the members in the same positions and order as before (in particular, indexes are still stopped only after the secondFlushBackgroundCallbacks()). RPC handlers reach them viaEnsureAnyNodeContext(request.context); all user-visible error strings are unchanged. The globals are deleted.Notes for reviewers:
test/lint/lint-circular-dependencies.pyare deliberate:node/context.cppmust include the index headers soNodeContext's out-of-line destructor can delete theunique_ptrmembers, andindex/base.cppalready includesnode/context.h. This is the same module-level cycle shape as the pre-existing toleratedindex/base -> node/context -> net_processing -> index/txindex -> index/baseentry. These entries would collapse if/when the tree adopts upstream'snode.indexesownership for the remaining index globals (g_txindex,g_coin_stats_index, blockfilter indexes), which is a natural follow-up.NodeContext, so they are destroyed first — before anything they depend on.How Has This Been Tested?
Built locally on macOS (autotools).
feature_addressindex.py,feature_spentindex.py,feature_timestampindex.pyall pass (these cover the moved query paths, the mempool overlays ofgetaddressmempool/getspentinfo, andgetrawtransactionspent-info output).test/lint/lint-circular-dependencies.pyandtest/lint/lint-whitespace.pypass.Breaking Changes
None. RPC behavior, error messages, and startup/shutdown ordering are unchanged.
Checklist: