backport: Merge bitcoin/bitcoin#27297, 30006, 26749#7389
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 7cb143e) |
![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: 5aa137ee7d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| void Add(std::vector<T>& vChecks) | ||
| void Add(std::vector<T>&& vChecks) |
There was a problem hiding this comment.
Changing CCheckQueueControl::Add to accept only std::vector<T>&& leaves existing Dash-specific LLMQ callers uncompilable: git grep "Add(vChecks" still finds queue_control->Add(vChecks) in src/llmq/commitment.cpp lines 76 and 90, and an lvalue vector cannot bind to this rvalue-reference parameter. Please either keep an lvalue-compatible overload or move/update those call sites as part of this API change.
Useful? React with 👍 / 👎.
|
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 (9)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughThe PR replaces swap-based patterns with move semantics throughout the check queue path. Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Validation as CChainState::ConnectBlock
participant Control as CCheckQueueControl
participant Queue as CCheckQueue
participant Worker as checkqueue worker
Validation->>Control: Add(std::move(vChecks))
Control->>Queue: Add(std::move(vChecks))
Queue->>Queue: move-insert checks into internal queue
Worker->>Queue: extract batch from tail range
Queue-->>Worker: moved vChecks batch
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
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): Backport of bitcoin#26749 changes CCheckQueue/CCheckQueueControl::Add to take std::vector&&, but the Dash-specific LLMQ caller in src/llmq/commitment.cpp was not adapted — both call sites still pass lvalues and will fail to compile. A secondary concern is that the Dash-specific BlsCheck element type has a user-declared destructor that suppresses implicit move operations, so the LLMQ check path will silently fall back to copies even after the call sites are fixed, defeating the refactor's purpose on this path.
🔴 6 blocking | 🟡 2 suggestion(s)
Findings not posted inline (4)
These findings could not be anchored to the current diff, but they are still part of this review.
- [BLOCKING]
src/llmq/commitment.cpp:76: queue_control->Add(vChecks) passes an lvalue — won't compile after the signature change — This PR changes CCheckQueueControl::Add (src/checkqueue.h:233) and CCheckQueue::Add (src/checkqueue.h:163) from taking std::vector& to std::vector&&. Every upstream call site in the diff was updated to use std::move(vChecks), but this Dash-specific LLMQ call site in CFinalCommitment::... - [BLOCKING]
src/llmq/commitment.cpp:90: Second LLMQ Add() call also passes an lvalue — same compile failure — Same problem as line 76: queue_control->Add(vChecks) passes a non-movable lvalue to the new Add(std::vector&&) overload. Both LLMQ call sites in this function must be updated together for the file to compile. - [SUGGESTION]
src/llmq/utils.h:45-58: BlsCheck suppresses implicit moves — LLMQ check path silently copies — After this backport, CCheckQueue::Loop uses std::make_move_iterator to drain the internal queue, and Add likewise inserts with std::make_move_iterator. Upstream prepared CScriptCheck for this by deleting copy ops and defaulting move ops. The Dash-specific BlsCheck (the element type used by m_bls_... - [BLOCKING]
src/llmq/commitment.cpp:76-90: Incomplete Dash adaptation for bitcoin#26749 — bitcoin#26749 changes CCheckQueueControl::Add and CCheckQueue::Add from taking std::vector& to std::vector&& and upstream updates all affected call sites to pass std::move(vChecks). Dash has an additional CCheckQueue user for LLMQ BLS checks that does not exist upstream. In CFinalCommitment...
🤖 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/commitment.cpp`:
- [BLOCKING] src/llmq/commitment.cpp:76: queue_control->Add(vChecks) passes an lvalue — won't compile after the signature change
This PR changes CCheckQueueControl<T>::Add (src/checkqueue.h:233) and CCheckQueue<T>::Add (src/checkqueue.h:163) from taking std::vector<T>& to std::vector<T>&&. Every upstream call site in the diff was updated to use std::move(vChecks), but this Dash-specific LLMQ call site in CFinalCommitment::VerifySignatureAsync was not. The lvalue vChecks cannot bind to the new rvalue reference parameter, so this file will fail to build. Upstream review couldn't catch this because the LLMQ caller is Dash-only.
- [BLOCKING] src/llmq/commitment.cpp:90: Second LLMQ Add() call also passes an lvalue — same compile failure
Same problem as line 76: queue_control->Add(vChecks) passes a non-movable lvalue to the new Add(std::vector<T>&&) overload. Both LLMQ call sites in this function must be updated together for the file to compile.
- [BLOCKING] src/llmq/commitment.cpp:76-90: Incomplete Dash adaptation for bitcoin#26749
bitcoin#26749 changes CCheckQueueControl::Add and CCheckQueue::Add from taking std::vector<T>& to std::vector<T>&& and upstream updates all affected call sites to pass std::move(vChecks). Dash has an additional CCheckQueue user for LLMQ BLS checks that does not exist upstream. In CFinalCommitment::VerifySignatureAsync, both queue_control->Add(vChecks) calls still pass an lvalue vector. With the new signature in src/checkqueue.h, these calls cannot bind to std::vector<utils::BlsCheck>&&, so the backport is incomplete and this Dash-specific file will not compile. This is a merge/adaptation gap rather than a missing upstream prerequisite; upstream had no corresponding LLMQ call site to update.
---
**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.
- [BLOCKING] src/llmq/commitment.cpp:76: queue_control->Add(vChecks) passes an lvalue — won't compile after the signature change
This PR changes CCheckQueueControl<T>::Add (src/checkqueue.h:233) and CCheckQueue<T>::Add (src/checkqueue.h:163) from taking std::vector<T>& to std::vector<T>&&. Every upstream call site in the diff was updated to use std::move(vChecks), but this Dash-specific LLMQ call site in CFinalCommitment::VerifySignatureAsync was not. The lvalue vChecks cannot bind to the new rvalue reference parameter, so this file will fail to build. Upstream review couldn't catch this because the LLMQ caller is Dash-only.
- [BLOCKING] src/llmq/commitment.cpp:90: Second LLMQ Add() call also passes an lvalue — same compile failure
Same problem as line 76: queue_control->Add(vChecks) passes a non-movable lvalue to the new Add(std::vector<T>&&) overload. Both LLMQ call sites in this function must be updated together for the file to compile.
- [BLOCKING] src/llmq/commitment.cpp:76-90: Incomplete Dash adaptation for bitcoin#26749
bitcoin#26749 changes CCheckQueueControl::Add and CCheckQueue::Add from taking std::vector<T>& to std::vector<T>&& and upstream updates all affected call sites to pass std::move(vChecks). Dash has an additional CCheckQueue user for LLMQ BLS checks that does not exist upstream. In CFinalCommitment::VerifySignatureAsync, both queue_control->Add(vChecks) calls still pass an lvalue vector. With the new signature in src/checkqueue.h, these calls cannot bind to std::vector<utils::BlsCheck>&&, so the backport is incomplete and this Dash-specific file will not compile. This is a merge/adaptation gap rather than a missing upstream prerequisite; upstream had no corresponding LLMQ call site to update.
---
**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.
In `src/llmq/utils.h`:
- [SUGGESTION] src/llmq/utils.h:45-58: BlsCheck suppresses implicit moves — LLMQ check path silently copies
After this backport, CCheckQueue::Loop uses std::make_move_iterator to drain the internal queue, and Add likewise inserts with std::make_move_iterator. Upstream prepared CScriptCheck for this by deleting copy ops and defaulting move ops. The Dash-specific BlsCheck (the element type used by m_bls_queue in src/llmq/blockprocessor.h) still declares a user-defined destructor (utils.h:54, defined at utils.cpp:537), which suppresses implicit move ctor/assignment. Elements will fall back to copying signature, pubkey vector, hash, and id string per element — the LLMQ path silently loses the performance benefit this PR was meant to deliver. The now-unused BlsCheck::swap should also be removed for consistency with the upstream refactor.
- [SUGGESTION] src/llmq/utils.h:45-58: BlsCheck suppresses implicit moves — LLMQ check path silently copies
After this backport, CCheckQueue::Loop uses std::make_move_iterator to drain the internal queue, and Add likewise inserts with std::make_move_iterator. Upstream prepared CScriptCheck for this by deleting copy ops and defaulting move ops. The Dash-specific BlsCheck (the element type used by m_bls_queue in src/llmq/blockprocessor.h) still declares a user-defined destructor (utils.h:54, defined at utils.cpp:537), which suppresses implicit move ctor/assignment. Elements will fall back to copying signature, pubkey vector, hash, and id string per element — the LLMQ path silently loses the performance benefit this PR was meant to deliver. The now-unused BlsCheck::swap should also be removed for consistency with the upstream refactor.
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
July_2026_4
branch
4 times, most recently
from
cd737ae to
7330bfb
Compare
vijaydasmp
changed the title
fae3490 test: Remove unused Check* default constructors (MarcoFalke) Pull request description: They are no longer needed after the removal of `swap`, see bitcoin#26749 (comment) Also, flatten a redundant `if` check. ACKs for top commit: hebasto: ACK fae3490 Tree-SHA512: c0bc0c16b5df0f16fc25e18d2414a2a3c4769da1aa30d53f8d267bc2e97dd79a0296db94c1e49cd1ca89bd42275d8c462f7bf47f03f105dfe867ebea6563454b
fd6a7d3 test: use sleepy wait-for-log in reindex readonly (Matthew Zipkin) Pull request description: Also rename the busy wait-for-log method to prevent recurrence. See bitcoin#27039 (comment) ACKs for top commit: maflcko: utACK fd6a7d3 achow101: ACK fd6a7d3 tdb3: ACK for fd6a7d3 rkrux: ACK [fd6a7d3](bitcoin@fd6a7d3) Tree-SHA512: 7ff0574833df1ec843159b35ee88b8bb345a513ac13ed0b72abd1bf330c454a3f9df4d927871b9e3d37bfcc07542b06ef63acef8e822cd18499adae8cbb0cda8
vijaydasmp
changed the title
…wap functions 95ad70a test: Default initialize `should_freeze` to `true` (Hennadii Stepanov) cea5052 refactor: Drop no longer used `swap` member functions (Hennadii Stepanov) a87fb6b clang-tidy: Fix modernize-use-default-member-init in `CScriptCheck` (Hennadii Stepanov) b4bed5c refactor: Drop no longer used `CScriptCheck()` default constructor (Hennadii Stepanov) d8427cc refactor: Use move semantics in `CCheckQueue::Loop` (Hennadii Stepanov) 9a0b524 clang-tidy, test: Fix bugprone-use-after-move in `Correct_Queue_range()` (Hennadii Stepanov) 04831fe refactor: Make move semantics explicit for callers (Hennadii Stepanov) 6c2d597 refactor: Use move semantics in `CCheckQueue::Add` (Hennadii Stepanov) 0682003 test, refactor: Avoid `CScriptCheck::swap` in `transaction_tests` (Hennadii Stepanov) 15209d9 consensus, refactor: Avoid `CScriptCheck::swap` in `CheckInputScripts` (Hennadii Stepanov) Pull request description: This PR makes code more succinct and readable by using move semantics. ACKs for top commit: martinus: re-ACK 95ad70a achow101: ACK 95ad70a TheCharlatan: re-ACK bitcoin@95ad70a MarcoFalke: re-ACK 95ad70a 🚥 Tree-SHA512: adda760891b12d252dc9b823fe7c41eed660364b6fb1a69f17607d7a31eb0bbb82a80d154a7acfaa241b5de37d42a293c2b6e059f26a8e92d88d3a87c99768fb
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cb143e3bd
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| queue.pop_back(); | ||
| } | ||
| auto start_it = queue.end() - nNow; | ||
| vChecks.assign(std::make_move_iterator(start_it), std::make_move_iterator(queue.end())); |
There was a problem hiding this comment.
When this template is instantiated as CCheckQueue<utils::BlsCheck> (src/llmq/blockprocessor.h:49), this make_move_iterator path still copies instead of moves: BlsCheck declares ~BlsCheck() and no move members (src/llmq/utils.h:52-54), so rvalues bind to the copy constructor. The old code swapped the queued check out cheaply; now dispatching here, and the analogous insert in Add, copies the signature, id string, and full m_pubkeys vector while m_mutex is held for every LLMQ commitment check. Please make BlsCheck actually movable or keep a cheap transfer path for this instantiation.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude-general=opus:failed, codex-general=gpt-5.5:completed, claude-dash-core-commit-history=opus:failed, codex-dash-core-commit-history=gpt-5.5:completed, claude-backport-reviewer=opus:failed, codex-backport-reviewer=gpt-5.5:completed; verifier=codex:gpt-5.5.
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): The current head fixes the prior LLMQ compile blockers: both Dash-specific CCheckQueueControl::Add() calls now move their vectors. The remaining in-scope issues are a blocking backport-completeness gap for the advertised bitcoin#30006 backport, plus a Dash-specific BlsCheck move-semantics gap and one commit-subject nitpick.
🔴 3 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
5 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 `test/functional/feature_reindex_readonly.py`:
- [BLOCKING] test/functional/feature_reindex_readonly.py:1: Backport bitcoin#27850 before claiming the full bitcoin#30006 backport
The upstream `bitcoin#30006` diff updates three files, including `test/functional/feature_reindex_readonly.py`, where it replaces the old busy `wait_for_debug_log([b"Reindexing finished"])` call with `assert_debug_log(["Reindexing finished"], timeout=60)`. This Dash backport carries the `feature_init.py` and `test_node.py` hunks, but Dash does not have `feature_reindex_readonly.py` or a corresponding `test_runner.py` entry. That missing file comes from upstream `bitcoin#27850`, so a PR titled as a full backport of `bitcoin#30006` is missing a prerequisite test chain unless the omission is explicitly documented as intentional.
- [BLOCKING] test/functional/feature_reindex_readonly.py:1: Missing prerequisite: bitcoin#27850
bitcoin#30006's upstream diff changes three files: test/functional/feature_init.py, test/functional/feature_reindex_readonly.py, and test/functional/test_framework/test_node.py. This Dash backport carries the feature_init.py and test_node.py hunks, but Dash has no test/functional/feature_reindex_readonly.py and no corresponding test_runner.py entry. Upstream's pre-#30006 file used `with self.nodes[0].wait_for_debug_log([b"Reindexing finished"]):`, and #30006 changes that to `assert_debug_log(["Reindexing finished"], timeout=60)`. That file was introduced by bitcoin#27850 (`541976b42eb`, commit `de8f9123afb test: cover read-only blockstore`) and later updated before #30006. I found no Dash-specific exclusion for this read-only blockstore/reindex test; Dash has the production `-fastprune` and `Reindexing finished` code paths, but not the upstream functional coverage. This is soft/test-only because the helper rename is otherwise adapted cleanly and no current Dash caller is broken, but a full bitcoin#30006 backport is missing this prerequisite test chain.
---
**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.
In `src/llmq/utils.h`:
- [SUGGESTION] src/llmq/utils.h:52-57: Give BlsCheck real move operations under the new check queue path
This PR changes `CCheckQueue::Add()` and `Loop()` to transfer queue elements with `std::make_move_iterator`, and the LLMQ callers now correctly pass `std::move(vChecks)`. However, Dash's queue element type `BlsCheck` still declares a user-provided destructor and no move constructor or move assignment, so its implicit move operations are suppressed and the new queue path falls back to copying the signature, public-key vector, hash, and id string. Upstream made this refactor effective by making `CScriptCheck` move-only; the Dash-specific LLMQ element should receive the same treatment, and the now-unused default constructor and `swap()` can be removed.
- [BLOCKING] src/llmq/utils.h:52-58: Incomplete Dash adaptation for bitcoin#26749: BlsCheck remains copy-only
Prior finding prior-3 is still valid. bitcoin#26749 changes CCheckQueue to enqueue and drain batches through `std::make_move_iterator`, and upstream adapted `CScriptCheck` by deleting copy operations and defaulting move operations. Dash's LLMQ queue uses `CCheckQueue<utils::BlsCheck>`, and the current head fixed the `Add(std::move(vChecks))` call sites, but `BlsCheck` still declares a default constructor, a user-declared destructor, and `swap(BlsCheck&)` with no explicit move constructor or move assignment. The user-declared destructor suppresses implicit move operations, so the new queue move path falls back to copying the signature, pubkey vector, hash, and id string. This does not break compilation or behavior, but it leaves the Dash-specific queue element without the equivalent move-semantics adaptation that bitcoin#26749 applied to upstream queue element types.
---
**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.
4 participants
bitcoin backports