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

furszy · 2022-05-12T21:34:52Z

Built on top of #23662, coming from comment #23662 (review).

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

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.

furszy · 2022-05-13T14:30:05Z

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

DrahtBot · 2022-05-13T17:02:12Z

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

Conflicts

No conflicts as of last run.

achow101 · 2022-05-18T17:36:40Z

ACK 73aea8c65e4810f3e41c8351f80ae43e276fc1d5

kristapsk · 2022-05-18T18:06:04Z

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

furszy · 2022-05-19T14:39:03Z

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

pushed 👍🏼.

achow101 · 2022-05-19T16:37:19Z

re-ACK 77915c1c21b7041c8e8ab76760bfd7e85e18fbab

theStack

Code-review ACK 77915c1c21b7041c8e8ab76760bfd7e85e18fbab

doc/release-notes.md

furszy · 2022-05-20T13:21:59Z

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

re-ACK 8a722974b2b9e54fcc0139dd795023a0f8ef3f6c

jonatack

Approach ACK

doc/release-notes.md

src/wallet/rpc/coins.cpp

test/functional/wallet_listreceivedby.py

jonatack · 2022-05-20T15:25:28Z

src/wallet/rpc/coins.cpp

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

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

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

furszy · 2022-05-20T19:34:52Z

Updated per feedback 👍🏼.

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

jonatack · 2022-05-20T20:43:10Z

ACK da8be104cdaa2bef955bfea2909ce75af9992a2b

kristapsk · 2022-05-20T20:47:08Z

Squash commits into one?

jonatack · 2022-05-20T20:54:53Z

doc/release-notes.md

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

…if the label is not in the address book.

theStack

re-ACK baa3ddc

w0xlt

ACK baa3ddc

achow101 · 2022-05-23T16:07:31Z

ACK baa3ddc

jonatack · 2022-05-23T18:34:30Z

Posthumous ACK

…esses were found in the address book baa3ddc doc: add release notes about `getreceivedbylabel` returning an error if the label is not in the address book. (furszy) 8897a21 rpc: getreceivedbylabel, don't loop over the entire wallet txs map if no destinations were found for the input label. (furszy) Pull request description: Built on top of bitcoin#23662, coming from comment bitcoin#23662 (review). 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). ACKs for top commit: achow101: ACK baa3ddc theStack: re-ACK baa3ddc w0xlt: ACK bitcoin@baa3ddc Tree-SHA512: 00e10365b179bf008da2f3ef8fbb3ee04a330426374020e3f2d0151b16991baba4ef2b944e4659452f3e4d6cb20f128d0918ddf0453933a25a4d9fd8414a1911

furszy mentioned this pull request May 12, 2022

rpc: improve getreceivedby{address,label} performance #23662

Merged

theStack reviewed May 12, 2022

View reviewed changes

DrahtBot added RPC/REST/ZMQ Wallet labels May 12, 2022

fjahr mentioned this pull request May 12, 2022

test: Fix race condition in index prune test #25123

Merged

furszy force-pushed the 2022_RPC_getreceivedbylabel_return_early branch from a208ddf to 6482941 Compare May 13, 2022 12:21

DrahtBot added the Needs rebase label May 16, 2022

furszy force-pushed the 2022_RPC_getreceivedbylabel_return_early branch from 6482941 to 73aea8c Compare May 17, 2022 12:45

DrahtBot removed the Needs rebase label May 17, 2022

maflcko changed the title ~~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 May 18, 2022

theStack approved these changes May 19, 2022

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

furszy force-pushed the 2022_RPC_getreceivedbylabel_return_early branch from 77915c1 to 8a72297 Compare May 20, 2022 13:21

theStack approved these changes May 20, 2022

View reviewed changes

jonatack reviewed May 20, 2022

View reviewed changes

furszy force-pushed the 2022_RPC_getreceivedbylabel_return_early branch from 8a72297 to a13b8a0 Compare May 20, 2022 19:28

furszy force-pushed the 2022_RPC_getreceivedbylabel_return_early branch from a13b8a0 to da8be10 Compare May 20, 2022 19:32

jonatack reviewed May 20, 2022

View reviewed changes

doc: add release notes about getreceivedbylabel returning an error …

baa3ddc

…if the label is not in the address book.

furszy force-pushed the 2022_RPC_getreceivedbylabel_return_early branch from da8be10 to baa3ddc Compare May 21, 2022 02:24

theStack approved these changes May 23, 2022

View reviewed changes

w0xlt approved these changes May 23, 2022

View reviewed changes

achow101 merged commit 5ebff43 into bitcoin:master May 23, 2022

bitcoin locked and limited conversation to collaborators May 23, 2023

furszy deleted the 2022_RPC_getreceivedbylabel_return_early branch May 27, 2023 01:51

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

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

Uh oh!

Conversation

furszy commented May 12, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

furszy commented May 13, 2022

Uh oh!

DrahtBot commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

achow101 commented May 18, 2022

Uh oh!

kristapsk commented May 18, 2022

Uh oh!

furszy commented May 19, 2022

Uh oh!

achow101 commented May 19, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented May 20, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack May 20, 2022

Choose a reason for hiding this comment

Uh oh!

furszy May 20, 2022

Choose a reason for hiding this comment

Uh oh!

furszy commented May 20, 2022

Uh oh!

jonatack commented May 20, 2022

Uh oh!

kristapsk commented May 20, 2022

Uh oh!

jonatack May 20, 2022

Choose a reason for hiding this comment

Uh oh!

furszy May 21, 2022

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented May 23, 2022

Uh oh!

jonatack commented May 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented May 13, 2022 •

edited

Loading