EVM: Faster create/create2 by holiman · Pull Request #17806 · ethereum/go-ethereum

holiman · 2018-10-02T08:20:11Z

This PR improves CREATE and CREATE2.

For CREATE2, reuse the hash of the initcode for later
For CREATE, don't store the jumpdest map, instead always calculate the map if the codehash is not available. Calculating the map is a fairly trivial loop which is faster than sha, so doing a sha to lookup a map is wasteful.
It also adds a method, debug.traceBadBlock(int), which can be used to trace a block which did not make it into the chain, but is available internally in badBlocks.

karalabe

Generally seems good to me. My two main questions is:

Is it worthwhile to track *common.Hash, or could we simply use common.Hash{} as the nil value? The rest of our code does that, seems simpler/more consistent.
You pass codeAndHash around by value. However, calling Hash on it changes it's contents, which if you passed by value into a nested method, won't propagate out. This is not an issue in the current code, because you Hash on the topmost context, but this is imho dangerous. Please pass it as a pointer everywhere (also create it as such).

karalabe · 2018-10-03T11:11:30Z

 	self          ContractRef

-	jumpdests destinations // result of JUMPDEST analysis.
+	jumpdests destinations // Aggregated result of JUMPDEST analysis.


Could we drop the destinations typdef altogether? Previously we at least had a methof on it, but now it's just a map[common.Hash]bitvec. There's not much point really to keep a type just for this.

karalabe · 2018-10-03T11:16:08Z


 	Code     []byte
-	CodeHash common.Hash
+	CodeHash *common.Hash


Wouldn't it be the same to keep this as it was and instead of checking for nil, we check for == (common.Hash{})?

karalabe · 2018-10-03T11:19:13Z


