allow checkpoint or genesis origin; refactoring #9976

kasey · 2021-12-02T23:39:25Z

This branch includes the last bit of refactoring to the blockchain service initialization. The blockchain service initialization code now checks the database to look for a checkpoint sync (aka weak subjectivity sync) block root. If one is found, it uses that root in place of the genesis root for code paths that need a block root value to fall back to in the case of a zero hash. This branch also includes much of the db initialization code, which is not used until the final of 3 branches for this feature, which adds a flag to specify the block+state for a checkpoint sync. This code saves the block and state to the db and marks the block as finalized.

What type of PR is this?

Uncomment one line below and remove others.

Bug fix
Feature
Documentation
Other

What does this PR do? Why is it needed?

First of 3 branches to introduce a minimal weak subjectivity sync implementation.

beacon-chain/db/kv/blocks.go

rauljordan · 2021-12-07T02:52:52Z

beacon-chain/db/kv/blocks.go

+
+	var root [32]byte
+	err := s.db.View(func(tx *bolt.Tx) error {
+		bkt := tx.Bucket(blocksBucket)


Do we want to use the blocks bucket? This is the kind of bucket that could undergo migrations and would require other developers to filter out other values in this bucket that aren't blocks, adding some burden down the line. Probably best to use a separate bucket for this and keep blocks bucket pure in its schema

I think it is fine to do this, other special blocks(genesis,etc) are also saved the same way. Adding another bucket seems excessive here.

Like Nishant says, I wound up using this bucket because that's where the equivalent key for the genesis root is stored. I wouldn't mind creating a new bucket to hold singleton keys like this, with an eye towards moving other keys the next time the main object of their bucket undergoes a migration.

beacon-chain/blockchain/receive_attestation.go

rauljordan · 2021-12-07T02:55:22Z

beacon-chain/db/kv/error.go

+	}
+}
+
+// DBError implements the Error method so that it can be asserted as an error.


Really nice pattern! I mentioned it in #9454

beacon-chain/db/kv/wss.go

beacon-chain/blockchain/service.go

nisdas · 2021-12-07T03:18:49Z

beacon-chain/blockchain/service.go

-		s.finalizedCheckpt = ethpb.CopyCheckpoint(finalizedCheckpoint)
-		s.prevFinalizedCheckpt = ethpb.CopyCheckpoint(finalizedCheckpoint)
-		s.resumeForkChoice(justifiedCheckpoint, finalizedCheckpoint)
+	finalizedState, err = s.cfg.StateGen.Resume(ctx, s.cfg.FinalizedStateAtStartUp)


There is a bit of an edge case here:
https://github.com/prysmaticlabs/prysm/blob/develop/beacon-chain/state/stategen/service.go#L88
https://github.com/prysmaticlabs/prysm/blob/develop/beacon-chain/state/stategen/service.go#L98

If the root in the db is a zero hash, would we be fine with returning this function returning a genesis state ? The reason I am asking, is because in the whole service, genesis root has been replaced with origin root. I am assuming the same logic also applies to stategen.

Or are we assuming here, that in the case of WSS, the checkpoints are already saved into the db, and this edge case isnt really valid/possible ?

are we assuming here, that in the case of WSS, the checkpoints are already saved into the db, and this edge case isnt really valid/possible

yes, good point, I am making that assumption wrt stategen. I almost undertook a bigger refactor to split the block chain service into separate genesis/checkpoint types, but my instinct was that this zero hash logic is so prevalent and subtle that I didn't want to make huge and confusing change all at once. I also had a branch where I implemented an "origin store" that would get either the checkpoint sync block or genesis, but it started growing in scope so I fell back to this simpler option.

This change came in while I was working on the branch and is a little out of step with the concepts in this PR:

prysm/beacon-chain/node/node.go

Line 453 in 790bf03

if r == params.BeaconConfig().ZeroHash {

As an extra defensive check, in the code path linked above, where we detect the zero hash and fall back to genesis, we could first try to look up the origin block root via OriginBlockRoot, and fail if one exists (because that indicates that somehow we tried to start up with an origin checkpoint, but something went wrong with the finalization db entry). If we feel the need to do this check in multiple places I might try out the origin store idea again.

Makes sense, thanks for the explanation ! I think having the check at only one place is fine now, as the assumption is always that the new weak subjectivity checkpoints are baked in.

nisdas · 2021-12-07T03:19:40Z

beacon-chain/blockchain/service.go


-		ss, err := slots.EpochStart(s.finalizedCheckpt.Epoch)
+	if flags.Get().HeadSync {


Unrelated to your PR, but we should remove this whole HeadSync chunk later, no one really uses it and it is not functional afaik even if anyone decided to use it.

I know no one uses it but maybe better to wait for the v3 release? We certainly missed this for v2

noted, we'll come back to this when we're ready to make breaking changes.

nisdas · 2021-12-07T03:45:58Z

beacon-chain/db/kv/error.go

@@ -0,0 +1,46 @@
+package kv


This is great

nisdas · 2021-12-07T03:48:26Z

beacon-chain/db/kv/wss.go

+// (ex: an open file) prepares the database so that the beacon node can begin
+// syncing, using the provided values as their point of origin. This is an alternative
+// to syncing from genesis, and should only be run on an empty database.
+func (s *Store) SaveOrigin(ctx context.Context, stateReader, blockReader io.Reader) error {


I guess one thing that is tricky and that we need to be aware of would be what if a user provides a phase0/altair/merge WSS state ? Currently the code assumes that the user only provides an altair block/state.

Agreed, copy/pasting from my response on this to Raul:
Working on making this more future proof in cp-sync-sniff-origin-init 65912a9
Still need to do a bit of testing but i think that work will decouple this code from relying on a particular proto struct.

terencechain · 2021-12-07T08:05:06Z

beacon-chain/state/v2/state_trie.go

+// InitializeFromSSZReader can be used when the source for a serialized BeaconState object
+// is an io.Reader. This allows client code to remain agnostic about whether the data comes
+// from the network or a file without needing to read the entire state into mem as a large byte slice.
+func InitializeFromSSZReader(r io.Reader) (*BeaconState, error) {


Curious how we can make this more forward-compatible say when BeaconStateMerge comes around. Do we duplicate code into v3 pkg?

Everyone landed on the same (totally valid) concern haha, copying one more time ;)
Working on making this more future proof in cp-sync-sniff-origin-init 65912a9
Still need to do a bit of testing but i think that work will decouple this code from relying on a particular proto struct.

beacon-chain/db/kv/wss.go

terencechain · 2021-12-07T08:08:59Z

beacon-chain/blockchain/service.go


-		ss, err := slots.EpochStart(s.finalizedCheckpt.Epoch)
+	if flags.Get().HeadSync {


I know no one uses it but maybe better to wait for the v3 release? We certainly missed this for v2

some quick readability improvements and simplifying the logic enforcing the startup ordering of the attestation processing routine

beacon-chain/blockchain/receive_attestation.go

nisdas

LGTM

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

use log.WithError for aggregation friendliness Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

kasey requested a review from a team as a code owner December 2, 2021 23:39

kasey requested review from jmozah, rauljordan and nisdas December 2, 2021 23:39

kasey force-pushed the cp-sync-origin-root branch 2 times, most recently from 058d0bf to 5edcfbd Compare December 2, 2021 23:53

kasey mentioned this pull request Dec 3, 2021

WSS refactor blockchain.Service Start() #9854

Closed

kasey force-pushed the cp-sync-origin-root branch 2 times, most recently from 0ae7a12 to 6dcc6fc Compare December 4, 2021 00:05

kasey changed the title ~~WIP: allow checkpoint or genesis origin; refactoring~~ allow checkpoint or genesis origin; refactoring Dec 6, 2021

rauljordan mentioned this pull request Dec 7, 2021

Encapsulate Nil Checks #9454

Open

rauljordan reviewed Dec 7, 2021

View reviewed changes

nisdas reviewed Dec 7, 2021

View reviewed changes

terencechain reviewed Dec 7, 2021

View reviewed changes

kasey added 2 commits December 8, 2021 08:47

allow checkpoint or genesis origin; refactoring

f955a88

some quick readability improvements and simplifying the logic enforcing the startup ordering of the attestation processing routine

address PR feedback

26a957b

kasey force-pushed the cp-sync-origin-root branch from 5617741 to 26a957b Compare December 8, 2021 15:25

gofmt

f500ffb

rauljordan previously approved these changes Dec 8, 2021

View reviewed changes

prestonvanloon reviewed Dec 8, 2021

View reviewed changes

beacon-chain/blockchain/receive_attestation.go Outdated Show resolved Hide resolved

beacon-chain/blockchain/receive_attestation.go Outdated Show resolved Hide resolved

beacon-chain/blockchain/receive_attestation.go Outdated Show resolved Hide resolved

nisdas previously approved these changes Dec 9, 2021

View reviewed changes

Update beacon-chain/blockchain/receive_attestation.go

06c1866

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

kasey dismissed stale reviews from nisdas and rauljordan via 06c1866 December 9, 2021 15:35

kasey and others added 2 commits December 9, 2021 09:37

Apply suggestions from code review

568bfa3

use log.WithError for aggregation friendliness Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

Merge branch 'develop' into cp-sync-origin-root

39d0940

rauljordan approved these changes Dec 9, 2021

View reviewed changes

Merge branch 'develop' into cp-sync-origin-root

fac8cb9

rauljordan added OK to merge Ready For Review A pull request ready for code review labels Dec 9, 2021

Merge refs/heads/develop into cp-sync-origin-root

d3521d8

prylabs-bulldozer bot merged commit 3c61cc7 into develop Dec 9, 2021

delete-merged-branch bot deleted the cp-sync-origin-root branch December 9, 2021 22:23

kasey mentioned this pull request Dec 14, 2021

WIP: beacon node cli flag for specifying origin block+state #10017

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow checkpoint or genesis origin; refactoring #9976

allow checkpoint or genesis origin; refactoring #9976

kasey commented Dec 2, 2021

rauljordan Dec 7, 2021

nisdas Dec 7, 2021

kasey Dec 7, 2021

rauljordan Dec 7, 2021

nisdas Dec 7, 2021

kasey Dec 7, 2021 •

edited

nisdas Dec 7, 2021

nisdas Dec 7, 2021

terencechain Dec 7, 2021

kasey Dec 8, 2021

nisdas Dec 7, 2021

nisdas Dec 7, 2021

kasey Dec 7, 2021

terencechain Dec 7, 2021

kasey Dec 7, 2021

terencechain Dec 7, 2021

nisdas left a comment


		ss, err := slots.EpochStart(s.finalizedCheckpt.Epoch)
		if flags.Get().HeadSync {

allow checkpoint or genesis origin; refactoring #9976

allow checkpoint or genesis origin; refactoring #9976

Conversation

kasey commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasey Dec 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisdas left a comment

Choose a reason for hiding this comment

kasey Dec 7, 2021 •

edited