Skip to content

Commit f2bbcb3

Browse files
committed
Use onion amount amt_to_forward for MPP set calculation
If routing nodes take less fees and pay the final node more than `amt_to_forward`, the receiver may see that `total_msat` has been met before all of the sender's intended HTLCs have arrived. The receiver may then prematurely claim the payment and release the payment hash, allowing routing nodes to claim the remaining HTLCs. Using the onion value `amt_to_forward` to determine when `total_msat` has been met allows the sender to control the set total.
1 parent fd21871 commit f2bbcb3

File tree

2 files changed

+115
-8
lines changed

2 files changed

+115
-8
lines changed

lightning/src/ln/channelmanager.rs

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,9 @@ struct ClaimableHTLC {
191191
cltv_expiry: u32,
192192
/// The amount (in msats) of this MPP part
193193
value: u64,
194+
/// The amount (in msats) that the sender intended to be sent in this MPP
195+
/// part (used for validating total MPP amount)
196+
sender_intended_value: u64,
194197
onion_payload: OnionPayload,
195198
timer_ticks: u8,
196199
/// The sum total of all MPP parts
@@ -2159,7 +2162,7 @@ where
21592162
payment_hash,
21602163
incoming_shared_secret: shared_secret,
21612164
incoming_amt_msat: Some(amt_msat),
2162-
outgoing_amt_msat: amt_msat,
2165+
outgoing_amt_msat: hop_data.amt_to_forward,
21632166
outgoing_cltv_value: hop_data.outgoing_cltv_value,
21642167
})
21652168
}
@@ -3232,7 +3235,7 @@ where
32323235
HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo {
32333236
prev_short_channel_id, prev_htlc_id, prev_funding_outpoint, prev_user_channel_id,
32343237
forward_info: PendingHTLCInfo {
3235-
routing, incoming_shared_secret, payment_hash, outgoing_amt_msat, ..
3238+
routing, incoming_shared_secret, payment_hash, incoming_amt_msat, outgoing_amt_msat, ..
32363239
}
32373240
}) => {
32383241
let (cltv_expiry, onion_payload, payment_data, phantom_shared_secret) = match routing {
@@ -3254,7 +3257,11 @@ where
32543257
incoming_packet_shared_secret: incoming_shared_secret,
32553258
phantom_shared_secret,
32563259
},
3257-
value: outgoing_amt_msat,
3260+
// We differentiate the received value from the sender intended value
3261+
// if possible so that we don't prematurely mark MPP payments complete
3262+
// if routing nodes overpay
3263+
value: incoming_amt_msat.unwrap_or(outgoing_amt_msat),
3264+
sender_intended_value: outgoing_amt_msat,
32583265
timer_ticks: 0,
32593266
total_msat: if let Some(data) = &payment_data { data.total_msat } else { outgoing_amt_msat },
32603267
cltv_expiry,
@@ -3309,9 +3316,9 @@ where
33093316
continue
33103317
}
33113318
}
3312-
let mut total_value = claimable_htlc.value;
3319+
let mut total_value = claimable_htlc.sender_intended_value;
33133320
for htlc in htlcs.iter() {
3314-
total_value += htlc.value;
3321+
total_value += htlc.sender_intended_value;
33153322
match &htlc.onion_payload {
33163323
OnionPayload::Invoice { .. } => {
33173324
if htlc.total_msat != $payment_data.total_msat {
@@ -3326,7 +3333,7 @@ where
33263333
}
33273334
if total_value >= msgs::MAX_VALUE_MSAT {
33283335
fail_htlc!(claimable_htlc, payment_hash);
3329-
} else if total_value - claimable_htlc.value >= $payment_data.total_msat {
3336+
} else if total_value - claimable_htlc.sender_intended_value >= $payment_data.total_msat {
33303337
log_trace!(self.logger, "Failing HTLC with payment_hash {} as payment is already claimable",
33313338
log_bytes!(payment_hash.0));
33323339
fail_htlc!(claimable_htlc, payment_hash);
@@ -3337,7 +3344,7 @@ where
33373344
receiver_node_id: Some(receiver_node_id),
33383345
payment_hash,
33393346
purpose: purpose(),
3340-
amount_msat: total_value,
3347+
amount_msat: htlcs.iter().map(|htlc| htlc.value).sum(),
33413348
via_channel_id: Some(prev_channel_id),
33423349
via_user_channel_id: Some(prev_user_channel_id),
33433350
});
@@ -3391,13 +3398,14 @@ where
33913398
}
33923399
match claimable_payments.claimable_htlcs.entry(payment_hash) {
33933400
hash_map::Entry::Vacant(e) => {
3401+
let amount_msat = claimable_htlc.value;
33943402
let purpose = events::PaymentPurpose::SpontaneousPayment(preimage);
33953403
e.insert((purpose.clone(), vec![claimable_htlc]));
33963404
let prev_channel_id = prev_funding_outpoint.to_channel_id();
33973405
new_events.push(events::Event::PaymentClaimable {
33983406
receiver_node_id: Some(receiver_node_id),
33993407
payment_hash,
3400-
amount_msat: outgoing_amt_msat,
3408+
amount_msat,
34013409
purpose,
34023410
via_channel_id: Some(prev_channel_id),
34033411
via_user_channel_id: Some(prev_user_channel_id),
@@ -6767,6 +6775,7 @@ impl Writeable for ClaimableHTLC {
67676775
(0, self.prev_hop, required),
67686776
(1, self.total_msat, required),
67696777
(2, self.value, required),
6778+
(3, self.sender_intended_value, required),
67706779
(4, payment_data, option),
67716780
(6, self.cltv_expiry, required),
67726781
(8, keysend_preimage, option),
@@ -6779,6 +6788,7 @@ impl Readable for ClaimableHTLC {
67796788
fn read<R: Read>(reader: &mut R) -> Result<Self, DecodeError> {
67806789
let mut prev_hop = crate::util::ser::RequiredWrapper(None);
67816790
let mut value = 0;
6791+
let mut sender_intended_value = None;
67826792
let mut payment_data: Option<msgs::FinalOnionHopData> = None;
67836793
let mut cltv_expiry = 0;
67846794
let mut total_msat = None;
@@ -6787,6 +6797,7 @@ impl Readable for ClaimableHTLC {
67876797
(0, prev_hop, required),
67886798
(1, total_msat, option),
67896799
(2, value, required),
6800+
(3, sender_intended_value, option),
67906801
(4, payment_data, option),
67916802
(6, cltv_expiry, required),
67926803
(8, keysend_preimage, option)
@@ -6815,6 +6826,7 @@ impl Readable for ClaimableHTLC {
68156826
prev_hop: prev_hop.0.unwrap(),
68166827
timer_ticks: 0,
68176828
value,
6829+
sender_intended_value: sender_intended_value.unwrap_or(value),
68186830
total_msat: total_msat.unwrap(),
68196831
onion_payload,
68206832
cltv_expiry,

lightning/src/ln/functional_tests.rs

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

7928+
#[test]
7929+
fn test_onion_value_mpp_set_calculation() {
7930+
// Test that we use the onion value `amt_to_forward` when
7931+
// calculating whether we've reached the `total_msat` of an MPP
7932+
// by having a routing node forward more than `amt_to_forward`
7933+
// and checking that the receiving node doesn't generate
7934+
// a PaymentClaimable event too early
7935+
let node_count = 4;
7936+
let chanmon_cfgs = create_chanmon_cfgs(node_count);
7937+
let node_cfgs = create_node_cfgs(node_count, &chanmon_cfgs);
7938+
let node_chanmgrs = create_node_chanmgrs(node_count, &node_cfgs, &vec![None; node_count]);
7939+
let mut nodes = create_network(node_count, &node_cfgs, &node_chanmgrs);
7940+
7941+
let chan_1_id = create_announced_chan_between_nodes(&nodes, 0, 1).0.contents.short_channel_id;
7942+
let chan_2_id = create_announced_chan_between_nodes(&nodes, 0, 2).0.contents.short_channel_id;
7943+
let chan_3_id = create_announced_chan_between_nodes(&nodes, 1, 3).0.contents.short_channel_id;
7944+
let chan_4_id = create_announced_chan_between_nodes(&nodes, 2, 3).0.contents.short_channel_id;
7945+
7946+
let total_msat = 100_000;
7947+
let expected_paths: &[&[&Node]] = &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]];
7948+
let (mut route, our_payment_hash, our_payment_preimage, our_payment_secret) = get_route_and_payment_hash!(&nodes[0], nodes[3], total_msat);
7949+
let sample_path = route.paths.pop().unwrap();
7950+
7951+
let mut path_1 = sample_path.clone();
7952+
path_1[0].pubkey = nodes[1].node.get_our_node_id();
7953+
path_1[0].short_channel_id = chan_1_id;
7954+
path_1[1].pubkey = nodes[3].node.get_our_node_id();
7955+
path_1[1].short_channel_id = chan_3_id;
7956+
path_1[1].fee_msat = 100_000;
7957+
route.paths.push(path_1);
7958+
7959+
let mut path_2 = sample_path.clone();
7960+
path_2[0].pubkey = nodes[2].node.get_our_node_id();
7961+
path_2[0].short_channel_id = chan_2_id;
7962+
path_2[1].pubkey = nodes[3].node.get_our_node_id();
7963+
path_2[1].short_channel_id = chan_4_id;
7964+
path_2[1].fee_msat = 1_000;
7965+
route.paths.push(path_2);
7966+
7967+
// Send payment
7968+
let payment_id = PaymentId(nodes[0].keys_manager.backing.get_secure_random_bytes());
7969+
let onion_session_privs = nodes[0].node.test_add_new_pending_payment(our_payment_hash, Some(our_payment_secret), payment_id, &route).unwrap();
7970+
nodes[0].node.test_send_payment_internal(&route, our_payment_hash, &Some(our_payment_secret), None, payment_id, Some(total_msat), onion_session_privs).unwrap();
7971+
check_added_monitors!(nodes[0], expected_paths.len());
7972+
7973+
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
7974+
assert_eq!(events.len(), expected_paths.len());
7975+
7976+
// First path
7977+
let ev = remove_first_msg_event_to_node(&expected_paths[0][0].node.get_our_node_id(), &mut events);
7978+
let mut payment_event = SendEvent::from_event(ev);
7979+
let mut prev_node = &nodes[0];
7980+
7981+
for (idx, &node) in expected_paths[0].iter().enumerate() {
7982+
assert_eq!(node.node.get_our_node_id(), payment_event.node_id);
7983+
7984+
if idx == 0 { // routing node
7985+
let session_priv = [3; 32];
7986+
let height = nodes[0].best_block_info().1;
7987+
let session_priv = SecretKey::from_slice(&session_priv).unwrap();
7988+
let mut onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
7989+
let (mut onion_payloads, _, _) = onion_utils::build_onion_payloads(&route.paths[0], 100_000, &Some(our_payment_secret), height + 1, &None).unwrap();
7990+
// Edit amt_to_forward to simulate the sender having set
7991+
// the final amount and the routing node taking less fee
7992+
onion_payloads[1].amt_to_forward = 99_000;
7993+
let new_onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash);
7994+
payment_event.msgs[0].onion_routing_packet = new_onion_packet;
7995+
}
7996+
7997+
node.node.handle_update_add_htlc(&prev_node.node.get_our_node_id(), &payment_event.msgs[0]);
7998+
check_added_monitors!(node, 0);
7999+
commitment_signed_dance!(node, prev_node, payment_event.commitment_msg, false);
8000+
expect_pending_htlcs_forwardable!(node);
8001+
8002+
if idx == 0 {
8003+
let mut events_2 = node.node.get_and_clear_pending_msg_events();
8004+
assert_eq!(events_2.len(), 1);
8005+
check_added_monitors!(node, 1);
8006+
payment_event = SendEvent::from_event(events_2.remove(0));
8007+
assert_eq!(payment_event.msgs.len(), 1);
8008+
} else {
8009+
let events_2 = node.node.get_and_clear_pending_events();
8010+
assert!(events_2.is_empty());
8011+
}
8012+
8013+
prev_node = node;
8014+
}
8015+
8016+
// Second path
8017+
let ev = remove_first_msg_event_to_node(&expected_paths[1][0].node.get_our_node_id(), &mut events);
8018+
pass_along_path(&nodes[0], expected_paths[1], 101_000, our_payment_hash.clone(), Some(our_payment_secret), ev, true, None);
8019+
8020+
claim_payment_along_route(&nodes[0], expected_paths, false, our_payment_preimage);
8021+
}
8022+
79288023
fn do_test_overshoot_mpp(msat_amounts: &[u64], total_msat: u64) {
79298024

79308025
let routing_node_count = msat_amounts.len();

0 commit comments

Comments
 (0)