From c453d04137997f8ca2f79ae123b4915062e2f369 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 3 Dec 2021 13:04:58 -0600 Subject: [PATCH] Ensure ChannelManager methods are idempotent 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 | 14 ++++++++----- lightning/src/ln/functional_test_utils.rs | 25 ++++++++++++++++++++++- lightning/src/util/events.rs | 13 ++++++++---- 3 files changed, 42 insertions(+), 10 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 0eaf244d..f9f7b044 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2443,7 +2443,8 @@ impl 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 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 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()); diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 01f9db53..c724f6ca 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -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) { diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 46ab9f74..959b180d 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -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>, }, - /// 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, -- 2.30.2