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.
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
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:
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?
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..
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.
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.
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.
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
ACKbdc2a656c2d2a61d226fde1d1fd4e79664106e18
in
src/test/key_tests.cpp:333
in
bdc2a656c2outdated
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));
329330+ // 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));
nit: BOOST_CHECK_MESSAGE often produces more readable results, but it may be inconvenient to set it up properly. If you think it adds value, it may be helpful to add some debug info as a message in case of likely failures.
I don’t think it adds much here? We are using BOOST_CHECK to to ensure the function call succeeds before proceeding, so getting a line number failure for BOOST_CHECK seems sufficient for debugging. Also, if dealing with valid keys, this function should never fail.
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.
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 */
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
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.
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.
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
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
4e8d61e750outdated
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());
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.
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.
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.
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?
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).
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).
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.
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!
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).
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).
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.
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?
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.
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.
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.
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.
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.
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?
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.
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
3ca329795coutdated
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; }
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.
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?
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?
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.
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:
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).
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
82af81b51aoutdated
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)
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
291292 /**
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
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)?
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?
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
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 tACK5e9f4f41de98c2cdcc74e31fe925b2f5cf9f6aec
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
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
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.
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>):
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
7bbb215e57outdated
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);
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.
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
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
49057fc4f1outdated
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));
302303+ // 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);
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
itornaza
commented at 6:10 pm on July 21, 2024:
none
re ACKb8b3a9f18670ec7bf246a57950cdae7e034a264d
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
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 :)
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
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
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?
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.
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.
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
fa855cfa0doutdated
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; }
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).
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:
(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:
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:
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:
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.
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:
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
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
utACKbfb2e6bcef0269141453e43cad56c566c33b9666
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
4cb66dea08outdated
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());
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
5d507a0091outdated
345@@ -345,4 +346,31 @@ BOOST_AUTO_TEST_CASE(bip341_test_h)
346 BOOST_CHECK(XOnlyPubKey::NUMS_H == H);
347 }
348349+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.
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
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.
DrahtBot requested review from paplorinc
on Aug 5, 2024
paplorinc
commented at 10:51 am on August 5, 2024:
contributor
ACKec973dd19719541dbcd6f3a6facf6f5dd7cf439c - 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
5d507a0091outdated
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);
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).
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.)
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
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-06-27 00:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
