Merge pull request lightningdevkit#3490 from TheBlueMatt/2024-12-nits

Fix fuzz deadlock

Author: valentinewallace

fuzz/src/chanmon_consistency.rs

@@ -47,7 +47,8 @@ use lightning::events::MessageSendEventsProvider;
 use lightning::ln::channel::FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
 use lightning::ln::channel_state::ChannelDetails;
 use lightning::ln::channelmanager::{
-	ChainParameters, ChannelManager, ChannelManagerReadArgs, PaymentId, RecipientOnionFields, Retry,
+	ChainParameters, ChannelManager, ChannelManagerReadArgs, PaymentId, RecentPaymentDetails,
+	RecipientOnionFields, Retry,
 };
 use lightning::ln::functional_test_utils::*;
 use lightning::ln::inbound_payment::ExpandedKey;
@@ -64,7 +65,6 @@ use lightning::routing::router::{
 use lightning::sign::{EntropySource, InMemorySigner, NodeSigner, Recipient, SignerProvider};
 use lightning::types::payment::{PaymentHash, PaymentPreimage, PaymentSecret};
 use lightning::util::config::UserConfig;
-use lightning::util::errors::APIError;
 use lightning::util::hash_tables::*;
 use lightning::util::logger::Logger;
 use lightning::util::ser::{Readable, ReadableArgs, Writeable, Writer};
@@ -442,57 +442,19 @@ impl KeyProvider {
 
 // Returns a bool indicating whether the payment failed.
 #[inline]
-fn check_payment_send_events(
-	source: &ChanMan, amt: u64, min_sendable: u64, max_sendable: u64,
-) -> bool {
-	let mut payment_failed = false;
-	let events = source.get_and_clear_pending_events();
-	assert!(events.len() == 2 || events.len() == 0);
-	for ev in events {
-		match ev {
-			events::Event::PaymentPathFailed {
-				failure: events::PathFailure::InitialSend { err },
-				..
-			} => {
-				check_api_err(err, amt > max_sendable || amt < min_sendable);
+fn check_payment_send_events(source: &ChanMan, sent_payment_id: PaymentId) -> bool {
+	for payment in source.list_recent_payments() {
+		match payment {
+			RecentPaymentDetails::Pending { payment_id, .. } if payment_id == sent_payment_id => {
+				return true;
 			},
-			events::Event::PaymentFailed { .. } => {},
-			_ => panic!(),
-		};
-		payment_failed = true;
-	}
-	// Note that while the max is a strict upper-bound, we can occasionally send substantially
-	// below the minimum, with some gap which is unusable immediately below the minimum. Thus,
-	// we don't check against min_value_sendable here.
-	assert!(payment_failed || (amt <= max_sendable));
-	payment_failed
-}
-
-#[inline]
-fn check_api_err(api_err: APIError, sendable_bounds_violated: bool) {
-	match api_err {
-		APIError::APIMisuseError { .. } => panic!("We can't misuse the API"),
-		APIError::FeeRateTooHigh { .. } => panic!("We can't send too much fee?"),
-		APIError::InvalidRoute { .. } => panic!("Our routes should work"),
-		APIError::ChannelUnavailable { err } => {
-			// Test the error against a list of errors we can hit, and reject
-			// all others. If you hit this panic, the list of acceptable errors
-			// is probably just stale and you should add new messages here.
-			match err.as_str() {
-				"Peer for first hop currently disconnected" => {},
-				_ if err.starts_with("Cannot send less than our next-HTLC minimum - ") => {},
-				_ if err.starts_with("Cannot send more than our next-HTLC maximum - ") => {},
-				_ => panic!("{}", err),
-			}
-			assert!(sendable_bounds_violated);
-		},
-		APIError::MonitorUpdateInProgress => {
-			// We can (obviously) temp-fail a monitor update
-		},
-		APIError::IncompatibleShutdownScript { .. } => {
-			panic!("Cannot send an incompatible shutdown script")
-		},
+			RecentPaymentDetails::Abandoned { payment_id, .. } if payment_id == sent_payment_id => {
+				return false;
+			},
+			_ => {},
+		}
 	}
+	return false;
 }
 
 type ChanMan<'a> = ChannelManager<
@@ -552,8 +514,11 @@ fn send_payment(
 		.find(|chan| chan.short_channel_id == Some(dest_chan_id))
 		.map(|chan| (chan.next_outbound_htlc_minimum_msat, chan.next_outbound_htlc_limit_msat))
 		.unwrap_or((0, 0));
-	let mut next_routes = source.router.next_routes.lock().unwrap();
-	next_routes.push_back(Route {
+	let route_params = RouteParameters::from_payment_params_and_value(
+		PaymentParameters::from_node_id(source.get_our_node_id(), TEST_FINAL_CLTV),
+		amt,
+	);
+	source.router.next_routes.lock().unwrap().push_back(Route {
 		paths: vec![Path {
 			hops: vec![RouteHop {
 				pubkey: dest.get_our_node_id(),
@@ -566,22 +531,22 @@ fn send_payment(
 			}],
 			blinded_tail: None,
 		}],
-		route_params: None,
+		route_params: Some(route_params.clone()),
 	});
-	let route_params = RouteParameters::from_payment_params_and_value(
-		PaymentParameters::from_node_id(source.get_our_node_id(), TEST_FINAL_CLTV),
-		amt,
-	);
-	if let Err(err) = source.send_payment(
-		payment_hash,
-		RecipientOnionFields::secret_only(payment_secret),
-		PaymentId(payment_id),
-		route_params,
-		Retry::Attempts(0),
-	) {
-		panic!("Errored with {:?} on initial payment send", err);
-	} else {
-		check_payment_send_events(source, amt, min_value_sendable, max_value_sendable)
+	let onion = RecipientOnionFields::secret_only(payment_secret);
+	let payment_id = PaymentId(payment_id);
+	let res =
+		source.send_payment(payment_hash, onion, payment_id, route_params, Retry::Attempts(0));
+	match res {
+		Err(err) => {
+			panic!("Errored with {:?} on initial payment send", err);
+		},
+		Ok(()) => {
+			let expect_failure = amt < min_value_sendable || amt > max_value_sendable;
+			let succeeded = check_payment_send_events(source, payment_id);
+			assert_eq!(succeeded, !expect_failure);
+			succeeded
+		},
 	}
 }
 
@@ -623,8 +588,11 @@ fn send_hop_payment(
 		.map(|chan| (chan.next_outbound_htlc_minimum_msat, chan.next_outbound_htlc_limit_msat))
 		.unwrap_or((0, 0));
 	let first_hop_fee = 50_000;
-	let mut next_routes = source.router.next_routes.lock().unwrap();
-	next_routes.push_back(Route {
+	let route_params = RouteParameters::from_payment_params_and_value(
+		PaymentParameters::from_node_id(source.get_our_node_id(), TEST_FINAL_CLTV),
+		amt,
+	);
+	source.router.next_routes.lock().unwrap().push_back(Route {
 		paths: vec![Path {
 			hops: vec![
 				RouteHop {
@@ -648,23 +616,23 @@ fn send_hop_payment(
 			],
 			blinded_tail: None,
 		}],
-		route_params: None,
+		route_params: Some(route_params.clone()),
 	});
-	let route_params = RouteParameters::from_payment_params_and_value(
-		PaymentParameters::from_node_id(source.get_our_node_id(), TEST_FINAL_CLTV),
-		amt,
-	);
-	if let Err(err) = source.send_payment(
-		payment_hash,
-		RecipientOnionFields::secret_only(payment_secret),
-		PaymentId(payment_id),
-		route_params,
-		Retry::Attempts(0),
-	) {
-		panic!("Errored with {:?} on initial payment send", err);
-	} else {
-		let sent_amt = amt + first_hop_fee;
-		check_payment_send_events(source, sent_amt, min_value_sendable, max_value_sendable)
+	let onion = RecipientOnionFields::secret_only(payment_secret);
+	let payment_id = PaymentId(payment_id);
+	let res =
+		source.send_payment(payment_hash, onion, payment_id, route_params, Retry::Attempts(0));
+	match res {
+		Err(err) => {
+			panic!("Errored with {:?} on initial payment send", err);
+		},
+		Ok(()) => {
+			let sent_amt = amt + first_hop_fee;
+			let expect_failure = sent_amt < min_value_sendable || sent_amt > max_value_sendable;
+			let succeeded = check_payment_send_events(source, payment_id);
+			assert_eq!(succeeded, !expect_failure);
+			succeeded
+		},
 	}
 }
 

lightning-persister/src/test_utils.rs

@@ -67,32 +67,46 @@ pub(crate) fn do_test_data_migration<S: MigratableKVStore, T: MigratableKVStore>
 ) {
 	// We fill the source with some bogus keys.
 	let dummy_data = [42u8; 32];
-	let num_primary_namespaces = 2;
-	let num_secondary_namespaces = 2;
+	let num_primary_namespaces = 3;
+	let num_secondary_namespaces = 3;
 	let num_keys = 3;
+	let mut expected_keys = Vec::new();
 	for i in 0..num_primary_namespaces {
-		let primary_namespace =
-			format!("testspace{}", KVSTORE_NAMESPACE_KEY_ALPHABET.chars().nth(i).unwrap());
+		let primary_namespace = if i == 0 {
+			String::new()
+		} else {
+			format!("testspace{}", KVSTORE_NAMESPACE_KEY_ALPHABET.chars().nth(i).unwrap())
+		};
 		for j in 0..num_secondary_namespaces {
-			let secondary_namespace =
-				format!("testsubspace{}", KVSTORE_NAMESPACE_KEY_ALPHABET.chars().nth(j).unwrap());
+			let secondary_namespace = if i == 0 || j == 0 {
+				String::new()
+			} else {
+				format!("testsubspace{}", KVSTORE_NAMESPACE_KEY_ALPHABET.chars().nth(j).unwrap())
+			};
 			for k in 0..num_keys {
 				let key =
 					format!("testkey{}", KVSTORE_NAMESPACE_KEY_ALPHABET.chars().nth(k).unwrap());
 				source_store
 					.write(&primary_namespace, &secondary_namespace, &key, &dummy_data)
 					.unwrap();
+				expected_keys.push((primary_namespace.clone(), secondary_namespace.clone(), key));
 			}
 		}
 	}
-	let total_num_entries = num_primary_namespaces * num_secondary_namespaces * num_keys;
-	let all_keys = source_store.list_all_keys().unwrap();
-	assert_eq!(all_keys.len(), total_num_entries);
+	expected_keys.sort();
+	expected_keys.dedup();
+
+	let mut source_list = source_store.list_all_keys().unwrap();
+	source_list.sort();
+	assert_eq!(source_list, expected_keys);
 
 	migrate_kv_store_data(source_store, target_store).unwrap();
 
-	assert_eq!(target_store.list_all_keys().unwrap().len(), total_num_entries);
-	for (p, s, k) in &all_keys {
+	let mut target_list = target_store.list_all_keys().unwrap();
+	target_list.sort();
+	assert_eq!(target_list, expected_keys);
+
+	for (p, s, k) in expected_keys.iter() {
 		assert_eq!(target_store.read(p, s, k).unwrap(), dummy_data);
 	}
 }

lightning/src/ln/outbound_payment.rs

@@ -1226,7 +1226,8 @@ impl OutboundPayments {
 
 		if route.route_params.as_ref() != Some(route_params) {
 			debug_assert!(false,
-				"Routers are expected to return a Route which includes the requested RouteParameters");
+				"Routers are expected to return a Route which includes the requested RouteParameters. Got {:?}, expected {:?}",
+				route.route_params, route_params);
 			route.route_params = Some(route_params.clone());
 		}