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

feat!: Allow validators outside the active set to validate on consumer chains #1878

Closed
wants to merge 30 commits into from
Closed

feat!: Allow validators outside the active set to validate on consumer chains #1878

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
0fcb57b
rough draft of one method of sending a reduced active set to the prov…
jtremback May 13, 2024
c0d8581
Add key for last consensus validator set on the provider
p-offtermatt May 14, 2024
b88cfb5
Refactor validator set storage to reduce duplication
p-offtermatt May 14, 2024
2e5a91a
Add draft ADR for active set validators
p-offtermatt May 14, 2024
2cddff5
Revert "Add draft ADR for active set validators"
p-offtermatt May 14, 2024
acdf06a
Add MaxProviderConsensusValidators param
p-offtermatt May 14, 2024
48f4085
Use MaxProviderConsensusValidators param
p-offtermatt May 14, 2024
5ccdb18
Utilize wrapper around staking module
p-offtermatt May 14, 2024
48a1aba
Use consensus power for throttle meter
p-offtermatt May 15, 2024
66139cc
Add new param to tests
p-offtermatt May 15, 2024
c11e65d
Start fixing genesis
p-offtermatt May 15, 2024
d21cf9a
Fix mock usage in tests
p-offtermatt May 15, 2024
87742aa
Make Genesis return consensus validators
p-offtermatt May 15, 2024
fb1a8e5
Add debugging info in InitGenesis
p-offtermatt May 15, 2024
b632aca
Add wrapped_genutil
p-offtermatt May 15, 2024
519a329
Fix wrapped genutil integration
p-offtermatt May 16, 2024
0285039
Start adding e2e test
p-offtermatt May 23, 2024
9e58c13
Adjust tests
p-offtermatt May 24, 2024
134d840
Merge branch 'main' into ph/max-consensus-validators
p-offtermatt May 24, 2024
70a6bf1
Adjust expected calls
p-offtermatt May 24, 2024
1265836
Set ProviderConsensusValSet in proposal test
p-offtermatt May 24, 2024
affd10f
Extract variable
p-offtermatt May 24, 2024
5ea92ea
Improve comment
p-offtermatt May 24, 2024
617b515
Merge branch 'main' into ph/max-consensus-validators
p-offtermatt Jun 5, 2024
9822bbf
Comment on slashing vs jailing
p-offtermatt Jun 5, 2024
b29b867
Update e2e test
p-offtermatt Jun 5, 2024
9b5e578
Remove commented code
p-offtermatt Jun 11, 2024
bfb83ab
Move createStakingValidator to testkeeper
p-offtermatt Jun 24, 2024
a7de72a
Address comments
p-offtermatt Jun 24, 2024
faff1a7
Address comments
p-offtermatt Jun 24, 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
58 changes: 52 additions & 6 deletions app/provider/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package app

import (
"encoding/json"
"fmt"
"io"
stdlog "log"
Expand Down Expand Up @@ -86,7 +87,6 @@ import (
"github.com/cosmos/cosmos-sdk/x/slashing"
slashingkeeper "github.com/cosmos/cosmos-sdk/x/slashing/keeper"
slashingtypes "github.com/cosmos/cosmos-sdk/x/slashing/types"
"github.com/cosmos/cosmos-sdk/x/staking"
stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
"github.com/cosmos/cosmos-sdk/x/upgrade"
Expand All @@ -107,6 +107,11 @@ import (
ibcproviderclient "github.com/cosmos/interchain-security/v4/x/ccv/provider/client"
ibcproviderkeeper "github.com/cosmos/interchain-security/v4/x/ccv/provider/keeper"
providertypes "github.com/cosmos/interchain-security/v4/x/ccv/provider/types"

wrapped_genutil "github.com/cosmos/interchain-security/v4/x/ccv/wrapped_genutil"
wrapped_staking "github.com/cosmos/interchain-security/v4/x/ccv/wrapped_staking"
Comment on lines +111 to +112
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to find better naming for these two modules.

Comment on lines +111 to +112
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming wrapped_genutil and wrapped_staking for clarity.

The current names wrapped_genutil and wrapped_staking are not very descriptive. Consider using names that reflect their functionality or the reason why they are wrapped, which could improve code readability and maintainability.


sdkmoduletypes "github.com/cosmos/cosmos-sdk/types/module"
)

const (
Expand All @@ -129,7 +134,8 @@ var (
genutil.NewAppModuleBasic(genutiltypes.DefaultMessageValidator),
bank.AppModuleBasic{},
capability.AppModuleBasic{},
staking.AppModuleBasic{},
// staking.AppModuleBasic{},
p-offtermatt marked this conversation as resolved.
Show resolved Hide resolved
wrapped_staking.AppModuleBasic{},
mint.AppModuleBasic{},
distr.AppModuleBasic{},
gov.NewAppModuleBasic(
Expand Down Expand Up @@ -498,7 +504,7 @@ func New(
// NOTE: Any module instantiated in the module manager that is later modified
// must be passed by reference here.
app.MM = module.NewManager(
genutil.NewAppModule(
wrapped_genutil.NewAppModule(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Genutil and staking need to be wrapped, because they send validator updates to CometBFT and we need to filter those.

app.AccountKeeper,
app.StakingKeeper,
app.BaseApp.DeliverTx,
Expand All @@ -513,7 +519,8 @@ func New(
mint.NewAppModule(appCodec, app.MintKeeper, app.AccountKeeper, nil, app.GetSubspace(minttypes.ModuleName)),
slashing.NewAppModule(appCodec, app.SlashingKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper, app.GetSubspace(slashingtypes.ModuleName)),
distr.NewAppModule(appCodec, app.DistrKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper, app.GetSubspace(distrtypes.ModuleName)),
staking.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper, app.GetSubspace(stakingtypes.ModuleName)),
// staking.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper, app.GetSubspace(stakingtypes.ModuleName)),
p-offtermatt marked this conversation as resolved.
Show resolved Hide resolved
wrapped_staking.NewAppModule(appCodec, *app.StakingKeeper, app.AccountKeeper, app.BankKeeper, app.GetSubspace(stakingtypes.ModuleName)),
upgrade.NewAppModule(&app.UpgradeKeeper),
evidence.NewAppModule(app.EvidenceKeeper),
ibc.NewAppModule(app.IBCKeeper),
Expand Down Expand Up @@ -608,7 +615,8 @@ func New(
capability.NewAppModule(appCodec, *app.CapabilityKeeper, false),
gov.NewAppModule(appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper, app.GetSubspace(govtypes.ModuleName)),
mint.NewAppModule(appCodec, app.MintKeeper, app.AccountKeeper, nil, app.GetSubspace(minttypes.ModuleName)),
staking.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper, app.GetSubspace(stakingtypes.ModuleName)),
// staking.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper, app.GetSubspace(stakingtypes.ModuleName)),
p-offtermatt marked this conversation as resolved.
Show resolved Hide resolved
wrapped_staking.NewAppModule(appCodec, *app.StakingKeeper, app.AccountKeeper, app.BankKeeper, app.GetSubspace(stakingtypes.ModuleName)),
distr.NewAppModule(appCodec, app.DistrKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper, app.GetSubspace(distrtypes.ModuleName)),
slashing.NewAppModule(appCodec, app.SlashingKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper, app.GetSubspace(slashingtypes.ModuleName)),
params.NewAppModule(app.ParamsKeeper),
Expand Down Expand Up @@ -721,7 +729,45 @@ func (app *App) InitChainer(ctx sdk.Context, req abci.RequestInitChain) abci.Res

app.UpgradeKeeper.SetModuleVersionMap(ctx, app.MM.GetVersionMap())

return app.MM.InitGenesis(ctx, app.appCodec, genesisState)
return InitGenesis(app.MM, ctx, app.appCodec, genesisState)
}

// InitGenesis performs init genesis functionality for modules. Exactly one
// module must return a non-empty validator set update to correctly initialize
// the chain.
func InitGenesis(m *sdkmoduletypes.Manager, ctx sdk.Context, cdc codec.JSONCodec, genesisData map[string]json.RawMessage) abci.ResponseInitChain {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this function here? How is it different from the original call to app.MM.InitGenesis?

var validatorUpdates []abci.ValidatorUpdate
ctx.Logger().Info("initializing blockchain state from genesis.json")
for _, moduleName := range m.OrderInitGenesis {
if genesisData[moduleName] == nil {
continue
}

if module, ok := m.Modules[moduleName].(sdkmoduletypes.HasGenesis); ok {
ctx.Logger().Debug("running initialization for module", "module", moduleName)

moduleValUpdates := module.InitGenesis(ctx, cdc, genesisData[moduleName])

// use these validator updates if provided, the module manager assumes
// only one module will update the validator set
if len(moduleValUpdates) > 0 {
fmt.Println("val updates by module", moduleName)
if len(validatorUpdates) > 0 {
panic("validator InitGenesis updates already set by a previous module")
}
validatorUpdates = moduleValUpdates
}
}
}

// a chain must initialize with a non-empty validator set
if len(validatorUpdates) == 0 {
panic(fmt.Sprintf("validator set is empty after InitGenesis, please ensure at least one validator is initialized with a delegation greater than or equal to the DefaultPowerReduction (%d)", sdk.DefaultPowerReduction))
}

return abci.ResponseInitChain{
Validators: validatorUpdates,
}
}

// LoadHeight loads a particular height
Expand Down
4 changes: 4 additions & 0 deletions proto/interchain_security/ccv/provider/v1/provider.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,10 @@ message Params {

// The number of blocks that comprise an epoch.
int64 blocks_per_epoch = 10;

// The maximal number of validators that will be passed
// to the consensus engine on the provider.
int64 max_provider_consensus_validators = 11;
Comment on lines +216 to +218
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The search results indicate that the field max_provider_consensus_validators is defined in the provider.proto file but there is no evidence of its documentation or validation in the codebase.

  • Ensure that the new field max_provider_consensus_validators is properly documented.
  • Implement validation logic for max_provider_consensus_validators to ensure it is correctly configured and used.
Analysis chain

Ensure the new field max_provider_consensus_validators is properly documented and validated.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the new field is used in the consensus engine configuration.

# Test: Search for the usage of `max_provider_consensus_validators`. Expect: Configurations or validations that use this field.
rg --type go $'max_provider_consensus_validators'

Length of output: 407

Script:

#!/bin/bash
# Description: Verify that the new field is documented and validated in the consensus engine configuration.

# Test: Search for documentation related to `max_provider_consensus_validators`.
rg --type go --type md --type proto 'max_provider_consensus_validators'

# Test: Search for validation logic related to `max_provider_consensus_validators`.
rg --type go 'validate.*max_provider_consensus_validators'

Length of output: 162

Script:

#!/bin/bash
# Description: Verify that the new field is documented and validated in the consensus engine configuration.

# Test: Search for documentation related to `max_provider_consensus_validators`.
rg --type go --type md --type 'proto' 'max_provider_consensus_validators'

# Test: Search for validation logic related to `max_provider_consensus_validators`.
rg --type go 'validate.*max_provider_consensus_validators'

Length of output: 162

Script:

#!/bin/bash
# Description: Verify that the new field is documented and validated in the consensus engine configuration.

# Test: Search for documentation related to `max_provider_consensus_validators`.
rg --type go --type md --glob '**/*.proto' 'max_provider_consensus_validators'

# Test: Search for validation logic related to `max_provider_consensus_validators`.
rg --type go 'validate.*max_provider_consensus_validators'

Length of output: 244

}

// SlashAcks contains cons addresses of consumer chain validators
Expand Down
19 changes: 19 additions & 0 deletions tests/e2e/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ const (
MulticonsumerTestCfg TestConfigType = "multi-consumer"
ConsumerMisbehaviourTestCfg TestConfigType = "consumer-misbehaviour"
CompatibilityTestCfg TestConfigType = "compatibility"
InactiveValsCfg TestConfigType = "inactive-vals"
)

// Attributes that are unique to a validator. Allows us to map (part of)
Expand Down Expand Up @@ -264,6 +265,8 @@ func GetTestConfig(cfgType TestConfigType, providerVersion, consumerVersion stri
testCfg = ConsumerMisbehaviourTestConfig()
case CompatibilityTestCfg:
testCfg = CompatibilityTestConfig(pv, cv)
case InactiveValsCfg:
testCfg = InactiveValsConfig()
p-offtermatt marked this conversation as resolved.
Show resolved Hide resolved
default:
panic(fmt.Sprintf("Invalid test config: %s", cfgType))
}
Expand Down Expand Up @@ -500,6 +503,22 @@ func CompatibilityTestConfig(providerVersion, consumerVersion string) TestConfig
return testCfg
}

func InactiveValsConfig() TestConfig {
tr := DefaultTestConfig()
tr.name = "InactiveValsConfig"
// set the MaxProviderConsensusValidators param to 2
proviConfig := tr.chainConfigs[ChainID("provi")]
proviConfig.GenesisChanges += " | .app_state.provider.params.max_provider_consensus_validators = \"2\""

consuConfig := tr.chainConfigs[ChainID("consu")]
// set the soft_opt_out threshold to 0% to make sure all validators are slashed for downtime
consuConfig.GenesisChanges += " | .app_state.ccvconsumer.params.soft_opt_out_threshold = \"0.0\""
tr.chainConfigs[ChainID("provi")] = proviConfig
tr.chainConfigs[ChainID("consu")] = consuConfig

return tr
}

func DefaultTestConfig() TestConfig {
tr := TestConfig{
name: string(DefaultTestCfg),
Expand Down
6 changes: 6 additions & 0 deletions tests/e2e/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,12 @@ var stepChoices = map[string]StepChoice{
description: "test partial set security for a Top-N chain",
testConfig: DefaultTestCfg,
},
"inactive-validators-on-consumer": {
name: "inactive-validators-on-consumer",
steps: stepsInactiveValidatorsOnConsumer(),
description: "test inactive validators on consumer",
testConfig: InactiveValsCfg,
},
"partial-set-security-validator-set-cap": {
name: "partial-set-security-validator-set-cap",
steps: stepsValidatorSetCappedChain(),
Expand Down
13 changes: 9 additions & 4 deletions tests/e2e/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,22 +328,22 @@ func (tr TestConfig) getRewards(chain ChainID, modelState Rewards) Rewards {

func (tr TestConfig) getReward(chain ChainID, validator ValidatorID, blockHeight uint, isNativeDenom bool) float64 {
valCfg := tr.validatorConfigs[validator]
delAddresss := valCfg.DelAddress
delAddress := valCfg.DelAddress
if chain != ChainID("provi") {
// use binary with Bech32Prefix set to ConsumerAccountPrefix
if valCfg.UseConsumerKey {
delAddresss = valCfg.ConsumerDelAddress
delAddress = valCfg.ConsumerDelAddress
} else {
// use the same address as on the provider but with different prefix
delAddresss = valCfg.DelAddressOnConsumer
delAddress = valCfg.DelAddressOnConsumer
Comment on lines +331 to +338
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor to improve address handling logic.

The logic for determining the delegate address based on the chain type is repeated multiple times throughout the file. Consider abstracting this logic into a helper function to reduce code duplication and improve maintainability.

- if chain != ChainID("provi") {
-     // use binary with Bech32Prefix set to ConsumerAccountPrefix
-     if valCfg.UseConsumerKey {
-         delAddress = valCfg.ConsumerDelAddress
-     } else {
-         // use the same address as on the provider but with different prefix
-         delAddress = valCfg.DelAddressOnConsumer
-     }
- }
+ delAddress = getDelegateAddress(chain, valCfg)

And then define getDelegateAddress appropriately.

Committable suggestion was skipped due to low confidence.

}
}

//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
bz, err := exec.Command("docker", "exec", tr.containerConfig.InstanceName, tr.chainConfigs[chain].BinaryName,

"query", "distribution", "rewards",
delAddresss,
delAddress,

`--height`, fmt.Sprint(blockHeight),
`--node`, tr.getQueryNode(chain),
Expand All @@ -358,6 +358,11 @@ func (tr TestConfig) getReward(chain ChainID, validator ValidatorID, blockHeight
denomCondition = `total.#(denom=="stake").amount`
}

if *verbose {
log.Println("Getting reward for chain: ", chain, " validator: ", validator, " block: ", blockHeight)
log.Println("Reward response: ", string(bz))
}
Comment on lines +361 to +364
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure verbose logging is controlled by a flag.

The verbose logging within the getReward function is always enabled. It's a good practice to control verbose output with a flag to prevent cluttering the log files in production environments.

- if *verbose {
-     log.Println("Getting reward for chain: ", chain, " validator: ", validator, " block: ", blockHeight)
-     log.Println("Reward response: ", string(bz))
- }
+ if tr.verbose {
+     log.Println("Getting reward for chain: ", chain, " validator: ", validator, " block: ", blockHeight)
+     log.Println("Reward response: ", string(bz))
+ }

Ensure that tr.verbose is a configurable flag within TestConfig.

Committable suggestion was skipped due to low confidence.


return gjson.Get(string(bz), denomCondition).Float()
}

Expand Down
Loading
Loading