Move more types out of ethcore #10880

dvdplm · 2019-07-12T12:35:37Z

This PR is a small step on the way to be able to extract the Engine trait from ethcore. The next step after this is probably to remove Machine from the trait but there are several tricky types that need moving/deep thought too:

ExecutedBlock: embeds State<StateDB> which means it can't be moved to common-types (circular ref.)
Proof: has a troublesome WithState variant which implies a dependency on Machine.
ConstructedVerifier: uses a trait that needs to be extracted somewhere sensible.
ClientIoMessage: this one is probably the trickiest to move outside of ethcore because of how the Execute variant is used.

…and probably others.

Reviewers guide

common-types now has plenty more dependencies
common-types now owns PreverifiedBlock and several machine related types: AuxiliaryData, AuxiliaryRequest, Call, CommonParams (here the crate gained way more code than I'd like: something we can perhaps fix with a trait), all error types
having independent errors simplifies error handling in account-state (and potentially others down the line)
the params() function in the Engine trait does not provide a default implementation anymore: the idea is that future PRs will remove the dependency on Machine all-together.
despite the high SNR here there are no changes to logic or other "dangerous" changes.

Remove botched ethcore-error crate

* master: Extricate PodAccount and state Account to own crates (#10838) logs (#10817) refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812) Break circular dependency between Client and Engine (part 1) (#10833) tests: Relates to #10655: Test instructions for Readme (#10835) refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)

…hore/extricate-state-backend * dp/chore/extricate-account-db-into-own-crate: third time's the charm test failure 2 test failure Extricate PodAccount and state Account to own crates (#10838) logs (#10817) refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812) Break circular dependency between Client and Engine (part 1) (#10833) tests: Relates to #10655: Test instructions for Readme (#10835) refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657)

* master: Extract AccountDB to account-db (#10839) test: Update Whisper test for invalid pool size (#10811)

* master: Improve logging and cleanup in miner around block sealing (#10745)

* master: whisper is no longer a part of parity-ethereum repo (#10855) [ethash] remove mem::uninitialized (#10861) Docker images renaming (#10863) Move the substate module into ethcore/executive (#10867)

debris

There are several things that I like and don't like about this pr. I definitely like unification of error types and cleanup possibilities that it brings. I don't like putting more modules into types/ crate which imo should be kept as small as possible. But all in all I believe that pros of this pr significantly out-wage cons and we can modularize types crate more after we finish our work with ethcore.

great work!

debris · 2019-07-14T15:30:31Z

ethcore/types/src/engines/machine.rs

+/// Auxiliary data fetcher for an Ethereum machine. In Ethereum-like machines
+/// there are two kinds of auxiliary data: bodies and receipts.
+#[derive(Default, Clone)]
+pub struct AuxiliaryData<'a> {


Can we keep those data structures together with Machine? I don't think that putting all plain data structures in the same crate is a good idea. imo this crate should define only the basic few (header, block, blockid..) and nothing else

You are right and I agree that with this pr the common-types crate becomes a bit too “heavy”. We need to move some types out of the way on the path to disentangle machine from engine though but that’s not done in this pr so maybe best to undo this change here and redo it in the next pr (or do it better).

debris · 2019-07-14T15:31:05Z

ethcore/types/src/engines/mod.rs

+
+/// Ethash-specific extensions.
+#[derive(Debug, Clone)]
+pub struct EthashExtensions {


ethcore/src/ethereum/ethash_eng.rs

ordian

a bunch of grumbles

ordian · 2019-07-15T09:19:45Z

ethcore/src/client/traits.rs

 use executive::Executed;
 use account_state::state::StateInfo;
 use trace::LocalizedTrace;
-use verification::queue::QueueInfo as BlockQueueInfo;
-use verification::queue::kind::blocks::Unverified;
+use verification::queue::kind::blocks::Unverified; // todo this is reexported from common_types


do you want to resolve the TODO in this PR?

let's do it in a separate one

It’s fine to do in separate pr: it’s a matter of checking what uses the type outside ethcore.

ethcore/src/verification/queue/mod.rs

ethcore/types/Cargo.toml

ethcore/src/lib.rs

ethcore/types/src/engines/mod.rs

ethcore/types/src/errors/block_error.rs

debris · 2019-07-16T11:08:19Z

I'll rebase this pr and fix review suggestions to get it in soon ;)

ethcore/src/ethereum/denominations.rs

ordian · 2019-07-17T10:10:40Z

ethcore/src/engines/mod.rs

-		"Engine error"
-	}
-}
-
 /// Seal type.
 #[derive(Debug, PartialEq, Eq)]
 pub enum Seal {


Feels like Seal and SealingState should go together, no?

ordian · 2019-07-17T10:18:35Z

ethcore/types/Cargo.toml


 [dev-dependencies]
-rustc-hex= "1.0"
+rustc-hex= "1.0" # todo: only needed for CommonParams to do from_hex on a const. Remove?


that's fine for tests

oh, yes, I should've removed the comment

dvdplm added 30 commits July 3, 2019 15:57

WIP move errors, pod_account and state account to own crates

f144f88

Sort out dependencies, fix broken code and tests

c8bf87f

Remove botched ethcore-error crate

remove template line

47bc6fb

fix review feedback

f8756bb

Remove test-only AccountDBMut::new

d54ebab

Extract AccountDB to account-db

7792810

Move Substate to state-account – wip

a55ca1e

Add lib.rs

201dac7

cleanup

e23acad

test failure

e0701e4

test failure 2

b42d16a

third time's the charm

e45d13f

Merge branch 'master' into dp/chore/extricate-state-backend

9b60a57

* master: Extract AccountDB to account-db (#10839) test: Update Whisper test for invalid pool size (#10811)

Merge branch 'master' into dp/chore/extricate-state-backend

c55e6b1

* master: Improve logging and cleanup in miner around block sealing (#10745)

Add factories crate

1c0722b

Use new factories crate

2114c12

Use factories crate

80ededa

Extract trace

d9ceaa6

Fix tests

9f3aa73

Sort out parity-util-mem and parking_lot

a6a25ba

cleanup

44a0904

WIP port over the rest of state from ethcore

ce3a211

Collect all impls for Machine

9ed1d47

some notes

67332d7

Rename pod-account to pod

0a42ea8

Move PodState to pod crate

d9ce43d

Use PodState from pod crate

aebee1d

Fix use clause for json tests

6344f9d

dvdplm added 4 commits July 12, 2019 14:40

Merge branch 'master' into dp/chore/move-more-types-out-of-ethcore

8e88f2e

* master: whisper is no longer a part of parity-ethereum repo (#10855) [ethash] remove mem::uninitialized (#10861) Docker images renaming (#10863) Move the substate module into ethcore/executive (#10867)

Fix some whitespace and a merge error

3cda700

ethcore tests

4febb27

test failures

1f97b2c

dvdplm self-assigned this Jul 12, 2019

dvdplm added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 12, 2019

dvdplm marked this pull request as ready for review July 12, 2019 14:47

dvdplm added 2 commits July 12, 2019 19:20

Restore Engine impls to master to make review a bit easier

30a5d86

cleanup

7c46f23

dvdplm requested review from debris and ordian July 12, 2019 17:48

whitespace

6a99771

ordian added this to the 2.7 milestone Jul 12, 2019

debris approved these changes Jul 14, 2019

View reviewed changes

ordian reviewed Jul 15, 2019

View reviewed changes

debris added 5 commits July 16, 2019 13:23

Merge branch 'master' into dp/chore/move-more-types-out-of-ethcore

ef3cced

applied review suggestions

588a85e

types does not depend on rustc-hex

e9da9ed

ethash engine moved to engine module

ca86b89

applied review suggestion

4ea83e6

debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 17, 2019

ordian approved these changes Jul 17, 2019

View reviewed changes

debris merged commit 1ef9d5b into master Jul 18, 2019

debris deleted the dp/chore/move-more-types-out-of-ethcore branch July 18, 2019 10:27

This was referenced Nov 5, 2019

Backport code structure changes #11241

Closed

Backport code structure changes #11242

Closed

Backport code structure changes #11243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move more types out of ethcore #10880

Move more types out of ethcore #10880

dvdplm commented Jul 12, 2019 •

edited

Loading

debris left a comment

debris Jul 14, 2019

dvdplm Jul 17, 2019

debris Jul 14, 2019

ordian left a comment

ordian Jul 15, 2019

debris Jul 17, 2019

dvdplm Jul 17, 2019

debris commented Jul 16, 2019

ordian Jul 17, 2019

ordian Jul 17, 2019

debris Jul 17, 2019

Move more types out of ethcore #10880

Move more types out of ethcore #10880

Conversation

dvdplm commented Jul 12, 2019 • edited Loading

debris 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

ordian 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

debris commented Jul 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm commented Jul 12, 2019 •

edited

Loading