Page MenuHomePhabricator

[chronik] add a test for the new JSON RPC interface
ClosedPublic

Authored by PiRK on Sat, Dec 7, 17:09.

Details

Reviewers
tobias_ruck
Fabien
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABCcedd98bd44e8: [chronik] add a test for the new JSON RPC interface
Summary

Add a basic functional test for the new Chronik Electrum interface added in D17185.
This adds a few methods to the test framework that can be reused for further testing of JSON RPC interface as more methods are added.

Reduce the port range again to accomodate for this third family of ports (see D16048)

Ref T3598

Test Plan

ninja check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Sat, Dec 7, 17:09

check also the id in the response matches the one in the query

tobias_ruck added a subscriber: tobias_ruck.
tobias_ruck added inline comments.
test/functional/chronik_electrum.py
17 ↗(On Diff #51474)

shouldn't we rather have this on by default in the test framework, for consistency with the other ports?

test/functional/test_framework/jsonrpc_utils.py
48 ↗(On Diff #51474)

is this how to do it? what if there's two consecutive responses in the TCP stream, wont they be garbled in data and break json.loads?

This revision now requires changes to proceed.Sat, Dec 7, 18:24
test/functional/test_framework/jsonrpc_utils.py
48 ↗(On Diff #51474)

I'm still no specialist in network protocols, but afaict if we do it in this blocking way we should not have any such conflict.
And even if we go for some gancy async programming, we should still use a single socket between the client and the server and they need to ensure to write data sequentially to it somehow with a lock.

In ElectrumABC i think there is a single thread that does all of this socket communication, and other threads can pass it the requests and fetch the responses from a queue.

test/functional/test_framework/jsonrpc_utils.py
40 ↗(On Diff #51474)

I need to not write id if it is None.
Currently i don't see a use case for a request with no id but it is possible as per the jsonrpc spec (notification)

test/functional/test_framework/jsonrpc_utils.py
48 ↗(On Diff #51474)

I mean if the responses are not garbled but just consecutive then we need to properly handle it (parse the data on newline characters...). But for now this can't happen with this synchronous way of requesting and immediately waiting (blocking) for the reply.

  • set the chronikelectrumbind param by default
  • rename jsonrpc_utils -> jsonrpctools.py (consistency with blocktools, txtools, avatools...)
  • set "id" and "params" only if they are not None. Both of these may be omitted as per spec (https://www.jsonrpc.org/specification#notification)
test/functional/test_framework/util.py
473 ↗(On Diff #51487)

I have mixed feelings about having this server enabled by default. On the positive side, we will have better coverage and guarantees that it does not break anything else. On the negative side it is a mostly useless overhead in every other chronik test to enable this.

473 ↗(On Diff #51487)

And possibly increased risks of causing port collisions

Fabien added inline comments.
test/functional/test_framework/util.py
473 ↗(On Diff #51487)

I'd rather have overhead than lower coverage

Fabien requested changes to this revision.Tue, Dec 10, 09:06
Fabien added inline comments.
test/functional/chronik_electrum.py
12 ↗(On Diff #51487)

ChronikElectrumBasic or similar. We will likely have one test per endpoint in the the end. Same for the file name.

29 ↗(On Diff #51487)

See comment below, this will be duplicated everywhere. While this is fine for checking the protocol errors, it's not a good api for calls like server.ping()

test/functional/test_framework/jsonrpctools.py
23 ↗(On Diff #51487)

You probably want an API that looks like the test_node, using __getattr__ to create the call.
e.g.:

electrum = ElectrumClient(node)  # node having the ip and port as attributes that can be extracted
electrum.server.ping() # The server attribute doesn't exist, call server.ping() method with no param.

Please look at how it works for the node RPC because you can certainly reuse most of the code.

46 ↗(On Diff #51487)

You need a boundary condition. If the server never respond, imagine what happens.

This revision now requires changes to proceed.Tue, Dec 10, 09:06

rework the API, use a client object similar to ChronikClient, set a timeout for the socket so that we get an error if the server never replies (or never sends the final \n)

Fabien requested changes to this revision.Wed, Dec 11, 10:37

The API is now very good imo

test/functional/chronik_electrum_basic.py
15 ↗(On Diff #51536)

It's now unused

test/functional/test_framework/jsonrpctools.py
101 ↗(On Diff #51536)

You need to add a size limit to the message also. Otherwise you could fill up your memory as long as you get data that doesn't contain the \n char

test/functional/test_framework/test_node.py
969 ↗(On Diff #51536)

That's not a very good comment :)

Also can this be included if chronik is not build (unlike the chronik client) ? I think so better check

This revision now requires changes to proceed.Wed, Dec 11, 10:37
test/functional/test_framework/test_node.py
969 ↗(On Diff #51536)

Yeah there should be no issue when importing this if chronik is not built. This has no dependency on chronik's protobuf file.

raise an error if response data is too large, improve doc for ChronikElectrumClient with code samples, remove unneed id param for MethodNameProxy

test/functional/chronik_electrum_basic.py
29–35 ↗(On Diff #51550)

Not sure how much we should test the internals of karyon_jsonrpc. We could also just test response.error is not None)

Fabien added inline comments.
test/functional/chronik_electrum_basic.py
29–35 ↗(On Diff #51550)

I think it's fine as is

PiRK added a task: Restricted Maniphest Task.
This revision is now accepted and ready to land.Mon, Dec 16, 10:03
PiRK edited the summary of this revision. (Show Details)

reduce port range to 3000 to keep the same effective range of ports

@bot build-linux64 build-linux-aarch64 build-chronik build-chronik-plugins