Add API to PSBT to enable signing inputs by tcharding · Pull Request #957 · rust-bitcoin/rust-bitcoin

tcharding · 2022-04-19T05:55:25Z

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.

DanGould

Concept ACK

src/util/psbt/mod.rs

tcharding · 2022-05-23T08:27:37Z

Thanks for the revew @dunxen, I'll take a look at your suggestions and re-spin the PR.

tcharding · 2022-05-24T06:07:27Z

Converting to draft because this PR is half baked, it has some taproot functionality and not all of it.

DanGould · 2022-05-25T04:48:43Z

It would be useful to advance the project at large to merge this without taproot support then add taproot support after since #940 and #935 depend on this functionality sans-taproot

dunxen · 2022-05-25T05:22:05Z

It would be useful to advance the project at large to merge this without taproot support then add taproot support after since #940 and #935 depend on this functionality sans-taproot

Yeah, I think this is reasonable. Getting taproot support in before next release would probably still be important (and doable). I have have no reservations about having it in a follow up PR :)

tcharding · 2022-05-25T05:30:49Z

Ok, in line with the two comments above I've updated this PR to explicitly not support taproot by returning an error if psbt.sign is called with a taproot input. Let's see what other reviewers think.

I've rebased #940 on top, perhaps you want to do #935 the same @DanGould, that would lend weight to having this merged.

tcharding · 2022-05-25T05:46:57Z

In case its of any use, I had a play with you 935 patch @DanGould, you can look at branch psbt-test-dan-gould on my fork if you want to.

dunxen · 2022-05-25T06:11:13Z

explicitly not support taproot by returning an error if psbt.sign is called with a taproot input.

Great! I was going to suggest exactly that 😃

I'll give this a ~~proper~~ review when I'm back home in a couple hours.

dunxen

Light review ACK 6fe41f7

dunxen · 2022-05-26T08:13:13Z

src/util/psbt/mod.rs

Since we check the bounds higher up in sign(), do we need to do it here too? Same with sighash()? I guess this is pretty defensive as it is now so no big deal :)

Suggested change

self.check_index_is_within_bounds(input_index)?;

Yes this is defensive, I personally prefer to see a length check anywhere I see an array access.

Kixunil · 2022-05-26T14:52:44Z

Will have to run soon, just a thought: it should be either sign_edsa or provide Taproot too.

tcharding · 2022-05-26T22:20:48Z

Its set up ready to add taproot, currently sign errors for a taproot input, for other inputs it calls sign_ecdsa. Happy with that?

Kixunil · 2022-05-30T12:53:36Z

I don't like the implied churn adding and removing error variants in the next release just because of this. Maybe unimplemented!() would be better. At least it'd increase motivation to add implementation soon. :)

tcharding · 2022-05-30T23:30:41Z

Fair point, I originally put a panic! but then thought an error would be slightly better. Happy to go with an unimplemented if there are no objections to that. Thanks for the review, this signing stuff is obscure (at least for me).

tcharding · 2022-09-14T01:35:13Z

Rebase only, no other changes.

apoelstra

ACK bac64ea55b720958508356b6d9a3cce058026c86

dunxen

ACK bac64ea

I can assist with the schnorr signing in a follow-up if you'd like :)

tcharding · 2022-09-15T21:30:42Z

I can assist with the schnorr signing in a follow-up if you'd like :)

That would be awesome!

tcharding · 2022-09-30T01:43:03Z

Rebase only, no other changes.

apoelstra

ACK cabafc5b0bfc7acbe1d3cb1f2b0c2605e93ea3b0

sanket1729

ACK cabafc5b0bfc7acbe1d3cb1f2b0c2605e93ea3b0. Reviewed the range-diff from a6355d2. I can work on follow-up with the hardcoded tests here.

I think this PR is getting to the point where Github is annoying to deal with. If others have any lingering issues, we can address those in follow ups.

tcharding · 2022-10-19T07:55:49Z

Thanks for all the reviews today @sanket1729, good to have you back here in rust-bitcoin :)

tcharding · 2022-10-19T07:57:41Z

2 acks, merge baby merge! This one has been open for six months (although admittedly for much of that time it was not in a mergable state)

apoelstra · 2022-10-19T12:44:24Z

Needs rebase, sorry!

The import statements in `psbt/mod.rs` are a bit of a mess, re-order them in an attempt to group like things and separate out things that are different (e.g. `pub use` from `use`). Refactor only, no logic changes.

Signing a PSBT requires no knowledge other than what we have here in this library and the PSBT ready to be signed. This code was pulled out of `rust-miniscript`. Add a `sign` method to the `PartiallySignedTransaction`.

We have a PSBT example that includes a custom signing module, we can remove that now and use the new PSBT signing API.

tcharding · 2022-10-19T19:08:41Z

Rebase only, no other changes.

dunxen

ACK dd8730e

apoelstra

ACK dd8730e

tcharding · 2022-10-20T19:37:10Z

Re-ack from @sanket1729 please.

sanket1729

reACK dd8730e

tcharding · 2022-10-20T20:24:16Z

So much red and purple in my notifications this morning - nice work crew.

