Adds a loadwallet RPCs. This allows wallets to be loaded dynamically during runtime without having to stop-start the node with new -wallet params.
Includes functional tests and release notes.
Limitations:
244 | @@ -252,10 +245,7 @@ void Shutdown() 245 | #endif 246 | UnregisterAllValidationInterfaces(); 247 | #ifdef ENABLE_WALLET 248 | - for (CWalletRef pwallet : vpwallets) { 249 | - delete pwallet; 250 | - } 251 | - vpwallets.clear(); 252 | + DeleteWallets();
It'd probably call this deallocWallets()
Yes, DeleteWallets() is a bad name. DeallocWallets() is good, or perhaps DetachWallets()
226 | @@ -227,40 +227,12 @@ bool CDB::Recover(const std::string& filename, void *callbackDataIn, bool (*reco 227 | 228 | bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& dataDir, std::string& errorStr) 229 | { 230 | - LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0));
What's the reasons for keeping this function (that now always returns true)?
You're right. This (and CWalletDB::VerifyEnvironment()) are now entirely vestigial and are not called. Both can be removed.
34 | @@ -35,6 +35,8 @@ 35 | #include <boost/algorithm/string/replace.hpp> 36 | #include <boost/thread.hpp> 37 | 38 | +/** Protects the vpwallets vector */ 39 | +CCriticalSection cs_wallets; 40 | std::vector<CWalletRef> vpwallets;
I guess this belongs to walletinit.h now?
Why? vpwallets is accessed by functions in several of the wallet source files.
I had the idea that walletinit.h/.c is kind of the multiwallet interface (wallet manager). The functions we have there (DeleteWallets(), VerifyWallets(), ShutdownWallets();, etc.) sounded for me after vpwallets belongs there...
IMO, the multiwallet map should ideally not sit within the objects (CWallet) implementation.
Tend to agree with @jonasschnelli here, though I agree the barrier isn't exactly clear (yet). Ideally we'd move more towards clarifying the interface here - making CWallet less and less aware of all the things around it (including other wallets). @sipa thoughts here?
2467 | + } 2468 | + 2469 | + return obj; 2470 | +} 2471 | + 2472 | +UniValue loadwallet(const JSONRPCRequest& request)
On further thought, I think I prefer the names attachwallet and detachwallet. That gives a bit more distinction from the dump/import RPCs and makes a stronger implication that the new .dat file is being loaded and attached as a separate wallet.
IMO this one should rebase on the other.
Makes logical sense. Downside is that the other is a more significant change, so may take longer to get merged.
If this really depends on that work then there's no other way than waiting. The other downside is that the other commits will show up here too.
Bad automatic merge of the testcase. Re-merged manually.
Manual rebase fixed the test bug. Travis now passes.
This is ready for initial review.
494 | - 495 | - connect(walletModel, SIGNAL(coinsSent(CWallet*,SendCoinsRecipient,QByteArray)), 496 | - paymentServer, SLOT(fetchPaymentACK(CWallet*,const SendCoinsRecipient&,QByteArray))); 497 | + // TODO: Expose secondary wallets 498 | + LOCK(cs_wallets); 499 | + if (!vpwallets.empty())
{.
501 | + walletModel = new WalletModel(platformStyle, vpwallets[0], optionsModel); 502 | + 503 | + window->addWallet(BitcoinGUI::DEFAULT_WALLET, walletModel); 504 | + window->setCurrentWallet(BitcoinGUI::DEFAULT_WALLET); 505 | + 506 | + connect(walletModel, SIGNAL(coinsSent(CWallet*,SendCoinsRecipient,QByteArray)),
Missing spaces after ,?
192 | - for (const std::string& walletFile : gArgs.GetArgs("-wallet")) { 193 | - if (boost::filesystem::path(walletFile).filename() != walletFile) { 194 | - return InitError(strprintf(_("Error loading wallet %s. -wallet parameter must only specify a filename (not a path)."), walletFile)); 195 | - } 196 | + // -salvagewallet can only be used when loading a single wallet 197 | + bool salvage_wallet = gArgs.GetBoolArg("-salvagewallet", false) && wallet_files.size() <= 1;
Throw an error if -salvagewallet and there are multiple wallets?
Sounds like a sensible change in behaviour. I think it's beyond the scope of this PR.
In commit "Allow single wallet to be verified"
It looks like WalletParameterInteraction is already throwing an error in this case. I think it would be better to drop the comment and wallet_files check here. If you'd prefer to keep the check, I think the comment should be something clearer like "// parameter interaction code should have thrown an error if -salvagewallet was enabled with more than wallet file, so the wallet_files size check here should have no effect."
Updated comment as suggested by @ryanofsky .
Appears this comment update was lost somewhere?
36 | @@ -37,6 +37,7 @@ static const std::string WALLET_ENDPOINT_BASE = "/wallet/"; 37 | 38 | CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request) 39 | { 40 | + AssertLockHeld(cs_wallets);
Why not just LOCK(cs_wallets) too?
Because I'd prefer to avoid recursive locking if possible.
Just curious, why?
Yes, definitely. cs_wallets is locked before cs_main in global lock order, so putting it too many places is just asking for trouble, IMO.
Quick review. All touched code could be updated as per dev notes.
Most importantly, With the LOCK(cs_wallets) it will be impossible to have concurrent RPC to different wallets, not to mention to the same wallet. Is this expected behavior? - I'm assuming there is a thread pool for RPC.
Thanks for the review @promag!
Regarding the locking: Yes, I've implemented this as a mutex for now, which as you point out would limit wallet RPCs to be single-threaded. I'd like to update this to be something a bit better before v0.16. Suggestions are either a shared mutex or (suggested by @theuni) a version of the decay pointers in #10738. The problem with using a shared mutex is that C++ doesn't have a standard library shared mutex until C++14 (std::shared_timed_mutex) so I'd either have to use boost::shared_mutex or roll my own.
(apologies - I thought I'd already written a note in this PR explaining this, but I must have failed to hit send!)
For now, I think review of this PR with the mutex is helpful, and I'll change it around to use a shared_mutex later.
This seems to ignore the whole issue of long-term wallet references, and probably locks cs_wallets too long.
Adding 0.16.0 milestone, I think this should be merged after 0.15.1 to prevent extra rebasing in the wallet code when porting back to 0.15.1.
2720 | + if (!EnsureWalletIsAvailable(*wallet_iter, request.fHelp)) { 2721 | + throw JSONRPCError(RPC_WALLET_ERROR, "Error accessing wallets"); 2722 | + } 2723 | + 2724 | + if ((*wallet_iter)->GetName() == walletName) { 2725 | +
Unneeded newline.
2713 | + 2714 | + std::string walletName = request.params[0].get_str(); 2715 | + 2716 | + bool unloaded = false; 2717 | + 2718 | + for (std::vector<CWalletRef>::iterator wallet_iter = vpwallets.begin(); wallet_iter != vpwallets.end(); wallet_iter++) {
Any reason for not just doing for (auto wallet : vpwallets) { here?
2657 | @@ -2658,35 +2658,53 @@ UniValue listwallets(const JSONRPCRequest& request) 2658 | 2659 | UniValue openwallet(const JSONRPCRequest& request) 2660 | { 2661 | - if (request.fHelp || request.params.size() != 1) 2662 | + if (request.fHelp || request.params.size() > 2) 2663 | throw std::runtime_error( 2664 | - "openwallet \"filename\"\n" 2665 | + "openwallet \"filename\" (\"passphrase\")\n"
Ugh, passwords on prompts leaves the password as plaintext in shell history etc. Can't really be fixed for HTTP-RPC I guess, but CLI could probably take a bool to mean "prompt me for password".
2665 | + "openwallet \"filename\" (\"passphrase\")\n" 2666 | "\nOpens a wallet from a wallet.dat file.\n" 2667 | + "\nUse the passphrase argument to unlock an encrypted wallet while opening.\n" 2668 | "\nArguments:\n" 2669 | "1. \"filename\" (string, required) The wallet.dat file\n" 2670 | + "2. \"passphrase\" (string, optional) The wallet.dat file\n"
Description is wrong.
utACK 6c5b84119d1ffa547bf094b28ca602bdaf07c21a
Needs rebase.
The openwallet should rescan?
Concept ACK.
I was able to use openwallet and closewallet.
I wonder how this interacts with #11383. E.g. I'm guessing the UI wouldn't pick up on this (not a big issue). I can try once both are rebased.
It would be nice if bitcoin-cli -rpcwallet=X would open X if it's not open yet, maybe close it when done (future PR).
Should there be a separate RPC for creating a new wallet or should that be done using openwallet? (my initial thoughts are that there should be a separate RPC)
I would prefer it if openwallet fails for wallets that don't exist, rather than creating a new wallet. I understand it makes sense for bitcoind -wallet to create a new one, because that's what a new user would expect, but I don't think that's needed for openwallet.
223 | - CWallet dummyWallet; 224 | - std::string backup_filename; 225 | - if (!CWalletDB::Recover(walletFile, (void *)&dummyWallet, CWalletDB::RecoverKeysOnlyFilter, backup_filename)) { 226 | - return false; 227 | - } 228 | + return InitError(strprintf(_("Error loading wallet %s. Duplicate wallet filename specified."), walletFile));
This is redundant now, no?
1079 | @@ -1080,6 +1080,9 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface 1080 | /** Mark a transaction as replaced by another transaction (e.g., BIP 125). */ 1081 | bool MarkReplaced(const uint256& originalHash, const uint256& newHash); 1082 | 1083 | + //! Verify wallet naming and perform salvage on the wallet if required 1084 | + static bool VerifyWallet(std::string walletFile, bool salvage_wallet);
nit: I'd rather not make this a static CWallet member. wallet.cpp is already plenty big and the stuff that it does seems more external db-related stuff and less wallet-related stuff. Ideally it'd be nice to more clearly separate the DB stuff from the wallet logic itself by pulling things like the DB-related stuff out of CreateWalletFromFile.
@TheBlueMatt btw my suggestion is to move (some) static code to a WalletManager (#13034).
183 | @@ -184,7 +184,8 @@ void TestSendCoins() 184 | BumpFee(transactionView, txid2, false /* expect disabled */, {} /* expected error */, false /* cancel */); 185 | BumpFee(transactionView, txid2, true /* expect disabled */, "already bumped" /* expected error */, false /* cancel */); 186 | 187 | - bitdb.Flush(true); 188 | + bitdb.Flush("wallet_test.dat");
In commit "call CDBEnv.shutdown() from FlushWallets"
Maybe could use dbw->GetName() instead of repeating wallet_test.dat here
580 | + LogPrint(BCLog::DB, "CDBEnv::Flush: Flush%s\n", fDbEnvInit ? "" : " database not started");
581 | if (!fDbEnvInit)
582 | return;
583 | {
584 | LOCK(cs_db);
585 | std::map<std::string, int>::iterator mi = mapFileUseCount.begin();
In commit "call CDBEnv.shutdown() from FlushWallets"
Could change this line to: mi = mapFileUseCount.find(strFile), and change the while to an if on the line below to skip unnecessary loop.
239 |
240 | void StopWallets() {
241 | - for (CWalletRef pwallet : vpwallets) {
242 | - pwallet->Flush(true);
243 | - }
244 | + if (gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)) return;
In commit "call CDBEnv.shutdown() from FlushWallets"
Is this line doing anything? Should add comment explaining
559 | @@ -560,6 +560,16 @@ bool CDB::Rewrite(CWalletDBWrapper& dbw, const char* pszSkip) 560 | } 561 | } 562 | 563 | +void CDBEnv::Shutdown() 564 | +{ 565 | + char** listp;
In commit "add CDBEnv::Shutdown() function"
I think this method should start with if (!fDbEnvInit) return; similar to the Flush method so shutdown code can be simple for callers (no need for -disablewallet check).
In commit "add CDBEnv::Shutdown() function"
Maybe AssertLockHeld(cs_db)
3760 | +bool CWallet::VerifyWallet(std::string walletFile, bool salvage_wallet, std::string& strError) 3761 | { 3762 | if (boost::filesystem::path(walletFile).filename() != walletFile) { 3763 | - return InitError(strprintf(_("Error loading wallet %s. wallet parameter must only specify a filename (not a path)."), walletFile)); 3764 | + strError = strprintf(_("Error loading wallet %s. wallet parameter must only specify a filename (not a path)."), walletFile); 3765 | + return InitError(strError);
It might be better to drop thee InitError calls here and let callers VerifyWallet callers invoke it. Otherwise calls to open wallets via RPC could trigger GUI message boxes and maybe hang.
Maybe mark work in progress due to unresolved locking issues.
It's not clear to me that exclusively locking cs_wallets is really that much worse than current behavior, since most current RPCs seem to acquire locks for a long time and effectively be single threaded. But it would be good to avoid this.
It might be straightforward to fix by making vpwallets hold shared pointers, and by releasing the cs_wallets lock except when accessing vpwallets. I don't think there is any need for strong_ptrs because there should be no reason to care what threads wallets are closed on.
Using a shared lock as a substitute for shared pointers seems a little kludgy to me since it would just be speeding up overbroad cs_wallets locks instead of removing them, and would it would still block operations on unrelated wallets when any wallet was being opened or closed.
Verify and Flush refactoring commits in this PR seem like they might be useful changes on their own, and could maybe be pulled out into separate PRs to simplify this one.
Needs rebase
Should there be a separate RPC for creating a new wallet or should that be done using openwallet? (my initial thoughts are that there should be a separate RPC)
I suggest there should be a new RPC command to create new wallets. Aside from least surprise, there is more than one type of wallet that the user could want created.
Needs rebase and issues (@ryanofsky) addressed.
Closing the wallet that is in use by qt (the one at vpwallets[0] will result in a crash. So additional handling for Qt stuff is needed.
I plan to pick this up again in the next few days. it requires:
considering how this interacts with multiwallet in the GUI
A good enough first step would be if the dropdowns update when wallets are loaded / unloaded through the console. Adding UI to load and unload wallets can be a followup PR.
This was discussed in the IRC meeting today: https://botbot.me/freenode/bitcoin-core-dev/2018-04-12/?msg=98919702&page=5
15:26 < wumpus> #topic dynamic wallet load/unload (promag)
15:26 < Randolf> Ack.
15:26 < promag> not sure what you guys think
15:26 < instagibbs_> what's the controversy in this topic :)
15:27 < jonasschnelli> [#10740](/bitcoin-bitcoin/10740/)
15:27 < sipa> it should happen, duh
15:27 < gribble> [#10740](/bitcoin-bitcoin/10740/) | [WIP] [wallet] dynamic loading/unloading of wallets by jnewbery · Pull Request [#10740](/bitcoin-bitcoin/10740/) · bitcoin/bitcoin · GitHub
15:27 < wumpus> seems like something we want to have at some point...
15:27 < promag> but I think luke agrees that wallet management should be with shared pointers
15:27 < sipa> how and when is another :)
15:27 < luke-jr> indeed, using names just asks for chaos with runtime-changing wallets
15:28 < promag> please read [#11402](/bitcoin-bitcoin/11402/) for some reasons to switch
15:28 < gribble> [#11402](/bitcoin-bitcoin/11402/) | Use shared pointer for wallet instances by promag · Pull Request [#11402](/bitcoin-bitcoin/11402/) · bitcoin/bitcoin · GitHub
15:28 < jonasschnelli> IMO 10740 can't create wallets, IMO first step would be to add a createwallet RPC call
15:29 < jonasschnelli> the whole creation/configuration setup if flawed since multiwallet
15:29 < jonasschnelli> stuff like -keypool should be per wallet
15:29 < jnewbery> jonasschnelli: you think createwallet should go in *before* load/unload?
15:29 < jonasschnelli> and persisted in the wallet file (as configuration section)
15:29 < jonasschnelli> jnewbery: not sure,.. just thinking
15:30 < jnewbery> seems reasonable to me
15:30 < jonasschnelli> createwallet could also *not* load the wallet in the first step (not ideal, but maybe reduces complexity)
15:30 < sipa> that seems strange... you could create a new wallet at run time but not use it?
15:30 < jonasschnelli> sipa: createwallet could also directly load/use the wallet
15:30 < jnewbery> I think createwallet would also load the new wallet, no?
15:30 < promag> create implies loading
15:30 < luke-jr> sipa: iow, 0 to 1 only.
15:31 < sipa> jonasschnelli: well then you need to have loading functionality first!
15:31 < sipa> and if you have it, why not expose it
15:31 < jonasschnelli> sipa: yes. That's a point.
15:31 < jnewbery> createwallet could also be done by bitcoin-wallet-tool
15:32 < jnewbery> (#8745)
15:32 < gribble> [#8745](/bitcoin-bitcoin/8745/) | [PoC] Add wallet inspection and modification tool "bitcoin-wallet-tool" by jonasschnelli · Pull Request [#8745](/bitcoin-bitcoin/8745/) · bitcoin/bitcoin · GitHub
15:32 < jonasschnelli> Yes. Would be possible...
15:32 < jonasschnelli> I just think the create-during-startup approach is not good
15:33 < promag> also related [#10973](/bitcoin-bitcoin/10973/)
15:33 < jnewbery> jonasschnelli: I agree
15:33 < gribble> [#10973](/bitcoin-bitcoin/10973/) | Refactor: separate wallet from node by ryanofsky · Pull Request [#10973](/bitcoin-bitcoin/10973/) · bitcoin/bitcoin · GitHub
15:33 < sipa> jonasschnelli: agree
15:33 < jonasschnelli> And as a first step I though createwallet would make sense.. but not loading it seems after a strange use-case
15:33 < luke-jr> load -> create -> unload
15:33 < jonasschnelli> but a nice first code/impl. step
15:33 < luke-jr> unload is the complex part tbh
15:33 < jnewbery> luke-jr: agree
15:34 < jonasschnelli> Agree with luke-jr. Maybe split unload away from the existing PR jnewbery ?
15:34 < jnewbery> yes
15:34 < jnewbery> I intend to pick up 10740 again soon, rebase and rework it
15:34 < promag> consider the use case: 1) rpc rescan wallet 2) in parallel unload wallet - should 2) wait for 1) ?
15:34 < luke-jr> probably
15:35 < jonasschnelli> Great. Dynamic loading/creating is a nice feature that we probably want for 0.17!
15:35 < promag> luke-jr: and if the unload is from the UI?
15:35 < jnewbery> promag: do you consider 11402 a prereq for load/unload? What about just load?
15:36 < jonasschnelli> the wallet-tool is IMO orthogonal to wallet creation
15:36 < jonasschnelli> *via RPC
15:36 < luke-jr> promag: probably the same
15:36 < promag> jnewbery: IMHO for both
15:36 < jonasschnelli> RPC seems to be a must, wallet-tool can be a better place to create some sorts of wallets (or inspect it), .. like encrypted wallets
15:36 < luke-jr> promag: at least initially
15:37 < jnewbery> promag: want to rebase and put on high priority for review then, if you consider it a blocker?
15:37 < promag> luke-jr: my point is that it should not block, you request the unload and go on, when the wallet is not used anymore it gets unloaded
15:38 < jonasschnelli> [#11402](/bitcoin-bitcoin/11402/)
15:38 < gribble> [#11402](/bitcoin-bitcoin/11402/) | Use shared pointer for wallet instances by promag · Pull Request [#11402](/bitcoin-bitcoin/11402/) · bitcoin/bitcoin · GitHub
15:38 < luke-jr> promag: you mean leave the wallet loaded, but invisible? that seems worst outcome IMO
15:38 < luke-jr> user may unload and just shut off the PC
15:38 < wumpus> the unload should probably be in two stages: after requesting it, RPC and the GUI lose access to it. Then it waits for current operations tofinish. Then the thing really gets delted.
15:38 < luke-jr> yes
15:38 < promag> luke-jr: then the application will wait for wallets to unload
15:38 < jnewbery> I don't think we need to worry about unload at this stage. First step is add load functionality, then createwallet functionality
15:38 < luke-jr> and make it visible to the user in the meantime
15:39 < luke-jr> jnewbery: +1
15:39 < promag> wumpus: right, hence shared pointers
I should reduce the scope of this PR to just a loadwallet RPC. A createwallet RPC should come next, followed by unloadwallet. (unloadwallet is where most of the difficulties are).
Updated PR text since this PR is now only for loading wallets (not creating or unloading).
Original text also had the following motivation:
Main motivation for this was the fact that several wallet parameters are actions for individual wallet load/creation, rather than properties of the wallet component. Examples are
-salvagewallet,-rescan,-usehdand-upgradewallet. Continuing with that config/loading model is difficult in a multi-wallet world - how can users run those actions on individual wallets when loading multiple wallets? This PR offers a solution: individual wallets can be loaded at run-time, and RPC parameters can be passed in to carry out the various wallet-loading actions. Note that none of those load actions are yet implemented in this PR, but can easily be added.
Overhauled this PR to just contain the loadwallet functionality.
It is built on @promag's PR #13017. Please review that first.
This also needs the cs_wallets locking functionality from #11402 to be thread-safe.
I'll rebase once those PRs are merged. I've pushed the branch now in case anyone wants to do early review.
2905 | + if (!wallet) { 2906 | + throw JSONRPCError(RPC_WALLET_ERROR, "Wallet creation failed."); 2907 | + } 2908 | + AddWallet(wallet); 2909 | + 2910 | + wallet->ReacceptWalletTransactions();
Should call CWallet::postInitProcess instead? Otherwise MaybeCompactWalletDB is not called if the daemon is launched without wallets.
Thanks for the review @promag - I've done as you suggested and called CWallet::postInitProcess(). Because rpc doesn't have the CScheduler reference when loading the wallet, I had to change the WalletInit::Start() function to always schedule background wallet flushes, even when started with no wallets (ie -nowallet). Note that when started with -disablewallet, the entire wallet module is disabled and it's not possible to load wallets later.
I've also rebased on [#13028 rebased on master].
Removed the [WIP] tag since I think this implementation is now complete and ready for review.
744 | @@ -747,6 +745,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface 745 | const CBlockIndex* m_last_block_processed = nullptr; 746 | 747 | public: 748 | + static std::atomic<bool> fFlushScheduled;
Remove.
done
324 | @@ -324,7 +325,12 @@ bool WalletInit::Open() const 325 | void WalletInit::Start(CScheduler& scheduler) const 326 | { 327 | for (CWallet* pwallet : GetWallets()) { 328 | - pwallet->postInitProcess(scheduler); 329 | + pwallet->postInitProcess();
Move CWallet::postInitProcess() content here:
for (CWallet* pwallet : GetWallets()) {
// Add wallet transactions that aren't already in a block to mempool
// Do this here as mempool requires genesis block to be loaded
pwallet->ReacceptWalletTransactions();
}
..
324 | @@ -324,7 +325,12 @@ bool WalletInit::Open() const 325 | void WalletInit::Start(CScheduler& scheduler) const 326 | { 327 | for (CWallet* pwallet : GetWallets()) { 328 | - pwallet->postInitProcess(scheduler); 329 | + pwallet->postInitProcess(); 330 | + } 331 | + 332 | + // Run a thread to flush wallet periodically 333 | + if (!CWallet::fFlushScheduled.exchange(true)) {
WalletInitInterface::Start() is called only once, remove this check (and fFlushScheduled)?
Yes - much better. Thanks!
1101 | @@ -1103,7 +1102,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface 1102 | * Wallet post-init setup 1103 | * Gives the wallet a chance to register repetitive tasks and complete post-init tasks 1104 | */ 1105 | - void postInitProcess(CScheduler& scheduler); 1106 | + void postInitProcess();
Post-init doesn't matter now that wallets are dynamically loaded. Remove?
I'm going to leave this function here for now. It seems reasonable to have a postInitProcess() step for loading wallets, even if it is only doing one thing.
This makes the diff smaller, and we can always remove postInitProcess() later if it really is unnecessary.
I had to change the WalletInit::Start() function to always schedule background wallet flushes
Much better out of CWallet IMO.
Needs rebase again.
3968 | @@ -3968,6 +3969,64 @@ std::vector<std::string> CWallet::GetDestValues(const std::string& prefix) const 3969 | return values; 3970 | } 3971 | 3972 | +bool CWallet::Verify(std::string wallet_file, bool salvage_wallet)
Lock cs_wallets?
nit, const std::string& wallet_file.
Lock cs_wallets?
Done. How does this interact with your WalletManager work? Was the idea that cs_wallets was only held by WalletManager?
nit, const std::string& wallet_file.
Thanks - fixed
I was thinking moving CWallet::Verify to WalletManager or calling part of it from WalletManager. Let's worry about that later.
2907 | + } 2908 | + AddWallet(wallet); 2909 | + 2910 | + wallet->postInitProcess(); 2911 | + 2912 | + UniValue obj(UniValue::VSTR);
Not object :trollface:
Anyway, could return object instead?
{
"name": ...
}
yep - changed to return an object
Ops, this is not done.
2911 | + 2912 | + UniValue obj(UniValue::VSTR); 2913 | + obj = wallet_file; 2914 | + 2915 | + return obj; 2916 | +
nit, empty line.
removed
Not removed.
159 | + 160 | + # Fail to load if wallet doesn't exist 161 | + assert_raises_rpc_error(-18, 'Wallet wallets not found.', self.nodes[0].loadwallet, 'wallets') 162 | + 163 | + # Fail to load duplicate wallets 164 | + assert_raises_rpc_error(-4, 'Wallet file verification failed: Error loading wallet w1. Duplicate -wallet filename specified.', self.nodes[0].loadwallet, 'w1')
Use wallet_names[0] instead of 'w1'?
yes, better. Changed
Some comments after testing.
169 | @@ -170,5 +170,24 @@ def run_test(self): 170 | assert_equal(w1.getwalletinfo()['paytxfee'], 0) 171 | assert_equal(w2.getwalletinfo()['paytxfee'], 4.0) 172 | 173 | + # dynamic wallet loading 174 | + self.restart_node(0, ['-nowallet']) 175 | + for wallet_name in wallet_names: 176 | + self.nodes[0].loadwallet(wallet_name)
If I add a line here to inspect the wallet after the load:
self.nodes[0].getwalletinfo()
OR
self.nodes[0].getbalance()
I get the json exception throw here after the second load operation. Is this expected behavior?
Yes - if there are multiple wallets loaded, you need to send the RPC request to the correct wallet endpoint. Look at TestNode's get_wallet_rpc() method.
Got it, thank you.
As a nit, It might be useful just to verify you can call getbalance on the loaded wallet, just to show loaded wallets have expected behavior.
Yes, good idea. I'll update the test
Test ACK 0569d2cd1324304ba2d78d1299454126c9d82b51 - just left a small test nit
0 | @@ -0,0 +1,6 @@ 1 | +Dynamic loading of wallets 2 | +-------------------------- 3 | + 4 | +Previously, wallets could only be loaded at startup, by specifying `-wallet` parameters on the command line or in the bitcoin.conf file. It is now possible to load wallets dynamically at runtime by calling the `loadwallet` RPC. The wallet file/directory must be located in the `walletdir` directory in order to be loaded.
The wallet file/directory must be located in the
walletdirdirectory in order to be loaded.
It works with external wallets (absolute path not related to -walletdir).
Thanks promag. Release notes update. Please let me know what you think.
Consider:
mkdir /tmp/ws
bitcoind -regtest -walletdir=/tmp/ws -wallet=w1 -wallet=w2
bitcoin-cli -regtest listwallets
[
"w1",
"w2"
]
Then:
bitcoind -regtest -walletdir=/tmp/ws -wallet=w1
bitcoin-cli -regtest loadwallet /tmp/ws/w2
bitcoin-cli -regtest listwallets
[
"w1",
"/tmp/ws/w2"
]
Also
bitcoind -regtest -walletdir=/tmp/ws -wallet=w1 -wallet=/tmp/ws/w2
bitcoin-cli -regtest listwallets
[
"w1",
"/tmp/ws/w2"
]
Is this fine or should the outputs be the same?
Should it be possible to specify an absolute path if it's in the walletdir?
Is this fine or should the outputs be the same? Should it be possible to specify an absolute path if it's in the walletdir?
I think that question is orthogonal to this PR. As you've noted, this is pre-existing behaviour and is exhibited when specifying wallet names on the command line or .conf file. This behaviour was specified in the PR that introduced external wallet files (here: #11687 (review)).
Can we save discussion on whether that's the best behaviour for outside this PR?
Release notes LGTM.
Tested ACK a395059 (previously tested 2d67656).
Can we save discussion on whether that's the best behaviour for outside this PR?
Absolutely!
Tested Re-ACK a395059d4f576cc1b3b61ef78f2e01dec7758d3f
@promag , @conscott - thanks for the review!
This could really do with some review from people with longer term wallet experience - nagging @TheBlueMatt @jonasschnelli @luke-jr @laanwj :slightly_smiling_face:
utACK a395059d4f576cc1b3b61ef78f2e01dec7758d3f
3037 | + 3038 | + return obj; 3039 | + 3040 | +} 3041 | + 3042 | +
Remove 2nd empty line.
rebased
3030 | + AddWallet(wallet); 3031 | + 3032 | + wallet->postInitProcess(); 3033 | + 3034 | + UniValue obj(UniValue::VOBJ); 3035 | + obj.pushKV("name", wallet_file);
nit, wallet->GetName() instead.
Yes, good idea. Updated.
Tested ACK 3d3e785.
3017 | + if (fs::symlink_status(wallet_path).type() == fs::file_not_found) { 3018 | + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + wallet_file + " not found."); 3019 | + } 3020 | + 3021 | + std::string dummy_warning; 3022 | + if (!CWallet::Verify(wallet_file, false, error, dummy_warning)) {
This needs some kind of lock to avoid double-loading a wallet in two rpc threads at once.
Hmm @jnewbery this is missing #10740 (review).
2993 | @@ -2994,6 +2994,49 @@ static UniValue listwallets(const JSONRPCRequest& request) 2994 | return obj; 2995 | } 2996 | 2997 | +UniValue loadwallet(const JSONRPCRequest& request) 2998 | +{ 2999 | + if (request.fHelp || request.params.size() != 1) 3000 | + throw std::runtime_error( 3001 | + "loadwallet \"filename\"\n" 3002 | + "\nLoads a wallet from a wallet file or directory.\n"
Maybe some help messages noting that certain config options still apply here - eg if you started with something like zapwallettxes, upgradewallet, rescan, etc it will take the same action for newly-loaded wallets here.
Done
Should probably fix the various memory leaks in CreateWalletFromFile first - we didnt really use to care cause it would just persist through operation, now we do.
Should probably fix the various memory leaks in CreateWalletFromFile first - we didnt really use to care cause it would just persist through operation, now we do. @TheBlueMatt #13063 fixes that.
#13063 seems overkill for this issue, we could just use a unique_ptr within CreateWalletFromFile.
On May 7, 2018 3:59:40 PM UTC, "João Barbosa" notifications@github.com wrote:
Should probably fix the various memory leaks in CreateWalletFromFile first - we didnt really use to care cause it would just persist through operation, now we do.
@TheBlueMatt #13063 fixes that.
-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/10740#issuecomment-387113002
@TheBlueMatt Then you mean #12647. Either way it's something that can be fixed in another PR.
@promag it can be fixed in another PR, but that would have to be merged before this one. Its really no big deal on master, but with an RPC that allows you to create arbitrary memory leaks, that's not great...
Should probably fix the various memory leaks in CreateWalletFromFile first - we didnt really use to care cause it would just persist through operation, now we do.
I've added the minimal fix from here: #12647 (review) to address the memory lead. @promag - that conflicts with your shared pointer PR, but it should be trivial for you to rebase that.
3025 | + throw JSONRPCError(RPC_WALLET_ERROR, "Wallet file verification failed: " + error); 3026 | + } 3027 | + 3028 | + CWallet * const wallet = CWallet::CreateWalletFromFile(wallet_file, fs::absolute(wallet_file, GetWalletDir())); 3029 | + if (!wallet) { 3030 | + throw JSONRPCError(RPC_WALLET_ERROR, "Wallet creation failed.");
"creation failed" is a confusing error message to give to the user here, "Wallet loading failed" would be more straightforward
Agree. Changed
3963 | + fs::path wallet_path = fs::absolute(wallet_file, GetWalletDir()); 3964 | + fs::file_type path_type = fs::symlink_status(wallet_path).type(); 3965 | + if (!(path_type == fs::file_not_found || path_type == fs::directory_file || 3966 | + (path_type == fs::symlink_file && fs::is_directory(wallet_path)) || 3967 | + (path_type == fs::regular_file && fs::path(wallet_file).filename() == wallet_file))) { 3968 | + error_string = strprintf(
As these errors are reported over RPC, we should stop translating them; RPC errors should not depend on the locale.
Yes - removed translation.
4022 | @@ -3973,7 +4023,8 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path& 4023 | 4024 | int64_t nStart = GetTimeMillis(); 4025 | bool fFirstRun = true; 4026 | - CWallet *walletInstance = new CWallet(name, WalletDatabase::Create(path)); 4027 | + auto wallet_deleter = MakeUnique<CWallet>(name, WalletDatabase::Create(path));
The name wallet_deleter scares me.
is temp_wallet (with a comment above) less scary?
Yes, certainly. What I'd prefer, ideally, is to just call this wallet_instance and use this smart pointer everywhere instead of CWallet* walletInstance = temp_wallet.get().
But this is fine, it's no longer scary.
ok, I'll change it if I need to update the branch for another reason.
Looks overall good to me. Some small comments.
utACK 5ef858bfed2438746db0ba096ff3c98755603cb8
3971 | + "or (for backwards compatibility) the name of an existing data file in -walletdir (%s)"), 3972 | + wallet_file, GetWalletDir())); 3973 | + } 3974 | + 3975 | + // Make sure that the wallet path doesn't clash with an existing wallet path 3976 | + std::set<fs::path> wallet_paths;
commit: [wallet] Add CWallet::Verify function
Using a set here is unnecessary, can just do a direct equality check.
Yes, much better idea.
3018 | + fs::path wallet_path = fs::absolute(wallet_file, GetWalletDir()); 3019 | + if (fs::symlink_status(wallet_path).type() == fs::file_not_found) { 3020 | + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + wallet_file + " not found."); 3021 | + } 3022 | + 3023 | + std::string dummy_warning;
commit: [wallet] [rpc] Add loadwallet RPC
Feels like this warning should get logged or maybe even returned in a "warning" field in the JSON response.
ok, added to the JSON response.
4143 | @@ -4141,7 +4144,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path& 4144 | } 4145 | 4146 | walletInstance->m_last_block_processed = chainActive.Tip(); 4147 | - RegisterValidationInterface(walletInstance); 4148 | + RegisterValidationInterface(temp_wallet.release());
commit: [wallet] Fix potential memory leak in CreateWalletFromFile
This isn't a complete fix because there are still early returns with errors in the rescan logic below. I believe it is safe to move the RegisterValidationInterface call to the bottom of the function as I argued in #12647 because cs_main gets locked on line 4135.
This isn't a complete fix because there are still early returns with errors in the rescan logic below. I believe it is safe to move the RegisterValidationInterface call to the bottom of the function as I argued in #12647 because cs_main gets locked on line 4135.
jimpo's right, f8c81def4f5f88074348890a613c1006824b1b5d [wallet] Fix potential memory leak in CreateWalletFromFile isn't currently a complete fix for all memory leaks. Also, his suggestion to move RegisterValidationInterface and fix more leaks should be ok because of the cs_main lock. If you want to make that change, I would just add a comment above the RegisterValidationInterface call that says, "it is safe to register for notifications after rescanning (no notifications will be missed) because cs_main is held during the entire scan." Only downside to this change is that if in the future we do optimize the rescan to not hold cs_main the entire time, it could introduce a bug where notifications are missed after the rescan completes.
Other options:
temp_wallet.release() call in this commit noting that walletInstance will be leaked if there is an error and the code below returns nullptr.temp_wallet, and make walletInstance a unique ptr. Return walletInstance.release() on the last line of this function. Add UnregisterValidationInterface(walletInstance.get()) calls below where nullptr is returned.
Done (and added comment)
Fix proposed by ryanofsky in
https://github.com/bitcoin/bitcoin/pull/12647#discussion_r174875670
WalletInit::Start calls postInitProcess() for each wallet. Previously
each call to postInitProcess() would attempt to schedule wallet
background flushing.
Just start wallet background flushing once from WalletInit::Start().
4017 | + if (fs::absolute(wallet->GetName(), GetWalletDir()) == wallet_path) { 4018 | + return InitError(strprintf(_("Error loading wallet %s. Duplicate -wallet filename specified."), wallet_file)); 4019 | + } 4020 | + } 4021 | + 4022 | + if (!wallet_paths.insert(wallet_path).second) {
[wallet] Add CWallet::Verify function
Can drop this and the declaration of wallet_paths above.
True, no longer necessary.
Sorry, one more thing then I'll ACK.
This allows a single wallet to be verified. Prior to this commit, all
wallets were verified together by the WalletInit::Verify() function at
start-up.
Individual wallet verification will be done when loading wallets
dynamically at runtime.
Pass an error message back from CWallet::Verify(), and call
InitError/InitWarning from WalletInit::Verify().
This means that we can call CWallet::Verify() independently from
WalletInit and not have InitErrors printed to stdout. It also means that
the error can be reported to the user if dynamic wallet load fails.
The new `loadwallet` RPC method allows an existing wallet to be loaded
dynamically at runtime.
`unloadwallet` and `createwallet` are not implemented. Notably,
`loadwallet` can only be used to load existing wallets, not to create a
new wallet.
Add testcases to wallet_multiwallet.py to test the new `loadwallet` RPC
method.
ACK cd53981