Repository navigation
backport: bitcoin#27125, #27419, #27491, #27611, #28379, partials #27576, #27636 - #8028
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
This pull request has conflicts, please rebase. |
|
✅ Final review complete — no blockers (commit 47f85a4) · triage: normal |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db00632ae0
ℹ️ 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".
| }; | ||
| node::BlockManager::Options blockman_opts{ | ||
| .chainparams = chainman_opts.chainparams, | ||
| .blocks_dir = gArgs.GetBlocksDirPath(), |
There was a problem hiding this comment.
When the experimental dash-chainstate tool is invoked with a scratch DATADIR, this commit removes the ForceSetArg that populated gArgs but still initializes the block manager here, the chainstate, and the Dash databases from gArgs. Those paths therefore fall back to the default ~/.dashcore, so the tool can read or write the live default databases while merely printing the supplied path; pass abs_datadir (and its blocks subdirectory) to every storage option instead.
AGENTS.md reference: AGENTS.md:L197-L202
Useful? React with 👍 / 👎.
| public: | ||
| virtual ~Notifications(){}; | ||
|
|
||
| virtual void blockTip(SynchronizationState state, CBlockIndex& index) {} |
There was a problem hiding this comment.
When a ChainLock marks an active-chain block conflicting, MarkConflictingBlock still invokes uiInterface.NotifyBlockTip directly at src/validation.cpp:4341 rather than this new method. A ChainstateManager supplied with a custom kernel::Notifications implementation consequently misses that Dash-specific tip change even though activate and invalidate paths report it; route the ChainLock path through m_chainman.GetNotifications().blockTip(...) as well.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| @@ -6246,7 +6215,7 @@ bool DeleteSnapshotChainstateFromDisk() | |||
| { | |||
| AssertLockHeld(::cs_main); | |||
|
|
|||
| auto snapshot_datadir = node::FindSnapshotChainstateDir(); | |||
| auto snapshot_datadir = node::FindSnapshotChainstateDir(gArgs.GetDataDirNet()); | |||
There was a problem hiding this comment.
When LoadChainstate reindexes a manager whose options.data_dir differs from process-global gArgs (the decoupled kernel/embedding case), this cleanup checks and may delete the global snapshot directory, while the later DetectSnapshotChainstate now scans m_options.datadir. That can remove an unrelated snapshot and leave the target snapshot in place so reindex startup fails; pass the load/manager datadir into DeleteSnapshotChainstateFromDisk instead.
AGENTS.md reference: AGENTS.md:L197-L202
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Phase 1 blocker gate
Verified all six supplied findings against head 98834f1: three compile-breaking leftovers, an experimental-tool datadir regression, and two incomplete Dash-specific migrations remain. CI reports a failing lint check and pending source builds; the workflow is still running and failed-job logs were unavailable, so the lint failure's cause is unconfirmed. Validation was static only; no builds or tests were run.
Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.
🔴 4 blocking | 🟡 2 suggestion(s)
6 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Duplicate chainparams definition breaks build
src/bitcoin-chainstate.cpp:119-120
main() already defines auto chainparams = CChainParams::Main(); at line 70. This second declaration is in the same scope and makes the dash-chainstate target fail to compile. Remove the second declaration and reuse the existing object.
// SETUP: Chainstate
source: muse-spark-1.3-contributor (phase1-reviewer: general)
🔴 Blocking: dash-chainstate ignores DATADIR argument, uses live datadir
src/bitcoin-chainstate.cpp:121-129
The PR removes the ForceSetArg call that connected argv[1] to gArgs, but chainman_opts.datadir and blockman_opts.blocks_dir still come from gArgs. CEvoDB, LLMQContext database parameters, and ChainstateLoadOptions::data_dir also retain global paths. Once the duplicate declaration is fixed, invoking this experimental tool with a scratch DATADIR can therefore open the default live databases instead, despite printing the supplied path and warning against using the actual datadir. Pass abs_datadir, and its blocks subdirectory where appropriate, to every storage option.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
🔴 Blocking: Uses removed CBaseChainParams::REGTEST constant
src/wallet/test/wallet_tests.cpp:753
This PR removes CBaseChainParams::REGTEST and changes TestChainSetup to take ChainType, but the AssetLockTestingSetup constructor still uses the removed constant. wallet_tests.cpp is included in the wallet unit-test target, so that target cannot compile. Use ChainType::REGTEST.
AssetLockTestingSetup() : TestChainSetup{107, ChainType::REGTEST, {"-dip3params=109:500"}}
source: muse-spark-1.3-contributor (phase1-reviewer: general)
🔴 Blocking: Uses removed CBaseChainParams::MAIN constant
src/wallet/test/fuzz/scriptpubkeyman.cpp:27
This PR removes CBaseChainParams::MAIN and changes SelectParams to accept ChainType, but initialize_spkm() still references the deleted constant. The wallet fuzz source remains included in Makefile.test.include, so builds enabling this target fail to compile. Use ChainType::MAIN.
SelectParams(ChainType::MAIN);
source: muse-spark-1.3-contributor (phase1-reviewer: general)
🟡 Suggestion: ChainLock conflict path bypasses kernel notifications
src/validation.cpp:4339-4342
MarkConflictingBlock disconnects conflicting active-chain blocks but still reports the resulting tip directly through uiInterface.NotifyBlockTip. ActivateBestChain and InvalidateBlock now report through the supplied kernel::Notifications implementation, so a custom notifier misses this Dash-specific tip transition. Route this path through m_chainman.GetNotifications().blockTip as well; the node notifier already forwards that callback to uiInterface, preserving normal node behavior.
// Only notify about a new block tip if the active chain was modified.
if (pindex_was_in_chain && pindex->pprev) {
m_chainman.GetNotifications().blockTip(GetSynchronizationState(IsInitialBlockDownload()), *pindex->pprev);
}
source: muse-spark-1.3-contributor (phase1-reviewer: general)
🟡 Suggestion: Snapshot cleanup uses global datadir while detection uses options datadir
src/validation.cpp:6214-6219
The migrated DeleteSnapshotChainstateFromDisk call explicitly uses gArgs.GetDataDirNet(), while ActivateSnapshot cleanup and DetectSnapshotChainstate now use m_options.datadir. LoadChainstate invokes this deletion during reindex. If the configured manager datadir differs from the global one, cleanup can delete an unrelated snapshot while leaving the intended snapshot in place, after which detection attempts to revive the snapshot that reindex needed to discard. Pass the configured manager datadir into this helper and update its caller.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-gate-verifier, role: verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large, cross-cutting kernel decoupling refactor, but the supplied changes do not clearly alter consensus rules, funds handling, cryptography, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— verifier; agentsol-gate-verifier - Phase 2 reviewers: not run (deferred by blocker gate)
🤖 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`:
- [BLOCKING] src/bitcoin-chainstate.cpp:119-120: Duplicate `chainparams` definition breaks build
main() already defines `auto chainparams = CChainParams::Main();` at line 70. This second declaration is in the same scope and makes the dash-chainstate target fail to compile. Remove the second declaration and reuse the existing object.
- [BLOCKING] src/bitcoin-chainstate.cpp:121-129: dash-chainstate ignores DATADIR argument, uses live datadir
The PR removes the ForceSetArg call that connected argv[1] to gArgs, but chainman_opts.datadir and blockman_opts.blocks_dir still come from gArgs. CEvoDB, LLMQContext database parameters, and ChainstateLoadOptions::data_dir also retain global paths. Once the duplicate declaration is fixed, invoking this experimental tool with a scratch DATADIR can therefore open the default live databases instead, despite printing the supplied path and warning against using the actual datadir. Pass abs_datadir, and its blocks subdirectory where appropriate, to every storage option.
In `src/wallet/test/wallet_tests.cpp`:
- [BLOCKING] src/wallet/test/wallet_tests.cpp:753: Uses removed `CBaseChainParams::REGTEST` constant
This PR removes CBaseChainParams::REGTEST and changes TestChainSetup to take ChainType, but the AssetLockTestingSetup constructor still uses the removed constant. wallet_tests.cpp is included in the wallet unit-test target, so that target cannot compile. Use ChainType::REGTEST.
In `src/wallet/test/fuzz/scriptpubkeyman.cpp`:
- [BLOCKING] src/wallet/test/fuzz/scriptpubkeyman.cpp:27: Uses removed `CBaseChainParams::MAIN` constant
This PR removes CBaseChainParams::MAIN and changes SelectParams to accept ChainType, but initialize_spkm() still references the deleted constant. The wallet fuzz source remains included in Makefile.test.include, so builds enabling this target fail to compile. Use ChainType::MAIN.
In `src/validation.cpp`:
- [SUGGESTION] src/validation.cpp:4339-4342: ChainLock conflict path bypasses kernel notifications
MarkConflictingBlock disconnects conflicting active-chain blocks but still reports the resulting tip directly through uiInterface.NotifyBlockTip. ActivateBestChain and InvalidateBlock now report through the supplied kernel::Notifications implementation, so a custom notifier misses this Dash-specific tip transition. Route this path through m_chainman.GetNotifications().blockTip as well; the node notifier already forwards that callback to uiInterface, preserving normal node behavior.
- [SUGGESTION] src/validation.cpp:6214-6219: Snapshot cleanup uses global datadir while detection uses options datadir
The migrated DeleteSnapshotChainstateFromDisk call explicitly uses gArgs.GetDataDirNet(), while ActivateSnapshot cleanup and DetectSnapshotChainstate now use m_options.datadir. LoadChainstate invokes this deletion during reindex. If the configured manager datadir differs from the global one, cleanup can delete an unrelated snapshot while leaving the intended snapshot in place, after which detection attempts to revive the snapshot that reindex needed to discard. Pass the configured manager datadir into this helper and update its caller.
thepastaclaw
added
the
pastaclaw:changes-requested
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
thepastaclaw
removed
the
pastaclaw:changes-requested
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Preliminary review — Phase 1 blocker gate
Verified the findings against exact head b574c8a: two blocking dash-chainstate defects and two non-blocking Dash-specific migration gaps remain; both prior wallet constant errors are fixed. Current CI reports linux64-build / Build source failing, other source builds pending, and linux64-test skipped; the failed run's logs are not yet available, so the failure's specific cause is unconfirmed. Validation was static only.
Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.
🔴 2 blocking | 🟡 2 suggestion(s)
4 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Duplicate chainparams definition breaks build
src/bitcoin-chainstate.cpp:119-120
main() already defines auto chainparams = CChainParams::Main(); at line 70. This second definition is in the same function scope, making the translation unit ill-formed and preventing dash-chainstate from compiling. Remove this declaration and reuse the existing object.
// SETUP: Chainstate
source: muse-spark-1.3-contributor (phase1-reviewer: general)
🔴 Blocking: dash-chainstate ignores DATADIR argument, uses live datadir
src/bitcoin-chainstate.cpp:121-129
The PR removes the ForceSetArg("-datadir", abs_datadir.string()) call, but the chainstate and block manager still obtain their paths from gArgs. EvoDB, LLMQ storage, and ChainstateLoadOptions::data_dir also continue using gArgs.GetDataDirNet(). Consequently, the supplied DATADIR is created and printed but does not select the databases: with default arguments, this experimental tool instead accesses the default ~/.dashcore storage. Pass abs_datadir to every database/load option and abs_datadir / "blocks" to the block manager; changing only the selected lines would leave the Dash databases pointed elsewhere.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
🟡 Suggestion: Snapshot cleanup uses global datadir while detection uses options datadir
src/validation.cpp:6214-6219
DeleteSnapshotChainstateFromDisk() now explicitly supplies the process-global datadir, whereas snapshot activation cleanup and detection use m_options.datadir. The Dash-specific reindex path in node::LoadChainstate() calls this function without passing its configured directory. When an embedded manager's directory differs from gArgs, reindex can delete a snapshot in the global directory while leaving the intended snapshot untouched, after which detection still finds it. Thread the configured manager/load datadir into the deletion helper and its caller so deletion and detection operate on the same storage.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
🟡 Suggestion: ChainLock conflict path bypasses kernel notifications
src/validation.cpp:4339-4342
The Dash-specific MarkConflictingBlock() path still sends active-chain tip changes directly to uiInterface, while this PR migrates the corresponding activation and invalidation paths to kernel::Notifications. A custom notifier therefore misses the tip change caused by a ChainLock conflict. Route this path through m_chainman.GetNotifications().blockTip() as well; the production node::KernelNotifications adapter already forwards that callback to uiInterface, preserving the existing UI behavior.
m_chainman.GetNotifications().blockTip(GetSynchronizationState(IsInitialBlockDownload()), *pindex->pprev);
source: muse-spark-1.3-contributor (phase1-reviewer: general)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-gate-verifier, role: verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large, cross-cutting kernel dependency and interface refactor, but the listed changes do not clearly alter consensus rules, funds handling, cryptography, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— verifier; agentsol-gate-verifier - Phase 2 reviewers: not run (deferred by blocker gate)
🤖 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`:
- [BLOCKING] src/bitcoin-chainstate.cpp:119-120: Duplicate `chainparams` definition breaks build
main() already defines `auto chainparams = CChainParams::Main();` at line 70. This second definition is in the same function scope, making the translation unit ill-formed and preventing dash-chainstate from compiling. Remove this declaration and reuse the existing object.
- [BLOCKING] src/bitcoin-chainstate.cpp:121-129: dash-chainstate ignores DATADIR argument, uses live datadir
The PR removes the `ForceSetArg("-datadir", abs_datadir.string())` call, but the chainstate and block manager still obtain their paths from `gArgs`. EvoDB, LLMQ storage, and `ChainstateLoadOptions::data_dir` also continue using `gArgs.GetDataDirNet()`. Consequently, the supplied DATADIR is created and printed but does not select the databases: with default arguments, this experimental tool instead accesses the default ~/.dashcore storage. Pass `abs_datadir` to every database/load option and `abs_datadir / "blocks"` to the block manager; changing only the selected lines would leave the Dash databases pointed elsewhere.
In `src/validation.cpp`:
- [SUGGESTION] src/validation.cpp:6214-6219: Snapshot cleanup uses global datadir while detection uses options datadir
`DeleteSnapshotChainstateFromDisk()` now explicitly supplies the process-global datadir, whereas snapshot activation cleanup and detection use `m_options.datadir`. The Dash-specific reindex path in `node::LoadChainstate()` calls this function without passing its configured directory. When an embedded manager's directory differs from `gArgs`, reindex can delete a snapshot in the global directory while leaving the intended snapshot untouched, after which detection still finds it. Thread the configured manager/load datadir into the deletion helper and its caller so deletion and detection operate on the same storage.
- [SUGGESTION] src/validation.cpp:4339-4342: ChainLock conflict path bypasses kernel notifications
The Dash-specific `MarkConflictingBlock()` path still sends active-chain tip changes directly to `uiInterface`, while this PR migrates the corresponding activation and invalidation paths to `kernel::Notifications`. A custom notifier therefore misses the tip change caused by a ChainLock conflict. Route this path through `m_chainman.GetNotifications().blockTip()` as well; the production `node::KernelNotifications` adapter already forwards that callback to `uiInterface`, preserving the existing UI behavior.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
thepastaclaw
removed
the
pastaclaw:changes-requested
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified one blocking kernel-link failure and one commit-boundary suggestion at adaa64f. Four prior findings are fixed; the two removed-constant findings are withdrawn because those callers were already migrated at the previously reviewed head. Static source inspection and the exact-head linux64_nowallet-build failure confirm the blocker; regular Linux tests and several other builds passed, while multiprocess/ASan builds and SQLite/TSan tests remain pending; no local builds or tests were run.
🔴 1 blocking | 🟡 1 suggestion(s)
2 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Complete kernel decoupling before removing its argument/system implementations
src/Makefile.am:1295-1296
The kernel loses the argument/system implementations but retains their consumers. Upstream bitcoin#27576 removes chainparams.cpp and chainparamsbase.cpp from the kernel and removes SelectParams() from the chainstate tool; Dash retains all three. Additional kernel consumers include evo/netinfo.cpp, which uses gArgs, and bls/bls_worker.cpp:68, which calls RenameThreadPool(). Their implementations now reside in common/args.cpp and common/system.cpp, neither of which is in the kernel dependency graph; dash-chainstate links only the kernel and its cryptographic dependencies. The exact-head linux64_nowallet-build log confirms undefined references to ArgsManager methods, gArgs, and RenameThreadPool before dash-chainstate fails to link: https://github.com/dashpay/dash/actions/runs/41639682763/job/125930410579. Complete the upstream removals and decouple the surviving Dash consumers before dropping their implementations. Removing only these two source-list entries is insufficient because the tool and Dash-specific consumers still depend on them.
source: gpt-6.1-sol (phase2-reviewer: general, backport-reviewer, dash-core-commit-history)
🟡 Suggestion: Fold migration repairs into the ChainType backport they amend
<commit:6034aecdc2a>:1
Commit a1feef5 introduces the ChainType migration but leaves an undeclared chainName reference in pow_tests.cpp, callers of the removed CBaseChainParams constants, and an enum passed to the string-keyed configuration lookup in ForceRemoveArg(). Commit 6034aec repairs these alongside its genuine bitcoin#27611 exhaustive-enum changes. Move those corrective hunks into a1feef5 so the migration commit is buildable and the series remains useful for bisecting. Keep the actual bitcoin#27611 changes as their own backport; squashing the upstream series is unnecessary.
source: gpt-6.1-sol (phase2-reviewer: dash-core-commit-history)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large, cross-cutting kernel dependency and interface refactor, but the changes shown do not establish modifications to consensus rules, funds handling, cryptography, peer-facing deserialization, or storage migrations that would qualify it as critical. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 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/Makefile.am`:
- [BLOCKING] src/Makefile.am:1295-1296: Complete kernel decoupling before removing its argument/system implementations
The kernel loses the argument/system implementations but retains their consumers. Upstream bitcoin#27576 removes chainparams.cpp and chainparamsbase.cpp from the kernel and removes SelectParams() from the chainstate tool; Dash retains all three. Additional kernel consumers include evo/netinfo.cpp, which uses gArgs, and bls/bls_worker.cpp:68, which calls RenameThreadPool(). Their implementations now reside in common/args.cpp and common/system.cpp, neither of which is in the kernel dependency graph; dash-chainstate links only the kernel and its cryptographic dependencies. The exact-head linux64_nowallet-build log confirms undefined references to ArgsManager methods, gArgs, and RenameThreadPool before dash-chainstate fails to link: https://github.com/dashpay/dash/actions/runs/41639682763/job/125930410579. Complete the upstream removals and decouple the surviving Dash consumers before dropping their implementations. Removing only these two source-list entries is insufficient because the tool and Dash-specific consumers still depend on them.
In `<commit:6034aecdc2a>`:
- [SUGGESTION] <commit:6034aecdc2a>:1: Fold migration repairs into the ChainType backport they amend
Commit a1feef5e14b introduces the ChainType migration but leaves an undeclared chainName reference in pow_tests.cpp, callers of the removed CBaseChainParams constants, and an enum passed to the string-keyed configuration lookup in ForceRemoveArg(). Commit 6034aecdc2a repairs these alongside its genuine bitcoin#27611 exhaustive-enum changes. Move those corrective hunks into a1feef5e14b so the migration commit is buildable and the series remains useful for bisecting. Keep the actual bitcoin#27611 changes as their own backport; squashing the upstream series is unnecessary.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete range at 417d83d and found no actionable in-scope defects: the omitted harness edits fall within declared partial scope or documented feature exclusions, and the previously withdrawn history concerns have not been reintroduced. Current CI shows passing completed builds, lint, and standard, no-wallet, SQLite, ASan, and TSan tests; the multiprocess build remains pending, with no failed code checks reported. Review was static only, the worktree remains unchanged, and the supplied base-drift analysis reports no merge conflicts.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large, cross-cutting refactor of kernel dependencies, configuration, and notification interfaces, but the described changes do not clearly alter consensus rules, funds handling, cryptography, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
CI failed after approval — commit 417d83d
The standing review approved this commit; this follow-up moves the verdict to COMMENT: checks that failed after the review look related to this change.
Failing checks:
Why they look related: The build fails with C++ incomplete-type errors involving llmq::UtilParameters and Boost.Hana tuple construction through evo/deterministicmns.h. The PR changes that header and related quorum headers and utilities, making include-dependency regressions from these refactors a plausible cause rather than an infrastructure failure.
Nothing was re-reviewed: the findings of the standing review are unchanged. The next push gets a full review; a re-run that passes does not restore the approval.
Review provenance
Source: CI-check lane gpt-6.1-sol (agent: triage); no reviewer or verifier lanes ran for this follow-up
- Verdict moved because CI failed on this commit after the review; failure logs read: 1 GitHub Actions job(s)
thepastaclaw
added
pastaclaw:commented
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
thepastaclaw
removed
the
pastaclaw:commented
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified head 734acc8: one blocking regression remains in the IWYU application step, confirmed by the failed multiprocess CI log. The earlier fixture-migration defects are fixed, and the settled withdrawals remain applicable. Current CI passes the regular, SQLite, no-wallet, ASan, and TSan tests and the other platform builds, but the multiprocess build fails and its downstream tests are skipped; this verification was static, and the supplied base-drift evidence reports no merge conflicts.
🔴 1 blocking
1 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Exclude generated-file records before applying IWYU fixes to the checkout
ci/dash/lint-tidy.sh:112-113
IWYU analyzes the build tree and reports changes for generated headers as well as tracked sources. After changing to the source checkout, fix_includes.py tries to open those generated paths there, where they do not exist. src/Makefile.am lists ipc/capnp/echo.capnp.h among the generated multiprocess outputs, not the distributed sources. The exact-head linux64_multiprocess-build job fails at this call with FileNotFoundError: [Errno 2] No such file or directory: 'ipc/capnp/echo.capnp.h', followed by exit code 1: https://github.com/dashpay/dash/actions/runs/41991787799/job/127126098647. Because the script uses set -e, this aborts artifact bundling and causes the downstream multiprocess tests to be skipped. Restrict the fixes to tracked checkout files, or handle generated-file records separately against the build tree, while retaining checkout edits for tracked sources.
source: gpt-6.1-sol (phase2-reviewer: general)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large, cross-cutting kernel dependency and interface refactor, but the described changes do not establish modifications to consensus rules, funds handling, cryptography, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 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 `ci/dash/lint-tidy.sh`:
- [BLOCKING] ci/dash/lint-tidy.sh:112-113: Exclude generated-file records before applying IWYU fixes to the checkout
IWYU analyzes the build tree and reports changes for generated headers as well as tracked sources. After changing to the source checkout, fix_includes.py tries to open those generated paths there, where they do not exist. src/Makefile.am lists ipc/capnp/echo.capnp.h among the generated multiprocess outputs, not the distributed sources. The exact-head linux64_multiprocess-build job fails at this call with `FileNotFoundError: [Errno 2] No such file or directory: 'ipc/capnp/echo.capnp.h'`, followed by exit code 1: https://github.com/dashpay/dash/actions/runs/41991787799/job/127126098647. Because the script uses `set -e`, this aborts artifact bundling and causes the downstream multiprocess tests to be skipped. Restrict the fixes to tracked checkout files, or handle generated-file records separately against the build tree, while retaining checkout edits for tracked sources.
bb7d0ff ci: lint multiprocess artifacts in a separate job (PastaClaw) f33c514 ci: run tidy after unit tests (Konstantin Akimov) 37241d4 refactor: define CQuorum destructor out of line (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Restore multiprocess CI on `develop` after #8010 expanded IWYU coverage to all source files (the backport of [Bitcoin Core bitcoin#27571](bitcoin#27571), upstream commit [`ddddf4957b02c83ed9b6c46b35d8ae1e137889d2`](bitcoin@ddddf49)). This is a lint/build-order regression, not an IPC runtime failure. There are two independent failures: 1. [Current develop's multiprocess job](https://github.com/dashpay/dash/actions/runs/41883763194/job/126770982902) builds successfully, then IWYU fails on `llmq/observer.cpp`: `std::default_delete<llmq::CFinalCommitment>` instantiates `sizeof` on an incomplete type through the inline `CQuorum` destructor. `quorums.h` only has a forward declaration of the commitment type. 2. Fixing that alone reveals a second failure: `lint-tidy.sh` runs `fix_includes.py --nosafe_headers`, modifying the distribution build tree to display IWYU suggestions. The subsequent `make check` rebuilds those unreviewed changes and fails on removed includes (for example, `tfm::formatValue`, RELIC's `md_map_sh256`, and chrono literals). [The destructor-only run on #8028](https://github.com/dashpay/dash/actions/runs/41905361595/job/126840014004) passed lint and failed in `Run unit tests` with exactly these errors. Requested by knst via PastaClaw. Source: the two proposed repairs on [knst's kernel-backport PR #8028](#8028). ## What was done? - Apply the exact source diff from [`e071b31927b77f187626493271360853f864326b`](e071b31): declare `~CQuorum()` in the header and default it in `quorums.cpp`, where `llmq/commitment.h` is already included. No extra header dependencies or quorum behavior changes. - Start with the exact workflow diff from [`417d83d5f5ed03cf07894e5d3069361eba9a31f7`](417d83d): run unit tests before tidy/IWYU rewrites the sources, matching the [test-before-tidy ordering in upstream's script at bitcoin#27571](https://github.com/bitcoin/bitcoin/blob/ddddf4957b02c83ed9b6c46b35d8ae1e137889d2/ci/test/06_script_b.sh). - Per [knst's review request](#8056 (review)), move tidy/IWYU and its ctcache handling into an independent `linux64_multiprocess-lint` job. It consumes the completed build artifact and restores the matching depends prefix using the existing cache/artifact fallback. Multiprocess functional tests depend on the build, **not** this slow lint job, so they can start concurrently. Cache pruning also waits for the new ctcache producer. - Explicitly validate that the multiprocess bundle contains `compile_commands.json` and retain its generated Qt sources for consumers of that database. Objects, libraries, and unit-test binaries remain excluded; other build targets retain their previous exclusions. IWYU remains enabled for all files and compiler errors remain fatal. The original two suggested hashes are Dash commits, not Bitcoin Core commits. Their diffs apply unchanged to current `develop`; none of the unrelated kernel backports on their parent branch are prerequisites. The workflow fix is subsequently extended by the requested independent lint job. The additional review-suggested hash `54877e2bfdf88afb3c40df1d585b6f12e508f903` could not be resolved in dashpay/dash, knst/dash, or bitcoin/bitcoin (all return “No commit found”); no uninspected patch from it is assumed necessary. This PR separates the shared CI repair from #8028. Neither proposed commit by itself fixes both failures. No upstream tests or hunks are omitted. ## How Has This Been Tested? - Reproduced IWYU's actual `llmq/observer.cpp` incomplete-type failure with the original header, using captured project compile flags; the same invocation with the fixed header exits successfully with no compiler errors. Environment: ARM64 Debian 13 container, Clang 19.1.7, IWYU 0.23, GCC 14 standard library, debug/multiprocess configuration and the multiprocess depends artifacts from current develop CI. - `git diff --check`, `bash -n ci/dash/bundle-artifacts.sh`, and ShellCheck on the changed shell script pass. YAML/dependency checks verify unit tests precede artifact upload, the new lint job consumes the completed build, functional tests have no dependency on the lint job, and pruning waits for both cache producers. - Artifact round-trip checks pass for multiprocess and normal Linux targets: compilation database, generated IPC/RELIC/Qt inputs, and runtime binaries survive; objects, libraries, dependency directories and test binaries do not. Generated Qt resource sources are retained only for multiprocess. Packaging rejects a missing compilation database. - `actionlint` passes with narrowly scoped ignores for the repository's existing ShellCheck warning classes and the local action schema's missing `allow-unsafe-pr-checkout` input (also used by the new checkout). No other workflow diagnostics. - Full local **non-GUI debug multiprocess build passed** (`bear --append -- make -j6`, in CI's distribution-tree layout). **`make -j6 check VERBOSE=1` passed**, including Dash, wallet, secp256k1, minisketch, UniValue, helper tests and benchmark sanity checks. - **Functional smoke tests passed against `dash-node`**: `DASHD="$PWD/src/dash-node" test/functional/test_runner.py -j2 --timeout-factor=4 --v2transport rpc_misc.py feature_shutdown.py feature_init.py`. All three passed, including `rpc_misc.py`'s spawned-process `echoipc` check; the runner's 21 Python unit tests also passed. - The exact CI image could not be pulled locally because GHCR returned `unauthorized`, so local validation used an isolated Debian container and restored depends artifacts (with local prefix/path adaptation). The enabled CI matrix supplies the additional build/test coverage. - [Initial fork push CI](https://github.com/thepastaclaw/dash/actions/runs/41985271623/job/127104987934) **passed the full multiprocess build, unit tests, and tidy/IWYU** with the corrected ordering. The [upstream PR run](https://github.com/dashpay/dash/actions/runs/41985280323/job/127105046673) passed build and lint but failed unit-test rebuilding under the **old base workflow**. - **[Updated fork push CI is fully green](https://github.com/thepastaclaw/dash/actions/runs/41994815733) for head `bb7d0fffae1422586d12c3de0c1785761111c165`**: [multiprocess build/unit tests](https://github.com/thepastaclaw/dash/actions/runs/41994815733/job/127137494180), [standalone tidy/IWYU with restored artifacts](https://github.com/thepastaclaw/dash/actions/runs/41994815733/job/127148049769), and [the full multiprocess functional-test job](https://github.com/thepastaclaw/dash/actions/runs/41994815733/job/127148050845) all passed. Lint and functional tests both started at 08:59:04 UTC; functional tests finished at 09:25:33 while lint was still running, validating the requested dependency separation. All other enabled fork jobs also passed. - The [latest upstream PR run](https://github.com/dashpay/dash/actions/runs/41994821999/job/127137539666) again passed build and lint but failed the post-IWYU unit-test rebuild under the base workflow; this is the CI caveat below, not a failure of the independently validated head workflow. **CI caveat:** CI uses `pull_request_target` and local reusable workflows from the base branch, so this PR's workflow reordering does not take effect in its own upstream PR run. The existing #8028 run also shows the old step ordering despite its head containing `417d83d5`. Any resulting post-IWYU `make check` rebuild failure must be distinguished from validation of the corrected head workflow. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation (not applicable: internal CI repair) - [x] I have assigned this pull request to a milestone 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- pastaclaw-job: cj-20261105-070003-7003 --> ACKs for top commit: knst: tACK bb7d0ff Tree-SHA512: 2936b36ea9edcb60518e8f0925e1dc5dc6cf349a4456aa5cce6b77596531e14f5f41103ccf8cd3a26c82c538a3ffbaaf358fbdd0c796956e2bee67587468ab72
|
This pull request has conflicts, please rebase. |
be55f54 move-only: Extract common/args and common/config.cpp from util/system (TheCharlatan) Pull request description: This pull request is part of the `libbitcoinkernel` project bitcoin#24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is bitcoin#27254. The pull request contains an extraction of ArgsManager related functions from util/system into their own common/ file. The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager. The ArgsManager belongs into the common library, since the kernel library should not depend on it. See [doc/design/libraries.md](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) for more information on this rationale. ACKs for top commit: MarcoFalke: re-ACK be55f54 🚲 ryanofsky: Code review ACK be55f54. Just small cleanups since the last review. hebasto: ACK be55f54, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 90eb03334af0155b823030b4f2ecf286d35058d700ee2ddbbaa445be19e31eb0fe982656f35bd14ecee3ad2c3d0db3746855cb8f3777eff7253713e42873e111 Co-authored-by: fanquake <fanquake@gmail.com>
78c2707 Refactor: Replace 'isMockableChain' with inline 'ChainType' check for 'submitpackage' (Tim Neubauer) 27b4084 Refactor: Remove m_is_test_chain (Tim Neubauer) Pull request description: Remove the m_is_test_chain bool Compiled and run tests locally bitcoin#28376 ACKs for top commit: MarcoFalke: re-ACK 78c2707 ajtowns: ACK 78c2707 Tree-SHA512: 2eedd855c379dd12b7ff28b0e03414680cc892313f16502f36e09906513df9c222e8cc2cea3ff4d9a4f47c9efdfa00d017f38398021b0c96d4543711206d6ff8 Co-authored-by: fanquake <fanquake@gmail.com>
d168458 scripted-diff: Remove unused chainparamsbase includes (TheCharlatan) e9ee8aa Add missing definitions in prep for scripted diff (TheCharlatan) ba8fc7d refactor: Replace string chain name constants with ChainTypes (TheCharlatan) 401453d refactor: Introduce ChainType getters for ArgsManager (TheCharlatan) bfc21c3 refactor: Create chaintype files (TheCharlatan) Pull request description: This pull request is part of the `libbitcoinkernel` project bitcoin#24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is also a follow up to bitcoin#26177. It replaces pull request bitcoin#27294, which just moved the constants to a new file, but did not re-declare them as enums. The code move of the chain name constants out of the `chainparamsbase` to their own separate header allows the kernel `chainparams` to no longer include `chainparamsbase`. The `chainparamsbase` contain references to the `ArgsManager` and networking related options that should not belong to the kernel library. Besides this move, the constants are re-declared as enums with helper functions facilitating string conversions. ACKs for top commit: ryanofsky: Code review ACK d168458. Just suggested changes since last review. Tree-SHA512: ac2fbe5cbbab4f52eae1e30af1f16700b6589eb4764c328a151a712adfc37f326cc94a65c385534c57d4bc92cc1a13bf1777d92bc924a20dbb30440e7380b316 Co-authored-by: fanquake <fanquake@gmail.com>
e230887 refactor: Use ChainType enum exhaustively (TheCharlatan) Pull request description: This is a follow up of bitcoin#27491, more concretely bitcoin#27491 (comment), for not using default cases (as per the style guide), and bitcoin#27491 (comment) and bitcoin#27491 (comment) for avoiding dead code. ACKs for top commit: fanquake: ACK e230887 - deals with almost all follow up comments out of bitcoin#27491. Tree-SHA512: 1794190b03b91d3ca349a4da08e9610dbb3432983eee7cb21ecc758d1d7d710560c97661de14cdf493c28c00ebe8977511b4696055c0940e7f815b622dbacd16 Co-authored-by: fanquake <fanquake@gmail.com>
…kstorage 5ff63a0 refactor, blockstorage: Replace stopafterblockimport arg (TheCharlatan) 18e5ba7 refactor, blockstorage: Replace blocksdir arg (TheCharlatan) 02a0899 refactor, BlockManager: Replace fastprune from arg with options (TheCharlatan) a498d69 refactor/iwyu: Complete includes for blockmanager_args (TheCharlatan) f0bb102 refactor: Move functions to BlockManager methods (TheCharlatan) cfbb212 zmq: Pass lambda to zmq's ZMQPublishRawBlockNotifier (TheCharlatan) 8ed4ff8 refactor: Declare g_zmq_notification_interface as unique_ptr (TheCharlatan) Pull request description: The libbitcoin_kernel library should not rely on the `ArgsManager`, but rather use option structs that can be passed to the various classes it uses. This PR removes reliance on the `ArgsManager` from the `blockstorage.*` files. Like similar prior work, it uses the options struct in the `BlockManager` that can be populated with `ArgsManager` values. Some related prior work: bitcoin#26889 bitcoin#25862 bitcoin#25527 bitcoin#25487 Related PR removing blockstorage globals: bitcoin#25781 ACKs for top commit: ryanofsky: Code review ACK 5ff63a0. Since last ACK just added std::move and fixed commit title. Sorry for the noise! mzumsande: Code Review ACK 5ff63a0 Tree-SHA512: 4bde8fd140a40b97eca923e9016d85dcea6fad6fd199731f158376294af59c3e8b163a0725aa47b4be3519b61828044e0a042deea005e0c28de21d8b6c3e1ea7 Co-authored-by: fanquake <fanquake@gmail.com>
…ibrary, interface_ui from validation. BACKPORT NOTE: util/system is not actually yet removed from kernel because it is still used in dash specific code 7d3b350 refactor: Move system from util to common library (TheCharlatan) 7eee356 refactor: Split util::AnyPtr into its own file (TheCharlatan) 44de325 refactor: Split util::insert into its own file (TheCharlatan) 9ec5da3 refactor: Move ScheduleBatchPriority to its own file (TheCharlatan) f871c69 kernel: Add warning method to notifications (TheCharlatan) 4452707 kernel: Add progress method to notifications (TheCharlatan) 84d7145 kernel: Add headerTip method to notifications (TheCharlatan) 447761c kernel: Add notification interface (TheCharlatan) Pull request description: This pull request is part of the `libbitcoinkernel` project bitcoin#27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". --- It removes the kernel library's dependency on `util/system` and `interface_ui`. `util/system` contains networking and shell-related code that should not be part of the kernel library. The following pull requests prepared `util/system` for this final step: bitcoin#27419 bitcoin#27254 bitcoin#27238. `interface_ui` defines functions for a more general node interface and has a dependency on `boost/signals2`. After applying the patches from this pull request, the kernel's reliance on boost is down to `boost::multiindex`. The approach implemented here introduces some indirection, which makes the code a bit harder to read. Any suggestions for improving or reworking this pull request to make it more concise, or even reworking it into a more proper interface, are appreciated. ACKs for top commit: MarcoFalke: re-ACK 7d3b350 (no change) 🎋 stickies-v: Code Review ACK 7d3b350 hebasto: re-ACK 7d3b350, only last two commits dropped since my [recent](bitcoin#27636 (review)) review. Tree-SHA512: c8cfc698dc9d78e20191c444708f2d957501229abe95e5806106d1126fb9c5fbcee686fb55645658c0107ce71f10646f37a2fdf7fde16bbf22cbf1ac885dd08d Co-authored-by: fanquake <fanquake@gmail.com>
…ms, chainparamsbase from kernel library BACKPORT NOTE keep common/args.cpp, config.cpp, settings.cpp in the kernel as they still used by dash specific code ---- db77f87 scripted-diff: move settings to common namespace (TheCharlatan) c27e4bd move-only: Move settings to the common library (TheCharlatan) c2dae5d kernel: Remove chainparams, chainparamsbase, args, settings from kernel library (TheCharlatan) 05870b1 refactor: Remove gArgs access from validation.cpp (TheCharlatan) 8789b11 refactor: Add path argument to FindSnapshotChainstateDir (TheCharlatan) ef95be3 refactor: Add stop_at_height option in ChainstateManager (TheCharlatan) Pull request description: This pull request is part of the `libbitcoinkernel` project bitcoin#27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". --- This completes the removal of the node's chainparams, chainparamsbase, args and settings files and their respective classes from the kernel library. This is the last pull request in a long series working towards decoupling the `ArgsManager` and the `gArgs` global from kernel code. These prior pull requests are: bitcoin#26177 bitcoin#27125 bitcoin#25527 bitcoin#25487 bitcoin#25290 ACKs for top commit: MarcoFalke: lgtm ACK db77f87 🍄 hebasto: ACK db77f87, I have reviewed the code and it looks OK. ryanofsky: Code review ACK db77f87. Looks great! Tree-SHA512: cbfbd705d056f2f10f16810d4f869eb152362fff2c5ddae5e1ac6785deae095588e52ad48b29d921962b085e51de1e0ecab6e50f46149ffe3c16250608a2c93a Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
thepastaclaw
removed
the
pastaclaw:changes-requested
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of 47f85a4 found no remaining actionable in-scope findings. The Dash-specific storage callers, configured directories, ChainType consumers, and notification routing address the previously identified correctness issues; the renewed omission-accounting and commit-boundary comments repeat settled findings without a demonstrated reintroduction. Current GitHub checks show successful completed builds, Linux test jobs, and linters, with multiprocess tests and multiprocess lint still pending; no local builds or tests were run.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large, cross-cutting kernel dependency refactor, but the listed changes do not clearly alter consensus rules, funds handling, cryptography, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
thepastaclaw
removed
the
pastaclaw:approved
|
I checked the running backportsys and performed its supported map refresh plus normal sheet import/planning (no version-scope change, skip reset, manual database update or deployment). Verified on current Dash
Map run 313 completed at 20:32:37 UTC. Before → after counts are unchanged: blocked 142→142, planned 11→11, batched 15→15, done 834→834, skipped 194→194 (irrelevant 48; upstream open 0/4). The refresh produced no new batch or upstream PR. The three predicted follow-ons are not three new available batches: bitcoin#28952 and bitcoin#29301 are already queued as top-ups in existing batch 201 (fork PR #128); neither is landed there yet. bitcoin#28721 is planned, but is the only item in the enabled 0.27 ready pool, below the four-item batch minimum. Quota headroom is also currently closed, so I have not forced execution or duplicated their ownership. There is a real reconciliation bug: the mapper only visits enabled-version nonterminal candidates, leaving verified merged 0.26 prerequisites recorded as skipped/blocked and preserving stale dependency refusals. I have a fix under local regression/full-suite testing; I will link the PR here when validation finishes. It recognizes verified develop coverage without reopening unmerged human skips or enabling another version, and re-probes affected enabled goals instead of forcing them ready. That code is not deployed; I am keeping operational results separate from the proposed fix. |
|
Follow-through: the reconciliation fix is ready in backport-system #46. Both CI runs are green; final-head CI passed 698 tests, formatting/lint and type checks. Local package builds and the focused safety regressions also pass. The fix recognizes already-merged prerequisites across stored versions before selecting map work, requires actual upstream-develop ancestry (not a fork-only marker), preserves prior decisions in an audit event, and re-probes affected enabled goals even on an unchanged base. It does not reopen unmerged human skips, enable v0.26 work, alter live batch ownership, or relax batch/CI/conflict/budget/quota gates. Actual deployed outcome, checked again at 21:12 UTC:
The supported refresh completed (map 313, 20:32:37 UTC), but the deployed reconciliation defect prevents correcting those prerequisite records. No new candidate became eligible because of this refresh, and no new batch/upstream PR was generated. Upstream occupancy is 0/4; the limiting factors are not that cap:
All three requested prerequisites are genuinely merged, as detailed in the verification above. The fix is not deployed: this job explicitly excludes deployments. The remaining step is review/merge and an authorized release deployment of #46, followed by the normal map/planner to measure any additional eligibility. I have kept that outstanding step explicit rather than reporting a proposed fix or queued items as completed backports. |
What was done?
Kernel related backports from Bitcoin Core v26. All of them are refactors that touches noticeable amount of files and will make multiple other backports conflict free.
How Has This Been Tested?
Run unit & functional tests.
Breaking Changes
N/A
Checklist: