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

[WIP] include raw bytes in Ethcore::BlockError #11493

Closed
wants to merge 14 commits into from
Closed

[WIP] include raw bytes in Ethcore::BlockError #11493

wants to merge 14 commits into from

Conversation

niklasad1
Copy link
Collaborator

@niklasad1 niklasad1 commented Feb 14, 2020 •
edited
Loading

This is a draft PR to fix #11403, i.e, include raw rlp block bytes in BlockError instead of at runtime have to ensure that it is returned every time a BadBlock is detected.

But, this doesn't solve #11403 completely because in a few major traits (Engine, ValidatorSet, EpochVerfier) returns EthcoreError without passing raw_bytes. Thus, it is not possible to return BlockError with raw_bytes in some scenarios such as header verification. That is why this PR introduces two redundant patterns Block(BlockError) and BadBlock((BlockError, Bytes)) to deal with that.

Another approach could be split Block and BadBlock into something more logical such as Header and Block but it is quite hard to know where to draw the line and it will requite lots of refactoring.

//cc @ordian @dvdplm (don't read the code, the description should be enough to reason about the basic idea, I need your help)

Benchmarks on goerli blocks

branch: na-ethcore-blockerror
2020-02-14 14:26:24 Import completed in 516 seconds, 1488674 blocks, 2883 blk/s, 627666 transactions, 1215 tx/s, 88240 Mgas, 170 Mgas/s

branch: master
2020-02-14 14:51:50 Import completed in 531 seconds, 1488674 blocks, 2802 blk/s, 627666 transactions, 1181 tx/s, 88240 Mgas, 166 Mgas/s

branch: stable
2020-02-14 15:35:56 Import completed in 493 seconds, 1488674 blocks, 3015 blk/s, 627666 transactions, 1271 tx/s, 88240 Mgas, 178 Mgas/s

@niklasad1 niklasad1 added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Feb 14, 2020
@niklasad1 niklasad1 force-pushed the na-ethcore-blockerror branch 2 times, most recently from af1057c to 143e949 Compare February 17, 2020 10:25
@ordian
Copy link
Collaborator

ordian commented Feb 17, 2020

Is it possible to introduce a new error BadBlock(Bytes)?

@niklasad1
[ethcore error]: introduce BlockErrorWithData
388524f 
The main motivation behind this change is to include `raw rlp block bytes` in `BlockError`.
Instead of at runtime have to ensure that it is returned every time a `BadBlock` is detected.

This commit doesn't solve it completly because in a few major traits (Engine, ValidatorSet, EpochVerfier)
returns `EthcoreError`. Thus, that is why we have `Option<Bytes>` in `BlockErrorWithData` and still have to
ensure that it is not `None` at runtime which is unfortunate. Related to this, all functions/methods that I
have detected that could return more precise error have been changed.

Also, this commit includes a bunch of `nit fixes` that are unrelated to #11403.
@niklasad1
fix test build
db4d6d0
@niklasad1
[benches verification]: fix build
e0a50c6
@niklasad1 niklasad1 force-pushed the na-ethcore-blockerror branch from 143e949 to e0a50c6 Compare February 17, 2020 11:52
@niklasad1
Copy link
Collaborator Author

niklasad1 commented Feb 17, 2020 •
edited
Loading

Is it possible to introduce a new error BadBlock(Bytes)?

I think so but where to add it then? For example, if it would go in EthcoreError we would lose the error cause right? The same applies to BlockError I suppose.

Explain the intention I don't get the point :)

EDIT: I think draft description was bad, I meant Block((BlockError, Bytes))

@ordian
Copy link
Collaborator

ordian commented Feb 17, 2020

I'll have to read the code first, but I meant adding a new enum variant to EthcoreError.

ordian
ordian reviewed Feb 17, 2020
View reviewed changes
ethcore/types/src/errors/ethcore_error.rs Outdated
#[display(fmt = "Block error: {}", _0)]
Block(BlockError),
#[display(fmt = "{}", _0)]
Block(BlockErrorWithData),
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if instead of replacing Block(BlockError) with Block(BlockErrorWithData), we add an additional variant
BadBlock(BlockErrorWithData) and do the conversion in the import/create function?

Copy link
Collaborator Author

@niklasad1 niklasad1 Feb 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

sounds good, thanks

ethcore/verification/src/queue/kind.rs Outdated Show resolved Hide resolved
@ordian
Copy link
Collaborator

ordian commented Feb 21, 2020

@niklasad1 could you extract nice but unrelated changes to a separate PR so we can focus the review on the actual fix?

@niklasad1
Copy link
Collaborator Author

niklasad1 commented Feb 21, 2020 •
edited
Loading

@niklasad1 could you extract nice but unrelated changes to a separate PR so we can focus the review on the actual fix?

Sure, sorry for the noise

niklasad1 added a commit that referenced this pull request Feb 21, 2020
@niklasad1
[cleanup]: various unrelated fixes from #11493
bd37cfa
niklasad1 added a commit that referenced this pull request Feb 21, 2020
@niklasad1
[cleanup]: various unrelated fixes from #11493
c224bd7
@niklasad1 niklasad1 mentioned this pull request Feb 21, 2020
Merged
dvdplm pushed a commit that referenced this pull request Feb 25, 2020
@niklasad1
[ethcore cleanup]: various unrelated fixes from #11493 (#11507)
cd70180 
* [cleanup]: various unrelated fixes from `#11493`

* [revert]: too verbose logging `seal`

* fix nit: don't mix from() and into()
@niklasad1 niklasad1 closed this Feb 29, 2020
ordian added a commit that referenced this pull request Mar 6, 2020
@ordian
Merge branch 'master' into ao-sled
4508b58 
* master: (27 commits)
  Faster kill_garbage (#11514)
  [EngineSigner]: don't sign message with only zeroes (#11524)
  fix compilation warnings (#11522)
  [ethcore cleanup]: various unrelated fixes from `#11493` (#11507)
  Add benchmark for transaction execution (#11509)
  Add Smart Contract License v1.0
  Misc fixes (#11510)
  [dependencies]: unify `rustc-hex` (#11506)
  Activate on-chain randomness in POA Sokol (#11505)
  Grab bag of cleanup (#11504)
  Implement eth/64 (EIP-2364) and drop support for eth/62 (#11472)
  [dependencies]: remove `util/macros` (#11501)
  OpenEthereum bootnodes are added (#11499)
  [ci benches]: use `all-features` (#11496)
  [verification]: make test-build compile standalone (#11495)
  complete null-signatures removal (#11491)
  Include the seal when populating the header for a new block (#11475)
  fix compilation warnings (#11492)
  cargo update -p cmake (#11490)
  update to published rlp-derive (#11489)
  ...
@niklasad1 niklasad1 deleted the na-ethcore-blockerror branch March 13, 2020 17:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ordian ordian ordian left review comments

Assignees
No one assigned
Labels
A3-inprogress ⏳ Pull request is in progress. No review needed at this stage.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rework EthcoreError::Block to include raw bytes of the error cause
2 participants
@niklasad1 @ordian