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

EVM gas refunds not implemented #2497

Closed
dominicletz opened this issue Jun 6, 2019 · 4 comments
Labels

Comments

@dominicletz
Copy link

@dominicletz dominicletz commented Jun 6, 2019

Expected Behavior

Not sure if this is a design decision or just, not done yet. In EVM Yellow Paper there are scenarios for gas refunds defined. E.g. the SSTORE command should refund 15000 gas when a storage value is set to zero.
I'm trying to run the ethereum test suite (https://github.com/ethereum/tests/tree/develop/BlockchainTests/) against the eeevm and was expecting the gasUsed values to match.

Actual Behavior

Unfortunately the aeternity eeevm gasUsed per transaction is higher compared to reference correct EVM implementations. This does not allow passing the official EVM tests. The walletReorganizeOwners tests is a good example using SSTORE refunds (https://github.com/ethereum/tests/blob/develop/BlockchainTests/bcWalletTest/walletReorganizeOwners.json). In this test already the second block fails to match the expected gasUsed values against the aeternity eeevm.

Question

For my own project I need the EVM compliance and hence wanted to ask if this is something you would welcome as a contribution or something you have excluded by design?

Best

@nikita-fuchs

This comment has been minimized.

Copy link
Contributor

@nikita-fuchs nikita-fuchs commented Jun 7, 2019

Hey dominicletz, the gas pricing model in the aevm diverges vom the evm's on purpose, it corresponds to the figures which runtime analyses of the execution time of different operations made necessary, just like it was done within the evm' pricing model. What does your project need the figures be exactly the same for?

Besides that: Contributions are always welcome ! (as long it's not changing the gas pricing model to unfitting values ;) )

@dominicletz

This comment has been minimized.

Copy link
Author

@dominicletz dominicletz commented Jun 10, 2019

Changing the gas pricing makes some sense, unfortunately, not being able to "back-configure" it makes it impossible to pass all the Ethereum EVM tests. I wanted check whether all EVM functions are implemented & implemented correctly, but ran as described into this issue early on. The figures don't need to be exactly the same during runtime, and adjusting them just during the tests like in your test case here would be fine: https://github.com/aeternity/aeternity/blob/master/apps/aevm/test/aevm_eeevm_tests.erl

But the problem with refunds is that the codepaths seem to be missing to deal with them from the eeevm module, so it's not even possible to adjust just for the tests.

E.g. for SSTORE refunds I'm thinking of a change such as:

diff --git a/src/epoch/aevm_eeevm.erl b/src/epoch/aevm_eeevm.erl
index 194b1ac..fcefc29 100644
--- a/src/epoch/aevm_eeevm.erl
+++ b/src/epoch/aevm_eeevm.erl
@@ -1540,6 +1540,19 @@ spend_op_gas(?CALL, State) ->
 spend_op_gas(?CALLCODE, State) ->
     %% Delay this until the actual operation
     State;
+spend_op_gas(?SSTORE, State) ->
+    Us0 = aevm_eeevm_stack:peek(0, State),
+    Us1 = aevm_eeevm_stack:peek(1, State),
+    Old = aevm_eeevm_store:load(Us0, State),
+    Gastable = aevm_eeevm_state:gastable(State),
+
+    {Gas, Refund} = case (Us1 =/= 0) andalso (Old =:= 0) of
+        true  -> {maps:get('GSSET', Gastable), 0};   %% Additional storage is needed
+        false -> {maps:get('GSRESET', Gastable), maps:get('RSCLEAR', Gastable)}  %% Resetting a new value in the store.
+    end,
+    RefundState = aevm_eeevm_state:set_refund(aevm_eeevm_state:refund(State) + Refund, State),
+    spend_gas_common({op, ?SSTORE}, Gas, RefundState);
+
 spend_op_gas(Op, State) ->
     spend_gas_common({op, Op}, aevm_gas:op_gas(Op, State), State).

So you could still change have your own same gas pricing model, but it would allow to use the EVM reference gas prices when running the tests, and check the user refund values. But it requires introducing refund/1 and set_refund/2 into aevm_eeevm_state as well.

@evdoks

This comment has been minimized.

Copy link
Contributor

@evdoks evdoks commented Sep 19, 2019

@dominicletz Currently aeternity does not plan to maintain support for EVM. While it may be beneficial for helping developers with Ethereum background to transition to Aeternity infrastructure, we have decided to concentrate on our own VM - FATE (Fast æternity Transaction Engine) that, we believe, provides a safer in more efficient approach to execution of smart contracts.

@evdoks evdoks added the wontfix label Sep 19, 2019
@evdoks evdoks closed this Sep 19, 2019
@dominicletz

This comment has been minimized.

Copy link
Author

@dominicletz dominicletz commented Sep 23, 2019

Got ya, that's too bad though. We're keeping a fork of your aeevm for now here with changes to make it more EVM compliant: https://github.com/diodechain/diode_server_ex/tree/master/src/epoch
Depending on the effort, we might transplant this though for EVMC in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.