suggestions for PR #4834

model/flow/mapfunc/identity.go

@@ -15,8 +15,8 @@ func WithInitialWeight(weight uint64) flow.IdentityMapFunc[flow.Identity] {
 }
 
 // WithWeight returns an anonymous function that assigns the given weight value
-// to `Identity.Weight`. This function is primarily intended for testing, as
-// Identity structs should be immutable by convention.
+// to `Identity.Weight`. We pass the input identity by value, i.e. copy on write,
+// to avoid modifying the original input.
 func WithWeight(weight uint64) flow.IdentityMapFunc[flow.Identity] {
 	return func(identity flow.Identity) flow.Identity {
 		identity.Weight = weight

model/flow/protocol_state.go

@@ -24,13 +24,10 @@ type DynamicIdentityEntryList []*DynamicIdentityEntry
 // plus some modifiers. We intend to restructure this code soon.
 // TODO: https://github.com/onflow/flow-go/issues/4649
 type ProtocolStateEntry struct {
-	// Setup and commit event IDs for previous epoch.
-	PreviousEpoch EpochStateContainer
-	// Setup and commit event IDs for previous epoch. These EventIDs are ZeroID if
-	// and only if the current Epoch is the first epoch after a spork or genesis.
-	CurrentEpoch EpochStateContainer
-	// Protocol state for next epoch. Could be nil if next epoch is not yet set up.
-	NextEpoch *EpochStateContainer
+	PreviousEpoch *EpochStateContainer // minimal dynamic properties for previous epoch [optional, nil for first epoch after spork, genesis]
+	CurrentEpoch  EpochStateContainer  // minimal dynamic properties for current epoch
+	NextEpoch     *EpochStateContainer // minimal dynamic properties for next epoch [optional, nil iff we are in staking phase]
+
 	// InvalidStateTransitionAttempted encodes whether an invalid state transition
 	// has been detected in this fork. Under normal operations, this value is false.
 	// The only possible state transition is false → true. When this happens,
@@ -147,49 +144,61 @@ func NewRichProtocolStateEntry(
 		NextEpochIdentityTable:    IdentityList{},
 	}
 
-	// ensure data is consistent
-	if protocolState.PreviousEpoch.SetupID != ZeroID {
-		if protocolState.PreviousEpoch.SetupID != previousEpochSetup.ID() {
-			return nil, fmt.Errorf("supplied previous epoch setup (%x) does not match protocol state (%x)",
-				previousEpochSetup.ID(),
-				protocolState.PreviousEpoch.SetupID)
+	// If previous epoch is specified: ensure respective epoch service events are not nil and consistent with commitments in `ProtocolStateEntry.PreviousEpoch`
+	if protocolState.PreviousEpoch != nil {
+		if protocolState.PreviousEpoch.SetupID != previousEpochSetup.ID() { // calling ID() will panic is EpochSetup event is nil
+			return nil, fmt.Errorf("supplied previous epoch's setup event (%x) does not match commitment (%x) in ProtocolStateEntry", previousEpochSetup.ID(), protocolState.PreviousEpoch.SetupID)
 		}
-		if protocolState.PreviousEpoch.CommitID != previousEpochCommit.ID() {
-			return nil, fmt.Errorf("supplied previous epoch commit (%x) does not match protocol state (%x)",
-				previousEpochCommit.ID(),
-				protocolState.PreviousEpoch.CommitID)
+		if protocolState.PreviousEpoch.CommitID != previousEpochCommit.ID() { // calling ID() will panic is EpochCommit event is nil
+			return nil, fmt.Errorf("supplied previous epoch's commit event (%x) does not match commitment (%x) in ProtocolStateEntry", previousEpochCommit.ID(), protocolState.PreviousEpoch.CommitID)
 		}
 	}
-	if protocolState.CurrentEpoch.SetupID != currentEpochSetup.ID() {
-		return nil, fmt.Errorf("supplied current epoch setup (%x) does not match protocol state (%x)",
-			currentEpochSetup.ID(),
-			protocolState.CurrentEpoch.SetupID)
+
+	// For current epoch: ensure respective epoch service events are not nil and consistent with commitments in `ProtocolStateEntry.CurrentEpoch`
+	if protocolState.CurrentEpoch.SetupID != currentEpochSetup.ID() { // calling ID() will panic is EpochSetup event is nil
+		return nil, fmt.Errorf("supplied current epoch's setup event (%x) does not match commitment (%x) in ProtocolStateEntry", currentEpochSetup.ID(), protocolState.CurrentEpoch.SetupID)
 	}
-	if protocolState.CurrentEpoch.CommitID != currentEpochCommit.ID() {
-		return nil, fmt.Errorf("supplied current epoch commit (%x) does not match protocol state (%x)",
-			currentEpochCommit.ID(),
-			protocolState.CurrentEpoch.CommitID)
+	if protocolState.CurrentEpoch.CommitID != currentEpochCommit.ID() { // calling ID() will panic is EpochCommit event is nil
+		return nil, fmt.Errorf("supplied current epoch's commit event (%x) does not match commitment (%x) in ProtocolStateEntry", currentEpochCommit.ID(), protocolState.CurrentEpoch.CommitID)
 	}
 
+	// if we are in staking phase (i.e. protocolState.NextEpoch == nil):
+	//  (1) Full identity table for current epoch contains active identities from current epoch.
+	//      If previous epoch exists, we add nodes from previous epoch that are leaving in the current epoch with 0 weight.
+	// otherwise, we are in epoch setup or epoch commit phase (i.e. protocolState.NextEpoch ≠ nil):
+	//  (2a) full identity table for current is active identities from current epoch + nodes joining in next epoch with 0 weight
+	//  (2b) furthermore, we also build the full identity table for the next epoch's staking phase:
+	//    active identities from next epoch + nodes from current epoch that are leaving at the end of the current epoch with 0 weight
 	var err error
 	nextEpoch := protocolState.NextEpoch
-	// if next epoch has been already committed, fill in data for it as well.
-	if nextEpoch != nil {
-		// sanity check consistency of input data
+	if nextEpoch == nil { // in staking phase: build full identity table for current epoch according to (1)
+		var previousEpochIdentitySkeletons IdentitySkeletonList
+		var previousEpochDynamicIdentities DynamicIdentityEntryList
+		if previousEpochSetup != nil {
+			previousEpochIdentitySkeletons = previousEpochSetup.Participants
+			previousEpochDynamicIdentities = protocolState.PreviousEpoch.ActiveIdentities
+		}
+		result.CurrentEpochIdentityTable, err = BuildIdentityTable(
+			protocolState.CurrentEpoch.ActiveIdentities,
+			currentEpochSetup.Participants,
+			previousEpochIdentitySkeletons,
+			previousEpochDynamicIdentities,
+		)
+		if err != nil {
+			return nil, fmt.Errorf("could not build identity table for staking phase: %w", err)
+		}
+	} else { // protocolState.NextEpoch ≠ nil, i.e. we are in epoch setup or epoch commit phase
+		// ensure respective epoch service events are not nil and consistent with commitments in `ProtocolStateEntry.NextEpoch`
 		if nextEpoch.SetupID != nextEpochSetup.ID() {
-			return nil, fmt.Errorf("inconsistent EpochSetup for constucting RichProtocolStateEntry, next protocol state states ID %v while input event has ID %v",
-				nextEpoch.SetupID, nextEpochSetup.ID())
+			return nil, fmt.Errorf("supplied next epoch's setup event (%x) does not match commitment (%x) in ProtocolStateEntry", nextEpoch.SetupID, nextEpochSetup.ID())
 		}
 		if nextEpoch.CommitID != ZeroID {
 			if nextEpoch.CommitID != nextEpochCommit.ID() {
-				return nil, fmt.Errorf("inconsistent EpochCommit for constucting RichProtocolStateEntry, next protocol state states ID %v while input event has ID %v",
-					nextEpoch.CommitID, nextEpochCommit.ID())
+				return nil, fmt.Errorf("supplied next epoch's commit event (%x) does not match commitment (%x) in ProtocolStateEntry", nextEpoch.CommitID, nextEpochCommit.ID())
 			}
 		}
 
-		// if next epoch is available, it means that we have observed epoch setup event and we are not anymore in staking phase,
-		// so we need to build the identity table using current and next epoch setup events.
-		result.CurrentEpochIdentityTable, err = BuildIdentityTable(
+		result.CurrentEpochIdentityTable, err = BuildIdentityTable( // build full identity table for current epoch according to (2a)
 			protocolState.CurrentEpoch.ActiveIdentities,
 			currentEpochSetup.Participants,
 			nextEpochSetup.Participants,
@@ -199,7 +208,7 @@ func NewRichProtocolStateEntry(
 			return nil, fmt.Errorf("could not build identity table for setup/commit phase: %w", err)
 		}
 
-		result.NextEpochIdentityTable, err = BuildIdentityTable(
+		result.NextEpochIdentityTable, err = BuildIdentityTable( // build full identity table for next epoch according to (2b)
 			nextEpoch.ActiveIdentities,
 			nextEpochSetup.Participants,
 			currentEpochSetup.Participants,
@@ -208,24 +217,7 @@ func NewRichProtocolStateEntry(
 		if err != nil {
 			return nil, fmt.Errorf("could not build next epoch identity table: %w", err)
 		}
-	} else {
-		// if next epoch is not yet created, it means that we are in staking phase,
-		// so we need to build the identity table using previous and current epoch setup events.
-		var previousEpochIdentities IdentitySkeletonList
-		if previousEpochSetup != nil {
-			previousEpochIdentities = previousEpochSetup.Participants
-		}
-		result.CurrentEpochIdentityTable, err = BuildIdentityTable(
-			protocolState.CurrentEpoch.ActiveIdentities,
-			currentEpochSetup.Participants,
-			previousEpochIdentities,
-			protocolState.PreviousEpoch.ActiveIdentities,
-		)
-		if err != nil {
-			return nil, fmt.Errorf("could not build identity table for staking phase: %w", err)
-		}
 	}
-
 	return result, nil
 }
 
@@ -255,7 +247,7 @@ func (e *ProtocolStateEntry) Copy() *ProtocolStateEntry {
 		return nil
 	}
 	return &ProtocolStateEntry{
-		PreviousEpoch:                   e.PreviousEpoch,
+		PreviousEpoch:                   e.PreviousEpoch.Copy(),
 		CurrentEpoch:                    *e.CurrentEpoch.Copy(),
 		NextEpoch:                       e.NextEpoch.Copy(),
 		InvalidStateTransitionAttempted: e.InvalidStateTransitionAttempted,
@@ -334,9 +326,8 @@ func (ll DynamicIdentityEntryList) ByNodeID(nodeID Identifier) (*DynamicIdentity
 // All Identity fields are deep-copied, _except_ for their keys, which
 // are copied by reference.
 func (ll DynamicIdentityEntryList) Copy() DynamicIdentityEntryList {
-	dup := make(DynamicIdentityEntryList, 0, len(ll))
-
 	lenList := len(ll)
+	dup := make(DynamicIdentityEntryList, 0, lenList)
 	for i := 0; i < lenList; i++ {
 		// copy the object
 		next := *(ll[i])
@@ -374,12 +365,11 @@ func BuildIdentityTable(
 	adjacentEpochIdentitySkeletons IdentitySkeletonList,
 	adjacentEpochDynamicIdentities DynamicIdentityEntryList,
 ) (IdentityList, error) {
-
-	targetEpochParticipants, err := ReconstructIdentities(targetEpochIdentitySkeletons, targetEpochDynamicIdentities)
+	targetEpochParticipants, err := ComposeFullIdentities(targetEpochIdentitySkeletons, targetEpochDynamicIdentities)
 	if err != nil {
 		return nil, fmt.Errorf("could not reconstruct participants for target epoch: %w", err)
 	}
-	adjacentEpochParticipants, err := ReconstructIdentities(adjacentEpochIdentitySkeletons, adjacentEpochDynamicIdentities)
+	adjacentEpochParticipants, err := ComposeFullIdentities(adjacentEpochIdentitySkeletons, adjacentEpochDynamicIdentities)
 	if err != nil {
 		return nil, fmt.Errorf("could not reconstruct participants for adjacent epoch: %w", err)
 	}
@@ -395,6 +385,7 @@ func BuildIdentityTable(
 		identity.Weight = 0
 		return identity
 	}))
+
 	return allEpochParticipants, nil
 }
 
@@ -410,9 +401,11 @@ func DynamicIdentityEntryListFromIdentities(identities IdentityList) DynamicIden
 	return dynamicIdentities
 }
 
-// ReconstructIdentities combines identity skeletons and dynamic identities to produce a flow.IdentityList.
+// ComposeFullIdentities combines identity skeletons and dynamic identities to produce a flow.IdentityList.
+// It enforces that the input slices `skeletons` and `dynamics` list the same identities (compared by nodeID)
+// in the same order. Otherwise, an exception if returned.
 // No errors are expected during normal operations.
-func ReconstructIdentities(skeletons IdentitySkeletonList, dynamics DynamicIdentityEntryList) (IdentityList, error) {
+func ComposeFullIdentities(skeletons IdentitySkeletonList, dynamics DynamicIdentityEntryList) (IdentityList, error) {
 	// sanity check: list of skeletons and dynamic should be the same
 	if len(skeletons) != len(dynamics) {
 		return nil, fmt.Errorf("invalid number of identities to reconstruct: expected %d, got %d", len(skeletons), len(dynamics))
@@ -423,7 +416,7 @@ func ReconstructIdentities(skeletons IdentitySkeletonList, dynamics DynamicIdent
 	for i := range dynamics {
 		// sanity check: identities should be sorted in the same order
 		if dynamics[i].NodeID != skeletons[i].NodeID {
-			return nil, fmt.Errorf("identites in protocol state are not in canonical order: expected %s, got %s", skeletons[i].NodeID, dynamics[i].NodeID)
+			return nil, fmt.Errorf("identites in protocol state are not consistently ordered: expected %s, got %s", skeletons[i].NodeID, dynamics[i].NodeID)
 		}
 		result = append(result, &Identity{
 			IdentitySkeleton: *skeletons[i],

model/flow/protocol_state_test.go

@@ -29,16 +29,12 @@ func TestNewRichProtocolStateEntry(t *testing.T) {
 			})
 		}
 		stateEntry := &flow.ProtocolStateEntry{
+			PreviousEpoch: nil,
 			CurrentEpoch: flow.EpochStateContainer{
 				SetupID:          setup.ID(),
 				CommitID:         currentEpochCommit.ID(),
 				ActiveIdentities: identities,
 			},
-			PreviousEpoch: flow.EpochStateContainer{
-				SetupID:          flow.ZeroID,
-				CommitID:         flow.ZeroID,
-				ActiveIdentities: nil,
-			},
 			InvalidStateTransitionAttempted: false,
 		}
 		entry, err := flow.NewRichProtocolStateEntry(

state/protocol/inmem/convert.go

@@ -334,11 +334,7 @@ func SnapshotFromBootstrapStateWithParams(
 		})
 	}
 	protocolState := &flow.ProtocolStateEntry{
-		PreviousEpoch: flow.EpochStateContainer{
-			SetupID:          flow.ZeroID,
-			CommitID:         flow.ZeroID,
-			ActiveIdentities: nil,
-		},
+		PreviousEpoch: nil,
 		CurrentEpoch: flow.EpochStateContainer{
 			SetupID:          setup.ID(),
 			CommitID:         commit.ID(),