Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial Schnorr signature support #1667

Closed
wants to merge 4 commits into from

Conversation

philipglazman
Copy link

@philipglazman philipglazman commented Nov 19, 2020

Adding a Schnorr BIP340 spec implementation to btcec. Hopefully this can be used as a building block as we get closer to Taproot activation as well as using it in signet/regtest.

Includes:

  • Default signing
  • Verification
  • Public key conversion from compressed public keys or 32 byte schnorr public keys
  • A new SchnorrPublicKey type that wraps the x(P) integer.
  • SchnorrPubKey() method for PrivateKey type

Does not include batch verification.

Used the BIP as of 3b1fb96 as reference.

Included test vectors from BIP340.

Haven't tested much on hardware yet.

Addresses #1212

@coveralls
Copy link

coveralls commented Nov 19, 2020

Pull Request Test Coverage Report for Build 526860955

  • 217 of 237 (91.56%) changed or added relevant lines in 2 files are covered.
  • 42 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+2.0%) to 53.622%

Changes Missing Coverage Covered Lines Changed/Added Lines %
btcec/schnorr.go 214 234 91.45%
Files with Coverage Reduction New Missed Lines %
rpcserverhelp.go 8 81.63%
peer/peer.go 11 75.55%
btcjson/chainsvrresults.go 23 57.67%
Totals Coverage Status
Change from base Build 407350020: 2.0%
Covered Lines: 21050
Relevant Lines: 39256

💛 - Coveralls

@jakesylvestre
Copy link
Collaborator

As per some of the concerns raised in IRC I've asked @ashwin111 to take a look at wether or not this is a constant-time implementation

@adiabat
Copy link
Contributor

adiabat commented Dec 11, 2020

Hi- I tested out this code and I'm not sure if it's the signing side or the verification side, but I get "cannot create signature" failures (when it fails verification of the signature it just produced).

Here's some code you could add to the schnorr_test.go file to cause this failure:

func TestRandomSignatures(t *testing.T) {
	for i := 0; i < 1000; i++ {
		var secretKeyBytes, messageBytes [32]byte
		rand.Read(secretKeyBytes[:])
		rand.Read(messageBytes[:])
		schnorrSecretKey, _ := PrivKeyFromBytes(S256(), secretKeyBytes[:])
		sig, err := schnorrSecretKey.SchnorrSign(messageBytes[:], nil)
		if err != nil {
			t.Fatalf("Sign message %x with key %x signature %x fails: %s\n",
				messageBytes, secretKeyBytes, sig, err.Error())
		}
	}
}

I don't think there's anything wrong with using random messages / private keys as those should be valid. If there are some restrictions they should be made clear and a different error message returned. (But I don't think that's the case here)

This failure seems to occur with the same probability (around 0.78%) whether the key is varied, the message is, or both are. This suggests it fails 1/128th of the time, so it may be a padding problem. I did not notice any pattern in the key or message that caused failure (there were some starting with 0 and some starting with f) but I didn't look that closely.

Please let me know if you're not able to reproduce this failure; I can show specific key / message pairs if that helps.

@adiabat
Copy link
Contributor

adiabat commented Dec 11, 2020

Looked a bit more, and schnorrVerify returns false from Fail if x(R) != r about 3/4ths the time, and Fail if y(R) is not even 1/4th the time.

So out of 512 random signatures, 1 will fail due to y(R) not even, and 3 fail due to x(R) !=r.

It might not actually map to powers of 2 this way, but it seems pretty close after running it for a while and trying millions of signatures.

@philipglazman
Copy link
Author

Thanks @adiabat. I was able to reproduce your issue.

It's was padding problem! Coordinates must be 32 bytes.

Added commit to fix the padding issue and return better error messages from schnorrVerify.

btcec/schnorr.go Outdated

// Fail if R is at infinity.
if Rx.Cmp(zero) == 0 || Ry.Cmp(zero) == 0 {
return false, errors.New("point R is at infinity")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of constructing new errors on every failure (you would recognize them only by the string), why not give them top-level names and make them part of the interface? then the tests can assert that the correct error is returned.

@gcsfred2
Copy link

Please consider prioritizing this PR.

@gcsfred2
Copy link

gcsfred2 commented Jun 15, 2021

@philipglazman Would you consider changing x to X in type SchnorrPublicKey or adding a constructor to SchnorrPublicKey ? I need to hold and represent public keys but not always I can initialize them from private keys. X may be necessary for serialization/deserialization (which is also my case, with protobuf).

@arshbot
Copy link
Contributor

arshbot commented Mar 21, 2022

@Roasbeef has this PR been superseded by #1787?

@Roasbeef
Copy link
Member

Yep! This functionality has landed in master.

@Roasbeef Roasbeef closed this Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants