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

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

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 outdated

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:297 in bdc2a656c2 outdated

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.

in src/key.h:256 in bdc2a656c2 outdated

251@@ -236,6 +252,41 @@ struct CExtKey {
252     void SetSeed(Span<const std::byte> seed);
253 };
254 
255+/** KeyPair

itornaza commented at 4:53 pm on July 10, 2024:

The same header is already and very thoughtfully provided for the following function a few lines above

KeyPair ComputeKeyPair(const uint256* merkle_root) const;

Maybe here a shorter header would be more appropriate to avoid repeating ourselves and make maintenance easier to handle (at the comment level), like so

0/** KeyPair
1 *
2 *  Wrapper class for the `secp256k1_keypair` type.
3 */

josibake commented at 6:31 pm on July 16, 2024:

Hey @itornaza , thanks for the review! I’ve incorporated your feedback and moved the comment regarding the merkle root to the ComputeKeyPair function and made this comment more specific to the class itself.

itornaza approved

itornaza commented at 5:03 pm on July 10, 2024: none

tested ACK bdc2a656c2d2a61d226fde1d1fd4e79664106e18

Built from source and run all unit, functional and extended tests and all of them pass. Looked through the code changes and they look great to me. I particularly like the default definition of the move constructors in KeyPair since there is a pointer member m_keydata in that class, plus the fact that the generation of the copy constructors that are not needed anywhere is suppressed due to the use of the default keyword which I consider an excellent programming practice. One small suggestion about repeating the KeyPair header for your consideration if you ever need to update for other more serious reasons.

in src/key.cpp:274 in 54b3c73ec5 outdated

276@@ -277,27 +277,7 @@ bool CKey::SignCompact(const uint256 &hash, std::vector<unsigned char>& vchSig)
277 
278 bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256* merkle_root, const uint256& aux) const
279 {
280-    assert(sig.size() == 64);

paplorinc commented at 6:36 pm on July 10, 2024:

Moving and modifying the code in the same commit (while many of the changes serve different purposes, mixing low and high-risk changes) makes the review a lot harder since the diff isn’t really useful – having separate red and green parts, not really a diff.

Additionally, the commit doesn’t explain each change separately. For example:

The exact reason for memory management changes (via make_secure_unique) is not explained, nor are the security implications or potential performance impacts discussed.
It’s unclear why GetKey is only used in tests. Why not delete it and introduce it in another PR where it’s actually needed by production code?
The fate of memory_cleanse(&keypair, sizeof(keypair)) is not explained, nor are the implications of its removal or replacement addressed.

It would help a lot if a commit did a single thing, such as a rename or cast change, changing a constant, moving implementation over to a new method, introducing a new class, or splitting a method into two methods, etc - where the commit message explains why the change was necessarry. This would separate trivial changes from high-risk ones that need extra attention, so we don’t have unrelated changes competing for our attention.

Please see https://github.com/bitcoin/bitcoin/pull/28280/commits for an example of focused, single-purpose commits that separate low and high-risk changes (e.g., preparatory refactorings from the focus of the PR), making the review a lot easier.

josibake commented at 6:21 pm on July 16, 2024:

Hey @paplorinc , thanks for taking a look! I agree, the first commit was quite dense. I’ve broken the first commit into smaller commits to aid with review and tried to include more information in the commit messages as to the motivation for the changes. Per your feedback, I’ve also removed the GetKey method as its not necessary.

Shuhaib07 commented at 4:16 am on August 12, 2024:

Okay

paplorinc changes_requested

paplorinc commented at 6:38 pm on July 10, 2024: contributor

Not yet sure about adding stuff that “will hopefully used this way one day”.

Refactoring is welcome, as long as it’s easy to review, which is not yet the case here, could you split the change into trivial, focused commits?

josibake force-pushed on Jul 16, 2024

josibake commented at 6:19 pm on July 16, 2024: member

Updated https://github.com/bitcoin/bitcoin/commit/bdc2a656c2d2a61d226fde1d1fd4e79664106e18 -> https://github.com/bitcoin/bitcoin/commit/5e9f4f41de98c2cdcc74e31fe925b2f5cf9f6aec (add-apply-taptweak-method-00 -> add-apply-taptweak-method-01 (compare)

Follows some CKey conventions with MakeKeyPairData, ClearKeyPairData, etc
Add copy constructors (same as CKey) - this is necessary to be able to hold KeyPair objects in stl containers, e.g., std::vector<KeyPair>
Breaks up the first commit into smaller, more focused commits to aid review (h/t @paplorinc)

Not much in terms of logical changes from the previous version, but broken into incremental steps to aid with review. Worth mentioning for reviewers: [staging] commits are changes that should not be considered final on their own but are incremental steps to make the overall change easier to review. Each commit is still atomic in that it compiles on its own, but having them in smaller steps like this helps quite a bit in understanding the diffs. However, if reviewers feel this is unnecessary, I’m happy to squash these staging commits back into a single commit.

This push also includes a rebase on master, but the compare is provided with both the old and new branch rebased on master.

josibake commented at 6:27 pm on July 16, 2024: member

cc @theuni - would appreciate your eyes on this again

in src/key.cpp:287 in 4e8d61e750 outdated

286-        if (!secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, &keypair, tweak.data())) return false;
287+        ret &= secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, &keypair, tweak.data());
288     }
289-    bool ret = secp256k1_schnorrsig_sign32(secp256k1_context_sign, sig.data(), hash.data(), &keypair, aux.data());
290+    if (!ret) return false;
291+    ret &= secp256k1_schnorrsig_sign32(secp256k1_context_sign, sig.data(), hash.data(), &keypair, aux.data());

paplorinc commented at 1:40 pm on July 17, 2024:

nit: it might be simplified to ret = - or even bool ret = if we narrow the scope of the previous one.

josibake commented at 3:46 pm on July 20, 2024:

Unless there is a functional disadvantage to having it this way, I prefer this for readability as it makes it more clear (to me, anyways) that we are accumulating the return values from the various secp256k1 function calls in the ret variable.

paplorinc commented at 5:14 pm on July 20, 2024:

it’s narrowing the scope, what would be the reason for reusing a variable and widening its scope? If you feel strongly about it, I don’t particularly mind either way.

paplorinc commented at 10:21 am on July 21, 2024:

resolved

in src/key.cpp:275 in 4e8d61e750 outdated

271@@ -272,21 +272,23 @@ bool CKey::SignCompact(const uint256 &hash, std::vector<unsigned char>& vchSig)
272 bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256* merkle_root, const uint256& aux) const
273 {
274     assert(sig.size() == 64);
275+    bool ret = 1;

paplorinc commented at 2:43 pm on July 17, 2024:

nit: my understanding is that you’ve chosen this style to mimic the calls in secp256k1_keypair_xonly_pub and friends - but maybe it would make sense to migrate to C++ style here, I couldn’t find any other bool in cpp files where we’re assigning ints, like we do in the C files, i.e. bool ret = true - or rather use it for the first assignment instead.

josibake commented at 3:42 pm on July 20, 2024:

Agree, more readable to have this as bool ret = true;, updated.

in src/key.cpp:286 in 4e8d61e750 outdated

285         uint256 tweak = XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);
286-        if (!secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, &keypair, tweak.data())) return false;
287+        ret &= secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, &keypair, tweak.data());
288     }
289-    bool ret = secp256k1_schnorrsig_sign32(secp256k1_context_sign, sig.data(), hash.data(), &keypair, aux.data());
290+    if (!ret) return false;

paplorinc commented at 2:44 pm on July 17, 2024:

we do not change behavior by exiting early before attempting to sign if there is a failure with the merkle_root logic.

Previously, when an error occurred, subsequent calculations weren’t performed. Since &= doesn’t short-circuit (and even if it did, pubkey_bytes would be allocated and ComputeTapTweakHash would run even if secp256k1_keypair_xonly_pub failed), the methods could be called even though a previous dependency failed. Are you sure this is just a refactoring and not a behavior change?

josibake commented at 2:14 pm on July 20, 2024:

I’m fairly confident this is not a behavior change, but could have missed something.

If you look at secp256k1_keypair_xonly_pub, this will only fail if there was an issue creating the keypair object, but this is checked above so we would have already short-circuited. The same is true for secp256k1_keypair_xonly_pubkey_serialize (only fails if we can’t load the keypair, which only fails if the keypair is malformed).

secp256k1_keypair_xonly_tweak_add will fail if tweak32 is greater than the curve order or if the tweak is the negation of the secret key.

Putting it all together, if we can’t create a valid keypair, the function will exit (same as before). If we do create a valid keypair, then the only error path we can hit is secp256k1_keypair_xonly_tweak_add and if we do hit an error here, we exit before attempting to sign (same as before).

