btcec/v2: create new schnorr package for BIP-340, move existing ecdsa implementation into new ecdsa package

[Roasbeef]

In this PR, as a follow up to #1773, we split the btcec package into ecdsa and schnorr packages, and implement BIP-340.

NOTE: This PR builds on top of #1773, only the last 3 commits are unique.

New ecdsa sub-package

In this PR, we create a new package to house the ECDSA-specific
logic in the new btcec/v2 package. This change is meant to mirror the
structure of the dcrec package, as we'll soon slot in our own custom
BIP-340 implementation.

Preliminary BIP-340 tagged hash routine

In this PR, we also add an implementation of the BIP-340 tagged hash
scheme. This initial version can be optimized quite a bit, for example,
we can hard code the output of frequently used sha256(tag) values and
save two sha256 invocations.

Test Vector Compliant* BIP-340 Implementation

In this PR, we add an initial implementation of BIP-340. Mirroring
the recently added ecsda package, we create a new schnorr package
with a unique Signature type and ParsePubkey function (heavily based
on the related dcrd package). The new Signature type implements the
fixed-sized 64-byte signatures, and theParsePubkey method only accepts
pubkeys that are 32-bytes in length, with an implicit sign byte.

The signing implementation by default, deviates from BIP-340 as it opts
to use rfc6979 deterministic signatures by default, which means callers
don't need to always pass in their own auxNonce randomness. A set of
functional arguments allows callers to pass in their own value, which is
the way all the included test vectors function.

The other optional functional argument added is the FastSign option
that allows callers to skip the final step of verifying each signature
they generate.

TODOs

• *Fix the failing 3rd test vector with comment: "test fails if msg is reduced modulo p or n". Oddly this sig verifies just fine, but if we generate then attempt to verify it fails. Might be some underlying mutation going on?
• Add additional tests for the functional arguments, and also a complete set of benchmarks.

Fixes #1212

Shouts out to @philipglazman for his initial PR taking a stab at this, the initial test structure was copied over to this PR.

[coveralls]

Pull Request Test Coverage Report for Build 1775334463

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 79.676%

---

Totals
Change from base Build 1753768655:	0.0%
Covered Lines:	1231
Relevant Lines:	1545

---

💛 - Coveralls

[Roasbeef]

Pushed up a fixup commit to address an issue causing that test to fail, all test vectors now pass!

Here's the section in the BIP describing the issue that exists without that fixup commit:

Note that the correctness of verification relies on the fact that lift_x always returns a point with an even Y coordinate. A hypothetical verification algorithm that treats points as public keys, and takes the point P directly as input would fail any time a point with odd Y is used. While it is possible to correct for this by negating points with odd Y coordinate before further processing, this would result in a scheme where every (message, signature) pair is valid for two public keys (a type of malleability that exists for ECDSA as well, but we don't wish to retain). We avoid these problems by treating just the X coordinate as public key.

I noticed that the CI as is, actually isn't running any of the tests in the v2 directory, likely some modules quirk we'll need to look into...

[Roasbeef]

Here's the section in the BIP describing the issue that exists without that fixup commit:

Leaning towards modifying things s.t. it always takes the set of serialized bytes and decompresses the point each time. This would put the implementation more in line with that section of the BIP.

The schnorrVerify function as is will accept a valid signature based on the two mirrored public keys (even y and odd y), but in practice we'll always parse the public keys with the new schnorr.ParsePubKey method which'll always return points with an even y coord. The prior tests failed as the public key generated by the private key directly was returned, instead of passing in the serialized variant.

One argument for keeping it struct-level as is (passing in *btcec.PublicKey), is that we can save some extra cycles but not always needing to re-parse the public key (which would only be the case if we didn't see a sig in the mempool?).

[naveensrinivasan]

It would be a good target for fuzzing. Thoughts?

[guggero]

Very nice! Did a first pass and like how small and readable this turned out, great work!
It would be nice to just have a pubKey.SerializeCompact() method and being able to parse 32-byte public keys in btcec.ParsePubKey(). But I guess that's the downside of using the implementation of another repo, we can't easily extend existing types.

BTW, I went ahead and implemented the small tagged hash precompute optimization that was mentioned in a TODO:
tagged-hash.patch.txt

[gcsfred2]

@Roasbeef Regarding this fragment in signature.go:

// Before we proceed, we want to ensure that the public key we're using
	// for verification always has an even y-coordinate. So we'll serialize
	// it, then parse it again to esure (sic) we only proceed with points that
	// have an even y-coordinate.
	pubKey, err := ParsePubKey(pub.SerializeCompressed()[1:])

The problem is that even though the given public key in variable pub has even Y coordinate, pubKey will have an odd Y if its X coordinate allows for odd and even Y. Then verification fails further on when comparing the given R with the calculated R, even though the public key X is valid. My interpretation of BIP340 is that such public keys are still valid ("Implicitly choosing the Y coordinate that is even"). The Sign function currently negates the private key if P.y is odd, but in threshold signing this operation is not trivial. Suggestion:

pubKey, err := ParsePubKey(pub.SerializeCompressed()[1:])
  if err != nil {
    return err
  }
  **pubKey = pub**

[Roasbeef]

Commenting so I don't lose track of this commit: 70ae843

[Roasbeef]

Rebased on top of #1773

[Roasbeef]

@gcsfred2 have you this this comment, and this comment?

[Roasbeef]

@gcsfred2 the alternative to that commit (which is now melded) and linked above in my prior comment is that we change the API to accept a []byte, and then use ParsePubkey at the very start. This matches the inention of the BIP authors, but would force most users of this API to serialize before passing the key in...

[wpaulino]

Looks good! Mostly just have minor comments on my end.

[wpaulino]

I couldn't find any documentation for this value, I assume it is up to the implementation to pick it?

[Roasbeef]

Check out this section of the BIP: https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#alternative-signing

This can be most reliably accomplished by not reusing the same private key across different signing schemes. For example, if the rand value was computed as per RFC6979 and the same secret key is used in deterministic ECDSA with RFC6979, the signatures can leak the secret key through nonce reuse.

To avoid that leak, we bind the randomness value derived from RFC6979 to the new signing context. As a result, given the same message, we'll generate a distinct nonce for ECDSA vs Schnorr, thereby alleviating any concerns here re leaking a secret key.

Note that this signing path is optional with how things are laid out atm. Using RFC 6979 here lets us have signing be deterministic as it is for ECDSA today, and doesn't require the caller to pass in extra randomness themselves authNonce.

[wpaulino]

My comment was referring to whether implementations are free to choose the extra data value being used here (sha256([]byte("BIP-340"))) as I don't see it referenced in the BIP.

[Roasbeef]

Ah I see, yeah that's a value we've chosen here, inspired by the other prefixes used for the tagged hash implementation across taproot. In theory, we could eventually make another BIP that just specifies the usage of this phrase for those wallets that want to default to deterministic signatures.

IMO unless you're doing some fancy zkp to have a hardware wallet prove proper nonce generation, then you just want to go with deterministic signatures, as then you rely less on your platform's RNG outside of initial seed generation 9which may have taken place elsewhere).

[wpaulino]

Nit: maybe a bit of premature optimization, we can check if pub has an even y-coordinate and avoid this.

[Roasbeef]

Indeed, but those the x and y are private variables as is, so we need to modify the upstream package, or convert into a jacobian point, then back to affine again to check. Also see this comment (and the one below it) for context: #1777 (comment)

[gcsfred2]

"but in practice we'll always parse the public keys with the new schnorr.ParsePubKey method which'll always return points with an even y coord" The problem with parsing the pub key and using the changed variable with a different y is that the calculated r further down may be different than the given r, failing the verification. I found this problem providing an odd pk.y to this commit.
Link to BTC's code: https://github.com/bitcoin/bitcoin/blob/3d223712d343d49ab2fc6d0fbf66b39663835192/src/secp256k1/src/modules/schnorrsig/main_impl.h#L215

[Roasbeef]

The problem with parsing the pub key and using the changed variable with a different y is that the calculated r further down may be different than the given r, failing the verification. I found this problem providing an odd pk.y to this commit.

Yes, before I added that fragment, we failed a test vector for this exact same reason (the challenge hash differed) as warned in this fragment of the BIP:

Note that the correctness of verification relies on the fact that lift_x always returns a point with an even Y coordinate. A hypothetical verification algorithm that treats points as public keys, and takes the point P directly as input would fail any time a point with odd Y is used. While it is possible to correct for this by negating points with odd Y coordinate before further processing, this would result in a scheme where every (message, signature) pair is valid for two public keys (a type of malleability that exists for ECDSA as well, but we don't wish to retain). We avoid these problems by treating just the X coordinate as public key.

Our verification function "treats the points as public keys" since it accepts a *btcec.PublicKey, rather than a [32]byte. If we accepted a byte slice, and made the caller do the serialization (which we do here) then we'd mirror the comment there more precisely. When a caller signs, they negate their private key if the actual public key happens to have an odd y coordinate.

If you comment out this line, the test vectors fail as the challenge hashes diverge.

Does this follow for you @gcsfred2? I want to make sure I'm not missing anything glaring here that can cause a divergence. If we want to do things more by the book, then we can just accept a byte slice, which has the effect of moving this fragment to the line before all callers attempt to verify a signature.

[Roasbeef]

Here's the test vector in question: https://github.com/bitcoin/bips/blob/master/bip-0340/test-vectors.py#L76 (same index in our code)

[Roasbeef]

Pushed up a rebased version now that the v2 module for btcec has landed in mater. Next push will address the latest set of comments.

[Roasbeef]

Aight I think this is g2g now, PTAL.

[wpaulino]

My comment was referring to whether implementations are free to choose the extra data value being used here (sha256([]byte("BIP-340"))) as I don't see it referenced in the BIP.

[Roasbeef]

Latest comments addressed!

Also added benchmarks in teh final commit:

⛰   go test -run=XXX -bench=.
goos: darwin
goarch: amd64
pkg: github.com/btcsuite/btcd/btcec/v2/schnorr
cpu: VirtualApple @ 2.50GHz
BenchmarkSigVerify-8     	    8000	    152468 ns/op	     960 B/op	      16 allocs/op
BenchmarkSign-8          	    4939	    215489 ns/op	    1408 B/op	      27 allocs/op
BenchmarkSignRfc6979-8   	    5106	    217416 ns/op	    2129 B/op	      37 allocs/op
PASS
ok  	github.com/btcsuite/btcd/btcec/v2/schnorr	4.629s

  bip340 ✘ ✭  roasbeef  ...btcd/btcec/schnorr 
⛰   go test -run=XXX -bench=.
goos: darwin
goarch: amd64
pkg: github.com/btcsuite/btcd/btcec/v2/schnorr
cpu: VirtualApple @ 2.50GHz
BenchmarkSigVerify-8     	    7982	    142826 ns/op	     960 B/op	      16 allocs/op
BenchmarkSign-8          	   18210	     65908 ns/op	     496 B/op	      12 allocs/op
BenchmarkSignRfc6979-8   	   16537	     78161 ns/op	    1216 B/op	      22 allocs/op
PASS
ok  	github.com/btcsuite/btcd/btcec/v2/schnorr	5.418s

The top is w/o fast sign (always verify the sig after you make it like the BIP prescribes), bottom is w/o (just sign). Also added a test case to gauge the impact of RFC 6979 as that ends up adding some extra hash invocations.

[Roasbeef]

BTW, I went ahead and implemented the small tagged hash precompute optimization that was mentioned in a TODO:
tagged-hash.patch.txt

Added this as well, thanks for the patch @guggero!

[guggero]

Nice, LGTM 🎉