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.

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

fix(rpc): validators endpoint fail during quorum rotation #959

Open
wants to merge 8 commits into
base: v1.3-dev
Choose a base branch
from

Conversation

lklimek
Copy link
Collaborator

@lklimek lklimek commented Oct 15, 2024

Issue being fixed or feature implemented

During quorum rotation, RPC requests to /validators fail with error:

{
  "code": -32603,
  "message": "Internal error",
  "data": "failed to create validator scoring strategy: could not initialize proposer: quorum hash mismatch at height 33226"
}

What was done?

  1. Fixed the issue
  2. Added logger to state store

How Has This Been Tested?

Modified e2e tests to chain halt on rotation and executed the /validators query.

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

  • New Features

    • Introduced enhanced logging capabilities for the state store, improving traceability during operations.
    • Added a new method to create a state store instance with a custom logger.
    • Implemented a new test case to validate loading validators during quorum rotations.
  • Bug Fixes

    • Improved error logging for proposer selection, providing better context during quorum rotations.
    • Enhanced error handling and messaging for loading validators and consensus parameters.
  • Documentation

    • Expanded comments for clarity in key methods related to proposer selection and state store initialization.

Copy link
Contributor

coderabbitai bot commented Oct 15, 2024

Walkthrough

The changes in this pull request involve substantial modifications to the dbStore struct and its methods, including the introduction of a new function for creating a state store with optional logging capabilities. The LoadValidators method has been redefined, and a new helper method, loadValidators, has been added to enhance the separation of concerns. Proposer selection logic has been improved, and error handling has been refined. Additionally, test cases have been updated for better organization and clarity.

Changes

File Path Change Summary
internal/state/store.go Introduced NewStoreWithLogger function for logger-enabled store creation; modified NewStore function signature; redefined LoadValidators for improved logging and proposer selection; added loadValidators helper function.
internal/state/store_test.go Enhanced mock block store functionality; added configureBlockMetaWithValidators helper function; introduced TestStoreLoadValidatorsOnRotation test case; updated TestStoreLoadValidators for clarity and improved error handling.

Poem

🐇 In the land of code where rabbits play,
New logs and checks brighten the day.
Proposers now dance with a clearer view,
As we hop along, our work shines anew!
With each little change, we leap with delight,
In the garden of code, everything feels right! 🌼


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>, please review it.
    • 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @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 using 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.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (4)
internal/consensus/versioned/selectproposer/height_round_proposer.go (2)

68-69: Approve comment addition with a minor suggestion.

The added comment provides valuable information about the method's precondition. This helps developers understand that s.valSet must be set correctly before calling this method.

Consider rephrasing the comment slightly for clarity:

-// Requires s.valSet to be set to correct validator set.
+// Requires s.valSet to be set to the correct validator set before calling this method.

90-98: Approve changes with a suggestion for error handling.

The modifications in this segment improve the code in several ways:

  1. Introduction of valSetHash enhances readability and potentially performance.
  2. The updated condition for quorum rotation detection is more explicit.
  3. Enhanced error logging provides more context for debugging.

These changes align well with the PR objective of fixing the validators endpoint failure during quorum rotation.

Consider wrapping the error returned at the end of this block with additional context:

-			return fmt.Errorf("quorum hash mismatch at height %d", height)
+			return fmt.Errorf("quorum hash mismatch at height %d: %w", height, 
+				types.ErrValidatorSetHashMismatch.Wrapf("got %X, expected %X", 
+					meta.Header.ValidatorsHash, valSetHash))

This change would provide even more context in the error message and use a specific error type, which could be helpful for error handling in calling functions.

internal/state/store.go (2)

686-686: Avoid logging sensitive validator information

The debug log at line 686:

store.logger.Debug("saving validator set", "height", height, "last_height_changed", lastHeightChanged, "validators", valSet)

includes the entire valSet, which may contain sensitive information. It's advisable to limit the logged information to non-sensitive data or ensure that detailed logging is only enabled in secure, non-production environments.

As an alternative, you could log only the size or hash of the validator set:

store.logger.Debug("saving validator set", "height", height, "last_height_changed", lastHeightChanged, "validator_set_size", len(valSet.Validators))

Line range hint 537-623: Potential for code duplication in proposer selection logic

The proposer selection logic has similar patterns in both the quorum rotation and non-rotation scenarios. Refactoring the common code into helper functions could improve readability and reduce duplication.

Consider extracting the proposer setting and strategy initialization into separate functions.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 525544e and 3b70797.

📒 Files selected for processing (4)
  • internal/consensus/versioned/selectproposer/height_round_proposer.go (2 hunks)
  • internal/inspect/inspect.go (1 hunks)
  • internal/state/store.go (5 hunks)
  • node/node.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
internal/inspect/inspect.go (1)

80-80: LGTM! Consider adding a debug log statement.

The addition of logging capability to the state store is a good improvement. It aligns well with the PR objectives and should help with debugging, especially during quorum rotation.

Consider adding a debug log statement immediately after this line to confirm the successful creation of the state store with logging enabled. This could help in troubleshooting if issues arise. For example:

ss := state.NewStoreWithLogger(sDB, logger.With("module", "state_store"))
logger.Debug("State store created with logging enabled")

To ensure the NewStoreWithLogger function exists and is used correctly, please run the following script:

✅ Verification successful

Verified! The NewStoreWithLogger function exists and is correctly utilized in internal/inspect/inspect.go. No further action is required.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the existence and usage of NewStoreWithLogger function
ast-grep --lang go --pattern 'func NewStoreWithLogger($_, $_, $_) $_'
rg --type go 'NewStoreWithLogger\('

Length of output: 395

internal/consensus/versioned/selectproposer/height_round_proposer.go (1)

Line range hint 1-238: Summary: Changes improve error handling and align with PR objectives.

The modifications in this file, particularly in the proposerFromStore method, enhance error handling and logging during the proposer selection process. These changes align well with the PR objectives and should address the issue of validators endpoint failure during quorum rotation.

To ensure the changes work as intended, consider running the following verification script:

This script will help identify any areas that might need additional attention or similar updates.

✅ Verification successful

Verified: The changes in height_round_proposer.go effectively improve error handling and logging as intended, aligning with the PR objectives. No issues were found based on the executed verification scripts.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the changes in proposerFromStore method are used correctly throughout the codebase.

# Test 1: Check for any direct calls to s.valSet.Hash() that might have been missed
echo "Checking for direct calls to s.valSet.Hash():"
rg --type go 's\.valSet\.Hash\(\)'

# Test 2: Verify that error logging includes the new context information
echo "Verifying error logging includes new context:"
rg --type go 'Error\("quorum rotation detected but not expected"'

# Test 3: Check for any other uses of ValidatorsHash that might need similar treatment
echo "Checking other uses of ValidatorsHash:"
rg --type go 'ValidatorsHash'

echo "Verification complete. Please review the output to ensure all relevant code paths have been updated."

Length of output: 14641

node/node.go (1)

139-139: Improved logging for state store: LGTM!

This change enhances the observability of the state store by incorporating a logger. The use of NewStoreWithLogger instead of NewStore allows for more detailed logging, which can be valuable for debugging and monitoring purposes. The logger is appropriately tagged with the module name "state_store", facilitating easier log filtering and analysis.

internal/state/store.go (3)

Line range hint 506-531: Ensure error handling covers all edge cases in loadValidators

The new helper function loadValidators improves code readability by separating the logic for loading validators. However, please ensure that all possible error scenarios are appropriately handled. In particular, consider what happens if lastStoredHeight is less than or equal to zero, or if loadValidatorsInfo fails to retrieve the validator set at lastStoredHeight.

To verify the robustness of error handling, you might run tests with edge case inputs where the validator set might be missing or corrupted.


537-542: Addition of debug logging enhances traceability

The added debug statement store.logger.Debug("Loading validators", "height", height) in the LoadValidators method is beneficial for tracking the flow of execution and diagnosing issues related to validator loading.


584-607: Verify proposer selection logic during quorum rotation

In the quorum rotation scenario, the code selects the first validator in the set as the proposer:

if !prevMeta.Header.ValidatorsHash.Equal(prevMeta.Header.NextValidatorsHash) {
    // rotation happened - we select 1st validator as proposer
    // ...
}

Please verify that this logic aligns with the consensus protocol's specifications. Selecting the first validator might not distribute proposer responsibilities fairly, especially in a network with frequent quorum changes.

To ensure the proposer selection is fair and follows the intended protocol, consider reviewing the consensus algorithm or consult the protocol documentation.

internal/state/store.go Outdated Show resolved Hide resolved
internal/state/store.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (3)
internal/consensus/versioned/selectproposer/height_proposer.go (1)

93-93: Approved: Improved error message clarity

The change from "quorum hash mismatch" to "validator set hash mismatch" in the error message is more accurate and aligns better with the actual comparison being made. This improvement enhances the clarity of the error without altering the functionality, which addresses the PR objective of fixing the /validators endpoint failure during quorum rotation.

For consistency, consider updating the error logging message above (lines 85-90) to also use "validator set" terminology instead of "quorum". This would ensure that all related error messages use consistent terminology.

internal/state/store.go (2)

Line range hint 532-611: Improved LoadValidators method, but consider refactoring for readability

The changes to the LoadValidators method appear to address the issue mentioned in the PR objectives regarding the failure of RPC requests to the /validators endpoint during quorum rotation. The new implementation is more robust, with improved error handling and logging. However, the complexity of the method has increased significantly.

While the changes are approved, consider the following suggestions to improve readability and maintainability:

  1. Extract the proposer selection logic into a separate method to reduce the overall complexity of LoadValidators.
  2. Add more inline comments explaining the different scenarios and why certain actions are taken.
  3. Consider breaking down some of the longer code blocks into smaller, well-named helper functions.

Would you like assistance in refactoring this method for improved readability?


Issues Found: Inconsistent Error Handling for LoadValidators

The verification of the LoadValidators method usage revealed inconsistencies in error handling across the codebase:

  1. Proper Error Handling:

    • In test files (e.g., state_test.go), calls to LoadValidators consistently check and handle errors appropriately.
  2. Missing Error Handling:

    • In main code files such as internal/state/store.go, internal/state/reactor.go, and internal/state/rollback.go, there are instances where LoadValidators is called without immediate error handling.

Recommendations:

  • Uniform Error Handling: Ensure that every invocation of LoadValidators in the production codebase includes proper error checking and handling to prevent potential crashes or undefined behaviors.
  • Code Review: Re-examine the sections of the code where error handling is missing to implement necessary safeguards.
  • Testing: Update existing tests or add new ones to cover scenarios where LoadValidators might fail, ensuring that the application responds gracefully to such failures.

By addressing these inconsistencies, the reliability and robustness of the application can be significantly enhanced.

🔗 Analysis chain

Line range hint 1-611: Overall assessment: Approved with suggestions for improvement

The changes in this file successfully address the main issue described in the PR objectives, particularly the failure of RPC requests to the /validators endpoint during quorum rotation. The modifications to the NewStore function and the LoadValidators method demonstrate good progress in resolving the problem and improving the overall functionality.

However, there are a few points that need attention:

  1. The inconsistency between the AI-generated summary and the actual code changes regarding the NewStoreWithLogger function needs to be resolved.
  2. While the LoadValidators method improvements are approved, consider refactoring it for better readability and maintainability as suggested in the previous comment.
  3. Ensure that all the changes are thoroughly tested, especially the new proposer selection logic in the LoadValidators method.

Overall, the changes are approved, but addressing these points would further enhance the quality and reliability of the code.

To ensure the changes are working as expected, consider running the following verification script:

This script will help identify how the LoadValidators method is being used throughout the codebase and ensure that error handling is implemented correctly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that the LoadValidators method is called correctly and handles errors properly

# Search for calls to LoadValidators
echo "Searching for calls to LoadValidators:"
rg --type go 'LoadValidators\(' -A 5 -B 5

# Search for error handling related to LoadValidators
echo "Searching for error handling related to LoadValidators:"
rg --type go 'err\s*:=.*LoadValidators\(' -A 5

Length of output: 27167

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3b70797 and 6551596.

📒 Files selected for processing (7)
  • internal/consensus/versioned/selectproposer/height_proposer.go (1 hunks)
  • internal/consensus/versioned/selectproposer/height_round_proposer.go (2 hunks)
  • internal/consensus/versioned/selectproposer/proposer_selector.go (0 hunks)
  • internal/inspect/inspect.go (1 hunks)
  • internal/state/store.go (1 hunks)
  • internal/state/store_test.go (3 hunks)
  • node/node.go (1 hunks)
💤 Files with no reviewable changes (1)
  • internal/consensus/versioned/selectproposer/proposer_selector.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • internal/consensus/versioned/selectproposer/height_round_proposer.go
  • internal/inspect/inspect.go
  • node/node.go
🧰 Additional context used
🔇 Additional comments (4)
internal/state/store.go (1)

125-130: Improved flexibility in NewStore function

