Add implementation of evmc::Host and state transition #484

chfast · 2022-07-15T17:18:22Z

This implements state transition as application of a single transaction to a predefined state. Most of the logic is in the evmc::Host implementation plus the transaction specific handling in evmone::state::transition().

These are used to execute all tests by the statetest runner.

The precompiles are not implemented.

codecov · 2022-07-15T17:44:59Z

Codecov Report

Merging #484 (6a9620d) into master (8314df3) will decrease coverage by 1.10%.
The diff coverage is 80.71%.

❗ Current head 6a9620d differs from pull request most recent head fd1d61c. Consider uploading reports for the commit fd1d61c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #484      +/-   ##
==========================================
- Coverage   99.08%   97.97%   -1.11%     
==========================================
  Files          54       59       +5     
  Lines        5348     5680     +332     
==========================================
+ Hits         5299     5565     +266     
- Misses         49      115      +66

Flag	Coverage Δ
blockchaintests	`77.78% <ø> (ø)`
statetests	`26.82% <99.63%> (+19.25%)`	⬆️
unittests	`91.70% <2.68%> (-5.56%)`	⬇️

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

Impacted Files	Coverage Δ
test/statetest/statetest.hpp	`100.00% <ø> (ø)`
test/state/host.hpp	`40.00% <40.00%> (ø)`
test/state/precompiles.cpp	`66.66% <66.66%> (ø)`
test/state/state.cpp	`79.74% <79.74%> (ø)`
test/state/host.cpp	`80.54% <80.54%> (ø)`
test/statetest/statetest_runner.cpp	`81.25% <81.25%> (ø)`
test/state/state.hpp	`88.46% <86.36%> (-11.54%)`	⬇️
test/statetest/statetest.cpp	`94.59% <92.85%> (-5.41%)`	⬇️
test/state/account.hpp	`100.00% <100.00%> (ø)`
test/statetest/statetest_loader.cpp	`80.86% <100.00%> (ø)`
... and 4 more

axic · 2022-09-02T18:53:26Z

test/state/host.hpp

+
+    [[nodiscard]] const auto& get_destructs() const noexcept { return m_destructs; }
+
+    [[nodiscard]] std::vector<Log>&& take_logs() noexcept { return std::move(m_logs); }


Why is this moving while the others are just references?

We return the logs in the end so we can move them out of the Host. The other structures are discarded.

axic · 2022-09-03T12:42:45Z

test/state/host.cpp

