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.

Sign up for GitHub

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

Jump to bottom

feat: refactor consumer validator set computation #2175

Merged
merged 5 commits into from
Aug 27, 2024
Merged

feat: refactor consumer validator set computation #2175

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c07c99e
add UT
sainoe Aug 27, 2024
8499f3c
nits
sainoe Aug 27, 2024
687a877
address comments
sainoe Aug 27, 2024
7e1a647
Update x/ccv/provider/keeper/partial_set_security.go
sainoe Aug 27, 2024
013de32
fix tests
sainoe Aug 27, 2024
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
32 changes: 15 additions & 17 deletions x/ccv/provider/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,29 +437,27 @@ func (k Keeper) hasToValidate(
if err != nil {
return false, nil
}

minPowerToOptIn := int64(0)
// If the consumer is TopN compute the minimum power
if topN := k.GetTopN(ctx, consumerId); topN > 0 {
// in a Top-N chain, we automatically opt in all validators that belong to the top N
minPower, found := k.GetMinimumPowerInTopN(ctx, consumerId)
if found {
k.OptInTopNValidators(ctx, consumerId, activeValidators, minPower)
} else {
k.Logger(ctx).Error("did not find min power in top N for chain", "chain", consumerId)
minPowerToOptIn, err = k.ComputeMinPowerInTopN(ctx, activeValidators, topN)
insumity marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't sufficient to try to get the minimum power from the store first and if you cannot find it, then compute it? Can the minimum power in the store be stale?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this is needed as the minimum power stored is indeed stale. The one of the new epoch will be computed at the end of the epoch. Could you add a comment above ComputeMinPowerInTopN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

if err != nil {
return false, err
}
}

// if the validator is opted in and belongs to the validators of the next epoch, then if nothing changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove the if the validator is opted in from the comment since we do not check k.IsOptedIn at this stage anymore.

// the validator would have to validate in the next epoch
if k.IsOptedIn(ctx, consumerId, provAddr) {
lastVals, err := k.GetLastBondedValidators(ctx)
if err != nil {
return false, err
}
nextValidators := k.ComputeNextValidators(ctx, consumerId, lastVals)
for _, v := range nextValidators {
consAddr := sdk.ConsAddress(v.ProviderConsAddr)
if provAddr.ToSdkConsAddr().Equals(consAddr) {
return true, nil
}
lastVals, err := k.GetLastBondedValidators(ctx)
if err != nil {
return false, err
}
nextValidators := k.ComputeNextValidators(ctx, consumerId, lastVals, minPowerToOptIn)
for _, v := range nextValidators {
consAddr := sdk.ConsAddress(v.ProviderConsAddr)
if provAddr.ToSdkConsAddr().Equals(consAddr) {
return true, nil
}
}

Expand Down
42 changes: 37 additions & 5 deletions x/ccv/provider/keeper/partial_set_security.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ func (k Keeper) OptInTopNValidators(ctx sdk.Context, consumerId string, bondedVa

// if validator already exists it gets overwritten
k.SetOptedIn(ctx, consumerId, types.NewProviderConsAddress(consAddr))
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
}
}
Expand Down Expand Up @@ -322,9 +321,17 @@ func NoMoreThanPercentOfTheSum(validators []types.ConsensusValidator, percent ui

// CanValidateChain returns true if the validator `providerAddr` is opted-in to chain with `consumerId` and the allowlist
// and denylist do not prevent the validator from validating the chain.
func (k Keeper) CanValidateChain(ctx sdk.Context, consumerId string, providerAddr types.ProviderConsAddress) bool {
func (k Keeper) CanValidateChain(ctx sdk.Context, consumerId string, providerAddr types.ProviderConsAddress, minPowerToOptIn int64) bool {
// check if the validator is already opted-in
optedIn := k.IsOptedIn(ctx, consumerId, providerAddr)

// check if the validator is automatically opt-in for a topN chain
sainoe marked this conversation as resolved.
Show resolved Hide resolved
if !optedIn && k.GetTopN(ctx, consumerId) > 0 {
optedIn = k.HasMinPower(ctx, providerAddr, uint64(minPowerToOptIn))
}

// only consider opted-in validators
return k.IsOptedIn(ctx, consumerId, providerAddr) &&
return optedIn &&
// if an allowlist is declared, only consider allowlisted validators
(k.IsAllowlistEmpty(ctx, consumerId) ||
k.IsAllowlisted(ctx, consumerId, providerAddr)) &&
Expand Down Expand Up @@ -352,7 +359,7 @@ func (k Keeper) FulfillsMinStake(ctx sdk.Context, consumerId string, providerAdd
}

// ComputeNextValidators computes the validators for the upcoming epoch based on the currently `bondedValidators`.
func (k Keeper) ComputeNextValidators(ctx sdk.Context, consumerId string, bondedValidators []stakingtypes.Validator) []types.ConsensusValidator {
func (k Keeper) ComputeNextValidators(ctx sdk.Context, consumerId string, bondedValidators []stakingtypes.Validator, minPowerToOptIn int64) []types.ConsensusValidator {
// sort the bonded validators by number of staked tokens in descending order
sort.Slice(bondedValidators, func(i, j int) bool {
return bondedValidators[i].GetBondedTokens().GT(bondedValidators[j].GetBondedTokens())
Expand All @@ -371,9 +378,34 @@ func (k Keeper) ComputeNextValidators(ctx sdk.Context, consumerId string, bonded

nextValidators := k.FilterValidators(ctx, consumerId, bondedValidators,
func(providerAddr types.ProviderConsAddress) bool {
return k.CanValidateChain(ctx, consumerId, providerAddr) && k.FulfillsMinStake(ctx, consumerId, providerAddr)
return k.CanValidateChain(ctx, consumerId, providerAddr, minPowerToOptIn) && k.FulfillsMinStake(ctx, consumerId, providerAddr)
})

nextValidators = k.CapValidatorSet(ctx, consumerId, nextValidators)
return k.CapValidatorsPower(ctx, consumerId, nextValidators)
}

// HasMinPower returns true if the `providerAddr` voting power is GTE than the given minimum power
func (k Keeper) HasMinPower(ctx sdk.Context, providerAddr types.ProviderConsAddress, minPower uint64) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Change minPower uint64 to minPower int64 to avoid unnecessary type casts.

val, err := k.stakingKeeper.GetValidatorByConsAddr(ctx, providerAddr.Address)
if err != nil {
k.Logger(ctx).Error("cannot get last validator power %s: %s", providerAddr, err)
sainoe marked this conversation as resolved.
Show resolved Hide resolved
return false
}

valAddr, err := sdk.ValAddressFromBech32(val.GetOperator())
if err != nil {
k.Logger(ctx).Error("could not retrieve validator address %s: %s",
val.GetOperator(), err)
return false
}

power, err := k.stakingKeeper.GetLastValidatorPower(ctx, valAddr)
if err != nil {
k.Logger(ctx).Error("could not retrieve last power of validator address: %s: %s",
val.GetOperator(), err)
return false
}

return uint64(power) >= minPower
}
113 changes: 100 additions & 13 deletions x/ccv/provider/keeper/partial_set_security_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package keeper_test

import (
"bytes"
"fmt"
"sort"
"testing"

Expand Down Expand Up @@ -384,29 +385,45 @@ func TestCanValidateChain(t *testing.T) {
providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
defer ctrl.Finish()

validator := createStakingValidator(ctx, mocks, 0, 1, 1)
consumerID := "0"

validator := createStakingValidator(ctx, mocks, 0, 1, 1) // adds GetLastValidatorPower expectation to mocks
consAddr, _ := validator.GetConsAddr()
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.CanValidateChain(ctx, "consumerId", providerAddr))
providerKeeper.SetOptedIn(ctx, "consumerId", types.NewProviderConsAddress(consAddr))
require.True(t, providerKeeper.CanValidateChain(ctx, "consumerId", providerAddr))
require.False(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 0))

// with TopN chains, the validator can be considered,
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(gomock.Any(), providerAddr.Address).Return(validator, nil).Times(2)
providerKeeper.SetConsumerPowerShapingParameters(ctx, consumerID, types.PowerShapingParameters{Top_N: 50})
// validator's power is LT the min power
require.False(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 2))
// validator's power is GTE the min power
require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 1))

// when validator is opted-in it can validate regardless of its min power
providerKeeper.SetOptedIn(ctx, consumerID, types.NewProviderConsAddress(consAddr))
require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 0))
Copy link
Contributor

@insumity insumity Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't the power be 2 here instead of 0 because the validator has a power of 1, we test that indeed the fact that the validator is opted-in, makes CanValidateChain return true?
Also below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Updated!


// With OptIn chains, validator can validate only if it has already opted-in
providerKeeper.SetConsumerPowerShapingParameters(ctx, consumerID, types.PowerShapingParameters{Top_N: 0})
require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 0))

// create an allow list but do not add the validator `providerAddr` to it
validatorA := createStakingValidator(ctx, mocks, 1, 1, 2)
consAddrA, _ := validatorA.GetConsAddr()
providerKeeper.SetAllowlist(ctx, "consumerId", types.NewProviderConsAddress(consAddrA))
require.False(t, providerKeeper.CanValidateChain(ctx, "consumerId", providerAddr))
providerKeeper.SetAllowlist(ctx, "consumerId", types.NewProviderConsAddress(consAddr))
require.True(t, providerKeeper.CanValidateChain(ctx, "consumerId", providerAddr))
providerKeeper.SetAllowlist(ctx, consumerID, types.NewProviderConsAddress(consAddrA))
require.False(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 0))
providerKeeper.SetAllowlist(ctx, consumerID, types.NewProviderConsAddress(consAddr))
require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 0))

// create a denylist but do not add validator `providerAddr` to it
providerKeeper.SetDenylist(ctx, "consumerId", types.NewProviderConsAddress(consAddrA))
require.True(t, providerKeeper.CanValidateChain(ctx, "consumerId", providerAddr))
providerKeeper.SetDenylist(ctx, consumerID, types.NewProviderConsAddress(consAddrA))
require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 0))
// add validator `providerAddr` to the denylist
providerKeeper.SetDenylist(ctx, "consumerId", types.NewProviderConsAddress(consAddr))
require.False(t, providerKeeper.CanValidateChain(ctx, "consumerId", providerAddr))
providerKeeper.SetDenylist(ctx, consumerID, types.NewProviderConsAddress(consAddr))
require.False(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 0))
}

func TestCapValidatorSet(t *testing.T) {
Expand Down Expand Up @@ -820,7 +837,7 @@ func TestIfInactiveValsDisallowedProperty(t *testing.T) {
providerKeeper.SetParams(ctx, params)

// Compute the next validators
nextVals := providerKeeper.ComputeNextValidators(ctx, "consumerId", vals)
nextVals := providerKeeper.ComputeNextValidators(ctx, "consumerId", vals, 0)

// Check that the length of nextVals is at most maxProviderConsensusVals
require.LessOrEqual(r, len(nextVals), int(maxProviderConsensusVals), "The length of nextVals should be at most maxProviderConsensusVals")
Expand All @@ -843,3 +860,73 @@ func TestIfInactiveValsDisallowedProperty(t *testing.T) {
}
})
}

func TestHasMinPower(t *testing.T) {
pk, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
defer ctrl.Finish()

providerConsPubKey := ed25519.GenPrivKeyFromSecret([]byte{1}).PubKey()
consAddr := sdk.ConsAddress(providerConsPubKey.Address())
providerAddr := types.NewProviderConsAddress(consAddr)

testCases := []struct {
name string
minPower uint64
expectation func(sdk.ConsAddress, testkeeper.MockedKeepers)
hasMinPower bool
}{
{
name: "cannot find validator by cons address",
expectation: func(sdk.ConsAddress, testkeeper.MockedKeepers) {
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(gomock.Any(), consAddr).
Return(stakingtypes.Validator{}, fmt.Errorf("validator not found")).Times(1)
},
hasMinPower: false,
}, {
name: "cannot convert validator operator address",
expectation: func(consAddr sdk.ConsAddress, mocks testkeeper.MockedKeepers) {
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(gomock.Any(), consAddr).Return(stakingtypes.Validator{OperatorAddress: "xxxx"}, nil).Times(1)
},
hasMinPower: false,
}, {
name: "cannot find last validator power",
expectation: func(consAddr sdk.ConsAddress, mocks testkeeper.MockedKeepers) {
valAddr := sdk.ValAddress(providerAddr.Address.Bytes())
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(gomock.Any(), consAddr).
Return(stakingtypes.Validator{OperatorAddress: valAddr.String()}, nil)
mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), valAddr).
Return(int64(0), fmt.Errorf("last power not found")).Times(1)
},
hasMinPower: false,
}, {
name: "validator power is LT min power",
expectation: func(consAddr sdk.ConsAddress, mocks testkeeper.MockedKeepers) {
valAddr := sdk.ValAddress(providerAddr.Address.Bytes())
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(gomock.Any(), consAddr).
Return(stakingtypes.Validator{OperatorAddress: valAddr.String()}, nil)
mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), valAddr).
Return(int64(5), nil).Times(1)
},
hasMinPower: false,
}, {
name: "validator power is GTE min power",
expectation: func(consAddr sdk.ConsAddress, mocks testkeeper.MockedKeepers) {
valAddr := sdk.ValAddress(providerAddr.Address.Bytes())
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(gomock.Any(), consAddr).
Return(stakingtypes.Validator{OperatorAddress: valAddr.String()}, nil)
mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), valAddr).
Return(int64(10), nil).Times(1)
},
hasMinPower: true,
},
}

minPower := uint64(10)

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
tc.expectation(consAddr, mocks)
require.Equal(t, tc.hasMinPower, pk.HasMinPower(ctx, providerAddr, minPower))
})
}
}
6 changes: 4 additions & 2 deletions x/ccv/provider/keeper/proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ func (k Keeper) MakeConsumerGenesis(
return gen, nil, errorsmod.Wrapf(stakingtypes.ErrNoValidatorFound, "error getting last bonded validators: %s", err)
}

minPower := int64(0)
if powerShapingParameters.Top_N > 0 {
// get the consensus active validators
// we do not want to base the power calculation for the top N
Expand All @@ -243,7 +244,7 @@ func (k Keeper) MakeConsumerGenesis(
}

// in a Top-N chain, we automatically opt in all validators that belong to the top N
minPower, err := k.ComputeMinPowerInTopN(ctx, activeValidators, powerShapingParameters.Top_N)
minPower, err = k.ComputeMinPowerInTopN(ctx, activeValidators, powerShapingParameters.Top_N)
if err != nil {
return gen, nil, err
}
Expand All @@ -255,8 +256,9 @@ func (k Keeper) MakeConsumerGenesis(
k.OptInTopNValidators(ctx, consumerId, activeValidators, minPower)
k.SetMinimumPowerInTopN(ctx, consumerId, minPower)
}

// need to use the bondedValidators, not activeValidators, here since the chain might be opt-in and allow inactive vals
nextValidators := k.ComputeNextValidators(ctx, consumerId, bondedValidators)
nextValidators := k.ComputeNextValidators(ctx, consumerId, bondedValidators, minPower)
k.SetConsumerValSet(ctx, consumerId, nextValidators)

// get the initial updates with the latest set consumer public keys
Expand Down
5 changes: 3 additions & 2 deletions x/ccv/provider/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ func (k Keeper) QueueVSCPackets(ctx sdk.Context) {
}
topN := k.GetTopN(ctx, consumerId)

minPower := int64(0)
if topN > 0 {
// in a Top-N chain, we automatically opt in all validators that belong to the top N
// of the active validators
Expand All @@ -208,7 +209,7 @@ func (k Keeper) QueueVSCPackets(ctx sdk.Context) {
panic(fmt.Errorf("failed to get active validators: %w", err))
}

minPower, err := k.ComputeMinPowerInTopN(ctx, activeValidators, topN)
minPower, err = k.ComputeMinPowerInTopN(ctx, activeValidators, topN)
if err != nil {
// we panic, since the only way to proceed would be to opt in all validators, which is not the intended behavior
panic(fmt.Errorf("failed to compute min power to opt in for chain %v: %w", consumerId, err))
Expand All @@ -220,7 +221,7 @@ func (k Keeper) QueueVSCPackets(ctx sdk.Context) {
k.OptInTopNValidators(ctx, consumerId, activeValidators, minPower)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to opt in validators here for computing the next validators, right? However, we want to automatically opt in to TopN to not change the logic. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly. It can be removed but it will require more refactoring. I will create an issue.

}

nextValidators := k.ComputeNextValidators(ctx, consumerId, bondedValidators)
nextValidators := k.ComputeNextValidators(ctx, consumerId, bondedValidators, minPower)

valUpdates := DiffValidators(currentValidators, nextValidators)
k.SetConsumerValSet(ctx, consumerId, nextValidators)
Expand Down
Loading