Simplify coin selection for sendcoins#8516
Conversation
Chinwendu20
requested review from
Crypt-iQ,
Roasbeef,
ellemouton,
yyforyongyu and
ziggie1984
as code owners
|
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 |
guggero
requested review from
guggero
and removed request for
Roasbeef,
yyforyongyu and
ziggie1984
|
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) |
ellemouton
requested review from
sputn1ck
and removed request for
Crypt-iQ and
ellemouton
|
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.. |
Chinwendu20
force-pushed
the
sendcoins
branch
3 times, most recently
from
c800df5 to
3e6ab6a
Compare
|
I think the taproot test needs to create a new node and not use the general Alice node. Two tests can request sendCoins to sweep all coins at the same time, creating the same transaction same txid, the other test can get the txid first into the mempool making the sweepall sendcoins operation fail in the other. I do not know if that makes sense. If it does I can open a new PR for a fix. In the meantime, this needs rebasing but I need to know where this stands in approval so I make changes at once. |
|
Maybe all tests should create their own node to prevent cases like this? |
Not sure how this applies to the tests, in here I think we should fix the itests in a separate commit in this PR. |
|
When I checked the logs for Alice related to this test, I saw this sendcoins is called sequentially in that test but other tests using the Alice node would call sendcoins in parallel, right? Maybe if we rerun this it would pass, seems like a case of unprecedented slow server response |
|
fyi I ran this test locally and it was successful. |
|
@Heebs the CI has been ran the failed tests that has to do with sendcoins are the ones using postgres backend, other itests pass. I think it is unrelated. Maybe as a separate issue we need to figure out why the response seems to be coming slower than expected for that case. |
There was a problem hiding this comment.
There are three itests failing atm:
1.) The CI required one https://github.com/lightningnetwork/lnd/actions/runs/9270373663/job/25641223443?pr=8516#step:5:385. This might be related to sweeper changes that @yyforyongyu intends to get fixed in v0.18.1-beta iiuc. If that's the case would you @yyforyongyu sign off on this PR that we can ignore the failure here?
2.) https://github.com/lightningnetwork/lnd/actions/runs/9270373663/job/25641222998?pr=8516#step:5:920 This I can repro locally with make itest-parallel backend=bitcoind dbbackend=postgres, but it runs a long time.
3.) https://github.com/lightningnetwork/lnd/actions/runs/9270373663/job/25641222017?pr=8516#step:5:924 This I can't repro, but it should get resolved if 2.) is resolved since it is the same error.
Responded to this here: #8516 (comment) |
|
Ah right, I guess then it depends on if we can ignore the force-closure itest which is flaky and will be fixed in v0.18.1 according to @yyforyongyu Could you confirm that this is the flake you are referring to? |
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
This doesn't look like a flake to me, will investigate - meanwhile could you rebase and push so we can rerun the CI.
There was a problem hiding this comment.
| "will be used as input for the transaction; " + | |
| "will be used as an input for the transaction; " + |
There was a problem hiding this comment.
Just that that is what is done in similar tests:
lnd/itest/lnd_channel_funding_utxo_selection_test.go
Lines 65 to 69 in 351e9a6
There was a problem hiding this comment.
yea those places could also be updated - this is just a nit, so non-blocking
There was a problem hiding this comment.
In go we almost always use anonymous structs for table driven test.
There was a problem hiding this comment.
Think there's a simpler one-line call you can use,
Lines 322 to 324 in 613bfc0
There was a problem hiding this comment.
node Alice should be created here
There was a problem hiding this comment.
While this test looks great, I'd suggest at least moving the tc.sendAll branch to a new test, either integrating it in testSweepAllCoins, or a new test testSendCoinSelectUtxoSendAll, so this test is easier to follow and maintain in the future.
There was a problem hiding this comment.
I think I can integrate it into the testSweepAllCoins. Looking at this again I think I can move this test to the lnd_misc_test.go file.
But if I separate the test, I would have to change the test name, right? Do you have any naming suggestions?
There was a problem hiding this comment.
I think the name is descriptive enough tho
There was a problem hiding this comment.
hmmm in this case we usually write := instead of var.
There was a problem hiding this comment.
yeah it's a bit different, but again it's non-blocking.
There was a problem hiding this comment.
why do we use opts ...SendOutputOption instead of inputs []wire.OutPoint or even a map inputs map[wire.OutPoint]struct?
There was a problem hiding this comment.
Not sure why we should consider using map[wire.OutPoint]struct. Also this way we can better extend the function in the future, also using this, inputs []wire.OutPoint would mean that we would have to change other existing callsite in the code.
If you want can still change it to, inputs []wire.OutPoint
There was a problem hiding this comment.
I'd prefer using explicit names and structs, so the caller can understand the params more easily. Re extensibility, yea that's also true, tho IMO a better way is to construct sth like SendOutputsRequest and use it as the param - future PRs tho.
There was a problem hiding this comment.
I don't think this is true as it depends on the conf targets we use when listing utxos from the wallet
There was a problem hiding this comment.
would be nice to also return which utxo is not found - maybe add something like fmt.Errorf("outpoint not found: %w", err)
There was a problem hiding this comment.
I think the fn library would output that alreadY:
Line 37 in f88f120
There was a problem hiding this comment.
Def - but it's pretty low-level and there's no context around this error. Imagine a second place also using this NewSubMap method, then we'd have a bit of trouble locating the place that generated the error.
I think it is unrelated to the PR, we can create a separate issue for that case so maybe we do not block this, same thing happens in another PR: We can track it here: #8806 |
|
Cool I think this is why the itests failed, so not related to this PR. |
hieblmi
left a comment
hieblmi
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
move to release notes for v0.18.1-beta.
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
|
@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. |
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
There's a missing check that we need to ensure provided utxos are known to the wallet.
There was a problem hiding this comment.
ok i see it now, this should be cherry-picked to where it's introduced.
There was a problem hiding this comment.
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.
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.
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)There was a problem hiding this comment.
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.
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.
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.
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.
Or maybe the Set structure in the fn package? What do you think?
There was a problem hiding this comment.
nit: select -> selected, same below
There was a problem hiding this comment.
select can actually be used as an adjective
There was a problem hiding this comment.
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.
prelim checks. I think I have seen this done a couple of places for lncli
There was a problem hiding this comment.
In that case, we should also fix them in future PRs.
There was a problem hiding this comment.
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.
There is acctually a logic change in this function. Hence AssertErr might not be really descriptive
There was a problem hiding this comment.
I think we can keep this assertion.
There was a problem hiding this comment.
There was a problem hiding this comment.
hmmm how did you create this commit? Like what was the command used?
There was a problem hiding this comment.
make tidy-module
There was a problem hiding this comment.
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.
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.
It depends - but we should discourage using it in itest because it's difficult to maintain.
There was a problem hiding this comment.
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 <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
This commit changes `SendCoinsAssertErr` to `SendCoinsReturnErr`. The function now returns the resulting error from calling `sendCoins`. Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
|
@yyforyongyu: review reminder |
|
Replaced by #8955. |
8 participants
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
utxosto thesendcoinscommand and corresponding rpc structs (SendCoinsRequestandSendCoinsResponse) 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
SendCoinsRequestandSendCoinsResponseto 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
sweepallfield 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,
utxoto 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.