fix(engine-api): preserve finalized tags during safe imports

[panos-xyz]

Summary

• keep FCU safe at zero so unsafe imports do not advance the RPC-visible safe tag
• preserve finalized cleanup by forwarding L1 finalized when present, otherwise using the lagged synthetic fallback
• add regression coverage for unsafe imports and stale safe reorg handling

Why

Morph main validator mode may not run the external BlockTagService, so reth still needs a non-zero finalized bound for engine-tree cleanup. At the same time, FCU safe must not be synthesized from the parent because unsafe imports would incorrectly mark their parent safe, and cached safe hashes can break parent-hash reorgs.

Verification

• cargo fmt --check
• cargo test -p morph-engine-api --lib
• cargo test -p morph-node --features test-utils --test it engine::

Summary by CodeRabbit

• Bug Fixes

  • Ensure forkchoice only advances the finalized tag from L1-derived finalized information; RPC-assembled new L2 blocks no longer advance the RPC-visible safe tag or seed finalized.
  • Prevent stale safe tags from persisting across reorgs.

• Tests

  • Added integration and regression tests validating L2 import behavior, safe-tag semantics, and forkchoice updates.

[coderabbitai]

Warning

Review limit reached

@panos-xyz, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 14 minutes. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 28e1afde-a601-444a-96bd-8442798b01ba

📥 Commits

Reviewing files that changed from the base of the PR and between 88a8632 and b41738e.

📒 Files selected for processing (2)

• crates/engine-api/src/builder.rs
• crates/node/tests/it/engine.rs

📝 Walkthrough

Walkthrough

Block-tag tracking is simplified to cache only an L1-derived finalized hash via BlockTagTracker::record_finalized_hash; safe tags are advanced from imports but not seeded into finalized. FCU imports set safe_block_hash = B256::ZERO and use the cached L1-finalized hash (or zero) via resolve_fcu_finalized_hash. Tests assert unsafe imports don't advance safe and reorgs don't retain stale safe state.

Changes

Finalized-only FCU tag forwarding

Layer / File(s)	Summary
Imports and doc tweak crates/engine-api/src/builder.rs	Adjust module imports and documentation to reflect finalized-only tracking and updated provider trait bounds.
BlockTagTracker refactor to finalized-only crates/engine-api/src/builder.rs	Replace dual safe+finalized cache with RwLock<Option<B256>> finalized-only cache. Add record_finalized_hash(finalized_hash: Option<B256>) and private accessor; update unit tests for finalized-only behavior.
new_safe_l2_block: advance safe only crates/engine-api/src/builder.rs	new_safe_l2_block calls set_block_tags(block_hash, B256::ZERO) so safe advances while finalized is not seeded during imports.
set_block_tags: persist finalized only crates/engine-api/src/builder.rs	set_block_tags maps zero finalized inputs to None and calls record_finalized_hash, removing prior safe+finalized caching.
FCU import: safe=ZERO, finalized from cache crates/engine-api/src/builder.rs	import_l2_block_via_engine rebuilds forkchoice with safe_block_hash = B256::ZERO and finalized_block_hash from resolve_fcu_finalized_hash (cached L1 tag or zero). Removes age/time fallback and safe-resolution logic; unit tests updated.
Integration tests for safe semantics and reorgs crates/node/tests/it/engine.rs	Adds new_l2_block_does_not_advance_safe_tag and safe_block_reorg_does_not_carry_stale_safe_into_forkchoice to validate that unsafe imports don't advance RPC-visible safe and reorgs don't leave stale safe in forkchoice.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• morph-l2/morph-reth#116: Modifies the same set_block_tags/safe-finalized handling path in builder.rs with related tag update changes.
• morph-l2/morph-reth#124: Touches FCU/forkchoice and BlockTagTracker logic similar to this PR's finalized-tag simplification.
• morph-l2/morph-reth#35: Earlier engine API refactor introducing engine_setBlockTags and related wiring that this PR builds upon.

Suggested reviewers

• chengwenxi
• anylots

Poem

🐰 I cached a finalized hop, left safe at resting zero,
L1 whispers final, FCU builds from what we know.
Unsafe blocks may leap, but RPC safe stays meek,
Reorgs swap the branches — no stale safe to keep!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The PR title claims to 'preserve finalized tags during safe imports,' but the actual primary change is to fix FCU safe handling by keeping it at zero during unsafe imports, with finalized handling being secondary.	Consider retitling to reflect the main fix, such as 'fix(engine-api): keep FCU safe zero during unsafe imports' or 'fix(engine-api): prevent unsafe imports from advancing RPC safe tag.'

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/engine-fcu-safe-tag

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

crates/engine-api/src/builder.rs (1)

93-95: ⚡ Quick win

Update stale safe-tag documentation to match FCU behavior.

This comment says FCU safe is derived from the head’s parent, but Line 917 now unconditionally sends B256::ZERO for safe_block_hash. Keeping contradictory docs in this path is risky for future changes.

♻️ Suggested doc fix

-/// Only finalized is cached: the FCU's safe hash is derived from the head's parent
-/// (always a canonical ancestor), not from a cache, because a cached safe recorded
-/// per import would be left dangling by a `new_safe_l2_block(parent_hash)` reorg.
+/// Only finalized is cached.
+///
+/// FCU safe is intentionally sent as `B256::ZERO` (not cached and not derived from
+/// parent) so unsafe imports never advance the RPC-visible safe tag. Safe is advanced
+/// only through `set_block_tags` / `new_safe_l2_block`.

🤖 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/builder.rs` around lines 93 - 95, The doc comment about
the FCU safe being derived from the head's parent is now stale because the code
unconditionally sends B256::ZERO for safe_block_hash (see the usage of
safe_block_hash and B256::ZERO in the FCU interaction); update the comment in
builder.rs near the FCU/safe export to state that safe_block_hash may be
B256::ZERO and is currently sent unconditionally (rather than claiming it’s
derived from head’s parent), and mention the rationale briefly (unconditional
zero sentinel) so the doc matches the behavior of the safe_block_hash variable
and the code path that constructs the FCU message.

🤖 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/node/tests/it/engine.rs`:
- Around line 406-420: After creating header_2a and computing block2a_hash, add
a precondition assertion that the provider actually marked 2A as the safe block
before you construct safe_2b; call the same client.request RPC pattern to fetch
the provider's current safe/canonical L2 block (the endpoint used elsewhere in
tests) and assert its hash equals block2a_hash so the test cannot false-pass
when 2A was never promoted to safe prior to the reorg attempt.

---

Nitpick comments:
In `@crates/engine-api/src/builder.rs`:
- Around line 93-95: The doc comment about the FCU safe being derived from the
head's parent is now stale because the code unconditionally sends B256::ZERO for
safe_block_hash (see the usage of safe_block_hash and B256::ZERO in the FCU
interaction); update the comment in builder.rs near the FCU/safe export to state
that safe_block_hash may be B256::ZERO and is currently sent unconditionally
(rather than claiming it’s derived from head’s parent), and mention the
rationale briefly (unconditional zero sentinel) so the doc matches the behavior
of the safe_block_hash variable and the code path that constructs the FCU
message.

🪄 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: 1f211d35-32fe-4e10-b9c2-13e5ba884a84

📥 Commits

Reviewing files that changed from the base of the PR and between b6c47fb and f03cb53.

📒 Files selected for processing (2)

• crates/engine-api/src/builder.rs
• crates/node/tests/it/engine.rs

[panos-xyz]

Verification against the pinned reth v2.2.0

I checked the PR's central premise — "a zero/unset finalized in the FCU nulls out reth's pruning bound, stalling changeset/sidechain cleanup" — against the exact pinned reth commit, not the local working tree.

⚠️ Methodology note: a local paradigmxyz/reth checkout can be far ahead of the tag we depend on (mine was v2.2.0-182-g192c7824b6, 182 commits ahead). Cargo.lock pins us to ?tag=v2.2.0#88505c7fcbfdebfd3b56d88c86b62e950043c6c4, so all quotes below are from git show v2.2.0:….

Finding: the premise does not hold at v2.2.0

1. Changeset eviction does NOT require a finalized tag. crates/engine/tree/src/tree/mod.rs (on_persistence_complete, v2.2.0 L1483-1504):

let min_threshold = last_persisted_block_number.saturating_sub(CHANGESET_CACHE_RETENTION_BLOCKS); // 64
let eviction_threshold =
    if let Some(finalized) = self.canonical_in_memory_state.get_finalized_num_hash() {
        finalized.number.min(min_threshold)   // finalized set: min(finalized, tip-64)
    } else {
        min_threshold                          // finalized UNSET: tip-64 — eviction STILL RUNS
    };
self.changeset_cache.evict(eviction_threshold);

The constant's own doc (L94) says retention works "even when the finalized block is not set (e.g., on L2s like Optimism)."

2. Canonical in-memory block cleanup is unconditional; finalized only gates non-canonical sidechain pruning. crates/engine/tree/src/tree/state.rs (remove_until, v2.2.0 L322-356):

self.remove_canonical_until(upper_bound.number, last_persisted_hash);   // unconditional — no finalized needed
if let Some(finalized_num_hash) = finalized_num_hash {
    self.prune_finalized_sidechains(finalized_num_hash);                // ONLY this needs finalized
}

last_valid_finalized() (forkchoice.rs) returns None for a zero hash, so with a zero finalized only prune_finalized_sidechains is skipped — changeset eviction and canonical block removal proceed at tip-64.

morph-reth uses the default TreeConfig (non-opstack), so CHANGESET_CACHE_RETENTION_BLOCKS = 64 and the default persistence threshold are in force exactly as above.

Consequences

• The ~120-line synthetic-fallback-finalized mechanism is not needed for its stated changeset/memory goal — that cleanup runs without any finalized.
• Its only genuine effect is enabling prune_finalized_sidechains (eviction of abandoned non-canonical forks) and keeping RPC finalized non-zero. Sidechain growth is negligible in normal Morph operation (centralized sequencer, few/no reorgs).
• Worse, because the synthetic finalized is anchored FCU_FALLBACK_FINALIZE_LAG = 128 behind head (i.e. below tip-64), it makes eviction_threshold = finalized.number < tip-64, so it actually retains MORE changesets than an unset finalized would — mildly counter-productive to the goal it cites.

Related risk this introduces

update_finalized_block (v2.2.0 mod.rs L3092-3124) returns OnForkChoiceUpdated::invalid_state() when the finalized hash is not a canonical ancestor after the head is canonicalized. In degraded mode (no BlockTagService), the synthetic finalized is persisted ~128 blocks behind the peak head; the monotonic target > current guard never lowers it, and reth never lowers finalized on a reorg. A corrective reorg deeper than the lag (a multi-batch L1 reorg, or a future V2/centralized-sequencer reorg that replaces >128 blocks) would leave the FCU carrying a non-canonical finalized → invalid_state → the import fails and re-fails on every retry → permanent stall. Not triggerable by today's forward-only derivation, but it is a latent footgun the mechanism creates.

Suggestion

Consider dropping the synthetic fallback and simply sending finalized = B256::ZERO in degraded mode (matching the old near-live behavior). reth still evicts changesets and reclaims canonical in-memory blocks at tip-64 without it, and ZERO is a no-op in update_finalized_block so it can never cause the reorg stall. The genuinely correct parts of this PR (FCU safe = ZERO, and new_safe_l2_block no longer seeding finalized = head) stand on their own and can be kept.

[panos-xyz]

Historical context: the premise was valid in older reth — but it was fixed upstream before v2.2.0

To be fair to the design rationale: the "zero finalized stalls changeset eviction" premise was literally true in older reth. The changeset eviction used to be gated entirely on if let Some(finalized), so an unset finalized meant the changeset cache was never evicted (unbounded growth).

Before reth commit 22a68756c71 (crates/engine/tree/src/tree/mod.rs, ~L1381):

// Evict trie changesets for blocks below the finalized block, but keep at least 64 blocks
if let Some(finalized) = self.canonical_in_memory_state.get_finalized_num_hash() {
    let min_threshold = last_persisted_block_number.saturating_sub(64);
    let eviction_threshold = finalized.number.min(min_threshold);
    ...
    self.changeset_cache.evict(eviction_threshold);
}
// no else branch — with finalized unset, the changeset cache was NOT evicted

That was fixed by reth #21354 — fix(tree): evict changeset cache even when finalized block is unset (commit 22a68756c71, 2026-01-23), which added the else { min_threshold } branch so eviction always runs at tip-64.

Timeline:

	date
reth #21354 (adds the finalized-unset eviction fallback)	2026-01-23
pinned v2.2.0 tag (88505c7)	2026-04-29

git merge-base --is-ancestor 22a68756c71 v2.2.0 → YES — the fix predates our pinned tag by ~3 months and is included in it.

So: the rationale was defending against a real reth behavior, but that behavior was already resolved upstream before the version we pin. Against v2.2.0 specifically, reth evicts changesets at tip-64 regardless of finalized, so the synthetic-finalized mechanism is no longer needed for changeset/memory cleanup. (Canonical in-memory block reclamation was always unconditional; only the changeset-cache path ever had the finalized gate.)