Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Universal specs for JSON-RPC API methods #217

Closed
cdetrio opened this issue Feb 16, 2017 · 22 comments
Closed

Universal specs for JSON-RPC API methods #217

cdetrio opened this issue Feb 16, 2017 · 22 comments
Labels

Comments

@cdetrio
Copy link
Member

cdetrio commented Feb 16, 2017

Universal specs for JSON-RPC API methods

This is a pre-draft outline of a Hive-based multi-client test suite for the JSON-RPC API. The specification format is JSON schema (inspired by the Kodi API description).

Existing RPC test suites include ethereum/rpc-tests and bas-vk/hive/tree/rpc. The former is based on mocha.js and the latter on go-ethereum's ethclient.Client api. This proposal intends to be a language and client-agnostic test and specification format.

Here's an example JSON schema specification, for eth_getBlockTransactionCountByHash, annotated for explanation:

{
    "$schema": "http:https://json-schema.org/draft-04/schema#",
    "id": "eth_getBlockTransactionCountByHash",
    "title": "eth_getBlockTransactionCountByHash",
    "description": "eth_getBlockTransactionCountByHash JSON-RPC method request and response schema.",

    "request": {
      "id": "#request",
      "allOf": [ // allOf means the request must pass both sub-schemas
        { "$ref": "jsonrpc-request.json" }, // first-pass generic validation, see file https://github.com/cdetrio/interfaces/blob/master/rpc-specs-tests/schemas/jsonrpc-request.json
        { "$ref": "#/definitions/request-obj" } // second-pass validation, specific to the RPC method
      ],
      "definitions": {
        "request-obj": {
          "id": "#request-obj",
          "properties": {
            "method": {
              "type": "string",
              "enum": ["eth_getBlockTransactionCountByHash"]
            },
            "params": {
              "type": "array",
              "items": [
                {
                  "type": "string",
                  "description": "DATA, 32 Bytes - Hash of a block."
                }
              ]
            }
          }
        }
      }
    },

    "response": {
      "id": "#response",
      "allOf": [
        { "$ref": "jsonrpc-response.json" },
        { "$ref": "#/definitions/response-obj" }
      ],
      "definitions": {
        "response-obj": {
          "id": "#response-obj",
          "properties": {
        		"result": {
              "type": "string",
              "description": "QUANTITY - integer of the number of transactions in this block."
        		}
        	}
        }
      }
    }
}

The description fields are not used for validation, but for generating RPC API method documentation as seen on the wiki.

The JSON schema specifications are paired with test cases. Here's an example file with test cases for eth_getBlockTransactionCountByHash:

{
  "title" : "eth_getBlockTransactionCountByHash",

  "schema": {
    "$ref": "../schemas/eth_getBlockTransactionCountByHash.json"
  },

  "chainConfig" : {
    "$ref": "../configs/bcRPC_API_Test.json"
  },

  "tests": [
    {
      "title": "eth_getBlockTransactionCountByHash for block with one tx",
      "request" : {
        "method" : "eth_getBlockTransactionCountByHash",
        "params" : ["0x4e9a67b663f9abe03e7e9fd5452c9497998337077122f44ee78a466f6a7358de"]
      },
      "expectedResponse" : {
        "result": "0x1"
      },
      "asserts": [
        {
          "description": "response is not empty",
          "program": ".receivedResponse.result != null"
        },
        {
          "description" : "transaction count should be equal",
          "program" : ".receivedResponse.result == .expectedResponse.result"
        }
      ]
    }
  ]
}

The asserts array specifies jq programs or "filters", which are used to compare the received response against the expected response.

The chainConfig option points to a set of blocks/transactions to import. The current chain configuration is inherited from ethereum/rpc-tests, which in turn borrows from the state tests repo. This configuration file is loaded by the test runner, and imported by the hive clients.

After hive runs the RPC tests against a set of clients, the results are displayed as a compatibility table: http:https://cdetrio.github.io/eth-compat-table/ (inspired by the table for ES6).

Todos

One remaining task is to specify a test format for the pub/sub methods. One possibility is a transitionEvent field to specify an event (e.g. arrival of a block or a pending transaction), and an onTransition field to describe the expected notifications.

Test suite maintenance

An automated maintenance process for the RPC test suite will be described in a forthcoming Meta-EIP. The current plan is to maintain updates to the RPC test suite through a voting contract, with one-developer one-vote where developer identities are verified using devcon2 tokens. (There is a separate plan to expand the Devcon2 tokens into a web of trust, in order to enable participation by any and all Ethereum developers/stakeholders). Tentatively named EipSignal, this automated governance process is somewhat in the spirit of the Yellow Paper Committee; however, the outcome of EipSignal votes will only mandate changes to the RPC API specification and test suite as described above, and will NOT govern "core/consensus" protocol changes. The work-in-progress on EipSignal can seen here: MetaMask/IPFS-Ethereum-Hackathon#14.

@axic
Copy link
Member

axic commented Feb 16, 2017

👍

The asserts array specifies jq programs or "filters", which are used to compare the received response against the expected response.

Is it possible to include strict verifications, such as a subset of regular expressions on some data fields? Thinking here about validating that an actual address was returned for example by personal_newAccount.

The chainConfig option points to a set of blocks/transactions to import. The current chain configuration is inherited from ethereum/rpc-tests, which in turn borrows from the state tests repo.

Have you seen cpp-ethereum's test_setChainParams RPC method? I think that might be the successor of that format.

@danfinlay
Copy link
Contributor

This is very exciting to me. Developers need a place they can look up stable methods, and client devs need a smooth way to evolve their interfaces.

Other clients that will be nice to add to the tested columns:

And I'm sure others, too. Once we have a strong singular test suite like this, building new RPC providers should become a much more stable process.

@bobsummerwill
Copy link

Really great to see this happening, @cdetrio, @FlySwatter!

Starting with the RPCs makes sense, and I hope that the patterns which are built can be extended to broader specifications over time. We've had discussions along very similar lines for Enterprise using Boardroom.

See also http:https://html5test.com.

@cdetrio
Copy link
Member Author

cdetrio commented Feb 20, 2017

Is it possible to include strict verifications, such as a subset of regular expressions on some data fields?

Yes, actually there's two ways to enforce a regexp. It can be done in the JSON schema using pattern, or as a test case using a jq filter.

@cdetrio
Copy link
Member Author

cdetrio commented Feb 20, 2017

Have you seen cpp-ethereum's test_setChainParams RPC method? I think that might be the successor of that format.

Ah, here's an issue on that: ethereum/interfaces#4

@cdetrio
Copy link
Member Author

cdetrio commented Feb 20, 2017

There's also https://github.com/ethcore/ethereum-rpc-json, which generates documentation but doesn't include tests to run against clients.

@rphmeier
Copy link

rphmeier commented Mar 20, 2017

http:https://cdetrio.github.io/eth-compat-table/

Seems like it measures compatibility against Geth, not against any spec.

@bobsummerwill
Copy link

bobsummerwill commented Mar 20, 2017

Hey @rphmeier.

See "Test suite maintenance" at the very top of this issue.

The existing tests are incomplete placeholders. The plan is to expand/refine those to become the missing client-neutral executable specification.

An automated maintenance process for the RPC test suite will be described in a forthcoming Meta-EIP. The current plan is to maintain updates to the RPC test suite through a voting contract, with one-developer one-vote where developer identities are verified using devcon2 tokens. (There is a separate plan to expand the Devcon2 tokens into a web of trust, in order to enable participation by any and all Ethereum developers/stakeholders). Tentatively named EipSignal, this automated governance process is somewhat in the spirit of the Yellow Paper Committee; however, the outcome of EipSignal votes will only mandate changes to the RPC API specification and test suite as described above, and will NOT govern "core/consensus" protocol changes. The work-in-progress on EipSignal can seen here: MetaMask/IPFS-Ethereum-Hackathon#14.

@danfinlay
Copy link
Contributor

@bobsummerwill beat me to it, but I'll still reword:

@rphmeier

Seems like it measures compatibility against Geth, not against any spec.

There is no current spec to measure against, which is why this EIP exists.

The current plan is to manage this test suite with a form of governance, at which point non-geth-compliant specs may emerge, or even become dominant.

Until then, the linked test suite should be treated as a template for specs, not as a proposed spec itself.

@rphmeier
Copy link

A test suite isn't a specification, it just tests conformity to some specification. The schema described here is not sufficient to specify certain primitives which all methods rely upon. This is exactly how you get into the situation of strange corner cases that each client implements differently while still being "to-spec".

The places where I see the current documentation lacking the most:

  • definition of pending state
  • definition of error messages
  • lack of consensus between client devs over changes

I'm glad that this EIP is working on point 3. But we need strong definitions of the primitives used to build the RPC methods rather than specific test cases which will just lead to overfitting.

It's misleading to label one specific implementation of ambiguous methods as "correct".

@danfinlay
Copy link
Contributor

danfinlay commented Mar 20, 2017

@rphmeier

I think we're actually starting to stray beyond the scope of this issue, but there isn't a better place for it. This issue is really about an RPC test format. We're beginning to talk about the governance process of managing those tests.

I understand the initial alarm, it could sound like someone is trying to hijack what "correct" means, but that's not our intention at all. We're not trying to shame Parity over a specific difference from Geth, we're trying to create a tool for all client devs to better coordinate their implementations.

The (current, in flux) plan is that a given suite of tests would accompany an EIP spec, not replace it. For example, for a given EIP, a variety of accompanying test suites could be nominated, and the most popular one(s?) could be displayed on the compatibility table.

Since the most popular test suite for a given EIP would be displayed, the test would not itself be an endorsement of that EIP spec, it would merely be a representation of it, and a way of automatically measuring compliance to it, which better represents the sort of ad-hoc standards process we actually have, where endorsement is almost solely represented through client implementation.

It certainly would not be a spec itself, since the table could include distinct, incompatible implementations of the same method, but it would provide a singular place to view and compare those incompatibilities. This table would not be about labeling any one implementation as correct, but instead, be a way of highlighting differences of implementation, for the benefit of both dapp and client developers.

I don't think a test suite has to encourage over-fitting, either. In the test format that @cdetrio has proposed, specific values of the response can be used to validate results, so it does not need to blindly enforce bit-conformity.

In the case of personal_sign, a reasonable test might check if a certain account with a published private key, given a certain message, produces a consistent signed output, and no more. This is not a spec, but adhering to it, and publishing this sample data, could have alleviated many headaches that came from having an under-specified feature adopted by a client.

I'm very interested in seeing solutions to 1 & 2 as well, and while a test suite may not be a complete solution for them, I think it may still be a useful tool for even those issues, when combined with EIP specifications.

@cdetrio
Copy link
Member Author

cdetrio commented Apr 6, 2017

@rphmeier Good points, I agree that pending state and error messages are two important areas to address.

EIP #136 addresses RPC error messages, and I think the schema here is sufficient to incorporate those error messages.

Pending state is trickier. Do you have any suggestions for how to define the pending state as a primitive?

I'm still unsure about this, but one thought is that perhaps an additional testing-specific RPC method would be useful, such as a test_setPendingBlock(block_rlp) method, similar to the test_addBlock(block_rlp) already supported by cpp-ethereum. Such RPC methods would make it easy to test assertions in a transitionEvent field (mentioned in the Todos above). On the other hand, I dislike the idea of test-specific RPC methods because it could lead to clients implementing the correct behavior for the test methods, but unexpected behavior in normal use. An example of this is ethereum/go-ethereum#2897, where eth_sendTransaction applies tx's to the pending block only if the client is not mining (if the client is mining, then you have to wait a block before the tx is applied to the pending state). Preventing these kinds of corner cases is the goal of black-box testing using Hive (as opposed to unit tests). I'd prefer to just use eth_sendTransaction and assert that the tx is applied immediately, specifying it as incorrect behavior if not, regardless of whether the client is mining.

On the third point, "lack of consensus between client devs over changes", that will be addressed by a separate EIP/meta-EIP, which will propose a governance/maintenance process. However, I wouldn't mind bootstrapping it with an initial test suite of "compatible/correct" behavior, determined informally by pre-existing standards and rough consensus. The current examples were generated against geth (two incompatible geth branches, in fact), but they are certainly only meant as examples.

The discussion here should focus on the spec or test schema/format. The term "spec" is used loosely, in the sense of an API spec, where the aim is to enable black-box testing of RPC method behavior. We want to settle on a format that is sufficient to specify behavior (at least implicitly if not explicitly) that is currently ambiguous, and test compliance.

@MicahZoltu
Copy link
Contributor

What I see as a major hole in these tests is that they don't appear to have a mechanism for setting up starting state. What these tests really should be testing is along the lines of:

Given state A; call RPC method B then C; expect result D

This means tests necessarily need to be more complex than a simple "given request, expect result". One way to achieve this (pseudocode only):

initializeGenesisBlock()
transactionHash = submitTransaction(myTransaction)
expect(transactionHash).to.not.be.null
mineAllTransactions()
newBlock = rpc_eth_getBlockByNumber("latest")
expect(newBlock.transactions).to.contain(transactionHash)

Alternatively, the test could explicitly setup expected state. For example:

startingState: [
	accounts: [
		{ address: "0x1234abcd", eth: "5", signingStrategy: "SIGN_EVERYTHING" },
	],
	blocks: [
		{ transactions: [], uncles: [], hash: "0xabcd1234", ... },
		{ transactions: [...], uncles: [...], hash: "...", ... },
	],
	contractState: [
		...
	],
	...
]

Then the test would call a one or more RPC methods and assert that it gets back a very well defined response.

The key here is that all tests should setup the current full chain state, then call a particular RPC method and assert on the results. Building a test suite like this is expensive, but is also valuable for multi-implementation compatibility. Note that this test doesn't care how an implementation achieves the result (implementation details), but it is well defined that given a particular state and an RPC call, an expected outcome is achieved.

As to how to codify "setup", I'll leave that up for discussion but I do think it is critical that COMPLETE setup is included in the test. The test should assume nothing about the current state of the world when it executes.

Personally, I'm a fan of the first style because it is generally easier to write tests when the test defines the path to the expected state, rather than requiring that the thing under test has the ability to set the current state. The first mechanism also allows true black box testing, where the test can be run against an endpoint without knowing its implementation. The second mechanism requires that the test have a back-door mechanism to setup current state.

danfinlay added a commit to danfinlay/ethjs-schema that referenced this issue Jun 8, 2017
[It has been brought to my attention](MetaMask/metamask-extension#1557) that this (and in turn MetaMask's) personal_sign param ordering is incorrect.

How did this happen? Was it [the lack of a standard RPC test suite?](ethereum/EIPs#217) was it just my mistake? I'll claim both.

The Wiki seems to be accurate in correcting us here, too:
https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_sign
SjonHortensius pushed a commit to SjonHortensius/ethjs-schema that referenced this issue Jul 31, 2017
[It has been brought to my attention](MetaMask/metamask-extension#1557) that this (and in turn MetaMask's) personal_sign param ordering is incorrect.

How did this happen? Was it [the lack of a standard RPC test suite?](ethereum/EIPs#217) was it just my mistake? I'll claim both.

The Wiki seems to be accurate in correcting us here, too:
https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_sign
@holiman
Copy link
Contributor

holiman commented Feb 15, 2018

I just now stumled upon this when searching for something like swagger to define and generate documentation for our json-rpc apis. Unfortunately, swagger isn't for json-rpc.

@cdetrio Have you found any good editors to write/generate the specifications above? Or did you do it by hand?
I also found jrgen, which is more geared towards our usecase, but doesn't appear quite as handy as the swagger editor.

I'd really like to have not only a specification, but also an editor which makes writing/validating the specification intuitive and simple. If it involves writing files by hand, then running some npm script to validate and generate docs based on our custom specification file, then I'm afraid it will just bitrot after a while.

@winsvega
Copy link
Contributor

winsvega commented Jun 7, 2018

Totally up for this.
I could write the boost test cases and you could run retesteth tool against any client to check request-response schemas.

@pipermerriam
Copy link
Member

@holiman I think I'd like to pick this up on my team. My thought was that we could add these schemas to the ethereum/tests repository and document how they should be run against the client's JSON-RPC responses. It'd still be up to each client to implement the tests.

@bobsummerwill
Copy link

Do it, @pipermerriam. That would be awesome!

When I spoke at the launch event for the EEA back in February 2017, 17 months ago, this was actually one of the areas of protocol specification which I highlighted as being most needed and highest impact - both for public Ethereum and for enterprise scenarios.

The lack of clear and testable JSON-RPC specifications really hurts everyone trying to build on top of Ethereum.

What are PegaSys doing for RPCs in Pantheon, @shahankhatch? Are you just trying to be close to Geth? Or did you do some modal thing like Parity have?

@bobsummerwill
Copy link

@rphmeier What do you see as the best path towards standardization of RPC specs across the various Ethereum clients, Robert?

As you said last year, there is a balance here to ensure that we don't "over-fit", and to ensure that any automated tests that we build are encoding unambiguous original specifications.

Any thoughts, @gavofyork?

@winsvega
Copy link
Contributor

retesteth is capable of running response requests check against clients via RPC
we just need to come up with default parameters that node will report to those commands.
eg. peercount 0. accounts - empty and so on.

@ethers
Copy link
Member

ethers commented Oct 14, 2018

Proposal: https://ethereum-magicians.org/t/eip-remote-procedure-call-specification/1537

@github-actions
Copy link

github-actions bot commented Jan 2, 2022

There has been no activity on this issue for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the stale label Jan 2, 2022
@github-actions
Copy link

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants