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

Fix Mismatched BlockRoots In State Replay #7559

Merged
merged 7 commits into from
Oct 18, 2020
Merged

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Oct 17, 2020

What type of PR is this?

Bug Fix

What does this PR do? Why is it needed?

  • During times without finality the database doesn't return the same number of blocks and its
    respective roots in-order. This is especially true for unfinalized blocks.

Follows on from #7019 and #7028

Which issues(s) does this PR fix?

Other notes for review

@nisdas nisdas requested a review from a team as a code owner October 17, 2020 13:45
@nisdas nisdas added the Ready For Review A pull request ready for code review label Oct 17, 2020
@codecov
Copy link

codecov bot commented Oct 17, 2020

Codecov Report

Merging #7559 into master will decrease coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7559      +/-   ##
==========================================
- Coverage   61.86%   61.71%   -0.16%     
==========================================
  Files         424      424              
  Lines       30068    29884     -184     
==========================================
- Hits        18602    18443     -159     
+ Misses       8487     8481       -6     
+ Partials     2979     2960      -19     

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Then the issue is on the block roots retrieval in db? Should we fix it on that end? Can we reproduce this with a DB test?

Not 100% sure about this because it feels like the real issue is getting masked away and we should fix it instead or at least open a tracking issue

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Please add regression test if possible

@nisdas
Copy link
Member Author

nisdas commented Oct 18, 2020

Then the issue is on the block roots retrieval in db? Should we fix it on that end? Can we reproduce this with a DB test?

So there is a slight race with retrieval of roots and their respective blocks. In times of network turbulence, you could see occasionally the 2 calls retrieving differing results. We shouldn't be using it this way, as there is always a chance of a mismatch. What I could instead do is expose the Blocks API to return the roots also.

prestonvanloon
prestonvanloon previously approved these changes Oct 18, 2020
terencechain
terencechain previously approved these changes Oct 18, 2020
@prestonvanloon prestonvanloon merged commit b502876 into master Oct 18, 2020
@delete-merged-branch delete-merged-branch bot deleted the fixMisMatchedRoots branch October 18, 2020 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants