test(e2e): fix proposer selection test #926
Conversation
WalkthroughThe changes introduce a new configuration capability to manage consensus version updates across various components of the application. This includes adding fields to multiple structs, modifying functions to handle these updates, and enhancing configuration files to reflect the new consensus versioning structure. The updates aim to streamline the management of consensus parameters and improve the organization of related settings within the application. Changes
Sequence Diagram(s)sequenceDiagram
participant Config
participant App
Config->>App: Add ConsensusVersionUpdates
App->>App: Process ConsensusVersionUpdates
App->>App: Create ConsensusParams
App-->>Config: Return Updated Configuration
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (10)
test/e2e/tests/e2e_test.go (2)
129-132: LGTM: Effective implementation of block limit.The changes effectively implement the block limit using the
maxBlocksconstant. This will help prevent excessively long test durations as intended.Consider adding a comment explaining why
tois adjusted instead offrom. For example:// limit blocks to fetch to avoid long test times if to-from > maxBlocks { + // Adjust 'to' to fetch the most recent blocks within the limit to = from + maxBlocks }This clarifies the decision to fetch the most recent blocks when limiting the range.
Line range hint
19-132: Overall assessment: Well-implemented optimization for e2e tests.The changes effectively address the potential issue of long-running tests by limiting the number of blocks fetched. The implementation is clean, well-commented, and doesn't introduce any breaking changes.
Consider adding a test case that specifically verifies the behavior when the number of blocks exceeds
maxBlocks. This would ensure that the limiting logic works as expected under various conditions.abci/example/kvstore/config.go (2)
73-74: Implement or document usage ofConsenusVersionUpdatesThe new
ConsenusVersionUpdatesfield is not used anywhere else in this file. Consider the following suggestions:
If the implementation is in other files, it would be helpful to add a comment indicating where the field is used or processed.
If the implementation is pending, consider adding a TODO comment to remind about implementing the logic for handling consensus version updates.
It might be beneficial to update the
validatorSetUpdatesmethod or add a new method to handle the consensus version updates, ensuring that the new configuration option is properly integrated into the application's logic.Example:
// TODO: Implement handling of ConsensusVersionUpdates in processConsensusParams method
Line range hint
1-74: Consider grouping related fields in theConfigstructThe
Configstruct contains various fields for different aspects of the application. To improve readability and maintainability, consider grouping related fields together. Here's a suggested reorganization:
Basic configuration:
DirKeyTypeBlock and state management:
RetainBlocksPersistIntervalSnapshotIntervalValidator management:
ValidatorUpdatesThesholdPublicKeyUpdateQuorumHashUpdatePerformance simulation:
PrepareProposalDelayMSProcessProposalDelayMSCheckTxDelayMSVoteExtensionDelayMSFinalizeBlockDelayMSDash-specific parameters:
ChainLockUpdatesPrivValServerTypeInitAppInitialCoreHeightTesting and versioning:
ConsensusVersionUpdates(after fixing the typo)Adding comments to separate these groups would further enhance the struct's readability.
test/e2e/tests/app_test.go (2)
254-260: LGTM with a suggestion: Add a safety check for non-stateless nodes.The logic to find the first non-stateless node is a good optimization. However, it assumes that at least one non-stateless node exists.
Consider adding a check to ensure that a non-stateless node was found:
for _, node = range nodes { if !node.Stateless() { break } } +if node == nil || node.Stateless() { + t.Fatal("No non-stateless node found in the testnet") +}
276-307: LGTM with a suggestion: Consider parameterizing the number of transactions.The changes in this section are well-structured and effectively test the scenario of transactions exceeding block size. The calculation of transaction size based on block parameters is a good approach for maintaining test validity across different configurations.
Consider making the number of transactions (currently hardcoded as 101) a parameter or a constant at the top of the file. This would make it easier to adjust the test conditions if needed:
+const numTxsForBigTxTest = 101 // ... (in the test function) -numTxs := 101 +numTxs := numTxsForBigTxTesttest/e2e/pkg/manifest.go (1)
59-65: LGTM! Consider enhancing the comment for clarity.The addition of the
ConsensusVersionUpdatesfield is well-implemented and aligns with the PR objectives. It provides a flexible way to specify consensus version changes at different heights.Consider slightly modifying the comment to clarify that this is for test scenarios:
- // ConsensusVersionUpdates is a map of heights to consensus versions, and - // will be sent by the ABCI application as a consensus params update. + // ConsensusVersionUpdates is a map of heights to consensus versions for testing, and + // will be sent by the ABCI application as a consensus params update in test scenarios.This change emphasizes that this field is specifically for testing purposes and not for production use.
test/e2e/pkg/testnet.go (1)
453-459: LGTM with suggestion: ConsensusVersionUpdates population logicThe logic for populating the
ConsensusVersionUpdatesmap is correct and includes proper error handling for invalid height strings. However, consider adding validation for the consensus version value (cpUpdate) to ensure it's within an expected range or meets certain criteria.Consider adding validation for
cpUpdate, for example:for heightStr, cpUpdate := range manifest.ConsensusVersionUpdates { height, err := strconv.Atoi(heightStr) if err != nil { return nil, fmt.Errorf("invalid consensus version update height %q: %w", height, err) } + if cpUpdate < 0 { + return nil, fmt.Errorf("invalid consensus version %d at height %d: must be non-negative", cpUpdate, height) + } testnet.ConsensusVersionUpdates[int64(height)] = cpUpdate }test/e2e/app/app.go (1)
66-68: Error handling could leak sensitive informationIncluding the original error
errin the formatted error message might expose internal details. Consider sanitizing the error message before returning it.Suggested change:
- return nil, fmt.Errorf("consensus_version_updates: failed to parse height %s: %w", h, err) + return nil, fmt.Errorf("consensus_version_updates: failed to parse height %s", h)test/e2e/tests/validator_test.go (1)
Line range hint
239-283: Refactor nested conditional logic in theIncrementmethodThe
Incrementmethod contains deeply nestedifstatements from lines 251 to 276, which can make the code harder to read and maintain.Consider refactoring the nested conditionals to improve readability. Using guard clauses or early returns can flatten the structure. Here's an example:
func (s *validatorSchedule) Increment(heights int64) error { for i := int64(0); i < heights; i++ { s.height++ // Consensus params update s.consensusVersionUpdate() cp := s.ConsensusParams() // Validator set update if s.height <= 1 { continue } // Validator set updates are offset by 1 update, ok := s.updates[s.height-1] if !ok { continue } thresholdPublicKeyUpdate, ok := s.thresholdPublicKeyUpdates[s.height-1] if !ok { continue } quorumHashUpdate, ok := s.quorumHashUpdates[s.height-1] if !ok { continue } currentQuorumHash := s.ValidatorProposer.ValidatorSet().QuorumHash if bytes.Equal(quorumHashUpdate, currentQuorumHash) { vs := s.ValidatorProposer.ValidatorSet() if err := vs.UpdateWithChangeSet(makeVals(update), thresholdPublicKeyUpdate, quorumHashUpdate); err != nil { return err } } else { vs := types.NewValidatorSet(makeVals(update), thresholdPublicKeyUpdate, btcjson.LLMQType_5_60, quorumHashUpdate, true) ps, err := selectproposer.NewProposerSelector(cp, vs, s.height, 0, nil, nil) if err != nil { return err } if cp.Version.ConsensusVersion == 0 { // Consensus version 0 had an issue where the first proposer didn't propose ps.ValidatorSet().IncProposerIndex(1) } s.ValidatorProposer = ps } if err := s.ValidatorProposer.UpdateHeightRound(s.height, 0); err != nil { return err } } return nil }This refactoring eliminates unnecessary nesting by checking conditions upfront and continuing to the next iteration if they're not met. This approach improves code readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- abci/example/kvstore/config.go (1 hunks)
- test/e2e/app/app.go (2 hunks)
- test/e2e/networks/rotate.toml (1 hunks)
- test/e2e/node/config.go (2 hunks)
- test/e2e/pkg/manifest.go (1 hunks)
- test/e2e/pkg/testnet.go (3 hunks)
- test/e2e/runner/setup.go (1 hunks)
- test/e2e/tests/app_test.go (1 hunks)
- test/e2e/tests/e2e_test.go (2 hunks)
- test/e2e/tests/validator_test.go (8 hunks)
🔇 Additional comments not posted (12)
test/e2e/node/config.go (1)
Line range hint
1-85: VerifyLoadConfigfunction and update documentationThe changes look good overall, but there are a couple of things to consider:
Verify that the
LoadConfigfunction (not visible in this diff) correctly handles the newConsensusVersionUpdatesfield when loading from a TOML file.Consider updating any relevant documentation (e.g., comments, README, or separate documentation files) to reflect the new
ConsensusVersionUpdatesconfiguration option.To check if the
LoadConfigfunction needs updating, you can run:Ensure that any components relying on this configuration are updated to handle the new consensus version updates appropriately.
test/e2e/tests/e2e_test.go (1)
19-21: LGTM: Well-defined constant with clear purpose.The introduction of
maxBlocksis a good approach to limit the number of blocks fetched during testing, which can help prevent excessively long test durations. The constant is well-named and its purpose is clearly explained in the comments.test/e2e/networks/rotate.toml (4)
84-89: LGTM: Validator update at height 1070 looks good.The new validator update section for height 1070 is consistent with the existing pattern in the file. It correctly assigns equal voting power (100) to validators 01 through 05.
92-97: LGTM: Validator update at height 1077 introduces a new validator set.The new validator update section for height 1077 is consistent with the existing pattern. It correctly assigns equal voting power (100) to validators 01, 07, 08, 10, and 11. This update changes the validator set compared to the previous update at height 1070, which is likely intentional for testing purposes.
83-104: Summary: New sections align well with PR objectives.The additions to the
rotate.tomlfile, including new validator update sections and the consensus version updates section, align well with the PR objectives. These changes provide more granular control over validator sets and consensus versions, which should facilitate testing of the new proposer selection algorithm and support for both consensus version 0 and 1.The new sections are consistent with the existing file structure and appear to be well-integrated. They should contribute to more comprehensive end-to-end testing scenarios.
99-103: New consensus version updates section added. Please clarify the meaning of values.The new
[consensus_version_updates]section is a valuable addition for testing consensus version changes. The updates are well-aligned with the validator update heights. However, the meaning of the values (0 and 1) is not immediately clear from the context.Could you please add a comment explaining what these values represent? For example, do they correspond to specific consensus versions or indicate whether an update should occur?
To ensure consistency and completeness, let's check if there are any other occurrences of consensus version updates in the file:
✅ Verification successful
Consensus version updates section verified.
The
[consensus_version_updates]section is unique within thetest/e2e/networks/rotate.tomlfile, with no other occurrences found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of consensus version updates rg --type toml 'consensus.*version' test/e2e/networks/rotate.tomlLength of output: 95
test/e2e/tests/app_test.go (3)
246-253: LGTM: Improved context handling for transaction broadcasting.The addition of a timeout context and the check for context cancellation before broadcasting are good practices. These changes enhance the control over the operation's duration and prevent unnecessary work.
308-308: LGTM: Improved test efficiency by removing redundant code.The removal of code that was broadcasting to all nodes and the establishment of the broadcasting context outside the loop are good improvements. These changes align with the new approach of using a single node and likely enhance the test's efficiency.
Line range hint
1-1: Overall assessment: Good improvements to the TestApp_TxTooBig function.The changes to this file, particularly in the
TestApp_TxTooBigfunction, have significantly improved the test's efficiency and robustness. The new approach of using a single non-stateless node, improved context handling, and better transaction size calculation are all positive changes.There are a few minor suggestions for further improvement:
- Add a safety check for the existence of non-stateless nodes.
- Consider addressing the issue with
ConsensusParamsfor the last height.- Parameterize the number of transactions used in the test.
These changes have enhanced the test's ability to verify the behavior of submitting transactions larger than the block size, which is crucial for ensuring the correct functioning of the blockchain application under various conditions.
test/e2e/runner/setup.go (1)
441-447: Summary: Consensus version updates support added successfullyThe changes introduced in this file successfully implement support for consensus version updates in the testnet configuration. This addition aligns well with the PR objectives and enhances the flexibility of the testing framework.
Key points:
- The implementation correctly handles the conversion of consensus version updates to the required format.
- The new configuration is integrated seamlessly into the existing
MakeAppConfigfunction.- The changes are localized and don't introduce any breaking changes to the existing codebase.
These modifications will allow for more comprehensive testing of consensus version changes in the e2e test suite.
test/e2e/pkg/testnet.go (2)
105-106: LGTM: ConsensusVersionUpdates field additionThe addition of the
ConsensusVersionUpdatesfield to theTestnetstruct is a good approach for managing consensus version updates at specific heights. The use ofint64for height andint32for version is appropriate and consistent with common practices.
221-221: LGTM: ConsensusVersionUpdates map initializationThe initialization of the
ConsensusVersionUpdatesmap is correct and consistent with the initialization of other similar fields in theTestnetstruct.
Issue being fixed or feature implemented
e2e tests fail due to new proposer selection algorithm
What was done?
validator_test.goto support consensus version 0 and 1 algorithmIn future, we might also introduce vote rejection logic to e2e tests, to emulate increasing rounds.
How Has This Been Tested?
CI/CD passed.
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Tests