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!: make OptInTopNValidators return an error #2259

Merged
merged 1 commit into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion x/ccv/provider/keeper/consumer_lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -330,8 +330,14 @@ func (k Keeper) MakeConsumerGenesis(
"consumerId", consumerId,
"minPower", minPower,
)
k.OptInTopNValidators(ctx, consumerId, activeValidators, minPower)

// set the minimal power of validators in the top N in the store
k.SetMinimumPowerInTopN(ctx, consumerId, minPower)

MSalopek marked this conversation as resolved.
Show resolved Hide resolved
err = k.OptInTopNValidators(ctx, consumerId, activeValidators, minPower)
if err != nil {
MSalopek marked this conversation as resolved.
Show resolved Hide resolved
return gen, nil, fmt.Errorf("unable to opt in topN validators in MakeConsumerGenesis, consumerId(%s): %w", consumerId, err)
}
}

// need to use the bondedValidators, not activeValidators, here since the chain might be opt-in and allow inactive vals
Expand Down
37 changes: 21 additions & 16 deletions x/ccv/provider/keeper/partial_set_security.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package keeper

import (
"fmt"

errorsmod "cosmossdk.io/errors"
storetypes "cosmossdk.io/store/types"

Expand Down Expand Up @@ -101,40 +103,43 @@ func (k Keeper) HandleOptOut(ctx sdk.Context, consumerId string, providerAddr ty
}

// OptInTopNValidators opts in to `consumerId` all the `bondedValidators` that have at least `minPowerToOptIn` power
func (k Keeper) OptInTopNValidators(ctx sdk.Context, consumerId string, bondedValidators []stakingtypes.Validator, minPowerToOptIn int64) {
func (k Keeper) OptInTopNValidators(
ctx sdk.Context,
consumerId string,
bondedValidators []stakingtypes.Validator,
minPowerToOptIn int64,
) error {
for _, val := range bondedValidators {
// log the validator
k.Logger(ctx).Debug("Checking whether to opt in validator because of top N", "validator", val.GetOperator())
k.Logger(ctx).Debug("Checking whether to opt in validator because of top N",
"consumerId", consumerId,
"validator", val.GetOperator(),
)

valAddr, err := sdk.ValAddressFromBech32(val.GetOperator())
if err != nil {
k.Logger(ctx).Error("could not retrieve validator address from operator address",
"validator operator address", val.GetOperator(),
"error", err.Error())
continue
return fmt.Errorf("converting operator address to validator address, consumerId(%s), validator(%s): %w",
consumerId, val.GetOperator(), err)
}
power, err := k.stakingKeeper.GetLastValidatorPower(ctx, valAddr)
if err != nil {
k.Logger(ctx).Error("could not retrieve last power of validator",
"validator operator address", val.GetOperator(),
"error", err.Error())
continue
return fmt.Errorf("getting validator power, consumerId(%s), validator(%s): %w",
consumerId, val.GetOperator(), err)
}
if power >= minPowerToOptIn {
consAddr, err := val.GetConsAddr()
if err != nil {
k.Logger(ctx).Error("could not retrieve validator consensus address",
"validator operator address", val.GetOperator(),
"error", err.Error())
continue
return fmt.Errorf("getting validator consensus address, consumerId(%s), validator(%s): %w",
consumerId, val.GetOperator(), err)
}

k.Logger(ctx).Debug("Opting in validator", "validator", val.GetOperator())
k.Logger(ctx).Debug("Opting in validator", "consumerId", consumerId, "validator", val.GetOperator())

// if validator already exists it gets overwritten
// if validator is already opted in, it gets overwritten
k.SetOptedIn(ctx, consumerId, types.NewProviderConsAddress(consAddr))
} // else validators that do not belong to the top N validators but were opted in, remain opted in
}
return nil
}

//
Expand Down
12 changes: 8 additions & 4 deletions x/ccv/provider/keeper/partial_set_security_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,8 @@ func TestOptInTopNValidators(t *testing.T) {
valDConsAddr, _ := valD.GetConsAddr()

// Start Test 1: opt in all validators with power >= 0
providerKeeper.OptInTopNValidators(ctx, CONSUMER_ID, []stakingtypes.Validator{valA, valB, valC, valD}, 0)
err := providerKeeper.OptInTopNValidators(ctx, CONSUMER_ID, []stakingtypes.Validator{valA, valB, valC, valD}, 0)
require.NoError(t, err)
expectedOptedInValidators := []providertypes.ProviderConsAddress{
providertypes.NewProviderConsAddress(valAConsAddr),
providertypes.NewProviderConsAddress(valBConsAddr),
Expand Down Expand Up @@ -216,7 +217,8 @@ func TestOptInTopNValidators(t *testing.T) {
// Start Test 2: opt in all validators with power >= 1
// We expect the same `expectedOptedInValidators` as when we opted in all validators with power >= 0 because the
// validators with the smallest power have power == 1
providerKeeper.OptInTopNValidators(ctx, CONSUMER_ID, []stakingtypes.Validator{valA, valB, valC, valD}, 0)
err = providerKeeper.OptInTopNValidators(ctx, CONSUMER_ID, []stakingtypes.Validator{valA, valB, valC, valD}, 0)
require.NoError(t, err)
actualOptedInValidators = providerKeeper.GetAllOptedIn(ctx, CONSUMER_ID)
sortUpdates(actualOptedInValidators)
require.Equal(t, expectedOptedInValidators, actualOptedInValidators)
Expand All @@ -227,7 +229,8 @@ func TestOptInTopNValidators(t *testing.T) {
providerKeeper.DeleteOptedIn(ctx, CONSUMER_ID, providertypes.NewProviderConsAddress(valDConsAddr))

// Start Test 3: opt in all validators with power >= 2 and hence we do not expect to opt in validator A
providerKeeper.OptInTopNValidators(ctx, CONSUMER_ID, []stakingtypes.Validator{valA, valB, valC, valD}, 2)
err = providerKeeper.OptInTopNValidators(ctx, CONSUMER_ID, []stakingtypes.Validator{valA, valB, valC, valD}, 2)
require.NoError(t, err)
expectedOptedInValidators = []providertypes.ProviderConsAddress{
providertypes.NewProviderConsAddress(valBConsAddr),
providertypes.NewProviderConsAddress(valCConsAddr),
Expand All @@ -246,7 +249,8 @@ func TestOptInTopNValidators(t *testing.T) {
providerKeeper.DeleteOptedIn(ctx, CONSUMER_ID, providertypes.NewProviderConsAddress(valDConsAddr))

// Start Test 4: opt in all validators with power >= 4 and hence we do not expect any opted-in validators
providerKeeper.OptInTopNValidators(ctx, CONSUMER_ID, []stakingtypes.Validator{valA, valB, valC, valD}, 4)
err = providerKeeper.OptInTopNValidators(ctx, CONSUMER_ID, []stakingtypes.Validator{valA, valB, valC, valD}, 4)
require.NoError(t, err)
require.Empty(t, providerKeeper.GetAllOptedIn(ctx, CONSUMER_ID))
}

Expand Down
5 changes: 4 additions & 1 deletion x/ccv/provider/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,10 @@ func (k Keeper) QueueVSCPackets(ctx sdk.Context) error {
// set the minimal power of validators in the top N in the store
k.SetMinimumPowerInTopN(ctx, consumerId, minPower)

k.OptInTopNValidators(ctx, consumerId, activeValidators, minPower)
err := k.OptInTopNValidators(ctx, consumerId, activeValidators, minPower)
if err != nil {
return fmt.Errorf("opting in topN validators, consumerId(%s), minPower(%d): %w", consumerId, minPower, err)
}
}

nextValidators, err := k.ComputeNextValidators(ctx, consumerId, bondedValidators, powerShapingParameters, minPower)
Expand Down
Loading