Add a new ChannelMoniorUpdateStep::ReleasePaymentComplete

`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 take the first step towards that notification, adding a new
`ChannelMonitorUpdateStep` for the completion notification, and
tracking HTLCs which make it to the `ChannelMonitor` in such
updates in a new map.

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -693,6 +693,20 @@ pub(crate) enum ChannelMonitorUpdateStep {
 	RenegotiatedFundingLocked {
 		funding_txid: Txid,
 	},
+	/// When a payment is finally resolved by the user handling an [`Event::PaymentSent`] or
+	/// [`Event::PaymentFailed`] event, the `ChannelManager` no longer needs to hear about it on
+	/// startup (which would cause it to re-hydrate the payment information even though the user
+	/// already learned about the payment's result).
+	///
+	/// This will remove the HTLC from [`ChannelMonitor::get_all_current_outbound_htlcs`] and
+	/// [`ChannelMonitor::get_onchain_failed_outbound_htlcs`].
+	///
+	/// Note that this is only generated for closed channels as this is implicit in the
+	/// [`Self::CommitmentSecret`] update which clears the payment information from all un-revoked
+	/// counterparty commitment transactions.
+	ReleasePaymentComplete {
+		htlc: SentHTLCId,
+	},
 }
 
 impl ChannelMonitorUpdateStep {
@@ -709,6 +723,7 @@ impl ChannelMonitorUpdateStep {
 			ChannelMonitorUpdateStep::ShutdownScript { .. } => "ShutdownScript",
 			ChannelMonitorUpdateStep::RenegotiatedFunding { .. } => "RenegotiatedFunding",
 			ChannelMonitorUpdateStep::RenegotiatedFundingLocked { .. } => "RenegotiatedFundingLocked",
+			ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => "ReleasePaymentComplete",
 		}
 	}
 }
@@ -747,6 +762,9 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep,
 		(1, commitment_txs, required_vec),
 		(3, htlc_data, required),
 	},
+	(7, ReleasePaymentComplete) => {
+		(1, htlc, required),
+	},
 	(8, LatestHolderCommitment) => {
 		(1, commitment_txs, required_vec),
 		(3, htlc_data, required),
@@ -1250,6 +1268,14 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
 	/// spending CSV for revocable outputs).
 	htlcs_resolved_on_chain: Vec<IrrevocablyResolvedHTLC>,
 
+	/// When a payment is resolved through an on-chain transaction, we tell the `ChannelManager`
+	/// about this via [`ChannelMonitor::get_onchain_failed_outbound_htlcs`] and
+	/// [`ChannelMonitor::get_all_current_outbound_htlcs`] at startup. We'll keep repeating the
+	/// same payments until they're eventually fully resolved by the user processing a
+	/// `PaymentSent` or `PaymentFailed` event, at which point the `ChannelManager` will inform of
+	/// this and we'll store the set of fully resolved payments here.
+	htlcs_resolved_to_user: HashSet<SentHTLCId>,
+
 	/// The set of `SpendableOutput` events which we have already passed upstream to be claimed.
 	/// These are tracked explicitly to ensure that we don't generate the same events redundantly
 	/// if users duplicatively confirm old transactions. Specifically for transactions claiming a
@@ -1654,6 +1680,7 @@ pub(crate) fn write_chanmon_internal<Signer: EcdsaChannelSigner, W: Writer>(
 		(29, channel_monitor.initial_counterparty_commitment_tx, option),
 		(31, channel_monitor.funding.channel_parameters, required),
 		(32, channel_monitor.pending_funding, optional_vec),
+		(33, channel_monitor.htlcs_resolved_to_user, required),
 		(34, channel_monitor.alternative_funding_confirmed, option),
 	});
 
@@ -1872,6 +1899,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 			funding_spend_confirmed: None,
 			confirmed_commitment_tx_counterparty_output: None,
 			htlcs_resolved_on_chain: Vec::new(),
+			htlcs_resolved_to_user: new_hash_set(),
 			spendable_txids_confirmed: Vec::new(),
 
 			best_block,
@@ -4218,6 +4246,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 						panic!("Attempted to replace shutdown script {} with {}", shutdown_script, scriptpubkey);
 					}
 				},
+				ChannelMonitorUpdateStep::ReleasePaymentComplete { htlc } => {
+					log_trace!(logger, "HTLC {htlc:?} permanently and fully resolved");
+					self.htlcs_resolved_to_user.insert(*htlc);
+				},
 			}
 		}
 
@@ -4243,11 +4275,13 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					|ChannelMonitorUpdateStep::RenegotiatedFundingLocked { .. } =>
 						is_pre_close_update = true,
 				// After a channel is closed, we don't communicate with our peer about it, so the
-				// only things we will update is getting a new preimage (from a different channel)
-				// or being told that the channel is closed. All other updates are generated while
-				// talking to our peer.
+				// only things we will update is getting a new preimage (from a different channel),
+				// being told that the channel is closed, or being told a payment which was
+				// resolved on-chain has had its resolution communicated to the user. All other
+				// updates are generated while talking to our peer.
 				ChannelMonitorUpdateStep::PaymentPreimage { .. } => {},
 				ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {},
+				ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => {},
 			}
 		}
 
@@ -6324,6 +6358,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 
 		let mut funding_spend_confirmed = None;
 		let mut htlcs_resolved_on_chain = Some(Vec::new());
+		let mut htlcs_resolved_to_user = Some(new_hash_set());
 		let mut funding_spend_seen = Some(false);
 		let mut counterparty_node_id = None;
 		let mut confirmed_commitment_tx_counterparty_output = None;
@@ -6357,6 +6392,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 			(29, initial_counterparty_commitment_tx, option),
 			(31, channel_parameters, (option: ReadableArgs, None)),
 			(32, pending_funding, optional_vec),
+			(33, htlcs_resolved_to_user, option),
 			(34, alternative_funding_confirmed, option),
 		});
 		if let Some(payment_preimages_with_info) = payment_preimages_with_info {
@@ -6516,6 +6552,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 			funding_spend_confirmed,
 			confirmed_commitment_tx_counterparty_output,
 			htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(),
+			htlcs_resolved_to_user: htlcs_resolved_to_user.unwrap(),
 			spendable_txids_confirmed: spendable_txids_confirmed.unwrap(),
 
 			best_block,