Merge bitcoin/bitcoin#27325: test: various converttopsbt check cleanups in rpc_psbt.py
#1142
Conversation
daba957 refactor: Make ListSelected return vector (Sebastian Falbesoner) 9477662 wallet: Move CoinCointrol definitions to .cpp (Aurèle Oulès) 1db23da wallet: Use std::optional for GetExternalOutput and fixups (Aurèle Oulès) becc45b scripted-diff: Rename setSelected->m_selected_inputs (Aurèle Oulès) Pull request description: - Moves CoinControl function definitions from `coincontrol.h` to `coincontrol.cpp` - Adds more documentation - Renames class member for an improved comprehension - Use `std::optional` for `GetExternalOutput` ACKs for top commit: achow101: ACK daba957 Xekyo: ACK daba957 Tree-SHA512: 3bf2dc834a3246c2f53f8c55154258e605fcb169431d3f7b156931f33c7e3b1ae28e03e16b37f9140a827890eb7798be485b2c36bfc23ff29bb01763f289a07c
…rpc_psbt.py afc2dd5 test: various `converttopsbt` check cleanups in rpc_psbt.py (Sebastian Falbesoner) Pull request description: In the functional test rpc_psbt.py, some comments around the `converttopsbt` RPC checks are wrong or outdated and can be removed: > _Error could be either "TX decode failed" (segwit inputs causes > parsing to fail) or "Inputs must not have scriptSigs and > scriptWitnesses"_ Decoding a valid TX with at least one input always succeeds with the [heuristic](https://github.com/bitcoin/bitcoin/blob/e352f5ab6b60ec1cc549997275e945238508cdee/src/core_read.cpp#L126), i.e. this comment is not right and we can assert for the error string "Inputs must not have scriptSigs and scriptWitnesses" on the calls below. > _We must set iswitness=True because the serialized transaction has > inputs and is therefore a witness transaction_ This is also unneeded (and confusing, w.r.t. "is therefore a witness transaction"?), for a TX with one input there is no need to set the `iswitness` parameter. For sake of completeness, we still keep one variant where iswitness is explicitly set to true. Lastly, there is a superflous `converttopsbt` call on the raw tx which is the same as just [about ~10 lines above](https://github.com/bitcoin/bitcoin/blob/master/test/functional/rpc_psbt.py#L393-L397), so it can be removed. ACKs for top commit: ismaelsadeeq: tested ACK afc2dd5 achow101: ACK afc2dd5 Tree-SHA512: 467efefdb3f61efdb79145044b90fc8dc2f0c425f078117a99112b0074e7d4a32c34e464f665fbf8de70d06caaa5d4ad5908c1d75d2e7607eccb0837480afab3
WalkthroughRefactors CCoinControl API to return values instead of using out-parameters, adds input-weight management and selection/query helpers, updates callsites in Qt, wallet code, and fuzz tests, and adjusts a PSBT functional test to rely on optional-return semantics and explicit permitsigdata handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.13.1)test/functional/rpc_psbt.py�[1;31mruff failed�[0m 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/wallet/test/fuzz/coincontrol.cpp (1)
56-73: Revert fuzz test additions in src/wallet/test/fuzz/coincontrol.cpp
The upstream PR bitcoin#27325 makes no changes to fuzz tests; please remove these edits and restrict this backport to the rpc_psbt.py test cleanups with Dash-specific witness adaptations.src/wallet/coincontrol.h (1)
18-150: Remove unrelated CCoinControl header changes
Upstream PR bitcoin#27325 only modifies test/functional/rpc_psbt.py—revert all edits to src/wallet/coincontrol.h to maintain backport fidelity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/qt/coincontroldialog.cpp(1 hunks)src/wallet/coincontrol.cpp(1 hunks)src/wallet/coincontrol.h(3 hunks)src/wallet/spend.cpp(3 hunks)src/wallet/test/fuzz/coincontrol.cpp(2 hunks)test/functional/rpc_psbt.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
C++20 codebase should be placed under src/
Files:
src/qt/coincontroldialog.cppsrc/wallet/coincontrol.cppsrc/wallet/coincontrol.hsrc/wallet/spend.cppsrc/wallet/test/fuzz/coincontrol.cpp
**
⚙️ 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/qt/coincontroldialog.cpptest/functional/rpc_psbt.pysrc/wallet/coincontrol.cppsrc/wallet/coincontrol.hsrc/wallet/spend.cppsrc/wallet/test/fuzz/coincontrol.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be placed in test/functional/ and written in Python
Files:
test/functional/rpc_psbt.py
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/wallet/test/fuzz/coincontrol.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#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
PR: DashCoreAutoGuix/dash#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
PR: DashCoreAutoGuix/dash#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
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: When backporting Bitcoin Core commits that use Python's textwrap.dedent() function in test files, the textwrap import statement needs to be explicitly added if it's missing in the Dash test file.
🧬 Code graph analysis (3)
test/functional/rpc_psbt.py (2)
src/wallet/rpc/spend.cpp (2)
signrawtransactionwithwallet(641-730)signrawtransactionwithwallet(641-641)test/functional/test_framework/util.py (1)
assert_raises_rpc_error(132-148)
src/wallet/coincontrol.cpp (1)
src/wallet/coincontrol.h (1)
HasSelected(84-141)
src/wallet/coincontrol.h (1)
src/wallet/coincontrol.cpp (24)
HasSelected(16-19)HasSelected(16-16)IsSelected(21-24)IsSelected(21-21)IsExternalSelected(26-29)IsExternalSelected(26-26)GetExternalOutput(31-39)GetExternalOutput(31-31)Select(41-44)Select(41-41)SelectExternal(46-50)SelectExternal(46-46)UnSelect(52-55)UnSelect(52-52)UnSelectAll(57-60)UnSelectAll(57-57)ListSelected(62-65)ListSelected(62-62)SetInputWeight(67-70)SetInputWeight(67-67)HasInputWeight(72-75)HasInputWeight(72-72)GetInputWeight(77-82)GetInputWeight(77-77)
🔇 Additional comments (4)
src/qt/coincontroldialog.cpp (1)
457-457: LGTM! API usage correctly updated to return-value pattern.The change correctly adapts to the new
ListSelected()API that returnsstd::vector<COutPoint>directly instead of filling an out-parameter. This is a clean mechanical refactor with no logic changes.test/functional/rpc_psbt.py (1)
228-235: Dash-specific error text looks goodNice catch updating the assertions to look for “scriptSigs” only and documenting the default
permitsigdata. That matches the Dash RPC error surface (no witness path) while keeping the upstream structure intact.src/wallet/spend.cpp (2)
68-73: Optional lookup mirrors the newer coin control APISwitching to the optional-returning
GetExternalOutputkeeps this helper aligned with the updated CCoinControl semantics and avoids an extra temporary. Looks good.
526-543: Preset input handling matches upstream shapeThe preset-input loop now mirrors the vector-returning
ListSelected()and reuses the optional accessor, which keeps the backport consistent with the refactored coin control API. No issues spotted.
|
|
||
| bool CCoinControl::HasSelected() const | ||
| { | ||
| return !m_selected_inputs.empty(); | ||
| } | ||
|
|
||
| bool CCoinControl::IsSelected(const COutPoint& output) const | ||
| { | ||
| return m_selected_inputs.count(output) > 0; | ||
| } | ||
|
|
||
| bool CCoinControl::IsExternalSelected(const COutPoint& output) const | ||
| { | ||
| return m_external_txouts.count(output) > 0; | ||
| } | ||
|
|
||
| std::optional<CTxOut> CCoinControl::GetExternalOutput(const COutPoint& outpoint) const | ||
| { | ||
| const auto ext_it = m_external_txouts.find(outpoint); | ||
| if (ext_it == m_external_txouts.end()) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| return std::make_optional(ext_it->second); | ||
| } | ||
|
|
||
| void CCoinControl::Select(const COutPoint& output) | ||
| { | ||
| m_selected_inputs.insert(output); | ||
| } | ||
|
|
||
| void CCoinControl::SelectExternal(const COutPoint& outpoint, const CTxOut& txout) | ||
| { | ||
| m_selected_inputs.insert(outpoint); | ||
| m_external_txouts.emplace(outpoint, txout); | ||
| } | ||
|
|
||
| void CCoinControl::UnSelect(const COutPoint& output) | ||
| { | ||
| m_selected_inputs.erase(output); | ||
| } | ||
|
|
||
| void CCoinControl::UnSelectAll() | ||
| { | ||
| m_selected_inputs.clear(); | ||
| } | ||
|
|
||
| std::vector<COutPoint> CCoinControl::ListSelected() const | ||
| { | ||
| return {m_selected_inputs.begin(), m_selected_inputs.end()}; | ||
| } | ||
|
|
||
| void CCoinControl::SetInputWeight(const COutPoint& outpoint, int64_t weight) | ||
| { | ||
| m_input_weights[outpoint] = weight; | ||
| } | ||
|
|
||
| bool CCoinControl::HasInputWeight(const COutPoint& outpoint) const | ||
| { | ||
| return m_input_weights.count(outpoint) > 0; | ||
| } | ||
|
|
||
| int64_t CCoinControl::GetInputWeight(const COutPoint& outpoint) const | ||
| { | ||
| auto it = m_input_weights.find(outpoint); | ||
| assert(it != m_input_weights.end()); | ||
| return it->second; | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Out-of-scope changes for PR bitcoin#27325 — revert from this backport.
Bitcoin PR bitcoin#27325 only touches test/functional/rpc_psbt.py; it does not change wallet code. These CCoinControl API/impl additions are unrelated and must not be included in this backport. Please drop this file from the PR. Based on learnings.
Upstream reference: Bitcoin PR bitcoin#27325 edits only rpc_psbt.py (16 total line changes). (github.com)
Remove CCoinControl changes from this backport
Bitcoin PR bitcoin#27325 only updates test/functional/rpc_psbt.py; the CCoinControl API/implementation in src/wallet/coincontrol.cpp is out of scope—drop this file from the PR. (bitcoin-irc.chaincode.com)
🤖 Prompt for AI Agents
src/wallet/coincontrol.cpp lines 15-82: The backport included changes to
CCoinControl which are out of scope for the PR; remove these changes by
reverting this file to the upstream/base branch state or by excluding it from
the commit (restore the original file content or checkout the file from the
target branch and recommit), ensuring only test/functional/rpc_psbt.py remains
changed in the PR.
Backport Information
Bitcoin Commit: 30bf70c (afc2dd5)
Bitcoin PR: bitcoin#27325
Target Version: 0.26
Batch: 415
Summary
This PR backports Bitcoin Core PR bitcoin#27325, which cleans up test comments and assertions in the rpc_psbt.py functional test.
Changes
Dash-Specific Adaptations
iswitness=Truewas removed as it's not relevant to DashTesting
test/functional/rpc_psbt.pyGenerated by Bitcoin to Dash backport automation
Summary by CodeRabbit