EIP-1283 #433
EIP-1283 #433
Conversation
ghost
assigned 
| defstruct cache: %{} | ||
|
|
||
| @type key_cache() :: %{ | ||
| integer() => %{current_value: integer() | :deleted, initial_value: integer() | nil} |
There was a problem hiding this comment.
while working on #416, I will try using ets tables as cache storage instead of simple maps
|
@ayrat555 do you think you could split the cache work and the EIP work into two separate PRs? It would make it easier to review. I'm not sure if that would be too difficult now or not. |
|
@germsvel I don't think so because for EIP-1283 we should have access to the current value. It's not possible without cache |
| @@ -85,18 +86,70 @@ defmodule EVM.Refunds do | |||
|
|
|||
| # `SSTORE` operations produce a refund when storage is set to zero from some non-zero value. | |||
| def refund(:sstore, [key, new_value], _machine_state, _sub_state, exec_env) do | |||
There was a problem hiding this comment.
Might be out of the scope of this PR/overkill but this is getting pretty complex. Do we want to pull it out into refunds/sstore.ex or something?
There was a problem hiding this comment.
A few comments, but overall it looks good to me. I was a looking at the EIP and saw some test cases presented there. What do you think of adding those as tests in mana? It's always helpful to verify the implementation is correct, especially with something like this where there seem to be lots of possible combinations of value and new_value, etc.
| @@ -2,13 +2,17 @@ defmodule Blockchain.Interface.AccountInterface do | |||
| @moduledoc """ | |||
| Defines an interface for methods to interact with contracts and accounts. | |||
| """ | |||
| alias Blockchain.Interface.AccountInterface.Cache | |||
There was a problem hiding this comment.
👍 I like that this is in its own module
| |> Enum.reduce(account_interface.state, fn {address, account_cache}, acc_state -> | ||
| account_cache | ||
| |> Map.to_list() | ||
| |> Enum.reduce(acc_state, fn {key, key_cache}, key_acc_state -> |
There was a problem hiding this comment.
This Enum.reduce within another Enum.reduce makes for complicated logic. What do you think about extracting this into a function? I think once it is extracted, we may even realize we can put part of this logic into the Cache module (though that's just a hunch, I may be wrong).
|
|
||
| case cached_value do | ||
| nil -> Account.get_storage(account_interface.state, address, key) | ||
| :deleted -> :key_not_found |
There was a problem hiding this comment.
We're returning :key_not_found to match the behavior of the Account.get_storage when a key isn't found, right? If so, I wonder if there's a better way to consolidate the knowledge of :key_not_found somewhere else? For example, maybe the Account module itself makes use of the cache if it finds one?
|
|
||
| alias Blockchain.Interface.AccountInterface.Cache | ||
|
|
||
| alias Blockchain.Interface.AccountInterface.Cache |
There was a problem hiding this comment.
I think this alias is duplicate. You have the same one on line 4
|
|
||
| describe "update_current_value/4" do | ||
| test "sets current_value" do | ||
| cache = %Cache{cache: %{}} |
There was a problem hiding this comment.
I don't think you need cache: %{} inside. That's the default for the Cache struct, right? You should be able to just do %Cache{}.
|
|
||
| describe "add_initial_value/4" do | ||
| test "sets initial_value" do | ||
| cache = %Cache{cache: %{}} |
There was a problem hiding this comment.
same comment here about not needing %{} inside since it's the default
| end | ||
|
|
||
| @spec get_initial_value(t(), Address.t(), integer()) :: integer() | nil | ||
| def get_initial_value(cache_struct, address, key) do |
There was a problem hiding this comment.
This is stylistic, so feel free to ignore, but what do you think of renaming these two functions that start with get_* into the same names without the get_*?
Cache.get_current_value -> Cache.current_value
Cache.get_initial_value -> Cache.initial_value
| if Configuration.eip1283_sstore_gas_cost_changed?(exec_env.config) do | ||
| eip1283_sstore_gas_cost([key, new_value], exec_env) | ||
| else | ||
| case ExecEnv.get_storage(exec_env, key) do |
There was a problem hiding this comment.
What do you think about extracting all this logic into a private function? That way you're dealing with the same level of abstraction in the operation_cost function. For example,
def operation_cost(:sstore, [key, new_value], _machine_state, exec_env) do
if Configuration.eip1283_sstore_gas_cost_changed?(exec_env.config) do
eip1283_sstore_gas_cost([key, new_value], exec_env)
else
# you might be able to think of a better name for this function
basic_sstore_gas_cost([key, new_value], exec_env)
end
| @@ -598,4 +602,32 @@ defmodule EVM.Gas do | |||
| {:original, original_cost} | |||
| end | |||
| end | |||
|
|
|||
| defp eip1283_sstore_gas_cost([key, new_value], exec_env) do | |||
| if Configuration.eip1283_sstore_gas_cost_changed?(exec_env.config) do | ||
| eip1283_sstore_refund([key, new_value], exec_env) | ||
| else | ||
| case ExecEnv.get_storage(exec_env, key) do |
There was a problem hiding this comment.
Love that we have extracted this into its own module. What do you think of extracting this case statement into a private function in this module so you have the same level of abstraction for the refund function?
Resolves #431
I created a cache for account storage modification in evm, So now we won't modify Merkle Patricia trie storage on sstore operations and only commit account storage changes on successful transaction execution ( for both message calls and contract creations).