[Splicing] Tx negotiation during splicing #3736
Conversation
|
👋 I see @wpaulino was un-assigned. |
optout21
force-pushed
the
splice-dual-tx4
branch
from
7f6dfbd to
c3778bc
Compare
1 similar comment
1 similar comment
1 similar comment
1 similar comment
1 similar comment
1 similar comment
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3736 +/- ##
==========================================
+ Coverage 88.92% 88.99% +0.07%
==========================================
Files 174 174
Lines 123869 123793 -76
Branches 123869 123793 -76
==========================================
+ Hits 110152 110174 +22
+ Misses 11256 11164 -92
+ Partials 2461 2455 -6
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:
|
optout21
force-pushed
the
splice-dual-tx4
branch
3 times, most recently
from
88d2e83 to
866368d
Compare
|
Ready for a new round of review. I have addressed the comments, applied most of them. There is still one to-do (update channel reserve values), that I will do, but the rest is ready for review. |
|
Ready for a new round of review. All pending and newly raised comments addressed. |
lightning/src/ln/channel.rs
Outdated
| fn begin_interactive_funding_tx_construction<ES: Deref>( | ||
| &mut self, signer_provider: &SP, entropy_source: &ES, holder_node_id: PublicKey, | ||
| change_destination_opt: Option<ScriptBuf>, | ||
| is_initiator: bool, change_destination_opt: Option<ScriptBuf>, |
There was a problem hiding this comment.
Updated to take FundingNegotiationContext with an added is_initiator field. Left it in interactivetxs.rs for the time being given it's tested there, too. Can move it at the end of the review.
lightning/src/ln/channel.rs
Outdated
| fn begin_interactive_funding_tx_construction<ES: Deref>( | ||
| &mut self, signer_provider: &SP, entropy_source: &ES, holder_node_id: PublicKey, | ||
| change_destination_opt: Option<ScriptBuf>, | ||
| is_initiator: bool, change_destination_opt: Option<ScriptBuf>, | ||
| prev_funding_input: Option<(TxIn, TransactionU16LenLimited)>, |
There was a problem hiding this comment.
Rebased. Now prev_funding_input is shared_funding_input passed as Option<SharedOwnedInput>.
lightning/src/ln/channel.rs
Outdated
|
|
||
| // TODO(splicing): Add prev funding tx as input, must be provided as a parameter | ||
| if is_initiator { | ||
| if let Some(prev_funding_input) = prev_funding_input { |
There was a problem hiding this comment.
Added a fixup for the shared input.
lightning/src/ln/channel.rs
Outdated
| #[cfg(splicing)] | ||
| fn as_renegotiating_channel(&mut self) -> Result<NegotiatingChannelView<SP>, &'static str> { | ||
| if let Some(ref mut pending_splice) = &mut self.pending_splice { | ||
| if let Some(ref mut funding) = &mut pending_splice.funding { |
There was a problem hiding this comment.
But we need the FundingScope and PendingSplice to initialize NegotiatingChannelView, right?
lightning/src/ln/channelmanager.rs
Outdated
| Err(tx_msg_str) => return Err(MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn( | ||
| format!("Got a {tx_msg_str} message with no interactive transaction construction expected or in-progress") | ||
| ), channel_id)), | ||
| Err(err) => return Err(MsgHandleErrInternal::from_chan_no_close(err, channel_id)), |
There was a problem hiding this comment.
We can't add it here. Instead, we would need to do it at each call site of as_negotiating_channel s (i.e. within each HandleTxMsgFn passed here and wherever else it is called).
lightning/src/ln/channel.rs
Outdated
| interactive_tx_constructor: &'a mut Option<InteractiveTxConstructor>, | ||
| interactive_tx_signing_session: &'a mut Option<InteractiveTxSigningSession>, |
There was a problem hiding this comment.
I don't think we need to track
interactive_tx_signing_sessionat all here, as long as we separately set the session infunding_tx_constructed.
This is called from Channel::funding_tx_constructed, so we wouldn't know where to set it on without
interactive_tx_constructoralso shouldn't be anOption, but it seems to be that way right now because ofbegin_interactive_funding_tx_constructionandfunding_tx_constructed.For
begin_interactive_funding_tx_construction, maybe it makes more sense to make it a standalone function (no longer a member onNegotiatingChannelView) that simply returns anInteractiveTxConstructorthat we can set on the channel separately.
We need it in the next commit when calling tx_add_input, et al.
lightning/src/ln/channel.rs
Outdated
| ) -> Result<Option<InteractiveTxMessageSend>, AbortReason> | ||
| where ES::Target: EntropySource | ||
| { | ||
| debug_assert!(matches!(self.context.channel_state, ChannelState::NegotiatingFunding(_))); | ||
| if self.is_splice { | ||
| debug_assert!(matches!(self.context.channel_state, ChannelState::ChannelReady(_))); |
There was a problem hiding this comment.
Was there any resolution on this?
I rebased, but still haven't actually used the shared input. I can see that's now done in |
|
The splicing test fails, but when properly setting the parameters for |
Thanks @optout21! That did fix the test though a bunch of assertions that had commented out marked FIXME still fail after uncommenting. Let me know if anything jumps out. |
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
Pushed a commit to fix the splice test. 3b31240 |
|
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| let post_value_to_self_msat = AddSigned::checked_add_signed( | ||
| prev_funding.value_to_self_msat, | ||
| our_funding_contribution_sats * 1000, | ||
| ); | ||
| debug_assert!(post_value_to_self_msat.is_some()); | ||
| let post_value_to_self_msat = post_value_to_self_msat.unwrap(); |
There was a problem hiding this comment.
The debug_assert!(post_value_to_self_msat.is_some()) assumes that adding our_funding_contribution_sats * 1000 to prev_funding.value_to_self_msat will never overflow. However, this isn't guaranteed, especially with large values.
If the addition overflows, post_value_to_self_msat will be None, and the subsequent unwrap() will panic. Consider handling the overflow case explicitly, perhaps by capping the value at u64::MAX or returning an error if the addition would overflow. This would make the code more robust against edge cases.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
This shouldn't be possible so long as our_funding_contribution_sats doesn't exceed the total bitcoin supply. I will add more checks in the splice-out follow-up PR.
This is a simple rename, DualFundingContext to FundingNegotiationContext, to suggest that this is use not only in dual-funded channel open. Also rename the field dual_funding_context to funding_negotiation_context.
PendingSplice holds a FundingScope being negotiated. However, when implementing funding negotiation, other states are possible depending on which party initiated the splice. Using an enum prevents needing various Option fields which may result in invalid states. When the user initiates the splice, the FundingNegotiationContext must be held until the counterparty responds with splice_ack. At that point enough information becomes available to create a new FundingScope and an InteractiveTxConstructor. When the counterparty initiates the splice, both a new FundingScope and an InteractiveTxConstructor can be created immediately when responding with splice_ack. After the transaction is constructed, those are no longer needed. At that point an InteractiveTxSigningSession is tracked until signatures are exchanged.
FundingNegotiationContext and PendingSplice both hold the user's contribution to a splice, which doesn't need to be duplicated. Instead, only store this in FundingNegotiationContext, which then can be used to create an InteractiveTxConstructor when transitioning to FundingNegotiation::Pending. This commit updates that code to properly compute change outputs using the FundingNegotiationContext by not considering the shared input since it is accounted for in the shared output. Co-authored-by: Wilmer Paulino <[email protected]> Co-authored-by: Jeffrey Czyz <[email protected]>
InteractiveTxConstructor was only used in PendingV2Channel methods, but for splicing those methods are needed for FundedChannel, too. Refactor the code such that each type has a method for accessing its InteractiveTxConstructor such that it can be called in either use, refactoring code out of PendingV2Channel as needed. Co-authored-by: Wilmer Paulino <[email protected]> Co-authored-by: Jeffrey Czyz <[email protected]>
Update splice_channel, split_init, and splice_ack to implement transitioning from splice initialization to funding transaction negotiation. Co-authored-by: optout <[email protected]> Co-authored-by: Jeffrey Czyz <[email protected]>
| if is_splice { | ||
| let message = "TODO Forced error, incomplete implementation".to_owned(); | ||
| // TODO(splicing) Forced error, as the use case is not complete | ||
| return Err(ChannelError::Close(( | ||
| message.clone(), | ||
| ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false), message } | ||
| ))); |
There was a problem hiding this comment.
When returning an error in the splice case, the function should reset funding.channel_transaction_parameters.funding_outpoint to None to maintain consistency with the non-splice error handling path at line 5515. This prevents leaving the channel in an inconsistent state if an error occurs during transaction construction.
| if is_splice { | |
| let message = "TODO Forced error, incomplete implementation".to_owned(); | |
| // TODO(splicing) Forced error, as the use case is not complete | |
| return Err(ChannelError::Close(( | |
| message.clone(), | |
| ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false), message } | |
| ))); | |
| if is_splice { | |
| let message = "TODO Forced error, incomplete implementation".to_owned(); | |
| // TODO(splicing) Forced error, as the use case is not complete | |
| funding.channel_transaction_parameters.funding_outpoint = None; | |
| return Err(ChannelError::Close(( | |
| message.clone(), | |
| ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false), message } | |
| ))); | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
@wpaulino I see that the call site in ChannelManager maps any error returned using MsgHandleErrInternal::send_err_msg_no_close. Doesn't that mean whenever we return ChannelError::Warn in Channel::funding_tx_constructed we'll send an error instead of a warning message?
More generally, do we want to distinguish between what is done for a splice vs v2-establishment? Presumably we don't want to close the channel for the former but would for the latter?
There was a problem hiding this comment.
Correct, we should fix that
lightning/src/ln/channel.rs
Outdated
| @@ -2934,7 +2852,7 @@ where | |||
| where | |||
| L::Target: Logger | |||
| { | |||
| let our_funding_satoshis = self.funding_negotiation_context.our_funding_satoshis; | |||
| let our_funding_satoshis = self.funding_negotiation_context.our_funding_contribution_satoshis; | |||
There was a problem hiding this comment.
Nit: move this down to where it's actually needed
| fn into_interactive_tx_constructor<SP: Deref, ES: Deref>( | ||
| self, context: &ChannelContext<SP>, funding: &FundingScope, signer_provider: &SP, | ||
| entropy_source: &ES, holder_node_id: PublicKey, change_destination_opt: Option<ScriptBuf>, | ||
| shared_funding_input: Option<SharedOwnedInput>, |
There was a problem hiding this comment.
We can/should probably track this in FundingNegotiationContext
| #[cfg(splicing)] | ||
| fn into_interactive_tx_constructor<SP: Deref, ES: Deref>( | ||
| self, context: &ChannelContext<SP>, funding: &FundingScope, signer_provider: &SP, | ||
| entropy_source: &ES, holder_node_id: PublicKey, change_destination_opt: Option<ScriptBuf>, |
There was a problem hiding this comment.
Once we support splice out, we'll want to make the current our_funding_inputs an enum, where the variants are:
SpliceIn { inputs: Vec<(TxIn, TransactionU16LenLimited)>, change_script: Option<ScriptBuf> }SpliceOut { outputs: Vec<TxOut> }Mixed { inputs: Vec<(TxIn, TransactionU16LenLimited)>, outputs: Vec<TxOut> }
| if self.our_funding_contribution_satoshis > 0 { | ||
| let change_value_opt = calculate_change_output_value( | ||
| &self, | ||
| funding.channel_transaction_parameters.splice_parent_funding_txid.is_some(), |
There was a problem hiding this comment.
Let's assert somewhere that when this is set, shared_funding_input also is
|
|
||
| if shared_input.is_some() { | ||
| if is_splice { | ||
| // FIXME: Needs to consider different weights based on channel type |
There was a problem hiding this comment.
TODO(splicing) if we're not going to fix it here
| }, | ||
| }; | ||
| if let Some(msg_send_event) = msg_send_event_opt { | ||
| peer_state.pending_msg_events.push(msg_send_event); | ||
| }; | ||
| if let Some(signing_session) = signing_session_opt { | ||
| if ready_to_sign { |
There was a problem hiding this comment.
Thoughts on naming this negotiation_complete or funding_tx_complete instead?
| let input = TxIn { | ||
| previous_output: funding_txo.into_bitcoin_outpoint(), | ||
| script_sig: ScriptBuf::new(), | ||
| sequence: Sequence::ZERO, |
There was a problem hiding this comment.
We may want to check what other implementations are using here
| })?; | ||
| let tx_msg_opt = interactive_tx_constructor.take_initiator_first_message(); | ||
|
|
||
| debug_assert!(self.interactive_tx_signing_session.is_none()); |
There was a problem hiding this comment.
Should be some as we're the splice initiator
| signer_provider, | ||
| entropy_source, | ||
| holder_node_id.clone(), | ||
| None, |
There was a problem hiding this comment.
There should be an optional change address set here
| if is_splice { | ||
| let message = "TODO Forced error, incomplete implementation".to_owned(); | ||
| // TODO(splicing) Forced error, as the use case is not complete | ||
| return Err(ChannelError::Close(( | ||
| message.clone(), | ||
| ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false), message } | ||
| ))); |
There was a problem hiding this comment.
Correct, we should fix that
Implementation of transaction negotiation during splicing.
Builds on 3407 and 3443.
No new phase,Funded(FundedChannel)is used throughout splicingFundedChannelandPendingV2Channelcan act as a transaction constructorPendingV2Channellogic is put behind a trait --FundingTxConstructorV2RenegotiatingScopeis used to store extra state during splicingFundingChannelcan act as aFundingTxConstructorV2, using the state fromRenegotiatingScope(if present)Since bothFundedChannelandFundingTxConstructorhas context(), context accessors are extracted into a common base trait,ChannelContextProvider(it is also shared byInitialRemoteCommitmentReceiver).(Also relevant: #3444)