Set accum of freshly added validator -(total voting power) (#2785)

* set the accum of a new validator to (-total voting power):

- disincentivize validators to unbond, then rebon to reset their
negative Accum to zero

additional unrelated changes:
- do not capitalize error msgs
- fix typo

* review comments: (re)capitalize errors & delete obsolete comments

* More changes suggested by @melekes

* WIP: do not batch clip (#2809)

* substract avgAccum on each iteration

- temporarily skip test

* remove unused method safeMulClip / safeMul

* always substract the avg accum

 - temp. skip another test

* remove overflow / underflow tests & add tests for avgAccum:

- add test for computeAvgAccum
- as we substract the avgAccum now we will not trivially over/underflow

* address @cwgoes' comments

* shift by avg at the end of IncrementAccum

* Add comment to MaxTotalVotingPower

* Guard inputs to not exceed MaxTotalVotingPower

* Address review comments:

 - do not fetch current validator from set again
 - update error message

* Address a few review comments:

 - fix typo
 - extract variable

* address more review comments:

 - clarify 1.125*totalVotingPower == totalVotingPower + (totalVotingPower >> 3)

* review comments: panic instead of "clipping":

 - total voting power is guarded to not exceed MaxTotalVotingPower ->
 panic if this invariant is violated

* fix failing test

lite/doc.go

@@ -121,7 +121,7 @@ If we cannot update directly from H -> H' because there was too much change to
 the validator set, then we can look for some Hm (H < Hm < H') with a validator
 set Vm.  Then we try to update H -> Hm and then Hm -> H' in two steps.  If one
 of these steps doesn't work, then we continue bisecting, until we eventually
-have to externally validate the valdiator set changes at every block.
+have to externally validate the validator set changes at every block.
 
 Since we never trust any server in this protocol, only the signatures
 themselves, it doesn't matter if the seed comes from a (possibly malicious)

state/execution.go

@@ -356,20 +356,48 @@ func updateValidators(currentSet *types.ValidatorSet, updates []*types.Validator
 		address := valUpdate.Address
 		_, val := currentSet.GetByAddress(address)
 		// valUpdate.VotingPower is ensured to be non-negative in validation method
-		if valUpdate.VotingPower == 0 {
-			// remove val
+		if valUpdate.VotingPower == 0 { // remove val
 			_, removed := currentSet.Remove(address)
 			if !removed {
 				return fmt.Errorf("Failed to remove validator %X", address)
 			}
-		} else if val == nil {
-			// add val
+		} else if val == nil { // add val
+			// make sure we do not exceed MaxTotalVotingPower by adding this validator:
+			totalVotingPower := currentSet.TotalVotingPower()
+			updatedVotingPower := valUpdate.VotingPower + totalVotingPower
+			overflow := updatedVotingPower > types.MaxTotalVotingPower || updatedVotingPower < 0
+			if overflow {
+				return fmt.Errorf(
+					"Failed to add new validator %v. Adding it would exceed max allowed total voting power %v",
+					valUpdate,
+					types.MaxTotalVotingPower)
+			}
+			// TODO: issue #1558 update spec according to the following:
+			// Set Accum to -C*totalVotingPower (with C ~= 1.125) to make sure validators can't
+			// unbond/rebond to reset their (potentially previously negative) Accum to zero.
+			//
+			// Contract: totalVotingPower < MaxTotalVotingPower to ensure Accum does
+			// not exceed the bounds of int64.
+			//
+			// Compute Accum = -1.125*totalVotingPower == -(totalVotingPower + (totalVotingPower >> 3)).
+			valUpdate.Accum = -(totalVotingPower + (totalVotingPower >> 3))
 			added := currentSet.Add(valUpdate)
 			if !added {
 				return fmt.Errorf("Failed to add new validator %v", valUpdate)
 			}
-		} else {
-			// update val
+		} else { // update val
+			// make sure we do not exceed MaxTotalVotingPower by updating this validator:
+			totalVotingPower := currentSet.TotalVotingPower()
+			curVotingPower := val.VotingPower
+			updatedVotingPower := totalVotingPower - curVotingPower + valUpdate.VotingPower
+			overflow := updatedVotingPower > types.MaxTotalVotingPower || updatedVotingPower < 0
+			if overflow {
+				return fmt.Errorf(
+					"Failed to update existing validator %v. Updating it would exceed max allowed total voting power %v",
+					valUpdate,
+					types.MaxTotalVotingPower)
+			}
+
 			updated := currentSet.Update(valUpdate)
 			if !updated {
 				return fmt.Errorf("Failed to update validator %X to %v", address, valUpdate)

types/signed_msg_type.go

@@ -10,7 +10,6 @@ const (
 
 	// Proposals
 	ProposalType SignedMsgType = 0x20
-
 )
 
 // IsVoteTypeValid returns true if t is a valid vote type.

types/validator.go

@@ -81,13 +81,13 @@ func (v *Validator) Hash() []byte {
 // as its redundant with the pubkey. This also excludes accum which changes
 // every round.
 func (v *Validator) Bytes() []byte {
-	return cdcEncode((struct {
+	return cdcEncode(struct {
 		PubKey      crypto.PubKey
 		VotingPower int64
 	}{
 		v.PubKey,
 		v.VotingPower,
-	}))
+	})
 }
 
 //----------------------------------------

types/validator_set.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"math"
+	"math/big"
 	"sort"
 	"strings"
 
@@ -11,6 +12,15 @@ import (
 	cmn "github.com/tendermint/tendermint/libs/common"
 )
 
+// The maximum allowed total voting power.
+// We set the accum of freshly added validators to -1.125*totalVotingPower.
+// To compute 1.125*totalVotingPower efficiently, we do:
+// totalVotingPower + (totalVotingPower >> 3) because
+// x + (x >> 3) = x + x/8 = x * (1 + 0.125).
+// MaxTotalVotingPower is the largest int64 `x` with the property that `x + (x >> 3)` is
+// still in the bounds of int64.
+const MaxTotalVotingPower = 8198552921648689607
+
 // ValidatorSet represent a set of *Validator at a given height.
 // The validators can be fetched by address or index.
 // The index is in order of .Address, so the indices are fixed
@@ -68,25 +78,64 @@ func (vals *ValidatorSet) IncrementAccum(times int) {
 		panic("Cannot call IncrementAccum with non-positive times")
 	}
 
-	// Add VotingPower * times to each validator and order into heap.
+	const shiftEveryNthIter = 10
+	var proposer *Validator
+	// call IncrementAccum(1) times times:
+	for i := 0; i < times; i++ {
+		shiftByAvgAccum := i%shiftEveryNthIter == 0
+		proposer = vals.incrementAccum(shiftByAvgAccum)
+	}
+	isShiftedAvgOnLastIter := (times-1)%shiftEveryNthIter == 0
+	if !isShiftedAvgOnLastIter {
 		validatorsHeap := cmn.NewHeap()
-	for _, val := range vals.Validators {
-		// Check for overflow both multiplication and sum.
-		val.Accum = safeAddClip(val.Accum, safeMulClip(val.VotingPower, int64(times)))
-		validatorsHeap.PushComparable(val, accumComparable{val})
+		vals.shiftByAvgAccum(validatorsHeap)
+	}
+	vals.Proposer = proposer
 }
 
-	// Decrement the validator with most accum times times.
-	for i := 0; i < times; i++ {
+func (vals *ValidatorSet) incrementAccum(subAvg bool) *Validator {
+	for _, val := range vals.Validators {
+		// Check for overflow for sum.
+		val.Accum = safeAddClip(val.Accum, val.VotingPower)
+	}
+
+	validatorsHeap := cmn.NewHeap()
+	if subAvg { // shift by avg accum
+		vals.shiftByAvgAccum(validatorsHeap)
+	} else { // just update the heap
+		for _, val := range vals.Validators {
+			validatorsHeap.PushComparable(val, accumComparable{val})
+		}
+	}
+
+	// Decrement the validator with most accum:
 	mostest := validatorsHeap.Peek().(*Validator)
 	// mind underflow
 	mostest.Accum = safeSubClip(mostest.Accum, vals.TotalVotingPower())
 
-		if i == times-1 {
-			vals.Proposer = mostest
-		} else {
-			validatorsHeap.Update(mostest, accumComparable{mostest})
+	return mostest
 }
+
+func (vals *ValidatorSet) computeAvgAccum() int64 {
+	n := int64(len(vals.Validators))
+	sum := big.NewInt(0)
+	for _, val := range vals.Validators {
+		sum.Add(sum, big.NewInt(val.Accum))
+	}
+	avg := sum.Div(sum, big.NewInt(n))
+	if avg.IsInt64() {
+		return avg.Int64()
+	}
+
+	// this should never happen: each val.Accum is in bounds of int64
+	panic(fmt.Sprintf("Cannot represent avg accum as an int64 %v", avg))
+}
+
+func (vals *ValidatorSet) shiftByAvgAccum(validatorsHeap *cmn.Heap) {
+	avgAccum := vals.computeAvgAccum()
+	for _, val := range vals.Validators {
+		val.Accum = safeSubClip(val.Accum, avgAccum)
+		validatorsHeap.PushComparable(val, accumComparable{val})
 	}
 }
 
@@ -144,10 +193,18 @@ func (vals *ValidatorSet) Size() int {
 // TotalVotingPower returns the sum of the voting powers of all validators.
 func (vals *ValidatorSet) TotalVotingPower() int64 {
 	if vals.totalVotingPower == 0 {
+		sum := int64(0)
 		for _, val := range vals.Validators {
 			// mind overflow
-			vals.totalVotingPower = safeAddClip(vals.totalVotingPower, val.VotingPower)
+			sum = safeAddClip(sum, val.VotingPower)
 		}
+		if sum > MaxTotalVotingPower {
+			panic(fmt.Sprintf(
+				"Total voting power should be guarded to not exceed %v; got: %v",
+				MaxTotalVotingPower,
+				sum))
+		}
+		vals.totalVotingPower = sum
 	}
 	return vals.totalVotingPower
 }
@@ -308,7 +365,7 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, height i
 		return nil
 	}
 	return fmt.Errorf("Invalid commit -- insufficient voting power: got %v, needed %v",
-		talliedVotingPower, (vals.TotalVotingPower()*2/3 + 1))
+		talliedVotingPower, vals.TotalVotingPower()*2/3+1)
 }
 
 // VerifyFutureCommit will check to see if the set would be valid with a different
@@ -391,7 +448,7 @@ func (vals *ValidatorSet) VerifyFutureCommit(newSet *ValidatorSet, chainID strin
 
 	if oldVotingPower <= oldVals.TotalVotingPower()*2/3 {
 		return cmn.NewError("Invalid commit -- insufficient old voting power: got %v, needed %v",
-			oldVotingPower, (oldVals.TotalVotingPower()*2/3 + 1))
+			oldVotingPower, oldVals.TotalVotingPower()*2/3+1)
 	}
 	return nil
 }
@@ -405,7 +462,7 @@ func (vals *ValidatorSet) StringIndented(indent string) string {
 	if vals == nil {
 		return "nil-ValidatorSet"
 	}
-	valStrings := []string{}
+	var valStrings []string
 	vals.Iterate(func(index int, val *Validator) bool {
 		valStrings = append(valStrings, val.String())
 		return false
@@ -476,24 +533,7 @@ func RandValidatorSet(numValidators int, votingPower int64) (*ValidatorSet, []Pr
 }
 
 ///////////////////////////////////////////////////////////////////////////////
-// Safe multiplication and addition/subtraction
-
-func safeMul(a, b int64) (int64, bool) {
-	if a == 0 || b == 0 {
-		return 0, false
-	}
-	if a == 1 {
-		return b, false
-	}
-	if b == 1 {
-		return a, false
-	}
-	if a == math.MinInt64 || b == math.MinInt64 {
-		return -1, true
-	}
-	c := a * b
-	return c, c/b != a
-}
+// Safe addition/subtraction
 
 func safeAdd(a, b int64) (int64, bool) {
 	if b > 0 && a > math.MaxInt64-b {
@@ -513,17 +553,6 @@ func safeSub(a, b int64) (int64, bool) {
 	return a - b, false
 }
 
-func safeMulClip(a, b int64) int64 {
-	c, overflow := safeMul(a, b)
-	if overflow {
-		if (a < 0 || b < 0) && !(a < 0 && b < 0) {
-			return math.MinInt64
-		}
-		return math.MaxInt64
-	}
-	return c
-}
-
 func safeAddClip(a, b int64) int64 {
 	c, overflow := safeAdd(a, b)
 	if overflow {

types/validator_set_test.go

@@ -128,7 +128,7 @@ func TestProposerSelection1(t *testing.T) {
 		newValidator([]byte("bar"), 300),
 		newValidator([]byte("baz"), 330),
 	})
-	proposers := []string{}
+	var proposers []string
 	for i := 0; i < 99; i++ {
 		val := vset.GetProposer()
 		proposers = append(proposers, string(val.Address))
@@ -305,53 +305,37 @@ func (valSet *ValidatorSet) fromBytes(b []byte) {
 
 //-------------------------------------------------------------------
 
-func TestValidatorSetTotalVotingPowerOverflows(t *testing.T) {
-	vset := NewValidatorSet([]*Validator{
+func TestValidatorSetTotalVotingPowerPanicsOnOverflow(t *testing.T) {
+	// NewValidatorSet calls IncrementAccum which calls TotalVotingPower()
+	// which should panic on overflows:
+	shouldPanic := func() {
+		NewValidatorSet([]*Validator{
 			{Address: []byte("a"), VotingPower: math.MaxInt64, Accum: 0},
 			{Address: []byte("b"), VotingPower: math.MaxInt64, Accum: 0},
 			{Address: []byte("c"), VotingPower: math.MaxInt64, Accum: 0},
 		})
-
-	assert.EqualValues(t, math.MaxInt64, vset.TotalVotingPower())
 	}
 
-func TestValidatorSetIncrementAccumOverflows(t *testing.T) {
-	// NewValidatorSet calls IncrementAccum(1)
-	vset := NewValidatorSet([]*Validator{
-		// too much voting power
-		0: {Address: []byte("a"), VotingPower: math.MaxInt64, Accum: 0},
-		// too big accum
-		1: {Address: []byte("b"), VotingPower: 10, Accum: math.MaxInt64},
-		// almost too big accum
-		2: {Address: []byte("c"), VotingPower: 10, Accum: math.MaxInt64 - 5},
-	})
-
-	assert.Equal(t, int64(0), vset.Validators[0].Accum, "0") // because we decrement val with most voting power
-	assert.EqualValues(t, math.MaxInt64, vset.Validators[1].Accum, "1")
-	assert.EqualValues(t, math.MaxInt64, vset.Validators[2].Accum, "2")
+	assert.Panics(t, shouldPanic)
 }
 
-func TestValidatorSetIncrementAccumUnderflows(t *testing.T) {
-	// NewValidatorSet calls IncrementAccum(1)
-	vset := NewValidatorSet([]*Validator{
-		0: {Address: []byte("a"), VotingPower: math.MaxInt64, Accum: math.MinInt64},
-		1: {Address: []byte("b"), VotingPower: 1, Accum: math.MinInt64},
-	})
-
-	vset.IncrementAccum(5)
-
-	assert.EqualValues(t, math.MinInt64, vset.Validators[0].Accum, "0")
-	assert.EqualValues(t, math.MinInt64, vset.Validators[1].Accum, "1")
+func TestAvgAccum(t *testing.T) {
+	// Create Validator set without calling IncrementAccum:
+	tcs := []struct {
+		vs   ValidatorSet
+		want int64
+	}{
+		0: {ValidatorSet{Validators: []*Validator{{Accum: 0}, {Accum: 0}, {Accum: 0}}}, 0},
+		1: {ValidatorSet{Validators: []*Validator{{Accum: math.MaxInt64}, {Accum: 0}, {Accum: 0}}}, math.MaxInt64 / 3},
+		2: {ValidatorSet{Validators: []*Validator{{Accum: math.MaxInt64}, {Accum: 0}}}, math.MaxInt64 / 2},
+		3: {ValidatorSet{Validators: []*Validator{{Accum: math.MaxInt64}, {Accum: math.MaxInt64}}}, math.MaxInt64},
+		4: {ValidatorSet{Validators: []*Validator{{Accum: math.MinInt64}, {Accum: math.MinInt64}}}, math.MinInt64},
+	}
+	for i, tc := range tcs {
+		got := tc.vs.computeAvgAccum()
+		assert.Equal(t, tc.want, got, "test case: %v", i)
 	}
 
-func TestSafeMul(t *testing.T) {
-	f := func(a, b int64) bool {
-		c, overflow := safeMul(a, b)
-		return overflow || (!overflow && c == a*b)
-	}
-	if err := quick.Check(f, nil); err != nil {
-		t.Error(err)
-	}
 }
 
 func TestSafeAdd(t *testing.T) {
@@ -364,13 +348,6 @@ func TestSafeAdd(t *testing.T) {
 	}
 }
 
-func TestSafeMulClip(t *testing.T) {
-	assert.EqualValues(t, math.MaxInt64, safeMulClip(math.MinInt64, math.MinInt64))
-	assert.EqualValues(t, math.MinInt64, safeMulClip(math.MaxInt64, math.MinInt64))
-	assert.EqualValues(t, math.MinInt64, safeMulClip(math.MinInt64, math.MaxInt64))
-	assert.EqualValues(t, math.MaxInt64, safeMulClip(math.MaxInt64, 2))
-}
-
 func TestSafeAddClip(t *testing.T) {
 	assert.EqualValues(t, math.MaxInt64, safeAddClip(math.MaxInt64, 10))
 	assert.EqualValues(t, math.MaxInt64, safeAddClip(math.MaxInt64, math.MaxInt64))