diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index d8291571884..b0703d8e6ed 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -1866,7 +1866,7 @@ impl PaymentTracker { }], blinded_tail: None, }], - route_params: Some(route_params.clone()), + route_params: route_params.clone(), }; let onion = RecipientOnionFields::secret_only(secret, amt); let res = source.send_payment_with_route(route, hash, onion, id); @@ -1946,7 +1946,7 @@ impl PaymentTracker { ], blinded_tail: None, }], - route_params: Some(route_params.clone()), + route_params: route_params.clone(), }; let onion = RecipientOnionFields::secret_only(secret, amt); let res = source.send_payment_with_route(route, hash, onion, id); @@ -2028,7 +2028,7 @@ impl PaymentTracker { PaymentParameters::from_node_id(dest.get_our_node_id(), TEST_FINAL_CLTV), amt, ); - let route = Route { paths, route_params: Some(route_params) }; + let route = Route { paths, route_params }; let onion = RecipientOnionFields::secret_only(secret, amt); let res = source.send_payment_with_route(route, hash, onion, id); let succeeded = match res { @@ -2132,7 +2132,7 @@ impl PaymentTracker { PaymentParameters::from_node_id(dest.get_our_node_id(), TEST_FINAL_CLTV), amt, ); - let route = Route { paths, route_params: Some(route_params) }; + let route = Route { paths, route_params }; let onion = RecipientOnionFields::secret_only(secret, amt); let res = source.send_payment_with_route(route, hash, onion, id); let succeeded = match res { diff --git a/lightning/src/ln/blinded_payment_tests.rs b/lightning/src/ln/blinded_payment_tests.rs index 32c0709ed5c..b4de3791679 100644 --- a/lightning/src/ln/blinded_payment_tests.rs +++ b/lightning/src/ln/blinded_payment_tests.rs @@ -2093,6 +2093,7 @@ fn test_trampoline_forward_payload_encoded_as_receive() { let bob_carol_scid = nodes[1].node().list_channels().iter().find(|c| c.channel_id == chan_id_bob_carol).unwrap().short_channel_id.unwrap(); let amt_msat = 1000; + let carol_cltv_expiry_delta = 24 + 39; let (payment_preimage, payment_hash, _) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), None); // We need the session priv to construct an invalid onion packet later. @@ -2148,7 +2149,7 @@ fn test_trampoline_forward_payload_encoded_as_receive() { short_channel_id: bob_carol_scid, channel_features: ChannelFeatures::empty(), fee_msat: 0, - cltv_expiry_delta: 24 + 39, + cltv_expiry_delta: carol_cltv_expiry_delta, maybe_announced_channel: false, } ], @@ -2159,7 +2160,7 @@ fn test_trampoline_forward_payload_encoded_as_receive() { pubkey: carol_node_id, node_features: Features::empty(), fee_msat: amt_msat, - cltv_expiry_delta: 24 + 39, + cltv_expiry_delta: carol_cltv_expiry_delta, }, ], hops: carol_blinded_hops, @@ -2168,7 +2169,10 @@ fn test_trampoline_forward_payload_encoded_as_receive() { final_value_msat: amt_msat, }) }], - route_params: None, + route_params: RouteParameters::from_payment_params_and_value( + PaymentParameters::from_node_id(carol_node_id, carol_cltv_expiry_delta), + amt_msat, + ), }; nodes[0].node.send_payment_with_route(route.clone(), payment_hash, RecipientOnionFields::spontaneous_empty(amt_msat), PaymentId(payment_hash.0)).unwrap(); @@ -2279,6 +2283,7 @@ fn do_test_trampoline_single_hop_receive(success: bool) { let bob_carol_scid = nodes[1].node().list_channels().iter().find(|c| c.channel_id == chan_id_bob_carol).unwrap().short_channel_id.unwrap(); let amt_msat = 1000; + let carol_cltv_expiry_delta = 104 + 39; let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), None); // Create a 1-hop blinded path for Carol. @@ -2314,7 +2319,7 @@ fn do_test_trampoline_single_hop_receive(success: bool) { short_channel_id: bob_carol_scid, channel_features: ChannelFeatures::empty(), fee_msat: 0, - cltv_expiry_delta: 104 + 39, + cltv_expiry_delta: carol_cltv_expiry_delta, maybe_announced_channel: false, } ], @@ -2325,7 +2330,7 @@ fn do_test_trampoline_single_hop_receive(success: bool) { pubkey: carol_node_id, node_features: Features::empty(), fee_msat: amt_msat, - cltv_expiry_delta: 104 + 39, + cltv_expiry_delta: carol_cltv_expiry_delta, }, ], hops: blinded_path.blinded_hops().to_vec(), @@ -2334,7 +2339,10 @@ fn do_test_trampoline_single_hop_receive(success: bool) { final_value_msat: amt_msat, }) }], - route_params: None, + route_params: RouteParameters::from_payment_params_and_value( + PaymentParameters::from_node_id(carol_node_id, carol_cltv_expiry_delta), + amt_msat, + ), }; nodes[0].node.send_payment_with_route(route.clone(), payment_hash, RecipientOnionFields::spontaneous_empty(amt_msat), PaymentId(payment_hash.0)).unwrap(); @@ -2618,7 +2626,13 @@ fn do_test_trampoline_relay(blinded: bool, test_case: TrampolineTestCase) { original_amt_msat, )), }], - route_params: None, + route_params: RouteParameters::from_payment_params_and_value( + PaymentParameters::from_node_id( + carol_node_id, + original_trampoline_cltv + excess_final_cltv, + ), + original_amt_msat, + ), }; nodes[0] @@ -2753,6 +2767,7 @@ fn test_trampoline_forward_rejection() { let bob_carol_scid = nodes[1].node().list_channels().iter().find(|c| c.channel_id == chan_id_bob_carol).unwrap().short_channel_id.unwrap(); let amt_msat = 1000; + let carol_cltv_expiry_delta = 24 + 24 + 39; let (payment_preimage, payment_hash, _) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), None); let route = Route { @@ -2776,7 +2791,7 @@ fn test_trampoline_forward_rejection() { short_channel_id: bob_carol_scid, channel_features: ChannelFeatures::empty(), fee_msat: 0, - cltv_expiry_delta: 24 + 24 + 39, + cltv_expiry_delta: carol_cltv_expiry_delta, maybe_announced_channel: false, } ], @@ -2808,7 +2823,10 @@ fn test_trampoline_forward_rejection() { final_value_msat: amt_msat, }) }], - route_params: None, + route_params: RouteParameters::from_payment_params_and_value( + PaymentParameters::from_node_id(carol_node_id, carol_cltv_expiry_delta), + amt_msat, + ), }; nodes[0].node.send_payment_with_route(route.clone(), payment_hash, RecipientOnionFields::spontaneous_empty(amt_msat), PaymentId(payment_hash.0)).unwrap(); diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 9633800db08..81062ea7cc3 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -2315,7 +2315,7 @@ fn test_path_paused_mpp() { route.paths[1].hops[0].pubkey = node_c_id; route.paths[1].hops[0].short_channel_id = chan_2_ann.contents.short_channel_id; route.paths[1].hops[1].short_channel_id = chan_4_id; - route.route_params.as_mut().unwrap().final_value_msat *= 2; + route.route_params.final_value_msat *= 2; // Set it so that the first monitor update (for the path 0 -> 1 -> 3) succeeds, but the second // (for the path 0 -> 2 -> 3) fails. @@ -4315,7 +4315,7 @@ fn do_test_partial_claim_mon_update_compl_actions(reload_a: bool, reload_b: bool route.paths[1].hops[0].pubkey = node_c_id; route.paths[1].hops[0].short_channel_id = chan_2_scid; route.paths[1].hops[1].short_channel_id = chan_4_scid; - route.route_params.as_mut().unwrap().final_value_msat *= 2; + route.route_params.final_value_msat *= 2; let paths = &[&[&nodes[1], &nodes[3]][..], &[&nodes[2], &nodes[3]][..]]; send_along_route_with_secret(&nodes[0], route, paths, 200_000, payment_hash, payment_secret); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 49392264709..5eb2146d7e9 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -5631,30 +5631,32 @@ impl< /// /// LDK will not automatically retry this payment, though it may be manually re-sent after an /// [`Event::PaymentFailed`] is generated. - #[rustfmt::skip] pub fn send_payment_with_route( - &self, mut route: Route, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, - payment_id: PaymentId + &self, route: Route, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, + payment_id: PaymentId, ) -> Result<(), RetryableSendFailure> { let best_block_height = self.best_block.read().unwrap().height; let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); - let route_params = route.route_params.clone().unwrap_or_else(|| { - // Create a dummy route params since they're a required parameter but unused in this case - let (payee_node_id, cltv_delta) = route.paths.first() - .and_then(|path| path.hops.last().map(|hop| (hop.pubkey, hop.cltv_expiry_delta as u32))) - .unwrap_or_else(|| (PublicKey::from_slice(&[2; 33]).unwrap(), MIN_FINAL_CLTV_EXPIRY_DELTA as u32)); - let dummy_payment_params = PaymentParameters::from_node_id(payee_node_id, cltv_delta); - RouteParameters::from_payment_params_and_value(dummy_payment_params, route.get_total_amount()) - }); - if route.route_params.is_none() { route.route_params = Some(route_params.clone()); } + let route_params = route.route_params.clone(); let router = FixedRouter::new(route); let logger = WithContext::for_payment(&self.logger, None, None, Some(payment_hash), payment_id); - self.pending_outbound_payments - .send_payment(payment_hash, recipient_onion, payment_id, Retry::Attempts(0), - route_params, &&router, self.list_usable_channels(), || self.compute_inflight_htlcs(), - &self.entropy_source, &self.node_signer, best_block_height, - &self.pending_events, |args| self.send_payment_along_path(args), &logger) + self.pending_outbound_payments.send_payment( + payment_hash, + recipient_onion, + payment_id, + Retry::Attempts(0), + route_params, + &&router, + self.list_usable_channels(), + || self.compute_inflight_htlcs(), + &self.entropy_source, + &self.node_signer, + best_block_height, + &self.pending_events, + |args| self.send_payment_along_path(args), + &logger, + ) } /// Sends a payment to the route found using the provided [`RouteParameters`], retrying failed @@ -21110,7 +21112,7 @@ mod tests { // Next, send a keysend payment with the same payment_hash and make sure it fails. nodes[0].node.send_spontaneous_payment( Some(payment_preimage), RecipientOnionFields::spontaneous_empty(100_000), - PaymentId(payment_preimage.0), route.route_params.clone().unwrap(), Retry::Attempts(0) + PaymentId(payment_preimage.0), route.route_params.clone(), Retry::Attempts(0) ).unwrap(); check_added_monitors(&nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); @@ -21266,7 +21268,7 @@ mod tests { ).unwrap(); let payment_hash = nodes[0].node.send_spontaneous_payment( Some(payment_preimage), RecipientOnionFields::spontaneous_empty(100_000), - PaymentId(payment_preimage.0), route.route_params.clone().unwrap(), Retry::Attempts(0) + PaymentId(payment_preimage.0), route.route_params.clone(), Retry::Attempts(0) ).unwrap(); check_added_monitors(&nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); @@ -21309,7 +21311,7 @@ mod tests { let payment_id_1 = PaymentId([44; 32]); let payment_hash = nodes[0].node.send_spontaneous_payment( Some(payment_preimage), RecipientOnionFields::spontaneous_empty(100_000), payment_id_1, - route.route_params.clone().unwrap(), Retry::Attempts(0) + route.route_params.clone(), Retry::Attempts(0) ).unwrap(); check_added_monitors(&nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); @@ -21427,7 +21429,7 @@ mod tests { route.paths[1].hops[0].pubkey = nodes[2].node.get_our_node_id(); route.paths[1].hops[0].short_channel_id = chan_2_id; route.paths[1].hops[1].short_channel_id = chan_4_id; - route.route_params.as_mut().unwrap().final_value_msat *= 2; + route.route_params.final_value_msat *= 2; nodes[0].node.send_payment_with_route(route, payment_hash, RecipientOnionFields::spontaneous_empty(200000), PaymentId(payment_hash.0)).unwrap(); diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 82c1c619e82..6e855c2a184 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -3492,14 +3492,14 @@ pub fn send_along_route_with_secret<'a, 'b, 'c>( recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: PaymentSecret, ) -> PaymentId { let payment_id = PaymentId(origin_node.keys_manager.backing.get_secure_random_bytes()); - origin_node.router.expect_find_route(route.route_params.clone().unwrap(), Ok(route.clone())); + origin_node.router.expect_find_route(route.route_params.clone(), Ok(route.clone())); origin_node .node .send_payment( our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret, recv_value), payment_id, - route.route_params.unwrap(), + route.route_params, Retry::Attempts(0), ) .unwrap(); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 52e2f2e96bf..826b07750fa 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -164,7 +164,7 @@ pub fn fake_network_test() { let route_params = RouteParameters::from_payment_params_and_value(payment_params, 1000000); let route = Route { paths: vec![Path { hops, blinded_tail: None }], - route_params: Some(route_params.clone()), + route_params: route_params.clone(), }; let path: &[_] = &[&nodes[2], &nodes[3], &nodes[1]]; let payment_preimage_1 = send_along_route(&nodes[1], route, path, 1000000).0; @@ -202,8 +202,7 @@ pub fn fake_network_test() { + 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 route = - Route { paths: vec![Path { hops, blinded_tail: None }], route_params: Some(route_params) }; + let route = Route { paths: vec![Path { hops, blinded_tail: None }], route_params }; let path: &[_] = &[&nodes[3], &nodes[2], &nodes[1]]; let payment_hash_2 = send_along_route(&nodes[1], route, path, 1000000).1; @@ -7214,7 +7213,7 @@ pub fn test_simple_mpp() { route.paths[1].hops[0].pubkey = node_c_id; route.paths[1].hops[0].short_channel_id = chan_2_id; route.paths[1].hops[1].short_channel_id = chan_4_id; - route.route_params.as_mut().unwrap().final_value_msat = 200_000; + route.route_params.final_value_msat = 200_000; let paths: &[&[_]] = &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]]; send_along_route_with_secret(&nodes[0], route, paths, 200_000, payment_hash, payment_secret); claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], paths, payment_preimage)); diff --git a/lightning/src/ln/max_payment_path_len_tests.rs b/lightning/src/ln/max_payment_path_len_tests.rs index c066f2c6d7b..242ba8000b8 100644 --- a/lightning/src/ln/max_payment_path_len_tests.rs +++ b/lightning/src/ln/max_payment_path_len_tests.rs @@ -110,7 +110,7 @@ fn large_payment_metadata() { custom_tlvs: Vec::new(), total_mpp_amount_msat: amt_msat, }; - let route_params = route_0_1.route_params.clone().unwrap(); + let route_params = route_0_1.route_params.clone(); let id = PaymentId(payment_hash.0); nodes[0] .node @@ -138,14 +138,14 @@ fn large_payment_metadata() { let (payment_hash_2, _, payment_secret_2, encrypted_metadata_2) = get_payment_hash!(nodes[2], payment_metadata.clone()); let (mut route_0_2, ..) = get_route_and_payment_hash!(&nodes[0], &nodes[2], amt_msat); - let mut route_params_0_2 = route_0_2.route_params.clone().unwrap(); + let mut route_params_0_2 = route_0_2.route_params.clone(); route_params_0_2.payment_params.max_path_length = 1; nodes[0].router.expect_find_route_query(route_params_0_2); max_sized_onion.payment_secret = Some(payment_secret_2); max_sized_onion.payment_metadata = Some(encrypted_metadata_2); let id = PaymentId(payment_hash_2.0); - let mut route_params = route_0_2.route_params.clone().unwrap(); + let mut route_params = route_0_2.route_params.clone(); let err = nodes[0] .node .send_payment(payment_hash_2, max_sized_onion.clone(), id, route_params, Retry::Attempts(0)) @@ -184,7 +184,7 @@ fn large_payment_metadata() { _ => panic!(), } - let route_params = route_0_1.route_params.clone().unwrap(); + let route_params = route_0_1.route_params.clone(); let err = nodes[0] .node .send_payment(payment_hash_2, too_large_onion, id, route_params, Retry::Attempts(0)) @@ -202,10 +202,10 @@ fn large_payment_metadata() { custom_tlvs: Vec::new(), total_mpp_amount_msat: amt_msat, }; - let mut route_params_0_2 = route_0_2.route_params.clone().unwrap(); + let mut route_params_0_2 = route_0_2.route_params.clone(); route_params_0_2.payment_params.max_path_length = 2; nodes[0].router.expect_find_route_query(route_params_0_2); - let route_params = route_0_2.route_params.unwrap(); + let route_params = route_0_2.route_params; nodes[0] .node .send_payment(payment_hash_2, onion_allowing_2_hops, id, route_params, Retry::Attempts(0)) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 234af588eae..d4ee3d36374 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -3034,7 +3034,7 @@ mod tests { use crate::ln::channelmanager::PaymentId; use crate::ln::msgs::{self, UpdateFailHTLC}; use crate::ln::types::ChannelId; - use crate::routing::router::{Path, PaymentParameters, Route, RouteHop}; + use crate::routing::router::{Path, PaymentParameters, Route, RouteHop, RouteParameters}; use crate::types::features::{ChannelFeatures, NodeFeatures}; use crate::types::payment::PaymentHash; use crate::util::ser::{VecWriter, Writeable, Writer}; @@ -3142,7 +3142,10 @@ mod tests { let secp_ctx = Secp256k1::new(); let path = build_test_path(); - let route = Route { paths: vec![path], route_params: None }; + let payment_params = PaymentParameters::from_node_id(path.hops.last().unwrap().pubkey, 0); + let route_params = + RouteParameters::from_payment_params_and_value(payment_params, path.final_value_msat()); + let route = Route { paths: vec![path], route_params }; let onion_keys = super::construct_onion_keys(&secp_ctx, &route.paths[0], &get_test_session_key()); diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 748dfe1e701..105ee355a9e 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -985,7 +985,7 @@ impl OutboundPayments { fn validate_found_route( route: &mut Route, route_params: &RouteParameters, logger: &WithContext, ) -> Result<(), ()> { - if route.route_params.as_ref() != Some(route_params) { + if route.route_params != *route_params { debug_assert!( false, "Routers are expected to return a Route which includes the requested RouteParameters. Got {:?}, expected {route_params:?}", @@ -996,7 +996,7 @@ fn validate_found_route( "Routers are expected to return a Route which includes the requested RouteParameters. Got {:?}, expected {route_params:?}", route.route_params ); - route.route_params = Some(route_params.clone()); + route.route_params = route_params.clone(); } route.debug_assert_route_meets_params(logger)?; @@ -1203,14 +1203,13 @@ impl OutboundPayments { }, }; - let payment_params = Some(route_params.payment_params.clone()); let mut outbounds = self.pending_outbound_payments.lock().unwrap(); let onion_session_privs = match outbounds.entry(payment_id) { hash_map::Entry::Occupied(entry) => match entry.get() { PendingOutboundPayment::InvoiceReceived { .. } => { let (retryable_payment, onion_session_privs) = Self::create_pending_payment( payment_hash, recipient_onion.clone(), keysend_preimage, None, Some(bolt12_invoice.clone()), &route, - Some(retry_strategy), payment_params, entropy_source, best_block_height, + Some(retry_strategy), entropy_source, best_block_height, ); *entry.into_mut() = retryable_payment; onion_session_privs @@ -1221,7 +1220,7 @@ impl OutboundPayments { } else { unreachable!() }; let (retryable_payment, onion_session_privs) = Self::create_pending_payment( payment_hash, recipient_onion.clone(), keysend_preimage, Some(invreq), Some(bolt12_invoice.clone()), &route, - Some(retry_strategy), payment_params, entropy_source, best_block_height + Some(retry_strategy), entropy_source, best_block_height ); outbounds.insert(payment_id, retryable_payment); onion_session_privs @@ -1617,7 +1616,7 @@ impl OutboundPayments { let onion_session_privs = self.add_new_pending_payment(payment_hash, recipient_onion.clone(), payment_id, keysend_preimage, &route, Some(retry_strategy), - Some(route_params.payment_params.clone()), entropy_source, best_block_height, None) + entropy_source, best_block_height, None) .map_err(|_| { log_error!(logger, "Payment with id {} is already pending. New payment had payment hash {}", payment_id, payment_hash); @@ -1923,11 +1922,24 @@ impl OutboundPayments { })) } - let route = Route { paths: vec![path], route_params: None }; + // `route_params` is a required field, but is unused when sending a probe along a fixed + // path. Construct dummy parameters from the path, leaving the fee budget unset to match + // the previous behavior of not tracking one for probes. + let route_params = { + let last_hop = path.hops.last().unwrap(); + let payment_params = + PaymentParameters::from_node_id(last_hop.pubkey, last_hop.cltv_expiry_delta); + RouteParameters { + payment_params, + final_value_msat: path.final_value_msat(), + max_total_routing_fee_msat: None, + } + }; + let route = Route { paths: vec![path], route_params }; let recipient_onion_fields = RecipientOnionFields::secret_only(payment_secret, route.get_total_amount()); let onion_session_privs = self.add_new_pending_payment(payment_hash, - recipient_onion_fields.clone(), payment_id, None, &route, None, None, + recipient_onion_fields.clone(), payment_id, None, &route, None, entropy_source, best_block_height, None ).map_err(|e| { debug_assert!(matches!(e, PaymentSendFailure::DuplicatePayment)); @@ -1983,14 +1995,14 @@ impl OutboundPayments { &self, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, payment_id: PaymentId, route: &Route, retry_strategy: Option, entropy_source: &ES, best_block_height: u32 ) -> Result, PaymentSendFailure> { - self.add_new_pending_payment(payment_hash, recipient_onion, payment_id, None, route, retry_strategy, None, entropy_source, best_block_height, None) + self.add_new_pending_payment(payment_hash, recipient_onion, payment_id, None, route, retry_strategy, entropy_source, best_block_height, None) } #[rustfmt::skip] pub(super) fn add_new_pending_payment( &self, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, payment_id: PaymentId, keysend_preimage: Option, route: &Route, retry_strategy: Option, - payment_params: Option, entropy_source: &ES, best_block_height: u32, + entropy_source: &ES, best_block_height: u32, bolt12_invoice: Option ) -> Result, PaymentSendFailure> { let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap(); @@ -1999,7 +2011,7 @@ impl OutboundPayments { hash_map::Entry::Vacant(entry) => { let (payment, onion_session_privs) = Self::create_pending_payment( payment_hash, recipient_onion, keysend_preimage, None, bolt12_invoice, route, retry_strategy, - payment_params, entropy_source, best_block_height + entropy_source, best_block_height ); entry.insert(payment); Ok(onion_session_privs) @@ -2012,7 +2024,7 @@ impl OutboundPayments { payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, keysend_preimage: Option, invoice_request: Option, bolt12_invoice: Option, route: &Route, retry_strategy: Option, - payment_params: Option, entropy_source: &ES, best_block_height: u32 + entropy_source: &ES, best_block_height: u32 ) -> (PendingOutboundPayment, Vec<[u8; 32]>) { let mut onion_session_privs = Vec::with_capacity(route.paths.len()); for _ in 0..route.paths.len() { @@ -2022,7 +2034,7 @@ impl OutboundPayments { let mut payment = PendingOutboundPayment::Retryable { retry_strategy, attempts: PaymentAttempts::new(), - payment_params, + payment_params: Some(route.route_params.payment_params.clone()), session_privs: new_hash_set(), pending_amt_msat: 0, pending_fee_msat: Some(0), @@ -2036,8 +2048,7 @@ impl OutboundPayments { starting_block_height: best_block_height, total_msat: route.get_total_amount(), onion_total_msat: recipient_onion.total_mpp_amount_msat, - remaining_max_total_routing_fee_msat: - route.route_params.as_ref().and_then(|p| p.max_total_routing_fee_msat), + remaining_max_total_routing_fee_msat: route.route_params.max_total_routing_fee_msat, }; for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) { @@ -2204,19 +2215,17 @@ impl OutboundPayments { results, payment_id, failed_paths_retry: if has_unsent { - if let Some(route_params) = &route.route_params { - let mut route_params = route_params.clone(); - // We calculate the leftover fee budget we're allowed to spend by - // subtracting the used fee from the total fee budget. - route_params.max_total_routing_fee_msat = route_params - .max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat)); - - // We calculate the remaining target amount by subtracting the succeded - // path values. - route_params.final_value_msat = route_params.final_value_msat - .saturating_sub(total_ok_amt_sent_msat); - Some(route_params) - } else { None } + let mut route_params = route.route_params.clone(); + // We calculate the leftover fee budget we're allowed to spend by + // subtracting the used fee from the total fee budget. + route_params.max_total_routing_fee_msat = route_params + .max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat)); + + // We calculate the remaining target amount by subtracting the succeded + // path values. + route_params.final_value_msat = route_params.final_value_msat + .saturating_sub(total_ok_amt_sent_msat); + Some(route_params) } else { None }, }) } else if has_err { @@ -2954,8 +2963,8 @@ mod tests { let pending_events = Mutex::new(VecDeque::new()); if on_retry { outbound_payments.add_new_pending_payment(PaymentHash([0; 32]), RecipientOnionFields::spontaneous_empty(0), - PaymentId([0; 32]), None, &Route { paths: vec![], route_params: None }, - Some(Retry::Attempts(1)), Some(expired_route_params.payment_params.clone()), + PaymentId([0; 32]), None, &Route { paths: vec![], route_params: expired_route_params.clone() }, + Some(Retry::Attempts(1)), &&keys_manager, 0, None).unwrap(); outbound_payments.find_route_and_send_payment( PaymentHash([0; 32]), PaymentId([0; 32]), expired_route_params, &&router, vec![], @@ -3000,8 +3009,8 @@ mod tests { let pending_events = Mutex::new(VecDeque::new()); if on_retry { outbound_payments.add_new_pending_payment(PaymentHash([0; 32]), RecipientOnionFields::spontaneous_empty(0), - PaymentId([0; 32]), None, &Route { paths: vec![], route_params: None }, - Some(Retry::Attempts(1)), Some(route_params.payment_params.clone()), + PaymentId([0; 32]), None, &Route { paths: vec![], route_params: route_params.clone() }, + Some(Retry::Attempts(1)), &&keys_manager, 0, None).unwrap(); outbound_payments.find_route_and_send_payment( PaymentHash([0; 32]), PaymentId([0; 32]), route_params, &&router, vec![], @@ -3048,13 +3057,13 @@ mod tests { cltv_expiry_delta: 0, maybe_announced_channel: true, }], blinded_tail: None }], - route_params: Some(route_params.clone()), + route_params: route_params.clone(), }; router.expect_find_route(route_params.clone(), Ok(route.clone())); let mut route_params_w_failed_scid = route_params.clone(); route_params_w_failed_scid.payment_params.previously_failed_channels.push(failed_scid); let mut route_w_failed_scid = route.clone(); - route_w_failed_scid.route_params = Some(route_params_w_failed_scid.clone()); + route_w_failed_scid.route_params = route_params_w_failed_scid.clone(); router.expect_find_route(route_params_w_failed_scid, Ok(route_w_failed_scid)); router.expect_find_route(route_params.clone(), Ok(route.clone())); router.expect_find_route(route_params.clone(), Ok(route.clone())); @@ -3418,7 +3427,7 @@ mod tests { blinded_tail: None, } ], - route_params: Some(route_params), + route_params, }) ); diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 77684919821..e86245abc60 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -96,7 +96,7 @@ fn mpp_failure() { route.paths[1].hops[0].pubkey = node_c_id; route.paths[1].hops[0].short_channel_id = chan_2_id; route.paths[1].hops[1].short_channel_id = chan_4_id; - route.route_params.as_mut().unwrap().final_value_msat *= 2; + route.route_params.final_value_msat *= 2; let paths: &[&[_]] = &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]]; send_along_route_with_secret(&nodes[0], route, paths, 200_000, payment_hash, payment_secret); @@ -138,11 +138,11 @@ fn mpp_retry() { route.paths[1].hops[0].pubkey = node_c_id; route.paths[1].hops[0].short_channel_id = chan_2_update.contents.short_channel_id; route.paths[1].hops[1].short_channel_id = chan_4_update.contents.short_channel_id; - route.route_params.as_mut().unwrap().final_value_msat *= 2; + route.route_params.final_value_msat *= 2; // Initiate the MPP payment. let id = PaymentId(hash.0); - let mut route_params = route.route_params.clone().unwrap(); + let mut route_params = route.route_params.clone(); nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); let onion = RecipientOnionFields::secret_only(pay_secret, amt_msat * 2); @@ -192,7 +192,7 @@ fn mpp_retry() { // Check the remaining max total routing fee for the second attempt is 50_000 - 1_000 msat fee // used by the first path route_params.max_total_routing_fee_msat = Some(max_fee - 1_000); - route.route_params = Some(route_params.clone()); + route.route_params = route_params.clone(); nodes[0].router.expect_find_route(route_params, Ok(route)); expect_and_process_pending_htlcs(&nodes[0], false); check_added_monitors(&nodes[0], 1); @@ -265,7 +265,7 @@ fn mpp_retry_overpay() { // Initiate the payment. let id = PaymentId(hash.0); - let mut route_params = route.route_params.clone().unwrap(); + let mut route_params = route.route_params.clone(); nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); let onion = RecipientOnionFields::secret_only(pay_secret, amt_msat); @@ -321,7 +321,7 @@ fn mpp_retry_overpay() { // base fee, but not for overpaid value of the first try. route_params.max_total_routing_fee_msat.as_mut().map(|m| *m -= 1000); - route.route_params = Some(route_params.clone()); + route.route_params = route_params.clone(); nodes[0].router.expect_find_route(route_params, Ok(route)); nodes[0].node.process_pending_htlc_forwards(); @@ -373,12 +373,12 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool, keysend: bool) { route.paths[1].hops[0].pubkey = node_c_id; route.paths[1].hops[0].short_channel_id = chan_2_update.contents.short_channel_id; route.paths[1].hops[1].short_channel_id = chan_4_update.contents.short_channel_id; - route.route_params.as_mut().unwrap().final_value_msat *= 2; + route.route_params.final_value_msat *= 2; // Initiate the MPP payment. let onion = RecipientOnionFields::secret_only(payment_secret, 200_000); if keysend { - let route_params = route.route_params.clone().unwrap(); + let route_params = route.route_params.clone(); nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); nodes[0] .node @@ -668,8 +668,8 @@ fn test_reject_mpp_keysend_htlc_mismatching_secret() { route.paths[0].hops[1].short_channel_id = chan_3_id; let payment_id_0 = PaymentId(nodes[0].keys_manager.backing.get_secure_random_bytes()); - nodes[0].router.expect_find_route(route.route_params.clone().unwrap(), Ok(route.clone())); - let params = route.route_params.clone().unwrap(); + nodes[0].router.expect_find_route(route.route_params.clone(), Ok(route.clone())); + let params = route.route_params.clone(); let onion = RecipientOnionFields::spontaneous_empty(amount); let retry = Retry::Attempts(0); nodes[0].node.send_spontaneous_payment(preimage, onion, payment_id_0, params, retry).unwrap(); @@ -716,10 +716,10 @@ fn test_reject_mpp_keysend_htlc_mismatching_secret() { route.paths[0].hops[1].short_channel_id = chan_4_id; let payment_id_1 = PaymentId(nodes[0].keys_manager.backing.get_secure_random_bytes()); - nodes[0].router.expect_find_route(route.route_params.clone().unwrap(), Ok(route.clone())); + nodes[0].router.expect_find_route(route.route_params.clone(), Ok(route.clone())); let onion = RecipientOnionFields::spontaneous_empty(amount); - let params = route.route_params.clone().unwrap(); + let params = route.route_params.clone(); let retry = Retry::Attempts(0); nodes[0].node.send_spontaneous_payment(preimage, onion, payment_id_1, params, retry).unwrap(); check_added_monitors(&nodes[0], 1); @@ -859,7 +859,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { 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 = route.route_params.unwrap().clone(); + let route_params = route.route_params.clone(); let onion = RecipientOnionFields::secret_only(payment_secret, amt_msat); let id = PaymentId(payment_hash.0); nodes[0].node.send_payment(payment_hash, onion, id, route_params, Retry::Attempts(1)).unwrap(); @@ -2138,7 +2138,7 @@ fn claimed_send_payment_idempotent() { None, RecipientOnionFields::spontaneous_empty(100_000), payment_id, - route.route_params.clone().unwrap(), + route.route_params.clone(), Retry::Attempts(0), ); match send_result { @@ -2221,7 +2221,7 @@ fn abandoned_send_payment_idempotent() { None, RecipientOnionFields::spontaneous_empty(100_000), payment_id, - route.route_params.clone().unwrap(), + route.route_params.clone(), Retry::Attempts(0), ); match send_result { @@ -3224,7 +3224,7 @@ fn auto_retry_partial_failure() { blinded_tail: None, }, ], - route_params: Some(route_params.clone()), + route_params: route_params.clone(), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route)); @@ -3262,7 +3262,7 @@ fn auto_retry_partial_failure() { blinded_tail: None, }, ], - route_params: Some(retry_1_params.clone()), + route_params: retry_1_params.clone(), }; nodes[0].router.expect_find_route(retry_1_params.clone(), Ok(retry_1_route)); @@ -3286,7 +3286,7 @@ fn auto_retry_partial_failure() { }], blinded_tail: None, }], - route_params: Some(retry_2_params.clone()), + route_params: retry_2_params.clone(), }; nodes[0].router.expect_find_route(retry_2_params, Ok(retry_2_route)); @@ -3449,7 +3449,7 @@ fn auto_retry_zero_attempts_send_error() { }], blinded_tail: None, }], - route_params: Some(route_params.clone()), + route_params: route_params.clone(), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route)); @@ -3587,18 +3587,18 @@ fn retry_multi_path_single_failed_payment() { blinded_tail: None, }, ], - route_params: Some(route_params.clone()), + route_params: 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.route_params.clone().unwrap().payment_params; + let mut pay_params = route.route_params.clone().payment_params; pay_params.previously_failed_channels.push(chans[1].short_channel_id.unwrap()); let mut retry_params = RouteParameters::from_payment_params_and_value(pay_params, 100_000_000); retry_params.max_total_routing_fee_msat = None; - route.route_params = Some(retry_params.clone()); + route.route_params = retry_params.clone(); nodes[0].router.expect_find_route(retry_params, Ok(route.clone())); { @@ -3694,7 +3694,7 @@ fn immediate_retry_on_failure() { }], blinded_tail: None, }], - route_params: Some(route_params.clone()), + route_params: route_params.clone(), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); // On retry, split the payment across both channels. @@ -3705,7 +3705,7 @@ fn immediate_retry_on_failure() { let mut pay_params = route_params.payment_params.clone(); pay_params.previously_failed_channels.push(chans[0].short_channel_id.unwrap()); let retry_params = RouteParameters::from_payment_params_and_value(pay_params, amt_msat); - route.route_params = Some(retry_params.clone()); + route.route_params = retry_params.clone(); nodes[0].router.expect_find_route(retry_params, Ok(route.clone())); let onion = RecipientOnionFields::secret_only(payment_secret, amt_msat); @@ -3829,9 +3829,9 @@ fn no_extra_retries_on_back_to_back_fail() { blinded_tail: None, }, ], - route_params: Some(route_params.clone()), + route_params: route_params.clone(), }; - route.route_params.as_mut().unwrap().max_total_routing_fee_msat = None; + route.route_params.max_total_routing_fee_msat = None; nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); let mut second_payment_params = route_params.payment_params.clone(); second_payment_params.previously_failed_channels = vec![chan_2_scid, chan_2_scid]; @@ -3841,7 +3841,7 @@ fn no_extra_retries_on_back_to_back_fail() { let mut retry_params = RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat); retry_params.max_total_routing_fee_msat = None; - route.route_params = Some(retry_params.clone()); + route.route_params = retry_params.clone(); nodes[0].router.expect_find_route(retry_params, Ok(route.clone())); // We can't use the commitment_signed_dance macro helper because in this test we'll be sending @@ -4074,7 +4074,7 @@ fn test_simple_partial_retry() { blinded_tail: None, }, ], - route_params: Some(route_params.clone()), + route_params: route_params.clone(), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); @@ -4086,7 +4086,7 @@ fn test_simple_partial_retry() { let mut retry_params = RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat / 2); retry_params.max_total_routing_fee_msat = None; - route.route_params = Some(retry_params.clone()); + route.route_params = retry_params.clone(); nodes[0].router.expect_find_route(retry_params, Ok(route.clone())); // We can't use the commitment_signed_dance macro helper because in this test we'll be sending @@ -4290,7 +4290,7 @@ fn test_threaded_payment_retries() { blinded_tail: None, }, ], - route_params: Some(route_params.clone()), + route_params: route_params.clone(), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); @@ -4313,7 +4313,7 @@ fn test_threaded_payment_retries() { // from here on out, the retry `RouteParameters` amount will be amt/1000 route_params.final_value_msat /= 1000; - route.route_params = Some(route_params.clone()); + route.route_params = route_params.clone(); route.paths.pop(); let end_time = Instant::now() + Duration::from_secs(1); @@ -4367,7 +4367,7 @@ fn test_threaded_payment_retries() { previously_failed_channels.clone(); new_route_params.max_total_routing_fee_msat.as_mut().map(|m| *m -= 100_000); route.paths[0].hops[1].short_channel_id += 1; - route.route_params = Some(new_route_params.clone()); + route.route_params = new_route_params.clone(); nodes[0].router.expect_find_route(new_route_params, Ok(route.clone())); let bs_fail_updates = get_htlc_update_msgs(&nodes[1], &node_a_id); @@ -4773,7 +4773,7 @@ fn do_test_custom_tlvs(spontaneous: bool, even_tlvs: bool, known_tlvs: bool) { total_mpp_amount_msat: amt_msat, }; if spontaneous { - let params = route.route_params.unwrap(); + let params = route.route_params; let retry = Retry::Attempts(0); nodes[0].node.send_spontaneous_payment(Some(preimage), onion, id, params, retry).unwrap(); } else { @@ -4848,7 +4848,7 @@ fn test_retry_custom_tlvs() { // Initiate the payment let id = PaymentId(hash.0); - let mut route_params = route.route_params.clone().unwrap(); + let mut route_params = route.route_params.clone(); let custom_tlvs = vec![((1 << 16) + 1, vec![0x42u8; 16])]; let onion = RecipientOnionFields::secret_only(payment_secret, amt_msat); @@ -4888,7 +4888,7 @@ fn test_retry_custom_tlvs() { // Retry the payment and make sure it succeeds let chan_2_scid = chan_2_update.contents.short_channel_id; route_params.payment_params.previously_failed_channels.push(chan_2_scid); - route.route_params = Some(route_params.clone()); + route.route_params = route_params.clone(); nodes[0].router.expect_find_route(route_params, Ok(route)); nodes[0].node.process_pending_htlc_forwards(); check_added_monitors(&nodes[0], 1); @@ -5603,7 +5603,7 @@ fn remove_pending_outbounds_on_buggy_router() { // Extend the path by itself, essentially simulating route going through same channel twice let cloned_hops = route.paths[0].hops.clone(); route.paths[0].hops.extend_from_slice(&cloned_hops); - let route_params = route.route_params.clone().unwrap(); + let route_params = route.route_params.clone(); nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); // Send the payment with one retry allowed, but the payment should still fail @@ -5662,40 +5662,6 @@ fn remove_pending_outbound_probe_on_buggy_path() { assert!(nodes[0].node.list_recent_payments().is_empty()); } -#[test] -fn pay_route_without_params() { - // Make sure we can use ChannelManager::send_payment_with_route to pay a route where - // Route::route_parameters is None. - let chanmon_cfgs = create_chanmon_cfgs(2); - let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); - let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); - let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - - let node_b_id = nodes[1].node.get_our_node_id(); - - create_announced_chan_between_nodes(&nodes, 0, 1); - - let amt_msat = 10_000; - let payment_params = PaymentParameters::from_node_id(node_b_id, TEST_FINAL_CLTV) - .with_bolt11_features(nodes[1].node.bolt11_invoice_features()) - .unwrap(); - let (mut route, hash, preimage, payment_secret) = - get_route_and_payment_hash!(nodes[0], nodes[1], payment_params, amt_msat); - route.route_params.take(); - - let onion = RecipientOnionFields::secret_only(payment_secret, amt_msat); - let id = PaymentId(hash.0); - nodes[0].node.send_payment_with_route(route, hash, onion, id).unwrap(); - - check_added_monitors(&nodes[0], 1); - let mut events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 1); - let node_1_msgs = remove_first_msg_event_to_node(&node_b_id, &mut events); - let path = &[&nodes[1]]; - pass_along_path(&nodes[0], path, amt_msat, hash, Some(payment_secret), node_1_msgs, true, None); - claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], &[path], preimage)); -} - #[test] fn max_out_mpp_path() { // In this setup, the sender is attempting to route an MPP payment split across the two channels @@ -5984,7 +5950,7 @@ fn bolt11_multi_node_mpp_with_retry() { // First route for A: same path but with fee_msat=0 at C to trigger a forwarding failure let mut first_route = route.clone(); first_route.paths[0].hops[0].fee_msat = 0; - first_route.route_params = Some(route_params.clone()); + first_route.route_params = route_params.clone(); nodes[0].router.expect_find_route(route_params.clone(), Ok(first_route)); // Retry route for A: the natural route with correct fees (will succeed) @@ -5995,7 +5961,7 @@ fn bolt11_multi_node_mpp_with_retry() { payment_params: retry_payment_params, max_total_routing_fee_msat: route_params.max_total_routing_fee_msat, }; - route.route_params = Some(retry_route_params.clone()); + route.route_params = retry_route_params.clone(); nodes[0].router.expect_find_route(retry_route_params, Ok(route)); // Node A pays 60_000 msat (part of the total) with retry enabled diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 01889b2ea60..1c7c1326ede 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -682,10 +682,7 @@ pub struct Route { /// The `route_params` parameter passed to [`find_route`]. /// /// This is used by `ChannelManager` to track information which may be required for retries. - /// - /// Will be `None` for objects serialized with LDK versions prior to 0.0.117. This field will - /// soon move to being required and must always be set. - pub route_params: Option, + pub route_params: RouteParameters, } impl Route { @@ -698,8 +695,8 @@ impl Route { /// [`htlc_minimum_msat`]: https://github.com/lightning/bolts/blob/master/07-routing-gossip.md#the-channel_update-message #[rustfmt::skip] pub fn get_total_fees(&self) -> u64 { - let overpaid_value_msat = self.route_params.as_ref() - .map_or(0, |p| self.get_total_amount().saturating_sub(p.final_value_msat)); + let overpaid_value_msat = + self.get_total_amount().saturating_sub(self.route_params.final_value_msat); overpaid_value_msat + self.paths.iter().map(|path| path.fee_msat()).sum::() } @@ -714,105 +711,102 @@ impl Route { } pub(crate) fn debug_assert_route_meets_params(&self, logger: L) -> Result<(), ()> { - if let Some(route_params) = self.route_params.as_ref() { - // Check that we actually pay less than the max fee we set. - if let Some(max_total_fee) = route_params.max_total_routing_fee_msat { - let total_fee = self.get_total_fees(); - if total_fee > max_total_fee { - let err = format!("Router returned an attempt to pay with a higher fee ({total_fee}msat) than we allowed ({max_total_fee}msat). Your router is critically buggy!"); - debug_assert!(false, "{}", err); - log_error!(logger, "{}", err); - return Err(()); - } + let route_params = &self.route_params; + // Check that we actually pay less than the max fee we set. + if let Some(max_total_fee) = route_params.max_total_routing_fee_msat { + let total_fee = self.get_total_fees(); + if total_fee > max_total_fee { + let err = format!("Router returned an attempt to pay with a higher fee ({total_fee}msat) than we allowed ({max_total_fee}msat). Your router is critically buggy!"); + debug_assert!(false, "{}", err); + log_error!(logger, "{}", err); + return Err(()); } + } + + if self.paths.is_empty() { + let err = "Selected route had no paths. Your router is buggy!"; + debug_assert!(false, "{}", err); + log_error!(logger, "{}", err); + return Err(()); + } - if self.paths.is_empty() { - let err = "Selected route had no paths. Your router is buggy!"; + for path in self.paths.iter() { + if path.hops.is_empty() { + let err = "Unusable path in route (path.hops.len() must be at least 1)"; debug_assert!(false, "{}", err); log_error!(logger, "{}", err); return Err(()); } - for path in self.paths.iter() { - if path.hops.is_empty() { - let err = "Unusable path in route (path.hops.len() must be at least 1)"; + let total_cltv_delta = path.total_cltv_expiry_delta(); + if total_cltv_delta > route_params.payment_params.max_total_cltv_expiry_delta { + let err = format!( + "Path had a total CLTV of {total_cltv_delta} which is greater than the maximum we're allowed {}", + route_params.payment_params.max_total_cltv_expiry_delta, + ); + debug_assert!(false, "{}", err); + log_error!(logger, "{}", err); + return Err(()); + } + + if path.hops.len() > route_params.payment_params.max_path_length.into() { + let err = format!( + "Path had a length of {}, which is greater than the maximum we're allowed ({})", + path.hops.len(), + route_params.payment_params.max_path_length, + ); + #[cfg(any(test, feature = "_test_utils"))] + debug_assert!(false, "{}", err); + log_error!(logger, "{}", err); + // This is a bug, but there's not a material safety risk to making this + // payment, so we don't bother to error here. + } + + if let Some(tail) = &path.blinded_tail { + let trampoline_cltv_sum: u32 = + tail.trampoline_hops.iter().map(|hop| hop.cltv_expiry_delta).sum(); + let last_hop_cltv_delta = path.hops.last().unwrap().cltv_expiry_delta; + if !tail.trampoline_hops.is_empty() && trampoline_cltv_sum != last_hop_cltv_delta { + let err = format!( + "Path had a total trampoline CLTV of {trampoline_cltv_sum}, which is not equal to the total last-hop CLTV delta of {last_hop_cltv_delta}" + ); debug_assert!(false, "{}", err); log_error!(logger, "{}", err); - return Err(()); } - - let total_cltv_delta = path.total_cltv_expiry_delta(); - if total_cltv_delta > route_params.payment_params.max_total_cltv_expiry_delta { + let last_trampoline_cltv_opt = + tail.trampoline_hops.last().map(|h| h.cltv_expiry_delta); + let last_trampoline_cltv = last_trampoline_cltv_opt.unwrap_or(u32::MAX); + if tail.excess_final_cltv_expiry_delta > last_trampoline_cltv { let err = format!( - "Path had a total CLTV of {total_cltv_delta} which is greater than the maximum we're allowed {}", - route_params.payment_params.max_total_cltv_expiry_delta, + "Last trampoline CLTV of {last_trampoline_cltv} is less than the excess blinded path cltv of {}", + tail.excess_final_cltv_expiry_delta ); debug_assert!(false, "{}", err); log_error!(logger, "{}", err); - return Err(()); } - - if path.hops.len() > route_params.payment_params.max_path_length.into() { + if tail.excess_final_cltv_expiry_delta > last_hop_cltv_delta { let err = format!( - "Path had a length of {}, which is greater than the maximum we're allowed ({})", - path.hops.len(), - route_params.payment_params.max_path_length, + "Last path hop CLTV of {last_hop_cltv_delta} is less than the excess blinded path cltv of {}", + tail.excess_final_cltv_expiry_delta ); - #[cfg(any(test, feature = "_test_utils"))] debug_assert!(false, "{}", err); log_error!(logger, "{}", err); - // This is a bug, but there's not a material safety risk to making this - // payment, so we don't bother to error here. - } - - if let Some(tail) = &path.blinded_tail { - let trampoline_cltv_sum: u32 = - tail.trampoline_hops.iter().map(|hop| hop.cltv_expiry_delta).sum(); - let last_hop_cltv_delta = path.hops.last().unwrap().cltv_expiry_delta; - if !tail.trampoline_hops.is_empty() - && trampoline_cltv_sum != last_hop_cltv_delta - { - let err = format!( - "Path had a total trampoline CLTV of {trampoline_cltv_sum}, which is not equal to the total last-hop CLTV delta of {last_hop_cltv_delta}" - ); - debug_assert!(false, "{}", err); - log_error!(logger, "{}", err); - } - let last_trampoline_cltv_opt = - tail.trampoline_hops.last().map(|h| h.cltv_expiry_delta); - let last_trampoline_cltv = last_trampoline_cltv_opt.unwrap_or(u32::MAX); - if tail.excess_final_cltv_expiry_delta > last_trampoline_cltv { - let err = format!( - "Last trampoline CLTV of {last_trampoline_cltv} is less than the excess blinded path cltv of {}", - tail.excess_final_cltv_expiry_delta - ); - debug_assert!(false, "{}", err); - log_error!(logger, "{}", err); - } - if tail.excess_final_cltv_expiry_delta > last_hop_cltv_delta { - let err = format!( - "Last path hop CLTV of {last_hop_cltv_delta} is less than the excess blinded path cltv of {}", - tail.excess_final_cltv_expiry_delta - ); - debug_assert!(false, "{}", err); - log_error!(logger, "{}", err); - } } } + } - // Test that we don't contain any "extra" MPP parts - while we're allowed to overshoot - // the `final_value_msat` specified in the `route_params`, we aren't allowed to have - // any MPP parts which aren't needed to meet `route_params.final_value_msat`. - let min_mpp_part = self.paths.iter().map(|h| h.final_value_msat()).min().unwrap_or(0); - if self.get_total_amount() - min_mpp_part >= route_params.final_value_msat { - let err = format!( - "Router returned an attempt to include more MPP parts than needed. The smallest MPP part ({min_mpp_part}msat) was not needed for a payment of {}msat. Your router is critically buggy!", - route_params.final_value_msat - ); - debug_assert!(false, "{}", err); - log_error!(logger, "{}", err); - return Err(()); - } + // Test that we don't contain any "extra" MPP parts - while we're allowed to overshoot + // the `final_value_msat` specified in the `route_params`, we aren't allowed to have + // any MPP parts which aren't needed to meet `route_params.final_value_msat`. + let min_mpp_part = self.paths.iter().map(|h| h.final_value_msat()).min().unwrap_or(0); + if self.get_total_amount() - min_mpp_part >= route_params.final_value_msat { + let err = format!( + "Router returned an attempt to include more MPP parts than needed. The smallest MPP part ({min_mpp_part}msat) was not needed for a payment of {}msat. Your router is critically buggy!", + route_params.final_value_msat + ); + debug_assert!(false, "{}", err); + log_error!(logger, "{}", err); + return Err(()); } Ok(()) @@ -850,12 +844,10 @@ impl Writeable for Route { } else if !blinded_tails.is_empty() { blinded_tails.push(None); } } write_tlv_fields!(writer, { - // 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), + (1, self.route_params.payment_params, required), (2, blinded_tails, optional_vec), - (3, self.route_params.as_ref().map(|p| p.final_value_msat), option), - (5, self.route_params.as_ref().and_then(|p| p.max_total_routing_fee_msat), option), + (3, self.route_params.final_value_msat, required), + (5, self.route_params.max_total_routing_fee_msat, option), }); Ok(()) } @@ -881,9 +873,9 @@ impl Readable for Route { paths.push(Path { hops, blinded_tail: None }); } _init_and_read_len_prefixed_tlv_fields!(reader, { - (1, payment_params, (option: ReadableArgs, min_final_cltv_expiry_delta)), + (1, payment_params, (required: ReadableArgs, min_final_cltv_expiry_delta)), (2, blinded_tails, optional_vec), - (3, final_value_msat, option), + (3, final_value_msat, required), (5, max_total_routing_fee_msat, option) }); let blinded_tails = blinded_tails.unwrap_or(Vec::new()); @@ -894,12 +886,10 @@ impl Readable for Route { } } - // 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, max_total_routing_fee_msat }) - } - _ => None, + let route_params = RouteParameters { + payment_params: payment_params.0.unwrap(), + final_value_msat: final_value_msat.0.unwrap(), + max_total_routing_fee_msat, }; Ok(Route { paths, route_params }) @@ -3908,7 +3898,7 @@ pub(crate) fn get_route( } } - let route = Route { paths, route_params: Some(route_params.clone()) }; + let route = Route { paths, route_params: route_params.clone() }; // Make sure we would never create a route whose total fees exceed max_total_routing_fee_msat. if let Some(max_total_routing_fee_msat) = route_params.max_total_routing_fee_msat { @@ -7504,7 +7494,7 @@ mod tests { short_channel_id: 0, fee_msat: 225, cltv_expiry_delta: 0, maybe_announced_channel: true, }, ], blinded_tail: None }], - route_params: None, + route_params: RouteParameters::from_payment_params_and_value(PaymentParameters::from_node_id(ln_test_utils::pubkey(42), 0), 225), }; assert_eq!(route.get_total_fees(), 250); @@ -7537,7 +7527,7 @@ mod tests { short_channel_id: 0, fee_msat: 150, cltv_expiry_delta: 0, maybe_announced_channel: true, }, ], blinded_tail: None }], - route_params: None, + route_params: RouteParameters::from_payment_params_and_value(PaymentParameters::from_node_id(ln_test_utils::pubkey(42), 0), 300), }; assert_eq!(route.get_total_fees(), 200); @@ -7549,7 +7539,13 @@ 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(), route_params: None }; + let route = Route { + paths: Vec::new(), + route_params: RouteParameters::from_payment_params_and_value( + PaymentParameters::from_node_id(ln_test_utils::pubkey(42), 0), + 0, + ), + }; assert_eq!(route.get_total_fees(), 0); assert_eq!(route.get_total_amount(), 0); @@ -8144,7 +8140,7 @@ mod tests { cltv_expiry_delta: 0, maybe_announced_channel: true, }], blinded_tail: None }], - route_params: None, + route_params: RouteParameters::from_payment_params_and_value(PaymentParameters::from_node_id(ln_test_utils::pubkey(42), 0), 200), }; let encoded_route = route.encode(); let decoded_route: Route = Readable::read(&mut Cursor::new(&encoded_route[..])).unwrap(); @@ -8340,7 +8336,7 @@ mod tests { excess_final_cltv_expiry_delta: 0, final_value_msat: 200, }), - }], route_params: None}; + }], route_params: RouteParameters::from_payment_params_and_value(PaymentParameters::from_node_id(ln_test_utils::pubkey(42), 0), 200)}; let payment_params = PaymentParameters::from_node_id(ln_test_utils::pubkey(47), 18); let (_, network_graph, _, _, _) = build_line_graph(); diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 892c9f4169d..7af41c19586 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -240,7 +240,7 @@ impl<'a> Router for TestRouter<'a> { assert_eq!(find_route_query, *params); if let Some(res) = find_route_res { if let Ok(ref route) = res { - assert_eq!(route.route_params, Some(find_route_query)); + assert_eq!(route.route_params, find_route_query); let scorer = self.scorer.read().unwrap(); let scorer = ScorerAccountingForInFlightHtlcs::new(scorer, &inflight_htlcs); for path in &route.paths { diff --git a/pending_changelog/route-params-required.txt b/pending_changelog/route-params-required.txt new file mode 100644 index 00000000000..ea47deb1ad6 --- /dev/null +++ b/pending_changelog/route-params-required.txt @@ -0,0 +1,4 @@ +# Backwards Compatibility + * `Route`s serialized by LDK versions prior to 0.0.117 can no longer be + deserialized, as parts of the now-required `Route::route_params` were not + written by those versions.