[Early Draft][Splicing] Interactive tx negotiation, with a single ChannelContext#3715
[Early Draft][Splicing] Interactive tx negotiation, with a single ChannelContext#3715optout21 wants to merge 6 commits intolightningdevkit:mainfrom
Conversation
|
👋 I see @wpaulino was un-assigned. |
| pub fn tx_add_input(&mut self, msg: &msgs::TxAddInput) -> InteractiveTxMessageSendResult { | ||
| match &mut self.phase { | ||
| ChannelPhase::UnfundedV2(chan) => chan.tx_add_input(msg), | ||
| #[cfg(splicing)] | ||
| ChannelPhase::RefundingV2(chan) => chan.tx_add_input(msg), | ||
| _ => panic!("Got tx_add_input in an invalid phase"), | ||
| } | ||
| } | ||
|
|
||
| pub fn tx_add_output(&mut self, msg: &msgs::TxAddOutput) -> InteractiveTxMessageSendResult { | ||
| match &mut self.phase { | ||
| ChannelPhase::UnfundedV2(chan) => chan.tx_add_output(msg), | ||
| #[cfg(splicing)] | ||
| ChannelPhase::RefundingV2(chan) => chan.tx_add_output(msg), | ||
| _ => panic!("Got tx_add_output in an invalid phase"), | ||
| } | ||
| } | ||
|
|
||
| pub fn tx_complete(&mut self, msg: &msgs::TxComplete) -> HandleTxCompleteResult { | ||
| match &mut self.phase { | ||
| ChannelPhase::UnfundedV2(chan) => chan.tx_complete(msg), | ||
| #[cfg(splicing)] | ||
| ChannelPhase::RefundingV2(chan) => chan.tx_complete(msg), | ||
| _ => panic!("Got tx_complete in an invalid phase"), | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think we can panic in these?
There was a problem hiding this comment.
The aim of this PR is first to get the big picture right, conceptually, and not to work out all the details at once. Of course this has to be refined.
| pub fn as_funded(&self) -> Option<&FundedChannel<SP>> { | ||
| if let ChannelPhase::Funded(channel) = &self.phase { | ||
| Some(channel) | ||
| } else { | ||
| None | ||
| match &self.phase { | ||
| ChannelPhase::Funded(channel) => Some(&channel), | ||
| #[cfg(splicing)] | ||
| ChannelPhase::RefundingV2(channel) => Some(&channel.funded_channel), | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| pub fn as_funded_mut(&mut self) -> Option<&mut FundedChannel<SP>> { | ||
| if let ChannelPhase::Funded(channel) = &mut self.phase { | ||
| Some(channel) | ||
| } else { | ||
| None | ||
| match &mut self.phase { | ||
| ChannelPhase::Funded(channel) => Some(channel), | ||
| #[cfg(splicing)] | ||
| ChannelPhase::RefundingV2(channel) => Some(&mut channel.funded_channel), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
Hmmm... can we do this? For instance, internal_update_add_htlc calls as_funded_mut. Presumably, for the RefundingV2 case, the channel would be in quiescence, so the check in internal_update_add_htlc would prevent this from being an issue. So maybe this is desirable if we need to cover both cases elsewhere. IIUC, we would be in quiescence if and only if we are in ChannelPhase::RefundingV2. cc: @wpaulino
There was a problem hiding this comment.
Touchy question. If the phase when the new funding has been negotiated but not yet locked is not represented by RefundingV2, then you are probably right.
However, currently as_funded is used in many cases, such as listing channels or force close.
| let (pending_splice_post, post_funding, dual_funding_context, unfunded_context) = | ||
| prev_chan.splice_init(msg, our_funding_contribution)?; | ||
|
|
||
| let _res = self.phase_from_funded_to_splice(post_funding, dual_funding_context, unfunded_context, pending_splice_post)?; |
There was a problem hiding this comment.
We should just inline phase_from_funded_to_splice in the two places it is used rather than introducing unreachables. There isn't much happening in there to justify refactoring it into a method, IMO.
There was a problem hiding this comment.
I consider these concerns low-level code improvement, not in focus in this PR. Being in separate functions increases visibility. Should be addressed in non-draft PR.
| // let (signing_session, holder_commitment_point, commitment_signed, event) = | ||
| let (commitment_signed, event) = | ||
| chan.funding_tx_constructed(signing_session, &&logger)?; | ||
| let _res = self.phase_from_splice_to_funded()?; |
There was a problem hiding this comment.
No error is swallowed here, ? is used. the return value is nothing (()). Or do you mean something else?
| let (pending_splice_post, post_funding, dual_funding_context, unfunded_context, our_funding_contribution) = | ||
| prev_chan.splice_ack(msg)?; | ||
|
|
||
| let _res = self.phase_from_funded_to_splice(post_funding, dual_funding_context, unfunded_context, pending_splice_post)?; |
There was a problem hiding this comment.
Similarly, why swallow the error here and in splice_init?
There was a problem hiding this comment.
No error is swallowed here, ? is used. the return value is nothing (()). Or do you mean something else?
|
👋 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. |
|
Continued in #3736, closing this one |
3 participants
This is a continuation of #3444, but without duplication of
ChannelContext.A
RefundingV2(RefundingChannel)phase is introduced, whereRefundingChannelacts both as funded and pending channel. It has:FundedChannel)PendingV2ChannelTrait, a new trait also implemented byPendingV2, so it can act as a Pending channel.RefundingChannelalso has similar fields toPendingV2Channel, except for context, which is reused from the funded channel.Related PRs:
Supersedes #3630 .