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?

01000000
00
01
  0000000000000000
  00
00000000

Wouldn't 010000000001000000000000 (which currently fails) be more suitable?

01000000
00
01
00
00
00000000

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:

01000000 // version (4 bytes)
00 // number of inputs (varint - 1 byte if value is 0)
01 // number of outputs (varint - 1 byte if value is 1)
  0000000000000000 // output 1 amount (8 bytes)
  00 // output 1 scriptSig size (varint - 1 byte if value is 0)
00000000 // nlocktime (4 bytes)

010000000001000000000000 is invalid and doesn't parse:

01000000 // version (4 bytes)
00 // number of inputs (varint - 1 byte if value is 0)
01 // number of outputs (varint - 1 byte if value is 1)
  00 // output 1 amount !!INVALID SHOULD BE 8 BYTES!!
  00 // etc
00000000

(in fact it'd be partially parsed as):

01000000 // version (4 bytes)
00 // number of inputs (varint - 1 byte if value is 0)
01 // number of outputs (varint - 1 byte if value is 1)
  000000000000.... // output 1 amount (!!INVALID expects 8 bytes - only 6 read!!)
...

Your other example 01000000000000000000 is valid as a partial transaction with 0 inputs and 0 outputs:

01000000 // version (4 bytes)
00 // number of inputs (varint - 1 byte if value is 0)
00 // number of outputs (varint - 1 byte if value is 0)
00000000 // 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. :)

Anyway thinking about this some more, feel free to disregard my suggestion. It's too ambiguous. Maybe the correct way is to fall back to parsing a non segwit transaction, as no inputs mean no witnesses too.

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:

            /* It's illegal to encode a witness when all vtxinwit entries are empty. */
            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.

@laanwj - this very small change allows bitcoin-tx to parse partial transactions and includes a test case to cover the new functionality. Are you happy to merge this?

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

• I should add that with this PR the transaction from the pastebin is parsed as a zero input pay to multisig on both 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.

Right ^, the difficulty is a 0 inputs, unfunded transaction looks a lot like segwit at first, then you might fail after it stops making sense, or like in the case I linked, maybe never fail.. unless there was a context to what this transaction was passed in for. (just my personal experience parsing partial 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:

• pre V0.13.1, both bitcoin-tx and bitcoin-cli decoderawtransaction were able to parse partial transactions
• V0.13.1 introduced ambiguity in encoding for a 0-input transactions. However, as @sipa notes above: "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."
• decoderawtransaction was updated to attempt to parse partial transactions here: 7030d9e#diff-01aa7d1d32f1b9e5a836c9c411978918L490 . bitcoin-tx was not updated at the same time.
• this PR makes the same change to 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

Yes, fair enough, it's an improvement.
utACK 7451cf5