Add unloadwallet RPC #13111

pull promag wants to merge 8 commits into bitcoin:master from promag:2018-04-unload-wallet changing 16 files +194 −22
  1. promag commented at 9:43 pm on April 28, 2018: member
    This patch adds wallet unload feature via RPC. It also adds UI support for unloaded wallets.
  2. promag force-pushed on Apr 28, 2018
  3. promag force-pushed on Apr 29, 2018
  4. promag force-pushed on Apr 29, 2018
  5. promag force-pushed on Apr 29, 2018
  6. promag force-pushed on Apr 29, 2018
  7. fanquake added the label Wallet on Apr 30, 2018
  8. fanquake added the label RPC/REST/ZMQ on Apr 30, 2018
  9. jnewbery added this to the "In progress" column in a project

  10. laanwj referenced this in commit 6378eef18f on May 24, 2018
  11. promag renamed this:
    [WIP] Add unloadwallet RPC
    Add unloadwallet RPC
    on May 24, 2018
  12. promag force-pushed on May 24, 2018
  13. promag force-pushed on May 24, 2018
  14. laanwj added this to the "Blockers" column in a project

  15. promag force-pushed on May 31, 2018
  16. promag force-pushed on Jun 1, 2018
  17. promag force-pushed on Jun 1, 2018
  18. promag commented at 10:32 pm on June 1, 2018: member
    Rebased, added release notes and functional tests.
  19. promag force-pushed on Jun 1, 2018
  20. promag commented at 10:56 pm on June 1, 2018: member

    If a wallet is loaded after being unloaded then a segfault is raised:

     0Process 10688 stopped
     1* thread [#2](/bitcoin-bitcoin/2/), name = 'bitcoin-httpworker', stop reason = EXC_BAD_ACCESS (code=1, address=0x608)
     2    frame [#0](/bitcoin-bitcoin/0/): 0x0000000100a0ad18 libdb_cxx-4.8.dylib`DbEnv::set_lg_dir(char const*) + 38
     3libdb_cxx-4.8.dylib`DbEnv::set_lg_dir:
     4->  0x100a0ad18 <+38>: callq  *0x608(%rdi)
     5    0x100a0ad1e <+44>: movl   %eax, %ebx
     6    0x100a0ad20 <+46>: testl  %ebx, %ebx
     7    0x100a0ad22 <+48>: je     0x100a0ad40               ; <+78>
     8Target 0: (bitcoind) stopped.
     9(lldb) bt
    10* thread [#2](/bitcoin-bitcoin/2/), name = 'bitcoin-httpworker', stop reason = EXC_BAD_ACCESS (code=1, address=0x608)
    11  * frame [#0](/bitcoin-bitcoin/0/): 0x0000000100a0ad18 libdb_cxx-4.8.dylib`DbEnv::set_lg_dir(char const*) + 38
    12    frame [#1](/bitcoin-bitcoin/1/): 0x000000010026f0bf bitcoind`BerkeleyEnvironment::Open(this=0x0000000101900e68, retry=<unavailable>) at db.cpp:150 [opt]
    13    frame [#2](/bitcoin-bitcoin/2/): 0x0000000100274a8a bitcoind`BerkeleyBatch::VerifyEnvironment(file_path=<unavailable>, errorStr="") at db.cpp:335 [opt]
    14    frame [#3](/bitcoin-bitcoin/3/): 0x000000010030bd1f bitcoind`CWallet::Verify(wallet_file="�.\n\0p\0\0\x19\0\0\0\0\0\0\0\0\0���\x7f\0\0\x19\0\0\0\0\0\0\0p�.\n\0p\0\0\x012�w�, salvage_wallet=<unavailable>, error_string=<unavailable>, warning_string="") at wallet.cpp:4022 [opt]
    15    frame [#4](/bitcoin-bitcoin/4/): 0x00000001002c1419 bitcoind`loadwallet(request=<unavailable>) at rpcwallet.cpp:3098 [opt]
    16    frame [#5](/bitcoin-bitcoin/5/): 0x00000001001820ba bitcoind`CRPCTable::execute(this=<unavailable>, request=0x000070000a2eaab0) const at server.cpp:497 [opt]
    17    frame [#6](/bitcoin-bitcoin/6/): 0x000000010001cc57 bitcoind`HTTPReq_JSONRPC(req=0x0000000101a7e1d0, (null)=<unavailable>) at httprpc.cpp:191 [opt]
    18    frame [#7](/bitcoin-bitcoin/7/): 0x000000010002977b bitcoind`HTTPWorkItem::operator()() [inlined] std::__1::function<bool (HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)>::operator(this=<unavailable>, __arg=0x0000000101a7e1d0, __arg=<unavailable>)(HTTPRequest*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) const at functional:1914 [opt]
    19    frame [#8](/bitcoin-bitcoin/8/): 0x000000010002976e bitcoind`HTTPWorkItem::operator(this=<unavailable>)() at httpserver.cpp:54 [opt]
    20    frame [#9](/bitcoin-bitcoin/9/): 0x000000010002b060 bitcoind`WorkQueue<HTTPClosure>::Run(this=0x0000000101801430) at httpserver.cpp:113 [opt]
    21    frame [#10](/bitcoin-bitcoin/10/): 0x000000010002c03a bitcoind`void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(WorkQueue<HTTPClosure>*), WorkQueue<HTTPClosure>*> >(void*) [inlined] decltype(std::__1::forward<void (*)(WorkQueue<HTTPClosure>*)>(fp)(std::__1::forward<WorkQueue<HTTPClosure>*>(fp0))) std::__1::__invoke<void (*)(WorkQueue<HTTPClosure>*), WorkQueue<HTTPClosure>*>(void (*&&)(WorkQueue<HTTPClosure>*), WorkQueue<HTTPClosure>*&&) at type_traits:4291 [opt]
    22    frame [#11](/bitcoin-bitcoin/11/): 0x000000010002c032 bitcoind`void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(WorkQueue<HTTPClosure>*), WorkQueue<HTTPClosure>*> >(void*) [inlined] void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(WorkQueue<HTTPClosure>*), WorkQueue<HTTPClosure>*, 2ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(WorkQueue<HTTPClosure>*), WorkQueue<HTTPClosure>*>&, std::__1::__tuple_indices<2ul>) at thread:336 [opt]
    23    frame [#12](/bitcoin-bitcoin/12/): 0x000000010002c032 bitcoind`void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(WorkQueue<HTTPClosure>*), WorkQueue<HTTPClosure>*> >(__vp=0x0000000101ad8af0) at thread:346 [opt]
    24    frame [#13](/bitcoin-bitcoin/13/): 0x00007fff77cd66c1 libsystem_pthread.dylib`_pthread_body + 340
    25    frame [#14](/bitcoin-bitcoin/14/): 0x00007fff77cd656d libsystem_pthread.dylib`_pthread_start + 377
    26    frame [#15](/bitcoin-bitcoin/15/): 0x00007fff77cd5c5d libsystem_pthread.dylib`thread_start + 13
    

    Edit: Fixed in last commit.

  21. promag force-pushed on Jun 1, 2018
  22. promag force-pushed on Jun 4, 2018
  23. laanwj commented at 7:27 am on June 5, 2018: member

    new warning:

    0/home/orion/projects/bitcoin/bitcoin/src/qt/bitcoingui.cpp:123:5: warning: field 'platformStyle' will be initialized after field 'm_wallet_selector_label' [-Wreorder]
    1    platformStyle(_platformStyle),
    2    ^
    31 warning generated.
    
  24. promag force-pushed on Jun 5, 2018
  25. in src/wallet/wallet.h:1104 in d968dc3fd1 outdated
    1100@@ -1101,6 +1101,8 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
    1101      * Address book entry changed.
    1102      * @note called with lock cs_wallet held.
    1103      */
    1104+    boost::signals2::signal<void ()> Unload;
    


    jnewbery commented at 4:16 pm on June 5, 2018:
    You’ve placed this between NotifyAddressBookChanged() and its comment. Please move it.

    promag commented at 1:34 am on June 7, 2018:
    Yap.
  26. ken2812221 commented at 10:27 am on June 6, 2018: contributor

    The wallet.dat might corrupt by running the script:

     0src/bitcoind -daemon -regtest
     1sleep 3
     2src/bitcoin-cli -regtest createwallet test
     3src/bitcoin-cli -regtest unloadwallet test
     4num=0
     5while [ $num -lt 1000 ]; do
     6    src/bitcoin-cli -regtest loadwallet test
     7    src/bitcoin-cli -regtest unloadwallet test
     8    num=$(($num+1))
     9    echo $num
    10done
    
  27. promag commented at 2:32 pm on June 6, 2018: member

    The wallet.dat might corrupt by running the script @ken2812221 corrupt how? Can’t reproduce.

  28. ken2812221 commented at 2:51 pm on June 6, 2018: contributor

    Oh, the wallet does not corrupt. It’s a libevent error, but the message says that the wallet corrupts. It might because I open so many connections at a short period of time, restart bitcoind fix the problem. Does not related to this PR

    02018-06-06T10:23:12Z  wallet                12033ms
    12018-06-06T10:23:12Z setKeyPool.size() = 2000
    22018-06-06T10:23:12Z mapWallet.size() = 0
    32018-06-06T10:23:12Z mapAddressBook.size() = 0
    42018-06-06T10:23:12Z libevent: Error from accept() call: Too many open files
    52018-06-06T10:23:12Z BerkeleyEnvironment::Close: Error -30973 closing database environment: BDB0087 DB_RUNRECOVERY: Fatal error, run database recovery
    62018-06-06T10:23:12Z Using BerkeleyDB version Berkeley DB 5.3.28: (September  9, 2013)
    72018-06-06T10:23:12Z Using wallet wallet.dat
    82018-06-06T10:23:12Z BerkeleyEnvironment::Open: LogDir=/home/user/.bitcoin/regtest/wallets/test/database ErrorFile=/home/user/.bitcoin/regtest/wallets/test/db.log
    
  29. promag commented at 2:55 pm on June 6, 2018: member
    02018-06-06T10:23:12Z BerkeleyEnvironment::Close: Error -30973 closing database environment: BDB0087 DB_RUNRECOVERY: Fatal error, run database recovery
    

    Is this related to the libevent error?

  30. ken2812221 commented at 3:12 pm on June 6, 2018: contributor
    2018-06-06T10:23:12Z libevent: Error from accept() call: Too many open files
    

    Probobly the wallet would fail to close after the libevent error. Restart the node fix the issue.

  31. in src/wallet/rpcwallet.cpp:3192 in d968dc3fd1 outdated
    3187+
    3188+    RemoveWallet(wallet);
    3189+    UnregisterValidationInterface(wallet.get());
    3190+    wallet->BlockUntilSyncedToCurrentChain();
    3191+    wallet->Flush(true);
    3192+    wallet->Unload();
    


    jnewbery commented at 3:57 pm on June 6, 2018:
    In commit rpc: Add unloadwallet RPC, this doesn’t build (CWallet.Unload() is only added in ui: Support wallets unloaded dynamically)

    promag commented at 1:33 am on June 7, 2018:
    Fixed.
  32. in test/functional/wallet_multiwallet.py:260 in d968dc3fd1 outdated
    255+        for wallet_name in self.nodes[0].listwallets():
    256+            self.nodes[0].unloadwallet(wallet_name)
    257+        assert_equal(self.nodes[0].listwallets(), [])
    258+
    259+        # Successfully load a previously unloaded wallet
    260+        self.nodes[0].loadwallet('w1')
    


    jnewbery commented at 4:02 pm on June 6, 2018:
    Please assert that wallet w1 appears in listwallets and is usable.

    promag commented at 1:33 am on June 7, 2018:
    Done.
  33. jnewbery commented at 4:02 pm on June 6, 2018: member

    There seems to be a bug where if you run loadwallet <wallet> and then unloadwallet <wallet>, <wallet> isn’t removed from the dropdown menu (if you call unloadwallet <wallet> on a wallet that was loaded at startup, it is removed.)

    In general, the GUI doesn’t seem to recognise unloading wallets that were loaded dynamically.

  34. promag force-pushed on Jun 7, 2018
  35. promag force-pushed on Jun 7, 2018
  36. in src/wallet/wallet.h:1101 in 3261367d3a outdated
    1096@@ -1097,6 +1097,13 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
    1097     //! Flush wallet (bitdb flush)
    1098     void Flush(bool shutdown=false);
    1099 
    1100+    /**
    1101+     * Wallet was unloaded.
    


    promag commented at 1:31 am on June 7, 2018:
    Incorrect comment 😪
  37. promag commented at 1:38 am on June 7, 2018: member

    @jnewbery thanks the review and testing. I believe I’ve addressed all your comments.

    wallet_model->moveToThread(thread()); was missing in the previous version which messed some QObject::connect.

    Also, calling RPC stop either with bitcoin-qt or bitcoind caused a segfault, now fixed.

    ACK on the unloadwallet usage calling without an argument defaults to the wallet referenced by the request URL?

  38. promag commented at 1:42 am on June 7, 2018: member
    0GUI: QObject::connect: Cannot queue arguments of type 'QVector<int>'
    1(Make sure 'QVector<int>' is registered using qRegisterMetaType().)
    

    @jonasschnelli this error was caused by incorrect WalletModel’s thread affinity, see above.

  39. promag force-pushed on Jun 7, 2018
  40. promag force-pushed on Jun 7, 2018
  41. promag force-pushed on Jun 7, 2018
  42. in src/wallet/rpcwallet.cpp:3191 in aa8150bb3b outdated
    3186+    if (!wallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
    3187+
    3188+    // Release the "main" shared pointer and prevent further notifications.
    3189+    // Note that any attempt to load the same wallet would fail until the wallet
    3190+    // is destroyed (see CheckUniqueFileid).
    3191+    RemoveWallet(wallet);
    


    jnewbery commented at 6:41 pm on June 8, 2018:
    Should we throw an error if RemoveWallet() returns false? (so if unloadwallet is called twice in quick succession there’s no chance of UnregisterValidationInterface() being called twice for the same wallet)

    promag commented at 8:51 pm on June 8, 2018:
    Indeed, will fix.

    promag commented at 10:50 am on June 12, 2018:
    Done.
  43. in test/functional/wallet_multiwallet.py:247 in aa8150bb3b outdated
    242+        assert_raises_rpc_error(-18, "Requested wallet does not exist or is not loaded", node.get_wallet_rpc("dummy").unloadwallet)
    243+
    244+        # Successfully unload the specified wallet name
    245+        self.nodes[0].unloadwallet("w1")
    246+        assert 'w1' not in self.nodes[0].listwallets()
    247+        w2.unloadwallet("w3")
    


    jnewbery commented at 6:44 pm on June 8, 2018:
    I’m not sure if this is desirable behaviour. Should it be possible to call unloadwallet wallet2 on wallet1’s endpoint?

    promag commented at 8:53 pm on June 8, 2018:
    So if the endpoint references a wallet then the RPC should have no arguments? If so LGTM.

    promag commented at 10:50 am on June 12, 2018:
    Done, although not sure how I could test it.
  44. in src/qt/bitcoingui.cpp:577 in aa8150bb3b outdated
    566@@ -566,6 +567,19 @@ bool BitcoinGUI::addWallet(WalletModel *walletModel)
    567     return walletFrame->addWallet(walletModel);
    568 }
    569 
    570+bool BitcoinGUI::removeWallet(WalletModel* walletModel)
    


    jnewbery commented at 7:38 pm on June 8, 2018:
    Should the m_wallet_selector Qlabel be hidden when the second last wallet is removed and there is only one wallet loaded? (and then re-revealed when a second wallet is loaded)?

    promag commented at 8:57 pm on June 8, 2018:
    If you don’t mind I would like to do it later. I have a couple of UI changes in the drawer.

    promag commented at 3:36 pm on June 18, 2018:
    @jnewbery done here.
  45. in src/qt/walletmodel.h:266 in aa8150bb3b outdated
    261@@ -261,6 +262,9 @@ class WalletModel : public QObject
    262     // Watch-only address added
    263     void notifyWatchonlyChanged(bool fHaveWatchonly);
    264 
    265+    // Signal that wallet is about to be removed
    266+    void unload();
    


    jnewbery commented at 8:22 pm on June 8, 2018:
    Is this unload() method doing anything?

    jnewbery commented at 5:58 pm on June 14, 2018:
    oops. Actually I think this declaration is required for BitcoinApplication to connect the removeWallet slot to the unload signal (https://github.com/bitcoin/bitcoin/pull/13111/files#diff-8c9d79ba40bf702f01685008550ac100R471). Is that right?
  46. jnewbery commented at 8:28 pm on June 8, 2018: member
    Looking very good. A few more comments inline
  47. promag commented at 8:55 pm on June 8, 2018: member
    @jnewbery thanks for the review, will address your comments. Also, I need to figure out the travis failure.
  48. promag force-pushed on Jun 11, 2018
  49. promag force-pushed on Jun 12, 2018
  50. rpc: Extract GetWalletNameFromJSONRPCRequest from GetWalletForJSONRPCRequest 537efe19e6
  51. promag force-pushed on Jun 12, 2018
  52. achow101 commented at 10:06 pm on June 12, 2018: member

    Tested ACK 4eb7e12ba833c78201cb32a2f4a075dd5407396c

    Tested ACK 6cb76924e58a4b49ccc519bdce1a74bb80d2f5f3

  53. promag commented at 0:21 am on June 13, 2018: member
    @achow101 if you tested the whole patch then the last commit is 6cb7692, GH sorts by date.
  54. achow101 commented at 0:24 am on June 13, 2018: member
    Oh, whoops. Yes, I tested the whole patch.
  55. laanwj commented at 3:39 pm on June 14, 2018: member
    Code changes LGTM - utACK 6cb76924e58a4b49ccc519bdce1a74bb80d2f5f3
  56. in test/functional/wallet_multiwallet.py:248 in 6cb76924e5 outdated
    243+        assert_raises_rpc_error(-8, "Cannot unload the requested wallet", w1.unloadwallet, "w2"),
    244+
    245+        # Successfully unload the specified wallet name
    246+        self.nodes[0].unloadwallet("w1")
    247+        assert 'w1' not in self.nodes[0].listwallets()
    248+        # assert 'w3' not in self.nodes[0].listwallets()
    


    jnewbery commented at 5:19 pm on June 14, 2018:
    Remove?
  57. in src/wallet/rpcwallet.cpp:69 in 6cb76924e5 outdated
    74@@ -66,11 +75,6 @@ bool EnsureWalletIsAvailable(CWallet * const pwallet, bool avoidException)
    75     if (pwallet) return true;
    76     if (avoidException) return false;
    77     if (!HasWallets()) {
    78-        // Note: It isn't currently possible to trigger this error because
    


    jnewbery commented at 5:20 pm on June 14, 2018:
    :kissing_smiling_eyes: :ok_hand:
  58. jnewbery commented at 7:07 pm on June 14, 2018: member

    As mentioned in IRC, WalletModel needs the unload() declaration readded.

    One other nit in the test.

  59. promag force-pushed on Jun 14, 2018
  60. promag commented at 8:47 pm on June 14, 2018: member
    @jnewbery thanks for the review!
  61. jonasschnelli commented at 10:42 am on June 18, 2018: contributor

    Issue QT:

    1. Start with a single (default) wallet
    2. Create or load a wallet ([default wallet] and [new_wallet] should be there in the dropdown).
    3. When [default wallet] is selected, call unloadwallet
    4. [default wallet] is still there in the RPC dropdown
    5. [default wallet] is still in the main window dropdown
    6. Selecting [default wallet] looks like one have selected [default wallet] but acctually new_wallet is selected
  62. promag force-pushed on Jun 18, 2018
  63. jonasschnelli commented at 1:14 pm on June 18, 2018: contributor
    Tested ACK 0496cfbf7b28bf65eb5726e5b119ff75d56e5df8 (after squash).
  64. rpc: Add unloadwallet RPC 6608c369b1
  65. test: Add functional tests for unloadwallet RPC 4940a20a46
  66. test: Wallet methods are disabled when no wallet is loaded ccbf7ae749
  67. doc: Add release notes for unloadwallet RPC 9f9b50d5fe
  68. ui: Support wallets unloaded dynamically 0ee77b2077
  69. bugfix: Remove dangling wallet env instance 0b82bac76d
  70. in src/wallet/rpcwallet.cpp:3184 in 0496cfbf7b outdated
    3178+            + HelpExampleRpc("unloadwallet", "wallet_name")
    3179+        );
    3180+    }
    3181+
    3182+    std::string wallet_name;
    3183+    if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) {
    


    jonasschnelli commented at 1:17 pm on June 18, 2018:
    nit: it’s unclear from the calls documentation that you can’t unload walletB by calling walletA-endpoint with unloadwallet walletB.

    promag commented at 3:36 pm on June 18, 2018:
    @jonasschnelli improved documentation.
  71. promag force-pushed on Jun 18, 2018
  72. PierreRochard commented at 2:45 pm on June 19, 2018: contributor

    Segfault with QT:

    1. Start with a single (default) wallet
    2. Create or load a wallet ([default wallet] and [new_wallet] should be there in the dropdown).
    3. When [new_wallet] is selected in the dropdown, call unloadwallet without any arguments

    image

    ~I’m still debugging it but~ I wasn’t able to debug it and wanted to bring it up and see if others can reproduce.

  73. promag commented at 11:28 pm on June 19, 2018: member
    @PierreRochard thanks for testing. I’m able to reproduce it. After your steps (which don’t harm), call generate 1. This is due to the missing deletes in https://github.com/bitcoin/bitcoin/blob/3f398d7a17f136cd4a67998406ca41a124ae2966/src/qt/walletview.cpp#L87-L90 which happens after https://github.com/bitcoin/bitcoin/pull/13111/files#diff-8c9d79ba40bf702f01685008550ac100R483. Will fix.
  74. bugfix: Delete walletView in WalletFrame::removeWallet fe65bdec23
  75. promag commented at 1:21 pm on June 20, 2018: member
    @PierreRochard actually WalletView was leaking. Added a separate commit since the problem already exists in master.
  76. jnewbery commented at 4:30 pm on June 20, 2018: member
    Tested ACK fe65bdec237776dbe094339509dfd2e63329a832 @jonasschnelli and @PierreRochard issues are fixed.
  77. PierreRochard commented at 5:27 pm on June 20, 2018: contributor

    @promag, thanks for the explanation, I really like this pull request. I think we should handle the case where GetWalletNameFromJSONRPCRequest returns false and there is no wallet_name name provided. Otherwise the endpoint chokes on wallet_name = request.params[0].get_str(); and provides an unexpected error to the user

    image

    Maybe the issue is that URI should be populated after unload/load, it currently isn’t:

    image

  78. promag commented at 5:49 pm on June 20, 2018: member
    @PierreRochard it can be improved in a follow up.
  79. PierreRochard commented at 6:08 pm on June 20, 2018: contributor
    That’s fair, tested ACK fe65bdec237776dbe094339509dfd2e63329a832
  80. jonasschnelli merged this on Jun 21, 2018
  81. jonasschnelli closed this on Jun 21, 2018

  82. jonasschnelli referenced this in commit 000abbb6b0 on Jun 21, 2018
  83. promag deleted the branch on Jun 21, 2018
  84. sipa removed this from the "Blockers" column in a project

  85. in src/wallet/wallet.cpp:86 in fe65bdec23
    78@@ -79,6 +79,15 @@ std::shared_ptr<CWallet> GetWallet(const std::string& name)
    79     return nullptr;
    80 }
    81 
    82+// Custom deleter for shared_ptr<CWallet>.
    83+static void ReleaseWallet(CWallet* wallet)
    84+{
    85+    LogPrintf("Releasing wallet %s\n", wallet->GetName());
    86+    wallet->BlockUntilSyncedToCurrentChain();
    


    ryanofsky commented at 6:40 pm on June 25, 2018:

    What’s the reason for this call? It seems very fragile because BlockUntilSyncedToCurrentChain may try to schedule a callback, but wallets currently get deleted after the scheduler is deleted:

    https://github.com/bitcoin/bitcoin/blob/baf3a3ab0c63b512b37d9f753768c1f020369088/src/init.cpp#L297-L299

    I was seeing this cause a crash due to a subtle change in behavior in one of my prs, which took me a long time to debug.


    promag commented at 8:53 pm on June 25, 2018:
    The initial idea was to avoid dangling pointers to the wallet in the validation queue. I agree that once in the deleter no more pointers to the wallet should exist. Do you suggest removing this call? Which PR are you referring?

    ryanofsky commented at 5:50 pm on June 26, 2018:

    The initial idea was to avoid dangling pointers to the wallet in the validation queue. @promag I think it would be better to just call UnregisterValidationInterface in this case. That looks like it would remove any wallet references from the notification code without needing to block wallet deletion on the scheduler thread or run additional notification handling code.

    I agree that once in the deleter no more pointers to the wallet should exist. Do you suggest removing this call? Which PR are you referring?

    Yesterday the problem I saw was in #10973. It slightly changed the way m_last_block_processed got set during rescans leading to a new SyncWithValidationInterfaceQueue() call in SyncWithValidationInterfaceQueue that doesn’t happen in master. The SyncWithValidationInterfaceQueue call would fail with an assert error due to signal scheduler being deleted before the wallet in the init code shown above.

    Today this is causing another problem in #11625. In this case, there seems to be a race condition there where the wallet sometimes gets deleted in a qt event thread after UnloadBlockIndex gets called in another thread.

  86. fanquake moved this from the "In progress" to the "Done" column in a project

  87. xdustinface referenced this in commit 61c36b5b60 on Mar 31, 2021
  88. xdustinface referenced this in commit 72349ea80c on Mar 31, 2021
  89. xdustinface referenced this in commit 5f44ba0c03 on Mar 31, 2021
  90. xdustinface referenced this in commit c376b64a4f on Apr 1, 2021
  91. PastaPastaPasta referenced this in commit a3cc51118e on Apr 1, 2021
  92. xdustinface referenced this in commit 39e572d4cf on Apr 4, 2021
  93. xdustinface referenced this in commit fcf250b6fd on Apr 4, 2021
  94. xdustinface referenced this in commit 75d375acb7 on Apr 5, 2021
  95. xdustinface referenced this in commit 0fe6666024 on Apr 13, 2021
  96. MarcoFalke locked this on Sep 8, 2021

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-03 13:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me