paplorinc commented at 5:27 pm on July 20, 2024:

Your version always executes all steps (e.g. computes XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root); even after secp256k1_keypair_load wasn’t successful, right? Previously this method wasn’t executed, so at best it’s a performance difference, at worst we’re hoping that each method handles this error properly (which wasn’t necessary before, since the first method guarded them).

josibake commented at 6:59 pm on July 20, 2024:

Right, my point is there is no way to call CKey::SignSchnorr such that _keypair_create will succeed but _xonly_pub or _serialize will fail. Annoyingly, this means in the old version we have branches in the code that we cannot test (easy to prove me wrong if you have a test case :wink:). Hence, I stand by the statement this is not a behavior change and we are merely disagreeing over code aesthetics.

I think you make a good point that future changes to libsecp256k1 could change this (although, I’d argue if we can create a keypair object but cannot extract and serialize the pubkey portion something is seriously broken), so I can add back the early returns. However, I’ll reiterate that I find it to be a bit smelly when we have if branches in the code we cannot test.

paplorinc commented at 7:12 pm on July 20, 2024:

Do you agree that computations are done now, that would have been avoided previously? It might return the same value, but it does different calculations than before.

it to be a bit smelly when we have if branches in the code we cannot test.

absolutely, but I don’t yet see how avoiding short-circuiting helps with the testing - but exploding it into multiple methods certainly did!

josibake commented at 7:57 pm on July 20, 2024:

Do you agree that computations are done now, that would have been avoided previously?

No, this is the part I am disagreeing with. As I explained in the first message, there is no way for _xonly_pub or _serialize to fail if keypair_create succeeds. If _create, _xonly_pub, and _serialize succeed, then XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root); is computed. After that, _xonly_tweak_add will be called. _xonly_tweak_add can fail if tweak32 is greater than the curve order or if it is the negation of the private key that was used to create keypair. If this function fails, CKey::SignSchnorr will exit before attempting to sign, which is the same behavior in the old version and the new version.

absolutely, but I don’t yet see how avoiding short-circuiting helps with the testing

It helps by removing dead branches from the code. I’d be much happier to keep it broken out into multiple if branches if we had test cases to exercise the if branches. But if we have if branches that can realistically never be hit, I think it makes the code overly complicated. In this case, we end up with a more complicated function in the hopes of preventing an unnecessary call to ComputeTapTweakHash, which realistically will never happen and hardly seems worth it given this is a relatively cheap operation (and one without side effects).

paplorinc commented at 8:16 pm on July 20, 2024:

there is no way for _xonly_pub or _serialize to fail if keypair_create succeeds

I wish it were as clear in the code as it was before the change. I’ll review it more thoroughly tomorrow. If you can make the code more explicit by replacing confusing ret &= code with asserts where certain values can only be true, that would be helpful (if that’s what you’re stating).

josibake commented at 9:09 am on July 21, 2024:

Spent some time thinking about this and realised the main issue here is we start with a valid secret key, create a keypair (which generates the public key in the process), and then extract the public key from the keypair and serialise it. Instead, we can just get the public key directly from the secret key via CKey::GetPubKey. This makes the original code more clear, eliminates the unnecessary if branches and makes the diff much simpler.

If you look into GetPubKey you can see it’s calling the same libsecp256k1 functions to generate the public key, but asserting true for the serialisation and removes the unnecessary load steps.

paplorinc commented at 10:21 am on July 21, 2024:

beautiful!

theStack commented at 5:11 pm on July 22, 2024:

Spent some time thinking about this and realised the main issue here is we start with a valid secret key, create a keypair (which generates the public key in the process), and then extract the public key from the keypair and serialise it. Instead, we can just get the public key directly from the secret key via CKey::GetPubKey. This makes the original code more clear, eliminates the unnecessary if branches and makes the diff much simpler.

Not sure if we care that much about signing speed, but isn’t this almost certainly slower, as the privkey->pubkey step (i.e. point multiplication with generator G) has to be done twice now?

josibake commented at 9:20 am on July 23, 2024:

Yep, this will be slower (in cases where we don’t have a merkle root and instead use the public key per BIP341’s recommendation). I looked to see if we had a benchmark for signing to see if I could quantify this, but didn’t see one. Was thinking about writing one but then talked myself out of it because I don’t think performance is as much a concern in signing (as opposed to verification, where it absolutely is a concern).

In this case, I do think improving the clarity of the code and removing extra steps at the cost of performance seems worth it? I can also write a benchmark to quantify if performance here is indeed a concern.

theStack commented at 10:27 am on July 23, 2024:

Fwiw, we have a signing benchmark for taproot key-path spends, haven’t verified yet if it covers the discussed code path though: https://github.com/bitcoin/bitcoin/blob/910d38b22f575cba9a3325de3f4c5ac667d4a487/src/bench/sign_transaction.cpp#L67

In this case, I do think improving the clarity of the code and removing extra steps at the cost of performance seems worth it? I can also write a benchmark to quantify if performance here is indeed a concern.

No hard feelings on that topic from my side, my guess would indeed be that in the grand scheme of signing an extra generator point multiplication is barely noticable. Just wanted to point out that this could be a slight drawback.

josibake commented at 10:51 am on July 23, 2024:

Ah, nice! I’ll see if I can easily modify the benchmark to validate that the difference is indeed negligible (and quantify the difference in the event it is not negligible).

Just wanted to point out that this could be a slight drawback.

It’s a good observation! I have a slight preference for clarity over performance in this context, but good you mentioned it in case others feel differently.

paplorinc commented at 10:55 am on July 23, 2024:

it doesn’t cover the path, but this one does:

 0diff --git a/src/bench/sign_transaction.cpp b/src/bench/sign_transaction.cpp
 1--- a/src/bench/sign_transaction.cpp	(revision 8d573611575c3fa66f08407aa9b02f91b29a94c3)
 2+++ b/src/bench/sign_transaction.cpp	(date 1721731869277)
 3@@ -12,6 +12,7 @@
 4 #include <script/script.h>
 5 #include <script/sign.h>
 6 #include <uint256.h>
 7+#include <test/util/random.h>
 8 #include <util/translation.h>
 9 
10 enum class InputType {
11@@ -66,5 +67,33 @@
12 static void SignTransactionECDSA(benchmark::Bench& bench)   { SignTransactionSingleInput(bench, InputType::P2WPKH); }
13 static void SignTransactionSchnorr(benchmark::Bench& bench) { SignTransactionSingleInput(bench, InputType::P2TR);   }
14 
15+static void SignSchnorrBenchmark(benchmark::Bench& bench, bool use_null_merkle_root)
16+{
17+    ECC_Context ecc_context{};
18+
19+    auto key = GenerateRandomKey();
20+    auto msg = InsecureRand256();
21+    auto merkle_root = use_null_merkle_root ? uint256() : InsecureRand256();
22+    auto aux = InsecureRand256();
23+    std::vector<unsigned char> sig(64);
24+
25+    bench.batch(1).unit("signature").run([&] {
26+        bool success = key.SignSchnorr(msg, sig, &merkle_root, aux);
27+        assert(success);
28+    });
29+}
30+
31+static void SignSchnorrWithMerkleRoot(benchmark::Bench& bench)
32+{
33+    SignSchnorrBenchmark(bench, /*use_null_merkle_root=*/false);
34+}
35+
36+static void SignSchnorrWithNullMerkleRoot(benchmark::Bench& bench)
37+{
38+    SignSchnorrBenchmark(bench, /*use_null_merkle_root=*/true);
39+}
40+
41 BENCHMARK(SignTransactionECDSA, benchmark::PriorityLevel::HIGH);
42 BENCHMARK(SignTransactionSchnorr, benchmark::PriorityLevel::HIGH);
43+BENCHMARK(SignSchnorrWithMerkleRoot, benchmark::PriorityLevel::HIGH);
44+BENCHMARK(SignSchnorrWithNullMerkleRoot, benchmark::PriorityLevel::HIGH);

which reveals that the new signing is ~15% slower: before:

ns/signature	signature/s	err%	total	benchmark
69,383.89	14,412.57	0.1%	1.06	`SignSchnorrWithMerkleRoot`
69,548.30	14,378.50	0.0%	1.06	`SignSchnorrWithNullMerkleRoot`

ns/signature	signature/s	err%	total	benchmark
70,189.31	14,247.18	0.2%	1.07	`SignSchnorrWithMerkleRoot`
69,464.39	14,395.87	0.1%	1.06	`SignSchnorrWithNullMerkleRoot`

after:

ns/signature	signature/s	err%	total	benchmark
83,055.75	12,040.11	0.2%	1.08	`SignSchnorrWithMerkleRoot`
80,971.71	12,349.99	0.1%	1.08	`SignSchnorrWithNullMerkleRoot`

ns/signature	signature/s	err%	total	benchmark
81,841.15	12,218.79	0.1%	1.08	`SignSchnorrWithMerkleRoot`
82,112.31	12,178.44	0.1%	1.08	`SignSchnorrWithNullMerkleRoot`

paplorinc commented at 9:51 am on July 30, 2024:

@josibake, what do you think of this?

josibake commented at 1:48 pm on August 3, 2024:

Thanks for writing the benchmark @paplorinc ! I’ve added a slightly modified version in the first commit. 15% is a tough pill to swallow, so I reverted back to extracting the xonly public key from the secp256k1_keypair object and serialising it. Per our early conversation, I replaced the early returns with asserts. This more closely mirrors what CKey::GetPubKey() is doing and I think better documents what the code is doing.

I added this as the last commit to keep the diffs as clean as possible.

paplorinc commented at 1:57 pm on August 3, 2024:

I think you found a good compromise, especially with the updated test which checks that it basically does the same thing.

in src/key.cpp:280 in 4e8d61e750 outdated

276     secp256k1_keypair keypair;
277     if (!secp256k1_keypair_create(secp256k1_context_sign, &keypair, UCharCast(begin()))) return false;
278     if (merkle_root) {
279         secp256k1_xonly_pubkey pubkey;
280-        if (!secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, &keypair)) return false;
281+        ret &= secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, &keypair);

