This will make it easier to implement more evolved schnorr constructs such as blinded signatures and multisig.
Details
- Reviewers
markblundeberg Fabien - Group Reviewers
Restricted Project
Adapted test cases to reflect the changes.
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- schnorrsigrefac
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 4786 Build 7635: Bitcoin ABC Teamcity Staging Build 7634: arc lint + arc unit
Event Timeline
src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
---|---|---|
154 | Does R need to be cleared here as well, since it's derived from k? If not, why? |
src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
---|---|---|
154 | R is public so it should be fine. (technically, the jacobi index of Y is one additional bit of private information that is normally erased before publication, however this bit does not have a connection that can lead to compromise of private key since it is uncorrelated with the rest of the nonce) | |
154 | jacobi symbol i mean | |
197 | Interesting that this sign flip trick still works with MuSig. If signers produce an aggregate R that needs to have its sign flipped, then all signers can individually notice this, flip their k's, and thereby arrive at the proper result. |
src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
---|---|---|
197 | Yes. |
src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
---|---|---|
197 | There was something bothering me about the way the k argument gets mutated here and I realized what it was... basically, if you were to call this function a second time with same variables, it may produce the wrong signature. In principle maybe you ought to flip the sign of R as well so that the caller learns the new corrected R. But meh, as long as it's clear that k is expected to be cleared immediately after and no second call, it doesn't matter. |
src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
---|---|---|
197 | The function is secp256k1_schnorr_compute_sig, not secp256k1_schnorr_compute_R |
src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
---|---|---|
188 | Should this also sanity-check the pubkey point argument (not point at infinity)? Alternatively if sanity checks are not needed, this function can return void. |
Seems fine overall, perhaps premature though, do you have an immediate need for these refactors?
It may also become useful to have Verify factored in a similar way, so that MuSig partial signatures can be verified / unit tested.
src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
---|---|---|
170 | No. This function does not own k nor set k. Making responsabilities unclear is exactly how you end up with codepath that do not clean what they should. | |
192 | See @markblundeberg 's comment. |
src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
---|---|---|
170 | OK, so it can be cleared in secp256k1_schnorr_sig_sign |
src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
---|---|---|
145 | Same comment as below -- why sanity check the private key but not also sanity-check the pubkey point argument (not point at infinity)? |