Add support for the official splicing protocol#2887
Conversation
7d1c23e to
33482ec
Compare
fa51c31 to
2d350e5
Compare
|
This is an observation that occurred to me while reviewing this PR that might be relevant for a future PR .. There will be times when using This seems to be technically possible in the protocol because CMD_BUMP_FUNDING_FEE uses the interactive tx protocol and can change the funding contribution with the Perhaps the biggest downside I can see (beside complexity) is that if your pending splice or rbf confirms before a rbf-splice, then you would need to renegotiate it as a new splice. Am I missing something here wrt fee savings? Do you think it would be worth considering this situation now rather than later? |
Yes, the main reason we're not providing this yet (even though this is in theory possible) is to manage complexity. It can get really complex very quickly once you start going down that road, because if each splice "part" maps to a specific action (e.g. an on-the-fly funding), it can be a huge mess to reconcile which parts where actually done and which parts need to be replayed if a previous RBF attempt confirms. |
eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxFunder.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/wire/internal/channel/version4/ChannelCodecs4.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenDualFunded.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala
Outdated
Show resolved
Hide resolved
remyers
left a comment
There was a problem hiding this comment.
Changes to tests to disallow sequences of unconfirmed splices looks good. I only had a few minor nits/questions.
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalSplicesStateSpec.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalSplicesStateSpec.scala
Outdated
Show resolved
Hide resolved
cb02ea9 to
e498115
Compare
|
Rebased to include changes linked to liquidity ads and on-the-fly funding. Some on-the-fly funding tests aren't passing anymore, because it's harder to make eclair mimick the behavior of a wallet that doesn't contribute at all to funding transactions: we'll need to re-work those tests, I'm not sure yet what exactly will be best. We will probably integrate the second and third commits independently, once we're confident Phoenix users all support quiescence. This should help us integrate this bit by bit and make sure test coverage is good enough. |
e498115 to
48f98f5
Compare
48f98f5 to
7c5fc44
Compare
7c5fc44 to
315c51b
Compare
833acf9 to
364c6d1
Compare
364c6d1 to
071ff45
Compare
071ff45 to
c981940
Compare
|
@remyers I rebased on top of #3083 and implemented the stronger batching logic for EDIT: the spec is now available in the last commit of lightning/bolts#1160 |
8a26d27 to
7af46f2
Compare
7af46f2 to
b440de4
Compare
090dcbe to
c42d6c1
Compare
9663d4a to
f9d0384
Compare
f9d0384 to
248a753
Compare
248a753 to
0e7228e
Compare
In lightning/bolts#1160 we add a TLV field to `commit_sig` messages to let the receiver know to which `funding_txid` this signature applies. This is more resilient than relying on the order of the `commit_sig` messages in the batch. This is an odd TLV, so we can start writing it right now without creating compatibility issues. We also slightly refactor existing code to make it easier to introduce a backwards-compat layer when migrating to the official splicing. We also increase the default number of RBF attempts allowed.
In lightning/bolts#1160, we introduce a message to let our peer know how many `commit_sig` messages they will receive and treat them as a batch. This replaces our previous version that did something similar, but by adding a batch TLV in every `commit_sig` message we send. We currently do both: we keep inserting the experimental batch TLV, and we start by sending a `start_batch` message (with the same information). Since it is an odd message (127), it should be safely ignored if our peer doesn't understand it.
0e7228e to
0964dd4
Compare
We replace our experimental version of `splice_init`, `splice_ack` and `splice_locked` by their official version. If our peer is using the experimental feature bit, we convert our outgoing messages to use the experimental encoding and incoming messages to the official messages. We also change the TLV fields added to `tx_add_input`, `tx_signatures` and `splice_locked` to match the spec version. We always write both the official and experimental TLV to updated nodes (because the experimental one is odd and will be ignored) but we drop the official TLV if our peer is using the experimental feature, because it won't understand the even TLV field. We do the same thing for the `commit_sig` TLV. For peers who support the official splicing version, we insert the `start_batch` message before the batch of `commit_sig` messages. This guarantees backwards-compatibility with peers who only support the experimental feature.
0964dd4 to
a0083a2
Compare
a6b6a23 to
759ff27
Compare
We introduce a `retransmit_flags` field to `my_current_funding_locked` and `next_funding` to ask our peer to retransmit `commitment_signed` or `announcement_signatures` if we're expecting them. With this change, we don't need to retransmit `splice_locked` on reconnection anymore to trigger the exchange of `announcement_signatures`. We don't need to retransmit it to let our peer know that we've seen enough confirmations for the splice either, since `my_current_funding_locked` implies that. This allows us to completely remove retransmission of `splice_locked` on reconnection, and also get rid of the `your_last_funding_locked` TLV, which greatly simplifies the reconnection logic. We do keep them for backwards-compatibility with existing Phoenix users though, but we'll be able to clean it up once they have updated. Note that this works with taproot channels since we will simply provide nonces in `channel_reestablish` when we need our peer to send announcement signatures (not supported yet since taproot channels are never announced). We rollback using the `next_commitment_number` to let our peer know that we haven't received their `commit_sig` and instead use the retransmit flags added to the `next_funding` TLV, unless our peer is using the legacy splicing protocol.
759ff27 to
4a8d0a5
Compare
|
Claude's review regarding backwards-compatibility with the previous protocol: |
We replace our experimental version of
splice_init,splice_ackandsplice_lockedby their official version (see lightning/bolts#1160). If our peer is using the experimental feature bit, we convert our outgoing messages to use the experimental encoding and incoming messages to the official messages.We also change the TLV fields added to
tx_add_input,tx_signaturesandsplice_lockedto match the spec version. We always write both the official and experimental TLV to updated nodes (because the experimental one is odd and will be ignored) but we drop the official TLV if our peer is using the experimental feature, because they won't understand the even TLV field.We do the same thing for the
commit_sigTLV. For peers who support the official splicing version, we insert thestart_batchmessage before the batch ofcommit_sigmessages as specified in lightning/bolts#1160.We also support both the legacy and official reconnection logic (TLV fields in
channel_reestablish). It is a bit painful because both protocols use TLV 1, but for different purposes, so we need to do it in a somewhat hacky way at the codec level.This ensures that we're able to create splice transactions, even with non-upgraded nodes (full backwards-compatibility). It would be best to make sure Phoenix users upgrade to a version that uses the official protocol though, to allow cleaning up the codebase and guarantee that we haven't missed an edge case.
Builds on top of #3261
TODO: