Run Consensus Tests on MuirGlacier#648
Conversation
…ing back to the Istanbul tests (no official MuirGlacier tests available)
… consolidate error prone fork alias lowercase modifications
Codecov Report
@@ Coverage Diff @@
## master #648 +/- ##
======================================
Coverage 91.4% 91.4%
======================================
Files 31 31
Lines 1978 1978
Branches 326 326
======================================
Hits 1808 1808
Misses 90 90
Partials 80 80
Continue to review full report at Codecov.
|
|
Update: State tests really take too much time now which is impractical. I've now added a new |
evertonfraga
left a comment
evertonfraga
left a comment
There was a problem hiding this comment.
I left two comments that might lead to changes. But i'm approving — these tests really take a long time to run! 👀
| exports.getRequiredForkConfigAlias = function (forkConfig) { | ||
| // Run the Istanbul tests for MuirGlacier since there are no dedicated tests | ||
| if (String(forkConfig).match(/^muirGlacier/i)) { | ||
| return 'Istanbul' |
There was a problem hiding this comment.
This is the core of the PR. LGTM 👍
| testGetterArgs.runSkipped = getSkipTests(argv.runSkipped, 'NONE') | ||
| testGetterArgs.skipVM = skipVM | ||
| testGetterArgs.forkConfig = getRequiredForkConfigAlias(FORK_CONFIG) | ||
| testGetterArgs.forkConfig = FORK_CONFIG_TEST_SUITE |
| // Tests for HFs before Istanbul have been moved under `LegacyTests/Constantinople`: | ||
| // https://github.com/ethereum/tests/releases/tag/v7.0.0-beta.1 | ||
| if (runnerArgs.forkConfig !== 'Istanbul') { | ||
| if (testGetterArgs.forkConfig !== 'Istanbul') { |
There was a problem hiding this comment.
We should safeguard this conditional with lteHardfork() instead, so Berlin HF tests aren't searched for in LegacyTests?
There was a problem hiding this comment.
Yes, I thought about that as well, just left it out for timing reasons.
|
@evertonfraga @s1na If one of you can prepare a short release PR on this in the next 1-2 hours, that would also be nice, won't make it in the next 4-5 hours. But also just if it fits in for you. |
3 participants
The HF comparison error fixed in #647 by @s1na was one of the most severe bugs in recent times, it actually makes the whole MuirGlacier pretty much unusable (this needs a bug fix release ASAP).
I am actually still amazed that this could slip through, this might need some further postmortem analysis. One major reason is actually the lack of consensus tests for the MuirGlacier HF.
This PR allows the VM to be tested with the MuirGlacier HF setting by falling back to the Istanbul tests. I also did some cleanup of the fork configuration in the test runner so to have a better distinction between the fork names/aliases used in the VM and the test runner.
I tested this with the pre #647 state of the VM (so what was actually released as the MuirGlacier release). Most of the consensus tests fail here.