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] Don't shut down after encrypting the wallet #11678

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
6 participants
@achow101
Copy link
Member

commented Nov 14, 2017

Instead of shutting down the entire software after encrypting a wallet, instead just stop and close all of the wallets and then reopen them. This will flush the wallets, clear them from memory, and then reopen them to ensure that no unencrypted keys remain after encrypting.

This is marked as WIP because there are a few bugs that I am still trying to figure out. ~~~is a locking bug that I'm still trying to figure out. After encrypting a wallet from the GUI, the GUI freezes. This appears to be a lock contention issue with cs_KeyStore at wallet/crypter.cpp:160. However I can't figure out why this is happening.~~~

@meshcollider
Copy link
Member

left a comment

Tested ACK on the RPC, I'm having a look into the GUI hang at the moment, it successfully stops, closes and opens the wallets and then hangs which I find weird

@@ -2386,8 +2387,10 @@ UniValue encryptwallet(const JSONRPCRequest& request)
// 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();

This comment has been minimized.

Copy link
@meshcollider

meshcollider Nov 14, 2017

Member

Above on line 2347 it says "Note that this will shutdown the server.\n" which should be removed now

This comment has been minimized.

Copy link
@achow101

achow101 Nov 14, 2017

Author Member

Done

@achow101 achow101 force-pushed the achow101:encrypt-no-restart branch Nov 14, 2017

@achow101

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2017

I resolved the hanging issue, but now there's another issue.

The hanging issue was because I forgot to set the CWallet pointers walletmodel, addresstablemodel, and transactiontablemodel to the new CWallet pointers that were created from reopening the wallet. So they were trying to lock on a cs_KeyStore that no longer existed.

@achow101 achow101 force-pushed the achow101:encrypt-no-restart branch Nov 14, 2017

@achow101

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2017

Resolved the other issue too. I forgot to also reset the signals to use the new wallets too. No longer WIP.

@achow101 achow101 changed the title [wip][wallet] Don't shut down after encrypting the wallet [wallet] Don't shut down after encrypting the wallet Nov 14, 2017

@promag

This comment has been minimized.

Copy link
Member

commented Nov 14, 2017

Concept ACK.

I get SIGABRT when I call encryptwallet from the debug window console. Here is the stack trace:

2017-11-14 14:02:32 Encrypting Wallet with an nDeriveIterations of 188608
2017-11-14 14:02:33 GUI: NotifyKeyStoreStatusChanged
2017-11-14 14:02:33 GUI: NotifyKeyStoreStatusChanged
2017-11-14 14:02:34 keypool added 2000 keys (1000 internal), size=2000 (1000 internal)
2017-11-14 14:02:35 CWallet::NewKeyPool rewrote keypool
2017-11-14 14:02:35 GUI: NotifyKeyStoreStatusChanged
2017-11-14 14:02:35 CDB::Rewrite: Rewriting wallet.dat...
2017-11-14 14:02:35 GUI: NotifyKeyStoreStatusChanged
2017-11-14 14:02:35 CDBEnv::Flush: Flush(true)
2017-11-14 14:02:35 CDBEnv::Flush: Flushing wallet.dat (refcount = 0)...
2017-11-14 14:02:35 CDBEnv::Flush: wallet.dat checkpoint
2017-11-14 14:02:35 CDBEnv::Flush: wallet.dat detach
2017-11-14 14:02:35 CDBEnv::Flush: wallet.dat closed
2017-11-14 14:02:35 CDBEnv::Flush: Flush(true) took              30ms
Assertion failed: (e == 0), function ~recursive_mutex, file /BuildRoot/Library/Caches/com.apple.xbs/Sources/libcxx/libcxx-307.5/src/mutex.cpp, line 86.
Process 24964 stopped
* thread #18, name = 'QThread', stop reason = signal SIGABRT
    frame #0: 0x00007fffcd6ddd42 libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill:
->  0x7fffcd6ddd42 <+10>: jae    0x7fffcd6ddd4c            ; <+20>
    0x7fffcd6ddd44 <+12>: movq   %rax, %rdi
    0x7fffcd6ddd47 <+15>: jmp    0x7fffcd6d6caf            ; cerror_nocancel
    0x7fffcd6ddd4c <+20>: retq
Target 0: (bitcoin-qt) stopped.
(lldb) bt
* thread #18, name = 'QThread', stop reason = signal SIGABRT
  * frame #0: 0x00007fffcd6ddd42 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fffcd7cb457 libsystem_pthread.dylib`pthread_kill + 90
    frame #2: 0x00007fffcd643420 libsystem_c.dylib`abort + 129
    frame #3: 0x00007fffcd60a893 libsystem_c.dylib`__assert_rtn + 320
    frame #4: 0x00007fffcc17b230 libc++.1.dylib`std::__1::recursive_mutex::~recursive_mutex() + 54
    frame #5: 0x00000001003e2393 bitcoin-qt`CWallet::~CWallet() [inlined] AnnotatedMixin<std::__1::recursive_mutex>::~AnnotatedMixin(this=0x000000010c050378) at sync.h:56 [opt]
    frame #6: 0x00000001003e238b bitcoin-qt`CWallet::~CWallet() [inlined] CCriticalSection::~CCriticalSection(this=0x000000010c050378) at sync.h:98 [opt]
    frame #7: 0x00000001003e2383 bitcoin-qt`CWallet::~CWallet() [inlined] CCriticalSection::~CCriticalSection(this=0x000000010c050378) at sync.h:96 [opt]
    frame #8: 0x00000001003e2383 bitcoin-qt`CWallet::~CWallet(this=0x000000010c0501b0) at wallet.h:773 [opt]
    frame #9: 0x00000001003dbc5c bitcoin-qt`CWallet::~CWallet() [inlined] CWallet::~CWallet(this=0x000000010c0501b0) at wallet.h:770 [opt]
    frame #10: 0x00000001003dbc57 bitcoin-qt`CWallet::~CWallet(this=0x000000010c0501b0) at wallet.h:770 [opt]
    frame #11: 0x000000010036290e bitcoin-qt`CloseWallets() at init.cpp:286 [opt]
    frame #12: 0x000000010039cd8f bitcoin-qt`encryptwallet(request=<unavailable>) at rpcwallet.cpp:2390 [opt]
    frame #13: 0x000000010027724a bitcoin-qt`CRPCTable::execute(this=<unavailable>, request=0x00007000105745b0) const at server.cpp:496 [opt]
    frame #14: 0x000000010006da53 bitcoin-qt`RPCConsole::RPCParseCommandLine(strResult="", strCommand="encryptwallet test\n", fExecute=<unavailable>, pstrFilteredOut="") at rpcconsole.cpp:314 [opt]
    frame #15: 0x000000010006bd5a bitcoin-qt`RPCExecutor::request(QString const&) [inlined] RPCConsole::RPCExecuteCommandLine(strResult="", strCommand="", pstrFilteredOut=<unavailable>) at rpcconsole.h:41 [opt]
    frame #16: 0x000000010006bd4e bitcoin-qt`RPCExecutor::request(this=0x00000001134357b0, command=<unavailable>) at rpcconsole.cpp:395 [opt]
    frame #17: 0x0000000101e0ade4 QtCore`QObject::event(QEvent*) + 788
    frame #18: 0x0000000101168e92 QtWidgets`QApplicationPrivate::notify_helper(QObject*, QEvent*) + 306
    frame #19: 0x000000010116a1af QtWidgets`QApplication::notify(QObject*, QEvent*) + 383
    frame #20: 0x0000000101de1f3f QtCore`QCoreApplication::notifyInternal2(QObject*, QEvent*) + 159
    frame #21: 0x0000000101de30d2 QtCore`QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) + 850
    frame #22: 0x0000000101e36419 QtCore`QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) + 73
    frame #23: 0x0000000101dddc92 QtCore`QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) + 418
    frame #24: 0x0000000101c1f3c1 QtCore`QThread::exec() + 113
    frame #25: 0x0000000101c231af QtCore`___lldb_unnamed_symbol300$$QtCore + 367
    frame #26: 0x00007fffcd7c893b libsystem_pthread.dylib`_pthread_body + 180
    frame #27: 0x00007fffcd7c8887 libsystem_pthread.dylib`_pthread_start + 286
    frame #28: 0x00007fffcd7c808d libsystem_pthread.dylib`thread_start + 13
@ryanofsky
Copy link
Contributor

left a comment

It would be good to fix whatever bugs prevent qt from being able to deal with opened and closed wallets, but it doesn't seem right that every wallet should be closed and reopened when one wallet is encrypted. I don't even see a reason why the wallet that is being encrypted needs have all it's in-memory state thrown away and then the same data reloaded from disk.

If the problem is just BDB "writing old data", it seems like we could add a CDBEnv::Reopen() method that would just close and reopen the DbEnv* and Db* pointers in the CDBEnv object. This might be a little tricky to implement (it would probably need to add a condition variable and wait for mapFileUseCount counters to go to 0, see CDB::PeriodicFlush), but I think it would add very little code and be faster and less buggy than unloading and reloading all kinds of things that don't need to be reloaded.

@achow101

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2017

but it doesn't seem right that every wallet should be closed and reopened when one wallet is encrypted.

This was easier to do since there are some things in the GUI that look specifically for vpwallets[0], so to close and reopen a wallet would require remove the pointer from that position in vpwallets and then replacing it with the newly opened wallet. That seemed to be more complicated to me than just closing and reopening all wallets, especially since order is more guaranteed to be preserved doing that than modifying vpwallets directly.

I don't even see a reason why the wallet that is being encrypted needs have all it's in-memory state thrown away and then the same data reloaded from disk.

I thought that there were still some in-memory things that needed to be discarded (e.g. unencrypted keys) to ensure security of the wallet.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Nov 14, 2017

Concept ACK.
My two worries:

  • let's hope no keys remain unencrypted in memory (I guess our approach is okay here)
  • concurrency especially with reduction of cs_main and the upcoming dynamic wallet loading (haven't checked the code)
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

Needs rebase

@achow101 achow101 force-pushed the achow101:encrypt-no-restart branch Nov 30, 2017

@achow101

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2017

Rebased.

@achow101 achow101 force-pushed the achow101:encrypt-no-restart branch to e6bb6bb Jan 10, 2018

@achow101

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

Rebased this.

The issue that @promag pointed out is because the GUI does not reset the walletmodel as it needs to when reloading the wallet when the rpc console is used to give the command. I'm not quite sure what the best approach for enabling that is though.

achow101 added some commits Nov 13, 2017

Keep the software running after encrypting a wallet
Insteading of shutting down the entire software after encrypting
a wallet, instead just stop and close all of the wallets and then
reopen them. This will flush the wallets, clear them from memory,
and then reopen them to ensure that no unencrypted keys remain
after encrypting.

@achow101 achow101 force-pushed the achow101:encrypt-no-restart branch from e6bb6bb to 7cb328d Feb 14, 2018

@achow101

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2018

Rebased.

I believe I fixed the issue mentioned earlier, although it is an extremely ugly hack that involves string comparisons. If anyone has a better suggestion, I'm all ears.

@promag

This comment has been minimized.

Copy link
Member

commented Feb 14, 2018

Will test.

@achow101 achow101 force-pushed the achow101:encrypt-no-restart branch to ca09b2b Feb 15, 2018

@promag
Copy link
Member

left a comment

NACK the current implementation. Models are instanced with the given wallet and should not be changed, instead new models should be instanced.

My suggestion is to build on top of #10740.

@achow101

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

Closing this so I can implement it the right way.

@achow101 achow101 closed this Feb 20, 2018

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

I don't even see a reason why the wallet that is being encrypted needs have all it's in-memory state thrown away and then the same data reloaded from disk.

I thought that there were still some in-memory things that needed to be discarded (e.g. unencrypted keys) to ensure security of the wallet.

I think before anything else, we should figure out exactly what needs to be cleared. For example, is there information in the DBEnv that needs to be cleared? If so, opening and closing the wallet will not clear it and a shutdown will still be needed (or the database improvements I suggested above).

@achow101

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

@ryanofsky The problem is described here: https://bitcointalk.org/index.php?topic=51474.0

I think the approach I was using works since it closes the wallets, resets the database environment, and then reopens the wallets. Specifically, I think the key part is the bitdb.Reset() call.

I wrote and used this script to check if there were any private keys being written to the log files: https://gist.github.com/achow101/7f7143e6c3d3fdc034d3470e72823e9d

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

@ryanofsky The problem is described here: https://bitcointalk.org/index.php?topic=51474.0

@achow101, then what were you referring to when you wrote "I thought that there were still some in-memory things that needed to be discarded"? I'd be interested to know why encryption can't be handled at the db/wallet layer instead of involving qt, rpc, hooks, callbacks, shared pointers or whatever else might be entailed in a new effort to "implement it the right way."

@achow101

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

@ryanofsky

then what were you referring to when you wrote "I thought that there were still some in-memory things that needed to be discarded"?

I was mistaken.

I'd be interested to know why encryption can't be handled at the db/wallet layer instead of involving qt, rpc, hooks, callbacks, shared pointers or whatever else might be entailed in a new effort to "implement it the right way."

The database environment needs to be reloaded, and AFAIK, you can't do this from within an individual wallet because reloading the database environment requires closing the databases of all wallets and reopening them. So it has to be done externally instead of within the db/wallet itself.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

it has to be done externally instead of within the db/wallet itself.

Would adding a method to close and reopen the database environment not work? This is what I was trying to suggest here: #11678 (review). I don't see what the problem would be. It would seem to require some interaction with the flush thread but otherwise could just be implemented at just the lowest layers and leave qt, rpc, and higher level wallet code untouched.

@achow101

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

Would adding a method to close and reopen the database environment not work?

I'm not sure that that would work. The CDB objects for each wallet would need to learn of the new Db pointer that was created on the reopen, but the CDBEnv doesn't know about the CDB objects nor does it need to (I think).

I can try that approach and see if I get anywhere with it.

@achow101

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

@ryanofsky I think it actually is possible to do that. I'll try it and make a PR if it works. It appears that I have misunderstood how the wallet handles Db stuff.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

Sounds good. I think you probably already found this, but the CDB objects are temporary, so you could just wait for them to be closed with mapFileUseCount. CDBEnv::Flush is kind of doing something like this already, although that code is messy and just gives up if the count is not 0.

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