From 4b91b3b5b8cde5fd1ed2fa95ed92fb3f6321a5fd Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 11 Jun 2024 11:14:06 +0200 Subject: [PATCH] test: Add integration test reproducing the LastValidators exceeding MaxValidators bug (#1945) * Add test reproducing the LastValidators exceeding MaxValidators * formatting * Update tests/integration/unbonding.go Co-authored-by: insumity * Update tests/integration/unbonding.go Co-authored-by: Philip Offtermatt <57488781+p-offtermatt@users.noreply.github.com> * document --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: insumity Co-authored-by: Philip Offtermatt <57488781+p-offtermatt@users.noreply.github.com> --- tests/integration/unbonding.go | 61 +++++++++++++++++++++++++++++- testutil/integration/debug_test.go | 4 ++ testutil/integration/interfaces.go | 4 ++ testutil/keeper/mocks.go | 28 ++++++++++++++ 4 files changed, 96 insertions(+), 1 deletion(-) diff --git a/tests/integration/unbonding.go b/tests/integration/unbonding.go index dedc3d6555..fddb504d14 100644 --- a/tests/integration/unbonding.go +++ b/tests/integration/unbonding.go @@ -4,7 +4,8 @@ import ( "time" "cosmossdk.io/math" - + sdk "github.com/cosmos/cosmos-sdk/types" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" providerkeeper "github.com/cosmos/interchain-security/v5/x/ccv/provider/keeper" ccv "github.com/cosmos/interchain-security/v5/x/ccv/types" ) @@ -467,3 +468,61 @@ func (s *CCVTestSuite) TestRedelegationProviderFirst() { // Check that ccv unbonding op has been deleted checkCCVUnbondingOp(s, s.providerCtx(), s.consumerChain.ChainID, valsetUpdateID, false) } + +// This test reproduces a fixed bug when an inactive validator enters back into the active set. +// It used to cause a panic in the provider module hook called by AfterUnbondingInitiated +// during the staking module EndBlock. +func (s *CCVTestSuite) TestTooManyLastValidators() { + sk := s.providerApp.GetTestStakingKeeper() + + getLastValsFn := func(ctx sdk.Context) []stakingtypes.Validator { + lastVals, err := sk.GetLastValidators(s.providerCtx()) + s.Require().NoError(err) + return lastVals + } + + // get current staking params + p, err := sk.GetParams(s.providerCtx()) + s.Require().NoError(err) + + // get validators, which are all active at the moment + vals, err := sk.GetAllValidators(s.providerCtx()) + s.Require().NoError(err) + + s.Require().Equal(len(vals), len(getLastValsFn(s.providerCtx()))) + + // jail a validator + val := vals[0] + consAddr, err := val.GetConsAddr() + s.Require().NoError(err) + sk.Jail(s.providerCtx(), consAddr) + + // save the current number of bonded vals + lastVals := getLastValsFn(s.providerCtx()) + + // pass one block to apply the validator set changes + // (calls ApplyAndReturnValidatorSetUpdates in the the staking module EndBlock) + s.providerChain.NextBlock() + + // verify that the number of bonded validators is decreased by one + s.Require().Equal(len(lastVals)-1, len(getLastValsFn(s.providerCtx()))) + + // update maximum validator to equal the number of bonded validators + p.MaxValidators = uint32(len(getLastValsFn(s.providerCtx()))) + sk.SetParams(s.providerCtx(), p) + + // pass one block to apply validator set changes + s.providerChain.NextBlock() + + // unjail validator + // Note that since validators are sorted in descending order, the unjailed validator + // enters the active set again since it's ranked first by voting power. + sk.Unjail(s.providerCtx(), consAddr) + + // pass another block to update the validator set + // which causes a panic due to a GetLastValidator call in + // ApplyAndReturnValidatorSetUpdates where the staking module has a inconsistent state + s.Require().NotPanics(s.providerChain.NextBlock) + s.Require().NotPanics(func() { sk.ApplyAndReturnValidatorSetUpdates(s.providerCtx()) }) + s.Require().NotPanics(func() { getLastValsFn(s.providerCtx()) }) +} diff --git a/testutil/integration/debug_test.go b/testutil/integration/debug_test.go index 1a847c4011..15036416e6 100644 --- a/testutil/integration/debug_test.go +++ b/testutil/integration/debug_test.go @@ -324,3 +324,7 @@ func TestAllocateTokensToValidator(t *testing.T) { func TestMultiConsumerRewardsDistribution(t *testing.T) { runCCVTestByName(t, "TestMultiConsumerRewardsDistribution") } + +func TestTooManyLastValidators(t *testing.T) { + runCCVTestByName(t, "TestTooManyLastValidators") +} diff --git a/testutil/integration/interfaces.go b/testutil/integration/interfaces.go index 4389c7698d..de9c3a6b0d 100644 --- a/testutil/integration/interfaces.go +++ b/testutil/integration/interfaces.go @@ -4,6 +4,7 @@ import ( "context" "time" + abci "github.com/cometbft/cometbft/abci/types" ibctesting "github.com/cosmos/ibc-go/v8/testing" "cosmossdk.io/core/comet" @@ -108,6 +109,9 @@ type TestStakingKeeper interface { GetUnbondingDelegation(ctx context.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) (ubd stakingtypes.UnbondingDelegation, err error) GetAllValidators(ctx context.Context) (validators []stakingtypes.Validator, err error) GetValidatorSet() stakingtypes.ValidatorSet + GetParams(ctx context.Context) (stakingtypes.Params, error) + SetParams(ctx context.Context, p stakingtypes.Params) error + ApplyAndReturnValidatorSetUpdates(ctx context.Context) (updates []abci.ValidatorUpdate, err error) } type TestBankKeeper interface { diff --git a/testutil/keeper/mocks.go b/testutil/keeper/mocks.go index 8e2c552c95..9c624a669b 100644 --- a/testutil/keeper/mocks.go +++ b/testutil/keeper/mocks.go @@ -122,6 +122,20 @@ func (mr *MockStakingKeeperMockRecorder) GetLastValidators(ctx interface{}) *gom return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLastValidators", reflect.TypeOf((*MockStakingKeeper)(nil).GetLastValidators), ctx) } +// GetParams mocks base method. +func (m *MockStakingKeeper) GetParams(ctx types0.Context) types5.Params { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetParams", ctx) + ret0, _ := ret[0].(types5.Params) + return ret0 +} + +// GetParams indicates an expected call of GetParams. +func (mr *MockStakingKeeperMockRecorder) GetParams(ctx interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetParams", reflect.TypeOf((*MockStakingKeeper)(nil).GetParams), ctx) +} + // GetRedelegationByUnbondingID mocks base method. func (m *MockStakingKeeper) GetRedelegationByUnbondingID(ctx context.Context, id uint64) (types3.Redelegation, error) { m.ctrl.T.Helper() @@ -372,6 +386,20 @@ func (mr *MockStakingKeeperMockRecorder) PutUnbondingOnHold(ctx, id interface{}) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PutUnbondingOnHold", reflect.TypeOf((*MockStakingKeeper)(nil).PutUnbondingOnHold), ctx, id) } +// SetParams mocks base method. +func (m *MockStakingKeeper) SetParams(ctx types0.Context, p types5.Params) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SetParams", ctx, p) + ret0, _ := ret[0].(error) + return ret0 +} + +// SetParams indicates an expected call of SetParams. +func (mr *MockStakingKeeperMockRecorder) SetParams(ctx, p interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetParams", reflect.TypeOf((*MockStakingKeeper)(nil).SetParams), ctx, p) +} + // Slash mocks base method. func (m *MockStakingKeeper) Slash(ctx context.Context, consAddr types1.ConsAddress, infractionHeight, power int64, slashFactor math.LegacyDec) (math.Int, error) { m.ctrl.T.Helper()