Handle new event processing logic when enqueuing forward event

This was a regression resulting from f2453b7 since we now process events
in a loop until there aren't any left. Processing events is done in
batches and they are not removed until we're done processing each batch.
Since handling a `PendingHTLCsForwardable` event will call back into the
`ChannelManager`, we'll still see the original forwarding event not
removed. Phantom payments will need an additional forwarding event
before being claimed to make them look real by taking more time.

Author: wpaulino

lightning-invoice/src/utils.rs

@@ -792,12 +792,13 @@ fn prefer_current_channel(min_inbound_capacity_msat: Option<u64>, current_channe
 
 #[cfg(test)]
 mod test {
+	use core::cell::RefCell;
 	use core::time::Duration;
 	use crate::{Currency, Description, InvoiceDescription, SignOrCreationError, CreationError};
 	use bitcoin_hashes::{Hash, sha256};
 	use bitcoin_hashes::sha256::Hash as Sha256;
 	use lightning::sign::PhantomKeysManager;
-	use lightning::events::{MessageSendEvent, MessageSendEventsProvider, Event};
+	use lightning::events::{MessageSendEvent, MessageSendEventsProvider, Event, EventsProvider};
 	use lightning::ln::{PaymentPreimage, PaymentHash};
 	use lightning::ln::channelmanager::{PhantomRouteHints, MIN_FINAL_CLTV_EXPIRY_DELTA, PaymentId, RecipientOnionFields, Retry};
 	use lightning::ln::functional_test_utils::*;
@@ -1357,13 +1358,20 @@ mod test {
 		// Note that we have to "forward pending HTLCs" twice before we see the PaymentClaimable as
 		// this "emulates" the payment taking two hops, providing some privacy to make phantom node
 		// payments "look real" by taking more time.
-		expect_pending_htlcs_forwardable_ignore!(nodes[fwd_idx]);
-		nodes[fwd_idx].node.process_pending_htlc_forwards();
-		expect_pending_htlcs_forwardable_ignore!(nodes[fwd_idx]);
-		nodes[fwd_idx].node.process_pending_htlc_forwards();
+		let other_events = RefCell::new(Vec::new());
+		let forward_event_handler = |event: Event| {
+			if let Event::PendingHTLCsForwardable { .. } = event {
+				nodes[fwd_idx].node.process_pending_htlc_forwards();
+			} else {
+				other_events.borrow_mut().push(event);
+			}
+		};
+		nodes[fwd_idx].node.process_pending_events(&forward_event_handler);
+		nodes[fwd_idx].node.process_pending_events(&forward_event_handler);
 
 		let payment_preimage_opt = if user_generated_pmt_hash { None } else { Some(payment_preimage) };
-		expect_payment_claimable!(&nodes[fwd_idx], payment_hash, payment_secret, payment_amt, payment_preimage_opt, invoice.recover_payee_pub_key());
+		assert_eq!(other_events.borrow().len(), 1);
+		check_payment_claimable(&other_events.borrow()[0], payment_hash, payment_secret, payment_amt, payment_preimage_opt, invoice.recover_payee_pub_key());
 		do_claim_payment_along_route(&nodes[0], &[&vec!(&nodes[fwd_idx])[..]], false, payment_preimage);
 		let events = nodes[0].node.get_and_clear_pending_events();
 		assert_eq!(events.len(), 2);

lightning/src/ln/channelmanager.rs

@@ -1978,6 +1978,8 @@ macro_rules! process_events_body {
 				let mut pending_events = $self.pending_events.lock().unwrap();
 				pending_events.drain(..num_events);
 				processed_all_events = pending_events.is_empty();
+				// Note that `push_pending_forwards_ev` relies on `pending_events_processor` being
+				// updated here with the `pending_events` lock acquired.
 				$self.pending_events_processor.store(false, Ordering::Release);
 			}
 
@@ -5613,8 +5615,8 @@ where
 				for (forward_info, prev_htlc_id) in pending_forwards.drain(..) {
 					let scid = match forward_info.routing {
 						PendingHTLCRouting::Forward { short_channel_id, .. } => short_channel_id,
-						PendingHTLCRouting::Receive { .. } => 0,
-						PendingHTLCRouting::ReceiveKeysend { .. } => 0,
+						PendingHTLCRouting::Receive { .. } => { 0 },
+						PendingHTLCRouting::ReceiveKeysend { .. } => { 0 },
 					};
 					// Pull this now to avoid introducing a lock order with `forward_htlcs`.
 					let is_our_scid = self.short_to_chan_info.read().unwrap().contains_key(&scid);
@@ -5686,16 +5688,21 @@ where
 		}
 	}
 
-	// We only want to push a PendingHTLCsForwardable event if no others are queued.
 	fn push_pending_forwards_ev(&self) {
 		let mut pending_events = self.pending_events.lock().unwrap();
-		let forward_ev_exists = pending_events.iter()
-			.find(|(ev, _)| if let events::Event::PendingHTLCsForwardable { .. } = ev { true } else { false })
-			.is_some();
-		if !forward_ev_exists {
-			pending_events.push_back((events::Event::PendingHTLCsForwardable {
-				time_forwardable:
-					Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS),
+		let is_processing_events = self.pending_events_processor.load(Ordering::Acquire);
+		let num_forward_events = pending_events.iter().filter(|(ev, _)|
+			if let events::Event::PendingHTLCsForwardable { .. } = ev { true } else { false }
+		).count();
+		// We only want to push a PendingHTLCsForwardable event if no others are queued. Processing
+		// events is done in batches and they are not removed until we're done processing each
+		// batch. Since handling a `PendingHTLCsForwardable` event will call back into the
+		// `ChannelManager`, we'll still see the original forwarding event not removed. Phantom
+		// payments will need an additional forwarding event before being claimed to make them look
+		// real by taking more time.
+		if (is_processing_events && num_forward_events <= 1) || num_forward_events < 1 {
+			pending_events.push_back((Event::PendingHTLCsForwardable {
+				time_forwardable: Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS),
 			}, None));
 		}
 	}

lightning/src/ln/functional_test_utils.rs

@@ -1806,6 +1806,28 @@ macro_rules! get_route_and_payment_hash {
 	}}
 }
 
+pub fn check_payment_claimable(
+	event: &Event, expected_payment_hash: PaymentHash, expected_payment_secret: PaymentSecret,
+	expected_recv_value: u64, expected_payment_preimage: Option<PaymentPreimage>,
+	expected_receiver_node_id: PublicKey,
+) {
+	match event {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, .. } => {
+			assert_eq!(expected_payment_hash, *payment_hash);
+			assert_eq!(expected_recv_value, *amount_msat);
+			assert_eq!(expected_receiver_node_id, receiver_node_id.unwrap());
+			match purpose {
+				PaymentPurpose::InvoicePayment { payment_preimage, payment_secret, .. } => {
+					assert_eq!(&expected_payment_preimage, payment_preimage);
+					assert_eq!(expected_payment_secret, *payment_secret);
+				},
+				_ => {},
+			}
+		},
+		_ => panic!("Unexpected event"),
+	}
+}
+
 #[macro_export]
 #[cfg(any(test, ldk_bench, feature = "_test_utils"))]
 macro_rules! expect_payment_claimable {
@@ -1815,22 +1837,8 @@ macro_rules! expect_payment_claimable {
 	($node: expr, $expected_payment_hash: expr, $expected_payment_secret: expr, $expected_recv_value: expr, $expected_payment_preimage: expr, $expected_receiver_node_id: expr) => {
 		let events = $node.node.get_and_clear_pending_events();
 		assert_eq!(events.len(), 1);
-		match events[0] {
-			$crate::events::Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, .. } => {
-				assert_eq!($expected_payment_hash, *payment_hash);
-				assert_eq!($expected_recv_value, amount_msat);
-				assert_eq!($expected_receiver_node_id, receiver_node_id.unwrap());
-				match purpose {
-					$crate::events::PaymentPurpose::InvoicePayment { payment_preimage, payment_secret, .. } => {
-						assert_eq!(&$expected_payment_preimage, payment_preimage);
-						assert_eq!($expected_payment_secret, *payment_secret);
-					},
-					_ => {},
-				}
-			},
-			_ => panic!("Unexpected event"),
-		}
-	}
+		$crate::ln::functional_test_utils::check_payment_claimable(&events[0], $expected_payment_hash, $expected_payment_secret, $expected_recv_value, $expected_payment_preimage, $expected_receiver_node_id)
+	};
 }
 
 #[macro_export]