fix(tests): accept tx-level exception for over-budget block (EIP-7778)#2949
Conversation
… (EIP-7778) In test_multi_transaction_gas_accounting, a block whose included transaction exceeds the remaining block gas was expected to fail only with the block-level BlockException.GAS_USED_OVERFLOW. A client may legitimately report the same condition at the transaction level: EELS raises GasUsedExceedsLimitError (an InvalidTransaction subclass) when `tx.gas > gas_available`, and evmone reports it as a rejected transaction (GAS_LIMIT_REACHED). List both BlockException.GAS_USED_OVERFLOW and TransactionException.GAS_ALLOWANCE_EXCEEDED as acceptable so either categorization passes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #2949 +/- ##
================================================
Coverage 90.50% 90.50%
================================================
Files 535 535
Lines 32407 32407
Branches 3011 3011
================================================
Hits 29331 29331
Misses 2559 2559
Partials 517 517
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
danceratopz
left a comment
There was a problem hiding this comment.
This is the right fix.
At least for now, but this is something we should definitely revisit when we attack the exception test topic properly.
The specs themselves also raise a transaction-level exception for this and it bothered me that we don't dogfood our own fixtures to actually verify this and avoid this issue. Indeed, it would appear other clients have essentially worked around this exception expectation of this test by avoiding distinct substring matches for both the transaction-level and the block-level exception. That's the only reason why no one else flagged this, but this is the better fix. I.e., 5 of the client exception mappers more or less define the following entries to ensure they pass this class of test:
GAS_ALLOWANCE_EXCEEDED: "Block gas limit exceeded" # TransactionException
GAS_USED_OVERFLOW: "Block gas limit exceeded" # BlockExceptionOn the specs side:
state_transition(chain, block) fork.py:229 ← block import/validation
entrypoint
└─ apply_body(...) fork.py:320/809
└─ for each tx: process_transaction(...) fork.py:854/926
└─ check_transaction(...) fork.py:974/482
└─ raise GasUsedExceedsLimitError fork.py:551 ← here
but this is not verified rigorously anywhere against the fixture's exception or we would have caught this earlier. Two issues:
- Nowhere do the specs currently consume all new test fixtures in CI.
- Even where specs consume fixtures, the
json-loaderonly verifies that an exception occurs, but not that it's the correct one, so we don't hold the specs to the same standards as the clients.
|
I only started matching error codes this week. And this is only partial for now. |
Just fyi, we could consider adding it |
🗒️ Description
In test_multi_transaction_gas_accounting, a block whose included transaction exceeds the remaining block gas was expected to fail only with the block-level BlockException.GAS_USED_OVERFLOW. A client may legitimately report the same condition at the transaction level as TransactionException.GAS_ALLOWANCE_EXCEEDED (the best I found).
🔗 Related Issues or PRs
N/A.
✅ Checklist
just statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture