btcsuite / btcd

An alternative full node bitcoin implementation written in Go (golang)
https://github.com/btcsuite/btcd/blob/master/README.md
ISC License
6.1k stars 2.31k forks source link

Schnorr signature verification deviates from BIP 340 #2017

Closed wydengyre closed 10 months ago

wydengyre commented 10 months ago

The relevant block of code begins here: https://github.com/btcsuite/btcd/blob/0aaa7c5e7b7f075f62c90e7fe0df6e28c0427c7d/btcec/schnorr/signature.go#L180

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 commented 10 months ago

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

Roasbeef commented 10 months ago

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).