Drop PendingHTLCsForwardable event

Author: tnull

fuzz/src/chanmon_consistency.rs

@@ -85,7 +85,7 @@ use bitcoin::secp256k1::{self, Message, PublicKey, Scalar, Secp256k1, SecretKey}
 use lightning::util::dyn_signer::DynSigner;
 
 use std::cell::RefCell;
-use std::cmp::{self, Ordering};
+use std::cmp;
 use std::mem;
 use std::sync::atomic;
 use std::sync::{Arc, Mutex};
@@ -1304,28 +1304,6 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 				// deduplicate the calls here.
 				let mut claim_set = new_hash_map();
 				let mut events = nodes[$node].get_and_clear_pending_events();
-				// Sort events so that PendingHTLCsForwardable get processed last. This avoids a
-				// case where we first process a PendingHTLCsForwardable, then claim/fail on a
-				// PaymentClaimable, claiming/failing two HTLCs, but leaving a just-generated
-				// PaymentClaimable event for the second HTLC in our pending_events (and breaking
-				// our claim_set deduplication).
-				events.sort_by(|a, b| {
-					if let events::Event::PaymentClaimable { .. } = a {
-						if let events::Event::PendingHTLCsForwardable { .. } = b {
-							Ordering::Less
-						} else {
-							Ordering::Equal
-						}
-					} else if let events::Event::PendingHTLCsForwardable { .. } = a {
-						if let events::Event::PaymentClaimable { .. } = b {
-							Ordering::Greater
-						} else {
-							Ordering::Equal
-						}
-					} else {
-						Ordering::Equal
-					}
-				});
 				let had_events = !events.is_empty();
 				for event in events.drain(..) {
 					match event {
@@ -1352,9 +1330,6 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 						},
 						events::Event::PaymentForwarded { .. } if $node == 1 => {},
 						events::Event::ChannelReady { .. } => {},
-						events::Event::PendingHTLCsForwardable { .. } => {
-							nodes[$node].process_pending_htlc_forwards();
-						},
 						events::Event::HTLCHandlingFailed { .. } => {},
 						_ => {
 							if out.may_fail.load(atomic::Ordering::Acquire) {
@@ -1365,6 +1340,9 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 						},
 					}
 				}
+				while nodes[$node].needs_pending_htlc_processing() {
+					nodes[$node].process_pending_htlc_forwards();
+				}
 				had_events
 			}};
 		}
@@ -1806,8 +1784,7 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 								last_pass_no_updates = false;
 								continue;
 							}
-							// ...making sure any pending PendingHTLCsForwardable events are handled and
-							// payments claimed.
+							// ...making sure any payments are claimed.
 							if process_events!(0, false) {
 								last_pass_no_updates = false;
 								continue;

fuzz/src/full_stack.rs

@@ -635,7 +635,6 @@ pub fn do_test(mut data: &[u8], logger: &Arc<dyn Logger>) {
 	let mut loss_detector =
 		MoneyLossDetector::new(&peers, channelmanager.clone(), monitor.clone(), peer_manager);
 
-	let mut should_forward = false;
 	let mut payments_received: Vec<PaymentHash> = Vec::new();
 	let mut intercepted_htlcs: Vec<InterceptId> = Vec::new();
 	let mut payments_sent: u16 = 0;
@@ -785,10 +784,7 @@ pub fn do_test(mut data: &[u8], logger: &Arc<dyn Logger>) {
 				}
 			},
 			7 => {
-				if should_forward {
-					channelmanager.process_pending_htlc_forwards();
-					should_forward = false;
-				}
+				channelmanager.process_pending_htlc_forwards();
 			},
 			8 => {
 				for payment in payments_received.drain(..) {
@@ -1004,9 +1000,6 @@ pub fn do_test(mut data: &[u8], logger: &Arc<dyn Logger>) {
 					//TODO: enhance by fetching random amounts from fuzz input?
 					payments_received.push(payment_hash);
 				},
-				Event::PendingHTLCsForwardable { .. } => {
-					should_forward = true;
-				},
 				Event::HTLCIntercepted { intercept_id, .. } => {
 					if !intercepted_htlcs.contains(&intercept_id) {
 						intercepted_htlcs.push(intercept_id);

lightning-dns-resolver/src/lib.rs

@@ -179,10 +179,7 @@ mod test {
 	use lightning::types::payment::PaymentHash;
 	use lightning::util::logger::Logger;
 
-	use lightning::{
-		commitment_signed_dance, expect_payment_claimed, expect_pending_htlcs_forwardable,
-		get_htlc_update_msgs,
-	};
+	use lightning::{commitment_signed_dance, expect_payment_claimed, get_htlc_update_msgs};
 	use lightning_types::string::UntrustedString;
 
 	use std::ops::Deref;
@@ -413,7 +410,7 @@ mod test {
 		let updates = get_htlc_update_msgs!(nodes[0], payee_id);
 		nodes[1].node.handle_update_add_htlc(payer_id, &updates.update_add_htlcs[0]);
 		commitment_signed_dance!(nodes[1], nodes[0], updates.commitment_signed, false);
-		expect_pending_htlcs_forwardable!(nodes[1]);
+		expect_and_process_pending_htlcs(&nodes[1], false);
 
 		let claimable_events = nodes[1].node.get_and_clear_pending_events();
 		assert_eq!(claimable_events.len(), 1);

lightning/src/events/mod.rs

@@ -50,7 +50,6 @@ use bitcoin::script::ScriptBuf;
 use bitcoin::secp256k1::PublicKey;
 use bitcoin::{OutPoint, Transaction};
 use core::ops::Deref;
-use core::time::Duration;
 
 #[allow(unused_imports)]
 use crate::prelude::*;
@@ -1196,21 +1195,6 @@ pub enum Event {
 		/// with channels in the public network graph.
 		short_channel_id: Option<u64>,
 	},
-	/// Used to indicate that [`ChannelManager::process_pending_htlc_forwards`] should be called at
-	/// a time in the future.
-	///
-	/// # Failure Behavior and Persistence
-	/// This event will eventually be replayed after failures-to-handle (i.e., the event handler
-	/// returning `Err(ReplayEvent ())`) and will be regenerated after restarts.
-	///
-	/// [`ChannelManager::process_pending_htlc_forwards`]: crate::ln::channelmanager::ChannelManager::process_pending_htlc_forwards
-	PendingHTLCsForwardable {
-		/// The minimum amount of time that should be waited prior to calling
-		/// process_pending_htlc_forwards. To increase the effort required to correlate payments,
-		/// you should wait a random amount of time in roughly the range (now + time_forwardable,
-		/// now + 5*time_forwardable).
-		time_forwardable: Duration,
-	},
 	/// Used to indicate that we've intercepted an HTLC forward. This event will only be generated if
 	/// you've encoded an intercept scid in the receiver's invoice route hints using
 	/// [`ChannelManager::get_intercept_scid`] and have set [`UserConfig::accept_intercept_htlcs`].
@@ -1815,11 +1799,7 @@ impl Writeable for Event {
 					(13, failure, required),
 				});
 			},
-			&Event::PendingHTLCsForwardable { time_forwardable: _ } => {
-				4u8.write(writer)?;
-				// Note that we now ignore these on the read end as we'll re-generate them in
-				// ChannelManager, we write them here only for backwards compatibility.
-			},
+			// 4u8 used to be `PendingHTLCsForwardable`
 			&Event::SpendableOutputs { ref outputs, channel_id } => {
 				5u8.write(writer)?;
 				write_tlv_fields!(writer, {

lightning/src/ln/async_signer_tests.rs

@@ -956,7 +956,7 @@ fn do_test_async_commitment_signature_ordering(monitor_update_failure: bool) {
 	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
 	check_added_monitors!(nodes[0], 1);
 
-	expect_pending_htlcs_forwardable!(nodes[1]);
+	expect_and_process_pending_htlcs(&nodes[1], false);
 
 	let events_5 = nodes[1].node.get_and_clear_pending_events();
 	check_payment_claimable(

lightning/src/ln/blinded_payment_tests.rs

@@ -439,7 +439,7 @@ fn do_forward_checks_failure(check: ForwardCheckFail, intro_fails: bool) {
 	check_added_monitors!(nodes[1], 0);
 	do_commitment_signed_dance(&nodes[1], &nodes[0], &updates_0_1.commitment_signed, true, true);
 
-	expect_pending_htlcs_forwardable!(nodes[1]);
+	expect_and_process_pending_htlcs(&nodes[1], false);
 	check_added_monitors!(nodes[1], 1);
 
 	if intro_fails {
@@ -477,7 +477,7 @@ fn do_forward_checks_failure(check: ForwardCheckFail, intro_fails: bool) {
 	check_added_monitors!(nodes[2], 0);
 	do_commitment_signed_dance(&nodes[2], &nodes[1], &updates_1_2.commitment_signed, true, true);
 
-	expect_pending_htlcs_forwardable!(nodes[2]);
+	expect_and_process_pending_htlcs(&nodes[2], false);
 	let failed_destination = match check {
 		ForwardCheckFail::InboundOnionCheck|ForwardCheckFail::ForwardPayloadEncodedAsReceive => HTLCHandlingFailureType::InvalidOnion,
 		ForwardCheckFail::OutboundChannelCheck =>
@@ -534,7 +534,7 @@ fn failed_backwards_to_intro_node() {
 	nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
 	check_added_monitors!(nodes[1], 0);
 	do_commitment_signed_dance(&nodes[1], &nodes[0], &payment_event.commitment_msg, false, false);
-	expect_pending_htlcs_forwardable!(nodes[1]);
+	expect_and_process_pending_htlcs(&nodes[1], false);
 	check_added_monitors!(&nodes[1], 1);
 
 	let mut events = nodes[1].node.get_and_clear_pending_msg_events();
@@ -548,7 +548,7 @@ fn failed_backwards_to_intro_node() {
 	check_added_monitors!(nodes[2], 0);
 	do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event.commitment_msg, true, true);
 
-	expect_pending_htlcs_forwardable!(nodes[2]);
+	expect_and_process_pending_htlcs(&nodes[2], false);
 	expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCHandlingFailureType::InvalidOnion]);
 	check_added_monitors(&nodes[2], 1);
 
@@ -626,7 +626,7 @@ fn do_forward_fail_in_process_pending_htlc_fwds(check: ProcessPendingHTLCsCheck,
 					// Disconnect the next-hop peer so when we go to forward in process_pending_htlc_forwards, the
 					// intro node will error backwards.
 					$curr_node.node.peer_disconnected($next_node.node.get_our_node_id());
-					expect_pending_htlcs_forwardable!($curr_node);
+					expect_and_process_pending_htlcs(&$curr_node, false);
 					expect_htlc_handling_failed_destinations!($curr_node.node.get_and_clear_pending_events(),
 						vec![HTLCHandlingFailureType::Forward { node_id: Some($next_node.node.get_our_node_id()), channel_id: $failed_chan_id }]);
 				},
@@ -636,10 +636,6 @@ fn do_forward_fail_in_process_pending_htlc_fwds(check: ProcessPendingHTLCsCheck,
 					$curr_node.node.force_close_broadcasting_latest_txn(&$failed_chan_id, &$next_node.node.get_our_node_id(), error_message.to_string()).unwrap();
 					let events = $curr_node.node.get_and_clear_pending_events();
 					match events[0] {
-						crate::events::Event::PendingHTLCsForwardable { .. } => {},
-						_ => panic!("Unexpected event {:?}", events),
-					};
-					match events[1] {
 						crate::events::Event::ChannelClosed { .. } => {},
 						_ => panic!("Unexpected event {:?}", events),
 					}
@@ -649,7 +645,6 @@ fn do_forward_fail_in_process_pending_htlc_fwds(check: ProcessPendingHTLCsCheck,
 					$curr_node.node.process_pending_htlc_forwards();
 					expect_htlc_handling_failed_destinations!($curr_node.node.get_and_clear_pending_events(),
 						vec![HTLCHandlingFailureType::InvalidForward { requested_forward_scid: $failed_scid }]);
-					$curr_node.node.process_pending_htlc_forwards();
 				},
 			}
 		}
@@ -662,7 +657,7 @@ fn do_forward_fail_in_process_pending_htlc_fwds(check: ProcessPendingHTLCsCheck,
 		return
 	}
 
-	expect_pending_htlcs_forwardable!(nodes[1]);
+	expect_and_process_pending_htlcs(&nodes[1], false);
 	check_added_monitors!(nodes[1], 1);
 
 	let mut updates_1_2 = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
@@ -729,7 +724,7 @@ fn do_blinded_intercept_payment(intercept_node_fails: bool) {
 	};
 	nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
 	commitment_signed_dance!(nodes[1], nodes[0], &payment_event.commitment_msg, false, true);
-	expect_pending_htlcs_forwardable!(nodes[1]);
+	expect_and_process_pending_htlcs(&nodes[1], false);
 
 	let events = nodes[1].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
@@ -747,15 +742,15 @@ fn do_blinded_intercept_payment(intercept_node_fails: bool) {
 
 	if intercept_node_fails {
 		nodes[1].node.fail_intercepted_htlc(intercept_id).unwrap();
-		expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(nodes[1], [HTLCHandlingFailureType::InvalidForward { requested_forward_scid: intercept_scid }]);
+		expect_pending_htlcs_forwardable_conditions(nodes[1].node.get_and_clear_pending_events(), &[HTLCHandlingFailureType::InvalidForward { requested_forward_scid: intercept_scid }]);
 		nodes[1].node.process_pending_htlc_forwards();
 		check_added_monitors!(&nodes[1], 1);
 		fail_blinded_htlc_backwards(payment_hash, 1, &[&nodes[0], &nodes[1]], false);
 		return
 	}
 
 	nodes[1].node.forward_intercepted_htlc(intercept_id, &channel_id, nodes[2].node.get_our_node_id(), expected_outbound_amount_msat).unwrap();
-	expect_pending_htlcs_forwardable!(nodes[1]);
+	expect_and_process_pending_htlcs(&nodes[1], false);
 
 	let payment_event = {
 		{
@@ -769,7 +764,7 @@ fn do_blinded_intercept_payment(intercept_node_fails: bool) {
 	};
 	nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), &payment_event.msgs[0]);
 	commitment_signed_dance!(nodes[2], nodes[1], &payment_event.commitment_msg, false, true);
-	expect_pending_htlcs_forwardable!(nodes[2]);
+	expect_and_process_pending_htlcs(&nodes[2], false);
 
 	expect_payment_claimable!(&nodes[2], payment_hash, payment_secret, amt_msat, None, nodes[2].node.get_our_node_id());
 	do_claim_payment_along_route(
@@ -958,7 +953,7 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
 	nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event_0_1.msgs[0]);
 	check_added_monitors!(nodes[1], 0);
 	do_commitment_signed_dance(&nodes[1], &nodes[0], &payment_event_0_1.commitment_msg, false, false);
-	expect_pending_htlcs_forwardable!(nodes[1]);
+	expect_and_process_pending_htlcs(&nodes[1], false);
 	check_added_monitors!(&nodes[1], 1);
 
 	let mut payment_event_1_2 = {
@@ -973,7 +968,7 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
 			nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), &payment_event_1_2.msgs[0]);
 			check_added_monitors!(nodes[2], 0);
 			do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
-			expect_pending_htlcs_forwardable!(nodes[2]);
+			expect_and_process_pending_htlcs(&nodes[2], false);
 			check_payment_claimable(
 				&nodes[2].node.get_and_clear_pending_events()[0], payment_hash, payment_secret, amt_msat,
 				None, nodes[2].node.get_our_node_id()
@@ -1009,7 +1004,7 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
 			nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), update_add);
 			check_added_monitors!(nodes[2], 0);
 			do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
-			expect_pending_htlcs_forwardable!(nodes[2]);
+			expect_and_process_pending_htlcs(&nodes[2], false);
 			expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCHandlingFailureType::InvalidOnion]);
 			check_added_monitors(&nodes[2], 1);
 		},
@@ -1019,7 +1014,7 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
 			nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), update_add);
 			check_added_monitors!(nodes[2], 0);
 			do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
-			expect_pending_htlcs_forwardable!(nodes[2]);
+			expect_and_process_pending_htlcs(&nodes[2], false);
 			expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCHandlingFailureType::Receive { payment_hash }]);
 			check_added_monitors(&nodes[2], 1);
 		},
@@ -1035,7 +1030,7 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
 
 			nodes[2].node.handle_shutdown(nodes[1].node.get_our_node_id(), &node_1_shutdown);
 			commitment_signed_dance!(nodes[2], nodes[1], (), false, true, false, false);
-			expect_pending_htlcs_forwardable!(nodes[2]);
+			expect_and_process_pending_htlcs(&nodes[2], false);
 			expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCHandlingFailureType::Receive { payment_hash }]);
 			check_added_monitors(&nodes[2], 1);
 		},
@@ -1044,16 +1039,15 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
 			nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), &payment_event_1_2.msgs[0]);
 			check_added_monitors!(nodes[2], 0);
 			do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
-			expect_pending_htlcs_forwardable!(nodes[2]);
-			expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(nodes[2],
-				[HTLCHandlingFailureType::Receive { payment_hash }]);
+			expect_and_process_pending_htlcs(&nodes[2], true);
+			expect_pending_htlcs_forwardable_conditions(nodes[2].node.get_and_clear_pending_events(), &[HTLCHandlingFailureType::Receive { payment_hash }]);
 			check_added_monitors!(nodes[2], 1);
 		},
 		ReceiveCheckFail::PaymentConstraints => {
 			nodes[2].node.handle_update_add_htlc(nodes[1].node.get_our_node_id(), &payment_event_1_2.msgs[0]);
 			check_added_monitors!(nodes[2], 0);
 			do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
-			expect_pending_htlcs_forwardable!(nodes[2]);
+			expect_and_process_pending_htlcs(&nodes[2], false);
 			expect_htlc_handling_failed_destinations!(nodes[2].node.get_and_clear_pending_events(), &[HTLCHandlingFailureType::Receive { payment_hash }]);
 			check_added_monitors(&nodes[2], 1);
 		}
@@ -1162,18 +1156,14 @@ fn blinded_path_retries() {
 			do_commitment_signed_dance(&nodes[0], &$intro_node, &updates.commitment_signed, false, false);
 
 			let mut events = nodes[0].node.get_and_clear_pending_events();
-			assert_eq!(events.len(), 2);
+			assert_eq!(events.len(), 1);
 			match events[0] {
 				Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently, ..  } => {
 					assert_eq!(payment_hash, ev_payment_hash);
 					assert_eq!(payment_failed_permanently, false);
 				},
 				_ => panic!("Unexpected event"),
 			}
-			match events[1] {
-				Event::PendingHTLCsForwardable { .. } => {},
-				_ => panic!("Unexpected event"),
-			}
 			nodes[0].node.process_pending_htlc_forwards();
 		}
 	}
@@ -1262,7 +1252,7 @@ fn min_htlc() {
 	nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event_0_1.msgs[0]);
 	check_added_monitors!(nodes[1], 0);
 	do_commitment_signed_dance(&nodes[1], &nodes[0], &payment_event_0_1.commitment_msg, true, true);
-	expect_pending_htlcs_forwardable!(nodes[1]);
+	expect_and_process_pending_htlcs(&nodes[1], false);
 	expect_htlc_handling_failed_destinations!(
 		nodes[1].node.get_and_clear_pending_events(),
 		&[HTLCHandlingFailureType::Forward { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_1_2.2 }]
@@ -1455,7 +1445,7 @@ fn fails_receive_tlvs_authentication() {
 
 	nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
 	do_commitment_signed_dance(&nodes[1], &nodes[0], &payment_event.commitment_msg, true, true);
-	expect_pending_htlcs_forwardable!(nodes[1]);
+	expect_and_process_pending_htlcs(&nodes[1], false);
 	nodes[1].node.process_pending_htlc_forwards();
 	check_added_monitors!(nodes[1], 1);
 	expect_htlc_handling_failed_destinations!(nodes[1].node.get_and_clear_pending_events(), &[HTLCHandlingFailureType::InvalidOnion]);