Skip to content

Commit 38e543e

Browse files
jkczyzjkczyz
committed
Generate PaymentPathSuccessful event for each path
A single PaymentSent event is generated when a payment is fulfilled. This is occurs when the preimage is revealed on the first claimed HTLC. For subsequent HTLCs, the event is not generated. In order to score channels involved with a successful payments, the scorer must be notified of each successful path involved in the payment. Add a PaymentPathSuccessful event for this purpose. Generate it whenever a part is removed from a pending outbound payment. This avoids duplicate events when reconnecting to a peer.
1 parent 8d886ee commit 38e543e

File tree

8 files changed

+189
-120
lines changed

8 files changed

+189
-120
lines changed

‎lightning/src/ln/chanmon_update_fail_tests.rs

Lines changed: 27 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -551,6 +551,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
551551
nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_revoke_and_ack);
552552
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
553553
check_added_monitors!(nodes[0], 1);
554+
expect_payment_path_successful!(nodes[0]);
554555

555556
expect_pending_htlcs_forwardable!(nodes[1]);
556557

@@ -1090,12 +1091,12 @@ fn test_monitor_update_fail_reestablish() {
10901091
let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
10911092
create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
10921093

1093-
let (our_payment_preimage, _, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000);
1094+
let (payment_preimage, _, _) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000);
10941095

10951096
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
10961097
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
10971098

1098-
assert!(nodes[2].node.claim_funds(our_payment_preimage));
1099+
assert!(nodes[2].node.claim_funds(payment_preimage));
10991100
check_added_monitors!(nodes[2], 1);
11001101
let mut updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
11011102
assert!(updates.update_add_htlcs.is_empty());
@@ -1159,13 +1160,7 @@ fn test_monitor_update_fail_reestablish() {
11591160
assert_eq!(updates.update_fulfill_htlcs.len(), 1);
11601161
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]);
11611162
commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, false);
1162-
1163-
let events = nodes[0].node.get_and_clear_pending_events();
1164-
assert_eq!(events.len(), 1);
1165-
match events[0] {
1166-
Event::PaymentSent { payment_preimage, .. } => assert_eq!(payment_preimage, our_payment_preimage),
1167-
_ => panic!("Unexpected event"),
1168-
}
1163+
expect_payment_sent!(nodes[0], payment_preimage);
11691164
}
11701165

11711166
#[test]
@@ -1300,7 +1295,7 @@ fn claim_while_disconnected_monitor_update_fail() {
13001295
let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).2;
13011296

13021297
// Forward a payment for B to claim
1303-
let (payment_preimage_1, payment_hash_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
1298+
let (payment_preimage_1, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
13041299

13051300
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
13061301
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
@@ -1395,16 +1390,7 @@ fn claim_while_disconnected_monitor_update_fail() {
13951390

13961391
nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa);
13971392
check_added_monitors!(nodes[0], 1);
1398-
1399-
let events = nodes[0].node.get_and_clear_pending_events();
1400-
assert_eq!(events.len(), 1);
1401-
match events[0] {
1402-
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
1403-
assert_eq!(*payment_preimage, payment_preimage_1);
1404-
assert_eq!(*payment_hash, payment_hash_1);
1405-
},
1406-
_ => panic!("Unexpected event"),
1407-
}
1393+
expect_payment_sent!(nodes[0], payment_preimage_1);
14081394

14091395
claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
14101396
}
@@ -1766,7 +1752,7 @@ fn monitor_update_claim_fail_no_response() {
17661752
let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).2;
17671753

17681754
// Forward a payment for B to claim
1769-
let (payment_preimage_1, payment_hash_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
1755+
let (payment_preimage_1, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
17701756

17711757
// Now start forwarding a second payment, skipping the last RAA so B is in AwaitingRAA
17721758
let (route, payment_hash_2, payment_preimage_2, payment_secret_2) = get_route_and_payment_hash!(nodes[0], nodes[1], 1000000);
@@ -1802,16 +1788,7 @@ fn monitor_update_claim_fail_no_response() {
18021788
let bs_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
18031789
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.update_fulfill_htlcs[0]);
18041790
commitment_signed_dance!(nodes[0], nodes[1], bs_updates.commitment_signed, false);
1805-
1806-
let events = nodes[0].node.get_and_clear_pending_events();
1807-
assert_eq!(events.len(), 1);
1808-
match events[0] {
1809-
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
1810-
assert_eq!(*payment_preimage, payment_preimage_1);
1811-
assert_eq!(*payment_hash, payment_hash_1);
1812-
},
1813-
_ => panic!("Unexpected event"),
1814-
}
1791+
expect_payment_sent!(nodes[0], payment_preimage_1);
18151792

18161793
claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
18171794
}
@@ -2323,7 +2300,7 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) {
23232300
assert!(updates.update_fee.is_none());
23242301
assert_eq!(updates.update_fulfill_htlcs.len(), 1);
23252302
nodes[1].node.handle_update_fulfill_htlc(&nodes[0].node.get_our_node_id(), &updates.update_fulfill_htlcs[0]);
2326-
expect_payment_sent!(nodes[1], payment_preimage_0);
2303+
expect_payment_sent_without_paths!(nodes[1], payment_preimage_0);
23272304
assert_eq!(updates.update_add_htlcs.len(), 1);
23282305
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &updates.update_add_htlcs[0]);
23292306
updates.commitment_signed
@@ -2342,7 +2319,18 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) {
23422319

23432320
commitment_signed_dance!(nodes[1], nodes[0], (), false, true, false);
23442321

2345-
expect_pending_htlcs_forwardable!(nodes[1]);
2322+
let events = nodes[1].node.get_and_clear_pending_events();
2323+
assert_eq!(events.len(), 2);
2324+
match events[0] {
2325+
Event::PendingHTLCsForwardable { .. } => { },
2326+
_ => panic!("Unexpected event"),
2327+
};
2328+
match events[1] {
2329+
Event::PaymentPathSuccessful { .. } => { },
2330+
_ => panic!("Unexpected event"),
2331+
};
2332+
2333+
nodes[1].node.process_pending_htlc_forwards();
23462334
expect_payment_received!(nodes[1], payment_hash_2, payment_secret_2, 100000);
23472335

23482336
claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1);
@@ -2427,9 +2415,10 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_f
24272415
bs_updates = Some(get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()));
24282416
assert_eq!(bs_updates.as_ref().unwrap().update_fulfill_htlcs.len(), 1);
24292417
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.as_ref().unwrap().update_fulfill_htlcs[0]);
2430-
expect_payment_sent!(nodes[0], payment_preimage);
2418+
expect_payment_sent_without_paths!(nodes[0], payment_preimage);
24312419
if htlc_status == HTLCStatusAtDupClaim::Cleared {
24322420
commitment_signed_dance!(nodes[0], nodes[1], &bs_updates.as_ref().unwrap().commitment_signed, false);
2421+
expect_payment_path_successful!(nodes[0]);
24332422
}
24342423
} else {
24352424
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
@@ -2453,10 +2442,11 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_f
24532442
bs_updates = Some(get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()));
24542443
assert_eq!(bs_updates.as_ref().unwrap().update_fulfill_htlcs.len(), 1);
24552444
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.as_ref().unwrap().update_fulfill_htlcs[0]);
2456-
expect_payment_sent!(nodes[0], payment_preimage);
2445+
expect_payment_sent_without_paths!(nodes[0], payment_preimage);
24572446
}
24582447
if htlc_status != HTLCStatusAtDupClaim::Cleared {
24592448
commitment_signed_dance!(nodes[0], nodes[1], &bs_updates.as_ref().unwrap().commitment_signed, false);
2449+
expect_payment_path_successful!(nodes[0]);
24602450
}
24612451
}
24622452

@@ -2620,7 +2610,7 @@ fn double_temp_error() {
26202610
assert_eq!(node_id, nodes[0].node.get_our_node_id());
26212611
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_1);
26222612
check_added_monitors!(nodes[0], 0);
2623-
expect_payment_sent!(nodes[0], payment_preimage_1);
2613+
expect_payment_sent_without_paths!(nodes[0], payment_preimage_1);
26242614
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &commitment_signed_b1);
26252615
check_added_monitors!(nodes[0], 1);
26262616
nodes[0].node.process_pending_htlc_forwards();
@@ -2658,6 +2648,7 @@ fn double_temp_error() {
26582648
};
26592649
nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &raa_b2);
26602650
check_added_monitors!(nodes[0], 1);
2651+
expect_payment_path_successful!(nodes[0]);
26612652

26622653
nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_fulfill_2);
26632654
check_added_monitors!(nodes[0], 0);

‎lightning/src/ln/channelmanager.rs

Lines changed: 51 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3491,14 +3491,22 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
34913491
}
34923492

34933493
fn finalize_claims(&self, mut sources: Vec<HTLCSource>) {
3494+
let mut pending_events = self.pending_events.lock().unwrap();
34943495
for source in sources.drain(..) {
3495-
if let HTLCSource::OutboundRoute { session_priv, payment_id, .. } = source {
3496+
if let HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } = source {
34963497
let mut session_priv_bytes = [0; 32];
34973498
session_priv_bytes.copy_from_slice(&session_priv[..]);
34983499
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
34993500
if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
35003501
assert!(payment.get().is_fulfilled());
3501-
payment.get_mut().remove(&session_priv_bytes, None);
3502+
if payment.get_mut().remove(&session_priv_bytes, None) {
3503+
pending_events.push(
3504+
events::Event::PaymentPathSuccessful {
3505+
payment_id,
3506+
path,
3507+
}
3508+
);
3509+
}
35023510
if payment.get().remaining_parts() == 0 {
35033511
payment.remove();
35043512
}
@@ -3515,32 +3523,41 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
35153523
session_priv_bytes.copy_from_slice(&session_priv[..]);
35163524
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
35173525
if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
3518-
let found_payment = !payment.get().is_fulfilled();
3519-
let fee_paid_msat = payment.get().get_pending_fee_msat();
3520-
payment.get_mut().mark_fulfilled();
3526+
let mut pending_events = self.pending_events.lock().unwrap();
3527+
if !payment.get().is_fulfilled() {
3528+
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
3529+
let fee_paid_msat = payment.get().get_pending_fee_msat();
3530+
pending_events.push(
3531+
events::Event::PaymentSent {
3532+
payment_id: Some(payment_id),
3533+
payment_preimage,
3534+
payment_hash,
3535+
fee_paid_msat,
3536+
}
3537+
);
3538+
payment.get_mut().mark_fulfilled();
3539+
}
3540+
35213541
if from_onchain {
35223542
// We currently immediately remove HTLCs which were fulfilled on-chain.
35233543
// This could potentially lead to removing a pending payment too early,
35243544
// with a reorg of one block causing us to re-add the fulfilled payment on
35253545
// restart.
35263546
// TODO: We should have a second monitor event that informs us of payments
35273547
// irrevocably fulfilled.
3528-
payment.get_mut().remove(&session_priv_bytes, Some(&path));
3548+
if payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
3549+
pending_events.push(
3550+
events::Event::PaymentPathSuccessful {
3551+
payment_id,
3552+
path,
3553+
}
3554+
);
3555+
}
3556+
35293557
if payment.get().remaining_parts() == 0 {
35303558
payment.remove();
35313559
}
35323560
}
3533-
if found_payment {
3534-
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
3535-
self.pending_events.lock().unwrap().push(
3536-
events::Event::PaymentSent {
3537-
payment_id: Some(payment_id),
3538-
payment_preimage,
3539-
payment_hash: payment_hash,
3540-
fee_paid_msat,
3541-
}
3542-
);
3543-
}
35443561
} else {
35453562
log_trace!(self.logger, "Received duplicative fulfill for HTLC with payment_preimage {}", log_bytes!(payment_preimage.0));
35463563
}
@@ -6325,9 +6342,10 @@ mod tests {
63256342
nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_third_raa);
63266343
check_added_monitors!(nodes[0], 1);
63276344

6328-
// Note that successful MPP payments will generate 1 event upon the first path's success. No
6329-
// further events will be generated for subsequence path successes.
6345+
// Note that successful MPP payments will generate a single PaymentSent event upon the first
6346+
// path's success and a PaymentPathSuccessful event for each path's success.
63306347
let events = nodes[0].node.get_and_clear_pending_events();
6348+
assert_eq!(events.len(), 3);
63316349
match events[0] {
63326350
Event::PaymentSent { payment_id: ref id, payment_preimage: ref preimage, payment_hash: ref hash, .. } => {
63336351
assert_eq!(Some(payment_id), *id);
@@ -6336,6 +6354,20 @@ mod tests {
63366354
},
63376355
_ => panic!("Unexpected event"),
63386356
}
6357+
match events[1] {
6358+
Event::PaymentPathSuccessful { payment_id: ref actual_payment_id, ref path } => {
6359+
assert_eq!(payment_id, *actual_payment_id);
6360+
assert_eq!(route.paths[0], *path);
6361+
},
6362+
_ => panic!("Unexpected event"),
6363+
}
6364+
match events[2] {
6365+
Event::PaymentPathSuccessful { payment_id: ref actual_payment_id, ref path } => {
6366+
assert_eq!(payment_id, *actual_payment_id);
6367+
assert_eq!(route.paths[0], *path);
6368+
},
6369+
_ => panic!("Unexpected event"),
6370+
}
63396371
}
63406372

63416373
#[test]

‎lightning/src/ln/functional_test_utils.rs

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1079,14 +1079,31 @@ macro_rules! expect_payment_received {
10791079
}
10801080
}
10811081

