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	ismaelsadeeq, paplorinc, itornaza, theStack
Stale 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.

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

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?

src/key.h

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 :)

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

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

theuni

ACK bdc2a65

src/test/key_tests.cpp

src/key.h

Sanity check that using CKey/CPubKey directly vs using secp256k1_keypair objects returns the same results for BIP341 key tweaking. Co-authored-by: l0rinc <[email protected]>

Add a `KeyPair` class which wraps the `secp256k1_keypair`. This keeps the secret data in secure memory and enables passing the `KeyPair` object directly to libsecp256k1 functions expecting a `secp256k1_keypair`. Motivation: when passing `CKeys` for taproot outputs to libsecp256k1 functions, the first step is to create a `secp256k1_keypair` data type and use that instead. This is so the libsecp256k1 function can determine if the key needs to be negated, e.g., when signing. This is a bit clunky in that it creates an extra step when using a `CKey` for a taproot output and also involves copying the secret data into a temporary object, which the caller must then take care to cleanse. In addition, the logic for applying the merkle_root tweak currently only exists in the `SignSchnorr` function. In a later commit, we will add the merkle_root tweaking logic to this function, which will make the merkle_root logic reusable outside of signing by using the `KeyPair` class directly. Co-authored-by: Cory Fields <[email protected]>

josibake · 2024-08-03T13:29:54Z

Update 9afa2c9 -> bfb2e6b (apply-taptweak-method-06-rebase -> apply-taptweak-method-07 (compare)

Add SignSchnorrTapTweakBenchmark (h/t @paplorinc)
- Modified the name from @paplorinc 's version and changed it to use minEpochIterations(100) instead of batch(1)
Removed public data() method. Since this is method is not currently used, it seems better to add this in the Silent Payments sending PR (as GetKeyPairData()) where it will be used
Renamed variables in the schnorr tweak smoke test
Replaced early returns in the if merkle_root block with asserts

l0rinc

utACK bfb2e6b

Thanks for your perseverance!

l0rinc · 2024-08-03T16:13:48Z

src/key.cpp

-    bool ret = secp256k1_schnorrsig_sign32(secp256k1_context_sign, sig.data(), hash.data(), &keypair, aux.data());
+    KeyPair kp = ComputeKeyPair(merkle_root);
+    if (!kp.IsValid()) return false;
+    auto keypair = reinterpret_cast<const secp256k1_keypair*>(kp.data());


This fails on the commit-by-commit CI task, we still need the getter in this commit - can be removed in the next where we'll access it directly

Ah, oversight on my part. I meant to combine these two commits in the last push but forgot 😅 The combined commit is still easy enough to review and I think this is more clear than adding a getter and then removing it in the next commit.

Move `SignSchnorr` to `KeyPair`. This makes `CKey::SignSchnorr` now compute a `KeyPair` object and then call `KeyPair::SignSchorr`. The notable changes are: * Move the merkle_root tweaking out of the sign function and into the KeyPair constructor * Remove the temporary secp256k1_keypair object and have the functions access m_keypair->data() directly

Reuse existing BIP340 tests, as there should be no behavior change between the two

Replace early returns in KeyPair::KeyPair() with asserts. The if statements imply there is an error we are handling, but keypair_xonly_pub and xonly_pubkey_serialize can only fail if the keypair object is malformed, i.e., it was created with a bad secret key. Since we check that the keypair was created successfully before attempting to extract the public key, using asserts more accurately documents what we expect here and removes untested branches from the code.

ismaelsadeeq

Code review ACK ec973dd

ismaelsadeeq · 2024-08-05T08:20:13Z

src/test/key_tests.cpp

+BOOST_AUTO_TEST_CASE(key_schnorr_tweak_smoke_test)
+{
+    // Sanity check to ensure we get the same tweak using CPubKey vs secp256k1 functions
+    secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);


nit:
secp256k1_context_create docs says

The only valid non-deprecated flag in recent library versions is * SECP256K1_CONTEXT_NONE, which will create a context sufficient for all functionality * offered by the library. All other (deprecated) flags will be treated as equivalent * to the SECP256K1_CONTEXT_NONE flag.

Suggested change

secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);

secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_NONE);

Ah, nice! Good catch. Will change if I end up retouching.

If you do that, please change other usages as well

Yeah, did a quick grep and it looks there is at least one other place in the fuzz tests where _SIGN is used, so this one is probably best as a follow up PR to replace all instances with _NONE in the codebase and perhaps add a mention to the developer notes.

ismaelsadeeq · 2024-08-05T09:24:26Z

src/key.cpp

+        secp256k1_xonly_pubkey pubkey;
+        if (!secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, keypair)) return;
+        unsigned char pubkey_bytes[32];
+        if (!secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey)) return;


nit: Will be nice to have a wrapper for this that generates a serialized x-only public key.
their is a dup of this same code in the test.

Not sure if this is still applicable after the change in the last commit to replace the if branches with asserts, but will take a look and updated if I end up needing to retouch.

src/key.cpp

src/test/key_tests.cpp

ismaelsadeeq · 2024-08-05T09:39:39Z

src/key.cpp

    static_assert(std::tuple_size<KeyType>() == sizeof(secp256k1_keypair));
    MakeKeyPairData();
    auto keypair = reinterpret_cast<secp256k1_keypair*>(m_keypair->data());
    bool success = secp256k1_keypair_create(secp256k1_context_sign, keypair, UCharCast(key.data()));


nice adding ec973dd

ismaelsadeeq · 2024-08-05T09:49:01Z

src/test/key_tests.cpp

+    BOOST_CHECK(secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey_old, nullptr, &keypair));
+    unsigned char pubkey_bytes_old[32];
+    BOOST_CHECK(secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes_old, &pubkey_old));
+    uint256 tweak_old = XOnlyPubKey(pubkey_bytes_old).ComputeTapTweakHash(merkle_root.IsNull() ? nullptr : &merkle_root);


The other XOnlyPubKey constructor is just stripping the 2...32 bytes of the CPubKey and calling the same constructor called with xonly_bytes.

l0rinc · 2024-08-05T10:51:42Z

ACK ec973dd - will happily reack if you decide to apply @ismaelsadeeq's suggestions

itornaza

trACK ec973dd

Rebuilt the commit, run all unit, functional and extended tests and all of them pass.

theStack

Code-review ACK ec973dd

Left a few non-critical nits below, feel free to ignore.

theStack · 2024-08-06T16:46:36Z

src/test/key_tests.cpp

+    CKey key;
+    key.MakeNewKey(true);


nit, if you have to retouch:

Suggested change

CKey key;

key.MakeNewKey(true);

CKey key = GenerateRandomKey();

theStack · 2024-08-06T17:38:19Z

src/test/key_tests.cpp

+    uint256 tweak_old = XOnlyPubKey(xonly_bytes).ComputeTapTweakHash(&merkle_root);
+
+    // CPubKey
+    CPubKey pubkey = key.GetPubKey();
+    uint256 tweak_new = XOnlyPubKey(pubkey).ComputeTapTweakHash(&merkle_root);


I think this effectively only tests that given a certain private key, the same XOnlyPubKey objects result with both used methods (once via the secp256k1 keypair functions, once with .GetPubKey()). So strictly speaking computing and comparing the tweak hashes on top of that isn't needed and could be removed, but no strong feelings about that, as it also doesn't hurt. (Seems like this was already discussed in #30051 (comment) ff., so feel free to ignore).

This was the result of a change that was reverted since, where it was important to test the exact before/after structure.

theStack · 2024-08-06T17:55:09Z

src/key.cpp

+        assert(secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, keypair));
+        assert(secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey));


nit: AFAIR, for a long time we preferred to store the return value in a ret boolean and assert only on that (see e.g. $ git grep ret.*secp256k1 vs $ git grep assert.*secp256k1), but not sure if we still have a developer guideline like "don't use assert with side-effects" in place (I think we once had, and removed it, so this way seems to be fine.)

We've had something like that before: #30051 (comment)

But it was ambiguous, as to whether it can fail or not during execution as we can see here:
https://github.com/hebasto/bitcoin/blob/master/src/pubkey.cpp#L345-L350 or https://github.com/hebasto/bitcoin/blob/master/src/secp256k1/src/secp256k1.c#L685

But @josibake argued that they cannot fail at that point - if they do it's a bug, but we didn't want to ignore the return values either. So we ended up asserting, like we did in GetPubKey already.

hebasto · 2024-08-09T14:08:51Z

The buildsystem related stuff has been ported to the CMake-based buildsystem in hebasto#317.

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

src/key.cpp Outdated Show resolved Hide resolved

src/key.cpp Outdated Show resolved Hide resolved

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

src/key.h Outdated Show resolved Hide resolved

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

l0rinc reviewed May 12, 2024

View reviewed changes

src/test/key_tests.cpp Outdated Show resolved Hide resolved

src/key.h Show resolved Hide resolved

This was referenced May 28, 2024

CKey: add Serialize and Unserialize #29295

Closed

Stratum v2 Template Provider (take 3) #29432

Closed

josibake and others added 2 commits August 3, 2024 15:16

tests: add key tweak smoke test

5d507a0

Sanity check that using CKey/CPubKey directly vs using secp256k1_keypair objects returns the same results for BIP341 key tweaking. Co-authored-by: l0rinc <[email protected]>

josibake force-pushed the add-apply-taptweak-method branch from 34e7014 to bfb2e6b Compare August 3, 2024 13:16

l0rinc approved these changes Aug 3, 2024

View reviewed changes

DrahtBot requested review from ismaelsadeeq and itornaza August 3, 2024 14:01

l0rinc reviewed Aug 3, 2024

View reviewed changes

josibake force-pushed the add-apply-taptweak-method branch from bfb2e6b to 8a878f5 Compare August 4, 2024 06:50

josibake added 3 commits August 4, 2024 08:51

tests: add tests for KeyPair

72a5822

Reuse existing BIP340 tests, as there should be no behavior change between the two

josibake force-pushed the add-apply-taptweak-method branch from 8a878f5 to ec973dd Compare August 4, 2024 06:52

DrahtBot added CI failed and removed CI failed labels Aug 4, 2024

ismaelsadeeq approved these changes Aug 5, 2024

View reviewed changes

DrahtBot requested a review from l0rinc August 5, 2024 10:34

itornaza approved these changes Aug 5, 2024

View reviewed changes

theStack approved these changes Aug 6, 2024

View reviewed changes

ryanofsky self-assigned this Aug 7, 2024

ryanofsky merged commit b38fb19 into bitcoin:master Aug 7, 2024

hebasto added the Needs CMake port label Aug 9, 2024

This was referenced Aug 9, 2024

build: Introduce CMake-based build system #30454

Merged

cmake: Port PR30051 from the master branch hebasto/bitcoin#317

Merged

hebasto removed the Needs CMake port label Aug 9, 2024

bitcoin deleted a comment Aug 12, 2024

bitcoin locked and limited conversation to collaborators Aug 12, 2025

	secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
	secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_NONE);

	CKey key;
	key.MakeNewKey(true);
	CKey key = GenerateRandomKey();

		assert(secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, keypair));
		assert(secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey));

crypto, refactor: add new KeyPair class #30051

crypto, refactor: add new KeyPair class #30051

Uh oh!

Conversation

josibake commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

josibake commented May 7, 2024

Uh oh!

theuni commented May 7, 2024

Uh oh!

josibake commented May 8, 2024

Uh oh!

Uh oh!

Uh oh!

josibake commented May 10, 2024

Uh oh!

theuni left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

josibake commented May 10, 2024

Uh oh!

josibake commented May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

theuni commented May 10, 2024

Uh oh!

josibake commented May 10, 2024

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

josibake commented Aug 3, 2024

Uh oh!

l0rinc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

josibake commented May 6, 2024 •

edited

Loading

DrahtBot commented May 6, 2024 •

edited

Loading

theuni left a comment •

edited

Loading

josibake commented May 10, 2024 •

edited

Loading

l0rinc left a comment •

edited

Loading

ismaelsadeeq Aug 5, 2024 •

edited

Loading

itornaza left a comment •

edited

Loading