Merge pull request #3801 from joostjager/fulfill-hold-times

Author: TheBlueMatt

fuzz/src/process_onion_failure.rs

@@ -115,7 +115,7 @@ fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 	let path = Path { hops, blinded_tail };
 
 	let htlc_source = HTLCSource::OutboundRoute {
-		path,
+		path: path.clone(),
 		session_priv,
 		first_hop_htlc_msat: 0,
 		payment_id,
@@ -133,8 +133,19 @@ fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 	} else {
 		None
 	};
-	let encrypted_packet = OnionErrorPacket { data: failure_data.into(), attribution_data };
+	let encrypted_packet =
+		OnionErrorPacket { data: failure_data.into(), attribution_data: attribution_data.clone() };
 	lightning::ln::process_onion_failure(&secp_ctx, &logger, &htlc_source, encrypted_packet);
+
+	if let Some(attribution_data) = attribution_data {
+		lightning::ln::decode_fulfill_attribution_data(
+			&secp_ctx,
+			&logger,
+			&path,
+			&session_priv,
+			attribution_data,
+		);
+	}
 }
 
 /// Method that needs to be added manually, {name}_test

lightning-background-processor/src/lib.rs

@@ -2733,6 +2733,7 @@ mod tests {
 				payment_id: PaymentId([42; 32]),
 				payment_hash: None,
 				path: path.clone(),
+				hold_times: Vec::new(),
 			});
 			let event = $receive.expect("PaymentPathSuccessful not handled within deadline");
 			match event {

lightning/src/events/mod.rs

@@ -1102,6 +1102,12 @@ pub enum Event {
 		///
 		/// May contain a closed channel if the HTLC sent along the path was fulfilled on chain.
 		path: Path,
+		/// The hold times as reported by each hop. The unit in which the hold times are expressed are 100's of
+		/// milliseconds. So a hop reporting 2 is a hold time that corresponds to roughly 200 milliseconds. As earlier
+		/// hops hold on to an HTLC for longer, the hold times in the list are expected to decrease. When our peer
+		/// didn't provide attribution data, the list is empty. The same applies to HTLCs that were resolved onchain.
+		/// Because of unavailability of hold times, the list may be shorter than the number of hops in the path.
+		hold_times: Vec<u32>,
 	},
 	/// Indicates an outbound HTLC we sent failed, likely due to an intermediary node being unable to
 	/// handle the HTLC.
@@ -1153,7 +1159,11 @@ pub enum Event {
 		error_code: Option<u16>,
 		#[cfg(any(test, feature = "_test_utils"))]
 		error_data: Option<Vec<u8>>,
-		#[cfg(any(test, feature = "_test_utils"))]
+		/// The hold times as reported by each hop. The unit in which the hold times are expressed are 100's of
+		/// milliseconds. So a hop reporting 2 is a hold time that corresponds to roughly 200 milliseconds. As earlier
+		/// hops hold on to an HTLC for longer, the hold times in the list are expected to decrease. When our peer
+		/// didn't provide attribution data, the list is empty. The same applies to HTLCs that were resolved onchain.
+		/// Because of unavailability of hold times, the list may be shorter than the number of hops in the path.
 		hold_times: Vec<u32>,
 	},
 	/// Indicates that a probe payment we sent returned successful, i.e., only failed at the destination.
@@ -1792,16 +1802,13 @@ impl Writeable for Event {
 				ref error_code,
 				#[cfg(any(test, feature = "_test_utils"))]
 				ref error_data,
-				#[cfg(any(test, feature = "_test_utils"))]
 				ref hold_times,
 			} => {
 				3u8.write(writer)?;
 				#[cfg(any(test, feature = "_test_utils"))]
 				error_code.write(writer)?;
 				#[cfg(any(test, feature = "_test_utils"))]
 				error_data.write(writer)?;
-				#[cfg(any(test, feature = "_test_utils"))]
-				hold_times.write(writer)?;
 				write_tlv_fields!(writer, {
 					(0, payment_hash, required),
 					(1, None::<NetworkUpdate>, option), // network_update in LDK versions prior to 0.0.114
@@ -1813,6 +1820,7 @@ impl Writeable for Event {
 					(9, None::<RouteParameters>, option), // retry in LDK versions prior to 0.0.115
 					(11, payment_id, option),
 					(13, failure, required),
+					(15, *hold_times, optional_vec),
 				});
 			},
 			&Event::PendingHTLCsForwardable { time_forwardable: _ } => {
@@ -1910,10 +1918,16 @@ impl Writeable for Event {
 					(4, funding_info, required),
 				})
 			},
-			&Event::PaymentPathSuccessful { ref payment_id, ref payment_hash, ref path } => {
+			&Event::PaymentPathSuccessful {
+				ref payment_id,
+				ref payment_hash,
+				ref path,
+				ref hold_times,
+			} => {
 				13u8.write(writer)?;
 				write_tlv_fields!(writer, {
 					(0, payment_id, required),
+					(1, *hold_times, optional_vec),
 					(2, payment_hash, option),
 					(4, path.hops, required_vec),
 					(6, path.blinded_tail, option),
@@ -2232,8 +2246,6 @@ impl MaybeReadable for Event {
 					let error_code = Readable::read(reader)?;
 					#[cfg(any(test, feature = "_test_utils"))]
 					let error_data = Readable::read(reader)?;
-					#[cfg(any(test, feature = "_test_utils"))]
-					let hold_times = Readable::read(reader)?;
 					let mut payment_hash = PaymentHash([0; 32]);
 					let mut payment_failed_permanently = false;
 					let mut network_update = None;
@@ -2242,6 +2254,7 @@ impl MaybeReadable for Event {
 					let mut short_channel_id = None;
 					let mut payment_id = None;
 					let mut failure_opt = None;
+					let mut hold_times = None;
 					read_tlv_fields!(reader, {
 						(0, payment_hash, required),
 						(1, network_update, upgradable_option),
@@ -2253,7 +2266,9 @@ impl MaybeReadable for Event {
 						(7, short_channel_id, option),
 						(11, payment_id, option),
 						(13, failure_opt, upgradable_option),
+						(15, hold_times, optional_vec),
 					});
+					let hold_times = hold_times.unwrap_or(Vec::new());
 					let failure =
 						failure_opt.unwrap_or_else(|| PathFailure::OnPath { network_update });
 					Ok(Some(Event::PaymentPathFailed {
@@ -2267,7 +2282,6 @@ impl MaybeReadable for Event {
 						error_code,
 						#[cfg(any(test, feature = "_test_utils"))]
 						error_data,
-						#[cfg(any(test, feature = "_test_utils"))]
 						hold_times,
 					}))
 				};
@@ -2413,14 +2427,19 @@ impl MaybeReadable for Event {
 				let mut f = || {
 					_init_and_read_len_prefixed_tlv_fields!(reader, {
 						(0, payment_id, required),
+						(1, hold_times, optional_vec),
 						(2, payment_hash, option),
 						(4, path, required_vec),
 						(6, blinded_tail, option),
 					});
+
+					let hold_times = hold_times.unwrap_or(Vec::new());
+
 					Ok(Some(Event::PaymentPathSuccessful {
 						payment_id: payment_id.0.unwrap(),
 						payment_hash,
 						path: Path { hops: path, blinded_tail },
+						hold_times,
 					}))
 				};
 				f()

lightning/src/ln/async_payments_tests.rs

@@ -607,7 +607,7 @@ fn async_receive_flow_success() {
 	let args = PassAlongPathArgs::new(&nodes[0], route[0], amt_msat, payment_hash, ev);
 	let claimable_ev = do_pass_along_path(args).unwrap();
 	let keysend_preimage = extract_payment_preimage(&claimable_ev);
-	let res =
+	let (res, _) =
 		claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], route, keysend_preimage));
 	assert_eq!(res, Some(PaidBolt12Invoice::StaticInvoice(static_invoice)));
 }
@@ -1726,7 +1726,7 @@ fn refresh_static_invoices() {
 	let claimable_ev = do_pass_along_path(args).unwrap();
 	let keysend_preimage = extract_payment_preimage(&claimable_ev);
 	let res = claim_payment_along_route(ClaimAlongRouteArgs::new(sender, route, keysend_preimage));
-	assert_eq!(res, Some(PaidBolt12Invoice::StaticInvoice(updated_invoice)));
+	assert_eq!(res.0, Some(PaidBolt12Invoice::StaticInvoice(updated_invoice)));
 }
 
 #[cfg_attr(feature = "std", ignore)]
@@ -2056,5 +2056,5 @@ fn invoice_server_is_not_channel_peer() {
 	let claimable_ev = do_pass_along_path(args).unwrap();
 	let keysend_preimage = extract_payment_preimage(&claimable_ev);
 	let res = claim_payment_along_route(ClaimAlongRouteArgs::new(sender, route, keysend_preimage));
-	assert_eq!(res, Some(PaidBolt12Invoice::StaticInvoice(invoice)));
+	assert_eq!(res.0, Some(PaidBolt12Invoice::StaticInvoice(invoice)));
 }

lightning/src/ln/blinded_payment_tests.rs

@@ -2341,6 +2341,8 @@ fn test_trampoline_unblinded_receive() {
 			None,
 		).unwrap();
 
+		// Use a different session key to construct the replacement onion packet. Note that the sender isn't aware of
+		// this and won't be able to decode the fulfill hold times.
 		let outer_session_priv = secret_from_hex("e52c20461ed7acd46c4e7b591a37610519179482887bd73bf3b94617f8f03677");
 
 		let (outer_payloads, _, _) = onion_utils::build_onion_payloads(&route.paths[0], outer_total_msat, &recipient_onion_fields, outer_starting_htlc_offset, &None, None, Some(trampoline_packet)).unwrap();

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -2890,8 +2890,12 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_f
 		as_raa = Some(get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, node_b_id));
 	}
 
-	let fulfill_msg =
-		msgs::UpdateFulfillHTLC { channel_id: chan_id_2, htlc_id: 0, payment_preimage };
+	let mut fulfill_msg = msgs::UpdateFulfillHTLC {
+		channel_id: chan_id_2,
+		htlc_id: 0,
+		payment_preimage,
+		attribution_data: None,
+	};
 	if second_fails {
 		nodes[2].node.fail_htlc_backwards(&payment_hash);
 		expect_pending_htlcs_forwardable_and_htlc_handling_failed!(
@@ -2900,15 +2904,24 @@ fn do_test_reconnect_dup_htlc_claims(htlc_status: HTLCStatusAtDupClaim, second_f
 		);
 		check_added_monitors!(nodes[2], 1);
 		get_htlc_update_msgs!(nodes[2], node_b_id);
+	// Note that we don't populate fulfill_msg.attribution_data here, which will lead to hold times being
+	// unavailable.
 	} else {
 		nodes[2].node.claim_funds(payment_preimage);
 		check_added_monitors!(nodes[2], 1);
 		expect_payment_claimed!(nodes[2], payment_hash, 100_000);
 
 		let cs_updates = get_htlc_update_msgs!(nodes[2], node_b_id);
 		assert_eq!(cs_updates.update_fulfill_htlcs.len(), 1);
-		// Check that the message we're about to deliver matches the one generated:
-		assert_eq!(fulfill_msg, cs_updates.update_fulfill_htlcs[0]);
+
+		// Check that the message we're about to deliver matches the one generated. Ignore attribution data.
+		assert_eq!(fulfill_msg.channel_id, cs_updates.update_fulfill_htlcs[0].channel_id);
+		assert_eq!(fulfill_msg.htlc_id, cs_updates.update_fulfill_htlcs[0].htlc_id);
+		assert_eq!(
+			fulfill_msg.payment_preimage,
+			cs_updates.update_fulfill_htlcs[0].payment_preimage
+		);
+		fulfill_msg.attribution_data = cs_updates.update_fulfill_htlcs[0].attribution_data.clone();
 	}
 	nodes[1].node.handle_update_fulfill_htlc(node_c_id, &fulfill_msg);
 	expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(1000), false, false);