Have `Route` hold `RouteParameters`
authorElias Rohrer <dev@tnull.de>
Thu, 31 Aug 2023 13:10:09 +0000 (15:10 +0200)
committerElias Rohrer <dev@tnull.de>
Wed, 6 Sep 2023 17:35:38 +0000 (19:35 +0200)
fuzz/src/chanmon_consistency.rs
lightning/src/ln/functional_tests.rs
lightning/src/ln/onion_utils.rs
lightning/src/ln/outbound_payment.rs
lightning/src/ln/payment_tests.rs
lightning/src/routing/router.rs
pending_changelog/routes_route_params.txt [new file with mode: 0644]

index 83470bf8bc8e063075bce4a217b7de72fa9c1808..4c79f0bee27f41e527bd40efdd51051e7dac15c9 100644 (file)
@@ -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);
index 8c71e5bd93c5851534505c6480d4084cb6d5f0c8..5b1895ac15c94eeaa8f4289db47a541924fe84bd 100644 (file)
@@ -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);
index d55077770f3baae35e2dab9c2f95fd2136071a50..8fdbdefef6592766b52b9ad0cf142e55ad045990 100644 (file)
@@ -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();
index 38ae89aec6b02187c7181c0bad1bf939e0cb2beb..67d90d2dbf8922496d04e7fa61cb8706d247dd48 100644 (file)
@@ -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();
index f8000423b565748e96794e6e3aedfb8a102916a8..0c94adb356084adc3bf5b13a5f547baeb4bbcf14 100644 (file)
@@ -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);
index 799b6407981341b0bc4fbfae0f387bdbe23186ad..8b3a96eb42ed21c04fe915d6c9674ced3d3da46a 100644 (file)
@@ -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<Path>,
-       /// 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<PaymentParameters>,
+       ///
+       /// Will be `None` for objects serialized with LDK versions prior to 0.0.117.
+       pub route_params: Option<RouteParameters>,
 }
 
 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 (file)
index 0000000..e88a1c7
--- /dev/null
@@ -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.