beacon/light: addressed review comments

beacon/light/canonical.go

@@ -31,7 +31,7 @@ import (
 // to avoid concurrent access.
 type canonicalStore[T any] struct {
 	keyPrefix []byte
-	periods   Range
+	periods   periodRange
 	cache     *lru.Cache[uint64, T]
 	encode    func(T) ([]byte, error)
 	decode    func([]byte) (T, error)
@@ -39,8 +39,8 @@ type canonicalStore[T any] struct {
 
 // newCanonicalStore creates a new canonicalStore and loads all keys associated
 // with the keyPrefix in order to determine the ranges available in the database.
-func newCanonicalStore[T any](db ethdb.KeyValueStore, keyPrefix []byte,
+func newCanonicalStore[T any](db ethdb.Iteratee, keyPrefix []byte,
-	encode func(T) ([]byte, error), decode func([]byte) (T, error)) *canonicalStore[T] {
+	encode func(T) ([]byte, error), decode func([]byte) (T, error)) (*canonicalStore[T], error) {
 	cs := &canonicalStore[T]{
 		keyPrefix: keyPrefix,
 		encode:    encode,
@@ -61,31 +61,24 @@ func newCanonicalStore[T any](db ethdb.KeyValueStore, keyPrefix []byte,
 		if first {
 			cs.periods.Start = period
 		} else if cs.periods.End != period {
-			log.Warn("Gap in the canonical chain database")
+			return nil, fmt.Errorf("Gap in the canonical chain database between periods %d and %d", cs.periods.End, period-1)
-			break // continuity guaranteed
 		}
 		first = false
 		cs.periods.End = period + 1
 	}
 	iter.Release()
-	return cs
+	return cs, nil
 }
 
 // databaseKey returns the database key belonging to the given period.
 func (cs *canonicalStore[T]) databaseKey(period uint64) []byte {
-	var (
+	return binary.BigEndian.AppendUint64(append([]byte{}, cs.keyPrefix...), period)
-		kl  = len(cs.keyPrefix)
-		key = make([]byte, kl+8)
-	)
-	copy(key[:kl], cs.keyPrefix)
-	binary.BigEndian.PutUint64(key[kl:], period)
-	return key
 }
 
 // add adds the given item to the database. It also ensures that the range remains
 // continuous. Can be used either with a batch or database backend.
 func (cs *canonicalStore[T]) add(backend ethdb.KeyValueWriter, period uint64, value T) error {
-	if !cs.periods.CanExpand(period) {
+	if !cs.periods.canExpand(period) {
 		return fmt.Errorf("period expansion is not allowed, first: %d, next: %d, period: %d", cs.periods.Start, cs.periods.End, period)
 	}
 	enc, err := cs.encode(value)
@@ -96,15 +89,15 @@ func (cs *canonicalStore[T]) add(backend ethdb.KeyValueWriter, period uint64, va
 		return err
 	}
 	cs.cache.Add(period, value)
-	cs.periods.Expand(period)
+	cs.periods.expand(period)
 	return nil
 }
 
 // deleteFrom removes items starting from the given period.
-func (cs *canonicalStore[T]) deleteFrom(batch ethdb.Batch, fromPeriod uint64) (deleted Range) {
+func (cs *canonicalStore[T]) deleteFrom(db ethdb.KeyValueWriter, fromPeriod uint64) (deleted periodRange) {
-	keepRange, deleteRange := cs.periods.Split(fromPeriod)
+	keepRange, deleteRange := cs.periods.split(fromPeriod)
-	deleteRange.Each(func(period uint64) {
+	deleteRange.each(func(period uint64) {
-		batch.Delete(cs.databaseKey(period))
+		db.Delete(cs.databaseKey(period))
 		cs.cache.Remove(period)
 	})
 	cs.periods = keepRange
@@ -113,22 +106,24 @@ func (cs *canonicalStore[T]) deleteFrom(batch ethdb.Batch, fromPeriod uint64) (d
 
 // get returns the item at the given period or the null value of the given type
 // if no item is present.
-func (cs *canonicalStore[T]) get(backend ethdb.KeyValueReader, period uint64) (value T, ok bool) {
+func (cs *canonicalStore[T]) get(backend ethdb.KeyValueReader, period uint64) (T, bool) {
-	if !cs.periods.Contains(period) {
+	var null T
-		return
+	if !cs.periods.contains(period) {
+		return null, false
 	}
-	if value, ok = cs.cache.Get(period); ok {
+	if value, ok := cs.cache.Get(period); ok {
-		return
+		return value, true
 	}
-	if enc, err := backend.Get(cs.databaseKey(period)); err == nil {
+	enc, err := backend.Get(cs.databaseKey(period))
-		if v, err := cs.decode(enc); err == nil {
+	if err != nil {
-			value, ok = v, true
-			cs.cache.Add(period, value)
-		} else {
-			log.Error("Error decoding canonical store value", "error", err)
-		}
-	} else {
 		log.Error("Canonical store value not found", "period", period, "start", cs.periods.Start, "end", cs.periods.End)
+		return null, false
 	}
-	return
+	value, err := cs.decode(enc)
+	if err != nil {
+		log.Error("Error decoding canonical store value", "error", err)
+		return null, false
+	}
+	cs.cache.Add(period, value)
+	return value, true
 }

beacon/light/committee_chain.go

@@ -123,9 +123,6 @@ func newCommitteeChain(db ethdb.KeyValueStore, config *types.ChainConfig, signer
 		}
 	)
 	s := &CommitteeChain{
-		fixedCommitteeRoots: newCanonicalStore[common.Hash](db, rawdb.FixedCommitteeRootKey, fixedCommitteeRootEncoder, fixedCommitteeRootDecoder),
-		committees:          newCanonicalStore[*types.SerializedSyncCommittee](db, rawdb.SyncCommitteeKey, committeeEncoder, committeeDecoder),
-		updates:             newCanonicalStore[*types.LightClientUpdate](db, rawdb.BestUpdateKey, updateEncoder, updateDecoder),
 		committeeCache:  lru.NewCache[uint64, syncCommittee](10),
 		db:              db,
 		sigVerifier:     sigVerifier,
@@ -140,12 +137,22 @@ func newCommitteeChain(db ethdb.KeyValueStore, config *types.ChainConfig, signer
 		},
 	}
 
-	if !s.checkConstraints() {
+	var err1, err2, err3 error
+	if s.fixedCommitteeRoots, err1 = newCanonicalStore[common.Hash](db, rawdb.FixedCommitteeRootKey, fixedCommitteeRootEncoder, fixedCommitteeRootDecoder); err1 != nil {
+		log.Error("Error creating fixed committee root store", "error", err1)
+	}
+	if s.committees, err2 = newCanonicalStore[*types.SerializedSyncCommittee](db, rawdb.SyncCommitteeKey, committeeEncoder, committeeDecoder); err2 != nil {
+		log.Error("Error creating committee store", "error", err2)
+	}
+	if s.updates, err3 = newCanonicalStore[*types.LightClientUpdate](db, rawdb.BestUpdateKey, updateEncoder, updateDecoder); err3 != nil {
+		log.Error("Error creating update store", "error", err3)
+	}
+	if err1 != nil || err2 != nil || err3 != nil || !s.checkConstraints() {
 		log.Info("Resetting invalid committee chain")
 		s.Reset()
 	}
 	// roll back invalid updates (might be necessary if forks have been changed since last time)
-	for !s.updates.periods.IsEmpty() {
+	for !s.updates.periods.isEmpty() {
 		update, ok := s.updates.get(s.db, s.updates.periods.End-1)
 		if !ok {
 			log.Error("Sync committee update missing", "period", s.updates.periods.End-1)
@@ -161,7 +168,7 @@ func newCommitteeChain(db ethdb.KeyValueStore, config *types.ChainConfig, signer
 			log.Error("Error writing batch into chain database", "error", err)
 		}
 	}
-	if !s.committees.periods.IsEmpty() {
+	if !s.committees.periods.isEmpty() {
 		log.Trace("Sync committee chain loaded", "first period", s.committees.periods.Start, "last period", s.committees.periods.End-1)
 	}
 	return s
@@ -169,14 +176,14 @@ func newCommitteeChain(db ethdb.KeyValueStore, config *types.ChainConfig, signer
 
 // checkConstraints checks committee chain validity constraints
 func (s *CommitteeChain) checkConstraints() bool {
-	isNotInFixedCommitteeRootRange := func(r Range) bool {
+	isNotInFixedCommitteeRootRange := func(r periodRange) bool {
-		return s.fixedCommitteeRoots.periods.IsEmpty() ||
+		return s.fixedCommitteeRoots.periods.isEmpty() ||
 			r.Start < s.fixedCommitteeRoots.periods.Start ||
 			r.Start >= s.fixedCommitteeRoots.periods.End
 	}
 
 	valid := true
-	if !s.updates.periods.IsEmpty() {
+	if !s.updates.periods.isEmpty() {
 		if isNotInFixedCommitteeRootRange(s.updates.periods) {
 			log.Error("Start update is not in the fixed roots range")
 			valid = false
@@ -186,7 +193,7 @@ func (s *CommitteeChain) checkConstraints() bool {
 			valid = false
 		}
 	}
-	if !s.committees.periods.IsEmpty() {
+	if !s.committees.periods.isEmpty() {
 		if isNotInFixedCommitteeRootRange(s.committees.periods) {
 			log.Error("Start committee is not in the fixed roots range")
 			valid = false
@@ -254,7 +261,7 @@ func (s *CommitteeChain) addFixedCommitteeRoot(period uint64, root common.Hash)
 
 	batch := s.db.NewBatch()
 	oldRoot := s.getCommitteeRoot(period)
-	if !s.fixedCommitteeRoots.periods.CanExpand(period) {
+	if !s.fixedCommitteeRoots.periods.canExpand(period) {
 		// Note: the fixed committee root range should always be continuous and
 		// therefore the expected syncing method is to forward sync and optionally
 		// backward sync periods one by one, starting from a checkpoint. The only
@@ -301,7 +308,7 @@ func (s *CommitteeChain) deleteFixedCommitteeRootsFrom(period uint64) error {
 	}
 	batch := s.db.NewBatch()
 	s.fixedCommitteeRoots.deleteFrom(batch, period)
-	if s.updates.periods.IsEmpty() || period <= s.updates.periods.Start {
+	if s.updates.periods.isEmpty() || period <= s.updates.periods.Start {
 		// Note: the first period of the update chain should always be fixed so if
 		// the fixed root at the first update is removed then the entire update chain
 		// and the proven committees have to be removed. Earlier committees in the
@@ -336,7 +343,7 @@ func (s *CommitteeChain) deleteCommitteesFrom(batch ethdb.Batch, period uint64)
 
 // addCommittee adds a committee at the given period if possible.
 func (s *CommitteeChain) addCommittee(period uint64, committee *types.SerializedSyncCommittee) error {
-	if !s.committees.periods.CanExpand(period) {
+	if !s.committees.periods.canExpand(period) {
 		return ErrInvalidPeriod
 	}
 	root := s.getCommitteeRoot(period)
@@ -346,7 +353,7 @@ func (s *CommitteeChain) addCommittee(period uint64, committee *types.Serialized
 	if root != committee.Root() {
 		return ErrWrongCommitteeRoot
 	}
-	if !s.committees.periods.Contains(period) {
+	if !s.committees.periods.contains(period) {
 		if err := s.committees.add(s.db, period, committee); err != nil {
 			return err
 		}
@@ -361,7 +368,7 @@ func (s *CommitteeChain) InsertUpdate(update *types.LightClientUpdate, nextCommi
 	defer s.chainmu.Unlock()
 
 	period := update.AttestedHeader.Header.SyncPeriod()
-	if !s.updates.periods.CanExpand(period) || !s.committees.periods.Contains(period) {
+	if !s.updates.periods.canExpand(period) || !s.committees.periods.contains(period) {
 		return ErrInvalidPeriod
 	}
 	if s.minimumUpdateScore.BetterThan(update.Score()) {
@@ -376,7 +383,7 @@ func (s *CommitteeChain) InsertUpdate(update *types.LightClientUpdate, nextCommi
 		}
 		return nil
 	}
-	if s.fixedCommitteeRoots.periods.Contains(period+1) && reorg {
+	if s.fixedCommitteeRoots.periods.contains(period+1) && reorg {
 		return ErrCannotReorg
 	}
 	if ok, err := s.verifyUpdate(update); err != nil {
@@ -384,7 +391,7 @@ func (s *CommitteeChain) InsertUpdate(update *types.LightClientUpdate, nextCommi
 	} else if !ok {
 		return ErrInvalidUpdate
 	}
-	addCommittee := !s.committees.periods.Contains(period+1) || reorg
+	addCommittee := !s.committees.periods.contains(period+1) || reorg
 	if addCommittee {
 		if nextCommittee == nil {
 			return ErrNeedCommittee
@@ -422,10 +429,10 @@ func (s *CommitteeChain) NextSyncPeriod() (uint64, bool) {
 	s.chainmu.RLock()
 	defer s.chainmu.RUnlock()
 
-	if s.committees.periods.IsEmpty() {
+	if s.committees.periods.isEmpty() {
 		return 0, false
 	}
-	if !s.updates.periods.IsEmpty() {
+	if !s.updates.periods.isEmpty() {
 		return s.updates.periods.End, true
 	}
 	return s.committees.periods.End - 1, true

beacon/light/range.go

@@ -16,32 +16,32 @@
 
 package light
 
-// Range represents a (possibly zero-length) range of integers (sync periods).
+// periodRange represents a (possibly zero-length) range of integers (sync periods).
-type Range struct {
+type periodRange struct {
 	Start, End uint64
 }
 
-// IsEmpty returns true if the length of the range is zero.
+// isEmpty returns true if the length of the range is zero.
-func (a Range) IsEmpty() bool {
+func (a periodRange) isEmpty() bool {
 	return a.End == a.Start
 }
 
-// Contains returns true if the range includes the given period.
+// contains returns true if the range includes the given period.
-func (a Range) Contains(period uint64) bool {
+func (a periodRange) contains(period uint64) bool {
 	return period >= a.Start && period < a.End
 }
 
-// CanExpand returns true if the range includes or can be expanded with the given
+// canExpand returns true if the range includes or can be expanded with the given
 // period (either the range is empty or the given period is inside, right before or
 // right after the range).
-func (a Range) CanExpand(period uint64) bool {
+func (a periodRange) canExpand(period uint64) bool {
-	return a.IsEmpty() || (period+1 >= a.Start && period <= a.End)
+	return a.isEmpty() || (period+1 >= a.Start && period <= a.End)
 }
 
-// Expand expands the range with the given period.
+// expand expands the range with the given period.
-// This method assumes that CanExpand returned true: otherwise this is a no-op.
+// This method assumes that canExpand returned true: otherwise this is a no-op.
-func (a *Range) Expand(period uint64) {
+func (a *periodRange) expand(period uint64) {
-	if a.IsEmpty() {
+	if a.isEmpty() {
 		a.Start, a.End = period, period+1
 		return
 	}
@@ -53,25 +53,25 @@ func (a *Range) Expand(period uint64) {
 	}
 }
 
-// Split splits the range into two ranges. The 'fromPeriod' will be the first
+// split splits the range into two ranges. The 'fromPeriod' will be the first
 // element in the second range (if present).
 // The original range is unchanged by this operation
-func (a *Range) Split(fromPeriod uint64) (Range, Range) {
+func (a *periodRange) split(fromPeriod uint64) (periodRange, periodRange) {
 	if fromPeriod <= a.Start {
 		// First range empty, everything in second range,
-		return Range{}, *a
+		return periodRange{}, *a
 	}
 	if fromPeriod >= a.End {
 		// Second range empty, everything in first range,
-		return *a, Range{}
+		return *a, periodRange{}
 	}
-	x := Range{a.Start, fromPeriod}
+	x := periodRange{a.Start, fromPeriod}
-	y := Range{fromPeriod, a.End}
+	y := periodRange{fromPeriod, a.End}
 	return x, y
 }
 
-// Each invokes the supplied function fn once per period in range
+// each invokes the supplied function fn once per period in range
-func (a *Range) Each(fn func(uint64)) {
+func (a *periodRange) each(fn func(uint64)) {
 	for p := a.Start; p < a.End; p++ {
 		fn(p)
 	}