Re-claim forwarded HTLCs on startup

[TheBlueMatt]

Because ChannelMonitorUpdates can complete asynchronously and
out-of-order now, a commitment_signedChannelMonitorUpdate from
a downstream channel could complete prior to the preimage
ChannelMonitorUpdate on the upstream channel. In that case, we may
not get a update_fulfill_htlc replay on startup. Thus, we have to
ensure any payment preimages contained in that downstream update are
re-claimed on startup.

Here we do this during the existing walk of the ChannelMonitor
preimages for closed channels.

This was originally a part of #2167 but got dropped as it was buggy. Its now been fixed. This should ideally go in 116 so that async users of 117 can safely downgrade, but if it doesn't make it that's somewhat okay.

Based on #2362.

[codecov-commenter]

Codecov Report

Patch coverage: 79.22% and project coverage change: +0.11 🎉

Comparison is base (0f2c4c0) 90.32% compared to head (192c5d2) 90.43%.

❗ Current head 192c5d2 differs from pull request most recent head 9ce7e8e. Consider uploading reports for the commit 9ce7e8e to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2364 +/- ##
==========================================
+ Coverage 90.32% 90.43% +0.11% 
==========================================
Files 106 106 Lines 54948 58208 +3260 Branches 54948 58208 +3260 ==========================================
+ Hits 49633 52642 +3009 - Misses 5315 5566 +251 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	89.42% <ø> (ø)
lightning/src/ln/channelmanager.rs	89.70% <79.22%> (+3.44%)	⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Rebased.

[alecchendev]

This generally looks pretty good to me, just one comment

[alecchendev]

I'm a little confused with this comment - I'm confused why only handling the completion_action after completing the monitor update is the only way to guarantee regenerating this update on startup? I'm thinking the completion_action that's used for claiming a forwarded HTLC upstream is just emitting a PaymentForwarded event right (I'm just looking at where this is used in claim_funds_internal), why would doing that before waiting for the monitor update to complete possibly mean the downstream preimage may be deleted before claiming upstream?

[TheBlueMatt]

When an HTLC which is forwarded is claimed, first we receive the preimage from the outbound edge, and we store the preimage in that channel's monitor. Then, we go and store the preimage in the inbound edge's monitor as well when we go to claim upstream. In the rare case that the outbound edge not only completes the initial monitor update but also one further monitor update, we will remove the preimage from that monitor and call it a day. By that point, we must make sure that the inbound edge's monitor has been safely updated and the preimage is durably in the previous channel, that is what the completion action does - unblocks the downstream channel monitor so that it can be updated.

[alecchendev]

Hmm okay, that part makes sense, thanks.

Just checking, we only unblock the downstream channel monitor when handling a MonitorUpdateCompletionAction::EmitEventAndFreeOtherChannel where downstream_counterparty_and_funding_outpoint = Some(..) right?

I'm wondering, is this then talking about the case where we generate this BackgroundEvent::ClosingMonitorUpdateRegeneratedOnStartup on startup, but then because we immediately handle the completion_action (instead of having a different kind of BackgroundEvent that does the completion_action after the monitor update completed like the comment suggests), in the situation that claim_funds_from_hop is called with a completion_action that returns a monitor update completion action that would unblock the outbound channel monitor, we'd risk the outbound channel completing a further monitor update and deleting the preimage? But in its current state this is safe because right now we only hit this path with a completion_action that returns a monitor completion action that doesn't free up an outbound channel (downstream_counterparty_and_funding_outpoint = None)?

I think my main confusion is around which parts of the comment are talking about this specific code path versus generally within claim_funds_from_hop?

[TheBlueMatt]

But in its current state this is safe..

No, in the current state it is not safe. We need to fix this on both this (the during-startup) case and on the not-during-startup case, but in either case its possible we remove the preimage before its durably on the inbound edge.

[valentinewallace]

CI sad, otherwise LGTM.

[TheBlueMatt]

Oops, background_events_processed_since_startup was test-only, I made it always-on.

[TheBlueMatt]

Squashed without further changes:

$ git diff-tree -U1 aabd35e7 d3811cd7
$ 

[TheBlueMatt]

Grrr, I missed one cfg:

$ git diff-tree -U1 d3811cd7 9ce7e8e6
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index faed19407..398975c65 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8888,3 +8888,2 @@ where
total_consistency_lock: RwLock::new(()),
- #[cfg(debug_assertions)]
background_events_processed_since_startup: AtomicBool::new(false),

[TheBlueMatt]

Because ChannelMonitorUpdates can complete asynchronously and
out-of-order now, a commitment_signedChannelMonitorUpdate from
a downstream channel could complete prior to the preimage
ChannelMonitorUpdate on the upstream channel. In that case, we may
not get a update_fulfill_htlc replay on startup. Thus, we have to
ensure any payment preimages contained in that downstream update are
re-claimed on startup.

Here we do this during the existing walk of the ChannelMonitor
preimages for closed channels.

This was originally a part of #2167 but got dropped as it was buggy. Its now been fixed. This should ideally go in 116 so that async users of 117 can safely downgrade, but if it doesn't make it that's somewhat okay.

Based on #2362.

[codecov-commenter]

Codecov Report

Patch coverage: 79.22% and project coverage change: +0.11 🎉

Comparison is base (0f2c4c0) 90.32% compared to head (192c5d2) 90.43%.

❗ Current head 192c5d2 differs from pull request most recent head 9ce7e8e. Consider uploading reports for the commit 9ce7e8e to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2364 +/- ##
==========================================
+ Coverage 90.32% 90.43% +0.11% 
==========================================
Files 106 106 Lines 54948 58208 +3260 Branches 54948 58208 +3260 ==========================================
+ Hits 49633 52642 +3009 - Misses 5315 5566 +251 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	89.42% <ø> (ø)
lightning/src/ln/channelmanager.rs	89.70% <79.22%> (+3.44%)	⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Rebased.

[alecchendev]

This generally looks pretty good to me, just one comment

[alecchendev]

I'm a little confused with this comment - I'm confused why only handling the completion_action after completing the monitor update is the only way to guarantee regenerating this update on startup? I'm thinking the completion_action that's used for claiming a forwarded HTLC upstream is just emitting a PaymentForwarded event right (I'm just looking at where this is used in claim_funds_internal), why would doing that before waiting for the monitor update to complete possibly mean the downstream preimage may be deleted before claiming upstream?

[TheBlueMatt]

When an HTLC which is forwarded is claimed, first we receive the preimage from the outbound edge, and we store the preimage in that channel's monitor. Then, we go and store the preimage in the inbound edge's monitor as well when we go to claim upstream. In the rare case that the outbound edge not only completes the initial monitor update but also one further monitor update, we will remove the preimage from that monitor and call it a day. By that point, we must make sure that the inbound edge's monitor has been safely updated and the preimage is durably in the previous channel, that is what the completion action does - unblocks the downstream channel monitor so that it can be updated.

[alecchendev]

Hmm okay, that part makes sense, thanks.

Just checking, we only unblock the downstream channel monitor when handling a MonitorUpdateCompletionAction::EmitEventAndFreeOtherChannel where downstream_counterparty_and_funding_outpoint = Some(..) right?

I'm wondering, is this then talking about the case where we generate this BackgroundEvent::ClosingMonitorUpdateRegeneratedOnStartup on startup, but then because we immediately handle the completion_action (instead of having a different kind of BackgroundEvent that does the completion_action after the monitor update completed like the comment suggests), in the situation that claim_funds_from_hop is called with a completion_action that returns a monitor update completion action that would unblock the outbound channel monitor, we'd risk the outbound channel completing a further monitor update and deleting the preimage? But in its current state this is safe because right now we only hit this path with a completion_action that returns a monitor completion action that doesn't free up an outbound channel (downstream_counterparty_and_funding_outpoint = None)?

I think my main confusion is around which parts of the comment are talking about this specific code path versus generally within claim_funds_from_hop?

[TheBlueMatt]

But in its current state this is safe..

No, in the current state it is not safe. We need to fix this on both this (the during-startup) case and on the not-during-startup case, but in either case its possible we remove the preimage before its durably on the inbound edge.

[valentinewallace]

CI sad, otherwise LGTM.

[TheBlueMatt]

Oops, background_events_processed_since_startup was test-only, I made it always-on.

[TheBlueMatt]

Squashed without further changes:

$ git diff-tree -U1 aabd35e7 d3811cd7
$ 

[TheBlueMatt]

Grrr, I missed one cfg:

$ git diff-tree -U1 d3811cd7 9ce7e8e6
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index faed19407..398975c65 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8888,3 +8888,2 @@ where
total_consistency_lock: RwLock::new(()),
- #[cfg(debug_assertions)]
background_events_processed_since_startup: AtomicBool::new(false),

[TheBlueMatt]

Because ChannelMonitorUpdates can complete asynchronously and
out-of-order now, a commitment_signedChannelMonitorUpdate from
a downstream channel could complete prior to the preimage
ChannelMonitorUpdate on the upstream channel. In that case, we may
not get a update_fulfill_htlc replay on startup. Thus, we have to
ensure any payment preimages contained in that downstream update are
re-claimed on startup.

Here we do this during the existing walk of the ChannelMonitor
preimages for closed channels.

This was originally a part of #2167 but got dropped as it was buggy. Its now been fixed. This should ideally go in 116 so that async users of 117 can safely downgrade, but if it doesn't make it that's somewhat okay.

Based on #2362.

[codecov-commenter]

Codecov Report

Patch coverage: 79.22% and project coverage change: +0.11 🎉

Comparison is base (0f2c4c0) 90.32% compared to head (192c5d2) 90.43%.

❗ Current head 192c5d2 differs from pull request most recent head 9ce7e8e. Consider uploading reports for the commit 9ce7e8e to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2364 +/- ##
==========================================
+ Coverage 90.32% 90.43% +0.11% 
==========================================
Files 106 106 Lines 54948 58208 +3260 Branches 54948 58208 +3260 ==========================================
+ Hits 49633 52642 +3009 - Misses 5315 5566 +251 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	89.42% <ø> (ø)
lightning/src/ln/channelmanager.rs	89.70% <79.22%> (+3.44%)	⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Rebased.

[alecchendev]

This generally looks pretty good to me, just one comment

[alecchendev]

I'm a little confused with this comment - I'm confused why only handling the completion_action after completing the monitor update is the only way to guarantee regenerating this update on startup? I'm thinking the completion_action that's used for claiming a forwarded HTLC upstream is just emitting a PaymentForwarded event right (I'm just looking at where this is used in claim_funds_internal), why would doing that before waiting for the monitor update to complete possibly mean the downstream preimage may be deleted before claiming upstream?

[TheBlueMatt]

When an HTLC which is forwarded is claimed, first we receive the preimage from the outbound edge, and we store the preimage in that channel's monitor. Then, we go and store the preimage in the inbound edge's monitor as well when we go to claim upstream. In the rare case that the outbound edge not only completes the initial monitor update but also one further monitor update, we will remove the preimage from that monitor and call it a day. By that point, we must make sure that the inbound edge's monitor has been safely updated and the preimage is durably in the previous channel, that is what the completion action does - unblocks the downstream channel monitor so that it can be updated.

[alecchendev]

Hmm okay, that part makes sense, thanks.

Just checking, we only unblock the downstream channel monitor when handling a MonitorUpdateCompletionAction::EmitEventAndFreeOtherChannel where downstream_counterparty_and_funding_outpoint = Some(..) right?

I'm wondering, is this then talking about the case where we generate this BackgroundEvent::ClosingMonitorUpdateRegeneratedOnStartup on startup, but then because we immediately handle the completion_action (instead of having a different kind of BackgroundEvent that does the completion_action after the monitor update completed like the comment suggests), in the situation that claim_funds_from_hop is called with a completion_action that returns a monitor update completion action that would unblock the outbound channel monitor, we'd risk the outbound channel completing a further monitor update and deleting the preimage? But in its current state this is safe because right now we only hit this path with a completion_action that returns a monitor completion action that doesn't free up an outbound channel (downstream_counterparty_and_funding_outpoint = None)?

I think my main confusion is around which parts of the comment are talking about this specific code path versus generally within claim_funds_from_hop?

[TheBlueMatt]

But in its current state this is safe..

No, in the current state it is not safe. We need to fix this on both this (the during-startup) case and on the not-during-startup case, but in either case its possible we remove the preimage before its durably on the inbound edge.

[valentinewallace]

CI sad, otherwise LGTM.

[TheBlueMatt]

Oops, background_events_processed_since_startup was test-only, I made it always-on.

[TheBlueMatt]

Squashed without further changes:

$ git diff-tree -U1 aabd35e7 d3811cd7
$ 

[TheBlueMatt]

Grrr, I missed one cfg:

$ git diff-tree -U1 d3811cd7 9ce7e8e6
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index faed19407..398975c65 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8888,3 +8888,2 @@ where
total_consistency_lock: RwLock::new(()),
- #[cfg(debug_assertions)]
background_events_processed_since_startup: AtomicBool::new(false),

[TheBlueMatt]

Because ChannelMonitorUpdates can complete asynchronously and
out-of-order now, a commitment_signedChannelMonitorUpdate from
a downstream channel could complete prior to the preimage
ChannelMonitorUpdate on the upstream channel. In that case, we may
not get a update_fulfill_htlc replay on startup. Thus, we have to
ensure any payment preimages contained in that downstream update are
re-claimed on startup.

Here we do this during the existing walk of the ChannelMonitor
preimages for closed channels.

This was originally a part of #2167 but got dropped as it was buggy. Its now been fixed. This should ideally go in 116 so that async users of 117 can safely downgrade, but if it doesn't make it that's somewhat okay.

Based on #2362.

[codecov-commenter]

Codecov Report

Patch coverage: 79.22% and project coverage change: +0.11 🎉

Comparison is base (0f2c4c0) 90.32% compared to head (192c5d2) 90.43%.

❗ Current head 192c5d2 differs from pull request most recent head 9ce7e8e. Consider uploading reports for the commit 9ce7e8e to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2364 +/- ##
==========================================
+ Coverage 90.32% 90.43% +0.11% 
==========================================
Files 106 106 Lines 54948 58208 +3260 Branches 54948 58208 +3260 ==========================================
+ Hits 49633 52642 +3009 - Misses 5315 5566 +251 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	89.42% <ø> (ø)
lightning/src/ln/channelmanager.rs	89.70% <79.22%> (+3.44%)	⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Rebased.

[alecchendev]

This generally looks pretty good to me, just one comment

[alecchendev]

I'm a little confused with this comment - I'm confused why only handling the completion_action after completing the monitor update is the only way to guarantee regenerating this update on startup? I'm thinking the completion_action that's used for claiming a forwarded HTLC upstream is just emitting a PaymentForwarded event right (I'm just looking at where this is used in claim_funds_internal), why would doing that before waiting for the monitor update to complete possibly mean the downstream preimage may be deleted before claiming upstream?

[TheBlueMatt]

When an HTLC which is forwarded is claimed, first we receive the preimage from the outbound edge, and we store the preimage in that channel's monitor. Then, we go and store the preimage in the inbound edge's monitor as well when we go to claim upstream. In the rare case that the outbound edge not only completes the initial monitor update but also one further monitor update, we will remove the preimage from that monitor and call it a day. By that point, we must make sure that the inbound edge's monitor has been safely updated and the preimage is durably in the previous channel, that is what the completion action does - unblocks the downstream channel monitor so that it can be updated.

[alecchendev]

Hmm okay, that part makes sense, thanks.

Just checking, we only unblock the downstream channel monitor when handling a MonitorUpdateCompletionAction::EmitEventAndFreeOtherChannel where downstream_counterparty_and_funding_outpoint = Some(..) right?

I'm wondering, is this then talking about the case where we generate this BackgroundEvent::ClosingMonitorUpdateRegeneratedOnStartup on startup, but then because we immediately handle the completion_action (instead of having a different kind of BackgroundEvent that does the completion_action after the monitor update completed like the comment suggests), in the situation that claim_funds_from_hop is called with a completion_action that returns a monitor update completion action that would unblock the outbound channel monitor, we'd risk the outbound channel completing a further monitor update and deleting the preimage? But in its current state this is safe because right now we only hit this path with a completion_action that returns a monitor completion action that doesn't free up an outbound channel (downstream_counterparty_and_funding_outpoint = None)?

I think my main confusion is around which parts of the comment are talking about this specific code path versus generally within claim_funds_from_hop?

[TheBlueMatt]

But in its current state this is safe..

No, in the current state it is not safe. We need to fix this on both this (the during-startup) case and on the not-during-startup case, but in either case its possible we remove the preimage before its durably on the inbound edge.

[valentinewallace]

CI sad, otherwise LGTM.

[TheBlueMatt]

Oops, background_events_processed_since_startup was test-only, I made it always-on.

[TheBlueMatt]

Squashed without further changes:

$ git diff-tree -U1 aabd35e7 d3811cd7
$ 

[TheBlueMatt]

Grrr, I missed one cfg:

$ git diff-tree -U1 d3811cd7 9ce7e8e6
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index faed19407..398975c65 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8888,3 +8888,2 @@ where
total_consistency_lock: RwLock::new(()),
- #[cfg(debug_assertions)]
background_events_processed_since_startup: AtomicBool::new(false),

[TheBlueMatt]

Because ChannelMonitorUpdates can complete asynchronously and
out-of-order now, a commitment_signedChannelMonitorUpdate from
a downstream channel could complete prior to the preimage
ChannelMonitorUpdate on the upstream channel. In that case, we may
not get a update_fulfill_htlc replay on startup. Thus, we have to
ensure any payment preimages contained in that downstream update are
re-claimed on startup.

Here we do this during the existing walk of the ChannelMonitor
preimages for closed channels.

This was originally a part of #2167 but got dropped as it was buggy. Its now been fixed. This should ideally go in 116 so that async users of 117 can safely downgrade, but if it doesn't make it that's somewhat okay.

Based on #2362.

[codecov-commenter]

Codecov Report

Patch coverage: 79.22% and project coverage change: +0.11 🎉

Comparison is base (0f2c4c0) 90.32% compared to head (192c5d2) 90.43%.

❗ Current head 192c5d2 differs from pull request most recent head 9ce7e8e. Consider uploading reports for the commit 9ce7e8e to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2364 +/- ##
==========================================
+ Coverage 90.32% 90.43% +0.11% 
==========================================
Files 106 106 Lines 54948 58208 +3260 Branches 54948 58208 +3260 ==========================================
+ Hits 49633 52642 +3009 - Misses 5315 5566 +251 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	89.42% <ø> (ø)
lightning/src/ln/channelmanager.rs	89.70% <79.22%> (+3.44%)	⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Rebased.

[alecchendev]

This generally looks pretty good to me, just one comment

[alecchendev]

I'm a little confused with this comment - I'm confused why only handling the completion_action after completing the monitor update is the only way to guarantee regenerating this update on startup? I'm thinking the completion_action that's used for claiming a forwarded HTLC upstream is just emitting a PaymentForwarded event right (I'm just looking at where this is used in claim_funds_internal), why would doing that before waiting for the monitor update to complete possibly mean the downstream preimage may be deleted before claiming upstream?

[TheBlueMatt]

When an HTLC which is forwarded is claimed, first we receive the preimage from the outbound edge, and we store the preimage in that channel's monitor. Then, we go and store the preimage in the inbound edge's monitor as well when we go to claim upstream. In the rare case that the outbound edge not only completes the initial monitor update but also one further monitor update, we will remove the preimage from that monitor and call it a day. By that point, we must make sure that the inbound edge's monitor has been safely updated and the preimage is durably in the previous channel, that is what the completion action does - unblocks the downstream channel monitor so that it can be updated.

[alecchendev]

Hmm okay, that part makes sense, thanks.

Just checking, we only unblock the downstream channel monitor when handling a MonitorUpdateCompletionAction::EmitEventAndFreeOtherChannel where downstream_counterparty_and_funding_outpoint = Some(..) right?

I'm wondering, is this then talking about the case where we generate this BackgroundEvent::ClosingMonitorUpdateRegeneratedOnStartup on startup, but then because we immediately handle the completion_action (instead of having a different kind of BackgroundEvent that does the completion_action after the monitor update completed like the comment suggests), in the situation that claim_funds_from_hop is called with a completion_action that returns a monitor update completion action that would unblock the outbound channel monitor, we'd risk the outbound channel completing a further monitor update and deleting the preimage? But in its current state this is safe because right now we only hit this path with a completion_action that returns a monitor completion action that doesn't free up an outbound channel (downstream_counterparty_and_funding_outpoint = None)?

I think my main confusion is around which parts of the comment are talking about this specific code path versus generally within claim_funds_from_hop?

[TheBlueMatt]

But in its current state this is safe..

No, in the current state it is not safe. We need to fix this on both this (the during-startup) case and on the not-during-startup case, but in either case its possible we remove the preimage before its durably on the inbound edge.

[valentinewallace]

CI sad, otherwise LGTM.

[TheBlueMatt]

Oops, background_events_processed_since_startup was test-only, I made it always-on.

[TheBlueMatt]

Squashed without further changes:

$ git diff-tree -U1 aabd35e7 d3811cd7
$ 

[TheBlueMatt]

Grrr, I missed one cfg:

$ git diff-tree -U1 d3811cd7 9ce7e8e6
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index faed19407..398975c65 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8888,3 +8888,2 @@ where
total_consistency_lock: RwLock::new(()),
- #[cfg(debug_assertions)]
background_events_processed_since_startup: AtomicBool::new(false),

[TheBlueMatt]

Because ChannelMonitorUpdates can complete asynchronously and
out-of-order now, a commitment_signedChannelMonitorUpdate from
a downstream channel could complete prior to the preimage
ChannelMonitorUpdate on the upstream channel. In that case, we may
not get a update_fulfill_htlc replay on startup. Thus, we have to
ensure any payment preimages contained in that downstream update are
re-claimed on startup.

Here we do this during the existing walk of the ChannelMonitor
preimages for closed channels.

This was originally a part of #2167 but got dropped as it was buggy. Its now been fixed. This should ideally go in 116 so that async users of 117 can safely downgrade, but if it doesn't make it that's somewhat okay.

Based on #2362.

[codecov-commenter]

Codecov Report

Patch coverage: 79.22% and project coverage change: +0.11 🎉

Comparison is base (0f2c4c0) 90.32% compared to head (192c5d2) 90.43%.

❗ Current head 192c5d2 differs from pull request most recent head 9ce7e8e. Consider uploading reports for the commit 9ce7e8e to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2364 +/- ##
==========================================
+ Coverage 90.32% 90.43% +0.11% 
==========================================
Files 106 106 Lines 54948 58208 +3260 Branches 54948 58208 +3260 ==========================================
+ Hits 49633 52642 +3009 - Misses 5315 5566 +251 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	89.42% <ø> (ø)
lightning/src/ln/channelmanager.rs	89.70% <79.22%> (+3.44%)	⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Rebased.

[alecchendev]

This generally looks pretty good to me, just one comment

[alecchendev]

I'm a little confused with this comment - I'm confused why only handling the completion_action after completing the monitor update is the only way to guarantee regenerating this update on startup? I'm thinking the completion_action that's used for claiming a forwarded HTLC upstream is just emitting a PaymentForwarded event right (I'm just looking at where this is used in claim_funds_internal), why would doing that before waiting for the monitor update to complete possibly mean the downstream preimage may be deleted before claiming upstream?

[TheBlueMatt]

When an HTLC which is forwarded is claimed, first we receive the preimage from the outbound edge, and we store the preimage in that channel's monitor. Then, we go and store the preimage in the inbound edge's monitor as well when we go to claim upstream. In the rare case that the outbound edge not only completes the initial monitor update but also one further monitor update, we will remove the preimage from that monitor and call it a day. By that point, we must make sure that the inbound edge's monitor has been safely updated and the preimage is durably in the previous channel, that is what the completion action does - unblocks the downstream channel monitor so that it can be updated.

[alecchendev]

Hmm okay, that part makes sense, thanks.

Just checking, we only unblock the downstream channel monitor when handling a MonitorUpdateCompletionAction::EmitEventAndFreeOtherChannel where downstream_counterparty_and_funding_outpoint = Some(..) right?

I'm wondering, is this then talking about the case where we generate this BackgroundEvent::ClosingMonitorUpdateRegeneratedOnStartup on startup, but then because we immediately handle the completion_action (instead of having a different kind of BackgroundEvent that does the completion_action after the monitor update completed like the comment suggests), in the situation that claim_funds_from_hop is called with a completion_action that returns a monitor update completion action that would unblock the outbound channel monitor, we'd risk the outbound channel completing a further monitor update and deleting the preimage? But in its current state this is safe because right now we only hit this path with a completion_action that returns a monitor completion action that doesn't free up an outbound channel (downstream_counterparty_and_funding_outpoint = None)?

I think my main confusion is around which parts of the comment are talking about this specific code path versus generally within claim_funds_from_hop?

[TheBlueMatt]

But in its current state this is safe..

No, in the current state it is not safe. We need to fix this on both this (the during-startup) case and on the not-during-startup case, but in either case its possible we remove the preimage before its durably on the inbound edge.

[valentinewallace]

CI sad, otherwise LGTM.

[TheBlueMatt]

Oops, background_events_processed_since_startup was test-only, I made it always-on.

[TheBlueMatt]

Squashed without further changes:

$ git diff-tree -U1 aabd35e7 d3811cd7
$ 

[TheBlueMatt]

Grrr, I missed one cfg:

$ git diff-tree -U1 d3811cd7 9ce7e8e6
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index faed19407..398975c65 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8888,3 +8888,2 @@ where
total_consistency_lock: RwLock::new(()),
- #[cfg(debug_assertions)]
background_events_processed_since_startup: AtomicBool::new(false),

[TheBlueMatt]

Because ChannelMonitorUpdates can complete asynchronously and
out-of-order now, a commitment_signedChannelMonitorUpdate from
a downstream channel could complete prior to the preimage
ChannelMonitorUpdate on the upstream channel. In that case, we may
not get a update_fulfill_htlc replay on startup. Thus, we have to
ensure any payment preimages contained in that downstream update are
re-claimed on startup.

Here we do this during the existing walk of the ChannelMonitor
preimages for closed channels.

This was originally a part of #2167 but got dropped as it was buggy. Its now been fixed. This should ideally go in 116 so that async users of 117 can safely downgrade, but if it doesn't make it that's somewhat okay.

Based on #2362.

[codecov-commenter]

Codecov Report

Patch coverage: 79.22% and project coverage change: +0.11 🎉

Comparison is base (0f2c4c0) 90.32% compared to head (192c5d2) 90.43%.

❗ Current head 192c5d2 differs from pull request most recent head 9ce7e8e. Consider uploading reports for the commit 9ce7e8e to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2364 +/- ##
==========================================
+ Coverage 90.32% 90.43% +0.11% 
==========================================
Files 106 106 Lines 54948 58208 +3260 Branches 54948 58208 +3260 ==========================================
+ Hits 49633 52642 +3009 - Misses 5315 5566 +251 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	89.42% <ø> (ø)
lightning/src/ln/channelmanager.rs	89.70% <79.22%> (+3.44%)	⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Rebased.

[alecchendev]

This generally looks pretty good to me, just one comment

[alecchendev]

I'm a little confused with this comment - I'm confused why only handling the completion_action after completing the monitor update is the only way to guarantee regenerating this update on startup? I'm thinking the completion_action that's used for claiming a forwarded HTLC upstream is just emitting a PaymentForwarded event right (I'm just looking at where this is used in claim_funds_internal), why would doing that before waiting for the monitor update to complete possibly mean the downstream preimage may be deleted before claiming upstream?

[TheBlueMatt]

When an HTLC which is forwarded is claimed, first we receive the preimage from the outbound edge, and we store the preimage in that channel's monitor. Then, we go and store the preimage in the inbound edge's monitor as well when we go to claim upstream. In the rare case that the outbound edge not only completes the initial monitor update but also one further monitor update, we will remove the preimage from that monitor and call it a day. By that point, we must make sure that the inbound edge's monitor has been safely updated and the preimage is durably in the previous channel, that is what the completion action does - unblocks the downstream channel monitor so that it can be updated.

[alecchendev]

Hmm okay, that part makes sense, thanks.

Just checking, we only unblock the downstream channel monitor when handling a MonitorUpdateCompletionAction::EmitEventAndFreeOtherChannel where downstream_counterparty_and_funding_outpoint = Some(..) right?

I'm wondering, is this then talking about the case where we generate this BackgroundEvent::ClosingMonitorUpdateRegeneratedOnStartup on startup, but then because we immediately handle the completion_action (instead of having a different kind of BackgroundEvent that does the completion_action after the monitor update completed like the comment suggests), in the situation that claim_funds_from_hop is called with a completion_action that returns a monitor update completion action that would unblock the outbound channel monitor, we'd risk the outbound channel completing a further monitor update and deleting the preimage? But in its current state this is safe because right now we only hit this path with a completion_action that returns a monitor completion action that doesn't free up an outbound channel (downstream_counterparty_and_funding_outpoint = None)?

I think my main confusion is around which parts of the comment are talking about this specific code path versus generally within claim_funds_from_hop?

[TheBlueMatt]

But in its current state this is safe..

No, in the current state it is not safe. We need to fix this on both this (the during-startup) case and on the not-during-startup case, but in either case its possible we remove the preimage before its durably on the inbound edge.

[valentinewallace]

CI sad, otherwise LGTM.

[TheBlueMatt]

Oops, background_events_processed_since_startup was test-only, I made it always-on.

[TheBlueMatt]

Squashed without further changes:

$ git diff-tree -U1 aabd35e7 d3811cd7
$ 

[TheBlueMatt]

Grrr, I missed one cfg:

$ git diff-tree -U1 d3811cd7 9ce7e8e6
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index faed19407..398975c65 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8888,3 +8888,2 @@ where
total_consistency_lock: RwLock::new(()),
- #[cfg(debug_assertions)]
background_events_processed_since_startup: AtomicBool::new(false),

[TheBlueMatt]

Because ChannelMonitorUpdates can complete asynchronously and
out-of-order now, a commitment_signedChannelMonitorUpdate from
a downstream channel could complete prior to the preimage
ChannelMonitorUpdate on the upstream channel. In that case, we may
not get a update_fulfill_htlc replay on startup. Thus, we have to
ensure any payment preimages contained in that downstream update are
re-claimed on startup.

Here we do this during the existing walk of the ChannelMonitor
preimages for closed channels.

This was originally a part of #2167 but got dropped as it was buggy. Its now been fixed. This should ideally go in 116 so that async users of 117 can safely downgrade, but if it doesn't make it that's somewhat okay.

Based on #2362.

[codecov-commenter]

Codecov Report

Patch coverage: 79.22% and project coverage change: +0.11 🎉

Comparison is base (0f2c4c0) 90.32% compared to head (192c5d2) 90.43%.

❗ Current head 192c5d2 differs from pull request most recent head 9ce7e8e. Consider uploading reports for the commit 9ce7e8e to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2364 +/- ##
==========================================
+ Coverage 90.32% 90.43% +0.11% 
==========================================
Files 106 106 Lines 54948 58208 +3260 Branches 54948 58208 +3260 ==========================================
+ Hits 49633 52642 +3009 - Misses 5315 5566 +251 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	89.42% <ø> (ø)
lightning/src/ln/channelmanager.rs	89.70% <79.22%> (+3.44%)	⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Rebased.

[alecchendev]

This generally looks pretty good to me, just one comment

[alecchendev]

I'm a little confused with this comment - I'm confused why only handling the completion_action after completing the monitor update is the only way to guarantee regenerating this update on startup? I'm thinking the completion_action that's used for claiming a forwarded HTLC upstream is just emitting a PaymentForwarded event right (I'm just looking at where this is used in claim_funds_internal), why would doing that before waiting for the monitor update to complete possibly mean the downstream preimage may be deleted before claiming upstream?

[TheBlueMatt]

When an HTLC which is forwarded is claimed, first we receive the preimage from the outbound edge, and we store the preimage in that channel's monitor. Then, we go and store the preimage in the inbound edge's monitor as well when we go to claim upstream. In the rare case that the outbound edge not only completes the initial monitor update but also one further monitor update, we will remove the preimage from that monitor and call it a day. By that point, we must make sure that the inbound edge's monitor has been safely updated and the preimage is durably in the previous channel, that is what the completion action does - unblocks the downstream channel monitor so that it can be updated.

[alecchendev]

Hmm okay, that part makes sense, thanks.

Just checking, we only unblock the downstream channel monitor when handling a MonitorUpdateCompletionAction::EmitEventAndFreeOtherChannel where downstream_counterparty_and_funding_outpoint = Some(..) right?

I'm wondering, is this then talking about the case where we generate this BackgroundEvent::ClosingMonitorUpdateRegeneratedOnStartup on startup, but then because we immediately handle the completion_action (instead of having a different kind of BackgroundEvent that does the completion_action after the monitor update completed like the comment suggests), in the situation that claim_funds_from_hop is called with a completion_action that returns a monitor update completion action that would unblock the outbound channel monitor, we'd risk the outbound channel completing a further monitor update and deleting the preimage? But in its current state this is safe because right now we only hit this path with a completion_action that returns a monitor completion action that doesn't free up an outbound channel (downstream_counterparty_and_funding_outpoint = None)?

I think my main confusion is around which parts of the comment are talking about this specific code path versus generally within claim_funds_from_hop?

[TheBlueMatt]

But in its current state this is safe..

No, in the current state it is not safe. We need to fix this on both this (the during-startup) case and on the not-during-startup case, but in either case its possible we remove the preimage before its durably on the inbound edge.

[valentinewallace]

CI sad, otherwise LGTM.

[TheBlueMatt]

Oops, background_events_processed_since_startup was test-only, I made it always-on.

[TheBlueMatt]

Squashed without further changes:

$ git diff-tree -U1 aabd35e7 d3811cd7
$ 

[TheBlueMatt]

Grrr, I missed one cfg:

$ git diff-tree -U1 d3811cd7 9ce7e8e6
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index faed19407..398975c65 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8888,3 +8888,2 @@ where
total_consistency_lock: RwLock::new(()),
- #[cfg(debug_assertions)]
background_events_processed_since_startup: AtomicBool::new(false),