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

ethcore: cleanup after #11531 #11546

Merged
merged 5 commits into from
Mar 5, 2020
Merged

ethcore: cleanup after #11531 #11546

merged 5 commits into from
Mar 5, 2020

Conversation

ordian
Copy link
Collaborator

@ordian ordian commented Mar 5, 2020

Bikeshed on naming is welcome!

@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). labels Mar 5, 2020
@ordian ordian requested review from dvdplm and niklasad1 March 5, 2020 11:07
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

I think I understand what the idea here is: to preserve the type safety and impede calls to enact() with unverified data. The price for that is yet another block type and somewhat less readable code.
I am biased and think that the more important property here should be readability. I think the type safety you’re after here is not 100% waterproof (constructing a PreverifiedBlock to pass in is still possible I think) so I’m not sure it’s worth it.
That said, if other feel this is better I’m fine merging it.

One idea I had was to look at separating the RLP from the block type higher up in the call graph, at the queue level. Maybe that’s a more natura separation for the data? So draining from the queue would yield a tuple of rlp and a pre verified block struct?

ethcore/src/block.rs Outdated Show resolved Hide resolved
@ordian ordian added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 5, 2020
@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Mar 5, 2020
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm, minor nits.

ethcore/verification/src/verification.rs Outdated Show resolved Hide resolved
ethcore/verification/src/queue/kind.rs Outdated Show resolved Hide resolved
@@ -296,7 +296,7 @@ impl Importer {
trace_time!("import_verified_blocks");
let start = Instant::now();

for mut block in blocks {
for (block, block_bytes) in blocks {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this, it makes it quite obvious what is going on.

@ordian ordian merged commit 5c3c979 into master Mar 5, 2020
@ordian ordian deleted the ao-cleanup-preverified-block branch March 5, 2020 16:00
ordian added a commit that referenced this pull request Mar 6, 2020
* master:
  ethcore: cleanup after #11531 (#11546)
  license update (#11543)
  Less cloning when importing blocks (#11531)
  Github Actions (#11528)
  Fix Alpine Dockerfile (#11538)
  Remove AuxiliaryData/AuxiliaryRequest (#11533)
  [journaldb]: cleanup (#11534)
ordian added a commit that referenced this pull request Mar 10, 2020
* master:
  Code cleanup in the sync module (#11552)
  initial cleanup (#11542)
  Warn if genesis constructor revert (#11550)
  ethcore: cleanup after #11531 (#11546)
  license update (#11543)
  Less cloning when importing blocks (#11531)
  Github Actions (#11528)
  Fix Alpine Dockerfile (#11538)
  Remove AuxiliaryData/AuxiliaryRequest (#11533)
  [journaldb]: cleanup (#11534)
  Remove references to parity-ethereum (#11525)
  Drop IPFS support (#11532)
  chain-supplier: fix warning reporting for GetNodeData request (#11530)
  Faster kill_garbage (#11514)
  [EngineSigner]: don't sign message with only zeroes (#11524)
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. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants