[wallet] Reopen CDBEnv after encryption instead of shutting down

achow101 commented at 9:29 pm on February 20, 2018: member

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 force-pushed on Feb 20, 2018

fanquake added the label Wallet on Feb 20, 2018

in src/wallet/db.cpp:594 in 6b2b3a7698 outdated

515+    for (auto it : mapDb) {
516+        filenames.push_back(it.first);
517+    }
518+    // Close the individual Db's
519+    for (std::string filename : filenames) {
520+        CloseDb(filename);

ryanofsky commented at 1:16 am on February 21, 2018:

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:

0m_cv_in_use.notify_all()

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

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

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 commented at 3:54 am on February 21, 2018: member

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

in src/wallet/db.h:38 in de939869f2 outdated

34@@ -35,10 +35,11 @@ class CDBEnv
35     void EnvShutdown();
36 
37 public:
38-    mutable CCriticalSection cs_db;
39+    std::recursive_mutex cs_db;

ryanofsky commented at 2:35 pm on February 21, 2018:

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.

achow101 commented at 3:32 pm on February 21, 2018:

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

ryanofsky commented at 5:45 pm on February 21, 2018:

#12493 (review)

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

What is the compiler error? The following compiles for me: c271de2d62881c0880bf64b830d72f686967bbfb (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.

achow101 commented at 6:19 pm on February 21, 2018:

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.

achow101 commented at 9:06 pm on February 21, 2018:

@ryanofsky I’ve used your commit.

in src/wallet/db.cpp:576 in de939869f2 outdated

511@@ -511,6 +512,15 @@ void CDBEnv::CloseDb(const std::string& strFile)
512 
513 void CDBEnv::ReloadDbEnv()
514 {
515+    // Make sure that no Db's are in use
516+    std::unique_lock<std::recursive_mutex> lock(cs_db);

ryanofsky commented at 2:39 pm on February 21, 2018:

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

achow101 commented at 3:51 pm on February 21, 2018:

Done

ryanofsky commented at 2:46 pm on February 21, 2018: member

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.

achow101 force-pushed on Feb 21, 2018

in src/wallet/wallet.cpp:722 in b2a0bb97bb outdated

689@@ -690,6 +690,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
690         // bits of the unencrypted private key in slack space in the database file.
691         dbw->Rewrite();
692 
693+        // BDB seems to have a bad habit of writing old data into

ryanofsky commented at 2:05 am on February 22, 2018:

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

achow101 commented at 2:48 am on February 22, 2018:

The comment was just copied over from rpcwallet.cpp.

in src/wallet/wallet.cpp:681 in b2a0bb97bb outdated

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

ryanofsky commented at 2:14 am on February 22, 2018:

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.

achow101 commented at 2:49 am on February 22, 2018:

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

ryanofsky commented at 2:09 pm on February 22, 2018:

#12493 (review)

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.

achow101 commented at 9:40 pm on February 22, 2018:

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

ryanofsky commented at 2:17 am on February 22, 2018: member

utACK 437d2dd56f81a6258243aca6ed137cbca9255b32

in src/qt/askpassphrasedialog.cpp:128 in 437d2dd56f outdated

122@@ -123,7 +123,7 @@ void AskPassphraseDialog::accept()
123                 {
124                     QMessageBox::warning(this, tr("Wallet encrypted"),
125                                          "<qt>" +
126-                                         tr("%1 will close now to finish the encryption process. "
127+                                         tr("Your wallet is now encrypted. "
128                                          "Remember that encrypting your wallet cannot fully protect "
129                                          "your bitcoins from being stolen by malware infecting your computer.").arg(tr(PACKAGE_NAME)) +

promag commented at 2:48 pm on February 26, 2018:

Remove .arg() since %1 was removed.

achow101 commented at 6:49 pm on February 26, 2018:

Done

in src/qt/askpassphrasedialog.cpp:124 in 437d2dd56f outdated

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

promag commented at 2:49 pm on February 26, 2018:

QMessageBox::information?

achow101 commented at 6:49 pm on February 26, 2018:

I don’t think it needs to change.

in src/wallet/db.cpp:589 in 437d2dd56f outdated

521+        }
522+        return true;
523+    });
524+
525+    std::vector<std::string> filenames;
526+    for (auto it : mapDb) {

promag commented at 2:55 pm on February 26, 2018:

Remove filenames and just iterate once, something like:

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

achow101 commented at 6:50 pm on February 26, 2018:

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

in src/wallet/db.cpp:766 in 437d2dd56f outdated

759@@ -733,3 +760,10 @@ void CWalletDBWrapper::Flush(bool shutdown)
760         env->Flush(shutdown);
761     }
762 }
763+
764+void CWalletDBWrapper::ReloadDbEnv()
765+{
766+    if(!IsDummy()) {

promag commented at 2:55 pm on February 26, 2018:

Nit, space after if.

achow101 commented at 6:49 pm on February 26, 2018:

Done

promag commented at 4:02 pm on February 26, 2018: member

Tested ACK 437d2dd.

Tested with multiple wallets and with different -walletdir.

achow101 force-pushed on Feb 26, 2018

achow101 commented at 5:56 pm on March 28, 2018: member

Rebased

achow101 force-pushed on Mar 28, 2018

achow101 force-pushed on Mar 31, 2018

achow101 commented at 4:16 pm on March 31, 2018: member

Fixed travis and rebased again.

achow101 commented at 9:38 pm on April 12, 2018: member

Rebased

achow101 force-pushed on Apr 12, 2018

achow101 commented at 4:36 am on May 5, 2018: member

Review beg

achow101 commented at 6:17 pm on June 12, 2018: member

Review Beg?

in src/wallet/db.cpp:576 in b766d7bb8f outdated

570@@ -570,6 +571,16 @@ void BerkeleyEnvironment::CloseDb(const std::string& strFile)
571 
572 void BerkeleyEnvironment::ReloadDbEnv()
573 {
574+    // Make sure that no Db's are in use
575+    AssertLockNotHeld(cs_db);
576+    std::unique_lock<std::recursive_mutex> lock(cs_db);

sipa commented at 0:02 am on July 14, 2018:

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.

achow101 commented at 0:04 am on July 14, 2018:

Done

achow101 force-pushed on Jul 14, 2018

in src/wallet/db.cpp:554 in 3cdc0ae104 outdated

550@@ -551,6 +551,7 @@ void BerkeleyBatch::Close()
551     {
552         LOCK(cs_db);
553         --env->mapFileUseCount[strFile];
554+        env->m_db_in_use.notify_all();

sipa commented at 0:09 am on July 14, 2018:

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

achow101 commented at 0:22 am on July 14, 2018:

Done

in src/wallet/db.cpp:589 in 3cdc0ae104 outdated

584+    std::vector<std::string> filenames;
585+    for (auto it : mapDb) {
586+        filenames.push_back(it.first);
587+    }
588+    // Close the individual Db's
589+    for (std::string filename : filenames) {

sipa commented at 0:10 am on July 14, 2018:

Nit: const std::string& filename.

achow101 commented at 0:22 am on July 14, 2018:

Done

sipa commented at 0:11 am on July 14, 2018: member

utACK 22c1c367062c960a28ca9b3b463b6ab6d96ebd02, just nits.

achow101 force-pushed on Jul 14, 2018

sipa commented at 0:26 am on July 14, 2018: member

utACK 06f17267ddafa089f093cfffc181d8c834d346e7

achow101 force-pushed on Jul 14, 2018

achow101 commented at 2:17 am on July 14, 2018: member

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

achow101 force-pushed on Jul 14, 2018

sipa commented at 9:57 pm on July 14, 2018: member

utACK bdad9c40595c1e4959ebce1b55894060b2d7c146

instagibbs approved

instagibbs commented at 5:26 pm on July 16, 2018: member

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 added the label Needs rebase on Aug 9, 2018

Move BerkeleyEnvironment deletion from internal method to callsite

Instead of having the object destroy itself, having the caller
destroy it.

a769461d5e

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.

5d296ac810

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.

d7637c5a3f

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.

c1dde3a949

achow101 commented at 6:39 pm on August 9, 2018: member

Rebased

achow101 force-pushed on Aug 9, 2018

DrahtBot removed the label Needs rebase on Aug 9, 2018

laanwj added this to the "Blockers" column in a project

in src/wallet/db.cpp:582 in c1dde3a949

577+{
578+    // Make sure that no Db's are in use
579+    AssertLockNotHeld(cs_db);
580+    std::unique_lock<CCriticalSection> lock(cs_db);
581+    m_db_in_use.wait(lock, [this](){
582+        for (auto& count : mapFileUseCount) {

promag commented at 9:15 am on August 19, 2018:

Don’t use iterator references?

promag commented at 9:58 pm on August 19, 2018: member

utACK c1dde3a.

PierreRochard commented at 0:08 am on August 30, 2018: contributor

Tested ACK c1dde3a949b36ce9c2155777b3fa1372e7ed97d8, 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 https://github.com/PierreRochard/bitcoin/commit/e9cd31c448622cd9030879706661bae5c4c6f3bb

I manually tested the thread safety when two wallet Dbs are in the same environment using @ryanofsky’s approach https://github.com/PierreRochard/bitcoin/commit/154dd6e147986c7ec8cbd005bb6e8e17a2e45ea7

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 commented at 0:36 am on August 30, 2018: member

@PierreRochard I’ll try to reproduce it.

laanwj removed this from the "Blockers" column in a project

jonasschnelli commented at 7:30 pm on August 30, 2018: contributor

utACK c1dde3a949b36ce9c2155777b3fa1372e7ed97d8

ken2812221 commented at 3:42 am on September 4, 2018: contributor

Tested ACK c1dde3a

laanwj merged this on Sep 14, 2018

laanwj closed this on Sep 14, 2018

laanwj referenced this in commit f09bc7ec98 on Sep 14, 2018

MarcoFalke added the label Needs release notes on Sep 14, 2018

in src/wallet/db.cpp:700 in a769461d5e outdated

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

ryanofsky commented at 5:56 pm on September 14, 2018:

In commit “Move BerkeleyEnvironment deletion from internal method to callsite” (a769461d5e37ddcb771ae836254fdc69177a28c4) @achow101 do remember if this change was this done for specific reason? It seems like an odd thing to include here.

achow101 commented at 6:38 pm on September 14, 2018:

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.

ryanofsky commented at 5:58 pm on September 14, 2018: member

utACK c1dde3a949b36ce9c2155777b3fa1372e7ed97d8

laanwj referenced this in commit d98777f302 on Oct 18, 2018

promag referenced this in commit 31e657b4f2 on Dec 6, 2018

promag referenced this in commit dcb032dcdf on Dec 6, 2018

promag referenced this in commit f455979eb1 on Mar 11, 2019

promag referenced this in commit 048fda2a66 on Mar 11, 2019

promag referenced this in commit 435df68c62 on Mar 11, 2019

promag referenced this in commit 1c98a758d0 on Mar 11, 2019

laanwj referenced this in commit 6cf81b01b4 on Mar 20, 2019

fanquake removed the label Needs release note on Mar 21, 2019

fanquake commented at 0:29 am on March 21, 2019: member

Release note added in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.18.0-Release-Notes-Draft/_compare/f36c2a4%5E...f36c2a4.

sidhujag referenced this in commit 6a807ebec4 on Mar 28, 2019

sidhujag referenced this in commit e0ef917ef8 on Mar 28, 2019

sidhujag referenced this in commit f3edb7fb9d on Mar 28, 2019

sidhujag referenced this in commit 2a244bb39d on Mar 28, 2019

sidhujag referenced this in commit 3ee64a0aa7 on Mar 28, 2019

uhliksk referenced this in commit 630430517f on Apr 21, 2019

uhliksk referenced this in commit 54e5662db4 on Apr 21, 2019

uhliksk referenced this in commit 12f8778541 on Apr 21, 2019

uhliksk referenced this in commit f74884a869 on Apr 21, 2019

Rishabh42 referenced this in commit ce3547fae6 on Apr 22, 2019

uhliksk referenced this in commit fc0ab2b1c4 on May 1, 2019

uhliksk referenced this in commit f6c5ddce2d on May 1, 2019

uhliksk referenced this in commit c9918ad938 on May 1, 2019

uhliksk referenced this in commit 89bd447e03 on May 1, 2019

jasonbcox referenced this in commit 8915b2f748 on Oct 25, 2019

deadalnix referenced this in commit 86b4474961 on Jan 19, 2020

xdustinface referenced this in commit 02e0b8e376 on Apr 4, 2021

xdustinface referenced this in commit dd80d4e9d7 on Apr 4, 2021

xdustinface referenced this in commit 7b4782fa8d on Apr 4, 2021

xdustinface referenced this in commit 5179599526 on Apr 5, 2021

xdustinface referenced this in commit e57e83aca4 on Apr 13, 2021

PastaPastaPasta referenced this in commit ee29fd7954 on Sep 18, 2021

PastaPastaPasta referenced this in commit 590886f0cc on Sep 24, 2021

PastaPastaPasta referenced this in commit f2dce0fb91 on Sep 28, 2021

kittywhiskers referenced this in commit fdef6755b1 on Oct 8, 2021

kittywhiskers referenced this in commit 13da055f5c on Oct 12, 2021

kittywhiskers referenced this in commit 0d15947f85 on Oct 16, 2021

kittywhiskers referenced this in commit 76af0435ca on Oct 16, 2021

kittywhiskers referenced this in commit 98a62e27cf on Oct 19, 2021

vijaydasmp referenced this in commit 8159eb8096 on Oct 21, 2021

pravblockc referenced this in commit 34219d5926 on Nov 18, 2021

DrahtBot locked this on Dec 16, 2021

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