-
Notifications
You must be signed in to change notification settings - Fork 421
refactor(invoice): align signature checks with BOLT11 semantics #4064
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
refactor(invoice): align signature checks with BOLT11 semantics #4064
Conversation
👋 Thanks for assigning @valentinewallace as a reviewer! |
9e89d85
to
c5c9121
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4064 +/- ##
==========================================
- Coverage 88.76% 88.33% -0.43%
==========================================
Files 176 177 +1
Lines 129518 131880 +2362
Branches 129518 131880 +2362
==========================================
+ Hits 114968 116502 +1534
- Misses 11945 12715 +770
- Partials 2605 2663 +58
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c5c9121
to
b91793a
Compare
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
🔔 2nd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
🔔 3rd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
Simplify `check_signature` logic to follow the BOLT11 rules discussed: - If an `n` field is present, verify the signature against the included pubkey using `secp256k1_ecdsa_verify`, which enforces normalized low-S form. - If no `n` field is present, rely on `secp256k1_ecdsa_recover` to extract the pubkey. Recovery accepts both high-S and low-S signatures, matching existing implementations (lnd, c-lightning). This avoids redundant recovery+verify checks while preserving interoperability.
… high-S signature
25347f9
to
c678a9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @erickcestari! Will land after CI passes
Simplify
check_signature
logic to follow the BOLT11 rules discussed:If an
n
field is present, verify the signature against the included pubkey usingsecp256k1_ecdsa_verify
, which enforces normalized low-S form.If no
n
field is present, rely onsecp256k1_ecdsa_recover
to extract the pubkey. Recovery accepts both high-S and low-S signatures, matching existing implementations (lnd, c-lightning).This avoids redundant recovery+verify checks while preserving interoperability.
Context: lightning/bolts#1284