Fuzz reloading with a stale monitor in chanmon_consistency - #3113

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz
Jun 12, 2024
Merged

Fuzz reloading with a stale monitor in chanmon_consistency#3113
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Now that we are gearing up to support fully async monitor storage, we really need to fuzz monitor updates not completing before a reload, which we do here in the chanmon_consistency fuzzer.

While there are more parts to async monitor updating that we need to fuzz, this at least gets us started by having basic async restart cases handled. In the future, we should extend this to make sure some basic properties (eg claim/balance consistency) remain true through chanmon_consistency runs.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from fd0ecbd to 7446a79CompareJune 10, 2024 21:01
@codecov-commenter

codecov-commenter commented Jun 10, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 97.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.65%. Comparing base (1d0c6c6) to head (2a0c900).
Report is 15 commits behind head on main.

FilesPatch %Lines
lightning/src/ln/chanmon_update_fail_tests.rs97.46%2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@ Coverage Diff @@## main #3113 +/- ##
==========================================
+ Coverage 89.84% 90.65% +0.80% 
==========================================
Files 119 119 Lines 97811 102825 +5014 Branches 97811 102825 +5014 ==========================================
+ Hits 87883 93217 +5334 + Misses 7364 7108 -256 + Partials 2564 2500 -64 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch 2 times, most recently from b7a57bc to 61aef9bCompareJune 10, 2024 21:46

@tnulltnull 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, just a few questions/comments.

Comment threadfuzz/src/chanmon_consistency.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
&|v: &mut Vec<_>| if !v.is_empty() { Some(v.remove(0)) } else { None }),
0xf1 =>
complete_monitor_update(&monitor_a, &chan_1_funding,
&|v: &mut Vec<_>| if v.len() > 1 { Some(v.remove(1)) } else { None }),

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.

Not quite sure why we alternate between 0 and 1 here? Would we gain anything in terms of coverage by choosing randomly from 0..len?

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 can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that, but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

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.

We can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that

Right, I imagined the latter.

but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

Yeah, that's why I thought covering the entire Vec might be worth it, but maybe it's not too important.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 3a944d2 to c05e508CompareJune 11, 2024 13:57
Comment threadlightning/src/ln/channelmanager.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from c05e508 to 9d0c1a1CompareJune 12, 2024 02:26
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Addressed feedback.

@tnull

tnull commented Jun 12, 2024

Copy link
Copy Markdown
Contributor

Addressed feedback.

Unfortunately the new test now reliably fails:

thread 'ln::chanmon_update_fail_tests::test_sync_async_persist_doesnt_hang' panicked at 'explicit panic', lightning/src/ln/chanmon_update_fail_tests.rs:3410:14

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 384d691 to 2a0c900CompareJune 12, 2024 14:11
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Ugh, sorry, pushed right before bed last night and didn't test enough :/

@valentinewallace

Copy link
Copy Markdown
Contributor

LGTM, feel free to squash IMO.

Now that we are gearing up to support fully async monitor storage,
we really need to fuzz monitor updates not completing before a
reload, which we do here in the `chanmon_consistency` fuzzer.
While there are more parts to async monitor updating that we need
to fuzz, this at least gets us started by having basic async
restart cases handled. In the future, we should extend this to make
sure some basic properties (eg claim/balance consistency) remain
true through `chanmon_consistency` runs.
When we have `ChannelMonitorUpdate`s which are completing both
synchronously and asynchronously, we need to consider a channel as
unblocked based on the `ChannelManager` monitor update queue,
rather than by checking the `update_id`s.
Consider the case where a channel is updated, leading to a
`ChannelMonitorUpdate` which completes asynchronously. The update
completes, but prior to the `ChannelManager` receiving the
`MonitorEvent::Completed` it generates a further
`ChannelMonitorUpdate`. This second update completes synchronously.
As a result, when the `MonitorEvent` is processed, the event's
`monitor_update_id` is the first update, but there are no updates
queued and the channel should be free to return to be unblocked.
Here we fix this by looking only at the `ChannelManager` update
queue, rather than the update_id of the `MonitorEvent`.
While we don't anticipate many users having both synchronous and
asynchronous persists in the same application, there isn't much
cost to supporting it, which we do here.
Found by the chanmon_consistency target.
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 2a0c900 to 920d96eCompareJune 12, 2024 15:32
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@tnull
tnull merged commit 5e3056e into lightningdevkit:mainJun 12, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@tnull@valentinewallace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fuzz reloading with a stale monitor in chanmon_consistency - #3113

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz
Jun 12, 2024
Merged

Fuzz reloading with a stale monitor in chanmon_consistency#3113
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Now that we are gearing up to support fully async monitor storage, we really need to fuzz monitor updates not completing before a reload, which we do here in the chanmon_consistency fuzzer.

While there are more parts to async monitor updating that we need to fuzz, this at least gets us started by having basic async restart cases handled. In the future, we should extend this to make sure some basic properties (eg claim/balance consistency) remain true through chanmon_consistency runs.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from fd0ecbd to 7446a79CompareJune 10, 2024 21:01
@codecov-commenter

codecov-commenter commented Jun 10, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 97.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.65%. Comparing base (1d0c6c6) to head (2a0c900).
Report is 15 commits behind head on main.

FilesPatch %Lines
lightning/src/ln/chanmon_update_fail_tests.rs97.46%2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@ Coverage Diff @@## main #3113 +/- ##
==========================================
+ Coverage 89.84% 90.65% +0.80% 
==========================================
Files 119 119 Lines 97811 102825 +5014 Branches 97811 102825 +5014 ==========================================
+ Hits 87883 93217 +5334 + Misses 7364 7108 -256 + Partials 2564 2500 -64 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch 2 times, most recently from b7a57bc to 61aef9bCompareJune 10, 2024 21:46

@tnulltnull 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, just a few questions/comments.

Comment threadfuzz/src/chanmon_consistency.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
&|v: &mut Vec<_>| if !v.is_empty() { Some(v.remove(0)) } else { None }),
0xf1 =>
complete_monitor_update(&monitor_a, &chan_1_funding,
&|v: &mut Vec<_>| if v.len() > 1 { Some(v.remove(1)) } else { None }),

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.

Not quite sure why we alternate between 0 and 1 here? Would we gain anything in terms of coverage by choosing randomly from 0..len?

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 can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that, but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

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.

We can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that

Right, I imagined the latter.

but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

Yeah, that's why I thought covering the entire Vec might be worth it, but maybe it's not too important.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 3a944d2 to c05e508CompareJune 11, 2024 13:57
Comment threadlightning/src/ln/channelmanager.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from c05e508 to 9d0c1a1CompareJune 12, 2024 02:26
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Addressed feedback.

@tnull

tnull commented Jun 12, 2024

Copy link
Copy Markdown
Contributor

Addressed feedback.

Unfortunately the new test now reliably fails:

thread 'ln::chanmon_update_fail_tests::test_sync_async_persist_doesnt_hang' panicked at 'explicit panic', lightning/src/ln/chanmon_update_fail_tests.rs:3410:14

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 384d691 to 2a0c900CompareJune 12, 2024 14:11
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Ugh, sorry, pushed right before bed last night and didn't test enough :/

@valentinewallace

Copy link
Copy Markdown
Contributor

LGTM, feel free to squash IMO.

Now that we are gearing up to support fully async monitor storage,
we really need to fuzz monitor updates not completing before a
reload, which we do here in the `chanmon_consistency` fuzzer.
While there are more parts to async monitor updating that we need
to fuzz, this at least gets us started by having basic async
restart cases handled. In the future, we should extend this to make
sure some basic properties (eg claim/balance consistency) remain
true through `chanmon_consistency` runs.
When we have `ChannelMonitorUpdate`s which are completing both
synchronously and asynchronously, we need to consider a channel as
unblocked based on the `ChannelManager` monitor update queue,
rather than by checking the `update_id`s.
Consider the case where a channel is updated, leading to a
`ChannelMonitorUpdate` which completes asynchronously. The update
completes, but prior to the `ChannelManager` receiving the
`MonitorEvent::Completed` it generates a further
`ChannelMonitorUpdate`. This second update completes synchronously.
As a result, when the `MonitorEvent` is processed, the event's
`monitor_update_id` is the first update, but there are no updates
queued and the channel should be free to return to be unblocked.
Here we fix this by looking only at the `ChannelManager` update
queue, rather than the update_id of the `MonitorEvent`.
While we don't anticipate many users having both synchronous and
asynchronous persists in the same application, there isn't much
cost to supporting it, which we do here.
Found by the chanmon_consistency target.
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 2a0c900 to 920d96eCompareJune 12, 2024 15:32
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@tnull
tnull merged commit 5e3056e into lightningdevkit:mainJun 12, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@tnull@valentinewallace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fuzz reloading with a stale monitor in chanmon_consistency - #3113

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz
Jun 12, 2024
Merged

Fuzz reloading with a stale monitor in chanmon_consistency#3113
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Now that we are gearing up to support fully async monitor storage, we really need to fuzz monitor updates not completing before a reload, which we do here in the chanmon_consistency fuzzer.

While there are more parts to async monitor updating that we need to fuzz, this at least gets us started by having basic async restart cases handled. In the future, we should extend this to make sure some basic properties (eg claim/balance consistency) remain true through chanmon_consistency runs.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from fd0ecbd to 7446a79CompareJune 10, 2024 21:01
@codecov-commenter

codecov-commenter commented Jun 10, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 97.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.65%. Comparing base (1d0c6c6) to head (2a0c900).
Report is 15 commits behind head on main.

FilesPatch %Lines
lightning/src/ln/chanmon_update_fail_tests.rs97.46%2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@ Coverage Diff @@## main #3113 +/- ##
==========================================
+ Coverage 89.84% 90.65% +0.80% 
==========================================
Files 119 119 Lines 97811 102825 +5014 Branches 97811 102825 +5014 ==========================================
+ Hits 87883 93217 +5334 + Misses 7364 7108 -256 + Partials 2564 2500 -64 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch 2 times, most recently from b7a57bc to 61aef9bCompareJune 10, 2024 21:46

@tnulltnull 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, just a few questions/comments.

Comment threadfuzz/src/chanmon_consistency.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
&|v: &mut Vec<_>| if !v.is_empty() { Some(v.remove(0)) } else { None }),
0xf1 =>
complete_monitor_update(&monitor_a, &chan_1_funding,
&|v: &mut Vec<_>| if v.len() > 1 { Some(v.remove(1)) } else { None }),

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.

Not quite sure why we alternate between 0 and 1 here? Would we gain anything in terms of coverage by choosing randomly from 0..len?

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 can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that, but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

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.

We can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that

Right, I imagined the latter.

but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

Yeah, that's why I thought covering the entire Vec might be worth it, but maybe it's not too important.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 3a944d2 to c05e508CompareJune 11, 2024 13:57
Comment threadlightning/src/ln/channelmanager.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from c05e508 to 9d0c1a1CompareJune 12, 2024 02:26
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Addressed feedback.

@tnull

tnull commented Jun 12, 2024

Copy link
Copy Markdown
Contributor

Addressed feedback.

Unfortunately the new test now reliably fails:

thread 'ln::chanmon_update_fail_tests::test_sync_async_persist_doesnt_hang' panicked at 'explicit panic', lightning/src/ln/chanmon_update_fail_tests.rs:3410:14

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 384d691 to 2a0c900CompareJune 12, 2024 14:11
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Ugh, sorry, pushed right before bed last night and didn't test enough :/

@valentinewallace

Copy link
Copy Markdown
Contributor

LGTM, feel free to squash IMO.

Now that we are gearing up to support fully async monitor storage,
we really need to fuzz monitor updates not completing before a
reload, which we do here in the `chanmon_consistency` fuzzer.
While there are more parts to async monitor updating that we need
to fuzz, this at least gets us started by having basic async
restart cases handled. In the future, we should extend this to make
sure some basic properties (eg claim/balance consistency) remain
true through `chanmon_consistency` runs.
When we have `ChannelMonitorUpdate`s which are completing both
synchronously and asynchronously, we need to consider a channel as
unblocked based on the `ChannelManager` monitor update queue,
rather than by checking the `update_id`s.
Consider the case where a channel is updated, leading to a
`ChannelMonitorUpdate` which completes asynchronously. The update
completes, but prior to the `ChannelManager` receiving the
`MonitorEvent::Completed` it generates a further
`ChannelMonitorUpdate`. This second update completes synchronously.
As a result, when the `MonitorEvent` is processed, the event's
`monitor_update_id` is the first update, but there are no updates
queued and the channel should be free to return to be unblocked.
Here we fix this by looking only at the `ChannelManager` update
queue, rather than the update_id of the `MonitorEvent`.
While we don't anticipate many users having both synchronous and
asynchronous persists in the same application, there isn't much
cost to supporting it, which we do here.
Found by the chanmon_consistency target.
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 2a0c900 to 920d96eCompareJune 12, 2024 15:32
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@tnull
tnull merged commit 5e3056e into lightningdevkit:mainJun 12, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@tnull@valentinewallace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fuzz reloading with a stale monitor in chanmon_consistency - #3113

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz
Jun 12, 2024
Merged

Fuzz reloading with a stale monitor in chanmon_consistency#3113
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Now that we are gearing up to support fully async monitor storage, we really need to fuzz monitor updates not completing before a reload, which we do here in the chanmon_consistency fuzzer.

While there are more parts to async monitor updating that we need to fuzz, this at least gets us started by having basic async restart cases handled. In the future, we should extend this to make sure some basic properties (eg claim/balance consistency) remain true through chanmon_consistency runs.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from fd0ecbd to 7446a79CompareJune 10, 2024 21:01
@codecov-commenter

codecov-commenter commented Jun 10, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 97.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.65%. Comparing base (1d0c6c6) to head (2a0c900).
Report is 15 commits behind head on main.

FilesPatch %Lines
lightning/src/ln/chanmon_update_fail_tests.rs97.46%2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@ Coverage Diff @@## main #3113 +/- ##
==========================================
+ Coverage 89.84% 90.65% +0.80% 
==========================================
Files 119 119 Lines 97811 102825 +5014 Branches 97811 102825 +5014 ==========================================
+ Hits 87883 93217 +5334 + Misses 7364 7108 -256 + Partials 2564 2500 -64 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch 2 times, most recently from b7a57bc to 61aef9bCompareJune 10, 2024 21:46

@tnulltnull 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, just a few questions/comments.

Comment threadfuzz/src/chanmon_consistency.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
&|v: &mut Vec<_>| if !v.is_empty() { Some(v.remove(0)) } else { None }),
0xf1 =>
complete_monitor_update(&monitor_a, &chan_1_funding,
&|v: &mut Vec<_>| if v.len() > 1 { Some(v.remove(1)) } else { None }),

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.

Not quite sure why we alternate between 0 and 1 here? Would we gain anything in terms of coverage by choosing randomly from 0..len?

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 can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that, but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

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.

We can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that

Right, I imagined the latter.

but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

Yeah, that's why I thought covering the entire Vec might be worth it, but maybe it's not too important.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 3a944d2 to c05e508CompareJune 11, 2024 13:57
Comment threadlightning/src/ln/channelmanager.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from c05e508 to 9d0c1a1CompareJune 12, 2024 02:26
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Addressed feedback.

@tnull

tnull commented Jun 12, 2024

Copy link
Copy Markdown
Contributor

Addressed feedback.

Unfortunately the new test now reliably fails:

thread 'ln::chanmon_update_fail_tests::test_sync_async_persist_doesnt_hang' panicked at 'explicit panic', lightning/src/ln/chanmon_update_fail_tests.rs:3410:14

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 384d691 to 2a0c900CompareJune 12, 2024 14:11
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Ugh, sorry, pushed right before bed last night and didn't test enough :/

@valentinewallace

Copy link
Copy Markdown
Contributor

LGTM, feel free to squash IMO.

Now that we are gearing up to support fully async monitor storage,
we really need to fuzz monitor updates not completing before a
reload, which we do here in the `chanmon_consistency` fuzzer.
While there are more parts to async monitor updating that we need
to fuzz, this at least gets us started by having basic async
restart cases handled. In the future, we should extend this to make
sure some basic properties (eg claim/balance consistency) remain
true through `chanmon_consistency` runs.
When we have `ChannelMonitorUpdate`s which are completing both
synchronously and asynchronously, we need to consider a channel as
unblocked based on the `ChannelManager` monitor update queue,
rather than by checking the `update_id`s.
Consider the case where a channel is updated, leading to a
`ChannelMonitorUpdate` which completes asynchronously. The update
completes, but prior to the `ChannelManager` receiving the
`MonitorEvent::Completed` it generates a further
`ChannelMonitorUpdate`. This second update completes synchronously.
As a result, when the `MonitorEvent` is processed, the event's
`monitor_update_id` is the first update, but there are no updates
queued and the channel should be free to return to be unblocked.
Here we fix this by looking only at the `ChannelManager` update
queue, rather than the update_id of the `MonitorEvent`.
While we don't anticipate many users having both synchronous and
asynchronous persists in the same application, there isn't much
cost to supporting it, which we do here.
Found by the chanmon_consistency target.
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 2a0c900 to 920d96eCompareJune 12, 2024 15:32
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@tnull
tnull merged commit 5e3056e into lightningdevkit:mainJun 12, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@tnull@valentinewallace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fuzz reloading with a stale monitor in chanmon_consistency - #3113

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz
Jun 12, 2024
Merged

Fuzz reloading with a stale monitor in chanmon_consistency#3113
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Now that we are gearing up to support fully async monitor storage, we really need to fuzz monitor updates not completing before a reload, which we do here in the chanmon_consistency fuzzer.

While there are more parts to async monitor updating that we need to fuzz, this at least gets us started by having basic async restart cases handled. In the future, we should extend this to make sure some basic properties (eg claim/balance consistency) remain true through chanmon_consistency runs.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from fd0ecbd to 7446a79CompareJune 10, 2024 21:01
@codecov-commenter

codecov-commenter commented Jun 10, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 97.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.65%. Comparing base (1d0c6c6) to head (2a0c900).
Report is 15 commits behind head on main.

FilesPatch %Lines
lightning/src/ln/chanmon_update_fail_tests.rs97.46%2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@ Coverage Diff @@## main #3113 +/- ##
==========================================
+ Coverage 89.84% 90.65% +0.80% 
==========================================
Files 119 119 Lines 97811 102825 +5014 Branches 97811 102825 +5014 ==========================================
+ Hits 87883 93217 +5334 + Misses 7364 7108 -256 + Partials 2564 2500 -64 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch 2 times, most recently from b7a57bc to 61aef9bCompareJune 10, 2024 21:46

@tnulltnull 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, just a few questions/comments.

Comment threadfuzz/src/chanmon_consistency.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
&|v: &mut Vec<_>| if !v.is_empty() { Some(v.remove(0)) } else { None }),
0xf1 =>
complete_monitor_update(&monitor_a, &chan_1_funding,
&|v: &mut Vec<_>| if v.len() > 1 { Some(v.remove(1)) } else { None }),

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.

Not quite sure why we alternate between 0 and 1 here? Would we gain anything in terms of coverage by choosing randomly from 0..len?

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 can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that, but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

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.

We can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that

Right, I imagined the latter.

but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

Yeah, that's why I thought covering the entire Vec might be worth it, but maybe it's not too important.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 3a944d2 to c05e508CompareJune 11, 2024 13:57
Comment threadlightning/src/ln/channelmanager.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from c05e508 to 9d0c1a1CompareJune 12, 2024 02:26
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Addressed feedback.

@tnull

tnull commented Jun 12, 2024

Copy link
Copy Markdown
Contributor

Addressed feedback.

Unfortunately the new test now reliably fails:

thread 'ln::chanmon_update_fail_tests::test_sync_async_persist_doesnt_hang' panicked at 'explicit panic', lightning/src/ln/chanmon_update_fail_tests.rs:3410:14

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 384d691 to 2a0c900CompareJune 12, 2024 14:11
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Ugh, sorry, pushed right before bed last night and didn't test enough :/

@valentinewallace

Copy link
Copy Markdown
Contributor

LGTM, feel free to squash IMO.

Now that we are gearing up to support fully async monitor storage,
we really need to fuzz monitor updates not completing before a
reload, which we do here in the `chanmon_consistency` fuzzer.
While there are more parts to async monitor updating that we need
to fuzz, this at least gets us started by having basic async
restart cases handled. In the future, we should extend this to make
sure some basic properties (eg claim/balance consistency) remain
true through `chanmon_consistency` runs.
When we have `ChannelMonitorUpdate`s which are completing both
synchronously and asynchronously, we need to consider a channel as
unblocked based on the `ChannelManager` monitor update queue,
rather than by checking the `update_id`s.
Consider the case where a channel is updated, leading to a
`ChannelMonitorUpdate` which completes asynchronously. The update
completes, but prior to the `ChannelManager` receiving the
`MonitorEvent::Completed` it generates a further
`ChannelMonitorUpdate`. This second update completes synchronously.
As a result, when the `MonitorEvent` is processed, the event's
`monitor_update_id` is the first update, but there are no updates
queued and the channel should be free to return to be unblocked.
Here we fix this by looking only at the `ChannelManager` update
queue, rather than the update_id of the `MonitorEvent`.
While we don't anticipate many users having both synchronous and
asynchronous persists in the same application, there isn't much
cost to supporting it, which we do here.
Found by the chanmon_consistency target.
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 2a0c900 to 920d96eCompareJune 12, 2024 15:32
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@tnull
tnull merged commit 5e3056e into lightningdevkit:mainJun 12, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@tnull@valentinewallace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fuzz reloading with a stale monitor in chanmon_consistency - #3113

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz
Jun 12, 2024
Merged

Fuzz reloading with a stale monitor in chanmon_consistency#3113
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Now that we are gearing up to support fully async monitor storage, we really need to fuzz monitor updates not completing before a reload, which we do here in the chanmon_consistency fuzzer.

While there are more parts to async monitor updating that we need to fuzz, this at least gets us started by having basic async restart cases handled. In the future, we should extend this to make sure some basic properties (eg claim/balance consistency) remain true through chanmon_consistency runs.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from fd0ecbd to 7446a79CompareJune 10, 2024 21:01
@codecov-commenter

codecov-commenter commented Jun 10, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 97.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.65%. Comparing base (1d0c6c6) to head (2a0c900).
Report is 15 commits behind head on main.

FilesPatch %Lines
lightning/src/ln/chanmon_update_fail_tests.rs97.46%2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@ Coverage Diff @@## main #3113 +/- ##
==========================================
+ Coverage 89.84% 90.65% +0.80% 
==========================================
Files 119 119 Lines 97811 102825 +5014 Branches 97811 102825 +5014 ==========================================
+ Hits 87883 93217 +5334 + Misses 7364 7108 -256 + Partials 2564 2500 -64 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch 2 times, most recently from b7a57bc to 61aef9bCompareJune 10, 2024 21:46

@tnulltnull 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, just a few questions/comments.

Comment threadfuzz/src/chanmon_consistency.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
&|v: &mut Vec<_>| if !v.is_empty() { Some(v.remove(0)) } else { None }),
0xf1 =>
complete_monitor_update(&monitor_a, &chan_1_funding,
&|v: &mut Vec<_>| if v.len() > 1 { Some(v.remove(1)) } else { None }),

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.

Not quite sure why we alternate between 0 and 1 here? Would we gain anything in terms of coverage by choosing randomly from 0..len?

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 can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that, but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

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.

We can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that

Right, I imagined the latter.

but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

Yeah, that's why I thought covering the entire Vec might be worth it, but maybe it's not too important.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 3a944d2 to c05e508CompareJune 11, 2024 13:57
Comment threadlightning/src/ln/channelmanager.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from c05e508 to 9d0c1a1CompareJune 12, 2024 02:26
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Addressed feedback.

@tnull

tnull commented Jun 12, 2024

Copy link
Copy Markdown
Contributor

Addressed feedback.

Unfortunately the new test now reliably fails:

thread 'ln::chanmon_update_fail_tests::test_sync_async_persist_doesnt_hang' panicked at 'explicit panic', lightning/src/ln/chanmon_update_fail_tests.rs:3410:14

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 384d691 to 2a0c900CompareJune 12, 2024 14:11
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Ugh, sorry, pushed right before bed last night and didn't test enough :/

@valentinewallace

Copy link
Copy Markdown
Contributor

LGTM, feel free to squash IMO.

Now that we are gearing up to support fully async monitor storage,
we really need to fuzz monitor updates not completing before a
reload, which we do here in the `chanmon_consistency` fuzzer.
While there are more parts to async monitor updating that we need
to fuzz, this at least gets us started by having basic async
restart cases handled. In the future, we should extend this to make
sure some basic properties (eg claim/balance consistency) remain
true through `chanmon_consistency` runs.
When we have `ChannelMonitorUpdate`s which are completing both
synchronously and asynchronously, we need to consider a channel as
unblocked based on the `ChannelManager` monitor update queue,
rather than by checking the `update_id`s.
Consider the case where a channel is updated, leading to a
`ChannelMonitorUpdate` which completes asynchronously. The update
completes, but prior to the `ChannelManager` receiving the
`MonitorEvent::Completed` it generates a further
`ChannelMonitorUpdate`. This second update completes synchronously.
As a result, when the `MonitorEvent` is processed, the event's
`monitor_update_id` is the first update, but there are no updates
queued and the channel should be free to return to be unblocked.
Here we fix this by looking only at the `ChannelManager` update
queue, rather than the update_id of the `MonitorEvent`.
While we don't anticipate many users having both synchronous and
asynchronous persists in the same application, there isn't much
cost to supporting it, which we do here.
Found by the chanmon_consistency target.
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 2a0c900 to 920d96eCompareJune 12, 2024 15:32
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@tnull
tnull merged commit 5e3056e into lightningdevkit:mainJun 12, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@tnull@valentinewallace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fuzz reloading with a stale monitor in chanmon_consistency - #3113

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz
Jun 12, 2024
Merged

Fuzz reloading with a stale monitor in chanmon_consistency#3113
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Now that we are gearing up to support fully async monitor storage, we really need to fuzz monitor updates not completing before a reload, which we do here in the chanmon_consistency fuzzer.

