DecodeHexTx: Try case where txn has inputs first

instagibbs commented at 3:35 pm on December 19, 2019: member

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.

MarcoFalke commented at 4:10 pm on December 19, 2019: member

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 commented at 4:29 pm on December 19, 2019: member

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.

in src/core_read.cpp:136 in 3f33f027e1 outdated

137+    if (try_no_witness) {
138+        CDataStream ssData(txData, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS);
139         try {
140             ssData >> tx;
141-            if (ssData.empty()) {
142+            if (ssData.eof() && (!try_witness || CheckTxScriptsSanity(tx))) {

instagibbs commented at 4:32 pm on December 19, 2019:

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

sipa commented at 4:34 pm on December 19, 2019:

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 commented at 4:45 pm on December 19, 2019:

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

DecodeHexTx: Try case where txn has inputs first 6020ce3c01

instagibbs force-pushed on Dec 19, 2019

practicalswift commented at 5:01 pm on December 19, 2019: contributor

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 commented at 8:25 pm on December 19, 2019: contributor

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 :)

in src/core_read.cpp:112 in 65445e7276 outdated

108@@ -109,28 +109,23 @@ static bool CheckTxScriptsSanity(const CMutableTransaction& tx)
109     return true;
110 }
111 
112-bool DecodeHexTx(CMutableTransaction& tx, const std::string& hex_tx, bool try_no_witness, bool try_witness)
113+bool DecodeTx(CMutableTransaction& tx, const std::vector<unsigned char> tx_data, bool try_no_witness, bool try_witness)

promag commented at 12:02 pm on December 20, 2019:

nit, static.

practicalswift commented at 1:57 pm on December 20, 2019:

@promag I want to be able to fuzz DecodeTx directly without going via DecodeHexTx :)

promag commented at 2:03 pm on December 20, 2019:

Oh in that case dismiss my suggestion!

promag commented at 9:43 pm on December 22, 2019:

@instagibbs make tx_data reference.

promag commented at 9:46 pm on December 22, 2019:

@practicalswift on a second though it could be static for now and then you remove it and maybe also declare it in the header.

instagibbs commented at 6:24 pm on December 30, 2019:

done

instagibbs commented at 6:24 pm on December 30, 2019:

done

DecodeHexTx: Break out transaction decoding logic into own function 27fc6a38f8

instagibbs force-pushed on Dec 30, 2019

instagibbs commented at 6:27 pm on December 30, 2019: member

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

ajtowns commented at 5:09 am on January 31, 2020: member

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:

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

laanwj added the label RPC/REST/ZMQ on Mar 12, 2020

laanwj added the label Bug on Mar 12, 2020

instagibbs commented at 6:54 pm on August 10, 2020: member

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

MarcoFalke added this to the milestone 0.22.0 on Aug 11, 2020

MarcoFalke removed this from the milestone 0.22.0 on Aug 11, 2020

MarcoFalke added this to the milestone 0.21.0 on Aug 11, 2020

ajtowns commented at 4:31 am on October 5, 2020: member

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:

 0version 00000000
 1vin 00
 2vout 01
 3  value [01]00000000000000
 4  spk [2a] [51 28020001000000f8ff00f700000037ff00ff23ae02ae00][00000001][0100][000000000][00][6000000000000]
 5locktime 00000000
 6
 7version 00000000
 8flag 0001
 9vin [01]
10  txid 00000000000000[2a][5128020001000000f8ff00f700000037ff00ff23ae02ae00]
11  n [00000001]
12  scriptSig [01 00]
13  nSequence [00000000]
14vout [00]
15witness [06000000000000]
16locktime 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 commented at 9:09 am on October 5, 2020: member

ACK 27fc6a38f813b65e5110c77925a335214aec756a

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 commented at 9:52 pm on October 13, 2020: member

ACK 27fc6a38f813b65e5110c77925a335214aec756a

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

laanwj merged this on Oct 15, 2020

laanwj closed this on Oct 15, 2020

sidhujag referenced this in commit c5573afce9 on Oct 16, 2020

decryp2kanon referenced this in commit ae823054db on Nov 3, 2020

DrahtBot locked this on Feb 15, 2022

DecodeHexTx: Try case where txn has inputs first #17775