Merge bitcoin/bitcoin#25595: Verify PSBT inputs rather than check for fields being empty #1222
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…being empty Backport of bitcoin#25595 Original commit: 2ac71d2 Adapted for Dash: - Removed witness_utxo check (Dash does not support SegWit) - Removed final_script_witness from PSBTInputSigned (not in Dash) - Adapted VerifyScript calls for Dash API (no witness parameter) - Test changes omitted: requires test_framework/psbt.py from bitcoin#25625
WalkthroughThe PR introduces a new verification function 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 (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 (4)
src/node/psbt.cpp(1 hunks)src/psbt.cpp(2 hunks)src/psbt.h(1 hunks)test/functional/rpc_psbt.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
C++20 codebase should be placed under src/
Files:
src/node/psbt.cppsrc/psbt.cppsrc/psbt.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/node/psbt.cppsrc/psbt.cpptest/functional/rpc_psbt.pysrc/psbt.h
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
🧠 Learnings (4)
📓 Common learnings
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-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-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-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-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-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 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.
📚 Learning: 2025-07-28T22:03:12.364Z
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.
Applied to files:
src/node/psbt.cppsrc/psbt.cpp
📚 Learning: 2025-07-28T20:34:29.061Z
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.
Applied to files:
src/node/psbt.cpptest/functional/rpc_psbt.py
📚 Learning: 2025-07-29T17:13:35.087Z
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 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.
Applied to files:
test/functional/rpc_psbt.py
🧬 Code graph analysis (2)
src/node/psbt.cpp (1)
src/psbt.cpp (2)
PSBTInputSignedAndVerified(184-209)PSBTInputSignedAndVerified(184-184)
src/psbt.h (1)
src/psbt.cpp (4)
PSBTInputSigned(241-244)PSBTInputSigned(241-241)PSBTInputSignedAndVerified(184-209)PSBTInputSignedAndVerified(184-184)
⏰ 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: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: Lint / Run linters
🔇 Additional comments (4)
test/functional/rpc_psbt.py (1)
507-509: LGTM - Formatting-only change.The blank line addition before the main guard is a minor formatting adjustment with no functional impact.
src/node/psbt.cpp (1)
61-85: LGTM - Correct integration of the new verification function.The change from a simple signed check to
PSBTInputSignedAndVerified(psbtx, i, &txdata)properly implements the backport's intent of verifying signatures rather than just checking for non-empty fields. Thetxdatais correctly precomputed earlier in the function (line 26) and passed by address.src/psbt.cpp (1)
268-270: LGTM - Correct integration of verification in SignPSBTInput.The early return when the input is already signed and verified is appropriate. The
txdataparameter can benullptr(handled by the verification function), allowing this to work for both dummy signature creation and actual signing paths.src/psbt.h (1)
871-872: Performance: Consider passingpsbtby const reference instead of by value.The
PartiallySignedTransactionstruct containsstd::optional<CMutableTransaction>, vectors, and maps. Passing it by value copies the entire structure on every call, which is expensive. This function is called in hot paths likeSignPSBTInputandAnalyzePSBT.Please verify whether the original Bitcoin Core commit (2ac71d2) also passes by value. If so, this is a faithful backport but may warrant a follow-up optimization. If not, the signature should be corrected:
-bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, unsigned int input_index, const PrecomputedTransactionData* txdata); +bool PSBTInputSignedAndVerified(const PartiallySignedTransaction& psbt, unsigned int input_index, const PrecomputedTransactionData* txdata);⛔ Skipped due to learnings
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.
Comment on lines
+184
to
+209
| bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, unsigned int input_index, const PrecomputedTransactionData* txdata) | ||
| { | ||
| CTxOut utxo; | ||
| assert(psbt.inputs.size() >= input_index); | ||
| const PSBTInput& input = psbt.inputs[input_index]; | ||
|
|
||
| if (input.non_witness_utxo) { | ||
| // If we're taking our information from a non-witness UTXO, verify that it matches the prevout. | ||
| COutPoint prevout = psbt.tx->vin[input_index].prevout; | ||
| if (prevout.n >= input.non_witness_utxo->vout.size()) { | ||
| return false; | ||
| } | ||
| if (input.non_witness_utxo->GetHash() != prevout.hash) { | ||
| return false; | ||
| } | ||
| utxo = input.non_witness_utxo->vout[prevout.n]; | ||
| } else { | ||
| return false; | ||
| } | ||
|
|
||
| if (txdata) { | ||
| return VerifyScript(input.final_script_sig, utxo.scriptPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker{&(*psbt.tx), input_index, utxo.nValue, *txdata, MissingDataBehavior::FAIL}); | ||
| } else { | ||
| return VerifyScript(input.final_script_sig, utxo.scriptPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker{&(*psbt.tx), input_index, utxo.nValue, MissingDataBehavior::FAIL}); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Off-by-one error in bounds assertion.
Line 187 uses >= which allows input_index == psbt.inputs.size(), but the subsequent access at line 188 would be out-of-bounds.
Apply this diff to fix the assertion:
- assert(psbt.inputs.size() >= input_index);
+ assert(input_index < psbt.inputs.size());Additionally, as noted in the header review, the function signature passes psbt by value which is inefficient. Consider using const PartiallySignedTransaction& psbt instead.
🤖 Prompt for AI Agents
In src/psbt.cpp around lines 184 to 209, the bounds assertion is inverted and
allows an out-of-range access; change the assertion to ensure input_index is
strictly less than psbt.inputs.size() (e.g. assert(input_index <
psbt.inputs.size())) and update the function signature to take the PSBT by const
reference (const PartiallySignedTransaction& psbt) to avoid unnecessary copying;
make corresponding symbol/name adjustments in the function body if needed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Backport of bitcoin#25595
This PR adds proper PSBT input verification by calling
VerifyScripton finalized inputs rather than just checking for non-empty fields. This ensures that signatures are actually valid before proceeding.Changes
PSBTInputSignedAndVerified()function that performs script verificationSignPSBTInput()to use the new verification functionAnalyzePSBT()to use the new verification functionDash Adaptations
witness_utxocheck (Dash does not support SegWit)final_script_witnessfrom checks (not present in Dash)VerifyScriptcalls for Dash's API (no witness parameter)test_framework/psbt.pyfrom test: add test for decoding PSBT with per-input preimage types bitcoin/bitcoin#25625 which is not yet backportedOriginal commit: 2ac71d2
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.