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

update eip-1014 according to coredev decision #1375

Merged
merged 13 commits into from
Oct 3, 2018

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Sep 3, 2018

This updates EIP 1014 with a third option for address calculation, the formula that was accepted at the coredev-call on Aug 10, 2018.
See #1014 (comment)

@eip-automerger
Copy link

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

  • EIP 1014 requires approval from one of (@vbuterin)

@axic
Copy link
Member

axic commented Sep 4, 2018

Shouldn't the different options be moved into the Rationale section, while leaving the final one under Specification ?

@axic
Copy link
Member

axic commented Sep 21, 2018

@Souptacular @5chdn @chfast is this the last version agreed on the call? If so, can this be merged?

@holiman holiman mentioned this pull request Sep 24, 2018
@@ -12,6 +12,8 @@ created: 2018-04-20

Adds a new opcode at 0xf5, which takes 4 stack arguments: endowment, memory_start, memory_length, salt. Behaves identically to CREATE, except using `sha3(msg.sender ++ salt ++ init_code)[12:]` instead of the usual sender-and-nonce-hash as the address where the contract is initialized at.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here still the wrong formula is cited.

It would also be good to have a short explanation of the difference between init_code and code added to the EIP.

@holgerd77
Copy link
Contributor

Some examples would be very helpful here for having the basic hash creation verified, optimally also providing both the complete contract code and the init code for the contract.

@holiman
Copy link
Contributor Author

holiman commented Sep 24, 2018

I agree. I'll try to update this PR with the comments from @axic (remove unused formulas) and some examples
Plus explain difference between code and initcode, plus reference EIP 684 regarding collisions, and also explain why single-tx delete+overwrite is impossible, due to nonce != 0 and selfdestruct cleanup only happens post execution

@holiman
Copy link
Contributor Author

holiman commented Sep 24, 2018

I have pushed some updates to this PR now. Please review (I still haven't added any examples though). cc @SergioDemianLerner

@axic
Copy link
Member

axic commented Sep 25, 2018

Can #1247 merged first?

@axic
Copy link
Member

axic commented Sep 27, 2018

Needs a rebase. Also should clarify how 0xff and salt are treated (endianess, width). @karalabe has raised these questions too, I think on the original PR.

@holgerd77
Copy link
Contributor

@holiman If you give a short answer on the question from @axic on 0xff and possible salt values, we or someone from the community might also be able to stick some first examples together which can be incorporated here.

@holiman
Copy link
Contributor Author

holiman commented Sep 28, 2018

Example 0

  • address 0x0000000000000000000000000000000000000000
  • salt 0x0000000000000000000000000000000000000000000000000000000000000000
  • code 0x00
  • result: 0x4D1A2e2bB4F88F0250f26Ffff098B0b30B26BF38

Example 1

  • address 0xdeadbeef00000000000000000000000000000000
  • salt 0x0000000000000000000000000000000000000000000000000000000000000000
  • code 0x00
  • result: 0xB928f69Bb1D91Cd65274e3c79d8986362984fDA3

Example 2

  • address 0xdeadbeef00000000000000000000000000000000
  • salt 0x000000000000000000000000feed000000000000000000000000000000000000
  • code 0x00
  • result: 0xD04116cDd17beBE565EB2422F2497E06cC1C9833

Example 3

  • address 0x0000000000000000000000000000000000000000
  • salt 0x0000000000000000000000000000000000000000000000000000000000000000
  • code 0xdeadbeef
  • result: 0x70f2b2914A2a4b783FaEFb75f459A580616Fcb5e

Example 4

  • address 0x00000000000000000000000000000000deadbeef
  • salt 0x00000000000000000000000000000000000000000000000000000000cafebabe
  • code 0xdeadbeef
  • result: 0x60f3f640a8508fC6a86d45DF051962668E1e8AC7

Example 5

  • address 0x00000000000000000000000000000000deadbeef
  • salt 0x00000000000000000000000000000000000000000000000000000000cafebabe
  • code 0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
  • result: 0x1d8bfDC5D46DC4f61D6b6115972536eBE6A8854C

@holiman
Copy link
Contributor Author

holiman commented Sep 28, 2018

Added examples, rebased, added comment about salt. I don't understand the question about endianness for 0xff ??

@holiman
Copy link
Contributor Author

holiman commented Sep 28, 2018

ping @vbuterin PTAL

Adds a new opcode at 0xf5, which takes 4 stack arguments: endowment, memory_start, memory_length, salt. Behaves identically to CREATE, except using `keccak256(msg.sender ++ salt ++ init_code)[12:]` instead of the usual sender-and-nonce-hash as the address where the contract is initialized at.
Adds a new opcode at 0xf5, which takes 4 stack arguments: endowment, memory_start, memory_length, salt. Behaves identically to CREATE, except using `keccak256( 0xff ++ address ++ salt ++ keccak256(init_code)))[12:]` instead of the usual sender-and-nonce-hash as the address where the contract is initialized at. The `salt` is a 32-byte stack item.

The coredev-call at 2018-08-10 decided to use the formula above.
Copy link
Member

Choose a reason for hiding this comment

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

Move it to Rationale?

EIPS/eip-1014.md Outdated

### Clarifications

The `initcode` is the code that, when executed, produces the runtime bytecode that will be placed into the state, and which typically is used by high level languages to implement a 'constructor'.
Copy link
Member

Choose a reason for hiding this comment

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

Should be init_code.

@zilm13 zilm13 mentioned this pull request Sep 28, 2018
5 tasks
@vbuterin
Copy link
Contributor

vbuterin commented Sep 28, 2018

I don't understand the question about endianness for 0xff ??

Yeah, me neither. 0xff is a single byte.

Salt is always 32 bytes.

Edit: this example from @holiman is correct.

Example 0

address 0x0000000000000000000000000000000000000000
salt 0x0000000000000000000000000000000000000000000000000000000000000000
code 0x00
result: 0x4D1A2e2bB4F88F0250f26Ffff098B0b30B26BF38

* Ensures that the hash preimage has a fixed size,

This also has the side-effect of being able to possibly reuse the `keccak256(init_code)` from earlier calculation, either within a client or via `EXTCODEHASH` if the init-code is deployed on-chain.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the init_code is not the code that gets installed into the smart contract. That's the difference between code and init_code.

Copy link
Contributor

Choose a reason for hiding this comment

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

So how it can improve EXTCODEHASH ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the thought was, that if it's a very large initcode, and the verification of a given contract needs to be cheap in gas, the init_code could be deployed on-chain.

If a contract A wants to verify a given contract address C, instead of feeding it the initcode and then force A do calculate the sha3(init_code), A could just use the EXTCODEHASH(D). Then A would know that yes, the initcode matches what's in D. Where D would be a piece of whitelisted init_code.

But yeah, maybe I should remove that. It was a 'hmm maybe this is useful for some scenario somewhere' kind of thing.

@SergioDemianLerner
Copy link
Contributor

SergioDemianLerner commented Sep 28, 2018

One issue that is still unaddressed is who pays the cost of hashing (if it turns out that it has significance compared to other costs)

It was raised by @holiman here:
#1014 (comment)

And I proposed a simple solution here: #1014 (comment)

@vbuterin
Copy link
Contributor

One issue that is still unaddressed from is who pays the cost of hashing (if it turns out that it has significance compared to other costs)

Ah I see; yes, this is genuinely an important point.

And I proposed a simple solution here: #1014 (comment)

That solution seems fine to me.

@axic
Copy link
Member

axic commented Sep 29, 2018

I don't understand the question about endianness for 0xff ??

Whether 0xff is an actual single byte or a stack item with the value of 0xff. In general I think the EIP is vague and could be improved regarding clarity.

@holiman
Copy link
Contributor Author

holiman commented Sep 29, 2018

Re the cost of hashing, I'll do some benchmarks. However, I think all initcode is already hashed today, for making a key to the lookup on valid-jumpdest-map. So that key can be reused, even though I suspect it may be is hashed twice right now (for geth I know it is)

@holgerd77
Copy link
Contributor

holgerd77 commented Sep 29, 2018

Hello @holiman Martin, having problems getting the examples to run, I am getting different hashes.

I assume the code field you provide in the examples is the initCode?

If so: can you rename this to initCode?
If not: how can I transform this? Can you still add some explanation of this to the EIP text?

Update with some contextual links:

@vbuterin
Copy link
Contributor

I'd support changing the first paragraph to:

Adds a new opcode at 0xf5, which takes 4 stack arguments: endowment, memory_start, memory_length, salt. Behaves identically to CREATE, except using keccak256( 0xff ++ address ++ salt ++ keccak256(init_code)))[12:] instead of the usual sender-and-nonce-hash as the address where the contract is initialized at. 0xff is included as a single byte, address always as exactly 20 bytes, salt always as exactly 32 bytes (so the data hashed is always exactly 85 bytes long).

