[Splicing] Tx negotiation during splicing #3736
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
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
lightning/src/ln/channel.rs
Outdated
| /// Data needed during splicing -- | ||
| /// when the funding transaction is being renegotiated in a funded channel. | ||
| #[cfg(splicing)] | ||
| struct RefundingScope { |
There was a problem hiding this comment.
Why are we introducing yet another structure as opposed to tracking all the fields here in PendingSplice?
There was a problem hiding this comment.
RefundingScope does not live through the whole lifetime of splicing. Not before splice_ack received, and not after tx_complete. PendingSplice has a longer lifetime. Also, the fields are belonging to each other. I could 'flatten' the structure, and just move the fields to PendingSplice, but I think it's clearer if they are in a struct, and can be set to None at once.
There was a problem hiding this comment.
Also, I did remove the PendingSpliceInit sub-struct, and included the few fields in PendingSplice directly (See 0f8acd3)
There was a problem hiding this comment.
I'm still not convinced this is helpful, FundedChannel and RefundingScope are almost the same, why not just implement the trait directly on FundedChannel?
There was a problem hiding this comment.
We have clarified as follows:
RefundingScopedoes not implement the trait, but theFundedChannelRefundingWrapperdoes- The wrapper makes sense to be kept
RefundingScopecould be dropped, and its 4 fields included directly inPendingSpliceFundedChannelRefundingWrappercould be returned byPendingV2Channelas well, and then the trait can be dropped, as only one struct would need it.
There was a problem hiding this comment.
RefundingScope has been dropped, its 4 fields are included directly in PendingSplice.
lightning/src/ln/channel.rs
Outdated
| @@ -2414,6 +2414,7 @@ pub(super) trait FundingTxConstructorV2<SP: Deref>: ChannelContextProvider<SP> w | |||
| 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_splice: bool, prev_funding_input: Option<(TxIn, TransactionU16LenLimited)>, | |||
There was a problem hiding this comment.
Why do we need is_splice if prev_funding_input being set implies we are splicing?
There was a problem hiding this comment.
Because the prev. funding input is set only by the initiator, and this method is used on both side (initiator and acceptor).
There was a problem hiding this comment.
The funding input still needs to be passed through to the interactive constructor though so we can make sure the counterparty adds it when we're not the initiator and we can check it's the same as we expect.
There was a problem hiding this comment.
It is, everything will go through it after begin_interactive_funding_tx_construction
There was a problem hiding this comment.
Clarified: the previous funding as an input is a shared input, and the splice-acceptor side also has to be aware of it, it has to check that it is being added by the initiator. So it has to provided as a special shared input, which is added during negotiation by the initiator, and checked by the acceptor.
There was a problem hiding this comment.
I've tried to do it, but ran into a problem with the tests. Apparently on the acceptor side (v1 channel) the field funding.funding_transaction is never set.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3736 +/- ##
==========================================
+ Coverage 89.73% 90.28% +0.55%
==========================================
Files 159 160 +1
Lines 128910 132032 +3122
Branches 128910 132032 +3122
==========================================
+ Hits 115676 119207 +3531
+ Misses 10536 10172 -364
+ Partials 2698 2653 -45 ☔ 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/channelmanager.rs
Outdated
| try_channel_entry!(self, peer_state, Err(ChannelError::Close(( | ||
| err.into(), | ||
| ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }, | ||
| ))), chan_entry) |
There was a problem hiding this comment.
Is closing the channel here a spec requirement? Seems like we could just drop the message or send a warning.
| self.internal_tx_msg(&counterparty_node_id, msg.channel_id, |channel: &mut Channel<SP>| { | ||
| match channel.as_unfunded_v2_mut() { | ||
| Some(unfunded_channel) => { | ||
| Ok(unfunded_channel.tx_add_input(msg).into_msg_send_event(counterparty_node_id)) | ||
| }, | ||
| None => Err("tx_add_input"), | ||
| } | ||
| Ok(channel.tx_add_input(msg)?.into_msg_send_event(counterparty_node_id)) |
There was a problem hiding this comment.
Both tx_add_input and internal_tx_msg have their own error strings that when combined don't make sense. We should probably drop the one from internal_tx_msg and have tx_add_input, etc. return a ChannelError.
There was a problem hiding this comment.
Makes sense, I've changed, also for tx_complete.
I did remove a Channel::Close from internal_tx_complete(), and I think that makes sense, since we don't want to close on a funded splicing channel in case of a tx_complete error.
lightning/src/ln/channelmanager.rs
Outdated
| // TODO try_channel_entry() | ||
| let splice_ack_msg = chan_entry.get_mut().splice_init(msg, our_funding_contribution, &self.signer_provider, |
There was a problem hiding this comment.
Any reason this is a TODO?
lightning/src/ln/channelmanager.rs
Outdated
| } else { | ||
| return Err(MsgHandleErrInternal::send_err_msg_no_close("Channel is not funded, cannot splice".to_owned(), msg.channel_id)); | ||
| // Handle inside channel | ||
| let tx_msg_opt = chan_entry.get_mut().splice_ack(msg, &self.signer_provider, &self.entropy_source, &self.get_our_node_id(), &self.logger) |
There was a problem hiding this comment.
Why are we not using try_channel_entry anymore?
| @@ -8649,9 +8980,20 @@ impl<SP: Deref> FundedChannel<SP> where | |||
| "Insufficient inputs for splicing; channel ID {}, err {}", | |||
| self.context.channel_id(), err, | |||
| )})?; | |||
| // Convert inputs | |||
| let mut funding_inputs = Vec::new(); | |||
| for (tx_in, tx, _w) in our_funding_inputs.into_iter() { | |||
There was a problem hiding this comment.
Let's just make our_funding_inputs an owned value
There was a problem hiding this comment.
Done, though an extra clone() is added in channelmanager.rs, due to optionally_notify closure.
There was a problem hiding this comment.
Hmm... that's unfortunate. It seems to stem form the fact that PersistenceNotifierGuard has an FnMut rather than an FnOnce. Might be able to fix that but probably better to save for a separate PR.
lightning/src/ln/channel.rs
Outdated
| our_funding_inputs: Vec::new(), // inputs go directly to [`FundingNegotiationContext`] above | ||
| awaiting_splice_ack: false, // we don't need any additional message for the handshake | ||
| refunding_scope, | ||
| }); | ||
| // TODO(splicing): Store msg.funding_pubkey |
There was a problem hiding this comment.
No, this is about storing the funding pubkey of the other party, as it may change. It's done nowhere as of yet.
There was a problem hiding this comment.
Ah, we just have to overwrite it in the new ChannelTransactionParameters
lightning/src/ln/channel.rs
Outdated
| /// Data needed during splicing -- | ||
| /// when the funding transaction is being renegotiated in a funded channel. | ||
| #[cfg(splicing)] | ||
| struct RefundingScope { |
There was a problem hiding this comment.
I'm still not convinced this is helpful, FundedChannel and RefundingScope are almost the same, why not just implement the trait directly on FundedChannel?
optout21
force-pushed
the
splice-dual-tx4
branch
2 times, most recently
from
0333069 to
54ddcbe
Compare
|
I've done some of the bigger changes, but there are still several outstanding. |
lightning/src/ln/channel.rs
Outdated
| @@ -5293,7 +5293,7 @@ fn check_v2_funding_inputs_sufficient( | |||
| } | |||
|
|
|||
| /// Context for dual-funded channels. | |||
lightning/src/ln/splicing_tests.rs
Outdated
|
|
||
| // TODO(splicing): Continue with commitment flow, new tx confirmation | ||
|
|
||
| /* |
There was a problem hiding this comment.
Instead of commenting this out, could you condition the error on whether we are in a test?
There was a problem hiding this comment.
It turned out the proper close works not, so I could uncomment this part B)
| #[cfg(splicing)] | ||
| struct FundedChannelRefundingWrapper<'a, SP: Deref> where SP::Target: SignerProvider { | ||
| struct NegotiatingV2ChannelView<'a, SP: Deref> where SP::Target: SignerProvider { |
There was a problem hiding this comment.
Maybe drop V2 since a V1 channel can be spliced?
| pub our_funding_inputs: Vec<(TxIn, TransactionU16LenLimited)>, | ||
| /// Set when splice_ack has been processed (on the initiator side), | ||
| /// used to prevent processing of multiple splice_ack's. | ||
| awaiting_splice_ack: bool, |
There was a problem hiding this comment.
IIUC, @wpaulino was saying we can check if those are Some?
| @@ -8649,9 +8980,20 @@ impl<SP: Deref> FundedChannel<SP> where | |||
| "Insufficient inputs for splicing; channel ID {}, err {}", | |||
| self.context.channel_id(), err, | |||
| )})?; | |||
| // Convert inputs | |||
| let mut funding_inputs = Vec::new(); | |||
| for (tx_in, tx, _w) in our_funding_inputs.into_iter() { | |||
There was a problem hiding this comment.
Hmm... that's unfortunate. It seems to stem form the fact that PersistenceNotifierGuard has an FnMut rather than an FnOnce. Might be able to fix that but probably better to save for a separate PR.
|
Rebased to current main (there were some conflicts) |
|
Comments addressed, I will clean/reorg up the commits. |
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.
The begin_interactive_funding_tx_construction() method is extended with the prev_funding_input optional parameter, containing the previous funding transaction, which will be added to the negotiation as an input by the initiator.
Introduce struct NegotiatingV2ChannelView to perform transaction negotiation logic, on top of both PendingV2Channel (dual-funded channel open) and FundedChannel (splicing).
Fill the logic for including transaction negotiation during splicing, implement the functions: splice_channel, splice_init, splice_ack, funding_tx_constructed. Also extend the test case test_v1_splice_in with the steps for funding negotiation during splicing.
|
I have rearranged the commits in this PR (there were some reverted changes, many fixes, etc.), into these 4:
Otherwise, no change applied this time. (Note: the old commits are available on branch splice-dual-tx4-save) |
Implementation of transaction negotiation during splicing.
Builds on 3407 and 3443.
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)FundedChannelandFundingTxConstructorhas context(), context accessors are extracted into a common base trait,ChannelContextProvider(it is also shared byInitialRemoteCommitmentReceiver).(Also relevant: #3444)