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

Prepare InteractiveTxBuilder to support splicing #2595

Merged
merged 5 commits into from
Feb 17, 2023

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Feb 3, 2023

Adding splicing to the InteractiveTxBuilder requires:

  • adding a shared input (the current funding output)
  • adding outputs (when splicing out)

The localAmount and remoteAmount provided are the amounts each peer contributes to the new funding output. Those amounts should be computed by the caller depending on what they intend to do (splice funds in/out). This model allows batching operations: if the caller wants to do several splices in and several splices out, this is easy to do by iterating over these operations and updating the targeted localAmount accordingly, the InteractiveTxFunder will take care of the rest.

Note that when splicing, we currently truncate previous balances to sats. This results in 1 sat being given away to miners as fees, and a passive participant losing up to 999 msat of their balance. This can be changed in the future depending on the final spec choice, and shouldn't need a codec update since the previous balances come from the commitment field provided in the purpose.

When splicing, the tx_add_input message we send for the shared input doesn't contain the previous transaction. There are two reasons for that:

  • it potentially doesn't fit into a lightning message (if > 65kB)
  • we don't need it, we know it correctly uses segwit

The changes aren't that big, but some of them are a bit subtle. The tests are quite verbose and contain some duplication, but I find them very easy to read and useful that way, most of the refactoring I tried made them less clear, so I chose to keep them that way for now. Let me know if you think some duplication should be removed.

Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, this works well and I was able to rebase the splices PR on top of it (see branch splices-20230210), but there are two potential leads that we may want to explore:

@t-bast t-bast force-pushed the interactive-tx-splice-builder branch from 4e0f3fe to 4809fbc Compare February 13, 2023 16:14
@t-bast
Copy link
Member Author

t-bast commented Feb 13, 2023

This is rebased on top of #2598 to make e2e testing of the splice prototype easier.

The second commit re-works the SharedTransaction object to clearly separate the shared input/output from other inputs/outputs.

It also moves the sharedInput_opt and localOutputs outside of the InteractiveTxParams. These fields don't really make sense in purpose however, because it would force us to have different Rbf classes for funding and splice, which is unnecessary. So I just put them directly in the InteractiveTxBuilder arguments.

There is one caveat which we need to think about: if those two fields aren't saved with the InteractiveTxParams, it could make it harder to RBF splices, because we will need to re-create the sharedInput_opt and the localOutputs. For the sharedInput_opt, we should have all the data we need (since it's just the previous funding input). For the localOutputs, we could look at the previous SharedTransaction in our localOnlyOutputs, but we currently don't know how to distinguish our splice outputs from our change output: maybe we should add even more information to the SharedTransaction to make that easy? We could for example tag each output with its type (shared, change or splice). That can be done in a follow-up PR though to minimize rebase work.

Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the change made to SharedTransaction in 4809fbc.

However, I'm more skeptical about the changes made to:

  • InteractiveTxBuilder: this breaks the convention that things related to building the transaction are to be put in InteractiveTxParams, and things related to building the commitments are to be put in InteractiveTxBuilder. Furthermore, swap-out is probably the main case where we will want to RBF the splice (user pays the fee, so will probably undershoot).
  • DATA_WAIT_FOR_DUAL_FUNDING_CREATED: I don't see the point of removing Common. It removes the symmetry with Params, and introduces a little bit of redundancy since initialization of localCommitIndex/remoteCommitIndex is done in two places now (when creating Purpose, and then independently when creating Common). Curious what motivated this change.

Adding splicing to the InteractiveTxBuilder requires:

- adding a shared input (the current funding output)
- adding outputs (when splicing out)

The `localAmount` and `remoteAmount` provided are the amounts each peer
contributes to the new funding output. Those amounts should be computed
by the caller depending on what they intend to do (splice funds in/out).
This model allows batching operations: if the caller wants to do several
splices in and several splices out, this is easy to do by iterating over
these operations and updating the targeted `localAmount` accordingly,
the InteractiveTxFunder will take care of the rest.

Note that when splicing, we currently truncate previous balances to sats.
This results in 1 sat being given away to miners as fees, and a passive
participant losing up to 999 msat of their balance. This can be changed
in the future depending on the final spec choice, and shouldn't need a
codec update since the previous balances come from the commitment field
provided in the purpose.

When splicing, the tx_add_input message we send for the shared input
doesn't contain the previous transaction. There are two reasons for
that:

- it potentially doesn't fit into a lightning message (if > 65kB)
- we don't need it, we know it correctly uses segwit
@t-bast t-bast force-pushed the interactive-tx-splice-builder branch from 4809fbc to 9e9f048 Compare February 16, 2023 16:21
@t-bast
Copy link
Member Author

t-bast commented Feb 16, 2023

This is now rebased on master and ready to review!

@t-bast t-bast requested a review from pm47 February 16, 2023 16:24
@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2023

Codecov Report

Merging #2595 (bee1e59) into master (202598d) will increase coverage by 0.12%.
The diff coverage is 97.40%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #2595      +/-   ##
==========================================
+ Coverage   85.50%   85.63%   +0.12%     
==========================================
  Files         208      211       +3     
  Lines       16578    16913     +335     
  Branches      693      742      +49     
==========================================
+ Hits        14175    14483     +308     
- Misses       2403     2430      +27     
Impacted Files Coverage Δ
...ala/fr/acinq/eclair/blockchain/OnChainWallet.scala 100.00% <ø> (ø)
...in/scala/fr/acinq/eclair/channel/ChannelData.scala 98.30% <ø> (ø)
...la/fr/acinq/eclair/transactions/Transactions.scala 96.58% <0.00%> (-0.26%) ⬇️
.../eclair/wire/protocol/LightningMessageCodecs.scala 99.35% <ø> (ø)
...inq/eclair/channel/fund/InteractiveTxBuilder.scala 89.65% <97.10%> (+0.76%) ⬆️
...cinq/eclair/channel/fund/InteractiveTxFunder.scala 92.43% <97.14%> (+5.22%) ⬆️
...ir/blockchain/bitcoind/rpc/BitcoinCoreClient.scala 96.66% <100.00%> (ø)
...inq/eclair/channel/fsm/ChannelOpenDualFunded.scala 88.21% <100.00%> (+0.36%) ⬆️
...acinq/eclair/channel/fsm/DualFundingHandlers.scala 80.64% <100.00%> (ø)
...ire/internal/channel/version3/ChannelCodecs3.scala 96.89% <100.00%> (+0.68%) ⬆️
... and 30 more

Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@t-bast t-bast merged commit fcc52a8 into master Feb 17, 2023
@t-bast t-bast deleted the interactive-tx-splice-builder branch February 17, 2023 11:53
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.

3 participants