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

Align Epoch Processing With Spec #1703

Merged
merged 15 commits into from
Feb 25, 2019
Merged

Align Epoch Processing With Spec #1703

merged 15 commits into from
Feb 25, 2019

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Feb 25, 2019

This is part of #1565


Description

Write why you are making the changes in this pull request

In epoch processing, when fetching epoch boundary attestations, we were using the JustifiedBlockRootHash32 field of the attestation instead of EpochBoundaryRootHash32, leading current epoch boundary attestations to be nil.

Additionally, our epoch processing was not aligned with the current spec. Finality/justification was completely outdated and not updating correctly. This PR updates all epoch processing details to match v0.3.

There were also some underflow issues. When calculating the previous epoch, always use the helpers.PrevEpoch function instead of helpers.CurrentEpoch() - 1 to prevent underflow.

@rauljordan rauljordan changed the title Fix Epoch Processing Attestations BUG Fix Epoch Processing Attestations Bug Feb 25, 2019
@rauljordan rauljordan added the Bug Something isn't working label Feb 25, 2019
@rauljordan rauljordan self-assigned this Feb 25, 2019
@rauljordan rauljordan added this to the Sapphire milestone Feb 25, 2019
@codecov
Copy link

codecov bot commented Feb 25, 2019

Codecov Report

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

@@          Coverage Diff           @@
##           master   #1703   +/-   ##
======================================
  Coverage    73.2%   73.2%           
======================================
  Files         100     100           
  Lines        7258    7258           
======================================
  Hits         5313    5313           
  Misses       1484    1484           
  Partials      461     461

continue
}
_, voteHeight, err := bs.powChainService.BlockExists(voteHash)
if err != nil {
log.Errorf("Could not fetch block height: %v", err)
log.Debugf("Could not fetch block height: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Why?

@rauljordan rauljordan changed the title Fix Epoch Processing Attestations Bug Align Epoch Processing With Spec Feb 25, 2019
@rauljordan rauljordan merged commit f1114ca into master Feb 25, 2019
@rauljordan rauljordan deleted the finality branch February 25, 2019 21:16
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.

2 participants