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).

Author: jkczyz

lightning/src/ln/channelmanager.rs

@@ -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());
 

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)  {

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<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,