Skip to content

Commit 6f002ea

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #1142 from TheBlueMatt/2021-10-on-sent-fees
Track the amount spent on fees as payments are retried
2 parents 59659d3 + 04d4a8f commit 6f002ea

File tree

9 files changed

+124
-60
lines changed

9 files changed

+124
-60
lines changed

‎lightning-invoice/src/payment.rs

+4-4
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

+4-4
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

+50-30
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();
@@ -474,8 +482,8 @@ impl PendingOutboundPayment {
474482
*self = PendingOutboundPayment::Fulfilled { session_privs };
475483
}
476484

477-
/// panics if part_amt_msat is None and !self.is_fulfilled
478-
fn remove(&mut self, session_priv: &[u8; 32], part_amt_msat: Option<u64>) -> bool {
485+
/// panics if path is None and !self.is_fulfilled
486+
fn remove(&mut self, session_priv: &[u8; 32], path: Option<&Vec<RouteHop>>) -> bool {
479487
let remove_res = match self {
480488
PendingOutboundPayment::Legacy { session_privs } |
481489
PendingOutboundPayment::Retryable { session_privs, .. } |
@@ -484,14 +492,19 @@ impl PendingOutboundPayment {
484492
}
485493
};
486494
if remove_res {
487-
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, .. } = self {
488-
*pending_amt_msat -= part_amt_msat.expect("We must only not provide an amount if the payment was already fulfilled");
495+
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {
496+
let path = path.expect("Fulfilling a payment should always come with a path");
497+
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
498+
*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+
}
489502
}
490503
}
491504
remove_res
492505
}
493506

494-
fn insert(&mut self, session_priv: [u8; 32], part_amt_msat: u64) -> bool {
507+
fn insert(&mut self, session_priv: [u8; 32], path: &Vec<RouteHop>) -> bool {
495508
let insert_res = match self {
496509
PendingOutboundPayment::Legacy { session_privs } |
497510
PendingOutboundPayment::Retryable { session_privs, .. } => {
@@ -500,8 +513,12 @@ impl PendingOutboundPayment {
500513
PendingOutboundPayment::Fulfilled { .. } => false
501514
};
502515
if insert_res {
503-
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, .. } = self {
504-
*pending_amt_msat += part_amt_msat;
516+
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {
517+
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
518+
*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+
}
505522
}
506523
}
507524
insert_res
@@ -2081,12 +2098,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
20812098
let payment = payment_entry.or_insert_with(|| PendingOutboundPayment::Retryable {
20822099
session_privs: HashSet::new(),
20832100
pending_amt_msat: 0,
2101+
pending_fee_msat: Some(0),
20842102
payment_hash: *payment_hash,
20852103
payment_secret: *payment_secret,
20862104
starting_block_height: self.best_block.read().unwrap().height(),
20872105
total_msat: total_value,
20882106
});
2089-
assert!(payment.insert(session_priv_bytes, path.last().unwrap().fee_msat));
2107+
assert!(payment.insert(session_priv_bytes, path));
20902108

20912109
send_res
20922110
} {
@@ -3107,11 +3125,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31073125
session_priv_bytes.copy_from_slice(&session_priv[..]);
31083126
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
31093127
if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
3110-
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
3111-
if payment.get_mut().remove(&session_priv_bytes, Some(path_last_hop.fee_msat)) &&
3112-
!payment.get().is_fulfilled()
3113-
{
3128+
if payment.get_mut().remove(&session_priv_bytes, Some(&path)) && !payment.get().is_fulfilled() {
31143129
let retry = if let Some(payee_data) = payee {
3130+
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
31153131
Some(RouteParameters {
31163132
payee: payee_data,
31173133
final_value_msat: path_last_hop.fee_msat,
@@ -3164,9 +3180,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31643180
session_priv_bytes.copy_from_slice(&session_priv[..]);
31653181
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
31663182
let mut all_paths_failed = false;
3167-
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
31683183
if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
3169-
if !payment.get_mut().remove(&session_priv_bytes, Some(path_last_hop.fee_msat)) {
3184+
if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
31703185
log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
31713186
return;
31723187
}
@@ -3183,6 +3198,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
31833198
}
31843199
mem::drop(channel_state_lock);
31853200
let retry = if let Some(payee_data) = payee {
3201+
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
31863202
Some(RouteParameters {
31873203
payee: payee_data.clone(),
31883204
final_value_msat: path_last_hop.fee_msat,
@@ -3457,8 +3473,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
34573473
let mut session_priv_bytes = [0; 32];
34583474
session_priv_bytes.copy_from_slice(&session_priv[..]);
34593475
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
3460-
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) {
34613477
let found_payment = !payment.get().is_fulfilled();
3478+
let fee_paid_msat = payment.get().get_pending_fee_msat();
34623479
payment.get_mut().mark_fulfilled();
34633480
if from_onchain {
34643481
// We currently immediately remove HTLCs which were fulfilled on-chain.
@@ -3467,22 +3484,22 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
34673484
// restart.
34683485
// TODO: We should have a second monitor event that informs us of payments
34693486
// irrevocably fulfilled.
3470-
payment.get_mut().remove(&session_priv_bytes, Some(path.last().unwrap().fee_msat));
3487+
payment.get_mut().remove(&session_priv_bytes, Some(&path));
34713488
if payment.get().remaining_parts() == 0 {
34723489
payment.remove();
34733490
}
34743491
}
3475-
found_payment
3476-
} else { false };
3477-
if found_payment {
3478-
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
3479-
self.pending_events.lock().unwrap().push(
3480-
events::Event::PaymentSent {
3481-
payment_id: Some(payment_id),
3482-
payment_preimage,
3483-
payment_hash: payment_hash
3484-
}
3485-
);
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+
}
34863503
} else {
34873504
log_trace!(self.logger, "Received duplicative fulfill for HTLC with payment_preimage {}", log_bytes!(payment_preimage.0));
34883505
}
@@ -5495,6 +5512,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
54955512
},
54965513
(2, Retryable) => {
54975514
(0, session_privs, required),
5515+
(1, pending_fee_msat, option),
54985516
(2, payment_hash, required),
54995517
(4, payment_secret, option),
55005518
(6, total_msat, required),
@@ -5951,16 +5969,18 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
59515969
session_priv_bytes[..].copy_from_slice(&session_priv[..]);
59525970
match pending_outbound_payments.as_mut().unwrap().entry(payment_id) {
59535971
hash_map::Entry::Occupied(mut entry) => {
5954-
let newly_added = entry.get_mut().insert(session_priv_bytes, path_amt);
5972+
let newly_added = entry.get_mut().insert(session_priv_bytes, &path);
59555973
log_info!(args.logger, "{} a pending payment path for {} msat for session priv {} on an existing pending payment with payment hash {}",
59565974
if newly_added { "Added" } else { "Had" }, path_amt, log_bytes!(session_priv_bytes), log_bytes!(htlc.payment_hash.0));
59575975
},
59585976
hash_map::Entry::Vacant(entry) => {
5977+
let path_fee = path.get_path_fees();
59595978
entry.insert(PendingOutboundPayment::Retryable {
59605979
session_privs: [session_priv_bytes].iter().map(|a| *a).collect(),
59615980
payment_hash: htlc.payment_hash,
59625981
payment_secret,
59635982
pending_amt_msat: path_amt,
5983+
pending_fee_msat: Some(path_fee),
59645984
total_msat: path_amt,
59655985
starting_block_height: best_block_height,
59665986
});
@@ -6259,7 +6279,7 @@ mod tests {
62596279
// further events will be generated for subsequence path successes.
62606280
let events = nodes[0].node.get_and_clear_pending_events();
62616281
match events[0] {
6262-
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, .. } => {
62636283
assert_eq!(Some(payment_id), *id);
62646284
assert_eq!(payment_preimage, *preimage);
62656285
assert_eq!(our_payment_hash, *hash);

‎lightning/src/ln/functional_test_utils.rs

+17-3
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,13 +1244,15 @@ 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) -> u64 {
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
}
12441251
assert!(expected_paths[0].last().unwrap().node.claim_funds(our_payment_preimage));
12451252
check_added_monitors!(expected_paths[0].last().unwrap(), expected_paths.len());
12461253

1254+
let mut expected_total_fee_msat = 0;
1255+
12471256
macro_rules! msgs_from_ev {
12481257
($ev: expr) => {
12491258
match $ev {
@@ -1286,6 +1295,7 @@ pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, exp
12861295
$node.node.handle_update_fulfill_htlc(&$prev_node.node.get_our_node_id(), &next_msgs.as_ref().unwrap().0);
12871296
let fee = $node.node.channel_state.lock().unwrap().by_id.get(&next_msgs.as_ref().unwrap().0.channel_id).unwrap().config.forwarding_fee_base_msat;
12881297
expect_payment_forwarded!($node, Some(fee as u64), false);
1298+
expected_total_fee_msat += fee as u64;
12891299
check_added_monitors!($node, 1);
12901300
let new_next_msgs = if $new_msgs {
12911301
let events = $node.node.get_and_clear_pending_msg_events();
@@ -1324,8 +1334,12 @@ pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, exp
13241334
last_update_fulfill_dance!(origin_node, expected_route.first().unwrap());
13251335
}
13261336
}
1337+
expected_total_fee_msat
1338+
}
1339+
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) {
1340+
let expected_total_fee_msat = do_claim_payment_along_route(origin_node, expected_paths, skip_last, our_payment_preimage);
13271341
if !skip_last {
1328-
expect_payment_sent!(origin_node, our_payment_preimage);
1342+
expect_payment_sent!(origin_node, our_payment_preimage, Some(expected_total_fee_msat));
13291343
}
13301344
}
13311345

0 commit comments

Comments
 (0)