Add feature flag to start from any beacon block in db #15000
Conversation
potuz
force-pushed
the
sync_from_head
branch
2 times, most recently
from
b3af6be to
602cb24
Compare
The new feature flag called --sync-from takes a string that can take values: - `head` or - a 0x-prefixed hex encoded beacon block root. The beacon block root or the head block root has to be known in db and has to be a descendant of the current justified checkpoint.
potuz
force-pushed
the
sync_from_head
branch
from
602cb24 to
9e96de0
Compare
beacon-chain/db/kv/blocks.go
Outdated
| err := s.db.View(func(tx *bolt.Tx) error { | ||
| bkt := tx.Bucket(blocksBucket) | ||
| headRoot := bkt.Get(headBlockRootKey) | ||
| if headRoot == nil { |
There was a problem hiding this comment.
nitpick: I looked at the Get code to make sure there's no way for a "not found" value to be an empty slice, so I'm confident what you have here works, but as a general rule, I think we should prefer len(headRoot) == 0 over headRoot == nil when checking byte slices.
| return nil, err | ||
| } | ||
| // This chain sets the justified checkpoint for every block, including some that are older than jp. | ||
| // This should be however safe for forkchoice at startup. |
There was a problem hiding this comment.
A comment explaining /why/ we are setting a future justified checkpoint on blocks that precede the jp would be helpful.
| log.Infof("Using Head root of %#x", root) | ||
| return root | ||
| } | ||
| root, err := bytesutil.DecodeHexWithLength(headStr, 32) |
There was a problem hiding this comment.
We should make a note to move this into cmd/beacon-chain/blockchain/options.go so we can fail faster if there is a user input validation error.
| st, | ||
| blk.Block().Slot(), | ||
| false, | ||
| }), "could not set head") | ||
| } |
There was a problem hiding this comment.
nitpick: IMO this style is distracting compared to the typical:
if err := s.setHead(&head{root, blk, st, blk.Block().Slot(), false}); err != nil {
return errors.Wrap(err, "could not set head")
}
return nil
There was a problem hiding this comment.
nitpick: we should avoid using positional struct fields where possible.
nitpick: It's also odd that setHead takes a struct pointer, when the pointer seems like a hack to make the func signature smaller - everywhere this is called, the struct is constructed. Looking at setHead, I would suggest breaking these fields out into individual args.
There was a problem hiding this comment.
This is a copy paste from develop, not sure what you want me to do here. I'll merge all the other suggestion and will take whatever you want on this one.
There was a problem hiding this comment.
For now suggestion is just to replace the highlighted lines with:
if err := s.setHead(&head{root, blk, st, blk.Block().Slot(), false}); err != nil {
return errors.Wrap(err, "could not set head")
}
return nil
I'll send a PR later for how I'd like to change setHead.
* Add feature flag to start from any beacon block in db The new feature flag called --sync-from takes a string that can take values: - `head` or - a 0x-prefixed hex encoded beacon block root. The beacon block root or the head block root has to be known in db and has to be a descendant of the current justified checkpoint. * Fix Bugs In Sync From Head (#15006) * Fix Bugs * Remove log * missing save * add tests * Kasey review #1 * Kasey's review #2 * Kasey's review #3 --------- Co-authored-by: Nishant Das <nishdas93@gmail.com>
The new feature flag called --sync-from takes a string that can take
values:
headorThe beacon block root or the head block root has to be known in db and
has to be a descendant of the current justified checkpoint.