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

Upstream and fix preflight probing #2534

Merged

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Aug 28, 2023

Fixes #2525.
Closes #2499.
Closes #2517.

Currently still WIP and hence in draft.

TODO:

@tnull tnull marked this pull request as draft August 28, 2023 13:08
@tnull tnull force-pushed the 2023-08-upstream-preflight-probing branch 3 times, most recently from d8aff41 to f53903e Compare August 28, 2023 14:28
lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@tnull tnull force-pushed the 2023-08-upstream-preflight-probing branch 3 times, most recently from f34757a to 6b34708 Compare September 4, 2023 10:31
lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@TheBlueMatt TheBlueMatt added this to the 0.0.117 milestone Sep 7, 2023
@tnull tnull force-pushed the 2023-08-upstream-preflight-probing branch from da48da0 to e11a513 Compare September 11, 2023 11:57
@tnull tnull marked this pull request as ready for review September 11, 2023 11:57
@tnull
Copy link
Contributor Author

tnull commented Sep 11, 2023

Rebased to resolve minor conflicts and undrafted.

@tnull tnull force-pushed the 2023-08-upstream-preflight-probing branch from e11a513 to 38d196b Compare September 11, 2023 12:14
@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2023

Codecov Report

Patch coverage: 88.07% and project coverage change: +0.46% 🎉

Comparison is base (9f3e127) 90.60% compared to head (f75ac9a) 91.07%.
Report is 43 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2534      +/-   ##
==========================================
+ Coverage   90.60%   91.07%   +0.46%     
==========================================
  Files         112      113       +1     
  Lines       58860    64894    +6034     
  Branches    58860    64894    +6034     
==========================================
+ Hits        53332    59100    +5768     
- Misses       5528     5794     +266     
Files Changed Coverage Δ
lightning-background-processor/src/lib.rs 85.89% <ø> (+4.48%) ⬆️
lightning/src/ln/onion_utils.rs 93.80% <ø> (+2.02%) ⬆️
lightning/src/routing/scoring.rs 95.71% <0.00%> (+1.43%) ⬆️
lightning-invoice/src/payment.rs 85.90% <50.00%> (-2.37%) ⬇️
lightning/src/ln/outbound_payment.rs 92.40% <66.66%> (+1.70%) ⬆️
lightning/src/ln/channelmanager.rs 91.59% <84.74%> (+3.67%) ⬆️
lightning/src/ln/payment_tests.rs 97.48% <98.38%> (+0.02%) ⬆️
lightning/src/ln/functional_tests.rs 98.11% <100.00%> (ø)
lightning/src/routing/router.rs 94.50% <100.00%> (+0.01%) ⬆️

... and 15 files with indirect coverage changes

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

lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved
lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved
lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/util/config.rs Outdated Show resolved Hide resolved
lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved
lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved
lightning/src/ln/payment_tests.rs Outdated Show resolved Hide resolved
lightning/src/ln/payment_tests.rs Show resolved Hide resolved
lightning/src/ln/payment_tests.rs Show resolved Hide resolved
@tnull tnull force-pushed the 2023-08-upstream-preflight-probing branch 4 times, most recently from 1535038 to cbc8cc9 Compare September 12, 2023 14:32
Previously, we'd leave the payment secret field empty while sending
probes, which resulted in having them rejected
with `(PERM|invalid_onion_payload)` by Eclair nodes.

In order to mitigate the issue, we just set a random payment secret.
@tnull tnull force-pushed the 2023-08-upstream-preflight-probing branch 2 times, most recently from 9b06a18 to ff91c98 Compare September 13, 2023 11:25
@tnull tnull force-pushed the 2023-08-upstream-preflight-probing branch 2 times, most recently from 15a93c6 to 315131d Compare September 14, 2023 10:52
lightning/src/routing/router.rs Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@tnull tnull force-pushed the 2023-08-upstream-preflight-probing branch from 315131d to 6087aff Compare September 15, 2023 07:57
@TheBlueMatt
Copy link
Collaborator

Feel free to squash the fixup commits, I think this LGTM.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Mostly minor things. The commit message for ae8f536 could use details on the motivation.

lightning/src/routing/router.rs Show resolved Hide resolved
lightning/src/routing/router.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved
@tnull tnull force-pushed the 2023-08-upstream-preflight-probing branch from 6087aff to 787fed4 Compare September 18, 2023 11:24
@tnull
Copy link
Contributor Author

tnull commented Sep 18, 2023

Feel free to squash the fixup commits, I think this LGTM.

Squashed the fixups.

Mostly minor things. The commit message for ae8f536 could use details on the motivation.

Addressed by the following changes:

