backport: bitcoin#25494 (kernel 2)#7462
Conversation
|
🔍 Review in progress — actively reviewing now (commit b0ad438) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
HEAD matches b3b91c0. Independent source and history checks confirm four in-scope compilation blockers: three incomplete bitcoin#25494 adaptations and one defect in the Dash-side chainstate refactor. The PR cannot build until all four are corrected.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol/high— general (failed),gpt-5.6-sol/high— dash-core-commit-history (failed),gpt-5.6-sol/high— backport-reviewer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking
1 additional finding(s) omitted (not in diff).
🤖 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/chainstate.cpp`:
- [BLOCKING] src/node/chainstate.cpp:78-81: Reference ChainstateLoadOptions after inlining Dash setup
Commit 8a7cf8ea04f inlined DashChainstateSetup into LoadChainstate but retained seven names that were parameters or caller-local variables before the refactor. `dash_dbs_in_memory`, `fReset`, `fReindexChainState`, `bls_threads`, `worker_count`, `max_recsigs_age`, and `mempool` are not declared in this scope. ChainstateLoadOptions exposes the corresponding values through `options`, including the combined LLMQ database wipe condition used by the removed call.
In `src/wallet/wallet.cpp`:
- [BLOCKING] src/wallet/wallet.cpp:1484: Conflict resolution drops the required WalletBatch argument
The Dash base passed `batch` as the third argument to SyncTransaction, but the BlockInfo conversion copied upstream's two-argument call shape. Dash's surviving declaration requires `SyncTransaction(const CTransactionRef&, const SyncTxState&, WalletBatch&, ...)`, and no two-argument overload exists. The already-created WalletBatch must remain attached to this call.
- [BLOCKING] src/wallet/wallet.cpp:1505: Conflict resolution leaves a stale height parameter use
The callback now receives only `interfaces::BlockInfo`, so the former `height` parameter no longer exists. Dash's conflict-recovery code was left using that removed identifier even though the surrounding converted code reads `block.height`. This is an undeclared-identifier compilation failure.
In `src/test/validation_chainstatemanager_tests.cpp`:
- [BLOCKING] src/test/validation_chainstatemanager_tests.cpp:568-569: Dash-only TxIndex restart callsite still uses removed APIs
bitcoin#25494 changed TxIndex to require a `std::unique_ptr<interfaces::Chain>` constructor argument and changed BaseIndex::Start to take no arguments. This Dash-only snapshot restart path still uses both old signatures, while the production and fixture call sites were updated. Neither expression matches the declarations at the reviewed head, so the unit-test target cannot compile.
…al step 198dc72 feat: remove g_txindex and govman from TestChainSetup as optional step (Konstantin Akimov) 901d908 refactor: remove GetTransaction and g_txindex usages from regressions tests of block-reward-reallocation (Konstantin Akimov) c9ddd4b refactor: remove GetTransaction and g_txindex usages from regressions tests of deterministicmns (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented g_txindex has own tests and there's no needs in tx-index in the common environment for regression tests This PR is a blocker for #7462 - particularly bitcoin#25494 ## What was done? Removed usages of g_txindex by calling `GetTransaction` from block_reward_reallocation_tests.cpp, evo_deterministicmns_tests.cpp Simplified and removed duplicated code from `evo_deterministicmns_tests.cpp` ## How Has This Been Tested? Run unit & functional tests. ## Breaking Changes n/a ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: a50e2a3ab85eef3a343e854f425174d6d4d0be840beb204893852f14b5cef7ad7e77c7024a16270ba7f6a4cb49a159b9e47c0320600efb583b52287b8312388b
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
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:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ea40d8a14
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
WalkthroughThe index framework now injects Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Chainstate
participant BaseIndex
participant IndexHook
participant IndexDB
Chainstate->>BaseIndex: provide block connection
BaseIndex->>IndexHook: CustomAppend(BlockInfo)
IndexHook->>IndexDB: write index entries
BaseIndex->>IndexDB: commit batch and best-block locator
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
src/index/base.cpp (1)
37-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
GetLocatorstatic.This helper function is only used within this translation unit. It should be declared
static(as it was in the upstream Bitcoin Core PR) or placed in an anonymous namespace to prevent global namespace pollution and potential One-Definition Rule (ODR) violations.♻️ Proposed refactor
-CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash) +static CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash) { CBlockLocator locator; bool found = chain.findBlock(block_hash, interfaces::FoundBlock().locator(locator)); assert(found); assert(!locator.IsNull()); return locator; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index/base.cpp` around lines 37 - 45, Declare the GetLocator function with internal linkage by adding static to its definition or placing it in an anonymous namespace. Keep its existing behavior and call sites unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wallet/wallet.cpp`:
- Line 1484: Update the SyncTransaction call in the block transaction-sync flow
to pass the existing WalletBatch instance as the required third argument,
preserving the block hash and height state arguments in their current order.
- Line 1505: Update the conflict-resolution callback around disconnect_height to
initialize it from the current height in the received interfaces::BlockInfo&
block parameter instead of the removed height argument, preserving the existing
disconnect behavior.
---
Nitpick comments:
In `@src/index/base.cpp`:
- Around line 37-45: Declare the GetLocator function with internal linkage by
adding static to its definition or placing it in an anonymous namespace. Keep
its existing behavior and call sites unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cacdbb27-7219-4661-96d7-7ac45adafd2c
📒 Files selected for processing (29)
src/Makefile.amsrc/core_write.cppsrc/index/addressindex.cppsrc/index/addressindex.hsrc/index/base.cppsrc/index/base.hsrc/index/blockfilterindex.cppsrc/index/blockfilterindex.hsrc/index/coinstatsindex.cppsrc/index/coinstatsindex.hsrc/index/spentindex.cppsrc/index/spentindex.hsrc/index/spentindex_types.hsrc/index/timestampindex.cppsrc/index/timestampindex.hsrc/index/txindex.cppsrc/index/txindex.hsrc/init.cppsrc/interfaces/chain.hsrc/kernel/chain.cppsrc/kernel/chain.hsrc/node/interfaces.cppsrc/test/blockfilter_index_tests.cppsrc/test/coinstatsindex_tests.cppsrc/test/txindex_tests.cppsrc/wallet/test/fuzz/notifications.cppsrc/wallet/wallet.cppsrc/wallet/wallet.htest/lint/lint-circular-dependencies.py
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Exact head 9ea40d8 retains two blocking wallet conflict-resolution errors: one call omits the required WalletBatch argument, and blockDisconnected references a removed height parameter. Both CodeRabbit comments independently identify the same confirmed issues, so changes are required before the backport can compile.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— backport-reviewer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— backport-reviewer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
7878f97 indexes, refactor: Remove CChainState use in index CommitInternal method (Ryan Ofsky) ee3a079 indexes, refactor: Remove CBlockIndex* uses in index Rewind methods (Ryan Ofsky) dc971be indexes, refactor: Remove CBlockIndex* uses in index WriteBlock methods (Ryan Ofsky) bef4e40 indexes, refactor: Remove CBlockIndex* uses in index Init methods (Ryan Ofsky) addb4f2 indexes, refactor: Remove CBlockIndex* uses in coinstatsindex LookUpOne function (Ryan Ofsky) 33b4d48 indexes, refactor: Pass Chain interface instead of CChainState class to indexes (Ryan Ofsky) a0b5b4a interfaces, refactor: Add more block information to block connected notifications (Ryan Ofsky) Pull request description: Start transitioning index code away from using internal node types like `CBlockIndex` and `CChain` so index code is less coupled to node code and index code will later be able to stop locking cs_main and sync without having to deal with validationinterface race conditions, and so new indexes are easier to write and can run as plugins or separate processes. This PR contains the first 7 commits from bitcoin#24230 (comment) which have been split off for easier review. Previous review comments can be found in bitcoin#24230 ACKs for top commit: MarcoFalke: ACK 7878f97 though did not review the last commit 🤼 mzumsande: Code Review ACK 7878f97 Tree-SHA512: f84ac2eb6dca2c305566ddeb35ea14d0b71c00860c0fd752bbcf1a0188be833d8c2a6ac9d3ef6ab5b46fbd02d7a24cbb8f60cf12464cb8ba208e22287f709989 Co-authored-by: fanquake <fanquake@gmail.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0ad438439
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!Init()) return false; | ||
|
|
||
| const CBlockIndex* index = m_best_block_index.load(); | ||
| if (!CustomInit(index ? std::make_optional(interfaces::BlockKey{index->GetBlockHash(), index->nHeight}) : std::nullopt)) { |
There was a problem hiding this comment.
Because the validation interface is registered before Init(), queued/background validation events can reach this index as soon as Init() latches m_synced true, but CustomInit() has not loaded subclass state yet. When starting an already-synced index while callbacks are pending, a BlockConnected in this window can append using uninitialized blockfilter file position or zeroed coinstats MuHash/totals, corrupting the index; keep callbacks gated until after CustomInit() completes or initialize custom state before setting m_synced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not actionable for this backport. The merged bitcoin#25494 implementation has this exact RegisterValidationInterface → Init() → CustomInit() ordering, including the comment explaining why registration precedes Init() (upstream lines 369–386). The backport is faithful here, and this finding does not identify a Dash-specific interaction. Any remaining lifecycle concern would need to be handled upstream or as a separate follow-up, not as a bitcoin#25494 backport fix.
Issue being fixed or feature implemented
Next batch of bitcoin backports
What was done?
Backport bitcoin#25494 and relevant refactoring
How Has This Been Tested?
Run unit / functional tests.
Breaking Changes
N/A
Checklist: