Implement start_batch message batching#3793
Implement start_batch message batching#3793TheBlueMatt merged 4 commits intolightningdevkit:mainfrom
start_batch message batching#3793Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
jkczyz
force-pushed
the
2025-05-start-batch
branch
from
97118cf to
b2d073f
Compare
jkczyz
force-pushed
the
2025-05-start-batch
branch
from
b2d073f to
5110534
Compare
|
Fixed some places where we disconnected but didn't send a warning (or disconnected when we should ignore and send a warning) according too the spec. Feels a little verbose, so let me know if there is some utility for this that I'm missing. |
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
wpaulino
left a comment
wpaulino
left a comment
There was a problem hiding this comment.
Feel free to squash, CI is failing though. LGTM so far, we just have to wait for the message_type TLV to be amended to the spec.
jkczyz
force-pushed
the
2025-05-start-batch
branch
from
5110534 to
c6d62c9
Compare
Squashed. Fixed CI by running |
|
Up to you, but we can probably drop b1abb5d once the optional |
jkczyz
force-pushed
the
2025-05-start-batch
branch
from
c6d62c9 to
f373238
Compare
Added a fixup that uses the |
|
Good to squash |
jkczyz
force-pushed
the
2025-05-start-batch
branch
from
f373238 to
ebf0d4e
Compare
|
Squashed and fixed linter failures. |
|
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
lightning/src/ln/msgs.rs
Outdated
| htlc_signatures | ||
| }, { | ||
| (0, batch, option), | ||
| (1, funding_txid, option), |
There was a problem hiding this comment.
Do we want to go ahead and set these to 1001 just in case things change again?
There was a problem hiding this comment.
Is the concern we'll have this merged and released before the spec is finalized?
There was a problem hiding this comment.
Pretty much, we can flip it back to 1 once we're ready to include splicing in a release.
There was a problem hiding this comment.
Done. Also needed to update the expected encoding in a test, which unfortunately I didn't realize was broken after I rebased for the last push.
There was a problem hiding this comment.
FYI: git range-diff ebf0d4e...43db395 should show the relevant changes.
jkczyz
force-pushed
the
2025-05-start-batch
branch
from
ebf0d4e to
b4f9039
Compare
Instead of batching commitment_signed messages using a batch TLV, the splicing spec has been updated to introduce a start_batch messages. It used to indicate that the next batch_size messages for the channel_id should be treated as one logical message. This commit simply adds the message while the following commits will implement the handling logic.
Update commitment_signed message to contain the funding_txid instead of both that and a batch_size. The spec was updated to batch messages using start_batch, which contains the batch_size. This commit also updates PeerManager to batch commitment_signed messages in this manner instead of the previous custom approach.
While the spec only includes commitment_signed messages in batches, there may be other types of batches in the future. Generalize the message batching code to allow for other types in the future.
If a message in a commitment_signed batch does not contain a funding_txid or has duplicates, the channel should be failed. Move this check from PeerManager to FundedChannel such that this can be done.
jkczyz
force-pushed
the
2025-05-start-batch
branch
from
b4f9039 to
43db395
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3793 +/- ##
==========================================
- Coverage 89.91% 89.84% -0.07%
==========================================
Files 160 160
Lines 129307 129375 +68
Branches 129307 129375 +68
==========================================
- Hits 116270 116241 -29
- Misses 10349 10438 +89
- Partials 2688 2696 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// The number of messages to follow. | ||
| pub batch_size: u16, | ||
| /// The type of all messages expected in the batch. | ||
| pub message_type: Option<u16>, |
There was a problem hiding this comment.
Why is this an Option if we require it?
There was a problem hiding this comment.
The spec doesn't spell out the requirements yet, but I assumed we'd need to send a warning before disconnecting. Also allows for additional logging.
4 participants
Instead of batching
commitment_signed messagesusing a batch TLV, the splicing spec has been updated to introduce astart_batchmessages. It used to indicate that the nextbatch_sizemessages for thechannel_idshould be treated as one logical message. Updatecommitment_signedmessage to contain thefunding_txidinstead of both that and abatch_size. Also, updatePeerManagerto batch accordingly instead of the previous custom approach.