Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Implement EIP1380 (call-to-self) in aleth-interpreter #5753

Merged
merged 4 commits into from
Oct 4, 2019

Conversation

halfalicious
Copy link
Contributor

@halfalicious halfalicious commented Sep 21, 2019

No tests since @gumb0 mentioned that there's no way to measure gas cost in tests which use aleth-interpreter.

@halfalicious
Copy link
Contributor Author

Still need to create tests

@codecov-io
Copy link

codecov-io commented Sep 21, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@872f2ef). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5753   +/-   ##
=========================================
  Coverage          ?   64.03%           
=========================================
  Files             ?      359           
  Lines             ?    30774           
  Branches          ?     3416           
=========================================
  Hits              ?    19706           
  Misses            ?     9844           
  Partials          ?     1224

@halfalicious halfalicious force-pushed the eip1380-callself-interpreter branch from 62cc8ef to b7d86d5 Compare October 3, 2019 03:57
@halfalicious
Copy link
Contributor Author

Rebased

@halfalicious halfalicious requested review from gumb0 and chfast and removed request for gumb0 October 3, 2019 03:58
@halfalicious halfalicious marked this pull request as ready for review October 3, 2019 03:58
@halfalicious halfalicious force-pushed the eip1380-callself-interpreter branch from b7d86d5 to e73b50a Compare October 3, 2019 04:02
evmc_address const destination = toEvmC(asAddress(m_SP[1]));

// Check for call-to-self (eip1380) and adjust gas accordingly
if (fromEvmC(m_message->destination) == fromEvmC(destination) && m_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.

You need to have the cheap m_rev >= EVMC_BERLIN check first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chfast Ah good point, comparing addresses is expensive since you're comparing 32 byte boost multiprecision values which are also first converted from big endian, instead of an EVMC_REVISION which is an enum (4 bytes).

@halfalicious halfalicious force-pushed the eip1380-callself-interpreter branch from e73b50a to 906f7f2 Compare October 3, 2019 16:26
@halfalicious halfalicious force-pushed the eip1380-callself-interpreter branch from 906f7f2 to d843b9b Compare October 4, 2019 04:37
@halfalicious
Copy link
Contributor Author

halfalicious commented Oct 4, 2019

@chfast I also removed the fromEvmc calls here:

// Check for call-to-self (eip1380) and adjust gas accordingly
if (m_rev >= EVMC_BERLIN && m_message->destination == destination)

I think that's okay since these are both evmc_address objects so they can be compared (well technically destination is evmc::address which is a subclass of evmc_address but I don't think the subclass implements anything which would affect the comparison).

@gumb0
Copy link
Member

gumb0 commented Oct 4, 2019

@halfalicious I removed the assignment to m_runGas from the metrics table, because it's actually already done for each opcode in fetchInstruction()

@halfalicious
Copy link
Contributor Author

@halfalicious I removed the assignment to m_runGas from the metrics table, because it's actually already done for each opcode in fetchInstruction()

@gumb0 Ah I see, it’s done here:

m_runGas = metric.gas_cost;

Thanks for the heads up!

@halfalicious halfalicious merged commit 515bbc7 into master Oct 4, 2019
@halfalicious halfalicious deleted the eip1380-callself-interpreter branch October 4, 2019 16:25
@gumb0 gumb0 mentioned this pull request Oct 5, 2019
13 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants