psbt: implement PSBTv2 (BIP-370) support#2496
psbt: implement PSBTv2 (BIP-370) support#2496TechLateef wants to merge 6 commits intobtcsuite:masterfrom
Conversation
guggero
left a comment
There was a problem hiding this comment.
Thanks for the PR! Code looks pretty good, but I think there are probably a few hidden edge cases that need to be ironed out.
It's a lot of code in a single commit, so not very easy to go through. If you want to increase your changes of finding a second reviewer, I suggest to split things up where possible.
I did a first pass, will probably require a few more rounds.
|
@guggero Thanks for the detailed feedback! I've gone through and addressed all the points you raised: Code style improvements:
Fixed the duplication issue in finalizer.go: Added proper test coverage: The unknowns bug fix: I think this addresses all the main concerns. The code is much cleaner now and should be easier to maintain going forward. Let me know if there's anything else that needs attention! |
d974375 to
c4d9112
Compare
Add the fundamental type definitions for PSBTv2 as specified in BIP-370: - Global fields: TxVersion, FallbackLocktime, InputCount, OutputCount, TxModifiable - Input fields: PreviousTxid, OutputIndex, Sequence, TimeLocktime, HeightLocktime - Output fields: Amount, Script
46f79e1 to
a91e2e5
Compare
|
Quick follow-up: I've made a few additional refinements for consistency:
The commit history has been cleaned up into 6 logical commits for easier review. Ready for another look when you have time! |
|
Thanks for the updates and the reworked commit structure! Will be useful when re-reviewing. Will take a look as soon as I find some time. |
guggero
left a comment
There was a problem hiding this comment.
I tasked Claude Code with a review, here's the result. You can find the commit with the fixed code and the additional test cases here: guggero@ef41278
You can use from that what you want, just please clean it up first, as it's agent-generated code.
Claude Code PR Review
Critical Bugs (FIXED)
1. SumUtxoInputValues panics on PSBTv2 (utils.go:297-335)
SumUtxoInputValues unconditionally dereferences packet.UnsignedTx.TxIn, which
is nil for v2 PSBTs. This causes a nil pointer panic. GetTxFee() calls this
function so it also crashes on v2.
Fix applied: Check packet.Version and use packet.Inputs / pInput.OutputIndex
for v2 instead of packet.UnsignedTx.TxIn. Tests: TestV2SumUtxoInputValues*,
TestV2GetTxFee.
2. Input serialization key ordering violates BIP-174 (partial_input.go)
BIP-174 requires key-value pairs in ascending key order. For unfinalized v2
inputs, the v2 fields (0x0e-0x12) were serialized after taproot fields
(0x13-0x18), violating the ordering requirement (0x0e < 0x13).
Fix applied: Split the non-finalized input block so that 0x02-0x06 are written
first, then 0x07-0x08 (FinalScriptSig/Witness), then v2 fields 0x0e-0x12, then
taproot fields 0x13-0x18. Test: TestV2InputSerializationKeyOrder.
Spec Compliance Issues (FIXED)
3. Missing locktime value validation (partial_input.go)
BIP-370 mandates:
PSBT_IN_REQUIRED_TIME_LOCKTIME: value must be >= 500,000,000PSBT_IN_REQUIRED_HEIGHT_LOCKTIME: value must be > 0 and < 500,000,000
The deserialization code accepted any uint32 value without validation.
Fix applied: Added range checks after reading the values. Tests:
TestV2TimeLocktimeMustBeGTE500M, TestV2TimeLocktimeBoundary,
TestV2HeightLocktimeMustBeGTZeroAndLT500M, TestV2HeightLocktimeValidBoundary.
4. No duplicate detection for v2 input fields (partial_input.go)
OutputIndex, TimeLocktime, HeightLocktime, and Sequence had no duplicate
key detection.
Fix applied: Added outputIndexSeen, sequenceSeen, timeLocktimeSeen,
heightLockSeen booleans. Tests: TestV2DuplicateInput*.
5. FallbackLocktime and TxModifiable duplicate detection (psbt.go)
These used if value != 0 for duplicate detection instead of dedicated booleans
like txVersionSeen.
Fix applied: Added fallbackLocktimeSeen and txModifiableSeen booleans.
Tests: TestV2DuplicateGlobalFallbackLocktime, TestV2DuplicateGlobalTxModifiable.
6. PSBT_OUT_AMOUNT type: signed vs unsigned (partial_output.go)
BIP-370 specifies a signed 64-bit integer. The struct used uint64.
Fix applied: Changed POutput.Amount from uint64 to int64 throughout,
matching wire.TxOut.Value. Test: TestV2AmountSignedInt64.
Design Issues (Not Fixed)
7. V2-specific fields not rejected in v0 PSBTs
BIP-370 says input fields 0x0e-0x12 and output fields 0x03-0x04 must be excluded
from v0. Currently they are silently parsed. A strict implementation would route
them to the unknown list for v0. This is left as-is for now since the version is
not yet known at per-input/per-output parsing time (it's a global field that comes
before the inputs/outputs).
8. NewV2 doesn't validate TxVersion (creator.go:72)
New() checks version >= MinTxVersion but NewV2() accepts any value. BIP-370
says "the transaction version number must be set to at least 2" when others will
add inputs/outputs.
9. AddInputV2/AddOutputV2 don't check TxModifiable flags
BIP-370 says the Constructor must check PSBT_GLOBAL_TX_MODIFIABLE before adding
inputs/outputs. The library doesn't enforce this, leaving it to callers.
10. Optional fields always serialized (psbt.go:666-700)
FallbackLocktime and TxModifiable are always written for v2, even when 0. Per
BIP-370 these are optional with a default of 0. Writing them is valid but verbose.
11. Silent TxVersion upgrade (psbt.go:567-569)
If txVersion=0 is explicitly present in a v2 PSBT, it's silently upgraded to 2.
This could mask invalid input data.
|
Appreciate the detailed review. I’m already on it I'll clean up the generated code, verify the fixes thoroughly, and make a proper push once everything checks out. |
Implement the core PSBTv2 packet handling including: - PSBTv2 serialization with proper field ordering - BIP-370 lock time determination algorithm - Version detection and validation - Unknown field handling with duplicate detection
Add PSBTv2 support to the complete PSBT workflow pipeline: - PSBTv2 packet creation with proper initialization - Update operations with version-aware field handling - Signing operations with PSBTv2 compatibility - Transaction extraction with version detection - Future-proof version check patterns
Add comprehensive PSBTv2 field management for inputs and outputs: - PSBTv2 input fields: PreviousTxid, OutputIndex, Sequence, Locktimes - PSBTv2 output fields: Amount, Script - Proper field serialization with BIP-174 ordering - Field copying and preservation during finalization - Unknown field handling with duplicate detection
Add PSBTv2-aware finalization logic: - Preserve required PSBTv2 fields during finalization - Maintain unknown fields as mandated by BIP-370 - Proper field copying with CopyInputFields method
Add complete test coverage for PSBTv2 implementation: - Lifecycle tests (creation, update, finalization, extraction) - Lock time determination algorithm validation - Field validation and serialization round-trips - BIP-370 compliance verification - Unknown field handling tests
a91e2e5 to
ef4954d
Compare
Summary
This PR adds full PSBTv2 support to the
btcutil/psbtpackage, implementing BIP-370.Fixes #2328.
This is also a prerequisite for full BIP-375 compliance in PR #2244 (Silent Payments), as noted by @benma.
Changes
New Global Fields (PSBTv2)
TxVersion(0x02): Transaction versionFallbackLocktime(0x03): Fallback locktimeInputCount(0x04): Number of inputsOutputCount(0x05): Number of outputsTxModifiable(0x06): Modifiability bitfieldNew Per-Input Fields
PreviousTxid(0x0E): Previous TXIDOutputIndex(0x0F): Previous output indexSequence(0x10): Sequence numberTimeLocktime(0x11): Time-based locktimeHeightLocktime(0x12): Height-based locktimeNew Per-Output Fields
Amount(0x03): Output valueScript(0x04): Output scriptPubKeyBIP Compliance
Tests Added
TestPsbtV2LifeCycle: Full create → sign → finalize → extract cycleTestPsbtV2Validation: Invalid V2 packets correctly rejectedTestPsbtV2Counts: InputCount/OutputCount round-tripTestPsbtV2Locktimes: Locktime validation edge casesTest Results
All existing tests continue to pass. No breaking changes to the V0 API.