Include base input fee in fee, in calculate_our_funding_satoshis()

[optout21]

It was found that the (still-unused) calculate_our_funding_satoshis() method doesn't take into account the base weight of the inputs in the fee estimation.
It was also found that this method also misses the intended funding amount.

This change:

• Adds the base weight for inputs to the fee estimation, and adds a unit test
• Breaks up calculate_our_funding_satoshis() into two, estimate_funding_transaction_fee(), and the output calculation. This is to allow using funding tx fee estimation, needed also by splicing (see [Splicing] Partial, handle splice_init & splice_ack messages #3407).
• Cleans up calculate_our_funding_satoshis(), as it's not used and misses the intended funding amount.

Note: this was triggered by work on splicing (#3407),

[optout21]

Note: #3407 also include this change, I will take care of merge if needed regardless of which PR is merged first.

[dunxen]

Basically LGTM, modulo Jeff's remaining comments.

[optout21]

Please consider for merging.

[jkczyz]

Could you re-organize the commits a bit? Let's do two commits where the first one contains the the base input fee fix and the new test. The second commit can contain the refactor and related test changes.

[optout21]

Let's do two commits where the first one contains the the base input fee fix and the new test. The second commit can contain the refactor and related test changes.

Commits reorganized

[jkczyz]

Please update PR description.

[wpaulino]

I had a look at how calculate_our_funding_satoshis is used and I'm a bit confused. For an inbound channel, we should have the user provide us the amount they want to contribute, along with the inputs, and we should make sure the inputs provided can cover their intended contribution amount after fees, etc. We shouldn't assume the user always wants to contribute as much as possible from their inputs (e.g., like send-all in an onchain wallet). So we basically want two modes: ContributeAll(inputs) and ContributeAmount(amount, inputs, change_address).

[optout21]

I had a look at how calculate_our_funding_satoshis is used and I'm a bit confused. ...

I'm also a bit confused.
The calculate_our_funding_satoshis is currently used in dual funding channel open, on the acceptor side only. Moreover, currently the provided inputs are hardcoded to be none, so this method is not effectively used.

The method computes the contributed inputs minus the proportional fees, that is, how much we could contribute to the funding with these inputs. If that's below dust, it means these inputs make no economic sense (BTW the error message is not very specific).
But indeed, I don't see here the distinction between the amount we contribute in the inputs and the amount we intend to contribute. Also, there is no information about eventual change (destination).

As currently there is no way to specify contributing inputs on the acceptor side, this logic is currently irrelevant. V2 opening as initiator is still missing entirely (still only in PR).

I made this spin-off to separate just the one-liner estimation fix.

[wpaulino]

Let's just delete calculate_our_funding_satoshis then? We probably want to think more about how the data we need from the user is given to us, and the method will have to change a good bit by then anyway.

[optout21]

Here's my conclusion:

• calculate_our_funding_satoshis is conceptually unclear, as check should take into account intended funding amount as well. Besides, it's not used in the current form, so it's better to remove it now (to avoid confusion).
• As contribution from acceptor is not supported, it's better to clean up the hardcoded empty values and the parameters in the level above.
• estimate_v2_funding_transaction_fee: This is not used either, but since it's to be used soon ([Splicing] Partial, handle splice_init & splice_ack messages #3407), and it has unit tests, I would keep it as done here.

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.98%. Comparing base (2c3f11d) to head (2bb7aa2).
Report is 75 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #3558 +/- ##
==========================================
+ Coverage 88.53% 89.98% +1.44% 
==========================================
Files 149 149 Lines 114985 130021 +15036 Branches 114985 130021 +15036 ==========================================
+ Hits 101803 116998 +15195 + Misses 10693 10619 -74 + Partials 2489 2404 -85 

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

[jkczyz]

• estimate_v2_funding_transaction_fee: This is not used either, but since it's to be used soon (Include base input fee in fee, in calculate_our_funding_satoshis() #3558), and it has unit tests, I would keep it as done here.

Should this say #3407?

[optout21]

Should this say #3407?

Of course, my bad, corrected

[jkczyz]

@wpaulino If you're ok with this then I'd say we just squash this PR into one commit.

[wpaulino]

Yeah looks good, let's keep two commits though: one removing the dead code, and another with the bug fix.

[optout21]

Squashed into the two commits.

[wpaulino]

Sorry, last few nits: please keep commit messages to 72 characters max and include the rationale behind dropping calculate_our_funding_satoshis in the commit message

[optout21]

Sorry, last few nits: please keep commit messages to 72 characters max and include the rationale behind dropping calculate_our_funding_satoshis in the commit message

Noted, commit msg/desc adjusted.

[jkczyz]

Sorry, last few nits: please keep commit messages to 72 characters max and include the rationale behind dropping calculate_our_funding_satoshis in the commit message

Noted, commit msg/desc adjusted.

LGTM, but looks like the commit descriptions are still over 72 characters. Could you re-wrap? FYI, we should use these guidelines: https://cbea.ms/git-commit/.

[optout21]

LGTM, but looks like the commit descriptions are still over 72 characters. Could you re-wrap? FYI, we should use these guidelines: https://cbea.ms/git-commit/.

Commit msgs adjusted!