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

feat(tests): EIP-7691: Fork transition tests #1082

Merged
merged 6 commits into from
Jan 22, 2025
Merged

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Jan 16, 2025

🗒️ Description

Implements fork transition tests for the blob target and max increment by EIP-7691 that occurs on the Prague activation.

Test expectation: At the first block of Prague, we use theTARGET_BLOB_GAS_PER_BLOCK,MAX_BLOB_GAS_PER_BLOCK and BLOB_BASE_FEE_UPDATE_FRACTION_PRAGUE constants in EIP-7691, just the same as we use for subsequent blocks for Prague:

tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_at_blob_genesis[fork_ShanghaiToCancunAtTime15k-max_blobs-blockchain_test]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_at_blob_genesis[fork_ShanghaiToCancunAtTime15k-max_blobs-blockchain_test_engine]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_at_blob_genesis[fork_ShanghaiToCancunAtTime15k-no_blobs-blockchain_test]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_at_blob_genesis[fork_ShanghaiToCancunAtTime15k-no_blobs-blockchain_test_engine]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_at_blob_genesis[fork_ShanghaiToCancunAtTime15k-target_blobs-blockchain_test]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_at_blob_genesis[fork_ShanghaiToCancunAtTime15k-target_blobs-blockchain_test_engine]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_post_blob_genesis[fork_CancunToPragueAtTime15k-max_blobs-blockchain_test]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_post_blob_genesis[fork_CancunToPragueAtTime15k-max_blobs-blockchain_test_engine]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_post_blob_genesis[fork_CancunToPragueAtTime15k-no_blobs_before-blockchain_test]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_post_blob_genesis[fork_CancunToPragueAtTime15k-no_blobs_before-blockchain_test_engine]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_post_blob_genesis[fork_CancunToPragueAtTime15k-no_blobs_after-blockchain_test]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_post_blob_genesis[fork_CancunToPragueAtTime15k-no_blobs_after-blockchain_test_engine]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_post_blob_genesis[fork_CancunToPragueAtTime15k-target_blobs-blockchain_test]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_post_blob_genesis[fork_CancunToPragueAtTime15k-target_blobs-blockchain_test_engine]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_post_blob_genesis[fork_CancunToPragueAtTime15k-single_blob_to_max_blobs-blockchain_test]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_post_blob_genesis[fork_CancunToPragueAtTime15k-single_blob_to_max_blobs-blockchain_test_engine]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_post_blob_genesis[fork_CancunToPragueAtTime15k-max_blobs_to_single_blob-blockchain_test]
tests/cancun/eip4844_blobs/test_excess_blob_gas_fork_transition.py::test_fork_transition_excess_blob_gas_post_blob_genesis[fork_CancunToPragueAtTime15k-max_blobs_to_single_blob-blockchain_test_engi

(output generated with fill --until Prague -k test_fork_transition_excess_blob_gas --collect-only -q)

Requires #1081 Merged

🔗 Related Issues

None

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.

@marioevz marioevz force-pushed the fork-transition-eip-7691 branch from c0c27f3 to 791a6ae Compare January 22, 2025 17:37
@danceratopz danceratopz added scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature labels Jan 22, 2025
@marioevz
Copy link
Member Author

marioevz commented Jan 22, 2025

Documenting here the drop in coverage when comparing the generated fixtures by this PR against main:

image

The culprit is that we were originally sending the blob transactions to a hard-coded empty address at Address(0x100) here:

@pytest.fixture
def destination_account() -> Address: # noqa: D103
return Address(0x100)

And was changed in this PR to a new dynamically generated contract with a single STOP opcode here:

@pytest.fixture
def destination_account(pre: Alloc) -> Address: # noqa: D103
return pre.deploy_contract(Op.STOP)

Since this test has nothing to do with empty accounts, I think this should be ok.

Coverage check doing its job very nicely here because if it was something else that was a bit more serious, it would have been caught here 💯

Copy link
Member

@danceratopz danceratopz 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!

These tests all pass consume direct with s1na/go-ethereum@f24d6f6

Not particular to the changes here (the tests were already so): The tests include type2 tx before the transition and then type3 tx after (no type2). I wondered if we should include type2 after transition as well, in addtion to the new type3 txs?

One other comment below.

* feat(tests): more edge cases for blob gas at transitions

* Apply suggestions from code review

---------

Co-authored-by: Mario Vega <[email protected]>
Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

LGTM!

@marioevz
Copy link
Member Author

Lost of coverage in the last commit is due to removal of Type-2 transactions, which are not the purpose of this test:
image

@marioevz marioevz merged commit 5e6a38e into main Jan 22, 2025
21 of 22 checks passed
@marioevz marioevz deleted the fork-transition-eip-7691 branch January 22, 2025 21:22
fselmo pushed a commit to fselmo/execution-spec-tests that referenced this pull request Jan 24, 2025
* refactor(tests): EIP-4844, EIP-7691: Fill fork transition blob tests in newer forks

* fix(tests): Rebase fixes, add more checks to the test

* chengelog

* feat(tests): more blob gas tests for fork transitions (ethereum#1107)

* feat(tests): more edge cases for blob gas at transitions

* Apply suggestions from code review

---------

Co-authored-by: Mario Vega <[email protected]>

* fix(tests): Remove type-2 txs, destination account is empty

* tox: typing

---------

Co-authored-by: danceratopz <[email protected]>
codeofcarson pushed a commit to codeofcarson/execution-spec-tests that referenced this pull request Jan 24, 2025
* refactor(tests): EIP-4844, EIP-7691: Fill fork transition blob tests in newer forks

* fix(tests): Rebase fixes, add more checks to the test

* chengelog

* feat(tests): more blob gas tests for fork transitions (ethereum#1107)

* feat(tests): more edge cases for blob gas at transitions

* Apply suggestions from code review

---------

Co-authored-by: Mario Vega <[email protected]>

* fix(tests): Remove type-2 txs, destination account is empty

* tox: typing

---------

Co-authored-by: danceratopz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants