feat!: Wire the provider module to return ValidatorUpdates, instead of the staking module

[p-offtermatt]

Description

Closes: Part of #1913

Main changes:

• x/ccv/provider/module.go:
  • Change the module to have ABCI Genesis & EndBlock
  • Accordingly, change InitGenesis and EndBlock to return ValidatorUpdates
• x/ccv/provider/keeper/relay.go: Adjust EndBlockVSU to return validator updates
  • ProviderValidatorUpdates is the important function to review
• x/ccv/no_valupdates_staking:
  • A wrapper for the staking module that does not return validator updates
• x/ccv/no_valupdates_genutil:
  • A wrapper for the genutil module that does not return validator updates
    • We need this here because the provider module uses the genutil module.
    • The genutil modules InitGenesis reads GenTXs and instantiates the initial validator set, see https://github.com/cosmos/cosmos-sdk/blob/0e6db148349f480e0309641f883fc690270eb491/x/genutil/gentx.go#L93
    • For an already-setup chain, the staking module already has its state set, so the InitGenesis from the genutil module doesn't appear relevant anymore
• app/provider/app.go:
  • The wiring was changed to wire the wrapper modules without valupdates in

---

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
• Targeted the correct branch (see PR Targeting)
• 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

Summary by CodeRabbit

• New Features

  • Introduced search functionality to the app.

• Bug Fixes

  • Corrected typos and updated error messages for better clarity.

• Refactor

  • Renamed ConsumerValidator to ConsensusValidator across the codebase.
  • Modified function signatures and updated imports for better alignment with new modules.

• Tests

  • Added new test scenarios for inactive validators.
  • Introduced new configurations for testing provider and consumer settings.

• Chores

  • Reordered import statements and removed unnecessary lines for better code organization.

[coderabbitai]

Walkthrough

Walkthrough

The changes predominantly involve updates, corrections, and new feature additions across various modules. There's a significant focus on renaming types from ConsumerValidator to ConsensusValidator, modifying import statements, and introducing new test configurations. Additionally, new functionality for initializing parameters and handling validator updates has been implemented, along with typo corrections and minor code improvements.

Changes

Files	Change Summaries
app/consumer-democracy/proposals_whitelisting.go	Corrected a typo in a comment.
app/provider/app.go	Modified import statements and updated AppModuleBasic declaration.
app/sovereign/export.go	Removed a line in prepForZeroHeightGenesis affecting commission withdrawal logic.
proto/interchain_security/ccv/provider/v1/...	Added new field last_provider_consensus_validators in GenesisState message.
proto/interchain_security/ccv/provider/v1/provider.proto	Renamed ConsumerValidator to ConsensusValidator and updated field names.
tests/e2e/config.go	Introduced InactiveProviderValsTestCfg and corresponding function.
tests/e2e/main.go	Added new entry to the stepChoices map.
tests/e2e/state.go	Updated GetReward function query and modified GetProposal variable declaration.
tests/e2e/steps_inactive_vals.go	Introduced test scenarios for validators validating on the consumer chain.
tests/integration/slashing.go	Replaced ConsumerValidator with ConsensusValidator.
testutil/keeper/mocks.go	Added new method GetBondedValidatorsByPower to MockStakingKeeper struct.
x/ccv/consumer/types/codec.go	Removed an empty line in the RegisterInterfaces function.
x/ccv/democracy/governance/module.go	Removed a line in the EndBlock function of AppModule.
x/ccv/no_valupdates_genutil/...	Introduced new wrapper around staking module with customized EndBlock function.
x/ccv/no_valupdates_staking/...	Added new AppModule struct with methods for module initialization and block processing.
x/ccv/provider/keeper/...	Corrected typos in error messages, reordered imports, updated InitGenesis to return validator updates, and adjusted field names.
x/ccv/provider/migrations/vX/...	Introduced InitializeMaxValidatorsForExistingConsumers function to set the maximum validators parameter.
x/ccv/provider/keeper/relay.go	Modified EndBlockVSU function to return validator updates and added ProviderValidatorUpdates function.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Validator
    participant ConsumerChain
    participant ProviderChain
    participant Module

    User->>Validator: Proposes block
    Validator->>ConsumerChain: Submits block
    ConsumerChain->>ProviderChain: Relays block info
    ProviderChain->>Module: Updates validator set
    Module->>ProviderChain: Returns validator updates
    ProviderChain->>ConsumerChain: Sends validator updates
    ConsumerChain->>Validator: Updates validator set
    Validator->>User: Confirms validation

Loading

---

Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 9c5028b and 60e9e7f.

Files selected for processing (1)

• x/ccv/provider/migrations/vX/migrations.go (1 hunks)

Additional context used

Path-based instructions (1)

x/ccv/provider/migrations/vX/migrations.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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.
  • @coderabbitai help me debug CodeRabbit configuration file.

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 an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @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.

[coderabbitai]

Actionable comments posted: 1

[coderabbitai]

Actionable comments posted: 1

[coderabbitai]

Ensure consistency in job naming and steps.

The new job inactive-provider-validators-on-consumer-test is consistent with other E2E test jobs. However, the step name E2E active set changes should be updated to reflect the actual test being run.

-      - name: E2E active set changes
+      - name: E2E inactive provider validators on consumer test

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	inactive-provider-validators-on-consumer-test:
	runs-on: ubuntu-latest
	timeout-minutes: 20
	steps:
	- uses: actions/setup-go@v5
	with:
	go-version: "1.22"
	- uses: actions/checkout@v4
	- name: Checkout LFS objects
	run: git lfs checkout
	- name: Setup Go
	uses: actions/setup-go@v5
	with:
	go-version: "1.22" # The Go version to download (if necessary) and use.
	- name: E2E active set changes
	run: go run ./tests/e2e/... --tc inactive-provider-validators-on-consumer
	inactive-provider-validators-on-consumer-test:
	runs-on: ubuntu-latest
	timeout-minutes: 20
	steps:
	- uses: actions/setup-go@v5
	with:
	go-version: "1.22"
	- uses: actions/checkout@v4
	- name: Checkout LFS objects
	run: git lfs checkout
	- name: Setup Go
	uses: actions/setup-go@v5
	with:
	go-version: "1.22" # The Go version to download (if necessary) and use.
	- name: E2E inactive provider validators on consumer test
	run: go run ./tests/e2e/... --tc inactive-provider-validators-on-consumer

[p-offtermatt]

Thanks rabbit

[coderabbitai]

@p-offtermatt, you're welcome! If you need any further assistance, feel free to ask.

[coderabbitai]

Actionable comments posted: 2

Outside diff range and nitpick comments (4)

x/ccv/provider/keeper/relay.go (4)

152-156: Document the logic for updating the provider consensus validator set.

The comments indicate the importance of updating the provider validator set before the rest of the logic. Ensure that this is well-documented and understood by maintainers to prevent future issues.

---

189-189: Clarify the purpose of currentValidators.

Add a comment to clarify the purpose of currentValidators and its role in the function.

// get the last validator set sent to consensus
currentValidators := k.GetLastProviderConsensusValSet(ctx)

---

193-196: Avoid potential out of range errors.

The code bounds maxValidators to the number of bonded validators to avoid out of range errors. Ensure this logic is correct and add a comment explaining it.

// avoid out of range errors by bounding the max validators to the number of bonded validators
if maxValidators > int64(len(bondedValidators)) {
    maxValidators = int64(len(bondedValidators))
}

---

209-211: Document the DiffValidators function.

Add a comment to explain what DiffValidators does to improve maintainability.

// calculate the difference between the current and next validator sets
valUpdates := DiffValidators(currentValidators, nextValidators)

[coderabbitai]

Actionable comments posted: 2

Outside diff range, codebase verification and nitpick comments (1)

x/ccv/provider/migrations/vX/migrations.go (1)

1-16: Add logging to track execution.

Adding logging statements will help track the execution of the function and debug any issues that may arise.

import (
	"log"
)

func InitializeMaxValidatorsForExistingConsumers(ctx sdk.Context, providerKeeper providerkeeper.Keeper) {
	log.Println("Initializing max validators for existing consumers")
	maxVals := providerKeeper.GetParams(ctx).MaxProviderConsensusValidators
	for _, chainID := range providerKeeper.GetAllRegisteredConsumerChainIDs(ctx) {
		log.Printf("Setting max validators for chainID: %s\n", chainID)
		providerKeeper.SetValidatorSetCap(ctx, chainID, uint32(maxVals))
	}
	log.Println("Finished initializing max validators for existing consumers")
}

[coderabbitai]

Handle potential errors from providerKeeper methods.

The providerKeeper methods may return errors that should be handled to ensure the function executes correctly.

func InitializeMaxValidatorsForExistingConsumers(ctx sdk.Context, providerKeeper providerkeeper.Keeper) error {
	maxVals, err := providerKeeper.GetParams(ctx).MaxProviderConsensusValidators
	if err != nil {
		return err
	}
	chainIDs, err := providerKeeper.GetAllRegisteredConsumerChainIDs(ctx)
	if err != nil {
		return err
	}
	for _, chainID := range chainIDs {
		if err := providerKeeper.SetValidatorSetCap(ctx, chainID, uint32(maxVals)); err != nil {
			return err
		}
	}
	return nil
}

---

Use a more descriptive function name.

The function name could be more descriptive to indicate that it sets the max validators for existing consumers.

func SetMaxValidatorsForExistingConsumers(ctx sdk.Context, providerKeeper providerkeeper.Keeper) error {
	maxVals, err := providerKeeper.GetParams(ctx).MaxProviderConsensusValidators
	if err != nil {
		return err
	}
	chainIDs, err := providerKeeper.GetAllRegisteredConsumerChainIDs(ctx)
	if err != nil {
		return err
	}
	for _, chainID := range chainIDs {
		if err := providerKeeper.SetValidatorSetCap(ctx, chainID, uint32(maxVals)); err != nil {
			return err
		}
	}
	return nil
}