[RPC] Fix decoderawtransaction decoding of segwit txs

achow101 commented at 3:08 am on January 12, 2017: member

Makes decoderawtransaction check for the marker and flag bytes present in segwit transactions and decodes them as such in order to avoid situations where segwit transactions are decoded as non-segwit transactions.

As per IRC discussion: https://botbot.me/freenode/bitcoin-core-dev/2017-01-12/?msg=79232896&page=1

Edit: Instead of checking for the bytes, it will just decode the transactions with the segwit formatting as that is the same behavior.

achow101 force-pushed on Jan 12, 2017

jonasschnelli added the label RPC/REST/ZMQ on Jan 12, 2017

MarcoFalke commented at 10:01 am on January 12, 2017: member

fundrawtransaction.py fails on travis

achow101 commented at 3:16 pm on January 12, 2017: member

It is failing because the transaction is a 0 input transaction which is being interpreted as a segwit transaction. As per the discussion on IRC, not being able to decode 0 input transactions is acceptable behavior.

sdaftuar commented at 8:39 pm on February 7, 2017: member

I know I suggested on IRC that we just let decoderawtransaction() fail on transactions with no inputs, but perhaps not working at all on 0-input transactions is a bigger regression than necessary?

I believe we only have ambiguity in the case where something might deserialize into a 0-input, 1-output transaction when witness serialization is disabled. So perhaps we should just assume the segwit deserialization if a transaction can be decoded both ways (not perfect, but the best we can do for now, I think), and otherwise allow for 0-input transactions?

Specifically: I think we can leave the existing call to DecodeHexTx unchanged (ie, pass in true as the last argument), and then add an additional check that, if mtx is a 0-input transaction, try to decode again with that last boolean set to false (to force segwit deserialization), and if it succeeds, prefer the segwit deserialization over the 0-input one.

We should document this behavior as well…

achow101 commented at 9:06 pm on February 7, 2017: member

I suppose that would work as well. It wouldn’t actually be checking for just 0 inputs but rather the 0x0001 flag that signals for segwit, so not all 0-input txs would have to go through both desers.

achow101 force-pushed on Feb 7, 2017

achow101 commented at 10:06 pm on February 7, 2017: member

I rebased and made @sdaftuar ’s suggestion. I can squash if this will be merged as this may not yet be the best solution.

Also, it still fails fundrawtransaction.py, at this test: https://github.com/bitcoin/bitcoin/blob/master/qa/rpc-tests/fundrawtransaction.py#L588. I don’t know what to do about that.

luke-jr commented at 9:34 am on February 16, 2017: member

What does this actually fix? DecodeHexTx already tries both deserializations, and only accepts them if the entire data is consumed…

jnewbery commented at 11:40 pm on February 16, 2017: member

I’ve tested this and it doesn’t appear to solve the originally reported problem in https://bitcointalk.org/index.php?topic=1748353.msg17477509#msg17477509 :

 0bitcoin-cli decoderawtransaction "02000000000101c564a62f94c025ac80137817d8658aabceaaad30412facecba1bd2255182e1c40000000000ffffffff01ca124c000000000016001443aac20a116e09ea4f7914be1c55e4c17aa600b7024730440220679eaf5e41eee49b38f3112ec90b49a655db21677db4d7fa80de67aeb987161102204024b85730fc106d0b870623fc221946e624413363eb627fe90eb5047d35565c012103335134d7414e1d1a154600b124a96f5ef2c6ca21434d2622469a96bd5262fd5600000000"
 1{
 2  "txid": "54ad9b3b24064c4033814ddc712a393cfa4e011eb7ac24374687511cd056eac5",
 3  "hash": "54ad9b3b24064c4033814ddc712a393cfa4e011eb7ac24374687511cd056eac5",
 4  "size": 191,
 5  "vsize": 191,
 6  "version": 2,
 7  "locktime": 0,
 8  "vin": [
 9  ],
10  "vout": [
11    {
12      "value": 27203371073.07775233,
13      "n": 0,
14      "scriptPubKey": {
15        "asm": "OP_LEFT 7817d8658aabceaaad30412facecba1bd22551 OP_SIZE OP_UNKNOWN OP_UNKNOWN 0 0 0 0 0 OP_INVALIDOPCODE OP_INVALIDOPCODE OP_INVALIDOPCODE OP_INVALIDOPCODE -74 4c000000000016001443aac20a116e09ea4f OP_PICK be1c55e4c17aa600b7024730440220679eaf5e41 OP_UNKNOWN OP_UNKNOWN OP_BOOLOR f3112ec90b49a655db21677db4d7fa80de67aeb987161102204024b85730fc106d0b870623fc221946e624413363eb627fe90eb5047d3556 12 33 3428659 OP_UNKNOWN [error]",
16        "hex": "80137817d8658aabceaaad30412facecba1bd2255182e1c40000000000ffffffff01ca124c000000000016001443aac20a116e09ea4f7914be1c55e4c17aa600b7024730440220679eaf5e41eee49b38f3112ec90b49a655db21677db4d7fa80de67aeb987161102204024b85730fc106d0b870623fc221946e624413363eb627fe90eb5047d35565c012103335134d7414e1d1a154600b124a96f5ef2c6ca21434d2622469a96bd5262fd56",
17        "type": "nonstandard"
18      }
19    }
20  ]
21}

am I doing something wrong?

achow101 commented at 0:26 am on February 17, 2017: member

You definitely did something wrong. It worked fine for me:

 0decoderawtransaction 02000000000101c564a62f94c025ac80137817d8658aabceaaad30412facecba1bd2255182e1c40000000000ffffffff01ca124c000000000016001443aac20a116e09ea4f7914be1c55e4c17aa600b7024730440220679eaf5e41eee49b38f3112ec90b49a655db21677db4d7fa80de67aeb987161102204024b85730fc106d0b870623fc221946e624413363eb627fe90eb5047d35565c012103335134d7414e1d1a154600b124a96f5ef2c6ca21434d2622469a96bd5262fd5600000000
 1
 2{
 3  "txid": "a24cec50d5cf861d1af4b634f8ed1968c0e9484724bfef5af7f8c383605978c8",
 4  "hash": "54ad9b3b24064c4033814ddc712a393cfa4e011eb7ac24374687511cd056eac5",
 5  "size": 191,
 6  "vsize": 110,
 7  "version": 2,
 8  "locktime": 0,
 9  "vin": [
10    {
11      "txid": "c4e1825125d21bbaecac2f4130adaaceab8a65d817781380ac25c0942fa664c5",
12      "vout": 0,
13      "scriptSig": {
14        "asm": "",
15        "hex": ""
16      },
17      "txinwitness": [
18        "30440220679eaf5e41eee49b38f3112ec90b49a655db21677db4d7fa80de67aeb987161102204024b85730fc106d0b870623fc221946e624413363eb627fe90eb5047d35565c01", 
19        "03335134d7414e1d1a154600b124a96f5ef2c6ca21434d2622469a96bd5262fd56"
20      ],
21      "sequence": 4294967295
22    }
23  ],
24  "vout": [
25    {
26      "value": 0.04985546,
27      "n": 0,
28      "scriptPubKey": {
29        "asm": "0 43aac20a116e09ea4f7914be1c55e4c17aa600b7",
30        "hex": "001443aac20a116e09ea4f7914be1c55e4c17aa600b7",
31        "type": "witness_v0_keyhash"
32      }
33    }
34  ]
35}

sipa commented at 0:46 am on February 17, 2017: member

I don’t understand what this PR is doing. As @luke-jr points out, DecodeHexTx always tries both deserializations (in fact, everything always tries both)… but it prefers the non-witness one when fTryNoWitness is passed, which is the case. If we want it to prefer the witness version in case of ambiguity, we should just pass false.

sipa commented at 0:55 am on February 17, 2017: member

Oh, I see… you’re looking at the hex code in order to determine the number of txins. Can you instead look at tx.vin.size() of the decode?

achow101 commented at 1:42 am on February 17, 2017: member

We discussed this on IRC a while ago. The issue is with 0-input transactions, specifically with witness transactions being interpreted as non-withess 0 input transactions. This PR is to make it prefer the witness serialization over a non-witness for 0-input transactions but maintain the original behavior for other transactions.

achow101 force-pushed on Feb 17, 2017

sipa commented at 1:56 am on February 17, 2017: member

@achow101 Isn’t that just identical to passing false to DecodeHexTx? The only case in which the decoding is ambiguous is where the non-witness transaction has 0 inputs.

achow101 commented at 2:11 am on February 17, 2017: member

@sipa I don’t think so. As I understand it, when you pass false to DecodeHexTx it will interpret a non-witness 0-input transaction as a witness transaction and thus fail to decode it. What this does is that such transactions will be decoded properly on the first pass with true being passed in so as to still be able to decode non-witness 0-input transactions instead of failing with decoding them as witness transactions.

in src/rpc/rawtransaction.cpp: in a992d4fbc3 outdated

530 
531-    if (!DecodeHexTx(mtx, request.params[0].get_str(), true))
532+    if (!DecodeHexTx(mtx, request.params[0].get_str(), true)) {
533         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
534+    }
535+    if (request.params[0].get_str().compare(8, 4, "0001") == 0) {

sipa commented at 2:16 am on February 17, 2017:

I think this can be written as if (mtx.vin.size() == 0) { instead.

sipa commented at 2:17 am on February 17, 2017: member

@achow101 I see, of course. The deserialization code doesn’t actually try two combinations.

achow101 force-pushed on Feb 17, 2017

achow101 commented at 2:22 am on February 17, 2017: member

I’m baffled as to why the tests magically pass now with @sipa’s change.

luke-jr commented at 6:13 am on February 17, 2017: member

It’s no longer checking vout.size()==1? (which it probably ought to..)

sipa commented at 6:19 am on February 17, 2017: member

A transaction with 0 inputs and 2 outputs can be mistaken for an (invalid) extended transaction as well.

achow101 commented at 1:58 pm on February 17, 2017: member

A transaction with 0 inputs and 2 outputs can be mistaken for an (invalid) extended transaction as well.

Right, but that shouldn’t be a problem now because it will properly decode as non-witness, fail to decode as witness and thus fallback to non-witness decoding.

jnewbery commented at 4:14 pm on February 17, 2017: member

Apologies. I had an old bitcoind in my path. I’ve retested and this now works.

I can explain why fundrawtransaction passes. It’s nothing to do with the change in decoderawtransaction. You’ve rebased off master and picked up #9650, which causes deserialization to fail if there are junk bytes at the end of a transaction. The test was failing on:

decoderawtransaction "0100000000010000000000000000066a047465737400000000"

With this PR and without #9650, we’d:

deserialize with fTryNoWitness set to True, and succeed in deserializing a non-witness 0-in,1-out transaction
deserialize with fTryNoWitness set to False, and succeed in deserializing a witness 0-in,0-out transaction and ignoring the trailing junk bytes
return the witness 0-in,0-out transaction

With #9650 we:

deserialize with fTryNoWitness set to True, and succeed in deserializing a non-witness 0-in,1-out transaction
deserialize with fTryNoWitness set to False, and fail because of the trailing junk bytes
return the witness 0-in,1-out transaction

(see - no magic :smile: )

A couple of general comments:

Can you add a code comment above the new code in decoderawtransaction() explaining why we’re deserializing two ways? It’s quite subtle, so some explanation here would be good.
Can you add the same fix to bitcoin-tx (src/bitcoin-tx.cpp)? It’s a pain, but while we have two tools for signing/decoding transactions, we should try to keep them aligned.

achow101 commented at 4:22 pm on February 17, 2017: member

@jnewbery Ah, that makes sense now.

Can you add a code comment above the new code in decoderawtransaction() explaining why we’re deserializing two ways? It’s quite subtle, so some explanation here would be good.

Sure.

Can you add the same fix to bitcoin-tx (src/bitcoin-tx.cpp)? It’s a pain, but while we have two tools for signing/decoding transactions, we should try to keep them aligned.

I assume this should go in around the only line that has DecodeHexTx()?

jnewbery commented at 4:29 pm on February 17, 2017: member

I assume this should go in around the only line that has DecodeHexTx()?

I think it needs a bit more thought than that. bitcoin-tx combines the functionality of createrawtransaction, signrawtransaction and decoderawtransaction. With your change here, the deserialization logic is different for those different RPCs. Should we try to mirror that in bitcoin-tx?

achow101 force-pushed on Feb 17, 2017

luke-jr commented at 10:46 pm on February 18, 2017: member

@achow101 This doesn’t even build now…

achow101 force-pushed on Feb 18, 2017

achow101 commented at 10:53 pm on February 18, 2017: member

whoops. Bad copy-paste. Fixed that and rebased onto master.

achow101 force-pushed on Feb 18, 2017

luke-jr commented at 11:27 pm on February 18, 2017: member

Better if you could rebase on 9828f9a9962c1bee5c343847030b9cfd87a40a5e

achow101 force-pushed on Feb 18, 2017

achow101 commented at 6:08 pm on March 2, 2017: member

It would be nice (perhaps necessary?) to have this in by the time segwit activates.

in src/rpc/rawtransaction.cpp: in 202c0dcdd5 outdated

531-    if (!DecodeHexTx(mtx, request.params[0].get_str(), true))
532+    // First attempt to decode the transaction as a non-witness transaction
533+    if (!DecodeHexTx(mtx, request.params[0].get_str(), true)) {
534         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
535+    }
536+    // If the transaction has 0-inputs (either a 0-input non-witness transaction or a normal transaction)

jnewbery commented at 6:33 pm on March 2, 2017:

what do you mean by ’normal transaction’ here?

achow101 commented at 6:50 pm on March 2, 2017:

that should be “witness transaction”

jnewbery commented at 6:37 pm on March 2, 2017: member

I don’t like that this breaks parity between bitcoin-tx sign and bitcoin-cli signrawtransaction.

bitcoin-tx sign will now decode the transaction as segwit when there’s ambiguity, but bitcoin-cli signrawtransaction will decode the transaction as 0-input non segwit when there’s ambiguity.

achow101 commented at 6:51 pm on March 2, 2017: member

@jnewbery I can remove the commit that changes bitcoin-tx

jnewbery commented at 9:18 pm on March 2, 2017: member

I can remove the commit that changes bitcoin-tx

But then you’ve broken the parity between bitcoin-tx and bitcoin-cli decoderawtransaction

achow101 commented at 4:24 pm on March 3, 2017: member

@jnewbery I’m not quite sure what you are asking me to do. Do you want to have decoderawtransaction, signrawtransaction, and bitcoin-tx all have the same decoding behavior?

jnewbery commented at 7:46 pm on March 3, 2017: member

I’d suggest making bitcoin-tx behave like signrawtransaction if it’s being used to sign a transaction, and behave like createrawtransaction/decoderawtransaction if it’s being used to decode, create or modify (without signing) a transaction.

DecodeRawTx decoding 0 input transactions

If a 0 input transaction decodes as a witness transaction properly, that decoding will be used. Otherwise it will default to the legacy deserialization.

578932648b

Use the same DecodeRawTx 0-input handling in bitcoin-tx rawtx decodings

Use the same handling of 0-input non-witness transactions and of normal witness transactions in bitcoin-tx's rawtx decoding step.

3969fe179a

achow101 force-pushed on Mar 6, 2017

achow101 commented at 2:25 pm on March 6, 2017: member

@jnewbery I don’t think it is possible to have separate behavior like that in bitcoin-tx. You can have multiple things happening in one bitcoin-tx command. You can have it modify a transaction and then sign it, and for all of those to act on the same transaction consistently, the transaction needs to be the same across all bitcoin-tx commands, so there can’t be separate decoding behavior. Also, the transaction is only decoded once for all bitcoin-tx commands.

I could change signrawtransaction to have the same decoding behavior, and it would make sense there since you aren’t going to be signing a 0-input transaction.

achow101 closed this on Jun 8, 2017

DrahtBot locked this on Sep 8, 2021

[RPC] Fix decoderawtransaction decoding of segwit txs #9522