From c99c1ac090284528c91f633e4c6ee829369f7f31 Mon Sep 17 00:00:00 2001 From: Alexander Hentschel Date: Tue, 17 Oct 2023 16:57:40 -0700 Subject: [PATCH 1/3] =?UTF-8?q?=E2=80=A2=20extending=20documentation=20aro?= =?UTF-8?q?und=20cluster=20committee=20=E2=80=A2=20compactifying=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../hotstuff/committees/cluster_committee.go | 95 ++++++++++--------- state/protocol/cluster.go | 4 +- 2 files changed, 55 insertions(+), 44 deletions(-) diff --git a/consensus/hotstuff/committees/cluster_committee.go b/consensus/hotstuff/committees/cluster_committee.go index f1d388af2df..99e9e895a75 100644 --- a/consensus/hotstuff/committees/cluster_committee.go +++ b/consensus/hotstuff/committees/cluster_committee.go @@ -20,18 +20,18 @@ import ( // implementation reference blocks on the cluster chain, which in turn reference // blocks on the main chain - this implementation manages that translation. type Cluster struct { - state protocol.State - payloads storage.ClusterPayloads - me flow.Identifier - // pre-computed leader selection for the full lifecycle of the cluster - selection *leader.LeaderSelection - // a filter that returns all members of the cluster committee allowed to vote - clusterMemberFilter flow.IdentityFilter[flow.Identity] - // initial set of cluster members, WITHOUT dynamic weight changes - initialClusterMembers flow.IdentitySkeletonList + state protocol.State + payloads storage.ClusterPayloads + me flow.Identifier + selection *leader.LeaderSelection // pre-computed leader selection for the full lifecycle of the cluster + + clusterMembers flow.IdentitySkeletonList // cluster members in canonical order as specified by the epoch smart contract + clusterMemberFilter flow.IdentityFilter[flow.Identity] // filter that returns true for all members of the cluster committee allowed to vote + weightThresholdForQC uint64 // computed based on initial cluster committee weights + weightThresholdForTO uint64 // computed based on initial cluster committee weights + + // initialClusterIdentities lists full Identities for cluster members (in canonical order) at time of cluster initialization by Epoch smart contract initialClusterIdentities flow.IdentityList - weightThresholdForQC uint64 // computed based on initial cluster committee weights - weightThresholdForTO uint64 // computed based on initial cluster committee weights } var _ hotstuff.Replicas = (*Cluster)(nil) @@ -44,7 +44,6 @@ func NewClusterCommittee( epoch protocol.Epoch, me flow.Identifier, ) (*Cluster, error) { - selection, err := leader.SelectionForCluster(cluster, epoch) if err != nil { return nil, fmt.Errorf("could not compute leader selection for cluster: %w", err) @@ -53,18 +52,8 @@ func NewClusterCommittee( initialClusterMembers := cluster.Members() totalWeight := initialClusterMembers.TotalWeight() initialClusterMembersSelector := initialClusterMembers.Selector() - // the next section is not very nice, but there are no dynamic identities for root block, - // and we need them to specificially handle querying of identities for root block - initialClusterIdentities := make(flow.IdentityList, 0, len(cluster.Members())) - for _, skeleton := range initialClusterMembers { - initialClusterIdentities = append(initialClusterIdentities, &flow.Identity{ - IdentitySkeleton: *skeleton, - DynamicIdentity: flow.DynamicIdentity{ - Weight: skeleton.InitialWeight, - Ejected: false, - }, - }) - } + initialClusterIdentities := constructInitialClusterIdentities(initialClusterMembers) + com := &Cluster{ state: state, payloads: payloads, @@ -76,7 +65,7 @@ func NewClusterCommittee( filter.Not(filter.Ejected), filter.HasWeight(true), ), - initialClusterMembers: initialClusterMembers, + clusterMembers: initialClusterMembers, initialClusterIdentities: initialClusterIdentities, weightThresholdForQC: WeightThresholdToBuildQC(totalWeight), weightThresholdForTO: WeightThresholdToTimeout(totalWeight), @@ -90,17 +79,14 @@ func (c *Cluster) IdentitiesByBlock(blockID flow.Identifier) (flow.IdentityList, // blockID is a collection block not a block produced by consensus, // to query the identities from protocol state, we need to use the reference block id from the payload // - // first retrieve the cluster block payload + // first retrieve the cluster block's payload payload, err := c.payloads.ByBlockID(blockID) if err != nil { return nil, fmt.Errorf("could not get cluster payload: %w", err) } - // an empty reference block ID indicates a root block - isRootBlock := payload.ReferenceBlockID == flow.ZeroID - - // use the initial cluster members for root block - if isRootBlock { + // An empty reference block ID indicates a root block. In this case, use the initial cluster members for root block + if isRootBlock := payload.ReferenceBlockID == flow.ZeroID; isRootBlock { return c.initialClusterIdentities, nil } @@ -110,18 +96,14 @@ func (c *Cluster) IdentitiesByBlock(blockID flow.Identifier) (flow.IdentityList, } func (c *Cluster) IdentityByBlock(blockID flow.Identifier, nodeID flow.Identifier) (*flow.Identity, error) { - - // first retrieve the cluster block payload + // first retrieve the cluster block's payload payload, err := c.payloads.ByBlockID(blockID) if err != nil { return nil, fmt.Errorf("could not get cluster payload: %w", err) } - // an empty reference block ID indicates a root block - isRootBlock := payload.ReferenceBlockID == flow.ZeroID - - // use the initial cluster members for root block - if isRootBlock { + // An empty reference block ID indicates a root block. In this case, use the initial cluster members for root block + if isRootBlock := payload.ReferenceBlockID == flow.ZeroID; isRootBlock { identity, ok := c.initialClusterIdentities.ByNodeID(nodeID) if !ok { return nil, model.NewInvalidSignerErrorf("node %v is not an authorized hotstuff participant", nodeID) @@ -143,11 +125,12 @@ func (c *Cluster) IdentityByBlock(blockID flow.Identifier, nodeID flow.Identifie return identity, nil } -// IdentitiesByEpoch returns the initial cluster members for this epoch. The view -// parameter is the view in the cluster consensus. Since clusters only exist for -// one epoch, we don't need to check the view. +// IdentitiesByEpoch returns the IdentitySkeletons of the cluster members in canonical order. +// This represents the cluster composition at the time the cluster was specified by the epoch smart +// contract (hence, we return IdentitySkeletons as opposed to full identities). Since clusters only +// exist for one epoch, we don't need to check the view. func (c *Cluster) IdentitiesByEpoch(_ uint64) (flow.IdentitySkeletonList, error) { - return c.initialClusterMembers, nil + return c.clusterMembers, nil } // IdentityByEpoch returns the node from the initial cluster members for this epoch. @@ -158,7 +141,7 @@ func (c *Cluster) IdentitiesByEpoch(_ uint64) (flow.IdentitySkeletonList, error) // - model.InvalidSignerError if nodeID was not listed by the Epoch Setup event as an // authorized participant in this cluster func (c *Cluster) IdentityByEpoch(view uint64, participantID flow.Identifier) (*flow.IdentitySkeleton, error) { - identity, ok := c.initialClusterMembers.ByNodeID(participantID) + identity, ok := c.clusterMembers.ByNodeID(participantID) if !ok { return nil, model.NewInvalidSignerErrorf("node %v is not an authorized hotstuff participant", participantID) } @@ -196,3 +179,29 @@ func (c *Cluster) Self() flow.Identifier { func (c *Cluster) DKG(_ uint64) (hotstuff.DKG, error) { panic("queried DKG of cluster committee") } + +// constructInitialClusterIdentities extends the IdentitySkeletons of the cluster members to their full Identities +// (in canonical order). at time of cluster initialization by Epoch smart contract. This represents the cluster +// composition at the time the cluster was specified by the epoch smart contract. +// +// CONTEXT: The EpochSetup event contains the IdentitySkeletons for each cluster, thereby specifying cluster membership. +// 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 also 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, when the EpochSetup event is emitted / processed, the weight of +// all cluster members equals their InitialWeight and the Ejected flag is false. +func constructInitialClusterIdentities(clusterMembers flow.IdentitySkeletonList) flow.IdentityList { + initialClusterIdentities := make(flow.IdentityList, 0, len(clusterMembers)) + for _, skeleton := range clusterMembers { + initialClusterIdentities = append(initialClusterIdentities, &flow.Identity{ + IdentitySkeleton: *skeleton, + DynamicIdentity: flow.DynamicIdentity{ + Weight: skeleton.InitialWeight, + Ejected: false, + }, + }) + } + return initialClusterIdentities +} diff --git a/state/protocol/cluster.go b/state/protocol/cluster.go index 6a62aa7c050..3001d026542 100644 --- a/state/protocol/cluster.go +++ b/state/protocol/cluster.go @@ -20,7 +20,9 @@ type Cluster interface { // EpochCounter returns the epoch counter for this cluster. EpochCounter() uint64 - // Members returns the initial set of collector nodes in this cluster. + // Members returns the IdentitySkeletons of the cluster members in canonical order. + // This represents the cluster composition at the time the cluster was specified by the epoch smart + // contract (hence, we return IdentitySkeletons as opposed to full identities). Members() flow.IdentitySkeletonList // RootBlock returns the root block for this cluster. From b2c97b5696506c80236821e1856c453e947f634f Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Thu, 19 Oct 2023 12:18:23 +0300 Subject: [PATCH 2/3] Apply suggestions from PR review --- cmd/bootstrap/run/cluster_qc.go | 4 +- .../committees/consensus_committee.go | 2 +- .../hotstuff/committees/leader/consensus.go | 2 +- consensus/integration/nodes_test.go | 1 - .../test/cluster_switchover_test.go | 2 +- engine/consensus/dkg/reactor_engine.go | 2 +- engine/execution/execution_test.go | 60 ++++++++----------- engine/verification/utils/unittest/helper.go | 6 +- insecure/corruptnet/network_egress_test.go | 2 +- insecure/corruptnet/network_test_helper.go | 2 +- .../integration/tests/composability_test.go | 2 +- model/flow/factory/cluster_list.go | 5 +- model/flow/filter/identity.go | 24 +++++--- model/flow/identity_list.go | 6 +- model/flow/mapfunc/identity.go | 6 ++ model/flow/protocol_state.go | 19 ++++-- utils/unittest/fixtures.go | 4 ++ 17 files changed, 83 insertions(+), 66 deletions(-) diff --git a/cmd/bootstrap/run/cluster_qc.go b/cmd/bootstrap/run/cluster_qc.go index 689a65a339c..39f06fd81f6 100644 --- a/cmd/bootstrap/run/cluster_qc.go +++ b/cmd/bootstrap/run/cluster_qc.go @@ -32,8 +32,8 @@ func GenerateClusterRootQC(signers []bootstrap.NodeInfo, allCommitteeMembers flo } // STEP 1.5: patch committee to include dynamic identities. This is a temporary measure until bootstrapping is refactored. - // We need to do this since the committee is used to create the QC uses dynamic identities, but clustering for root block contain only - // static identities since there no state transitions haven't happened yet. + // We need a Committee for creating the cluster's root QC and the Committee requires dynamic identities to be instantiated. + // The clustering for root block contain only static identities, since there no state transitions have happened yet. dynamicCommitteeMembers := make(flow.IdentityList, 0, len(allCommitteeMembers)) for _, participant := range allCommitteeMembers { dynamicCommitteeMembers = append(dynamicCommitteeMembers, &flow.Identity{ diff --git a/consensus/hotstuff/committees/consensus_committee.go b/consensus/hotstuff/committees/consensus_committee.go index 172f67cd4e8..f4dd5548670 100644 --- a/consensus/hotstuff/committees/consensus_committee.go +++ b/consensus/hotstuff/committees/consensus_committee.go @@ -55,7 +55,7 @@ func newStaticEpochInfo(epoch protocol.Epoch) (*staticEpochInfo, error) { if err != nil { return nil, fmt.Errorf("could not initial identities: %w", err) } - initialCommittee := initialIdentities.Filter(filter.IsAllowedConsensusCommitteeMember).ToSkeleton() + initialCommittee := initialIdentities.Filter(filter.IsConsensusCommitteeMember).ToSkeleton() dkg, err := epoch.DKG() if err != nil { return nil, fmt.Errorf("could not get dkg: %w", err) diff --git a/consensus/hotstuff/committees/leader/consensus.go b/consensus/hotstuff/committees/leader/consensus.go index a2c1400b8e0..f278e690f76 100644 --- a/consensus/hotstuff/committees/leader/consensus.go +++ b/consensus/hotstuff/committees/leader/consensus.go @@ -43,7 +43,7 @@ func SelectionForConsensus(epoch protocol.Epoch) (*LeaderSelection, error) { firstView, rng, int(finalView-firstView+1), // add 1 because both first/final view are inclusive - identities.Filter(filter.IsAllowedConsensusCommitteeMember), + identities.Filter(filter.IsConsensusCommitteeMember), ) return leaders, err } diff --git a/consensus/integration/nodes_test.go b/consensus/integration/nodes_test.go index 940556bea75..dbb6a9cd350 100644 --- a/consensus/integration/nodes_test.go +++ b/consensus/integration/nodes_test.go @@ -472,7 +472,6 @@ func createNode( rootQC, err := rootSnapshot.QuorumCertificate() require.NoError(t, err) - // selector := filter.HasRole[flow.Identity](flow.RoleConsensus) committee, err := committees.NewConsensusCommittee(state, localID) require.NoError(t, err) protocolStateEvents.AddConsumer(committee) diff --git a/engine/collection/test/cluster_switchover_test.go b/engine/collection/test/cluster_switchover_test.go index 1e72d754f79..d9797b89ab8 100644 --- a/engine/collection/test/cluster_switchover_test.go +++ b/engine/collection/test/cluster_switchover_test.go @@ -60,7 +60,7 @@ func NewClusterSwitchoverTestCase(t *testing.T, conf ClusterSwitchoverTestConf) identityRoles := unittest.CompleteIdentitySet(unittest.IdentityListFixture(int(conf.collectors), unittest.WithRole(flow.RoleCollection))...) identities := flow.IdentityList{} for _, missingRole := range identityRoles { - nodeInfo := unittest.PrivateNodeInfosFixture(1, unittest.WithRole(missingRole.Role))[0] + nodeInfo := unittest.PrivateNodeInfoFixture(unittest.WithRole(missingRole.Role)) tc.nodeInfos = append(tc.nodeInfos, nodeInfo) identities = append(identities, nodeInfo.Identity()) } diff --git a/engine/consensus/dkg/reactor_engine.go b/engine/consensus/dkg/reactor_engine.go index e9c0669cf9b..b1055b9ff89 100644 --- a/engine/consensus/dkg/reactor_engine.go +++ b/engine/consensus/dkg/reactor_engine.go @@ -181,7 +181,7 @@ func (e *ReactorEngine) startDKGForEpoch(currentEpochCounter uint64, first *flow log.Fatal().Err(err).Msg("could not retrieve epoch info") } - committee := curDKGInfo.identities.Filter(filter.IsAllowedConsensusCommitteeMember) + committee := curDKGInfo.identities.Filter(filter.IsConsensusCommitteeMember) log.Info(). Uint64("phase1", curDKGInfo.phase1FinalView). diff --git a/engine/execution/execution_test.go b/engine/execution/execution_test.go index cb4228842a7..0ac4d14e23a 100644 --- a/engine/execution/execution_test.go +++ b/engine/execution/execution_test.go @@ -43,26 +43,22 @@ func TestExecutionFlow(t *testing.T) { chainID := flow.Testnet - colID := unittest.PrivateNodeInfosFixture( - 1, + colID := unittest.PrivateNodeInfoFixture( unittest.WithRole(flow.RoleCollection), unittest.WithKeys, - )[0] - conID := unittest.PrivateNodeInfosFixture( - 1, + ) + conID := unittest.PrivateNodeInfoFixture( unittest.WithRole(flow.RoleConsensus), unittest.WithKeys, - )[0] - exeID := unittest.PrivateNodeInfosFixture( - 1, + ) + exeID := unittest.PrivateNodeInfoFixture( unittest.WithRole(flow.RoleExecution), unittest.WithKeys, - )[0] - verID := unittest.PrivateNodeInfosFixture( - 1, + ) + verID := unittest.PrivateNodeInfoFixture( unittest.WithRole(flow.RoleVerification), unittest.WithKeys, - )[0] + ) identities := unittest.CompleteIdentitySet(colID.Identity(), conID.Identity(), exeID.Identity(), verID.Identity()). Sort(order.Canonical[flow.Identity]) @@ -357,21 +353,18 @@ func TestFailedTxWillNotChangeStateCommitment(t *testing.T) { chainID := flow.Emulator - colNodeInfo := unittest.PrivateNodeInfosFixture( - 1, + colNodeInfo := unittest.PrivateNodeInfoFixture( unittest.WithRole(flow.RoleCollection), unittest.WithKeys, - )[0] - conNodeInfo := unittest.PrivateNodeInfosFixture( - 1, + ) + conNodeInfo := unittest.PrivateNodeInfoFixture( unittest.WithRole(flow.RoleConsensus), unittest.WithKeys, - )[0] - exe1NodeInfo := unittest.PrivateNodeInfosFixture( - 1, + ) + exe1NodeInfo := unittest.PrivateNodeInfoFixture( unittest.WithRole(flow.RoleExecution), unittest.WithKeys, - )[0] + ) colID := colNodeInfo.Identity() conID := conNodeInfo.Identity() @@ -516,31 +509,26 @@ func TestBroadcastToMultipleVerificationNodes(t *testing.T) { chainID := flow.Emulator - colID := unittest.PrivateNodeInfosFixture( - 1, + colID := unittest.PrivateNodeInfoFixture( unittest.WithRole(flow.RoleCollection), unittest.WithKeys, - )[0] - conID := unittest.PrivateNodeInfosFixture( - 1, + ) + conID := unittest.PrivateNodeInfoFixture( unittest.WithRole(flow.RoleConsensus), unittest.WithKeys, - )[0] - exeID := unittest.PrivateNodeInfosFixture( - 1, + ) + exeID := unittest.PrivateNodeInfoFixture( unittest.WithRole(flow.RoleExecution), unittest.WithKeys, - )[0] - ver1ID := unittest.PrivateNodeInfosFixture( - 1, + ) + ver1ID := unittest.PrivateNodeInfoFixture( unittest.WithRole(flow.RoleVerification), unittest.WithKeys, - )[0] - ver2ID := unittest.PrivateNodeInfosFixture( - 1, + ) + ver2ID := unittest.PrivateNodeInfoFixture( unittest.WithRole(flow.RoleVerification), unittest.WithKeys, - )[0] + ) identities := unittest.CompleteIdentitySet(colID.Identity(), conID.Identity(), diff --git a/engine/verification/utils/unittest/helper.go b/engine/verification/utils/unittest/helper.go index d963fdb82bd..d3ce766ec5b 100644 --- a/engine/verification/utils/unittest/helper.go +++ b/engine/verification/utils/unittest/helper.go @@ -628,13 +628,13 @@ func bootstrapSystem( bootstrapNodesInfo := make([]bootstrap.NodeInfo, 0) var verID bootstrap.NodeInfo for _, missingRole := range unittest.CompleteIdentitySet() { - nodeInfo := unittest.PrivateNodeInfosFixture(1, unittest.WithRole(missingRole.Role))[0] + nodeInfo := unittest.PrivateNodeInfoFixture(unittest.WithRole(missingRole.Role)) if nodeInfo.Role == flow.RoleVerification { verID = nodeInfo } bootstrapNodesInfo = append(bootstrapNodesInfo, nodeInfo) } - bootstrapNodesInfo = append(bootstrapNodesInfo, unittest.PrivateNodeInfosFixture(1, unittest.WithRole(flow.RoleExecution))...) // adds extra execution node + bootstrapNodesInfo = append(bootstrapNodesInfo, unittest.PrivateNodeInfoFixture(unittest.WithRole(flow.RoleExecution))) // adds extra execution node identities := bootstrap.ToIdentityList(bootstrapNodesInfo) @@ -645,7 +645,7 @@ func bootstrapSystem( if !authorized { // creates a new verification node identity that is unauthorized for this epoch - verID = unittest.PrivateNodeInfosFixture(1, unittest.WithRole(flow.RoleVerification))[0] + verID = unittest.PrivateNodeInfoFixture(unittest.WithRole(flow.RoleVerification)) bootstrapNodesInfo = append(bootstrapNodesInfo, verID) identities = append(identities, verID.Identity()) diff --git a/insecure/corruptnet/network_egress_test.go b/insecure/corruptnet/network_egress_test.go index 81facb551db..072f4394a9f 100644 --- a/insecure/corruptnet/network_egress_test.go +++ b/insecure/corruptnet/network_egress_test.go @@ -26,7 +26,7 @@ import ( // The attacker is mocked out in this test. func TestHandleOutgoingEvent_AttackerRegistered(t *testing.T) { codec := unittest.NetworkCodec() - corruptedIdentity := unittest.PrivateNodeInfosFixture(1, unittest.WithAddress(insecure.DefaultAddress))[0] + corruptedIdentity := unittest.PrivateNodeInfoFixture(unittest.WithAddress(insecure.DefaultAddress)) flowNetwork := mocknetwork.NewNetwork(t) ccf := mockinsecure.NewCorruptConduitFactory(t) ccf.On("RegisterEgressController", mock.Anything).Return(nil) diff --git a/insecure/corruptnet/network_test_helper.go b/insecure/corruptnet/network_test_helper.go index 76d8e2545ac..1f7ff8b1cf4 100644 --- a/insecure/corruptnet/network_test_helper.go +++ b/insecure/corruptnet/network_test_helper.go @@ -32,7 +32,7 @@ func corruptNetworkFixture(t *testing.T, logger zerolog.Logger, corruptedID ...f // create corruptible network with no attacker registered codec := unittest.NetworkCodec() - corruptedIdentity := unittest.PrivateNodeInfosFixture(1, unittest.WithAddress(insecure.DefaultAddress))[0] + corruptedIdentity := unittest.PrivateNodeInfoFixture(unittest.WithAddress(insecure.DefaultAddress)) // some tests will want to create corruptible network with a specific ID if len(corruptedID) > 0 { corruptedIdentity.NodeID = corruptedID[0] diff --git a/insecure/integration/tests/composability_test.go b/insecure/integration/tests/composability_test.go index 3d8190a68ca..9e996697b7e 100644 --- a/insecure/integration/tests/composability_test.go +++ b/insecure/integration/tests/composability_test.go @@ -122,7 +122,7 @@ func TestCorruptNetworkFrameworkHappyPath(t *testing.T) { // withCorruptNetwork creates a real corrupt network, starts it, runs the "run" function, and then stops it. func withCorruptNetwork(t *testing.T, run func(*testing.T, flow.Identity, *corruptnet.Network, *stub.Hub)) { codec := unittest.NetworkCodec() - corruptedIdentity := unittest.PrivateNodeInfosFixture(1, unittest.WithAddress(insecure.DefaultAddress))[0] + corruptedIdentity := unittest.PrivateNodeInfoFixture(unittest.WithAddress(insecure.DefaultAddress)) // life-cycle management of orchestratorNetwork. ctx, cancel := context.WithCancel(context.Background()) diff --git a/model/flow/factory/cluster_list.go b/model/flow/factory/cluster_list.go index aa86dcc7146..49ffae74447 100644 --- a/model/flow/factory/cluster_list.go +++ b/model/flow/factory/cluster_list.go @@ -15,7 +15,7 @@ import ( func NewClusterList(assignments flow.AssignmentList, collectors flow.IdentitySkeletonList) (flow.ClusterList, error) { // build a lookup for all the identities by node identifier - lookup := make(map[flow.Identifier]*flow.IdentitySkeleton) + lookup := collectors.Lookup() for _, collector := range collectors { lookup[collector.NodeID] = collector } @@ -23,7 +23,8 @@ func NewClusterList(assignments flow.AssignmentList, collectors flow.IdentitySke return nil, fmt.Errorf("duplicate collector in list") } - // replicate the identifier list but use identities instead + // assignments only contains the NodeIDs for each cluster. In the following, we + // substitute them with the respective IdentitySkeletons. clusters := make(flow.ClusterList, 0, len(assignments)) for i, participants := range assignments { cluster := make(flow.IdentitySkeletonList, 0, len(participants)) diff --git a/model/flow/filter/identity.go b/model/flow/filter/identity.go index c8b3bac7a96..99ac738f644 100644 --- a/model/flow/filter/identity.go +++ b/model/flow/filter/identity.go @@ -52,8 +52,11 @@ func Not[T flow.GenericIdentity](filter flow.IdentityFilter[T]) flow.IdentityFil } } -// In returns a filter for identities within the input list. This is equivalent -// to HasNodeID, but for list-typed inputs. +// In returns a filter for identities within the input list. For an input identity i, +// the filter returns true if and only if i ∈ list. +// Caution: The filter solely operates on NodeIDs. Other identity fields are not compared. +// This function is just a compact representation of `HasNodeID[T](list.NodeIDs()...)` +// which behaves algorithmically the same way. func In[T flow.GenericIdentity](list flow.GenericIdentityList[T]) flow.IdentityFilter[T] { return HasNodeID[T](list.NodeIDs()...) } @@ -91,7 +94,12 @@ func HasInitialWeight[T flow.GenericIdentity](hasWeight bool) flow.IdentityFilte } } -// HasWeight returns a filter for nodes with non-zero weight. +// HasWeight filters Identities by their weight: +// When `hasWeight == true`: +// - for an input identity i, the filter returns true if and only if i's weight is greater than zero +// +// When `hasWeight == false`: +// - for an input identity i, the filter returns true if and only if i's weight is zero func HasWeight(hasWeight bool) flow.IdentityFilter[flow.Identity] { return func(identity *flow.Identity) bool { return (identity.Weight > 0) == hasWeight @@ -122,9 +130,11 @@ var IsValidCurrentEpochParticipant = And( Not(Ejected), // ejection will change signer index ) -// IsAllowedConsensusCommitteeMember is a identity filter for all members of -// the consensus committee allowed to participate. -var IsAllowedConsensusCommitteeMember = And( +// IsConsensusCommitteeMember is an identity filter for all members of the consensus committee. +// Formally, a Node X is a Consensus Committee Member if and only if X is a consensus node with +// positive initial stake. This is specified by the EpochSetup Event and remains static +// throughout the epoch. +var IsConsensusCommitteeMember = And( HasRole[flow.IdentitySkeleton](flow.RoleConsensus), HasInitialWeight[flow.IdentitySkeleton](true), ) @@ -139,4 +149,4 @@ var IsVotingConsensusCommitteeMember = And[flow.Identity]( // IsValidDKGParticipant is an identity filter for all DKG participants. It is // equivalent to the filter for consensus committee members, as these are // the same group for now. -var IsValidDKGParticipant = IsAllowedConsensusCommitteeMember +var IsValidDKGParticipant = IsConsensusCommitteeMember diff --git a/model/flow/identity_list.go b/model/flow/identity_list.go index 0aa05fcd0fb..8e2f842dadc 100644 --- a/model/flow/identity_list.go +++ b/model/flow/identity_list.go @@ -317,8 +317,10 @@ func IdentitySkeletonListEqualTo(lhs, rhs IdentitySkeletonList) bool { } // Exists takes a previously sorted Identity list and searches it for the target -// identity by its NodeID. Caution: other identity fields are not compared. -// CAUTION: The identity list MUST be sorted prior to calling this method +// identity by its NodeID. +// CAUTION: +// - Other identity fields are not compared. +// - The identity list MUST be sorted prior to calling this method. func (il GenericIdentityList[T]) Exists(target *T) bool { return il.IdentifierExists((*target).GetNodeID()) } diff --git a/model/flow/mapfunc/identity.go b/model/flow/mapfunc/identity.go index 681e21d1088..a58bc7d0844 100644 --- a/model/flow/mapfunc/identity.go +++ b/model/flow/mapfunc/identity.go @@ -4,6 +4,9 @@ import ( "github.com/onflow/flow-go/model/flow" ) +// WithInitialWeight returns an anonymous function that assigns the given weight value +// to `Identity.InitialWeight`. This function is primarily intended for testing, as +// Identity structs should be immutable by convention. func WithInitialWeight(weight uint64) flow.IdentityMapFunc[flow.Identity] { return func(identity flow.Identity) flow.Identity { identity.InitialWeight = weight @@ -11,6 +14,9 @@ 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. 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 443c285a279..cf3444a77ac 100644 --- a/model/flow/protocol_state.go +++ b/model/flow/protocol_state.go @@ -209,14 +209,14 @@ func NewRichProtocolStateEntry( } 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 otherIdentities IdentitySkeletonList + var previousEpochIdentities IdentitySkeletonList if previousEpochSetup != nil { - otherIdentities = previousEpochSetup.Participants + previousEpochIdentities = previousEpochSetup.Participants } result.CurrentEpochIdentityTable, err = BuildIdentityTable( protocolState.CurrentEpoch.ActiveIdentities, currentEpochSetup.Participants, - otherIdentities, + previousEpochIdentities, ) if err != nil { return nil, fmt.Errorf("could not build identity table for staking phase: %w", err) @@ -354,8 +354,9 @@ func (ll DynamicIdentityEntryList) Sort(less IdentifierOrder) DynamicIdentityEnt } // BuildIdentityTable constructs the full identity table for the target epoch by combining data from: -// 1. The target epoch's Dynamic Identities. -// 2. The target epoch's IdentitySkeletons +// 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 // (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 @@ -369,7 +370,13 @@ func BuildIdentityTable( targetEpochIdentitySkeletons IdentitySkeletonList, adjacentEpochIdentitySkeletons IdentitySkeletonList, ) (IdentityList, error) { - // produce a unique set for current and previous epoch participants + // Combine the participants of the current and adjacent epoch. The method `GenericIdentityList.Union` + // already implements the following required conventions: + // 1. Preference for IdentitySkeleton of the target epoch: + // In case an IdentitySkeleton with the same NodeID exists in the target epoch as well as + // in the adjacent epoch, we use the IdentitySkeleton for the target epoch (for example, + // to account for changes of keys, address, initial weight, etc). + // 2. Canonical ordering allEpochParticipants := targetEpochIdentitySkeletons.Union(adjacentEpochIdentitySkeletons) // sanity check: size of identities should be equal to previous and current epoch participants combined if len(allEpochParticipants) != len(targetEpochDynamicIdentities) { diff --git a/utils/unittest/fixtures.go b/utils/unittest/fixtures.go index 2ad4a4609dc..8ba2c2f87b2 100644 --- a/utils/unittest/fixtures.go +++ b/utils/unittest/fixtures.go @@ -1094,6 +1094,10 @@ func NodeInfosFixture(n int, opts ...func(*flow.Identity)) []bootstrap.NodeInfo return nodeInfos } +func PrivateNodeInfoFixture(opts ...func(*flow.Identity)) bootstrap.NodeInfo { + return PrivateNodeInfosFixture(1, opts...)[0] +} + func PrivateNodeInfosFixture(n int, opts ...func(*flow.Identity)) []bootstrap.NodeInfo { il := IdentityListFixture(n, opts...) nodeInfos := make([]bootstrap.NodeInfo, 0, n) From 244af7b94621f4183609ee73684cd0aa66dc815c Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Thu, 19 Oct 2023 13:52:21 +0300 Subject: [PATCH 3/3] Simplified bootstrap in cluster switchover test --- engine/collection/test/cluster_switchover_test.go | 13 +++++-------- engine/verification/utils/unittest/helper.go | 1 - model/flow/filter/identity.go | 2 +- utils/unittest/fixtures.go | 7 +++++-- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/engine/collection/test/cluster_switchover_test.go b/engine/collection/test/cluster_switchover_test.go index d9797b89ab8..4785969fd28 100644 --- a/engine/collection/test/cluster_switchover_test.go +++ b/engine/collection/test/cluster_switchover_test.go @@ -56,14 +56,11 @@ func NewClusterSwitchoverTestCase(t *testing.T, conf ClusterSwitchoverTestConf) t: t, conf: conf, } - - identityRoles := unittest.CompleteIdentitySet(unittest.IdentityListFixture(int(conf.collectors), unittest.WithRole(flow.RoleCollection))...) - identities := flow.IdentityList{} - for _, missingRole := range identityRoles { - nodeInfo := unittest.PrivateNodeInfoFixture(unittest.WithRole(missingRole.Role)) - tc.nodeInfos = append(tc.nodeInfos, nodeInfo) - identities = append(identities, nodeInfo.Identity()) - } + tc.nodeInfos = unittest.PrivateNodeInfosFromIdentityList( + unittest.CompleteIdentitySet( + unittest.IdentityListFixture(int(conf.collectors), unittest.WithRole(flow.RoleCollection))...), + ) + identities := model.ToIdentityList(tc.nodeInfos) collectors := identities.Filter(filter.HasRole[flow.Identity](flow.RoleCollection)).ToSkeleton() assignment := unittest.ClusterAssignment(tc.conf.clusters, collectors) clusters, err := factory.NewClusterList(assignment, collectors) diff --git a/engine/verification/utils/unittest/helper.go b/engine/verification/utils/unittest/helper.go index d3ce766ec5b..2fa9e00a936 100644 --- a/engine/verification/utils/unittest/helper.go +++ b/engine/verification/utils/unittest/helper.go @@ -635,7 +635,6 @@ func bootstrapSystem( bootstrapNodesInfo = append(bootstrapNodesInfo, nodeInfo) } bootstrapNodesInfo = append(bootstrapNodesInfo, unittest.PrivateNodeInfoFixture(unittest.WithRole(flow.RoleExecution))) // adds extra execution node - identities := bootstrap.ToIdentityList(bootstrapNodesInfo) collector := &metrics.NoopCollector{} diff --git a/model/flow/filter/identity.go b/model/flow/filter/identity.go index 99ac738f644..a4121bcda4d 100644 --- a/model/flow/filter/identity.go +++ b/model/flow/filter/identity.go @@ -139,7 +139,7 @@ var IsConsensusCommitteeMember = And( HasInitialWeight[flow.IdentitySkeleton](true), ) -// IsVotingConsensusCommitteeMember is a identity filter for all members of +// IsVotingConsensusCommitteeMember is an identity filter for all members of // the consensus committee allowed to vote. var IsVotingConsensusCommitteeMember = And[flow.Identity]( HasRole[flow.Identity](flow.RoleConsensus), diff --git a/utils/unittest/fixtures.go b/utils/unittest/fixtures.go index 8ba2c2f87b2..2d479d98dcd 100644 --- a/utils/unittest/fixtures.go +++ b/utils/unittest/fixtures.go @@ -1099,8 +1099,11 @@ func PrivateNodeInfoFixture(opts ...func(*flow.Identity)) bootstrap.NodeInfo { } func PrivateNodeInfosFixture(n int, opts ...func(*flow.Identity)) []bootstrap.NodeInfo { - il := IdentityListFixture(n, opts...) - nodeInfos := make([]bootstrap.NodeInfo, 0, n) + return PrivateNodeInfosFromIdentityList(IdentityListFixture(n, opts...)) +} + +func PrivateNodeInfosFromIdentityList(il flow.IdentityList) []bootstrap.NodeInfo { + nodeInfos := make([]bootstrap.NodeInfo, 0, len(il)) for _, identity := range il { nodeInfo := bootstrap.PrivateNodeInfoFromIdentity(identity, KeyFixture(crypto.ECDSAP256), KeyFixture(crypto.BLSBLS12381)) nodeInfos = append(nodeInfos, nodeInfo)