+
+bytes32 Host::get_storage(const address& addr, const bytes32& key) const noexcept
+{
+    const auto& acc = m_state.get(addr);


Wouldn't this put it into the hot list?

This assumes the account must exist already. And it is hot already.

axic · 2022-09-03T12:43:18Z

test/state/host.cpp

+{
+bool Host::account_exists(const address& addr) const noexcept
+{
+    const auto* const acc = m_state.get_or_null(addr);


Wouldn't this also put it into the hot list?

This will not (it will not create an account if it doesn't exist). And I believe it should not - this is related to old call cost penalty check. But we should have a test which check if this does not affect account access list.

axic · 2022-09-03T12:45:38Z

test/state/state.hpp

+        return nullptr;
+    }
+
+    Account& get_or_create(const address& addr) { return m_accounts[addr]; }


Hm, so will get() fail if the account doesn't exists? Or why do you need get_or_create?

maybe this is just operator[] ?

I replaced single operator[] with this ugly names because I want to know where new accounts are created (in the context of State cache). This already requires one more round of redesign and probably one more when state journal will be introduced.

From the current usage I need should replace this one with:
get_or_create(address, Account{...}) because I often need to modify the created account straight away.

test/state/precompiles.cpp

axic · 2022-09-03T12:49:07Z

test/statetest/statetest_runner.cpp

+        for (size_t case_index = 0; case_index != cases.size(); ++case_index)
+        {
+            SCOPED_TRACE(std::string{evmc::to_string(rev)} + '/' + std::to_string(case_index));
+            // if (rev != EVMC_FRONTIER)


What are these comments?

There are for debugging. You can narrow a test to single revision and/or case.

gumb0 · 2022-09-23T10:43:05Z

test/state/state.hpp

@@ -24,6 +24,24 @@ class State
        assert(r.second);
        return r.first->second;
    }
+
+    Account& get(const address& addr)


should there be const version?

Also for other methods below

The const version is not required because the only user of this is Host having non-const reference.

test/state/state.hpp

gumb0 · 2022-09-23T12:29:57Z

test/state/state.cpp

+typename std::unordered_map<Key, T, Hash, KeyEqual, Alloc>::size_type erase_if(
+    std::unordered_map<Key, T, Hash, KeyEqual, Alloc>& c, Pred pred)
+{
+    auto old_size = c.size();


can be const

gumb0 · 2022-09-26T13:14:36Z

test/state/state.cpp

+    if (sender_balance_check < tx_max_cost_512)
+        return {};
+
+    sender_balance_check -= static_cast<intx::uint256>(tx_max_cost_512);


why not check sender_acc.balance >= tx_max_cost_512 + tx.value ?

gumb0 · 2022-09-26T13:17:39Z

test/state/state.cpp

+std::optional<std::vector<Log>> transition(
+    State& state, const BlockInfo& block, const Transaction& tx, evmc_revision rev, evmc::VM& vm)
+{
+    if (rev < EVMC_LONDON && tx.kind == Transaction::Kind::eip1559)


Maybe makes sense to extract transaction-related checks into a separate transaction validity funtion.

gumb0 · 2022-09-26T13:26:49Z

test/state/state.cpp

+
+    Host host{rev, vm, state, block, tx};
+
+    const auto result = host.call(build_message(tx, execution_gas_limit));


Where do sender and recipient get their touched flag set? inside call?

In Host::call() for CALL only: recipient_acc.touched = true;.
Sender is not touched.
Are you confused by "touched" vs "warm"?

gumb0 · 2022-09-27T17:56:22Z

test/state/host.cpp

+    auto& new_acc = m_state.get_or_create(msg.recipient);
+    if (m_rev >= EVMC_SPURIOUS_DRAGON)
+        new_acc.nonce = 1;
+    new_acc.storage.clear();  // In case of collision.


confused why this is needed, doesn't it return error above in case of collision?

The collision check ignores storage so you can have an "empty" account with non-empty storage (only in tests though). I have better version of this with better comments and I should bring it here.

test/state/host.cpp

gumb0 · 2022-09-28T09:50:35Z

test/state/host.cpp

+    auto access_addresses_snapshot = m_accessed_addresses;
+    auto logs_snapshot = m_logs.size();
+
+    auto result = execute_message(*msg);


can be const?

No because we have to move it.

Do you mean to return value? copy will be elided even if it's const I think

Yes, but Result is move-only type

test/state/host.cpp

gumb0 · 2022-09-28T10:36:53Z

test/state/host.cpp

+    // TODO: Predefined warm addresses can be applied to the state cache before execution.
+
+    // Transaction {sender,to} are always warm.
+    if (addr == m_tx.to)


Why not add all these and access list to m_accessed_addresses in Host constructor?

On todo list. I have it done already so I will include the change.

Done, but a lot of new code landed.

gumb0 · 2022-09-29T18:41:31Z

test/statetest/statetest_runner.cpp

+// Copyright 2022 The evmone Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+#include "../state/mpt_hash.hpp"


Better <test/state/mpt_hash.hpp> ?

Doesn't work. I probably would need to specify additional include paths what does not sound worth it.

test/statetest/statetest.cpp

test/state/state.hpp

This implements state transition as application of a single transaction to a predefined state. Most of the logic is in evmc::Host implementation plus the transaction specific handling in evmone::state::transition().

chfast force-pushed the state_transition branch 2 times, most recently from eb7bc35 to 229995c Compare July 15, 2022 17:27

chfast requested review from axic, gumb0 and yperbasis August 1, 2022 12:01

chfast force-pushed the state_transition branch 2 times, most recently from cea1e32 to e69c964 Compare August 26, 2022 11:49

axic reviewed Sep 2, 2022

View reviewed changes

axic reviewed Sep 3, 2022

View reviewed changes

test/state/precompiles.cpp Show resolved Hide resolved

axic reviewed Sep 3, 2022

View reviewed changes

chfast force-pushed the state_transition branch 3 times, most recently from a0cdc84 to 7399a0c Compare September 8, 2022 14:10

chfast force-pushed the state_transition branch from 7399a0c to bf42bde Compare September 21, 2022 13:43

gumb0 reviewed Sep 23, 2022

View reviewed changes

test/state/state.hpp Outdated Show resolved Hide resolved

gumb0 reviewed Sep 23, 2022

View reviewed changes

gumb0 reviewed Sep 26, 2022

View reviewed changes

gumb0 reviewed Sep 27, 2022

View reviewed changes

test/state/host.cpp Outdated Show resolved Hide resolved

gumb0 reviewed Sep 27, 2022

View reviewed changes

test/state/host.cpp Show resolved Hide resolved

chfast commented Sep 27, 2022

View reviewed changes

test/state/host.cpp Outdated Show resolved Hide resolved

gumb0 reviewed Sep 28, 2022

View reviewed changes

test/state/host.cpp Show resolved Hide resolved

gumb0 reviewed Sep 28, 2022

View reviewed changes

gumb0 reviewed Sep 29, 2022

View reviewed changes

test/statetest/statetest.cpp Outdated Show resolved Hide resolved

gumb0 reviewed Sep 29, 2022

View reviewed changes

test/state/state.hpp Outdated Show resolved Hide resolved

chfast force-pushed the state_transition branch from 76ccc3b to 5d2f2a7 Compare October 9, 2022 19:53

chfast force-pushed the state_transition branch 3 times, most recently from 8aa716a to 0d6e918 Compare October 21, 2022 10:35

chfast force-pushed the state_transition branch 2 times, most recently from 40d0da0 to 6a9620d Compare October 27, 2022 12:11

chfast and others added 5 commits October 27, 2022 14:45

Add implementation of evmc::Host and state transition

5c64d69

This implements state transition as application of a single transaction to a predefined state. Most of the logic is in evmc::Host implementation plus the transaction specific handling in evmone::state::transition().

Add precompiles without implementation

af78565

statetest: Implement state test runner

9f56f6e

statetest: Set default test filter

b5b9b6d

ci: Temporarily ignore failing state tests

fd1d61c

chfast force-pushed the state_transition branch from 6a9620d to fd1d61c Compare October 27, 2022 12:51

chfast merged commit faf64b2 into master Oct 27, 2022

chfast deleted the state_transition branch October 27, 2022 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add implementation of evmc::Host and state transition #484

Add implementation of evmc::Host and state transition #484

chfast commented Jul 15, 2022

codecov bot commented Jul 15, 2022 •

edited

Loading

axic Sep 2, 2022

chfast Sep 3, 2022

axic Sep 3, 2022

chfast Sep 3, 2022

axic Sep 3, 2022

chfast Sep 3, 2022

axic Sep 3, 2022

gumb0 Sep 23, 2022

chfast Sep 26, 2022

axic Sep 3, 2022

chfast Sep 3, 2022

gumb0 Sep 23, 2022

gumb0 Sep 23, 2022 •

edited

Loading

gumb0 Sep 23, 2022

chfast Sep 26, 2022

gumb0 Sep 23, 2022

gumb0 Sep 26, 2022

gumb0 Sep 26, 2022

gumb0 Sep 26, 2022

chfast Sep 26, 2022

gumb0 Sep 27, 2022

chfast Sep 27, 2022

gumb0 Sep 28, 2022

chfast Sep 29, 2022

gumb0 Sep 29, 2022

chfast Oct 27, 2022

gumb0 Sep 28, 2022

chfast Sep 29, 2022

chfast Sep 29, 2022

gumb0 Sep 29, 2022

chfast Sep 30, 2022


		[[nodiscard]] const auto& get_destructs() const noexcept { return m_destructs; }

		[[nodiscard]] std::vector<Log>&& take_logs() noexcept { return std::move(m_logs); }


		Host host{rev, vm, state, block, tx};

		const auto result = host.call(build_message(tx, execution_gas_limit));

Add implementation of evmc::Host and state transition #484

Add implementation of evmc::Host and state transition #484

Conversation

chfast commented Jul 15, 2022

codecov bot commented Jul 15, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 15, 2022 •

edited

Loading

gumb0 Sep 23, 2022 •

edited

Loading