Stop re-hydrating pending payments once they are fully resolved

`MonitorEvent`s aren't delivered to the `ChannelManager` in a
durable fasion - if the `ChannelManager` fetches the pending
`MonitorEvent`s, then the `ChannelMonitor` gets persisted (i.e. due
to a block update) then the node crashes, prior to persisting the
`ChannelManager` again, the `MonitorEvent` and its effects on the
`ChannelManger` will be lost. This isn't likely in a sync persist
environment, but in an async one this could be an issue.

Note that this is only an issue for closed channels -
`MonitorEvent`s only inform the `ChannelManager` that a channel is
closed (which the `ChannelManager` will learn on startup or when it
next tries to advance the channel state), that
`ChannelMonitorUpdate` writes completed (which the `ChannelManager`
will detect on startup), or that HTLCs resolved on-chain post
closure. Of the three, only the last is problematic to lose prior
to a reload.

In previous commits we ensured that HTLC resolutions which came to
`ChannelManager` via a `MonitorEvent` were replayed on startup if
the `MonitorEvent` was lost. However, in cases where the
`ChannelManager` was so stale that it didn't have the payment state
for an HTLC at all, we only re-add it in cases where
`ChannelMonitor::get_pending_or_resolved_outbound_htlcs` includes
it.

Because constantly re-adding a payment state and then failing it
would generate lots of noise for users on startup (not to mention
risk of confusing stale payment events for the latest state of a
payment when the `PaymentId` has been reused to retry a payment).
Thus, `get_pending_or_resolved_outbound_htlcs` does not include
state for HTLCs which were resolved on chain with a preimage or
HTLCs which were resolved on chain with a timeout after
`ANTI_REORG_DELAY` confirmations.

This critera matches the critera for generating a `MonitorEvent`,
and works great under the assumption that `MonitorEvent`s are
reliably delivered. However, if they are not, and our
`ChannelManager` is lost or substantially old (or, in a future
where we do not persist `ChannelManager` at all), we will not end
up seeing payment resolution events for an HTLC.

Instead, we really want to tell our `ChannelMonitor`s when the
resolution of an HTLC is complete. Note that we don't particularly
care about non-payment HTLCs, as there is no re-hydration of state
to do there - `ChannelManager` load ignores forwarded HTLCs coming
back from `get_pending_or_resolved_outbound_htlcs` as there's
nothing to do - we always attempt to replay the success/failure and
figure out if it mattered based on whether there was still an HTLC
to claim/fail.

Here we, finally, begin actually using the new
`ChannelMonitorUpdateStep::ReleasePaymentComplete` updates,
skipping re-hydration of pending payments once they have been fully
resolved through to a user `Event`.

Backport of 226520b

Fixed conflicts in:
 * lightning/src/chain/channelmonitor.rs
 * lightning/src/ln/channelmanager.rs
 * lightning/src/ln/functional_tests.rs to address differences
   causing upstream to (somewhat spuriously) generate
   `ChannelMonitorUpdate`s for channels force-closed by commitment
   transaction confirmation whereas this branch does not,
 * lightning/src/ln/payment_tests.rs

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -2577,10 +2577,6 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 	/// Gets the set of outbound HTLCs which can be (or have been) resolved by this
 	/// `ChannelMonitor`. This is used to determine if an HTLC was removed from the channel prior
 	/// to the `ChannelManager` having been persisted.
-	///
-	/// This is similar to [`Self::get_pending_or_resolved_outbound_htlcs`] except it includes
-	/// HTLCs which were resolved on-chain (i.e. where the final HTLC resolution was done by an
-	/// event from this `ChannelMonitor`).
 	pub(crate) fn get_all_current_outbound_htlcs(
 		&self,
 	) -> HashMap<HTLCSource, (HTLCOutputInCommitment, Option<PaymentPreimage>)> {
@@ -2593,8 +2589,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 				for &(ref htlc, ref source_option) in latest_outpoints.iter() {
 					if let &Some(ref source) = source_option {
 						let htlc_id = SentHTLCId::from_source(source);
-						let preimage_opt = us.counterparty_fulfilled_htlcs.get(&htlc_id).cloned();
-						res.insert((**source).clone(), (htlc.clone(), preimage_opt));
+						if !us.htlcs_resolved_to_user.contains(&htlc_id) {
+							let preimage_opt =
+								us.counterparty_fulfilled_htlcs.get(&htlc_id).cloned();
+							res.insert((**source).clone(), (htlc.clone(), preimage_opt));
+						}
 					}
 				}
 			}
@@ -2650,6 +2649,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 						} else {
 							continue;
 						};
+						let htlc_id = SentHTLCId::from_source(source);
+						if us.htlcs_resolved_to_user.contains(&htlc_id) {
+							continue;
+						}
+
 						let confirmed = $htlc_iter.find(|(_, conf_src)| Some(source) == *conf_src);
 						if let Some((confirmed_htlc, _)) = confirmed {
 							let filter = |v: &&IrrevocablyResolvedHTLC| {
@@ -2724,93 +2728,6 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 		res
 	}
 
-	/// Gets the set of outbound HTLCs which are pending resolution in this channel or which were
-	/// resolved with a preimage from our counterparty.
-	///
-	/// This is used to reconstruct pending outbound payments on restart in the ChannelManager.
-	///
-	/// Currently, the preimage is unused, however if it is present in the relevant internal state
-	/// an HTLC is always included even if it has been resolved.
-	pub(crate) fn get_pending_or_resolved_outbound_htlcs(&self) -> HashMap<HTLCSource, (HTLCOutputInCommitment, Option<PaymentPreimage>)> {
-		let us = self.inner.lock().unwrap();
-		// We're only concerned with the confirmation count of HTLC transactions, and don't
-		// actually care how many confirmations a commitment transaction may or may not have. Thus,
-		// we look for either a FundingSpendConfirmation event or a funding_spend_confirmed.
-		let confirmed_txid = us.funding_spend_confirmed.or_else(|| {
-			us.onchain_events_awaiting_threshold_conf.iter().find_map(|event| {
-				if let OnchainEvent::FundingSpendConfirmation { .. } = event.event {
-					Some(event.txid)
-				} else { None }
-			})
-		});
-
-		if confirmed_txid.is_none() {
-			// If we have not seen a commitment transaction on-chain (ie the channel is not yet
-			// closed), just get the full set.
-			mem::drop(us);
-			return self.get_all_current_outbound_htlcs();
-		}
-
-		let mut res = new_hash_map();
-		macro_rules! walk_htlcs {
-			($holder_commitment: expr, $htlc_iter: expr) => {
-				for (htlc, source) in $htlc_iter {
-					if us.htlcs_resolved_on_chain.iter().any(|v| v.commitment_tx_output_idx == htlc.transaction_output_index) {
-						// We should assert that funding_spend_confirmed is_some() here, but we
-						// have some unit tests which violate HTLC transaction CSVs entirely and
-						// would fail.
-						// TODO: Once tests all connect transactions at consensus-valid times, we
-						// should assert here like we do in `get_claimable_balances`.
-					} else if htlc.offered == $holder_commitment {
-						// If the payment was outbound, check if there's an HTLCUpdate
-						// indicating we have spent this HTLC with a timeout, claiming it back
-						// and awaiting confirmations on it.
-						let htlc_update_confd = us.onchain_events_awaiting_threshold_conf.iter().any(|event| {
-							if let OnchainEvent::HTLCUpdate { commitment_tx_output_idx: Some(commitment_tx_output_idx), .. } = event.event {
-								// If the HTLC was timed out, we wait for ANTI_REORG_DELAY blocks
-								// before considering it "no longer pending" - this matches when we
-								// provide the ChannelManager an HTLC failure event.
-								Some(commitment_tx_output_idx) == htlc.transaction_output_index &&
-									us.best_block.height >= event.height + ANTI_REORG_DELAY - 1
-							} else if let OnchainEvent::HTLCSpendConfirmation { commitment_tx_output_idx, .. } = event.event {
-								// If the HTLC was fulfilled with a preimage, we consider the HTLC
-								// immediately non-pending, matching when we provide ChannelManager
-								// the preimage.
-								Some(commitment_tx_output_idx) == htlc.transaction_output_index
-							} else { false }
-						});
-						let counterparty_resolved_preimage_opt =
-							us.counterparty_fulfilled_htlcs.get(&SentHTLCId::from_source(source)).cloned();
-						if !htlc_update_confd || counterparty_resolved_preimage_opt.is_some() {
-							res.insert(source.clone(), (htlc.clone(), counterparty_resolved_preimage_opt));
-						}
-					}
-				}
-			}
-		}
-
-		let txid = confirmed_txid.unwrap();
-		if Some(txid) == us.current_counterparty_commitment_txid || Some(txid) == us.prev_counterparty_commitment_txid {
-			walk_htlcs!(false, us.counterparty_claimable_outpoints.get(&txid).unwrap().iter().filter_map(|(a, b)| {
-				if let &Some(ref source) = b {
-					Some((a, &**source))
-				} else { None }
-			}));
-		} else if txid == us.current_holder_commitment_tx.txid {
-			walk_htlcs!(true, us.current_holder_commitment_tx.htlc_outputs.iter().filter_map(|(a, _, c)| {
-				if let Some(source) = c { Some((a, source)) } else { None }
-			}));
-		} else if let Some(prev_commitment) = &us.prev_holder_signed_commitment_tx {
-			if txid == prev_commitment.txid {
-				walk_htlcs!(true, prev_commitment.htlc_outputs.iter().filter_map(|(a, _, c)| {
-					if let Some(source) = c { Some((a, source)) } else { None }
-				}));
-			}
-		}
-
-		res
-	}
-
 	pub(crate) fn get_stored_preimages(&self) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> {
 		self.inner.lock().unwrap().payment_preimages.clone()
 	}

lightning/src/ln/channelmanager.rs

@@ -14064,7 +14064,7 @@ where
 				let counterparty_opt = outpoint_to_peer.get(&monitor.get_funding_txo().0);
 				// outpoint_to_peer missing the funding outpoint implies the channel is closed
 				if counterparty_opt.is_none() {
-					for (htlc_source, (htlc, _)) in monitor.get_pending_or_resolved_outbound_htlcs() {
+					for (htlc_source, (htlc, _)) in monitor.get_all_current_outbound_htlcs() {
 						let logger = WithChannelMonitor::from(&args.logger, monitor, Some(htlc.payment_hash));
 						if let HTLCSource::OutboundRoute { payment_id, session_priv, path, .. } = htlc_source {
 							if path.hops.is_empty() {

lightning/src/ln/functional_tests.rs

@@ -3711,6 +3711,8 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
 	let events = nodes[1].node.get_and_clear_pending_events();
 	if deliver_bs_raa {
 		check_added_monitors(&nodes[1], 1);
+	} else {
+		check_added_monitors(&nodes[1], 0);
 	}
 	assert_eq!(events.len(), if deliver_bs_raa { 3 + nodes.len() - 1 } else { 4 + nodes.len() });
 	assert!(events.iter().any(|ev| matches!(
@@ -3727,7 +3729,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
 	)));
 
 	nodes[1].node.process_pending_htlc_forwards();
-	check_added_monitors!(nodes[1], 1);
+	check_added_monitors(&nodes[1], 1);
 
 	let mut events = nodes[1].node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), if deliver_bs_raa { 4 } else { 3 });

lightning/src/ln/payment_tests.rs

@@ -973,9 +973,6 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) {
 	nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id());
 
 	nodes[0].node.test_process_background_events();
-	check_added_monitors(&nodes[0], 1); // TODO: Removed in the next commit as this only required
-									// when we are still seeing all payments, even resolved
-									// ones.
 
 	let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]);
 	reconnect_args.send_channel_ready = (true, true);
@@ -1005,9 +1002,6 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) {
 	nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id());
 
 	nodes[0].node.test_process_background_events();
-	check_added_monitors(&nodes[0], 1); // TODO: Removed in the next commit as this only required
-									// when we are still seeing all payments, even resolved
-									// ones.
 
 	reconnect_nodes(ReconnectArgs::new(&nodes[0], &nodes[1]));
 
@@ -1024,7 +1018,10 @@ fn test_completed_payment_not_retryable_on_reload() {
 	do_test_completed_payment_not_retryable_on_reload(false);
 }
 
-fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(persist_manager_post_event: bool, confirm_commitment_tx: bool, payment_timeout: bool) {
+fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(
+	persist_manager_post_event: bool, persist_monitor_after_events: bool,
+	confirm_commitment_tx: bool, payment_timeout: bool,
+) {
 	// When a Channel is closed, any outbound HTLCs which were relayed through it are simply
 	// dropped. From there, the ChannelManager relies on the ChannelMonitor having a copy of the
 	// relevant fail-/claim-back data and processes the HTLC fail/claim when the ChannelMonitor tells
@@ -1115,36 +1112,58 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(persist_manager_post_event: bo
 		chan_manager_serialized = nodes[0].node.encode();
 	}
 
-	let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode();
+	let mut mon_ser = Vec::new();
+	if !persist_monitor_after_events {
+		mon_ser = get_monitor!(nodes[0], chan_id).encode();
+	}
 	if payment_timeout {
 		let conditions = PaymentFailedConditions::new().from_mon_update();
 		expect_payment_failed_conditions(&nodes[0], payment_hash, false, conditions);
 	} else {
 		expect_payment_sent(&nodes[0], payment_preimage, None, true, true);
 	}
+	// Note that if we persist the monitor before processing the events, above, we'll always get
+	// them replayed on restart no matter what
+	if persist_monitor_after_events {
+		mon_ser = get_monitor!(nodes[0], chan_id).encode();
+	}
 
 	// If we persist the ChannelManager after we get the PaymentSent event, we shouldn't get it
 	// twice.
 	if persist_manager_post_event {
 		chan_manager_serialized = nodes[0].node.encode();
+	} else if persist_monitor_after_events {
+		// Persisting the monitor after the events (resulting in a new monitor being persisted) but
+		// didn't persist the manager will result in an FC, which we don't test here.
+		panic!();
 	}
 
 	// Now reload nodes[0]...
-	reload_node!(nodes[0], &chan_manager_serialized, &[&chan_0_monitor_serialized], persister, new_chain_monitor, nodes_0_deserialized);
+	reload_node!(nodes[0], &chan_manager_serialized, &[&mon_ser], persister, new_chain_monitor, nodes_0_deserialized);
 
 	check_added_monitors(&nodes[0], 0);
-	if persist_manager_post_event {
+	if persist_manager_post_event && persist_monitor_after_events {
 		assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
-		check_added_monitors(&nodes[0], 2);
+		check_added_monitors(&nodes[0], 0);
 	} else if payment_timeout {
-		let conditions = PaymentFailedConditions::new().from_mon_update();
+		let mut conditions = PaymentFailedConditions::new();
+		if !persist_monitor_after_events {
+			conditions = conditions.from_mon_update();
+		}
 		expect_payment_failed_conditions(&nodes[0], payment_hash, false, conditions);
+		check_added_monitors(&nodes[0], 0);
 	} else {
+		if persist_manager_post_event {
+			assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
+		} else {
+			expect_payment_sent(&nodes[0], payment_preimage, None, true, false);
+		}
 		// After reload, the ChannelManager identified the failed payment and queued up the
-		// PaymentSent and corresponding ChannelMonitorUpdate to mark the payment handled, but
-		// while processing the pending `MonitorEvent`s (which were not processed before the
-		// monitor was persisted) we will end up with a duplicate ChannelMonitorUpdate.
-		expect_payment_sent(&nodes[0], payment_preimage, None, true, false);
+		// PaymentSent (or not, if `persist_manager_post_event` resulted in us detecting we
+		// already did that) and corresponding ChannelMonitorUpdate to mark the payment
+		// handled, but while processing the pending `MonitorEvent`s (which were not processed
+		// before the monitor was persisted) we will end up with a duplicate
+		// ChannelMonitorUpdate.
 		check_added_monitors(&nodes[0], 2);
 	}
 
@@ -1158,12 +1177,15 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(persist_manager_post_event: bo
 
 #[test]
 fn test_dup_htlc_onchain_doesnt_fail_on_reload() {
-	do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, true);
-	do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, false);
-	do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, false);
-	do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, true, true);
-	do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, true, false);
-	do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, false);
+	do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, true, true);
+	do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, true, false);
+	do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, true, false, false);
+	do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, true, true);
+	do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, true, false);
+	do_test_dup_htlc_onchain_doesnt_fail_on_reload(true, false, false, false);
+	do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, true, true);
+	do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, true, false);
+	do_test_dup_htlc_onchain_doesnt_fail_on_reload(false, false, false, false);
 }
 
 #[test]
@@ -3513,15 +3535,9 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint:
 	let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode();
 	reload_node!(nodes[0], test_default_channel_config(), &nodes[0].node.encode(), &[&chan_0_monitor_serialized], persister_b, chain_monitor_b, nodes_0_deserialized_b);
 
-	// Because the pending payment will currently stick around forever, we'll apply a
-	// ChannelMonitorUpdate on each startup to attempt to remove it.
-	// TODO: This will be dropped in the next commit after we actually remove the payment!
-	check_added_monitors(&nodes[0], 0);
 	nodes[0].node.test_process_background_events();
-	check_added_monitors(&nodes[0], 1);
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert!(events.is_empty());
-	check_added_monitors(&nodes[0], 0);
 
 	// Ensure that we don't generate any further events even after the channel-closing commitment
 	// transaction is confirmed on-chain.
@@ -3536,9 +3552,6 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint:
 	reload_node!(nodes[0], test_default_channel_config(), &nodes[0].node.encode(), &[&chan_0_monitor_serialized], persister_c, chain_monitor_c, nodes_0_deserialized_c);
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert!(events.is_empty());
-
-	// TODO: This will be dropped in the next commit after we actually remove the payment!
-	check_added_monitors(&nodes[0], 2);
 }
 
 #[test]