feat(exception mapper): class to verify exception strings #795

winsvega · 2024-09-12T09:34:15Z

🗒️ Description

Extent exception mapper with base class so that any t8n can define exception string maps for any exception types

I put transaction exception validation into exception mapper for convinience atm.
Example exception message:

src/ethereum_test_exceptions/exception_mapper.py:84: in check_transaction
    raise Exception(
E   Exception: TransactionException (pos=1, nonce=0x1)
E   Error: exception mismatch:
E    want = 'Undefined' (TransactionException.TYPE_3_TX_INVALID_BLOB_VERSIONED_HASH),
E    got  = 'blob 0 has invalid hash version' (UndefinedException.UNDEFINED_EXCEPTION)
E    No message defined for TransactionException.TYPE_3_TX_INVALID_BLOB_VERSIONED_HASH, please add it to GethExceptionMapper
E    No exception defined for error message got, please add it to GethExceptionMapper

🔗 Related Issues

✅ Checklist

All: Set appropriate labels for the changes.
All: Considered squashing commits to improve commit history.
All: Added an entry to CHANGELOG.md.
All: Considered updating the online docs in the ./docs/ directory.
Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

winsvega · 2024-09-12T10:32:20Z

src/ethereum_test_specs/base.py

@@ -46,6 +49,13 @@ def verify_transactions(txs: List[Transaction], result: Result) -> List[int]:
            raise Exception(f"tx expected to fail succeeded: pos={i}, nonce={tx.nonce}")
        elif not tx.error and error:
            raise Exception(f"tx unexpectedly failed: {error}")
+        elif isinstance(tx.error, TransactionException) and error:


perhaps this whole part should be inside ExceptionMapper. since we compare x and y and have to cover cases each time the exception is verified
expected y, got x == y
expected y, got x != y
expected 0, got x
expected 0, got 0

marioevz

I like the ExceptionMapper idea, I think it might be too t8n-focused but it could be used in many other places, e.g. exceptions from the Engine API when we run the tests in hive.

I think this PR can start with TransitionToolExceptionMapper defined in src/evm_transition_tool to make ExceptionMapper a bit more generic to open the door to future PRs to implement other subclasses.

marioevz · 2024-09-18T18:36:09Z

src/ethereum_test_exceptions/exception_mapper.py

+        )
+        return exception
+
+    def check_transaction(self, tx_error_message: str | None, tx, tx_pos: int):


Transaction here would cause a cycle import, and I think that is because this method should not be defined here because not all tests depend on transaction exceptions (only state_test).

Probably better to be in ethereum_test_specs/state.py.

After looking through all the changes, I think what we could do is have a subclass called TransitionToolExceptionMapper that is specifically dedicated to map exceptions from the transition tool, and this can be used to subclass specific GethTransaitionToolExceptionMapper and so on.

Another approach could be that we don't need the subclass GethTransaitionToolExceptionMapper and only TransitionToolExceptionMapper, and then in src/evm_transition_tool/geth.py we create an instance of TransitionToolExceptionMapper that contains the map, so we avoid creating too many classes and also avoid instantiating the same GethTransaitionToolExceptionMapper every time we instantiate the t8n tool.

marioevz · 2024-09-18T18:37:34Z

src/ethereum_test_specs/base.py

@@ -31,7 +32,9 @@ def __str__(self):  # noqa: D105
        return f"{self.message}: Expected {self.expected_hash}, got {self.actual_hash}"


-def verify_transactions(txs: List[Transaction], result: Result) -> List[int]:
+def verify_transactions(


I think we should take the opportunity and move this somewhere else, maybe an src/ethereum_test_specs/helpers.py file?

marioevz · 2024-09-18T18:38:46Z

src/ethereum_test_specs/base.py

@@ -12,6 +12,7 @@
 from pydantic import BaseModel, Field

 from ethereum_test_base_types import to_hex
+from ethereum_test_exceptions import ExceptionMapper, TransactionException


Suggested change

from ethereum_test_exceptions import ExceptionMapper, TransactionException

from ethereum_test_exceptions import ExceptionMapper

Unused import.

marioevz · 2024-09-18T19:05:15Z

src/ethereum_test_exceptions/geth_exceptions.py

+from .exceptions import EOFException, TransactionException
+
+
+class GethExceptionMapper(ExceptionMapper):


These are very directly related to the Geth T8N implementation, it might be a good idea to simply move it into src/evm_transition_tool/geth.py.

Same for the evmone_exceptions.py.

Yes. This can also be a base exception mapper.
Then geth t8n defines geth exception mapper for t8n.
And geth consume defines exception mapper for consume driver.

feat(exception mapper): class to verify exception strings

b780afc

winsvega commented Sep 12, 2024

View reviewed changes

add GethExceptionMapper

83308e3

marioevz reviewed Sep 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(exception mapper): class to verify exception strings #795

feat(exception mapper): class to verify exception strings #795

winsvega commented Sep 12, 2024 •

edited

Loading

winsvega Sep 12, 2024 •

edited

Loading

marioevz left a comment

marioevz Sep 18, 2024

marioevz Sep 18, 2024 •

edited

Loading

marioevz Sep 18, 2024

marioevz Sep 18, 2024

marioevz Sep 18, 2024

winsvega Sep 18, 2024

	from ethereum_test_exceptions import ExceptionMapper, TransactionException
	from ethereum_test_exceptions import ExceptionMapper

		from .exceptions import EOFException, TransactionException


		class GethExceptionMapper(ExceptionMapper):

feat(exception mapper): class to verify exception strings #795

Are you sure you want to change the base?

feat(exception mapper): class to verify exception strings #795

Conversation

winsvega commented Sep 12, 2024 • edited Loading

🗒️ Description

🔗 Related Issues

✅ Checklist

winsvega Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

marioevz left a comment

Choose a reason for hiding this comment

marioevz Sep 18, 2024

Choose a reason for hiding this comment

marioevz Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

marioevz Sep 18, 2024

Choose a reason for hiding this comment

marioevz Sep 18, 2024

Choose a reason for hiding this comment

marioevz Sep 18, 2024

Choose a reason for hiding this comment

winsvega Sep 18, 2024

Choose a reason for hiding this comment

winsvega commented Sep 12, 2024 •

edited

Loading

winsvega Sep 12, 2024 •

edited

Loading

marioevz Sep 18, 2024 •

edited

Loading