IncrementAccum upon RPC /validators; Sanity checks and comments (#2808)

CHANGELOG_PENDING.md

@@ -30,3 +30,5 @@ program](https://hackerone.com/tendermint).
 - [mempool] \#2835 Remove local int64 counter from being stored in every tx
 
 ### BUG FIXES:
+
+- [rpc] \#2808 RPC validators calls IncrementAccum if necessary

state/state.go

@@ -64,7 +64,8 @@ type State struct {
 	// Validators are persisted to the database separately every time they change,
 	// so we can query for historical validator sets.
 	// Note that if s.LastBlockHeight causes a valset change,
-	// we set s.LastHeightValidatorsChanged = s.LastBlockHeight + 1
+	// we set s.LastHeightValidatorsChanged = s.LastBlockHeight + 1 + 1
+	// Extra +1 due to nextValSet delay.
 	NextValidators              *types.ValidatorSet
 	Validators                  *types.ValidatorSet
 	LastValidators              *types.ValidatorSet

state/state_test.go

@@ -3,9 +3,10 @@ package state
 import (
 	"bytes"
 	"fmt"
-	"github.com/tendermint/tendermint/libs/log"
 	"testing"
 
+	"github.com/tendermint/tendermint/libs/log"
+
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	abci "github.com/tendermint/tendermint/abci/types"
@@ -260,6 +261,27 @@ func TestOneValidatorChangesSaveLoad(t *testing.T) {
 	}
 }
 
+func TestStoreLoadValidatorsIncrementsAccum(t *testing.T) {
+	const valSetSize = 2
+	tearDown, stateDB, state := setupTestCase(t)
+	state.Validators = genValSet(valSetSize)
+	state.NextValidators = state.Validators.CopyIncrementAccum(1)
+	SaveState(stateDB, state)
+	defer tearDown(t)
+
+	nextHeight := state.LastBlockHeight + 1
+
+	v0, err := LoadValidators(stateDB, nextHeight)
+	assert.Nil(t, err)
+	acc0 := v0.Validators[0].Accum
+
+	v1, err := LoadValidators(stateDB, nextHeight+1)
+	assert.Nil(t, err)
+	acc1 := v1.Validators[0].Accum
+
+	assert.NotEqual(t, acc1, acc0, "expected Accum value to change between heights")
+}
+
 // TestValidatorChangesSaveLoad tests saving and loading a validator set with
 // changes.
 func TestManyValidatorChangesSaveLoad(t *testing.T) {

state/store.go

@@ -89,7 +89,9 @@ func saveState(db dbm.DB, state State, key []byte) {
 	nextHeight := state.LastBlockHeight + 1
 	// If first block, save validators for block 1.
 	if nextHeight == 1 {
-		lastHeightVoteChanged := int64(1) // Due to Tendermint validator set changes being delayed 1 block.
+		// This extra logic due to Tendermint validator set changes being delayed 1 block.
+		// It may get overwritten due to InitChain validator updates.
+		lastHeightVoteChanged := int64(1)
 		saveValidatorsInfo(db, nextHeight, lastHeightVoteChanged, state.Validators)
 	}
 	// Save next validators.
@@ -191,12 +193,14 @@ func LoadValidators(db dbm.DB, height int64) (*types.ValidatorSet, error) {
 				),
 			)
 		}
+		valInfo2.ValidatorSet.IncrementAccum(int(height - valInfo.LastHeightChanged)) // mutate
 		valInfo = valInfo2
 	}
 
 	return valInfo.ValidatorSet, nil
 }
 
+// CONTRACT: Returned ValidatorsInfo can be mutated.
 func loadValidatorsInfo(db dbm.DB, height int64) *ValidatorsInfo {
 	buf := db.Get(calcValidatorsKey(height))
 	if len(buf) == 0 {
@@ -215,18 +219,22 @@ func loadValidatorsInfo(db dbm.DB, height int64) *ValidatorsInfo {
 	return v
 }
 
-// saveValidatorsInfo persists the validator set for the next block to disk.
+// saveValidatorsInfo persists the validator set.
+// `height` is the effective height for which the validator is responsible for signing.
 // It should be called from s.Save(), right before the state itself is persisted.
 // If the validator set did not change after processing the latest block,
 // only the last height for which the validators changed is persisted.
-func saveValidatorsInfo(db dbm.DB, nextHeight, changeHeight int64, valSet *types.ValidatorSet) {
+func saveValidatorsInfo(db dbm.DB, height, lastHeightChanged int64, valSet *types.ValidatorSet) {
+	if lastHeightChanged > height {
+		panic("LastHeightChanged cannot be greater than ValidatorsInfo height")
+	}
 	valInfo := &ValidatorsInfo{
-		LastHeightChanged: changeHeight,
+		LastHeightChanged: lastHeightChanged,
 	}
-	if changeHeight == nextHeight {
+	if lastHeightChanged == height {
 		valInfo.ValidatorSet = valSet
 	}
-	db.Set(calcValidatorsKey(nextHeight), valInfo.Bytes())
+	db.Set(calcValidatorsKey(height), valInfo.Bytes())
 }
 
 //-----------------------------------------------------------------------------

types/validator_set.go

@@ -62,7 +62,11 @@ func (vals *ValidatorSet) CopyIncrementAccum(times int) *ValidatorSet {
 
 // IncrementAccum increments accum of each validator and updates the
 // proposer. Panics if validator set is empty.
+// `times` must be positive.
 func (vals *ValidatorSet) IncrementAccum(times int) {
+	if times <= 0 {
+		panic("Cannot call IncrementAccum with non-positive times")
+	}
 
 	// Add VotingPower * times to each validator and order into heap.
 	validatorsHeap := cmn.NewHeap()

types/validator_set_test.go

@@ -86,6 +86,19 @@ func TestCopy(t *testing.T) {
 	}
 }
 
+// Test that IncrementAccum requires positive times.
+func TestIncrementAccumPositiveTimes(t *testing.T) {
+	vset := NewValidatorSet([]*Validator{
+		newValidator([]byte("foo"), 1000),
+		newValidator([]byte("bar"), 300),
+		newValidator([]byte("baz"), 330),
+	})
+
+	assert.Panics(t, func() { vset.IncrementAccum(-1) })
+	assert.Panics(t, func() { vset.IncrementAccum(0) })
+	vset.IncrementAccum(1)
+}
+
 func BenchmarkValidatorSetCopy(b *testing.B) {
 	b.StopTimer()
 	vset := NewValidatorSet([]*Validator{})
@@ -239,7 +252,7 @@ func TestProposerSelection3(t *testing.T) {
 		mod := (cmn.RandInt() % 5) + 1
 		if cmn.RandInt()%mod > 0 {
 			// sometimes its up to 5
-			times = cmn.RandInt() % 5
+			times = (cmn.RandInt() % 4) + 1
 		}
 		vset.IncrementAccum(times)