The changes to the NewStore function enhance its flexibility by allowing an optional logger parameter. This is a good improvement as it enables users to provide a custom logger when needed, while still defaulting to a noop logger if none is provided. The implementation correctly handles edge cases, such as when an empty or nil logger is passed.

internal/state/store_test.go (3)

36-39: Refactored code improves modularity and readability

The extraction of configureBlockMetaWithValidators enhances code reusability by centralizing block metadata configuration.


115-116: Consistent use of helper function enhances maintainability

Utilizing configureBlockMetaWithValidators in TestStoreLoadValidators promotes consistency and reduces code duplication.


247-250: Clarify assignment of LastHeightConsensusParamsChanged

Assigning state.LastHeightConsensusParamsChanged = h + 1 may not reflect the intended consensus parameter change height.

Please ensure that h + 1 correctly represents the height at which consensus parameters changed. If the change occurs at height h, consider setting:

state.LastHeightConsensusParamsChanged = h

If necessary, verify the correctness across the codebase:

✅ Verification successful

Assignment of LastHeightConsensusParamsChanged is consistent and correct within the test context.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify assignments to LastHeightConsensusParamsChanged

# Search for assignments and display context
rg --type go -A 2 'LastHeightConsensusParamsChanged\s*='

Length of output: 2361

internal/state/store_test.go Outdated Show resolved Hide resolved
internal/state/store_test.go Outdated Show resolved Hide resolved
internal/state/store_test.go Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
internal/state/store.go (2)

Line range hint 507-530: Consider consistent error handling

The loadValidators method is well-structured and includes appropriate error handling and logging. However, there's an opportunity to improve consistency in error handling.

Consider wrapping all errors with ErrNoValSetForHeight for consistency. This would make error handling in the calling functions more uniform.

Example refactor:

 func (store dbStore) loadValidators(height int64) (*tmstate.ValidatorsInfo, error) {
 	valInfo, err := loadValidatorsInfo(store.db, height)
 	if err != nil {
-		return nil, ErrNoValSetForHeight{Height: height, Err: err}
+		return nil, ErrNoValSetForHeight{Height: height, Err: fmt.Errorf("failed to load validators info: %w", err)}
 	}

 	if valInfo.ValidatorSet == nil {
 		lastStoredHeight := lastStoredHeightFor(height, valInfo.LastHeightChanged)
 		store.logger.Debug("Validator set is nil, loading last stored height", "height", height, "last_height_changed", valInfo.LastHeightChanged, "last_stored_height", lastStoredHeight)
 		valInfo, err = loadValidatorsInfo(store.db, lastStoredHeight)
 		if err != nil || valInfo.ValidatorSet == nil {
-			return nil,
-				fmt.Errorf("couldn't find validators at height %d (height %d was originally requested): %w",
+			return nil, ErrNoValSetForHeight{Height: height, Err: fmt.Errorf("couldn't find validators at height %d (height %d was originally requested): %w",
 					lastStoredHeight,
 					height,
 					err,
-				)
+				)}
 		}
 	}

 	return valInfo, nil
 }

Line range hint 532-622: Consider refactoring for improved readability

The changes to the LoadValidators method significantly improve the proposer selection logic, especially during quorum rotation. The added logging enhances debuggability. However, the method has become quite complex and could benefit from refactoring for improved readability and maintainability.

Consider extracting the proposer selection logic into separate methods, one for handling quorum rotation and another for normal proposer advancement. This would make the main method easier to follow and the logic easier to test independently.

Here's a suggested refactor outline:

func (store dbStore) LoadValidators(height int64, bs selectproposer.BlockStore) (*types.ValidatorSet, error) {
    // ... (existing code for loading validators and consensus params)

    if meta := bs.LoadBlockMeta(height); meta != nil {
        return store.handleExistingBlockMeta(meta, valSet, cp)
    }

    return store.handleMissingBlockMeta(height, valSet, cp, bs)
}

func (store dbStore) handleExistingBlockMeta(meta *types.BlockMeta, valSet *types.ValidatorSet, cp types.ConsensusParams) (*types.ValidatorSet, error) {
    // ... (logic for when we have the block meta)
}

func (store dbStore) handleMissingBlockMeta(height int64, valSet *types.ValidatorSet, cp types.ConsensusParams, bs selectproposer.BlockStore) (*types.ValidatorSet, error) {
    // ... (logic for when we don't have the block meta)
}

This refactoring would make the main LoadValidators method more concise and easier to understand at a glance, while still maintaining the improved logic for proposer selection.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 6551596 and f5db91d.

📒 Files selected for processing (1)
  • internal/state/store.go (5 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/state/store.go (1)
Learnt from: lklimek
PR: dashpay/tenderdash#959
File: internal/state/store.go:0-0
Timestamp: 2024-10-15T13:59:42.522Z
Learning: In `internal/state/store.go`, when addressing logging inconsistencies in the proposer selection logic, the team prefers to remove existing logs to maintain consistency.
🔇 Additional comments (2)
internal/state/store.go (2)

125-130: LGTM: Improved flexibility in logger initialization

The changes to the NewStore function enhance its flexibility by allowing an optional logger to be passed. If no logger is provided, it defaults to a no-op logger. This is a good practice that improves logging capabilities without breaking existing usage.


685-685: LGTM: Improved logging

The addition of debug logging statements throughout the file enhances the overall logging capabilities of the package. This is beneficial for debugging and monitoring purposes.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
internal/state/store.go (1)

Line range hint 531-621: Excellent refactoring of proposer selection logic

The changes to the LoadValidators method significantly improve the proposer selection process:

  1. The handling of quorum rotation scenarios is now more robust.
  2. Error handling and logging additions enhance debuggability.
  3. The overall structure of the method is clearer and easier to follow.

These improvements will make the code more maintainable and reliable.

Minor optimization suggestion:

In the else block (lines 605-621), consider moving the common logic for creating the proposer selector strategy outside the if-else statement to reduce code duplication.

Here's a suggested refactoring:

 	} else {
-		// set proposer from previous block
-		if err := valSet.SetProposer(prevMeta.Header.ProposerProTxHash); err != nil {
-			return nil, fmt.Errorf("could not set proposer: %w", err)
-		}
-		strategy, err := selectproposer.NewProposerSelector(cp, valSet, prevMeta.Header.Height, prevMeta.Round, bs, store.logger)
-		if err != nil {
-			return nil, fmt.Errorf("failed to create validator scoring strategy: %w", err)
-		}
-
-		// now, advance to (height,0)
-		if err := strategy.UpdateHeightRound(height, 0); err != nil {
-			return nil, fmt.Errorf("failed to update validator scores at height %d, round 0: %w", height, err)
-		}
-
-		return strategy.ValidatorSet(), nil
+		// set proposer from previous block
+		if err := valSet.SetProposer(prevMeta.Header.ProposerProTxHash); err != nil {
+			return nil, fmt.Errorf("could not set proposer: %w", err)
+		}
 	}
+
+	strategy, err := selectproposer.NewProposerSelector(cp, valSet, prevMeta.Header.Height, prevMeta.Round, bs, store.logger)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create validator scoring strategy: %w", err)
+	}
+
+	// now, advance to (height,0)
+	if err := strategy.UpdateHeightRound(height, 0); err != nil {
+		return nil, fmt.Errorf("failed to update validator scores at height %d, round 0: %w", height, err)
+	}
+
+	return strategy.ValidatorSet(), nil
 }

This change reduces code duplication and makes the logic flow more consistent.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between f5db91d and ada43a1.

📒 Files selected for processing (2)
  • internal/state/store.go (4 hunks)
  • internal/state/store_test.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (12)
internal/state/store_test.go (7)

36-39: LGTM: Improved code organization

The use of configureBlockMetaWithValidators enhances readability and maintainability by extracting the block store configuration logic into a separate function.


Line range hint 41-57: LGTM: Well-structured helper function

The new configureBlockMetaWithValidators function effectively encapsulates the logic for configuring block metadata with validators, improving code reusability and clarity.


41-41: Fix typographical error in comment

There's an extra forward slash at the beginning of the comment.

Please update the comment from:

// / configureBlockMetaWithValidators configures the block store to return proposers based on the height.

to:

// configureBlockMetaWithValidators configures the block store to return proposers based on the height.

36-36: LGTM: Consistent use of new helper function

The test case has been updated to use the new configureBlockMetaWithValidators function, maintaining consistency with the recent changes and improving readability.


176-269: LGTM: Comprehensive test for validator loading during rotation

This new test function is well-structured and covers various scenarios:

  1. It uses table-driven tests to cover multiple combinations of rotations, validator counts, and consensus versions.
  2. The setup of state and block stores is thorough and accurately simulates the rotation scenarios.
  3. The test verifies the correct loading of validators during rotation, which is a critical functionality.

The comprehensive nature of this test will help ensure the robustness of the validator loading mechanism during quorum rotations.


176-178: Fix typographical error in test comments

There are extra forward slashes at the beginning of the test description comments.

Please update the comments from:

// / Given a set of blocks in the block store and two validator sets that rotate,
// / When we load the validators during quorum rotation,
// / Then we receive the correct validators for each height.

to:

// Given a set of blocks in the block store and two validator sets that rotate,
// When we load the validators during quorum rotation,
// Then we receive the correct validators for each height.

260-261: Fix variable shadowing of err

At line 261, err is redeclared using := within a scope where err is already defined. This can lead to confusion and potential bugs in error handling.

To prevent this, modify the code to assign to the existing err variable:

- err := stateStore.SaveValidatorSets(uncommittedHeight, uncommittedHeight, expectedValidators)
+ err = stateStore.SaveValidatorSets(uncommittedHeight, uncommittedHeight, expectedValidators)
internal/state/store.go (5)

125-130: Excellent improvement in logger handling!

The changes to the NewStore function effectively address the previous refactor suggestion. The implementation now allows for flexible logger configuration while maintaining backward compatibility. This is a good example of improving code without introducing breaking changes.


507-529: Well-structured helper method with improved error handling

The new loadValidators method is a great addition:

  1. It improves code organization by separating the validator loading logic.
  2. The error handling is comprehensive, providing detailed error messages.
  3. The added logging will be helpful for debugging purposes.

These changes enhance the maintainability and debuggability of the code.


684-684: Helpful debug logging added

The addition of this debug log statement is a good improvement. It will provide valuable information during troubleshooting and debugging sessions, making it easier to track the state of validator sets at different heights.


512-524: Improved error handling and messaging

The enhancements to error handling throughout the file are commendable:

  1. Error messages are more detailed and provide better context.
  2. New error types (e.g., ErrNoValSetForHeight) have been introduced for more specific error handling.
  3. The additional context in error messages (such as including both the requested height and the actual height where an issue occurred) will greatly aid in debugging and troubleshooting.

These improvements will make it easier to diagnose and resolve issues in production environments.

Also applies to: 539-540, 593-598


Line range hint 1-764: Excellent overall improvements to the state store implementation

The changes made to this file represent a significant improvement to the state store implementation:

  1. The addition of the loadValidators helper method improves code organization and reusability.
  2. The refactoring of the LoadValidators method enhances the proposer selection logic, particularly in handling quorum rotation scenarios.
  3. Improved error handling and more informative error messages throughout the file will aid in debugging and troubleshooting.
  4. The addition of debug logging statements provides valuable information for tracking the state at various points.
  5. The changes are consistent with the rest of the codebase and follow good coding practices.

These improvements enhance the maintainability, debuggability, and overall robustness of the state store implementation. Great work on this refactoring effort!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
internal/state/store_test.go (1)

Line range hint 41-57: LGTM: Well-structured helper function

The new configureBlockMetaWithValidators function effectively centralizes the logic for configuring the block store, improving code organization and reusability.

Consider removing the extra forward slash at the beginning of the function comment:

-// / configureBlockMetaWithValidators configures the block store to return proposers based on the height.
+// configureBlockMetaWithValidators configures the block store to return proposers based on the height.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ada43a1 and cbe1a76.

📒 Files selected for processing (1)
  • internal/state/store_test.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
internal/state/store_test.go (2)

36-39: LGTM: Improved code organization

The extraction of logic into configureBlockMetaWithValidators enhances readability and maintainability.


150-152: LGTM: Improved error handling

The enhanced error message provides more context about the failure, which is beneficial for debugging and understanding test outcomes.

internal/state/store_test.go Show resolved Hide resolved
@@ -122,8 +122,12 @@ type dbStore struct {
var _ Store = (*dbStore)(nil)

// NewStore creates the dbStore of the state pkg.
func NewStore(db dbm.DB) Store {
return dbStore{db, log.NewNopLogger()}
func NewStore(db dbm.DB, logger ...log.Logger) Store {
Copy link
Member

Choose a reason for hiding this comment

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

Please explain with a comment in the code what is happening here

@shumkov shumkov linked an issue Oct 17, 2024 that may be closed by this pull request
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Oct 27, 2024
@github-actions github-actions bot removed the Stale label Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tenderdash validators request error
2 participants