-
Notifications
You must be signed in to change notification settings - Fork 424
Mixed mode splicing #4261
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?
Mixed mode splicing #4261
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
fc608ab to
a89b894
Compare
d9f02da to
c9cade5
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4261 +/- ##
========================================
Coverage 89.33% 89.34%
========================================
Files 180 180
Lines 139042 139228 +186
Branches 139042 139228 +186
========================================
+ Hits 124219 124393 +174
- Misses 12196 12207 +11
- Partials 2627 2628 +1
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:
|
When adding support for mixed splice-in and splice-out, the contribution amount will need to be computed based on the splice-in and splice-out values. Rather than add a third variant to SpliceContribution, which could have an invalid contribution amount, use a more general struct that can represent splice-in, splice-out, and mixed. Constructors are provided for the typical splice-in and splice-out case whereas support for the mixed case will be added in an independent change.
Some splicing use cases require to simultaneously splice in and out in the same splice transaction. Add support for such splices using the funding inputs to pay the appropriate fees just like the splice-in case, opposed to using the channel value like the splice-out case. This requires using the contributed input value when checking if the inputs are sufficient to cover fees, not the net contributed value. The latter may be negative in the net splice-out case.
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
c9cade5 to
175bf79
Compare
| }, | ||
| pub struct SpliceContribution { | ||
| /// The amount to contribute to the splice. | ||
| value: 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.
Can you remind me why we need this? Can we not just have a constructor that calculates the change output and adds it to outputs if needed?
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.
There's a bunch of others things need to calculate change (e.g., signer provider, channel keys id, fee rate, holder dust limit, shared input/output). See:
rust-lightning/lightning/src/ln/channel.rs
Lines 6678 to 6717 in e663517
| let change_value_opt = if self.our_funding_contribution > SignedAmount::ZERO { | |
| match calculate_change_output_value( | |
| &self, | |
| self.shared_funding_input.is_some(), | |
| &shared_funding_output.script_pubkey, | |
| context.holder_dust_limit_satoshis, | |
| ) { | |
| Ok(change_value_opt) => change_value_opt, | |
| Err(reason) => { | |
| return Err(self.into_negotiation_error(reason)); | |
| }, | |
| } | |
| } else { | |
| None | |
| }; | |
| if let Some(change_value) = change_value_opt { | |
| let change_script = if let Some(script) = self.change_script { | |
| script | |
| } else { | |
| match signer_provider.get_destination_script(context.channel_keys_id) { | |
| Ok(script) => script, | |
| Err(_) => { | |
| let reason = AbortReason::InternalError("Error getting change script"); | |
| return Err(self.into_negotiation_error(reason)); | |
| }, | |
| } | |
| }; | |
| let mut change_output = | |
| TxOut { value: Amount::from_sat(change_value), script_pubkey: change_script }; | |
| let change_output_weight = get_output_weight(&change_output.script_pubkey).to_wu(); | |
| let change_output_fee = | |
| fee_for_weight(self.funding_feerate_sat_per_1000_weight, change_output_weight); | |
| let change_value_decreased_with_fee = change_value.saturating_sub(change_output_fee); | |
| // Check dust limit again | |
| if change_value_decreased_with_fee > context.holder_dust_limit_satoshis { | |
| change_output.value = Amount::from_sat(change_value_decreased_with_fee); | |
| self.our_funding_outputs.push(change_output); | |
| } | |
| } |
It also checks if we have enough input to pay for change, so the constructor would be be fallible.
Maybe some of that code can be moved here using some fake values for the shared output, for instance. It's currently used for dual funding, too, though. The user would need to obtain and pass the shared input unless a fake is also fine. Not sure if this would be worth it, though.
|
|
||
| /// The inputs included in the splice's funding transaction to meet the contributed amount | ||
| /// plus fees. Any excess amount will be sent to a change output. | ||
| inputs: Vec<FundingTxInput>, |
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.
Should we just leave these fields public as they were (implicitly cause it was an enum)? Could then presumably drop the splice_in_and_out constructor?
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.
Hmm... not sure if we want the user to calculate the net contribution value given it is partly derived from the supplied outputs.
Maybe we could actually keep it as an enum (adding an extra variant) and have a method to derive the net contribution? I'm not 100% certain we need to use a struct, and the enum approach is nice because the fields are named when initializing a SpliceContribution variant.
Some splicing use cases require to simultaneously splice in and out in the same splice transaction. Add support for such splices using the funding inputs to pay the appropriate fees just like the splice-in case, opposed to using the channel value like the splice-out case. This requires using the contributed input value when checking if the inputs are sufficient to cover fees, not the net contributed value. The latter may be negative in the net splice-out case.