clients: add ethereumjs

[cbrzn]

This PR aims to add EthereumJS as a client so it can be tested against the hive. Currently, the objective is to make sure that we are good to go with the Engine API. Here is a track of what we have green so far https://hackmd.io/lKw1UArUSWu1_lOdbt3heQ?view

TODO:

• Remove readme in merge-ethereumjs folder
• Generate version.txt file on script

[fjl]

Can ethereumjs also run other tests or is this only about the engine API?

[cbrzn]

@fjl this is only about engine API - but once we are ready with it we plan to also run other tests!

[cbrzn]

Hey @fjl this is ready for review, currently, we are passing 29 of 43 tests (You can check the hackmd link in the description of the PR) and will focus on fixing the red ones. Since I don't think the initialization script will change, I would say this is good to merge

Note: Basically I forked the original hackmd (https://notes.ethereum.org/xnKiAcNlT2aZ66DdH8LM6g?view) not sure if I should modify this one once this is merged? Lmk what do you think it's the best path, and thanks for your time 🙏

[holgerd77]

Anyone able to give this a review/merge? 🙂 (we are excitedly waiting to see this in action! 😋)

[fjl]

I'm OK with merging this PR optimistically (it can't break anything), but let's call rename the client to simply ethereumjs.

[holgerd77]

I'm OK with merging this PR optimistically (it can't break anything), but let's call rename the client to simply ethereumjs.

Cool! 😄 👍

Not sure if @cbrzn knows what to do, but for me it is not directly obvious: so what kind of renaming are you referring to? So the clients in the clients directly have the merge related tests also placed in merge-* folders - e.g. merge-nethermind - so I guess you do not refer to renaming that (or do you)?

Is there another client naming reference in the code base (couldn't find any)?

[cbrzn]

I'm OK with merging this PR optimistically (it can't break anything), but let's call rename the client to simply ethereumjs.

Cool! smile +1

Not sure if @cbrzn knows what to do, but for me it is not directly obvious: so what kind of renaming are you referring to? So the clients in the clients directly have the merge related tests also placed in merge-* folders - e.g. merge-nethermind - so I guess you do not refer to renaming that (or do you)?

Is there another client naming reference in the code base (couldn't find any)?

Hey @holgerd77 I missed previous comment from flj, I think what he means is to the merge-ethereumjs folder. Geth and Nethermind has the merge-* prefix because (this is just my guess) they probably use different parameters in the initialization to work with the Engine API, but currently for geth there's a PR to just have the go-ethereum folder.

I will do this later today :)

[holgerd77]

@cbrzn ah ok, thanks, then let's do this pre-actively. 😋 Thanks for tackling directly.

[fjl]

@holgerd77 In hive, there is a strict separation between clients and tests. The client definitions should ideally only exist once, and clients can implement various interfaces (e.g. eth1 client, beacon client, validator...).

The merge-* client definitions were added temporarily in order to prototype the merge tests. We will back-port the merge features into the main client definitions very soon.

[cbrzn]

We should be good to go now! Let me know if something else is needed 🙏 @fjl

[holgerd77]

🎉

Very cool! 👍 😄

Am I correct that there should now be some results showing up on https://hivetests.ethdevops.io/ from our client, or is there some (any) additional in-between steps yet to be taken?

[fjl]

There are additional steps required to run it. We will include it in future runs when we will deploy the test runner next time.

[holgerd77]

There are additional steps required to run it. We will include it in future runs when we will deploy the test runner next time.

Ah ok, thanks, good to know! 👍

(sorry, guess I am close to having all basic questions answered 😋 😬): once you deploy will this then directly run the whole test suite (so also the non-merge tests) already?

[fjl]

So, the way the public test instance works is, it just runs a shell script like

while true;
do

./hive --sim 'ethereum/engine' --client go-ethereum
./hive --sim 'ethereum/engine' --client merge-nethermind
./hive --sim 'devp2p' --client go-ethereum
./hive --sim 'devp2p' --client besu
./hive --sim 'devp2p' --client nethermind
...

done

We can add a test run with ethereumjs for the engine simulator.

[holgerd77]

There are additional steps required to run it. We will include it in future runs when we will deploy the test runner next time.

Any update on this?

[holiman]

@holgerd77 which tests should we execute? I'm fine with executing all, but if the consensus tests aren't supported, we can shave off ~24 hours of execution by omitting specifically that test.

[fjl]

ethereumjs does not support most tests because it is not a full eth1 client. AFAIK the client definition is only capable of running the ethereum/engine tests.

[holiman]

I added it on several tests for now, can always disable it later

[holgerd77]

ethereumjs does not support most tests because it is not a full eth1 client

Ah no, we are relatively close on being a "full" eth1 client - if the definition is to expose most of the JSON RPC API and being able to sync and execute blocks. We do not have some more fancy sync mechanisms yet (snap sync), but I guess that is not tested here anyhow?

So you can activate for the whole (most of the) test suite I guess.

[holgerd77]

I added it on several tests for now, can always disable it later

Thanks! 👍 🙂 🙏

[holiman]

Sgtm!

[holgerd77]

Sgtm!

? 🙂

This PR was merged months ago, was this a reaction to any new events or similar or does this trigger anything new?

Anyhow: all the best 👋, just a bit puzzled. 😋

[holiman]

just a bit puzzled. yum

So am I. I've seen this happen several times -- emails written weeks/months ago are popping in on github these last couple of days. Must have gotten stuck internally at google.

Another example: ethereum/go-ethereum#25822 (comment)