From 61cf75265ebff493af5a04550a5ff8b250f3b2f6 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 31 Aug 2023 15:10:09 +0200 Subject: [PATCH] Have `Route` hold `RouteParameters` --- fuzz/src/chanmon_consistency.rs | 6 ++-- lightning/src/ln/functional_tests.rs | 8 +++-- lightning/src/ln/onion_utils.rs | 2 +- lightning/src/ln/outbound_payment.rs | 12 ++++---- lightning/src/ln/payment_tests.rs | 32 ++++++++++++-------- lightning/src/routing/router.rs | 37 ++++++++++++++++------- pending_changelog/routes_route_params.txt | 3 ++ 7 files changed, 65 insertions(+), 35 deletions(-) create mode 100644 pending_changelog/routes_route_params.txt diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 83470bf8..4c79f0be 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -376,7 +376,7 @@ fn send_payment(source: &ChanMan, dest: &ChanMan, dest_chan_id: u64, amt: u64, p fee_msat: amt, cltv_expiry_delta: 200, }], blinded_tail: None }], - payment_params: None, + route_params: None, }, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_id)) { check_payment_err(err, amt > max_value_sendable || amt < min_value_sendable); false @@ -409,7 +409,7 @@ fn send_hop_payment(source: &ChanMan, middle: &ChanMan, middle_chan_id: u64, des channel_features: middle.channel_features(), fee_msat: first_hop_fee, cltv_expiry_delta: 100, - },RouteHop { + }, RouteHop { pubkey: dest.get_our_node_id(), node_features: dest.node_features(), short_channel_id: dest_chan_id, @@ -417,7 +417,7 @@ fn send_hop_payment(source: &ChanMan, middle: &ChanMan, middle_chan_id: u64, des fee_msat: amt, cltv_expiry_delta: 200, }], blinded_tail: None }], - payment_params: None, + route_params: None, }, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_id)) { let sent_amt = amt + first_hop_fee; check_payment_err(err, sent_amt < min_value_sendable || sent_amt > max_value_sendable); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 8c71e5bd..5b1895ac 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1049,7 +1049,9 @@ fn fake_network_test() { }); hops[1].fee_msat = chan_4.1.contents.fee_base_msat as u64 + chan_4.1.contents.fee_proportional_millionths as u64 * hops[2].fee_msat as u64 / 1000000; hops[0].fee_msat = chan_3.0.contents.fee_base_msat as u64 + chan_3.0.contents.fee_proportional_millionths as u64 * hops[1].fee_msat as u64 / 1000000; - let payment_preimage_1 = send_along_route(&nodes[1], Route { paths: vec![Path { hops, blinded_tail: None }], payment_params: None }, &vec!(&nodes[2], &nodes[3], &nodes[1])[..], 1000000).0; + let payment_preimage_1 = send_along_route(&nodes[1], + Route { paths: vec![Path { hops, blinded_tail: None }], route_params: None }, + &vec!(&nodes[2], &nodes[3], &nodes[1])[..], 1000000).0; let mut hops = Vec::with_capacity(3); hops.push(RouteHop { @@ -1078,7 +1080,9 @@ fn fake_network_test() { }); hops[1].fee_msat = chan_2.1.contents.fee_base_msat as u64 + chan_2.1.contents.fee_proportional_millionths as u64 * hops[2].fee_msat as u64 / 1000000; hops[0].fee_msat = chan_3.1.contents.fee_base_msat as u64 + chan_3.1.contents.fee_proportional_millionths as u64 * hops[1].fee_msat as u64 / 1000000; - let payment_hash_2 = send_along_route(&nodes[1], Route { paths: vec![Path { hops, blinded_tail: None }], payment_params: None }, &vec!(&nodes[3], &nodes[2], &nodes[1])[..], 1000000).1; + let payment_hash_2 = send_along_route(&nodes[1], + Route { paths: vec![Path { hops, blinded_tail: None }], route_params: None }, + &vec!(&nodes[3], &nodes[2], &nodes[1])[..], 1000000).1; // Claim the rebalances... fail_payment(&nodes[1], &vec!(&nodes[3], &nodes[2], &nodes[1])[..], payment_hash_2); diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index d5507777..8fdbdefe 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -1007,7 +1007,7 @@ mod tests { short_channel_id: 0, fee_msat: 0, cltv_expiry_delta: 0 // We fill in the payloads manually instead of generating them from RouteHops. }, ], blinded_tail: None }], - payment_params: None, + route_params: None, }; let onion_keys = super::construct_onion_keys(&secp_ctx, &route.paths[0], &get_test_session_key()).unwrap(); diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 38ae89ae..67d90d2d 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -976,7 +976,7 @@ impl OutboundPayments { })) } - let route = Route { paths: vec![path], payment_params: None }; + let route = Route { paths: vec![path], route_params: None }; let onion_session_privs = self.add_new_pending_payment(payment_hash, RecipientOnionFields::spontaneous_empty(), payment_id, None, &route, None, None, entropy_source, best_block_height)?; @@ -1145,9 +1145,9 @@ impl OutboundPayments { results, payment_id, failed_paths_retry: if pending_amt_unsent != 0 { - if let Some(payment_params) = &route.payment_params { + if let Some(payment_params) = route.route_params.as_ref().map(|p| p.payment_params.clone()) { Some(RouteParameters { - payment_params: payment_params.clone(), + payment_params: payment_params, final_value_msat: pending_amt_unsent, }) } else { None } @@ -1569,7 +1569,7 @@ mod tests { let pending_events = Mutex::new(VecDeque::new()); if on_retry { outbound_payments.add_new_pending_payment(PaymentHash([0; 32]), RecipientOnionFields::spontaneous_empty(), - PaymentId([0; 32]), None, &Route { paths: vec![], payment_params: None }, + PaymentId([0; 32]), None, &Route { paths: vec![], route_params: None }, Some(Retry::Attempts(1)), Some(expired_route_params.payment_params.clone()), &&keys_manager, 0).unwrap(); outbound_payments.retry_payment_internal( @@ -1613,7 +1613,7 @@ mod tests { let pending_events = Mutex::new(VecDeque::new()); if on_retry { outbound_payments.add_new_pending_payment(PaymentHash([0; 32]), RecipientOnionFields::spontaneous_empty(), - PaymentId([0; 32]), None, &Route { paths: vec![], payment_params: None }, + PaymentId([0; 32]), None, &Route { paths: vec![], route_params: None }, Some(Retry::Attempts(1)), Some(route_params.payment_params.clone()), &&keys_manager, 0).unwrap(); outbound_payments.retry_payment_internal( @@ -1657,7 +1657,7 @@ mod tests { fee_msat: 0, cltv_expiry_delta: 0, }], blinded_tail: None }], - payment_params: Some(payment_params), + route_params: Some(route_params.clone()), }; router.expect_find_route(route_params.clone(), Ok(route.clone())); let mut route_params_w_failed_scid = route_params.clone(); diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index f8000423..0c94adb3 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -94,7 +94,7 @@ fn mpp_retry() { // Initiate the MPP payment. let payment_id = PaymentId(payment_hash.0); - let mut route_params = RouteParameters::from_payment_params_and_value(route.payment_params.clone().unwrap(), amt_msat); + let mut route_params = route.route_params.clone().unwrap(); nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret), @@ -524,7 +524,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { let amt_msat = 1_000_000; let (route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], amt_msat); let (payment_preimage_1, payment_hash_1, _, payment_id_1) = send_along_route(&nodes[0], route.clone(), &[&nodes[1], &nodes[2]], 1_000_000); - let route_params = RouteParameters::from_payment_params_and_value(route.payment_params.clone().unwrap(), amt_msat); + let route_params = route.route_params.unwrap().clone(); nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap(); check_added_monitors!(nodes[0], 1); @@ -2211,7 +2211,7 @@ fn auto_retry_partial_failure() { cltv_expiry_delta: 100, }], blinded_tail: None }, ], - payment_params: Some(route_params.payment_params.clone()), + route_params: Some(route_params.clone()), }; let retry_1_route = Route { paths: vec![ @@ -2232,7 +2232,7 @@ fn auto_retry_partial_failure() { cltv_expiry_delta: 100, }], blinded_tail: None }, ], - payment_params: Some(route_params.payment_params.clone()), + route_params: Some(route_params.clone()), }; let retry_2_route = Route { paths: vec![ @@ -2245,7 +2245,7 @@ fn auto_retry_partial_failure() { cltv_expiry_delta: 100, }], blinded_tail: None }, ], - payment_params: Some(route_params.payment_params.clone()), + route_params: Some(route_params.clone()), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route)); let mut payment_params = route_params.payment_params.clone(); @@ -2497,13 +2497,13 @@ fn retry_multi_path_single_failed_payment() { cltv_expiry_delta: 100, }], blinded_tail: None }, ], - payment_params: Some(payment_params), + route_params: Some(route_params.clone()), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); // On retry, split the payment across both channels. route.paths[0].hops[0].fee_msat = 50_000_001; route.paths[1].hops[0].fee_msat = 50_000_000; - let mut pay_params = route.payment_params.clone().unwrap(); + let mut pay_params = route.route_params.clone().unwrap().payment_params; pay_params.previously_failed_channels.push(chans[1].short_channel_id.unwrap()); nodes[0].router.expect_find_route( // Note that the second request here requests the amount we originally failed to send, @@ -2578,7 +2578,9 @@ fn immediate_retry_on_failure() { cltv_expiry_delta: 100, }], blinded_tail: None }, ], - payment_params: Some(PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)), + route_params: Some(RouteParameters::from_payment_params_and_value( + PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV), + 100_000_001)), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); // On retry, split the payment across both channels. @@ -2684,7 +2686,9 @@ fn no_extra_retries_on_back_to_back_fail() { cltv_expiry_delta: 100, }], blinded_tail: None } ], - payment_params: Some(PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV)), + route_params: Some(RouteParameters::from_payment_params_and_value( + PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV), + 100_000_000)), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); let mut second_payment_params = route_params.payment_params.clone(); @@ -2882,7 +2886,9 @@ fn test_simple_partial_retry() { cltv_expiry_delta: 100, }], blinded_tail: None } ], - payment_params: Some(PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV)), + route_params: Some(RouteParameters::from_payment_params_and_value( + PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV), + 100_000_000)), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); let mut second_payment_params = route_params.payment_params.clone(); @@ -3044,7 +3050,9 @@ fn test_threaded_payment_retries() { cltv_expiry_delta: 100, }], blinded_tail: None } ], - payment_params: Some(PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV)), + route_params: Some(RouteParameters::from_payment_params_and_value( + PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV), + amt_msat - amt_msat / 1000)), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); @@ -3470,7 +3478,7 @@ fn test_retry_custom_tlvs() { // Initiate the payment let payment_id = PaymentId(payment_hash.0); - let mut route_params = RouteParameters::from_payment_params_and_value(route.payment_params.clone().unwrap(), amt_msat); + let mut route_params = route.route_params.clone().unwrap(); let custom_tlvs = vec![((1 << 16) + 1, vec![0x42u8; 16])]; let onion_fields = RecipientOnionFields::secret_only(payment_secret); diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 799b6407..8b3a96eb 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -337,10 +337,12 @@ pub struct Route { /// [`BlindedTail`]s are present, then the pubkey of the last [`RouteHop`] in each path must be /// the same. pub paths: Vec, - /// The `payment_params` parameter passed via [`RouteParameters`] to [`find_route`]. + /// The `route_params` parameter passed to [`find_route`]. /// /// This is used by `ChannelManager` to track information which may be required for retries. - pub payment_params: Option, + /// + /// Will be `None` for objects serialized with LDK versions prior to 0.0.117. + pub route_params: Option, } impl Route { @@ -383,8 +385,11 @@ impl Writeable for Route { } } write_tlv_fields!(writer, { - (1, self.payment_params, option), + // For compatibility with LDK versions prior to 0.0.117, we take the individual + // RouteParameters' fields and reconstruct them on read. + (1, self.route_params.as_ref().map(|p| &p.payment_params), option), (2, blinded_tails, optional_vec), + (3, self.route_params.as_ref().map(|p| p.final_value_msat), option), }); Ok(()) } @@ -411,6 +416,7 @@ impl Readable for Route { _init_and_read_len_prefixed_tlv_fields!(reader, { (1, payment_params, (option: ReadableArgs, min_final_cltv_expiry_delta)), (2, blinded_tails, optional_vec), + (3, final_value_msat, option), }); let blinded_tails = blinded_tails.unwrap_or(Vec::new()); if blinded_tails.len() != 0 { @@ -419,14 +425,23 @@ impl Readable for Route { path.blinded_tail = blinded_tail_opt; } } - Ok(Route { paths, payment_params }) + + // If we previously wrote the corresponding fields, reconstruct RouteParameters. + let route_params = match (payment_params, final_value_msat) { + (Some(payment_params), Some(final_value_msat)) => { + Some(RouteParameters { payment_params, final_value_msat }) + } + _ => None, + }; + + Ok(Route { paths, route_params }) } } /// Parameters needed to find a [`Route`]. /// /// Passed to [`find_route`] and [`build_route_from_hops`]. -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, Hash, PartialEq, Eq)] pub struct RouteParameters { /// The parameters of the failed payment path. pub payment_params: PaymentParameters, @@ -2489,7 +2504,7 @@ where L::Target: Logger { } } - let route = Route { paths, payment_params: Some(payment_params.clone()) }; + let route = Route { paths, route_params: Some(route_params.clone()) }; log_info!(logger, "Got route: {}", log_route!(route)); Ok(route) } @@ -5953,7 +5968,7 @@ mod tests { short_channel_id: 0, fee_msat: 225, cltv_expiry_delta: 0 }, ], blinded_tail: None }], - payment_params: None, + route_params: None, }; assert_eq!(route.get_total_fees(), 250); @@ -5986,7 +6001,7 @@ mod tests { short_channel_id: 0, fee_msat: 150, cltv_expiry_delta: 0 }, ], blinded_tail: None }], - payment_params: None, + route_params: None, }; assert_eq!(route.get_total_fees(), 200); @@ -5998,7 +6013,7 @@ mod tests { // In an earlier version of `Route::get_total_fees` and `Route::get_total_amount`, they // would both panic if the route was completely empty. We test to ensure they return 0 // here, even though its somewhat nonsensical as a route. - let route = Route { paths: Vec::new(), payment_params: None }; + let route = Route { paths: Vec::new(), route_params: None }; assert_eq!(route.get_total_fees(), 0); assert_eq!(route.get_total_amount(), 0); @@ -6597,7 +6612,7 @@ mod tests { fee_msat: 100, cltv_expiry_delta: 0, }], blinded_tail: None }], - payment_params: None, + route_params: None, }; let encoded_route = route.encode(); let decoded_route: Route = Readable::read(&mut Cursor::new(&encoded_route[..])).unwrap(); @@ -6691,7 +6706,7 @@ mod tests { excess_final_cltv_expiry_delta: 0, final_value_msat: 200, }), - }], payment_params: None}; + }], route_params: None}; let payment_params = PaymentParameters::from_node_id(ln_test_utils::pubkey(47), 18); let (_, network_graph, _, _, _) = build_line_graph(); diff --git a/pending_changelog/routes_route_params.txt b/pending_changelog/routes_route_params.txt new file mode 100644 index 00000000..e88a1c78 --- /dev/null +++ b/pending_changelog/routes_route_params.txt @@ -0,0 +1,3 @@ +# Backwards Compatibility + +- `Route` objects written with LDK versions prior to 0.0.117 won't be retryable after being deserialized with LDK 0.0.117 or above. -- 2.30.2