R4R: Offset validator set power calculations by one block

.circleci/config.yml

@@ -136,6 +136,24 @@ jobs:
             export PATH="$GOBIN:$PATH"
             make test_sim_gaia_fast
 
+  test_sim_gaia_multi_seed:
+    <<: *defaults
+    parallelism: 1
+    steps:
+      - attach_workspace:
+          at: /tmp/workspace
+      - checkout
+      - run:
+          name: dependencies
+          command: |
+            export PATH="$GOBIN:$PATH"
+            make get_vendor_deps
+      - run:
+          name: Test multi-seed Gaia simulation
+          command: |
+            export PATH="$GOBIN:$PATH"
+            make test_sim_gaia_multi_seed
+
   test_cover:
     <<: *defaults
     parallelism: 4
@@ -240,6 +258,9 @@ workflows:
       - test_sim_gaia_fast:
           requires:
             - setup_dependencies
+      - test_sim_gaia_multi_seed:
+          requires:
+            - setup_dependencies
       - test_cover:
           requires:
             - setup_dependencies

Makefile

@@ -162,9 +162,9 @@ test_sim_gaia_fast:
 	@echo "Running quick Gaia simulation. This may take several minutes..."
 	@go test ./cmd/gaia/app -run TestFullGaiaSimulation -SimulationEnabled=true -SimulationNumBlocks=400 -SimulationBlockSize=200 -SimulationCommit=true -v -timeout 24h
 
-test_sim_gaia_full:
-	@echo "Running full multi-seed Gaia simulation. This may take awhile!"
-	@sh scripts/multisim.sh
+test_sim_gaia_multi_seed:
+	@echo "Running multi-seed Gaia simulation. This may take awhile!"
+	@bash scripts/multisim.sh 10
 
 SIM_NUM_BLOCKS ?= 210
 SIM_BLOCK_SIZE ?= 200
@@ -241,4 +241,4 @@ localnet-stop:
 check_tools check_dev_tools get_tools get_dev_tools get_vendor_deps draw_deps test test_cli test_unit \
 test_cover test_lint benchmark devdoc_init devdoc devdoc_save devdoc_update \
 build-linux build-docker-gaiadnode localnet-start localnet-stop \
-format check-ledger test_sim_gaia_nondeterminism test_sim_modules test_sim_gaia_fast test_sim_gaia_slow update_tools update_dev_tools
+format check-ledger test_sim_gaia_nondeterminism test_sim_modules test_sim_gaia_fast test_sim_gaia_multi_seed update_tools update_dev_tools

PENDING.md

