Lean on the holding cell when batch-forwarding/failing HTLCs #1863
Conversation
Codecov ReportBase: 90.69% // Head: 90.74% // Increases project coverage by
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
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. |
|
Pushed two additional commits to lean on the holding cell when doing fee updates as well...that sweet, sweet -100 total LoC. |
|
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
force-pushed
the
2022-11-holding-cell-batch-update
branch
from
9d2affd to
2ee043c
Compare
|
Rebased without changes, CI should pass now. |
| @@ -5639,41 +5641,6 @@ impl<Signer: Sign> Channel<Signer> { | |||
| Ok(Some(res)) | |||
| } | |||
|
|
|||
| /// Creates a signed commitment transaction to send to the remote peer. | |||
There was a problem hiding this comment.
nit: I think ideally this would've been removed in the previous-previous commit
There was a problem hiding this comment.
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
force-pushed
the
2022-11-holding-cell-batch-update
branch
from
2ee043c to
2409913
Compare
|
(Maybe for follow-up) would it further simplify things to do the same batching in |
|
Yea, that one's substantially more complicated, but work there is coming soon (tm). |
TheBlueMatt
force-pushed
the
2022-11-holding-cell-batch-update
branch
from
2409913 to
d30db59
Compare
|
Rebased. |
TheBlueMatt
force-pushed
the
2022-11-holding-cell-batch-update
branch
2 times, most recently
from
74a1070 to
2233650
Compare
|
Rebased. Took this opportunity to squash since there were a lot of fixups. |
There was a problem hiding this comment.
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.
TheBlueMatt
force-pushed
the
2022-11-holding-cell-batch-update
branch
from
2233650 to
719bf79
Compare
Yea, most of this code predates me knowing you could even do links in docs, sooooo... |
TheBlueMatt
force-pushed
the
2022-11-holding-cell-batch-update
branch
from
719bf79 to
67d3ac8
Compare
TheBlueMatt
force-pushed
the
2022-11-holding-cell-batch-update
branch
from
65d9d0f to
5e8193f
Compare
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.
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.
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.
The methods return `Ok(())` always, they just happen to never return in the case of a duplicate claim if debug assertions are enabled.
TheBlueMatt
force-pushed
the
2022-11-holding-cell-batch-update
branch
from
5e8193f to
1833070
Compare
|
Squashed without change. |
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 aChannelresponsibility to generate commitments at the correct time, with the abstraction leaking intoChannelManagerwith thesend_htlcandget_update_fail_htlcmethod docs having clear comments about howsend_commitmentMUST be called prior to calling otherChannelmethods.Luckily
Channelalready 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.