R4R: Results from x/staking & x/slashing peer review

PENDING.md

@@ -84,6 +84,7 @@ IMPROVEMENTS
   for tx size in the ante handler.
   * [\#3454](https://github.com/cosmos/cosmos-sdk/pull/3454) Add `--jail-whitelist` to `gaiad export` to enable testing of complex exports
   * [\#3424](https://github.com/cosmos/cosmos-sdk/issues/3424) Allow generation of gentxs with empty memo field.
+  * \#3507 General cleanup, removal of unnecessary struct fields, undelegation bugfix, and comment clarification in x/staking and x/slashing
 
 * SDK
   * [\#2605] x/params add subkey accessing

client/lcd/lcd_test.go

@@ -646,7 +646,6 @@ func TestBonding(t *testing.T) {
 	redelegation := getRedelegations(t, port, addr, operAddrs[0], operAddrs[1])
 	require.Len(t, redelegation, 1)
 	require.Len(t, redelegation[0].Entries, 1)
-	require.Equal(t, rdTokens, redelegation[0].Entries[0].Balance.Amount)
 
 	delegatorUbds := getDelegatorUnbondingDelegations(t, port, addr)
 	require.Len(t, delegatorUbds, 1)
@@ -656,7 +655,6 @@ func TestBonding(t *testing.T) {
 	delegatorReds := getRedelegations(t, port, addr, nil, nil)
 	require.Len(t, delegatorReds, 1)
 	require.Len(t, delegatorReds[0].Entries, 1)
-	require.Equal(t, rdTokens, delegatorReds[0].Entries[0].Balance.Amount)
 
 	validatorUbds := getValidatorUnbondingDelegations(t, port, operAddrs[0])
 	require.Len(t, validatorUbds, 1)
@@ -666,7 +664,6 @@ func TestBonding(t *testing.T) {
 	validatorReds := getRedelegations(t, port, nil, operAddrs[0], nil)
 	require.Len(t, validatorReds, 1)
 	require.Len(t, validatorReds[0].Entries, 1)
-	require.Equal(t, rdTokens, validatorReds[0].Entries[0].Balance.Amount)
 
 	// TODO Undonding status not currently implemented
 	// require.Equal(t, sdk.Unbonding, bondedValidators[0].Status)

cmd/gaia/app/app.go

@@ -355,10 +355,6 @@ func (h StakingHooks) AfterValidatorBonded(ctx sdk.Context, consAddr sdk.ConsAdd
 	h.dh.AfterValidatorBonded(ctx, consAddr, valAddr)
 	h.sh.AfterValidatorBonded(ctx, consAddr, valAddr)
 }
-func (h StakingHooks) AfterValidatorPowerDidChange(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
-	h.dh.AfterValidatorPowerDidChange(ctx, consAddr, valAddr)
-	h.sh.AfterValidatorPowerDidChange(ctx, consAddr, valAddr)
-}
 func (h StakingHooks) AfterValidatorBeginUnbonding(ctx sdk.Context, consAddr sdk.ConsAddress, valAddr sdk.ValAddress) {
 	h.dh.AfterValidatorBeginUnbonding(ctx, consAddr, valAddr)
 	h.sh.AfterValidatorBeginUnbonding(ctx, consAddr, valAddr)

cmd/gaia/app/export.go

@@ -150,7 +150,6 @@ func (app *GaiaApp) prepForZeroHeightGenesis(ctx sdk.Context, jailWhiteList []st
 			panic("expected validator, not found")
 		}
 
-		validator.BondHeight = 0
 		validator.UnbondingHeight = 0
 		valConsAddrs = append(valConsAddrs, validator.ConsAddress())
 		if applyWhiteList && !whiteListMap[addr.String()] {

docs/spec/staking/hooks.md

@@ -15,8 +15,6 @@ following hooks can registered with staking:
    - called when a validator is bonded
  - `AfterValidatorBeginUnbonding(Context, ConsAddress, ValAddress)`
    - called when a validator begins unbonding
- - `AfterValidatorPowerDidChange(Context, ConsAddress, ValAddress)`
-   - called at EndBlock when a validator's power is changed
  - `BeforeDelegationCreated(Context, AccAddress, ValAddress)`
    - called when a delegation is created
  - `BeforeDelegationSharesModified(Context, AccAddress, ValAddress)`

docs/spec/staking/state.md

@@ -187,7 +187,6 @@ type RedelegationEntry struct {
     CompletionTime time.Time // unix time for redelegation completion
     InitialBalance sdk.Coin  // initial balance when redelegation started
     Balance        sdk.Coin  // current balance (current value held in destination validator)
-    SharesSrc      sdk.Dec   // amount of source-validator shares removed by redelegation
     SharesDst      sdk.Dec   // amount of destination-validator shares created by redelegation
 }
 ```

types/int.go

@@ -18,6 +18,8 @@ func gt(i *big.Int, i2 *big.Int) bool { return i.Cmp(i2) == 1 }
 
 func lt(i *big.Int, i2 *big.Int) bool { return i.Cmp(i2) == -1 }
 
+func lte(i *big.Int, i2 *big.Int) bool { return i.Cmp(i2) <= 0 }
+
 func add(i *big.Int, i2 *big.Int) *big.Int { return new(big.Int).Add(i, i2) }
 
 func sub(i *big.Int, i2 *big.Int) *big.Int { return new(big.Int).Sub(i, i2) }
@@ -191,6 +193,11 @@ func (i Int) LT(i2 Int) bool {
 	return lt(i.i, i2.i)
 }
 
+// LTE returns true if first Int is less than or equal to second
+func (i Int) LTE(i2 Int) bool {
+	return lte(i.i, i2.i)
+}
+
 // Add adds Int from another
 func (i Int) Add(i2 Int) (res Int) {
 	res = Int{add(i.i, i2.i)}

types/stake.go

@@ -47,7 +47,6 @@ type Validator interface {
 	GetTendermintPower() int64    // validation power in tendermint
 	GetCommission() Dec           // validator commission rate
 	GetDelegatorShares() Dec      // total outstanding delegator shares
-	GetBondHeight() int64         // height in which the validator became active
 	GetDelegatorShareExRate() Dec // tokens per delegator share exchange rate
 }
 
@@ -123,7 +122,6 @@ type StakingHooks interface {
 
 	AfterValidatorBonded(ctx Context, consAddr ConsAddress, valAddr ValAddress)         // Must be called when a validator is bonded
 	AfterValidatorBeginUnbonding(ctx Context, consAddr ConsAddress, valAddr ValAddress) // Must be called when a validator begins unbonding
-	AfterValidatorPowerDidChange(ctx Context, consAddr ConsAddress, valAddr ValAddress) // Called at EndBlock when a validator's power did change
 
 	BeforeDelegationCreated(ctx Context, delAddr AccAddress, valAddr ValAddress)        // Must be called when a delegation is created
 	BeforeDelegationSharesModified(ctx Context, delAddr AccAddress, valAddr ValAddress) // Must be called when a delegation's shares are modified

x/distribution/keeper/hooks.go

@@ -82,8 +82,6 @@ func (h Hooks) AfterValidatorBeginUnbonding(ctx sdk.Context, _ sdk.ConsAddress,
 }
 func (h Hooks) AfterValidatorBonded(ctx sdk.Context, _ sdk.ConsAddress, valAddr sdk.ValAddress) {
 }
-func (h Hooks) AfterValidatorPowerDidChange(ctx sdk.Context, _ sdk.ConsAddress, valAddr sdk.ValAddress) {
-}
 func (h Hooks) BeforeValidatorSlashed(ctx sdk.Context, valAddr sdk.ValAddress, fraction sdk.Dec) {
 	// record the slash event
 	h.k.updateValidatorSlashFraction(ctx, valAddr, fraction)

x/slashing/client/module_client.go

@@ -5,6 +5,7 @@ import (
 	amino "github.com/tendermint/go-amino"
 
 	"github.com/cosmos/cosmos-sdk/client"
+	"github.com/cosmos/cosmos-sdk/x/slashing"
 	"github.com/cosmos/cosmos-sdk/x/slashing/client/cli"
 )
 
@@ -22,7 +23,7 @@ func NewModuleClient(storeKey string, cdc *amino.Codec) ModuleClient {
 func (mc ModuleClient) GetQueryCmd() *cobra.Command {
 	// Group slashing queries under a subcommand
 	slashingQueryCmd := &cobra.Command{
-		Use:   "slashing",
+		Use:   slashing.ModuleName,
 		Short: "Querying commands for the slashing module",
 	}
 
@@ -40,7 +41,7 @@ func (mc ModuleClient) GetQueryCmd() *cobra.Command {
 // GetTxCmd returns the transaction commands for this module
 func (mc ModuleClient) GetTxCmd() *cobra.Command {
 	slashingTxCmd := &cobra.Command{
-		Use:   "slashing",
+		Use:   slashing.ModuleName,
 		Short: "Slashing transactions subcommands",
 	}
 

x/slashing/errors.go

@@ -10,7 +10,7 @@ type CodeType = sdk.CodeType
 
 const (
 	// Default slashing codespace
-	DefaultCodespace sdk.CodespaceType = "SLASH"
+	DefaultCodespace sdk.CodespaceType = ModuleName
 
 	CodeInvalidValidator      CodeType = 101
 	CodeValidatorJailed       CodeType = 102

x/slashing/genesis.go

@@ -21,7 +21,7 @@ type MissedBlock struct {
 	Missed bool  `json:"missed"`
 }
 
-// HubDefaultGenesisState - default GenesisState used by Cosmos Hub
+// DefaultGenesisState - default GenesisState used by Cosmos Hub
 func DefaultGenesisState() GenesisState {
 	return GenesisState{
 		Params:       DefaultParams(),

x/slashing/handler.go

@@ -31,6 +31,7 @@ func handleMsgUnjail(ctx sdk.Context, msg MsgUnjail, k Keeper) sdk.Result {
 		return ErrMissingSelfDelegation(k.codespace).Result()
 	}
 
+	// cannot be unjailed if not jailed
 	if !validator.GetJailed() {
 		return ErrValidatorNotJailed(k.codespace).Result()
 	}
@@ -42,6 +43,7 @@ func handleMsgUnjail(ctx sdk.Context, msg MsgUnjail, k Keeper) sdk.Result {
 		return ErrNoValidatorForAddress(k.codespace).Result()
 	}
 
+	// cannot be unjailed if tombstoned
 	if info.Tombstoned {
 		return ErrValidatorJailed(k.codespace).Result()
 	}

x/slashing/hooks.go

@@ -66,7 +66,6 @@ func (h Hooks) AfterValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) {
 
 // nolint - unused hooks
 func (h Hooks) AfterValidatorBeginUnbonding(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress)  {}
-func (h Hooks) AfterValidatorPowerDidChange(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress)  {}
 func (h Hooks) BeforeValidatorModified(_ sdk.Context, _ sdk.ValAddress)                          {}
 func (h Hooks) BeforeDelegationCreated(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress)        {}
 func (h Hooks) BeforeDelegationSharesModified(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) {}

x/slashing/keeper.go

@@ -39,15 +39,19 @@ func NewKeeper(cdc *codec.Codec, key sdk.StoreKey, vs sdk.ValidatorSet, paramspa
 // power: power of the double-signing validator at the height of infraction
 func (k Keeper) handleDoubleSign(ctx sdk.Context, addr crypto.Address, infractionHeight int64, timestamp time.Time, power int64) {
 	logger := ctx.Logger().With("module", "x/slashing")
+
+	// calculate the age of the evidence
 	time := ctx.BlockHeader().Time
 	age := time.Sub(timestamp)
+
+	// fetch the validator public key
 	consAddr := sdk.ConsAddress(addr)
 	pubkey, err := k.getPubkey(ctx, addr)
 	if err != nil {
 		panic(fmt.Sprintf("Validator consensus-address %v not found", consAddr))
 	}
 
-	// Reject evidence if the double is too old
+	// Reject evidence if the double-sign is too old
 	if age > k.MaxEvidenceAge(ctx) {
 		logger.Info(fmt.Sprintf("Ignored double sign from %s at height %d, age of %d past max age of %d",
 			pubkey.Address(), infractionHeight, age, k.MaxEvidenceAge(ctx)))
@@ -62,18 +66,20 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, addr crypto.Address, infractio
 		// Tendermint might break this assumption at some point.
 		return
 	}
+
+	// fetch the validator signing info
 	signInfo, found := k.getValidatorSigningInfo(ctx, consAddr)
 	if !found {
 		panic(fmt.Sprintf("Expected signing info for validator %s but not found", consAddr))
 	}
 
-	// Validator is already tombstoned
+	// validator is already tombstoned
 	if signInfo.Tombstoned {
 		logger.Info(fmt.Sprintf("Ignored double sign from %s at height %d, validator already tombstoned", pubkey.Address(), infractionHeight))
 		return
 	}
 
-	// Double sign confirmed
+	// double sign confirmed
 	logger.Info(fmt.Sprintf("Confirmed double sign from %s at height %d, age of %d", pubkey.Address(), infractionHeight, age))
 
 	// We need to retrieve the stake distribution which signed the block, so we subtract ValidatorUpdateDelay from the evidence height.
@@ -98,7 +104,7 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, addr crypto.Address, infractio
 		k.validatorSet.Jail(ctx, consAddr)
 	}
 
-	// Set slashed so far to total slash
+	// Set tombstoned to be true
 	signInfo.Tombstoned = true
 
 	// Set jailed until to be forever (max time)
@@ -118,12 +124,15 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p
 	if err != nil {
 		panic(fmt.Sprintf("Validator consensus-address %v not found", consAddr))
 	}
-	// Local index, so counts blocks validator *should* have signed
-	// Will use the 0-value default signing info if not present, except for start height
+
+	// fetch signing info
 	signInfo, found := k.getValidatorSigningInfo(ctx, consAddr)
 	if !found {
 		panic(fmt.Sprintf("Expected signing info for validator %s but not found", consAddr))
 	}
+
+	// this is a relative index, so it counts blocks the validator *should* have signed
+	// will use the 0-value default signing info if not present, except for start height
 	index := signInfo.IndexOffset % k.SignedBlocksWindow(ctx)
 	signInfo.IndexOffset++
 
@@ -148,14 +157,19 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p
 	if missed {
 		logger.Info(fmt.Sprintf("Absent validator %s at height %d, %d missed, threshold %d", addr, height, signInfo.MissedBlocksCounter, k.MinSignedPerWindow(ctx)))
 	}
+
 	minHeight := signInfo.StartHeight + k.SignedBlocksWindow(ctx)
 	maxMissed := k.SignedBlocksWindow(ctx) - k.MinSignedPerWindow(ctx)
+
+	// if we are past the minimum height and the validator has missed too many blocks, punish them
 	if height > minHeight && signInfo.MissedBlocksCounter > maxMissed {
 		validator := k.validatorSet.ValidatorByConsAddr(ctx, consAddr)
 		if validator != nil && !validator.GetJailed() {
+
 			// Downtime confirmed: slash and jail the validator
 			logger.Info(fmt.Sprintf("Validator %s past min height of %d and below signed blocks threshold of %d",
 				pubkey.Address(), minHeight, k.MinSignedPerWindow(ctx)))
+
 			// We need to retrieve the stake distribution which signed the block, so we subtract ValidatorUpdateDelay from the evidence height,
 			// and subtract an additional 1 since this is the LastCommit.
 			// Note that this *can* result in a negative "distributionHeight" up to -ValidatorUpdateDelay-1,
@@ -165,6 +179,7 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p
 			k.validatorSet.Slash(ctx, consAddr, distributionHeight, power, k.SlashFractionDowntime(ctx))
 			k.validatorSet.Jail(ctx, consAddr)
 			signInfo.JailedUntil = ctx.BlockHeader().Time.Add(k.DowntimeJailDuration(ctx))
+
 			// We need to reset the counter & array so that the validator won't be immediately slashed for downtime upon rebonding.
 			signInfo.MissedBlocksCounter = 0
 			signInfo.IndexOffset = 0

x/slashing/keys.go

@@ -8,14 +8,17 @@ import (
 )
 
 const (
+	// ModuleName is the name of the module
+	ModuleName = "slashing"
+
 	// StoreKey is the store key string for slashing
-	StoreKey = "slashing"
+	StoreKey = ModuleName
 
 	// RouterKey is the message route for slashing
-	RouterKey = "slashing"
+	RouterKey = ModuleName
 
 	// QuerierRoute is the querier route for slashing
-	QuerierRoute = "slashing"
+	QuerierRoute = ModuleName
 )
 
 // key prefix bytes