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 - Commits
- rABC2564826f8528: [schnorr] Refactor the signature process in reusable component
Adapted test cases to reflect the changes.
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
| src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
|---|---|---|
| 154 ↗ | (On Diff #7058) | 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 ↗ | (On Diff #7058) | 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 ↗ | (On Diff #7058) | jacobi symbol i mean |
| 197 ↗ | (On Diff #7058) | 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 ↗ | (On Diff #7058) | Yes. |
| src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
|---|---|---|
| 197 ↗ | (On Diff #7058) | 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 ↗ | (On Diff #7058) | The function is secp256k1_schnorr_compute_sig, not secp256k1_schnorr_compute_R |
| src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
|---|---|---|
| 188 ↗ | (On Diff #7058) | 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 ↗ | (On Diff #7058) | 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 ↗ | (On Diff #7058) | See @markblundeberg 's comment. |
| src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
|---|---|---|
| 170 ↗ | (On Diff #7058) | OK, so it can be cleared in secp256k1_schnorr_sig_sign |
| src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
|---|---|---|
| 145 ↗ | (On Diff #7058) | Same comment as below -- why sanity check the private key but not also sanity-check the pubkey point argument (not point at infinity)? |
Comment Actions
getting this rolling again ...
| src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
|---|---|---|
| 188 ↗ | (On Diff #7058) | Since this refactor is definitely intended to be used in cases where pubkey unrelated to privkey it's logical to check that pubkey is not point at infinity as that would cause a crash in secp256k1_schnorr_compute_e. |
| src/secp256k1/src/modules/schnorr/schnorr_impl.h | ||
|---|---|---|
| 145 ↗ | (On Diff #7058) | That function never use it, so there is really no point doing it here. |