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

  • Updated forkchoice tag handling so only the L1-derived finalized hash is forwarded, while the RPC-visible safe tag for RPC-assembled L2 blocks no longer advances.
  • Prevented stale safe tags from carrying over after reorgs, ensuring forkchoice reflects the latest imported safe state.

• Tests

  • Added new integration and regression coverage for L2 safe-tag semantics and forkchoice updates.
  • Adjusted assertions to validate finalized-only caching behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e44d2f36-aef7-41cc-ae70-47b3f5f1347c

📥 Commits

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

📒 Files selected for processing (2)

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

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/node/tests/it/engine.rs

---

📝 Walkthrough

Walkthrough

BlockTagTracker is refactored from dual safe+finalized caching to finalized-only storage. L2 import paths (new_l2_block, new_l2_block_v2, new_safe_l2_block) now pass safe_block_hash to import_l2_block_via_engine: unsafe imports use B256::ZERO, safe imports use the block hash. FCU forkchoice construction uses safe from the caller argument and finalized from the cached L1 tag (or zero when absent), removing age/time-based fallback logic. Tests assert unsafe imports don't advance RPC-visible safe and reorgs don't leave stale safe in forkchoice.

Changes

Finalized-only FCU tag forwarding

Layer / File(s)	Summary
BlockTagTracker refactor to finalized-only storage crates/engine-api/src/builder.rs	Replace dual safe+finalized cache with RwLock<Option<B256>> finalized-only storage. Remove record_block_tags API; add record_finalized_hash(finalized_hash: Option<B256>). Update imports, provider trait bounds, and unit tests for finalized-only behavior.
L2 import entrypoints: safe/finalized seeding crates/engine-api/src/builder.rs	Update new_l2_block, new_l2_block_v2, and new_safe_l2_block to pass safe_block_hash to import_l2_block_via_engine: unsafe imports use B256::ZERO; safe imports use the block hash. Remove finalized seeding logic.
set_block_tags: record finalized only crates/engine-api/src/builder.rs	Modify set_block_tags to call record_finalized_hash for finalized persistence, mapping B256::ZERO to None and ignoring None to preserve prior cached values. Remove prior safe+finalized dual caching.
FCU import forkchoice construction crates/engine-api/src/builder.rs	Update import_l2_block_via_engine signature to accept safe_block_hash parameter. Rewrite forkchoice construction with safe from caller argument and finalized from BlockTagTracker cache only. Remove age/time-based fallback and head-based resolution logic; update unit tests.
Integration tests for safe semantics and reorgs crates/node/tests/it/engine.rs	Add new_l2_block_does_not_advance_safe_tag test to verify unsafe imports don't advance RPC-visible safe tag. Add safe_block_reorg_does_not_carry_stale_safe_into_forkchoice test to verify reorgs don't retain stale safe in forkchoice state.

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

• anylots
• chengwenxi

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 refers to 'preserve finalized tags during safe imports,' but the core changeset centers on zeroing FCU safe tags to prevent unsafe imports from advancing RPC-visible safe, and refactoring finalized handling with a synthetic fallback mechanism. The title captures only the finalized preservation aspect and does not reflect the primary change (safe-tag zeroing) or the reviewer's critical finding that the synthetic finalized mechanism is problematic.	Revise the title to reflect the main change: e.g., 'fix(engine-api): zero FCU safe tag on non-safe imports' or 'fix(engine-api): prevent unsafe imports from advancing RPC safe tag'. If retaining finalized logic, clarify its actual purpose (sidechain pruning only, not changeset cleanup per reviewer analysis).

✅ 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 docstrings

• Create stacked PR
• Commit on current branch

🧪 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.)