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

Schnorr signature verification deviates from BIP 340 #2017

Closed
wydengyre opened this issue Aug 12, 2023 · 2 comments · Fixed by #2018
Closed

Schnorr signature verification deviates from BIP 340 #2017

wydengyre opened this issue Aug 12, 2023 · 2 comments · Fixed by #2018

Comments

@wydengyre
Copy link
Contributor

wydengyre commented Aug 12, 2023

The relevant block of code begins here:

if overflow := e.SetBytes((*[32]byte)(commitment)); overflow != 0 {

The complete code:

        if overflow := e.SetBytes((*[32]byte)(commitment)); overflow != 0 {
		str := "hash of (r || P || m) too big"
		return signatureError(ecdsa_schnorr.ErrSchnorrHashValue, str)
	}

This will fail to verify a signature whose challenge hash is interpreted as a scalar above the secp256k1 curve order.

The verification algorithm specified in BIP340 allows for such signatures. See https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#verification

The relevant math is Let e = int(hashBIP0340/challenge(bytes(r) || bytes(P) || m)) mod n. It doesn't fail on overflow.

Here is the challenge calculation used in Bitcoin Core, which also does modular arithmetic, rather than failing on overflow:
https://github.com/bitcoin/bitcoin/blob/3654d84c6f53e5137f9208851ff904c248b4741f/src/secp256k1/src/modules/schnorrsig/main_impl.h#L116

It seems that in the (perhaps extremely unlikely) event of getting a block containing a signature whose challenge hash is above the curve order, this might cause a fork?

@wydengyre
Copy link
Contributor Author

A quick note: I haven't included a test vector here because generating such a signature would be computationally prohibitive.

@Roasbeef
Copy link
Member

A quick note: I haven't included a test vector here because generating such a signature would be computationally prohibitive.

Yep, it would need more hash invocations than what was required to arrive at the current main chain (which passed 80 bits in the past 2 years or so).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants