Move in-flight ChannelMonitorUpdates to ChannelManager #2362
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TheBlueMatt
commented
Jun 19, 2023
TheBlueMatt
force-pushed
the
2023-06-unblocked-mons-in-manager
branch
from
486a65e
to
f732734
Compare
Codecov ReportPatch coverage:
❗ 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
☔ View full report in Codecov by Sentry. |
This was referenced Jun 20, 2023
wpaulino
reviewed
Jun 20, 2023
TheBlueMatt
force-pushed
the
2023-06-unblocked-mons-in-manager
branch
4 times, most recently
from
32a31af
to
3fe91e1
Compare
dunxen
reviewed
Jun 21, 2023
lightning/src/ln/channelmanager.rs
Outdated
| // filter for uniqueness here. | ||
| in_flight_updates.push($update); | ||
| } | ||
| let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap()); |
There was a problem hiding this comment.
Isn't it only correct to call last() if we actually pushed an update above?
There was a problem hiding this comment.
I mean if the vec contains something then it should also be safe?
There was a problem hiding this comment.
Isn't it possible that we've already called update_channel with the last() monitor update, though?
There was a problem hiding this comment.
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.
TheBlueMatt
force-pushed
the
2023-06-unblocked-mons-in-manager
branch
from
3fe91e1
to
e721025
Compare
alecchendev
reviewed
Jun 21, 2023
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
force-pushed
the
2023-06-unblocked-mons-in-manager
branch
from
e721025
to
2704bf3
Compare
|
Rebased. |
lightning/src/ln/channelmanager.rs
Outdated
| // filter for uniqueness here. | ||
| in_flight_updates.push($update); | ||
| } | ||
| let update_res = $self.chain_monitor.update_channel($funding_txo, in_flight_updates.last().unwrap()); |
There was a problem hiding this comment.
Isn't it possible that we've already called update_channel with the last() monitor update, though?
TheBlueMatt
force-pushed
the
2023-06-unblocked-mons-in-manager
branch
from
2704bf3
to
653c606
Compare
TheBlueMatt
force-pushed
the
2023-06-unblocked-mons-in-manager
branch
from
653c606
to
d18668b
Compare
wpaulino
reviewed
Jun 23, 2023
| 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
force-pushed
the
2023-06-unblocked-mons-in-manager
branch
from
d18668b
to
cd76e97
Compare
|
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(); |
wpaulino
previously approved these changes
Jun 23, 2023
valentinewallace
previously approved these changes
Jun 23, 2023
TheBlueMatt
dismissed stale reviews from valentinewallace and wpaulino
via
e089a40
TheBlueMatt
force-pushed
the
2023-06-unblocked-mons-in-manager
branch
from
cd76e97
to
e089a40
Compare
|
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 { |
wpaulino
approved these changes
Jun 23, 2023
valentinewallace
approved these changes
Jun 23, 2023
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
force-pushed
the
2023-06-unblocked-mons-in-manager
branch
from
e089a40
to
9c3ad28
Compare
|
Grr, intermediate commit had intermediate state, had to shuffle around one line diff, but no total change: $ git diff-tree -U1 e089a404 9c3ad28f
$ |
wpaulino
approved these changes
Jun 23, 2023
valentinewallace
approved these changes
Jun 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Because
ChannelMonitorUpdates can be generated for achannel which is already closed, and must still be tracked
through their completion, storing them in a
Channeldoesn'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 theChannelManager, leavingblocked
ChannelMonitorUpdates in theChannelas theywere.
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