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

Track finalized block root index and filter in blocks by range #3873

Merged
merged 9 commits into from
Oct 29, 2019

Conversation

prestonvanloon
Copy link
Member

@prestonvanloon prestonvanloon commented Oct 28, 2019

Resolves #3858.
Resolves #3819.
Resolves #3848.

This PR keeps track of finalized block roots in the database. This index can then be queried to ensure that the block is a finalized block. When serving blocks at RPC, the beacon chain node will query the index to filter non-finalized blocks.

@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

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

@@           Coverage Diff           @@
##           master    #3873   +/-   ##
=======================================
  Coverage   59.69%   59.69%           
=======================================
  Files         191      191           
  Lines       12177    12177           
=======================================
  Hits         7269     7269           
  Misses       3913     3913           
  Partials      995      995

@prestonvanloon
Copy link
Member Author

I want to handle the case where the index is being built, but the process is interrupted. This could lead to an invalid / incomplete index.

@@ -38,7 +38,7 @@ go_image(
goarch = "amd64",
goos = "linux",
importpath = "github.com/prysmaticlabs/prysm/validator",
pure = "on",
pure = "off",
Copy link
Member

Choose a reason for hiding this comment

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

?

@@ -77,7 +77,7 @@ docker_push(
go_binary(
name = "validator",
embed = [":go_default_library"],
pure = "on", # Enabled unless there is a valid reason to include cgo dep.
pure = "off", # Enabled unless there is a valid reason to include cgo dep.
Copy link
Member

Choose a reason for hiding this comment

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

same here, there is no cgo dep

Copy link
Member Author

Choose a reason for hiding this comment

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

oops, will revert these


container := &dbpb.FinalizedBlockRootContainer{
ParentRoot: block.ParentRoot,
ChildRoot: previousRoot,
Copy link
Member

Choose a reason for hiding this comment

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

The first time the walk is performed , previousRoot will be an empty byte slice

Copy link
Member Author

Choose a reason for hiding this comment

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

That is expected, there is no finalized child root yet. We just learned of a new finalized block and this is the highest finalized block to exist so far.

// index bucket or genesis block root.
var walk = func() error {
for {
if bytes.Equal(root, genesisRoot) {
Copy link
Member

Choose a reason for hiding this comment

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

this walks all the way back to genesis each time we updated the finalized checkpoint, shouldn't it only walk back to the previous finalized checkpoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, see line 67

traceutil.AnnotateError(span, err)
return err
}
return bkt.Put(block.ParentRoot, enc)
Copy link
Member

Choose a reason for hiding this comment

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

ah sorry just saw this, its good then in that case

featureconfig.Init(fc)
}

func TestStore_IsFinalizedBlock(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to cover a few more cases ? ex: Across multiple epochs, with skipped slots at epoch boundaries,etc

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, I'll think about overnight. There isn't any different code path for mulitple epochs with skipped slots. Epoch boundries aren't relevant here. As long as the finalized chain is a chain then this should work.

terencechain
terencechain previously approved these changes Oct 29, 2019
nisdas
nisdas previously approved these changes Oct 29, 2019
@prestonvanloon prestonvanloon dismissed stale reviews from nisdas and terencechain via 7397696 October 29, 2019 14:51
@prestonvanloon prestonvanloon merged commit 0481eb4 into master Oct 29, 2019
@delete-merged-branch delete-merged-branch bot deleted the finalized_block_root_index branch October 29, 2019 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Critical Highest, immediate priority item
Projects
None yet
3 participants