paplorinc commented at 2:45 pm on July 17, 2024:

Is there any advantage in reusing ret, or could we do bool ret = secp256k1_keypair_xonly_pub and return inside the if instead?

josibake commented at 2:20 pm on July 20, 2024:

Personally, I don’t see any reason to not reuse ret and I think it makes the logic easier to follow vs re-initializing the variable. Regarding returning inside the if (ret), as mentioned in #30051 (review), keypair_xonly_pub will only fail if the keypair is malformed, so if we’ve successfully created the keypair this call should never fail. So if schnorrsig_verify fails, we check it on the next line and clear the signature data before returning. I find this easier to follow than having a return inside the if (ret) block (and we don’t gain anything by having an early return inside the if (ret) block.

josibake commented at 9:10 am on July 21, 2024:

Resolving since this is no longer relevant.

in src/key.cpp:283 in 4e8d61e750 outdated

280-        if (!secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, &keypair)) return false;
281+        ret &= secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, &keypair);
282         unsigned char pubkey_bytes[32];
283-        if (!secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey)) return false;
284+        ret &= secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey);
285         uint256 tweak = XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);

paplorinc commented at 3:01 pm on July 17, 2024:

off topic: we might not need a copy of pubkey_bytes in this case, but I guess that’s unrelated to the PR

in src/key.cpp:294 in 4e8d61e750 outdated

294         secp256k1_xonly_pubkey pubkey_verify;
295-        ret = secp256k1_keypair_xonly_pub(secp256k1_context_static, &pubkey_verify, nullptr, &keypair);
296+        ret &= secp256k1_keypair_xonly_pub(secp256k1_context_static, &pubkey_verify, nullptr, &keypair);
297         ret &= secp256k1_schnorrsig_verify(secp256k1_context_static, sig.data(), hash.begin(), 32, &pubkey_verify);
298     }
299     if (!ret) memory_cleanse(sig.data(), sig.size());

paplorinc commented at 3:02 pm on July 17, 2024:

off topic: I’m out of my depth here, but curious, what’s the reason for not needing a cleanup of keypair in the merkle_root branch as well? Was that a bug? Do we have tests for that?

josibake commented at 2:29 pm on July 20, 2024:

Good catch, I think you’re right that if xonly_tweak_add were to fail (e.g. tweak32 is the negation of the secret key), then we would return without explicitly clearing the keypair variable. That being said, the variable should already be cleared when it goes out of scope, so the explicit memory_cleanse is more of a belt and suspenders approach, from what I understand.

in src/key.cpp:1 in 4e8d61e750 outdated

paplorinc commented at 3:04 pm on July 17, 2024:

nit: typos in commit message: Accumlute and returning early on the function

paplorinc commented at 7:30 pm on July 17, 2024:

nit: typo in commit message: his allows keeps the secret,allows for passing, the logic for for applying, and the finally move

paplorinc commented at 8:11 pm on July 17, 2024:

nit: typo in commit message: Instead of calling ... instead invalidate

josibake commented at 3:39 pm on July 20, 2024:

Fixed.

josibake commented at 3:40 pm on July 20, 2024:

Fixed.

josibake commented at 3:40 pm on July 20, 2024:

Fixed.

paplorinc commented at 3:04 pm on July 17, 2024: contributor

Started reviewing, but got a bit stuck at the first commit, will continue a bit later

in src/key.h:281 in 3ca329795c outdated

276+
277+    /**
278+     * This method will be made read-only in a later commit.
279+     * It is only read/write here to simplify the diff in the next two commits
280+     */
281+    unsigned char* data() { return m_keypair ? m_keypair->data() : nullptr; }

paplorinc commented at 7:36 pm on July 17, 2024:

nit: would it make sense to use IsValid() here?

josibake commented at 3:40 pm on July 20, 2024:

Agree, I think IsValid makes this more readable, updated.

in src/key.cpp:275 in 21d031e786 outdated

272@@ -273,22 +273,22 @@ bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint2
273 {
274     assert(sig.size() == 64);
275     bool ret = 1;
276-    secp256k1_keypair keypair;
277-    if (!secp256k1_keypair_create(secp256k1_context_sign, &keypair, UCharCast(begin()))) return false;
278+    KeyPair keypair = ComputeKeyPair();
279+    if (!keypair.IsValid()) return false;

paplorinc commented at 7:45 pm on July 17, 2024:

maybe it would simplify the diff if we extracted the keypair.data() here

0    KeyPair keypair = ComputeKeyPair();
1    if (!keypair.IsValid()) return false;
2    const secp256k1_keypair *keypairData = reinterpret_cast<const secp256k1_keypair *>(keypair.data());

josibake commented at 2:48 pm on July 20, 2024:

Hm, I don’t think it makes much of a difference for the diff and makes this particular staging commit more confusing by creating the KeyPair object just to pull out the data back out into a secp256k1_keypair object. The main advantage of the KeyPair wrapper class is that we are keeping the keypair in secure memory, which we would be undoing here by copying it out into a keypair object.

paplorinc commented at 5:34 pm on July 20, 2024:

there’s a lot of ugly duplication because of all the reinterpret_cast changes. Wouldn’t it make sense to extract those and simplify the function and the diff?

paplorinc commented at 7:09 pm on July 20, 2024:

Or maybe introduce a casting helper like https://github.com/bitcoin/bitcoin/blob/master/src/span.h#L295

josibake commented at 7:20 pm on July 20, 2024:

Ah, sorry, I misunderstood your suggestion as “extract” (copy) the data out into a variable but you’re suggesting saving the pointer as a variable. I think this would simplify the diff a lot, will give it a shot.

The only thing I don’t like here is needing to pass it as non-const to two functions and const to the other two, but I think its okay to pass a non-const object to a function with const in the signature?

paplorinc commented at 7:51 pm on July 20, 2024:

if you end up using similar casting helper methods, the const part won’t matter anymore, right?

josibake commented at 9:14 am on July 21, 2024:

Took your suggestion of saving the pointer in a variable. I’m not sure about a cast helper, mainly because I don’t want to put secp256k1_keypair into a header file, so seems better to just have people use reinterpret cast wherever KeyPair is being used.

paplorinc commented at 10:20 am on July 21, 2024:

nice!

in src/key.cpp:284 in 21d031e786 outdated

283+        ret &= secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, reinterpret_cast<const secp256k1_keypair*>(keypair.data()));
284         unsigned char pubkey_bytes[32];
285         ret &= secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey);
286         uint256 tweak = XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);
287-        ret &= secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, &keypair, tweak.data());
288+        ret &= secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, reinterpret_cast<secp256k1_keypair*>(keypair.data()), tweak.data());

paplorinc commented at 7:54 pm on July 17, 2024:

