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 in #11257
Request URI is not used for anything except multiwallet request dispatching, so this change has no other effect.
I don’t like the fact that you’re creating a ‘private’ _call method in the AuthServiceProxy class, and then calling it (and the existing private method _batch) from a test script.
One of the advantages of the TestNode class is that on the whole the test scripts don’t interface directly with the AuthServiceProxy class. That was deliberate and would allow us to:
AuthServiceProxyWrapper coverage class easilyIs there a way you can add the batch functionality to the TestNode class?
381@@ -382,11 +382,10 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
382 throw JSONRPCError(RPC_INVALID_REQUEST, "Params must be an array or object");
383 }
384
385-static UniValue JSONRPCExecOne(const UniValue& req)
386+static UniValue JSONRPCExecOne(JSONRPCRequest jreq, const UniValue& req)
const std::string& uri instead?
Receive const std::string& uri instead?
Don’t see the advantage, and the disadvantage is it leaves this code open to the exact same bug in the future if another field is added to JSONRPCRequest. Better to copy the whole object, IMO.
191@@ -192,7 +192,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
192
193 // array of requests
194 } else if (valRequest.isArray())
195- strReply = JSONRPCExecBatch(valRequest.get_array());
JSONRPCExecOne I believe.
Kind of unrelated, but above in L186 we could call JSONRPCExecOne I believe.
It would be good to unify the code paths more, but this wouldn’t be equivalent because it would handle errors differently. (Execone doesn’t use JSONErrorReply, so doesn’t set http error codes.)
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 <nicolas.dorier@gmail.com>
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
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.
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.
I don’t like the fact that you’re creating a ‘private’ _call method in the AuthServiceProxy class, and then calling it (and the existing private method _batch) from a test script.
One of the advantages of the TestNode class is that on the whole the test scripts don’t interface directly with the AuthServiceProxy class. That was deliberate and would allow us to: remove the slightly cludgy AuthServiceProxyWrapper coverage class easily modify the auth service proxy or even swap it out for a new implementation without having to change the scripts.
Is there a way you can add the batch functionality to the TestNode class?
All I’m really doing here is adding a simple 3-line test to multiwallet.py using the existing _batch method:
0batch = w1._batch([w1.getblockchaininfo._get_request(), w1.getwalletinfo._get_request()])
1assert_equal(batch[0]["result"]["chain"], "regtest")
2assert_equal(batch[1]["result"]["walletname"], "w1")
This only required two minor improvements to AuthServiceProxy and AuthServiceProxyWrapper, so I think the approach is good for this PR, which is supposed to be an RPC bugfix and not a test framework restructuring.
But to discuss possible future test framework improvements, TestNode seems does not seem like a great place to move RPC sending code. The division where authproxy handles json & http stuff and test_node handles test stuff seems like a good one, and I think don’t think it’d be desirable to scramble it all up without a clear motivation. I also don’t think that batch rpc requests should follow a different code path from single rpc requests.
As far as I know, nothing in this PR makes it more difficult to remove AuthServiceProxyWrapper in the future if that is a goal. The actual change I’m making to AuthServiceProxyWrapper is a bugfix and cleanup that makes the class less invasive.
If calling methods that begin with underscores is a problem (and I think it’s a pretty superficial problem if it is a problem at all), we could simply drop the underscores, or make get_request and batch be standalone functions instead of member functions, or be operators like % and «.
This fixes a bug in coverage logging that's been around since the logging was
introduced.
Without this change, batch RPC calls are not included in coverage logs.
Tests bug reported in https://github.com/bitcoin/bitcoin/issues/11257
Added 2 commits 52174747fd82db3118946d36e2b067fb5c41176e -> 3ca5398f2efb3a09b9be65b6ee4aa7acff2c3cd9 (pr/mb.4 -> pr/mb.5, compare)
I went ahead and added % and « operators for making batch rpc calls in a new commit. I realized batch rpc calls weren’t being included in coverage logs, and operator syntax seemed like a less invasive way than overriding _get_reqest and _batch methods for AuthServiceProxyWrapper to be able to log the calls.
If calling methods that begin with underscores is a problem (and I think it’s a pretty superficial problem if it is a problem at all), we could simply drop the underscores, or make get_request and batch be standalone functions instead of member functions, or be operators like % and «.
The new operator overrides are cute, but perhaps unnecessary.
My objection to using ‘private’ methods in the test script is simply one of convention. Obviously leading underscores don’t mean anything to the python interpreter, but to us humans it means ’this method/member is private and won’t be accessed directly outside of this class/module’. There’s no need to break that assumption needlessly.
But to discuss possible future test framework improvements, TestNode seems does not seem like a great place to move RPC sending code.
I disagree, but like you say this is a PR for fixing a bitcoind bug and not test framework restructuring. This PR does seem to be an improvement to the AuthServiceProxyWrapper class.
Concept ACK with a weak preference to remove the syntactic sugar and rename the methods to make them ‘public’.
’truly private’ methods are double underscore.
There’s no such thing as truly private methods in Python: https://docs.python.org/3/tutorial/classes.html#private-variables
I mean, it’s semantics, but technically neither does C++ because you can get around it (in some cases; like without private static globals?) as follows:
0struct A {
1private:
2void foo() {}
3}
4struct B {
5void foo() {}
6}
7
8auto x = new A{};
9((B*) x)->foo();
In any case, the scare quotes were around ’truly private’: it’s not truly private, but it does make you call a mangled name. Then again, not many languages stop you from manipulating an object in your memory space if you want to manipulate it.
@JeremyRubin That only works for virtual methods, AFAIK, and it works because the slot is at the same offset in the vtable. Non-virtual methods have the class name encoded into the symbol name
That said, let’s try to respect private methods even though it’s not perfectly enforced, it’s meant as a convention to communicate to other people working on the code that the method shouldn’t be used externally…
