Adds a new flag commitmax to the command openchannel of lncli

[bjarnemagnussen]

Description

This PR introduces a new flag commitmax for openchannel that for the channel capacity will use the whole available wallet balance up to the allowed maximum funding limit (currently ~0.16 btc) when opening a new channel.

Motivation

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.

Further, it seems that this possibility has at least come up as most of the functionality to make this possible already exists, e.g.:

lnd/lnwallet/chanfunding/assembler.go

Lines 67 to 72 in 80af295

	// SubtractFees should be set if we intend to spend exactly LocalAmt
	// when opening the channel, subtracting the fees from the funding
	// output. This can be used for instance to use all our remaining funds
	// to open the channel, since it will take fees into
	// account.
	SubtractFees bool

Edge Case

The commitmax flag makes use of the CoinSelectSubtractFees function to select funds. This implies that if the wallet balance is greater than the sum of the maximum allowed funding limit (~0.16 btc) and the fee, then for the new channel the capacity will be the maximum allowed (~0.16 btc) subtracted by the fee, instead of exactly the maximum allowed (~0.16 btc). This is a slightly wrong behaviour, as in this case it would have been possible to exactly max out the channel capacity and additionally have enough available funds to pay the fee.

I think a solution to this would require adding a field e.g. CommitMax to the chanfunding.Request and adding logic to the ProvisionChannel function inside lnd/lnwallet/chanfunding/wallet_assembler.go to distinguish between opening channels with CommitMax and without. But I think this is too much to specifically change for such an edge case.

Disclaimer

I added this feature because I felt that such a flag was missing and could be useful. But I also implemented it just to get a better understanding of the lnd codebase. Let me know if this feature is useful or not for others.

Testing

I am unsure if this change requires to add tests. But since this change does only use functionality that already existed, I don't see where tests should be added.

Pull Request Checklist

• If this is your first time contributing, we recommend you read the Code
  Contribution Guidelines
• All changes are Go version 1.12 compliant
• The code being submitted is commented according to Code Documentation and Commenting
• For new code: Code is accompanied by tests which exercise both
  the positive and negative (error paths) conditions (if applicable)
• Code has been formatted with go fmt
• 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)
• Running make check does not fail any tests
• Running go vet does not report any issues
• Running make lint does not report any new issues that did not
  already exist
• 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.
• Commits have a logical structure according to Ideal Git Commit Structure

[Roasbeef]

A better way to do this is to use functionality added along side CreateSweepAllTx call to do it from a coin selection perspective. In this case the "delivery" address is just the funding output.

As implemented in this PR, it isn't concurrent safe, as the balance may shift when it's obtained on the CLI compared to when we actually go to fund the transaction. If the above function is used, then this isn't an issue as it locks all coin selection (and funding).

Also a better name for this feature imo is something along the lines of --fundall. See this issue for some prior context as well: #619

[bjarnemagnussen]

Thanks for your input @Roasbeef ! Those are some great insights you gave me. I will take another look at it and try to update this PR.

[bjarnemagnussen]

Will reopen PR as this has undergone major changes.

[ziggie1984]

would be cool to add the information in the comment which was discussed below why the +1 is needed here