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

Fixed the error during dry run #2365

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Oct 16, 2024

Because of race condition it was possible that dry run will fail. I moved the check to production, where we really need to require this behavior. But in the case of the dry run we only care about previous block existence.

Checklist

  • New behavior is reflected in tests

Before requesting review

  • I have reviewed the code myself

@xgreenx xgreenx requested a review from a team October 16, 2024 11:29
@xgreenx xgreenx self-assigned this Oct 16, 2024
Comment on lines -362 to -363
block.entity.header().time();

Copy link
Member

Choose a reason for hiding this comment

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

nice catch

@rymnc rymnc requested a review from a team October 16, 2024 11:34
AurelienFT
AurelienFT previously approved these changes Oct 16, 2024
acerone85
acerone85 previously approved these changes Oct 16, 2024
Copy link
Contributor

@acerone85 acerone85 left a comment

Choose a reason for hiding this comment

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

Did not go in depth with the review, but I can follow the main idea behind the fix.
LGTM

@xgreenx xgreenx changed the base branch from release/v0.40.0 to master October 17, 2024 11:52
@xgreenx xgreenx dismissed stale reviews from acerone85 and AurelienFT October 17, 2024 11:52

The base branch was changed.

AurelienFT
AurelienFT previously approved these changes Oct 17, 2024
@xgreenx xgreenx enabled auto-merge (squash) October 30, 2024 09:31
Copy link
Contributor

@rafal-ch rafal-ch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -649,27 +649,33 @@ mod dry_run {
}

#[tokio::test]
async fn dry_run__errors_early_if_height_is_lower_than_chain_tip() {
Copy link
Member

Choose a reason for hiding this comment

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

I did a quick scan but couldn't see a test for this on the produce or produce_predefined. Maybe I missed it. Do you think we need that coverage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have test case for BlockHeightShouldBeHigherThanPrevious error.

image

But after introduction of the state rewind feature, I don't think that BlockHeightShouldBeHigherThanPrevious error makes a lot of sense. Basically importer already is responsible and upgradable executor do this kind of check inside, some maybe we can just remove it from the block producer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants