fix(l2): preserve storage backup in undo_last_transaction#6510
fix(l2): preserve storage backup in undo_last_transaction#6510
Conversation
…_transaction restores SSTORE changes. The L2Hook's finalize_non_privileged_execution was clearing call_frame_backup to scope its Phase 2 rollback, but this destroyed the SSTORE backup before BackupHook could save it into tx_backup. As a result, undo_last_transaction restored account info (balances, nonces) but silently lost all storage slot changes. Fix: save the execution backup with std::mem::take before Phase 2 begins, then merge it back after Phase 2 succeeds. Phase 2's rollback still only undoes Phase 2 mutations (same behavior as before), but BackupHook now sees both execution and finalize backups. This is critical for the Credible Layer integration where transactions are executed speculatively during block building and undone if the sidecar rejects them.
Lines of code reportTotal lines added: Detailed view |
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
instead of letting other's inner map wholesale replace self's. Previously, if both backups recorded slots for the same address, plain HashMap::extend on original_account_storage_slots would drop any slots only present in self, even when other's inner map contained unrelated slots. This affects both the L2Hook finalize merge path and BackupHook::finalize_execution, which both rely on extend to combine temporally-separate backups into a single tx-level undo snapshot. With the fix, per-slot merging preserves every slot backed up by either side (other wins per duplicate slot key, matching the "older backup has more original values" semantics already expected for original_accounts_info).
…sion test. This is the complement to finalize_mutation_failure_reverts_all_changes: that test asserts Phase 2 mutations ARE rolled back on finalize failure; this one asserts execution-time storage writes are NOT. Together they lock in the invariant that restore_cache_state on Phase 2 error is scoped to Phase 2's backup only — a future refactor that merged the saved execution_backup into call_frame_backup before the error check would regress this test. Also extract non_privileged_l2_env helper and reuse it in both tests to remove the ~25-line Environment block duplicated between them.
…per address" This reverts commit a4f7e45.
…orage_slots. Collapses another ~25 lines of duplicated Environment setup to a single call — base_fee == gas_price and priority_fee = 0 maps to non_privileged_l2_env(.., gas_price, gas_price).
It asserted that execution-time SSTOREs remain in the DB when Phase 2 finalize errors — but that behavior depends on the caller-discards-VM-on-Err contract and isn't a guarantee worth locking in. A future strengthening of the rollback (undo execution mutations on Phase 2 error too, closer to classical EVM semantics) would have been falsely flagged as a regression by this test. finalize_mutation_failure_reverts_all_changes already covers the useful assertion (Phase 2 mutations ARE rolled back).
…ess instead of letting other's inner map wholesale replace self's. Previously, if both backups recorded slots for the same address, plain HashMap::extend on original_account_storage_slots would drop any slots only present in self, even when other's inner map contained unrelated slots. This affects both the L2Hook finalize merge path and BackupHook::finalize_execution, which both rely on extend to combine temporally-separate backups into a single tx-level undo snapshot. With the fix, per-slot merging preserves every slot backed up by either side (other wins per duplicate slot key, matching the "older backup has more original values" semantics already expected for original_accounts_info).
🤖 Kimi Code ReviewThis PR fixes a critical storage rollback bug in the L2 hook where Review SummaryCorrectness: ✅ Correct. The per-slot merge in Security/Consensus: ✅ Critical fix. The previous behavior caused Code Quality: Excellent. The use of Detailed Analysis1.
|
🤖 Codex Code ReviewNo findings. The fix looks consistent with the hook ordering: The added regression in l2_hook_tests.rs covers the reported storage-loss path well. I could not run the targeted tests here because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR fixes a silent data-corruption bug in The Confidence Score: 5/5Safe to merge; the fix is correct, well-scoped, and directly validated by the new regression test. All findings are P2 style suggestions. The core logic — using No files require special attention.
|
| Filename | Overview |
|---|---|
| crates/vm/levm/src/hooks/l2_hook.rs | Core fix: replaces clear() with std::mem::take + post-Phase-2 merge, correctly preserving SSTORE backups for undo_last_transaction while keeping Phase 2 rollback semantics intact. |
| crates/vm/levm/src/call_frame.rs | Improves CallFrameBackup::extend from coarse outer-map replacement to per-slot merging, preventing a secondary data loss when two phases touched different slots of the same address. |
| test/tests/levm/l2_hook_tests.rs | Adds targeted regression test that verifies storage-slot restoration after undo_last_transaction; also extracts a non_privileged_l2_env helper to reduce boilerplate in existing tests. |
Sequence Diagram
sequenceDiagram
participant L2Hook
participant VM as VM (call_frame_backup)
participant BackupHook
Note over L2Hook,BackupHook: BEFORE fix
L2Hook->>VM: call_frame_backup.clear() ← destroys SSTORE data
L2Hook->>VM: apply_finalize_mutations() (Phase 2)
BackupHook->>VM: clone call_frame_backup → tx_backup (missing storage!)
Note over L2Hook,BackupHook: AFTER fix
L2Hook->>VM: execution_backup = mem::take(call_frame_backup)
Note over VM: call_frame_backup is now empty
L2Hook->>VM: apply_finalize_mutations() (Phase 2 records into empty backup)
alt Phase 2 fails
L2Hook->>VM: restore_cache_state() (only Phase 2 reverted)
else Phase 2 succeeds
L2Hook->>VM: call_frame_backup.extend(execution_backup)
Note over VM: backup has BOTH execution + Phase 2 entries
BackupHook->>VM: clone call_frame_backup → tx_backup (complete!)
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: test/tests/levm/l2_hook_tests.rs
Line: 229
Comment:
**Potential panic on underflow in test helper**
`gas_price - base_fee_per_gas` is a bare `u64` subtraction. In debug builds, passing `base_fee_per_gas > gas_price` panics; in release builds it wraps silently. Since this helper will likely attract more callers, `saturating_sub` makes the invariant self-documenting without changing existing behaviour.
```suggestion
tx_max_priority_fee_per_gas: Some(U256::from(gas_price.saturating_sub(base_fee_per_gas))),
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Fix CallFrameBackup::extend to merge inn..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| base_fee_per_gas: U256::from(base_fee_per_gas), | ||
| base_blob_fee_per_gas: U256::from(1), | ||
| gas_price: U256::from(gas_price), | ||
| tx_max_priority_fee_per_gas: Some(U256::from(gas_price - base_fee_per_gas)), |
There was a problem hiding this comment.
gas_price - base_fee_per_gas is a bare u64 subtraction. In debug builds, passing base_fee_per_gas > gas_price panics; in release builds it wraps silently. Since this helper will likely attract more callers, saturating_sub makes the invariant self-documenting without changing existing behaviour.
| tx_max_priority_fee_per_gas: Some(U256::from(gas_price - base_fee_per_gas)), | |
| tx_max_priority_fee_per_gas: Some(U256::from(gas_price.saturating_sub(base_fee_per_gas))), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/tests/levm/l2_hook_tests.rs
Line: 229
Comment:
**Potential panic on underflow in test helper**
`gas_price - base_fee_per_gas` is a bare `u64` subtraction. In debug builds, passing `base_fee_per_gas > gas_price` panics; in release builds it wraps silently. Since this helper will likely attract more callers, `saturating_sub` makes the invariant self-documenting without changing existing behaviour.
```suggestion
tx_max_priority_fee_per_gas: Some(U256::from(gas_price.saturating_sub(base_fee_per_gas))),
```
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewHere is the review: PR #6510 Review — fix(l2): preserve storage backup in undo_last_transactionOverall: The fix is correct and addresses a real state-corruption bug. The root cause analysis is accurate, and the solution is clean. A few issues worth flagging before merging.
|
| Severity | Item |
|---|---|
| Medium | bal_checkpoint from execution_backup is silently dropped in extend; correctness for BAL restoration in undo_last_transaction should be verified and documented. |
| Medium | Hook execution order is a silent, load-bearing invariant — add a comment in l2_hooks(). |
| Low | extend "other wins" semantics are undocumented and asymmetric with merge_call_frame_backup_with_parent; add a doc comment. |
| Low | gas_price - base_fee_per_gas in non_privileged_l2_env can panic; use saturating_sub or assert. |
| Info | Phase 2 failure silently drops execution_backup; the safety argument depends on VM-scoped cache and should be noted. |
Automated review by Claude (Anthropic) · sonnet · custom prompt
There was a problem hiding this comment.
Pull request overview
Fixes an L2 rollback correctness issue where undo_last_transaction could restore account fields but miss storage slot changes because L2Hook::finalize_non_privileged_execution cleared call_frame_backup before BackupHook captured the tx snapshot.
Changes:
- Preserve execution-time
call_frame_backupacross L2 finalize “Phase 2” bymem::take+ merge-back so tx-level undo sees full backup data. - Fix
CallFrameBackup::extendto merge per-address storage-slot maps without dropping slots when both backups touch the same address. - Add a regression test ensuring
undo_last_transaction()restores SSTORE’d storage slots.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/vm/levm/src/hooks/l2_hook.rs |
Preserves and re-merges execution backups around Phase 2 finalize mutations so BackupHook can snapshot storage changes for tx undo. |
crates/vm/levm/src/call_frame.rs |
Corrects backup merging to avoid losing storage-slot entries for the same address during extend. |
test/tests/levm/l2_hook_tests.rs |
Adds helper for non-privileged L2 env setup and a regression test for storage restoration via undo_last_transaction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gas_price: U256::from(gas_price), | ||
| tx_max_priority_fee_per_gas: Some(U256::from(gas_price - base_fee_per_gas)), | ||
| tx_max_fee_per_gas: Some(U256::from(gas_price)), | ||
| block_gas_limit: gas_limit * 2, |
There was a problem hiding this comment.
gas_price - base_fee_per_gas can underflow (panic in debug, wrap in release) if a future test calls non_privileged_l2_env with gas_price < base_fee_per_gas. Consider using saturating_sub, checked_sub with an expect, or asserting gas_price >= base_fee_per_gas to make the helper robust.
ElFantasma
left a comment
ElFantasma
left a comment
There was a problem hiding this comment.
Just one suggestion. It LGTM
| @@ -326,8 +326,14 @@ impl CallFrameBackup { | |||
| } | |||
|
|
|||
| pub fn extend(&mut self, other: CallFrameBackup) { | |||
There was a problem hiding this comment.
The overwrite-on-duplicate behavior here is correct (callers always pass the older/more-original backup as other, so its values win for duplicate slots → truly-original is preserved), but that invariant is non-obvious. Worth documenting:
/// Merges `other` into `self`, per-address. For slots present in both,
/// `other`'s values win. Callers MUST pass the older/more-original backup
/// as `other` so the truly-original value is preserved (matches the
/// `or_insert` semantic in `backup_storage_slot`).
Both current call sites (l2_hook::finalize_non_privileged_execution passing execution_backup and backup_hook::finalize_execution passing pre_execution_backup) follow this — but a future caller could easily get it wrong without the comment.
…o a future caller doesn't pass arguments in the wrong order. Both current callers (L2Hook merging execution_backup, BackupHook merging pre_execution_backup) rely on the older/more-original backup being passed as `other` to preserve the truly-original value, matching backup_storage_slot's or_insert behavior. Also switch non_privileged_l2_env from a bare `gas_price - base_fee_per_gas` (debug-panic / release-wrap on underflow) to saturating_sub, since the helper will likely attract more callers with arbitrary fee values.
… `#[cfg(feature = "l2")]` in test/tests/l2/utils.rs. The function's only callers (shared_bridge.rs, integration_tests.rs) are already feature-gated, so the function and its imports were emitting dead-code / unused-imports warnings on default builds.
… changes: the v11.0.0 workspace version bump (#6571), the --no-precompile-cache CLI flag for benchmarking (#6562), and the L2Hook storage backup fix in undo_last_transaction (#6510). The only conflict was in test/tests/l2/utils.rs, where both branches gated read_env_file_by_config behind #[cfg(feature = "l2")] using different styles. Kept the local approach (single cfg on the function with use statements inside) over the per-import cfg gates from main, since both achieve the same outcome and the local form is less duplicative. test/tests/levm/l2_hook_tests.rs was auto-merged cleanly: the local refactor that extracted TestDatabase into super::test_db combines correctly with the new non_privileged_l2_env helper and the undo_last_transaction_restores_storage_slots regression test from main.
Motivation
undo_last_transactionsilently fails to restore storage slot changes on L2, corrupting state when transactions are speculatively executed and then rolled back (e.g., Credible Layer integration).The root cause:
L2Hook::finalize_non_privileged_executionclearscall_frame_backupat line 151 to scope its Phase 2 rollback, but this also destroys the SSTORE backup beforeBackupHook::finalize_executioncan capture it intotx_backup. The result is thatundo_last_transactionrestores account info (balances, nonces) but silently loses all storage slot changes.Description
Instead of clearing the backup, save it with
std::mem::takebefore Phase 2 begins, then merge it back after Phase 2 succeeds. This preserves the Phase 2 rollback semantics (on error, only Phase 2 mutations are reverted) while ensuringBackupHooksees the complete execution + finalize backup.How to Test
New regression test
undo_last_transaction_restores_storage_slots: creates a contract withslot[0] = 42, writes0xDEAD, callsundo_last_transaction(), asserts the slot is restored. Without the fix, it remains0xDEAD.Checklist
l2_hook_testspass (5/5)cargo fmtcleancargo clippyclean (1 pre-existing warning unrelated to this change)