diff --git a/crates/core/src/main.rs b/crates/core/src/main.rs index 81ca8c8..ff11586 100644 --- a/crates/core/src/main.rs +++ b/crates/core/src/main.rs @@ -4,7 +4,6 @@ mod helpers; use anyhow::{Context as _, bail}; use clap::Parser; -use revive_dt_common::types::ParsedTestSpecifier; use revive_dt_report::{ReportAggregator, TestCaseStatus}; use schemars::schema_for; use tracing::{info, level_filters::LevelFilter}; @@ -64,40 +63,15 @@ fn main() -> anyhow::Result<()> { ) .await?; - // Error out if there are any failing tests. - let failures = report + let contains_failure = report .execution_information - .into_iter() - .flat_map(|(metadata_file_path, metadata_file_report)| { - metadata_file_report.case_reports.into_iter().flat_map( - move |(case_idx, case_report)| { - let metadata_file_path = metadata_file_path.clone(); - case_report.mode_execution_reports.into_iter().filter_map( - move |(mode, execution_report)| { - if let Some(TestCaseStatus::Failed { reason }) = - execution_report.status - { - let parsed_test_specifier = - ParsedTestSpecifier::CaseWithMode { - metadata_file_path: metadata_file_path - .clone() - .into_inner(), - case_idx: case_idx.into_inner(), - mode, - }; - Some((parsed_test_specifier, reason)) - } else { - None - } - }, - ) - }, - ) - }) - .collect::>(); + .values() + .flat_map(|values| values.case_reports.values()) + .flat_map(|values| values.mode_execution_reports.values()) + .any(|report| matches!(report.status, Some(TestCaseStatus::Failed { .. }))); - if !failures.is_empty() { - bail!("Some tests failed: {failures:#?}") + if contains_failure { + bail!("Some tests failed") } Ok(()) @@ -117,40 +91,15 @@ fn main() -> anyhow::Result<()> { ) .await?; - // Error out if there are any failing tests. - let failures = report + let contains_failure = report .execution_information - .into_iter() - .flat_map(|(metadata_file_path, metadata_file_report)| { - metadata_file_report.case_reports.into_iter().flat_map( - move |(case_idx, case_report)| { - let metadata_file_path = metadata_file_path.clone(); - case_report.mode_execution_reports.into_iter().filter_map( - move |(mode, execution_report)| { - if let Some(TestCaseStatus::Failed { reason }) = - execution_report.status - { - let parsed_test_specifier = - ParsedTestSpecifier::CaseWithMode { - metadata_file_path: metadata_file_path - .clone() - .into_inner(), - case_idx: case_idx.into_inner(), - mode, - }; - Some((parsed_test_specifier, reason)) - } else { - None - } - }, - ) - }, - ) - }) - .collect::>(); + .values() + .flat_map(|values| values.case_reports.values()) + .flat_map(|values| values.mode_execution_reports.values()) + .any(|report| matches!(report.status, Some(TestCaseStatus::Failed { .. }))); - if !failures.is_empty() { - bail!("Some tests failed: {failures:#?}") + if contains_failure { + bail!("Some benchmarks failed") } Ok(()) diff --git a/crates/node/src/node_implementations/geth.rs b/crates/node/src/node_implementations/geth.rs index 67673a3..fbf2433 100644 --- a/crates/node/src/node_implementations/geth.rs +++ b/crates/node/src/node_implementations/geth.rs @@ -250,7 +250,7 @@ impl GethNode { construct_concurrency_limited_provider::( self.connection_string.as_str(), FallbackGasFiller::default() - .with_use_fallback_gas_filler(self.use_fallback_gas_filler), + .with_fallback_mechanism(self.use_fallback_gas_filler), ChainIdFiller::new(Some(CHAIN_ID)), NonceFiller::new(self.nonce_manager.clone()), self.wallet.clone(), diff --git a/crates/node/src/node_implementations/lighthouse_geth.rs b/crates/node/src/node_implementations/lighthouse_geth.rs index e672ea2..b59bd4a 100644 --- a/crates/node/src/node_implementations/lighthouse_geth.rs +++ b/crates/node/src/node_implementations/lighthouse_geth.rs @@ -379,7 +379,7 @@ impl LighthouseGethNode { construct_concurrency_limited_provider::( self.ws_connection_string.as_str(), FallbackGasFiller::default() - .with_use_fallback_gas_filler(self.use_fallback_gas_filler), + .with_fallback_mechanism(self.use_fallback_gas_filler), ChainIdFiller::new(Some(CHAIN_ID)), NonceFiller::new(self.nonce_manager.clone()), self.wallet.clone(), diff --git a/crates/node/src/node_implementations/substrate.rs b/crates/node/src/node_implementations/substrate.rs index 3e128dc..f29a706 100644 --- a/crates/node/src/node_implementations/substrate.rs +++ b/crates/node/src/node_implementations/substrate.rs @@ -47,7 +47,7 @@ use tracing::{instrument, trace}; use crate::{ Node, - constants::{CHAIN_ID, INITIAL_BALANCE}, + constants::INITIAL_BALANCE, helpers::{Process, ProcessReadinessWaitBehavior}, provider_utils::{ ConcreteProvider, FallbackGasFiller, construct_concurrency_limited_provider, @@ -327,13 +327,9 @@ impl SubstrateNode { .get_or_try_init(|| async move { construct_concurrency_limited_provider::( self.rpc_url.as_str(), - FallbackGasFiller::new( - u64::MAX, - 50_000_000_000, - 1_000_000_000, - self.use_fallback_gas_filler, - ), - ChainIdFiller::new(Some(CHAIN_ID)), + FallbackGasFiller::default() + .with_fallback_mechanism(self.use_fallback_gas_filler), + ChainIdFiller::default(), NonceFiller::new(self.nonce_manager.clone()), self.wallet.clone(), ) diff --git a/crates/node/src/node_implementations/zombienet.rs b/crates/node/src/node_implementations/zombienet.rs index 76e8722..3d1ed3e 100644 --- a/crates/node/src/node_implementations/zombienet.rs +++ b/crates/node/src/node_implementations/zombienet.rs @@ -334,12 +334,8 @@ impl ZombienetNode { .get_or_try_init(|| async move { construct_concurrency_limited_provider::( self.connection_string.as_str(), - FallbackGasFiller::new( - u64::MAX, - 5_000_000_000, - 1_000_000_000, - self.use_fallback_gas_filler, - ), + FallbackGasFiller::default() + .with_fallback_mechanism(self.use_fallback_gas_filler), ChainIdFiller::default(), // TODO: use CHAIN_ID constant NonceFiller::new(self.nonce_manager.clone()), self.wallet.clone(), diff --git a/crates/node/src/provider_utils/fallback_gas_filler.rs b/crates/node/src/provider_utils/fallback_gas_filler.rs index c7f209f..39eb790 100644 --- a/crates/node/src/provider_utils/fallback_gas_filler.rs +++ b/crates/node/src/provider_utils/fallback_gas_filler.rs @@ -1,65 +1,71 @@ +use std::{borrow::Cow, fmt::Display}; + use alloy::{ + eips::BlockNumberOrTag, network::{Network, TransactionBuilder}, providers::{ Provider, SendableTx, - fillers::{GasFiller, TxFiller}, + ext::DebugApi, + fillers::{GasFillable, GasFiller, TxFiller}, }, - transports::{TransportError, TransportResult}, + rpc::types::trace::geth::{ + GethDebugBuiltInTracerType, GethDebugTracerType, GethDebugTracingCallOptions, + GethDebugTracingOptions, + }, + transports::{RpcError, TransportResult}, }; -// Percentage padding applied to estimated gas (e.g. 120 = 20% padding) -const GAS_ESTIMATE_PADDING_NUMERATOR: u64 = 120; -const GAS_ESTIMATE_PADDING_DENOMINATOR: u64 = 100; - -#[derive(Clone, Debug)] +/// An implementation of [`GasFiller`] with a fallback mechanism for reverting transactions. +/// +/// This struct provides a fallback mechanism for alloy's [`GasFiller`] which kicks in when a +/// transaction's dry run fails due to it reverting allowing us to get gas estimates even for +/// failing transactions. In this codebase, this is very important since the MatterLabs tests +/// expect some transactions in the test suite revert. Since we're expected to run a number of +/// assertions on these reverting transactions we must commit them to the ledger. +/// +/// Therefore, this struct does the following: +/// +/// 1. It first attempts to estimate the gas through the mechanism implemented in the [`GasFiller`]. +/// 2. If it fails, then we perform a debug trace of the transaction to find out how much gas the +/// transaction needs until it reverts. +/// 3. We fill in these values (either the success or failure case) into the transaction. +/// +/// The fallback mechanism of this filler can be completely disabled if we don't want it to be used. +/// In that case, this gas filler will act in an identical way to alloy's [`GasFiller`]. +/// +/// We then fill in these values into the transaction. +/// +/// The previous implementation of this fallback gas filler relied on making use of default values +/// for the gas limit in order to be able to submit the reverting transactions to the network. But, +/// it introduced a number of issues that we weren't anticipating at the time when it was built. +#[derive(Clone, Copy, Debug)] pub struct FallbackGasFiller { + /// The inner [`GasFiller`] which we pass all of the calls to in the happy path. inner: GasFiller, - default_gas_limit: u64, - default_max_fee_per_gas: u128, - default_priority_fee: u128, - use_fallback_gas_filler: bool, + + /// A [`bool`] that controls if the fallback mechanism is enabled or not. + enable_fallback_mechanism: bool, } impl FallbackGasFiller { - pub fn new( - default_gas_limit: u64, - default_max_fee_per_gas: u128, - default_priority_fee: u128, - use_fallback_gas_filler: bool, - ) -> Self { + pub fn new() -> Self { Self { - inner: GasFiller, - default_gas_limit, - default_max_fee_per_gas, - default_priority_fee, - use_fallback_gas_filler, + inner: Default::default(), + enable_fallback_mechanism: true, } } - pub fn with_default_gas_limit(mut self, default_gas_limit: u64) -> Self { - self.default_gas_limit = default_gas_limit; + pub fn with_fallback_mechanism(mut self, enable: bool) -> Self { + self.enable_fallback_mechanism = enable; self } - pub fn with_default_max_fee_per_gas(mut self, default_max_fee_per_gas: u128) -> Self { - self.default_max_fee_per_gas = default_max_fee_per_gas; - self + pub fn with_fallback_mechanism_enabled(self) -> Self { + self.with_fallback_mechanism(true) } - pub fn with_default_priority_fee(mut self, default_priority_fee: u128) -> Self { - self.default_priority_fee = default_priority_fee; - self - } - - pub fn with_use_fallback_gas_filler(mut self, use_fallback_gas_filler: bool) -> Self { - self.use_fallback_gas_filler = use_fallback_gas_filler; - self - } -} - -impl Default for FallbackGasFiller { - fn default() -> Self { - FallbackGasFiller::new(25_000_000, 1_000_000_000, 1_000_000_000, true) + pub fn with_fallback_mechanism_disabled(self) -> Self { + self.with_fallback_mechanism(false) } } @@ -67,32 +73,89 @@ impl TxFiller for FallbackGasFiller where N: Network, { - type Fillable = Option<>::Fillable>; + type Fillable = >::Fillable; fn status( &self, tx: &::TransactionRequest, ) -> alloy::providers::fillers::FillerControlFlow { - >::status(&self.inner, tx) + TxFiller::::status(&self.inner, tx) } - fn fill_sync(&self, _: &mut alloy::providers::SendableTx) {} + fn fill_sync(&self, _: &mut SendableTx) {} async fn prepare>( &self, provider: &P, tx: &::TransactionRequest, ) -> TransportResult { - match self.inner.prepare(provider, tx).await { - Ok(fill) => Ok(Some(fill)), - Err(err) => { - tracing::debug!(error = ?err, "Gas Provider Estimation Failed, using fallback"); + match ( + self.inner.prepare(provider, tx).await, + self.enable_fallback_mechanism, + ) { + // Return the same thing if either this calls succeeds, or if the call falls and the + // fallback mechanism is disabled. + (rtn @ Ok(..), ..) | (rtn @ Err(..), false) => rtn, + (Err(..), true) => { + // Perform a trace of the transaction. + let trace = provider + .debug_trace_call( + tx.clone(), + BlockNumberOrTag::Latest.into(), + GethDebugTracingCallOptions { + tracing_options: GethDebugTracingOptions { + tracer: Some(GethDebugTracerType::BuiltInTracer( + GethDebugBuiltInTracerType::CallTracer, + )), + ..Default::default() + }, + state_overrides: Default::default(), + block_overrides: Default::default(), + }, + ) + .await? + .try_into_call_frame() + .map_err(|err| { + RpcError::LocalUsageError( + FallbackGasFillerError::new(format!( + "Expected a callframe trace, but got: {err:?}" + )) + .boxed(), + ) + })?; - if !self.use_fallback_gas_filler { - Err(err) - } else { - Ok(None) + let gas_used = u64::try_from(trace.gas_used).map_err(|_| { + RpcError::LocalUsageError( + FallbackGasFillerError::new( + "Transaction trace returned a value of gas used that exceeds u64", + ) + .boxed(), + ) + })?; + let gas_limit = gas_used.saturating_mul(120) / 100; + + if let Some(gas_price) = tx.gas_price() { + return Ok(GasFillable::Legacy { + gas_limit, + gas_price, + }); } + + let estimate = if let (Some(max_fee_per_gas), Some(max_priority_fee_per_gas)) = + (tx.max_fee_per_gas(), tx.max_priority_fee_per_gas()) + { + alloy::eips::eip1559::Eip1559Estimation { + max_fee_per_gas, + max_priority_fee_per_gas, + } + } else { + provider.estimate_eip1559_fees().await? + }; + + Ok(GasFillable::Eip1559 { + gas_limit, + estimate, + }) } } } @@ -100,31 +163,35 @@ where async fn fill( &self, fillable: Self::Fillable, - mut tx: alloy::providers::SendableTx, + tx: SendableTx, ) -> TransportResult> { - if let Some(fill) = fillable { - let mut tx = self.inner.fill(fill, tx).await?; - if let Some(builder) = tx.as_mut_builder() { - if let Some(estimated) = builder.gas_limit() { - let padded = estimated - .checked_mul(GAS_ESTIMATE_PADDING_NUMERATOR) - .and_then(|v| v.checked_div(GAS_ESTIMATE_PADDING_DENOMINATOR)) - .unwrap_or(u64::MAX); - builder.set_gas_limit(padded); - } - } - Ok(tx) - } else if self.use_fallback_gas_filler { - if let Some(builder) = tx.as_mut_builder() { - builder.set_gas_limit(self.default_gas_limit); - builder.set_max_fee_per_gas(self.default_max_fee_per_gas); - builder.set_max_priority_fee_per_gas(self.default_priority_fee); - } - Ok(tx) - } else { - Err(TransportError::UnsupportedFeature( - "Fallback gas filler is disabled and we're attempting to do a gas estimate on a failing transaction", - )) - } + self.inner.fill(fillable, tx).await } } + +impl Default for FallbackGasFiller { + fn default() -> Self { + Self::new() + } +} + +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +struct FallbackGasFillerError(Cow<'static, str>); + +impl FallbackGasFillerError { + pub fn new(string: impl Into>) -> Self { + Self(string.into()) + } + + pub fn boxed(self) -> Box { + Box::new(self) + } +} + +impl Display for FallbackGasFillerError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + Display::fmt(&self.0, f) + } +} + +impl std::error::Error for FallbackGasFillerError {}