Enforces low-S canonical signatures when the n field is present in a BOLT11 invoice
#10372
Conversation
Summary of ChangesHello @MPins, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a critical update to LND's BOLT11 invoice processing by enforcing low-S canonical signatures when the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enforces low-S canonical signatures for BOLT11 invoices when the destination public key is provided in the n field, aligning with recent BOLT spec updates. The changes include the core logic in zpay32/decode.go and corresponding test cases. My review focuses on improving the implementation's maintainability by leveraging existing library functions. Overall, the change is correct and well-tested.
| // Ensure the signature is in canonical low-S form. | ||
| sValue := new(big.Int).SetBytes(sigBase256[32:64]) | ||
| halfOrder := new(big.Int).Rsh(btcec.S256().N, 1) | ||
| if sValue.Cmp(halfOrder) > 0 { | ||
| return nil, fmt.Errorf("signature is non-canonical " + | ||
| "(high-S)") | ||
| } |
There was a problem hiding this comment.
For maintainability, it's better to use the existing IsCanonical method from the btcec library to check for low-S signatures instead of re-implementing the logic. This makes the code cleaner and relies on the vetted crypto library implementation.
// Ensure the signature is in canonical low-S form.
ecdsaSig, ok := signature.(*ecdsa.Signature)
if !ok {
// This should not happen for BOLT-11 invoices.
return nil, fmt.Errorf("signature is not an ECDSA signature")
}
if !ecdsaSig.IsCanonical() {
return nil, fmt.Errorf("signature is non-canonical (high-S)")
}There was a problem hiding this comment.
"github.com/btcsuite/btcd/btcec/v2/ecdsa" has no field or method IsCanonical
zpay32/decode.go
Outdated
| // Ensure the signature is in canonical low-S form. | ||
| sValue := new(big.Int).SetBytes(sigBase256[32:64]) | ||
| halfOrder := new(big.Int).Rsh(btcec.S256().N, 1) | ||
| if sValue.Cmp(halfOrder) > 0 { |
There was a problem hiding this comment.
Perhaps let's get a helper function in btcec?
There was a problem hiding this comment.
We can extract it and re-use from here: https://github.com/btcsuite/btcd/blob/9ff0780da683ec1fa88ce9b9f2f4fd39d5592221/txscript/engine.go#L1399-L1412.
Also the bounds on the slice above my not be entirely correct, as the signature encoding itself can be variable size.
There was a problem hiding this comment.
A helper function in btcec makes perfect sense; I’ll open a PR for that as well and use it here.
From my understanding, the slice bounds here in LND are not a problem, since the signature uses compact recoverable encoding (32-byte r, 32-byte s, 1-byte recovery_id). This would only be an issue with DER-encoded signatures. Let me know if I’m misunderstanding anything.
This change enforces low-S canonical signatures when `n` is present and adds the corresponding test vectors.
|
Hello @Roasbeef the helper function was created btcsuite/btcd#2463 |
3 participants
Fixes #10222
This change enforces low-S canonical signatures in BOLT11 invoices when
nis present and adds the corresponding Bolts test vectors (PR#1284 and PR#1298).Depends on btcsuite/btcd#2463