wallet, refactor: #24584 follow-ups

josibake commented at 11:16 am on July 29, 2022: member

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.

fanquake added the label Wallet on Jul 29, 2022

josibake commented at 11:18 am on July 29, 2022: member

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 commented at 12:16 pm on July 29, 2022: member

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

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

josibake force-pushed on Jul 29, 2022

DrahtBot commented at 9:59 pm on July 29, 2022: contributor

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.

in test/functional/wallet_avoid_mixing_output_types.py:127 in e40eaf6e1d outdated

123@@ -124,6 +124,7 @@ def set_test_params(self):
124 
125     def skip_test_if_missing_module(self):
126         self.skip_if_no_wallet()
127+        self.skip_if_no_sqlite()

aureleoules commented at 11:36 am on July 30, 2022:

this doesn’t seem to fix it :/

0python test/functional/wallet_avoid_mixing_output_types.py
1...
2test_framework.authproxy.JSONRPCException: Legacy wallets cannot provide bech32m addresses (-8)

josibake commented at 5:16 pm on July 31, 2022:

i think this is the best we can do right now with the test framework. if no sqlite, skip. if sqlite and bdb, pass the --descriptors flag, else the test will fail

in src/wallet/spend.cpp:109 in e40eaf6e1d outdated

112+
113+void CoinsResult::erase(std::set<COutPoint>& preset_coins)
114+{
115+    for (auto& it : coins) {
116+        auto& vec = it.second;
117+        auto i = find_if(vec.begin(), vec.end(), [&](const COutput &c) { return preset_coins.count(c.outpoint);});

aureleoules commented at 11:36 am on July 30, 2022:

0        auto i = std::find_if(vec.begin(), vec.end(), [&](const COutput &c) { return preset_coins.count(c.outpoint);});

aureleoules commented at 11:55 am on July 30, 2022: member

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

w0xlt commented at 1:58 pm on August 1, 2022: contributor

Approach ACK. These changes improve code readability and organization.

in src/wallet/spend.h:41 in b4852bce6a outdated

37@@ -38,6 +38,7 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* walle
38  * the CoinsResult struct as if it was a vector
39  */
40 struct CoinsResult {
41+    std::map<OutputType, std::vector<COutput>> coins;

furszy commented at 4:38 pm on August 1, 2022:

would make it private so the field access is encapsulated.

josibake commented at 9:33 am on August 2, 2022:

id prefer to keep it simple for now. im not convinced it’s worth re-writing the coinselection_tests to create TxoutTypes just to map them back to COutPuts and to add methods just for checking the size in the availablecoins_tests

in src/wallet/spend.h:50 in b4852bce6a outdated

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

furszy commented at 4:39 pm on August 1, 2022:

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

in src/wallet/scriptpubkeyman.cpp:1973 in c81cb2aa86 outdated

1965@@ -1966,6 +1966,10 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_
1966         desc_prefix = "tr(" + xpub  + "/86'";
1967         break;
1968     }
1969+    case OutputType::UNKNOWN: {
1970+        // Do nothing if the address is of UNKNOWN OutputType
1971+        return false;
1972+    }

furszy commented at 6:29 pm on August 1, 2022:

Could assert(false) here. DescriptorScriptPubKeyMan::SetupDescriptorGeneration is called from CWallet::SetupDescriptorScriptPubKeyMans and this latter function is already skipping this type.

josibake commented at 8:44 am on August 2, 2022:

good point, SetupDescriptorGeneration is actually defined on the DescriptorScriptPubKeyMan and we should never have a spk_m setup for the unknown type, so assert(false) is better than just returning a bool.

furszy commented at 6:43 pm on August 1, 2022: member

3e6cbb2d doesn’t compile without ac0585d4. Better to squash them.

furszy commented at 6:48 pm on August 1, 2022: member

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

josibake commented at 8:43 am on August 2, 2022: member

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 commented at 9:45 am on August 2, 2022: member

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 force-pushed on Aug 2, 2022

josibake commented at 10:46 am on August 2, 2022: member

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

furszy commented at 1:56 pm on August 2, 2022: member

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 commented at 4:59 pm on August 3, 2022: none

Approach ACK: 7089df5e9e084a61d66860e01f5aeffdcd348df9

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

in src/wallet/spend.cpp:145 in a0abea2669 outdated

140+            else return OutputType::BECH32;
141+            break;
142+        case TxoutType::SCRIPTHASH:
143+        case TxoutType::PUBKEYHASH:
144+            return OutputType::LEGACY;
145+            break;

adam2k commented at 6:29 pm on August 3, 2022:

nit: two of the case statements with the early return have break; and the first one does not. Could this be more consistent without the break; statements?

in src/wallet/spend.cpp:132 in a0abea2669 outdated

127+void CoinsResult::push_back(OutputType type, const COutput& out)
128+{
129+    coins[type].emplace_back(out);
130+}
131+
132+static OutputType getOutputType(TxoutType type, bool is_from_p2sh)

achow101 commented at 6:16 pm on August 4, 2022:

In a0abea26692e63e9205ec68bdbc04cfad8c95a3d “refactor: add new helper methods”

nit: We prefer UpperCamelCase for function names.

in src/wallet/spend.cpp:127 in a0abea2669 outdated

122+    for (auto& it : coins) {
123+        Shuffle(it.second.begin(), it.second.end(), rng_fast);
124+    }
125+}
126+
127+void CoinsResult::push_back(OutputType type, const COutput& out)

achow101 commented at 6:16 pm on August 4, 2022:

In a0abea26692e63e9205ec68bdbc04cfad8c95a3d “refactor: add new helper methods”

nit: We prefer UpperCamelCase for function names.

in src/wallet/spend.cpp:120 in a0abea2669 outdated

115+            break;
116+        }
117+    }
118+}
119+
120+void CoinsResult::shuffle(FastRandomContext& rng_fast)

achow101 commented at 6:16 pm on August 4, 2022:

In a0abea26692e63e9205ec68bdbc04cfad8c95a3d “refactor: add new helper methods”

nit: We prefer UpperCamelCase for function names.

in src/wallet/spend.cpp:108 in a0abea2669 outdated

104@@ -105,6 +105,49 @@ void CoinsResult::clear()
105     other.clear();
106 }
107 
108+void CoinsResult::erase(std::set<COutPoint>& preset_coins)

achow101 commented at 6:16 pm on August 4, 2022:

In a0abea26692e63e9205ec68bdbc04cfad8c95a3d “refactor: add new helper methods”

nit: We prefer UpperCamelCase for function names.

achow101 commented at 6:53 pm on August 4, 2022: member

41703e57cc0d2693533240ed916b9e55283433c6 does not compile

DrahtBot added the label Needs rebase on Aug 5, 2022

refactor: add UNKNOWN OutputType

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

f5649db9d5

refactor: add new helper methods

add Shuffle, Erase, and Add to CoinsResult struct
add a helper function for mapping TxoutType to OutputType

Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

3f27a2adce

scripted-diff: Uppercase function names

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-

b6b50b0f2b

wallet: switch to new shuffle, erase, push_back

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>

db09aec937

test: add missing BOOST_ASSERT

this was missed in the original PR

0760ce0b9e

test: only run test for descriptor wallets

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

f47ff71761

refactor: improve readability for AttemptSelection

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.

8cd21bb279

josibake force-pushed on Aug 10, 2022

josibake commented at 1:21 pm on August 10, 2022: member

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 commented at 1:28 pm on August 10, 2022: member

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

DrahtBot removed the label Needs rebase on Aug 10, 2022

aureleoules commented at 10:29 am on August 15, 2022: member

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

in src/wallet/spend.h:41 in 8cd21bb279

46-    std::vector<COutput> bech32;
47-    std::vector<COutput> bech32m;
48-
49-    /** Other is a catch-all for anything that doesn't match the known OutputTypes */
50-    std::vector<COutput> other;
51+    std::map<OutputType, std::vector<COutput>> coins;

LarryRuane commented at 3:02 am on August 16, 2022:

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.

josibake commented at 11:06 am on August 17, 2022:

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

LarryRuane commented at 3:05 am on August 16, 2022: contributor

Concept, code review ACK 8cd21bb2799d37ed00dc9d0490bb5f5f1375932b

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

in src/wallet/spend.h:38 in db09aec937 outdated

33@@ -34,26 +34,18 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* walle
34  * This struct is really just a wrapper around OutputType vectors with a convenient
35  * method for concatenating and returning all COutputs as one vector.
36  *
37- * Clear(), Size() methods are implemented so that one can interact with
38- * the CoinsResult struct as if it was a vector
39+ * Size(), Clear(), Erase(), Shuffle(), and Add() methods are implemented to
40+ * allow easy interaction with the struct.

furszy commented at 1:36 pm on August 16, 2022:

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

josibake commented at 11:07 am on August 17, 2022:

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

furszy approved

furszy commented at 1:44 pm on August 16, 2022: member

utACK 8cd21bb2. 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.

in src/wallet/spend.cpp:127 in 8cd21bb279

137+}
138+
139+void CoinsResult::Add(OutputType type, const COutput& out)
140+{
141+    coins[type].emplace_back(out);
142+}

jonatack commented at 2:11 pm on August 16, 2022:

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:

0void 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

josibake commented at 11:09 am on August 17, 2022:

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

jonatack commented at 11:29 am on August 17, 2022:

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 commented at 0:00 am on August 17, 2022: member

ACK 8cd21bb2799d37ed00dc9d0490bb5f5f1375932b

achow101 merged this on Aug 17, 2022

achow101 closed this on Aug 17, 2022

in src/outputtype.h:30 in 8cd21bb279

25 static constexpr auto OUTPUT_TYPES = std::array{
26     OutputType::LEGACY,
27     OutputType::P2SH_SEGWIT,
28     OutputType::BECH32,
29     OutputType::BECH32M,
30+    OutputType::UNKNOWN,

maflcko commented at 7:04 pm on August 18, 2022:

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

furszy commented at 7:49 pm on August 18, 2022:

Straightforward #25869

achow101 referenced this in commit 0f0508bc72 on Aug 19, 2022

bitcoin locked this on Aug 18, 2023

josibake deleted the branch on Jan 26, 2024

wallet, refactor: #24584 follow-ups #25734

Conflicts