adds fundmax flag to openchannel#4029
adds fundmax flag to openchannel#4029bjarnemagnussen wants to merge 8 commits intolightningnetwork:masterfrom
fundmax flag to openchannel#4029Conversation
|
The wallet/fundingmanager already has a Line 69 in 3aca9d2 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 But I did use the I saw there may also be an open TODO in that regard for the excess fee. Furthermore, |
|
@bjarnemagnussen I'm not sure I understand the limitation of the current API. If you do a regular channel opening with the |
|
@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 functionsThere is this little mismatch between The Inside 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 casesI 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 |
halseth
left a comment
There was a problem hiding this comment.
This looks pretty good! Could an alternative way to communicate our intention to use all our funds to the wallet be a fundUpTo bool? That way we would reuse the localAmt field for our maximum channels size, but communicate with the boolean that we allow creating a smaller channel if the funds are not available.
We could also take this a step further and create a chanSizeType enum that would have values SubtractFees, FundUpTo and ExactChanSize.
bjarnemagnussen
left a comment
There was a problem hiding this comment.
Thanks @halseth for your review and the helpful inputs!
Could an alternative way to communicate our intention to use all our funds to the wallet be a
fundUpTobool? That way we would reuse thelocalAmtfield for our maximum channels size, but communicate with the boolean that we allow creating a smaller channel if the funds are not available.
I have been considering this. But there were a few things I fell over:
- There is the minimum channel funding limit
minChanFundingSizedefined inlndthat we always want to exceed. And we may therefore want to be able to define a minimum value at the channel funding whenfundAllis set. - When called from the RPC it must also be assured that the funding amount is at least the amount we will afterwards try to push to the remote (see comment with the code).
- With a minimum channel funding limit set inside
localAmtwe keep the behaviour that the channel opening will fail with funds less than what is set with thelocalAmt.
I am therefore not sure if we can work around this in another way?
We could also take this a step further and create a
chanSizeTypeenum that would have valuesSubtractFees,FundUpToandExactChanSize.
I like your idea of defining a chanSizeType enum for the different cases! But how it will look probably depends on if the minimum amount is needed or not from the discussion above.
|
I am still not sure how to handle both the |
|
Could you rebase the PR please? I'm going to take over the review for @halseth.
Didn't read through all comments yet, but couldn't we just check the |
57b80a3 to
f7b419f
Compare
|
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
Remark: In the current implementation an edge case can raise a bug resulting in a channel capacity greater than |
f7b419f to
bed6742
Compare
guggero
left a comment
There was a problem hiding this comment.
I looked at the PR now in more detail. I do see how it would be useful to just create a maximum channel, no matter how much is in the wallet. But perhaps it would then be better to call the flag --fundmax?
What does make this PR a bit more complicated is the handling of the push amount (initial remote balance). Maybe we should just disallow the combination of flags/parameters? Or work with a percent margin.
I'm going to test this a bit more tomorrow, so far I've only looked at the code.
bed6742 to
d14e12f
Compare
|
I commented above before committing, so sorry for that! I have renamed |
|
No worries. I'll have a look at it today and test it more thoroughly. |
|
Let me just one more time mention #4029 (comment) 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 ( If needed, I could take a look at the TODO inside |
810d331 to
6d3de89
Compare
|
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? |
|
@guggero waiting eagerly for this feature. |
|
Okay cool. Feel free to test and leave a review to make sure this works as expected for your use case(s). Will try to pick it up this week too. |
guggero
left a comment
There was a problem hiding this comment.
We're getting pretty close, I think. The added complexity with the reserve balance does make everything a bit harder. I hope you understand why this PR undergoes so much scrutiny since it touches the core funding logic, where (as you've probably noticed) a lot of things can have an influence.
The integration tests look really good now, though! I don't think we need to also add a test with the new commitment type, since fee wise they look just like anchor channels.
| } | ||
|
|
||
| // Inputs should be all funds in the wallet. | ||
| if len(fundingTx.TxIn) != len(test.coins) { |
There was a problem hiding this comment.
Should we add a test case with multiple inputs so this check actually does make sense?
There was a problem hiding this comment.
| runTestCase := func(carol, dave *lntest.HarnessNode, | ||
| testCase *chanFundMaxTestCase) func(t *testing.T) { | ||
|
|
||
| return func(t *testing.T) { |
There was a problem hiding this comment.
To avoid this nesting and the code within reaching over the 80 character limit, we shouldn't return a function here, but instead just define a function that takes t, carol, dave, testCase and then call it in the test case loop below.
There was a problem hiding this comment.
| amount: 37000, | ||
| // The transaction fee to open the channel must be subtracted from | ||
| // Carol's balance (since wallet balance < max-chan-size). | ||
| carolBalance: btcutil.Amount(37000) - fundingFee(1, false), |
There was a problem hiding this comment.
We should add a test with a low balance and the anchor reserve, just to make sure the interplay here is correct.
There was a problem hiding this comment.
Was added by @bjarnemagnussen, to be reviewed here: f851139#diff-583407cd98e4e423efd8a6afe4acb5b67395da6ae79927b11f9e3b989f822313R142
| Usage: "(optional) if set, the wallet will attempt to " + | ||
| "commit the maximum possible local amount to the channel. " + | ||
| "Should not be set together with local_amt", | ||
| "commit the maximum possible local amount to " + |
There was a problem hiding this comment.
This whole commit should be squashed with the respective commits where the initial changes were introduced.
| if remoteInitialBalance >= funding.MinChanFundingSize { | ||
| // As default value for the minimum local amount use the larger | ||
| // of the amount to be pushed to the remote or the allowed | ||
| // minimum channel size. |
There was a problem hiding this comment.
To be honest, these comments don't really help me to understand why something is done. They just describe the code below in words. We should add the context from the discussions in this PR instead. Otherwise someone looking at this in a year or so won't know what was discussed.
There was a problem hiding this comment.
Tried to make it clearer here: 58bc9bd#diff-674c79ba02b6e9c45a994388392689814f3538800351b2fd43fb3ade21e1effbR1916
| selectedCoins, changeAmt, err := CoinSelect( | ||
| feeRate, maxAmt, dustLimit, coins, | ||
| ) | ||
| if err == nil { |
There was a problem hiding this comment.
This whole method is very hard to grasp without staring at it for a long time. Since this selects coins, it makes me very nervous.
So here two suggestions to streamline the code in this area.
- Use one
switchstatement:
switch {
case err == nil:
case err.(type) == *ErrInsufficientFunds:
default:
}
- Instead of setting the err to
errReservedValueInvalidated, do the actual call toCoinSelectSubtractFeesdirectly.
There was a problem hiding this comment.
| } | ||
|
|
||
| if commitTypeHasAnchors(testCase.commitmentType) { | ||
| err = checkWalletBalance(carol, lnwallet.ReservedValue(1)) |
|
|
||
| localFundingAmt := req.LocalFundingAmt | ||
| remoteFundingAmt := req.RemoteFundingAmt | ||
| hasAnchors := req.CommitType.HasAnchors() |
There was a problem hiding this comment.
These changes to the wallet probably deserve their own commit.
| } | ||
|
|
||
| // Note: | ||
| // The fundmax flag is NOT allowed to be combiend with local_amt above. |
There was a problem hiding this comment.
typo (combiend -> combined)
There was a problem hiding this comment.
|
I was wondering why the author commit dates are not in a chronological order ? (does it come from rebasing single commits ?) |
| updateChan := make(chan *lnrpc.OpenStatusUpdate) | ||
|
|
||
| // Initiate a fund channel, and inspect the funding tx. | ||
| pushAmt := btcutil.Amount(0) |
There was a problem hiding this comment.
should we also test a pushamt greater than the MinChanFundingSize?
There was a problem hiding this comment.
|
will this PR also allow for --fundmax with wumbo channels, meaning that the UpperLimit is adjusted when wumbo channels are active ? (As far as I understood the code, its capped at the 2^24-1 limit) |
Justed tested it with wumbo channels works nice 👍 |
| "fmt" | ||
| "testing" | ||
|
|
||
| "github.com/btcsuite/btcutil" |
There was a problem hiding this comment.
Erroneous import:
| "github.com/btcsuite/btcutil" | |
| "github.com/btcsuite/btcd/btcutil" |
Also I'm no go expert but other test files contain //go:build xxx in the top:
+//go:build !rpctest
// +build !rpctestThere was a problem hiding this comment.
Fixed in the continuation of this PR, here: #6903
| RemoteAmt: req.RemoteFundingAmt, | ||
| LocalAmt: req.LocalFundingAmt, | ||
| FundUpToMaxAmt: req.FundUpToMaxAmt, | ||
| ReservedAmt: ReservedValue(numAnchorChans), |
There was a problem hiding this comment.
This will lead to inconsistent behavior.
For private channels (i.e mobile wallets), we do not right now reserve any sats for anchor outputs.
If you specify the amount when opening a private channel, you'll be able to use all funds (no sats will be reserved).
Related commit: 8e1087d
Also related topic: #6574
Possible fix:
+isPublic := req.Flags&lnwire.FFAnnounceChannel != 0
[...]
-if req.FundUpToMaxAmt > 0 {
+if req.FundUpToMaxAmt > 0 && isPublic {
numAnchorChans, err = l.currentNumAnchorChans()
if err != nil {
req.err <- err
req.resp <- nil
return
}
if hasAnchors {
numAnchorChans++
}
}There was a problem hiding this comment.
Addressed and described here with comments from the related commit 8e1087d: 60ddaa7#diff-3ebebe355b386e69fdd9742a8d6ed60cb3b2377a9c10a3d3b831650e47c7d258R779
|
@bjarnemagnussen hey this is on the milestone for our next release, do you have time to resume work on it so it can finally land? |
|
@hieblmi keen on making progress on this PR, so it would be great if you can pick it up and run with it. We are hoping this issue can make it in release 16. Happy to work with you to make progress on this. You may want to reach out to @bjarnemagnussen if he can provide any assistance on this. |
| } | ||
|
|
||
| // errReservedValueInvalidated is an error used internally for the coin select | ||
| // algorithm to determine that |
I've added a test in this commit: 1301bde |
|
@bjarnemagnussen, remember to re-request review from reviewers when ready |
|
It looks like all comments were addressed in the replacement PR #6903, so I'm going to close this one. |
Related to PR #4024, but reopened as a new PR as this feature has undergone major changes.
Description
This PR introduces a new flag
fundallforopenchannel, 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
sendcoinshas a flagsweepallthat will spend all the coins from the wallet. However no such flag exists when opening new channels usingopenchannel.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
CoinSelectUpToAmounthas 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
Contribution Guidelines
the positive and negative (error paths) conditions (if applicable)
go fmt(the tab character should be counted as 8 characters, not 4, as some IDEs do
per default)
make checkdoes not fail any testsgo vetdoes not report any issuesmake lintdoes not report any new issues that did notalready exist
cases it can be justifiable to violate this condition. In that case, the
reason should be stated in the commit message.