Files that changed from the base of the PR and between 61a747b and 07c1fc9.

• test/e2e/major_upgrade_v3.go (4 hunks)

test/e2e/major_upgrade_v3.go (4)

10-15: LGTM: Import statements updated appropriately

The new import statements are correctly added to support the enhanced functionality in the test. These imports are necessary for accessing constants and types related to different versions of the application and Tendermint types.

---

81-86: LGTM: Timer and ticker adjustments improve test reliability

The increased timer duration (20 minutes) and decreased ticker interval (3 seconds) are good improvements. These changes allow more time for the upgrade process to complete and enable more frequent checks of the upgrade status, enhancing the test's reliability.

---

Line range hint 1-191: Overall assessment: Significant improvements to upgrade test

The changes made to the MajorUpgradeToV3 function significantly enhance the upgrade test's reliability and thoroughness. Key improvements include:

1. More precise tracking of the upgrade height.
2. Comprehensive verification of block timeouts post-upgrade.
3. Handling of multiple rounds and the precise upgrade height.

These changes address most of the previous review comments and greatly improve the test's ability to catch potential issues in the upgrade process. The minor suggestions provided for error handling and consistency checks can further refine the test, but overall, the implementation is solid and well-thought-out.

---

45-45: 🛠️ Refactor suggestion

Consider a more flexible approach for version specification

The hardcoded commit hash "8f57f47169ca1ec07d615f5078d35032d379e39a" might make the test less portable across different branches. Consider using an environment variable or a configuration file to specify the version, allowing for easier updates and better flexibility in different testing scenarios.

Here's a suggestion for a more flexible approach:

-	version := "8f57f47169ca1ec07d615f5078d35032d379e39a"
+	version := os.Getenv("TEST_VERSION")
+	if version == "" {
+		version = "8f57f47169ca1ec07d615f5078d35032d379e39a" // fallback to current commit hash
+	}

This change allows you to set the version via an environment variable, falling back to the current commit hash if not specified.

Files that changed from the base of the PR and between 07c1fc9 and d969f68.

• app/app.go (5 hunks)
• app/module/configurator_test.go (1 hunks)
• app/test/upgrade_test.go (7 hunks)
• pkg/appconsts/global_consts.go (0 hunks)
• pkg/appconsts/v1/app_consts.go (1 hunks)
• pkg/appconsts/v2/app_consts.go (1 hunks)
• pkg/appconsts/v3/app_consts.go (1 hunks)
• pkg/appconsts/versioned_consts.go (2 hunks)
• x/signal/integration_test.go (1 hunks)
• x/signal/keeper.go (3 hunks)
• x/signal/keeper_test.go (4 hunks)

• pkg/appconsts/global_consts.go

• app/test/upgrade_test.go
• pkg/appconsts/v1/app_consts.go
• pkg/appconsts/v2/app_consts.go
• pkg/appconsts/v3/app_consts.go

pkg/appconsts/versioned_consts.go (2)

7-8: LGTM: New imports for versioned constants.

The addition of imports for v1 and v2 packages is appropriate for accessing version-specific constants. This approach supports the versioned logic implemented in the new functions.

---

Line range hint 1-99: Overall LGTM: Versioned constants implementation meets PR objectives.

The changes in this file successfully implement versioned timeout and upgrade height delay values, aligning with the PR objectives. The new functions GetTimeoutPropose, GetTimeoutCommit, and UpgradeHeightDelay provide a clean and consistent approach to handling different versions.

Minor suggestions have been made to improve code clarity and error handling, particularly in the UpgradeHeightDelay function. These improvements, while not critical, would enhance the overall quality and maintainability of the code.

Great job on implementing this feature!

x/signal/integration_test.go (1)

80-80: Approved change, but clarification needed on version variable

The change from appconsts.DefaultUpgradeHeightDelay to appconsts.UpgradeHeightDelay(version) aligns with the PR objectives of implementing version-specific timeouts. This modification allows for more flexible upgrade scheduling based on the current version.

However, a few points need attention:

1. Please clarify where the version variable is defined and what its value is at this point in the test.
2. Consider adding a comment explaining the reason for this change, e.g., "Use version-specific upgrade delay for more accurate testing".
3. Ensure that the UpgradeHeightDelay function is correctly implemented to handle all possible version values.

To verify the implementation of UpgradeHeightDelay, please run the following script:

✅ Verification successful

Change Verified: UpgradeHeightDelay Function Implements Version-Based Logic

The UpgradeHeightDelay function correctly incorporates version-specific logic using a switch statement on the version variable. This ensures that the upgrade delay is dynamically determined based on the current version, aligning with the PR objectives.

No issues found with the implemented changes.

• Verified that UpgradeHeightDelay is defined in pkg/appconsts/versioned_consts.go.
• Confirmed the presence of version-based conditionals within the function.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of UpgradeHeightDelay function

# Test: Search for the UpgradeHeightDelay function definition
echo "Searching for UpgradeHeightDelay function definition:"
ast-grep --lang go --pattern 'func UpgradeHeightDelay($version int64) int64 {
  $$$
}'

# Test: Check for any switch statements or conditionals based on version
echo "Checking for version-based logic in UpgradeHeightDelay:"
rg --type go -A 10 'func UpgradeHeightDelay.*\{' | rg 'switch|if.*version'

Length of output: 449

x/signal/keeper.go (3)

8-8: LGTM: New import added for appconsts package.

The addition of this import is necessary for using appconsts.UpgradeHeightDelay(version) in the TryUpgrade method. This change centralizes the upgrade height calculation logic, which is a good practice for maintainability.

---

107-107: LGTM: Upgrade height calculation updated in TryUpgrade method.

The use of appconsts.UpgradeHeightDelay(version) centralizes the upgrade height delay logic and potentially allows for version-specific delay values. This change improves the flexibility and maintainability of the upgrade process.

Please ensure that the UpgradeHeightDelay function in the appconsts package is correctly implemented. Run the following script to verify:

#!/bin/bash
# Description: Check the implementation of UpgradeHeightDelay function

# Test: Display the UpgradeHeightDelay function implementation
ast-grep --lang go --pattern $'func UpgradeHeightDelay(version uint64) int64 {
  $$$
}'

---

54-56: LGTM: NewKeeper function signature updated.

The removal of the upgradeHeightDelayBlocks parameter aligns with the changes in the Keeper struct and simplifies the initialization process.

Please ensure that all calls to NewKeeper across the codebase have been updated to reflect this change. Run the following script to verify:

x/signal/keeper_test.go (3)

186-186: Version-specific upgrade height delay

The UpgradeHeightDelay function is now called with a version parameter. This change allows for version-specific upgrade height delays, providing more flexibility in the upgrade process.

To ensure this change is applied consistently, please run the following script:

#!/bin/bash
# Description: Check for usage of UpgradeHeightDelay function

# Search for all calls to UpgradeHeightDelay
rg --type go "UpgradeHeightDelay\("

# Check if there are any remaining uses of DefaultUpgradeHeightDelay
rg --type go "DefaultUpgradeHeightDelay"

---

429-429: Updated assertion for upgrade height

The assertion for UpgradeHeight now uses appconsts.UpgradeHeightDelay(v2.Version), which is consistent with the version-specific upgrade height delay approach.

To ensure this change is applied consistently across all test cases, please run the following script:

#!/bin/bash
# Description: Check for assertions related to UpgradeHeight in test files

# Search for assertions related to UpgradeHeight
rg --type go "assert.*UpgradeHeight.*UpgradeHeightDelay" test/

---

72-72: Keeper initialization updated

The NewKeeper function call has been modified to remove the appconsts.DefaultUpgradeHeightDelay parameter. This change reflects an update in the keeper's initialization process.

To ensure this change is consistent across the codebase, please run the following script:

app/app.go (2)

283-283: Simplify SignalKeeper Initialization

The SignalKeeper is correctly initialized without the UpgradeHeightDelay() parameter, aligning with the updated design and simplifying the instantiation.

---

541-542: Ensure AppVersion Handles Zero Value

As previously noted, if this method is called when a node first starts up, resp.AppVersion may be zero. Ensure that GetTimeoutPropose and GetTimeoutCommit can handle a zero AppVersion without causing errors.

Files that changed from the base of the PR and between d969f68 and 7383fa0.

• pkg/appconsts/consensus_consts.go (1 hunks)
• test/e2e/major_upgrade_v3.go (5 hunks)

pkg/appconsts/consensus_consts.go (2)

Line range hint 1-12: Verify impact of removing TimeoutPropose and TimeoutCommit

The constants TimeoutPropose and TimeoutCommit have been removed from this file. This change could have significant implications for the consensus mechanism.

Let's verify the impact of removing these constants:

#!/bin/bash
# Description: Check for references to removed constants and potential replacements

# Search for TimeoutPropose and TimeoutCommit usage
echo "Removed constants usage:"
rg --type go "TimeoutPropose|TimeoutCommit"

# Search for potential replacements or new timeout management
echo "\nPotential new timeout management:"
rg --type go "(?i)timeout.*(?:propose|commit)"

# Search for changes in consensus-related files
echo "\nRecent changes in consensus-related files:"
git log -n 5 --pretty=format:"%h - %s" --grep="consensus"

Please review the script output to ensure that the removal of these constants is properly handled throughout the codebase, and that any new timeout management is correctly implemented.

---

10-10: Verify impact of reduced GoalBlockTime

The GoalBlockTime has been significantly reduced from 15 seconds to 6 seconds. This change aligns with the PR objectives but may have wide-ranging effects on the system's performance and behavior.

Let's verify the usage and impact of this change:

Please review the script output to ensure that this change is consistent with other parts of the codebase and doesn't introduce any conflicts or unexpected behavior.

test/e2e/major_upgrade_v3.go (3)

10-15: LGTM: New imports added for version-specific constants and Tendermint types

The addition of appconsts, v2, v3, and tmtypes imports is appropriate for the upgrade test. These imports will allow the use of version-specific constants and Tendermint block types, which are necessary for implementing and verifying the upgrade process.

---

52-52: LGTM: Consistent use of dynamically retrieved version

The changes in lines 52 and 58 correctly use the latestVersion variable for adding the Docker image and creating genesis nodes. This ensures consistency with the earlier change and maintains the flexibility of the test.

Also applies to: 58-58

---

69-69: LGTM: Consistent use of dynamically retrieved version for tx client

The change in line 69 correctly uses the latestVersion variable for creating the transaction client. This maintains consistency with the earlier changes and ensures that the correct version is used throughout the test setup.

Files that changed from the base of the PR and between 7383fa0 and fbbd9e6.

• pkg/appconsts/consensus_consts.go (0 hunks)
• test/e2e/major_upgrade_v3.go (5 hunks)

• pkg/appconsts/consensus_consts.go

test/e2e/major_upgrade_v3.go (1)

10-12: Improved imports and upgrade height

The addition of version-specific imports and the increase of upgradeHeightV3 to 40 are good improvements. This allows for more precise version checking and gives the network more time to stabilize before the upgrade.

Also applies to: 21-21