Add a new `ChannelMoniorUpdateStep::ReleasePaymentComplete`
What the developer wrote
Authored by Matt Corallo
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.
What changed, and why it matters
This commit adds a new internal bookkeeping step in Lightning Dev Kit's channel monitor so that, after a channel is closed and an HTLC is resolved on-chain, the code can remember that the user has already been told the payment finished. The goal is to avoid repeatedly replaying the same 'your payment completed/failed' message every time the node restarts, especially in setups where monitor events might be lost between a channel monitor persist and a channel manager persist. It is a defensive correctness fix, not a remote exploit.
Treat as a defensive fix in a larger patch series. Review the follow-up commit(s) that actually generate ReleasePaymentComplete updates from ChannelManager to ensure the new step is emitted at the correct point and that race conditions between user event handling and monitor persistence are handled. Verify that htlcs_resolved_to_user is consulted when populating get_all_current_outbound_htlcs and get_onchain_failed_outbound_htlcs in subsequent patches.
Security signals we found
Durability gap between ChannelMonitor persist and ChannelManager persist can lose MonitorEvent effects
Lost MonitorEvent for closed-channel HTLC resolution could prevent payment resolution events from reaching user after restart
New ChannelMonitorUpdateStep ReleasePaymentComplete added to track user-resolved payments
New persisted htlcs_resolved_to_user set marks HTLCs whose resolution has been communicated to user
Serialization TLV 33 added for new field; backward-compatible option used during read
Update classified as post-close update alongside PaymentPreimage and ChannelForceClosed
Evidence from the diff
The patch introduces ChannelMonitorUpdateStep::ReleasePaymentComplete and a new htlcs_resolved_to_user HashSet
Changed components
lightning/src/chain/channelmonitor.rsChannelMonitorUpdateStep enumChannelMonitorImpl structChannelMonitor serialization/deserializationChannelMonitor update application logicInspect captured patch +40 / −3
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index f22850e..e1a0f40 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/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,
Why this scored 43/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.