Skip to content

Commit 70f2873

Browse files
valentinewallacevalentinewallace
committed
Fix computing in-flight HTLCs in between retries + test
1 parent c7144ac commit 70f2873

File tree

3 files changed

+37
-23
lines changed

3 files changed

+37
-23
lines changed

‎lightning/src/ln/channelmanager.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -2546,7 +2546,7 @@ where
25462546
let best_block_height = self.best_block.read().unwrap().height();
25472547
self.pending_outbound_payments
25482548
.send_payment(payment_hash, payment_secret, payment_id, retry_strategy, route_params,
2549-
&self.router, self.list_usable_channels(), self.compute_inflight_htlcs(),
2549+
&self.router, self.list_usable_channels(), || self.compute_inflight_htlcs(),
25502550
&self.entropy_source, &self.node_signer, best_block_height, &self.logger,
25512551
|path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv|
25522552
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
26462646
let best_block_height = self.best_block.read().unwrap().height();
26472647
self.pending_outbound_payments.send_spontaneous_payment(payment_preimage, payment_id,
26482648
retry_strategy, route_params, &self.router, self.list_usable_channels(),
2649-
self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height,
2649+
|| self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height,
26502650
&self.logger,
26512651
|path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv|
26522652
self.send_payment_along_path(path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv))

‎lightning/src/ln/outbound_payment.rs

+23-20
Original file line numberDiff line numberDiff line change
@@ -402,22 +402,23 @@ impl OutboundPayments {
402402
}
403403
}
404404

405-
pub(super) fn send_payment<R: Deref, ES: Deref, NS: Deref, F, L: Deref>(
405+
pub(super) fn send_payment<R: Deref, ES: Deref, NS: Deref, IH, SP, L: Deref>(
406406
&self, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, payment_id: PaymentId,
407407
retry_strategy: Retry, route_params: RouteParameters, router: &R,
408-
first_hops: Vec<ChannelDetails>, inflight_htlcs: InFlightHtlcs, entropy_source: &ES,
409-
node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: F,
408+
first_hops: Vec<ChannelDetails>, compute_inflight_htlcs: IH, entropy_source: &ES,
409+
node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: SP,
410410
) -> Result<(), PaymentSendFailure>
411411
where
412412
R::Target: Router,
413413
ES::Target: EntropySource,
414414
NS::Target: NodeSigner,
415415
L::Target: Logger,
416-
F: Fn(&Vec<RouteHop>, &Option<PaymentParameters>, &PaymentHash, &Option<PaymentSecret>, u64,
416+
IH: Fn() -> InFlightHtlcs,
417+
SP: Fn(&Vec<RouteHop>, &Option<PaymentParameters>, &PaymentHash, &Option<PaymentSecret>, u64,
417418
u32, PaymentId, &Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>,
418419
{
419420
self.pay_internal(payment_id, Some((payment_hash, payment_secret, None, retry_strategy)),
420-
route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer,
421+
route_params, router, first_hops, &compute_inflight_htlcs, entropy_source, node_signer,
421422
best_block_height, logger, &send_payment_along_path)
422423
.map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e })
423424
}
@@ -439,25 +440,26 @@ impl OutboundPayments {
439440
.map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e })
440441
}
441442

442-
pub(super) fn send_spontaneous_payment<R: Deref, ES: Deref, NS: Deref, F, L: Deref>(
443+
pub(super) fn send_spontaneous_payment<R: Deref, ES: Deref, NS: Deref, IH, SP, L: Deref>(
443444
&self, payment_preimage: Option<PaymentPreimage>, payment_id: PaymentId,
444445
retry_strategy: Retry, route_params: RouteParameters, router: &R,
445-
first_hops: Vec<ChannelDetails>, inflight_htlcs: InFlightHtlcs, entropy_source: &ES,
446-
node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: F
446+
first_hops: Vec<ChannelDetails>, inflight_htlcs: IH, entropy_source: &ES,
447+
node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: SP
447448
) -> Result<PaymentHash, PaymentSendFailure>
448449
where
449450
R::Target: Router,
450451
ES::Target: EntropySource,
451452
NS::Target: NodeSigner,
452453
L::Target: Logger,
453-
F: Fn(&Vec<RouteHop>, &Option<PaymentParameters>, &PaymentHash, &Option<PaymentSecret>, u64,
454+
IH: Fn() -> InFlightHtlcs,
455+
SP: Fn(&Vec<RouteHop>, &Option<PaymentParameters>, &PaymentHash, &Option<PaymentSecret>, u64,
454456
u32, PaymentId, &Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>,
455457
{
456458
let preimage = payment_preimage
457459
.unwrap_or_else(|| PaymentPreimage(entropy_source.get_secure_random_bytes()));
458460
let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner());
459461
self.pay_internal(payment_id, Some((payment_hash, &None, Some(preimage), retry_strategy)),
460-
route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer,
462+
route_params, router, first_hops, &inflight_htlcs, entropy_source, node_signer,
461463
best_block_height, logger, &send_payment_along_path)
462464
.map(|()| payment_hash)
463465
.map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e })
@@ -525,26 +527,27 @@ impl OutboundPayments {
525527
}
526528
if let Some((payment_id, route_params)) = retry_id_route_params {
527529
core::mem::drop(outbounds);
528-
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) {
530+
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) {
529531
log_info!(logger, "Errored retrying payment: {:?}", e);
530532
}
531533
} else { break }
532534
}
533535
}
534536

535-
fn pay_internal<R: Deref, NS: Deref, ES: Deref, F, L: Deref>(
537+
fn pay_internal<R: Deref, NS: Deref, ES: Deref, IH, SP, L: Deref>(
536538
&self, payment_id: PaymentId,
537539
initial_send_info: Option<(PaymentHash, &Option<PaymentSecret>, Option<PaymentPreimage>, Retry)>,
538540
route_params: RouteParameters, router: &R, first_hops: Vec<ChannelDetails>,
539-
inflight_htlcs: InFlightHtlcs, entropy_source: &ES, node_signer: &NS, best_block_height: u32,
540-
logger: &L, send_payment_along_path: &F,
541+
inflight_htlcs: &IH, entropy_source: &ES, node_signer: &NS, best_block_height: u32,
542+
logger: &L, send_payment_along_path: &SP,
541543
) -> Result<(), PaymentSendFailure>
542544
where
543545
R::Target: Router,
544546
ES::Target: EntropySource,
545547
NS::Target: NodeSigner,
546548
L::Target: Logger,
547-
F: Fn(&Vec<RouteHop>, &Option<PaymentParameters>, &PaymentHash, &Option<PaymentSecret>, u64,
549+
IH: Fn() -> InFlightHtlcs,
550+
SP: Fn(&Vec<RouteHop>, &Option<PaymentParameters>, &PaymentHash, &Option<PaymentSecret>, u64,
548551
u32, PaymentId, &Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>
549552
{
550553
#[cfg(feature = "std")] {
@@ -557,7 +560,7 @@ impl OutboundPayments {
557560

558561
let route = router.find_route(
559562
&node_signer.get_node_id(Recipient::Node).unwrap(), &route_params,
560-
Some(&first_hops.iter().collect::<Vec<_>>()), &inflight_htlcs
563+
Some(&first_hops.iter().collect::<Vec<_>>()), &inflight_htlcs(),
561564
).map_err(|e| PaymentSendFailure::ParameterError(APIError::APIMisuseError {
562565
err: format!("Failed to find a route for payment {}: {:?}", log_bytes!(payment_id.0), e), // TODO: add APIError::RouteNotFound
563566
}))?;
@@ -1235,12 +1238,12 @@ mod tests {
12351238
};
12361239
let err = if on_retry {
12371240
outbound_payments.pay_internal(
1238-
PaymentId([0; 32]), None, expired_route_params, &&router, vec![], InFlightHtlcs::new(),
1241+
PaymentId([0; 32]), None, expired_route_params, &&router, vec![], &|| InFlightHtlcs::new(),
12391242
&&keys_manager, &&keys_manager, 0, &&logger, &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err()
12401243
} else {
12411244
outbound_payments.send_payment(
12421245
PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), expired_route_params,
1243-
&&router, vec![], InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger,
1246+
&&router, vec![], || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger,
12441247
|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err()
12451248
};
12461249
if let PaymentSendFailure::ParameterError(APIError::APIMisuseError { err }) = err {
@@ -1278,12 +1281,12 @@ mod tests {
12781281
&Route { paths: vec![], payment_params: None }, Some(Retry::Attempts(1)),
12791282
Some(route_params.payment_params.clone()), &&keys_manager, 0).unwrap();
12801283
outbound_payments.pay_internal(
1281-
PaymentId([0; 32]), None, route_params, &&router, vec![], InFlightHtlcs::new(),
1284+
PaymentId([0; 32]), None, route_params, &&router, vec![], &|| InFlightHtlcs::new(),
12821285
&&keys_manager, &&keys_manager, 0, &&logger, &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err()
12831286
} else {
12841287
outbound_payments.send_payment(
12851288
PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), route_params,
1286-
&&router, vec![], InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger,
1289+
&&router, vec![], || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger,
12871290
|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err()
12881291
};
12891292
if let PaymentSendFailure::ParameterError(APIError::APIMisuseError { err }) = err {

‎lightning/src/ln/payment_tests.rs

+12-1
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,9 @@ use crate::ln::features::InvoiceFeatures;
2121
use crate::ln::msgs;
2222
use crate::ln::msgs::ChannelMessageHandler;
2323
use crate::ln::outbound_payment::Retry;
24-
use crate::routing::gossip::RoutingFees;
24+
use crate::routing::gossip::{EffectiveCapacity, RoutingFees};
2525
use crate::routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RouteParameters};
26+
use crate::routing::scoring::ChannelUsage;
2627
use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
2728
use crate::util::test_utils;
2829
use crate::util::errors::APIError;
@@ -2175,6 +2176,16 @@ fn retry_multi_path_single_failed_payment() {
21752176
final_value_msat: 100_000_001, final_cltv_expiry_delta: TEST_FINAL_CLTV
21762177
}, Ok(route.clone()));
21772178

2179+
{
2180+
let scorer = chanmon_cfgs[0].scorer.lock().unwrap();
2181+
// The initial send attempt, 2 paths
2182+
scorer.expect_usage(ChannelUsage { amount_msat: 10_000, inflight_htlc_msat: 0, effective_capacity: EffectiveCapacity::Unknown } );
2183+
scorer.expect_usage(ChannelUsage { amount_msat: 100_000_001, inflight_htlc_msat: 0, effective_capacity: EffectiveCapacity::Unknown } );
2184+
// The retry, 2 paths. Ensure that the in-flight HTLC amount is factored in.
2185+
scorer.expect_usage(ChannelUsage { amount_msat: 50_000_001, inflight_htlc_msat: 10_000, effective_capacity: EffectiveCapacity::Unknown } );
2186+
scorer.expect_usage(ChannelUsage { amount_msat: 50_000_000, inflight_htlc_msat: 0, effective_capacity: EffectiveCapacity::Unknown } );
2187+
}
2188+
21782189
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
21792190
let htlc_msgs = nodes[0].node.get_and_clear_pending_msg_events();
21802191
assert_eq!(htlc_msgs.len(), 2);

0 commit comments

Comments
 (0)