nimbus-eth2 icon indicating copy to clipboard operation
nimbus-eth2 copied to clipboard

Harden the web3's json parser against objects with missing or `null` fields

Open zah opened this issue 1 year ago • 5 comments

There is a problem where a crash occurs in the web3 JSON parser.

The idea is that it could have something to do with nully fields. However this is not 100% certain yet.

Starting by writing a few unit tests to try and reproduce the crash, and then fixing the crash.

A second PR could be to fuzz different areas of the downloaded JSON blob and fuzzing to find similar errors.

This problem appears with deposit blocks.

zah avatar Aug 24 '22 10:08 zah

JSON RPC serialisation (Geth) Rest API ETH2

Hex has sometimes a leading 0x Generic converters Exact correct string for every type

tavurth avatar Sep 14 '22 11:09 tavurth

The relevant standard is here: https://github.com/ethereum/execution-apis/ (this is what we should produce) - practically speaking, clients may produce slightly different jsons, and we should have tests covering that we can parse whatever the current parsers accept (and not crash on various nulls of course)

The eth2 rest api is orthogonal and not related to this repo or issue

arnetheduck avatar Sep 14 '22 12:09 arnetheduck

@arnetheduck You mentioned about the different client responses (JSON responses) however I see mostly that there is only one response example in each tests folder. 🤔

As I read from the tooling docs this is mostly used by ETH to ensure that everything is black-box and clients can interchange whatever validator they like without issues.

From this I assume that we should just cover the entirety of this folder of Json's as tests, but this doesn't all seem entirely applicable to Web3 or am I wrong there?

Perhaps you have a bit more information about what the additional tests have to cover exactly?

Edit Ok I went over this with Zah and I will cover most of these tests in the repo you posted as well as implementing some fuzzy extra tests if I have time.

tavurth avatar Sep 19 '22 09:09 tavurth

Ok I went over this with Zah and I will cover most of these tests in the repo you posted

Indeed, the API test suite is an excellent start - in addition to that, we need a place where fields and types get coverage, something like https://github.com/status-im/nim-web3/blob/master/tests/test_quantity.nim, but extended to the other types in the library (hashes, nonces and so on) - the quantity tests represent an attempt to start such a test suite - this is a good place to put strange cases (such as the handling of out-of-range integers). It's good to have these in place before attempting fuzzing - also because when fuzzing finds an issue, we ideally want to create a non-fuzzing test case that covers that failure (so that we don't have to run fuzzing to discover "simple" regressions).

Another problem in nim-web3 is the usage of compile time constants like gnosisChainBinary - compile time constants are of course not a sustainable solution, and this will need to be addressed somehow - the gnosis chain used a different consensus algorithm (aura from parity) and therefore has a slightly different block structure - when we read blocks from gnosis, some of the fields are not present and we need a way to handle that gracefully so that nim-web3 can be used to read such chains side-by-side with "normal" ethereum chains (for example if you're building a bridge).

There's an extended discussion here: https://github.com/status-im/nimbus-eth2/pull/3415 - alternatives include a full new object type for blocks and duplication of all calls, the creative usage of optionals or using defaults (empty values).

arnetheduck avatar Sep 21 '22 07:09 arnetheduck

Finally, the error messages from the library are completely useless - when you make a request for data that is not present or the request is malformed, nim-web3 breaks down completely because of how it enforces the json-to-object mapping and isn't capable of digesting the various shapes of errors that different data sources put out - https://github.com/status-im/nimbus-eth2/issues/4060 is an example and there are plenty more.

arnetheduck avatar Sep 21 '22 07:09 arnetheduck