search

ethereum / evmone

Fast Ethereum Virtual Machine implementation
Apache License 2.0
864 stars 286 forks source link

state: Separate transaction validation from transition #1069

Closed chfast closed 3 weeks ago

chfast commented 3 weeks ago

This change separates transaction validation in state::validate_transaction() from state::transition() which previously was also validating the transaction.

Readability goes up in cost of slightly lower efficiency. The change is also good for API because transaction validation usually happens in different place than execution.

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.24%. Comparing base (66216f9) to head (93f1c45). Report is 2 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1069 +/- ## ======================================= Coverage 94.23% 94.24% ======================================= Files 158 158 Lines 17081 17080 -1 ======================================= Hits 16097 16097 + Misses 984 983 -1 ``` | [Flag](https://app.codecov.io/gh/ethereum/evmone/pull/1069/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | Coverage Δ | | |---|---|---| | [eof_execution_spec_tests](https://app.codecov.io/gh/ethereum/evmone/pull/1069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `17.79% <39.47%> (+<0.01%)` | :arrow_up: | | [ethereum_tests](https://app.codecov.io/gh/ethereum/evmone/pull/1069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `26.50% <44.73%> (+<0.01%)` | :arrow_up: | | [ethereum_tests_silkpre](https://app.codecov.io/gh/ethereum/evmone/pull/1069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `19.10% <45.16%> (ø)` | | | [execution_spec_tests](https://app.codecov.io/gh/ethereum/evmone/pull/1069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `20.22% <44.73%> (+<0.01%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/ethereum/evmone/pull/1069/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `89.29% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files with missing lines](https://app.codecov.io/gh/ethereum/evmone/pull/1069?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | Coverage Δ | | |---|---|---| | [test/state/state.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/1069?src=pr&el=tree&filepath=test%2Fstate%2Fstate.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC9zdGF0ZS9zdGF0ZS5jcHA=) | `99.59% <100.00%> (-0.01%)` | :arrow_down: | | [test/state/state.hpp](https://app.codecov.io/gh/ethereum/evmone/pull/1069?src=pr&el=tree&filepath=test%2Fstate%2Fstate.hpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC9zdGF0ZS9zdGF0ZS5ocHA=) | `100.00% <ø> (ø)` | | | [test/state/test\_state.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/1069?src=pr&el=tree&filepath=test%2Fstate%2Ftest_state.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC9zdGF0ZS90ZXN0X3N0YXRlLmNwcA==) | `100.00% <100.00%> (+1.92%)` | :arrow_up: | | [test/unittests/state\_tx\_test.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/1069?src=pr&el=tree&filepath=test%2Funittests%2Fstate_tx_test.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC91bml0dGVzdHMvc3RhdGVfdHhfdGVzdC5jcHA=) | `100.00% <100.00%> (ø)` | |
chfast commented 3 weeks ago

LGTM, can we fix one thing up when we're at it, it caught me off guard a bit when revieweing: blockchaintest_runner.cpp seems to be the only one to not qualify test::transition with test::, so at first I thought state::transition was meant there and something was not right.

Can you add the test:: there?

Rather not now, because this file is not modified here and there are multiple cases like this. Tests should use evmone::test and rarely but explicitly state::.