Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: backport regression mem tests for Hub halt (#1945) #1965

Merged
merged 2 commits into from
Jun 14, 2024
Merged

chore: backport regression mem tests for Hub halt (#1945) #1965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b91b3b
test: Add integration test reproducing the LastValidators exceeding M…
sainoe Jun 11, 2024
aeeedc7
update mocks
sainoe Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 60 additions & 1 deletion tests/integration/unbonding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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()) })
}
Comment on lines +475 to +528
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function TestTooManyLastValidators effectively tests the critical bug fix related to validators re-entering the active set. Consider adding more detailed comments within the function to explain the purpose of each operation for future maintainability.

+ // Initialize staking keeper
+ // Retrieve the last validators from the context
+ // Check the initial condition that the number of active validators equals the number of last validators
+ // Jail one validator to simulate the bug scenario
+ // Verify the state changes after jailing the validator
+ // Adjust the maximum validators to trigger the condition
+ // Unjail the validator and verify the final state to ensure no panic occurs
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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()) })
}
func (s *CCVTestSuite) TestTooManyLastValidators() {
// Initialize staking keeper
sk := s.providerApp.GetTestStakingKeeper()
// Retrieve the last validators from the context
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)
// Check the initial condition that the number of active validators equals the number of last validators
s.Require().Equal(len(vals), len(getLastValsFn(s.providerCtx())))
// Jail one validator to simulate the bug scenario
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 the state changes after jailing the validator
s.Require().Equal(len(lastVals)-1, len(getLastValsFn(s.providerCtx())))
// Adjust the maximum validators to trigger the condition
p.MaxValidators = uint32(len(getLastValsFn(s.providerCtx())))
sk.SetParams(s.providerCtx(), p)
// pass one block to apply validator set changes
s.providerChain.NextBlock()
// Unjail the validator and verify the final state to ensure no panic occurs
// 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()) })
}

4 changes: 4 additions & 0 deletions testutil/integration/debug_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
4 changes: 4 additions & 0 deletions testutil/integration/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down
Loading