From a5456a9c7050db471b9ee7edfc57aa0767eac5c8 Mon Sep 17 00:00:00 2001 From: Felix Leupold Date: Thu, 28 Mar 2024 11:31:44 +0100 Subject: [PATCH] Reject too gas intensive solutions already in driver competition (#2563) # Description #2526 started limiting the amount of gas the driver allows per solution. However, the limit was only applied after solutions had been ranked and participated in the autopilot's auction. This lead to large settlements effectively clogging the auction cycle as solvers would continue to win the competition but then fail to submit. This PR filters out such solutions already in the driver internal ranking phase so that too large settlements never participate as solution candidate. # Changes - [x] Make `Gas::new` return an error if gas limit is too high - [x] Introduce a new solver error type that solver engines are notified about. ## How to test Adjusted high gas limit driver test ## Related Issues Fixes #2511 (or rather makes it obsolete) --- .../src/domain/competition/solution/mod.rs | 2 + .../domain/competition/solution/settlement.rs | 37 +++++---- crates/driver/src/infra/notify/mod.rs | 4 + crates/driver/src/tests/cases/settle.rs | 75 +++++++++++-------- crates/driver/src/tests/setup/blockchain.rs | 8 +- crates/driver/src/tests/setup/mod.rs | 9 +++ 6 files changed, 83 insertions(+), 52 deletions(-) diff --git a/crates/driver/src/domain/competition/solution/mod.rs b/crates/driver/src/domain/competition/solution/mod.rs index f909d08a3d..6f78a064c6 100644 --- a/crates/driver/src/domain/competition/solution/mod.rs +++ b/crates/driver/src/domain/competition/solution/mod.rs @@ -522,6 +522,8 @@ pub mod error { NonBufferableTokensUsed(BTreeSet), #[error("invalid internalization: uninternalized solution fails to simulate")] FailingInternalization, + #[error("Gas estimate of {0:?} exceeded the per settlement limit of {1:?}")] + GasLimitExceeded(eth::Gas, eth::Gas), #[error("insufficient solver account Ether balance, required {0:?}")] SolverAccountInsufficientBalance(eth::Ether), #[error("attempted to merge settlements generated by different solvers")] diff --git a/crates/driver/src/domain/competition/solution/settlement.rs b/crates/driver/src/domain/competition/solution/settlement.rs index 44372c2f39..dc2ad56ed1 100644 --- a/crates/driver/src/domain/competition/solution/settlement.rs +++ b/crates/driver/src/domain/competition/solution/settlement.rs @@ -123,7 +123,7 @@ impl Settlement { ) .await?; let price = eth.gas_price().await?; - let gas = Gas::new(gas, eth.block_gas_limit(), price); + let gas = Gas::new(gas, eth.block_gas_limit(), price)?; // Ensure that the solver has sufficient balance for the settlement to be mined. if eth.balance(settlement.solver).await? < gas.required_balance() { @@ -337,18 +337,11 @@ pub struct Gas { impl Gas { /// Computes settlement gas parameters given estimates for gas and gas /// price. - pub fn new(estimate: eth::Gas, block_limit: eth::Gas, price: eth::GasPrice) -> Self { - // Specify a different gas limit than the estimated gas when executing a - // settlement transaction. This allows the transaction to be resilient - // to small variations in actual gas usage. - // Also, some solutions can have significant gas refunds that are refunded at - // the end of execution, so we want to increase gas limit enough so - // those solutions don't revert with out of gas error. - const GAS_LIMIT_FACTOR: f64 = 2.0; - let estimate_with_buffer = - eth::U256::from_f64_lossy(eth::U256::to_f64_lossy(estimate.into()) * GAS_LIMIT_FACTOR) - .into(); - + pub fn new( + estimate: eth::Gas, + block_limit: eth::Gas, + price: eth::GasPrice, + ) -> Result { // We don't allow for solutions to take up more than half of the block's gas // limit. This is to ensure that block producers attempt to include the // settlement transaction in the next block as long as it is reasonably @@ -360,12 +353,26 @@ impl Gas { // whose gas limit exceed the remaining space (without simulating the actual // gas required). let max_gas = eth::Gas(block_limit.0 / 2); + if estimate > max_gas { + return Err(solution::Error::GasLimitExceeded(estimate, max_gas)); + } + + // Specify a different gas limit than the estimated gas when executing a + // settlement transaction. This allows the transaction to be resilient + // to small variations in actual gas usage. + // Also, some solutions can have significant gas refunds that are refunded at + // the end of execution, so we want to increase gas limit enough so + // those solutions don't revert with out of gas error. + const GAS_LIMIT_FACTOR: f64 = 2.0; + let estimate_with_buffer = + eth::U256::from_f64_lossy(eth::U256::to_f64_lossy(estimate.into()) * GAS_LIMIT_FACTOR) + .into(); - Self { + Ok(Self { estimate, limit: std::cmp::min(max_gas, estimate_with_buffer), price, - } + }) } /// The balance required to ensure settlement execution with the given gas diff --git a/crates/driver/src/infra/notify/mod.rs b/crates/driver/src/infra/notify/mod.rs index 300dafb7e6..b01ac403ea 100644 --- a/crates/driver/src/infra/notify/mod.rs +++ b/crates/driver/src/infra/notify/mod.rs @@ -76,6 +76,10 @@ pub fn encoding_failed( } solution::Error::FailingInternalization => return, solution::Error::DifferentSolvers => return, + solution::Error::GasLimitExceeded(used, limit) => notification::Kind::DriverError(format!( + "Settlement gas limit exceeded: used {}, limit {}", + used.0, limit.0 + )), }; solver.notify(auction_id, Some(solution_id.clone()), notification); diff --git a/crates/driver/src/tests/cases/settle.rs b/crates/driver/src/tests/cases/settle.rs index aec5fda80d..867eea843f 100644 --- a/crates/driver/src/tests/cases/settle.rs +++ b/crates/driver/src/tests/cases/settle.rs @@ -1,10 +1,13 @@ -use crate::{ - domain::competition::order, - tests::{ - self, - cases::{EtherExt, DEFAULT_SOLVER_FEE}, - setup::{ab_order, ab_pool, ab_solution}, +use { + crate::{ + domain::competition::order, + tests::{ + self, + cases::{EtherExt, DEFAULT_SOLVER_FEE}, + setup::{ab_order, ab_pool, ab_solution}, + }, }, + web3::Transport, }; /// Run a matrix of tests for all meaningful combinations of order kind and @@ -73,33 +76,39 @@ async fn private_rpc_with_high_risk_solution() { err.kind("FailedToSubmit"); } -// /// Checks that we can settle transactions that have a gas limit higher than -// /// half the block size -// #[tokio::test] -// #[ignore] -// async fn high_gas_limit() { -// let test = tests::setup() -// .name("high gas limit") -// .pool(ab_pool()) -// .order(ab_order()) -// .solution(ab_solution().increase_gas(15_000_000)) -// .done() -// .await; +#[tokio::test] +#[ignore] +async fn too_much_gas() { + let test = tests::setup() + .name("too much gas") + .pool(ab_pool()) + .order(ab_order()) + .solution(ab_solution().increase_gas(6_000_000)) + .rpc_args(vec!["--gas-limit".into(), "10000000".into()]) + .done() + .await; + test.solve().await.ok().empty(); +} -// // Set to 30M for solving purposes -// test.web3() -// .transport() -// .execute("evm_setBlockGasLimit", vec![serde_json::json!(30_000_000)]) -// .await -// .unwrap(); +#[tokio::test] +#[ignore] +async fn high_gas_limit() { + let test = tests::setup() + .name("high gas limit") + .pool(ab_pool()) + .order(ab_order()) + .solution(ab_solution().increase_gas(4_000_000)) + .rpc_args(vec!["--gas-limit".into(), "10000000".into()]) + .done() + .await; -// test.solve().await.ok(); + test.solve().await.ok().orders(&[ab_order()]); -// // Assume validators downvoted gas limit to 29.9M, solution still settles -// test.web3() -// .transport() -// .execute("evm_setBlockGasLimit", vec![serde_json::json!(29_900_000)]) -// .await -// .unwrap(); -// test.settle().await.ok().await; -// } + // Assume validators downvoted gas limit, solution still settles + test.web3() + .transport() + .execute("evm_setBlockGasLimit", vec![serde_json::json!(9_000_000)]) + .await + .unwrap(); + test.settle().await.ok().await; +} diff --git a/crates/driver/src/tests/setup/blockchain.rs b/crates/driver/src/tests/setup/blockchain.rs index c805e85e0f..7b5a8288e3 100644 --- a/crates/driver/src/tests/setup/blockchain.rs +++ b/crates/driver/src/tests/setup/blockchain.rs @@ -164,6 +164,7 @@ pub struct Config { pub trader_secret_key: SecretKey, pub solvers: Vec, pub settlement_address: Option, + pub rpc_args: Vec, } impl Blockchain { @@ -175,7 +176,7 @@ impl Blockchain { // should be happening from the primary_account of the node, will do this // later - let node = Node::new().await; + let node = Node::new(&config.rpc_args).await; let web3 = Web3::new(DynTransport::new( web3::transports::Http::new(&node.url()).expect("valid URL"), )); @@ -772,7 +773,7 @@ impl std::fmt::Debug for Node { impl Node { /// Spawn a new node instance. - async fn new() -> Self { + async fn new(extra_args: &[String]) -> Self { use tokio::io::AsyncBufReadExt as _; // Allow using some custom logic to spawn `anvil` by setting `ANVIL_COMMAND`. @@ -784,8 +785,7 @@ impl Node { .arg("0") // use 0 to let `anvil` use any open port .arg("--balance") .arg("1000000") - .arg("--gas-limit") - .arg("30000000") + .args(extra_args) .stdout(std::process::Stdio::piped()) .spawn() .unwrap(); diff --git a/crates/driver/src/tests/setup/mod.rs b/crates/driver/src/tests/setup/mod.rs index 98cc1be4e2..1e7b6c9e46 100644 --- a/crates/driver/src/tests/setup/mod.rs +++ b/crates/driver/src/tests/setup/mod.rs @@ -485,6 +485,7 @@ pub fn setup() -> Setup { enable_simulation: true, settlement_address: Default::default(), mempools: vec![Mempool::Public], + rpc_args: vec!["--gas-limit".into(), "10000000".into()], } } @@ -506,6 +507,8 @@ pub struct Setup { settlement_address: Option, /// Via which mempool the solutions should be submitted mempools: Vec, + /// Extra configuration for the RPC node + rpc_args: Vec, } /// The validity of a solution. @@ -763,6 +766,11 @@ impl Setup { self } + pub fn rpc_args(mut self, rpc_args: Vec) -> Self { + self.rpc_args = rpc_args; + self + } + /// Create the test: set up onchain contracts and pools, start a mock HTTP /// server for the solver and start the HTTP server for the driver. pub async fn done(self) -> Test { @@ -799,6 +807,7 @@ impl Setup { trader_secret_key, solvers: self.solvers.clone(), settlement_address: self.settlement_address, + rpc_args: self.rpc_args, }) .await; let mut solutions = Vec::new();