New evmone API: StateView & StateDiff by chfast · Pull Request #802 · ipsilon/evmone

chfast · 2024-01-31T13:37:48Z

A step towards of the new API for evmone. It uses the test runners as users and modifies the State API from state.h.

the user provides read-only StateView interface with (simpler than State) representation of the State.
the transaction execution (i.e. evmone via transition()) takes the StateView and produces StateDiff.
user must apply the StateDiff to its StateView implementation.

The current state diff building is based on visiting the intra state cache of accounts loaded from initial state. This approach is relatively simple but has some false positives and may not be efficient.

codecov · 2024-02-02T10:19:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.23%. Comparing base (8e4a055) to head (3350cd7).
Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
+ Coverage   94.21%   94.23%   +0.01%     
==========================================
  Files         153      155       +2     
  Lines       15934    15968      +34     
==========================================
+ Hits        15012    15047      +35     
+ Misses        922      921       -1

Flag	Coverage Δ
eof_execution_spec_tests	`17.69% <93.10%> (+0.15%)`	⬆️
ethereum_tests	`27.49% <100.00%> (+0.15%)`	⬆️
ethereum_tests_silkpre	`19.35% <93.39%> (+0.20%)`	⬆️
execution_spec_tests	`20.61% <98.27%> (+0.15%)`	⬆️
unittests	`89.02% <98.27%> (+0.01%)`	⬆️

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

Files with missing lines	Coverage Δ
test/state/account.hpp	`100.00% <100.00%> (ø)`
test/state/host.cpp	`100.00% <100.00%> (ø)`
test/state/state.cpp	`100.00% <100.00%> (ø)`
test/state/state.hpp	`100.00% <100.00%> (ø)`
test/state/state_diff.hpp	`100.00% <100.00%> (ø)`
test/state/state_view.hpp	`100.00% <100.00%> (ø)`
test/state/system_contracts.cpp	`100.00% <100.00%> (ø)`
test/state/test_state.cpp	`100.00% <100.00%> (ø)`
test/state/test_state.hpp	`88.88% <100.00%> (+8.88%)`	⬆️
test/state/transaction.hpp	`100.00% <ø> (ø)`

chfast · 2024-09-16T10:32:05Z

    // TODO: Cache code hash. It will be needed also to compute the MPT hash.
+    const auto raw_code = m_state.get_code(addr);
+    if (is_eof_container(raw_code))
+        return keccak256(raw_code.substr(0, 2));


It is probably good idea to handle EOF ext code and possible code hash caching up front.

Proposed in #1035.

pdobacz

I still need to wrap head around this, but here some early comments

