Conversation
karlb
force-pushed
the
karlb/remove-need-for-revm-fork
branch
from
f7883a5 to
601f0d3
Compare
This is just a temporary measure until we switch to an revm version including bluealloy/revm#3160.
Keep all state changes in the EVM journal instead of using set_storage: - System calls execute without calling finalize() - State changes remain in journal for main transaction to see - Add call_read_only wrapper using checkpoint/revert to prevent accidental state changes in query operations - Apply read-only wrapper to get_balance, get_currencies, get_exchange_rates, get_intrinsic_gas This eliminates the need for manual state merging and the revm fork.
karlb
force-pushed
the
karlb/remove-need-for-revm-fork
branch
from
601f0d3 to
47a4a6c
Compare
karlb
changed the base branch from
seolaoh/apply-hokulea
to
release/v1.0.0-rc.4
Now that we use neither `Account.mark_cold` nor `set_storage`, we can go back to using upstream revm.
karlb
force-pushed
the
karlb/remove-need-for-revm-fork
branch
from
47a4a6c to
322db8d
Compare
| // Clear transient storage (EIP-1153) | ||
| evm.ctx().journal_mut().transient_storage.clear(); | ||
| // Restore transaction_id for correct warm/cold accounting | ||
| evm.ctx().journal_mut().transaction_id = prev_transaction_id; |
There was a problem hiding this comment.
Do we need to do other operations in evm.finalize() as well?
// Clear coinbase address warming for next tx
warm_addresses.clear_coinbase_and_access_list();
let state = mem::take(state); // clear the state in journal
logs.clear();
*depth = 0;
There was a problem hiding this comment.
Also previously we reset the transaction_id to 0 when finalize(), but now we restore the prev_transaction_id. Does this change the behavior in any way?
There was a problem hiding this comment.
These are good points. I'll have to take a closer look at these.
There was a problem hiding this comment.
- warm_addresses.clear_coinbase_and_access_list: This is already
called in commit_tx, which is called inexecution_result. - clearing
state: We want to keep the state, otherwise we'll have to revert back to the state merging and use set_state again - logs: We could clear the logs, but they are already cleared in
post_execution::output, so it would not change anything. Should we assert an empty state to quickly notice when our assumptions are broken? - depth: The checkpointing works on relative depth values, so I don't think resetting the depths has an effect. We could add
depth = 0for consistency withfinalizeor we could keep the current state to avoid meddling with the internals more than necessary. - transaction_id: We need to go back to the transaction_id use in the debit to preserve the warm addresses (which are tracked by transaction_id), otherwise we would have to do state merging for the warm addresses again. This is my least favorite part of this PR, but I don't see a better way to do it.
| //! Core contract interactions for Celo system calls. | ||
| //! | ||
| //! System calls are executed WITHOUT calling `finalize()`, keeping state changes in the EVM's journal. | ||
| //! If the main transaction reverts, debit changes persist the main tx is executed in a subcall. |
There was a problem hiding this comment.
I didn't quite catch it, could you elaborate a bit more?
There was a problem hiding this comment.
The finalize removed all changes from the journal, so that we had to explicitly merge them back in. Now we just leave them in the journal, which is what we want for state and account changes. We don't want some other changes though, which we now explicitly have to undo. Changing the transaction_id back also sets back the warmness due to how the warmness is tracked (which is probably not the best way to do it, there's a warm_addresses.clear_coinbase_and_access_list() in my notes).
The basic thought is that we want from a "discard by default" approach to a "keep by default" approach, which allows us to avoid the set_state.
There was a problem hiding this comment.
For the main tx, it already does checkpointing and reverting as we want it without further changes, like it is done for EVM subcalls. I'll have to look up the details (the checkpoint is created in make_call_frame, but it's a lot of calls until there).
There was a problem hiding this comment.
Some more details for the main tx revert handling:
The main transaction is treated as a call frame, getting the same checkpoint/revert mechanism as any EVM subcall:
- Handler creates initial frame and calls
run_exec_loop(evm, first_frame_input): https://github.com/celo-org/revm/blob/4cb77454b9a763a05ea337a910bf65a781ab16bc/crates/handler/src/handler.rs#L196-L199 make_call_frame()creates a checkpoint and stores it in the frame: https://github.com/celo-org/revm/blob/4cb77454b9a763a05ea337a910bf65a781ab16bc/crates/handler/src/frame.rs#L168 -- Main transaction executes: The interpreter runs with all state changes recorded in the journal after the checkpoint
- Result determines commit or revert: https://github.com/celo-org/revm/blob/4cb77454b9a763a05ea337a910bf65a781ab16bc/crates/handler/src/frame.rs#L409-L413
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| let account_ptr = account_ref as *const Account as *mut Account; | ||
| unsafe { | ||
| (*account_ptr).mark_cold(); | ||
| } |
There was a problem hiding this comment.
Bug: Dangerous Dereference: Immutable Reference Mutation
Casting &Account (obtained via Deref) to *mut Account and dereferencing it invokes undefined behavior. This violates Rust's aliasing rules by mutating through a pointer derived from an immutable reference, even with exclusive access to the journal. Miri and Stacked Borrows would flag this as UB, potentially causing miscompilation or crashes.
This makes things at least a bit clearer. I'm having a hard to adding explanations for all important aspects without creating a lot of text and linking to constantly changing revm code.
3 participants
Avoids the use of methods from our revm fork, so that we can go back to upstream revm:
mark_colduntil we use a version of revm that includes feat(context): add mark_cold method to JournaledAccount bluealloy/revm#3160set_storageby moving away from our state merging approach. I prefer this approach to our previous one.Closes https://github.com/celo-org/celo-blockchain-planning/issues/1281
Note
Switches to upstream revm/op-revm and refactors core system calls and fee handling to keep state in the journal, removing fork-only APIs; adds a temporary unsafe workaround for transfer precompile coldness and updates deps.
revm/op-revm(remove[patch]git overrides); update to registry versions and refreshCargo.lock.crates/celo-revm/src/contracts/core_contracts.rs:call_read_onlyand revisedcallkeep state in the journal (nofinalize()/state merge), clear transient storage, and restoretransaction_id.ExecutionResult; avoidset_storageentirely.crates/celo-revm/src/contracts/erc20.rs): usecall_read_only/call, return(logs, gas_used)(removeEvmState) and keep journaled changes.crates/celo-revm/src/handler.rs): remove manual state merge (apply_state_to_journal), rely on journaled state; adjust CIP-64 debit/credit flow and log collection; simplify balance/nonce journaling.crates/celo-revm/src/precompiles/transfer.rs): add temporary unsafe path to callAccount::mark_cold()to restore cold status after transfers.hokulea-client/v1.0.2.reth-*crates tov1.9.1.testListREADME.mdwith additional deposit/revert and uncategorized failure scenarios.Written by Cursor Bugbot for commit fbe2e97. This will update automatically on new commits. Configure here.