From ff90836e7031059efcb4d669fd9cf291812f6c31 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 18 Oct 2023 08:46:54 +0300 Subject: [PATCH 01/15] WIP. Updated prev epoch to contain active identities. Updated ActiveIdentities to contain only epoch related identities --- model/flow/protocol_state.go | 75 ++++++++++------ model/flow/protocol_state_test.go | 4 +- state/protocol/inmem/convert.go | 6 +- .../inmem/dynamic_protocol_state_test.go | 6 +- state/protocol/protocol_state/updater.go | 2 +- storage/badger/protocol_state.go | 6 +- storage/badger/protocol_state_test.go | 8 +- utils/unittest/fixtures.go | 88 +++++++------------ 8 files changed, 97 insertions(+), 98 deletions(-) diff --git a/model/flow/protocol_state.go b/model/flow/protocol_state.go index 443c285a279..98ae4176fdc 100644 --- a/model/flow/protocol_state.go +++ b/model/flow/protocol_state.go @@ -25,7 +25,7 @@ type DynamicIdentityEntryList []*DynamicIdentityEntry // TODO: https://github.com/onflow/flow-go/issues/4649 type ProtocolStateEntry struct { // Setup and commit event IDs for previous epoch. - PreviousEpochEventIDs EventIDs + 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 @@ -148,16 +148,16 @@ func NewRichProtocolStateEntry( } // ensure data is consistent - if protocolState.PreviousEpochEventIDs.SetupID != ZeroID { - if protocolState.PreviousEpochEventIDs.SetupID != previousEpochSetup.ID() { + 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.PreviousEpochEventIDs.SetupID) + protocolState.PreviousEpoch.SetupID) } - if protocolState.PreviousEpochEventIDs.CommitID != previousEpochCommit.ID() { + if protocolState.PreviousEpoch.CommitID != previousEpochCommit.ID() { return nil, fmt.Errorf("supplied previous epoch commit (%x) does not match protocol state (%x)", previousEpochCommit.ID(), - protocolState.PreviousEpochEventIDs.CommitID) + protocolState.PreviousEpoch.CommitID) } } if protocolState.CurrentEpoch.SetupID != currentEpochSetup.ID() { @@ -193,6 +193,7 @@ func NewRichProtocolStateEntry( protocolState.CurrentEpoch.ActiveIdentities, currentEpochSetup.Participants, nextEpochSetup.Participants, + nextEpoch.ActiveIdentities, ) if err != nil { return nil, fmt.Errorf("could not build identity table for setup/commit phase: %w", err) @@ -202,6 +203,7 @@ func NewRichProtocolStateEntry( nextEpoch.ActiveIdentities, nextEpochSetup.Participants, currentEpochSetup.Participants, + protocolState.CurrentEpoch.ActiveIdentities, ) if err != nil { return nil, fmt.Errorf("could not build next epoch identity table: %w", err) @@ -217,6 +219,7 @@ func NewRichProtocolStateEntry( protocolState.CurrentEpoch.ActiveIdentities, currentEpochSetup.Participants, otherIdentities, + protocolState.PreviousEpoch.ActiveIdentities, ) if err != nil { return nil, fmt.Errorf("could not build identity table for staking phase: %w", err) @@ -232,12 +235,12 @@ func (e *ProtocolStateEntry) ID() Identifier { return ZeroID } body := struct { - PreviousEpochEventIDs EventIDs + PreviousEpochID Identifier CurrentEpochID Identifier NextEpochID Identifier InvalidStateTransitionAttempted bool }{ - PreviousEpochEventIDs: e.PreviousEpochEventIDs, + PreviousEpochID: e.PreviousEpoch.ID(), CurrentEpochID: e.CurrentEpoch.ID(), NextEpochID: e.NextEpoch.ID(), InvalidStateTransitionAttempted: e.InvalidStateTransitionAttempted, @@ -252,7 +255,7 @@ func (e *ProtocolStateEntry) Copy() *ProtocolStateEntry { return nil } return &ProtocolStateEntry{ - PreviousEpochEventIDs: e.PreviousEpochEventIDs, + PreviousEpoch: e.PreviousEpoch, CurrentEpoch: *e.CurrentEpoch.Copy(), NextEpoch: e.NextEpoch.Copy(), InvalidStateTransitionAttempted: e.InvalidStateTransitionAttempted, @@ -282,7 +285,7 @@ func (e *RichProtocolStateEntry) Copy() *RichProtocolStateEntry { // EpochStatus returns epoch status for the current protocol state. func (e *ProtocolStateEntry) EpochStatus() *EpochStatus { return &EpochStatus{ - PreviousEpoch: e.PreviousEpochEventIDs, + PreviousEpoch: e.PreviousEpoch.EventIDs(), CurrentEpoch: e.CurrentEpoch.EventIDs(), NextEpoch: e.NextEpoch.EventIDs(), InvalidServiceEventIncorporated: e.InvalidStateTransitionAttempted, @@ -368,27 +371,45 @@ func BuildIdentityTable( targetEpochDynamicIdentities DynamicIdentityEntryList, targetEpochIdentitySkeletons IdentitySkeletonList, adjacentEpochIdentitySkeletons IdentitySkeletonList, + adjacentEpochDynamicIdentities DynamicIdentityEntryList, ) (IdentityList, error) { - // produce a unique set for current and previous epoch participants - allEpochParticipants := targetEpochIdentitySkeletons.Union(adjacentEpochIdentitySkeletons) - // sanity check: size of identities should be equal to previous and current epoch participants combined - if len(allEpochParticipants) != len(targetEpochDynamicIdentities) { - return nil, fmt.Errorf("invalid number of identities in protocol state: expected %d, got %d", len(allEpochParticipants), len(targetEpochDynamicIdentities)) - } - // build full identity table for current epoch - var result IdentityList - for i, identity := range targetEpochDynamicIdentities { - // sanity check: identities should be sorted in canonical order - if identity.NodeID != allEpochParticipants[i].NodeID { - return nil, fmt.Errorf("identites in protocol state are not in canonical order: expected %s, got %s", allEpochParticipants[i].NodeID, identity.NodeID) + reconstructIdentityTable := func(skeletons IdentitySkeletonList, dynamics DynamicIdentityEntryList) (IdentityList, error) { + // sanity check: size of identities should be equal to previous and current epoch participants combined + if len(skeletons) != len(dynamics) { + return nil, fmt.Errorf("invalid number of identities in protocol state: expected %d, got %d", len(skeletons), len(dynamics)) } - result = append(result, &Identity{ - IdentitySkeleton: *allEpochParticipants[i], - DynamicIdentity: identity.Dynamic, - }) + + // build full identity table for current epoch + var result IdentityList + for i := range dynamics { + // sanity check: identities should be sorted in canonical 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) + } + result = append(result, &Identity{ + IdentitySkeleton: *skeletons[i], + DynamicIdentity: dynamics[i].Dynamic, + }) + } + return result, nil } - return result, nil + + targetEpochParticipants, err := reconstructIdentityTable(targetEpochIdentitySkeletons, targetEpochDynamicIdentities) + if err != nil { + return nil, fmt.Errorf("could not reconstruct participants for target epoch: %w", err) + } + adjacentEpochParticipants, err := reconstructIdentityTable(adjacentEpochIdentitySkeletons, adjacentEpochDynamicIdentities) + if err != nil { + return nil, fmt.Errorf("could not reconstruct participants for adjacent epoch: %w", err) + } + + // produce a unique set for current and previous epoch participants + allEpochParticipants := targetEpochParticipants.Union(adjacentEpochParticipants.Map(func(identity Identity) Identity { + identity.Weight = 0 + return identity + })) + return allEpochParticipants, nil } // DynamicIdentityEntryListFromIdentities converts IdentityList to DynamicIdentityEntryList. diff --git a/model/flow/protocol_state_test.go b/model/flow/protocol_state_test.go index 5148c68f0ca..b150f576306 100644 --- a/model/flow/protocol_state_test.go +++ b/model/flow/protocol_state_test.go @@ -34,7 +34,7 @@ func TestNewRichProtocolStateEntry(t *testing.T) { CommitID: currentEpochCommit.ID(), ActiveIdentities: identities, }, - PreviousEpochEventIDs: flow.EventIDs{}, + PreviousEpoch: flow.EventIDs{}, InvalidStateTransitionAttempted: false, } entry, err := flow.NewRichProtocolStateEntry( @@ -161,7 +161,7 @@ func TestProtocolStateEntry_Copy(t *testing.T) { cpy := entry.Copy() assert.Equal(t, entry, cpy) assert.NotSame(t, entry.NextEpoch, cpy.NextEpoch) - assert.NotSame(t, entry.PreviousEpochEventIDs, cpy.PreviousEpochEventIDs) + assert.NotSame(t, entry.PreviousEpoch, cpy.PreviousEpoch) assert.NotSame(t, entry.CurrentEpoch, cpy.CurrentEpoch) cpy.InvalidStateTransitionAttempted = !entry.InvalidStateTransitionAttempted diff --git a/state/protocol/inmem/convert.go b/state/protocol/inmem/convert.go index 496f50cd423..83214eafd12 100644 --- a/state/protocol/inmem/convert.go +++ b/state/protocol/inmem/convert.go @@ -334,7 +334,11 @@ func SnapshotFromBootstrapStateWithParams( }) } protocolState := &flow.ProtocolStateEntry{ - PreviousEpochEventIDs: flow.EventIDs{}, + PreviousEpoch: flow.EpochStateContainer{ + SetupID: flow.ZeroID, + CommitID: flow.ZeroID, + ActiveIdentities: nil, + }, CurrentEpoch: flow.EpochStateContainer{ SetupID: setup.ID(), CommitID: commit.ID(), diff --git a/state/protocol/inmem/dynamic_protocol_state_test.go b/state/protocol/inmem/dynamic_protocol_state_test.go index 5885feb73fe..229c9734dae 100644 --- a/state/protocol/inmem/dynamic_protocol_state_test.go +++ b/state/protocol/inmem/dynamic_protocol_state_test.go @@ -33,7 +33,7 @@ func TestDynamicProtocolStateAdapter(t *testing.T) { entry := unittest.ProtocolStateFixture() adapter := inmem.NewDynamicProtocolStateAdapter(entry, globalParams) status := adapter.EpochStatus() - assert.Equal(t, entry.PreviousEpochEventIDs, status.PreviousEpoch) + assert.Equal(t, entry.PreviousEpoch, status.PreviousEpoch) assert.Equal(t, flow.EventIDs{ SetupID: entry.CurrentEpoch.SetupID, CommitID: entry.CurrentEpoch.CommitID, @@ -48,7 +48,7 @@ func TestDynamicProtocolStateAdapter(t *testing.T) { adapter := inmem.NewDynamicProtocolStateAdapter(entry, globalParams) status := adapter.EpochStatus() - assert.Equal(t, entry.PreviousEpochEventIDs, status.PreviousEpoch) + assert.Equal(t, entry.PreviousEpoch, status.PreviousEpoch) assert.Equal(t, flow.EventIDs{ SetupID: entry.CurrentEpoch.SetupID, CommitID: entry.CurrentEpoch.CommitID, @@ -63,7 +63,7 @@ func TestDynamicProtocolStateAdapter(t *testing.T) { entry := unittest.ProtocolStateFixture(unittest.WithNextEpochProtocolState()) adapter := inmem.NewDynamicProtocolStateAdapter(entry, globalParams) status := adapter.EpochStatus() - assert.Equal(t, entry.PreviousEpochEventIDs, status.PreviousEpoch) + assert.Equal(t, entry.PreviousEpoch, status.PreviousEpoch) assert.Equal(t, flow.EventIDs{ SetupID: entry.CurrentEpoch.SetupID, CommitID: entry.CurrentEpoch.CommitID, diff --git a/state/protocol/protocol_state/updater.go b/state/protocol/protocol_state/updater.go index f846d564063..cc419a7f98f 100644 --- a/state/protocol/protocol_state/updater.go +++ b/state/protocol/protocol_state/updater.go @@ -233,7 +233,7 @@ func (u *Updater) TransitionToNextEpoch() error { return fmt.Errorf("protocol state transition is only allowed when enterring next epoch") } u.state = &flow.ProtocolStateEntry{ - PreviousEpochEventIDs: u.state.CurrentEpoch.EventIDs(), + PreviousEpoch: u.state.CurrentEpoch, CurrentEpoch: *u.state.NextEpoch, InvalidStateTransitionAttempted: false, } diff --git a/storage/badger/protocol_state.go b/storage/badger/protocol_state.go index 5fa5c90e3a9..21da93a5d52 100644 --- a/storage/badger/protocol_state.go +++ b/storage/badger/protocol_state.go @@ -144,12 +144,12 @@ func newRichProtocolStateEntry( err error ) // query and fill in epoch setups and commits for previous and current epochs - if protocolState.PreviousEpochEventIDs.SetupID != flow.ZeroID { - previousEpochSetup, err = setups.ByID(protocolState.PreviousEpochEventIDs.SetupID) + if protocolState.PreviousEpoch.SetupID != flow.ZeroID { + previousEpochSetup, err = setups.ByID(protocolState.PreviousEpoch.SetupID) if err != nil { return nil, fmt.Errorf("could not retrieve previous epoch setup: %w", err) } - previousEpochCommit, err = commits.ByID(protocolState.PreviousEpochEventIDs.CommitID) + previousEpochCommit, err = commits.ByID(protocolState.PreviousEpoch.CommitID) if err != nil { return nil, fmt.Errorf("could not retrieve previous epoch commit: %w", err) } diff --git a/storage/badger/protocol_state_test.go b/storage/badger/protocol_state_test.go index 62388c5b9a8..0f3b007292b 100644 --- a/storage/badger/protocol_state_test.go +++ b/storage/badger/protocol_state_test.go @@ -195,14 +195,14 @@ func assertRichProtocolStateValidity(t *testing.T, state *flow.RichProtocolState var previousEpochParticipants flow.IdentityList // invariant: PreviousEpochSetup and PreviousEpochCommit should be present if respective ID is not zero. - if state.PreviousEpochEventIDs.SetupID != flow.ZeroID { + if state.PreviousEpoch.SetupID != flow.ZeroID { // invariant: PreviousEpochSetup and PreviousEpochCommit are for the same epoch. Never nil. assert.Equal(t, state.CurrentEpochSetup.Counter, state.PreviousEpochSetup.Counter+1, "current epoch setup should be next after previous epoch") assert.Equal(t, state.PreviousEpochSetup.Counter, state.PreviousEpochCommit.Counter, "previous epoch setup and commit should be for the same epoch") // invariant: PreviousEpochSetup and PreviousEpochCommit IDs are the equal to the ID of the protocol state entry. Never nil. - assert.Equal(t, state.PreviousEpochSetup.ID(), state.ProtocolStateEntry.PreviousEpochEventIDs.SetupID, "epoch setup should be for correct event ID") - assert.Equal(t, state.PreviousEpochCommit.ID(), state.ProtocolStateEntry.PreviousEpochEventIDs.CommitID, "epoch commit should be for correct event ID") + assert.Equal(t, state.PreviousEpochSetup.ID(), state.ProtocolStateEntry.PreviousEpoch.SetupID, "epoch setup should be for correct event ID") + assert.Equal(t, state.PreviousEpochCommit.ID(), state.ProtocolStateEntry.PreviousEpoch.CommitID, "epoch commit should be for correct event ID") for _, participant := range state.PreviousEpochSetup.Participants { if identity, found := state.CurrentEpochIdentityTable.ByNodeID(participant.NodeID); found { @@ -231,7 +231,7 @@ func assertRichProtocolStateValidity(t *testing.T, state *flow.RichProtocolState assert.Equal(t, allIdentities, state.CurrentEpochIdentityTable, "identities should be a full identity table for the current epoch, without duplicates") for i, identity := range state.CurrentEpoch.ActiveIdentities { - assert.Equal(t, identity.NodeID, allIdentities[i].NodeID, "identity node ID should match") + assert.Equal(t, identity.NodeID, participantsFromCurrentEpochSetup[i].NodeID, "active identities should hold value from current epoch") } nextEpoch := state.NextEpoch diff --git a/utils/unittest/fixtures.go b/utils/unittest/fixtures.go index 2ad4a4609dc..43a5f3c1e8c 100644 --- a/utils/unittest/fixtures.go +++ b/utils/unittest/fixtures.go @@ -4,6 +4,7 @@ import ( "bytes" crand "crypto/rand" "fmt" + "github.com/onflow/flow-go/model/flow/mapfunc" "math/rand" "net" "testing" @@ -2594,9 +2595,10 @@ func RootProtocolStateFixture() *flow.RichProtocolStateEntry { CommitID: currentEpochCommit.ID(), ActiveIdentities: flow.DynamicIdentityEntryListFromIdentities(allIdentities), }, - PreviousEpochEventIDs: flow.EventIDs{ - SetupID: flow.ZeroID, - CommitID: flow.ZeroID, + PreviousEpoch: flow.EpochStateContainer{ + SetupID: flow.ZeroID, + CommitID: flow.ZeroID, + ActiveIdentities: nil, }, InvalidStateTransitionAttempted: false, NextEpoch: nil, @@ -2633,45 +2635,41 @@ func ProtocolStateFixture(options ...func(*flow.RichProtocolStateEntry)) *flow.R // reuse same participant for current epoch sameParticipant := *prevEpochSetup.Participants[1] setup.Participants[1] = &sameParticipant + setup.Participants = setup.Participants.Sort(order.Canonical[flow.IdentitySkeleton]) }) currentEpochCommit := EpochCommitFixture(func(commit *flow.EpochCommit) { commit.Counter = currentEpochSetup.Counter }) - allIdentities := make(flow.IdentityList, 0, len(currentEpochSetup.Participants)) - for _, identity := range currentEpochSetup.Participants { - allIdentities = append(allIdentities, &flow.Identity{ - IdentitySkeleton: *identity, - DynamicIdentity: flow.DynamicIdentity{ - Weight: identity.InitialWeight, - Ejected: false, - }, - }) - } - for _, identity := range prevEpochSetup.Participants { - if _, found := allIdentities.ByNodeID(identity.NodeID); !found { - allIdentities = append(allIdentities, &flow.Identity{ + buildDefaultIdentities := func(setup *flow.EpochSetup) flow.IdentityList { + epochIdentities := make(flow.IdentityList, 0, len(setup.Participants)) + for _, identity := range setup.Participants { + epochIdentities = append(epochIdentities, &flow.Identity{ IdentitySkeleton: *identity, DynamicIdentity: flow.DynamicIdentity{ - Weight: 0, + Weight: identity.InitialWeight, Ejected: false, }, }) } + return epochIdentities.Sort(order.Canonical[flow.Identity]) } - allIdentities = allIdentities.Sort(order.Canonical[flow.Identity]) + prevEpochIdentities := buildDefaultIdentities(prevEpochSetup) + currentEpochIdentities := buildDefaultIdentities(currentEpochSetup) + allIdentities := currentEpochIdentities.Union(prevEpochIdentities.Map(mapfunc.WithWeight(0))) entry := &flow.RichProtocolStateEntry{ ProtocolStateEntry: &flow.ProtocolStateEntry{ CurrentEpoch: flow.EpochStateContainer{ SetupID: currentEpochSetup.ID(), CommitID: currentEpochCommit.ID(), - ActiveIdentities: flow.DynamicIdentityEntryListFromIdentities(allIdentities), + ActiveIdentities: flow.DynamicIdentityEntryListFromIdentities(currentEpochIdentities), }, - PreviousEpochEventIDs: flow.EventIDs{ - SetupID: prevEpochSetup.ID(), - CommitID: prevEpochCommit.ID(), + PreviousEpoch: flow.EpochStateContainer{ + SetupID: prevEpochSetup.ID(), + CommitID: prevEpochCommit.ID(), + ActiveIdentities: flow.DynamicIdentityEntryListFromIdentities(prevEpochIdentities), }, InvalidStateTransitionAttempted: false, NextEpoch: nil, @@ -2707,32 +2705,15 @@ func WithNextEpochProtocolState() func(entry *flow.RichProtocolStateEntry) { // reuse same participant for current epoch sameParticipant := *entry.CurrentEpochSetup.Participants[1] setup.Participants[1] = &sameParticipant + setup.Participants = setup.Participants.Sort(order.Canonical[flow.IdentitySkeleton]) }) nextEpochCommit := EpochCommitFixture(func(commit *flow.EpochCommit) { commit.Counter = nextEpochSetup.Counter }) - entry.CurrentEpochIdentityTable = nil - identitiesStateLookup := entry.CurrentEpoch.ActiveIdentities.Lookup() - for _, identity := range entry.CurrentEpochSetup.Participants { - entry.CurrentEpochIdentityTable = append(entry.CurrentEpochIdentityTable, &flow.Identity{ - IdentitySkeleton: *identity, - DynamicIdentity: identitiesStateLookup[identity.NodeID].Dynamic, - }) - } - + nextEpochParticipants := make(flow.IdentityList, 0, len(nextEpochSetup.Participants)) for _, identity := range nextEpochSetup.Participants { - if _, found := entry.CurrentEpochIdentityTable.ByNodeID(identity.NodeID); !found { - entry.CurrentEpochIdentityTable = append(entry.CurrentEpochIdentityTable, &flow.Identity{ - IdentitySkeleton: *identity, - DynamicIdentity: flow.DynamicIdentity{ - Weight: 0, - Ejected: false, - }, - }) - } - - entry.NextEpochIdentityTable = append(entry.NextEpochIdentityTable, &flow.Identity{ + nextEpochParticipants = append(nextEpochParticipants, &flow.Identity{ IdentitySkeleton: *identity, DynamicIdentity: flow.DynamicIdentity{ Weight: identity.InitialWeight, @@ -2740,27 +2721,20 @@ func WithNextEpochProtocolState() func(entry *flow.RichProtocolStateEntry) { }, }) } + nextEpochParticipants = nextEpochParticipants.Sort(order.Canonical[flow.Identity]) - for _, identity := range entry.CurrentEpochSetup.Participants { - if _, found := entry.NextEpochIdentityTable.ByNodeID(identity.NodeID); !found { - entry.NextEpochIdentityTable = append(entry.NextEpochIdentityTable, &flow.Identity{ - IdentitySkeleton: *identity, - DynamicIdentity: flow.DynamicIdentity{ - Weight: 0, - Ejected: false, - }, - }) - } - } + currentEpochParticipants := entry.CurrentEpochIdentityTable.Filter(func(identity *flow.Identity) bool { + _, found := entry.CurrentEpochSetup.Participants.ByNodeID(identity.NodeID) + return found + }).Sort(order.Canonical[flow.Identity]) - entry.CurrentEpochIdentityTable = entry.CurrentEpochIdentityTable.Sort(order.Canonical[flow.Identity]) - entry.NextEpochIdentityTable = entry.NextEpochIdentityTable.Sort(order.Canonical[flow.Identity]) + entry.CurrentEpochIdentityTable = currentEpochParticipants.Union(nextEpochParticipants.Map(mapfunc.WithWeight(0))) + entry.NextEpochIdentityTable = nextEpochParticipants.Union(currentEpochParticipants.Map(mapfunc.WithWeight(0))) - entry.CurrentEpoch.ActiveIdentities = flow.DynamicIdentityEntryListFromIdentities(entry.CurrentEpochIdentityTable) entry.NextEpoch = &flow.EpochStateContainer{ SetupID: nextEpochSetup.ID(), CommitID: nextEpochCommit.ID(), - ActiveIdentities: flow.DynamicIdentityEntryListFromIdentities(entry.NextEpochIdentityTable), + ActiveIdentities: flow.DynamicIdentityEntryListFromIdentities(nextEpochParticipants), } entry.NextEpochSetup = nextEpochSetup entry.NextEpochCommit = nextEpochCommit From d8046b5baaddf827d49589615c04c8e1463c05dd Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 18 Oct 2023 10:20:29 +0300 Subject: [PATCH 02/15] Updated storage layer assertions and process of reconstructing identity table --- model/flow/protocol_state.go | 48 ++++++++++++++------------- storage/badger/protocol_state_test.go | 34 +++++++++---------- 2 files changed, 40 insertions(+), 42 deletions(-) diff --git a/model/flow/protocol_state.go b/model/flow/protocol_state.go index 98ae4176fdc..19de13c4e18 100644 --- a/model/flow/protocol_state.go +++ b/model/flow/protocol_state.go @@ -374,32 +374,11 @@ func BuildIdentityTable( adjacentEpochDynamicIdentities DynamicIdentityEntryList, ) (IdentityList, error) { - reconstructIdentityTable := func(skeletons IdentitySkeletonList, dynamics DynamicIdentityEntryList) (IdentityList, error) { - // sanity check: size of identities should be equal to previous and current epoch participants combined - if len(skeletons) != len(dynamics) { - return nil, fmt.Errorf("invalid number of identities in protocol state: expected %d, got %d", len(skeletons), len(dynamics)) - } - - // build full identity table for current epoch - var result IdentityList - for i := range dynamics { - // sanity check: identities should be sorted in canonical 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) - } - result = append(result, &Identity{ - IdentitySkeleton: *skeletons[i], - DynamicIdentity: dynamics[i].Dynamic, - }) - } - return result, nil - } - - targetEpochParticipants, err := reconstructIdentityTable(targetEpochIdentitySkeletons, targetEpochDynamicIdentities) + targetEpochParticipants, err := ReconstructIdentities(targetEpochIdentitySkeletons, targetEpochDynamicIdentities) if err != nil { return nil, fmt.Errorf("could not reconstruct participants for target epoch: %w", err) } - adjacentEpochParticipants, err := reconstructIdentityTable(adjacentEpochIdentitySkeletons, adjacentEpochDynamicIdentities) + adjacentEpochParticipants, err := ReconstructIdentities(adjacentEpochIdentitySkeletons, adjacentEpochDynamicIdentities) if err != nil { return nil, fmt.Errorf("could not reconstruct participants for adjacent epoch: %w", err) } @@ -423,3 +402,26 @@ func DynamicIdentityEntryListFromIdentities(identities IdentityList) DynamicIden } return dynamicIdentities } + +// ReconstructIdentities combines identity skeletons and dynamic identities to produce a flow.IdentityList. +// No errors are expected during normal operations. +func ReconstructIdentities(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)) + } + + // reconstruct identities from skeleton and dynamic parts + var result IdentityList + 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) + } + result = append(result, &Identity{ + IdentitySkeleton: *skeletons[i], + DynamicIdentity: dynamics[i].Dynamic, + }) + } + return result, nil +} diff --git a/storage/badger/protocol_state_test.go b/storage/badger/protocol_state_test.go index 0f3b007292b..4fc99d04280 100644 --- a/storage/badger/protocol_state_test.go +++ b/storage/badger/protocol_state_test.go @@ -101,7 +101,6 @@ func TestProtocolStateMergeParticipants(t *testing.T) { store := NewProtocolState(metrics, setups, commits, db, DefaultCacheSize) stateEntry := unittest.ProtocolStateFixture() - require.Equal(t, stateEntry.CurrentEpochSetup.Participants[1], stateEntry.PreviousEpochSetup.Participants[1]) // change address of participant in current epoch, so we can distinguish it from the one in previous epoch // when performing assertion. newAddress := "123" @@ -193,7 +192,10 @@ func assertRichProtocolStateValidity(t *testing.T, state *flow.RichProtocolState assert.Equal(t, state.CurrentEpochSetup.ID(), state.ProtocolStateEntry.CurrentEpoch.SetupID, "epoch setup should be for correct event ID") assert.Equal(t, state.CurrentEpochCommit.ID(), state.ProtocolStateEntry.CurrentEpoch.CommitID, "epoch commit should be for correct event ID") - var previousEpochParticipants flow.IdentityList + var ( + previousEpochParticipants flow.IdentityList + err error + ) // invariant: PreviousEpochSetup and PreviousEpochCommit should be present if respective ID is not zero. if state.PreviousEpoch.SetupID != flow.ZeroID { // invariant: PreviousEpochSetup and PreviousEpochCommit are for the same epoch. Never nil. @@ -204,36 +206,30 @@ func assertRichProtocolStateValidity(t *testing.T, state *flow.RichProtocolState assert.Equal(t, state.PreviousEpochSetup.ID(), state.ProtocolStateEntry.PreviousEpoch.SetupID, "epoch setup should be for correct event ID") assert.Equal(t, state.PreviousEpochCommit.ID(), state.ProtocolStateEntry.PreviousEpoch.CommitID, "epoch commit should be for correct event ID") - for _, participant := range state.PreviousEpochSetup.Participants { - if identity, found := state.CurrentEpochIdentityTable.ByNodeID(participant.NodeID); found { - previousEpochParticipants = append(previousEpochParticipants, identity) - } - } + // invariant: ReconstructIdentities ensures that we can build full identities of previous epoch active participants. + previousEpochParticipants, err = flow.ReconstructIdentities(state.PreviousEpochSetup.Participants, state.PreviousEpoch.ActiveIdentities) + assert.NoError(t, err, "should be able to reconstruct previous epoch active participants") } - participantsFromCurrentEpochSetup := state.CurrentEpochIdentityTable.Filter(func(i *flow.Identity) bool { - _, exists := state.CurrentEpochSetup.Participants.ByNodeID(i.NodeID) - return exists - }) + // invariant: ReconstructIdentities ensures that we can build full identities of current epoch active participants. + participantsFromCurrentEpochSetup, err := flow.ReconstructIdentities(state.CurrentEpochSetup.Participants, state.CurrentEpoch.ActiveIdentities) + assert.NoError(t, err, "should be able to reconstruct current epoch active participants") // invariant: Identities is a full identity table for the current epoch. Identities are sorted in canonical order. Without duplicates. Never nil. var allIdentities, participantsFromNextEpochSetup flow.IdentityList if state.NextEpoch != nil { - participantsFromNextEpochSetup = state.NextEpochIdentityTable.Filter(func(i *flow.Identity) bool { - _, exists := state.NextEpochSetup.Participants.ByNodeID(i.NodeID) - return exists - }) + // setup/commit phase + // invariant: ReconstructIdentities ensures that we can build full identities of next epoch active participants. + participantsFromNextEpochSetup, err = flow.ReconstructIdentities(state.NextEpochSetup.Participants, state.NextEpoch.ActiveIdentities) + assert.NoError(t, err, "should be able to reconstruct next epoch active participants") allIdentities = participantsFromCurrentEpochSetup.Union(participantsFromNextEpochSetup.Map(mapfunc.WithWeight(0))) } else { + // staking phase allIdentities = participantsFromCurrentEpochSetup.Union(previousEpochParticipants.Map(mapfunc.WithWeight(0))) } assert.Equal(t, allIdentities, state.CurrentEpochIdentityTable, "identities should be a full identity table for the current epoch, without duplicates") - for i, identity := range state.CurrentEpoch.ActiveIdentities { - assert.Equal(t, identity.NodeID, participantsFromCurrentEpochSetup[i].NodeID, "active identities should hold value from current epoch") - } - nextEpoch := state.NextEpoch if nextEpoch == nil { return From 641dbca8235dc13609871fc85ded0be244bcc448 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 18 Oct 2023 11:30:49 +0300 Subject: [PATCH 03/15] Changed how state updater processes epoch setup event. Updated tests --- model/flow/protocol_state_test.go | 13 ++++- .../inmem/dynamic_protocol_state_test.go | 6 +-- state/protocol/protocol_state/updater.go | 44 +--------------- state/protocol/protocol_state/updater_test.go | 50 +++++++------------ 4 files changed, 33 insertions(+), 80 deletions(-) diff --git a/model/flow/protocol_state_test.go b/model/flow/protocol_state_test.go index b150f576306..784e42952b0 100644 --- a/model/flow/protocol_state_test.go +++ b/model/flow/protocol_state_test.go @@ -34,7 +34,11 @@ func TestNewRichProtocolStateEntry(t *testing.T) { CommitID: currentEpochCommit.ID(), ActiveIdentities: identities, }, - PreviousEpoch: flow.EventIDs{}, + PreviousEpoch: flow.EpochStateContainer{ + SetupID: flow.ZeroID, + CommitID: flow.ZeroID, + ActiveIdentities: nil, + }, InvalidStateTransitionAttempted: false, } entry, err := flow.NewRichProtocolStateEntry( @@ -47,7 +51,7 @@ func TestNewRichProtocolStateEntry(t *testing.T) { nil, ) assert.NoError(t, err) - expectedIdentities, err := flow.BuildIdentityTable(identities, setup.Participants, nil) + expectedIdentities, err := flow.BuildIdentityTable(identities, setup.Participants, nil, nil) assert.NoError(t, err) assert.Equal(t, expectedIdentities, entry.CurrentEpochIdentityTable, "should be equal to current epoch setup participants") }) @@ -72,6 +76,7 @@ func TestNewRichProtocolStateEntry(t *testing.T) { stateEntry.CurrentEpoch.ActiveIdentities, stateEntry.CurrentEpochSetup.Participants, stateEntry.PreviousEpochSetup.Participants, + stateEntry.PreviousEpoch.ActiveIdentities, ) assert.NoError(t, err) assert.Equal(t, expectedIdentities, richEntry.CurrentEpochIdentityTable, "should be equal to current epoch setup participants + previous epoch setup participants") @@ -102,6 +107,7 @@ func TestNewRichProtocolStateEntry(t *testing.T) { stateEntry.CurrentEpoch.ActiveIdentities, stateEntry.CurrentEpochSetup.Participants, stateEntry.NextEpochSetup.Participants, + stateEntry.NextEpoch.ActiveIdentities, ) assert.NoError(t, err) assert.Equal(t, expectedIdentities, richEntry.CurrentEpochIdentityTable, "should be equal to current epoch setup participants + next epoch setup participants") @@ -110,6 +116,7 @@ func TestNewRichProtocolStateEntry(t *testing.T) { stateEntry.NextEpoch.ActiveIdentities, stateEntry.NextEpochSetup.Participants, stateEntry.CurrentEpochSetup.Participants, + stateEntry.CurrentEpoch.ActiveIdentities, ) assert.NoError(t, err) assert.Equal(t, expectedIdentities, richEntry.NextEpochIdentityTable, "should be equal to next epoch setup participants + current epoch setup participants") @@ -138,6 +145,7 @@ func TestNewRichProtocolStateEntry(t *testing.T) { stateEntry.CurrentEpoch.ActiveIdentities, stateEntry.CurrentEpochSetup.Participants, stateEntry.NextEpochSetup.Participants, + stateEntry.NextEpoch.ActiveIdentities, ) assert.NoError(t, err) assert.Equal(t, expectedIdentities, richEntry.CurrentEpochIdentityTable, "should be equal to current epoch setup participants + next epoch setup participants") @@ -145,6 +153,7 @@ func TestNewRichProtocolStateEntry(t *testing.T) { stateEntry.NextEpoch.ActiveIdentities, stateEntry.NextEpochSetup.Participants, stateEntry.CurrentEpochSetup.Participants, + stateEntry.CurrentEpoch.ActiveIdentities, ) assert.NoError(t, err) assert.Equal(t, expectedIdentities, richEntry.NextEpochIdentityTable, "should be equal to next epoch setup participants + current epoch setup participants") diff --git a/state/protocol/inmem/dynamic_protocol_state_test.go b/state/protocol/inmem/dynamic_protocol_state_test.go index 229c9734dae..cb757ee2661 100644 --- a/state/protocol/inmem/dynamic_protocol_state_test.go +++ b/state/protocol/inmem/dynamic_protocol_state_test.go @@ -33,7 +33,7 @@ func TestDynamicProtocolStateAdapter(t *testing.T) { entry := unittest.ProtocolStateFixture() adapter := inmem.NewDynamicProtocolStateAdapter(entry, globalParams) status := adapter.EpochStatus() - assert.Equal(t, entry.PreviousEpoch, status.PreviousEpoch) + assert.Equal(t, entry.PreviousEpoch.EventIDs(), status.PreviousEpoch) assert.Equal(t, flow.EventIDs{ SetupID: entry.CurrentEpoch.SetupID, CommitID: entry.CurrentEpoch.CommitID, @@ -48,7 +48,7 @@ func TestDynamicProtocolStateAdapter(t *testing.T) { adapter := inmem.NewDynamicProtocolStateAdapter(entry, globalParams) status := adapter.EpochStatus() - assert.Equal(t, entry.PreviousEpoch, status.PreviousEpoch) + assert.Equal(t, entry.PreviousEpoch.EventIDs(), status.PreviousEpoch) assert.Equal(t, flow.EventIDs{ SetupID: entry.CurrentEpoch.SetupID, CommitID: entry.CurrentEpoch.CommitID, @@ -63,7 +63,7 @@ func TestDynamicProtocolStateAdapter(t *testing.T) { entry := unittest.ProtocolStateFixture(unittest.WithNextEpochProtocolState()) adapter := inmem.NewDynamicProtocolStateAdapter(entry, globalParams) status := adapter.EpochStatus() - assert.Equal(t, entry.PreviousEpoch, status.PreviousEpoch) + assert.Equal(t, entry.PreviousEpoch.EventIDs(), status.PreviousEpoch) assert.Equal(t, flow.EventIDs{ SetupID: entry.CurrentEpoch.SetupID, CommitID: entry.CurrentEpoch.CommitID, diff --git a/state/protocol/protocol_state/updater.go b/state/protocol/protocol_state/updater.go index cc419a7f98f..5b2f5cb372b 100644 --- a/state/protocol/protocol_state/updater.go +++ b/state/protocol/protocol_state/updater.go @@ -86,34 +86,12 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { // by definition, this will include identities from current epoch + identities from previous epoch with 0 weight. activeIdentitiesLookup := u.parentState.CurrentEpoch.ActiveIdentities.Lookup() - currentEpochSetupParticipants := u.parentState.CurrentEpochSetup.Participants // construct identities for current epoch: current epoch participants + next epoch participants with 0 weight - currentEpochIdentities := make(flow.DynamicIdentityEntryList, 0, len(currentEpochSetupParticipants)) // In this loop, we will perform step 1 from above. - for _, identity := range currentEpochSetupParticipants { - identityParentState := activeIdentitiesLookup[identity.NodeID] - currentEpochIdentities = append(currentEpochIdentities, &flow.DynamicIdentityEntry{ - NodeID: identity.NodeID, - Dynamic: identityParentState.Dynamic, - }) - } - nextEpochIdentities := make(flow.DynamicIdentityEntryList, 0, len(currentEpochIdentities)) - currentEpochIdentitiesLookup := currentEpochIdentities.Lookup() + nextEpochIdentities := make(flow.DynamicIdentityEntryList, 0, len(epochSetup.Participants)) // For an `identity` participating in the upcoming epoch, we effectively perform steps 2 and 3 from above within a single loop. for _, identity := range epochSetup.Participants { - // Step 2: node is _not_ participating in the current epoch, but joining in the upcoming epoch. - // The node is allowed to join the network already in this epoch's Setup Phase, but has weight 0. - if _, found := currentEpochIdentitiesLookup[identity.NodeID]; !found { - currentEpochIdentities = append(currentEpochIdentities, &flow.DynamicIdentityEntry{ - NodeID: identity.NodeID, - Dynamic: flow.DynamicIdentity{ - Weight: 0, - Ejected: false, - }, - }) - } - // Step 3: for the next epoch we include every identity from its setup event; identityParentState, found := activeIdentitiesLookup[identity.NodeID] nextEpochIdentities = append(nextEpochIdentities, &flow.DynamicIdentityEntry{ @@ -125,26 +103,6 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { }) } - nextEpochIdentitiesLookup := nextEpochIdentities.Lookup() - // Step 4: we need to extend the next epoch's identities by adding identities that are leaving at the end of - // the current epoch. Specifically, each identity from the current epoch that is _not_ listed in the - // Setup Event for the next epoch is added with 0 weight and the _current_ value of the Ejected flag. - for _, identity := range currentEpochSetupParticipants { - if _, found := nextEpochIdentitiesLookup[identity.NodeID]; !found { - identityParentState := activeIdentitiesLookup[identity.NodeID] - nextEpochIdentities = append(nextEpochIdentities, &flow.DynamicIdentityEntry{ - NodeID: identity.NodeID, - Dynamic: flow.DynamicIdentity{ - Weight: 0, - Ejected: identityParentState.Dynamic.Ejected, - }, - }) - } - } - - // IMPORTANT: per convention, identities must be listed on canonical order! - u.state.CurrentEpoch.ActiveIdentities = currentEpochIdentities.Sort(order.IdentifierCanonical) - // construct protocol state entry for next epoch u.state.NextEpoch = &flow.EpochStateContainer{ SetupID: epochSetup.ID(), diff --git a/state/protocol/protocol_state/updater_test.go b/state/protocol/protocol_state/updater_test.go index 3c3feab7a1b..c1bd031280a 100644 --- a/state/protocol/protocol_state/updater_test.go +++ b/state/protocol/protocol_state/updater_test.go @@ -7,7 +7,6 @@ import ( "github.com/stretchr/testify/suite" "github.com/onflow/flow-go/model/flow" - "github.com/onflow/flow-go/model/flow/mapfunc" "github.com/onflow/flow-go/model/flow/order" "github.com/onflow/flow-go/utils/unittest" ) @@ -297,18 +296,8 @@ func (s *UpdaterSuite) TestProcessEpochSetupHappyPath() { setup.Participants = setupParticipants.ToSkeleton() }) - participantsFromCurrentEpochSetup := s.parentProtocolState.CurrentEpochIdentityTable.Filter(func(i *flow.Identity) bool { - _, exists := s.parentProtocolState.CurrentEpochSetup.Participants.ByNodeID(i.NodeID) - return exists - }) - // for current epoch we will have all the nodes from the current epoch + nodes from the next epoch with 0 weight - expectedCurrentEpochIdentityTable := flow.DynamicIdentityEntryListFromIdentities( - participantsFromCurrentEpochSetup.Union(setupParticipants.Map(mapfunc.WithWeight(0))), - ) - // for next epoch we will have all the nodes from the next epoch + nodes from the current epoch with 0 weight - expectedNextEpochIdentityTable := flow.DynamicIdentityEntryListFromIdentities( - setupParticipants.Union(participantsFromCurrentEpochSetup.Map(mapfunc.WithWeight(0))), - ) + // for next epoch we will have all the identities from setup event + expectedNextEpochActiveIdentities := flow.DynamicIdentityEntryListFromIdentities(setupParticipants) // process actual event err := s.updater.ProcessEpochSetup(setup) @@ -316,22 +305,25 @@ func (s *UpdaterSuite) TestProcessEpochSetupHappyPath() { updatedState, _, hasChanges := s.updater.Build() require.True(s.T(), hasChanges, "should have changes") - require.Equal(s.T(), expectedCurrentEpochIdentityTable, updatedState.CurrentEpoch.ActiveIdentities) + require.Equal(s.T(), s.parentProtocolState.PreviousEpoch.ActiveIdentities, updatedState.PreviousEpoch.ActiveIdentities, + "should not change active identities for previous epoch") + require.Equal(s.T(), s.parentProtocolState.CurrentEpoch.ActiveIdentities, updatedState.CurrentEpoch.ActiveIdentities, + "should not change active identities for current epoch") nextEpoch := updatedState.NextEpoch require.NotNil(s.T(), nextEpoch, "should have next epoch protocol state") require.Equal(s.T(), nextEpoch.SetupID, setup.ID(), "should have correct setup ID for next protocol state") - require.Equal(s.T(), expectedNextEpochIdentityTable, nextEpoch.ActiveIdentities) + require.Equal(s.T(), expectedNextEpochActiveIdentities, nextEpoch.ActiveIdentities, + "should have filled active identities for next epoch") } // TestProcessEpochSetupWithSameParticipants tests that processing epoch setup with overlapping participants results in correctly // built updated protocol state. It should build a union of participants from current and next epoch for current and // next epoch protocol states respectively. func (s *UpdaterSuite) TestProcessEpochSetupWithSameParticipants() { - participantsFromCurrentEpochSetup := s.parentProtocolState.CurrentEpochIdentityTable.Filter(func(i *flow.Identity) bool { - _, exists := s.parentProtocolState.CurrentEpochSetup.Participants.ByNodeID(i.NodeID) - return exists - }).Sort(order.Canonical[flow.Identity]) + participantsFromCurrentEpochSetup, err := flow.ReconstructIdentities(s.parentProtocolState.CurrentEpochSetup.Participants, + s.parentProtocolState.CurrentEpoch.ActiveIdentities) + require.NoError(s.T(), err) overlappingNodes, err := participantsFromCurrentEpochSetup.Sample(2) require.NoError(s.T(), err) @@ -345,19 +337,13 @@ func (s *UpdaterSuite) TestProcessEpochSetupWithSameParticipants() { require.NoError(s.T(), err) updatedState, _, _ := s.updater.Build() - expectedParticipants := flow.DynamicIdentityEntryListFromIdentities( - participantsFromCurrentEpochSetup.Union(setupParticipants.Map(mapfunc.WithWeight(0))), - ) - require.Equal(s.T(), updatedState.CurrentEpoch.ActiveIdentities, - expectedParticipants, - "should have all participants from current epoch and next epoch, but without duplicates") - - nextEpochParticipants := flow.DynamicIdentityEntryListFromIdentities( - setupParticipants.Union(participantsFromCurrentEpochSetup.Map(mapfunc.WithWeight(0))), - ) - require.Equal(s.T(), updatedState.NextEpoch.ActiveIdentities, - nextEpochParticipants, - "should have all participants from previous epoch and current epoch, but without duplicates") + require.Equal(s.T(), s.parentProtocolState.CurrentEpoch.ActiveIdentities, + updatedState.CurrentEpoch.ActiveIdentities, + "should not change active identities for current epoch") + + expectedNextEpochActiveIdentities := flow.DynamicIdentityEntryListFromIdentities(setupParticipants) + require.Equal(s.T(), expectedNextEpochActiveIdentities, updatedState.NextEpoch.ActiveIdentities, + "should have filled active identities for next epoch") } // TestEpochSetupAfterIdentityChange tests that after processing epoch an setup event, all previously made changes to the identity table From dd04ca45eb145bdb5bf0339f6fb4265641cdee1f Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 18 Oct 2023 14:07:15 +0300 Subject: [PATCH 04/15] Changed how identities are applied to protocol state. Fixed some tests --- consensus/integration/epoch_test.go | 23 --------------- state/protocol/protocol_state/updater.go | 36 +++++++++++++++--------- 2 files changed, 22 insertions(+), 37 deletions(-) diff --git a/consensus/integration/epoch_test.go b/consensus/integration/epoch_test.go index 4381e7fbe48..2abac93f04d 100644 --- a/consensus/integration/epoch_test.go +++ b/consensus/integration/epoch_test.go @@ -11,7 +11,6 @@ import ( "github.com/onflow/flow-go/model/encodable" "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/model/flow/filter" - "github.com/onflow/flow-go/model/flow/mapfunc" "github.com/onflow/flow-go/model/flow/order" "github.com/onflow/flow-go/state/protocol" "github.com/onflow/flow-go/state/protocol/inmem" @@ -220,7 +219,6 @@ func withNextEpoch( encodableSnapshot := snapshot.Encodable() currEpoch := &encodableSnapshot.Epochs.Current // take pointer so assignments apply - currentEpochIdentities, _ := snapshot.Identities(filter.Any) nextEpochIdentities = nextEpochIdentities.Sort(order.Canonical[flow.Identity]) currEpoch.FinalView = currEpoch.FirstView + curEpochViews - 1 // first epoch lasts curEpochViews @@ -249,27 +247,6 @@ func withNextEpoch( // update protocol state protocolState := encodableSnapshot.ProtocolState - // set identities for root snapshot to include next epoch identities, - // since we are in committed phase - protocolState.CurrentEpoch.ActiveIdentities = flow.DynamicIdentityEntryListFromIdentities( - append( - // all the current epoch identities - currentEpochIdentities, - // and all the NEW identities in next epoch, with 0 weight - nextEpochIdentities. - Filter(filter.Not(filter.In[flow.Identity](currentEpochIdentities))). - Map(mapfunc.WithWeight(0))..., - ).Sort(order.Canonical[flow.Identity])) - - nextEpochIdentities = append( - // all the next epoch identities - nextEpochIdentities, - // and all identities from current epoch, with 0 weight - currentEpochIdentities. - Filter(filter.Not(filter.In(nextEpochIdentities))). - Map(mapfunc.WithWeight(0))..., - ).Sort(order.Canonical[flow.Identity]) - // setup ID has changed, need to update it convertedEpochSetup, _ := protocol.ToEpochSetup(inmem.NewEpoch(*currEpoch)) protocolState.CurrentEpoch.SetupID = convertedEpochSetup.ID() diff --git a/state/protocol/protocol_state/updater.go b/state/protocol/protocol_state/updater.go index 5b2f5cb372b..9e3b4cb821f 100644 --- a/state/protocol/protocol_state/updater.go +++ b/state/protocol/protocol_state/updater.go @@ -22,13 +22,18 @@ type Updater struct { state *flow.ProtocolStateEntry candidate *flow.Header - // nextEpochIdentitiesLookup is a map from NodeID → DynamicIdentityEntry for the _current_ epoch, containing the - // same identities as in the EpochStateContainer `state.CurrentEpoch.Identities`. Note that map values are pointers, + // prevEpochIdentitiesLookup is a map from NodeID → DynamicIdentityEntry for the _previous_ epoch, containing the + // same identities as in the EpochStateContainer `state.PreviousEpoch.ActiveIdentities`. Note that map values are pointers, + // so writes to map values will modify the respective DynamicIdentityEntry in EpochStateContainer. + prevEpochIdentitiesLookup map[flow.Identifier]*flow.DynamicIdentityEntry + + // currentEpochIdentitiesLookup is a map from NodeID → DynamicIdentityEntry for the _current_ epoch, containing the + // same identities as in the EpochStateContainer `state.CurrentEpoch.ActiveIdentities`. Note that map values are pointers, // so writes to map values will modify the respective DynamicIdentityEntry in EpochStateContainer. currentEpochIdentitiesLookup map[flow.Identifier]*flow.DynamicIdentityEntry // nextEpochIdentitiesLookup is a map from NodeID → DynamicIdentityEntry for the _next_ epoch, containing the - // same identities as in the EpochStateContainer `state.NextEpoch.Identities`. Note that map values are pointers, + // same identities as in the EpochStateContainer `state.NextEpoch.ActiveIdentities`. Note that map values are pointers, // so writes to map values will modify the respective DynamicIdentityEntry in EpochStateContainer. nextEpochIdentitiesLookup map[flow.Identifier]*flow.DynamicIdentityEntry } @@ -145,17 +150,20 @@ func (u *Updater) ProcessEpochCommit(epochCommit *flow.EpochCommit) error { // No errors are expected during normal operations. func (u *Updater) UpdateIdentity(updated *flow.DynamicIdentityEntry) error { u.ensureLookupPopulated() - currentEpochIdentity, found := u.currentEpochIdentitiesLookup[updated.NodeID] - if !found { - return fmt.Errorf("expected to find identity for current epoch, but (%v) not found", updated.NodeID) + prevEpochIdentity, foundInPrev := u.prevEpochIdentitiesLookup[updated.NodeID] + if foundInPrev { + prevEpochIdentity.Dynamic = updated.Dynamic } - - currentEpochIdentity.Dynamic = updated.Dynamic - if u.state.NextEpoch != nil { - nextEpochIdentity, found := u.nextEpochIdentitiesLookup[updated.NodeID] - if found { - nextEpochIdentity.Dynamic = updated.Dynamic - } + currentEpochIdentity, foundInCurrent := u.currentEpochIdentitiesLookup[updated.NodeID] + if foundInCurrent { + currentEpochIdentity.Dynamic = updated.Dynamic + } + nextEpochIdentity, foundInNext := u.nextEpochIdentitiesLookup[updated.NodeID] + if foundInNext { + nextEpochIdentity.Dynamic = updated.Dynamic + } + if !foundInPrev && !foundInCurrent && !foundInNext { + return fmt.Errorf("expected to find identity for prev, current or next epoch, but (%v) was not found", updated.NodeID) } return nil } @@ -226,6 +234,6 @@ func (u *Updater) rebuildIdentityLookup() { if u.state.NextEpoch != nil { u.nextEpochIdentitiesLookup = u.state.NextEpoch.ActiveIdentities.Lookup() } else { - u.nextEpochIdentitiesLookup = nil + u.prevEpochIdentitiesLookup = u.state.PreviousEpoch.ActiveIdentities.Lookup() } } From 09fdfbe8134dd639d2f34d94d97c3e418d8789aa Mon Sep 17 00:00:00 2001 From: Alexander Hentschel Date: Fri, 20 Oct 2023 01:15:07 -0700 Subject: [PATCH 05/15] =?UTF-8?q?model/flow/protocol=5Fstate.go:=20?= =?UTF-8?q?=E2=80=A2=20revised=20goDoc=20of=20protocol=20state=20implement?= =?UTF-8?q?ation=20=E2=80=A2=20changed=20`ProtocolStateEntry.PreviousEpoch?= =?UTF-8?q?`=20to=20pointer=20(consistent=20with=20`ProtocolStateEntry.Nex?= =?UTF-8?q?tEpoch`)=20=E2=80=A2=20There=20should=20be=20no=20significant?= =?UTF-8?q?=20algorithmic=20changes.=20Though,=20I=20have=20switched=20the?= =?UTF-8?q?=20order=20of=20the=20if=20and=20else=20branches=20when=20proce?= =?UTF-8?q?ssing=20an=20Epoch=20Setup=20event.=20For=20me,=20this=20is=20m?= =?UTF-8?q?uch=20more=20in=20line=20with=20the=20intuitive=20order=20of=20?= =?UTF-8?q?documentation.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit state/protocol/protocol_state/updater.go: • notable updates and revisions of goDoc • tried to address concerns around inconsistent handling of invariances • updated code to work with `ProtocolStateEntry.PreviousEpoch` being potentially a nil pointer storage/badger/protocol_state_test.go: • detailed goDoc revisions for test `assertRichProtocolStateValidity` --- model/flow/mapfunc/identity.go | 4 +- model/flow/protocol_state.go | 117 ++++++++--------- model/flow/protocol_state_test.go | 2 +- state/protocol/inmem/convert.go | 2 +- state/protocol/protocol_state/updater.go | 123 ++++++++++-------- state/protocol/protocol_state/updater_test.go | 7 +- storage/badger/protocol_state.go | 2 +- storage/badger/protocol_state_test.go | 59 +++++---- utils/unittest/fixtures.go | 6 +- 9 files changed, 176 insertions(+), 146 deletions(-) diff --git a/model/flow/mapfunc/identity.go b/model/flow/mapfunc/identity.go index a58bc7d0844..ad03ec5cee2 100644 --- a/model/flow/mapfunc/identity.go +++ b/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 diff --git a/model/flow/protocol_state.go b/model/flow/protocol_state.go index 7a669393da0..48f255d3435 100644 --- a/model/flow/protocol_state.go +++ b/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], diff --git a/model/flow/protocol_state_test.go b/model/flow/protocol_state_test.go index 784e42952b0..1585361d91a 100644 --- a/model/flow/protocol_state_test.go +++ b/model/flow/protocol_state_test.go @@ -34,7 +34,7 @@ func TestNewRichProtocolStateEntry(t *testing.T) { CommitID: currentEpochCommit.ID(), ActiveIdentities: identities, }, - PreviousEpoch: flow.EpochStateContainer{ + PreviousEpoch: &flow.EpochStateContainer{ SetupID: flow.ZeroID, CommitID: flow.ZeroID, ActiveIdentities: nil, diff --git a/state/protocol/inmem/convert.go b/state/protocol/inmem/convert.go index 83214eafd12..8c1488f4aca 100644 --- a/state/protocol/inmem/convert.go +++ b/state/protocol/inmem/convert.go @@ -334,7 +334,7 @@ func SnapshotFromBootstrapStateWithParams( }) } protocolState := &flow.ProtocolStateEntry{ - PreviousEpoch: flow.EpochStateContainer{ + PreviousEpoch: &flow.EpochStateContainer{ SetupID: flow.ZeroID, CommitID: flow.ZeroID, ActiveIdentities: nil, diff --git a/state/protocol/protocol_state/updater.go b/state/protocol/protocol_state/updater.go index 9e3b4cb821f..2374d66a7d4 100644 --- a/state/protocol/protocol_state/updater.go +++ b/state/protocol/protocol_state/updater.go @@ -22,20 +22,13 @@ type Updater struct { state *flow.ProtocolStateEntry candidate *flow.Header - // prevEpochIdentitiesLookup is a map from NodeID → DynamicIdentityEntry for the _previous_ epoch, containing the - // same identities as in the EpochStateContainer `state.PreviousEpoch.ActiveIdentities`. Note that map values are pointers, - // so writes to map values will modify the respective DynamicIdentityEntry in EpochStateContainer. - prevEpochIdentitiesLookup map[flow.Identifier]*flow.DynamicIdentityEntry - - // currentEpochIdentitiesLookup is a map from NodeID → DynamicIdentityEntry for the _current_ epoch, containing the - // same identities as in the EpochStateContainer `state.CurrentEpoch.ActiveIdentities`. Note that map values are pointers, - // so writes to map values will modify the respective DynamicIdentityEntry in EpochStateContainer. - currentEpochIdentitiesLookup map[flow.Identifier]*flow.DynamicIdentityEntry - - // nextEpochIdentitiesLookup is a map from NodeID → DynamicIdentityEntry for the _next_ epoch, containing the - // same identities as in the EpochStateContainer `state.NextEpoch.ActiveIdentities`. Note that map values are pointers, - // so writes to map values will modify the respective DynamicIdentityEntry in EpochStateContainer. - nextEpochIdentitiesLookup map[flow.Identifier]*flow.DynamicIdentityEntry + // The following fields are maps from NodeID → DynamicIdentityEntry for the nodes that are *active* in the respective epoch. + // Active means that these nodes are authorized to contribute to extending the chain. Formally, as node is active if and only + // if it is listed in the EpochSetup event for the respective epoch. + + prevEpochIdentitiesLookup map[flow.Identifier]*flow.DynamicIdentityEntry // lookup for nodes active in the previous epoch, may be nil or empty + currentEpochIdentitiesLookup map[flow.Identifier]*flow.DynamicIdentityEntry // lookup for nodes active in the current epoch, never nil or empty + nextEpochIdentitiesLookup map[flow.Identifier]*flow.DynamicIdentityEntry // lookup for nodes active in the next epoch, may be nil or empty } var _ protocol.StateUpdater = (*Updater)(nil) @@ -67,7 +60,7 @@ func (u *Updater) Build() (updatedState *flow.ProtocolStateEntry, stateID flow.I // No errors are expected during normal operations. func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { if epochSetup.Counter != u.parentState.CurrentEpochSetup.Counter+1 { - return fmt.Errorf("invalid epoch setup counter, expecting %v got %v", u.parentState.CurrentEpochSetup.Counter+1, epochSetup.Counter) + return fmt.Errorf("invalid epoch setup counter, expecting %d got %d", u.parentState.CurrentEpochSetup.Counter+1, epochSetup.Counter) } if u.state.NextEpoch != nil { return fmt.Errorf("protocol state has already a setup event") @@ -75,49 +68,67 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { if u.state.InvalidStateTransitionAttempted { return nil // won't process new events if we are in EECC } - // Observing epoch setup impacts current and next epoch. - // - For the current epoch, we stop returning identities from previous epoch. - // Instead, we will return identities of current epoch + identities from the next epoch with 0 weight. - // - For the next epoch we need to additionally include identities from previous(current) epoch with 0 weight. - // We will do this in next steps: - // 1. Add identities from current epoch setup event to currentEpochIdentities. - // 2. Add identities from next epoch setup event to currentEpochIdentities, with 0 weight, - // but only if they are not present in currentEpochIdentities. - // 3. Add identities from next epoch setup event to nextEpochIdentities. - // 4. Add identities from current epoch setup event to nextEpochIdentities, with 0 weight, - // but only if they are not present in nextEpochIdentities. - - // lookup of dynamic data for current protocol state identities - // by definition, this will include identities from current epoch + identities from previous epoch with 0 weight. - activeIdentitiesLookup := u.parentState.CurrentEpoch.ActiveIdentities.Lookup() - - // construct identities for current epoch: current epoch participants + next epoch participants with 0 weight - // In this loop, we will perform step 1 from above. - - nextEpochIdentities := make(flow.DynamicIdentityEntryList, 0, len(epochSetup.Participants)) - // For an `identity` participating in the upcoming epoch, we effectively perform steps 2 and 3 from above within a single loop. - for _, identity := range epochSetup.Participants { - // Step 3: for the next epoch we include every identity from its setup event; - identityParentState, found := activeIdentitiesLookup[identity.NodeID] - nextEpochIdentities = append(nextEpochIdentities, &flow.DynamicIdentityEntry{ - NodeID: identity.NodeID, + + // When observing setup event for subsequent epoch, construct the EpochStateContainer for `ProtocolStateEntry.NextEpoch`. + // Context: + // Note that the `EpochStateContainer.ActiveIdentities` only contains the nodes that are *active* in the next epoch. Active means + // that these nodes are authorized to contribute to extending the chain. Nodes are listed in `ActiveIdentities` if and only if + // they are part of the EpochSetup event for the respective epoch. + // + // sanity checking SAFETY-CRITICAL INVARIANT (I): + // While ejection status and dynamic weight are not part of the EpochSetup event, we can supplement this information as follows: + // - Per convention, service events are delivered (asynchronously) in an *order-preserving* manner. Furthermore, weight changes or + // node ejection is entirely mediated by system smart contracts and delivered via service events. + // - Therefore, the EpochSetup event contains the up-to-date snapshot of the cluster members. Any weight changes or node ejection + // that happened before should be reflected in the EpochSetup event. Specifically, the initial weight should be reduced and ejected + // nodes should be no longer listed in the EpochSetup event. + // - Hence, the following invariant must be satisfied by the system smart contracts for all active nodes in the upcoming epoch: + // (i) When the EpochSetup event is emitted / processed, the weight of all active nodes equals their InitialWeight and + // (ii) The Ejected flag is false. Node X being ejected in epoch N (necessarily via a service event emitted by the system + // smart contracts earlier) but also being listed in the setup event for the subsequent epoch (service event emitted by + // the system smart contracts later) is illegal. + // sanity checking SAFETY-CRITICAL INVARIANT (II): + // - Per convention, the system smart contracts should list the IdentitySkeletons in canonical order. This is useful for + // most efficient construction of the full active Identities for an epoch. + + // For collector clusters, we rely on invariants (I) and (II) holding. See `committees.Cluster` for details, specifically function + // `constructInitialClusterIdentities(..)`. While the system smart contract must satisfy this invariant, we run a sanity check below. + // TODO: In case the invariant is violated (likely bug in system smart contracts), we should go into EECC and not reject the block containing the service event. + // + activeIdentitiesLookup := u.parentState.CurrentEpoch.ActiveIdentities.Lookup() // lookup NodeID → DynamicIdentityEntry for nodes _active_ in the current epoch + nextEpochActiveIdentities := make(flow.DynamicIdentityEntryList, 0, len(epochSetup.Participants)) + prevNodeID := epochSetup.Participants[0].NodeID + for idx, nextEpochIdentitySkeleton := range epochSetup.Participants { + // sanity checking invariant (I): + currentEpochDynamicProperties, found := activeIdentitiesLookup[nextEpochIdentitySkeleton.NodeID] + if found && currentEpochDynamicProperties.Dynamic.Ejected { // invariance violated + return fmt.Errorf("node %v is ejected in current epoch %d but readmitted by EpochSetup event for epoch %d", nextEpochIdentitySkeleton.NodeID, u.parentState.CurrentEpochSetup.Counter, epochSetup.Counter) + } + + // sanity checking invariant (II): + if idx > 0 && !order.IdentifierCanonical(prevNodeID, nextEpochIdentitySkeleton.NodeID) { + return fmt.Errorf("epoch setup event lists active participants not in canonical ordering") + } + prevNodeID = nextEpochIdentitySkeleton.NodeID + + nextEpochActiveIdentities = append(nextEpochActiveIdentities, &flow.DynamicIdentityEntry{ + NodeID: nextEpochIdentitySkeleton.NodeID, Dynamic: flow.DynamicIdentity{ - Weight: identity.InitialWeight, - Ejected: found && identityParentState.Dynamic.Ejected, + Weight: nextEpochIdentitySkeleton.InitialWeight, + Ejected: false, }, }) } - // construct protocol state entry for next epoch + // construct data container specifying next epoch u.state.NextEpoch = &flow.EpochStateContainer{ SetupID: epochSetup.ID(), CommitID: flow.ZeroID, - ActiveIdentities: nextEpochIdentities.Sort(order.IdentifierCanonical), + ActiveIdentities: nextEpochActiveIdentities, } - // since identities have changed, rebuild identity lookups, so we can safely process - // subsequent epoch commit event and update identities afterward. - u.rebuildIdentityLookup() + // subsequent epoch commit event and update identities afterwards. + u.nextEpochIdentitiesLookup = u.state.NextEpoch.ActiveIdentities.Lookup() return nil } @@ -129,7 +140,7 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { // No errors are expected during normal operations. func (u *Updater) ProcessEpochCommit(epochCommit *flow.EpochCommit) error { if epochCommit.Counter != u.parentState.CurrentEpochSetup.Counter+1 { - return fmt.Errorf("invalid epoch commit counter, expecting %v got %v", u.parentState.CurrentEpochSetup.Counter+1, epochCommit.Counter) + return fmt.Errorf("invalid epoch commit counter, expecting %d got %d", u.parentState.CurrentEpochSetup.Counter+1, epochCommit.Counter) } if u.state.NextEpoch == nil { return fmt.Errorf("protocol state has been setup yet") @@ -199,7 +210,7 @@ func (u *Updater) TransitionToNextEpoch() error { return fmt.Errorf("protocol state transition is only allowed when enterring next epoch") } u.state = &flow.ProtocolStateEntry{ - PreviousEpoch: u.state.CurrentEpoch, + PreviousEpoch: &u.state.CurrentEpoch, CurrentEpoch: *u.state.NextEpoch, InvalidStateTransitionAttempted: false, } @@ -227,13 +238,19 @@ func (u *Updater) ensureLookupPopulated() { u.rebuildIdentityLookup() } -// rebuildIdentityLookup re-generates `currentEpochIdentitiesLookup` and `nextEpochIdentitiesLookup` from the -// underlying identity lists `state.CurrentEpoch.Identities` and `state.NextEpoch.Identities`, respectively. +// rebuildIdentityLookup re-generates lookups of *active* participants for +// previous (optional, if u.state.PreviousEpoch ≠ nil), current (required) and +// next epoch (optional, if u.state.NextEpoch ≠ nil). func (u *Updater) rebuildIdentityLookup() { + if u.state.PreviousEpoch != nil { + u.prevEpochIdentitiesLookup = u.state.PreviousEpoch.ActiveIdentities.Lookup() + } else { + u.prevEpochIdentitiesLookup = nil + } u.currentEpochIdentitiesLookup = u.state.CurrentEpoch.ActiveIdentities.Lookup() if u.state.NextEpoch != nil { u.nextEpochIdentitiesLookup = u.state.NextEpoch.ActiveIdentities.Lookup() } else { - u.prevEpochIdentitiesLookup = u.state.PreviousEpoch.ActiveIdentities.Lookup() + u.nextEpochIdentitiesLookup = nil } } diff --git a/state/protocol/protocol_state/updater_test.go b/state/protocol/protocol_state/updater_test.go index c1bd031280a..f888343e002 100644 --- a/state/protocol/protocol_state/updater_test.go +++ b/state/protocol/protocol_state/updater_test.go @@ -321,9 +321,14 @@ func (s *UpdaterSuite) TestProcessEpochSetupHappyPath() { // built updated protocol state. It should build a union of participants from current and next epoch for current and // next epoch protocol states respectively. func (s *UpdaterSuite) TestProcessEpochSetupWithSameParticipants() { - participantsFromCurrentEpochSetup, err := flow.ReconstructIdentities(s.parentProtocolState.CurrentEpochSetup.Participants, + participantsFromCurrentEpochSetup, err := flow.ComposeFullIdentities(s.parentProtocolState.CurrentEpochSetup.Participants, s.parentProtocolState.CurrentEpoch.ActiveIdentities) require.NoError(s.T(), err) + // Function `ComposeFullIdentities` verified that `Participants` and `ActiveIdentities` have identical ordering w.r.t nodeID. + // By construction, `participantsFromCurrentEpochSetup` lists the full Identities in the same ordering as `Participants` and + // `ActiveIdentities`. By confirming that `participantsFromCurrentEpochSetup` follows canonical ordering, we can conclude that + // also `Participants` and `ActiveIdentities` are canonically ordered. + require.True(s.T(), participantsFromCurrentEpochSetup.Sorted(order.Canonical[flow.Identity]), "participants in current epoch's setup event are not in canonical order") overlappingNodes, err := participantsFromCurrentEpochSetup.Sample(2) require.NoError(s.T(), err) diff --git a/storage/badger/protocol_state.go b/storage/badger/protocol_state.go index 21da93a5d52..4b3663dd429 100644 --- a/storage/badger/protocol_state.go +++ b/storage/badger/protocol_state.go @@ -144,7 +144,7 @@ func newRichProtocolStateEntry( err error ) // query and fill in epoch setups and commits for previous and current epochs - if protocolState.PreviousEpoch.SetupID != flow.ZeroID { + if protocolState.PreviousEpoch != nil { previousEpochSetup, err = setups.ByID(protocolState.PreviousEpoch.SetupID) if err != nil { return nil, fmt.Errorf("could not retrieve previous epoch setup: %w", err) diff --git a/storage/badger/protocol_state_test.go b/storage/badger/protocol_state_test.go index 4fc99d04280..105130927bd 100644 --- a/storage/badger/protocol_state_test.go +++ b/storage/badger/protocol_state_test.go @@ -9,6 +9,7 @@ import ( "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/model/flow/mapfunc" + "github.com/onflow/flow-go/model/flow/order" "github.com/onflow/flow-go/module/metrics" "github.com/onflow/flow-go/storage/badger/transaction" "github.com/onflow/flow-go/utils/unittest" @@ -185,10 +186,10 @@ func TestProtocolStateRootSnapshot(t *testing.T) { // assertRichProtocolStateValidity checks if RichProtocolState holds its invariant and is correctly populated by storage layer. func assertRichProtocolStateValidity(t *testing.T, state *flow.RichProtocolStateEntry) { - // invariant: CurrentEpochSetup and CurrentEpochCommit are for the same epoch. Never nil. + // invariants: + // - CurrentEpochSetup and CurrentEpochCommit are for the same epoch. Never nil. + // - CurrentEpochSetup and CurrentEpochCommit IDs match respective commitments in the `ProtocolStateEntry`. assert.Equal(t, state.CurrentEpochSetup.Counter, state.CurrentEpochCommit.Counter, "current epoch setup and commit should be for the same epoch") - - // invariant: CurrentEpochSetup and CurrentEpochCommit IDs are the equal to the ID of the protocol state entry. Never nil. assert.Equal(t, state.CurrentEpochSetup.ID(), state.ProtocolStateEntry.CurrentEpoch.SetupID, "epoch setup should be for correct event ID") assert.Equal(t, state.CurrentEpochCommit.ID(), state.ProtocolStateEntry.CurrentEpoch.CommitID, "epoch commit should be for correct event ID") @@ -197,52 +198,66 @@ func assertRichProtocolStateValidity(t *testing.T, state *flow.RichProtocolState err error ) // invariant: PreviousEpochSetup and PreviousEpochCommit should be present if respective ID is not zero. - if state.PreviousEpoch.SetupID != flow.ZeroID { + if state.PreviousEpoch != nil { // invariant: PreviousEpochSetup and PreviousEpochCommit are for the same epoch. Never nil. - assert.Equal(t, state.CurrentEpochSetup.Counter, state.PreviousEpochSetup.Counter+1, "current epoch setup should be next after previous epoch") + assert.Equal(t, state.PreviousEpochSetup.Counter+1, state.CurrentEpochSetup.Counter, "current epoch (%d) should be following right after previous epoch (%d)", state.CurrentEpochSetup.Counter, state.PreviousEpochSetup.Counter) assert.Equal(t, state.PreviousEpochSetup.Counter, state.PreviousEpochCommit.Counter, "previous epoch setup and commit should be for the same epoch") // invariant: PreviousEpochSetup and PreviousEpochCommit IDs are the equal to the ID of the protocol state entry. Never nil. assert.Equal(t, state.PreviousEpochSetup.ID(), state.ProtocolStateEntry.PreviousEpoch.SetupID, "epoch setup should be for correct event ID") assert.Equal(t, state.PreviousEpochCommit.ID(), state.ProtocolStateEntry.PreviousEpoch.CommitID, "epoch commit should be for correct event ID") - // invariant: ReconstructIdentities ensures that we can build full identities of previous epoch active participants. - previousEpochParticipants, err = flow.ReconstructIdentities(state.PreviousEpochSetup.Participants, state.PreviousEpoch.ActiveIdentities) + // invariant: ComposeFullIdentities ensures that we can build full identities of previous epoch's active participants. This step also confirms that the + // previous epoch's `Participants` [IdentitySkeletons] and `ActiveIdentities` [DynamicIdentity properties] list the same nodes in canonical ordering. + previousEpochParticipants, err = flow.ComposeFullIdentities(state.PreviousEpochSetup.Participants, state.PreviousEpoch.ActiveIdentities) assert.NoError(t, err, "should be able to reconstruct previous epoch active participants") + // Function `ComposeFullIdentities` verified that `Participants` and `ActiveIdentities` have identical ordering w.r.t nodeID. + // By construction, `participantsFromCurrentEpochSetup` lists the full Identities in the same ordering as `Participants` and + // `ActiveIdentities`. By confirming that `participantsFromCurrentEpochSetup` follows canonical ordering, we can conclude that + // also `Participants` and `ActiveIdentities` are canonically ordered. + require.True(t, previousEpochParticipants.Sorted(order.Canonical[flow.Identity]), "participants in previous epoch's setup event are not in canonical order") } - // invariant: ReconstructIdentities ensures that we can build full identities of current epoch active participants. - participantsFromCurrentEpochSetup, err := flow.ReconstructIdentities(state.CurrentEpochSetup.Participants, state.CurrentEpoch.ActiveIdentities) + // invariant: ComposeFullIdentities ensures that we can build full identities of current epoch's *active* participants. This step also confirms that the + // current epoch's `Participants` [IdentitySkeletons] and `ActiveIdentities` [DynamicIdentity properties] list the same nodes in canonical ordering. + participantsFromCurrentEpochSetup, err := flow.ComposeFullIdentities(state.CurrentEpochSetup.Participants, state.CurrentEpoch.ActiveIdentities) assert.NoError(t, err, "should be able to reconstruct current epoch active participants") + require.True(t, participantsFromCurrentEpochSetup.Sorted(order.Canonical[flow.Identity]), "participants in current epoch's setup event are not in canonical order") - // invariant: Identities is a full identity table for the current epoch. Identities are sorted in canonical order. Without duplicates. Never nil. + // invariants for `CurrentEpochIdentityTable`: + // - full identity table containing *active* nodes for the current epoch + weight-zero identities of adjacent epoch + // - Identities are sorted in canonical order. Without duplicates. Never nil. var allIdentities, participantsFromNextEpochSetup flow.IdentityList if state.NextEpoch != nil { // setup/commit phase - // invariant: ReconstructIdentities ensures that we can build full identities of next epoch active participants. - participantsFromNextEpochSetup, err = flow.ReconstructIdentities(state.NextEpochSetup.Participants, state.NextEpoch.ActiveIdentities) + // invariant: ComposeFullIdentities ensures that we can build full identities of next epoch's *active* participants. This step also confirms that the + // next epoch's `Participants` [IdentitySkeletons] and `ActiveIdentities` [DynamicIdentity properties] list the same nodes in canonical ordering. + participantsFromNextEpochSetup, err = flow.ComposeFullIdentities(state.NextEpochSetup.Participants, state.NextEpoch.ActiveIdentities) assert.NoError(t, err, "should be able to reconstruct next epoch active participants") allIdentities = participantsFromCurrentEpochSetup.Union(participantsFromNextEpochSetup.Map(mapfunc.WithWeight(0))) } else { // staking phase allIdentities = participantsFromCurrentEpochSetup.Union(previousEpochParticipants.Map(mapfunc.WithWeight(0))) } - assert.Equal(t, allIdentities, state.CurrentEpochIdentityTable, "identities should be a full identity table for the current epoch, without duplicates") + require.True(t, allIdentities.Sorted(order.Canonical[flow.Identity]), "current epoch's identity table is not in canonical order") - nextEpoch := state.NextEpoch - if nextEpoch == nil { + // check next epoch; only applicable during setup/commit phase + if state.NextEpoch == nil { // during staking phase, next epoch is not yet specified; hence there is nothing else to check return } - // invariant: NextEpochSetup and NextEpochCommit are for the same epoch. Never nil. - assert.Equal(t, state.NextEpochSetup.Counter, state.NextEpochCommit.Counter, "next epoch setup and commit should be for the same epoch") - // invariant: NextEpochSetup and NextEpochCommit IDs are the equal to the ID of the protocol state entry. Never nil. - assert.Equal(t, state.NextEpochSetup.ID(), nextEpoch.SetupID, "epoch setup should be for correct event ID") - assert.Equal(t, state.NextEpochCommit.ID(), nextEpoch.CommitID, "epoch commit should be for correct event ID") + // invariants: + // - NextEpochSetup and NextEpochCommit are for the same epoch. Never nil. + // - NextEpochSetup and NextEpochCommit IDs match respective commitments in the `ProtocolStateEntry`. + assert.Equal(t, state.CurrentEpochSetup.Counter+1, state.NextEpochSetup.Counter, "next epoch (%d) should be following right after current epoch (%d)", state.NextEpochSetup.Counter, state.CurrentEpochSetup.Counter) + assert.Equal(t, state.NextEpochSetup.Counter, state.NextEpochCommit.Counter, "next epoch setup and commit should be for the same epoch") + assert.Equal(t, state.NextEpochSetup.ID(), state.NextEpoch.SetupID, "epoch setup should be for correct event ID") + assert.Equal(t, state.NextEpochCommit.ID(), state.NextEpoch.CommitID, "epoch commit should be for correct event ID") - // invariant: Identities is a full identity table for the current epoch. Identities are sorted in canonical order. Without duplicates. Never nil. + // invariants for `NextEpochIdentityTable`: + // - full identity table containing *active* nodes for next epoch + weight-zero identities of current epoch + // - Identities are sorted in canonical order. Without duplicates. Never nil. allIdentities = participantsFromNextEpochSetup.Union(participantsFromCurrentEpochSetup.Map(mapfunc.WithWeight(0))) - assert.Equal(t, allIdentities, state.NextEpochIdentityTable, "identities should be a full identity table for the next epoch, without duplicates") } diff --git a/utils/unittest/fixtures.go b/utils/unittest/fixtures.go index ee84c67acf0..38f7a14936e 100644 --- a/utils/unittest/fixtures.go +++ b/utils/unittest/fixtures.go @@ -4,7 +4,6 @@ import ( "bytes" crand "crypto/rand" "fmt" - "github.com/onflow/flow-go/model/flow/mapfunc" "math/rand" "net" "testing" @@ -34,6 +33,7 @@ import ( "github.com/onflow/flow-go/model/encoding" "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/model/flow/filter" + "github.com/onflow/flow-go/model/flow/mapfunc" "github.com/onflow/flow-go/model/flow/order" "github.com/onflow/flow-go/model/messages" "github.com/onflow/flow-go/model/verification" @@ -2602,7 +2602,7 @@ func RootProtocolStateFixture() *flow.RichProtocolStateEntry { CommitID: currentEpochCommit.ID(), ActiveIdentities: flow.DynamicIdentityEntryListFromIdentities(allIdentities), }, - PreviousEpoch: flow.EpochStateContainer{ + PreviousEpoch: &flow.EpochStateContainer{ SetupID: flow.ZeroID, CommitID: flow.ZeroID, ActiveIdentities: nil, @@ -2673,7 +2673,7 @@ func ProtocolStateFixture(options ...func(*flow.RichProtocolStateEntry)) *flow.R CommitID: currentEpochCommit.ID(), ActiveIdentities: flow.DynamicIdentityEntryListFromIdentities(currentEpochIdentities), }, - PreviousEpoch: flow.EpochStateContainer{ + PreviousEpoch: &flow.EpochStateContainer{ SetupID: prevEpochSetup.ID(), CommitID: prevEpochCommit.ID(), ActiveIdentities: flow.DynamicIdentityEntryListFromIdentities(prevEpochIdentities), From d9ae36dfb7d23e856ce52cc2b9d0f0c705dd071b Mon Sep 17 00:00:00 2001 From: Alexander Hentschel Date: Fri, 20 Oct 2023 01:21:13 -0700 Subject: [PATCH 06/15] minor comment revision --- state/protocol/protocol_state/updater.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/state/protocol/protocol_state/updater.go b/state/protocol/protocol_state/updater.go index 2374d66a7d4..d69a4edda1b 100644 --- a/state/protocol/protocol_state/updater.go +++ b/state/protocol/protocol_state/updater.go @@ -24,7 +24,8 @@ type Updater struct { // The following fields are maps from NodeID → DynamicIdentityEntry for the nodes that are *active* in the respective epoch. // Active means that these nodes are authorized to contribute to extending the chain. Formally, as node is active if and only - // if it is listed in the EpochSetup event for the respective epoch. + // if it is listed in the EpochSetup event for the respective epoch. Note that map values are pointers, so writes to map values + // will modify the respective DynamicIdentityEntry in EpochStateContainer. prevEpochIdentitiesLookup map[flow.Identifier]*flow.DynamicIdentityEntry // lookup for nodes active in the previous epoch, may be nil or empty currentEpochIdentitiesLookup map[flow.Identifier]*flow.DynamicIdentityEntry // lookup for nodes active in the current epoch, never nil or empty From 15e3e334ffa2ed2d8eba1aae2ee7ed24c8e44532 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 20 Oct 2023 16:15:39 +0300 Subject: [PATCH 07/15] Fixed tests --- model/flow/protocol_state_test.go | 6 +- state/protocol/inmem/convert.go | 6 +- state/protocol/protocol_state/updater_test.go | 63 ++++++++++++------- utils/unittest/fixtures.go | 7 +-- 4 files changed, 45 insertions(+), 37 deletions(-) diff --git a/model/flow/protocol_state_test.go b/model/flow/protocol_state_test.go index 1585361d91a..b23b3a66bb5 100644 --- a/model/flow/protocol_state_test.go +++ b/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( diff --git a/state/protocol/inmem/convert.go b/state/protocol/inmem/convert.go index 8c1488f4aca..bdadd156423 100644 --- a/state/protocol/inmem/convert.go +++ b/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(), diff --git a/state/protocol/protocol_state/updater_test.go b/state/protocol/protocol_state/updater_test.go index f888343e002..97c0de05c05 100644 --- a/state/protocol/protocol_state/updater_test.go +++ b/state/protocol/protocol_state/updater_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/suite" "github.com/onflow/flow-go/model/flow" + "github.com/onflow/flow-go/model/flow/filter" "github.com/onflow/flow-go/model/flow/order" "github.com/onflow/flow-go/utils/unittest" ) @@ -214,10 +215,12 @@ func (s *UpdaterSuite) TestUpdateIdentityHappyPath() { s.updater = NewUpdater(s.candidate, s.parentProtocolState) currentEpochParticipants := s.parentProtocolState.CurrentEpochIdentityTable.Copy() - weightChanges, err := currentEpochParticipants.Sample(2) + weightChanges, err := currentEpochParticipants.Sample(3) require.NoError(s.T(), err) ejectedChanges, err := currentEpochParticipants.Sample(2) require.NoError(s.T(), err) + require.Greater(s.T(), len(weightChanges), len(ejectedChanges), + "due to sampling and test setup we want to have more weight changes than ejected changes") for i, identity := range weightChanges { identity.DynamicIdentity.Weight = uint64(100 * i) } @@ -236,17 +239,24 @@ func (s *UpdaterSuite) TestUpdateIdentityHappyPath() { updatedState, _, hasChanges := s.updater.Build() require.True(s.T(), hasChanges, "should have changes") - requireUpdatesApplied := func(identityLookup map[flow.Identifier]*flow.DynamicIdentityEntry) { - for _, identity := range allUpdates { - updatedIdentity := identityLookup[identity.NodeID] - require.Equal(s.T(), identity.NodeID, updatedIdentity.NodeID) - require.Equal(s.T(), identity.DynamicIdentity, updatedIdentity.Dynamic, "identity should be updated") + // assert that all changes made in the previous epoch are preserved + currentEpochLookup := updatedState.CurrentEpoch.ActiveIdentities.Lookup() + nextEpochLookup := updatedState.NextEpoch.ActiveIdentities.Lookup() + + for _, updated := range allUpdates { + currentEpochIdentity, foundInCurrentEpoch := currentEpochLookup[updated.NodeID] + if foundInCurrentEpoch { + require.Equal(s.T(), updated.NodeID, currentEpochIdentity.NodeID) + require.Equal(s.T(), updated.DynamicIdentity, currentEpochIdentity.Dynamic) } - } - // check if changes are reflected in current and next epochs - requireUpdatesApplied(updatedState.CurrentEpoch.ActiveIdentities.Lookup()) - requireUpdatesApplied(updatedState.NextEpoch.ActiveIdentities.Lookup()) + nextEpochIdentity, foundInNextEpoch := nextEpochLookup[updated.NodeID] + if foundInNextEpoch { + require.Equal(s.T(), updated.NodeID, nextEpochIdentity.NodeID) + require.Equal(s.T(), updated.DynamicIdentity, nextEpochIdentity.Dynamic) + } + require.True(s.T(), foundInCurrentEpoch || foundInNextEpoch, "identity should be found in either current or next epoch") + } } // TestProcessEpochSetupInvariants tests if processing epoch setup when invariants are violated doesn't update internal structures. @@ -358,10 +368,12 @@ func (s *UpdaterSuite) TestEpochSetupAfterIdentityChange() { _, exists := s.parentProtocolState.CurrentEpochSetup.Participants.ByNodeID(i.NodeID) return exists }).Sort(order.Canonical[flow.Identity]) - weightChanges, err := participantsFromCurrentEpochSetup.Sample(2) + weightChanges, err := participantsFromCurrentEpochSetup.Sample(3) require.NoError(s.T(), err) ejectedChanges, err := participantsFromCurrentEpochSetup.Sample(2) require.NoError(s.T(), err) + require.Greater(s.T(), len(weightChanges), len(ejectedChanges), + "due to sampling and test setup we want to have more weight changes than ejected changes") for i, identity := range weightChanges { identity.DynamicIdentity.Weight = uint64(100 * (i + 1)) } @@ -403,7 +415,12 @@ func (s *UpdaterSuite) TestEpochSetupAfterIdentityChange() { setup := unittest.EpochSetupFixture(func(setup *flow.EpochSetup) { setup.Counter = s.parentProtocolState.CurrentEpochSetup.Counter + 1 - setup.Participants = append(setup.Participants, allUpdates.ToSkeleton()...) // add those nodes that were changed in previous epoch + // add those nodes that were changed in the previous epoch, but not those that were ejected + // it's important to exclude ejected nodes, since we expect that service smart contract has emitted ejection operation + // and service events are delivered (asynchronously) in an *order-preserving* manner meaning if ejection has happened before + // epoch setup then there is no possible way that it will include ejected node unless there is a severe bug in the service contract. + setup.Participants = append(setup.Participants, weightChanges.ToSkeleton()...).Filter( + filter.Not(filter.In(ejectedChanges.ToSkeleton()))).Sort(order.Canonical[flow.IdentitySkeleton]) }) err = s.updater.ProcessEpochSetup(setup) @@ -417,24 +434,28 @@ func (s *UpdaterSuite) TestEpochSetupAfterIdentityChange() { for _, updated := range ejectedChanges { currentEpochIdentity := currentEpochLookup[updated.NodeID] - nextEpochIdentity := nextEpochLookup[updated.NodeID] require.Equal(s.T(), updated.NodeID, currentEpochIdentity.NodeID) - require.Equal(s.T(), updated.NodeID, nextEpochIdentity.NodeID) - require.Equal(s.T(), updated.Ejected, currentEpochIdentity.Dynamic.Ejected) - require.Equal(s.T(), updated.Ejected, nextEpochIdentity.Dynamic.Ejected) + + _, foundInNextEpoch := nextEpochLookup[updated.NodeID] + require.False(s.T(), foundInNextEpoch) } for _, updated := range weightChanges { currentEpochIdentity := currentEpochLookup[updated.NodeID] - nextEpochIdentity := nextEpochLookup[updated.NodeID] require.Equal(s.T(), updated.NodeID, currentEpochIdentity.NodeID) - require.Equal(s.T(), updated.NodeID, nextEpochIdentity.NodeID) - require.Equal(s.T(), updated.DynamicIdentity.Weight, currentEpochIdentity.Dynamic.Weight) require.NotEqual(s.T(), updated.InitialWeight, currentEpochIdentity.Dynamic.Weight, "since we have updated weight it should not be equal to initial weight") - require.Equal(s.T(), updated.InitialWeight, nextEpochIdentity.Dynamic.Weight, - "we take information about weight from next epoc setup event") + + // it's possible that we have sampled weight and ejected changes for the same node so we need to check if it was ejected + if nextEpochIdentity, found := nextEpochLookup[updated.NodeID]; found { + require.Equal(s.T(), updated.NodeID, nextEpochIdentity.NodeID) + require.Equal(s.T(), updated.InitialWeight, nextEpochIdentity.Dynamic.Weight, + "we take information about weight from next epoc setup event") + } else { + _, wasEjected := ejectedChanges.ByNodeID(updated.NodeID) + require.True(s.T(), wasEjected, "only if node is ejected it could be missing from next epoch lookup") + } } } diff --git a/utils/unittest/fixtures.go b/utils/unittest/fixtures.go index 38f7a14936e..517dd764f08 100644 --- a/utils/unittest/fixtures.go +++ b/utils/unittest/fixtures.go @@ -2596,17 +2596,12 @@ func RootProtocolStateFixture() *flow.RichProtocolStateEntry { } return &flow.RichProtocolStateEntry{ ProtocolStateEntry: &flow.ProtocolStateEntry{ - + PreviousEpoch: nil, CurrentEpoch: flow.EpochStateContainer{ SetupID: currentEpochSetup.ID(), CommitID: currentEpochCommit.ID(), ActiveIdentities: flow.DynamicIdentityEntryListFromIdentities(allIdentities), }, - PreviousEpoch: &flow.EpochStateContainer{ - SetupID: flow.ZeroID, - CommitID: flow.ZeroID, - ActiveIdentities: nil, - }, InvalidStateTransitionAttempted: false, NextEpoch: nil, }, From 95dbf1efb3d7563365811ff2ed40946dfeee7003 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 23 Oct 2023 12:25:32 +0300 Subject: [PATCH 08/15] Added test for BuildIdentityTable --- model/flow/protocol_state_test.go | 82 +++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/model/flow/protocol_state_test.go b/model/flow/protocol_state_test.go index b23b3a66bb5..1e4d6335df5 100644 --- a/model/flow/protocol_state_test.go +++ b/model/flow/protocol_state_test.go @@ -1,6 +1,8 @@ package flow_test import ( + "fmt" + "github.com/onflow/flow-go/model/flow/order" "testing" "github.com/stretchr/testify/assert" @@ -185,3 +187,83 @@ func TestProtocolStateEntry_Copy(t *testing.T) { }) assert.NotEqual(t, entry.CurrentEpoch.ActiveIdentities, cpy.CurrentEpoch.ActiveIdentities) } + +// TestBuildIdentityTable tests if BuildIdentityTable returns a correct identity, whenever we pass arguments with or without +// overlap. It also tests if the function returns an error when the arguments are not ordered in the same order. +func TestBuildIdentityTable(t *testing.T) { + t.Run("happy-path-no-identities-overlap", func(t *testing.T) { + targetEpochIdentities := unittest.IdentityListFixture(10).Sort(order.Canonical[flow.Identity]) + adjacentEpochIdentities := unittest.IdentityListFixture(10).Sort(order.Canonical[flow.Identity]) + + identityList, err := flow.BuildIdentityTable( + flow.DynamicIdentityEntryListFromIdentities(targetEpochIdentities), + targetEpochIdentities.ToSkeleton(), + adjacentEpochIdentities.ToSkeleton(), + flow.DynamicIdentityEntryListFromIdentities(adjacentEpochIdentities), + ) + assert.NoError(t, err) + + expectedIdentities := targetEpochIdentities.Union(adjacentEpochIdentities.Map(func(identity flow.Identity) flow.Identity { + identity.Weight = 0 + return identity + })) + assert.Equal(t, expectedIdentities, identityList) + }) + t.Run("happy-path-identities-overlap", func(t *testing.T) { + targetEpochIdentities := unittest.IdentityListFixture(10).Sort(order.Canonical[flow.Identity]) + adjacentEpochIdentities := unittest.IdentityListFixture(10) + sampledIdentities, err := targetEpochIdentities.Sample(2) + // change address so we can assert that we take identities from target epoch and not adjacent epoch + for i, identity := range sampledIdentities.Copy() { + identity.Address = fmt.Sprintf("%d", i) + adjacentEpochIdentities = append(adjacentEpochIdentities, identity) + } + assert.NoError(t, err) + + identityList, err := flow.BuildIdentityTable( + flow.DynamicIdentityEntryListFromIdentities(targetEpochIdentities), + targetEpochIdentities.ToSkeleton(), + adjacentEpochIdentities.ToSkeleton(), + flow.DynamicIdentityEntryListFromIdentities(adjacentEpochIdentities), + ) + assert.NoError(t, err) + + expectedIdentities := targetEpochIdentities.Union(adjacentEpochIdentities.Map(func(identity flow.Identity) flow.Identity { + identity.Weight = 0 + return identity + })) + assert.Equal(t, expectedIdentities, identityList) + }) + t.Run("target-epoch-identities-not-ordered", func(t *testing.T) { + targetEpochIdentities := unittest.IdentityListFixture(10).Sort(order.Canonical[flow.Identity]) + targetEpochIdentitySkeletons, err := targetEpochIdentities.ToSkeleton().Shuffle() + assert.NoError(t, err) + targetEpochDynamicIdentities := flow.DynamicIdentityEntryListFromIdentities(targetEpochIdentities) + + adjacentEpochIdentities := unittest.IdentityListFixture(10).Sort(order.Canonical[flow.Identity]) + identityList, err := flow.BuildIdentityTable( + targetEpochDynamicIdentities, + targetEpochIdentitySkeletons, + adjacentEpochIdentities.ToSkeleton(), + flow.DynamicIdentityEntryListFromIdentities(adjacentEpochIdentities), + ) + assert.Error(t, err) + assert.Empty(t, identityList) + }) + t.Run("adjacent-epoch-identities-not-ordered", func(t *testing.T) { + adjacentEpochIdentities := unittest.IdentityListFixture(10).Sort(order.Canonical[flow.Identity]) + adjacentEpochIdentitySkeletons, err := adjacentEpochIdentities.ToSkeleton().Shuffle() + assert.NoError(t, err) + adjacentEpochDynamicIdentities := flow.DynamicIdentityEntryListFromIdentities(adjacentEpochIdentities) + + targetEpochIdentities := unittest.IdentityListFixture(10).Sort(order.Canonical[flow.Identity]) + identityList, err := flow.BuildIdentityTable( + flow.DynamicIdentityEntryListFromIdentities(targetEpochIdentities), + targetEpochIdentities.ToSkeleton(), + adjacentEpochIdentitySkeletons, + adjacentEpochDynamicIdentities, + ) + assert.Error(t, err) + assert.Empty(t, identityList) + }) +} From df2bfab007cb168d9a7908536711d5cfe675ea57 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 23 Oct 2023 13:10:49 +0300 Subject: [PATCH 09/15] Changed order of arguments in BuildIdentityTable --- model/flow/protocol_state.go | 17 ++++++----------- model/flow/protocol_state_test.go | 20 ++++++++++---------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/model/flow/protocol_state.go b/model/flow/protocol_state.go index 48f255d3435..8c25551f288 100644 --- a/model/flow/protocol_state.go +++ b/model/flow/protocol_state.go @@ -179,8 +179,8 @@ func NewRichProtocolStateEntry( previousEpochDynamicIdentities = protocolState.PreviousEpoch.ActiveIdentities } result.CurrentEpochIdentityTable, err = BuildIdentityTable( - protocolState.CurrentEpoch.ActiveIdentities, currentEpochSetup.Participants, + protocolState.CurrentEpoch.ActiveIdentities, previousEpochIdentitySkeletons, previousEpochDynamicIdentities, ) @@ -198,9 +198,9 @@ func NewRichProtocolStateEntry( } } - result.CurrentEpochIdentityTable, err = BuildIdentityTable( // build full identity table for current epoch according to (2a) - protocolState.CurrentEpoch.ActiveIdentities, + result.CurrentEpochIdentityTable, err = BuildIdentityTable( currentEpochSetup.Participants, + protocolState.CurrentEpoch.ActiveIdentities, nextEpochSetup.Participants, nextEpoch.ActiveIdentities, ) @@ -208,9 +208,9 @@ func NewRichProtocolStateEntry( return nil, fmt.Errorf("could not build identity table for setup/commit phase: %w", err) } - result.NextEpochIdentityTable, err = BuildIdentityTable( // build full identity table for next epoch according to (2b) - nextEpoch.ActiveIdentities, + result.NextEpochIdentityTable, err = BuildIdentityTable( nextEpochSetup.Participants, + nextEpoch.ActiveIdentities, currentEpochSetup.Participants, protocolState.CurrentEpoch.ActiveIdentities, ) @@ -359,12 +359,7 @@ func (ll DynamicIdentityEntryList) Sort(less IdentifierOrder) DynamicIdentityEnt // // It also performs sanity checks to make sure that the data is consistent. // No errors are expected during normal operation. All errors indicate inconsistent or invalid inputs. -func BuildIdentityTable( - targetEpochDynamicIdentities DynamicIdentityEntryList, - targetEpochIdentitySkeletons IdentitySkeletonList, - adjacentEpochIdentitySkeletons IdentitySkeletonList, - adjacentEpochDynamicIdentities DynamicIdentityEntryList, -) (IdentityList, error) { +func BuildIdentityTable(targetEpochIdentitySkeletons IdentitySkeletonList, targetEpochDynamicIdentities DynamicIdentityEntryList, adjacentEpochIdentitySkeletons IdentitySkeletonList, adjacentEpochDynamicIdentities DynamicIdentityEntryList) (IdentityList, error) { targetEpochParticipants, err := ComposeFullIdentities(targetEpochIdentitySkeletons, targetEpochDynamicIdentities) if err != nil { return nil, fmt.Errorf("could not reconstruct participants for target epoch: %w", err) diff --git a/model/flow/protocol_state_test.go b/model/flow/protocol_state_test.go index 1e4d6335df5..72a78881f11 100644 --- a/model/flow/protocol_state_test.go +++ b/model/flow/protocol_state_test.go @@ -49,7 +49,7 @@ func TestNewRichProtocolStateEntry(t *testing.T) { nil, ) assert.NoError(t, err) - expectedIdentities, err := flow.BuildIdentityTable(identities, setup.Participants, nil, nil) + expectedIdentities, err := flow.BuildIdentityTable(setup.Participants, identities, nil, nil) assert.NoError(t, err) assert.Equal(t, expectedIdentities, entry.CurrentEpochIdentityTable, "should be equal to current epoch setup participants") }) @@ -71,8 +71,8 @@ func TestNewRichProtocolStateEntry(t *testing.T) { ) assert.NoError(t, err) expectedIdentities, err := flow.BuildIdentityTable( - stateEntry.CurrentEpoch.ActiveIdentities, stateEntry.CurrentEpochSetup.Participants, + stateEntry.CurrentEpoch.ActiveIdentities, stateEntry.PreviousEpochSetup.Participants, stateEntry.PreviousEpoch.ActiveIdentities, ) @@ -102,8 +102,8 @@ func TestNewRichProtocolStateEntry(t *testing.T) { ) assert.NoError(t, err) expectedIdentities, err := flow.BuildIdentityTable( - stateEntry.CurrentEpoch.ActiveIdentities, stateEntry.CurrentEpochSetup.Participants, + stateEntry.CurrentEpoch.ActiveIdentities, stateEntry.NextEpochSetup.Participants, stateEntry.NextEpoch.ActiveIdentities, ) @@ -111,8 +111,8 @@ func TestNewRichProtocolStateEntry(t *testing.T) { assert.Equal(t, expectedIdentities, richEntry.CurrentEpochIdentityTable, "should be equal to current epoch setup participants + next epoch setup participants") assert.Nil(t, richEntry.NextEpochCommit) expectedIdentities, err = flow.BuildIdentityTable( - stateEntry.NextEpoch.ActiveIdentities, stateEntry.NextEpochSetup.Participants, + stateEntry.NextEpoch.ActiveIdentities, stateEntry.CurrentEpochSetup.Participants, stateEntry.CurrentEpoch.ActiveIdentities, ) @@ -140,16 +140,16 @@ func TestNewRichProtocolStateEntry(t *testing.T) { ) assert.NoError(t, err) expectedIdentities, err := flow.BuildIdentityTable( - stateEntry.CurrentEpoch.ActiveIdentities, stateEntry.CurrentEpochSetup.Participants, + stateEntry.CurrentEpoch.ActiveIdentities, stateEntry.NextEpochSetup.Participants, stateEntry.NextEpoch.ActiveIdentities, ) assert.NoError(t, err) assert.Equal(t, expectedIdentities, richEntry.CurrentEpochIdentityTable, "should be equal to current epoch setup participants + next epoch setup participants") expectedIdentities, err = flow.BuildIdentityTable( - stateEntry.NextEpoch.ActiveIdentities, stateEntry.NextEpochSetup.Participants, + stateEntry.NextEpoch.ActiveIdentities, stateEntry.CurrentEpochSetup.Participants, stateEntry.CurrentEpoch.ActiveIdentities, ) @@ -196,8 +196,8 @@ func TestBuildIdentityTable(t *testing.T) { adjacentEpochIdentities := unittest.IdentityListFixture(10).Sort(order.Canonical[flow.Identity]) identityList, err := flow.BuildIdentityTable( - flow.DynamicIdentityEntryListFromIdentities(targetEpochIdentities), targetEpochIdentities.ToSkeleton(), + flow.DynamicIdentityEntryListFromIdentities(targetEpochIdentities), adjacentEpochIdentities.ToSkeleton(), flow.DynamicIdentityEntryListFromIdentities(adjacentEpochIdentities), ) @@ -221,8 +221,8 @@ func TestBuildIdentityTable(t *testing.T) { assert.NoError(t, err) identityList, err := flow.BuildIdentityTable( - flow.DynamicIdentityEntryListFromIdentities(targetEpochIdentities), targetEpochIdentities.ToSkeleton(), + flow.DynamicIdentityEntryListFromIdentities(targetEpochIdentities), adjacentEpochIdentities.ToSkeleton(), flow.DynamicIdentityEntryListFromIdentities(adjacentEpochIdentities), ) @@ -242,8 +242,8 @@ func TestBuildIdentityTable(t *testing.T) { adjacentEpochIdentities := unittest.IdentityListFixture(10).Sort(order.Canonical[flow.Identity]) identityList, err := flow.BuildIdentityTable( - targetEpochDynamicIdentities, targetEpochIdentitySkeletons, + targetEpochDynamicIdentities, adjacentEpochIdentities.ToSkeleton(), flow.DynamicIdentityEntryListFromIdentities(adjacentEpochIdentities), ) @@ -258,8 +258,8 @@ func TestBuildIdentityTable(t *testing.T) { targetEpochIdentities := unittest.IdentityListFixture(10).Sort(order.Canonical[flow.Identity]) identityList, err := flow.BuildIdentityTable( - flow.DynamicIdentityEntryListFromIdentities(targetEpochIdentities), targetEpochIdentities.ToSkeleton(), + flow.DynamicIdentityEntryListFromIdentities(targetEpochIdentities), adjacentEpochIdentitySkeletons, adjacentEpochDynamicIdentities, ) From 25624f230ea320172691020a563672a3665e6e33 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 23 Oct 2023 13:11:42 +0300 Subject: [PATCH 10/15] Linted --- model/flow/protocol_state_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/flow/protocol_state_test.go b/model/flow/protocol_state_test.go index 72a78881f11..91a2b1da21b 100644 --- a/model/flow/protocol_state_test.go +++ b/model/flow/protocol_state_test.go @@ -2,12 +2,12 @@ package flow_test import ( "fmt" - "github.com/onflow/flow-go/model/flow/order" "testing" "github.com/stretchr/testify/assert" "github.com/onflow/flow-go/model/flow" + "github.com/onflow/flow-go/model/flow/order" "github.com/onflow/flow-go/utils/unittest" ) From f92636da460585ad8ab98ba42a2e97e8e683a662 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 23 Oct 2023 13:13:25 +0300 Subject: [PATCH 11/15] Apply suggestions from code review Co-authored-by: Jordan Schalm --- state/protocol/protocol_state/updater.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/state/protocol/protocol_state/updater.go b/state/protocol/protocol_state/updater.go index d69a4edda1b..5cfc673f59f 100644 --- a/state/protocol/protocol_state/updater.go +++ b/state/protocol/protocol_state/updater.go @@ -23,7 +23,7 @@ type Updater struct { candidate *flow.Header // The following fields are maps from NodeID → DynamicIdentityEntry for the nodes that are *active* in the respective epoch. - // Active means that these nodes are authorized to contribute to extending the chain. Formally, as node is active if and only + // Active means that these nodes are authorized to contribute to extending the chain. Formally, a node is active if and only // if it is listed in the EpochSetup event for the respective epoch. Note that map values are pointers, so writes to map values // will modify the respective DynamicIdentityEntry in EpochStateContainer. @@ -80,7 +80,7 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { // While ejection status and dynamic weight are not part of the EpochSetup event, we can supplement this information as follows: // - Per convention, service events are delivered (asynchronously) in an *order-preserving* manner. Furthermore, weight changes or // node ejection is entirely mediated by system smart contracts and delivered via service events. - // - Therefore, the EpochSetup event contains the up-to-date snapshot of the cluster members. Any weight changes or node ejection + // - Therefore, the EpochSetup event contains the up-to-date snapshot of the epoch participants. Any weight changes or node ejection // that happened before should be reflected in the EpochSetup event. Specifically, the initial weight should be reduced and ejected // nodes should be no longer listed in the EpochSetup event. // - Hence, the following invariant must be satisfied by the system smart contracts for all active nodes in the upcoming epoch: @@ -94,7 +94,7 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { // For collector clusters, we rely on invariants (I) and (II) holding. See `committees.Cluster` for details, specifically function // `constructInitialClusterIdentities(..)`. While the system smart contract must satisfy this invariant, we run a sanity check below. - // TODO: In case the invariant is violated (likely bug in system smart contracts), we should go into EECC and not reject the block containing the service event. + // TODO: In case the invariant is violated (likely bug in system smart contracts), we should go into epoch fallback mode and not reject the block containing the service event. // activeIdentitiesLookup := u.parentState.CurrentEpoch.ActiveIdentities.Lookup() // lookup NodeID → DynamicIdentityEntry for nodes _active_ in the current epoch nextEpochActiveIdentities := make(flow.DynamicIdentityEntryList, 0, len(epochSetup.Participants)) From 0172dd130c724b60f94787dd2c1f54b9cd7cc0e0 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 23 Oct 2023 14:27:39 +0300 Subject: [PATCH 12/15] Changed expected error types of ProcessEpochSetup and ProcessEpochCommit. Updated tests, error handling, docs --- state/protocol/badger/mutator.go | 10 ++++++ state/protocol/badger/mutator_test.go | 22 ++++++++++++ state/protocol/protocol_state.go | 6 ++-- state/protocol/protocol_state/updater.go | 26 +++++++------- state/protocol/protocol_state/updater_test.go | 35 +++++++++++++++++++ 5 files changed, 84 insertions(+), 15 deletions(-) diff --git a/state/protocol/badger/mutator.go b/state/protocol/badger/mutator.go index 2f24168fea6..46f90d0bf2f 100644 --- a/state/protocol/badger/mutator.go +++ b/state/protocol/badger/mutator.go @@ -1121,6 +1121,11 @@ func (m *FollowerState) handleEpochServiceEvents(candidate *flow.Block, updater err = updater.ProcessEpochSetup(ev) if err != nil { + if protocol.IsInvalidServiceEventError(err) { + // we have observed an invalid service event, which triggers epoch fallback mode + updater.SetInvalidStateTransitionAttempted() + return dbUpdates, nil + } return nil, irrecoverable.NewExceptionf("could not process epoch setup event: %w", err) } @@ -1149,6 +1154,11 @@ func (m *FollowerState) handleEpochServiceEvents(candidate *flow.Block, updater err = updater.ProcessEpochCommit(ev) if err != nil { + if protocol.IsInvalidServiceEventError(err) { + // we have observed an invalid service event, which triggers epoch fallback mode + updater.SetInvalidStateTransitionAttempted() + return dbUpdates, nil + } return nil, irrecoverable.NewExceptionf("could not process epoch commit event: %w", err) } diff --git a/state/protocol/badger/mutator_test.go b/state/protocol/badger/mutator_test.go index 71b2fcbdde6..17209205c7d 100644 --- a/state/protocol/badger/mutator_test.go +++ b/state/protocol/badger/mutator_test.go @@ -1404,6 +1404,28 @@ func TestExtendEpochSetupInvalid(t *testing.T) { assertEpochEmergencyFallbackTriggered(t, state, true) }) }) + + t.Run("participants not ordered (EECC)", func(t *testing.T) { + util.RunWithFullProtocolState(t, rootSnapshot, func(db *badger.DB, state *protocol.ParticipantState) { + block1, createSetup := setupState(t, db, state) + + _, receipt, seal := createSetup(func(setup *flow.EpochSetup) { + var err error + setup.Participants, err = setup.Participants.Shuffle() + require.NoError(t, err) + }) + + receiptBlock, sealingBlock := unittest.SealBlock(t, state, block1, receipt, seal) + err := state.Finalize(context.Background(), receiptBlock.ID()) + require.NoError(t, err) + // epoch fallback not triggered before finalization + assertEpochEmergencyFallbackTriggered(t, state, false) + err = state.Finalize(context.Background(), sealingBlock.ID()) + require.NoError(t, err) + // epoch fallback triggered after finalization + assertEpochEmergencyFallbackTriggered(t, state, true) + }) + }) } // TestExtendEpochCommitInvalid tests that incorporating an invalid EpochCommit diff --git a/state/protocol/protocol_state.go b/state/protocol/protocol_state.go index 2a6a7ad0530..d1cdb9a19a2 100644 --- a/state/protocol/protocol_state.go +++ b/state/protocol/protocol_state.go @@ -79,14 +79,16 @@ type StateUpdater interface { // identities from previous+current epochs and start returning identities from current+next epochs. // As a result of this operation protocol state for the next epoch will be created. // CAUTION: Caller must validate input event. - // No errors are expected during normal operations. + // Expected errors during normal operations: + // - `protocol.InvalidServiceEventError` - an invalid service event with respect to the protocol state has been supplied. ProcessEpochSetup(epochSetup *flow.EpochSetup) error // ProcessEpochCommit updates current protocol state with data from epoch commit event. // Observing an epoch setup commit, transitions protocol state from setup to commit phase, at this point we have // finished construction of the next epoch. // As a result of this operation protocol state for next epoch will be committed. // CAUTION: Caller must validate input event. - // No errors are expected during normal operations. + // Expected errors during normal operations: + // - `protocol.InvalidServiceEventError` - an invalid service event with respect to the protocol state has been supplied. ProcessEpochCommit(epochCommit *flow.EpochCommit) error // UpdateIdentity updates identity table with new identity entry. // Should pass identity which is already present in the table, otherwise an exception will be raised. diff --git a/state/protocol/protocol_state/updater.go b/state/protocol/protocol_state/updater.go index d69a4edda1b..6329919ee5d 100644 --- a/state/protocol/protocol_state/updater.go +++ b/state/protocol/protocol_state/updater.go @@ -58,16 +58,17 @@ func (u *Updater) Build() (updatedState *flow.ProtocolStateEntry, stateID flow.I // - we stop returning identities from previous+current epochs and instead returning identities from current+next epochs. // // As a result of this operation protocol state for the next epoch will be created. -// No errors are expected during normal operations. +// Expected errors during normal operations: +// - `protocol.InvalidServiceEventError` - an invalid service event with respect to the protocol state has been supplied. func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { if epochSetup.Counter != u.parentState.CurrentEpochSetup.Counter+1 { - return fmt.Errorf("invalid epoch setup counter, expecting %d got %d", u.parentState.CurrentEpochSetup.Counter+1, epochSetup.Counter) + return protocol.NewInvalidServiceEventErrorf("invalid epoch setup counter, expecting %d got %d", u.parentState.CurrentEpochSetup.Counter+1, epochSetup.Counter) } if u.state.NextEpoch != nil { - return fmt.Errorf("protocol state has already a setup event") + return protocol.NewInvalidServiceEventErrorf("protocol state has already a setup event") } if u.state.InvalidStateTransitionAttempted { - return nil // won't process new events if we are in EECC + return nil // won't process new events if we are in epoch fallback mode. } // When observing setup event for subsequent epoch, construct the EpochStateContainer for `ProtocolStateEntry.NextEpoch`. @@ -94,8 +95,6 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { // For collector clusters, we rely on invariants (I) and (II) holding. See `committees.Cluster` for details, specifically function // `constructInitialClusterIdentities(..)`. While the system smart contract must satisfy this invariant, we run a sanity check below. - // TODO: In case the invariant is violated (likely bug in system smart contracts), we should go into EECC and not reject the block containing the service event. - // activeIdentitiesLookup := u.parentState.CurrentEpoch.ActiveIdentities.Lookup() // lookup NodeID → DynamicIdentityEntry for nodes _active_ in the current epoch nextEpochActiveIdentities := make(flow.DynamicIdentityEntryList, 0, len(epochSetup.Participants)) prevNodeID := epochSetup.Participants[0].NodeID @@ -103,12 +102,12 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { // sanity checking invariant (I): currentEpochDynamicProperties, found := activeIdentitiesLookup[nextEpochIdentitySkeleton.NodeID] if found && currentEpochDynamicProperties.Dynamic.Ejected { // invariance violated - return fmt.Errorf("node %v is ejected in current epoch %d but readmitted by EpochSetup event for epoch %d", nextEpochIdentitySkeleton.NodeID, u.parentState.CurrentEpochSetup.Counter, epochSetup.Counter) + return protocol.NewInvalidServiceEventErrorf("node %v is ejected in current epoch %d but readmitted by EpochSetup event for epoch %d", nextEpochIdentitySkeleton.NodeID, u.parentState.CurrentEpochSetup.Counter, epochSetup.Counter) } // sanity checking invariant (II): if idx > 0 && !order.IdentifierCanonical(prevNodeID, nextEpochIdentitySkeleton.NodeID) { - return fmt.Errorf("epoch setup event lists active participants not in canonical ordering") + return protocol.NewInvalidServiceEventErrorf("epoch setup event lists active participants not in canonical ordering") } prevNodeID = nextEpochIdentitySkeleton.NodeID @@ -138,19 +137,20 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { // Observing an epoch setup commit, transitions protocol state from setup to commit phase, at this point we have // finished construction of the next epoch. // As a result of this operation protocol state for next epoch will be committed. -// No errors are expected during normal operations. +// Expected errors during normal operations: +// - `protocol.InvalidServiceEventError` - an invalid service event with respect to the protocol state has been supplied. func (u *Updater) ProcessEpochCommit(epochCommit *flow.EpochCommit) error { if epochCommit.Counter != u.parentState.CurrentEpochSetup.Counter+1 { - return fmt.Errorf("invalid epoch commit counter, expecting %d got %d", u.parentState.CurrentEpochSetup.Counter+1, epochCommit.Counter) + return protocol.NewInvalidServiceEventErrorf("invalid epoch commit counter, expecting %d got %d", u.parentState.CurrentEpochSetup.Counter+1, epochCommit.Counter) } if u.state.NextEpoch == nil { - return fmt.Errorf("protocol state has been setup yet") + return protocol.NewInvalidServiceEventErrorf("protocol state has been setup yet") } if u.state.NextEpoch.CommitID != flow.ZeroID { - return fmt.Errorf("protocol state has already a commit event") + return protocol.NewInvalidServiceEventErrorf("protocol state has already a commit event") } if u.state.InvalidStateTransitionAttempted { - return nil // won't process new events if we are going to enter EECC + return nil // won't process new events if we are going to enter epoch fallback mode } u.state.NextEpoch.CommitID = epochCommit.ID() diff --git a/state/protocol/protocol_state/updater_test.go b/state/protocol/protocol_state/updater_test.go index 97c0de05c05..5ea301d66d0 100644 --- a/state/protocol/protocol_state/updater_test.go +++ b/state/protocol/protocol_state/updater_test.go @@ -9,6 +9,7 @@ import ( "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/model/flow/filter" "github.com/onflow/flow-go/model/flow/order" + "github.com/onflow/flow-go/state/protocol" "github.com/onflow/flow-go/utils/unittest" ) @@ -140,6 +141,7 @@ func (s *UpdaterSuite) TestProcessEpochCommit() { }) err := s.updater.ProcessEpochCommit(commit) require.Error(s.T(), err) + require.True(s.T(), protocol.IsInvalidServiceEventError(err)) }) s.Run("no next epoch protocol state", func() { commit := unittest.EpochCommitFixture(func(commit *flow.EpochCommit) { @@ -147,6 +149,7 @@ func (s *UpdaterSuite) TestProcessEpochCommit() { }) err := s.updater.ProcessEpochCommit(commit) require.Error(s.T(), err) + require.True(s.T(), protocol.IsInvalidServiceEventError(err)) }) s.Run("invalid state transition attempted", func() { updater := NewUpdater(s.candidate, s.parentProtocolState) @@ -189,6 +192,7 @@ func (s *UpdaterSuite) TestProcessEpochCommit() { // processing another epoch commit has to be an error since we have already processed one err = updater.ProcessEpochCommit(commit) require.Error(s.T(), err) + require.True(s.T(), protocol.IsInvalidServiceEventError(err)) newState, _, _ := updater.Build() require.Equal(s.T(), commit.ID(), newState.NextEpoch.CommitID, "next epoch must be committed") @@ -267,6 +271,7 @@ func (s *UpdaterSuite) TestProcessEpochSetupInvariants() { }) err := s.updater.ProcessEpochSetup(setup) require.Error(s.T(), err) + require.True(s.T(), protocol.IsInvalidServiceEventError(err)) }) s.Run("invalid state transition attempted", func() { updater := NewUpdater(s.candidate, s.parentProtocolState) @@ -290,6 +295,36 @@ func (s *UpdaterSuite) TestProcessEpochSetupInvariants() { err = updater.ProcessEpochSetup(setup) require.Error(s.T(), err) + require.True(s.T(), protocol.IsInvalidServiceEventError(err)) + }) + s.Run("participants not sorted", func() { + updater := NewUpdater(s.candidate, s.parentProtocolState) + setup := unittest.EpochSetupFixture(func(setup *flow.EpochSetup) { + setup.Counter = s.parentProtocolState.CurrentEpochSetup.Counter + 1 + var err error + setup.Participants, err = setup.Participants.Shuffle() + require.NoError(s.T(), err) + }) + err := updater.ProcessEpochSetup(setup) + require.Error(s.T(), err) + require.True(s.T(), protocol.IsInvalidServiceEventError(err)) + }) + s.Run("epoch setup state conflicts with protocol state", func() { + conflictingIdentity := s.parentProtocolState.ProtocolStateEntry.CurrentEpoch.ActiveIdentities[0] + conflictingIdentity.Dynamic.Ejected = true + + updater := NewUpdater(s.candidate, s.parentProtocolState) + setup := unittest.EpochSetupFixture(func(setup *flow.EpochSetup) { + setup.Counter = s.parentProtocolState.CurrentEpochSetup.Counter + 1 + // using same identities as in previous epoch should result in an error since + // we have ejected conflicting identity but it was added back in epoch setup + // such epoch setup event is invalid. + setup.Participants = s.parentProtocolState.CurrentEpochSetup.Participants + }) + + err := updater.ProcessEpochSetup(setup) + require.Error(s.T(), err) + require.True(s.T(), protocol.IsInvalidServiceEventError(err)) }) } From f6d519bd62f081a1d4d21a5ece7bb14fed59ff11 Mon Sep 17 00:00:00 2001 From: Alexander Hentschel Date: Mon, 23 Oct 2023 11:29:16 -0700 Subject: [PATCH 13/15] updated goDoc of method `BuildIdentityTable` to completely describe all inputs --- model/flow/protocol_state.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/model/flow/protocol_state.go b/model/flow/protocol_state.go index 8c25551f288..29eab3700a4 100644 --- a/model/flow/protocol_state.go +++ b/model/flow/protocol_state.go @@ -348,16 +348,19 @@ func (ll DynamicIdentityEntryList) Sort(less IdentifierOrder) DynamicIdentityEnt } // BuildIdentityTable constructs the full identity table for the target epoch by combining data from: -// 1. The Dynamic Identities for the nodes that are _active_ in the target epoch (i.e. the dynamic identity -// fields for the IdentitySkeletons contained in the EpochSetup event for the respective epoch). -// 2. The IdentitySkeletons for the nodes that are _active_ in the target epoch +// 1. The IdentitySkeletons for the nodes that are _active_ in the target epoch // (recorded in EpochSetup event and immutable throughout the epoch). -// 3. [optional] An adjacent epoch's IdentitySkeletons (can be empty or nil), as recorded in the -// adjacent epoch's setup event. For a target epoch N, the epochs N-1 and N+1 are defined to be -// adjacent. Adjacent epochs do not _necessarily_ exist (e.g. consider a spork comprising only -// a single epoch), in which case this input is nil or empty. +// 2. The Dynamic Identities for the nodes that are _active_ in the target epoch (i.e. the dynamic identity +// fields for the IdentitySkeletons contained in the EpochSetup event for the respective epoch). +// +// Optionally, identity information for an adjacent epoch is given if and only if an adjacent epoch exists. For +// a target epoch N, the epochs N-1 and N+1 are defined to be adjacent. Adjacent epochs do not necessarily exist +// (e.g. consider a spork comprising only a single epoch), in which case the respective inputs are nil or empty. +// 3. [optional] An adjacent epoch's IdentitySkeletons as recorded in the adjacent epoch's setup event. +// 4. [optional] An adjacent epoch's Dynamic Identities. // -// It also performs sanity checks to make sure that the data is consistent. +// The function enforces that the input slices pertaining to the same epoch contain the same identities +// (compared by nodeID) in the same order. Otherwise, an exception is returned. // No errors are expected during normal operation. All errors indicate inconsistent or invalid inputs. func BuildIdentityTable(targetEpochIdentitySkeletons IdentitySkeletonList, targetEpochDynamicIdentities DynamicIdentityEntryList, adjacentEpochIdentitySkeletons IdentitySkeletonList, adjacentEpochDynamicIdentities DynamicIdentityEntryList) (IdentityList, error) { targetEpochParticipants, err := ComposeFullIdentities(targetEpochIdentitySkeletons, targetEpochDynamicIdentities) @@ -398,7 +401,7 @@ func DynamicIdentityEntryListFromIdentities(identities IdentityList) DynamicIden // 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. +// in the same order. Otherwise, an exception is returned. // No errors are expected during normal operations. func ComposeFullIdentities(skeletons IdentitySkeletonList, dynamics DynamicIdentityEntryList) (IdentityList, error) { // sanity check: list of skeletons and dynamic should be the same From 49934ce513f6f849a2c5ecb44a675ff45a610c58 Mon Sep 17 00:00:00 2001 From: Alexander Hentschel Date: Mon, 23 Oct 2023 13:41:08 -0700 Subject: [PATCH 14/15] updated code-internal documentation to precisely explain the identity ordering assumptions in the system smart contracts vs the core protocol layer. --- model/flow/filter/identity.go | 2 -- state/protocol/protocol_state/updater.go | 10 +++++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/model/flow/filter/identity.go b/model/flow/filter/identity.go index 8721f773767..6bf5bfad9b3 100644 --- a/model/flow/filter/identity.go +++ b/model/flow/filter/identity.go @@ -1,5 +1,3 @@ -// (c) 2019 Dapper Labs - ALL RIGHTS RESERVED - package filter import ( diff --git a/state/protocol/protocol_state/updater.go b/state/protocol/protocol_state/updater.go index ca3fd78d4df..e5b8bf918f8 100644 --- a/state/protocol/protocol_state/updater.go +++ b/state/protocol/protocol_state/updater.go @@ -59,7 +59,7 @@ func (u *Updater) Build() (updatedState *flow.ProtocolStateEntry, stateID flow.I // // As a result of this operation protocol state for the next epoch will be created. // Expected errors during normal operations: -// - `protocol.InvalidServiceEventError` - an invalid service event with respect to the protocol state has been supplied. +// - `protocol.InvalidServiceEventError` if the service event is not a valid state transition for the current protocol state func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { if epochSetup.Counter != u.parentState.CurrentEpochSetup.Counter+1 { return protocol.NewInvalidServiceEventErrorf("invalid epoch setup counter, expecting %d got %d", u.parentState.CurrentEpochSetup.Counter+1, epochSetup.Counter) @@ -90,8 +90,12 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { // smart contracts earlier) but also being listed in the setup event for the subsequent epoch (service event emitted by // the system smart contracts later) is illegal. // sanity checking SAFETY-CRITICAL INVARIANT (II): - // - Per convention, the system smart contracts should list the IdentitySkeletons in canonical order. This is useful for - // most efficient construction of the full active Identities for an epoch. + // - Per convention, the `flow.EpochSetup` event should list the IdentitySkeletons in canonical order. This is useful + // for most efficient construction of the full active Identities for an epoch. We enforce this here at the gateway + // to the protocol state, when we incorporate new information from the EpochSetup event. + // - Note that the system smart contracts manage the identity table as an unordered set! For the protocol state, we desire a fixed + // ordering to simplify various implementation details, like the DKG. Therefore, we order identities in `flow.EpochSetup` during + // conversion from cadence to Go in the function `convert.ServiceEvent(flow.ChainID, flow.Event)` in package `model/convert` // For collector clusters, we rely on invariants (I) and (II) holding. See `committees.Cluster` for details, specifically function // `constructInitialClusterIdentities(..)`. While the system smart contract must satisfy this invariant, we run a sanity check below. From c63887451f37d346bf3f633d36a490a0e3f2a0bd Mon Sep 17 00:00:00 2001 From: Alexander Hentschel Date: Mon, 23 Oct 2023 16:49:07 -0700 Subject: [PATCH 15/15] minor re-ordering of the logic and in-code documentation of function `Updater.ProcessEpochSetup()` such that the documentation and implementation are in the same order --- state/protocol/badger/mutator.go | 2 -- state/protocol/protocol_state.go | 4 +-- state/protocol/protocol_state/updater.go | 34 ++++++++++++------------ 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/state/protocol/badger/mutator.go b/state/protocol/badger/mutator.go index 46f90d0bf2f..e3982d55250 100644 --- a/state/protocol/badger/mutator.go +++ b/state/protocol/badger/mutator.go @@ -1,5 +1,3 @@ -// (c) 2019 Dapper Labs - ALL RIGHTS RESERVED - package badger import ( diff --git a/state/protocol/protocol_state.go b/state/protocol/protocol_state.go index d1cdb9a19a2..44c138b8a56 100644 --- a/state/protocol/protocol_state.go +++ b/state/protocol/protocol_state.go @@ -80,7 +80,7 @@ type StateUpdater interface { // As a result of this operation protocol state for the next epoch will be created. // CAUTION: Caller must validate input event. // Expected errors during normal operations: - // - `protocol.InvalidServiceEventError` - an invalid service event with respect to the protocol state has been supplied. + // - `protocol.InvalidServiceEventError` if the service event is not a valid state transition for the current protocol state ProcessEpochSetup(epochSetup *flow.EpochSetup) error // ProcessEpochCommit updates current protocol state with data from epoch commit event. // Observing an epoch setup commit, transitions protocol state from setup to commit phase, at this point we have @@ -88,7 +88,7 @@ type StateUpdater interface { // As a result of this operation protocol state for next epoch will be committed. // CAUTION: Caller must validate input event. // Expected errors during normal operations: - // - `protocol.InvalidServiceEventError` - an invalid service event with respect to the protocol state has been supplied. + // - `protocol.InvalidServiceEventError` if the service event is not a valid state transition for the current protocol state ProcessEpochCommit(epochCommit *flow.EpochCommit) error // UpdateIdentity updates identity table with new identity entry. // Should pass identity which is already present in the table, otherwise an exception will be raised. diff --git a/state/protocol/protocol_state/updater.go b/state/protocol/protocol_state/updater.go index e5b8bf918f8..36251464b44 100644 --- a/state/protocol/protocol_state/updater.go +++ b/state/protocol/protocol_state/updater.go @@ -65,7 +65,7 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { return protocol.NewInvalidServiceEventErrorf("invalid epoch setup counter, expecting %d got %d", u.parentState.CurrentEpochSetup.Counter+1, epochSetup.Counter) } if u.state.NextEpoch != nil { - return protocol.NewInvalidServiceEventErrorf("protocol state has already a setup event") + return protocol.NewInvalidServiceEventErrorf("repeated setup for epoch %d", epochSetup.Counter) } if u.state.InvalidStateTransitionAttempted { return nil // won't process new events if we are in epoch fallback mode. @@ -78,6 +78,13 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { // they are part of the EpochSetup event for the respective epoch. // // sanity checking SAFETY-CRITICAL INVARIANT (I): + // - Per convention, the `flow.EpochSetup` event should list the IdentitySkeletons in canonical order. This is useful + // for most efficient construction of the full active Identities for an epoch. We enforce this here at the gateway + // to the protocol state, when we incorporate new information from the EpochSetup event. + // - Note that the system smart contracts manage the identity table as an unordered set! For the protocol state, we desire a fixed + // ordering to simplify various implementation details, like the DKG. Therefore, we order identities in `flow.EpochSetup` during + // conversion from cadence to Go in the function `convert.ServiceEvent(flow.ChainID, flow.Event)` in package `model/convert` + // sanity checking SAFETY-CRITICAL INVARIANT (II): // While ejection status and dynamic weight are not part of the EpochSetup event, we can supplement this information as follows: // - Per convention, service events are delivered (asynchronously) in an *order-preserving* manner. Furthermore, weight changes or // node ejection is entirely mediated by system smart contracts and delivered via service events. @@ -85,17 +92,10 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { // that happened before should be reflected in the EpochSetup event. Specifically, the initial weight should be reduced and ejected // nodes should be no longer listed in the EpochSetup event. // - Hence, the following invariant must be satisfied by the system smart contracts for all active nodes in the upcoming epoch: - // (i) When the EpochSetup event is emitted / processed, the weight of all active nodes equals their InitialWeight and - // (ii) The Ejected flag is false. Node X being ejected in epoch N (necessarily via a service event emitted by the system + // (i) The Ejected flag is false. Node X being ejected in epoch N (necessarily via a service event emitted by the system // smart contracts earlier) but also being listed in the setup event for the subsequent epoch (service event emitted by // the system smart contracts later) is illegal. - // sanity checking SAFETY-CRITICAL INVARIANT (II): - // - Per convention, the `flow.EpochSetup` event should list the IdentitySkeletons in canonical order. This is useful - // for most efficient construction of the full active Identities for an epoch. We enforce this here at the gateway - // to the protocol state, when we incorporate new information from the EpochSetup event. - // - Note that the system smart contracts manage the identity table as an unordered set! For the protocol state, we desire a fixed - // ordering to simplify various implementation details, like the DKG. Therefore, we order identities in `flow.EpochSetup` during - // conversion from cadence to Go in the function `convert.ServiceEvent(flow.ChainID, flow.Event)` in package `model/convert` + // (ii) When the EpochSetup event is emitted / processed, the weight of all active nodes equals their InitialWeight and // For collector clusters, we rely on invariants (I) and (II) holding. See `committees.Cluster` for details, specifically function // `constructInitialClusterIdentities(..)`. While the system smart contract must satisfy this invariant, we run a sanity check below. @@ -104,21 +104,21 @@ func (u *Updater) ProcessEpochSetup(epochSetup *flow.EpochSetup) error { prevNodeID := epochSetup.Participants[0].NodeID for idx, nextEpochIdentitySkeleton := range epochSetup.Participants { // sanity checking invariant (I): - currentEpochDynamicProperties, found := activeIdentitiesLookup[nextEpochIdentitySkeleton.NodeID] - if found && currentEpochDynamicProperties.Dynamic.Ejected { // invariance violated - return protocol.NewInvalidServiceEventErrorf("node %v is ejected in current epoch %d but readmitted by EpochSetup event for epoch %d", nextEpochIdentitySkeleton.NodeID, u.parentState.CurrentEpochSetup.Counter, epochSetup.Counter) - } - - // sanity checking invariant (II): if idx > 0 && !order.IdentifierCanonical(prevNodeID, nextEpochIdentitySkeleton.NodeID) { return protocol.NewInvalidServiceEventErrorf("epoch setup event lists active participants not in canonical ordering") } prevNodeID = nextEpochIdentitySkeleton.NodeID + // sanity checking invariant (II.i): + currentEpochDynamicProperties, found := activeIdentitiesLookup[nextEpochIdentitySkeleton.NodeID] + if found && currentEpochDynamicProperties.Dynamic.Ejected { // invariance violated + return protocol.NewInvalidServiceEventErrorf("node %v is ejected in current epoch %d but readmitted by EpochSetup event for epoch %d", nextEpochIdentitySkeleton.NodeID, u.parentState.CurrentEpochSetup.Counter, epochSetup.Counter) + } + nextEpochActiveIdentities = append(nextEpochActiveIdentities, &flow.DynamicIdentityEntry{ NodeID: nextEpochIdentitySkeleton.NodeID, Dynamic: flow.DynamicIdentity{ - Weight: nextEpochIdentitySkeleton.InitialWeight, + Weight: nextEpochIdentitySkeleton.InitialWeight, // according to invariant (II.ii) Ejected: false, }, })