Add unloadwallet RPC #13111

promag · 2018-04-28T21:43:11Z

This patch adds wallet unload feature via RPC. It also adds UI support for unloaded wallets.

80b4910 wallet: Use shared pointer to retain wallet instance (João Barbosa) Pull request description: Currently there are 3 places where it makes sense to retain a wallet shared pointer: - `vpwallets`; - `interfaces::Wallet` interface instance - used by the UI; - wallet RPC functions - given by `GetWalletForJSONRPCRequest`. The way it is now it is possible to have, for instance, listunspent RPC and in parallel unload the wallet (once #13111 is merged) without blocking. Once the RPC finishes, the shared pointer will release the wallet. It is also possible to get all existing wallets without blocking because the caller keeps a local list of shared pointers. This is mostly relevant for wallet unloading. This PR replaces #11402. Tree-SHA512: b7e37c7e1ab56626085afe2d40b1628e8d4f0dbda08df01b7e618ecd2d894ce9b83d4219443f444ba889096286eff002f163cb0a48f37063b62e9ba4ccfa6cce

promag · 2018-06-01T22:32:08Z

Rebased, added release notes and functional tests.

promag · 2018-06-01T22:56:19Z

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

Process 10688 stopped
* thread #2, name = 'bitcoin-httpworker', stop reason = EXC_BAD_ACCESS (code=1, address=0x608)
    frame #0: 0x0000000100a0ad18 libdb_cxx-4.8.dylib`DbEnv::set_lg_dir(char const*) + 38
libdb_cxx-4.8.dylib`DbEnv::set_lg_dir:
->  0x100a0ad18 <+38>: callq  *0x608(%rdi)
    0x100a0ad1e <+44>: movl   %eax, %ebx
    0x100a0ad20 <+46>: testl  %ebx, %ebx
    0x100a0ad22 <+48>: je     0x100a0ad40               ; <+78>
Target 0: (bitcoind) stopped.
(lldb) bt
* thread #2, name = 'bitcoin-httpworker', stop reason = EXC_BAD_ACCESS (code=1, address=0x608)
  * frame #0: 0x0000000100a0ad18 libdb_cxx-4.8.dylib`DbEnv::set_lg_dir(char const*) + 38
    frame #1: 0x000000010026f0bf bitcoind`BerkeleyEnvironment::Open(this=0x0000000101900e68, retry=<unavailable>) at db.cpp:150 [opt]
    frame #2: 0x0000000100274a8a bitcoind`BerkeleyBatch::VerifyEnvironment(file_path=<unavailable>, errorStr="") at db.cpp:335 [opt]
    frame #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]
    frame #4: 0x00000001002c1419 bitcoind`loadwallet(request=<unavailable>) at rpcwallet.cpp:3098 [opt]
    frame #5: 0x00000001001820ba bitcoind`CRPCTable::execute(this=<unavailable>, request=0x000070000a2eaab0) const at server.cpp:497 [opt]
    frame #6: 0x000000010001cc57 bitcoind`HTTPReq_JSONRPC(req=0x0000000101a7e1d0, (null)=<unavailable>) at httprpc.cpp:191 [opt]
    frame #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]
    frame #8: 0x000000010002976e bitcoind`HTTPWorkItem::operator(this=<unavailable>)() at httpserver.cpp:54 [opt]
    frame #9: 0x000000010002b060 bitcoind`WorkQueue<HTTPClosure>::Run(this=0x0000000101801430) at httpserver.cpp:113 [opt]
    frame #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]
    frame #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]
    frame #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]
    frame #13: 0x00007fff77cd66c1 libsystem_pthread.dylib`_pthread_body + 340
    frame #14: 0x00007fff77cd656d libsystem_pthread.dylib`_pthread_start + 377
    frame #15: 0x00007fff77cd5c5d libsystem_pthread.dylib`thread_start + 13

Edit: Fixed in last commit.

laanwj · 2018-06-05T07:27:54Z

new warning:

/home/orion/projects/bitcoin/bitcoin/src/qt/bitcoingui.cpp:123:5: warning: field 'platformStyle' will be initialized after field 'm_wallet_selector_label' [-Wreorder]
    platformStyle(_platformStyle),
    ^
1 warning generated.

ken2812221 · 2018-06-06T10:27:54Z

The wallet.dat might corrupt by running the script:

src/bitcoind -daemon -regtest
sleep 3
src/bitcoin-cli -regtest createwallet test
src/bitcoin-cli -regtest unloadwallet test
num=0
while [ $num -lt 1000 ]; do
    src/bitcoin-cli -regtest loadwallet test
    src/bitcoin-cli -regtest unloadwallet test
    num=$(($num+1))
    echo $num
done

promag · 2018-06-06T14:32:14Z

The wallet.dat might corrupt by running the script

@ken2812221 corrupt how? Can't reproduce.

ken2812221 · 2018-06-06T14:51:05Z

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

2018-06-06T10:23:12Z  wallet                12033ms
2018-06-06T10:23:12Z setKeyPool.size() = 2000
2018-06-06T10:23:12Z mapWallet.size() = 0
2018-06-06T10:23:12Z mapAddressBook.size() = 0
2018-06-06T10:23:12Z libevent: Error from accept() call: Too many open files
2018-06-06T10:23:12Z BerkeleyEnvironment::Close: Error -30973 closing database environment: BDB0087 DB_RUNRECOVERY: Fatal error, run database recovery
2018-06-06T10:23:12Z Using BerkeleyDB version Berkeley DB 5.3.28: (September  9, 2013)
2018-06-06T10:23:12Z Using wallet wallet.dat
2018-06-06T10:23:12Z BerkeleyEnvironment::Open: LogDir=/home/user/.bitcoin/regtest/wallets/test/database ErrorFile=/home/user/.bitcoin/regtest/wallets/test/db.log

promag · 2018-06-06T14:55:15Z

2018-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?

ken2812221 · 2018-06-06T15:12:37Z

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.

jnewbery

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.

jnewbery · 2018-06-05T16:16:12Z

src/wallet/wallet.h

@@ -1101,6 +1101,8 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
     * Address book entry changed.
     * @note called with lock cs_wallet held.
     */
+    boost::signals2::signal<void ()> Unload;


You've placed this between NotifyAddressBookChanged() and its comment. Please move it.

jnewbery · 2018-06-06T15:57:23Z

src/wallet/rpcwallet.cpp

+    UnregisterValidationInterface(wallet.get());
+    wallet->BlockUntilSyncedToCurrentChain();
+    wallet->Flush(true);
+    wallet->Unload();


In commit rpc: Add unloadwallet RPC, this doesn't build (CWallet.Unload() is only added in ui: Support wallets unloaded dynamically)

jnewbery · 2018-06-06T16:02:01Z

test/functional/wallet_multiwallet.py

+        assert_equal(self.nodes[0].listwallets(), [])
+
+        # Successfully load a previously unloaded wallet
+        self.nodes[0].loadwallet('w1')


Please assert that wallet w1 appears in listwallets and is usable.

PierreRochard · 2018-06-19T14:45:33Z

Segfault with QT:

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

Process 87183 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
    frame #0: 0x00000001000b3edc bitcoin-qt`SendCoinsDialog::updateSmartFeeLabel(this=0x0000000124329810) at sendcoinsdialog.cpp:697 [opt]
   694 	    coin_control.m_feerate.reset(); // Explicitly use only fee estimation rate for smart fee labels
   695 	    int returned_target;
   696 	    FeeReason reason;
-> 697 	    CFeeRate feeRate = CFeeRate(model->wallet().getMinimumFee(1000, coin_control, &returned_target, &reason));
   698 	
   699 	    ui->labelSmartFee->setText(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), feeRate.GetFeePerK()) + "/kB");
   700

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

promag · 2018-06-19T23:28:25Z

@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

bitcoin/src/qt/walletview.cpp

Lines 87 to 90 in 3f398d7

    
           WalletView::~WalletView() 
        
           { 
        
           }

which happens after https://github.com/bitcoin/bitcoin/pull/13111/files#diff-8c9d79ba40bf702f01685008550ac100R483. Will fix.

promag · 2018-06-20T13:21:37Z

@PierreRochard actually WalletView was leaking. Added a separate commit since the problem already exists in master.

jnewbery · 2018-06-20T16:30:06Z

Tested ACK fe65bde

@jonasschnelli and @PierreRochard issues are fixed.

PierreRochard · 2018-06-20T17:27:48Z

@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

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

promag · 2018-06-20T17:49:35Z

@PierreRochard it can be improved in a follow up.

PierreRochard · 2018-06-20T18:08:36Z

That's fair, tested ACK fe65bde

fe65bde bugfix: Delete walletView in WalletFrame::removeWallet (João Barbosa) 0b82bac bugfix: Remove dangling wallet env instance (João Barbosa) 0ee77b2 ui: Support wallets unloaded dynamically (João Barbosa) 9f9b50d doc: Add release notes for unloadwallet RPC (João Barbosa) ccbf7ae test: Wallet methods are disabled when no wallet is loaded (João Barbosa) 4940a20 test: Add functional tests for unloadwallet RPC (João Barbosa) 6608c36 rpc: Add unloadwallet RPC (João Barbosa) 537efe1 rpc: Extract GetWalletNameFromJSONRPCRequest from GetWalletForJSONRPCRequest (João Barbosa) Pull request description: This patch adds wallet unload feature via RPC. It also adds UI support for unloaded wallets. Tree-SHA512: 7c7f9f32f7a2266d2df574aa6b95f993c3dc82736f93304562122beb8756fb28cd22d03866b48f493c747441f22d30e196b098dec435cc25e035633f090351ea

ryanofsky · 2018-06-25T18:40:02Z

src/wallet/wallet.cpp

+static void ReleaseWallet(CWallet* wallet)
+{
+    LogPrintf("Releasing wallet %s\n", wallet->GetName());
+    wallet->BlockUntilSyncedToCurrentChain();


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:

bitcoin/src/init.cpp

Lines 297 to 299 in baf3a3a

GetMainSignals().UnregisterBackgroundSignalScheduler();

GetMainSignals().UnregisterWithMempoolSignals(mempool);

g_wallet_init_interface.Close();

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.

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?

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.

80b4910 wallet: Use shared pointer to retain wallet instance (João Barbosa) Pull request description: Currently there are 3 places where it makes sense to retain a wallet shared pointer: - `vpwallets`; - `interfaces::Wallet` interface instance - used by the UI; - wallet RPC functions - given by `GetWalletForJSONRPCRequest`. The way it is now it is possible to have, for instance, listunspent RPC and in parallel unload the wallet (once bitcoin#13111 is merged) without blocking. Once the RPC finishes, the shared pointer will release the wallet. It is also possible to get all existing wallets without blocking because the caller keeps a local list of shared pointers. This is mostly relevant for wallet unloading. This PR replaces bitcoin#11402. Tree-SHA512: b7e37c7e1ab56626085afe2d40b1628e8d4f0dbda08df01b7e618ecd2d894ce9b83d4219443f444ba889096286eff002f163cb0a48f37063b62e9ba4ccfa6cce

) 80b4910 wallet: Use shared pointer to retain wallet instance (João Barbosa) Pull request description: Currently there are 3 places where it makes sense to retain a wallet shared pointer: - `vpwallets`; - `interfaces::Wallet` interface instance - used by the UI; - wallet RPC functions - given by `GetWalletForJSONRPCRequest`. The way it is now it is possible to have, for instance, listunspent RPC and in parallel unload the wallet (once bitcoin#13111 is merged) without blocking. Once the RPC finishes, the shared pointer will release the wallet. It is also possible to get all existing wallets without blocking because the caller keeps a local list of shared pointers. This is mostly relevant for wallet unloading. This PR replaces bitcoin#11402. Tree-SHA512: b7e37c7e1ab56626085afe2d40b1628e8d4f0dbda08df01b7e618ecd2d894ce9b83d4219443f444ba889096286eff002f163cb0a48f37063b62e9ba4ccfa6cce Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>

fe65bde bugfix: Delete walletView in WalletFrame::removeWallet (João Barbosa) 0b82bac bugfix: Remove dangling wallet env instance (João Barbosa) 0ee77b2 ui: Support wallets unloaded dynamically (João Barbosa) 9f9b50d doc: Add release notes for unloadwallet RPC (João Barbosa) ccbf7ae test: Wallet methods are disabled when no wallet is loaded (João Barbosa) 4940a20 test: Add functional tests for unloadwallet RPC (João Barbosa) 6608c36 rpc: Add unloadwallet RPC (João Barbosa) 537efe1 rpc: Extract GetWalletNameFromJSONRPCRequest from GetWalletForJSONRPCRequest (João Barbosa) Pull request description: This patch adds wallet unload feature via RPC. It also adds UI support for unloaded wallets. Tree-SHA512: 7c7f9f32f7a2266d2df574aa6b95f993c3dc82736f93304562122beb8756fb28cd22d03866b48f493c747441f22d30e196b098dec435cc25e035633f090351ea

promag force-pushed the 2018-04-unload-wallet branch from 580e822 to 72bbee3 Compare April 28, 2018 21:46

promag mentioned this pull request Apr 28, 2018

Split event handlers by wallet #13070

Closed

promag force-pushed the 2018-04-unload-wallet branch 4 times, most recently from 55eb478 to 4cb3e57 Compare April 29, 2018 09:34

fanquake added Wallet RPC/REST/ZMQ labels Apr 30, 2018

jnewbery mentioned this pull request Apr 30, 2018

Dynamic wallet load / create / unload #13059

Closed

10 tasks

promag mentioned this pull request May 22, 2018

Use shared pointer to retain wallet instance #13063

Merged

promag changed the title ~~[WIP] Add unloadwallet RPC~~ Add unloadwallet RPC May 24, 2018

promag force-pushed the 2018-04-unload-wallet branch 2 times, most recently from a07427d to 8b490cd Compare May 24, 2018 16:38

promag force-pushed the 2018-04-unload-wallet branch 3 times, most recently from c3cbfd8 to 3a0da11 Compare June 1, 2018 22:28

promag force-pushed the 2018-04-unload-wallet branch from 3a0da11 to e95206b Compare June 1, 2018 22:33

promag force-pushed the 2018-04-unload-wallet branch 2 times, most recently from 4f9f422 to ba6a970 Compare June 4, 2018 21:41

promag force-pushed the 2018-04-unload-wallet branch from f33ff8c to d968dc3 Compare June 5, 2018 10:18

jnewbery reviewed Jun 6, 2018

View reviewed changes

promag added 3 commits June 18, 2018 16:35

doc: Add release notes for unloadwallet RPC

9f9b50d

ui: Support wallets unloaded dynamically

0ee77b2

bugfix: Remove dangling wallet env instance

0b82bac

promag force-pushed the 2018-04-unload-wallet branch from 0496cfb to 0b82bac Compare June 18, 2018 15:35

bugfix: Delete walletView in WalletFrame::removeWallet

fe65bde

jonasschnelli merged commit fe65bde into bitcoin:master Jun 21, 2018

promag deleted the 2018-04-unload-wallet branch June 21, 2018 14:26

Bushstar mentioned this pull request Jun 25, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#349

Merged

ryanofsky reviewed Jun 25, 2018

View reviewed changes

This was referenced Jun 26, 2018

Add BitcoinApplication & RPCConsole tests #11625

Merged

Refactor: separate wallet from node #10973

Merged

ryanofsky mentioned this pull request Sep 25, 2018

[bugfix] wallet: Fix duplicate fileid detection #14320

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

	GetMainSignals().UnregisterBackgroundSignalScheduler();
	GetMainSignals().UnregisterWithMempoolSignals(mempool);
	g_wallet_init_interface.Close();

Add unloadwallet RPC #13111

Add unloadwallet RPC #13111

Uh oh!

Conversation

promag commented Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jun 1, 2018

Uh oh!

promag commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 5, 2018

Uh oh!

ken2812221 commented Jun 6, 2018

Uh oh!

promag commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken2812221 commented Jun 6, 2018

Uh oh!

promag commented Jun 6, 2018

Uh oh!

ken2812221 commented Jun 6, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PierreRochard commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jun 20, 2018

Uh oh!

jnewbery commented Jun 20, 2018

Uh oh!

PierreRochard commented Jun 20, 2018

Uh oh!

promag commented Jun 20, 2018

Uh oh!

PierreRochard commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag commented Apr 28, 2018 •

edited

Loading

promag commented Jun 1, 2018 •

edited

Loading

promag commented Jun 6, 2018 •

edited

Loading

promag Jun 7, 2018 •

edited

Loading

PierreRochard commented Jun 19, 2018 •

edited

Loading

promag commented Jun 19, 2018 •

edited

Loading