wallet: simplify ListCoins implementation #25659
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
ACK 6451afa.
- Verified that default values have not been changed in the
AvailableCoinsParamsstruct. - I understand that skipped coins are skipped in the
AvailableCoinsfunction instead of being post-processed to avoid iterating over all coins again. ListCoinsis now much more readable.
6451afa to
adfdd02
Compare
|
Thanks for the feedback @aureleoules!
On master, by default, we skip the locked coins inside |
adfdd02 to
5f5dd3a
Compare
|
Rebased on master due conflicts with recently merged 24584. Ready to go again. |
5f5dd3a to
e8b9bd8
Compare
|
rebased |
|
Concept ACK |
There was a problem hiding this comment.
Concept ACK on simplifying ListCoins but a little concerned with the general trend of making AvailableCoins a general purpose wallet traversal tool. I'm more thinking out loud here, so feel free to ignore me.
It seems we've added a few flags recently to AvailableCoins to remove additional lookups in other wallet functions and simplify code, which is great, but I'm worried that instead of simplifying we are instead obfuscating complexity and expanding the responsibility of AvailableCoins to do too many things.
This is more of a meta concern and not really a criticism of this PR, so don't want to block if other reviewers feel strongly this is the right way to go. Personally, I'm more in favor of leaving it as is for now and instead starting a discussion on a more holistic solution. If we do decide to keep increasing the responsibility of AvailableCoins, I'd recommend we leave the function signature as is and not wrap params in a struct so as to keep complexity explicit rather than hidden.
|
Are you still working on this? |
|
Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened. |
|
@achow101 please re-open it. Will rebase it, it will be smaller now. |
e8b9bd8 to
65e5d15
Compare
|
rebased, status updated. |
65e5d15 to
64494e6
Compare
9622fe6 test: move coins result test to wallet_tests.cpp (furszy) f69347d test: extend and simplify availablecoins_tests (furszy) 212ccdf wallet: AvailableCoins, add arg to include/skip locked coins (furszy) Pull request description: Negative PR with extended test coverage :). 1) Cleaned duplicated code and added coverage for the 'AvailableCoins' incremental result. 2) The class `AvailableCoinsTestingSetup` inside `availablecoins_tests.cpp` is a plain copy of `ListCoinsTestingSetup` that is inside `wallet_tests.cpp`. So, deleted the file and moved the `BasicOutputTypesTest` test case to `wallet_tests.cpp`. 3) Added arg to include/skip locked coins from the `AvailableCoins` result. This is needed for point (1) as otherwise the wallet will spend the coins that we recently created due its closeness to the recipient amount. Note: this last point comes from #25659 where I'm using the same functionality to clean/speedup another flow as well. ACKs for top commit: achow101: ACK 9622fe6 theStack: ACK 9622fe6 aureleoules: reACK 9622fe6, nice cleanup! Tree-SHA512: 1ed9133120bfe8815455d1ad317bb0ff96e11a0cc34ee8098716ab9b001749168fa649212b2fa14b330c1686cb1f29039ff1f88ae306db68881b0428c038f388
Can remove the locked coins lookup if we include them directly inside the AvailableCoins result
The function is only used in ListCoins.
64494e6 to
a2ac6f9
Compare
|
rebased post #25789 merge. Now this PR purely cleans up an ugly flow. |
There was a problem hiding this comment.
ACK a2ac6f9, LGTM
I verified the call to AvailableCoins takes the same coin controls params as AvailableCoinsListUnspent except for skip_locked which is now false and fAllowWatchOnly to allow the removal of the large and redundant chunk of code below.
There was a problem hiding this comment.
Code-review ACK a2ac6f9
As discussed in IRC, note that the first commit is more than just a refactor, as it introduces some changes in behaviour regarding locked coins, since the filtering in AvailableCoins is much more detailled than the manual loop in ListCoins that is replaced:
- locked unsafe coins are are included on master, but excluded on the PR (
CCoinControl'sm_include_unsafe_inputsis false by default) - locked immature coinbase coins are included on master, but excluded on the PR (
CoinFilterParams'include_immature_coinbaseis false by default) - locked coins that are unconfirmed but not in the mempool are included in the master, but excluded on the PR (no option available to pass that, but I'm not sure how to even reach such a state)
However, all of those behaviour changes seem to make sense, the commit could be seen as a bugfix. It doesn't make much sense to display unsafe or immature coins for selection in the GUI (that's the only place where ListCoins is currently used, via the listCoins interface). For a follow-up candidate, would be nice to add a proper test for ListCoins which would have detected the behaviour changes.
|
Thanks for the detailed review @theStack! |
|
ACK a2ac6f9
Additionally all locked coins are not able to be selected in the GUI, so this does not materially affect what users are able to choose. However it does affect what is displayed to users. |
a2ac6f9 wallet: unify FindNonChangeParentOutput functions (furszy) b3f4e82 wallet: simplify ListCoins implementation (furszy) Pull request description: Focused on the following changes: 1) Removed the entire locked coins lookup that was inside `ListCoins` by including them directly on the `AvailableCoins` result (where we were skipping them before). 2) Unified both `FindNonChangeParentOutput` functions (only called from `ListCoins`) ACKs for top commit: achow101: ACK a2ac6f9 aureleoules: ACK a2ac6f9, LGTM theStack: Code-review ACK a2ac6f9 Tree-SHA512: f72b21ee1600c5992559b5dcd8ff260527afac2ec696737f998343a0850b84d439e7f86ea52a14cc0cddabf132a30bf5b52fb34950578ac323083d4375b937f1
Focused on the following changes:
ListCoinsby including them directly on theAvailableCoinsresult (where we were skipping them before).FindNonChangeParentOutputfunctions (only called fromListCoins)