Selfdestruct optimizations for Frontier and Homestead by halfalicious · Pull Request #5718 · ethereum/aleth

halfalicious · 2019-08-16T04:11:42Z

Since self-destruct operations have no gas cost on Frontier and Homestead, avoid checking the contract balance and destination account existence when executing a self-destruct in both aleth-interpreter and LegacyVM.

codecov-io · 2019-08-16T04:27:21Z

Codecov Report

Merging #5718 into master will increase coverage by <.01%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##           master    #5718      +/-   ##
==========================================
+ Coverage    63.2%   63.21%   +<.01%     
==========================================
  Files         353      353              
  Lines       30271    30272       +1     
  Branches     3390     3391       +1     
==========================================
+ Hits        19134    19135       +1     
+ Misses       9904     9900       -4     
- Partials     1233     1237       +4

chfast · 2019-08-16T13:13:44Z

libaleth-interpreter/VM.cpp

-                    m_runGas += VMSchedule::callNewAccount;
+                // After EIP158 zero-value suicides do not have to pay account creation gas.
+                u256 const balance =
+                    fromEvmC(m_context->host->get_balance(m_context, &m_message->destination));


This is still "wrong", because get_balance() will be executed in case of m_rev == EVMC_TANGERINE_WHISTLE where its value does not matter.

@chfast Ah thought that the changes should apply to Homestead / Frontier, I'll take another look at which hard forks the EIPs were enabled on. According to this they were both enabled on Tangerine Whistle but that's not right if we don't care about the balance on Tangerine Whistle: https://ethereum.stackexchange.com/questions/13014/please-provide-a-summary-of-the-ethereum-hard-forks

No need to check external sources. The logic here is correct. Just if you check the if at https://github.com/ethereum/aleth/pull/5718/files#diff-4a2862de6e660dc5045835d8879be719R389 you will see that:

you can swap the checks in ||.

The balance has be checked only if the first condition is false.

@chfast I'm confused...the line that you're referring to comes after balance retrieval so changing the checks will have no impact on whether or not we check the balance on Tangerine Whistle?

Line in question:

aleth/libaleth-interpreter/VM.cpp

Line 389 in fcb1f3d

if (balance > 0 || m_rev == EVMC_TANGERINE_WHISTLE)

@chfast I'm still a little confused about this part of the aleth-interpreter changes:

aleth/libaleth-interpreter/VM.cpp

Lines 383 to 388 in 29ed24e

// Self-destructs only have gas cost starting with Tangerine Whistle

if (m_rev >= EVMC_TANGERINE_WHISTLE)

{

// After EIP158 zero-value suicides do not have to pay account creation gas.

if (m_rev == EVMC_TANGERINE_WHISTLE ||

fromEvmC(m_context->host->get_balance(m_context, &m_message->destination)) > 0)

The check for EVMC_TANGERINE_WHISTLE on line 387 implies that on that hard fork, all suicides (0-value or otherwise) have an account-creation charge. This implies that EIP158 (which removes the account-creation charge for 0-value selfdestructs) was included in Spurious Dragon, but the following says that it was included in Tangerine Whistle: https://ethereum.stackexchange.com/questions/13014/please-provide-a-summary-of-the-ethereum-hard-forks

So to summarize, EIP158 was included in Spurious Dragon and not Tangerine Whistle?

EIP-158 was activated at block 2'675'000 which is Spurious Dragon

aleth/libethashseal/genesis/mainNetwork.cpp

Line 29 in 7e6095d

"EIP158ForkBlock": "0x28d138",

in EIPs actually EIP-161 supercedes EIP-158 and is listed in meta-EIP https://eips.ethereum.org/EIPS/eip-607

@gumb0 Thanks for the info! Is there a reliable list that you can recommend of hard forks + the block numbers that they occurred at?

I think meta-EIPs should correctly reflect the reality of mainnet https://eips.ethereum.org/meta

@gumb0 Perfect, that's exactly what I was looking for!

libaleth-interpreter/VM.cpp

halfalicious · 2019-08-24T05:05:21Z

cc @chfast / @gumb0

gumb0 · 2019-08-26T15:26:01Z

libevm/LegacyVM.cpp

+            // Starting with EIP150, self-destructs need to pay both gas cost and account creation
+            // gas cost. Starting with EIP158, 0-value self-destructs don't need to pay this account
+            // creation cost.
+            if (m_schedule->zeroValueTransferChargesNewAccountGas() ||


aleth-interpreter part is quite clear, but I'm not the fan of the change in LegacyVM.

I think we can assume that eip158 is always activated not earlier than eip150, and then simplify it to very similar to interpterter's version:

if (m_schedule->suicideChargesNewAccountGas()) { if (m_schedule->zeroValueTransferChargesNewAccountGas() || m_ext->balance(m_ext->myAddress) > 0) { if (!m_ext->exists(dest)) m_runGas += m_schedule->callNewAccountGas; } }

Maybe add assert(m_schedule->zeroValueTransferChargesNewAccountGas() || m_schedule->suicideChargesNewAccountGas()) - this was your first condition, and it should be always true when eip158 comes after eip150

@gumb0 / @chfast : After taking another look at this I don't like the feature-style checks in this opcode...I think they end up making things more confusing since they add a layer of indirection between our test case and the EIPs being checked. I think that a simpler / more intuitive approach would be something like this:

// Starting with EIP150, self-destructs need to pay both gas cost and account creation // gas cost. Starting with EIP158, 0-value self-destructs don't need to pay this account // creation cost. if (m_schedule->eip150Mode || (m_schedule->eip158Mode && m_ext->balance(m_ext->myAddress) > 0)) { if (!m_ext->exists(dest)) m_runGas += m_schedule->callNewAccountGas; }

This assumes that m_schedule->eip150Mode will not be true for the Spurious Dragon schedule.

Agree that using eips would be simpler, but

This assumes that m_schedule->eip150Mode will not be true for the Spurious Dragon schedule.

this assumption is wrong, because eip150Mode is true for Tangerine Whistle and further, so including Spurious Dragon.

So I think it should be

if (m_schedule->eip150Mode && (!m_schedule->eip158Mode || m_ext->balance(m_ext->myAddress) > 0)) { if (!m_ext->exists(dest)) m_runGas += m_schedule->callNewAccountGas; }

or leave it as 3 nested ifs as I suggested above similar to interpreter version.

chfast

Still ok for me.

chfast · 2019-08-27T08:36:25Z

libaleth-interpreter/VM.cpp

                throwDisallowedStateChange();

+            // Self-destructs only have gas cost starting with Tangerine Whistle
            m_runGas = m_rev >= EVMC_TANGERINE_WHISTLE ? 5000 : 0;


This line is gone. Please rebase.

halfalicious · 2019-08-28T03:17:25Z

Rebased

halfalicious · 2019-08-28T03:46:17Z

cc @gumb0 / @chfast

Update balance/account checks for SUICIDE opcode in aleth-interpreter and LegacyVM so that they are only performed when necessary.

halfalicious · 2019-08-28T03:56:01Z

Squashed all of the code commits together since I don't think 5 commits is necessary for such a small set of changes.

halfalicious force-pushed the selfdestruct-optimizations branch from 26866ed to 6b639ee Compare August 16, 2019 04:16

halfalicious requested review from chfast and gumb0 and removed request for chfast August 16, 2019 04:23

halfalicious force-pushed the selfdestruct-optimizations branch from 6b639ee to fcb1f3d Compare August 16, 2019 04:27

chfast suggested changes Aug 16, 2019

View reviewed changes

chfast approved these changes Aug 19, 2019

View reviewed changes

gumb0 reviewed Aug 23, 2019

View reviewed changes

libaleth-interpreter/VM.cpp Outdated Show resolved Hide resolved

gumb0 reviewed Aug 26, 2019

View reviewed changes

chfast approved these changes Aug 27, 2019

View reviewed changes

halfalicious force-pushed the selfdestruct-optimizations branch from 23b9e43 to 1aad58d Compare August 28, 2019 03:17

halfalicious force-pushed the selfdestruct-optimizations branch from c47376b to e701f4e Compare August 28, 2019 03:33

halfalicious force-pushed the selfdestruct-optimizations branch from e701f4e to e18ecad Compare August 28, 2019 03:52

halfalicious added 2 commits August 27, 2019 20:52

Avoid unnecessary suicide balance/account checks

b4d300d

Update balance/account checks for SUICIDE opcode in aleth-interpreter and LegacyVM so that they are only performed when necessary.

Update changelog

b565b8c

halfalicious force-pushed the selfdestruct-optimizations branch from e18ecad to b565b8c Compare August 28, 2019 03:55

Remove line not needed after rebase

00c643e

gumb0 approved these changes Aug 28, 2019

View reviewed changes

halfalicious merged commit c477606 into master Aug 28, 2019

	// Self-destructs only have gas cost starting with Tangerine Whistle
	if (m_rev >= EVMC_TANGERINE_WHISTLE)
	{
	// After EIP158 zero-value suicides do not have to pay account creation gas.
	if (m_rev == EVMC_TANGERINE_WHISTLE \|\|
	fromEvmC(m_context->host->get_balance(m_context, &m_message->destination)) > 0)

Conversation

halfalicious commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Uh oh!

halfalicious commented Aug 24, 2019

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!

chfast left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfalicious commented Aug 28, 2019

Uh oh!

halfalicious commented Aug 28, 2019

Uh oh!

halfalicious commented Aug 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

halfalicious commented Aug 16, 2019 •

edited

Loading

codecov-io commented Aug 16, 2019 •

edited

Loading