-
Notifications
You must be signed in to change notification settings - Fork 14
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
base: v1.3-dev
Are you sure you want to change the base?
Conversation
WalkthroughThe changes in this pull request involve substantial modifications to the Changes
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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:
- Introduction of
valSetHash
enhances readability and potentially performance.- The updated condition for quorum rotation detection is more explicit.
- 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 informationThe 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 logicThe 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
📒 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 ininternal/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 ofNewStore
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 inloadValidators
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 iflastStoredHeight
is less than or equal to zero, or ifloadValidatorsInfo
fails to retrieve the validator set atlastStoredHeight
.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 traceabilityThe added debug statement
store.logger.Debug("Loading validators", "height", height)
in theLoadValidators
method is beneficial for tracking the flow of execution and diagnosing issues related to validator loading.
584-607
: Verify proposer selection logic during quorum rotationIn 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.
There was a problem hiding this 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 clarityThe 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
: ImprovedLoadValidators
method, but consider refactoring for readabilityThe 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:
- Extract the proposer selection logic into a separate method to reduce the overall complexity of
LoadValidators
.- Add more inline comments explaining the different scenarios and why certain actions are taken.
- 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:
Proper Error Handling:
- In test files (e.g.,
state_test.go
), calls toLoadValidators
consistently check and handle errors appropriately.Missing Error Handling:
- In main code files such as
internal/state/store.go
,internal/state/reactor.go
, andinternal/state/rollback.go
, there are instances whereLoadValidators
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 improvementThe 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 theNewStore
function and theLoadValidators
method demonstrate good progress in resolving the problem and improving the overall functionality.However, there are a few points that need attention:
- The inconsistency between the AI-generated summary and the actual code changes regarding the
NewStoreWithLogger
function needs to be resolved.- While the
LoadValidators
method improvements are approved, consider refactoring it for better readability and maintainability as suggested in the previous comment.- 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 5Length of output: 27167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 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 inNewStore
functionThe 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 readabilityThe extraction of
configureBlockMetaWithValidators
enhances code reusability by centralizing block metadata configuration.
115-116
: Consistent use of helper function enhances maintainabilityUtilizing
configureBlockMetaWithValidators
inTestStoreLoadValidators
promotes consistency and reduces code duplication.
247-250
: Clarify assignment ofLastHeightConsensusParamsChanged
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 heighth
, consider setting:state.LastHeightConsensusParamsChanged = hIf 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
There was a problem hiding this 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 handlingThe
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 readabilityThe 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
📒 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 initializationThe 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 loggingThe addition of debug logging statements throughout the file enhances the overall logging capabilities of the package. This is beneficial for debugging and monitoring purposes.
There was a problem hiding this 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 logicThe changes to the
LoadValidators
method significantly improve the proposer selection process:
- The handling of quorum rotation scenarios is now more robust.
- Error handling and logging additions enhance debuggability.
- 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 theif-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
📒 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 organizationThe 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 functionThe new
configureBlockMetaWithValidators
function effectively encapsulates the logic for configuring block metadata with validators, improving code reusability and clarity.
41-41
: Fix typographical error in commentThere'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 functionThe 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 rotationThis new test function is well-structured and covers various scenarios:
- It uses table-driven tests to cover multiple combinations of rotations, validator counts, and consensus versions.
- The setup of state and block stores is thorough and accurately simulates the rotation scenarios.
- 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 commentsThere 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 oferr
At line 261,
err
is redeclared using:=
within a scope whereerr
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 handlingThe new
loadValidators
method is a great addition:
- It improves code organization by separating the validator loading logic.
- The error handling is comprehensive, providing detailed error messages.
- The added logging will be helpful for debugging purposes.
These changes enhance the maintainability and debuggability of the code.
684-684
: Helpful debug logging addedThe 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 messagingThe enhancements to error handling throughout the file are commendable:
- Error messages are more detailed and provide better context.
- New error types (e.g.,
ErrNoValSetForHeight
) have been introduced for more specific error handling.- 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 implementationThe changes made to this file represent a significant improvement to the state store implementation:
- The addition of the
loadValidators
helper method improves code organization and reusability.- The refactoring of the
LoadValidators
method enhances the proposer selection logic, particularly in handling quorum rotation scenarios.- Improved error handling and more informative error messages throughout the file will aid in debugging and troubleshooting.
- The addition of debug logging statements provides valuable information for tracking the state at various points.
- 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!
There was a problem hiding this 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 functionThe 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
📒 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 organizationThe extraction of logic into
configureBlockMetaWithValidators
enhances readability and maintainability.
150-152
: LGTM: Improved error handlingThe enhanced error message provides more context about the failure, which is beneficial for debugging and understanding test outcomes.
@@ -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 { |
There was a problem hiding this comment.
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
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. |
Issue being fixed or feature implemented
During quorum rotation, RPC requests to
/validators
fail with error:What was done?
How Has This Been Tested?
Modified e2e tests to chain halt on rotation and executed the /validators query.
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Documentation