From 8e79209afeee8267cd4042d08ff133aed288e439 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 4 Sep 2018 17:08:22 -0400 Subject: [PATCH 01/11] Update GetTendermintUpdates Only return ABCI validators that match certain criteria --- x/stake/keeper/validator.go | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/x/stake/keeper/validator.go b/x/stake/keeper/validator.go index 47a5d5f66fa0..666e29110b95 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -201,16 +201,32 @@ func (k Keeper) GetValidatorsByPower(ctx sdk.Context) []types.Validator { // Accumulated updates to the active/bonded validator set for tendermint // get the most recently updated validators +// +// CONTRACT: Only validators with non-zero power or zero-power that were bonded +// at the previous block height are returned to Tendermint. func (k Keeper) GetTendermintUpdates(ctx sdk.Context) (updates []abci.Validator) { store := ctx.KVStore(k.storeKey) - iterator := sdk.KVStorePrefixIterator(store, TendermintUpdatesKey) //smallest to largest + iterator := sdk.KVStorePrefixIterator(store, TendermintUpdatesKey) for ; iterator.Valid(); iterator.Next() { - valBytes := iterator.Value() - var val abci.Validator - k.cdc.MustUnmarshalBinary(valBytes, &val) - updates = append(updates, val) + var abciVal abci.Validator + + abciValBytes := iterator.Value() + k.cdc.MustUnmarshalBinary(abciValBytes, &abciVal) + + val, ok := k.GetValidator(ctx, abciVal.GetAddress()) + if !ok { + panic(fmt.Sprintf("validator record not found for address: %v\n", abciVal.GetAddress())) + } + + prevBonded := val.BondHeight < ctx.BlockHeight() + zeroPower := val.GetPower().Equal(sdk.ZeroDec()) + + if !zeroPower || zeroPower && prevBonded { + updates = append(updates, abciVal) + } } + iterator.Close() return } From a04c52b30a3bffe98a648ff51b2ea0d8cda84911 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 4 Sep 2018 18:38:14 -0400 Subject: [PATCH 02/11] Fix previous bug on missing ABCI validator updates and fix unit tests --- x/stake/keeper/validator.go | 20 ++++++++++++-------- x/stake/keeper/validator_test.go | 22 +++++++++++++++++----- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/x/stake/keeper/validator.go b/x/stake/keeper/validator.go index 666e29110b95..69a3f16e3bb3 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -214,15 +214,19 @@ func (k Keeper) GetTendermintUpdates(ctx sdk.Context) (updates []abci.Validator) abciValBytes := iterator.Value() k.cdc.MustUnmarshalBinary(abciValBytes, &abciVal) - val, ok := k.GetValidator(ctx, abciVal.GetAddress()) - if !ok { - panic(fmt.Sprintf("validator record not found for address: %v\n", abciVal.GetAddress())) - } - - prevBonded := val.BondHeight < ctx.BlockHeight() - zeroPower := val.GetPower().Equal(sdk.ZeroDec()) + val, found := k.GetValidator(ctx, abciVal.GetAddress()) + if found { + // The validator is new or already exists in the store and must adhere to + // Tendermint invariants. + prevBonded := val.BondHeight < ctx.BlockHeight() + zeroPower := val.GetPower().Equal(sdk.ZeroDec()) - if !zeroPower || zeroPower && prevBonded { + if !zeroPower || zeroPower && prevBonded { + updates = append(updates, abciVal) + } + } else { + // Add the ABCI validator in such a case where the validator was removed + // from the store as it must have existed before. updates = append(updates, abciVal) } } diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index 2f471a2a5e70..3647667ebc2a 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -16,8 +16,11 @@ func TestSetValidator(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 10) pool := keeper.GetPool(ctx) + valPubKey := PKs[0] + valAddr := sdk.ValAddress(valPubKey.Address().Bytes()) + // test how the validator is set from a purely unbonbed pool - validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) + validator := types.NewValidator(valAddr, valPubKey, types.Description{}) validator, pool, _ = validator.AddTokensFromDel(pool, sdk.NewInt(10)) require.Equal(t, sdk.Unbonded, validator.Status) assert.True(sdk.DecEq(t, sdk.NewDec(10), validator.Tokens)) @@ -26,14 +29,14 @@ func TestSetValidator(t *testing.T) { keeper.UpdateValidator(ctx, validator) // after the save the validator should be bonded - validator, found := keeper.GetValidator(ctx, addrVals[0]) + validator, found := keeper.GetValidator(ctx, valAddr) require.True(t, found) require.Equal(t, sdk.Bonded, validator.Status) assert.True(sdk.DecEq(t, sdk.NewDec(10), validator.Tokens)) assert.True(sdk.DecEq(t, sdk.NewDec(10), validator.DelegatorShares)) // Check each store for being saved - resVal, found := keeper.GetValidator(ctx, addrVals[0]) + resVal, found := keeper.GetValidator(ctx, valAddr) assert.True(ValEq(t, validator, resVal)) require.True(t, found) @@ -641,8 +644,13 @@ func TestClearTendermintUpdates(t *testing.T) { validators := make([]types.Validator, len(amts)) for i, amt := range amts { pool := keeper.GetPool(ctx) - validators[i] = types.NewValidator(sdk.ValAddress(Addrs[i]), PKs[i], types.Description{}) + + valPubKey := PKs[i] + valAddr := sdk.ValAddress(valPubKey.Address().Bytes()) + + validators[i] = types.NewValidator(valAddr, valPubKey, types.Description{}) validators[i], pool, _ = validators[i].AddTokensFromDel(pool, sdk.NewInt(amt)) + keeper.SetPool(ctx, pool) keeper.UpdateValidator(ctx, validators[i]) } @@ -661,7 +669,11 @@ func TestGetTendermintUpdatesAllNone(t *testing.T) { var validators [2]types.Validator for i, amt := range amts { pool := keeper.GetPool(ctx) - validators[i] = types.NewValidator(sdk.ValAddress(Addrs[i]), PKs[i], types.Description{}) + + valPubKey := PKs[i+1] + valAddr := sdk.ValAddress(valPubKey.Address().Bytes()) + + validators[i] = types.NewValidator(valAddr, valPubKey, types.Description{}) validators[i], pool, _ = validators[i].AddTokensFromDel(pool, sdk.NewInt(amt)) keeper.SetPool(ctx, pool) } From fd261872691511a7c93a9641a837a7ad95c24401 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 4 Sep 2018 18:43:49 -0400 Subject: [PATCH 03/11] Add fatal to enforce validator update invariant to simulation --- x/mock/simulation/random_simulate_blocks.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/x/mock/simulation/random_simulate_blocks.go b/x/mock/simulation/random_simulate_blocks.go index 14ca6c2166b1..4fd7e2a533e6 100644 --- a/x/mock/simulation/random_simulate_blocks.go +++ b/x/mock/simulation/random_simulate_blocks.go @@ -129,7 +129,7 @@ func SimulateFromSeed( request = RandomRequestBeginBlock(r, validators, livenessTransitionMatrix, evidenceFraction, pastTimes, pastSigningValidators, event, header, log) // Update the validator set - validators = updateValidators(tb, r, validators, res.ValidatorUpdates, event) + validators = updateValidators(tb, r, log, validators, res.ValidatorUpdates, event) } fmt.Printf("\nSimulation complete. Final height (blocks): %d, final time (seconds), : %v, operations ran %d\n", header.Height, header.Time, opCount) @@ -320,19 +320,14 @@ func AssertAllInvariants(t *testing.T, app *baseapp.BaseApp, tests []Invariant, } // updateValidators mimicks Tendermint's update logic -func updateValidators(tb testing.TB, r *rand.Rand, current map[string]mockValidator, updates []abci.Validator, event func(string)) map[string]mockValidator { +func updateValidators(tb testing.TB, r *rand.Rand, log string, current map[string]mockValidator, updates []abci.Validator, event func(string)) map[string]mockValidator { for _, update := range updates { switch { case update.Power == 0: - // // TEMPORARY DEBUG CODE TO PROVE THAT THE OLD METHOD WAS BROKEN - // // (i.e. didn't catch in the event of problem) - // if val, ok := tb.(*testing.T); ok { - // require.NotNil(val, current[string(update.PubKey.Data)]) - // } - // // CORRECT CHECK - // if _, ok := current[string(update.PubKey.Data)]; !ok { - // tb.Fatalf("tried to delete a nonexistent validator") - // } + if _, ok := current[string(update.PubKey.Data)]; !ok { + tb.Fatalf("tried to delete a nonexistent validator: %v", log) + } + event("endblock/validatorupdates/kicked") delete(current, string(update.PubKey.Data)) default: From 540bae4dbb706ae5654b15e2dde241c729806adb Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 5 Sep 2018 09:23:05 -0400 Subject: [PATCH 04/11] Update GetTendermintUpdates contract --- x/stake/keeper/validator.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x/stake/keeper/validator.go b/x/stake/keeper/validator.go index 69a3f16e3bb3..5b4c063989f3 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -203,7 +203,8 @@ func (k Keeper) GetValidatorsByPower(ctx sdk.Context) []types.Validator { // get the most recently updated validators // // CONTRACT: Only validators with non-zero power or zero-power that were bonded -// at the previous block height are returned to Tendermint. +// at the previous block height or were removed from the validator set entirely +// are returned to Tendermint. func (k Keeper) GetTendermintUpdates(ctx sdk.Context) (updates []abci.Validator) { store := ctx.KVStore(k.storeKey) From cdc288a3f9f4db875404997c043a41ee7451d844 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 5 Sep 2018 09:52:40 -0400 Subject: [PATCH 05/11] Implement TestGetTendermintUpdatesBonded unit test --- x/stake/keeper/validator_test.go | 64 ++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index 3647667ebc2a..a83b5098bb5f 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -907,3 +907,67 @@ func TestGetTendermintUpdatesPowerDecrease(t *testing.T) { require.Equal(t, validators[0].ABCIValidator(), updates[0]) require.Equal(t, validators[1].ABCIValidator(), updates[1]) } + +func TestGetTendermintUpdatesBonded(t *testing.T) { + ctx, _, keeper := CreateTestInput(t, false, 1000) + params := keeper.GetParams(ctx) + params.MaxValidators = uint16(3) + + keeper.SetParams(ctx, params) + + amts := []int64{100, 100} + var validators [2]types.Validator + + // initialize some validators into the state + for i, amt := range amts { + pool := keeper.GetPool(ctx) + valPubKey := PKs[i+1] + valAddr := sdk.ValAddress(valPubKey.Address().Bytes()) + + validators[i] = types.NewValidator(valAddr, valPubKey, types.Description{}) + validators[i], pool, _ = validators[i].AddTokensFromDel(pool, sdk.NewInt(amt)) + + keeper.SetPool(ctx, pool) + validators[i] = keeper.UpdateValidator(ctx, validators[i]) + } + + // verify initial Tendermint updates are correct + updates := keeper.GetTendermintUpdates(ctx) + require.Equal(t, len(validators), len(updates)) + require.Equal(t, validators[0].ABCIValidator(), updates[0]) + require.Equal(t, validators[1].ABCIValidator(), updates[1]) + + keeper.ClearTendermintUpdates(ctx) + require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx))) + + // update initial validator set + for i, amt := range amts { + pool := keeper.GetPool(ctx) + validators[i], pool, _ = validators[i].AddTokensFromDel(pool, sdk.NewInt(amt)) + + keeper.SetPool(ctx, pool) + validators[i] = keeper.UpdateValidator(ctx, validators[i]) + } + + // add a validator that goes from zero power, to non-zero power, back to zero + // power + pool := keeper.GetPool(ctx) + valPubKey := PKs[len(validators)+1] + valAddr := sdk.ValAddress(valPubKey.Address().Bytes()) + amt := sdk.NewInt(100) + + validator := types.NewValidator(valAddr, valPubKey, types.Description{}) + validator, pool, _ = validator.AddTokensFromDel(pool, amt) + + keeper.SetPool(ctx, pool) + validator = keeper.UpdateValidator(ctx, validator) + + validator, pool, _ = validator.RemoveDelShares(pool, sdk.NewDecFromInt(amt)) + validator = keeper.UpdateValidator(ctx, validator) + + // verify initial Tendermint updates are correct + updates = keeper.GetTendermintUpdates(ctx) + require.Equal(t, len(validators), len(updates)) + require.Equal(t, validators[0].ABCIValidator(), updates[0]) + require.Equal(t, validators[1].ABCIValidator(), updates[1]) +} From 9dc493069bcadf613b6bb792446ad610f27354d6 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 5 Sep 2018 10:02:25 -0400 Subject: [PATCH 06/11] Add additional case to TestGetTendermintUpdatesBonded --- x/stake/keeper/validator_test.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index a83b5098bb5f..bc43a0ab2c0e 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -949,8 +949,8 @@ func TestGetTendermintUpdatesBonded(t *testing.T) { validators[i] = keeper.UpdateValidator(ctx, validators[i]) } - // add a validator that goes from zero power, to non-zero power, back to zero - // power + // add a new validator that goes from zero power, to non-zero power, back to + // zero power pool := keeper.GetPool(ctx) valPubKey := PKs[len(validators)+1] valAddr := sdk.ValAddress(valPubKey.Address().Bytes()) @@ -965,9 +965,20 @@ func TestGetTendermintUpdatesBonded(t *testing.T) { validator, pool, _ = validator.RemoveDelShares(pool, sdk.NewDecFromInt(amt)) validator = keeper.UpdateValidator(ctx, validator) + // add a new validator that increases in power + valPubKey = PKs[len(validators)+2] + valAddr = sdk.ValAddress(valPubKey.Address().Bytes()) + + validator = types.NewValidator(valAddr, valPubKey, types.Description{}) + validator, pool, _ = validator.AddTokensFromDel(pool, sdk.NewInt(500)) + + keeper.SetPool(ctx, pool) + validator = keeper.UpdateValidator(ctx, validator) + // verify initial Tendermint updates are correct updates = keeper.GetTendermintUpdates(ctx) - require.Equal(t, len(validators), len(updates)) - require.Equal(t, validators[0].ABCIValidator(), updates[0]) - require.Equal(t, validators[1].ABCIValidator(), updates[1]) + require.Equal(t, len(validators)+1, len(updates)) + require.Equal(t, validator.ABCIValidator(), updates[0]) + require.Equal(t, validators[0].ABCIValidator(), updates[1]) + require.Equal(t, validators[1].ABCIValidator(), updates[2]) } From b0d25afa24a5bd61820e61dd2ad809584d9d791d Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 5 Sep 2018 10:05:17 -0400 Subject: [PATCH 07/11] Update pending log --- PENDING.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/PENDING.md b/PENDING.md index 205f001b6357..6ddf09993ac5 100644 --- a/PENDING.md +++ b/PENDING.md @@ -100,6 +100,8 @@ BUG FIXES * [cli] \#1997 Handle panics gracefully when `gaiacli stake {delegation,unbond}` fail to unmarshal delegation. * Gaia + * [x/stake] Return correct Tendermint validator update set on `EndBlocker` by not + including non previously bonded validators that have zero power. [#2189](https://github.com/cosmos/cosmos-sdk/issues/2189) * SDK * \#1988 Make us compile on OpenBSD (disable ledger) [#1988] (https://github.com/cosmos/cosmos-sdk/issues/1988) From 05de30896bd34bace8c2aad043c42c274ff63bf1 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 7 Sep 2018 15:20:25 -0400 Subject: [PATCH 08/11] Rename GetTendermintUpdates --- docs/spec/staking/end_block.md | 2 +- x/stake/handler.go | 2 +- x/stake/keeper/validator.go | 2 +- x/stake/keeper/validator_test.go | 68 ++++++++++++++++---------------- 4 files changed, 37 insertions(+), 37 deletions(-) diff --git a/docs/spec/staking/end_block.md b/docs/spec/staking/end_block.md index c2fe143baf5f..a294692610e9 100644 --- a/docs/spec/staking/end_block.md +++ b/docs/spec/staking/end_block.md @@ -12,7 +12,7 @@ the changes cleared ```golang EndBlock() ValidatorSetChanges - vsc = GetTendermintUpdates() + vsc = GetValidTendermintUpdates() ClearTendermintUpdates() return vsc ``` diff --git a/x/stake/handler.go b/x/stake/handler.go index 4b478fffd7b0..115c891eb470 100644 --- a/x/stake/handler.go +++ b/x/stake/handler.go @@ -52,7 +52,7 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) (ValidatorUpdates []abci.Valid k.SetIntraTxCounter(ctx, 0) // calculate validator set changes - ValidatorUpdates = k.GetTendermintUpdates(ctx) + ValidatorUpdates = k.GetValidTendermintUpdates(ctx) k.ClearTendermintUpdates(ctx) return } diff --git a/x/stake/keeper/validator.go b/x/stake/keeper/validator.go index 5b4c063989f3..4653c1a765ba 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -205,7 +205,7 @@ func (k Keeper) GetValidatorsByPower(ctx sdk.Context) []types.Validator { // CONTRACT: Only validators with non-zero power or zero-power that were bonded // at the previous block height or were removed from the validator set entirely // are returned to Tendermint. -func (k Keeper) GetTendermintUpdates(ctx sdk.Context) (updates []abci.Validator) { +func (k Keeper) GetValidTendermintUpdates(ctx sdk.Context) (updates []abci.Validator) { store := ctx.KVStore(k.storeKey) iterator := sdk.KVStorePrefixIterator(store, TendermintUpdatesKey) diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index bc43a0ab2c0e..c86052f4a026 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -48,7 +48,7 @@ func TestSetValidator(t *testing.T) { require.Equal(t, 1, len(resVals)) assert.True(ValEq(t, validator, resVals[0])) - updates := keeper.GetTendermintUpdates(ctx) + updates := keeper.GetValidTendermintUpdates(ctx) require.Equal(t, 1, len(updates)) require.Equal(t, validator.ABCIValidator(), updates[0]) } @@ -655,14 +655,14 @@ func TestClearTendermintUpdates(t *testing.T) { keeper.UpdateValidator(ctx, validators[i]) } - updates := keeper.GetTendermintUpdates(ctx) + updates := keeper.GetValidTendermintUpdates(ctx) require.Equal(t, len(amts), len(updates)) keeper.ClearTendermintUpdates(ctx) - updates = keeper.GetTendermintUpdates(ctx) + updates = keeper.GetValidTendermintUpdates(ctx) require.Equal(t, 0, len(updates)) } -func TestGetTendermintUpdatesAllNone(t *testing.T) { +func TestGetValidTendermintUpdatesAllNone(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000) amts := []int64{10, 20} @@ -680,11 +680,11 @@ func TestGetTendermintUpdatesAllNone(t *testing.T) { // test from nothing to something // tendermintUpdate set: {} -> {c1, c3} - require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx))) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) validators[0] = keeper.UpdateValidator(ctx, validators[0]) validators[1] = keeper.UpdateValidator(ctx, validators[1]) - updates := keeper.GetTendermintUpdates(ctx) + updates := keeper.GetValidTendermintUpdates(ctx) assert.Equal(t, 2, len(updates)) assert.Equal(t, validators[0].ABCIValidator(), updates[0]) assert.Equal(t, validators[1].ABCIValidator(), updates[1]) @@ -692,12 +692,12 @@ func TestGetTendermintUpdatesAllNone(t *testing.T) { // test from something to nothing // tendermintUpdate set: {} -> {c1, c2, c3, c4} keeper.ClearTendermintUpdates(ctx) - require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx))) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) keeper.RemoveValidator(ctx, validators[0].Operator) keeper.RemoveValidator(ctx, validators[1].Operator) - updates = keeper.GetTendermintUpdates(ctx) + updates = keeper.GetValidTendermintUpdates(ctx) assert.Equal(t, 2, len(updates)) assert.Equal(t, tmtypes.TM2PB.PubKey(validators[0].PubKey), updates[0].PubKey) assert.Equal(t, tmtypes.TM2PB.PubKey(validators[1].PubKey), updates[1].PubKey) @@ -705,7 +705,7 @@ func TestGetTendermintUpdatesAllNone(t *testing.T) { assert.Equal(t, int64(0), updates[1].Power) } -func TestGetTendermintUpdatesIdentical(t *testing.T) { +func TestGetValidTendermintUpdatesIdentical(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000) amts := []int64{10, 20} @@ -719,16 +719,16 @@ func TestGetTendermintUpdatesIdentical(t *testing.T) { validators[0] = keeper.UpdateValidator(ctx, validators[0]) validators[1] = keeper.UpdateValidator(ctx, validators[1]) keeper.ClearTendermintUpdates(ctx) - require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx))) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) // test identical, // tendermintUpdate set: {} -> {} validators[0] = keeper.UpdateValidator(ctx, validators[0]) validators[1] = keeper.UpdateValidator(ctx, validators[1]) - require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx))) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) } -func TestGetTendermintUpdatesSingleValueChange(t *testing.T) { +func TestGetValidTendermintUpdatesSingleValueChange(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000) amts := []int64{10, 20} @@ -742,7 +742,7 @@ func TestGetTendermintUpdatesSingleValueChange(t *testing.T) { validators[0] = keeper.UpdateValidator(ctx, validators[0]) validators[1] = keeper.UpdateValidator(ctx, validators[1]) keeper.ClearTendermintUpdates(ctx) - require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx))) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) // test single value change // tendermintUpdate set: {} -> {c1'} @@ -750,13 +750,13 @@ func TestGetTendermintUpdatesSingleValueChange(t *testing.T) { validators[0].Tokens = sdk.NewDec(600) validators[0] = keeper.UpdateValidator(ctx, validators[0]) - updates := keeper.GetTendermintUpdates(ctx) + updates := keeper.GetValidTendermintUpdates(ctx) require.Equal(t, 1, len(updates)) require.Equal(t, validators[0].ABCIValidator(), updates[0]) } -func TestGetTendermintUpdatesMultipleValueChange(t *testing.T) { +func TestGetValidTendermintUpdatesMultipleValueChange(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000) amts := []int64{10, 20} @@ -770,7 +770,7 @@ func TestGetTendermintUpdatesMultipleValueChange(t *testing.T) { validators[0] = keeper.UpdateValidator(ctx, validators[0]) validators[1] = keeper.UpdateValidator(ctx, validators[1]) keeper.ClearTendermintUpdates(ctx) - require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx))) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) // test multiple value change // tendermintUpdate set: {c1, c3} -> {c1', c3'} @@ -781,13 +781,13 @@ func TestGetTendermintUpdatesMultipleValueChange(t *testing.T) { validators[0] = keeper.UpdateValidator(ctx, validators[0]) validators[1] = keeper.UpdateValidator(ctx, validators[1]) - updates := keeper.GetTendermintUpdates(ctx) + updates := keeper.GetValidTendermintUpdates(ctx) require.Equal(t, 2, len(updates)) require.Equal(t, validators[0].ABCIValidator(), updates[0]) require.Equal(t, validators[1].ABCIValidator(), updates[1]) } -func TestGetTendermintUpdatesInserted(t *testing.T) { +func TestGetValidTendermintUpdatesInserted(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000) amts := []int64{10, 20, 5, 15, 25} @@ -801,12 +801,12 @@ func TestGetTendermintUpdatesInserted(t *testing.T) { validators[0] = keeper.UpdateValidator(ctx, validators[0]) validators[1] = keeper.UpdateValidator(ctx, validators[1]) keeper.ClearTendermintUpdates(ctx) - require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx))) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) // test validtor added at the beginning // tendermintUpdate set: {} -> {c0} validators[2] = keeper.UpdateValidator(ctx, validators[2]) - updates := keeper.GetTendermintUpdates(ctx) + updates := keeper.GetValidTendermintUpdates(ctx) require.Equal(t, 1, len(updates)) require.Equal(t, validators[2].ABCIValidator(), updates[0]) @@ -814,7 +814,7 @@ func TestGetTendermintUpdatesInserted(t *testing.T) { // tendermintUpdate set: {} -> {c0} keeper.ClearTendermintUpdates(ctx) validators[3] = keeper.UpdateValidator(ctx, validators[3]) - updates = keeper.GetTendermintUpdates(ctx) + updates = keeper.GetValidTendermintUpdates(ctx) require.Equal(t, 1, len(updates)) require.Equal(t, validators[3].ABCIValidator(), updates[0]) @@ -822,12 +822,12 @@ func TestGetTendermintUpdatesInserted(t *testing.T) { // tendermintUpdate set: {} -> {c0} keeper.ClearTendermintUpdates(ctx) validators[4] = keeper.UpdateValidator(ctx, validators[4]) - updates = keeper.GetTendermintUpdates(ctx) + updates = keeper.GetValidTendermintUpdates(ctx) require.Equal(t, 1, len(updates)) require.Equal(t, validators[4].ABCIValidator(), updates[0]) } -func TestGetTendermintUpdatesWithCliffValidator(t *testing.T) { +func TestGetValidTendermintUpdatesWithCliffValidator(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000) params := types.DefaultParams() params.MaxValidators = 2 @@ -844,31 +844,31 @@ func TestGetTendermintUpdatesWithCliffValidator(t *testing.T) { validators[0] = keeper.UpdateValidator(ctx, validators[0]) validators[1] = keeper.UpdateValidator(ctx, validators[1]) keeper.ClearTendermintUpdates(ctx) - require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx))) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) // test validator added at the end but not inserted in the valset // tendermintUpdate set: {} -> {} keeper.UpdateValidator(ctx, validators[2]) - updates := keeper.GetTendermintUpdates(ctx) + updates := keeper.GetValidTendermintUpdates(ctx) require.Equal(t, 0, len(updates)) // test validator change its power and become a gotValidator (pushing out an existing) // tendermintUpdate set: {} -> {c0, c4} keeper.ClearTendermintUpdates(ctx) - require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx))) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) pool := keeper.GetPool(ctx) validators[2], pool, _ = validators[2].AddTokensFromDel(pool, sdk.NewInt(10)) keeper.SetPool(ctx, pool) validators[2] = keeper.UpdateValidator(ctx, validators[2]) - updates = keeper.GetTendermintUpdates(ctx) + updates = keeper.GetValidTendermintUpdates(ctx) require.Equal(t, 2, len(updates), "%v", updates) require.Equal(t, validators[0].ABCIValidatorZero(), updates[0]) require.Equal(t, validators[2].ABCIValidator(), updates[1]) } -func TestGetTendermintUpdatesPowerDecrease(t *testing.T) { +func TestGetValidTendermintUpdatesPowerDecrease(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000) amts := []int64{100, 100} @@ -882,7 +882,7 @@ func TestGetTendermintUpdatesPowerDecrease(t *testing.T) { validators[0] = keeper.UpdateValidator(ctx, validators[0]) validators[1] = keeper.UpdateValidator(ctx, validators[1]) keeper.ClearTendermintUpdates(ctx) - require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx))) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) // check initial power require.Equal(t, sdk.NewDec(100).RoundInt64(), validators[0].GetPower().RoundInt64()) @@ -902,13 +902,13 @@ func TestGetTendermintUpdatesPowerDecrease(t *testing.T) { require.Equal(t, sdk.NewDec(70).RoundInt64(), validators[1].GetPower().RoundInt64()) // Tendermint updates should reflect power change - updates := keeper.GetTendermintUpdates(ctx) + updates := keeper.GetValidTendermintUpdates(ctx) require.Equal(t, 2, len(updates)) require.Equal(t, validators[0].ABCIValidator(), updates[0]) require.Equal(t, validators[1].ABCIValidator(), updates[1]) } -func TestGetTendermintUpdatesBonded(t *testing.T) { +func TestGetValidTendermintUpdatesBonded(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000) params := keeper.GetParams(ctx) params.MaxValidators = uint16(3) @@ -932,13 +932,13 @@ func TestGetTendermintUpdatesBonded(t *testing.T) { } // verify initial Tendermint updates are correct - updates := keeper.GetTendermintUpdates(ctx) + updates := keeper.GetValidTendermintUpdates(ctx) require.Equal(t, len(validators), len(updates)) require.Equal(t, validators[0].ABCIValidator(), updates[0]) require.Equal(t, validators[1].ABCIValidator(), updates[1]) keeper.ClearTendermintUpdates(ctx) - require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx))) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) // update initial validator set for i, amt := range amts { @@ -976,7 +976,7 @@ func TestGetTendermintUpdatesBonded(t *testing.T) { validator = keeper.UpdateValidator(ctx, validator) // verify initial Tendermint updates are correct - updates = keeper.GetTendermintUpdates(ctx) + updates = keeper.GetValidTendermintUpdates(ctx) require.Equal(t, len(validators)+1, len(updates)) require.Equal(t, validator.ABCIValidator(), updates[0]) require.Equal(t, validators[0].ABCIValidator(), updates[1]) From ff4ffffe344eaedd833c7484a381af1c1de04ede Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 7 Sep 2018 15:28:06 -0400 Subject: [PATCH 09/11] Fix linting --- x/gov/simulation/msgs.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/x/gov/simulation/msgs.go b/x/gov/simulation/msgs.go index 2bb4b07b081c..2f20c1a898a7 100644 --- a/x/gov/simulation/msgs.go +++ b/x/gov/simulation/msgs.go @@ -151,12 +151,14 @@ func SimulateMsgVote(k gov.Keeper, sk stake.Keeper) simulation.Operation { return operationSimulateMsgVote(k, sk, nil, -1) } -func operationSimulateMsgVote(k gov.Keeper, sk stake.Keeper, key crypto.PrivKey, proposalID int64) simulation.Operation { +func operationSimulateMsgVote(k gov.Keeper, _ stake.Keeper, key crypto.PrivKey, proposalID int64) simulation.Operation { return func(tb testing.TB, r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context, keys []crypto.PrivKey, log string, event func(string)) (action string, fOp []simulation.FutureOperation, err sdk.Error) { if key == nil { key = simulation.RandomKey(r, keys) } + var ok bool + if proposalID < 0 { proposalID, ok = randomProposalID(r, k, ctx) if !ok { @@ -165,15 +167,18 @@ func operationSimulateMsgVote(k gov.Keeper, sk stake.Keeper, key crypto.PrivKey, } addr := sdk.AccAddress(key.PubKey().Address()) option := randomVotingOption(r) + msg := gov.NewMsgVote(addr, proposalID, option) if msg.ValidateBasic() != nil { tb.Fatalf("expected msg to pass ValidateBasic: %s, log %s", msg.GetSignBytes(), log) } + ctx, write := ctx.CacheContext() result := gov.NewHandler(k)(ctx, msg) if result.IsOK() { write() } + event(fmt.Sprintf("gov/MsgVote/%v", result.IsOK())) action = fmt.Sprintf("TestMsgVote: ok %v, msg %s", result.IsOK(), msg.GetSignBytes()) return action, nil, nil From f7f20f11489752bae3eea7a2f764e55bc278b4a0 Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Wed, 12 Sep 2018 06:45:38 +0000 Subject: [PATCH 10/11] R4R: Ensure Tendermint Validator Update Invariants - Part II (#2298) * Add a unit test showing invalid TM validator updates with bonded vals * Re-add defensive check in UpdateBondedValidators for jailed validators * Cleanup and set bond height in keeper#bondValidator * Get pool after getting context with new block height in unit test * Fix broken unit tests * Update prevBonded value * Rename validator to oldValidator in bondIncrement --- x/stake/keeper/validator.go | 40 ++++++++--------- x/stake/keeper/validator_test.go | 77 +++++++++++++++++++++++++++++++- 2 files changed, 95 insertions(+), 22 deletions(-) diff --git a/x/stake/keeper/validator.go b/x/stake/keeper/validator.go index 27c79097f076..b9ff9bfdb8e1 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -219,7 +219,7 @@ func (k Keeper) GetValidTendermintUpdates(ctx sdk.Context) (updates []abci.Valid if found { // The validator is new or already exists in the store and must adhere to // Tendermint invariants. - prevBonded := val.BondHeight < ctx.BlockHeight() + prevBonded := val.BondHeight < ctx.BlockHeight() && val.BondHeight > val.UnbondingHeight zeroPower := val.GetPower().Equal(sdk.ZeroDec()) if !zeroPower || zeroPower && prevBonded { @@ -264,7 +264,7 @@ func (k Keeper) UpdateValidator(ctx sdk.Context, validator types.Validator) type validator = k.updateForJailing(ctx, oldFound, oldValidator, validator) powerIncreasing := k.getPowerIncreasing(ctx, oldFound, oldValidator, validator) - validator.BondHeight, validator.BondIntraTxCounter = k.bondIncrement(ctx, oldFound, oldValidator, validator) + validator.BondHeight, validator.BondIntraTxCounter = k.bondIncrement(ctx, oldFound, oldValidator) valPower := k.updateValidatorPower(ctx, oldFound, oldValidator, validator, pool) cliffPower := k.GetCliffValidatorPower(ctx) @@ -336,7 +336,7 @@ func (k Keeper) updateCliffValidator(ctx sdk.Context, affectedVal types.Validato oldCliffVal, found := k.GetValidator(ctx, cliffAddr) if !found { - panic(fmt.Sprintf("cliff validator record not found for address: %v\n", cliffAddr)) + panic(fmt.Sprintf("cliff validator record not found for address: %X\n", cliffAddr)) } // Create a validator iterator ranging from smallest to largest by power @@ -349,11 +349,11 @@ func (k Keeper) updateCliffValidator(ctx sdk.Context, affectedVal types.Validato ownerAddr := iterator.Value() currVal, found := k.GetValidator(ctx, ownerAddr) if !found { - panic(fmt.Sprintf("validator record not found for address: %v\n", ownerAddr)) + panic(fmt.Sprintf("validator record not found for address: %X\n", ownerAddr)) } if currVal.Status != sdk.Bonded || currVal.Jailed { - panic(fmt.Sprintf("unexpected jailed or unbonded validator for address: %s\n", ownerAddr)) + panic(fmt.Sprintf("unexpected jailed or unbonded validator for address: %X\n", ownerAddr)) } newCliffVal = currVal @@ -366,13 +366,10 @@ func (k Keeper) updateCliffValidator(ctx sdk.Context, affectedVal types.Validato newCliffValRank := GetValidatorsByPowerIndexKey(newCliffVal, pool) if bytes.Equal(affectedVal.OperatorAddr, newCliffVal.OperatorAddr) { - // The affected validator remains the cliff validator, however, since // the store does not contain the new power, update the new power rank. store.Set(ValidatorPowerCliffKey, affectedValRank) - } else if bytes.Compare(affectedValRank, newCliffValRank) > 0 { - // The affected validator no longer remains the cliff validator as it's // power is greater than the new cliff validator. k.setCliffValidator(ctx, newCliffVal, pool) @@ -403,18 +400,20 @@ func (k Keeper) getPowerIncreasing(ctx sdk.Context, oldFound bool, oldValidator, // get the bond height and incremented intra-tx counter // nolint: unparam -func (k Keeper) bondIncrement(ctx sdk.Context, oldFound bool, oldValidator, - newValidator types.Validator) (height int64, intraTxCounter int16) { +func (k Keeper) bondIncrement( + ctx sdk.Context, found bool, oldValidator types.Validator) (height int64, intraTxCounter int16) { - // if already a validator, copy the old block height and counter, else set them - if oldFound && oldValidator.Status == sdk.Bonded { + // if already a validator, copy the old block height and counter + if found && oldValidator.Status == sdk.Bonded { height = oldValidator.BondHeight intraTxCounter = oldValidator.BondIntraTxCounter return } + height = ctx.BlockHeight() counter := k.GetIntraTxCounter(ctx) intraTxCounter = counter + k.SetIntraTxCounter(ctx, counter+1) return } @@ -482,10 +481,15 @@ func (k Keeper) UpdateBondedValidators( // if we've reached jailed validators no further bonded validators exist if validator.Jailed { + if validator.Status == sdk.Bonded { + panic(fmt.Sprintf("jailed validator cannot be bonded, address: %X\n", ownerAddr)) + } + break } - // increment bondedValidatorsCount / get the validator to bond + // increment the total number of bonded validators and potentially mark + // the validator to bond if validator.Status != sdk.Bonded { validatorToBond = validator if newValidatorBonded { @@ -494,13 +498,6 @@ func (k Keeper) UpdateBondedValidators( newValidatorBonded = true } - // increment the total number of bonded validators and potentially mark - // the validator to bond - if validator.Status != sdk.Bonded { - validatorToBond = validator - newValidatorBonded = true - } - bondedValidatorsCount++ iterator.Next() } @@ -533,7 +530,6 @@ func (k Keeper) UpdateBondedValidators( // validator was newly bonded and has greater power k.beginUnbondingValidator(ctx, oldCliffVal) } else { - // otherwise begin unbonding the affected validator, which must // have been kicked out affectedValidator = k.beginUnbondingValidator(ctx, affectedValidator) @@ -683,6 +679,8 @@ func (k Keeper) bondValidator(ctx sdk.Context, validator types.Validator) types. panic(fmt.Sprintf("should not already be bonded, validator: %v\n", validator)) } + validator.BondHeight = ctx.BlockHeight() + // set the status validator, pool = validator.UpdateStatus(pool, sdk.Bonded) k.SetPool(ctx, pool) diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index 304712b9a42a..d9531ae1a7af 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -908,7 +908,7 @@ func TestGetValidTendermintUpdatesPowerDecrease(t *testing.T) { require.Equal(t, validators[1].ABCIValidator(), updates[1]) } -func TestGetValidTendermintUpdatesBonded(t *testing.T) { +func TestGetValidTendermintUpdatesNewValidator(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000) params := keeper.GetParams(ctx) params.MaxValidators = uint16(3) @@ -982,3 +982,78 @@ func TestGetValidTendermintUpdatesBonded(t *testing.T) { require.Equal(t, validators[0].ABCIValidator(), updates[1]) require.Equal(t, validators[1].ABCIValidator(), updates[2]) } + +func TestGetValidTendermintUpdatesBondTransition(t *testing.T) { + ctx, _, keeper := CreateTestInput(t, false, 1000) + params := keeper.GetParams(ctx) + params.MaxValidators = uint16(2) + + keeper.SetParams(ctx, params) + + amts := []int64{100, 200, 300} + var validators [3]types.Validator + + // initialize some validators into the state + for i, amt := range amts { + pool := keeper.GetPool(ctx) + moniker := fmt.Sprintf("%d", i) + valPubKey := PKs[i+1] + valAddr := sdk.ValAddress(valPubKey.Address().Bytes()) + + validators[i] = types.NewValidator(valAddr, valPubKey, types.Description{Moniker: moniker}) + validators[i], pool, _ = validators[i].AddTokensFromDel(pool, sdk.NewInt(amt)) + + keeper.SetPool(ctx, pool) + validators[i] = keeper.UpdateValidator(ctx, validators[i]) + } + + // verify initial Tendermint updates are correct + updates := keeper.GetValidTendermintUpdates(ctx) + require.Equal(t, 2, len(updates)) + require.Equal(t, validators[2].ABCIValidator(), updates[0]) + require.Equal(t, validators[1].ABCIValidator(), updates[1]) + + keeper.ClearTendermintUpdates(ctx) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) + + // delegate to validator with lowest power but not enough to bond + ctx = ctx.WithBlockHeight(1) + pool := keeper.GetPool(ctx) + + validator, found := keeper.GetValidator(ctx, validators[0].OperatorAddr) + require.True(t, found) + + validator, pool, _ = validator.AddTokensFromDel(pool, sdk.NewInt(1)) + + keeper.SetPool(ctx, pool) + validators[0] = keeper.UpdateValidator(ctx, validator) + + // verify initial Tendermint updates are correct + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) + + // create a series of events that will bond and unbond the validator with + // lowest power in a single block context (height) + ctx = ctx.WithBlockHeight(2) + pool = keeper.GetPool(ctx) + + validator, found = keeper.GetValidator(ctx, validators[1].OperatorAddr) + require.True(t, found) + + validator, pool, _ = validator.RemoveDelShares(pool, validator.DelegatorShares) + + keeper.SetPool(ctx, pool) + validator = keeper.UpdateValidator(ctx, validator) + + validator, pool, _ = validator.AddTokensFromDel(pool, sdk.NewInt(250)) + + keeper.SetPool(ctx, pool) + validators[1] = keeper.UpdateValidator(ctx, validator) + + // verify initial Tendermint updates are correct + updates = keeper.GetValidTendermintUpdates(ctx) + require.Equal(t, 1, len(updates)) + require.Equal(t, validators[1].ABCIValidator(), updates[0]) + + keeper.ClearTendermintUpdates(ctx) + require.Equal(t, 0, len(keeper.GetValidTendermintUpdates(ctx))) +} From c16f33fe7f8bd11ad060b89135086137428013f9 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 12 Sep 2018 15:04:45 +0800 Subject: [PATCH 11/11] Fix tiny x/mock issue from merge --- x/mock/simulation/random_simulate_blocks.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/x/mock/simulation/random_simulate_blocks.go b/x/mock/simulation/random_simulate_blocks.go index cb8f8af304fb..3f91b2a6631d 100644 --- a/x/mock/simulation/random_simulate_blocks.go +++ b/x/mock/simulation/random_simulate_blocks.go @@ -158,7 +158,7 @@ func SimulateFromSeed( request = RandomRequestBeginBlock(r, validators, livenessTransitionMatrix, evidenceFraction, pastTimes, pastSigningValidators, event, header) // Update the validator set - validators = updateValidators(tb, r, log, validators, res.ValidatorUpdates, event) + validators = updateValidators(tb, r, validators, res.ValidatorUpdates, event) } if stopEarly { DisplayEvents(events) @@ -340,15 +340,14 @@ func RandomRequestBeginBlock(r *rand.Rand, validators map[string]mockValidator, } // updateValidators mimicks Tendermint's update logic -func updateValidators( - tb testing.TB, r *rand.Rand, log string, current map[string]mockValidator, - updates []abci.Validator, event func(string)) map[string]mockValidator { +// nolint: unparam +func updateValidators(tb testing.TB, r *rand.Rand, current map[string]mockValidator, updates []abci.Validator, event func(string)) map[string]mockValidator { for _, update := range updates { switch { case update.Power == 0: if _, ok := current[string(update.PubKey.Data)]; !ok { - tb.Fatalf("tried to delete a nonexistent validator: %v", log) + tb.Fatalf("tried to delete a nonexistent validator") } event("endblock/validatorupdates/kicked")