Skip to content

Comments

simulators/ethereum/engine: fix Reorg using Sync tests#560

Merged
fjl merged 17 commits intoethereum:masterfrom
MariusVanDerWijden:ReorgSync
Jul 14, 2022
Merged

simulators/ethereum/engine: fix Reorg using Sync tests#560
fjl merged 17 commits intoethereum:masterfrom
MariusVanDerWijden:ReorgSync

Conversation

@MariusVanDerWijden
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden commented Jun 1, 2022 •
edited
Loading

The tests used to start two node n1 and n2, and create a chain p1...p10 with p9 (usually) being the bad block. p9 and p10 were fed to node n2, while p1...p8 were fed to n1. The test then sends forkChoiceUpdates to block p10 to n2 and expects {INVALID, LVH:p8}. However this requires n2 to cache the un-validated payloads p9 and p10 and execute them later on. This caching is not required by the spec though.

The new tests use a slightly hacked version of geth as n1 which allows for importing invalid payloads, so we feed p1...p9 to n1 and then send forkChoiceUpdates to n2 for block p10. n2 should sync from n1, detect the invalid chain and return {INVALID, LVH:p8}.

I also refactored the Reorg using Sync and Reorg using NewPayload out for better readability.

The only test that still fails is "Invalid Ancestor Chain Re-Org, Invalid Number, Invalid P9', Reveal using sync (go-ethereum)". Since the header chain is not continuous, because the number is invalid, we don't have the parent saved in our database therefore we will panic. I might just remove this test for cleanliness as it doesn't really test that much

@MariusVanDerWijden MariusVanDerWijden marked this pull request as ready for review July 5, 2022 08:11
@MariusVanDerWijden MariusVanDerWijden changed the title [WIP] Reorg sync simulators/ethereum/engine: fix Reorg using Sync tests Jul 5, 2022
marioevz
marioevz reviewed Jul 8, 2022
View reviewed changes
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.

Looks great overall, I think this is more in line with what would happen in real case scenario.
I ran the tests and only Erigon seems to pass these tests. Is a change in geth necessary for them to pass ?

