Skip to content

Commit 29a6383

Browse files
valentinewallacevalentinewallace
authored
Merge pull request #3655 from shaavan/i2274
Expand `PaymentClaimable` to include all inbound channel IDs for a payment
2 parents 16307f6 + b347b05 commit 29a6383

File tree

6 files changed

+97
-37
lines changed

6 files changed

+97
-37
lines changed

‎lightning/src/events/mod.rs

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -805,10 +805,10 @@ pub enum Event {
805805
/// Information for claiming this received payment, based on whether the purpose of the
806806
/// payment is to pay an invoice or to send a spontaneous payment.
807807
purpose: PaymentPurpose,
808-
/// The `channel_id` indicating over which channel we received the payment.
809-
via_channel_id: Option<ChannelId>,
810-
/// The `user_channel_id` indicating over which channel we received the payment.
811-
via_user_channel_id: Option<u128>,
808+
/// The `(channel_id, user_channel_id)` pairs over which the payment was received.
809+
///
810+
/// This will be an incomplete vector for MPP payment events created/serialized using LDK version 0.1.0 and prior.
811+
inbound_channel_ids: Vec<(ChannelId, Option<u128>)>,
812812
/// The block height at which this payment will be failed back and will no longer be
813813
/// eligible for claiming.
814814
///
@@ -1553,7 +1553,7 @@ impl Writeable for Event {
15531553
// drop any channels which have not yet exchanged funding_signed.
15541554
},
15551555
&Event::PaymentClaimable { ref payment_hash, ref amount_msat, counterparty_skimmed_fee_msat,
1556-
ref purpose, ref receiver_node_id, ref via_channel_id, ref via_user_channel_id,
1556+
ref purpose, ref receiver_node_id, ref inbound_channel_ids,
15571557
ref claim_deadline, ref onion_fields, ref payment_id,
15581558
} => {
15591559
1u8.write(writer)?;
@@ -1587,20 +1587,30 @@ impl Writeable for Event {
15871587
}
15881588
let skimmed_fee_opt = if counterparty_skimmed_fee_msat == 0 { None }
15891589
else { Some(counterparty_skimmed_fee_msat) };
1590+
1591+
let (via_channel_id_legacy, via_user_channel_id_legacy) = match inbound_channel_ids.last() {
1592+
Some((chan_id, user_chan_id)) => (Some(*chan_id), *user_chan_id),
1593+
None => (None, None),
1594+
};
15901595
write_tlv_fields!(writer, {
15911596
(0, payment_hash, required),
15921597
(1, receiver_node_id, option),
15931598
(2, payment_secret, option),
1594-
(3, via_channel_id, option),
1599+
// Marked as legacy in version 0.2.0; superseded by `inbound_channel_ids`, which
1600+
// includes all channel IDs used in the payment instead of only the last one.
1601+
(3, via_channel_id_legacy, option),
15951602
(4, amount_msat, required),
1596-
(5, via_user_channel_id, option),
1603+
// Marked as legacy in version 0.2.0 for the same reason as `via_channel_id_legacy`;
1604+
// superseded by `via_user_channel_ids`.
1605+
(5, via_user_channel_id_legacy, option),
15971606
// Type 6 was `user_payment_id` on 0.0.103 and earlier
15981607
(7, claim_deadline, option),
15991608
(8, payment_preimage, option),
16001609
(9, onion_fields, option),
16011610
(10, skimmed_fee_opt, option),
16021611
(11, payment_context, option),
16031612
(13, payment_id, option),
1613+
(15, *inbound_channel_ids, optional_vec),
16041614
});
16051615
},
16061616
&Event::PaymentSent { ref payment_id, ref payment_preimage, ref payment_hash, ref amount_msat, ref fee_paid_msat, ref bolt12_invoice } => {
@@ -1898,41 +1908,47 @@ impl MaybeReadable for Event {
18981908
let mut counterparty_skimmed_fee_msat_opt = None;
18991909
let mut receiver_node_id = None;
19001910
let mut _user_payment_id = None::<u64>; // Used in 0.0.103 and earlier, no longer written in 0.0.116+.
1901-
let mut via_channel_id = None;
1911+
let mut via_channel_id_legacy = None;
19021912
let mut claim_deadline = None;
1903-
let mut via_user_channel_id = None;
1913+
let mut via_user_channel_id_legacy = None;
19041914
let mut onion_fields = None;
19051915
let mut payment_context = None;
19061916
let mut payment_id = None;
1917+
let mut inbound_channel_ids_opt = None;
19071918
read_tlv_fields!(reader, {
19081919
(0, payment_hash, required),
19091920
(1, receiver_node_id, option),
19101921
(2, payment_secret, option),
1911-
(3, via_channel_id, option),
1922+
(3, via_channel_id_legacy, option),
19121923
(4, amount_msat, required),
1913-
(5, via_user_channel_id, option),
1924+
(5, via_user_channel_id_legacy, option),
19141925
(6, _user_payment_id, option),
19151926
(7, claim_deadline, option),
19161927
(8, payment_preimage, option),
19171928
(9, onion_fields, option),
19181929
(10, counterparty_skimmed_fee_msat_opt, option),
19191930
(11, payment_context, option),
19201931
(13, payment_id, option),
1932+
(15, inbound_channel_ids_opt, optional_vec),
19211933
});
19221934
let purpose = match payment_secret {
19231935
Some(secret) => PaymentPurpose::from_parts(payment_preimage, secret, payment_context)
19241936
.map_err(|()| msgs::DecodeError::InvalidValue)?,
19251937
None if payment_preimage.is_some() => PaymentPurpose::SpontaneousPayment(payment_preimage.unwrap()),
19261938
None => return Err(msgs::DecodeError::InvalidValue),
19271939
};
1940+
1941+
let inbound_channel_ids = inbound_channel_ids_opt
1942+
.or_else(|| via_channel_id_legacy.map(|chan_id| vec![(chan_id, via_user_channel_id_legacy)]))
1943+
.unwrap_or_default();
1944+
19281945
Ok(Some(Event::PaymentClaimable {
19291946
receiver_node_id,
19301947
payment_hash,
19311948
amount_msat,
19321949
counterparty_skimmed_fee_msat: counterparty_skimmed_fee_msat_opt.unwrap_or(0),
19331950
purpose,
1934-
via_channel_id,
1935-
via_user_channel_id,
1951+
inbound_channel_ids,
19361952
claim_deadline,
19371953
onion_fields,
19381954
payment_id,

‎lightning/src/ln/blinded_payment_tests.rs

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,8 +247,26 @@ fn mpp_to_one_hop_blinded_path() {
247247
Some(payment_secret), ev.clone(), false, None);
248248

249249
let ev = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
250-
pass_along_path(&nodes[0], expected_route[1], amt_msat, payment_hash.clone(),
250+
let event = pass_along_path(&nodes[0], expected_route[1], amt_msat, payment_hash.clone(),
251251
Some(payment_secret), ev.clone(), true, None);
252+
253+
match event.unwrap() {
254+
Event::PaymentClaimable { mut inbound_channel_ids, .. } => {
255+
let mut expected_inbound_channel_ids = nodes[3].node.list_channels()
256+
.iter()
257+
.map(|d| (d.channel_id, Some(d.user_channel_id)))
258+
.collect::<Vec<(_, _)>>();
259+
260+
// `list_channels` returns channels in arbitrary order, so we sort both vectors
261+
// to ensure the comparison is order-agnostic.
262+
inbound_channel_ids.sort();
263+
expected_inbound_channel_ids.sort();
264+
265+
assert_eq!(inbound_channel_ids, expected_inbound_channel_ids);
266+
}
267+
_ => panic!("Unexpected event"),
268+
}
269+
252270
claim_payment_along_route(
253271
ClaimAlongRouteArgs::new(&nodes[0], expected_route, payment_preimage)
254272
);

‎lightning/src/ln/chanmon_update_fail_tests.rs

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
125125
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
126126
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
127127
let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
128+
let user_channel_id = nodes[1].node.list_channels()[0].user_channel_id;
128129

129130
let (route, payment_hash_1, payment_preimage_1, payment_secret_1) = get_route_and_payment_hash!(&nodes[0], nodes[1], 1000000);
130131

@@ -166,11 +167,11 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
166167
let events_3 = nodes[1].node.get_and_clear_pending_events();
167168
assert_eq!(events_3.len(), 1);
168169
match events_3[0] {
169-
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
170+
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
170171
assert_eq!(payment_hash_1, *payment_hash);
171172
assert_eq!(amount_msat, 1_000_000);
172173
assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
173-
assert_eq!(via_channel_id, Some(channel_id));
174+
assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(user_channel_id))]);
174175
match &purpose {
175176
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
176177
assert!(payment_preimage.is_none());
@@ -248,6 +249,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
248249
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
249250
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
250251
let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
252+
let user_channel_id = nodes[1].node.list_channels()[0].user_channel_id;
251253

252254
let (payment_preimage_1, payment_hash_1, ..) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
253255

@@ -548,11 +550,11 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
548550
let events_5 = nodes[1].node.get_and_clear_pending_events();
549551
assert_eq!(events_5.len(), 1);
550552
match events_5[0] {
551-
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
553+
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
552554
assert_eq!(payment_hash_2, *payment_hash);
553555
assert_eq!(amount_msat, 1_000_000);
554556
assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
555-
assert_eq!(via_channel_id, Some(channel_id));
557+
assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(user_channel_id))]);
556558
match &purpose {
557559
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
558560
assert!(payment_preimage.is_none());
@@ -602,6 +604,7 @@ fn test_monitor_update_fail_cs() {
602604
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
603605
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
604606
let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
607+
let user_channel_id = nodes[1].node.list_channels()[0].user_channel_id;
605608

606609
let (route, our_payment_hash, payment_preimage, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 1000000);
607610
{
@@ -666,11 +669,11 @@ fn test_monitor_update_fail_cs() {
666669
let events = nodes[1].node.get_and_clear_pending_events();
667670
assert_eq!(events.len(), 1);
668671
match events[0] {
669-
Event::PaymentClaimable { payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
672+
Event::PaymentClaimable { payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
670673
assert_eq!(payment_hash, our_payment_hash);
671674
assert_eq!(amount_msat, 1_000_000);
672675
assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
673-
assert_eq!(via_channel_id, Some(channel_id));
676+
assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(user_channel_id))]);
674677
match &purpose {
675678
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
676679
assert!(payment_preimage.is_none());
@@ -1679,12 +1682,11 @@ fn test_monitor_update_fail_claim() {
16791682
let events = nodes[0].node.get_and_clear_pending_events();
16801683
assert_eq!(events.len(), 2);
16811684
match events[0] {
1682-
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, via_user_channel_id, .. } => {
1685+
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
16831686
assert_eq!(payment_hash_2, *payment_hash);
16841687
assert_eq!(1_000_000, amount_msat);
16851688
assert_eq!(receiver_node_id.unwrap(), nodes[0].node.get_our_node_id());
1686-
assert_eq!(via_channel_id, Some(channel_id));
1687-
assert_eq!(via_user_channel_id, Some(42));
1689+
assert_eq!(*inbound_channel_ids.last().unwrap(), (channel_id, Some(42)));
16881690
match &purpose {
16891691
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
16901692
assert!(payment_preimage.is_none());
@@ -1696,11 +1698,11 @@ fn test_monitor_update_fail_claim() {
16961698
_ => panic!("Unexpected event"),
16971699
}
16981700
match events[1] {
1699-
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
1701+
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
17001702
assert_eq!(payment_hash_3, *payment_hash);
17011703
assert_eq!(1_000_000, amount_msat);
17021704
assert_eq!(receiver_node_id.unwrap(), nodes[0].node.get_our_node_id());
1703-
assert_eq!(via_channel_id, Some(channel_id));
1705+
assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(42))]);
17041706
match &purpose {
17051707
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
17061708
assert!(payment_preimage.is_none());

‎lightning/src/ln/channelmanager.rs

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -930,6 +930,15 @@ impl ClaimablePayment {
930930
self.htlcs.iter().map(|htlc| (htlc.prev_hop.channel_id, htlc.prev_hop.htlc_id))
931931
)
932932
}
933+
934+
/// Returns the inbound `(channel_id, user_channel_id)` pairs for all HTLCs associated with the payment.
935+
///
936+
/// Note: The `user_channel_id` will be `None` for HTLCs created using LDK version 0.0.117 or prior.
937+
fn inbound_channel_ids(&self) -> Vec<(ChannelId, Option<u128>)> {
938+
self.htlcs.iter().map(|htlc| {
939+
(htlc.prev_hop.channel_id, htlc.prev_hop.user_channel_id)
940+
}).collect()
941+
}
933942
}
934943

935944
/// Represent the channel funding transaction type.
@@ -6353,8 +6362,7 @@ where
63536362
purpose: $purpose,
63546363
amount_msat,
63556364
counterparty_skimmed_fee_msat,
6356-
via_channel_id: Some(prev_channel_id),
6357-
via_user_channel_id: Some(prev_user_channel_id),
6365+
inbound_channel_ids: claimable_payment.inbound_channel_ids(),
63586366
claim_deadline: Some(earliest_expiry - HTLC_FAIL_BACK_BUFFER),
63596367
onion_fields: claimable_payment.onion_fields.clone(),
63606368
payment_id: Some(payment_id),
@@ -11032,7 +11040,18 @@ where
1103211040
let events = core::cell::RefCell::new(Vec::new());
1103311041
let event_handler = |event: events::Event| Ok(events.borrow_mut().push(event));
1103411042
self.process_pending_events(&event_handler);
11035-
events.into_inner()
11043+
let collected_events = events.into_inner();
11044+
11045+
// To expand the coverage and make sure all events are properly serialised and deserialised,
11046+
// we test all generated events round-trip:
11047+
for event in &collected_events {
11048+
let ser = event.encode();
11049+
if let Some(deser) = events::Event::read(&mut &ser[..]).expect("event should deserialize") {
11050+
assert_eq!(&deser, event, "event should roundtrip correctly");
11051+
}
11052+
}
11053+
11054+
collected_events
1103611055
}
1103711056

1103811057
#[cfg(feature = "_test_utils")]

‎lightning/src/ln/functional_test_utils.rs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2770,7 +2770,7 @@ pub fn do_pass_along_path<'a, 'b, 'c>(args: PassAlongPathArgs) -> Option<Event>
27702770
assert_eq!(events_2.len(), 1);
27712771
match &events_2[0] {
27722772
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat,
2773-
receiver_node_id, ref via_channel_id, ref via_user_channel_id,
2773+
receiver_node_id, ref inbound_channel_ids,
27742774
claim_deadline, onion_fields, ..
27752775
} => {
27762776
assert_eq!(our_payment_hash, *payment_hash);
@@ -2804,8 +2804,11 @@ pub fn do_pass_along_path<'a, 'b, 'c>(args: PassAlongPathArgs) -> Option<Event>
28042804
},
28052805
}
28062806
assert_eq!(*amount_msat, recv_value);
2807-
assert!(node.node.list_channels().iter().any(|details| details.channel_id == via_channel_id.unwrap()));
2808-
assert!(node.node.list_channels().iter().any(|details| details.user_channel_id == via_user_channel_id.unwrap()));
2807+
let channels = node.node.list_channels();
2808+
for (chan_id, user_chan_id) in inbound_channel_ids {
2809+
let chan = channels.iter().find(|details| &details.channel_id == chan_id).unwrap();
2810+
assert_eq!(*user_chan_id, Some(chan.user_channel_id));
2811+
}
28092812
assert!(claim_deadline.unwrap() > node.best_block_info().1);
28102813
},
28112814
_ => panic!("Unexpected event"),

‎lightning/src/ln/functional_tests.rs

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1932,6 +1932,7 @@ pub fn test_channel_reserve_holding_cell_htlcs() {
19321932
let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
19331933
let chan_1 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 190000, 1001);
19341934
let chan_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 190000, 1001);
1935+
let chan_2_user_id = nodes[2].node.list_channels()[0].user_channel_id;
19351936

19361937
let mut stat01 = get_channel_value_stat!(nodes[0], nodes[1], chan_1.2);
19371938
let mut stat11 = get_channel_value_stat!(nodes[1], nodes[0], chan_1.2);
@@ -2126,11 +2127,11 @@ pub fn test_channel_reserve_holding_cell_htlcs() {
21262127
let events = nodes[2].node.get_and_clear_pending_events();
21272128
assert_eq!(events.len(), 2);
21282129
match events[0] {
2129-
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
2130+
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
21302131
assert_eq!(our_payment_hash_21, *payment_hash);
21312132
assert_eq!(recv_value_21, amount_msat);
21322133
assert_eq!(nodes[2].node.get_our_node_id(), receiver_node_id.unwrap());
2133-
assert_eq!(via_channel_id, Some(chan_2.2));
2134+
assert_eq!(*inbound_channel_ids, vec![(chan_2.2, Some(chan_2_user_id))]);
21342135
match &purpose {
21352136
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
21362137
assert!(payment_preimage.is_none());
@@ -2142,11 +2143,11 @@ pub fn test_channel_reserve_holding_cell_htlcs() {
21422143
_ => panic!("Unexpected event"),
21432144
}
21442145
match events[1] {
2145-
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
2146+
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
21462147
assert_eq!(our_payment_hash_22, *payment_hash);
21472148
assert_eq!(recv_value_22, amount_msat);
21482149
assert_eq!(nodes[2].node.get_our_node_id(), receiver_node_id.unwrap());
2149-
assert_eq!(via_channel_id, Some(chan_2.2));
2150+
assert_eq!(*inbound_channel_ids, vec![(chan_2.2, Some(chan_2_user_id))]);
21502151
match &purpose {
21512152
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
21522153
assert!(payment_preimage.is_none());
@@ -4259,6 +4260,7 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
42594260
} else {
42604261
create_announced_chan_between_nodes(&nodes, 0, 1).2
42614262
};
4263+
let user_channel_id = nodes[1].node.list_channels()[0].user_channel_id;
42624264

42634265
let (route, payment_hash_1, payment_preimage_1, payment_secret_1) = get_route_and_payment_hash!(nodes[0], nodes[1], 1_000_000);
42644266

@@ -4372,11 +4374,11 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
43724374
let events_2 = nodes[1].node.get_and_clear_pending_events();
43734375
assert_eq!(events_2.len(), 1);
43744376
match events_2[0] {
4375-
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
4377+
Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref inbound_channel_ids, .. } => {
43764378
assert_eq!(payment_hash_1, *payment_hash);
43774379
assert_eq!(amount_msat, 1_000_000);
43784380
assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
4379-
assert_eq!(via_channel_id, Some(channel_id));
4381+
assert_eq!(*inbound_channel_ids, vec![(channel_id, Some(user_channel_id))]);
43804382
match &purpose {
43814383
PaymentPurpose::Bolt11InvoicePayment { payment_preimage, payment_secret, .. } => {
43824384
assert!(payment_preimage.is_none());

0 commit comments

Comments
 (0)