From a769461d5e37ddcb771ae836254fdc69177a28c4 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 13 Jul 2018 19:15:30 -0700 Subject: [PATCH 1/4] Move BerkeleyEnvironment deletion from internal method to callsite Instead of having the object destroy itself, having the caller destroy it. --- src/wallet/db.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index d0fe51801..536243be4 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -697,7 +697,6 @@ void BerkeleyEnvironment::Flush(bool fShutdown) if (!fMockDb) { fs::remove_all(fs::path(strPath) / "database"); } - g_dbenvs.erase(strPath); } } } @@ -796,6 +795,10 @@ 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; + } } } From 5d296ac810755dc47f105eb95b52b7e2bcb8aea8 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 20 Feb 2018 15:28:42 -0500 Subject: [PATCH 2/4] Add function to close all Db's and reload the databae environment Adds a ReloadDbEnv function to BerkeleyEnvironment in order to close all Db instances, closes the environment, resets it, and then reopens the BerkeleyEnvironment. Also adds a ReloadDbEnv function to BerkeleyDatabase that calls BerkeleyEnvironment's ReloadDbEnv. --- src/wallet/db.cpp | 34 ++++++++++++++++++++++++++++++++++ src/wallet/db.h | 4 ++++ 2 files changed, 38 insertions(+) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 536243be4..2d57c92cc 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -556,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) @@ -572,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()) { @@ -802,3 +829,10 @@ void BerkeleyDatabase::Flush(bool shutdown) } } } + +void BerkeleyDatabase::ReloadDbEnv() +{ + if (!IsDummy()) { + env->ReloadDbEnv(); + } +} diff --git a/src/wallet/db.h b/src/wallet/db.h index b078edab7..467ed13b4 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -38,6 +38,7 @@ public: std::unique_ptr dbenv; std::map mapFileUseCount; std::map mapDb; + std::condition_variable_any m_db_in_use; BerkeleyEnvironment(const fs::path& env_directory); ~BerkeleyEnvironment(); @@ -75,6 +76,7 @@ public: void CheckpointLSN(const std::string& strFile); void CloseDb(const std::string& strFile); + void ReloadDbEnv(); DbTxn* TxnBegin(int flags = DB_TXN_WRITE_NOSYNC) { @@ -145,6 +147,8 @@ public: void IncrementUpdateCounter(); + void ReloadDbEnv(); + std::atomic nUpdateCounter; unsigned int nLastSeen; unsigned int nLastFlushed; From d7637c5a3f1d62922594cdfb6272e30dacf60ce9 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 20 Feb 2018 16:08:36 -0500 Subject: [PATCH 3/4] After encrypting the wallet, reload the database environment Calls ReloadDbEnv after encrypting the wallet so that the database environment is flushed, closed, and reopened to prevent unencrypted keys from being saved on disk. --- src/wallet/wallet.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 597d108ae..61d068622 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -719,6 +719,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) // 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); From c1dde3a949b36ce9c2155777b3fa1372e7ed97d8 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 20 Feb 2018 16:09:51 -0500 Subject: [PATCH 4/4] No longer shutdown after encrypting the wallet Since the database environment is flushed, closed, and reopened during EncryptWallet, there is no need to shut down the software anymore. --- src/qt/askpassphrasedialog.cpp | 5 ++--- src/wallet/rpcwallet.cpp | 7 +------ test/functional/rpc_fundrawtransaction.py | 6 ++---- test/functional/test_framework/test_node.py | 8 -------- test/functional/wallet_bumpfee.py | 3 +-- test/functional/wallet_dump.py | 3 +-- test/functional/wallet_encryption.py | 3 +-- test/functional/wallet_keypool.py | 4 +--- 8 files changed, 9 insertions(+), 30 deletions(-) diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp index 812d2251e..612331523 100644 --- a/src/qt/askpassphrasedialog.cpp +++ b/src/qt/askpassphrasedialog.cpp @@ -123,16 +123,15 @@ void AskPassphraseDialog::accept() { QMessageBox::warning(this, 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 bitcoins from being stolen by malware infecting your computer.").arg(tr(PACKAGE_NAME)) + + "your bitcoins 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. " "For security reasons, previous backups of the unencrypted wallet file " "will become useless as soon as you start using the new, encrypted wallet.") + ""); - QApplication::quit(); } else { diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 73dfebf11..a1d58aba0 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2729,7 +2729,6 @@ static 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" "\nExamples:\n" @@ -2767,11 +2766,7 @@ static UniValue encryptwallet(const JSONRPCRequest& request) 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; Bitcoin server stopping, restart to run with encrypted wallet. The keypool has been flushed and a new HD seed was generated (if you are using HD). You need to make a new backup."; + return "wallet encrypted; The keypool has been flushed and a new HD seed was generated (if you are using HD). You need to make a new backup."; } static UniValue lockunspent(const JSONRPCRequest& request) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index a806de43b..a4985a7a1 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -475,10 +475,8 @@ class RawTransactionsTest(BitcoinTestFramework): ############################################################ # locked wallet test - self.stop_node(0) - self.nodes[1].node_encrypt_wallet("test") - self.stop_node(2) - self.stop_node(3) + self.nodes[1].encryptwallet("test") + self.stop_nodes() self.start_nodes() # This test is not meant to test fee estimation and we'd like diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 56ea110f1..792d74de5 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -268,14 +268,6 @@ class TestNode(): assert_msg = "bitcoind should have exited with expected error " + expected_msg self._raise_assertion_error(assert_msg) - def node_encrypt_wallet(self, passphrase): - """"Encrypts the wallet. - - This causes bitcoind 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, *, 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 356393cfa..5a7a1375a 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -36,8 +36,7 @@ class BumpFeeTest(BitcoinTestFramework): 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_bi(self.nodes, 0, 1) diff --git a/test/functional/wallet_dump.py b/test/functional/wallet_dump.py index 63316d964..7ddfd2c80 100755 --- a/test/functional/wallet_dump.py +++ b/test/functional/wallet_dump.py @@ -121,8 +121,7 @@ class WalletDumpTest(BitcoinTestFramework): assert_equal(witness_addr_ret, witness_addr) # p2sh-p2wsh address added to the first key #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 cec966025..d5e797164 100755 --- a/test/functional/wallet_encryption.py +++ b/test/functional/wallet_encryption.py @@ -30,8 +30,7 @@ class WalletEncryptionTest(BitcoinTestFramework): 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 b00649162..1d8e0cdf3 100755 --- a/test/functional/wallet_keypool.py +++ b/test/functional/wallet_keypool.py @@ -20,9 +20,7 @@ class KeyPoolTest(BitcoinTestFramework): assert(addr_before_encrypting_data['hdseedid'] == wallet_info_old['hdseedid']) # Encrypt wallet and wait to terminate - nodes[0].node_encrypt_wallet('test') - # Restart node 0 - self.start_node(0) + nodes[0].encryptwallet('test') # Keep creating keys addr = nodes[0].getnewaddress() addr_data = nodes[0].getaddressinfo(addr)