This will make it easier to implement more evolved schnorr constructs such as blinded signatures and multisig.
Details
Details
- Reviewers
markblundeberg Fabien - Group Reviewers
Restricted Project
Adapted test cases to reflect the changes.
Diff Detail
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. | |
Comment Actions
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)? | |