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

Move more code into state-account #10840

Merged
merged 75 commits into from
Jul 8, 2019
Merged

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Jul 4, 2019

Moves ethcore/state into a new account-state crate.

State management has a piece that is tightly coupled to other ethcore modules, mainly Machine and Excutive (and related types and support code).
In order to extract the state management code from ethcore I have left one module – executive_state – in ethcore that isolates the code where the state code is too entangled with the rest of ethcore to be tackled at this time. The tests for state management are also still in ethcore because of the tight coupling with other ethcore modules. While this is rather ungainly and unsatisfactory I think it's an improvement to have this code in an isolated unit that makes it obvious where the coupling takes place.

The end result is that we now have 6k loc less in ethcore and the following new crates:

We also have a somewhat smaller API surface on ethcore.

The diff is very noisy and for that I apologise. The only really new code is in executive_state.rs and the rest is mostly cleanup and munging. None of the TODOs that show up as "green" are mine (but still worthy of consideration ofc). I have used this branch to sync Goerli.

Builds on #10839

@dvdplm dvdplm self-assigned this Jul 4, 2019
dvdplm added 20 commits July 4, 2019 16:09
* 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)
Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

Great PR! I have only a few questions about the code. I believe that those changes are very needed and are a huge step towards getting ethcore more modular

ethcore/factories/src/lib.rs Outdated Show resolved Hide resolved
ethcore/src/client/evm_test_client.rs Outdated Show resolved Hide resolved
ethcore/src/error.rs Show resolved Hide resolved
ethcore/trace/Cargo.toml Show resolved Hide resolved
ethcore/trace/Cargo.toml Show resolved Hide resolved
ethcore/trace/src/db.rs Outdated Show resolved Hide resolved
ethcore/trace/src/db.rs Outdated Show resolved Hide resolved
ethcore/trace/src/types/flat.rs Show resolved Hide resolved
rpc/src/lib.rs Show resolved Hide resolved
@dvdplm dvdplm requested a review from debris July 8, 2019 10:00
* master:
  EIP-1702: Generalized Account Versioning Scheme (#10771)
Copy link
Collaborator

@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.

Looks good, definitely a good step forward.

ethcore/account-state/Cargo.toml Show resolved Hide resolved
ethcore/account-state/src/substate.rs Show resolved Hide resolved
ethcore/blockchain/src/database_extras.rs Show resolved Hide resolved
ethcore/factories/Cargo.toml Outdated Show resolved Hide resolved
ethcore/trace/src/db.rs Outdated Show resolved Hide resolved
ethcore/trace/Cargo.toml Show resolved Hide resolved
ethcore-blockchain = { path = "../blockchain" }
ethcore-db = { path = "../db" }
ethereum-types = "0.6"
evm = { path = "../evm" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again requiring entire EVM seems odd, would be worth to remove it in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We use CallType from the evm, that's all. Seems like a good opportunity for a refactor indeed.


[dependencies]
common-types = { path = "../types"}
ethcore-blockchain = { path = "../blockchain" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need the entire blockchain or just some trait definitions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently we use BlockChainDB and the contentious DatabaseExtras, e.g.:

pub struct TraceDB<T> where T: DatabaseExtras {
	/// cache
	traces: RwLock<HashMap<H256, FlatBlockTraces>>,
	/// hashes of cached traces
	cache_manager: RwLock<CacheManager<H256>>,
	/// db
	db: Arc<dyn BlockChainDB>,
	/// tracing enabled
	enabled: bool,
	/// extras
	extras: Arc<T>,
}

Copy link
Collaborator Author

@dvdplm dvdplm Jul 8, 2019

Choose a reason for hiding this comment

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

What is your thinking here? Are you thinking we should move traits to a common-traits crate?

ethcore/trace/src/types/flat.rs Show resolved Hide resolved
Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

looks good. I think we should get it in asap and we can fix trace <-> blockchain relation later on

@@ -100,7 +108,7 @@ impl<'a> EvmTestClient<'a> {
}

/// Change default function for dump state (default does not dump)
pub fn set_dump_state_fn(&mut self, dump_state: fn(&state::State<state_db::StateDB>) -> Option<pod_state::PodState>) {
pub fn set_dump_state(&mut self) {
self.dump_state = dump_state;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this function looks a bit weird

@dvdplm
Copy link
Collaborator Author

dvdplm commented Jul 8, 2019

@tomusdrw @debris Merging this but I'll make sure the following gets addressed in follow up PRs:

  1. sort out the trace <–> blockchain interdependencies
    cf. Move more code into state-account #10840 (comment)

  2. attempt to avoid pulling in EVM in trace and account-state

  3. make the conditional "dump state" suck a bit less

@dvdplm dvdplm merged commit 44cc442 into master Jul 8, 2019
@dvdplm dvdplm deleted the dp/chore/extricate-state-backend branch July 8, 2019 16:18
dvdplm added a commit that referenced this pull request Jul 8, 2019
* master:
  Move more code into state-account (#10840)
  Remove compiler warning (#10865)
  [ethash] use static_assertions crate (#10860)
ordian added a commit that referenced this pull request Jul 9, 2019
* master:
  Run cargo fix on a few of the worst offenders (#10854)
  removed redundant fork choice abstraction (#10849)
  Extract state-db from ethcore (#10858)
  Fix fork choice (#10837)
  Move more code into state-account (#10840)
  Remove compiler warning (#10865)
  [ethash] use static_assertions crate (#10860)
dvdplm added a commit that referenced this pull request Jul 9, 2019
* master:
  Run cargo fix on a few of the worst offenders (#10854)
  removed redundant fork choice abstraction (#10849)
  Extract state-db from ethcore (#10858)
  Fix fork choice (#10837)
  Move more code into state-account (#10840)
  Remove compiler warning (#10865)
  [ethash] use static_assertions crate (#10860)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants