Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions circle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ commands:
steps:
- restore_cache:
name: "Restore Silkworm cache"
key: &silkworm-cache-key silkworm-v1
key: &silkworm-cache-key silkworm-20210407
- run:
name: "Check Silkworm cache"
command: |
Expand All @@ -38,11 +38,11 @@ commands:
working_directory: ~/silkworm/src
command: |
[ "$SILKWORM_BUILD" = true ] || exit 0
git clone --no-checkout --single-branch https://github.com/torquem-ch/silkworm.git .
git checkout 614fabfcf19b8e06f0dcbe3e9af2345dcf748f3e
git clone --no-checkout --single-branch --branch eip2929_update https://github.com/torquem-ch/silkworm.git .
git checkout b083086cd4692b2a7f54af06644324a833fb4d3e
git submodule init
git submodule deinit tests
git submodule update --recursive --depth=1 --progress
git submodule update --init --recursive --depth=1 --progress
- run:
name: "Configure Silkworm"
working_directory: ~/silkworm
Expand Down Expand Up @@ -247,7 +247,7 @@ jobs:
- build
- build_silkworm
- download_consensus_tests:
rev: v7.0.0
rev: 8.0.2
- run:
name: "Silkworm-driven consensus tests"
working_directory: ~/build
Expand Down
37 changes: 32 additions & 5 deletions lib/evmone/baseline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,15 @@ evmc_result baseline_execute(ExecutionState& state) noexcept
address(state);
break;
case OP_BALANCE:
balance(state);
{
const auto status_code = balance(state);
if (status_code != EVMC_SUCCESS)
{
state.status = status_code;
goto exit;
}
break;
}
case OP_ORIGIN:
origin(state);
break;
Expand Down Expand Up @@ -270,8 +277,15 @@ evmc_result baseline_execute(ExecutionState& state) noexcept
gasprice(state);
break;
case OP_EXTCODESIZE:
extcodesize(state);
{
const auto status_code = extcodesize(state);
if (status_code != EVMC_SUCCESS)
{
state.status = status_code;
goto exit;
}
break;
}
case OP_EXTCODECOPY:
{
const auto status_code = extcodecopy(state);
Expand All @@ -296,9 +310,15 @@ evmc_result baseline_execute(ExecutionState& state) noexcept
break;
}
case OP_EXTCODEHASH:
extcodehash(state);
{
const auto status_code = extcodehash(state);
if (status_code != EVMC_SUCCESS)
{
state.status = status_code;
goto exit;
}
break;

}
case OP_BLOCKHASH:
blockhash(state);
break;
Expand Down Expand Up @@ -381,8 +401,15 @@ evmc_result baseline_execute(ExecutionState& state) noexcept
msize(state);
break;
case OP_SLOAD:
sload(state);
{
const auto status_code = sload(state);
if (status_code != EVMC_SUCCESS)
{
state.status = status_code;
goto exit;
}
break;
}
case OP_SSTORE:
{
const auto status_code = sstore(state);
Expand Down
30 changes: 29 additions & 1 deletion lib/evmone/instruction_traits.hpp
Original file line number Diff line number Diff line change
@@ -1,12 +1,27 @@
// evmone: Fast Ethereum Virtual Machine implementation
// Copyright 2020 The evmone Authors.
// SPDX-License-Identifier: Apache-2.0
#pragma once

#include <evmc/instructions.h>
#include <array>

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@chfast chfast Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

inline constexpr auto cold_account_access_cost = 2600;
inline constexpr auto warm_storage_read_cost = 100;

/// Additional cold account access cost.
///
/// The warm access cost is unconditionally applied for every account access instruction.
/// If the access turns out to be cold, this cost must be applied additionally.
inline constexpr auto additional_cold_account_access_cost =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cold_account_access_cost - warm_storage_read_cost;
/// @}

/// The EVM instruction traits.
struct Traits
{
Expand Down Expand Up @@ -333,5 +348,18 @@ constexpr inline std::array<int16_t, 256> gas_costs<EVMC_ISTANBUL> = []() noexce
}();

template <>
constexpr inline auto gas_costs<EVMC_BERLIN> = gas_costs<EVMC_ISTANBUL>;
constexpr inline std::array<int16_t, 256> gas_costs<EVMC_BERLIN> = []() noexcept {
auto table = gas_costs<EVMC_ISTANBUL>;
table[OP_EXTCODESIZE] = warm_storage_read_cost;
table[OP_EXTCODECOPY] = warm_storage_read_cost;
table[OP_EXTCODEHASH] = warm_storage_read_cost;
table[OP_BALANCE] = warm_storage_read_cost;
table[OP_CALL] = warm_storage_read_cost;
table[OP_CALLCODE] = warm_storage_read_cost;
table[OP_DELEGATECALL] = warm_storage_read_cost;
table[OP_STATICCALL] = warm_storage_read_cost;
table[OP_SLOAD] = warm_storage_read_cost;
return table;
}();

} // namespace evmone::instr
101 changes: 78 additions & 23 deletions lib/evmone/instructions.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
#pragma once

#include "execution_state.hpp"
#include "instruction_traits.hpp"
#include <ethash/keccak.hpp>
#include <evmc/instructions.h>

namespace evmone
{
Expand Down Expand Up @@ -279,10 +279,19 @@ inline void address(ExecutionState& state) noexcept
state.stack.push(intx::be::load<uint256>(state.msg->destination));
}

inline void balance(ExecutionState& state) noexcept
inline evmc_status_code balance(ExecutionState& state) noexcept
{
auto& x = state.stack.top();
x = intx::be::load<uint256>(state.host.get_balance(intx::be::trunc<evmc::address>(x)));
const auto addr = intx::be::trunc<evmc::address>(x);

if (state.rev >= EVMC_BERLIN && state.host.access_account(addr) == EVMC_ACCESS_COLD)
{
if ((state.gas_left -= instr::additional_cold_account_access_cost) < 0)
return EVMC_OUT_OF_GAS;
}

x = intx::be::load<uint256>(state.host.get_balance(addr));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@yperbasis yperbasis Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return EVMC_SUCCESS;
}

inline void origin(ExecutionState& state) noexcept
Expand Down Expand Up @@ -394,10 +403,19 @@ inline void gasprice(ExecutionState& state) noexcept
state.stack.push(intx::be::load<uint256>(state.host.get_tx_context().tx_gas_price));
}

inline void extcodesize(ExecutionState& state) noexcept
inline evmc_status_code extcodesize(ExecutionState& state) noexcept
{
auto& x = state.stack.top();
x = state.host.get_code_size(intx::be::trunc<evmc::address>(x));
const auto addr = intx::be::trunc<evmc::address>(x);

if (state.rev >= EVMC_BERLIN && state.host.access_account(addr) == EVMC_ACCESS_COLD)
{
if ((state.gas_left -= instr::additional_cold_account_access_cost) < 0)
return EVMC_OUT_OF_GAS;
}

x = state.host.get_code_size(addr);
return EVMC_SUCCESS;
}

inline evmc_status_code extcodecopy(ExecutionState& state) noexcept
Expand All @@ -418,6 +436,12 @@ inline evmc_status_code extcodecopy(ExecutionState& state) noexcept
if ((state.gas_left -= copy_cost) < 0)
return EVMC_OUT_OF_GAS;

if (state.rev >= EVMC_BERLIN && state.host.access_account(addr) == EVMC_ACCESS_COLD)
{
if ((state.gas_left -= instr::additional_cold_account_access_cost) < 0)
return EVMC_OUT_OF_GAS;
}

auto data = s != 0 ? &state.memory[dst] : nullptr;
auto num_bytes_copied = state.host.copy_code(addr, src, data, s);
if (s - num_bytes_copied > 0)
Expand Down Expand Up @@ -460,10 +484,19 @@ inline evmc_status_code returndatacopy(ExecutionState& state) noexcept
return EVMC_SUCCESS;
}

inline void extcodehash(ExecutionState& state) noexcept
inline evmc_status_code extcodehash(ExecutionState& state) noexcept
{
auto& x = state.stack.top();
x = intx::be::load<uint256>(state.host.get_code_hash(intx::be::trunc<evmc::address>(x)));
const auto addr = intx::be::trunc<evmc::address>(x);

if (state.rev >= EVMC_BERLIN && state.host.access_account(addr) == EVMC_ACCESS_COLD)
{
if ((state.gas_left -= instr::additional_cold_account_access_cost) < 0)
return EVMC_OUT_OF_GAS;
}

x = intx::be::load<uint256>(state.host.get_code_hash(addr));
return EVMC_SUCCESS;
}


Expand Down Expand Up @@ -561,11 +594,25 @@ inline evmc_status_code mstore8(ExecutionState& state) noexcept
return EVMC_SUCCESS;
}

inline void sload(ExecutionState& state) noexcept
inline evmc_status_code sload(ExecutionState& state) noexcept
{
auto& x = state.stack.top();
x = intx::be::load<uint256>(
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 &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

state.host.access_storage(state.msg->destination, key) == EVMC_ACCESS_COLD)
{
// The warm storage access cost is already applied (from the cost table).
// Here we need to apply additional cold storage access cost.
constexpr auto additional_cold_sload_cost =
instr::cold_sload_cost - instr::warm_storage_read_cost;
if ((state.gas_left -= additional_cold_sload_cost) < 0)
return EVMC_OUT_OF_GAS;
}

x = intx::be::load<uint256>(state.host.get_storage(state.msg->destination, key));

return EVMC_SUCCESS;
}

inline evmc_status_code sstore(ExecutionState& state) noexcept
Expand All @@ -578,34 +625,36 @@ inline evmc_status_code sstore(ExecutionState& state) noexcept

const auto key = intx::be::store<evmc::bytes32>(state.stack.pop());
const auto value = intx::be::store<evmc::bytes32>(state.stack.pop());
const auto status = state.host.set_storage(state.msg->destination, key, value);

int cost = 0;
if (state.rev >= EVMC_BERLIN &&
state.host.access_storage(state.msg->destination, key) == EVMC_ACCESS_COLD)
cost = instr::cold_sload_cost;

const auto status = state.host.set_storage(state.msg->destination, key, value);

switch (status)
{
case EVMC_STORAGE_UNCHANGED:
if (state.rev >= EVMC_ISTANBUL)
case EVMC_STORAGE_MODIFIED_AGAIN:
if (state.rev >= EVMC_BERLIN)
cost += instr::warm_storage_read_cost;
else if (state.rev == EVMC_ISTANBUL)
cost = 800;
else if (state.rev == EVMC_CONSTANTINOPLE)
cost = 200;
else
cost = 5000;
break;
case EVMC_STORAGE_MODIFIED:
cost = 5000;
break;
case EVMC_STORAGE_MODIFIED_AGAIN:
if (state.rev >= EVMC_ISTANBUL)
cost = 800;
else if (state.rev == EVMC_CONSTANTINOPLE)
cost = 200;
case EVMC_STORAGE_DELETED:
if (state.rev >= EVMC_BERLIN)
cost += 5000 - instr::cold_sload_cost;
else
cost = 5000;
break;
case EVMC_STORAGE_ADDED:
cost = 20000;
break;
case EVMC_STORAGE_DELETED:
cost = 5000;
cost += 20000;
break;
}
if ((state.gas_left -= cost) < 0)
Expand Down Expand Up @@ -680,6 +729,12 @@ inline evmc_status_code selfdestruct(ExecutionState& state) noexcept

const auto beneficiary = intx::be::trunc<evmc::address>(state.stack[0]);

if (state.rev >= EVMC_BERLIN && state.host.access_account(beneficiary) == EVMC_ACCESS_COLD)
{
if ((state.gas_left -= instr::cold_account_access_cost) < 0)
return EVMC_OUT_OF_GAS;
}

if (state.rev >= EVMC_TANGERINE_WHISTLE)
{
if (state.rev == EVMC_TANGERINE_WHISTLE || state.host.get_balance(state.msg->destination))
yperbasis marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
6 changes: 6 additions & 0 deletions lib/evmone/instructions_calls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ evmc_status_code call(ExecutionState& state) noexcept

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

if (state.rev >= EVMC_BERLIN && state.host.access_account(dst) == EVMC_ACCESS_COLD)
{
if ((state.gas_left -= instr::additional_cold_account_access_cost) < 0)
return EVMC_OUT_OF_GAS;
}

if (!check_memory(state, input_offset, input_size))
return EVMC_OUT_OF_GAS;

Expand Down
1 change: 1 addition & 0 deletions test/unittests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ add_executable(evmone-unittests
evm_fixture.hpp
evm_test.cpp
evm_calls_test.cpp
evm_eip2929_test.cpp
evm_state_test.cpp
evm_other_test.cpp
evmone_test.cpp
Expand Down