wallet, refactor: #24584 follow-ups #25734

josibake · 2022-07-29T11:16:21Z

This PR is to address follow-ups for #24584, specifically:

Remove redundant, hard-to-read code by adding a new OutputType and adding shuffle, erase, and push_back methods for CoinsResult
Add missing BOOST_ASSERT to unit test
Ensure functional test only runs if using descriptor wallets
Improve readability of AttemptSelection by removing triple-nested if statement

Note for reviewers: commit refactor: add new helper methods should throw an "unused function warning"; the function is used in the next commit. Also, commit wallet: switch to new shuffle, erase, push_back will fail to compile, but this is fixed in the next commit with a scripted-diff. the commits are separate like this (code change then scripted-diff) to improve legibility.

josibake · 2022-07-29T11:18:35Z

tagging @furszy , @LarryRuane , @xekyo , and @aureleoules for review as this addresses feedback from y'all on #24584 . thanks for the thorough review and great suggestions, let me know if i missed anything!

furszy · 2022-07-29T12:16:14Z

Cool. Obvious concept ACK due co-authoring some of the commits.

This cleans a lot of boilerplate code of the recently introduced changes.

DrahtBot · 2022-07-29T21:59:09Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25806 (wallet: single outputs grouping calculation by furszy)
#25789 (test: clean and extend availablecoins_tests coverage by furszy)
#25685 (wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection by furszy)
#25659 (wallet: simplify ListCoins implementation by furszy)
#24897 ([Draft / POC] Silent Payments by w0xlt)
#23417 (wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101)
#22341 (rpc: add getxpub by Sjors)
#19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)

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.

aureleoules

Concept ACK
This simplifies the code and is easier to read!
I left some comments.

test/functional/wallet_avoid_mixing_output_types.py

src/wallet/spend.cpp

w0xlt

Approach ACK.
These changes improve code readability and organization.

furszy

3e6cbb2 doesn't compile without ac0585d. Better to squash them.

src/wallet/spend.h

furszy · 2022-08-01T16:39:07Z

src/wallet/spend.h

