Refactor updateState #2865 (#2929)

* Refactor updateState #2865

* Apply suggestions from code review

Co-Authored-By: danil-lashin <danil-lashin@yandex.ru>

* Apply suggestions from code review

state/execution.go

@@ -107,8 +107,18 @@ func (blockExec *BlockExecutor) ApplyBlock(state State, blockID types.BlockID, b
 
 	fail.Fail() // XXX
 
+	// these are tendermint types now
+	validatorUpdates, err := types.PB2TM.ValidatorUpdates(abciResponses.EndBlock.ValidatorUpdates)
+	if err != nil {
+		return state, err
+	}
+
+	if len(validatorUpdates) > 0 {
+		blockExec.logger.Info("Updates to validators", "updates", makeValidatorUpdatesLogString(validatorUpdates))
+	}
+
 	// Update the state with the block and responses.
-	state, err = updateState(blockExec.logger, state, blockID, &block.Header, abciResponses)
+	state, err = updateState(state, blockID, &block.Header, abciResponses, validatorUpdates)
 	if err != nil {
 		return state, fmt.Errorf("Commit failed for application: %v", err)
 	}
@@ -132,7 +142,7 @@ func (blockExec *BlockExecutor) ApplyBlock(state State, blockID types.BlockID, b
 
 	// Events are fired after everything else.
 	// NOTE: if we crash between Commit and Save, events wont be fired during replay
-	fireEvents(blockExec.logger, blockExec.eventBus, block, abciResponses)
+	fireEvents(blockExec.logger, blockExec.eventBus, block, abciResponses, validatorUpdates)
 
 	return state, nil
 }
@@ -311,16 +321,10 @@ func getBeginBlockValidatorInfo(block *types.Block, lastValSet *types.ValidatorS
 // If more or equal than 1/3 of total voting power changed in one block, then
 // a light client could never prove the transition externally. See
 // ./lite/doc.go for details on how a light client tracks validators.
-func updateValidators(currentSet *types.ValidatorSet, abciUpdates []abci.ValidatorUpdate) ([]*types.Validator, error) {
+func updateValidators(currentSet *types.ValidatorSet, updates []*types.Validator) error {
-	updates, err := types.PB2TM.ValidatorUpdates(abciUpdates)
-	if err != nil {
-		return nil, err
-	}
-
-	// these are tendermint types now
 	for _, valUpdate := range updates {
 		if valUpdate.VotingPower < 0 {
-			return nil, fmt.Errorf("Voting power can't be negative %v", valUpdate)
+			return fmt.Errorf("Voting power can't be negative %v", valUpdate)
 		}
 
 		address := valUpdate.Address
@@ -329,32 +333,32 @@ func updateValidators(currentSet *types.ValidatorSet, abciUpdates []abci.Validat
 			// remove val
 			_, removed := currentSet.Remove(address)
 			if !removed {
-				return nil, fmt.Errorf("Failed to remove validator %X", address)
+				return fmt.Errorf("Failed to remove validator %X", address)
 			}
 		} else if val == nil {
 			// add val
 			added := currentSet.Add(valUpdate)
 			if !added {
-				return nil, fmt.Errorf("Failed to add new validator %v", valUpdate)
+				return fmt.Errorf("Failed to add new validator %v", valUpdate)
 			}
 		} else {
 			// update val
 			updated := currentSet.Update(valUpdate)
 			if !updated {
-				return nil, fmt.Errorf("Failed to update validator %X to %v", address, valUpdate)
+				return fmt.Errorf("Failed to update validator %X to %v", address, valUpdate)
 			}
 		}
 	}
-	return updates, nil
+	return nil
 }
 
 // updateState returns a new State updated according to the header and responses.
 func updateState(
-	logger log.Logger,
 	state State,
 	blockID types.BlockID,
 	header *types.Header,
 	abciResponses *ABCIResponses,
+	validatorUpdates []*types.Validator,
 ) (State, error) {
 
 	// Copy the valset so we can apply changes from EndBlock
@@ -364,14 +368,12 @@ func updateState(
 	// Update the validator set with the latest abciResponses.
 	lastHeightValsChanged := state.LastHeightValidatorsChanged
 	if len(abciResponses.EndBlock.ValidatorUpdates) > 0 {
-		validatorUpdates, err := updateValidators(nValSet, abciResponses.EndBlock.ValidatorUpdates)
+		err := updateValidators(nValSet, validatorUpdates)
 		if err != nil {
 			return state, fmt.Errorf("Error changing validator set: %v", err)
 		}
 		// Change results from this height but only applies to the next next height.
 		lastHeightValsChanged = header.Height + 1 + 1
-
-		logger.Info("Updates to validators", "updates", makeValidatorUpdatesLogString(validatorUpdates))
 	}
 
 	// Update validator accums and set state variables.
@@ -417,7 +419,7 @@ func updateState(
 // Fire NewBlock, NewBlockHeader.
 // Fire TxEvent for every tx.
 // NOTE: if Tendermint crashes before commit, some or all of these events may be published again.
-func fireEvents(logger log.Logger, eventBus types.BlockEventPublisher, block *types.Block, abciResponses *ABCIResponses) {
+func fireEvents(logger log.Logger, eventBus types.BlockEventPublisher, block *types.Block, abciResponses *ABCIResponses, validatorUpdates []*types.Validator) {
 	eventBus.PublishEventNewBlock(types.EventDataNewBlock{
 		Block:            block,
 		ResultBeginBlock: *abciResponses.BeginBlock,
@@ -438,12 +440,9 @@ func fireEvents(logger log.Logger, eventBus types.BlockEventPublisher, block *ty
 		}})
 	}
 
-	abciValUpdates := abciResponses.EndBlock.ValidatorUpdates
+	if len(validatorUpdates) > 0 {
-	if len(abciValUpdates) > 0 {
-		// if there were an error, we would've stopped in updateValidators
-		updates, _ := types.PB2TM.ValidatorUpdates(abciValUpdates)
 		eventBus.PublishEventValidatorSetUpdates(
-			types.EventDataValidatorSetUpdates{ValidatorUpdates: updates})
+			types.EventDataValidatorSetUpdates{ValidatorUpdates: validatorUpdates})
 	}
 }
 

state/execution_test.go

@@ -218,7 +218,9 @@ func TestUpdateValidators(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			_, err := updateValidators(tc.currentSet, tc.abciUpdates)
+			updates, err := types.PB2TM.ValidatorUpdates(tc.abciUpdates)
+			assert.NoError(t, err)
+			err = updateValidators(tc.currentSet, updates)
 			if tc.shouldErr {
 				assert.Error(t, err)
 			} else {

state/state_test.go

@@ -5,12 +5,11 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/tendermint/tendermint/libs/log"
-
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
 	abci "github.com/tendermint/tendermint/abci/types"
-	crypto "github.com/tendermint/tendermint/crypto"
+	"github.com/tendermint/tendermint/crypto"
 	"github.com/tendermint/tendermint/crypto/ed25519"
 	cmn "github.com/tendermint/tendermint/libs/common"
 	dbm "github.com/tendermint/tendermint/libs/db"
@@ -223,6 +222,7 @@ func TestOneValidatorChangesSaveLoad(t *testing.T) {
 	_, val := state.Validators.GetByIndex(0)
 	power := val.VotingPower
 	var err error
+	var validatorUpdates []*types.Validator
 	for i := int64(1); i < highestHeight; i++ {
 		// When we get to a change height, use the next pubkey.
 		if changeIndex < len(changeHeights) && i == changeHeights[changeIndex] {
@@ -230,8 +230,10 @@ func TestOneValidatorChangesSaveLoad(t *testing.T) {
 			power++
 		}
 		header, blockID, responses := makeHeaderPartsResponsesValPowerChange(state, i, power)
-		state, err = updateState(log.TestingLogger(), state, blockID, &header, responses)
+		validatorUpdates, err = types.PB2TM.ValidatorUpdates(responses.EndBlock.ValidatorUpdates)
-		assert.Nil(t, err)
+		require.NoError(t, err)
+		state, err = updateState(state, blockID, &header, responses, validatorUpdates)
+		require.NoError(t, err)
 		nextHeight := state.LastBlockHeight + 1
 		saveValidatorsInfo(stateDB, nextHeight+1, state.LastHeightValidatorsChanged, state.NextValidators)
 	}
@@ -303,7 +305,10 @@ func TestManyValidatorChangesSaveLoad(t *testing.T) {
 
 	// Save state etc.
 	var err error
-	state, err = updateState(log.TestingLogger(), state, blockID, &header, responses)
+	var validatorUpdates []*types.Validator
+	validatorUpdates, err = types.PB2TM.ValidatorUpdates(responses.EndBlock.ValidatorUpdates)
+	require.NoError(t, err)
+	state, err = updateState(state, blockID, &header, responses, validatorUpdates)
 	require.Nil(t, err)
 	nextHeight := state.LastBlockHeight + 1
 	saveValidatorsInfo(stateDB, nextHeight+1, state.LastHeightValidatorsChanged, state.NextValidators)
@@ -375,6 +380,7 @@ func TestConsensusParamsChangesSaveLoad(t *testing.T) {
 	changeIndex := 0
 	cp := params[changeIndex]
 	var err error
+	var validatorUpdates []*types.Validator
 	for i := int64(1); i < highestHeight; i++ {
 		// When we get to a change height, use the next params.
 		if changeIndex < len(changeHeights) && i == changeHeights[changeIndex] {
@@ -382,7 +388,9 @@ func TestConsensusParamsChangesSaveLoad(t *testing.T) {
 			cp = params[changeIndex]
 		}
 		header, blockID, responses := makeHeaderPartsResponsesParams(state, i, cp)
-		state, err = updateState(log.TestingLogger(), state, blockID, &header, responses)
+		validatorUpdates, err = types.PB2TM.ValidatorUpdates(responses.EndBlock.ValidatorUpdates)
+		require.NoError(t, err)
+		state, err = updateState(state, blockID, &header, responses, validatorUpdates)
 
 		require.Nil(t, err)
 		nextHeight := state.LastBlockHeight + 1