wallet: Filter-out "send" addresses from listreceivedby*
#25973
Conversation
44aa363 to
ce7fc73
Compare
0c0544d to
62880a9
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
| @@ -103,6 +104,27 @@ def test_listreceivedbyaddress(self): | |||
| res = self.nodes[1].listreceivedbyaddress(0, True, True, other_addr) | |||
| assert_equal(len(res), 0) | |||
|
|
|||
| def test_listreceivedby(self): | |||
There was a problem hiding this comment.
In 62880a9 "test: add coverage for listreceivedby* no "send" purpose addrs return"
nit: s/test_listreceivedby/test_listreceivedby_excludes_send
There was a problem hiding this comment.
listreceivedbyaddress&listreceivedbylabelRPCs 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.
|
Is it possible for an address that has been received from to get marked as a "send" address and subsequently not get returned by |
|
@kouloumos would #26813 also be closed by this? |
|
@willcl-ark no, possibly by #25979 (or any other attempt to improve/standardize the change detection) |
|
ACK 62880a9 |
To lookup the label. There is a silent merge conflict with master. |
There was a problem hiding this comment.
Code review ACK 62880a9, 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.
| @@ -137,6 +137,8 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons | |||
| const auto& func = [&](const CTxDestination& address, const std::string& label, const std::string& purpose, bool is_change) { | |||
| if (is_change) return; // no change addresses | |||
|
|
|||
| if (purpose == "send") return; // no send addresses | |||
There was a problem hiding this comment.
In commit "wallet: Filter-out "send" addresses from listreceivedby*" (ce7fc73)
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.
|
Are you still working on this? |
|
Closing as up for grabs due to lack of activity. |
|
Some of this was picked up in #30972. |
listreceivedbyaddress&listreceivedbylabelRPCs 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 wheninclude_empty=truethe entire address book is included in the result.This creates 2 issues:
include_watchonly=falsethe watchonly addresses are not included inmapTallyleading to them being considered empty during the subsequent cross-reference with the address book thus becoming part of the result wheninclude_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.pytests 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.