-
Notifications
You must be signed in to change notification settings - Fork 2.2k
funding: fund channel with selected utxos #7516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
OpenChannel RPC0d9ac8b to
625234d
Compare
Roasbeef
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Super useful feature to make it easier to do coin control/selection funding on the CLI.
Did an initial pass with a few comments that jumped to mind.
ziggie1984
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid contribution 🤓, good structure, very nice itests.
Keep up the good work :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: does this error msg account for the reserve it the wallet tho, doesn't look like it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it doesn't, in this test case the user chooses too much local_amt for what they have in their wallet. And it's not an anchor but a static remote key channel, so no reserve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we decrease the reserve requirement by the reserve = reserve - (sumAll-sumManuel) amount here to let non selected coins also account for the reserve ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah right, but only if sumAll-sumManual < reserve. I thought about something like:
excess := sumAll - sumManual
if excess >= reserve {
reserve = 0
} else {
reserve -= excess
}
|
Appreciate the review @ziggie1984, I will get to your comments in a bit. |
shaurya947
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice feature! I was wondering if reordering the commits would make reviewing easier:
cli -> RPC(protobuf) -> rpcserver.go -> chanfunding -> lnwallet -> itest
b03a52e to
50b2658
Compare
8dbdd41 to
b18cd83
Compare
ziggie1984
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost ready had a final question regarding concurrent safety when manual utxo selection and allCoins selection happens in different places.
Some other remarks also in regards of the discussion during the review club:
- We went by the idea, if we selected more than what is needed we just neglect the other inputs? No consolidation option?
- Is the coin-selection strategy now part of this PR or is this still not influenced?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we be sure that sumAll and sumManuel don't run in any race condition so that this always stays positive ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that the selection happens in one place it is guaranteed that we do not run into a race condition so this is fine here 👍
|
@ziggie1984 thanks for your review. I placed the coin availability checks within the coin lock so to avoid a race here. As to the coin selection and consolidation strategy discussed in the review club - this seems like an optimization that I want to address in a separate PR, once the basic functionality here is merged. |
ziggie1984
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Had some minor nits but this looks very good, tested it locally and works perfect ! This feature is really cool.
rpcserver.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is not what the below code does. It does not really check if outpoints are present imo? Just converts these to types ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
lnwallet/chanfunding/assembler.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Commit Msg: adds ability to fund a channel with a selected set of coins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We could still use := in the line and it would not redeclare the manuallySelectedCoins variable, or is this the style we have to adhere in such a case ? If so maybe an entry in the code_formatting_rules.md file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that the selection happens in one place it is guaranteed that we do not run into a race condition so this is fine here 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate more, do you mean adding new external inputs as well when the internal ones do not suffice?
Roasbeef
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🍾
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could also assert that the coins are locked as well at this stage. Though, if things proceed as expected here (final transaction actually broadcast), then this'll happen eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mismatch in godoc name: runTestCase -> runUtxoSelectionTestCase
rpcserver.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
This PR implements feature #6955.
Background
It extends the ways in which the user can instruct the
OpenChannelRPC to allocate funds when opening a channel. Specifically, it adds the ability to select individual utxos from the user's wallet as the basis for funding. Besides funding viapsbts, there are currently two ways in which the user can define the amount a channel is funded with,local_amtandfundmax. The former selects coins until the amount is reached and creates the funding output and a change output with the excess. The later uses up all available funds towards a channel opening. Both source from lnd's internal wallet.This PR allows both these funding options to instead source from a set of defined utxos.
Examples
To see how this works lets consider two examples:
lncli openchannel --utxo=A --utxo=B --local_amt 100000Instead of considering the entire available wallet balance only two utxos from our wallet are taken into account for funding a 100k cap channel.
lncli openchannel --utxo=A --utxo=B --fundmaxInstead of spending the entire wallet balance on a channel opening
fundmaxonly consumes the specified utxos for it.Wallet reserve considerations
In both examples we have to ensure that our wallet keeps enough funds to cover for the anchor reserve requirement.
For example, in the
fundmaxcase, if the remaining wallet balance is less than the anchor reserve requirement, we have to carve out the remaining sats from the specified utxo set. Similarly this has to be handled for thelocal_amtcase.