This adds a new regtest file interface_rpc.py, containing a test for batch JSON-RPC requests. Those were previously not tested at all. Tests for basic requests are not really necessary, as those are used anyway in lots of other regtests.
The existing interface_http.py file is more about the underlying HTTP connection, so adding a new interface file for the JSON-RPC specific things makes sense.
29 | + {"method": "getblockcount", "id": 1}, 30 | + {"method": "getbestblockhash", "id": 2}, 31 | + # Request that will fail. The whole batch request should still 32 | + # work fine. 33 | + {"method": "invalidmethod", "id": 3}, 34 | + ]
nit, add another call here or move one of the above?
I was suggesting to put the invalid method between valid methods.
I see, that makes sense - changed.
There's a test for batching added in 4526d21, but it's part of the wallet tests so adding a general one is probably good.
30 | + {"method": "getbestblockhash", "id": 2}, 31 | + # Request that will fail. The whole batch request should still 32 | + # work fine. 33 | + {"method": "invalidmethod", "id": 3}, 34 | + ] 35 | + results = self.send_request(request)
you're not using the .batch() functionality of the RPC proxy?
No, as I didn't know it exists. Will update the code later.
Changed - that's indeed much simpler!
39 | + assert_equal(result_by_id[3]['error'], None) 40 | + assert_equal(result_by_id[3]['result'], self.node.getbestblockhash()) 41 | + 42 | + def run_test(self): 43 | + self.node = self.nodes[0] 44 | + self.node.generate(10)
Remove this (unnecessary for the test and also deprecated) and update accordingly?
Done
utACK 06ffc4b19b8ca8a8173caf5d3e90d0651e22d72d
38 | + 39 | + assert_equal(result_by_id[3]['error'], None) 40 | + assert_equal(result_by_id[3]['result'], self.node.getbestblockhash()) 41 | + 42 | + def run_test(self): 43 | + self.node = self.nodes[0]
nit, could just use self.nodes[0] above.
I chose to use self.node to make it more explicit in the code that there is only one and no index is needed, but I guess it is clear without that as well. Changed.
13 | + self.setup_clean_chain = True 14 | + 15 | + def test_batch_request(self): 16 | + self.log.info("Testing basic JSON-RPC batch request...") 17 | + 18 | + request = [
nit, could inline.
Done
20 | + {"method": "getblockcount", "id": 1}, 21 | + # Request that will fail. The whole batch request should still 22 | + # work fine. 23 | + {"method": "invalidmethod", "id": 2}, 24 | + # Another call that should succeed. 25 | + {"method": "getbestblockhash", "id": 3},
nit, could use getblockchaininfo to avoid getbestblockhash below.
Why would that be better / why should we "avoid" getbestblockhash? A function that just returns a string seems simpler to me than one that returns an object (that's why I chose it), but it doesn't really matter. So I'm happy to change it, but I don't see why that would be better?
Just to save another call when it's irrelevant for this test.
I see - but for that, getblockchaininfo won't help, either, as I would also have to call that on the node again to get the "expected" data. I could just call getblockcount again - would you prefer that?
Or alternatively just check that there is some result, without checking what it is.
Exactly, doesn't matter full expected result.
Ok, I've changed the code now to just check that result is not None without checking what it actually is (so that no second RPC is needed for that).
utACK 06ffc4b, just some nits.
Where is Travis?
23 | + {"method": "invalidmethod", "id": 2}, 24 | + # Another call that should succeed. 25 | + {"method": "getbestblockhash", "id": 3}, 26 | + ]) 27 | + 28 | + result_by_id = {}
What is the reason to index by id? Could do something like:
assert_equal(result[0]["id"], 1)
assert_equal(result[0]["error"], None)
assert_equal(result[0]["result"], 0)
...
since the response order matters.
One (minor) reason is that this verifies indirectly that the server handles the id field correctly. But the main reason is to make sure that we match up the right responses to requests. According to my understanding of the spec, the order is not guaranteed to be the same as in the request. (It is for Bitcoin Core's implementation, but that may change in the future.)
Indeed! From the specs:
The Server MAY process a batch rpc call as a set of concurrent tasks, processing them in any order and with any width of parallelism.
The Response objects being returned from a batch call MAY be returned in any order within the Array. The Client SHOULD match contexts between the set of Request objects and the resulting set of Response objects based on the id member within each Object.
This thread can be resolved then.
14 | + 15 | + def test_batch_request(self): 16 | + self.log.info("Testing basic JSON-RPC batch request...") 17 | + 18 | + results = self.nodes[0].batch([ 19 | + # A basic requests that will work fine.
Bad grammar?
Good catch, that is a left over from before the order was changed. Updated.
Thanks for the quick fixes @domob1812, looks great! just left 2 more comments.
This adds a new regtest file 'interface_rpc.py', containing a test for
batch JSON-RPC requests. Those were previously not tested at all. Tests
for basic requests are not really necessary, as those are used anyway
in lots of other regtests.
The existing interface_http.py file is more about the underlying HTTP
connection, so adding a new interface file for the JSON-RPC specific
things makes sense.
utACK 3d2c7d6.
utACK 3d2c7d6