rpc: getreceivedbylabel, return early if no addresses were found in the address book #25122

furszy commented at 9:34 PM on May 12, 2022: member

Built on top of #23662, coming from comment #23662#pullrequestreview-971407999.

If wallet.GetLabelAddresses() returns an empty vector (the wallet does not have stored destinations with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away. Otherwise, we are walking through all the wallet txs + outputs for no reason (output_scripts is empty).

theStack commented at 10:53 PM on May 12, 2022: member

Concept ACK

Thanks for following up! As expressed in #23662 (comment), I think users should be notified with an error if a label is passed that doesn't exist in the wallet address book. Not sure yet if it's worth the effort, but one could even go one step further and throw two different error messages for the cases

there is no such label in the address book (i.e. wallet.GetLabelAddresses returns an empty set)
there is a label, but it doesn't apply to any address that is ours (i.e. after IsMine filtering, output_scripts is empty)

As of the current state of the PR, I think the simplest (minimal-diff) solution would be to move that part

    if (output_scripts.empty()) {
        throw JSONRPCError(RPC_WALLET_ERROR, "Address/es not found in wallet");
    }

to the end of the if (by_label) { ... } block. Then you could also be more specific with the message and say, e.g. "Label not found in wallet" (as you outlined in #23662 (comment)). Note also that the CI indicates that the functional test wallet_listreceivedby.py needs to be adapted.

DrahtBot added the label RPC/REST/ZMQ on May 12, 2022

DrahtBot added the label Wallet on May 12, 2022

furszy force-pushed on May 13, 2022

furszy commented at 2:30 PM on May 13, 2022: member

yep pushed. Tests passing now. Will rebase it once your PR gets merged.

DrahtBot commented at 5:02 PM on May 13, 2022: member

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on May 16, 2022

furszy force-pushed on May 17, 2022

DrahtBot removed the label Needs rebase on May 17, 2022

achow101 commented at 5:36 PM on May 18, 2022: member

ACK 73aea8c65e4810f3e41c8351f80ae43e276fc1d5

MarcoFalke renamed this:
~~rpc: getreceivedbylabel, return early if no addresses were found in the address book.~~
rpc: getreceivedbylabel, return early if no addresses were found in the address book
on May 18, 2022

kristapsk commented at 6:06 PM on May 18, 2022: contributor

This needs release notes, as API is changed for cases when specified label does not exist in wallet.

furszy commented at 2:39 PM on May 19, 2022: member

This needs release notes, as API is changed for cases when specified label does not exist in wallet.

pushed 👍🏼.

achow101 commented at 4:37 PM on May 19, 2022: member

re-ACK 77915c1c21b7041c8e8ab76760bfd7e85e18fbab

in doc/release-notes.md:61 in 77915c1c21 outdated

  56 | @@ -57,6 +57,9 @@ Updated RPCs
  57 |  
  58 |  Changes to wallet related RPCs can be found in the Wallet section below.
  59 |  
  60 | +- The `getreceivedbylabel` command will now return an error "Label not found
  61 | +  in wallet" if the label is not in the address book.

theStack commented at 5:33 PM on May 19, 2022:

nit: IIRC, we usually describe the concrete error code in the release notes for RPC changes (also in general, the PR number in parantheses).

  in wallet" (-4) if the label is not in the address book. (#25122)

furszy commented at 1:19 PM on May 20, 2022:

ah, didn't know it. One sec, pushing it..

theStack approved

theStack commented at 5:34 PM on May 19, 2022: member

Code-review ACK 77915c1c21b7041c8e8ab76760bfd7e85e18fbab

furszy force-pushed on May 20, 2022

furszy commented at 1:21 PM on May 20, 2022: member

nit: IIRC, we usually describe the concrete error code in the release notes for RPC changes (also in general, the PR number in parantheses).

pushed 👍🏼.

theStack approved

theStack commented at 1:25 PM on May 20, 2022: member

re-ACK 8a722974b2b9e54fcc0139dd795023a0f8ef3f6c

in doc/release-notes.md:60 in 8a722974b2 outdated

  56 | @@ -57,6 +57,9 @@ Updated RPCs
  57 |  
  58 |  Changes to wallet related RPCs can be found in the Wallet section below.
  59 |  
  60 | +- The `getreceivedbylabel` command will now return an error "Label not found

jonatack commented at 2:54 PM on May 20, 2022:

s/command/RPC/
set off error message with commas
use present tense (consistency with the other notes)

-- The `getreceivedbylabel` command will now return an error "Label not found
-  in wallet" (-4) if the label is not in the address book. (#25122)
+- RPC `getreceivedbylabel` now returns an error, "Label not found
+  in wallet" (-4), if the label is not in the address book. (#25122)

even better, no need for the error message itself:

-- The `getreceivedbylabel` command will now return an error "Label not found
-  in wallet" (-4) if the label is not in the address book. (#25122)
+- RPC `getreceivedbylabel` now returns an error if the label is not in the
+  address book. (#25122)

in src/wallet/rpc/coins.cpp:29 in 8a722974b2 outdated

  39 | -            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address");
  40 | -        }
  41 | -        CScript script_pub_key = GetScriptForDestination(dest);
  42 | -        if (!wallet.IsMine(script_pub_key)) {
  43 | -            throw JSONRPCError(RPC_WALLET_ERROR, "Address not found in wallet");
  44 | +        if (!IsValidDestination(dest)) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address");

jonatack commented at 3:02 PM on May 20, 2022:

No need to change the existing bracket formatting for this conditional.

in test/functional/wallet_listreceivedby.py:122 in 8a722974b2 outdated

 118 | @@ -119,6 +119,9 @@ def run_test(self):
 119 |          # Trying to getreceivedby for an address the wallet doesn't own should return an error
 120 |          assert_raises_rpc_error(-4, "Address not found in wallet", self.nodes[0].getreceivedbyaddress, addr)
 121 |  
 122 | +        # Trying to getreceivedby for a label the wallet doesn't own should return an error

jonatack commented at 3:11 PM on May 20, 2022:

either move the test further down into a getreceivedbylabel section or change the comment to a log
s/getreceivedby/getreceivedbylabel/ (below a suggested change to the comment to be like the other logging output)

        self.log.info("getreceivedbylabel returns an error if the wallet doesn't own the label")

in src/wallet/rpc/coins.cpp:36 in 8a722974b2 outdated

  44 | +        if (!IsValidDestination(dest)) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address");
  45 | +        addresses.insert(dest);
  46 | +    }
  47 | +
  48 | +    // Filter by own scripts only
  49 | +    std::set<CScript> output_scripts;

jonatack commented at 3:25 PM on May 20, 2022:

Just a thought, and not necessarily in this PR, but by moving this line back to the top the entire section touched here can be bracket-scoped or extracted out to a function that returns std::set<CScript> output_scripts

furszy commented at 7:39 PM on May 20, 2022:

yep, I structured it do some further decoupling but didn't find a good reason to add it here.

jonatack commented at 3:25 PM on May 20, 2022: member

Approach ACK

furszy force-pushed on May 20, 2022

rpc: getreceivedbylabel, don't loop over the entire wallet txs map if no destinations were found for the input label.

If wallet.GetLabelAddresses() returns an empty vector (the wallet does not have addresses with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away.

8897a21658

furszy force-pushed on May 20, 2022

furszy commented at 7:34 PM on May 20, 2022: member

Updated per feedback 👍🏼.

Release-notes text.
Moved the test line inside the "getreceivedbylabel Test" section + reworded the comment.

jonatack commented at 8:43 PM on May 20, 2022: member

ACK da8be104cdaa2bef955bfea2909ce75af9992a2b

kristapsk commented at 8:47 PM on May 20, 2022: contributor

Squash commits into one?

doc: add release notes about `getreceivedbylabel` returning an error if the label is not in the address book. baa3ddc49c

in doc/release-notes.md:62 in da8be104cd outdated

  56 | @@ -57,6 +57,9 @@ Updated RPCs
  57 |  
  58 |  Changes to wallet related RPCs can be found in the Wallet section below.
  59 |  
  60 | +- RPC `getreceivedbylabel` now returns an error, "Label not found
  61 | +  in wallet" (-4), if the label is not in the address book. (#25122)
  62 | +

jonatack commented at 8:54 PM on May 20, 2022:

nit, just saw the release note might be in the wrong section

--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -57,9 +57,6 @@ Updated RPCs
 
 Changes to wallet related RPCs can be found in the Wallet section below.
 
-- RPC `getreceivedbylabel` now returns an error, "Label not found
-  in wallet" (-4), if the label is not in the address book. (#25122)
-
 New RPCs
 --------
 
@@ -81,6 +78,9 @@ Tools and Utilities
 Wallet
 ------
 
+- RPC `getreceivedbylabel` now returns an error, "Label not found
+  in wallet" (-4), if the label is not in the address book. (#25122)
+
 GUI changes
 -----------

furszy commented at 12:56 PM on May 21, 2022:

pushed

furszy force-pushed on May 21, 2022

theStack approved

theStack commented at 9:57 AM on May 23, 2022: member

re-ACK baa3ddc49c46d00e3e0de06e494656f0f00b0ee8

w0xlt approved

w0xlt commented at 1:21 PM on May 23, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/25122/commits/baa3ddc49c46d00e3e0de06e494656f0f00b0ee8

achow101 commented at 4:07 PM on May 23, 2022: member

ACK baa3ddc49c46d00e3e0de06e494656f0f00b0ee8

achow101 merged this on May 23, 2022

achow101 closed this on May 23, 2022

jonatack commented at 6:34 PM on May 23, 2022: member

Posthumous ACK

sidhujag referenced this in commit 99ee44cab2 on May 28, 2022

DrahtBot locked this on May 23, 2023

furszy deleted the branch on May 27, 2023