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

Branch

chronikelectrumtest

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 31625
Build 62747: Build Diff	build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 62746: arc lint + arc unit

Event Timeline

PiRK created this revision.Sat, Dec 7, 17:09

Herald added a reviewer: Restricted Project. · View Herald TranscriptSat, Dec 7, 17:09

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

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

Harbormaster completed remote builds in B31573: Diff 51473.Sat, Dec 7, 17:29

Harbormaster completed remote builds in B31574: Diff 51474.Sat, Dec 7, 17:41

tobias_ruck requested changes to this revision.Sat, Dec 7, 18:24

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

PiRK added inline comments.Sat, Dec 7, 20:59

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.

PiRK added inline comments.Sat, Dec 7, 21:03

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)

PiRK added inline comments.Sat, Dec 7, 21:07

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)

PiRK added inline comments.Mon, Dec 9, 08:28

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

Harbormaster completed remote builds in B31581: Diff 51487.Mon, Dec 9, 08:44

Fabien added a subscriber: Fabien.Mon, Dec 9, 13:09

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)

Harbormaster completed remote builds in B31614: Diff 51536.Wed, Dec 11, 09:35

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