From 480382676c0b4bde287993282a84b23c950c13d4 Mon Sep 17 00:00:00 2001 From: Dan Coombs Date: Wed, 18 Dec 2024 19:39:11 -0600 Subject: [PATCH] fix(pool): fix pool candidates metrics to align with builder (#943) --- crates/pool/src/mempool/pool.rs | 58 +++++++++++------------------- crates/pool/src/mempool/uo_pool.rs | 39 ++++++++------------ crates/sim/src/lib.rs | 4 +-- crates/sim/src/precheck.rs | 42 +++++++++++++--------- 4 files changed, 61 insertions(+), 82 deletions(-) diff --git a/crates/pool/src/mempool/pool.rs b/crates/pool/src/mempool/pool.rs index 84d9a09bb..c285c2ccf 100644 --- a/crates/pool/src/mempool/pool.rs +++ b/crates/pool/src/mempool/pool.rs @@ -12,7 +12,7 @@ // If not, see https://www.gnu.org/licenses/. use std::{ - cmp::{self, Ordering}, + cmp::Ordering, collections::{hash_map::Entry, BTreeSet, HashMap, HashSet}, sync::Arc, time::{Duration, SystemTime, UNIX_EPOCH}, @@ -24,11 +24,12 @@ use metrics::{Gauge, Histogram}; use metrics_derive::Metrics; use parking_lot::RwLock; use rundler_provider::DAGasOracleSync; +use rundler_sim::FeeUpdate; use rundler_types::{ chain::ChainSpec, da::DAGasBlockData, pool::{MempoolError, PoolOperation}, - Entity, EntityType, GasFees, Timestamp, UserOperation, UserOperationId, UserOperationVariant, + Entity, EntityType, Timestamp, UserOperation, UserOperationId, UserOperationVariant, }; use rundler_utils::{emit::WithEntryPoint, math}; use tokio::sync::broadcast; @@ -207,7 +208,6 @@ where )); let hash = self.add_operation_internal(pool_op)?; - self.update_metrics(); Ok(hash) } @@ -233,8 +233,7 @@ where block_number: u64, block_timestamp: Timestamp, block_da_data: Option<&DAGasBlockData>, - candidate_gas_fees: GasFees, - base_fee: u128, + gas_fees: FeeUpdate, ) { let sys_block_time = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -242,7 +241,6 @@ where let block_delta_time = sys_block_time.saturating_sub(self.prev_sys_block_time); let block_delta_height = block_number.saturating_sub(self.prev_block_number); - let candidate_gas_price = base_fee + candidate_gas_fees.max_priority_fee_per_gas; let mut expired = Vec::new(); let mut num_candidates = 0; let mut events = vec![]; @@ -266,7 +264,7 @@ where let required_da_gas = da_gas_oracle.calc_da_gas_sync( &op.po.da_gas_data, block_da_data, - op.uo().gas_price(base_fee), + op.uo().gas_price(gas_fees.base_fee), ); let required_pvg = op.uo().required_pre_verification_gas( @@ -298,11 +296,9 @@ where } } - let uo_gas_price = cmp::min( - op.uo().max_fee_per_gas(), - op.uo().max_priority_fee_per_gas() + base_fee, - ); - if candidate_gas_price > uo_gas_price { + if op.uo().max_fee_per_gas() < gas_fees.uo_fees.max_fee_per_gas + || op.uo().max_priority_fee_per_gas() < gas_fees.uo_fees.max_priority_fee_per_gas + { // don't mark as ineligible, but also not a candidate continue; } @@ -324,6 +320,7 @@ where self.metrics.num_candidates.set(num_candidates as f64); self.prev_block_number = block_number; self.prev_sys_block_time = sys_block_time; + self.update_metrics(); } pub(crate) fn address_count(&self, address: &Address) -> usize { @@ -343,9 +340,7 @@ where } pub(crate) fn remove_operation_by_hash(&mut self, hash: B256) -> Option> { - let ret = self.remove_operation_internal(hash, None); - self.update_metrics(); - ret + self.remove_operation_internal(hash, None) } // STO-040 @@ -428,10 +423,7 @@ where .uo() .hash(mined_op.entry_point, self.config.chain_spec.id); - let ret = self.remove_operation_internal(hash, Some(block_number)); - - self.update_metrics(); - ret + self.remove_operation_internal(hash, Some(block_number)) } pub(crate) fn unmine_operation(&mut self, mined_op: &MinedOp) -> Option> { @@ -440,10 +432,9 @@ where self.mined_hashes_with_block_numbers .remove(&(block_number, hash)); - if let Err(error) = self.put_back_unmined_operation(op.clone()) { + if let Err(error) = self.add_operation_internal(op.clone()) { info!("Could not put back unmined operation: {error}"); }; - self.update_metrics(); Some(op.po.clone()) } @@ -479,7 +470,6 @@ where for &hash in &to_remove { self.remove_operation_internal(hash, None); } - self.update_metrics(); to_remove } @@ -495,7 +485,6 @@ where for &hash in &to_remove { self.remove_operation_internal(hash, None); } - self.update_metrics(); to_remove } @@ -510,7 +499,6 @@ where } self.mined_hashes_with_block_numbers.remove(&(bn, hash)); } - self.update_metrics(); } pub(crate) fn clear(&mut self) { @@ -546,10 +534,6 @@ where Ok(removed) } - fn put_back_unmined_operation(&mut self, op: Arc) -> MempoolResult { - self.add_operation_internal(op) - } - fn add_operation_internal( &mut self, pool_op: Arc, @@ -592,6 +576,7 @@ where Err(MempoolError::DiscardedOnInsert)?; } + self.update_metrics(); Ok(hash) } @@ -619,6 +604,7 @@ where } self.pool_size -= op.mem_size(); + self.update_metrics(); Some(op.po.clone()) } @@ -1201,7 +1187,7 @@ mod tests { po1.valid_time_range.valid_until = Timestamp::from(1); let hash = pool.add_operation(po1.clone(), 0).unwrap(); - pool.do_maintenance(0, Timestamp::from(2), None, GasFees::default(), 0); + pool.do_maintenance(0, Timestamp::from(2), None, FeeUpdate::default()); assert_eq!(None, pool.get_operation_by_hash(hash)); } @@ -1221,7 +1207,7 @@ mod tests { po3.valid_time_range.valid_until = 9.into(); let hash3 = pool.add_operation(po3.clone(), 0).unwrap(); - pool.do_maintenance(0, Timestamp::from(10), None, GasFees::default(), 0); + pool.do_maintenance(0, Timestamp::from(10), None, FeeUpdate::default()); assert_eq!(None, pool.get_operation_by_hash(hash1)); assert!(pool.get_operation_by_hash(hash2).is_some()); @@ -1271,8 +1257,7 @@ mod tests { 0, 0.into(), Some(&DAGasBlockData::default()), - GasFees::default(), - 0, + FeeUpdate::default(), ); assert_eq!(pool.best_operations().collect::>().len(), 1); // UO is now eligible @@ -1307,8 +1292,7 @@ mod tests { 0, 0.into(), Some(&DAGasBlockData::default()), - GasFees::default(), - 0, + FeeUpdate::default(), ); assert_eq!(pool.best_operations().collect::>().len(), 0); @@ -1343,8 +1327,7 @@ mod tests { 0, 0.into(), Some(&DAGasBlockData::default()), - GasFees::default(), - 0, + FeeUpdate::default(), ); assert_eq!(pool.best_operations().collect::>().len(), 1); @@ -1382,8 +1365,7 @@ mod tests { 0, 0.into(), Some(&DAGasBlockData::default()), - GasFees::default(), - base_fee, + FeeUpdate::default(), ); assert_eq!(pool.best_operations().collect::>().len(), 0); diff --git a/crates/pool/src/mempool/uo_pool.rs b/crates/pool/src/mempool/uo_pool.rs index d547904a8..658287615 100644 --- a/crates/pool/src/mempool/uo_pool.rs +++ b/crates/pool/src/mempool/uo_pool.rs @@ -23,13 +23,13 @@ use parking_lot::RwLock; use rundler_provider::{ DAGasOracleSync, EvmProvider, ProvidersWithEntryPointT, SimulationProvider, StateOverride, }; -use rundler_sim::{Prechecker, Simulator}; +use rundler_sim::{FeeUpdate, Prechecker, Simulator}; use rundler_types::{ pool::{ MempoolError, PaymasterMetadata, PoolOperation, Reputation, ReputationStatus, StakeStatus, }, - Entity, EntityUpdate, EntityUpdateType, EntryPointVersion, GasFees, UserOperation, - UserOperationId, UserOperationVariant, + Entity, EntityUpdate, EntityUpdateType, EntryPointVersion, UserOperation, UserOperationId, + UserOperationVariant, }; use rundler_utils::emit::WithEntryPoint; use tokio::sync::broadcast; @@ -67,8 +67,7 @@ struct UoPoolState { throttled_ops: HashSet, block_number: u64, block_hash: B256, - gas_fees: GasFees, - base_fee: u128, + gas_fees: FeeUpdate, } impl UoPool @@ -95,8 +94,7 @@ where throttled_ops: HashSet::new(), block_number: 0, block_hash: B256::ZERO, - gas_fees: GasFees::default(), - base_fee: 0, + gas_fees: FeeUpdate::default(), }), reputation, paymaster, @@ -347,15 +345,15 @@ where // update required bundle fees and update metrics match self.pool_providers.prechecker().update_fees().await { - Ok((bundle_fees, base_fee)) => { - let max_fee = match format_units(bundle_fees.max_fee_per_gas, "gwei") { + Ok(fees) => { + let max_fee = match format_units(fees.bundle_fees.max_fee_per_gas, "gwei") { Ok(s) => s.parse::().unwrap_or_default(), Err(_) => 0.0, }; self.metrics.current_max_fee_gwei.set(max_fee); let max_priority_fee = - match format_units(bundle_fees.max_priority_fee_per_gas, "gwei") { + match format_units(fees.bundle_fees.max_priority_fee_per_gas, "gwei") { Ok(s) => s.parse::().unwrap_or_default(), Err(_) => 0.0, }; @@ -363,7 +361,7 @@ where .current_max_priority_fee_gwei .set(max_priority_fee); - let base_fee_f64 = match format_units(base_fee, "gwei") { + let base_fee_f64 = match format_units(fees.base_fee, "gwei") { Ok(s) => s.parse::().unwrap_or_default(), Err(_) => 0.0, }; @@ -374,8 +372,7 @@ where let mut state = self.state.write(); state.block_number = update.latest_block_number; state.block_hash = update.latest_block_hash; - state.gas_fees = bundle_fees; - state.base_fee = base_fee; + state.gas_fees = fees; } } Err(e) => { @@ -441,13 +438,11 @@ where // pool maintenance let gas_fees = state.gas_fees; - let base_fee = state.base_fee; state.pool.do_maintenance( update.latest_block_number, update.latest_block_timestamp, da_block_data.as_ref(), gas_fees, - base_fee, ); } let maintenance_time = start.elapsed(); @@ -935,7 +930,7 @@ mod tests { da::DAGasUOData, pool::{PrecheckViolation, SimulationViolation}, v0_6::UserOperation, - EntityInfo, EntityInfos, EntityType, EntryPointVersion, GasFees, + EntityInfo, EntityInfos, EntityType, EntryPointVersion, UserOperation as UserOperationTrait, ValidTimeRange, }; @@ -1898,15 +1893,9 @@ mod tests { args.allowlist.clone().unwrap_or_default(), )); - prechecker.expect_update_fees().returning(|| { - Ok(( - GasFees { - max_fee_per_gas: 0, - max_priority_fee_per_gas: 0, - }, - 0, - )) - }); + prechecker + .expect_update_fees() + .returning(|| Ok(FeeUpdate::default())); for op in ops { prechecker.expect_check().returning(move |_, _| { diff --git a/crates/sim/src/lib.rs b/crates/sim/src/lib.rs index e7419d9d3..41a5a736c 100644 --- a/crates/sim/src/lib.rs +++ b/crates/sim/src/lib.rs @@ -49,8 +49,8 @@ mod precheck; #[cfg(feature = "test-utils")] pub use precheck::MockPrechecker; pub use precheck::{ - PrecheckError, PrecheckReturn, Prechecker, PrecheckerImpl, Settings as PrecheckSettings, - MIN_CALL_GAS_LIMIT, + FeeUpdate, PrecheckError, PrecheckReturn, Prechecker, PrecheckerImpl, + Settings as PrecheckSettings, MIN_CALL_GAS_LIMIT, }; /// Simulation and violation checking diff --git a/crates/sim/src/precheck.rs b/crates/sim/src/precheck.rs index b9bf601d1..40dbe0df1 100644 --- a/crates/sim/src/precheck.rs +++ b/crates/sim/src/precheck.rs @@ -43,6 +43,17 @@ pub struct PrecheckReturn { pub required_pre_verification_gas: u128, } +/// Updated fees from the fee estimator +#[derive(Copy, Clone, Debug, Default)] +pub struct FeeUpdate { + /// Bundle fees + pub bundle_fees: GasFees, + /// User operation fees + pub uo_fees: GasFees, + /// Current base fee + pub base_fee: u128, +} + /// Trait for checking if a user operation is valid before simulation /// according to the spec rules. #[cfg_attr(feature = "test-utils", automock(type UO = rundler_types::v0_6::UserOperation;))] @@ -61,7 +72,7 @@ pub trait Prechecker: Send + Sync { /// Update and return the bundle fees. /// /// This MUST be called at block boundaries before checking any operations. - async fn update_fees(&self) -> anyhow::Result<(GasFees, u128)>; + async fn update_fees(&self) -> anyhow::Result; } /// Precheck error @@ -145,13 +156,7 @@ struct AsyncData { #[derive(Copy, Clone, Debug)] struct AsyncDataCache { - fees: Option, -} - -#[derive(Copy, Clone, Debug)] -struct FeeCache { - bundle_fees: GasFees, - base_fee: u128, + fees: Option, } #[async_trait::async_trait] @@ -183,16 +188,19 @@ where }) } - async fn update_fees(&self) -> anyhow::Result<(GasFees, u128)> { + async fn update_fees(&self) -> anyhow::Result { let (bundle_fees, base_fee) = self.fee_estimator.required_bundle_fees(None).await?; - - let mut cache = self.cache.write().unwrap(); - cache.fees = Some(FeeCache { + let uo_fees = self.fee_estimator.required_op_fees(bundle_fees); + let fee_update = FeeUpdate { bundle_fees, + uo_fees, base_fee, - }); + }; + + let mut cache = self.cache.write().unwrap(); + cache.fees = Some(fee_update); - Ok((bundle_fees, base_fee)) + Ok(fee_update) } } @@ -365,7 +373,7 @@ where op: &UO, block: BlockHashOrNumber, ) -> anyhow::Result { - let (_, base_fee) = self.get_fees().await?; + let FeeUpdate { base_fee, .. } = self.get_fees().await?; let ( factory_exists, @@ -431,9 +439,9 @@ where .context("precheck should get sender balance") } - async fn get_fees(&self) -> anyhow::Result<(GasFees, u128)> { + async fn get_fees(&self) -> anyhow::Result { if let Some(fees) = self.cache.read().unwrap().fees { - return Ok((fees.bundle_fees, fees.base_fee)); + return Ok(fees); } self.update_fees().await }