Skip to content

feat(tests): EIP-8037 reject when calldata_floor > TX_MAX_GAS_LIMIT#2898

Open
chfast wants to merge 2 commits into
ethereum:forks/amsterdamfrom
chfast:feat/eip-8037-intrinsic-cap-at-validation
Open

feat(tests): EIP-8037 reject when calldata_floor > TX_MAX_GAS_LIMIT#2898
chfast wants to merge 2 commits into
ethereum:forks/amsterdamfrom
chfast:feat/eip-8037-intrinsic-cap-at-validation

Conversation

@chfast

@chfast chfast commented May 22, 2026

Copy link
Copy Markdown
Member

🗒️ Description

EIP-8037 requires max(intrinsic_regular, calldata_floor) <= TX_MAX_GAS_LIMIT. The existing tx.gas_limit < total_intrinsic check already covers the intrinsic-regular arm, so only the calldata_floor arm is observably untested.

Parametrize types 1 and 2 over floor_binds, intrinsic_binds, and neither_binds.

🔗 Related Issues or PRs

N/A.

✅ Checklist

  • All: Ran fast static checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    just static
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
  • Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
  • Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

marioevz
marioevz previously approved these changes Jun 9, 2026

@marioevz marioevz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The test looks solid, just some minor comments.

@marioevz marioevz changed the base branch from devnets/bal/7 to forks/amsterdam June 9, 2026 20:42
@marioevz marioevz dismissed their stale review June 9, 2026 20:42

The base branch was changed.

@marioevz

marioevz commented Jun 9, 2026

Copy link
Copy Markdown
Member

@chfast I changed the base because EIP-8037 is now merged to forks/amsterdam but git didn't take it so well. Could you rebase please?

EIP-8037 requires max(intrinsic_regular, calldata_floor) <=
TX_MAX_GAS_LIMIT. The existing `tx.gas_limit < total_intrinsic` check
already covers the intrinsic-regular arm, so only the calldata_floor
arm is observably untested.

Parametrize types 1 and 2 over floor_binds, intrinsic_binds, and
neither_binds. evmone amsterdam/main without fd35d839 fails the
floor_binds rows; geth bal-devnet-7 and EELS pass all.
@chfast chfast force-pushed the feat/eip-8037-intrinsic-cap-at-validation branch from c83f348 to cbf96d8 Compare June 10, 2026 07:11
@chfast chfast requested a review from marioevz June 10, 2026 07:12
@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.00%. Comparing base (84e39e1) to head (f41c4b3).
⚠️ Report is 2 commits behind head on forks/amsterdam.

Files with missing lines Patch % Lines
src/ethereum/forks/amsterdam/transactions.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##           forks/amsterdam    #2898      +/-   ##
===================================================
- Coverage            90.53%   89.00%   -1.53%     
===================================================
  Files                  535      496      -39     
  Lines                32897    30100    -2797     
  Branches              3021     2725     -296     
===================================================
- Hits                 29782    26790    -2992     
- Misses                2596     2835     +239     
+ Partials               519      475      -44     
Flag Coverage Δ
unittests 89.00% <50.00%> (-1.53%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@spencer-tb spencer-tb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice test additions! I don't think these fully hit the correct checks in EELS yet.

EELS validates intrinsic > tx.gas, floor > tx.gas in that order and then the two 8037 checks: intrinsic.regular > TX_MAX_GAS_LIMIT, intrinsic.calldata_floor > TX_MAX_GAS_LIMIT. With the tx gas_limit = cap + 1 both tx cases are rejected by the first 2 checks that we had originally.

I attached a suggestion below to target the intrinsic.regular > TX_MAX_GAS_LIMIT, intrinsic.calldata_floor > TX_MAX_GAS_LIMIT checks respectively. Let me know if it makes sense!

Additionally I think we could have a different exception type for these cases to distinguish the difference. The latter can be a follow up or feel free if it makes sense, would require an EELS spec change.

Comment on lines +71 to +80
if scenario == "floor_binds":
data = b"\x01" * ((cap - gas_costs.TX_BASE) // floor_per_byte + 1)
access_list = []
elif scenario == "intrinsic_binds":
data = b""
n = (cap - gas_costs.TX_BASE) // gas_costs.TX_ACCESS_LIST_ADDRESS + 1
access_list = [
AccessList(address=Address(i + 1), storage_keys=[])
for i in range(n)
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if scenario == "floor_binds":
data = b"\x01" * ((cap - gas_costs.TX_BASE) // floor_per_byte + 1)
access_list = []
elif scenario == "intrinsic_binds":
data = b""
n = (cap - gas_costs.TX_BASE) // gas_costs.TX_ACCESS_LIST_ADDRESS + 1
access_list = [
AccessList(address=Address(i + 1), storage_keys=[])
for i in range(n)
]
gas_limit = cap + 1
if scenario == "floor_binds":
data = b"\x01" * ((cap - gas_costs.TX_BASE) // floor_per_byte + 1)
access_list = []
gas_limit = fork.transaction_data_floor_cost_calculator()(
data=data, access_list=access_list
)
elif scenario == "intrinsic_binds":
data = b""
n = (cap - gas_costs.TX_BASE) // gas_costs.TX_ACCESS_LIST_ADDRESS + 1
access_list = [
AccessList(address=Address(i + 1), storage_keys=[])
for i in range(n)
]
gas_limit = fork.transaction_intrinsic_cost_calculator()(
calldata=data, access_list=access_list
)

[
pytest.param(
"floor_binds",
TransactionException.INTRINSIC_GAS_TOO_LOW,

@spencer-tb spencer-tb Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
TransactionException.INTRINSIC_GAS_TOO_LOW,
TransactionException.FLOOR_GAS_EXCEEDS_MAXIMUM,

),
pytest.param(
"intrinsic_binds",
TransactionException.INTRINSIC_GAS_TOO_LOW,

@spencer-tb spencer-tb Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
TransactionException.INTRINSIC_GAS_TOO_LOW,
TransactionException.INTRINSIC_GAS_COST_EXCEEDS_MAXIMUM,

… over cap

Per review: EELS raised a single InsufficientTransactionGasError for both
`intrinsic.regular > TX_MAX_GAS_LIMIT` and
`intrinsic.calldata_floor > TX_MAX_GAS_LIMIT`, conflating the EIP-8037/7825
cap rule with the generic gas-too-low check. Add distinct exceptions:

- EELS amsterdam: IntrinsicGasCostExceedsMaximumError,
  FloorGasExceedsMaximumError; validate_transaction raises them.
- Framework: TransactionException.INTRINSIC_GAS_COST_EXCEEDS_MAXIMUM,
  FLOOR_GAS_EXCEEDS_MAXIMUM, mapped in the execution-specs CLI.
- test_intrinsic_or_floor_cap_at_validation: expect the distinct errors and
  size gas_limit to the exact floor/intrinsic cost so the rejection is
  pinned to the cap rule, not gas_limit < intrinsic.
@spencer-tb

Copy link
Copy Markdown
Contributor

Thanks, moving this to #2915 as will have spec change! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants