From 8592d35b2b98a78c5146b48c594249bd994a8f07 Mon Sep 17 00:00:00 2001 From: dancoombs Date: Sat, 14 Oct 2023 08:25:34 -0500 Subject: [PATCH] fix(pool): add back already known check after refactor --- crates/pool/src/mempool/pool.rs | 56 +++++++++----------- crates/pool/src/mempool/uo_pool.rs | 68 ++++++++++++++++++++++++- crates/pool/src/server/remote/client.rs | 2 +- 3 files changed, 91 insertions(+), 35 deletions(-) diff --git a/crates/pool/src/mempool/pool.rs b/crates/pool/src/mempool/pool.rs index d2025cb95..9069e7861 100644 --- a/crates/pool/src/mempool/pool.rs +++ b/crates/pool/src/mempool/pool.rs @@ -97,7 +97,16 @@ impl PoolInner { } } - pub(crate) fn check_replacement_fees(&self, op: &UserOperation) -> MempoolResult<()> { + /// Returns hash of operation to replace if operation is a replacement + pub(crate) fn check_replacement(&self, op: &UserOperation) -> MempoolResult> { + // Check if operation already known + if self + .by_hash + .contains_key(&op.op_hash(self.config.entry_point, self.config.chain_id)) + { + return Err(MempoolError::OperationAlreadyKnown); + } + if let Some(pool_op) = self.by_id.get(&op.id()) { let (replacement_priority_fee, replacement_fee) = self.get_min_replacement_fees(pool_op.uo()); @@ -106,12 +115,19 @@ impl PoolInner { || op.max_fee_per_gas < replacement_fee { return Err(MempoolError::ReplacementUnderpriced( - replacement_priority_fee, - replacement_fee, + pool_op.uo().max_priority_fee_per_gas, + pool_op.uo().max_fee_per_gas, )); } + + Ok(Some( + pool_op + .uo() + .op_hash(self.config.entry_point, self.config.chain_id), + )) + } else { + Ok(None) } - Ok(()) } pub(crate) fn add_operation(&mut self, op: PoolOperation) -> MempoolResult { @@ -226,34 +242,10 @@ impl PoolInner { op: Arc, submission_id: Option, ) -> MempoolResult { - // Check if operation already known - if self - .by_hash - .contains_key(&op.uo.op_hash(self.config.entry_point, self.config.chain_id)) - { - return Err(MempoolError::OperationAlreadyKnown); - } - - // Check for replacement by ID - if let Some(pool_op) = self.by_id.get(&op.uo.id()) { - let (replacement_priority_fee, replacement_fee) = - self.get_min_replacement_fees(pool_op.uo()); - - // replace only if higher gas - if op.uo.max_priority_fee_per_gas >= replacement_priority_fee - && op.uo.max_fee_per_gas >= replacement_fee - { - self.remove_operation_by_hash( - pool_op - .uo() - .op_hash(self.config.entry_point, self.config.chain_id), - ); - } else { - return Err(MempoolError::ReplacementUnderpriced( - pool_op.uo().max_priority_fee_per_gas, - pool_op.uo().max_fee_per_gas, - )); - } + // Check if operation already known or replacing an existing operation + // if replacing, remove the existing operation + if let Some(hash) = self.check_replacement(&op.uo)? { + self.remove_operation_by_hash(hash); } // Check sender count in mempool. If sender has too many operations, must be staked diff --git a/crates/pool/src/mempool/uo_pool.rs b/crates/pool/src/mempool/uo_pool.rs index bfa9fe20e..4b13d3331 100644 --- a/crates/pool/src/mempool/uo_pool.rs +++ b/crates/pool/src/mempool/uo_pool.rs @@ -232,8 +232,9 @@ where ); } - // Check if op is replacing another op, and if so, ensure its fees are high enough - self.state.read().pool.check_replacement_fees(&op)?; + // Check if op is already known or replacing another, and if so, ensure its fees are high enough + // do this before simulation to save resources + self.state.read().pool.check_replacement(&op)?; // Prechecks self.prechecker.check(&op).await?; @@ -722,6 +723,69 @@ mod tests { assert_eq!(pool.best_operations(1, 0).unwrap(), vec![]); } + #[tokio::test] + async fn test_already_known() { + let op = create_op(Address::random(), 0, 0); + let pool = create_pool(vec![op.clone()]); + + let _ = pool + .add_operation(OperationOrigin::Local, op.op.clone()) + .await + .unwrap(); + + let err = pool + .add_operation(OperationOrigin::Local, op.op.clone()) + .await + .unwrap_err(); + assert!(matches!(err, MempoolError::OperationAlreadyKnown)); + + check_ops(pool.best_operations(1, 0).unwrap(), vec![op.op]); + } + + #[tokio::test] + async fn test_replacement_underpriced() { + let op = create_op(Address::random(), 0, 100); + let pool = create_pool(vec![op.clone()]); + + let _ = pool + .add_operation(OperationOrigin::Local, op.op.clone()) + .await + .unwrap(); + + let mut replacement = op.op.clone(); + replacement.max_fee_per_gas = replacement.max_fee_per_gas + 1; + + let err = pool + .add_operation(OperationOrigin::Local, replacement) + .await + .unwrap_err(); + + assert!(matches!(err, MempoolError::ReplacementUnderpriced(_, _))); + + check_ops(pool.best_operations(1, 0).unwrap(), vec![op.op]); + } + + #[tokio::test] + async fn test_replacement() { + let op = create_op(Address::random(), 0, 5); + let pool = create_pool(vec![op.clone()]); + + let _ = pool + .add_operation(OperationOrigin::Local, op.op.clone()) + .await + .unwrap(); + + let mut replacement = op.op.clone(); + replacement.max_fee_per_gas = replacement.max_fee_per_gas + 1; + + let _ = pool + .add_operation(OperationOrigin::Local, replacement.clone()) + .await + .unwrap(); + + check_ops(pool.best_operations(1, 0).unwrap(), vec![replacement]); + } + #[derive(Clone, Debug)] struct OpWithErrors { op: UserOperation, diff --git a/crates/pool/src/server/remote/client.rs b/crates/pool/src/server/remote/client.rs index 48b336463..2910737bf 100644 --- a/crates/pool/src/server/remote/client.rs +++ b/crates/pool/src/server/remote/client.rs @@ -67,7 +67,7 @@ impl RemotePoolClient { } // Handler for the new block subscription. This will attempt to resubscribe if the gRPC - // connection disconnects using expenential backoff. + // connection disconnects using exponential backoff. async fn new_heads_subscription_handler( client: OpPoolClient, tx: mpsc::UnboundedSender,