Move in-flight ChannelMonitorUpdates to ChannelManager by TheBlueMatt · Pull Request #2362 · lightningdevkit/rust-lightning

TheBlueMatt · 2023-06-19T06:33:36Z

Because ChannelMonitorUpdates can be generated for a
channel which is already closed, and must still be tracked
through their completion, storing them in a Channel
doesn't make sense - we'd have to have a redundant place to
put them post-closure and handle both storage locations
equivalently.

Instead, here, we move to storing in-flight
ChannelMonitorUpdates to the ChannelManager, leaving
blocked ChannelMonitorUpdates in the Channel as they
were.

This is the first (and largest) step towards #2168, and after this we're at least clear of the serialization-breaking parts, so could in theory cut a test version even without fully fixing 2168 (which needs fixing for 116 though).

Most of the work for #2317

codecov-commenter · 2023-06-20T02:24:06Z

Codecov Report

Patch coverage: 83.20% and project coverage change: -0.02⚠️

Comparison is base (15b1c9b) 90.30% compared to head (9c3ad28) 90.29%.

❗ 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 #2362 +/- ##
==========================================
- Coverage 90.30% 90.29% -0.02% 
==========================================
Files 106 106 Lines 54747 54920 +173 Branches 54747 54920 +173 ==========================================
+ Hits 49441 49591 +150 - Misses 5306 5329 +23

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`85.07% <0.00%> (-0.14%)`	⬇️
lightning/src/ln/channel.rs	`89.42% <83.33%> (-0.08%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.19% <83.96%> (-0.11%)`	⬇️

... and 13 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.

dunxen

Concept ACK. Just reviewing f52ed23 a bit more in-depth.

dunxen

LGTM. Can't see anything else glaringly wrong from my knowledge of monitor updates.

valentinewallace · 2023-06-21T18:32:35Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it only correct to call last() if we actually pushed an update above?

I mean if the vec contains something then it should also be safe?

Isn't it possible that we've already called update_channel with the last() monitor update, though?

Hmmmm, yea, maybe, I updated the code to handle it. We shouldn't be double-calling, but indeed I think there's maybe a case where we have two background updates to apply and we just apply the last() twice.

alecchendev

Was mostly just absorbing the context here, nothing outstanding to comment on

In the coming commits we'll move to storing in-flight `ChannelMonitorUpdate`s in the `ChannelManager` rather in the `Channel` (which will then only retain `ChannelMonitorUpdate`s which have not yet been released/are blocked. This will simplify handling of pending `ChannelMonitorUpdate` after a channel has closed by not having to move them into the `ChannelManager`.

Most of the calls to the `handle_new_monitor_update` macro had the exact same pattern - calling `update_monitor` followed by the macro. Given that common pattern will grow to first pushing the new monitor onto an in-flight set and then calling `update_monitor` unifying the pattern into a single macro now avoids more code churn in the coming commits.

By giving up on a tiny bit of parallelism and tweaking the return types, we can make the `handle_new_monitor_update` macro a bit clearer - now the only cases where its called after a monitor was updated was when the monitor was initially committed.

TheBlueMatt · 2023-06-21T22:52:43Z

Rebased.

valentinewallace · 2023-06-22T18:02:13Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it possible that we've already called update_channel with the last() monitor update, though?

valentinewallace

LGTM after this.

wpaulino

Feel free to squash.

wpaulino · 2023-06-23T18:01:46Z

- highest_applied_update_id, channel.get().context.get_latest_monitor_update_id());
+ let remaining_in_flight =
+ if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) {
+ pending.retain(|upd| upd.update_id > highest_applied_update_id);


If there are multiple updates in flight, would you call channel_monitor_updated once after they're all done, or after each one is done? If the latter, and they complete out of order, it seems this doesn't work?

Only after they're all done, this is implemented in ChainMonitor. We should probably move to doing it per-update, but we previously didn't track the updates in-flight so couldn't.

TheBlueMatt · 2023-06-23T18:24:35Z

Fixed spelling and squashed:

$ git diff-tree -U1 d18668b4 cd76e97c
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 942998840..cff623caa 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8399,3 +8399,3 @@ where
//
- // Because tha actual handling of the in-flight updates is the same, its macro'ized here:+ // Because the actual handling of the in-flight updates is the same, it's macro'ized here:
let mut pending_background_events = Vec::new();

TheBlueMatt · 2023-06-23T18:43:56Z

Oops, fixed one more bug @wpaulino pointed out that got introduced in one of the fixups:

$ git diff-tree -U1 cd76e97c e089a404
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 36260f1d2..69a69e7a1 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -865,3 +865,3 @@ pub(super) struct ChannelContext<Signer: ChannelSigner> {
/// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we
-	/// store it here and only release it to [`ChannelManager`] once it asks for it.+	/// store it here and only release it to the `ChannelManager` once it asks for it.
blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cff623caa..f38c6c13d 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -1923,3 +1923,3 @@ macro_rules! handle_new_monitor_update {
{
- in_flight_updates.pop();+ let _ = in_flight_updates.remove(idx);
if in_flight_updates.is_empty() && $chan.blocked_monitor_updates_pending() == 0 {

Because `ChannelMonitorUpdate`s can be generated for a channel which is already closed, and must still be tracked through their completion, storing them in a `Channel` doesn't make sense - we'd have to have a redundant place to put them post-closure and handle both storage locations equivalently. Instead, here, we move to storing in-flight `ChannelMonitorUpdate`s to the `ChannelManager`, leaving blocked `ChannelMonitorUpdate`s in the `Channel` as they were.

Now that all `ChannelMonitorUpdate`s stored in `Channel` are blocked we don't need a bool to track it.

`Channel::get_latest_complete_monitor_update_id` no longer refers to complete updates, but rather ones which were passed to the `ChannelManager` and which the `CHannel` no longer knows about. Thus, we rename it `get_latest_unblocked_monitor_update_id`.

To differentiate between in-flight pending completion and blocked updates.

TheBlueMatt · 2023-06-23T19:25:56Z

Grr, intermediate commit had intermediate state, had to shuffle around one line diff, but no total change:

$ git diff-tree -U1 e089a404 9c3ad28f
$

TheBlueMatt · 2023-06-19T06:33:36Z

Because ChannelMonitorUpdates can be generated for a
channel which is already closed, and must still be tracked
through their completion, storing them in a Channel
doesn't make sense - we'd have to have a redundant place to
put them post-closure and handle both storage locations
equivalently.

Instead, here, we move to storing in-flight
ChannelMonitorUpdates to the ChannelManager, leaving
blocked ChannelMonitorUpdates in the Channel as they
were.

This is the first (and largest) step towards #2168, and after this we're at least clear of the serialization-breaking parts, so could in theory cut a test version even without fully fixing 2168 (which needs fixing for 116 though).

Most of the work for #2317

codecov-commenter · 2023-06-20T02:24:06Z

Codecov Report

Patch coverage: 83.20% and project coverage change: -0.02⚠️

Comparison is base (15b1c9b) 90.30% compared to head (9c3ad28) 90.29%.

❗ 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 #2362 +/- ##
==========================================
- Coverage 90.30% 90.29% -0.02% 
==========================================
Files 106 106 Lines 54747 54920 +173 Branches 54747 54920 +173 ==========================================
+ Hits 49441 49591 +150 - Misses 5306 5329 +23

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`85.07% <0.00%> (-0.14%)`	⬇️
lightning/src/ln/channel.rs	`89.42% <83.33%> (-0.08%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.19% <83.96%> (-0.11%)`	⬇️

... and 13 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.

dunxen

Concept ACK. Just reviewing f52ed23 a bit more in-depth.

dunxen

LGTM. Can't see anything else glaringly wrong from my knowledge of monitor updates.

valentinewallace · 2023-06-21T18:32:35Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it only correct to call last() if we actually pushed an update above?

I mean if the vec contains something then it should also be safe?

Isn't it possible that we've already called update_channel with the last() monitor update, though?

Hmmmm, yea, maybe, I updated the code to handle it. We shouldn't be double-calling, but indeed I think there's maybe a case where we have two background updates to apply and we just apply the last() twice.

alecchendev

Was mostly just absorbing the context here, nothing outstanding to comment on

In the coming commits we'll move to storing in-flight `ChannelMonitorUpdate`s in the `ChannelManager` rather in the `Channel` (which will then only retain `ChannelMonitorUpdate`s which have not yet been released/are blocked. This will simplify handling of pending `ChannelMonitorUpdate` after a channel has closed by not having to move them into the `ChannelManager`.

Most of the calls to the `handle_new_monitor_update` macro had the exact same pattern - calling `update_monitor` followed by the macro. Given that common pattern will grow to first pushing the new monitor onto an in-flight set and then calling `update_monitor` unifying the pattern into a single macro now avoids more code churn in the coming commits.

By giving up on a tiny bit of parallelism and tweaking the return types, we can make the `handle_new_monitor_update` macro a bit clearer - now the only cases where its called after a monitor was updated was when the monitor was initially committed.

TheBlueMatt · 2023-06-21T22:52:43Z

Rebased.

valentinewallace · 2023-06-22T18:02:13Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it possible that we've already called update_channel with the last() monitor update, though?

valentinewallace

LGTM after this.

wpaulino

Feel free to squash.

wpaulino · 2023-06-23T18:01:46Z

- highest_applied_update_id, channel.get().context.get_latest_monitor_update_id());
+ let remaining_in_flight =
+ if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) {
+ pending.retain(|upd| upd.update_id > highest_applied_update_id);


If there are multiple updates in flight, would you call channel_monitor_updated once after they're all done, or after each one is done? If the latter, and they complete out of order, it seems this doesn't work?

Only after they're all done, this is implemented in ChainMonitor. We should probably move to doing it per-update, but we previously didn't track the updates in-flight so couldn't.

TheBlueMatt · 2023-06-23T18:24:35Z

Fixed spelling and squashed:

$ git diff-tree -U1 d18668b4 cd76e97c
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 942998840..cff623caa 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8399,3 +8399,3 @@ where
//
- // Because tha actual handling of the in-flight updates is the same, its macro'ized here:+ // Because the actual handling of the in-flight updates is the same, it's macro'ized here:
let mut pending_background_events = Vec::new();

TheBlueMatt · 2023-06-23T18:43:56Z

Oops, fixed one more bug @wpaulino pointed out that got introduced in one of the fixups:

$ git diff-tree -U1 cd76e97c e089a404
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 36260f1d2..69a69e7a1 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -865,3 +865,3 @@ pub(super) struct ChannelContext<Signer: ChannelSigner> {
/// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we
-	/// store it here and only release it to [`ChannelManager`] once it asks for it.+	/// store it here and only release it to the `ChannelManager` once it asks for it.
blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cff623caa..f38c6c13d 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -1923,3 +1923,3 @@ macro_rules! handle_new_monitor_update {
{
- in_flight_updates.pop();+ let _ = in_flight_updates.remove(idx);
if in_flight_updates.is_empty() && $chan.blocked_monitor_updates_pending() == 0 {

Because `ChannelMonitorUpdate`s can be generated for a channel which is already closed, and must still be tracked through their completion, storing them in a `Channel` doesn't make sense - we'd have to have a redundant place to put them post-closure and handle both storage locations equivalently. Instead, here, we move to storing in-flight `ChannelMonitorUpdate`s to the `ChannelManager`, leaving blocked `ChannelMonitorUpdate`s in the `Channel` as they were.

Now that all `ChannelMonitorUpdate`s stored in `Channel` are blocked we don't need a bool to track it.

`Channel::get_latest_complete_monitor_update_id` no longer refers to complete updates, but rather ones which were passed to the `ChannelManager` and which the `CHannel` no longer knows about. Thus, we rename it `get_latest_unblocked_monitor_update_id`.

To differentiate between in-flight pending completion and blocked updates.

TheBlueMatt · 2023-06-23T19:25:56Z

Grr, intermediate commit had intermediate state, had to shuffle around one line diff, but no total change:

$ git diff-tree -U1 e089a404 9c3ad28f
$

TheBlueMatt · 2023-06-19T06:33:36Z

Because ChannelMonitorUpdates can be generated for a
channel which is already closed, and must still be tracked
through their completion, storing them in a Channel
doesn't make sense - we'd have to have a redundant place to
put them post-closure and handle both storage locations
equivalently.

Instead, here, we move to storing in-flight
ChannelMonitorUpdates to the ChannelManager, leaving
blocked ChannelMonitorUpdates in the Channel as they
were.

This is the first (and largest) step towards #2168, and after this we're at least clear of the serialization-breaking parts, so could in theory cut a test version even without fully fixing 2168 (which needs fixing for 116 though).

Most of the work for #2317

codecov-commenter · 2023-06-20T02:24:06Z

Codecov Report

Patch coverage: 83.20% and project coverage change: -0.02⚠️

Comparison is base (15b1c9b) 90.30% compared to head (9c3ad28) 90.29%.

❗ 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 #2362 +/- ##
==========================================
- Coverage 90.30% 90.29% -0.02% 
==========================================
Files 106 106 Lines 54747 54920 +173 Branches 54747 54920 +173 ==========================================
+ Hits 49441 49591 +150 - Misses 5306 5329 +23

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`85.07% <0.00%> (-0.14%)`	⬇️
lightning/src/ln/channel.rs	`89.42% <83.33%> (-0.08%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.19% <83.96%> (-0.11%)`	⬇️

... and 13 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.

dunxen

Concept ACK. Just reviewing f52ed23 a bit more in-depth.

dunxen

LGTM. Can't see anything else glaringly wrong from my knowledge of monitor updates.

valentinewallace · 2023-06-21T18:32:35Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it only correct to call last() if we actually pushed an update above?

I mean if the vec contains something then it should also be safe?

Isn't it possible that we've already called update_channel with the last() monitor update, though?

Hmmmm, yea, maybe, I updated the code to handle it. We shouldn't be double-calling, but indeed I think there's maybe a case where we have two background updates to apply and we just apply the last() twice.

alecchendev

Was mostly just absorbing the context here, nothing outstanding to comment on

In the coming commits we'll move to storing in-flight `ChannelMonitorUpdate`s in the `ChannelManager` rather in the `Channel` (which will then only retain `ChannelMonitorUpdate`s which have not yet been released/are blocked. This will simplify handling of pending `ChannelMonitorUpdate` after a channel has closed by not having to move them into the `ChannelManager`.

Most of the calls to the `handle_new_monitor_update` macro had the exact same pattern - calling `update_monitor` followed by the macro. Given that common pattern will grow to first pushing the new monitor onto an in-flight set and then calling `update_monitor` unifying the pattern into a single macro now avoids more code churn in the coming commits.

By giving up on a tiny bit of parallelism and tweaking the return types, we can make the `handle_new_monitor_update` macro a bit clearer - now the only cases where its called after a monitor was updated was when the monitor was initially committed.

TheBlueMatt · 2023-06-21T22:52:43Z

Rebased.

valentinewallace · 2023-06-22T18:02:13Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it possible that we've already called update_channel with the last() monitor update, though?

valentinewallace

LGTM after this.

wpaulino

Feel free to squash.

wpaulino · 2023-06-23T18:01:46Z

- highest_applied_update_id, channel.get().context.get_latest_monitor_update_id());
+ let remaining_in_flight =
+ if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) {
+ pending.retain(|upd| upd.update_id > highest_applied_update_id);


If there are multiple updates in flight, would you call channel_monitor_updated once after they're all done, or after each one is done? If the latter, and they complete out of order, it seems this doesn't work?

Only after they're all done, this is implemented in ChainMonitor. We should probably move to doing it per-update, but we previously didn't track the updates in-flight so couldn't.

TheBlueMatt · 2023-06-23T18:24:35Z

Fixed spelling and squashed:

$ git diff-tree -U1 d18668b4 cd76e97c
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 942998840..cff623caa 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8399,3 +8399,3 @@ where
//
- // Because tha actual handling of the in-flight updates is the same, its macro'ized here:+ // Because the actual handling of the in-flight updates is the same, it's macro'ized here:
let mut pending_background_events = Vec::new();

TheBlueMatt · 2023-06-23T18:43:56Z

Oops, fixed one more bug @wpaulino pointed out that got introduced in one of the fixups:

$ git diff-tree -U1 cd76e97c e089a404
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 36260f1d2..69a69e7a1 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -865,3 +865,3 @@ pub(super) struct ChannelContext<Signer: ChannelSigner> {
/// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we
-	/// store it here and only release it to [`ChannelManager`] once it asks for it.+	/// store it here and only release it to the `ChannelManager` once it asks for it.
blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cff623caa..f38c6c13d 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -1923,3 +1923,3 @@ macro_rules! handle_new_monitor_update {
{
- in_flight_updates.pop();+ let _ = in_flight_updates.remove(idx);
if in_flight_updates.is_empty() && $chan.blocked_monitor_updates_pending() == 0 {

Because `ChannelMonitorUpdate`s can be generated for a channel which is already closed, and must still be tracked through their completion, storing them in a `Channel` doesn't make sense - we'd have to have a redundant place to put them post-closure and handle both storage locations equivalently. Instead, here, we move to storing in-flight `ChannelMonitorUpdate`s to the `ChannelManager`, leaving blocked `ChannelMonitorUpdate`s in the `Channel` as they were.

Now that all `ChannelMonitorUpdate`s stored in `Channel` are blocked we don't need a bool to track it.

`Channel::get_latest_complete_monitor_update_id` no longer refers to complete updates, but rather ones which were passed to the `ChannelManager` and which the `CHannel` no longer knows about. Thus, we rename it `get_latest_unblocked_monitor_update_id`.

To differentiate between in-flight pending completion and blocked updates.

TheBlueMatt · 2023-06-23T19:25:56Z

Grr, intermediate commit had intermediate state, had to shuffle around one line diff, but no total change:

$ git diff-tree -U1 e089a404 9c3ad28f
$

TheBlueMatt · 2023-06-19T06:33:36Z

Because ChannelMonitorUpdates can be generated for a
channel which is already closed, and must still be tracked
through their completion, storing them in a Channel
doesn't make sense - we'd have to have a redundant place to
put them post-closure and handle both storage locations
equivalently.

Instead, here, we move to storing in-flight
ChannelMonitorUpdates to the ChannelManager, leaving
blocked ChannelMonitorUpdates in the Channel as they
were.

This is the first (and largest) step towards #2168, and after this we're at least clear of the serialization-breaking parts, so could in theory cut a test version even without fully fixing 2168 (which needs fixing for 116 though).

Most of the work for #2317

codecov-commenter · 2023-06-20T02:24:06Z

Codecov Report

Patch coverage: 83.20% and project coverage change: -0.02⚠️

Comparison is base (15b1c9b) 90.30% compared to head (9c3ad28) 90.29%.

❗ 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 #2362 +/- ##
==========================================
- Coverage 90.30% 90.29% -0.02% 
==========================================
Files 106 106 Lines 54747 54920 +173 Branches 54747 54920 +173 ==========================================
+ Hits 49441 49591 +150 - Misses 5306 5329 +23

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`85.07% <0.00%> (-0.14%)`	⬇️
lightning/src/ln/channel.rs	`89.42% <83.33%> (-0.08%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.19% <83.96%> (-0.11%)`	⬇️

... and 13 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.

dunxen

Concept ACK. Just reviewing f52ed23 a bit more in-depth.

dunxen

LGTM. Can't see anything else glaringly wrong from my knowledge of monitor updates.

valentinewallace · 2023-06-21T18:32:35Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it only correct to call last() if we actually pushed an update above?

I mean if the vec contains something then it should also be safe?

Isn't it possible that we've already called update_channel with the last() monitor update, though?

Hmmmm, yea, maybe, I updated the code to handle it. We shouldn't be double-calling, but indeed I think there's maybe a case where we have two background updates to apply and we just apply the last() twice.

alecchendev

Was mostly just absorbing the context here, nothing outstanding to comment on

In the coming commits we'll move to storing in-flight `ChannelMonitorUpdate`s in the `ChannelManager` rather in the `Channel` (which will then only retain `ChannelMonitorUpdate`s which have not yet been released/are blocked. This will simplify handling of pending `ChannelMonitorUpdate` after a channel has closed by not having to move them into the `ChannelManager`.

Most of the calls to the `handle_new_monitor_update` macro had the exact same pattern - calling `update_monitor` followed by the macro. Given that common pattern will grow to first pushing the new monitor onto an in-flight set and then calling `update_monitor` unifying the pattern into a single macro now avoids more code churn in the coming commits.

By giving up on a tiny bit of parallelism and tweaking the return types, we can make the `handle_new_monitor_update` macro a bit clearer - now the only cases where its called after a monitor was updated was when the monitor was initially committed.

TheBlueMatt · 2023-06-21T22:52:43Z

Rebased.

valentinewallace · 2023-06-22T18:02:13Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it possible that we've already called update_channel with the last() monitor update, though?

valentinewallace

LGTM after this.

wpaulino

Feel free to squash.

wpaulino · 2023-06-23T18:01:46Z

- highest_applied_update_id, channel.get().context.get_latest_monitor_update_id());
+ let remaining_in_flight =
+ if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) {
+ pending.retain(|upd| upd.update_id > highest_applied_update_id);


If there are multiple updates in flight, would you call channel_monitor_updated once after they're all done, or after each one is done? If the latter, and they complete out of order, it seems this doesn't work?

Only after they're all done, this is implemented in ChainMonitor. We should probably move to doing it per-update, but we previously didn't track the updates in-flight so couldn't.

TheBlueMatt · 2023-06-23T18:24:35Z

Fixed spelling and squashed:

$ git diff-tree -U1 d18668b4 cd76e97c
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 942998840..cff623caa 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8399,3 +8399,3 @@ where
//
- // Because tha actual handling of the in-flight updates is the same, its macro'ized here:+ // Because the actual handling of the in-flight updates is the same, it's macro'ized here:
let mut pending_background_events = Vec::new();

TheBlueMatt · 2023-06-23T18:43:56Z

Oops, fixed one more bug @wpaulino pointed out that got introduced in one of the fixups:

$ git diff-tree -U1 cd76e97c e089a404
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 36260f1d2..69a69e7a1 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -865,3 +865,3 @@ pub(super) struct ChannelContext<Signer: ChannelSigner> {
/// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we
-	/// store it here and only release it to [`ChannelManager`] once it asks for it.+	/// store it here and only release it to the `ChannelManager` once it asks for it.
blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cff623caa..f38c6c13d 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -1923,3 +1923,3 @@ macro_rules! handle_new_monitor_update {
{
- in_flight_updates.pop();+ let _ = in_flight_updates.remove(idx);
if in_flight_updates.is_empty() && $chan.blocked_monitor_updates_pending() == 0 {

Because `ChannelMonitorUpdate`s can be generated for a channel which is already closed, and must still be tracked through their completion, storing them in a `Channel` doesn't make sense - we'd have to have a redundant place to put them post-closure and handle both storage locations equivalently. Instead, here, we move to storing in-flight `ChannelMonitorUpdate`s to the `ChannelManager`, leaving blocked `ChannelMonitorUpdate`s in the `Channel` as they were.

Now that all `ChannelMonitorUpdate`s stored in `Channel` are blocked we don't need a bool to track it.

`Channel::get_latest_complete_monitor_update_id` no longer refers to complete updates, but rather ones which were passed to the `ChannelManager` and which the `CHannel` no longer knows about. Thus, we rename it `get_latest_unblocked_monitor_update_id`.

To differentiate between in-flight pending completion and blocked updates.

TheBlueMatt · 2023-06-23T19:25:56Z

Grr, intermediate commit had intermediate state, had to shuffle around one line diff, but no total change:

$ git diff-tree -U1 e089a404 9c3ad28f
$

TheBlueMatt · 2023-06-19T06:33:36Z

Because ChannelMonitorUpdates can be generated for a
channel which is already closed, and must still be tracked
through their completion, storing them in a Channel
doesn't make sense - we'd have to have a redundant place to
put them post-closure and handle both storage locations
equivalently.

Instead, here, we move to storing in-flight
ChannelMonitorUpdates to the ChannelManager, leaving
blocked ChannelMonitorUpdates in the Channel as they
were.

This is the first (and largest) step towards #2168, and after this we're at least clear of the serialization-breaking parts, so could in theory cut a test version even without fully fixing 2168 (which needs fixing for 116 though).

Most of the work for #2317

codecov-commenter · 2023-06-20T02:24:06Z

Codecov Report

Patch coverage: 83.20% and project coverage change: -0.02⚠️

Comparison is base (15b1c9b) 90.30% compared to head (9c3ad28) 90.29%.

❗ 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 #2362 +/- ##
==========================================
- Coverage 90.30% 90.29% -0.02% 
==========================================
Files 106 106 Lines 54747 54920 +173 Branches 54747 54920 +173 ==========================================
+ Hits 49441 49591 +150 - Misses 5306 5329 +23

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`85.07% <0.00%> (-0.14%)`	⬇️
lightning/src/ln/channel.rs	`89.42% <83.33%> (-0.08%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.19% <83.96%> (-0.11%)`	⬇️

... and 13 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.

dunxen

Concept ACK. Just reviewing f52ed23 a bit more in-depth.

dunxen

LGTM. Can't see anything else glaringly wrong from my knowledge of monitor updates.

valentinewallace · 2023-06-21T18:32:35Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it only correct to call last() if we actually pushed an update above?

I mean if the vec contains something then it should also be safe?

Isn't it possible that we've already called update_channel with the last() monitor update, though?

Hmmmm, yea, maybe, I updated the code to handle it. We shouldn't be double-calling, but indeed I think there's maybe a case where we have two background updates to apply and we just apply the last() twice.

alecchendev

Was mostly just absorbing the context here, nothing outstanding to comment on

In the coming commits we'll move to storing in-flight `ChannelMonitorUpdate`s in the `ChannelManager` rather in the `Channel` (which will then only retain `ChannelMonitorUpdate`s which have not yet been released/are blocked. This will simplify handling of pending `ChannelMonitorUpdate` after a channel has closed by not having to move them into the `ChannelManager`.

Most of the calls to the `handle_new_monitor_update` macro had the exact same pattern - calling `update_monitor` followed by the macro. Given that common pattern will grow to first pushing the new monitor onto an in-flight set and then calling `update_monitor` unifying the pattern into a single macro now avoids more code churn in the coming commits.

By giving up on a tiny bit of parallelism and tweaking the return types, we can make the `handle_new_monitor_update` macro a bit clearer - now the only cases where its called after a monitor was updated was when the monitor was initially committed.

TheBlueMatt · 2023-06-21T22:52:43Z

Rebased.

valentinewallace · 2023-06-22T18:02:13Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it possible that we've already called update_channel with the last() monitor update, though?

valentinewallace

LGTM after this.

wpaulino

Feel free to squash.

wpaulino · 2023-06-23T18:01:46Z

- highest_applied_update_id, channel.get().context.get_latest_monitor_update_id());
+ let remaining_in_flight =
+ if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) {
+ pending.retain(|upd| upd.update_id > highest_applied_update_id);


If there are multiple updates in flight, would you call channel_monitor_updated once after they're all done, or after each one is done? If the latter, and they complete out of order, it seems this doesn't work?

Only after they're all done, this is implemented in ChainMonitor. We should probably move to doing it per-update, but we previously didn't track the updates in-flight so couldn't.

TheBlueMatt · 2023-06-23T18:24:35Z

Fixed spelling and squashed:

$ git diff-tree -U1 d18668b4 cd76e97c
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 942998840..cff623caa 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8399,3 +8399,3 @@ where
//
- // Because tha actual handling of the in-flight updates is the same, its macro'ized here:+ // Because the actual handling of the in-flight updates is the same, it's macro'ized here:
let mut pending_background_events = Vec::new();

TheBlueMatt · 2023-06-23T18:43:56Z

Oops, fixed one more bug @wpaulino pointed out that got introduced in one of the fixups:

$ git diff-tree -U1 cd76e97c e089a404
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 36260f1d2..69a69e7a1 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -865,3 +865,3 @@ pub(super) struct ChannelContext<Signer: ChannelSigner> {
/// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we
-	/// store it here and only release it to [`ChannelManager`] once it asks for it.+	/// store it here and only release it to the `ChannelManager` once it asks for it.
blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cff623caa..f38c6c13d 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -1923,3 +1923,3 @@ macro_rules! handle_new_monitor_update {
{
- in_flight_updates.pop();+ let _ = in_flight_updates.remove(idx);
if in_flight_updates.is_empty() && $chan.blocked_monitor_updates_pending() == 0 {

Because `ChannelMonitorUpdate`s can be generated for a channel which is already closed, and must still be tracked through their completion, storing them in a `Channel` doesn't make sense - we'd have to have a redundant place to put them post-closure and handle both storage locations equivalently. Instead, here, we move to storing in-flight `ChannelMonitorUpdate`s to the `ChannelManager`, leaving blocked `ChannelMonitorUpdate`s in the `Channel` as they were.

Now that all `ChannelMonitorUpdate`s stored in `Channel` are blocked we don't need a bool to track it.

`Channel::get_latest_complete_monitor_update_id` no longer refers to complete updates, but rather ones which were passed to the `ChannelManager` and which the `CHannel` no longer knows about. Thus, we rename it `get_latest_unblocked_monitor_update_id`.

To differentiate between in-flight pending completion and blocked updates.

TheBlueMatt · 2023-06-23T19:25:56Z

Grr, intermediate commit had intermediate state, had to shuffle around one line diff, but no total change:

$ git diff-tree -U1 e089a404 9c3ad28f
$

TheBlueMatt · 2023-06-19T06:33:36Z

Because ChannelMonitorUpdates can be generated for a
channel which is already closed, and must still be tracked
through their completion, storing them in a Channel
doesn't make sense - we'd have to have a redundant place to
put them post-closure and handle both storage locations
equivalently.

Instead, here, we move to storing in-flight
ChannelMonitorUpdates to the ChannelManager, leaving
blocked ChannelMonitorUpdates in the Channel as they
were.

This is the first (and largest) step towards #2168, and after this we're at least clear of the serialization-breaking parts, so could in theory cut a test version even without fully fixing 2168 (which needs fixing for 116 though).

Most of the work for #2317

codecov-commenter · 2023-06-20T02:24:06Z

Codecov Report

Patch coverage: 83.20% and project coverage change: -0.02⚠️

Comparison is base (15b1c9b) 90.30% compared to head (9c3ad28) 90.29%.

❗ 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 #2362 +/- ##
==========================================
- Coverage 90.30% 90.29% -0.02% 
==========================================
Files 106 106 Lines 54747 54920 +173 Branches 54747 54920 +173 ==========================================
+ Hits 49441 49591 +150 - Misses 5306 5329 +23

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`85.07% <0.00%> (-0.14%)`	⬇️
lightning/src/ln/channel.rs	`89.42% <83.33%> (-0.08%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.19% <83.96%> (-0.11%)`	⬇️

... and 13 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.

dunxen

Concept ACK. Just reviewing f52ed23 a bit more in-depth.

dunxen

LGTM. Can't see anything else glaringly wrong from my knowledge of monitor updates.

valentinewallace · 2023-06-21T18:32:35Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it only correct to call last() if we actually pushed an update above?

I mean if the vec contains something then it should also be safe?

Isn't it possible that we've already called update_channel with the last() monitor update, though?

Hmmmm, yea, maybe, I updated the code to handle it. We shouldn't be double-calling, but indeed I think there's maybe a case where we have two background updates to apply and we just apply the last() twice.

alecchendev

Was mostly just absorbing the context here, nothing outstanding to comment on

In the coming commits we'll move to storing in-flight `ChannelMonitorUpdate`s in the `ChannelManager` rather in the `Channel` (which will then only retain `ChannelMonitorUpdate`s which have not yet been released/are blocked. This will simplify handling of pending `ChannelMonitorUpdate` after a channel has closed by not having to move them into the `ChannelManager`.

Most of the calls to the `handle_new_monitor_update` macro had the exact same pattern - calling `update_monitor` followed by the macro. Given that common pattern will grow to first pushing the new monitor onto an in-flight set and then calling `update_monitor` unifying the pattern into a single macro now avoids more code churn in the coming commits.

By giving up on a tiny bit of parallelism and tweaking the return types, we can make the `handle_new_monitor_update` macro a bit clearer - now the only cases where its called after a monitor was updated was when the monitor was initially committed.

TheBlueMatt · 2023-06-21T22:52:43Z

Rebased.

valentinewallace · 2023-06-22T18:02:13Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it possible that we've already called update_channel with the last() monitor update, though?

valentinewallace

LGTM after this.

wpaulino

Feel free to squash.

wpaulino · 2023-06-23T18:01:46Z

- highest_applied_update_id, channel.get().context.get_latest_monitor_update_id());
+ let remaining_in_flight =
+ if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) {
+ pending.retain(|upd| upd.update_id > highest_applied_update_id);


If there are multiple updates in flight, would you call channel_monitor_updated once after they're all done, or after each one is done? If the latter, and they complete out of order, it seems this doesn't work?

Only after they're all done, this is implemented in ChainMonitor. We should probably move to doing it per-update, but we previously didn't track the updates in-flight so couldn't.

TheBlueMatt · 2023-06-23T18:24:35Z

Fixed spelling and squashed:

$ git diff-tree -U1 d18668b4 cd76e97c
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 942998840..cff623caa 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8399,3 +8399,3 @@ where
//
- // Because tha actual handling of the in-flight updates is the same, its macro'ized here:+ // Because the actual handling of the in-flight updates is the same, it's macro'ized here:
let mut pending_background_events = Vec::new();

TheBlueMatt · 2023-06-23T18:43:56Z

Oops, fixed one more bug @wpaulino pointed out that got introduced in one of the fixups:

$ git diff-tree -U1 cd76e97c e089a404
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 36260f1d2..69a69e7a1 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -865,3 +865,3 @@ pub(super) struct ChannelContext<Signer: ChannelSigner> {
/// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we
-	/// store it here and only release it to [`ChannelManager`] once it asks for it.+	/// store it here and only release it to the `ChannelManager` once it asks for it.
blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cff623caa..f38c6c13d 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -1923,3 +1923,3 @@ macro_rules! handle_new_monitor_update {
{
- in_flight_updates.pop();+ let _ = in_flight_updates.remove(idx);
if in_flight_updates.is_empty() && $chan.blocked_monitor_updates_pending() == 0 {

Because `ChannelMonitorUpdate`s can be generated for a channel which is already closed, and must still be tracked through their completion, storing them in a `Channel` doesn't make sense - we'd have to have a redundant place to put them post-closure and handle both storage locations equivalently. Instead, here, we move to storing in-flight `ChannelMonitorUpdate`s to the `ChannelManager`, leaving blocked `ChannelMonitorUpdate`s in the `Channel` as they were.

Now that all `ChannelMonitorUpdate`s stored in `Channel` are blocked we don't need a bool to track it.

`Channel::get_latest_complete_monitor_update_id` no longer refers to complete updates, but rather ones which were passed to the `ChannelManager` and which the `CHannel` no longer knows about. Thus, we rename it `get_latest_unblocked_monitor_update_id`.

To differentiate between in-flight pending completion and blocked updates.

TheBlueMatt · 2023-06-23T19:25:56Z

Grr, intermediate commit had intermediate state, had to shuffle around one line diff, but no total change:

$ git diff-tree -U1 e089a404 9c3ad28f
$

TheBlueMatt · 2023-06-19T06:33:36Z

Because ChannelMonitorUpdates can be generated for a
channel which is already closed, and must still be tracked
through their completion, storing them in a Channel
doesn't make sense - we'd have to have a redundant place to
put them post-closure and handle both storage locations
equivalently.

Instead, here, we move to storing in-flight
ChannelMonitorUpdates to the ChannelManager, leaving
blocked ChannelMonitorUpdates in the Channel as they
were.

This is the first (and largest) step towards #2168, and after this we're at least clear of the serialization-breaking parts, so could in theory cut a test version even without fully fixing 2168 (which needs fixing for 116 though).

Most of the work for #2317

codecov-commenter · 2023-06-20T02:24:06Z

Codecov Report

Patch coverage: 83.20% and project coverage change: -0.02⚠️

Comparison is base (15b1c9b) 90.30% compared to head (9c3ad28) 90.29%.

❗ 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 #2362 +/- ##
==========================================
- Coverage 90.30% 90.29% -0.02% 
==========================================
Files 106 106 Lines 54747 54920 +173 Branches 54747 54920 +173 ==========================================
+ Hits 49441 49591 +150 - Misses 5306 5329 +23

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`85.07% <0.00%> (-0.14%)`	⬇️
lightning/src/ln/channel.rs	`89.42% <83.33%> (-0.08%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.19% <83.96%> (-0.11%)`	⬇️

... and 13 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.

dunxen

Concept ACK. Just reviewing f52ed23 a bit more in-depth.

dunxen

LGTM. Can't see anything else glaringly wrong from my knowledge of monitor updates.

valentinewallace · 2023-06-21T18:32:35Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it only correct to call last() if we actually pushed an update above?

I mean if the vec contains something then it should also be safe?

Isn't it possible that we've already called update_channel with the last() monitor update, though?

Hmmmm, yea, maybe, I updated the code to handle it. We shouldn't be double-calling, but indeed I think there's maybe a case where we have two background updates to apply and we just apply the last() twice.

alecchendev

Was mostly just absorbing the context here, nothing outstanding to comment on

In the coming commits we'll move to storing in-flight `ChannelMonitorUpdate`s in the `ChannelManager` rather in the `Channel` (which will then only retain `ChannelMonitorUpdate`s which have not yet been released/are blocked. This will simplify handling of pending `ChannelMonitorUpdate` after a channel has closed by not having to move them into the `ChannelManager`.

Most of the calls to the `handle_new_monitor_update` macro had the exact same pattern - calling `update_monitor` followed by the macro. Given that common pattern will grow to first pushing the new monitor onto an in-flight set and then calling `update_monitor` unifying the pattern into a single macro now avoids more code churn in the coming commits.

By giving up on a tiny bit of parallelism and tweaking the return types, we can make the `handle_new_monitor_update` macro a bit clearer - now the only cases where its called after a monitor was updated was when the monitor was initially committed.

TheBlueMatt · 2023-06-21T22:52:43Z

Rebased.

valentinewallace · 2023-06-22T18:02:13Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it possible that we've already called update_channel with the last() monitor update, though?

valentinewallace

LGTM after this.

wpaulino

Feel free to squash.

wpaulino · 2023-06-23T18:01:46Z

- highest_applied_update_id, channel.get().context.get_latest_monitor_update_id());
+ let remaining_in_flight =
+ if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) {
+ pending.retain(|upd| upd.update_id > highest_applied_update_id);


If there are multiple updates in flight, would you call channel_monitor_updated once after they're all done, or after each one is done? If the latter, and they complete out of order, it seems this doesn't work?

Only after they're all done, this is implemented in ChainMonitor. We should probably move to doing it per-update, but we previously didn't track the updates in-flight so couldn't.

TheBlueMatt · 2023-06-23T18:24:35Z

Fixed spelling and squashed:

$ git diff-tree -U1 d18668b4 cd76e97c
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 942998840..cff623caa 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8399,3 +8399,3 @@ where
//
- // Because tha actual handling of the in-flight updates is the same, its macro'ized here:+ // Because the actual handling of the in-flight updates is the same, it's macro'ized here:
let mut pending_background_events = Vec::new();

TheBlueMatt · 2023-06-23T18:43:56Z

Oops, fixed one more bug @wpaulino pointed out that got introduced in one of the fixups:

$ git diff-tree -U1 cd76e97c e089a404
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 36260f1d2..69a69e7a1 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -865,3 +865,3 @@ pub(super) struct ChannelContext<Signer: ChannelSigner> {
/// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we
-	/// store it here and only release it to [`ChannelManager`] once it asks for it.+	/// store it here and only release it to the `ChannelManager` once it asks for it.
blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cff623caa..f38c6c13d 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -1923,3 +1923,3 @@ macro_rules! handle_new_monitor_update {
{
- in_flight_updates.pop();+ let _ = in_flight_updates.remove(idx);
if in_flight_updates.is_empty() && $chan.blocked_monitor_updates_pending() == 0 {

Because `ChannelMonitorUpdate`s can be generated for a channel which is already closed, and must still be tracked through their completion, storing them in a `Channel` doesn't make sense - we'd have to have a redundant place to put them post-closure and handle both storage locations equivalently. Instead, here, we move to storing in-flight `ChannelMonitorUpdate`s to the `ChannelManager`, leaving blocked `ChannelMonitorUpdate`s in the `Channel` as they were.

Now that all `ChannelMonitorUpdate`s stored in `Channel` are blocked we don't need a bool to track it.

`Channel::get_latest_complete_monitor_update_id` no longer refers to complete updates, but rather ones which were passed to the `ChannelManager` and which the `CHannel` no longer knows about. Thus, we rename it `get_latest_unblocked_monitor_update_id`.

To differentiate between in-flight pending completion and blocked updates.

TheBlueMatt · 2023-06-23T19:25:56Z

Grr, intermediate commit had intermediate state, had to shuffle around one line diff, but no total change:

$ git diff-tree -U1 e089a404 9c3ad28f
$

TheBlueMatt · 2023-06-19T06:33:36Z

Because ChannelMonitorUpdates can be generated for a
channel which is already closed, and must still be tracked
through their completion, storing them in a Channel
doesn't make sense - we'd have to have a redundant place to
put them post-closure and handle both storage locations
equivalently.

Instead, here, we move to storing in-flight
ChannelMonitorUpdates to the ChannelManager, leaving
blocked ChannelMonitorUpdates in the Channel as they
were.

This is the first (and largest) step towards #2168, and after this we're at least clear of the serialization-breaking parts, so could in theory cut a test version even without fully fixing 2168 (which needs fixing for 116 though).

Most of the work for #2317

codecov-commenter · 2023-06-20T02:24:06Z

Codecov Report

Patch coverage: 83.20% and project coverage change: -0.02⚠️

Comparison is base (15b1c9b) 90.30% compared to head (9c3ad28) 90.29%.

❗ 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 #2362 +/- ##
==========================================
- Coverage 90.30% 90.29% -0.02% 
==========================================
Files 106 106 Lines 54747 54920 +173 Branches 54747 54920 +173 ==========================================
+ Hits 49441 49591 +150 - Misses 5306 5329 +23

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`85.07% <0.00%> (-0.14%)`	⬇️
lightning/src/ln/channel.rs	`89.42% <83.33%> (-0.08%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.19% <83.96%> (-0.11%)`	⬇️

... and 13 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.

dunxen

Concept ACK. Just reviewing f52ed23 a bit more in-depth.

dunxen

LGTM. Can't see anything else glaringly wrong from my knowledge of monitor updates.

valentinewallace · 2023-06-21T18:32:35Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it only correct to call last() if we actually pushed an update above?

I mean if the vec contains something then it should also be safe?

Isn't it possible that we've already called update_channel with the last() monitor update, though?

Hmmmm, yea, maybe, I updated the code to handle it. We shouldn't be double-calling, but indeed I think there's maybe a case where we have two background updates to apply and we just apply the last() twice.

alecchendev

Was mostly just absorbing the context here, nothing outstanding to comment on

In the coming commits we'll move to storing in-flight `ChannelMonitorUpdate`s in the `ChannelManager` rather in the `Channel` (which will then only retain `ChannelMonitorUpdate`s which have not yet been released/are blocked. This will simplify handling of pending `ChannelMonitorUpdate` after a channel has closed by not having to move them into the `ChannelManager`.

Most of the calls to the `handle_new_monitor_update` macro had the exact same pattern - calling `update_monitor` followed by the macro. Given that common pattern will grow to first pushing the new monitor onto an in-flight set and then calling `update_monitor` unifying the pattern into a single macro now avoids more code churn in the coming commits.

By giving up on a tiny bit of parallelism and tweaking the return types, we can make the `handle_new_monitor_update` macro a bit clearer - now the only cases where its called after a monitor was updated was when the monitor was initially committed.

TheBlueMatt · 2023-06-21T22:52:43Z

Rebased.

valentinewallace · 2023-06-22T18:02:13Z

+ // filter for uniqueness here.
+ in_flight_updates.push($update);
+ }
+ let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap());


Isn't it possible that we've already called update_channel with the last() monitor update, though?

valentinewallace

LGTM after this.

wpaulino

Feel free to squash.

wpaulino · 2023-06-23T18:01:46Z

- highest_applied_update_id, channel.get().context.get_latest_monitor_update_id());
+ let remaining_in_flight =
+ if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) {
+ pending.retain(|upd| upd.update_id > highest_applied_update_id);


If there are multiple updates in flight, would you call channel_monitor_updated once after they're all done, or after each one is done? If the latter, and they complete out of order, it seems this doesn't work?

Only after they're all done, this is implemented in ChainMonitor. We should probably move to doing it per-update, but we previously didn't track the updates in-flight so couldn't.

TheBlueMatt · 2023-06-23T18:24:35Z

Fixed spelling and squashed:

$ git diff-tree -U1 d18668b4 cd76e97c
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 942998840..cff623caa 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -8399,3 +8399,3 @@ where
//
- // Because tha actual handling of the in-flight updates is the same, its macro'ized here:+ // Because the actual handling of the in-flight updates is the same, it's macro'ized here:
let mut pending_background_events = Vec::new();

TheBlueMatt · 2023-06-23T18:43:56Z

Oops, fixed one more bug @wpaulino pointed out that got introduced in one of the fixups:

$ git diff-tree -U1 cd76e97c e089a404
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 36260f1d2..69a69e7a1 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -865,3 +865,3 @@ pub(super) struct ChannelContext<Signer: ChannelSigner> {
/// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we
-	/// store it here and only release it to [`ChannelManager`] once it asks for it.+	/// store it here and only release it to the `ChannelManager` once it asks for it.
blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cff623caa..f38c6c13d 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -1923,3 +1923,3 @@ macro_rules! handle_new_monitor_update {
{
- in_flight_updates.pop();+ let _ = in_flight_updates.remove(idx);
if in_flight_updates.is_empty() && $chan.blocked_monitor_updates_pending() == 0 {

Because `ChannelMonitorUpdate`s can be generated for a channel which is already closed, and must still be tracked through their completion, storing them in a `Channel` doesn't make sense - we'd have to have a redundant place to put them post-closure and handle both storage locations equivalently. Instead, here, we move to storing in-flight `ChannelMonitorUpdate`s to the `ChannelManager`, leaving blocked `ChannelMonitorUpdate`s in the `Channel` as they were.

Now that all `ChannelMonitorUpdate`s stored in `Channel` are blocked we don't need a bool to track it.

`Channel::get_latest_complete_monitor_update_id` no longer refers to complete updates, but rather ones which were passed to the `ChannelManager` and which the `CHannel` no longer knows about. Thus, we rename it `get_latest_unblocked_monitor_update_id`.

To differentiate between in-flight pending completion and blocked updates.

TheBlueMatt · 2023-06-23T19:25:56Z

Grr, intermediate commit had intermediate state, had to shuffle around one line diff, but no total change:

$ git diff-tree -U1 e089a404 9c3ad28f
$

TheBlueMatt added this to the 0.0.116 milestone Jun 19, 2023

TheBlueMatt linked an issue Jun 19, 2023 that may be closed by this pull request
Make Channel::pending_monitor_updates TLV even #2317
Closed

TheBlueMatt commented Jun 19, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from 486a65e to f732734Compare June 20, 2023 02:23

This was referenced Jun 20, 2023
Block the mon update removing a preimage until upstream mon writes #2169
Merged
Re-claim forwarded HTLCs on startup #2364
Merged

TheBlueMatt added the Seeking Code Review label Jun 20, 2023

dunxen reviewed Jun 20, 2023
View reviewed changes

TheBlueMatt removed a link to an issue Jun 20, 2023
Make Channel::pending_monitor_updates TLV even #2317
Closed

TheBlueMatt modified the milestones: 0.0.116, 0.0.116alphaJun 20, 2023

wpaulino reviewed Jun 20, 2023
View reviewed changes

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch 4 times, most recently from 32a31af to 3fe91e1Compare June 21, 2023 00:06

dunxen reviewed Jun 21, 2023
View reviewed changes

valentinewallace reviewed Jun 21, 2023
View reviewed changes

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from 3fe91e1 to e721025Compare June 21, 2023 20:55

alecchendev reviewed Jun 21, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs

TheBlueMatt added 4 commits June 21, 2023 22:37

Drop the now-unused push_blockable_mon_update
5e528ff

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from e721025 to 2704bf3Compare June 21, 2023 22:52

valentinewallace reviewed Jun 22, 2023
View reviewed changes

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from 2704bf3 to 653c606Compare June 22, 2023 20:49

valentinewallace reviewed Jun 23, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from 653c606 to d18668bCompare June 23, 2023 17:53

wpaulino reviewed Jun 23, 2023
View reviewed changes

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from d18668b to cd76e97Compare June 23, 2023 18:24

wpaulino previously approved these changes Jun 23, 2023
View reviewed changes

valentinewallace previously approved these changes Jun 23, 2023
View reviewed changes

TheBlueMatt dismissed stale reviews from valentinewallace and wpaulino via e089a40June 23, 2023 18:43

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from cd76e97 to e089a40Compare June 23, 2023 18:43

wpaulino approved these changes Jun 23, 2023
View reviewed changes

valentinewallace approved these changes Jun 23, 2023
View reviewed changes

TheBlueMatt added 5 commits June 23, 2023 19:25

Drop the now-unused update_id param to monitor update macros
e186593

Remove the blocked param on ChannelMonitorUpdates in Channel
60e671b
Now that all `ChannelMonitorUpdate`s stored in `Channel` are blocked we don't need a bool to track it.

Rename Channel::pending_monitor_updates to blocked
9c3ad28
To differentiate between in-flight pending completion and blocked updates.

TheBlueMatt force-pushed the 2023-06-unblocked-mons-in-manager branch from e089a40 to 9c3ad28Compare June 23, 2023 19:25

wpaulino approved these changes Jun 23, 2023
View reviewed changes

valentinewallace approved these changes Jun 23, 2023
View reviewed changes

TheBlueMatt merged commit f833448 into lightningdevkit:mainJun 23, 2023

TheBlueMatt modified the milestones: 0.0.116alpha, 0.0.116 Jun 24, 2023