crypto, refactor: add new KeyPair class #30051

josibake · 2024-05-06T11:00:17Z

Broken out from #28201

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 CKey::SignSchnorr method.

This PR moves introduces a KeyPair class which wraps a secp256k1_keypair type 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 CKey to a secp256k1_keypair when doing anything with taproot keys, it seems generally useful to have a way to model this type in our code base.

DrahtBot · 2024-05-06T11:00:20Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

theuni

This function feels kinda weird. As it's named ApplyTapTweak, I would expect it to modify *this. It also takes in a uint256* merkle_root but doesn't check for null, a reference would make more sense.

Perhaps this would make more sense as a static utility function that takes input/output keys?

josibake · 2024-05-07T09:51:01Z

This function feels kinda weird. As it's named ApplyTapTweak, I would expect it to modify *this. It also takes in a uint256* merkle_root but doesn't check for null, a reference would make more sense.

Perhaps this would make more sense as a static utility function that takes input/output keys?

Utility function might make more sense here: could do a utility function with secp256k1_keypairs as in/out args:

int compute_taptweak(secp256k1_keypair in, unsigned char merkle_root, secp256k1_keypair out)

and use that inside ::SignSchnorr. This means we still only creating one secp256k1_keypair when signing. For usage outside of signing, wrap that utility function in a function takes and returns CKeys as arguments:

CKey tweaked_key ComputeTapTweak(CKey& internal_key, uint256& merkle_root) {
    // .. do stuff
    compute_taptweak(..);
    return tweaked_key;

WDYT?

theuni · 2024-05-07T14:21:27Z

ComputeTapTweak is more like what I had in mind, yes.

josibake · 2024-05-08T10:05:57Z

Updated to use a utility function (instead of a method on CKey) per the @theuni 's feedback

theuni · 2024-05-08T16:44:21Z

src/key.cpp

-        if (!secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey)) return false;
-        uint256 tweak = XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);
-        if (!secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, &keypair, tweak.data())) return false;
+        CKey tweaked_key;


Will anything ever use this key directly? Or will it always just be turned back into a secp256k1_keypair? It seems the CKey output is just a wrapper that causes overhead here. I guess for the sake of not exposing secp256k1_keypair to the caller?

Yep! From the description:

The motivation is to be able to use the ::ApplyTapTweak logic outside of signing, e.g. in silent payments when retrieving the private key. Outside of silent payments, having this method would support any future use cases where the tweaked key is needed outside of signing.

This commit was broken out of the silent payments sending PR, because there we need the tweaked CKey. However, we do eventually turn it back into a keypair when passing it to libsecp, so maybe there is an argument for just returning the keypair? But passing around libsecp types seemed a bit weird..

Hmm, if what you need is a wrapped keypair, how about creating a wrapped keypair? :) theuni@db1d199

^^ Completely untested. Maybe state via ApplyTapTweak is unnecessary as the ctor could just take a pointer to a merkle root instead?

Nice, thanks for the suggestion! This makes a ton more sense. I think it would be better to have the ctor take a pointer to the merkle root because ApplyTapTweak is something that you a) only want to do once over the lifetime of the object and b) will always be applied if a merkle_root is present (even if its merkle_root.IsNull() == true). I don't think this is actually a use case, but if for whatever reason you did need to do something with the key and then later apply a merkle root tweak, you could just use the CKey first and then create a KeyPair(*this, merkle_root) with the merkle root when needed.

Also, what do you think about something like KeyPair CKey::ComputeKeyPair(*merkle_root); method, instead of KeyPair(CKey, merkle_root)?

Will pull this in and test it in #28122 and #28201.

Please consider the above a doodle, change it however you'd like. I just wanted to sketch out the concept of a wrapped keypair.

Yeah, for sure! But thanks, very helpful doodle. Not sure why I was so apprehensive about passing around a keypair, but seeing you write it out helped it click for me.

Updated as suggested here: https://github.com/theuni/bitcoin/commits/keypairtaptweak/

Awesome, thanks @theuni ! I pulled this commit in and also add a "simple read only vector like interface" to the KeyPair class (same as CKey). Needed for: https://github.com/josibake/bitcoin/blob/33868a74dc7b3403ff38343899c37feb4c88d0c6/src/common/bip352.cpp#L204
I also rebased #28122 on this PR and confirmed everything works with the new KeyPair class.

That looks much better, thanks. Exposing the vector-like interface is a shame, but I don't see any way around it for secp256k1_silentpayments_sender_create_outputs.

src/key.cpp

josibake · 2024-05-10T12:41:31Z

Updated to use the new KeyPair wrapper class (h/t @theuni).

theuni

It's weird to see this hooked up but unused. It could also use some simple test vectors.

Mind killing both birds by adding some tests, at least 1 for each merkle_root state?

theuni · 2024-05-10T14:11:53Z

src/key.h

@@ -203,6 +205,8 @@ class CKey
    ECDHSecret ComputeBIP324ECDHSecret(const EllSwiftPubKey& their_ellswift,
                                       const EllSwiftPubKey& our_ellswift,
                                       bool initiating) const;
+
+    KeyPair ComputeKeyPair(const uint256* merkle_root) const;


This could use a comment.

josibake · 2024-05-10T16:46:14Z

@theuni Updated with a comment and added KeyPair to the BIP340 test vectors. This does test all possible merkle_root states and ensures everything is 1-to-1 with XOnlyPubKey::ComputeTapTweakHash and CKey::SignSchnorr

josibake · 2024-05-10T17:38:37Z

Updated to add a move constructor to the KeyPair class. I noticed this was missing while trying to use the new class in #28201 (i.e. creating a std::vector of KeyPairs). @theuni assuming this was just missed, but if not curious if you have any objections to adding a move constructor on KeyPair?

EDIT: also rebased on master to fix conflicts

theuni

Mind changing the dumb c-style casts to reinterpret_cast so it's clear that they can't be static_casts ? Sorry, I know that's my code.

utACK after that.

theuni · 2024-05-10T17:48:02Z

PR description needs an update too :)

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 <cory-nospam-@coryfields.com>

Reuse existing bip340 test vectors.

josibake · 2024-05-10T17:59:18Z

@theuni title, description, and dumb c-style casts updated!

theuni

ACK bdc2a65

paplorinc · 2024-05-12T10:16:07Z

src/test/key_tests.cpp

+        // 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));


nit: BOOST_CHECK_MESSAGE often produces more readable results, but it may be inconvenient to set it up properly. If you think it adds value, it may be helpful to add some debug info as a message in case of likely failures.

I don't think it adds much here? We are using BOOST_CHECK to to ensure the function call succeeds before proceeding, so getting a line number failure for BOOST_CHECK seems sufficient for debugging. Also, if dealing with valid keys, this function should never fail.

paplorinc · 2024-05-12T10:18:42Z

src/key.h

+private:
+    KeyPair(const CKey& key, const uint256* merkle_root);
+
+    using KeyType = std::array<unsigned char, 96>;


nit: would it make sense to extract the 96 to a const?

constexpr size_t SECP256K1_KEYPAIR_SIZE = 96; using KeyType = std::array<unsigned char, SECP256K1_KEYPAIR_SIZE>;

I don't really see a benefit. We shouldn't be exposing this value to anything outside the class, so defining a constexpr here seems unnecessarily verbose.

josibake mentioned this pull request May 6, 2024

Silent Payments: sending #28201

Draft

bitcoin deleted a comment from Cinnamonrou May 6, 2024

theuni reviewed May 6, 2024

View reviewed changes

josibake force-pushed the add-apply-taptweak-method branch from e8d1b22 to 6cc72ce Compare May 8, 2024 10:04

josibake mentioned this pull request May 8, 2024

BIP352 tracking issue #28536

Open

15 tasks

theuni reviewed May 8, 2024

View reviewed changes

josibake force-pushed the add-apply-taptweak-method branch 3 times, most recently from f92dde3 to 0caa324 Compare May 10, 2024 12:17

josibake force-pushed the add-apply-taptweak-method branch from 0caa324 to 8d33daf Compare May 10, 2024 14:12

theuni reviewed May 10, 2024

View reviewed changes

josibake force-pushed the add-apply-taptweak-method branch from 03d98c0 to e82c2c7 Compare May 10, 2024 16:43

josibake force-pushed the add-apply-taptweak-method branch from e82c2c7 to c2c5e72 Compare May 10, 2024 17:36

theuni reviewed May 10, 2024

View reviewed changes

josibake and others added 2 commits May 10, 2024 19:52

tests: add test for KeyPair

bdc2a65

Reuse existing bip340 test vectors.

josibake force-pushed the add-apply-taptweak-method branch from c2c5e72 to bdc2a65 Compare May 10, 2024 17:53

josibake changed the title ~~crypto, refactor: add method for applying the taptweak~~ crypto, refactor: add new KeyPair class May 10, 2024

theuni approved these changes May 10, 2024

View reviewed changes

paplorinc reviewed May 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto, refactor: add new KeyPair class #30051

crypto, refactor: add new KeyPair class #30051

josibake commented May 6, 2024 •

edited

DrahtBot commented May 6, 2024 •

edited

theuni left a comment

josibake commented May 7, 2024

theuni commented May 7, 2024

josibake commented May 8, 2024

theuni May 8, 2024

josibake May 8, 2024

theuni May 8, 2024 •

edited

josibake May 9, 2024

theuni May 9, 2024

josibake May 9, 2024

theuni May 9, 2024

josibake May 10, 2024 •

edited

theuni May 10, 2024

josibake commented May 10, 2024

theuni left a comment •

edited

theuni May 10, 2024

josibake May 10, 2024

josibake commented May 10, 2024

josibake commented May 10, 2024 •

edited

theuni left a comment

theuni commented May 10, 2024

josibake commented May 10, 2024

theuni left a comment

paplorinc May 12, 2024

josibake May 13, 2024

paplorinc May 12, 2024

josibake May 13, 2024

crypto, refactor: add new KeyPair class #30051

Are you sure you want to change the base?

crypto, refactor: add new KeyPair class #30051

Conversation

josibake commented May 6, 2024 • edited

DrahtBot commented May 6, 2024 • edited

Code Coverage

Reviews

theuni left a comment

Choose a reason for hiding this comment

josibake commented May 7, 2024

theuni commented May 7, 2024

josibake commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josibake May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josibake commented May 10, 2024

theuni left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josibake commented May 10, 2024

josibake commented May 10, 2024 • edited

theuni left a comment

Choose a reason for hiding this comment

theuni commented May 10, 2024

josibake commented May 10, 2024

theuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josibake commented May 6, 2024 •

edited

DrahtBot commented May 6, 2024 •

edited

theuni May 8, 2024 •

edited

josibake May 10, 2024 •

edited

theuni left a comment •

edited

josibake commented May 10, 2024 •

edited