Introduce Reply Paths for BOLT12 Invoice in Offers Flow. by shaavan · Pull Request #3163 · lightningdevkit/rust-lightning

shaavan · 2024-07-09T14:39:39Z

This PR builds on #3087 and addresses this comment.

Changes:

Updates the Offers message flow to include reply_path with the sent BOLT12Invoice.
Ensures that in case of an error, the counterparty can send back any InvoiceError along the reply_path.
Updates the Offers test to check for the invoice's reply_path wherever applicable.

codecov-commenter · 2024-07-09T14:47:24Z

Codecov Report

Attention: Patch coverage is 77.04918% with 14 lines in your changes missing coverage. Please review.

Project coverage is 90.39%. Comparing base (5e62df7) to head (7b49993).
Report is 62 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	66.66%	9 Missing ⚠️
lightning/src/offers/signer.rs	68.75%	5 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3163 +/- ##
==========================================
+ Coverage 89.82% 90.39% +0.57% 
==========================================
Files 126 126 Lines 103024 108017 +4993 Branches 103024 108017 +4993 ==========================================
+ Hits 92543 97644 +5101 - Misses 7769 7815 +46 + Partials 2712 2558 -154

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

TheBlueMatt · 2024-07-16T20:14:38Z

Needs rebase now.

shaavan · 2024-07-17T13:27:51Z

Updated from pr3163.01 to pr3163.02 (diff):
Addressed @jkczyz and @TheBlueMatt comments

Changes:

Rebase on main.
Introduce a fixup commit to introduce context in respond_with_reply_path.

TheBlueMatt · 2024-07-17T14:54:50Z

Please include details about what you're doing and why in the commit messages.

jkczyz · 2024-07-17T14:58:51Z

Please include details about what you're doing and why in the commit messages.

Sorry about being lax when reviewing this. @shaavan I recommend this reference, which we mention in the project contributor guidelines: https://cbea.ms/git-commit/

TheBlueMatt · 2024-07-17T15:01:16Z

I'm usually pretty lax as long as its clear from the code what's going on, but in this case I look at the commit and we're just adding a blinded path, which doesn't tell me anything about why. I have some context from a separate commit in a different PR, but I don't want to rely on that :)

shaavan · 2024-07-18T10:27:22Z

Updated from pr3163.02 to pr3163.03 (diff):
Addressed @TheBlueMatt and @jkczyz comments

Changes:

Squashed the commits
Update the Commit description to be clearly detailed.

Thanks, @TheBlueMatt for the suggestion, and @jkczyz for the resource!

TheBlueMatt · 2024-08-06T15:47:22Z

 match response {
- Ok(invoice) => responder.respond(OffersMessage::Invoice(invoice)),
+ Ok(invoice) => {
+ let context = MessageContext::Offers(OffersContext::Unknown {});


What are we actually gonna do with the InvoiceError now? This is for an inbound payment, presumably we'll just log and move on? Do we want to authenticate the BP at all? @jkczyz

Yeah, I'd figure we'd just log. What is "BP" refer to here?

Oh, blinded path. 😛 We could though maybe a lower log level is fine for now. Not sure how much we can do about sending another invoice without another request.

shaavan · 2024-08-20T12:29:40Z

Updated from pr3163.03 to pr3163.04 (diff):

Changes:

Rebase on main, to resolve merge conflicts.

shaavan · 2024-08-21T09:44:43Z

Updated from pr3163.04 to pr3163.05 (diff):

Changes:

Moved the relevant change to the relevant commit to have a clean commit history.

shaavan · 2024-08-28T08:22:29Z

Updated from pr3163.05 to pr3163.06 (diff):

Changes:

Rebase on main.

shaavan · 2024-08-28T08:30:44Z

Updated from pr3163.06 to pr3163.07 (diff):
Addressed @jkczyz comment

Update the check in a test to maintain channel setup consistency with the other tests.

shaavan · 2024-08-29T11:39:27Z

Updated from pr3163.07 to pr3163.08 (diff):
Addressed @jkczyz comment

Remove an extra redundant node introduced earlier in the test.

When a InvoiceError is received for a sent BOLT12Invoice, the corresponding PaymentHash is to be logged. Introduce hmac construction and verification function for PaymentHash for this purpose.

shaavan · 2024-09-06T12:34:24Z

Updated from pr3163.08 to pr3163.09 (diff):
Addressed @TheBlueMatt comment

Changes:

Introduce HMAC calculation and verification for OffersContext::InboundPayment.

shaavan · 2024-09-09T12:26:15Z

Updated from pr3163.09 to pr3163.10 (diff):
Addressed @jkczyz comments

Changes:

Update docs.
Always use new nonce for the reply path sent with Bolt12Invoice.

- The trait defines the public method one may define for creating and verifying the HMAC. - Using a pub trait to define these method allows the flexibility for other `OffersMessageHandler` construct to construct the HMAC and authenticate the message.

shaavan · 2024-09-10T10:30:51Z

Updated from pr3163.10 to pr3163.11 (diff):
Addressed @jkczyz comment

Changes:

Introduce Verification trait that define the public function required for creating and verifying HMAC.
Define the trait impl for PaymentId, and PaymentHash.

TheBlueMatt

LGTM, feel free to squash.

Introduce HMAC and nonce calculation when sending Invoice with reply path, so that if we receive InvoiceError back for the corresponding Invoice we can verify the payment hash before logging it.

1. Introduced reply_path in BOLT12Invoices to address a gap in error handling. Previously, if a BOLT12Invoice sent in the offers flow generated an Invoice Error, the payer had no way to send this error back to the payee. 2. By adding a reply_path to the Invoice Message, the payer can now communicate any errors back to the payee, ensuring better error handling and communication within the offers flow.

1. Updated the Offers Test to check the reply paths in BOLT12 Invoices. 2. Changed the `extract_invoice` return type from `Option<BlindedMessagePath>` to `BlindedMessagePath` since all BOLT12Invoices now have a corresponding reply path by default.

shaavan · 2024-09-11T13:26:24Z

Updated from pr3163.11 to pr3163.12 (diff):
Addressed @TheBlueMatt comment

Changes:

Squash fixups.

shaavan mentioned this pull request Jul 9, 2024
Allow blinded path diversification by expanding create_blinded_paths#3087
Merged

jkczyz self-requested a review July 9, 2024 14:54

jkczyz reviewed Jul 9, 2024
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/offers_tests.rs Outdated

shaavan mentioned this pull request Jul 16, 2024
Authenticate use of offer blinded paths #3139
Merged

shaavan force-pushed the invoice_reply_path branch from 1a6175e to eb0d3ccCompare July 17, 2024 13:27

shaavan force-pushed the invoice_reply_path branch from eb0d3cc to c2dd2fdCompare July 18, 2024 10:27

TheBlueMatt reviewed Aug 6, 2024
View reviewed changes

shaavan force-pushed the invoice_reply_path branch from c2dd2fd to 890607eCompare August 20, 2024 12:29

shaavan force-pushed the invoice_reply_path branch from 890607e to bd171eeCompare August 21, 2024 09:44

jkczyz mentioned this pull request Aug 22, 2024
Remove message type bound on ResponseInstruction#3263
Merged

shaavan force-pushed the invoice_reply_path branch from bd171ee to 213bafdCompare August 28, 2024 08:22

shaavan force-pushed the invoice_reply_path branch from 213bafd to f84a00eCompare August 28, 2024 08:30

jkczyz reviewed Aug 28, 2024
View reviewed changes

Comment threadlightning/src/ln/offers_tests.rs Outdated

shaavan force-pushed the invoice_reply_path branch from f84a00e to e74642dCompare August 29, 2024 11:39

jkczyz previously approved these changes Aug 30, 2024
View reviewed changes

TheBlueMatt reviewed Sep 5, 2024
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

valentinewallace mentioned this pull request Sep 5, 2024
Support paying static invoices #3140
Merged

HMAC Construction and Verification for PaymentHash
e2ee325
When a InvoiceError is received for a sent BOLT12Invoice, the corresponding PaymentHash is to be logged. Introduce hmac construction and verification function for PaymentHash for this purpose.

shaavan dismissed jkczyz’s stale review via 2d794b5September 6, 2024 12:34

shaavan force-pushed the invoice_reply_path branch from e74642d to 2d794b5Compare September 6, 2024 12:34

jkczyz reviewed Sep 6, 2024
View reviewed changes

Comment threadlightning/src/blinded_path/message.rs Outdated
Comment threadlightning/src/blinded_path/message.rs Outdated
Comment threadlightning/src/blinded_path/message.rs
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated

shaavan force-pushed the invoice_reply_path branch from 2d794b5 to c794bddCompare September 9, 2024 12:26

shaavan force-pushed the invoice_reply_path branch from c794bdd to e8b2409Compare September 10, 2024 10:24

Introduce Verification trait.
6500277
- The trait defines the public method one may define for creating and verifying the HMAC. - Using a pub trait to define these method allows the flexibility for other `OffersMessageHandler` construct to construct the HMAC and authenticate the message.

shaavan force-pushed the invoice_reply_path branch from e8b2409 to e9a3e2cCompare September 10, 2024 10:30

TheBlueMatt reviewed Sep 10, 2024
View reviewed changes

shaavan added 3 commits September 11, 2024 18:54

Add HMAC, and nonce to OffersContext::InboundPayment
8b479ac
Introduce HMAC and nonce calculation when sending Invoice with reply path, so that if we receive InvoiceError back for the corresponding Invoice we can verify the payment hash before logging it.

shaavan force-pushed the invoice_reply_path branch from e9a3e2c to 7b49993Compare September 11, 2024 13:26

jkczyz approved these changes Sep 11, 2024
View reviewed changes

TheBlueMatt approved these changes Sep 11, 2024
View reviewed changes

TheBlueMatt merged commit 4178dd7 into lightningdevkit:mainSep 11, 2024

shaavan deleted the invoice_reply_path branch September 12, 2024 12:56

Uh oh!

Conversation

shaavan commented Jul 9, 2024

Changes:

Uh oh!

codecov-commenter commented Jul 9, 2024• edited by codecov BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jul 16, 2024

Uh oh!

shaavan commented Jul 17, 2024

Uh oh!

TheBlueMatt commented Jul 17, 2024

Uh oh!

jkczyz commented Jul 17, 2024

Uh oh!

TheBlueMatt commented Jul 17, 2024

Uh oh!

shaavan commented Jul 18, 2024

Uh oh!

TheBlueMattAug 6, 2024

Choose a reason for hiding this comment

Uh oh!

jkczyzAug 20, 2024

Choose a reason for hiding this comment

Uh oh!

jkczyzAug 20, 2024

Choose a reason for hiding this comment

Uh oh!

shaavan commented Aug 20, 2024

Uh oh!

shaavan commented Aug 21, 2024

Uh oh!

shaavan commented Aug 28, 2024

Uh oh!

shaavan commented Aug 28, 2024

Uh oh!

Uh oh!

shaavan commented Aug 29, 2024

Uh oh!

Uh oh!

shaavan commented Sep 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaavan commented Sep 9, 2024

Uh oh!

shaavan commented Sep 10, 2024

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan commented Sep 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jul 9, 2024•
edited by codecov Bot
Loading