From e8b0458f16747a5c1cc5eea361da945241adb105 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 21 Dec 2017 14:04:05 -0600 Subject: [PATCH 1/3] check for overflow and underflow while choosing proposer Refs #919 --- types/validator_set.go | 45 +++++++++++++++++++++++++++++++++---- types/validator_set_test.go | 44 ++++++++++++++++++++++++++++++++++-- 2 files changed, 83 insertions(+), 6 deletions(-) diff --git a/types/validator_set.go b/types/validator_set.go index 134e4e06e..9aaa6830d 100644 --- a/types/validator_set.go +++ b/types/validator_set.go @@ -48,12 +48,17 @@ func NewValidatorSet(vals []*Validator) *ValidatorSet { } // incrementAccum and update the proposer -// TODO: mind the overflow when times and votingPower shares too large. func (valSet *ValidatorSet) IncrementAccum(times int) { // Add VotingPower * times to each validator and order into heap. validatorsHeap := cmn.NewHeap() for _, val := range valSet.Validators { - val.Accum += val.VotingPower * int64(times) // TODO: mind overflow + res, overflow := signedMulWithOverflowCheck(val.VotingPower, int64(times)) + // check for overflow both multiplication and sum + if !overflow && val.Accum <= mostPositive-res { + val.Accum += res + } else { + val.Accum = mostPositive + } validatorsHeap.Push(val, accumComparable{val}) } @@ -63,7 +68,13 @@ func (valSet *ValidatorSet) IncrementAccum(times int) { if i == times-1 { valSet.Proposer = mostest } - mostest.Accum -= int64(valSet.TotalVotingPower()) + + // mind underflow + if mostest.Accum >= mostNegative+valSet.TotalVotingPower() { + mostest.Accum -= valSet.TotalVotingPower() + } else { + mostest.Accum = mostNegative + } validatorsHeap.Update(mostest, accumComparable{mostest}) } } @@ -117,7 +128,13 @@ func (valSet *ValidatorSet) Size() int { func (valSet *ValidatorSet) TotalVotingPower() int64 { if valSet.totalVotingPower == 0 { for _, val := range valSet.Validators { - valSet.totalVotingPower += val.VotingPower + // mind overflow + if valSet.totalVotingPower <= mostPositive-val.VotingPower { + valSet.totalVotingPower += val.VotingPower + } else { + valSet.totalVotingPower = mostPositive + return valSet.totalVotingPower + } } } return valSet.totalVotingPower @@ -425,3 +442,23 @@ func RandValidatorSet(numValidators int, votingPower int64) (*ValidatorSet, []*P sort.Sort(PrivValidatorsByAddress(privValidators)) return valSet, privValidators } + +const mostNegative int64 = -mostPositive - 1 +const mostPositive int64 = 1<<63 - 1 + +func signedMulWithOverflowCheck(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 == mostNegative || b == mostNegative { + return -1, true + } + c := a * b + return c, c/b != a +} diff --git a/types/validator_set_test.go b/types/validator_set_test.go index 572b7b007..c65f507fa 100644 --- a/types/validator_set_test.go +++ b/types/validator_set_test.go @@ -5,8 +5,9 @@ import ( "strings" "testing" - "github.com/tendermint/go-crypto" - "github.com/tendermint/go-wire" + "github.com/stretchr/testify/assert" + crypto "github.com/tendermint/go-crypto" + wire "github.com/tendermint/go-wire" cmn "github.com/tendermint/tmlibs/common" ) @@ -190,6 +191,45 @@ func TestProposerSelection3(t *testing.T) { } } +func TestValidatorSetIncrementAccumOverflows(t *testing.T) { + // NewValidatorSet calls IncrementAccum(1) + vset := NewValidatorSet([]*Validator{ + // too much voting power + 0: {Address: []byte("a"), VotingPower: mostPositive, Accum: 0}, + // too big accum + 1: {Address: []byte("b"), VotingPower: 10, Accum: mostPositive}, + // almost too big accum + 2: {Address: []byte("c"), VotingPower: 10, Accum: mostPositive - 5}, + }) + + assert.Equal(t, int64(0), vset.Validators[0].Accum, "0") // because we decrement val with most voting power + assert.Equal(t, mostPositive, vset.Validators[1].Accum, "1") + assert.Equal(t, mostPositive, vset.Validators[2].Accum, "2") +} + +func TestValidatorSetIncrementAccumUnderflows(t *testing.T) { + // NewValidatorSet calls IncrementAccum(1) + vset := NewValidatorSet([]*Validator{ + 0: {Address: []byte("a"), VotingPower: mostPositive, Accum: mostNegative}, + 1: {Address: []byte("b"), VotingPower: 1, Accum: mostNegative}, + }) + + vset.IncrementAccum(5) + + assert.Equal(t, mostNegative, vset.Validators[0].Accum, "0") + assert.Equal(t, mostNegative, vset.Validators[1].Accum, "1") +} + +func TestValidatorSetTotalVotingPowerOverflows(t *testing.T) { + vset := NewValidatorSet([]*Validator{ + {Address: []byte("a"), VotingPower: mostPositive, Accum: 0}, + {Address: []byte("b"), VotingPower: mostPositive, Accum: 0}, + {Address: []byte("c"), VotingPower: mostPositive, Accum: 0}, + }) + + assert.Equal(t, mostPositive, vset.TotalVotingPower()) +} + func BenchmarkValidatorSetCopy(b *testing.B) { b.StopTimer() vset := NewValidatorSet([]*Validator{}) From 69c3a7640bc48957ed3984596da276264b3f1038 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 25 Dec 2017 18:38:35 -0600 Subject: [PATCH 2/3] add safeAdd & safeSub plus quickcheck tests --- types/validator_set.go | 39 +++++++++++++++++++++++++++++-------- types/validator_set_test.go | 35 ++++++++++++++++++++++++++------- 2 files changed, 59 insertions(+), 15 deletions(-) diff --git a/types/validator_set.go b/types/validator_set.go index 9aaa6830d..a9b986590 100644 --- a/types/validator_set.go +++ b/types/validator_set.go @@ -52,10 +52,15 @@ func (valSet *ValidatorSet) IncrementAccum(times int) { // Add VotingPower * times to each validator and order into heap. validatorsHeap := cmn.NewHeap() for _, val := range valSet.Validators { - res, overflow := signedMulWithOverflowCheck(val.VotingPower, int64(times)) // check for overflow both multiplication and sum - if !overflow && val.Accum <= mostPositive-res { - val.Accum += res + res, overflow := safeMul(val.VotingPower, int64(times)) + if !overflow { + res2, overflow2 := safeAdd(val.Accum, res) + if !overflow2 { + val.Accum = res2 + } else { + val.Accum = mostPositive + } } else { val.Accum = mostPositive } @@ -70,8 +75,9 @@ func (valSet *ValidatorSet) IncrementAccum(times int) { } // mind underflow - if mostest.Accum >= mostNegative+valSet.TotalVotingPower() { - mostest.Accum -= valSet.TotalVotingPower() + res, underflow := safeSub(mostest.Accum, valSet.TotalVotingPower()) + if !underflow { + mostest.Accum = res } else { mostest.Accum = mostNegative } @@ -129,8 +135,9 @@ func (valSet *ValidatorSet) TotalVotingPower() int64 { if valSet.totalVotingPower == 0 { for _, val := range valSet.Validators { // mind overflow - if valSet.totalVotingPower <= mostPositive-val.VotingPower { - valSet.totalVotingPower += val.VotingPower + res, overflow := safeAdd(valSet.totalVotingPower, val.VotingPower) + if !overflow { + valSet.totalVotingPower = res } else { valSet.totalVotingPower = mostPositive return valSet.totalVotingPower @@ -443,10 +450,13 @@ func RandValidatorSet(numValidators int, votingPower int64) (*ValidatorSet, []*P return valSet, privValidators } +/////////////////////////////////////////////////////////////////////////////// +// Safe multiplication and addition/subtraction + const mostNegative int64 = -mostPositive - 1 const mostPositive int64 = 1<<63 - 1 -func signedMulWithOverflowCheck(a, b int64) (int64, bool) { +func safeMul(a, b int64) (int64, bool) { if a == 0 || b == 0 { return 0, false } @@ -462,3 +472,16 @@ func signedMulWithOverflowCheck(a, b int64) (int64, bool) { c := a * b return c, c/b != a } + +func safeAdd(a, b int64) (int64, bool) { + if b > 0 && a > mostPositive-b { + return -1, true + } else if b < 0 && a < mostNegative-b { + return -1, true + } + return a + b, false +} + +func safeSub(a, b int64) (int64, bool) { + return safeAdd(a, -b) +} diff --git a/types/validator_set_test.go b/types/validator_set_test.go index c65f507fa..dd2a59999 100644 --- a/types/validator_set_test.go +++ b/types/validator_set_test.go @@ -4,6 +4,7 @@ import ( "bytes" "strings" "testing" + "testing/quick" "github.com/stretchr/testify/assert" crypto "github.com/tendermint/go-crypto" @@ -191,6 +192,16 @@ func TestProposerSelection3(t *testing.T) { } } +func TestValidatorSetTotalVotingPowerOverflows(t *testing.T) { + vset := NewValidatorSet([]*Validator{ + {Address: []byte("a"), VotingPower: mostPositive, Accum: 0}, + {Address: []byte("b"), VotingPower: mostPositive, Accum: 0}, + {Address: []byte("c"), VotingPower: mostPositive, Accum: 0}, + }) + + assert.Equal(t, mostPositive, vset.TotalVotingPower()) +} + func TestValidatorSetIncrementAccumOverflows(t *testing.T) { // NewValidatorSet calls IncrementAccum(1) vset := NewValidatorSet([]*Validator{ @@ -220,14 +231,24 @@ func TestValidatorSetIncrementAccumUnderflows(t *testing.T) { assert.Equal(t, mostNegative, vset.Validators[1].Accum, "1") } -func TestValidatorSetTotalVotingPowerOverflows(t *testing.T) { - vset := NewValidatorSet([]*Validator{ - {Address: []byte("a"), VotingPower: mostPositive, Accum: 0}, - {Address: []byte("b"), VotingPower: mostPositive, Accum: 0}, - {Address: []byte("c"), VotingPower: mostPositive, Accum: 0}, - }) +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) + } +} - assert.Equal(t, mostPositive, vset.TotalVotingPower()) +func TestSafeAdd(t *testing.T) { + f := func(a, b int64) bool { + c, overflow := safeAdd(a, b) + return overflow || (!overflow && c == a+b) + } + if err := quick.Check(f, nil); err != nil { + t.Error(err) + } } func BenchmarkValidatorSetCopy(b *testing.B) { From 1339a44402b7e4f343b1051658c9b267a0cd970e Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 26 Dec 2017 14:13:12 -0600 Subject: [PATCH 3/3] add safe*Clip funcs --- types/validator_set.go | 80 +++++++++++++++++++++++-------------- types/validator_set_test.go | 47 ++++++++++++++++------ 2 files changed, 83 insertions(+), 44 deletions(-) diff --git a/types/validator_set.go b/types/validator_set.go index a9b986590..3876c19d7 100644 --- a/types/validator_set.go +++ b/types/validator_set.go @@ -3,6 +3,7 @@ package types import ( "bytes" "fmt" + "math" "sort" "strings" @@ -53,17 +54,7 @@ func (valSet *ValidatorSet) IncrementAccum(times int) { validatorsHeap := cmn.NewHeap() for _, val := range valSet.Validators { // check for overflow both multiplication and sum - res, overflow := safeMul(val.VotingPower, int64(times)) - if !overflow { - res2, overflow2 := safeAdd(val.Accum, res) - if !overflow2 { - val.Accum = res2 - } else { - val.Accum = mostPositive - } - } else { - val.Accum = mostPositive - } + val.Accum = safeAddClip(val.Accum, safeMulClip(val.VotingPower, int64(times))) validatorsHeap.Push(val, accumComparable{val}) } @@ -75,12 +66,7 @@ func (valSet *ValidatorSet) IncrementAccum(times int) { } // mind underflow - res, underflow := safeSub(mostest.Accum, valSet.TotalVotingPower()) - if !underflow { - mostest.Accum = res - } else { - mostest.Accum = mostNegative - } + mostest.Accum = safeSubClip(mostest.Accum, valSet.TotalVotingPower()) validatorsHeap.Update(mostest, accumComparable{mostest}) } } @@ -135,13 +121,7 @@ func (valSet *ValidatorSet) TotalVotingPower() int64 { if valSet.totalVotingPower == 0 { for _, val := range valSet.Validators { // mind overflow - res, overflow := safeAdd(valSet.totalVotingPower, val.VotingPower) - if !overflow { - valSet.totalVotingPower = res - } else { - valSet.totalVotingPower = mostPositive - return valSet.totalVotingPower - } + valSet.totalVotingPower = safeAddClip(valSet.totalVotingPower, val.VotingPower) } } return valSet.totalVotingPower @@ -453,9 +433,6 @@ func RandValidatorSet(numValidators int, votingPower int64) (*ValidatorSet, []*P /////////////////////////////////////////////////////////////////////////////// // Safe multiplication and addition/subtraction -const mostNegative int64 = -mostPositive - 1 -const mostPositive int64 = 1<<63 - 1 - func safeMul(a, b int64) (int64, bool) { if a == 0 || b == 0 { return 0, false @@ -466,7 +443,7 @@ func safeMul(a, b int64) (int64, bool) { if b == 1 { return a, false } - if a == mostNegative || b == mostNegative { + if a == math.MinInt64 || b == math.MinInt64 { return -1, true } c := a * b @@ -474,14 +451,55 @@ func safeMul(a, b int64) (int64, bool) { } func safeAdd(a, b int64) (int64, bool) { - if b > 0 && a > mostPositive-b { + if b > 0 && a > math.MaxInt64-b { return -1, true - } else if b < 0 && a < mostNegative-b { + } else if b < 0 && a < math.MinInt64-b { return -1, true } return a + b, false } func safeSub(a, b int64) (int64, bool) { - return safeAdd(a, -b) + if b > 0 && a < math.MinInt64+b { + return -1, true + } else if b < 0 && a > math.MaxInt64+b { + return -1, true + } + 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 + } else { + return math.MaxInt64 + } + } + return c +} + +func safeAddClip(a, b int64) int64 { + c, overflow := safeAdd(a, b) + if overflow { + if b < 0 { + return math.MinInt64 + } else { + return math.MaxInt64 + } + } + return c +} + +func safeSubClip(a, b int64) int64 { + c, overflow := safeSub(a, b) + if overflow { + if b > 0 { + return math.MinInt64 + } else { + return math.MaxInt64 + } + } + return c } diff --git a/types/validator_set_test.go b/types/validator_set_test.go index dd2a59999..9c7512378 100644 --- a/types/validator_set_test.go +++ b/types/validator_set_test.go @@ -2,6 +2,7 @@ package types import ( "bytes" + "math" "strings" "testing" "testing/quick" @@ -194,41 +195,41 @@ func TestProposerSelection3(t *testing.T) { func TestValidatorSetTotalVotingPowerOverflows(t *testing.T) { vset := NewValidatorSet([]*Validator{ - {Address: []byte("a"), VotingPower: mostPositive, Accum: 0}, - {Address: []byte("b"), VotingPower: mostPositive, Accum: 0}, - {Address: []byte("c"), VotingPower: mostPositive, Accum: 0}, + {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.Equal(t, mostPositive, vset.TotalVotingPower()) + 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: mostPositive, Accum: 0}, + 0: {Address: []byte("a"), VotingPower: math.MaxInt64, Accum: 0}, // too big accum - 1: {Address: []byte("b"), VotingPower: 10, Accum: mostPositive}, + 1: {Address: []byte("b"), VotingPower: 10, Accum: math.MaxInt64}, // almost too big accum - 2: {Address: []byte("c"), VotingPower: 10, Accum: mostPositive - 5}, + 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.Equal(t, mostPositive, vset.Validators[1].Accum, "1") - assert.Equal(t, mostPositive, vset.Validators[2].Accum, "2") + assert.EqualValues(t, math.MaxInt64, vset.Validators[1].Accum, "1") + assert.EqualValues(t, math.MaxInt64, vset.Validators[2].Accum, "2") } func TestValidatorSetIncrementAccumUnderflows(t *testing.T) { // NewValidatorSet calls IncrementAccum(1) vset := NewValidatorSet([]*Validator{ - 0: {Address: []byte("a"), VotingPower: mostPositive, Accum: mostNegative}, - 1: {Address: []byte("b"), VotingPower: 1, Accum: mostNegative}, + 0: {Address: []byte("a"), VotingPower: math.MaxInt64, Accum: math.MinInt64}, + 1: {Address: []byte("b"), VotingPower: 1, Accum: math.MinInt64}, }) vset.IncrementAccum(5) - assert.Equal(t, mostNegative, vset.Validators[0].Accum, "0") - assert.Equal(t, mostNegative, vset.Validators[1].Accum, "1") + assert.EqualValues(t, math.MinInt64, vset.Validators[0].Accum, "0") + assert.EqualValues(t, math.MinInt64, vset.Validators[1].Accum, "1") } func TestSafeMul(t *testing.T) { @@ -251,6 +252,26 @@ 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)) + assert.EqualValues(t, math.MinInt64, safeAddClip(math.MinInt64, -10)) +} + +func TestSafeSubClip(t *testing.T) { + assert.EqualValues(t, math.MinInt64, safeSubClip(math.MinInt64, 10)) + assert.EqualValues(t, 0, safeSubClip(math.MinInt64, math.MinInt64)) + assert.EqualValues(t, math.MinInt64, safeSubClip(math.MinInt64, math.MaxInt64)) + assert.EqualValues(t, math.MaxInt64, safeSubClip(math.MaxInt64, -10)) +} + func BenchmarkValidatorSetCopy(b *testing.B) { b.StopTimer() vset := NewValidatorSet([]*Validator{})