Dual funding extension needed also for splicing: begin_interactive_funding_tx_construction

[optout21]

Add these two methods to the current dual funding implementation:
begin_interactive_funding_tx_construction()
calculate_change_output_value()
add_funding_change_output()

These changes are in the pipeline for dual funding implementation, and needed for dual funding negotiation during splicing, in #3444 .

[dunxen]

Makes sense to get this this in so it can unblock you as #2302 will still take time. Is this much different from what is in #2302/any of the splicing PRs?

[optout21]

TODO: I need to run a comparison against current version of dual funding WIP

[optout21]

Did the following:

• break up maybe_add_funding_change_output() into two: one part for determining if a change needs to be added, and the other to actually add it
• moved need_to_add_funding_change_output() into interactivetxs.rs
• added unit tests for need_to_add_funding_change_output()

[jkczyz]

Could you update the commit message with the rationale for the change? (i.e., specifically how it will be used in splicing)

[jkczyz]

Will there ever be a situation where we will need these inputs again? Just wondering what the implications are for take'ing this.

[optout21]

I think the intention with the Option<Vec> was so that it can be taken to avoid cloning.
I've changed that to simply Vec, and use mem::swap to take it out.

[jkczyz]

Chatted a bit more offline. My concerns were around whether we are leaving ourselves in an invalid state by swapping / taking. For dual funding, IIUC, any failure in begin_interactive_funding_tx_construction would result in us dropping the channel (@dunxen is this accurate?). And for splicing, any failure would result in us abandoning the splice. So maybe this is ok.

However, it would be nice if our phase transitions could capture this. For instance, would it make sense to have transitions that effectively encapsulated DualFundingChannelContext, InteractiveTxConstructor, and InteractiveTxSigningSession in separate phases akin to #3423 (comment)?

No need to hold up this PR on this as it would be better to do after #3418. But would love to hear what others think. Specifically, can we get away with forgetting about DualFundingChannelContext once and InteractiveTxConstructor is created? In #2302, I see that it is included in a funded Channel in an Option. How is it needed then?

[dunxen]

@jkczyz, Well for dual_funding we do go ahead and just abandon the channel even though it's technically could just be a tx_abort which means we could start negotiating again without abandoning the channel. However, at that stage we might need the holder to provide other inputs anyway which would need to happen on initiating or accepting a V2 channel. So best to just close it.

For splicing, yeah I'd imagine it would be abandoning the splice. @optout21 can confirm if that's what we'll do.

[optout21]

Yes, in case of failed negotiation state we should abandon the splicing process. With the current solution that involves dropping the OutboundV2Channel instance -- much like in case of open -- and the already Funded original channel instance just remains.

[jkczyz]

Rather than using an Option we can simply mem::swap with an empty Vec.

[optout21]

Well, there is an unwrap involved as well. If I get the suggestion, it would be like this:

    let mut funding_inputs_opt = None;
    mem::swap(&mut self.dual_funding_context_mut().our_funding_inputs, &mut funding_inputs_opt);
    let mut funding_inputs = funding_inputs_opt.unwrap_or(Vec::new());

instead of the original

    let mut funding_inputs = self.dual_funding_context_mut().our_funding_inputs.take().unwrap_or_else(|| vec![]);

I think it's not an improvement in this case, I kept it as is.

[jkczyz]

You wouldn't need an Option at all if the field stays as it was. It would just be:

let mut funding_inputs = vec![];
mem::swap(&mut self.dual_funding_context_mut().our_funding_inputs, &mut funding_inputs);

But see other comment, too.

[optout21]

Update: I've change to simply Vec, so a simple mem::swap can be used.

[optout21]

Review comments addressed, all resolved from my side (left a few open for visibility)

[codecov]

Codecov Report

Attention: Patch coverage is 62.05534% with 96 lines in your changes missing coverage. Please review.

Project coverage is 89.63%. Comparing base (ddeaab6) to head (a379f1d).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	5.88%	96 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3443      +/-   ##
==========================================
- Coverage   89.69%   89.63%   -0.07%     
==========================================
  Files         130      130              
  Lines      107472   107719     +247     
  Branches   107472   107719     +247     
==========================================
+ Hits        96396    96550     +154     
- Misses       8674     8767      +93     
  Partials     2402     2402              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.