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

Add support for EXTCODEHASH #4676

Merged
merged 5 commits into from
Sep 27, 2018
Merged

Add support for EXTCODEHASH #4676

merged 5 commits into from
Sep 27, 2018

Conversation

jwasinger
Copy link
Contributor

@jwasinger jwasinger commented Aug 3, 2018

Implementation for EXTCODEHASH opcode support in Solidity. For now, this seems to work with LLL assembly, which is what I am using to write some EXTCODEHASH state tests.

Implements https://eips.ethereum.org/EIPS/eip-1052

@codecov
Copy link

codecov bot commented Aug 3, 2018

Codecov Report

Merging #4676 into develop will decrease coverage by <.01%.
The diff coverage is 76.92%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4676      +/-   ##
===========================================
- Coverage    87.92%   87.91%   -0.01%     
===========================================
  Files          314      314              
  Lines        31766    31779      +13     
  Branches      3748     3749       +1     
===========================================
+ Hits         27930    27940      +10     
- Misses        2572     2575       +3     
  Partials      1264     1264
Flag Coverage Δ
#all 87.91% <76.92%> (-0.01%) ⬇️
#syntax 28.76% <7.69%> (-0.01%) ⬇️

@@ -82,6 +82,7 @@ enum class Instruction: uint8_t
EXTCODECOPY, ///< copy external code (from another contract)
RETURNDATASIZE = 0x3d, ///< get size of return data buffer
RETURNDATACOPY = 0x3e, ///< copy return data in current environment to memory
EXTCODEHASH = 0x3f,
Copy link
Member

Choose a reason for hiding this comment

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

Needs description.

@@ -215,6 +216,7 @@ static const std::map<Instruction, InstructionInfo> c_instructionInfo =
{ Instruction::EXTCODECOPY, { "EXTCODECOPY", 0, 4, 0, true, Tier::ExtCode } },
{ Instruction::RETURNDATASIZE, {"RETURNDATASIZE", 0, 0, 1, false, Tier::Base } },
{ Instruction::RETURNDATACOPY, {"RETURNDATACOPY", 0, 3, 0, true, Tier::VeryLow } },
{ Instruction::EXTCODESIZE, { "EXTCODEHASH", 0, 1, 1, false, Tier::ExtCode } },
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be wrong?

@@ -112,6 +112,7 @@ GasMeter::GasConsumption GasMeter::estimateMax(AssemblyItem const& _item, bool _
gas += wordGas(GasCosts::copyGas, m_state->relativeStackElement(-2));
break;
case Instruction::EXTCODESIZE:
case Instruction::EXTCODEHASH:
gas = GasCosts::extCodeGas(m_evmVersion);
Copy link
Member

Choose a reason for hiding this comment

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

Is this really charged the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

Most likely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I'm sorry, - it seems to be the same as Instruction::BALANCE.

@axic
Copy link
Member

axic commented Aug 4, 2018

Needs a changelog entry and tests too (see tests for create2).

@chriseth
Copy link
Contributor

Will take this over.

@chriseth
Copy link
Contributor

chriseth commented Aug 16, 2018

Is it accepted for constantinople in the same way as shift instructions are?

@axic
Copy link
Member

axic commented Sep 26, 2018

@chriseth rebase and fixed this.

@axic axic force-pushed the extcodehash branch 2 times, most recently from 155e783 to 054932b Compare September 26, 2018 13:54
axic
axic previously approved these changes Sep 26, 2018
@axic
Copy link
Member

axic commented Sep 26, 2018

@chriseth I think this is fine to merge. (Also needed for creating tests in github.com/ethereum/tests)

@axic
Copy link
Member

axic commented Sep 26, 2018

@chriseth added documentation

| | | | keccak256(<address> . n . keccak256(mem[p...(p+s))) and send v |
| | | | wei and return the new address |
| | | | keccak256(0xff . <address> . n . keccak256(mem[p...(p+s))) |
| | | | and send v wei and return the new address |
Copy link
Member

Choose a reason for hiding this comment

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

This was missed in #5013.

| create(v, p, s) | | F | create new contract with code mem[p...(p+s)) and send v wei |
| | | | and return the new address |
+-------------------------+-----+---+-----------------------------------------------------------------+
| create2(v, n, p, s) | | C | create new contract with code mem[p...(p+s)) at address |
| | | | keccak256(<address> . n . keccak256(mem[p...(p+s))) and send v |
| | | | wei and return the new address |
| | | | keccak256(0xff . <address> . n . keccak256(mem[p...(p+s))) |
Copy link
Contributor

Choose a reason for hiding this comment

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

could use u256(n) to clarify padding?

Copy link
Member

Choose a reason for hiding this comment

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

Started doing that but ran into a lot more to explain. Will prepare a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants