Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wallet] Reopen CDBEnv after encryption instead of shutting down #12493

Merged
merged 4 commits into from Sep 14, 2018
Merged
Diff settings

Always

Just for now

@@ -123,16 +123,15 @@ void AskPassphraseDialog::accept()
{
QMessageBox::warning(this, tr("Wallet encrypted"),

This comment has been minimized.

Copy link
@promag

promag Feb 26, 2018

Member

QMessageBox::information?

This comment has been minimized.

Copy link
@achow101

achow101 Feb 26, 2018

Author Member

I don't think it needs to change.

"<qt>" +
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.") +
"<br><br><b>" +
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.") +
"</b></qt>");
QApplication::quit();
}
else
{
Copy path View file
@@ -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<CCriticalSection> lock(cs_db);
m_db_in_use.wait(lock, [this](){
for (auto& count : mapFileUseCount) {

This comment has been minimized.

Copy link
@promag

promag Aug 19, 2018

Member

Don't use iterator references?

if (count.second > 0) return false;
}
return true;
});

std::vector<std::string> filenames;
for (auto it : mapDb) {

This comment has been minimized.

Copy link
@promag

promag Feb 26, 2018

Member

Remove filenames and just iterate once, something like:

while (!mapDb.empty()) {
  std::string filename = mapDb.begin().first;
  ...
}

This comment has been minimized.

Copy link
@achow101

achow101 Feb 26, 2018

Author Member

This doesn't work (causes test failures with encryptwallet). I think it's becauese mapDb is modified by CloseDb too.

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()) {
@@ -697,7 +724,6 @@ void BerkeleyEnvironment::Flush(bool fShutdown)
if (!fMockDb) {
fs::remove_all(fs::path(strPath) / "database");
}
g_dbenvs.erase(strPath);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 14, 2018

Contributor

In commit "Move BerkeleyEnvironment deletion from internal method to callsite" (a769461)

@achow101 do remember if this change was this done for specific reason? It seems like an odd thing to include here.

This comment has been minimized.

Copy link
@achow101

achow101 Sep 14, 2018

Author Member

IIRC there are two reasons for this. The first was that we needed this in the next commit to avoid having a destroyed db environment because we want to reopen it. The second reason that I don't think it is good for an object to destroy its own references as that's what it does here.

}
}
}
@@ -796,6 +822,17 @@ 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;
}
}
}

void BerkeleyDatabase::ReloadDbEnv()
{
if (!IsDummy()) {
env->ReloadDbEnv();
}
}
Copy path View file
@@ -38,6 +38,7 @@ class BerkeleyEnvironment
std::unique_ptr<DbEnv> dbenv;
std::map<std::string, int> mapFileUseCount;
std::map<std::string, Db*> mapDb;
std::condition_variable_any m_db_in_use;

BerkeleyEnvironment(const fs::path& env_directory);
~BerkeleyEnvironment();
@@ -75,6 +76,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)
{
@@ -145,6 +147,8 @@ class BerkeleyDatabase

void IncrementUpdateCounter();

void ReloadDbEnv();

std::atomic<unsigned int> nUpdateCounter;
unsigned int nLastSeen;
unsigned int nLastFlushed;
Copy path View file
@@ -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)
Copy path View file
@@ -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);

@@ -475,10 +475,8 @@ def run_test(self):

############################################################
# 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
@@ -268,14 +268,6 @@ def assert_start_raises_init_error(self, extra_args=None, expected_msg=None, mat
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.
@@ -36,8 +36,7 @@ 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_bi(self.nodes, 0, 1)
@@ -121,8 +121,7 @@ def run_test(self):
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()
@@ -30,8 +30,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)
@@ -20,9 +20,7 @@ def run_test(self):
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)
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.