Skip to content

Handle pre-startup and closed-channel monitor update completion actions - #2391

Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom
TheBlueMatt:2023-07-all-compl-actions
Jul 12, 2023
Merged

Handle pre-startup and closed-channel monitor update completion actions#2391
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom
TheBlueMatt:2023-07-all-compl-actions

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Jul 5, 2023

Copy link
Copy Markdown
Collaborator

This handles two cases that were teed up by, but not fixed in, #2167. Built on #2364 and should be roughly the last of the fixes needed for async monitor completion until 117.

@TheBlueMattTheBlueMatt added this to the 0.0.116 milestone Jul 5, 2023
@TheBlueMatt

TheBlueMatt commented Jul 5, 2023

Copy link
Copy Markdown
CollaboratorAuthor

Tests in the last few commits in #2169 though still need to write some more comments in one.

@TheBlueMatt
TheBlueMattforce-pushed the 2023-07-all-compl-actions branch from d97a168 to d2b4778CompareJuly 5, 2023 17:13
@TheBlueMattTheBlueMatt mentioned this pull request Jul 5, 2023
Closed
jkczyz
jkczyz reviewed Jul 6, 2023
View reviewed changes
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs Outdated
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
Comment on lines 2366 to 2367
// disconnect the peers. Note that the fuzzer originally found this issue because
// deserializing a ChannelManager in this state causes an assertion failure.

@jkczyzjkczyzJul 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't quite clear what issue the fuzzer had found before and if it would still find it after moving the initialization of chan_0_monitor_serialized.

@TheBlueMattTheBlueMattJul 10, 2023

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when the test was written we had no concept for in-flight monitor updates - we expected the user to (magically) call monitor_update_completed on the monitor update that they'd previously marked in-flight after startup. With the change here to finally actually "complete" monitor updates that completed while we were shut down, we have to do something to mark the monitor updates as actually-not-complete, which we do by loading with the old monitor.

@wpaulino

wpaulino commented Jul 10, 2023

Copy link
Copy Markdown
Contributor

Needs rebase.

@TheBlueMatt
TheBlueMattforce-pushed the 2023-07-all-compl-actions branch from d2b4778 to 8900ef1CompareJuly 10, 2023 21:03
@TheBlueMatt

TheBlueMatt commented Jul 10, 2023

Copy link
Copy Markdown
CollaboratorAuthor

Rebased and addressed feedback.

@TheBlueMatt
Handle monitor completion actions for closed channels
d450e0f
If a channel has been closed, there may still be some
`ChannelMonitorUpdate`(s) which are pending completion. These
in-flight updates may also be blocking another channel from letting
an update fly, e.g. for forwarded payments where the payment
preimage will be removed from the downstream channel after the
upstream channel has closed.
Luckily all the infrastructure to handle this case is already in
place - we just need to process the
`monitor_update_blocked_actions` for closed channels.
@TheBlueMatt
TheBlueMattforce-pushed the 2023-07-all-compl-actions branch from 8900ef1 to 76f331eCompareJuly 10, 2023 21:40
alecchendev
alecchendev reviewed Jul 11, 2023
View reviewed changes

@alecchendevalecchendev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM

Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
valentinewallace
valentinewallace reviewed Jul 11, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2023-07-all-compl-actions branch from 76f331e to 9f5b70eCompareJuly 11, 2023 19:56
@codecov-commenter

codecov-commenter commented Jul 11, 2023•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Patch coverage: 37.50% and project coverage change: +0.26 🎉

Comparison is base (4c342bd) 90.31% compared to head (9f5b70e) 90.58%.

❗ 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 #2391 +/- ##
==========================================
+ Coverage 90.31% 90.58% +0.26% 
==========================================
Files 106 106 Lines 55166 57314 +2148 Branches 55166 57314 +2148 ==========================================
+ Hits 49824 51917 +2093 - Misses 5342 5397 +55
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs88.67% <24.24%> (+2.72%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️

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

valentinewallace
valentinewallace reviewed Jul 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
update: update.clone(),
});
}
if $chan_in_flight_upds.is_empty() {

@valentinewallacevalentinewallaceJul 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check if any updates were actually removed before generating the background event?

@TheBlueMattTheBlueMattJul 12, 2023

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only get here if there were some updates in the in_flight_monitor_updates map, which is only filled in with updates for channels with in-flight updates during serialization.

valentinewallace
valentinewallace reviewed Jul 12, 2023
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after squash

@TheBlueMatt
Run monitor update completion actions for pre-startup completion
f9521a4
If a `ChannelMonitorUpdate` completes being persisted, but the
`ChannelManager` isn't informed thereof (or isn't persisted) before
shutdown, on startup we may still have it listed as in-flight. When
we compare the available `ChannelMonitor` with the in-flight set,
we'll notice it completed and remove it, however this may leave
some post-update actions dangling which need to complete.
Here we handle this with a new `BackgroundEvent` indicating we need
to handle any post-update action(s) for a given channel.
@TheBlueMatt
Add comment describing when a completion action can be discarded
550cf91
In an older PR a reviewer had asked why the discarding of a channel
being blocked on another monitor update is okay if the blocked
channel has since closed. At the time, this was not actually okay -
the monitor updates in the channel weren't moved to the
`ChannelManager` on close so the whole pipeline was busted, but
with the changes in 4041f08 the
handling of channel closes with pending monitor updates is now
correct, and so is the existing code block.
@TheBlueMatt
TheBlueMattforce-pushed the 2023-07-all-compl-actions branch from 9f5b70e to 550cf91CompareJuly 12, 2023 20:53
@TheBlueMatt

TheBlueMatt commented Jul 12, 2023

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without change:

$ git diff-tree -U1 9f5b70ea 550cf914
$

@TheBlueMatt
TheBlueMatt merged commit df237ba into lightningdevkit:mainJul 12, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes
+1 more reviewer
@alecchendevalecchendevalecchendev left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.0.116

Development

Successfully merging this pull request may close these issues.

6 participants

@TheBlueMatt@wpaulino@codecov-commenter@jkczyz@valentinewallace@alecchendev
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Handle pre-startup and closed-channel monitor update completion actions by TheBlueMatt · Pull Request #2391 · lightningdevkit/rust-lightning · GitHub
Skip to content
Handle pre-startup and closed-channel monitor update completion actions - #2391
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Jul 12, 2023
Merged
Handle pre-startup and closed-channel monitor update completion actions#2391
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

Collaborator
This handles two cases that were teed up by, but not fixed in, #2167. Built on #2364 and should be roughly the last of the fixes needed for async monitor completion until 117.
@TheBlueMattTheBlueMatt
added this to the 0.0.116 milestone
Jul 5, 2023
@TheBlueMatt
TheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Tests in the last few commits in #2169 though still need to write some more comments in one.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d97a168 to
d2b4778CompareJuly 5, 2023 17:13
@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 5, 2023

Closed

jkczyz
jkczyz
reviewed
Jul 6, 2023
View reviewed changes
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
Outdated

Comment threadlightning/src/ln/chanmon_update_fail_tests.rs

Comment on lines

2366
 to 
2367

 // disconnect the peers. Note that the fuzzer originally found this issue because
 // deserializing a ChannelManager in this state causes an assertion failure.
@jkczyzjkczyzJul 6, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't quite clear what issue the fuzzer had found before and if it would still find it after moving the initialization of chan_0_monitor_serialized.
@TheBlueMattTheBlueMattJul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when the test was written we had no concept for in-flight monitor updates - we expected the user to (magically) call monitor_update_completed on the monitor update that they'd previously marked in-flight after startup. With the change here to finally actually "complete" monitor updates that completed while we were shut down, we have to do something to mark the monitor updates as actually-not-complete, which we do by loading with the old monitor.
@wpaulino
wpaulino
commented
Jul 10, 2023

Copy link


Copy Markdown

Contributor
Needs rebase.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d2b4778 to
8900ef1CompareJuly 10, 2023 21:03
@TheBlueMatt
TheBlueMatt
commented
Jul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Rebased and addressed feedback.
@TheBlueMatt
Handle monitor completion actions for closed channels
d450e0f
If a channel has been closed, there may still be some
`ChannelMonitorUpdate`(s) which are pending completion. These
in-flight updates may also be blocking another channel from letting
an update fly, e.g. for forwarded payments where the payment
preimage will be removed from the downstream channel after the
upstream channel has closed.
Luckily all the infrastructure to handle this case is already in
place - we just need to process the
`monitor_update_blocked_actions` for closed channels.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
8900ef1 to
76f331eCompareJuly 10, 2023 21:40
alecchendev
alecchendev
reviewed
Jul 11, 2023
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
valentinewallace
valentinewallace
reviewed
Jul 11, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
76f331e to
9f5b70eCompareJuly 11, 2023 19:56
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 37.50% and project coverage change: +0.26 🎉
Comparison is base (4c342bd) 90.31% compared to head (9f5b70e) 90.58%.
❗ 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 #2391 +/- ##
==========================================
+ Coverage 90.31% 90.58% +0.26% 
==========================================
Files 106 106 Lines 55166 57314 +2148 Branches 55166 57314 +2148 ==========================================
+ Hits 49824 51917 +2093 - Misses 5342 5397 +55 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs88.67% <24.24%> (+2.72%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
... and 4 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.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 update: update.clone(),
 });
 }
 if $chan_in_flight_upds.is_empty() {
@valentinewallacevalentinewallaceJul 12, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check if any updates were actually removed before generating the background event?
@TheBlueMattTheBlueMattJul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only get here if there were some updates in the in_flight_monitor_updates map, which is only filled in with updates for channels with in-flight updates during serialization.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after squash
@TheBlueMatt
Run monitor update completion actions for pre-startup completion
f9521a4
If a `ChannelMonitorUpdate` completes being persisted, but the
`ChannelManager` isn't informed thereof (or isn't persisted) before
shutdown, on startup we may still have it listed as in-flight. When
we compare the available `ChannelMonitor` with the in-flight set,
we'll notice it completed and remove it, however this may leave
some post-update actions dangling which need to complete.
Here we handle this with a new `BackgroundEvent` indicating we need
to handle any post-update action(s) for a given channel.
@TheBlueMatt
Add comment describing when a completion action can be discarded
550cf91
In an older PR a reviewer had asked why the discarding of a channel
being blocked on another monitor update is okay if the blocked
channel has since closed. At the time, this was not actually okay -
the monitor updates in the channel weren't moved to the
`ChannelManager` on close so the whole pipeline was busted, but
with the changes in 4041f08 the
handling of channel closes with pending monitor updates is now
correct, and so is the existing code block.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
9f5b70e to
550cf91CompareJuly 12, 2023 20:53
@TheBlueMatt
TheBlueMatt
commented
Jul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without change:
$ git diff-tree -U1 9f5b70ea 550cf914
$ 
@TheBlueMatt
TheBlueMatt
merged commit df237ba
into
lightningdevkit:mainJul 12, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

+1 more reviewer

@alecchendevalecchendevalecchendev left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.116

Development

Successfully merging this pull request may close these issues.

6 participants

@TheBlueMatt@wpaulino@codecov-commenter@jkczyz@valentinewallace@alecchendev

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Handle pre-startup and closed-channel monitor update completion actions by TheBlueMatt · Pull Request #2391 · lightningdevkit/rust-lightning · GitHub
Skip to content
Handle pre-startup and closed-channel monitor update completion actions - #2391
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Jul 12, 2023
Merged
Handle pre-startup and closed-channel monitor update completion actions#2391
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

Collaborator
This handles two cases that were teed up by, but not fixed in, #2167. Built on #2364 and should be roughly the last of the fixes needed for async monitor completion until 117.
@TheBlueMattTheBlueMatt
added this to the 0.0.116 milestone
Jul 5, 2023
@TheBlueMatt
TheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Tests in the last few commits in #2169 though still need to write some more comments in one.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d97a168 to
d2b4778CompareJuly 5, 2023 17:13
@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 5, 2023

Closed

jkczyz
jkczyz
reviewed
Jul 6, 2023
View reviewed changes
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
Outdated

Comment threadlightning/src/ln/chanmon_update_fail_tests.rs

Comment on lines

2366
 to 
2367

 // disconnect the peers. Note that the fuzzer originally found this issue because
 // deserializing a ChannelManager in this state causes an assertion failure.
@jkczyzjkczyzJul 6, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't quite clear what issue the fuzzer had found before and if it would still find it after moving the initialization of chan_0_monitor_serialized.
@TheBlueMattTheBlueMattJul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when the test was written we had no concept for in-flight monitor updates - we expected the user to (magically) call monitor_update_completed on the monitor update that they'd previously marked in-flight after startup. With the change here to finally actually "complete" monitor updates that completed while we were shut down, we have to do something to mark the monitor updates as actually-not-complete, which we do by loading with the old monitor.
@wpaulino
wpaulino
commented
Jul 10, 2023

Copy link


Copy Markdown

Contributor
Needs rebase.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d2b4778 to
8900ef1CompareJuly 10, 2023 21:03
@TheBlueMatt
TheBlueMatt
commented
Jul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Rebased and addressed feedback.
@TheBlueMatt
Handle monitor completion actions for closed channels
d450e0f
If a channel has been closed, there may still be some
`ChannelMonitorUpdate`(s) which are pending completion. These
in-flight updates may also be blocking another channel from letting
an update fly, e.g. for forwarded payments where the payment
preimage will be removed from the downstream channel after the
upstream channel has closed.
Luckily all the infrastructure to handle this case is already in
place - we just need to process the
`monitor_update_blocked_actions` for closed channels.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
8900ef1 to
76f331eCompareJuly 10, 2023 21:40
alecchendev
alecchendev
reviewed
Jul 11, 2023
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
valentinewallace
valentinewallace
reviewed
Jul 11, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
76f331e to
9f5b70eCompareJuly 11, 2023 19:56
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 37.50% and project coverage change: +0.26 🎉
Comparison is base (4c342bd) 90.31% compared to head (9f5b70e) 90.58%.
❗ 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 #2391 +/- ##
==========================================
+ Coverage 90.31% 90.58% +0.26% 
==========================================
Files 106 106 Lines 55166 57314 +2148 Branches 55166 57314 +2148 ==========================================
+ Hits 49824 51917 +2093 - Misses 5342 5397 +55 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs88.67% <24.24%> (+2.72%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
... and 4 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.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 update: update.clone(),
 });
 }
 if $chan_in_flight_upds.is_empty() {
@valentinewallacevalentinewallaceJul 12, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check if any updates were actually removed before generating the background event?
@TheBlueMattTheBlueMattJul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only get here if there were some updates in the in_flight_monitor_updates map, which is only filled in with updates for channels with in-flight updates during serialization.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after squash
@TheBlueMatt
Run monitor update completion actions for pre-startup completion
f9521a4
If a `ChannelMonitorUpdate` completes being persisted, but the
`ChannelManager` isn't informed thereof (or isn't persisted) before
shutdown, on startup we may still have it listed as in-flight. When
we compare the available `ChannelMonitor` with the in-flight set,
we'll notice it completed and remove it, however this may leave
some post-update actions dangling which need to complete.
Here we handle this with a new `BackgroundEvent` indicating we need
to handle any post-update action(s) for a given channel.
@TheBlueMatt
Add comment describing when a completion action can be discarded
550cf91
In an older PR a reviewer had asked why the discarding of a channel
being blocked on another monitor update is okay if the blocked
channel has since closed. At the time, this was not actually okay -
the monitor updates in the channel weren't moved to the
`ChannelManager` on close so the whole pipeline was busted, but
with the changes in 4041f08 the
handling of channel closes with pending monitor updates is now
correct, and so is the existing code block.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
9f5b70e to
550cf91CompareJuly 12, 2023 20:53
@TheBlueMatt
TheBlueMatt
commented
Jul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without change:
$ git diff-tree -U1 9f5b70ea 550cf914
$ 
@TheBlueMatt
TheBlueMatt
merged commit df237ba
into
lightningdevkit:mainJul 12, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

+1 more reviewer

@alecchendevalecchendevalecchendev left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.116

Development

Successfully merging this pull request may close these issues.

6 participants

@TheBlueMatt@wpaulino@codecov-commenter@jkczyz@valentinewallace@alecchendev

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Handle pre-startup and closed-channel monitor update completion actions by TheBlueMatt · Pull Request #2391 · lightningdevkit/rust-lightning · GitHub
Skip to content
Handle pre-startup and closed-channel monitor update completion actions - #2391
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Jul 12, 2023
Merged
Handle pre-startup and closed-channel monitor update completion actions#2391
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

Collaborator
This handles two cases that were teed up by, but not fixed in, #2167. Built on #2364 and should be roughly the last of the fixes needed for async monitor completion until 117.
@TheBlueMattTheBlueMatt
added this to the 0.0.116 milestone
Jul 5, 2023
@TheBlueMatt
TheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Tests in the last few commits in #2169 though still need to write some more comments in one.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d97a168 to
d2b4778CompareJuly 5, 2023 17:13
@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 5, 2023

Closed

jkczyz
jkczyz
reviewed
Jul 6, 2023
View reviewed changes
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
Outdated

Comment threadlightning/src/ln/chanmon_update_fail_tests.rs

Comment on lines

2366
 to 
2367

 // disconnect the peers. Note that the fuzzer originally found this issue because
 // deserializing a ChannelManager in this state causes an assertion failure.
@jkczyzjkczyzJul 6, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't quite clear what issue the fuzzer had found before and if it would still find it after moving the initialization of chan_0_monitor_serialized.
@TheBlueMattTheBlueMattJul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when the test was written we had no concept for in-flight monitor updates - we expected the user to (magically) call monitor_update_completed on the monitor update that they'd previously marked in-flight after startup. With the change here to finally actually "complete" monitor updates that completed while we were shut down, we have to do something to mark the monitor updates as actually-not-complete, which we do by loading with the old monitor.
@wpaulino
wpaulino
commented
Jul 10, 2023

Copy link


Copy Markdown

Contributor
Needs rebase.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d2b4778 to
8900ef1CompareJuly 10, 2023 21:03
@TheBlueMatt
TheBlueMatt
commented
Jul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Rebased and addressed feedback.
@TheBlueMatt
Handle monitor completion actions for closed channels
d450e0f
If a channel has been closed, there may still be some
`ChannelMonitorUpdate`(s) which are pending completion. These
in-flight updates may also be blocking another channel from letting
an update fly, e.g. for forwarded payments where the payment
preimage will be removed from the downstream channel after the
upstream channel has closed.
Luckily all the infrastructure to handle this case is already in
place - we just need to process the
`monitor_update_blocked_actions` for closed channels.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
8900ef1 to
76f331eCompareJuly 10, 2023 21:40
alecchendev
alecchendev
reviewed
Jul 11, 2023
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
valentinewallace
valentinewallace
reviewed
Jul 11, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
76f331e to
9f5b70eCompareJuly 11, 2023 19:56
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 37.50% and project coverage change: +0.26 🎉
Comparison is base (4c342bd) 90.31% compared to head (9f5b70e) 90.58%.
❗ 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 #2391 +/- ##
==========================================
+ Coverage 90.31% 90.58% +0.26% 
==========================================
Files 106 106 Lines 55166 57314 +2148 Branches 55166 57314 +2148 ==========================================
+ Hits 49824 51917 +2093 - Misses 5342 5397 +55 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs88.67% <24.24%> (+2.72%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
... and 4 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.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 update: update.clone(),
 });
 }
 if $chan_in_flight_upds.is_empty() {
@valentinewallacevalentinewallaceJul 12, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check if any updates were actually removed before generating the background event?
@TheBlueMattTheBlueMattJul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only get here if there were some updates in the in_flight_monitor_updates map, which is only filled in with updates for channels with in-flight updates during serialization.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after squash
@TheBlueMatt
Run monitor update completion actions for pre-startup completion
f9521a4
If a `ChannelMonitorUpdate` completes being persisted, but the
`ChannelManager` isn't informed thereof (or isn't persisted) before
shutdown, on startup we may still have it listed as in-flight. When
we compare the available `ChannelMonitor` with the in-flight set,
we'll notice it completed and remove it, however this may leave
some post-update actions dangling which need to complete.
Here we handle this with a new `BackgroundEvent` indicating we need
to handle any post-update action(s) for a given channel.
@TheBlueMatt
Add comment describing when a completion action can be discarded
550cf91
In an older PR a reviewer had asked why the discarding of a channel
being blocked on another monitor update is okay if the blocked
channel has since closed. At the time, this was not actually okay -
the monitor updates in the channel weren't moved to the
`ChannelManager` on close so the whole pipeline was busted, but
with the changes in 4041f08 the
handling of channel closes with pending monitor updates is now
correct, and so is the existing code block.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
9f5b70e to
550cf91CompareJuly 12, 2023 20:53
@TheBlueMatt
TheBlueMatt
commented
Jul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without change:
$ git diff-tree -U1 9f5b70ea 550cf914
$ 
@TheBlueMatt
TheBlueMatt
merged commit df237ba
into
lightningdevkit:mainJul 12, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

+1 more reviewer

@alecchendevalecchendevalecchendev left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.116

Development

Successfully merging this pull request may close these issues.

6 participants

@TheBlueMatt@wpaulino@codecov-commenter@jkczyz@valentinewallace@alecchendev

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Handle pre-startup and closed-channel monitor update completion actions by TheBlueMatt · Pull Request #2391 · lightningdevkit/rust-lightning · GitHub
Skip to content
Handle pre-startup and closed-channel monitor update completion actions - #2391
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Jul 12, 2023
Merged
Handle pre-startup and closed-channel monitor update completion actions#2391
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

Collaborator
This handles two cases that were teed up by, but not fixed in, #2167. Built on #2364 and should be roughly the last of the fixes needed for async monitor completion until 117.
@TheBlueMattTheBlueMatt
added this to the 0.0.116 milestone
Jul 5, 2023
@TheBlueMatt
TheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Tests in the last few commits in #2169 though still need to write some more comments in one.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d97a168 to
d2b4778CompareJuly 5, 2023 17:13
@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 5, 2023

Closed

jkczyz
jkczyz
reviewed
Jul 6, 2023
View reviewed changes
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
Outdated

Comment threadlightning/src/ln/chanmon_update_fail_tests.rs

Comment on lines

2366
 to 
2367

 // disconnect the peers. Note that the fuzzer originally found this issue because
 // deserializing a ChannelManager in this state causes an assertion failure.
@jkczyzjkczyzJul 6, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't quite clear what issue the fuzzer had found before and if it would still find it after moving the initialization of chan_0_monitor_serialized.
@TheBlueMattTheBlueMattJul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when the test was written we had no concept for in-flight monitor updates - we expected the user to (magically) call monitor_update_completed on the monitor update that they'd previously marked in-flight after startup. With the change here to finally actually "complete" monitor updates that completed while we were shut down, we have to do something to mark the monitor updates as actually-not-complete, which we do by loading with the old monitor.
@wpaulino
wpaulino
commented
Jul 10, 2023

Copy link


Copy Markdown

Contributor
Needs rebase.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d2b4778 to
8900ef1CompareJuly 10, 2023 21:03
@TheBlueMatt
TheBlueMatt
commented
Jul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Rebased and addressed feedback.
@TheBlueMatt
Handle monitor completion actions for closed channels
d450e0f
If a channel has been closed, there may still be some
`ChannelMonitorUpdate`(s) which are pending completion. These
in-flight updates may also be blocking another channel from letting
an update fly, e.g. for forwarded payments where the payment
preimage will be removed from the downstream channel after the
upstream channel has closed.
Luckily all the infrastructure to handle this case is already in
place - we just need to process the
`monitor_update_blocked_actions` for closed channels.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
8900ef1 to
76f331eCompareJuly 10, 2023 21:40
alecchendev
alecchendev
reviewed
Jul 11, 2023
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
valentinewallace
valentinewallace
reviewed
Jul 11, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
76f331e to
9f5b70eCompareJuly 11, 2023 19:56
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 37.50% and project coverage change: +0.26 🎉
Comparison is base (4c342bd) 90.31% compared to head (9f5b70e) 90.58%.
❗ 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 #2391 +/- ##
==========================================
+ Coverage 90.31% 90.58% +0.26% 
==========================================
Files 106 106 Lines 55166 57314 +2148 Branches 55166 57314 +2148 ==========================================
+ Hits 49824 51917 +2093 - Misses 5342 5397 +55 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs88.67% <24.24%> (+2.72%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
... and 4 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.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 update: update.clone(),
 });
 }
 if $chan_in_flight_upds.is_empty() {
@valentinewallacevalentinewallaceJul 12, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check if any updates were actually removed before generating the background event?
@TheBlueMattTheBlueMattJul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only get here if there were some updates in the in_flight_monitor_updates map, which is only filled in with updates for channels with in-flight updates during serialization.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after squash
@TheBlueMatt
Run monitor update completion actions for pre-startup completion
f9521a4
If a `ChannelMonitorUpdate` completes being persisted, but the
`ChannelManager` isn't informed thereof (or isn't persisted) before
shutdown, on startup we may still have it listed as in-flight. When
we compare the available `ChannelMonitor` with the in-flight set,
we'll notice it completed and remove it, however this may leave
some post-update actions dangling which need to complete.
Here we handle this with a new `BackgroundEvent` indicating we need
to handle any post-update action(s) for a given channel.
@TheBlueMatt
Add comment describing when a completion action can be discarded
550cf91
In an older PR a reviewer had asked why the discarding of a channel
being blocked on another monitor update is okay if the blocked
channel has since closed. At the time, this was not actually okay -
the monitor updates in the channel weren't moved to the
`ChannelManager` on close so the whole pipeline was busted, but
with the changes in 4041f08 the
handling of channel closes with pending monitor updates is now
correct, and so is the existing code block.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
9f5b70e to
550cf91CompareJuly 12, 2023 20:53
@TheBlueMatt
TheBlueMatt
commented
Jul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without change:
$ git diff-tree -U1 9f5b70ea 550cf914
$ 
@TheBlueMatt
TheBlueMatt
merged commit df237ba
into
lightningdevkit:mainJul 12, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

+1 more reviewer

@alecchendevalecchendevalecchendev left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.116

Development

Successfully merging this pull request may close these issues.

6 participants

@TheBlueMatt@wpaulino@codecov-commenter@jkczyz@valentinewallace@alecchendev

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Handle pre-startup and closed-channel monitor update completion actions by TheBlueMatt · Pull Request #2391 · lightningdevkit/rust-lightning · GitHub
Skip to content
Handle pre-startup and closed-channel monitor update completion actions - #2391
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Jul 12, 2023
Merged
Handle pre-startup and closed-channel monitor update completion actions#2391
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

Collaborator
This handles two cases that were teed up by, but not fixed in, #2167. Built on #2364 and should be roughly the last of the fixes needed for async monitor completion until 117.
@TheBlueMattTheBlueMatt
added this to the 0.0.116 milestone
Jul 5, 2023
@TheBlueMatt
TheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Tests in the last few commits in #2169 though still need to write some more comments in one.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d97a168 to
d2b4778CompareJuly 5, 2023 17:13
@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 5, 2023

Closed

jkczyz
jkczyz
reviewed
Jul 6, 2023
View reviewed changes
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
Outdated

Comment threadlightning/src/ln/chanmon_update_fail_tests.rs

Comment on lines

2366
 to 
2367

 // disconnect the peers. Note that the fuzzer originally found this issue because
 // deserializing a ChannelManager in this state causes an assertion failure.
@jkczyzjkczyzJul 6, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't quite clear what issue the fuzzer had found before and if it would still find it after moving the initialization of chan_0_monitor_serialized.
@TheBlueMattTheBlueMattJul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when the test was written we had no concept for in-flight monitor updates - we expected the user to (magically) call monitor_update_completed on the monitor update that they'd previously marked in-flight after startup. With the change here to finally actually "complete" monitor updates that completed while we were shut down, we have to do something to mark the monitor updates as actually-not-complete, which we do by loading with the old monitor.
@wpaulino
wpaulino
commented
Jul 10, 2023

Copy link


Copy Markdown

Contributor
Needs rebase.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d2b4778 to
8900ef1CompareJuly 10, 2023 21:03
@TheBlueMatt
TheBlueMatt
commented
Jul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Rebased and addressed feedback.
@TheBlueMatt
Handle monitor completion actions for closed channels
d450e0f
If a channel has been closed, there may still be some
`ChannelMonitorUpdate`(s) which are pending completion. These
in-flight updates may also be blocking another channel from letting
an update fly, e.g. for forwarded payments where the payment
preimage will be removed from the downstream channel after the
upstream channel has closed.
Luckily all the infrastructure to handle this case is already in
place - we just need to process the
`monitor_update_blocked_actions` for closed channels.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
8900ef1 to
76f331eCompareJuly 10, 2023 21:40
alecchendev
alecchendev
reviewed
Jul 11, 2023
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
valentinewallace
valentinewallace
reviewed
Jul 11, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
76f331e to
9f5b70eCompareJuly 11, 2023 19:56
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 37.50% and project coverage change: +0.26 🎉
Comparison is base (4c342bd) 90.31% compared to head (9f5b70e) 90.58%.
❗ 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 #2391 +/- ##
==========================================
+ Coverage 90.31% 90.58% +0.26% 
==========================================
Files 106 106 Lines 55166 57314 +2148 Branches 55166 57314 +2148 ==========================================
+ Hits 49824 51917 +2093 - Misses 5342 5397 +55 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs88.67% <24.24%> (+2.72%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
... and 4 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.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 update: update.clone(),
 });
 }
 if $chan_in_flight_upds.is_empty() {
@valentinewallacevalentinewallaceJul 12, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check if any updates were actually removed before generating the background event?
@TheBlueMattTheBlueMattJul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only get here if there were some updates in the in_flight_monitor_updates map, which is only filled in with updates for channels with in-flight updates during serialization.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after squash
@TheBlueMatt
Run monitor update completion actions for pre-startup completion
f9521a4
If a `ChannelMonitorUpdate` completes being persisted, but the
`ChannelManager` isn't informed thereof (or isn't persisted) before
shutdown, on startup we may still have it listed as in-flight. When
we compare the available `ChannelMonitor` with the in-flight set,
we'll notice it completed and remove it, however this may leave
some post-update actions dangling which need to complete.
Here we handle this with a new `BackgroundEvent` indicating we need
to handle any post-update action(s) for a given channel.
@TheBlueMatt
Add comment describing when a completion action can be discarded
550cf91
In an older PR a reviewer had asked why the discarding of a channel
being blocked on another monitor update is okay if the blocked
channel has since closed. At the time, this was not actually okay -
the monitor updates in the channel weren't moved to the
`ChannelManager` on close so the whole pipeline was busted, but
with the changes in 4041f08 the
handling of channel closes with pending monitor updates is now
correct, and so is the existing code block.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
9f5b70e to
550cf91CompareJuly 12, 2023 20:53
@TheBlueMatt
TheBlueMatt
commented
Jul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without change:
$ git diff-tree -U1 9f5b70ea 550cf914
$ 
@TheBlueMatt
TheBlueMatt
merged commit df237ba
into
lightningdevkit:mainJul 12, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

+1 more reviewer

@alecchendevalecchendevalecchendev left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.116

Development

Successfully merging this pull request may close these issues.

6 participants

@TheBlueMatt@wpaulino@codecov-commenter@jkczyz@valentinewallace@alecchendev

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Handle pre-startup and closed-channel monitor update completion actions by TheBlueMatt · Pull Request #2391 · lightningdevkit/rust-lightning · GitHub
Skip to content
Handle pre-startup and closed-channel monitor update completion actions - #2391
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Jul 12, 2023
Merged
Handle pre-startup and closed-channel monitor update completion actions#2391
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

Collaborator
This handles two cases that were teed up by, but not fixed in, #2167. Built on #2364 and should be roughly the last of the fixes needed for async monitor completion until 117.
@TheBlueMattTheBlueMatt
added this to the 0.0.116 milestone
Jul 5, 2023
@TheBlueMatt
TheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Tests in the last few commits in #2169 though still need to write some more comments in one.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d97a168 to
d2b4778CompareJuly 5, 2023 17:13
@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 5, 2023

Closed

jkczyz
jkczyz
reviewed
Jul 6, 2023
View reviewed changes
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
Outdated

Comment threadlightning/src/ln/chanmon_update_fail_tests.rs

Comment on lines

2366
 to 
2367

 // disconnect the peers. Note that the fuzzer originally found this issue because
 // deserializing a ChannelManager in this state causes an assertion failure.
@jkczyzjkczyzJul 6, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't quite clear what issue the fuzzer had found before and if it would still find it after moving the initialization of chan_0_monitor_serialized.
@TheBlueMattTheBlueMattJul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when the test was written we had no concept for in-flight monitor updates - we expected the user to (magically) call monitor_update_completed on the monitor update that they'd previously marked in-flight after startup. With the change here to finally actually "complete" monitor updates that completed while we were shut down, we have to do something to mark the monitor updates as actually-not-complete, which we do by loading with the old monitor.
@wpaulino
wpaulino
commented
Jul 10, 2023

Copy link


Copy Markdown

Contributor
Needs rebase.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d2b4778 to
8900ef1CompareJuly 10, 2023 21:03
@TheBlueMatt
TheBlueMatt
commented
Jul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Rebased and addressed feedback.
@TheBlueMatt
Handle monitor completion actions for closed channels
d450e0f
If a channel has been closed, there may still be some
`ChannelMonitorUpdate`(s) which are pending completion. These
in-flight updates may also be blocking another channel from letting
an update fly, e.g. for forwarded payments where the payment
preimage will be removed from the downstream channel after the
upstream channel has closed.
Luckily all the infrastructure to handle this case is already in
place - we just need to process the
`monitor_update_blocked_actions` for closed channels.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
8900ef1 to
76f331eCompareJuly 10, 2023 21:40
alecchendev
alecchendev
reviewed
Jul 11, 2023
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
valentinewallace
valentinewallace
reviewed
Jul 11, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
76f331e to
9f5b70eCompareJuly 11, 2023 19:56
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 37.50% and project coverage change: +0.26 🎉
Comparison is base (4c342bd) 90.31% compared to head (9f5b70e) 90.58%.
❗ 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 #2391 +/- ##
==========================================
+ Coverage 90.31% 90.58% +0.26% 
==========================================
Files 106 106 Lines 55166 57314 +2148 Branches 55166 57314 +2148 ==========================================
+ Hits 49824 51917 +2093 - Misses 5342 5397 +55 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs88.67% <24.24%> (+2.72%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
... and 4 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.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 update: update.clone(),
 });
 }
 if $chan_in_flight_upds.is_empty() {
@valentinewallacevalentinewallaceJul 12, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check if any updates were actually removed before generating the background event?
@TheBlueMattTheBlueMattJul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only get here if there were some updates in the in_flight_monitor_updates map, which is only filled in with updates for channels with in-flight updates during serialization.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after squash
@TheBlueMatt
Run monitor update completion actions for pre-startup completion
f9521a4
If a `ChannelMonitorUpdate` completes being persisted, but the
`ChannelManager` isn't informed thereof (or isn't persisted) before
shutdown, on startup we may still have it listed as in-flight. When
we compare the available `ChannelMonitor` with the in-flight set,
we'll notice it completed and remove it, however this may leave
some post-update actions dangling which need to complete.
Here we handle this with a new `BackgroundEvent` indicating we need
to handle any post-update action(s) for a given channel.
@TheBlueMatt
Add comment describing when a completion action can be discarded
550cf91
In an older PR a reviewer had asked why the discarding of a channel
being blocked on another monitor update is okay if the blocked
channel has since closed. At the time, this was not actually okay -
the monitor updates in the channel weren't moved to the
`ChannelManager` on close so the whole pipeline was busted, but
with the changes in 4041f08 the
handling of channel closes with pending monitor updates is now
correct, and so is the existing code block.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
9f5b70e to
550cf91CompareJuly 12, 2023 20:53
@TheBlueMatt
TheBlueMatt
commented
Jul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without change:
$ git diff-tree -U1 9f5b70ea 550cf914
$ 
@TheBlueMatt
TheBlueMatt
merged commit df237ba
into
lightningdevkit:mainJul 12, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

+1 more reviewer

@alecchendevalecchendevalecchendev left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.116

Development

Successfully merging this pull request may close these issues.

6 participants

@TheBlueMatt@wpaulino@codecov-commenter@jkczyz@valentinewallace@alecchendev

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
Handle pre-startup and closed-channel monitor update completion actions by TheBlueMatt · Pull Request #2391 · lightningdevkit/rust-lightning · GitHub
Skip to content
Handle pre-startup and closed-channel monitor update completion actions - #2391
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Jul 12, 2023
Merged
Handle pre-startup and closed-channel monitor update completion actions#2391
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2023-07-all-compl-actions
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

Collaborator
This handles two cases that were teed up by, but not fixed in, #2167. Built on #2364 and should be roughly the last of the fixes needed for async monitor completion until 117.
@TheBlueMattTheBlueMatt
added this to the 0.0.116 milestone
Jul 5, 2023
@TheBlueMatt
TheBlueMatt
commented
Jul 5, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Tests in the last few commits in #2169 though still need to write some more comments in one.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d97a168 to
d2b4778CompareJuly 5, 2023 17:13
@TheBlueMattTheBlueMatt
mentioned this pull request
Jul 5, 2023

Closed

jkczyz
jkczyz
reviewed
Jul 6, 2023
View reviewed changes
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
Outdated

Comment threadlightning/src/ln/chanmon_update_fail_tests.rs

Comment on lines

2366
 to 
2367

 // disconnect the peers. Note that the fuzzer originally found this issue because
 // deserializing a ChannelManager in this state causes an assertion failure.
@jkczyzjkczyzJul 6, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't quite clear what issue the fuzzer had found before and if it would still find it after moving the initialization of chan_0_monitor_serialized.
@TheBlueMattTheBlueMattJul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when the test was written we had no concept for in-flight monitor updates - we expected the user to (magically) call monitor_update_completed on the monitor update that they'd previously marked in-flight after startup. With the change here to finally actually "complete" monitor updates that completed while we were shut down, we have to do something to mark the monitor updates as actually-not-complete, which we do by loading with the old monitor.
@wpaulino
wpaulino
commented
Jul 10, 2023

Copy link


Copy Markdown

Contributor
Needs rebase.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
d2b4778 to
8900ef1CompareJuly 10, 2023 21:03
@TheBlueMatt
TheBlueMatt
commented
Jul 10, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Rebased and addressed feedback.
@TheBlueMatt
Handle monitor completion actions for closed channels
d450e0f
If a channel has been closed, there may still be some
`ChannelMonitorUpdate`(s) which are pending completion. These
in-flight updates may also be blocking another channel from letting
an update fly, e.g. for forwarded payments where the payment
preimage will be removed from the downstream channel after the
upstream channel has closed.
Luckily all the infrastructure to handle this case is already in
place - we just need to process the
`monitor_update_blocked_actions` for closed channels.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
8900ef1 to
76f331eCompareJuly 10, 2023 21:40
alecchendev
alecchendev
reviewed
Jul 11, 2023
View reviewed changes
@alecchendevalecchendev
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM
Comment threadlightning/src/ln/chanmon_update_fail_tests.rs
valentinewallace
valentinewallace
reviewed
Jul 11, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
76f331e to
9f5b70eCompareJuly 11, 2023 19:56
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 37.50% and project coverage change: +0.26 🎉
Comparison is base (4c342bd) 90.31% compared to head (9f5b70e) 90.58%.
❗ 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 #2391 +/- ##
==========================================
+ Coverage 90.31% 90.58% +0.26% 
==========================================
Files 106 106 Lines 55166 57314 +2148 Branches 55166 57314 +2148 ==========================================
+ Hits 49824 51917 +2093 - Misses 5342 5397 +55 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs88.67% <24.24%> (+2.72%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
... and 4 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.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
 update: update.clone(),
 });
 }
 if $chan_in_flight_upds.is_empty() {
@valentinewallacevalentinewallaceJul 12, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check if any updates were actually removed before generating the background event?
@TheBlueMattTheBlueMattJul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only get here if there were some updates in the in_flight_monitor_updates map, which is only filled in with updates for channels with in-flight updates during serialization.
valentinewallace
valentinewallace
reviewed
Jul 12, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after squash
@TheBlueMatt
Run monitor update completion actions for pre-startup completion
f9521a4
If a `ChannelMonitorUpdate` completes being persisted, but the
`ChannelManager` isn't informed thereof (or isn't persisted) before
shutdown, on startup we may still have it listed as in-flight. When
we compare the available `ChannelMonitor` with the in-flight set,
we'll notice it completed and remove it, however this may leave
some post-update actions dangling which need to complete.
Here we handle this with a new `BackgroundEvent` indicating we need
to handle any post-update action(s) for a given channel.
@TheBlueMatt
Add comment describing when a completion action can be discarded
550cf91
In an older PR a reviewer had asked why the discarding of a channel
being blocked on another monitor update is okay if the blocked
channel has since closed. At the time, this was not actually okay -
the monitor updates in the channel weren't moved to the
`ChannelManager` on close so the whole pipeline was busted, but
with the changes in 4041f08 the
handling of channel closes with pending monitor updates is now
correct, and so is the existing code block.
@TheBlueMatt
TheBlueMattforce-pushed
the
2023-07-all-compl-actions
branch
from
9f5b70e to
550cf91CompareJuly 12, 2023 20:53
@TheBlueMatt
TheBlueMatt
commented
Jul 12, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without change:
$ git diff-tree -U1 9f5b70ea 550cf914
$ 
@TheBlueMatt
TheBlueMatt
merged commit df237ba
into
lightningdevkit:mainJul 12, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

+1 more reviewer

@alecchendevalecchendevalecchendev left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.116

Development

Successfully merging this pull request may close these issues.

6 participants

@TheBlueMatt@wpaulino@codecov-commenter@jkczyz@valentinewallace@alecchendev