fix(state): proposer selected twice when prev block round > 0

abci/example/kvstore/verify_test.go

@@ -85,7 +85,7 @@ func (e *blockExecutor) createBlock(txs types.Txs, commit *types.Commit) *types.
 	if commit == nil {
 		commit = &types.Commit{}
 	}
-	proposer := e.state.Validators.GetProposer()
+	proposer := e.state.GetProposerFromState(e.state.LastBlockHeight+1, 0)
 	block := e.state.MakeBlock(
 		e.state.LastBlockHeight+1,
 		txs,

dash/quorum/validator_conn_executor_test.go

@@ -621,7 +621,7 @@ func makeState(nVals int, height int64) (sm.State, dbm.DB) {
 }
 
 func makeBlock(ctx context.Context, t *testing.T, blockExec *sm.BlockExecutor, state sm.State, _height int64, commit *types.Commit) *types.Block {
-	block, crs, err := blockExec.CreateProposalBlock(ctx, 1, 0, state, commit, state.Validators.Proposer.ProTxHash, 1)
+	block, crs, err := blockExec.CreateProposalBlock(ctx, 1, 0, state, commit, state.Validators.Proposer().ProTxHash, 1)
 	require.NoError(t, err)
 
 	err = crs.UpdateBlock(block)

dash/quorum/validator_map_test.go

@@ -3,9 +3,10 @@ package quorum
 import (
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/dashpay/tenderdash/dash/quorum/mock"
 	"github.com/dashpay/tenderdash/types"
-	"github.com/stretchr/testify/assert"
 )
 
 func Test_validatorMap_String(t *testing.T) {
@@ -30,11 +31,11 @@ func Test_validatorMap_String(t *testing.T) {
 		{
 			vm: newValidatorMap(vals),
 			contains: []string{
-				"<nil> VP:0 A:0 N:tcp://0100000000000000000000000000000000000000@127.0.0.1:1}",
-				"<nil> VP:0 A:0 N:tcp://0200000000000000000000000000000000000000@127.0.0.1:2}",
-				"<nil> VP:0 A:0 N:tcp://0300000000000000000000000000000000000000@127.0.0.1:3}",
-				"<nil> VP:0 A:0 N:tcp://0400000000000000000000000000000000000000@127.0.0.1:4}",
-				"<nil> VP:0 A:0 N:tcp://0500000000000000000000000000000000000000@127.0.0.1:5}",
+				"<nil> VP:0 N:tcp://0100000000000000000000000000000000000000@127.0.0.1:1}",
+				"<nil> VP:0 N:tcp://0200000000000000000000000000000000000000@127.0.0.1:2}",
+				"<nil> VP:0 N:tcp://0300000000000000000000000000000000000000@127.0.0.1:3}",
+				"<nil> VP:0 N:tcp://0400000000000000000000000000000000000000@127.0.0.1:4}",
+				"<nil> VP:0 N:tcp://0500000000000000000000000000000000000000@127.0.0.1:5}",
 			},
 		},
 	}

docs/architecture/adr-d002-tenderdash-feature-versioning.md

@@ -0,0 +1,73 @@
+# ADR D002: Tenderdash Consensus Versioning
+
+## Status
+
+Proposed
+
+## Context
+
+Tenderdash currently lacks a flexible mechanism for versioning and feature toggling. The need has arisen to introduce new consensus features without requiring hard forks or block version changes. Specifically, there is a proposal to add a new consensus parameter, proposerSelectionStrategy, which changes the way Tenderdash selects a proposer.
+
+## Problem statement
+
+Tenderdash requires a mechanism to adapt and evolve without necessitating hard forks. The current system lacks flexibility in managing changes, which can include:
+
+- **Bug Fixes**: Addressing and resolving defects in the codebase.
+- **New Features**: Introducing new functionalities to enhance the system.
+- **Modifications to Existing Features**: Updating or improving current features to meet evolving requirements.
+
+The challenge is to implement these changes in a way that allows for seamless updates, ensuring that Tenderdash can remain robust and adaptable without disrupting the network through hard forks.
+
+We shall also ensure separation of concerns, to avoid building logic that depends on particular use case/blockchain (like Dash Platform).
+
+## Proposed solutions
+
+### Solution 1: Use Block Version
+
+The block header contains a `Version` field, which includes the block version. This versioning mechanism can be utilized to manage changes in consensus logic and feature implementations.
+
+#### Pros
+
+- **Existing Field**: The version field is already present in the block header, providing a built-in mechanism for versioning.
+- **Consistency**: Using block versions ensures that all nodes in the network are synchronized with the same versioning logic.
+
+#### Cons
+
+- **Complex Decision-Making**: Determining when to switch logic based on block version can be complex and may depend on specific cases, such as the Dash Platform mainnet.
+- **Hard Fork Requirement**: Changing the block version is considered a hard fork, which is a significant and complex task that requires coordination across the network.
+- **Hardcoded Version**: The block version is hardcoded in `version/version.go`, making changes cumbersome and extensive.
+- **Client Compatibility**: All clients must understand and support all block versions, adding to the complexity and increasing the risk of compatibility issues.
+
+### Scenario 2: Introduce features mechanism
+
+Introduce a new mechanism within consensus parameters to define and control various features. This mechanism will allow for dynamic feature toggling based on consensus parameters rather than block versions. Each feature will have a corresponding parameter that can be set to enable, disable, or select different implementations of the feature.
+
+#### Details
+
+1. **New Consensus Parameters**:
+   - `features`: A map or array of feature flags and their corresponding implementations. Each feature will have a key representing the feature name and a value representing the selected implementation or state (e.g., enabled, disabled).
+
+2. **Implementation**:
+   - Modify the consensus parameters to include the `features` map or array.
+   - Implement factory design pattern to use implementation selected by consensus params.
+   - (Optional) Organize features into a separate directory in the project directory tree
+
+3. **Example Usage**:
+   - At genesis, `features["proposerSelectionStrategy"]` is set to `height`.
+   - At block height 10000, the ABCI App can change `features["proposerSelectionStrategy"]` to `heightRound`.
+   - At block height 30000, the ABCI App can change `features["proposerSelectionStrategy"]` back to `height`.
+
+
+#### Pros
+
+- **Flexibility**: Using consensus parameters for feature toggling allows for more granular control over consensus behavior without requiring hard forks or block version changes.
+- **Scalability**: This approach is scalable as it avoids the need to create new block versions for each feature change.
+- **Separation of Concerns**: Tenderdash logic should not depend on the ABCI App specific use case. Consensus parameters provide a clean separation between the consensus engine and the application logic.
+
+#### Cons
+
+- Potential complexity in managing a large number of consensus parameters.
+
+## Decision
+
+TODO

internal/consensus/gossiper.go

@@ -203,10 +203,10 @@ func (g *msgGossiper) GossipCommit(ctx context.Context, rs cstypes.RoundState, p
 	if prs.HasCommit {
 		return
 	}
-	logger := g.logger.With([]any{
+	logger := g.logger.With(
 		"height", rs.Height,
 		"peer_height", prs.Height,
-	})
+	)
 	var commit *types.Commit
 	blockStoreBase := g.blockStore.Base()
 	if rs.Height == prs.Height+1 {

internal/consensus/msgs_test.go

@@ -78,7 +78,7 @@ func TestMsgToProto(t *testing.T) {
 	vote, err := factory.MakeVote(
 		ctx,
 		pv,
-		&types.ValidatorSet{Proposer: val, Validators: []*types.Validator{val}, QuorumHash: quorumHash, ThresholdPublicKey: pk},
+		&types.ValidatorSet{Validators: []*types.Validator{val}, QuorumHash: quorumHash, ThresholdPublicKey: pk},
 		"chainID",
 		0,
 		1,

internal/consensus/pbts_test.go

@@ -293,7 +293,7 @@ type timestampedEvent struct {
 
 func (p *pbtsTestHarness) pickProposer() types.PrivValidator {
 	stateData := p.observedState.GetStateData()
-	proposer := stateData.Validators.GetProposer()
+	proposer := stateData.ProposerSelector.MustGetProposer(p.currentHeight, p.currentRound)
 	p.observedState.logger.Debug("picking proposer", "protxhash", proposer.ProTxHash)
 
 	allVals := append(p.otherValidators, p.observedValidator)

internal/consensus/peer_state.go

@@ -14,6 +14,7 @@ import (
 	cstypes "github.com/dashpay/tenderdash/internal/consensus/types"
 	"github.com/dashpay/tenderdash/libs/bits"
 	"github.com/dashpay/tenderdash/libs/log"
+	"github.com/dashpay/tenderdash/libs/math"
 	tmproto "github.com/dashpay/tenderdash/proto/tendermint/types"
 	"github.com/dashpay/tenderdash/types"
 )
@@ -214,7 +215,11 @@ func (ps *PeerState) PickVoteToSend(votes types.VoteSetReader) (*types.Vote, boo
 	}
 
 	if index, ok := votes.BitArray().Sub(psVotes).PickRandom(); ok {
-		vote := votes.GetByIndex(int32(index))
+		idx, err := math.SafeConvertInt32(int64(index))
+		if err != nil {
+			panic(fmt.Errorf("failed to convert index to int32: %w", err))
+		}
+		vote := votes.GetByIndex(idx)
 		if vote != nil {
 			return vote, true
 		}

internal/consensus/reactor.go

@@ -223,7 +223,7 @@ func (r *Reactor) SwitchToConsensus(ctx context.Context, state sm.State, skipWAL
 
 	// NOTE: The line below causes broadcastNewRoundStepRoutine() to broadcast a
 	// NewRoundStepMessage.
-	stateData.updateToState(state, nil)
+	stateData.updateToState(state, nil, r.state.blockStore)
 	err := r.state.stateDataStore.Update(stateData)
 	if err != nil {
 		panic(err)

internal/consensus/replay.go

@@ -74,7 +74,6 @@ func (cs *State) readReplayMessage(ctx context.Context, msg *TimedWALMessage, ne
 		case *ProposalMessage:
 			p := msg.Proposal
 			if cs.config.WalSkipRoundsToLast && p.Round > stateData.Round {
-				stateData.Validators.IncrementProposerPriority(p.Round - stateData.Round)
 				stateData.Votes.SetRound(p.Round)
 				stateData.Round = p.Round
 			}

internal/consensus/replay_test.go

@@ -495,7 +495,7 @@ func createSignSendProposal(ctx context.Context,
 	height := stateData.RoundState.Height
 	round := stateData.RoundState.Round
 
-	proposer := stateData.Validators.GetProposer()
+	proposer := stateData.ProposerSelector.MustGetProposer(height, round)
 	proposerVs := findValByProTxHash(ctx, t, vss, proposer.ProTxHash)
 	proposerCs := findStateByProTxHash(t, css, proposer.ProTxHash)
 

internal/consensus/replayer_test.go

@@ -188,7 +188,6 @@ func TestInitChainGenesisTime(t *testing.T) {
 	)
 	require.NoError(t, err)
 	vset.ThresholdPublicKey = recoveredThresholdPublicKey
-	proposerProTxHash := vset.GetProposer().ProTxHash
 
 	genDoc := tmtypes.GenesisDoc{
 		ChainID:            "test-chain",
@@ -208,6 +207,8 @@ func TestInitChainGenesisTime(t *testing.T) {
 	require.NoError(t, err)
 	stateStore := sm.NewStore(dbm.NewMemDB())
 	blockStore := store.NewBlockStore(dbm.NewMemDB())
+	proposer := smState.GetProposerFromState(1, 0)
+	proposerProTxHash := proposer.ProTxHash
 	replayer := newBlockReplayer(stateStore, blockStore, &genDoc, eventBus, proxyApp, proposerProTxHash)
 
 	// use replayer to call initChain

internal/consensus/state.go

@@ -334,7 +334,7 @@ func (cs *State) updateStateFromStore() error {
 		}
 	}
 
-	stateData.updateToState(state, nil)
+	stateData.updateToState(state, nil, cs.blockStore)
 	err = cs.stateDataStore.Update(stateData)
 	if err != nil {
 		return err

internal/consensus/state_apply_commit.go

@@ -34,9 +34,6 @@ func (c *ApplyCommitAction) Execute(ctx context.Context, stateEvent StateEvent)
 	event := stateEvent.Data.(*ApplyCommitEvent)
 	stateData := stateEvent.StateData
 	commit := event.Commit
-	c.logger.Info("applying commit", "commit", commit)
-
-	block, blockParts := stateData.ProposalBlock, stateData.ProposalBlockParts
 
 	height := stateData.Height
 	round := stateData.Round
@@ -45,6 +42,9 @@ func (c *ApplyCommitAction) Execute(ctx context.Context, stateEvent StateEvent)
 		height = commit.Height
 		round = commit.Round
 	}
+	c.logger.Info("applying commit", "commit", commit, "height", height, "round", round)
+
+	block, blockParts := stateData.ProposalBlock, stateData.ProposalBlockParts
 
 	c.blockExec.mustEnsureProcess(ctx, &stateData.RoundState, round)
 	c.blockExec.mustValidate(ctx, stateData)
@@ -88,7 +88,7 @@ func (c *ApplyCommitAction) Execute(ctx context.Context, stateEvent StateEvent)
 	c.RecordMetrics(stateData, height, block, lastBlockMeta)
 
 	// NewHeightStep!
-	stateData.updateToState(stateCopy, commit)
+	stateData.updateToState(stateCopy, commit, c.blockStore)
 	err = stateData.Save()
 	if err != nil {
 		return err

internal/consensus/state_data.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/dashpay/tenderdash/config"
 	cstypes "github.com/dashpay/tenderdash/internal/consensus/types"
+	selectproposer "github.com/dashpay/tenderdash/internal/consensus/versioned/selectproposer"
 	sm "github.com/dashpay/tenderdash/internal/state"
 	"github.com/dashpay/tenderdash/libs/eventemitter"
 	"github.com/dashpay/tenderdash/libs/log"
@@ -144,7 +145,12 @@ func (s *StateData) Save() error {
 }
 
 func (s *StateData) isProposer(proTxHash types.ProTxHash) bool {
-	return proTxHash != nil && bytes.Equal(s.Validators.GetProposer().ProTxHash.Bytes(), proTxHash.Bytes())
+	prop, err := s.ProposerSelector.GetProposer(s.Height, s.Round)
+	if err != nil {
+		s.logger.Error("error getting proposer", "err", err, "height", s.Height, "round", s.Round)
+		return false
+	}
+	return proTxHash != nil && bytes.Equal(prop.ProTxHash.Bytes(), proTxHash.Bytes())
 }
 
 func (s *StateData) isValidator(proTxHash types.ProTxHash) bool {
@@ -177,11 +183,17 @@ func (s *StateData) updateRoundStep(round int32, step cstypes.RoundStepType) {
 	}
 	s.Round = round
 	s.Step = step
+
+	if err := s.ProposerSelector.UpdateHeightRound(s.Height, round); err != nil {
+		s.logger.Error("error updating proposer scores",
+			"height", s.Height, "round", round,
+			"err", err)
+	}
 }
 
 // Updates State and increments height to match that of state.
 // The round becomes 0 and cs.Step becomes cstypes.RoundStepNewHeight.
-func (s *StateData) updateToState(state sm.State, commit *types.Commit) {
+func (s *StateData) updateToState(state sm.State, commit *types.Commit, blockStore selectproposer.BlockStore) {
 	if s.CommitRound > -1 && 0 < s.Height && s.Height != state.LastBlockHeight {
 		panic(fmt.Sprintf(
 			"updateToState() expected state height of %v but found %v",
@@ -221,7 +233,6 @@ func (s *StateData) updateToState(state sm.State, commit *types.Commit) {
 	}
 
 	// Reset fields based on state.
-	validators := state.Validators
 
 	switch {
 	case state.LastBlockHeight == 0: // Very first commit should be empty.
@@ -252,11 +263,31 @@ func (s *StateData) updateToState(state sm.State, commit *types.Commit) {
 		height = state.InitialHeight
 	}
 
-	s.logger.Trace("updating state height", "newHeight", height)
+	// state.Validators contain validator set at (state.LastBlockHeight+1, 0)
+	validators := state.Validators
 
-	// RoundState fields
-	s.updateHeight(height)
-	s.updateRoundStep(0, cstypes.RoundStepNewHeight)
+	if s.Validators == nil || !bytes.Equal(s.Validators.QuorumHash, validators.QuorumHash) {
+		s.logger.Info("Updating validators", "from", s.Validators.BasicInfoString(),
+			"to", validators.BasicInfoString())
+	}
+
+	s.Validators = validators
+	var err error
+
+	s.ProposerSelector, err = selectproposer.NewProposerSelector(
+		state.ConsensusParams,
+		s.Validators,
+		height,
+		0,
+		blockStore,
+		s.logger,
+	)
+	if err != nil {
+		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.logger.Trace("updating state height", "newHeight", height)
 
 	if s.CommitTime.IsZero() {
 		// "Now" makes it easier to sync up dev nodes.
@@ -265,12 +296,6 @@ func (s *StateData) updateToState(state sm.State, commit *types.Commit) {
 		s.StartTime = s.CommitTime
 	}
 
-	if s.Validators == nil || !bytes.Equal(s.Validators.QuorumHash, validators.QuorumHash) {
-		s.logger.Info("Updating validators", "from", s.Validators.BasicInfoString(),
-			"to", validators.BasicInfoString())
-	}
-
-	s.Validators = validators
 	s.Proposal = nil
 	s.ProposalReceiveTime = time.Time{}
 	s.ProposalBlock = nil
@@ -288,6 +313,10 @@ func (s *StateData) updateToState(state sm.State, commit *types.Commit) {
 	s.TriggeredTimeoutPrecommit = false
 
 	s.state = state
+
+	// RoundState fields
+	s.updateHeight(height)
+	s.updateRoundStep(0, cstypes.RoundStepNewHeight)
 }
 
 func (s *StateData) updateHeight(height int64) {

internal/consensus/state_enter_new_round.go

@@ -66,18 +66,10 @@ func (c *EnterNewRoundAction) Execute(ctx context.Context, stateEvent StateEvent
 		"round", stateData.Round,
 		"step", stateData.Step)
 
-	// increment validators if necessary
-	validators := stateData.Validators
-	if stateData.Round < round {
-		validators = validators.Copy()
-		validators.IncrementProposerPriority(round - stateData.Round)
-	}
-
 	// Setup new round
 	// we don't fire newStep for this step,
 	// but we fire an event, so update the round step first
 	stateData.updateRoundStep(round, cstypes.RoundStepNewRound)
-	stateData.Validators = validators
 	if round == 0 {
 		// We've already reset these upon new height,
 		// and meanwhile we might have received a proposal