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.

Sign up for GitHub

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

Jump to bottom

[ETCM-709] Improve ommers validations. #948

Merged
merged 1 commit into from
Apr 14, 2021
Merged

[ETCM-709] Improve ommers validations. #948

merged 1 commit into from
Apr 14, 2021

Conversation

leo-bogastry
Copy link
Contributor

@leo-bogastry leo-bogastry commented Mar 23, 2021 •
edited
Loading

Description

In testnet we observed the following error message Block import error BlockImportFailed(OmmersAncestorsError), but looking at the code it was not possible to know exactly why the validation failed.
The goal of this PR is to allow for better debugging in case we observe the same error message again.

Proposed Solution

Replaced OmmersAncestorsError by more specific OmmerIsAncestorError and OmmerParentIsNotAncestorError.
Renamed OmmersNotValidError to OmmersHeaderError and added the underlying reason for failure
Refactored validateOmmersAncestors method.
Refactored validateOmmersHeaders method to not hide the underlying failing reason.
Reorganized some packages because some classes were not in the correct package and/or the Spec was not in the correct package. Renamed some specs to better reflect the class being tested
Raised the log level of some logs in BlockFetcher because there are barely any logs this actor when debug logs are disabled

Testing

Connected to Sagano with FastSync off and RegularSync is importing blocks properly

@leo-bogastry leo-bogastry force-pushed the feature/ETCM-709-improve-ommers-validation branch 3 times, most recently from 416518a to 932853b Compare March 24, 2021 12:35
@leo-bogastry leo-bogastry requested a review from milenkara March 24, 2021 13:19
jvdp
jvdp approved these changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@jvdp jvdp left a comment

Choose a reason for hiding this comment

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

Better errors and simpler code is better 👍

I have no opinion about the log level stuff. I think we might want to bump default log level for end users to warning anyway? There's room for a story to really think about what we want out of logging, I think.

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala Show resolved Hide resolved
src/main/scala/io/iohk/ethereum/consensus/validators/std/StdOmmersValidator.scala Outdated Show resolved Hide resolved
src/main/scala/io/iohk/ethereum/consensus/ethash/validators/OmmersValidator.scala Outdated Show resolved Hide resolved
src/main/scala/io/iohk/ethereum/consensus/validators/std/StdOmmersValidator.scala Outdated Show resolved Hide resolved
src/main/scala/io/iohk/ethereum/consensus/validators/std/StdOmmersValidator.scala Show resolved Hide resolved
src/test/scala/io/iohk/ethereum/consensus/validators/std/StdOmmersValidatorSpec.scala Show resolved Hide resolved
@leo-bogastry leo-bogastry force-pushed the feature/ETCM-709-improve-ommers-validation branch 5 times, most recently from 7e4cd10 to 31c4792 Compare April 2, 2021 08:21
1015bit
1015bit reviewed Apr 7, 2021
View reviewed changes
Copy link
Contributor

@1015bit 1015bit left a comment

Choose a reason for hiding this comment

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

Looks good! Some comments regarding the refactoring and log levels.

src/main/scala/io/iohk/ethereum/consensus/validators/std/StdOmmersValidator.scala Outdated Show resolved Hide resolved
src/main/scala/io/iohk/ethereum/consensus/validators/std/StdOmmersValidator.scala

if (ommersThatAreAncestors.nonEmpty) Left(OmmerIsAncestorError)
else if (!ommersParentsAreAllAncestors) Left(OmmerParentIsNotAncestorError)
else Right(OmmersValid)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't default to Right in the validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because no error was found, what would you return there then?

Copy link
Contributor

@1015bit 1015bit Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I gave an example below why defaulting to Right is problematic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but where?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, missed the comment. I was referring to #948 (comment)

In general I'd say the problem with defaulting to Right is that it will also return Right for every unhandled/unexpected case while the other way around is more restrictive, making the errors more visible.

In this specific case I'd re-add at the very least the check that was removed in https://github.com/input-output-hk/mantis/pull/948/files#diff-4ffb3286be2984d1679ee76bbb2e5e404f64147f1d77ab0eb95f8031a1379635L160

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the same check that will be added in ETCM-776

Copy link
Contributor

Choose a reason for hiding this comment

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

ETCM-776 was created after I wrote that comment.

...and I still think that defaulting to Right in validations is less secure.

Copy link
Contributor Author

@leo-bogastry leo-bogastry Apr 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I understand the philosophy of what you are saying, but I also think that in this case it would really overcomplicate the code. The whole reason why I was not able to understand why a block validation failed in testnet was because those two separate errors OmmerIsAncestorError and OmmerParentIsNotAncestorError did not exist.
This code is only making these two specific validations otherwise there is no error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I can propose is a bigger refactoring, than ETCM-776. GetNBlocksBack should be part of the for comprehension, inside the more generic validate method and validateOmmersAncestors could be divided into two method (each one returning only one error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have rewritten ETCM-776

src/main/scala/io/iohk/ethereum/consensus/validators/std/StdOmmersValidator.scala Show resolved Hide resolved
src/main/scala/io/iohk/ethereum/consensus/ethash/validators/OmmersValidator.scala Outdated Show resolved Hide resolved
src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala Outdated Show resolved Hide resolved
src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala Outdated Show resolved Hide resolved
@leo-bogastry leo-bogastry force-pushed the feature/ETCM-709-improve-ommers-validation branch 7 times, most recently from 0c2d3b4 to 79a3f27 Compare April 12, 2021 09:21
@leo-bogastry
Copy link
Contributor Author

Tested FastSync and RegularSync once again in etc

1015bit
1015bit reviewed Apr 13, 2021
View reviewed changes
src/main/scala/io/iohk/ethereum/consensus/validators/std/StdOmmersValidator.scala Outdated
else {
val ommersHeaderError = validationsResult
.collect { case Left(error) => error }
.map(error => OmmersHeaderError(List(error)))
Copy link
Contributor

Choose a reason for hiding this comment

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

.map and .reduce are not needed...can be simplified to

val errors = validationsResult.collect { case Left(error) => error }.toList
Left(OmmersHeaderError(errors))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I really overcomplicated that :)

Copy link
Contributor

Choose a reason for hiding this comment

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

:)

@leo-bogastry leo-bogastry force-pushed the feature/ETCM-709-improve-ommers-validation branch from 79a3f27 to 8753c48 Compare April 13, 2021 07:46
@leo-bogastry leo-bogastry force-pushed the feature/ETCM-709-improve-ommers-validation branch from 8753c48 to 5fb0381 Compare April 13, 2021 09:11
[ETCM-709] Improve ommers validations
2ac53c6 
Replaced OmmersAncestorsError by more specific OmmerIsAncestorError and OmmerParentIsNotAncestorError.
Renamed OmmersNotValidError to OmmersHeaderError and added the underlying reason for failure
Refactored validateOmmersAncestors method.
Refactored validateOmmersHeaders method to not hide the underlying failing reason.
Reorganized some packages because some classes were not in the correct package and/or the Spec was not in the correct package. Renamed some specs to better reflect the class being tested
@leo-bogastry leo-bogastry force-pushed the feature/ETCM-709-improve-ommers-validation branch from 5fb0381 to 2ac53c6 Compare April 14, 2021 10:43
@leo-bogastry leo-bogastry merged commit 25c3966 into develop Apr 14, 2021
@leo-bogastry leo-bogastry deleted the feature/ETCM-709-improve-ommers-validation branch April 14, 2021 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzajkowski dzajkowski dzajkowski left review comments

@jvdp jvdp jvdp approved these changes

@1015bit 1015bit 1015bit approved these changes

@AnastasiiaL AnastasiiaL Awaiting requested review from AnastasiiaL

@bsuieric bsuieric Awaiting requested review from bsuieric

@robinraju robinraju Awaiting requested review from robinraju

@grale88 grale88 Awaiting requested review from grale88

@milenkara milenkara Awaiting requested review from milenkara

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@leo-bogastry @jvdp @1015bit @dzajkowski