funding: add fundmax flag to OpenChannelRequest#6903
funding: add fundmax flag to OpenChannelRequest#6903Roasbeef merged 10 commits intolightningnetwork:masterfrom
fundmax flag to OpenChannelRequest#6903Conversation
|
This PR so far has only taken #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. |
hieblmi
force-pushed
the
fundmax-flag
branch
from
ed8e8f2 to
256ac3e
Compare
|
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. |
|
Thanks @guggero, yes good point. I will attribute the commits to Bjarne. |
hieblmi
force-pushed
the
fundmax-flag
branch
4 times, most recently
from
3d67cf2 to
dff8e88
Compare
|
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 -> #4029 (review), try and make |
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. |
hieblmi
force-pushed
the
fundmax-flag
branch
7 times, most recently
from
b9d8b32 to
96ab85a
Compare
|
@guggero If the open channel is being invoked with a psbt(e.g. for a specific UTXO) and @hieblmi have you covered this scenario in your testing? From the description on the PR it appears that the scope of |
|
@saubyk I don't see psbt scenarios covered in the parent PR. I am currently incorporating requested changes to the It makes sense to use the |
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
force-pushed
the
fundmax-flag
branch
4 times, most recently
from
6c4930c to
60ddaa7
Compare
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Looks really good now, just had some additonal minor comments, if they are addressed I am good :)
lnwallet/chanfunding/assembler.go
Outdated
There was a problem hiding this comment.
nit: Not sure why we need this Reserve Check here, isn't it done upstream in the VerifyConstraints function ?
There was a problem hiding this comment.
We need it because we cannot do the following check in the 'parseChannelOpenReq' but have to wait till the coin selection has succeeded: https://github.com/lightningnetwork/lnd/pull/6903/files#diff-674c79ba02b6e9c45a994388392689814f3538800351b2fd43fb3ade21e1effbR2013
The additional check is here: https://github.com/lightningnetwork/lnd/pull/6903/files#diff-ba25a5144f3090a192b11b77b5cbd5b63d1d33cfe61dd1e29cb15dabb27261e9R315
There was a problem hiding this comment.
Hmm ok, so yeah the check in the lncli is good, but because we do the check for fundmax in the backend, I think doing it in the VerifyContraints is enough or ? But decide by yourself, one additonal check does not harm anyone
There was a problem hiding this comment.
Oh now I see it. Yes, VerifyConstraints tests exactly this. I will remove RemoteChanReserve and the redundant check I added in the wallet.
There was a problem hiding this comment.
maybe outputt the expected error here ?
There was a problem hiding this comment.
nit: If more than 20% of all outputs go to fees (funds in general is a bit vague could be sum of all inputs?)
There was a problem hiding this comment.
I will call it 'available wallet funds'. Sould be clear in the fundmax context imo
There was a problem hiding this comment.
Q: Is it common in itests to replicate the functions in the codebase rather then use them here?
There was a problem hiding this comment.
I doubt that this is a common pattern.
There was a problem hiding this comment.
is it better to replicate the code rather than using the feecalculation form the actual code? Asking out of curiosity how to do this testing, my suggestion would come with the risk of testing way too "whitetesty" but depending on why you wrote this function you basically looked how the real source is doing it an replicated it so not sure whats the best way to test in such circumstances ?
There was a problem hiding this comment.
Edit: @ziggie1984 Yes good point. I think initially this function was added for brevity and to work around the private scope of calculateFees in coin_select.go.
We'd have to export the private function to be able to test it. Or adjust the test fee function in case the calculation in the implementation changes, as was done during this PR when the default P2TR change outputs were introduced.
I am curious to hear more thoughts on this.
There was a problem hiding this comment.
nit: see my comment above I don't think we need this check here its already done in a higher level
funding/manager_test.go
Outdated
There was a problem hiding this comment.
I think its a good idea to have such a check, or we include a reserve for all types except the one we no don't need one ?
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
LGTM, thanks for tackling this one !
There was a problem hiding this comment.
is it better to replicate the code rather than using the feecalculation form the actual code? Asking out of curiosity how to do this testing, my suggestion would come with the risk of testing way too "whitetesty" but depending on why you wrote this function you basically looked how the real source is doing it an replicated it so not sure whats the best way to test in such circumstances ?
There was a problem hiding this comment.
Incredible work on this PR 🔥 The test coverage and PR description also deserve a special shout out 🚀
I think this is good to go. All my comments are non-blocking.
The reason the itests are failing is actually cause the itest that runs right before the one you added does not clean up its channel properly. The following diff will fix it:
diff --git a/itest/lnd_htlc_test.go b/itest/lnd_htlc_test.go
index 4e73a385b..fc04fe8d0 100644
--- a/itest/lnd_htlc_test.go
+++ b/itest/lnd_htlc_test.go
@@ -24,6 +24,7 @@ func testLookupHtlcResolution(ht *lntest.HarnessTest) {
cp := ht.OpenChannel(
alice, carol, lntest.OpenChannelParams{Amt: chanAmt},
)
+ defer ht.CloseChannel(alice, cp)
// Channel should be ready for payments.
const payAmt = 100
Will approve this once the above diff is in ✅
There was a problem hiding this comment.
doesnt seem like they were reverted? also, no need to revert them, can just put them in a separate commit :)
|
Once #7511 is in, then the diff I posted will no longer be necessary :) |
Thank you @ellemouton, I've submitted my changes but it still looks like an |
|
@hieblmi we started a new parallel development branch |
|
Thx for the instructions @guggero, I've just rebased with this dev branch. Would be awesome if we get it into v0.16.0. |
Since 0.16.0 is already in RC3 (which might or might not be the last one), that ship has sailed, unfortunately. But this looks to be very much on track for 0.16.1 (hence the dev branch). |
|
Got it, I misread/misunderstood the branching concept here. It makes sense and thx for clarifying. |
ellemouton
left a comment
ellemouton
left a comment
There was a problem hiding this comment.
A very well deserved LGTM 🔥 Excellent work on this - your test coverage is fantastic!!
Just needs a rebase onto the 0-16-1-staging branch
Crypt-iQ
left a comment
Crypt-iQ
left a comment
There was a problem hiding this comment.
The reserve still needs to be checked remoteChanReserve >= localFundingAmt/5
|
@hieblmi, remember to re-request review from reviewers when ready |
Allows to define a maximum amount to provision a channel opening with using a new field `FundUpToMaxAmt` on the `Request` struct. Also adds a new coin select function `CoinSelectUpToAmount` to select coins up to a maximum amount respecting a minimum amount.
Adds handling of the `FundMax` field when parsing an `OpenChannelRequest` with `rpcServer.parseOpenChannelReq`.
12 participants
CC @bjarnemagnussen, @saubyk, @positiveblue: This PR continues the work of PR #4029. I am pasting the previous PR description below to make adjustments.
The original PR is related to #4024.
Description
This PR introduces a new flag
fundmaxforOpenChannelRequest, 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:
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
SendCoinsRPC offers asend_allflag that attempts to send all the coins under control of the internal wallet to a specified address. Similarly,OpenChannelshould implement functionality to use all available wallet funds towards a channel opening.Implementation
A new
FundMaxflag is added to theOpenChannelRPC,--fundmaxtolncli openchannelrespectively. As of now this flag cannot be used in combination withpsbt.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
CoinSelectUpToAmounthas been introduced. It selects coins up to the requested amount, or below if there are not sufficient funds available. More specifically the functionfundmaxamount is available exclusive of funding transaction fee and optional anchor channel reserve.fundmaxrequest.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.
fundmaxbehavior for anchor channels and future channel typesSince 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
fundmaxflow a new sanity test has been added to the funding manager(seeTestCommitmentTypeFundmaxSanityCheck). 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.