[Consensus] Use SignerIndices in CollectionGuarantee (3/4) (#2140)

* implement IdentitiesByIndices * implement encoding and decoding * remove IdentitiesByIndices * use ParentVoterIndices * use chainID * add ChainID * fix test case * remove file * sort assignment to canonical order * fix guarantee * fix execution ingestion core * use signer indices * add identifierOrder * use guarantor.FindGuarantors * fix tests * fix tests * fix tests * disable tests * fix tests * fix tests, add validation * fix fvm test * fix access tests * fix tests * fix tests * fix select_filter_test tests * fix access tests * fix access integration * fix access integration tests * revert fvm_test changes * update fvm_test * fix exec tests * fix ingestion engine tests * fix test cases * remove todo * fix error handling * add comment * fix tests * update logging * refactor * fix tests * remove function * fix error wrapping * update comment * update tests * remove unverified signature * revert mutator tests change * update comment * address comments * add tests * add check in NewClusterList to ensure the assignments are sorted in canonical order * rename method * consensus ingestion core log signer indices * remove tests log * refactor canonical order check * replace order.ByNodeIDAsc with order.Canonical * [Consensus] Refactor for guarantee signer indices (4/4) (#2204) * wip * wip * starting to fix tests * adding tests * happy path test * Added toDo for fixing tests for unhappy paths * • fixed packer tests • consolidated logic for checking the padded bits * re-gen mocks * fix validPadding * fix findguarantors * refactor ingestion/core.go * move FindGuarantors to protocol * remove commented code * fix name * fix tests * small refactor * fix import * Apply suggestions from code review Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> * fix error type * fix type * fix error message * update comments * update comment * update tests * fix identity_test * fix tests * fix unittest * fix cluster tests * fix tests * fixtures ingestion engine tests * fix execution_test * fix consensus inclusion tests * fix bootstrap constraint check * fix cycle dep from NewClusterList (#2225) Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
onflow · Mar 30, 2022 · c94e689 · c94e689
1 parent 619a82e
commit c94e689
Show file tree

Hide file tree

Showing 111 changed files with 2,285 additions and 1,412 deletions.
diff --git a/cmd/bootstrap/cmd/clusters.go b/cmd/bootstrap/cmd/clusters.go
@@ -5,6 +5,8 @@ import (
 	model "github.com/onflow/flow-go/model/bootstrap"
 	"github.com/onflow/flow-go/model/cluster"
 	"github.com/onflow/flow-go/model/flow"
+	"github.com/onflow/flow-go/model/flow/assignment"
+	"github.com/onflow/flow-go/model/flow/factory"
 	"github.com/onflow/flow-go/model/flow/filter"
 )
 
@@ -23,20 +25,22 @@ func constructClusterAssignment(partnerNodes, internalNodes []model.NodeInfo, se
 	internals = internals.DeterministicShuffle(seed)
 
 	nClusters := flagCollectionClusters
-	assignments := make(flow.AssignmentList, nClusters)
+	identifierLists := make([]flow.IdentifierList, nClusters)
 
 	// first, round-robin internal nodes into each cluster
 	for i, node := range internals {
-		assignments[i%len(assignments)] = append(assignments[i%len(assignments)], node.NodeID)
+		identifierLists[i%len(identifierLists)] = append(identifierLists[i%len(identifierLists)], node.NodeID)
 	}
 
 	// next, round-robin partner nodes into each cluster
 	for i, node := range partners {
-		assignments[i%len(assignments)] = append(assignments[i%len(assignments)], node.NodeID)
+		identifierLists[i%len(identifierLists)] = append(identifierLists[i%len(identifierLists)], node.NodeID)
 	}
 
+	assignments := assignment.FromIdentifierLists(identifierLists)
+
 	collectors := append(partners, internals...)
-	clusters, err := flow.NewClusterList(assignments, collectors)
+	clusters, err := factory.NewClusterList(assignments, collectors)
 	if err != nil {
 		log.Fatal().Err(err).Msg("could not create cluster list")
 	}

diff --git a/cmd/bootstrap/cmd/constraints.go b/cmd/bootstrap/cmd/constraints.go
@@ -50,12 +50,12 @@ func checkConstraints(partnerNodes, internalNodes []model.NodeInfo) {
 			if _, exists := internals.ByNodeID(node.NodeID); exists {
 				clusterInternalCount++
 			}
-			if clusterInternalCount <= clusterPartnerCount*2 {
-				log.Fatal().Msgf(
-					"will not bootstrap configuration without Byzantine majority within cluster: "+
-						"(partners=%d, internals=%d, min_internals=%d)",
-					clusterPartnerCount, clusterInternalCount, clusterPartnerCount*2+1)
-			}
+		}
+		if clusterInternalCount <= clusterPartnerCount*2 {
+			log.Fatal().Msgf(
+				"will not bootstrap configuration without Byzantine majority within cluster: "+
+					"(partners=%d, internals=%d, min_internals=%d)",
+				clusterPartnerCount, clusterInternalCount, clusterPartnerCount*2+1)
 		}
 		partnerCOLCount += clusterPartnerCount
 		internalCOLCount += clusterInternalCount

diff --git a/cmd/bootstrap/cmd/seal.go b/cmd/bootstrap/cmd/seal.go
@@ -7,7 +7,7 @@ import (
 	"github.com/onflow/flow-go/model/dkg"
 	"github.com/onflow/flow-go/model/flow"
 	"github.com/onflow/flow-go/model/flow/order"
-	"github.com/onflow/flow-go/module/packer"
+	"github.com/onflow/flow-go/module/signature"
 )
 
 func constructRootResultAndSeal(
@@ -47,7 +47,7 @@ func constructRootResultAndSeal(
 	qcsWithSignerIDs := make([]*flow.QuorumCertificateWithSignerIDs, 0, len(clusterQCs))
 	for i, clusterQC := range clusterQCs {
 		members := assignments[i]
-		signerIDs, err := packer.DecodeSignerIdentifiersFromIndices(members, clusterQC.SignerIndices)
+		signerIDs, err := signature.DecodeSignerIndicesToIdentifiers(members, clusterQC.SignerIndices)
 		if err != nil {
 			log.Fatal().Err(err).Msgf("could not decode signer IDs from clusterQC at index %v", i)
 		}

diff --git a/cmd/util/cmd/epochs/cmd/reset_test.go b/cmd/util/cmd/epochs/cmd/reset_test.go
@@ -30,7 +30,7 @@ func TestReset_LocalSnapshot(t *testing.T) {
 		unittest.RunWithTempDir(t, func(bootDir string) {
 
 			// create a root snapshot
-			rootSnapshot := unittest.RootSnapshotFixture(unittest.IdentityListFixture(10))
+			rootSnapshot := unittest.RootSnapshotFixture(unittest.IdentityListFixture(10, unittest.WithAllRoles()))
 
 			// write snapshot to correct path in bootDir
 			err := writeRootSnapshot(bootDir, rootSnapshot)
@@ -62,7 +62,7 @@ func TestReset_LocalSnapshot(t *testing.T) {
 		unittest.RunWithTempDir(t, func(bootDir string) {
 
 			// create a root snapshot
-			rootSnapshot := unittest.RootSnapshotFixture(unittest.IdentityListFixture(10))
+			rootSnapshot := unittest.RootSnapshotFixture(unittest.IdentityListFixture(10, unittest.WithAllRoles()))
 
 			// write snapshot to correct path in bootDir
 			err := writeRootSnapshot(bootDir, rootSnapshot)

diff --git a/consensus/follower_test.go b/consensus/follower_test.go
@@ -5,6 +5,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/onflow/flow-go/module/signature"
+
 	"github.com/rs/zerolog"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
@@ -16,7 +18,6 @@ import (
 	"github.com/onflow/flow-go/consensus/hotstuff/model"
 	"github.com/onflow/flow-go/model/flow"
 	mockmodule "github.com/onflow/flow-go/module/mock"
-	"github.com/onflow/flow-go/module/packer"
 	mockstorage "github.com/onflow/flow-go/storage/mock"
 	"github.com/onflow/flow-go/utils/unittest"
 )
@@ -69,9 +70,9 @@ func (s *HotStuffFollowerSuite) SetupTest() {
 
 	// mock consensus committee
 	s.committee = &mockhotstuff.Committee{}
-	s.committee.On("Identities", mock.Anything, mock.Anything).Return(
-		func(blockID flow.Identifier, selector flow.IdentityFilter) flow.IdentityList {
-			return identities.Filter(selector)
+	s.committee.On("Identities", mock.Anything).Return(
+		func(blockID flow.Identifier) flow.IdentityList {
+			return identities
 		},
 		nil,
 	)
@@ -107,7 +108,7 @@ func (s *HotStuffFollowerSuite) SetupTest() {
 		View:      52078,
 	}
 
-	signerIndices, err := packer.EncodeSignerIdentifiersToIndices(identities.NodeIDs(), identities.NodeIDs()[:3])
+	signerIndices, err := signature.EncodeSignersToIndices(identities.NodeIDs(), identities.NodeIDs()[:3])
 	require.NoError(s.T(), err)
 	s.rootQC = &flow.QuorumCertificate{
 		View:          s.rootHeader.View,
@@ -339,7 +340,7 @@ func (mc *MockConsensus) extendBlock(blockView uint64, parent *flow.Header) *flo
 	nextBlock := unittest.BlockHeaderWithParentFixture(parent)
 	nextBlock.View = blockView
 	nextBlock.ProposerID = mc.identities[int(blockView)%len(mc.identities)].NodeID
-	signerIndices, _ := packer.EncodeSignerIdentifiersToIndices(mc.identities.NodeIDs(), mc.identities.NodeIDs())
+	signerIndices, _ := signature.EncodeSignersToIndices(mc.identities.NodeIDs(), mc.identities.NodeIDs())
 	nextBlock.ParentVoterIndices = signerIndices
 	return &nextBlock
 }
diff --git a/consensus/hotstuff/committees/leader/leader_selection_test.go b/consensus/hotstuff/committees/leader/leader_selection_test.go
@@ -304,59 +304,55 @@ func TestZeroStakedNodeWillNotBeSelected(t *testing.T) {
 		rng, err := random.NewChacha20PRG(someSeed, []byte("leader_selec"))
 		require.NoError(t, err)
 
-		for i := 0; i < 100; i++ {
-			// create 1002 nodes with all 0 stake
-			identities := unittest.IdentityListFixture(1002, unittest.WithStake(0))
-
-			// create 2 nodes with 1 stake, and place them in between
-			// index 233-777
-			n := rng.UintN(777-233) + 233
-			m := rng.UintN(777-233) + 233
-			identities[n].Stake = 1
-			identities[m].Stake = 1
-
-			// the following code check the zero staker should not be selected
-			stakeful := identities.Filter(filter.HasStake(true))
-
-			count := 1000
-			selectionFromAll, err := ComputeLeaderSelectionFromSeed(0, someSeed, count, identities)
-			require.NoError(t, err)
+		// create 1002 nodes with all 0 stake
+		identities := unittest.IdentityListFixture(1002, unittest.WithStake(0))
 
-			selectionFromStakeful, err := ComputeLeaderSelectionFromSeed(0, someSeed, count, stakeful)
-			require.NoError(t, err)
+		// create 2 nodes with 1 stake, and place them in between
+		// index 233-777
+		n := rng.UintN(777-233) + 233
+		m := rng.UintN(777-233) + 233
+		identities[n].Stake = 1
+		identities[m].Stake = 1
 
-			for i := 0; i < count; i++ {
-				nodeIDFromAll, err := selectionFromAll.LeaderForView(uint64(i))
-				require.NoError(t, err)
+		// the following code check the zero staker should not be selected
+		stakeful := identities.Filter(filter.HasStake(true))
 
-				nodeIDFromStakeful, err := selectionFromStakeful.LeaderForView(uint64(i))
-				require.NoError(t, err)
+		count := 1000
+		selectionFromAll, err := ComputeLeaderSelectionFromSeed(0, someSeed, count, identities)
+		require.NoError(t, err)
 
-				// the selection should be the same
-				require.Equal(t, nodeIDFromStakeful, nodeIDFromAll)
-			}
+		selectionFromStakeful, err := ComputeLeaderSelectionFromSeed(0, someSeed, count, stakeful)
+		require.NoError(t, err)
+
+		for i := 0; i < count; i++ {
+			nodeIDFromAll, err := selectionFromAll.LeaderForView(uint64(i))
+			require.NoError(t, err)
+
+			nodeIDFromStakeful, err := selectionFromStakeful.LeaderForView(uint64(i))
+			require.NoError(t, err)
+
+			// the selection should be the same
+			require.Equal(t, nodeIDFromStakeful, nodeIDFromAll)
 		}
 
 		t.Run("if there is only 1 node has stake, then it will be always be the leader and the only leader", func(t *testing.T) {
 			rng, err := random.NewChacha20PRG(someSeed, []byte("leader_selec"))
 			require.NoError(t, err)
 
-			for i := 0; i < 100; i++ {
-				identities := unittest.IdentityListFixture(1000, unittest.WithStake(0))
+			identities := unittest.IdentityListFixture(1000, unittest.WithStake(0))
 
-				n := rng.UintN(1000)
-				stake := n + 1
-				identities[n].Stake = stake
-				onlyStaked := identities[n]
+			n := rng.UintN(1000)
+			stake := n + 1
+			identities[n].Stake = stake
+			onlyStaked := identities[n]
 
-				selections, err := ComputeLeaderSelectionFromSeed(0, someSeed, 1000, identities)
-				require.NoError(t, err)
+			selections, err := ComputeLeaderSelectionFromSeed(0, someSeed, 1000, identities)
+			require.NoError(t, err)
 
-				for i := 0; i < 1000; i++ {
-					nodeID, err := selections.LeaderForView(uint64(i))
-					require.NoError(t, err)
-					require.Equal(t, onlyStaked.NodeID, nodeID)
-				}
+			for i := 0; i < 1000; i++ {
+				nodeID, err := selections.LeaderForView(uint64(i))
+				require.NoError(t, err)
+				require.Equal(t, onlyStaked.NodeID, nodeID)
 			}
 		})
 	})

diff --git a/consensus/hotstuff/integration/instance_test.go b/consensus/hotstuff/integration/instance_test.go
@@ -23,15 +23,13 @@ import (
 	"github.com/onflow/flow-go/consensus/hotstuff/notifications"
 	"github.com/onflow/flow-go/consensus/hotstuff/pacemaker"
 	"github.com/onflow/flow-go/consensus/hotstuff/pacemaker/timeout"
-	hsig "github.com/onflow/flow-go/consensus/hotstuff/signature"
 	"github.com/onflow/flow-go/consensus/hotstuff/validator"
 	"github.com/onflow/flow-go/consensus/hotstuff/voteaggregator"
 	"github.com/onflow/flow-go/consensus/hotstuff/votecollector"
 	"github.com/onflow/flow-go/consensus/hotstuff/voter"
 	"github.com/onflow/flow-go/model/flow"
 	"github.com/onflow/flow-go/module/irrecoverable"
 	module "github.com/onflow/flow-go/module/mock"
-	"github.com/onflow/flow-go/module/packer"
 	msig "github.com/onflow/flow-go/module/signature"
 	"github.com/onflow/flow-go/utils/unittest"
 )
@@ -209,7 +207,7 @@ func NewInstance(t require.TestingT, options ...Option) *Instance {
 				View:     block.View,
 				BlockID:  block.BlockID,
 				SignerID: in.localID,
-				SigData:  unittest.RandomBytes(hsig.SigLen * 2), // double sig, one staking, one beacon
+				SigData:  unittest.RandomBytes(msig.SigLen * 2), // double sig, one staking, one beacon
 			}
 			return vote
 		},
@@ -222,10 +220,8 @@ func NewInstance(t require.TestingT, options ...Option) *Instance {
 				voterIDs = append(voterIDs, vote.SignerID)
 			}
 
-			signerIndices, err := packer.EncodeSignerIdentifiersToIndices(in.participants.NodeIDs(), voterIDs)
-			if err != nil {
-				panic(fmt.Errorf("could not encode signer indices: %w", err))
-			}
+			signerIndices, err := msig.EncodeSignersToIndices(in.participants.NodeIDs(), voterIDs)
+			require.NoError(t, err, "could not encode signer indices")
 
 			qc := &flow.QuorumCertificate{
 				View:          votes[0].View,
@@ -351,7 +347,7 @@ func NewInstance(t require.TestingT, options ...Option) *Instance {
 	rbRector := helper.MakeRandomBeaconReconstructor(msig.RandomBeaconThreshold(int(in.participants.Count())))
 	rbRector.On("Verify", mock.Anything, mock.Anything).Return(nil).Maybe()
 
-	indices, err := toIndices(in.participants.NodeIDs(), in.participants.NodeIDs())
+	indices, err := msig.EncodeSignersToIndices(in.participants.NodeIDs(), []flow.Identifier(in.participants.NodeIDs()))
 	require.NoError(t, err)
 
 	packer := &mocks.Packer{}
@@ -391,10 +387,6 @@ func NewInstance(t require.TestingT, options ...Option) *Instance {
 	return &in
 }
 
-func toIndices(all []flow.Identifier, signers []flow.Identifier) ([]byte, error) {
-	return packer.EncodeSignerIdentifiersToIndices(all, signers)
-}
-
 func (in *Instance) Run() error {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer func() {

diff --git a/consensus/hotstuff/mocks/packer.go b/consensus/hotstuff/mocks/packer.go
diff --git a/consensus/hotstuff/mocks/verifier.go b/consensus/hotstuff/mocks/verifier.go
diff --git a/consensus/hotstuff/model/errors.go b/consensus/hotstuff/model/errors.go
@@ -9,7 +9,6 @@ import (
 
 var (
 	ErrUnverifiableBlock = errors.New("block proposal can't be verified, because its view is above the finalized view, but its QC is below the finalized view")
-	ErrInvalidFormat     = errors.New("invalid signature format")
 	ErrInvalidSignature  = errors.New("invalid signature")
 )
 
@@ -26,6 +25,28 @@ func IsNoVoteError(err error) bool {
 	return errors.As(err, &e)
 }
 
+// InvalidFormatError indicates that some data has an incompatible format.
+type InvalidFormatError struct {
+	err error
+}
+
+func NewInvalidFormatError(err error) error {
+	return InvalidFormatError{err}
+}
+
+func NewInvalidFormatErrorf(msg string, args ...interface{}) error {
+	return InvalidFormatError{fmt.Errorf(msg, args...)}
+}
+
+func (e InvalidFormatError) Error() string { return e.err.Error() }
+func (e InvalidFormatError) Unwrap() error { return e.err }
+
+// IsInvalidFormatError returns whether err is a InvalidFormatError
+func IsInvalidFormatError(err error) bool {
+	var e InvalidFormatError
+	return errors.As(err, &e)
+}
+
 // ConfigurationError indicates that a constructor or component was initialized with
 // invalid or inconsistent parameters.
 type ConfigurationError struct {

diff --git a/consensus/hotstuff/packer/packer.go → consensus/hotstuff/model/signature_data.go b/consensus/hotstuff/packer/packer.go → consensus/hotstuff/model/signature_data.go
@@ -1,10 +1,8 @@
-package packer
+package model
 
 import (
 	"bytes"
-	"fmt"
 
-	"github.com/onflow/flow-go/consensus/hotstuff/model"
 	"github.com/onflow/flow-go/crypto"
 	"github.com/onflow/flow-go/model/encoding/rlp"
 )
@@ -49,7 +47,7 @@ func UnpackRandomBeaconSig(sigData []byte) (crypto.Signature, error) {
 	packer := SigDataPacker{}
 	sig, err := packer.Decode(sigData)
 	if err != nil {
-		return nil, fmt.Errorf("could not decode sig data %s: %w", err, model.ErrInvalidFormat)
+		return nil, NewInvalidFormatErrorf("could not decode sig data: %w", err)
 	}
 	return sig.ReconstructedRandomBeaconSig, nil
 }