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

Don't hash the init_code of CREATE. #9688

Merged
merged 1 commit into from
Oct 4, 2018
Merged

Don't hash the init_code of CREATE. #9688

merged 1 commit into from
Oct 4, 2018

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Oct 2, 2018

It's usually wasteful to cache it anyway, since it's rarely re-used. Also for large init codes it hashing might be slower then jump destination computation anyway.

Similar PR for geth: ethereum/go-ethereum#17806

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 2, 2018
Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. I think it would be fine, but I'm not really really certain about the performance impact -- we trade off a keccak over recalculation of jump destinations, and this impacts all CREATE opcodes and contract creation transactions.

  • Previously, for create executions, we would re-calculate keccak(init_code) at every jump opcode, but cache jump destinations.
  • After this PR, for create executions, we would re-calculate jump destinations at every jump opcode. The assumption here is that because keccak itself needs iteration of the init_code anyway, it must be more expensive compared with calculating jump destinations.

Out of scope of this PR, there's indeed a way to cache jump destinations without calculating keccak(init_code) -- by not using SharedCache, but per-instance local cache for current code if ActionParams::code_hash is None.


if let Some(d) = self.jump_destinations.lock().get_mut(code_hash) {
return d.0.clone();
if let Some(d) = self.jump_destinations.lock().get_mut(code_hash) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not caused by this PR, but I think here we can use get instead of get_mut?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a MemoryLruCache, get_mut is used to update the LRU list.

@holiman
Copy link
Contributor

holiman commented Oct 3, 2018

but per-instance local cache

That's a pretty good idea, actually. Probably a must-have, even

@holiman
Copy link
Contributor

holiman commented Oct 3, 2018

I added that to the geth PR. I originally intended to have it, but forgot it :)

Copy link
Collaborator Author

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

this impacts all CREATE opcodes and contract creation transactions.

It only impacts CREATE, not CREATE2. For CREATE2 we re-use the code_hash generated during contract address calculation.

by not using SharedCache, but per-instance local cache for current cod

We have a local cache for that here:
https://github.com/paritytech/parity-ethereum/pull/9688/files#diff-f24406c9371a7a621c14519c77ce4bddR387

So we didn't and don't calculate hash nor jump destinations on every JUMP opcode, it was done only one within a particular execution context.


if let Some(d) = self.jump_destinations.lock().get_mut(code_hash) {
return d.0.clone();
if let Some(d) = self.jump_destinations.lock().get_mut(code_hash) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a MemoryLruCache, get_mut is used to update the LRU list.

@5chdn 5chdn added this to the 2.2 milestone Oct 3, 2018
@5chdn 5chdn added P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible. B1-patch-beta 🕷 labels Oct 3, 2018
@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 4, 2018
@debris debris merged commit 4186467 into master Oct 4, 2018
@debris debris deleted the td-jumpdestinations branch October 4, 2018 11:32
@5chdn 5chdn mentioned this pull request Oct 8, 2018
28 tasks
5chdn pushed a commit that referenced this pull request Oct 8, 2018
dvdplm added a commit that referenced this pull request Oct 9, 2018
…mon-deps

* origin/master:
  fix (light/provider) : Make `read_only executions` read-only (#9591)
  ethcore: fix detection of major import (#9552)
  return 0 on error (#9705)
  ethcore: delay ropsten hardfork (#9704)
  make instantSeal engine backwards compatible, closes #9696 (#9700)
  Implement CREATE2 gas changes and fix some potential overflowing (#9694)
  Don't hash the init_code of CREATE. (#9688)
  ethcore: minor optimization of modexp by using LR exponentiation (#9697)
  removed redundant clone before each block import (#9683)
andresilva pushed a commit that referenced this pull request Oct 9, 2018
5chdn added a commit that referenced this pull request Oct 9, 2018
* parity-version: bump beta to 2.1.2

* docs(rpc): push the branch along with tags (#9578)

* docs(rpc): push the branch along with tags

* ci: remove old rpc docs script

* Remove snapcraft clean (#9585)

* Revert " add snapcraft package image (master) (#9584)"

This reverts commit ceaedbb.

* Update package-snap.sh

* Update .gitlab-ci.yml

* ci: fix regex 🙄 (#9597)

* docs(rpc): annotate tag with the provided message (#9601)

* Update ropsten.json (#9602)

* HF in POA Sokol (2018-09-19) (#9607)

poanetwork/poa-chain-spec#86

* fix(network): don't disconnect reserved peers (#9608)

The priority of && and || was borked.

* fix failing node-table tests on mac os, closes #9632 (#9633)

* ethcore-io retries failed work steal (#9651)

* ethcore-io uses newer version of crossbeam && retries failed work steal

* ethcore-io non-mio service uses newer crossbeam

* remove master from releasable branches (#9655)

* remove master from releasable branches

need backporting in beta 
fix https://gitlab.parity.io/parity/parity-ethereum/-/jobs/101065 etc

* add except for snap packages for master

* Test fix for windows cache name... (#9658)

* Test fix for windows cache name...

* Fix variable name.

* fix(light_fetch): avoid race with BlockNumber::Latest (#9665)

* Calculate sha3 instead of sha256 for push-release. (#9673)

* Calculate sha3 instead of sha256 for push-release.

* Add pushes to the script.

* 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

* ci: fix push script (#9679)

* ci: fix push script

* Fix copying & running on windows.

* CI: Remove unnecessary pipes (#9681)

* ci: reduce gitlab pipelines significantly

* ci: build pipeline for PR

* ci: remove dead weight

* ci: remove github release script

* ci: remove forever broken aura tests

* ci: add random stuff to the end of the pipes

* ci: add wind and mac to the end of the pipe

* ci: remove snap artifacts

* ci: (re)move dockerfiles

* ci: clarify job names

* ci: add cargo audit job

* ci: make audit script executable

* ci: ignore snap and docker files for rust check

* ci: simplify audit script

* ci: rename misc to optional

* ci: add publish script to releaseable branches

* ci: more verbose cp command for windows build

* ci: fix weird binary checksum logic in push script

* ci: fix regex in push script for windows

* ci: simplify gitlab caching

* docs: align README with ci changes

* ci: specify default cargo target dir

* ci: print verbose environment

* ci: proper naming of scripts

* ci: restore docker files

* ci: use docker hub file

* ci: use cargo home instead of cargo target dir

* ci: touch random rust file to trigger real builds

* ci: set cargo target dir for audit script

* ci: remove temp file

* ci: don't export the cargo target dir in the audit script

* ci: fix windows unbound variable

* docs: fix gitlab badge path

* rename deprecated gitlab ci variables

https://docs.gitlab.com/ee/ci/variables/#9-0-renaming

* ci: fix git compare for nightly builds

* test: skip c++ example for all platforms but linux

* ci: add random rust file to trigger tests

* ci: remove random rust file

* disable cpp lib test for mac, win and beta (#9686)

* cleanup ci merge

* ci: fix tests

* fix bad-block reporting no reason (#9638)

* ethcore: fix detection of major import (#9552)

* sync: set state to idle after sync is completed

* sync: refactor sync reset

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

* Docker: run as parity user (#9689)

* 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

* make instantSeal engine backwards compatible, closes #9696 (#9700)

* ethcore: delay ropsten hardfork (#9704)

* fix (light/provider) : Make `read_only executions` read-only (#9591)

* `ExecutionsRequest` from light-clients as read-only

This changes so all `ExecutionRequests` from light-clients are executed
as read-only which the `virtual``flag == true ensures.

This boost up the current transaction to always succeed

Note, this only affects `eth_estimateGas` and `eth_call` AFAIK.

* grumbles(revert renaming) : TransactionProof

* grumbles(trace) : remove incorrect trace

* grumbles(state/prove_tx) : explicit `virt`

Remove the boolean flag to determine that a `state::prove_transaction`
whether it should be executed in a virtual context or not.

Because of that also rename the function to
`state::prove_transction_virtual` to make more clear

* CI: Skip docs job for nightly (#9693)

* ci: force-tag wiki changes

* ci: force-tag wiki changes

* ci: skip docs job for master and nightly

* ci: revert docs job checking for nightly tag

* ci: exclude docs job from nightly builds in gitlab script
andresilva pushed a commit that referenced this pull request Oct 9, 2018
andresilva pushed a commit that referenced this pull request Oct 9, 2018
andresilva pushed a commit that referenced this pull request Oct 9, 2018
andresilva pushed a commit that referenced this pull request Oct 10, 2018
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. P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants