diff --git a/Cargo.lock b/Cargo.lock index 4f236b51b..6e324f522 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4108,6 +4108,7 @@ dependencies = [ "serde", "serde_json", "serde_with", + "strum", "thiserror", "tokio", "tracing", diff --git a/bin/rundler/src/cli/pool.rs b/bin/rundler/src/cli/pool.rs index cc57f2ccd..f549005d6 100644 --- a/bin/rundler/src/cli/pool.rs +++ b/bin/rundler/src/cli/pool.rs @@ -59,12 +59,12 @@ pub struct PoolArgs { pub max_size_in_bytes: usize, #[arg( - long = "pool.max_userops_per_sender", - name = "pool.max_userops_per_sender", - env = "POOL_MAX_USEROPS_PER_SENDER", + long = "pool.same_sender_mempool_count", + name = "pool.same_sender_mempool_count", + env = "SAME_SENDER_MEMPOOL_COUNT", default_value = "4" )] - pub max_userops_per_sender: usize, + pub same_sender_mempool_count: usize, #[arg( long = "pool.min_replacement_fee_increase_percentage", @@ -114,7 +114,7 @@ pub struct PoolArgs { long = "pool.throttled_entity_live_blocks", name = "pool.throttled_entity_live_blocks", env = "POOL_THROTTLED_ENTITY_LIVE_BLOCKS", - default_value = "4" + default_value = "10" )] pub throttled_entity_live_blocks: u64, } @@ -156,7 +156,7 @@ impl PoolArgs { chain_id: common.chain_id, // Currently use the same shard count as the number of builders num_shards: common.num_builders, - max_userops_per_sender: self.max_userops_per_sender, + same_sender_mempool_count: self.same_sender_mempool_count, min_replacement_fee_increase_percentage: self .min_replacement_fee_increase_percentage, max_size_of_pool_bytes: self.max_size_in_bytes, diff --git a/crates/pool/proto/op_pool/op_pool.proto b/crates/pool/proto/op_pool/op_pool.proto index 4ae00299b..f5b4df0c5 100644 --- a/crates/pool/proto/op_pool/op_pool.proto +++ b/crates/pool/proto/op_pool/op_pool.proto @@ -409,7 +409,7 @@ message PaymasterBalanceTooLow { message MaxOperationsReachedError { uint64 num_ops = 1; - bytes sender_address = 2; + bytes entity_address = 2; } message EntityThrottledError { diff --git a/crates/pool/src/mempool/error.rs b/crates/pool/src/mempool/error.rs index c36565c31..5fed9ab6f 100644 --- a/crates/pool/src/mempool/error.rs +++ b/crates/pool/src/mempool/error.rs @@ -35,8 +35,8 @@ pub enum MempoolError { /// and the replacement operation has lower gas price. #[error("Replacement operation underpriced. Existing priority fee: {0}. Existing fee: {1}")] ReplacementUnderpriced(U256, U256), - /// Max operations reached for this sender - #[error("Max operations ({0}) reached for sender {1}")] + /// Max operations reached for unstaked sender [UREP-010] or unstaked non-sender entity [UREP-020] + #[error("Max operations ({0}) reached for entity {1}")] MaxOperationsReached(usize, Address), /// Multiple roles violation /// Spec rule: STO-040 diff --git a/crates/pool/src/mempool/mod.rs b/crates/pool/src/mempool/mod.rs index 11bc8f228..0a8827570 100644 --- a/crates/pool/src/mempool/mod.rs +++ b/crates/pool/src/mempool/mod.rs @@ -37,7 +37,6 @@ use ethers::types::{Address, H256, U256}; use mockall::automock; use rundler_sim::{EntityInfos, MempoolConfig, PrecheckSettings, SimulationSettings}; use rundler_types::{Entity, EntityType, EntityUpdate, UserOperation, ValidTimeRange}; -use strum::IntoEnumIterator; use tonic::async_trait; pub(crate) use uo_pool::UoPool; @@ -119,7 +118,7 @@ pub struct PoolConfig { /// Chain ID this pool targets pub chain_id: u64, /// The maximum number of operations an unstaked sender can have in the mempool - pub max_userops_per_sender: usize, + pub same_sender_mempool_count: usize, /// The minimum fee bump required to replace an operation in the mempool /// Applies to both priority fee and fee. Expressed as an integer percentage value pub min_replacement_fee_increase_percentage: u64, @@ -215,9 +214,11 @@ pub struct PaymasterMetadata { impl PoolOperation { /// Returns true if the operation contains the given entity. pub fn contains_entity(&self, entity: &Entity) -> bool { - self.entity_address(entity.kind) - .map(|address| address == entity.address) - .unwrap_or(false) + if let Some(e) = self.entity_infos.get(entity.kind) { + e.address == entity.address + } else { + false + } } /// Returns true if the operation requires the given entity to stake. @@ -239,20 +240,31 @@ impl PoolOperation { /// Returns an iterator over all entities that are included in this operation. pub fn entities(&'_ self) -> impl Iterator + '_ { - EntityType::iter().filter_map(|entity| { - self.entity_address(entity) - .map(|address| Entity::new(entity, address)) + self.entity_infos + .entities() + .map(|(t, entity)| Entity::new(t, entity.address)) + } + + /// Returns an iterator over all entities that need stake in this operation. This can be a subset of entities that are staked in the operation. + pub fn entities_requiring_stake(&'_ self) -> impl Iterator + '_ { + self.entity_infos.entities().filter_map(|(t, entity)| { + if self.requires_stake(t) { + Entity::new(t, entity.address).into() + } else { + None + } }) } - /// Returns an iterator over all staked entities that are included in this operation. - pub fn staked_entities(&'_ self) -> impl Iterator + '_ { - EntityType::iter() - .filter(|entity| self.requires_stake(*entity)) - .filter_map(|entity| { - self.entity_address(entity) - .map(|address| Entity::new(entity, address)) - }) + /// Return all the unstaked entities that are used in this operation. + pub fn unstaked_entities(&'_ self) -> impl Iterator + '_ { + self.entity_infos.entities().filter_map(|(t, entity)| { + if entity.is_staked { + None + } else { + Entity::new(t, entity.address).into() + } + }) } /// Compute the amount of heap memory the PoolOperation takes up. @@ -261,19 +273,12 @@ impl PoolOperation { + self.uo.heap_size() + self.entities_needing_stake.len() * std::mem::size_of::() } - - fn entity_address(&self, entity: EntityType) -> Option
{ - match entity { - EntityType::Account => Some(self.uo.sender), - EntityType::Paymaster => self.uo.paymaster(), - EntityType::Factory => self.uo.factory(), - EntityType::Aggregator => self.aggregator, - } - } } #[cfg(test)] mod tests { + use rundler_sim::EntityInfo; + use super::*; #[test] @@ -298,7 +303,24 @@ mod tests { sim_block_number: 0, entities_needing_stake: vec![EntityType::Account, EntityType::Aggregator], account_is_staked: true, - entity_infos: EntityInfos::default(), + entity_infos: EntityInfos { + factory: Some(EntityInfo { + address: factory, + is_staked: false, + }), + sender: EntityInfo { + address: sender, + is_staked: false, + }, + paymaster: Some(EntityInfo { + address: paymaster, + is_staked: false, + }), + aggregator: Some(EntityInfo { + address: aggregator, + is_staked: false, + }), + }, }; assert!(po.requires_stake(EntityType::Account)); @@ -306,11 +328,6 @@ mod tests { assert!(!po.requires_stake(EntityType::Factory)); assert!(po.requires_stake(EntityType::Aggregator)); - assert_eq!(po.entity_address(EntityType::Account), Some(sender)); - assert_eq!(po.entity_address(EntityType::Paymaster), Some(paymaster)); - assert_eq!(po.entity_address(EntityType::Factory), Some(factory)); - assert_eq!(po.entity_address(EntityType::Aggregator), Some(aggregator)); - let entities = po.entities().collect::>(); assert_eq!(entities.len(), 4); for e in entities { diff --git a/crates/pool/src/mempool/pool.rs b/crates/pool/src/mempool/pool.rs index 3f8e9aa74..26b6d2711 100644 --- a/crates/pool/src/mempool/pool.rs +++ b/crates/pool/src/mempool/pool.rs @@ -39,7 +39,6 @@ use crate::chain::{DepositInfo, MinedOp}; pub(crate) struct PoolInnerConfig { entry_point: Address, chain_id: u64, - max_userops_per_sender: usize, max_size_of_pool_bytes: usize, min_replacement_fee_increase_percentage: u64, throttled_entity_mempool_count: u64, @@ -51,7 +50,6 @@ impl From for PoolInnerConfig { Self { entry_point: config.entry_point, chain_id: config.chain_id, - max_userops_per_sender: config.max_userops_per_sender, max_size_of_pool_bytes: config.max_size_of_pool_bytes, min_replacement_fee_increase_percentage: config.min_replacement_fee_increase_percentage, throttled_entity_mempool_count: config.throttled_entity_mempool_count, @@ -437,16 +435,6 @@ impl PoolInner { self.remove_operation_by_hash(hash); } - // Check sender count in mempool. If sender has too many operations, must be staked - if self.address_count(&op.uo.sender) >= self.config.max_userops_per_sender - && !op.account_is_staked - { - return Err(MempoolError::MaxOperationsReached( - self.config.max_userops_per_sender, - op.uo.sender, - )); - } - // check or update paymaster balance if let Some(paymaster_meta) = paymaster_meta { self.paymaster_balances @@ -613,6 +601,8 @@ impl PoolMetrics { #[cfg(test)] mod tests { + use rundler_sim::{EntityInfo, EntityInfos}; + use super::*; #[test] @@ -796,6 +786,10 @@ mod tests { ]; for mut op in ops.into_iter() { op.aggregator = Some(agg); + op.entity_infos.aggregator = Some(EntityInfo { + address: agg, + is_staked: false, + }); pool.add_operation(op.clone(), None).unwrap(); } assert_eq!(pool.by_hash.len(), 3); @@ -817,6 +811,10 @@ mod tests { ]; for mut op in ops.into_iter() { op.uo.paymaster_and_data = paymaster.as_bytes().to_vec().into(); + op.entity_infos.paymaster = Some(EntityInfo { + address: op.uo.paymaster().unwrap(), + is_staked: false, + }); pool.add_operation(op.clone(), None).unwrap(); } assert_eq!(pool.by_hash.len(), 3); @@ -827,20 +825,6 @@ mod tests { assert!(pool.best.is_empty()); } - #[test] - fn too_many_ops() { - let args = conf(); - let mut pool = PoolInner::new(args.clone()); - let addr = Address::random(); - for i in 0..args.max_userops_per_sender { - let op = create_op(addr, i, 1); - pool.add_operation(op, None).unwrap(); - } - - let op = create_op(addr, args.max_userops_per_sender, 1); - assert!(pool.add_operation(op, None).is_err()); - } - #[test] fn address_count() { let mut pool = PoolInner::new(conf()); @@ -851,8 +835,20 @@ mod tests { let mut op = create_op(sender, 0, 1); op.uo.paymaster_and_data = paymaster.as_bytes().to_vec().into(); + op.entity_infos.paymaster = Some(EntityInfo { + address: op.uo.paymaster().unwrap(), + is_staked: false, + }); op.uo.init_code = factory.as_bytes().to_vec().into(); + op.entity_infos.factory = Some(EntityInfo { + address: op.uo.factory().unwrap(), + is_staked: false, + }); op.aggregator = Some(aggregator); + op.entity_infos.aggregator = Some(EntityInfo { + address: aggregator, + is_staked: false, + }); let count = 5; let mut hashes = vec![]; @@ -901,11 +897,11 @@ mod tests { pool.add_operation(op, None).unwrap(); } - let op = create_op(Address::random(), args.max_userops_per_sender, 1); + let op = create_op(Address::random(), 4, 1); assert!(pool.add_operation(op, None).is_err()); // on equal gas, worst should remain because it came first - let op = create_op(Address::random(), args.max_userops_per_sender, 2); + let op = create_op(Address::random(), 4, 2); let result = pool.add_operation(op, None); assert!(result.is_ok(), "{:?}", result.err()); } @@ -949,6 +945,10 @@ mod tests { let mut po1 = create_op(sender, 0, 10); po1.uo.max_priority_fee_per_gas = 10.into(); po1.uo.paymaster_and_data = paymaster1.as_bytes().to_vec().into(); + po1.entity_infos.paymaster = Some(EntityInfo { + address: po1.uo.paymaster().unwrap(), + is_staked: false, + }); let _ = pool.add_operation(po1, None).unwrap(); assert_eq!(pool.address_count(&paymaster1), 1); @@ -956,6 +956,10 @@ mod tests { let mut po2 = create_op(sender, 0, 11); po2.uo.max_priority_fee_per_gas = 11.into(); po2.uo.paymaster_and_data = paymaster2.as_bytes().to_vec().into(); + po2.entity_infos.paymaster = Some(EntityInfo { + address: po2.uo.paymaster().unwrap(), + is_staked: false, + }); let _ = pool.add_operation(po2.clone(), None).unwrap(); assert_eq!(pool.address_count(&sender), 1); @@ -1029,7 +1033,6 @@ mod tests { PoolInnerConfig { entry_point: Address::random(), chain_id: 1, - max_userops_per_sender: 16, min_replacement_fee_increase_percentage: 10, max_size_of_pool_bytes: 20 * mem_size_of_ordered_pool_op(), throttled_entity_mempool_count: 4, @@ -1051,8 +1054,18 @@ mod tests { sender, nonce: nonce.into(), max_fee_per_gas: max_fee_per_gas.into(), + ..UserOperation::default() }, + entity_infos: EntityInfos { + factory: None, + sender: EntityInfo { + address: sender, + is_staked: false, + }, + paymaster: None, + aggregator: None, + }, ..PoolOperation::default() } } diff --git a/crates/pool/src/mempool/reputation.rs b/crates/pool/src/mempool/reputation.rs index ad1ee7b64..0020d2678 100644 --- a/crates/pool/src/mempool/reputation.rs +++ b/crates/pool/src/mempool/reputation.rs @@ -320,7 +320,7 @@ impl AddressReputation { // make sure we aren't dividing by 0 0 } else { - included * self.params.inclusion_rate_factor / seen + std::cmp::min(included, 10_000) + self.params.inclusion_rate_factor * included / seen + std::cmp::min(included, 10_000) }; // return ops allowed, as defined by UREP-020 diff --git a/crates/pool/src/mempool/uo_pool.rs b/crates/pool/src/mempool/uo_pool.rs index c269af40c..e4d820706 100644 --- a/crates/pool/src/mempool/uo_pool.rs +++ b/crates/pool/src/mempool/uo_pool.rs @@ -195,8 +195,8 @@ where state.throttled_ops.remove(&op.hash); if let Some(op) = state.pool.mine_operation(op, update.latest_block_number) { - // Only account for a staked entity once - for entity_addr in op.staked_entities().map(|e| e.address).unique() { + // Only account for an entity once + for entity_addr in op.entities().map(|e| e.address).unique() { self.reputation.add_included(entity_addr); } mined_op_count += 1; @@ -208,9 +208,9 @@ where } if let Some(op) = state.pool.unmine_operation(op) { - // Only account for a staked entity once - for entity_addr in op.staked_entities().map(|e| e.address).unique() { - self.reputation.add_included(entity_addr); + // Only account for an entity once + for entity_addr in op.entities().map(|e| e.address).unique() { + self.reputation.remove_included(entity_addr); } unmined_op_count += 1; } @@ -250,17 +250,17 @@ where if let Some(block) = block_seen { if update.latest_block_number - block > self.config.throttled_entity_live_blocks { - to_remove.insert(*hash); + to_remove.insert((*hash, block)); } } } - for hash in to_remove { + for (hash, added_at_block) in to_remove { state.pool.remove_operation_by_hash(hash); state.throttled_ops.remove(&hash); self.emit(OpPoolEvent::RemovedOp { op_hash: hash, reason: OpRemovalReason::ThrottledAndOld { - added_at_block_number: state.block_number, + added_at_block_number: added_at_block, current_block_number: update.latest_block_number, }, }) @@ -415,6 +415,34 @@ where entity_infos: sim_result.entity_infos, }; + // Check sender count in mempool. If sender has too many operations, must be staked + { + let state = self.state.read(); + if !pool_op.account_is_staked + && state.pool.address_count(&pool_op.uo.sender) + >= self.config.same_sender_mempool_count + { + return Err(MempoolError::MaxOperationsReached( + self.config.same_sender_mempool_count, + pool_op.uo.sender, + )); + } + + // Check unstaked non-sender entity counts in the mempool + for entity in pool_op + .unstaked_entities() + .filter(|e| e.address != pool_op.entity_infos.sender.address) + { + let ops_allowed = self.reputation.get_ops_allowed(entity.address); + if state.pool.address_count(&entity.address) >= ops_allowed as usize { + return Err(MempoolError::MaxOperationsReached( + ops_allowed as usize, + entity.address, + )); + } + } + } + // Add op to pool let hash = { let mut state = self.state.write(); @@ -628,11 +656,12 @@ impl UoPoolMetrics { mod tests { use std::collections::HashMap; - use ethers::types::Bytes; + use ethers::types::{Bytes, H160}; use rundler_provider::{MockEntryPoint, MockPaymasterHelper}; use rundler_sim::{ - MockPrechecker, MockSimulator, PrecheckError, PrecheckSettings, PrecheckViolation, - SimulationError, SimulationResult, SimulationSettings, SimulationViolation, ViolationError, + EntityInfo, EntityInfos, MockPrechecker, MockSimulator, PrecheckError, PrecheckSettings, + PrecheckViolation, SimulationError, SimulationResult, SimulationSettings, + SimulationViolation, ViolationError, }; use rundler_types::{DepositInfo, EntityType, GasFees, ValidTimeRange}; @@ -915,7 +944,6 @@ mod tests { #[tokio::test] async fn test_throttled_account() { let address = Address::random(); - let mut ops = Vec::new(); for i in 0..5 { ops.push(create_op_with_errors(address, i, 2, None, None, true)); @@ -1230,6 +1258,26 @@ mod tests { assert_eq!(pool_op, None); } + #[tokio::test] + async fn too_many_ops_for_unstaked_sender() { + let mut ops = vec![]; + let addr = H160::random(); + for i in 0..5 { + ops.push(create_op(addr, i, 1, None)) + } + let pool = create_pool(ops.clone()); + + for op in ops.iter().take(4) { + pool.add_operation(OperationOrigin::Local, op.op.clone()) + .await + .unwrap(); + } + assert!(pool + .add_operation(OperationOrigin::Local, ops[4].op.clone()) + .await + .is_err()); + } + #[derive(Clone, Debug)] struct OpWithErrors { op: UserOperation, @@ -1298,6 +1346,13 @@ mod tests { account_is_staked: op.staked, block_number: Some(0), valid_time_range: op.valid_time_range, + entity_infos: EntityInfos { + sender: EntityInfo { + address: op.op.sender, + is_staked: false, + }, + ..EntityInfos::default() + }, ..SimulationResult::default() }) } @@ -1307,7 +1362,6 @@ mod tests { let args = PoolConfig { entry_point: Address::random(), chain_id: 1, - max_userops_per_sender: 16, min_replacement_fee_increase_percentage: 10, max_size_of_pool_bytes: 10000, blocklist: None, @@ -1316,6 +1370,7 @@ mod tests { sim_settings: SimulationSettings::default(), mempool_channel_configs: HashMap::new(), num_shards: 1, + same_sender_mempool_count: 4, throttled_entity_mempool_count: 4, throttled_entity_live_blocks: 10, }; @@ -1496,8 +1551,8 @@ mod tests { fn get_ops_allowed(&self, address: Address) -> u64 { let counts = self.counts.read(); - let seen = *counts.seen.get(&address).unwrap(); - let included = *counts.included.get(&address).unwrap(); + let seen = *counts.seen.get(&address).unwrap_or(&0); + let included = *counts.included.get(&address).unwrap_or(&0); let inclusion_based_count = if seen == 0 { // make sure we aren't dividing by 0 0 diff --git a/crates/pool/src/server/remote/error.rs b/crates/pool/src/server/remote/error.rs index 88e0af3f9..a64e2b480 100644 --- a/crates/pool/src/server/remote/error.rs +++ b/crates/pool/src/server/remote/error.rs @@ -90,7 +90,7 @@ impl TryFrom for MempoolError { Some(mempool_error::Error::MaxOperationsReached(e)) => { MempoolError::MaxOperationsReached( e.num_ops as usize, - from_bytes(&e.sender_address)?, + from_bytes(&e.entity_address)?, ) } Some(mempool_error::Error::EntityThrottled(e)) => MempoolError::EntityThrottled( @@ -156,7 +156,7 @@ impl From for ProtoMempoolError { error: Some(mempool_error::Error::MaxOperationsReached( MaxOperationsReachedError { num_ops: ops as u64, - sender_address: addr.as_bytes().to_vec(), + entity_address: addr.as_bytes().to_vec(), }, )), }, diff --git a/crates/sim/Cargo.toml b/crates/sim/Cargo.toml index 43423d71e..7e6ba4faf 100644 --- a/crates/sim/Cargo.toml +++ b/crates/sim/Cargo.toml @@ -27,6 +27,7 @@ reqwest.workspace = true tokio = { workspace = true, features = ["macros"] } tracing.workspace = true url.workspace = true +strum.workspace = true mockall = {workspace = true, optional = true } diff --git a/crates/sim/src/simulation/simulation.rs b/crates/sim/src/simulation/simulation.rs index dd2d7560b..19cb56c0c 100644 --- a/crates/sim/src/simulation/simulation.rs +++ b/crates/sim/src/simulation/simulation.rs @@ -32,6 +32,7 @@ use rundler_types::{ contracts::i_entry_point::FailedOp, Entity, EntityType, StorageSlot, UserOperation, ValidTimeRange, }; +use strum::IntoEnumIterator; use super::{ mempool::{match_mempools, AllowEntity, AllowRule, MempoolConfig, MempoolMatchResult}, @@ -798,6 +799,11 @@ impl EntityInfos { } } + /// Get iterator over the entities + pub fn entities(&'_ self) -> impl Iterator + '_ { + EntityType::iter().filter_map(|t| self.get(t).map(|info| (t, info))) + } + fn override_is_staked(&mut self, allow_unstaked_addresses: &HashSet
) { if let Some(mut factory) = self.factory { factory.override_is_staked(allow_unstaked_addresses) @@ -811,7 +817,8 @@ impl EntityInfos { } } - fn get(self, entity: EntityType) -> Option { + /// Get the EntityInfo of a specific entity + pub fn get(self, entity: EntityType) -> Option { match entity { EntityType::Factory => self.factory, EntityType::Account => Some(self.sender), diff --git a/docs/cli.md b/docs/cli.md index 9687ee28f..8af37ebef 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -117,8 +117,8 @@ List of command line options for configuring the Pool. - *Only required when running in distributed mode* - `--pool.max_size_in_bytes`: Maximum size in bytes for the pool (default: `500000000`, `0.5 GB`) - env: *POOL_MAX_SIZE_IN_BYTES* -- `--pool.max_userops_per_sender`: Maximum number of user operations per sender (default: `4`) - - env: *POOL_MAX_USEROPS_PER_SENDER* +- `--pool.same_sender_mempool_count`: Maximum number of user operations for an unstaked sender (default: `4`) + - env: *POOL_SAME_SENDER_MEMPOOL_COUNT* - `--pool.min_replacement_fee_increase_percentage`: Minimum replacement fee increase percentage (default: `10`) - env: *POOL_MIN_REPLACEMENT_FEE_INCREASE_PERCENTAGE* - `--pool.blocklist_path`: Path to a blocklist file (e.g `blocklist.json`, `s3://my-bucket/blocklist.json`)