Fix PSBT deserialization of 0-input transactions

achow101 commented at 6:26 pm on August 13, 2018: member

0-input transactions can be ambiguously deserialized as being witness transactions. Since the unsigned transaction is never serialized as a witness transaction as it has no witnesses, we should always deserialize it as a non-witness transaction and set the serialization flags as such.

When a transaction is serliazed for the non-witness-utxo, it is always a valid network transaction and thus it should be always be deserialized as a witness transaction and the deserialzation flags are set as such.

Fixes #13958

achow101 force-pushed on Aug 13, 2018

sipa added this to the milestone 0.17.0 on Aug 13, 2018

in src/script/sign.h:307 in 1be6ef60d6 outdated

301@@ -302,6 +302,9 @@ struct PSBTInput
302                     } else if (key.size() != 1) {
303                         throw std::ios_base::failure("Non-witness utxo key is more than one byte type");
304                     }
305+                    // Set the stream to unserialize with witness since this is always a valid network transaction

sipa commented at 8:40 pm on August 13, 2018:

You can instead use UnserializeFromVector(OverrideStream(&s, s.GetType(), s.GetVersion() & ~SERIALIZE_TRANSACTION_NO_WITNESS); to avoid permanently changing the flags on s (similar below).

achow101 commented at 10:01 pm on August 13, 2018:

Done

sipa commented at 8:46 pm on August 13, 2018: member

Would you mind also just dropping the witness after reading the non-witness UTXO field (and/or before writing)?

sipa added the label Bug on Aug 13, 2018

achow101 force-pushed on Aug 13, 2018

Fix PSBT deserialization of 0-input transactions

0-input transactions can be ambiguously deserialized as being witness
transactions. Since the unsigned transaction is never serialized as
a witness transaction as it has no witnesses, we should always
deserialize it as a non-witness transaction and set the serialization
flags as such.

Also always serialize the unsigned transaction as a non-witness transaction.

43811e6338

Serialize non-witness utxo as a non-witness tx but always deserialize as witness

Strip out the witnesses when serializing the non-witness utxo. However
witness serializations are allowed, so make sure we always deserialize
as witness.

bd19cc78cf

achow101 force-pushed on Aug 13, 2018

achow101 commented at 10:01 pm on August 13, 2018: member

Would you mind also just dropping the witness after reading the non-witness UTXO field (and/or before writing)?

Done.

sipa commented at 10:47 pm on August 13, 2018: member

utACK bd19cc78cfc455cf06e120adb0d12c2f96ba8fca

kallewoof commented at 5:23 am on August 14, 2018: member

Tested ACK bd19cc78cfc455cf06e120adb0d12c2f96ba8fca

in src/script/sign.h:226 in bd19cc78cf

222@@ -223,7 +223,8 @@ struct PSBTInput
223         // If there is a non-witness utxo, then don't add the witness one.
224         if (non_witness_utxo) {
225             SerializeToVector(s, PSBT_IN_NON_WITNESS_UTXO);
226-            SerializeToVector(s, non_witness_utxo);
227+            OverrideStream<Stream> os(&s, s.GetType(), s.GetVersion() | SERIALIZE_TRANSACTION_NO_WITNESS);

Empact commented at 6:35 am on August 14, 2018:

nit: could use the WithOrVersion helper here and elsewhere https://github.com/bitcoin/bitcoin/blob/ddc3ec92b0e655a3da21ac2e85ec2e7ecb66c65b/src/streams.h#L66-L70

in src/streams.h:64 in bd19cc78cf

60@@ -61,6 +61,7 @@ class OverrideStream
61 
62     int GetVersion() const { return nVersion; }
63     int GetType() const { return nType; }
64+    size_t size() const { return stream->size(); }

Empact commented at 6:39 am on August 14, 2018:

nit: could be nice to put size with read, write, as they are all delegated.

laanwj added the label Needs backport on Aug 14, 2018

laanwj commented at 9:19 am on August 14, 2018: member

utACK bd19cc78cfc455cf06e120adb0d12c2f96ba8fca

meshcollider commented at 1:33 pm on August 14, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/13960/commits/bd19cc78cfc455cf06e120adb0d12c2f96ba8fca

ken2812221 referenced this in commit 3e5424faf6 on Aug 14, 2018

laanwj merged this on Aug 14, 2018

laanwj closed this on Aug 14, 2018

fanquake referenced this in commit 8c4cd2bd89 on Aug 15, 2018

fanquake referenced this in commit 517010e30e on Aug 15, 2018

fanquake removed the label Needs backport on Aug 15, 2018

fanquake commented at 2:04 am on August 15, 2018: member

Will be backported in #13976

laanwj referenced this in commit 4a2960f73e on Aug 15, 2018

uhliksk referenced this in commit 402bd31569 on Aug 29, 2018

uhliksk referenced this in commit f8550b4c5c on Aug 29, 2018

dongcarl commented at 5:26 am on September 9, 2018: member

When a transaction is serliazed for the non-witness-utxo, it is always a valid network transaction and thus it should be always be deserialized as a witness transaction and the deserialzation flags are set as such.

Surely you mean “…it should be always be deserialized as a non-witness transaction…”?

sipa commented at 5:30 am on September 9, 2018: member

@dongcarl No. “Deserialized as a witness transaction” here means “permit witnesses, and if ambiguous, prefer the interpretation with witness over the one with 0 inputs”. As it’s always a valid fully-signed transaction, it can’t have 0 inputs, and thus we should indeed prefer the witness version.

dongcarl commented at 5:36 am on September 9, 2018: member

@sipa Makes sense!

HashUnlimited referenced this in commit 46372117ad on Sep 14, 2018

HashUnlimited referenced this in commit bef9fe8a18 on Sep 14, 2018

kittywhiskers referenced this in commit dba778d481 on Aug 1, 2021

kittywhiskers referenced this in commit a246be3198 on Aug 1, 2021

UdjinM6 referenced this in commit 15055e46f6 on Aug 3, 2021

kittywhiskers referenced this in commit c1a0f89be0 on Aug 5, 2021

MarcoFalke locked this on Sep 8, 2021

Fix PSBT deserialization of 0-input transactions #13960