1082+
#[cfg(test)]
1083+
macro_rules! expect_payment_sent_without_paths {
1084+
($node: expr, $expected_payment_preimage: expr) => {
1085+
expect_payment_sent!($node, $expected_payment_preimage, None::<u64>, false);
1086+
};
1087+
($node: expr, $expected_payment_preimage: expr, $expected_fee_msat_opt: expr) => {
1088+
expect_payment_sent!($node, $expected_payment_preimage, $expected_fee_msat_opt, false);
1089+
}
1090+
}
1091+
10821092
macro_rules! expect_payment_sent {
10831093
($node: expr, $expected_payment_preimage: expr) => {
1084-
expect_payment_sent!($node, $expected_payment_preimage, None::<u64>);
1094+
expect_payment_sent!($node, $expected_payment_preimage, None::<u64>, true);
10851095
};
10861096
($node: expr, $expected_payment_preimage: expr, $expected_fee_msat_opt: expr) => {
1097+
expect_payment_sent!($node, $expected_payment_preimage, $expected_fee_msat_opt, true);
1098+
};
1099+
($node: expr, $expected_payment_preimage: expr, $expected_fee_msat_opt: expr, $expect_paths: expr) => {
10871100
let events = $node.node.get_and_clear_pending_events();
10881101
let expected_payment_hash = PaymentHash(Sha256::hash(&$expected_payment_preimage.0).into_inner());
1089-
assert_eq!(events.len(), 1);
1102+
if $expect_paths {
1103+
assert!(events.len() > 1);
1104+
} else {
1105+
assert_eq!(events.len(), 1);
1106+
}
10901107
match events[0] {
10911108
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash, ref fee_paid_msat } => {
10921109
assert_eq!($expected_payment_preimage, *payment_preimage);
@@ -1098,6 +1115,26 @@ macro_rules! expect_payment_sent {
10981115
},
10991116
_ => panic!("Unexpected event"),
11001117
}
1118+
if $expect_paths {
1119+
for i in 1..events.len() {
1120+
match events[i] {
1121+
Event::PaymentPathSuccessful { .. } => {},
1122+
_ => panic!("Unexpected event"),
1123+
}
1124+
}
1125+
}
1126+
}
1127+
}
1128+
1129+
#[cfg(test)]
1130+
macro_rules! expect_payment_path_successful {
1131+
($node: expr) => {
1132+
let events = $node.node.get_and_clear_pending_events();
1133+
assert_eq!(events.len(), 1);
1134+
match events[0] {
1135+
Event::PaymentPathSuccessful { .. } => {},
1136+
_ => panic!("Unexpected event"),
1137+
}
11011138
}
11021139
}
11031140

0 commit comments

Comments
 (0)