search

ethereum / execution-apis

Collection of APIs provided by Ethereum execution layer clients
Creative Commons Zero v1.0 Universal
967 stars 380 forks source link

Proposal: Error handling specification #1 #286

Open JosephK95 opened 2 years ago

JosephK95 commented 2 years ago

Hi all,

I am working on a project that validates whether four Ethereum client implementations (Geth, Erigon, Nethermind, Besu) behave consistently to one another.

And while doing the experiments, I found out that the clients handle error cases in very much different ways:

  1. Invalid block argument to getBlockTransactionCount

    • (Geth, Erigon, Besu) returns null
    • (Nethermind) returns an Error

  2. Invalid block argument to getPastLogs

    • (Geth, Erigon, Besu) returns []
    • (Nethermind) returns an Error

  3. Invalid block argument to getTransactionFromBlock

    • (Geth, Erigon, Besu) returns null
    • (Nethermind) returns an Error

  4. Invalid block argument to getBlockUncleCount

    • (Geth, Erigon, Besu) returns null
    • (Nethermind) returns an Error

  5. Invalid block argument to getUncle

    • (Geth, Erigon, Besu) returns null
    • (Nethermind) returns an Error

  6. Invalid block argument to getCode (1)

    • (Geth, Nethermind, Besu) returns an Error
    • (Erigon) returns the code of the address

  7. Invalid block argument to getCode (2)

    • (Geth, Erigon, Nethermind) returns an Error
    • (Besu) returns null

  8. Invalid block argument to getBalance

    • (Geth, Erigon, Nethermind) returns an Error
    • (Besu) returns null

  9. Invalid block argument to getTransactionCount

    • (Geth, Erigon, Nethermind) returns an Error
    • (Besu) returns 0

  10. Invalid block argument to getStorageAt

    • (Geth, Erigon, Nethermind) returns an Error
    • (Besu) returns null
  1. Unknown account to getProof
    • (Geth, Nethermind) returns an account object
    • (Besu) returns an Error
    • Note: Erigon does not support eth_getProof

  1. Wrong range to getPastLogs

    • (Geth, Besu) returns []
    • (Erigon, Nethermind) returns an Error

  2. Wrong range to getFeeHistory

    • (Geth, Erigon) returns { "oldestBlock": "0x0", "gasUsedRatio": null }
    • (Nethermind, Besu) returns an Error

  1. Invalid index to getTransactionFromBlock

    • (Geth, Erigon, Besu) returns null
    • (Nethermind) returns an Error

  2. Invalid index to getUncle

    • (Geth, Erigon, Besu) returns null
    • (Nethermind) returns an Error

I assume these discrepancies derived from the lack of error handling specification in this document. So, how about specifying the expected results (e.g., Error, null, 0, [ ], ...) so that different client implementations behave consistently in error cases as well?

Example test cases can be found in the following link: https://github.com/JosephK95/RPCTests

Thanks and please let me know if there is anything I can help you with.

lightclient commented 2 years ago

@JosephK95 thank you for these reports, this is very helpful. Just curious - are you intending to convert these into tests for this repository or did you just want to log these issues with us?

JosephK95 commented 2 years ago

Hi, @lightclient.

Thanks for your response. My initial purpose was to share the issues with you so that you can consider reflecting them into the specification.

I thought you could specify the expected return values in corner cases just like the Ethereum website did for some of the APIs: https://ethereum.org/ko/developers/docs/apis/json-rpc/

For example, the website specifies the following for the return value of eth_getTransactionByBlockNumberAndIndex:

Returns

Object - A transaction object, or null when no transaction was found:

While the decision making can be somewhat subjective, I personally think the justifications from Geth sound reasonable: https://github.com/ethereum/go-ethereum/issues/25495

And while reading your comment, I thought it would be honorable if our project can somehow contribute to the tests in your repository.

Considering that we have techniques to automatically generate valid chains and JavaScript test cases that trigger RPC requests, could you inform us which form of tests would fit your needs that we can contribute to?

Thanks.

lightclient commented 2 years ago

Could you please publish your examples.tar.gz to a repository so I can view without downloading and extracting a tar?

I am grateful for your contributions and would be very happy to have you contribute your tests to this repository. Unfortunately, it may be difficult to do so. Although the testing format is very simple (see the tests directory here, then you can view a test with the io extension), it relies heavily on the chain generation step.

Like you, I have written a tool (rpctestgen) to build a chain and execute the RPC calls. My hesitation is that I anticipate more changes to these files over time as we fill in the tests with more test cases. Because all of the tests are currently generated using a single chain.rlp and genesis.json file, I would need i) rerun your tool in the future (meaning I need to ensure it continues working in perpetuity) or ii) we would need to sort out a new layout for the tests so each test case defines which chain it will run against. ii) will probably require doing i) regardless though, because RPC methods will occasionally change and in those cases we'd need to regenerate tests anyways.

So, tldr; I'm apprehensive about accepting new tests from a generator that I don't maintain, because I will need to keep an eye on it going forward.

--

If you are interested in writing your tests in rpctestgen, I would be happy to provide guidance. But the feedback you have provided is already extremely useful.

JosephK95 commented 2 years ago

Hi, @lightclient.

Thanks for your detailed response. You can find the example test cases in the following repository: https://github.com/JosephK95/RPCTests

Could you give me some examples on how the changes of RPC methods can invalidate original test cases? (I am just asking to understand your points more clearly.)

Also, could you let us know if you have any plans to update your specification and standardize the expected values for error cases?

lightclient commented 2 years ago

Sorry for the late response @JosephK95, must've missed this.

Could you give me some examples on how the changes of RPC methods can invalidate original test cases?

So here, I was specifically referring to changing the underlying chain. For example, suppose we have a test for eth_getBalance. The genesis lists 0xaa with 1 wei. I generate the test fixtures for this repo and it statically lists the response as something like {"balance": "0x1"}, along with the genesis.json + chain.rlp. Now, if for some reason I need to test something else where the balance is different and I update the genesis to list 0xaa with 2 wei, I need to regenerate the test fixtures to account for the update. If we also have test fixtures generated using your tool, I would then now need to run your tool again. Essentially meaning someone would need to ensure it continues running.

The other situation we may need to regenerate tests is if the RPC method definition is changed in some way. For example, a new field "unit": "wei" is added to the above response. We could manually add those, but would much prefer they are generated automatically.

Also, could you let us know if you have any plans to update your specification and standardize the expected values for error cases?

We do intend to continue maintaining this specification. I don't have the bandwidth in the short term to work on standardizing expected values for error cases. I would gladly accept a PR of test cases so that individual client teams can see that their client deviates from the expected response.