types: ValidatorSet.Update preserves Accum (#2941)

* types: ValidatorSet.Update preserves ProposerPriority

This solves the other issue discovered as part of #2718,
where Accum (now called ProposerPriority) is reset to
0 every time a validator is updated.

* update changelog

* add test

* update comment

* Update types/validator_set_test.go

Co-Authored-By: ebuchman <ethan@coinculture.info>

CHANGELOG_PENDING.md

@@ -12,23 +12,37 @@ program](https://hackerone.com/tendermint).
 ### BREAKING CHANGES:
 
 * CLI/RPC/Config
+  - [rpc] \#2932 Rename `accum` to `proposer_priority`
 
 * Apps
 
 * Go API
-
-- [db] [\#2913](https://github.com/tendermint/tendermint/pull/2913) ReverseIterator API change -- start < end, and end is exclusive.
+  - [db] [\#2913](https://github.com/tendermint/tendermint/pull/2913)
+    ReverseIterator API change -- start < end, and end is exclusive.
+  - [types] \#2932 Rename `Validator.Accum` to `Validator.ProposerPriority`
 
 * Blockchain Protocol
-  * [state] \#2714 Validators can now only use pubkeys allowed within ConsensusParams.ValidatorParams
+  - [state] \#2714 Validators can now only use pubkeys allowed within
+    ConsensusParams.ValidatorParams
 
 * P2P Protocol
+  - [consensus] [\#2871](https://github.com/tendermint/tendermint/issues/2871)
+    Remove *ProposalHeartbeat* message as it serves no real purpose
+  - [state] Fixes for proposer selection:
+    - \#2785 Accum for new validators is `-1.125*totalVotingPower` instead of 0
+    - \#2941 val.Accum is preserved during ValidatorSet.Update to avoid being
+      reset to 0
 
 ### FEATURES:
 
 ### IMPROVEMENTS:
-- [consensus] [\#2871](https://github.com/tendermint/tendermint/issues/2871) Remove *ProposalHeartbeat* infrastructure as it serves no real purpose
 
 ### BUG FIXES:
 - [types] \#2938 Fix regression in v0.26.4 where we panic on empty
   genDoc.Validators
+- [state] \#2785 Fix accum for new validators to be `-1.125*totalVotingPower`
+  instead of 0, forcing them to wait before becoming the proposer. Also:
+    - do not batch clip
+    - keep accums averaged near 0
+- [types] \#2941 Preserve val.Accum during ValidatorSet.Update to avoid it being
+  reset to 0 every time a validator is updated

types/validator_set.go

@@ -80,7 +80,7 @@ func (vals *ValidatorSet) IncrementProposerPriority(times int) {
 
 	const shiftEveryNthIter = 10
 	var proposer *Validator
-	// call IncrementAccum(1) times times:
+	// call IncrementProposerPriority(1) times times:
 	for i := 0; i < times; i++ {
 		shiftByAvgProposerPriority := i%shiftEveryNthIter == 0
 		proposer = vals.incrementProposerPriority(shiftByAvgProposerPriority)
@@ -272,13 +272,22 @@ func (vals *ValidatorSet) Add(val *Validator) (added bool) {
 	}
 }
 
-// Update updates val and returns true. It returns false if val is not present
-// in the set.
+// Update updates the ValidatorSet by copying in the val.
+// If the val is not found, it returns false; otherwise,
+// it returns true. The val.ProposerPriority field is ignored
+// and unchanged by this method.
 func (vals *ValidatorSet) Update(val *Validator) (updated bool) {
 	index, sameVal := vals.GetByAddress(val.Address)
 	if sameVal == nil {
 		return false
 	}
+	// Overwrite the ProposerPriority so it doesn't change.
+	// During block execution, the val passed in here comes
+	// from ABCI via PB2TM.ValidatorUpdates. Since ABCI
+	// doesn't know about ProposerPriority, PB2TM.ValidatorUpdates
+	// uses the default value of 0, which would cause issues for
+	// proposer selection every time a validator's voting power changes.
+	val.ProposerPriority = sameVal.ProposerPriority
 	vals.Validators[index] = val.Copy()
 	// Invalidate cache
 	vals.Proposer = nil

types/validator_set_test.go

@@ -62,8 +62,15 @@ func TestValidatorSetBasic(t *testing.T) {
 
 	// update
 	assert.False(t, vset.Update(randValidator_()))
-	val.VotingPower = 100
+	_, val = vset.GetByAddress(val.Address)
+	val.VotingPower += 100
+	proposerPriority := val.ProposerPriority
+	// Mimic update from types.PB2TM.ValidatorUpdates which does not know about ProposerPriority
+	// and hence defaults to 0.
+	val.ProposerPriority = 0
 	assert.True(t, vset.Update(val))
+	_, val = vset.GetByAddress(val.Address)
+	assert.Equal(t, proposerPriority, val.ProposerPriority)
 
 	// remove
 	val2, removed := vset.Remove(randValidator_().Address)