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

Nonce functions #757

Open
real-or-random opened this issue Jun 10, 2020 · 5 comments
Open

Nonce functions #757

real-or-random opened this issue Jun 10, 2020 · 5 comments

Comments

@real-or-random
Copy link
Contributor

At the moment you can call secp256k1_ecdsa_sign with parameters noncefp == NULL and noncedata != NULL. This basically says "give me whatever function you think you should be the default but feed it this data", which is not really meaningful. This will be relevant if we want to change the default in the future, which is not too unlikely I think.

I see two ways:

  • Either we simply disallow this call (ARG_CHECK), or
  • we provide the guarantee that any default function will accept and use some (fixed-length) string of additional entropy.

After the investigation into synthetic nonces in BIP340, I slightly prefer the second option because I don't want to discourage people from adding entropy while leaving the choice of the function to the library. What do you think?

@jonasnick
Copy link
Contributor

I don't want to discourage people from adding entropy while leaving the choice of the function to the library.

I'd be surprised if people don't make use of this already and while it's not explicitly documented I don't think we would ever switch the default to a function that doesn't add noncedata entropy.

@elichai
Copy link
Contributor

elichai commented Jun 15, 2020

I'd be surprised if people don't make use of this already

In rust-secp we don't, if this will be documented I'll add a feature that will seed randomness into the ecdsa_sign nonce function,

bitcoin core actually uses that entropy as counter and does not seed it (https://github.com/bitcoin/bitcoin/blob/195822f1e05e2f36002c906667d4c639663f23b5/src/key.cpp#L215 (I might open a PR to make the counter start at a random point))

@real-or-random
Copy link
Contributor Author

@elichai but both rust-secp and core specify secp256k1_nonce_function_rfc6979 instead of null.

bitcoin core actually uses that entropy as counter and does not seed it (https://github.com/bitcoin/bitcoin/blob/195822f1e05e2f36002c906667d4c639663f23b5/src/key.cpp#L215 (I might open a PR to make the counter start at a random point))

If we believe in the results of BIP340, we may even want to have randomness there in every call and not only at the start. https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#default-signing

We should also propose to change the derivation function to the one used in BIP340, but that's a larger thing. @jonasnick Is this something we could change in the Schnorr PR? Maybe introduce the nonce derivation function in a separate PR and make it available for ECDSA already now?

@jonasnick
Copy link
Contributor

The BIP340 nonce function as implemented in #558 is not a secp256k1_nonce_function because it takes the xonly_pubkey as an additional argument so we'd need some kind of wrapper. But would be easy to open a PR with a copy of the schnorrsig PR nonce function and rebase the schnorrsig PR on top if that gets merged.

@real-or-random
Copy link
Contributor Author

real-or-random commented Sep 23, 2020

So we he had to introduce a new API for Schnorr nonce functions ("hardened") in order to support hashing the public key. Can we works towards unifying the two types? Maybe with an optional public key argument, and if it's NULL, we hash a constant string?
edit: This could help us deprecating the current nonce function and switch the default nonce function for ECDSA to the BIP340 function.

Here's another issue:
Our BIP340 implementation implements an algo16 argument for the nonce function. I think the API is somewhat unusual and not really ergonomic. We read up to 16 bytes, ignoring trailing zero bytes. So this is neither an ordinary 16 byte array, nor is it an zero-terminated ordinary string.
We tend to avoid variable-length strings but maybe in this case expecting a byte array and a length will be better?

See

/* Tag the hash with algo16 which is important to avoid nonce reuse across
* algorithms. If this nonce function is used in BIP-340 signing as defined
* in the spec, an optimized tagging implementation is used. */
if (memcmp(algo16, bip340_algo16, 16) == 0) {
secp256k1_nonce_function_bip340_sha256_tagged(&sha);
} else {
int algo16_len = 16;
/* Remove terminating null bytes */
while (algo16_len > 0 && !algo16[algo16_len - 1]) {
algo16_len--;
}
secp256k1_sha256_initialize_tagged(&sha, algo16, algo16_len);
}

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

No branches or pull requests

3 participants