feat: override timeout commit via --timeout-commit
#4103
Conversation
rootulp
requested review from
staheri14,
ninabarbakadze,
cmwaters,
rach-id and
evan-forbes
and removed request for
a team and
staheri14
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files, primarily to enhance the Changes
Assessment against linked issues
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
pkg/appconsts/chain_ids.go (1)
Line range hint
1-5: Missing chain ID constants referenced in PublicNetworks.The
MochaChainIDandMainnetChainIDconstants are referenced inPublicNetworksbut are not defined in this file.Add the missing constants:
const ( ArabicaChainID = "arabica-11" + MochaChainID = "mocha-4" + MainnetChainID = "celestia" )cmd/celestia-appd/cmd/app_exporter.go (1)
23-23: Improve clarity of the hardcoded timeout parameter.The hardcoded
0value for the timeout parameter is not immediately clear to future maintainers. Consider using a named constant or retrieving the value from configuration.- application := app.New(logger, db, traceStore, uint(1), config, 0, 0, appOptions) + const defaultExportTimeout time.Duration = 0 + application := app.New(logger, db, traceStore, uint(1), config, 0, defaultExportTimeout, appOptions)cmd/celestia-appd/cmd/app_server.go (1)
Line range hint
46-65: Consider refactoring using the builder pattern.The
app.Newconstructor has many parameters which makes it hard to maintain and prone to errors when parameter order changes. Consider implementing the builder pattern for better maintainability.Would you like me to provide an example implementation of the builder pattern for this use case?
scripts/single-node.sh (1)
105-105: LGTM! Consider adding a comment about testing usage.The
--timeout-commit 1sflag is correctly implemented. However, adding a brief comment above this line would help clarify that this setting is specifically for testing purposes.+ # Override default timeout for faster block production in test environment --timeout-commit 1s \README.md (1)
131-136: LGTM! Consider enhancing the documentation.The documentation clearly explains the feature. Consider adding:
- A note about the flag being restricted to private testnets
- A warning about not using this in production environments
If you are running celestia-app in tests, you may want to override the `timeout_commit` to produce blocks faster. By default, a celestia-app chain with app version >= 3 will produce blocks every ~6 seconds. To produce blocks faster, you can override the `timeout_commit` with the `--timeout-commit` flag. +> [!WARNING] +> The `--timeout-commit` flag is only supported on private testnets and should not be used in production environments. + ```shell # Start celestia-appd with a one second timeout commit. celestia-appd start --timeout-commit 1s
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
Makefile(1 hunks)README.md(1 hunks)app/app.go(8 hunks)cmd/celestia-appd/cmd/app_exporter.go(1 hunks)cmd/celestia-appd/cmd/app_server.go(1 hunks)cmd/celestia-appd/cmd/root.go(3 hunks)cmd/celestia-appd/cmd/start.go(2 hunks)pkg/appconsts/chain_ids.go(1 hunks)scripts/single-node.sh(1 hunks)
🔇 Additional comments (11)
pkg/appconsts/chain_ids.go (1)
7-7: LGTM on PublicNetworks implementation.
The PublicNetworks slice effectively groups the chain IDs for validation purposes, aligning with the PR objective to prevent timeout modifications on public networks.
cmd/celestia-appd/cmd/root.go (2)
41-42: LGTM! Well-defined constant.
The constant is appropriately named and follows the existing naming convention.
142-146: LGTM! Flag integration is clean and well-documented.
The flag is properly integrated with a clear description indicating its testing purpose. The use of Duration type is appropriate for timeout values.
Makefile (1)
51-51: LGTM: Improved build feedback
The added echo command provides better visibility into the build process.
cmd/celestia-appd/cmd/start.go (2)
17-17: LGTM: Required import added
Import statement correctly added for accessing public network constants.
121-123: LGTM: Well-implemented validation for timeout-commit flag
The check effectively prevents the use of timeout-commit flag on public networks, with a clear error message that guides users on proper usage.
app/app.go (5)
7-7: LGTM: Required import added
Import statement correctly added for time.Duration type.
174-177: LGTM: Well-documented field addition
The timeoutCommit field is clearly documented with its purpose and usage restrictions.
196-196: LGTM: Constructor updated consistently
The New function is properly updated to accept and set the timeoutCommit parameter.
Also applies to: 223-223
491-493: LGTM: Consistent timeout handling across methods
The EndBlocker, Info, and InitChain methods are consistently updated to use the new timeout commit logic.
Also applies to: 549-551, 575-577
860-868: LGTM: Well-implemented helper method
The getTimeoutCommit method properly handles both custom and default timeout values.
cmd/celestia-appd/cmd/start.go
Outdated
| if contains(appconsts.PublicNetworks, clientCtx.ChainID) && serverCtx.Viper.GetDuration(TimeoutCommitFlag) != 0 { | ||
| return fmt.Errorf("the --timeout-commit flag was used on %v but it is unsupported on public networks: %v. The --timeout-commit flag should only be used on private testnets.", clientCtx.ChainID, strings.Join(appconsts.PublicNetworks, ", ")) | ||
| } |
rootulp
dismissed stale reviews from cmwaters, rach-id, and evan-forbes
via
a3534af
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
test/util/testnode/config.go (1)
202-202: Consider using the provided timeout commit value.The function hardcodes
time.Millisecond*0inwrapEndBlockerwhile ignoring the timeout commit parameter. Consider using the provided parameter for consistency.-app.SetEndBlocker(wrapEndBlocker(app, time.Millisecond*0)) +app.SetEndBlocker(wrapEndBlocker(app, d))app/app_test.go (1)
33-33: Consider using test constants for timeout values.Multiple test cases use the same hardcoded timeout value. Consider defining a test constant for better maintainability.
+const testTimeoutCommit = time.Second -timeoutCommit := time.Second +timeoutCommit := testTimeoutCommitAlso applies to: 70-70, 166-166
app/app.go (1)
860-868: Enhance method documentation.While the implementation is correct, consider adding more detailed documentation about the default behavior when timeoutCommit is 0.
-// getTimeoutCommit returns the timeoutCommit if a user has overridden it via the -// --timeout-commit flag. Otherwise, it returns the default timeout commit based -// on the app version. +// getTimeoutCommit returns the timeout commit duration to use for the given app version. +// If the user has overridden the default via the --timeout-commit flag (timeoutCommit != 0), +// it returns the user-specified duration. Otherwise, it returns the default timeout commit +// for the app version using appconsts.GetTimeoutCommit. +// +// Note: Custom timeout commits should only be used in testing environments and never +// on public networks (Arabica, Mocha, or Mainnet Beta). func (app *App) getTimeoutCommit(appVersion uint64) time.Duration {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
app/app.go(8 hunks)app/app_test.go(6 hunks)app/test/upgrade_test.go(1 hunks)cmd/celestia-appd/cmd/start.go(1 hunks)test/tokenfilter/setup.go(1 hunks)test/util/malicious/app.go(1 hunks)test/util/test_app.go(2 hunks)test/util/testnode/config.go(2 hunks)tools/chainbuilder/integration_test.go(1 hunks)tools/chainbuilder/main.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/chainbuilder/integration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/celestia-appd/cmd/start.go
🔇 Additional comments (11)
test/util/malicious/app.go (1)
59-59: LGTM: Correctly adapted to the new app initialization signature.
The change maintains compatibility with the new timeout commit parameter while preserving the malicious test utilities' functionality.
test/util/testnode/config.go (1)
179-179: LGTM: Correctly added timeout commit parameter.
The change aligns with the PR objectives to support custom timeout settings in testing environments.
app/app_test.go (2)
8-8: LGTM: Added required import.
The time import is necessary for the Duration type used in timeout commit parameters.
36-36: LGTM: Correctly updated app initialization calls.
All app initialization calls have been updated to include the timeout commit parameter, maintaining consistency across tests.
Also applies to: 72-72, 109-109, 181-181
test/tokenfilter/setup.go (1)
149-149: LGTM: Default timeout commit value added.
The addition of the default timeout commit parameter (0) is consistent with the new feature implementation.
app/test/upgrade_test.go (1)
256-256: Verify timeout commit behavior during upgrades.
The default timeout commit value (0) is added correctly. Since this file tests upgrades and verifies timeout values in EndBlock responses, ensure that the timeout commit behavior is properly tested across version upgrades.
✅ Verification successful
The verification results show that timeout commit values are properly tested across version upgrades in app/test/upgrade_test.go. The tests consistently verify that:
- The timeout commit values match the expected values from
appconsts.GetTimeoutCommit()for each version - These checks are performed during version transitions (v1 -> v2 -> v3)
- The verification happens in both EndBlock responses and info responses
Let me verify the appconsts implementation to ensure the timeout values are properly defined:
Let me check the version-specific timeout values to complete the verification:
Timeout commit behavior is properly tested during upgrades
The verification confirms that:
- The
GetTimeoutCommitfunction is properly implemented inpkg/appconsts/versioned_consts.gowith version-specific values - Timeout values are consistently verified in upgrade tests across all versions (v1 -> v2 -> v3)
- The tests check timeout values in both EndBlock responses and info responses
- The default timeout commit value (0) passed to
app.New()is appropriate as the actual timeout is determined by the version-specific constants
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that timeout commit values are tested across version upgrades
# Check if the tests verify timeout commit values during upgrades
rg -A 5 "TimeoutCommit" "app/test/upgrade_test.go"
Length of output: 1603
Script:
#!/bin/bash
# Check the implementation of timeout commit constants
rg -A 3 "GetTimeoutCommit" "app/appconsts/"
Length of output: 96
Script:
#!/bin/bash
# Search for GetTimeoutCommit in the entire codebase
rg -A 3 "GetTimeoutCommit"
# Also search for any file containing "timeout" in its name
fd timeout
Length of output: 2900
Script:
#!/bin/bash
# Check version-specific timeout values
rg "TimeoutCommit =" pkg/appconsts/
Length of output: 167
Script:
#!/bin/bash
# Check v3 timeout values
rg "TimeoutCommit =" pkg/appconsts/v3/
Length of output: 40
test/util/test_app.go (1)
110-110: LGTM: Consistent default timeout commit values.
The default timeout commit parameter (0) is consistently added to both test utility functions:
- NewTestApp()
- SetupTestAppWithUpgradeHeight()
This maintains consistency across the test suite while supporting the new timeout commit feature.
Also applies to: 471-471
app/app.go (4)
174-177: LGTM! Clear documentation of testing-only feature.
The field documentation clearly states the intended use case and explicitly warns against usage on public networks.
223-223: LGTM! Clean field initialization.
The timeoutCommit field is properly initialized in the App struct.
491-491: LGTM! Consistent timeout handling across lifecycle methods.
The timeoutCommit is consistently applied in EndBlocker, Info, and InitChain using the helper method.
Also applies to: 549-549, 575-575
Line range hint 174-868: Verify test coverage for timeout commit changes.
Since this change affects consensus timeouts, ensure there are comprehensive tests covering:
- Default timeout behavior
- Custom timeout behavior in test environments
- Rejection of custom timeouts on public networks
✅ Verification successful
Timeout commit changes are properly tested and safeguarded
The codebase has comprehensive test coverage and safeguards for timeout commit changes:
- Tests verify timeout behavior across all app versions (v1, v2, v3) in
app/test/upgrade_test.go - Custom timeout commits are restricted to private testnets only, with explicit checks preventing usage on public networks
- Default timeout values are properly versioned in
pkg/appconsts/with clear documentation - E2E tests validate timeout behavior during version upgrades
- Test utilities support configurable timeouts for testing scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage of timeout commit functionality
# Look for timeout commit related tests
rg -A 5 "TestTimeoutCommit|timeout.*commit.*test" --type go
# Check for any existing timeout commit configurations in test files
rg -A 5 "timeoutCommit.*:.*\d+.*s" --type go
Length of output: 1630
Script:
#!/bin/bash
# Let's check for test coverage more comprehensively
# Look for timeout commit related code and tests
echo "=== Searching for timeout commit related code ==="
rg -A 5 "timeoutCommit|TimeoutCommit" --type go
# Look for app version specific timeout settings
echo "=== Searching for app version specific timeout settings ==="
rg -A 5 "GetTimeoutCommit|GetTimeoutPropose" --type go
# Check for test files containing timeout configurations
echo "=== Searching for test files with timeout configurations ==="
fd -e go -e yaml test | xargs rg -l "timeout.*commit"
Length of output: 26528
| 0, // upgrade height v2 | ||
| 0, // timeout commit |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add support for configurable timeout commit via command line flag
The hardcoded timeout commit value of 0 contradicts the PR objectives which aim to support custom timeout settings for testing environments.
Consider these changes:
- Add a command-line flag for timeout commit:
func main() {
rootCmd.Flags().Int("num-blocks", 100, "Number of blocks to generate")
+ rootCmd.Flags().Duration("timeout-commit", 0, "Block timeout commit duration for testing. Only applicable for private testnets")- Update the BuilderConfig struct:
type BuilderConfig struct {
NumBlocks int
BlockSize int
BlockInterval time.Duration
+ TimeoutCommit time.Duration
ExistingDir string
Namespace share.Namespace
ChainID string
AppVersion uint64
UpToTime bool
}- Pass the timeout commit to app.New:
- 0, // timeout commit
+ cfg.TimeoutCommit,- Add validation to restrict timeout commit changes to private testnets:
func Run(ctx context.Context, cfg BuilderConfig, dir string) error {
+ // Validate timeout commit usage
+ if cfg.TimeoutCommit > 0 && !strings.HasPrefix(cfg.ChainID, "private-") {
+ return fmt.Errorf("timeout commit can only be set for private testnets (chain IDs prefixed with 'private-')")
+ }Committable suggestion skipped: line range outside the PR's diff.
| @@ -188,6 +193,7 @@ func New( | |||
| invCheckPeriod uint, | |||
| encodingConfig encoding.Config, | |||
| upgradeHeightV2 int64, | |||
| timeoutCommit time.Duration, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for public network protection.
Consider adding validation in the constructor to prevent setting custom timeout commits on public networks (Arabica, Mocha, or Mainnet Beta) as mentioned in the documentation.
func New(
logger log.Logger,
db dbm.DB,
traceStore io.Writer,
invCheckPeriod uint,
encodingConfig encoding.Config,
upgradeHeightV2 int64,
timeoutCommit time.Duration,
appOpts servertypes.AppOptions,
baseAppOptions ...func(*baseapp.BaseApp),
) *App {
+ // Prevent custom timeout commits on public networks
+ chainID := cast.ToString(appOpts.Get("chain-id"))
+ if timeoutCommit != 0 && (chainID == "arabica-11" || chainID == "mocha-4" || chainID == "celestia") {
+ panic("custom timeout commits are not supported on public networks")
+ }📝 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.
| timeoutCommit time.Duration, | |
| func New( | |
| logger log.Logger, | |
| db dbm.DB, | |
| traceStore io.Writer, | |
| invCheckPeriod uint, | |
| encodingConfig encoding.Config, | |
| upgradeHeightV2 int64, | |
| timeoutCommit time.Duration, | |
| appOpts servertypes.AppOptions, | |
| baseAppOptions ...func(*baseapp.BaseApp), | |
| ) *App { | |
| // Prevent custom timeout commits on public networks | |
| chainID := cast.ToString(appOpts.Get("chain-id")) | |
| if timeoutCommit != 0 && (chainID == "arabica-11" || chainID == "mocha-4" || chainID == "celestia") { | |
| panic("custom timeout commits are not supported on public networks") | |
| } | |
| timeoutCommit time.Duration, |
cmwaters
added
the
backport:v3.x
Closes #4080 ## Testing 1. `./scripts/single-node.sh` now makes blocks faster 🎊 2. `./scripts/arabica.sh` throws an error if you try to add `--timeout-commit 1s` ``` Error: the --timeout-commit flag was used on arabica-11 but it is unsupported on public networks: arabica-11, mocha-4, celestia. The --timeout-commit flag should only be used on private testnets. ``` (cherry picked from commit d20916c) # Conflicts: # cmd/celestia-appd/cmd/start.go # scripts/single-node.sh
…#4114) Closes #4080 ## Testing 1. `./scripts/single-node.sh` now makes blocks faster 🎊 2. `./scripts/arabica.sh` throws an error if you try to add `--timeout-commit 1s` ``` Error: the --timeout-commit flag was used on arabica-11 but it is unsupported on public networks: arabica-11, mocha-4, celestia. The --timeout-commit flag should only be used on private testnets. ``` --------- Co-authored-by: Rootul P <[email protected]>
Closes #4080
Testing
./scripts/single-node.shnow makes blocks faster 🎊./scripts/arabica.shthrows an error if you try to add--timeout-commit 1s