ethereum / execution-specs

Specification for the Execution Layer. Tracking network upgrades.
Creative Commons Zero v1.0 Universal
867 stars 240 forks source link

test_ RPCs #208

Closed qbzzt closed 5 days ago

qbzzt commented 3 years ago

In the consensus testing project (https://github.com/ethereum/retesteth/) we have a number of RPC methods that are required to communicate with a client for our tests (https://github.com/ethereum/retesteth/wiki/RPC-Methods). May I add them to this project, in the hope that more clients will implement them?

lightclient commented 3 years ago

@qbzzt unfortunately I'm not sure the best way to do this. Right now, the spec mainly contains the eth RPC namespace. It's not clear to me where we should draw the line. There are other useful namespaces like test, personal, and wallet. I can see arguments for/against. Maybe @MicahZoltu @gregthegreek @timbeiko @tkstanczak have thoughts?

MicahZoltu commented 3 years ago

We initially said we wanted to focus on the eth_ namespace in order to figure out how we want things to look/work and establish a good workflow before expanding out into other areas.

At the same time, if someone is actively looking to contribute, but only toward a specific namespace (like test), the value of having a motivated editor may outweigh the downside of not waiting like originally planned.

timbeiko commented 3 years ago

I'll caveat all this with saying that this repo will likely undergo pretty extensive changes post-London as we figure out how to merge the Eth1 and Eth2 specs. It seems likely we'll end up with a set of consensus specs and non-consensus ones.

qbzzt commented 3 years ago

At the same time, if someone is actively looking to contribute, but only toward a specific namespace (like test), the value of having a motivated editor may outweigh the downside of not waiting like originally planned.

That is what I'm asking to do - not for you to write it, but for me to do it. Our testing project wants to have this documented to encourage more client developers to implement it so we'll be able to test directly (currently only besu does). I'll be doing the work.

lightclient commented 3 years ago

IIRC, geth has deprecated support for the test namespace in favor of t8n. Is that the case?

qbzzt commented 3 years ago

IIRC, geth has deprecated support for the test namespace in favor of t8n. Is that the case?

Yes, it has been that way for a while which is why we didn't care about documenting it. But now Besu is using it.

lightclient commented 3 years ago

IMO, the fact that geth actively removed the namespace from their client in pursuit of a different approach is a good enough reason to not include it in this repo. More broadly, is there a reason to continue pursuing the test namespace over t8n?

garyschulte commented 3 years ago

IMO, the fact that geth actively removed the namespace from their client in pursuit of a different approach is a good enough reason to not include it in this repo. More broadly, is there a reason to continue pursuing the test namespace over t8n?

There may be additional client diversity benefits regarding test development and correctness, but from a client testing perspective we need some conformed way to run the reference tests against clients.

In the absence of the test namespace, how should we validate that the reference tests pass for a given client? Besu actually has its own implementation of a reference test runner, but in some places it uses mocks and reference test-specific implementations. The net result is that the Besu reference test runner is much more permissive than retesteth and I have found many more test regressions using the retesteth runner than our in-house implementation.

IMO, if we want to be able to use the reference tests with a reference test engine, there should be a low friction way to do it.

garyschulte commented 3 years ago

I'll caveat all this with saying that this repo will likely undergo pretty extensive changes post-London as we figure out how to merge the Eth1 and Eth2 specs. It seems likely we'll end up with a set of consensus specs and non-consensus ones.

Seems we could add a third test-specific spec. Json-schema specs are composable, so ...

winsvega commented 3 years ago

the main benefit for us (test writers) having test RPC methods
that in case you could implement those rpc methods in such a way that it uses the core client logic - that would mean we would be teststing the client core logic directly.

t8ntool does not provide access to the client logic. there fore all blockchain logic is done on retesteth side (an actually there are a lot of failures currenlty with gasLimit checkups)

if we were to use besu RPC we would be able to generate besu's version of gasLimit formula checks not having all this issues.

lightclient commented 3 years ago

In the absence of the test namespace, how should we validate that the reference tests pass for a given client?

I think the current best way is to use the hive method. Basically, take the BlockchainTests and convert them to a genesis (header + alloc) and then apply blocks in order. At the end, ensure the latest block header is the expected one.

The way I understand it, the flow is generally:

  1. Write an ethereum/tests test.
  2. Use retesteth to fill the test.
  3. retesteth uses t8n to calculate the transition specifics (e.g. tx trie root, new state root, gas used, etc). For miner-attributable errors (failed validity condition), t8n continues to output a result so that retesteth can build invalid blocks.
  4. The BlockchainTest style tests have i) a genesis config (header + alloc) ii) a list of blocks to apply and iii) the expected final block. With that, hive can setup the environment for clients and then run {client} --import block-N.rlp. Once Hive runs out of blocks, it will check that latest block hash is the expected block hash.

So, everything driven by the initial ethereum/tests testcase.

If we were to use besu RPC we would be able to generate besu's version of gasLimit formula checks not having all this issues.

I'm not quite sure I understand the gasLimit checks you're referring to? The fact that it can move at most limit // 1024 up or down per block? Or did you mean the baseFee? I think I generally understand your point though - the baseFee for example is calculated for every block based on the gasUsed and gasLimit of the previous block. From the retesteth side, it would be nice if there were a black box to ask what the next baseFee will be. This way, retesteth doesn't do any computation itself. IUC (maybe @holiman can chime in), the reason t8n doesn't have these facilities builtin is that would not allow for invalid blocks to be made. E.g. currently you can specify any arbitrary values for baseFee. retesteth will fill the tests, but when the blocks are imported into a properly implemented client they will be rejected. If t8n filled those parameters automatically, this couldn't be easily tests. The general philosophy, it seems, is for the initial ethereum/test to drive everything. Calculating each baseFee-like parameter by hand is quite the burden though, so retesteth alleviates it to a degree.

Would it be possible to have a method test_calcBaseFee and test_calcGasLimit? Or maybe a cli interface for calculating some of these one of parameters? I think that would allow retesteth to follow the t8n-hive flow, without needing to implement consensus-specific functions.

that would mean we would be testing the client core logic directly.

Could you elaborate here? It seems like the hive framework does test the core logic directly? Or do I misunderstand your point?

lightclient commented 3 years ago

It's also possible that I still misunderstand how the test namespace is useful to retesteth. If so, please let me know :) I think it would be particularly useful to explain what the RPC end points provide that the above flow does not.

SamWilsn commented 5 days ago

This belongs over at https://github.com/ethereum/execution-apis