backport: Merge bitcoin/bitcoin#27297, partial #30006, #26749 - #7565
Conversation
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
… readonly BACKPORT NOTE: feature_reindex_readonly.py does not exist in Dash Core. 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)
…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
|
🕓 Ready for review — 2 ahead in queue (commit 8531f57) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
WalkthroughThe check queue APIs now accept rvalue vectors and move elements into queue storage and worker batches. Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 8531f575d1
ℹ️ 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".
|
|
||
| @contextlib.contextmanager | ||
| def wait_for_debug_log(self, expected_msgs, timeout=60): | ||
| def busy_wait_for_debug_log(self, expected_msgs, timeout=60): |
There was a problem hiding this comment.
When the default functional suite runs rpc_coinjoin.py, test_newkeypool_stops_mixing() still calls TestNode.wait_for_debug_log(), but this rename removes that method and a repo-wide search finds no alias or replacement, so the test deterministically raises AttributeError before starting CoinJoin. Update that Dash-specific caller to use busy_wait_for_debug_log() as well.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
| std::vector<utils::BlsCheck> vChecks; | ||
| vChecks.emplace_back(membersSig, memberPubKeys, commitmentHash, members_id_string); | ||
| queue_control->Add(vChecks); | ||
| queue_control->Add(std::move(vChecks)); |
There was a problem hiding this comment.
When a non-null multi-member LLMQ commitment is queued here, std::move(vChecks) does not actually move its BlsCheck elements: BlsCheck declares a destructor but no move constructor or assignment operator, so the rvalue is handled by its implicit copy operations. Consequently both CCheckQueue::Add() and Loop() copy the signature, string, and as many as 400 public keys while holding the queue mutex, replacing the previous constant-time swaps with serialized deep copies; declare explicit move operations for this Dash-specific check type.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Same as prior pr; just renamed commits; merging
5 participants
Issue being fixed or feature implemented
Backports from Bitcoin Core. Re-creates #7389 with review feedback applied (marking bitcoin#30006 as a partial backport).
What was done?
Backports the following Bitcoin Core pull requests:
test: Remove unused Check* default constructorstest: use sleepy wait-for-log in reindex readonlyfeature_reindex_readonly.pydoes not exist in Dash Core.refactor: Use move semantics instead of custom swap functionsCCheckQueueandCScriptCheckincluding Dash LLMQ signature checkqueue logic insrc/llmq/commitment.cpp.How Has This Been Tested?
src/test/test_dash --run_test=checkqueue_tests(passed)src/test/test_dash(passed)test/functional/feature_init.py(passed)test/lint/all-lint.py(passed)Breaking Changes
None
Checklist: