fix: follow-up changes for #7451 [bitcoin#25308 and related changes] - #7471
Merged
Conversation
5 tasks
|
⛔ Blockers found — Sonnet deferred (commit a3fdcd6) |
|
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 (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughChainstate loading now uses Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Startup as Chainstate startup
participant Options as ChainstateLoadOptions
participant Loader as LoadChainstate
participant Resources as Chainstate resources
participant Verifier as VerifyLoadedChainstate
Startup->>Options: populate managers, paths, and callbacks
Startup->>Loader: pass options and dependency outputs
Loader->>Resources: initialize EvoDB, LLMQ, MN, and helper resources
Startup->>Verifier: pass options and BLS callback
Verifier-->>Startup: return verification status and error
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
suggested changes
Jul 17, 2026
thepastaclaw
left a comment
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:
|
knst
marked this pull request as draft
July 21, 2026 16:32
|
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
suggested changes
Jul 31, 2026
thepastaclaw
left a comment
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.
UdjinM6
added a commit
to UdjinM6/dash
that referenced
this pull request
Jul 31, 2026
- node/chainstate.h: include <util/fs.h> for the by-value fs::path member instead of relying on the transitive include from <validation.h>, and drop the now-unused forward declaration of fs::path - init.cpp, test/util/setup_common.cpp: __func__ inside a lambda expands to "operator()", so spell out the enclosing function name in the bls_legacy_scheme log line - init.cpp: note why the mempool has to release the managers before they are destroyed - validation_chainstatemanager_tests.cpp: drop the two vestigial clhandler->Stop() calls, and the include they needed The last one used to read `m_node.llmq_ctx->Stop()`, which stopped the LLMQ worker threads before `DashChainstateSetupClose()` destroyed the context. When clhandler moved out of LLMQContext into NodeContext those calls were mechanically retargeted at the chainlock handler, which is a different object with a different lifetime. `ChainlockHandler::Stop()` only stops the handler's own scheduler, and `ChainlockHandler::Start()` is never called outside of init.cpp, so in unit tests nothing is ever queued on it. The handler is also neither destroyed here nor registered with the validation interface in the test fixtures, and ~ChainlockHandler() already stops and joins its thread.
|
pls see 73516a3 |
- node/chainstate.h: include <util/fs.h> for the by-value fs::path member instead of relying on the transitive include from <validation.h>, and drop the now-unused forward declaration of fs::path - init.cpp, test/util/setup_common.cpp: __func__ inside a lambda expands to "operator()", so spell out the enclosing function name in the bls_legacy_scheme log line - init.cpp: note why the mempool has to release the managers before they are destroyed - validation_chainstatemanager_tests.cpp: drop the two vestigial clhandler->Stop() calls, and the include they needed The last one used to read `m_node.llmq_ctx->Stop()`, which stopped the LLMQ worker threads before `DashChainstateSetupClose()` destroyed the context. When clhandler moved out of LLMQContext into NodeContext those calls were mechanically retargeted at the chainlock handler, which is a different object with a different lifetime. `ChainlockHandler::Stop()` only stops the handler's own scheduler, and `ChainlockHandler::Start()` is never called outside of init.cpp, so in unit tests nothing is ever queued on it. The handler is also neither destroyed here nor registered with the validation interface in the test fixtures, and ~ChainlockHandler() already stops and joins its thread.
thepastaclaw
suggested changes
Aug 1, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The final tree at a3fdcd6 has coherent chainstate option wiring and manager teardown, and the dash-chainstate defaults change is explicitly within the PR's stated scope. The PR remains blocked because permanent ancestor 0774f56 cannot compile independently; the latest review-feedback bundle should also be split and folded into the commits it amends.
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 additional finding(s) omitted (not in diff).
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 `<commit:a3fdcd6>`:
- [SUGGESTION] <commit:a3fdcd6>:1: Split and fold the generic review-feedback commit
Commit a3fdcd6c7f2 bundles corrections belonging to different logical changes: the complete `fs::path` include and corrected lambda log names amend bf1608e414a, while the manager-lifetime explanation belongs with 570a2028367. Fold those changes into their owning commits. The vestigial ChainlockHandler cleanup can either be folded into the test refactor or retained as a focused commit with a durable subject such as `test: drop vestigial ChainlockHandler stops`, rather than keeping a review-conversation-dependent bundle.
UdjinM6
approved these changes
Aug 1, 2026
5 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: