-
Notifications
You must be signed in to change notification settings - Fork 267
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
Use final spec values for splicing #2887
base: master
Are you sure you want to change the base?
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - the refactor for RBF makes it easier to read/understand.
I found a few nits but main issues to take a look at are related to adding logic to reject an TxInitRbf or SpliceInit when the parent splice is unconfirmed.
eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxFunder.scala
Outdated
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
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
We already use a minimum depth of 6 before announcing channels to protect against reorgs. However, we allowed using the channel for payments after only 3 confirmations (for small channels). A reorg of 3 blocks that invalidates the funding transaction would allow our peer to potentially steal funds. It's more consistent to use the same depth for announcing the channel and actually using it. Note that for wumbo channels, we already scaled the number of confirmations based on the size of the channel. For closing transaction, we don't need the same reorg safety, since we keep watching the funding output for any transaction that spends it, and concurrently spend any commitment transaction that we detect. We thus keep a minimum depth of 3 for closing transactions.
We were still using values from before the halving. We update those values and change the scaling factor to a reasonable scaling. This protects channels against attackers with significant mining power.
Now that we wait for at least 6 confirmations before considering a channel confirmed, we can simplify our channel announcement logic. Whenever a channel reaches the confirmed state, it can be announced to the network (if nodes wish to announce it). We thus don't need the "deeply buried" state and the "temporary" scid anymore. The logic is much simpler to follow: when the channel confirms, we internally update the real scid to match the confirmed funding tx and send our `announcement_signatures`. When we receive our peer's `announcement_signatures`, we stash them if the funding tx doesn't have enough confirmations yet, otherwise we announce the channel and create a new `channel_update` that uses the real scid. Whenever we create a `channel_update`, we simply look at whether the channel is announced or not to choose which scid to use. This will make it much simpler to announce splice transactions, which don't need a "deeply buried" state either and will instead simply rely on whether the splice transaction is confirmed or not to generate `announcement_signatures`.
We now support splicing on public channels: once the splice transaction is confirmed and locked on both sides, nodes will exchange announcement signatures that allows them to create a `channel_announcement` that they then broadcast to the network. This requires reworking the data model to include the announcement and the real `short_channel_id` in each commitment, which lets us cleanly distinguish real `short_channel_id`s from aliases (which are set at the channel level regardless of the current commitments). The flow now becomes: - when the funding transaction of a commitment confirms, we set the corresponding real `short_channel_id` in that commitment - if the channel is public and we've received `channel_ready` or `splice_locked`, we send our `announcement_signatures` - if we receive `announcement_signatures` for a commitment for which the funding transaction is unconfirmed, we stash it and replay it when the transaction confirms - if we receive `announcement_signatures` for a confirmed commitment, and we don't have a more recently announced commitment, we generate a `channel_announcement`, store it with the commitment and update our router data When creating a `channel_update` for a public channel, we always use the `short_channel_id` that matches the latest announcement we created. This is very important to guarantee that nodes receiving our updates will not discard them because they cannot match it to a channel. For private channels, we stop allowing usage of the `short_channel_id` for routing: `scid_alias` MUST be used, which ensures that the channel utxo isn't revealed. Note that when migrating to taproot channels, `splice_locked` will be used to transmit nonces for the announcement signatures, which will be compatible with the existing flow (and similarly, `channel_ready` will be used for the initial funding transaction). They are retransmitted on reconnection to ensure that the announcements can be generated.
315c51b
to
833acf9
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. This guarantees backwards-compatibility with peers who only support the experimental feature.
833acf9
to
364c6d1
Compare
We replace our experimental version of
splice_init
,splice_ack
andsplice_locked
by 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_signatures
andsplice_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 they won't understand the even TLV field.This guarantees backwards-compatibility with peers who only support the experimental feature.
@ddustin you should be able to start cross-compat tests based on this branch.
NB: builds on top of #2968