funding: add `fundmax` flag to `OpenChannelRequest`

[hieblmi]

CC @bjarnemagnussen, @saubyk, @positiveblue: This PR continues the work of PR https://github.com/lightningnetwork/lnd/pull/4029. I am pasting the previous PR description below to make adjustments.

The original PR is related to https://github.com/lightningnetwork/lnd/pull/4024.

Description

This PR introduces a new flag fundmax for OpenChannelRequest, which when opening a new channel uses the whole available wallet balance for the channel capacity up to the allowed maximum funding limit (~0.16 BTC by default, 10 BTC if wumbo channels are enabled) while considering channel funding fees and an optional anchor reserve.

Motivation

A frequently requested feature of lnd is the ability to spend all available wallet funds towards a channel opening. This would allow to put the maximum available balance towards a node operation not leaving any funds idle. For reference see:

• https://github.com/Ride-The-Lightning/RTL/issues/588
• https://github.com/lightningnetwork/lnd/issues/619

To achieve a similar behavior users today manually select amounts that slightly undershoot the optimal sweep amount which leaves the required reserve plus a small residual balance in the wallet. Ideally that small balance, a consequence of the undershooting, should go towards the channel opening.

The SendCoins RPC offers a send_all flag that attempts to send all the coins under control of the internal wallet to a specified address. Similarly, OpenChannel should implement functionality to use all available wallet funds towards a channel opening.

Implementation

A new FundMax flag is added to the OpenChannel RPC, --fundmax to lncli openchannel respectively. As of now this flag cannot be used in combination with psbt. When using this flag the implementation ensures that the channel capacity adheres on the lower end to the user specified minimum channel size, or if not defined, to lnd's default minimum channel size(20_000 sats). The maximum channel size is bound by either the user specified maximum channel size or lnd's default maximum channel size, ~16M sats, and 10BTC if wumbo channels are enabled.

New coin select function

To select coins that represent this narrowed down balance, a new coin select function CoinSelectUpToAmount has been introduced. It selects coins up to the requested amount, or below if there are not sufficient funds available. More specifically the function

1. first checks if the fundmax amount is available exclusive of funding transaction fee and optional anchor channel reserve.
2. if the first check fails the function will try to subtract fees and anchor reserve from the total wallet balance and see if that fulfils our fundmax request.

If the coin selection succeeds in any of the two cases then the function additionally checks if the remaining wallet balance is below the dust limit and if so it will add it to the channel funding amount.

fundmax behavior for anchor channels and future channel types

Since anchor channels require an extra wallet reserve to ensure the ability to fee-bump stuck transactions the implementation reserves sufficient funds for public anchor channels. Since private anchor channels are mostly used as last hops in a route and usually do not take part in routing activity they are not considered for an anchor reserve.

To remind future channel-type developers to consider the required reserve of the fundmax flow a new sanity test has been added to the funding manager(see TestCommitmentTypeFundmaxSanityCheck). Whenever a new channel type is added to the proto files this test fails unless consciously handled by the implementer.

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.

[hieblmi]

This PR so far has only taken https://github.com/lightningnetwork/lnd/pull/4029 and rebased it with the latest master. Next steps are to make the build pipeline checks succeed and then address the comments in this PR's predecessor.

[guggero]

Thanks for picking up this PR! I think you should attribute the original author in the commits. At least those that you took over more or less unchanged.

[hieblmi]

Thanks @guggero, yes good point. I will attribute the commits to Bjarne.

[hieblmi]

I've went through the parent PR top to bottom and now have a better grasp of what the difficulties were up to this stage and how some of them were addressed even though it still feels a little complex.

The parent PR has been rebased here with all its commits attributed to @bjarnemagnussen. Some linter issues and variable contexts have been adjusted so that all tests are passing.

I will now pick up the work from @guggero's last change requests -> https://github.com/lightningnetwork/lnd/pull/4029#pullrequestreview-866128472, try and make CoinSelectUpToAmount more legible and add the requested tests.

[guggero]

I will now pick up the work from @guggero's last change requests -> https://github.com/lightningnetwork/lnd/pull/4029#pullrequestreview-866128472, try and make CoinSelectUpToAmount more legible and add the requested tests.

Sounds good to me! Can you please re-request review once you've submitted the changes? Going to remove the review request for now as the PR is still work in progress.

[saubyk]

@guggero If the open channel is being invoked with a psbt(e.g. for a specific UTXO) and fundmax flag is specified, is it possible to open the channel with just the amount in the UTXO (minus fee)?

@hieblmi have you covered this scenario in your testing? From the description on the PR it appears that the scope of fundmax is only to ensure that entire wallet balance is used up for opening the channel.

[hieblmi]

@saubyk I don't see psbt scenarios covered in the parent PR. I am currently incorporating requested changes to the itest and am at this stage to add a test for the anchor reserve: https://github.com/lightningnetwork/lnd/pull/4029/files/2a2d6ca9c66038f3bad15a9818c978ed05285a56..827685603340218c103a492efeca97f37ce7e45e#r794496922.

It makes sense to use the fundmax flag in the context of a psbt channel opening but don't think that this has been considered in this PR so far.

[guggero]

@guggero If the open channel is being invoked with a psbt(e.g. for a specific UTXO) and fundmax flag is specified, is it possible to open the channel with just the amount in the UTXO (minus fee)?

I'm not sure. I think the PSBT funding bypasses the whole coin selection process and therefore most of the changes in this PR. But I think that question would make for an excellent integration test case in this PR.

[hieblmi]

@guggero I think I've addressed all the changes you and others had requested in the previous PR https://github.com/lightningnetwork/lnd/pull/4029 and linked them to this PR starting from your comment here:https://github.com/lightningnetwork/lnd/pull/4029#pullrequestreview-866128472.

Furthermore I think there is still a point up for discussion from the previous PR namely whther or not we should add an integration test that fails if a new channel commitment type is introduced to remind us to account for its fundmax behavior as proposed by @bjarnemagnussen in this comment: https://github.com/lightningnetwork/lnd/pull/4029#issuecomment-1000242502?

Regarding the psbt test I have a noobish question. Doesn't a psbt channel opening in combination with --fundmax only make sense if the channel opener is able to select coins from the same wallet as the signer? If so that would exclude all offline signing flows unless they implement some sort of fundmax standard. It might make sense to use it in the watch-only scenario described here https://github.com/lightningnetwork/lnd/blob/master/docs/psbt.md#2b-use-lnd-to-create-a-funding-transaction. But on the other hand I don't see calls to CoinSelect, CoinSelectSubtractFees or CoinSelectUpToAmount from the psbt_assembler. Hence @guggero's hunch that we bypass the coin selection in the psbt flow is I think right.

[guggero]

@hieblmi thanks a lot for the updates. I'll take a look at the PR some time soon.

About your questions:

whther or not we should add an integration test that fails if a new channel commitment type is introduced

Sure, anything that makes our lives easier in the future is nice. Though I'm not sure it's an easy thing to do. Since we don't yet know what the new channel type would be, how could we write a test against it?

Doesn't a psbt channel opening in combination with --fundmax only make sense if the channel opener is able to select coins from the same wallet as the signer?

I think the idea of the --fundmax flag in combination with the PSBT would be tell the wallet to open the largest possible channel from the UTXOs already selected in the PSBT. At least that would be my personal expectation of how this should work. Basically skip all coin selection and choose the channel size so there's no change output.

[hieblmi]

Sure, anything that makes our lives easier in the future is nice. Though I'm not sure it's an easy thing to do. Since we don't yet know what the new channel type would be, how could we write a test against it?

I've added a test in this commit: https://github.com/lightningnetwork/lnd/pull/6903/commits/1301bdef9616977bee1dfcd5727b3ea0549333ae It keeps an independent list of all commitment types up to this point in time. The proto types defined in lightning.pb.go from the map CommitmentType_name or CommitmentType_value are then compared to it. A missing channel type in the local test list leads the developer to the unit test that fails which keeps instructions to consider the --fundmax flag.

Doesn't a psbt channel opening in combination with --fundmax only make sense if the channel opener is able to select coins from the same wallet as the signer?

I think the idea of the --fundmax flag in combination with the PSBT would be tell the wallet to open the largest possible channel from the UTXOs already selected in the PSBT. At least that would be my personal expectation of how this should work. Basically skip all coin selection and choose the channel size so there's no change output.

To clarify my understanding of the psbt channel funding flow, could you tell me if I am getting the sequence you are suggesting right?(following this psbt.md)

In the first step we create the interactive psbt funding session with --fundmax:

lncli openchannel --node_key xxx --fundmax --psbt

OUTPUT_ADDRESS

In step 2 the creation of the funding transaction is then supposed to consume the total of the specified inputs without change output minus fees/ reserve etc.?

lncli wallet psbt fund --outputs='{"OUTPUT_ADDRESS"}' --inputs='["INPUT_1:IDX_1","INPUT_2:IDX_2"]'
{
        "psbt": "...",
        "change_output_index": NONE
        "locks": [
                ...
        ]
}

On another note, I was thinking that it might be desirable to have an option in openchannel that allows for the specific selection of outpoints to fund a channel. Something along the lines of lncli openchannel --node_key... --out_points tx1:idx tx2:idx.... Would that make sense?

[hieblmi]

Thanks for the review @guggero - would you also be able to comment on my questions above about a new test case and the psbt/fundmax behavior? --> https://github.com/lightningnetwork/lnd/pull/6903#issuecomment-1254996287

[guggero]

In step 2 the creation of the funding transaction is then supposed to consume the total of the specified inputs without change output minus fees/ reserve etc.?

Yes, that's the idea. But because the PR is already pretty complex, I'd only add that if it can easily be incorporated into the current flow. If it requires a big additional diff, then I'd say for now just add an error that the FundMax and Psbt cannot be used at the same time.

[guggero]

Thanks for the review @guggero - would you also be able to comment on my questions above about a new test case and the psbt/fundmax behavior? --> #6903 (comment)

The test case looks good, nice idea to grab the commitment types from the RPC package!

[hieblmi]

@guggero I think I addressed all the issues you flagged in your last review and tried to stick to the 80 char line limit in a bunch of other places.

Some more remarks:

1. During testing I realized that using the minchansize from our config here which is propagated to the wallet_assembler here puts a minimum size on our outgoing channel where as per definition this config parameter is supposed to only affect incoming channel sizes. Say I only allowed peers to open 5M sat channels to me I wouldn't be able to use --fundmax unless my wallet balance was >= 5M sats. Instead I think we should use MinChanDefaultSize here which is 20k sats.

2. Regarding your https://github.com/lightningnetwork/lnd/pull/6903#discussion_r984681440 I will have a look if we can easily find out if we already have public channels with the peer we want to open to, and if so also provision reserves for private channels to that peer. Any other suggestions? EDIT: One way this might work is checking in the funding manager in handleInitFundingMsg if a public channel shows up for our peer in listchannels and if we find one and if we fund a private fundmax channel, then we can set a flag here https://github.com/lightningnetwork/lnd/blob/6ab6ba91bbe53775c93b962b6b9fa67834c01ae2/funding/manager.go#L4006 to figure out if we need to consider a reserve here https://github.com/lightningnetwork/lnd/pull/6903/files#diff-3ebebe355b386e69fdd9742a8d6ed60cb3b2377a9c10a3d3b831650e47c7d258R784. Comments on this approach are highly appreciated.

3. If it requires a big additional diff, then I'd say for now just add an error that the FundMax and Psbt cannot be used at the same time.

I think this functionality could be taken care of in the psbt part of this ticket: https://github.com/lightningnetwork/lnd/issues/6949. For now I have added a preventative check here https://github.com/lightningnetwork/lnd/commit/7936ef83105db15fac66ab5800d3d28189c5d3d0#diff-81f87a9144ebe01ee1916dad1a5d76287e7c76ced4fd2ae553a8e9ea5a17c190R364.

[lightninglabs-deploy]

@guggero: review reminder @hieblmi, remember to re-request review from reviewers when ready

[guggero]

@hieblmi sorry for the late answer. Here's my input to your three questions:

1. Ah yes, good catch. I agree that we should choose MinChanDefaultSize instead of the configurable one as that is indeed only for inbound channels.
2. The logic for reserving balance is independent of peers. If you look at CurrentNumAnchorChans() in lnwallet/wallet.go you see that we count all announced anchor channels, including pending closed. So the logic in your PR should be: Get the current number of public anchor channels (using mentioned method preferably) and add 1 to it if the channel being opened is public and an anchor channel. Otherwise don't increase the number. Then use that number to calculate the required reserve.
3. That sounds good! Though we should also add that check in the RPC server itself for any RPC/REST users. I think you can just require the funding shim to be nil if fund max is set and vice versa.

[joostjager]

Given the size of the PR, it may be useful to explain the motivation in a bit more detail.

The PR description explains that there is no fundmax flag (like there is for SendCoins) and that it is cumbersome to fund the max amount manually, but why is this an important feature to have?

A comment on how this intersects with anchor channels and the need to always keep some balance/utxos available for fee bumping could be helpful too.

[hieblmi]

Hi @joostjager, yes I agree - I will add context about the motivation and details of this PR in the description in a bit.

[hieblmi]

Re your points @guggero

@hieblmi sorry for the late answer. Here's my input to your three questions:

1. Ah yes, good catch. I agree that we should choose MinChanDefaultSize instead of the configurable one as that is indeed only for inbound channels.

The correct field is MinChanFundingSize which I added.

2. The logic for reserving balance is independent of peers. If you look at CurrentNumAnchorChans() in lnwallet/wallet.go you see that we count all announced anchor channels, including pending closed. So the logic in your PR should be: Get the current number of public anchor channels (using mentioned method preferably) and add 1 to it if the channel being opened is public and an anchor channel. Otherwise don't increase the number. Then use that number to calculate the required reserve.

As you suggested we now consider CurrentNumAnchorChans() + 1 anchor reserve outputs in the fundmax flow if the channel being funded is a public channel. Otherwise we reserve for CurrentNumAnchorChans() anchor outputs. See here. CC @hsjoberg

3. That sounds good! Though we should also add that check in the RPC server itself for any RPC/REST users. I think you can just require the funding shim to be nil if fund max is set and vice versa.

Added a check here.

I also wonder if we can refactor the in.FundingShim check from here into parseOpenChannelRequest like the other fields of the funding request.

I've also fixed some line length limits which makes the PR a bit harder to read. Hope that's ok.

[hieblmi]

@guggero, I think I addressed all your nits.

Additionally I added a test case to check for skipping the anchor reserve in case we start the --fundmax flow for a private anchor channel (was added here). In that case we don't expect the wallet balance to decrease by the anchor reserve and also don't expect fees for a change output(fundingFee(1, false)).

I also moved some of the itest checks to assertions.go.

[hieblmi]

Given the size of the PR, it may be useful to explain the motivation in a bit more detail.

The PR description explains that there is no fundmax flag (like there is for SendCoins) and that it is cumbersome to fund the max amount manually, but why is this an important feature to have?

A comment on how this intersects with anchor channels and the need to always keep some balance/utxos available for fee bumping could be helpful too.

@joostjager I've updated the PR description to give more context on this new feature.

[joostjager]

Thanks for extending the description. As to why users want this, I suppose it's to put the maximum capital to work in the opened channel and not leave any money doing nothing in the wallet?

[hieblmi]

Thanks for extending the description. As to why users want this, I suppose it's to put the maximum capital to work in the opened channel and not leave any money doing nothing in the wallet?

That for sure is a good reason, at least when I myself manually select all funds. I added it to the description.

