-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Block backfilling #12968
Merged
Merged
Block backfilling #12968
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
fab874d
backfill service
kasey 10abf71
fix bug where origin state is never unlocked
kasey ea73296
support mvslice states
kasey b20a3d7
use renamed interface
kasey c497112
refactor db code to skip block cache for backfill
kasey bc26f1b
lint
kasey 316e637
add test for verifier.verify
kasey 7619573
enable service in service init test
kasey 1a463c0
cancellation cleanup
kasey 6722712
adding nil checks to configset juggling
kasey 1ef5ba7
assume blocks are available by default
kasey e577f2f
block saving path refactor and bugfix
kasey d25d43c
fix fillback test
kasey a76ec16
fix BackfillStatus init tests
kasey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| package kv | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "github.com/pkg/errors" | ||
| "github.com/prysmaticlabs/prysm/v4/proto/dbval" | ||
| bolt "go.etcd.io/bbolt" | ||
| "go.opencensus.io/trace" | ||
| "google.golang.org/protobuf/proto" | ||
| ) | ||
|
|
||
| // SaveBackfillStatus encodes the given BackfillStatus protobuf struct and writes it to a single key in the db. | ||
| // This value is used by the backfill service to keep track of the range of blocks that need to be synced. It is also used by the | ||
| // code that serves blocks or regenerates states to keep track of what range of blocks are available. | ||
| func (s *Store) SaveBackfillStatus(ctx context.Context, bf *dbval.BackfillStatus) error { | ||
| _, span := trace.StartSpan(ctx, "BeaconDB.SaveBackfillStatus") | ||
| defer span.End() | ||
| bfb, err := proto.Marshal(bf) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return s.db.Update(func(tx *bolt.Tx) error { | ||
| bucket := tx.Bucket(blocksBucket) | ||
| return bucket.Put(backfillStatusKey, bfb) | ||
| }) | ||
| } | ||
|
|
||
| // BackfillStatus retrieves the most recently saved version of the BackfillStatus protobuf struct. | ||
| // This is used to persist information about backfill status across restarts. | ||
| func (s *Store) BackfillStatus(ctx context.Context) (*dbval.BackfillStatus, error) { | ||
| _, span := trace.StartSpan(ctx, "BeaconDB.BackfillStatus") | ||
| defer span.End() | ||
| bf := &dbval.BackfillStatus{} | ||
| err := s.db.View(func(tx *bolt.Tx) error { | ||
| bucket := tx.Bucket(blocksBucket) | ||
| bs := bucket.Get(backfillStatusKey) | ||
| if len(bs) == 0 { | ||
| return errors.Wrap(ErrNotFound, "BackfillStatus not found") | ||
| } | ||
| return proto.Unmarshal(bs, bf) | ||
| }) | ||
| return bf, err | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| package kv | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/prysmaticlabs/prysm/v4/encoding/bytesutil" | ||
| "github.com/prysmaticlabs/prysm/v4/proto/dbval" | ||
| "github.com/prysmaticlabs/prysm/v4/testing/require" | ||
| "google.golang.org/protobuf/proto" | ||
| ) | ||
|
|
||
| func TestBackfillRoundtrip(t *testing.T) { | ||
| db := setupDB(t) | ||
| b := &dbval.BackfillStatus{} | ||
| b.LowSlot = 23 | ||
| b.LowRoot = bytesutil.PadTo([]byte("low"), 32) | ||
| b.LowParentRoot = bytesutil.PadTo([]byte("parent"), 32) | ||
| m, err := proto.Marshal(b) | ||
| require.NoError(t, err) | ||
| ub := &dbval.BackfillStatus{} | ||
| require.NoError(t, proto.Unmarshal(m, ub)) | ||
| require.Equal(t, b.LowSlot, ub.LowSlot) | ||
| require.DeepEqual(t, b.LowRoot, ub.LowRoot) | ||
| require.DeepEqual(t, b.LowParentRoot, ub.LowParentRoot) | ||
|
|
||
| ctx := context.Background() | ||
| require.NoError(t, db.SaveBackfillStatus(ctx, b)) | ||
| dbub, err := db.BackfillStatus(ctx) | ||
| require.NoError(t, err) | ||
|
|
||
| require.Equal(t, b.LowSlot, dbub.LowSlot) | ||
| require.DeepEqual(t, b.LowRoot, dbub.LowRoot) | ||
| require.DeepEqual(t, b.LowParentRoot, dbub.LowParentRoot) | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean I cannot backfill any further? I was imagining one could backfill all the way back to genesis, thus having the best of both worlds: an archival node that follows the head as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the PR description, we'll do a follow up feature to add a flag like
--backfill-to-epochthat will allow the user to specify an earlier backfill target:--backfill-to-epoch=0to go all the way to genesis. Actually this reminds me that I need to file a separate issue for this, currently we only have #13003 which is related but doesn't cover it. Just added this one #13031There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is the minimum by the spec, shouldn't we have this value by default ?
Reasoning is that on node default's all of the beacon history is always maintained. If a user does not care about it, they can then just give a recent epoch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My stance is that the main purpose of beacon nodes is to participate in the consensus protocol. Following from the principals of finalization and weak subjectivity, most nodes are really only interested in blocks forward from the beginning of the weak subjectivity period. Older history is not useful for participating in consensus, it is only interesting for archival purposes. So our default behavior should not be to download older history that is not useful to most of the network.
I am open to having my mind changed on this issue :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than participating in consensus, having the ability to download and serve historical blocks should be an expected duty of a normal node. There is no alternate protocol to serve historical blocks from the consensus layer network. If all nodes simply stop saving historical blocks, you would end up with very few peers who would have full chain histories. It is the same reason execution layer nodes also save all historical blocks even when snap syncing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @kasey here and would leave this default. Blocks are kept in the EL and that allows state recovery. We do not need the CL blocks more than for accounting purposes on archive nodes, and it's their business to have these data, instead of the default user providing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strongly disagree, its the same reason execution clients haven not relinquished support for serving/persisting historical blocks. Even though those blocks are purely used to access historical state which is of no use now, users are still able to do so. We can continue the conversation offline