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

Conversation

Projects
None yet
@achow101
Copy link
Member

achow101 commented Feb 20, 2018

This is the replacement for #11678 which implements @ryanofsky's suggestion.

Shutting down the software was to prevent the BDB environment from writing unencrypted private keys to disk in the database log files, as was noted here. This PR replaces the shutdown behavior with a CDBEnv flush, close, and reopen which achieves the same effect: everything is cleanly flushed and closed, the log files are removed, and then the environment reopened to continue normal operation.

To ensure that no unencrypted private keys are in the log files after encrypting the wallet, I wrote this script to pull private keys from the original wallet file and searches for these keys in the log files (note that you will have to change your file paths to make it work on your own machine).

As for concerns about private keys being written to slack space or being kept in memory, these behaviors no longer exist after the original wallet encryption PR and the shutting down solution from 2011.

cc @ryanofsky

@achow101 achow101 force-pushed the achow101:dbenv-reopen branch to b56e661 Feb 20, 2018

@fanquake fanquake added the Wallet label Feb 20, 2018

}
// Close the individual Db's
for (std::string filename : filenames) {
CloseDb(filename);

This comment has been minimized.

@ryanofsky

ryanofsky Feb 21, 2018

Contributor

This is a great start! I don't think think it is thread safe yet, though, because in parallel with this thread, a block connected notification could be coming in, or another another RPC could be being made that is using one of the Db* or DbEnv* pointers that this closes.

I think all you need to do to make this thread safe is wait for the mapFileUseCount entries to go to zero. You could do this with a condition variable. For example, if you added a std::condition_variable m_cv_in_use; member to CDBEnv you could trigger it in CDB::Close:

m_cv_in_use.notify_all()

and wait for it at the top of CDBEnv::ReloadDbEnv:

WAIT_LOCK(cs_db, lock);
m_cv_in_use.wait(lock, [this](){
    for (count : mapFileUseCount) {
        if (count.second > 0) return false;
    }
    return true;
});

This is one possible approach. Other approaches may be simpler or better. One drawback of this approach is that if there are a lot of background writes happening in different wallets, ReloadDbEnv could get starved out waiting for all wallets to be simultaneously not in use.

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Feb 21, 2018

@ryanofsky I've implemented something which is basically what you described. It seems to work, although I'm not sure how to test the thread safe-ness of it. Let me know what you think.

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

It seems to work, although I'm not sure how to test the thread safe-ness of it.

It'd be a little tricky to test the threadsafeness in a unit test. I think you'd need to insert hooks that would allow the test to block threads at particular points. If you want to test in a more ad-hoc way, though, I think you could do this by inserting a sleep in CDB::Write and starting bitcoin with two wallets loaded. You could then call an RPC that triggers a write in one wallet and hits the sleep. Then call an RPC on the other wallet to trigger ReloadDbEnv. If the condition variable is working correctly, the ReloadDbEnv call in the second wallet should get stuck until the sleep is over and the first wallet completes its write.

src/wallet/db.h Outdated
@@ -35,10 +35,11 @@ class CDBEnv
void EnvShutdown();

public:
mutable CCriticalSection cs_db;
std::recursive_mutex cs_db;

This comment has been minimized.

@ryanofsky

ryanofsky Feb 21, 2018

Contributor

In commit "Replace cs_db with a recursive mutex use with a conditional_variable_any"

cs_db is actually already a recursive mutex (it inherits from std::recursive_mutex). I think you could just leave it unchanged, and continue using the LOCK macro instead of std::lock_guard everywhere. This would make the commit smaller & simpler.

This comment has been minimized.

@achow101

achow101 Feb 21, 2018

Author Member

I tried that, but it kept giving me a compiler error.

This comment has been minimized.

@ryanofsky

ryanofsky Feb 21, 2018

Contributor

#12493 (comment)

I tried that, but it kept giving me a compiler error.

What is the compiler error? The following compiles for me: c271de2 (fetchable with git fetch https://github.com/ryanofsky/bitcoin pr/lreload).

I didn't do any real testing with it but it passes python & unit tests.

This comment has been minimized.

@achow101

achow101 Feb 21, 2018

Author Member

Ah, I was doing something with the macros and CCriticalSection. It was an error about not having an unlock method.

I'll test this out, but from testing I just did with the current code, it looks like it does work, so this should work too.

This comment has been minimized.

@achow101

achow101 Feb 21, 2018

Author Member

@ryanofsky I've used your commit.

src/wallet/db.cpp Outdated
@@ -511,6 +512,15 @@ void CDBEnv::CloseDb(const std::string& strFile)

void CDBEnv::ReloadDbEnv()
{
// Make sure that no Db's are in use
std::unique_lock<std::recursive_mutex> lock(cs_db);

This comment has been minimized.

@ryanofsky

ryanofsky Feb 21, 2018

Contributor

In commit "Replace cs_db with a recursive mutex use with a conditional_variable_any"

Would be good to add an AssertLockNotHeld(cs_db); above this, since it would be a bug if the mutex were held recursively when this was called (wait could hang if the lock wasn't released).

This comment has been minimized.

@achow101

achow101 Feb 21, 2018

Author Member

Done

@achow101 achow101 force-pushed the achow101:dbenv-reopen branch 2 times, most recently Feb 21, 2018

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK 437d2dd56f81a6258243aca6ed137cbca9255b32

@@ -690,6 +690,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
// bits of the unencrypted private key in slack space in the database file.
dbw->Rewrite();

// BDB seems to have a bad habit of writing old data into

This comment has been minimized.

@ryanofsky

ryanofsky Feb 22, 2018

Contributor

In commit "After encrypting the wallet, reload the database environment"

Delayed flushing seems more like a legitimate design tradeoff than a bad habit. Maybe just say something like "flush and reload the database environment here to clear out any data in memory that might be left behind after the rewrite above."

This comment has been minimized.

@achow101

achow101 Feb 22, 2018

Author Member

The comment was just copied over from rpcwallet.cpp.

@@ -690,6 +690,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
// bits of the unencrypted private key in slack space in the database file.
dbw->Rewrite();

This comment has been minimized.

@ryanofsky

ryanofsky Feb 22, 2018

Contributor

In commit "After encrypting the wallet, reload the database environment"

Looking at the CDB::Rewrite implementation I noticed that it already has a while loop that sleeps until mapFileUseCount (for just one wallet) is 0. You could take advantage of this if you wanted by tweaking the while loop there to wait for all use counts in the environment to be 0 and then calling ReloadDbEnv inside CDB::Rewrite. This would be a simplification since it would let you get rid of the new condition variable, and it would also make the new waiting code more consistent with previous code.

This is just a suggestion, though. Your current implementation seems fine, too.

This comment has been minimized.

@achow101

achow101 Feb 22, 2018

Author Member

I don't want to do anything to CDB::Rewrite since it is used in places other than encryptwallet.

This comment has been minimized.

@ryanofsky

ryanofsky Feb 22, 2018

Contributor

#12493 (comment)

I don't want to do anything to CDB::Rewrite since it is used in places other than encryptwallet

I didn't realize Rewrite was called other places, and the current approach does seem fine. But if you did want to unify the UseCount waiting logic, I think you could do it in a pretty clean way by adding a bool reload_env or similar option to CDB::Rewrite. I think in terms code clarity, having the option would actually be an improvement over always reloading.

This comment has been minimized.

@achow101

achow101 Feb 22, 2018

Author Member

I think I'll leave it as it is now.

@promag
Copy link
Member

promag left a comment

Tested ACK 437d2dd.

Tested with multiple wallets and with different -walletdir.

src/qt/askpassphrasedialog.cpp Outdated
@@ -123,7 +123,7 @@ void AskPassphraseDialog::accept()
{
QMessageBox::warning(this, tr("Wallet encrypted"),
"<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)) +

This comment has been minimized.

@promag

promag Feb 26, 2018

Member

Remove .arg() since %1 was removed.

This comment has been minimized.

@achow101

achow101 Feb 26, 2018

Author Member

Done

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

This comment has been minimized.

@promag

promag Feb 26, 2018

Member

QMessageBox::information?

This comment has been minimized.

@achow101

achow101 Feb 26, 2018

Author Member

I don't think it needs to change.

});

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

This comment has been minimized.

@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.

@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.

src/wallet/db.cpp Outdated

void CWalletDBWrapper::ReloadDbEnv()
{
if(!IsDummy()) {

This comment has been minimized.

@promag

promag Feb 26, 2018

Member

Nit, space after if.

This comment has been minimized.

@achow101

achow101 Feb 26, 2018

Author Member

Done

@achow101 achow101 force-pushed the achow101:dbenv-reopen branch Feb 26, 2018

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Mar 28, 2018

Rebased

@achow101 achow101 force-pushed the achow101:dbenv-reopen branch 2 times, most recently Mar 28, 2018

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Mar 31, 2018

Fixed travis and rebased again.

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Apr 12, 2018

Rebased

@achow101 achow101 force-pushed the achow101:dbenv-reopen branch 2 times, most recently Apr 12, 2018

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented May 5, 2018

Review beg

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Jun 12, 2018

Review Beg?

src/wallet/db.cpp Outdated
@@ -570,6 +571,16 @@ 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<std::recursive_mutex> lock(cs_db);

This comment has been minimized.

@sipa

sipa Jul 14, 2018

Member

It would be slightly cleaner to use std::unique_lock<CCriticalSection> here.

Also, please squash this into the first commit - currently your PR introduces a bug and then fixes it later.

We should probably also work on making the sync.h primitives more C++11 compatible, but let's leave that for later.

This comment has been minimized.

@achow101

achow101 Jul 14, 2018

Author Member

Done

@achow101 achow101 force-pushed the achow101:dbenv-reopen branch Jul 14, 2018

@sipa
Copy link
Member

sipa left a comment

utACK 22c1c367062c960a28ca9b3b463b6ab6d96ebd02, just nits.

src/wallet/db.cpp Outdated
@@ -551,6 +551,7 @@ void BerkeleyBatch::Close()
{
LOCK(cs_db);
--env->mapFileUseCount[strFile];
env->m_db_in_use.notify_all();

This comment has been minimized.

@sipa

sipa Jul 14, 2018

Member

Performance nit: notifying while not holding the lock is slightly faster (currently you'll wake the waiting thread, which then immediately needs to sleep again because the lock is still held).

This comment has been minimized.

@achow101

achow101 Jul 14, 2018

Author Member

Done

src/wallet/db.cpp Outdated
filenames.push_back(it.first);
}
// Close the individual Db's
for (std::string filename : filenames) {

This comment has been minimized.

@sipa

sipa Jul 14, 2018

Member

Nit: const std::string& filename.

This comment has been minimized.

@achow101

achow101 Jul 14, 2018

Author Member

Done

@achow101 achow101 force-pushed the achow101:dbenv-reopen branch Jul 14, 2018

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jul 14, 2018

utACK 06f17267ddafa089f093cfffc181d8c834d346e7

@achow101 achow101 force-pushed the achow101:dbenv-reopen branch Jul 14, 2018

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Jul 14, 2018

Rebased and added a commit to fix the silent merge conflict (test failure).

@achow101 achow101 force-pushed the achow101:dbenv-reopen branch Jul 14, 2018

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jul 14, 2018

utACK bdad9c40595c1e4959ebce1b55894060b2d7c146

@instagibbs
Copy link
Member

instagibbs left a comment

utACK

provided the locking behavior is as I expect, where the lock will be acquired without fail by the .wait. The documentation isn't super clear and I'm definitely not an expert.

@DrahtBot DrahtBot removed the Needs rebase label Aug 9, 2018

@laanwj laanwj added this to Blockers in High-priority for review Aug 16, 2018

@promag
Copy link
Member

promag left a comment

utACK c1dde3a.

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.

@promag

promag Aug 19, 2018

Member

Don't use iterator references?

@PierreRochard

This comment has been minimized.

Copy link
Contributor

PierreRochard commented Aug 30, 2018

Tested ACK c1dde3a, this is a good UX improvement, thanks @achow101

I manually tested that there were indeed private keys in the logs after encrypting with the environment reload commented out, tested that reloading the environment does remove the logs with a lightly modified version of @achow101's script PierreRochard@e9cd31c

I manually tested the thread safety when two wallet Dbs are in the same environment using @ryanofsky's approach PierreRochard@154dd6e

NB: I think I found a bug when setting the thread safety test up, the default wallet and a loaded wallet which are in the same directory will be in two different environments because the default wallet's BerkeleyEnvironment object's strPath member walletdir I passed in the config has a trailing slash but loadwallet regtest2.dat results in an environment without the trailing slash:
https://gist.github.com/PierreRochard/47ae74e5c0bc618a3b1a3f5ae5443196

@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 30, 2018

@PierreRochard I'll try to reproduce it.

@laanwj laanwj removed this from Blockers in High-priority for review Aug 30, 2018

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 30, 2018

utACK c1dde3a

@ken2812221

This comment has been minimized.

Copy link
Member

ken2812221 commented Sep 4, 2018

Tested ACK c1dde3a

@laanwj laanwj merged commit c1dde3a into bitcoin:master Sep 14, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Sep 14, 2018

Merge #12493: [wallet] Reopen CDBEnv after encryption instead of shut…
…ting down

c1dde3a No longer shutdown after encrypting the wallet (Andrew Chow)
d7637c5 After encrypting the wallet, reload the database environment (Andrew Chow)
5d296ac Add function to close all Db's and reload the databae environment (Andrew Chow)
a769461 Move BerkeleyEnvironment deletion from internal method to callsite (Andrew Chow)

Pull request description:

  This is the replacement for #11678 which implements @ryanofsky's [suggestion](#11678 (review)).

  Shutting down the software was to prevent the BDB environment from writing unencrypted private keys to disk in the database log files, as was noted [here](https://bitcointalk.org/index.php?topic=51474.msg616068#msg616068). This PR replaces the shutdown behavior with a CDBEnv flush, close, and reopen which achieves the same effect: everything is cleanly flushed and closed, the log files are removed, and then the environment reopened to continue normal operation.

  To ensure that no unencrypted private keys are in the log files after encrypting the wallet, I wrote [this script](https://gist.github.com/achow101/7f7143e6c3d3fdc034d3470e72823e9d) to pull private keys from the original wallet file and searches for these keys in the log files (note that you will have to change your file paths to make it work on your own machine).

  As for concerns about private keys being written to slack space or being kept in memory, these behaviors no longer exist after the original wallet encryption PR and the shutting down solution from 2011.

  cc @ryanofsky

Tree-SHA512: 34b894283b0677a873d06dee46dff8424dec85a2973009ac9b84bcf3d22d05f227c494168c395219d9aee3178e420cf70d4b3eeacc9785aa86b6015d25758e75
@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK c1dde3a

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

This comment has been minimized.

@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.

@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.

laanwj added a commit that referenced this pull request Oct 18, 2018

Merge #14146: wallet: Remove trailing separators from -walletdir arg
2d47163 wallet: Remove trailing separators from -walletdir arg (Pierre Rochard)
ea3009e wallet: Add walletdir arg unit tests (Pierre Rochard)

Pull request description:

  If a user passes in a path with a trailing separator as the `walletdir`, multiple BerkeleyEnvironments may be created in the same directory which can lead to data corruption.

  Discovered while reviewing #12493 (comment)

Tree-SHA512: f2bbf1749d904fd3f326b88f2ead58c8386034355910906d7faea155d518642e9cd4ceb3cae272f2d9d8feb61f126523e1c97502799d24e4315bb53e49fd7c09

@promag promag referenced this pull request Dec 6, 2018

Merged

0.17: Backport #14453 #14880

promag added a commit to promag/bitcoin that referenced this pull request Dec 6, 2018

qa: Ensure wallet unload during walletpassphrase timeout
0.17 branch doesn't include bitcoin#12493 which changed encryptwallet behavior. For that
reason the test is adjusted.

Github-Pull: bitcoin#14453
Rebased-From: 8907df9

bvbfan added a commit to bvbfan/bitcoin that referenced this pull request Feb 15, 2019

qa: Ensure wallet unload during walletpassphrase timeout
0.17 branch doesn't include bitcoin#12493 which changed encryptwallet behavior. For that
reason the test is adjusted.

Github-Pull: bitcoin#14453
Rebased-From: 8907df9

promag added a commit to promag/bitcoin that referenced this pull request Mar 11, 2019

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.

Github-Pull: bitcoin#12493
Rebased-From: 5d296ac

promag added a commit to promag/bitcoin that referenced this pull request Mar 11, 2019

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.

Github-Pull: bitcoin#12493
Rebased-From: d7637c5

promag added a commit to promag/bitcoin that referenced this pull request Mar 11, 2019

Move BerkeleyEnvironment deletion from internal method to callsite
Instead of having the object destroy itself, having the caller
destroy it.

Github-Pull: bitcoin#12493
Rebased-From: a769461

promag added a commit to promag/bitcoin that referenced this pull request Mar 11, 2019

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.

Github-Pull: bitcoin#12493
Rebased-From: c1dde3a

@promag promag referenced this pull request Mar 11, 2019

Merged

0.17: Backport 15297 #15575

LitecoinZ added a commit to litecoinz-core/litecoinz that referenced this pull request Mar 17, 2019

Backport bitcoin#15297
- bitcoin#12493 [wallet] Reopen CDBEnv after encryption instead of shutting down
- bitcoin#14350 Add WalletLocation class
- bitcoin#14320 [bugfix] wallet: Fix duplicate fileid detection
- bitcoin#14552 wallet: detecting duplicate wallet by comparing the db filename.
- bitcoin#11911 Free BerkeleyEnvironment instances when not in use
- bitcoin#15297 wallet: Releases dangling files on BerkeleyEnvironment::Close

laanwj added a commit that referenced this pull request Mar 20, 2019

Merge #15575: 0.17: Backport 15297
fe95f84 qa: Test .walletlock file is closed (João Barbosa)
2e9e904 wallet: Close wallet env lock file (João Barbosa)
22cdb6c wallet: Close dbenv error file db.log (João Barbosa)
f20513b Tests: add unit tests for GetWalletEnv (Pierre Rochard)
85c6263 Trivial: add doxygen-compatible comments relating to BerkeleyEnvironment (Pierre Rochard)
f22d02f Free BerkeleyEnvironment instances when not in use (Russell Yanofsky)
0a9af2d wallet: Create IsDatabaseLoaded function (Chun Kuan Lee)
7751ea3 Refactor: Move m_db pointers into BerkeleyDatabase (Russell Yanofsky)
caf1146 wallet: Add trailing wallet.dat when detecting duplicate wallet if it's a directory. (Chun Kuan Lee)
34da2b7 tests: add test case for loading copied wallet twice (Chun Kuan Lee)
8965b6a wallet: Fix duplicate fileid (Chun Kuan Lee)
16e5759 wallet: Refactor to use WalletLocation (João Barbosa)
21693ff wallet: Add WalletLocation utility class (João Barbosa)
1c98a75 No longer shutdown after encrypting the wallet (Andrew Chow)
435df68 Move BerkeleyEnvironment deletion from internal method to callsite (Andrew Chow)
048fda2 After encrypting the wallet, reload the database environment (Andrew Chow)
f455979 Add function to close all Db's and reload the databae environment (Andrew Chow)

Pull request description:

  This PR backports the following pull requests:
   - #12493 [wallet] Reopen CDBEnv after encryption instead of shutting down
   - #14350 Add WalletLocation class
   - #14320 [bugfix] wallet: Fix duplicate fileid detection
   - #14552 wallet: detecting duplicate wallet by comparing the db filename.
   - #11911 Free BerkeleyEnvironment instances when not in use
   - #15297 wallet: Releases dangling files on BerkeleyEnvironment::Close

Tree-SHA512: 52d759bc4f140ca96e39b37746cc20e786741b08ddc658a87ea77fbcfbb481f1c7b75aba4fc57ca9bca8ca7154e535da1fdd650fd114873655cd85c490c79f14
@fanquake

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.