Skip to content

Commit e82a6b1

Browse files
committed
Allow overshooting total_msat for an MPP
While retrying a failed path of an MPP, a node may want to overshoot the `total_msat` in order to use a path with an `htlc_minimum_msat` greater than the remaining value being sent. This commit no longer fails MPPs that overshoot the `total_msat`, except when there is at least one HTLC in that MPP that has a value greater than the difference between the received amount and `total_msat`.
1 parent 0b1a64f commit e82a6b1

File tree

4 files changed

+124
-9
lines changed

4 files changed

+124
-9
lines changed

lightning/src/ln/channelmanager.rs

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2584,7 +2584,7 @@ where
25842584
}
25852585

25862586
#[cfg(test)]
2587-
fn test_send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>, payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: Vec<[u8; 32]>) -> Result<(), PaymentSendFailure> {
2587+
pub(super) fn test_send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>, payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: Vec<[u8; 32]>) -> Result<(), PaymentSendFailure> {
25882588
let best_block_height = self.best_block.read().unwrap().height();
25892589
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
25902590
self.pending_outbound_payments.test_send_payment_internal(route, payment_hash, payment_secret, keysend_preimage, payment_id, recv_value_msat, onion_session_privs, &self.node_signer, best_block_height,
@@ -3274,11 +3274,17 @@ where
32743274
_ => unreachable!(),
32753275
}
32763276
}
3277-
if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data.total_msat {
3278-
log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)",
3279-
log_bytes!(payment_hash.0), total_value, $payment_data.total_msat);
3277+
let smallest_htlc_value = htlcs.iter().map(|htlc| htlc.value).min().unwrap_or(claimable_htlc.value);
3278+
if total_value >= msgs::MAX_VALUE_MSAT {
3279+
log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over the maximum possible msats {}",
3280+
log_bytes!(payment_hash.0), total_value, msgs::MAX_VALUE_MSAT);
32803281
fail_htlc!(claimable_htlc, payment_hash);
3281-
} else if total_value == $payment_data.total_msat {
3282+
} else if total_value >= $payment_data.total_msat && smallest_htlc_value <= total_value - $payment_data.total_msat {
3283+
log_trace!(self.logger,
3284+
"Failing HTLCs with payment_hash {} as an individual HTLC has a value {} which is smaller than the difference between the total paid {} and total_msat {}",
3285+
log_bytes!(payment_hash.0), smallest_htlc_value, total_value, $payment_data.total_msat);
3286+
fail_htlc!(claimable_htlc, payment_hash);
3287+
} else if total_value >= $payment_data.total_msat {
32823288
let prev_channel_id = prev_funding_outpoint.to_channel_id();
32833289
htlcs.push(claimable_htlc);
32843290
new_events.push(events::Event::PaymentClaimable {
@@ -3935,7 +3941,7 @@ where
39353941
log_info!(self.logger, "Attempted to claim an incomplete payment which no longer had any available HTLCs!");
39363942
return;
39373943
}
3938-
if claimable_amt_msat != expected_amt_msat.unwrap() {
3944+
if claimable_amt_msat < expected_amt_msat.unwrap() {
39393945
self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash);
39403946
log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",
39413947
expected_amt_msat.unwrap(), claimable_amt_msat);

lightning/src/ln/functional_tests.rs

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7925,6 +7925,116 @@ fn test_can_not_accept_unknown_inbound_channel() {
79257925
}
79267926
}
79277927

7928+
fn do_test_overshoot_mpp(msat_amounts: &[u64], total_msat: u64, should_fail: bool) {
7929+
7930+
let routing_node_count = msat_amounts.len();
7931+
let node_count = routing_node_count + 2;
7932+
7933+
let chanmon_cfgs = create_chanmon_cfgs(node_count);
7934+
let node_cfgs = create_node_cfgs(node_count, &chanmon_cfgs);
7935+
let node_chanmgrs = create_node_chanmgrs(node_count, &node_cfgs, &vec![None; node_count]);
7936+
let nodes = create_network(node_count, &node_cfgs, &node_chanmgrs);
7937+
7938+
let src_idx = 0;
7939+
let dst_idx = 1;
7940+
7941+
// Create channels for each amount
7942+
let mut expected_paths = Vec::with_capacity(routing_node_count);
7943+
let mut src_chan_ids = Vec::with_capacity(routing_node_count);
7944+
let mut dst_chan_ids = Vec::with_capacity(routing_node_count);
7945+
for i in 0..routing_node_count {
7946+
let routing_node = 2 + i;
7947+
let src_chan_id = create_announced_chan_between_nodes(&nodes, src_idx, routing_node).0.contents.short_channel_id;
7948+
src_chan_ids.push(src_chan_id);
7949+
let dst_chan_id = create_announced_chan_between_nodes(&nodes, routing_node, dst_idx).0.contents.short_channel_id;
7950+
dst_chan_ids.push(dst_chan_id);
7951+
let path = vec![&nodes[routing_node], &nodes[dst_idx]];
7952+
expected_paths.push(path);
7953+
}
7954+
let expected_paths: Vec<&[&Node]> = expected_paths.iter().map(|route| route.as_slice()).collect();
7955+
7956+
// Create a route for each amount
7957+
let example_amount = 100000;
7958+
let (mut route, our_payment_hash, our_payment_preimage, our_payment_secret) = get_route_and_payment_hash!(&nodes[src_idx], nodes[dst_idx], example_amount);
7959+
let sample_path = route.paths.pop().unwrap();
7960+
for i in 0..routing_node_count {
7961+
let routing_node = 2 + i;
7962+
let mut path = sample_path.clone();
7963+
path[0].pubkey = nodes[routing_node].node.get_our_node_id();
7964+
path[0].short_channel_id = src_chan_ids[i];
7965+
path[1].pubkey = nodes[dst_idx].node.get_our_node_id();
7966+
path[1].short_channel_id = dst_chan_ids[i];
7967+
path[1].fee_msat = msat_amounts[i];
7968+
route.paths.push(path);
7969+
}
7970+
7971+
// Send payment with manually set total_msat
7972+
let payment_id = PaymentId(nodes[src_idx].keys_manager.backing.get_secure_random_bytes());
7973+
let onion_session_privs = nodes[src_idx].node.test_add_new_pending_payment(our_payment_hash, Some(our_payment_secret), payment_id, &route).unwrap();
7974+
nodes[src_idx].node.test_send_payment_internal(&route, our_payment_hash, &Some(our_payment_secret), None, payment_id, Some(total_msat), onion_session_privs).unwrap();
7975+
check_added_monitors!(nodes[src_idx], expected_paths.len());
7976+
7977+
// Similar to pass_along_route but handles failures
7978+
let mut total_value = 0;
7979+
let mut events = nodes[src_idx].node.get_and_clear_pending_msg_events();
7980+
assert_eq!(events.len(), expected_paths.len());
7981+
7982+
for (path_idx, expected_path) in expected_paths.iter().enumerate() {
7983+
let ev = remove_first_msg_event_to_node(&expected_path[0].node.get_our_node_id(), &mut events);
7984+
7985+
total_value += route.paths[path_idx][1].fee_msat;
7986+
let payment_claimable_expected = total_value >= total_msat;
7987+
7988+
let mut payment_event = SendEvent::from_event(ev);
7989+
let mut prev_node = &nodes[src_idx];
7990+
for (idx, &node) in expected_path.iter().enumerate() {
7991+
assert_eq!(node.node.get_our_node_id(), payment_event.node_id);
7992+
7993+
node.node.handle_update_add_htlc(&prev_node.node.get_our_node_id(), &payment_event.msgs[0]);
7994+
check_added_monitors!(node, 0);
7995+
commitment_signed_dance!(node, prev_node, payment_event.commitment_msg, false);
7996+
expect_pending_htlcs_forwardable!(node);
7997+
prev_node = node;
7998+
7999+
let is_routing = idx != expected_path.len() - 1;
8000+
if is_routing {
8001+
let mut events_2 = node.node.get_and_clear_pending_msg_events();
8002+
assert_eq!(events_2.len(), 1);
8003+
check_added_monitors!(node, 1);
8004+
payment_event = SendEvent::from_event(events_2.remove(0));
8005+
assert_eq!(payment_event.msgs.len(), 1);
8006+
} else if !payment_claimable_expected {
8007+
let events_2 = node.node.get_and_clear_pending_events();
8008+
assert!(events_2.is_empty());
8009+
} else if should_fail {
8010+
expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(node, vec![HTLCDestination::FailedPayment { payment_hash: our_payment_hash }]);
8011+
check_added_monitors!(node, 1);
8012+
let msg_events = node.node.get_and_clear_pending_msg_events();
8013+
match msg_events[0] {
8014+
MessageSendEvent::UpdateHTLCs { .. } => {},
8015+
_ => panic!("Expected MessageSendEvent::UpdateHTLCs")
8016+
}
8017+
} else {
8018+
expect_payment_claimable!(node, our_payment_hash, our_payment_secret, total_value);
8019+
claim_payment_along_route(&nodes[src_idx], &expected_paths, false, our_payment_preimage);
8020+
}
8021+
}
8022+
}
8023+
8024+
}
8025+
8026+
#[test]
8027+
fn test_overshoot_mpp() {
8028+
// Test an MPP that overruns the intended amount
8029+
do_test_overshoot_mpp(&[100_000, 100_000, 101_000], 300_000, false);
8030+
// Test an MPP where the smallest HTLC amount is less than or equal to
8031+
// the difference between the total amount received and the expected total_msat
8032+
do_test_overshoot_mpp(&[100_000, 10_000, 100_000], 200_000, true);
8033+
do_test_overshoot_mpp(&[100_000, 20_000, 5_000, 90_000], 200_000, true);
8034+
// Note this is not meant to test for HTLCs that arrive after the total_msat
8035+
// has already been received - see test_dup_htlc_second_rejected
8036+
}
8037+
79288038
#[test]
79298039
fn test_simple_mpp() {
79308040
// Simple test of sending a multi-path payment.

lightning/src/ln/outbound_payment.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -915,7 +915,6 @@ impl OutboundPayments {
915915
return Err(PaymentSendFailure::PathParameterError(path_errs));
916916
}
917917
if let Some(amt_msat) = recv_value_msat {
918-
debug_assert!(amt_msat >= total_value);
919918
total_value = amt_msat;
920919
}
921920

lightning/src/util/events.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ pub enum HTLCDestination {
230230
///
231231
/// Some of the reasons may include:
232232
/// * HTLC Timeouts
233-
/// * Expected MPP amount to claim does not equal HTLC total
233+
/// * Total claimable HTLC value exceeds expected MPP amount by an entire HTLC
234234
/// * Claimable amount does not match expected amount
235235
FailedPayment {
236236
/// The payment hash of the payment we attempted to process.
@@ -929,7 +929,7 @@ pub enum Event {
929929
/// * Insufficient capacity in the outbound channel
930930
/// * While waiting to forward the HTLC, the channel it is meant to be forwarded through closes
931931
/// * When an unknown SCID is requested for forwarding a payment.
932-
/// * Claiming an amount for an MPP payment that exceeds the HTLC total
932+
/// * Claiming an MPP amount that exceeds the expected total by an entire HTLC
933933
/// * The HTLC has timed out
934934
///
935935
/// This event, however, does not get generated if an HTLC fails to meet the forwarding

0 commit comments

Comments
 (0)