feat!: version UpgradeHeightDelay
#3980
Conversation
evan-forbes
added
WS: Maintenance 🔧
evan-forbes
requested review from
rootulp,
cmwaters,
staheri14 and
ninabarbakadze
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the application's initialization, upgrade handling, and timeout configurations. Key changes include the removal of the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (14)
pkg/appconsts/v1/app_consts.go (1)
3-14: Overall assessment: Changes align with PR objectives and enhance configuration options.The additions to this file successfully introduce new configuration constants that align with the PR objectives. The UpgradeHeightDelay constant, in particular, addresses the concerns raised in issue #3978 by providing a configurable delay for upgrades.
These changes enhance the flexibility of the system and should allow for easier testing and configuration in various environments. The clear comments and logical values chosen for the constants contribute to the maintainability of the code.
To ensure these changes are fully integrated:
- Verify that these new constants are used appropriately throughout the codebase.
- Update any relevant documentation to reflect these new configuration options.
- Consider adding tests that utilize these new constants to ensure they behave as expected in different scenarios.
test/util/testnode/app_wrapper.go (2)
11-13: Enhance function documentation for clarity.While the current comment provides a basic explanation, it could be more informative. Consider expanding the documentation to include:
- The purpose of modifying the timeout commit.
- When this wrapper should be used.
- Any potential side effects or considerations when using this wrapper.
This additional information would help developers understand when and how to use this function effectively.
14-21: LGTM with a minor suggestion: Consider adding parameter validation.The implementation correctly wraps the original end blocker, preserving its behavior while modifying only the timeout. This approach is clean and unlikely to introduce bugs.
To further improve robustness, consider adding validation for the
timeoutCommitparameter. For example:if timeoutCommit < 0 { panic("timeoutCommit must be non-negative") }This would prevent potential issues with negative timeout values.
pkg/appconsts/v3/app_consts.go (1)
14-17: LGTM: UpgradeHeightDelay constant added with clear explanation.The addition of the UpgradeHeightDelay constant with its explanatory comment is excellent. The calculation is correct and the 7-day delay seems reasonable for allowing network participants to prepare for an upgrade.
Suggestion for improved clarity:
Consider updating the comment to explicitly mention the 6-second block time assumption:
// UpgradeHeightDelay is the number of blocks after a quorum has been // reached that the chain should upgrade to the new version. Assuming a block -// interval of 12 seconds, this is 7 days. +// interval of 6 seconds, this is 7 days. UpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 6) // 7 days * 24 hours * 60 minutes * 60 seconds / 6 seconds per block = 100,800 blocks.test/util/genesis/genesis.go (2)
76-79: Approved: Method rename improves clarityThe renaming of
WithModifiertoWithModifiersaccurately reflects that the method can accept multiple modifiers. This change enhances code readability and self-documentation.Consider updating the comment to reflect the plural form:
-// WithModifiers adds a genesis modifier to the genesis. +// WithModifiers adds one or more genesis modifiers to the genesis.
Line range hint
100-108: Approved: Method rename clarifies functionalityThe renaming of
WithAccountstoWithValidatorsmore accurately describes the method's purpose, which is to add validators to the genesis. This change enhances code clarity and reduces potential confusion.Consider updating the comment to provide more detail:
-// WithValidators adds the given validators to the genesis. +// WithValidators adds the given validators to the genesis, creating new validator accounts.test/e2e/benchmark/benchmark.go (1)
Line range hint
25-56: LGTM! Consider enhancing error handling.The
NewBenchmarkTestfunction is well-implemented, properly initializing the benchmark test environment. It correctly uses the provided manifest for configuration and sets up the necessary components.Consider wrapping the error returned from
testnet.Newwith additional context:testNet, err := testnet.New(kn, testnet.Options{ Grafana: testnet.GetGrafanaInfoFromEnvVar(), ChainID: manifest.ChainID, GenesisModifiers: manifest.GetGenesisModifiers(), }) -testnet.NoError("failed to create testnet", err) +if err != nil { + return nil, fmt.Errorf("failed to create testnet: %w", err) +}This change would provide more context in case of an error, making debugging easier.
test/util/testnode/config.go (3)
Line range hint
177-188: Approve changes with a suggestion for the timeout duration.The addition of
SetEndBlockerwith a wrapper function is a good improvement as it adds a timeout mechanism to prevent long-running end blocker operations from blocking the chain. However, the 30ms timeout seems quite short and might be too restrictive for some operations.Consider making the timeout duration configurable or increasing it to a more lenient value (e.g., 100ms or 200ms) to accommodate potentially longer-running operations while still preventing excessive delays.
Line range hint
195-206: Address inconsistency in end blocker timeout.The
CustomAppCreatorfunction sets the end blocker timeout to 0 milliseconds, which effectively disables the timeout mechanism. This is inconsistent with theDefaultAppCreatorfunction and might lead to unexpected behavior.Consider one of the following options:
- Align the timeout with
DefaultAppCreator(30ms).- Make the timeout configurable through a parameter.
- If there's a specific reason for disabling the timeout in
CustomAppCreator, please add a comment explaining the rationale.
Line range hint
1-206: Approve changes with suggestions for documentation.The updates to
DefaultTendermintConfigintroduce several important changes:
TimeoutCommitis set to 1ms, which might be too aggressive for slower environments.MaxTxBytesandMaxBodyBytesare set to allow very large transactions and responses.TimeoutBroadcastTxCommitis set to 1 minute.While these changes may be suitable for specific test scenarios, they could pose risks if used in production environments.
Please add comments explaining:
- The rationale behind these configurations.
- Potential risks associated with these settings.
- Why these values were chosen and in which scenarios they are appropriate.
This documentation will help future developers understand the purpose and implications of these configurations.
test/util/testnode/node_interaction_api.go (1)
55-57: Add nil check and documentation toGetTMNode()methodThe
GetTMNode()method is a good addition for accessing thetmNodefield. However, it lacks a nil check and documentation.Consider modifying the method as follows:
+// GetTMNode returns the Tendermint node associated with this context. +// It returns nil if no node has been set. func (c *Context) GetTMNode() *node.Node { + if c.tmNode == nil { + return nil + } return c.tmNode }This change adds documentation and ensures that the method doesn't panic if
tmNodeis nil.app/default_overrides.go (1)
Line range hint
1-290: Consider reviewing overall configuration settingsWhile not directly related to the changes in this PR, this file contains critical settings that significantly impact the blockchain's behavior. It might be beneficial to conduct a broader review of these configuration settings to ensure they align with the current project goals and performance expectations.
Key areas to consider:
- Consensus parameters (e.g., block size, gas limits)
- Module-specific configurations (e.g., staking, slashing, governance)
- IBC and ICA settings
- Performance-related settings (e.g., mempool configuration, state sync)
Would you like assistance in identifying any potential areas for optimization or adjustment in these configurations?
go.mod (1)
Line range hint
1-263: Overall dependency management strategyThe
go.modfile shows several important updates and replacements:
- Tendermint updated to v1.43.0-tm-v0.34.35
- cosmos-sdk replaced with a fork
- ledger-cosmos-go pinned to v0.12.4
- gogo/protobuf replaced with a fork
- syndtr/goleveldb pinned to a specific commit
While these changes likely address specific needs or avoid known issues, they also introduce potential challenges in terms of long-term maintenance and compatibility.
Consider implementing a dependency management strategy that includes:
- Regular reviews of pinned dependencies to check for important updates or security patches.
- A process for evaluating and incorporating upstream changes into forked repositories.
- Documentation of the reasons for each fork or pin, and the conditions under which you would consider reverting to the official versions.
- Automated dependency update checks and security scans to proactively identify when updates are needed.
This strategy will help balance the need for stability with the importance of staying current and secure.
app/test/upgrade_test.go (1)
109-114: Suggestion: MoveappVersionassignment outside the loopThe variable
appVersionis assignedv2.Versionat each iteration but does not change within the loop. Consider moving the assignment outside the loop to improve readability and avoid redundant assignments.Apply this diff to move
appVersionout of the loop:+ appVersion := v2.Version for height := initialHeight; height < initialHeight+appconsts.UpgradeHeightDelay(v2.Version); height++ { - appVersion := v2.Version _ = testApp.BeginBlock(abci.RequestBeginBlock{ Header: tmproto.Header{ Height: height, Version: tmversion.Consensus{App: appVersion}, }, })
🛑 Comments failed to post (11)
pkg/appconsts/v2/app_consts.go (1)
9-9: 💡 Codebase verification
TimeoutPropose is defined but not used
The
TimeoutProposeconstant inpkg/appconsts/v2/app_consts.gois currently not used anywhere in the codebase. Please review its necessity and consider removing it if it's not required.🔗 Analysis chain
Verify the TimeoutPropose value.
The
TimeoutProposeconstant is well-named and follows Go conventions. However, please ensure that 10 seconds is the appropriate duration for this timeout in your system's context.To verify if this timeout is used consistently across the codebase, run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for uses of TimeoutPropose rg --type go 'TimeoutPropose'Length of output: 2649
test/e2e/benchmark/manifest.go (1)
83-84: 💡 Codebase verification
GenesisAppVersion is not set in Manifest instantiations
The
GenesisAppVersionfield is declared and assigned inmanifest.gobut is not provided when creatingManifestinstances intest/e2e/benchmark/throughput.go. Please ensure that all instances ofManifestset theGenesisAppVersionfield appropriately.
- File:
test/e2e/benchmark/throughput.go🔗 Analysis chain
Verify usage of GenesisAppVersion in the codebase
While the addition of
GenesisAppVersionand its usage inGetConsensusParamsis good, it's important to ensure that this new field is properly utilized throughout the codebase. Please verify:
- That all code creating
Manifestinstances provides a value forGenesisAppVersion.- Whether
GetGenesisModifiersor other methods ofManifestshould use this new field.- Any other parts of the codebase that might need updates to accommodate this new field.
To help with this verification, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Manifest struct instantiations echo "Searching for Manifest struct instantiations:" rg --type go 'Manifest\s*{' -A 10 # Search for GenesisAppVersion usage echo "\nSearching for GenesisAppVersion usage:" rg --type go 'GenesisAppVersion' # Search for potential places where GenesisAppVersion might be needed echo "\nSearching for potential places where GenesisAppVersion might be needed:" rg --type go 'GetGenesisModifiers|NewManifest|CreateManifest'Length of output: 884
Script:
#!/bin/bash # Corrected search for Manifest struct instantiations echo "Searching for Manifest struct instantiations:" rg --type go 'Manifest\s*\{' -A 10 # Search for GenesisAppVersion usage echo "\nSearching for GenesisAppVersion usage:" rg --type go 'GenesisAppVersion' # Search for potential places where GenesisAppVersion might be needed echo "\nSearching for potential places where GenesisAppVersion might be needed:" rg --type go 'GetGenesisModifiers|NewManifest|CreateManifest'Length of output: 2296
test/util/testnode/node_interaction_api.go (2)
37-37:
⚠️ Potential issueConsider initializing the
tmNodefield in theNewContextfunctionThe addition of the
tmNodefield is a good way to store a reference to the Tendermint node. However, it's not initialized in theNewContextfunction, which could lead to nil pointer dereferences ifGetTMNode()is called before the field is set.Consider modifying the
NewContextfunction to accept a*node.Nodeparameter and initialize thetmNodefield. For example:-func NewContext(goContext context.Context, keyring keyring.Keyring, tmConfig *tmconfig.Config, chainID, apiAddress string) Context { +func NewContext(goContext context.Context, keyring keyring.Keyring, tmConfig *tmconfig.Config, chainID, apiAddress string, tmNode *node.Node) Context { // ... existing code ... - return Context{goContext: goContext, Context: clientContext, apiAddress: apiAddress} + return Context{goContext: goContext, Context: clientContext, apiAddress: apiAddress, tmNode: tmNode} }This change ensures that
tmNodeis properly initialized when creating a newContext.Committable suggestion was skipped due to low confidence.
37-37: 💡 Codebase verification
Issues Found: Missing Initialization of
tmNodeinContextInstancesThe
tmNodefield has been successfully added to theContextstruct innode_interaction_api.go. However, several instances whereContextis instantiated do not initialize thetmNodefield. This omission can lead to potentialnilpointer dereferences and unexpected behavior.Action Items:
- Review all
Contextinstantiations and ensure that thetmNodefield is properly initialized.- Update the following locations to include
tmNodewhere necessary:
test/util/testnode/node_interaction_api.gox/tokenfilter/ibc_middleware_test.gox/signal/keeper_test.gox/blob/ante/blob_share_decorator_test.gox/blob/ante/max_total_blob_size_ante_test.gopkg/proof/proof_test.gocmd/celestia-appd/cmd/root.goapp/ante/tx_size_gas_test.goapp/ante/panic_test.goapp/ante/gov_test.goapp/module/versioned_ibc_module_test.goapp/module/manager_test.goEnsure that each
Contextinitialization includes thetmNodefield, either by passing a valid*node.Nodeinstance or handling it appropriately based on the specific context of usage.🔗 Analysis chain
Verify impact on existing codebase
The addition of the
tmNodefield andGetTMNode()method enhances theContextstruct without directly impacting existing functionality. However, it's important to ensure that these changes are properly integrated across the codebase.Please run the following script to identify potential areas that might need updates:
Review the results to ensure that:
- All
NewContextcalls are updated if you modify its signature.- Consider if any of the found locations could benefit from using
GetTMNode().Also applies to: 55-57
Further Verification Needed
To ensure that the
tmNodefield is correctly integrated into allContextinstances, please run the following script:Review the results to ensure that:
- The
Contextstruct definition includes thetmNode *node.Nodefield.- All instances where
Contextis instantiated appropriately initialize thetmNodefield.
Action Required: Correct and Re-run Verification Script
The previous script successfully found the
tmNodefield in theContextstruct. However, there was an error in searching forContextstruct initializations due to an unescaped{in the regex pattern. Please rerun the following corrected script to identify allContextinitializations and ensure thetmNodefield is properly set:Review the results to ensure that:
- The
Contextstruct definition includes thetmNode *node.Nodefield.- All instances where
Contextis instantiated appropriately initialize thetmNodefield.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of Context creation and potential places where GetTMNode() might be useful. # Find Context creations echo "Searching for Context creations:" rg --type go "NewContext\(" -A 3 # Find potential places where GetTMNode() might be useful echo "\nSearching for potential GetTMNode() usage:" rg --type go "Context.*\.(Client|Block|Status)"Length of output: 16403
Script:
# #!/bin/bash # Description: Verify the inclusion of the tmNode field in Context struct and its initializations. # 1. Check the Context struct definition for tmNode echo "Checking Context struct definition for tmNode field:" rg --type go "type Context struct" -A 10 | grep "tmNode" # 2. Find all Context struct initializations to verify tmNode is being set echo -e "\nSearching for Context struct initializations:" rg --type go "Context{" --context 2Length of output: 462
Script:
# #!/bin/bash # Description: Verify the inclusion of the tmNode field in Context struct and its initializations. # 1. Check the Context struct definition for tmNode echo "Checking Context struct definition for tmNode field:" rg --type go "type Context struct" -A 10 | grep "tmNode" # 2. Find all Context struct initializations to verify tmNode is being set echo -e "\nSearching for Context struct initializations:" rg --type go "Context\{" --context 2Length of output: 6129
test/e2e/major_upgrade_v3.go (2)
140-142:
⚠️ Potential issueEnsure
blockTimesis not empty before accessing its elementsThe code assumes that
blockTimeshas at least one element when accessingblockTimes[0]andblockTimes[len(blockTimes)-1]. IfblockTimesis empty, this will cause an index out of range panic.Add a check to ensure that
blockTimesis not empty before accessing its elements to prevent potential runtime errors.Apply this diff to add the safety check:
+if len(blockTimes) == 0 { + return fmt.Errorf("no block durations recorded to validate") +} startDur := blockTimes[0] endDur := blockTimes[len(blockTimes)-1]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if len(blockTimes) == 0 { return fmt.Errorf("no block durations recorded to validate") } startDur := blockTimes[0] endDur := blockTimes[len(blockTimes)-1]
143-149:
⚠️ Potential issueImprove error handling by directly returning errors
In the block duration validation at lines 143-149, using
testnet.NoErrorwith a newly constructed error and an empty message is inappropriate. Thetestnet.NoErrorfunction is intended to check for unexpected errors, not to handle expected test failures.Consider directly returning the formatted error to make the test failure explicit and improve code readability.
Apply this diff to improve error handling:
if startDur < time.Second*10 { - testnet.NoError("", fmt.Errorf("blocks for v2 are too short %v", len(blockTimes))) + return fmt.Errorf("blocks for v2 are too short, number of blocks checked: %v", len(blockTimes)) } if endDur > time.Second*7 { - testnet.NoError("", fmt.Errorf("blocks for v3 are too long %v", len(blockTimes))) + return fmt.Errorf("blocks for v3 are too long, number of blocks checked: %v", len(blockTimes)) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if startDur < time.Second*10 { return fmt.Errorf("blocks for v2 are too short, number of blocks checked: %v", len(blockTimes)) } if endDur > time.Second*7 { return fmt.Errorf("blocks for v3 are too long, number of blocks checked: %v", len(blockTimes)) }app/test/upgrade_test.go (1)
119-120:
⚠️ Potential issueIssue: Inconsistent
Heightparameter inEndBlockcallWithin the loop,
EndBlockis called with a constantHeightof3 + appconsts.UpgradeHeightDelay(v2.Version), which doesn't correspond to the currentheightin the loop. This may lead to incorrect block processing and test inaccuracies. Consider using the loop variableheightas theHeightparameter.Apply this diff to fix the
Heightparameter:endBlockResp = testApp.EndBlock(abci.RequestEndBlock{ - Height: 3 + appconsts.UpgradeHeightDelay(v2.Version), + Height: height, })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Height: height, })test/e2e/testnet/testnet.go (3)
396-399:
⚠️ Potential issueCorrect indexing in the loop over 'filteredGenesisNodes'
The variable
iinfor i, node := range filteredGenesisNodesdoes not correspond to the original indices oft.nodesorgenesisNodes. This makes the checkisInIndices(i, indices)inaccurate. SincefilteredGenesisNodesis already filtered, this additional check is unnecessary.As above, removing the redundant check will resolve this issue.
428-435: 🛠️ Refactor suggestion
Optimize 'isInIndices' function for efficiency
The
isInIndicesfunction performs a linear search, which could become inefficient with larger index lists. Consider using a map for constant-time lookups.Optimized version:
func isInIndices(i int, indices []int) bool { indexMap := make(map[int]struct{}, len(indices)) for _, index := range indices { indexMap[index] = struct{}{} } _, exists := indexMap[i] return exists }
215-219:
⚠️ Potential issueEnsure consistent filtering when waiting for txClients to start
While
StartTxClientscorrectly filters and starts txClients based on the providedindices, the subsequent loop that waits for the txClients to be running does not apply this filtering. This could lead to waiting on txClients that were not started in this invocation.Apply this diff to fix the issue:
// wait for txsims to start - for _, txsim := range t.txClients { + for i, txsim := range t.txClients { + if len(indices) != 0 && !isInIndices(i, indices) { + continue + } err := txsim.Instance.Execution().WaitInstanceIsRunning(ctx) if err != nil { return fmt.Errorf("txsim %s failed to run: %w", txsim.Name, err) } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (t *Testnet) StartTxClients(ctx context.Context, indices ...int) error { for i, txsim := range t.txClients { if len(indices) != 0 && !isInIndices(i, indices) { continue } // Start txsim code (not shown in the original snippet) } // wait for txsims to start for i, txsim := range t.txClients { if len(indices) != 0 && !isInIndices(i, indices) { continue } err := txsim.Instance.Execution().WaitInstanceIsRunning(ctx) if err != nil { return fmt.Errorf("txsim %s failed to run: %w", txsim.Name, err) } } return nil }x/signal/keeper_test.go (1)
72-72:
⚠️ Potential issuePassing
nilasstoreKeyinNewKeepermay cause issuesAt line 72,
signal.NewKeeperis called withnilfor thestoreKeyparameter. IfstoreKeyis required for the keeper's functionality, passingnilmay lead to runtime errors or unexpected behavior. Consider providing a validsdk.StoreKey, similar to howsignalStoreis used at line 455.
|
Another option is to set a time instead of a number of blocks, which becomes the delay. This makes it easier to know when exactly the switch to vX will happen |
I like that the most! if we postpone to v4, then I definitely think we should do that and bother with the migration etc |
| // reached that the chain should upgrade to the new version. Assuming a block | ||
| // interval of 12 seconds, this is 7 days. |
There was a problem hiding this comment.
| // reached that the chain should upgrade to the new version. Assuming a block | |
| // interval of 12 seconds, this is 7 days. | |
| // reached that the chain should upgrade to the new version. Assuming a block | |
| // interval of 6 seconds, this is 7 days. |
|
merging now but I'm assigning myself the flups thanks everyone!! |
Overview
closes #3978 #3979
this is just a nice to have for v3. If we're worried about this causing issues at all, we should just postpone this imo.