Lean on the holding cell when batch-forwarding/failing HTLCs - #1863

Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-11-holding-cell-batch-update
Dec 7, 2022
Merged

Lean on the holding cell when batch-forwarding/failing HTLCs#1863
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-11-holding-cell-batch-update

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Nov 19, 2022

Copy link
Copy Markdown
Collaborator

When we batch HTLC updates, we currently do the explicit queueing plus the commitment generation in the ChannelManager. This is a bit strange as its ultimately really a Channel responsibility to generate commitments at the correct time, with the abstraction leaking into ChannelManager with the send_htlc and get_update_fail_htlc method docs having clear comments about how send_commitment MUST be called prior to calling other Channel methods.

Luckily Channel already has an update queue - the holding cell. Thus, we can trivially rewrite the batch update logic as inserting the desired updates into the holding cell and then asking all channels to clear their holding cells.

@codecov-commenter

codecov-commenter commented Nov 19, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Base: 90.69% // Head: 90.74% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (65d9d0f) compared to base (52edb35).
Patch coverage: 83.05% of modified lines in pull request are covered.

❗ Current head 65d9d0f differs from pull request most recent head 5e8193f. Consider uploading reports for the commit 5e8193f to get more accurate results

Additional details and impacted files
@@ Coverage Diff @@## main #1863 +/- ##
==========================================
+ Coverage 90.69% 90.74% +0.05% 
==========================================
Files 91 91 Lines 48408 48326 -82 Branches 48408 48326 -82 ==========================================
- Hits 43902 43853 -49 + Misses 4506 4473 -33
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs86.57% <70.96%> (+0.29%)⬆️
lightning/src/ln/channel.rs88.98% <96.42%> (+0.13%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.86%)⬇️
lightning/src/chain/channelmonitor.rs90.79% <0.00%> (+0.05%)⬆️
lightning/src/ln/functional_tests.rs97.14% <0.00%> (+0.09%)⬆️
lightning/src/util/events.rs25.22% <0.00%> (+0.22%)⬆️
lightning/src/ln/peer_channel_encryptor.rs93.62% <0.00%> (+0.24%)⬆️

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.

@TheBlueMatt

TheBlueMatt commented Nov 21, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Pushed two additional commits to lean on the holding cell when doing fee updates as well...that sweet, sweet -100 total LoC.

@TheBlueMatt

TheBlueMatt commented Nov 21, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Note that the fuzzing failure here is resolved in #1859. Will leave this PR as free-standing but that will need to land first.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-11-holding-cell-batch-update branch from 9d2affd to 2ee043cCompareNovember 22, 2022 01:10
@TheBlueMatt

TheBlueMatt commented Nov 22, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Rebased without changes, CI should pass now.

valentinewallace
valentinewallace reviewed Nov 23, 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.

Nice cleanup, no major feedback. Will take another look next week

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs
Ok(Some(res))
}

/// Creates a signed commitment transaction to send to the remote peer.

@valentinewallacevalentinewallaceNov 23, 2022

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.

nit: I think ideally this would've been removed in the previous-previous commit

@TheBlueMattTheBlueMattNov 25, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

send_update_fee_and_commit is removed in the "updating fees" commit, and it relies on send_commitment, so we can only remove it in the last commit.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-11-holding-cell-batch-update branch from 2ee043c to 2409913CompareNovember 25, 2022 20:30
@valentinewallace

valentinewallace commented Nov 28, 2022

Copy link
Copy Markdown
Contributor

(Maybe for follow-up) would it further simplify things to do the same batching in ChannelManager::send_payment_along_path?

@TheBlueMatt

TheBlueMatt commented Nov 28, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Yea, that one's substantially more complicated, but work there is coming soon (tm).

valentinewallace
valentinewallace reviewed Nov 28, 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.

Basically LGTM, good time for a second reviewer

Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMattforce-pushed the 2022-11-holding-cell-batch-update branch from 2409913 to d30db59CompareNovember 28, 2022 23:57
@TheBlueMatt

TheBlueMatt commented Nov 30, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Rebased.

@TheBlueMatt
TheBlueMattforce-pushed the 2022-11-holding-cell-batch-update branch 2 times, most recently from 74a1070 to 2233650CompareDecember 2, 2022 01:10
@TheBlueMatt

TheBlueMatt commented Dec 2, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Rebased. Took this opportunity to squash since there were a lot of fixups.

tnull
tnull reviewed Dec 2, 2022
View reviewed changes

@tnulltnull left a comment •
edited
Loading

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.

Did a quick first pass to get a high-level overview. These are predominately doc nits, main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.

Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
tnull
tnull reviewed Dec 2, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs Outdated
@TheBlueMatt
TheBlueMattforce-pushed the 2022-11-holding-cell-batch-update branch from 2233650 to 719bf79CompareDecember 2, 2022 18:56
@TheBlueMatt

TheBlueMatt commented Dec 2, 2022

Copy link
Copy Markdown
CollaboratorAuthor

main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.

Yea, most of this code predates me knowing you could even do links in docs, sooooo...

@TheBlueMatt
TheBlueMattforce-pushed the 2022-11-holding-cell-batch-update branch from 719bf79 to 67d3ac8CompareDecember 2, 2022 19:01
@TheBlueMattTheBlueMatt mentioned this pull request Dec 3, 2022
Merged
tnull
tnull reviewed Dec 5, 2022
View reviewed changes

@tnulltnull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, just a few nits/questions.

Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
@TheBlueMatt
TheBlueMattforce-pushed the 2022-11-holding-cell-batch-update branch from 65d9d0f to 5e8193fCompareDecember 5, 2022 18:01
tnull
tnull reviewed Dec 6, 2022
View reviewed changes

@tnulltnull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Feel free to squash, I think. Should also alleviate the CI hangup..

valentinewallace
valentinewallace reviewed Dec 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.

ACK after squash

@TheBlueMatt
Lean on the holding cell when batch-forwarding/failing HTLCs
cae7c81
When we batch HTLC updates, we currently do the explicit queueing
plus the commitment generation in the `ChannelManager`. This is a
bit strange as its ultimately really a `Channel` responsibility to
generate commitments at the correct time, with the abstraction
leaking into `ChannelManager` with the `send_htlc` and
`get_update_fail_htlc` method docs having clear comments about
how `send_commitment` MUST be called prior to calling other
`Channel` methods.
Luckily `Channel` already has an update queue - the holding cell.
Thus, we can trivially rewrite the batch update logic as inserting
the desired updates into the holding cell and then asking all
channels to clear their holding cells.
@TheBlueMatt
Free the holding cells during background timer ticks
1531378
We currently free the channel holding cells in
`get_and_clear_pending_msg_events`, blocking outbound messages
while we do so. This is fine, but may block the message pipeline
longer than we need to. In the next commit we'll push
timer-originating channel fee updates out through the holding cell
pipeline, leaning more on that freeing in the future.
Thus, to avoid a regression in message time, here we clear the
holding cell after processing all timer events. This also avoids
needing to change tests in the next commit.
@TheBlueMatt
Lean on the holding cell for commitments when updating fees
aa9630b
Like the previous commit, here we update the update_fee+commit
logic to simply push the fee update into the holding cell and then
use the standard holding-cell-freeing codepaths to actually send
the commitment update. This removes a substantial amount of code,
reducing redundant codepaths and keeping channel state machine
logic in channel.rs.
@TheBlueMatt
Correct confusing docs on Channel methods
1833070
The methods return `Ok(())` always, they just happen to never
return in the case of a duplicate claim if debug assertions are
enabled.
@TheBlueMatt
TheBlueMattforce-pushed the 2022-11-holding-cell-batch-update branch from 5e8193f to 1833070CompareDecember 6, 2022 18:18
@TheBlueMatt

TheBlueMatt commented Dec 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without change.

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

Reviewers

@tnulltnulltnull approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@tnulltnull

@valentinewallacevalentinewallace

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@tnull
, '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
Lean on the holding cell when batch-forwarding/failing HTLCs - #1863
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Dec 7, 2022
Merged
Lean on the holding cell when batch-forwarding/failing HTLCs#1863
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 19, 2022

Copy link


Copy Markdown

Collaborator
When we batch HTLC updates, we currently do the explicit queueing plus the commitment generation in the ChannelManager. This is a bit strange as its ultimately really a Channel responsibility to generate commitments at the correct time, with the abstraction leaking into ChannelManager with the send_htlc and get_update_fail_htlc method docs having clear comments about how send_commitment MUST be called prior to calling other Channel methods.
Luckily Channel already has an update queue - the holding cell. Thus, we can trivially rewrite the batch update logic as inserting the desired updates into the holding cell and then asking all channels to clear their holding cells.
@codecov-commenter
codecov-commenter
commented
Nov 19, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.69% // Head: 90.74% // Increases project coverage by +0.05% 🎉
Coverage data is based on head (65d9d0f) compared to base (52edb35).

Patch coverage: 83.05% of modified lines in pull request are covered.
❗ Current head 65d9d0f differs from pull request most recent head 5e8193f. Consider uploading reports for the commit 5e8193f to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## main #1863 +/- ##
==========================================
+ Coverage 90.69% 90.74% +0.05% 
==========================================
Files 91 91 Lines 48408 48326 -82 Branches 48408 48326 -82 ==========================================
- Hits 43902 43853 -49 + Misses 4506 4473 -33 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs86.57% <70.96%> (+0.29%)⬆️
lightning/src/ln/channel.rs88.98% <96.42%> (+0.13%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.86%)⬇️
lightning/src/chain/channelmonitor.rs90.79% <0.00%> (+0.05%)⬆️
lightning/src/ln/functional_tests.rs97.14% <0.00%> (+0.09%)⬆️
lightning/src/util/events.rs25.22% <0.00%> (+0.22%)⬆️
lightning/src/ln/peer_channel_encryptor.rs93.62% <0.00%> (+0.24%)⬆️
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.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Pushed two additional commits to lean on the holding cell when doing fee updates as well...that sweet, sweet -100 total LoC.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Note that the fuzzing failure here is resolved in #1859. Will leave this PR as free-standing but that will need to land first.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
9d2affd to
2ee043cCompareNovember 22, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Nov 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased without changes, CI should pass now.
valentinewallace
valentinewallace
reviewed
Nov 23, 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.

Nice cleanup, no major feedback. Will take another look next week
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
 Ok(Some(res))
	}

	/// Creates a signed commitment transaction to send to the remote peer.
@valentinewallacevalentinewallaceNov 23, 2022

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.

nit: I think ideally this would've been removed in the previous-previous commit
@TheBlueMattTheBlueMattNov 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

send_update_fee_and_commit is removed in the "updating fees" commit, and it relies on send_commitment, so we can only remove it in the last commit.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2ee043c to
2409913CompareNovember 25, 2022 20:30
@valentinewallace
valentinewallace
commented
Nov 28, 2022

Copy link


Copy Markdown

Contributor
(Maybe for follow-up) would it further simplify things to do the same batching in ChannelManager::send_payment_along_path?
@TheBlueMatt
TheBlueMatt
commented
Nov 28, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Yea, that one's substantially more complicated, but work there is coming soon (tm).
valentinewallace
valentinewallace
reviewed
Nov 28, 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.

Basically LGTM, good time for a second reviewer
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2409913 to
d30db59CompareNovember 28, 2022 23:57
@TheBlueMatt
TheBlueMatt
commented
Nov 30, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
2 times, most recently
from
74a1070 to
2233650CompareDecember 2, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Took this opportunity to squash since there were a lot of fixups.
tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
@tnulltnull
left a comment
•

edited

Loading

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.

Did a quick first pass to get a high-level overview. These are predominately doc nits, main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2233650 to
719bf79CompareDecember 2, 2022 18:56
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Yea, most of this code predates me knowing you could even do links in docs, sooooo...
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
719bf79 to
67d3ac8CompareDecember 2, 2022 19:01
@TheBlueMattTheBlueMatt
mentioned this pull request
Dec 3, 2022
 Merged

tnull
tnull
reviewed
Dec 5, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good, just a few nits/questions.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
65d9d0f to
5e8193fCompareDecember 5, 2022 18:01
tnull
tnull
reviewed
Dec 6, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Feel free to squash, I think. Should also alleviate the CI hangup..
valentinewallace
valentinewallace
reviewed
Dec 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.

ACK after squash
@TheBlueMatt
Lean on the holding cell when batch-forwarding/failing HTLCs
cae7c81
When we batch HTLC updates, we currently do the explicit queueing
plus the commitment generation in the `ChannelManager`. This is a
bit strange as its ultimately really a `Channel` responsibility to
generate commitments at the correct time, with the abstraction
leaking into `ChannelManager` with the `send_htlc` and
`get_update_fail_htlc` method docs having clear comments about
how `send_commitment` MUST be called prior to calling other
`Channel` methods.
Luckily `Channel` already has an update queue - the holding cell.
Thus, we can trivially rewrite the batch update logic as inserting
the desired updates into the holding cell and then asking all
channels to clear their holding cells.
@TheBlueMatt
Free the holding cells during background timer ticks
1531378
We currently free the channel holding cells in
`get_and_clear_pending_msg_events`, blocking outbound messages
while we do so. This is fine, but may block the message pipeline
longer than we need to. In the next commit we'll push
timer-originating channel fee updates out through the holding cell
pipeline, leaning more on that freeing in the future.
Thus, to avoid a regression in message time, here we clear the
holding cell after processing all timer events. This also avoids
needing to change tests in the next commit.
@TheBlueMatt
Lean on the holding cell for commitments when updating fees
aa9630b
Like the previous commit, here we update the update_fee+commit
logic to simply push the fee update into the holding cell and then
use the standard holding-cell-freeing codepaths to actually send
the commitment update. This removes a substantial amount of code,
reducing redundant codepaths and keeping channel state machine
logic in channel.rs.
@TheBlueMatt
Correct confusing docs on Channel methods
1833070
The methods return `Ok(())` always, they just happen to never
return in the case of a duplicate claim if debug assertions are
enabled.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
5e8193f to
1833070CompareDecember 6, 2022 18:18
@TheBlueMatt
TheBlueMatt
commented
Dec 6, 2022

Copy link


Copy Markdown

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

Reviewers

@tnulltnulltnull approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@tnulltnull
@valentinewallacevalentinewallace

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@tnull

, '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
Lean on the holding cell when batch-forwarding/failing HTLCs - #1863
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Dec 7, 2022
Merged
Lean on the holding cell when batch-forwarding/failing HTLCs#1863
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 19, 2022

Copy link


Copy Markdown

Collaborator
When we batch HTLC updates, we currently do the explicit queueing plus the commitment generation in the ChannelManager. This is a bit strange as its ultimately really a Channel responsibility to generate commitments at the correct time, with the abstraction leaking into ChannelManager with the send_htlc and get_update_fail_htlc method docs having clear comments about how send_commitment MUST be called prior to calling other Channel methods.
Luckily Channel already has an update queue - the holding cell. Thus, we can trivially rewrite the batch update logic as inserting the desired updates into the holding cell and then asking all channels to clear their holding cells.
@codecov-commenter
codecov-commenter
commented
Nov 19, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.69% // Head: 90.74% // Increases project coverage by +0.05% 🎉
Coverage data is based on head (65d9d0f) compared to base (52edb35).

Patch coverage: 83.05% of modified lines in pull request are covered.
❗ Current head 65d9d0f differs from pull request most recent head 5e8193f. Consider uploading reports for the commit 5e8193f to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## main #1863 +/- ##
==========================================
+ Coverage 90.69% 90.74% +0.05% 
==========================================
Files 91 91 Lines 48408 48326 -82 Branches 48408 48326 -82 ==========================================
- Hits 43902 43853 -49 + Misses 4506 4473 -33 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs86.57% <70.96%> (+0.29%)⬆️
lightning/src/ln/channel.rs88.98% <96.42%> (+0.13%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.86%)⬇️
lightning/src/chain/channelmonitor.rs90.79% <0.00%> (+0.05%)⬆️
lightning/src/ln/functional_tests.rs97.14% <0.00%> (+0.09%)⬆️
lightning/src/util/events.rs25.22% <0.00%> (+0.22%)⬆️
lightning/src/ln/peer_channel_encryptor.rs93.62% <0.00%> (+0.24%)⬆️
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.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Pushed two additional commits to lean on the holding cell when doing fee updates as well...that sweet, sweet -100 total LoC.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Note that the fuzzing failure here is resolved in #1859. Will leave this PR as free-standing but that will need to land first.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
9d2affd to
2ee043cCompareNovember 22, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Nov 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased without changes, CI should pass now.
valentinewallace
valentinewallace
reviewed
Nov 23, 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.

Nice cleanup, no major feedback. Will take another look next week
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
 Ok(Some(res))
	}

	/// Creates a signed commitment transaction to send to the remote peer.
@valentinewallacevalentinewallaceNov 23, 2022

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.

nit: I think ideally this would've been removed in the previous-previous commit
@TheBlueMattTheBlueMattNov 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

send_update_fee_and_commit is removed in the "updating fees" commit, and it relies on send_commitment, so we can only remove it in the last commit.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2ee043c to
2409913CompareNovember 25, 2022 20:30
@valentinewallace
valentinewallace
commented
Nov 28, 2022

Copy link


Copy Markdown

Contributor
(Maybe for follow-up) would it further simplify things to do the same batching in ChannelManager::send_payment_along_path?
@TheBlueMatt
TheBlueMatt
commented
Nov 28, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Yea, that one's substantially more complicated, but work there is coming soon (tm).
valentinewallace
valentinewallace
reviewed
Nov 28, 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.

Basically LGTM, good time for a second reviewer
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2409913 to
d30db59CompareNovember 28, 2022 23:57
@TheBlueMatt
TheBlueMatt
commented
Nov 30, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
2 times, most recently
from
74a1070 to
2233650CompareDecember 2, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Took this opportunity to squash since there were a lot of fixups.
tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
@tnulltnull
left a comment
•

edited

Loading

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.

Did a quick first pass to get a high-level overview. These are predominately doc nits, main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2233650 to
719bf79CompareDecember 2, 2022 18:56
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Yea, most of this code predates me knowing you could even do links in docs, sooooo...
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
719bf79 to
67d3ac8CompareDecember 2, 2022 19:01
@TheBlueMattTheBlueMatt
mentioned this pull request
Dec 3, 2022
 Merged

tnull
tnull
reviewed
Dec 5, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good, just a few nits/questions.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
65d9d0f to
5e8193fCompareDecember 5, 2022 18:01
tnull
tnull
reviewed
Dec 6, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Feel free to squash, I think. Should also alleviate the CI hangup..
valentinewallace
valentinewallace
reviewed
Dec 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.

ACK after squash
@TheBlueMatt
Lean on the holding cell when batch-forwarding/failing HTLCs
cae7c81
When we batch HTLC updates, we currently do the explicit queueing
plus the commitment generation in the `ChannelManager`. This is a
bit strange as its ultimately really a `Channel` responsibility to
generate commitments at the correct time, with the abstraction
leaking into `ChannelManager` with the `send_htlc` and
`get_update_fail_htlc` method docs having clear comments about
how `send_commitment` MUST be called prior to calling other
`Channel` methods.
Luckily `Channel` already has an update queue - the holding cell.
Thus, we can trivially rewrite the batch update logic as inserting
the desired updates into the holding cell and then asking all
channels to clear their holding cells.
@TheBlueMatt
Free the holding cells during background timer ticks
1531378
We currently free the channel holding cells in
`get_and_clear_pending_msg_events`, blocking outbound messages
while we do so. This is fine, but may block the message pipeline
longer than we need to. In the next commit we'll push
timer-originating channel fee updates out through the holding cell
pipeline, leaning more on that freeing in the future.
Thus, to avoid a regression in message time, here we clear the
holding cell after processing all timer events. This also avoids
needing to change tests in the next commit.
@TheBlueMatt
Lean on the holding cell for commitments when updating fees
aa9630b
Like the previous commit, here we update the update_fee+commit
logic to simply push the fee update into the holding cell and then
use the standard holding-cell-freeing codepaths to actually send
the commitment update. This removes a substantial amount of code,
reducing redundant codepaths and keeping channel state machine
logic in channel.rs.
@TheBlueMatt
Correct confusing docs on Channel methods
1833070
The methods return `Ok(())` always, they just happen to never
return in the case of a duplicate claim if debug assertions are
enabled.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
5e8193f to
1833070CompareDecember 6, 2022 18:18
@TheBlueMatt
TheBlueMatt
commented
Dec 6, 2022

Copy link


Copy Markdown

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

Reviewers

@tnulltnulltnull approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@tnulltnull
@valentinewallacevalentinewallace

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@tnull

, '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
Lean on the holding cell when batch-forwarding/failing HTLCs - #1863
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Dec 7, 2022
Merged
Lean on the holding cell when batch-forwarding/failing HTLCs#1863
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 19, 2022

Copy link


Copy Markdown

Collaborator
When we batch HTLC updates, we currently do the explicit queueing plus the commitment generation in the ChannelManager. This is a bit strange as its ultimately really a Channel responsibility to generate commitments at the correct time, with the abstraction leaking into ChannelManager with the send_htlc and get_update_fail_htlc method docs having clear comments about how send_commitment MUST be called prior to calling other Channel methods.
Luckily Channel already has an update queue - the holding cell. Thus, we can trivially rewrite the batch update logic as inserting the desired updates into the holding cell and then asking all channels to clear their holding cells.
@codecov-commenter
codecov-commenter
commented
Nov 19, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.69% // Head: 90.74% // Increases project coverage by +0.05% 🎉
Coverage data is based on head (65d9d0f) compared to base (52edb35).

Patch coverage: 83.05% of modified lines in pull request are covered.
❗ Current head 65d9d0f differs from pull request most recent head 5e8193f. Consider uploading reports for the commit 5e8193f to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## main #1863 +/- ##
==========================================
+ Coverage 90.69% 90.74% +0.05% 
==========================================
Files 91 91 Lines 48408 48326 -82 Branches 48408 48326 -82 ==========================================
- Hits 43902 43853 -49 + Misses 4506 4473 -33 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs86.57% <70.96%> (+0.29%)⬆️
lightning/src/ln/channel.rs88.98% <96.42%> (+0.13%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.86%)⬇️
lightning/src/chain/channelmonitor.rs90.79% <0.00%> (+0.05%)⬆️
lightning/src/ln/functional_tests.rs97.14% <0.00%> (+0.09%)⬆️
lightning/src/util/events.rs25.22% <0.00%> (+0.22%)⬆️
lightning/src/ln/peer_channel_encryptor.rs93.62% <0.00%> (+0.24%)⬆️
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.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Pushed two additional commits to lean on the holding cell when doing fee updates as well...that sweet, sweet -100 total LoC.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Note that the fuzzing failure here is resolved in #1859. Will leave this PR as free-standing but that will need to land first.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
9d2affd to
2ee043cCompareNovember 22, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Nov 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased without changes, CI should pass now.
valentinewallace
valentinewallace
reviewed
Nov 23, 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.

Nice cleanup, no major feedback. Will take another look next week
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
 Ok(Some(res))
	}

	/// Creates a signed commitment transaction to send to the remote peer.
@valentinewallacevalentinewallaceNov 23, 2022

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.

nit: I think ideally this would've been removed in the previous-previous commit
@TheBlueMattTheBlueMattNov 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

send_update_fee_and_commit is removed in the "updating fees" commit, and it relies on send_commitment, so we can only remove it in the last commit.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2ee043c to
2409913CompareNovember 25, 2022 20:30
@valentinewallace
valentinewallace
commented
Nov 28, 2022

Copy link


Copy Markdown

Contributor
(Maybe for follow-up) would it further simplify things to do the same batching in ChannelManager::send_payment_along_path?
@TheBlueMatt
TheBlueMatt
commented
Nov 28, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Yea, that one's substantially more complicated, but work there is coming soon (tm).
valentinewallace
valentinewallace
reviewed
Nov 28, 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.

Basically LGTM, good time for a second reviewer
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2409913 to
d30db59CompareNovember 28, 2022 23:57
@TheBlueMatt
TheBlueMatt
commented
Nov 30, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
2 times, most recently
from
74a1070 to
2233650CompareDecember 2, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Took this opportunity to squash since there were a lot of fixups.
tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
@tnulltnull
left a comment
•

edited

Loading

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.

Did a quick first pass to get a high-level overview. These are predominately doc nits, main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2233650 to
719bf79CompareDecember 2, 2022 18:56
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Yea, most of this code predates me knowing you could even do links in docs, sooooo...
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
719bf79 to
67d3ac8CompareDecember 2, 2022 19:01
@TheBlueMattTheBlueMatt
mentioned this pull request
Dec 3, 2022
 Merged

tnull
tnull
reviewed
Dec 5, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good, just a few nits/questions.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
65d9d0f to
5e8193fCompareDecember 5, 2022 18:01
tnull
tnull
reviewed
Dec 6, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Feel free to squash, I think. Should also alleviate the CI hangup..
valentinewallace
valentinewallace
reviewed
Dec 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.

ACK after squash
@TheBlueMatt
Lean on the holding cell when batch-forwarding/failing HTLCs
cae7c81
When we batch HTLC updates, we currently do the explicit queueing
plus the commitment generation in the `ChannelManager`. This is a
bit strange as its ultimately really a `Channel` responsibility to
generate commitments at the correct time, with the abstraction
leaking into `ChannelManager` with the `send_htlc` and
`get_update_fail_htlc` method docs having clear comments about
how `send_commitment` MUST be called prior to calling other
`Channel` methods.
Luckily `Channel` already has an update queue - the holding cell.
Thus, we can trivially rewrite the batch update logic as inserting
the desired updates into the holding cell and then asking all
channels to clear their holding cells.
@TheBlueMatt
Free the holding cells during background timer ticks
1531378
We currently free the channel holding cells in
`get_and_clear_pending_msg_events`, blocking outbound messages
while we do so. This is fine, but may block the message pipeline
longer than we need to. In the next commit we'll push
timer-originating channel fee updates out through the holding cell
pipeline, leaning more on that freeing in the future.
Thus, to avoid a regression in message time, here we clear the
holding cell after processing all timer events. This also avoids
needing to change tests in the next commit.
@TheBlueMatt
Lean on the holding cell for commitments when updating fees
aa9630b
Like the previous commit, here we update the update_fee+commit
logic to simply push the fee update into the holding cell and then
use the standard holding-cell-freeing codepaths to actually send
the commitment update. This removes a substantial amount of code,
reducing redundant codepaths and keeping channel state machine
logic in channel.rs.
@TheBlueMatt
Correct confusing docs on Channel methods
1833070
The methods return `Ok(())` always, they just happen to never
return in the case of a duplicate claim if debug assertions are
enabled.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
5e8193f to
1833070CompareDecember 6, 2022 18:18
@TheBlueMatt
TheBlueMatt
commented
Dec 6, 2022

Copy link


Copy Markdown

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

Reviewers

@tnulltnulltnull approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@tnulltnull
@valentinewallacevalentinewallace

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@tnull

, '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
Lean on the holding cell when batch-forwarding/failing HTLCs - #1863
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Dec 7, 2022
Merged
Lean on the holding cell when batch-forwarding/failing HTLCs#1863
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 19, 2022

Copy link


Copy Markdown

Collaborator
When we batch HTLC updates, we currently do the explicit queueing plus the commitment generation in the ChannelManager. This is a bit strange as its ultimately really a Channel responsibility to generate commitments at the correct time, with the abstraction leaking into ChannelManager with the send_htlc and get_update_fail_htlc method docs having clear comments about how send_commitment MUST be called prior to calling other Channel methods.
Luckily Channel already has an update queue - the holding cell. Thus, we can trivially rewrite the batch update logic as inserting the desired updates into the holding cell and then asking all channels to clear their holding cells.
@codecov-commenter
codecov-commenter
commented
Nov 19, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.69% // Head: 90.74% // Increases project coverage by +0.05% 🎉
Coverage data is based on head (65d9d0f) compared to base (52edb35).

Patch coverage: 83.05% of modified lines in pull request are covered.
❗ Current head 65d9d0f differs from pull request most recent head 5e8193f. Consider uploading reports for the commit 5e8193f to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## main #1863 +/- ##
==========================================
+ Coverage 90.69% 90.74% +0.05% 
==========================================
Files 91 91 Lines 48408 48326 -82 Branches 48408 48326 -82 ==========================================
- Hits 43902 43853 -49 + Misses 4506 4473 -33 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs86.57% <70.96%> (+0.29%)⬆️
lightning/src/ln/channel.rs88.98% <96.42%> (+0.13%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.86%)⬇️
lightning/src/chain/channelmonitor.rs90.79% <0.00%> (+0.05%)⬆️
lightning/src/ln/functional_tests.rs97.14% <0.00%> (+0.09%)⬆️
lightning/src/util/events.rs25.22% <0.00%> (+0.22%)⬆️
lightning/src/ln/peer_channel_encryptor.rs93.62% <0.00%> (+0.24%)⬆️
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.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Pushed two additional commits to lean on the holding cell when doing fee updates as well...that sweet, sweet -100 total LoC.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Note that the fuzzing failure here is resolved in #1859. Will leave this PR as free-standing but that will need to land first.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
9d2affd to
2ee043cCompareNovember 22, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Nov 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased without changes, CI should pass now.
valentinewallace
valentinewallace
reviewed
Nov 23, 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.

Nice cleanup, no major feedback. Will take another look next week
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
 Ok(Some(res))
	}

	/// Creates a signed commitment transaction to send to the remote peer.
@valentinewallacevalentinewallaceNov 23, 2022

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.

nit: I think ideally this would've been removed in the previous-previous commit
@TheBlueMattTheBlueMattNov 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

send_update_fee_and_commit is removed in the "updating fees" commit, and it relies on send_commitment, so we can only remove it in the last commit.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2ee043c to
2409913CompareNovember 25, 2022 20:30
@valentinewallace
valentinewallace
commented
Nov 28, 2022

Copy link


Copy Markdown

Contributor
(Maybe for follow-up) would it further simplify things to do the same batching in ChannelManager::send_payment_along_path?
@TheBlueMatt
TheBlueMatt
commented
Nov 28, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Yea, that one's substantially more complicated, but work there is coming soon (tm).
valentinewallace
valentinewallace
reviewed
Nov 28, 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.

Basically LGTM, good time for a second reviewer
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2409913 to
d30db59CompareNovember 28, 2022 23:57
@TheBlueMatt
TheBlueMatt
commented
Nov 30, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
2 times, most recently
from
74a1070 to
2233650CompareDecember 2, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Took this opportunity to squash since there were a lot of fixups.
tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
@tnulltnull
left a comment
•

edited

Loading

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.

Did a quick first pass to get a high-level overview. These are predominately doc nits, main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2233650 to
719bf79CompareDecember 2, 2022 18:56
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Yea, most of this code predates me knowing you could even do links in docs, sooooo...
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
719bf79 to
67d3ac8CompareDecember 2, 2022 19:01
@TheBlueMattTheBlueMatt
mentioned this pull request
Dec 3, 2022
 Merged

tnull
tnull
reviewed
Dec 5, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good, just a few nits/questions.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
65d9d0f to
5e8193fCompareDecember 5, 2022 18:01
tnull
tnull
reviewed
Dec 6, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Feel free to squash, I think. Should also alleviate the CI hangup..
valentinewallace
valentinewallace
reviewed
Dec 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.

ACK after squash
@TheBlueMatt
Lean on the holding cell when batch-forwarding/failing HTLCs
cae7c81
When we batch HTLC updates, we currently do the explicit queueing
plus the commitment generation in the `ChannelManager`. This is a
bit strange as its ultimately really a `Channel` responsibility to
generate commitments at the correct time, with the abstraction
leaking into `ChannelManager` with the `send_htlc` and
`get_update_fail_htlc` method docs having clear comments about
how `send_commitment` MUST be called prior to calling other
`Channel` methods.
Luckily `Channel` already has an update queue - the holding cell.
Thus, we can trivially rewrite the batch update logic as inserting
the desired updates into the holding cell and then asking all
channels to clear their holding cells.
@TheBlueMatt
Free the holding cells during background timer ticks
1531378
We currently free the channel holding cells in
`get_and_clear_pending_msg_events`, blocking outbound messages
while we do so. This is fine, but may block the message pipeline
longer than we need to. In the next commit we'll push
timer-originating channel fee updates out through the holding cell
pipeline, leaning more on that freeing in the future.
Thus, to avoid a regression in message time, here we clear the
holding cell after processing all timer events. This also avoids
needing to change tests in the next commit.
@TheBlueMatt
Lean on the holding cell for commitments when updating fees
aa9630b
Like the previous commit, here we update the update_fee+commit
logic to simply push the fee update into the holding cell and then
use the standard holding-cell-freeing codepaths to actually send
the commitment update. This removes a substantial amount of code,
reducing redundant codepaths and keeping channel state machine
logic in channel.rs.
@TheBlueMatt
Correct confusing docs on Channel methods
1833070
The methods return `Ok(())` always, they just happen to never
return in the case of a duplicate claim if debug assertions are
enabled.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
5e8193f to
1833070CompareDecember 6, 2022 18:18
@TheBlueMatt
TheBlueMatt
commented
Dec 6, 2022

Copy link


Copy Markdown

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

Reviewers

@tnulltnulltnull approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@tnulltnull
@valentinewallacevalentinewallace

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@tnull

, '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
Lean on the holding cell when batch-forwarding/failing HTLCs - #1863
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Dec 7, 2022
Merged
Lean on the holding cell when batch-forwarding/failing HTLCs#1863
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 19, 2022

Copy link


Copy Markdown

Collaborator
When we batch HTLC updates, we currently do the explicit queueing plus the commitment generation in the ChannelManager. This is a bit strange as its ultimately really a Channel responsibility to generate commitments at the correct time, with the abstraction leaking into ChannelManager with the send_htlc and get_update_fail_htlc method docs having clear comments about how send_commitment MUST be called prior to calling other Channel methods.
Luckily Channel already has an update queue - the holding cell. Thus, we can trivially rewrite the batch update logic as inserting the desired updates into the holding cell and then asking all channels to clear their holding cells.
@codecov-commenter
codecov-commenter
commented
Nov 19, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.69% // Head: 90.74% // Increases project coverage by +0.05% 🎉
Coverage data is based on head (65d9d0f) compared to base (52edb35).

Patch coverage: 83.05% of modified lines in pull request are covered.
❗ Current head 65d9d0f differs from pull request most recent head 5e8193f. Consider uploading reports for the commit 5e8193f to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## main #1863 +/- ##
==========================================
+ Coverage 90.69% 90.74% +0.05% 
==========================================
Files 91 91 Lines 48408 48326 -82 Branches 48408 48326 -82 ==========================================
- Hits 43902 43853 -49 + Misses 4506 4473 -33 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs86.57% <70.96%> (+0.29%)⬆️
lightning/src/ln/channel.rs88.98% <96.42%> (+0.13%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.86%)⬇️
lightning/src/chain/channelmonitor.rs90.79% <0.00%> (+0.05%)⬆️
lightning/src/ln/functional_tests.rs97.14% <0.00%> (+0.09%)⬆️
lightning/src/util/events.rs25.22% <0.00%> (+0.22%)⬆️
lightning/src/ln/peer_channel_encryptor.rs93.62% <0.00%> (+0.24%)⬆️
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.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Pushed two additional commits to lean on the holding cell when doing fee updates as well...that sweet, sweet -100 total LoC.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Note that the fuzzing failure here is resolved in #1859. Will leave this PR as free-standing but that will need to land first.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
9d2affd to
2ee043cCompareNovember 22, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Nov 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased without changes, CI should pass now.
valentinewallace
valentinewallace
reviewed
Nov 23, 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.

Nice cleanup, no major feedback. Will take another look next week
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
 Ok(Some(res))
	}

	/// Creates a signed commitment transaction to send to the remote peer.
@valentinewallacevalentinewallaceNov 23, 2022

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.

nit: I think ideally this would've been removed in the previous-previous commit
@TheBlueMattTheBlueMattNov 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

send_update_fee_and_commit is removed in the "updating fees" commit, and it relies on send_commitment, so we can only remove it in the last commit.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2ee043c to
2409913CompareNovember 25, 2022 20:30
@valentinewallace
valentinewallace
commented
Nov 28, 2022

Copy link


Copy Markdown

Contributor
(Maybe for follow-up) would it further simplify things to do the same batching in ChannelManager::send_payment_along_path?
@TheBlueMatt
TheBlueMatt
commented
Nov 28, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Yea, that one's substantially more complicated, but work there is coming soon (tm).
valentinewallace
valentinewallace
reviewed
Nov 28, 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.

Basically LGTM, good time for a second reviewer
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2409913 to
d30db59CompareNovember 28, 2022 23:57
@TheBlueMatt
TheBlueMatt
commented
Nov 30, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
2 times, most recently
from
74a1070 to
2233650CompareDecember 2, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Took this opportunity to squash since there were a lot of fixups.
tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
@tnulltnull
left a comment
•

edited

Loading

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.

Did a quick first pass to get a high-level overview. These are predominately doc nits, main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2233650 to
719bf79CompareDecember 2, 2022 18:56
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Yea, most of this code predates me knowing you could even do links in docs, sooooo...
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
719bf79 to
67d3ac8CompareDecember 2, 2022 19:01
@TheBlueMattTheBlueMatt
mentioned this pull request
Dec 3, 2022
 Merged

tnull
tnull
reviewed
Dec 5, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good, just a few nits/questions.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
65d9d0f to
5e8193fCompareDecember 5, 2022 18:01
tnull
tnull
reviewed
Dec 6, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Feel free to squash, I think. Should also alleviate the CI hangup..
valentinewallace
valentinewallace
reviewed
Dec 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.

ACK after squash
@TheBlueMatt
Lean on the holding cell when batch-forwarding/failing HTLCs
cae7c81
When we batch HTLC updates, we currently do the explicit queueing
plus the commitment generation in the `ChannelManager`. This is a
bit strange as its ultimately really a `Channel` responsibility to
generate commitments at the correct time, with the abstraction
leaking into `ChannelManager` with the `send_htlc` and
`get_update_fail_htlc` method docs having clear comments about
how `send_commitment` MUST be called prior to calling other
`Channel` methods.
Luckily `Channel` already has an update queue - the holding cell.
Thus, we can trivially rewrite the batch update logic as inserting
the desired updates into the holding cell and then asking all
channels to clear their holding cells.
@TheBlueMatt
Free the holding cells during background timer ticks
1531378
We currently free the channel holding cells in
`get_and_clear_pending_msg_events`, blocking outbound messages
while we do so. This is fine, but may block the message pipeline
longer than we need to. In the next commit we'll push
timer-originating channel fee updates out through the holding cell
pipeline, leaning more on that freeing in the future.
Thus, to avoid a regression in message time, here we clear the
holding cell after processing all timer events. This also avoids
needing to change tests in the next commit.
@TheBlueMatt
Lean on the holding cell for commitments when updating fees
aa9630b
Like the previous commit, here we update the update_fee+commit
logic to simply push the fee update into the holding cell and then
use the standard holding-cell-freeing codepaths to actually send
the commitment update. This removes a substantial amount of code,
reducing redundant codepaths and keeping channel state machine
logic in channel.rs.
@TheBlueMatt
Correct confusing docs on Channel methods
1833070
The methods return `Ok(())` always, they just happen to never
return in the case of a duplicate claim if debug assertions are
enabled.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
5e8193f to
1833070CompareDecember 6, 2022 18:18
@TheBlueMatt
TheBlueMatt
commented
Dec 6, 2022

Copy link


Copy Markdown

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

Reviewers

@tnulltnulltnull approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@tnulltnull
@valentinewallacevalentinewallace

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@tnull

, '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
Lean on the holding cell when batch-forwarding/failing HTLCs - #1863
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Dec 7, 2022
Merged
Lean on the holding cell when batch-forwarding/failing HTLCs#1863
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 19, 2022

Copy link


Copy Markdown

Collaborator
When we batch HTLC updates, we currently do the explicit queueing plus the commitment generation in the ChannelManager. This is a bit strange as its ultimately really a Channel responsibility to generate commitments at the correct time, with the abstraction leaking into ChannelManager with the send_htlc and get_update_fail_htlc method docs having clear comments about how send_commitment MUST be called prior to calling other Channel methods.
Luckily Channel already has an update queue - the holding cell. Thus, we can trivially rewrite the batch update logic as inserting the desired updates into the holding cell and then asking all channels to clear their holding cells.
@codecov-commenter
codecov-commenter
commented
Nov 19, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.69% // Head: 90.74% // Increases project coverage by +0.05% 🎉
Coverage data is based on head (65d9d0f) compared to base (52edb35).

Patch coverage: 83.05% of modified lines in pull request are covered.
❗ Current head 65d9d0f differs from pull request most recent head 5e8193f. Consider uploading reports for the commit 5e8193f to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## main #1863 +/- ##
==========================================
+ Coverage 90.69% 90.74% +0.05% 
==========================================
Files 91 91 Lines 48408 48326 -82 Branches 48408 48326 -82 ==========================================
- Hits 43902 43853 -49 + Misses 4506 4473 -33 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs86.57% <70.96%> (+0.29%)⬆️
lightning/src/ln/channel.rs88.98% <96.42%> (+0.13%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.86%)⬇️
lightning/src/chain/channelmonitor.rs90.79% <0.00%> (+0.05%)⬆️
lightning/src/ln/functional_tests.rs97.14% <0.00%> (+0.09%)⬆️
lightning/src/util/events.rs25.22% <0.00%> (+0.22%)⬆️
lightning/src/ln/peer_channel_encryptor.rs93.62% <0.00%> (+0.24%)⬆️
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.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Pushed two additional commits to lean on the holding cell when doing fee updates as well...that sweet, sweet -100 total LoC.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Note that the fuzzing failure here is resolved in #1859. Will leave this PR as free-standing but that will need to land first.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
9d2affd to
2ee043cCompareNovember 22, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Nov 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased without changes, CI should pass now.
valentinewallace
valentinewallace
reviewed
Nov 23, 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.

Nice cleanup, no major feedback. Will take another look next week
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
 Ok(Some(res))
	}

	/// Creates a signed commitment transaction to send to the remote peer.
@valentinewallacevalentinewallaceNov 23, 2022

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.

nit: I think ideally this would've been removed in the previous-previous commit
@TheBlueMattTheBlueMattNov 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

send_update_fee_and_commit is removed in the "updating fees" commit, and it relies on send_commitment, so we can only remove it in the last commit.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2ee043c to
2409913CompareNovember 25, 2022 20:30
@valentinewallace
valentinewallace
commented
Nov 28, 2022

Copy link


Copy Markdown

Contributor
(Maybe for follow-up) would it further simplify things to do the same batching in ChannelManager::send_payment_along_path?
@TheBlueMatt
TheBlueMatt
commented
Nov 28, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Yea, that one's substantially more complicated, but work there is coming soon (tm).
valentinewallace
valentinewallace
reviewed
Nov 28, 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.

Basically LGTM, good time for a second reviewer
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2409913 to
d30db59CompareNovember 28, 2022 23:57
@TheBlueMatt
TheBlueMatt
commented
Nov 30, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
2 times, most recently
from
74a1070 to
2233650CompareDecember 2, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Took this opportunity to squash since there were a lot of fixups.
tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
@tnulltnull
left a comment
•

edited

Loading

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.

Did a quick first pass to get a high-level overview. These are predominately doc nits, main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2233650 to
719bf79CompareDecember 2, 2022 18:56
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Yea, most of this code predates me knowing you could even do links in docs, sooooo...
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
719bf79 to
67d3ac8CompareDecember 2, 2022 19:01
@TheBlueMattTheBlueMatt
mentioned this pull request
Dec 3, 2022
 Merged

tnull
tnull
reviewed
Dec 5, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good, just a few nits/questions.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
65d9d0f to
5e8193fCompareDecember 5, 2022 18:01
tnull
tnull
reviewed
Dec 6, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Feel free to squash, I think. Should also alleviate the CI hangup..
valentinewallace
valentinewallace
reviewed
Dec 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.

ACK after squash
@TheBlueMatt
Lean on the holding cell when batch-forwarding/failing HTLCs
cae7c81
When we batch HTLC updates, we currently do the explicit queueing
plus the commitment generation in the `ChannelManager`. This is a
bit strange as its ultimately really a `Channel` responsibility to
generate commitments at the correct time, with the abstraction
leaking into `ChannelManager` with the `send_htlc` and
`get_update_fail_htlc` method docs having clear comments about
how `send_commitment` MUST be called prior to calling other
`Channel` methods.
Luckily `Channel` already has an update queue - the holding cell.
Thus, we can trivially rewrite the batch update logic as inserting
the desired updates into the holding cell and then asking all
channels to clear their holding cells.
@TheBlueMatt
Free the holding cells during background timer ticks
1531378
We currently free the channel holding cells in
`get_and_clear_pending_msg_events`, blocking outbound messages
while we do so. This is fine, but may block the message pipeline
longer than we need to. In the next commit we'll push
timer-originating channel fee updates out through the holding cell
pipeline, leaning more on that freeing in the future.
Thus, to avoid a regression in message time, here we clear the
holding cell after processing all timer events. This also avoids
needing to change tests in the next commit.
@TheBlueMatt
Lean on the holding cell for commitments when updating fees
aa9630b
Like the previous commit, here we update the update_fee+commit
logic to simply push the fee update into the holding cell and then
use the standard holding-cell-freeing codepaths to actually send
the commitment update. This removes a substantial amount of code,
reducing redundant codepaths and keeping channel state machine
logic in channel.rs.
@TheBlueMatt
Correct confusing docs on Channel methods
1833070
The methods return `Ok(())` always, they just happen to never
return in the case of a duplicate claim if debug assertions are
enabled.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
5e8193f to
1833070CompareDecember 6, 2022 18:18
@TheBlueMatt
TheBlueMatt
commented
Dec 6, 2022

Copy link


Copy Markdown

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

Reviewers

@tnulltnulltnull approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@tnulltnull
@valentinewallacevalentinewallace

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@tnull

, '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
Lean on the holding cell when batch-forwarding/failing HTLCs - #1863
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Dec 7, 2022
Merged
Lean on the holding cell when batch-forwarding/failing HTLCs#1863
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-11-holding-cell-batch-update
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Nov 19, 2022

Copy link


Copy Markdown

Collaborator
When we batch HTLC updates, we currently do the explicit queueing plus the commitment generation in the ChannelManager. This is a bit strange as its ultimately really a Channel responsibility to generate commitments at the correct time, with the abstraction leaking into ChannelManager with the send_htlc and get_update_fail_htlc method docs having clear comments about how send_commitment MUST be called prior to calling other Channel methods.
Luckily Channel already has an update queue - the holding cell. Thus, we can trivially rewrite the batch update logic as inserting the desired updates into the holding cell and then asking all channels to clear their holding cells.
@codecov-commenter
codecov-commenter
commented
Nov 19, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 90.69% // Head: 90.74% // Increases project coverage by +0.05% 🎉
Coverage data is based on head (65d9d0f) compared to base (52edb35).

Patch coverage: 83.05% of modified lines in pull request are covered.
❗ Current head 65d9d0f differs from pull request most recent head 5e8193f. Consider uploading reports for the commit 5e8193f to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## main #1863 +/- ##
==========================================
+ Coverage 90.69% 90.74% +0.05% 
==========================================
Files 91 91 Lines 48408 48326 -82 Branches 48408 48326 -82 ==========================================
- Hits 43902 43853 -49 + Misses 4506 4473 -33 
Impacted FilesCoverage Δ
lightning/src/ln/channelmanager.rs86.57% <70.96%> (+0.29%)⬆️
lightning/src/ln/channel.rs88.98% <96.42%> (+0.13%)⬆️
lightning/src/chain/onchaintx.rs94.44% <0.00%> (-0.86%)⬇️
lightning/src/chain/channelmonitor.rs90.79% <0.00%> (+0.05%)⬆️
lightning/src/ln/functional_tests.rs97.14% <0.00%> (+0.09%)⬆️
lightning/src/util/events.rs25.22% <0.00%> (+0.22%)⬆️
lightning/src/ln/peer_channel_encryptor.rs93.62% <0.00%> (+0.24%)⬆️
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.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Pushed two additional commits to lean on the holding cell when doing fee updates as well...that sweet, sweet -100 total LoC.
@TheBlueMatt
TheBlueMatt
commented
Nov 21, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Note that the fuzzing failure here is resolved in #1859. Will leave this PR as free-standing but that will need to land first.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
9d2affd to
2ee043cCompareNovember 22, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Nov 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased without changes, CI should pass now.
valentinewallace
valentinewallace
reviewed
Nov 23, 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.

Nice cleanup, no major feedback. Will take another look next week
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
 Ok(Some(res))
	}

	/// Creates a signed commitment transaction to send to the remote peer.
@valentinewallacevalentinewallaceNov 23, 2022

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.

nit: I think ideally this would've been removed in the previous-previous commit
@TheBlueMattTheBlueMattNov 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

send_update_fee_and_commit is removed in the "updating fees" commit, and it relies on send_commitment, so we can only remove it in the last commit.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2ee043c to
2409913CompareNovember 25, 2022 20:30
@valentinewallace
valentinewallace
commented
Nov 28, 2022

Copy link


Copy Markdown

Contributor
(Maybe for follow-up) would it further simplify things to do the same batching in ChannelManager::send_payment_along_path?
@TheBlueMatt
TheBlueMatt
commented
Nov 28, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Yea, that one's substantially more complicated, but work there is coming soon (tm).
valentinewallace
valentinewallace
reviewed
Nov 28, 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.

Basically LGTM, good time for a second reviewer
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2409913 to
d30db59CompareNovember 28, 2022 23:57
@TheBlueMatt
TheBlueMatt
commented
Nov 30, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
2 times, most recently
from
74a1070 to
2233650CompareDecember 2, 2022 01:10
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Took this opportunity to squash since there were a lot of fixups.
tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
@tnulltnull
left a comment
•

edited

Loading

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.

Did a quick first pass to get a high-level overview. These are predominately doc nits, main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

tnull
tnull
reviewed
Dec 2, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
2233650 to
719bf79CompareDecember 2, 2022 18:56
@TheBlueMatt
TheBlueMatt
commented
Dec 2, 2022

Copy link


Copy Markdown

CollaboratorAuthor
main point being that I'd advocate to treat any doc comments as if they were pub, i.e., making sure to link and use backticks in order to allow the cargo doc --document-private-items CI to catch any inconsistencies.
Yea, most of this code predates me knowing you could even do links in docs, sooooo...
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
719bf79 to
67d3ac8CompareDecember 2, 2022 19:01
@TheBlueMattTheBlueMatt
mentioned this pull request
Dec 3, 2022
 Merged

tnull
tnull
reviewed
Dec 5, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good, just a few nits/questions.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
65d9d0f to
5e8193fCompareDecember 5, 2022 18:01
tnull
tnull
reviewed
Dec 6, 2022
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Feel free to squash, I think. Should also alleviate the CI hangup..
valentinewallace
valentinewallace
reviewed
Dec 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.

ACK after squash
@TheBlueMatt
Lean on the holding cell when batch-forwarding/failing HTLCs
cae7c81
When we batch HTLC updates, we currently do the explicit queueing
plus the commitment generation in the `ChannelManager`. This is a
bit strange as its ultimately really a `Channel` responsibility to
generate commitments at the correct time, with the abstraction
leaking into `ChannelManager` with the `send_htlc` and
`get_update_fail_htlc` method docs having clear comments about
how `send_commitment` MUST be called prior to calling other
`Channel` methods.
Luckily `Channel` already has an update queue - the holding cell.
Thus, we can trivially rewrite the batch update logic as inserting
the desired updates into the holding cell and then asking all
channels to clear their holding cells.
@TheBlueMatt
Free the holding cells during background timer ticks
1531378
We currently free the channel holding cells in
`get_and_clear_pending_msg_events`, blocking outbound messages
while we do so. This is fine, but may block the message pipeline
longer than we need to. In the next commit we'll push
timer-originating channel fee updates out through the holding cell
pipeline, leaning more on that freeing in the future.
Thus, to avoid a regression in message time, here we clear the
holding cell after processing all timer events. This also avoids
needing to change tests in the next commit.
@TheBlueMatt
Lean on the holding cell for commitments when updating fees
aa9630b
Like the previous commit, here we update the update_fee+commit
logic to simply push the fee update into the holding cell and then
use the standard holding-cell-freeing codepaths to actually send
the commitment update. This removes a substantial amount of code,
reducing redundant codepaths and keeping channel state machine
logic in channel.rs.
@TheBlueMatt
Correct confusing docs on Channel methods
1833070
The methods return `Ok(())` always, they just happen to never
return in the case of a duplicate claim if debug assertions are
enabled.
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-11-holding-cell-batch-update
branch
from
5e8193f to
1833070CompareDecember 6, 2022 18:18
@TheBlueMatt
TheBlueMatt
commented
Dec 6, 2022

Copy link


Copy Markdown

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

Reviewers

@tnulltnulltnull approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@tnulltnull
@valentinewallacevalentinewallace

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@tnull