Wallet: “listreceivedby*” fix #30972

1. 
   BrandonOdiwuor commented at 4:59 pm on September 25, 2024: contributor

   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

2. 
   DrahtBot commented at 4:59 pm on September 25, 2024: contributor

   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. A summary of reviews will appear here.

   Conflicts

   No conflicts as of last run.

3. BrandonOdiwuor marked this as a draft on Sep 25, 2024
4. DrahtBot added the label CI failed on Sep 25, 2024
5. 
   DrahtBot commented at 5:24 pm on September 25, 2024: contributor

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

6. wallet: 'Filter' out 'send' addresses from 'listreceivedby*' 1ebbc00fd8
7. test: split 'listreceivedby' tests in subtests

   Co-authored-by: Andreas Kouloumos <kouloumosa@gmail.com>

   c4968d5147
8. test: 'listreceivedby*' does not return send address

   Co-authored-by: Andreas Kouloumos <kouloumosa@gmail.com>

   a35115f17c
9. BrandonOdiwuor force-pushed on Sep 26, 2024
10. BrandonOdiwuor renamed this:
    Wallet listreceivedby fix
    Wallet: "listreceivedby*" fix
    on Sep 26, 2024
11. BrandonOdiwuor marked this as ready for review on Sep 26, 2024
12. DrahtBot removed the label CI failed on Sep 26, 2024
13. DrahtBot added the label Wallet on Sep 26, 2024
14. 
    polespinasa commented at 7:27 pm on November 13, 2024: none

    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.

15. 
    fanquake commented at 3:31 pm on February 20, 2025: member

    Maybe @achow101 or @furszy want to review here?
16. in src/wallet/rpc/transactions.cpp:146 in 1ebbc00fd8 outdated

    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+
    

    ---

    ---

    

    furszy commented at 5:17 pm on March 11, 2025:

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

Contributors
BrandonOdiwuor DrahtBot polespinasa fanquake furszy

Labels
Wallet