Allow blinded path diversification by expanding `create_blinded_paths` by shaavan · Pull Request #3087 · lightningdevkit/rust-lightning

shaavan · 2024-05-31T19:29:25Z

The current usage of blinded_paths is limited because create_blinded_path only returns a single BlindedPath.
This PR expands the functionality of create_blinded_path by allowing it to return multiple BlindedPaths, as determined by the new count parameter.
Additionally, this PR integrates this new capability throughout the codebase by:
- Allowing multiple paths in offers and refund builders.
- Sending Offers Response messages, such as InvoiceRequest (in pay_for_offer) and Invoice (in request_refund_payment), using multiple reply paths.
As a proof-of-concept, this PR increases the maximum count of create_blinded_paths to 10, enabling the generation of more reply paths. It also increases the number of blinded_paths used in offer and refund builders and responders to 5, demonstrating the usage of multiple reply paths.

codecov-commenter · 2024-05-31T19:57:25Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.78%. Comparing base (78c0eaa) to head (957b337).

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

Additional details and impacted files

@@ Coverage Diff @@## main #3087 +/- ##
==========================================
- Coverage 89.80% 89.78% -0.02% 
==========================================
Files 121 121 Lines 100045 100207 +162 Branches 100045 100207 +162 ==========================================
+ Hits 89845 89975 +130 - Misses 7533 7558 +25 - Partials 2667 2674 +7

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

shaavan · 2024-06-03T18:47:56Z

Updated from pr3087.01 to pr3087.02 (diff):
Addressed @jkczyz comment

Changes:

Remove the redundant MAX_PATHS constant and the associated debug_assert!
Reduce the number of paths created in offers, and refund builder to 1, and introduce a TODO comment.
Refactor the path functions
Update pay_for_offer to allow setting REQUEST_LIMIT to the number of messages created, instead of the number of the path used. Also updated the comment accordingly.

shaavan · 2024-06-06T14:24:22Z

Updated from pr3087.02 to pr3087.03 (diff):
Changes:

Rebase on main.

shaavan · 2024-06-06T14:36:13Z

Updated from pr3087.03 to pr3087.04 (diff):
Addressed @jkczyz comments

Changes:

Move the and_then call to the call, and revert the Offer and Refund Builder path functions to the original. The function again adds only a single path to offer/refund paths.
Refactored the pay_for_offer code to remove redundant clonings.
Updated request_refund_payment to use similar REQUEST_LIMIT as pay_for_offer.
Introduce a constant MAX_REPLY_PATHS to pay_for_offer and request_refund_payment.

shaavan · 2024-06-10T14:04:40Z

Updated from pr3087.04 to pr3087.05 (diff):
Addressed @TheBlueMatt and @jkczyz

Updates:

Reverted changes to MessageRouter's create_blinded_paths.
Update the create_blinded_path function to return the entire vector returned by create_blinded_paths, and hence remove the count parameter.

shaavan · 2024-06-10T14:17:39Z

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

Changes:

Rebase on main.
Also update create_blinded_path_using_absolute_expiry and create_compact_blinded_path to return Vec<BlindedPath>

shaavan · 2024-06-12T07:46:44Z

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

Changes:

Reverted the changes in onion_messenger.rs as they were not affecting the behavior.
Updated function names and docs to better suit their roles.
Remove the TODO comment as the exact interface is to be decided.

jbesraa

Looks good
Is it worth adding a test to cover the new code?

jkczyz · 2024-06-20T19:26:43Z

Looks good Is it worth adding a test to cover the new code?

Chatted with @shaavan earlier today about how to go about testing this.

shaavan · 2024-06-24T12:05:03Z

Updated from pr3087.07 to pr3087.08 (diff):
Addressed @TheBlueMatt, @jbesraa comment

Changes:

Pulled REQUEST_LIMIT into a global const, and renamed it to OFFERS_MESSAGE_REQUEST_LIMIT.
Introduce a test to check the new behavior.

Thanks, @jkczyz, for the pointers and inspirations for the testing.

shaavan · 2024-06-29T13:17:55Z

Updated from pr3087.09 to pr3087.10 (diff):
Addressed @jbesraa comments

Changes:

Referenced the PaymentId in the const's documentation.

shaavan · 2024-07-02T09:12:09Z

Updated from pr3087.10 to pr3087.11 (diff):
Addressed @jkczyz comments

Changes:

Replace invoices -> requests to match the context the word is used.
Remove the reference for PaymentId since it's already used in the file.

shaavan · 2024-07-05T13:03:00Z

Updated from pr3087.11 to pr3087.12 (diff):
Addressed @jkczyz comment

Changes:

Update extract_invoice() to also return the corresponding reply_path.
Introduce reply_path_diversification test for refund case.

shaavan · 2024-07-05T13:16:25Z

Updated from pr3087.12 to pr3087.13 (diff):

Changes:

Rebase on main.
Change the return type of extract_invoice() from (Bolt12Invoice, BlindedPath) -> (Bolt12Invoice, Option<BlindedPath>), to avoid some test failures.

jkczyz · 2024-07-09T01:17:14Z

+fn extract_invoice<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, message: &OnionMessage) -> (Bolt12Invoice, Option<BlindedPath>) {
 	match node.onion_messenger.peel_onion_message(message) {
- Ok(PeeledOnion::Receive(message, _, _)) => match message {
+ Ok(PeeledOnion::Receive(message, _, reply_path)) => match message {


As a follow-up PR, could you start including a reply path with invoices for offers? We should have the capability now, but we just haven't updated ChannelManager's implementation of OffersMessageHandler yet.

Sure! Added it to the task list! 🚀

shaavan · 2024-07-09T13:08:48Z

Updated from pr3087.13 to pr3087.14 (diff):
Addressed @jkczyz comments

Updates:

Rename added test, to be more explicit.
Update added const comment to be more clear.

jkczyz

The change looks good. I went through the tests and noted where we should remove checks that aren't relevant to the new behavior. There may be tests where we are asserting more than necessary, but might as well avoid doing that in newer tests.

…ication - Previously, the `create_blinded_path` function was limited to returning a single `BlindedPath`, which restricted the usage of `blinded_paths`. - This commit extends the `create_blinded_path` function to return the entire blinded path vector generated by the `MessageRouter`'s `create_blinded_paths`. - The updated functionality is integrated across the codebase, enabling the sending of Offers Response messages, such as `InvoiceRequest` (in `pay_for_offer`) and `Invoice` (in `request_refund_payment`), utilizing multiple reply paths.

- This will be utilised in the following commit for a test.

shaavan · 2024-07-11T13:48:48Z

Updated from pr3087.14 to pr3087.15 (diff):

Updates:

Rebase on main to resolve merge conflicts.

jkczyz

LGTM. Please squash the fixup.

shaavan · 2024-07-12T14:19:21Z

Updated from pr3087.16 to pr3087.17 (diff):
Addressed @jkczyz comment

Squashed the f: commits together.
Fixed one failing test by reintroducing an important line of code.

TheBlueMatt

LGTM. Diff since @jkczyz's "LGTM" is trivial, so landing:

$ git diff-tree -U2 e6bb965bcd3fede82b956c5abb17d6e501c8d9af 957b33712ad3d5a7aed624e4f565887817fd9944
diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs
index 2eddb1991..cdd78d02c 100644
--- a/lightning/src/ln/offers_tests.rs+++ b/lightning/src/ln/offers_tests.rs@@ -1019,4 +1019,6 @@ fn send_invoice_for_refund_with_distinct_reply_path() {
expect_recent_payment!(alice, RecentPaymentDetails::AwaitingInvoice, payment_id);
+	let _expected_invoice = david.node.request_refund_payment(&refund).unwrap();+
connect_peers(david, bob);
$

jkczyz self-requested a review May 31, 2024 19:56

jkczyz reviewed Jun 3, 2024
View reviewed changes

Comment threadlightning/src/onion_message/messenger.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/offers/offer.rs Outdated
Comment threadlightning/src/offers/refund.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated

shaavan force-pushed the reply_path_diversity branch from f0c2e7d to f32a93aCompare June 3, 2024 18:48

jkczyz reviewed Jun 5, 2024
View reviewed changes

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

shaavan force-pushed the reply_path_diversity branch from f32a93a to c8e7993Compare June 6, 2024 14:24

TheBlueMatt reviewed Jun 6, 2024
View reviewed changes

Comment threadlightning/src/onion_message/messenger.rs Outdated

shaavan force-pushed the reply_path_diversity branch from c8e7993 to 4032289Compare June 6, 2024 14:36

shaavan force-pushed the reply_path_diversity branch from 4032289 to 56013beCompare June 10, 2024 14:04

shaavan force-pushed the reply_path_diversity branch from 56013be to b6e38c2Compare June 10, 2024 14:17

jkczyz reviewed Jun 11, 2024
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/onion_message/messenger.rs Outdated

shaavan force-pushed the reply_path_diversity branch from b6e38c2 to 4461115Compare June 12, 2024 07:46

TheBlueMatt reviewed Jun 12, 2024
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs

TheBlueMatt previously approved these changes Jun 13, 2024
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

jbesraa reviewed Jun 13, 2024
View reviewed changes

TheBlueMatt assigned jkczyz Jun 20, 2024

shaavan dismissed TheBlueMatt’s stale review via d672475June 24, 2024 12:00

shaavan force-pushed the reply_path_diversity branch from d672475 to 5d66998Compare June 24, 2024 12:04

TheBlueMatt previously approved these changes Jun 28, 2024
View reviewed changes

jkczyz reviewed Jun 28, 2024
View reviewed changes

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

shaavan dismissed TheBlueMatt’s stale review via f541586June 29, 2024 12:46

shaavan force-pushed the reply_path_diversity branch from 5d66998 to f541586Compare June 29, 2024 12:46

shaavan force-pushed the reply_path_diversity branch from f541586 to 94354f3Compare June 29, 2024 13:17

jbesraa approved these changes Jul 1, 2024
View reviewed changes

jkczyz reviewed Jul 1, 2024
View reviewed changes

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

shaavan force-pushed the reply_path_diversity branch from 94354f3 to 0354d8dCompare July 2, 2024 09:12

jkczyz reviewed Jul 2, 2024
View reviewed changes

Comment threadlightning/src/ln/offers_tests.rs Outdated

shaavan force-pushed the reply_path_diversity branch from 0354d8d to 43e821cCompare July 5, 2024 13:02

shaavan force-pushed the reply_path_diversity branch from 43e821c to 893f62dCompare July 5, 2024 13:16

jkczyz reviewed Jul 9, 2024
View reviewed changes

shaavan force-pushed the reply_path_diversity branch from 893f62d to 212ade4Compare July 9, 2024 13:08

shaavan mentioned this pull request Jul 9, 2024
Introduce Reply Paths for BOLT12 Invoice in Offers Flow. #3163
Merged

jkczyz reviewed Jul 9, 2024
View reviewed changes

jkczyz mentioned this pull request Jul 10, 2024
Introduce Retry InvoiceRequest Flow #3010
Merged

shaavan added 2 commits July 11, 2024 19:12

Update extract_invoice to also return the reply_path.
887e999
- This will be utilised in the following commit for a test.

shaavan force-pushed the reply_path_diversity branch from 212ade4 to f5aa883Compare July 11, 2024 13:48

shaavan force-pushed the reply_path_diversity branch from 0e16cdb to e6bb965Compare July 11, 2024 14:02

jkczyz reviewed Jul 12, 2024
View reviewed changes

shaavan force-pushed the reply_path_diversity branch from e6bb965 to a643112Compare July 12, 2024 14:13

Introduce tests to check verify reply path diversification
957b337

shaavan force-pushed the reply_path_diversity branch from a643112 to 957b337Compare July 12, 2024 14:16

TheBlueMatt approved these changes Jul 16, 2024
View reviewed changes

TheBlueMatt merged commit 6ed398d into lightningdevkit:mainJul 16, 2024

shaavan deleted the reply_path_diversity branch July 17, 2024 10:44

Uh oh!

Conversation

shaavan commented May 31, 2024

Uh oh!

codecov-commenter commented May 31, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaavan commented Jun 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaavan commented Jun 6, 2024

Uh oh!

Uh oh!

shaavan commented Jun 6, 2024

Uh oh!

shaavan commented Jun 10, 2024

Uh oh!

shaavan commented Jun 10, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaavan commented Jun 12, 2024

Uh oh!

Uh oh!

Uh oh!

jbesraa left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz commented Jun 20, 2024

Uh oh!

shaavan commented Jun 24, 2024

Uh oh!

Uh oh!

Uh oh!

shaavan commented Jun 29, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaavan commented Jul 2, 2024

Uh oh!

Uh oh!

shaavan commented Jul 5, 2024

Uh oh!

shaavan commented Jul 5, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkczyzJul 9, 2024

Choose a reason for hiding this comment

Uh oh!

shaavanJul 9, 2024

Choose a reason for hiding this comment

Uh oh!

shaavanJul 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shaavan commented Jul 9, 2024

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 31, 2024•
edited
Loading

shaavan commented Jul 12, 2024•
edited
Loading