From 3d1d2d787a04b4e177d1bd57ed0591eb5bb12d35 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Fri, 23 Aug 2024 12:16:59 +0200 Subject: [PATCH 1/5] Fix bug and add test case --- tests/e2e/config.go | 160 ++++++++++++++++-- tests/e2e/main.go | 6 + tests/e2e/steps.go | 138 +++++++++++++++ x/ccv/provider/keeper/partial_set_security.go | 17 +- x/ccv/provider/keeper/proposal.go | 20 +++ x/ccv/types/utils.go | 37 +--- 6 files changed, 335 insertions(+), 43 deletions(-) diff --git a/tests/e2e/config.go b/tests/e2e/config.go index c8d4342560..b1bffb9a6f 100644 --- a/tests/e2e/config.go +++ b/tests/e2e/config.go @@ -84,20 +84,21 @@ type ( type TestConfigType string const ( - DefaultTestCfg TestConfigType = "default" - ChangeoverTestCfg TestConfigType = "changeover" - DemocracyTestCfg TestConfigType = "democracy" - DemocracyRewardTestCfg TestConfigType = "democracy-reward" - SlashThrottleTestCfg TestConfigType = "slash-throttling" - MulticonsumerTestCfg TestConfigType = "multi-consumer" - ConsumerMisbehaviourTestCfg TestConfigType = "consumer-misbehaviour" - CompatibilityTestCfg TestConfigType = "compatibility" - SmallMaxValidatorsTestCfg TestConfigType = "small-max-validators" - InactiveProviderValsTestCfg TestConfigType = "inactive-provider-vals" - GovTestCfg TestConfigType = "gov" - InactiveValsGovTestCfg TestConfigType = "inactive-vals-gov" - InactiveValsMintTestCfg TestConfigType = "inactive-vals-mint" - MintTestCfg TestConfigType = "mint" + DefaultTestCfg TestConfigType = "default" + ChangeoverTestCfg TestConfigType = "changeover" + DemocracyTestCfg TestConfigType = "democracy" + DemocracyRewardTestCfg TestConfigType = "democracy-reward" + SlashThrottleTestCfg TestConfigType = "slash-throttling" + MulticonsumerTestCfg TestConfigType = "multi-consumer" + ConsumerMisbehaviourTestCfg TestConfigType = "consumer-misbehaviour" + CompatibilityTestCfg TestConfigType = "compatibility" + SmallMaxValidatorsTestCfg TestConfigType = "small-max-validators" + InactiveProviderValsTestCfg TestConfigType = "inactive-provider-vals" + GovTestCfg TestConfigType = "gov" + InactiveValsGovTestCfg TestConfigType = "inactive-vals-gov" + InactiveValsMintTestCfg TestConfigType = "inactive-vals-mint" + MintTestCfg TestConfigType = "mint" + InactiveValsExtraValsTestCfg TestConfigType = "inactive-vals-extra-vals" ) type TestConfig struct { @@ -205,6 +206,8 @@ func GetTestConfig(cfgType TestConfigType, providerVersion, consumerVersion stri testCfg = InactiveValsMintTestConfig() case MintTestCfg: testCfg = MintTestConfig() + case InactiveValsExtraValsTestCfg: + testCfg = InactiveValsExtraValsTestConfig() default: panic(fmt.Sprintf("Invalid test config: %s", cfgType)) } @@ -610,6 +613,25 @@ func InactiveProviderValsTestConfig() TestConfig { return tr } +func InactiveValsExtraValsTestConfig() TestConfig { + tr := InactiveProviderValsTestConfig() + + // set the MaxProviderConsensusValidators param to 4 + proviConfig := tr.chainConfigs[ChainID("provi")] + proviConfig.GenesisChanges += " | .app_state.provider.params.max_provider_consensus_validators = \"4\"" + // set max validators to 5 + proviConfig.GenesisChanges += " | .app_state.staking.params.max_validators = \"5\"" + tr.chainConfigs[ChainID("provi")] = proviConfig + + // add the extra validators to the validator config + extraVals := GetExtraValidatorConfigs() + for valId, val := range extraVals { + tr.validatorConfigs[valId] = val + } + + return tr +} + func SmallMaxValidatorsTestConfig() TestConfig { cfg := DefaultTestConfig() @@ -1274,3 +1296,113 @@ func GetHermesConfig(hermesVersion, queryNodeIP string, chainCfg ChainConfig, is } return hermesConfig } + +func GetExtraValidatorConfigs() map[ValidatorID]ValidatorConfig { + return map[ValidatorID]ValidatorConfig{ + ValidatorID("david"): { + Mnemonic: "save arm pill nothing riot park analyst fever couple use reform hotel involve captain ride spell cricket spoil admit proud file renew below add", + DelAddress: "cosmos1jv9j37zakskecthedez2xuvkd7aj4v96u6wm57", + // DelAddressOnConsumer: "consumer1dkas8mu4kyhl5jrh4nzvm65qz588hy9qahzgv6", + ValoperAddress: "cosmosvaloper1jv9j37zakskecthedez2xuvkd7aj4v96ew6wcd", + // ValoperAddressOnConsumer: "consumervaloper1dkas8mu4kyhl5jrh4nzvm65qz588hy9qj0phzw", + ValconsAddress: "cosmosvalcons1vde2tkme0336durkp7qlehyw2v0r2rgm5lfcw5", + ValconsAddressOnConsumer: "consumervalcons1vde2tkme0336durkp7qlehyw2v0r2rgmmxnal5", + PrivValidatorKey: `{ + "address": "6372A5DB797C63A6F0760F81FCDC8E531E350D1B", + "pub_key": { + "type": "tendermint/PubKeyEd25519", + "value": "JFt8aKr1AnubC23rVEUza0pl3DQC0VdC6jUlnkjCh5o=" + }, + "priv_key": { + "type": "tendermint/PrivKeyEd25519", + "value": "mBErI1aFTt2VHNiWkb14mEfSIfUU6PHndJCKaJ2XjkwkW3xoqvUCe5sLbetURTNrSmXcNALRV0LqNSWeSMKHmg==" + } + }`, + NodeKey: `{"priv_key":{"type":"tendermint/PrivKeyEd25519","value":"LJQ/VDAYtEEaJC3NA32fZ2oLLm9akLeLVnBlJ2WOYEMLohbwfac0x42Qh23E/ByEMliSjfGvLFQZIYzRqwJcLA=="}}`, + IpSuffix: "7", + + // // consumer chain assigned key + // ConsumerMnemonic: "grunt list hour endless observe better spoil penalty lab duck only layer vague fantasy satoshi record demise topple space shaft solar practice donor sphere", + // ConsumerDelAddress: "consumer1q90l6j6lzzgt460ehjj56azknlt5yrd44y2uke", + // ConsumerDelAddressOnProvider: "cosmos1q90l6j6lzzgt460ehjj56azknlt5yrd4s38n97", + // ConsumerValoperAddress: "consumervaloper1q90l6j6lzzgt460ehjj56azknlt5yrd46ufrcd", + // ConsumerValoperAddressOnProvider: "cosmosvaloper1q90l6j6lzzgt460ehjj56azknlt5yrd449nxfd", + // ConsumerValconsAddress: "consumervalcons1uuec3cjxajv5te08p220usrjhkfhg9wyref26m", + // ConsumerValconsAddressOnProvider: "cosmosvalcons1uuec3cjxajv5te08p220usrjhkfhg9wyvqn0tm", + // ConsumerValPubKey: `{"@type":"/cosmos.crypto.ed25519.PubKey","key":"QlG+iYe6AyYpvY1z9RNJKCVlH14Q/qSz4EjGdGCru3o="}`, + // ConsumerPrivValidatorKey: `{"address":"E73388E246EC9945E5E70A94FE4072BD937415C4","pub_key":{"type":"tendermint/PubKeyEd25519","value":"QlG+iYe6AyYpvY1z9RNJKCVlH14Q/qSz4EjGdGCru3o="},"priv_key":{"type":"tendermint/PrivKeyEd25519","value":"OFR4w+FC6EMw5fAGTrHVexyPrjzQ7QfqgZOMgVf0izlCUb6Jh7oDJim9jXP1E0koJWUfXhD+pLPgSMZ0YKu7eg=="}}`, + // ConsumerNodeKey: `{"priv_key":{"type":"tendermint/PrivKeyEd25519","value":"uhPCqnL2KE8m/8OFNLQ5bN3CJr6mds+xfBi0E4umT/s2uWiJhet+vbYx88DHSdof3gGFNTIzAIxSppscBKX96w=="}}`, + // UseConsumerKey: false, + }, + ValidatorID("eve"): { + Mnemonic: "board lava muffin daughter frozen chimney chest whale give subject inquiry forward alter gasp busy flee wire ecology invite code comic cloth shoot pen", + DelAddress: "cosmos1p56m290cgys4396e3f8p4kj9lrzwak7zemg45t", + // DelAddressOnConsumer: "consumer1dkas8mu4kyhl5jrh4nzvm65qz588hy9qahzgv6", + ValoperAddress: "cosmosvaloper1p56m290cgys4396e3f8p4kj9lrzwak7zu0uqcc", + // ValoperAddressOnConsumer: "consumervaloper1dkas8mu4kyhl5jrh4nzvm65qz588hy9qj0phzw", + ValconsAddress: "cosmosvalcons1dqvy6lz440hj4zxjske5knsyx60ac5estqx6k2", + ValconsAddressOnConsumer: "consumervalcons1dqvy6lz440hj4zxjske5knsyx60ac5esyeul82", + PrivValidatorKey: `{ + "address": "68184D7C55ABEF2A88D285B34B4E04369FDC5330", + "pub_key": { + "type": "tendermint/PubKeyEd25519", + "value": "QbLLxm/mNHfS9WWXTxvt30D2xeC7/HRrMrZJIVOtj9s=" + }, + "priv_key": { + "type": "tendermint/PrivKeyEd25519", + "value": "LDPp4B9/Q18yZBJv2zXMnCA+NB9wvaM3XAkWLuCvbaFBssvGb+Y0d9L1ZZdPG+3fQPbF4Lv8dGsytkkhU62P2w==" + } + }`, + NodeKey: `{"priv_key":{"type":"tendermint/PrivKeyEd25519","value":"eyDlEXWWP5KwD2IKZeJ/bvf8jT+pVsXYVjpV2HfP+GEjngJe2dNbuuNqtC6L7SFcp5/W2aOIKsslASqv+Oai9Q=="}}`, + IpSuffix: "8", + + // // consumer chain assigned key + // ConsumerMnemonic: "grunt list hour endless observe better spoil penalty lab duck only layer vague fantasy satoshi record demise topple space shaft solar practice donor sphere", + // ConsumerDelAddress: "consumer1q90l6j6lzzgt460ehjj56azknlt5yrd44y2uke", + // ConsumerDelAddressOnProvider: "cosmos1q90l6j6lzzgt460ehjj56azknlt5yrd4s38n97", + // ConsumerValoperAddress: "consumervaloper1q90l6j6lzzgt460ehjj56azknlt5yrd46ufrcd", + // ConsumerValoperAddressOnProvider: "cosmosvaloper1q90l6j6lzzgt460ehjj56azknlt5yrd449nxfd", + // ConsumerValconsAddress: "consumervalcons1uuec3cjxajv5te08p220usrjhkfhg9wyref26m", + // ConsumerValconsAddressOnProvider: "cosmosvalcons1uuec3cjxajv5te08p220usrjhkfhg9wyvqn0tm", + // ConsumerValPubKey: `{"@type":"/cosmos.crypto.ed25519.PubKey","key":"QlG+iYe6AyYpvY1z9RNJKCVlH14Q/qSz4EjGdGCru3o="}`, + // ConsumerPrivValidatorKey: `{"address":"E73388E246EC9945E5E70A94FE4072BD937415C4","pub_key":{"type":"tendermint/PubKeyEd25519","value":"QlG+iYe6AyYpvY1z9RNJKCVlH14Q/qSz4EjGdGCru3o="},"priv_key":{"type":"tendermint/PrivKeyEd25519","value":"OFR4w+FC6EMw5fAGTrHVexyPrjzQ7QfqgZOMgVf0izlCUb6Jh7oDJim9jXP1E0koJWUfXhD+pLPgSMZ0YKu7eg=="}}`, + // ConsumerNodeKey: `{"priv_key":{"type":"tendermint/PrivKeyEd25519","value":"uhPCqnL2KE8m/8OFNLQ5bN3CJr6mds+xfBi0E4umT/s2uWiJhet+vbYx88DHSdof3gGFNTIzAIxSppscBKX96w=="}}`, + // UseConsumerKey: false, + }, + ValidatorID("fred"): { + Mnemonic: "squeeze runway ivory cause throw diagram camp cricket clutch lens venture panel explain transfer dove notice nest twist plate van paddle rude summer give", + DelAddress: "cosmos13s90cyesdm2292pn8mnzmjm0ez3nd7jaw32tdq", + // // DelAddressOnConsumer: "consumer1dkas8mu4kyhl5jrh4nzvm65qz588hy9qahzgv6", + ValoperAddress: "cosmosvaloper13s90cyesdm2292pn8mnzmjm0ez3nd7jat977pn", + // // ValoperAddressOnConsumer: "consumervaloper1dkas8mu4kyhl5jrh4nzvm65qz588hy9qj0phzw", + ValconsAddress: "cosmosvalcons1xvuktnaz3rvwmldw7ktv7lcn2xf7l252wmsv5e", + ValconsAddressOnConsumer: "consumervalcons1xvuktnaz3rvwmldw7ktv7lcn2xf7l252pz2f9e", + PrivValidatorKey: `{ + "address": "333965CFA288D8EDFDAEF596CF7F135193EFAA8A", + "pub_key": { + "type": "tendermint/PubKeyEd25519", + "value": "nC7g+8/y3NNx7D6Ae970H9954JeqX7SyAxNHh5GnJGs=" + }, + "priv_key": { + "type": "tendermint/PrivKeyEd25519", + "value": "otxstGMSCO0T4CU/Ouxxaam+HUFoL9ArKmMqvSaaCaCcLuD7z/Lc03HsPoB73vQf33ngl6pftLIDE0eHkackaw==" + } + }`, + NodeKey: `{"priv_key":{"type":"tendermint/PrivKeyEd25519","value":"oeJWEaFbLHIgZEbCeVDeYKDqM23fv3j/FobYdhIffQYN2X9MUvBlkhi4Uz6dLQ+vSfZIZb2x2vPcgJsCUpLGnQ=="}}`, + IpSuffix: "9", + + // // consumer chain assigned key + // ConsumerMnemonic: "grunt list hour endless observe better spoil penalty lab duck only layer vague fantasy satoshi record demise topple space shaft solar practice donor sphere", + // ConsumerDelAddress: "consumer1q90l6j6lzzgt460ehjj56azknlt5yrd44y2uke", + // ConsumerDelAddressOnProvider: "cosmos1q90l6j6lzzgt460ehjj56azknlt5yrd4s38n97", + // ConsumerValoperAddress: "consumervaloper1q90l6j6lzzgt460ehjj56azknlt5yrd46ufrcd", + // ConsumerValoperAddressOnProvider: "cosmosvaloper1q90l6j6lzzgt460ehjj56azknlt5yrd449nxfd", + // ConsumerValconsAddress: "consumervalcons1uuec3cjxajv5te08p220usrjhkfhg9wyref26m", + // ConsumerValconsAddressOnProvider: "cosmosvalcons1uuec3cjxajv5te08p220usrjhkfhg9wyvqn0tm", + // ConsumerValPubKey: `{"@type":"/cosmos.crypto.ed25519.PubKey","key":"QlG+iYe6AyYpvY1z9RNJKCVlH14Q/qSz4EjGdGCru3o="}`, + // ConsumerPrivValidatorKey: `{"address":"E73388E246EC9945E5E70A94FE4072BD937415C4","pub_key":{"type":"tendermint/PubKeyEd25519","value":"QlG+iYe6AyYpvY1z9RNJKCVlH14Q/qSz4EjGdGCru3o="},"priv_key":{"type":"tendermint/PrivKeyEd25519","value":"OFR4w+FC6EMw5fAGTrHVexyPrjzQ7QfqgZOMgVf0izlCUb6Jh7oDJim9jXP1E0koJWUfXhD+pLPgSMZ0YKu7eg=="}}`, + // ConsumerNodeKey: `{"priv_key":{"type":"tendermint/PrivKeyEd25519","value":"uhPCqnL2KE8m/8OFNLQ5bN3CJr6mds+xfBi0E4umT/s2uWiJhet+vbYx88DHSdof3gGFNTIzAIxSppscBKX96w=="}}`, + // UseConsumerKey: false, + }, + } +} diff --git a/tests/e2e/main.go b/tests/e2e/main.go index 5b364e1fc3..2864c9b413 100644 --- a/tests/e2e/main.go +++ b/tests/e2e/main.go @@ -246,6 +246,12 @@ var stepChoices = map[string]StepChoice{ description: "test minting without inactive validators as a sanity check", testConfig: MintTestCfg, }, + "inactive-vals-outside-max-validators": { + name: "inactive-vals-outside-max-validators", + steps: stepsInactiveValsTopNReproduce(), + description: "tests the behaviour of inactive validators with a top N = 100 chain and when max_validators is smaller than the total number of validators", + testConfig: InactiveValsExtraValsTestCfg, + }, } func getTestCaseUsageString() string { diff --git a/tests/e2e/steps.go b/tests/e2e/steps.go index 5e16a61776..279b312e7f 100644 --- a/tests/e2e/steps.go +++ b/tests/e2e/steps.go @@ -1,5 +1,12 @@ package main +import ( + "strconv" + + gov "github.com/cosmos/cosmos-sdk/x/gov/types/v1" + clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" +) + type Step struct { Action interface{} State State @@ -138,3 +145,134 @@ var consumerDoubleDowntimeSteps = concatSteps( stepsRedelegate("consu"), stepsDoubleDowntime("consu"), ) + +func stepsInactiveValsTopNReproduce() []Step { + alice_power := uint(30) + bob_power := uint(29) + carol_power := uint(20) + david_power := uint(10) + eve_power := uint(7) + fred_power := uint(4) + + return []Step{ + { + Action: StartChainAction{ + Chain: ChainID("provi"), + Validators: []StartChainValidator{ + {Id: ValidatorID("alice"), Stake: alice_power * 1000000, Allocation: 10000000000}, + {Id: ValidatorID("bob"), Stake: bob_power * 1000000, Allocation: 10000000000}, + {Id: ValidatorID("carol"), Stake: carol_power * 1000000, Allocation: 10000000000}, + {Id: ValidatorID("david"), Stake: david_power * 1000000, Allocation: 10000000000}, + {Id: ValidatorID("eve"), Stake: eve_power * 1000000, Allocation: 10000000000}, + {Id: ValidatorID("fred"), Stake: fred_power * 1000000, Allocation: 10000000000}, + }, + }, + State: State{ + ChainID("provi"): ChainState{ + ValPowers: &map[ValidatorID]uint{ + ValidatorID("alice"): alice_power, + ValidatorID("bob"): bob_power, + ValidatorID("carol"): carol_power, + ValidatorID("david"): david_power, + ValidatorID("eve"): 0, // max provider consensus validators is 4, so eve and fred are at 0 power + ValidatorID("fred"): 0, + }, + StakedTokens: &map[ValidatorID]uint{ + ValidatorID("alice"): alice_power * 1000000, + ValidatorID("bob"): bob_power * 1000000, + ValidatorID("carol"): carol_power * 1000000, + ValidatorID("david"): david_power * 1000000, + ValidatorID("eve"): eve_power * 1000000, + ValidatorID("fred"): fred_power * 1000000, + }, + }, + }, + }, + { + Action: SubmitConsumerAdditionProposalAction{ + Chain: ChainID("provi"), + From: ValidatorID("alice"), + Deposit: 10000001, + ConsumerChain: ChainID("consu"), + SpawnTime: 0, + InitialHeight: clienttypes.Height{RevisionNumber: 0, RevisionHeight: 1}, + TopN: 100, + AllowInactiveVals: false, + }, + State: State{ + ChainID("provi"): ChainState{ + Proposals: &map[uint]Proposal{ + 1: ConsumerAdditionProposal{ + Deposit: 10000001, + Chain: ChainID("consu"), + SpawnTime: 0, + InitialHeight: clienttypes.Height{RevisionNumber: 0, RevisionHeight: 1}, + Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_VOTING_PERIOD)), + }, + }, + }, + }, + }, + { + Action: VoteGovProposalAction{ + Chain: ChainID("provi"), + From: []ValidatorID{ValidatorID("alice"), ValidatorID("bob"), ValidatorID("carol"), ValidatorID("david"), ValidatorID("eve")}, + Vote: []string{"yes", "yes", "yes", "yes", "yes"}, + PropNumber: 1, + }, + State: State{ + ChainID("provi"): ChainState{ + Proposals: &map[uint]Proposal{ + 1: ConsumerAdditionProposal{ + Deposit: 10000001, + Chain: ChainID("consu"), + SpawnTime: 0, + InitialHeight: clienttypes.Height{RevisionNumber: 0, RevisionHeight: 1}, + Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_PASSED)), + }, + }, + HasToValidate: &map[ValidatorID][]ChainID{ + ValidatorID("alice"): {"consu"}, + ValidatorID("bob"): {"consu"}, + ValidatorID("carol"): {"consu"}, + ValidatorID("david"): {"consu"}, + ValidatorID("eve"): {}, + ValidatorID("fred"): {}, + }, + }, + }, + }, + { + Action: StartConsumerChainAction{ + ConsumerChain: ChainID("consu"), + ProviderChain: ChainID("provi"), + Validators: []StartChainValidator{ + {Id: ValidatorID("alice"), Stake: alice_power * 1000000, Allocation: 10000000000}, + {Id: ValidatorID("bob"), Stake: bob_power * 1000000, Allocation: 10000000000}, + {Id: ValidatorID("carol"), Stake: carol_power * 1000000, Allocation: 10000000000}, + {Id: ValidatorID("david"), Stake: david_power * 1000000, Allocation: 10000000000}, + {Id: ValidatorID("eve"), Stake: eve_power * 1000000, Allocation: 10000000000}, + {Id: ValidatorID("fred"), Stake: fred_power * 1000000, Allocation: 10000000000}, + }, + // For consumers that're launching with the provider being on an earlier version + // of ICS before the soft opt-out threshold was introduced, we need to set the + // soft opt-out threshold to 0.05 in the consumer genesis to ensure that the + // consumer binary doesn't panic. Sdk requires that all params are set to valid + // values from the genesis file. + GenesisChanges: ".app_state.ccvconsumer.params.soft_opt_out_threshold = \"0.05\"", + }, + State: State{ + ChainID("consu"): ChainState{ + ValPowers: &map[ValidatorID]uint{ + ValidatorID("alice"): alice_power, + ValidatorID("bob"): bob_power, + ValidatorID("carol"): carol_power, + ValidatorID("david"): david_power, + ValidatorID("eve"): 0, + ValidatorID("fred"): 0, + }, + }, + }, + }, + } +} diff --git a/x/ccv/provider/keeper/partial_set_security.go b/x/ccv/provider/keeper/partial_set_security.go index 23881162c5..f157bee955 100644 --- a/x/ccv/provider/keeper/partial_set_security.go +++ b/x/ccv/provider/keeper/partial_set_security.go @@ -91,6 +91,9 @@ func (k Keeper) HandleOptOut(ctx sdk.Context, chainID string, providerAddr types // OptInTopNValidators opts in to `chainID` all the `bondedValidators` that have at least `minPowerToOptIn` power func (k Keeper) OptInTopNValidators(ctx sdk.Context, chainID string, bondedValidators []stakingtypes.Validator, minPowerToOptIn int64) { for _, val := range bondedValidators { + // log the validator + k.Logger(ctx).Info("Opting in validator", "validator", val.GetOperator()) + valAddr, err := sdk.ValAddressFromBech32(val.GetOperator()) if err != nil { k.Logger(ctx).Error("could not retrieve validator address: %s: %s", @@ -104,6 +107,9 @@ func (k Keeper) OptInTopNValidators(ctx sdk.Context, chainID string, bondedValid continue } if power >= minPowerToOptIn { + k.Logger(ctx).Info("Before getting cons addr, but power is good", + "validator", val.GetOperator(), + "power", power) consAddr, err := val.GetConsAddr() if err != nil { k.Logger(ctx).Error("could not retrieve validators consensus address: %s: %s", @@ -111,9 +117,18 @@ func (k Keeper) OptInTopNValidators(ctx sdk.Context, chainID string, bondedValid continue } + k.Logger(ctx).Info("Opting in validator", "validator", val.GetOperator()) + // if validator already exists it gets overwritten k.SetOptedIn(ctx, chainID, types.NewProviderConsAddress(consAddr)) - } // else validators that do not belong to the top N validators but were opted in, remain opted in + } else { + // else validators that do not belong to the top N validators but were opted in, remain opted in + k.Logger(ctx).Info("After getting cons addr, but power is not good", + "validator", val.GetOperator(), + "power", power, + "minPowerToOptIn", + minPowerToOptIn) + } } } diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index 96d70b48a2..e6dd3810a6 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -275,13 +275,33 @@ func (k Keeper) MakeConsumerGenesis( if err != nil { return gen, nil, errorsmod.Wrapf(stakingtypes.ErrNoValidatorFound, "error getting last active bonded validators: %s", err) } + + // for debugging purposes, log the active validators + for _, val := range activeValidators { + k.Logger(ctx).Info("active validator", + "operator", val.GetOperator(), + "power", val.Tokens, + ) + } + // in a Top-N chain, we automatically opt in all validators that belong to the top N minPower, err := k.ComputeMinPowerInTopN(ctx, activeValidators, prop.Top_N) if err != nil { return gen, nil, err } + // log the minimum power in top N + k.Logger(ctx).Info("minimum power in top N", + "chainID", chainID, + "minPower", minPower, + ) k.OptInTopNValidators(ctx, chainID, activeValidators, minPower) k.SetMinimumPowerInTopN(ctx, chainID, minPower) + + // log the minimum power in top N + k.Logger(ctx).Info("minimum power in top N", + "chainID", chainID, + "minPower", minPower, + ) } // need to use the bondedValidators, not activeValidators, here since the chain might be opt-in and allow inactive vals nextValidators := k.ComputeNextValidators(ctx, chainID, bondedValidators) diff --git a/x/ccv/types/utils.go b/x/ccv/types/utils.go index c4c6af72d0..03e5f63be1 100644 --- a/x/ccv/types/utils.go +++ b/x/ccv/types/utils.go @@ -2,6 +2,7 @@ package types import ( "errors" + fmt "fmt" "reflect" "sort" "strings" @@ -128,38 +129,18 @@ func GetConsAddrFromBech32(bech32str string) (sdk.ConsAddress, error) { // GetLastBondedValidatorsUtil iterates the last validator powers in the staking module // and returns the first maxVals many validators with the largest powers. func GetLastBondedValidatorsUtil(ctx sdk.Context, stakingKeeper StakingKeeper, logger log.Logger, maxVals uint32) ([]stakingtypes.Validator, error) { - lastPowers := make([]stakingtypes.LastValidatorPower, maxVals) - - i := 0 - err := stakingKeeper.IterateLastValidatorPowers(ctx, func(addr sdk.ValAddress, power int64) (stop bool) { - lastPowers[i] = stakingtypes.LastValidatorPower{Address: addr.String(), Power: power} - i++ - return i >= int(maxVals) // stop iteration if true - }) + // get the bonded validators from the staking module, sorted by power + bondedValidators, err := stakingKeeper.GetBondedValidatorsByPower(ctx) if err != nil { - return nil, err + panic(fmt.Errorf("failed to get bonded validators: %w", err)) } - // truncate the lastPowers - lastPowers = lastPowers[:i] - - bondedValidators := make([]stakingtypes.Validator, len(lastPowers)) - - for index, p := range lastPowers { - addr, err := sdk.ValAddressFromBech32(p.Address) - if err != nil { - logger.Error("Invalid validator address", "address", p.Address, "error", err) - continue - } + // get the first maxVals many validators + if uint32(len(bondedValidators)) < maxVals { + return bondedValidators, nil // no need to truncate + } - val, err := stakingKeeper.GetValidator(ctx, addr) - if err != nil { - logger.Error(err.Error(), addr.String()) - continue - } + bondedValidators = bondedValidators[:maxVals] - // gather all the bonded validators in order to construct the consumer validator set for consumer chain `chainID` - bondedValidators[index] = val - } return bondedValidators, nil } From 430379af84cb2c2a82e32d19e5efa907da8c0997 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Fri, 23 Aug 2024 12:27:46 +0200 Subject: [PATCH 2/5] Clean up log messages --- tests/e2e/config.go | 6 ++++++ x/ccv/provider/keeper/partial_set_security.go | 12 +----------- x/ccv/provider/keeper/proposal.go | 16 +--------------- 3 files changed, 8 insertions(+), 26 deletions(-) diff --git a/tests/e2e/config.go b/tests/e2e/config.go index b1bffb9a6f..4fa762b5f7 100644 --- a/tests/e2e/config.go +++ b/tests/e2e/config.go @@ -1297,6 +1297,12 @@ func GetHermesConfig(hermesVersion, queryNodeIP string, chainCfg ChainConfig, is return hermesConfig } +// GetExtraValidatorConfigs returns a map of extra validator configurations. +// These are configurations that are not part of the default configurations, +// for cases where more validators are needed. +// Commented out fields are fields that can be set, but are not necessary for the test +// they are used in so far. +// These are left as guidance to fill out as they become relevant in the future. func GetExtraValidatorConfigs() map[ValidatorID]ValidatorConfig { return map[ValidatorID]ValidatorConfig{ ValidatorID("david"): { diff --git a/x/ccv/provider/keeper/partial_set_security.go b/x/ccv/provider/keeper/partial_set_security.go index f157bee955..872222d222 100644 --- a/x/ccv/provider/keeper/partial_set_security.go +++ b/x/ccv/provider/keeper/partial_set_security.go @@ -92,7 +92,7 @@ func (k Keeper) HandleOptOut(ctx sdk.Context, chainID string, providerAddr types func (k Keeper) OptInTopNValidators(ctx sdk.Context, chainID string, bondedValidators []stakingtypes.Validator, minPowerToOptIn int64) { for _, val := range bondedValidators { // log the validator - k.Logger(ctx).Info("Opting in validator", "validator", val.GetOperator()) + k.Logger(ctx).Info("Checking whether to opt in validator because of top N", "validator", val.GetOperator()) valAddr, err := sdk.ValAddressFromBech32(val.GetOperator()) if err != nil { @@ -107,9 +107,6 @@ func (k Keeper) OptInTopNValidators(ctx sdk.Context, chainID string, bondedValid continue } if power >= minPowerToOptIn { - k.Logger(ctx).Info("Before getting cons addr, but power is good", - "validator", val.GetOperator(), - "power", power) consAddr, err := val.GetConsAddr() if err != nil { k.Logger(ctx).Error("could not retrieve validators consensus address: %s: %s", @@ -121,13 +118,6 @@ func (k Keeper) OptInTopNValidators(ctx sdk.Context, chainID string, bondedValid // if validator already exists it gets overwritten k.SetOptedIn(ctx, chainID, types.NewProviderConsAddress(consAddr)) - } else { - // else validators that do not belong to the top N validators but were opted in, remain opted in - k.Logger(ctx).Info("After getting cons addr, but power is not good", - "validator", val.GetOperator(), - "power", power, - "minPowerToOptIn", - minPowerToOptIn) } } } diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index e6dd3810a6..9c2f620636 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -276,32 +276,18 @@ func (k Keeper) MakeConsumerGenesis( return gen, nil, errorsmod.Wrapf(stakingtypes.ErrNoValidatorFound, "error getting last active bonded validators: %s", err) } - // for debugging purposes, log the active validators - for _, val := range activeValidators { - k.Logger(ctx).Info("active validator", - "operator", val.GetOperator(), - "power", val.Tokens, - ) - } - // in a Top-N chain, we automatically opt in all validators that belong to the top N minPower, err := k.ComputeMinPowerInTopN(ctx, activeValidators, prop.Top_N) if err != nil { return gen, nil, err } // log the minimum power in top N - k.Logger(ctx).Info("minimum power in top N", + k.Logger(ctx).Info("minimum power in top N at consumer genesis", "chainID", chainID, "minPower", minPower, ) k.OptInTopNValidators(ctx, chainID, activeValidators, minPower) k.SetMinimumPowerInTopN(ctx, chainID, minPower) - - // log the minimum power in top N - k.Logger(ctx).Info("minimum power in top N", - "chainID", chainID, - "minPower", minPower, - ) } // need to use the bondedValidators, not activeValidators, here since the chain might be opt-in and allow inactive vals nextValidators := k.ComputeNextValidators(ctx, chainID, bondedValidators) From 02ce7cb4325ab652b298f2aea18a573420804308 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Fri, 23 Aug 2024 12:33:19 +0200 Subject: [PATCH 3/5] Adjust mock expectations --- testutil/keeper/expectations.go | 28 ++++--------------- testutil/keeper/unit_test_helpers.go | 2 +- x/ccv/consumer/keeper/changeover_test.go | 3 -- x/ccv/consumer/keeper/keeper_test.go | 1 - x/ccv/provider/keeper/grpc_query_test.go | 5 ++-- x/ccv/provider/keeper/legacy_proposal_test.go | 2 +- .../keeper/partial_set_security_test.go | 2 +- x/ccv/provider/keeper/proposal_test.go | 12 ++++---- x/ccv/provider/keeper/relay_test.go | 8 +++--- x/ccv/provider/proposal_handler_test.go | 2 +- 10 files changed, 21 insertions(+), 44 deletions(-) diff --git a/testutil/keeper/expectations.go b/testutil/keeper/expectations.go index 9b57643167..3ceaf4fd2b 100644 --- a/testutil/keeper/expectations.go +++ b/testutil/keeper/expectations.go @@ -217,36 +217,18 @@ func GetMocksForSlashValidator( } } -// SetupMocksForLastBondedValidatorsExpectation sets up the expectation for the `IterateLastValidatorPowers` `MaxValidators`, and `GetValidator` methods of the `mockStakingKeeper` object. +// SetupMocksForLastBondedValidatorsExpectation sets up the expectation for the `GetBondedValidatorsByPower` and `MaxValidators` methods of the `mockStakingKeeper` object. // These are needed in particular when calling `GetLastBondedValidators` from the provider keeper. // Times is the number of times the expectation should be called. Provide -1 for `AnyTimes“. -func SetupMocksForLastBondedValidatorsExpectation(mockStakingKeeper *MockStakingKeeper, maxValidators uint32, vals []stakingtypes.Validator, powers []int64, times int) { - iteratorCall := mockStakingKeeper.EXPECT().IterateLastValidatorPowers(gomock.Any(), gomock.Any()).DoAndReturn( - func(ctx sdk.Context, cb func(sdk.ValAddress, int64) bool) error { - for i, val := range vals { - if stop := cb(sdk.ValAddress(val.OperatorAddress), powers[i]); stop { - break - } - } - return nil - }) +func SetupMocksForLastBondedValidatorsExpectation(mockStakingKeeper *MockStakingKeeper, maxValidators uint32, vals []stakingtypes.Validator, times int) { + validatorsCall := mockStakingKeeper.EXPECT().GetBondedValidatorsByPower(gomock.Any()).Return(vals, nil) maxValidatorsCall := mockStakingKeeper.EXPECT().MaxValidators(gomock.Any()).Return(maxValidators, nil) if times == -1 { - iteratorCall.AnyTimes() + validatorsCall.AnyTimes() maxValidatorsCall.AnyTimes() } else { - iteratorCall.Times(times) + validatorsCall.Times(times) maxValidatorsCall.Times(times) } - - // set up mocks for GetValidator calls - for _, val := range vals { - getValCall := mockStakingKeeper.EXPECT().GetValidator(gomock.Any(), sdk.ValAddress(val.OperatorAddress)).Return(val, nil) - if times == -1 { - getValCall.AnyTimes() - } else { - getValCall.Times(times) - } - } } diff --git a/testutil/keeper/unit_test_helpers.go b/testutil/keeper/unit_test_helpers.go index 0ac48f8e6c..f458ac4c3b 100644 --- a/testutil/keeper/unit_test_helpers.go +++ b/testutil/keeper/unit_test_helpers.go @@ -222,7 +222,7 @@ func SetupForStoppingConsumerChain(t *testing.T, ctx sdk.Context, ) { t.Helper() - SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{}, []int64{}, 1) + SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{}, 1) expectations := GetMocksForCreateConsumerClient(ctx, &mocks, "chainID", clienttypes.NewHeight(4, 5)) diff --git a/x/ccv/consumer/keeper/changeover_test.go b/x/ccv/consumer/keeper/changeover_test.go index b9b160ffec..ed76d956c0 100644 --- a/x/ccv/consumer/keeper/changeover_test.go +++ b/x/ccv/consumer/keeper/changeover_test.go @@ -29,8 +29,6 @@ func TestChangeoverToConsumer(t *testing.T) { cIds[4].SDKStakingValidator(), } - powers := []int64{55, 87324, 2, 42389479, 9089080} - // Instantiate 5 ics val updates for use in test initialValUpdates := []abci.ValidatorUpdate{ {Power: 55, PubKey: cIds[5].TMProtoCryptoPublicKey()}, @@ -106,7 +104,6 @@ func TestChangeoverToConsumer(t *testing.T) { mocks.MockStakingKeeper, 180, // max validators tc.lastSovVals, - powers, -1) // any times // Add ref to standalone staking keeper diff --git a/x/ccv/consumer/keeper/keeper_test.go b/x/ccv/consumer/keeper/keeper_test.go index ea90b2002f..3857c38c25 100644 --- a/x/ccv/consumer/keeper/keeper_test.go +++ b/x/ccv/consumer/keeper/keeper_test.go @@ -188,7 +188,6 @@ func TestGetLastSovereignValidators(t *testing.T) { mocks.MockStakingKeeper, 180, []stakingtypes.Validator{val}, - []int64{1000}, 1, ) diff --git a/x/ccv/provider/keeper/grpc_query_test.go b/x/ccv/provider/keeper/grpc_query_test.go index 1f45ee7f4d..27e6d00155 100644 --- a/x/ccv/provider/keeper/grpc_query_test.go +++ b/x/ccv/provider/keeper/grpc_query_test.go @@ -146,7 +146,6 @@ func TestQueryConsumerValidators(t *testing.T) { ctx, providerAddr1.ToSdkConsAddr()).Return(val, nil).Times(1) res, _ = pk.QueryConsumerValidators(ctx, &req) require.Equal(t, expectedCommissionRate, res.Validators[0].Rate) - } func TestQueryConsumerChainsValidatorHasToValidate(t *testing.T) { @@ -157,7 +156,7 @@ func TestQueryConsumerChainsValidatorHasToValidate(t *testing.T) { valConsAddr, _ := val.GetConsAddr() providerAddr := types.NewProviderConsAddress(valConsAddr) mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valConsAddr).Return(val, nil).AnyTimes() - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{val}, []int64{1}, -1) // -1 to allow the calls "AnyTimes" + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{val}, -1) // -1 to allow the calls "AnyTimes" req := types.QueryConsumerChainsValidatorHasToValidateRequest{ ProviderAddress: providerAddr.String(), @@ -248,7 +247,7 @@ func TestGetConsumerChain(t *testing.T) { } powers := []int64{50, 150, 300, 500} // sum = 1000 maxValidators := uint32(180) - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, maxValidators, vals, powers, -1) // -1 to allow the calls "AnyTimes" + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, maxValidators, vals, -1) // -1 to allow the calls "AnyTimes" for i, val := range vals { valAddr, err := sdk.ValAddressFromBech32(val.GetOperator()) diff --git a/x/ccv/provider/keeper/legacy_proposal_test.go b/x/ccv/provider/keeper/legacy_proposal_test.go index aeb990c503..23c2c6a948 100644 --- a/x/ccv/provider/keeper/legacy_proposal_test.go +++ b/x/ccv/provider/keeper/legacy_proposal_test.go @@ -111,7 +111,7 @@ func TestHandleLegacyConsumerAdditionProposal(t *testing.T) { if tc.expAppendProp { // Mock calls are only asserted if we expect a client to be created. - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{}, []int64{}, 1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{}, 1) gomock.InOrder( testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, tc.prop.ChainId, clienttypes.NewHeight(2, 3))..., ) diff --git a/x/ccv/provider/keeper/partial_set_security_test.go b/x/ccv/provider/keeper/partial_set_security_test.go index 923c307bd1..e180554239 100644 --- a/x/ccv/provider/keeper/partial_set_security_test.go +++ b/x/ccv/provider/keeper/partial_set_security_test.go @@ -134,7 +134,7 @@ func TestHandleOptOutFromTopNChain(t *testing.T) { valDConsAddr, _ := valD.GetConsAddr() mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valDConsAddr).Return(valD, nil).AnyTimes() - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 4, []stakingtypes.Validator{valA, valB, valC, valD}, []int64{1, 2, 3, 4}, -1) // -1 to allow mocks AnyTimes + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 4, []stakingtypes.Validator{valA, valB, valC, valD}, -1) // -1 to allow mocks AnyTimes // initialize the minPowerInTopN correctly minPowerInTopN, err := providerKeeper.ComputeMinPowerInTopN(ctx, []stakingtypes.Validator{valA, valB, valC, valD}, 50) diff --git a/x/ccv/provider/keeper/proposal_test.go b/x/ccv/provider/keeper/proposal_test.go index 92a971c0f4..472cb3c64c 100644 --- a/x/ccv/provider/keeper/proposal_test.go +++ b/x/ccv/provider/keeper/proposal_test.go @@ -121,7 +121,7 @@ func TestHandleConsumerAdditionProposal(t *testing.T) { if tc.expAppendProp { // Mock calls are only asserted if we expect a client to be created. - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, []int64{}, 1) // returns empty validator set + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, 1) // returns empty validator set gomock.InOrder( testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, tc.prop.ChainId, clienttypes.NewHeight(2, 3))..., ) @@ -165,7 +165,7 @@ func TestCreateConsumerClient(t *testing.T) { description: "No state mutation, new client should be created", setup: func(providerKeeper *providerkeeper.Keeper, ctx sdk.Context, mocks *testkeeper.MockedKeepers) { // Valid client creation is asserted with mock expectations here - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, []int64{}, 1) // returns empty validator set + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, 1) // returns empty validator set gomock.InOrder( testkeeper.GetMocksForCreateConsumerClient(ctx, mocks, "chainID", clienttypes.NewHeight(4, 5))..., ) @@ -181,7 +181,7 @@ func TestCreateConsumerClient(t *testing.T) { mocks.MockStakingKeeper.EXPECT().UnbondingTime(gomock.Any()).Times(0) mocks.MockClientKeeper.EXPECT().CreateClient(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) mocks.MockClientKeeper.EXPECT().GetSelfConsensusState(gomock.Any(), gomock.Any()).Times(0) - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, []int64{}, 0) // returns empty validator set + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, 0) // returns empty validator set }, expClientCreated: false, }, @@ -671,7 +671,7 @@ func TestMakeConsumerGenesis(t *testing.T) { // ctx = ctx.WithChainID("testchain1") // chainID is obtained from ctx ctx = ctx.WithBlockHeight(5) // RevisionHeight obtained from ctx - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, []int64{}, 1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, 1) gomock.InOrder(testkeeper.GetMocksForMakeConsumerGenesis(ctx, &mocks, 1814400000000000)...) // matches params from jsonString @@ -929,7 +929,7 @@ func TestBeginBlockInit(t *testing.T) { // opt in a sample validator so the chain's proposal can successfully execute validator := cryptotestutil.NewCryptoIdentityFromIntSeed(0).SDKStakingValidator() consAddr, _ := validator.GetConsAddr() - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{validator}, []int64{0}, -1) // -1 to allow any number of calls + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{validator}, -1) // -1 to allow any number of calls valAddr, _ := sdk.ValAddressFromBech32(validator.GetOperator()) mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), valAddr).Return(int64(1), nil).AnyTimes() @@ -1041,7 +1041,7 @@ func TestBeginBlockCCR(t *testing.T) { expectations := []*gomock.Call{} for _, prop := range pendingProps { // A consumer chain is setup corresponding to each prop, making these mocks necessary - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, []int64{}, 1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, 1) expectations = append(expectations, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, prop.ChainId, clienttypes.NewHeight(2, 3))...) expectations = append(expectations, testkeeper.GetMocksForSetConsumerChain(ctx, &mocks, prop.ChainId)...) diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index 20fa7a6356..91de369851 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -66,7 +66,7 @@ func TestQueueVSCPackets(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() mocks := testkeeper.NewMockedKeepers(ctrl) - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, []int64{}, 1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, 1) pk := testkeeper.NewInMemProviderKeeper(keeperParams, mocks) // no-op if tc.packets is empty @@ -101,7 +101,7 @@ func TestQueueVSCPacketsDoesNotResetConsumerValidatorsHeights(t *testing.T) { valB := createStakingValidator(ctx, mocks, 2, 2, 2) valBConsAddr, _ := valB.GetConsAddr() mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valBConsAddr).Return(valB, nil).AnyTimes() - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 2, []stakingtypes.Validator{valA, valB}, []int64{1, 2}, -1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 2, []stakingtypes.Validator{valA, valB}, -1) // set a consumer client, so we have a consumer chain (i.e., `k.GetAllConsumerChains(ctx)` is non empty) providerKeeper.SetConsumerClientId(ctx, "chainID", "clientID") @@ -603,7 +603,7 @@ func TestEndBlockVSU(t *testing.T) { powers = append(powers, int64(i+1)) } - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 5, lastValidators, powers, -1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 5, lastValidators, -1) sort.Slice(lastValidators, func(i, j int) bool { return lastValidators[i].GetConsensusPower(sdk.DefaultPowerReduction) > @@ -732,7 +732,7 @@ func TestQueueVSCPacketsWithPowerCapping(t *testing.T) { valEPubKey, _ := valE.TmConsPublicKey() mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valEConsAddr).Return(valE, nil).AnyTimes() - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 5, []stakingtypes.Validator{valA, valB, valC, valD, valE}, []int64{1, 3, 4, 8, 16}, -1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 5, []stakingtypes.Validator{valA, valB, valC, valD, valE}, -1) // add a consumer chain providerKeeper.SetConsumerClientId(ctx, "chainID", "clientID") diff --git a/x/ccv/provider/proposal_handler_test.go b/x/ccv/provider/proposal_handler_test.go index 74704e85e4..67bcad7d15 100644 --- a/x/ccv/provider/proposal_handler_test.go +++ b/x/ccv/provider/proposal_handler_test.go @@ -100,7 +100,7 @@ func TestProviderProposalHandler(t *testing.T) { // Mock expectations depending on expected outcome switch { case tc.expValidConsumerAddition: - testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{}, []int64{}, 1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{}, 1) gomock.InOrder(testkeeper.GetMocksForCreateConsumerClient( ctx, &mocks, "chainID", clienttypes.NewHeight(2, 3), )...) From 62f159e3ec73ab953925f1b30469b6a315852ce9 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Fri, 23 Aug 2024 13:02:41 +0200 Subject: [PATCH 4/5] Fix integration tests --- tests/integration/partial_set_security_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/integration/partial_set_security_test.go b/tests/integration/partial_set_security_test.go index 1fc2f6a6ee..ebc6336fee 100644 --- a/tests/integration/partial_set_security_test.go +++ b/tests/integration/partial_set_security_test.go @@ -2,6 +2,7 @@ package integration import ( "slices" + "sort" "testing" "cosmossdk.io/math" @@ -119,9 +120,19 @@ func TestMinStake(t *testing.T) { lastVals, err := providerKeeper.GetLastBondedValidators(s.providerChain.GetContext()) s.Require().NoError(err) + // Assuming tc.stakedTokens is defined somewhere in your test case + // Create a copy of the tc.stakedTokens slice + sortedTokens := make([]int64, len(tc.stakedTokens)) + copy(sortedTokens, tc.stakedTokens) + + // Sort the copied slice in descending order + sort.Slice(sortedTokens, func(i, j int) bool { + return sortedTokens[i] > sortedTokens[j] + }) + for i, val := range lastVals { // check that the initial state was set correctly - require.Equal(s.T(), math.NewInt(tc.stakedTokens[i]), val.Tokens) + require.Equal(s.T(), math.NewInt(sortedTokens[i]), val.Tokens) } // check the validator set on the consumer chain is the original one From 27ef6dcd46f16cad2e610ed3cf622fcddc8f1985 Mon Sep 17 00:00:00 2001 From: Philip Offtermatt Date: Fri, 23 Aug 2024 14:39:06 +0200 Subject: [PATCH 5/5] Adjust info to debug; return err instead of panicking --- x/ccv/provider/keeper/partial_set_security.go | 4 ++-- x/ccv/types/utils.go | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/x/ccv/provider/keeper/partial_set_security.go b/x/ccv/provider/keeper/partial_set_security.go index 872222d222..3112124724 100644 --- a/x/ccv/provider/keeper/partial_set_security.go +++ b/x/ccv/provider/keeper/partial_set_security.go @@ -92,7 +92,7 @@ func (k Keeper) HandleOptOut(ctx sdk.Context, chainID string, providerAddr types func (k Keeper) OptInTopNValidators(ctx sdk.Context, chainID string, bondedValidators []stakingtypes.Validator, minPowerToOptIn int64) { for _, val := range bondedValidators { // log the validator - k.Logger(ctx).Info("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", "validator", val.GetOperator()) valAddr, err := sdk.ValAddressFromBech32(val.GetOperator()) if err != nil { @@ -114,7 +114,7 @@ func (k Keeper) OptInTopNValidators(ctx sdk.Context, chainID string, bondedValid continue } - k.Logger(ctx).Info("Opting in validator", "validator", val.GetOperator()) + k.Logger(ctx).Debug("Opting in validator", "validator", val.GetOperator()) // if validator already exists it gets overwritten k.SetOptedIn(ctx, chainID, types.NewProviderConsAddress(consAddr)) diff --git a/x/ccv/types/utils.go b/x/ccv/types/utils.go index 03e5f63be1..05e2387eb8 100644 --- a/x/ccv/types/utils.go +++ b/x/ccv/types/utils.go @@ -2,7 +2,6 @@ package types import ( "errors" - fmt "fmt" "reflect" "sort" "strings" @@ -132,7 +131,7 @@ func GetLastBondedValidatorsUtil(ctx sdk.Context, stakingKeeper StakingKeeper, l // get the bonded validators from the staking module, sorted by power bondedValidators, err := stakingKeeper.GetBondedValidatorsByPower(ctx) if err != nil { - panic(fmt.Errorf("failed to get bonded validators: %w", err)) + return nil, err } // get the first maxVals many validators