Interactive TX negotiation tracks shared outputs

[optout21]

Interactive TX negotiation now tracks the shared funding output. This allows for better checks, better contribution verification.
Shared inputs are not included, those are splicing-specific, shall be done later.
Besed on 36e7452 by @dunxen .

One notable change is that local/remote contribution are not computed by filtering by serial_id and adding values, but summing local/remote values across all inputs/outputs (i.e. in build_transaction()https://github.com/lightningdevkit/rust-lightning/pull/2989/files#diff-c92e994fd9949ebcb6a8d9ec76553e8497ee223b707b51905e35017b80124c77R467).

Housekeeping: this should come after #2981

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.77465% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 91.11%. Comparing base (0e22b12) to head (02a8244).
Report is 94 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/interactivetxs.rs	95.77%	18 Missing and 6 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2989 +/- ##
==========================================
+ Coverage 89.15% 91.11% +1.96% 
==========================================
Files 118 118 Lines 97678 107009 +9331 Branches 97678 107009 +9331 ==========================================
+ Hits 87080 97496 +10416 + Misses 8357 7145 -1212 - Partials 2241 2368 +127 

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

[optout21]

Changed NegotiationContext to store local and remote inputs/output in separate vectors (in separate commit for now)

[dunxen]

Changed NegotiationContext to store local and remote inputs/output in separate vectors (in separate commit for now)

Sorry about this, but I've reverted this in #2981 as there just ended up being too much cloning going around. I believe we will still be fine with the splicing additions here to have single maps for inputs/outputs.

Edit: I've also removed the minimal splicing specific stuff from #2981 so that all that can be done atomically in this PR. See: #2981 (comment).

Basically just removed the to_remote_value_satoshis and the check which wasn't necessarily complete for both splice in and out. So if you rebase and see that then just take note why it was removed.

So a PR title change might be appropriate here.

Thanks! 🙏

[optout21]

Rebased after merge of #2981 .

[dunxen]

Apologies for the delay. I've done some initial review and so far LGTM. Did you want to tackle splicing calculations in a separate PR? I was thinking it would be okay to do that here and track the shared inputs too. Or do imagine that to be quite involved and more than a few extra commits?

Either way this looks good to take out of draft if you're happy and I'll give it another round once you're happy 🙂

[optout21]

Apologies for the delay. I've done some initial review and so far LGTM. Did you want to tackle splicing calculations in a separate PR? I was thinking it would be okay to do that here and track the shared inputs too. Or do imagine that to be quite involved and more than a few extra commits?

With current state of splicing work, I still don't see exactly the way (and the need) to track the shared output, so I would defer that to the time splicing is added.

[optout21]

Folded in proposed comment changes; marked ready for review.

[dunxen]

Folded in proposed comment changes; marked ready for review.

Thanks. I'll give this another look over today.

[dunxen]

LGTM. I'm currently writing some more tests at the channelmanager-level to cover some more interesting funding cases.

Looking forward to rebasing on this.

[optout21]

Made some smaller changes according to review comments.

[optout21]

The last review questions made me thinking, if the API can be made more straightforward.

The user of the InteractiveTxConstructor has to supply the provided outputs (TxOut's), plus it has to provide the script pubkey of the expected shared output (one of the outputs), and its contribution from that. This setup has a few easy places for inconsistency.

Maybe it would be better to wrap the provided outputs in an enum, for different types:

• shared output, but its value belonging entirely to initiator (typical funding output)
• shared output, with partial value belonging initiator (this value has to be provided, this is a less typical case)
• output belonging to initiator (e.g. change)

Additionally, for the infrequent case when we don't specify any outputs, but expect a shared output and some balance belonging to us, an optional (pubkey+localvalue) should be provided.

Maybe this way the API would be easier to understand.
I put the PR back to Draft, for a 2nd thought.

[TheBlueMatt]

Rather than pushing type info upstream, maybe we should have simpler (public) constructors for the InteractiveTxConstructor that are context-specific? ie InteractiveTxConstructor::for_splicing and InteractiveTxConstructor::for_dual_funding are kinda separate things, and could use separate constructors. That would simplify the public interface without adding more types for callers to keep track of.

[optout21]

Reworked:

• In the InteractiveTxConstructor::new(), the type of the contributed outputs has to be specified -- controlled by local node, shared output but all belongs to the local node, or shared output with joint ownership (split).
• Additional expected shared output script has to be specified only in the rare case when the local node does not add a shared output, but expects the remote node to add a joint-owned shared output.

[optout21]

Rather than pushing type info upstream, maybe we should have simpler (public) constructors for the InteractiveTxConstructor that are context-specific? ie InteractiveTxConstructor::for_splicing and InteractiveTxConstructor::for_dual_funding are kinda separate things, and could use separate constructors.

Good idea, though this version aims at dual funding only (only funding outputs), splicing is not considered in this PR

[dunxen]

I've been writing some new tests for #2302 and have encountered some issues due to not tracking the shared output within the constructor and when validating the constructed tx, so this PR is a little bit of a blocker for 2302. I'll integrate these changes for now though so I can carry on with the new tests. :)

[TheBlueMatt]

Sorry for the delay here :(

[optout21]

Addressed review comments (sorry, they fall of my priority table...).

Remaining outstanding:

• rebase
• whether there should be a check in validate_tx for the existence of a shared output

[optout21]

Did remaining changes:

• Add check for missing funding output in validate_tx()
• Make expected_shared_funding_output mandatory in NegotiationContext, as suggested by @TheBlueMatt . In InteractiveTxConstructor either a shared output has to be provided, or a pubkey for the shared output to be added by the remote.

[dunxen]

Just some comment nits, but LGTM. I believe this needs to be formatted with rustfmt as it's not excluded.

[optout21]

Did formatting, plus 2 minor comment wording change (review).

[optout21]

If this is merged before #3137, these interactivetx.rs changes will disappear from #3137 diff. That would make sense, as this PR has been mostly reviewed already.

[dunxen]

If this is merged before #3137, these interactivetx.rs changes will disappear from #3137 diff. That would make sense, as this PR has been mostly reviewed already.

Yip, that's the plan. I just cherry-picked this.

[dunxen]

Since this only affects interactivetxs.rs, I'm going to go ahead and merge this and any other nits/issues can be addressed in a follow-up. Then that should unblock 3137.

[TheBlueMatt]

Post-merge ACK, but a few nits.

[TheBlueMatt]

This should be >=, right (as = should have gone through SharedControlFullyOwned)?

[TheBlueMatt]

This is wrong, right? Single maybe be owned by the counterparty, depending on where its stored.

[TheBlueMatt]

There's no reason for this to be pub, right? Its an internal abstraction.

[TheBlueMatt]

There's no reason for this to be pub, right? Its an internal abstraction.

[optout21]

Looks like visibility could be reduced indeed. I will check in the wider scope where this class is actually used.
These are used in ConstructedTransaction, which is pub(crate).

[TheBlueMatt]

Sounds good, thanks. A followup reducing visibility of the nits here would be much appreciated.