Wallet: "listreceivedby*" fix #30972

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).

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

In a1d90fb3b1baefeffb59cf00bad4be3268040ce5 "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
         }
 

In https://github.com/bitcoin/bitcoin/commit/a1d90fb3b1baefeffb59cf00bad4be3268040ce5 "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
         }

In 486cd4779909c2403a308b05f537db276e780f2a "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)
 

In https://github.com/bitcoin/bitcoin/commit/486cd4779909c2403a308b05f537db276e780f2a "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

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