> git diff-tree -U2  6087affd 787fed49
diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs
index 60d82b54..f5b20d87 100644
--- a/lightning-invoice/src/payment.rs
+++ b/lightning-invoice/src/payment.rs
@@ -34,6 +34,5 @@ use core::time::Duration;
 /// If you wish to use a different payment idempotency token, see [`pay_invoice_with_id`].
 pub fn pay_invoice<C: AChannelManager>(
-       invoice: &Bolt11Invoice, retry_strategy: Retry,
-       channelmanager: &C
+       invoice: &Bolt11Invoice, retry_strategy: Retry, channelmanager: &C
 ) -> Result<PaymentId, PaymentError>
 {
@@ -54,6 +53,5 @@ pub fn pay_invoice<C: AChannelManager>(
 /// See [`pay_invoice`] for a variant which uses the [`PaymentHash`] for the idempotency token.
 pub fn pay_invoice_with_id<C: AChannelManager>(
-       invoice: &Bolt11Invoice, payment_id: PaymentId, retry_strategy: Retry,
-       channelmanager: &C
+       invoice: &Bolt11Invoice, payment_id: PaymentId, retry_strategy: Retry, channelmanager: &C
 ) -> Result<(), PaymentError>
 {
@@ -72,6 +70,5 @@ pub fn pay_invoice_with_id<C: AChannelManager>(
 /// [`pay_zero_value_invoice_with_id`].
 pub fn pay_zero_value_invoice<C: AChannelManager>(
-       invoice: &Bolt11Invoice, amount_msats: u64, retry_strategy: Retry,
-       channelmanager: &C
+       invoice: &Bolt11Invoice, amount_msats: u64, retry_strategy: Retry, channelmanager: &C
 ) -> Result<PaymentId, PaymentError>
 {
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index f7950cb3..785595a8 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -3567,8 +3567,8 @@ where
                                                "Avoided sending payment probe all the way to last hop {} as it is likely unannounced.",
                                                last_path_hop.short_channel_id
-                                               );
+                                       );
                                        let final_value_msat = path.final_value_msat();
                                        path.hops.pop();
-                                       if let Some(new_last) = path.hops.iter_mut().last() {
+                                       if let Some(new_last) = path.hops.last_mut() {
                                                new_last.fee_msat += final_value_msat;
                                        }
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 62d454cb..415e7b54 100644
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -2500,5 +2500,5 @@ where L::Target: Logger {
                                network_graph.node(&hop.node_id).map_or(false, |hop_node|
                                        hop_node.channels.iter().any(|scid| network_graph.channel(*scid)
-                                                       .map_or(false, |c| c.as_directed_from(&prev_hop_node_id).is_some()))
+                                               .map_or(false, |c| c.as_directed_from(&prev_hop_node_id).is_some()))
                                        )
                        };

When sending preflight probes, we want to exclude last hops that are
possibly announced. To this end, we here include a new field in
`RouteHop` that will be `true` when we either def. know the hop to be
announced, or, if there exist public channels between the hop's
counterparties that this hop might refer to (i.e., be an alias for).
We add a `ChannelManager::send_preflight_probes` method that can be used
to send pre-flight probes given some [`RouteParameters`]. Additionally,
we add convenience methods in for spontaneous probes and send pre-flight
probes for a given invoice.

As pre-flight probes might take up some of the available liquidity, we
here introduce that channels whose available liquidity is less than the
required amount times
`UserConfig::preflight_probing_liquidity_limit_multiplier` won't be used
to send pre-flight probes.

This commit is a more or less a carbon copy of the pre-flight
probing code recently added to LDK Node.
If the last hop was provided by route hint we assume it's not an announced channel.
If furthermore only a single route hint is provided we refrain from probing through
all the way to the end and instead probe up to the second-to-last channel.

Optimally we'd do this not based on above mentioned assumption but
rather by checking inclusion in our network graph. However, we don't
have access to our graph in `ChannelManager`.
@tnull tnull force-pushed the 2023-08-upstream-preflight-probing branch from 787fed4 to f75ac9a Compare September 18, 2023 13:08
Comment on lines +3570 to +3574
let final_value_msat = path.final_value_msat();
path.hops.pop();
if let Some(new_last) = path.hops.last_mut() {
new_last.fee_msat += final_value_msat;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this consider blinded paths somehow? Presumably the last hop towards a blinded path is always pub so we'll never hit this case anyway, but still.

@TheBlueMatt TheBlueMatt merged commit 36af1f0 into lightningdevkit:main Sep 18, 2023
11 of 14 checks passed
@TheBlueMatt TheBlueMatt mentioned this pull request Sep 18, 2023
57 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants