BIP174 (de)serialization support #103

dongcarl · 2018-07-06T05:41:56Z

This is currently a non-working work-in-progress to add BIP174 (de)serialization support to rust-bitcoin.

I'm wondering about the conventions of this project so I can conform to them better, namely:

Could I add the type aliases I'm defining here to the relevant modules? (e.g. adding KeyID to util::bip32::ExtendedPubKey)
Does psbt.rs belong in src/network or some other module?

First time contributing... Excited 😄

dongcarl · 2018-07-24T11:52:48Z

After a few days of grinding away, I have an implementation with tests here

Too tired right now to write out the entire documentation but here are some comments:

This code is much DRY-er than the bitcoind implementation thanks to macros and traits (yay rust)
The decode/encode process has an intermediary type: PSBTRawKVType, which is inserted/extracted from PSBTMaps
Currently the PartiallySignedTransaction type can merge (as a combiner) and implements Into<Transaction> (as a extractor), although this code is not as robust as the encoding/decoding code

Any initial comments are welcome as I attempt to finalize this

TheBlueMatt · 2018-07-24T15:35:43Z

With all the new serialization/deserialization code this almost certainly needs new entries in fuzz/fuzz_targets.

dongcarl · 2018-08-03T01:00:53Z

@TheBlueMatt Works now, tests passed, fuzz tests are there.

dongcarl · 2018-08-10T20:52:16Z

@TheBlueMatt @apoelstra Cleaned up my commits... First 3 commits which are non-PSBT are also available as cherry-picks at #122 #123 #124

dongcarl · 2018-08-10T21:32:09Z

Made PSBT-specific deserialization not influence other modules

jeandudey

Greate work! I left my two cents 👍

jeandudey · 2018-08-10T21:16:16Z

src/util/mod.rs

-    UnsupportedWitnessVersion(u8)
+    UnsupportedWitnessVersion(u8),
+    /// PSBT-related error
+    PSBT(psbt::Error),


Shouldn't this variant be named Psbt? All types with acronyms in the codebase only have the first letter uppercased e.g. Sha256dHash (C-CASE).

See 7a6df92 and 5168c7c, will rebase

I think that guideline applies to all types with the PSBT prefix (PSBTGlobal and such ones).

jeandudey · 2018-08-10T21:22:32Z

src/util/mod.rs

@@ -165,3 +171,8 @@ impl From<secp256k1::Error> for Error {
    }
 }

+impl From<psbt::Error> for Error {


Please add a #[doc(hidden)] attribute (C-HIDDEN).

Should that be added to all the From<*> for Error impls?

@jeandudey let's open up another PR after this gets merged to fix all of them?

Yes, it's up for another PR.

Going to wait until this one's merged to do this for all Froms at once

Maybe we should change SimpleDecoder to always use util::Error rather than being parameterized, and add a pub use util::Error at the top-level, in the same PR.

#127 added #[doc(hidden)] everywhere - we should do so in this PR then as well.

jeandudey · 2018-08-10T21:28:28Z

src/util/psbt/psbtmap/psbtglobal.rs

+    }
+}
+
+impl From<Transaction> for Result<PSBTGlobal, util::Error> {


It seems weird to implement this to a std type, isn't it more clear to have a from_transaction method in PSBTGlobal?

Something along the lines:

impl PSBTGlobal { pub fn from_transaction(tx: Transaction) -> Result<PSBTGlobal, util::Error> { .... } }

How about I add this and leave the From version as well?

Done for both PsbtGlobal and PartiallySignedTransaction

apoelstra · 2018-08-12T13:58:53Z

Needs rebasing on the new ChildNumber semantics.

apoelstra · 2018-08-12T17:40:44Z

src/util/psbt/mod.rs

+    }
+}
+
+impl From<Transaction> for Result<PartiallySignedTransaction, util::Error> {


Is it a standard Rust idiom to impl From on a Result? I find it very confusing, From is supposed to be for things that are conceptually trivial conversions or reinterpretations. But a Transaction is nothing at all like a Result<T, E>.

I would prefer that PartiallySignedTransaction had a from_transaction method on it.

I find it weird too, seems confunsing

Agreed, I was trying to emulate the new TryFrom trait that isn't in 1.14.0, but will just use the dedicated method.

apoelstra · 2018-08-12T17:44:42Z

src/util/psbt/mod.rs

+    fn consensus_decode(d: &mut D) -> Result<Self, D::Error> {
+        let magic: [u8; 4] = ConsensusDecodable::consensus_decode(d)?;
+
+        if [b'p', b's', b'b', b't'] != magic {


nit: you can use *b"psbt" rather than an explicit array here

Awesome! Much cleaner...

apoelstra · 2018-08-12T17:45:14Z

src/util/psbt/mod.rs

+
+impl<S: SimpleEncoder<Error = util::Error>> ConsensusEncodable<S> for PartiallySignedTransaction {
+    fn consensus_encode(&self, s: &mut S) -> Result<(), S::Error> {
+        [b'p', b's', b'b', b't'].consensus_encode(s)?;


nit: b"psbt".consensus_encode(s)?

apoelstra · 2018-08-12T17:50:53Z

src/util/psbt/mod.rs

+    pub fn from_unsigned_tx(tx: Transaction) -> Result<Self, util::Error> {
+        Ok(PartiallySignedTransaction {
+            inputs: Vec::with_capacity(tx.input.len()),
+            outputs: Vec::with_capacity(tx.output.len()),


I think this is wrong - search " The transaction format is specified as follows:" in BIP 174 which says that there must be a map for every input and output. Using with_capacity() will create empty vectors, so that serializing a fresh PartiallySignedTransaction will result in an invalid PSBT.

I think providing empty maps with vec![Default::default(); tx.input.len()] should be fine; then an Updater can add actual info.

apoelstra · 2018-08-12T18:52:03Z

I'd like if none of the types had Psbt prefixing them; you can pub use them in util/psbt/mod.rs so that users can import the module then use them like psbt::Global rather than needing to import the full type and writing PsbtGlobal.

apoelstra · 2018-08-12T19:02:46Z

src/util/psbt/psbtmap/psbtglobal.rs

+        match tx_raw_kv_pair.len() {
+            0 => Err(D::Error::from(Error::MustHaveUnsignedTx)),
+            1 => {
+                let tx: Transaction = deserialize(&tx_raw_kv_pair[0].value[..])?;


You need to check at some point that raw_kv_pair.key.key.is_empty(), because as written this will deserialize any key with the type_value 0x00, no matter its length or contents, which the BIP forbids.

Also, Transaction::deserialize will ignore trailing bytes, which you should flag an error on (and also flag an error if it's signed, has a witness array, etc). Annoyingly to catch trailing bytes you need to copy the RawDecoder logic out of network::serialize::deserialize, unwrap the rawdecoder when its done, and check that the underlying cursor is spent.

Hmmm... Anything else that will ignore trailing bytes? I'd be happy to check for all of them

Yes, the deserializer will ignore trailing bytes on all Bitcoin objects because they all self-describe their sizes, and the deserializer is meant to work with streams where "no more bytes" is not always well-defined.

Hmmm... I think that makes sense for ConsensusDecodable, but perhaps deserialize should error if the cursor is not consumed as the caller is explicitly saying that the bytes provided should deserialize to a certain type?

TheBlueMatt · 2018-08-24T19:38:56Z

After long IRC back-and-forth, it seems we need to have a separate Transaction decoder for PSBT - because we want to support 0-input-decoding in non-witness-serialization-format in PSBT, but probably cant do it in non-PSBT (to fix the SegWit-ambiguity-bug, the easiest thing to do is to encode 0-input txn with witnesses always).

dongcarl · 2018-08-24T22:15:53Z

@TheBlueMatt Good thing we have PSBT specific serialization/deserialization traits... Will do.

src/util/psbt/map/global.rs

stevenroose · 2019-02-20T14:48:04Z

src/util/psbt/map/input.rs

+            }
+        }
+
+        if self.final_script_sig.is_none() && self.final_script_witness.is_none() {


Hmm, are we sure we want to enforce this like this? Instead of making the caller decide to provide this redundant information?

Or is this a protocol requirement?

From the BIP:

The Input Finalizer must only accept a PSBT. For each input, the Input Finalizer determines if the input has enough data to pass validation. If it does, it must construct the scriptSig and scriptWitness and place them into the input key-value map. All other data except the UTXO and unknown fields in the input key-value map should be cleared from the PSBT. The UTXO should be kept to allow Transaction Extractors to verify the final network serialized transaction.

Perhaps the function could be renamed in the future, it's more providing an intermediary representation where everything will be serialized rather than just spitting out all the Pairs (which is what get_pairs seems to imply).

Yeah I think get_pairs doesn't qualify as the "Input Finalizer" from the spec.

I'd be more in favor of an additional method finalize or remove_redundant_fields hat strips all no-longer-relevant information from it. Now this would break round-tripping PSBT structs that have redundant information as get_pairs is used for serializing.

I agree with @stevenroose. These if statements also prevent PSBTs from round-tripping which makes fuzzing harder.

@stevenroose @apoelstra How about we remove these if statements for now, and in a future PR introduce finalize?

apoelstra · 2019-02-23T15:11:20Z

src/util/psbt/map/global.rs

+                type_value: 0u8,
+                key: vec![],
+            },
+            value: self.unsigned_tx.serialize(),


This needs to be replaced with manual serialization code to ensure 0-input transactions are serialized without witnesses. (And the corresponding unit test needs to be changed.)

+ value: { + let mut ret = vec![]; + self.unsigned_tx.version.consensus_encode(&mut ret).unwrap(); + self.unsigned_tx.input.consensus_encode(&mut ret).unwrap(); + self.unsigned_tx.output.consensus_encode(&mut ret).unwrap(); + self.unsigned_tx.lock_time.consensus_encode(&mut ret).unwrap(); + ret + }, });

works.

Same for the non_witneess_utxo in Input? Would it make sense if I just made sure that psbt::Serialize for Transaction always manually serializes?

Hmm, non_witness_utxo doesn't necessarily mean that the previous tx doesn't have witnesses, right? Not sure if they are entirely irrelevant. They are for txid generation, which I think this field is primarily used for, but I wouldn't dare claim they are entirely unused.

@dongcarl the test vectors have witnesses in non_witness_utxo. You need to support segwit witnesses. (And this is fine, everything is unambiguous since the referenced transactions need to be valid, i.e. have a nonzero number of outputs).

@achow101 out of curiosity, was this deliberate?

Yes (maybe)

apoelstra · 2019-02-23T15:16:12Z

Couple comments which don't really match any lines of code:

As I mentioned on IRC, secp256k1::PublicKey should be replaced everywhere by util::key::PublicKey to ensure compressedness is preserved.
We can't test perfect round-tripping because hashmaps don't have guaranteed orders; but it would be good to reserialize PSBTs in our fuzz tests and at least test that the lengths and some sort of order-independent checksum remain the same. (I'd accept a sum of all the bytes as an "order-independent checksum".)

dongcarl · 2019-02-27T23:34:05Z

Key types are replaced and manual serialization done.

stevenroose

Looks good! Just the nit about adding an explanatory comment.

stevenroose · 2019-02-28T12:36:30Z

src/util/psbt/map/global.rs

+                self.unsigned_tx.input.consensus_encode(&mut ret)?;
+                self.unsigned_tx.output.consensus_encode(&mut ret)?;
+                self.unsigned_tx.lock_time.consensus_encode(&mut ret)?;
+                ret


Only nit: Add a comment explaining why you're doing the manual serialization here. New readers will be confused otherwise.

- Implement psbt::Error data type - Implement conversion from psbt::Error to util::Error - Create util::psbt module - Create non-public util::psbt::error module

- Add (en)decoding logic for said data types

- Implement psbt::Map trait for psbt::Global - Add converting constructor logic from Transaction for psbt::Global - Add (en)decoding logic for psbt::Global - Always deserialize unsigned_tx as non-witness - Add trait for PSBT (de)serialization - Implement PSBT (de)serialization trait for relevant psbt::Global types - Add macros for consensus::encode-backed PSBT (de)serialization implementations - Add macro for implementing encoding logic for PSBT key-value maps

- Implement psbt::Map trait for psbt::Output - Add (en)decoding logic for psbt::Output - Implement PSBT (de)serialization trait for relevant psbt::Output types - Add macro for merging fields for PSBT key-value maps - Add macro for implementing decoding logic for PSBT key-value maps - Add convenience macro for implementing both encoding and decoding logic for PSBT key-value maps - Add macro for inserting raw PSBT key-value pairs into PSBT key-value maps - Add macro for getting raw PSBT key-value pairs from PSBT key-value maps

dongcarl · 2019-02-28T16:01:52Z

I think that the following changes can be done in separate PRs, and I currently don't have the bandwidth to complete them:

Some kind of finalize method as detailed by @stevenroose here: BIP174 (de)serialization support #103 (comment) (I will update this current PR to be dumb when serializing/deserializing)
Improved fuzzing as detailed by @apoelstra here: BIP174 (de)serialization support #103 (comment)

Let's get minimal functionality merged in, then we can take our time expanding on it.

- Implement psbt::Map trait for psbt::Input - Add (en)decoding logic for psbt::Input - Implement PSBT (de)serialization trait for relevant psbt::Input types

- Add merging logic for PartiallySignedTransactions - Add (en)decoding logic for PartiallySignedTransaction - Add converting constructor logic from Transaction for PartiallySignedTransaction - Add extracting constructor logic from PartiallySignedTransaction for Transaction Squashed in fixes from stevenroose <stevenroose@gmail.com> - Prevent PSBT::extract_tx from panicking - Make PartiallySignedTransaction fields public

- Add macro for decoding and unwrapping PartiallySignedTransaction from hex string

apoelstra · 2019-02-28T16:46:51Z

Sounds good. I can do fuzzing. Probably it is blocked on deciding how to parse hybrid keys (currently we interpret them as uncompressed and therefore reserialize them wrong; maybe we should reject them, but they are permissible on the blockchain..)

stevenroose

ACK e5b5912

Fix minimum_sum_amount field name

Relax cc dependency requirements.

dongcarl force-pushed the psbt branch from 5709fc3 to a8cd55e Compare July 6, 2018 08:49

TheBlueMatt mentioned this pull request Jul 20, 2018

deserialize for Transaction Err if there are no transaction inputs #104

Closed

dongcarl force-pushed the psbt branch 3 times, most recently from 320948e to d64fcec Compare August 2, 2018 22:08

dongcarl changed the title ~~WIP: BIP174 (de)serialization support~~ Ready for Review: BIP174 (de)serialization support Aug 3, 2018

dongcarl force-pushed the psbt branch from e5bce4f to 2722dd0 Compare August 10, 2018 20:43

dongcarl force-pushed the psbt branch from 2722dd0 to 619937b Compare August 10, 2018 21:28

jeandudey suggested changes Aug 10, 2018

View reviewed changes

dongcarl force-pushed the psbt branch 3 times, most recently from 9b17114 to 866c2b5 Compare August 10, 2018 23:44

apoelstra reviewed Aug 12, 2018

View reviewed changes

dongcarl mentioned this pull request Aug 13, 2018

Pinning down an Error type for SimpleDecoder and SimpleEncoder #131

Closed

dongcarl force-pushed the psbt branch 2 times, most recently from da0bf67 to da36b34 Compare August 17, 2018 19:01

apoelstra added this to the 0.15 milestone Aug 20, 2018

stevenroose reviewed Feb 21, 2019

View reviewed changes

dongcarl force-pushed the psbt branch from 2b4b31f to de7b5b1 Compare February 21, 2019 16:51

apoelstra self-requested a review February 22, 2019 15:33

apoelstra reviewed Feb 23, 2019

View reviewed changes

dongcarl force-pushed the psbt branch from de7b5b1 to fcda408 Compare February 27, 2019 23:27

stevenroose previously approved these changes Feb 28, 2019

View reviewed changes

stevenroose self-requested a review February 28, 2019 12:37

dongcarl added 5 commits February 28, 2019 10:54

Add PSBT-specific Error data type

4fa39c4

- Implement psbt::Error data type - Implement conversion from psbt::Error to util::Error - Create util::psbt module - Create non-public util::psbt::error module

Add data types for raw PSBT key-value pairs

528e393

- Add (en)decoding logic for said data types

Add trait for PSBT key-value maps

2715a6e

dongcarl added 5 commits February 28, 2019 11:11

Add PSBT input data key-value map type

badb0f2

- Implement psbt::Map trait for psbt::Input - Add (en)decoding logic for psbt::Input - Implement PSBT (de)serialization trait for relevant psbt::Input types

Add test vectors from BIP174 specification

bc73b31

- Add macro for decoding and unwrapping PartiallySignedTransaction from hex string

Add fuzz testing for PartiallySignedTransaction

f74ec3e

Add copyright notice to PSBT-related files

e5b5912

dongcarl dismissed stevenroose’s stale review via e5b5912 February 28, 2019 16:17

dongcarl force-pushed the psbt branch from fcda408 to e5b5912 Compare February 28, 2019 16:17

apoelstra self-requested a review February 28, 2019 16:47

apoelstra approved these changes Feb 28, 2019

View reviewed changes

stevenroose approved these changes Feb 28, 2019

View reviewed changes

apoelstra merged commit 67e4671 into rust-bitcoin:master Feb 28, 2019

Tibo-lg pushed a commit to p2pderivatives/rust-bitcoin that referenced this pull request Sep 1, 2020

Merge pull request rust-bitcoin#103 from lgalabru/fix/minimum-sum-amount

b164aa7

Fix minimum_sum_amount field name

Davidson-Souza pushed a commit to Davidson-Souza/rust-bitcoin that referenced this pull request Jul 12, 2023

Merge pull request rust-bitcoin#103 from alekseysidorov/master

bf9b972

Relax cc dependency requirements.

apoelstra mentioned this pull request Sep 13, 2024

Fix GetKey for sets (plus some related changes) #3356

Merged

BIP174 (de)serialization support #103

BIP174 (de)serialization support #103

Conversation

dongcarl commented Jul 6, 2018

dongcarl commented Jul 24, 2018

TheBlueMatt commented Jul 24, 2018

dongcarl commented Aug 3, 2018

dongcarl commented Aug 10, 2018

dongcarl commented Aug 10, 2018

jeandudey left a comment

Choose a reason for hiding this comment

jeandudey Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Aug 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Aug 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 24, 2018

dongcarl commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

apoelstra commented Feb 23, 2019

dongcarl commented Feb 27, 2019

stevenroose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongcarl commented Feb 28, 2019

apoelstra commented Feb 28, 2019

stevenroose left a comment

Choose a reason for hiding this comment

jeandudey Aug 10, 2018 •

edited

Loading

stevenroose Feb 21, 2019 •

edited

Loading

achow101 Feb 25, 2019 •

edited

Loading