chore!: only distribute rewards to validators that have been validati…

…ng a consumer chain for some backport #1929 (#1983)

* fix commented distribution test

* feat!: only distribute rewards to validators that have been validating a consumer chain for some time (#1929)

* init commit

* added a warning

* took into account comments

* init commit

* added a warning

* took into account comments

* added a comment

* Update .changelog/unreleased/improvements/provider/1929-distribute-rewards-to-long-term-validating-validators.md

Co-authored-by: Marius Poke <[email protected]>

* Update .changelog/unreleased/state-breaking/provider/1929-distribute-rewards-to-long-term-validating-validators.md

Co-authored-by: Marius Poke <[email protected]>

* took into account comments

* doc

* update consensus version to 7

* Update x/ccv/provider/migrations/v6/migration_test.go

Co-authored-by: insumity <[email protected]>

* nit

---------

Co-authored-by: insumity <[email protected]>

.changelog/unreleased/improvements/provider/1929-distribute-rewards-to-long-term-validating-validators.md

@@ -0,0 +1,3 @@
+- Only start distributing rewards to validators after they have been validating
+  for a fixed number of blocks. Introduces the `NumberOfEpochsToStartReceivingRewards` param. 
+  ([\#1929](https://github.com/cosmos/interchain-security/pull/1929))

.changelog/unreleased/state-breaking/provider/1929-distribute-rewards-to-long-term-validating-validators.md

@@ -0,0 +1,3 @@
+- Only start distributing rewards to validators after they have been validating
+  for a fixed number of blocks. Introduces the `NumberOfEpochsToStartReceivingRewards` param. 
+  ([\#1929](https://github.com/cosmos/interchain-security/pull/1929))

docs/docs/validators/withdraw_rewards.md

@@ -2,8 +2,17 @@
 sidebar_position: 3
 ---
 
-# Withdrawing consumer chain validator rewards
+# Consumer chain validator rewards
 
+:::warning
+A validator can only receive rewards from a consumer chain if the validator has been validating the consumer chain
+for some time. Specifically, the validator has to be a consumer validator of the consumer chain for at least
+`NumberOfEpochsToStartReceivingRewards * BlocksPerEpoch` blocks (run `interchain-security-pd query provider params` for
+the actual values of the `NumberOfEpochsToStartReceivingRewards` and `BlocksPerEpoch` params).
+:::
+
+
+## Withdrawing rewards
 Here are example steps for withdrawing rewards from consumer chains in the provider chain
 
 :::info

proto/interchain_security/ccv/provider/v1/provider.proto

@@ -251,6 +251,9 @@ message Params {
 
   // The number of blocks that comprise an epoch.
   int64 blocks_per_epoch = 10;
+
+  // The number of epochs a validator has to validate a consumer chain in order to start receiving rewards from that chain.
+  int64 number_of_epochs_to_start_receiving_rewards = 11;
 }
 
 // SlashAcks contains cons addresses of consumer chain validators
@@ -368,6 +371,11 @@ message ConsumerValidator {
   int64 power = 2;
   // public key the validator uses on the consumer chain during this epoch
   tendermint.crypto.PublicKey consumer_public_key = 3;
+  // height the validator had when it FIRST became a consumer validator
+  // If a validator becomes a consumer validator at height `H` and is continuously a consumer validator for all the upcoming
+  // epochs, then the height of the validator SHOULD remain `H`. This height only resets to a different height if a validator
+  // stops being a consumer validator during an epoch and later becomes again a consumer validator.
+  int64 join_height = 4;
 }
 // ConsumerRewardsAllocation stores the rewards allocated by a consumer chain
 // to the consumer rewards pool. It is used to allocate the tokens to the consumer 

tests/integration/distribution.go

@@ -137,6 +137,14 @@ func (s *CCVTestSuite) TestRewardsDistribution() {
 	}
 	consuValsRewards := consumerValsOutstandingRewardsFunc(s.providerCtx())
 
+	// increase the block height so validators are eligible for consumer rewards (see `IsEligibleForConsumerRewards`)
+	numberOfBlocksToStartReceivingRewards :=
+		providerKeeper.GetNumberOfEpochsToStartReceivingRewards(s.providerCtx()) * providerKeeper.GetBlocksPerEpoch(s.providerCtx())
+
+	for s.providerCtx().BlockHeight() <= numberOfBlocksToStartReceivingRewards {
+		s.providerChain.NextBlock()
+	}
+
 	// Transfer rewards from consumer to provider and distribute rewards to
 	// validators and community pool by calling BeginBlockRD
 	relayAllCommittedPackets(
@@ -711,9 +719,14 @@ func (s *CCVTestSuite) TestAllocateTokens() {
 
 	totalRewards := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(100))}
 
+	// increase the block height so validators are eligible for consumer rewards (see `IsEligibleForConsumerRewards`)
+	numberOfBlocksToStartReceivingRewards := providerKeeper.GetNumberOfEpochsToStartReceivingRewards(
+		s.providerCtx()) * providerKeeper.GetBlocksPerEpoch(s.providerCtx())
+	providerCtx := s.providerCtx().WithBlockHeight(numberOfBlocksToStartReceivingRewards + s.providerCtx().BlockHeight())
+
 	// fund consumer rewards pool
 	bankKeeper.SendCoinsFromAccountToModule(
-		s.providerCtx(),
+		providerCtx,
 		s.providerChain.SenderAccount.GetAddress(),
 		providertypes.ConsumerRewardsPool,
 		totalRewards,
@@ -724,7 +737,7 @@ func (s *CCVTestSuite) TestAllocateTokens() {
 	for chainID := range s.consumerBundles {
 		// update consumer allocation
 		providerKeeper.SetConsumerRewardsAllocation(
-			s.providerCtx(),
+			providerCtx,
 			chainID,
 			providertypes.ConsumerRewardsAllocation{
 				Rewards: sdk.NewDecCoinsFromCoins(rewardsPerChain...),
@@ -735,7 +748,7 @@ func (s *CCVTestSuite) TestAllocateTokens() {
 	// iterate over the validators and verify that no validator has outstanding rewards
 	totalValsRewards := sdk.DecCoins{}
 	for _, val := range s.providerChain.Vals.Validators {
-		valRewards, err := distributionKeeper.GetValidatorOutstandingRewards(s.providerCtx(), sdk.ValAddress(val.Address))
+		valRewards, err := distributionKeeper.GetValidatorOutstandingRewards(providerCtx, sdk.ValAddress(val.Address))
 		s.Require().NoError(err)
 		totalValsRewards = totalValsRewards.Add(valRewards.Rewards...)
 	}
@@ -745,17 +758,17 @@ func (s *CCVTestSuite) TestAllocateTokens() {
 	// At this point the distribution module account
 	// only holds the community pool's tokens
 	// since there are no validators with outstanding rewards
-	lastCommPool := getDistrAcctBalFn(s.providerCtx())
+	lastCommPool := getDistrAcctBalFn(providerCtx)
 
 	// execute BeginBlock to trigger the token allocation
-	providerKeeper.BeginBlockRD(s.providerCtx())
+	providerKeeper.BeginBlockRD(providerCtx)
 
 	valNum := len(s.providerChain.Vals.Validators)
 	consNum := len(s.consumerBundles)
 
 	// compute the expected validators token allocation by subtracting the community tax
 	rewardsPerChainDec := sdk.NewDecCoinsFromCoins(rewardsPerChain...)
-	communityTax, err := distributionKeeper.GetCommunityTax(s.providerCtx())
+	communityTax, err := distributionKeeper.GetCommunityTax(providerCtx)
 	s.Require().NoError(err)
 
 	rewardsPerChainTrunc, _ := rewardsPerChainDec.
@@ -767,7 +780,7 @@ func (s *CCVTestSuite) TestAllocateTokens() {
 	// verify the validator tokens allocation
 	// note that the validators have the same voting power to keep things simple
 	for _, val := range s.providerChain.Vals.Validators {
-		valRewards, err := distributionKeeper.GetValidatorOutstandingRewards(s.providerCtx(), sdk.ValAddress(val.Address))
+		valRewards, err := distributionKeeper.GetValidatorOutstandingRewards(providerCtx, sdk.ValAddress(val.Address))
 		s.Require().NoError(err)
 
 		s.Require().Equal(
@@ -779,13 +792,13 @@ func (s *CCVTestSuite) TestAllocateTokens() {
 	// check that the total expected rewards are transferred to the distribution module account
 
 	// store the decimal remainders in the consumer reward allocations
-	allocRemainderPerChain := providerKeeper.GetConsumerRewardsAllocation(s.providerCtx(), s.consumerChain.ChainID).Rewards
+	allocRemainderPerChain := providerKeeper.GetConsumerRewardsAllocation(providerCtx, s.consumerChain.ChainID).Rewards
 
 	// compute the total rewards distributed to the distribution module balance (validator outstanding rewards + community pool tax),
 	totalRewardsDistributed := sdk.NewDecCoinsFromCoins(totalRewards...).Sub(allocRemainderPerChain.MulDec(math.LegacyNewDec(int64(consNum))))
 
 	// compare the expected total rewards against the distribution module balance
-	s.Require().Equal(lastCommPool.Add(totalRewardsDistributed...), getDistrAcctBalFn(s.providerCtx()))
+	s.Require().Equal(lastCommPool.Add(totalRewardsDistributed...), getDistrAcctBalFn(providerCtx))
 }
 
 // getEscrowBalance gets the current balances in the escrow account holding the transferred tokens to the provider
@@ -820,7 +833,7 @@ func (s *CCVTestSuite) prepareRewardDist() {
 	s.coordinator.CommitNBlocks(s.consumerChain, uint64(blocksToGo))
 }
 
-func (s *CCVTestSuite) TestAllocateTokensToValidator() {
+func (s *CCVTestSuite) TestAllocateTokensToConsumerValidators() {
 	providerKeeper := s.providerApp.GetProviderKeeper()
 	distributionKeeper := s.providerApp.GetTestDistributionKeeper()
 	bankKeeper := s.providerApp.GetTestBankKeeper()
@@ -866,6 +879,10 @@ func (s *CCVTestSuite) TestAllocateTokensToValidator() {
 		s.Run(tc.name, func() {
 			ctx, _ := s.providerCtx().CacheContext()
 
+			// increase the block height so validators are eligible for consumer rewards (see `IsEligibleForConsumerRewards`)
+			ctx = ctx.WithBlockHeight(providerKeeper.GetNumberOfEpochsToStartReceivingRewards(ctx)*providerKeeper.GetBlocksPerEpoch(ctx) +
+				ctx.BlockHeight())
+
 			// change the consumer valset
 			consuVals := providerKeeper.GetConsumerValSet(ctx, chainID)
 			providerKeeper.DeleteConsumerValSet(ctx, chainID)
@@ -948,6 +965,116 @@ func (s *CCVTestSuite) TestAllocateTokensToValidator() {
 	}
 }
 
+// TestAllocateTokensToConsumerValidatorsWithDifferentValidatorHeights tests `AllocateTokensToConsumerValidators` with
+// consumer validators that have different heights. Specifically, test that validators that have been consumer validators
+// for some time receive rewards, while validators that recently became consumer validators do not receive rewards.
+func (s *CCVTestSuite) TestAllocateTokensToConsumerValidatorsWithDifferentValidatorHeights() {
+	// Note this test is an adaptation of a `TestAllocateTokensToConsumerValidators` testcase.
+	providerKeeper := s.providerApp.GetProviderKeeper()
+	distributionKeeper := s.providerApp.GetTestDistributionKeeper()
+	bankKeeper := s.providerApp.GetTestBankKeeper()
+
+	chainID := s.consumerChain.ChainID
+
+	tokens := sdk.DecCoins{sdk.NewDecCoinFromDec(sdk.DefaultBondDenom, math.LegacyNewDecFromIntWithPrec(math.NewInt(999), 2))}
+	rate := math.LegacyOneDec()
+	expAllocated := sdk.DecCoins{sdk.NewDecCoinFromDec(sdk.DefaultBondDenom, math.LegacyNewDecFromIntWithPrec(math.NewInt(999), 2))}
+
+	ctx, _ := s.providerCtx().CacheContext()
+	// If the provider chain has not yet reached `GetNumberOfEpochsToStartReceivingRewards * GetBlocksPerEpoch` block height,
+	// then all validators receive rewards (see `IsEligibleForConsumerRewards`). In this test, we want to check whether
+	// validators receive rewards or not based on how long they have been consumer validators. Because of this, we increase the block height.
+	ctx = ctx.WithBlockHeight(providerKeeper.GetNumberOfEpochsToStartReceivingRewards(ctx)*providerKeeper.GetBlocksPerEpoch(ctx) + 1)
+
+	// update the consumer validators
+	consuVals := providerKeeper.GetConsumerValSet(ctx, chainID)
+	// first 2 validators were consumer validators since block height 1 and hence get rewards
+	consuVals[0].JoinHeight = 1
+	consuVals[1].JoinHeight = 1
+	// last 2 validators were consumer validators since block height 2 and hence do not get rewards because they
+	// have not been consumer validators for `GetNumberOfEpochsToStartReceivingRewards * GetBlocksPerEpoch` blocks
+	consuVals[2].JoinHeight = 2
+	consuVals[3].JoinHeight = 2
+	providerKeeper.SetConsumerValSet(ctx, chainID, consuVals)
+
+	providerKeeper.DeleteConsumerValSet(ctx, chainID)
+	providerKeeper.SetConsumerValSet(ctx, chainID, consuVals)
+	consuVals = providerKeeper.GetConsumerValSet(ctx, chainID)
+
+	// set the same consumer commission rate for all consumer validators
+	for _, v := range consuVals {
+		provAddr := providertypes.NewProviderConsAddress(sdk.ConsAddress(v.ProviderConsAddr))
+		err := providerKeeper.SetConsumerCommissionRate(
+			ctx,
+			chainID,
+			provAddr,
+			rate,
+		)
+		s.Require().NoError(err)
+	}
+
+	// allocate tokens
+	res := providerKeeper.AllocateTokensToConsumerValidators(
+		ctx,
+		chainID,
+		tokens,
+	)
+
+	// check that the expected result is returned
+	s.Require().Equal(expAllocated, res)
+
+	// rewards are expected to be allocated evenly between validators 3 and 4
+	rewardsPerVal := expAllocated.QuoDec(math.LegacyNewDec(int64(2)))
+
+	// assert that the rewards are allocated to the first 2 validators
+	for _, v := range consuVals[0:2] {
+		valAddr := sdk.ValAddress(v.ProviderConsAddr)
+		rewards, err := s.providerApp.GetTestDistributionKeeper().GetValidatorOutstandingRewards(
+			ctx,
+			valAddr,
+		)
+		s.Require().NoError(err)
+		s.Require().Equal(rewardsPerVal, rewards.Rewards)
+
+		// send rewards to the distribution module
+		valRewardsTrunc, _ := rewards.Rewards.TruncateDecimal()
+		err = bankKeeper.SendCoinsFromAccountToModule(
+			ctx,
+			s.providerChain.SenderAccount.GetAddress(),
+			distrtypes.ModuleName,
+			valRewardsTrunc)
+		s.Require().NoError(err)
+
+		// check that validators can withdraw their rewards
+		withdrawnCoins, err := distributionKeeper.WithdrawValidatorCommission(
+			ctx,
+			valAddr,
+		)
+		s.Require().NoError(err)
+
+		// check that the withdrawn coins is equal to the entire reward amount
+		// times the set consumer commission rate
+		commission := rewards.Rewards.MulDec(rate)
+		c, _ := commission.TruncateDecimal()
+		s.Require().Equal(withdrawnCoins, c)
+
+		// check that validators get rewards in their balance
+		s.Require().Equal(withdrawnCoins, bankKeeper.GetAllBalances(ctx, sdk.AccAddress(valAddr)))
+	}
+
+	// assert that no rewards are allocated to the last 2 validators because they have not been consumer validators
+	// for at least `GetNumberOfEpochsToStartReceivingRewards * GetBlocksPerEpoch` blocks
+	for _, v := range consuVals[2:4] {
+		valAddr := sdk.ValAddress(v.ProviderConsAddr)
+		rewards, err := s.providerApp.GetTestDistributionKeeper().GetValidatorOutstandingRewards(
+			ctx,
+			valAddr,
+		)
+		s.Require().NoError(err)
+		s.Require().Zero(rewards.Rewards)
+	}
+}
+
 // TestMultiConsumerRewardsDistribution tests the rewards distribution of multiple consumers chains
 func (s *CCVTestSuite) TestMultiConsumerRewardsDistribution() {
 	s.SetupAllCCVChannels()

x/ccv/provider/keeper/distribution.go

@@ -85,6 +85,10 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
 			continue
 		}
 
+		// if rewardsCollected.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 {
@@ -163,6 +167,15 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
 	}
 }
 
+// IsEligibleForConsumerRewards returns `true` if the validator with `consumerValidatorHeight` has been a consumer
+// validator for a long period of time and hence is eligible to receive rewards, and false otherwise
+func (k Keeper) IsEligibleForConsumerRewards(ctx sdk.Context, consumerValidatorHeight int64) bool {
+	numberOfBlocksToStartReceivingRewards := k.GetNumberOfEpochsToStartReceivingRewards(ctx) * k.GetBlocksPerEpoch(ctx)
+
+	// a validator is eligible for rewards if it has been a consumer validator for `NumberOfEpochsToStartReceivingRewards` epochs
+	return (ctx.BlockHeight() - consumerValidatorHeight) >= numberOfBlocksToStartReceivingRewards
+}
+
 // AllocateTokensToConsumerValidators allocates tokens
 // to the given consumer chain's validator set
 func (k Keeper) AllocateTokensToConsumerValidators(
@@ -183,6 +196,11 @@ func (k Keeper) AllocateTokensToConsumerValidators(
 
 	// Allocate tokens by iterating over the consumer validators
 	for _, consumerVal := range k.GetConsumerValSet(ctx, chainID) {
+		// if a validator is not eligible, this means that the other eligible validators would get more rewards
+		if !k.IsEligibleForConsumerRewards(ctx, consumerVal.JoinHeight) {
+			continue
+		}
+
 		consAddr := sdk.ConsAddress(consumerVal.ProviderConsAddr)
 
 		// get the validator tokens fraction using its voting power
@@ -253,6 +271,12 @@ func (k Keeper) GetConsumerRewardsPool(ctx sdk.Context) sdk.Coins {
 func (k Keeper) ComputeConsumerTotalVotingPower(ctx sdk.Context, chainID string) (totalPower int64) {
 	// sum the consumer validators set voting powers
 	for _, v := range k.GetConsumerValSet(ctx, chainID) {
+
+		// only consider the voting power of a validator that would receive rewards (i.e., validator has been validating for a number of blocks)
+		if !k.IsEligibleForConsumerRewards(ctx, v.JoinHeight) {
+			continue
+		}
+
 		totalPower += v.Power
 	}
 

x/ccv/provider/keeper/distribution_test.go

@@ -24,6 +24,14 @@ func TestComputeConsumerTotalVotingPower(t *testing.T) {
 	keeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
 	defer ctrl.Finish()
 
+	// `ComputeConsumerTotalVotingPower` used in this test retrieves the blocks per epoch, so we need to set this param
+	params := providertypes.DefaultParams()
+	params.BlocksPerEpoch = 1
+	keeper.SetParams(ctx, params)
+
+	// increase the block height so validators are eligible for consumer rewards (see `IsEligibleForConsumerRewards`)
+	ctx = ctx.WithBlockHeight(params.NumberOfEpochsToStartReceivingRewards * params.BlocksPerEpoch)
+
 	createVal := func(power int64) tmtypes.Validator {
 		signer := tmtypes.NewMockPV()
 		val := tmtypes.NewValidator(signer.PrivKey.PubKey(), power)
@@ -271,3 +279,21 @@ func TestGetConsumerRewardsAllocationNil(t *testing.T) {
 	}
 	require.Equal(t, expectedRewardAllocation, alloc)
 }
+
+func TestIsEligibleForConsumerRewards(t *testing.T) {
+	keeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
+	defer ctrl.Finish()
+
+	params := providertypes.DefaultParams()
+	params.NumberOfEpochsToStartReceivingRewards = 10
+	params.BlocksPerEpoch = 5
+	keeper.SetParams(ctx, params)
+
+	numberOfBlocks := params.NumberOfEpochsToStartReceivingRewards * params.BlocksPerEpoch
+
+	require.False(t, keeper.IsEligibleForConsumerRewards(ctx.WithBlockHeight(numberOfBlocks-1), 0))
+	require.True(t, keeper.IsEligibleForConsumerRewards(ctx.WithBlockHeight(numberOfBlocks), 0))
+	require.True(t, keeper.IsEligibleForConsumerRewards(ctx.WithBlockHeight(numberOfBlocks+1), 0))
+	require.True(t, keeper.IsEligibleForConsumerRewards(ctx.WithBlockHeight(numberOfBlocks+1), 1))
+	require.False(t, keeper.IsEligibleForConsumerRewards(ctx.WithBlockHeight(numberOfBlocks+1), 2))
+}

x/ccv/provider/keeper/params.go

@@ -78,6 +78,13 @@ func (k Keeper) GetBlocksPerEpoch(ctx sdk.Context) int64 {
 	return params.BlocksPerEpoch
 }
 
+// GetNumberOfEpochsToStartReceivingRewards returns the number of epochs needed by a validator to continuously validate
+// to start receiving rewards
+func (k Keeper) GetNumberOfEpochsToStartReceivingRewards(ctx sdk.Context) int64 {
+	params := k.GetParams(ctx)
+	return params.NumberOfEpochsToStartReceivingRewards
+}
+
 // GetParams returns the paramset for the provider module
 func (k Keeper) GetParams(ctx sdk.Context) types.Params {
 	store := ctx.KVStore(k.storeKey)

x/ccv/provider/keeper/params_test.go

@@ -50,6 +50,7 @@ func TestParams(t *testing.T) {
 			Amount: math.NewInt(10000000),
 		},
 		600,
+		24,
 	)
 	providerKeeper.SetParams(ctx, newParams)
 	params = providerKeeper.GetParams(ctx)