From 90b1e7d144b267bd102e4a858a1a6b4d1a9c8967 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 27 Nov 2024 14:39:03 -0500 Subject: [PATCH 1/4] Mark deprecated send_payment_with_route as test and fuzz only This method has been deprecated for several versions in favor of ChannelManager::send_payment, and we want to remove it from the public API entirely prior to the 0.1 release. However, >150 tests use it so put off removing the method entirely. --- lightning/src/ln/channelmanager.rs | 89 ++++++++++++----------- lightning/src/ln/functional_test_utils.rs | 35 ++------- lightning/src/ln/functional_tests.rs | 8 +- lightning/src/ln/outbound_payment.rs | 9 +-- 4 files changed, 62 insertions(+), 79 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index a58863ab1..0051765c0 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4615,14 +4615,22 @@ where } } - /// Sends a payment along a given route. - /// - /// This method is *DEPRECATED*, use [`Self::send_payment`] instead. If you wish to fix the - /// route for a payment, do so by matching the [`PaymentId`] passed to - /// [`Router::find_route_with_id`]. - /// - /// Value parameters are provided via the last hop in route, see documentation for [`RouteHop`] - /// fields for more info. + // Deprecated send method, for testing use [`Self::send_payment`] and + // [`TestRouter::expect_find_route`] instead. + // + // [`TestRouter::expect_find_route`]: crate::util::test_utils::TestRouter::expect_find_route + #[cfg(any(test, fuzzing))] + pub fn send_payment_with_route(&self, route: Route, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, payment_id: PaymentId) -> Result<(), PaymentSendFailure> { + let best_block_height = self.best_block.read().unwrap().height; + let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); + self.pending_outbound_payments + .send_payment_with_route(&route, payment_hash, recipient_onion, payment_id, + &self.entropy_source, &self.node_signer, best_block_height, + |args| self.send_payment_along_path(args)) + } + + /// Sends a payment to the route found using the provided [`RouteParameters`], retrying failed + /// payment paths based on the provided `Retry`. /// /// May generate [`UpdateHTLCs`] message(s) event on success, which should be relayed (e.g. via /// [`PeerManager::process_events`]). @@ -4630,9 +4638,9 @@ where /// # Avoiding Duplicate Payments /// /// If a pending payment is currently in-flight with the same [`PaymentId`] provided, this - /// method will error with an [`APIError::InvalidRoute`]. Note, however, that once a payment - /// is no longer pending (either via [`ChannelManager::abandon_payment`], or handling of an - /// [`Event::PaymentSent`] or [`Event::PaymentFailed`]) LDK will not stop you from sending a + /// method will error with [`RetryableSendFailure::DuplicatePayment`]. Note, however, that once a + /// payment is no longer pending (either via [`ChannelManager::abandon_payment`], or handling of + /// an [`Event::PaymentSent`] or [`Event::PaymentFailed`]) LDK will not stop you from sending a /// second payment with the same [`PaymentId`]. /// /// Thus, in order to ensure duplicate payments are not sent, you should implement your own @@ -4646,43 +4654,18 @@ where /// using [`ChannelMonitorUpdateStatus::InProgress`]), the payment may be lost on restart. See /// [`ChannelManager::list_recent_payments`] for more information. /// - /// # Possible Error States on [`PaymentSendFailure`] + /// Routes are automatically found using the [`Router] provided on startup. To fix a route for a + /// particular payment, match the [`PaymentId`] passed to [`Router::find_route_with_id`]. /// - /// Each path may have a different return value, and [`PaymentSendFailure`] may return a `Vec` with - /// each entry matching the corresponding-index entry in the route paths, see - /// [`PaymentSendFailure`] for more info. - /// - /// In general, a path may raise: - /// * [`APIError::InvalidRoute`] when an invalid route or forwarding parameter (cltv_delta, fee, - /// node public key) is specified. - /// * [`APIError::ChannelUnavailable`] if the next-hop channel is not available as it has been - /// closed, doesn't exist, or the peer is currently disconnected. - /// * [`APIError::MonitorUpdateInProgress`] if a new monitor update failure prevented sending the - /// relevant updates. - /// - /// Note that depending on the type of the [`PaymentSendFailure`] the HTLC may have been - /// irrevocably committed to on our end. In such a case, do NOT retry the payment with a - /// different route unless you intend to pay twice! - /// - /// [`RouteHop`]: crate::routing::router::RouteHop /// [`Event::PaymentSent`]: events::Event::PaymentSent /// [`Event::PaymentFailed`]: events::Event::PaymentFailed /// [`UpdateHTLCs`]: events::MessageSendEvent::UpdateHTLCs /// [`PeerManager::process_events`]: crate::ln::peer_handler::PeerManager::process_events /// [`ChannelMonitorUpdateStatus::InProgress`]: crate::chain::ChannelMonitorUpdateStatus::InProgress - #[cfg_attr(not(any(test, feature = "_test_utils")), deprecated(note = "Use `send_payment` instead"))] - pub fn send_payment_with_route(&self, route: Route, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, payment_id: PaymentId) -> Result<(), PaymentSendFailure> { - let best_block_height = self.best_block.read().unwrap().height; - let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); - self.pending_outbound_payments - .send_payment_with_route(&route, payment_hash, recipient_onion, payment_id, - &self.entropy_source, &self.node_signer, best_block_height, - |args| self.send_payment_along_path(args)) - } - - /// Similar to [`ChannelManager::send_payment_with_route`], but will automatically find a route based on - /// `route_params` and retry failed payment paths based on `retry_strategy`. - pub fn send_payment(&self, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, payment_id: PaymentId, route_params: RouteParameters, retry_strategy: Retry) -> Result<(), RetryableSendFailure> { + pub fn send_payment( + &self, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, payment_id: PaymentId, + route_params: RouteParameters, retry_strategy: Retry + ) -> Result<(), RetryableSendFailure> { let best_block_height = self.best_block.read().unwrap().height; let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); self.pending_outbound_payments @@ -4897,8 +4880,26 @@ where /// would be able to guess -- otherwise, an intermediate node may claim the payment and it will /// never reach the recipient. /// - /// See [`send_payment`] documentation for more details on the return value of this function - /// and idempotency guarantees provided by the [`PaymentId`] key. + /// See [`send_payment`] documentation for more details on the idempotency guarantees provided by + /// the [`PaymentId`] key. + /// + /// # Possible Error States on [`PaymentSendFailure`] + /// + /// Each path may have a different return value, and [`PaymentSendFailure`] may return a `Vec` with + /// each entry matching the corresponding-index entry in the route paths, see + /// [`PaymentSendFailure`] for more info. + /// + /// In general, a path may raise: + /// * [`APIError::InvalidRoute`] when an invalid route or forwarding parameter (cltv_delta, fee, + /// node public key) is specified. + /// * [`APIError::ChannelUnavailable`] if the next-hop channel is not available as it has been + /// closed, doesn't exist, or the peer is currently disconnected. + /// * [`APIError::MonitorUpdateInProgress`] if a new monitor update failure prevented sending the + /// relevant updates. + /// + /// Note that depending on the type of the [`PaymentSendFailure`] the HTLC may have been + /// irrevocably committed to on our end. In such a case, do NOT retry the payment with a + /// different route unless you intend to pay twice! /// /// Similar to regular payments, you MUST NOT reuse a `payment_preimage` value. See /// [`send_payment`] for more information about the risks of duplicate preimage usage. diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 6c6b24bce..fbf751e94 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -17,17 +17,17 @@ use crate::events::{ClaimedHTLC, ClosureReason, Event, HTLCDestination, MessageS use crate::events::bump_transaction::{BumpTransactionEvent, BumpTransactionEventHandler, Wallet, WalletSource}; use crate::ln::types::ChannelId; use crate::types::payment::{PaymentPreimage, PaymentHash, PaymentSecret}; -use crate::ln::channelmanager::{AChannelManager, ChainParameters, ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure, RecipientOnionFields, PaymentId, MIN_CLTV_EXPIRY_DELTA}; +use crate::ln::channelmanager::{AChannelManager, ChainParameters, ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, RecipientOnionFields, PaymentId, MIN_CLTV_EXPIRY_DELTA}; use crate::types::features::InitFeatures; use crate::ln::msgs; use crate::ln::msgs::{ChannelMessageHandler, OnionMessageHandler, RoutingMessageHandler}; +use crate::ln::outbound_payment::Retry; use crate::ln::peer_handler::IgnoringMessageHandler; use crate::onion_message::messenger::OnionMessenger; use crate::routing::gossip::{P2PGossipSync, NetworkGraph, NetworkUpdate}; use crate::routing::router::{self, PaymentParameters, Route, RouteParameters}; use crate::sign::{EntropySource, RandomBytes}; use crate::util::config::{UserConfig, MaxDustHTLCExposure}; -use crate::util::errors::APIError; #[cfg(test)] use crate::util::logger::Logger; use crate::util::scid_utils; @@ -2600,8 +2600,11 @@ pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>( pub fn send_along_route_with_secret<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route, expected_paths: &[&[&Node<'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.node.send_payment_with_route(route, our_payment_hash, - RecipientOnionFields::secret_only(our_payment_secret), payment_id).unwrap(); + origin_node.router.expect_find_route(route.route_params.clone().unwrap(), Ok(route.clone())); + origin_node.node.send_payment( + our_payment_hash, RecipientOnionFields::secret_only(our_payment_secret), payment_id, + route.route_params.unwrap(), Retry::Attempts(0) + ).unwrap(); check_added_monitors!(origin_node, expected_paths.len()); pass_along_route(origin_node, expected_paths, recv_value, our_payment_hash, our_payment_secret); payment_id @@ -3103,30 +3106,6 @@ pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: (res.0, res.1, res.2, res.3) } -pub fn route_over_limit<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) { - let payment_params = PaymentParameters::from_node_id(expected_route.last().unwrap().node.get_our_node_id(), TEST_FINAL_CLTV) - .with_bolt11_features(expected_route.last().unwrap().node.bolt11_invoice_features()).unwrap(); - let route_params = RouteParameters::from_payment_params_and_value(payment_params, recv_value); - let network_graph = origin_node.network_graph.read_only(); - let scorer = test_utils::TestScorer::new(); - let seed = [0u8; 32]; - let keys_manager = test_utils::TestKeysInterface::new(&seed, Network::Testnet); - let random_seed_bytes = keys_manager.get_secure_random_bytes(); - let route = router::get_route(&origin_node.node.get_our_node_id(), &route_params, &network_graph, - None, origin_node.logger, &scorer, &Default::default(), &random_seed_bytes).unwrap(); - assert_eq!(route.paths.len(), 1); - assert_eq!(route.paths[0].hops.len(), expected_route.len()); - for (node, hop) in expected_route.iter().zip(route.paths[0].hops.iter()) { - assert_eq!(hop.pubkey, node.node.get_our_node_id()); - } - - let (_, our_payment_hash, our_payment_secret) = get_payment_preimage_hash!(expected_route.last().unwrap()); - unwrap_send_err!(origin_node.node.send_payment_with_route(route, our_payment_hash, - RecipientOnionFields::secret_only(our_payment_secret), PaymentId(our_payment_hash.0)), - true, APIError::ChannelUnavailable { ref err }, - assert!(err.contains("Cannot send value that would put us over the max HTLC value in flight our peer will accept"))); -} - pub fn send_payment<'a, 'b, 'c>(origin: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret, PaymentId) { let res = route_payment(&origin, expected_route, recv_value); claim_payment(&origin, expected_route, res.0); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index dc8edacb8..5749b46fe 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1092,8 +1092,12 @@ 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_params = PaymentParameters::from_node_id( + nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV + ).with_bolt11_features(nodes[1].node.bolt11_invoice_features()).unwrap(); + let route_params = RouteParameters::from_payment_params_and_value(payment_params, 1000000); let payment_preimage_1 = send_along_route(&nodes[1], - Route { paths: vec![Path { hops, blinded_tail: None }], route_params: None }, + Route { paths: vec![Path { hops, blinded_tail: None }], route_params: Some(route_params.clone()) }, &vec!(&nodes[2], &nodes[3], &nodes[1])[..], 1000000).0; let mut hops = Vec::with_capacity(3); @@ -1127,7 +1131,7 @@ 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 }], route_params: None }, + Route { paths: vec![Path { hops, blinded_tail: None }], route_params: Some(route_params) }, &vec!(&nodes[3], &nodes[2], &nodes[1])[..], 1000000).1; // Claim the rebalances... diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 1b546ac72..e8a550215 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -478,11 +478,9 @@ pub enum RetryableSendFailure { OnionPacketSizeExceeded, } -/// If a payment fails to send with [`ChannelManager::send_payment_with_route`], it can be in one -/// of several states. This enum is returned as the Err() type describing which state the payment -/// is in, see the description of individual enum states for more. -/// -/// [`ChannelManager::send_payment_with_route`]: crate::ln::channelmanager::ChannelManager::send_payment_with_route +/// If a payment fails to send to a route, it can be in one of several states. This enum is returned +/// as the Err() type describing which state the payment is in, see the description of individual +/// enum states for more. #[derive(Clone, Debug, PartialEq, Eq)] pub enum PaymentSendFailure { /// A parameter which was passed to send_payment was invalid, preventing us from attempting to @@ -776,6 +774,7 @@ impl OutboundPayments { best_block_height, logger, pending_events, &send_payment_along_path) } + #[cfg(any(test, fuzzing))] pub(super) fn send_payment_with_route( &self, route: &Route, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, payment_id: PaymentId, entropy_source: &ES, node_signer: &NS, best_block_height: u32, From 351efc852ed063d9ec9b3716cc31327e737d953d Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 27 Nov 2024 15:50:27 -0500 Subject: [PATCH 2/4] Remove support for specifying route with send_spontaneous_payment The old API is confusing and we want to remove it for 0.1. --- lightning/src/ln/blinded_payment_tests.rs | 6 +- lightning/src/ln/channelmanager.rs | 81 ++++++++-------------- lightning/src/ln/outbound_payment.rs | 27 -------- lightning/src/ln/payment_tests.rs | 84 ++++++++++------------- lightning/src/ln/reload_tests.rs | 2 +- 5 files changed, 72 insertions(+), 128 deletions(-) diff --git a/lightning/src/ln/blinded_payment_tests.rs b/lightning/src/ln/blinded_payment_tests.rs index 302dc87a1..5af098a68 100644 --- a/lightning/src/ln/blinded_payment_tests.rs +++ b/lightning/src/ln/blinded_payment_tests.rs @@ -1235,7 +1235,7 @@ fn blinded_keysend() { nodes.iter().skip(1).map(|n| n.node.get_our_node_id()).collect(), &[&chan_upd_1_2], &chanmon_cfgs[2].keys_manager); - let payment_hash = nodes[0].node.send_spontaneous_payment_with_retry(Some(keysend_preimage), RecipientOnionFields::spontaneous_empty(), PaymentId(keysend_preimage.0), route_params, Retry::Attempts(0)).unwrap(); + let payment_hash = nodes[0].node.send_spontaneous_payment(Some(keysend_preimage), RecipientOnionFields::spontaneous_empty(), PaymentId(keysend_preimage.0), route_params, Retry::Attempts(0)).unwrap(); check_added_monitors(&nodes[0], 1); let expected_route: &[&[&Node]] = &[&[&nodes[1], &nodes[2]]]; @@ -1289,7 +1289,7 @@ fn blinded_mpp_keysend() { RouteParameters::from_payment_params_and_value(pay_params, amt_msat) }; - let payment_hash = nodes[0].node.send_spontaneous_payment_with_retry(Some(keysend_preimage), RecipientOnionFields::spontaneous_empty(), PaymentId(keysend_preimage.0), route_params, Retry::Attempts(0)).unwrap(); + let payment_hash = nodes[0].node.send_spontaneous_payment(Some(keysend_preimage), RecipientOnionFields::spontaneous_empty(), PaymentId(keysend_preimage.0), route_params, Retry::Attempts(0)).unwrap(); check_added_monitors!(nodes[0], 2); let expected_route: &[&[&Node]] = &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]]; @@ -1327,7 +1327,7 @@ fn invalid_keysend_payment_secret() { &chanmon_cfgs[2].keys_manager ); - let payment_hash = nodes[0].node.send_spontaneous_payment_with_retry(Some(keysend_preimage), RecipientOnionFields::spontaneous_empty(), PaymentId(keysend_preimage.0), route_params, Retry::Attempts(0)).unwrap(); + let payment_hash = nodes[0].node.send_spontaneous_payment(Some(keysend_preimage), RecipientOnionFields::spontaneous_empty(), PaymentId(keysend_preimage.0), route_params, Retry::Attempts(0)).unwrap(); check_added_monitors(&nodes[0], 1); let expected_route: &[&[&Node]] = &[&[&nodes[1], &nodes[2]]]; diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 0051765c0..8b9c5ce69 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -53,7 +53,9 @@ use crate::ln::channel_state::ChannelDetails; use crate::types::features::{Bolt12InvoiceFeatures, ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures}; #[cfg(any(feature = "_test_utils", test))] use crate::types::features::Bolt11InvoiceFeatures; -use crate::routing::router::{BlindedTail, InFlightHtlcs, Path, Payee, PaymentParameters, Route, RouteParameters, Router}; +use crate::routing::router::{BlindedTail, InFlightHtlcs, Path, Payee, PaymentParameters, RouteParameters, Router}; +#[cfg(any(test, fuzzing))] +use crate::routing::router::Route; use crate::ln::onion_payment::{check_incoming_htlc_cltv, create_recv_pending_htlc_info, create_fwd_pending_htlc_info, decode_incoming_update_add_htlc_onion, InboundHTLCErr, NextPacketDetails}; use crate::ln::msgs; use crate::ln::onion_utils; @@ -4880,47 +4882,21 @@ where /// would be able to guess -- otherwise, an intermediate node may claim the payment and it will /// never reach the recipient. /// - /// See [`send_payment`] documentation for more details on the idempotency guarantees provided by - /// the [`PaymentId`] key. - /// - /// # Possible Error States on [`PaymentSendFailure`] - /// - /// Each path may have a different return value, and [`PaymentSendFailure`] may return a `Vec` with - /// each entry matching the corresponding-index entry in the route paths, see - /// [`PaymentSendFailure`] for more info. - /// - /// In general, a path may raise: - /// * [`APIError::InvalidRoute`] when an invalid route or forwarding parameter (cltv_delta, fee, - /// node public key) is specified. - /// * [`APIError::ChannelUnavailable`] if the next-hop channel is not available as it has been - /// closed, doesn't exist, or the peer is currently disconnected. - /// * [`APIError::MonitorUpdateInProgress`] if a new monitor update failure prevented sending the - /// relevant updates. - /// - /// Note that depending on the type of the [`PaymentSendFailure`] the HTLC may have been - /// irrevocably committed to on our end. In such a case, do NOT retry the payment with a - /// different route unless you intend to pay twice! - /// /// Similar to regular payments, you MUST NOT reuse a `payment_preimage` value. See /// [`send_payment`] for more information about the risks of duplicate preimage usage. /// - /// [`send_payment`]: Self::send_payment - pub fn send_spontaneous_payment(&self, route: &Route, payment_preimage: Option, recipient_onion: RecipientOnionFields, payment_id: PaymentId) -> Result { - let best_block_height = self.best_block.read().unwrap().height; - let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); - self.pending_outbound_payments.send_spontaneous_payment_with_route( - route, payment_preimage, recipient_onion, payment_id, &self.entropy_source, - &self.node_signer, best_block_height, |args| self.send_payment_along_path(args)) - } - - /// Similar to [`ChannelManager::send_spontaneous_payment`], but will automatically find a route - /// based on `route_params` and retry failed payment paths based on `retry_strategy`. + /// See [`send_payment`] documentation for more details on the idempotency guarantees provided by + /// the [`PaymentId`] key. /// /// See [`PaymentParameters::for_keysend`] for help in constructing `route_params` for spontaneous /// payments. /// + /// [`send_payment`]: Self::send_payment /// [`PaymentParameters::for_keysend`]: crate::routing::router::PaymentParameters::for_keysend - pub fn send_spontaneous_payment_with_retry(&self, payment_preimage: Option, recipient_onion: RecipientOnionFields, payment_id: PaymentId, route_params: RouteParameters, retry_strategy: Retry) -> Result { + pub fn send_spontaneous_payment( + &self, payment_preimage: Option, recipient_onion: RecipientOnionFields, + payment_id: PaymentId, route_params: RouteParameters, retry_strategy: Retry + ) -> Result { let best_block_height = self.best_block.read().unwrap().height; let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); self.pending_outbound_payments.send_spontaneous_payment(payment_preimage, recipient_onion, @@ -14330,6 +14306,7 @@ mod tests { use crate::ln::functional_test_utils::*; use crate::ln::msgs::{self, ErrorAction}; use crate::ln::msgs::ChannelMessageHandler; + use crate::ln::outbound_payment::Retry; use crate::prelude::*; use crate::routing::router::{PaymentParameters, RouteParameters, find_route}; use crate::util::errors::APIError; @@ -14447,8 +14424,10 @@ mod tests { pass_along_path(&nodes[0], &[&nodes[1]], 200_000, our_payment_hash, Some(payment_secret), events.drain(..).next().unwrap(), false, None); // Next, send a keysend payment with the same payment_hash and make sure it fails. - nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), - RecipientOnionFields::spontaneous_empty(), PaymentId(payment_preimage.0)).unwrap(); + nodes[0].node.send_spontaneous_payment( + Some(payment_preimage), RecipientOnionFields::spontaneous_empty(), + PaymentId(payment_preimage.0), route.route_params.clone().unwrap(), Retry::Attempts(0) + ).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); @@ -14560,12 +14539,10 @@ mod tests { let route_params = RouteParameters::from_payment_params_and_value( PaymentParameters::for_keysend(expected_route.last().unwrap().node.get_our_node_id(), TEST_FINAL_CLTV, false), 100_000); - let route = find_route( - &nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph, - None, nodes[0].logger, &scorer, &Default::default(), &random_seed_bytes + nodes[0].node.send_spontaneous_payment( + Some(payment_preimage), RecipientOnionFields::spontaneous_empty(), + PaymentId(payment_preimage.0), route_params.clone(), Retry::Attempts(0) ).unwrap(); - nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), - RecipientOnionFields::spontaneous_empty(), PaymentId(payment_preimage.0)).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); @@ -14598,8 +14575,10 @@ mod tests { &nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph, None, nodes[0].logger, &scorer, &Default::default(), &random_seed_bytes ).unwrap(); - let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), - RecipientOnionFields::spontaneous_empty(), PaymentId(payment_preimage.0)).unwrap(); + let payment_hash = nodes[0].node.send_spontaneous_payment( + Some(payment_preimage), RecipientOnionFields::spontaneous_empty(), + PaymentId(payment_preimage.0), route.route_params.clone().unwrap(), Retry::Attempts(0) + ).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); @@ -14637,8 +14616,10 @@ mod tests { // To start (3), send a keysend payment but don't claim it. let payment_id_1 = PaymentId([44; 32]); - let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), - RecipientOnionFields::spontaneous_empty(), payment_id_1).unwrap(); + let payment_hash = nodes[0].node.send_spontaneous_payment( + Some(payment_preimage), RecipientOnionFields::spontaneous_empty(), payment_id_1, + route.route_params.clone().unwrap(), Retry::Attempts(0) + ).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); @@ -14651,13 +14632,11 @@ mod tests { PaymentParameters::for_keysend(expected_route.last().unwrap().node.get_our_node_id(), TEST_FINAL_CLTV, false), 100_000 ); - let route = find_route( - &nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph, - None, nodes[0].logger, &scorer, &Default::default(), &random_seed_bytes - ).unwrap(); let payment_id_2 = PaymentId([45; 32]); - nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), - RecipientOnionFields::spontaneous_empty(), payment_id_2).unwrap(); + nodes[0].node.send_spontaneous_payment( + Some(payment_preimage), RecipientOnionFields::spontaneous_empty(), payment_id_2, route_params, + Retry::Attempts(0) + ).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index e8a550215..f4c2e402c 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -815,33 +815,6 @@ impl OutboundPayments { .map(|()| payment_hash) } - pub(super) fn send_spontaneous_payment_with_route( - &self, route: &Route, payment_preimage: Option, - recipient_onion: RecipientOnionFields, payment_id: PaymentId, entropy_source: &ES, - node_signer: &NS, best_block_height: u32, send_payment_along_path: F - ) -> Result - where - ES::Target: EntropySource, - NS::Target: NodeSigner, - F: Fn(SendAlongPathArgs) -> Result<(), APIError>, - { - let preimage = payment_preimage - .unwrap_or_else(|| PaymentPreimage(entropy_source.get_secure_random_bytes())); - let payment_hash = PaymentHash(Sha256::hash(&preimage.0).to_byte_array()); - let onion_session_privs = self.add_new_pending_payment(payment_hash, recipient_onion.clone(), - payment_id, Some(preimage), &route, None, None, entropy_source, best_block_height)?; - - match self.pay_route_internal(route, payment_hash, &recipient_onion, Some(preimage), None, - payment_id, None, onion_session_privs, node_signer, best_block_height, &send_payment_along_path - ) { - Ok(()) => Ok(payment_hash), - Err(e) => { - self.remove_outbound_if_all_failed(payment_id, &e); - Err(e) - } - } - } - pub(super) fn send_payment_for_bolt12_invoice< R: Deref, ES: Deref, NS: Deref, NL: Deref, IH, SP, L: Deref >( diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index a6cd55ba0..0c9c5d0e9 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -24,9 +24,9 @@ use crate::types::payment::{PaymentHash, PaymentSecret, PaymentPreimage}; use crate::ln::chan_utils; use crate::ln::msgs::ChannelMessageHandler; use crate::ln::onion_utils; -use crate::ln::outbound_payment::{IDEMPOTENCY_TIMEOUT_TICKS, Retry}; +use crate::ln::outbound_payment::{IDEMPOTENCY_TIMEOUT_TICKS, Retry, RetryableSendFailure}; use crate::routing::gossip::{EffectiveCapacity, RoutingFees}; -use crate::routing::router::{get_route, Path, PaymentParameters, Route, Router, RouteHint, RouteHintHop, RouteHop, RouteParameters, find_route}; +use crate::routing::router::{get_route, Path, PaymentParameters, Route, Router, RouteHint, RouteHintHop, RouteHop, RouteParameters}; use crate::routing::scoring::ChannelUsage; use crate::util::config::UserConfig; use crate::util::test_utils; @@ -369,13 +369,11 @@ fn mpp_receive_timeout() { #[test] fn test_keysend_payments() { - do_test_keysend_payments(false, false); - do_test_keysend_payments(false, true); - do_test_keysend_payments(true, false); - do_test_keysend_payments(true, true); + do_test_keysend_payments(false); + do_test_keysend_payments(true); } -fn do_test_keysend_payments(public_node: bool, with_retry: bool) { +fn do_test_keysend_payments(public_node: bool) { 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]); @@ -386,33 +384,16 @@ fn do_test_keysend_payments(public_node: bool, with_retry: bool) { } else { create_chan_between_nodes(&nodes[0], &nodes[1]); } - let payer_pubkey = nodes[0].node.get_our_node_id(); let payee_pubkey = nodes[1].node.get_our_node_id(); let route_params = RouteParameters::from_payment_params_and_value( PaymentParameters::for_keysend(payee_pubkey, 40, false), 10000); - let network_graph = nodes[0].network_graph; - let channels = nodes[0].node.list_usable_channels(); - let first_hops = channels.iter().collect::>(); - let first_hops = if public_node { None } else { Some(first_hops.as_slice()) }; - - let scorer = test_utils::TestScorer::new(); - let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes(); - let route = find_route( - &payer_pubkey, &route_params, &network_graph, first_hops, - nodes[0].logger, &scorer, &Default::default(), &random_seed_bytes - ).unwrap(); - { let test_preimage = PaymentPreimage([42; 32]); - if with_retry { - nodes[0].node.send_spontaneous_payment_with_retry(Some(test_preimage), - RecipientOnionFields::spontaneous_empty(), PaymentId(test_preimage.0), - route_params, Retry::Attempts(1)).unwrap() - } else { - nodes[0].node.send_spontaneous_payment(&route, Some(test_preimage), - RecipientOnionFields::spontaneous_empty(), PaymentId(test_preimage.0)).unwrap() - }; + nodes[0].node.send_spontaneous_payment( + Some(test_preimage), RecipientOnionFields::spontaneous_empty(), PaymentId(test_preimage.0), + route_params, Retry::Attempts(1) + ).unwrap(); } check_added_monitors!(nodes[0], 1); let send_event = SendEvent::from_node(&nodes[0]); @@ -441,22 +422,18 @@ fn test_mpp_keysend() { create_announced_chan_between_nodes(&nodes, 0, 2); create_announced_chan_between_nodes(&nodes, 1, 3); create_announced_chan_between_nodes(&nodes, 2, 3); - let network_graph = nodes[0].network_graph; - let payer_pubkey = nodes[0].node.get_our_node_id(); let payee_pubkey = nodes[3].node.get_our_node_id(); let recv_value = 15_000_000; let route_params = RouteParameters::from_payment_params_and_value( PaymentParameters::for_keysend(payee_pubkey, 40, true), recv_value); - let scorer = test_utils::TestScorer::new(); - let random_seed_bytes = chanmon_cfgs[0].keys_manager.get_secure_random_bytes(); - let route = find_route(&payer_pubkey, &route_params, &network_graph, None, nodes[0].logger, - &scorer, &Default::default(), &random_seed_bytes).unwrap(); let payment_preimage = PaymentPreimage([42; 32]); let payment_secret = PaymentSecret(payment_preimage.0); - let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), - RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_preimage.0)).unwrap(); + let payment_hash = nodes[0].node.send_spontaneous_payment( + Some(payment_preimage), RecipientOnionFields::secret_only(payment_secret), + PaymentId(payment_preimage.0), route_params, Retry::Attempts(0) + ).unwrap(); check_added_monitors!(nodes[0], 2); let expected_route: &[&[&Node]] = &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]]; @@ -499,7 +476,11 @@ 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].node.send_spontaneous_payment(&route, Some(payment_preimage), RecipientOnionFields::spontaneous_empty(), payment_id_0).unwrap(); + nodes[0].router.expect_find_route(route.route_params.clone().unwrap(), Ok(route.clone())); + nodes[0].node.send_spontaneous_payment( + Some(payment_preimage), RecipientOnionFields::spontaneous_empty(), payment_id_0, + route.route_params.clone().unwrap(), Retry::Attempts(0) + ).unwrap(); check_added_monitors!(nodes[0], 1); let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); @@ -541,7 +522,11 @@ 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].node.send_spontaneous_payment(&route, Some(payment_preimage), RecipientOnionFields::spontaneous_empty(), payment_id_1).unwrap(); + nodes[0].router.expect_find_route(route.route_params.clone().unwrap(), Ok(route.clone())); + nodes[0].node.send_spontaneous_payment( + Some(payment_preimage), RecipientOnionFields::spontaneous_empty(), payment_id_1, + route.route_params.clone().unwrap(), Retry::Attempts(0) + ).unwrap(); check_added_monitors!(nodes[0], 1); let update_2 = get_htlc_update_msgs!(nodes[0], nodes[2].node.get_our_node_id()); @@ -1569,9 +1554,11 @@ fn claimed_send_payment_idempotent() { // Further, if we try to send a spontaneous payment with the same payment_id it should // also be rejected. let send_result = nodes[0].node.send_spontaneous_payment( - &route, None, RecipientOnionFields::spontaneous_empty(), payment_id); + None, RecipientOnionFields::spontaneous_empty(), payment_id, + route.route_params.clone().unwrap(), Retry::Attempts(0) + ); match send_result { - Err(PaymentSendFailure::DuplicatePayment) => {}, + Err(RetryableSendFailure::DuplicatePayment) => {}, _ => panic!("Unexpected send result: {:?}", send_result), } } @@ -1646,9 +1633,11 @@ fn abandoned_send_payment_idempotent() { // Further, if we try to send a spontaneous payment with the same payment_id it should // also be rejected. let send_result = nodes[0].node.send_spontaneous_payment( - &route, None, RecipientOnionFields::spontaneous_empty(), payment_id); + None, RecipientOnionFields::spontaneous_empty(), payment_id, + route.route_params.clone().unwrap(), Retry::Attempts(0) + ); match send_result { - Err(PaymentSendFailure::DuplicatePayment) => {}, + Err(RetryableSendFailure::DuplicatePayment) => {}, _ => panic!("Unexpected send result: {:?}", send_result), } } @@ -2287,8 +2276,8 @@ fn do_automatic_retries(test: AutoRetry) { ClaimAlongRouteArgs::new(&nodes[0], &[&[&nodes[1], &nodes[2]]], payment_preimage) ); } else if test == AutoRetry::Spontaneous { - nodes[0].node.send_spontaneous_payment_with_retry(Some(payment_preimage), - RecipientOnionFields::spontaneous_empty(), PaymentId(payment_hash.0), route_params, + nodes[0].node.send_spontaneous_payment(Some(payment_preimage), + RecipientOnionFields::spontaneous_empty(), PaymentId(payment_hash.0), route_params.clone(), Retry::Attempts(1)).unwrap(); pass_failed_attempt_with_retry_along_path!(channel_id_2, true); @@ -2308,7 +2297,7 @@ fn do_automatic_retries(test: AutoRetry) { } else if test == AutoRetry::FailAttempts { // Ensure ChannelManager will not retry a payment if it has run out of payment attempts. nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret), - PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap(); + PaymentId(payment_hash.0), route_params.clone(), Retry::Attempts(1)).unwrap(); pass_failed_attempt_with_retry_along_path!(channel_id_2, true); // Open a new channel with no liquidity on the second hop so we can find a (bad) route for @@ -3704,7 +3693,10 @@ fn do_test_custom_tlvs(spontaneous: bool, even_tlvs: bool, known_tlvs: bool) { custom_tlvs: custom_tlvs.clone() }; if spontaneous { - nodes[0].node.send_spontaneous_payment(&route, Some(our_payment_preimage), onion_fields, payment_id).unwrap(); + nodes[0].node.send_spontaneous_payment( + Some(our_payment_preimage), onion_fields, payment_id, route.route_params.unwrap(), + Retry::Attempts(0) + ).unwrap(); } else { nodes[0].node.send_payment_with_route(route, our_payment_hash, onion_fields, payment_id).unwrap(); } diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index d614737a0..e1f6116cf 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -532,7 +532,7 @@ fn do_test_data_loss_protect(reconnect_panicing: bool, substantially_old: bool, // `not_stale` to test the boundary condition. let pay_params = PaymentParameters::for_keysend(nodes[1].node.get_our_node_id(), 100, false); let route_params = RouteParameters::from_payment_params_and_value(pay_params, 40000); - nodes[0].node.send_spontaneous_payment_with_retry(None, RecipientOnionFields::spontaneous_empty(), PaymentId([0; 32]), route_params, Retry::Attempts(0)).unwrap(); + nodes[0].node.send_spontaneous_payment(None, RecipientOnionFields::spontaneous_empty(), PaymentId([0; 32]), route_params, Retry::Attempts(0)).unwrap(); check_added_monitors(&nodes[0], 1); let update_add_commit = SendEvent::from_node(&nodes[0]); From 66bdc627128b8e7e929c273e4acef90a67a63482 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 27 Nov 2024 17:01:39 -0500 Subject: [PATCH 3/4] Stop using PaymentSendFailure within ProbeSendFailure Removes the final usage of PaymentSendFailure from public API. This (confusing) error matched with prior versions of LDK where users had to handle payment retries themselves. Since auto-retry was introduced, the only non-deprecated use remaining was for probe send errors. Probes only have one path, though, so refactor ProbeSendFailure to omit usage of PaymentSendFailure. We don't make this error private yet because it's still used by some fuzzing code as well as internally to outbound_payments, but it isn't returned by any public functions anymore. --- lightning/src/ln/channelmanager.rs | 4 +-- lightning/src/ln/outbound_payment.rs | 51 ++++++++++++++++++++++++---- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 8b9c5ce69..a8690d667 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4908,7 +4908,7 @@ where /// Send a payment that is probing the given route for liquidity. We calculate the /// [`PaymentHash`] of probes based on a static secret and a random [`PaymentId`], which allows /// us to easily discern them from real payments. - pub fn send_probe(&self, path: Path) -> Result<(PaymentHash, PaymentId), PaymentSendFailure> { + pub fn send_probe(&self, path: Path) -> Result<(PaymentHash, PaymentId), ProbeSendFailure> { let best_block_height = self.best_block.read().unwrap().height; let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); self.pending_outbound_payments.send_probe(path, self.probing_cookie_secret, @@ -5026,7 +5026,7 @@ where res.push(self.send_probe(path).map_err(|e| { log_error!(self.logger, "Failed to send pre-flight probe: {:?}", e); - ProbeSendFailure::SendingFailed(e) + e })?); } diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index f4c2e402c..2cfee6221 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -576,8 +576,24 @@ pub enum Bolt12PaymentError { pub enum ProbeSendFailure { /// We were unable to find a route to the destination. RouteNotFound, - /// We failed to send the payment probes. - SendingFailed(PaymentSendFailure), + /// A parameter which was passed to [`ChannelManager::send_probe`] was invalid, preventing us from + /// attempting to send the probe at all. + /// + /// You can freely resend the probe (with the parameter error fixed). + /// + /// Because the probe failed outright, no payment tracking is done and no + /// [`Event::ProbeFailed`] events will be generated. + /// + /// [`ChannelManager::send_probe`]: crate::ln::channelmanager::ChannelManager::send_probe + /// [`Event::ProbeFailed`]: crate::events::Event::ProbeFailed + ParameterError(APIError), + /// Indicates that a payment for the provided [`PaymentId`] is already in-flight and has not + /// yet completed (i.e. generated an [`Event::ProbeSuccessful`] or [`Event::ProbeFailed`]). + /// + /// [`PaymentId`]: crate::ln::channelmanager::PaymentId + /// [`Event::ProbeSuccessful`]: crate::events::Event::ProbeSuccessful + /// [`Event::ProbeFailed`]: crate::events::Event::ProbeFailed + DuplicateProbe, } /// Information which is provided, encrypted, to the payment recipient when sending HTLCs. @@ -1497,7 +1513,7 @@ impl OutboundPayments { pub(super) fn send_probe( &self, path: Path, probing_cookie_secret: [u8; 32], entropy_source: &ES, node_signer: &NS, best_block_height: u32, send_payment_along_path: F - ) -> Result<(PaymentHash, PaymentId), PaymentSendFailure> + ) -> Result<(PaymentHash, PaymentId), ProbeSendFailure> where ES::Target: EntropySource, NS::Target: NodeSigner, @@ -1509,7 +1525,7 @@ impl OutboundPayments { let payment_hash = probing_cookie_from_id(&payment_id, probing_cookie_secret); if path.hops.len() < 2 && path.blinded_tail.is_none() { - return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { + return Err(ProbeSendFailure::ParameterError(APIError::APIMisuseError { err: "No need probing a path with less than two hops".to_string() })) } @@ -1517,7 +1533,11 @@ impl OutboundPayments { let route = Route { paths: vec![path], route_params: None }; let onion_session_privs = self.add_new_pending_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret), payment_id, None, &route, None, None, - entropy_source, best_block_height)?; + entropy_source, best_block_height + ).map_err(|e| { + debug_assert!(matches!(e, PaymentSendFailure::DuplicatePayment)); + ProbeSendFailure::DuplicateProbe + })?; let recipient_onion_fields = RecipientOnionFields::spontaneous_empty(); match self.pay_route_internal(&route, payment_hash, &recipient_onion_fields, @@ -1527,7 +1547,26 @@ impl OutboundPayments { Ok(()) => Ok((payment_hash, payment_id)), Err(e) => { self.remove_outbound_if_all_failed(payment_id, &e); - Err(e) + match e { + PaymentSendFailure::DuplicatePayment => Err(ProbeSendFailure::DuplicateProbe), + PaymentSendFailure::ParameterError(err) => Err(ProbeSendFailure::ParameterError(err)), + PaymentSendFailure::PartialFailure { results, .. } + | PaymentSendFailure::PathParameterError(results) => { + debug_assert_eq!(results.len(), 1); + let err = results.into_iter() + .find(|res| res.is_err()) + .map(|err| err.unwrap_err()) + .unwrap_or(APIError::APIMisuseError { err: "Unexpected error".to_owned() }); + Err(ProbeSendFailure::ParameterError(err)) + }, + PaymentSendFailure::AllFailedResendSafe(mut errors) => { + debug_assert_eq!(errors.len(), 1); + let err = errors + .pop() + .unwrap_or(APIError::APIMisuseError { err: "Unexpected error".to_owned() }); + Err(ProbeSendFailure::ParameterError(err)) + } + } } } } From bcaba29f9209071c4dd6932e7a1d7f2786eb1e99 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Mon, 2 Dec 2024 13:49:39 -0500 Subject: [PATCH 4/4] Remove deprecated send_payment_with_route usage from fuzzing This allows us to make the PaymentSendFailure error type private, as well as reduce the visibility of the vestigial send_payment_with_route method that was already made test and fuzz-only in a previous commit. --- fuzz/src/chanmon_consistency.rs | 191 ++++++++++++++------------- lightning/src/ln/channelmanager.rs | 15 ++- lightning/src/ln/outbound_payment.rs | 2 +- 3 files changed, 113 insertions(+), 95 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 8fdd7b03e..421a0d09a 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -47,8 +47,7 @@ use lightning::events::MessageSendEventsProvider; use lightning::ln::channel::FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE; use lightning::ln::channel_state::ChannelDetails; use lightning::ln::channelmanager::{ - ChainParameters, ChannelManager, ChannelManagerReadArgs, PaymentId, PaymentSendFailure, - RecipientOnionFields, + ChainParameters, ChannelManager, ChannelManagerReadArgs, PaymentId, RecipientOnionFields, Retry, }; use lightning::ln::functional_test_utils::*; use lightning::ln::msgs::{ @@ -58,7 +57,9 @@ use lightning::ln::script::ShutdownScript; use lightning::ln::types::ChannelId; use lightning::offers::invoice::UnsignedBolt12Invoice; use lightning::onion_message::messenger::{Destination, MessageRouter, OnionMessagePath}; -use lightning::routing::router::{InFlightHtlcs, Path, Route, RouteHop, RouteParameters, Router}; +use lightning::routing::router::{ + InFlightHtlcs, Path, PaymentParameters, Route, RouteHop, RouteParameters, Router, +}; use lightning::sign::{ EntropySource, InMemorySigner, KeyMaterial, NodeSigner, Recipient, SignerProvider, }; @@ -82,6 +83,7 @@ use bitcoin::secp256k1::{self, Message, PublicKey, Scalar, Secp256k1, SecretKey} use lightning::io::Cursor; use std::cmp::{self, Ordering}; +use std::collections::VecDeque; use std::mem; use std::sync::atomic; use std::sync::{Arc, Mutex}; @@ -112,13 +114,18 @@ impl FeeEstimator for FuzzEstimator { } } -struct FuzzRouter {} +struct FuzzRouter { + pub next_routes: Mutex>, +} impl Router for FuzzRouter { fn find_route( &self, _payer: &PublicKey, _params: &RouteParameters, _first_hops: Option<&[&ChannelDetails]>, _inflight_htlcs: InFlightHtlcs, ) -> Result { + if let Some(route) = self.next_routes.lock().unwrap().pop_front() { + return Ok(route); + } Err(msgs::LightningError { err: String::from("Not implemented"), action: msgs::ErrorAction::IgnoreError, @@ -434,6 +441,34 @@ impl KeyProvider { } } +// Returns a bool indicating whether the payment failed. +#[inline] +fn check_payment_send_events( + source: &ChanMan, amt: u64, min_sendable: u64, max_sendable: u64, +) -> bool { + let mut payment_failed = false; + let events = source.get_and_clear_pending_events(); + assert!(events.len() == 2 || events.len() == 0); + for ev in events { + match ev { + events::Event::PaymentPathFailed { + failure: events::PathFailure::InitialSend { err }, + .. + } => { + check_api_err(err, amt > max_sendable || amt < min_sendable); + }, + events::Event::PaymentFailed { .. } => {}, + _ => panic!(), + }; + payment_failed = true; + } + // Note that while the max is a strict upper-bound, we can occasionally send substantially + // below the minimum, with some gap which is unusable immediately below the minimum. Thus, + // we don't check against min_value_sendable here. + assert!(payment_failed || (amt <= max_sendable)); + payment_failed +} + #[inline] fn check_api_err(api_err: APIError, sendable_bounds_violated: bool) { match api_err { @@ -460,34 +495,6 @@ fn check_api_err(api_err: APIError, sendable_bounds_violated: bool) { }, } } -#[inline] -fn check_payment_err(send_err: PaymentSendFailure, sendable_bounds_violated: bool) { - match send_err { - PaymentSendFailure::ParameterError(api_err) => { - check_api_err(api_err, sendable_bounds_violated) - }, - PaymentSendFailure::PathParameterError(per_path_results) => { - for res in per_path_results { - if let Err(api_err) = res { - check_api_err(api_err, sendable_bounds_violated); - } - } - }, - PaymentSendFailure::AllFailedResendSafe(per_path_results) => { - for api_err in per_path_results { - check_api_err(api_err, sendable_bounds_violated); - } - }, - PaymentSendFailure::PartialFailure { results, .. } => { - for res in results { - if let Err(api_err) = res { - check_api_err(api_err, sendable_bounds_violated); - } - } - }, - PaymentSendFailure::DuplicatePayment => panic!(), - } -} type ChanMan<'a> = ChannelManager< Arc, @@ -546,34 +553,36 @@ fn send_payment( .find(|chan| chan.short_channel_id == Some(dest_chan_id)) .map(|chan| (chan.next_outbound_htlc_minimum_msat, chan.next_outbound_htlc_limit_msat)) .unwrap_or((0, 0)); - if let Err(err) = source.send_payment_with_route( - Route { - paths: vec![Path { - hops: vec![RouteHop { - pubkey: dest.get_our_node_id(), - node_features: dest.node_features(), - short_channel_id: dest_chan_id, - channel_features: dest.channel_features(), - fee_msat: amt, - cltv_expiry_delta: 200, - maybe_announced_channel: true, - }], - blinded_tail: None, + let mut next_routes = source.router.next_routes.lock().unwrap(); + next_routes.push_back(Route { + paths: vec![Path { + hops: vec![RouteHop { + pubkey: dest.get_our_node_id(), + node_features: dest.node_features(), + short_channel_id: dest_chan_id, + channel_features: dest.channel_features(), + fee_msat: amt, + cltv_expiry_delta: 200, + maybe_announced_channel: true, }], - route_params: None, - }, + blinded_tail: None, + }], + route_params: None, + }); + let route_params = RouteParameters::from_payment_params_and_value( + PaymentParameters::from_node_id(source.get_our_node_id(), TEST_FINAL_CLTV), + amt, + ); + if let Err(err) = source.send_payment( payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_id), + route_params, + Retry::Attempts(0), ) { - check_payment_err(err, amt > max_value_sendable || amt < min_value_sendable); - false + panic!("Errored with {:?} on initial payment send", err); } else { - // Note that while the max is a strict upper-bound, we can occasionally send substantially - // below the minimum, with some gap which is unusable immediately below the minimum. Thus, - // we don't check against min_value_sendable here. - assert!(amt <= max_value_sendable); - true + check_payment_send_events(source, amt, min_value_sendable, max_value_sendable) } } @@ -615,46 +624,48 @@ fn send_hop_payment( .map(|chan| (chan.next_outbound_htlc_minimum_msat, chan.next_outbound_htlc_limit_msat)) .unwrap_or((0, 0)); let first_hop_fee = 50_000; - if let Err(err) = source.send_payment_with_route( - Route { - paths: vec![Path { - hops: vec![ - RouteHop { - pubkey: middle.get_our_node_id(), - node_features: middle.node_features(), - short_channel_id: middle_chan_id, - channel_features: middle.channel_features(), - fee_msat: first_hop_fee, - cltv_expiry_delta: 100, - maybe_announced_channel: true, - }, - RouteHop { - pubkey: dest.get_our_node_id(), - node_features: dest.node_features(), - short_channel_id: dest_chan_id, - channel_features: dest.channel_features(), - fee_msat: amt, - cltv_expiry_delta: 200, - maybe_announced_channel: true, - }, - ], - blinded_tail: None, - }], - route_params: None, - }, + let mut next_routes = source.router.next_routes.lock().unwrap(); + next_routes.push_back(Route { + paths: vec![Path { + hops: vec![ + RouteHop { + pubkey: middle.get_our_node_id(), + node_features: middle.node_features(), + short_channel_id: middle_chan_id, + channel_features: middle.channel_features(), + fee_msat: first_hop_fee, + cltv_expiry_delta: 100, + maybe_announced_channel: true, + }, + RouteHop { + pubkey: dest.get_our_node_id(), + node_features: dest.node_features(), + short_channel_id: dest_chan_id, + channel_features: dest.channel_features(), + fee_msat: amt, + cltv_expiry_delta: 200, + maybe_announced_channel: true, + }, + ], + blinded_tail: None, + }], + route_params: None, + }); + let route_params = RouteParameters::from_payment_params_and_value( + PaymentParameters::from_node_id(source.get_our_node_id(), TEST_FINAL_CLTV), + amt, + ); + if let Err(err) = source.send_payment( payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_id), + route_params, + Retry::Attempts(0), ) { - let sent_amt = amt + first_hop_fee; - check_payment_err(err, sent_amt < min_value_sendable || sent_amt > max_value_sendable); - false + panic!("Errored with {:?} on initial payment send", err); } else { - // Note that while the max is a strict upper-bound, we can occasionally send substantially - // below the minimum, with some gap which is unusable immediately below the minimum. Thus, - // we don't check against min_value_sendable here. - assert!(amt + first_hop_fee <= max_value_sendable); - true + let sent_amt = amt + first_hop_fee; + check_payment_send_events(source, sent_amt, min_value_sendable, max_value_sendable) } } @@ -662,7 +673,7 @@ fn send_hop_payment( pub fn do_test(data: &[u8], underlying_out: Out, anchors: bool) { let out = SearchingOutput::new(underlying_out); let broadcast = Arc::new(TestBroadcaster {}); - let router = FuzzRouter {}; + let router = FuzzRouter { next_routes: Mutex::new(VecDeque::new()) }; macro_rules! make_node { ($node_id: expr, $fee_estimator: expr) => {{ diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index a8690d667..ca6b7192d 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -125,7 +125,9 @@ use core::time::Duration; use core::ops::Deref; use bitcoin::hex::impl_fmt_traits; // Re-export this for use in the public API. -pub use crate::ln::outbound_payment::{Bolt12PaymentError, PaymentSendFailure, ProbeSendFailure, Retry, RetryableSendFailure, RecipientOnionFields}; +pub use crate::ln::outbound_payment::{Bolt12PaymentError, ProbeSendFailure, Retry, RetryableSendFailure, RecipientOnionFields}; +#[cfg(test)] +pub(crate) use crate::ln::outbound_payment::PaymentSendFailure; use crate::ln::script::ShutdownScript; // We hold various information about HTLC relay in the HTLC objects in Channel itself: @@ -2376,7 +2378,9 @@ where fee_estimator: LowerBoundedFeeEstimator, chain_monitor: M, tx_broadcaster: T, - #[allow(unused)] + #[cfg(fuzzing)] + pub router: R, + #[cfg(not(fuzzing))] router: R, message_router: MR, @@ -4621,8 +4625,11 @@ where // [`TestRouter::expect_find_route`] instead. // // [`TestRouter::expect_find_route`]: crate::util::test_utils::TestRouter::expect_find_route - #[cfg(any(test, fuzzing))] - pub fn send_payment_with_route(&self, route: Route, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, payment_id: PaymentId) -> Result<(), PaymentSendFailure> { + #[cfg(test)] + pub(crate) fn send_payment_with_route( + &self, route: Route, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields, + payment_id: PaymentId + ) -> Result<(), PaymentSendFailure> { let best_block_height = self.best_block.read().unwrap().height; let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); self.pending_outbound_payments diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 2cfee6221..2f3bb3378 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -482,7 +482,7 @@ pub enum RetryableSendFailure { /// as the Err() type describing which state the payment is in, see the description of individual /// enum states for more. #[derive(Clone, Debug, PartialEq, Eq)] -pub enum PaymentSendFailure { +pub(crate) enum PaymentSendFailure { /// A parameter which was passed to send_payment was invalid, preventing us from attempting to /// send the payment at all. ///