Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Add EIP-1014 transition config flag #9268

Merged
merged 10 commits into from
Aug 31, 2018
Merged

Add EIP-1014 transition config flag #9268

merged 10 commits into from
Aug 31, 2018

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Aug 2, 2018

rel #8427

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 2, 2018
@sorpaas sorpaas added this to the 2.1 milestone Aug 2, 2018
@Tbaut
Copy link
Contributor

Tbaut commented Aug 2, 2018

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

Unfortunately I don't think the CREATE2 opcode from EIP1014 is compatible with EIP86. The opcode is represented by a different hexcode and the order of arguments in the stack is different.

@andresilva
Copy link
Contributor

Maybe the EIPs should be updated so that both interfaces are compatible?

@sorpaas
Copy link
Collaborator Author

sorpaas commented Aug 7, 2018

Hmm indeed. I apparently missed this important difference. I added a comment on ethereum/EIPs#1014. Let me put this on ice until the spec is cleared out.

@sorpaas sorpaas added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 7, 2018
@5chdn 5chdn mentioned this pull request Aug 7, 2018
6 tasks
@sorpaas
Copy link
Collaborator Author

sorpaas commented Aug 13, 2018

Per discussion during the last AllCoreDevs, the issue is that even if we keep EIP-86, it will need to be vastly changed. So it was decided to deprecate EIP-86:

  • I removed all EIP-86 related flags.
  • Null signed transaction functionalities are kept. We're still using null signed transactions some places in our code. In execution context, the old config eip86 is only used to know whether we should keep null signed address nonce unchanged. So I renamed it to keep_unsigned_nonce.
  • Comply the CREATE2 definition with the current EIP-1014 (Option 2). This also affects KIP-4, but I guess that's unreleased feature so it should be okay.

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Aug 13, 2018
@cheme cheme self-requested a review August 21, 2018 15:43
Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

The PR seems fine (until next change of the eip formula (I hope not), still a few minor points :

@sorpaas
Copy link
Collaborator Author

sorpaas commented Aug 22, 2018

we can try to avoid the call to nonce for our new opcode by changing contract_address function prototype a bit.

Can't think of a good way to refactor that right now. I think it would need to give a state reference to contract_address fn but that'll certainly not worth it. I'll keep thinking though. :)

if I understood correctly keep_unsigned_nonce is only here for deprecating a functionality in the future, but not related to this EIP

keep_unsigned_nonce is for null signed transaction, which was a part of EIP-86. I think that's a generally useful feature even though we don't want EIP-86 itself any more (for example, the old Casper spec needs null signed transaction). And yes, that change is not related to EIP-1014.

@sorpaas
Copy link
Collaborator Author

sorpaas commented Aug 24, 2018

Ooops! I apparently forgot to push the fix commit for @cheme's grumbles. Sorry!

@cheme
Copy link
Contributor

cheme commented Aug 24, 2018

I also forgot the 'grumble' tag, I am glad I understand correctly the keep_unsigned_once, seems like I do not have other comments.

@cheme cheme closed this Aug 24, 2018
@cheme cheme reopened this Aug 24, 2018
@cheme
Copy link
Contributor

cheme commented Aug 24, 2018

Sorry for the misclick (I accidently close the pr).

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM! Is the EIP discussion finalized? This PR implements the address derivation as sha3(0xff ++ msg.sender ++ salt ++ sha3(init_code))) which is different from the EIP but seems to be what's settled in the EIP issue for now. Do you think we should merge this PR or wait to make sure the discussion is over? Also, should we update pwasm create2 defition?

@sorpaas
Copy link
Collaborator Author

sorpaas commented Aug 29, 2018

Yeah I think let's just wait till this Friday's meeting (ethereum/pm#55), and merge this if there's no other issues.

With this PR, create2 also uses the sha3(0xff ++ msg.sender ++ salt ++ sha3(init_code))). I'll get the spec update done soon.

@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 30, 2018
@5chdn
Copy link
Contributor

5chdn commented Aug 30, 2018

Putting this on ice till Friday.