Might make the code more uniform (and maybe the diff simpler, if we’d extract it as suggested above) if instead of not making this a const cast, like the rest, we rather added the const to the method’s signature:

 0diff --git a/src/secp256k1/include/secp256k1_extrakeys.h b/src/secp256k1/include/secp256k1_extrakeys.h
 1--- a/src/secp256k1/include/secp256k1_extrakeys.h	(revision 21d031e7867ef97c07cee691c115d59b3aebfd19)
 2+++ b/src/secp256k1/include/secp256k1_extrakeys.h	(date 1721245901582)
 3@@ -236,7 +236,7 @@
 4  */
 5 SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_keypair_xonly_tweak_add(
 6     const secp256k1_context *ctx,
 7-    secp256k1_keypair *keypair,
 8+    const secp256k1_keypair *keypair,
 9     const unsigned char *tweak32
10 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);
11 
12Index: src/secp256k1/src/modules/extrakeys/main_impl.h
13IDEA additional info:
14Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
15<+>UTF-8
16===================================================================
17diff --git a/src/secp256k1/src/modules/extrakeys/main_impl.h b/src/secp256k1/src/modules/extrakeys/main_impl.h
18--- a/src/secp256k1/src/modules/extrakeys/main_impl.h	(revision 21d031e7867ef97c07cee691c115d59b3aebfd19)
19+++ b/src/secp256k1/src/modules/extrakeys/main_impl.h	(date 1721245849520)
20@@ -252,7 +252,7 @@
21     return 1;
22 }
23 
24-int secp256k1_keypair_xonly_tweak_add(const secp256k1_context* ctx, secp256k1_keypair *keypair, const unsigned char *tweak32) {
25+int secp256k1_keypair_xonly_tweak_add(const secp256k1_context* ctx, const secp256k1_keypair *keypair, const unsigned char *tweak32) {
26     secp256k1_ge pk;
27     secp256k1_scalar sk;
28     int y_parity;

josibake commented at 2:51 pm on July 20, 2024:

secp256k1_keypair_xonly_tweak_add modifies the keypair object with tweak32, so it can’t be const. More generally, libsecp256k1 is a dependency of Bitcoin Core, so we shouldn’t ever be modifying secp256k1_ functions in our code base (despite us pulling it in as a subtree).

paplorinc commented at 5:35 pm on July 20, 2024:

right, I thought that make would catch that, not sure why it allowed a memset on a const (most be C vs C++ boundary, I guess).

josibake commented at 6:01 pm on July 20, 2024:

That is surprising. I applied your patch directly in bitcoin-core/secp256k1 and did get a warning:

 0src/modules/extrakeys/main_impl.h:266:12: warning: passing argument 1 of ‘memset’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
 1  266 |     memset(keypair, 0, sizeof(*keypair));
 2      |            ^~~~~~~
 3In file included from src/ecmult_impl.h:10,
 4                 from src/secp256k1.c:30:
 5/usr/include/string.h:61:28: note: expected ‘void *’ but argument is of type ‘const secp256k1_keypair *’
 6   61 | extern void *memset (void *__s, int __c, size_t __n) __THROW __nonnull ((1));
 7      |                      ~~~~~~^~~
 8src/modules/extrakeys/main_impl.h:278:32: warning: passing argument 1 of ‘secp256k1_keypair_save’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
 9  278 |         secp256k1_keypair_save(keypair, &sk, &pk);
10      |                                ^~~~~~~
11src/modules/extrakeys/main_impl.h:156:55: note: expected ‘secp256k1_keypair *’ but argument is of type ‘const secp256k1_keypair *’
12  156 | static void secp256k1_keypair_save(secp256k1_keypair *keypair, const secp256k1_scalar *sk, secp256k1_ge *pk) {

which is good, but I would have also expected it to fail to compile :thinking:

paplorinc commented at 10:22 am on July 21, 2024:

it must be a C vs C++ inconsistency - we’re not affected by it anymore, you can resolve this

in src/key.h:218 in 82af81b51a outdated

213+     *
214+     *  - If merkle_root == nullptr: no tweaking is done, use the internal key directly (this is
215+     *                               used for signatures in BIP342 script).
216+     *  - If merkle_root->IsNull():  tweak the internal key with H_TapTweak(pubkey) (this is used for
217+     *                               key path spending when no scripts are present).
218+     *  - Otherwise:                 tweak the internal key H_TapTweak(pubkey || *merkle_root)

paplorinc commented at 8:05 pm on July 17, 2024:

0     *  - Otherwise:                 tweak the internal key with H_TapTweak(pubkey || *merkle_root)

josibake commented at 3:40 pm on July 20, 2024:

Fixed.

in src/key.h:219 in 82af81b51a outdated

214+     *  - If merkle_root == nullptr: no tweaking is done, use the internal key directly (this is
215+     *                               used for signatures in BIP342 script).
216+     *  - If merkle_root->IsNull():  tweak the internal key with H_TapTweak(pubkey) (this is used for
217+     *                               key path spending when no scripts are present).
218+     *  - Otherwise:                 tweak the internal key H_TapTweak(pubkey || *merkle_root)
219+     *                               (this is used for key path spending, with specific

paplorinc commented at 8:05 pm on July 17, 2024:

0     *                               (this is used for key path spending with a specific

josibake commented at 3:40 pm on July 20, 2024:

Fixed.

in src/key.h:293 in 82af81b51a outdated

291 
292     /**
293-     * This method will be made read-only in a later commit.
294-     * It is only read/write here to simplify the diff in the next two commits
295+     * This is provided as a read-only method for passing a KeyPair object to secp256k1 functions
296+     * expecting a `secp256k1_keypair`. This avoids need to create a `secp256k1_keypair` object and

paplorinc commented at 8:09 pm on July 17, 2024:

0     * expecting a `secp256k1_keypair`. This avoids the need to create a `secp256k1_keypair` object and

josibake commented at 3:39 pm on July 20, 2024:

Fixed.

in src/key.cpp:278 in a76cc3650d outdated

284-    }
285-    if (!ret) memory_cleanse(sig.data(), sig.size());
286-    memory_cleanse(&keypair, sizeof(keypair));
287+    bool ret = keypair.SignSchnorr(hash, sig, aux);
288+    /* Clear the keypair */
289+    keypair = KeyPair();

paplorinc commented at 8:13 pm on July 17, 2024:

Might be a naive question, but isn’t this a behavior change? Does it have any security implications (e.g. could it leave keys in the memory or something)?

itornaza commented at 10:10 am on July 18, 2024:

I am also wandering how the assignment involving the default constructor will clear the existing keypair data from memory, but I might be naive as well! Since keypair is a local variable in this function shouldn’t the KeyPair destructor do the secure erase job anyway at the end of the scope through RAII since m_keypair is a secure_unique_ptr?

josibake commented at 3:18 pm on July 20, 2024:

Since keypair is a local variable in this function shouldn’t the KeyPair destructor do the secure erase job anyway at the end of the scope through RAII since m_keypair is a secure_unique_ptr

Correct, which should happen regardless of whether or not we explicitly clear the key data with keypair = KeyPair();. Having keypair = KeyPair() a belt and suspenders by explicitly clearing the variable. This is because if we create a KeyPair object with the default constructor, there is no call to MakeKeyPairData, hence m_keypair is empty. When we assign this default constructed KeyPair to keypair, it calls ClearKeyPairData on keypair, which resets it. The assignment operator was copied from CKey and you can see an example of the same pattern used here: https://github.com/bitcoin/bitcoin/blob/5e9f4f41de98c2cdcc74e31fe925b2f5cf9f6aec/src/bip324.cpp#L66-L70

paplorinc commented at 10:23 am on July 21, 2024:

I’m still a bit hesitant here, see my related comment

josibake commented at 5:11 pm on July 21, 2024:

My last comment was wrong, @itornaza is correct: the KeyPair destructor is doing the secure erase job and the kp = KeyPair(); is unnecessary.

paplorinc commented at 8:15 pm on July 17, 2024: contributor

Very nice separation into commits, left some questions

in src/key.cpp:275 in 5e9f4f41de outdated

290-        ret &= secp256k1_schnorrsig_verify(secp256k1_context_static, sig.data(), hash.begin(), 32, &pubkey_verify);
291-    }
292-    if (!ret) memory_cleanse(sig.data(), sig.size());
293-    memory_cleanse(&keypair, sizeof(keypair));
294+    KeyPair keypair = ComputeKeyPair(merkle_root);
295+    if (!keypair.IsValid()) return false;

itornaza commented at 10:19 am on July 18, 2024:

It would be trivial to also erase the keypair even if it turns out not to be valid, or would that be paranoid?

josibake commented at 3:01 pm on July 20, 2024:

IsValid() returns false when m_keypair data is a nullptr, which effectively means the key data was never created or has been cleared (see src/support/allocators/secure.h)

itornaza approved

itornaza commented at 4:35 pm on July 18, 2024: none

re tACK 5e9f4f41de98c2cdcc74e31fe925b2f5cf9f6aec

Thank you for your commit redo in a more granular way. Just a couple of questions really in the discussion above. To be thorough, rebuilt and retested the commit and all tests included the extended suite pass.

DrahtBot requested review from theuni on Jul 18, 2024

josibake force-pushed on Jul 20, 2024

josibake commented at 3:39 pm on July 20, 2024: member

Updated https://github.com/bitcoin/bitcoin/commit/5e9f4f41de98c2cdcc74e31fe925b2f5cf9f6aec -> https://github.com/bitcoin/bitcoin/commit/5d9a6cf6286f9a7f93527ea76b910537d709a860 (add-apply-taptweak-method-01 -> apply-taptweak-method-02 (compare)

Fix typos in commit messages, src/key.h (h/t @paplorinc)
Use IsValid() instead of m_keypair ? (h/t @paplorinc)

Thanks for the re-review @paplorinc and @itornaza!

in src/key.cpp:275 in 36ecf72a04 outdated

271@@ -272,21 +272,23 @@ bool CKey::SignCompact(const uint256 &hash, std::vector<unsigned char>& vchSig)
272 bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256* merkle_root, const uint256& aux) const
273 {
274     assert(sig.size() == 64);
275+    bool ret = true;

paplorinc commented at 5:25 pm on July 20, 2024:

nit: we could move this down, after the first early return, since it’s not needed before that

josibake commented at 9:16 am on July 21, 2024:

Removed all of the instances of initialisation the bool outside of a function call.

paplorinc changes_requested

paplorinc commented at 5:38 pm on July 20, 2024: contributor

Thanks for the fixes! I’m still not convinced that the lack of short-circuits didn’t introduce any change. Please prove me wrong, I want to ACK this :)

josibake force-pushed on Jul 21, 2024

josibake commented at 8:59 am on July 21, 2024: member

Update https://github.com/bitcoin/bitcoin/commit/5d9a6cf6286f9a7f93527ea76b910537d709a860 -> https://github.com/bitcoin/bitcoin/commit/49057fc4f1e73a14f673c934573d727ae0229779 (apply-taptweak-method-02 -> apply-taptweak-method-03 (compare)

Use CKey::GetPubKey() instead of extracting and serializing the public key from the keypair object
Save reinterpret_cast as a variable to simplify diff/code (h/t @paplorinc )

Overall, I think this simplifies things quite a bit. If you look at the internals of CKey::GetPubKey(), there are already asserts for intermediate libsecp256k1 function calls which make it clear these calls (_serialize, for example) are always expected to succeed when using a valid secret key.

in src/key.cpp:278 in 1cbf89693c outdated

274@@ -275,10 +275,7 @@ bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint2
275     secp256k1_keypair keypair;
276     if (!secp256k1_keypair_create(secp256k1_context_sign, &keypair, UCharCast(begin()))) return false;
277     if (merkle_root) {
278-        secp256k1_xonly_pubkey pubkey;
279-        if (!secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, &keypair)) return false;
280-        unsigned char pubkey_bytes[32];
281-        if (!secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey)) return false;
282+        CPubKey pubkey_bytes = GetPubKey();

paplorinc commented at 9:40 am on July 21, 2024:

this is SO MUCH BETTER! \:D/ (it even has the asserts that we were talking about)

I didn’t trust, but verified, of course, if you think it makes sense, please add this test to the commit (you can add me as co-author if you want as l0rinc <pap.lorinc@gmail.com>):

 0BOOST_AUTO_TEST_CASE(key_sign_schnorr_tweak_test)
 1{
 2    secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
 3
 4    for (int i = 0; i < 1000; ++i) {
 5        CKey key;
 6        key.MakeNewKey(true);
 7        uint256 merkle_root = InsecureRandBool() ? uint256(0) : InsecureRand256();
 8
 9        // Old method
10        secp256k1_keypair keypair;
11        BOOST_CHECK(secp256k1_keypair_create(secp256k1_context_sign, &keypair, UCharCast(key.begin())));
12        secp256k1_xonly_pubkey pubkey_old;
13        BOOST_CHECK(secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey_old, nullptr, &keypair));
14        unsigned char pubkey_bytes_old[32];
15        BOOST_CHECK(secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes_old, &pubkey_old));
16        uint256 tweak_old = XOnlyPubKey(pubkey_bytes_old).ComputeTapTweakHash(merkle_root.IsNull() ? nullptr : &merkle_root);
17
18        // New method
19        CPubKey pubkey_bytes_new = key.GetPubKey();
20        uint256 tweak_new = XOnlyPubKey(pubkey_bytes_new).ComputeTapTweakHash(merkle_root.IsNull() ? nullptr : &merkle_root);
21
22        BOOST_CHECK_EQUAL(tweak_old, tweak_new);
23    }
24
25    secp256k1_context_destroy(secp256k1_context_sign);
26}

josibake commented at 5:19 pm on July 21, 2024:

Thanks! I added a modified version of this as a test. I took out the iterations because creating the keys, signing, etc is an expensive operation and I don’t think it adds anything to do multiple rounds every time the tests are ran (i.e., if it this were to fail, it will fail for all keys, not a particular key).

This also means the merkle root isn’t be switched, but that’s fine since the thing we actually want to test here is that we get the same results using two different methods of getting the public key for the tweak.

in src/key.cpp:283 in 7bbb215e57 outdated

287     if (ret) {
288         // Additional verification step to prevent using a potentially corrupted signature
289         secp256k1_xonly_pubkey pubkey_verify;
290-        ret = secp256k1_keypair_xonly_pub(secp256k1_context_static, &pubkey_verify, nullptr, &keypair);
291+        ret = secp256k1_keypair_xonly_pub(secp256k1_context_static, &pubkey_verify, nullptr, keypair);
292         ret &= secp256k1_schnorrsig_verify(secp256k1_context_static, sig.data(), hash.begin(), 32, &pubkey_verify);

paplorinc commented at 9:46 am on July 21, 2024:

nit: I understand this was the code before and that we’ve talked about this before, but in other places we use short circuiting logic for such code: https://github.com/bitcoin/bitcoin/blob/master/src/secp256k1/src/modules/extrakeys/main_impl.h#L185. Will leave it up to you to decide which is better, seems safer to me not to call verify when we’re already in an invalid state.

josibake commented at 5:22 pm on July 21, 2024:

Leaving as is for the reasons mentioned in #30051 (review)

paplorinc commented at 8:27 pm on July 21, 2024:

One last attempt from my part, but it’s not a blocker, would appreciate if you would consider it anyway: in the previous example we’ve solved this basically by changing the return aggregation (into ret) with assertions instead, since we knew that in reality we can’t have short-circuiting here. It could make sense to do the same here (if that’s the reasoning) and leave a simple return here as well.

in src/key.cpp:427 in 7bbb215e57 outdated

424 {
425     static_assert(std::tuple_size<KeyType>() == sizeof(secp256k1_keypair));
426     MakeKeyPairData();
427     auto keypair = reinterpret_cast<secp256k1_keypair*>(m_keypair->data());
428-
429     bool ret = secp256k1_keypair_create(secp256k1_context_sign, keypair, UCharCast(key.data()));

paplorinc commented at 9:53 am on July 21, 2024:

nit: in other cases it makes sense to call it ret, since it’s the return value, here it’s just a validity check - if you think it makes sense, consider renaming to avoid the surprise

josibake commented at 5:15 pm on July 21, 2024:

Updated.

in src/key.cpp:428 in 8b7f460b1f outdated

423@@ -433,6 +424,21 @@ KeyPair::KeyPair(const CKey& key, const uint256* merkle_root)
424     if (!ret) ClearKeyPairData();
425 }
426 
427+bool KeyPair::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256& aux) const
428+{
429+    assert(sig.size() == 64);

paplorinc commented at 10:07 am on July 21, 2024:

nit: it’s 64 because of https://github.com/bitcoin/bitcoin/blob/master/src/script/sign.cpp#L86, right? It’s not really called from other places, the assert seems like noise here.

josibake commented at 3:50 pm on July 21, 2024:

It’s 64 because Schnorr signatures, unlike ECDSA signatures, are by definition 64 bytes (see https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#description), so we definitely want to keep this assert.

paplorinc commented at 4:01 pm on July 21, 2024:

Yes, of course, but that’s already the case for every caller currently (i.e. not part of a public api, we know all the callers), and we’re not really asserting this in other such methods, so it seems redundant to me. If you disagree, just resolve it, not a big deal either way.

in src/test/key_tests.cpp:308 in 49057fc4f1 outdated

299@@ -299,6 +300,13 @@ BOOST_AUTO_TEST_CASE(bip340_test_vectors)
300         // Verify those signatures for good measure.
301         BOOST_CHECK(pubkey.VerifySchnorr(msg256, sig64));
302 
303+        // Repeat the same check, but use the KeyPair directly without any merkle tweak
304+        KeyPair keypair = key.ComputeKeyPair(/*merkle_root=*/nullptr);
305+        bool kp_ok = keypair.SignSchnorr(msg256, sig64, aux256);
306+        BOOST_CHECK(kp_ok);
307+        BOOST_CHECK(pubkey.VerifySchnorr(msg256, sig64));
308+        BOOST_CHECK(std::vector<unsigned char>(sig64, sig64 + 64) == sig);

paplorinc commented at 10:12 am on July 21, 2024:

BOOST_CHECK has very bad error messages, consider using the _EQUAL or in this case _EQUAL_COLLECTIONS for better errors:

0        BOOST_CHECK_EQUAL_COLLECTIONS(std::begin(sig64), std::end(sig64), sig.begin(), sig.end());

josibake commented at 4:24 pm on July 21, 2024:

In this case, I think it’s better to match the style of the existing test but I’ve made a note regarding _EQUAL and _EQUAL_COLLECTIONS going forward.

in src/key.cpp:278 in 49057fc4f1 outdated

293-    memory_cleanse(&keypair, sizeof(keypair));
294+    KeyPair kp = ComputeKeyPair(merkle_root);
295+    if (!kp.IsValid()) return false;
296+    bool ret = kp.SignSchnorr(hash, sig, aux);
297+    /* Clear the keypair */
298+    kp = KeyPair();

paplorinc commented at 10:16 am on July 21, 2024:

this is the only remaining part that I’m uncomfortable with, even the IDE is complaining:

which I’m not getting for BIP324Cipher::Initialize, since it’s assigning a field, not a local variable.

Is there a more intuitive way to do this? Do we have tests to check that this value is cleared?

josibake commented at 5:09 pm on July 21, 2024:

Ah! I was misunderstanding m_key = CKey(). Since CKey (and KeyPair) are using secure allocator, the contents will be cleared before deletion (e.g. when the variable goes out of scope). But in the case of BIP324Cipher::Initialize, it seems m_key is used to initialize the connection, but then needs to be explicitly cleared by setting with an empty CKey (invalid CKey), because the member field will not go out of scope and be cleared.

So in this case, we don’t need the kp = KeyPair(); line at all. Also worth mentioning: memory_cleanse(keypair.. was needed before because the keypair object was not using a secure allocator.

paplorinc commented at 10:25 am on July 21, 2024: contributor

Nice, left a few nits and a clarification

josibake force-pushed on Jul 21, 2024

josibake commented at 5:15 pm on July 21, 2024: member

Update https://github.com/bitcoin/bitcoin/commit/49057fc4f1e73a14f673c934573d727ae0229779 - > https://github.com/bitcoin/bitcoin/commit/b8b3a9f18670ec7bf246a57950cdae7e034a264d (apply-taptweak-method-03 -> apply-taptweak-method-04 (compare)

Renamed ret to success in KeyPair constructor (h/t @paplorinc)
Removed unnecessary KeyPair() call
Added key tweak smoke test (h/t @paplorinc )

itornaza approved

itornaza commented at 6:10 pm on July 21, 2024: none

re ACK b8b3a9f18670ec7bf246a57950cdae7e034a264d

Both of you did an amazing job over this weekend and I learned a lot by just watching this thread! Just to re-confirm from my side as well, the aforementioned KeyPair() call removal comes without any security implications as we discussed above #30051 (review). However, just for the sake of paranoia I build the commit in debug configuration and run the tests in debug mode using lldb and confirmed that m_keypair becomes a nullptr at the end of CKey::SignSchnorr() scope.

All unit, functional and extended tests pass as well.

DrahtBot requested review from paplorinc on Jul 21, 2024

in src/key.cpp:275 in b8b3a9f186 outdated

292-    if (!ret) memory_cleanse(sig.data(), sig.size());
293-    memory_cleanse(&keypair, sizeof(keypair));
294-    return ret;
295+    KeyPair kp = ComputeKeyPair(merkle_root);
296+    if (!kp.IsValid()) return false;
297+    return kp.SignSchnorr(hash, sig, aux);

paplorinc commented at 8:20 pm on July 21, 2024:

nit:

0    return kp.IsValid() && kp.SignSchnorr(hash, sig, aux);

josibake commented at 9:48 am on July 22, 2024:

Updated.

paplorinc approved

paplorinc commented at 8:29 pm on July 21, 2024: contributor

utACK b8b3a9f18670ec7bf246a57950cdae7e034a264d

Appreciate your perseverance and creativity @josibake, and your tests @itornaza.

Shuhaib07 approved

josibake force-pushed on Jul 22, 2024

josibake commented at 9:48 am on July 22, 2024: member

Update https://github.com/bitcoin/bitcoin/commit/b8b3a9f18670ec7bf246a57950cdae7e034a264d -> https://github.com/bitcoin/bitcoin/commit/56267cf84281789186035f09898cce3d749748ec (apply-taptweak-method-04 -> apply-taptweak-method-05 (compare)

Realised I unintentionally dropped @theuni as a co-author of the KeyPair class. Also applied @paplorinc ’s suggestion for CKey::SignScnorr. Apologies for making you guys re-ack again, this should be the last one :)

paplorinc commented at 10:03 am on July 22, 2024: contributor

CI failure seems unrelated, please restart or rebase so we can ACK - and don’t worry about our reacks :)

Update https://github.com/bitcoin/bitcoin/commit/b8b3a9f18670ec7bf246a57950cdae7e034a264d -> https://github.com/bitcoin/bitcoin/commit/56267cf84281789186035f09898cce3d749748ec (apply-taptweak-method-04 -> apply-taptweak-method-05 (compare)

I usually review the changes first via:

josibake force-pushed on Jul 22, 2024

DrahtBot added the label CI failed on Jul 22, 2024

DrahtBot commented at 10:40 am on July 22, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27740474818

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

josibake commented at 10:47 am on July 22, 2024: member

Update https://github.com/bitcoin/bitcoin/commit/56267cf84281789186035f09898cce3d749748ec -> https://github.com/bitcoin/bitcoin/commit/e998ca2c53bb8320e102a02a06182f8bc49a4f90 (apply-taptweak-method-05 -> apply-taptweak-method-06 (compare)

Fix CI failure (due to moving test: add key tweak smoke test up, but without adding #include secp256k1_keypair

Not sure about the other CI failures, but if they fail again I’ll rebase on master.

I usually review the changes first via:

Adding the update summary is more for my own book keeping. Also, pushing the individual numbered branches for each change allows reviewers to easily check the diff between force pushes without needing to use github’s UI. It also allows reviewers to easily go back and verify old force pushes, whereas I think github deletes the force pushed commits after a certain amount of time?

josibake force-pushed on Jul 22, 2024

josibake commented at 10:53 am on July 22, 2024: member

Rebased on master https://github.com/bitcoin/bitcoin/commit/e998ca2c53bb8320e102a02a06182f8bc49a4f90 -> https://github.com/bitcoin/bitcoin/commit/9afa2c9e50370b2918377f3f3eac0950a4296ec0 (apply-taptweak-method-06 -> apply-taptweak-method-06 (compare)

josibake commented at 11:35 am on July 22, 2024: member

lint CI failure is unrelated and fixed by https://github.com/bitcoin/bitcoin/pull/30499

paplorinc commented at 11:38 am on July 22, 2024: contributor

lint CI failure is unrelated and fixed by #30499

~seems that PR is also failing~ fixed now

DrahtBot removed the label CI failed on Jul 22, 2024

paplorinc commented at 3:22 pm on July 22, 2024: contributor

utACK 9afa2c9e50370b2918377f3f3eac0950a4296ec0

DrahtBot requested review from itornaza on Jul 22, 2024

in src/test/key_tests.cpp:365 in a10ecc45fa outdated

360+    BOOST_CHECK(secp256k1_keypair_create(secp256k1_context_sign, &keypair, UCharCast(key.begin())));
361+    secp256k1_xonly_pubkey pubkey_old;
362+    BOOST_CHECK(secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey_old, nullptr, &keypair));
363+    unsigned char pubkey_bytes_old[32];
364+    BOOST_CHECK(secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes_old, &pubkey_old));
365+    uint256 tweak_old = XOnlyPubKey(pubkey_bytes_old).ComputeTapTweakHash(merkle_root.IsNull() ? nullptr : &merkle_root);

sipa commented at 3:42 pm on July 22, 2024:

In commit “tests: add key tweak smoke test”

The merkle_root.IsNull() condition will never hold here. Perhaps it makes sense to run this test in a loop, and only set merkle_root to InsecureRand256() in some of the iterations?

josibake commented at 9:44 am on July 23, 2024:

This is how @paplorinc originally proposed the test: #30051 (review)

I felt like computing multiple keys in a loop was overkill and removed the merkle root switching since that’s not really what’s being tested here. The goal here is to sanity check that we get the same public key bytes from GetPubKey() as we would get directly accessing the secp256k1_keypair using the _xonly_pub and _serialize.

So perhaps better to just remove the IsNull() check, considering this is being checked in the manner you describe in the bip340_test_vectors test?

ismaelsadeeq commented at 4:12 pm on August 2, 2024:

Should this instead check that the two pub keys are the same? If they are, we certain that ComputeTapTweakHash will generate same tweak.

josibake commented at 1:42 pm on August 3, 2024:

I’ve updated the to remove the merkle_root.IsNull() check. @ismaelsadeeq I kept the ComputeTapTweakHash line because they are testing slightly different paths: in the first case, an XOnlyPubKey is being constructed from a 32 byte unsigned char array, in the second case it’s being created from a CPubKey object. I renamed the variables to try to make this more clear in the test.

ismaelsadeeq commented at 9:49 am on August 5, 2024:

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

josibake commented at 11:34 am on August 5, 2024:

Agree the code paths are very similar but I think the same argument as #30051 (review) applies here (admittedly much weaker).

in src/key.h:288 in fa855cfa0d outdated

283+     *
284+     * Recall that `secp256k1_keypair` is an opaque data type, so this method should only be used
285+     * for passing a KeyPair object as a secp256k1_keypair and should never be used to access the
286+     * underlying keypair bytes directly.
287+     */
288+    const unsigned char* data() const { return IsValid() ? m_keypair->data() : nullptr; }

sipa commented at 4:43 pm on July 22, 2024:

If the only purpose of this function is constructing a const secp256k1_keypair*, I think it would be better to make it private (it seems only accessed inside KeyPair anyway?), and give it another name (data() is used in STL containers to give raw access to the contents, which seems to be explicitly what we don’t want here).

itornaza commented at 6:35 pm on July 22, 2024:

If there is not a better alternative, I would suggest secp256k1_keypair() as a candidate for renaming this data() member function to further signify its use through the cast and be called like so:

reinterpret_cast<const secp256k1_keypair*>(keypair.secp256k1_keypair())

josibake commented at 9:38 am on July 23, 2024:

(just realised I need to update the description of this PR to be a bit more clear on the motivation) @sipa - my main motivations for having the KeyPair class is to a) encapsulate the taptweak logic so that it can be used outside of signing and b) to be able to pass the KeyPair directly to secp256k1_* functions expecting a const secp256k1_keypair *. For the latter, I think we will need some sort of public function. Here is an example of how this used in #28201:

https://github.com/bitcoin/bitcoin/blob/38067a6ef07d9b5108e75207960bdb7a21c14b70/src/common/bip352.cpp#L206-L221

I do agree a rename would be much better here, perhaps ReadKeyPairData()/GetKeyPairData(), or something like @itornaza ’s suggestion, secp256k1_keypair()?.

Another option is leave the read-only public method out of this PR (since it’s not currently used) and add the read-only method in #28201 ?

ismaelsadeeq commented at 4:57 pm on August 2, 2024:

+1 on renaming to GetKeyPairData

josibake commented at 1:39 pm on August 3, 2024:

I’ve removed data() for now and will add GetKeyPairData as a read-only method in #28201

itornaza approved

itornaza commented at 6:37 pm on July 22, 2024: none

trACK 9afa2c9e50370b2918377f3f3eac0950a4296ec0

Reviewed the changes in the key_tests.cpp since my last review on this PR. Again, all unit and functional including the extended tests pass. Just dropped a name candidate if you consider renaming KeyPair::data() and run out of options.

Shuhaib07 approved

ismaelsadeeq commented at 4:03 pm on July 24, 2024: member

Concept ACK

clean refactor IMO.

paplorinc commented at 10:44 am on August 1, 2024: contributor

To make sure my benchmarks related post doesn’t get lost in resolved comment land, lemme’ repost it here:

 0diff --git a/src/bench/sign_transaction.cpp b/src/bench/sign_transaction.cpp
 1--- a/src/bench/sign_transaction.cpp	(revision 8d573611575c3fa66f08407aa9b02f91b29a94c3)
 2+++ b/src/bench/sign_transaction.cpp	(date 1721731869277)
 3@@ -12,6 +12,7 @@
 4 #include <script/script.h>
 5 #include <script/sign.h>
 6 #include <uint256.h>
 7+#include <test/util/random.h>
 8 #include <util/translation.h>
 9 
10 enum class InputType {
11@@ -66,5 +67,33 @@
12 static void SignTransactionECDSA(benchmark::Bench& bench)   { SignTransactionSingleInput(bench, InputType::P2WPKH); }
13 static void SignTransactionSchnorr(benchmark::Bench& bench) { SignTransactionSingleInput(bench, InputType::P2TR);   }
14 
15+static void SignSchnorrBenchmark(benchmark::Bench& bench, bool use_null_merkle_root)
16+{
17+    ECC_Context ecc_context{};
18+
19+    auto key = GenerateRandomKey();
20+    auto msg = InsecureRand256();
21+    auto merkle_root = use_null_merkle_root ? uint256() : InsecureRand256();
22+    auto aux = InsecureRand256();
23+    std::vector<unsigned char> sig(64);
24+
25+    bench.batch(1).unit("signature").run([&] {
26+        bool success = key.SignSchnorr(msg, sig, &merkle_root, aux);
27+        assert(success);
28+    });
29+}
30+
31+static void SignSchnorrWithMerkleRoot(benchmark::Bench& bench)
32+{
33+    SignSchnorrBenchmark(bench, /*use_null_merkle_root=*/false);
34+}
35+
36+static void SignSchnorrWithNullMerkleRoot(benchmark::Bench& bench)
37+{
38+    SignSchnorrBenchmark(bench, /*use_null_merkle_root=*/true);
39+}
40+
41 BENCHMARK(SignTransactionECDSA, benchmark::PriorityLevel::HIGH);
42 BENCHMARK(SignTransactionSchnorr, benchmark::PriorityLevel::HIGH);
43+BENCHMARK(SignSchnorrWithMerkleRoot, benchmark::PriorityLevel::HIGH);
44+BENCHMARK(SignSchnorrWithNullMerkleRoot, benchmark::PriorityLevel::HIGH);

which reveals: before:

ns/signature	signature/s	err%	total	benchmark
69,383.89	14,412.57	0.1%	1.06	`SignSchnorrWithMerkleRoot`
69,548.30	14,378.50	0.0%	1.06	`SignSchnorrWithNullMerkleRoot`

ns/signature	signature/s	err%	total	benchmark
70,189.31	14,247.18	0.2%	1.07	`SignSchnorrWithMerkleRoot`
69,464.39	14,395.87	0.1%	1.06	`SignSchnorrWithNullMerkleRoot`

after:

ns/signature	signature/s	err%	total	benchmark
83,055.75	12,040.11	0.2%	1.08	`SignSchnorrWithMerkleRoot`
80,971.71	12,349.99	0.1%	1.08	`SignSchnorrWithNullMerkleRoot`

ns/signature	signature/s	err%	total	benchmark
81,841.15	12,218.79	0.1%	1.08	`SignSchnorrWithMerkleRoot`
82,112.31	12,178.44	0.1%	1.08	`SignSchnorrWithNullMerkleRoot`

i.e because of the signing code simplifications signing is now ~15% slower. We could commit the benchmarks and consider a speedup in a separate PR, if this is problematic.

in src/key.cpp:274 in 7d160f116c outdated

278-        CPubKey pubkey_bytes = GetPubKey();
279-        uint256 tweak = XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);
280-        if (!secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, &keypair, tweak.data())) return false;
281-    }
282-    bool ret = secp256k1_schnorrsig_sign32(secp256k1_context_sign, sig.data(), hash.data(), &keypair, aux.data());
283+    KeyPair kp = ComputeKeyPair(merkle_root);

ismaelsadeeq commented at 5:04 pm on August 2, 2024:

nitty-nit: I think ConstructKeyPair might be a better name?

josibake commented at 1:37 pm on August 3, 2024:

I prefer to use Compute* whenever an actual computation is happening. In this case, calling ComputeKeyPair will compute a public key for the private key and (optionally) tweak the newly created key pair.

ismaelsadeeq commented at 9:26 am on August 5, 2024:

fair enough

in src/key.cpp:428 in a380faa124 outdated

419@@ -431,6 +420,21 @@ KeyPair::KeyPair(const CKey& key, const uint256* merkle_root)
420     if (!success) ClearKeyPairData();
421 }
422 
423+bool KeyPair::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256& aux) const
424+{
425+    assert(sig.size() == 64);

ismaelsadeeq commented at 5:35 pm on August 2, 2024:

m_keypair->data() can be a nullptr, should we return false early if it is?

josibake commented at 1:32 pm on August 3, 2024:

Good point. Since a KeyPair can be constructed and in an invalid state, I’ve moved the if (!IsValid()) return false; from CKey::SignSchnorr to here.

in src/test/key_tests.cpp:300 in 9afa2c9e50 outdated

299@@ -300,6 +300,13 @@ BOOST_AUTO_TEST_CASE(bip340_test_vectors)
300         // Verify those signatures for good measure.

ismaelsadeeq commented at 5:45 pm on August 2, 2024:

Is 9afa2c9e50370b2918377f3f3eac0950a4296ec0 needed, I think this is just a duplicate, because key.SignSchnorr is implicitly doing the same thing.

josibake commented at 1:35 pm on August 3, 2024:

I don’t think there is any harm having a test for both and have a slight preference for keeping it this way only because if someone were to change CKey::SignSchnorr in the future (e.g., to not use KeyPair or remove the method entirely), KeyPair would end up untested.

ismaelsadeeq commented at 9:38 am on August 5, 2024:

Yes, good point.

josibake force-pushed on Aug 3, 2024

bench: add benchmark for signing with a taptweak

Add benchmarks for signing with null and non-null merkle_root arguments.
Null and non-null merkle_root arguments will apply the taptweaks
H_TapTweak(P) and H_TapTweak(P | merkle_root), respectively, to the
private key during signing.

This benchmark is added to verify there are no significant performance
changes after moving the taptweak signing logic in a later commit.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>

f14900b6e4

tests: add key tweak smoke test

Sanity check that using CKey/CPubKey directly vs using secp256k1_keypair objects
returns the same results for BIP341 key tweaking.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>

5d507a0091

crypto: add KeyPair wrapper class

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

c39fd39ba8

josibake force-pushed on Aug 3, 2024

josibake commented at 1:29 pm on August 3, 2024: member

Update https://github.com/bitcoin/bitcoin/commit/9afa2c9e50370b2918377f3f3eac0950a4296ec0 -> https://github.com/bitcoin/bitcoin/commit/bfb2e6bcef0269141453e43cad56c566c33b9666 (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

paplorinc approved

paplorinc commented at 2:01 pm on August 3, 2024: contributor

utACK bfb2e6bcef0269141453e43cad56c566c33b9666

Thanks for your perseverance!

DrahtBot requested review from itornaza on Aug 3, 2024

DrahtBot requested review from ismaelsadeeq on Aug 3, 2024

in src/key.cpp:277 in 4cb66dea08 outdated

283-        if (!secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, &keypair, tweak.data())) return false;
284-    }
285-    bool ret = secp256k1_schnorrsig_sign32(secp256k1_context_sign, sig.data(), hash.data(), &keypair, aux.data());
286+    KeyPair kp = ComputeKeyPair(merkle_root);
287+    if (!kp.IsValid()) return false;
288+    auto keypair = reinterpret_cast<const secp256k1_keypair*>(kp.data());

paplorinc commented at 4:13 pm on August 3, 2024:

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

josibake commented at 6:55 am on August 4, 2024:

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.

josibake force-pushed on Aug 4, 2024

refactor: move SignSchnorr to KeyPair

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

cebb08b121

tests: add tests for KeyPair

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

72a5822d43

refactor: remove un-tested early returns

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.

ec973dd197

josibake force-pushed on Aug 4, 2024

DrahtBot added the label CI failed on Aug 4, 2024

in src/test/key_tests.cpp:352 in 5d507a0091 outdated

345@@ -345,4 +346,31 @@ BOOST_AUTO_TEST_CASE(bip341_test_h)
346     BOOST_CHECK(XOnlyPubKey::NUMS_H == H);
347 }
348 
349+BOOST_AUTO_TEST_CASE(key_schnorr_tweak_smoke_test)
350+{
351+    // Sanity check to ensure we get the same tweak using CPubKey vs secp256k1 functions
352+    secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);

ismaelsadeeq commented at 8:20 am on August 5, 2024:

nit: secp256k1_context_create docs says

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

0    secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_NONE);

josibake commented at 11:08 am on August 5, 2024:

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

paplorinc commented at 11:11 am on August 5, 2024:

If you do that, please change other usages as well

josibake commented at 11:49 am on August 5, 2024:

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.

DrahtBot removed the label CI failed on Aug 5, 2024

in src/key.cpp:419 in cebb08b121 outdated

416     bool success = secp256k1_keypair_create(secp256k1_context_sign, keypair, UCharCast(key.data()));
417+    if (success && merkle_root) {
418+        secp256k1_xonly_pubkey pubkey;
419+        if (!secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, keypair)) return;
420+        unsigned char pubkey_bytes[32];
421+        if (!secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey)) return;

ismaelsadeeq commented at 9:24 am on August 5, 2024:

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.

josibake commented at 11:19 am on August 5, 2024:

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.

in src/key.cpp:414 in ec973dd197

413@@ -414,9 +414,9 @@ KeyPair::KeyPair(const CKey& key, const uint256* merkle_root)
414     bool success = secp256k1_keypair_create(secp256k1_context_sign, keypair, UCharCast(key.data()));

ismaelsadeeq commented at 9:39 am on August 5, 2024:

nice adding ec973dd19719541dbcd6f3a6facf6f5dd7cf439c

ismaelsadeeq approved

ismaelsadeeq commented at 10:34 am on August 5, 2024: member

Code review ACK ec973dd19719541dbcd6f3a6facf6f5dd7cf439c

DrahtBot requested review from paplorinc on Aug 5, 2024

paplorinc commented at 10:51 am on August 5, 2024: contributor

ACK ec973dd19719541dbcd6f3a6facf6f5dd7cf439c - will happily reack if you decide to apply @ismaelsadeeq’s suggestions

itornaza approved

itornaza commented at 3:29 pm on August 5, 2024: none

trACK ec973dd19719541dbcd6f3a6facf6f5dd7cf439c

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

in src/test/key_tests.cpp:355 in 5d507a0091 outdated

350+{
351+    // Sanity check to ensure we get the same tweak using CPubKey vs secp256k1 functions
352+    secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
353+
354+    CKey key;
355+    key.MakeNewKey(true);

theStack commented at 4:46 pm on August 6, 2024:

nit, if you have to retouch:

0    CKey key = GenerateRandomKey();

in src/test/key_tests.cpp:369 in 5d507a0091 outdated

364+    BOOST_CHECK(secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, xonly_bytes, &xonly_pubkey));
365+    uint256 tweak_old = XOnlyPubKey(xonly_bytes).ComputeTapTweakHash(&merkle_root);
366+
367+    // CPubKey
368+    CPubKey pubkey = key.GetPubKey();
369+    uint256 tweak_new = XOnlyPubKey(pubkey).ComputeTapTweakHash(&merkle_root);

theStack commented at 5:38 pm on August 6, 2024:

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 (review) ff., so feel free to ignore).

paplorinc commented at 7:49 am on August 7, 2024:

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

in src/key.cpp:419 in ec973dd197

413@@ -414,9 +414,9 @@ KeyPair::KeyPair(const CKey& key, const uint256* merkle_root)
414     bool success = secp256k1_keypair_create(secp256k1_context_sign, keypair, UCharCast(key.data()));
415     if (success && merkle_root) {
416         secp256k1_xonly_pubkey pubkey;
417-        if (!secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, keypair)) return;
418         unsigned char pubkey_bytes[32];
419-        if (!secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey)) return;
420+        assert(secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, keypair));
421+        assert(secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey));

theStack commented at 5:55 pm on August 6, 2024:

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

paplorinc commented at 8:01 am on August 7, 2024:

We’ve had something like that before: #30051 (review)

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.

theStack approved

theStack commented at 5:59 pm on August 6, 2024: contributor

Code-review ACK ec973dd19719541dbcd6f3a6facf6f5dd7cf439c

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

ryanofsky assigned ryanofsky on Aug 7, 2024

ryanofsky merged this on Aug 7, 2024

ryanofsky closed this on Aug 7, 2024

Shuhaib07 commented at 7:03 pm on August 7, 2024: none

Okay

hebasto added the label Needs CMake port on Aug 9, 2024

hebasto commented at 2:08 pm on August 9, 2024: member

The buildsystem related stuff has been ported to the CMake-based buildsystem in https://github.com/hebasto/bitcoin/pull/317.

hebasto removed the label Needs CMake port on Aug 9, 2024

hebasto referenced this in commit 783a27b71d on Aug 9, 2024

Shuhaib07 commented at 4:21 am on August 12, 2024: none

Approve

bitcoin deleted a comment on Aug 12, 2024

crypto, refactor: add new KeyPair class #30051

Code Coverage

Reviews

Conflicts