gui icon indicating copy to clipboard operation
gui copied to clipboard

[WIP/Draft] Add Address tab

Open w0xlt opened this issue 2 years ago • 3 comments

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.

image

w0xlt avatar Jul 22 '22 00:07 w0xlt

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

sipa avatar Jul 22 '22 12:07 sipa

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

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

luke-jr avatar Jul 27 '22 02:07 luke-jr

🐙 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 avatar Aug 05 '22 13:08 DrahtBot

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 avatar Nov 05 '22 01:11 DrahtBot