Fixes #16159,
This PR builds on #25973, fixing listreceivedby* RPCs by filtering out send addresses using IsMine (see #25973 (review)). It also breaks down the listreceivedby tests into subtests and adds a test to verify ’listreceivedby*’ does not return send addresses
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30972.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, 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.
Possible typos and grammar issues:
drahtbot_id_4_m
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30659059850
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.
Lgtm Tested ack a35115f17caf550326e12ff3044fe617a1a53fd3
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.
140@@ -141,6 +141,9 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons
141 const auto& func = [&](const CTxDestination& address, const std::string& label, bool is_change, const std::optional<AddressPurpose>& purpose) {
142 if (is_change) return; // no change addresses
143
144+ LOCK(wallet.cs_wallet);
145+ if (!wallet.IsMine(address)) return; // no send addresses
146+
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).
IsMine check if the address is not in mapTally
Co-authored-by: Andreas Kouloumos <kouloumosa@gmail.com>
Co-authored-by: Andreas Kouloumos <kouloumosa@gmail.com>
134@@ -135,8 +135,12 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons
135 if (is_change) return; // no change addresses
136
137 auto it = mapTally.find(address);
138- if (it == mapTally.end() && !fIncludeEmpty)
139+ if (it == mapTally.end() && !fIncludeEmpty) {
140 return;
141+ } else if (it == mapTally.end()) {
142+ LOCK(wallet.cs_wallet);
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.
0--- a/src/wallet/rpc/transactions.cpp
1+++ b/src/wallet/rpc/transactions.cpp
2@@ -73,6 +73,8 @@ struct tallyitem
3
4 static UniValue ListReceived(const CWallet& wallet, const UniValue& params, const bool by_label, const bool include_immature_coinbase) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
5 {
6+ AssertLockHeld(wallet.cs_wallet);
7+
8 // Minimum confirmations
9 int nMinDepth = 1;
10 if (!params[0].isNull())
11@@ -131,14 +133,13 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons
12 UniValue ret(UniValue::VARR);
13 std::map<std::string, tallyitem> label_tally;
14
15- const auto& func = [&](const CTxDestination& address, const std::string& label, bool is_change, const std::optional<AddressPurpose>& purpose) {
16+ const auto& func = [&](const CTxDestination& address, const std::string& label, bool is_change, const std::optional<AddressPurpose>& purpose) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) {
17 if (is_change) return; // no change addresses
18
19 auto it = mapTally.find(address);
20 if (it == mapTally.end() && !fIncludeEmpty) {
21 return;
22 } else if (it == mapTally.end()) {
23- LOCK(wallet.cs_wallet);
24 if (!wallet.IsMine(address)) return; // no send addresses
25 }
26
134@@ -135,8 +135,12 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons
135 if (is_change) return; // no change addresses
136
137 auto it = mapTally.find(address);
138- if (it == mapTally.end() && !fIncludeEmpty)
139+ if (it == mapTally.end() && !fIncludeEmpty) {
In https://github.com/bitcoin/bitcoin/commit/a1d90fb3b1baefeffb59cf00bad4be3268040ce5 “wallet: ‘Filter’ out ‘send’ addresses from ’listreceivedby*’”
For making it concise.
0--- a/src/wallet/rpc/transactions.cpp
1+++ b/src/wallet/rpc/transactions.cpp
2@@ -73,6 +73,8 @@ struct tallyitem
3 auto it = mapTally.find(address);
4- if (it == mapTally.end() && !fIncludeEmpty) {
5- return;
6- } else if (it == mapTally.end()) {
7- LOCK(wallet.cs_wallet);
8+ if (it == mapTally.end()) {
9+ if (!fIncludeEmpty) return;
10 if (!wallet.IsMine(address)) return; // no send addresses
11 }
122+ assert_array_result(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True), {"address": address}, {}, True)
123+
124+ self.log.info("Tests listreceivedbylabel does not return label of address with 'send' purpose")
125+ expected = {"label": label, "amount": Decimal("0.1")}
126+ assert_array_result(self.nodes[0].listreceivedbylabel(minconf=0, include_empty=True), {"label": label}, expected)
127+ assert_array_result(self.nodes[1].listreceivedbylabel(minconf=0, include_empty=True), {"label": label}, {}, True)
In 486cd4779909c2403a308b05f537db276e780f2a “test: ’listreceivedby*’ does not return send address”
Nit (feel free to ignore): For expressiveness.
0--- a/test/functional/wallet_listreceivedby.py
1+++ b/test/functional/wallet_listreceivedby.py
2@@ -119,12 +119,12 @@ class ReceivedByTest(BitcoinTestFramework):
3
4 expected = {"address": address, "label": label, "amount": Decimal("0.1"), "confirmations": 0, "txids": [txid, ]}
5 assert_array_result(self.nodes[0].listreceivedbyaddress(minconf=0, include_empty=True), {"address": address}, expected)
6- assert_array_result(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True), {"address": address}, {}, True)
7+ assert_array_result(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True), {"address": address}, {}, should_not_find=True)
8
9 self.log.info("Tests listreceivedbylabel does not return label of address with 'send' purpose")
10 expected = {"label": label, "amount": Decimal("0.1")}
11 assert_array_result(self.nodes[0].listreceivedbylabel(minconf=0, include_empty=True), {"label": label}, expected)
12- assert_array_result(self.nodes[1].listreceivedbylabel(minconf=0, include_empty=True), {"label": label}, {}, True)
13+ assert_array_result(self.nodes[1].listreceivedbylabel(minconf=0, include_empty=True), {"label": label}, {}, should_not_find=True)
14
104@@ -104,12 +105,33 @@ def test_listreceivedbyaddress(self):
105 res = self.nodes[1].listreceivedbyaddress(0, True, True, other_addr)
106 assert_equal(len(res), 0)
107
108+ def test_listreceivedby(self):
109+ self.log.info("Tests listreceivedbyaddress does not return address with 'send' purpose")
110+ label = "node0address"
111+ address = self.nodes[0].getnewaddress(label)
112+ # using setlabel for an address that does not belong to the wallet assigns a "send" purpose to that address
113+ self.nodes[1].setlabel(address, label) # address has now assigned a "send" purpose on node1
In https://github.com/bitcoin/bitcoin/commit/486cd4779909c2403a308b05f537db276e780f2a “test: ’listreceivedby*’ does not return send address”
0- self.nodes[1].setlabel(address, label) # address has now assigned a "send" purpose on node1
1+ self.nodes[1].setlabel(address, label) # address has now been assigned a "send" purpose in node1's wallet
Concept ACK 486cd4779909c2403a308b05f537db276e780f2a
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.
74- {"address": empty_addr, "label": "", "amount": 0, "confirmations": 0, "txids": []})
75+ expected_tx = {"address": empty_addr, "label": "", "amount": 0, "confirmations": 0, "txids": []}
76+ assert_array_result(self.nodes[1].listreceivedbyaddress(0, True), {"address": empty_addr}, expected_tx)
77
78- # No returned addy should be a change addr
79+ self.log.info("- does not return changes addresses")
“changes addresses” -> “change addresses” [“change addresses” is the correct term for output addresses, “changes” is a verb here and incorrect]