-
Notifications
You must be signed in to change notification settings - Fork 380
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
Blinded pathfinding groundwork #2146
Blinded pathfinding groundwork #2146
Conversation
9ffccf0
to
510db9f
Compare
Oops, benchmark made me realize that the scoring methods need updates too. Will push in a bit. |
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #2146 +/- ##
==========================================
+ Coverage 91.39% 91.70% +0.30%
==========================================
Files 104 104
Lines 51431 53043 +1612
Branches 51431 53043 +1612
==========================================
+ Hits 47007 48643 +1636
+ Misses 4424 4400 -24
... and 35 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
6e98fa5
to
b5f4349
Compare
Rebased. |
b5f4349
to
f2a9bb2
Compare
f2a9bb2
to
86056fc
Compare
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.
Generally lgtm, note that we'll want to squash down the commits that fix things after the path introduction to avoid having broken commits in the history. To make it simpler, I wonder if it isn't easier to split the vec->path commit into one commit that does that transition without adding the blinded_tail field, and then a separate one that does.
lightning/src/routing/router.rs
Outdated
} | ||
} | ||
if blinded_tails.is_some() && blinded_tails.as_ref().unwrap().len() != self.paths.len() { | ||
return Err(io::Error::new(io::ErrorKind::Other, "Missing blinded tail for path (if blinded tails are included, there must be 1 set per path)")) |
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.
Can we just make the serialization format support this by pushing the option onto the vec? I mean I guess we hopefully won't ever actually use this, but in general all of our in-memory structs have a strong guarantee that they will never fail to serialize as long as the underlying Write
doesn't fail a push.
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.
I'm assuming we don't want to write anything unless a blinded tail is actually present, since it's an even tlv. So we could do what you're suggesting, but that would unnecessarily allocate a vec of None
s in most cases, and then we'd have to check that none of them are Some
before writing, which all seems unfortunate(?).
I think it would be ok to just remove this check, since we have strong in-memory guarantees as you say?
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.
On second thought, let's support writing the None
s? We don't have to always allocate - we can allocate the required number when we find the first blinded_tail set. The issue here is we dont have any in-memory guarantees that there arent some paths that have a blinded tail and others that dont, and while its kinda nonsense, its also the case that someone could construct such a route and we could pay it without issue.
Also needs rebase, sorry :(. |
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.
Concept ACK
4d1c9a0
to
96e3ff0
Compare
Rebased. |
d74e4ed
to
35e6e58
Compare
lightning/src/routing/router.rs
Outdated
|
||
/// A path in a [`Route`] to the payment recipient. Must always be at least length one. While the | ||
/// maximum length of any given path is variable, keeping the length of any path less than or equal | ||
/// to 19 should currently ensure it is viable. |
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.
Is this true with blinded paths, we have to give each hop a pubkey right, so the max length is much shorter.
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.
Ah I meant that path.len()
should be <=19, which includes blinded hops. Will clarify.
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.
But my point is that its not 19, as each blinded hop is substantially longer than non-blinded ones, the maximum length is variable. cc #2201
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.
we have to give each hop a pubkey right
I believe we only need to give the intro node's payload a (blinding point) pubkey. But there is more data in blinded hop payloads, yeah.
I updated the comment to caveat that 19 only applies to paths without a blinded tail. Do you want to go through updating MAX_PATH_LENGTH_ESTIMATE
and other docs updates in this PR, or save this for later addressing #2201?
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.
We can address in 2201, I think, we're not generating such paths yet anyway.
lightning/src/routing/router.rs
Outdated
} | ||
} | ||
if blinded_tails.is_some() && blinded_tails.as_ref().unwrap().len() != self.paths.len() { | ||
return Err(io::Error::new(io::ErrorKind::Other, "Missing blinded tail for path (if blinded tails are included, there must be 1 set per path)")) |
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.
On second thought, let's support writing the None
s? We don't have to always allocate - we can allocate the required number when we find the first blinded_tail set. The issue here is we dont have any in-memory guarantees that there arent some paths that have a blinded tail and others that dont, and while its kinda nonsense, its also the case that someone could construct such a route and we could pay it without issue.
lightning/src/routing/router.rs
Outdated
let mut max_path_offset = payment_params.max_total_cltv_expiry_delta - path_total_cltv_expiry_delta; | ||
max_path_offset = cmp::max( | ||
max_path_offset - (max_path_offset % MEDIAN_HOP_CLTV_EXPIRY_DELTA), | ||
max_path_offset % MEDIAN_HOP_CLTV_EXPIRY_DELTA); | ||
shadow_ctlv_expiry_delta_offset = cmp::min(shadow_ctlv_expiry_delta_offset, max_path_offset); | ||
|
||
// Add 'shadow' CLTV offset to the final hop | ||
if let Some(last_hop) = path.last_mut() { | ||
if let Some(tail) = path.blinded_tail.as_mut() { | ||
tail.final_cltv_expiry_delta = tail.final_cltv_expiry_delta |
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 we skip adding it to the last hop in this case?
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.
For the current docs to be accurate, we would need to mirror the cltv_expiry_delta
in path.hops.last()
and BlindedTail
for 1-hop blinded paths. We could add a special case to the docs and make it 0 in the last hop, if you prefer.
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.
Oh oh, right, because the last hop includes the blinded CLTV delta, okay, nvm.
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.
Okay, wait, I'm confused on this again - the way I read the current docs the blinded path's final_cltv_expiry_delta
is the final cltv expiry delta the recipient wants to receive, and the cltv_expiry_delta
of the last hop is the difference between that and the introduction point's cltv expiry. Thus, if we only want to change the last hop's cltv (and have it bubble up through the route) we should just change the blinded path part.
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.
Ah, yeah, you're right. Updating this and the docs. I had the last hop as containing the entire blinded path's cltv delta, including the recipient's, but that doesn't make sense.
Btw, I'm not entirely sure how to get the final_cltv_expiry_delta
for the last hop. It appears that eclair computes it as 0 + random_shadow_offset
. The route blinding spec indicates that a max_cltv_expiry
should be provided by the receiver in the BOLT12 invoice, but that doesn't seem to be a thing in the offers spec.
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.
Ah! I guess the cltv delta in the BlindedPayInfo
includes the recipient, then? Let's just remove the cltv delta from the blinded tail, then?
a526988
to
9861527
Compare
if let Some(blinded_tail) = &path.blinded_tail { | ||
if blinded_tails.is_empty() { | ||
blinded_tails = Vec::with_capacity(path.hops.len()); | ||
for _ in 0..idx { | ||
blinded_tails.push(None); | ||
} | ||
} | ||
blinded_tails.push(Some(blinded_tail)); | ||
} else if !blinded_tails.is_empty() { blinded_tails.push(None); } |
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.
Should it be the case that if one path has a blinded tail then all should? Should we fail if that's not the case?
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.
Originally I required all-or-nothing for Path
BlindedTail
s, but ended up allowing it because even though we'll never construct such Route
's, we'd have no issue paying them. See #2146 (comment)
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.
It should, but I complained because in general we have a strong guarantee that if something exists in-memory, it can be serialized. We don't have a reasonable way to provide/handle errors serializing objects, so we generally panic when writing to a sink that cannot fail (eg &mut Vec::new()
). If we're particularly worried about it, we should make the fields private and enforce the preconditions when updating/creating a route, but I'm not sure we need to be - while somewhat nonsensical, it is possible to pay such a route just fine.
BlindedHop { blinded_node_id: ln_test_utils::pubkey(49), encrypted_payload: Vec::new() } | ||
], | ||
}; | ||
// (De)serialize a Route with 1 blinded path out of two total paths. |
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.
Likewise, would this ever happen?
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.
Probably not, but it was broken in a prior iteration so thought I'd test it. Fine to remove.
lightning/src/routing/router.rs
Outdated
@@ -2111,6 +2111,8 @@ fn add_random_cltv_offset(route: &mut Route, payment_params: &PaymentParameters, | |||
let network_nodes = network_graph.nodes(); | |||
|
|||
for path in route.paths.iter_mut() { | |||
if path.blinded_tail.as_ref().map_or(false, |tail| tail.hops.len() > 1) { continue } |
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.
Could probably use a comment as to why this will skip.
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.
I'm also confused as to why we're doing this? I guess we just assume there's enough privacy if we're paying a blinded path? Maybe let's make the constant 2 instead?
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.
I think I was concerned we might blow up the cltv expiry if blinded path recipients add their own shadow offsets, but I think it makes more sense to just always add a shadow offset for blinded paths, rather than only adding it for 1-hop blinded paths. Updated.
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.
Given we're refusing to pay blinded paths and we moved the value calculation to happen before the addition of the blinded paths object, I don't really care if this is squashed anymore or not. Because our value calculations are right I'm okay with not, even if its a bit weird our serialization is lossy in the intermediate commits.
lightning/src/routing/router.rs
Outdated
let mut max_path_offset = payment_params.max_total_cltv_expiry_delta - path_total_cltv_expiry_delta; | ||
max_path_offset = cmp::max( | ||
max_path_offset - (max_path_offset % MEDIAN_HOP_CLTV_EXPIRY_DELTA), | ||
max_path_offset % MEDIAN_HOP_CLTV_EXPIRY_DELTA); | ||
shadow_ctlv_expiry_delta_offset = cmp::min(shadow_ctlv_expiry_delta_offset, max_path_offset); | ||
|
||
// Add 'shadow' CLTV offset to the final hop | ||
if let Some(last_hop) = path.last_mut() { | ||
if let Some(tail) = path.blinded_tail.as_mut() { | ||
tail.final_cltv_expiry_delta = tail.final_cltv_expiry_delta |
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.
Okay, wait, I'm confused on this again - the way I read the current docs the blinded path's final_cltv_expiry_delta
is the final cltv expiry delta the recipient wants to receive, and the cltv_expiry_delta
of the last hop is the difference between that and the introduction point's cltv expiry. Thus, if we only want to change the last hop's cltv (and have it bubble up through the route) we should just change the blinded path part.
lightning/src/routing/router.rs
Outdated
@@ -2111,6 +2111,8 @@ fn add_random_cltv_offset(route: &mut Route, payment_params: &PaymentParameters, | |||
let network_nodes = network_graph.nodes(); | |||
|
|||
for path in route.paths.iter_mut() { | |||
if path.blinded_tail.as_ref().map_or(false, |tail| tail.hops.len() > 1) { continue } |
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.
I'm also confused as to why we're doing this? I guess we just assume there's enough privacy if we're paying a blinded path? Maybe let's make the constant 2 instead?
@@ -721,8 +718,8 @@ impl OutboundPayments { | |||
} | |||
}; | |||
for path in route.paths.iter() { | |||
if path.len() == 0 { | |||
log_error!(logger, "length-0 path in route"); | |||
if path.hops.len() == 0 { |
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.
Wait, isn't this actually okay? If we're the introduction node for a blinded path away from us the hops should be empty?
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.
In the case that we're the intro node, we'll want to advance the blinded path before pathfinding (otherwise how can we pathfind to ourselves?). So I think we-are-the-intro-node should be an error in payment parameters initialization.
Therefore, I think hops.len()
will always be > 0
? If that's not the case, I have a bunch of path.hops.first().unwrap()
s to update..
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.
Ah, okay, I wasn't sure what the approach was, that makes sense.
dd8e87b
to
d974016
Compare
LGTM, feel free to squash. |
This lays groundwork for adding blinded path info to Path
d974016
to
6383245
Compare
Squashed without diff |
871a609
to
7631b6a
Compare
Squashed the commit disallowing paying blinded payment paths into the commit that adds blinded path info to |
7631b6a
to
dc426a4
Compare
dc426a4
to
b131634
Compare
0.0.115 - Apr 24, 2023 - "Rebroadcast the Bugfixes" API Updates =========== * The MSRV of the main LDK crates has been increased to 1.48 (lightningdevkit#2107). * Attempting to claim an un-expired payment on a channel which has closed no longer fails. The expiry time of payments is exposed via `PaymentClaimable::claim_deadline` (lightningdevkit#2148). * `payment_metadata` is now supported in `Invoice` deserialization, sending, and receiving (via a new `RecipientOnionFields` struct) (lightningdevkit#2139, lightningdevkit#2127). * `Event::PaymentFailed` now exposes a failure reason (lightningdevkit#2142). * BOLT12 messages now support stateless generation and validation (lightningdevkit#1989). * The `NetworkGraph` is now pruned of stale data after RGS processing (lightningdevkit#2161). * Max inbound HTLCs in-flight can be changed in the handshake config (lightningdevkit#2138). * `lightning-transaction-sync` feature `esplora-async-https` was added (lightningdevkit#2085). * A `ChannelPending` event is now emitted after the initial handshake (lightningdevkit#2098). * `PaymentForwarded::outbound_amount_forwarded_msat` was added (lightningdevkit#2136). * `ChannelManager::list_channels_by_counterparty` was added (lightningdevkit#2079). * `ChannelDetails::feerate_sat_per_1000_weight` was added (lightningdevkit#2094). * `Invoice::fallback_addresses` was added to fetch `bitcoin` types (lightningdevkit#2023). * The offer/refund description is now exposed in `Invoice{,Request}` (lightningdevkit#2206). Backwards Compatibility ======================= * Payments sent with the legacy `*_with_route` methods on LDK 0.0.115+ will no longer be retryable via the LDK 0.0.114- `retry_payment` method (lightningdevkit#2139). * `Event::PaymentPathFailed::retry` was removed and will always be `None` for payments initiated on 0.0.115 which fail on an earlier version (lightningdevkit#2063). * `Route`s and `PaymentParameters` with blinded path information will not be readable on prior versions of LDK. Such objects are not currently constructed by LDK, but may be when processing BOLT12 data in a coming release (lightningdevkit#2146). * Providing `ChannelMonitorUpdate`s generated by LDK 0.0.115 to a `ChannelMonitor` on 0.0.114 or before may panic (lightningdevkit#2059). Note that this is in general unsupported, and included here only for completeness. Bug Fixes ========= * Fixed a case where `process_events_async` may `poll` a `Future` which has already completed (lightningdevkit#2081). * Fixed deserialization of `u16` arrays. This bug may have previously corrupted the historical buckets in a `ProbabilisticScorer`. Users relying on the historical buckets may wish to wipe their scorer on upgrade to remove corrupt data rather than waiting on it to decay (lightningdevkit#2191). * The `process_events_async` task is now `Send` and can thus be polled on a multi-threaded runtime (lightningdevkit#2199). * Fixed a missing macro export causing `impl_writeable_tlv_based_enum{,_upgradable}` calls to not compile (lightningdevkit#2091). * Fixed compilation of `lightning-invoice` with both `no-std` and serde (lightningdevkit#2187) * Fix an issue where the `background-processor` would not wake when a `ChannelMonitorUpdate` completed asynchronously, causing delays (lightningdevkit#2090). * Fix an issue where `process_events_async` would exit immediately (lightningdevkit#2145). * `Router` calls from the `ChannelManager` now call `find_route_with_id` rather than `find_route`, as was intended and described in the API (lightningdevkit#2092). * Ensure `process_events_async` always exits if any sleep future returns true, not just if all sleep futures repeatedly return true (lightningdevkit#2145). * `channel_update` messages no longer set the disable bit unless the peer has been disconnected for some time. This should resolve cases where channels are disabled for extended periods of time (lightningdevkit#2198). * We no longer remove CLN nodes from the network graph for violating the BOLT spec in some cases after failing to pay through them (lightningdevkit#2220). * Fixed a debug assertion which may panic under heavy load (lightningdevkit#2172). * `CounterpartyForceClosed::peer_msg` is now wrapped in UntrustedString (lightningdevkit#2114) * Fixed a potential deadlock in `funding_transaction_generated` (lightningdevkit#2158). Security ======== * Transaction re-broadcasting is now substantially more aggressive, including a new regular rebroadcast feature called on a timer from the `background-processor` or from `ChainMonitor::rebroadcast_pending_claims`. This should substantially increase transaction confirmation reliability without relying on downstream `TransactionBroadcaster` implementations for rebroadcasting (lightningdevkit#2203, lightningdevkit#2205, lightningdevkit#2208). * Implemented the changes from BOLT PRs lightningdevkit#1031, lightningdevkit#1032, and lightningdevkit#1040 which resolve a privacy vulnerability which allows an intermediate node on the path to discover the final destination for a payment (lightningdevkit#2062). In total, this release features 110 files changed, 11928 insertions, 6368 deletions in 215 commits from 21 authors, in alphabetical order: * Advait * Alan Cohen * Alec Chen * Allan Douglas R. de Oliveira * Arik Sosman * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * John Cantrell * Lucas Soriano del Pino * Marc Tyndel * Matt Corallo * Paul Miller * Steven * Steven Williamson * Steven Zhao * Tony Giorgio * Valentine Wallace * Wilmer Paulino * benthecarman * munjesi
This lays some groundwork commits for #2120. Split out because we'll need some more interface changes and I don't want to hold up the serialization breakage portion, which we want to land for 0.0.115.