diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index 13698cde32..9291e57a76 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -74,33 +74,33 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { return } - // Iterate over all registered consumer chains - for _, consumerChainID := range k.GetAllRegisteredConsumerIds(ctx) { + // Iterate over all launched consumer chains + for _, consumerId := range k.GetAllRegisteredConsumerIds(ctx) { // note that it's possible that no rewards are collected even though the // reward pool isn't empty. This can happen if the reward pool holds some tokens // of non-whitelisted denominations. - alloc := k.GetConsumerRewardsAllocation(ctx, consumerChainID) + alloc := k.GetConsumerRewardsAllocation(ctx, consumerId) if alloc.Rewards.IsZero() { continue } // temporary workaround to keep CanWithdrawInvariant happy // general discussions here: https://github.com/cosmos/cosmos-sdk/issues/2906#issuecomment-441867634 - if k.ComputeConsumerTotalVotingPower(ctx, consumerChainID) == 0 { + if k.ComputeConsumerTotalVotingPower(ctx, consumerId) == 0 { rewardsToSend, rewardsChange := alloc.Rewards.TruncateDecimal() err := k.distributionKeeper.FundCommunityPool(context.Context(ctx), rewardsToSend, k.accountKeeper.GetModuleAccount(ctx, types.ConsumerRewardsPool).GetAddress()) if err != nil { k.Logger(ctx).Error( "fail to allocate rewards from consumer chain %s to community pool: %s", - consumerChainID, + consumerId, err, ) } // set the consumer allocation to the remaining reward decimals alloc.Rewards = rewardsChange - k.SetConsumerRewardsAllocation(ctx, consumerChainID, alloc) + k.SetConsumerRewardsAllocation(ctx, consumerId, alloc) return } @@ -112,7 +112,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { if err != nil { k.Logger(ctx).Error( "cannot get community tax while allocating rewards from consumer chain %s: %s", - consumerChainID, + consumerId, err, ) continue @@ -132,7 +132,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { if err != nil { k.Logger(ctx).Error( "cannot send rewards to distribution module account %s: %s", - consumerChainID, + consumerId, err, ) continue @@ -141,7 +141,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { // allocate tokens to consumer validators k.AllocateTokensToConsumerValidators( ctx, - consumerChainID, + consumerId, sdk.NewDecCoinsFromCoins(validatorsRewardsTrunc...), ) @@ -151,7 +151,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { if err != nil { k.Logger(ctx).Error( "fail to allocate rewards from consumer chain %s to community pool: %s", - consumerChainID, + consumerId, err, ) continue @@ -159,7 +159,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { // set consumer allocations to the remaining rewards decimals alloc.Rewards = validatorsRewardsChange.Add(remainingChanges...) - k.SetConsumerRewardsAllocation(ctx, consumerChainID, alloc) + k.SetConsumerRewardsAllocation(ctx, consumerId, alloc) } } diff --git a/x/ccv/provider/keeper/hooks.go b/x/ccv/provider/keeper/hooks.go index 841db8330e..a1576ff33d 100644 --- a/x/ccv/provider/keeper/hooks.go +++ b/x/ccv/provider/keeper/hooks.go @@ -115,44 +115,19 @@ func (h Hooks) AfterProposalSubmission(goCtx context.Context, proposalId uint64) return fmt.Errorf("cannot retrieve proposal with id: %d", proposalId) } - hasUpdateConsumerMsg := false - for _, msg := range p.GetMessages() { - sdkMsg, isMsgUpdateConsumer := msg.GetCachedValue().(*providertypes.MsgUpdateConsumer) - if isMsgUpdateConsumer { - // A `MsgUpdateConsumer` can only succeed if the owner of the consumer chain is the gov module. - // If that's not the case, we immediately fail the proposal. - // Note that someone could potentially change the owner of a chain to be that of the gov module - // while a proposal is active and before the proposal is executed. Even then, we still do not allow - // `MsgUpdateConsumer` proposals if the owner of the chain is not the gov module to avoid someone forgetting - // to change the owner address while the proposal is active. - ownerAddress, err := h.k.GetConsumerOwnerAddress(ctx, sdkMsg.ConsumerId) - if err != nil { - return fmt.Errorf("cannot find owner address for consumer with consumer id (%s): %s", sdkMsg.ConsumerId, err.Error()) - } else if ownerAddress != h.k.GetAuthority() { - return fmt.Errorf("owner address (%s) is not the gov module (%s)", ownerAddress, h.k.GetAuthority()) - } - - if hasUpdateConsumerMsg { - return fmt.Errorf("proposal can contain at most one `MsgUpdateConsumer` message") - } - hasUpdateConsumerMsg = true - h.k.SetProposalIdToConsumerId(ctx, proposalId, sdkMsg.ConsumerId) - } + err = DoesNotHaveDeprecatedMessage(&p) + if err != nil { + return err + } - // if the proposal contains a deprecated message, cancel the proposal - _, isMsgConsumerAddition := msg.GetCachedValue().(*providertypes.MsgConsumerAddition) - if isMsgConsumerAddition { - return fmt.Errorf("proposal cannot contain deprecated `MsgConsumerAddition`; use `MsgCreateConsumer` instead") - } + msgUpdateConsumer, err := h.k.HasAtMostOnceCorrectMsgUpdateConsumer(ctx, &p) + if err != nil { + return err + } - _, isMsgConsumerModification := msg.GetCachedValue().(*providertypes.MsgConsumerModification) - if isMsgConsumerModification { - return fmt.Errorf("proposal cannot contain deprecated `MsgConsumerModification`; use `MsgUpdateConsumer` instead") - } - _, isMsgConsumerRemoval := msg.GetCachedValue().(*providertypes.MsgConsumerRemoval) - if isMsgConsumerRemoval { - return fmt.Errorf("proposal cannot contain deprecated `MsgConsumerRemoval`; use `MsgRemoveConsumer` instead") - } + if msgUpdateConsumer != nil { + // a correctly set `MsgUpdateConsumer` was found + h.k.SetProposalIdToConsumerId(ctx, proposalId, msgUpdateConsumer.ConsumerId) } return nil diff --git a/x/ccv/provider/keeper/permissionless.go b/x/ccv/provider/keeper/permissionless.go index 1c4a6a5597..af979ff07e 100644 --- a/x/ccv/provider/keeper/permissionless.go +++ b/x/ccv/provider/keeper/permissionless.go @@ -6,6 +6,7 @@ import ( "encoding/binary" "fmt" sdk "github.com/cosmos/cosmos-sdk/types" + govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" "github.com/cosmos/interchain-security/v5/x/ccv/provider/types" "strconv" "time" @@ -692,3 +693,54 @@ func (k Keeper) CanLaunch(ctx sdk.Context, consumerId string) (time.Time, bool) return initializationParameters.SpawnTime, spawnTimeInTheFuture && genesisHashSet && binaryHashSet && consumerRedistributionFractionSet && blocksPerDistributionTransmissionSet && historicalEntriesSet } + +// HasAtMostOnceCorrectMsgUpdateConsumer checks that the proposal has at most one `MsgUpdateConsumer` that is +// correctly set (i.e., the owner address of the to-be-updated consumer corresponds to the gov module). Returns +// the single `MsgUpdateConsumer` message if only one correctly set exists. +func (k Keeper) HasAtMostOnceCorrectMsgUpdateConsumer(ctx sdk.Context, proposal *govv1.Proposal) (*types.MsgUpdateConsumer, error) { + var msgUpdateConsumer *types.MsgUpdateConsumer + for _, msg := range proposal.GetMessages() { + sdkMsg, isMsgUpdateConsumer := msg.GetCachedValue().(*types.MsgUpdateConsumer) + if isMsgUpdateConsumer { + // A `MsgUpdateConsumer` can only succeed if the owner of the consumer chain is the gov module. + // If that's not the case, we immediately fail the proposal. + // Note that someone could potentially change the owner of a chain to be that of the gov module + // while a proposal is active and before the proposal is executed. Even then, we still do not allow + // `MsgUpdateConsumer` proposals if the owner of the chain is not the gov module to avoid someone forgetting + // to change the owner address while the proposal is active. + ownerAddress, err := k.GetConsumerOwnerAddress(ctx, sdkMsg.ConsumerId) + if err != nil { + return nil, fmt.Errorf("cannot find owner address for consumer with consumer id (%s): %s", sdkMsg.ConsumerId, err.Error()) + } else if ownerAddress != k.GetAuthority() { + return nil, fmt.Errorf("owner address (%s) is not the gov module (%s)", ownerAddress, k.GetAuthority()) + } + + if msgUpdateConsumer != nil { + return nil, fmt.Errorf("proposal can contain at most one `MsgUpdateConsumer` message") + } + msgUpdateConsumer = sdkMsg + } + } + return msgUpdateConsumer, nil +} + +// DoesNotHaveDeprecatedMessage checks that the provided proposal does not contain any deprecated messages and returns +// an error if this is the case +func DoesNotHaveDeprecatedMessage(proposal *govv1.Proposal) error { + for _, msg := range proposal.GetMessages() { + // if the proposal contains a deprecated message, cancel the proposal + _, isMsgConsumerAddition := msg.GetCachedValue().(*types.MsgConsumerAddition) + if isMsgConsumerAddition { + return fmt.Errorf("proposal cannot contain deprecated `MsgConsumerAddition`; use `MsgCreateConsumer` instead") + } + _, isMsgConsumerModification := msg.GetCachedValue().(*types.MsgConsumerModification) + if isMsgConsumerModification { + return fmt.Errorf("proposal cannot contain deprecated `MsgConsumerModification`; use `MsgUpdateConsumer` instead") + } + _, isMsgConsumerRemoval := msg.GetCachedValue().(*types.MsgConsumerRemoval) + if isMsgConsumerRemoval { + return fmt.Errorf("proposal cannot contain deprecated `MsgConsumerRemoval`; use `MsgRemoveConsumer` instead") + } + } + return nil +} diff --git a/x/ccv/provider/keeper/permissionless_test.go b/x/ccv/provider/keeper/permissionless_test.go index 7e290dea5b..ebd02e61d4 100644 --- a/x/ccv/provider/keeper/permissionless_test.go +++ b/x/ccv/provider/keeper/permissionless_test.go @@ -2,6 +2,7 @@ package keeper_test import ( sdk "github.com/cosmos/cosmos-sdk/types" + govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" testkeeper "github.com/cosmos/interchain-security/v5/testutil/keeper" @@ -720,3 +721,60 @@ func TestCanLaunch(t *testing.T) { require.NoError(t, err) require.False(t, canLaunch) } + +func TestHasAtMostOnceCorrectMsgUpdateConsumer(t *testing.T) { + providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) + defer ctrl.Finish() + + expectedMsgUpdateConsumer := providertypes.MsgUpdateConsumer{Signer: "signer", ConsumerId: "consumerId", NewOwnerAddress: "new owner address"} + + proposal, err := govv1.NewProposal([]sdk.Msg{&expectedMsgUpdateConsumer}, 1, time.Now(), time.Now().Add(1*time.Hour), "metadata", "title", "summary", sdk.AccAddress{}, false) + require.NoError(t, err) + + _, err = providerKeeper.HasAtMostOnceCorrectMsgUpdateConsumer(ctx, &proposal) + require.ErrorContains(t, err, "cannot find owner address") + + // set owner address that is not the gov module + providerKeeper.SetConsumerOwnerAddress(ctx, "consumerId", "owner address") + _, err = providerKeeper.HasAtMostOnceCorrectMsgUpdateConsumer(ctx, &proposal) + require.ErrorContains(t, err, "is not the gov module") + + // set owner address that is the gov module + providerKeeper.SetConsumerOwnerAddress(ctx, "consumerId", "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn") + actualMsgUpdateConsumer, err := providerKeeper.HasAtMostOnceCorrectMsgUpdateConsumer(ctx, &proposal) + require.NoError(t, err) + require.Equal(t, expectedMsgUpdateConsumer, *actualMsgUpdateConsumer) + + // a proposal with 2 `MsgUpdateConsumer` messages + invalidProposal, err := govv1.NewProposal([]sdk.Msg{&expectedMsgUpdateConsumer, &expectedMsgUpdateConsumer}, 1, time.Now(), time.Now().Add(1*time.Hour), "metadata", "title", "summary", sdk.AccAddress{}, false) + actualMsgUpdateConsumer, err = providerKeeper.HasAtMostOnceCorrectMsgUpdateConsumer(ctx, &invalidProposal) + require.ErrorContains(t, err, "proposal can contain at most one") + require.Nil(t, actualMsgUpdateConsumer) +} + +func TestDoesNotHaveDeprecatedMessage(t *testing.T) { + msgConsumerAddition := providertypes.MsgConsumerAddition{} + proposal, err := govv1.NewProposal([]sdk.Msg{&msgConsumerAddition}, 1, time.Now(), time.Now().Add(1*time.Hour), "metadata", "title", "summary", sdk.AccAddress{}, false) + require.NoError(t, err) + err = keeper.DoesNotHaveDeprecatedMessage(&proposal) + require.ErrorContains(t, err, "cannot contain deprecated `MsgConsumerAddition`") + + msgConsumerModification := providertypes.MsgConsumerModification{} + proposal, err = govv1.NewProposal([]sdk.Msg{&msgConsumerModification}, 1, time.Now(), time.Now().Add(1*time.Hour), "metadata", "title", "summary", sdk.AccAddress{}, false) + require.NoError(t, err) + err = keeper.DoesNotHaveDeprecatedMessage(&proposal) + require.ErrorContains(t, err, "cannot contain deprecated `MsgConsumerModification`") + + msgConsumerRemoval := providertypes.MsgConsumerRemoval{} + proposal, err = govv1.NewProposal([]sdk.Msg{&msgConsumerRemoval}, 1, time.Now(), time.Now().Add(1*time.Hour), "metadata", "title", "summary", sdk.AccAddress{}, false) + require.NoError(t, err) + err = keeper.DoesNotHaveDeprecatedMessage(&proposal) + require.ErrorContains(t, err, "cannot contain deprecated `MsgConsumerRemoval`") + + // a proposal with no deprecated messages + msgUpdateConsumer := providertypes.MsgUpdateConsumer{Signer: "signer", ConsumerId: "consumerId", NewOwnerAddress: "new owner address"} + proposal, err = govv1.NewProposal([]sdk.Msg{&msgUpdateConsumer}, 1, time.Now(), time.Now().Add(1*time.Hour), "metadata", "title", "summary", sdk.AccAddress{}, false) + require.NoError(t, err) + err = keeper.DoesNotHaveDeprecatedMessage(&proposal) + require.Nil(t, err) +} diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index 407d0e2bb3..e16ec3ebff 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -488,13 +488,13 @@ func (k Keeper) BeginBlockCCR(ctx sdk.Context) { "err", err.Error()) continue } - // The cached context is created with a new EventManager so we merge the event - // into the original context - // TODO (PERMISSIONLESS): verify this here and in the initialized chains to launch - ctx.EventManager().EmitEvents(cachedCtx.EventManager().Events()) k.SetConsumerPhase(cachedCtx, consumerId, Stopped) k.RemoveConsumerToBeStoppedFromStopTime(ctx, consumerId, stopTime) + + // The cached context is created with a new EventManager so we merge the event into the original context + ctx.EventManager().EmitEvents(cachedCtx.EventManager().Events()) + writeFn() k.Logger(ctx).Info("executed consumer removal",