From d71cdb0575b5de227a3a1e65b188bb5bd23ac5bd Mon Sep 17 00:00:00 2001 From: insumity Date: Fri, 17 May 2024 11:22:20 +0200 Subject: [PATCH] fix!: drop chain proposals with empty validator set at spawn time (#1888) * init commit * Update x/ccv/provider/keeper/proposal.go Co-authored-by: MSalopek * added one more test case --------- Co-authored-by: MSalopek (cherry picked from commit a0645d80636cb60dabf285bc21375d927f4295e4) # Conflicts: # testutil/keeper/expectations.go --- testutil/keeper/expectations.go | 3 ++ testutil/keeper/unit_test_helpers.go | 1 + x/ccv/provider/keeper/proposal.go | 13 +++++++ x/ccv/provider/keeper/proposal_test.go | 49 ++++++++++++++++++++++++- x/ccv/provider/proposal_handler_test.go | 1 + 5 files changed, 66 insertions(+), 1 deletion(-) diff --git a/testutil/keeper/expectations.go b/testutil/keeper/expectations.go index 5a37f3a164..b3d2dc995b 100644 --- a/testutil/keeper/expectations.go +++ b/testutil/keeper/expectations.go @@ -57,8 +57,11 @@ func GetMocksForMakeConsumerGenesis(ctx sdk.Context, mocks *MockedKeepers, mocks.MockClientKeeper.EXPECT().GetSelfConsensusState(gomock.Any(), clienttypes.GetSelfHeight(ctx)).Return(&ibctmtypes.ConsensusState{}, nil).Times(1), +<<<<<<< HEAD mocks.MockStakingKeeper.EXPECT().IterateLastValidatorPowers(gomock.Any(), gomock.Any()).Times(1), +======= +>>>>>>> a0645d8 (fix!: drop chain proposals with empty validator set at spawn time (#1888)) } } diff --git a/testutil/keeper/unit_test_helpers.go b/testutil/keeper/unit_test_helpers.go index abba1a23c9..ef208cb907 100644 --- a/testutil/keeper/unit_test_helpers.go +++ b/testutil/keeper/unit_test_helpers.go @@ -215,6 +215,7 @@ func SetupForStoppingConsumerChain(t *testing.T, ctx sdk.Context, providerKeeper *providerkeeper.Keeper, mocks MockedKeepers, ) { t.Helper() + mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1) expectations := GetMocksForCreateConsumerClient(ctx, &mocks, "chainID", clienttypes.NewHeight(4, 5)) expectations = append(expectations, GetMocksForSetConsumerChain(ctx, &mocks, "chainID")...) diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index 1d2bf19a4f..9a0086e540 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -410,6 +410,19 @@ func (k Keeper) BeginBlockInit(ctx sdk.Context) { continue } + consumerGenesis, found := k.GetConsumerGenesis(cachedCtx, prop.ChainId) + if !found { + // drop the proposal + ctx.Logger().Info("consumer genesis could not be created") + continue + } + + if len(consumerGenesis.Provider.InitialValSet) == 0 { + // drop the proposal + ctx.Logger().Info("consumer genesis initial validator set is empty - no validators opted in") + continue + } + // The cached context is created with a new EventManager so we merge the event // into the original context ctx.EventManager().EmitEvents(cachedCtx.EventManager().Events()) diff --git a/x/ccv/provider/keeper/proposal_test.go b/x/ccv/provider/keeper/proposal_test.go index 9b769d2f94..5f8d6aa409 100644 --- a/x/ccv/provider/keeper/proposal_test.go +++ b/x/ccv/provider/keeper/proposal_test.go @@ -3,6 +3,7 @@ package keeper_test import ( "bytes" "encoding/json" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" "sort" "testing" "time" @@ -115,6 +116,7 @@ func TestHandleConsumerAdditionProposal(t *testing.T) { if tc.expAppendProp { // Mock calls are only asserted if we expect a client to be created. + mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1) gomock.InOrder( testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, tc.prop.ChainId, clienttypes.NewHeight(2, 3))..., ) @@ -158,6 +160,7 @@ func TestCreateConsumerClient(t *testing.T) { description: "No state mutation, new client should be created", setup: func(providerKeeper *providerkeeper.Keeper, ctx sdk.Context, mocks *testkeeper.MockedKeepers) { // Valid client creation is asserted with mock expectations here + mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1) gomock.InOrder( testkeeper.GetMocksForCreateConsumerClient(ctx, mocks, "chainID", clienttypes.NewHeight(4, 5))..., ) @@ -796,6 +799,7 @@ func TestMakeConsumerGenesis(t *testing.T) { // ctx = ctx.WithChainID("testchain1") // chainID is obtained from ctx ctx = ctx.WithBlockHeight(5) // RevisionHeight obtained from ctx + mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1) gomock.InOrder(testkeeper.GetMocksForMakeConsumerGenesis(ctx, &mocks, 1814400000000000)...) // matches params from jsonString @@ -1005,6 +1009,22 @@ func TestBeginBlockInit(t *testing.T) { nil, nil, ).(*providertypes.ConsumerAdditionProposal), + providertypes.NewConsumerAdditionProposal( + "title", "opt-in chain with no validator opted in", "chain6", clienttypes.NewHeight(3, 4), []byte{}, []byte{}, + now.Add(-time.Minute).UTC(), + "0.75", + 10, + "", + 10000, + 100000000000, + 100000000000, + 100000000000, + 0, + 0, + 0, + nil, + nil, + ).(*providertypes.ConsumerAdditionProposal), } // Expect client creation for only the first, second, and fifth proposals (spawn time already passed and valid) @@ -1012,6 +1032,10 @@ func TestBeginBlockInit(t *testing.T) { expectedCalls = append(expectedCalls, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chain2", clienttypes.NewHeight(3, 4))...) expectedCalls = append(expectedCalls, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chain5", clienttypes.NewHeight(3, 4))...) + // The sixth proposal would have spawn time passed and hence needs the mocks but the client will not be + // created because `chain6` is an Opt In chain and has no validator opted in + expectedCalls = append(expectedCalls, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chain6", clienttypes.NewHeight(3, 4))...) + gomock.InOrder(expectedCalls...) for _, prop := range pendingProps { @@ -1021,6 +1045,8 @@ func TestBeginBlockInit(t *testing.T) { // opt in a sample validator so the chain's proposal can successfully execute validator := cryptotestutil.NewCryptoIdentityFromIntSeed(0).SDKStakingValidator() consAddr, _ := validator.GetConsAddr() + mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Return([]stakingtypes.Validator{validator}).AnyTimes() + mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), validator.GetOperator()).Return(int64(1)).AnyTimes() providerKeeper.SetOptedIn(ctx, pendingProps[4].ChainId, providertypes.NewProviderConsAddress(consAddr)) providerKeeper.BeginBlockInit(ctx) @@ -1061,10 +1087,30 @@ func TestBeginBlockInit(t *testing.T) { _, found = providerKeeper.GetPendingConsumerAdditionProp( ctx, pendingProps[4].SpawnTime, pendingProps[4].ChainId) require.False(t, found) - // sixth proposal was successfully executed and hence consumer genesis was created + // fifth proposal was successfully executed and hence consumer genesis was created _, found = providerKeeper.GetConsumerGenesis(ctx, pendingProps[4].ChainId) require.True(t, found) + // sixth proposal corresponds to an Opt-In chain with no opted-in validators and hence the + // proposal is not successful + _, found = providerKeeper.GetPendingConsumerAdditionProp( + ctx, pendingProps[5].SpawnTime, pendingProps[5].ChainId) + // the proposal was dropped and deleted + require.False(t, found) + // no consumer genesis is created + _, found = providerKeeper.GetConsumerGenesis(ctx, pendingProps[5].ChainId) + require.False(t, found) + // no consumer client is associated with this chain + _, found = providerKeeper.GetConsumerClientId(ctx, pendingProps[5].ChainId) + require.False(t, found) + // no fields should be set for this (check some of them) + _, found = providerKeeper.GetTopN(ctx, pendingProps[5].ChainId) + require.False(t, found) + _, found = providerKeeper.GetValidatorsPowerCap(ctx, pendingProps[5].ChainId) + require.False(t, found) + _, found = providerKeeper.GetValidatorSetCap(ctx, pendingProps[5].ChainId) + require.False(t, found) + // test that Top N is set correctly require.True(t, providerKeeper.IsTopN(ctx, "chain1")) topN, found := providerKeeper.GetTopN(ctx, "chain1") @@ -1104,6 +1150,7 @@ func TestBeginBlockCCR(t *testing.T) { expectations := []*gomock.Call{} for _, prop := range pendingProps { // A consumer chain is setup corresponding to each prop, making these mocks necessary + mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1) expectations = append(expectations, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, prop.ChainId, clienttypes.NewHeight(2, 3))...) expectations = append(expectations, testkeeper.GetMocksForSetConsumerChain(ctx, &mocks, prop.ChainId)...) diff --git a/x/ccv/provider/proposal_handler_test.go b/x/ccv/provider/proposal_handler_test.go index 185db25b07..8d5b696b40 100644 --- a/x/ccv/provider/proposal_handler_test.go +++ b/x/ccv/provider/proposal_handler_test.go @@ -96,6 +96,7 @@ func TestProviderProposalHandler(t *testing.T) { // Mock expectations depending on expected outcome switch { case tc.expValidConsumerAddition: + mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1) gomock.InOrder(testkeeper.GetMocksForCreateConsumerClient( ctx, &mocks, "chainID", clienttypes.NewHeight(2, 3), )...)