[FreshEyes] crypto, refactor: add new KeyPair class #15
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The wallet returns an untweaked internal key for taproot outputs. If the output commits to a tree of scripts, this key needs to be tweaked with the merkle root. Even if the output does not commit to a tree of scripts, BIP341/342 recommend commiting to a hash of the public key. Previously, this logic for applying the taptweak was implemented in the ::SignSchnorr method. Move this tweaking and signing logic to a new class, KeyPair, and add a method to CKey for computing a KeyPair, CKey::ComputeKeyPair. This class is a wrapper for the `secp256k1_keypair` type. The motivation is to be able to use the the tweaked internal key outside of signing, e.g. in silent payments when retreiving the private key before ECDH. Having the KeyPair class is also a general improvement in that we almost always convert to `secp256k1_keypair` objects when using taproot private keys with libsecp256k1. Co-authored-by: Cory Fields <[email protected]>
Reuse existing bip340 test vectors.
|
There were 36 issue comments left by 4 reviewers for the pull request |
![fresheyes-staging-bot[bot]](https://avatars.githubusercontent.com/in/855196?s=60&v=4){kind=small}
| * (this is used for key path spending, with specific | ||
| * Merkle root of the script tree). | ||
| */ | ||
| KeyPair ComputeKeyPair(const uint256* merkle_root) const; |
There was a problem hiding this comment.
2 authors commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1596806833at 2024/05/10, 14:11:53 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1596979700at 2024/05/10, 16:44:18 UTC.
| // Repeat the same check, but use the KeyPair directly without any merkle tweak | ||
| KeyPair keypair = key.ComputeKeyPair(/*merkle_root=*/nullptr); | ||
| CKey keypair_seckey; | ||
| BOOST_CHECK(keypair.GetKey(keypair_seckey)); |
There was a problem hiding this comment.
2 authors commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1597605164at 2024/05/12, 10:16:07 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1598114179at 2024/05/13, 08:55:21 UTC.
| private: | ||
| KeyPair(const CKey& key, const uint256* merkle_root); | ||
|
|
||
| using KeyType = std::array<unsigned char, 96>; |
There was a problem hiding this comment.
2 authors commented here with:
- comment link
https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1597605647at 2024/05/12, 10:18:42 UTC - comment link
https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1598119960at 2024/05/13, 08:59:33 UTC.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The author josibake wrote the following PR called crypto, refactor: add new KeyPair class, issue number 30051 in bitcoin/bitcoin cloned by FreshEyes below:
Broken out from
#28201The wallet returns an untweaked internal key for taproot outputs. If the output commits to a tree of scripts, this key needs to be tweaked with the merkle root. Even if the output does not commit to a tree of scripts, BIP341/342 recommend commiting to a hash of the public key.
Previously, this logic for applying the taptweak was implemented in the
CKey::SignSchnorrmethod.This PR moves introduces a KeyPair class which wraps a
secp256k1_keypairtype and refactors SignSchnorr to use this new KeyPair. The KeyPair class is created with an optional merkle_root argument and the logic from BIP341 is applied depending on the state of the merkle_root argument.The motivation for this refactor is to be able to use the tap tweak logic outside of signing, e.g. in silent payments when retrieving the private key (see
#28201).Outside of silent payments, since we almost always convert a
CKeyto asecp256k1_keypairwhen doing anything with taproot keys, it seems generally useful to have a way to model this type in our code base.