From f0886526ca58a9c513a09469de573f476691599e Mon Sep 17 00:00:00 2001 From: Jordan Schalm Date: Wed, 4 Sep 2024 05:39:34 -0700 Subject: [PATCH 1/5] fix cluster assignment >2/3 internal check During refactoring in 2024-04, this check was modified to incorrectly use the wrong variable (not operating on only Collection Nodes). This commit uses the correct variable, and renames the filtered lists. --- cmd/util/cmd/common/clusters.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/cmd/util/cmd/common/clusters.go b/cmd/util/cmd/common/clusters.go index 6c77db7fbb3..5e5d8e894a4 100644 --- a/cmd/util/cmd/common/clusters.go +++ b/cmd/util/cmd/common/clusters.go @@ -38,9 +38,9 @@ import ( // - error: if any error occurs. Any error returned from this function is irrecoverable. func ConstructClusterAssignment(log zerolog.Logger, partnerNodes, internalNodes flow.IdentityList, numCollectionClusters int) (flow.AssignmentList, flow.ClusterList, error) { - partners := partnerNodes.Filter(filter.HasRole[flow.Identity](flow.RoleCollection)) - internals := internalNodes.Filter(filter.HasRole[flow.Identity](flow.RoleCollection)) - nCollectors := len(partners) + len(internals) + partnerCollectors := partnerNodes.Filter(filter.HasRole[flow.Identity](flow.RoleCollection)) + internalCollectors := internalNodes.Filter(filter.HasRole[flow.Identity](flow.RoleCollection)) + nCollectors := len(partnerCollectors) + len(internalCollectors) // ensure we have at least as many collection nodes as clusters if nCollectors < int(numCollectionClusters) { @@ -49,11 +49,11 @@ func ConstructClusterAssignment(log zerolog.Logger, partnerNodes, internalNodes } // shuffle both collector lists based on a non-deterministic algorithm - partners, err := partners.Shuffle() + partnerCollectors, err := partnerCollectors.Shuffle() if err != nil { log.Fatal().Err(err).Msg("could not shuffle partners") } - internals, err = internals.Shuffle() + internalCollectors, err = internalCollectors.Shuffle() if err != nil { log.Fatal().Err(err).Msg("could not shuffle internals") } @@ -63,7 +63,7 @@ func ConstructClusterAssignment(log zerolog.Logger, partnerNodes, internalNodes // to control strictly more than 2/3 of the cluster's total weight. // The following is a heuristic that distributes collectors round-robbin across the specified number of clusters. // This heuristic only works when all collectors have equal weight! The following sanity check enforces this: - if len(partnerNodes) > 0 && len(partnerNodes) > 2*len(internalNodes) { + if len(partnerCollectors) > 0 && len(partnerCollectors) > 2*len(internalCollectors) { return nil, nil, fmt.Errorf("requiring at least x>0 number of partner nodes and y > 2x number of internal nodes, but got x,y=%d,%d", len(partnerNodes), len(internalNodes)) } // sanity check ^ enforces that there is at least one internal node, hence `internalNodes[0].InitialWeight` is always a valid reference weight @@ -74,7 +74,7 @@ func ConstructClusterAssignment(log zerolog.Logger, partnerNodes, internalNodes constraint := make([]int, numCollectionClusters) // first, round-robin internal nodes into each cluster - for i, node := range internals { + for i, node := range internalCollectors { if node.InitialWeight != refWeight { return nil, nil, fmt.Errorf("current implementation requires all collectors (partner & interal nodes) to have equal weight") } @@ -84,7 +84,7 @@ func ConstructClusterAssignment(log zerolog.Logger, partnerNodes, internalNodes } // next, round-robin partner nodes into each cluster - for i, node := range partners { + for i, node := range partnerCollectors { if node.InitialWeight != refWeight { return nil, nil, fmt.Errorf("current implementation requires all collectors (partner & interal nodes) to have equal weight") } @@ -102,7 +102,7 @@ func ConstructClusterAssignment(log zerolog.Logger, partnerNodes, internalNodes assignments := assignment.FromIdentifierLists(identifierLists) - collectors := append(partners, internals...) + collectors := append(partnerCollectors, internalCollectors...) clusters, err := factory.NewClusterList(assignments, collectors.ToSkeleton()) if err != nil { log.Fatal().Err(err).Msg("could not create cluster list") From 466b2e6854d07b23409acc55c2e326d032b2ef67 Mon Sep 17 00:00:00 2001 From: Jordan Schalm Date: Wed, 4 Sep 2024 05:55:01 -0700 Subject: [PATCH 2/5] Update cmd/util/cmd/common/clusters.go --- cmd/util/cmd/common/clusters.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/util/cmd/common/clusters.go b/cmd/util/cmd/common/clusters.go index 5e5d8e894a4..9eecffc0a61 100644 --- a/cmd/util/cmd/common/clusters.go +++ b/cmd/util/cmd/common/clusters.go @@ -64,7 +64,7 @@ func ConstructClusterAssignment(log zerolog.Logger, partnerNodes, internalNodes // The following is a heuristic that distributes collectors round-robbin across the specified number of clusters. // This heuristic only works when all collectors have equal weight! The following sanity check enforces this: if len(partnerCollectors) > 0 && len(partnerCollectors) > 2*len(internalCollectors) { - return nil, nil, fmt.Errorf("requiring at least x>0 number of partner nodes and y > 2x number of internal nodes, but got x,y=%d,%d", len(partnerNodes), len(internalNodes)) + return nil, nil, fmt.Errorf("requiring at least x>0 number of partner collection nodes and y > 2x number of internal collection nodes, but got x,y=%d,%d", len(partnerNodes), len(internalNodes)) } // sanity check ^ enforces that there is at least one internal node, hence `internalNodes[0].InitialWeight` is always a valid reference weight refWeight := internalNodes[0].InitialWeight From 666fa16b8f895446139b11ffcf6503a3312473b3 Mon Sep 17 00:00:00 2001 From: Jordan Schalm Date: Wed, 4 Sep 2024 05:56:12 -0700 Subject: [PATCH 3/5] Apply suggestions from code review --- cmd/util/cmd/common/clusters.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/util/cmd/common/clusters.go b/cmd/util/cmd/common/clusters.go index 9eecffc0a61..379d1409bc8 100644 --- a/cmd/util/cmd/common/clusters.go +++ b/cmd/util/cmd/common/clusters.go @@ -64,10 +64,10 @@ func ConstructClusterAssignment(log zerolog.Logger, partnerNodes, internalNodes // The following is a heuristic that distributes collectors round-robbin across the specified number of clusters. // This heuristic only works when all collectors have equal weight! The following sanity check enforces this: if len(partnerCollectors) > 0 && len(partnerCollectors) > 2*len(internalCollectors) { - return nil, nil, fmt.Errorf("requiring at least x>0 number of partner collection nodes and y > 2x number of internal collection nodes, but got x,y=%d,%d", len(partnerNodes), len(internalNodes)) + return nil, nil, fmt.Errorf("requiring at least x>0 number of partner collection nodes and y > 2x number of internal collection nodes, but got x,y=%d,%d", len(partnerCollectors), len(internalCollectors)) } // sanity check ^ enforces that there is at least one internal node, hence `internalNodes[0].InitialWeight` is always a valid reference weight - refWeight := internalNodes[0].InitialWeight + refWeight := internalCollectors[0].InitialWeight identifierLists := make([]flow.IdentifierList, numCollectionClusters) // array to track the 2/3 internal-nodes constraint (internal_nodes > 2 * partner_nodes) From a82f198cae61315f788873b79fabf640089a3d37 Mon Sep 17 00:00:00 2001 From: Jordan Schalm Date: Wed, 4 Sep 2024 05:56:35 -0700 Subject: [PATCH 4/5] Update cmd/util/cmd/common/clusters.go --- cmd/util/cmd/common/clusters.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/util/cmd/common/clusters.go b/cmd/util/cmd/common/clusters.go index 379d1409bc8..03aeba29529 100644 --- a/cmd/util/cmd/common/clusters.go +++ b/cmd/util/cmd/common/clusters.go @@ -66,7 +66,7 @@ func ConstructClusterAssignment(log zerolog.Logger, partnerNodes, internalNodes if len(partnerCollectors) > 0 && len(partnerCollectors) > 2*len(internalCollectors) { return nil, nil, fmt.Errorf("requiring at least x>0 number of partner collection nodes and y > 2x number of internal collection nodes, but got x,y=%d,%d", len(partnerCollectors), len(internalCollectors)) } - // sanity check ^ enforces that there is at least one internal node, hence `internalNodes[0].InitialWeight` is always a valid reference weight + // sanity check ^ enforces that there is at least one internal node, hence `internalCollectors[0].InitialWeight` is always a valid reference weight refWeight := internalCollectors[0].InitialWeight identifierLists := make([]flow.IdentifierList, numCollectionClusters) From fd6e94562af7ea522678749bf32f9c909e459d0a Mon Sep 17 00:00:00 2001 From: Jordan Schalm Date: Wed, 4 Sep 2024 08:50:59 -0700 Subject: [PATCH 5/5] remove incorrect check --- cmd/util/cmd/common/clusters.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/cmd/util/cmd/common/clusters.go b/cmd/util/cmd/common/clusters.go index 03aeba29529..6451f1494e7 100644 --- a/cmd/util/cmd/common/clusters.go +++ b/cmd/util/cmd/common/clusters.go @@ -58,15 +58,7 @@ func ConstructClusterAssignment(log zerolog.Logger, partnerNodes, internalNodes log.Fatal().Err(err).Msg("could not shuffle internals") } - // The following is a heuristic for distributing the internal collector nodes (private staking key available - // to generate QC for cluster root block) and partner nodes (private staking unknown). We need internal nodes - // to control strictly more than 2/3 of the cluster's total weight. - // The following is a heuristic that distributes collectors round-robbin across the specified number of clusters. - // This heuristic only works when all collectors have equal weight! The following sanity check enforces this: - if len(partnerCollectors) > 0 && len(partnerCollectors) > 2*len(internalCollectors) { - return nil, nil, fmt.Errorf("requiring at least x>0 number of partner collection nodes and y > 2x number of internal collection nodes, but got x,y=%d,%d", len(partnerCollectors), len(internalCollectors)) - } - // sanity check ^ enforces that there is at least one internal node, hence `internalCollectors[0].InitialWeight` is always a valid reference weight + // capture first reference weight to validate that all collectors have equal weight refWeight := internalCollectors[0].InitialWeight identifierLists := make([]flow.IdentifierList, numCollectionClusters)