chore!: only distribute rewards to validators that have been validating a consumer chain for some backport #1929

[sainoe]

[should be merged after #1982]

backport 1929.

Summary by CodeRabbit

• Tests

  • Updated reward distribution logic in test cases to accommodate new block height criteria and context adjustments.
  • Added tests for allocating tokens to consumer validators with varying block heights.

• New Features

  • Introduced eligibility criteria for consumer rewards based on validator duration.

• Refactor

  • Enhanced parameter handling and validations in migration test functions.

[coderabbitai]

Walkthrough

Walkthrough

The changes primarily focus on the reward distribution logic for consumer validators within a blockchain system. This includes making validators eligible for rewards based on block height adjustments and context updates, enhancing methods for reward allocation, and adding tests to verify these functionalities. Parameter checks and migrations in tests were also updated to support these changes.

Changes

File Path	Change Summary
tests/integration/distribution.go	Adjusted block height and context for reward eligibility and distribution among validators.
x/ccv/provider/keeper/distribution.go	Implemented IsEligibleForConsumerRewards function to determine reward eligibility of validators based on validation duration.
x/ccv/provider/migrations/v6/migration_test.go	Modified test to check parameter values before and after migration, removed t.Helper(), and updated parameter handling.

---

Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f7f38b3 and a787cca.

Files selected for processing (1)

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

Files skipped from review as they are similar to previous changes (1)

• x/ccv/provider/migrations/v6/migration_test.go

---

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: 4

[coderabbitai]

Ensure the correct handling of multiple errors in CreateConsumerValidator.

Multiple error checks are performed without handling the possibility of an error in each step. It's crucial to handle each potential error immediately after it occurs to avoid misbehavior or incorrect data processing.

if err != nil {
-	return types.ConsumerValidator{}, err
+	return types.ConsumerValidator{}, fmt.Errorf("could not retrieve validator's (%+v) consensus address: %w", validator, err)
}

Committable suggestion was skipped due to low confidence.

[coderabbitai]

Consider handling errors gracefully instead of using panic in GetConsumerValidator.

Using panic can lead to chain halts if not handled properly, especially in consensus-related methods. Consider returning an error and handling it appropriately in the calling function.

- panic(fmt.Errorf("failed to unmarshal ConsumerValidator: %w", err))
+ return types.ConsumerValidator{}, fmt.Errorf("failed to unmarshal ConsumerValidator: %w", err)

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

	// GetConsumerValidator returns the consumer validator with `providerAddr` if it exists for chain `chainID`
	func (k Keeper) GetConsumerValidator(ctx sdk.Context, chainID string, providerAddr types.ProviderConsAddress) (types.ConsumerValidator, bool) {
	store := ctx.KVStore(k.storeKey)
	marshalledConsumerValidator := store.Get(types.ConsumerValidatorKey(chainID, providerAddr.ToSdkConsAddr()))
	if marshalledConsumerValidator == nil {
	return types.ConsumerValidator{}, false
	}
	var validator types.ConsumerValidator
	if err := validator.Unmarshal(marshalledConsumerValidator); err != nil {
	panic(fmt.Errorf("failed to unmarshal ConsumerValidator: %w", err))
	}
	return validator, true
	}
	// GetConsumerValidator returns the consumer validator with `providerAddr` if it exists for chain `chainID`
	func (k Keeper) GetConsumerValidator(ctx sdk.Context, chainID string, providerAddr types.ProviderConsAddress) (types.ConsumerValidator, bool, error) {
	store := ctx.KVStore(k.storeKey)
	marshalledConsumerValidator := store.Get(types.ConsumerValidatorKey(chainID, providerAddr.ToSdkConsAddr()))
	if marshalledConsumerValidator == nil {
	return types.ConsumerValidator{}, false, nil
	}
	var validator types.ConsumerValidator
	if err := validator.Unmarshal(marshalledConsumerValidator); err != nil {
	return types.ConsumerValidator{}, false, fmt.Errorf("failed to unmarshal ConsumerValidator: %w", err)
	}
	return validator, true, nil
	}

Tools

GitHub Check: CodeQL

[warning] 87-87: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt

[coderabbitai]

Improve heading formatting

Ensure that headings are surrounded by blank lines to comply with markdown best practices and enhance readability.

+ 
## Withdrawing rewards

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

	## Withdrawing rewards
	## Withdrawing rewards

Tools

Markdownlint

15-15: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines

[coderabbitai]

Tip

Codebase Verification

The rg output indicates that there are comments regarding decimals and remaining decimals, but there are no explicit tests that handle edge conditions related to decimal remainders.

Summary:

• The code in tests/integration/distribution.go verifies the handling of decimal remainders.
• There are no explicit tests for edge conditions such as very small decimal remainders, very large numbers, or zero remainders.

Conclusion:

The current tests do not explicitly cover edge conditions for decimal remainders. It is important to add tests that handle these scenarios to ensure financial accuracy under all conditions.

Actionable Directive:

• Add explicit tests for edge conditions: Ensure that the test cases in tests/integration/distribution.go include scenarios with very small decimal remainders, very large numbers, and zero remainders.

Locations to update:

• tests/integration/distribution.go

Category:

Analysis chain

Check logic for handling decimal remainders in reward distribution.

The code on lines 160-163 handles the decimal remainders from the distribution calculations, ensuring they remain in the consumer allocations. This is crucial for financial accuracy. Ensure that the logic for handling these remainders is thoroughly tested, especially under various edge conditions.

---

The review comment asks to verify the logic for handling decimal remainders in reward distribution and ensure that this logic is thoroughly tested, especially under various edge conditions. The rg command output shows the relevant lines of code and some context around them.

Steps to take:

1. Identify the lines of code that handle decimal remainders:

   • The lines of interest are:

     rewardsAlloc := providerKeeper.GetConsumerRewardsAllocation(s.providerCtx(), s.consumerChain.ChainID)
     remainingAlloc := rewardsAlloc.Rewards.AmountOf(rewardsIBCdenom)
     s.Require().True(remainingAlloc.LTE(math.LegacyOneDec()))

2. Verify the presence of tests that cover these lines of code:

   • The rg command output shows that these lines are part of a test in tests/integration/distribution.go.

3. Ensure that these tests cover various edge conditions:

   • We need to check for tests that specifically cover edge conditions related to decimal remainders in reward distribution.

To proceed, we should look for other instances where remainingAlloc is used and ensure that edge cases are tested. We can use ast-grep to find all occurrences of remainingAlloc and check if they are part of comprehensive test cases.

---

The ast-grep output confirms that remainingAlloc is only used in tests/integration/distribution.go at lines 162, 163, and 171. This suggests that the handling of decimal remainders is indeed tested in this file. However, to ensure that edge conditions are properly covered, we need to examine the tests in more detail.

Steps to take:

1. Examine the test cases in tests/integration/distribution.go:

   • We need to check the surrounding code to understand the context of these tests and ensure edge cases are covered.

2. Identify edge conditions:

   • Examples of edge conditions might include scenarios with very small decimal remainders, very large numbers, or zero remainders.

To proceed, we should examine the content of tests/integration/distribution.go around the identified lines to understand the test cases and ensure they cover edge conditions.

---

The sed output provides more context around the test cases in tests/integration/distribution.go. The code verifies that the decimal remainders from the distribution are handled correctly and that the remaining allocation is less than or equal to one decimal unit. Additionally, it checks the reward pool and distribution module account balances.

Summary of the test cases:

1. Decimal Remainders:
   • Ensures that remainingAlloc is less than or equal to one decimal unit.
2. Reward Pool Balance:
   • Verifies that the reward pool holds the correct amount, including the remaining decimals.
3. Distribution Module Account Balance:
   • Checks that the distribution module account balance matches the expected consumer rewards.

Next Steps:

1. Check for edge conditions:
   • Ensure that the tests cover scenarios with very small decimal remainders, very large numbers, and zero remainders.

To confirm the presence of edge condition tests, we should search for test cases that explicitly handle these scenarios.

Scripts executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Description: Verify the handling of decimal remainders in reward distribution.
# Expect: Proper tests covering edge conditions.
rg --type go 'remainingAlloc' --context 5

Length of output: 1856

---

---

Script:

#!/bin/bash
# Find all occurrences of `remainingAlloc` in the codebase
ast-grep --lang go --pattern 'remainingAlloc' 

Length of output: 364

---

---

Script:

#!/bin/bash
# Display the content of tests/integration/distribution.go around the identified lines
sed -n '150,180p' tests/integration/distribution.go

Length of output: 1526

---

---

Script:

#!/bin/bash
# Search for test cases that handle edge conditions related to decimal remainders in tests/integration/distribution.go
rg --type go 'decimal|remainder|edge' tests/integration/distribution.go

Length of output: 232

[coderabbitai]

Actionable comments posted: 3

[coderabbitai]

Optimize block height adjustment logic

The logic for increasing the block height to make validators eligible for consumer rewards is repetitive across different test cases. Consider refactoring this into a utility function to avoid code duplication and improve maintainability.

- ctx = ctx.WithBlockHeight(providerKeeper.GetNumberOfEpochsToStartReceivingRewards(ctx)*providerKeeper.GetBlocksPerEpoch(ctx) + ctx.BlockHeight())
+ ctx = increaseBlockHeightForRewards(ctx, providerKeeper)

And add a new function:

func increaseBlockHeightForRewards(ctx sdk.Context, keeper ProviderKeeper) sdk.Context {
    numberOfBlocks := keeper.GetNumberOfEpochsToStartReceivingRewards(ctx) * keeper.GetBlocksPerEpoch(ctx)
    return ctx.WithBlockHeight(numberOfBlocks + ctx.BlockHeight())
}

[coderabbitai]

Optimize the reward allocation loop by checking eligibility before other operations.

In the AllocateTokensToConsumerValidators function, the eligibility check for each validator (IsEligibleForConsumerRewards) is performed within the loop after other operations have started. Consider moving this check to the beginning of the loop to avoid unnecessary computations for ineligible validators.

for _, consumerVal := range k.GetConsumerValSet(ctx, chainID) {
+   if !k.IsEligibleForConsumerRewards(ctx, consumerVal.JoinHeight) {
+       continue
+   }
    consAddr := sdk.ConsAddress(consumerVal.ProviderConsAddr)
    // other operations...
}

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

	// if a validator is not eligible, this means that the other eligible validators would get more rewards
	if !k.IsEligibleForConsumerRewards(ctx, consumerVal.JoinHeight) {
	continue
	}
	for _, consumerVal := range k.GetConsumerValSet(ctx, chainID) {
	if !k.IsEligibleForConsumerRewards(ctx, consumerVal.JoinHeight) {
	continue
	}
	consAddr := sdk.ConsAddress(consumerVal.ProviderConsAddr)
	// other operations...
	}

[coderabbitai]

Refactor the voting power computation to improve efficiency.

In the ComputeConsumerTotalVotingPower function, the eligibility check for each validator is done inside the loop. This can be optimized by filtering the validators list first to include only those eligible for rewards, thereby reducing the number of iterations.

- for _, v := range k.GetConsumerValSet(ctx, chainID) {
-     if !k.IsEligibleForConsumerRewards(ctx, v.JoinHeight) {
-         continue
-     }
-     totalPower += v.Power
- }
+ eligibleValidators := filterEligibleValidators(k.GetConsumerValSet(ctx, chainID), ctx)
+ for _, v := eligibleValidators {
+     totalPower += v.Power
+ }

Committable suggestion was skipped due to low confidence.

[insumity]

LGTM