simulators/ethereum/engine/enginetests.go Outdated
{
Name: "Invalid Ancestor Chain Re-Org, Invalid GasUsed, Invalid P9', Reveal using sync",
TimeoutSeconds: 15,
Name: "Invalid Ancestor Chain Re-Org, Invalid GasUsed, Invalid P8', Reveal using sync",
Copy link
Member

@marioevz marioevz Jul 8, 2022

Choose a reason for hiding this comment

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

Why are some P8' instead of P9'?

Copy link
Member Author

@MariusVanDerWijden MariusVanDerWijden Jul 14, 2022

Choose a reason for hiding this comment

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

Because I am unable to mine invalid blocks on top of an invalid block on a node. The next block is send via NewPayload. See also description

@MariusVanDerWijden
simulators/ethereum/engine: separate sync and new payload tests
689cf39
@MariusVanDerWijden
simulators/ethereum/engine: use local node
bdd6caf
@MariusVanDerWijden
simulators/ethereum/engine: use local node
9044649
@MariusVanDerWijden
simulators/ethereum/engine: more fixes
e2d02a6
@MariusVanDerWijden
more stuff
cfb26cc
@MariusVanDerWijden
simulators/ethereum/engine: newest updates
ca49229
@MariusVanDerWijden
simulators/ethereum/engine: newest updates
1ef335b
@MariusVanDerWijden
simulators/ethereum/engine: fix prevrandao test
87bb323
@MariusVanDerWijden
simulators/ethereum/engine: fix emptyTx fields
2a790c4
@MariusVanDerWijden
simulators/ethereum/engine: write header number
fe02641
@MariusVanDerWijden
simulators/ethereum/engine: reduce diff
6daefcc
@MariusVanDerWijden
simulators/ethereum/engine: reduce diff, use correct master
d8e3d7e
@MariusVanDerWijden
simulators/ethereum/engine: reduce diff
624f8d2
@MariusVanDerWijden
simulators/ethereum/engine: reduce diff
78e4e8f
@MariusVanDerWijden
simulators/ethereum/engine: fix missing trie node errors
9da903e
@MariusVanDerWijden
simulators/ethereum/engine: allow for invalid transactions in tests
5e57816
@MariusVanDerWijden
Copy link
Member Author

MariusVanDerWijden commented Jul 14, 2022

Yes geth needs another PR in order to pass these tests

@marioevz
Copy link
Member

marioevz commented Jul 14, 2022

New failing tests totals:

Attached are the full log files for each failing test on this PR for every client.

@fjl
Copy link
Collaborator

fjl commented Jul 14, 2022

Should we just merge this?

@marioevz
Copy link
Member

marioevz commented Jul 14, 2022

Should we just merge this?

I think so, these new issues look like genuine issues in some of the clients, I just wanted to share the logs with the clients' devs so they get a sense of what is being changed.

@fjl fjl merged commit cf4fbf9 into ethereum:master Jul 14, 2022
racytech pushed a commit to racytech/hive that referenced this pull request Apr 4, 2025
@h4ck3rk3y
fix: fix doc string typo (ethereum#560)
13de3f6
racytech pushed a commit to racytech/hive that referenced this pull request Apr 4, 2025
@github-actions
chore(main): release 2.2.0 (ethereum#530)
f7887aa 
🤖 I have created a release *beep* *boop*
---


##
[2.2.0](ethpandaops/ethereum-package@2.1.0...2.2.0)
(2024-04-19)


### Features

* add assertoor test for per PR CI job
([ethereum#537](ethpandaops/ethereum-package#537))
([8ef5c57](ethpandaops/ethereum-package@8ef5c57))
* add blutgang rpc load balancer
([ethereum#569](ethpandaops/ethereum-package#569))
([1be5f95](ethpandaops/ethereum-package@1be5f95))
* add dugtrio beacon load balancer
([ethereum#568](ethpandaops/ethereum-package#568))
([56d2fa3](ethpandaops/ethereum-package@56d2fa3))
* add new assertoor test to per ci jobs
([ethereum#545](ethpandaops/ethereum-package#545))
([3005d46](ethpandaops/ethereum-package@3005d46))
* use new rpc snooper from `ethpandaops/rpc-snooper`
([ethereum#567](ethpandaops/ethereum-package#567))
([5676f0d](ethpandaops/ethereum-package@5676f0d))


### Bug Fixes

* add --contract-deployment-block parameter for Prysm
([ethereum#557](ethpandaops/ethereum-package#557))
([d8dfbae](ethpandaops/ethereum-package@d8dfbae))
* Added '--enable-private-discovery' to Grandine
([ethereum#541](ethpandaops/ethereum-package#541))
([a1ae708](ethpandaops/ethereum-package@a1ae708))
* beaconchain explorer
([ethereum#531](ethpandaops/ethereum-package#531))
([b62ed6f](ethpandaops/ethereum-package@b62ed6f))
* beaconchain explorer
([ethereum#538](ethpandaops/ethereum-package#538))
([ce1f337](ethpandaops/ethereum-package@ce1f337))
* blobber incorrect url
([ethereum#528](ethpandaops/ethereum-package#528))
([6f84e3d](ethpandaops/ethereum-package@6f84e3d))
* bump json rpc snooper
([ethereum#553](ethpandaops/ethereum-package#553))
([f69c4a7](ethpandaops/ethereum-package@f69c4a7))
* disable full sync if gcmode is archive
([ethereum#563](ethpandaops/ethereum-package#563))
([b7592ec](ethpandaops/ethereum-package@b7592ec))
* disable pbss when gcmode archive set
([ethereum#559](ethpandaops/ethereum-package#559))
([e085462](ethpandaops/ethereum-package@e085462))
* disable pbss when gcmode archive set, force hash based init
([ethereum#562](ethpandaops/ethereum-package#562))
([3e1c7a6](ethpandaops/ethereum-package@3e1c7a6))
* disable static peers
([ethereum#529](ethpandaops/ethereum-package#529))
([c5d4028](ethpandaops/ethereum-package@c5d4028))
* enable single node mode on lodestar by default
([ethereum#558](ethpandaops/ethereum-package#558))
([555ad7d](ethpandaops/ethereum-package@555ad7d))
* fix doc string typo
([ethereum#560](ethpandaops/ethereum-package#560))
([13de3f6](ethpandaops/ethereum-package@13de3f6))
* fix failing persistence test
([ethereum#554](ethpandaops/ethereum-package#554))
([99242d6](ethpandaops/ethereum-package@99242d6))
* increase mem limit of snooper
([ethereum#546](ethpandaops/ethereum-package#546))
([6ba5770](ethpandaops/ethereum-package@6ba5770))
* prysm beacon http url
([ethereum#536](ethpandaops/ethereum-package#536))
([4914531](ethpandaops/ethereum-package@4914531))
* prysm beacon_http_url
([ethereum#535](ethpandaops/ethereum-package#535))
([ee7528c](ethpandaops/ethereum-package@ee7528c))
* prysm vc
([ethereum#533](ethpandaops/ethereum-package#533))
([72ddeb2](ethpandaops/ethereum-package@72ddeb2))
* remove un-needed prysm vc check
([ethereum#542](ethpandaops/ethereum-package#542))
([f6326fe](ethpandaops/ethereum-package@f6326fe))
* set application protocol to be http for rpc
([ethereum#548](ethpandaops/ethereum-package#548))
([905de7c](ethpandaops/ethereum-package@905de7c))
* set the correct default vc image
([ethereum#544](ethpandaops/ethereum-package#544))
([953741d](ethpandaops/ethereum-package@953741d))
* uniformize keymanager
([ethereum#534](ethpandaops/ethereum-package#534))
([a6a2830](ethpandaops/ethereum-package@a6a2830))
* update prometheus api
([ethereum#539](ethpandaops/ethereum-package#539))
([d2b9fb8](ethpandaops/ethereum-package@d2b9fb8))
* update vc <> cl matrix
([ethereum#564](ethpandaops/ethereum-package#564))
([0ffcf74](ethpandaops/ethereum-package@0ffcf74))
* update vc compatibility matrix
([ethereum#543](ethpandaops/ethereum-package#543))
([58c4684](ethpandaops/ethereum-package@58c4684))
* use `minimal-preset` images for dora & assertoor when minimal preset
is used
([ethereum#532](ethpandaops/ethereum-package#532))
([ad7773e](ethpandaops/ethereum-package@ad7773e))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marioevz marioevz marioevz left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MariusVanDerWijden @marioevz @fjl