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

Don't insert head to proto array DAG as index 0 #4749

Merged
merged 2 commits into from Feb 5, 2020
Merged

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Feb 5, 2020

Looking at the Vizgraph, I was confused on why initial sync and regular sync don’t belong in the same branch. As we didn’t think it was that big of the deal, the following warning log was temporary as it eventually goes away:
[2020-02-04 15:31:07] WARN blockchain: Skip head update for slot 187055: head at slot 187008 with weight 4831 is not eligible, finalizedEpoch 5842 != 5843, justifiedEpoch 5843 != 5844

After looking at this more, we realized to resume syncing, node gets the 1.) head block then start start sync from 2.) last finalized check point. So fork choice insert head block into the array as index 0. The issue is illustrated as the following:

Index 0, block_at_slot: 888 (head block)
Index 1, block_at_slot: 800 (last finalized block)
Index 2, block_at_slot: 801 (last finalized block)
Index 3, block_at_slot: 802 (last finalized block)
…
…
Index 889, block_at_slot: 889 (head_block_slot + 1)

What's wrong with above is the node at index 889 got built on top of index 0 instead of index 888 and became its separate branch. So everything from index 1 to index 888 basically didn’t count which is why it takes some time to get a new head when node first starts. Bottom branch is 1 to 888. And top is 889 built on top of index 0. The graph proved it:

Screen Shot 2020-02-04 at 3 32 30 PM

To fix this, a node simply should not insert head to index 0, there's just no reason to do it.

@terencechain terencechain self-assigned this Feb 5, 2020
@terencechain terencechain added Bug Something isn't working Forkchoice labels Feb 5, 2020
@codecov
Copy link

codecov bot commented Feb 5, 2020

Codecov Report

Merging #4749 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #4749      +/-   ##
==========================================
- Coverage   43.12%   43.11%   -0.01%     
==========================================
  Files         203      203              
  Lines       15257    15258       +1     
==========================================
  Hits         6579     6579              
- Misses       7562     7563       +1     
  Partials     1116     1116

@nisdas nisdas merged commit 8ad174f into master Feb 5, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-save-head-root branch February 5, 2020 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants