rustyrussell / secp256k1-py

Python FFI bindings for libsecp256k1 (maintained)
MIT License
56 stars 12 forks source link

Signature verification fails on some (correct) signatures #17

Open cornwarecjp opened 7 months ago

cornwarecjp commented 7 months ago

I tried to use secp256k1-py to verify a couple of (simple P2PKH) signatures found on the Bitcoin blockchain, and some succeeded and some failed. I think it's safe to assume that all these signatures are correct, otherwise they wouldn't be accepted in the blockchain, so it's weird that some failed. That leaves some possibilities:

I found two transactions, where one succeeds and the other fails, that are nearly identical in format: 1 P2PKH input, 2 P2PKH outputs, same version number, no lock time & so on; they even have the same key used for their input. Manually looking at the sighash generation of the failing tx, it seems correct. So I wanted to see whether secp256k1-py contains a bug.

For comparison, I had the same signatures checked by another library, ecdsa (pip install ecdsa). It accepts both signatures, confirming that Bitcoin is OK and that my own code is OK. Here is the code for comparing both:

from ecdsa.util import sigdecode_der

import secp256k1

#Dummy hash class to stop ecdsa lib from applying a hash function:
class H:
    def __init__(self, x):
        self.x = x
    def digest(self):
        return self.x

def test(caption, public_key, sig, message):
    print(caption)

    pk = secp256k1.PublicKey(public_key, raw=True)
    print('  Result from secp256k1 library: ', pk.ecdsa_verify(message, pk.ecdsa_deserialize(sig), raw=True))

    vk = ecdsa.VerifyingKey.from_string(public_key, curve=ecdsa.SECP256k1, hashfunc=H)
    print('  Result from ecdsa library: ', vk.verify(sig, message, sigdecode=sigdecode_der))

#From tx 8f04cac419d838551106eb496f9871f3fb6d37aff3e3b25d412ab1b066018564:
test(
    'First test',
    public_key = bytes.fromhex('047f57e6e8ea7e8ddedceebc2ce66b5052fe84f84f2e97db39f7c62d50de046542b5eec51e63c34a4dbdf3ff0dc8a7df838368d185789f594c91aff4259906ad8a'),
    sig = bytes.fromhex('30440220160573869f08163c66da05af95f85331c6cad428b0033e6387d722418f924add02202d2578b265b087d3ad4b3bc9ff1bda236232e72c9cf8f03d4309cf8a85466bbd'),
    message = bytes.fromhex('0b65b86140290842899ddcd558dfa11b87f88e90c411ee351266a89788341d22'),
    )
#From tx 9c6ad0648e29ae18f8a866282784d2fc48b29c28b48522f564e9b381f1084de8:
test(
    'Second test',
    public_key = bytes.fromhex('047f57e6e8ea7e8ddedceebc2ce66b5052fe84f84f2e97db39f7c62d50de046542b5eec51e63c34a4dbdf3ff0dc8a7df838368d185789f594c91aff4259906ad8a'),
    sig = bytes.fromhex('3045022006aa95fc6529fe54ea8ee9e3b4ea7a2b85a080caec2d2b8d534d283e09d6ba43022100db2605b3d9b1732697926dec7b6a1e037fe5af7ac04cc01c7a83efdb37a6f476'),
    message = bytes.fromhex('751eed770ba7c2222f09923dc022b32c1bdd65e9d74f3eb024ea89b5545c2cbd'),
    )

I get the following output:

First test
  Result from secp256k1 library:  True
  Result from ecdsa library:  True
Second test
  Result from secp256k1 library:  False
  Result from ecdsa library:  True
cornwarecjp commented 7 months ago

I suspect this may be because these are signatures from very old Bitcoin transactions. IIRC, Bitcoin has soft-forked at some point, where it started requiring signatures to be in some canonical format. Maybe the failing signature was from before that soft-fork.

Decoding the DER format, a difference I noticed was that the second signature has an s-value that starts with 0x00: 00db2605b3d9b1732697926dec7b6a1e037fe5af7ac04cc01c7a83efdb37a6f476

If secp256k1 (or its Python wrapper) rejects non-canonical signatures, that leaves the question whether that's desirable. It's certainly necessary for checking signatures on new Bitcoin transactions, but for validating old (pre-softfork) transactions it isn't what you want. For non-Bitcoin applications, it might depend on your application (e.g. do you want compatibility with other libraries that might emit non-canonical signatures).

So maybe it should be an option that can be passed by the user (and this issue can be closed if that option already exists and I overlooked it).