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

Get Previous Block Root From State #2003

Closed
wants to merge 11 commits into from
Closed

Get Previous Block Root From State #2003

wants to merge 11 commits into from

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Mar 15, 2019

This resolves #1980, where we retrieve block root from the state instead of performing a read for it from the db. This also resolves issues with initial sync in the case of skipped slots.

@codecov
Copy link

codecov bot commented Mar 15, 2019

Codecov Report

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

@@          Coverage Diff           @@
##           master   #2003   +/-   ##
======================================
  Coverage    69.9%   69.9%           
======================================
  Files         112     112           
  Lines        8814    8814           
======================================
  Hits         6161    6161           
  Misses       2036    2036           
  Partials      617     617

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.

other than spacing, everything looks good!

@@ -8,6 +8,8 @@ import (
"fmt"
"time"

"github.com/prysmaticlabs/prysm/shared/bytesutil"

Copy link
Member

Choose a reason for hiding this comment

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

we should remove the spacing here? stupid IDE... 😞

Copy link
Member

Choose a reason for hiding this comment

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

yes

@nisdas
Copy link
Member Author

nisdas commented Mar 17, 2019

Don't Merge this in yet, its dependent on #2006 getting merged in first

@terencechain
Copy link
Member

Don't Merge this in yet, its dependent on #2006 getting merged in first

Why does this depend on #2006?

func (c *ChainService) ChainHeadRoot() ([32]byte, error) {
head, err := c.beaconDB.ChainHead()
func (c *ChainService) ChainHeadRoot(state *pb.BeaconState) ([32]byte, error) {
root, err := b.BlockRoot(state, state.Slot-1)
Copy link
Member

Choose a reason for hiding this comment

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

we should be careful of this, imagine the following the scenario:
validator current time: 25s
beacon node state slot: 3
Beacon node has just processed the head for slot 3, validator wants whatever the head is at slot 3 but given state.Slot-1, validator can only get the block root at slot 2 but it should be 3

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @nisdas is this still relevant?

Copy link
Contributor

Choose a reason for hiding this comment

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

Bump @nisdas

@prestonvanloon prestonvanloon added this to the Sapphire milestone Mar 21, 2019
@nisdas
Copy link
Member Author

nisdas commented Apr 5, 2019

Not relevant anymore

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.

Use Previous Block Root From State instead of Saved Block
4 participants