fix: default engine.persistence-threshold to 0#227
Conversation
📝 WalkthroughWalkthroughEngine defaults initialization has been added to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![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
🧹 Nitpick comments (1)
src/main.rs (1)
28-28: Remove the newly added inline code comment.Line [28] introduces a Rust code comment in a path covered by the no-comments rule.
As per coding guidelines, "
src/**/*.rs: No comments in code unless explicitly requested".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` at line 28, Remove the inline Rust comment that reads "Override bera-reth recommended defaults" from src/main.rs to comply with the no-comments rule for src/**/*.rs; locate the exact comment string in the file and delete the comment line so the code contains no inline comments unless explicitly requested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.rs`:
- Around line 29-33: Replace the magic zeros and generic panic in the engine
defaults initialization: extract the literal 0 used in
DefaultEngineValues::default().with_persistence_threshold(0).with_memory_block_buffer_target(0)
into documented constants (e.g. DEFAULT_PERSISTENCE_THRESHOLD and
DEFAULT_MEMORY_BLOCK_BUFFER_TARGET) and use them in the calls; remove the
expect(...) on try_init() and propagate the error using a custom startup error
type (e.g. StartupError) or map the failure to a descriptive error variant and
return or log it at startup instead of panicking; update any function signatures
to return Result where needed so DefaultEngineValues::try_init() errors are
handled cleanly.
---
Nitpick comments:
In `@src/main.rs`:
- Line 28: Remove the inline Rust comment that reads "Override bera-reth
recommended defaults" from src/main.rs to comply with the no-comments rule for
src/**/*.rs; locate the exact comment string in the file and delete the comment
line so the code contains no inline comments unless explicitly requested.
| reth_node_core::args::DefaultEngineValues::default() | ||
| .with_persistence_threshold(0) | ||
| .with_memory_block_buffer_target(0) | ||
| .try_init() | ||
| .expect("engine defaults must be set before CLI parsing"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace magic literals and generic panic in startup init.
Lines [30]-[31] add raw 0 defaults, and Line [33] adds a generic expect(...) panic path. Please promote these defaults to documented constants and handle init failure via a custom startup error flow instead of a generic panic.
As per coding guidelines, "src/**/*.rs: Extract magic numbers into documented constants" and "src/**/*.rs: Use custom error types over generic errors".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.rs` around lines 29 - 33, Replace the magic zeros and generic panic
in the engine defaults initialization: extract the literal 0 used in
DefaultEngineValues::default().with_persistence_threshold(0).with_memory_block_buffer_target(0)
into documented constants (e.g. DEFAULT_PERSISTENCE_THRESHOLD and
DEFAULT_MEMORY_BLOCK_BUFFER_TARGET) and use them in the calls; remove the
expect(...) on try_init() and propagate the error using a custom startup error
type (e.g. StartupError) or map the failure to a descriptive error variant and
return or log it at startup instead of panicking; update any function signatures
to return Result where needed so DefaultEngineValues::try_init() errors are
handled cleanly.
1 participant
This PR defaults
--engine.persistence-thresholdand--engine.memory-block-buffer-targetto 0 by settingDefaultEngineValuesbefore CLI parsing, so users don't need to remember to pass these flags manually.Without this override, users who forget to set
--engine.persistence-threshold 0can experience panics in debug mode on fast machines, and risk state loss on unclean shutdown.This aligns with our production checklist and all deployment scripts. Users can still override via the CLI if needed.
Test plan
Summary by CodeRabbit