Merge pull request #1138 from TheBlueMatt/2021-10-payee-in-monitors

Store `Payee` info with HTLCs

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -305,6 +305,7 @@ fn send_payment(source: &ChanMan, dest: &ChanMan, dest_chan_id: u64, amt: u64, p
 			fee_msat: amt,
 			cltv_expiry_delta: 200,
 		}]],
+		payee: None,
 	}, payment_hash, &Some(payment_secret)) {
 		check_payment_err(err);
 		false
@@ -330,6 +331,7 @@ fn send_hop_payment(source: &ChanMan, middle: &ChanMan, middle_chan_id: u64, des
 			fee_msat: amt,
 			cltv_expiry_delta: 200,
 		}]],
+		payee: None,
 	}, payment_hash, &Some(payment_secret)) {
 		check_payment_err(err);
 		false

lightning/src/ln/channel.rs

@@ -5768,6 +5768,7 @@ mod tests {
 				first_hop_htlc_msat: 548,
 				payment_id: PaymentId([42; 32]),
 				payment_secret: None,
+				payee: None,
 			}
 		});
 

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::{Route, RouteHop};
+use routing::router::{Payee, PaymentPathRetry, Route, RouteHop};
 use ln::msgs;
 use ln::msgs::NetAddress;
 use ln::onion_utils;
@@ -201,6 +201,7 @@ pub(crate) enum HTLCSource {
 		first_hop_htlc_msat: u64,
 		payment_id: PaymentId,
 		payment_secret: Option<PaymentSecret>,
+		payee: Option<Payee>,
 	},
 }
 #[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash
@@ -211,13 +212,14 @@ impl core::hash::Hash for HTLCSource {
 				0u8.hash(hasher);
 				prev_hop_data.hash(hasher);
 			},
-			HTLCSource::OutboundRoute { path, session_priv, payment_id, payment_secret, first_hop_htlc_msat } => {
+			HTLCSource::OutboundRoute { path, session_priv, payment_id, payment_secret, first_hop_htlc_msat, payee } => {
 				1u8.hash(hasher);
 				path.hash(hasher);
 				session_priv[..].hash(hasher);
 				payment_id.hash(hasher);
 				payment_secret.hash(hasher);
 				first_hop_htlc_msat.hash(hasher);
+				payee.hash(hasher);
 			},
 		}
 	}
@@ -231,6 +233,7 @@ impl HTLCSource {
 			first_hop_htlc_msat: 0,
 			payment_id: PaymentId([2; 32]),
 			payment_secret: None,
+			payee: None,
 		}
 	}
 }
@@ -2021,7 +2024,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	}
 
 	// Only public for testing, this should otherwise never be called direcly
-	pub(crate) fn send_payment_along_path(&self, path: &Vec<RouteHop>, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>, total_value: u64, cur_height: u32, payment_id: PaymentId, keysend_preimage: &Option<PaymentPreimage>) -> Result<(), APIError> {
+	pub(crate) fn send_payment_along_path(&self, path: &Vec<RouteHop>, payee: &Option<Payee>, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>, total_value: u64, cur_height: u32, payment_id: PaymentId, keysend_preimage: &Option<PaymentPreimage>) -> Result<(), APIError> {
 		log_trace!(self.logger, "Attempting to send payment for path with next hop {}", path.first().unwrap().short_channel_id);
 		let prng_seed = self.keys_manager.get_secure_random_bytes();
 		let session_priv_bytes = self.keys_manager.get_secure_random_bytes();
@@ -2071,6 +2074,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 							first_hop_htlc_msat: htlc_msat,
 							payment_id,
 							payment_secret: payment_secret.clone(),
+							payee: payee.clone(),
 						}, onion_packet, &self.logger),
 					channel_state, chan);
 
@@ -2209,7 +2213,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		let cur_height = self.best_block.read().unwrap().height() + 1;
 		let mut results = Vec::new();
 		for path in route.paths.iter() {
-			results.push(self.send_payment_along_path(&path, &payment_hash, payment_secret, total_value, cur_height, payment_id, &keysend_preimage));
+			results.push(self.send_payment_along_path(&path, &route.payee, &payment_hash, payment_secret, total_value, cur_height, payment_id, &keysend_preimage));
 		}
 		let mut has_ok = false;
 		let mut has_err = false;
@@ -3098,14 +3102,22 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					self.fail_htlc_backwards_internal(channel_state,
 						htlc_src, &payment_hash, HTLCFailReason::Reason { failure_code, data: onion_failure_data});
 				},
