Simple, backwards compatible RPC multiwallet support. #10829

Closed
wants to merge 1 commit into
from
Jump to file or symbol
Failed to load files and symbols.
+80 −2
Split
View
@@ -213,6 +213,12 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
std::string firstLetter = category.substr(0,1);
boost::to_upper(firstLetter);
strRet += "== " + firstLetter + category.substr(1) + " ==\n";
+ if (category == "wallet") {
+ strRet += "\nWhen more than one wallet is loaded (multiple -`wallet=filename` options passed\n"
@morcos

morcos Jul 15, 2017

Contributor

I think it might be nice to also add this string to each of the individual wallet helps?
Or if there is some way to return it when it is the cause of the error?

At least for me I rarely use the overall help or I grep it for the type of thing I'm looking for since it is so much output. And I worry that users will waste a lot of time not realizing what the problem is.

+ "to bitcoind), wallet RPCs must be called with an extra named JSON-RPC `wallet`\n"
+ "parameter containing the wallet filename to disambiguate which wallet file the\n"
+ "RPC is intended for. Failure to specify will result in method disabled errors.\n\n";
+ }
}
}
strRet += strHelp + "\n";
@@ -472,6 +478,11 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
hole += 1;
}
}
+ auto wallet = argsIn.find("wallet");
@morcos

morcos Jul 15, 2017

Contributor

Named arguments allow you to specify an argument more than once and then the last specified value controls.
I'm not sure if this is desirable behavior before this PR, but certainly after this PR it could lead to unexpected actions if for some reason multiple wallet arguments are specified.

@laanwj

laanwj Jul 17, 2017 edited

Owner

Named arguments allow you to specify an argument more than once and then the last specified value controls.

We could make it an error to specify a key twice, that seems acceptable in the context of JSON parsing because the standard doesn't say anything about duplicate keys (so it can depend on the context). But let's not add that to the scope of this PR.

(this would need to be enforced for any named argument, not just wallet=)

@morcos

morcos Jul 17, 2017

Contributor

Yes that makes sense to me.

+ if (wallet != argsIn.end() && wallet->second->isStr()) {
+ out.wallet = wallet->second->getValStr();
+ argsIn.erase(wallet);
+ }
// If there are still arguments in the argsIn map, this is an error.
if (!argsIn.empty()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Unknown named parameter " + argsIn.begin()->first);
View
@@ -42,11 +42,25 @@ class JSONRPCRequest
public:
UniValue id;
std::string strMethod;
+ /**
+ * Parameters from JSON-RPC request.
+ * This will be either an object or an array when the JSONRPCRequest object is
+ * originally created and parsed. But it will be transformed into an
+ * array before being passed to the RPC method implementation (using the
+ * list of named arguments provided by the implementation).
+ */
UniValue params;
bool fHelp;
std::string URI;
std::string authUser;
+ /**
+ * Optional wallet name, set for backwards compatibility if the RPC method
+ * was called with a named "wallet" parameter and the RPC method
+ * implementation doesn't handle it itself.
+ */
+ std::string wallet;
+
JSONRPCRequest() : id(NullUniValue), params(NullUniValue), fHelp(false) {}
void parse(const UniValue& valRequest);
};
View
@@ -32,8 +32,15 @@
CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
{
- // TODO: Some way to access secondary wallets
- return vpwallets.empty() ? nullptr : vpwallets[0];
+ if (!request.wallet.empty()) {
+ for (const auto& wallet : ::vpwallets) {
+ if (request.wallet == wallet->GetName()) {
+ return wallet;
+ }
+ }
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Requested wallet does not exist or is not loaded");
+ }
+ return ::vpwallets.size() == 1 || (request.fHelp && ::vpwallets.size() > 0) ? ::vpwallets[0] : nullptr;
}
std::string HelpRequiringPassphrase(CWallet * const pwallet)
@@ -0,0 +1,45 @@
+#!/usr/bin/env python3
+# Copyright (c) 2017 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test the wallet."""
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import *
+
+class MultiWalletTest(BitcoinTestFramework):
+
+ def __init__(self):
+ super().__init__()
+ self.setup_clean_chain = True
+ self.num_nodes = 1
+ self.extra_args = [['-wallet=w1', '-wallet=w2','-wallet=w3']]
+
+ def setup_network(self):
+ self.nodes = self.start_nodes(1, self.options.tmpdir, self.extra_args[:1])
+
+ def run_test(self):
+ self.nodes[0].generate(nblocks=1, wallet="w1")
+
+ #check default wallet balance
+ assert_raises_jsonrpc(-32601, "Method not found (disabled)", self.nodes[0].getwalletinfo)
+
+ #check w1 wallet balance
+ walletinfo = self.nodes[0].getwalletinfo(wallet="w1")
+ assert_equal(walletinfo['immature_balance'], 50)
+
+ #check w1 wallet balance
+ walletinfo = self.nodes[0].getwalletinfo(wallet="w2")
+ assert_equal(walletinfo['immature_balance'], 0)
+
+ self.nodes[0].generate(nblocks=101, wallet="w1")
+ assert_equal(self.nodes[0].getbalance(wallet="w1"), 100)
+ assert_equal(self.nodes[0].getbalance(wallet="w2"), 0)
+ assert_equal(self.nodes[0].getbalance(wallet="w3"), 0)
+
+ huh=self.nodes[0].getnewaddress(wallet="w2")
+ self.nodes[0].sendtoaddress(address=huh, amount=1, wallet="w1")
+ self.nodes[0].generate(nblocks=1, wallet="w1")
+ assert_equal(self.nodes[0].getbalance(wallet="w2"), 1)
+
+if __name__ == '__main__':
+ MultiWalletTest().main()
@@ -63,6 +63,7 @@
'segwit.py',
# vv Tests less than 2m vv
'wallet.py',
+ 'multiwallet.py',
'wallet-accounts.py',
'p2p-segwit.py',
'wallet-dump.py',