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

simplify verification #11249

Merged
merged 4 commits into from
Nov 12, 2019
Merged

simplify verification #11249

merged 4 commits into from
Nov 12, 2019

Conversation

debris
Copy link
Collaborator

@debris debris commented Nov 12, 2019

This pr attempts to simplify a bit a header/block verification process.

Changes:

  • I removed NoopVerifier as it was never used
  • after removal of NoopVerifier, the only remaining implementation of Verifier was CanonVerifier so I also removed it as well as the trait itself
  • last param of fn verify_block_family is no longer optional. (it was always called with Some)
  • I split fn verify_header_params into 2 functions:
    • fn verify_header_params
    • fn verify_header_time which does only timestamp verification of the header
  • I formatted several extremely long lines so they are now more readable

@debris debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Nov 12, 2019
@debris debris requested a review from dvdplm November 12, 2019 01:21
@dvdplm
Copy link
Collaborator

dvdplm commented Nov 12, 2019

* I split `fn verify_header_params` into 2 functions

What was the reason you did this? Like, what's the gain having them split in two?

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

Did you notice any difference in the benches? I don't expect much of an impact, but you never know.

@dvdplm dvdplm added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 12, 2019
@debris
Copy link
Collaborator Author

debris commented Nov 12, 2019

What was the reason you did this? Like, what's the gain having them split in two?

The function was taking an additional argument - bool, which was used to decide whether time of the header should be checked. By splitting it into two functions I reduced the complexity of the code.

Did you notice any difference in the benches? I don't expect much of an impact, but you never know.

No, I forgot to alter the verify_block_family function call in one of the tests. It's fixed in d90e057

@ordian ordian added this to the 2.7 milestone Nov 12, 2019
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Nov 12, 2019
@ordian ordian merged commit db1ea1d into master Nov 12, 2019
@ordian ordian deleted the simplify-verifier branch November 12, 2019 14:05
dvdplm added a commit that referenced this pull request Nov 20, 2019
* master:
  Clarify what first_block `None` means (#11269)
  removed redundant VMType enum with one variant (#11266)
  Ensure jsonrpc threading settings are sane (#11267)
  Return Ok(None) when the registrar contract returns empty slice (#11257)
  Add a benchmark for snapshot::account::to_fat_rlps() (#11185)
  Fix misc compile warnings (#11258)
  simplify verification (#11249)
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.

3 participants