EIP-2929: Gas cost increases for state access opcodes by yperbasis · Pull Request #289 · ipsilon/evmone

yperbasis · 2021-02-15T12:30:46Z

Keeping as draft until ipsilon/evmc#571 is merged.

chfast · 2021-02-16T10:45:36Z

lib/evmone/instructions_calls.cpp


    state.stack.push(0);  // Assume failure.

+    if (state.rev >= EVMC_BERLIN && state.host.access_account(dst) == EVMC_COLD_ACCESS)


Minimal improvement here would be to merge access_account() and account_exists(). I believe the account can have 3 statuses then: hot, cold, non-existing.

BTW, can a non-existing account become hot?

Not sure that merging access_account() with account_exists() is worth it. It might save a function call, but the two are quite different: account_exists() (EIP-161) is a constant operation that doesn't change anything, while access_account() modifies accessed_addresses substate (EIP-2929).

SELFDESTRUCT puts non-existing beneficiary into a warm set, the same I thihk with calls, so warm/cold and existing/non-existing seem to be orthogonal.

One could also think that account_exists puts non-existing account into a warm set, it's used only in SELFDESTRUCT and calls which do it, and it would reflect reality of updating non-existence caches in clients.

chfast · 2021-02-16T10:48:34Z

lib/evmone/instructions.hpp

+            return EVMC_OUT_OF_GAS;
+    }
+
+    x = intx::be::load<uint256>(state.host.get_balance(addr));


Here and with other methods accessing information about accounts the get_balance() can be extended to return both account status and the balance. The gas check should be done after the call, although I'm sure @holiman would be happy with such implementation. But my reasoning that even "access account" makes it hot so it can also return the balance in the same time.

That was my thinking at first with sload, to return both the value and the access status. But then I realised that it complicates the API. I believe that having a separate method specifically to record access in the sense of EIP-2929 leads to a simpler API compared to reporting access status in various methods. The performance overhead shouldn't be too big, just an extra function call.

lib/evmone/instructions.hpp

codecov-io · 2021-03-24T09:17:28Z

Codecov Report

Merging #289 (cc7614b) into master (b9facfa) will decrease coverage by 0.67%.
The diff coverage is 60.93%.

❗ Current head cc7614b differs from pull request most recent head 3b07834. Consider uploading reports for the commit 3b07834 to get more accurate results

@@            Coverage Diff             @@
##           master     #289      +/-   ##
==========================================
- Coverage   99.75%   99.07%   -0.68%     
==========================================
  Files          24       24              
  Lines        3659     3694      +35     
==========================================
+ Hits         3650     3660      +10     
- Misses          9       34      +25

Flag	Coverage Δ
unittests	`99.07% <60.93%> (-0.68%)`	⬇️

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

Impacted Files	Coverage Δ
lib/evmone/instructions.cpp	`100.00% <ø> (ø)`
lib/evmone/instructions_calls.cpp	`97.93% <33.33%> (-2.07%)`	⬇️
lib/evmone/baseline.cpp	`98.42% <50.00%> (-1.40%)`	⬇️
lib/evmone/instructions.hpp	`96.58% <66.66%> (-3.42%)`	⬇️

chfast · 2021-04-07T13:39:01Z

Consensus tests are going to be updated later in #298.

gumb0 · 2021-04-09T09:51:03Z

lib/evmone/instruction_traits.hpp

 namespace evmone::instr
 {
+/// EIP-2929 constants (https://eips.ethereum.org/EIPS/eip-2929).
+inline constexpr auto cold_sload_cost = 2100;


Not sure what we're going to do with these constants if they change in future forks... Mabye just prepend with berlin_. But I guess it's ok to leave like this now.

True. But it would be easy to move them to constexpr array. E.g. instr::constants[EVMC_BERLIN].cold_sload_cost.

gumb0 · 2021-04-09T10:31:37Z

lib/evmone/instruction_traits.hpp

+inline constexpr auto cold_sload_cost = 2100;
+inline constexpr auto cold_account_access_cost = 2600;
+inline constexpr auto warm_storage_read_cost = 100;
+inline constexpr auto additional_cold_account_access_cost =


Maybe introduce also additional_sload_cost and use it in sload for consistency.

I used the name, but only locally where it is used. Also explained the global definition.

gumb0 · 2021-04-09T11:03:40Z

test/unittests/evm_eip2929_test.cpp

+
+    const auto& r = host.recorded_account_accesses;
+    ASSERT_EQ(r.size(), 24);
+    EXPECT_EQ(r[0], msg.sender);


Maybe could be better-looking with some fancy assert

EXPECT_THAT(r, ElementsAre(msg.sender, msg.destination, ...));

This is fine so I don't need to count these in my head. And I don't need to import Google Mock.

axic · 2021-04-12T11:19:29Z

lib/evmone/instructions.hpp

-        state.host.get_storage(state.msg->destination, intx::be::store<evmc::bytes32>(x)));
+    const auto key = intx::be::store<evmc::bytes32>(x);
+
+    if (state.rev >= EVMC_BERLIN &&


Is there any measureable performance diff for these revision checks? Should we templetize based on version at some point?

There are not benchmarks for state access. I could add some, but these are may not be "portable" because depending on the pre-state initialization.

I don't think it is worth duplicating the code with templates for these. However, it may be good to priorities the most recent revision in the code.

axic · 2021-04-12T11:48:25Z

lib/evmone/instructions.cpp

    table[OP_SHL] = op<shl>;
    table[OP_SHR] = op<shr>;
    table[OP_SAR] = op<sar>;
-    table[OP_EXTCODEHASH] = op<extcodehash>;


Why is this removed here?

Oh yeah, I was wandering the same @yperbasis.

Oh, I simply removed duplicated code; see line 323.

Ah ok. I will sort these in separate PR then.

This uses temporary Silkworm version matching EVMC 8.0.0-alpha.1 ABI.

axic · 2021-04-20T16:44:51Z

This was merged under #301.

yperbasis marked this pull request as draft February 15, 2021 12:30

yperbasis requested a review from chfast February 15, 2021 12:30

yperbasis mentioned this pull request Feb 15, 2021

Support EIP-2929: Gas cost increases for state access opcodes ipsilon/evmc#571

Merged

chfast reviewed Feb 16, 2021

View reviewed changes

chfast requested review from axic, gumb0 and holiman February 16, 2021 17:29

holiman reviewed Feb 16, 2021

View reviewed changes

lib/evmone/instructions.hpp Show resolved Hide resolved

yperbasis force-pushed the eip-2929 branch from b400975 to a251d9a Compare February 19, 2021 13:12

yperbasis changed the title ~~[WIP] EIP-2929: Gas cost increases for state access opcodes~~ EIP-2929: Gas cost increases for state access opcodes Feb 19, 2021

chfast force-pushed the eip-2929 branch 4 times, most recently from c53634e to 915c089 Compare April 7, 2021 13:37

chfast changed the base branch from master to evmc April 7, 2021 13:38

chfast force-pushed the eip-2929 branch 4 times, most recently from 6f18e82 to c63312d Compare April 8, 2021 15:15

gumb0 reviewed Apr 9, 2021

View reviewed changes

chfast force-pushed the evmc branch from 227faee to d37a46d Compare April 9, 2021 12:50

chfast force-pushed the eip-2929 branch from c63312d to e9c827e Compare April 9, 2021 12:59

axic reviewed Apr 12, 2021

View reviewed changes

chfast and others added 2 commits April 12, 2021 17:53

Update EVMC to version with EIP-2929

2a5b247

Set instruction costs for Berlin / EIP-2929

a48e89d

yperbasis and others added 6 commits April 12, 2021 17:53

Implement EIP-2929 for SLOAD and SSTORE

931783c

Implement EIP-2929 for EXT* & BALANCE instructions

2587bb7

Implement EIP-2929 for CALL* instructions

f879b2d

Implement EIP-2929 for SELFDESTRUCT

d04e32e

ci: Upgrade Silkworm and Ethereum tests (8.0.2)

9528ab4

This uses temporary Silkworm version matching EVMC 8.0.0-alpha.1 ABI.

test: Add EIP-2929 unit tests

3ff3507

chfast force-pushed the eip-2929 branch from e9c827e to 3ff3507 Compare April 12, 2021 15:53

chfast force-pushed the evmc branch from d37a46d to b7cd0c8 Compare April 13, 2021 11:53

chfast closed this Apr 13, 2021

chfast deleted the branch ipsilon:evmc April 13, 2021 12:00

chfast mentioned this pull request Apr 13, 2021

EIP-2929: Gas cost increases for state access opcodes #301

Merged


		state.stack.push(0); // Assume failure.

		if (state.rev >= EVMC_BERLIN && state.host.access_account(dst) == EVMC_COLD_ACCESS)

Conversation

yperbasis commented Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yperbasis Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

yperbasis Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Mar 24, 2021

Codecov Report

Uh oh!

chfast commented Apr 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chfast Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

axic commented Apr 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yperbasis commented Feb 15, 2021 •

edited

Loading

yperbasis Feb 16, 2021 •

edited

Loading

yperbasis Feb 16, 2021 •

edited

Loading

chfast Apr 9, 2021 •

edited

Loading