fix: Revert "feat: report reorg if checkpoint index decreased but block height stayed the same or increased" #7252
Conversation
…ight sta…" This reverts commit 8fc8325.
|
📝 WalkthroughWalkthroughThis PR simplifies checkpoint handling by removing the Changes
Sequence Diagram(s)sequenceDiagram
participant Validator as Validator Submit
participant Reporter as Reorg Reporter
participant Storage as Checkpoint Storage
Note over Validator: Before: verify_checkpoint phase
Validator->>Storage: retrieve_latest_checkpoint_info()
Storage-->>Validator: CheckpointInfo
Note over Validator: After: inline handling
Validator->>Validator: Create ReorgEvent(checkpoint_index)
Validator->>Validator: Log error
alt Mismatch detected
Validator->>Reporter: report_reorg(height or period)
Validator->>Storage: write reorg status
Storage-->>Validator: result
alt Write fails
Validator->>Validator: panic with message
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span multiple files with varied modifications: type removal, trait API simplification, struct field consolidation, and control flow reorganization in the validator. The changes are mostly cohesive in purpose (checkpoint simplification) but require separate reasoning for each file's context, particularly the validator's refactored error-handling logic. Test updates are largely mechanical (variable renames, mock adjustments) but contribute to overall complexity. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/main/agents/relayer/src/msg/db_loader.rs (1)
55-66: Bug: wrong type passed to DirectionalNonceIterator::new (u32.into() vs Option)
unwrap_or_default().into()yields au32, not anOption<u32>. This won’t compile. Wrap explicitly.Apply one of these minimal fixes:
- let high_nonce_iter = DirectionalNonceIterator::new( - // If the high nonce is None, we start from the beginning - high_nonce.unwrap_or_default().into(), + let high_nonce_iter = DirectionalNonceIterator::new( + // If the high nonce is None, we start from the beginning (nonce = 0) + high_nonce.or(Some(0)), NonceDirection::High, db.clone(), domain.clone(), );Alternative:
- high_nonce.unwrap_or_default().into(), + Some(high_nonce.unwrap_or_default()),rust/main/agents/validator/src/submit/tests.rs (1)
573-573: Align expected log text with implementationCode logs “different values, overwriting”, not “different signature”. Update the assertion so the test stops bellyaching.
- logs_contain("Checkpoint already submitted, but with different signature, overwriting"); + logs_contain("Checkpoint already submitted, but with different values, overwriting");
🧹 Nitpick comments (3)
rust/main/agents/validator/src/submit.rs (1)
264-275: Enrich panic context (indices) for faster ops triageInclude local vs correctness indices in the panic to cut through the muck during incidents. Optional, but helpful.
- let mut panic_message = "Incorrect tree root. Most likely a reorg has occurred. Please reach out for help, this is a potentially serious error impacting signed messages. Do NOT forcefully resume operation of this validator. Keep it crashlooping or shut down until you receive support.".to_owned(); + let mut panic_message = format!( + "Incorrect tree root (local idx {}, correctness idx {}). Most likely a reorg has occurred. Please reach out for help, this is a potentially serious error impacting signed messages. Do NOT forcefully resume operation of this validator. Keep it crashlooping or shut down until you receive support.", + checkpoint.index, + correctness_checkpoint.index + );rust/main/agents/validator/src/submit/tests.rs (2)
244-251: Fix repeated typo: merke → merkleMinor clean-up; makes the tests easier to read.
- let pre_reorg_merke_insertions = [ + let pre_reorg_merkle_insertions = [ @@ - for insertion in pre_reorg_merke_insertions.iter() { + for insertion in pre_reorg_merkle_insertions.iter() { @@ - pre_reorg_merke_insertions[0], - pre_reorg_merke_insertions[1], + pre_reorg_merkle_insertions[0], + pre_reorg_merkle_insertions[1], @@ - .returning(move |sequence| Ok(Some(pre_reorg_merke_insertions[*sequence as usize]))); + .returning(move |sequence| Ok(Some(pre_reorg_merkle_insertions[*sequence as usize]))); @@ - let pre_reorg_merke_insertions = [ + let pre_reorg_merkle_insertions = [ @@ - for insertion in pre_reorg_merke_insertions.iter() { + for insertion in pre_reorg_merkle_insertions.iter() { @@ - pre_reorg_merke_insertions[0], - pre_reorg_merke_insertions[1], + pre_reorg_merkle_insertions[0], + pre_reorg_merkle_insertions[1], @@ - .returning(move |sequence| Ok(Some(pre_reorg_merke_insertions[*sequence as usize]))); + .returning(move |sequence| Ok(Some(pre_reorg_merkle_insertions[*sequence as usize]))); @@ - let pre_reorg_merke_insertions = [ + let pre_reorg_merkle_insertions = [ @@ - for insertion in pre_reorg_merke_insertions.iter() { + for insertion in pre_reorg_merkle_insertions.iter() { @@ - pre_reorg_merke_insertions[0], - pre_reorg_merke_insertions[1], + pre_reorg_merkle_insertions[0], + pre_reorg_merkle_insertions[1], @@ - .returning(move |sequence| Ok(Some(pre_reorg_merke_insertions[*sequence as usize]))); + .returning(move |sequence| Ok(Some(pre_reorg_merkle_insertions[*sequence as usize])));Also applies to: 256-257, 274-274, 357-364, 369-371, 387-387, 462-469, 474-476, 492-492
230-231: Deflake timestamp assertionThe 1‑second window is tight and can flake under load. Widen slightly.
- assert!(timestamp_diff.abs() < 1); + assert!(timestamp_diff.abs() <= 2);If you want me to verify flakiness locally with a stress loop, I can script it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
rust/main/agents/relayer/src/msg/db_loader.rs(1 hunks)rust/main/agents/relayer/src/msg/pending_message.rs(0 hunks)rust/main/agents/validator/src/submit.rs(2 hunks)rust/main/agents/validator/src/submit/tests.rs(8 hunks)rust/main/hyperlane-base/src/db/mod.rs(1 hunks)rust/main/hyperlane-base/src/db/rocks/hyperlane_db.rs(1 hunks)rust/main/hyperlane-base/src/settings/checkpoint_syncer.rs(1 hunks)rust/main/hyperlane-core/src/types/checkpoint.rs(1 hunks)rust/main/hyperlane-core/src/types/reorg.rs(1 hunks)
💤 Files with no reviewable changes (1)
- rust/main/agents/relayer/src/msg/pending_message.rs
🧰 Additional context used
📓 Path-based instructions (4)
rust/main/{hyperlane-core,hyperlane-base}/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Keep shared Rust core crates in rust/main/{hyperlane-core,hyperlane-base}
Files:
rust/main/hyperlane-core/src/types/checkpoint.rsrust/main/hyperlane-base/src/db/mod.rsrust/main/hyperlane-base/src/db/rocks/hyperlane_db.rsrust/main/hyperlane-base/src/settings/checkpoint_syncer.rsrust/main/hyperlane-core/src/types/reorg.rs
rust/main/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Run cargo clippy for Rust code linting
Files:
rust/main/hyperlane-core/src/types/checkpoint.rsrust/main/hyperlane-base/src/db/mod.rsrust/main/hyperlane-base/src/db/rocks/hyperlane_db.rsrust/main/agents/relayer/src/msg/db_loader.rsrust/main/agents/validator/src/submit/tests.rsrust/main/agents/validator/src/submit.rsrust/main/hyperlane-base/src/settings/checkpoint_syncer.rsrust/main/hyperlane-core/src/types/reorg.rs
rust/main/agents/relayer/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain relayer agent Rust sources under rust/main/agents/relayer
Files:
rust/main/agents/relayer/src/msg/db_loader.rs
rust/main/agents/validator/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain validator agent Rust sources under rust/main/agents/validator
Files:
rust/main/agents/validator/src/submit/tests.rsrust/main/agents/validator/src/submit.rs
🧠 Learnings (1)
📚 Learning: 2025-09-02T18:44:06.598Z
Learnt from: CR
PR: hyperlane-xyz/hyperlane-monorepo#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T18:44:06.598Z
Learning: Applies to rust/main/chains/{hyperlane-ethereum,hyperlane-cosmos,hyperlane-sealevel,hyperlane-fuel}/**/src/**/*.rs : Keep chain support implementations within rust/main/chains/{hyperlane-ethereum,hyperlane-cosmos,hyperlane-sealevel,hyperlane-fuel}
Applied to files:
rust/main/agents/validator/src/submit/tests.rs
🧬 Code graph analysis (4)
rust/main/agents/relayer/src/msg/db_loader.rs (1)
rust/main/hyperlane-core/src/test_utils.rs (1)
dummy_domain(44-53)
rust/main/agents/validator/src/submit/tests.rs (2)
rust/main/hyperlane-core/src/test_utils.rs (1)
dummy_domain(44-53)rust/main/agents/validator/src/submit.rs (2)
new(42-66)new(485-499)
rust/main/agents/validator/src/submit.rs (4)
rust/main/chains/hyperlane-ethereum/src/contracts/merkle_tree_hook.rs (1)
tree(301-311)rust/main/chains/hyperlane-starknet/src/merkle_tree_hook.rs (2)
tree(95-130)tree(107-111)rust/main/chains/hyperlane-sealevel/src/merkle_tree_hook.rs (1)
tree(20-27)rust/main/hyperlane-core/src/traits/merkle_tree_hook.rs (1)
tree(37-37)
rust/main/hyperlane-base/src/settings/checkpoint_syncer.rs (1)
rust/main/hyperlane-core/src/chain.rs (1)
from_blocks(53-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: infra-test
- GitHub Check: build-and-push-to-gcr
- GitHub Check: e2e-matrix (cosmwasm)
- GitHub Check: e2e-matrix (sealevel)
- GitHub Check: e2e-matrix (starknet)
- GitHub Check: e2e-matrix (radix)
- GitHub Check: e2e-matrix (evm)
- GitHub Check: e2e-matrix (cosmosnative)
- GitHub Check: test-rs
- GitHub Check: lint-rs
- GitHub Check: lander-coverage
🔇 Additional comments (7)
rust/main/hyperlane-core/src/types/checkpoint.rs (1)
7-7: LGTM on the import shiftDropping CheckpointInfo dependencies here aligns with the revert. No functional changes to signing hash. Smells right.
rust/main/agents/relayer/src/msg/db_loader.rs (1)
425-426: Import tweak is fineBringing in UniqueIdentifier fits the mock API changes. No ogre-sized concerns.
rust/main/hyperlane-base/src/settings/checkpoint_syncer.rs (1)
210-217: Test update to checkpoint_index looks correctSwitch to the unified
checkpoint_indexfield is consistent with the core type change; the rest of the struct fields stay intact.Please run tests touching ReorgEvent to confirm no stale field names remain.
rust/main/hyperlane-base/src/db/rocks/hyperlane_db.rs (1)
8-12: Imports aligned with encode/decode helpersAdding
Encode/DecodeandUniqueIdentifiermatches usage below. All good from the bog.rust/main/hyperlane-base/src/db/mod.rs (1)
6-8: Import cleanup verified cleanThe sweep turned up nothin'—no stragglers lurking about. All those checkpoint-related types got properly removed, no loose ends left in the swamp. The import realignment is solid and complete.
rust/main/hyperlane-core/src/types/reorg.rs (1)
13-15: Docs and field rename look goodSingle
checkpoint_indexis clearer and matches validator usage. Smells fresh.rust/main/agents/validator/src/submit/tests.rs (1)
226-227: Assertion update matches new APIUsing
reorg_event.checkpoint_indexis correct here.
ameten
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7252 +/- ##
=====================================
Coverage 0.00% 0
=====================================
Files 1 0 -1
Lines 14 0 -14
=====================================
+ Misses 14 0 -14
🚀 New features to boost your workflow:
|
Reverts #7212
Summary by CodeRabbit