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

feat: add coincurve instead of secp256k1 #46

Merged
merged 7 commits into from
Jul 19, 2024
Merged

feat: add coincurve instead of secp256k1 #46

merged 7 commits into from
Jul 19, 2024

Conversation

dni
Copy link
Member

@dni dni commented Jul 18, 2024

- changed secp256k1 to coincurve
- add testcase for verifying pubkey
lightning/bolts#1184
- added keep_payee flag for encoding
- added tests for signature model
- added test for invalid signature
@dni dni requested review from prusnak and motorina0 July 18, 2024 07:17
@prusnak
Copy link

prusnak commented Jul 18, 2024

Can you also try to remove ecdsa from dependencies and rewrite its usage with coincurve? Should be possible

tests/test_signature.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@motorina0 motorina0 left a comment

Choose a reason for hiding this comment

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

LGTM

see comments

@prusnak
Copy link

prusnak commented Jul 18, 2024

Can you also try to remove ecdsa from dependencies and rewrite its usage with coincurve? Should be possible

Hm, it seems that coincurve does not provide an easy way how to use a signature in hex-string format (not DER).

We could use the following to convert hex-string sig to DER, but it does not help us with getting rid of the dependency :-/

from ecdsa import SECP256k1
from ecdsa.util import sigdecode_string, sigencode_der_canonize

r, s = sigdecode_string(signature_data, SECP256k1.order)
der_sig = sigencode_der_canonize(r, s, SECP256k1.order)

@dni
Copy link
Member Author

dni commented Jul 19, 2024

Can you also try to remove ecdsa from dependencies and rewrite its usage with coincurve? Should be possible

Hm, it seems that coincurve does not provide an easy way how to use a signature in hex-string format (not DER).

We could use the following to convert hex-string sig to DER, but it does not help us with getting rid of the dependency :-/

from ecdsa import SECP256k1
from ecdsa.util import sigdecode_string, sigencode_der_canonize

r, s = sigdecode_string(signature_data, SECP256k1.order)
der_sig = sigencode_der_canonize(r, s, SECP256k1.order)

yep, took me a bit to figure it out.

        sig = deserialize_recoverable(self.signature_data)
        sig = recoverable_convert(sig)
        sig = cdata_to_der(sig)
        if not verify_signature(
            sig, message(self.hrp, self.signing_data), bytes.fromhex(payee)
        ):
            raise ValueError("Invalid signature")

@dni dni merged commit 6dccbd2 into main Jul 19, 2024
10 checks passed
@dni dni deleted the coincurve branch July 19, 2024 07:07
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 this pull request may close these issues.

3 participants