Fix uninitialized URI in batch RPC requests #11277

ryanofsky · 2017-09-07T21:45:34Z

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.

NicolasDorier · 2017-09-10T06:04:28Z

I really want that for 0.15, as I rely on this for TumbleBit, will test soon.

NicolasDorier · 2017-09-10T07:16:42Z

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

jnewbery · 2017-09-11T13:55:31Z

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?

promag · 2017-09-13T20:59:07Z

src/rpc/server.cpp

@@ -382,11 +382,10 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
        throw JSONRPCError(RPC_INVALID_REQUEST, "Params must be an array or object");
 }

-static UniValue JSONRPCExecOne(const UniValue& req)
+static UniValue JSONRPCExecOne(JSONRPCRequest jreq, const UniValue& req)


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

promag · 2017-09-13T21:28:20Z

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());


Kind of unrelated, but above in L186 we could call 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.)

JeremyRubin · 2017-10-03T14:13:08Z

needs rebase! utack c0c652c

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> bitcoin#11257 Request URI is not used for anything except multiwallet request dispatching, so this change has no other effects. Fixes bitcoin#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.

ryanofsky

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

ryanofsky · 2017-10-03T21:49:35Z

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());


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

ryanofsky · 2017-10-03T21:51:52Z

src/rpc/server.cpp

@@ -382,11 +382,10 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
        throw JSONRPCError(RPC_INVALID_REQUEST, "Params must be an array or object");
 }

-static UniValue JSONRPCExecOne(const UniValue& req)
+static UniValue JSONRPCExecOne(JSONRPCRequest jreq, const UniValue& req)


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.

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 bitcoin#11257

ryanofsky · 2017-10-04T07:30:15Z

Added 2 commits 5217474 -> 3ca5398 (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.

jnewbery · 2017-10-04T17:42:39Z

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

ryanofsky · 2017-10-04T19:04:53Z

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

JeremyRubin · 2017-10-04T20:24:41Z

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.

jnewbery · 2017-10-04T21:08:00Z

'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

JeremyRubin · 2017-10-04T23:26:10Z

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.

laanwj · 2017-10-11T15:59:17Z

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

sipa · 2017-10-12T01:28:54Z

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

NicolasDorier · 2017-10-12T05:08:56Z

Can we add to milestone 0.15.1?

luke-jr

tACK

laanwj · 2017-10-12T12:54:39Z

utACK 4526d21

@NicolasDorier

4526d21 Add test for multiwallet batch RPC calls (Russell Yanofsky) 74182f2 Add missing batch rpc calls to python coverage logs (Russell Yanofsky) 505530c Add missing multiwallet rpc calls to python coverage logs (Russell Yanofsky) 9f67646 Make AuthServiceProxy._batch method usable (Russell Yanofsky) e02007a Limit AuthServiceProxyWrapper.__getattr__ wrapping (Russell Yanofsky) edafc71 Fix uninitialized URI in batch RPC requests (Russell Yanofsky) Pull request description: 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. Tree-SHA512: b3907af48a6323f864bb045ee2fa56b604188b835025ef82ba3d81673244c04228d796323cec208a676e7cd578a95ec7c7ba1e84d0158b93844d5dda8f6589b9

ruimarinho · 2017-11-01T03:07:51Z

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

maflcko · 2017-11-01T16:00:14Z

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

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> bitcoin#11257 Request URI is not used for anything except multiwallet request dispatching, so this change has no other effects. Fixes bitcoin#11257 Github-Pull: bitcoin#11277 Rebased-From: edafc71

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. Github-Pull: bitcoin#11277 Rebased-From: e02007a

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. Github-Pull: bitcoin#11277 Rebased-From: 9f67646

This fixes a bug in coverage logging that's been around since the logging was introduced. Github-Pull: bitcoin#11277 Rebased-From: 505530c

Without this change, batch RPC calls are not included in coverage logs. Github-Pull: bitcoin#11277 Rebased-From: 74182f2

Tests bug reported in bitcoin#11257 Github-Pull: bitcoin#11277 Rebased-From: 4526d21

maflcko · 2017-11-09T19:40:04Z

Removing backport tag: #11647

@NicolasDorier

4526d21 Add test for multiwallet batch RPC calls (Russell Yanofsky) 74182f2 Add missing batch rpc calls to python coverage logs (Russell Yanofsky) 505530c Add missing multiwallet rpc calls to python coverage logs (Russell Yanofsky) 9f67646 Make AuthServiceProxy._batch method usable (Russell Yanofsky) e02007a Limit AuthServiceProxyWrapper.__getattr__ wrapping (Russell Yanofsky) edafc71 Fix uninitialized URI in batch RPC requests (Russell Yanofsky) Pull request description: 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 bitcoin#11257 Request URI is not used for anything except multiwallet request dispatching, so this change has no other effect. Tree-SHA512: b3907af48a6323f864bb045ee2fa56b604188b835025ef82ba3d81673244c04228d796323cec208a676e7cd578a95ec7c7ba1e84d0158b93844d5dda8f6589b9

@NicolasDorier

4526d21 Add test for multiwallet batch RPC calls (Russell Yanofsky) 74182f2 Add missing batch rpc calls to python coverage logs (Russell Yanofsky) 505530c Add missing multiwallet rpc calls to python coverage logs (Russell Yanofsky) 9f67646 Make AuthServiceProxy._batch method usable (Russell Yanofsky) e02007a Limit AuthServiceProxyWrapper.__getattr__ wrapping (Russell Yanofsky) edafc71 Fix uninitialized URI in batch RPC requests (Russell Yanofsky) Pull request description: 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 bitcoin#11257 Request URI is not used for anything except multiwallet request dispatching, so this change has no other effect. Tree-SHA512: b3907af48a6323f864bb045ee2fa56b604188b835025ef82ba3d81673244c04228d796323cec208a676e7cd578a95ec7c7ba1e84d0158b93844d5dda8f6589b9

@NicolasDorier

4526d21 Add test for multiwallet batch RPC calls (Russell Yanofsky) 74182f2 Add missing batch rpc calls to python coverage logs (Russell Yanofsky) 505530c Add missing multiwallet rpc calls to python coverage logs (Russell Yanofsky) 9f67646 Make AuthServiceProxy._batch method usable (Russell Yanofsky) e02007a Limit AuthServiceProxyWrapper.__getattr__ wrapping (Russell Yanofsky) edafc71 Fix uninitialized URI in batch RPC requests (Russell Yanofsky) Pull request description: 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 bitcoin#11257 Request URI is not used for anything except multiwallet request dispatching, so this change has no other effect. Tree-SHA512: b3907af48a6323f864bb045ee2fa56b604188b835025ef82ba3d81673244c04228d796323cec208a676e7cd578a95ec7c7ba1e84d0158b93844d5dda8f6589b9

laanwj added the RPC/REST/ZMQ label Sep 7, 2017

ryanofsky force-pushed the pr/mb branch from b7205b8 to c0c652c Compare September 7, 2017 21:50

NicolasDorier mentioned this pull request Sep 10, 2017

[RPC] Bug: RPC Batching does not work with more than 1 multi wallet #11257

Closed

promag reviewed Sep 13, 2017

View reviewed changes

ryanofsky added 3 commits October 3, 2017 15:25

Make AuthServiceProxy._batch method usable

9f67646

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.

ryanofsky force-pushed the pr/mb branch from c0c652c to 1027a14 Compare October 3, 2017 22:25

ryanofsky commented Oct 3, 2017

View reviewed changes

ryanofsky added 3 commits October 4, 2017 03:03

Add missing multiwallet rpc calls to python coverage logs

505530c

This fixes a bug in coverage logging that's been around since the logging was introduced.

Add missing batch rpc calls to python coverage logs

74182f2

Without this change, batch RPC calls are not included in coverage logs.

Add test for multiwallet batch RPC calls

4526d21

Tests bug reported in bitcoin#11257

ryanofsky force-pushed the pr/mb branch from 1027a14 to 236a437 Compare October 4, 2017 07:21

ryanofsky force-pushed the pr/mb branch from 0611fae to 3ca5398 Compare October 4, 2017 07:34

ryanofsky force-pushed the pr/mb branch from 3ca5398 to 4526d21 Compare October 4, 2017 19:04

luke-jr approved these changes Oct 12, 2017

View reviewed changes

luke-jr mentioned this pull request Oct 12, 2017

Basic Multiwallet GUI support #11383

Closed

laanwj added this to the 0.15.1 milestone Oct 12, 2017

laanwj merged commit 4526d21 into bitcoin:master Oct 12, 2017

maflcko added the Needs backport label Nov 1, 2017

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 8, 2017

Add missing batch rpc calls to python coverage logs

1c8c7f8

Without this change, batch RPC calls are not included in coverage logs. Github-Pull: bitcoin#11277 Rebased-From: 74182f2

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 8, 2017

Add test for multiwallet batch RPC calls

3a6cdd4

Tests bug reported in bitcoin#11257 Github-Pull: bitcoin#11277 Rebased-From: 4526d21

maflcko removed the Needs backport label Nov 9, 2017

maflcko removed this from the 0.15.2 milestone Nov 9, 2017

jnewbery deleted the pr/mb branch November 9, 2017 19:42

jnewbery restored the pr/mb branch November 9, 2017 19:42

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Fix uninitialized URI in batch RPC requests #11277

Fix uninitialized URI in batch RPC requests #11277

Uh oh!

Conversation

ryanofsky commented Sep 7, 2017

Uh oh!

NicolasDorier commented Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Sep 11, 2017

Uh oh!

promag Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 3, 2017

Choose a reason for hiding this comment

Uh oh!

promag Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyRubin commented Oct 3, 2017

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 3, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Oct 4, 2017

Uh oh!

ryanofsky commented Oct 4, 2017

Uh oh!

JeremyRubin commented Oct 4, 2017

Uh oh!

jnewbery commented Oct 4, 2017

Uh oh!

JeremyRubin commented Oct 4, 2017

Uh oh!

laanwj commented Oct 11, 2017

Uh oh!

sipa commented Oct 12, 2017

Uh oh!

NicolasDorier commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 12, 2017

Uh oh!

ruimarinho commented Nov 1, 2017

Uh oh!

maflcko commented Nov 1, 2017

Uh oh!

maflcko commented Nov 9, 2017

Uh oh!

Uh oh!

NicolasDorier commented Sep 10, 2017 •

edited

Loading

NicolasDorier commented Sep 10, 2017 •

edited

Loading

ryanofsky Oct 3, 2017 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky Oct 3, 2017 •

edited

Loading

ryanofsky commented Oct 4, 2017 •

edited

Loading

NicolasDorier commented Oct 12, 2017 •

edited

Loading