adds `fundmax` flag to `openchannel`

[bjarnemagnussen]

Related to PR #4024, but reopened as a new PR as this feature has undergone major changes.

Description

This PR introduces a new flag fundall for openchannel, which when opening a new channel uses the whole available wallet balance for the channel capacity up to the allowed maximum funding limit (currently ~0.16 btc).

Motivation

See also #619

The command sendcoins has a flag sweepall that will spend all the coins from the wallet. However no such flag exists when opening new channels using openchannel.

Wanting to use the total available wallet balance in the local amount when opening a channel requires calculating the needed fee and subtracting it from the balance manually.

Implementation

A new coin select function CoinSelectUpToAmount has been introduced, which selects coins up to the requested amount, or below if there are not sufficient funds available. Fees and dust amounts are added or subtracted from this amount or the change output depending on the available funds, see also the new test cases.

Considerations

This implementation should be concurrent safe, becuase it locks the coins prior to selecting them, as currently done by the wallet assembler for the coin select subroutine.

Pull Request Checklist

• [x] If this is your first time contributing, we recommend you read the Code Contribution Guidelines
• [x] All changes are Go version 1.12 compliant
• [x] The code being submitted is commented according to Code Documentation and Commenting
• [x] For new code: Code is accompanied by tests which exercise both the positive and negative (error paths) conditions (if applicable)
• [x] Code has been formatted with go fmt
• [x] For code and documentation: lines are wrapped at 80 characters (the tab character should be counted as 8 characters, not 4, as some IDEs do per default)
• [x] Running make check does not fail any tests
• [x] Running go vet does not report any issues
• [x] Running make lint does not report any new issues that did not already exist
• [x] All commits build properly and pass tests. Only in exceptional cases it can be justifiable to violate this condition. In that case, the reason should be stated in the commit message.
• [x] Commits have a logical structure according to Ideal Git Commit Structure

[halseth]

The wallet/fundingmanager already has a subtractFees flag that can be used for this purpose: https://github.com/lightningnetwork/lnd/blob/3aca9d24b849822b0c345985f2c4d27cad688b22/lnwallet/wallet.go#L69

It should significantly reduce the complexity of this PR I think.

[bjarnemagnussen]

The wallet/fundingmanager already has a subtractFees flag that can be used for this purpose:

https://github.com/lightningnetwork/lnd/blob/3aca9d24b849822b0c345985f2c4d27cad688b22/lnwallet/wallet.go#L69

It should significantly reduce the complexity of this PR I think.

I was trying to use this flag, but since the maximum funding amount is limited to the constant lnd.MaxFundingAmount, we don't always want to use it. It is only if the balance is less than lnd.MaxFundingAmount, and in special cases where the balance is just slightly greater than lnd.MaxFundingAmount but not enough to cover the fees. Then we actually will subtract the fee from the total balance which will give a funding amount a little less than this constant.

But I did use the CoinSelectSubtractFee function for inspiration a lot, and it may be that I should try to combine the new CoinSelectUpToAmount with it?

I saw there may also be an open TODO in that regard for the excess fee. Furthermore, CoinSelect does not make correct fee calculation if change amount is dust, which is solved by CoinSelectSubtractFee and CoinSelectUpToAmount.

[halseth]

@bjarnemagnussen I'm not sure I understand the limitation of the current API. If you do a regular channel opening with the MaxFundingAmt value, it will open that channel if the balance is available for that channel size + the fee. If the balance is not available, one can use the subtractFees flag, wit a value set to the remaining wallet balance. Wouldn't this work?

[bjarnemagnussen]

@halseth This works well, and I have added a commit to do as you suggest! :) Hopefully this is something along the line of what you were thinking?

Mismatch between coin select functions

There is this little mismatch between CoinSelect and CoinSelectSubtractFees that probably should be considered.

The CoinSelect function always uses a fixed fee calculation for one input and one change output. Let's assume the balance is one satoshi greater than the sum of MaxFundingAmt and a fee for one input and no change output. Then the CoinSelect function will return an error for insufficient funds, and the CoinSelectSubtractFees function takes over.

Inside CoinSelectSubtractFees we use the whole balance but the smaller fee calculation for one input and no change output. The final output amount returned from CoinSelectSubtractFees will therefore be the smaller fee subtracted from the total balance, which will result in an amount one satoshi greater than the MaxFundingAmt.

An easy fix is to use a sanity check that simply makes sure that we do not return an amount greater than what we asked for. Any difference should be small (fee difference between one and no change output) and can directly go towards the fee.

Test cases

I have for now added a test case "WIP: mismatch of fee calculation between functions" that will pass due to the added sanity check explained above, but otherwise would give rise to an output amount greater than the specified maximum.

There is also an oddity regarding testing for dust change, as it depends on which branch inside CoinSelectUpToAmount is used. Since CoinSelectUpToAmount now makes use of CoinSelect and CoinSelectSubtractFees it depends on their different ways of handling dust change. I have removed testing for dust change as those should be handled by the other coin select functions. I have therefore also removed the test case "no amount to pay fee for change".

[bjarnemagnussen]

I am still not sure how to handle both the MaxFundingAmount and the minChanFundingSize better with the fundall flag. Any other idea on how to streamline this?

[guggero]

Could you rebase the PR please? I'm going to take over the review for @halseth.

I am still not sure how to handle both the MaxFundingAmount and the minChanFundingSize better with the fundall flag. Any other idea on how to streamline this?

Didn't read through all comments yet, but couldn't we just check the walletbalance from the command line and if it's higher than MaxFundingAmount we just say "wallet balance exceeds minimum channel size, --fundall flag cannot be used, please specify exact amount" or something?

[bjarnemagnussen]

I have rebased to the current v0.10.0-beta.rc5 master commit.

Just to make sure we expect the same behaviour I am listing some cases how the fundall flag should open channels:

• In general if balance < minChanFundingSize: Fail just as is current behaviour for openchannel.
• If balance < MaxFundingAmount + open-tx-fee: Open a channel by maxing out the capacity resulting in no change, equivalent to opening a channel using the whole balance with SubtractFees set.
• If balance >= MaxFundingAmount + open-tx-fee: Open a channel using MaxFundingAmount with a change amount to the wallet.

Remark: In the current implementation an edge case can raise a bug resulting in a channel capacity greater than MaxFundingAmount. This is caught with a sanity check, see https://github.com/lightningnetwork/lnd/pull/4029#issuecomment-592955663 above.

[bjarnemagnussen]

I commented above before committing, so sorry for that!

I have renamed fundall to fundmax and added comments in the code to explain a bit more what is going on.

[guggero]

No worries. I'll have a look at it today and test it more thoroughly.

[bjarnemagnussen]

Let me just one more time mention https://github.com/lightningnetwork/lnd/pull/4029#issuecomment-592955663 for when you find the time to test this more thoroughly.

I am not particularly happy about this edge case and how it is currently resolved. I think this should be solved by aligning the fee calculation of the two coin-selection functions (CoinSelect and CoinSelectSubtractFees). But I am not sure if there are concrete plans to do this in the near future, since TODOs already exist in the code of the CoinSelect function.

If needed, I could take a look at the TODO inside CoinSelect to make coinSelect not estimate change output for dust change, which is already correctly handled by ´CoinSelectSubtractFee`.

[guggero]

I tested this a bit on regtest today. Ran the following test cases:

1. Wallet amount is dust (2000sat): output amount(-0.00004087 BTC) after subtracting fees(0.00006087 BTC) below dust limit(0.00000573 BTC) :heavy_check_mark:
2. Wallet amount < min chan size (18000sat): available funds(0.00017739 BTC) below the minimum amount(0.0002 BTC) :heavy_check_mark:
3. Wallet amount > min chan size (20400sat): new channel over 20k sats :heavy_check_mark:
4. Wallet amount > max chan size (20000000sat): new max size channel :heavy_check_mark:
5. Wallet amount > max chan size, push amount == amount: Funder balance too small (-9049000) with fee=9050 sat, minimum=1146 sat required :heavy_check_mark:
6. Wallet amount > max chan size, push amount 16766000: good (local balance 2165, remote balance 16766000, fee 9050, reserve: 167772) :heavy_check_mark:
7. Test the above without --fundmax flag: good, same result :heavy_check_mark:

So far so good. I'll have a look at the fee calculation difference between CoinSelectSubtractFee and CoinSelect you mentioned.

[bjarnemagnussen]

I am looking into your suggested changes but have a bit of noob question regarding where to add the tests.

Most of them can indirectly be tested with the coin select function, such as the coin select function not picking up dust amounts; behaviour of selecting coins close to some max amount; etc.

But I cannot find a dedicated place where "channel opening"/wallet_assembler.go is tested. Since the logic to make sure the selected amount is not below LocalAmount is inside the wallet_assembler.go file, I am not sure where to add the test for it without an existing wallet_assembler_test.go file.

This yields another question: Would it make sense to add a parameter minAmt to CoinSelectUpToAmount and instead have the logic to not return selected coins below this amount inside this function and test it there?

[guggero]

I think the unit test you added is sufficient. It does test the added coin selection code.

What I mean is to add a new itest (see lntest/itest). There you can start a new node and send the amount you want to test with to it, then open channels. It's a bit of work to set everything up. But there isn't an easier way to test the interplay between the funding manager and the wallet currently.

[bjarnemagnussen]

I have taken a look at the suggested changes and hopefully addressed them with the new commits.

Regarding the integration tests: I am not sure if they are fine like that. I think it could have been made more compact with a single testing function, but I wasn't sure how to simply "reset" the nodes "carol" and "dave", so that they have no UTXOs in the wallet. Therefore I "killed" those nodes and made new ones with each new testing function.

//edit: Rebased to current master since the rpc-proto seems to have been adjusted a bit.

[bjarnemagnussen]

Thanks for your inputs @guggero! It took me a few weekends but I feel that the integration tests are now much better structured and clear.

Two "special" test cases

I left two "special" test cases (wallet amount > max chan size, push amount == max-chan-size and wallet amount > max chan size, push amount == max-chan-size - 1), which also contain comments marked with //! that will be deleted after discussing them. I think they are important tests but are actually not directly related to the fundmax flag. They also trigger an error with implementation details in mind, and therefore I am not sure if they should be kept or not.

Regarding the failed CI pipeline

• I am not sure why the linter reports failure. I can see that inside the fundChannel function of fundmanager_test.go the parameter numConfs is unsued, but I didn't touch that and only added one more parameter fundMax, which is used.
• The neutrino integration fails with an error that I think should be unrelated to my added tests?

Let me know if you can see how to improve it, or if I missed something!

[bjarnemagnussen]

Thanks @guggero! I have looked into the issues you raised and hope that they are fixed now. See also my two comments regarding your suggestions to the rest of the code.

I think that the code now follows the style guidelines better, but if you should find anything more just let me know.

[halseth]

Needs rebase!

[bjarnemagnussen]

Rebased and added a commit to adjust the maximum funding amount based on if Wumbo is enabled both locally and advertised by the remote node. See also my two comments regarding some implementation details to make sure subtleties are understood correctly.

[bjarnemagnussen]

I have pushed the changes! See comments for a few points that are still unclear.

[bjarnemagnussen]

@guggero I will take a look in the next weeks!

// Edit: And thanks for picking it up again!

[bjarnemagnussen]

I will wait for PR #4588 to be merged, which allows some minor simplification of this PR as then the fee calculation is streamlined. We then don't need to make the sanity check for edge cases.

[guggero]

@bjarnemagnussen #4588 has been merged, this can now be rebased.

[bjarnemagnussen]

This is still a WIP as I will add new logic in the next days.

I have rebased the code and it was a bit more involved as a lot of refactoring has happened, including deleted files, etc.

I have removed logic for wumbo for now, since I need to clarify a few things. It seems that a new flag maxchansize was introduced, which overrides the default values. I was looking through the code, but it seems there is no way to communicate between the nodes their own maxChanSize parameter, which they will accept for incoming channels?

The problem then is (with and without Wumbo), that currently fundmax uses the default max-value (0.16... BTC), but should respect the funding managers maxChanSize, which is easy to fix. But even then it is not known what value the remote node would accept.

A solution may be something along the lines of allowing an optional value with the fundmax flag, which sets the maximum value to try to open a channel node with and can be set to the remote's maxChanSize if it is known, and otherwise if the value is set to default 0 will use the local node's maxChanSize.

I will incorporate this with additional commits in the next days.

[bjarnemagnussen]

I have rebased and added a new commit 810d331 with the following changes.

When using the fundmax flag it will now use the options set with maxchansize and minchansize. This way if wumbo is enabled but the maximum channel size has been set lower, the fundmax flag will respect this limit even though this setting is meant for incoming channels.

I think this is a good balance of staying safe in regard to not unexpectedly spend all wumbo funds with a channel open, while keeping this flag useful. However, I would also consider e.g. letting fundmax only spend up to the "legacy" maximum channel balance (or should it be the configured maxchansize if it is lower?) and adding a fundmaxwumbo to spend up to the new wumbo maximum channel size.

The rest of the commits are as before, however with changes due to the rebase.

[guggero]

@bjarnemagnussen sorry this didn't make it into 0.14.0. Are you willing to give this another attempt for 0.15? I think we're really close and just need to find another reviewer (and currently a rebase plus an addition to the not-yet-existing 0.15 release notes).

[bjarnemagnussen]

Hey @guggero, this PR feels like my little "lifework" in LND for so long that it has been running under review already :) So yes, I am definitely willing to give this another attempt!

As a first step I will look into getting it working again on current master.

[guggero]

@bjarnemagnussen cool, glad to hear you're willing to see this one through :sweat_smile: can you please re-request review once you were able to rebase? Thanks!

[guggero]

!lightninglabs-deploy mute 2022-Feb-01

[bjarnemagnussen]

I have rebased this PR and added two commits to respect the reserve wallet balance in case of anchor channels.

One thing to consider is regarding the different commit types. Should the integration test also check the "lease" commit type, and should there be an integration test that fails whenever a new type is introduced that is not tested for to be sure the behaviour is always correct?

[saubyk]

@guggero waiting eagerly for this feature.