DecodeHexTx: Try case where txn has inputs first

[instagibbs]

Alternative/complementary to #17773 to avoid random decoderawtransaction failures. Most cases this is used now is on complete transactions, especially with the uptake of PSBT.

[maflcko]

Yeah, I wanted to do the same, but there was no rationale on why no_witness is tried first, so I didn't touch the code. Concept ACK.

[sipa]

This is incorrect I think. If you swap the order, the "!try_witness" needs to turn into a "!try_no_witness" on the other side.

[instagibbs]

@sipa you mean here? Why would that make sense? We're inside a try_no_witness block already

[sipa]

No, in the other clause. It's supposed to bypass the sanity check when only one of try_witness and try_no_witness is set.

[instagibbs]

ah I see, fixed it up(within my understanding) and put a quick comment there for future readers' comprehension

[practicalswift]

While touching this file and DecodeHexTx: what about separating the tx decoding from the unrelated hex decoding? Such as having say a DecodeTx do the decoding which DecodeHexTx would call.

When fuzzing the tx decoding part of this code it would be nice to not having to waste CPU cycles on hex encoding followed by immediate hex decoding for no reason at all :)

[practicalswift]

The interaction between the parameters bool try_no_witness and bool try_witness might surprise callers (both before and after this change). Can that be improved?

For the casual observer it is easy to think that:

• DecodeHexTx(mtx, tx_hex, false, true) == true implies DecodeHexTx(mtx, tx_hex, true, true) == true, and
• DecodeHexTx(mtx, tx_hex, true, false) == true implies DecodeHexTx(mtx, tx_hex, true, true) == true.

Which is not the case :)

[instagibbs]

@practicalswift trying to touch as little as possible for this change, but yes it's something that should certainly be made simpler

[ajtowns]

Concept ACK; code looks right to me, and it fixes #18028.

But in primitives/transaction.h SerializeTransaction we'll still do non-extended format for unfunded partial transactions with no inputs, which could still lead to us spitting out a hex representation that we misparse when reading it back in. So it might be sensible to also change:

--- a/src/primitives/transaction.h
+++ b/src/primitives/transaction.h
@@ -222,7 +222,7 @@ inline void UnserializeTransaction(TxType& tx, Stream& s) {
         for (size_t i = 0; i < tx.vin.size(); i++) {
             s >> tx.vin[i].scriptWitness.stack;
         }
-        if (!tx.HasWitness()) {
+        if (!tx.HasWitness() && !tx.vin.empty()) {
             /* It's illegal to encode witnesses when all witness stacks are empty. */
             throw std::ios_base::failure("Superfluous witness record");
         }
@@ -244,10 +244,14 @@ inline void SerializeTransaction(const TxType& tx, Stream& s) {
     if (fAllowWitness) {
         /* Check whether witnesses need to be serialized. */
         if (tx.HasWitness()) {
             flags |= 1;
         }
+        /* If non-extended format might be ambiguous, use extended format */
+        if (tx.vin.empty()) {
+            flags |= 1;
+        }
     }
     if (flags) {
         /* Use extended format in case witnesses are to be serialized. */
         std::vector<CTxIn> vinDummy;
         s << vinDummy;

[instagibbs]

@ajtowns I'd rather not touch serialization code for this if possible since it's already hard enough to get review :)

[ajtowns]

I tried writing a fuzzer to see if reasonable partial txs with one output and no inputs would be incorrectly decoded as a witness tx. There are a few, but they seem reasonably unlikely (requiring the only output to pay to a non-compressed, non-hashed pubkey, or an OP_RETURN). One more plausible example is: 00000000000101000000000000002a51280302000002000000000001000000000400000300022800020000050000007f00030002280002000000000000 which breaks down as:

version 00000000
vin 00
vout 01
  value [01]00000000000000
  spk [2a] [51 28020001000000f8ff00f700000037ff00ff23ae02ae00][00000001][0100][000000000][00][6000000000000]
locktime 00000000

version 00000000
flag 0001
vin [01]
  txid 00000000000000[2a][5128020001000000f8ff00f700000037ff00ff23ae02ae00]
  n [00000001]
  scriptSig [01 00]
  nSequence [00000000]
vout [00]
witness [06000000000000]
locktime 00000000

In particular, the vout's nValue needs to decode to a plausible number of vins, the script pubkey has to decode as a plausible scriptSig, vout array and witness stack, and the scriptSig and any pubkeys in vout need to have valid ops to pass the sanity check. That seems reasonably unlikely, something lke 8 in 256**5 maybe, rather the 1 in 256ish chance seen in #18028.

As the PR desc notes, for any cases where a partial tx is incorrectly decoded as a witness tx, using a PSBT instead will avoid the problem.

[EDIT: dropped ACK, need to check what happens if valid witness tx that would have decoded as non-witness is included in block first]

[ajtowns]

ACK 27fc6a3

A valid witness tx (on my regtest anyway) which decodes incorrectly as a 0-in 1-out non-witness tx is 020000000001013f4977dd2c309fac4339d4396fe269d6d2d111c8ac64a876e8ac7e18a2ea6f3d0000000000ffffffff0120e2e76153010000160014352067b30b49447ad453215ffa023e8e61f511f202473044022034bb381d70480932d8e13a20512b9be7f364187e083eb2583e0b5b128cbfba3c022047d593ebd6e3ddf1d408baae60eb30fbf7383c7813f90aa3236da32fae22813e01210327152f1c320a6a3a48cd5912b85081db3b6ae0ba384e9cf389071184953734a700000000. Currently "sendrawtransaction" won't accept it, however if it's mined in a block it will be correctly decoded, so (unsurprisingly) fixing this doesn't result in an obvious hardfork.

That tx was constructed by:

• finding a pubkey that's a valid script (eg, "03 __ __ __ 1c ...")
• grinding a tx with an output that pays to that pubkey via p2wpkh, and whose txid ends in "43 ac __ __ __ __ __ __ __"
• making a 1-input 1-output tx spending that output

That's a 1-in-16M chance of occurring accidently, but requires only 66k sha256d operations to find.

[achow101]

ACK 27fc6a3

If we dropped the *rawtransaction RPCs (except for sendrawtransaction) and had everyone use PSBTs, this problem would go away :)