-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Simplify coin selection for sendcoins #8516
Conversation
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Hello @hieblmi here is the PR |
Modified it, no need for this |
Hello @sputn1ck, do you think I should create a different function instead of adding an arg to an exported one as per your comment here: btcsuite/btcwallet#912 (comment) |
tACK. Tested locally and worked as expected with one utxo, sweep, and multiple utxos. Would like to see this merged as I am signing PSBTs manually because my application requires 1 input --> 1 output transactions. |
Is there an update on this @Chinwendu20 ? |
Thanks I should push an update by the end of this week.. |
c800df5
to
3e6ab6a
Compare
b6bede7
to
3677d4b
Compare
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.
tACK, apart from the failing itest cases which seem to be unrelated to this PR I have nits left.
Great work again on this @Chinwendu20.
lnwallet/btcwallet/btcwallet.go
Outdated
@@ -974,12 +974,13 @@ func (b *BtcWallet) ImportTaprootScript(scope waddrmgr.KeyScope, | |||
// the specified outputs. In the case the wallet has insufficient funds, or the | |||
// outputs are non-standard, a non-nil error will be returned. | |||
// | |||
// NOTE: This method requires the global coin selection lock to be held. | |||
// NOTE: This method requires the global coin selection lock to be held.n |
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: remove n
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 is not addressed
cmd/lncli/commands.go
Outdated
cli.StringSliceFlag{ | ||
Name: "utxo", | ||
Usage: "a utxo specified as outpoint(tx:idx) which " + | ||
"will be used as input for the transaction; " + |
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: period instead of ;
@@ -384,6 +384,9 @@ | |||
required](https://github.com/lightningnetwork/lnd/pull/8681) to be consistent | |||
with the flow when a payment request isn't used. | |||
|
|||
* [Added `outpoints` to `SendCoinsRequest`]( |
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.
move to release notes for v0.18.1-beta
.
Signed-off-by: Ononiwu Maureen <[email protected]>
23df67f
to
7065b64
Compare
@guggero we have two approvals now, I think we can merge? |
Wanted to make sure that @yyforyongyu's CRs were addressed, would be good to get his go too. |
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's a missing check that we need to ensure provided utxos are known to the wallet.
lnwallet/btcwallet/btcwallet.go
Outdated
@@ -974,12 +974,13 @@ func (b *BtcWallet) ImportTaprootScript(scope waddrmgr.KeyScope, | |||
// the specified outputs. In the case the wallet has insufficient funds, or the | |||
// outputs are non-standard, a non-nil error will be returned. | |||
// | |||
// NOTE: This method requires the global coin selection lock to be held. | |||
// NOTE: This method requires the global coin selection lock to be held.n |
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 is not addressed
lnwallet/btcwallet/btcwallet.go
Outdated
@@ -974,7 +974,7 @@ func (b *BtcWallet) ImportTaprootScript(scope waddrmgr.KeyScope, | |||
// the specified outputs. In the case the wallet has insufficient funds, or the | |||
// outputs are non-standard, a non-nil error will be returned. | |||
// | |||
// NOTE: This method requires the global coin selection lock to be held.n | |||
// NOTE: This method requires the global coin selection lock to be held. |
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.
ok i see it now, this should be cherry-picked to where it's introduced.
sweep/walletsweep.go
Outdated
func CraftSweepAllTx(feeRate, maxFeeRate chainfee.SatPerKWeight, | ||
blockHeight uint32, deliveryAddrs []DeliveryAddr, | ||
changeAddr btcutil.Address, coinSelectLocker CoinSelectionLocker, | ||
utxoSource UtxoSource, outputLeaser OutputLeaser, | ||
signer input.Signer, minConfs int32) (*WalletSweepPackage, error) { | ||
signer input.Signer, minConfs int32, | ||
selectUtxos ...wire.OutPoint) (*WalletSweepPackage, error) { |
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.
Why do we want to use a variadic param rather than a slice here? I think it's easier if it's selectUtxos []wire.OutPoint)
and we can pass the selectOutpoints
instead of selectOutpoints...
in SendCoins
?
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.
Not really easier in my opinion as that would also mean we would have to make changes other places this function was called
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 the diff is fairly simple,
diff --git a/rpcserver.go b/rpcserver.go
index 80cff0763..619c635a8 100644
--- a/rpcserver.go
+++ b/rpcserver.go
@@ -1389,7 +1389,7 @@ func (r *rpcServer) SendCoins(ctx context.Context,
sweepTxPkg, err := sweep.CraftSweepAllTx(
feePerKw, maxFeeRate, uint32(bestHeight), nil,
targetAddr, wallet, wallet, wallet.WalletController,
- r.server.cc.Signer, minConfs, selectOutpoints...,
+ r.server.cc.Signer, minConfs, selectOutpoints,
)
if err != nil {
return nil, err
@@ -1444,7 +1444,7 @@ func (r *rpcServer) SendCoins(ctx context.Context,
outputs, targetAddr, wallet, wallet,
wallet.WalletController,
r.server.cc.Signer, minConfs,
- selectOutpoints...,
+ selectOutpoints,
)
if err != nil {
return nil, err
diff --git a/sweep/walletsweep.go b/sweep/walletsweep.go
index f0ec70e7c..1fa2068ea 100644
--- a/sweep/walletsweep.go
+++ b/sweep/walletsweep.go
@@ -220,7 +220,7 @@ func CraftSweepAllTx(feeRate, maxFeeRate chainfee.SatPerKWeight,
changeAddr btcutil.Address, coinSelectLocker CoinSelectionLocker,
utxoSource UtxoSource, outputLeaser OutputLeaser,
signer input.Signer, minConfs int32,
- selectUtxos ...wire.OutPoint) (*WalletSweepPackage, error) {
+ selectUtxos []wire.OutPoint) (*WalletSweepPackage, error) {
// TODO(roasbeef): turn off ATPL as well when available?
diff --git a/sweep/walletsweep_test.go b/sweep/walletsweep_test.go
index 8cc93c941..724b9d85d 100644
--- a/sweep/walletsweep_test.go
+++ b/sweep/walletsweep_test.go
@@ -346,7 +346,7 @@ func TestCraftSweepAllTxCoinSelectFail(t *testing.T) {
_, err := CraftSweepAllTx(
0, 0, 10, nil, nil, coinSelectLocker, utxoSource, utxoLeaser,
- nil, 0,
+ nil, 0, nil,
)
// Since we instructed the coin select locker to fail above, we should
@@ -372,7 +372,7 @@ func TestCraftSweepAllTxUnknownWitnessType(t *testing.T) {
_, err := CraftSweepAllTx(
0, 0, 10, nil, nil, coinSelectLocker, utxoSource, utxoLeaser,
- nil, 0,
+ nil, 0, nil,
)
// Since passed in a p2wsh output, which is unknown, we should fail to
@@ -461,7 +461,7 @@ func TestCraftSweepAllTx(t *testing.T) {
sweepPkg, err := CraftSweepAllTx(
0, 0, 10, nil, deliveryAddr, coinSelectLocker,
utxoSource, utxoLeaser, signer, 0,
- tc.selectUtxos...,
+ tc.selectUtxos,
)
if tc.errString != "" {
require.ErrorContains(t, err, tc.errString)
sweep/walletsweep.go
Outdated
@@ -260,6 +263,19 @@ func CraftSweepAllTx(feeRate, maxFeeRate chainfee.SatPerKWeight, | |||
|
|||
log.Trace("[WithCoinSelectLock] finished fetching UTXOs") | |||
|
|||
// Use select utxos, if provided. | |||
if len(selectUtxos) > 0 { | |||
if fn.HasDuplicates(selectUtxos) { |
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 check has been repeated multiple times - if it's already done in SendCoins
so there's no need to check it 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.
CraftSweepAllTx is an independent function it is not bound to be used in sendcoins only, so I think it makes sense that it carries out this check as well
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.
In that case, all other places should remove this check, there's no need to duplicate this logic and we should implement it once - easier to test and maintain.
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.
A better option is to use set (map
) - that way we don't even need to worry about duplicates.
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.
Or maybe the Set structure in the fn
package? What do you think?
sweep/walletsweep_test.go
Outdated
selectUtxos: nil, | ||
}, | ||
{ | ||
name: "sweep select utxos in wallet, no error", |
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: select -> selected, same below
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.
select can actually be used as an adjective
cmd/lncli/commands.go
Outdated
if ctx.IsSet("utxo") { | ||
utxos := ctx.StringSlice("utxo") | ||
|
||
if fn.HasDuplicates(utxos) { |
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 is checked again - we should enforce the single responsibility principle and have only one place to implement this logic.
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.
prelim checks. I think I have seen this done a couple of places for lncli
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.
In that case, we should also fix them in future PRs.
@@ -376,14 +376,15 @@ func (h *HarnessRPC) SendCoins( | |||
return resp | |||
} | |||
|
|||
// SendCoinsAssertErr sends a given amount of money to the specified address | |||
// from the passed node and asserts an error has returned. | |||
func (h *HarnessRPC) SendCoinsAssertErr(req *lnrpc.SendCoinsRequest) { |
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 stick to the XXXAssertErr
pattern? That's the pattern used in all other assertions, OpenChannelAssertErr
, ConnectPeerAssertErr
, etc.
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 is acctually a logic change in this function. Hence AssertErr might not be really descriptive
ctxt, cancel := context.WithTimeout(h.runCtx, DefaultTimeout) | ||
defer cancel() | ||
|
||
_, err := h.LN.SendCoins(ctxt, req) | ||
require.Error(h, err, "node %s didn't not return an error", h.Name) |
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 we can keep this assertion.
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.
@@ -13,6 +13,7 @@ require ( | |||
github.com/btcsuite/btcwallet v0.16.10-0.20240404104514-b2f31f9045fb | |||
github.com/btcsuite/btcwallet/wallet/txauthor v1.3.4 | |||
github.com/btcsuite/btcwallet/wallet/txrules v1.2.1 | |||
github.com/btcsuite/btcwallet/wallet/txsizes v1.2.4 |
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.
hmmm how did you create this commit? Like what was the command used?
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.
make tidy-module
Txid: sweepHash[:], | ||
Label: "label that will not work", | ||
Overwrite: false, | ||
tcs := []struct { |
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've laid out my view on table-driven test here and I think we should avoid it in itest.
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.
In general, are you saying that if a function has more than two args we should not use TDD for it?
I like using table-driven tests when the outputs of a function are only determined by one or two inputs in a test setup,
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.
It depends - but we should discourage using it in itest because it's difficult to maintain.
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 am sorry I am having difficulty refactoring this to not use TDD. I am not sure what the non TDD version should look like. Would it be something like this?
similarfunction := func(t *testing.T, req *sendcoinsRequest, someconditionalsthatwecheck){
----
}
// With select utxos
ht.Run(){
similarfunction()
}
// With no select utxos
ht.Run(){
similarfunction()
}
Would I need to refactor testsendcoinsselectutxos
as well?
@yyforyongyu I think we do that already? I responded to that in the specific code piece you wrote the review in. Let me know if that works. |
Signed-off-by: Ononiwu Maureen <[email protected]>
Signed-off-by: Ononiwu Maureen <[email protected]>
Signed-off-by: Ononiwu Maureen <[email protected]>
Signed-off-by: Ononiwu Maureen <[email protected]>
Signed-off-by: Ononiwu Maureen <[email protected]>
This commit changes `SendCoinsAssertErr` to `SendCoinsReturnErr`. The function now returns the resulting error from calling `sendCoins`. Signed-off-by: Ononiwu Maureen <[email protected]>
Signed-off-by: Ononiwu Maureen <[email protected]>
Signed-off-by: Ononiwu Maureen <[email protected]>
Signed-off-by: Ononiwu Maureen <[email protected]>
@yyforyongyu: review reminder |
Replaced by #8955. |
Change Description
This PR fixes this issue: #6949 (comment)
Depends on: btcsuite/btcwallet#912 and #8729
In this well detailed issue comment, the process of sending funds using utxos selected by the sender currently has over five steps in which one has to call various psbt APIs.
The goal of this PR is to improve user experience by including
utxos
to thesendcoins
command and corresponding rpc structs (SendCoinsRequest
andSendCoinsResponse
) which would enable the sender described to achieve the same aim with one command.This is done by adding a new field to rpc structs
SendCoinsRequest
andSendCoinsResponse
to enable users request for that functionality and enable the driving function (in which we modify its functionality as well) use these utxos when crafting the transaction for this functionality.Additionally, the
sweepall
field in the above mentioned request and response rpc structs when true now not only sweeps ALL funds in the wallet but also ALL funds in the selected utxos when used in conjunction with the new select utxos field.The lncli sendcoins command was updated to include the flag,
utxo
to enable this functionality on that end.There were also slight internal logic change, where a slice of utxos are now accepted by relevant functions as variadic argument, functional options to enable this functionality.
Steps to Test
Steps for reviewers to follow to test the change.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.