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

Break circular dependency between Client and Engine (part 1) #10833

Merged
merged 6 commits into from
Jul 4, 2019
Merged

Break circular dependency between Client and Engine (part 1) #10833

merged 6 commits into from
Jul 4, 2019

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Jul 2, 2019 •
edited
Loading

ref. #9114

Don't use the Client to look up the parent block so we can calculate the parent step. Instead, store the parent header in the OpenBlock and pass it to on_close_block().

It is unsatisfying to store a whole Header in all OpenBlocks/ClosedBlocks just because Aura need the parent step (a u64), but all calls to OpenBlock::new() already pass in the parent Header (so no new lookup/decoding). I tried to store a reference instead but reopen() is problematic.

@dvdplm dvdplm self-assigned this Jul 2, 2019
dvdplm
dvdplm commented Jul 2, 2019
View reviewed changes
ethcore/src/block.rs
@@ -195,7 +195,7 @@ impl<'x> OpenBlock<'x> {
engine.populate_from_parent(&mut r.block.header, parent);

engine.machine().on_new_block(&mut r.block)?;
engine.on_new_block(&mut r.block, is_epoch_begin, &mut ancestry.into_iter())?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ancestry was never used so I'm not sure what it was used for tbh.

Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch!

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was for engines that need to compute additional information regarding finalisation (for example, finalise blocks based on ancestor finalisation information).

Looks like we at least don’t need it right now!

@dvdplm dvdplm marked this pull request as ready for review July 2, 2019 18:32
@dvdplm dvdplm added the A0-pleasereview 🤓 Pull request needs code review. label Jul 2, 2019
debris
debris approved these changes Jul 3, 2019
View reviewed changes
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 great!

ethcore/src/block.rs
@@ -195,7 +195,7 @@ impl<'x> OpenBlock<'x> {
engine.populate_from_parent(&mut r.block.header, parent);

engine.machine().on_new_block(&mut r.block)?;
engine.on_new_block(&mut r.block, is_epoch_begin, &mut ancestry.into_iter())?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch!

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 3, 2019
@dvdplm dvdplm requested a review from sorpaas July 3, 2019 07:04
sorpaas
sorpaas approved these changes Jul 4, 2019
View reviewed changes
Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

LGTM!

ethcore/src/block.rs
@@ -195,7 +195,7 @@ impl<'x> OpenBlock<'x> {
engine.populate_from_parent(&mut r.block.header, parent);

engine.machine().on_new_block(&mut r.block)?;
engine.on_new_block(&mut r.block, is_epoch_begin, &mut ancestry.into_iter())?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was for engines that need to compute additional information regarding finalisation (for example, finalise blocks based on ancestor finalisation information).

Looks like we at least don’t need it right now!

@dvdplm dvdplm merged commit 582a4ea into master Jul 4, 2019
@dvdplm dvdplm deleted the dp/fix/disentangle-engine-and-client branch July 4, 2019 11:43
dvdplm added a commit that referenced this pull request Jul 4, 2019
@dvdplm
Merge branch 'master' into dp/fix/prevent-building-block-on-top-of-sa…
95ec389 
…me-parent

* master:
  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)
  idiomatic changes to PodState (#10834)
  Allow --nat extip:your.host.here.org (#10830)
  When updating the client or when called from RPC, sleep should mean sleep (#10814)
  Remove excessive warning (#10831)
  Fix typo in README.md (#10828)
  ethcore does not use byteorder (#10829)
  Better logging when backfilling ancient blocks fail (#10796)
  depends: Update wordlist to v1.3 (#10823)
@ordian ordian added this to the 2.6 milestone Jul 4, 2019
dvdplm added a commit that referenced this pull request Jul 4, 2019
@dvdplm
Merge branch 'master' into ng-fix-fork-choice
26f80c2 
* master:
  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)
  idiomatic changes to PodState (#10834)
  Allow --nat extip:your.host.here.org (#10830)
  When updating the client or when called from RPC, sleep should mean sleep (#10814)
  Remove excessive warning (#10831)
  Fix typo in README.md (#10828)
  ethcore does not use byteorder (#10829)
  Better logging when backfilling ancient blocks fail (#10796)
  depends: Update wordlist to v1.3 (#10823)
  cargo update -p smallvec (#10822)
  replace memzero with zeroize crate (#10816)
  Don't repeat the logic from Default impl (#10813)
  removed additional_params method (#10818)
  Add Constantinople eips to the dev (instant_seal) config (#10809)
dvdplm added a commit that referenced this pull request Jul 4, 2019
@dvdplm
Merge branch 'master' into luke-whisper-cli-test
cc45ea8 
* master: (21 commits)
  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)
  idiomatic changes to PodState (#10834)
  Allow --nat extip:your.host.here.org (#10830)
  When updating the client or when called from RPC, sleep should mean sleep (#10814)
  Remove excessive warning (#10831)
  Fix typo in README.md (#10828)
  ethcore does not use byteorder (#10829)
  Better logging when backfilling ancient blocks fail (#10796)
  depends: Update wordlist to v1.3 (#10823)
  cargo update -p smallvec (#10822)
  replace memzero with zeroize crate (#10816)
  Don't repeat the logic from Default impl (#10813)
  removed additional_params method (#10818)
  Add Constantinople eips to the dev (instant_seal) config (#10809)
  removed redundant fmt::Display implementations (#10806)
  revert changes to .gitlab-ci.yml (#10807)
  Add filtering capability to `parity_pendingTransactions` (issue 8269) (#10506)
  ...
dvdplm added a commit that referenced this pull request Jul 4, 2019
@dvdplm
Merge branch 'master' into dp/chore/extricate-account-db-into-own-crate
b7c7aae 
* 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)
dvdplm added a commit that referenced this pull request Jul 4, 2019
@dvdplm
Merge branch 'dp/chore/extricate-account-db-into-own-crate' into dp/c…
9553c8c 
…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)
This was referenced Nov 5, 2019
Closed
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sorpaas sorpaas sorpaas approved these changes

@debris debris debris approved these changes

Assignees

@dvdplm dvdplm

Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.6
Development

Successfully merging this pull request may close these issues.
4 participants
@dvdplm @debris @sorpaas @ordian