Skip to content

Track the amount spent on fees as payments are retried #1142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 27, 2021
Merged

Track the amount spent on fees as payments are retried #1142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
517a154
Add a utility trait in `router` to get the fees along a given path
TheBlueMatt Oct 27, 2021
af94db8
Pass the failing/succeeding `Path` to PendingOutboundPayment meths
TheBlueMatt Oct 27, 2021
04d4a8f
Track the amount spent on fees as payments are retried
TheBlueMatt Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions lightning-invoice/src/payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ mod tests {
assert_eq!(*payer.attempts.borrow(), 1);

invoice_payer.handle_event(&Event::PaymentSent {
payment_id, payment_preimage, payment_hash
payment_id, payment_preimage, payment_hash, fee_paid_msat: None
});
assert_eq!(*event_handled.borrow(), true);
assert_eq!(*payer.attempts.borrow(), 1);
Expand Down Expand Up @@ -472,7 +472,7 @@ mod tests {
assert_eq!(*payer.attempts.borrow(), 2);

invoice_payer.handle_event(&Event::PaymentSent {
payment_id, payment_preimage, payment_hash
payment_id, payment_preimage, payment_hash, fee_paid_msat: None
});
assert_eq!(*event_handled.borrow(), true);
assert_eq!(*payer.attempts.borrow(), 2);
Expand Down Expand Up @@ -514,7 +514,7 @@ mod tests {
assert_eq!(*payer.attempts.borrow(), 2);

invoice_payer.handle_event(&Event::PaymentSent {
payment_id, payment_preimage, payment_hash
payment_id, payment_preimage, payment_hash, fee_paid_msat: None
});
assert_eq!(*event_handled.borrow(), true);
assert_eq!(*payer.attempts.borrow(), 2);
Expand Down Expand Up @@ -802,7 +802,7 @@ mod tests {
assert_eq!(*payer.attempts.borrow(), 1);

invoice_payer.handle_event(&Event::PaymentSent {
payment_id, payment_preimage, payment_hash
payment_id, payment_preimage, payment_hash, fee_paid_msat: None
});
assert_eq!(*event_handled.borrow(), true);
assert_eq!(*payer.attempts.borrow(), 1);
Expand Down
8 changes: 4 additions & 4 deletions lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
let events_3 = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events_3.len(), 1);
match events_3[0] {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
assert_eq!(*payment_preimage, payment_preimage_1);
assert_eq!(*payment_hash, payment_hash_1);
},
Expand Down Expand Up @@ -397,7 +397,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
let events_3 = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events_3.len(), 1);
match events_3[0] {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
assert_eq!(*payment_preimage, payment_preimage_1);
assert_eq!(*payment_hash, payment_hash_1);
},
Expand Down Expand Up @@ -1399,7 +1399,7 @@ fn claim_while_disconnected_monitor_update_fail() {
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
assert_eq!(*payment_preimage, payment_preimage_1);
assert_eq!(*payment_hash, payment_hash_1);
},
Expand Down Expand Up @@ -1806,7 +1806,7 @@ fn monitor_update_claim_fail_no_response() {
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
assert_eq!(*payment_preimage, payment_preimage_1);
assert_eq!(*payment_hash, payment_hash_1);
},
Expand Down
80 changes: 50 additions & 30 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use chain::transaction::{OutPoint, TransactionData};
use ln::{PaymentHash, PaymentPreimage, PaymentSecret};
use ln::channel::{Channel, ChannelError, ChannelUpdateStatus, UpdateFulfillCommitFetch};
use ln::features::{InitFeatures, NodeFeatures};
use routing::router::{Payee, Route, RouteHop, RouteParameters};
use routing::router::{Payee, Route, RouteHop, RoutePath, RouteParameters};
use ln::msgs;
use ln::msgs::NetAddress;
use ln::onion_utils;
Expand Down Expand Up @@ -436,6 +436,8 @@ pub(crate) enum PendingOutboundPayment {
payment_hash: PaymentHash,
payment_secret: Option<PaymentSecret>,
pending_amt_msat: u64,
/// Used to track the fee paid. Only present if the payment was serialized on 0.0.103+.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggested wording: "Used to track the fees from paths that have not yet succeeded or failed."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm? No, we use it to track the full fees, we just do that by only tracking the pending fees and then deciding that's the fees actually paid when we succeed, since it will be unless someone gave us free money.

pending_fee_msat: Option<u64>,
/// The total payment amount across all paths, used to verify that a retry is not overpaying.
total_msat: u64,
/// Our best known block height at the time this payment was initiated.
Expand All @@ -462,6 +464,12 @@ impl PendingOutboundPayment {
_ => false,
}
}
fn get_pending_fee_msat(&self) -> Option<u64> {
match self {
PendingOutboundPayment::Retryable { pending_fee_msat, .. } => pending_fee_msat.clone(),
_ => None,
}
}

fn mark_fulfilled(&mut self) {
let mut session_privs = HashSet::new();
Expand All @@ -474,8 +482,8 @@ impl PendingOutboundPayment {
*self = PendingOutboundPayment::Fulfilled { session_privs };
}

/// panics if part_amt_msat is None and !self.is_fulfilled
fn remove(&mut self, session_priv: &[u8; 32], part_amt_msat: Option<u64>) -> bool {
/// panics if path is None and !self.is_fulfilled
fn remove(&mut self, session_priv: &[u8; 32], path: Option<&Vec<RouteHop>>) -> bool {
let remove_res = match self {
PendingOutboundPayment::Legacy { session_privs } |
PendingOutboundPayment::Retryable { session_privs, .. } |
Expand All @@ -484,14 +492,19 @@ impl PendingOutboundPayment {
}
};
if remove_res {
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, .. } = self {
*pending_amt_msat -= part_amt_msat.expect("We must only not provide an amount if the payment was already fulfilled");
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {
let path = path.expect("Fulfilling a payment should always come with a path");
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
*pending_amt_msat -= path_last_hop.fee_msat;
if let Some(fee_msat) = pending_fee_msat.as_mut() {
*fee_msat -= path.get_path_fees();
}
}
}
remove_res
}

fn insert(&mut self, session_priv: [u8; 32], part_amt_msat: u64) -> bool {
fn insert(&mut self, session_priv: [u8; 32], path: &Vec<RouteHop>) -> bool {
let insert_res = match self {
PendingOutboundPayment::Legacy { session_privs } |
PendingOutboundPayment::Retryable { session_privs, .. } => {
Expand All @@ -500,8 +513,12 @@ impl PendingOutboundPayment {
PendingOutboundPayment::Fulfilled { .. } => false
};
if insert_res {
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, .. } = self {
*pending_amt_msat += part_amt_msat;
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
*pending_amt_msat += path_last_hop.fee_msat;
if let Some(fee_msat) = pending_fee_msat.as_mut() {
*fee_msat += path.get_path_fees();
}
}
}
insert_res
Expand Down Expand Up @@ -2081,12 +2098,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
let payment = payment_entry.or_insert_with(|| PendingOutboundPayment::Retryable {
session_privs: HashSet::new(),
pending_amt_msat: 0,
pending_fee_msat: Some(0),
payment_hash: *payment_hash,
payment_secret: *payment_secret,
starting_block_height: self.best_block.read().unwrap().height(),
total_msat: total_value,
});
assert!(payment.insert(session_priv_bytes, path.last().unwrap().fee_msat));
assert!(payment.insert(session_priv_bytes, path));

send_res
} {
Expand Down Expand Up @@ -3107,11 +3125,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
session_priv_bytes.copy_from_slice(&session_priv[..]);
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
if payment.get_mut().remove(&session_priv_bytes, Some(path_last_hop.fee_msat)) &&
!payment.get().is_fulfilled()
{
if payment.get_mut().remove(&session_priv_bytes, Some(&path)) && !payment.get().is_fulfilled() {
let retry = if let Some(payee_data) = payee {
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
Some(RouteParameters {
payee: payee_data,
final_value_msat: path_last_hop.fee_msat,
Expand Down Expand Up @@ -3164,9 +3180,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
session_priv_bytes.copy_from_slice(&session_priv[..]);
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
let mut all_paths_failed = false;
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
if !payment.get_mut().remove(&session_priv_bytes, Some(path_last_hop.fee_msat)) {
if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
return;
}
Expand All @@ -3183,6 +3198,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
}
mem::drop(channel_state_lock);
let retry = if let Some(payee_data) = payee {
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
Some(RouteParameters {
payee: payee_data.clone(),
final_value_msat: path_last_hop.fee_msat,
Expand Down Expand Up @@ -3457,8 +3473,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
let mut session_priv_bytes = [0; 32];
session_priv_bytes.copy_from_slice(&session_priv[..]);
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
let found_payment = if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
let found_payment = !payment.get().is_fulfilled();
let fee_paid_msat = payment.get().get_pending_fee_msat();
payment.get_mut().mark_fulfilled();
if from_onchain {
// We currently immediately remove HTLCs which were fulfilled on-chain.
Expand All @@ -3467,22 +3484,22 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
// restart.
// TODO: We should have a second monitor event that informs us of payments
// irrevocably fulfilled.
payment.get_mut().remove(&session_priv_bytes, Some(path.last().unwrap().fee_msat));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the last two params here should be None for consistency.

I think it'd be a little more readable if we dropped the lock then called finalize_claims rather than duplicating the finalize_claims logic

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These actually should diverge more, see the TODO here - we'd really like to mark the payment fulfilled but not remove the HTLC until after the commitment signed dance completes.

payment.get_mut().remove(&session_priv_bytes, Some(&path));
if payment.get().remaining_parts() == 0 {
payment.remove();
}
}
found_payment
} else { false };
if found_payment {
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
self.pending_events.lock().unwrap().push(
events::Event::PaymentSent {
payment_id: Some(payment_id),
payment_preimage,
payment_hash: payment_hash
}
);
if found_payment {
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
self.pending_events.lock().unwrap().push(
events::Event::PaymentSent {
payment_id: Some(payment_id),
payment_preimage,
payment_hash: payment_hash,
fee_paid_msat,
}
);
}
} else {
log_trace!(self.logger, "Received duplicative fulfill for HTLC with payment_preimage {}", log_bytes!(payment_preimage.0));
}
Expand Down Expand Up @@ -5495,6 +5512,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
},
(2, Retryable) => {
(0, session_privs, required),
(1, pending_fee_msat, option),
(2, payment_hash, required),
(4, payment_secret, option),
(6, total_msat, required),
Expand Down Expand Up @@ -5951,16 +5969,18 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
session_priv_bytes[..].copy_from_slice(&session_priv[..]);
match pending_outbound_payments.as_mut().unwrap().entry(payment_id) {
hash_map::Entry::Occupied(mut entry) => {
let newly_added = entry.get_mut().insert(session_priv_bytes, path_amt);
let newly_added = entry.get_mut().insert(session_priv_bytes, &path);
log_info!(args.logger, "{} a pending payment path for {} msat for session priv {} on an existing pending payment with payment hash {}",
if newly_added { "Added" } else { "Had" }, path_amt, log_bytes!(session_priv_bytes), log_bytes!(htlc.payment_hash.0));
},
hash_map::Entry::Vacant(entry) => {
let path_fee = path.get_path_fees();
entry.insert(PendingOutboundPayment::Retryable {
session_privs: [session_priv_bytes].iter().map(|a| *a).collect(),
payment_hash: htlc.payment_hash,
payment_secret,
pending_amt_msat: path_amt,
pending_fee_msat: Some(path_fee),
total_msat: path_amt,
starting_block_height: best_block_height,
});
Expand Down Expand Up @@ -6259,7 +6279,7 @@ mod tests {
// further events will be generated for subsequence path successes.
let events = nodes[0].node.get_and_clear_pending_events();
match events[0] {
Event::PaymentSent { payment_id: ref id, payment_preimage: ref preimage, payment_hash: ref hash } => {
Event::PaymentSent { payment_id: ref id, payment_preimage: ref preimage, payment_hash: ref hash, .. } => {
assert_eq!(Some(payment_id), *id);
assert_eq!(payment_preimage, *preimage);
assert_eq!(our_payment_hash, *hash);
Expand Down
20 changes: 17 additions & 3 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1081,13 +1081,20 @@ macro_rules! expect_payment_received {

macro_rules! expect_payment_sent {
($node: expr, $expected_payment_preimage: expr) => {
expect_payment_sent!($node, $expected_payment_preimage, None::<u64>);
};
($node: expr, $expected_payment_preimage: expr, $expected_fee_msat_opt: expr) => {
let events = $node.node.get_and_clear_pending_events();
let expected_payment_hash = PaymentHash(Sha256::hash(&$expected_payment_preimage.0).into_inner());
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash, ref fee_paid_msat } => {
assert_eq!($expected_payment_preimage, *payment_preimage);
assert_eq!(expected_payment_hash, *payment_hash);
assert!(fee_paid_msat.is_some());
if $expected_fee_msat_opt.is_some() {
assert_eq!(*fee_paid_msat, $expected_fee_msat_opt);
}
},
_ => panic!("Unexpected event"),
}
Expand Down Expand Up @@ -1237,13 +1244,15 @@ pub fn send_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route
(our_payment_preimage, our_payment_hash, our_payment_secret, payment_id)
}

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) {
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 {
for path in expected_paths.iter() {
assert_eq!(path.last().unwrap().node.get_our_node_id(), expected_paths[0].last().unwrap().node.get_our_node_id());
}
assert!(expected_paths[0].last().unwrap().node.claim_funds(our_payment_preimage));
check_added_monitors!(expected_paths[0].last().unwrap(), expected_paths.len());

let mut expected_total_fee_msat = 0;

macro_rules! msgs_from_ev {
($ev: expr) => {
match $ev {
Expand Down Expand Up @@ -1286,6 +1295,7 @@ pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, exp
$node.node.handle_update_fulfill_htlc(&$prev_node.node.get_our_node_id(), &next_msgs.as_ref().unwrap().0);
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;
expect_payment_forwarded!($node, Some(fee as u64), false);
expected_total_fee_msat += fee as u64;
check_added_monitors!($node, 1);
let new_next_msgs = if $new_msgs {
let events = $node.node.get_and_clear_pending_msg_events();
Expand Down Expand Up @@ -1324,8 +1334,12 @@ pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, exp
last_update_fulfill_dance!(origin_node, expected_route.first().unwrap());
}
}
expected_total_fee_msat
}
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) {
let expected_total_fee_msat = do_claim_payment_along_route(origin_node, expected_paths, skip_last, our_payment_preimage);
if !skip_last {
expect_payment_sent!(origin_node, our_payment_preimage);
expect_payment_sent!(origin_node, our_payment_preimage, Some(expected_total_fee_msat));
}
}

Expand Down
Loading