Possibly add to the end:

Additionally, an extra GSHA3WORD * ceil(len(init_code) / 32) gas is charged.

@holgerd77
Copy link
Contributor

Update: ok, failing hash creation was (as most of the time 😄) a bug on the implementation side, now fixed this and can confirm that all examples are working.

I've created an example json file from these over on ethereumjs-util (currently still within an open PR) if someone want to reuse.

Nevertheless would pledge to rename code to init_code in the examples to avoid confusion. Also is there any reason for these mixed capital- and non-capital-letters in the result value definitions (I have now taken the lowercase version for our example json file)?

@holiman
Copy link
Contributor Author

holiman commented Sep 29, 2018

I'll

  • rename code to initcode
  • update text with @vbuterin's text

Re upper/lowercase, I printed it from golang which automatically checksums addresses on String().

I don't think we should add gascost for initcode until it's been discussed and ok:ed by allcoredevs.

Until then, I'll try to benchmark it and see if it's an issue

@holiman
Copy link
Contributor Author

holiman commented Sep 30, 2018

Fixed, ptal

@rmeissner
Copy link
Contributor

Since there are additional gas costs involved now, could the expected gas costs be part of the tests?

@holiman
Copy link
Contributor Author

holiman commented Oct 2, 2018

For reference - discussion here: https://gitter.im/ethereum/AllCoreDevs?at=5bb315de6e5a401c2d037971

I have now updated the PR again, to include cost per byte. Yes, that should be included in testcases, I guess...

@holiman
Copy link
Contributor Author

holiman commented Oct 2, 2018

Testcases updated

@holiman
Copy link
Contributor Author

holiman commented Oct 3, 2018

This PR has been sitting a long time, and the EIP that is "current" contains errors. I'm going to admin-merge this, since,

  • It seems to be approved, at least implicitly, but the eip author @vbuterin
  • The gascost changes seems to have been met with approval from the allcoredev gitter room
  • It's very bad to have a stale EIP that does not contain what the actual clients have implemented or should implement

@holiman holiman merged commit 1d50630 into ethereum:master Oct 3, 2018
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.

8 participants