Fix segwit transaction parsing in PSBT#5684
Conversation
In PSBT, the unsigned transaction must always be encoded in a non-SegWit format. Serialised PSBT transactions with a segwit encoded unsigned tx should be rejected as invalid, as demonstrated by invalid PSBT test vector 19 from BIP-174. Adjust PSBT global map parsing to reject segwit unsigned transactions. Introduce a PSBT error variant for segwit unsigned transactions. Add test vector 19 to psbt module tests.
|
I haven't reviewed yet but I wonder if this should be done against |
Sounds good to me. Maybe the metaphorical slap could be removing the line indicating PSBT support from the top of the root README? |
|
To discuss the dev path of PSBT I wrote this: #5700 |
| // serialization (0x0001 after the version). Bytes there | ||
| // could mean 0 inputs + 1 output or segwit. Since we already | ||
| // decoded 0 inputs successfully we can assume a failure to | ||
| // decode outputs is actually segwit serialisation. |
There was a problem hiding this comment.
In 040d44e
I don't buy this assumption. I feel this just makes the error reporting more convoluted and confusing.
There was a problem hiding this comment.
Does the unit test you provided actually fail without this change?
There was a problem hiding this comment.
Does the unit test you provided actually fail without this change?
Not exactly. It fails to parse either way, but the error before is due to an out of range output amount value. I could try and put together another test case that parses successfully but uses segwit if you'd prefer.
There was a problem hiding this comment.
Well, there will not exist any test case that passes with the old code and fails with the new, because you haven't changed any error paths, just the way that the error is constructed.
I wouldn't mind expanding the output-parsing error (and the locktime-parsing error) to say something like "this might be caused by putting a segwit-encoded transaction into a PSBT". But we can't assume this is the case, and it seems kinda unlikely to me given that there shouldn't be any software out there that produces PSBTs like this.
There was a problem hiding this comment.
Woops. Sorry. It's been a bit since I actually looked over this.
If a segwit encoded transaction parses as valid, then we can't discount it because 0-in 1-out is a valid transaction format, and as you said a failure to parse is equally likely to be something like a bad amount value in an output as it is to be an invalid encoding for the whole transaction.
Should I just close this then? Or is adding to the docs on the error/function to clarify the (slim) possibility of an unexpected segwit tx encoding an acceptable middle-ground?
|
Closed because it's an incorrect solution to a largely non-existent issue. See the discussion above: #5684 (comment) |
4 participants
In PSBT, the unsigned transaction must always be encoded in a non-SegWit format (see BIP-174). Serialised PSBT transactions with a segwit encoded unsigned tx should be rejected as invalid, as demonstrated by invalid PSBT test vector 19 from BIP-174.
Adjust PSBT global map parsing to reject segwit unsigned transactions.
Introduce a PSBT error variant for segwit unsigned transactions.
Add test vector 19 to psbt module tests.