Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: apply audit suggestions #1925

Merged
merged 12 commits into from
Jun 5, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
- Apply audit suggestions that include a bug fix in the way we compute the
insumity marked this conversation as resolved.
Show resolved Hide resolved
maximum capped power. ([\#1925](https://github.com/cosmos/interchain-
security/pull/1925))
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
- Apply audit suggestions that include a bug fix in the way we compute the
insumity marked this conversation as resolved.
Show resolved Hide resolved
maximum capped power. ([\#1925](https://github.com/cosmos/interchain-
security/pull/1925))
4 changes: 3 additions & 1 deletion docs/docs/features/power-shaping.md
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not audit specific but was added after a question raised by @LexaMichaelides (Hypha):

What is the intended behaviour when the cap is exceeded? What logic governs which validators get to be in the set and which are excluded?

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.
Comment on lines +9 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider rephrasing to avoid repetition and enhance readability.

- be useful for chains that want to have a smaller validator set for faster blocks or lower overhead. If more validators
+ be beneficial for chains seeking a smaller validator set for faster blocks or lower overhead. If more validators
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
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.
be beneficial for chains seeking 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.
Tools
LanguageTool

[style] ~9-~9: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...et, which can be useful for chains that want to have a smaller validator set for faster...

Markdownlint

11-11: null
Lists should be surrounded by blank lines

:::info
This is only applicable to Opt In chains (chains with Top N = 0).
:::
22 changes: 11 additions & 11 deletions tests/e2e/steps_partial_set_security.go
Original file line number Diff line number Diff line change
@@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We fixed a rounding issue based on "Inaccurate Rounding of maxPower" suggestion and hence those numbers had to be changed.

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,
},
},
2 changes: 1 addition & 1 deletion x/ccv/consumer/keeper/soft_opt_out.go
Original file line number Diff line number Diff line change
@@ -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
12 changes: 6 additions & 6 deletions x/ccv/provider/keeper/distribution.go
Original file line number Diff line number Diff line change
@@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed due to the "math.LegacyNewDec(totalPower) should be initialized once before entering the loop" suggestion.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken from "Minor Code Improvements" and specifically "The new allocation.Rewards value (code) should match the changeCoins value from [...]"


// 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed due to the "The comment in the function ComputeConsumerTotalVotingPower incorrectly states that it sums the voting powers of opted-in validators. Instead, it sums the voting powers of the actual validator set." comment.

for _, v := range k.GetConsumerValSet(ctx, chainID) {
totalPower += v.Power
}
22 changes: 6 additions & 16 deletions x/ccv/provider/keeper/keeper.go
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in this file stem from the "Minor Code Improvements" suggestions.

}

return validatorSetUpdateId
return binary.BigEndian.Uint64(bz)
}

// SetValsetUpdateBlockHeight sets the block height for a given valset update id
@@ -1305,6 +1301,8 @@ func (k Keeper) HasToValidate(
minPower, err := k.ComputeMinPowerToOptIn(ctx, chainID, 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()
}
7 changes: 1 addition & 6 deletions x/ccv/provider/keeper/msg_server.go
Original file line number Diff line number Diff line change
@@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had by reference to be able to show that it's optional but following "Minor Code Improvements" we remove the reference to msg.ConsumerKey.

if err != nil {
return nil, err
}
57 changes: 30 additions & 27 deletions x/ccv/provider/keeper/partial_set_security.go
Original file line number Diff line number Diff line change
@@ -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
}
@@ -65,11 +65,20 @@ func (k Keeper) HandleOptOut(ctx sdk.Context, chainID string, providerAddr types
power := k.stakingKeeper.GetLastValidatorPower(ctx, validator.GetOperator())
minPowerToOptIn, err := k.ComputeMinPowerToOptIn(ctx, chainID, 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())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that:

cosmossdk.io/errors.Wrapf does not support error-wrapping directive %w

and because of this I use %s with err.Error() here.


}

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,11 @@ 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.
// to belong to the `topN` validators for a Top N chain.
func (k Keeper) ComputeMinPowerToOptIn(ctx sdk.Context, chainID string, 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)
if topN < 50 || topN > 100 {
insumity marked this conversation as resolved.
Show resolved Hide resolved
return 0, fmt.Errorf("trying to compute minimum power for a Top N chain (%s) with an incorrect"+
" topN value (%d). topN has to be in [50, 100]", chainID, topN)
insumity marked this conversation as resolved.
Show resolved Hide resolved
}

totalPower := sdk.ZeroDec()
@@ -110,18 +119,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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using directly NewDec here and above because of "Code Efficiency Improvements and Optimization Opportunities" suggestions.

if powerSum.Quo(totalPower).GTE(topNThreshold) {
return power, nil
}
@@ -140,18 +149,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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed based on "Code Efficiency Improvements and Optimization Opportunities" suggestions.

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 +212,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()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed due to "Inaccurate Rounding of maxPower" suggestion.
Note that I did not apply the "Potential Optimization in Max Power Capping Logic" suggestion because it seems to me that if we do everything in one for loop, we would make the code slightly harder to read with no real performance benefit (i.e., number of validators are a few hundreds).


if maxPower == 0 {
// edge case: set `maxPower` to 1 to avoid setting the power of a validator to 0
@@ -271,8 +273,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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed based on "Potential Optimization in Max Power Capping Logic" suggestion:

The function FilterOptedInAndAllowAndDenylistedPredicate serves as a predicate to check whether a given validator meets certain criteria. However, naming it as a "Filter" may be misleading.

// only consider opted-in validators
return k.IsOptedIn(ctx, chainID, providerAddr) &&
// if an allowlist is declared, only consider allowlisted validators
@@ -287,7 +290,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)
Loading