-
Notifications
You must be signed in to change notification settings - Fork 20.3k
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
tests: fix flaw in state test runner #26299
Conversation
The random being set in t8n env params on <=berlin? So its a t8n bug only? In the tests if current Difficulty is set and current random is set together. Current random will be passed to t8n only on >=Merge, current Difficulty is passed for test generated <Merge. If no difficulty is set it comes as default 0x020000 on pre Merge. If no current random is set, the current Difficulty is used as Random on Merge. This when formibg evm t8n env file |
This is not
Since it's a general state tests, it may contain multiple post-sections, but in this case there was only one, for |
I just tried updating the tests, btw, and the bug that this PR fixes also triggers on cases like https://github.com/ethereum/tests/blob/develop/GeneralStateTests/stRandom/randomStatetest153.json#L13 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, I think I remember when I wrote this I thought that random should be applied whenever set, but it makes more sense to only set it post-london. It kinda means that tests without random for london are not really possible though (when random is defined)
This (alone) causes test failures, I think it's better to do in tandem with a test-update, as in #26314 |
This PR builds on #26299, but also updates the tests to the most recent version, which includes tests regarding TheMerge. This change adds checks to the beacon consensus engine, making it more strict in validating the pre- and post-headers, and not relying on the caller to have already correctly sanitized the headers/blocks.
This PR builds on ethereum#26299, but also updates the tests to the most recent version, which includes tests regarding TheMerge. This change adds checks to the beacon consensus engine, making it more strict in validating the pre- and post-headers, and not relying on the caller to have already correctly sanitized the headers/blocks.
This PR fixes a flaw uncovered by @guidovranken in a bounty-report. Essentially, even on berlin rules, geth reacts to the
RANDOM
being set, and clears the DIFFICULTY, resulting in theDIFFICULTY
op returning zero later on.Geth trace, where geth spits out
0x0
for difficulty:I modified the statetest from the report a bit, so that it has
currentDifficulty
at0x020001
andcurrentRandom
set to..2002
, to disambiguate the two:With this PR, geth now correctly spits out
0x20001
As for besu and nethermind, nethermind erroneously spits out
0x20002
But besu correctly spits out
0x20001
:cc @MarekM25 @LukaszRozmej
This test does only
It caused a differing state root because
REVERT(0x614de8ee56f00cbc4139, 0)
succeeds (size zero) whereas a non-zero size fails. However, the stateroot does not change if0x20002
is used instead of0x20001
. So it's not a "great" test to include as is in the reference tests, but it should be fairly simple to tweak it and include it. cc @winsvega