Correct payment resolution after on chain failure of dust HTLCs - #1691

Merged
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry
Sep 7, 2022
Merged

Correct payment resolution after on chain failure of dust HTLCs#1691
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Sep 2, 2022

Copy link
Copy Markdown
Collaborator

Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.

Fixes#1653.

@TheBlueMattTheBlueMatt added this to the 0.0.111 milestone Sep 2, 2022
@codecov-commenter

codecov-commenter commented Sep 2, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1691 (b0a0955) into main (00f08c9) will decrease coverage by 0.10%.
The diff coverage is 97.59%.

@@ Coverage Diff @@## main #1691 +/- ##
==========================================
- Coverage 90.88% 90.78% -0.11% 
==========================================
Files 85 85 Lines 46220 47408 +1188 Branches 46220 47408 +1188 ==========================================
+ Hits 42007 43038 +1031 - Misses 4213 4370 +157
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs93.58% <93.33%> (-0.02%)⬇️
lightning/src/ln/payment_tests.rs98.89% <98.96%> (+0.01%)⬆️
lightning/src/chain/channelmonitor.rs91.23% <100.00%> (+0.06%)⬆️
lightning/src/ln/priv_short_conf_tests.rs96.84% <100.00%> (+0.24%)⬆️
lightning/src/ln/functional_tests.rs96.83% <0.00%> (-0.29%)⬇️
lightning/src/lib.rs100.00% <0.00%> (ø)
lightning/src/ln/features.rs99.60% <0.00%> (+0.14%)⬆️
lightning/src/ln/msgs.rs87.01% <0.00%> (+1.26%)⬆️
lightning/src/util/time.rs82.55% <0.00%> (+1.30%)⬆️
lightning/src/onion_message/functional_tests.rs98.47% <0.00%> (+2.59%)⬆️
... and 3 more

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

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, test fails as expected on main

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
wpaulino
wpaulino reviewed Sep 6, 2022
View reviewed changes

@wpaulinowpaulino 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. There are a few warnings/failures to fix after the latest push.

Also wrote a commit to create a type alias for the test ChannelManager if you want to include it here, see https://gist.github.com/wpaulino/edc59a12b749452d8bcce2bb0816cee2.

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes
Comment threadlightning/src/chain/channelmonitor.rs
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Oops lol I guess there was a reason the channel was being opened 0conf in the test.

@wpaulino

wpaulino commented Sep 6, 2022

Copy link
Copy Markdown
Contributor

Feel free to squash.

@TheBlueMatt
Correct payment resolution after on chain failure of dust HTLCs
a795e45
Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.
Fixeslightningdevkit#1653.
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@valentinewallace
valentinewallace merged commit e45db2b into lightningdevkit:mainSep 7, 2022
@wpaulinowpaulino mentioned this pull request Sep 9, 2022
Merged
@valentinewallacevalentinewallace mentioned this pull request Sep 9, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@valentinewallacevalentinewallace

Labels

None yet

Projects

None yet

Milestone

0.0.111

Development

Successfully merging this pull request may close these issues.

Dust HTLCs on closed channels are always retryable

4 participants

@TheBlueMatt@codecov-commenter@wpaulino@valentinewallace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e 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

Correct payment resolution after on chain failure of dust HTLCs - #1691

Merged
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry
Sep 7, 2022
Merged

Correct payment resolution after on chain failure of dust HTLCs#1691
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Sep 2, 2022

Copy link
Copy Markdown
Collaborator

Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.

Fixes#1653.

@TheBlueMattTheBlueMatt added this to the 0.0.111 milestone Sep 2, 2022
@codecov-commenter

codecov-commenter commented Sep 2, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1691 (b0a0955) into main (00f08c9) will decrease coverage by 0.10%.
The diff coverage is 97.59%.

@@ Coverage Diff @@## main #1691 +/- ##
==========================================
- Coverage 90.88% 90.78% -0.11% 
==========================================
Files 85 85 Lines 46220 47408 +1188 Branches 46220 47408 +1188 ==========================================
+ Hits 42007 43038 +1031 - Misses 4213 4370 +157
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs93.58% <93.33%> (-0.02%)⬇️
lightning/src/ln/payment_tests.rs98.89% <98.96%> (+0.01%)⬆️
lightning/src/chain/channelmonitor.rs91.23% <100.00%> (+0.06%)⬆️
lightning/src/ln/priv_short_conf_tests.rs96.84% <100.00%> (+0.24%)⬆️
lightning/src/ln/functional_tests.rs96.83% <0.00%> (-0.29%)⬇️
lightning/src/lib.rs100.00% <0.00%> (ø)
lightning/src/ln/features.rs99.60% <0.00%> (+0.14%)⬆️
lightning/src/ln/msgs.rs87.01% <0.00%> (+1.26%)⬆️
lightning/src/util/time.rs82.55% <0.00%> (+1.30%)⬆️
lightning/src/onion_message/functional_tests.rs98.47% <0.00%> (+2.59%)⬆️
... and 3 more

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

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, test fails as expected on main

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
wpaulino
wpaulino reviewed Sep 6, 2022
View reviewed changes

@wpaulinowpaulino 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. There are a few warnings/failures to fix after the latest push.

Also wrote a commit to create a type alias for the test ChannelManager if you want to include it here, see https://gist.github.com/wpaulino/edc59a12b749452d8bcce2bb0816cee2.

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes
Comment threadlightning/src/chain/channelmonitor.rs
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Oops lol I guess there was a reason the channel was being opened 0conf in the test.

@wpaulino

wpaulino commented Sep 6, 2022

Copy link
Copy Markdown
Contributor

Feel free to squash.

@TheBlueMatt
Correct payment resolution after on chain failure of dust HTLCs
a795e45
Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.
Fixeslightningdevkit#1653.
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@valentinewallace
valentinewallace merged commit e45db2b into lightningdevkit:mainSep 7, 2022
@wpaulinowpaulino mentioned this pull request Sep 9, 2022
Merged
@valentinewallacevalentinewallace mentioned this pull request Sep 9, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@valentinewallacevalentinewallace

Labels

None yet

Projects

None yet

Milestone

0.0.111

Development

Successfully merging this pull request may close these issues.

Dust HTLCs on closed channels are always retryable

4 participants

@TheBlueMatt@codecov-commenter@wpaulino@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

Correct payment resolution after on chain failure of dust HTLCs - #1691

Merged
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry
Sep 7, 2022
Merged

Correct payment resolution after on chain failure of dust HTLCs#1691
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Sep 2, 2022

Copy link
Copy Markdown
Collaborator

Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.

Fixes#1653.

@TheBlueMattTheBlueMatt added this to the 0.0.111 milestone Sep 2, 2022
@codecov-commenter

codecov-commenter commented Sep 2, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1691 (b0a0955) into main (00f08c9) will decrease coverage by 0.10%.
The diff coverage is 97.59%.

@@ Coverage Diff @@## main #1691 +/- ##
==========================================
- Coverage 90.88% 90.78% -0.11% 
==========================================
Files 85 85 Lines 46220 47408 +1188 Branches 46220 47408 +1188 ==========================================
+ Hits 42007 43038 +1031 - Misses 4213 4370 +157
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs93.58% <93.33%> (-0.02%)⬇️
lightning/src/ln/payment_tests.rs98.89% <98.96%> (+0.01%)⬆️
lightning/src/chain/channelmonitor.rs91.23% <100.00%> (+0.06%)⬆️
lightning/src/ln/priv_short_conf_tests.rs96.84% <100.00%> (+0.24%)⬆️
lightning/src/ln/functional_tests.rs96.83% <0.00%> (-0.29%)⬇️
lightning/src/lib.rs100.00% <0.00%> (ø)
lightning/src/ln/features.rs99.60% <0.00%> (+0.14%)⬆️
lightning/src/ln/msgs.rs87.01% <0.00%> (+1.26%)⬆️
lightning/src/util/time.rs82.55% <0.00%> (+1.30%)⬆️
lightning/src/onion_message/functional_tests.rs98.47% <0.00%> (+2.59%)⬆️
... and 3 more

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

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, test fails as expected on main

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
wpaulino
wpaulino reviewed Sep 6, 2022
View reviewed changes

@wpaulinowpaulino 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. There are a few warnings/failures to fix after the latest push.

Also wrote a commit to create a type alias for the test ChannelManager if you want to include it here, see https://gist.github.com/wpaulino/edc59a12b749452d8bcce2bb0816cee2.

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes
Comment threadlightning/src/chain/channelmonitor.rs
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Oops lol I guess there was a reason the channel was being opened 0conf in the test.

@wpaulino

wpaulino commented Sep 6, 2022

Copy link
Copy Markdown
Contributor

Feel free to squash.

@TheBlueMatt
Correct payment resolution after on chain failure of dust HTLCs
a795e45
Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.
Fixeslightningdevkit#1653.
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@valentinewallace
valentinewallace merged commit e45db2b into lightningdevkit:mainSep 7, 2022
@wpaulinowpaulino mentioned this pull request Sep 9, 2022
Merged
@valentinewallacevalentinewallace mentioned this pull request Sep 9, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@valentinewallacevalentinewallace

Labels

None yet

Projects

None yet

Milestone

0.0.111

Development

Successfully merging this pull request may close these issues.

Dust HTLCs on closed channels are always retryable

4 participants

@TheBlueMatt@codecov-commenter@wpaulino@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 \u003e 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

Correct payment resolution after on chain failure of dust HTLCs - #1691

Merged
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry
Sep 7, 2022
Merged

Correct payment resolution after on chain failure of dust HTLCs#1691
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Sep 2, 2022

Copy link
Copy Markdown
Collaborator

Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.

Fixes#1653.

@TheBlueMattTheBlueMatt added this to the 0.0.111 milestone Sep 2, 2022
@codecov-commenter

codecov-commenter commented Sep 2, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1691 (b0a0955) into main (00f08c9) will decrease coverage by 0.10%.
The diff coverage is 97.59%.

@@ Coverage Diff @@## main #1691 +/- ##
==========================================
- Coverage 90.88% 90.78% -0.11% 
==========================================
Files 85 85 Lines 46220 47408 +1188 Branches 46220 47408 +1188 ==========================================
+ Hits 42007 43038 +1031 - Misses 4213 4370 +157
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs93.58% <93.33%> (-0.02%)⬇️
lightning/src/ln/payment_tests.rs98.89% <98.96%> (+0.01%)⬆️
lightning/src/chain/channelmonitor.rs91.23% <100.00%> (+0.06%)⬆️
lightning/src/ln/priv_short_conf_tests.rs96.84% <100.00%> (+0.24%)⬆️
lightning/src/ln/functional_tests.rs96.83% <0.00%> (-0.29%)⬇️
lightning/src/lib.rs100.00% <0.00%> (ø)
lightning/src/ln/features.rs99.60% <0.00%> (+0.14%)⬆️
lightning/src/ln/msgs.rs87.01% <0.00%> (+1.26%)⬆️
lightning/src/util/time.rs82.55% <0.00%> (+1.30%)⬆️
lightning/src/onion_message/functional_tests.rs98.47% <0.00%> (+2.59%)⬆️
... and 3 more

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

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, test fails as expected on main

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
wpaulino
wpaulino reviewed Sep 6, 2022
View reviewed changes

@wpaulinowpaulino 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. There are a few warnings/failures to fix after the latest push.

Also wrote a commit to create a type alias for the test ChannelManager if you want to include it here, see https://gist.github.com/wpaulino/edc59a12b749452d8bcce2bb0816cee2.

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes
Comment threadlightning/src/chain/channelmonitor.rs
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Oops lol I guess there was a reason the channel was being opened 0conf in the test.

@wpaulino

wpaulino commented Sep 6, 2022

Copy link
Copy Markdown
Contributor

Feel free to squash.

@TheBlueMatt
Correct payment resolution after on chain failure of dust HTLCs
a795e45
Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.
Fixeslightningdevkit#1653.
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@valentinewallace
valentinewallace merged commit e45db2b into lightningdevkit:mainSep 7, 2022
@wpaulinowpaulino mentioned this pull request Sep 9, 2022
Merged
@valentinewallacevalentinewallace mentioned this pull request Sep 9, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@valentinewallacevalentinewallace

Labels

None yet

Projects

None yet

Milestone

0.0.111

Development

Successfully merging this pull request may close these issues.

Dust HTLCs on closed channels are always retryable

4 participants

@TheBlueMatt@codecov-commenter@wpaulino@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

Correct payment resolution after on chain failure of dust HTLCs - #1691

Merged
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry
Sep 7, 2022
Merged

Correct payment resolution after on chain failure of dust HTLCs#1691
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Sep 2, 2022

Copy link
Copy Markdown
Collaborator

Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.

Fixes#1653.

@TheBlueMattTheBlueMatt added this to the 0.0.111 milestone Sep 2, 2022
@codecov-commenter

codecov-commenter commented Sep 2, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1691 (b0a0955) into main (00f08c9) will decrease coverage by 0.10%.
The diff coverage is 97.59%.

@@ Coverage Diff @@## main #1691 +/- ##
==========================================
- Coverage 90.88% 90.78% -0.11% 
==========================================
Files 85 85 Lines 46220 47408 +1188 Branches 46220 47408 +1188 ==========================================
+ Hits 42007 43038 +1031 - Misses 4213 4370 +157
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs93.58% <93.33%> (-0.02%)⬇️
lightning/src/ln/payment_tests.rs98.89% <98.96%> (+0.01%)⬆️
lightning/src/chain/channelmonitor.rs91.23% <100.00%> (+0.06%)⬆️
lightning/src/ln/priv_short_conf_tests.rs96.84% <100.00%> (+0.24%)⬆️
lightning/src/ln/functional_tests.rs96.83% <0.00%> (-0.29%)⬇️
lightning/src/lib.rs100.00% <0.00%> (ø)
lightning/src/ln/features.rs99.60% <0.00%> (+0.14%)⬆️
lightning/src/ln/msgs.rs87.01% <0.00%> (+1.26%)⬆️
lightning/src/util/time.rs82.55% <0.00%> (+1.30%)⬆️
lightning/src/onion_message/functional_tests.rs98.47% <0.00%> (+2.59%)⬆️
... and 3 more

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

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, test fails as expected on main

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
wpaulino
wpaulino reviewed Sep 6, 2022
View reviewed changes

@wpaulinowpaulino 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. There are a few warnings/failures to fix after the latest push.

Also wrote a commit to create a type alias for the test ChannelManager if you want to include it here, see https://gist.github.com/wpaulino/edc59a12b749452d8bcce2bb0816cee2.

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes
Comment threadlightning/src/chain/channelmonitor.rs
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Oops lol I guess there was a reason the channel was being opened 0conf in the test.

@wpaulino

wpaulino commented Sep 6, 2022

Copy link
Copy Markdown
Contributor

Feel free to squash.

@TheBlueMatt
Correct payment resolution after on chain failure of dust HTLCs
a795e45
Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.
Fixeslightningdevkit#1653.
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@valentinewallace
valentinewallace merged commit e45db2b into lightningdevkit:mainSep 7, 2022
@wpaulinowpaulino mentioned this pull request Sep 9, 2022
Merged
@valentinewallacevalentinewallace mentioned this pull request Sep 9, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@valentinewallacevalentinewallace

Labels

None yet

Projects

None yet

Milestone

0.0.111

Development

Successfully merging this pull request may close these issues.

Dust HTLCs on closed channels are always retryable

4 participants

@TheBlueMatt@codecov-commenter@wpaulino@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

Correct payment resolution after on chain failure of dust HTLCs - #1691

Merged
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry
Sep 7, 2022
Merged

Correct payment resolution after on chain failure of dust HTLCs#1691
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Sep 2, 2022

Copy link
Copy Markdown
Collaborator

Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.

Fixes#1653.

@TheBlueMattTheBlueMatt added this to the 0.0.111 milestone Sep 2, 2022
@codecov-commenter

codecov-commenter commented Sep 2, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1691 (b0a0955) into main (00f08c9) will decrease coverage by 0.10%.
The diff coverage is 97.59%.

@@ Coverage Diff @@## main #1691 +/- ##
==========================================
- Coverage 90.88% 90.78% -0.11% 
==========================================
Files 85 85 Lines 46220 47408 +1188 Branches 46220 47408 +1188 ==========================================
+ Hits 42007 43038 +1031 - Misses 4213 4370 +157
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs93.58% <93.33%> (-0.02%)⬇️
lightning/src/ln/payment_tests.rs98.89% <98.96%> (+0.01%)⬆️
lightning/src/chain/channelmonitor.rs91.23% <100.00%> (+0.06%)⬆️
lightning/src/ln/priv_short_conf_tests.rs96.84% <100.00%> (+0.24%)⬆️
lightning/src/ln/functional_tests.rs96.83% <0.00%> (-0.29%)⬇️
lightning/src/lib.rs100.00% <0.00%> (ø)
lightning/src/ln/features.rs99.60% <0.00%> (+0.14%)⬆️
lightning/src/ln/msgs.rs87.01% <0.00%> (+1.26%)⬆️
lightning/src/util/time.rs82.55% <0.00%> (+1.30%)⬆️
lightning/src/onion_message/functional_tests.rs98.47% <0.00%> (+2.59%)⬆️
... and 3 more

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

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, test fails as expected on main

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
wpaulino
wpaulino reviewed Sep 6, 2022
View reviewed changes

@wpaulinowpaulino 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. There are a few warnings/failures to fix after the latest push.

Also wrote a commit to create a type alias for the test ChannelManager if you want to include it here, see https://gist.github.com/wpaulino/edc59a12b749452d8bcce2bb0816cee2.

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes
Comment threadlightning/src/chain/channelmonitor.rs
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Oops lol I guess there was a reason the channel was being opened 0conf in the test.

@wpaulino

wpaulino commented Sep 6, 2022

Copy link
Copy Markdown
Contributor

Feel free to squash.

@TheBlueMatt
Correct payment resolution after on chain failure of dust HTLCs
a795e45
Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.
Fixeslightningdevkit#1653.
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@valentinewallace
valentinewallace merged commit e45db2b into lightningdevkit:mainSep 7, 2022
@wpaulinowpaulino mentioned this pull request Sep 9, 2022
Merged
@valentinewallacevalentinewallace mentioned this pull request Sep 9, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@valentinewallacevalentinewallace

Labels

None yet

Projects

None yet

Milestone

0.0.111

Development

Successfully merging this pull request may close these issues.

Dust HTLCs on closed channels are always retryable

4 participants

@TheBlueMatt@codecov-commenter@wpaulino@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

Correct payment resolution after on chain failure of dust HTLCs - #1691

Merged
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry
Sep 7, 2022
Merged

Correct payment resolution after on chain failure of dust HTLCs#1691
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Sep 2, 2022

Copy link
Copy Markdown
Collaborator

Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.

Fixes#1653.

@TheBlueMattTheBlueMatt added this to the 0.0.111 milestone Sep 2, 2022
@codecov-commenter

codecov-commenter commented Sep 2, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1691 (b0a0955) into main (00f08c9) will decrease coverage by 0.10%.
The diff coverage is 97.59%.

@@ Coverage Diff @@## main #1691 +/- ##
==========================================
- Coverage 90.88% 90.78% -0.11% 
==========================================
Files 85 85 Lines 46220 47408 +1188 Branches 46220 47408 +1188 ==========================================
+ Hits 42007 43038 +1031 - Misses 4213 4370 +157
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs93.58% <93.33%> (-0.02%)⬇️
lightning/src/ln/payment_tests.rs98.89% <98.96%> (+0.01%)⬆️
lightning/src/chain/channelmonitor.rs91.23% <100.00%> (+0.06%)⬆️
lightning/src/ln/priv_short_conf_tests.rs96.84% <100.00%> (+0.24%)⬆️
lightning/src/ln/functional_tests.rs96.83% <0.00%> (-0.29%)⬇️
lightning/src/lib.rs100.00% <0.00%> (ø)
lightning/src/ln/features.rs99.60% <0.00%> (+0.14%)⬆️
lightning/src/ln/msgs.rs87.01% <0.00%> (+1.26%)⬆️
lightning/src/util/time.rs82.55% <0.00%> (+1.30%)⬆️
lightning/src/onion_message/functional_tests.rs98.47% <0.00%> (+2.59%)⬆️
... and 3 more

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

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, test fails as expected on main

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
wpaulino
wpaulino reviewed Sep 6, 2022
View reviewed changes

@wpaulinowpaulino 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. There are a few warnings/failures to fix after the latest push.

Also wrote a commit to create a type alias for the test ChannelManager if you want to include it here, see https://gist.github.com/wpaulino/edc59a12b749452d8bcce2bb0816cee2.

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes
Comment threadlightning/src/chain/channelmonitor.rs
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Oops lol I guess there was a reason the channel was being opened 0conf in the test.

@wpaulino

wpaulino commented Sep 6, 2022

Copy link
Copy Markdown
Contributor

Feel free to squash.

@TheBlueMatt
Correct payment resolution after on chain failure of dust HTLCs
a795e45
Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.
Fixeslightningdevkit#1653.
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@valentinewallace
valentinewallace merged commit e45db2b into lightningdevkit:mainSep 7, 2022
@wpaulinowpaulino mentioned this pull request Sep 9, 2022
Merged
@valentinewallacevalentinewallace mentioned this pull request Sep 9, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@valentinewallacevalentinewallace

Labels

None yet

Projects

None yet

Milestone

0.0.111

Development

Successfully merging this pull request may close these issues.

Dust HTLCs on closed channels are always retryable

4 participants

@TheBlueMatt@codecov-commenter@wpaulino@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

Correct payment resolution after on chain failure of dust HTLCs - #1691

Merged
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry
Sep 7, 2022
Merged

Correct payment resolution after on chain failure of dust HTLCs#1691
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-08-dust-retry

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Sep 2, 2022

Copy link
Copy Markdown
Collaborator

Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.

Fixes#1653.

@TheBlueMattTheBlueMatt added this to the 0.0.111 milestone Sep 2, 2022
@codecov-commenter

codecov-commenter commented Sep 2, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1691 (b0a0955) into main (00f08c9) will decrease coverage by 0.10%.
The diff coverage is 97.59%.

@@ Coverage Diff @@## main #1691 +/- ##
==========================================
- Coverage 90.88% 90.78% -0.11% 
==========================================
Files 85 85 Lines 46220 47408 +1188 Branches 46220 47408 +1188 ==========================================
+ Hits 42007 43038 +1031 - Misses 4213 4370 +157
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs93.58% <93.33%> (-0.02%)⬇️
lightning/src/ln/payment_tests.rs98.89% <98.96%> (+0.01%)⬆️
lightning/src/chain/channelmonitor.rs91.23% <100.00%> (+0.06%)⬆️
lightning/src/ln/priv_short_conf_tests.rs96.84% <100.00%> (+0.24%)⬆️
lightning/src/ln/functional_tests.rs96.83% <0.00%> (-0.29%)⬇️
lightning/src/lib.rs100.00% <0.00%> (ø)
lightning/src/ln/features.rs99.60% <0.00%> (+0.14%)⬆️
lightning/src/ln/msgs.rs87.01% <0.00%> (+1.26%)⬆️
lightning/src/util/time.rs82.55% <0.00%> (+1.30%)⬆️
lightning/src/onion_message/functional_tests.rs98.47% <0.00%> (+2.59%)⬆️
... and 3 more

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

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, test fails as expected on main

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs Outdated
Comment threadlightning/src/ln/payment_tests.rs
wpaulino
wpaulino reviewed Sep 6, 2022
View reviewed changes

@wpaulinowpaulino 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. There are a few warnings/failures to fix after the latest push.

Also wrote a commit to create a type alias for the test ChannelManager if you want to include it here, see https://gist.github.com/wpaulino/edc59a12b749452d8bcce2bb0816cee2.

valentinewallace
valentinewallace reviewed Sep 6, 2022
View reviewed changes
Comment threadlightning/src/chain/channelmonitor.rs
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Oops lol I guess there was a reason the channel was being opened 0conf in the test.

@wpaulino

wpaulino commented Sep 6, 2022

Copy link
Copy Markdown
Contributor

Feel free to squash.

@TheBlueMatt
Correct payment resolution after on chain failure of dust HTLCs
a795e45
Previously, we wouldn't mark a dust HTLC as permanently resolved if
the commitment transaction went on chain. This resulted in us
always considering the HTLC as pending on restart, when we load the
pending payments set from the monitors.
Fixeslightningdevkit#1653.
@TheBlueMatt

TheBlueMatt commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further changes.

@valentinewallace
valentinewallace merged commit e45db2b into lightningdevkit:mainSep 7, 2022
@wpaulinowpaulino mentioned this pull request Sep 9, 2022
Merged
@valentinewallacevalentinewallace mentioned this pull request Sep 9, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@valentinewallacevalentinewallace

Labels

None yet

Projects

None yet

Milestone

0.0.111

Development

Successfully merging this pull request may close these issues.

Dust HTLCs on closed channels are always retryable

4 participants

@TheBlueMatt@codecov-commenter@wpaulino@valentinewallace