+type codeAndHash struct {
+	code []byte
+	hash *common.Hash


Same here. Generally our code considers common.Hash{} to be the nil/uninited hash. Do we really need the pointers?

karalabe · 2018-10-03T11:19:37Z

+	codeAndHash := codeAndHash{code: code}
 	contractAddr = crypto.CreateAddress(caller.Address(), evm.StateDB.GetNonce(caller.Address()))
-	return evm.create(caller, code, gas, value, contractAddr)
+	return evm.create(caller, codeAndHash, gas, value, contractAddr)


You can probably inline codeAndHash{code: code} here., a bit shorter/cleaner (imho).

karalabe · 2018-10-03T13:54:49Z

+}
+
+func (c *codeAndHash) Hash() common.Hash {
+	if c.hash == emptyCodeHash {


This is not correct AFAIK. It should be common.Hash{}. The emptyCodeHash is Keccak256("").

karalabe · 2018-10-03T13:55:05Z

+	}
+	var analysis bitvec
+	// Do we have a contract hash already?
+	if c.CodeHash != emptyCodeHash {


This is not correct AFAIK. It should be common.Hash{}. The emptyCodeHash is Keccak256("").

karalabe · 2018-10-04T07:31:23Z

+	if OpCode(c.Code[udest]) != JUMPDEST {
+		return false
+	}
+	var analysis bitvec


You don't use this anywhere outside of the nested if below. You could delete it from here and instead of declaring exists explicitly below too, just do analysis, exist := c.jumpdests[c.CodeHash].

Did you want to use this for something and perhaps forgot?

You're right, it can be simplified. I iterated a bit on that loop, and it's probably some leftover from that

karalabe

LGTM

wuestholz · 2018-10-08T14:12:34Z

@holiman @karalabe I noticed that after this change my execution tracer behaves differently when accessing the code hash of a contract that is currently being created. For instance, in function CaptureState, contract.CodeHash (where contract is of type *vm.contract) now seems to return an empty hash. Is this the new intended behavior?

karalabe · 2018-10-08T14:18:29Z

No, that's most definitely a regression. @holiman do you have time to take a look?

holiman · 2018-10-08T14:30:01Z

Ah, crap. Probably trivial to fix, unsure if I'll be able to make it in time for release, if we release today though.

holiman · 2018-10-08T16:04:42Z

Hm, actually.. The contract.codeHash is not exposed to javascript tracers, are they? If you're writing your own tracer in go, you could more easily solve it by hashing it yourself.

if c.CodeHash == (common.Hash{}) {
 c.CodeHash = crypto.Keccak256Hash(c.code)
}

Or did I misunderstand the usecase?

holiman · 2018-10-08T16:07:41Z

The new intended behaviour is to not calculate the codehash for initcode. The only times we have the code hash is

When the code is loaded from the trie
When the code is created via CREATE2, which has a penalty-fee since we're forced to hash the code.

We could specifically calculate it if debug tracing is turned on, but I'm not sure that's a better solution than hashing it lazily inside the specific tracer that needs it.

wuestholz · 2018-10-08T18:06:39Z

@karalabe @holiman Thank you very much for looking into this.

I see. Yes, I'm using my own tracer in Go. I was going to change my code to compute the hash myself, but wasn't sure if this change in behavior was intended. :) In that case, I will just change my code.

saracen · 2019-06-28T02:50:35Z

In case anybody else stumbles across the problem I did:

CreateAddress2's signature remains the same, CreateAddress2(common.Address, [32]byte, []byte), across multiple versions, but the result is different. :)

v1.8.13: return common.BytesToAddress(Keccak256([]byte{0xff}, b.Bytes(), salt.Bytes(), code)[12:])
v1.8.14: return common.BytesToAddress(Keccak256([]byte{0xff}, b.Bytes(), salt[:], Keccak256(code))[12:])
v1.8.17: return common.BytesToAddress(Keccak256([]byte{0xff}, b.Bytes(), salt[:], inithash)[12:])

holiman added 3 commits October 1, 2018 13:58

core/vm/runtim: benchmark create/create2

c4e3625

core/vm: do less hashing in CREATE2

efc3287

core/vm: avoid storing jumpdest analysis for initcode

67bad85

holiman requested a review from karalabe October 2, 2018 08:20

tomusdrw mentioned this pull request Oct 2, 2018

Don't hash the init_code of CREATE. openethereum/parity-ethereum#9688

Merged

holiman added 3 commits October 3, 2018 10:43

core/vm: avoid unneccesary lookups, remove unused fields

cc28a29

core/vm: go formatting tests

880f698

core/vm: save jumpdest analysis locally

72f655d

holiman force-pushed the faster_create branch from e1725d6 to 72f655d Compare October 3, 2018 08:43

karalabe reviewed Oct 3, 2018

View reviewed changes

holiman added 2 commits October 3, 2018 14:22

core/vm: use common.Hash instead of nil, fix review comments

5d5a01e

core/vm: removed type destinations

843d483

karalabe reviewed Oct 3, 2018

View reviewed changes

holiman added 2 commits October 3, 2018 16:00

core/vm: correct check for empty hash

4727670

eth: more elegant api_tracer

f616a39

karalabe reviewed Oct 4, 2018

View reviewed changes

karalabe added this to the 1.8.17 milestone Oct 4, 2018

core/vm: address review concerns

8f04ccd

karalabe approved these changes Oct 4, 2018

View reviewed changes

karalabe merged commit 89a3245 into ethereum:master Oct 4, 2018

Conversation

holiman commented Oct 2, 2018

Uh oh!

karalabe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karalabe left a comment

Choose a reason for hiding this comment

Uh oh!

wuestholz commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karalabe commented Oct 8, 2018

Uh oh!

holiman commented Oct 8, 2018

Uh oh!

holiman commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

holiman commented Oct 8, 2018

Uh oh!

wuestholz commented Oct 8, 2018

Uh oh!

saracen commented Jun 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wuestholz commented Oct 8, 2018 •

edited

Loading

holiman commented Oct 8, 2018 •

edited

Loading