Refactor, second episode: move to PSBTs by darosior · Pull Request #21 · revault/revault_tx

darosior · 2020-10-01T14:23:35Z

This is based on #10 (as pretty much everything now..) and implements the usage of PSBT in our RevaultTransaction management, from the creation to the satisfaction.

This unlocked a few side improvements, such as:

Get rid of the RevaultSatisfier struct
Libbitcoinconsensus verification is back!
Even more typesafety (eg for TxOuts and script descriptors)

The methods of the RevaultTransaction traits have been modified with BIP174 roles in mind, which will likely guide us toward less footguns downstream.

darosior · 2020-10-02T09:35:01Z

Rebased after #10 rebase.

darosior · 2020-10-02T09:45:05Z

Rebased after #10 merge 🎉

src/prevouts.rs

darosior · 2020-10-02T10:56:44Z

src/transactions.rs

+        sigmap: &'a mut BTreeMap<bitcoin::PublicKey, Vec<u8>>,
+        sequence: u32,
+    ) -> RevaultInputSatisfier {
+        // This hack isn't going to last, see above.


Actually it is, here or upstream :)

darosior · 2020-10-02T13:13:07Z

Rebased on master and to clean up the history. Since 931274e breaks bisectability i wonder if i should squash it on 8af4279.

Also added a commit at the root as we broke master again...

edouardparis · 2020-10-02T13:30:36Z

src/transactions.rs

+                    bitcoin::Address::p2wsh(witness_script, bitcoin::Network::Bitcoin)
+                        .script_pubkey();
+                if expected_script_pubkey
+                    != psbtin


weird indentation, Is it rustfmt ?

JSwambo · 2020-10-02T13:42:35Z

src/txins.rs

+    /// Get a reference to the txout this txin refers
+    fn txout(&self) -> &T;
+    /// Get the actual txout this txin refers
+    fn get_txout(self) -> T;


rename to as_outpoint(), as_txout(), into_txout() as per #19 ?

I considered doing an actual cleanup PR for this and the documentation (maybe some more test cov as well). I could avoid introducing more here but... That incurs some non-trivial rebases (opened the issue after finishing this, fwiw)

JSwambo · 2020-10-02T13:48:12Z

src/txouts.rs

@@ -12,28 +15,36 @@ pub trait RevaultTxOut: fmt::Debug + Clone + PartialEq {
    fn inner_txout(&self) -> &TxOut;
    /// Get the actual inner txout
    fn get_txout(self) -> TxOut;


Here too (#19)... as_ and into_

src/transactions.rs

JSwambo

Ack 5e4dd40

src/transactions.rs

edouardparis

ACK 30305bc

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

This also removes the useless test_transaction_creation() test, just added to highlight the API. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

Still prep work for PSBT integration Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

This avoids confusion, also some slight drive-by doc improvements. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

This was coming. PrevOuts can be reused and with the inclusion of the TxOut into them we lost the Copy, intuitively bounding their lifetimes to one usage. Besides, we had those monstruous inputs in RevaultTransactions' new()s. Now that their sequence is bounded to them, they are one-shot transaction inputs, and their usage is much more intuitive! Of course, this helps me with PSBT as well :-) Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

This: - Replaces the inner Transaction by a PSBT in RevaultTransaction - Adapts the API to use BIP174 roles (somewhat) - Implements the satisfaction as part of the RevaultTransaction, under the Finalizer role. - Still makes use of our internal satisfier (even though the pub API is now part of RevaultTransaction). - Reuses some logic from Miniscript descriptors until upstream implements comprehensive PSBT support for them. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

We won't finalize a transaction that does not pass the libbitcoinconsensus check. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

darosior · 2020-10-02T17:19:57Z

Ok so:

improved the API for the add_signature() method as pointed by @edouardparis
addressed @JSwambo nits
squashed some commits to maintain bisectability (rebase master -x "cargo test")

Can you re review please ?

edouardparis

ACK a0e0ff5

e1ab2c6 transactions: remove state consistency TODO and useless alloc (Antoine Poinsot) 3d2b1f5 transactions: centralize the signature_hash() method (Antoine Poinsot) 6d63c38 transactions: don't unwrap in add_signature() (Antoine Poinsot) 108fc00 Improve documentation, remove error mod (Antoine Poinsot) 1715199 Cargo: use latest stable Miniscript version (Antoine Poinsot) bead86f Change method names to conventional ones (Antoine Poinsot) Pull request description: Based on #21 ACKs for top commit: JSwambo: Ack e1ab2c6 Tree-SHA512: 09e938dc55bf8b3b5330cc2a36695f940d3ccb5ad806a4409eea2160b859f3903cbcf0e69e02567173ade31b573f8a793a42d08bc64a498ef98ddb0aa8c506b2

darosior changed the title ~~Refactor, season 2: move to PSBTs~~ Refactor, second episode: move to PSBTs Oct 2, 2020

darosior force-pushed the psbt branch from aee5e84 to 24c110e Compare October 2, 2020 09:34

darosior force-pushed the psbt branch from 24c110e to 4573209 Compare October 2, 2020 09:44

darosior commented Oct 2, 2020

View reviewed changes

darosior force-pushed the psbt branch 2 times, most recently from 5f84d66 to 5e4dd40 Compare October 2, 2020 13:11

darosior requested review from JSwambo and edouardparis October 2, 2020 13:13

edouardparis reviewed Oct 2, 2020

View reviewed changes

JSwambo reviewed Oct 2, 2020

View reviewed changes

src/transactions.rs Outdated Show resolved Hide resolved

JSwambo approved these changes Oct 2, 2020

View reviewed changes

edouardparis reviewed Oct 2, 2020

View reviewed changes

src/transactions.rs Outdated Show resolved Hide resolved

darosior force-pushed the psbt branch from d32da1f to 30305bc Compare October 2, 2020 16:52

edouardparis self-requested a review October 2, 2020 16:54

edouardparis approved these changes Oct 2, 2020

View reviewed changes

darosior added 10 commits October 2, 2020 19:00

transactions: cleanup imports

515d5a2

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

transactions: make serialize() propagate Errors

6efc2bd

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

transactions: return actual error type for encoding errors

8320b5e

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

txouts: add the output's witness script to the struct

63d2514

This also removes the useless test_transaction_creation() test, just added to highlight the API. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

prevouts: add the spent RevaultTxOut to RevaultPrevouts

de80fb9

Still prep work for PSBT integration Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

transactions: rename into_prevout() into into_outpoint()

9852b9d

This avoids confusion, also some slight drive-by doc improvements. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

txins: rename src/prevouts into src/txins

c2975d8

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

transactions: sanity check the PSBT input when adding a signature

3a0ffbd

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

darosior added 2 commits October 2, 2020 19:16

transactions: always check the transaction against libbitcoinconsensus

f560107

We won't finalize a transaction that does not pass the libbitcoinconsensus check. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

scripts: wrap the Descriptor in TxOut-specific newtypes

a0e0ff5

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

darosior force-pushed the psbt branch from 30305bc to a0e0ff5 Compare October 2, 2020 17:19

darosior requested review from JSwambo and edouardparis October 2, 2020 17:20

edouardparis approved these changes Oct 2, 2020

View reviewed changes

darosior mentioned this pull request Oct 3, 2020

Misc cleanups #23

Merged

darosior merged commit 71fc7f8 into revault:master Oct 3, 2020

JSwambo mentioned this pull request Oct 6, 2020

Serde support for revault transactions #28

Merged

Conversation

darosior commented Oct 1, 2020

Uh oh!

darosior commented Oct 2, 2020

Uh oh!

darosior commented Oct 2, 2020

Uh oh!

Uh oh!

darosior Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darosior commented Oct 2, 2020

Uh oh!

edouardparis Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

darosior Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

JSwambo Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darosior Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

JSwambo Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JSwambo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edouardparis left a comment

Choose a reason for hiding this comment

Uh oh!

darosior commented Oct 2, 2020

Uh oh!

edouardparis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

darosior Oct 2, 2020 •

edited

Loading

JSwambo Oct 2, 2020 •

edited

Loading