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

p-offtermatt · 2024-05-14T11:43:36Z

Description

Closes: #1913

This PR allows validators that are not participating in consensus on the provider chain to validate on consumer chains.
The extremely high level overview can be found here (I recommend reading this to get the high-level idea):
https://informalsystems.notion.site/Non-active-validators-on-consumer-chains-d4b700e19a7d447c8d9a2e69cca3e8d0?pvs=74

I recommend then reviewing the e2e test so that you can see what the new logic does in practice.
Other notable files:

validator_set_storage.go and provider_consensus.go: generalize the existing functionality for storing a consumer val set
wrapped_genutil/wrapped_staking: wrappers around these modules to prevent them from sending valupdates to comet, but still get their functionality otherwise
params.go: introduce a new parameter for the maximal consensus validators on the provider

Some changed I will make later but which should not block reviewing:

Rename ConsumerValidator and its fields to reflect that it also is used for the validator set on the provider: this change will make the diff much larger and less informative, so holding off on it
additional integration

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Included the correct type prefix in the PR title
Added ! to the type prefix if the change is state-machine breaking
Confirmed this PR does not introduce changes requiring state migrations, OR migration code has been added to consumer and/or provider modules

The LastConsensusValidatorSet needs to be initialized via migration to get a correct diff, this is still TODO

Targeted the correct branch (see PR Targeting)

Maybe this should target a feature branch?

Provided a link to the relevant issue or specification
Followed the guidelines for building SDK modules
Included the necessary unit and integration tests
Added a changelog entry to CHANGELOG.md
Included comments for documenting Go code
Updated the relevant documentation or specification
Reviewed "Files changed" and left comments if necessary
Confirmed all CI checks have passed
If this PR is library API breaking, bump the go.mod version string of the repo, and follow through on a new major release

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! the type prefix if the change is state-machine breaking
confirmed this PR does not introduce changes requiring state migrations, OR confirmed migration code has been added to consumer and/or provider modules
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage

…ider's consensus

This reverts commit 2e5a91a.

x/ccv/provider/module.go

x/ccv/provider/keeper/validator_set_storage.go

