rpc, wallet: Add listaddresses RPC #23019

lsilva01 commented at 5:46 PM on September 17, 2021: contributor

This PR adds an address explorer RPC to Bitcoin Core, similar to "Address" tab of Electrum or Specter Desktop.

This allows the user to know any arbitrary range of external and internal addresses and can be used with legacy or descriptor wallet.

This same functionality can be used to build an "Address" tab in the GUI later.

For better visualization in manual tests, the reviewer can start the node with the keypool reduced to 2. (default is 1000) and create a new wallet.

./src/bitcoind -keypool=2
./src/bitcoin-cli -rpcwallet=<wallet_name> listaddresses
./src/bitcoin-cli -rpcwallet=<wallet_name> listaddresses "p2sh-segwit"
./test/functional/wallet_listaddresses.py --descriptors
./test/functional/wallet_listaddresses.py --legacy-wallet

DrahtBot added the label RPC/REST/ZMQ on Sep 17, 2021

DrahtBot added the label Wallet on Sep 17, 2021

lsilva01 force-pushed on Sep 17, 2021

DrahtBot commented at 1:47 AM on September 18, 2021: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23647 (Split rpcwallet into multiple smaller parts by meshcollider)
#23545 (scripted-diff: Use clang-tidy syntax for C++ named arguments [WIP; TOO MANY CONFLICTS] by MarcoFalke)
#23417 (wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101)
#22929 (wallet: Automatically add receiving destinations to the address book by S3RK)
#22341 (rpc: add getxpub by Sjors)
#19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)
#15719 (Wallet passive startup by ryanofsky)

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.

lsilva01 renamed this:
~~rpc: Add listaddresses RPC~~
rpc, wallet: Add listaddresses RPC
on Sep 20, 2021

in src/wallet/rpcwallet.cpp:311 in fdc4031e85 outdated

 253 | +                                {RPCResult::Type::NUM, "index", "The address position"},
 254 | +                                {RPCResult::Type::STR, "address", "The bitcoin address"},
 255 | +                                {RPCResult::Type::STR, "hdkeypath", "The BIP 32 HD path of the address"},
 256 | +                                {RPCResult::Type::STR, "descriptor", "The address descriptor"},
 257 | +                                {RPCResult::Type::BOOL, "internal", "The address is internal (change) or external (receive)"},
 258 | +                                {RPCResult::Type::NUM, "amount", "Available amount at address"},

luke-jr commented at 5:21 PM on September 20, 2021:

Addresses don't hold funds. They only receive, never send.

Instead, it should just tally the total received ever.

lsilva01 commented at 11:33 PM on September 21, 2021:

The code to count the total received is already available in the function. It can be done.

But address explorers in most software wallets, such as Electrum and Specter Desktop, only show the funds available to be spent at each address. I think the idea is to see how the UTXO values are distributed across the addresses (if there is any significant gap, for example).

luke-jr changes_requested

achow101 commented at 7:03 PM on September 24, 2021: member

Approach NACK

I started reviewing, but I have some fundamental issues with the approach taken. I like the general idea, but this specific implementation has issues.

First of all, why does this even need to take parameters? I don't see why a "starting index" or "ending index" need to be provided. We shouldn't need to derive addresses on the fly because they should already be in the wallet in some way. This really should use the address book, the keypool, and key metadata rather than re-deriving. Index just doesn't really make sense for what this RPC purports to do. This also ends up making assumptions about what addresses are receiving and change, depending on how they are derived. But that is not reliable, especially for wallets prior to the HD chain split. In fact, using this with such a wallet will result in an assertion failure, and that is just not acceptable. The correct thing is to use the wallet's built in change determining function(s) so that those that are listed as change actually correspond with the things that the wallet will see as change.

Likewise, why is this limited to just the default address type? A wallet will typically have addresses of address types that are not the default. These should be output as well.

Furthermore, this RPC appears to only work on HD wallets, which severely limits is usability. Many people do use non-HD wallets still, and I would expect something that just lists out the wallet's addresses is not limited to just HD wallets.

lsilva01 force-pushed on Sep 27, 2021

lsilva01 commented at 6:41 AM on September 27, 2021: contributor

@achow101 thanks for detailed review.

My original implementation used the keypool (for legacy wallets), m_wallet_descriptor (for descriptor wallets) and key metadata. Originally, the ScriptPubKeyMan::ListAddresses() methods also didn't call TopUp().

But the problem is the following case: if the user wants to know some address whose index is greater than the keypool total (such as 2395th), an error would be generated. Deriving addresses solved this issue. The listaddresses RPC would work regardless of keypool size.

Since the problem was in the approach, not in concept, I changed the code of this PR to the original version, that uses the keypool, m_wallet_descriptor and key metadata. So the way I've found to resolve the keypool limitation is the following code, but I'm not sure this is a good user experience.

if ((size_t) end_index >= setKeyPool->size()) {
    auto s_end_index = strprintf("%u", end_index);
    auto s_set_keypool_size = strprintf("%u", setKeyPool->size() - 1);
    auto error_message = "Error: end_index (" + s_end_index + ") is greater than last keypool item (" + s_set_keypool_size + ").";
    error = _(error_message.c_str());
    return false;
}

I didn't understand the reason to remove start_index and end_index. They work more like pagination. Without them, the command would return 2000 items (the default keypool size * 2). Removing them, what would be the other way to have a good visualization of the result?

I also added address type as parameter.

achow101 commented at 4:25 PM on September 27, 2021: member

But the problem is the following case: if the user wants to know some address whose index is greater than the keypool total (such as 2395th), an error would be generated. Deriving addresses solved this issue. The listaddresses RPC would work regardless of keypool size.

I don't think that is a situation we should support. It would mean that users could get addresses that are not currently being watched by the wallet. I think that there would be the expectation that the wallet is watching such an address and so any funds sent to them would appear in the wallet. But that would not be the case.

I didn't understand the reason to remove start_index and end_index. They work more like pagination. Without them, the command would return 2000 items (the default keypool size * 2).

I think that it is completely reasonable to return such a large result. There could be options for verbosity, e.g. a non-verbose output is just the addresses, then verbose is with all of the additional information that this PR currently includes.

We have many RPCs that will output a ton of data from the wallet, e.g. listunspent or listtransactions.

lsilva01 force-pushed on Sep 29, 2021

lsilva01 marked this as a draft on Sep 29, 2021

lsilva01 force-pushed on Sep 29, 2021

lsilva01 force-pushed on Sep 30, 2021

lsilva01 commented at 3:58 AM on September 30, 2021: contributor

I removed the indexes and changed the code to use address book, the keypool, and key metadata. The results of this RPC are:

. Legacy wallet: The addresses from address book (those returned by getnewaddress) are not ordered, but the those from keypool are in the correct BIP32 derivation order. This is a limitation on legacy wallet result, since keypool erases the key in getnewaddress. I'm not sure there is a simple way to order them.

. Descriptor wallet: All the addresses are in the correct BIP32 derivation order, as it is possible to iterate over the m_wallet_descriptor.cache using m_wallet_descriptor.range_start and m_wallet_descriptor.range_end.

Updated test information in PR description.

lsilva01 marked this as ready for review on Sep 30, 2021

in src/wallet/rpcwallet.cpp:296 in dd28fcb4d8 outdated

 291 | +
 292 | +    auto addressbalances = GetAddressBalances(*pwallet);
 293 | +
 294 | +    UniValue result(UniValue::VOBJ);
 295 | +
 296 | +    for (bool internal: { false, true }) {

achow101 commented at 6:40 PM on September 30, 2021:

In dd28fcb4d868763b47a62a41d2114462407481c2 "Add listaddresses RPC"

It seems like instead of doing this, you could just have CWallet::ListAddresses do internal too.

lsilva01 commented at 11:30 PM on October 1, 2021:

Done in 4c2625a .

in src/wallet/wallet.cpp:2178 in dd28fcb4d8 outdated

2173 | +            }
2174 | +        }
2175 | +    }
2176 | +
2177 | +    for(auto destination : destinations) {
2178 | +        auto meta = spk_man->GetMetadata(destination);

achow101 commented at 6:51 PM on September 30, 2021:

In dd28fcb4d868763b47a62a41d2114462407481c2 "Add listaddresses RPC"

This section is somewhat duplicated with getaddressinfo. I think it would be better to refactor that so that these can share the same code. Additionally, this does not need to be in the wallet (and thus require the introduction of another struct). I would prefer if you just did it in the RPC code.

lsilva01 commented at 11:36 PM on October 1, 2021:

Done in fcade9b.

Created a new function that both getaddressinfo and listaddresses call. This new function keeps the order of getaddressinfo fields.

in src/wallet/scriptpubkeyman.cpp:1642 in dd28fcb4d8 outdated

1637 | +{
1638 | +    LOCK(cs_desc_man);
1639 | +
1640 | +    addresses.reserve(m_wallet_descriptor.range_end);
1641 | +
1642 | +    for (int32_t i = m_wallet_descriptor.range_start; i < m_wallet_descriptor.range_end; ++i) {

achow101 commented at 6:54 PM on September 30, 2021:

In dd28fcb4d868763b47a62a41d2114462407481c2 "Add listaddresses RPC"

It would be more efficient to iterate m_map_script_pub_keys than to expand the descriptor again. This would also work for descriptors that have hardened derivation as the last step.

lsilva01 commented at 11:32 PM on October 1, 2021:

Done in 4c2625a.

m_map_script_pub_keys is sorted by descriptor range index before iteration.

Add listaddresses RPC 4c2625aa9a

Add functional test for listaddresses RPC b54ccfb87b

lsilva01 force-pushed on Oct 1, 2021

Change getaddressinfo() to use AddDestinationInfo() function

getaddressinfo() and listddresses() have fields in common.
AddDestinationInfo() was created to reuse the same code for both.
In this commit, getaddressinfo() is changed to call this function.

fcade9b64d

lsilva01 force-pushed on Oct 1, 2021

DrahtBot added the label Needs rebase on Dec 2, 2021

DrahtBot commented at 7:44 PM on December 2, 2021: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

DrahtBot commented at 1:06 PM on March 21, 2022: 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.

DrahtBot commented at 7:03 AM on July 25, 2022: 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.

MarcoFalke commented at 7:14 AM on July 25, 2022: member

Closing for now. Let us know when you are working on this again and if it should be reopened.

MarcoFalke closed this on Jul 25, 2022

bitcoin locked this on Jul 25, 2023