fix(consensus): mark future timestamps as transient#133
Conversation
📝 WalkthroughWalkthroughAdds ChangesMorphConsensus transient error classification
Engine RPC error mapping cleanup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
panos-xyz
force-pushed
the
codex/fix-transient-future-timestamp
branch
from
68c1e4c to
cc0c65d
Compare
![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 (2)
crates/consensus/src/validation.rs (1)
1138-1148: ⚡ Quick winConsider adding test coverage for non-transient errors.
The test correctly verifies that
TimestampIsInFutureis transient. To ensure the implementation doesn't accidentally mark other errors as transient, consider adding a test case that verifies at least one otherConsensusErrorvariant returnsfalse.✅ Example test for non-transient error
assert!(Consensus::<Block>::is_transient_error(&consensus, &error)); } +#[test] +fn test_other_consensus_errors_are_not_transient() { + let chain_spec = create_test_chainspec(); + let consensus = MorphConsensus::new(chain_spec); + + // Test that a non-timestamp error is not transient + let error = ConsensusError::BaseFeeMissing; + assert!(!Consensus::<Block>::is_transient_error(&consensus, &error)); +} + #[test] fn test_validate_header_gas_limit_exceeds_max() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/consensus/src/validation.rs` around lines 1138 - 1148, Add a new test function to complement the existing test_timestamp_in_future_is_transient_error test. This new test should instantiate the MorphConsensus and the is_transient_error method with a different ConsensusError variant (one that should not be transient, such as an error related to invalid block or signature validation) and assert that is_transient_error returns false. This will provide coverage that the implementation correctly distinguishes between transient and non-transient errors rather than potentially marking all errors as transient.crates/engine-api/src/rpc.rs (1)
121-124: Consider structured error logging for RPC failures.The removal of
tracing::error!logging from these seven methods reduces error visibility in production environments where debug logs are typically disabled. While errors are still returned to RPC clients, operators lose server-side error signals for monitoring and alerting.Consider one of these approaches:
- Re-add error-level logging in the error path (e.g.,
tracing::error!(target: "morph::engine", error = %e, "RPC method failed")before.into())- Ensure your observability stack captures RPC error responses through other means (metrics, middleware, etc.)
- Document the rationale if error logging was intentionally removed to reduce log volume
Also applies to: 141-141, 152-155, 166-166, 177-177, 187-190, 208-208
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine-api/src/rpc.rs` around lines 121 - 124, Re-add structured error-level logging to all seven RPC methods in crates/engine-api/src/rpc.rs to restore error visibility in production environments. For each affected method (at lines 121-124, 141, 152-155, 166, 177, 187-190, and 208), add a tracing::error! call in the error path before the .into() conversion. Use a consistent log format such as tracing::error!(target: "morph::engine", error = %e, "RPC method failed") to capture the actual error details along with method context, enabling operators to monitor and alert on RPC failures through server-side signals.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/consensus/src/validation.rs`:
- Around line 332-334: The is_transient_error method implementation is correct,
but the existing test only covers the positive case where TimestampIsInFuture is
transient. Add additional test cases to the test for is_transient_error that
verify negative scenarios, asserting that other ConsensusError variants such as
ExtraDataExceedsMax return false from is_transient_error. This ensures the
method correctly identifies which errors are transient and which are not.
---
Nitpick comments:
In `@crates/consensus/src/validation.rs`:
- Around line 1138-1148: Add a new test function to complement the existing
test_timestamp_in_future_is_transient_error test. This new test should
instantiate the MorphConsensus and the is_transient_error method with a
different ConsensusError variant (one that should not be transient, such as an
error related to invalid block or signature validation) and assert that
is_transient_error returns false. This will provide coverage that the
implementation correctly distinguishes between transient and non-transient
errors rather than potentially marking all errors as transient.
In `@crates/engine-api/src/rpc.rs`:
- Around line 121-124: Re-add structured error-level logging to all seven RPC
methods in crates/engine-api/src/rpc.rs to restore error visibility in
production environments. For each affected method (at lines 121-124, 141,
152-155, 166, 177, 187-190, and 208), add a tracing::error! call in the error
path before the .into() conversion. Use a consistent log format such as
tracing::error!(target: "morph::engine", error = %e, "RPC method failed") to
capture the actual error details along with method context, enabling operators
to monitor and alert on RPC failures through server-side signals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4687c14-f11e-41a2-9eae-27d90d47f4b8
📒 Files selected for processing (2)
crates/consensus/src/validation.rscrates/engine-api/src/rpc.rs
| fn is_transient_error(&self, error: &ConsensusError) -> bool { | ||
| matches!(error, ConsensusError::TimestampIsInFuture { .. }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify matches! macro usage and check for syntax issues
# Check Rust version and toolchain
rustc --version || echo "rustc not available"
# Search for other uses of matches! in the codebase to see if it's used elsewhere
rg -n 'matches!\(' --type=rust -C2
# Check if there are any known issues with matches! in similar contexts
ast-grep --pattern $'impl Consensus<$_> for $_ {
$$$
fn is_transient_error(&$_, error: &ConsensusError) -> bool {
$$$
}
$$$
}'Repository: morph-l2/morph-reth
Length of output: 25311
🏁 Script executed:
# Check the test implementation at lines 1138-1148
sed -n '1138,1148p' crates/consensus/src/validation.rs
# Search for other tests or usages of is_transient_error
rg -n 'is_transient_error' crates/consensus/src/validation.rsRepository: morph-l2/morph-reth
Length of output: 663
The matches! macro warning is a false positive; add a test for negative cases.
The implementation is correct—matches! is used successfully throughout the codebase including in this same file. However, the test only verifies that TimestampIsInFuture is transient. Add a test case asserting that other error variants (e.g., ConsensusError::ExtraDataExceedsMax) return false from is_transient_error.
🧰 Tools
🪛 GitHub Actions: PR `#133` / 0_Analyze (rust).txt
[warning] 333-333: Rustc warning: macro expansion failed for 'matches' (macro expansion failed)
🪛 GitHub Actions: PR `#133` / Analyze (rust)
[warning] 333-333: Rust warning: macro expansion failed for 'matches'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/consensus/src/validation.rs` around lines 332 - 334, The
is_transient_error method implementation is correct, but the existing test only
covers the positive case where TimestampIsInFuture is transient. Add additional
test cases to the test for is_transient_error that verify negative scenarios,
asserting that other ConsensusError variants such as ExtraDataExceedsMax return
false from is_transient_error. This ensures the method correctly identifies
which errors are transient and which are not.
1 participant
Summary
ConsensusError::TimestampIsInFutureas transient inMorphConsensusso reth can retry briefly future-dated payloads instead of caching them as invalid.Root Cause
Morph header validation correctly rejects headers whose timestamp is ahead of the local clock, but
MorphConsensusdid not overrideis_transient_error. With reth v2.2.0, that caused short clock-skew rejections to enter the invalid header cache, so later payloads linked to the same block kept failing as descendants of a previously rejected block.Impact
A block that is only ahead of the local clock can now be retried once local time catches up. Other consensus errors keep the default non-transient behavior.
Test Plan
cargo fmt --all -- --checkcargo test -p morph-consensuscargo clippy -p morph-consensus --all-targets -- -D warningscargo clippy --all --all-targets -- -D warningscargo test --doc --all --verboseSummary by CodeRabbit
Refactor
Tests