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
psbt: Return internal key for key path spend #2652
psbt: Return internal key for key path spend #2652
Conversation
CC @sanket1729, I went ahead and implement your suggestion in the description of #2557 since it seems the most sane and had no replies/nacks. |
9203eda
to
77527e3
Compare
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 77527e3.
Thanks for taking this up <3
Pull Request Test Coverage Report for Build 8545199835Details
💛 - Coveralls |
No sweat, was nice to do a real PR instead of release management. |
@@ -301,7 +298,7 @@ impl Psbt { | |||
&mut self, | |||
k: &K, | |||
secp: &Secp256k1<C>, | |||
) -> Result<SigningKeys, (SigningKeys, SigningErrors)> | |||
) -> Result<SigningKeysMap, (SigningKeysMap, SigningErrors)> |
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.
In 77527e3:
Github won't let me comment directly on it, but the Returns
section of the doccomment for this method should be rewritten to use backtick links, reduce repetition, to update to the new SigningKeysMap
type, and to mention that we return internal keys.
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.
Oh boy, sloppiest PR this week by far.
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.
Lol, well, it was just one doccomment. It just happened to have multiple issues :).
We recently added support for signing taproot inputs but forgot to update the docs to reflect this. Remove stale rustdoc from `Psbt::sign` function.
There is no need to panic if input index is out of bounds because we have a function to check the validity of the `input_index` argument and use it in other places already.
When signing a Taproot input (in a PSBT) using a key path spend we currently return the pubkey associated with key that signs. However it is common to think of the internal key as being the one that signs even though this is not technically true. We also have the internal key in the PSBT so matching against it is less surprising. When using the `Psbt` type to sign a Taproot input using a key path spend return the internal key.
77527e3
to
14040e2
Compare
Had another crack at documenting the change (in rustdocs). Rebased also. |
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 14040e2
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 14040e2
When signing a Taproot input (in a PSBT) using a key path spend we currently return the pubkey associated with key that signs. However it is common to think of the internal key as being the one that signs even though this is not technically true. We also have the internal key in the PSBT so matching against it is less surprising.
When using the
Psbt
type to sign a Taproot input using a key path spend return the internal key.Done as part of #2557, this is the release blocking part.