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.

Author: TheBlueMatt

lightning-invoice/src/payment.rs

@@ -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);
@@ -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);
@@ -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);
@@ -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);

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -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);
 					},
@@ -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);
 				},
@@ -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);
 		},
@@ -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);
 		},

lightning/src/ln/channelmanager.rs

@@ -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;
@@ -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+.
+		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.
@@ -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();
@@ -484,10 +492,13 @@ impl PendingOutboundPayment {
 			}
 		};
 		if remove_res {
-			if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, .. } = self {
+			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
@@ -502,9 +513,12 @@ impl PendingOutboundPayment {
 			PendingOutboundPayment::Fulfilled { .. } => false
 		};
 		if insert_res {
-			if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, .. } = self {
+			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
@@ -2084,6 +2098,7 @@ 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(),
@@ -3458,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.
@@ -3473,17 +3489,17 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 							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));
 				}
@@ -5496,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),
@@ -5957,11 +5974,13 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 										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,
 									});
@@ -6260,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);

lightning/src/ln/functional_test_utils.rs

@@ -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"),
 		}
@@ -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 {
@@ -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();
@@ -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));
 	}
 }
 

lightning/src/ln/functional_tests.rs

@@ -2659,7 +2659,7 @@ fn test_htlc_on_chain_success() {
 	let mut first_claimed = false;
 	for event in events {
 		match event {
-			Event::PaymentSent { payment_id: _, payment_preimage, payment_hash } => {
+			Event::PaymentSent { payment_preimage, payment_hash, .. } => {
 				if payment_preimage == our_payment_preimage && payment_hash == payment_hash_1 {
 					assert!(!first_claimed);
 					first_claimed = true;
@@ -3350,7 +3350,7 @@ fn test_simple_peer_disconnect() {
 		let events = nodes[0].node.get_and_clear_pending_events();
 		assert_eq!(events.len(), 2);
 		match events[0] {
-			Event::PaymentSent { payment_id: _, payment_preimage, payment_hash } => {
+			Event::PaymentSent { payment_preimage, payment_hash, .. } => {
 				assert_eq!(payment_preimage, payment_preimage_3);
 				assert_eq!(payment_hash, payment_hash_3);
 			},
@@ -3514,7 +3514,7 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
 		let events_4 = nodes[0].node.get_and_clear_pending_events();
 		assert_eq!(events_4.len(), 1);
 		match events_4[0] {
-			Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
+			Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
 				assert_eq!(payment_preimage_1, *payment_preimage);
 				assert_eq!(payment_hash_1, *payment_hash);
 			},
@@ -3555,7 +3555,7 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
 			let events_4 = nodes[0].node.get_and_clear_pending_events();
 			assert_eq!(events_4.len(), 1);
 			match events_4[0] {
-				Event::PaymentSent { payment_id: _, ref payment_preimage, ref payment_hash } => {
+				Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
 					assert_eq!(payment_preimage_1, *payment_preimage);
 					assert_eq!(payment_hash_1, *payment_hash);
 				},
@@ -3790,7 +3790,7 @@ fn test_drop_messages_peer_disconnect_dual_htlc() {
 			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);
 				},
@@ -5059,7 +5059,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	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, our_payment_preimage);
 			assert_eq!(*payment_hash, duplicate_payment_hash);
 		}
@@ -5572,7 +5572,7 @@ fn do_htlc_claim_local_commitment_only(use_dust: bool) {
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
 	match events[0] {
-		Event::PaymentSent { payment_id: _, payment_preimage, payment_hash } => {
+		Event::PaymentSent { payment_preimage, payment_hash, .. } => {
 			assert_eq!(payment_preimage, our_payment_preimage);
 			assert_eq!(payment_hash, our_payment_hash);
 		},
@@ -6002,7 +6002,7 @@ fn test_free_and_fail_holding_cell_htlcs() {
 	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);
 		}

lightning/src/ln/payment_tests.rs

@@ -296,7 +296,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
 	let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 
 	let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
-	create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
+	let (_, _, chan_id_2, _) = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
 
 	// Serialize the ChannelManager prior to sending payments
 	let nodes_0_serialized = nodes[0].node.encode();
@@ -445,15 +445,22 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
 	// Finally, retry the payment (which was reloaded from the ChannelMonitor when nodes[0] was
 	// reloaded) via a route over the new channel, which work without issue and eventually be
 	// received and claimed at the recipient just like any other payment.
-	let (new_route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
+	let (mut new_route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000);
+
+	// Update the fee on the middle hop to ensure PaymentSent events have the correct (retried) fee
+	// and not the original fee. We also update node[1]'s relevant config as
+	// do_claim_payment_along_route expects us to never overpay.
+	nodes[1].node.channel_state.lock().unwrap().by_id.get_mut(&chan_id_2).unwrap().config.forwarding_fee_base_msat += 100_000;
+	new_route.paths[0][0].fee_msat += 100_000;
 
 	assert!(nodes[0].node.retry_payment(&new_route, payment_id_1).is_err()); // Shouldn't be allowed to retry a fulfilled payment
 	nodes[0].node.retry_payment(&new_route, payment_id).unwrap();
 	check_added_monitors!(nodes[0], 1);
 	let mut events = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), 1);
 	pass_along_path(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000, payment_hash, Some(payment_secret), events.pop().unwrap(), true, None);
-	claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], false, payment_preimage);
+	do_claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], false, payment_preimage);
+	expect_payment_sent!(nodes[0], payment_preimage, Some(new_route.paths[0][0].fee_msat));
 }
 
 #[test]

lightning/src/ln/shutdown_tests.rs

@@ -131,7 +131,7 @@ fn updates_shutdown_wait() {
 	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!(our_payment_preimage, *payment_preimage);
 			assert_eq!(our_payment_hash, *payment_hash);
 		},
@@ -309,7 +309,7 @@ fn do_test_shutdown_rebroadcast(recv_count: u8) {
 	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!(our_payment_preimage, *payment_preimage);
 			assert_eq!(our_payment_hash, *payment_hash);
 		},