pdobacz · 2024-09-16T16:44:19Z

 [[nodiscard]] bool is_create_collision(const Account& acc) noexcept
 {
-    if (acc.nonce != 0 || !acc.code.empty())
+    // TODO: This requires much more testing:


better file as issue(s)?

pdobacz · 2024-09-16T16:52:58Z

        std::variant<JournalBalanceChange, JournalTouched, JournalStorageChange, JournalNonceBump,
            JournalCreate, JournalTransientStorageChange, JournalDestruct, JournalAccessAccount>;

+    const StateView* m_cold = nullptr;


comment pls what this means, making sure to answer its relation to the notion of cold/warm storage (access lists and whatnot). I kinda know but would appreciate the comment to make sure and make it easier to digest.

I would maybe rename this and m_accounts: m_initial_state and m_modified_accounts maybe.

The "initial" and "modified" sounds nice. How about just m_initial and m_modified?

Wrap the usage of the state transition API from the `evmone::state` for tests so that the new API in `evmone::test` only exposes `TestState` and hides `state::State`. This isolates usage of the `evmone::state` to lower the disturbance caused by API modifications, e.g. in #802.

chfast · 2024-09-30T09:48:14Z

-    if (std::ranges::any_of(
-            acc.storage, [](auto& e) noexcept { return !is_zero(e.second.current); }))
+    if (acc.code_hash != Account::EMPTY_CODE_HASH)
+        return true;


This is not covered by any tests, so more work for #1037. Should I remove/shorten the TODO comments here?

gumb0 · 2024-10-08T15:58:39Z

+        // TODO(clang): In old Clang emplace_back without Account doesn't compile.
+        //   NOLINTNEXTLINE(modernize-use-emplace)
+        auto& a = diff.modified_accounts.emplace_back(StateDiff::Entry{addr, m.nonce, m.balance});
+        if (m.just_created && !m.code.empty())


Note for the future: in 7702 the code may change 😱 (when resetting existing delegation to another account)

It should be doable.

gumb0 · 2024-10-08T16:41:20Z

Why StateView has to be an interface?

Introduce the type `StateDiff` to represent a set of modifications to the State. Implement the procedure to collect the set of changes out of an intra state object.

Implement `TestState::appy(StateDiff)` procedure to apply state changes back to the state.

chfast · 2024-10-09T11:14:13Z

Why StateView has to be an interface?

This is the interface a user of the API has to implement (similarly to EVMC's Host). In evmone the user is TestState but later there are going to be implementations in Silkworm and Solidity.

Return `StateDiff` out of: - `state::transition()` (as a subobject of `TransactionReceipt`) - `state::system_call()` - `state::finalize()`

Introduce `StateView` interface as a way to read the initial/cold State.

Implement `StateView` interface in `TestState`. This will be used later.

gumb0 · 2024-10-09T13:51:40Z

    if (is_precompile(m_rev, msg.code_address))
        return call_precompile(m_rev, msg);

-    // In case msg.recipient == msg.code_address, this is the second lookup of the same address.


Comment is still true I think.

Kind of true, but now this is the State::get_code() API issue: how to combine local account lookup with code lookup.

Modify the `State` to require `StateView` interface as a parameter and use it to read the initial state values. This drops `TestState` dependency on `State`.

Wrap the usage of the state transition API from the `evmone::state` for tests so that the new API in `evmone::test` only exposes `TestState` and hides `state::State`. This isolates usage of the `evmone::state` to lower the disturbance caused by API modifications, e.g. in ipsilon/evmone#802.

chfast mentioned this pull request Feb 1, 2024

test: New State transition tests #806

Merged

chfast force-pushed the state/state_view branch from 0f92b8a to af88125 Compare February 2, 2024 10:15

chfast force-pushed the state/state_view branch 3 times, most recently from 1ea121a to 3f608b1 Compare February 2, 2024 14:07

chfast force-pushed the master branch from 606b735 to ffd7390 Compare February 2, 2024 14:20

chfast force-pushed the state/state_view branch 3 times, most recently from d77ecd8 to 83342e2 Compare February 6, 2024 10:34

chfast force-pushed the master branch from 7717742 to d5e55fc Compare March 26, 2024 20:50

chfast force-pushed the state/state_view branch 2 times, most recently from 0028573 to 1bab196 Compare August 30, 2024 13:33

chfast self-assigned this Sep 9, 2024

chfast force-pushed the state/state_view branch 3 times, most recently from e4e3eb4 to 838a5cf Compare September 16, 2024 10:01

chfast changed the title ~~New API prototype: StateView~~ New evmone API: StateView & StateDiff Sep 16, 2024

chfast marked this pull request as ready for review September 16, 2024 10:27

chfast requested review from gumb0 and pdobacz September 16, 2024 10:30

chfast commented Sep 16, 2024

View reviewed changes

pdobacz reviewed Sep 16, 2024

View reviewed changes

chfast mentioned this pull request Sep 18, 2024

test: Wrap usage of state transition API #1013

Merged

chfast force-pushed the state/state_view branch from 28caf35 to a7cac6e Compare September 18, 2024 07:55

chfast force-pushed the state/state_view branch from a7cac6e to 5ef5009 Compare September 18, 2024 08:41

chfast force-pushed the state/state_view branch from 5ef5009 to 65c7d8e Compare September 18, 2024 15:59

chfast force-pushed the state/state_view branch 2 times, most recently from 4c5c323 to d69bc4c Compare September 28, 2024 14:33

chfast requested a review from pdobacz September 28, 2024 14:33

pdobacz approved these changes Sep 30, 2024

View reviewed changes

chfast commented Sep 30, 2024

View reviewed changes

chfast force-pushed the state/state_view branch 2 times, most recently from 8a8e576 to da4cd30 Compare September 30, 2024 12:57

gumb0 reviewed Oct 8, 2024

View reviewed changes

Comment thread test/state/state.hpp Outdated

gumb0 reviewed Oct 8, 2024

View reviewed changes

Comment thread test/state/test_state.cpp Outdated

chfast added 2 commits October 9, 2024 13:08

state: Introduce StateDiff

029d377

Introduce the type `StateDiff` to represent a set of modifications to the State. Implement the procedure to collect the set of changes out of an intra state object.

test: Implement StateDiff handling in TestState

190dca0

Implement `TestState::appy(StateDiff)` procedure to apply state changes back to the state.

chfast force-pushed the state/state_view branch from da4cd30 to bd366dc Compare October 9, 2024 11:12

gumb0 reviewed Oct 9, 2024

View reviewed changes

Comment thread test/state/test_state.cpp Outdated

gumb0 reviewed Oct 9, 2024

View reviewed changes

Comment thread test/state/test_state.cpp Outdated

gumb0 reviewed Oct 9, 2024

View reviewed changes

Comment thread test/state/account.hpp Outdated

chfast added 3 commits October 9, 2024 15:50

state: Produce StateDiff from state modifying functions

0f636ca

Return `StateDiff` out of: - `state::transition()` (as a subobject of `TransactionReceipt`) - `state::system_call()` - `state::finalize()`

state: Introduce StateView interface

3f36824

Introduce `StateView` interface as a way to read the initial/cold State.

test: TestState implements StateView

bee9b60

Implement `StateView` interface in `TestState`. This will be used later.

gumb0 reviewed Oct 9, 2024

View reviewed changes

chfast force-pushed the state/state_view branch from bd366dc to 889d23f Compare October 9, 2024 13:53

gumb0 approved these changes Oct 9, 2024

View reviewed changes

state: Use StateView from State

3350cd7

Modify the `State` to require `StateView` interface as a parameter and use it to read the initial state values. This drops `TestState` dependency on `State`.

chfast force-pushed the state/state_view branch from 889d23f to 3350cd7 Compare October 9, 2024 14:06

chfast merged commit 8399303 into master Oct 9, 2024

chfast deleted the state/state_view branch October 9, 2024 15:36

chfast mentioned this pull request Oct 10, 2024

state: update account.hpp #965

Closed

Conversation

chfast commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdobacz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gumb0 commented Oct 8, 2024

Uh oh!

chfast commented Oct 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chfast commented Jan 31, 2024 •

edited

Loading

codecov Bot commented Feb 2, 2024 •

edited

Loading