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

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

Merged
halfalicious merged 4 commits intomasterfrom
eip1380-callself-interpreter
Oct 4, 2019
Merged

Implement EIP1380 (call-to-self) in aleth-interpreter#5753
halfalicious merged 4 commits intomasterfrom
eip1380-callself-interpreter

Conversation

@halfalicious
Copy link
Copy Markdown
Contributor

@halfalicious halfalicious commented Sep 21, 2019 •
edited
Loading

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

@halfalicious
Copy link
Copy Markdown
Contributor Author

halfalicious commented Sep 21, 2019

Still need to create tests

@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 21, 2019 •
edited
Loading

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
Copy Markdown
Contributor Author

halfalicious commented Oct 3, 2019

Rebased

@halfalicious halfalicious requested review from chfast and gumb0 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
chfast
chfast suggested changes Oct 3, 2019
View reviewed changes
libaleth-interpreter/VMCalls.cpp Outdated
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
Copy Markdown
Member

@chfast chfast Oct 3, 2019

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
Copy Markdown
Contributor Author

@halfalicious halfalicious Oct 3, 2019

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
Copy Markdown
Contributor Author

halfalicious commented Oct 4, 2019 •
edited
Loading

@chfast I also removed the fromEvmc calls here:

aleth/libaleth-interpreter/VMCalls.cpp

Lines 197 to 198 in d843b9b

// 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
Copy Markdown
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
Copy Markdown
Contributor Author

halfalicious 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()

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

aleth/libaleth-interpreter/VM.cpp

Line 220 in 06dad45

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
Open
13 tasks
@halfalicious halfalicious mentioned this pull request Oct 15, 2019
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@chfast chfast chfast approved these changes

@gumb0 gumb0 gumb0 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@halfalicious @codecov-io @gumb0 @chfast