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

Init-sync queue assert order and uniqueness of blocks emitted #6146

Merged
merged 17 commits into from Jun 8, 2020

Conversation

farazdagi
Copy link
Contributor

@farazdagi farazdagi commented Jun 5, 2020

What type of PR is this?

Feature

What does this PR do? Why is it needed?

  • Duplicate blocks are filtered out.
  • Unnecessary blocks stream notification removed.

Which issues(s) does this PR fix?

Fixes #5857

Other notes for review

  • Redundant block requests are in core of the init-sync process: peers can provide wrong data, slot ranges need to be re-requested, and processing redone. Therefore, extra caution must be applied when filtering out blocks.
  • Current implementation assumes that if processBlock function didn't end up in error, block can be considered as processed, and its slot is logged. All future requests to processBlock must be made for a higher slots.
  • When implemented in Check Whether Block Is Already Processed #5850 we had an issue which is summarized by Terence as following:

Archived point is set to 256 slots
In attempt1, the node is syncing between slot 256 to 512.
The node restarts at 312, in attempt2, the node resyncs from 256 but now the node wouldn’t process the blocks between 256 to 312 anymore and that’s detrimental to new-state-mgmt

This problem will not occur with filtering at the round robin level - as on restart, slot's after the head slot will be re-checked again.

@farazdagi farazdagi self-assigned this Jun 5, 2020
@codecov
Copy link

codecov bot commented Jun 7, 2020

Codecov Report

Merging #6146 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6146   +/-   ##
=======================================
  Coverage   59.92%   59.92%           
=======================================
  Files         323      323           
  Lines       27443    27443           
=======================================
  Hits        16446    16446           
  Misses       8769     8769           
  Partials     2228     2228           

@farazdagi farazdagi marked this pull request as ready for review June 8, 2020 14:00
@farazdagi farazdagi requested a review from a team as a code owner June 8, 2020 14:00
@farazdagi farazdagi added OK to merge Ready For Review A pull request ready for code review and removed In Progress labels Jun 8, 2020
@prylabs-bulldozer prylabs-bulldozer bot merged commit ba63186 into master Jun 8, 2020
@delete-merged-branch delete-merged-branch bot deleted the init-sync-queue-dedup-blocks branch June 8, 2020 14:52
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.

Assert that only non-processed blocks are sent in init-sync queue
4 participants