Wallet: "listreceivedby*" fix

[BrandonOdiwuor]

Fixes #16159,

This PR builds on #25973, fixing listreceivedby* RPCs by filtering out send addresses using IsMine (see #25973 (comment)). It also breaks down the listreceivedby tests into subtests and adds a test to verify 'listreceivedby*' does not return send addresses

[DrahtBot]

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30972.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	achow101, rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

• “changes addresses” -> “change addresses” [“change addresses” is the correct term for output addresses, “changes” is a verb here and incorrect]

^{drahtbot_id_4_m}

[DrahtBot]

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/30659059850}

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being
  incompatible with the current code in the target branch). If so, make sure to rebase on the latest
  commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the
  affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

[polespinasa]

Lgtm
Tested ack a35115f

I ran the unit test and functional test (without bdb) and all pass.
Also tested manually using regtest the steps in function test_listreceivedby and the behaviour is the expected.

[fanquake]

Maybe @achow101 or @furszy want to review here?

[furszy]

Since the mapTally loading procedure already performs the IsMine check, we could use that instead of executing it again for the addresses that received some coins.
In other words, if an element exists in mapTally, we can be certain that everything inside it belongs to the wallet.

So, ideally, we could decouple the mapTally existence check from the "include empty" check (the one that is just below this line), which would avoid this second IsMine() call for the non-empty addresses (we would need to execute it for the addresses that have no associated value in mapTally).

[BrandonOdiwuor]

I have updated the code to only do the second IsMine check if the address is not in mapTally

[achow101]

Concept ACK

[BrandonOdiwuor]

Rebased and updated to address #30972 (comment)

[rkrux]

Concept ACK 486cd47

IMO it'd be a good idea to rebase this branch over master so that the changes of #32523 PR are incorporated that removes the outdated ismine enum which is seen quite a lot in this branch currently while reviewing.

[rkrux]

In a1d90fb "wallet: 'Filter' out 'send' addresses from 'listreceivedby*'"

Instead of trying to lock again, assert that the function is already holding the lock? I checked that both the callers of the ListReceived function are acquiring the required lock.

--- a/src/wallet/rpc/transactions.cpp
+++ b/src/wallet/rpc/transactions.cpp
@@ -73,6 +73,8 @@ struct tallyitem
 
 static UniValue ListReceived(const CWallet& wallet, const UniValue& params, const bool by_label, const bool include_immature_coinbase) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
 {
+    AssertLockHeld(wallet.cs_wallet);
+
     // Minimum confirmations
     int nMinDepth = 1;
     if (!params[0].isNull())
@@ -131,14 +133,13 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons
     UniValue ret(UniValue::VARR);
     std::map<std::string, tallyitem> label_tally;
 
-    const auto& func = [&](const CTxDestination& address, const std::string& label, bool is_change, const std::optional<AddressPurpose>& purpose) {
+    const auto& func = [&](const CTxDestination& address, const std::string& label, bool is_change, const std::optional<AddressPurpose>& purpose) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) {
         if (is_change) return; // no change addresses
 
         auto it = mapTally.find(address);
         if (it == mapTally.end() && !fIncludeEmpty) {
             return;
         } else if (it == mapTally.end()) {
-            LOCK(wallet.cs_wallet);
             if (!wallet.IsMine(address)) return; // no send addresses
         }
 

[rkrux]

In a1d90fb "wallet: 'Filter' out 'send' addresses from 'listreceivedby*'"

For making it concise.

--- a/src/wallet/rpc/transactions.cpp
+++ b/src/wallet/rpc/transactions.cpp
@@ -73,6 +73,8 @@ struct tallyitem
         auto it = mapTally.find(address);
-        if (it == mapTally.end() && !fIncludeEmpty) {
-            return;
-        } else if (it == mapTally.end()) {
-            LOCK(wallet.cs_wallet);
+        if (it == mapTally.end()) {
+            if (!fIncludeEmpty) return;
             if (!wallet.IsMine(address)) return; // no send addresses
         }

[rkrux]

In 486cd47 "test: 'listreceivedby*' does not return send address"

Nit (feel free to ignore): For expressiveness.

--- a/test/functional/wallet_listreceivedby.py
+++ b/test/functional/wallet_listreceivedby.py
@@ -119,12 +119,12 @@ class ReceivedByTest(BitcoinTestFramework):
 
         expected = {"address": address, "label": label, "amount": Decimal("0.1"), "confirmations": 0, "txids": [txid, ]}
         assert_array_result(self.nodes[0].listreceivedbyaddress(minconf=0, include_empty=True), {"address": address}, expected)
-        assert_array_result(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True), {"address": address}, {}, True)
+        assert_array_result(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True), {"address": address}, {}, should_not_find=True)
 
         self.log.info("Tests listreceivedbylabel does not return label of address with 'send' purpose")
         expected = {"label": label, "amount": Decimal("0.1")}
         assert_array_result(self.nodes[0].listreceivedbylabel(minconf=0, include_empty=True), {"label": label}, expected)
-        assert_array_result(self.nodes[1].listreceivedbylabel(minconf=0, include_empty=True), {"label": label}, {}, True)
+        assert_array_result(self.nodes[1].listreceivedbylabel(minconf=0, include_empty=True), {"label": label}, {}, should_not_find=True)
 

[rkrux]

In 486cd47 "test: 'listreceivedby*' does not return send address"

- self.nodes[1].setlabel(address, label) # address has now assigned a "send" purpose on node1
+ self.nodes[1].setlabel(address, label) # address has now been assigned a "send" purpose in node1's wallet

[maflcko]

“changes addresses” -> “change addresses” [“change addresses” is the correct term for output addresses, “changes” is a verb here and incorrect]