loadwallet RPC - load wallet at runtime
#10740
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();
deallocWallets()
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));
true)?
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;
walletinit.h now?
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.
2467+ }
2468+
2469+ return obj;
2470+}
2471+
2472+UniValue loadwallet(const JSONRPCRequest& request)
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.
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)),
,?
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;
-salvagewallet and there are multiple wallets?
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.”
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);
LOCK(cs_wallets) too?
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.
cs_wallets too long.
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+
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++) {
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"
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"
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));
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);
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);
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.
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
015:26 < wumpus> #topic dynamic wallet load/unload (promag)
115:26 < Randolf> Ack.
215:26 < promag> not sure what you guys think
315:26 < instagibbs_> what's the controversy in this topic :)
415:27 < jonasschnelli> [#10740](/bitcoin-bitcoin/10740/)
515:27 < sipa> it should happen, duh
615:27 < gribble> [#10740](/bitcoin-bitcoin/10740/) | [WIP] [wallet] dynamic loading/unloading of wallets by jnewbery · Pull Request [#10740](/bitcoin-bitcoin/10740/) · bitcoin/bitcoin · GitHub
715:27 < wumpus> seems like something we want to have at some point...
815:27 < promag> but I think luke agrees that wallet management should be with shared pointers
915:27 < sipa> how and when is another :)
1015:27 < luke-jr> indeed, using names just asks for chaos with runtime-changing wallets
1115:28 < promag> please read [#11402](/bitcoin-bitcoin/11402/) for some reasons to switch
1215:28 < gribble> [#11402](/bitcoin-bitcoin/11402/) | Use shared pointer for wallet instances by promag · Pull Request [#11402](/bitcoin-bitcoin/11402/) · bitcoin/bitcoin · GitHub
1315:28 < jonasschnelli> IMO 10740 can't create wallets, IMO first step would be to add a createwallet RPC call
1415:29 < jonasschnelli> the whole creation/configuration setup if flawed since multiwallet
1515:29 < jonasschnelli> stuff like -keypool should be per wallet
1615:29 < jnewbery> jonasschnelli: you think createwallet should go in *before* load/unload?
1715:29 < jonasschnelli> and persisted in the wallet file (as configuration section)
1815:29 < jonasschnelli> jnewbery: not sure,.. just thinking
1915:30 < jnewbery> seems reasonable to me
2015:30 < jonasschnelli> createwallet could also *not* load the wallet in the first step (not ideal, but maybe reduces complexity)
2115:30 < sipa> that seems strange... you could create a new wallet at run time but not use it?
2215:30 < jonasschnelli> sipa: createwallet could also directly load/use the wallet
2315:30 < jnewbery> I think createwallet would also load the new wallet, no?
2415:30 < promag> create implies loading
2515:30 < luke-jr> sipa: iow, 0 to 1 only.
2615:31 < sipa> jonasschnelli: well then you need to have loading functionality first!
2715:31 < sipa> and if you have it, why not expose it
2815:31 < jonasschnelli> sipa: yes. That's a point.
2915:31 < jnewbery> createwallet could also be done by bitcoin-wallet-tool
3015:32 < jnewbery> (#8745)
3115: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
3215:32 < jonasschnelli> Yes. Would be possible...
3315:32 < jonasschnelli> I just think the create-during-startup approach is not good
3415:33 < promag> also related [#10973](/bitcoin-bitcoin/10973/)
3515:33 < jnewbery> jonasschnelli: I agree
3615:33 < gribble> [#10973](/bitcoin-bitcoin/10973/) | Refactor: separate wallet from node by ryanofsky · Pull Request [#10973](/bitcoin-bitcoin/10973/) · bitcoin/bitcoin · GitHub
3715:33 < sipa> jonasschnelli: agree
3815:33 < jonasschnelli> And as a first step I though createwallet would make sense.. but not loading it seems after a strange use-case
3915:33 < luke-jr> load -> create -> unload
4015:33 < jonasschnelli> but a nice first code/impl. step
4115:33 < luke-jr> unload is the complex part tbh
4215:33 < jnewbery> luke-jr: agree
4315:34 < jonasschnelli> Agree with luke-jr. Maybe split unload away from the existing PR jnewbery ?
4415:34 < jnewbery> yes
4515:34 < jnewbery> I intend to pick up 10740 again soon, rebase and rework it
4615:34 < promag> consider the use case: 1) rpc rescan wallet 2) in parallel unload wallet - should 2) wait for 1) ?
4715:34 < luke-jr> probably
4815:35 < jonasschnelli> Great. Dynamic loading/creating is a nice feature that we probably want for 0.17!
4915:35 < promag> luke-jr: and if the unload is from the UI?
5015:35 < jnewbery> promag: do you consider 11402 a prereq for load/unload? What about just load?
5115:36 < jonasschnelli> the wallet-tool is IMO orthogonal to wallet creation
5215:36 < jonasschnelli> *via RPC
5315:36 < luke-jr> promag: probably the same
5415:36 < promag> jnewbery: IMHO for both
5515: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
5615:36 < luke-jr> promag: at least initially
5715:37 < jnewbery> promag: want to rebase and put on high priority for review then, if you consider it a blocker?
5815: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
5915:38 < jonasschnelli> [#11402](/bitcoin-bitcoin/11402/)
6015:38 < gribble> [#11402](/bitcoin-bitcoin/11402/) | Use shared pointer for wallet instances by promag · Pull Request [#11402](/bitcoin-bitcoin/11402/) · bitcoin/bitcoin · GitHub
6115:38 < luke-jr> promag: you mean leave the wallet loaded, but invisible? that seems worst outcome IMO
6215:38 < luke-jr> user may unload and just shut off the PC
6315: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.
6415:38 < luke-jr> yes
6515:38 < promag> luke-jr: then the application will wait for wallets to unload
6615:38 < jnewbery> I don't think we need to worry about unload at this stage. First step is add load functionality, then createwallet functionality
6715:38 < luke-jr> and make it visible to the user in the meantime
6815:39 < luke-jr> jnewbery: +1
6915: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();
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;
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:
0for (CWallet* pwallet : GetWallets()) {
1 // Add wallet transactions that aren't already in a block to mempool
2 // Do this here as mempool requires genesis block to be loaded
3 pwallet->ReacceptWalletTransactions();
4}
5..
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)?
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();
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.
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)
cs_wallets?
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
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?
0{
1 "name": ...
2}
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')
wallet_names[0] instead of 'w1'?
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:
0self.nodes[0].getwalletinfo()
OR
0self.nodes[0].getbalance()
I get the json exception throw here after the second load operation. Is this expected behavior?
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.
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).
Consider:
0mkdir /tmp/ws
1bitcoind -regtest -walletdir=/tmp/ws -wallet=w1 -wallet=w2
2bitcoin-cli -regtest listwallets
3[
4 "w1",
5 "w2"
6]
Then:
0bitcoind -regtest -walletdir=/tmp/ws -wallet=w1
1bitcoin-cli -regtest loadwallet /tmp/ws/w2
2bitcoin-cli -regtest listwallets
3[
4 "w1",
5 "/tmp/ws/w2"
6]
Also
0bitcoind -regtest -walletdir=/tmp/ws -wallet=w1 -wallet=/tmp/ws/w2
1bitcoin-cli -regtest listwallets
2[
3 "w1",
4 "/tmp/ws/w2"
5]
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!
@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:
3037+
3038+ return obj;
3039+
3040+}
3041+
3042+
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)) {
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"
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
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.");
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(
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));
wallet_deleter scares me.
temp_wallet (with a comment above) less scary?
CWallet* walletInstance = temp_wallet.get().
But this is fine, it’s no longer scary.
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.
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.
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.
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.
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.