feat(oe): Enable OE by default (target: v8.0+) #2595
Conversation
WalkthroughThe changes in this pull request modify the default value of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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[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 (1)
protocol/app/flags/flags.go (1)
83-84: Consider adding validation for version compatibilitySince this feature is specifically targeted for v8.0+, consider adding version validation in the
Validate()method to prevent unintended usage in earlier versions.Would you like me to propose a version validation implementation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
protocol/app/flags/flags.go(1 hunks)
🔇 Additional comments (1)
protocol/app/flags/flags.go (1)
83-83:
This change modifies the default behavior of block execution for all nodes that don't explicitly set this flag. This is a significant change that could affect consensus and should be clearly documented as a state-breaking change.
Let's verify the impact by checking for any explicit flag settings in configuration files:
| DefaultOptimisticExecutionEnabled = true | ||
| DefaultOptimisticExecutionTestAbortRate = 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add migration guide and version compatibility notes
Since this is targeting v8.0+, please:
- Document the version requirement
- Add migration instructions for existing nodes
- Document any performance implications or requirements
Consider adding these details in the flag description:
cmd.Flags().Bool(
OptimisticExecutionEnabled,
- DefaultOptimisticExecutionEnabled,
- "Whether to enable optimistic block execution",
+ DefaultOptimisticExecutionEnabled,
+ "Whether to enable optimistic block execution. "+
+ "Default: enabled (as of v8.0+). "+
+ "Note: Disabling this flag on v8.0+ may impact performance. "+
+ "For migration instructions, see: <docs-url>",
)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
protocol/testutil/app/app.go (1)
Line range hint
1301-1322: Consider improving command arguments organization.While the implementation is correct, the command arguments list could be more maintainable with the following improvements:
- Group related flags together (e.g., daemon flags, oracle flags)
- Consider using constants for repeated values like "false"
- Add comments to explain the purpose of each group of flags
Example organization:
cmdArgs := []string{ "start", - // Do not start tendermint. - "--grpc-only", - "true", - "--home", - validatorHomeDir, - // TODO(CORE-29): Allow the daemons to be launched and cleaned-up successfully by default. - "--price-daemon-enabled", - "false", - "--bridge-daemon-enabled", - "false", - "--liquidation-daemon-enabled", - "false", - "--bridge-daemon-eth-rpc-endpoint", - "https://eth-sepolia.g.alchemy.com/v2/demo", - "--oracle.enabled=false", - "--oracle.metrics_enabled=false", - "--log_level=error", + // Core settings + "--grpc-only", "true", // Do not start tendermint + "--home", validatorHomeDir, + "--log_level=error", + + // Daemon settings + // TODO(CORE-29): Allow the daemons to be launched and cleaned-up successfully by default + "--price-daemon-enabled=false", + "--bridge-daemon-enabled=false", + "--liquidation-daemon-enabled=false", + "--bridge-daemon-eth-rpc-endpoint=https://eth-sepolia.g.alchemy.com/v2/demo", + + // Oracle settings + "--oracle.enabled=false", + "--oracle.metrics_enabled=false", }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
protocol/testutil/app/app.go(5 hunks)
🔇 Additional comments (2)
protocol/testutil/app/app.go (2)
1252-1252: LGTM! Function signature change enhances configurability.
The addition of the variadic parameter additionalCmdArgs allows for flexible injection of command-line arguments, improving the function's testability and configurability.
536-540: LGTM! Appropriate usage of optimistic execution flag.
The crashingApp instance is correctly configured to disable optimistic execution, which:
- Aligns with testing requirements
- Allows testing both enabled and disabled states
- Maintains consistency during app restarts
Also applies to: 566-570
teddyding
force-pushed
the
td/enable-oe-default
branch
from
50fffbf
to
cd6c7cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
protocol/testutil/app/app.go (1)
1311-1311: LGTM! Consider adding a comment explaining the default test behavior.The addition of
--optimistic-execution-enabled=falseflag is correct and aligns with the PR's objective of enabling optimistic execution by default in production while allowing it to be explicitly disabled in tests.Consider adding a comment above this line explaining why optimistic execution is disabled by default in test environments, for example:
+ // Disable optimistic execution by default in tests to ensure deterministic behavior "--optimistic-execution-enabled=false",
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores