Run London fork tests #2021

carver · 2021-08-31T03:05:05Z

What was wrong?

Fix #2017

How was it fixed?

Add circle job
Merge/Rebase on Preliminary london fixups #2022
Merge/Rebase on EIP-3529 Reduction in Refunds #2020
Merge/Rebase on EIP-3541 london test fixes #2029
Fix all broken tests
- Crash-fix: configure base fee on genesis header
- Bugfix: reset return data on contract collision CREATE / CREATE2
- Bugfix: properly validate 1/1024th gas limit deviation from parent as an exclusive limit
- Bugfix: validate uncles gas limit properly and account for the Berlin -> London transition (gas limit doubles) when validating

To-Do

Clean up commit history

Add entry to the release notes

Cute Animal Picture

carver · 2021-08-31T17:02:07Z

Okay, I think it makes sense to wait until #2020 is merged before trying to debug more.

carver · 2021-09-20T23:54:36Z

Just a few errors left. Some notes along the way:

It looks like uncles may not be getting validated correctly (I'm not convinced this is a London issue. I think we may have been incorrectly passing the test for earlier forks by raising a different validation error than expected).
Looks like several 1559 gas limit issues. The test name isn't quite enough to go on. Might be worth just asking in the testing channel for what the purposes of the failing tests are.

fselmo · 2021-09-29T20:17:37Z

It looks like uncles may not be getting validated correctly (I'm not convinced this is a London issue. I think we may have been incorrectly passing the test for earlier forks by raising a different validation error than expected).

Looks like you were right on the money with this one 👌... uncle gas limit wasn't being validated all the way (not London related though also required Berlin -> London transition validation) but also gas limit increase / decrease factor (1/1024) is an exclusive limit whereas it had been inclusive this whole time. The last few commits here should highlight these changes and all the current tests now pass.

I did remove the silenced London tests from some previous commits here in a separate PR to let circleci build it for me. Out of all 48 tests skipped, 9 fail.

However...

6 of those (all tests in this commit) have been previously skipped for other forks due to having a "synthetic state"? I wasn't around for that conversation, not entirely sure why we skip these other than we may not have a proper setup to accurately test them? It seems to me like we can keep this commit.
- GeneralStateTests/stRevertTest/RevertInCreateInInit (1 test)
- GeneralStateTests/stCreate2/RevertInCreateInInitCreate2 (1 test)
- GeneralStateTests/stSStoreTest/InitCollision (all variants - 4 tests)
3 were vm performance tests that timed out:
- VMTests/vmPerformance/loopMul (all variants - 3 tests)

@carver thoughts? Seems to me like we are good to go if we decide that the above assumptions are good: that synthetic state tests and vm performance timeout tests can be skipped for now.

relevant links for "synthetic state" explanation (from comments within test_blockchain.py skipped tests):

carver

Cool, looks good to me! I can't approve "my own" PR X)

Agree that it's okay to leave those other tests as skipped or xfail (as appropriate) for now.

I'll do a bit of tidying work and get it merged!

carver · 2021-09-30T16:33:44Z

eth/vm/logic/system.py

+                self.logger.debug2(
+                    "%s failure: %s",
+                    self.mnemonic,
+                    f"Insufficient Funds: {storage_address_balance} < {stack_data.endowment}"
+                )
            elif stack_too_deep:
-                err_msg = "Stack limit reached"
-            self.logger.debug2("%s failure: %s", self.mnemonic, err_msg,)
+                self.logger.debug2("%s failure: %s", self.mnemonic, "Stack limit reached")


There was an implicit invariant check in the previous code that's not here anymore (because if neither of the if checks were true, the logger would fail on a missing err_msg. Maybe add an else: raise RuntimeError("unreachable")?

carver · 2021-09-30T16:36:09Z

tests/json-fixtures/blockchain/test_blockchain.py

@@ -369,6 +369,11 @@ def assert_imported_genesis_header_unchanged(genesis_fields, genesis_header):
            )


+EXPECTED_BAD_BLOCK_EXCEPTIONS = (
+    TypeError, rlp.DecodingError, rlp.DeserializationError, ValidationError, AssertionError


nit: trailing commas are nice

carver · 2021-09-30T17:15:19Z

eth/vm/base.py

+        uncle_parent_gas_limit = uncle_parent.gas_limit
+        if not hasattr(uncle_parent, 'base_fee_per_gas') and hasattr(uncle, 'base_fee_per_gas'):
+            # if Berlin -> London transition, double the parent limit for validation
+            uncle_parent_gas_limit *= 2
+
+        validate_gas_limit(uncle.gas_limit, uncle_parent_gas_limit)


Oof, this is a bummer, but I don't have a better suggestion right now.

Hah, yes, same... I think it can use a refactor at some point for sure.

carver · 2021-09-30T17:16:27Z

docs/guides/understanding_the_mining_process.rst

@@ -380,4 +380,4 @@ zero value transfer transaction.
  ... )

  >>> chain.mine_block(mix_hash=mix_hash, nonce=nonce)
-  <ByzantiumBlock(#Block #1-0x41f6..2913)>


Why did this change? Something about the gas limit being off by one?

@carver yep

Exploit tests seem to have moved in a way that caused our previous skipping to fail. We only skipped tests that started with bcExploitTest, now we skip them if that is the name of a folder anywhere in the test path.

For example: InitCollision_d0g0v0_London

- We have an 'expectException' flag we can reference for validating should_be_good_block for blockchain tests. These changes tweak blockchain test validation to use this flag since some block fixtures for tests have both good and bad blocks in them now. - If the gas limit is AT 1/1024th of the parent gas limit, this is also unacceptable. These changes add equal-to signs when comparing the gas limit to the parent. This passes currently-failing ethereum tests >= London.

The gas_limit was improperly allowed to be 1/1024th of the parent gas limit. However, this is an exclusive limit that the new gas limit must be inside of, therefore requiring +1 or -1 from the bound as the inclusive, allowable values.

carver · 2021-09-30T17:48:39Z

I snuck in a little renaming of the objects in the logs to help identify which block is built locally and which is proposed by the test. I also skipped a few more slow tests that were causing the London job to be significantly slower than the others.

carver force-pushed the london-tests branch from ebfc9c0 to 4dd3f7c Compare August 31, 2021 16:59

carver mentioned this pull request Sep 1, 2021

Preliminary london fixups #2022

Merged

6 tasks

carver force-pushed the london-tests branch from 4dd3f7c to 3d60236 Compare September 3, 2021 15:32

carver mentioned this pull request Sep 3, 2021

fix: make blake2b optional #2012

Merged

2 tasks

fubuloubu mentioned this pull request Sep 5, 2021

blocker: Ubuntu 20.04LTS Raspberry Pi pip install eth-ape fails at requirement blake2b-py with ERROR: Could not find a version ApeWorX/ape#101

Open

carver force-pushed the london-tests branch 3 times, most recently from 6e23997 to e18b170 Compare September 16, 2021 22:10

fselmo mentioned this pull request Sep 17, 2021

EIP-3541 london test fixes #2029

Merged

2 tasks

Run London fork tests

7733bf5

carver force-pushed the london-tests branch from e18b170 to 640dc6b Compare September 20, 2021 17:37

fselmo force-pushed the london-tests branch 4 times, most recently from 27f7c3a to 9ed3c84 Compare September 29, 2021 17:06

fselmo marked this pull request as ready for review September 29, 2021 20:50

carver commented Sep 30, 2021

View reviewed changes

carver added a commit to carver/py-evm that referenced this pull request Sep 30, 2021

Add release notes for PR ethereum#2021, issue ethereum#2017

d993114

carver force-pushed the london-tests branch from 9ed3c84 to d993114 Compare September 30, 2021 17:32

carver and others added 8 commits September 30, 2021 10:46

Skip some slow tests

a839a1a

Exploit tests seem to have moved in a way that caused our previous skipping to fail. We only skipped tests that started with bcExploitTest, now we skip them if that is the name of a folder anywhere in the test path.

Skip non-consensus-relevant London tests that fail

4420efd

For example: InitCollision_d0g0v0_London

reset return data on contract collision

fddce69

validate uncles appropriately across berlin -> london transition

6b14d65

doc updates due to sphinx update errors

04e13b8

Improve readability of fixture block import fails

0e74b17

Add release notes for PR ethereum#2021, issue ethereum#2017

92095eb

carver force-pushed the london-tests branch from d993114 to 92095eb Compare September 30, 2021 17:47

carver merged commit cb6f44c into ethereum:master Sep 30, 2021

carver deleted the london-tests branch September 30, 2021 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run London fork tests #2021

Run London fork tests #2021

carver commented Aug 31, 2021 •

edited

Loading

carver commented Aug 31, 2021

carver commented Sep 20, 2021

fselmo commented Sep 29, 2021 •

edited

Loading

carver left a comment

carver Sep 30, 2021

carver Sep 30, 2021

carver Sep 30, 2021

fselmo Sep 30, 2021

carver Sep 30, 2021

fselmo Sep 30, 2021

carver commented Sep 30, 2021

Run London fork tests #2021

Run London fork tests #2021

Conversation

carver commented Aug 31, 2021 • edited Loading

What was wrong?

How was it fixed?

To-Do

Cute Animal Picture

carver commented Aug 31, 2021

carver commented Sep 20, 2021

fselmo commented Sep 29, 2021 • edited Loading

carver left a comment

Choose a reason for hiding this comment

carver Sep 30, 2021

Choose a reason for hiding this comment

carver Sep 30, 2021

Choose a reason for hiding this comment

carver Sep 30, 2021

Choose a reason for hiding this comment

fselmo Sep 30, 2021

Choose a reason for hiding this comment

carver Sep 30, 2021

Choose a reason for hiding this comment

fselmo Sep 30, 2021

Choose a reason for hiding this comment

carver commented Sep 30, 2021

carver commented Aug 31, 2021 •

edited

Loading

fselmo commented Sep 29, 2021 •

edited

Loading