Add sign_ecdsa_with_noncedata and sign_ecdsa_recoverable_with_noncedata

[junderw]

Fixes #424

As discussed on IRC (starts at 09:14).

These methods will allow for users to generate multiple signatures with the same private key and message by utilizing one of the Variants mention in RFC6979 which is exposed by libsecp256k1 via the noncedata argument.

The reasoning behind adding this is to allow our library to migrate from using the -sys crate. Currently we support using this noncedata argument, and would like to continue doing so while at the same time migrating away from -sys crate.

[junderw]

I have verified the fixed test vectors with node-secp256k1.

[tcharding]

FWIW, seems sane to me. I had some nits during review but your PR is perfectly in line with the rest of the module so I didn't bother with them :)

Thanks!

[apoelstra]

Where was this discussed on IRC? You can generate multiple signatures with the same privkey and message with or without extra noncedata.

It's fine to add these methods but they are not needed for security.

[junderw]

they are not needed for security.

Perhaps my wording was off.

In other words: "You can generate multiple signatures with the same private key and message while still using RFC6979 deterministic nonce generation for signatures."

This is not to say non-RFC6979-compliant schemes can not be secure, though.

Nor is it to say that using sign_ecdsa (using RFC6979 without the "extra entropy") is insecure... just that some users of our library asked for the ability to utilize the noncedata portion of libsecp256k1, so we added it to our API, and then we switched over to using Rust+WASM with secp256k1-sys, and I am moving it to use secp256k1 now, and noticed this API was missing.

[junderw]

IRC logs: https://gnusha.org/bitcoin-rust/2022-03-19.log

Starts at 09:14

[apoelstra]

From your logs, and this conversation, I am even more confused about the motivation for this. Low-R is directly supported by the library in the sign_ecdsa_grind_r function.

This PR does nothing to change whether or not the user can switch off of RFC6979, it only gives them the option to pass extra entropy into 6979 (ok, this technically deviates from the spec, but it's still morally 6979).

In other words: "You can generate multiple signatures with the same private key and message while still using RFC6979 deterministic nonce generation for signatures."

Yes. But this has nothing at all to do with the ability to add extra entropy.

[apoelstra]

Anyway concept ACK adding this functionality. Though I'd appreciate if you edited the PR description to say that it's because it's a libsecp functionality that you are using, and want to keep using, rather than the "security" justification.

[junderw]

this technically deviates from the spec

While true that this functionality is only mentioned in the 3.6. Variants section under the 2nd bullet point ("Additional data may be added to the input of HMAC, concatenated after bits2octets(H(m)):") it's one of those cases where "is part of the spec" is grey depending on what one considers "the spec"... since it's a semi-official "Variant" of the spec.

---

I am even more confused about the motivation for this. Low-R is directly supported by the library in the sign_ecdsa_grind_r function.

I mention possibly using the low R function this library provides if we decided to explicitly remove the noncedata feature from our library.

[From IRC]:
11:30 < junderw[m]> We could move low R grinding to this lower API that uses Rust, and then use the low R grinding method rust-bitcoin has.
11:30 < junderw[m]> But that's a breaking change, and a lot of shuffling around to explicitly remove a feature that someone requested.

We would need to remove the ability to add in the "Variant RFC6979" aka noncedata feature from our library, which was explicitly asked for by a user.

I only mentioned "low R" feature because it's the only time that any of our other higher level libraries make use of the noncedata feature, but I then admitted that our only use of the feature could theoretically be replaced by removing noncedata support and just wrapping the low R function from this library, but that would only be if we explicitly wanted to remove noncedata support, which we don't.

In retrospect, I shouldn't have mentioned anything unrelated to the motivation of this feature addition. Including the information of how our other libraries use the feature and how those use cases could be replaced by using another feature in this library is irrelevant to my case. The minimal info I should have relayed was:

1. Users asked for noncedata support, so we added it long before switching to Rust+WASM and are maintaining it
2. We are trying to move away from -sys crate we use currently
3. noncedata support was not in the main crate

---

I'd appreciate if you edited the PR description to say that it's because it's a libsecp functionality that you are using, and want to keep using, rather than the "security" justification.

Done.

[apoelstra]

Thanks! Sorry for all the noise -- concept ACK this PR and its description. I will review it more carefully tomorrow.

[apoelstra]

ACK f93ca81

[apoelstra]

Looks good to me! I'm a little skeptical that the _pointer variants are super useful but they don't hurt anything.

[junderw]

👍 I don't mind if they get refactored away someday.

I tend to split things into multiple lines and add descriptive extra variables to make the lines shorter and easier to read.