[hieblmi]

Did an initial pass just to start loading the context for this diff. Great work @hieblmi! For now I have just left some questions just to help with my understanding 👍 (also, quick one: saw you removed my request for review before - pls dont do this as then I would normally forget (all PRs need at least 2 reviewers))

Thanks for your review @ellemouton! I will go over it in some time.

Apologies for removing you as a reviewer, I wasn't aware of it and don't know how that happened.

[hieblmi]

😬 now requested review from @ellemouton but @guggero's review request disappeard 😅

[hieblmi]

One more note, I realized that in my last commits I forgot a check from the original PR in cmd_open_channel.go which I added. It's here: https://github.com/lightningnetwork/lnd/pull/6903/commits/efe3d95782639899af172df7c5378e7afd824519#diff-81f87a9144ebe01ee1916dad1a5d76287e7c76ced4fd2ae553a8e9ea5a17c190R363

[hieblmi]

First pass done, I think this is almost there - great work. Just think the style of the PR needs some work. I think the style changes should either be in separate commits or ideally in a different PR. It makes it harder to review since it makes a larger diff. Also I think some of the commits can be split up so that they only modify one package at a time.

Hi @Crypt-iQ, thanks for taking the time to review. I think I addressed all of your and @ellemouton's points. I've removed some of my code formatting and plan to submit a separate PR for it. I've also split one commit. Please let me know which other commits you'd like to see split up.

EDIT: Should we merge the coin selection without and with reserve consideration? The commits are https://github.com/lightningnetwork/lnd/pull/6903/commits/13569967f304bbd1d69c9e369ad60cdd8540f7c1 and https://github.com/lightningnetwork/lnd/pull/6903/commits/35225dbab643450dd0a5c32695d14f7ce4ce7ae4. In the predecessor of this PR it was required to keep them separate but this PR might be more readable if they'd be merged?

A bit unrelated to the changeset: I'm not sure if adding this behavior is ideal. Let's say a user spends all of their funds (minus anchor fee-bumping reserves) into a channel. If the user then has to force close a different channel(s) and uses all of their fee-bumping reserve, it's possible that they can no longer resolve the HTLC-timeout/HTLC-success clauses on their commitment transactions. If this leads to them failing to act, they could lose money. There doesn't seem to be an easy way of calculating what a good wallet balance is, which makes the problem harder to solve.

Good point. I think the problem you describe already exists today for users that spend more or less the entirety of their remaining wallet balance towards a channel opening to potentially maximize fee revenue(see https://github.com/lightningnetwork/lnd/issues/619). I too think that it's tricky to calculate an ideal wallet reserve to provide for the case where you have to get your HTLC timeout tx into a block. This could be a configurable field for users with different risk appetite or a fixed overshooting reserve(30k sats) per channel could be kept in wallet?

An easier approach might be to simply warn the user of the consequences when using --fundmax by prompting them with yes/no before the funding flow is executed.

[ziggie1984]

Thanks for all the work you put in this PR @hieblmi @bjarnemagnussen .

After reviewing this PR it feels to me that somehow the approach is too complicated by design, I am a fairly new contributor but it took me very long to understand all the changes done in this PR and especially all the edge-cases this PR bring with it.

Not sure but feels like maybe approach this thing from a complete new perspective. This part of the code will be for sure very hard to maintain for a new contributor.

Another solution would be to increase the verbosity (descriptions for example) of the testcases, especially make it very clear which error should occur when, so approaching these changes from the testcases are more clear.

Again thanks for this hard work :), this change is by far not trivial (as it would seem at first glance)

[Crypt-iQ]

@ziggie1984 in the future I think you should generate one review rather than multiple reviews since github will make multiple reviews take up more space with the headings and whatnot - keep up the review work though 👍

[hieblmi]

Thanks yalls for reviews, good catches @ziggie1984, really appreciate it. I will be tied up til next week until I can return to work on this PR. But glad to hear from @Crypt-iQ that we are getting close :-).