Skip to content
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

Merged
merged 3 commits into from Apr 4, 2024

Conversation

tcharding
Copy link
Member

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.

  • Patch 1: Fix stale docs
  • Patch 2: Remove unnecessary panic
  • Patch 3: Change the key returned when signing Taproot input as key path spend

Done as part of #2557, this is the release blocking part.

@tcharding tcharding added this to the 0.32.0 milestone Apr 3, 2024
@tcharding tcharding added the P-high High priority label Apr 3, 2024
@github-actions github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Apr 3, 2024
@tcharding
Copy link
Member Author

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.

sanket1729
sanket1729 previously approved these changes Apr 3, 2024
Copy link
Member

@sanket1729 sanket1729 left a 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

@coveralls
Copy link

coveralls commented Apr 3, 2024

Pull Request Test Coverage Report for Build 8545199835

Details

  • 3 of 9 (33.33%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 83.117%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/psbt/mod.rs 3 9 33.33%
Totals Coverage Status
Change from base Build 8539889544: 0.0%
Covered Lines: 19186
Relevant Lines: 23083

💛 - Coveralls

@tcharding
Copy link
Member Author

tcharding commented Apr 3, 2024

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)>
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.
@tcharding
Copy link
Member Author

tcharding commented Apr 3, 2024

Had another crack at documenting the change (in rustdocs). Rebased also.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 14040e2

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 14040e2

@apoelstra apoelstra merged commit df9b31d into rust-bitcoin:master Apr 4, 2024
168 checks passed
@tcharding tcharding deleted the 04-03-return-internal-key branch April 5, 2024 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bitcoin PRs modifying the bitcoin crate P-high High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants