Stricter batching of commit_sig messages on the wire
#3083
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `TransportHandler` is a very old actor that we never refactored much and needed a bit of clean-up. There is no reason to make it generic, it only supports sending lightning messages on the wire. If we ever need to make it generic in the future, we can easily do it, but for simplicity it should only handle lightning messages for now.
We introduce a `CommitSigBatch` class to group `commit_sig` messages when splice transactions are pending. We use this class to ensure that all the `commit_sig` messages in the batch are sent together to our peer, without any other messages in-between.
t-bast
force-pushed
the
splice-message-batch
branch
from
1171341 to
ef6945b
Compare
remyers
reviewed
May 15, 2025
eclair-core/src/main/scala/fr/acinq/eclair/io/PeerConnection.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/io/PeerConnectionSpec.scala
Outdated
Show resolved
Hide resolved
We move the incoming `commit_sig` batching logic outside of the channel and into the `PeerConnection` instead. This slightly simplifies the channel FSM and its tests, since the `PeerConnection` actor is simpler. We unfortunately cannot easily do this in the `TransportHandler` because of our buffered read of the encrypted messages, which may split batches and make it more complex to correctly group messages.
t-bast
force-pushed
the
splice-message-batch
branch
from
8286762 to
71fb4fa
Compare
pm47
reviewed
May 22, 2025
eclair-core/src/main/scala/fr/acinq/eclair/wire/protocol/LightningMessageTypes.scala
Show resolved
Hide resolved
pm47
approved these changes
May 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There has been a lot of discussion on the spec about how we should batch
commit_sigmessages during splices. We reached an agreement to guarantee that the messages are sent together, without any other message in-between. See lightning/bolts@b5b5572 for more details.Since our experimental splice version does not guarantee this, we start by:
commit_sigmessages, which is transparent for our peerscommit_sigmessages that may have other messages in-between, to support the current experimental version while preparing a world where we always batchstart_batchmessage (but we keep backwards-compatibility support)This PR can be integrated right now, since it doesn't introduce any new spec messages or TLVs. I recommend reviewing it commit-by-commit (and read the commit messages). The first commit simply refactors the
TransportHandler, because I initially hoped that we could support the batching there: but it turns out that because of our buffered reads of encrypted streams, this would be very complex, so I ended up handling batching inPeerConnection, but I thought the refactoring was still useful. Let me know if I should just drop that first commit.