Add psbt watch-only/cold-storage example by tcharding · Pull Request #940 · rust-bitcoin/rust-bitcoin

tcharding · 2022-04-05T05:39:36Z

Add an example PSBT workflow. The workflow we simulate is that of a setup using a watch-only online wallet (contains only public keys) and a cold-storage wallet (contains the private keys).

We create and update a PSBT using the watch-only wallet then pass the PSBT to the cold-storage wallet to sign.

Partially resolves #892 (more done in #935).

Note

This PR includes a sub-module in examples/psbt.rs that implements ECDSA signing. This will hopefully eventually be merged into the main crate by way of #957. We have three PRs that add examples/tests of PSBTs that need to do signing, in order to help us design a good AP in #957 I think it would be beneficial to complete and merge these three PRs.

This PR (PSBT ECDSA example)
PBST ECDSA integration test: Add psbt BIP174 test vectors tests #935
PSBT taproot example: examples: Add taproot PSBT example workflow #999

Note to self, this will need a change to test.sh if #1079 merges.

examples/psbt.rs

tcharding · 2022-04-06T01:50:48Z

Thanks @DanGould, you the man! I used all your suggestions and everything works now, I was able to broadcast the transaction with bitcoind. I added a Co-authored-by tag!

apoelstra

ACK af148d182efcf37d79028e4a516bba75138fc273

Wow! Did not read the code super carefully but skimmed all of it. A very thorough example.

Can me merged before or after 0.28.0, it's completely independent of the release.

tcharding · 2022-04-18T21:53:51Z

~~I'd like to hold off on merging this until ~~the bdk sighash license/upstream stuff is resolved.~~ #957 gets done.~~

I'd like to merge this before #957 so we can use this as a way to prove signing works as its coded, at least for ECDSA.

tcharding · 2022-04-20T06:14:45Z

Converting this to draft until #957 gets sorted.

tcharding · 2022-06-02T01:10:53Z

@DanGould I decided to throw this up in a state that can be merged because the PR to add PSBT sign functionality (#957) is proving to be thorny and I do not know how long till it will be mergable. Sorry for all the back and forward on signing but if you like you could cut'n'paste the psbt_sign module out of this into #935 and then try to get it merged as well. I cannot think of a better way to share the signing code than duplication. I now think finishing #957 will be easier with this and #935 merged. What do you rekon?

tcharding · 2022-06-28T01:01:10Z

Converting back to draft because I've made changes to this over in #957 after review and cannot be bothered maintaining two branches.

tcharding · 2022-06-30T00:58:20Z

This PSBT example has been hanging around for months now. I've back and forthed, both mentally, and draft/undraft, a million times because of not knowing if its better to do #957 before or after this PR. Also, @DanGould has #935 open with similar confusion. Now @dunxen has #999 going to show taproot psbt signing.

Can we just merge these tests/examples and worry about cleaning them up to use #957 when/if that merges. My arguments are:

Its a PITA having these examples/tests in limbo
The other two devs are newish contributors and they did a fair bit of work on their PRs, it would be nice to recognize this and merge them instead of waiting on what is quite a curly PR (Add API to PSBT to enable signing inputs #957)
We would only be merging into tests/ and examples/ so this is not a risk to the library
With three devs looking at this, at least in the short term, I don't think there is an undue a maintenance burden

Cheers

tcharding · 2022-06-30T01:29:14Z

Re-named the example file to examples/ecdsa-psbt.rs in line with #999's usage of taproot-psbt.rs.

apoelstra

ACK a7f2d210d213e60c758a801f053af273f46b24a0

apoelstra · 2022-06-30T15:20:14Z

Pretty cool! Yeah, let's just get this in. I could find some things to nit but it's not worth keeping this in limbo.

tcharding · 2022-06-30T22:59:34Z

sick!

tcharding · 2022-07-11T05:23:12Z

I could find some things to nit but it's not worth keeping this in limbo.

Please do nit if you think its worth it and if it will help the other psbt example/test PRs. Otherwise I can come back and improve all three after they merge.

tcharding · 2022-07-17T22:17:06Z

Changes in force push:

rebased
Removed explicit scope required for Rust 1.29

apoelstra

ACK ee8d06dc474aca967a7cab8e0a56b2b308eb0453

Add an example PSBT workflow. The workflow we simulate is that of a setup using a watch-only online wallet (contains only public keys) and a cold-storage wallet (contains the private keys). We create and update a PSBT using the watch-only wallet then pass the PSBT to the cold-storage wallet to sign. Co-authored-by: Dan Gould <d@ngould.dev>

We just added a PSBT example file, run it from CI for all test runs.

tcharding · 2022-07-19T01:04:01Z

Changes in force push:

Rebased to pick up Sequence changes
Added a line to test.sh to run the example in all test runs (separate patch)

apoelstra

ACK cd2369b

DanGould · 2022-07-19T03:51:42Z

@sanket1729 this needs just one more approval

sanket1729

ACK cd2369b. The example is clean, you can address the comments in followups.

A lot of the code would be cleaned up after #957 .

This PR is really big and already has one ACK. In an effort of moving things forward and considering that my comments are really nits, I am merging this and creating an issue for addressing the follow-ups so that we forget them.

sanket1729 · 2022-07-19T18:23:00Z

examples/ecdsa-psbt.rs

+
+        let mut script_witness: Witness = Witness::new();
+        script_witness.push(&sigs[0].serialize());
+        script_witness.push(self.input_xpub.to_pub().serialize());


nit: You can push pk from the key in the partial_keys map here instead. It seems more natural to use that instead of deriving it again.

examples/ecdsa-psbt.rs

sanket1729 · 2022-07-19T18:46:29Z

examples/ecdsa-psbt.rs

+    /// The xpub derived from `INPUT_UTXO_DERIVATION_PATH`.
+    input_xpub: ExtendedPubKey,
+    /// The master extended pubkey fingerprint.
+    master_fingerprint: Fingerprint,


nit: I think we should save the derivation path for both account_0 and input_xpub instead of global constants.

sanket1729 · 2022-07-19T18:48:05Z

contrib/test.sh

    cargo test --verbose --features="$feature"
 done

+cargo run --example ecdsa-psbt --features=bitcoinconsensus


Not related to this issue, but I just noted that other examples are not being when "DO_NO_STD" is not set

… example cd2369b Run ecdsa-psbt example in test script (Tobin C. Harding) 6967c0e Add psbt example (Tobin Harding) Pull request description: Add an example PSBT workflow. The workflow we simulate is that of a setup using a watch-only online wallet (contains only public keys) and a cold-storage wallet (contains the private keys). We create and update a PSBT using the watch-only wallet then pass the PSBT to the cold-storage wallet to sign. Partially resolves #892 (more done in #935). ## Note This PR includes a sub-module in `examples/psbt.rs` that implements ECDSA signing. This will hopefully eventually be merged into the main crate by way of rust-bitcoin/rust-bitcoin#957. We have three PRs that add examples/tests of PSBTs that need to do signing, in order to help us design a good AP in #957 I think it would be beneficial to complete and merge these three PRs. 1. This PR (PSBT ECDSA example) 2. PBST ECDSA integration test: rust-bitcoin/rust-bitcoin#935 3. PSBT taproot example: rust-bitcoin/rust-bitcoin#999 Note to self, this will need a change to `test.sh` if #1079 merges. ACKs for top commit: apoelstra: ACK cd2369b sanket1729: ACK cd2369b. The example is clean, you can address the comments in followups. Tree-SHA512: c4fb8ec631bf8bfc30534e8974b1f6c4bb7cc6def165a4ee2bb7aa73f5aa7fdc11d2000ca25792a4b534b2c5bf1592efe542ada14a9d702c7dfacbaa808f3aea

@sanket1729

dd8730e Use new PSBT signing API in example (Tobin C. Harding) d2367fb Add PSBT sign functionality (Tobin C. Harding) b80e5ae Re-order import statements (Tobin C. Harding) Pull request description: Add an API for signing inputs to the `PSBT` struct. This is work based on code in `rust-miniscript` and the API design suggestions below from @sanket1729 and @Kixunil. Please note, this adds an `unimplemented!` call for taproot inputs. ECDSA signing is complete. Includes a patch adding the psbt example from #940 updated to use this new api. Run `cargo run --example psbt --features=bitcoinconsensus` to test it out. ACKs for top commit: dunxen: ACK dd8730e apoelstra: ACK dd8730e sanket1729: reACK dd8730e Tree-SHA512: 6345571e53cd3aa4b7ad962536da47ae03ab7c0b088107dc4104676bdb64fcf892e8fa60e0b716f3ef158d88d7058938bf267046721ccf74b2d1b092e9b9aaaa

tcharding force-pushed the psbt-bitcoind-example branch from 15b7b5d to fe87ff8 Compare April 5, 2022 05:45

tcharding changed the title ~~Add psbt example~~ Add psbt watch-only/cold-storage example Apr 5, 2022

DanGould suggested changes Apr 5, 2022

View reviewed changes

examples/psbt.rs Outdated Show resolved Hide resolved

examples/psbt.rs Outdated Show resolved Hide resolved

DanGould reviewed Apr 5, 2022

View reviewed changes

examples/psbt.rs Outdated Show resolved Hide resolved

DanGould reviewed Apr 5, 2022

View reviewed changes

examples/psbt.rs Outdated Show resolved Hide resolved

tcharding mentioned this pull request Apr 5, 2022

Add psbt BIP174 test vectors tests #935

Merged

tcharding force-pushed the psbt-bitcoind-example branch from fe87ff8 to 00dfd34 Compare April 6, 2022 01:49

tcharding force-pushed the psbt-bitcoind-example branch from 00dfd34 to af148d1 Compare April 6, 2022 03:11

tcharding marked this pull request as ready for review April 6, 2022 03:23

apoelstra previously approved these changes Apr 18, 2022

View reviewed changes

tcharding mentioned this pull request Apr 19, 2022

Add API to PSBT to enable signing inputs #957

Merged

tcharding marked this pull request as draft April 20, 2022 06:14

tcharding mentioned this pull request May 3, 2022

Write example for taproot psbt APIs #893

Closed

tcharding dismissed apoelstra’s stale review via a60188e May 19, 2022 05:52

tcharding force-pushed the psbt-bitcoind-example branch 4 times, most recently from d50d2d9 to b5d090d Compare May 25, 2022 05:28

tcharding force-pushed the psbt-bitcoind-example branch from b5d090d to c6b1006 Compare June 2, 2022 01:02

tcharding marked this pull request as ready for review June 2, 2022 01:07

tcharding mentioned this pull request Jun 8, 2022

WIP: Demonstrate usage of the new PSBT signing API #1045

Closed

tcharding marked this pull request as draft June 28, 2022 01:01

tcharding force-pushed the psbt-bitcoind-example branch from c6b1006 to a072bd4 Compare June 30, 2022 00:50

tcharding marked this pull request as ready for review June 30, 2022 00:58

tcharding force-pushed the psbt-bitcoind-example branch from a072bd4 to a7f2d21 Compare June 30, 2022 01:28

apoelstra previously approved these changes Jun 30, 2022

View reviewed changes

DanGould mentioned this pull request Jul 4, 2022

Moving bunch of stuff from my payjoin crate here #630

Open

tcharding dismissed apoelstra’s stale review via ee8d06d July 17, 2022 22:16

tcharding force-pushed the psbt-bitcoind-example branch from a7f2d21 to ee8d06d Compare July 17, 2022 22:16

apoelstra previously approved these changes Jul 17, 2022

View reviewed changes

tcharding and others added 2 commits July 19, 2022 10:59

Run ecdsa-psbt example in test script

cd2369b

We just added a PSBT example file, run it from CI for all test runs.

tcharding dismissed apoelstra’s stale review via cd2369b July 19, 2022 01:03

tcharding force-pushed the psbt-bitcoind-example branch from ee8d06d to cd2369b Compare July 19, 2022 01:03

apoelstra approved these changes Jul 19, 2022

View reviewed changes

sanket1729 approved these changes Jul 19, 2022

View reviewed changes

sanket1729 merged commit d6d5415 into rust-bitcoin:master Jul 19, 2022

tcharding deleted the psbt-bitcoind-example branch July 20, 2022 01:37

sanket1729 mentioned this pull request Jul 28, 2022

Don't use pub keyword in examples, and don't derive everything but the kitchen sink in examples #1141

Closed

Conversation

tcharding commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tcharding commented Apr 6, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Jun 28, 2022

Uh oh!

tcharding commented Jun 30, 2022

Uh oh!

tcharding commented Jun 30, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Jun 30, 2022

Uh oh!

tcharding commented Jun 30, 2022

Uh oh!

tcharding commented Jul 11, 2022

Uh oh!

tcharding commented Jul 17, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Jul 19, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

DanGould commented Jul 19, 2022

Uh oh!

sanket1729 left a comment • 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!

sanket1729 Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sanket1729 Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

sanket1729 Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tcharding commented Apr 5, 2022 •

edited

Loading

tcharding commented Apr 18, 2022 •

edited

Loading

tcharding commented Apr 20, 2022 •

edited

Loading

tcharding commented Jun 2, 2022 •

edited

Loading

sanket1729 left a comment •

edited

Loading