add new max channel size config option

[calvinrzachman]

• let users specify their MAXIMUM WUMBO with new config option which sets the maximum channel size lnd will accept
• current implementation is a simple check by the fundingManager rather than anything to do with the ChannelAcceptor
• Add test cases which verify that maximum channel limit is respected for wumbo/non-wumbo channels
• use --maxchansize 0 value to distinguish set/unset config. If user sets max value to 0 it will not do anything as 0 is currently used to indicate to the funding manager that the limit should not be enforced. This seems justifiable since --maxchansize=0 doesn't seem to make sense at first glance.
• add integration test case to ensure that config parsing and validation is proper. I simplified the funding managers check electing to rely on config.go to correctly parse and set up either i) non wumbo default limit of 0.16 BTC OR ii) wumbo default soft limit of 10 BTC OR iii) a user specified max channel size

Addresses: #4557

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)
• For bug fixes: Code is accompanied by new tests which trigger
  the bug being fixed to prevent regressions
• Any new logging statements use an appropriate subsystem and
  logging level
• Code has been formatted with go fmt
• Protobuf files (lnrpc/**/*.proto) have been formatted with
  make rpc-format and compiled with make rpc
• 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]

Straightforward diff, thanks for the contribution! Only request is to add either a unit test at the funding manager, or extend an existing itest to exercise the code path.

[Roasbeef]

👍

[Roasbeef]

NP on not using a channel acceptor, this is much simpler after all, in the future we may opt to roll it out into one, just so we can have all the logic coupled in a single place.

Should this configurable maximum be above the old soft-limit or should we permit any value? If above soft limit only, ensure that this setting does not override wumbo channel enablement (ie: don't impose a cap that is lower than the old limit)

What do you mean above the old soft limit? You mean like ensuring if wumbo isn't activated, then we don't allow this value to be set?

One other thing I think we should consider is adding a new default soft-limit if wumbo is activated. Just to make sure users are protected somewhat, and need to explicitly allow mega-wumbo channels.

[calvinrzachman]

Hi @Roasbeef, thanks for the review. I was hoping to get the test case and validation added in before you made a first pass, but alas. I’ve added those in and made a couple changes.

Apologies for any confusion, here is where things are currently:

• ANY explicit user setting of --maxchansize is respected (both for wumbo and non-wumbo).
• For non-wumbo configurations this limit remains 16777215 satoshis by default as specified in BOLT-0002. NOTE: This is already enforced by this existing code.
• For wumbo funding managers this limit is NOT enforced by default (currently indicated by zero value). We could modify this to impose a new new default soft-limit in the wumbo case as you mentioned.

Maybe there is a better way to do this than using a zero value. This breaks down should the user set --maxchansize=0 but perhaps it's okay since that is most certainly not a valid setting anyway. An alternative way to distinguish between the zero value and an unset value is with pointers.

One more thing: Right now I only enforce --maxchansize inside handleFundingOpen() which I believe is in response to an OpenChannel message from a peer. It makes sense to me to enforce this on channels created by user inside handleInitFundingMsg(). Let me know what you think and whether I took this in the wrong direction.

[halseth]

Thanks for the PR, awesome change 👍

I agree that perhaps a soft-limit by default is preferred. If you opt into wumbo, then you should be able to open channels of any size you like, but IMO it should be a second opt-in to start accepting huuuge channels.

[calvinrzachman]

At first a new larger default seemed to defeat the nature of WUMBO channels, but given that people will be entrusting their wealth to lnd, that's starting to sound like a better and better idea.

Any thoughts on what the new default maximum channel size should be?

[Roasbeef]

Any thoughts on what the new default maximum channel size should be?

10 BTC?

[Roasbeef]

If you opt into wumbo, then you should be able to open channels of any size you like, but IMO it should be a second opt-in to start accepting huuuge channels.

Yeh there's a distinction here:

• pre wumbo .16 is the limit
• post wumbo you can open any size you want
• post wumbo there's a new default soft limit for accepting
• post wumbo, users can modify the default acceptance max

[Roasbeef]

This PR is super close! IMO we want to have a default soft-limit for for wumbo chans in place. In the past, we threw around a value of 10 BTC which seemed reasonable at the time. This would only be for accepting the channels as mentioned.

We'd like to tag our v0.11.1 version in a few days here, and I think this is really close, so I'd like to get it in.

[calvinrzachman]

Latest update contains enforcement for 10 BTC default wumbo limit. I elected to setup defaults in ValidateConfig() inside config.go and keep the simplest check I could think of in the Funding Manager. Since the funding manager now trusts only its config in determining the MaxChanSize, I added a new itest to verify that config.go handles explicit user setting of --maxchansize and both wumbo/non-wumbo defaults.

I agree this is close. One thing I am unsure on right now is whether my setting of the default limits will screw up the case where lnd is run with Litecoin. I will think on this a bit more and do my best to finish this PR off this weekend.

[Roasbeef]

One thing I am unsure on right now is whether my setting of the default limits will screw up the case where lnd is run with Litecoin.

The default can always be overridden in that case. FWIW, the 0.16 number itself was very Bitcoin specific.

[Roasbeef]

The latest set of changes is looking pretty good to me! Final thing is to either squashed down these commits into one, or update the title+body for each of the commits. As the change is relatively contained, I think maybe we can just squash them into one. Thanks again for working on this!

[Roasbeef]

LGTM 🧬

Changed looking good to me, final nit is just to squash into a single commit. Will test this out a bit locally and also wait on the other reviewer before merging.

[calvinrzachman]

@Roasbeef Squashed ✅ Thanks for the guidance on the way through. It was fun to start learning the 2nd most important codebase in the world. I'll be on the lookout for another issue/PR soon.

[cfromknecht]

Thanks @calvinrzachman!! Very clean PR, LGTM 👌