Skip to content

Commit cd01677

Browse files
TheBlueMattTheBlueMatt
committed
Track the amount spent on fees as payments are retried
Especially once we merge the `InvoicePayer` logic soon, we'll want to expose the total fee paid in the `PaymentSent` event.
1 parent f00628e commit cd01677

File tree

8 files changed

+90
-40
lines changed

8 files changed

+90
-40
lines changed

‎lightning-invoice/src/payment.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -430,7 +430,7 @@ mod tests {
430430
assert_eq!(*payer.attempts.borrow(), 1);
431431

432432
invoice_payer.handle_event(&Event::PaymentSent {
433-
payment_id, payment_preimage, payment_hash
433+
payment_id, payment_preimage, payment_hash, fee_paid_msat: None
434434
});
435435
assert_eq!(*event_handled.borrow(), true);
436436
assert_eq!(*payer.attempts.borrow(), 1);
@@ -472,7 +472,7 @@ mod tests {
472472
assert_eq!(*payer.attempts.borrow(), 2);
473473

474474
invoice_payer.handle_event(&Event::PaymentSent {
475-
payment_id, payment_preimage, payment_hash
475+
payment_id, payment_preimage, payment_hash, fee_paid_msat: None
476476
});
477477
assert_eq!(*event_handled.borrow(), true);
478478
assert_eq!(*payer.attempts.borrow(), 2);
@@ -514,7 +514,7 @@ mod tests {
514514
assert_eq!(*payer.attempts.borrow(), 2);
515515

516516
invoice_payer.handle_event(&Event::PaymentSent {
517-
payment_id, payment_preimage, payment_hash
517+
payment_id, payment_preimage, payment_hash, fee_paid_msat: None
518518
});
519519
assert_eq!(*event_handled.borrow(), true);
520520
assert_eq!(*payer.attempts.borrow(), 2);
@@ -802,7 +802,7 @@ mod tests {
802802
assert_eq!(*payer.attempts.borrow(), 1);
803803

804804
invoice_payer.handle_event(&Event::PaymentSent {
805-
payment_id, payment_preimage, payment_hash
805+
payment_id, payment_preimage, payment_hash, fee_paid_msat: None
806806
});
807807
assert_eq!(*event_handled.borrow(), true);
808808
assert_eq!(*payer.attempts.borrow(), 1);

‎lightning/src/ln/chanmon_update_fail_tests.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
304304
let events_3 = nodes[0].node.get_and_clear_pending_events();
305305
assert_eq!(events_3.len(), 1);
306306
match events_3[0] {
307-
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
307+
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
308308
assert_eq!(*payment_preimage, payment_preimage_1);
309309
assert_eq!(*payment_hash, payment_hash_1);
310310
},
@@ -397,7 +397,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
397397
let events_3 = nodes[0].node.get_and_clear_pending_events();
398398
assert_eq!(events_3.len(), 1);
399399
match events_3[0] {
400-
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
400+
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
401401
assert_eq!(*payment_preimage, payment_preimage_1);
402402
assert_eq!(*payment_hash, payment_hash_1);
403403
},
@@ -1399,7 +1399,7 @@ fn claim_while_disconnected_monitor_update_fail() {
13991399
let events = nodes[0].node.get_and_clear_pending_events();
14001400
assert_eq!(events.len(), 1);
14011401
match events[0] {
1402-
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
1402+
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
14031403
assert_eq!(*payment_preimage, payment_preimage_1);
14041404
assert_eq!(*payment_hash, payment_hash_1);
14051405
},
@@ -1806,7 +1806,7 @@ fn monitor_update_claim_fail_no_response() {
18061806
let events = nodes[0].node.get_and_clear_pending_events();
18071807
assert_eq!(events.len(), 1);
18081808
match events[0] {
1809-
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
1809+
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
18101810
assert_eq!(*payment_preimage, payment_preimage_1);
18111811
assert_eq!(*payment_hash, payment_hash_1);
18121812
},

‎lightning/src/ln/channelmanager.rs‎

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ use chain::transaction::{OutPoint, TransactionData};
4545
use ln::{PaymentHash, PaymentPreimage, PaymentSecret};
4646
use ln::channel::{Channel, ChannelError, ChannelUpdateStatus, UpdateFulfillCommitFetch};
4747
use ln::features::{InitFeatures, NodeFeatures};
48-
use routing::router::{Payee, Route, RouteHop, RouteParameters};
48+
use routing::router::{Payee, Route, RouteHop, RoutePath, RouteParameters};
4949
use ln::msgs;
5050
use ln::msgs::NetAddress;
5151
use ln::onion_utils;
@@ -436,6 +436,8 @@ pub(crate) enum PendingOutboundPayment {
436436
payment_hash: PaymentHash,
437437
payment_secret: Option<PaymentSecret>,
438438
pending_amt_msat: u64,
439+
/// Used to track the fee paid. Only present if the payment was serialized on 0.0.103+.
440+
pending_fee_msat: Option<u64>,
439441
/// The total payment amount across all paths, used to verify that a retry is not overpaying.
440442
total_msat: u64,
441443
/// Our best known block height at the time this payment was initiated.
@@ -462,6 +464,12 @@ impl PendingOutboundPayment {
462464
_ => false,
463465
}
464466
}
467+
fn get_pending_fee_msat(&self) -> Option<u64> {
468+
match self {
469+
PendingOutboundPayment::Retryable { pending_fee_msat, .. } => pending_fee_msat.clone(),
470+
_ => None,
471+
}
472+
}
465473

466474
fn mark_fulfilled(&mut self) {
467475
let mut session_privs = HashSet::new();
@@ -484,10 +492,13 @@ impl PendingOutboundPayment {
484492
}
485493
};
486494
if remove_res {
487-
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, .. } = self {
495+
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {
488496
let path = path.expect("Fulfilling a payment should always come with a path");
489497
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
490498
*pending_amt_msat -= path_last_hop.fee_msat;
499+
if let Some(fee_msat) = pending_fee_msat.as_mut() {
500+
*fee_msat -= path.get_path_fees();
501+
}
491502
}
492503
}
493504
remove_res
@@ -502,9 +513,12 @@ impl PendingOutboundPayment {
502513
PendingOutboundPayment::Fulfilled { .. } => false
503514
};
504515
if insert_res {
505-
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, .. } = self {
516+
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {
506517
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
507518
*pending_amt_msat += path_last_hop.fee_msat;
519+
if let Some(fee_msat) = pending_fee_msat.as_mut() {
520+
*fee_msat += path.get_path_fees();
521+
}
508522
}
509523
}
510524
insert_res
@@ -2084,6 +2098,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
20842098
let payment = payment_entry.or_insert_with(|| PendingOutboundPayment::Retryable {
20852099
session_privs: HashSet::new(),
20862100
pending_amt_msat: 0,
2101+
pending_fee_msat: Some(0),
20872102
payment_hash: *payment_hash,
20882103
payment_secret: *payment_secret,
20892104
starting_block_height: self.best_block.read().unwrap().height(),
@@ -3458,8 +3473,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
34583473
let mut session_priv_bytes = [0; 32];
34593474
session_priv_bytes.copy_from_slice(&session_priv[..]);
34603475
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
3461-
let found_payment = if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
3476+
if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
34623477
let found_payment = !payment.get().is_fulfilled();
3478+
let fee_paid_msat = payment.get().get_pending_fee_msat();
34633479
payment.get_mut().mark_fulfilled();
34643480
if from_onchain {
34653481
// We currently immediately remove HTLCs which were fulfilled on-chain.
@@ -3473,17 +3489,17 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
34733489
payment.remove();
34743490
}
34753491
}
3476-
found_payment
3477-
} else { false };
3478-
if found_payment {
3479-
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
3480-
self.pending_events.lock().unwrap().push(
3481-
events::Event::PaymentSent {
3482-
payment_id: Some(payment_id),
3483-
payment_preimage,
3484-
payment_hash: payment_hash
3485-
}
3486-
);
3492+
if found_payment {
3493+
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
3494+
self.pending_events.lock().unwrap().push(
3495+
events::Event::PaymentSent {
3496+
payment_id: Some(payment_id),
3497+
payment_preimage,
3498+
payment_hash: payment_hash,
3499+
fee_paid_msat,
3500+
}
3501+
);
3502+
}
34873503
} else {
34883504
log_trace!(self.logger, "Received duplicative fulfill for HTLC with payment_preimage {}", log_bytes!(payment_preimage.0));
34893505
}
@@ -5496,6 +5512,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
54965512
},
54975513
(2, Retryable) => {
54985514
(0, session_privs, required),
5515+
(1, pending_fee_msat, option),
54995516
(2, payment_hash, required),
55005517
(4, payment_secret, option),
55015518
(6, total_msat, required),
@@ -5957,11 +5974,13 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
59575974
if newly_added { "Added" } else { "Had" }, path_amt, log_bytes!(session_priv_bytes), log_bytes!(htlc.payment_hash.0));
59585975
},
59595976
hash_map::Entry::Vacant(entry) => {
5977+
let path_fee = path.get_path_fees();
59605978
entry.insert(PendingOutboundPayment::Retryable {
59615979
session_privs: [session_priv_bytes].iter().map(|a| *a).collect(),
59625980
payment_hash: htlc.payment_hash,
59635981
payment_secret,
59645982
pending_amt_msat: path_amt,
5983+
pending_fee_msat: Some(path_fee),
59655984
total_msat: path_amt,
59665985
starting_block_height: best_block_height,
59675986
});
@@ -6260,7 +6279,7 @@ mod tests {
62606279
// further events will be generated for subsequence path successes.
62616280
let events = nodes[0].node.get_and_clear_pending_events();
62626281
match events[0] {
6263-
Event::PaymentSent { payment_id: ref id, payment_preimage: ref preimage, payment_hash: ref hash } => {
6282+
Event::PaymentSent { payment_id: ref id, payment_preimage: ref preimage, payment_hash: ref hash, .. } => {
62646283
assert_eq!(Some(payment_id), *id);
62656284
assert_eq!(payment_preimage, *preimage);
62666285
assert_eq!(our_payment_hash, *hash);

‎lightning/src/ln/functional_test_utils.rs‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1081,13 +1081,20 @@ macro_rules! expect_payment_received {
10811081

10821082
macro_rules! expect_payment_sent {
10831083
($node: expr, $expected_payment_preimage: expr) => {
1084+
expect_payment_sent!($node, $expected_payment_preimage, None::<u64>);
1085+
};
1086+
($node: expr, $expected_payment_preimage: expr, $expected_fee_msat_opt: expr) => {
10841087
let events = $node.node.get_and_clear_pending_events();
10851088
let expected_payment_hash = PaymentHash(Sha256::hash(&$expected_payment_preimage.0).into_inner());
10861089
assert_eq!(events.len(), 1);
10871090
match events[0] {
1088-
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
1091+
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash, ref fee_paid_msat } => {
10891092
assert_eq!($expected_payment_preimage, *payment_preimage);
10901093
assert_eq!(expected_payment_hash, *payment_hash);
1094+
assert!(fee_paid_msat.is_some());
1095+
if $expected_fee_msat_opt.is_some() {
1096+
assert_eq!(*fee_paid_msat, $expected_fee_msat_opt);
1097+
}
10911098
},
10921099
_ => panic!("Unexpected event"),
10931100
}
@@ -1237,7 +1244,7 @@ pub fn send_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route
12371244
(our_payment_preimage, our_payment_hash, our_payment_secret, payment_id)
12381245
}
12391246

1240-
pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_preimage: PaymentPreimage) {
1247+
pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_preimage: PaymentPreimage) {
12411248
for path in expected_paths.iter() {
12421249
assert_eq!(path.last().unwrap().node.get_our_node_id(), expected_paths[0].last().unwrap().node.get_our_node_id());
12431250
}
@@ -1324,6 +1331,9 @@ pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, exp
13241331
last_update_fulfill_dance!(origin_node, expected_route.first().unwrap());
13251332
}
13261333
}
1334+
}
1335+
pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_preimage: PaymentPreimage) {
1336+
do_claim_payment_along_route(origin_node, expected_paths, skip_last, our_payment_preimage);
13271337
if !skip_last {
13281338
expect_payment_sent!(origin_node, our_payment_preimage);
13291339
}

‎lightning/src/ln/functional_tests.rs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2659,7 +2659,7 @@ fn test_htlc_on_chain_success() {
26592659
let mut first_claimed = false;
26602660
for event in events {
26612661
match event {
2662-
Event::PaymentSent { payment_id: _, payment_preimage, payment_hash } => {
2662+
Event::PaymentSent { payment_preimage, payment_hash, .. } => {
26632663
if payment_preimage == our_payment_preimage && payment_hash == payment_hash_1 {
26642664
assert!(!first_claimed);
26652665
first_claimed = true;
@@ -3350,7 +3350,7 @@ fn test_simple_peer_disconnect() {
33503350
let events = nodes[0].node.get_and_clear_pending_events();
33513351
assert_eq!(events.len(), 2);
33523352
match events[0] {
3353-
Event::PaymentSent { payment_id: _, payment_preimage, payment_hash } => {
3353+
Event::PaymentSent { payment_preimage, payment_hash, .. } => {
33543354
assert_eq!(payment_preimage, payment_preimage_3);
33553355
assert_eq!(payment_hash, payment_hash_3);
33563356
},
@@ -3514,7 +3514,7 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
35143514
let events_4 = nodes[0].node.get_and_clear_pending_events();
35153515
assert_eq!(events_4.len(), 1);
35163516
match events_4[0] {
3517-
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
3517+
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
35183518
assert_eq!(payment_preimage_1, *payment_preimage);
35193519
assert_eq!(payment_hash_1, *payment_hash);
35203520
},
@@ -3555,7 +3555,7 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
35553555
let events_4 = nodes[0].node.get_and_clear_pending_events();
35563556
assert_eq!(events_4.len(), 1);
35573557
match events_4[0] {
3558-
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
3558+
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
35593559
assert_eq!(payment_preimage_1, *payment_preimage);
35603560
assert_eq!(payment_hash_1, *payment_hash);
35613561
},
@@ -3790,7 +3790,7 @@ fn test_drop_messages_peer_disconnect_dual_htlc() {
37903790
let events_3 = nodes[0].node.get_and_clear_pending_events();
37913791
assert_eq!(events_3.len(), 1);
37923792
match events_3[0] {
3793-
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
3793+
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
37943794
assert_eq!(*payment_preimage, payment_preimage_1);
37953795
assert_eq!(*payment_hash, payment_hash_1);
37963796
},
@@ -5059,7 +5059,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
50595059

50605060
let events = nodes[0].node.get_and_clear_pending_events();
50615061
match events[0] {
5062-
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
5062+
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
50635063
assert_eq!(*payment_preimage, our_payment_preimage);
50645064
assert_eq!(*payment_hash, duplicate_payment_hash);
50655065
}
@@ -5572,7 +5572,7 @@ fn do_htlc_claim_local_commitment_only(use_dust: bool) {
55725572
let events = nodes[0].node.get_and_clear_pending_events();
55735573
assert_eq!(events.len(), 1);
55745574
match events[0] {
5575-
Event::PaymentSent { payment_id: _, payment_preimage, payment_hash } => {
5575+
Event::PaymentSent { payment_preimage, payment_hash, .. } => {
55765576
assert_eq!(payment_preimage, our_payment_preimage);
55775577
assert_eq!(payment_hash, our_payment_hash);
55785578
},
@@ -6002,7 +6002,7 @@ fn test_free_and_fail_holding_cell_htlcs() {
60026002
let events = nodes[0].node.get_and_clear_pending_events();
60036003
assert_eq!(events.len(), 1);
60046004
match events[0] {
6005-
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
6005+
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
60066006
assert_eq!(*payment_preimage, payment_preimage_1);
60076007
assert_eq!(*payment_hash, payment_hash_1);
60086008
}

‎lightning/src/ln/payment_tests.rs‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
296296
let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
297297

298298
let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
299-
create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
299+
let (_, _, chan_id_2, _) = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
300300

301301
// Serialize the ChannelManager prior to sending payments
302302
let nodes_0_serialized = nodes[0].node.encode();
@@ -445,15 +445,22 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
445445
// Finally, retry the payment (which was reloaded from the ChannelMonitor when nodes[0] was
446446
// reloaded) via a route over the new channel, which work without issue and eventually be
447447
// received and claimed at the recipient just like any other payment.
448-
let (new_route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
448+
let (mut new_route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
449+
450+
// Update the fee on the middle hop to ensure PaymentSent events have the correct (retried) fee
451+
// and not the original fee. We also update node[1]'s relevant config as
452+
// do_claim_payment_along_route expects us to never overpay.
453+
nodes[1].node.channel_state.lock().unwrap().by_id.get_mut(&chan_id_2).unwrap().config.forwarding_fee_base_msat += 100_000;
454+
new_route.paths[0][0].fee_msat += 100_000;
449455

450456
assert!(nodes[0].node.retry_payment(&new_route, payment_id_1).is_err()); // Shouldn't be allowed to retry a fulfilled payment
451457
nodes[0].node.retry_payment(&new_route, payment_id).unwrap();
452458
check_added_monitors!(nodes[0], 1);
453459
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
454460
assert_eq!(events.len(), 1);
455461
pass_along_path(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000, payment_hash, Some(payment_secret), events.pop().unwrap(), true, None);
456-
claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], false, payment_preimage);
462+
do_claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], false, payment_preimage);
463+
expect_payment_sent!(nodes[0], payment_preimage, Some(new_route.paths[0][0].fee_msat));
457464
}
458465

459466
#[test]

‎lightning/src/ln/shutdown_tests.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ fn updates_shutdown_wait() {
131131
let events = nodes[0].node.get_and_clear_pending_events();
132132
assert_eq!(events.len(), 1);
133133
match events[0] {
134-
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
134+
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
135135
assert_eq!(our_payment_preimage, *payment_preimage);
136136
assert_eq!(our_payment_hash, *payment_hash);
137137
},
@@ -309,7 +309,7 @@ fn do_test_shutdown_rebroadcast(recv_count: u8) {
309309
let events = nodes[0].node.get_and_clear_pending_events();
310310
assert_eq!(events.len(), 1);
311311
match events[0] {
312-
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
312+
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
313313
assert_eq!(our_payment_preimage, *payment_preimage);
314314
assert_eq!(our_payment_hash, *payment_hash);
315315
},

0 commit comments

Comments
 (0)