-
Notifications
You must be signed in to change notification settings - Fork 417
Add splice-out support #3979
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
base: main
Are you sure you want to change the base?
Add splice-out support #3979
Conversation
🎉 This PR is now ready for review! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3979 +/- ##
==========================================
- Coverage 88.94% 88.94% -0.01%
==========================================
Files 174 174
Lines 124200 124575 +375
Branches 124200 124575 +375
==========================================
+ Hits 110471 110797 +326
- Misses 11253 11277 +24
- Partials 2476 2501 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
acab158
to
2d39059
Compare
Splice contributions should never exceed the total bitcoin supply. This check prevents a potential overflow when converting the contribution from sats to msats. The commit additionally begins to store the contribution using SignedAmount.
…text Once the counterparty supplies their funding contribution, there is no longer a need to store it in FundingNegotiationContext as it will have already been used to create a FundingScope.
2d39059
to
381fba6
Compare
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
lightning/src/ln/channel.rs
Outdated
pub enum FundingTxContributions { | ||
/// When only inputs -- except for a possible change output -- are contributed to the funding | ||
/// transaction. This must correspond to a positive contribution amount. | ||
InputsOnly { |
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.
We should also include the contribution amount in each variant as they'll be slightly different: InputsOnly
must be > 0, OutputsOnly
must be < 0, and the mixed case can be SignedAmount
.
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.
Done. Just used Amount
for both variant and added a value
method to return a SignedAmount
for internal use. A fixup removes Amount
from SpliceOut
since it can be summed from the outputs as mentioned in another comment.
lightning/src/ln/channel.rs
Outdated
@@ -6071,6 +6074,30 @@ impl FundingNegotiationContext { | |||
} | |||
} | |||
|
|||
/// The components of a funding transaction that are contributed by one party. | |||
pub enum FundingTxContributions { |
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.
Does this mean we'll have higher-level variants for splicing and dual-funding that map to this one? I would like being able to name these SpliceIn/Out
for splicing.
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.
Discussed offline. Dropped FundingTxContributions
in favor of SplicingContribution
with SpliceIn
and SpliceOut
variants. FundingNegotiationContext
now holds the inputs as before as well as the outputs.
lightning/src/ln/channel.rs
Outdated
@@ -6079,7 +6081,7 @@ pub enum FundingTxContributions { | |||
InputsOnly { | |||
/// The inputs used to meet the contributed amount. Any excess amount will be sent to a | |||
/// change output. | |||
inputs: Vec<(TxIn, Transaction)>, | |||
inputs: Vec<(TxIn, Transaction, Weight)>, |
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.
Let's make this a struct now so we can properly document each field. The weight here should be the witness_weight
for the input to be spent. We may even be able to reuse bump_transaction::Utxo
.
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.
We may even be able to reuse
bump_transaction::Utxo
.
Hmm... that doesn't have a TxIn
. We also need the full previous transaction in InteractiveTxConstructor
to use as prevtx
in tx_add_input
.
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.
We would just need to track TxIn::sequence
and prevtx
separately if we choose to go with it
lightning/src/ln/channel.rs
Outdated
}, | ||
/// When both inputs and outputs are contributed to the funding transaction. | ||
#[cfg(test)] | ||
InputsAndOutputs { |
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.
I think we should wait to add this as a follow-up
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.
Added earlier because the tests needed a FundingTxContributions
with both inputs and outputs. But no longer needed now that FundingNegotiationContext
has individual fields.
lightning/src/ln/channel.rs
Outdated
our_funding_contribution_satoshis, | ||
); | ||
// FIXME: Should we check value_to_self instead? Do HTLCs need to be accounted for? | ||
// FIXME: Check that we can pay for the outputs from the channel value? |
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.
We probably don't even need our_funding_contribution
for a splice out, we can just assume it from adding all the output values
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.
Ah, good point. For paying for the outputs, seems we should actually use check_v2_funding_inputs_sufficient
for both splice-in and splice-out and account for outputs?
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.
Ah, actually that only looks at the input from the previous funding transaction for weight, not amount.
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.
Re-wrote this to ensure fees for the funding outputs are accounted for. One thing of note is that our_funding_contribution
for a splice-out needs to be adjusted by the fees to be paid (i.e., fees need to be subtracted from the negative our_funding_contribution
to increase the amount being removed from the channel).
Also, a bit of an oversight, but we don't yet support accepting a splice-out. Technically, it could contain both inputs and outputs even if the net amount is negative. But I think we shouldn't care as long as we check everything is sane upon exchanging tx_complete
.
lightning/src/ln/channel.rs
Outdated
self.funding.get_value_satoshis(), | ||
our_funding_contribution_satoshis, | ||
); | ||
// FIXME: Should we check value_to_self instead? Do HTLCs need to be accounted for? |
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.
Definitely value_to_self
, and we need to make sure we stay above the reserve and can still afford fees if we're the channel initiator
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
TransactionU16LenLimited was used to limit Transaction serialization size to u16::MAX. This was because messages can not be longer than u16::MAX bytes when serialized for the transport layer. However, this limit doesn't take into account other fields in a message containing a Transaction, including the length of the transaction itself. Remove TransactionU16LenLimited and instead check any user supplied transactions in the context of the enclosing message (e.g. TxAddInput).
381fba6
to
396948e
Compare
ChannelManager::splice_channel takes witness weights with the funding inputs. Storing these in FundingNegotiationContext allows us to use them when calculating the change output and include them in a common struct used for initiating a splice-in. In preparation for having ChannelManager::splice_channel take FundingTxContributions, add a weight to the FundingTxContributions::InputsOnly, which supports the splice-in use case.
The funding inputs used for splicing and v2 channel establishment are passed as a tuple of txin, prevtx, and witness weight. Add a struct so that the items included can be better documented.
ChannelManager::splice_channel takes individual parameters to support splice-in. Change these to an enum such that it can be used for splice-out as well.
Update SpliceContribution with a variant used to support splice-out (i.e., removing funds from a channel). The TxOut values must not exceed the users channel balance after accounting for fees and the reserve requirement.
396948e
to
59fbd7e
Compare
Splice-in support was added in #3736. This PR expands
ChannelManager::splice_channel
to support splice-out (i.e., removing funds from a channel). This is accomplished by adding aFundingTxContributions
enum to cover both use cases.Depends on #3736.