From 79b6fcf8b5d10a0ccdceb846370dd6870b6a32b5 Mon Sep 17 00:00:00 2001 From: Alex Ostrovski Date: Mon, 30 Sep 2024 12:01:22 +0300 Subject: [PATCH] fix(api): Fix batch fee input for `debug` namespace (#2948) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What ❔ Changes the `debug_traceCall` handler to use the fee provider for recent blocks (pending / latest / committed). ## Why ❔ Previously, the `debug` namespace used a static batch fee input retrieved at the node initialization. This looks incorrect. ## Checklist - [x] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [x] Tests for the changes have been added / updated. - [x] Documentation comments have been added / updated. - [x] Code has been formatted via `zk fmt` and `zk lint`. --- core/lib/types/src/api/mod.rs | 26 ++---- .../api_server/src/web3/namespaces/debug.rs | 34 +++---- core/node/api_server/src/web3/testonly.rs | 92 ++++++++++++------- core/node/api_server/src/web3/tests/debug.rs | 2 +- core/node/api_server/src/web3/tests/mod.rs | 33 ++++--- core/node/api_server/src/web3/tests/vm.rs | 61 ++++++++++-- core/node/api_server/src/web3/tests/ws.rs | 10 +- core/node/consensus/src/testonly.rs | 24 ++--- 8 files changed, 168 insertions(+), 114 deletions(-) diff --git a/core/lib/types/src/api/mod.rs b/core/lib/types/src/api/mod.rs index 432b6c309c10..1e5a1b3fe65d 100644 --- a/core/lib/types/src/api/mod.rs +++ b/core/lib/types/src/api/mod.rs @@ -754,21 +754,17 @@ pub enum CallTracerBlockResult { } impl CallTracerBlockResult { - pub fn unwrap_flatten(self) -> Vec { + pub fn unwrap_flat(self) -> Vec { match self { - Self::CallTrace(_) => { - panic!("Result is a FlatCallTrace") - } - Self::FlatCallTrace(a) => a, + Self::CallTrace(_) => panic!("Result is a FlatCallTrace"), + Self::FlatCallTrace(trace) => trace, } } pub fn unwrap_default(self) -> Vec { match self { - Self::CallTrace(a) => a, - Self::FlatCallTrace(_) => { - panic!("Result is a CallTrace") - } + Self::CallTrace(trace) => trace, + Self::FlatCallTrace(_) => panic!("Result is a CallTrace"), } } } @@ -783,19 +779,15 @@ pub enum CallTracerResult { impl CallTracerResult { pub fn unwrap_flat(self) -> Vec { match self { - Self::CallTrace(_) => { - panic!("Result is a FlatCallTrace") - } - Self::FlatCallTrace(a) => a, + Self::CallTrace(_) => panic!("Result is a FlatCallTrace"), + Self::FlatCallTrace(trace) => trace, } } pub fn unwrap_default(self) -> DebugCall { match self { - Self::CallTrace(a) => a, - Self::FlatCallTrace(_) => { - panic!("Result is a CallTrace") - } + Self::CallTrace(trace) => trace, + Self::FlatCallTrace(_) => panic!("Result is a CallTrace"), } } } diff --git a/core/node/api_server/src/web3/namespaces/debug.rs b/core/node/api_server/src/web3/namespaces/debug.rs index 68c7951cee45..71560e4ddb8c 100644 --- a/core/node/api_server/src/web3/namespaces/debug.rs +++ b/core/node/api_server/src/web3/namespaces/debug.rs @@ -8,7 +8,6 @@ use zksync_types::{ ResultDebugCall, SupportedTracers, TracerConfig, }, debug_flat_call::{Action, CallResult, DebugCallFlat}, - fee_model::BatchFeeInput, l2::L2Tx, transaction_request::CallRequest, web3, H256, U256, @@ -22,27 +21,12 @@ use crate::{ #[derive(Debug, Clone)] pub(crate) struct DebugNamespace { - batch_fee_input: BatchFeeInput, state: RpcState, } impl DebugNamespace { pub async fn new(state: RpcState) -> anyhow::Result { - let fee_input_provider = &state.tx_sender.0.batch_fee_input_provider; - // FIXME (PLA-1033): use the fee input provider instead of a constant value - let batch_fee_input = fee_input_provider - .get_batch_fee_input_scaled( - state.api_config.estimate_gas_scale_factor, - state.api_config.estimate_gas_scale_factor, - ) - .await - .context("cannot get batch fee input")?; - - Ok(Self { - // For now, the same scaling is used for both the L1 gas price and the pubdata price - batch_fee_input, - state, - }) + Ok(Self { state }) } pub(crate) fn map_call( @@ -257,12 +241,22 @@ impl DebugNamespace { if request.gas.is_none() { request.gas = Some(block_args.default_eth_call_gas(&mut connection).await?); } + let fee_input = if block_args.resolves_to_latest_sealed_l2_block() { - self.batch_fee_input + // It is important to drop a DB connection before calling the provider, since it acquires a connection internally + // on the main node. + drop(connection); + let scale_factor = self.state.api_config.estimate_gas_scale_factor; + let fee_input_provider = &self.state.tx_sender.0.batch_fee_input_provider; + // For now, the same scaling is used for both the L1 gas price and the pubdata price + fee_input_provider + .get_batch_fee_input_scaled(scale_factor, scale_factor) + .await? } else { - block_args.historical_fee_input(&mut connection).await? + let fee_input = block_args.historical_fee_input(&mut connection).await?; + drop(connection); + fee_input }; - drop(connection); let call_overrides = request.get_call_overrides()?; let call = L2Tx::from_request(request.into(), MAX_ENCODED_TX_SIZE)?; diff --git a/core/node/api_server/src/web3/testonly.rs b/core/node/api_server/src/web3/testonly.rs index 03aa4e68e6a5..93309fc09cf1 100644 --- a/core/node/api_server/src/web3/testonly.rs +++ b/core/node/api_server/src/web3/testonly.rs @@ -97,42 +97,72 @@ impl ApiServerHandles { } } -pub async fn spawn_http_server( - api_config: InternalApiConfig, +/// Builder for test server instances. +#[derive(Debug)] +pub struct TestServerBuilder { pool: ConnectionPool, + api_config: InternalApiConfig, tx_executor: MockOneshotExecutor, method_tracer: Arc, - stop_receiver: watch::Receiver, -) -> ApiServerHandles { - spawn_server( - ApiTransportLabel::Http, - api_config, - pool, - None, - tx_executor, - method_tracer, - stop_receiver, - ) - .await - .0 } -pub async fn spawn_ws_server( - api_config: InternalApiConfig, - pool: ConnectionPool, - stop_receiver: watch::Receiver, - websocket_requests_per_minute_limit: Option, -) -> (ApiServerHandles, mpsc::UnboundedReceiver) { - spawn_server( - ApiTransportLabel::Ws, - api_config, - pool, - websocket_requests_per_minute_limit, - MockOneshotExecutor::default(), - Arc::default(), - stop_receiver, - ) - .await +impl TestServerBuilder { + /// Creates a new builder. + pub fn new(pool: ConnectionPool, api_config: InternalApiConfig) -> Self { + Self { + api_config, + pool, + tx_executor: MockOneshotExecutor::default(), + method_tracer: Arc::default(), + } + } + + /// Sets a transaction / call executor for this builder. + #[must_use] + pub fn with_tx_executor(mut self, tx_executor: MockOneshotExecutor) -> Self { + self.tx_executor = tx_executor; + self + } + + /// Sets an RPC method tracer for this builder. + #[must_use] + pub fn with_method_tracer(mut self, tracer: Arc) -> Self { + self.method_tracer = tracer; + self + } + + /// Builds an HTTP server. + pub async fn build_http(self, stop_receiver: watch::Receiver) -> ApiServerHandles { + spawn_server( + ApiTransportLabel::Http, + self.api_config, + self.pool, + None, + self.tx_executor, + self.method_tracer, + stop_receiver, + ) + .await + .0 + } + + /// Builds a WS server. + pub async fn build_ws( + self, + websocket_requests_per_minute_limit: Option, + stop_receiver: watch::Receiver, + ) -> (ApiServerHandles, mpsc::UnboundedReceiver) { + spawn_server( + ApiTransportLabel::Ws, + self.api_config, + self.pool, + websocket_requests_per_minute_limit, + self.tx_executor, + self.method_tracer, + stop_receiver, + ) + .await + } } async fn spawn_server( diff --git a/core/node/api_server/src/web3/tests/debug.rs b/core/node/api_server/src/web3/tests/debug.rs index 7711570c3c54..4f021b777aec 100644 --- a/core/node/api_server/src/web3/tests/debug.rs +++ b/core/node/api_server/src/web3/tests/debug.rs @@ -137,7 +137,7 @@ impl HttpTest for TraceBlockFlatTest { }), ) .await? - .unwrap_flatten(); + .unwrap_flat(); // A transaction with 2 nested calls will convert into 3 Flattened calls. // Also in this test, all tx have the same # of nested calls diff --git a/core/node/api_server/src/web3/tests/mod.rs b/core/node/api_server/src/web3/tests/mod.rs index fe90f1483a5a..632e263c6536 100644 --- a/core/node/api_server/src/web3/tests/mod.rs +++ b/core/node/api_server/src/web3/tests/mod.rs @@ -58,7 +58,7 @@ use zksync_web3_decl::{ }; use super::*; -use crate::web3::testonly::{spawn_http_server, spawn_ws_server}; +use crate::web3::testonly::TestServerBuilder; mod debug; mod filters; @@ -245,14 +245,11 @@ async fn test_http_server(test: impl HttpTest) { let genesis = GenesisConfig::for_tests(); let mut api_config = InternalApiConfig::new(&web3_config, &contracts_config, &genesis); api_config.filters_disabled = test.filters_disabled(); - let mut server_handles = spawn_http_server( - api_config, - pool.clone(), - test.transaction_executor(), - test.method_tracer(), - stop_receiver, - ) - .await; + let mut server_handles = TestServerBuilder::new(pool.clone(), api_config) + .with_tx_executor(test.transaction_executor()) + .with_method_tracer(test.method_tracer()) + .build_http(stop_receiver) + .await; let local_addr = server_handles.wait_until_ready().await; let client = Client::http(format!("http://{local_addr}/").parse().unwrap()) @@ -306,6 +303,17 @@ async fn store_l2_block( number: L2BlockNumber, transaction_results: &[TransactionExecutionResult], ) -> anyhow::Result { + let header = create_l2_block(number.0); + store_custom_l2_block(storage, &header, transaction_results).await?; + Ok(header) +} + +async fn store_custom_l2_block( + storage: &mut Connection<'_, Core>, + header: &L2BlockHeader, + transaction_results: &[TransactionExecutionResult], +) -> anyhow::Result<()> { + let number = header.number; for result in transaction_results { let l2_tx = result.transaction.clone().try_into().unwrap(); let tx_submission_result = storage @@ -328,19 +336,18 @@ async fn store_l2_block( .append_storage_logs(number, &[l2_block_log]) .await?; - let new_l2_block = create_l2_block(number.0); - storage.blocks_dal().insert_l2_block(&new_l2_block).await?; + storage.blocks_dal().insert_l2_block(header).await?; storage .transactions_dal() .mark_txs_as_executed_in_l2_block( - new_l2_block.number, + number, transaction_results, 1.into(), ProtocolVersionId::latest(), false, ) .await?; - Ok(new_l2_block) + Ok(()) } async fn seal_l1_batch( diff --git a/core/node/api_server/src/web3/tests/vm.rs b/core/node/api_server/src/web3/tests/vm.rs index b8f74370303e..d8086c6c6add 100644 --- a/core/node/api_server/src/web3/tests/vm.rs +++ b/core/node/api_server/src/web3/tests/vm.rs @@ -1,8 +1,11 @@ //! Tests for the VM-instantiating methods (e.g., `eth_call`). -use std::sync::{ - atomic::{AtomicU32, Ordering}, - Mutex, +use std::{ + str, + sync::{ + atomic::{AtomicU32, Ordering}, + Mutex, + }, }; use api::state_override::{OverrideAccount, StateOverride}; @@ -51,6 +54,11 @@ impl ExpectedFeeInput { self.expect_for_block(api::BlockNumber::Pending, scale); } + #[allow(dead_code)] + fn expect_custom(&self, expected: BatchFeeInput) { + *self.0.lock().unwrap() = expected; + } + fn assert_eq(&self, actual: BatchFeeInput) { let expected = *self.0.lock().unwrap(); // We do relaxed comparisons to deal with the fact that the fee input provider may convert inputs to pubdata independent form. @@ -87,11 +95,18 @@ impl CallTest { expected_fee_input.assert_eq(env.l1_batch.fee_input); let expected_block_number = match tx.execute.calldata() { - b"pending" => latest_block + 1, - b"latest" => latest_block, + b"pending" => latest_block.0 + 1, + b"latest" => latest_block.0, + block if block.starts_with(b"block=") => str::from_utf8(block) + .expect("non-UTF8 calldata") + .strip_prefix("block=") + .unwrap() + .trim() + .parse() + .expect("invalid block number"), data => panic!("Unexpected calldata: {data:?}"), }; - assert_eq!(env.l1_batch.first_l2_block.number, expected_block_number.0); + assert_eq!(env.l1_batch.first_l2_block.number, expected_block_number); ExecutionResult::Success { output: b"output".to_vec(), @@ -115,7 +130,6 @@ impl HttpTest for CallTest { // Store an additional L2 block because L2 block #0 has some special processing making it work incorrectly. let mut connection = pool.connection().await?; store_l2_block(&mut connection, L2BlockNumber(1), &[]).await?; - drop(connection); let call_result = client .call(Self::call_request(b"pending"), None, None) @@ -148,6 +162,22 @@ impl HttpTest for CallTest { panic!("Unexpected error: {error:?}"); } + // Check that the method handler fetches fee inputs for recent blocks. To do that, we create a new block + // with a large fee input; it should be loaded by `ApiFeeInputProvider` and override the input provided by the wrapped mock provider. + let mut block_header = create_l2_block(2); + block_header.batch_fee_input = + ::default_batch_fee_input_scaled( + FeeParams::sensible_v1_default(), + 2.5, + 2.5, + ); + store_custom_l2_block(&mut connection, &block_header, &[]).await?; + // Fee input is not scaled further as per `ApiFeeInputProvider` implementation + self.fee_input.expect_custom(block_header.batch_fee_input); + let call_request = CallTest::call_request(b"block=3"); + let call_result = client.call(call_request.clone(), None, None).await?; + assert_eq!(call_result.0, b"output"); + Ok(()) } } @@ -484,7 +514,6 @@ impl HttpTest for TraceCallTest { // Store an additional L2 block because L2 block #0 has some special processing making it work incorrectly. let mut connection = pool.connection().await?; store_l2_block(&mut connection, L2BlockNumber(1), &[]).await?; - drop(connection); self.fee_input.expect_default(Self::FEE_SCALE); let call_request = CallTest::call_request(b"pending"); @@ -530,6 +559,22 @@ impl HttpTest for TraceCallTest { panic!("Unexpected error: {error:?}"); } + // Check that the method handler fetches fee inputs for recent blocks. To do that, we create a new block + // with a large fee input; it should be loaded by `ApiFeeInputProvider` and override the input provided by the wrapped mock provider. + let mut block_header = create_l2_block(2); + block_header.batch_fee_input = + ::default_batch_fee_input_scaled( + FeeParams::sensible_v1_default(), + 3.0, + 3.0, + ); + store_custom_l2_block(&mut connection, &block_header, &[]).await?; + // Fee input is not scaled further as per `ApiFeeInputProvider` implementation + self.fee_input.expect_custom(block_header.batch_fee_input); + let call_request = CallTest::call_request(b"block=3"); + let call_result = client.trace_call(call_request.clone(), None, None).await?; + Self::assert_debug_call(&call_request, &call_result.unwrap_default()); + Ok(()) } } diff --git a/core/node/api_server/src/web3/tests/ws.rs b/core/node/api_server/src/web3/tests/ws.rs index 39f991aba047..28b2e2beb554 100644 --- a/core/node/api_server/src/web3/tests/ws.rs +++ b/core/node/api_server/src/web3/tests/ws.rs @@ -177,13 +177,9 @@ async fn test_ws_server(test: impl WsTest) { drop(storage); let (stop_sender, stop_receiver) = watch::channel(false); - let (mut server_handles, pub_sub_events) = spawn_ws_server( - api_config, - pool.clone(), - stop_receiver, - test.websocket_requests_per_minute_limit(), - ) - .await; + let (mut server_handles, pub_sub_events) = TestServerBuilder::new(pool.clone(), api_config) + .build_ws(test.websocket_requests_per_minute_limit(), stop_receiver) + .await; let local_addr = server_handles.wait_until_ready().await; let client = Client::ws(format!("ws://{local_addr}").parse().unwrap()) diff --git a/core/node/consensus/src/testonly.rs b/core/node/consensus/src/testonly.rs index 1a486b3fc64c..c37dd6a7fecb 100644 --- a/core/node/consensus/src/testonly.rs +++ b/core/node/consensus/src/testonly.rs @@ -20,7 +20,7 @@ use zksync_l1_contract_interface::i_executor::structures::StoredBatchInfo; use zksync_metadata_calculator::{ LazyAsyncTreeReader, MetadataCalculator, MetadataCalculatorConfig, }; -use zksync_node_api_server::web3::{state::InternalApiConfig, testonly::spawn_http_server}; +use zksync_node_api_server::web3::{state::InternalApiConfig, testonly::TestServerBuilder}; use zksync_node_genesis::GenesisParams; use zksync_node_sync::{ fetcher::{FetchedTransaction, IoCursorExt as _}, @@ -646,14 +646,9 @@ impl StateKeeperRunner { &configs::contracts::ContractsConfig::for_tests(), &configs::GenesisConfig::for_tests(), ); - let mut server = spawn_http_server( - cfg, - self.pool.0.clone(), - Default::default(), - Arc::default(), - stop_recv, - ) - .await; + let mut server = TestServerBuilder::new(self.pool.0.clone(), cfg) + .build_http(stop_recv) + .await; if let Ok(addr) = ctx.wait(server.wait_until_ready()).await { self.addr.send_replace(Some(addr)); tracing::info!("API server ready!"); @@ -731,14 +726,9 @@ impl StateKeeperRunner { &configs::contracts::ContractsConfig::for_tests(), &configs::GenesisConfig::for_tests(), ); - let mut server = spawn_http_server( - cfg, - self.pool.0.clone(), - Default::default(), - Arc::default(), - stop_recv, - ) - .await; + let mut server = TestServerBuilder::new(self.pool.0.clone(), cfg) + .build_http(stop_recv) + .await; if let Ok(addr) = ctx.wait(server.wait_until_ready()).await { self.addr.send_replace(Some(addr)); tracing::info!("API server ready!");