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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Jan 31 2019, 13:39
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 31 2019, 13:39
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox edited the summary of this revision. (Show Details)Feb 1 2019, 00:44
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.

deadalnix added inline comments.Feb 3 2019, 03:15
src/secp256k1/src/modules/schnorr/schnorr_impl.h
197 ↗(On Diff #7058)

Yes.

markblundeberg added inline comments.Feb 3 2019, 04:04
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.

deadalnix added inline comments.Feb 3 2019, 17:44
src/secp256k1/src/modules/schnorr/schnorr_impl.h
197 ↗(On Diff #7058)

The function is secp256k1_schnorr_compute_sig, not secp256k1_schnorr_compute_R

markblundeberg added inline comments.Feb 4 2019, 14:53
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 requested review of this revision.Feb 13 2019, 18:46
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.

Fabien added inline comments.Feb 13 2019, 18:56
src/secp256k1/src/modules/schnorr/schnorr_impl.h
170 ↗(On Diff #7058)

OK, so it can be cleared in secp256k1_schnorr_sig_sign

markblundeberg added inline comments.Feb 17 2019, 01:16
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)?

markblundeberg requested changes to this revision.Apr 13 2019, 14:47

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
deadalnix added inline comments.Apr 17 2019, 15:30
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.

deadalnix updated this revision to Diff 8095.Apr 17 2019, 15:35

dd a check for the point at infinity in secp256k1_schnorr_compute_sig

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