@sorpaas sorpaas merged commit caca3a8 into master Aug 31, 2018
@sorpaas sorpaas deleted the sp-eip1014 branch August 31, 2018 15:43
@andresilva andresilva removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Aug 31, 2018
dvdplm added a commit that referenced this pull request Sep 4, 2018
* master:
  evmbin: escape newlines in json errors (#9458)
  use kvdb-* and parity-snappy crates from crates.io (#9441)
  Add EIP-1014 transition config flag (#9268)
  add tags for runner selection of build-linux jobs (#9451)
  Remove unused BlockStatus::Pending (#9447)
  ci: only include local paths in coverage script (except target) (#9437)
  Add POA Networks: Core and Sokol (#9413)
  docker: install missing dependencies in arm target dockerfiles (#9436)
  Random small cleanups (#9423)
andresilva pushed a commit that referenced this pull request Oct 9, 2018
* Add EIP-1014 transition config flag

* Remove EIP-86 configs

* Change CREATE2 opcode index to 0xf5

* Move salt to the last item in the stack

* Change sendersaltandaddress scheme to comply with current EIP-1014

* Fix json configs

* Fix create2 test

* Fix deprecated comments
andresilva pushed a commit that referenced this pull request Oct 9, 2018
* Add EIP-1014 transition config flag

* Remove EIP-86 configs

* Change CREATE2 opcode index to 0xf5

* Move salt to the last item in the stack

* Change sendersaltandaddress scheme to comply with current EIP-1014

* Fix json configs

* Fix create2 test

* Fix deprecated comments
andresilva pushed a commit that referenced this pull request Oct 9, 2018
* Add EIP-1014 transition config flag

* Remove EIP-86 configs

* Change CREATE2 opcode index to 0xf5

* Move salt to the last item in the stack

* Change sendersaltandaddress scheme to comply with current EIP-1014

* Fix json configs

* Fix create2 test

* Fix deprecated comments
andresilva pushed a commit that referenced this pull request Oct 10, 2018
* Add EIP-1014 transition config flag

* Remove EIP-86 configs

* Change CREATE2 opcode index to 0xf5

* Move salt to the last item in the stack

* Change sendersaltandaddress scheme to comply with current EIP-1014

* Fix json configs

* Fix create2 test

* Fix deprecated comments
5chdn pushed a commit that referenced this pull request Oct 10, 2018
* ethash: implement EIP-1234 (#9187)

* Implement EIP-1052 (EXTCODEHASH) and fix several issues in state account cache (#9234)

* Implement EIP-1052 and fix several issues related to account cache

* Fix jsontests

* Merge two matches together

* Avoid making unnecessary Arc<Vec>

* Address grumbles

* Comply EIP-86 with the new definition (#9140)

* Comply EIP-86 with the new CREATE2 opcode

* Fix rpc compile

* Fix interpreter CREATE/CREATE2 stack pop difference

* Add unreachable! to fix compile

* Fix instruction_info

* Fix gas check due to new stack item

* Add new tests in executive

* Fix have_create2 comment

* Remove all unused references of eip86_transition and block_number

* Implement KIP4: create2 for wasm (#9277)

* Basic implementation for kip4

* Add KIP-4 config flags

* typo: docs fix

* Fix args offset

* Add tests for create2

* tests: evm

* Update wasm-tests and fix all gas costs

* Update wasm-tests

* Update wasm-tests and fix gas costs

* `gasleft` extern implemented for WASM runtime (kip-6) (#9357)

* Wasm gasleft extern added

* wasm_gasleft_activation_transition -> kip4_transition

* use kip-6 switch

* gasleft_panic -> gasleft_fail rename

* call_msg_gasleft test added and gas_left agustments because this openethereum/wasm-tests#52

* change .. to _

* fix comment for the have_gasleft param

* update tests (openethereum/wasm-tests@0edbf86)

* Add EIP-1014 transition config flag (#9268)

* Add EIP-1014 transition config flag

* Remove EIP-86 configs

* Change CREATE2 opcode index to 0xf5

* Move salt to the last item in the stack

* Change sendersaltandaddress scheme to comply with current EIP-1014

* Fix json configs

* Fix create2 test

* Fix deprecated comments

* EIP 1283: Net gas metering for SSTORE without dirty maps (#9319)

* Implement last_checkpoint_storage_at

* Add reverted_storage_at for externalities

* sstore_clears_count -> sstore_clears_refund

* Implement eip1283 for evm

* Add eip1283Transition params

* evm: fix tests

* jsontests: fix test

* Return checkpoint index when creating

* Comply with spec Version II

* Fix docs

* Fix jsontests feature compile

* Address grumbles

* Fix no-checkpoint-entry case

* Remove unnecessary expect

* Add test for State::checkpoint_storage_at

* Add executive level test for eip1283

* Hard-code transaction_checkpoint_index to 0

* Fix jsontests

* Add tests for checkpoint discard/revert

* Require checkpoint to be empty for kill_account and commit

* Get code coverage

* Use saturating_add/saturating_sub

* Fix issues in insert_cache

* Clear the state again

* Fix original_storage_at

* Early return for empty RLP trie storage

* Update comments

* Fix borrow_mut issue

* Simplify checkpoint_storage_at if branches

* Better commenting for gas handling code

* Address naming grumbles

* More tests

* Fix an issue in overwrite_with

* Add another test

* Fix comment

* Remove unnecessary bracket

* Move orig to inner if

* Remove test coverage for this PR

* Add tests for executive original value

* Add warn! for an unreachable cause

* Update state tests execution model (#9440)

* Update & fix JSON state tests.

* Update tests to be able to run ethtest at
021fe3d410773024cd5f0387e62db6e6ec800f32.

- Touch user in state
- Adjust transaction tests to new json format

* Switch to same commit for submodule ethereum/test as geth (next includes constantinople changes).
Added test `json_tests::trie::generic::TrieTests_trieanyorder` and a few
difficulty tests.

* Remove trietestnextprev as it would require to parse differently and
implement it.

* Support new (shitty) format of transaction tests.

* Ignore junk in ethereum/tests repo.

* Ignore incorrect test.

* Update to a later commit

* Move block number to a constant.

* Fix ZK2 test - touched account should also be cleared.

* Fix conflict resolution

* Fix checkpointing when creating contract failed (#9514)

* In create memory calculation is the same for create2 because the additional parameter was popped before. (#9522)

* Enable all Constantinople hard fork changes in constantinople_test.json (#9505)

* Enable all Constantinople hard fork changes in constantinople_test.json

* Address grumbles

* Remove EIP-210 activation

* 8m -> 5m

* Temporarily add back eip210 transition so we can get test passed

* Add eip210_test and remove eip210 transition from const_test

* Add constantinople conf to EvmTestClient. (#9570)

* Add constantinople conf to EvmTestClient.

* Skip some test to update submodule etheureum/tests submodule to latest.

* Put skipping 'under issue' test behind a feature.

* Change blockReward for const-test to pass ethereum/tests

* Update tests to new constantinple definition (change of reward at block
5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible)Update tests to new constantinple definition (change
of reward at block 5).
Switch 'reference' to string, that way we can include issues from others
repo (more flexible).

* Fix modexp and bn128_mul gas prices in chain config

* Changes `run_test_path` method to append its directory results (without
that it stop testing at the first file failure).
Add some missing tests.
Add skip for those (block create2 is one hundred percent false but on
hive we can see that geth and aleth got similar issue for this item).

* retab current.json

* Update reference to parity issue for failing tests.

* Hardfork the testnets (#9562)

* ethcore: propose hardfork block number 4230000 for ropsten

* ethcore: propose hardfork block number 9000000 for kovan

* ethcore: enable kip-4 and kip-6 on kovan

* etcore: bump kovan hardfork to block 9.2M

* ethcore: fix ropsten constantinople block number to 4.2M

* ethcore: disable difficulty_test_ropsten until ethereum/tests are updated upstream

* Don't hash the init_code of CREATE. (#9688)

* Implement CREATE2 gas changes and fix some potential overflowing (#9694)

* Implement CREATE2 gas changes and fix some potential overflowing

* Ignore create2 state tests

* Split CREATE and CREATE2 in gasometer

* Generalize rounding (x + 31) / 32 to to_word_size

* ethcore: delay ropsten hardfork (#9704)

* Add hardcoded headers (#9730)

* add foundation hardcoded header #6486017

* add ropsten hardcoded headers #4202497

* add kovan hardcoded headers #9023489

* gitlab ci: releasable_branches: change variables condition to schedule (#9729)

* HF in POA Core (2018-10-22) (#9724)

poanetwork/poa-chain-spec#87
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants