From 74257ca33e1becbf89cb3015dcc03a8380aa4b10 Mon Sep 17 00:00:00 2001 From: Marius Poke Date: Thu, 12 Sep 2024 10:04:15 +0200 Subject: [PATCH] fix!: make OptInTopNValidators return an error (#2259) make OptInTopNValidators return an error --- x/ccv/provider/keeper/consumer_lifecycle.go | 8 +++- x/ccv/provider/keeper/partial_set_security.go | 37 +++++++++++-------- .../keeper/partial_set_security_test.go | 12 ++++-- x/ccv/provider/keeper/relay.go | 5 ++- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/x/ccv/provider/keeper/consumer_lifecycle.go b/x/ccv/provider/keeper/consumer_lifecycle.go index a93d98efa0..9c9e14f59b 100644 --- a/x/ccv/provider/keeper/consumer_lifecycle.go +++ b/x/ccv/provider/keeper/consumer_lifecycle.go @@ -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) + + err = k.OptInTopNValidators(ctx, consumerId, activeValidators, minPower) + if err != nil { + 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 diff --git a/x/ccv/provider/keeper/partial_set_security.go b/x/ccv/provider/keeper/partial_set_security.go index f96a47ceee..d2a4c1df46 100644 --- a/x/ccv/provider/keeper/partial_set_security.go +++ b/x/ccv/provider/keeper/partial_set_security.go @@ -1,6 +1,8 @@ package keeper import ( + "fmt" + errorsmod "cosmossdk.io/errors" storetypes "cosmossdk.io/store/types" @@ -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 } // diff --git a/x/ccv/provider/keeper/partial_set_security_test.go b/x/ccv/provider/keeper/partial_set_security_test.go index 878e72a642..fc1ab73683 100644 --- a/x/ccv/provider/keeper/partial_set_security_test.go +++ b/x/ccv/provider/keeper/partial_set_security_test.go @@ -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), @@ -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) @@ -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), @@ -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)) } diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 2a956d2f8f..cf8a3de7cf 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -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)