Segwit has added ambiguity for 0-input transactions (the 0 can be read as the dummy byte indicating that this is a segwit transcation).
bitcoin-tx needs to be able to parse 0-input partial transactions. This PR causes bitcoin-tx to call DecodeHexTx() with the fTryNoWitness flag set, so we attempt to deserialize the transaction as a pre-segwit transaction.
This PR also adds new test cases to verify that bitcoin-tx parses a partial transaction correctly.
adding my comment here instead of on the files changed tab
Is this 01000000000100000000000000000000000000 supposed to be parsed as a zero input, one output to 0x00 transaction?
001000000
100
201
3 0000000000000000
4 00
500000000
Wouldn’t 010000000001000000000000 (which currently fails) be more suitable?
001000000
100
201
300
400
500000000
Following 01000000000000000000 behavior.
Unless there’s another reason for using that one.
@fivepiece , I believe your second transaction is invalid. Commented transactions:
01000000000100000000000000000000000000 is a valid partial transaction with 0 inputs and 1 output:
001000000 // version (4 bytes)
100 // number of inputs (varint - 1 byte if value is 0)
201 // number of outputs (varint - 1 byte if value is 1)
3 0000000000000000 // output 1 amount (8 bytes)
4 00 // output 1 scriptSig size (varint - 1 byte if value is 0)
500000000 // nlocktime (4 bytes)
010000000001000000000000 is invalid and doesn’t parse:
001000000 // version (4 bytes)
100 // number of inputs (varint - 1 byte if value is 0)
201 // number of outputs (varint - 1 byte if value is 1)
3 00 // output 1 amount !!INVALID SHOULD BE 8 BYTES!!
4 00 // etc
500000000
(in fact it’d be partially parsed as):
001000000 // version (4 bytes)
100 // number of inputs (varint - 1 byte if value is 0)
201 // number of outputs (varint - 1 byte if value is 1)
3 000000000000.... // output 1 amount (!!INVALID expects 8 bytes - only 6 read!!)
4...
Your other example 01000000000000000000 is valid as a partial transaction with 0 inputs and 0 outputs:
001000000 // version (4 bytes)
100 // number of inputs (varint - 1 byte if value is 0)
200 // number of outputs (varint - 1 byte if value is 0)
300000000 // nlocktime (4 bytes)
@jnewbery I guess I should clarify my question then. I was wondering why 01000000000100000000000000000000000000 was given as a test case at all, as it seems very specific.
My understanding was that you were out to implement a “blank” segwit transaction in that test, and I think 010000000001000000000000 is a better representation, because it’s very similar to the non-segwit bank tx 01000000000000000000, only with the addition of the marker and flag.
[edit]
I agree that 010000000001000000000000 currently fails parsing, but I think that it shouldn’t. :)
Correct. If none of the inputs have witnesses or if the witnesses are all empty, then there shouldn’t be a witness structure attached to the transaction
From transaction.h:
0 /* It's illegal to encode a witness when all vtxinwit entries are empty. */
1 throw std::ios_base::failure("Superfluous witness record");
In this case there are no transaction inputs, so no witnesses, and so the witness dummy byte and flag shouldn’t be set.
Right re. transaction.h, also I’ve asked a related question about this in -dev yesterday. Initially I read that comment as a reference to actual witness data (sigs, scripts), but I see the point of not needing segwit markers at all for no input transactions.
Thanks for clearing it up.
I don’t like “try to parse the transaction as pre-segwit transaction” much - This implies ambiguity, can the ambiguity cause problems here?
If so I’d prefer if this was an explicit flag somewhere.
I’ll paste this here as well, commented in #-core-dev
so specifically re. parsing partial transactions as pre-segwit (#8837), I actually hit this issue in my own toy parser and it shows in core too: http://paste.debian.net/plainh/8c80d8cd so having some flag would be great
bitcoin-tx and bitcoin-cli
The issue is that segwit only guarantees unambiguous encoding for valid complete transactions. In places where raw transactions are expected that consume inputs that are not necessarily complete, ambiguity is possible. This is the case for the decoderawtransaction and fundrawtransaction RPCs, and for bitcoin-tx. Since inputs to those are usually in fact not signed, they should usually not have any witnesses at all, and thus (trying to) decode without seems like a reasonable default.
I do agree it’s undesirable to have ambiguity in the first place, but I think a longer term solution will include a separate encoding for incomplete transactions. We may need that anyway for more complex signing such as for MAST and Schnorr, where the communication between the participants may include (primarily) information that does not end up in the final transaction.
I do agree it’s undesirable to have ambiguity in the first place, but I think a longer term solution will include a separate encoding for incomplete transactions.
Not only a separate encoding, but it needs to be distinguishable non-ambiguously from completed transactions.
The conversation in this PR seems to have wandered a bit into the desirability of having a separate encoding for partial transactions. I agree that’s a good idea, but it seems out of scope for this PR.
To clarify what this PR achieves:
bitcoin-tx and bitcoin-cli decoderawtransaction were able to parse partial transactionsdecoderawtransaction was updated to attempt to parse partial transactions here: https://github.com/bitcoin/bitcoin/commit/7030d9eb47254499bba14f1c00abc6bf493efd91#diff-01aa7d1d32f1b9e5a836c9c411978918L490 . bitcoin-tx was not updated at the same time.bitcoin-tx as was made to decoderawtransaction and adds test cases. This restores the pre-v0.13.1 behaviour and brings bitcoin-tx in line with decoderawtransaction
Restore pre V0.13.1 functionality to bitcoin-tx and allow it to parse 0-input partial transactions.
Labels
Utils/log/libs