Use shared pointer to retain wallet instance #13063

pull promag wants to merge 1 commits into bitcoin:master from promag:2018-04-wallet-sharedptr changing 15 files +239 −139
  1. promag commented at 11:11 PM on April 23, 2018: member

    Currently there are 3 places where it makes sense to retain a wallet shared pointer:

    • vpwallets;
    • interfaces::Wallet interface instance - used by the UI;
    • wallet RPC functions - given by GetWalletForJSONRPCRequest.

    The way it is now it is possible to have, for instance, listunspent RPC and in parallel unload the wallet (once #13111 is merged) without blocking. Once the RPC finishes, the shared pointer will release the wallet.

    It is also possible to get all existing wallets without blocking because the caller keeps a local list of shared pointers.

    This is mostly relevant for wallet unloading.

    This PR replaces #11402.

  2. promag force-pushed on Apr 23, 2018
  3. promag force-pushed on Apr 23, 2018
  4. fanquake added the label Wallet on Apr 24, 2018
  5. jnewbery commented at 3:59 PM on April 24, 2018: member

    needs rebase

  6. promag force-pushed on Apr 24, 2018
  7. promag commented at 4:30 PM on April 24, 2018: member

    Rebased.

  8. Empact commented at 9:19 PM on April 25, 2018: member

    I like the idea of CWalletRef here. std::shared_ptr<CWallet> buries the lede a bit.

  9. jonasschnelli commented at 7:56 AM on April 26, 2018: contributor

    Concept ACK. A bit unfortunate that we have a lot of CWallet* const pwallet = wallet.get(); calls outside of any lock which AFAIK partially defeats the improvements the shared pointer adds.

  10. promag commented at 9:04 AM on April 26, 2018: member

    A bit unfortunate that we have a lot of CWallet* const pwallet = wallet.get(); calls outside of any lock @jonasschnelli what you mean by outside? There is always a std::shared_ptr<CWallet> in scope when .get() pointer is used. The only exception is RegisterValidationInterface(walletInstance.get());.

  11. jonasschnelli commented at 9:05 AM on April 26, 2018: contributor

    @promag: Yes. Your right.

  12. promag force-pushed on Apr 29, 2018
  13. jnewbery added this to the "In progress" column in a project
  14. promag force-pushed on May 7, 2018
  15. promag force-pushed on May 7, 2018
  16. promag commented at 8:39 PM on May 7, 2018: member

    Rebased on master.

  17. promag force-pushed on May 8, 2018
  18. in src/interfaces/wallet.cpp:457 in 1279e2bdc1 outdated 
     452 | @@ -453,11 +453,12 @@ class WalletImpl : public Wallet
 453 |          return MakeHandler(m_wallet.NotifyWatchonlyChanged.connect(fn));
 454 |      }
 455 |  
 456 | +    std::shared_ptr<CWallet> m_shared_wallet;
 457 |      CWallet& m_wallet;
    jnewbery commented at 2:08 PM on May 8, 2018:

    Is it possible to remove m_wallet and just keep m_shared_wallet?

    promag commented at 2:23 PM on May 22, 2018:

    Yes, sure, but the diff will grow.

    jnewbery commented at 3:12 PM on May 22, 2018:

    Can be done in a follow-up PR.

  19. in src/wallet/rpcwallet.cpp:138 in 1279e2bdc1 outdated 
     133 | @@ -134,7 +134,9 @@ static std::string LabelFromValue(const UniValue& value)
 134 |  
 135 |  static UniValue getnewaddress(const JSONRPCRequest& request)
 136 |  {
 137 | -    CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
 138 | +    std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
 139 | +    CWallet* const pwallet = wallet.get();
    jnewbery commented at 2:09 PM on May 8, 2018:

    Can we update EnsureWalletIsAvailable() to take a std::shared_ptr<CWallet>, and then remove the need to create a CWallet* const in all of these rpc methods?

    promag commented at 2:22 PM on May 22, 2018:

    The problem is that there are a lot of other calls that receive CWallet* and the diff started to be big. I can do that in a separate branch so you can see, WDYT?

    jnewbery commented at 3:12 PM on May 22, 2018:

    Definitely. Can be done in a follow-up PR.

  20. in src/wallet/init.cpp:229 in 1279e2bdc1 outdated 
     258 | @@ -259,7 +259,7 @@ bool WalletInit::Open() const
 259 |      }
 260 |  
 261 |      for (const std::string& walletFile : gArgs.GetArgs("-wallet")) {
 262 | -        CWallet * const pwallet = CWallet::CreateWalletFromFile(walletFile, fs::absolute(walletFile, GetWalletDir()));
 263 | +        std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(walletFile, fs::absolute(walletFile, GetWalletDir()));
    jnewbery commented at 2:21 PM on May 8, 2018:

    nit: it'd be nice to rename these variables to wallet (yes, I know these are (smart) pointers, but pwallet everywhere else means raw pointers)

  21. jnewbery commented at 2:22 PM on May 8, 2018: member

    utACK 1279e2bdc1493e1d7b53ab43d09d16ac0926eb65

  22. jnewbery commented at 2:23 PM on May 8, 2018: member

    Can you update the PR description with the motivation for this (possibly just copy from #11402)?

  23. jonasschnelli approved
  24. jonasschnelli commented at 6:54 AM on May 9, 2018: contributor

    utACK 1279e2bdc1493e1d7b53ab43d09d16ac0926eb65

    I agree with @jnewbery that removing m_wallet in interface/wallet.cpp would be good, though I think this can also be done in another pull.

  25. laanwj added this to the "Blockers" column in a project
  26. in src/wallet/rpcwallet.h:23 in 1279e2bdc1 outdated 
      19 | @@ -20,7 +20,7 @@ void RegisterWalletRPCCommands(CRPCTable &t);
  20 |   * @param[in] request JSONRPCRequest that wishes to access a wallet
  21 |   * @return nullptr if no wallet should be used, or a pointer to the CWallet
  22 |   */
  23 | -CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request);
  24 | +std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request);
    jimpo commented at 12:01 AM on May 19, 2018:

    I think this should just return a raw pointer or CWallet&.

    promag commented at 2:18 PM on May 22, 2018:

    The RPC handler must hold the wallet to prevent concurrent unload.

  27. jimpo commented at 12:08 AM on May 19, 2018: contributor

    I feel like this can all be done with unique_ptrs and references, which is preferable. Basically,

    std::vector<std::unique_ptr<CWallet>> vpwallets;  // Has owning references to all the wallets

