[wallet] Don’t shut down after encrypting the wallet #11678

pull achow101 wants to merge 3 commits into bitcoin:master from achow101:encrypt-no-restart changing 20 files +104 −49
  1. achow101 commented at 0:14 am on November 14, 2017: member

    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.

  2. fanquake added the label Wallet on Nov 14, 2017
  3. fanquake added the label GUI on Nov 14, 2017
  4. in src/wallet/rpcwallet.cpp:2530 in db7a0c966e outdated 
    2386@@ -2386,8 +2387,10 @@ UniValue encryptwallet(const JSONRPCRequest& request)
2387     // BDB seems to have a bad habit of writing old data into
2388     // slack space in .dat files; that is bad if the old data is
2389     // unencrypted private keys. So:
2390-    StartShutdown();
    meshcollider commented at 0:43 am on November 14, 2017:
    Above on line 2347 it says "Note that this will shutdown the server.\n" which should be removed now
    achow101 commented at 4:36 am on November 14, 2017:
    Done
  5. meshcollider commented at 1:00 am on November 14, 2017: contributor
    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
  6. achow101 force-pushed on Nov 14, 2017
  7. achow101 commented at 4:38 am on November 14, 2017: member

    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.

  8. achow101 force-pushed on Nov 14, 2017
  9. achow101 commented at 5:16 am on November 14, 2017: member
    Resolved the other issue too. I forgot to also reset the signals to use the new wallets too. No longer WIP.
  10. achow101 renamed this:
    [wip][wallet] Don't shut down after encrypting the wallet
    [wallet] Don't shut down after encrypting the wallet
    on Nov 14, 2017
  11. promag commented at 2:05 pm on November 14, 2017: member

    Concept ACK.

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

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

    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.

  13. achow101 commented at 8:04 pm on November 14, 2017: member

    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.

  14. jonasschnelli commented at 8:19 pm on November 14, 2017: contributor

    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)
  15. jonasschnelli commented at 11:23 pm on November 29, 2017: contributor
    Needs rebase
  16. achow101 force-pushed on Nov 30, 2017
  17. achow101 commented at 4:21 pm on November 30, 2017: member
    Rebased.
  18. achow101 force-pushed on Jan 10, 2018
  19. achow101 commented at 9:14 pm on January 10, 2018: member

    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.

  20. 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.
    eed429d237
  21. fixup! Reload wallets when encryptwallet is used from rpcconsole 7cb328d101
  22. achow101 force-pushed on Feb 14, 2018
  23. achow101 commented at 5:39 am on February 14, 2018: member

    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.

  24. promag commented at 8:52 am on February 14, 2018: member
    Will test.
  25. fixup! ifdef enable wallets (ewewewewewew) ca09b2b745
  26. achow101 force-pushed on Feb 15, 2018
  27. promag commented at 9:49 pm on February 15, 2018: member

    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.

  28. achow101 commented at 3:04 am on February 20, 2018: member
    Closing this so I can implement it the right way.
  29. achow101 closed this on Feb 20, 2018
  30. ryanofsky commented at 1:07 pm on February 20, 2018: member

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

  31. achow101 commented at 3:44 pm on February 20, 2018: member

    @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

  32. ryanofsky commented at 6:33 pm on February 20, 2018: member

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

  33. achow101 commented at 6:46 pm on February 20, 2018: member

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

  34. ryanofsky commented at 6:51 pm on February 20, 2018: member

    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#pullrequestreview-76464511. 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.

  35. achow101 commented at 7:57 pm on February 20, 2018: member

    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.

  36. achow101 commented at 8:26 pm on February 20, 2018: member
    @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.
  37. ryanofsky commented at 9:06 pm on February 20, 2018: member
    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.
  38. laanwj referenced this in commit f09bc7ec98 on Sep 14, 2018
  39. xdustinface referenced this in commit 02e0b8e376 on Apr 4, 2021
  40. xdustinface referenced this in commit dd80d4e9d7 on Apr 4, 2021
  41. xdustinface referenced this in commit 7b4782fa8d on Apr 4, 2021
  42. xdustinface referenced this in commit 5179599526 on Apr 5, 2021
  43. xdustinface referenced this in commit e57e83aca4 on Apr 13, 2021
  44. DrahtBot locked this on Sep 8, 2021


achow101 meshcollider promag ryanofsky jonasschnelli

Labels
GUI Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me