Fail HTLCs which were removed from a channel but not persisted - #1857

Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-11-reload-htlc
Dec 5, 2022
Merged

Fail HTLCs which were removed from a channel but not persisted#1857
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-11-reload-htlc

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Nov 16, 2022•
edited
Loading

Copy link
Copy Markdown
Collaborator

Based on #1830 (I didn't pull it all in, but there's one commit that just takes one function from there).

When a channel is force-closed, if a ChannelMonitor update is
completed but a ChannelManager persist has not yet happened,
HTLCs which were removed in the latest (persisted) ChannelMonitor
update will not be failed even though they do not appear in the
commitment transaction which went on chain. This is because the
ChannelManager thinks the ChannelMonitor is responsible for
them (as it is stale), but the ChannelMonitor has no knowledge of
the HTLC at all (as it is not stale).

The fix for this is relatively simple - we need to check for this
specific case and fail back such HTLCs when deserializing a
ChannelManager

@TheBlueMattTheBlueMatt added this to the 0.0.113 milestone Nov 16, 2022
@TheBlueMatt
TheBlueMattforce-pushed the 2022-11-reload-htlc branch 2 times, most recently from 5180e9e to d793bb8CompareNovember 16, 2022 18:37
@TheBlueMattTheBlueMatt mentioned this pull request Nov 16, 2022
Closed
@codecov-commenter

codecov-commenter commented Nov 16, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Base: 90.60% // Head: 91.78% // Increases project coverage by +1.17% 🎉

Coverage data is based on head (3149544) compared to base (a4c4301).
Patch coverage: 93.36% of modified lines in pull request are covered.

Additional details and impacted files
@@ Coverage Diff @@## main #1857 +/- ##
==========================================
+ Coverage 90.60% 91.78% +1.17% 
==========================================
Files 91 91 Lines 47969 56197 +8228 Branches 47969 56197 +8228 ==========================================
+ Hits 43463 51581 +8118 - Misses 4506 4616 +110
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs96.52% <ø> (+2.87%)⬆️
lightning/src/ln/channel.rs91.30% <80.00%> (+2.56%)⬆️
lightning/src/ln/channelmanager.rs89.40% <82.97%> (+4.29%)⬆️
lightning/src/ln/reload_tests.rs95.50% <96.55%> (+0.25%)⬆️
lightning/src/chain/channelmonitor.rs93.29% <97.67%> (+2.55%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.43%)⬇️
lightning/src/ln/peer_channel_encryptor.rs93.38% <0.00%> (-0.25%)⬇️
lightning/src/ln/reorg_tests.rs100.00% <0.00%> (ø)
lightning/src/ln/payment_tests.rs98.92% <0.00%> (+0.02%)⬆️
lightning/src/ln/monitor_tests.rs99.66% <0.00%> (+0.10%)⬆️
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@valentinewallace

valentinewallace commented Nov 21, 2022

Copy link
Copy Markdown
Contributor

Needs rebase

@TheBlueMatt

TheBlueMatt commented Nov 21, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Rebased.

@dunxen
dunxen self-requested a review November 28, 2022 17:44
dunxen
dunxen reviewed Nov 29, 2022
View reviewed changes

@dunxendunxen 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 AFAICT. Just nits. One optional.

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/ln/reload_tests.rs Outdated
dunxen
dunxen previously approved these changes Dec 5, 2022
View reviewed changes
@TheBlueMatt
Expose the full set of outbound HTLCs from a ChannelMonitor
e142f67
This expands the outbound-HTLC-listing support in `ChannelMonitor`
to include not only the set of outbound HTLCs which have not yet
been resolved but to also include the full set of HTLCs which the
`ChannelMonitor` is currently able to to or has already finalized.
This will be used in the next commit to fail-back HTLCs which were
removed from a channel in the ChannelMonitor but not in a Channel.
Using the existing `get_pending_outbound_htlcs` for this purpose is
subtly broken - if the channel is already closed, an HTLC fail may
have completed on chain and is no longer "pending" to the monitor,
but the fail event is still in the monitor waiting to be handed
back to the `ChannelMonitor` when polled.
@TheBlueMatt
Avoid attempting to forward to a closed chan on stale-data reload
0bb87dd
If, after forwarding a payment to our counterparty, we restart with
a ChannelMonitor update having been persisted, but the
corresponding ChannelManager update was not persisted, we'll still
have the forwarded HTLC in the `forward_htlcs` map on start. This
will cause us to generate a (spurious) `PendingHTLCsForwardable`
event. However, when we go to forward said HTLC, we'll notice the
channel has been closed and leave it up to the `ChannelMontior` to
finalize the HTLC.
This is all fine today - we won't lose any funds, we'll just
generate an excess forwardable event and then fail to forward.
However, in the future when we allow for forward-time channel
changes this could break. Thus, its worth adding tests for this
behavior today, and, while we're at it, removing the spurious
forwardable HTLCs event.
@TheBlueMatt
Fail HTLCs which were removed from a channel but not persisted
dbe4aad
When a channel is force-closed, if a `ChannelMonitor` update is
completed but a `ChannelManager` persist has not yet happened,
HTLCs which were removed in the latest (persisted) `ChannelMonitor`
update will not be failed even though they do not appear in the
commitment transaction which went on chain. This is because the
`ChannelManager` thinks the `ChannelMonitor` is responsible for
them (as it is stale), but the `ChannelMonitor` has no knowledge of
the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this
specific case and fail back such HTLCs when deserializing a
`ChannelManager`
@TheBlueMatt

TheBlueMatt commented Dec 5, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Squashed fixups.

@TheBlueMatt
TheBlueMatt merged commit f4ab077 into lightningdevkit:mainDec 5, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dunxendunxendunxen approved these changes
@wpaulinowpaulinowpaulino approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.0.113

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@valentinewallace@dunxen@wpaulino
, '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
Fail HTLCs which were removed from a channel but not persisted - #1857
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Dec 5, 2022
Merged
Fail HTLCs which were removed from a channel but not persisted#1857
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Collaborator
Based on #1830 (I didn't pull it all in, but there's one commit that just takes one function from there).
When a channel is force-closed, if a ChannelMonitor update is

completed but a ChannelManager persist has not yet happened,

HTLCs which were removed in the latest (persisted) ChannelMonitor

update will not be failed even though they do not appear in the

commitment transaction which went on chain. This is because the
ChannelManager thinks the ChannelMonitor is responsible for

them (as it is stale), but the ChannelMonitor has no knowledge of

the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this

specific case and fail back such HTLCs when deserializing a
ChannelManager
@TheBlueMattTheBlueMatt
added this to the 0.0.113 milestone
Nov 16, 2022
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-reload-htlc
branch
2 times, most recently
from
5180e9e to
d793bb8CompareNovember 16, 2022 18:37
@TheBlueMattTheBlueMatt
mentioned this pull request
Nov 16, 2022

Closed

@codecov-commenter
codecov-commenter
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.60% // Head: 91.78% // Increases project coverage by +1.17% 🎉
Coverage data is based on head (3149544) compared to base (a4c4301).

Patch coverage: 93.36% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1857 +/- ##
==========================================
+ Coverage 90.60% 91.78% +1.17% 
==========================================
Files 91 91 Lines 47969 56197 +8228 Branches 47969 56197 +8228 ==========================================
+ Hits 43463 51581 +8118 - Misses 4506 4616 +110 
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs96.52% <ø> (+2.87%)⬆️
lightning/src/ln/channel.rs91.30% <80.00%> (+2.56%)⬆️
lightning/src/ln/channelmanager.rs89.40% <82.97%> (+4.29%)⬆️
lightning/src/ln/reload_tests.rs95.50% <96.55%> (+0.25%)⬆️
lightning/src/chain/channelmonitor.rs93.29% <97.67%> (+2.55%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.43%)⬇️
lightning/src/ln/peer_channel_encryptor.rs93.38% <0.00%> (-0.25%)⬇️
lightning/src/ln/reorg_tests.rs100.00% <0.00%> (ø)
lightning/src/ln/payment_tests.rs98.92% <0.00%> (+0.02%)⬆️
lightning/src/ln/monitor_tests.rs99.66% <0.00%> (+0.10%)⬆️
... and 12 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@valentinewallace
valentinewallace
commented
Nov 21, 2022

Copy link


Copy Markdown

Contributor
Needs rebase
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@dunxen
dunxen
self-requested a review
November 28, 2022 17:44
dunxen
dunxen
reviewed
Nov 29, 2022
View reviewed changes
@dunxendunxen
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 AFAICT. Just nits. One optional.
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/reload_tests.rs
Outdated

dunxen
dunxen
previously approved these changes
Dec 5, 2022
View reviewed changes
@TheBlueMatt
Expose the full set of outbound HTLCs from a ChannelMonitor
e142f67
This expands the outbound-HTLC-listing support in `ChannelMonitor`
to include not only the set of outbound HTLCs which have not yet
been resolved but to also include the full set of HTLCs which the
`ChannelMonitor` is currently able to to or has already finalized.
This will be used in the next commit to fail-back HTLCs which were
removed from a channel in the ChannelMonitor but not in a Channel.
Using the existing `get_pending_outbound_htlcs` for this purpose is
subtly broken - if the channel is already closed, an HTLC fail may
have completed on chain and is no longer "pending" to the monitor,
but the fail event is still in the monitor waiting to be handed
back to the `ChannelMonitor` when polled.
@TheBlueMatt
Avoid attempting to forward to a closed chan on stale-data reload
0bb87dd
If, after forwarding a payment to our counterparty, we restart with
a ChannelMonitor update having been persisted, but the
corresponding ChannelManager update was not persisted, we'll still
have the forwarded HTLC in the `forward_htlcs` map on start. This
will cause us to generate a (spurious) `PendingHTLCsForwardable`
event. However, when we go to forward said HTLC, we'll notice the
channel has been closed and leave it up to the `ChannelMontior` to
finalize the HTLC.
This is all fine today - we won't lose any funds, we'll just
generate an excess forwardable event and then fail to forward.
However, in the future when we allow for forward-time channel
changes this could break. Thus, its worth adding tests for this
behavior today, and, while we're at it, removing the spurious
forwardable HTLCs event.
@TheBlueMatt
Fail HTLCs which were removed from a channel but not persisted
dbe4aad
When a channel is force-closed, if a `ChannelMonitor` update is
completed but a `ChannelManager` persist has not yet happened,
HTLCs which were removed in the latest (persisted) `ChannelMonitor`
update will not be failed even though they do not appear in the
commitment transaction which went on chain. This is because the
`ChannelManager` thinks the `ChannelMonitor` is responsible for
them (as it is stale), but the `ChannelMonitor` has no knowledge of
the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this
specific case and fail back such HTLCs when deserializing a
`ChannelManager`
@TheBlueMatt
TheBlueMatt
commented
Dec 5, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed fixups.
@TheBlueMatt
TheBlueMatt
merged commit f4ab077
into
lightningdevkit:mainDec 5, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen approved these changes
@wpaulinowpaulinowpaulino approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.113

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@valentinewallace@dunxen@wpaulino

, '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
Fail HTLCs which were removed from a channel but not persisted - #1857
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Dec 5, 2022
Merged
Fail HTLCs which were removed from a channel but not persisted#1857
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Collaborator
Based on #1830 (I didn't pull it all in, but there's one commit that just takes one function from there).
When a channel is force-closed, if a ChannelMonitor update is

completed but a ChannelManager persist has not yet happened,

HTLCs which were removed in the latest (persisted) ChannelMonitor

update will not be failed even though they do not appear in the

commitment transaction which went on chain. This is because the
ChannelManager thinks the ChannelMonitor is responsible for

them (as it is stale), but the ChannelMonitor has no knowledge of

the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this

specific case and fail back such HTLCs when deserializing a
ChannelManager
@TheBlueMattTheBlueMatt
added this to the 0.0.113 milestone
Nov 16, 2022
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-reload-htlc
branch
2 times, most recently
from
5180e9e to
d793bb8CompareNovember 16, 2022 18:37
@TheBlueMattTheBlueMatt
mentioned this pull request
Nov 16, 2022

Closed

@codecov-commenter
codecov-commenter
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.60% // Head: 91.78% // Increases project coverage by +1.17% 🎉
Coverage data is based on head (3149544) compared to base (a4c4301).

Patch coverage: 93.36% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1857 +/- ##
==========================================
+ Coverage 90.60% 91.78% +1.17% 
==========================================
Files 91 91 Lines 47969 56197 +8228 Branches 47969 56197 +8228 ==========================================
+ Hits 43463 51581 +8118 - Misses 4506 4616 +110 
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs96.52% <ø> (+2.87%)⬆️
lightning/src/ln/channel.rs91.30% <80.00%> (+2.56%)⬆️
lightning/src/ln/channelmanager.rs89.40% <82.97%> (+4.29%)⬆️
lightning/src/ln/reload_tests.rs95.50% <96.55%> (+0.25%)⬆️
lightning/src/chain/channelmonitor.rs93.29% <97.67%> (+2.55%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.43%)⬇️
lightning/src/ln/peer_channel_encryptor.rs93.38% <0.00%> (-0.25%)⬇️
lightning/src/ln/reorg_tests.rs100.00% <0.00%> (ø)
lightning/src/ln/payment_tests.rs98.92% <0.00%> (+0.02%)⬆️
lightning/src/ln/monitor_tests.rs99.66% <0.00%> (+0.10%)⬆️
... and 12 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@valentinewallace
valentinewallace
commented
Nov 21, 2022

Copy link


Copy Markdown

Contributor
Needs rebase
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@dunxen
dunxen
self-requested a review
November 28, 2022 17:44
dunxen
dunxen
reviewed
Nov 29, 2022
View reviewed changes
@dunxendunxen
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 AFAICT. Just nits. One optional.
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/reload_tests.rs
Outdated

dunxen
dunxen
previously approved these changes
Dec 5, 2022
View reviewed changes
@TheBlueMatt
Expose the full set of outbound HTLCs from a ChannelMonitor
e142f67
This expands the outbound-HTLC-listing support in `ChannelMonitor`
to include not only the set of outbound HTLCs which have not yet
been resolved but to also include the full set of HTLCs which the
`ChannelMonitor` is currently able to to or has already finalized.
This will be used in the next commit to fail-back HTLCs which were
removed from a channel in the ChannelMonitor but not in a Channel.
Using the existing `get_pending_outbound_htlcs` for this purpose is
subtly broken - if the channel is already closed, an HTLC fail may
have completed on chain and is no longer "pending" to the monitor,
but the fail event is still in the monitor waiting to be handed
back to the `ChannelMonitor` when polled.
@TheBlueMatt
Avoid attempting to forward to a closed chan on stale-data reload
0bb87dd
If, after forwarding a payment to our counterparty, we restart with
a ChannelMonitor update having been persisted, but the
corresponding ChannelManager update was not persisted, we'll still
have the forwarded HTLC in the `forward_htlcs` map on start. This
will cause us to generate a (spurious) `PendingHTLCsForwardable`
event. However, when we go to forward said HTLC, we'll notice the
channel has been closed and leave it up to the `ChannelMontior` to
finalize the HTLC.
This is all fine today - we won't lose any funds, we'll just
generate an excess forwardable event and then fail to forward.
However, in the future when we allow for forward-time channel
changes this could break. Thus, its worth adding tests for this
behavior today, and, while we're at it, removing the spurious
forwardable HTLCs event.
@TheBlueMatt
Fail HTLCs which were removed from a channel but not persisted
dbe4aad
When a channel is force-closed, if a `ChannelMonitor` update is
completed but a `ChannelManager` persist has not yet happened,
HTLCs which were removed in the latest (persisted) `ChannelMonitor`
update will not be failed even though they do not appear in the
commitment transaction which went on chain. This is because the
`ChannelManager` thinks the `ChannelMonitor` is responsible for
them (as it is stale), but the `ChannelMonitor` has no knowledge of
the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this
specific case and fail back such HTLCs when deserializing a
`ChannelManager`
@TheBlueMatt
TheBlueMatt
commented
Dec 5, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed fixups.
@TheBlueMatt
TheBlueMatt
merged commit f4ab077
into
lightningdevkit:mainDec 5, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen approved these changes
@wpaulinowpaulinowpaulino approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.113

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@valentinewallace@dunxen@wpaulino

, '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
Fail HTLCs which were removed from a channel but not persisted - #1857
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Dec 5, 2022
Merged
Fail HTLCs which were removed from a channel but not persisted#1857
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Collaborator
Based on #1830 (I didn't pull it all in, but there's one commit that just takes one function from there).
When a channel is force-closed, if a ChannelMonitor update is

completed but a ChannelManager persist has not yet happened,

HTLCs which were removed in the latest (persisted) ChannelMonitor

update will not be failed even though they do not appear in the

commitment transaction which went on chain. This is because the
ChannelManager thinks the ChannelMonitor is responsible for

them (as it is stale), but the ChannelMonitor has no knowledge of

the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this

specific case and fail back such HTLCs when deserializing a
ChannelManager
@TheBlueMattTheBlueMatt
added this to the 0.0.113 milestone
Nov 16, 2022
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-reload-htlc
branch
2 times, most recently
from
5180e9e to
d793bb8CompareNovember 16, 2022 18:37
@TheBlueMattTheBlueMatt
mentioned this pull request
Nov 16, 2022

Closed

@codecov-commenter
codecov-commenter
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.60% // Head: 91.78% // Increases project coverage by +1.17% 🎉
Coverage data is based on head (3149544) compared to base (a4c4301).

Patch coverage: 93.36% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1857 +/- ##
==========================================
+ Coverage 90.60% 91.78% +1.17% 
==========================================
Files 91 91 Lines 47969 56197 +8228 Branches 47969 56197 +8228 ==========================================
+ Hits 43463 51581 +8118 - Misses 4506 4616 +110 
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs96.52% <ø> (+2.87%)⬆️
lightning/src/ln/channel.rs91.30% <80.00%> (+2.56%)⬆️
lightning/src/ln/channelmanager.rs89.40% <82.97%> (+4.29%)⬆️
lightning/src/ln/reload_tests.rs95.50% <96.55%> (+0.25%)⬆️
lightning/src/chain/channelmonitor.rs93.29% <97.67%> (+2.55%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.43%)⬇️
lightning/src/ln/peer_channel_encryptor.rs93.38% <0.00%> (-0.25%)⬇️
lightning/src/ln/reorg_tests.rs100.00% <0.00%> (ø)
lightning/src/ln/payment_tests.rs98.92% <0.00%> (+0.02%)⬆️
lightning/src/ln/monitor_tests.rs99.66% <0.00%> (+0.10%)⬆️
... and 12 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@valentinewallace
valentinewallace
commented
Nov 21, 2022

Copy link


Copy Markdown

Contributor
Needs rebase
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@dunxen
dunxen
self-requested a review
November 28, 2022 17:44
dunxen
dunxen
reviewed
Nov 29, 2022
View reviewed changes
@dunxendunxen
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 AFAICT. Just nits. One optional.
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/reload_tests.rs
Outdated

dunxen
dunxen
previously approved these changes
Dec 5, 2022
View reviewed changes
@TheBlueMatt
Expose the full set of outbound HTLCs from a ChannelMonitor
e142f67
This expands the outbound-HTLC-listing support in `ChannelMonitor`
to include not only the set of outbound HTLCs which have not yet
been resolved but to also include the full set of HTLCs which the
`ChannelMonitor` is currently able to to or has already finalized.
This will be used in the next commit to fail-back HTLCs which were
removed from a channel in the ChannelMonitor but not in a Channel.
Using the existing `get_pending_outbound_htlcs` for this purpose is
subtly broken - if the channel is already closed, an HTLC fail may
have completed on chain and is no longer "pending" to the monitor,
but the fail event is still in the monitor waiting to be handed
back to the `ChannelMonitor` when polled.
@TheBlueMatt
Avoid attempting to forward to a closed chan on stale-data reload
0bb87dd
If, after forwarding a payment to our counterparty, we restart with
a ChannelMonitor update having been persisted, but the
corresponding ChannelManager update was not persisted, we'll still
have the forwarded HTLC in the `forward_htlcs` map on start. This
will cause us to generate a (spurious) `PendingHTLCsForwardable`
event. However, when we go to forward said HTLC, we'll notice the
channel has been closed and leave it up to the `ChannelMontior` to
finalize the HTLC.
This is all fine today - we won't lose any funds, we'll just
generate an excess forwardable event and then fail to forward.
However, in the future when we allow for forward-time channel
changes this could break. Thus, its worth adding tests for this
behavior today, and, while we're at it, removing the spurious
forwardable HTLCs event.
@TheBlueMatt
Fail HTLCs which were removed from a channel but not persisted
dbe4aad
When a channel is force-closed, if a `ChannelMonitor` update is
completed but a `ChannelManager` persist has not yet happened,
HTLCs which were removed in the latest (persisted) `ChannelMonitor`
update will not be failed even though they do not appear in the
commitment transaction which went on chain. This is because the
`ChannelManager` thinks the `ChannelMonitor` is responsible for
them (as it is stale), but the `ChannelMonitor` has no knowledge of
the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this
specific case and fail back such HTLCs when deserializing a
`ChannelManager`
@TheBlueMatt
TheBlueMatt
commented
Dec 5, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed fixups.
@TheBlueMatt
TheBlueMatt
merged commit f4ab077
into
lightningdevkit:mainDec 5, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen approved these changes
@wpaulinowpaulinowpaulino approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.113

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@valentinewallace@dunxen@wpaulino

, '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
Fail HTLCs which were removed from a channel but not persisted - #1857
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Dec 5, 2022
Merged
Fail HTLCs which were removed from a channel but not persisted#1857
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Collaborator
Based on #1830 (I didn't pull it all in, but there's one commit that just takes one function from there).
When a channel is force-closed, if a ChannelMonitor update is

completed but a ChannelManager persist has not yet happened,

HTLCs which were removed in the latest (persisted) ChannelMonitor

update will not be failed even though they do not appear in the

commitment transaction which went on chain. This is because the
ChannelManager thinks the ChannelMonitor is responsible for

them (as it is stale), but the ChannelMonitor has no knowledge of

the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this

specific case and fail back such HTLCs when deserializing a
ChannelManager
@TheBlueMattTheBlueMatt
added this to the 0.0.113 milestone
Nov 16, 2022
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-reload-htlc
branch
2 times, most recently
from
5180e9e to
d793bb8CompareNovember 16, 2022 18:37
@TheBlueMattTheBlueMatt
mentioned this pull request
Nov 16, 2022

Closed

@codecov-commenter
codecov-commenter
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.60% // Head: 91.78% // Increases project coverage by +1.17% 🎉
Coverage data is based on head (3149544) compared to base (a4c4301).

Patch coverage: 93.36% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1857 +/- ##
==========================================
+ Coverage 90.60% 91.78% +1.17% 
==========================================
Files 91 91 Lines 47969 56197 +8228 Branches 47969 56197 +8228 ==========================================
+ Hits 43463 51581 +8118 - Misses 4506 4616 +110 
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs96.52% <ø> (+2.87%)⬆️
lightning/src/ln/channel.rs91.30% <80.00%> (+2.56%)⬆️
lightning/src/ln/channelmanager.rs89.40% <82.97%> (+4.29%)⬆️
lightning/src/ln/reload_tests.rs95.50% <96.55%> (+0.25%)⬆️
lightning/src/chain/channelmonitor.rs93.29% <97.67%> (+2.55%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.43%)⬇️
lightning/src/ln/peer_channel_encryptor.rs93.38% <0.00%> (-0.25%)⬇️
lightning/src/ln/reorg_tests.rs100.00% <0.00%> (ø)
lightning/src/ln/payment_tests.rs98.92% <0.00%> (+0.02%)⬆️
lightning/src/ln/monitor_tests.rs99.66% <0.00%> (+0.10%)⬆️
... and 12 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@valentinewallace
valentinewallace
commented
Nov 21, 2022

Copy link


Copy Markdown

Contributor
Needs rebase
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@dunxen
dunxen
self-requested a review
November 28, 2022 17:44
dunxen
dunxen
reviewed
Nov 29, 2022
View reviewed changes
@dunxendunxen
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 AFAICT. Just nits. One optional.
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/reload_tests.rs
Outdated

dunxen
dunxen
previously approved these changes
Dec 5, 2022
View reviewed changes
@TheBlueMatt
Expose the full set of outbound HTLCs from a ChannelMonitor
e142f67
This expands the outbound-HTLC-listing support in `ChannelMonitor`
to include not only the set of outbound HTLCs which have not yet
been resolved but to also include the full set of HTLCs which the
`ChannelMonitor` is currently able to to or has already finalized.
This will be used in the next commit to fail-back HTLCs which were
removed from a channel in the ChannelMonitor but not in a Channel.
Using the existing `get_pending_outbound_htlcs` for this purpose is
subtly broken - if the channel is already closed, an HTLC fail may
have completed on chain and is no longer "pending" to the monitor,
but the fail event is still in the monitor waiting to be handed
back to the `ChannelMonitor` when polled.
@TheBlueMatt
Avoid attempting to forward to a closed chan on stale-data reload
0bb87dd
If, after forwarding a payment to our counterparty, we restart with
a ChannelMonitor update having been persisted, but the
corresponding ChannelManager update was not persisted, we'll still
have the forwarded HTLC in the `forward_htlcs` map on start. This
will cause us to generate a (spurious) `PendingHTLCsForwardable`
event. However, when we go to forward said HTLC, we'll notice the
channel has been closed and leave it up to the `ChannelMontior` to
finalize the HTLC.
This is all fine today - we won't lose any funds, we'll just
generate an excess forwardable event and then fail to forward.
However, in the future when we allow for forward-time channel
changes this could break. Thus, its worth adding tests for this
behavior today, and, while we're at it, removing the spurious
forwardable HTLCs event.
@TheBlueMatt
Fail HTLCs which were removed from a channel but not persisted
dbe4aad
When a channel is force-closed, if a `ChannelMonitor` update is
completed but a `ChannelManager` persist has not yet happened,
HTLCs which were removed in the latest (persisted) `ChannelMonitor`
update will not be failed even though they do not appear in the
commitment transaction which went on chain. This is because the
`ChannelManager` thinks the `ChannelMonitor` is responsible for
them (as it is stale), but the `ChannelMonitor` has no knowledge of
the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this
specific case and fail back such HTLCs when deserializing a
`ChannelManager`
@TheBlueMatt
TheBlueMatt
commented
Dec 5, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed fixups.
@TheBlueMatt
TheBlueMatt
merged commit f4ab077
into
lightningdevkit:mainDec 5, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen approved these changes
@wpaulinowpaulinowpaulino approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.113

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@valentinewallace@dunxen@wpaulino

, '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
Fail HTLCs which were removed from a channel but not persisted - #1857
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Dec 5, 2022
Merged
Fail HTLCs which were removed from a channel but not persisted#1857
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Collaborator
Based on #1830 (I didn't pull it all in, but there's one commit that just takes one function from there).
When a channel is force-closed, if a ChannelMonitor update is

completed but a ChannelManager persist has not yet happened,

HTLCs which were removed in the latest (persisted) ChannelMonitor

update will not be failed even though they do not appear in the

commitment transaction which went on chain. This is because the
ChannelManager thinks the ChannelMonitor is responsible for

them (as it is stale), but the ChannelMonitor has no knowledge of

the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this

specific case and fail back such HTLCs when deserializing a
ChannelManager
@TheBlueMattTheBlueMatt
added this to the 0.0.113 milestone
Nov 16, 2022
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-reload-htlc
branch
2 times, most recently
from
5180e9e to
d793bb8CompareNovember 16, 2022 18:37
@TheBlueMattTheBlueMatt
mentioned this pull request
Nov 16, 2022

Closed

@codecov-commenter
codecov-commenter
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.60% // Head: 91.78% // Increases project coverage by +1.17% 🎉
Coverage data is based on head (3149544) compared to base (a4c4301).

Patch coverage: 93.36% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1857 +/- ##
==========================================
+ Coverage 90.60% 91.78% +1.17% 
==========================================
Files 91 91 Lines 47969 56197 +8228 Branches 47969 56197 +8228 ==========================================
+ Hits 43463 51581 +8118 - Misses 4506 4616 +110 
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs96.52% <ø> (+2.87%)⬆️
lightning/src/ln/channel.rs91.30% <80.00%> (+2.56%)⬆️
lightning/src/ln/channelmanager.rs89.40% <82.97%> (+4.29%)⬆️
lightning/src/ln/reload_tests.rs95.50% <96.55%> (+0.25%)⬆️
lightning/src/chain/channelmonitor.rs93.29% <97.67%> (+2.55%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.43%)⬇️
lightning/src/ln/peer_channel_encryptor.rs93.38% <0.00%> (-0.25%)⬇️
lightning/src/ln/reorg_tests.rs100.00% <0.00%> (ø)
lightning/src/ln/payment_tests.rs98.92% <0.00%> (+0.02%)⬆️
lightning/src/ln/monitor_tests.rs99.66% <0.00%> (+0.10%)⬆️
... and 12 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@valentinewallace
valentinewallace
commented
Nov 21, 2022

Copy link


Copy Markdown

Contributor
Needs rebase
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@dunxen
dunxen
self-requested a review
November 28, 2022 17:44
dunxen
dunxen
reviewed
Nov 29, 2022
View reviewed changes
@dunxendunxen
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 AFAICT. Just nits. One optional.
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/reload_tests.rs
Outdated

dunxen
dunxen
previously approved these changes
Dec 5, 2022
View reviewed changes
@TheBlueMatt
Expose the full set of outbound HTLCs from a ChannelMonitor
e142f67
This expands the outbound-HTLC-listing support in `ChannelMonitor`
to include not only the set of outbound HTLCs which have not yet
been resolved but to also include the full set of HTLCs which the
`ChannelMonitor` is currently able to to or has already finalized.
This will be used in the next commit to fail-back HTLCs which were
removed from a channel in the ChannelMonitor but not in a Channel.
Using the existing `get_pending_outbound_htlcs` for this purpose is
subtly broken - if the channel is already closed, an HTLC fail may
have completed on chain and is no longer "pending" to the monitor,
but the fail event is still in the monitor waiting to be handed
back to the `ChannelMonitor` when polled.
@TheBlueMatt
Avoid attempting to forward to a closed chan on stale-data reload
0bb87dd
If, after forwarding a payment to our counterparty, we restart with
a ChannelMonitor update having been persisted, but the
corresponding ChannelManager update was not persisted, we'll still
have the forwarded HTLC in the `forward_htlcs` map on start. This
will cause us to generate a (spurious) `PendingHTLCsForwardable`
event. However, when we go to forward said HTLC, we'll notice the
channel has been closed and leave it up to the `ChannelMontior` to
finalize the HTLC.
This is all fine today - we won't lose any funds, we'll just
generate an excess forwardable event and then fail to forward.
However, in the future when we allow for forward-time channel
changes this could break. Thus, its worth adding tests for this
behavior today, and, while we're at it, removing the spurious
forwardable HTLCs event.
@TheBlueMatt
Fail HTLCs which were removed from a channel but not persisted
dbe4aad
When a channel is force-closed, if a `ChannelMonitor` update is
completed but a `ChannelManager` persist has not yet happened,
HTLCs which were removed in the latest (persisted) `ChannelMonitor`
update will not be failed even though they do not appear in the
commitment transaction which went on chain. This is because the
`ChannelManager` thinks the `ChannelMonitor` is responsible for
them (as it is stale), but the `ChannelMonitor` has no knowledge of
the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this
specific case and fail back such HTLCs when deserializing a
`ChannelManager`
@TheBlueMatt
TheBlueMatt
commented
Dec 5, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed fixups.
@TheBlueMatt
TheBlueMatt
merged commit f4ab077
into
lightningdevkit:mainDec 5, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen approved these changes
@wpaulinowpaulinowpaulino approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.113

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@valentinewallace@dunxen@wpaulino

, '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
Fail HTLCs which were removed from a channel but not persisted - #1857
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Dec 5, 2022
Merged
Fail HTLCs which were removed from a channel but not persisted#1857
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Collaborator
Based on #1830 (I didn't pull it all in, but there's one commit that just takes one function from there).
When a channel is force-closed, if a ChannelMonitor update is

completed but a ChannelManager persist has not yet happened,

HTLCs which were removed in the latest (persisted) ChannelMonitor

update will not be failed even though they do not appear in the

commitment transaction which went on chain. This is because the
ChannelManager thinks the ChannelMonitor is responsible for

them (as it is stale), but the ChannelMonitor has no knowledge of

the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this

specific case and fail back such HTLCs when deserializing a
ChannelManager
@TheBlueMattTheBlueMatt
added this to the 0.0.113 milestone
Nov 16, 2022
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-reload-htlc
branch
2 times, most recently
from
5180e9e to
d793bb8CompareNovember 16, 2022 18:37
@TheBlueMattTheBlueMatt
mentioned this pull request
Nov 16, 2022

Closed

@codecov-commenter
codecov-commenter
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.60% // Head: 91.78% // Increases project coverage by +1.17% 🎉
Coverage data is based on head (3149544) compared to base (a4c4301).

Patch coverage: 93.36% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1857 +/- ##
==========================================
+ Coverage 90.60% 91.78% +1.17% 
==========================================
Files 91 91 Lines 47969 56197 +8228 Branches 47969 56197 +8228 ==========================================
+ Hits 43463 51581 +8118 - Misses 4506 4616 +110 
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs96.52% <ø> (+2.87%)⬆️
lightning/src/ln/channel.rs91.30% <80.00%> (+2.56%)⬆️
lightning/src/ln/channelmanager.rs89.40% <82.97%> (+4.29%)⬆️
lightning/src/ln/reload_tests.rs95.50% <96.55%> (+0.25%)⬆️
lightning/src/chain/channelmonitor.rs93.29% <97.67%> (+2.55%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.43%)⬇️
lightning/src/ln/peer_channel_encryptor.rs93.38% <0.00%> (-0.25%)⬇️
lightning/src/ln/reorg_tests.rs100.00% <0.00%> (ø)
lightning/src/ln/payment_tests.rs98.92% <0.00%> (+0.02%)⬆️
lightning/src/ln/monitor_tests.rs99.66% <0.00%> (+0.10%)⬆️
... and 12 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@valentinewallace
valentinewallace
commented
Nov 21, 2022

Copy link


Copy Markdown

Contributor
Needs rebase
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@dunxen
dunxen
self-requested a review
November 28, 2022 17:44
dunxen
dunxen
reviewed
Nov 29, 2022
View reviewed changes
@dunxendunxen
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 AFAICT. Just nits. One optional.
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/reload_tests.rs
Outdated

dunxen
dunxen
previously approved these changes
Dec 5, 2022
View reviewed changes
@TheBlueMatt
Expose the full set of outbound HTLCs from a ChannelMonitor
e142f67
This expands the outbound-HTLC-listing support in `ChannelMonitor`
to include not only the set of outbound HTLCs which have not yet
been resolved but to also include the full set of HTLCs which the
`ChannelMonitor` is currently able to to or has already finalized.
This will be used in the next commit to fail-back HTLCs which were
removed from a channel in the ChannelMonitor but not in a Channel.
Using the existing `get_pending_outbound_htlcs` for this purpose is
subtly broken - if the channel is already closed, an HTLC fail may
have completed on chain and is no longer "pending" to the monitor,
but the fail event is still in the monitor waiting to be handed
back to the `ChannelMonitor` when polled.
@TheBlueMatt
Avoid attempting to forward to a closed chan on stale-data reload
0bb87dd
If, after forwarding a payment to our counterparty, we restart with
a ChannelMonitor update having been persisted, but the
corresponding ChannelManager update was not persisted, we'll still
have the forwarded HTLC in the `forward_htlcs` map on start. This
will cause us to generate a (spurious) `PendingHTLCsForwardable`
event. However, when we go to forward said HTLC, we'll notice the
channel has been closed and leave it up to the `ChannelMontior` to
finalize the HTLC.
This is all fine today - we won't lose any funds, we'll just
generate an excess forwardable event and then fail to forward.
However, in the future when we allow for forward-time channel
changes this could break. Thus, its worth adding tests for this
behavior today, and, while we're at it, removing the spurious
forwardable HTLCs event.
@TheBlueMatt
Fail HTLCs which were removed from a channel but not persisted
dbe4aad
When a channel is force-closed, if a `ChannelMonitor` update is
completed but a `ChannelManager` persist has not yet happened,
HTLCs which were removed in the latest (persisted) `ChannelMonitor`
update will not be failed even though they do not appear in the
commitment transaction which went on chain. This is because the
`ChannelManager` thinks the `ChannelMonitor` is responsible for
them (as it is stale), but the `ChannelMonitor` has no knowledge of
the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this
specific case and fail back such HTLCs when deserializing a
`ChannelManager`
@TheBlueMatt
TheBlueMatt
commented
Dec 5, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed fixups.
@TheBlueMatt
TheBlueMatt
merged commit f4ab077
into
lightningdevkit:mainDec 5, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen approved these changes
@wpaulinowpaulinowpaulino approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.113

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@valentinewallace@dunxen@wpaulino

, '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
Fail HTLCs which were removed from a channel but not persisted - #1857
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Dec 5, 2022
Merged
Fail HTLCs which were removed from a channel but not persisted#1857
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-reload-htlc
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Collaborator
Based on #1830 (I didn't pull it all in, but there's one commit that just takes one function from there).
When a channel is force-closed, if a ChannelMonitor update is

completed but a ChannelManager persist has not yet happened,

HTLCs which were removed in the latest (persisted) ChannelMonitor

update will not be failed even though they do not appear in the

commitment transaction which went on chain. This is because the
ChannelManager thinks the ChannelMonitor is responsible for

them (as it is stale), but the ChannelMonitor has no knowledge of

the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this

specific case and fail back such HTLCs when deserializing a
ChannelManager
@TheBlueMattTheBlueMatt
added this to the 0.0.113 milestone
Nov 16, 2022
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-reload-htlc
branch
2 times, most recently
from
5180e9e to
d793bb8CompareNovember 16, 2022 18:37
@TheBlueMattTheBlueMatt
mentioned this pull request
Nov 16, 2022

Closed

@codecov-commenter
codecov-commenter
commented
Nov 16, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.60% // Head: 91.78% // Increases project coverage by +1.17% 🎉
Coverage data is based on head (3149544) compared to base (a4c4301).

Patch coverage: 93.36% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #1857 +/- ##
==========================================
+ Coverage 90.60% 91.78% +1.17% 
==========================================
Files 91 91 Lines 47969 56197 +8228 Branches 47969 56197 +8228 ==========================================
+ Hits 43463 51581 +8118 - Misses 4506 4616 +110 
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs96.52% <ø> (+2.87%)⬆️
lightning/src/ln/channel.rs91.30% <80.00%> (+2.56%)⬆️
lightning/src/ln/channelmanager.rs89.40% <82.97%> (+4.29%)⬆️
lightning/src/ln/reload_tests.rs95.50% <96.55%> (+0.25%)⬆️
lightning/src/chain/channelmonitor.rs93.29% <97.67%> (+2.55%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.43%)⬇️
lightning/src/ln/peer_channel_encryptor.rs93.38% <0.00%> (-0.25%)⬇️
lightning/src/ln/reorg_tests.rs100.00% <0.00%> (ø)
lightning/src/ln/payment_tests.rs98.92% <0.00%> (+0.02%)⬆️
lightning/src/ln/monitor_tests.rs99.66% <0.00%> (+0.10%)⬆️
... and 12 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
@valentinewallace
valentinewallace
commented
Nov 21, 2022

Copy link


Copy Markdown

Contributor
Needs rebase
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@dunxen
dunxen
self-requested a review
November 28, 2022 17:44
dunxen
dunxen
reviewed
Nov 29, 2022
View reviewed changes
@dunxendunxen
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 AFAICT. Just nits. One optional.
Comment threadlightning/src/chain/channelmonitor.rs
Outdated

Comment threadlightning/src/ln/reload_tests.rs
Outdated

dunxen
dunxen
previously approved these changes
Dec 5, 2022
View reviewed changes
@TheBlueMatt
Expose the full set of outbound HTLCs from a ChannelMonitor
e142f67
This expands the outbound-HTLC-listing support in `ChannelMonitor`
to include not only the set of outbound HTLCs which have not yet
been resolved but to also include the full set of HTLCs which the
`ChannelMonitor` is currently able to to or has already finalized.
This will be used in the next commit to fail-back HTLCs which were
removed from a channel in the ChannelMonitor but not in a Channel.
Using the existing `get_pending_outbound_htlcs` for this purpose is
subtly broken - if the channel is already closed, an HTLC fail may
have completed on chain and is no longer "pending" to the monitor,
but the fail event is still in the monitor waiting to be handed
back to the `ChannelMonitor` when polled.
@TheBlueMatt
Avoid attempting to forward to a closed chan on stale-data reload
0bb87dd
If, after forwarding a payment to our counterparty, we restart with
a ChannelMonitor update having been persisted, but the
corresponding ChannelManager update was not persisted, we'll still
have the forwarded HTLC in the `forward_htlcs` map on start. This
will cause us to generate a (spurious) `PendingHTLCsForwardable`
event. However, when we go to forward said HTLC, we'll notice the
channel has been closed and leave it up to the `ChannelMontior` to
finalize the HTLC.
This is all fine today - we won't lose any funds, we'll just
generate an excess forwardable event and then fail to forward.
However, in the future when we allow for forward-time channel
changes this could break. Thus, its worth adding tests for this
behavior today, and, while we're at it, removing the spurious
forwardable HTLCs event.
@TheBlueMatt
Fail HTLCs which were removed from a channel but not persisted
dbe4aad
When a channel is force-closed, if a `ChannelMonitor` update is
completed but a `ChannelManager` persist has not yet happened,
HTLCs which were removed in the latest (persisted) `ChannelMonitor`
update will not be failed even though they do not appear in the
commitment transaction which went on chain. This is because the
`ChannelManager` thinks the `ChannelMonitor` is responsible for
them (as it is stale), but the `ChannelMonitor` has no knowledge of
the HTLC at all (as it is not stale).
The fix for this is relatively simple - we need to check for this
specific case and fail back such HTLCs when deserializing a
`ChannelManager`
@TheBlueMatt
TheBlueMatt
commented
Dec 5, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed fixups.
@TheBlueMatt
TheBlueMatt
merged commit f4ab077
into
lightningdevkit:mainDec 5, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen approved these changes
@wpaulinowpaulinowpaulino approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.113

Development

Successfully merging this pull request may close these issues.

5 participants

@TheBlueMatt@codecov-commenter@valentinewallace@dunxen@wpaulino