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.

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.

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

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.

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.

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

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

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

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