-
Notifications
You must be signed in to change notification settings - Fork 384
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
Upstream and fix preflight probing #2534
Conversation
d8aff41
to
f53903e
Compare
f34757a
to
6b34708
Compare
da48da0
to
e11a513
Compare
Rebased to resolve minor conflicts and undrafted. |
e11a513
to
38d196b
Compare
Codecov ReportPatch coverage:
❗ 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
☔ View full report in Codecov by Sentry. |
1535038
to
cbc8cc9
Compare
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.
9b06a18
to
ff91c98
Compare
15a93c6
to
315131d
Compare
315131d
to
6087aff
Compare
Feel free to squash the fixup commits, I think this LGTM. |
There was a problem hiding this 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.
6087aff
to
787fed4
Compare
Squashed the fixups.
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`.
787fed4
to
f75ac9a
Compare
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; | ||
} |
There was a problem hiding this comment.
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.
Fixes #2525.
Closes #2499.
Closes #2517.
Currently still WIP and hence in draft.
TODO: