crypto, refactor: add new KeyPair class

josibake commented at 11:00 am on May 6, 2024: member

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 commented at 11:00 am on May 6, 2024: contributor

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29432 (Stratum v2 Template Provider (take 3) by Sjors)
#29295 (CKey: add Serialize and Unserialize by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

bitcoin deleted a comment on May 6, 2024

theuni commented at 5:59 pm on May 6, 2024: member

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 commented at 9:51 am on May 7, 2024: member

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:

0int 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:

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

WDYT?

theuni commented at 2:21 pm on May 7, 2024: member

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

josibake force-pushed on May 8, 2024

josibake commented at 10:05 am on May 8, 2024: member

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

in src/key.cpp:314 in 6cc72ced11 outdated

316-        if (!secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, &keypair)) return false;
317-        unsigned char pubkey_bytes[32];
318-        if (!secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey)) return false;
319-        uint256 tweak = XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);
320-        if (!secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, &keypair, tweak.data())) return false;
321+        CKey tweaked_key;

theuni commented at 4:44 pm on May 8, 2024:

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?

josibake commented at 6:26 pm on May 8, 2024:

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..

theuni commented at 8:08 pm on May 8, 2024:

Hmm, if what you need is a wrapped keypair, how about creating a wrapped keypair? :) https://github.com/theuni/bitcoin/commit/db1d199bf93eae046a62245db8f7782a94f25e9d

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

josibake commented at 7:46 am on May 9, 2024:

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.

theuni commented at 12:37 pm on May 9, 2024:

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

josibake commented at 12:54 pm on May 9, 2024:

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.

theuni commented at 6:22 pm on May 9, 2024:

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

josibake commented at 12:33 pm on May 10, 2024:

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.

theuni commented at 2:19 pm on May 10, 2024:

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.

in src/key.cpp:317 in 6cc72ced11 outdated

319-        uint256 tweak = XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);
320-        if (!secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, &keypair, tweak.data())) return false;
321+        CKey tweaked_key;
322+        if (!ComputeTapTweak(*this, *merkle_root, tweaked_key)) return false;
323+        ret = secp256k1_keypair_create(secp256k1_context_sign, &keypair, UCharCast(tweaked_key.begin()));
324+        tweaked_key = CKey();

theuni commented at 4:46 pm on May 8, 2024:

This is going out of scope anyway, no need?

josibake force-pushed on May 10, 2024

josibake commented at 12:41 pm on May 10, 2024: member

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

in src/key.h:221 in 0caa3241b0 outdated

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

theuni commented at 2:11 pm on May 10, 2024:

This could use a comment.

josibake commented at 4:44 pm on May 10, 2024:

Done.

josibake force-pushed on May 10, 2024

theuni commented at 2:16 pm on May 10, 2024: member

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?

josibake force-pushed on May 10, 2024

josibake commented at 4:46 pm on May 10, 2024: member

@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 force-pushed on May 10, 2024

josibake commented at 5:38 pm on May 10, 2024: member

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 commented at 5:43 pm on May 10, 2024: member

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 commented at 5:48 pm on May 10, 2024: member

PR description needs an update too :)

crypto: Add a KeyPair wrapper class

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>

54b3c73ec5

tests: add test for KeyPair

Reuse existing bip340 test vectors.

bdc2a656c2

josibake force-pushed on May 10, 2024

josibake renamed this:
~~crypto, refactor: add method for applying the taptweak~~
crypto, refactor: add new KeyPair class
on May 10, 2024

josibake commented at 5:59 pm on May 10, 2024: member

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

theuni approved

theuni commented at 6:35 pm on May 10, 2024: member

ACK bdc2a656c2d2a61d226fde1d1fd4e79664106e18

in src/test/key_tests.cpp:333 in bdc2a656c2

326@@ -327,6 +327,16 @@ BOOST_AUTO_TEST_CASE(bip340_test_vectors)
327         // Verify those signatures for good measure.
328         BOOST_CHECK(pubkey.VerifySchnorr(msg256, sig64));
329 
330+        // Repeat the same check, but use the KeyPair directly without any merkle tweak
331+        KeyPair keypair = key.ComputeKeyPair(/*merkle_root=*/nullptr);
332+        CKey keypair_seckey;
333+        BOOST_CHECK(keypair.GetKey(keypair_seckey));

paplorinc commented at 10:16 am on May 12, 2024:

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.

josibake commented at 8:55 am on May 13, 2024:

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.

in src/key.h:286 in bdc2a656c2

281+    const std::byte* begin() const { return data(); }
282+    const std::byte* end() const { return data() + size(); }
283+private:
284+    KeyPair(const CKey& key, const uint256* merkle_root);
285+
286+    using KeyType = std::array<unsigned char, 96>;

paplorinc commented at 10:18 am on May 12, 2024:

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

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

josibake commented at 8:59 am on May 13, 2024:

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.

crypto, refactor: add new KeyPair class #30051

Code Coverage

Reviews

Conflicts