chore: first proposer does not work

internal/consensus/state_data.go

@@ -264,7 +264,7 @@ func (s *StateData) updateToState(state sm.State, commit *types.Commit, blockSto
 		height = state.InitialHeight
 	}
 
-	// state.Validators contain validator set at (state.LastBlockHeight, 0)
+	// state.Validators contain validator set at (state.LastBlockHeight+1, 0)
 	validators := state.Validators
 
 	if s.Validators == nil || !bytes.Equal(s.Validators.QuorumHash, validators.QuorumHash) {
@@ -274,29 +274,16 @@ func (s *StateData) updateToState(state sm.State, commit *types.Commit, blockSto
 
 	s.Validators = validators
 	var err error
-	if s.state.LastBlockHeight == 0 {
-		s.ProposerSelector, err = validatorscoring.NewProposerStrategy(
-			state.ConsensusParams,
-			s.Validators,
-			state.InitialHeight,
-			0,
-			blockStore,
-		)
-	} else {
-		// validators == state.Validators contain validator set at (state.LastBlockHeight, 0)
-		s.ProposerSelector, err = validatorscoring.NewProposerStrategy(
-			state.ConsensusParams,
-			s.Validators,
-			state.LastBlockHeight,
-			state.LastBlockRound,
-			blockStore,
-		)
-	}
+
+	s.ProposerSelector, err = validatorscoring.NewProposerStrategy(
+		state.ConsensusParams,
+		s.Validators,
+		height,
+		0,
+		blockStore,
+	)
 	if err != nil {
-		s.logger.Error("error creating proposer selector",
-			"height", s.state.LastBlockHeight, "round", 0,
-			"validators", s.Validators,
-			"err", err)
+		s.logger.Error("error creating proposer selector", "height", height, "round", 0, "validators", s.Validators, "err", err)
 		panic(fmt.Sprintf("error creating proposer selector: %v", err))
 	}
 	s.ProposerSelector.SetLogger(s.logger)

internal/features/validatorscoring/height_round_score.go

@@ -54,7 +54,7 @@ func NewHeightRoundProposerSelector(vset *types.ValidatorSet, currentHeight int6
 	// if we have a block store, we can determine the proposer for the current height;
 	// otherwise we just trust the state of `vset`
 	if bs != nil && bs.Base() > 0 && currentHeight >= bs.Base() {
-		if err := selectRound0Proposer(currentHeight, s.valSet, s.bs, s.logger); err != nil {
+		if _, err := selectRound0Proposer(currentHeight, s.valSet, s.bs, s.logger); err != nil {
 			return nil, fmt.Errorf("could not initialize proposer: %w", err)
 		}
 		s.valSet.IncProposerIndex(currentRound)
@@ -100,8 +100,8 @@ func (s *heightRoundProposerSelector) updateScores(newHeight int64, newRound int
 		if s.bs == nil || s.bs.Base() > s.height {
 			return fmt.Errorf("cannot jump more than one height without data in block store: %d -> %d", s.height, newHeight)
 		}
-		// FIXME: we assume that no consensus version update happened in the meantime
-		if err := selectRound0Proposer(newHeight, s.valSet, s.bs, s.logger); err != nil {
+		// we assume that no consensus version update happened in the meantime
+		if _, err := selectRound0Proposer(newHeight, s.valSet, s.bs, s.logger); err != nil {
 			return fmt.Errorf("could not determine proposer: %w", err)
 		}
 

internal/features/validatorscoring/height_round_score_test.go

@@ -2,6 +2,7 @@ package validatorscoring_test
 
 import (
 	"bytes"
+	"math/big"
 	"math/rand"
 	"testing"
 
@@ -14,52 +15,51 @@ import (
 )
 
 func TestProposerSelectionHR(t *testing.T) {
+	const nVals = 4
 	const initialHeight = 1
 
-	proTxHashes := make([]crypto.ProTxHash, 4)
-	proTxHashes[0] = crypto.Checksum([]byte("avalidator_address12"))
-	proTxHashes[1] = crypto.Checksum([]byte("bvalidator_address12"))
-	proTxHashes[2] = crypto.Checksum([]byte("cvalidator_address12"))
-	proTxHashes[3] = crypto.Checksum([]byte("dvalidator_address12"))
+	proTxHashes := make([]crypto.ProTxHash, 0, nVals)
+	for i := 0; i < nVals; i++ {
+		protx := make([]byte, crypto.ProTxHashSize)
+		big.NewInt(int64(i + 1)).FillBytes(protx)
+		proTxHashes = append(proTxHashes, protx)
+	}
 
 	vset, _ := types.GenerateValidatorSet(types.NewValSetParam(proTxHashes))
 
-	vs, err := validatorscoring.NewProposerStrategy(types.ConsensusParams{}, vset.Copy(), initialHeight, 0, nil)
-	require.NoError(t, err)
-
 	// initialize proposers
-	proposerOrder := make([]*types.Validator, 4)
-	for i := 0; i < 4; i++ {
-		proposerOrder[i] = vs.MustGetProposer(int64(i+initialHeight), 0)
+	proposerOrder := make([]*types.Validator, vset.Size())
+	for i := 0; i < vset.Size(); i++ {
+		proposerOrder[i] = vset.Validators[i]
 	}
 
-	// h for the loop
-	// j for the times
-	// we should go in order for ever, despite some IncrementProposerPriority with times > 1
 	var (
-		h int
-		j uint32
+		h             int
+		proposerIndex uint32
 	)
 
 	// HEIGHT ROUND STRATEGY
 
-	// With rounds strategy, we should have different order of proposers
-	vs, err = validatorscoring.NewProposerStrategy(types.ConsensusParams{
+	vs, err := validatorscoring.NewProposerStrategy(types.ConsensusParams{
 		Version: types.VersionParams{
 			ConsensusVersion: int32(tmtypes.VersionParams_CONSENSUS_VERSION_1),
 		},
-	}, vset.Copy(), 1, 0, nil)
+	}, vset.Copy(), initialHeight, 0, nil)
 	require.NoError(t, err)
 
-	j = 0
+	proposerIndex = 0
 	for h = 1; h <= 10000; h++ {
 		got := vs.MustGetProposer(int64(h), 0).ProTxHash
-		expected := proposerOrder[j%4].ProTxHash
+		expected := proposerOrder[proposerIndex%nVals].ProTxHash
 		if !bytes.Equal(got, expected) {
-			t.Fatalf("vset.Proposer (%X) does not match expected proposer (%X) for (%d, %d)", got, expected, h, j)
+			t.Fatalf("vset.Proposer (%X) does not match expected proposer (%X) for (%d, %d)", got, expected, h, proposerIndex)
 		}
 
 		round := uint32(rand.Int31n(100))
 		require.NoError(t, vs.UpdateScores(int64(h), int32(round)))
+
+		// t.Logf("Height: %d, Round: %d, proposer index %d", h, round, proposerIndex)
+		// we expect proposer increase for each round, plus one for next height
+		proposerIndex += 1 + round
 	}
 }

internal/features/validatorscoring/height_score.go

@@ -53,29 +53,46 @@ func NewHeightBasedScoringStrategy(vset *types.ValidatorSet, currentHeight int64
 	// if we have a block store, we can determine the proposer for the current height;
 	// otherwise we just trust the state of `vset`
 	if bs != nil && bs.Base() > 0 && currentHeight >= bs.Base() {
-		if err := selectRound0Proposer(currentHeight, s.valSet, s.bs, s.logger); err != nil {
+		if err := s.selectProposer(currentHeight); err != nil {
 			return nil, fmt.Errorf("could not initialize proposer: %w", err)
 		}
 	}
 	return s, nil
 }
 
+func (s *heightBasedScoringStrategy) selectProposer(height int64) error {
+	rotated, err := selectRound0Proposer(height, s.valSet, s.bs, s.logger)
+	if err != nil {
+		return err
+	}
+	if rotated {
+		// workaround for backwards-compatibility with old (broken) code
+		s.valSet.IncProposerIndex(1)
+	}
+
+	return nil
+}
+
 // selectRound0Proposer determines the proposer for the given height and round 0 based on validators read from the block
 // store `bs`.
 // It updates the proposer in the validator set `valSet`.
-func selectRound0Proposer(height int64, valSet *types.ValidatorSet, bs BlockCommitStore, logger log.Logger) error {
+//
+// ## Returns
+//
+// * `qorumRotated` `bool` - true if quorum rotation was detected; false otherwise
+func selectRound0Proposer(height int64, valSet *types.ValidatorSet, bs BlockCommitStore, logger log.Logger) (qorumRotated bool, err error) {
 	if bs == nil {
-		return fmt.Errorf("block store is nil")
+		return qorumRotated, fmt.Errorf("block store is nil")
 	}
 
 	// special case for genesis
 	if height == 0 || height == 1 {
 		// we take first proposer from the validator set
 		if err := valSet.SetProposer(valSet.Validators[0].ProTxHash); err != nil {
-			return fmt.Errorf("could not determine proposer: %w", err)
+			return qorumRotated, fmt.Errorf("could not determine proposer: %w", err)
 		}
 
-		return nil
+		return qorumRotated, nil
 	}
 
 	meta := bs.LoadBlockMeta(height)
@@ -86,37 +103,51 @@ func selectRound0Proposer(height int64, valSet *types.ValidatorSet, bs BlockComm
 		// block not found; we try previous height, and will just add 1 to proposer index
 		meta = bs.LoadBlockMeta(height - 1)
 		if meta == nil {
-			return fmt.Errorf("could not find block meta for previous height %d", height-1)
+			return qorumRotated, fmt.Errorf("could not find block meta for previous height %d", height-1)
 		}
 		addToIdx = 1
-	}
 
-	if !meta.Header.ValidatorsHash.Equal(valSet.Hash()) {
-		logger.Debug("quorum rotation happened",
-			"height", height,
-			"validators_hash", meta.Header.ValidatorsHash, "quorum_hash", valSet.QuorumHash,
-			"validators", valSet,
-			"meta", meta)
-		// quorum rotation happened - we select 1st validator as proposer, and don't rotate
-		// NOTE: We use index 1 due to bug in original code - we need to preserve the original bad behavior
-		// to avoid breaking consensus
-		proposer = valSet.GetByIndex(1).ProTxHash
-
-		addToIdx = 0
+		// check if validators changed; no need to do it when `height` is found as it already uses correct validators
+		if !meta.Header.ValidatorsHash.Equal(valSet.Hash()) {
+			logger.Debug("quorum rotation happened",
+				"height", height,
+				"validators_hash", meta.Header.ValidatorsHash, "quorum_hash", valSet.QuorumHash,
+				"validators", valSet)
+			// quorum rotation happened - we select 1st validator as proposer, and don't rotate
+			// NOTE: We use index 1 due to bug in original code - we need to preserve the original bad behavior
+			// to avoid breaking consensus
+			proposer = valSet.GetByIndex(0).ProTxHash
+			addToIdx = 0
+			qorumRotated = true
+		} else {
+			proposer = meta.Header.ProposerProTxHash
+		}
 	} else {
-		proposer = meta.Header.ProposerProTxHash
+		if !meta.Header.ValidatorsHash.Equal(valSet.Hash()) {
+			logger.Error("quorum rotation detected but not expected",
+				"height", height,
+				"validators_hash", meta.Header.ValidatorsHash, "quorum_hash", valSet.QuorumHash,
+				"validators", valSet)
+
+			return false, fmt.Errorf("quorum hash mismatch at height %d", height)
+		} else {
+			// block `height` found
+			proposer = meta.Header.ProposerProTxHash
+			// rewind rounds, as the proposer is for round `meta.Round`
+			addToIdx = -meta.Round
+		}
 	}
 
 	if err := valSet.SetProposer(proposer); err != nil {
-		return fmt.Errorf("could not set proposer: %w", err)
+		return qorumRotated, fmt.Errorf("could not set proposer: %w", err)
 	}
 
-	if (addToIdx + meta.Round) > 0 {
+	if addToIdx != 0 {
 		// we want to return proposer at round 0, so we move back
-		valSet.IncProposerIndex(addToIdx - meta.Round)
+		valSet.IncProposerIndex(addToIdx)
 	}
 
-	return nil
+	return qorumRotated, nil
 }
 
 // UpdateScores updates the scores of the validators to the given height.
@@ -149,7 +180,7 @@ func (s *heightBasedScoringStrategy) updateScores(newHeight int64, _round int32)
 		}
 		// FIXME: we assume that no consensus version update happened in the meantime
 
-		if err := selectRound0Proposer(newHeight, s.valSet, s.bs, s.logger); err != nil {
+		if err := s.selectProposer(newHeight); err != nil {
 			return fmt.Errorf("could not determine proposer: %w", err)
 		}
 

internal/state/store_test.go

@@ -92,6 +92,7 @@ func TestStoreLoadValidators(t *testing.T) {
 	// initialize block store - create mock validators for each height
 	blockStoreVS := expectedVS.Copy()
 	blockStore := mocks.NewBlockStore(t)
+	blockStore.On("Base").Return(int64(1)).Maybe()
 	for h := int64(1); h <= valSetCheckpointInterval; h++ {
 		blockStore.On("LoadBlockMeta", h).Return(&types.BlockMeta{
 			Header: types.Header{

test/e2e/networks/rotate.toml

@@ -50,6 +50,36 @@ validator04 = 100
 validator05 = 100
 validator09 = 100
 
+[validator_update.1060]
+validator01 = 100
+validator02 = 100
+validator03 = 100
+validator04 = 100
+validator05 = 100
+
+[validator_update.1061]
+validator01 = 100
+validator07 = 100
+validator08 = 100
+validator09 = 100
+validator10 = 100
+
+
+[validator_update.1062]
+validator01 = 100
+validator02 = 100
+validator03 = 100
+validator04 = 100
+validator05 = 100
+
+
+[validator_update.1063]
+validator01 = 100
+validator03 = 100
+validator04 = 100
+validator05 = 100
+validator11 = 100
+
 [node.seed01]
 mode = "seed"
 perturb = ["restart"]