While there are more parts to async monitor updating that we need to fuzz, this at least gets us started by having basic async restart cases handled. In the future, we should extend this to make sure some basic properties (eg claim/balance consistency) remain true through chanmon_consistency runs.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from fd0ecbd to 7446a79CompareJune 10, 2024 21:01
@codecov-commenter

codecov-commenter commented Jun 10, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 97.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.65%. Comparing base (1d0c6c6) to head (2a0c900).
Report is 15 commits behind head on main.

FilesPatch %Lines
lightning/src/ln/chanmon_update_fail_tests.rs97.46%2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@ Coverage Diff @@## main #3113 +/- ##
==========================================
+ Coverage 89.84% 90.65% +0.80% 
==========================================
Files 119 119 Lines 97811 102825 +5014 Branches 97811 102825 +5014 ==========================================
+ Hits 87883 93217 +5334 + Misses 7364 7108 -256 + Partials 2564 2500 -64 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch 2 times, most recently from b7a57bc to 61aef9bCompareJune 10, 2024 21:46

@tnulltnull 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, just a few questions/comments.

Comment threadfuzz/src/chanmon_consistency.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
&|v: &mut Vec<_>| if !v.is_empty() { Some(v.remove(0)) } else { None }),
0xf1 =>
complete_monitor_update(&monitor_a, &chan_1_funding,
&|v: &mut Vec<_>| if v.len() > 1 { Some(v.remove(1)) } else { None }),

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.

Not quite sure why we alternate between 0 and 1 here? Would we gain anything in terms of coverage by choosing randomly from 0..len?

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 can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that, but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

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.

We can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that

Right, I imagined the latter.

but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

Yeah, that's why I thought covering the entire Vec might be worth it, but maybe it's not too important.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 3a944d2 to c05e508CompareJune 11, 2024 13:57
Comment threadlightning/src/ln/channelmanager.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from c05e508 to 9d0c1a1CompareJune 12, 2024 02:26
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Addressed feedback.

@tnull

tnull commented Jun 12, 2024

Copy link
Copy Markdown
Contributor

Addressed feedback.

Unfortunately the new test now reliably fails:

thread 'ln::chanmon_update_fail_tests::test_sync_async_persist_doesnt_hang' panicked at 'explicit panic', lightning/src/ln/chanmon_update_fail_tests.rs:3410:14

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 384d691 to 2a0c900CompareJune 12, 2024 14:11
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Ugh, sorry, pushed right before bed last night and didn't test enough :/

@valentinewallace

Copy link
Copy Markdown
Contributor

LGTM, feel free to squash IMO.

Now that we are gearing up to support fully async monitor storage,
we really need to fuzz monitor updates not completing before a
reload, which we do here in the `chanmon_consistency` fuzzer.
While there are more parts to async monitor updating that we need
to fuzz, this at least gets us started by having basic async
restart cases handled. In the future, we should extend this to make
sure some basic properties (eg claim/balance consistency) remain
true through `chanmon_consistency` runs.
When we have `ChannelMonitorUpdate`s which are completing both
synchronously and asynchronously, we need to consider a channel as
unblocked based on the `ChannelManager` monitor update queue,
rather than by checking the `update_id`s.
Consider the case where a channel is updated, leading to a
`ChannelMonitorUpdate` which completes asynchronously. The update
completes, but prior to the `ChannelManager` receiving the
`MonitorEvent::Completed` it generates a further
`ChannelMonitorUpdate`. This second update completes synchronously.
As a result, when the `MonitorEvent` is processed, the event's
`monitor_update_id` is the first update, but there are no updates
queued and the channel should be free to return to be unblocked.
Here we fix this by looking only at the `ChannelManager` update
queue, rather than the update_id of the `MonitorEvent`.
While we don't anticipate many users having both synchronous and
asynchronous persists in the same application, there isn't much
cost to supporting it, which we do here.
Found by the chanmon_consistency target.
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 2a0c900 to 920d96eCompareJune 12, 2024 15:32
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@tnull
tnull merged commit 5e3056e into lightningdevkit:mainJun 12, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

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

Fuzz reloading with a stale monitor in chanmon_consistency - #3113

Merged
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz
Jun 12, 2024
Merged

Fuzz reloading with a stale monitor in chanmon_consistency#3113
tnull merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2024-04-async-monitor-fuzz

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Now that we are gearing up to support fully async monitor storage, we really need to fuzz monitor updates not completing before a reload, which we do here in the chanmon_consistency fuzzer.

While there are more parts to async monitor updating that we need to fuzz, this at least gets us started by having basic async restart cases handled. In the future, we should extend this to make sure some basic properties (eg claim/balance consistency) remain true through chanmon_consistency runs.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from fd0ecbd to 7446a79CompareJune 10, 2024 21:01
@codecov-commenter

codecov-commenter commented Jun 10, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 97.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.65%. Comparing base (1d0c6c6) to head (2a0c900).
Report is 15 commits behind head on main.

FilesPatch %Lines
lightning/src/ln/chanmon_update_fail_tests.rs97.46%2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@ Coverage Diff @@## main #3113 +/- ##
==========================================
+ Coverage 89.84% 90.65% +0.80% 
==========================================
Files 119 119 Lines 97811 102825 +5014 Branches 97811 102825 +5014 ==========================================
+ Hits 87883 93217 +5334 + Misses 7364 7108 -256 + Partials 2564 2500 -64 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch 2 times, most recently from b7a57bc to 61aef9bCompareJune 10, 2024 21:46

@tnulltnull 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, just a few questions/comments.

Comment threadfuzz/src/chanmon_consistency.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
&|v: &mut Vec<_>| if !v.is_empty() { Some(v.remove(0)) } else { None }),
0xf1 =>
complete_monitor_update(&monitor_a, &chan_1_funding,
&|v: &mut Vec<_>| if v.len() > 1 { Some(v.remove(1)) } else { None }),

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.

Not quite sure why we alternate between 0 and 1 here? Would we gain anything in terms of coverage by choosing randomly from 0..len?

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 can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that, but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

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.

We can't chose randomly because fuzzer results need to be deterministic based on the input. We could read another byte of fuzz input and use that

Right, I imagined the latter.

but we really want the fuzz input to be as dense as possible, so using a single bit for it seems sufficient. I'm not sure that we'd have any issues that can't be expressed through finishing (first, second, last), but could see some issues arising where we need to finish something in the middle that isnt first or last.

Yeah, that's why I thought covering the entire Vec might be worth it, but maybe it's not too important.

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 3a944d2 to c05e508CompareJune 11, 2024 13:57
Comment threadlightning/src/ln/channelmanager.rs
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs Outdated
Comment threadfuzz/src/chanmon_consistency.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from c05e508 to 9d0c1a1CompareJune 12, 2024 02:26
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Addressed feedback.

@tnull

tnull commented Jun 12, 2024

Copy link
Copy Markdown
Contributor

Addressed feedback.

Unfortunately the new test now reliably fails:

thread 'ln::chanmon_update_fail_tests::test_sync_async_persist_doesnt_hang' panicked at 'explicit panic', lightning/src/ln/chanmon_update_fail_tests.rs:3410:14

@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 384d691 to 2a0c900CompareJune 12, 2024 14:11
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Ugh, sorry, pushed right before bed last night and didn't test enough :/

@valentinewallace

Copy link
Copy Markdown
Contributor

LGTM, feel free to squash IMO.

Now that we are gearing up to support fully async monitor storage,
we really need to fuzz monitor updates not completing before a
reload, which we do here in the `chanmon_consistency` fuzzer.
While there are more parts to async monitor updating that we need
to fuzz, this at least gets us started by having basic async
restart cases handled. In the future, we should extend this to make
sure some basic properties (eg claim/balance consistency) remain
true through `chanmon_consistency` runs.
When we have `ChannelMonitorUpdate`s which are completing both
synchronously and asynchronously, we need to consider a channel as
unblocked based on the `ChannelManager` monitor update queue,
rather than by checking the `update_id`s.
Consider the case where a channel is updated, leading to a
`ChannelMonitorUpdate` which completes asynchronously. The update
completes, but prior to the `ChannelManager` receiving the
`MonitorEvent::Completed` it generates a further
`ChannelMonitorUpdate`. This second update completes synchronously.
As a result, when the `MonitorEvent` is processed, the event's
`monitor_update_id` is the first update, but there are no updates
queued and the channel should be free to return to be unblocked.
Here we fix this by looking only at the `ChannelManager` update
queue, rather than the update_id of the `MonitorEvent`.
While we don't anticipate many users having both synchronous and
asynchronous persists in the same application, there isn't much
cost to supporting it, which we do here.
Found by the chanmon_consistency target.
@TheBlueMatt
TheBlueMattforce-pushed the 2024-04-async-monitor-fuzz branch from 2a0c900 to 920d96eCompareJune 12, 2024 15:32
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@tnull
tnull merged commit 5e3056e into lightningdevkit:mainJun 12, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@tnull@valentinewallace