diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp index e3073693c0229..9fe8566d71b4e 100644 --- a/src/qt/askpassphrasedialog.cpp +++ b/src/qt/askpassphrasedialog.cpp @@ -351,9 +351,9 @@ void AskPassphraseDialog::warningMessage() openStandardDialog( tr("Wallet encrypted"), "" + - tr("%1 will close now to finish the encryption process. " + tr("Your wallet is now encrypted. " "Remember that encrypting your wallet cannot fully protect " - "your PIVs from being stolen by malware infecting your computer.").arg(PACKAGE_NAME) + + "your PIVs from being stolen by malware infecting your computer.") + "

" + tr("IMPORTANT: Any previous backups you have made of your wallet file " "should be replaced with the newly generated, encrypted wallet file. " @@ -362,7 +362,6 @@ void AskPassphraseDialog::warningMessage() "", tr("OK") ); - QApplication::quit(); } void AskPassphraseDialog::errorEncryptingWallet() diff --git a/src/test/librust/sapling_rpc_wallet_tests.cpp b/src/test/librust/sapling_rpc_wallet_tests.cpp index d6ee944b7f200..c5c98c66aa046 100644 --- a/src/test/librust/sapling_rpc_wallet_tests.cpp +++ b/src/test/librust/sapling_rpc_wallet_tests.cpp @@ -14,6 +14,7 @@ #include "key_io.h" #include "consensus/merkle.h" #include "wallet/wallet.h" +#include "wallet/walletutil.h" #include "sapling/key_io_sapling.h" #include "sapling/address.h" @@ -497,7 +498,14 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling) vpwallets.erase(vpwallets.begin()); } -BOOST_AUTO_TEST_CASE(rpc_wallet_encrypted_wallet_sapzkeys) +struct RealWalletTestingSetup : public WalletTestingSetupBase +{ + RealWalletTestingSetup() : WalletTestingSetupBase(CBaseChainParams::MAIN, + "test_wallet", + WalletDatabase::Create(fs::absolute("test_wallet", GetWalletDir()))) {}; +}; + +BOOST_FIXTURE_TEST_CASE(rpc_wallet_encrypted_wallet_sapzkeys, RealWalletTestingSetup) { UniValue retValue; int n = 100; @@ -535,6 +543,7 @@ BOOST_AUTO_TEST_CASE(rpc_wallet_encrypted_wallet_sapzkeys) PushCurrentDirectory push_dir(gArgs.GetArg("-datadir","/tmp/thisshouldnothappen")); BOOST_CHECK(m_wallet.EncryptWallet(strWalletPass)); + BOOST_CHECK(m_wallet.IsCrypted()); // Verify we can still list the keys imported BOOST_CHECK_NO_THROW(retValue = CallRPC("listshieldaddresses")); @@ -547,6 +556,7 @@ BOOST_AUTO_TEST_CASE(rpc_wallet_encrypted_wallet_sapzkeys) // We can't call RPC walletpassphrase as that invokes RPCRunLater which breaks tests. // So we manually unlock. BOOST_CHECK(m_wallet.Unlock(strWalletPass)); + BOOST_CHECK(m_wallet.IsCrypted()); // Now add a key BOOST_CHECK_NO_THROW(CallRPC("getnewshieldaddress")); diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 89b1399faa031..1f69de6331858 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -24,6 +24,7 @@ namespace { + //! Make sure database has a unique fileid within the environment. If it //! doesn't, throw an error. BDB caches do not work properly when more than one //! open database has the same fileid (values written to one database may show @@ -33,25 +34,19 @@ namespace { //! (https://docs.oracle.com/cd/E17275_01/html/programmer_reference/program_copy.html), //! so bitcoin should never create different databases with the same fileid, but //! this error can be triggered if users manually copy database files. -void CheckUniqueFileid(const BerkeleyEnvironment& env, const std::string& filename, Db& db) +void CheckUniqueFileid(const BerkeleyEnvironment& env, const std::string& filename, Db& db, WalletDatabaseFileId& fileid) { if (env.IsMock()) return; - u_int8_t fileid[DB_FILE_ID_LEN]; - int ret = db.get_mpf()->get_fileid(fileid); + int ret = db.get_mpf()->get_fileid(fileid.value); if (ret != 0) { throw std::runtime_error(strprintf("BerkeleyBatch: Can't open database %s (get_fileid failed with %d)", filename, ret)); } - for (const auto& item : env.mapDb) { - u_int8_t item_fileid[DB_FILE_ID_LEN]; - if (item.second && item.second->get_mpf()->get_fileid(item_fileid) == 0 && - memcmp(fileid, item_fileid, sizeof(fileid)) == 0) { - const char* item_filename = nullptr; - item.second->get_dbname(&item_filename, nullptr); + for (const auto& item : env.m_fileids) { + if (fileid == item.second && &fileid != &item.second) { throw std::runtime_error(strprintf("BerkeleyBatch: Can't open database %s (duplicates fileid %s from %s)", filename, - HexStr(item_fileid), - item_filename ? item_filename : "(unknown database)")); + HexStr(item.second.value), item.first)); } } } @@ -60,6 +55,11 @@ RecursiveMutex cs_db; std::map g_dbenvs; //!< Map from directory name to open db environment. } // namespace +bool WalletDatabaseFileId::operator==(const WalletDatabaseFileId& rhs) const +{ + return memcmp(value, &rhs.value, sizeof(value)) == 0; +} + BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename) { fs::path env_directory; @@ -106,7 +106,7 @@ void BerkeleyEnvironment::Close() int ret = dbenv->close(0); if (ret != 0) - LogPrintf("Error %d shutting down database environment: %s\n", ret, DbEnv::strerror(ret)); + LogPrintf("%s: Error %d closing database environment: %s\n", __func__, ret, DbEnv::strerror(ret)); if (!fMockDb) DbEnv((u_int32_t)0).remove(strPath.c_str(), 0); } @@ -172,8 +172,12 @@ bool BerkeleyEnvironment::Open(bool retry) nEnvFlags, S_IRUSR | S_IWUSR); if (ret != 0) { - dbenv->close(0); - LogPrintf("BerkeleyEnvironment::Open: Error %d opening database environment: %s\n", ret, DbEnv::strerror(ret)); + LogPrintf("%s: Error %d opening database environment: %s\n", __func__, ret, DbEnv::strerror(ret)); + int ret2 = dbenv->close(0); + if (ret2 != 0) { + LogPrintf("%s: Error %d closing failed database environment: %s\n", __func__, ret2, DbEnv::strerror(ret2)); + } + Reset(); if (retry) { // try moving the database env out of the way fs::path pathDatabaseBak = pathIn / strprintf("database.%d.bak", GetTime()); @@ -499,8 +503,8 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo // be implemented, so no equality checks are needed at all. (Newer // versions of BDB have an set_lk_exclusive method for this // purpose, but the older version we use does not.) - for (auto& env : g_dbenvs) { - CheckUniqueFileid(env.second, strFilename, *pdb_temp); + for (const auto& env : g_dbenvs) { + CheckUniqueFileid(env.second, strFilename, *pdb_temp, this->env->m_fileids[strFilename]); } pdb = pdb_temp.release(); @@ -552,6 +556,7 @@ void BerkeleyBatch::Close() LOCK(cs_db); --env->mapFileUseCount[strFile]; } + env->m_db_in_use.notify_all(); } void BerkeleyEnvironment::CloseDb(const std::string& strFile) @@ -568,6 +573,32 @@ void BerkeleyEnvironment::CloseDb(const std::string& strFile) } } +void BerkeleyEnvironment::ReloadDbEnv() +{ + // Make sure that no Db's are in use + AssertLockNotHeld(cs_db); + std::unique_lock lock(cs_db); + m_db_in_use.wait(lock, [this](){ + for (auto& count : mapFileUseCount) { + if (count.second > 0) return false; + } + return true; + }); + + std::vector filenames; + for (auto it : mapDb) { + filenames.push_back(it.first); + } + // Close the individual Db's + for (const std::string& filename : filenames) { + CloseDb(filename); + } + // Reset the environment + Flush(true); // This will flush and close the environment + Reset(); + Open(true); +} + bool BerkeleyBatch::Rewrite(BerkeleyDatabase& database, const char* pszSkip) { if (database.IsDummy()) { @@ -693,7 +724,6 @@ void BerkeleyEnvironment::Flush(bool fShutdown) if (!fMockDb) { fs::remove_all(fs::path(strPath) / "database"); } - g_dbenvs.erase(strPath); } } } @@ -793,12 +823,24 @@ void BerkeleyDatabase::Flush(bool shutdown) { if (!IsDummy()) { env->Flush(shutdown); - if (shutdown) env = nullptr; + if (shutdown) { + LOCK(cs_db); + g_dbenvs.erase(env->Directory().string()); + env = nullptr; + } else { + // TODO: To avoid g_dbenvs.erase erasing the environment prematurely after the + // first database shutdown when multiple databases are open in the same + // environment, should replace raw database `env` pointers with shared or weak + // pointers, or else separate the database and environment shutdowns so + // environments can be shut down after databases. + env->m_fileids.erase(strFile); + } } } -void BerkeleyDatabase::CloseAndReset() +void BerkeleyDatabase::ReloadDbEnv() { - env->Close(); - env->Reset(); + if (!IsDummy()) { + env->ReloadDbEnv(); + } } diff --git a/src/wallet/db.h b/src/wallet/db.h index b34ec06732c41..447d51af90a3f 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -24,6 +25,11 @@ static const unsigned int DEFAULT_WALLET_DBLOGSIZE = 100; static const bool DEFAULT_WALLET_PRIVDB = true; +struct WalletDatabaseFileId { + u_int8_t value[DB_FILE_ID_LEN]; + bool operator==(const WalletDatabaseFileId& rhs) const; +}; + class BerkeleyEnvironment { private: @@ -37,6 +43,8 @@ class BerkeleyEnvironment std::unique_ptr dbenv; std::map mapFileUseCount; std::map mapDb; + std::unordered_map m_fileids; + std::condition_variable_any m_db_in_use; BerkeleyEnvironment(const fs::path& env_directory); ~BerkeleyEnvironment(); @@ -75,6 +83,7 @@ class BerkeleyEnvironment void CheckpointLSN(const std::string& strFile); void CloseDb(const std::string& strFile); + void ReloadDbEnv(); DbTxn* TxnBegin(int flags = DB_TXN_WRITE_NOSYNC) { @@ -143,11 +152,10 @@ class BerkeleyDatabase */ void Flush(bool shutdown); - /** Close and reset. - */ - void CloseAndReset(); - void IncrementUpdateCounter(); + + void ReloadDbEnv(); + std::atomic nUpdateCounter; unsigned int nLastSeen; unsigned int nLastFlushed; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index d4491475941dc..f070d7d04e9e8 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3630,7 +3630,6 @@ UniValue encryptwallet(const JSONRPCRequest& request) "will require the passphrase to be set prior the making these calls.\n" "Use the walletpassphrase call for this, and then walletlock call.\n" "If the wallet is already encrypted, use the walletpassphrasechange call.\n" - "Note that this will shutdown the server.\n" "\nArguments:\n" "1. \"passphrase\" (string) The pass phrase to encrypt the wallet with. It must be at least 1 character, but should be long.\n" @@ -3669,11 +3668,7 @@ UniValue encryptwallet(const JSONRPCRequest& request) if (!pwallet->EncryptWallet(strWalletPass)) throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: Failed to encrypt the wallet."); - // BDB seems to have a bad habit of writing old data into - // slack space in .dat files; that is bad if the old data is - // unencrypted private keys. So: - StartShutdown(); - return "wallet encrypted; pivx server stopping, restart to run with encrypted wallet. The keypool has been flushed, you need to make a new backup."; + return "wallet encrypted; The keypool has been flushed, you need to make a new backup."; } UniValue listunspent(const JSONRPCRequest& request) diff --git a/src/wallet/test/wallet_test_fixture.cpp b/src/wallet/test/wallet_test_fixture.cpp index aec006823ead0..240c4394ada8b 100644 --- a/src/wallet/test/wallet_test_fixture.cpp +++ b/src/wallet/test/wallet_test_fixture.cpp @@ -10,8 +10,10 @@ #include "wallet/rpcwallet.h" #include "wallet/wallet.h" -WalletTestingSetup::WalletTestingSetup(const std::string& chainName): - SaplingTestingSetup(chainName), m_wallet("mock", WalletDatabase::CreateMock()) +WalletTestingSetupBase::WalletTestingSetupBase(const std::string& chainName, + const std::string& wallet_name, + std::unique_ptr db) : + SaplingTestingSetup(chainName), m_wallet(wallet_name, std::move(db)) { bool fFirstRun; m_wallet.LoadWallet(fFirstRun); @@ -20,7 +22,10 @@ WalletTestingSetup::WalletTestingSetup(const std::string& chainName): RegisterWalletRPCCommands(tableRPC); } -WalletTestingSetup::~WalletTestingSetup() +WalletTestingSetupBase::~WalletTestingSetupBase() { UnregisterValidationInterface(&m_wallet); } + +WalletTestingSetup::WalletTestingSetup(const std::string& chainName) : + WalletTestingSetupBase(chainName, "mock", WalletDatabase::CreateMock()) {} diff --git a/src/wallet/test/wallet_test_fixture.h b/src/wallet/test/wallet_test_fixture.h index 6c76d5abddc65..a30068382e213 100644 --- a/src/wallet/test/wallet_test_fixture.h +++ b/src/wallet/test/wallet_test_fixture.h @@ -12,14 +12,20 @@ /** Testing setup and teardown for wallet. */ -struct WalletTestingSetup : public SaplingTestingSetup +struct WalletTestingSetupBase : public SaplingTestingSetup { - WalletTestingSetup(const std::string& chainName = CBaseChainParams::MAIN); - ~WalletTestingSetup(); - + WalletTestingSetupBase(const std::string& chainName, + const std::string& wallet_name, + std::unique_ptr db); + ~WalletTestingSetupBase(); CWallet m_wallet; }; +struct WalletTestingSetup : public WalletTestingSetupBase +{ + WalletTestingSetup(const std::string& chainName = CBaseChainParams::MAIN); +}; + struct WalletRegTestingSetup : public WalletTestingSetup { WalletRegTestingSetup() : WalletTestingSetup(CBaseChainParams::REGTEST) {} diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9409db15fe26f..3dec71b370986 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -879,6 +879,12 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) // Need to completely rewrite the wallet file; if we don't, bdb might keep // bits of the unencrypted private key in slack space in the database file. database->Rewrite(); + + // BDB seems to have a bad habit of writing old data into + // slack space in .dat files; that is bad if the old data is + // unencrypted private keys. So: + database->ReloadDbEnv(); + } NotifyStatusChanged(this); diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 6913386dbf0fe..578bc59d7c645 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -10,7 +10,6 @@ assert_raises_rpc_error, assert_greater_than, assert_greater_than_or_equal, - connect_nodes, count_bytes, find_vout_for_address, Decimal, @@ -371,11 +370,7 @@ def test_spend_2of2(self): def test_locked_wallet(self): self.log.info("test locked wallet") - self.nodes[1].node_encrypt_wallet("test") - self.start_node(1) - connect_nodes(self.nodes[0], 1) - connect_nodes(self.nodes[1], 2) - self.sync_all() + self.nodes[1].encryptwallet("test") # Drain the keypool. self.nodes[1].getnewaddress() diff --git a/test/functional/sapling_wallet_nullifiers.py b/test/functional/sapling_wallet_nullifiers.py index e5a2edeea3038..7d6309e41edb7 100755 --- a/test/functional/sapling_wallet_nullifiers.py +++ b/test/functional/sapling_wallet_nullifiers.py @@ -7,11 +7,7 @@ from decimal import Decimal from test_framework.test_framework import PivxTestFramework -from test_framework.util import assert_equal, assert_true, connect_nodes, get_coinstake_address - -def connect_nodes_bi(nodes, a, b): - connect_nodes(nodes[a], b) - connect_nodes(nodes[b], a) +from test_framework.util import assert_equal, assert_true, get_coinstake_address class WalletNullifiersTest(PivxTestFramework): @@ -45,14 +41,7 @@ def run_test (self): self.nodes[1].importsaplingkey(myzkey) # encrypt node 1 wallet and wait to terminate - self.nodes[1].node_encrypt_wallet("test") - - # restart node 1 - self.start_node(1, self.extra_args[1]) - connect_nodes_bi(self.nodes, 0, 1) - connect_nodes_bi(self.nodes, 2, 1) - connect_nodes_bi(self.nodes, 3, 1) - self.sync_all() + self.nodes[1].encryptwallet("test") # send node 0 shield addr to note 2 zaddr recipients = [] diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index bf5d9c8e1842a..ebccb5adc1ae8 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -277,14 +277,6 @@ def assert_debug_log(self, expected_msgs): if re.search(re.escape(expected_msg), log, flags=re.MULTILINE) is None: raise AssertionError('Expected message "{}" does not partially match log:\n\n{}\n\n'.format(expected_msg, print_log)) - def node_encrypt_wallet(self, passphrase): - """"Encrypts the wallet. - - This causes pivxd to shutdown, so this method takes - care of cleaning up resources.""" - self.encryptwallet(passphrase) - self.wait_until_stopped() - def add_p2p_connection(self, p2p_conn, *args, wait_for_verack=True, **kwargs): """Add a p2p connection to the node. diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index 188af82d1cee4..b0f767a87f182 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -26,7 +26,6 @@ assert_greater_than, assert_raises_rpc_error, bytes_to_hex_str, - connect_nodes, hex_str_to_bytes ) @@ -47,11 +46,9 @@ def set_test_params(self): def run_test(self): # Encrypt wallet for test_locked_wallet_fails test - self.nodes[1].node_encrypt_wallet(WALLET_PASSPHRASE) - self.start_node(1) + self.nodes[1].encryptwallet(WALLET_PASSPHRASE) self.nodes[1].walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT) - connect_nodes(self.nodes[0], 1) self.sync_all() peer_node, rbf_node = self.nodes diff --git a/test/functional/wallet_dump.py b/test/functional/wallet_dump.py index ab830a5fa5f60..4a61ce1dea301 100755 --- a/test/functional/wallet_dump.py +++ b/test/functional/wallet_dump.py @@ -107,8 +107,7 @@ def run_test (self): assert_equal(found_addr_rsv, 90 * 3) # 90 keys external plus 100% internal keys plus 100% staking keys #encrypt wallet, restart, unlock and dump - self.nodes[0].node_encrypt_wallet('test') - self.start_node(0) + self.nodes[0].encryptwallet('test') self.nodes[0].walletpassphrase('test', 10) # Should be a no-op: self.nodes[0].keypoolrefill() diff --git a/test/functional/wallet_encryption.py b/test/functional/wallet_encryption.py index 4b82a5689d58f..a8c4d6b54b718 100755 --- a/test/functional/wallet_encryption.py +++ b/test/functional/wallet_encryption.py @@ -31,8 +31,7 @@ def run_test(self): assert_equal(len(privkey), 52) # Encrypt the wallet - self.nodes[0].node_encrypt_wallet(passphrase) - self.start_node(0) + self.nodes[0].encryptwallet(passphrase) # Test that the wallet is encrypted assert_raises_rpc_error(-13, "Please enter the wallet passphrase with walletpassphrase first", self.nodes[0].dumpprivkey, address) diff --git a/test/functional/wallet_keypool.py b/test/functional/wallet_keypool.py index f3effc97cb977..427c2ed5188c4 100755 --- a/test/functional/wallet_keypool.py +++ b/test/functional/wallet_keypool.py @@ -21,9 +21,7 @@ def run_test(self): nodes[0].validateaddress(addr_before_encrypting) # Encrypt wallet and wait to terminate - nodes[0].node_encrypt_wallet('test') - # Restart node 0 - self.start_node(0, self.extra_args[0]) + nodes[0].encryptwallet('test') # Keep creating keys addr = nodes[0].getnewaddress() nodes[0].validateaddress(addr)