[Trace] Distinguish between `create` and `create2` #11311

fleupold · 2019-12-06T15:49:22Z

Closes #10922 and implements the approach suggested by @tomusdrw.

This PR introduces a new field on the Create struct in the trace module. This allows differentiating between create and create2 contract deployments. The information comes from the exec_instruction method inside the ethcore interpreter module (there we have access to the low level OpCode).

It's then passed on via the ActionType enum (previously CallType) on the ActionParams struct, and eventually translated into the trace object.

It looks like at the moment CREATE opcodes are stored as CallType::None. I therefore reused this subtype (persisted as 0u32) and added a new one for CREATE2.

Test plan

Checkout master (not this PR), compile and run private test chain

./target/debug/parity --config dev --tracing=on --unlock="0x00a329c0648769a73afac7f9381e08fb43dbea72" --password=./password

Checkout this test repo with a simple deploy2 factory
- npm ci
- make deploy
- make test (capture block number that is printed)
Get trace of that block

curl -w "\n" -H "Content-Type: application/json" -X POST --data '{"jsonrpc":"2.0","method":"trace_replayBlockTransactions","params":["<block_number>",["trace"]],"id":67}' http://localhost:8545 | python -m json.tool

See no creation type in json output
Checkout this PR, build and run on same private chain
Rerun the rpc call with the same block number
Now observe "create_type": "create2", in output
The trace of block_number - 1 should have a "create_type": "create",
Same will work if you run make test again

parity-cla-bot · 2019-12-06T15:49:25Z

It looks like @fleupold hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

parity-cla-bot · 2019-12-06T15:50:33Z

It looks like @fleupold signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

tomusdrw

Overall looks good, a few improvements that we could do though.

ethcore/machine/src/externalities.rs

ethcore/trace/src/types/trace.rs

ethcore/wasm/src/runtime.rs

rpc/src/v1/types/trace.rs

dvdplm · 2019-12-06T18:01:10Z

ethcore/vm/src/action_params.rs

@@ -89,7 +89,7 @@ pub struct ActionParams {
 	/// Input data.
 	pub data: Option<Bytes>,
 	/// Type of call


Docs here could be more helpful maybe?

ethcore/trace/src/types/trace.rs

dvdplm · 2019-12-06T18:15:33Z

ethcore/trace/src/types/trace.rs

@@ -122,6 +124,7 @@ impl From<ActionParams> for Create {
 			value: p.value.value(),
 			gas: p.gas,
 			init: p.code.map_or_else(Vec::new, |c| (*c).clone()),
+			create_type: p.call_type,


This reads a bit awkwardly imo, "is it a create type or a call type?", so maybe it should be renamed action_type in ActionParams?

dvdplm · 2019-12-06T18:26:29Z

ethcore/vm/src/action_params.rs

@@ -130,7 +130,7 @@ impl From<ethjson::vm::Transaction> for ActionParams {
 			gas: t.gas.into(),
 			gas_price: t.gas_price.into(),
 			value: ActionValue::Transfer(t.value.into()),
-			call_type: match address.is_zero() { true => CallType::None, false => CallType::Call },	// TODO @debris is this correct?
+			call_type: if address.is_zero() { ActionType::Create } else { ActionType::Call },


Probably a dumb q but is it true that a vm::Transaction with a CREATE2 is indistinguishable from one with CREATE?

I'm actually not quite sure whether ActionType::Create is right here. The type json::vm::Transaction is for the legacy vmTests in jsontests. It should not deal with call/create, but only the internal execution of EVM, and thus, it should always be just plain ActionType::Call?

Anyway, I'm not entirely sure either, but it won't matter too much -- the legacy vmtests is in the process of being removed and we might be able to remove this function really soon!

So what's the best way forward? Should I unconditionally use ActionType::Call or leave as is?

My recommendation is to unconditionally use ActionType::Call, because that is, based on my understanding, how vmtests works. (I would appreciate it if someone else can take a glance on this again, just in case if I'm wrong.)

Practically as long as we're passing all vmtests everything should be fine.

…loosely typed action type

dvdplm

lgtm modulo minor documentation grumbles.

dvdplm · 2019-12-19T07:46:21Z

ethcore/trace/src/types/trace.rs

@@ -33,6 +33,56 @@ pub struct CallResult {
 	pub output: Bytes,
 }

+/// `Call` type


I think we can do better here. Please document what the CallType is and what it is used for.

dvdplm · 2019-12-19T07:51:57Z

ethcore/trace/src/types/trace.rs

@@ -51,6 +101,48 @@ impl CreateResult {
 	}
 }

+/// `Create` method


Suggested change

/// `Create` method

/// Distinguish between use of `CREATE` and `CREATE2` opcodes in an action.

fleupold · 2019-12-20T12:39:23Z

@sorpaas would you be able to take another look?

sorpaas · 2019-12-20T14:28:26Z

ethcore/vm/src/action_params.rs

@@ -130,7 +130,7 @@ impl From<ethjson::vm::Transaction> for ActionParams {
 			gas: t.gas.into(),
 			gas_price: t.gas_price.into(),
 			value: ActionValue::Transfer(t.value.into()),
-			call_type: match address.is_zero() { true => CallType::None, false => CallType::Call },	// TODO @debris is this correct?
+			action_type: if address.is_zero() { ActionType::Create } else { ActionType::Call },


I still believe that ActionType::Create here may not be correct. All vmtests are raw VM calls.

fleupold · 2020-01-03T16:44:48Z

@sorpaas Would you be able to take another look?

tomusdrw

Nice! Looks good to me, I'd only add skip_serializing_if for creationMethod and it should be good to go!

tomusdrw · 2020-01-09T12:31:39Z

ethcore/trace/src/types/trace.rs

+}
+
+impl CreationMethod {
+	fn maybe_new(action_type: ActionType) -> Option<Self> {


Both of the fn maybe_new methods look more like they could be a TryFrom implementation, but I don't mind the current.

Just checked that it was me suggesting fn new() -> Option<Self> 😂

tomusdrw · 2020-01-09T12:35:18Z

rpc/src/v1/types/trace.rs

@@ -223,6 +223,8 @@ pub struct Create {
 	gas: U256,
 	/// Initialization code
 	init: Bytes,
+	// Create Type
+	creation_method: Option<CreationMethod>,


Can we still add #[serde_skip_serializing_if="Option::is_none"]?

sorpaas

LGTM after what @tomusdrw said!

…maybe_new

ordian · 2020-01-10T15:29:04Z

cargo audit check is fixed #11378, merge with master after that PR is merged and we're good to go :)

ordian · 2020-01-13T10:10:31Z

Thanks!

This reverts commit 87e1080.

…11427) This reverts commit 87e1080.

* Revert "[Trace] Distinguish between `create` and `create2` (#11311)" (#11427) This reverts commit 87e1080. * Bump version * Changelog * Update publish-docker.sh (#11428) Add :latest tag to building stable releases Co-authored-by: s3krit <pugh@s3kr.it>

…11311)" (#11427)" This reverts commit 5d4993b.

…pstream * master: Avoid long state queries when serving GetNodeData requests (#11444) Cargo.lock: cargo update -p kvdb-rocksdb (#11447) rlp_derive: cleanup (#11446) chore: remove unused dependencies (#11432) update kvdb-rocksdb to 0.4 (#11442) Rough architecutre diagram. (#11396) ethjson: impl Copy for hash type wrapper (#11423) Remove dead bootnodes, add new geth bootnodes (#11441) goerli: replace foundation bootnode (#11433) Update publish-docker.sh (#11428) Revert "[Trace] Distinguish between `create` and `create2` (#11311)" (#11427)

* rlp_derive: update syn & co * rlp_derive: remove dummy_const * rlp_derive: remove unused attirubutes * rlp-derive: change authors * rlp_derive: add rlp(default) attribute * Revert "Revert "[Trace] Distinguish between `create` and `create2` (#11311)" (#11427)" This reverts commit 5d4993b. * trace: backwards compatible call_type and creation_method * trace: add rlp backward compatibility tests * cleanup * i know, i hate backwards compatibility too * address review grumbles

* update classic testnet bootnodes (#11398) * update classic testnet bootnodes * Update kotti.json * Update kotti.json * Update kotti.json * Update mordor.json * verification: fix race same block + misc (#11400) * ethcore: fix race in verification * verification: fix some nits * verification: refactor err type `Kind::create` * fix: tests * address grumbles * address grumbles: don't panic * fix: export hardcoded sync format (#11416) * fix: export hardcoded sync format * address grumbles * make tests compile with rustc_hex 2.0 * fix grumbles: impl LowerHex for encoded Header * goerli: replace foundation bootnode (#11433) * Remove dead bootnodes, add new geth bootnodes (#11441) * update kvdb-rocksdb to 0.4 (#11442) * Avoid long state queries when serving GetNodeData requests (#11444) * Remove dead bootnodes, add new geth bootnodes * More granular locking when fetching state Finish GetDataNode requests early if queries take too long * typo * Use latest kvdb-rocksdb * Cleanup * Update ethcore/sync/src/chain/supplier.rs Co-Authored-By: Andronik Ordian <write@reusable.software> * Address review grumbles * Fix compilation * Address review grumbles Co-authored-by: Andronik Ordian <write@reusable.software> * rlp_derive: cleanup (#11446) * rlp_derive: update syn & co * rlp_derive: remove dummy_const * rlp_derive: remove unused attirubutes * rlp-derive: change authors * Cargo.lock: cargo update -p kvdb-rocksdb (#11447) * Cargo.lock: new lockfile format Manual backport of #11448 * gcc to clang (#11453) * gcc to clang test ``` export CC="sccache "$CC export CXX="sccache "$CXX ``` darwin build ``` CC=clang CXX=clang ``` * darwin - > default clang * Bump version and CHANGELOG.md * chore: remove unused dependencies (#11432) * fix: compiler warnings * chore: remove unused dependencies * Update CHANGELOG.md * update Cargo.lock * update CHANGELOG.md * backwards compatible call_type creation_method (#11450) * rlp_derive: update syn & co * rlp_derive: remove dummy_const * rlp_derive: remove unused attirubutes * rlp-derive: change authors * rlp_derive: add rlp(default) attribute * Revert "Revert "[Trace] Distinguish between `create` and `create2` (#11311)" (#11427)" This reverts commit 5d4993b. * trace: backwards compatible call_type and creation_method * trace: add rlp backward compatibility tests * cleanup * i know, i hate backwards compatibility too * address review grumbles * update CHANGELOG.md * just to make sure we don't screw up this again (#11455) * Update CHANGELOG.md Co-authored-by: Talha Cross <47772477+soc1c@users.noreply.github.com> Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com> Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: David <dvdplm@gmail.com> Co-authored-by: Andronik Ordian <write@reusable.software> Co-authored-by: Denis S. Soldatov aka General-Beck <general.beck@gmail.com>

fleupold added 3 commits December 6, 2019 15:07

adding a CreateType to ActionParams

278938d

rename calltype to action type

ca09ab4

comments & line lengths

8925bfd

dvdplm added A0-pleasereview 🤓 Pull request needs code review. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 M4-core ⛓ Core client code / Rust. labels Dec 6, 2019

dvdplm requested a review from sorpaas December 6, 2019 16:03

tomusdrw reviewed Dec 6, 2019

View reviewed changes

dvdplm reviewed Dec 6, 2019

View reviewed changes

fleupold added 6 commits December 13, 2019 15:01

rebame ActionParams.call_type

9357640

Making call/create type optional

7c433ea

Moving strict create/call type into trace package instead of storing …

4f49d57

…loosely typed action type

fix build

e6af320

Deriving ActionType from address_scheme in ext.create

c05a1f3

trigger build

eb0a81a

dvdplm approved these changes Dec 19, 2019

View reviewed changes

more detailed comments

12abb6f

sorpaas reviewed Dec 20, 2019

View reviewed changes

Change actions_type to call in vmtests

4b8073d

dvdplm requested review from tomusdrw, dvdplm and sorpaas January 8, 2020 17:31

tomusdrw approved these changes Jan 9, 2020

View reviewed changes

sorpaas approved these changes Jan 9, 2020

View reviewed changes

skipping serialization for Option::None and using TryFrom instead of …

df95d10

…maybe_new

retrigger build

d3cad9d

ordian added this to the 2.7 milestone Jan 10, 2020

fleupold added 2 commits January 10, 2020 18:01

Merge remote-tracking branch 'upstream/master' into 10922/create2_trace

9f7499c

trigger build

9659f19

ordian merged commit 87e1080 into openethereum:master Jan 13, 2020

ordian added a commit that referenced this pull request Jan 30, 2020

Revert "[Trace] Distinguish between create and create2 (#11311)"

097612e

This reverts commit 87e1080.

dvdplm added a commit that referenced this pull request Jan 30, 2020

Revert "[Trace] Distinguish between create and create2 (#11311)"

27da66d

This reverts commit 87e1080.

dvdplm added a commit that referenced this pull request Jan 30, 2020

Revert "[Trace] Distinguish between create and create2 (#11311)" (#…

5d4993b

…11427) This reverts commit 87e1080.

dvdplm added a commit that referenced this pull request Jan 30, 2020

Revert "[Trace] Distinguish between create and create2 (#11311)" (#…

4e04590

…11427) This reverts commit 87e1080.

ordian added a commit that referenced this pull request Feb 4, 2020

Revert "Revert "[Trace] Distinguish between create and create2 (#…

e188f47

…11311)" (#11427)" This reverts commit 5d4993b.

ordian mentioned this pull request Feb 4, 2020

backwards compatible call_type creation_method #11450

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Trace] Distinguish between `create` and `create2` #11311

[Trace] Distinguish between `create` and `create2` #11311

fleupold commented Dec 6, 2019

parity-cla-bot commented Dec 6, 2019

parity-cla-bot commented Dec 6, 2019

tomusdrw left a comment

dvdplm Dec 6, 2019

dvdplm Dec 6, 2019

dvdplm Dec 6, 2019

sorpaas Dec 8, 2019

fleupold Dec 13, 2019

sorpaas Dec 13, 2019

dvdplm left a comment

dvdplm Dec 19, 2019

dvdplm Dec 19, 2019

fleupold commented Dec 20, 2019

sorpaas Dec 20, 2019

fleupold commented Jan 3, 2020

tomusdrw left a comment

tomusdrw Jan 9, 2020

tomusdrw Jan 9, 2020

sorpaas left a comment

ordian commented Jan 10, 2020

ordian commented Jan 13, 2020

	/// `Create` method
	/// Distinguish between use of `CREATE` and `CREATE2` opcodes in an action.

[Trace] Distinguish between create and create2 #11311

[Trace] Distinguish between create and create2 #11311

Conversation

fleupold commented Dec 6, 2019

Test plan

parity-cla-bot commented Dec 6, 2019

parity-cla-bot commented Dec 6, 2019

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold commented Dec 20, 2019

Choose a reason for hiding this comment

fleupold commented Jan 3, 2020

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorpaas left a comment

Choose a reason for hiding this comment

ordian commented Jan 10, 2020

ordian commented Jan 13, 2020

[Trace] Distinguish between `create` and `create2` #11311

[Trace] Distinguish between `create` and `create2` #11311