Skip to content

Commit

Permalink
R4R: Ensure Tendermint Validator Update Invariants - Part II (#2298)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
alexanderbez authored and cwgoes committed Sep 12, 2018
1 parent ebb2b2e commit f7f20f1
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 22 deletions.
40 changes: 19 additions & 21 deletions x/stake/keeper/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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()
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
77 changes: 76 additions & 1 deletion x/stake/keeper/validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)))
}

0 comments on commit f7f20f1

Please sign in to comment.