Handle new event processing logic when enqueuing forward event
authorWilmer Paulino <wilmer@wilmerpaulino.com>
Wed, 5 Jul 2023 20:29:12 +0000 (13:29 -0700)
committerWilmer Paulino <wilmer@wilmerpaulino.com>
Mon, 10 Jul 2023 16:49:31 +0000 (09:49 -0700)
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.

lightning-invoice/src/utils.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_test_utils.rs

index c052706975968105c95b7dbf08c4ec7132b6c6c5..199aad064694c975d6e97cc132e3b460a84c1b71 100644 (file)
@@ -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);
index 65f442f1c097ea80018e33f9c56d06f1f61cc072..2cbda06684915a882ee74475368bbd51c20d59bd 100644 (file)
@@ -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);
                        }
 
@@ -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));
                }
        }
index 64bd679a94f89cdf32402a2681166028fce44ca7..f2c783fa67fc08584f1c0d0540a58c7796329907 100644 (file)
@@ -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]