-				HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
+				HTLCSource::OutboundRoute { session_priv, payment_id, path, payee, .. } => {
 					let mut session_priv_bytes = [0; 32];
 					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) {
-						if payment.get_mut().remove(&session_priv_bytes, Some(path.last().unwrap().fee_msat)) &&
+						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()
 						{
+							let retry = if let Some(payee_data) = payee {
+								Some(PaymentPathRetry {
+									payee: payee_data,
+									final_value_msat: path_last_hop.fee_msat,
+									final_cltv_expiry_delta: path_last_hop.cltv_expiry_delta,
+								})
+							} else { None };
 							self.pending_events.lock().unwrap().push(
 								events::Event::PaymentPathFailed {
 									payment_hash,
@@ -3114,7 +3126,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 									all_paths_failed: payment.get().remaining_parts() == 0,
 									path: path.clone(),
 									short_channel_id: None,
-									retry: None,
+									retry,
 									#[cfg(test)]
 									error_code: None,
 									#[cfg(test)]
@@ -3146,13 +3158,14 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		// from block_connected which may run during initialization prior to the chain_monitor
 		// being fully configured. See the docs for `ChannelManagerReadArgs` for more.
 		match source {
-			HTLCSource::OutboundRoute { ref path, session_priv, payment_id, .. } => {
+			HTLCSource::OutboundRoute { ref path, session_priv, payment_id, ref payee, .. } => {
 				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 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().unwrap().fee_msat)) {
+					if !payment.get_mut().remove(&session_priv_bytes, Some(path_last_hop.fee_msat)) {
 						log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
 						return;
 					}
@@ -3167,8 +3180,15 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
 					return;
 				}
-				log_trace!(self.logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
 				mem::drop(channel_state_lock);
+				let retry = if let Some(payee_data) = payee {
+					Some(PaymentPathRetry {
+						payee: payee_data.clone(),
+						final_value_msat: path_last_hop.fee_msat,
+						final_cltv_expiry_delta: path_last_hop.cltv_expiry_delta,
+					})
+				} else { None };
+				log_trace!(self.logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
 				match &onion_error {
 					&HTLCFailReason::LightningError { ref err } => {
 #[cfg(test)]
@@ -3186,7 +3206,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 								all_paths_failed,
 								path: path.clone(),
 								short_channel_id,
-								retry: None,
+								retry,
 #[cfg(test)]
 								error_code: onion_error_code,
 #[cfg(test)]
@@ -3215,7 +3235,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 								all_paths_failed,
 								path: path.clone(),
 								short_channel_id: Some(path.first().unwrap().short_channel_id),
-								retry: None,
+								retry,
 #[cfg(test)]
 								error_code: Some(*failure_code),
 #[cfg(test)]
@@ -5378,12 +5398,14 @@ impl Readable for HTLCSource {
 				let mut path = Some(Vec::new());
 				let mut payment_id = None;
 				let mut payment_secret = None;
+				let mut payee = None;
 				read_tlv_fields!(reader, {
 					(0, session_priv, required),
 					(1, payment_id, option),
 					(2, first_hop_htlc_msat, required),
 					(3, payment_secret, option),
 					(4, path, vec_type),
+					(5, payee, option),
 				});
 				if payment_id.is_none() {
 					// For backwards compat, if there was no payment_id written, use the session_priv bytes
@@ -5396,6 +5418,7 @@ impl Readable for HTLCSource {
 					path: path.unwrap(),
 					payment_id: payment_id.unwrap(),
 					payment_secret,
+					payee,
 				})
 			}
 			1 => Ok(HTLCSource::PreviousHopData(Readable::read(reader)?)),
@@ -5407,7 +5430,7 @@ impl Readable for HTLCSource {
 impl Writeable for HTLCSource {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::io::Error> {
 		match self {
-			HTLCSource::OutboundRoute { ref session_priv, ref first_hop_htlc_msat, ref path, payment_id, payment_secret } => {
+			HTLCSource::OutboundRoute { ref session_priv, ref first_hop_htlc_msat, ref path, payment_id, payment_secret, payee } => {
 				0u8.write(writer)?;
 				let payment_id_opt = Some(payment_id);
 				write_tlv_fields!(writer, {
@@ -5416,6 +5439,7 @@ impl Writeable for HTLCSource {
 					(2, first_hop_htlc_msat, required),
 					(3, payment_secret, option),
 					(4, path, vec_type),
+					(5, payee, option),
 				 });
 			}
 			HTLCSource::PreviousHopData(ref field) => {
@@ -6160,7 +6184,7 @@ mod tests {
 		// Use the utility function send_payment_along_path to send the payment with MPP data which
 		// indicates there are more HTLCs coming.
 		let cur_height = CHAN_CONFIRM_DEPTH + 1; // route_payment calls send_payment, which adds 1 to the current height. So we do the same here to match.
-		nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
+		nodes[0].node.send_payment_along_path(&route.paths[0], &route.payee, &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
 		check_added_monitors!(nodes[0], 1);
 		let mut events = nodes[0].node.get_and_clear_pending_msg_events();
 		assert_eq!(events.len(), 1);
@@ -6190,7 +6214,7 @@ mod tests {
 		expect_payment_failed!(nodes[0], our_payment_hash, true);
 
 		// Send the second half of the original MPP payment.
-		nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
+		nodes[0].node.send_payment_along_path(&route.paths[0], &route.payee, &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
 		check_added_monitors!(nodes[0], 1);
 		let mut events = nodes[0].node.get_and_clear_pending_msg_events();
 		assert_eq!(events.len(), 1);

lightning/src/ln/functional_test_utils.rs

@@ -1114,9 +1114,12 @@ macro_rules! expect_payment_failed_with_update {
 		let events = $node.node.get_and_clear_pending_events();
 		assert_eq!(events.len(), 1);
 		match events[0] {
-			Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, ref network_update, ref error_code, ref error_data, .. } => {
+			Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, ref network_update, ref error_code, ref error_data, ref path, ref retry, .. } => {
 				assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash");
 				assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest value");
+				assert!(retry.is_some(), "expected retry.is_some()");
+				assert_eq!(retry.as_ref().unwrap().final_value_msat, path.last().unwrap().fee_msat, "Retry amount should match last hop in path");
+				assert_eq!(retry.as_ref().unwrap().payee.pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path");
 				assert!(error_code.is_some(), "expected error_code.is_some() = true");
 				assert!(error_data.is_some(), "expected error_data.is_some() = true");
 				match network_update {
@@ -1143,9 +1146,12 @@ macro_rules! expect_payment_failed {
 		let events = $node.node.get_and_clear_pending_events();
 		assert_eq!(events.len(), 1);
 		match events[0] {
-			Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, network_update: _, ref error_code, ref error_data, .. } => {
+			Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, network_update: _, ref error_code, ref error_data, ref path, ref retry, .. } => {
 				assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash");
 				assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest value");
+				assert!(retry.is_some(), "expected retry.is_some()");
+				assert_eq!(retry.as_ref().unwrap().final_value_msat, path.last().unwrap().fee_msat, "Retry amount should match last hop in path");
+				assert_eq!(retry.as_ref().unwrap().payee.pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path");
 				assert!(error_code.is_some(), "expected error_code.is_some() = true");
 				assert!(error_data.is_some(), "expected error_data.is_some() = true");
 				$(

lightning/src/ln/functional_tests.rs

@@ -919,7 +919,7 @@ fn fake_network_test() {
 	});
 	hops[1].fee_msat = chan_4.1.contents.fee_base_msat as u64 + chan_4.1.contents.fee_proportional_millionths as u64 * hops[2].fee_msat as u64 / 1000000;
 	hops[0].fee_msat = chan_3.0.contents.fee_base_msat as u64 + chan_3.0.contents.fee_proportional_millionths as u64 * hops[1].fee_msat as u64 / 1000000;
-	let payment_preimage_1 = send_along_route(&nodes[1], Route { paths: vec![hops] }, &vec!(&nodes[2], &nodes[3], &nodes[1])[..], 1000000).0;
+	let payment_preimage_1 = send_along_route(&nodes[1], Route { paths: vec![hops], payee: None }, &vec!(&nodes[2], &nodes[3], &nodes[1])[..], 1000000).0;
 
 	let mut hops = Vec::with_capacity(3);
 	hops.push(RouteHop {
@@ -948,7 +948,7 @@ fn fake_network_test() {
 	});
 	hops[1].fee_msat = chan_2.1.contents.fee_base_msat as u64 + chan_2.1.contents.fee_proportional_millionths as u64 * hops[2].fee_msat as u64 / 1000000;
 	hops[0].fee_msat = chan_3.1.contents.fee_base_msat as u64 + chan_3.1.contents.fee_proportional_millionths as u64 * hops[1].fee_msat as u64 / 1000000;
-	let payment_hash_2 = send_along_route(&nodes[1], Route { paths: vec![hops] }, &vec!(&nodes[3], &nodes[2], &nodes[1])[..], 1000000).1;
+	let payment_hash_2 = send_along_route(&nodes[1], Route { paths: vec![hops], payee: None }, &vec!(&nodes[3], &nodes[2], &nodes[1])[..], 1000000).1;
 
 	// Claim the rebalances...
 	fail_payment(&nodes[1], &vec!(&nodes[3], &nodes[2], &nodes[1])[..], payment_hash_2);
@@ -3912,7 +3912,7 @@ fn do_test_htlc_timeout(send_partial_mpp: bool) {
 		// indicates there are more HTLCs coming.
 		let cur_height = CHAN_CONFIRM_DEPTH + 1; // route_payment calls send_payment, which adds 1 to the current height. So we do the same here to match.
 		let payment_id = PaymentId([42; 32]);
-		nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200000, cur_height, payment_id, &None).unwrap();
+		nodes[0].node.send_payment_along_path(&route.paths[0], &route.payee, &our_payment_hash, &Some(payment_secret), 200000, cur_height, payment_id, &None).unwrap();
 		check_added_monitors!(nodes[0], 1);
 		let mut events = nodes[0].node.get_and_clear_pending_msg_events();
 		assert_eq!(events.len(), 1);

lightning/src/ln/onion_utils.rs

@@ -555,6 +555,7 @@ mod tests {
 						short_channel_id: 0, fee_msat: 0, cltv_expiry_delta: 0 // Test vectors are garbage and not generateble from a RouteHop, we fill in payloads manually
 					},
 			]],
+			payee: None,
 		};
 
 		let session_priv = SecretKey::from_slice(&hex::decode("4141414141414141414141414141414141414141414141414141414141414141").unwrap()[..]).unwrap();

lightning/src/routing/router.rs

@@ -70,6 +70,12 @@ pub struct Route {
 	/// given path is variable, keeping the length of any path to less than 20 should currently
 	/// ensure it is viable.
 	pub paths: Vec<Vec<RouteHop>>,
+	/// The `payee` parameter passed to [`get_route`].
+	/// This is used by `ChannelManager` to track information which may be required for retries,
+	/// provided back to you via [`Event::PaymentPathFailed`].
+	///
+	/// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed
+	pub payee: Option<Payee>,
 }
 
 impl Route {
@@ -106,7 +112,9 @@ impl Writeable for Route {
 				hop.write(writer)?;
 			}
 		}
-		write_tlv_fields!(writer, {});
+		write_tlv_fields!(writer, {
+			(1, self.payee, option),
+		});
 		Ok(())
 	}
 }
@@ -124,8 +132,11 @@ impl Readable for Route {
 			}
 			paths.push(hops);
 		}
-		read_tlv_fields!(reader, {});
-		Ok(Route { paths })
+		let mut payee = None;
+		read_tlv_fields!(reader, {
+			(1, payee, option),
+		});
+		Ok(Route { paths, payee })
 	}
 }
 
@@ -153,10 +164,10 @@ impl_writeable_tlv_based!(PaymentPathRetry, {
 });
 
 /// The recipient of a payment.
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Hash, PartialEq, Eq)]
 pub struct Payee {
 	/// The node id of the payee.
-	pubkey: PublicKey,
+	pub pubkey: PublicKey,
 
 	/// Features supported by the payee.
 	///
@@ -1442,7 +1453,10 @@ where L::Target: Logger {
 		}
 	}
 
-	let route = Route { paths: selected_paths.into_iter().map(|path| path.into_iter().collect()).collect::<Result<Vec<_>, _>>()? };
+	let route = Route {
+		paths: selected_paths.into_iter().map(|path| path.into_iter().collect()).collect::<Result<Vec<_>, _>>()?,
+		payee: Some(payee.clone()),
+	};
 	log_info!(logger, "Got route to {}: {}", payee.pubkey, log_route!(route));
 	Ok(route)
 }
@@ -4600,6 +4614,7 @@ mod tests {
 					short_channel_id: 0, fee_msat: 225, cltv_expiry_delta: 0
 				},
 			]],
+			payee: None,
 		};
 
 		assert_eq!(route.get_total_fees(), 250);
@@ -4632,6 +4647,7 @@ mod tests {
 					short_channel_id: 0, fee_msat: 150, cltv_expiry_delta: 0
 				},
 			]],
+			payee: None,
 		};
 
 		assert_eq!(route.get_total_fees(), 200);
@@ -4643,7 +4659,7 @@ mod tests {
 		// In an earlier version of `Route::get_total_fees` and `Route::get_total_amount`, they
 		// would both panic if the route was completely empty. We test to ensure they return 0
 		// here, even though its somewhat nonsensical as a route.
-		let route = Route { paths: Vec::new() };
+		let route = Route { paths: Vec::new(), payee: None };
 
 		assert_eq!(route.get_total_fees(), 0);
 		assert_eq!(route.get_total_amount(), 0);