Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop check_seal support from validate_header #1909

Merged

Conversation

@cburgdorf
Copy link
Contributor

cburgdorf commented Jan 10, 2020

What was wrong?

VM.validate_header recently became an instance method (was a classmethod before). The only reason why it became an instance method is because it has an optional seal check and seal checks can only be performed on VM instances.

How was it fixed?

@carver suggested to take the seal check out of this method. I initially was a bit skeptical about that as it comes with the risk of simply forgetting about the check. But in practice, there are few places where we explicitly need to call this and higher level code usually deals with APIs such as validate_chain that cover the seal check internally.

So yeah, this PR drops check_seal from validate_header, makes it a classmethod and changes all call sites accordingly.

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

@cburgdorf cburgdorf force-pushed the cburgdorf:christoph/refactor/validate_header branch from ccc51d4 to bc5e54b Jan 10, 2020
@cburgdorf cburgdorf mentioned this pull request Jan 10, 2020
0 of 2 tasks complete
@cburgdorf cburgdorf force-pushed the cburgdorf:christoph/refactor/validate_header branch 2 times, most recently from 3195739 to befb75f Jan 10, 2020
@cburgdorf cburgdorf requested a review from carver Jan 10, 2020
try:
self._consensus.validate_seal(header)
except ValidationError as exc:
self.cls_logger.warning(

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Jan 10, 2020

Author Contributor

I moved this here because I think the standard logging on a failed seal check is a net good for debugging etc.

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Jan 10, 2020

Member

So one potentially negative effect of this is that if someone sent us an invalid header in trinity and it hits this method then trinity will output this warning message to the console. Is that what we want?

This comment has been minimized.

Copy link
@carver

carver Jan 10, 2020

Contributor

Is that what we want?

Hm, not sure. Geth does print out warnings about bad blocks, and it causes some concern/confusion.

Logging is clearly good. I'm ambivalent on whether it should be a warning.

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Jan 10, 2020

Author Contributor

What if this default log entry would only go into debug or even debug2 instead?

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Jan 10, 2020

Member

debug seems appropriate.

@carver
carver approved these changes Jan 11, 2020
Copy link
Contributor

carver left a comment

Yup, actually I am less ambivalent about this than I thought. I like it quite a bit.

The boolean flag that triggers other checks underneath had a bit of a code smell to me that I'm happy to see cleaned up.

Yeah, it comes with the risk of forgetting to check_seal(), but I think the other benefits outweigh it. 👍

@cburgdorf cburgdorf force-pushed the cburgdorf:christoph/refactor/validate_header branch from befb75f to 4a4ee2c Jan 13, 2020
@cburgdorf cburgdorf merged commit 239c72c into ethereum:master Jan 13, 2020
21 checks passed
21 checks passed
ci/circleci: py36-benchmark Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py36-database Your tests passed on CircleCI!
Details
ci/circleci: py36-docs Your tests passed on CircleCI!
Details
ci/circleci: py36-lint Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-byzantium Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-constantinople Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-frontier Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-homestead Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-istanbul Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-petersburg Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-spurious_dragon Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-tangerine_whistle Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-transition Your tests passed on CircleCI!
Details
ci/circleci: py36-transactions Your tests passed on CircleCI!
Details
ci/circleci: py36-vm Your tests passed on CircleCI!
Details
ci/circleci: py37-core Your tests passed on CircleCI!
Details
ci/circleci: py37-database Your tests passed on CircleCI!
Details
ci/circleci: py37-lint Your tests passed on CircleCI!
Details
ci/circleci: py37-transactions Your tests passed on CircleCI!
Details
ci/circleci: py37-vm Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.