Have the wallet give out destinations instead of keys #16237

pull achow101 wants to merge 3 commits into bitcoin:master from achow101:cwallet-getnewaddr changing 10 files +144 −121
  1. achow101 commented at 9:40 PM on June 18, 2019: member

    The wallet should give out destinations instead of keys. It should be the one that handles the conversion from key to destination and the setting of the label, not the caller. In order to do this, two new member functions are introduced GetNewDestination() and GetNewChangeDestination(). Additionally, CReserveKey is changed to be ReserveDestination and represents destinations whose keys can be returned to the keypool.

  2. fanquake added the label Wallet on Jun 18, 2019
  3. DrahtBot commented at 10:00 PM on June 18, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16361 (Remove redundant pre-TopUpKeypool checks by instagibbs)
    • #16341 (WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
    • #16208 (wallet: Consume CReserveKey on successful CreateTransaction by instagibbs)
    • #14582 (wallet: try -avoidpartialspends mode and use its result if fees do not change by kallewoof)

    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.

  4. in src/wallet/wallet.h:1126 in 2010b1a13e outdated 
    1122 | @@ -1123,6 +1123,8 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
1123 |  
1124 |      std::set<CTxDestination> GetLabelAddresses(const std::string& label) const;
1125 |  
1126 | +    bool GetNewAddress(const OutputType type, const std::string label, CTxDestination& dest, std::string& error);
    promag commented at 2:11 PM on June 19, 2019:

    2010b1a13e6b7eaa681cc62ae58079ccbec72816

    This should be atomic or add EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) annotation?

    Also, could make GetKeyFromPool private?

    achow101 commented at 4:14 PM on June 20, 2019:

    Done. Also fixed typo

  5. promag commented at 2:19 PM on June 19, 2019: member

    Concept ACK

    You have a typo in commit 7d014e492cdcca847cc0e46f0457b1bc89831f90 message.

  6. ryanofsky commented at 4:40 PM on June 19, 2019: member

    Concept ACK. Will review more closely, but this seems like a nice, straightforward change.

  7. meshcollider added this to the "PRs" column in a project
  8. achow101 force-pushed on Jun 20, 2019
  9. in src/interfaces/wallet.cpp:147 in 835ae1282d outdated 
     139 | @@ -140,9 +140,10 @@ class WalletImpl : public Wallet
 140 |      void abortRescan() override { m_wallet->AbortRescan(); }
 141 |      bool backupWallet(const std::string& filename) override { return m_wallet->BackupWallet(filename); }
 142 |      std::string getWalletName() override { return m_wallet->GetName(); }
 143 | -    bool getKeyFromPool(bool internal, CPubKey& pub_key) override
 144 | +    bool getNewAddress(const OutputType type, const std::string label, CTxDestination& dest) override
 145 |      {
 146 | -        return m_wallet->GetKeyFromPool(pub_key, internal);
 147 | +        std::string error;
 148 | +        return m_wallet->GetNewAddress(type, label, dest, error);
    MarcoFalke commented at 5:10 PM on June 20, 2019: 
    interfaces/wallet.cpp:146:16: error: calling function 'GetNewAddress' requires holding mutex 'm_wallet->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
    achow101 commented at 5:21 PM on June 20, 2019:

    Fixed

  10. achow101 force-pushed on Jun 20, 2019
  11. in src/wallet/wallet.cpp:2799 in 89df83470d outdated 
    2797 | +                CTxDestination dest;
2798 | +                const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend);
2799 | +                bool ret = reserveaddr.GetReservedAddress(change_type, dest, true);
2800 |                  if (!ret)
2801 |                  {
2802 |                      strFailReason = _("Keypool ran out, please call keypoolrefill first");
    laanwj commented at 11:23 AM on June 27, 2019:

    nit: the message is translated here, but not in the new occurences. As this looks like a message that will be returned over the RPC interface (referring to a RPC command, at least). Maybe better to not translate it here either?

    achow101 commented at 6:46 PM on July 9, 2019:

    I've made it untranslated.

  12. laanwj commented at 11:24 AM on June 27, 2019: member

    Concept ACK (my remark was going to be that the wallet should return Destinations, not addresses, but that's exactly that this does)

  13. Sjors commented at 3:29 PM on July 9, 2019: member

    Concept ACK.

    In line with @laanwj's comment, maybe rename GetNew(Change)Address to GetNew(Change)Destination, and ReserveAddress to ReserveDestination.

    GetReservedAddress is still used in two places; maybe kill it in this PR?

    Travis seems unhappy about a locking issue. On macOS I also get some warnings: <img width="1117" alt="Schermafbeelding 2019-07-09 om 16 19 25" src="https://user-images.githubusercontent.com/10217/60904816-a8e08e80-a26b-11e9-85a1-eaf278fc24e7.png">

  14. achow101 force-pushed on Jul 9, 2019
  15. achow101 force-pushed on Jul 9, 2019
  16. achow101 commented at 6:43 PM on July 9, 2019: member

    In line with @laanwj's comment, maybe rename GetNew(Change)Address to GetNew(Change)Destination, and ReserveAddress to ReserveDestination.

    I've renamed it.

    GetReservedAddress is still used in two places; maybe kill it in this PR?

    It is?

    Travis seems unhappy about a locking issue. On macOS I also get some warnings:

    Should be fixed now.

  17. achow101 force-pushed on Jul 9, 2019
  18. in src/wallet/wallet.cpp:3514 in 5d9184caf9 outdated 
    3512 | @@ -3513,6 +3513,26 @@ bool CWallet::GetKeyFromPool(CPubKey& result, bool internal)
3513 |      return true;
3514 |  }
3515 |  
3516 | +bool CWallet::GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, std::string& error)
3517 | +{
    instagibbs commented at 7:18 PM on July 9, 2019:

    micro-nit: can clear error here.

    achow101 commented at 8:28 PM on July 9, 2019:

    Does it matter?

    instagibbs commented at 8:35 PM on July 9, 2019:

    micro-matters in case someone calls the function twice and doesn't clear the error themself? :)

    achow101 commented at 8:43 PM on July 9, 2019:

    Done

  19. in src/wallet/wallet.cpp:3734 in 9be25c211c outdated 
    3731 | +    dest = address;
3732 |      return true;
3733 |  }
3734 |  
3735 | -void CReserveKey::KeepKey()
3736 | +void ReserveDestination::KeepAddress()
    instagibbs commented at 7:22 PM on July 9, 2019:

    We're using "destination" in GetReservedDestination but "address" here. We should pick one?

    achow101 commented at 8:34 PM on July 9, 2019:

    Whoops, fixed

  20. in src/wallet/wallet.cpp:3743 in 9be25c211c outdated 
    3741 |      vchPubKey = CPubKey();
3742 | +    address = CNoDestination();
3743 |  }
3744 |  
3745 | -void CReserveKey::ReturnKey()
3746 | +void ReserveDestination::ReturnAddress()
    instagibbs commented at 7:22 PM on July 9, 2019:

    We're using "destination" in GetReservedDestination but "address" here. We should pick one?

    achow101 commented at 8:35 PM on July 9, 2019:

    Fixed

  21. in src/wallet/wallet.cpp:3535 in 3a58f6d8b4 outdated 
    3529 | @@ -3530,6 +3530,22 @@ bool CWallet::GetNewDestination(const OutputType type, const std::string label,
3530 |      return true;
3531 |  }
3532 |  
3533 | +bool CWallet::GetNewChangeDestination(const OutputType type, CTxDestination& dest, std::string& error)
3534 | +{
    instagibbs commented at 7:24 PM on July 9, 2019:

    nit: same comment about clearing the error string

    achow101 commented at 8:44 PM on July 9, 2019:

    Done

  22. instagibbs commented at 7:24 PM on July 9, 2019: member

    concept ACK

  23. achow101 force-pushed on Jul 9, 2019
  24. achow101 renamed this:
    Have the wallet give out addresses instead of keys
    Have the wallet give out destinations instead of keys
    on Jul 9, 2019
  26. Add GetNewDestination to CWallet to fetch new destinations
    Instead of having the same multiple lines of code everywhere
that new destinations are fetched, introduce GetNewDestination as
a member function of CWallet which does the key fetching, label
setting, script generation, and destination generation.
    172213be5b
  27. Replace CReserveKey with ReserveDestinatoin
    Instead of reserving keys, reserve destinations which are backed by keys
    33d13edd2b
  28. Add GetNewChangeDestination for getting new change Destinations
    Adds a GetNewChangeDestination that has the same objective as GetNewDestination
    8e7f930828
  29. achow101 force-pushed on Jul 9, 2019
  30. instagibbs commented at 8:48 PM on July 9, 2019: member

    re-utACK https://github.com/bitcoin/bitcoin/pull/16237/commits/8e7f930828a9f8f9be1c90ff45e3fdfef1980eaf

    only changes are clearing error messages at beginning of GetNew*

  31. sipa commented at 9:39 PM on July 9, 2019: member

    ACK 8e7f930828a9f8f9be1c90ff45e3fdfef1980eaf. Concept ACK as this gives a much cleaner abstraction to work with, and light code review ACK.

    It's bit confusing that constructing a (filled) ReserveDestination does not actually require a destination type, preventing the interface from being able to select where to draw change from based on the address type desired. Perhaps something for a follow-up?

  32. laanwj commented at 9:45 AM on July 10, 2019: member

    ACK 8e7f930828a9f8f9be1c90ff45e3fdfef1980eaf

  33. laanwj merged this on Jul 10, 2019
  34. laanwj closed this on Jul 10, 2019
  35. laanwj referenced this in commit 8d1286014c on Jul 10, 2019
  36. Sjors commented at 12:59 PM on July 10, 2019: member

    The macOS warnings I saw before are indeed gone now.

  37. sidhujag referenced this in commit d8122c6e8e on Jul 10, 2019
  38. fanquake moved this from the "PRs" to the "Done" column in a project
  39. deadalnix referenced this in commit 922fd79e11 on Jul 1, 2020
  40. deadalnix referenced this in commit bf733950e6 on Jul 1, 2020
  41. deadalnix referenced this in commit ac5ff19dd4 on Jul 1, 2020
  42. MarcoFalke locked this on Dec 16, 2021
Contributors
achow101DrahtBotpromagryanofskyMarcoFalkelaanwjSjorsinstagibbssipa
Labels
Wallet
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-19 00:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me