From 86ecb7f5059f2a64bfe23bb4eef1e77c274bb932 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 16 May 2022 17:55:04 +0200 Subject: [PATCH 1/3] test: improve TestValidatorDippingInAndOut --- x/slashing/keeper/hooks.go | 3 +-- x/slashing/keeper/keeper_test.go | 24 +++++++++++++++++++----- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/x/slashing/keeper/hooks.go b/x/slashing/keeper/hooks.go index 09d1afe02fac..9dd346d232df 100644 --- a/x/slashing/keeper/hooks.go +++ b/x/slashing/keeper/hooks.go @@ -11,8 +11,7 @@ import ( func (k Keeper) AfterValidatorBonded(ctx sdk.Context, address sdk.ConsAddress, _ sdk.ValAddress) error { // Update the signing info start height or create a new signing info - _, found := k.GetValidatorSigningInfo(ctx, address) - if !found { + if _, found := k.GetValidatorSigningInfo(ctx, address); !found { signingInfo := types.NewValidatorSigningInfo( address, ctx.BlockHeight(), diff --git a/x/slashing/keeper/keeper_test.go b/x/slashing/keeper/keeper_test.go index 7385c1f4d7e2..831d657692b2 100644 --- a/x/slashing/keeper/keeper_test.go +++ b/x/slashing/keeper/keeper_test.go @@ -200,7 +200,9 @@ func TestValidatorDippingInAndOut(t *testing.T) { valAddr := sdk.ValAddress(addr) tstaking.CreateValidatorWithValPower(valAddr, val, power, true) - staking.EndBlocker(ctx, app.StakingKeeper) + validatorUpdates := staking.EndBlocker(ctx, app.StakingKeeper) + require.Equal(t, 2, len(validatorUpdates)) + tstaking.CheckValidator(valAddr, stakingtypes.Bonded, false) // 100 first blocks OK height := int64(0) @@ -210,9 +212,10 @@ func TestValidatorDippingInAndOut(t *testing.T) { } // kick first validator out of validator set - tstaking.CreateValidatorWithValPower(sdk.ValAddress(pks[1].Address()), pks[1], 101, true) - validatorUpdates := staking.EndBlocker(ctx, app.StakingKeeper) + tstaking.CreateValidatorWithValPower(sdk.ValAddress(pks[1].Address()), pks[1], power+1, true) + validatorUpdates = staking.EndBlocker(ctx, app.StakingKeeper) require.Equal(t, 2, len(validatorUpdates)) + tstaking.CheckValidator(sdk.ValAddress(pks[1].Address()), stakingtypes.Bonded, false) tstaking.CheckValidator(valAddr, stakingtypes.Unbonding, false) // 600 more blocks happened @@ -225,7 +228,7 @@ func TestValidatorDippingInAndOut(t *testing.T) { validatorUpdates = staking.EndBlocker(ctx, app.StakingKeeper) require.Equal(t, 2, len(validatorUpdates)) tstaking.CheckValidator(valAddr, stakingtypes.Bonded, false) - newPower := int64(150) + newPower := power + 50 // validator misses a block app.SlashingKeeper.HandleValidatorSignature(ctx, val.Address(), newPower, false) @@ -248,8 +251,10 @@ func TestValidatorDippingInAndOut(t *testing.T) { // check all the signing information signInfo, found := app.SlashingKeeper.GetValidatorSigningInfo(ctx, consAddr) require.True(t, found) + require.Equal(t, int64(0), signInfo.StartHeight) require.Equal(t, int64(0), signInfo.MissedBlocksCounter) require.Equal(t, int64(0), signInfo.IndexOffset) + // array should be cleared for offset := int64(0); offset < app.SlashingKeeper.SignedBlocksWindow(ctx); offset++ { missed := app.SlashingKeeper.GetValidatorMissedBlockBitArray(ctx, consAddr, offset) @@ -262,13 +267,22 @@ func TestValidatorDippingInAndOut(t *testing.T) { // validator rejoins and starts signing again app.StakingKeeper.Unjail(ctx, consAddr) + staking.EndBlocker(ctx, app.StakingKeeper) + + // start signing again app.SlashingKeeper.HandleValidatorSignature(ctx, val.Address(), newPower, true) - height++ // validator should not be kicked since we reset counter/array when it was jailed staking.EndBlocker(ctx, app.StakingKeeper) tstaking.CheckValidator(valAddr, stakingtypes.Bonded, false) + // check start height is correctly set + signInfo, found = app.SlashingKeeper.GetValidatorSigningInfo(ctx, consAddr) + require.True(t, found) + require.Equal(t, height, signInfo.StartHeight) // TODO I don't think it should fail + require.Equal(t, int64(0), signInfo.MissedBlocksCounter) + require.Equal(t, int64(0), signInfo.IndexOffset) + // validator misses 501 blocks latest = height for ; height < latest+501; height++ { From aed1ab7f5c274eb130f748600c57bb410bc39e75 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Wed, 18 May 2022 12:55:06 +0200 Subject: [PATCH 2/3] implement behavior matching comment --- x/slashing/keeper/hooks.go | 24 +++++++++++++++++----- x/slashing/keeper/keeper_test.go | 34 +++++++++++--------------------- x/slashing/spec/01_concepts.md | 6 ++---- x/slashing/spec/05_hooks.md | 5 +++++ 4 files changed, 38 insertions(+), 31 deletions(-) diff --git a/x/slashing/keeper/hooks.go b/x/slashing/keeper/hooks.go index 9dd346d232df..a306f76c210d 100644 --- a/x/slashing/keeper/hooks.go +++ b/x/slashing/keeper/hooks.go @@ -11,8 +11,11 @@ import ( func (k Keeper) AfterValidatorBonded(ctx sdk.Context, address sdk.ConsAddress, _ sdk.ValAddress) error { // Update the signing info start height or create a new signing info - if _, found := k.GetValidatorSigningInfo(ctx, address); !found { - signingInfo := types.NewValidatorSigningInfo( + signingInfo, found := k.GetValidatorSigningInfo(ctx, address) + if found { + signingInfo.StartHeight = ctx.BlockHeight() + } else { + signingInfo = types.NewValidatorSigningInfo( address, ctx.BlockHeight(), 0, @@ -20,9 +23,10 @@ func (k Keeper) AfterValidatorBonded(ctx sdk.Context, address sdk.ConsAddress, _ false, 0, ) - k.SetValidatorSigningInfo(ctx, address, signingInfo) } + k.SetValidatorSigningInfo(ctx, address, signingInfo) + return nil } @@ -73,17 +77,27 @@ func (h Hooks) AfterValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) er func (h Hooks) AfterValidatorBeginUnbonding(_ sdk.Context, _ sdk.ConsAddress, _ sdk.ValAddress) error { return nil } -func (h Hooks) BeforeValidatorModified(_ sdk.Context, _ sdk.ValAddress) error { return nil } + +func (h Hooks) BeforeValidatorModified(_ sdk.Context, _ sdk.ValAddress) error { + return nil +} + func (h Hooks) BeforeDelegationCreated(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) error { return nil } + func (h Hooks) BeforeDelegationSharesModified(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) error { return nil } + func (h Hooks) BeforeDelegationRemoved(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) error { return nil } + func (h Hooks) AfterDelegationModified(_ sdk.Context, _ sdk.AccAddress, _ sdk.ValAddress) error { return nil } -func (h Hooks) BeforeValidatorSlashed(_ sdk.Context, _ sdk.ValAddress, _ sdk.Dec) error { return nil } + +func (h Hooks) BeforeValidatorSlashed(_ sdk.Context, _ sdk.ValAddress, _ sdk.Dec) error { + return nil +} diff --git a/x/slashing/keeper/keeper_test.go b/x/slashing/keeper/keeper_test.go index 831d657692b2..799565009f22 100644 --- a/x/slashing/keeper/keeper_test.go +++ b/x/slashing/keeper/keeper_test.go @@ -219,11 +219,11 @@ func TestValidatorDippingInAndOut(t *testing.T) { tstaking.CheckValidator(valAddr, stakingtypes.Unbonding, false) // 600 more blocks happened - height = 700 + height = height + 600 ctx = ctx.WithBlockHeight(height) // validator added back in - tstaking.DelegateWithPower(sdk.AccAddress(pks[2].Address()), sdk.ValAddress(pks[0].Address()), 50) + tstaking.DelegateWithPower(sdk.AccAddress(pks[2].Address()), valAddr, 50) validatorUpdates = staking.EndBlocker(ctx, app.StakingKeeper) require.Equal(t, 2, len(validatorUpdates)) @@ -237,9 +237,9 @@ func TestValidatorDippingInAndOut(t *testing.T) { // shouldn't be jailed/kicked yet tstaking.CheckValidator(valAddr, stakingtypes.Bonded, false) - // validator misses 500 more blocks, 501 total - latest := height - for ; height < latest+500; height++ { + // validator misses the an additional 500 more blocks, after the cooling off period of SignedBlockWindow (here 1000 blocks). + latest := app.SlashingKeeper.SignedBlocksWindow(ctx) + height + for ; height < latest+app.SlashingKeeper.MinSignedPerWindow(ctx); height++ { ctx = ctx.WithBlockHeight(height) app.SlashingKeeper.HandleValidatorSignature(ctx, val.Address(), newPower, false) } @@ -251,15 +251,9 @@ func TestValidatorDippingInAndOut(t *testing.T) { // check all the signing information signInfo, found := app.SlashingKeeper.GetValidatorSigningInfo(ctx, consAddr) require.True(t, found) - require.Equal(t, int64(0), signInfo.StartHeight) - require.Equal(t, int64(0), signInfo.MissedBlocksCounter) - require.Equal(t, int64(0), signInfo.IndexOffset) - - // array should be cleared - for offset := int64(0); offset < app.SlashingKeeper.SignedBlocksWindow(ctx); offset++ { - missed := app.SlashingKeeper.GetValidatorMissedBlockBitArray(ctx, consAddr, offset) - require.False(t, missed) - } + require.Equal(t, int64(700), signInfo.StartHeight) + require.Equal(t, int64(499), signInfo.MissedBlocksCounter) + require.Equal(t, int64(499), signInfo.IndexOffset) // some blocks pass height = int64(5000) @@ -267,9 +261,7 @@ func TestValidatorDippingInAndOut(t *testing.T) { // validator rejoins and starts signing again app.StakingKeeper.Unjail(ctx, consAddr) - staking.EndBlocker(ctx, app.StakingKeeper) - // start signing again app.SlashingKeeper.HandleValidatorSignature(ctx, val.Address(), newPower, true) // validator should not be kicked since we reset counter/array when it was jailed @@ -279,13 +271,11 @@ func TestValidatorDippingInAndOut(t *testing.T) { // check start height is correctly set signInfo, found = app.SlashingKeeper.GetValidatorSigningInfo(ctx, consAddr) require.True(t, found) - require.Equal(t, height, signInfo.StartHeight) // TODO I don't think it should fail - require.Equal(t, int64(0), signInfo.MissedBlocksCounter) - require.Equal(t, int64(0), signInfo.IndexOffset) + require.Equal(t, height, signInfo.StartHeight) - // validator misses 501 blocks - latest = height - for ; height < latest+501; height++ { + // validator misses 501 blocks after SignedBlockWindow period (1000 blocks) + latest = app.SlashingKeeper.SignedBlocksWindow(ctx) + height + for ; height < latest+app.SlashingKeeper.MinSignedPerWindow(ctx); height++ { ctx = ctx.WithBlockHeight(height) app.SlashingKeeper.HandleValidatorSignature(ctx, val.Address(), newPower, false) } diff --git a/x/slashing/spec/01_concepts.md b/x/slashing/spec/01_concepts.md index 9505706f90d3..ea7c6b319bae 100644 --- a/x/slashing/spec/01_concepts.md +++ b/x/slashing/spec/01_concepts.md @@ -43,16 +43,14 @@ _Vu_ : validator unbonded ### Single Double Sign Infraction -<-----------------> -[----------C1----D1,Vu-----] +\[----------C1----D1,Vu-----\] A single infraction is committed then later discovered, at which point the validator is unbonded and slashed at the full amount for the infraction. ### Multiple Double Sign Infractions -<---------------------------> -[----------C1--C2---C3---D1,D2,D3Vu-----] +\[----------C1--C2---C3---D1,D2,D3Vu-----\] Multiple infractions are committed and then later discovered, at which point the validator is jailed and slashed for only one infraction. Because the validator diff --git a/x/slashing/spec/05_hooks.md b/x/slashing/spec/05_hooks.md index d1234e58ee05..a839689429a8 100644 --- a/x/slashing/spec/05_hooks.md +++ b/x/slashing/spec/05_hooks.md @@ -21,6 +21,8 @@ The following hooks impact the slashing state: Upon successful first-time bonding of a new validator, we create a new `ValidatorSigningInfo` structure for the now-bonded validator, which `StartHeight` of the current block. +If the validator was out of the validator set and gets bonded again, its new bonded height is set. + ```go onValidatorBonded(address sdk.ValAddress) @@ -32,7 +34,10 @@ onValidatorBonded(address sdk.ValAddress) JailedUntil : time.Unix(0, 0), Tombstone : false, MissedBloskCounter : 0 + } else { + signingInfo.StartHeight = CurrentHeight } + setValidatorSigningInfo(signingInfo) } From cb9ba34d392c3bf737bf2b48453963119d3df942 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Wed, 18 May 2022 15:40:10 +0200 Subject: [PATCH 3/3] Update x/slashing/keeper/keeper_test.go --- x/slashing/keeper/keeper_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/slashing/keeper/keeper_test.go b/x/slashing/keeper/keeper_test.go index 799565009f22..08502fa89c94 100644 --- a/x/slashing/keeper/keeper_test.go +++ b/x/slashing/keeper/keeper_test.go @@ -237,7 +237,7 @@ func TestValidatorDippingInAndOut(t *testing.T) { // shouldn't be jailed/kicked yet tstaking.CheckValidator(valAddr, stakingtypes.Bonded, false) - // validator misses the an additional 500 more blocks, after the cooling off period of SignedBlockWindow (here 1000 blocks). + // validator misses an additional 500 more blocks, after the cooling off period of SignedBlockWindow (here 1000 blocks). latest := app.SlashingKeeper.SignedBlocksWindow(ctx) + height for ; height < latest+app.SlashingKeeper.MinSignedPerWindow(ctx); height++ { ctx = ctx.WithBlockHeight(height)