btcec/schnorr: document signing and verifying divergences against BIP340 and libsecp256k1 behavior - #2546
Conversation
fa873c7 to
eb63bb5
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the btcec/schnorr BIP340 signing and verification implementation to match the current BIP340/libsecp256k1 behavior, particularly around variable-length messages and default nonce derivation when auxiliary randomness is not provided.
Changes:
- Allow Schnorr signing and verification to accept variable-length messages (no 32-byte length enforcement).
- Change signing behavior so omitting
CustomNonceis treated equivalently to providing 32 zero bytes (matching libsecp256k1/BIP340 behavior, removing the RFC6979 fallback). - Update/extend test vectors to cover omitted auxRand behavior and non-32-byte message cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| btcec/schnorr/signature.go | Adjusts Sign/Verify behavior to accept variable-length messages and align default auxRand handling with BIP340/libsecp256k1; includes comment/doc updates needed. |
| btcec/schnorr/signature_test.go | Updates/extends BIP340 test vectors and signing options setup to validate new behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // When CustomNonce is not passed in, to generate the same signature as | ||
| // libsecp256k1 does, use a zeroed slice as the auxRand, otherwise, use | ||
| // the auxRand provided by the passed CustomNonce. | ||
| var authNonce [32]byte | ||
| if opts.authNonce != nil { | ||
| // Step 6. | ||
| // | ||
| // t = bytes(d) xor tagged_hash("BIP0340/aux", a) | ||
| privBytes := privKeyScalar.Bytes() | ||
| t := chainhash.TaggedHash( | ||
| chainhash.TagBIP0340Aux, (*opts.authNonce)[:], | ||
| ) | ||
| for i := 0; i < len(t); i++ { | ||
| t[i] ^= privBytes[i] | ||
| } | ||
| copy(authNonce[:], opts.authNonce[:]) | ||
| } |
There was a problem hiding this comment.
While this converges toward the secp approach, does it also introduce any nonce reuse risk?
If a bug causes signatures to be produced with a zeroed nonce, the private key could be extracted. Given that, I think falling back to RFC6979 would provide an extra safety layer.
Rejecting a zeroed nonce outright is another option, but that seems to conflict with the goal of this PR, which is to match secp signing behavior.
At minimum, I think it would be helpful to document why secp considers this safe with a zeroed nonce, and why this code historically used RFC6979 instead.
There was a problem hiding this comment.
We should also zero t. With a zero nonce, the XOR operation leaves a recoverable private key in that variable.
There was a problem hiding this comment.
The data derived from auxRand (t) is hashed with the public key and the message to generate rand, so, there's no risk of nonce reusage by using a zeroed auxRand. Also, the name CustomNonce is misleading, the correct name should be AuxRand, for this reason I marked CustomNonce as deprecated and added AuxRand in replacement.
Now, zeroing both t and privBytes.
There was a problem hiding this comment.
This implementation was written in the commit d6d38ad, which states:
The signing implementation by default, deviates from BIP-340 as it opts
to use rfc6979 deterministic signatures by default, which means callers
don't need to always pass in their own `auxNonce` randomness. A set of
functional arguments allows callers to pass in their own value, which is
the way all the included test vectors function.
Refreshing it again, BIP-340 allows the auxRand to be all zeroes, which is the case for multiple of the test vectors. The generated rand is always safe to use, different messages will result in different nonces, even with auxRand being all zeroes, so there's no safety added by using RFC6979 here, only additional compute cost.
libsecp256k1 states that passing null (which in this case is not passing AuxRand) is the same as passing the zeroed 32 byte array. Adopting this enables faster signature generation and a behavior that matches both libsecp256k1 and BIP-340 without compromising security.
Would be nice to see @Roasbeef take on this.
e34131a to
a842210
Compare
|
|
5dc2203 to
7d2a2e6
Compare
GustavoStingelin
left a comment
There was a problem hiding this comment.
Cool, LGTM!
I noticed a few linting issues, but it looks like linting is not running in CI yet. I opened an issue to track that separately: #2549
7d2a2e6 to
b6b32fe
Compare
So this was a purposeful decision w.r.t the API. Do you have a concrete use case of signing arbitrary length messages?
Again a divergence from the original design goals of the API. RFC6979 is a safer default, as it makes nonce re-use harder for callers. |
For now, the objective is to pass the BIP-340 test vectors.
The generated rand is always safe to use, different messages will result in different nonces, even with auxRand being all zeroes, so there's no safety added by using RFC6979 here, only additional compute cost. |
TechLateef
left a comment
There was a problem hiding this comment.
Since the nonce derivation in BIP340 includes the private key and message, zeroed auxRand doesn't introduce nonce reuse risk. That said, @Roasbeef concern about API safety for callers who don't understand the internals is worth considering maybe a doc comment explicitly warning that passing no AuxRand is equivalent to zeroed bytes (not random) would help callers make an informed decision.
|
Thanks for the work! Some thoughts: 1: I think we should stick to the original design with the 32 byte check since we're changing the public API and the behavior. If passing the BIP340 tests are the goal, testing the unexported functions 2: I went over RFC6979 vs 32 zeros and came to the conclusion that zeroing out doesn't really affect safety. For single sig it doesn't matter anyways since the r value is different for each message. For multi-sig, it doesn't matter if you're using 0s or RFC6979 since they're both deterministic. But again, since this will cause the API to break, I'm leaning towards keeping it the same. |
b6b32fe to
38b09ba
Compare
Add note to signing procedures about how to produce BIP340 compliant signature (passing zero initialized 32 byte array as CustomNonce). And also about the limitation of signing arbitrary size messages, as required by BIP340. Fix leakages of private key / nonce material. Fix reference docs for BIP-340 signature generation. Improved formatting.
38b09ba to
03e9fd4
Compare
|
As the deviation from BIP340 does not introduce vulnerabilities, I've changed the commit to keep the current behavior (as @kcalvinalvin suggested), but also added more documentation covering that BIP340 signatures can be generated by using a zero 32 byte array with CustomNonce and also about arbitrary length messages not being supported. Also fixed the docs and some private key and nonce leaks. |
Change Description
Sign and Verify: document the requirement of 32 byte messages, as BIP340 requires and libsecp256k1 supports variable length messages.
Sign: since RFC6979 is used when
CustomNonceis not passed, which is a deviation from BIP340 and secp256k1, a not was added explaining that passing an empty 32 byte array with CustomNonce is the same as the default behavior of libsecp256k1 when producing signatures.Several leakages of sensitive material were fixed.
I would like to make the default behavior BIP340 compliant, matching libsecp256k1, but as this would introduce different behavior, even with the same API, the recommendation was to keep the current behavior but document it better.
Closes #2501 #2542 #2543
Steps to Test
Pull Request Checklist
Testing
Code Style and Documentation
📝 Please see our Contribution Guidelines for further guidance.