Fix simulation bugs; Incorprates #2676 from Sunny (#2677)

* Fix simulation bugs; Incorprates #2676 from Sunny
* Address review feedback; Update PENDING

PENDING.md

@@ -42,15 +42,15 @@ IMPROVEMENTS
 * Gaia
 
 * SDK
- - #2573 [x/distribution] add accum invariance
- - #2556 [x/mock/simulation] Fix debugging output
- - #2396 [x/mock/simulation] Change parameters to get more slashes
- - #2617 [x/mock/simulation] Randomize all genesis parameters
- - #2669 [x/stake] Added invarant check to make sure validator's power aligns with its spot in the power store.
+ - \#2573 [x/distribution] add accum invariance
+ - \#2556 [x/mock/simulation] Fix debugging output
+ - \#2396 [x/mock/simulation] Change parameters to get more slashes
+ - \#2617 [x/mock/simulation] Randomize all genesis parameters
+ - \#2669 [x/stake] Added invarant check to make sure validator's power aligns with its spot in the power store.
  - \#1924 [x/mock/simulation] Use a transition matrix for block size
  - \#2660 [x/mock/simulation] Staking transactions get tested far more frequently
- - #2610 [x/stake] Block redelegation to and from the same validator
- - #2652 [x/auth] Add benchmark for get and set account
+ - \#2610 [x/stake] Block redelegation to and from the same validator
+ - \#2652 [x/auth] Add benchmark for get and set account
 
 * Tendermint
 
@@ -62,10 +62,10 @@ BUG FIXES
 * Gaia CLI  (`gaiacli`)
 
 * Gaia
- - #2670 [x/stake] fixed incorrent `IterateBondedValidators` and split into two functions: `IterateBondedValidators` and `IterateLastBlockConsValidators`
+ - \#2670 [x/stake] fixed incorrent `IterateBondedValidators` and split into two functions: `IterateBondedValidators` and `IterateLastBlockConsValidators`
 
 * SDK
- - #2625 [x/gov] fix AppendTag function usage error
-
+ - \#2625 [x/gov] fix AppendTag function usage error
+ - \#2677 [x/stake, x/distribution] various staking/distribution fixes as found by the simulator
 
 * Tendermint

cmd/gaia/app/app.go

@@ -109,38 +109,40 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, baseAppOptio
 		app.cdc,
 		app.keyParams, app.tkeyParams,
 	)
-	app.stakeKeeper = stake.NewKeeper(
+	stakeKeeper := stake.NewKeeper(
 		app.cdc,
 		app.keyStake, app.tkeyStake,
 		app.bankKeeper, app.paramsKeeper.Subspace(stake.DefaultParamspace),
 		app.RegisterCodespace(stake.DefaultCodespace),
 	)
 	app.mintKeeper = mint.NewKeeper(app.cdc, app.keyMint,
 		app.paramsKeeper.Subspace(mint.DefaultParamspace),
-		app.stakeKeeper, app.feeCollectionKeeper,
+		&stakeKeeper, app.feeCollectionKeeper,
 	)
 	app.distrKeeper = distr.NewKeeper(
 		app.cdc,
 		app.keyDistr,
 		app.paramsKeeper.Subspace(distr.DefaultParamspace),
-		app.bankKeeper, app.stakeKeeper, app.feeCollectionKeeper,
+		app.bankKeeper, &stakeKeeper, app.feeCollectionKeeper,
 		app.RegisterCodespace(stake.DefaultCodespace),
 	)
 	app.slashingKeeper = slashing.NewKeeper(
 		app.cdc,
 		app.keySlashing,
-		app.stakeKeeper, app.paramsKeeper.Subspace(slashing.DefaultParamspace),
+		&stakeKeeper, app.paramsKeeper.Subspace(slashing.DefaultParamspace),
 		app.RegisterCodespace(slashing.DefaultCodespace),
 	)
 	app.govKeeper = gov.NewKeeper(
 		app.cdc,
 		app.keyGov,
-		app.paramsKeeper, app.paramsKeeper.Subspace(gov.DefaultParamspace), app.bankKeeper, app.stakeKeeper,
+		app.paramsKeeper, app.paramsKeeper.Subspace(gov.DefaultParamspace), app.bankKeeper, &stakeKeeper,
 		app.RegisterCodespace(gov.DefaultCodespace),
 	)
 
 	// register the staking hooks
-	app.stakeKeeper = app.stakeKeeper.WithHooks(
+	// NOTE: stakeKeeper above are passed by reference,
+	// so that it can be modified like below:
+	app.stakeKeeper = *stakeKeeper.SetHooks(
 		NewHooks(app.distrKeeper.Hooks(), app.slashingKeeper.Hooks()))
 
 	// register message routes

x/distribution/keeper/hooks.go

@@ -1,6 +1,8 @@
 package keeper
 
 import (
+	"fmt"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/distribution/types"
 )
@@ -38,11 +40,12 @@ func (k Keeper) onValidatorBonded(ctx sdk.Context, valAddr sdk.ValAddress) {
 	k.onValidatorModified(ctx, valAddr)
 }
 
-// XXX Consider removing this after debugging.
+// Sanity check, very useful!
 func (k Keeper) onValidatorPowerDidChange(ctx sdk.Context, valAddr sdk.ValAddress) {
 	vi := k.GetValidatorDistInfo(ctx, valAddr)
 	if vi.FeePoolWithdrawalHeight != ctx.BlockHeight() {
-		panic("expected validator dist info FeePoolWithdrawalHeight to be updated, but was not.")
+		panic(fmt.Sprintf("expected validator (%v) dist info FeePoolWithdrawalHeight to be updated to %v, but was %v.",
+			valAddr.String(), ctx.BlockHeight(), vi.FeePoolWithdrawalHeight))
 	}
 }
 

x/distribution/keeper/test_common.go

@@ -130,7 +130,7 @@ func CreateTestInputAdvanced(t *testing.T, isCheckTx bool, initCoins int64,
 	keeper := NewKeeper(cdc, keyDistr, pk.Subspace(DefaultParamspace), ck, sk, fck, types.DefaultCodespace)
 
 	// set the distribution hooks on staking
-	sk = sk.WithHooks(keeper.Hooks())
+	sk.SetHooks(keeper.Hooks())
 
 	// set genesis items required for distribution
 	keeper.SetFeePool(ctx, types.InitialFeePool())

x/mock/simulation/random_simulate_blocks.go

@@ -14,7 +14,7 @@ import (
 	"time"
 
 	abci "github.com/tendermint/tendermint/abci/types"
-	common "github.com/tendermint/tendermint/libs/common"
+	cmn "github.com/tendermint/tendermint/libs/common"
 	tmtypes "github.com/tendermint/tendermint/types"
 
 	"github.com/cosmos/cosmos-sdk/baseapp"
@@ -65,7 +65,7 @@ func SimulateFromSeed(tb testing.TB, app *baseapp.BaseApp,
 	testingMode, t, b := getTestingMode(tb)
 	fmt.Printf("Starting SimulateFromSeed with randomness created with seed %d\n", int(seed))
 	r := rand.New(rand.NewSource(seed))
-	params := RandomParams(r)
+	params := RandomParams(r) // := DefaultParams()
 	fmt.Printf("Randomized simulation params: %+v\n", params)
 	timestamp := randTimestamp(r)
 	fmt.Printf("Starting the simulation from time %v, unixtime %v\n", timestamp.UTC().Format(time.UnixDate), timestamp.Unix())
@@ -365,7 +365,7 @@ func getKeys(validators map[string]mockValidator) []string {
 }
 
 // randomProposer picks a random proposer from the current validator set
-func randomProposer(r *rand.Rand, validators map[string]mockValidator) common.HexBytes {
+func randomProposer(r *rand.Rand, validators map[string]mockValidator) cmn.HexBytes {
 	keys := getKeys(validators)
 	if len(keys) == 0 {
 		return nil

x/slashing/keeper.go

@@ -49,6 +49,15 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, addr crypto.Address, infractio
 		panic(fmt.Sprintf("Validator consensus-address %v not found", consAddr))
 	}
 
+	// Get validator.
+	validator := k.validatorSet.ValidatorByConsAddr(ctx, consAddr)
+	if validator == nil || validator.GetStatus() == sdk.Unbonded {
+		// Defensive.
+		// Simulation doesn't take unbonding periods into account, and
+		// Tendermint might break this assumption at some point.
+		return
+	}
+
 	// Double sign too old
 	maxEvidenceAge := k.MaxEvidenceAge(ctx)
 	if age > maxEvidenceAge {
@@ -80,7 +89,6 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, addr crypto.Address, infractio
 	k.validatorSet.Slash(ctx, consAddr, distributionHeight, power, revisedFraction)
 
 	// Jail validator if not already jailed
-	validator := k.validatorSet.ValidatorByConsAddr(ctx, consAddr)
 	if !validator.GetJailed() {
 		k.validatorSet.Jail(ctx, consAddr)
 	}

x/slashing/test_common.go

@@ -87,8 +87,8 @@ func createTestInput(t *testing.T, defaults Params) (sdk.Context, bank.Keeper, s
 	}
 	require.Nil(t, err)
 	paramstore := paramsKeeper.Subspace(DefaultParamspace)
-	keeper := NewKeeper(cdc, keySlashing, sk, paramstore, DefaultCodespace)
-	sk = sk.WithHooks(keeper.Hooks())
+	keeper := NewKeeper(cdc, keySlashing, &sk, paramstore, DefaultCodespace)
+	sk.SetHooks(keeper.Hooks())
 
 	require.NotPanics(t, func() {
 		InitGenesis(ctx, keeper, GenesisState{defaults}, genesis)

x/stake/handler.go

@@ -31,11 +31,27 @@ func NewHandler(k keeper.Keeper) sdk.Handler {
 }
 
 // Called every block, update validator set
-func EndBlocker(ctx sdk.Context, k keeper.Keeper) (ValidatorUpdates []abci.ValidatorUpdate) {
+func EndBlocker(ctx sdk.Context, k keeper.Keeper) (validatorUpdates []abci.ValidatorUpdate) {
 	endBlockerTags := sdk.EmptyTags()
 
+	// Reset the intra-transaction counter.
+	k.SetIntraTxCounter(ctx, 0)
+
+	// Calculate validator set changes.
+	//
+	// NOTE: ApplyAndReturnValidatorSetUpdates has to come before
+	// UnbondAllMatureValidatorQueue.
+	// This fixes a bug when the unbonding period is instant (is the case in
+	// some of the tests). The test expected the validator to be completely
+	// unbonded after the Endblocker (go from Bonded -> Unbonding during
+	// ApplyAndReturnValidatorSetUpdates and then Unbonding -> Unbonded during
+	// UnbondAllMatureValidatorQueue).
+	validatorUpdates = k.ApplyAndReturnValidatorSetUpdates(ctx)
+
+	// Unbond all mature validators from the unbonding queue.
 	k.UnbondAllMatureValidatorQueue(ctx)
 
+	// Remove all mature unbonding delegations from the ubd queue.
 	matureUnbonds := k.DequeueAllMatureUnbondingQueue(ctx, ctx.BlockHeader().Time)
 	for _, dvPair := range matureUnbonds {
 		err := k.CompleteUnbonding(ctx, dvPair.DelegatorAddr, dvPair.ValidatorAddr)
@@ -49,6 +65,7 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) (ValidatorUpdates []abci.Valid
 		))
 	}
 
+	// Remove all mature redelegations from the red queue.
 	matureRedelegations := k.DequeueAllMatureRedelegationQueue(ctx, ctx.BlockHeader().Time)
 	for _, dvvTriplet := range matureRedelegations {
 		err := k.CompleteRedelegation(ctx, dvvTriplet.DelegatorAddr, dvvTriplet.ValidatorSrcAddr, dvvTriplet.ValidatorDstAddr)
@@ -62,12 +79,6 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) (ValidatorUpdates []abci.Valid
 			tags.DstValidator, []byte(dvvTriplet.ValidatorDstAddr.String()),
 		))
 	}
-
-	// reset the intra-transaction counter
-	k.SetIntraTxCounter(ctx, 0)
-
-	// calculate validator set changes
-	ValidatorUpdates = k.ApplyAndReturnValidatorSetUpdates(ctx)
 	return
 }
 

x/stake/handler_test.go

@@ -494,11 +494,12 @@ func TestMultipleMsgCreateValidator(t *testing.T) {
 		got := handleMsgBeginUnbonding(ctx, msgBeginUnbonding, keeper)
 		require.True(t, got.IsOK(), "expected msg %d to be ok, got %v", i, got)
 		var finishTime time.Time
+		// Jump to finishTime for unbonding period and remove from unbonding queue
 		types.MsgCdc.MustUnmarshalBinaryLengthPrefixed(got.Data, &finishTime)
 		ctx = ctx.WithBlockTime(finishTime)
 		EndBlocker(ctx, keeper)
 
-		//Check that the account is unbonded
+		// Check that the validator is deleted from state
 		validators := keeper.GetValidators(ctx, 100)
 		require.Equal(t, len(validatorAddrs)-(i+1), len(validators),
 			"expected %d validators got %d", len(validatorAddrs)-(i+1), len(validators))
@@ -1013,7 +1014,7 @@ func TestBondUnbondRedelegateSlashTwice(t *testing.T) {
 	EndBlocker(ctx, keeper)
 
 	// validator power should have been reduced to zero
-	// ergo validator should have been removed from the store
-	_, found = keeper.GetValidator(ctx, valA)
-	require.False(t, found)
+	// validator should be in unbonding state
+	validator, _ = keeper.GetValidator(ctx, valA)
+	require.Equal(t, validator.GetStatus(), sdk.Unbonding)
 }

x/stake/keeper/delegation.go

@@ -349,6 +349,13 @@ func (k Keeper) DequeueAllMatureRedelegationQueue(ctx sdk.Context, currTime time
 func (k Keeper) Delegate(ctx sdk.Context, delAddr sdk.AccAddress, bondAmt sdk.Coin,
 	validator types.Validator, subtractAccount bool) (newShares sdk.Dec, err sdk.Error) {
 
+	// In some situations, the exchange rate becomes invalid, e.g. if
+	// validator loses all tokens due to slashing.  In this case,
+	// make all future delegations invalid.
+	if validator.DelegatorShareExRate().IsZero() {
+		return sdk.ZeroDec(), types.ErrDelegatorShareExRateInvalid(k.Codespace())
+	}
+
 	// Get or create the delegator delegation
 	delegation, found := k.GetDelegation(ctx, delAddr, validator.OperatorAddr)
 	if !found {

x/stake/keeper/keeper.go

@@ -36,7 +36,7 @@ func NewKeeper(cdc *codec.Codec, key, tkey sdk.StoreKey, ck bank.Keeper, paramst
 }
 
 // Set the validator hooks
-func (k Keeper) WithHooks(sh sdk.StakingHooks) Keeper {
+func (k *Keeper) SetHooks(sh sdk.StakingHooks) *Keeper {
 	if k.hooks != nil {
 		panic("cannot set validator hooks twice")
 	}

x/stake/keeper/slash.go

@@ -108,12 +108,6 @@ func (k Keeper) Slash(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeigh
 	pool.LooseTokens = pool.LooseTokens.Sub(tokensToBurn)
 	k.SetPool(ctx, pool)
 
-	// remove validator if it has no more tokens
-	if validator.DelegatorShares.IsZero() && validator.Status == sdk.Unbonded {
-		// if not unbonded, we must instead remove validator in EndBlocker once it finishes its unbonding period
-		k.RemoveValidator(ctx, validator.OperatorAddr)
-	}
-
 	// Log that a slash occurred!
 	logger.Info(fmt.Sprintf(
 		"validator %s slashed by slash factor of %s; burned %v tokens",
@@ -236,6 +230,7 @@ func (k Keeper) slashRedelegation(ctx sdk.Context, validator types.Validator, re
 		if sharesToUnbond.GT(delegation.Shares) {
 			sharesToUnbond = delegation.Shares
 		}
+
 		tokensToBurn, err := k.unbond(ctx, redelegation.DelegatorAddr, redelegation.ValidatorDstAddr, sharesToUnbond)
 		if err != nil {
 			panic(fmt.Errorf("error unbonding delegator: %v", err))

x/stake/keeper/slash_test.go

@@ -348,9 +348,9 @@ func TestSlashWithUnbondingDelegation(t *testing.T) {
 	keeper.ApplyAndReturnValidatorSetUpdates(ctx)
 	// read updated validator
 	// power decreased by 1 again, validator is out of stake
-	// ergo validator should have been removed from the store
-	_, found = keeper.GetValidatorByConsAddr(ctx, consAddr)
-	require.False(t, found)
+	// validator should be in unbonding period
+	validator, _ = keeper.GetValidatorByConsAddr(ctx, consAddr)
+	require.Equal(t, validator.GetStatus(), sdk.Unbonding)
 }
 
 // tests Slash at a previous height with a redelegation
@@ -450,16 +450,16 @@ func TestSlashWithRedelegation(t *testing.T) {
 	// apply TM updates
 	keeper.ApplyAndReturnValidatorSetUpdates(ctx)
 	// read updated validator
-	// validator decreased to zero power, should have been removed from the store
-	_, found = keeper.GetValidatorByConsAddr(ctx, consAddr)
-	require.False(t, found)
+	// validator decreased to zero power, should be in unbonding period
+	validator, _ = keeper.GetValidatorByConsAddr(ctx, consAddr)
+	require.Equal(t, validator.GetStatus(), sdk.Unbonding)
 
 	// slash the validator again, by 100%
 	// no stake remains to be slashed
 	ctx = ctx.WithBlockHeight(12)
-	// validator no longer in the store
-	_, found = keeper.GetValidatorByConsAddr(ctx, consAddr)
-	require.False(t, found)
+	// validator still in unbonding period
+	validator, _ = keeper.GetValidatorByConsAddr(ctx, consAddr)
+	require.Equal(t, validator.GetStatus(), sdk.Unbonding)
 	keeper.Slash(ctx, consAddr, 10, 10, sdk.OneDec())
 
 	// read updating redelegation
@@ -472,9 +472,9 @@ func TestSlashWithRedelegation(t *testing.T) {
 	// no more bonded tokens burned
 	require.Equal(t, int64(16), oldPool.BondedTokens.Sub(newPool.BondedTokens).RoundInt64())
 	// read updated validator
-	// power still zero, still not in the store
-	_, found = keeper.GetValidatorByConsAddr(ctx, consAddr)
-	require.False(t, found)
+	// power still zero, still in unbonding period
+	validator, _ = keeper.GetValidatorByConsAddr(ctx, consAddr)
+	require.Equal(t, validator.GetStatus(), sdk.Unbonding)
 }
 
 // tests Slash at a previous height with both an unbonding delegation and a redelegation

x/stake/keeper/val_state_change.go

@@ -78,8 +78,8 @@ func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx sdk.Context) (updates []ab
 		if !found || !bytes.Equal(oldPowerBytes, newPowerBytes) {
 			updates = append(updates, validator.ABCIValidatorUpdate())
 
-			// XXX Assert that the validator had updated its ValidatorDistInfo.FeePoolWithdrawalHeight.
-			// XXX This hook probably shouldn't exist.  Maybe rethink the hook system.
+			// Assert that the validator had updated its ValidatorDistInfo.FeePoolWithdrawalHeight.
+			// This hook is extremely useful, otherwise lazy accum bugs will be difficult to solve.
 			if k.hooks != nil {
 				k.hooks.OnValidatorPowerDidChange(ctx, validator.ConsAddress(), valAddr)
 			}
@@ -108,11 +108,6 @@ func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx sdk.Context) (updates []ab
 		// bonded to unbonding
 		k.bondedToUnbonding(ctx, validator)
 
-		// remove validator if it has no more tokens
-		if validator.Tokens.IsZero() {
-			k.RemoveValidator(ctx, validator.OperatorAddr)
-		}
-
 		// delete from the bonded validator index
 		k.DeleteLastValidatorPower(ctx, sdk.ValAddress(valAddrBytes))
 

x/stake/keeper/validator.go

@@ -155,6 +155,7 @@ func (k Keeper) RemoveValidatorTokensAndShares(ctx sdk.Context, validator types.
 
 // Update the tokens of an existing validator, update the validators power index key
 func (k Keeper) RemoveValidatorTokens(ctx sdk.Context, validator types.Validator, tokensToRemove sdk.Dec) types.Validator {
+
 	pool := k.GetPool(ctx)
 	k.DeleteValidatorByPowerIndex(ctx, validator, pool)
 	validator, pool = validator.RemoveTokens(pool, tokensToRemove)
@@ -189,6 +190,9 @@ func (k Keeper) RemoveValidator(ctx sdk.Context, address sdk.ValAddress) {
 	if !found {
 		return
 	}
+	if validator.Status != sdk.Unbonded {
+		panic("Cannot call RemoveValidator on bonded or unbonding validators")
+	}
 
 	// delete the old validator record
 	store := ctx.KVStore(k.storeKey)
@@ -357,10 +361,9 @@ func (k Keeper) UnbondAllMatureValidatorQueue(ctx sdk.Context) {
 			if !found || val.GetStatus() != sdk.Unbonding {
 				continue
 			}
+			k.unbondingToUnbonded(ctx, val)
 			if val.GetDelegatorShares().IsZero() {
 				k.RemoveValidator(ctx, val.OperatorAddr)
-			} else {
-				k.unbondingToUnbonded(ctx, val)
 			}
 		}
 		store.Delete(validatorTimesliceIterator.Key())