Align get_route's interface with ChannelManager and Invoice #946

jkczyz · 2021-06-09T04:43:53Z

Update get_route's interface to work better with ChannelManager and Invoice.

~~pass result of ChannelManager::list_usable_channels as first_hops without converting~~
accept multi-hop hints for last_hops from Invoice::routes without copying

This is only the interface change for #945. Only the last hop from each RouteHint is considered by get_route for now.

lightning/src/routing/router.rs

codecov · 2021-06-09T05:05:18Z

Codecov Report

Merging #946 (d0355b7) into main (a8038a8) will decrease coverage by 0.02%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main     #946      +/-   ##
==========================================
- Coverage   90.59%   90.57%   -0.03%     
==========================================
  Files          60       60              
  Lines       30425    30423       -2     
==========================================
- Hits        27564    27556       -8     
- Misses       2861     2867       +6

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`84.26% <60.00%> (+0.39%)`	⬆️
lightning-invoice/src/de.rs	`80.86% <80.00%> (ø)`
lightning-invoice/src/lib.rs	`88.49% <83.33%> (-0.40%)`	⬇️
lightning-invoice/src/ser.rs	`92.19% <100.00%> (ø)`
lightning/src/routing/router.rs	`95.95% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.17% <0.00%> (-0.05%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.85% <0.00%> (-0.05%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8038a8...d0355b7. Read the comment docs.

lightning-invoice/src/lib.rs

TheBlueMatt · 2021-06-10T16:20:44Z

Lets land this for 0.0.98 to clean up the API which will clean up our docs and sample a good chunk.

TheBlueMatt

This basically looks good.

TheBlueMatt · 2021-06-10T18:53:28Z

lightning/src/routing/router.rs

-pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, payee: &PublicKey, payee_features: Option<InvoiceFeatures>, first_hops: Option<&[&ChannelDetails]>,
-	last_hops: &[&RouteHintHop], final_value_msat: u64, final_cltv: u32, logger: L) -> Result<Route, LightningError> where L::Target: Logger {
+pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, payee: &PublicKey, payee_features: Option<InvoiceFeatures>, first_hops: Option<&[ChannelDetails]>,
+	last_hops: Vec<&RouteHint>, final_value_msat: u64, final_cltv: u32, logger: L) -> Result<Route, LightningError> where L::Target: Logger {


Can we take a &[&RouteHint]?

I was hoping to make it easy for callers by allowing them to pass the result of Invoice::route_hints directly. I guess they could just take a slice now that route_hints does the conversion, but I'll have to either touch or revert a bunch of code. Any specific reason a slice would be preferred?

I don't think there's any reason to need this by-value here? I suppose &Vec is also fine but slice is more general.

Updated to take a slice.

lightning-invoice/src/lib.rs

Used for extracting more than one field of the same type.

TheBlueMatt

LGTM, can you squash fixups?

TheBlueMatt · 2021-06-11T02:04:20Z

lightning/src/routing/router.rs

@@ -343,8 +347,8 @@ fn compute_fees(amount_msat: u64, channel_fees: RoutingFees) -> Option<u64> {
 /// The fees on channels from us to next-hops are ignored (as they are assumed to all be
 /// equal), however the enabled/disabled bit on such channels as well as the
 /// htlc_minimum_msat/htlc_maximum_msat *are* checked as they may change based on the receiving node.
-pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, payee: &PublicKey, payee_features: Option<InvoiceFeatures>, first_hops: Option<&[&ChannelDetails]>,
-	last_hops: &[&RouteHintHop], final_value_msat: u64, final_cltv: u32, logger: L) -> Result<Route, LightningError> where L::Target: Logger {
+pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, payee: &PublicKey, payee_features: Option<InvoiceFeatures>, first_hops: Option<&[ChannelDetails]>,


Oops, ugh, the ChannelDetails reference is here mostly for the bindings - the bindings take an array of pointers with some additional metadata, so they can't trivially convert them to [ChannelDetails] without a bunch of copies.

Dropped the commit but it does make the user do more work at the call site. Here is what the guide is looking like:

let amt_pico_btc = invoice.amount_pico_btc() .expect("ERROR: invalid invoice: must contain amount to pay"); let amt_msat = amt_pico_btc / 10; let payer_pubkey = channel_manager.get_our_node_id(); let network_graph = router.network_graph.read().unwrap(); let payee_pubkey = invoice.recover_payee_pub_key(); let payee_features = invoice.features().cloned(); let first_hops = channel_manager.list_usable_channels(); let last_hops = invoice.route_hints(); let final_cltv = invoice.min_final_cltv_expiry() as u32; let route = router::get_route( &payer_pubkey, &network_graph, &payee_pubkey, payee_features, Some(&first_hops.iter().collect::<Vec<_>>()), &last_hops, amt_msat, final_cltv, logger.clone(), ).expect("ERROR: failed to find route"); let payment_hash = PaymentHash(invoice.payment_hash().clone().into_inner()); let payment_secret = invoice.payment_secret().cloned(); channel_manager.send_payment(&route, payment_hash, &payment_secret) .expect("ERROR: failed to send payment");

Later, we may want take a pass through how our interfaces to see how well they mesh together.

Yea, its very not-ideal, I agree. I'm not really sure how to fix it in the bindings context, though, sadly.

lightning/src/routing/router.rs

TheBlueMatt

Needs squash.

Lightning invoices allow for zero or more multi-hop route hints. Update get_route's interface to accept such hints, although only the last hop from each is used for the time being. Moves RouteHint from lightning-invoice crate to lightning crate. Adds a PrivateRoute wrapper around RouteHint for use in lightning-invoice.

valentinewallace · 2021-06-11T15:53:13Z

lightning-invoice/src/utils.rs

@@ -128,7 +123,7 @@ mod test {
 			&invoice.recover_payee_pub_key(),
 			Some(invoice.features().unwrap().clone()),
 			Some(&first_hops.iter().collect::<Vec<_>>()),
-			&last_hops.iter().collect::<Vec<_>>(),
+			&last_hops,


TheBlueMatt reviewed Jun 9, 2021

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

jkczyz force-pushed the 2021-06-get-route-interface branch 2 times, most recently from b883ade to 422f79b Compare June 10, 2021 02:26

TheBlueMatt reviewed Jun 10, 2021

View reviewed changes

lightning-invoice/src/lib.rs Outdated Show resolved Hide resolved

TheBlueMatt added this to the 0.0.98 milestone Jun 10, 2021

jkczyz force-pushed the 2021-06-get-route-interface branch from 422f79b to 2e00de6 Compare June 10, 2021 18:31

TheBlueMatt reviewed Jun 10, 2021

View reviewed changes

jkczyz force-pushed the 2021-06-get-route-interface branch from 2e00de6 to b1abfb2 Compare June 10, 2021 19:53

Add find_all_extract macro to lightning-invoice

c9d1ed5

Used for extracting more than one field of the same type.

jkczyz force-pushed the 2021-06-get-route-interface branch 2 times, most recently from 199ea3f to 7ea6106 Compare June 10, 2021 23:15

TheBlueMatt approved these changes Jun 11, 2021

View reviewed changes

jkczyz force-pushed the 2021-06-get-route-interface branch from 7ea6106 to b56fdd0 Compare June 11, 2021 01:41

TheBlueMatt reviewed Jun 11, 2021

View reviewed changes

jkczyz force-pushed the 2021-06-get-route-interface branch from b56fdd0 to abf1deb Compare June 11, 2021 08:21

TheBlueMatt approved these changes Jun 11, 2021

View reviewed changes

jkczyz added 2 commits June 11, 2021 08:44

Fix type name in docs

d0355b7

jkczyz force-pushed the 2021-06-get-route-interface branch from abf1deb to d0355b7 Compare June 11, 2021 15:45

valentinewallace approved these changes Jun 11, 2021

View reviewed changes

TheBlueMatt merged commit 7b4b753 into lightningdevkit:main Jun 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align get_route's interface with ChannelManager and Invoice #946

Align get_route's interface with ChannelManager and Invoice #946

jkczyz commented Jun 9, 2021 •

edited

Loading

codecov bot commented Jun 9, 2021 •

edited

Loading

TheBlueMatt commented Jun 10, 2021

TheBlueMatt left a comment

TheBlueMatt Jun 10, 2021

jkczyz Jun 10, 2021

TheBlueMatt Jun 10, 2021

jkczyz Jun 10, 2021

TheBlueMatt left a comment

TheBlueMatt Jun 11, 2021

jkczyz Jun 11, 2021

TheBlueMatt Jun 11, 2021

TheBlueMatt left a comment

valentinewallace Jun 11, 2021

Align get_route's interface with ChannelManager and Invoice #946

Align get_route's interface with ChannelManager and Invoice #946

Conversation

jkczyz commented Jun 9, 2021 • edited Loading

codecov bot commented Jun 9, 2021 • edited Loading

Codecov Report

TheBlueMatt commented Jun 10, 2021

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Jun 9, 2021 •

edited

Loading

codecov bot commented Jun 9, 2021 •

edited

Loading