Merge pull request #1201 from jkczyz/2021-12-idempotent-channelmanager

Ensure ChannelManager methods are idempotent

Author: TheBlueMatt

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

@@ -531,7 +531,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());
@@ -559,6 +559,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
 }
 
@@ -1067,6 +1072,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();
 	}}
 }
 
@@ -1080,6 +1088,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();
 		}
 	}}
 }
@@ -1407,6 +1418,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) {
@@ -1551,6 +1568,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,