Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: explicit behavior for nonce update persistence #729

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 39 additions & 8 deletions crates/cheatcodes/src/inspector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,39 @@ impl ArbitraryStorage {
/// List of transactions that can be broadcasted.
pub type BroadcastableTransactions = VecDeque<BroadcastableTransaction>;

/// Allows overriding nonce update behavior for the tx caller in the zkEVM.
///
/// Since each CREATE or CALL is executed as a separate transaction within zkEVM, we currently skip
/// persisting nonce updates as it erroneously increments the tx nonce. However, under certain
/// situations, e.g. deploying contracts, transacts, etc. the nonce updates must be persisted.
#[derive(Default, Debug, Clone)]
pub enum ZkPersistNonceUpdate {
/// Never update the nonce. This is currently the default behavior.
#[default]
Never,
/// Override the default behavior, and persist nonce update for tx caller for the next
/// zkEVM execution _only_.
PersistNext,
}

impl ZkPersistNonceUpdate {
/// Persist nonce update for the tx caller for next execution.
pub fn persist_next(&mut self) {
*self = Self::PersistNext;
}

/// Retrieve if a nonce update must be persisted, or not. Resets the state to default.
pub fn get(&mut self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually rename this a bit to be slightly more explicit. I mean the &mut still indicates that we will mutate it but I believe a more descriptive name would be better...
Maybe read_and_reset or get_and_clear

let persist_nonce_update = match self {
ZkPersistNonceUpdate::Never => false,
ZkPersistNonceUpdate::PersistNext => true,
};
*self = Default::default();

persist_nonce_update
}
}

/// Setting for migrating the database to zkEVM storage when starting in ZKsync mode.
/// The migration is performed on the DB via the inspector so must only be performed once.
#[derive(Debug, Default, Clone)]
Expand Down Expand Up @@ -609,9 +642,8 @@ pub struct Cheatcodes {
/// providing the necessary level of isolation.
pub persisted_factory_deps: HashMap<H256, Vec<u8>>,

/// Whether to persist nonce updates in ZK-VM. This is an option to act as a sticky flag
/// for the nonce updates.
pub zk_should_update_nonce: Option<bool>,
/// Nonce update persistence behavior in zkEVM for the tx caller.
pub zk_persist_nonce_update: ZkPersistNonceUpdate,
}

// This is not derived because calling this in `fn new` with `..Default::default()` creates a second
Expand Down Expand Up @@ -709,7 +741,7 @@ impl Cheatcodes {
persisted_factory_deps: Default::default(),
paymaster_params: None,
zk_use_factory_deps: Default::default(),
zk_should_update_nonce: None,
zk_persist_nonce_update: Default::default(),
}
}

Expand Down Expand Up @@ -1236,14 +1268,14 @@ impl Cheatcodes {
let factory_deps = self.dual_compiled_contracts.fetch_all_factory_deps(contract);
tracing::debug!(contract = contract.name, "using dual compiled contract");

let zk_persist_nonce_update = self.zk_persist_nonce_update.get();
let ccx = foundry_zksync_core::vm::CheatcodeTracerContext {
mocked_calls: self.mocked_calls.clone(),
expected_calls: Some(&mut self.expected_calls),
accesses: self.accesses.as_mut(),
persisted_factory_deps: Some(&mut self.persisted_factory_deps),
paymaster_data: self.paymaster_params.take(),
should_update_nonce: self.broadcast.is_some() ||
self.zk_should_update_nonce.take().unwrap_or_default(),
persist_nonce_update: self.broadcast.is_some() || zk_persist_nonce_update,
};

let zk_create = foundry_zksync_core::vm::ZkCreateInputs {
Expand Down Expand Up @@ -1873,8 +1905,7 @@ where {
accesses: self.accesses.as_mut(),
persisted_factory_deps: Some(&mut self.persisted_factory_deps),
paymaster_data: self.paymaster_params.take(),
should_update_nonce: self.broadcast.is_some() ||
self.zk_should_update_nonce.take().unwrap_or_default(),
persist_nonce_update: self.broadcast.is_some() || self.zk_persist_nonce_update.get(),
elfedy marked this conversation as resolved.
Show resolved Hide resolved
};

let mut gas = Gas::new(call.gas_limit);
Expand Down
4 changes: 2 additions & 2 deletions crates/evm/evm/src/executors/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ impl Executor {
pub fn call_with_env(&self, mut env: EnvWithHandlerCfg) -> eyre::Result<RawCallResult> {
let mut inspector = self.inspector().clone();
if let Some(cheatcodes) = inspector.cheatcodes.as_mut() {
cheatcodes.zk_should_update_nonce = Some(true);
cheatcodes.zk_persist_nonce_update.persist_next();
}
let mut backend = CowBackend::new_borrowed(self.backend());
let result = match &self.zk_tx {
Expand All @@ -454,7 +454,7 @@ impl Executor {
pub fn transact_with_env(&mut self, mut env: EnvWithHandlerCfg) -> eyre::Result<RawCallResult> {
let mut inspector = self.inspector.clone();
if let Some(cheatcodes) = inspector.cheatcodes.as_mut() {
cheatcodes.zk_should_update_nonce = Some(true);
cheatcodes.zk_persist_nonce_update.persist_next();
}
let backend = &mut self.backend;
let result_and_state = match self.zk_tx.take() {
Expand Down
4 changes: 2 additions & 2 deletions crates/zksync/core/src/vm/inspect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ where
info!(?call_ctx, "executing transaction in zk vm");

let initiator_address = tx.common_data.initiator_address;
let should_update_nonce = ccx.should_update_nonce;
let persist_nonce_update = ccx.persist_nonce_update;

if tx.common_data.signature.is_empty() {
// FIXME: This is a hack to make sure that the signature is not empty.
Expand Down Expand Up @@ -336,7 +336,7 @@ where
let filtered = modified_storage.iter().filter(|(k, _)| {
!(k.address() == &NONCE_HOLDER_ADDRESS &&
get_nonce_key(&initiator_address) == **k &&
!should_update_nonce)
!persist_nonce_update)
});

for (k, v) in filtered {
Expand Down
2 changes: 1 addition & 1 deletion crates/zksync/core/src/vm/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ where

let mut ccx = CheatcodeTracerContext {
persisted_factory_deps,
should_update_nonce: true,
persist_nonce_update: true,
..Default::default()
};

Expand Down
4 changes: 2 additions & 2 deletions crates/zksync/core/src/vm/tracers/cheatcode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ pub struct CheatcodeTracerContext<'a> {
pub persisted_factory_deps: Option<&'a mut HashMap<H256, Vec<u8>>>,
/// Paymaster data
pub paymaster_data: Option<ZkPaymasterData>,
/// Whether to persist the nonce updates or not
pub should_update_nonce: bool,
/// Whether to persist nonce update for the tx caller, or not.
pub persist_nonce_update: bool,
}

/// Tracer result to return back to foundry.
Expand Down