Page MenuHomePhabricator

[schnorr] Refactor the signature process in reusable component
ClosedPublic

Authored by deadalnix on Jan 31 2019, 13:39.

Details

Summary

This will make it easier to implement more evolved schnorr constructs such as blinded signatures and multisig.

Test Plan

Adapted test cases to reflect the changes.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
schnorrsigrefac
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5480
Build 9022: Bitcoin ABC Buildbot (legacy)
Build 9021: arc lint + arc unit

Event Timeline

jasonbcox added inline comments.
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?

markblundeberg added inline comments.
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.

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.

This revision is now accepted and ready to land.Feb 4 2019, 14:57
Fabien requested changes to this revision.Feb 11 2019, 09:49
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/secp256k1/src/modules/schnorr/schnorr_impl.h
170 ↗(On Diff #7058)

Clear k here, to avoid relying on secp256k1_schnorr_sig_generate_k behavior

192 ↗(On Diff #7058)

Moving this part to secp256k1_schnorr_compute_k_R() would make the flow easier to follow.

This revision now requires changes to proceed.Feb 11 2019, 09:49
deadalnix added inline comments.
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)?

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.

This revision now requires changes to proceed.Apr 13 2019, 14:47
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.

dd a check for the point at infinity in secp256k1_schnorr_compute_sig

This revision is now accepted and ready to land.Apr 20 2019, 13:30
This revision was automatically updated to reflect the committed changes.