fix: follow-up changes for #7451 [bitcoin#25308 and related changes] - #7471
fix: follow-up changes for #7451 [bitcoin#25308 and related changes]#7471knst wants to merge 4 commits into
Conversation
|
⛔ Blockers found — Sonnet deferred (commit 570a202) |
|
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 (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughChainstate loading now receives managers, paths, callbacks, and LLMQ defaults through Estimated code review effort: 4 (Complex) | ~45 minutes 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/init.cpp (1)
2095-2104: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTear down the previous chainstate resource graph before a load retry.
A failed load can leave Dash resources bound to the current chain manager, but the reindex retry replaces that manager and
LoadChainstatesubsequently destroys dependencies before dependents.
src/init.cpp#L2095-L2104: release the existing graph before allowing the loop to recreatenode.mempoolandnode.chainman.src/node/chainstate.cpp#L47-L82: perform replacement in reverse dependency order:chain_helper, mempool connection,llmq_ctx,dmnman, thenevodb.🤖 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/init.cpp` around lines 2095 - 2104, Before setting fReindex in the retry path around the failed load check in src/init.cpp lines 2095-2104, tear down the existing chainstate resource graph so the loop can safely recreate node.mempool and node.chainman. In src/node/chainstate.cpp lines 47-82, update the replacement cleanup order to release chain_helper, the mempool connection, llmq_ctx, dmnman, and evodb in reverse dependency order before destroying dependents.
🧹 Nitpick comments (2)
src/test/validation_chainstatemanager_tests.cpp (1)
53-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typographical error in comment.
📝 Proposed fix
- // Initialize chain_helpper + // Initialize chain_helper node.chain_helper.reset();🤖 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/test/validation_chainstatemanager_tests.cpp` around lines 53 - 54, Correct the misspelled “chain_helpper” in the initialization comment above node.chain_helper.reset() to “chain_helper”.src/test/util/setup_common.cpp (1)
328-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcode the function name to avoid unhelpful
__func__output.The
__func__macro inside a lambda expression will evaluate tooperator(), not the enclosing function's name (LoadVerifyActivateChainstate). This results in an unhelpful log message. Consider hardcoding the function name to improve traceability.📝 Proposed fix
- std::tie(status, error) = VerifyLoadedChainstate(*Assert(m_node.chainman), options, [](bool bls_state) { - LogPrintf("%s: bls_legacy_scheme=%d\n", __func__, bls_state); + std::tie(status, error) = VerifyLoadedChainstate(*Assert(m_node.chainman), options, [](bool bls_state) { + LogPrintf("LoadVerifyActivateChainstate: bls_legacy_scheme=%d\n", bls_state); });🤖 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/test/util/setup_common.cpp` around lines 328 - 330, Update the LogPrintf call in the VerifyLoadedChainstate callback within LoadVerifyActivateChainstate to use the enclosing function name explicitly instead of __func__. Preserve the existing bls_state value and log format.
🤖 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/test/util/setup_common.cpp`:
- Around line 404-409: Reorder teardown in the setup cleanup sequence so
m_node.mempool->DisconnectManagers() executes before m_node.llmq_ctx.reset().
Preserve the existing chain_helper and dmnman cleanup behavior, changing only
the ordering needed to disconnect mempool managers while the InstantSend/LLMQ
context remains valid.
---
Outside diff comments:
In `@src/init.cpp`:
- Around line 2095-2104: Before setting fReindex in the retry path around the
failed load check in src/init.cpp lines 2095-2104, tear down the existing
chainstate resource graph so the loop can safely recreate node.mempool and
node.chainman. In src/node/chainstate.cpp lines 47-82, update the replacement
cleanup order to release chain_helper, the mempool connection, llmq_ctx, dmnman,
and evodb in reverse dependency order before destroying dependents.
---
Nitpick comments:
In `@src/test/util/setup_common.cpp`:
- Around line 328-330: Update the LogPrintf call in the VerifyLoadedChainstate
callback within LoadVerifyActivateChainstate to use the enclosing function name
explicitly instead of __func__. Preserve the existing bls_state value and log
format.
In `@src/test/validation_chainstatemanager_tests.cpp`:
- Around line 53-54: Correct the misspelled “chain_helpper” in the
initialization comment above node.chain_helper.reset() to “chain_helper”.
🪄 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: 1418401a-d0ad-4ee0-8d87-ac762aed5f1c
📒 Files selected for processing (9)
src/bitcoin-chainstate.cppsrc/init.cppsrc/llmq/options.hsrc/llmq/signing.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/test/validation_chainstatemanager_tests.cpp
💤 Files with no reviewable changes (2)
- src/llmq/signing.h
- src/test/util/setup_common.h
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The exact head 4bde03a and full review range were verified. The final tree contains one behavior-changing Dash adaptation, and the commit stack contains an intermediate commit that cannot compile because its inlined chainstate code uses undeclared identifiers. The stack should be rewritten to keep every commit buildable and preserve the dash-chainstate option values.
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)
🔴 1 blocking | 🟡 1 suggestion(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/chainstate.cpp`:
- [BLOCKING] src/node/chainstate.cpp:78-80: Fold the build-restoring chainstate changes into the refactor
Commit 171e0361b95 inlines DashChainstateSetup into LoadChainstate but passes the undeclared identifiers dash_dbs_in_memory, fReset, fReindexChainState, bls_threads, worker_count, max_recsigs_age, and mempool. Commit 4a49ad20725 later replaces them with ChainstateLoadOptions members, confirming that 171e0361b95 cannot compile independently. Fold those build-restoring changes into the refactor so every permanent commit remains buildable and usable for bisecting.
In `src/bitcoin-chainstate.cpp`:
- [SUGGESTION] src/bitcoin-chainstate.cpp:110-118: Incorrect transformation: dash-chainstate option values are not preserved
Before this range, dash-chainstate explicitly set bls_threads, worker_count, and max_recsigs_age to 1, preserving the arguments used before the bitcoin#25308 parameter-to-options refactor. This change removes those assignments and inherits defaults of 0 BLS-check threads, a hardware-dependent worker count between 1 and 4, and a recovered-signature age of 604800 seconds. That changes the executable's behavior even though the backport only requires changing how the values are passed. Keep the three explicit values unless this behavioral change is proposed and justified separately.
|
Fable found a few issues: src/node/chainstate.cpp
src/node/chainstate.h
src/test/validation_chainstatemanager_tests.cpp
nits:
|
|
This pull request has conflicts, please rebase. |
…etup They are pretty trivial and simple but they are obstacle to implement bitcoin#25308 due to un-needed complexity
Revert changes that introduced by non-backported yet bitcoin#31061 Set default value for worker_count Move data_dir, mn_metaman, sporkman, chainlocks, mn_sync to node::ChainstateLoadOptions same as mempool is done Pass notify_bls_state to VerifyLoadedChainstate only Added missing changes for strLoadError from bitcoin#25308 Added missing changes for node::fPruneMode node::fReindex from bitcoin#25308 Fixed duplicated entities in src/test/util/setup_common.cpp as follow-up for bitcoin#25290
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The exact head contains the intended manager-lifetime fixes, and all eight previously reported header, pointer-contract, test-lifetime, callback, and cleanup issues are resolved. Two issues remain: the permanent commit stack contains an unbuildable intermediate refactor, and the dash-chainstate adaptation silently replaces three explicit LLMQ values with materially different defaults.
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 (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)
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/bitcoin-chainstate.cpp`:
- [SUGGESTION] src/bitcoin-chainstate.cpp:109-116: Preserve dash-chainstate option values through the refactor
Before bitcoin#25308, this executable passed `1` for `bls_threads`, `worker_count`, and `max_recsigs_age`; the original Dash backport preserved those values as explicit `ChainstateLoadOptions` assignments. This PR removes them, causing the executable to inherit `DEFAULT_BLSCHECK_THREADS` (`0`), a hardware-dependent worker count from 1 to 4, and `DEFAULT_MAX_RECOVERED_SIGS_AGE` (`604800`). That is a behavior change hidden inside a parameter-to-options follow-up. Restore the explicit values unless the behavior change is proposed and justified separately.
Issue being fixed or feature implemented
Follow-up changes for #7451
What was done?
They are pretty trivial and simple but they are obstacle to backport refactor: Reduce number of LoadChainstate parameters and return values bitcoin/bitcoin#25308 as close as possible due to un-needed complexity
CTxMemPoolfromArgsManagerbitcoin/bitcoin#25290How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: