Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wallet] `loadwallet` RPC - load wallet at runtime #10740

Merged
merged 7 commits into from May 16, 2018

Conversation

@jnewbery
Copy link
Member

commented Jul 4, 2017

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:

  • currently this functionality is only available through the RPC interface.
  • wallets loaded in this way will not be displayed in the GUI.

@fanquake fanquake added the Wallet label Jul 5, 2017

@jnewbery jnewbery force-pushed the jnewbery:load_unload_wallet branch Jul 5, 2017

src/init.cpp Outdated
delete pwallet;
}
vpwallets.clear();
DeleteWallets();

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jul 5, 2017

Member

It'd probably call this deallocWallets()

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 7, 2017

Author Member

Yes, DeleteWallets() is a bad name. DeallocWallets() is good, or perhaps DetachWallets()

src/wallet/db.cpp Outdated
@@ -227,40 +227,12 @@ bool CDB::Recover(const std::string& filename, void *callbackDataIn, bool (*reco

bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& dataDir, std::string& errorStr)
{
LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0));

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jul 5, 2017

Member

What's the reasons for keeping this function (that now always returns true)?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 7, 2017

Author Member

You're right. This (and CWalletDB::VerifyEnvironment()) are now entirely vestigial and are not called. Both can be removed.

src/wallet/wallet.cpp Outdated
@@ -35,6 +35,8 @@
#include <boost/algorithm/string/replace.hpp>
#include <boost/thread.hpp>

/** Protects the vpwallets vector */
CCriticalSection cs_wallets;
std::vector<CWalletRef> vpwallets;

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jul 5, 2017

Member

I guess this belongs to walletinit.h now?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 7, 2017

Author Member

Why? vpwallets is accessed by functions in several of the wallet source files.

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jul 9, 2017

Member

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.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Dec 3, 2017

Contributor

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?

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2017

This overlaps significantly with #10762. Not sure how I should ask for review. Perhaps find out which one people think is the priority? Or I could split the walletinit.cpp commits into their own PR.

src/wallet/rpcwallet.cpp Outdated
return obj;
}

UniValue loadwallet(const JSONRPCRequest& request)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 7, 2017

Author Member

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.

@promag

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

@jnewbery IMO this one should rebase on the other.

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2017

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.

@promag

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

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.

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2017

I've changed the PR sequence, so this will now depend on #10767. If I get positive concept feedbac on #10767 I'll rebase this on top of it.

For now, I'm still just looking for concept feedback.

@jnewbery jnewbery force-pushed the jnewbery:load_unload_wallet branch Aug 28, 2017

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2017

Rebased on #10767 . Reviewers, please review #10767 first.

@jnewbery jnewbery force-pushed the jnewbery:load_unload_wallet branch Aug 30, 2017

MarcoFalke added a commit that referenced this pull request Sep 7, 2017
Merge #10767: [wallet] Clarify wallet initialization / destruction in…
…terface

5d2a399 [trivial] fixup comment for VerifyWallets() (John Newbery)
43b0e81 [wallet] Add StartWallets() function to wallet/init.cpp (John Newbery)
290f3c5 [wallet] Add RegisterWalletRPC() function to wallet/init.cpp (John Newbery)
062d631 [wallet] Add CloseWallets() function to wallet/init.cpp (John Newbery)
77fe07c [wallet] Add StopWallets() function to wallet/init.cpp (John Newbery)
2da5eaf [wallet] Add FlushWallets() function to wallet/init.cpp (John Newbery)
1b9cee6 [wallet] Rename WalletVerify() to VerifyWallets() (John Newbery)
9c76ba1 [wallet] Rename InitLoadWallet() to OpenWallets() (John Newbery)

Pull request description:

  Apologies for the mostly code move only PR. This is a pre-req for both #10740 and #10762

  All wallet component initialization/destruction functions are now in their own `wallet/init.cpp` translation unit and are no longer static functions on the CWallet class. The bitcoin_server also no longer has any knowledge that there are multiple wallets in vpwallet.

  There should be no changes in behavior from this PR.

Tree-SHA512: 7c260eb094f2fa1a88d803769ba60935810968a7309f731135e4b17623b97f18c03bbcd293c942093d1efce62c6c978f9ff484d54dc9a60bc2fcb5af2d160fcd

@jnewbery jnewbery force-pushed the jnewbery:load_unload_wallet branch to 91aea7e Sep 11, 2017

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2017

Rebased now that #10767 is merged. Still a work in progress. PR text updated.

@jnewbery jnewbery force-pushed the jnewbery:load_unload_wallet branch 3 times, most recently Sep 15, 2017

@jnewbery jnewbery changed the title [WIP] [wallet] dynamic loading/unloading of wallets [wallet] dynamic loading/unloading of wallets Sep 15, 2017

@jnewbery jnewbery force-pushed the jnewbery:load_unload_wallet branch Sep 20, 2017

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2017

Bad automatic merge of the testcase. Re-merged manually.

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2017

Manual rebase fixed the test bug. Travis now passes.

This is ready for initial review.

@promag
Copy link
Member

left a comment

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.

src/qt/bitcoin.cpp Outdated
paymentServer, SLOT(fetchPaymentACK(CWallet*,const SendCoinsRecipient&,QByteArray)));
// TODO: Expose secondary wallets
LOCK(cs_wallets);
if (!vpwallets.empty())

This comment has been minimized.

Copy link
@promag

promag Sep 21, 2017

Member

{.

src/qt/bitcoin.cpp Outdated
window->addWallet(BitcoinGUI::DEFAULT_WALLET, walletModel);
window->setCurrentWallet(BitcoinGUI::DEFAULT_WALLET);

connect(walletModel, SIGNAL(coinsSent(CWallet*,SendCoinsRecipient,QByteArray)),

This comment has been minimized.

Copy link
@promag

promag Sep 21, 2017

Member

Missing spaces after ,?

src/wallet/init.cpp Outdated
return InitError(strprintf(_("Error loading wallet %s. -wallet parameter must only specify a filename (not a path)."), walletFile));
}
// -salvagewallet can only be used when loading a single wallet
bool salvage_wallet = gArgs.GetBoolArg("-salvagewallet", false) && wallet_files.size() <= 1;

This comment has been minimized.

Copy link
@promag

promag Sep 21, 2017

Member

Throw an error if -salvagewallet and there are multiple wallets?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 21, 2017

Author Member

Sounds like a sensible change in behaviour. I think it's beyond the scope of this PR.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 20, 2017

Contributor

#10740 (comment)

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 24, 2018

Author Member

Updated comment as suggested by @ryanofsky .

This comment has been minimized.

Copy link
@jimpo

jimpo May 9, 2018

Contributor

Appears this comment update was lost somewhere?

src/wallet/rpcwallet.cpp Outdated
@@ -37,6 +37,7 @@ static const std::string WALLET_ENDPOINT_BASE = "/wallet/";

CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
{
AssertLockHeld(cs_wallets);

This comment has been minimized.

Copy link
@promag

promag Sep 21, 2017

Member

Why not just LOCK(cs_wallets) too?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 21, 2017

Author Member

Because I'd prefer to avoid recursive locking if possible.

This comment has been minimized.

Copy link
@promag

promag Sep 21, 2017

Member

Just curious, why?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Dec 3, 2017

Contributor

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.

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2017

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.

@laanwj laanwj added this to Blockers in High-priority for review Sep 21, 2017

@promag

This comment has been minimized.

Copy link
Member

commented May 7, 2018

@TheBlueMatt Then you mean #12647. Either way it's something that can be fixed in another PR.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented May 7, 2018

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

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented May 7, 2018

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 (comment) to address the memory lead. @promag - that conflicts with your shared pointer PR, but it should be trivial for you to rebase that.

@jnewbery jnewbery force-pushed the jnewbery:load_unload_wallet branch May 7, 2018

@laanwj
Copy link
Member

left a comment

Looks overall good to me. Some small comments.

src/wallet/rpcwallet.cpp Outdated

CWallet * const wallet = CWallet::CreateWalletFromFile(wallet_file, fs::absolute(wallet_file, GetWalletDir()));
if (!wallet) {
throw JSONRPCError(RPC_WALLET_ERROR, "Wallet creation failed.");

This comment has been minimized.

Copy link
@laanwj

laanwj May 9, 2018

Member

"creation failed" is a confusing error message to give to the user here, "Wallet loading failed" would be more straightforward

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 9, 2018

Author Member

Agree. Changed

if (!(path_type == fs::file_not_found || path_type == fs::directory_file ||
(path_type == fs::symlink_file && fs::is_directory(wallet_path)) ||
(path_type == fs::regular_file && fs::path(wallet_file).filename() == wallet_file))) {
error_string = strprintf(

This comment has been minimized.

Copy link
@laanwj

laanwj May 9, 2018

Member

As these errors are reported over RPC, we should stop translating them; RPC errors should not depend on the locale.

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 9, 2018

Author Member

Yes - removed translation.

src/wallet/wallet.cpp Outdated
@@ -3973,7 +4023,8 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&

int64_t nStart = GetTimeMillis();
bool fFirstRun = true;
CWallet *walletInstance = new CWallet(name, WalletDatabase::Create(path));
auto wallet_deleter = MakeUnique<CWallet>(name, WalletDatabase::Create(path));

This comment has been minimized.

Copy link
@laanwj

laanwj May 9, 2018

Member

The name wallet_deleter scares me.

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 9, 2018

Author Member

is temp_wallet (with a comment above) less scary?

This comment has been minimized.

Copy link
@laanwj

laanwj May 9, 2018

Member

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.

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 9, 2018

Author Member

ok, I'll change it if I need to update the branch for another reason.

@jnewbery jnewbery force-pushed the jnewbery:load_unload_wallet branch May 9, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented May 9, 2018

utACK 5ef858bfed2438746db0ba096ff3c98755603cb8

src/wallet/wallet.cpp Outdated
}

// Make sure that the wallet path doesn't clash with an existing wallet path
std::set<fs::path> wallet_paths;

This comment has been minimized.

Copy link
@jimpo

jimpo May 9, 2018

Contributor

commit: [wallet] Add CWallet::Verify function

Using a set here is unnecessary, can just do a direct equality check.

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 15, 2018

Author Member

Yes, much better idea.

src/wallet/rpcwallet.cpp Outdated
throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + wallet_file + " not found.");
}

std::string dummy_warning;

This comment has been minimized.

Copy link
@jimpo

jimpo May 9, 2018

Contributor

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.

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 15, 2018

Author Member

ok, added to the JSON response.

src/wallet/wallet.cpp Outdated
@@ -4141,7 +4144,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
}

walletInstance->m_last_block_processed = chainActive.Tip();
RegisterValidationInterface(walletInstance);
RegisterValidationInterface(temp_wallet.release());

This comment has been minimized.

Copy link
@jimpo

jimpo May 9, 2018

Contributor

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 comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 15, 2018

Contributor

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:

  • Just drop this commit and deal with memory leaks in a separate PR.
  • Just add a comment above the temp_wallet.release() call in this commit noting that walletInstance will be leaked if there is an error and the code below returns nullptr.
  • Get rid of 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.

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 15, 2018

Author Member

Done (and added comment)

src/wallet/init.cpp Outdated
return InitError(strprintf(_("Error loading wallet %s. -wallet parameter must only specify a filename (not a path)."), walletFile));
}
// -salvagewallet can only be used when loading a single wallet
bool salvage_wallet = gArgs.GetBoolArg("-salvagewallet", false) && wallet_files.size() <= 1;

This comment has been minimized.

Copy link
@jimpo

jimpo May 9, 2018

Contributor

Appears this comment update was lost somewhere?

@laanwj

This comment has been minimized.

Copy link
Member

commented May 14, 2018

This seems very near ready for merge except for @jimpo's nits.

jnewbery added 2 commits Apr 19, 2018
[wallet] setup wallet background flushing in WalletInit directly
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().

@jnewbery jnewbery force-pushed the jnewbery:load_unload_wallet branch May 15, 2018

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented May 15, 2018

Rebased and addressed @jimpo's comments

@jimpo
Copy link
Contributor

left a comment

Sorry, one more thing then I'll ACK.

src/wallet/wallet.cpp Outdated
}
}

if (!wallet_paths.insert(wallet_path).second) {

This comment has been minimized.

Copy link
@jimpo

jimpo May 15, 2018

Contributor

[wallet] Add CWallet::Verify function

Can drop this and the declaration of wallet_paths above.

This comment has been minimized.

Copy link
@promag

promag May 16, 2018

Member

True, no longer necessary.

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 16, 2018

Author Member

Thanks @jimpo. Fixed

jnewbery added 5 commits Apr 18, 2018
[wallet] Add CWallet::Verify function
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.
[wallet] Pass error message back from CWallet::Verify()
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.
[wallet] [rpc] Add loadwallet RPC
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.
[wallet] [tests] Test loadwallet
Add testcases to wallet_multiwallet.py to test the new `loadwallet` RPC
method.

@jnewbery jnewbery force-pushed the jnewbery:load_unload_wallet branch to cd53981 May 16, 2018

@jimpo

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

ACK cd53981

@laanwj laanwj merged commit cd53981 into bitcoin:master May 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request May 16, 2018
Merge #10740: [wallet] `loadwallet` RPC - load wallet at runtime
cd53981 [docs] Add release notes for `loadwallet` RPC. (John Newbery)
a46aeb6 [wallet] [tests] Test loadwallet (John Newbery)
5d15260 [wallet] [rpc] Add loadwallet RPC (John Newbery)
876eb64 [wallet] Pass error message back from CWallet::Verify() (John Newbery)
e0e90db [wallet] Add CWallet::Verify function (John Newbery)
470316c [wallet] setup wallet background flushing in WalletInit directly (John Newbery)
59b87a2 [wallet] Fix potential memory leak in CreateWalletFromFile (John Newbery)

Pull request description:

  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:

  - currently this functionality is only available through the RPC interface.
  - wallets loaded in this way will not be displayed in the GUI.

Tree-SHA512: f80dfe32b77f5c97ea3732ac538de7d6ed7e7cd0413c2ec91096bb652ad9bccf05d847ddbe81e7cd3cd44eb8030a51a5f00083871228b1b9b0b8398994f6f9f1

@jnewbery jnewbery deleted the jnewbery:load_unload_wallet branch May 16, 2018

@laanwj laanwj removed this from Blockers in High-priority for review May 17, 2018

@fanquake fanquake moved this from In progress to Done in Multiwallet support Jun 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.