Merge bitcoin/bitcoin#25963: CBlockLocator: performance-move-const-arg Clang tidy fixup#1210
Conversation
…tidy fixup 6b24dfe CBlockLocator: performance-move-const-arg Clang tidy fixups (Jon Atack) Pull request description: Fix Clang-tidy CI errors on master. See https://cirrus-ci.com/task/4806752200818688?logs=ci#L4696 for an example. ACKs for top commit: MarcoFalke: review ACK 6b24dfe vasild: ACK 6b24dfe Tree-SHA512: 7a67acf7b42da07b63fbb392236e9a7be8cf35c36e37ca980c4467fe8295c2eda8aef10f41a1e3036cd9ebece47fa957fc3256033f853bd6a97ce2ca42799a0a
WalkthroughThis PR introduces move semantics to the CBlockLocator class by changing its constructor parameter from a const lvalue reference to an rvalue reference. Call sites are updated to pass std::move-wrapped vectors to enable efficient transfer of ownership without copies. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/chain.cpp(1 hunks)src/primitives/block.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
C++20 codebase should be placed under src/
Files:
src/chain.cppsrc/primitives/block.h
**
⚙️ CodeRabbit configuration file
**: # CodeRabbit AI Review Instructions for Dash BackportsYour Role
You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.
Critical Validation Rules
1. File Operations Must Match (AUTO-REJECT if violated)
- If Bitcoin modifies an existing file → Dash MUST modify (not create new)
- If Bitcoin creates a new file → Dash creates
- If Bitcoin deletes a file → Dash deletes
- Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys
2. Size Ratio Check (80-150% of Bitcoin)
- Count functional lines changed (exclude comments/whitespace)
- Dash changes should be 80-150% of Bitcoin's size
- Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash
3. No Scope Creep
- Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
- No unrelated refactoring or style changes
- Only Bitcoin's intended changes + minimal Dash adaptations
4. Bitcoin-Specific Code Detection
- Auto-reject witness/segwit code:
msg_wtxidrelay,MSG_WTX, witness imports- Auto-reject RBF (replace-by-fee) functionality
- Note: PSBT is supported in Dash (don't flag)
5. Mandatory Adaptations Only
bitcoin→dashin strings/pathsBitcoin→Dashin user-facing text- Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
- Hardcoded test values specific to Dash
- No other changes unless absolutely required
6. Completeness Check
- All files changed in Bitcoin must be present
- Extra files need clear justification (Dash-specific compatibility)
- Missing files = incomplete backport
Review Process
- First: Check file operations match exactly
- Second: Calculate size ratio
- Third: Scan for scope creep patterns
- Fourth: Detect Bitcoin-specific code
- Fifth: Verify all changes are minimal adaptations
Output Format
VALIDATION: [PASS/FAIL] File Operatio...
Files:
src/chain.cppsrc/primitives/block.h
🧠 Learnings (1)
📓 Common learnings
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T19:54:21.426Z
Learning: In Dash backports from Bitcoin Core, including necessary compilation fixes (such as API compatibility changes like UniValue get_int() → getInt<int>()) alongside the core backport is standard and expected practice. These compatibility fixes ensure the backported code compiles in Dash's evolved codebase while preserving Bitcoin's original functionality and intent.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-27T22:35:10.176Z
Learning: In Dash backports, src/dashbls files are vendored dependencies that should not be modified during Bitcoin Core backports unless there is specific justification. Unauthorized modifications to vendored dependencies should be removed to maintain code integrity.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: In Dash backports from Bitcoin Core, when the DIFFICULTY_ADJUSTMENT_INTERVAL constant is missing, it should be defined as 24 for Dash (different from Bitcoin's value), as seen in the getnetworkhashps RPC backport fix.
🧬 Code graph analysis (2)
src/chain.cpp (1)
test/functional/test_framework/messages.py (1)
CBlockLocator(419-436)
src/primitives/block.h (1)
test/functional/test_framework/messages.py (1)
CBlockLocator(419-436)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (1)
src/primitives/block.h (1)
245-245: LGTM: Move semantics correctly implemented.The constructor signature change from
const std::vector<uint256>&tostd::vector<uint256>&&withstd::move(have)initialization correctly enables move semantics, eliminating unnecessary vector copies during construction. This matches the intent of Bitcoin Core's Clang-Tidy performance-move-const-arg fixup.
| } | ||
|
|
||
| return CBlockLocator(vHave); | ||
| return CBlockLocator(std::move(vHave)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual code in src/chain.cpp around line 57
wc -l src/chain.cppRepository: DashCoreAutoGuix/dash
Length of output: 82
🏁 Script executed:
# Get the context around line 57
sed -n '30,65p' src/chain.cppRepository: DashCoreAutoGuix/dash
Length of output: 1127
🏁 Script executed:
# Check the CBlockLocator constructor definition
rg -n -B2 -A3 'CBlockLocator\(' src/primitives/block.hRepository: DashCoreAutoGuix/dash
Length of output: 286
🏁 Script executed:
# Find all CBlockLocator constructor call sites in the codebase
rg -n 'CBlockLocator\s*\(' --type=cpp | head -20Repository: DashCoreAutoGuix/dash
Length of output: 454
🌐 Web query:
Bitcoin Core GetLocator CBlockLocator std::move rvalue reference
💡 Result:
Short answer: GetLocator returns a CBlockLocator by value; you should not std::move a local CBlockLocator when returning it — return it normally and let the compiler elide or implicitly move it.
Why:
- Bitcoin Core's CChain::GetLocator returns a CBlockLocator (a struct holding a vector of block hashes) by value. [1][2]
- Applying std::move to a local in a return statement prevents RVO/NRVO and can make the compiler perform a move where copy-elision would have been cheaper or automatic; returning the local by name lets the compiler elide or implicitly move as appropriate. Don't return an rvalue-reference (T&&) from such functions. [3][4]
References:
- CChain::GetLocator (Bitcoin Core doxygen). [1]
- CBlockLocator definition (primitives/block.h doc). [2]
- Guidance on not using std::move in return statements (rvalue-reference / RVO explanation). [3][4]
🏁 Script executed:
# Let's check the git history to see what Bitcoin commit this backports
git log --oneline src/chain.cpp | head -10Repository: DashCoreAutoGuix/dash
Length of output: 162
🏁 Script executed:
# Check for any Bitcoin commit references in recent changes
git log --all --oneline --grep="Bitcoin" | head -20Repository: DashCoreAutoGuix/dash
Length of output: 2139
🏁 Script executed:
# Search for comments or references indicating the source Bitcoin commit
rg -i "backport|bitcoin.*commit|bitcoin.*pr" --type=cpp src/chain.cpp | head -5Repository: DashCoreAutoGuix/dash
Length of output: 47
🌐 Web query:
Bitcoin Core CChain::GetLocator implementation #25963
💡 Result:
Do you want (A) a link to PR bitcoin#25963 on github, (B) a short summary of the change, or (C) the code diff / explanation of CChain::GetLocator implementation?
🏁 Script executed:
# Check the Clang tidy commit that was applied
git show 788883f04 --statRepository: DashCoreAutoGuix/dash
Length of output: 50378
🏁 Script executed:
# Look at what the commit actually changed
git show 788883f04 src/chain.cpp 2>/dev/null | head -80Repository: DashCoreAutoGuix/dash
Length of output: 2916
Incorrect adaptation: std::move() prevents compiler optimizations.
This change contradicts C++ best practices. Applying std::move() to a local variable in a return statement prevents Return Value Optimization (RVO) and Named Return Value Optimization (NRVO), which are more efficient than explicit moves. The compiler should automatically elide or move the local vHave without manual std::move().
Correct code:
return CBlockLocator(vHave);While Bitcoin bitcoin#25963 added this change, it represents a Clang tidy "fix" that actually reduces performance by inhibiting compiler optimizations. This should be reverted to the standard pattern.
🤖 Prompt for AI Agents
In src/chain.cpp around line 57, the return currently uses std::move(vHave)
which prevents RVO/NRVO and can degrade performance; remove the std::move and
return the local directly (construct CBlockLocator from vHave without std::move)
so the compiler can apply RVO/NRVO and optimize the return.
✅ Verification CompleteBitcoin Commit: b936123 (bitcoin#25963) SummaryThis backport correctly applies the Clang-tidy performance-move-const-arg fix to Dash. The changes are faithful to Bitcoin's intent with appropriate adaptation for Dash's code structure. Changes Verified
Validation Results
ConclusionThe backport is faithful and correctly adapted. The architectural difference between Bitcoin's free-function pattern and Dash's member function pattern is properly handled. Approving with 1 acceptable flaky test failure. 🤖 Automated verification by backport agent |
DashCoreAutoGuix
added
the
verified
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
NACK.
changes in src/primitives/block.h are correct
changes in src/chain.cpp are incorrect. Need to do after bitcoin#25717
4 participants
Summary
Backports bitcoin#25963 to Dash.
This PR applies the Clang-tidy
performance-move-const-argfix forCBlockLocator:CBlockLocatorconstructor fromconst std::vector<uint256>&tostd::vector<uint256>&&for move semanticsCChain::GetLocator()to usestd::move()appropriatelyNote: Bitcoin's
src/chain.cpphad a different implementation (free-standingGetLocatorwithLocatorEntriesfunction), while Dash uses a member function pattern. The fix is adapted accordingly - Bitcoin removed unnecessarystd::movefrom a return value (already an rvalue), while Dash addsstd::moveto the local variable to work with the new move-only constructor.Original commit: b936123
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.