@@ -28,6 +28,7 @@ BREAKING CHANGES
     * [x/stake, x/slashing] [#1305](https://github.com/cosmos/cosmos-sdk/issues/1305) - Rename "revoked" to "jailed"
     * [x/stake] [#1676] Revoked and jailed validators put into the unbonding state
     * [x/stake] [#1877] Redelegations/unbonding-delegation from unbonding validator have reduced time
+    * [x/slashing] \#1789 Slashing changes for Tendermint validator set offset (NextValSet)
     * [x/stake] [\#2040](https://github.com/cosmos/cosmos-sdk/issues/2040) Validator
     operator type has now changed to `sdk.ValAddress`
     * [x/stake] [\#2221](https://github.com/cosmos/cosmos-sdk/issues/2221) New

client/rpc/validators.go

@@ -11,10 +11,10 @@ import (
 
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/client/context"
+	"github.com/cosmos/cosmos-sdk/client/utils"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/spf13/viper"
 	tmtypes "github.com/tendermint/tendermint/types"
-	"github.com/cosmos/cosmos-sdk/client/utils"
 )
 
 // TODO these next two functions feel kinda hacky based on their placement

scripts/multisim.sh

@@ -1,12 +1,14 @@
 #!/bin/bash
 
 seeds=(1 2 4 7 9 20 32 123 4728 37827 981928 87821 891823782 989182 89182391)
+blocks=$1
 
-echo "Running multi-seed simulation with seeds: ${seeds[@]}"
+echo "Running multi-seed simulation with seeds ${seeds[@]}"
+echo "Running $blocks blocks per seed"
 echo "Edit scripts/multisim.sh to add new seeds. Keeping parameters in the file makes failures easy to reproduce."
-echo "This script will kill all sub-simulations on SIGINT/SIGTERM/EXIT (i.e. Ctrl-C)."
+echo "This script will kill all sub-simulations on SIGINT/SIGTERM (i.e. Ctrl-C)."
 
-trap 'kill $(jobs -pr)' SIGINT SIGTERM EXIT
+trap 'kill $(jobs -pr)' SIGINT SIGTERM
 
 tmpdir=$(mktemp -d)
 echo "Using temporary log directory: $tmpdir"
@@ -16,7 +18,7 @@ sim() {
 	echo "Running full Gaia simulation with seed $seed. This may take awhile!"
   file="$tmpdir/gaia-simulation-seed-$seed-date-$(date -Iseconds -u).stdout"
   echo "Writing stdout to $file..."
-	go test ./cmd/gaia/app -run TestFullGaiaSimulation -SimulationEnabled=true -SimulationNumBlocks=1000 \
+	go test ./cmd/gaia/app -run TestFullGaiaSimulation -SimulationEnabled=true -SimulationNumBlocks=$blocks \
     -SimulationVerbose=true -SimulationCommit=true -SimulationSeed=$seed -v -timeout 24h > $file
 }
 
@@ -26,7 +28,7 @@ for seed in ${seeds[@]}; do
   sim $seed &
   pids[${i}]=$!
   i=$(($i+1))
-  sleep 0.1 # start in order, nicer logs
+  sleep 5 # start in order, nicer logs
 done
 
 echo "Simulation processes spawned, waiting for completion..."
@@ -37,10 +39,12 @@ i=0
 for pid in ${pids[*]}; do
   wait $pid
   last=$?
+  seed=${seeds[${i}]}
   if [ $last -ne 0 ]; then
-    seed=${seeds[${i}]}
     echo "Simulation with seed $seed failed!"
     code=1
+  else
+    echo "Simulation with seed $seed OK!"
   fi
   i=$(($i+1))
 done

x/distribution/types/validator_info.go

@@ -19,9 +19,9 @@ func NewValidatorDistInfo(operatorAddr sdk.ValAddress, currentHeight int64) Vali
 	return ValidatorDistInfo{
 		OperatorAddr:            operatorAddr,
 		FeePoolWithdrawalHeight: currentHeight,
-		Pool:           DecCoins{},
-		PoolCommission: DecCoins{},
-		DelAccum:       NewTotalAccum(currentHeight),
+		Pool:                    DecCoins{},
+		PoolCommission:          DecCoins{},
+		DelAccum:                NewTotalAccum(currentHeight),
 	}
 }
 

x/mock/simulation/random_simulate_blocks.go

@@ -77,6 +77,9 @@ func SimulateFromSeed(tb testing.TB, app *baseapp.BaseApp,
 	}
 
 	validators := initChain(r, accs, setups, app, appStateFn)
+	// Second variable to keep pending validator set (delayed one block since TM 0.24)
+	// Initially this is the same as the initial validator set
+	nextValidators := validators
 
 	header := abci.Header{Height: 0, Time: timestamp}
 	opCount := 0
@@ -160,8 +163,9 @@ func SimulateFromSeed(tb testing.TB, app *baseapp.BaseApp,
 		// Generate a random RequestBeginBlock with the current validator set for the next block
 		request = RandomRequestBeginBlock(r, validators, livenessTransitionMatrix, evidenceFraction, pastTimes, pastVoteInfos, event, header)
 
-		// Update the validator set
-		validators = updateValidators(tb, r, validators, res.ValidatorUpdates, event)
+		// Update the validator set, which will be reflected in the application on the next block
+		validators = nextValidators
+		nextValidators = updateValidators(tb, r, validators, res.ValidatorUpdates, event)
 	}
 	if stopEarly {
 		DisplayEvents(events)

x/slashing/keeper.go

@@ -56,17 +56,26 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, addr crypto.Address, infractio
 	// Double sign confirmed
 	logger.Info(fmt.Sprintf("Confirmed double sign from %s at height %d, age of %d less than max age of %d", pubkey.Address(), infractionHeight, age, maxEvidenceAge))
 
+	// We need to retrieve the stake distribution which signed the block, so we subtract ValidatorUpdateDelay from the evidence height.
+	// Note that this *can* result in a "distributionHeight" of -1,
+	// i.e. at the end of the pre-genesis block (none) = at the beginning of the genesis block.
+	// That's fine since this is just used to filter unbonding delegations & redelegations.
+	distributionHeight := infractionHeight - ValidatorUpdateDelay
+
 	// Cap the amount slashed to the penalty for the worst infraction
 	// within the slashing period when this infraction was committed
 	fraction := k.SlashFractionDoubleSign(ctx)
-	revisedFraction := k.capBySlashingPeriod(ctx, consAddr, fraction, infractionHeight)
+	revisedFraction := k.capBySlashingPeriod(ctx, consAddr, fraction, distributionHeight)
 	logger.Info(fmt.Sprintf("Fraction slashed capped by slashing period from %v to %v", fraction, revisedFraction))
 
 	// Slash validator
-	k.validatorSet.Slash(ctx, consAddr, infractionHeight, power, revisedFraction)
+	k.validatorSet.Slash(ctx, consAddr, distributionHeight, power, revisedFraction)
 
-	// Jail validator
-	k.validatorSet.Jail(ctx, consAddr)
+	// Jail validator if not already jailed
+	validator := k.validatorSet.ValidatorByConsAddr(ctx, consAddr)
+	if !validator.GetJailed() {
+		k.validatorSet.Jail(ctx, consAddr)
+	}
 
 	// Set validator jail duration
 	signInfo, found := k.getValidatorSigningInfo(ctx, consAddr)
@@ -123,7 +132,13 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p
 			// 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)))
-			k.validatorSet.Slash(ctx, consAddr, height, power, k.SlashFractionDowntime(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 "distributionHeight" of -1 or -2,
+			// i.e. at the end of the pre-genesis block (none) = at the beginning of the genesis block.
+			// That's fine since this is just used to filter unbonding delegations & redelegations.
+			distributionHeight := height - ValidatorUpdateDelay - 1
+			k.validatorSet.Slash(ctx, consAddr, distributionHeight, power, k.SlashFractionDowntime(ctx))
 			k.validatorSet.Jail(ctx, consAddr)
 			signInfo.JailedUntil = ctx.BlockHeader().Time.Add(k.DowntimeUnbondDuration(ctx))
 		} else {

x/slashing/keeper_test.go

@@ -22,20 +22,21 @@ func init() {
 
 // Test that a validator is slashed correctly
 // when we discover evidence of infraction
-// TODO fix this test to not be using the same pubkey/address for signing and operating, it's confusing
 func TestHandleDoubleSign(t *testing.T) {
 
 	// initial setup
 	ctx, ck, sk, _, keeper := createTestInput(t)
+	// validator added pre-genesis
+	ctx = ctx.WithBlockHeight(-1)
 	sk = sk.WithHooks(keeper.Hooks())
 	amtInt := int64(100)
-	addr, val, amt := addrs[0], pks[0], sdk.NewInt(amtInt)
-	got := stake.NewHandler(sk)(ctx, newTestMsgCreateValidator(addr, val, amt))
+	operatorAddr, val, amt := addrs[0], pks[0], sdk.NewInt(amtInt)
+	got := stake.NewHandler(sk)(ctx, newTestMsgCreateValidator(operatorAddr, val, amt))
 	require.True(t, got.IsOK())
 	validatorUpdates := stake.EndBlocker(ctx, sk)
 	keeper.AddValidators(ctx, validatorUpdates)
-	require.Equal(t, ck.GetCoins(ctx, sdk.AccAddress(addr)), sdk.Coins{{sk.GetParams(ctx).BondDenom, initCoins.Sub(amt)}})
-	require.True(t, sdk.NewDecFromInt(amt).Equal(sk.Validator(ctx, addr).GetPower()))
+	require.Equal(t, ck.GetCoins(ctx, sdk.AccAddress(operatorAddr)), sdk.Coins{{sk.GetParams(ctx).BondDenom, initCoins.Sub(amt)}})
+	require.True(t, sdk.NewDecFromInt(amt).Equal(sk.Validator(ctx, operatorAddr).GetPower()))
 
 	// handle a signature to set signing info
 	keeper.handleValidatorSignature(ctx, val.Address(), amtInt, true)
@@ -44,88 +45,88 @@ func TestHandleDoubleSign(t *testing.T) {
 	keeper.handleDoubleSign(ctx, val.Address(), 0, time.Unix(0, 0), amtInt)
 
 	// should be jailed
-	require.True(t, sk.Validator(ctx, addr).GetJailed())
+	require.True(t, sk.Validator(ctx, operatorAddr).GetJailed())
 	// unjail to measure power
-	sk.Unjail(ctx, sdk.ConsAddress(addr)) // TODO distinguish cons address
+	sk.Unjail(ctx, sdk.ConsAddress(val.Address()))
 	// power should be reduced
 	require.Equal(
 		t, sdk.NewDecFromInt(amt).Mul(sdk.NewDec(19).Quo(sdk.NewDec(20))),
-		sk.Validator(ctx, addr).GetPower(),
+		sk.Validator(ctx, operatorAddr).GetPower(),
 	)
 	ctx = ctx.WithBlockHeader(abci.Header{Time: time.Unix(1, 0).Add(keeper.MaxEvidenceAge(ctx))})
 
 	// double sign past max age
 	keeper.handleDoubleSign(ctx, val.Address(), 0, time.Unix(0, 0), amtInt)
 	require.Equal(
 		t, sdk.NewDecFromInt(amt).Mul(sdk.NewDec(19).Quo(sdk.NewDec(20))),
-		sk.Validator(ctx, addr).GetPower(),
+		sk.Validator(ctx, operatorAddr).GetPower(),
 	)
 }
 
 // Test that the amount a validator is slashed for multiple double signs
 // is correctly capped by the slashing period in which they were committed
-// TODO properly distinguish between consensus and operator address is variable names
 func TestSlashingPeriodCap(t *testing.T) {
 
 	// initial setup
 	ctx, ck, sk, _, keeper := createTestInput(t)
 	sk = sk.WithHooks(keeper.Hooks())
 	amtInt := int64(100)
-	addr, amt := addrs[0], sdk.NewInt(amtInt)
-	valConsPubKey, valConsAddr := pks[0], sdk.ConsAddress(pks[0].Address())
-	got := stake.NewHandler(sk)(ctx, newTestMsgCreateValidator(addr, valConsPubKey, amt))
+	operatorAddr, amt := addrs[0], sdk.NewInt(amtInt)
+	valConsPubKey, valConsAddr := pks[0], pks[0].Address()
+	got := stake.NewHandler(sk)(ctx, newTestMsgCreateValidator(operatorAddr, valConsPubKey, amt))
 	require.True(t, got.IsOK())
 	validatorUpdates := stake.EndBlocker(ctx, sk)
+	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1)
 	keeper.AddValidators(ctx, validatorUpdates)
-	require.Equal(t, ck.GetCoins(ctx, sdk.AccAddress(addr)), sdk.Coins{{sk.GetParams(ctx).BondDenom, initCoins.Sub(amt)}})
-	require.True(t, sdk.NewDecFromInt(amt).Equal(sk.Validator(ctx, addr).GetPower()))
+	require.Equal(t, ck.GetCoins(ctx, sdk.AccAddress(operatorAddr)), sdk.Coins{{sk.GetParams(ctx).BondDenom, initCoins.Sub(amt)}})
+	require.True(t, sdk.NewDecFromInt(amt).Equal(sk.Validator(ctx, operatorAddr).GetPower()))
 
 	// handle a signature to set signing info
-	keeper.handleValidatorSignature(ctx, valConsPubKey.Address(), amtInt, true)
+	keeper.handleValidatorSignature(ctx, valConsAddr, amtInt, true)
 
 	// double sign less than max age
-	keeper.handleDoubleSign(ctx, valConsPubKey.Address(), 0, time.Unix(0, 0), amtInt)
+	keeper.handleDoubleSign(ctx, valConsAddr, 1, time.Unix(0, 0), amtInt)
 	// should be jailed
-	require.True(t, sk.Validator(ctx, addr).GetJailed())
+	require.True(t, sk.Validator(ctx, operatorAddr).GetJailed())
 	// end block
 	stake.EndBlocker(ctx, sk)
 	// update block height
-	ctx = ctx.WithBlockHeight(int64(1))
+	ctx = ctx.WithBlockHeight(int64(2))
 	// unjail to measure power
-	sk.Unjail(ctx, valConsAddr)
+	sk.Unjail(ctx, sdk.ConsAddress(valConsAddr))
 	// end block
 	stake.EndBlocker(ctx, sk)
 	// power should be reduced
 	expectedPower := sdk.NewDecFromInt(amt).Mul(sdk.NewDec(19).Quo(sdk.NewDec(20)))
-	require.Equal(t, expectedPower, sk.Validator(ctx, addr).GetPower())
+	require.Equal(t, expectedPower, sk.Validator(ctx, operatorAddr).GetPower())
 
 	// double sign again, same slashing period
-	keeper.handleDoubleSign(ctx, valConsPubKey.Address(), 0, time.Unix(0, 0), amtInt)
+	keeper.handleDoubleSign(ctx, valConsAddr, 1, time.Unix(0, 0), amtInt)
 	// should be jailed
-	require.True(t, sk.Validator(ctx, addr).GetJailed())
+	require.True(t, sk.Validator(ctx, operatorAddr).GetJailed())
 	// end block
 	stake.EndBlocker(ctx, sk)
 	// update block height
-	ctx = ctx.WithBlockHeight(int64(2))
+	ctx = ctx.WithBlockHeight(int64(3))
 	// unjail to measure power
-	sk.Unjail(ctx, valConsAddr)
+	sk.Unjail(ctx, sdk.ConsAddress(valConsAddr))
 	// end block
 	stake.EndBlocker(ctx, sk)
 	// power should be equal, no more should have been slashed
 	expectedPower = sdk.NewDecFromInt(amt).Mul(sdk.NewDec(19).Quo(sdk.NewDec(20)))
-	require.Equal(t, expectedPower, sk.Validator(ctx, addr).GetPower())
+	require.Equal(t, expectedPower, sk.Validator(ctx, operatorAddr).GetPower())
 
 	// double sign again, new slashing period
-	keeper.handleDoubleSign(ctx, valConsPubKey.Address(), 2, time.Unix(0, 0), amtInt)
+	keeper.handleDoubleSign(ctx, valConsAddr, 3, time.Unix(0, 0), amtInt)
 	// should be jailed
-	require.True(t, sk.Validator(ctx, addr).GetJailed())
+	require.True(t, sk.Validator(ctx, operatorAddr).GetJailed())
 	// unjail to measure power
-	sk.Unjail(ctx, valConsAddr)
+	sk.Unjail(ctx, sdk.ConsAddress(valConsAddr))
 	// end block
 	stake.EndBlocker(ctx, sk)
 	// power should be reduced
 	expectedPower = sdk.NewDecFromInt(amt).Mul(sdk.NewDec(18).Quo(sdk.NewDec(20)))
-	require.Equal(t, expectedPower, sk.Validator(ctx, addr).GetPower())
+	require.Equal(t, expectedPower, sk.Validator(ctx, operatorAddr).GetPower())
 }
 
 // Test a validator through uptime, downtime, revocation,
@@ -196,6 +197,32 @@ func TestHandleAbsentValidator(t *testing.T) {
 	validator, _ = sk.GetValidatorByConsAddr(ctx, sdk.GetConsAddress(val))
 	require.Equal(t, sdk.Unbonding, validator.GetStatus())
 
+	// validator should have been slashed
+	require.Equal(t, int64(99), validator.GetTokens().RoundInt64())
+
+	// 502nd block *also* missed (since the LastCommit would have still included the just-unbonded validator)
+	height++
+	ctx = ctx.WithBlockHeight(height)
+	keeper.handleValidatorSignature(ctx, val.Address(), amtInt, false)
+	info, found = keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address()))
+	require.True(t, found)
+	require.Equal(t, int64(0), info.StartHeight)
+	require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)-2, info.SignedBlocksCounter)
+
+	// end block
+	stake.EndBlocker(ctx, sk)
+
+	// validator should not have been slashed any more, since it was already jailed
+	validator, _ = sk.GetValidatorByConsAddr(ctx, sdk.GetConsAddress(val))
+	require.Equal(t, int64(99), validator.GetTokens().RoundInt64())
+
+	// 502nd block *double signed* (oh no!)
+	keeper.handleDoubleSign(ctx, val.Address(), height, ctx.BlockHeader().Time, amtInt)
+
+	// validator should have been slashed
+	validator, _ = sk.GetValidatorByConsAddr(ctx, sdk.GetConsAddress(val))
+	require.Equal(t, int64(94), validator.GetTokens().RoundInt64())
+
 	// unrevocation should fail prior to jail expiration
 	got = slh(ctx, NewMsgUnjail(addr))
 	require.False(t, got.IsOK())
@@ -215,13 +242,13 @@ func TestHandleAbsentValidator(t *testing.T) {
 	// validator should have been slashed
 	pool = sk.GetPool(ctx)
 	slashAmt := sdk.NewDec(amtInt).Mul(keeper.SlashFractionDowntime(ctx)).RoundInt64()
-	require.Equal(t, amtInt-slashAmt, pool.BondedTokens.RoundInt64())
+	require.Equal(t, amtInt-slashAmt-5, pool.BondedTokens.RoundInt64())
 
 	// validator start height should have been changed
 	info, found = keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address()))
 	require.True(t, found)
 	require.Equal(t, height, info.StartHeight)
-	require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)-1, info.SignedBlocksCounter)
+	require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)-2, info.SignedBlocksCounter)
 
 	// validator should not be immediately jailed again
 	height++