b8bd31d Promote rust-miniscript finalizer (DanGould) 16bf6f6 Test PSBT integration (DanGould) 6b6ef52 Add OP_0 alias for OP_PUSHBYTES_0 (DanGould) 72935a0 Move test_data/* tests/data (Tobin Harding) Pull request description: resolves #892 * Initial patch adds OP_0 alias for OP_PUSHBYTES_0 as [approved here](#935 (comment)) * Second patch is the bulk of this work. It tests BIP 174 PSBT integration defined at [The BIP's Test Vectors Section](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki#test-vectors) using #957 * Third patch points users to rust-miniscript for a more useful finalizer ACKs for top commit: tcharding: ACK b8bd31d sanket1729: ACK b8bd31d. Thanks for sticking with this PR. This looks awesome after #957 Tree-SHA512: dc68e524d4349530b082baf5032460fa56593b0ef192125c0b7d7e00954e5593f386b7f1984fc00106b4b9eafbf29cc80ab368dbd26b710eba0962dbd89e0013

tcharding mentioned this pull request Apr 19, 2022

Add psbt watch-only/cold-storage example #940

Merged

tcharding force-pushed the sign-psbt branch from d049896 to 43b03da Compare April 20, 2022 00:49

DanGould reviewed Apr 27, 2022

View reviewed changes

src/util/psbt/mod.rs Outdated Show resolved Hide resolved

src/util/psbt/mod.rs Outdated Show resolved Hide resolved

tcharding force-pushed the sign-psbt branch 2 times, most recently from 8764c85 to bdf7aa6 Compare May 19, 2022 05:55

tcharding marked this pull request as ready for review May 19, 2022 06:08

tcharding marked this pull request as draft May 19, 2022 06:12

tcharding force-pushed the sign-psbt branch from bdf7aa6 to 3af31de Compare May 19, 2022 06:15

tcharding mentioned this pull request May 19, 2022

Add psbt BIP174 test vectors tests #935

Merged

tcharding force-pushed the sign-psbt branch from 3af31de to f5644da Compare May 20, 2022 02:50

tcharding marked this pull request as ready for review May 20, 2022 02:54

dunxen reviewed May 21, 2022

View reviewed changes

src/util/psbt/mod.rs Outdated Show resolved Hide resolved

src/util/psbt/mod.rs Outdated Show resolved Hide resolved

src/util/psbt/mod.rs Outdated Show resolved Hide resolved

tcharding marked this pull request as draft May 24, 2022 06:07

tcharding force-pushed the sign-psbt branch 2 times, most recently from d22a8da to a795f43 Compare May 24, 2022 23:55

tcharding mentioned this pull request May 25, 2022

examples: Add taproot PSBT example workflow #999

Merged

tcharding force-pushed the sign-psbt branch from a795f43 to 6fe41f7 Compare May 25, 2022 05:21

tcharding marked this pull request as ready for review May 25, 2022 05:31

dunxen reviewed May 26, 2022

View reviewed changes

tcharding force-pushed the sign-psbt branch from 56191bf to bac64ea Compare September 14, 2022 01:49

apoelstra previously approved these changes Sep 14, 2022

View reviewed changes

dunxen previously approved these changes Sep 15, 2022

View reviewed changes

tcharding dismissed stale reviews from dunxen and apoelstra via cabafc5 September 30, 2022 01:42

tcharding force-pushed the sign-psbt branch from bac64ea to cabafc5 Compare September 30, 2022 01:42

apoelstra previously approved these changes Sep 30, 2022

View reviewed changes

sanket1729 previously approved these changes Oct 19, 2022

View reviewed changes

tcharding added 3 commits October 20, 2022 05:59

Re-order import statements

b80e5ae

The import statements in `psbt/mod.rs` are a bit of a mess, re-order them in an attempt to group like things and separate out things that are different (e.g. `pub use` from `use`). Refactor only, no logic changes.

Add PSBT sign functionality

d2367fb

Signing a PSBT requires no knowledge other than what we have here in this library and the PSBT ready to be signed. This code was pulled out of `rust-miniscript`. Add a `sign` method to the `PartiallySignedTransaction`.

Use new PSBT signing API in example

dd8730e

We have a PSBT example that includes a custom signing module, we can remove that now and use the new PSBT signing API.

tcharding dismissed stale reviews from sanket1729 and apoelstra via dd8730e October 19, 2022 19:08

tcharding force-pushed the sign-psbt branch from cabafc5 to dd8730e Compare October 19, 2022 19:08

dunxen approved these changes Oct 19, 2022

View reviewed changes

apoelstra approved these changes Oct 19, 2022

View reviewed changes

sanket1729 approved these changes Oct 20, 2022

View reviewed changes

sanket1729 merged commit fcf9bd0 into rust-bitcoin:master Oct 20, 2022

tcharding deleted the sign-psbt branch October 20, 2022 20:01

apoelstra mentioned this pull request Jan 21, 2024

Make Psbt::sighash_ecdsa private #2367

Closed

Conversation

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

Uh oh!

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tcharding commented May 23, 2022

Uh oh!

tcharding commented May 24, 2022

Uh oh!

DanGould commented May 25, 2022

Uh oh!

dunxen commented May 25, 2022

Uh oh!

tcharding commented May 25, 2022

Uh oh!

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

Uh oh!

Uh oh!

dunxen commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunxen left a comment

Choose a reason for hiding this comment

Uh oh!

dunxen May 26, 2022

Choose a reason for hiding this comment

Uh oh!

tcharding May 26, 2022

Choose a reason for hiding this comment

Uh oh!

Kixunil commented May 26, 2022

Uh oh!

tcharding commented May 26, 2022

Uh oh!

Kixunil commented May 30, 2022

Uh oh!

tcharding commented May 30, 2022

Uh oh!

tcharding commented Sep 14, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

dunxen left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 15, 2022

Uh oh!

tcharding commented Sep 30, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Oct 19, 2022

Uh oh!

tcharding commented Oct 19, 2022

Uh oh!

apoelstra commented Oct 19, 2022

Uh oh!

tcharding commented Oct 19, 2022

Uh oh!

dunxen left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Oct 20, 2022

Uh oh!

sanket1729 left a comment

tcharding commented Apr 19, 2022 •

edited

Loading

tcharding commented May 25, 2022 •

edited

Loading

dunxen commented May 25, 2022 •

edited

Loading