From: Valentine Wallace Date: Tue, 7 Feb 2023 19:10:41 +0000 (-0500) Subject: Fix computing in-flight HTLCs in between retries + test X-Git-Tag: v0.0.114-beta~19^2~1 X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=825ea9d062886a369bda9d30ad6890fada7f2ed8;p=rust-lightning Fix computing in-flight HTLCs in between retries + test --- diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index cfb462b01..234d072a8 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2546,7 +2546,7 @@ where let best_block_height = self.best_block.read().unwrap().height(); self.pending_outbound_payments .send_payment(payment_hash, payment_secret, payment_id, retry_strategy, route_params, - &self.router, self.list_usable_channels(), self.compute_inflight_htlcs(), + &self.router, self.list_usable_channels(), || self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height, &self.logger, |path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv| self.send_payment_along_path(path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv)) @@ -2646,7 +2646,7 @@ where let best_block_height = self.best_block.read().unwrap().height(); self.pending_outbound_payments.send_spontaneous_payment(payment_preimage, payment_id, retry_strategy, route_params, &self.router, self.list_usable_channels(), - self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height, + || self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height, &self.logger, |path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv| self.send_payment_along_path(path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv)) diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index b1b610bf8..31ba9cb37 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -402,22 +402,23 @@ impl OutboundPayments { } } - pub(super) fn send_payment( + pub(super) fn send_payment( &self, payment_hash: PaymentHash, payment_secret: &Option, payment_id: PaymentId, retry_strategy: Retry, route_params: RouteParameters, router: &R, - first_hops: Vec, inflight_htlcs: InFlightHtlcs, entropy_source: &ES, - node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: F, + first_hops: Vec, compute_inflight_htlcs: IH, entropy_source: &ES, + node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: SP, ) -> Result<(), PaymentSendFailure> where R::Target: Router, ES::Target: EntropySource, NS::Target: NodeSigner, L::Target: Logger, - F: Fn(&Vec, &Option, &PaymentHash, &Option, u64, + IH: Fn() -> InFlightHtlcs, + SP: Fn(&Vec, &Option, &PaymentHash, &Option, u64, u32, PaymentId, &Option, [u8; 32]) -> Result<(), APIError>, { self.pay_internal(payment_id, Some((payment_hash, payment_secret, None, retry_strategy)), - route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer, + route_params, router, first_hops, &compute_inflight_htlcs, entropy_source, node_signer, best_block_height, logger, &send_payment_along_path) .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) } @@ -439,25 +440,26 @@ impl OutboundPayments { .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) } - pub(super) fn send_spontaneous_payment( + pub(super) fn send_spontaneous_payment( &self, payment_preimage: Option, payment_id: PaymentId, retry_strategy: Retry, route_params: RouteParameters, router: &R, - first_hops: Vec, inflight_htlcs: InFlightHtlcs, entropy_source: &ES, - node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: F + first_hops: Vec, inflight_htlcs: IH, entropy_source: &ES, + node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: SP ) -> Result where R::Target: Router, ES::Target: EntropySource, NS::Target: NodeSigner, L::Target: Logger, - F: Fn(&Vec, &Option, &PaymentHash, &Option, u64, + IH: Fn() -> InFlightHtlcs, + SP: Fn(&Vec, &Option, &PaymentHash, &Option, u64, u32, PaymentId, &Option, [u8; 32]) -> Result<(), APIError>, { let preimage = payment_preimage .unwrap_or_else(|| PaymentPreimage(entropy_source.get_secure_random_bytes())); let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner()); self.pay_internal(payment_id, Some((payment_hash, &None, Some(preimage), retry_strategy)), - route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer, + route_params, router, first_hops, &inflight_htlcs, entropy_source, node_signer, best_block_height, logger, &send_payment_along_path) .map(|()| payment_hash) .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) @@ -525,26 +527,27 @@ impl OutboundPayments { } if let Some((payment_id, route_params)) = retry_id_route_params { core::mem::drop(outbounds); - if let Err(e) = self.pay_internal(payment_id, None, route_params, router, first_hops(), inflight_htlcs(), entropy_source, node_signer, best_block_height, logger, &send_payment_along_path) { + if let Err(e) = self.pay_internal(payment_id, None, route_params, router, first_hops(), &inflight_htlcs, entropy_source, node_signer, best_block_height, logger, &send_payment_along_path) { log_info!(logger, "Errored retrying payment: {:?}", e); } } else { break } } } - fn pay_internal( + fn pay_internal( &self, payment_id: PaymentId, initial_send_info: Option<(PaymentHash, &Option, Option, Retry)>, route_params: RouteParameters, router: &R, first_hops: Vec, - inflight_htlcs: InFlightHtlcs, entropy_source: &ES, node_signer: &NS, best_block_height: u32, - logger: &L, send_payment_along_path: &F, + inflight_htlcs: &IH, entropy_source: &ES, node_signer: &NS, best_block_height: u32, + logger: &L, send_payment_along_path: &SP, ) -> Result<(), PaymentSendFailure> where R::Target: Router, ES::Target: EntropySource, NS::Target: NodeSigner, L::Target: Logger, - F: Fn(&Vec, &Option, &PaymentHash, &Option, u64, + IH: Fn() -> InFlightHtlcs, + SP: Fn(&Vec, &Option, &PaymentHash, &Option, u64, u32, PaymentId, &Option, [u8; 32]) -> Result<(), APIError> { #[cfg(feature = "std")] { @@ -557,7 +560,7 @@ impl OutboundPayments { let route = router.find_route( &node_signer.get_node_id(Recipient::Node).unwrap(), &route_params, - Some(&first_hops.iter().collect::>()), &inflight_htlcs + Some(&first_hops.iter().collect::>()), &inflight_htlcs(), ).map_err(|e| PaymentSendFailure::ParameterError(APIError::APIMisuseError { err: format!("Failed to find a route for payment {}: {:?}", log_bytes!(payment_id.0), e), // TODO: add APIError::RouteNotFound }))?; @@ -1235,12 +1238,12 @@ mod tests { }; let err = if on_retry { outbound_payments.pay_internal( - PaymentId([0; 32]), None, expired_route_params, &&router, vec![], InFlightHtlcs::new(), + PaymentId([0; 32]), None, expired_route_params, &&router, vec![], &|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() } else { outbound_payments.send_payment( PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), expired_route_params, - &&router, vec![], InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, + &&router, vec![], || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, |_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() }; if let PaymentSendFailure::ParameterError(APIError::APIMisuseError { err }) = err { @@ -1278,12 +1281,12 @@ mod tests { &Route { paths: vec![], payment_params: None }, Some(Retry::Attempts(1)), Some(route_params.payment_params.clone()), &&keys_manager, 0).unwrap(); outbound_payments.pay_internal( - PaymentId([0; 32]), None, route_params, &&router, vec![], InFlightHtlcs::new(), + PaymentId([0; 32]), None, route_params, &&router, vec![], &|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() } else { outbound_payments.send_payment( PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), route_params, - &&router, vec![], InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, + &&router, vec![], || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, |_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() }; if let PaymentSendFailure::ParameterError(APIError::APIMisuseError { err }) = err { diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index c82534ec8..dbd78b6f8 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -21,8 +21,9 @@ use crate::ln::features::InvoiceFeatures; use crate::ln::msgs; use crate::ln::msgs::ChannelMessageHandler; use crate::ln::outbound_payment::Retry; -use crate::routing::gossip::RoutingFees; +use crate::routing::gossip::{EffectiveCapacity, RoutingFees}; use crate::routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RouteParameters}; +use crate::routing::scoring::ChannelUsage; use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider}; use crate::util::test_utils; use crate::util::errors::APIError; @@ -2175,6 +2176,16 @@ fn retry_multi_path_single_failed_payment() { final_value_msat: 100_000_001, final_cltv_expiry_delta: TEST_FINAL_CLTV }, Ok(route.clone())); + { + let scorer = chanmon_cfgs[0].scorer.lock().unwrap(); + // The initial send attempt, 2 paths + scorer.expect_usage(chans[0].short_channel_id.unwrap(), ChannelUsage { amount_msat: 10_000, inflight_htlc_msat: 0, effective_capacity: EffectiveCapacity::Unknown }); + scorer.expect_usage(chans[1].short_channel_id.unwrap(), ChannelUsage { amount_msat: 100_000_001, inflight_htlc_msat: 0, effective_capacity: EffectiveCapacity::Unknown }); + // The retry, 2 paths. Ensure that the in-flight HTLC amount is factored in. + scorer.expect_usage(chans[0].short_channel_id.unwrap(), ChannelUsage { amount_msat: 50_000_001, inflight_htlc_msat: 10_000, effective_capacity: EffectiveCapacity::Unknown }); + scorer.expect_usage(chans[1].short_channel_id.unwrap(), ChannelUsage { amount_msat: 50_000_000, inflight_htlc_msat: 0, effective_capacity: EffectiveCapacity::Unknown }); + } + nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap(); let htlc_msgs = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(htlc_msgs.len(), 2);