Prevent redundant PaymentClaimed events for closed channels#4467
Draft
valentinewallace wants to merge 5 commits intolightningdevkit:mainfrom
Draft
Prevent redundant PaymentClaimed events for closed channels#4467valentinewallace wants to merge 5 commits intolightningdevkit:mainfrom
PaymentClaimed events for closed channels#4467valentinewallace wants to merge 5 commits intolightningdevkit:mainfrom
Conversation
Useful in the next commit because we'll need to repurpose the same handling code for a new monitor update that is similar to ReleasePaymentComplete, but for inbound payments to avoid regenerating redundant PaymentClaimed events.
When an Event::PaymentClaimed is processed by the user, we need to track that so we don't keep regenerating the event redundantly on startup. Here we add a monitor update for that. Note that this update will only be generated for closed channels -- if the channel is open, the inbound payment is pruned automatically when the HTLC is no longer present in any unrevoked commitment transaction, which stops the redundant event regeneration. Upcoming commits will add generation and handling of this monitor update
When an Event::PaymentClaimed is processed by the user, we need to track that so we don't keep regenerating the event redundantly on startup. Here we add a map to claims in this state, similar to the existing htlcs_resolved_to_user map. Note that this map will only be updated for closed channels -- if the channel is open, the inbound payment is pruned automatically when the HTLC is no longer present in any unrevoked commitment transaction, which stops the redundant event regeneration. Upcoming commits will add the actual tracking that uses this map, as well as generating the relevant monitor update to trigger this tracking
When an Event::PaymentClaimed is processed by the user, we need to track that so we don't keep regenerating the event redundantly on startup. Here we add an event completion action for that. Note that this action will only be generated for closed channels -- if the channel is open, the inbound payment is pruned automatically when the HTLC is no longer present in any unrevoked commitment transaction, which stops the redundant event regeneration. Similar to EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate, but for inbound payments. Actual generation and handling of this completion action is added in an upcoming commit.
Previously, if we had an inbound payment on a closed channel that we started claiming but did not end up removing from the commitment tx, we would generate a PaymentClaimed event for the payment on every restart until the channelmonitor was archived. The past few commits have laid the groundwork to get rid of this redundancy -- here we now generate an event completion action for when the PaymentClaimed is processed by the user, at which point a monitor update will be released that tells the channel monitor that the user has processed the PaymentClaimed event. The monitor tracks this internally such that the pending claim will no longer be returned to the ChannelManager when reconstructing the set of mid-claim inbound payments on startup, and thus no longer generate the redundant event.
|
👋 Hi! This PR is now in draft status. |
valentinewallace
commented
Mar 6, 2026
| @@ -3249,6 +3270,20 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> { | |||
|
|
|||
| pub(crate) fn get_stored_preimages( | |||
Contributor
Author
There was a problem hiding this comment.
Not sure if we want to also rename this method
valentinewallace
commented
Mar 6, 2026
| // behavior (RAA updates are blocked until the PaymentClaimed event is handled). | ||
| // For closed channels, we use InboundPaymentClaimedChannelMonitorUpdate to persist | ||
| // that the PaymentClaimed event has been handled, preventing regeneration on restart. | ||
| if is_channel_closed { |
Contributor
Author
There was a problem hiding this comment.
I went with only generating these new monitor updates for claims on closed channels. So after #4462 we will still potentially have increased redundant PaymentClaimed events on restart for claims on open channels, until the claim is removed from all unrevoked commit txs. The approach is currently simple so I'm not sure it's worth fixing that, but open to thoughts.
4 tasks
TheBlueMatt
reviewed
Mar 6, 2026
| @@ -3249,6 +3270,20 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> { | |||
|
|
|||
| pub(crate) fn get_stored_preimages( | |||
| pub(crate) fn get_stored_preimages( | ||
| &self, | ||
| ) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> { | ||
| let inner = self.inner.lock().unwrap(); |
Collaborator
There was a problem hiding this comment.
Hmm, can we do this on a per-PaymentClaimDetails basis instead? In theory we can have two payments with the same hash/preimage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, if we had an inbound payment on a closed channel that we started
claiming but did not end up removing from the commitment tx, we would generate
a
PaymentClaimedevent for the payment on every restart until thechannelmonitor was archived.
Becomes more important with #4462 -- currently we rely on checking whether a payment is present in
ChannelManager::pending_claiming_paymentsto prevent some redundant event generation, but once we start always reconstructing that map we can no longer perform those checks.