From 37b91f9c0c0249d00f56730451fe806826f06cf9 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Tue, 17 Sep 2024 16:07:56 +0200 Subject: [PATCH] chore: first proposer does not work --- internal/consensus/state_data.go | 33 +++----- .../validatorscoring/height_round_score.go | 6 +- .../height_round_score_test.go | 44 +++++----- .../features/validatorscoring/height_score.go | 81 +++++++++++++------ internal/state/store_test.go | 1 + test/e2e/networks/rotate.toml | 30 +++++++ 6 files changed, 122 insertions(+), 73 deletions(-) diff --git a/internal/consensus/state_data.go b/internal/consensus/state_data.go index 5817adefa..95a78e4ca 100644 --- a/internal/consensus/state_data.go +++ b/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) diff --git a/internal/features/validatorscoring/height_round_score.go b/internal/features/validatorscoring/height_round_score.go index 3f9234d60..acb0455fd 100644 --- a/internal/features/validatorscoring/height_round_score.go +++ b/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) } diff --git a/internal/features/validatorscoring/height_round_score_test.go b/internal/features/validatorscoring/height_round_score_test.go index 5d3a1c2c1..9000dc7e2 100644 --- a/internal/features/validatorscoring/height_round_score_test.go +++ b/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 } } diff --git a/internal/features/validatorscoring/height_score.go b/internal/features/validatorscoring/height_score.go index 293f1b2b3..8a5c42a8c 100644 --- a/internal/features/validatorscoring/height_score.go +++ b/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) } diff --git a/internal/state/store_test.go b/internal/state/store_test.go index b1b5486c7..d5dbcf5e6 100644 --- a/internal/state/store_test.go +++ b/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{ diff --git a/test/e2e/networks/rotate.toml b/test/e2e/networks/rotate.toml index 736e56765..14190da09 100644 --- a/test/e2e/networks/rotate.toml +++ b/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"]