rpc, wallet: add ability to retrieve all address book entries #26174

pull w0xlt wants to merge 1 commits into bitcoin:master from w0xlt:list_address_book changing 1 files +14 −6

w0xlt commented at 1:38 PM on September 24, 2022: contributor

As far as I know, there is currently no way to retrieve all address book entries via RPC. This functionality can be added by slightly modifying the getaddressesbylabel and making the label parameter optional.

PR:

./src/bitcoin-cli -regtest getaddressesbylabel
{
  "bcrt1q23d9qmeqrmdfazp79e5xavpgrzqmmtjf7k2p7u": {
    "label": "label03",
    "purpose": "receive"
  },
  "bcrt1pynqgl3mh8qmwj496nfg6ucxd8jw0wz3rz7jtf4s4v6e9zv6a4cnqdcl79k": {
    "label": "label02",
    "purpose": "receive"
  },
  "bcrt1pty99hxsx7trd53ul7lhl4ee82ggqslyq54mg5mte5wv8nfu7xxfqcf0pzz": {
    "label": "label01",
    "purpose": "receive"
  }
}

On master branch, it is not possible to run that RPC without passing a label parameter.

rpc, wallet: add ability to retrieve all address book entries 37f809aad5
in src/wallet/rpc/addresses.cpp:641 in 32a9f345c0 outdated
```
 639 | @@ -640,14 +640,15 @@ RPCHelpMan getaddressesbylabel()
 640 |      return RPCHelpMan{"getaddressesbylabel",
 641 |                  "\nReturns the list of addresses assigned the specified label.\n",
```
jarolrod commented at 5:31 PM on September 24, 2022:

If you want to do this, you should mention that if no label is supplied then all address book entry addresses are returned

w0xlt commented at 7:07 PM on September 24, 2022:

Done in 37f809aad549368911b8358c1c6e6cb0e8c5bc7e. Thanks.
w0xlt force-pushed on Sep 24, 2022
luke-jr commented at 12:02 AM on September 25, 2022: member

Concept ~0. Seems like a hack.

Code looks fine though.
aureleoules commented at 7:46 AM on September 26, 2022: member

Concept ACK but not sure about the approach. Would it be better to create a new RPC getaddresses with filters such as label, purpose, creation_date, watch_only, minconf, etc? Which would deprecate getaddressesbylabel.
glozow added the label Wallet on Sep 26, 2022
DrahtBot commented at 3:12 PM on September 28, 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

Concept ACK aureleoules, rajarshimaitra, 1440000bytes

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #26186 (rpc: Sanitize label name in various RPCs with tests by aureleoules)
- #24897 ([Draft / POC] Silent Payments by w0xlt)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
aureleoules commented at 10:34 AM on October 19, 2022: member

Another approach would be to take a label * to list all addresses. This is a hack but it's already used for backwards compatibility in listtransactions.
DrahtBot added the label Needs rebase on Jan 10, 2023
DrahtBot commented at 11:01 PM on January 10, 2023: contributor



🐙 This pull request conflicts with the target branch and needs rebase.
rajarshimaitra commented at 1:51 PM on March 27, 2023: contributor

Concept ACK. This is useful for various situations where a bunch of imported spks needs to be retrieved from the wallet. Maybe to compare with an external source and check if any new spks need to be imported into the watchlist. Previously it could be done by making redundant labels just for this call, but having this option to dump everything seems useful.

I will go through the code changes in more detail. Adding one simple, functional test to check the behavior will be helpful.
ghost commented at 3:14 PM on March 27, 2023: none

Concept ACK
DrahtBot commented at 1:18 AM on June 25, 2023: contributor


There hasn't been much activity lately and the patch still needs rebase. What is the status here?
- Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
- Is it no longer relevant? ➡️ Please close.
- Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
achow101 commented at 4:01 PM on September 20, 2023: member

The PR didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open PRs.

Closing due to lack of interest.
achow101 closed this on Sep 20, 2023
bitcoin locked this on Sep 19, 2024