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

deserialize block only once during verification #9161

Merged
merged 1 commit into from
Jul 25, 2018
Merged

Conversation

debris
Copy link
Collaborator

@debris debris commented Jul 18, 2018

first pull request that tries to limit number of times the block is being deserialized.

before:

  • uncles were deserialized 3 times during verification

now:

  • uncles are deserialized only once

@debris debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 18, 2018
@5chdn 5chdn added this to the 2.1 milestone Jul 18, 2018
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!

@sorpaas sorpaas added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 25, 2018
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Can't say I fully understand the implications, but it LGTM.


let transactions = block.transactions
.into_iter()
.map(|t| {
let t = engine.verify_transaction_unordered(t, &header)?;
if let Some(max_nonce) = nonce_cap {
if t.nonce >= max_nonce {
return Err(BlockError::TooManyTransactions(t.sender()).into());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reasons why we have return Err(....into()) in some places and bail!(...) in other ones apart from historical?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say it's for historical reasons and lack of style enforcement (I always forget to use bail!). At some point we should migrate from error_chain to failure and we can do a cleanup to have more consistent error handling.

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM. Just a minor formatting nit.


let transactions = block.transactions
.into_iter()
.map(|t| {
let t = engine.verify_transaction_unordered(t, &header)?;
if let Some(max_nonce) = nonce_cap {
if t.nonce >= max_nonce {
return Err(BlockError::TooManyTransactions(t.sender()).into());
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say it's for historical reasons and lack of style enforcement (I always forget to use bail!). At some point we should migrate from error_chain to failure and we can do a cleanup to have more consistent error handling.

}));
}
let expected_uncles = keccak(block_rlp.at(2)?.as_raw());
if &expected_uncles != block.header.uncles_hash(){
Copy link
Contributor

Choose a reason for hiding this comment

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

block.header.uncles_hash(){ that poor { can't breathe. 😛

@debris debris merged commit 143411a into master Jul 25, 2018
@debris debris deleted the block_verification branch July 25, 2018 12:36
@andresilva andresilva mentioned this pull request Jul 25, 2018
ordian added a commit to ordian/parity that referenced this pull request Jul 31, 2018
* 'master' of https://github.com/paritytech/parity:
  removed client error (openethereum#9253)
  Implement EIP-1052 (EXTCODEHASH) and fix several issues in state account cache (openethereum#9234)
  Improve Tracer documentation (openethereum#9237)
  Update Dockerfile (openethereum#9242)
  block cleanup (openethereum#9117)
  Increase the number of sessions. (openethereum#9203)
  add changelog for 1.11.8 stable and 2.0.1 beta (openethereum#9230)
  fix typo (openethereum#9232)
  Fix potential as_usize overflow when casting from U256 in miner (openethereum#9221)
  Allow old blocks from peers with lower difficulty (openethereum#9226)
  Removes duplicate libudev-dev from Dockerfile (openethereum#9220)
  snap: remove ssl dependencies from snapcraft definition (openethereum#9222)
  remove ssl from dockerfiles, closes openethereum#8880 (openethereum#9195)
  Insert PROOF messages for some cases in blockchain (openethereum#9141)
  [Chain] Add more bootnodes (openethereum#9174)
  ethcore: update bn version (openethereum#9217)
  deserialize block only once during verification (openethereum#9161)
  Simple build instruction fix (openethereum#9215)
  Added --tx-queue-no-early-reject flag to disable early tx queue rejects (openethereum#9143)
@ordian ordian mentioned this pull request Aug 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants