Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ECDSA sign-to-contract module #111

Conversation

apoelstra
Copy link
Contributor

This is a backport and rebase of bitcoin-core/secp256k1#669

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general. I think we can simplify a few things as mentioned in the comments. Also, we should add ecdsa_sign_s2c and ecdsa_s2c_anti_nonce_covert_channel_client_commit to the ctime_tests.

src/secp256k1.c Outdated Show resolved Hide resolved
src/secp256k1.c Outdated Show resolved Hide resolved
src/secp256k1.c Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
include/secp256k1.h Outdated Show resolved Hide resolved
src/modules/ecdsa_sign_to_contract/main_impl.h Outdated Show resolved Hide resolved
include/secp256k1_ecdsa_sign_to_contract.h Outdated Show resolved Hide resolved
src/modules/ecdsa_sign_to_contract/main_impl.h Outdated Show resolved Hide resolved
src/modules/ecdsa_sign_to_contract/tests_impl.h Outdated Show resolved Hide resolved
src/modules/ecdsa_sign_to_contract/tests_impl.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these functions need to be added to the constant-time tests.

src/secp256k1.c Outdated
if (!secp256k1_ec_commit_tweak(ctx, tweak, pubkey, data, data_size)) {
return 0;
}
return secp256k1_ec_privkey_tweak_add(ctx, seckey, tweak);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be secp256k1_ec_seckey_tweak_add (we renamed those, the old ones are deprecated)

src/secp256k1.c Outdated
Comment on lines 779 to 781
if (data_size == 0) {
/* That's probably not what the caller wanted */
return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this. The empty string is a valid thing to commit to, depending on the scenario. I think we should avoid this special casing also because it makes things harder for wrappers such as the language bindings.

Copy link
Contributor

@jonasnick jonasnick Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and we can make this function void

include/secp256k1.h Outdated Show resolved Hide resolved
src/tests.c Outdated
Comment on lines 4614 to 4615
/* Uninitialized opening can't be serialized. Actually testing that would be
* undefined behavior. Therefore we simulate it by setting the opening to 0. */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd just say "Opening with all bytes set to zero can't be serialized."

(Whether the uninitialized opening can be serialized is UB, so this is indeed nothing we can test.)

extern "C" {
#endif

/** Same as secp256k1_ecdsa_sign, but s2c_data32 is committed to by adding `hash(k*G, s2c_data32)` to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: formally, "G" is not specified here... Feel free to ignore.

* commitment to the data is known. This is for example important in the
* anti-nonce-covert-channel protocol.
*/
secp256k1_sha256 sha;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a tagged hash.

src/modules/ecdsa_sign_to_contract/main_impl.h Outdated Show resolved Hide resolved
ARG_CHECK(rand_commitment32 != NULL);
ARG_CHECK(rand32 != NULL);

secp256k1_sha256_initialize(&sha);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tagged hash

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would need to be same tagged hash as secdsa_s2c_sign uses to create noncedata from the s2c_data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want two separate tagged hashes, one for the data commitment and the other for the P→P+eG commitment?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes. We should separate domains as much as possible.

while (1) {
int overflow = 0;
if (!secp256k1_nonce_function_default(nonce32, msg32, seckey32, NULL, rand_commitment32, count)) {
/* cannot happen with secp256k1_nonce_function_default */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also call the error callback here, not sure if it's better.

size_t i;
unsigned char privkey[32] = {
0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55,
0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0x55,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL that trailing commas are allowed here. :D

@apoelstra
Copy link
Contributor Author

apoelstra commented Dec 6, 2020

Redid the whole API. I think I addressed all comments except to add ctime tests, which I will do in another commit.

@apoelstra apoelstra force-pushed the 2020-11--s2c-backport-669 branch 4 times, most recently from 7746166 to 56bc956 Compare December 6, 2020 22:29
include/secp256k1_ecdsa_s2c.h Show resolved Hide resolved
include/secp256k1_ecdsa_s2c.h Outdated Show resolved Hide resolved
src/secp256k1.c Outdated Show resolved Hide resolved
include/secp256k1_ecdsa_s2c.h Outdated Show resolved Hide resolved
include/secp256k1_ecdsa_s2c.h Show resolved Hide resolved
include/secp256k1_ecdsa_s2c.h Outdated Show resolved Hide resolved
src/secp256k1.c Outdated Show resolved Hide resolved
src/eccommit_impl.h Outdated Show resolved Hide resolved
@apoelstra apoelstra force-pushed the 2020-11--s2c-backport-669 branch 2 times, most recently from 6e2e5eb to 21b1980 Compare December 8, 2020 23:56
Comment on lines 125 to 126
* If this fails, it indicates a potentially serious problem with the hardware device,
* and we strongly discourage its continued use.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps s/fails/fails or if the signing device aborts after step 3/ and s/use./use (see Rationale below)./ With rationale

  • If the hardware device is not discarded after wrongly or not at all responding after step 3, the hardware device can successfully exfiltrate the signing key even if the host only lets signatures pass after verifying the commitment. This is because after step 3, the signing device knows the final nonce and can simply abort if it does not match the desired bias.

Hm, I'm really skeptical about implementers getting this right.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah... Idk, throwing away the device may not be a solution either if it's the only thing that stores your key. It's just hard to give advice in that case.

We should consistently call it "signing device" or "hardware device".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing away the signer cause of one failed session might be too much, especially since there might be transient errors (e.g. a communication error after step 3). It might be good to distinguish between a stopped process after step 3, and an actually failed commitment check.

Maybe something like 'If the signing process fails repeatedly'? To produce even one signature with a desired bias, multiple attempts by the signing device might be needed, and multiple signatures with a correct bias are needed to leak a full key.

Of course, it might fail 'repeatedly' over many days or months and for many different transactions, which is harder to recognize for a user 🤷

Copy link
Contributor Author

@apoelstra apoelstra Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, failing every few weeks or months is not a big deal. In principle you can exfiltrate a nonce that way but in practice you cannot. Even if you have a whole bit of bias (and failing every other time is likely to make a user throw away your device even if they don't know any crypto..) you still need hundreds of signatures with the same key to exfiltrate a nonce this way. And a single bit is on the edge of what's computationally feasible to work with.

It's probably fine to say "if this fails a noticeable number of times, over the lifetime of the device, you should throw it away".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonasnick

If the hardware device is not discarded after wrongly or not at all responding after step 3, the hardware device can successfully exfiltrate the signing key even if the host only lets signatures pass after verifying the commitment. This is because after step 3, the signing device knows the final nonce and can simply abort if it does not match the desired bias.

It only knows the final nonce if the host nonce is the same in a restart, right? If the host picks a new random nonce contribution every time, a restart after step 2 should be safe.

So the comment in the code:

If, at any point from this step onward, the hardware device fails, it is
okay to restart the protocol using exactly the same rho and checking
that the hardware device proposes exactly the same R. Otherwise, the
hardware device may be selectively aborting and thereby biasing the set of
nonces that are used in actual signatures.

is not complete I think. We could add that the protocol can also be restarted with a fresh rho.

Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand

  1. Restart after step 2 is the same as restart after step 3, because the host must provide the same rho in step 3.
  2. Restarting with a fresh rho means restarting from step 1 which allows the hardware wallet to bias the final nonce.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restart after step 2 is the same as restart after step 3, because the host must provide the same rho in step 3.

How so? After step 2 (before step 3), the signer does not know the final nonce yet.

I get that after step 3, the signer knows rho and with that the final nonce, so it can introduce bias by aborting the process.

But if there is a restart before that (e.g. after the signer commitment is delivered, before sending the rho), restarting with a fresh rho should be fine?

Not that it matters much in practice, it was more for my information and to maybe move the comment in the code to step 3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think I misunderstood.

a restart after step 2 should be safe.

You mean you can restart the whole protocol if step 3 hasn't happened yet ( I thought you mean that after restart the protocol can continue after step 2). I think in that case it can also be run with the same rho.The host can just re-request step 2 if the HWW failst to provide a step 2 messge. Yeah I think it would make sense to move the "If, at any point from this step onward, the hardware device fails, it is [...]" section to step 3 but on the other hand it shouldn't make a difference in practice - at least I don't really see how the protocol can fail at step 3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming.

at least I don't really see how the protocol can fail at step 3.

When the host sends rho, the signer could just not respond, or respond with an error message, or respond with invalid data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably fine to say "if this fails a noticeable number of times, over the lifetime of the device, you should throw it away".

Throwing it away isn't enough. You should act like your master secret has been compromised.

* and by Pieter Wuille (as "Scheme 6") here:
* https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-March/017667.html
*
* In order to ensure the host cannot trick the signing device into revealing its
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worries me that this is a mix of protocol description and usage instructions.

I think it will be easier for the user to have clear instructions with just what functions must be called in which order and on what device and what messages are transferred. And we can still have a protocol description.

Host:

  • Call..
  • Initiate signing session by sending...
  • ..

Singing device:

  • Listen for messages ...
  • Call...
  • ...

Now that I wrote that, this will ideally be example code. But we could do this in another PR instead if that's easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it in another PR. I think it's reasonable to have a protocol description which includes which API functions correspond to what steps.

@real-or-random
Copy link
Collaborator

Note (no need to do this in this PR, I just want to write it down):

I think we could add synthetic nonces to this scheme. Sure this will break its determinism, and then the HW needs to keep state. But I think there’s no need for secure storage. HW could simply draw some aux_rand, add it to the nonce derivation and store it. Then in the second round, look up aux_rand and recompute the nonce.

@apoelstra
Copy link
Contributor Author

apoelstra commented Dec 11, 2020

remaining todo

  • update docs for what to do in case of failure
  • make anti_klepto functions for all steps, even if they are just wrappers (or almost-wrappers) around s2c
  • add example code
  • check on that overflow check we don't do in s2c (should we fail? or comment that sigcheck would fail?)

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

859d73e lgtm mod nit. Feel free to squash.

include/secp256k1_ecdsa_s2c.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the API is pretty nice now!
I haven't checked the tests in detail. Sorry that this is my first in-depth review and that I have many late comments now....

include/secp256k1_ecdsa_s2c.h Outdated Show resolved Hide resolved
include/secp256k1_ecdsa_s2c.h Outdated Show resolved Hide resolved
include/secp256k1_ecdsa_s2c.h Outdated Show resolved Hide resolved
* seckey: pointer to a 32-byte secret key (cannot be NULL)
* s2c_data32: pointer to a 32-byte data to commit to in the nonce (cannot be NULL)
*/
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ecdsa_s2c_sign(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the normal sign function does not have SECP256K1_WARN_UNUSED_RESULT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, it should.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe, no idea. I suspect it's like this on purpose. The unused result is catastrophic for verification but typically not for signing. If you ask me, I'd prefer to keep it consistent but it doesn't really matter in the end.

src/eccommit.h Outdated Show resolved Hide resolved
src/modules/ecdsa_s2c/main_impl.h Outdated Show resolved Hide resolved
src/modules/ecdsa_s2c/main_impl.h Outdated Show resolved Hide resolved
Comment on lines 121 to 126
/* Check that sig_r == commitment_x (mod n)
* sig_r is the x coordinate of R represented by a scalar.
* commitment_x is the x coordinate of the commitment (field element).
*
* It is sufficient to only compare the x coordinates as it is as difficult to find an client
* commitment to the negation of the point as to any other point. There is a small reduction in
* security as it is easier to find a collision with a point and its negation.
*/
secp256k1_ecdsa_signature_load(ctx, &sigr, &sigs, sig);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this comment is right, this is still binding commitment scheme even if we don't verify the y bit.

But now the commitment itself is malleable, and I think we should simply avoid this. This may also ask for re-implementations of this scheme to disagree if this bit should be checked or not (like with the EdDSA mess). I think we should implement the strongest possible check.

In particular I don't think we save much performance by not checking the y coordinate. The same is true for code complexity. The additional code is probably fewer lines than this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the additional code? I need to verify the ECDSA siganture to get the full R point don't I?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok, I understand now. I was on the wrong here track, and I believe the comment led me to onto this track.

It is sufficient to only compare the x coordinates as it is as difficult to find an client commitment to the negation of the point as to any other point. There is a small reduction in security as it is easier to find a collision with a point and its negation.

The commitment comprises only the x-coordinate, so you can of course only verify the x-coordinate. And I would not call this a reduction in security. If you don't include y-bit, you can't expect that it provides any security.

Having said this, it's true that if the signature is valid, it implies a particular y-bit, so for valid signatures, you could compare the entire point. But since we separated verification of the commitment and the signature, this is not relevant for us.

Now when I read the comment again, it makes sense in the context of the antiklepto protocol. (Note also the term "client commitment".) But this should not bother us either since we also separated s2c and antiklepto.

src/modules/ecdsa_s2c/main_impl.h Outdated Show resolved Hide resolved
src/secp256k1.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixes look good, feel free to squash. 👍

src/secp256k1.c Outdated Show resolved Hide resolved
const unsigned char* msg32,
const unsigned char* seckey,
const unsigned char* host_data32
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4) SECP256K1_ARG_NONNULL(5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add int* recid here and populate it if it is not NULL? It is only a matter of passing the argument down to secp256k1_ecdsa_sign_inner().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't want to support recoverable signatures any more than we absolutely have to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the BitBox02 we make use of it. I plan on rolling out the anti-klepto protocol there, so it would definitely make my life a bit easier. Otherwise I will likely maintain a small commit on top which adds this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm gonna make you maintain a patch, sorry. It's only a couple lines for you to expose this variable but for us to do it properly we'd need to add another function gated on the SECP256K1_RECOVERY feature flag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but for my information: why gate it behind SECP256K1_RECOVERY? I would have just exposed without that. It is not in the recovery module after all, all the functionality is present in secp256k1.c.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because SECP256K1_RECOVERY is the (semi-deprecated) feature flag to get recoverable signatures :). We don't want to support back-door ways to get this functionality.

@benma
Copy link
Contributor

benma commented Dec 16, 2020

tACK 374d944. I used this branch to add support for the anti klepto protocol when signing a transaction in the BitBox02, including successful host verification in the bitbox02 client libraries. Thanks again for picking up my original PR.

@apoelstra
Copy link
Contributor Author

Okay, I think we're actually good to go now.

* hardware device may be selectively aborting and thereby biasing the set of
* nonces that are used in actual signatures.
*
* It takes many (>100) such aborts before there is a plausible attack, given
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Micro-nit: I'd say it takes about a 100 such aborts not more. To communicate 200 bits you need no more than 100 aborts on average and the remaining 56 bits are somewhat possible to brute-force.

src/eccommit.h Outdated
static int secp256k1_ec_pubkey_tweak_add_helper(const secp256k1_ecmult_context* ecmult_ctx, secp256k1_ge *p, const unsigned char *tweak);

/** Compute an ec commitment tweak as hash(pubkey, data). */
static void secp256k1_ec_commit_tweak(unsigned char *tweak32, secp256k1_ge* pubp, secp256k1_sha256* sha, const unsigned char *data, size_t data_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should document in (all) ec_commit functions that pubp must not be the point at infinity. Or alternatively, correctly deal with that case and declassify only the infinity bit in ecdsa_sign_inner. I implemented that here: https://github.com/jonasnick/secp256k1-zkp/commits/ecdsa-s2c-inf Feel free to cherry-pick & squash.

@real-or-random
Copy link
Collaborator

Fixes look good.

I think this one is still open:
#111 (comment) (change the comment)

And not sure if you overlooked that one or delibaretly choose to keep the current version:
#111 (comment)

@apoelstra
Copy link
Contributor Author

@jonasnick sure, cherry-picked your commit. I have a mild preference for just documenting that the function won't work, since all of our usage avoids the point at infinity, but since you took the time to implement this and add a test and fix the ctime tests, I might as well take your complete work :).

@real-or-random ok changed the comment. It was unclear to me from your earlier comment that you actually wanted me to make a change there.

@jonasnick
Copy link
Contributor

Thought that the ec_commit functions will be useful in other contexts where it isn't as clear that there is no point at infinity. PR looks good. Are you going to squash this?

@apoelstra
Copy link
Contributor Author

Yep, give me 10 minutes to squash.

apoelstra and others added 5 commits December 21, 2020 20:49
Co-authored-by: Marko Bencun <mbencun+pgp@gmail.com>
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
Co-authored-by: Marko Bencun <mbencun+pgp@gmail.com>
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
Co-authored-by: Marko Bencun <mbencun+pgp@gmail.com>
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
Co-authored-by: Marko Bencun <mbencun+pgp@gmail.com>
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
@apoelstra
Copy link
Contributor Author

Heh, took more than ten minutes. But there you go.

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 👍

ACK 47efb5e

@benma
Copy link
Contributor

benma commented Dec 29, 2020

@apoelstra is this PR ready to be merged? I have a bit of downtime to work on the downstream integration.

@apoelstra
Copy link
Contributor Author

I think so. @real-or-random @jonasnick can you merge this?

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 47efb5e

@real-or-random real-or-random merged commit 673e551 into BlockstreamResearch:secp256k1-zkp Jan 4, 2021
@benma
Copy link
Contributor

benma commented Jan 4, 2021

Awesome. Thanks everyone!


*commitp = *pubp;
return secp256k1_ec_commit_tweak(tweak, commitp, sha, data, data_size)
&& secp256k1_ec_pubkey_tweak_add_helper(ecmult_ctx, commitp, tweak);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

secp256k1_ec_pubkey_tweak_add_helper (and secp256k1_ec_seckey_tweak_add_helper below) implement the unusual BIP32 semantics of failing when the tweak value exceeds the group order. No other modern protocol does this. BIP-340's default signing takes k' = int(rand) mod n; Jonas's proposed key aggregation's ComputeCoefficient is taken modulo n. And when you write up the ecdsa_s2c protocol documentation, I expect you will also want to take the this tweak value modulo n.

I don't think we want to fold in BIP32's behavior into the s2c protocol specification, just because it is expedient from a libsecp256k1 implementation perspective.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jonas points out that the tweaking in BIP-341 isn't taken modulo n.

So I give up. Tweaks are not taken modulo n for some reason, but everything else is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants