[WIP/Draft] Add Address tab

w0xlt commented at 0:26 am on July 22, 2022: contributor

This PR proposes a new tab called “Addresses” which shows some wallet addresses and also adds the listaddress RPC. This is a common feature that many wallets have, but not Bitcoin Core clients (GUI and bitcoin-cli). Most of commits were picked from https://github.com/bitcoin/bitcoin/pull/23019, but they were considerably modified.

As stated in the title, it is still a draft. I would like to check if reviewers find this feature interesting before proceeding (since Bitcoin Core has a different security model and architecture than most wallets).

Some caveats:

. This PR, at moment (91717d3d0181ef3b211c38dde72d313acd25b5ea), seems to work fine for descriptor wallets as m_map_script_pub_keys keeps all the keys, even after they have been generated (for receiving or change).

. But for legacy wallet, it has been showing inconsistent behavior. setInternalKeyPool and setExternalKeyPool remove the generated keys. I retrieved them from m_address_book but apparently this only works for the receiving keys.

. To extract the index of a key in the HD path, this PR uses a regex. Although it seems reliable, may have performance impact. Ideally, regex should not be used. If this feature is implemented for descriptor wallets only, this is not necessary as ScriptPubKey Map stores the index.

. The Address table shows m_hd_chain.m_next_{external,internal}_index + 5 addresses for legacy wallets and m_wallet_descriptor.next_index + 5 for descriptor. It means it shows up to the last generated address + 5.

. Just out of curiosity, I tested loading all addresses available in keypool (by default, 1000 keys for each keypool / descriptor) and this has a significant negative impact on wallet loading. I wonder if it would be possible to do this with good performance.

. pre-HD wallets have not been tested.

Feedback and suggestions on how to handle the issues mentioned above are welcome.

add struct `AddressInfo` cebe69b909

add `ScriptPubKeyMan::ListAddresses()` fc85bd6e8e

add `CWallet::ListAddresses` 471c8d41c0

add `listaddresses` RPC d6178c7927

w0xlt force-pushed on Jul 22, 2022

add `listaddresses` tests abeff842bf

add `interfaces::Wallet::listAddresses` 7d7f35bd40

add `AddressInfoTableModel` 28453c43f4

add `m_address_info_table_model` to `class WalletModel` effa8aab8e

update Address table when new address is generated beba5473c7

w0xlt force-pushed on Jul 22, 2022

jarolrod added the label Feature on Jul 22, 2022

jarolrod added the label UX on Jul 22, 2022

add `class AddressInfoView` f7b84ac310

add `AddressInfoView` to `WalletView` 8c89a9310f

add `gotoAddressInfoPage` in `class WalletFrame` c07970d5e2

add `Addresses` tab 91717d3d01

w0xlt force-pushed on Jul 22, 2022

sipa commented at 12:21 pm on July 22, 2022: contributor

I thought this was deliberate, because addresses are intended to be single-use.

jarolrod commented at 7:19 am on July 25, 2022: member

Before getting to the performance improvements, this may have to undergo some soul searching.

This should not be added as a primary tab in the GUI; it’s just not a core utility or function. Having this as one of the five main actions would confuse many users. Additionally, presuming a more “advanced” user is interested in this, you shouldn’t be so interested in your addresses that a case is made to have this as a main function.

If someone is interested in all the addresses like this, it would be better to keep it associated with how user’s currently look into their address book.

There should be a way to integrate this into the address book window. That should create more of a reason for introducing this change within the GUI.

luke-jr commented at 2:10 am on July 27, 2022: member

Concept NACK. This is just a more complicated/confusing address book.

Also, addresses do not have balances. They only receive, never send.

DrahtBot added the label Needs rebase on Aug 5, 2022

DrahtBot commented at 1:44 pm on August 5, 2022: contributor

🐙 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”.

DrahtBot commented at 1:12 am on November 5, 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 1:51 am on February 3, 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.

hebasto commented at 2:06 pm on March 27, 2023: member

Closing this due to lack of activity. Feel free to reopen.

DrahtBot commented at 2:06 pm on March 27, 2023: 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 NACK	luke-jr

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

hebasto closed this on Mar 27, 2023

bitcoin-core locked this on Mar 26, 2024

[WIP/Draft] Add Address tab #634

Reviews