backport: Merge bitcoin#25077, 26533#7431
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (43)
🚧 Files skipped from review as they are similar to previous changes (39)
WalkthroughThis PR adds startup cleanup for already-pruned block and undo files, wires the cleanup into block-index loading, and adds unit and functional coverage. It also introduces Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
thepastaclaw
force-pushed
the
takeover-7190-backport-26533-27302
branch
from
3ad62bb to
aaa04a9
Compare
thepastaclaw
changed the title
|
Updated this draft to bitcoin#26533 only. Pasta spotted the bitcoin#27302 weirdness correctly: upstream bitcoin#27302 expects bitcoin#27419's common/args split, so bitcoin#27302 should wait until bitcoin#27419 is backported rather than being adapted across Dash's older src/util/system layout. |
|
✅ Review complete (commit 98bba2d) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: claude/opus general, dash-core-commit-history, backport-reviewer failed quota; codex/gpt-5.5 general, dash-core-commit-history, backport-reviewer completed. Verifier: codex/gpt-5.5 completed after claude/opus verifier failed quota.
Policy gate: an agent-reported missing upstream prerequisite was restored as a blocking finding. This PR is a full Bitcoin backport, and omitted upstream hunks require their prerequisite PRs unless the finding is explicitly allowlisted (intentional_exclusion / policy_override). The agent's original evidence is preserved in the finding(s) below.
Prior verifier summary (overridden by policy gate): Verified the worktree is at aaa04a9 and checked the only agent finding against the local source and history. The bitcoin#25077 GetMutex() point is a harmless test-only adaptation: upstream's helper is just an alias for ::cs_main, and this Dash backport locks cs_main directly at the same sites. No in-scope review findings remain.
Note: GitHub does not allow me to submit REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the policy-enforced blocking finding.
🔴 1 blocking
🤖 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/test/blockmanager_tests.cpp`:
- [BLOCKING] src/test/blockmanager_tests.cpp:53-60: Missing prerequisite: bitcoin#25077
Upstream bitcoin#26533 adds this test using `WITH_LOCK(chainman->GetMutex(), ...)` and `LOCK(chainman->GetMutex())`. That API was introduced by bitcoin#25077, merge 7087b1f17ca, commit fa530bcb9c1 (`Add ChainstateManager::GetMutex(), an alias for ::cs_main`). Dash's base branch has no `ChainstateManager::GetMutex()` (`git grep GetMutex upstream/develop` is empty), so this cherry-pick rewrites the new test to lock `cs_main` directly. The adaptation is equivalent to Dash's current locking shape and is test-only, so this is non-blocking per the agent (restored to blocking by policy), but it is still a soft missing upstream prerequisite by diff-on-diff completeness.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
aaa04a9 to
053b524
Compare
thepastaclaw
changed the title
![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: 053b524a8d
ℹ️ 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".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Note: GitHub does not allow me to submit REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the verified blocking findings.
Policy gate: an agent-reported missing upstream prerequisite was restored as a blocking finding. This PR is a full Bitcoin backport, and omitted upstream hunks require their prerequisite PRs unless the finding is explicitly allowlisted (intentional_exclusion / policy_override). The agent's original evidence is preserved in the finding(s) below.
Prior verifier summary (overridden by policy gate): Source: reviewers: claude/opus general, dash-core-commit-history, backport-reviewer failed quota; codex/gpt-5.5 general, dash-core-commit-history, backport-reviewer completed. Verifier: claude/opus failed quota; codex/gpt-5.5 completed.
Prior finding reconciliation: prior-1 is FIXED. Current head 053b524 includes commit 3878459 for bitcoin#25077, ChainstateManager::GetMutex() is present in src/validation.h, and src/test/blockmanager_tests.cpp now uses chainman.GetMutex() at the previously flagged blockmanager test sites.
Carried-forward prior findings: None.
New findings in latest delta aaa04a9..053b524: Two verified omitted bitcoin#25077 hunks remain: src/bitcoin-chainstate.cpp and src/wallet/test/availablecoins_tests.cpp still read chain-tip state without the new chainstate mutex.
Additional cumulative current-head findings: None beyond the two verified omitted-hunk findings. The two codex-general wallet-test findings were not carried forward because they describe upstream test patterns introduced by bitcoin#25077 rather than Dash-specific merge-resolution defects, and are lower-signal than the concrete omitted hunks.
🔴 2 blocking
2 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/bitcoin-chainstate.cpp`:
- [BLOCKING] src/bitcoin-chainstate.cpp:158-164: Incomplete backport: bitcoin#25077
Upstream bitcoin#25077 changed this datadir-reporting block to take `LOCK(chainman.GetMutex())` before reading `chainman.IsSnapshotActive()`, `chainman.ActiveHeight()`, `chainman.ActiveChainstate().IsInitialBlockDownload()`, and `chainman.ActiveTip()`. Dash still has this built source as `dash-chainstate`, but commit 3878459fa73 omitted that hunk. Current head therefore calls `ActiveHeight()` and `ActiveTip()` without the mutex even though the same backport added `EXCLUSIVE_LOCKS_REQUIRED(GetMutex())` annotations to those accessors.
In `src/wallet/test/availablecoins_tests.cpp`:
- [BLOCKING] src/wallet/test/availablecoins_tests.cpp:45-49: Incomplete test hunk from bitcoin#25077
Upstream bitcoin#25077 added chainstate locking in this helper before reading `m_node.chainman->ActiveChain().Tip()` and `ActiveChain().Height()`. Dash has the same available-coins test, but commit 3878459fa73 did not update it, so current head still performs these chain-tip reads without the new mutex. The CoinJoin constructor differences are valid Dash adaptations, but they do not explain this missing synchronization.
---
**Policy gate (backport-prereq-restore):** For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. `intentional_exclusion: true` or a matching entry in `policy_overrides`). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
053b524 to
f7ec53c
Compare
|
Fixed the two bitcoin#25077 completeness gaps from the automated review and folded them into the existing bitcoin#25077 backport commit.
Local validation: Separate local Codex backport-reviewer gate passed at |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7ec53c31f
ℹ️ 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".
thepastaclaw
force-pushed
the
takeover-7190-backport-26533-27302
branch
from
f7ec53c to
eb077b8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb077b8d23
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
Source: reviewers: codex/gpt-5.5 general, codex/gpt-5.5 dash-core-commit-history, codex/gpt-5.5 backport-reviewer; failed lanes: claude/opus general, claude/opus dash-core-commit-history, claude/opus backport-reviewer; verifier: codex/gpt-5.5.
Policy gate: checked after verification; no current missing-prerequisite finding required restoration. The prior missing-prerequisite findings are resolved at this head.
Carried-forward prior findings from 053b524: prior-053b524a-1 and prior-053b524a-2 are FIXED at current head because the referenced sites now lock chainman.GetMutex() before the chain-tip reads. Unverified connector findings: the interfaces_tests.cpp dangling-reference report was dropped, while the signing_shares.cpp cs_main-over-quorum-scan report was validated and deduplicated across Codex lanes. New findings in latest delta f7ec53c..eb077b8: one in-scope LLMQ lock-scope/performance suggestion; CodeRabbit has no concrete inline findings.
🟡 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/llmq/signing_shares.cpp`:
- [SUGGESTION] src/llmq/signing_shares.cpp:747: Avoid holding cs_main while scanning quorums
The new `WITH_LOCK(::cs_main, ...)` wrapper holds the chainstate mutex for the entire `SelectQuorumForSigning()` call. That callee already has a narrow internal `cs_main` section to read the active-chain height and select `pindexStart`, then calls the `CBlockIndex*` overload of `qman.ScanQuorums()`, which is documented as cs_main-free and can take quorum-manager locks, consult caches, read/write quorum contributions, and build quorum data. In this async signing path, a quorumless signing request can now keep `cs_main` held across that heavier quorum scan/DB work, blocking validation and P2P paths longer than needed. Snapshot the needed chain index or height under `cs_main`, then release it before scanning quorums.
Prior Finding Reconciliation
- Incomplete backport: bitcoin#25077 - FIXED: current
src/bitcoin-chainstate.cpplockschainman.GetMutex()beforeIsSnapshotActive(),ActiveHeight(),ActiveChainstate(), andActiveTip(). - Incomplete test hunk from bitcoin#25077 - FIXED: current
src/wallet/test/availablecoins_tests.cpplocksm_node.chainman->GetMutex()before readingActiveChain().Tip()andActiveChain().Height().
thepastaclaw
force-pushed
the
takeover-7190-backport-26533-27302
branch
from
eb077b8 to
5faf859
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5faf859d82
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/node/interfaces.cpp`:
- Around line 346-347: Combine the two cs_main reads into one lock scope so
`next_sb` and the budget are derived from the same chain state. In
`src/node/interfaces.cpp`, update the logic around
`CSuperblock::GetNearestSuperblocksHeights` and `CSuperblock::GetPaymentsLimit`
so `context().chainman->ActiveHeight()` and `context().chainman->ActiveChain()`
are both accessed under a single `WITH_LOCK(::cs_main, ...)` block, then compute
`next_sb` and `budget` from those captured values to keep them consistent.
In `@src/rpc/rawtransaction.cpp`:
- Around line 765-768: The height validation and the indexed lookup in
rawtransaction.cpp are split across separate cs_main critical sections, creating
a TOCTOU race. Update the logic around the nSpecificCoreHeight check and
chainman.ActiveChain()[nSpecificCoreHeight.value()] access so both happen within
one WITH_LOCK(::cs_main, ...) block, then only call chain_helper.GetCreditPool
on the validated block index from that same atomic section. Ensure the code
still throws RPC_INVALID_PARAMETER for out-of-range heights and never passes a
null block index to GetCreditPool.
🪄 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: bbb65035-a69a-4c37-a77e-022ba82543ea
📒 Files selected for processing (37)
src/bench/duplicate_inputs.cppsrc/bitcoin-chainstate.cppsrc/chainlock/handler.cppsrc/governance/signing.cppsrc/init.cppsrc/llmq/ehf_signals.cppsrc/llmq/signing_shares.cppsrc/net_processing.cppsrc/node/blockstorage.cppsrc/node/blockstorage.hsrc/node/interfaces.cppsrc/qt/test/wallettests.cppsrc/rest.cppsrc/rpc/blockchain.cppsrc/rpc/governance.cppsrc/rpc/quorums.cppsrc/rpc/rawtransaction.cppsrc/test/block_reward_reallocation_tests.cppsrc/test/blockmanager_tests.cppsrc/test/bls_tests.cppsrc/test/evo_cbtx_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/evo_mnhf_tests.cppsrc/test/evo_trivialvalidation.cppsrc/test/evo_utils_tests.cppsrc/test/interfaces_tests.cppsrc/test/miner_tests.cppsrc/test/util/mining.cppsrc/test/validation_block_tests.cppsrc/test/validation_chainstatemanager_tests.cppsrc/validation.cppsrc/validation.hsrc/wallet/test/availablecoins_tests.cppsrc/wallet/test/spend_tests.cppsrc/wallet/test/wallet_tests.cpptest/functional/feature_remove_pruned_files_on_startup.pytest/functional/test_runner.py
✅ Files skipped from review due to trivial changes (1)
- src/validation.cpp
🚧 Files skipped from review as they are similar to previous changes (12)
- src/node/blockstorage.h
- src/bitcoin-chainstate.cpp
- src/test/validation_chainstatemanager_tests.cpp
- src/net_processing.cpp
- src/test/validation_block_tests.cpp
- test/functional/test_runner.py
- src/rest.cpp
- src/node/blockstorage.cpp
- src/test/blockmanager_tests.cpp
- src/qt/test/wallettests.cpp
- src/validation.h
- test/functional/feature_remove_pruned_files_on_startup.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: codex/gpt-5.5 general, codex/gpt-5.5 dash-core-commit-history, codex/gpt-5.5 backport-reviewer; failed lanes: claude/opus general, claude/opus dash-core-commit-history, claude/opus backport-reviewer; verifier: codex/gpt-5.5.
Carried-forward prior findings: prior-eb077b8d-1 is STILL VALID at 5faf859; the current code still holds cs_main around the full quorum-selection call. New findings in latest delta: none; the three latest-delta edits only adjust test/bench ActiveChain access patterns and do not introduce a new issue. CodeRabbit has no concrete inline comment_id findings to validate.
🟡 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/llmq/signing_shares.cpp`:
- [SUGGESTION] src/llmq/signing_shares.cpp:747: Avoid holding cs_main while scanning quorums
The new WITH_LOCK(::cs_main, ...) wrapper keeps the chainstate mutex held for the entire SelectQuorumForSigning() call. That callee already narrows its own cs_main use to reading active_chain.Height() and active_chain[startBlockHeight], then calls the CBlockIndex* overload of qman.ScanQuorums(), which quorumsman.h documents as cs_main-free. ScanQuorums can then take quorum-manager locks, consult caches, look up mined commitments, and build or read quorum contribution data while cs_main is still held by the caller. In the AsyncSignIfMember path where quorumHash is null, this widens a chain-tip data-race fix into avoidable chainstate lock contention during LLMQ quorum scanning. Snapshot the needed active-chain state under cs_main, then release it before scanning quorums.
5faf859 to
f8abaa6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/chainlock/handler.cpp`:
- Around line 105-106: `VerifyChainLock()` is using `active_chain` outside the
protection of `cs_main`, which can race with chain updates. In
`chainlock::Handler`, keep the `ActiveChain()` access and the
`VerifyChainLock()` call under the same `cs_main` lock, or otherwise pass only a
stable snapshot of the chain data needed by `VerifyChainLock()`. Use the
`active_chain` and `VerifyChainLock` symbols to locate the affected flow.
🪄 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: df9fdd7b-1da2-4800-b183-c26e7a058634
📒 Files selected for processing (39)
src/bench/duplicate_inputs.cppsrc/bitcoin-chainstate.cppsrc/chainlock/handler.cppsrc/governance/signing.cppsrc/init.cppsrc/llmq/ehf_signals.cppsrc/llmq/signing_shares.cppsrc/net_processing.cppsrc/node/blockstorage.cppsrc/node/blockstorage.hsrc/node/interfaces.cppsrc/qt/test/wallettests.cppsrc/rest.cppsrc/rpc/blockchain.cppsrc/rpc/governance.cppsrc/rpc/quorums.cppsrc/rpc/rawtransaction.cppsrc/test/block_reward_reallocation_tests.cppsrc/test/blockmanager_tests.cppsrc/test/bls_tests.cppsrc/test/evo_cbtx_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/evo_mnhf_tests.cppsrc/test/evo_trivialvalidation.cppsrc/test/evo_utils_tests.cppsrc/test/interfaces_tests.cppsrc/test/miner_tests.cppsrc/test/util/mining.cppsrc/test/validation_block_tests.cppsrc/test/validation_chainstatemanager_tests.cppsrc/validation.cppsrc/validation.hsrc/wallet/test/availablecoins_tests.cppsrc/wallet/test/spend_tests.cppsrc/wallet/test/util.cppsrc/wallet/test/util.hsrc/wallet/test/wallet_tests.cpptest/functional/feature_remove_pruned_files_on_startup.pytest/functional/test_runner.py
✅ Files skipped from review due to trivial changes (1)
- src/qt/test/wallettests.cpp
🚧 Files skipped from review as they are similar to previous changes (33)
- src/test/evo_mnhf_tests.cpp
- test/functional/test_runner.py
- src/test/evo_cbtx_tests.cpp
- src/test/evo_utils_tests.cpp
- src/bitcoin-chainstate.cpp
- src/test/miner_tests.cpp
- src/wallet/test/util.h
- src/bench/duplicate_inputs.cpp
- src/test/util/mining.cpp
- src/governance/signing.cpp
- src/test/validation_chainstatemanager_tests.cpp
- src/node/blockstorage.h
- src/test/validation_block_tests.cpp
- src/rpc/rawtransaction.cpp
- src/validation.h
- src/node/interfaces.cpp
- src/test/blockmanager_tests.cpp
- src/validation.cpp
- src/rest.cpp
- src/node/blockstorage.cpp
- src/test/block_reward_reallocation_tests.cpp
- src/rpc/governance.cpp
- src/llmq/signing_shares.cpp
- src/net_processing.cpp
- src/rpc/blockchain.cpp
- src/test/bls_tests.cpp
- src/wallet/test/availablecoins_tests.cpp
- test/functional/feature_remove_pruned_files_on_startup.py
- src/test/evo_trivialvalidation.cpp
- src/rpc/quorums.cpp
- src/init.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/wallet/test/wallet_tests.cpp
thepastaclaw
force-pushed
the
takeover-7190-backport-26533-27302
branch
from
993ebb2 to
90ee9ca
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
takeover-7190-backport-26533-27302
branch
from
90ee9ca to
767b7a9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
takeover-7190-backport-26533-27302
branch
from
767b7a9 to
d8e7698
Compare
fac04cb refactor: Add lock annotations to Active* methods (MacroFake) fac15ff Fix logical race in rest_getutxos (MacroFake) fa97a52 Fix UB/data-race in RPCNotifyBlockChange (MacroFake) fa530bc Add ChainstateManager::GetMutex(), an alias for ::cs_main (MacroFake) Pull request description: This fixes two issues: * A data race in `ActiveChain`, which returns a reference to the chain (a `std::vector`), which is not thread safe. See also below traceback. * A corrupt rest response, which returns a blockheight and blockhash, which are unrelated to each other and to the result, as the chain might advance between each call without cs_main held. The issues are fixed by taking cs_main and holding it for the required time. ``` ================== WARNING: ThreadSanitizer: data race (pid=32335) Write of size 8 at 0x7b3c000008f0 by thread T22 (mutexes: write M131626, write M151, write M131553): #0 std::__1::enable_if<(is_move_constructible<CBlockIndex**>::value) && (is_move_assignable<CBlockIndex**>::value), void>::type std::__1::swap<CBlockIndex**>(CBlockIndex**&, CBlockIndex**&) /usr/lib/llvm-13/bin/../include/c++/v1/__utility/swap.h:39:7 (bitcoind+0x501239) #1 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::__swap_out_circular_buffer(std::__1::__split_buffer<CBlockIndex*, std::__1::allocator<CBlockIndex*>&>&) /usr/lib/llvm-13/bin/../include/c++/v1/vector:977:5 (bitcoind+0x501239) #2 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::__append(unsigned long) /usr/lib/llvm-13/bin/../include/c++/v1/vector:1117:9 (bitcoind+0x501239) #3 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::resize(unsigned long) /usr/lib/llvm-13/bin/../include/c++/v1/vector:2046:15 (bitcoind+0x4ffe29) #4 CChain::SetTip(CBlockIndex*) src/chain.cpp:19:12 (bitcoind+0x4ffe29) #5 CChainState::ConnectTip(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) src/validation.cpp:2748:13 (bitcoind+0x475d00) #6 CChainState::ActivateBestChainStep(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, bool&, ConnectTrace&) src/validation.cpp:2884:18 (bitcoind+0x47739e) #7 CChainState::ActivateBestChain(BlockValidationState&, std::__1::shared_ptr<CBlock const>) src/validation.cpp:3011:22 (bitcoind+0x477baf) #8 node::ThreadImport(ChainstateManager&, std::__1::vector<fs::path, std::__1::allocator<fs::path> >, ArgsManager const&) src/node/blockstorage.cpp:883:30 (bitcoind+0x23cd74) #9 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1657:9 (bitcoind+0x15863e) #10 decltype(static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(fp)()) std::__1::__invoke<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x15863e) #11 void std::__1::__invoke_void_return_wrapper<void, true>::__call<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/__functional/invoke.h:61:9 (bitcoind+0x15863e) #12 std::__1::__function::__alloc_func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:171:16 (bitcoind+0x15863e) #13 std::__1::__function::__func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:345:12 (bitcoind+0x15863e) #14 std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x88891f) #15 std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x88891f) #16 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f) #17 decltype(static_cast<void (*>(fp)(static_cast<char const*>(fp0), static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(fp0))) std::__1::__invoke<void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(void (*&&)(char const*, std::__1::function<void ()>), char const*&&, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x157e6a) #18 void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-13/bin/../include/c++/v1/thread:280:5 (bitcoind+0x157e6a) #19 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7> >(void*) /usr/lib/llvm-13/bin/../include/c++/v1/thread:291:5 (bitcoind+0x157e6a) Previous read of size 8 at 0x7b3c000008f0 by main thread: #0 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::size() const /usr/lib/llvm-13/bin/../include/c++/v1/vector:680:61 (bitcoind+0x15179d) #1 CChain::Tip() const src/./chain.h:449:23 (bitcoind+0x15179d) #2 ChainstateManager::ActiveTip() const src/./validation.h:927:59 (bitcoind+0x15179d) #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1841:35 (bitcoind+0x15179d) #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) Location is heap block of size 232 at 0x7b3c00000870 allocated by main thread: #0 operator new(unsigned long) <null> (bitcoind+0x132668) #1 ChainstateManager::InitializeChainstate(CTxMemPool*, std::__1::optional<uint256> const&) src/validation.cpp:4851:21 (bitcoind+0x48e26b) #2 node::LoadChainstate(bool, ChainstateManager&, CTxMemPool*, bool, Consensus::Params const&, bool, long, long, long, bool, bool, std::__1::function<bool ()>, std::__1::function<void ()>) src/node/chainstate.cpp:31:14 (bitcoind+0x24de07) #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1438:32 (bitcoind+0x14e994) #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) Mutex M131626 (0x7b3c00000898) created at: #0 pthread_mutex_lock <null> (bitcoind+0xda898) #1 std::__1::mutex::lock() <null> (libc++.so.1+0x49f35) #2 node::ThreadImport(ChainstateManager&, std::__1::vector<fs::path, std::__1::allocator<fs::path> >, ArgsManager const&) src/node/blockstorage.cpp:883:30 (bitcoind+0x23cd74) #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1657:9 (bitcoind+0x15863e) #4 decltype(static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(fp)()) std::__1::__invoke<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x15863e) #5 void std::__1::__invoke_void_return_wrapper<void, true>::__call<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/__functional/invoke.h:61:9 (bitcoind+0x15863e) #6 std::__1::__function::__alloc_func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:171:16 (bitcoind+0x15863e) #7 std::__1::__function::__func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:345:12 (bitcoind+0x15863e) #8 std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x88891f) #9 std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x88891f) #10 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f) #11 decltype(static_cast<void (*>(fp)(static_cast<char const*>(fp0), static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(fp0))) std::__1::__invoke<void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(void (*&&)(char const*, std::__1::function<void ()>), char const*&&, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x157e6a) #12 void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-13/bin/../include/c++/v1/thread:280:5 (bitcoind+0x157e6a) #13 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7> >(void*) /usr/lib/llvm-13/bin/../include/c++/v1/thread:291:5 (bitcoind+0x157e6a) Mutex M151 (0x55aacb8ea030) created at: #0 pthread_mutex_init <null> (bitcoind+0xbed2f) #1 std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3) #2 __libc_start_main <null> (libc.so.6+0x29eba) Mutex M131553 (0x7b4c000042e0) created at: #0 pthread_mutex_init <null> (bitcoind+0xbed2f) #1 std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3) #2 std::__1::__unique_if<CTxMemPool>::__unique_single std::__1::make_unique<CTxMemPool, CBlockPolicyEstimator*, int const&>(CBlockPolicyEstimator*&&, int const&) /usr/lib/llvm-13/bin/../include/c++/v1/__memory/unique_ptr.h:728:32 (bitcoind+0x15c81d) #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1426:24 (bitcoind+0x14e7b4) #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) Thread T22 'b-loadblk' (tid=32370, running) created by main thread at: #0 pthread_create <null> (bitcoind+0xbd5bd) #1 std::__1::__libcpp_thread_create(unsigned long*, void* (*)(void*), void*) /usr/lib/llvm-13/bin/../include/c++/v1/__threading_support:443:10 (bitcoind+0x155e06) #2 std::__1::thread::thread<void (*)(char const*, std::__1::function<void ()>), char const (&) [8], AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, void>(void (*&&)(char const*, std::__1::function<void ()>), char const (&) [8], AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/thread:307:16 (bitcoind+0x155e06) #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1656:29 (bitcoind+0x150164) #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) SUMMARY: ThreadSanitizer: data race /usr/lib/llvm-13/bin/../include/c++/v1/__utility/swap.h:39:7 in std::__1::enable_if<(is_move_constructible<CBlockIndex**>::value) && (is_move_assignable<CBlockIndex**>::value), void>::type std::__1::swap<CBlockIndex**>(CBlockIndex**&, CBlockIndex**&) ================== ``` From https://cirrus-ci.com/task/5612886578954240?logs=ci#L4868 ACKs for top commit: achow101: re-ACK fac04cb theStack: Code-review ACK fac04cb Tree-SHA512: 9d619f99ff6373874c7ffe1db20674575605646b4b54b692fb54515a4a49f110a770026d7320ed6dfeaa7976be4cd89e93f821acdbf22c7662bd1c5be0cedcd2
thepastaclaw
force-pushed
the
takeover-7190-backport-26533-27302
branch
from
d8e7698 to
7fb0fce
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: general:claude[opus] (failed_or_unparseable), dash-core-commit-history:claude[opus] (failed_or_unparseable), backport-reviewer:claude[opus] (failed_or_unparseable), general:codex[gpt-5.5], dash-core-commit-history:codex[gpt-5.5], backport-reviewer:codex[gpt-5.5]; verifier: codex[gpt-5.5] (completed).
Verified checked-out head 7fb0fce16d324511b31927baeba3de3530931ab5. No prior findings were carried forward from the clean d8e76982 review, the latest delta d8e76982..7fb0fce1 is tree-empty with identical trees, and cumulative inspection of the current bitcoin#25077/bitcoin#26533 backport/adaptation stack found no in-scope issues.
Prior reconciliation:
- Prior review at
d8e76982found no issues, so there are no prior findings to classify as still valid, fixed, outdated, or intentionally deferred.
Carried-forward prior findings:
- None.
New findings in latest delta:
- None.
Cumulative current-head findings:
- None.
Note: GitHub does not allow me to submit APPROVE on my own PR, so this verified clean result is posted as a COMMENT.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Move Dash-specific follow-up changes out of the faithful bitcoin#25077 backport commit. These hunks update Dash-only ChainLocks, LLMQ, governance, quorum RPC, Evo test, bench, and wallet-test helper call sites for the ActiveChain()/ActiveTip()/ActiveHeight() locking contract introduced by bitcoin#25077.
…s on startup 3141eab test: add functional test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth) e252909 test: add unit test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth) 77557dd prune: scan and unlink already pruned block files on startup (Andrew Toth) Pull request description: There are a few cases where we can mark a block and undo file as pruned in our block index, but not actually remove the files from disk. 1. If we call `FindFilesToPrune` or `FindFilesToPruneManual` and crash before `UnlinkPrunedFiles`. 2. If on Windows there is an open file handle to the file somewhere else when calling `fs::remove` in `UnlinkPrunedFiles` (https://en.cppreference.com/w/cpp/filesystem/remove, https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-deletefilew#remarks). This could be from another process, or if we are calling `ReadBlockFromDisk`/`ReadRawBlockFromDisk` without having a lock on `cs_main` (which has been allowed since bitcoin@ccd8ef6). This PR mitigates this by scanning all pruned block files on startup after `LoadBlockIndexDB` and unlinking them again. ACKs for top commit: achow101: ACK 3141eab pablomartin4btc: re-ACK with added functional test 3141eab. furszy: Code review ACK 3141eab theStack: Code-review ACK 3141eab Tree-SHA512: 6c73bc57838ad1b7e5d441af3c4d6bf4c61c4382e2b86485e57fbb74a61240710c0ceeceb8b4834e610ecfa3175c6955c81ea4b2285fee11ca6383f472979d8d (cherry picked from commit bb136aa)
thepastaclaw
force-pushed
the
takeover-7190-backport-26533-27302
branch
from
b8eff03 to
98bba2d
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: general:claude[opus] (failed_quota), dash-core-commit-history:claude[opus] (failed_quota), backport-reviewer:claude[opus] (failed_quota), general:codex[gpt-5.5] (failed_auth), dash-core-commit-history:codex[gpt-5.5] (failed_auth), backport-reviewer:codex[gpt-5.5] (failed_auth), fallback general+dash-core-commit-history+backport-reviewer:openai/gpt-5.5 (completed); verifier: openai/gpt-5.5 (completed).
Verified checked-out head 98bba2d50c469078764803930fa2a9ec38f722e0. Prior review at 7fb0fce16d324511b31927baeba3de3530931ab5 found no issues, so there are no carried-forward prior findings. Latest delta 7fb0fce1..98bba2d5 rewrites evo_deterministicmns_tests locking and reorders the adaptation/bitcoin#26533 stack; cumulative inspection of the current bitcoin#25077/bitcoin#26533 backport/adaptation stack found no in-scope issues.
Prior reconciliation:
- Prior review at
7fb0fce1found no issues, so there are no prior findings to classify as still valid, fixed, outdated, or intentionally deferred.
Carried-forward prior findings:
- None.
New findings in latest delta:
- None.
Cumulative current-head findings:
- None.
Note: GitHub does not allow me to submit APPROVE on my own PR, so this verified clean result is posted as a COMMENT.
|
@coderabbitai review |
✅ Action performedReview finished.
|
5 participants
backport: Merge bitcoin#25077, 26533
Issue being fixed or feature implemented
This backports bitcoin#25077 and bitcoin#26533. bitcoin#25077
is the prerequisite that introduces
ChainstateManager::GetMutex()and fixeschain-tip reads that need to hold the chainstate mutex. bitcoin#26533 then uses
that API while adding startup cleanup for block and undo files already marked as
pruned.
bitcoin#27302 was originally included here, but it depends on the structural
bitcoin#27419
common/argsextraction. To keep proper diff-on-diff history,bitcoin#27302 is intentionally left out until bitcoin#27419 lands.
What was done?
Backported bitcoin#25077:
ChainstateManager::GetMutex()as the::cs_mainalias used by newerupstream code
Backported bitcoin#26533:
pruned
Dash-specific adaptations:
AutoFileAPI to Dash'sCAutoFile-tinyblkdefault and pruned governanceshutdown warning
How Has This Been Tested?
On macOS, in a configured worktree:
Breaking Changes
None.
Checklist
and collaborators only)