Page MenuHomePhabricator

[secp256k1] Implement Schnorr signatures
Needs ReviewPublic

Authored by deadalnix on Sun, Dec 2, 19:48.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Summary

This implement Schnorr signatures on secp256k1 as per https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr.mediawiki

This implement a variation of Schnorr similar to edDSA by Daniel J. Bernstein.

Test Plan

Added test cases for the signature scheme.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
schnorr
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4312
Build 6689: Bitcoin ABC Teamcity Staging
Build 6688: arc lint + arc unit

Event Timeline

deadalnix created this revision.Sun, Dec 2, 19:48
Herald added a reviewer: Restricted Project. · View Herald TranscriptSun, Dec 2, 19:48
Herald added a subscriber: schancel. · View Herald Transcript
deadalnix updated this revision to Diff 6281.Tue, Dec 4, 21:00

Explain test cases more in details.

deadalnix updated this revision to Diff 6282.Tue, Dec 4, 21:17

Various nits

Fabien requested changes to this revision.Thu, Dec 6, 16:46
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/secp256k1/src/modules/schnorr/schnorr_impl.h
30 ↗(On Diff #6282)

Could you introduce r = R.x somewhere in the comment before this line ?

46 ↗(On Diff #6282)

May be removed since this is not implemented yet

69 ↗(On Diff #6282)

Shouldn't Rj be cleared after this ?

This revision now requires changes to proceed.Thu, Dec 6, 16:46
deadalnix marked 2 inline comments as done.Fri, Dec 7, 19:54
deadalnix added inline comments.
src/secp256k1/src/modules/schnorr/schnorr_impl.h
46 ↗(On Diff #6282)

No, this is an explanation of how the signature check works and this is part of it.

69 ↗(On Diff #6282)

Try it.

deadalnix updated this revision to Diff 6295.Fri, Dec 7, 19:57

Use R.x in part of the explaination where r is not defined

deadalnix updated this revision to Diff 6301.Fri, Dec 7, 21:46

Refactor the signing code.

  • Extract a function to generate a proper value for k
  • Use that fucntion in the signing code.

It makes the overall code flow much better.

Fabien requested changes to this revision.Fri, Dec 7, 22:26
Fabien added inline comments.
src/secp256k1/src/modules/schnorr/schnorr_impl.h
159 ↗(On Diff #6301)

static is no longer needed

This revision now requires changes to proceed.Fri, Dec 7, 22:26

Just wondering, there are two implementations here, e.g., "secp256k1_schnorr_verify" and "secp256k1_schnorr_sig_verify". I understand one is from Amaury and one is from Pieter. What is the goal here, keep both or keep just one (and which?)?

deadalnix updated this revision to Diff 6302.Sat, Dec 8, 00:10

remove useless static

deadalnix updated this revision to Diff 6303.Sat, Dec 8, 01:49

reorder code so verify come before sign. This allows to change the signature code (for instance for multisig) without causing the verification code lost in the middle of the signature code.

@markblundeberg Both are necessary. one is a public API, one is a private one.

deadalnix updated this revision to Diff 6305.Sat, Dec 8, 15:45

Fix mixture of tab and spaces

Fabien requested changes to this revision.Tue, Dec 11, 20:16
Fabien added inline comments.
src/secp256k1/src/modules/schnorr/main_impl.h
49 ↗(On Diff #6305)

You should check seckey for overflow, this will avoid e*x to fail when signing

src/secp256k1/src/modules/schnorr/tests_impl.h
214 ↗(On Diff #6305)

Test vector 5

445 ↗(On Diff #6305)

r = field size

478 ↗(On Diff #6305)

s = n

This revision now requires changes to proceed.Tue, Dec 11, 20:16
deadalnix marked 2 inline comments as done.Fri, Dec 14, 08:29
deadalnix added inline comments.
src/secp256k1/src/modules/schnorr/main_impl.h
49 ↗(On Diff #6305)

If you pass an invalid secret key, then the signing process will fail, yes. This is not a bug.

src/secp256k1/src/modules/schnorr/tests_impl.h
214 ↗(On Diff #6305)

Test vector 5 is about an invalid pubkey, so not very relevent for us.

deadalnix updated this revision to Diff 6341.Fri, Dec 14, 14:35

Fix comments on test cases