New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add API to PSBT to enable signing inputs #957
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
8764c85
to
bdf7aa6
Compare
Thanks for the revew @dunxen, I'll take a look at your suggestions and re-spin the PR. |
Converting to draft because this PR is half baked, it has some taproot functionality and not all of it. |
d22a8da
to
a795f43
Compare
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 :) |
Ok, in line with the two comments above I've updated this PR to explicitly not support taproot by returning an error if I've rebased #940 on top, perhaps you want to do #935 the same @DanGould, that would lend weight to having this merged. |
In case its of any use, I had a play with you 935 patch @DanGould, you can look at branch |
Great! I was going to suggest exactly that 😃 I'll give this a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Light review ACK 6fe41f7
src/util/psbt/mod.rs
Outdated
self.check_index_is_within_bounds(input_index)?; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
self.check_index_is_within_bounds(input_index)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is defensive, I personally prefer to see a length check anywhere I see an array access.
Will have to run soon, just a thought: it should be either |
Its set up ready to add taproot, currently |
I don't like the implied churn adding and removing error variants in the next release just because of this. Maybe |
Fair point, I originally put a |
That would be awesome! |
bac64ea
to
cabafc5
Compare
Rebase only, no other changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK cabafc5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the reviews today @sanket1729, good to have you back here in |
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) |
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.
Rebase only, no other changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK dd8730e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK dd8730e
Re-ack from @sanket1729 please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reACK dd8730e
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
Add an API for signing inputs to the
PSBT
struct. This is work based on code inrust-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.