From edafc718ad071993d10b3b9a1e1828bbd1f8ce54 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 7 Sep 2017 17:20:26 -0400 Subject: [PATCH 1/6] Fix uninitialized URI in batch RPC requests This fixes "Wallet file not specified" errors when making batch wallet RPC calls with more than one wallet loaded. This issue was reported by NicolasDorier https://github.com/bitcoin/bitcoin/issues/11257 Request URI is not used for anything except multiwallet request dispatching, so this change has no other effects. Fixes #11257 --- src/httprpc.cpp | 2 +- src/rpc/server.cpp | 7 +++---- src/rpc/server.h | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/httprpc.cpp b/src/httprpc.cpp index 91f96ef2072..93f0a18668b 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -192,7 +192,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &) // array of requests } else if (valRequest.isArray()) - strReply = JSONRPCExecBatch(valRequest.get_array()); + strReply = JSONRPCExecBatch(jreq, valRequest.get_array()); else throw JSONRPCError(RPC_PARSE_ERROR, "Top-level object parse error"); diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index a73b697e01e..39bcfc6903f 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -389,11 +389,10 @@ bool IsDeprecatedRPCEnabled(const std::string& method) return find(enabled_methods.begin(), enabled_methods.end(), method) != enabled_methods.end(); } -static UniValue JSONRPCExecOne(const UniValue& req) +static UniValue JSONRPCExecOne(JSONRPCRequest jreq, const UniValue& req) { UniValue rpc_result(UniValue::VOBJ); - JSONRPCRequest jreq; try { jreq.parse(req); @@ -413,11 +412,11 @@ static UniValue JSONRPCExecOne(const UniValue& req) return rpc_result; } -std::string JSONRPCExecBatch(const UniValue& vReq) +std::string JSONRPCExecBatch(const JSONRPCRequest& jreq, const UniValue& vReq) { UniValue ret(UniValue::VARR); for (unsigned int reqIdx = 0; reqIdx < vReq.size(); reqIdx++) - ret.push_back(JSONRPCExecOne(vReq[reqIdx])); + ret.push_back(JSONRPCExecOne(jreq, vReq[reqIdx])); return ret.write() + "\n"; } diff --git a/src/rpc/server.h b/src/rpc/server.h index 31d63042710..74c4a9e8019 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -191,7 +191,7 @@ extern std::string HelpExampleRpc(const std::string& methodname, const std::stri bool StartRPC(); void InterruptRPC(); void StopRPC(); -std::string JSONRPCExecBatch(const UniValue& vReq); +std::string JSONRPCExecBatch(const JSONRPCRequest& jreq, const UniValue& vReq); // Retrieves any serialization flags requested in command line argument int RPCSerializationFlags(); From e02007aade3d449f030fe5c8b12beddd7df1b232 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 7 Sep 2017 17:29:26 -0400 Subject: [PATCH 2/6] Limit AuthServiceProxyWrapper.__getattr__ wrapping Change AuthServiceProxyWrapper.__getattr__ to only wrap proxied attributes, not real attributes. This way AuthServiceProxyWrapper can continue logging RPC calls without complicating other object usages, and special case handling for the .url property can be dropped. --- test/functional/test_framework/coverage.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/test/functional/test_framework/coverage.py b/test/functional/test_framework/coverage.py index 227b1a17afb..c0202e5609d 100644 --- a/test/functional/test_framework/coverage.py +++ b/test/functional/test_framework/coverage.py @@ -31,10 +31,11 @@ class AuthServiceProxyWrapper(object): self.auth_service_proxy_instance = auth_service_proxy_instance self.coverage_logfile = coverage_logfile - def __getattr__(self, *args, **kwargs): - return_val = self.auth_service_proxy_instance.__getattr__( - *args, **kwargs) - + def __getattr__(self, name): + return_val = getattr(self.auth_service_proxy_instance, name) + if not isinstance(return_val, type(self.auth_service_proxy_instance)): + # If proxy getattr returned an unwrapped value, do the same here. + return return_val return AuthServiceProxyWrapper(return_val, self.coverage_logfile) def __call__(self, *args, **kwargs): @@ -52,10 +53,6 @@ class AuthServiceProxyWrapper(object): return return_val - @property - def url(self): - return self.auth_service_proxy_instance.url - def __truediv__(self, relative_uri): return AuthServiceProxyWrapper(self.auth_service_proxy_instance / relative_uri) From 9f67646f173dd29464666b34de2ec9cfc480c11a Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 7 Sep 2017 17:38:11 -0400 Subject: [PATCH 3/6] Make AuthServiceProxy._batch method usable Split off AuthServiceProxy.get_request method to make it easier to batch RPC requests without duplicating code and remove leading underscore from _batch method. This does not change any existing behavior. --- test/functional/test_framework/authproxy.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test/functional/test_framework/authproxy.py b/test/functional/test_framework/authproxy.py index b3671cbdc51..747bda309c5 100644 --- a/test/functional/test_framework/authproxy.py +++ b/test/functional/test_framework/authproxy.py @@ -138,17 +138,20 @@ class AuthServiceProxy(object): self.__conn.request(method, path, postdata, headers) return self._get_response() - def __call__(self, *args, **argsn): + def get_request(self, *args, **argsn): AuthServiceProxy.__id_count += 1 log.debug("-%s-> %s %s"%(AuthServiceProxy.__id_count, self._service_name, json.dumps(args, default=EncodeDecimal, ensure_ascii=self.ensure_ascii))) if args and argsn: raise ValueError('Cannot handle both named and positional arguments') - postdata = json.dumps({'version': '1.1', - 'method': self._service_name, - 'params': args or argsn, - 'id': AuthServiceProxy.__id_count}, default=EncodeDecimal, ensure_ascii=self.ensure_ascii) + return {'version': '1.1', + 'method': self._service_name, + 'params': args or argsn, + 'id': AuthServiceProxy.__id_count} + + def __call__(self, *args, **argsn): + postdata = json.dumps(self.get_request(*args, **argsn), default=EncodeDecimal, ensure_ascii=self.ensure_ascii) response = self._request('POST', self.__url.path, postdata.encode('utf-8')) if response['error'] is not None: raise JSONRPCException(response['error']) @@ -158,7 +161,7 @@ class AuthServiceProxy(object): else: return response['result'] - def _batch(self, rpc_call_list): + def batch(self, rpc_call_list): postdata = json.dumps(list(rpc_call_list), default=EncodeDecimal, ensure_ascii=self.ensure_ascii) log.debug("--> "+postdata) return self._request('POST', self.__url.path, postdata.encode('utf-8')) From 505530c6cffeab8dc1f75f54ae0dfdcdb55d370b Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 4 Oct 2017 03:03:07 -0400 Subject: [PATCH 4/6] Add missing multiwallet rpc calls to python coverage logs This fixes a bug in coverage logging that's been around since the logging was introduced. --- test/functional/test_framework/coverage.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/functional/test_framework/coverage.py b/test/functional/test_framework/coverage.py index c0202e5609d..8101775ce1b 100644 --- a/test/functional/test_framework/coverage.py +++ b/test/functional/test_framework/coverage.py @@ -54,7 +54,8 @@ class AuthServiceProxyWrapper(object): return return_val def __truediv__(self, relative_uri): - return AuthServiceProxyWrapper(self.auth_service_proxy_instance / relative_uri) + return AuthServiceProxyWrapper(self.auth_service_proxy_instance / relative_uri, + self.coverage_logfile) def get_filename(dirname, n_node): """ From 74182f235cd04dcac7a8b3e763bc9add549745e1 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 4 Oct 2017 03:07:01 -0400 Subject: [PATCH 5/6] Add missing batch rpc calls to python coverage logs Without this change, batch RPC calls are not included in coverage logs. --- test/functional/test_framework/coverage.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/functional/test_framework/coverage.py b/test/functional/test_framework/coverage.py index 8101775ce1b..84049e76bc2 100644 --- a/test/functional/test_framework/coverage.py +++ b/test/functional/test_framework/coverage.py @@ -45,18 +45,24 @@ class AuthServiceProxyWrapper(object): """ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) + self._log_call() + return return_val + + def _log_call(self): rpc_method = self.auth_service_proxy_instance._service_name if self.coverage_logfile: with open(self.coverage_logfile, 'a+', encoding='utf8') as f: f.write("%s\n" % rpc_method) - return return_val - def __truediv__(self, relative_uri): return AuthServiceProxyWrapper(self.auth_service_proxy_instance / relative_uri, self.coverage_logfile) + def get_request(self, *args, **kwargs): + self._log_call() + return self.auth_service_proxy_instance.get_request(*args, **kwargs) + def get_filename(dirname, n_node): """ Get a filename unique to the test process ID and node. From 4526d21e52aa94f12121fcf01047c04f82c4990a Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 7 Sep 2017 17:40:25 -0400 Subject: [PATCH 6/6] Add test for multiwallet batch RPC calls Tests bug reported in https://github.com/bitcoin/bitcoin/issues/11257 --- test/functional/multiwallet.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/functional/multiwallet.py b/test/functional/multiwallet.py index b4e15a33221..ec808e95cd4 100755 --- a/test/functional/multiwallet.py +++ b/test/functional/multiwallet.py @@ -76,5 +76,9 @@ class MultiWalletTest(BitcoinTestFramework): assert_equal(w2.getbalance(), 1) assert_equal(w3.getbalance(), 2) + batch = w1.batch([w1.getblockchaininfo.get_request(), w1.getwalletinfo.get_request()]) + assert_equal(batch[0]["result"]["chain"], "regtest") + assert_equal(batch[1]["result"]["walletname"], "w1") + if __name__ == '__main__': MultiWalletTest().main()