wallet: Filter-out "send" addresses from `listreceivedby*`

kouloumos commented at 3:15 pm on September 1, 2022: contributor

listreceivedbyaddress & listreceivedbylabel RPCs list balances by receiving address and label respectively. That works by creating a tally (mapTally[address]) from all the transaction outputs in the wallet and then cross-reference that with the address book in order to calculate which addresses received funds.

If an address is in the address book but not in the tally, it’s considered empty (never received funds) and is not returned unless include_empty=true. That effectively means that when include_empty=true the entire address book is included in the result.

This creates 2 issues:

As reported in #16159, the address book also includes addresses with a “send” purpose which should not be part of the response.
As reported in #10468, if include_watchonly=false the watchonly addresses are not included in mapTally leading to them being considered empty during the subsequent cross-reference with the address book thus becoming part of the result when include_empty=true.

This PR fixes #16159 by filtering-out “send” addresses, see #16159 (comment) for more details on the issue. I haven’t thought of a nice solution to fix (2) yet.

Edit: Split wallet_listreceivedby.py tests for code maintainability as tests for more than one RPC have been added since the creation of the file. Added test coverage for the introduced change.

wallet: Filter-out "send" addresses from `listreceivedby*` ce7fc73657

kouloumos force-pushed on Sep 1, 2022

DrahtBot added the label Wallet on Sep 1, 2022

test: split listreceivedby test into subtests 527154afe8

test: add coverage for `listreceivedby*` no "send" purpose addrs return 62880a9bb1

kouloumos force-pushed on Sep 2, 2022

aureleoules approved

aureleoules commented at 4:58 pm on September 20, 2022: member

ACK 62880a9bb167bc0569175d597a865e6baccce71b. I verified that the commit ce7fc736576664701b09bea811d77113948c38bb fixes the tests in 62880a9bb167bc0569175d597a865e6baccce71b. 527154afe8dcdc7d0a788e00ad9fc26f0268ad1c looks good to me, only a refactor.

DrahtBot commented at 4:30 am on September 23, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	aureleoules, achow101, ryanofsky

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.

in test/functional/wallet_listreceivedby.py:107 in 62880a9bb1

103@@ -103,6 +104,27 @@ def test_listreceivedbyaddress(self):
104         res = self.nodes[1].listreceivedbyaddress(0, True, True, other_addr)
105         assert_equal(len(res), 0)
106 
107+    def test_listreceivedby(self):

achow101 commented at 9:03 pm on November 21, 2022:

In 62880a9bb167bc0569175d597a865e6baccce71b “test: add coverage for listreceivedby* no “send” purpose addrs return”

nit: s/test_listreceivedby/test_listreceivedby_excludes_send

furszy commented at 9:27 pm on January 8, 2023: member

listreceivedbyaddress & listreceivedbylabel RPCs list balances by receiving address and label respectively. That works by creating a tally (mapTally[address]) from all the transaction outputs in the wallet and then cross-reference that with the address book in order to calculate which addresses received funds.

Why do we need to cross-reference with the address book if we already traversed the whole wallet’s transactions outputs and filtered-out all the ones that are not belonging to the wallet?

Seems that we could directly use the mapTally information without looping around the address book. Only would need to implement a CWallet::GetAddrLabel(const CTxDestination& dest) to obtain the label whenever is needed.

davidgumberg commented at 6:54 pm on January 18, 2023: contributor

Is it possible for an address that has been received from to get marked as a “send” address and subsequently not get returned by listreceived*?

willcl-ark commented at 10:39 am on January 23, 2023: member

@kouloumos would #26813 also be closed by this?

furszy commented at 12:44 pm on January 23, 2023: member

@willcl-ark no, possibly by #25979 (or any other attempt to improve/standardize the change detection)

achow101 commented at 6:14 pm on March 9, 2023: member

ACK 62880a9bb167bc0569175d597a865e6baccce71b

achow101 commented at 6:37 pm on May 3, 2023: member

Why do we need to cross-reference with the address book if we already traversed the whole wallet’s transactions outputs and filtered-out all the ones that are not belonging to the wallet?

To lookup the label.

There is a silent merge conflict with master.

in src/wallet/rpc/transactions.cpp:140 in ce7fc73657 outdated

136@@ -137,6 +137,8 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons
137     const auto& func = [&](const CTxDestination& address, const std::string& label, const std::string& purpose, bool is_change) {
138         if (is_change) return; // no change addresses
139 
140+        if (purpose == "send") return; // no send addresses

ryanofsky commented at 1:40 pm on July 20, 2023:

In commit “wallet: Filter-out “send” addresses from listreceivedby*” (ce7fc736576664701b09bea811d77113948c38bb)

It like it would be better to call IsMine here instead of relying on the purpose field. The PR description says this fix only covers the include_empty=true, include_watchonly=true case, not the include_watchonly=false case, so maybe you could fix both cases by using the isminetype result returned by IsMine instead of purpose == “send”.

Also, I’m not 100% sure about this, and am curious about other opinions, but in general the purpose field seems like it is set pretty haphazardly in code, so things that rely on it could easily break, and probably better not to use it when alternatives are available.

ryanofsky approved

ryanofsky commented at 1:44 pm on July 20, 2023: contributor

Code review ACK 62880a9bb167bc0569175d597a865e6baccce71b, but this needs to be rebased due to a silent conflict with #27217. This seems like a helpful fix and the test cleanup is also nice.

Another suggestion if you feel like it would be to add the new test before making the code change and have it verify the previous behavior. Then fix the bug and update the test in the same commit, so it is easier to see how behavior changes in the diff.

achow101 commented at 7:27 pm on August 24, 2023: member

Are you still working on this?

achow101 added the label Waiting for author on Sep 1, 2023

achow101 added the label Needs rebase on Sep 5, 2023

DrahtBot removed the label Needs rebase on Sep 5, 2023

achow101 commented at 3:59 pm on September 20, 2023: member

Closing as up for grabs due to lack of activity.

achow101 closed this on Sep 20, 2023

achow101 added the label Up for grabs on Sep 20, 2023

bitcoin locked this on Sep 19, 2024

fanquake removed the label Up for grabs on Oct 2, 2024

fanquake commented at 10:33 am on October 2, 2024: member

Some of this was picked up in #30972.

maflcko removed the label Waiting for author on Oct 2, 2024

wallet: Filter-out “send” addresses from listreceivedby* #25973

Reviews

Conflicts

wallet: Filter-out “send” addresses from `listreceivedby*` #25973