fix!: apply audit suggestions (#1925)

* init commit * added CHANGELOG entries * added nit simplification change * addressed comment by Hypha * took into account err returned by ComputeMinPowerToOptIn * fixed test failing * build(deps): bump github.com/cosmos/ibc-go/v7 from 7.5.0 to 7.5.1 (#1924) * build(deps): bump github.com/cosmos/ibc-go/v7 from 7.5.0 to 7.5.1 Bumps [github.com/cosmos/ibc-go/v7](https://github.com/cosmos/ibc-go) from 7.5.0 to 7.5.1. - [Release notes](https://github.com/cosmos/ibc-go/releases) - [Changelog](https://github.com/cosmos/ibc-go/blob/v7.5.1/CHANGELOG.md) - [Commits](cosmos/ibc-go@v7.5.0...v7.5.1) --- updated-dependencies: - dependency-name: github.com/cosmos/ibc-go/v7 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * update changelog entry --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: mpoke <[email protected]> * build(deps): bump bufbuild/buf-setup-action from 1.32.0 to 1.32.1 (#1923) Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.32.0 to 1.32.1. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](bufbuild/buf-setup-action@v1.32.0...v1.32.1) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump bufbuild/buf-setup-action from 1.32.1 to 1.32.2 (#1934) Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.32.1 to 1.32.2. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](bufbuild/buf-setup-action@v1.32.1...v1.32.2) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump github.com/spf13/viper from 1.18.2 to 1.19.0 (#1936) Bumps [github.com/spf13/viper](https://github.com/spf13/viper) from 1.18.2 to 1.19.0. - [Release notes](https://github.com/spf13/viper/releases) - [Commits](spf13/viper@v1.18.2...v1.19.0) --- updated-dependencies: - dependency-name: github.com/spf13/viper dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump docker/login-action from 3.1.0 to 3.2.0 (#1935) Bumps [docker/login-action](https://github.com/docker/login-action) from 3.1.0 to 3.2.0. - [Release notes](https://github.com/docker/login-action/releases) - [Commits](docker/login-action@e92390c...0d4c9c5) --- updated-dependencies: - dependency-name: docker/login-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * took into account comments --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: mpoke <[email protected]>
cosmos · Jun 13, 2024 · 7651cdc · 7651cdc
1 parent d7f04a5
commit 7651cdc
Show file tree

Hide file tree

Showing 13 changed files with 104 additions and 101 deletions.
diff --git a/.changelog/unreleased/bug-fixes/provider/1925-apply-audit-suggestions.md b/.changelog/unreleased/bug-fixes/provider/1925-apply-audit-suggestions.md
@@ -0,0 +1,3 @@
+- Apply audit suggestions that include a bug fix in the way we compute the
+  maximum capped power. ([\#1925](https://github.com/cosmos/interchain-
+  security/pull/1925))
diff --git a/.changelog/unreleased/state-breaking/provider/1925-apply-audit-suggestions.md b/.changelog/unreleased/state-breaking/provider/1925-apply-audit-suggestions.md
@@ -0,0 +1,3 @@
+- Apply audit suggestions that include a bug fix in the way we compute the
+  maximum capped power. ([\#1925](https://github.com/cosmos/interchain-
+  security/pull/1925))
diff --git a/docs/docs/features/power-shaping.md b/docs/docs/features/power-shaping.md
@@ -6,7 +6,9 @@ several "power shaping" mechanisms for consumer chains.
 These are:
 1) **Capping the size of the validator set**: The consumer chain can specify a maximum number of validators it
 wants to have in its validator set. This can be used to limit the number of validators in the set, which can
-be useful for chains that want to have a smaller validator set for faster blocks or lower overhead.
+be useful for chains that want to have a smaller validator set for faster blocks or lower overhead. If more validators
+than the maximum size have opted in on a consumer chain, only the validators with the highest power, up to the specified
+maximum, will validate the consumer chain.
 :::info
 This is only applicable to Opt In chains (chains with Top N = 0).
 :::

diff --git a/tests/e2e/steps_partial_set_security.go b/tests/e2e/steps_partial_set_security.go
@@ -1391,11 +1391,11 @@ func stepsValidatorsPowerCappedChain() []Step {
 				ChainID("consu"): ChainState{
 					ValPowers: &map[ValidatorID]uint{
 						// the powers of the validators on the consumer chain are different from the provider chain
-						// because the consumer chain is power capped. Note that the total power is 600 (= 194 + 203 + 203)
-						// and 203 / 600.0 = 0.338 < 34% that is the power cap.
-						ValidatorID("alice"): 194,
-						ValidatorID("bob"):   203,
-						ValidatorID("carol"): 203,
+						// because the consumer chain is power capped. Note that the total power is 600 (= 192 + 204 + 204)
+						// and 204 / 600.0 = 0.34 <= 34% that is the power cap.
+						ValidatorID("alice"): 192,
+						ValidatorID("bob"):   204,
+						ValidatorID("carol"): 204,
 					},
 				},
 			},
@@ -1428,9 +1428,9 @@ func stepsValidatorsPowerCappedChain() []Step {
 			State: State{
 				ChainID("consu"): ChainState{
 					ValPowers: &map[ValidatorID]uint{
-						ValidatorID("alice"): 194,
-						ValidatorID("bob"):   203,
-						ValidatorID("carol"): 203,
+						ValidatorID("alice"): 192,
+						ValidatorID("bob"):   204,
+						ValidatorID("carol"): 204,
 					},
 				},
 				ChainID("provi"): ChainState{
@@ -1454,9 +1454,9 @@ func stepsValidatorsPowerCappedChain() []Step {
 					ValPowers: &map[ValidatorID]uint{
 						// "carol" has opted out, and we only have 2 validators left validating. Power capping only operates
 						// in a best-effort basis and with 2 validators we cannot guarantee that no validator has more
-						// than 34% of the voting power. In this case, both validators get the same voting power (50% = 101 / 202).
-						ValidatorID("alice"): 101,
-						ValidatorID("bob"):   101,
+						// than 34% of the voting power. In this case, both validators get the same voting power (50% = 102 / 204).
+						ValidatorID("alice"): 102,
+						ValidatorID("bob"):   102,
 						ValidatorID("carol"): 0,
 					},
 				},

diff --git a/x/ccv/consumer/keeper/soft_opt_out.go b/x/ccv/consumer/keeper/soft_opt_out.go
@@ -55,7 +55,7 @@ func (k Keeper) UpdateSmallestNonOptOutPower(ctx sdk.Context) {
 	// get total power in set
 	totalPower := sdk.ZeroDec()
 	for _, val := range valset {
-		totalPower = totalPower.Add(sdk.NewDecFromInt(sdk.NewInt(val.Power)))
+		totalPower = totalPower.Add(sdk.NewDec(val.Power))
 	}
 
 	// get power of the smallest validator that cannot soft opt out

diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go
@@ -157,8 +157,8 @@ func (k Keeper) AllocateTokensToConsumerValidators(
 	}
 
 	// get the total voting power of the consumer valset
-	totalPower := k.ComputeConsumerTotalVotingPower(ctx, chainID)
-	if totalPower == 0 {
+	totalPower := math.LegacyNewDec(k.ComputeConsumerTotalVotingPower(ctx, chainID))
+	if totalPower.IsZero() {
 		return allocated
 	}
 
@@ -171,7 +171,7 @@ func (k Keeper) AllocateTokensToConsumerValidators(
 		consAddr := sdk.ConsAddress(consumerVal.ProviderConsAddr)
 
 		// get the validator tokens fraction using its voting power
-		powerFraction := math.LegacyNewDec(consumerVal.Power).QuoTruncate(math.LegacyNewDec(totalPower))
+		powerFraction := math.LegacyNewDec(consumerVal.Power).QuoTruncate(totalPower)
 		tokensFraction := tokens.MulDecTruncate(powerFraction)
 
 		// get the validator type struct for the consensus address
@@ -211,14 +211,14 @@ func (k Keeper) TransferConsumerRewardsToDistributionModule(
 	}
 
 	// Truncate coin rewards
-	rewardsToSend, _ := allocation.Rewards.TruncateDecimal()
+	rewardsToSend, remRewards := allocation.Rewards.TruncateDecimal()
 
 	// NOTE the consumer rewards allocation isn't a module account, however its coins
 	// are held in the consumer reward pool module account. Thus the consumer
 	// rewards allocation must be reduced separately from the SendCoinsFromModuleToAccount call.
 
 	// Update consumer rewards allocation with the remaining decimal coins
-	allocation.Rewards = allocation.Rewards.Sub(sdk.NewDecCoinsFromCoins(rewardsToSend...))
+	allocation.Rewards = remRewards
 
 	// Send coins to distribution module account
 	err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ConsumerRewardsPool, distrtypes.ModuleName, rewardsToSend)
@@ -259,7 +259,7 @@ func (k Keeper) GetConsumerRewardsPool(ctx sdk.Context) sdk.Coins {
 // ComputeConsumerTotalVotingPower returns the validator set total voting power
 // for the given consumer chain
 func (k Keeper) ComputeConsumerTotalVotingPower(ctx sdk.Context, chainID string) (totalPower int64) {
-	// sum the opted-in validators set voting powers
+	// 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)

diff --git a/x/ccv/provider/keeper/keeper.go b/x/ccv/provider/keeper/keeper.go
@@ -257,7 +257,7 @@ func (k Keeper) GetAllConsumerChains(ctx sdk.Context) (chains []types.Chain) {
 
 		var minPowerInTopN int64
 		if found && topN > 0 {
-			res, err := k.ComputeMinPowerToOptIn(ctx, chainID, k.stakingKeeper.GetLastValidators(ctx), topN)
+			res, err := k.ComputeMinPowerToOptIn(ctx, k.stakingKeeper.GetLastValidators(ctx), topN)
 			if err != nil {
 				k.Logger(ctx).Error("failed to compute min power to opt in for chain", "chain", chainID, "error", err)
 				minPowerInTopN = -1
@@ -759,13 +759,9 @@ func (k Keeper) GetValidatorSetUpdateId(ctx sdk.Context) (validatorSetUpdateId u
 	bz := store.Get(types.ValidatorSetUpdateIdKey())
 
 	if bz == nil {
-		validatorSetUpdateId = 0
-	} else {
-		// Unmarshal
-		validatorSetUpdateId = binary.BigEndian.Uint64(bz)
+		return 0
 	}
-
-	return validatorSetUpdateId
+	return binary.BigEndian.Uint64(bz)
 }
 
 // SetValsetUpdateBlockHeight sets the block height for a given valset update id
@@ -1302,9 +1298,11 @@ func (k Keeper) HasToValidate(
 	bondedValidators := k.stakingKeeper.GetLastValidators(ctx)
 	if topN, found := k.GetTopN(ctx, chainID); found && topN > 0 {
 		// in a Top-N chain, we automatically opt in all validators that belong to the top N
-		minPower, err := k.ComputeMinPowerToOptIn(ctx, chainID, bondedValidators, topN)
+		minPower, err := k.ComputeMinPowerToOptIn(ctx, bondedValidators, topN)
 		if err == nil {
 			k.OptInTopNValidators(ctx, chainID, bondedValidators, minPower)
+		} else {
+			k.Logger(ctx).Error("failed to compute min power to opt in for chain", "chain", chainID, "error", err)
 		}
 	}
 
@@ -1528,11 +1526,7 @@ func (k Keeper) IsAllowlistEmpty(ctx sdk.Context, chainID string) bool {
 	iterator := sdk.KVStorePrefixIterator(store, types.ChainIdWithLenKey(types.AllowlistPrefix, chainID))
 	defer iterator.Close()
 
-	if iterator.Valid() {
-		return false
-	}
-
-	return true
+	return !iterator.Valid()
 }
 
 // SetDenylist denylists validator with `providerAddr` address on chain `chainID`
@@ -1595,9 +1589,5 @@ func (k Keeper) IsDenylistEmpty(ctx sdk.Context, chainID string) bool {
 	iterator := sdk.KVStorePrefixIterator(store, types.ChainIdWithLenKey(types.DenylistPrefix, chainID))
 	defer iterator.Close()
 
-	if iterator.Valid() {
-		return false
-	}
-
-	return true
+	return !iterator.Valid()
 }
diff --git a/x/ccv/provider/keeper/msg_server.go b/x/ccv/provider/keeper/msg_server.go
@@ -157,12 +157,7 @@ func (k msgServer) OptIn(goCtx context.Context, msg *types.MsgOptIn) (*types.Msg
 	}
 	providerConsAddr := types.NewProviderConsAddress(consAddrTmp)
 
-	if msg.ConsumerKey != "" {
-		err = k.Keeper.HandleOptIn(ctx, msg.ChainId, providerConsAddr, &msg.ConsumerKey)
-	} else {
-		err = k.Keeper.HandleOptIn(ctx, msg.ChainId, providerConsAddr, nil)
-	}
-
+	err = k.Keeper.HandleOptIn(ctx, msg.ChainId, providerConsAddr, msg.ConsumerKey)
 	if err != nil {
 		return nil, err
 	}

diff --git a/x/ccv/provider/keeper/partial_set_security.go b/x/ccv/provider/keeper/partial_set_security.go
@@ -14,7 +14,7 @@ import (
 
 // HandleOptIn prepares validator `providerAddr` to opt in to `chainID` with an optional `consumerKey` consumer public key.
 // Note that the validator only opts in at the end of an epoch.
-func (k Keeper) HandleOptIn(ctx sdk.Context, chainID string, providerAddr types.ProviderConsAddress, consumerKey *string) error {
+func (k Keeper) HandleOptIn(ctx sdk.Context, chainID string, providerAddr types.ProviderConsAddress, consumerKey string) error {
 	if !k.IsConsumerProposedOrRegistered(ctx, chainID) {
 		return errorsmod.Wrapf(
 			types.ErrUnknownConsumerChainId,
@@ -23,8 +23,8 @@ func (k Keeper) HandleOptIn(ctx sdk.Context, chainID string, providerAddr types.
 
 	k.SetOptedIn(ctx, chainID, providerAddr)
 
-	if consumerKey != nil {
-		consumerTMPublicKey, err := k.ParseConsumerKey(*consumerKey)
+	if consumerKey != "" {
+		consumerTMPublicKey, err := k.ParseConsumerKey(consumerKey)
 		if err != nil {
 			return err
 		}
@@ -63,13 +63,22 @@ func (k Keeper) HandleOptOut(ctx sdk.Context, chainID string, providerAddr types
 				"validator with consensus address %s could not be found", providerAddr.ToSdkConsAddr())
 		}
 		power := k.stakingKeeper.GetLastValidatorPower(ctx, validator.GetOperator())
-		minPowerToOptIn, err := k.ComputeMinPowerToOptIn(ctx, chainID, k.stakingKeeper.GetLastValidators(ctx), topN)
+		minPowerToOptIn, err := k.ComputeMinPowerToOptIn(ctx, k.stakingKeeper.GetLastValidators(ctx), topN)
 
-		if err != nil || power >= minPowerToOptIn {
+		if err != nil {
+			k.Logger(ctx).Error("failed to compute min power to opt in for chain", "chain", chainID, "error", err)
 			return errorsmod.Wrapf(
 				types.ErrCannotOptOutFromTopN,
-				"validator with power (%d) cannot opt out from Top N chain because all validators"+
-					"with at least %d power have to validate", power, minPowerToOptIn)
+				"validator with power (%d) cannot opt out from Top N chain (%s) because the min power"+
+					" could not be computed: %s", power, chainID, err.Error())
+
+		}
+
+		if power >= minPowerToOptIn {
+			return errorsmod.Wrapf(
+				types.ErrCannotOptOutFromTopN,
+				"validator with power (%d) cannot opt out from Top N chain (%s) because all validators"+
+					" with at least %d power have to validate", power, chainID, minPowerToOptIn)
 		}
 	}
 
@@ -97,11 +106,14 @@ func (k Keeper) OptInTopNValidators(ctx sdk.Context, chainID string, bondedValid
 }
 
 // ComputeMinPowerToOptIn returns the minimum power needed for a validator (from the bonded validators)
-// to belong to the `topN` validators. `chainID` is only used for logging purposes.
-func (k Keeper) ComputeMinPowerToOptIn(ctx sdk.Context, chainID string, bondedValidators []stakingtypes.Validator, topN uint32) (int64, error) {
+// to belong to the `topN` validators for a Top N chain.
+func (k Keeper) ComputeMinPowerToOptIn(ctx sdk.Context, bondedValidators []stakingtypes.Validator, topN uint32) (int64, error) {
 	if topN == 0 || topN > 100 {
-		return 0, fmt.Errorf("trying to compute minimum power with an incorrect topN value (%d)."+
-			"topN has to be between (0, 100]", topN)
+		// Note that Top N chains have a lower limit on `topN`, namely that topN cannot be less than 50.
+		// However, we can envision that this method could be used for other (future) reasons where this might not
+		// be the case. For this, this method operates for `topN`s in (0, 100].
+		return 0, fmt.Errorf("trying to compute minimum power with an incorrect"+
+			" topN value (%d). topN has to be in (0, 100]", topN)
 	}
 
 	totalPower := sdk.ZeroDec()
@@ -110,18 +122,18 @@ func (k Keeper) ComputeMinPowerToOptIn(ctx sdk.Context, chainID string, bondedVa
 	for _, val := range bondedValidators {
 		power := k.stakingKeeper.GetLastValidatorPower(ctx, val.GetOperator())
 		powers = append(powers, power)
-		totalPower = totalPower.Add(sdk.NewDecFromInt(sdk.NewInt(power)))
+		totalPower = totalPower.Add(sdk.NewDec(power))
 	}
 
 	// sort by powers descending
 	sort.Slice(powers, func(i, j int) bool {
 		return powers[i] > powers[j]
 	})
 
-	topNThreshold := sdk.NewDecFromInt(sdk.NewInt(int64(topN))).QuoInt64(int64(100))
+	topNThreshold := sdk.NewDec(int64(topN)).QuoInt64(int64(100))
 	powerSum := sdk.ZeroDec()
 	for _, power := range powers {
-		powerSum = powerSum.Add(sdk.NewDecFromInt(sdk.NewInt(power)))
+		powerSum = powerSum.Add(sdk.NewDec(power))
 		if powerSum.Quo(totalPower).GTE(topNThreshold) {
 			return power, nil
 		}
@@ -140,18 +152,12 @@ func (k Keeper) CapValidatorSet(ctx sdk.Context, chainID string, validators []ty
 		return validators
 	}
 
-	if validatorSetCap, found := k.GetValidatorSetCap(ctx, chainID); found && validatorSetCap != 0 {
+	if validatorSetCap, found := k.GetValidatorSetCap(ctx, chainID); found && validatorSetCap != 0 && int(validatorSetCap) < len(validators) {
 		sort.Slice(validators, func(i, j int) bool {
 			return validators[i].Power > validators[j].Power
 		})
 
-		minNumberOfValidators := 0
-		if len(validators) < int(validatorSetCap) {
-			minNumberOfValidators = len(validators)
-		} else {
-			minNumberOfValidators = int(validatorSetCap)
-		}
-		return validators[:minNumberOfValidators]
+		return validators[:int(validatorSetCap)]
 	} else {
 		return validators
 	}
@@ -209,9 +215,8 @@ func NoMoreThanPercentOfTheSum(validators []types.ConsumerValidator, percent uin
 	// If `s > n * maxPower` there's no solution and the algorithm would set everything to `maxPower`.
 	// ----------------
 
-	// Computes `(sum(validators) * percent) / 100`. Because `sdk.Dec` does not provide a `Floor` function, but only
-	// a `Ceil` one, we use `Ceil` and subtract one.
-	maxPower := sdk.NewDec(sum(validators)).Mul(sdk.NewDec(int64(percent))).QuoInt64(100).Ceil().RoundInt64() - 1
+	// Computes `floor((sum(validators) * percent) / 100)`
+	maxPower := sdk.NewDec(sum(validators)).Mul(sdk.NewDec(int64(percent))).QuoInt64(100).TruncateInt64()
 
 	if maxPower == 0 {
 		// edge case: set `maxPower` to 1 to avoid setting the power of a validator to 0
@@ -271,8 +276,9 @@ func NoMoreThanPercentOfTheSum(validators []types.ConsumerValidator, percent uin
 	return updatedValidators
 }
 
-// FilterOptedInAndAllowAndDenylistedPredicate filters the opted-in validators that are allowlisted and not denylisted
-func (k Keeper) FilterOptedInAndAllowAndDenylistedPredicate(ctx sdk.Context, chainID string, providerAddr types.ProviderConsAddress) bool {
+// CanValidateChain returns true if the validator `providerAddr` is opted-in to chain `chainID` and the allowlist and
+// denylist do not prevent the validator from validating the chain.
+func (k Keeper) CanValidateChain(ctx sdk.Context, chainID string, providerAddr types.ProviderConsAddress) bool {
 	// only consider opted-in validators
 	return k.IsOptedIn(ctx, chainID, providerAddr) &&
 		// if an allowlist is declared, only consider allowlisted validators
@@ -287,7 +293,7 @@ func (k Keeper) FilterOptedInAndAllowAndDenylistedPredicate(ctx sdk.Context, cha
 func (k Keeper) ComputeNextValidators(ctx sdk.Context, chainID string, bondedValidators []stakingtypes.Validator) []types.ConsumerValidator {
 	nextValidators := k.FilterValidators(ctx, chainID, bondedValidators,
 		func(providerAddr types.ProviderConsAddress) bool {
-			return k.FilterOptedInAndAllowAndDenylistedPredicate(ctx, chainID, providerAddr)
+			return k.CanValidateChain(ctx, chainID, providerAddr)
 		})
 
 	nextValidators = k.CapValidatorSet(ctx, chainID, nextValidators)