feat(l1+evm+precompiles): add L1-backed zone db#698
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
0394607 to
0c94b43
Compare
0c94b43 to
604908a
Compare
|
cyclops audit |
|
cc @0xrusowsky Cyclops audit event published. View workflow run Config: config: |
👁️ Cyclops Security Review
🧭 Finalizing · mode=
Findings
⚙️ Controls
📜 48 events🔍 |
| fn execute_inner( | ||
| &mut self, | ||
| execute: impl FnOnce( | ||
| &mut TempoEvm<AnchoredZoneDb<DB, L1>, I>, | ||
| ) -> Result<TempoResult, AdaptedEvmError<DB::Error>>, | ||
| ) -> Result<TempoResult, ZoneEvmError<DB::Error>> { | ||
| let snapshot = self.anchor_controller().phase(); | ||
| let mut result = match execute(&mut self.inner) { | ||
| Ok(result) => result, | ||
| Err(error) => { | ||
| self.anchor_controller().restore(snapshot); | ||
| return Err(map_adapter_error(error)); | ||
| } | ||
| }; | ||
| if !result.result.is_success() { | ||
| self.anchor_controller().restore(snapshot); | ||
| return Ok(result); | ||
| } | ||
| if let Err(error) = self.inner.db().sanitize_state(&mut result.state) { | ||
| self.anchor_controller().restore(snapshot); | ||
| return Err(error.into_evm_error()); | ||
| } | ||
| Ok(result) | ||
| } | ||
| } |
There was a problem hiding this comment.
can we not avoid this logic? we always have advanceTempo as first tx in the block so i think entire block's execution would use the same L1 state always?
There was a problem hiding this comment.
simplified state machine logic b62c49f (this PR)
| if let Some(account) = state.get(&TIP403_REGISTRY_ADDRESS) { | ||
| if account.info != account.original_info() { | ||
| return l1_write(TIP403_REGISTRY_ADDRESS, U256::ZERO); | ||
| } | ||
| for (slot, value) in &account.storage { | ||
| if value.is_changed() { | ||
| return l1_write(TIP403_REGISTRY_ADDRESS, *slot); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
is this actually reachable given that we ban all mutating selectors? it would be kinda unfortunate if txs can execute all the way to here and only then get rejected
There was a problem hiding this comment.
should be unreachable yeah, i only added it cause it feels like an invariant worth enforcing?
There was a problem hiding this comment.
Yeah seems like a reasonable defense in depth.
| /// Restores execution-local L1 anchor state when a simulated transaction is not committed. | ||
| fn execute_transaction_with_commit_condition( | ||
| &mut self, | ||
| tx: impl ExecutableTx<Self>, | ||
| f: impl FnOnce(&Self::Result) -> CommitChanges, | ||
| ) -> Result<Option<GasOutput>, BlockExecutionError> { | ||
| let snapshot = self.evm().anchor_controller().phase(); | ||
| let output = self.execute_transaction_without_commit(tx)?; | ||
| if !f(&output).should_commit() { | ||
| self.evm().anchor_controller().restore(snapshot); | ||
| return Ok(None); | ||
| } | ||
| Ok(Some(self.commit_transaction(output))) | ||
| } |
There was a problem hiding this comment.
execute_transaction_without_commit should not advance any state either. it is expected that execute_transaction_without_commit might be called multiple times in a row with different transactions
| impl<DB: fmt::Debug, L1> fmt::Debug for AnchoredZoneDb<DB, L1> { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.debug_struct("AnchoredZoneDb") | ||
| .field("inner", &self.inner) | ||
| .field("controller", &self.controller) | ||
| .finish_non_exhaustive() | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: can we move this to after the main AnchoredZoneDb impl block?
| /// Database error produced by [`AnchoredZoneDb`]. | ||
| #[derive(Debug, Error)] | ||
| pub enum AnchoredZoneDbError<E> { | ||
| /// Error from the caller-provided database. | ||
| #[error("inner database error: {0}")] | ||
| Inner(#[source] E), | ||
| /// The selected Zone state contains an invalid Tempo anchor. | ||
| #[error("invalid Tempo anchor (does not fit in u64): {0}")] | ||
| AnchorOverflow(U256), | ||
| /// The execution-local anchor transition is inconsistent. | ||
| #[error(transparent)] | ||
| AnchorTransition(#[from] L1AnchorError), | ||
| /// Exact anchored L1 storage was unavailable. | ||
| #[error("Tempo L1 storage unavailable address={address} slot={slot} block={anchor}: {source}")] | ||
| L1Read { | ||
| address: Address, | ||
| slot: B256, | ||
| anchor: u64, | ||
| #[source] | ||
| source: PrecompileError, | ||
| }, | ||
| /// A transaction attempted to persist mirrored Tempo-owned state. | ||
| #[error("write to mirrored Tempo storage address={address} slot={slot}")] | ||
| L1Write { address: Address, slot: U256 }, | ||
| } | ||
|
|
||
| impl<E: DBErrorMarker> DBErrorMarker for AnchoredZoneDbError<E> {} | ||
|
|
||
| impl<E: DBErrorMarker> AnchoredZoneDbError<E> { |
There was a problem hiding this comment.
nit: can we move this after the main AnchoredZoneDb logic? Same thing for PackedPolicySlot.
|
|
||
| if let Some(account) = state.get(&TIP403_REGISTRY_ADDRESS) { | ||
| if account.info != account.original_info() { | ||
| return l1_write(TIP403_REGISTRY_ADDRESS, U256::ZERO); |
There was a problem hiding this comment.
nit: its a bit nicer IMO to use Err directly rather than a closure. Makes it a bit easier to read.
use AnchoredZoneDbError::L1Write;
// --snip--
return Err(L1Write { address, slot });| if let Some(account) = state.get(&TIP403_REGISTRY_ADDRESS) { | ||
| if account.info != account.original_info() { | ||
| return l1_write(TIP403_REGISTRY_ADDRESS, U256::ZERO); | ||
| } | ||
| for (slot, value) in &account.storage { | ||
| if value.is_changed() { | ||
| return l1_write(TIP403_REGISTRY_ADDRESS, *slot); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Yeah seems like a reasonable defense in depth.
| } | ||
|
|
||
| let anchor = self.anchor()?; | ||
| let l1 = self.l1_storage(address, slot, anchor)?; |
There was a problem hiding this comment.
This can return stale policy state after the first read. L1StateProvider::get_storage treats the latest cached value at or before anchor as valid, but policy events still update only PolicyCache, they do not write or invalidate the corresponding raw L1StateCache slots.
For example, after a slot is cached at N and changed on L1 at N+1, this read at N+1 returns the N value without making an RPC request. Cache warmth can then make nodes execute the same block with different policy state.
| if storage::merge_transfer_policy_id(U256::ZERO, value.present_value) | ||
| != storage::merge_transfer_policy_id(U256::ZERO, observed.l1) |
There was a problem hiding this comment.
This rejects the mandatory enableToken path whenever the token already has a non-default L1 policy.
ZoneTokenFactory::enable_token calls upstream TIP20Token::initialize, which writes transfer policy ID 1 into this packed slot. If the overlaid L1 field is anything other than 1, this comparison returns an error, causing the entire advanceTempo system transaction to fail and stalling block production.
Initialization needs an exception that discards/restores the mirrored field without treating the upstream default write as an attempted L1 mutation.
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
This replacement lookup drops the upstream RECEIVE_POLICY_GUARD_ADDRESS precompile. With L1 backed TIP403 execution, upstream TIP20 can enforce a receive policy, move blocked funds into the guard, and record a claim receipt internally.
However, users cannot call the guard's public claim or burnBlockedReceipt methods because this lookup returns None for its address, leaving those funds permanently locked. We should either explicitly reject receive policy resolution or find a different sollution.
| fn decode_and_check<C: SolCall>(args: &[u8], check: impl FnOnce(C) -> CallCheck) -> CallCheck { | ||
| match C::abi_decode_raw_validate(args) { | ||
| Ok(decoded) => check(decoded), | ||
| Err(_) => CallCheck::Continue, | ||
| } | ||
| } |
There was a problem hiding this comment.
Protected selectors must not continue when strict ABI validation fails. Upstream dispatch accepts some encodings that abi_decode_raw_validate rejects.
For example, setting a nonzero byte in an address word's upper padding makes this helper return Continue, while upstream still decodes the address and successfully serves balanceOf.
| // System txs do not pay protocol fees. Resolving their validator token here could read L1 | ||
| // policy state at N before `advanceTempo` moves the controller to N+1, causing the anchor | ||
| // advancement to fail. Thus, FeeAMM override is restricted to regular fee-paying txs. | ||
| if !tx_env.is_system_tx { | ||
| self.override_validator_token(); | ||
| } |
There was a problem hiding this comment.
Lets add a TODO to remove this once the ZoneFeeManager lands.
| block_number, | ||
| address_to_storage_value(pending_sequencer), | ||
| ); | ||
| set_cache_slot(PORTAL_SEQUENCER_SLOT, current_sequencer.into_word()); |
There was a problem hiding this comment.
Can we derive this slot (as well as other slots) from the sol! bindings rather than using a const?
| /// Canonical TIP-403 registry address, shared with Tempo L1. | ||
| pub const ZONE_TIP403_PROXY_ADDRESS: Address = TIP403_REGISTRY_ADDRESS; |
There was a problem hiding this comment.
Do we need a separate const for this or can we just use TIP403_REGISTRY_ADDRESS directly?
|
|
||
| const FIXED_TRANSFER_GAS: u64 = 100_000; | ||
| /// Fixed gas charged for TIP20 transfer and approval selectors on the zone. | ||
| pub const TIP20_FIXED_TRANSFER_GAS: u64 = 100_000; |
There was a problem hiding this comment.
Why do we need this rather than using upstream pricing?
| Some(Ok(Self::policy_forbids_output())) | ||
| } | ||
| Err(e) => Some(Err(e)), | ||
| impl TIP20Rules { |
There was a problem hiding this comment.
All of these functions are essentially the same thing. We could write one check_auth function and pass in the authorized address. For example:
fn check_auth(&self, caller: Address, authorized: Address) -> CallCheck {
if caller == authorized {
CallCheck::Continue
} else {
unauthorized()
}
}
// --snip--
ITIP20::mintCall::SELECTOR | ITIP20::mintWithMemoCall::SELECTOR => {
self.check_auth(caller, ZONE_INBOX_ADDRESS)
}
ITIP20::burnCall::SELECTOR | ITIP20::burnWithMemoCall::SELECTOR => {
self.check_auth(caller, ZONE_OUTBOX_ADDRESS)
}
// --snip--| ) -> Result<TempoResult, ZoneEvmError<DB::Error>> { | ||
| let result = match execute(&mut self.inner) { | ||
| Ok(mut result) => { | ||
| if result.result.is_success() |
There was a problem hiding this comment.
Reverts and halts still return state that the block executor commits, including the nonce and Tempo fee transition, so sanitization cannot be success only. B
Before execution, collect_fee_pre_tx authorizes the fee token transfer through TIP403 and mutates that same token's balances/reward state. Those fee effects remain when the user call later reverts.
The returned touched token account contains the L1 overlaid policy bits, but this branch skips sanitize_state, and commit_transaction persists them into canonical Zone storage.
Should we sanitize every Ok(result) regardless of ExecutionResult? The reverted user-frame writes have already been rolled back, while the persistent fee/nonce transition still needs its mirrored fields removed.
| if value.is_changed() { | ||
| return l1_write(TIP403_REGISTRY_ADDRESS, *slot); | ||
| } |
There was a problem hiding this comment.
Checking only is_changed() validates that the call did not modify the L1 value, but it does not remove that value from the returned transition. The adapter supplied the L1 value as both original_value and present_value, and revm commits every storage entry of a touched account.
For example, with canonical Zone slot 0 equal to 5 and the mirrored L1 value equal to 99, a policyIdCounter() transaction succeeds, sanitize_state returns Ok, and the touched registry account in result.state contains 99. Committing that result overwrites the Zone slot even though the transaction was read only.
cc @klkvr to double check if Im overlooking something here.
| @@ -0,0 +1,3 @@ | |||
| //! Shared test utilities for Zone EVM tests. | |||
|
|
|||
| pub(crate) use zone_precompiles::test_utils::MockL1Reader as TestL1; | |||
| @@ -65,10 +230,11 @@ pub fn zone_rpc_error(msg: impl core::fmt::Display) -> PrecompileError { | |||
| PrecompileError::Fatal(alloc::format!("{ZONE_RPC_ERROR_PREFIX} {msg}")) | |||
| } | |||
|
|
|||
| /// Returns `true` if the error string was produced by [`zone_rpc_error`]. | |||
| /// Returns `true` if the error chain contains a failure produced by [`zone_rpc_error`]. | |||
| pub fn is_zone_rpc_error(err: &str) -> bool { | |||
| err.starts_with(ZONE_RPC_ERROR_PREFIX) | |||
| err.contains(ZONE_RPC_ERROR_PREFIX) | |||
| } | |||
There was a problem hiding this comment.
What is the motivation for this? Should we have a proper error rather than [zone rpc] prefix?
| pub(crate) struct NoCallRules; | ||
| impl CallRules for NoCallRules { | ||
| fn is_delegate_call_allowed(&self) -> bool { | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you remind me why/when we need to delegate call with precompiles? We explictly restrict this upstream in Tempo via the tempo_precompile macro.
4 participants
replaces #626
High-level goal
before this PR stack, Zones had two representations of Tempo L1 state:
PolicyCache/PolicyProvider, populated from typed TIP-403 and TIP-20 events with RPC fallback. CustomZoneTip20TokenandZoneTip403ProxyRegistryimplementations used it to reproduce upstream policy behavior.(address, slot), used byTempoStateand Zone system contracts for portal storage reads.this duplicated policy semantics and left two L1-state paths that had to remain aligned in block anchoring, event decoding, invalidation, RPC fallback, and upstream behavior. The goal is to remove the semantic policy implementation from the EVM path and execute upstream Tempo code against one factory-installed L1-aware database. The database wraps the normal caller-provided EVM database and:
TempoStatetransferPolicyIdfield in TIP-20’s packed slot, preserving Zone-local fieldsfrom upstream Tempo's perspective these remain ordinary EVM storage operations, but selected slots are virtualized to finalized L1 state. This makes upstream Tempo code the source of execution semantics rather than maintaining Zone-specific policy implementations or installing a separate storage provider around each precompile call.
EVM Database
database.rsaddsAnchoredZoneDb, a database adapter installed byZoneEvmFactoryfor every EVM created for payload building, canonical import, RPC calls, tracing, transaction-pool validation, and maintenance execution. The factory uses the alloy-evm database-context extension so the internal Tempo context can execute overAnchoredZoneDb<DB>whileZoneEvmcontinues to expose and return the original caller-providedDB.this keeps Reth’s state ownership and commit path unchanged. Reads pass through the adapter during execution, while successful state transitions are validated and sanitized before Reth commits them through the original database. Inner database failures preserve their original error type; anchor, L1-read, and mirrored-write failures are propagated as typed custom EVM errors.
the adapter and native
TempoStateprecompile share one execution-local anchor controller. It tracks whether execution is still at the parent Tempo anchor or has advanced to its direct child, rejects reads at an inconsistent anchor, rejects parent-anchor reads before advancement, and rejects duplicate or non-contiguous advancement. Noncommitting payload simulations restore the controller snapshot so they cannot leak anchor state into included transaction execution.until TIP-20 policy storage moves fully into TIP-403, the adapter retains compatibility with the current packed layout. It reads the local packed word first, replaces only the L1-owned
transferPolicyIdfield for execution, and restores the local field before canonical transitions are committed. TIP-403 account or storage mutations fail closed.Precompile Execution
execution.rsnow has one execution path for Zone-native and upstream Tempo precompiles. Every wrapper receives the same EVM configuration, storage-action recorder, and non-creditable-slot set. Whether a storage read is local or L1-backed is entirely a database concern rather than a choice between local and L1-aware precompile providers.CallRulescontains only Zone-specific admission behavior:Continueor ABI-encoded revert dataTIP-20, TIP-403, and
TipFeeManagernow execute the upstream Tempo implementations directly over ordinaryEvmPrecompileStorageProviderstorage. The remaining Zone-specific behavior is limited to privacy checks, bridge mint/burn authorization, read-only TIP-403 selectors, fixed gas, and delegate-call policy.this removes
ZonePrecompileStorageProvider, the separate local/L1-backed execution orchestration, and the custom L1-awareZoneFeeManager. Tempo’s ordinary protocol fee manager now observes the same anchored policy state through the EVM database adapter.Follow-up work
PolicyProvider#701