BIP174 (de)serialization support

[dongcarl]

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:

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

First time contributing... Excited 😄

[dongcarl]

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:

1. This code is much DRY-er than the bitcoind implementation thanks to macros and traits (yay rust)
2. The decode/encode process has an intermediary type: PSBTRawKVType, which is inserted/extracted from PSBTMaps
3. 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]

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

[dongcarl]

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

[dongcarl]

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

[dongcarl]

Made PSBT-specific deserialization not influence other modules

[jeandudey]

Greate work! I left my two cents 👍

[jeandudey]

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

[dongcarl]

See 7a6df92 and 5168c7c, will rebase

[jeandudey]

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

[dongcarl]

Done

[jeandudey]

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

[dongcarl]

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

[jeandudey]

Ideally

[dongcarl]

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

[jeandudey]

Yes, it's up for another PR.

[dongcarl]

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

[apoelstra]

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.

[apoelstra]

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

[jeandudey]

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> {
        ....
    }
}

[dongcarl]

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

[dongcarl]

Done for both PsbtGlobal and PartiallySignedTransaction

[apoelstra]

Needs rebasing on the new ChildNumber semantics.

[apoelstra]

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.

[jeandudey]

I find it weird too, seems confunsing

[dongcarl]

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]

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

[dongcarl]

Awesome! Much cleaner...

[apoelstra]

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

[apoelstra]

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]

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]

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.

[dongcarl]

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

[apoelstra]

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.

[dongcarl]

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]

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]

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

[apoelstra]

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.

[dongcarl]

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?

[stevenroose]

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.

[apoelstra]

@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).

[apoelstra]

@achow101 out of curiosity, was this deliberate?

[achow101]

Yes (maybe)

[apoelstra]

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

1. As I mentioned on IRC, secp256k1::PublicKey should be replaced everywhere by util::key::PublicKey to ensure compressedness is preserved.
2. 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]

Key types are replaced and manual serialization done.

[stevenroose]

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

[stevenroose]

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

[dongcarl]

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

1. 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)
2. 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.

[apoelstra]

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