Skip to content

splice: Implement start_batch #8335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 14, 2025

Conversation

ddustin
Copy link
Collaborator

@ddustin ddustin commented Jun 9, 2025

Implement the sending of start_batch as per t-bast’s spec. We need to receive start_batch as the new spec moves batch_size out of commitment_signed and instead relays that value in start_batch. For sending we implement an internal batching approach.

This approach bundles all the message bytes for start_batch and corresponding commitment_signeds into a single peer_write call. Because each message needs to be encrypted individually, we introduce an internal peer wire message type, protocol_batch_element. This type is detected by connectd and is not meant to be sent to the peer.

connectd reads the message to get information on the batch element (currently just element_size for the individual message size and channel_id for consistency with all other peer_wire message types) and does not forward it along to the peer.

Finally connectd encrypts each message element individually, batches all the encrypted messages into one large byte array, and sends the whole array in one go on the actual wire to the peer.

This rigmarole is to make sure no other messages are sent between start_batch and all of the batch messages -- without introducing any mutexes or locks to connectd.

Changelog-Added: support for start_batch

@t-bast
Copy link

t-bast commented Jun 12, 2025

Hey! I just added a small change to start_batch as requested by jkczyz to include a message_type TLV, see lightning/bolts@a821628 for more details! I think it makes it simpler for the initial implementation to only allow start_batch that contain this TLV set to 132 (commitment_signed) and leave more general cases to future implementation (if it ever becomes necessary).

@ddustin ddustin force-pushed the ddustin/start_batch branch 2 times, most recently from 35b5281 to 07fa3ad Compare June 13, 2025 17:58
@ddustin
Copy link
Collaborator Author

ddustin commented Jun 13, 2025

Hey! I just added a small change to start_batch as requested by jkczyz to include a message_type TLV, see lightning/bolts@a821628 for more details! I think it makes it simpler for the initial implementation to only allow start_batch that contain this TLV set to 132 (commitment_signed) and leave more general cases to future implementation (if it ever becomes necessary).

I will add the message_type TLV 👍

@ddustin ddustin force-pushed the ddustin/start_batch branch 9 times, most recently from 417a728 to f3acf0a Compare June 16, 2025 18:19
@ddustin ddustin added this to the v25.09 milestone Jul 20, 2025
@ddustin ddustin requested a review from niftynei July 20, 2025 18:25
@ddustin ddustin force-pushed the ddustin/start_batch branch from f3acf0a to 5ec179a Compare July 20, 2025 18:29
@ddustin ddustin requested a review from rustyrussell August 4, 2025 20:20
@madelinevibes madelinevibes added the Status::Ready for Review The work has been completed and is now awaiting evaluation or approval. label Aug 6, 2025
Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

lgtm!

@ddustin ddustin force-pushed the ddustin/start_batch branch 2 times, most recently from afb2a1d to a5aa486 Compare August 12, 2025 19:25
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Looks great, love the design. Minor changes only.

@ddustin ddustin force-pushed the ddustin/start_batch branch 3 times, most recently from cb0e19e to df21256 Compare August 14, 2025 01:08
@rustyrussell rustyrussell self-requested a review August 14, 2025 02:46
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack! Please autosquash and apply...

Actually, I'll babysit this!

We add `start_batch` to match t-bast’s splicing spec and we add a new internal wire type `WIRE_PROTOCOL_BATCH_ELEMENT` using the type number 0

Changelog-Added: support for `start_batch`
The new spec sends `batch_size` in `start_batch` and removes it from `commitment_signed` so we need to stop processing it in `commitment_signed`.

Since the tlv is now reduced to one element and that automagically turns it into a direct use TLV so we have to update the code everywhere it is referenced.
Since `batch_size` has moved into this new message, we can’t ignore it anymore and have to process it
Since handling commit sig batches is coming for multiple locations now, add more explicity error handling so log messages are more useful.
Implement the sending of `start_batch` and `protocol_batch_element` from `channeld` to `connectd`.

Each real peer wire message is prefixed with `protocol_batch_element` so connectd can know the size of the message that were batched together.

`connectd` intercepts `protocol_batch_element` messages and eats them (doesn’t forward them to peer) to get individual messages out of the batch.

It needs this to be able to encrypt them individiaully. Afterwards it recombines the now encrypted messages into a single message to send over the wire to the peer.

`channeld` remains responsible for making `start_batch` the first message of the message bundle.
@rustyrussell rustyrussell enabled auto-merge (rebase) August 14, 2025 02:48
@rustyrussell rustyrussell merged commit 052f36c into ElementsProject:master Aug 14, 2025
109 of 116 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status::Ready for Review The work has been completed and is now awaiting evaluation or approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants