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

Sending to Offer without signing_pubkey #3017

Merged
merged 4 commits into from Apr 29, 2024

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Apr 24, 2024

If an Offer contains a path, the blinded_node_id of the path's final hop can be used when signing an invoice. This helps to further reduce offer QR code size by making offer_node_id optional when offer_paths is set. Allow parsing (and thus sending to) such offers. Also replaces Refund's use of offer_paths with a new invreq_paths TLV in order to differentiate a Bolt12Invoice for an Offer from one for a Refund.

Receiving to an Offer without a signing_pubkey is not supported yet.

@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 92.42424% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 89.49%. Comparing base (2c0fcf2) to head (b7635c4).
Report is 29 commits behind head on main.

Files Patch % Lines
lightning/src/offers/invoice.rs 95.34% 2 Missing and 2 partials ⚠️
lightning/src/offers/invoice_request.rs 87.50% 4 Missing ⚠️
lightning/src/offers/offer.rs 90.47% 3 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs 81.81% 2 Missing ⚠️
lightning/src/offers/refund.rs 95.23% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3017      +/-   ##
==========================================
+ Coverage   89.13%   89.49%   +0.35%     
==========================================
  Files         118      118              
  Lines       97492   100077    +2585     
  Branches    97492   100077    +2585     
==========================================
+ Hits        86903    89564    +2661     
+ Misses       8349     8261      -88     
- Partials     2240     2252      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jkczyz jkczyz marked this pull request as ready for review April 24, 2024 21:11
@jkczyz jkczyz requested a review from TheBlueMatt April 25, 2024 14:08
@TheBlueMatt TheBlueMatt added this to the 0.0.123 milestone Apr 25, 2024
@TheBlueMatt
Copy link
Collaborator

Feel free to squash imo

If an Offer contains a path, the blinded_node_id of the path's final hop
can be used as the signing pubkey. Make Offer::signing_pubkey and
OfferContents::signing_pubkey return an Option to support this. Upcoming
commits will implement this behavior.
If an offer has at least one path, it may omit the signing pubkey and
use the blinded node id of the last hop of a path to sign an invoice.
Allow parsing such offers but not yet creating them.
Instead of reusing OfferTlvStream::paths, add a dedicated paths TLV to
InvoiceRequestTlvStream such that it can be used in Refund. This allows
for an Offer without a signing_pubkey and still be able to differentiate
whether an invoice is for an offer or a refund.
When parsing a Bolt12Invoice use both the Offer's signing_pubkey and
paths to determine if it is for an Offer or a Refund. Previously, an
Offer was required to have a signing_pubkey. But now that it is
optional, the Offers paths can be used to make the determination.
Additionally, check that the invoice matches one of the blinded node ids
from the paths' last hops.
Comment on lines +1450 to +1452
.iter()
.filter_map(|path| path.blinded_hops.last())
.any(|last_hop| fields.signing_pubkey == last_hop.blinded_node_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the spec, it looks like we MUST verify that the signing pubkey matches a path we sent an invoice request to. I'm not certain it matters or if the spec should change but it looks like we don't specifically check that at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is a slight deviation as we don't have the blinded node id for the path that the invoice was sent over when parsing. We could do some sort of check at the handler level, but even that would require piping that data through.

);
pending_offers_messages.push(message);
} else {
debug_assert!(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Its usually worth having a comment on these kinds of assertions as to why we think its not reachable.

@TheBlueMatt TheBlueMatt merged commit 2b14cc4 into lightningdevkit:main Apr 29, 2024
16 checks passed
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request May 6, 2024
…pubkey

Sending to `Offer` without `signing_pubkey`
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.

None yet

4 participants