Fix uninitialized URI in batch RPC requests #11277

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 really want that for 0.15, as I rely on this for TumbleBit, will test soon.

tACK c0c652ccc24f4c3dbd200f14822f10c373a5198d @laanwj I hope this can make it to 0.15. I enjoy multi-wallet, but I really can't use it in any project without the batch support, as it would require me to rewrite things that already work.

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?

needs rebase! utack c0c652c

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:

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

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

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

Updated 3ca5398f2efb3a09b9be65b6ee4aa7acff2c3cd9 -> 4526d21e52aa94f12121fcf01047c04f82c4990a (pr/mb.5 -> pr/mb.6) to get rid of operator overloading.

Chiming in to say that _call is fine -- it's just to indicate that it's an intended for internal use function in python, 'truly private' methods are double underscore.

'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:

struct A {
private:
void foo() {}
}
struct B {
void foo() {}
}

auto x = new A{};
((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...

@JeremyRubin Even if it works, I'm pretty sure that it's UB.

Can we add to milestone 0.15.1?

tACK

utACK 4526d21

Tested and working as expected. Mixing node with non-node requests now works fine. @laanwj any chance of backporting this to 0.15.1?

@ruimarinho Thanks for the ping. Somehow the backport label was missing and I forgot to include it in #11447

Removing backport tag: #11647