Multi-hop route hints are now considered. Issue #945 #1030
Conversation
abhik-99
changed the title
Codecov Report
@@ Coverage Diff @@
## main #1030 +/- ##
=======================================
Coverage 90.84% 90.85%
=======================================
Files 61 61
Lines 31534 31601 +67
=======================================
+ Hits 28646 28710 +64
- Misses 2888 2891 +3
Continue to review full report at Codecov.
|
lightning/src/routing/router.rs
Outdated
| @@ -373,11 +373,14 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye | |||
| if final_value_msat == 0 { | |||
| return Err(LightningError{err: "Cannot send a payment of 0 msat".to_owned(), action: ErrorAction::IgnoreError}); | |||
| } | |||
|
|
|||
| let last_hops = last_hops.clone().iter().map(|hops| hops.0.clone()).collect::<Vec<_>>(); | |||
There was a problem hiding this comment.
Clipppy says this (I'm not sure why you added the clone at all - we should be able to just iterate directly and maybe drop this line entirely).
error: using `clone` on a double-reference; this will copy the reference instead of cloning the inner type
--> lightning/src/routing/router.rs:377:18
|
377 | let last_hops = last_hops.clone().iter().map(|hops| hops.0.clone()).collect::<Vec<_>>();
| ^^^^^^^^^^^^^^^^^
|
= note: `#[deny(clippy::clone_double_ref)]` on by default
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_double_ref
help: try dereferencing it
lightning/src/routing/router.rs
Outdated
| @@ -373,11 +373,14 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye | |||
| if final_value_msat == 0 { | |||
| return Err(LightningError{err: "Cannot send a payment of 0 msat".to_owned(), action: ErrorAction::IgnoreError}); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
nit: whitespace at end-of-line here.
| // sufficient value to route `final_value_msat`. Note that in the case of "0-value" | ||
| // invoices where the invoice does not specify value this may not be the case, but | ||
| // better to include the hints than not. | ||
| if add_entry!(hop.short_channel_id, hop.src_node_id, payee, directional_info, Some((final_value_msat + 999) / 1000), &empty_channel_features, 0, path_value_msat, 0) { |
There was a problem hiding this comment.
A multi-hop route hint isn't simply more route hints to get to the payee, but instead channels which eventually get to the payee, through multiple private channels.
Thus, this is no longer correct - anything but the last hop is no longer from hop.src_node_id to payee but instead from hop.src_node_id to the next hop in route (except the last entry, which is to payee).
| assert_eq!(route.paths[0][1].node_features.le_flags(), &id_to_feature_flags(3)); | ||
| assert_eq!(route.paths[0][1].channel_features.le_flags(), &id_to_feature_flags(4)); | ||
|
|
||
| assert_eq!(route.paths[0][2].pubkey, nodes[4]); |
There was a problem hiding this comment.
In order to properly test the route hints, you should disable some of the other paths to node 6, especially the one taken here over channel 6 and 11.
lightning/src/routing/router.rs
Outdated
| @@ -373,11 +373,14 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye | |||
| if final_value_msat == 0 { | |||
| return Err(LightningError{err: "Cannot send a payment of 0 msat".to_owned(), action: ErrorAction::IgnoreError}); | |||
| } | |||
|
|
|||
| let last_hops = &(*last_hops).clone().iter().map(|hops| hops.0.clone()).collect::<Vec<_>>(); | |||
There was a problem hiding this comment.
You should be able to avoid the clone entirely, I think, and just do last_hops.iter().
| return Err(LightningError{err: "Last hop cannot have a payee as a source.".to_owned(), action: ErrorAction::IgnoreError}); | ||
| for routes in last_hops.iter() { | ||
| for last_hop in routes.iter() { | ||
| if last_hop.src_node_id == *payee { |
There was a problem hiding this comment.
AFAIU we already support multiple r-fields, what we don't support and this PR implements is multiple hops per-r-field ?
If so I think you could rename last_hops to r_fields, routes to the singular route as each r field constitutes a revealed forward route, and last_hop to hop only as it's not necessarily the latest link of the route anymore ?
There was a problem hiding this comment.
r_fields is pretty opaque, maybe paths_to_payee?
Probable Solution to Issue #945 .
Edited the last_hops to now consider all the RouteHintHop(s) in the RouteHint instead of only the last RouteHintHop. The tests modified are:-