Ensure ChannelManager methods are idempotent
authorJeffrey Czyz <jkczyz@gmail.com>
Fri, 3 Dec 2021 19:04:58 +0000 (13:04 -0600)
committerJeffrey Czyz <jkczyz@gmail.com>
Mon, 6 Dec 2021 23:18:33 +0000 (17:18 -0600)
During event handling, ChannelManager methods may need to be called as
indicated in the Event documentation. Ensure that these calls are
idempotent for the same event rather than panicking. This allows users
to persist events for later handling without needing to worry about
processing the same event twice (e.g., if ChannelManager is not
persisted but the events were, the restarted ChannelManager would return
some of the same events).

lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_test_utils.rs
lightning/src/util/events.rs

index 0eaf244d0e47653cc04b2cd0c508f519820f5c78..f9f7b04402202a3aa9a45f8036b9a48fd58ec631 100644 (file)
@@ -2443,7 +2443,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// Returns an [`APIError::APIMisuseError`] if the funding_transaction spent non-SegWit outputs
        /// or if no output was found which matches the parameters in [`Event::FundingGenerationReady`].
        ///
-       /// Panics if a funding transaction has already been provided for this channel.
+       /// Returns [`APIError::ChannelUnavailable`] if a funding transaction has already been provided
+       /// for the channel or if the channel has been closed as indicated by [`Event::ChannelClosed`].
        ///
        /// May panic if the output found in the funding transaction is duplicative with some other
        /// channel (note that this should be trivially prevented by using unique funding transaction
@@ -2458,6 +2459,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// create a new channel with a conflicting funding transaction.
        ///
        /// [`Event::FundingGenerationReady`]: crate::util::events::Event::FundingGenerationReady
+       /// [`Event::ChannelClosed`]: crate::util::events::Event::ChannelClosed
        pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_transaction: Transaction) -> Result<(), APIError> {
                let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
 
@@ -3353,19 +3355,21 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                }
        }
 
-       /// Provides a payment preimage in response to a PaymentReceived event, returning true and
-       /// generating message events for the net layer to claim the payment, if possible. Thus, you
-       /// should probably kick the net layer to go send messages if this returns true!
+       /// Provides a payment preimage in response to [`Event::PaymentReceived`], generating any
+       /// [`MessageSendEvent`]s needed to claim the payment.
        ///
        /// Note that if you did not set an `amount_msat` when calling [`create_inbound_payment`] or
        /// [`create_inbound_payment_for_hash`] you must check that the amount in the `PaymentReceived`
        /// event matches your expectation. If you fail to do so and call this method, you may provide
        /// the sender "proof-of-payment" when they did not fulfill the full expected payment.
        ///
-       /// May panic if called except in response to a PaymentReceived event.
+       /// Returns whether any HTLCs were claimed, and thus if any new [`MessageSendEvent`]s are now
+       /// pending for processing via [`get_and_clear_pending_msg_events`].
        ///
+       /// [`Event::PaymentReceived`]: crate::util::events::Event::PaymentReceived
        /// [`create_inbound_payment`]: Self::create_inbound_payment
        /// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash
+       /// [`get_and_clear_pending_msg_events`]: MessageSendEventsProvider::get_and_clear_pending_msg_events
        pub fn claim_funds(&self, payment_preimage: PaymentPreimage) -> bool {
                let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
 
index 01f9db53439ca5558cd6f25c2e7a4077e167f7a2..c724f6ca6c58f2474c01c1a318611a0612bd6897 100644 (file)
@@ -530,7 +530,7 @@ pub fn sign_funding_transaction<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, node_b: &
        let (temporary_channel_id, tx, funding_output) = create_funding_transaction(node_a, channel_value, 42);
        assert_eq!(temporary_channel_id, expected_temporary_channel_id);
 
-       node_a.node.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap();
+       assert!(node_a.node.funding_transaction_generated(&temporary_channel_id, tx.clone()).is_ok());
        check_added_monitors!(node_a, 0);
 
        let funding_created_msg = get_event_msg!(node_a, MessageSendEvent::SendFundingCreated, node_b.node.get_our_node_id());
@@ -558,6 +558,11 @@ pub fn sign_funding_transaction<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, node_b: &
        assert_eq!(node_a.tx_broadcaster.txn_broadcasted.lock().unwrap()[0], tx);
        node_a.tx_broadcaster.txn_broadcasted.lock().unwrap().clear();
 
+       // Ensure that funding_transaction_generated is idempotent.
+       assert!(node_a.node.funding_transaction_generated(&temporary_channel_id, tx.clone()).is_err());
+       assert!(node_a.node.get_and_clear_pending_msg_events().is_empty());
+       check_added_monitors!(node_a, 0);
+
        tx
 }
 
@@ -1057,6 +1062,9 @@ macro_rules! expect_pending_htlcs_forwardable {
        ($node: expr) => {{
                expect_pending_htlcs_forwardable_ignore!($node);
                $node.node.process_pending_htlc_forwards();
+
+               // Ensure process_pending_htlc_forwards is idempotent.
+               $node.node.process_pending_htlc_forwards();
        }}
 }
 
@@ -1070,6 +1078,9 @@ macro_rules! expect_pending_htlcs_forwardable_from_events {
                };
                if $ignore {
                        $node.node.process_pending_htlc_forwards();
+
+                       // Ensure process_pending_htlc_forwards is idempotent.
+                       $node.node.process_pending_htlc_forwards();
                }
        }}
 }
@@ -1392,6 +1403,12 @@ pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>,
                        last_update_fulfill_dance!(origin_node, expected_route.first().unwrap());
                }
        }
+
+       // Ensure that claim_funds is idempotent.
+       assert!(!expected_paths[0].last().unwrap().node.claim_funds(our_payment_preimage));
+       assert!(expected_paths[0].last().unwrap().node.get_and_clear_pending_msg_events().is_empty());
+       check_added_monitors!(expected_paths[0].last().unwrap(), 0);
+
        expected_total_fee_msat
 }
 pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_preimage: PaymentPreimage) {
@@ -1536,6 +1553,12 @@ pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe
                        }
                }
        }
+
+       // Ensure that fail_htlc_backwards is idempotent.
+       assert!(!expected_paths[0].last().unwrap().node.fail_htlc_backwards(&our_payment_hash));
+       assert!(expected_paths[0].last().unwrap().node.get_and_clear_pending_events().is_empty());
+       assert!(expected_paths[0].last().unwrap().node.get_and_clear_pending_msg_events().is_empty());
+       check_added_monitors!(expected_paths[0].last().unwrap(), 0);
 }
 
 pub fn fail_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &[&Node<'a, 'b, 'c>], our_payment_hash: PaymentHash)  {
index 46ab9f74924e453b5446c27a783616cab0a0ff5d..959b180d5101330f18edfe631d29315cf1660f24 100644 (file)
@@ -153,10 +153,13 @@ impl_writeable_tlv_based_enum_upgradable!(ClosureReason,
 #[derive(Clone, Debug)]
 pub enum Event {
        /// Used to indicate that the client should generate a funding transaction with the given
-       /// parameters and then call ChannelManager::funding_transaction_generated.
-       /// Generated in ChannelManager message handling.
+       /// parameters and then call [`ChannelManager::funding_transaction_generated`].
+       /// Generated in [`ChannelManager`] message handling.
        /// Note that *all inputs* in the funding transaction must spend SegWit outputs or your
        /// counterparty can steal your funds!
+       ///
+       /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
+       /// [`ChannelManager::funding_transaction_generated`]: crate::ln::channelmanager::ChannelManager::funding_transaction_generated
        FundingGenerationReady {
                /// The random channel_id we picked which you'll need to pass into
                /// ChannelManager::funding_transaction_generated.
@@ -271,8 +274,10 @@ pub enum Event {
 #[cfg(test)]
                error_data: Option<Vec<u8>>,
        },
-       /// Used to indicate that ChannelManager::process_pending_htlc_forwards should be called at a
-       /// time in the future.
+       /// Used to indicate that [`ChannelManager::process_pending_htlc_forwards`] should be called at
+       /// a time in the future.
+       ///
+       /// [`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,