From 222665c7a3431f62125b4b94427346865ca6274b Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Tue, 27 Oct 2020 15:14:39 +0000 Subject: [PATCH 1/5] sort validators like tendermint --- x/staking/keeper/historical_info.go | 2 +- x/staking/keeper/historical_info_test.go | 17 ++++++++----- x/staking/types/historical_info.go | 3 ++- x/staking/types/validator.go | 27 +++++++++++++++++++++ x/staking/types/validator_test.go | 31 ++++++++++++++++++++++++ 5 files changed, 72 insertions(+), 8 deletions(-) diff --git a/x/staking/keeper/historical_info.go b/x/staking/keeper/historical_info.go index 20c5a8ce85c9..403ad886e0ad 100644 --- a/x/staking/keeper/historical_info.go +++ b/x/staking/keeper/historical_info.go @@ -90,7 +90,7 @@ func (k Keeper) TrackHistoricalInfo(ctx sdk.Context) { } // Create HistoricalInfo struct - lastVals := k.GetLastValidators(ctx) + lastVals := k.GetBondedValidatorsByPower(ctx) historicalEntry := types.NewHistoricalInfo(ctx.BlockHeader(), lastVals) // Set latest HistoricalInfo at current height diff --git a/x/staking/keeper/historical_info_test.go b/x/staking/keeper/historical_info_test.go index 0ccdee9408eb..fce2678baa48 100644 --- a/x/staking/keeper/historical_info_test.go +++ b/x/staking/keeper/historical_info_test.go @@ -31,7 +31,7 @@ func TestHistoricalInfo(t *testing.T) { recv, found := app.StakingKeeper.GetHistoricalInfo(ctx, 2) require.True(t, found, "HistoricalInfo not found after set") require.Equal(t, hi, recv, "HistoricalInfo not equal") - require.True(t, sort.IsSorted(types.Validators(recv.Valset)), "HistoricalInfo validators is not sorted") + require.True(t, sort.IsSorted(types.ValidatorsByVotingPower(recv.Valset)), "HistoricalInfo validators is not sorted") app.StakingKeeper.DeleteHistoricalInfo(ctx, 2) @@ -76,15 +76,20 @@ func TestTrackHistoricalInfo(t *testing.T) { require.True(t, found) require.Equal(t, hi5, recv) - // Set last validators in keeper + // Set bonded validators in keeper val1 := teststaking.NewValidator(t, addrVals[2], PKs[2]) + val1.Status = types.Bonded + val1.Tokens = sdk.NewInt(100) app.StakingKeeper.SetValidator(ctx, val1) - app.StakingKeeper.SetLastValidatorPower(ctx, val1.GetOperator(), 10) + app.StakingKeeper.SetValidatorByPowerIndex(ctx, val1) val2 := teststaking.NewValidator(t, addrVals[3], PKs[3]) - vals := []types.Validator{val1, val2} - sort.Sort(types.Validators(vals)) + val2.Status = types.Bonded + val2.Tokens = sdk.NewInt(80) app.StakingKeeper.SetValidator(ctx, val2) - app.StakingKeeper.SetLastValidatorPower(ctx, val2.GetOperator(), 8) + app.StakingKeeper.SetValidatorByPowerIndex(ctx, val2) + + vals := []types.Validator{val1, val2} + sort.Sort(types.ValidatorsByVotingPower(vals)) // Set Header for BeginBlock context header := tmproto.Header{ diff --git a/x/staking/types/historical_info.go b/x/staking/types/historical_info.go index 10df3ced0207..1a8461ad130c 100644 --- a/x/staking/types/historical_info.go +++ b/x/staking/types/historical_info.go @@ -14,7 +14,8 @@ import ( // NewHistoricalInfo will create a historical information struct from header and valset // it will first sort valset before inclusion into historical info func NewHistoricalInfo(header tmproto.Header, valSet Validators) HistoricalInfo { - sort.Sort(valSet) + // Must sort in the same way that tendermint does + sort.Sort(ValidatorsByVotingPower(valSet)) return HistoricalInfo{ Header: header, diff --git a/x/staking/types/validator.go b/x/staking/types/validator.go index 8b5cfcdf14eb..16e2488dba66 100644 --- a/x/staking/types/validator.go +++ b/x/staking/types/validator.go @@ -123,6 +123,33 @@ func (v Validators) Swap(i, j int) { v[j] = it } +// ValidatorsByVotingPower implements sort.Interface for []Validator based on +// the VotingPower and Address fields. +// Copied from tendermint/types/validator_set.go +type ValidatorsByVotingPower []Validator + +func (valz ValidatorsByVotingPower) Len() int { return len(valz) } + +func (valz ValidatorsByVotingPower) Less(i, j int) bool { + if valz[i].ConsensusPower() == valz[j].ConsensusPower() { + var addrI, addrJ []byte + pkI, errI := valz[i].TmConsPubKey() + pkJ, errJ := valz[j].TmConsPubKey() + // If errors are nil, sort by address, + // otherwise don't sort by address and just return false + if errI == nil && errJ == nil { + addrI = pkI.Address().Bytes() + addrJ = pkJ.Address().Bytes() + } + return bytes.Compare(addrI, addrJ) == -1 + } + return valz[i].ConsensusPower() > valz[j].ConsensusPower() +} + +func (valz ValidatorsByVotingPower) Swap(i, j int) { + valz[i], valz[j] = valz[j], valz[i] +} + // UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces func (v Validators) UnpackInterfaces(c codectypes.AnyUnpacker) error { for i := range v { diff --git a/x/staking/types/validator_test.go b/x/staking/types/validator_test.go index 2e9d961169a0..64afc97681bd 100644 --- a/x/staking/types/validator_test.go +++ b/x/staking/types/validator_test.go @@ -274,6 +274,37 @@ func TestValidatorsSortDeterminism(t *testing.T) { } } +// Check SortTendermint sorts the same as tendermint +func TestValidatorsSortTendermint(t *testing.T) { + vals := make([]Validator, 100) + + for i := range vals { + pk := ed25519.GenPrivKey().PubKey() + pk2 := ed25519.GenPrivKey().PubKey() + vals[i] = newValidator(t, sdk.ValAddress(pk2.Address()), pk) + vals[i].Status = Bonded + vals[i].Tokens = sdk.NewInt(rand.Int63()) + } + // create some validators with the same power + for i := 0; i < 10; i++ { + vals[i].Tokens = sdk.NewInt(1000000) + } + + valz := Validators(vals) + + // create expected tendermint validators by converting to tendermint then sorting + expectedVals, err := valz.ToTmValidators() + require.NoError(t, err) + sort.Sort(tmtypes.ValidatorsByVotingPower(expectedVals)) + + // sort in SDK and then convert to tendermint + sort.Sort(ValidatorsByVotingPower(valz)) + actualVals, err := valz.ToTmValidators() + require.NoError(t, err) + + require.Equal(t, expectedVals, actualVals, "sorting in SDK is not the same as sorting in Tendermint") +} + func TestValidatorToTm(t *testing.T) { vals := make(Validators, 10) expected := make([]*tmtypes.Validator, 10) From 3187fa07c53d60d7db0908b8b84ecf874ab5fb5f Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Wed, 28 Oct 2020 16:54:32 +0000 Subject: [PATCH 2/5] address comments --- x/staking/keeper/historical_info.go | 2 +- x/staking/keeper/historical_info_test.go | 8 ++------ x/staking/types/validator.go | 14 +++++--------- 3 files changed, 8 insertions(+), 16 deletions(-) diff --git a/x/staking/keeper/historical_info.go b/x/staking/keeper/historical_info.go index 403ad886e0ad..20c5a8ce85c9 100644 --- a/x/staking/keeper/historical_info.go +++ b/x/staking/keeper/historical_info.go @@ -90,7 +90,7 @@ func (k Keeper) TrackHistoricalInfo(ctx sdk.Context) { } // Create HistoricalInfo struct - lastVals := k.GetBondedValidatorsByPower(ctx) + lastVals := k.GetLastValidators(ctx) historicalEntry := types.NewHistoricalInfo(ctx.BlockHeader(), lastVals) // Set latest HistoricalInfo at current height diff --git a/x/staking/keeper/historical_info_test.go b/x/staking/keeper/historical_info_test.go index fce2678baa48..0729e661f1e7 100644 --- a/x/staking/keeper/historical_info_test.go +++ b/x/staking/keeper/historical_info_test.go @@ -78,15 +78,11 @@ func TestTrackHistoricalInfo(t *testing.T) { // Set bonded validators in keeper val1 := teststaking.NewValidator(t, addrVals[2], PKs[2]) - val1.Status = types.Bonded - val1.Tokens = sdk.NewInt(100) app.StakingKeeper.SetValidator(ctx, val1) - app.StakingKeeper.SetValidatorByPowerIndex(ctx, val1) + app.StakingKeeper.SetLastValidatorPower(ctx, val1.GetOperator(), 10) val2 := teststaking.NewValidator(t, addrVals[3], PKs[3]) - val2.Status = types.Bonded - val2.Tokens = sdk.NewInt(80) app.StakingKeeper.SetValidator(ctx, val2) - app.StakingKeeper.SetValidatorByPowerIndex(ctx, val2) + app.StakingKeeper.SetLastValidatorPower(ctx, val2.GetOperator(), 8) vals := []types.Validator{val1, val2} sort.Sort(types.ValidatorsByVotingPower(vals)) diff --git a/x/staking/types/validator.go b/x/staking/types/validator.go index 16e2488dba66..1d337b41171a 100644 --- a/x/staking/types/validator.go +++ b/x/staking/types/validator.go @@ -125,6 +125,7 @@ func (v Validators) Swap(i, j int) { // ValidatorsByVotingPower implements sort.Interface for []Validator based on // the VotingPower and Address fields. +// The validators are sorted first by their voting power (descending). Secondary index - Address (ascending). // Copied from tendermint/types/validator_set.go type ValidatorsByVotingPower []Validator @@ -132,15 +133,10 @@ func (valz ValidatorsByVotingPower) Len() int { return len(valz) } func (valz ValidatorsByVotingPower) Less(i, j int) bool { if valz[i].ConsensusPower() == valz[j].ConsensusPower() { - var addrI, addrJ []byte - pkI, errI := valz[i].TmConsPubKey() - pkJ, errJ := valz[j].TmConsPubKey() - // If errors are nil, sort by address, - // otherwise don't sort by address and just return false - if errI == nil && errJ == nil { - addrI = pkI.Address().Bytes() - addrJ = pkJ.Address().Bytes() - } + // If GetConsAddr returns error, then both addresses + // are empty and Less will return false + addrI, _ := valz[i].GetConsAddr() + addrJ, _ := valz[j].GetConsAddr() return bytes.Compare(addrI, addrJ) == -1 } return valz[i].ConsensusPower() > valz[j].ConsensusPower() From 18cfab164fa0f53450324f87ec29219af453e5a7 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Wed, 28 Oct 2020 16:58:21 +0000 Subject: [PATCH 3/5] switch ordering in tests --- x/staking/keeper/historical_info_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/staking/keeper/historical_info_test.go b/x/staking/keeper/historical_info_test.go index 0729e661f1e7..a9411d0db974 100644 --- a/x/staking/keeper/historical_info_test.go +++ b/x/staking/keeper/historical_info_test.go @@ -82,7 +82,7 @@ func TestTrackHistoricalInfo(t *testing.T) { app.StakingKeeper.SetLastValidatorPower(ctx, val1.GetOperator(), 10) val2 := teststaking.NewValidator(t, addrVals[3], PKs[3]) app.StakingKeeper.SetValidator(ctx, val2) - app.StakingKeeper.SetLastValidatorPower(ctx, val2.GetOperator(), 8) + app.StakingKeeper.SetLastValidatorPower(ctx, val2.GetOperator(), 80) vals := []types.Validator{val1, val2} sort.Sort(types.ValidatorsByVotingPower(vals)) From c444f93db3e601550ce514a382de5c4846bf6be8 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Wed, 28 Oct 2020 17:07:19 +0000 Subject: [PATCH 4/5] change sort logic in error case --- x/staking/types/validator.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/x/staking/types/validator.go b/x/staking/types/validator.go index 1d337b41171a..19e032dbabac 100644 --- a/x/staking/types/validator.go +++ b/x/staking/types/validator.go @@ -133,10 +133,12 @@ func (valz ValidatorsByVotingPower) Len() int { return len(valz) } func (valz ValidatorsByVotingPower) Less(i, j int) bool { if valz[i].ConsensusPower() == valz[j].ConsensusPower() { - // If GetConsAddr returns error, then both addresses - // are empty and Less will return false - addrI, _ := valz[i].GetConsAddr() - addrJ, _ := valz[j].GetConsAddr() + addrI, errI := valz[i].GetConsAddr() + addrJ, errJ := valz[j].GetConsAddr() + // If either returns error, then return false + if errI != nil || errJ != nil { + return false + } return bytes.Compare(addrI, addrJ) == -1 } return valz[i].ConsensusPower() > valz[j].ConsensusPower() From 723533c9b445d9a2465863a10c7c3d44fc3d3402 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Thu, 29 Oct 2020 11:23:34 +0000 Subject: [PATCH 5/5] don't change test validators array order --- x/staking/keeper/keeper_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/x/staking/keeper/keeper_test.go b/x/staking/keeper/keeper_test.go index 0342dfea9bda..2f9819989ff0 100644 --- a/x/staking/keeper/keeper_test.go +++ b/x/staking/keeper/keeper_test.go @@ -41,7 +41,11 @@ func (suite *KeeperTestSuite) SetupTest() { Height: 5, } - hi := types.NewHistoricalInfo(header, validators) + // sort a copy of the validators, so that original validators does not + // have its order changed + sortedVals := make([]types.Validator, len(validators)) + copy(sortedVals, validators) + hi := types.NewHistoricalInfo(header, sortedVals) app.StakingKeeper.SetHistoricalInfo(ctx, 5, &hi) suite.app, suite.ctx, suite.queryClient, suite.addrs, suite.vals = app, ctx, queryClient, addrs, validators