Implement start_batch message batching
#3793
Conversation
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.
|
👋 Thanks for assigning @wpaulino as a reviewer! |
lightning/src/ln/peer_handler.rs
Outdated
| .commitment_signed_batch | ||
| .get_or_insert_with(|| (msg.channel_id, BTreeMap::new())); | ||
| if let wire::Message::StartBatch(msg) = message { | ||
| if peer_lock.commitment_signed_batch.is_some() { |
There was a problem hiding this comment.
If a new batched message gets introduced, we'll have to make this a bit more generic, but I guess it's fine for now
There was a problem hiding this comment.
Went ahead and added support for other types in the future. It requires having an Unknown enum variant since we don't know the message type until we receive the first message in the batch. Wonder if it would be worth adding the message type id to start_batch? The code would be marginally better as we'd remove one of the two needed unreachables. The second could be removed by using swap, which we decided against using when writing the original code in favor of a move, IIRC.
There was a problem hiding this comment.
Wonder if it would be worth adding the message type id to start_batch?
Sounds like a good idea. The message in its current format seems very tailored to the commitment_signed use case, which is fine, but we may regret in the future. It'd be nice to have the channel_id and message_type_id be optional TLVs (requiring one or the other to be present of course) that you can specify for your specific use case.
|
👋 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. |
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.
jkczyz
force-pushed
the
2025-05-start-batch
branch
from
97118cf to
b2d073f
Compare
lightning/src/ln/peer_handler.rs
Outdated
| let funding_txid = match msg.funding_txid { | ||
| Some(funding_txid) => funding_txid, | ||
| None => { | ||
| log_debug!(logger, "Peer {} sent batched commitment_signed without a funding_txid for channel {}", log_pubkey!(their_node_id), message_batch.channel_id); | ||
| return Err(PeerHandleError { }.into()); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Looks like the spec wants us to send an error and fail the channel in this case.
- If the sending node sent `start_batch` and we are processing a batch of
`commitment_signed` messages:
- If `funding_txid` is missing in one of the `commitment_signed` messages:
- MUST send an `error` and fail the channel.
Does this imply we need to move this handling -- and thus the BTreeMap use -- into the channel layer?
There was a problem hiding this comment.
Seems like it, at least we can still keep the message type and channel_id checks at the peer layer. Disconnecting does not help since they'll just resend the same message upon reconnection (I guess only until the splice becomes locked, but it's a buggy peer regardless).
There was a problem hiding this comment.
Updated to fail the channel.
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
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. |
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.