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

feat: account impersonating improvements and eth_sendTransaction #137

Closed
wants to merge 4 commits into from
Closed
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
13 changes: 13 additions & 0 deletions src/eth_test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
use jsonrpc_core::{BoxFuture, Result};
use jsonrpc_derive::rpc;
use zksync_basic_types::H256;
use zksync_types::transaction_request::CallRequest;

///
/// ETH namespace extension for the test node.
///
#[rpc]
pub trait EthTestNodeNamespaceT {
#[rpc(name = "eth_sendTransaction")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that this is implemented, can you enable the following e2e test? xdescribe -> describe

describe("hardhat_impersonateAccount & hardhat_stopImpersonatingAccount" ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Where I can find this test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

At the bottom of the following file e2e-tests/test/hardhat-api.tests.ts

You can also search for the describe(...) I posted above in the repo and it'll show up

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see it, looks like my branch is outdated

fn send_transaction(&self, tx: CallRequest) -> BoxFuture<Result<H256>>;
}
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ pub mod bootloader_debug;
pub mod configuration_api;
pub mod console_log;
pub mod deps;
pub mod eth_test;
pub mod filters;
pub mod fork;
pub mod formatter;
Expand Down
5 changes: 4 additions & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ mod cache;
mod configuration_api;
mod console_log;
mod deps;
mod eth_test;
mod evm;
mod filters;
mod fork;
Expand Down Expand Up @@ -52,6 +53,7 @@ use futures::{
use jsonrpc_core::MetaIoHandler;
use zksync_basic_types::{L2ChainId, H160, H256};

use crate::eth_test::EthTestNodeNamespaceT;
use crate::{configuration_api::ConfigurationApiNamespace, node::TEST_NODE_NETWORK_ID};
use zksync_core::api_server::web3::backend_jsonrpc::namespaces::{
eth::EthNamespaceT, net::NetNamespaceT, zks::ZksNamespaceT,
Expand Down Expand Up @@ -118,7 +120,8 @@ async fn build_json_http<

let io_handler = {
let mut io = MetaIoHandler::with_middleware(LoggingMiddleware::new(log_level_filter));
io.extend_with(node.to_delegate());
io.extend_with(EthNamespaceT::to_delegate(node.clone()));
io.extend_with(EthTestNodeNamespaceT::to_delegate(node));
io.extend_with(net.to_delegate());
io.extend_with(config_api.to_delegate());
io.extend_with(evm.to_delegate());
Expand Down
166 changes: 138 additions & 28 deletions src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
filters::{EthFilters, FilterType, LogFilter},
fork::{ForkDetails, ForkSource, ForkStorage},
formatter,
system_contracts::{self, Options, SystemContracts},
system_contracts::{self, SystemContracts},
utils::{
self, adjust_l1_gas_price_for_tx, bytecode_to_factory_dep, to_human_size, IntoBoxedFuture,
},
Expand All @@ -24,6 +24,7 @@ use std::{
sync::{Arc, RwLock},
};

use crate::eth_test::EthTestNodeNamespaceT;
use vm::{
constants::{
BLOCK_GAS_LIMIT, BLOCK_OVERHEAD_PUBDATA, ETH_CALL_GAS_LIMIT, MAX_PUBDATA_PER_BLOCK,
Expand Down Expand Up @@ -413,7 +414,14 @@ impl<S: std::fmt::Debug + ForkSource> InMemoryNodeInner<S> {

let storage = storage_view.to_rc_ptr();

let execution_mode = TxExecutionMode::EstimateFee;
let execution_mode = if self
.impersonated_accounts
.contains(&l2_tx.common_data.initiator_address)
{
TxExecutionMode::EthCall
} else {
TxExecutionMode::EstimateFee
};
let mut batch_env = self.create_l1_batch_env(storage.clone());
batch_env.l1_gas_price = l1_gas_price;
let system_env = self.create_system_env(
Expand Down Expand Up @@ -654,6 +662,15 @@ pub struct InMemoryNode<S> {
inner: Arc<RwLock<InMemoryNodeInner<S>>>,
}

// Derive doesn't work
impl<S> Clone for InMemoryNode<S> {
fn clone(&self) -> Self {
Self {
inner: self.inner.clone(),
}
}
}

fn contract_address_from_tx_result(execution_result: &VmExecutionResultAndLogs) -> Option<H160> {
for query in execution_result.logs.storage_logs.iter().rev() {
if query.log_type == StorageLogQueryType::InitialWrite
Expand Down Expand Up @@ -757,7 +774,7 @@ impl<S: ForkSource + std::fmt::Debug> InMemoryNode<S> {
log::info!("Running {:?} transactions (one per batch)", txs.len());

for tx in txs {
self.run_l2_tx(tx, TxExecutionMode::VerifyExecute)?;
self.run_l2_tx(tx)?;
}

Ok(())
Expand Down Expand Up @@ -1029,24 +1046,7 @@ impl<S: ForkSource + std::fmt::Debug> InMemoryNode<S> {

let batch_env = inner.create_l1_batch_env(storage.clone());

// if we are impersonating an account, we need to use non-verifying system contracts
let nonverifying_contracts;
let bootloader_code = {
if inner
.impersonated_accounts
.contains(&l2_tx.common_data.initiator_address)
{
tracing::info!(
"πŸ•΅οΈ Executing tx from impersonated account {:?}",
l2_tx.common_data.initiator_address
);
nonverifying_contracts =
SystemContracts::from_options(&Options::BuiltInWithoutSecurity);
nonverifying_contracts.contracts(execution_mode)
} else {
inner.system_contracts.contracts(execution_mode)
}
};
let bootloader_code = inner.system_contracts.contracts(execution_mode);
let system_env = inner.create_system_env(bootloader_code.clone(), execution_mode);

let mut vm = Vm::new(
Expand Down Expand Up @@ -1216,7 +1216,7 @@ impl<S: ForkSource + std::fmt::Debug> InMemoryNode<S> {
}

/// Runs L2 transaction and commits it to a new block.
fn run_l2_tx(&self, l2_tx: L2Tx, execution_mode: TxExecutionMode) -> Result<(), String> {
fn run_l2_tx(&self, l2_tx: L2Tx) -> Result<(), String> {
let tx_hash = l2_tx.hash();
log::info!("");
log::info!("Executing {}", format!("{:?}", tx_hash).bold());
Expand All @@ -1229,6 +1229,24 @@ impl<S: ForkSource + std::fmt::Debug> InMemoryNode<S> {
inner.filters.notify_new_pending_transaction(tx_hash);
}

let execution_mode = if self
.inner
.read()
.map_err(|e| format!("Failed to acquire read lock: {}", e))?
.impersonated_accounts
.contains(&l2_tx.common_data.initiator_address)
{
tracing::info!(
"πŸ•΅οΈ Executing tx from impersonated account {:?}",
l2_tx.common_data.initiator_address
);
// Using the eth call here to avoid all the account checks
// TODO: think about fictive blocks in case of impersonating via eth call
TxExecutionMode::EthCall
Copy link
Collaborator

Choose a reason for hiding this comment

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

the alternative, is to use the other DefaultAccount implementation in such case.

(as EthCall is in theory not supposed to be used for blocks that we commit)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but then the bootloader will do a check that the sender is an account and it's not in the kernel space

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah right, you actually want to have even more permissions...

The cool thing, is that we have a local copy of bootloader, so we can add some additional modes to do exactly that - and we can be explicit about it (instead of depending on a lucky side effect of EthCall..)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I was planning to do that with preprocessing, but for now that's just a quick solution

} else {
TxExecutionMode::VerifyExecute
};

let (keys, result, block, bytecodes) =
self.run_l2_tx_inner(l2_tx.clone(), execution_mode)?;

Expand Down Expand Up @@ -1359,12 +1377,16 @@ impl<S: ForkSource + std::fmt::Debug> InMemoryNode<S> {
inner.filters.notify_new_block(empty_block_hash);

{
// That's why here, we increase the batch by 1, but miniblock (and timestamp) by 2.
// You can look at insert_fictive_l2_block function in VM to see how this fake block is inserted.

inner.current_batch += 1;
inner.current_miniblock += 2;
inner.current_timestamp += 2;
inner.current_miniblock += 1;
inner.current_timestamp += 1;

// If it was not eth call, we increase the batch by 1, but miniblock (and timestamp) by 2.
// You can look at insert_fictive_l2_block function in VM to see how this fake block is inserted.
if let TxExecutionMode::VerifyExecute = execution_mode {
Copy link
Collaborator

Choose a reason for hiding this comment

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

uh, let's avoid this... I'm worried that we'll get bootloader in a bad spot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What exactly? Do you mean a missing fictive block?

inner.current_miniblock += 1;
inner.current_timestamp += 1;
}
}

Ok(())
Expand Down Expand Up @@ -1714,7 +1736,7 @@ impl<S: Send + Sync + 'static + ForkSource + std::fmt::Debug> EthNamespaceT for
.boxed();
};

match self.run_l2_tx(l2_tx.clone(), TxExecutionMode::VerifyExecute) {
match self.run_l2_tx(l2_tx.clone()) {
Ok(_) => Ok(hash).into_boxed_future(),
Err(e) => {
let error_message = format!("Execution error: {}", e);
Expand Down Expand Up @@ -2434,6 +2456,94 @@ impl<S: Send + Sync + 'static + ForkSource + std::fmt::Debug> EthNamespaceT for
}
}

impl<S: Send + Sync + 'static + ForkSource + std::fmt::Debug> EthTestNodeNamespaceT
for InMemoryNode<S>
{
/// Sends a transaction to the L2 network. Can be used for the impersonated account.
///
/// # Arguments
///
/// * `tx` - A `CallRequest` struct representing the transaction.
///
/// # Returns
///
/// A future that resolves to the hash of the transaction if successful, or an error if the transaction is invalid or execution fails.
fn send_transaction(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add unit tests for send_transaction

&self,
tx: zksync_types::transaction_request::CallRequest,
) -> jsonrpc_core::BoxFuture<jsonrpc_core::Result<zksync_basic_types::H256>> {
let chain_id = match self.inner.read() {
Ok(reader) => reader.fork_storage.chain_id,
Err(_) => {
return futures::future::err(into_jsrpc_error(Web3Error::InternalError)).boxed()
}
};

let mut tx_req = TransactionRequest::from(tx);
// If the sender is impersonated signature will be ignored.
tx_req.r = Some(U256::default());
tx_req.s = Some(U256::default());
tx_req.v = Some(U64::from(27));

let hash = match tx_req.get_tx_hash(chain_id) {
Ok(result) => result,
Err(e) => {
return futures::future::err(into_jsrpc_error(Web3Error::SerializationError(e)))
.boxed()
}
};
// v = 27 corresponds to 0
let bytes = tx_req.get_signed_bytes(
&PackedEthSignature::from_rsv(&H256::default(), &H256::default(), 0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we use PackedEthSignature::default?

chain_id,
);
let mut l2_tx: L2Tx = match L2Tx::from_request(tx_req, MAX_TX_SIZE) {
Ok(tx) => tx,
Err(e) => {
return futures::future::err(into_jsrpc_error(Web3Error::SerializationError(e)))
.boxed()
}
};

l2_tx.set_input(bytes, hash);
if hash != l2_tx.hash() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add a comment on when does this happen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think never, this code was stolen from the send_raw_transaction method. Maybe I will remove it

return futures::future::err(into_jsrpc_error(Web3Error::InvalidTransactionData(
zksync_types::ethabi::Error::InvalidData,
)))
.boxed();
};

match self.inner.read() {
Ok(reader) => {
if !reader
.impersonated_accounts
.contains(&l2_tx.common_data.initiator_address)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we also print a clear error message, that this method is ONLY for impersonated accounts.
(and mention the initiator_address, and how many entries we have in impersonated_accounts list).

(as I'm worried that people will try using this method without knowing this)

return futures::future::err(into_jsrpc_error(
Web3Error::InvalidTransactionData(zksync_types::ethabi::Error::InvalidData),
))
.boxed();
}
}
Err(_) => {
return futures::future::err(into_jsrpc_error(Web3Error::InternalError)).boxed()
}
}

match self.run_l2_tx(l2_tx.clone()) {
Ok(_) => Ok(hash).into_boxed_future(),
Err(e) => {
let error_message = format!("Execution error: {}", e);
futures::future::err(into_jsrpc_error(Web3Error::SubmitTransactionError(
error_message,
l2_tx.hash().as_bytes().to_vec(),
)))
.boxed()
}
}
}
}

#[cfg(test)]
mod tests {
use crate::{
Expand Down