From f1ee37319a7a211e5fb325406d62db5b61dbd30e Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 17 Aug 2020 15:21:50 -0400 Subject: [PATCH] wallet: Reload previously loaded wallets on GUI startup Enable the GUI to also use the load_on_startup feature. Wallets loaded in the GUI always have load_on_startup=true. When they are unloaded from the GUI, load_on_startup=false. To facilitate this change, UpdateWalletSetting is moved into the wallet module and called from within LoadWallet, RemoveWallet, and Createwallet. This change does not actually touch the GUI code but rather the wallet functions that are shared between the GUI and RPC. --- doc/release-notes-15937.md | 19 +++++---- src/interfaces/wallet.cpp | 6 +-- src/qt/test/addressbooktests.cpp | 2 +- src/qt/test/wallettests.cpp | 2 +- src/wallet/load.cpp | 26 +----------- src/wallet/load.h | 6 --- src/wallet/rpcwallet.cpp | 30 ++++---------- src/wallet/test/wallet_tests.cpp | 6 +-- src/wallet/wallet.cpp | 70 +++++++++++++++++++++++++++++--- src/wallet/wallet.h | 14 +++++-- 10 files changed, 104 insertions(+), 77 deletions(-) diff --git a/doc/release-notes-15937.md b/doc/release-notes-15937.md index ec7d355dfaf..1ab817b0e59 100644 --- a/doc/release-notes-15937.md +++ b/doc/release-notes-15937.md @@ -1,12 +1,15 @@ Configuration ------------- -The `createwallet`, `loadwallet`, and `unloadwallet` RPCs now accept -`load_on_startup` options that modify bitcoin's dynamic configuration in -`\/settings.json`, and can add or remove a wallet from the list of -wallets automatically loaded at startup. Unless these options are explicitly -set to true or false, the load on startup wallet list is not modified, so this -change is backwards compatible. +Wallets created or loaded in the GUI will now be automatically loaded on +startup, so they don't need to be manually reloaded next time Bitcoin is +started. The list of wallets to load on startup is stored in +`\/settings.json` and augments any command line or `bitcoin.conf` +`-wallet=` settings that specify more wallets to load. Wallets that are +unloaded in the GUI get removed from the settings list so they won't load again +automatically next startup. (#19754) -In the future, the GUI will start updating the same startup wallet list as the -RPCs to automatically reopen wallets previously opened in the GUI. +The `createwallet`, `loadwallet`, and `unloadwallet` RPCs now accept +`load_on_startup` options to modify the settings list. Unless these options are +explicitly set to true or false, the list is not modified, so the RPC methods +remain backwards compatible. (#15937) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 63c109658ea..28839b2ffc7 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -446,7 +446,7 @@ public: CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; } void remove() override { - RemoveWallet(m_wallet); + RemoveWallet(m_wallet, false /* load_on_start */); } bool isLegacy() override { return m_wallet->IsLegacy(); } std::unique_ptr handleUnload(UnloadFn fn) override @@ -517,12 +517,12 @@ public: std::unique_ptr createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, WalletCreationStatus& status, bilingual_str& error, std::vector& warnings) override { std::shared_ptr wallet; - status = CreateWallet(*m_context.chain, passphrase, wallet_creation_flags, name, error, warnings, wallet); + status = CreateWallet(*m_context.chain, passphrase, wallet_creation_flags, name, true /* load_on_start */, error, warnings, wallet); return MakeWallet(std::move(wallet)); } std::unique_ptr loadWallet(const std::string& name, bilingual_str& error, std::vector& warnings) override { - return MakeWallet(LoadWallet(*m_context.chain, WalletLocation(name), error, warnings)); + return MakeWallet(LoadWallet(*m_context.chain, WalletLocation(name), true /* load_on_start */, error, warnings)); } std::string getWalletDir() override { diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index 84f981dff31..d6781460a71 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -112,7 +112,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node) ClientModel clientModel(node, &optionsModel); AddWallet(wallet); WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel, platformStyle.get()); - RemoveWallet(wallet); + RemoveWallet(wallet, nullopt); EditAddressDialog editAddressDialog(EditAddressDialog::NewSendingAddress); editAddressDialog.setModel(walletModel.getAddressTableModel()); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index adcfe0d25c3..98065803e9b 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -167,7 +167,7 @@ void TestGUI(interfaces::Node& node) ClientModel clientModel(node, &optionsModel); AddWallet(wallet); WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel, platformStyle.get()); - RemoveWallet(wallet); + RemoveWallet(wallet, nullopt); sendCoinsDialog.setModel(&walletModel); transactionView.setModel(&walletModel); diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index ae14769edbd..5bbc8b91f71 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -118,30 +118,8 @@ void UnloadWallets() while (!wallets.empty()) { auto wallet = wallets.back(); wallets.pop_back(); - RemoveWallet(wallet); + std::vector warnings; + RemoveWallet(wallet, nullopt, warnings); UnloadWallet(std::move(wallet)); } } - -bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name) -{ - util::SettingsValue setting_value = chain.getRwSetting("wallet"); - if (!setting_value.isArray()) setting_value.setArray(); - for (const util::SettingsValue& value : setting_value.getValues()) { - if (value.isStr() && value.get_str() == wallet_name) return true; - } - setting_value.push_back(wallet_name); - return chain.updateRwSetting("wallet", setting_value); -} - -bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_name) -{ - util::SettingsValue setting_value = chain.getRwSetting("wallet"); - if (!setting_value.isArray()) return true; - util::SettingsValue new_value(util::SettingsValue::VARR); - for (const util::SettingsValue& value : setting_value.getValues()) { - if (!value.isStr() || value.get_str() != wallet_name) new_value.push_back(value); - } - if (new_value.size() == setting_value.size()) return true; - return chain.updateRwSetting("wallet", new_value); -} diff --git a/src/wallet/load.h b/src/wallet/load.h index 30f1a4c90d5..ff4f5b4b23d 100644 --- a/src/wallet/load.h +++ b/src/wallet/load.h @@ -34,10 +34,4 @@ void StopWallets(); //! Close all wallets. void UnloadWallets(); -//! Add wallet name to persistent configuration so it will be loaded on startup. -bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name); - -//! Remove wallet name from persistent configuration so it will not be loaded on startup. -bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_name); - #endif // BITCOIN_WALLET_LOAD_H diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index a43fa102668..74259099c91 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -230,18 +230,6 @@ static void SetFeeEstimateMode(const CWallet* pwallet, CCoinControl& cc, const U } } -static void UpdateWalletSetting(interfaces::Chain& chain, - const std::string& wallet_name, - const UniValue& load_on_startup, - std::vector& warnings) -{ - if (load_on_startup.isTrue() && !AddWalletSetting(chain, wallet_name)) { - warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup.")); - } else if (load_on_startup.isFalse() && !RemoveWalletSetting(chain, wallet_name)) { - warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may still be loaded next node startup.")); - } -} - static UniValue getnewaddress(const JSONRPCRequest& request) { RPCHelpMan{"getnewaddress", @@ -2528,11 +2516,10 @@ static UniValue loadwallet(const JSONRPCRequest& request) bilingual_str error; std::vector warnings; - std::shared_ptr const wallet = LoadWallet(*context.chain, location, error, warnings); + Optional load_on_start = request.params[1].isNull() ? nullopt : Optional(request.params[1].get_bool()); + std::shared_ptr const wallet = LoadWallet(*context.chain, location, load_on_start, error, warnings); if (!wallet) throw JSONRPCError(RPC_WALLET_ERROR, error.original); - UpdateWalletSetting(*context.chain, location.GetName(), request.params[1], warnings); - UniValue obj(UniValue::VOBJ); obj.pushKV("name", wallet->GetName()); obj.pushKV("warning", Join(warnings, Untranslated("\n")).original); @@ -2662,7 +2649,8 @@ static UniValue createwallet(const JSONRPCRequest& request) bilingual_str error; std::shared_ptr wallet; - WalletCreationStatus status = CreateWallet(*context.chain, passphrase, flags, request.params[0].get_str(), error, warnings, wallet); + Optional load_on_start = request.params[6].isNull() ? nullopt : Optional(request.params[6].get_bool()); + WalletCreationStatus status = CreateWallet(*context.chain, passphrase, flags, request.params[0].get_str(), load_on_start, error, warnings, wallet); switch (status) { case WalletCreationStatus::CREATION_FAILED: throw JSONRPCError(RPC_WALLET_ERROR, error.original); @@ -2673,8 +2661,6 @@ static UniValue createwallet(const JSONRPCRequest& request) // no default case, so the compiler can warn about missing cases } - UpdateWalletSetting(*context.chain, request.params[0].get_str(), request.params[6], warnings); - UniValue obj(UniValue::VOBJ); obj.pushKV("name", wallet->GetName()); obj.pushKV("warning", Join(warnings, Untranslated("\n")).original); @@ -2717,15 +2703,13 @@ static UniValue unloadwallet(const JSONRPCRequest& request) // Release the "main" shared pointer and prevent further notifications. // Note that any attempt to load the same wallet would fail until the wallet // is destroyed (see CheckUniqueFileid). - if (!RemoveWallet(wallet)) { + std::vector warnings; + Optional load_on_start = request.params[1].isNull() ? nullopt : Optional(request.params[1].get_bool()); + if (!RemoveWallet(wallet, load_on_start, warnings)) { throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded"); } - interfaces::Chain& chain = wallet->chain(); - std::vector warnings; - UnloadWallet(std::move(wallet)); - UpdateWalletSetting(chain, wallet_name, request.params[1], warnings); UniValue result(UniValue::VOBJ); result.pushKV("warning", Join(warnings, Untranslated("\n")).original); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index c46e90d64bc..f400e7e9442 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -229,7 +229,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) "downloading and rescanning the relevant blocks (see -reindex and -rescan " "options).\"}},{\"success\":true}]", 0, oldTip->GetBlockTimeMax(), TIMESTAMP_WINDOW)); - RemoveWallet(wallet); + RemoveWallet(wallet, nullopt); } } @@ -275,7 +275,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) request.params.push_back(backup_file); ::dumpwallet().HandleRequest(request); - RemoveWallet(wallet); + RemoveWallet(wallet, nullopt); } // Call importwallet RPC and verify all blocks with timestamps >= BLOCK_TIME @@ -292,7 +292,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) AddWallet(wallet); wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); ::importwallet().HandleRequest(request); - RemoveWallet(wallet); + RemoveWallet(wallet, nullopt); BOOST_CHECK_EQUAL(wallet->mapWallet.size(), 3U); BOOST_CHECK_EQUAL(m_coinbase_txns.size(), 103U); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 34a5b7b8d21..fcdf3b9c7da 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -33,6 +33,8 @@ #include #include +#include + #include #include @@ -54,6 +56,42 @@ static RecursiveMutex cs_wallets; static std::vector> vpwallets GUARDED_BY(cs_wallets); static std::list g_load_wallet_fns GUARDED_BY(cs_wallets); +bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name) +{ + util::SettingsValue setting_value = chain.getRwSetting("wallet"); + if (!setting_value.isArray()) setting_value.setArray(); + for (const util::SettingsValue& value : setting_value.getValues()) { + if (value.isStr() && value.get_str() == wallet_name) return true; + } + setting_value.push_back(wallet_name); + return chain.updateRwSetting("wallet", setting_value); +} + +bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_name) +{ + util::SettingsValue setting_value = chain.getRwSetting("wallet"); + if (!setting_value.isArray()) return true; + util::SettingsValue new_value(util::SettingsValue::VARR); + for (const util::SettingsValue& value : setting_value.getValues()) { + if (!value.isStr() || value.get_str() != wallet_name) new_value.push_back(value); + } + if (new_value.size() == setting_value.size()) return true; + return chain.updateRwSetting("wallet", new_value); +} + +static void UpdateWalletSetting(interfaces::Chain& chain, + const std::string& wallet_name, + Optional load_on_startup, + std::vector& warnings) +{ + if (load_on_startup == nullopt) return; + if (load_on_startup.get() && !AddWalletSetting(chain, wallet_name)) { + warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup.")); + } else if (!load_on_startup.get() && !RemoveWalletSetting(chain, wallet_name)) { + warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may still be loaded next node startup.")); + } +} + bool AddWallet(const std::shared_ptr& wallet) { LOCK(cs_wallets); @@ -65,18 +103,32 @@ bool AddWallet(const std::shared_ptr& wallet) return true; } -bool RemoveWallet(const std::shared_ptr& wallet) +bool RemoveWallet(const std::shared_ptr& wallet, Optional load_on_start, std::vector& warnings) { assert(wallet); + + interfaces::Chain& chain = wallet->chain(); + std::string name = wallet->GetName(); + // Unregister with the validation interface which also drops shared ponters. wallet->m_chain_notifications_handler.reset(); LOCK(cs_wallets); std::vector>::iterator i = std::find(vpwallets.begin(), vpwallets.end(), wallet); if (i == vpwallets.end()) return false; vpwallets.erase(i); + + // Write the wallet setting + UpdateWalletSetting(chain, name, load_on_start, warnings); + return true; } +bool RemoveWallet(const std::shared_ptr& wallet, Optional load_on_start) +{ + std::vector warnings; + return RemoveWallet(wallet, load_on_start, warnings); +} + std::vector> GetWallets() { LOCK(cs_wallets); @@ -148,7 +200,7 @@ void UnloadWallet(std::shared_ptr&& wallet) } namespace { -std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector& warnings) +std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const WalletLocation& location, Optional load_on_start, bilingual_str& error, std::vector& warnings) { try { if (!CWallet::Verify(chain, location, error, warnings)) { @@ -163,6 +215,10 @@ std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const Wall } AddWallet(wallet); wallet->postInitProcess(); + + // Write the wallet setting + UpdateWalletSetting(chain, location.GetName(), load_on_start, warnings); + return wallet; } catch (const std::runtime_error& e) { error = Untranslated(e.what()); @@ -171,19 +227,19 @@ std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const Wall } } // namespace -std::shared_ptr LoadWallet(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector& warnings) +std::shared_ptr LoadWallet(interfaces::Chain& chain, const WalletLocation& location, Optional load_on_start, bilingual_str& error, std::vector& warnings) { auto result = WITH_LOCK(g_loading_wallet_mutex, return g_loading_wallet_set.insert(location.GetName())); if (!result.second) { error = Untranslated("Wallet already being loading."); return nullptr; } - auto wallet = LoadWalletInternal(chain, location, error, warnings); + auto wallet = LoadWalletInternal(chain, location, load_on_start, error, warnings); WITH_LOCK(g_loading_wallet_mutex, g_loading_wallet_set.erase(result.first)); return wallet; } -WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, bilingual_str& error, std::vector& warnings, std::shared_ptr& result) +WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, Optional load_on_start, bilingual_str& error, std::vector& warnings, std::shared_ptr& result) { // Indicate that the wallet is actually supposed to be blank and not just blank to make it encrypted bool create_blank = (wallet_creation_flags & WALLET_FLAG_BLANK_WALLET); @@ -254,6 +310,10 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& AddWallet(wallet); wallet->postInitProcess(); result = wallet; + + // Write the wallet settings + UpdateWalletSetting(chain, name, load_on_start, warnings); + return WalletCreationStatus::SUCCESS; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ebd2e91492e..73e9aef8ca5 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -50,10 +50,11 @@ struct bilingual_str; void UnloadWallet(std::shared_ptr&& wallet); bool AddWallet(const std::shared_ptr& wallet); -bool RemoveWallet(const std::shared_ptr& wallet); +bool RemoveWallet(const std::shared_ptr& wallet, Optional load_on_start, std::vector& warnings); +bool RemoveWallet(const std::shared_ptr& wallet, Optional load_on_start); std::vector> GetWallets(); std::shared_ptr GetWallet(const std::string& name); -std::shared_ptr LoadWallet(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector& warnings); +std::shared_ptr LoadWallet(interfaces::Chain& chain, const WalletLocation& location, Optional load_on_start, bilingual_str& error, std::vector& warnings); std::unique_ptr HandleLoadWallet(LoadWalletFn load_wallet); enum class WalletCreationStatus { @@ -62,7 +63,7 @@ enum class WalletCreationStatus { ENCRYPTION_FAILED }; -WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, bilingual_str& error, std::vector& warnings, std::shared_ptr& result); +WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, Optional load_on_start, bilingual_str& error, std::vector& warnings, std::shared_ptr& result); //! -paytxfee default constexpr CAmount DEFAULT_PAY_TX_FEE = 0; @@ -1340,4 +1341,11 @@ public: // be IsAllFromMe). int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet); int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector& txouts, bool use_max_sig = false); + +//! Add wallet name to persistent configuration so it will be loaded on startup. +bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name); + +//! Remove wallet name from persistent configuration so it will not be loaded on startup. +bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_name); + #endif // BITCOIN_WALLET_WALLET_H