CWallet& AddWallet(std::unique_ptr<CWallet> wallet);  // Adds and gives ownership to vpwallets
std::unique_ptr<CWallet> RemoveWallet(const CWallet& wallet);  // Removes and releases ownership from vpwallets

std::vector<CWallet&> GetWallets();
CWallet& GetWallet(const std::string& name);

    Any particular reason they need to be shared?

  28. danielsam approved
  29. danielsam commented at 6:48 PM on May 21, 2018: none

    Looks good

  30. MarcoFalke commented at 7:26 PM on May 21, 2018: member

    Needs rebase

  31. promag force-pushed on May 22, 2018
  32. promag commented at 2:14 PM on May 22, 2018: member 
    std::unique_ptr<CWallet> RemoveWallet(const CWallet& wallet);  // Removes and releases ownership from vpwallets

    @jimpo without shared_ptr the caller of RemoveWallet must ensure no one is using the wallet before releasing it. This may require synchronization between multiple threads, including UI.

    With shared_ptr, the last pointer can safely release the wallet.

  33. promag commented at 2:17 PM on May 22, 2018: member

    @jnewbery improved PR description.

  34. promag force-pushed on May 22, 2018
  35. promag commented at 2:28 PM on May 22, 2018: member

    Rebased due to merge of #12560 and #13059.

  36. promag force-pushed on May 22, 2018
  37. jnewbery commented at 3:49 PM on May 22, 2018: member

    Fails build:

    CXX      wallet/libbitcoin_wallet_a-wallet.o
wallet/wallet.cpp: In member function ‘DBErrors CWallet::LoadWallet(bool&)’:
wallet/wallet.cpp:3204:32: error: no match for call to ‘(boost::signals2::signal<void(std::shared_ptr<CWallet>)>) (CWallet* const)’
     uiInterface.LoadWallet(this);
                                ^
  38. wallet: Use shared pointer to retain wallet instance 80b4910f7d
  39. promag force-pushed on May 22, 2018
  40. promag commented at 4:19 PM on May 22, 2018: member

    Fixed @jnewbery.

  41. in src/wallet/wallet.cpp:4342 in 80b4910f7d 
    4338 | @@ -4342,7 +4339,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
4339 |      }
4340 |  
4341 |      // Register with the validation interface. It's ok to do this after rescan since we're still holding cs_main.
4342 | -    RegisterValidationInterface(temp_wallet.release());
4343 | +    RegisterValidationInterface(walletInstance.get());
    jimpo commented at 4:49 PM on May 22, 2018:

    When does this get unregistered? Seems like the validation interface queue may hold a pointer after the wallet is released.

    promag commented at 5:41 PM on May 22, 2018:

    When UnregisterAllValidationInterfaces is called. Obviously unloadwallet RPC or the Unload wallet action in the UI will have to unregister.

  42. jimpo commented at 4:50 PM on May 22, 2018: contributor

    Ah, yes, I see the issue with unique_ptr.

  43. jnewbery commented at 6:14 PM on May 22, 2018: member

    utACK 80b4910f7d87983f50047074c3c2397b0a5c4e92

  44. jimpo commented at 6:56 PM on May 22, 2018: contributor

    utACK 80b4910

  45. laanwj merged this on May 24, 2018
  46. laanwj closed this on May 24, 2018
  47. laanwj referenced this in commit 6378eef18f on May 24, 2018
  48. MarcoFalke removed this from the "Blockers" column in a project
  49. fanquake moved this from the "In progress" to the "Done" column in a project
  50. MarcoFalke referenced this in commit 6579d80572 on Jun 20, 2018
  51. promag deleted the branch on Apr 11, 2020
  52. PastaPastaPasta referenced this in commit 5d483106f8 on Dec 16, 2020
  53. PastaPastaPasta referenced this in commit ce5b06ed66 on Dec 18, 2020
  54. MarcoFalke referenced this in commit eea6196c3d on Mar 10, 2021
  55. xdustinface referenced this in commit 61c36b5b60 on Mar 31, 2021
  56. xdustinface referenced this in commit 72349ea80c on Mar 31, 2021
  57. xdustinface referenced this in commit 5f44ba0c03 on Mar 31, 2021
  58. xdustinface referenced this in commit c376b64a4f on Apr 1, 2021
  59. PastaPastaPasta referenced this in commit a3cc51118e on Apr 1, 2021
  60. xdustinface referenced this in commit a34ac73dd2 on Apr 4, 2021
  61. xdustinface referenced this in commit af40021c9f on Apr 4, 2021
  62. xdustinface referenced this in commit 713e241b6f on Apr 4, 2021
  63. xdustinface referenced this in commit d84ee328ff on Apr 4, 2021
  64. xdustinface referenced this in commit 773e39e1e5 on Apr 5, 2021
  65. xdustinface referenced this in commit a8454e22b0 on Apr 13, 2021
  66. DrahtBot locked this on Feb 15, 2022
Contributors
promagjnewberyEmpactjonasschnellijimpodanielsamMarcoFalke
Labels
Wallet
Linked (view graph)
#11402 Use shared pointer for wallet instances#13059 Dynamic wallet load / create / unload#13111 Add unloadwallet RPC#13316 make check fails for configure option --with-incompatible-bdb and Berkeley DB 5.3#13506 Qt: load wallet in UI after possible init aborts#18592 rpc: replace raw pointers with shared_ptrs#21331 rpc: replace wallet raw pointers with references (#18592 rebased)
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-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me