@@ -54,6 +55,9 @@ struct CoinsResult {
     * i.e., methods can work with individual OutputType vectors or on the entire object */
    uint64_t size() const;
    void clear();
+    void erase(std::set<COutPoint>& preset_coins);


Note: this function will not longer be needed after #25685.

src/wallet/scriptpubkeyman.cpp

furszy

Another point, CanGetAddresses and GetActiveScriptPubKeyMans loop through the available output types vector as well. And they are not included in c81cb2a

josibake · 2022-08-02T08:43:05Z

Another point, CanGetAddresses and GetActiveScriptPubKeyMans loop through the available output types vector as well. And they are not included in c81cb2a

both functions you mentioned call GetScriptPubKeyMan(), which checks if there is a spk_m for the specified output type in either m_internal_spk_managers or m_external_spk_managers. spk_ms are setup by SetupDescriptorPubKeyMans, where we are skipping the unknown type

josibake · 2022-08-02T09:45:19Z

3e6cbb2 doesn't compile without ac0585d. Better to squash them.

I split the changes into two commits to make the review easier and to be able to do the second part as a scripted-diff. This is mentioned in the commit message and the description, so it shouldn't be a surprise to reviewers

josibake · 2022-08-02T10:46:14Z

force pushed to address comments from @furszy and @aureleoules ; git range-diff master e40eaf6 7089df5

furszy · 2022-08-02T13:56:02Z

3e6cbb2 doesn't compile without ac0585d. Better to squash them.

I split the changes into two commits to make the review easier and to be able to do the second part as a scripted-diff. This is mentioned in the commit message and the description, so it shouldn't be a surprise to reviewers

Isn't about being surprised by it or not. It's about being able to compile each commit separately while traversing the repository history. It's useful when you are tracking bugs to know where test started failing.
Plus AFAIK, it's a repository "policy".

adam2k · 2022-08-03T16:59:59Z

Approach ACK: 7089df5

I'm leaving a ~~few~~ minor nits. Feel free to ignore or take ~~them~~ it into consideration.

src/wallet/spend.cpp

achow101

41703e5 does not compile

src/wallet/spend.cpp

add to enum, array and handle UNKNOWN in various case statements

add Shuffle, Erase, and Add to CoinsResult struct add a helper function for mapping TxoutType to OutputType Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

Change `CoinsResult` functions to uppercase to be consistent with the style guide. -BEGIN VERIFY SCRIPT- git grep -l "available_coins" | grep -v mempool_stress.cpp | xargs sed -i "s/available_coins\.\(size\|all\|clear\)/available_coins\.\u\1/" git grep -l AvailableCoins | xargs sed -i "/AvailableCoins/ s/\(all()\|size()\|clear()\)/\u\1/" sed -i "s/\(clear()\|all()\|size()\)/\u&/g" src/wallet/spend.h sed -i "/CoinsResult::/ s/\(clear()\|all()\|size()\)/\u&/" src/wallet/spend.cpp sed -i "s/result.size/result.Size/" src/wallet/spend.cpp sed -i "s/this->size/this->Size/" src/wallet/spend.cpp -END VERIFY SCRIPT-

switch to new methods, remove old code. this also updates the Size, All, and Clear methods to now use the coins map. this commit is not strictly a refactor because previously coin selection was never run over the UNKNOWN type until the last step when being run over all. now that we are iterating over each, it is run over UNKNOWN but this is expected to be empty most of the time. Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

this was missed in the original PR

since this test uses bech32m, we skip unless sqlite is used, which is the same as checking if we are using descriptor wallets or not

it was pointed out by a few reviewers that the code block at the end of attempt selection was difficult to follow and lacked comments. refactor to get rid of triple nested if statement and improve readibility.

josibake · 2022-08-10T13:21:23Z

3e6cbb2 doesn't compile without ac0585d. Better to squash them.

I split the changes into two commits to make the review easier and to be able to do the second part as a scripted-diff. This is mentioned in the commit message and the description, so it shouldn't be a surprise to reviewers

Isn't about being surprised by it or not. It's about being able to compile each commit separately while traversing the repository history. It's useful when you are tracking bugs to know where test started failing. Plus AFAIK, it's a repository "policy".

Good point re: repository history; I squashed the two together so it compiles now

josibake · 2022-08-10T13:28:30Z

force pushed to rebase, address feedback + a few minor fixups, and added a scripted-diff to uppercase the CoinsResult functions. git range-diff master 7089df5 8cd21bb

aureleoules

ACK 8cd21bb.
Verified all the comments of the original PR have been addressed.
This PR makes available_coins easier to use and simplifies the code overall.

LarryRuane

Concept, code review ACK 8cd21bb

Summary of my review: I made a branch to show suggestions, feel free to cherry-pick.

LarryRuane · 2022-08-16T03:02:29Z

src/wallet/spend.h

-
-    /** Other is a catch-all for anything that doesn't match the known OutputTypes */
-    std::vector<COutput> other;
+    std::map<OutputType, std::vector<COutput>> coins;


Perhaps a std::map is overkill since there are only a few fixed types, and they're represented as small integers (since it's an enum). It seems like an array, with type as the array index, would be more appropriate. I started to leave a suggestion here to make this an array, but first wanted to check that suggestion would work, so I made the necessary changes and pushed it up to a branch: https://github.com/LarryRuane/bitcoin/commits/suggestions-25734

That branch directly modifies only the second commit "refactor: add new helper methods", then I cherry-picked the following commits to work with that commit (so the changes to those commits are only side-effects).

I like the way this came out, but I understand if you would rather not lose any ACKs or take the trouble to cherry-pick this branch. Just a suggestion.

Thanks for the review and suggestions, @LarryRuane and sorry for the slow response. I do like this approach. Will definitely keep this in mind if I am working on the CoinsResult struct again soon

furszy

utACK 8cd21bb. Left a small, non-blocking, comment.

Plus, I would also like to have the GetCoinsByType that @LarryRuane suggested (would remove the ByType wording as the type filter is explicitly provided as a method argument).

But, overall, it's a non-blocking comment. We can move forward with it.

furszy · 2022-08-16T13:36:07Z

src/wallet/spend.h

+ * Size(), Clear(), Erase(), Shuffle(), and Add() methods are implemented to
+ * allow easy interaction with the struct.


I think that this comment is more suited for a commit description, not for the sources. It doesn't provide any meaningful information.

good point, I'll remove if I touch this in a later PR

jonatack · 2022-08-16T14:11:52Z

src/wallet/spend.cpp

+void CoinsResult::Add(OutputType type, const COutput& out)
+{
+    coins[type].emplace_back(out);
+}


Quick look on GitHub while waiting for a build:

any reason not to inline some of these methods? i.e. remove it from the .cpp and change the .h file to the following:

void Add(OutputType type, const COutput& output) { coins[type].emplace_back(output); }

same for Clear() (and maybe Shuffle() but only if it's a hotspot)

naming nit: a param named out reads like an out-param; maybe output instead

header nit: maybe add missing #include <random.h> in the commit where the call to ::Shuffle is added, feel free to ignore

Thanks for the review @jonatack , great suggestions. Generally speaking, is there a heuristic we use in core for when to inline vs not?

Also, will include your suggestions if working with CoinsResult again

when to inline vs not?

Inlining improves performance with the potential trade-off of increasing the code in the .h file and making changes to it slower to compile, so I think about it as: inline where performance is important, or for methods that are both very short (e.g. a single line or maybe 2-3) and that don't add an include dependency to the header file (rather than in the implementation file). Maybe people have other criteria or thoughts about it, though.

achow101 · 2022-08-17T00:00:08Z

ACK 8cd21bb

maflcko · 2022-08-18T19:04:53Z

src/outputtype.h

 };

 static constexpr auto OUTPUT_TYPES = std::array{
    OutputType::LEGACY,
    OutputType::P2SH_SEGWIT,
    OutputType::BECH32,
    OutputType::BECH32M,
+    OutputType::UNKNOWN,


See https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=50329

Straightforward #25869

…TYPES array 5b4fdbb wallet: remove UNKNOWN type from OUTPUT_TYPES array (furszy) Pull request description: Fixing bitcoin/bitcoin#25734 (comment) -> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=50329 The `OUTPUT_TYPES` array contain the known active output types only. And it's solely used to create/walk-through the active spkms. So, no need to add the `UNKNOWN` type here. ACKs for top commit: achow101: ACK 5b4fdbb w0xlt: ACK bitcoin/bitcoin@5b4fdbb LarryRuane: ACK 5b4fdbb Tree-SHA512: dee2dc362a1b0c777555e5ee4d355a3351340591d0096f74e8c3a25f374cb2d9aef26145977ff4dd0f8cc940da9464eb5541eb2895bc19f8cbd6bb6d292ab9a9

fanquake added the Wallet label Jul 29, 2022

josibake mentioned this pull request Jul 29, 2022

rpc: Filter inputs by type during CoinSelection #25183

Closed

josibake force-pushed the josibake-24584-follow-ups branch 2 times, most recently from 8468045 to e40eaf6 Compare July 29, 2022 15:57

aureleoules reviewed Jul 30, 2022

View reviewed changes

test/functional/wallet_avoid_mixing_output_types.py Show resolved Hide resolved

src/wallet/spend.cpp Outdated Show resolved Hide resolved

w0xlt reviewed Aug 1, 2022

View reviewed changes

furszy reviewed Aug 1, 2022

View reviewed changes

josibake force-pushed the josibake-24584-follow-ups branch 2 times, most recently from 6cc5cce to 7089df5 Compare August 2, 2022 10:45

adam2k reviewed Aug 3, 2022

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

This was referenced Aug 4, 2022

refactor: Replace BResult with util::Result #25721

Merged

wallet: Do not match legacy addresses for change type #24362

Closed

achow101 reviewed Aug 4, 2022

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

src/wallet/spend.cpp Outdated Show resolved Hide resolved

src/wallet/spend.cpp Outdated Show resolved Hide resolved

src/wallet/spend.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Aug 5, 2022

furszy mentioned this pull request Aug 8, 2022

wallet: group outputs only once, decouple it from Coin Selection #25806

Merged

aureleoules mentioned this pull request Aug 9, 2022

wallet: Check max transaction weight in CoinSelection #25729

Merged

josibake and others added 2 commits August 10, 2022 10:17

refactor: add UNKNOWN OutputType

f5649db

add to enum, array and handle UNKNOWN in various case statements

refactor: add new helper methods

3f27a2a

add Shuffle, Erase, and Add to CoinsResult struct add a helper function for mapping TxoutType to OutputType Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

josibake and others added 5 commits August 10, 2022 15:19

test: add missing BOOST_ASSERT

0760ce0

this was missed in the original PR

test: only run test for descriptor wallets

f47ff71

since this test uses bech32m, we skip unless sqlite is used, which is the same as checking if we are using descriptor wallets or not

refactor: improve readability for AttemptSelection

8cd21bb

it was pointed out by a few reviewers that the code block at the end of attempt selection was difficult to follow and lacked comments. refactor to get rid of triple nested if statement and improve readibility.

josibake force-pushed the josibake-24584-follow-ups branch from 7089df5 to 8cd21bb Compare August 10, 2022 13:20

DrahtBot removed the Needs rebase label Aug 10, 2022

This was referenced Aug 10, 2022

test: clean and extend availablecoins_tests coverage #25789

Merged

rpc: add path to gethdkey #22341

Closed

wallet: Migrate legacy wallets to descriptor wallets #19602

Merged

aureleoules reviewed Aug 15, 2022

View reviewed changes

LarryRuane reviewed Aug 16, 2022

View reviewed changes

furszy approved these changes Aug 16, 2022

View reviewed changes

jonatack reviewed Aug 16, 2022

View reviewed changes

DrahtBot mentioned this pull request Aug 16, 2022

[Draft / POC] Silent Payments #24897

Closed

achow101 merged commit 64f7a19 into bitcoin:master Aug 17, 2022

maflcko reviewed Aug 18, 2022

View reviewed changes

furszy mentioned this pull request Aug 18, 2022

wallet: remove UNKNOWN type from OUTPUT_TYPES array #25869

Merged

fanquake mentioned this pull request Nov 28, 2022

wallet: bugfix, invalid CoinsResult cached total amount #26560

Merged

bitcoin locked and limited conversation to collaborators Aug 18, 2023

josibake deleted the josibake-24584-follow-ups branch January 26, 2024 10:50

		* Size(), Clear(), Erase(), Shuffle(), and Add() methods are implemented to
		* allow easy interaction with the struct.

wallet, refactor: #24584 follow-ups #25734

wallet, refactor: #24584 follow-ups #25734

Uh oh!

Conversation

josibake commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josibake commented Jul 29, 2022

Uh oh!

furszy commented Jul 29, 2022

Uh oh!

DrahtBot commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josibake commented Aug 2, 2022

Uh oh!

josibake commented Aug 2, 2022

Uh oh!

josibake commented Aug 2, 2022

Uh oh!

furszy commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adam2k commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josibake commented Aug 10, 2022

Uh oh!

josibake commented Aug 10, 2022

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josibake commented Jul 29, 2022 •

edited

Loading

DrahtBot commented Jul 29, 2022 •

edited

Loading

furszy left a comment •

edited

Loading

furszy commented Aug 2, 2022 •

edited

Loading

adam2k commented Aug 3, 2022 •

edited

Loading

furszy left a comment •

edited

Loading

jonatack Aug 16, 2022 •

edited

Loading

jonatack Aug 17, 2022 •

edited

Loading