Page MenuHomePhabricator

[schnorr] Refactor the signature process in reusable component
Needs ReviewPublic

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

Details

Reviewers
markblundeberg
Fabien
Group Reviewers
Restricted Project
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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 4786
Build 7635: Bitcoin ABC Teamcity Staging
Build 7634: arc lint + arc unit

Event Timeline

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

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

deadalnix added inline comments.Sun, Feb 3, 03:15
src/secp256k1/src/modules/schnorr/schnorr_impl.h
197

Yes.

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

deadalnix added inline comments.Sun, Feb 3, 17:44
src/secp256k1/src/modules/schnorr/schnorr_impl.h
197

The function is secp256k1_schnorr_compute_sig, not secp256k1_schnorr_compute_R

markblundeberg added inline comments.Mon, Feb 4, 14:53
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.

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

Clear k here, to avoid relying on secp256k1_schnorr_sig_generate_k behavior

192

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

This revision now requires changes to proceed.Mon, Feb 11, 09:49
deadalnix requested review of this revision.Wed, Feb 13, 18:46
deadalnix added inline comments.
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.

Fabien added inline comments.Wed, Feb 13, 18:56
src/secp256k1/src/modules/schnorr/schnorr_impl.h
170

OK, so it can be cleared in secp256k1_schnorr_sig_sign

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