backwards compatible call_type creation_method #11450

ordian · 2020-02-04T16:31:48Z

on top of #11449

this should allow having tracing nodes db synced with 2.6 and 2.7

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

* master: rlp_derive: cleanup (#11446)

dvdplm · 2020-02-04T18:07:19Z

How should we test this change? Do you have a step-by-step recipe?

dvdplm · 2020-02-04T18:10:13Z

@fleupold Can you take a look at this please?

ordian · 2020-02-04T18:18:47Z

the #11427 was reverted in e188f47 and it shouldn't be reviewed (or the reversion can be extracted into a separate PR)

To test this

Sync the tracing db from genesis a bit with 2.6
Stop 2.6 and continue syncing with 2.7.0.
Stop 2.7.0 and continue with this branch
Curl some traces (trace_filter) in ranges synced with different versions.

There are some unit tests as well 8aca13b

ethcore/trace/src/types/trace.rs

dvdplm · 2020-02-05T07:17:26Z

ethcore/trace/src/types/trace.rs

+		}))
+	}
+}
+
 /// Description of a _call_ action, either a `CALL` operation or a message transaction.
 #[derive(Debug, Clone, PartialEq, RlpEncodable, RlpDecodable)]
 pub struct Call {


What do you think about annotating that data of this type end up rlp-ized in the database and thus any changes need to be made backwards compatibly?

dvdplm · 2020-02-05T07:19:44Z

ethcore/trace/src/types/trace.rs

@@ -113,6 +242,9 @@ pub struct Create {
 	pub gas: U256,
 	/// The init code.
 	pub init: Bytes,
+	/// Creation method (CREATE vs CREATE2).


I think we should add a not to the docs for this type that data of this type ends up in the database and so great care is necessary when making changes.

fleupold

I'm very sorry for merging the #11311 in a bad state. I originally had backwards compatibility in mind and tested specifically for that. I didn't rerun the test plan after the last set of changes, which was very bad.

Generally I'm wondering if it's worth all this hassle in order to change an enum<None, a, b, ...> to an Option<enum<a, b, ...>> type. Aren't they semantically the same?

fleupold · 2020-02-05T08:05:43Z

rpc/src/v1/types/trace.rs

@@ -240,8 +244,6 @@ impl From<trace::Create> for Create {
 #[derive(Debug, Serialize)]
 #[serde(rename_all = "lowercase")]
 pub enum CallType {
-	/// None
-	None,


Since all the trouble is coming from killing this variant here, would it maybe be better to leave the None type on the enum directly? Does it make much of a difference to have an enum with 5 variants vs a nested enum (Option where some has 4 variants)?

The difference is that Option<CallType> and CallType from 2.6 have different rlp serialization layout, the former is a list, and the latter is serialized as an int. If we want to be backwards compatible with both 2.6 and 2.7.0, we need to be able to deserialize both versions to the same struct.

fleupold · 2020-02-05T08:11:13Z

ethcore/trace/src/types/trace.rs

+impl Decodable for CallType {
+	fn decode(rlp: &Rlp) -> Result<Self, DecoderError> {
+		rlp.as_val().and_then(|v| Ok(match v {
+			0u32 => CallType::Call,


Is this correct? In the old version, when we serialized a 0 this meant CallType::None, but it will now be read to CallType::Call. Shouldn't this all be shifted by 1?

Option<CallType> is serialized as list https://github.com/paritytech/parity-common/blob/a8329435e19d46b3e50145a9b5a399bfac52a31f/rlp/src/impls.rs#L70-L99 and I assume that #11311 was reviewed properly, so we don't have to re-review it again 😅

Actually, I think you're right, let me test this again.

tomusdrw

lgtm!

tomusdrw · 2020-02-05T10:37:35Z

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: ActionType::Call,


Doesn't that change the JSON tests results?

I haven't touched that code, so I assume it was reviewed
#11311 (comment)

dvdplm · 2020-02-05T10:39:18Z

To test this

1. Sync the tracing db from genesis a bit with 2.6

2. Stop 2.6 and continue syncing with 2.7.0.

3. Stop 2.7.0 and continue with this branch

4. Curl some traces (`trace_filter`) in ranges synced with different versions.

Works here. :)

* master: gcc to clang (#11453) Avoid copies (#11451) Cargo.lock: cargo lock translate (#11448) Avoid long state queries when serving GetNodeData requests (#11444) Cargo.lock: cargo update -p kvdb-rocksdb (#11447)

* 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>

…pstream * master: upgrade some of the dependencies (#11467) Some more release track changes to README.md (#11465) Update simple one-line installer due to switching to a single stable release track (#11463) update Dockerfile (#11461) Implement EIP-2124 (#11456) [eth classic chainspec]: remove `balance = 1` (#11459) just to make sure we don't screw up this again (#11455) backwards compatible call_type creation_method (#11450) gcc to clang (#11453) Avoid copies (#11451) Cargo.lock: cargo lock translate (#11448)

* master: [eth classic chainspec]: remove `balance = 1` (#11459) just to make sure we don't screw up this again (#11455) backwards compatible call_type creation_method (#11450)

ordian added 8 commits February 4, 2020 14:52

rlp_derive: update syn & co

63fe6fa

rlp_derive: remove dummy_const

453a02d

rlp_derive: remove unused attirubutes

9211344

rlp-derive: change authors

6e54f21

rlp_derive: add rlp(default) attribute

4270b24

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

e188f47

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

trace: backwards compatible call_type and creation_method

1861477

trace: add rlp backward compatibility tests

8aca13b

ordian added A0-pleasereview 🤓 Pull request needs code review. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. M4-core ⛓ Core client code / Rust. labels Feb 4, 2020

ordian added 2 commits February 4, 2020 18:18

Merge branch 'master' into ao-fix-rlp-calltype-backwards-compatible

4404b4a

* master: rlp_derive: cleanup (#11446)

cleanup

5faea99

dvdplm requested a review from tomusdrw February 4, 2020 18:10

dvdplm reviewed Feb 5, 2020

View reviewed changes

fleupold reviewed Feb 5, 2020

View reviewed changes

tomusdrw approved these changes Feb 5, 2020

View reviewed changes

dvdplm approved these changes Feb 5, 2020

View reviewed changes

ordian added 3 commits February 5, 2020 12:36

i know, i hate backwards compatibility too

3462e01

address review grumbles

9a021c9

Merge branch 'master' into ao-fix-rlp-calltype-backwards-compatible

c317584

* master: gcc to clang (#11453) Avoid copies (#11451) Cargo.lock: cargo lock translate (#11448) Avoid long state queries when serving GetNodeData requests (#11444) Cargo.lock: cargo update -p kvdb-rocksdb (#11447)

dvdplm merged commit 6265433 into master Feb 5, 2020

dvdplm deleted the ao-fix-rlp-calltype-backwards-compatible branch February 5, 2020 14:30

ordian mentioned this pull request Feb 5, 2020

rlp_derive: #[rlp(default)] attribute #11449

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backwards compatible call_type creation_method #11450

backwards compatible call_type creation_method #11450

ordian commented Feb 4, 2020

dvdplm commented Feb 4, 2020

dvdplm commented Feb 4, 2020

ordian commented Feb 4, 2020 •

edited

Loading

dvdplm Feb 5, 2020

dvdplm Feb 5, 2020

fleupold left a comment

fleupold Feb 5, 2020 •

edited

Loading

ordian Feb 5, 2020 •

edited

Loading

fleupold Feb 5, 2020

ordian Feb 5, 2020

ordian Feb 5, 2020

tomusdrw left a comment

tomusdrw Feb 5, 2020

ordian Feb 5, 2020

dvdplm commented Feb 5, 2020

backwards compatible call_type creation_method #11450

backwards compatible call_type creation_method #11450

Conversation

ordian commented Feb 4, 2020

dvdplm commented Feb 4, 2020

dvdplm commented Feb 4, 2020

ordian commented Feb 4, 2020 • edited Loading

dvdplm Feb 5, 2020

Choose a reason for hiding this comment

dvdplm Feb 5, 2020

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

fleupold Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

ordian Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

fleupold Feb 5, 2020

Choose a reason for hiding this comment

ordian Feb 5, 2020

Choose a reason for hiding this comment

ordian Feb 5, 2020

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw Feb 5, 2020

Choose a reason for hiding this comment

ordian Feb 5, 2020

Choose a reason for hiding this comment

dvdplm commented Feb 5, 2020

ordian commented Feb 4, 2020 •

edited

Loading

fleupold Feb 5, 2020 •

edited

Loading

ordian Feb 5, 2020 •

edited

Loading