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

simulators/eth2/engine: Implement EL mock to test CLs #540

Merged
merged 4 commits into from
Jun 14, 2022

Conversation

MariusVanDerWijden
Copy link
Member

This PR implements a new class of simulators used to test CL implementations. We start a proxy between EL and CL which allows us to modify requests and responses to and from the EL.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing! I only have one suggestion on the changes which I think would help us greatly to design the test cases more deterministically.

return fmt.Sprintf("http://%v:%d", p.IP, p.port), nil
}

func (p *Proxy) AddRequest(spoof *proxy.Spoof) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be great to have the option to add a callback, something like func (p *Proxy) AddRequestCallback(callback func(interface{}) *proxy.Spoof) , and the callback function receives the request and you can respond on a request-by-request basis whether to spoof or not.

I think at the moment AddRequests spoofs every single request it receives from the CL which is ok, but being able to decide on which requests to spoof would let us fine-tune the test cases. E.g. a test case where only the transition payload is corrupted, but every payload after that is ok.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I thought about this too, but unfortunately raul's proxy does not support that. I can try to implement it in the proxy so we can use it in hive eventually

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a PR to raul's repo to add this functionality: rauljordan/engine-proxy#4
I haven't tested the changes but will do tomorrow.

var engineAddrs []string
for _, index := range eth1Endpoints {
eth1Node := testnet.proxies[index]
userRPC, err := eth1Node.UserRPCAddress()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had trouble using the UserRPCAddress of the proxy, I kept getting "the method net_version does not exist/is not available" errors.
I used testnet.eth1[index].UserRPCAddress for the userRPC and testnet.proxies[index] for engineRPC.

@marioevz
Copy link
Member

This looks good to me.

I tested the callbacks from my PR in the proxy and it's working ok.

There are a couple of changes required for the callbacks to work in this simulator, but we have to wait for my PR to be merged in Raul's engine-proxy.

I can do these required changes in a later PR, or we can wait and incorporate them here, however you decide.

Copy link
Collaborator

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine for me to merge this, if you think it's ready?

@MariusVanDerWijden
Copy link
Member Author

I think it's okay

@MariusVanDerWijden MariusVanDerWijden merged commit ecc01fe into ethereum:master Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants