Skip to content
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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Jul 23, 2024

We replace our experimental version of splice_init, splice_ack and splice_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 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.

We also implement RBF support for splice transactions in the last commit: that's the part of this PR that really needs a thorough review. I can probably port some parts of this commit to master as preparatory steps if that makes it easier to review.

@ddustin you should be able to start cross-compat tests based on this branch. Note that we currently only support unannounced channels (we haven't implemented the gossip part yet).

@t-bast t-bast force-pushed the splicing-official branch 2 times, most recently from 7d1c23e to 33482ec Compare August 1, 2024 14:34
@t-bast t-bast requested a review from remyers August 1, 2024 14:36
@t-bast t-bast force-pushed the splicing-official branch 2 times, most recently from fa51c31 to 2d350e5 Compare August 1, 2024 16:17
@remyers
Copy link
Contributor

remyers commented Aug 13, 2024

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 CMD_BUMP_FUNDING_FEE instead of CMD_SPLICE would cost less on-chain and confirm faster if you want to perform an additional splice while a pending splice is unconfirmed. The total cost to create a new funding transaction with CMD_SPLICE will often be higher than making the same changes with CMD_BUMP_FUNDING_FEE at a slightly higher fee rate. If you are blocked from making a new splice because a pending splice has been fee bumped, you could also make a new fee bump to splice in/out value rather than waiting for the previously rbf'd splice to confirm.

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 funding_output_contribution TLV. Figuring out if a splice or rbf-splice is cheaper will also need to consider the additional fees paid to bump our counter party's inputs and outputs.

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?

@t-bast
Copy link
Member Author

t-bast commented Aug 13, 2024

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.

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.

Copy link
Contributor

@remyers remyers left a 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.

Copy link
Contributor

@remyers remyers left a 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.

@t-bast
Copy link
Member Author

t-bast commented Oct 8, 2024

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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants