From bbe1c548b2b2b9a419d9ac1ed8aa1b0936dabdc9 Mon Sep 17 00:00:00 2001 From: insumity Date: Wed, 5 Jun 2024 14:52:42 +0200 Subject: [PATCH] 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](https://github.com/cosmos/ibc-go/compare/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] * update changelog entry --------- Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: mpoke * 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](https://github.com/bufbuild/buf-setup-action/compare/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] 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](https://github.com/bufbuild/buf-setup-action/compare/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] 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](https://github.com/spf13/viper/compare/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] 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](https://github.com/docker/login-action/compare/e92390c5fb421da1463c202d546fed0ec5c39f20...0d4c9c5ea7693da7b068278f7b52bda2a190a446) --- updated-dependencies: - dependency-name: docker/login-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * took into account comments --------- Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: mpoke --- .../provider/1925-apply-audit-suggestions.md | 3 + .../provider/1925-apply-audit-suggestions.md | 3 + docs/docs/features/power-shaping.md | 4 +- tests/e2e/steps_partial_set_security.go | 22 +++---- x/ccv/consumer/keeper/soft_opt_out.go | 2 +- x/ccv/provider/keeper/distribution.go | 12 ++-- x/ccv/provider/keeper/keeper.go | 26 +++----- x/ccv/provider/keeper/msg_server.go | 7 +-- x/ccv/provider/keeper/partial_set_security.go | 62 ++++++++++--------- .../keeper/partial_set_security_test.go | 49 +++++++-------- x/ccv/provider/keeper/proposal.go | 4 +- x/ccv/provider/keeper/relay.go | 7 ++- x/ccv/provider/keeper/validator_set_update.go | 4 +- 13 files changed, 104 insertions(+), 101 deletions(-) create mode 100644 .changelog/unreleased/bug-fixes/provider/1925-apply-audit-suggestions.md create mode 100644 .changelog/unreleased/state-breaking/provider/1925-apply-audit-suggestions.md diff --git a/.changelog/unreleased/bug-fixes/provider/1925-apply-audit-suggestions.md b/.changelog/unreleased/bug-fixes/provider/1925-apply-audit-suggestions.md new file mode 100644 index 0000000000..3d12033a4e --- /dev/null +++ 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 new file mode 100644 index 0000000000..3d12033a4e --- /dev/null +++ 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 index 9884c6184b..726ff031c6 100644 --- 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 index 164ff0d067..5e86ede3a8 100644 --- 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 index e5990ff65d..9221cca851 100644 --- 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 index 67e6193e5d..f59f111301 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -140,8 +140,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 } @@ -150,7 +150,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 @@ -190,14 +190,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) @@ -238,7 +238,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) { totalPower += v.Power } diff --git a/x/ccv/provider/keeper/keeper.go b/x/ccv/provider/keeper/keeper.go index 98930a3085..438cb5aa67 100644 --- 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 index 1f7f51b589..c4c7a04085 100644 --- 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 index 0d5177089a..1810bcc0f8 100644 --- 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,7 +122,7 @@ 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 @@ -118,10 +130,10 @@ func (k Keeper) ComputeMinPowerToOptIn(ctx sdk.Context, chainID string, bondedVa 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) diff --git a/x/ccv/provider/keeper/partial_set_security_test.go b/x/ccv/provider/keeper/partial_set_security_test.go index afbd420e7f..cb1fbab9d4 100644 --- a/x/ccv/provider/keeper/partial_set_security_test.go +++ b/x/ccv/provider/keeper/partial_set_security_test.go @@ -30,11 +30,11 @@ func TestHandleOptIn(t *testing.T) { providerAddr := types.NewProviderConsAddress([]byte("providerAddr")) // trying to opt in to a non-proposed and non-registered chain returns an error - require.Error(t, providerKeeper.HandleOptIn(ctx, "unknownChainID", providerAddr, nil)) + require.Error(t, providerKeeper.HandleOptIn(ctx, "unknownChainID", providerAddr, "")) providerKeeper.SetProposedConsumerChain(ctx, "chainID", 1) require.False(t, providerKeeper.IsOptedIn(ctx, "chainID", providerAddr)) - providerKeeper.HandleOptIn(ctx, "chainID", providerAddr, nil) + providerKeeper.HandleOptIn(ctx, "chainID", providerAddr, "") require.True(t, providerKeeper.IsOptedIn(ctx, "chainID", providerAddr)) } @@ -71,7 +71,7 @@ func TestHandleOptInWithConsumerKey(t *testing.T) { consumerKey := "{\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is=\"}" expectedConsumerPubKey, _ := providerKeeper.ParseConsumerKey(consumerKey) - err := providerKeeper.HandleOptIn(ctx, "chainID", providerAddr, &consumerKey) + err := providerKeeper.HandleOptIn(ctx, "chainID", providerAddr, consumerKey) require.NoError(t, err) // assert that the consumeKey was assigned to `providerAddr` validator on chain with id `chainID` @@ -299,56 +299,55 @@ func TestComputeMinPowerToOptIn(t *testing.T) { createStakingValidator(ctx, mocks, 5, 6), } - m, err := providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 100) + m, err := providerKeeper.ComputeMinPowerToOptIn(ctx, bondedValidators, 100) require.NoError(t, err) require.Equal(t, int64(1), m) - m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 97) + m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, bondedValidators, 97) require.NoError(t, err) require.Equal(t, int64(1), m) - m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 96) + m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, bondedValidators, 96) require.NoError(t, err) require.Equal(t, int64(3), m) - m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 85) + m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, bondedValidators, 85) require.NoError(t, err) require.Equal(t, int64(3), m) - m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 84) + m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, bondedValidators, 84) require.NoError(t, err) require.Equal(t, int64(5), m) - m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 65) + m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, bondedValidators, 65) require.NoError(t, err) require.Equal(t, int64(5), m) - m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 64) + m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, bondedValidators, 64) require.NoError(t, err) require.Equal(t, int64(6), m) - m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 41) + m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, bondedValidators, 50) require.NoError(t, err) require.Equal(t, int64(6), m) - m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 40) + m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, bondedValidators, 40) require.NoError(t, err) require.Equal(t, int64(10), m) - m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 1) + m, err = providerKeeper.ComputeMinPowerToOptIn(ctx, bondedValidators, 1) require.NoError(t, err) require.Equal(t, int64(10), m) - // exceptional case when we erroneously call with `topN == 0` or `topN > 100` - _, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 0) + _, err = providerKeeper.ComputeMinPowerToOptIn(ctx, bondedValidators, 0) require.Error(t, err) - _, err = providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 101) + _, err = providerKeeper.ComputeMinPowerToOptIn(ctx, bondedValidators, 101) require.Error(t, err) } -// TestFilterOptedInAndAllowAndDenylistedPredicate returns true if `validator` is opted in, in `chainID. -func TestFilterOptedInAndAllowAndDenylistedPredicate(t *testing.T) { +// TestCanValidateChain returns true if `validator` is opted in, in `chainID. +func TestCanValidateChain(t *testing.T) { providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) defer ctrl.Finish() @@ -357,24 +356,24 @@ func TestFilterOptedInAndAllowAndDenylistedPredicate(t *testing.T) { providerAddr := types.NewProviderConsAddress(consAddr) // with no allowlist or denylist, the validator has to be opted in, in order to consider it - require.False(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr)) + require.False(t, providerKeeper.CanValidateChain(ctx, "chainID", providerAddr)) providerKeeper.SetOptedIn(ctx, "chainID", types.NewProviderConsAddress(consAddr)) - require.True(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr)) + require.True(t, providerKeeper.CanValidateChain(ctx, "chainID", providerAddr)) // create an allow list but do not add the validator `providerAddr` to it validatorA := createStakingValidator(ctx, mocks, 1, 1) consAddrA, _ := validatorA.GetConsAddr() providerKeeper.SetAllowlist(ctx, "chainID", types.NewProviderConsAddress(consAddrA)) - require.False(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr)) + require.False(t, providerKeeper.CanValidateChain(ctx, "chainID", providerAddr)) providerKeeper.SetAllowlist(ctx, "chainID", types.NewProviderConsAddress(consAddr)) - require.True(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr)) + require.True(t, providerKeeper.CanValidateChain(ctx, "chainID", providerAddr)) // create a denylist but do not add validator `providerAddr` to it providerKeeper.SetDenylist(ctx, "chainID", types.NewProviderConsAddress(consAddrA)) - require.True(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr)) + require.True(t, providerKeeper.CanValidateChain(ctx, "chainID", providerAddr)) // add validator `providerAddr` to the denylist providerKeeper.SetDenylist(ctx, "chainID", types.NewProviderConsAddress(consAddr)) - require.False(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr)) + require.False(t, providerKeeper.CanValidateChain(ctx, "chainID", providerAddr)) } func TestCapValidatorSet(t *testing.T) { @@ -536,7 +535,7 @@ func noMoreThanPercent(validators []types.ConsumerValidator, percent uint32) boo } for _, v := range validators { - if (float64(v.Power)/float64(sum))*100.0 > float64(percent) { + if float64(v.Power)*100.0 > float64(percent)*float64(sum) { return false } } diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index 15281c797b..651d9b95f5 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -297,9 +297,11 @@ func (k Keeper) MakeConsumerGenesis( 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, prop.Top_N) + minPower, err := k.ComputeMinPowerToOptIn(ctx, bondedValidators, prop.Top_N) if err == nil { k.OptInTopNValidators(ctx, chainID, bondedValidators, minPower) + } else { + return gen, nil, err } } diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 230ed0a96a..96cca06bfc 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -223,11 +223,14 @@ func (k Keeper) QueueVSCPackets(ctx sdk.Context) { for _, chain := range k.GetAllConsumerChains(ctx) { currentValidators := k.GetConsumerValSet(ctx, chain.ChainId) - if topN, found := k.GetTopN(ctx, chain.ChainId); found && topN > 0 { + if chain.Top_N > 0 { // in a Top-N chain, we automatically opt in all validators that belong to the top N - minPower, err := k.ComputeMinPowerToOptIn(ctx, chain.ChainId, bondedValidators, topN) + minPower, err := k.ComputeMinPowerToOptIn(ctx, bondedValidators, chain.Top_N) if err == nil { k.OptInTopNValidators(ctx, chain.ChainId, bondedValidators, minPower) + } else { + // we just log here and do not panic because panic-ing would halt the provider chain + k.Logger(ctx).Error("failed to compute min power to opt in for chain", "chain", chain.ChainId, "error", err) } } diff --git a/x/ccv/provider/keeper/validator_set_update.go b/x/ccv/provider/keeper/validator_set_update.go index 264fdd67f2..7fa175e1b9 100644 --- a/x/ccv/provider/keeper/validator_set_update.go +++ b/x/ccv/provider/keeper/validator_set_update.go @@ -101,12 +101,12 @@ func DiffValidators( ) []abci.ValidatorUpdate { var updates []abci.ValidatorUpdate - isCurrentValidator := make(map[string]types.ConsumerValidator) + isCurrentValidator := make(map[string]types.ConsumerValidator, len(currentValidators)) for _, val := range currentValidators { isCurrentValidator[val.ConsumerPublicKey.String()] = val } - isNextValidator := make(map[string]types.ConsumerValidator) + isNextValidator := make(map[string]types.ConsumerValidator, len(nextValidators)) for _, val := range nextValidators { isNextValidator[val.ConsumerPublicKey.String()] = val }