+	store := ctx.KVStore(k.storeKey)
+	bz, err := validator.Marshal()
+	if err != nil {
+		panic(fmt.Errorf("failed to marshal ConsumerValidator: %w", err))


x/ccv/provider/keeper/validator_set_storage.go

+		iterator.Value()
+		var validator types.ConsumerValidator
+		if err := validator.Unmarshal(iterator.Value()); err != nil {
+			panic(fmt.Errorf("failed to unmarshal ConsumerValidator: %w", err))


mpoke · 2024-05-28T08:33:53Z

x/ccv/provider/module.go

+	// Important: must be called before EndBlockVSU, because
+	// EndBlockVSU needs to know the updated provider validator set
+	// to compute the minimum power in the top N
+	providerUpdates := am.keeper.ProviderValidatorUpdates(ctx)


This function should be part of EndBlockVSU. Just return providerUpdates from EndBlockVSU.

p-offtermatt · 2024-05-24T10:09:51Z

app/provider/app.go

@@ -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(


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

coderabbitai · 2024-05-24T10:21:39Z

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

Commits

Files that changed from the base of the PR and between 369554c and 5ea92ea.

Files ignored due to path filters (1)

x/ccv/provider/types/provider.pb.go is excluded by !**/*.pb.go

Files selected for processing (38)

app/provider/app.go (8 hunks)
proto/interchain_security/ccv/provider/v1/provider.proto (1 hunks)
tests/e2e/config.go (3 hunks)
tests/e2e/main.go (1 hunks)
tests/e2e/state.go (2 hunks)
tests/e2e/steps_inactive_vals.go (1 hunks)
testutil/keeper/mocks.go (1 hunks)
testutil/keeper/unit_test_helpers.go (2 hunks)
x/ccv/provider/keeper/genesis.go (2 hunks)
x/ccv/provider/keeper/genesis_test.go (2 hunks)
x/ccv/provider/keeper/keeper.go (2 hunks)
x/ccv/provider/keeper/keeper_test.go (1 hunks)
x/ccv/provider/keeper/params.go (2 hunks)
x/ccv/provider/keeper/params_test.go (1 hunks)
x/ccv/provider/keeper/partial_set_security.go (3 hunks)
x/ccv/provider/keeper/partial_set_security_test.go (6 hunks)
x/ccv/provider/keeper/proposal.go (1 hunks)
x/ccv/provider/keeper/proposal_test.go (4 hunks)
x/ccv/provider/keeper/provider_consensus.go (1 hunks)
x/ccv/provider/keeper/provider_consensus_test.go (1 hunks)
x/ccv/provider/keeper/relay.go (3 hunks)
x/ccv/provider/keeper/relay_test.go (2 hunks)
x/ccv/provider/keeper/throttle.go (1 hunks)
x/ccv/provider/keeper/throttle_test.go (13 hunks)
x/ccv/provider/keeper/validator_set_storage.go (1 hunks)
x/ccv/provider/keeper/validator_set_update.go (2 hunks)
x/ccv/provider/module.go (2 hunks)
x/ccv/provider/module_test.go (3 hunks)
x/ccv/provider/types/genesis_test.go (13 hunks)
x/ccv/provider/types/keys.go (2 hunks)
x/ccv/provider/types/keys_test.go (1 hunks)
x/ccv/provider/types/params.go (6 hunks)
x/ccv/provider/types/params_test.go (1 hunks)
x/ccv/types/expected_keepers.go (1 hunks)
x/ccv/wrapped_genutil/doc.go (1 hunks)
x/ccv/wrapped_genutil/module.go (1 hunks)
x/ccv/wrapped_staking/doc.go (1 hunks)
x/ccv/wrapped_staking/module.go (1 hunks)

 _____________________________________________
< My middle name? 'Bugfinder Extraordinaire.' >
 ---------------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

mpoke

Great work @p-offtermatt. The general logic LGTM. See my comment below.

Things missing:

migration (e.g., the LastProviderConsensusValSet needs to be initialized)
docs on integration (i.e., how to wire this in app.go, especially given the wrapped modules)
the provider module needs to implement the staking keeper interface, similarly to the consumer module (some of the modules that use the staking keeper need to use the provider keeper instead, e.g., mint, gov, distribution, IBC, ... )
finalized ADR

mpoke · 2024-05-28T08:30:02Z

x/ccv/provider/types/params.go

@@ -82,6 +88,7 @@ func NewParams(
 		SlashMeterReplenishFraction:        slashMeterReplenishFraction,
 		ConsumerRewardDenomRegistrationFee: consumerRewardDenomRegistrationFee,
 		BlocksPerEpoch:                     blocksPerEpoch,
+		MaxProviderConsensusValidators:     maxProviderConsensusValidators,


This param needs to be validated in the Validate() below.

mpoke · 2024-05-28T08:33:53Z

x/ccv/provider/module.go

+	// Important: must be called before EndBlockVSU, because
+	// EndBlockVSU needs to know the updated provider validator set
+	// to compute the minimum power in the top N
+	providerUpdates := am.keeper.ProviderValidatorUpdates(ctx)


This function should be part of EndBlockVSU. Just return providerUpdates from EndBlockVSU.

mpoke · 2024-05-28T08:37:53Z

app/provider/app.go

+	wrapped_genutil "github.com/cosmos/interchain-security/v4/x/ccv/wrapped_genutil"
+	wrapped_staking "github.com/cosmos/interchain-security/v4/x/ccv/wrapped_staking"


We need to find better naming for these two modules.

mpoke · 2024-05-28T08:38:16Z

app/provider/app.go

@@ -129,7 +134,8 @@ var (
 		genutil.NewAppModuleBasic(genutiltypes.DefaultMessageValidator),
 		bank.AppModuleBasic{},
 		capability.AppModuleBasic{},
-		staking.AppModuleBasic{},
+		// staking.AppModuleBasic{},


Remove commented code.

mpoke · 2024-05-28T08:38:34Z

app/provider/app.go

@@ -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)),


ditto: remove commented code

mpoke · 2024-05-28T09:08:27Z

x/ccv/provider/keeper/relay.go

@@ -217,17 +219,18 @@ func (k Keeper) SendVSCPacketsToChain(ctx sdk.Context, chainID, channelID string
 func (k Keeper) QueueVSCPackets(ctx sdk.Context) {
 	valUpdateID := k.GetValidatorSetUpdateId(ctx) // current valset update ID

-	// get the bonded validators from the staking module


This docstring is still relevant. Explain better why bondedValidators is needed and why consensusValidators is needed (i.e., for TopN chains).

mpoke · 2024-05-28T09:14:39Z

x/ccv/provider/keeper/throttle.go

-	totalPower := k.stakingKeeper.GetLastTotalPower(ctx)
+
+	// Get total total power of the last consensus validator set on the provider
+	totalPower := k.GetLastTotalProviderConsensusPower(ctx)


Note that this changes the behavior. stakingKeeper.GetLastTotalPower gets the total power from the store, while GetLastTotalProviderConsensusPower iterates over 180 keys and recomputes the power every time.

mpoke · 2024-05-28T09:15:53Z

x/ccv/provider/keeper/validator_set_update.go

 	ctx sdk.Context,
 	chainID string,
 	validator types.ConsumerValidator,
 ) {
-	store := ctx.KVStore(k.storeKey)
-	bz, err := validator.Marshal()
-	if err != nil {
-		panic(fmt.Errorf("failed to marshal ConsumerValidator: %w", err))
-	}
-
-	store.Set(types.ConsumerValidatorKey(chainID, validator.ProviderConsAddr), bz)


mpoke · 2024-05-28T09:18:05Z

x/ccv/wrapped_genutil/module.go

+
+// InitGenesis delegates the InitGenesis call to the underlying x/genutil module,
+// however, it returns no validator updates as validator updates will be provided by the provider module.
+func (am AppModule) InitGenesis(ctx sdk.Context, cdc codec.JSONCodec, data json.RawMessage) []abci.ValidatorUpdate {


Why is this needed? We don't have a wrapped genutil in the democracy module on the consumer. How is this different?

mpoke · 2024-05-28T09:25:54Z

x/ccv/provider/keeper/genesis.go

+		}
+	}
+
+	k.SetLastProviderConsensusValSet(ctx, reducedValSet)


This should also be exported in ExportGenesis

insumity · 2024-05-28T13:53:24Z

tests/e2e/steps_inactive_vals.go

+				State: State{
+					ChainID("provi"): ChainState{
+						ValPowers: &map[ValidatorID]uint{
+							ValidatorID("alice"): 0, // alice is still not in the active set, and should now be jailed too


What do you mean by and should now be jailed too?
alice is not in the active set on the provider, so what is this testing? Even if alice is to be jailed on the provider, can we actually test with ValPowers here?

Update: I just saw the comment below

// we need to double-check that alice is actually jailed, so we get bob jailed, too, which usually would mean alice gets into power

so maybe that is something you can talk about here as well.

insumity · 2024-05-28T13:56:05Z

tests/e2e/steps_inactive_vals.go

+					},
+				},
+			},
+


nit: remove new line

insumity · 2024-05-28T13:58:40Z

tests/e2e/steps_inactive_vals.go

+				State: State{
+					ChainID("provi"): ChainState{
+						ValPowers: &map[ValidatorID]uint{
+							ValidatorID("alice"): 100, // alice is back as an active consensus validator


I guess alice did not get slashed due to DowntimeSlashAction because DowntimeSlashAction was on the consu chain, right?

Downtime on consumer chains does not slash, just jail

I should comment on the distinction :)

insumity · 2024-05-28T14:03:53Z

tests/e2e/steps_inactive_vals.go

+				},
+			},
+		},
+		// Οpt in "alice" and "bob" so the chain is not empty when it is about to start. Note, that "alice" and "bob" use


This is a setup function, so why not opt in all 3 validators at once? What's the benefit of opting in bob after the chain starts?

insumity · 2024-05-28T14:05:41Z

testutil/keeper/unit_test_helpers.go

@@ -298,3 +299,21 @@ func GetNewCrossChainValidator(t *testing.T) consumertypes.CrossChainValidator {
 	require.NoError(t, err)
 	return validator
 }
+
+// CreateStakingValidator helper function to generate a validator with the given power and with a provider address based on index


The exact same function exists here. Maybe clean up so that only one function exists (the one you have here).

insumity · 2024-05-28T15:08:03Z

x/ccv/provider/keeper/relay.go

+	}
+	for _, val := range bondedValidators[:maxValidators] {
+		// create the validator from the staking validator
+		consAddr, err := val.GetConsAddr()


You could potentially reuse CreateConsumerValidator with an empty chainID.

insumity · 2024-05-28T15:22:06Z

x/ccv/provider/keeper/relay.go

+				"error", err)
+			continue
+		}
+		nextValidator := types.ConsumerValidator{


General comment but maybe we should not be calling those **Consumer** validators anymore because they're also provider validators.

Yeah, agreed. Didn't think of a good, unambigious way for this yet

insumity · 2024-05-28T15:23:27Z

x/ccv/provider/keeper/proposal.go

@@ -262,11 +262,14 @@ func (k Keeper) MakeConsumerGenesis(
 	// get the bonded validators from the staking module
 	bondedValidators := k.stakingKeeper.GetLastValidators(ctx)

+	// get the consensus validators (to compute the minimal power in the top N)
+	consensusValidators := k.GetLastProviderConsensusValSet(ctx)


Why do we need both consensus and bonded validators?

insumity · 2024-05-28T15:26:52Z

x/ccv/provider/keeper/genesis.go

+
+	reducedValSet := make([]types.ConsumerValidator, len(valSet))
+	for i, val := range valSet {
+		consAddr, err := val.GetConsAddr()


See previous comment on potentially using CreateConsumerValidator with empty chainID.

jtremback and others added 6 commits May 12, 2024 21:55

rough draft of one method of sending a reduced active set to the prov…

0fcb57b

…ider's consensus

Add key for last consensus validator set on the provider

c0d8581

Refactor validator set storage to reduce duplication

b88cfb5

Add draft ADR for active set validators

2e5a91a

Revert "Add draft ADR for active set validators"

2cddff5

This reverts commit 2e5a91a.

Add MaxProviderConsensusValidators param

acdf06a

github-actions bot added the C:x/provider Assigned automatically by the PR labeler label May 14, 2024

github-advanced-security bot found potential problems May 14, 2024

View reviewed changes

p-offtermatt added 2 commits May 14, 2024 17:00

Use MaxProviderConsensusValidators param

48f4085

Utilize wrapper around staking module

5ccdb18

github-actions bot added the C:Testing Assigned automatically by the PR labeler label May 14, 2024

p-offtermatt added 9 commits May 15, 2024 08:45

Use consensus power for throttle meter

48a1aba

Add new param to tests

66139cc

Start fixing genesis

c11e65d

Fix mock usage in tests

d21cf9a

Make Genesis return consensus validators

87742aa

Add debugging info in InitGenesis

fb1a8e5

Add wrapped_genutil

b632aca

Fix wrapped genutil integration

519a329

Start adding e2e test

0285039

github-actions bot added the C:x/types Assigned automatically by the PR labeler label May 23, 2024

p-offtermatt added 2 commits May 24, 2024 11:31

Adjust tests

9e58c13

Merge branch 'main' into ph/max-consensus-validators

134d840

github-advanced-security bot found potential problems May 24, 2024

View reviewed changes

p-offtermatt added 3 commits May 24, 2024 11:48

Adjust expected calls

70a6bf1

Set ProviderConsensusValSet in proposal test

1265836

Extract variable

affd10f

p-offtermatt changed the title ~~feat!: Allow validators outside the active set to validate on consumer chains~~ feat!: Allow validators outside the active set to validate on consumer chains (DO NOT MERGE) May 24, 2024

p-offtermatt changed the title ~~feat!: Allow validators outside the active set to validate on consumer chains (DO NOT MERGE)~~ feat!: Allow validators outside the active set to validate on consumer chains May 24, 2024

p-offtermatt changed the base branch from main to feat/non-active-validators May 24, 2024 10:08

p-offtermatt commented May 24, 2024

View reviewed changes

Improve comment

5ea92ea

p-offtermatt marked this pull request as ready for review May 24, 2024 10:21

p-offtermatt requested a review from a team as a code owner May 24, 2024 10:21

mpoke reviewed May 28, 2024

View reviewed changes

insumity reviewed May 28, 2024

View reviewed changes

tests/e2e/steps_inactive_vals.go

},

},

},

Copy link

Contributor

insumity May 28, 2024

Choose a reason for hiding this comment

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

nit: remove new line

insumity reviewed May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

p-offtermatt commented May 14, 2024 •

edited

mpoke May 28, 2024

p-offtermatt May 24, 2024

coderabbitai bot commented May 24, 2024

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

mpoke left a comment

mpoke May 28, 2024

mpoke May 28, 2024

mpoke May 28, 2024

mpoke May 28, 2024

mpoke May 28, 2024

mpoke May 28, 2024

mpoke May 28, 2024

mpoke May 28, 2024

mpoke May 28, 2024

mpoke May 28, 2024

insumity May 28, 2024 •

edited

insumity May 28, 2024

insumity May 28, 2024 •

edited

p-offtermatt May 28, 2024

insumity May 28, 2024

insumity May 28, 2024

insumity May 28, 2024

insumity May 28, 2024

p-offtermatt May 28, 2024

insumity May 28, 2024

insumity May 28, 2024

		wrapped_genutil "github.com/cosmos/interchain-security/v4/x/ccv/wrapped_genutil"
		wrapped_staking "github.com/cosmos/interchain-security/v4/x/ccv/wrapped_staking"

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

Are you sure you want to change the base?

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

Conversation

p-offtermatt commented May 14, 2024 • edited

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot commented May 24, 2024

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

mpoke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insumity May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insumity May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-offtermatt commented May 14, 2024 •

edited

CodeRabbit Configration File (`.coderabbit.yaml`)

insumity May 28, 2024 •

edited

insumity May 28, 2024 •

edited