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

Remove InvalidParentHash in favor of assert! #8300

Merged
merged 3 commits into from
Apr 5, 2018

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Apr 4, 2018

As the TODO item states (and I manually checked), InvalidParentHash error if happens, indicate a codebase bug. (Only used in verify_parent and parent is always fetched locally.)

So here we remove InvalidParentHash and use an assert! instead.

@sorpaas
Copy link
Collaborator Author

sorpaas commented Apr 4, 2018

I need to look into this. The test indicates that there're cases where InvalidParentHash is probably expected behavior...

@5chdn 5chdn added the A0-pleasereview 🤓 Pull request needs code review. label Apr 4, 2018
@5chdn 5chdn added this to the 1.11 milestone Apr 4, 2018
@5chdn 5chdn added the M4-core ⛓ Core client code / Rust. label Apr 4, 2018
@sorpaas sorpaas changed the title Remove InvalidParentHash in favor of assert! [WIP] Remove InvalidParentHash in favor of assert! Apr 4, 2018
@sorpaas sorpaas changed the title [WIP] Remove InvalidParentHash in favor of assert! Remove InvalidParentHash in favor of assert! Apr 4, 2018
@sorpaas
Copy link
Collaborator Author

sorpaas commented Apr 4, 2018

Okay I think this is fixed now. I should have asserted a true case, but instead asserted false.

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 4, 2018
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. Needs conflict fix.

@andresilva andresilva merged commit e4168c2 into openethereum:master Apr 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.

4 participants