Skip to content

Commit

Permalink
Limit maxFeePerGas to reasonable value (#2352)
Browse files Browse the repository at this point in the history
# Description
We are seeing a bunch of simulation failures of seasolver on GnosisChain
due to `InsufficientBalance`. Those only show up after their solution
has been chosen as the winning one and is trying to settle, thus
delaying settlements by one auction (or potentially more if their solver
keeps winning).
This stems from the fact that the current `required_balance` method,
which we check during solution simulation, doesn't use the
`maxFeePerGas` (which is 1000 gwei) but instead some multiple of the
current base fee (<1 gwei on Gnosis Chain). However, the RPC node
requires an account to have at least `gasLimit * maxFeePerGas` ETH of
balance in order to accept its transaction.

This would lead to a balance requirement of ~15xDAI on Gnosis Chain
(many orders of magnitude more than is actually needed for a
transaction). At the same time, EIP1559 specifies that the base fee can
only grow by 12.5% per block. Therefore setting a `maxFeePerGas` very
high unnecessarily restricts the balance required to execute a
settlement within the expected time frame (2 blocks for the auction + 5
blocks max for inclusion). It was introduced in #2295 to simplify our
logic. This PR somewhat reverts those changes by limiting the
`maxFeePerGas` to a reasonable upper bound given the current base fee.

# Changes
<!-- List of detailed changes (how the change is accomplished) -->

- [x] When creating a GasPrice take current base fee into account when
computing `maxFeePerGas`
- [x] Use `maxFeePerGas` in `required_balance`

## How to test
Existing unit tests (will look into adding a test for the solver balance
check)
  • Loading branch information
fleupold authored Feb 2, 2024
1 parent e3ca21c commit e6e1495
Show file tree
Hide file tree
Showing 9 changed files with 100 additions and 71 deletions.
2 changes: 1 addition & 1 deletion crates/driver/src/boundary/mempool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ impl Mempool {
.transaction_count(solver.address().into(), None)
.await
.map_err(anyhow::Error::from)?;
let max_fee_per_gas = eth::U256::from(settlement.gas.price.max).to_f64_lossy();
let max_fee_per_gas = eth::U256::from(settlement.gas.price.max()).to_f64_lossy();
let gas_price_estimator = SubmitterGasPriceEstimator {
inner: self.gas_price_estimator.as_ref(),
max_fee_per_gas: max_fee_per_gas.min(self.config.gas_price_cap.to_f64_lossy()),
Expand Down
23 changes: 1 addition & 22 deletions crates/driver/src/domain/competition/solution/settlement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -396,27 +396,6 @@ impl Gas {
/// The balance required to ensure settlement execution with the given gas
/// parameters.
pub fn required_balance(&self) -> eth::Ether {
self.limit * self.fee_per_gas()
}

/// Compute an upper bound for `max_fee_per_gas` for the given settlement.
fn fee_per_gas(&self) -> eth::FeePerGas {
// We multiply a fixed factor of the current base fee per
// gas, which is chosen to be the maximum possible increase to the base
// fee per gas over 12 blocks, also including the "tip".
//
// This is computed as an approximation of:
// MAX_FEE_FACTOR = MAX_INCREASE_PER_BLOCK ** (DEADLINE_IN_BLOCKS +
// SOLVING_TIME) = 1.125 ** (10 + 2) = 1.125 ** 12
//
// The value of `MAX_GAS_INCREASE_PER_BLOCK` comes from EIP-1559, which
// dictates that the block base fee can increase by a maximum of 12.5%
// from one block to another.
const MAX_FEE_FACTOR: f64 = 4.2;
eth::U256::from_f64_lossy(
eth::U256::to_f64_lossy(self.price.base.into()) * MAX_FEE_FACTOR
+ eth::U256::to_f64_lossy(self.price.tip.into()),
)
.into()
self.limit * self.price.max()
}
}
31 changes: 28 additions & 3 deletions crates/driver/src/domain/eth/gas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,13 @@ impl Zero for Gas {
#[derive(Debug, Clone, Copy)]
pub struct GasPrice {
/// The maximum total fee that should be charged.
pub max: FeePerGas,
max: FeePerGas,
/// The maximum priority fee (i.e. the tip to the block proposer) that
/// can be charged.
pub tip: FeePerGas,
tip: FeePerGas,
/// The current base gas price that will be charged to all accounts on the
/// next block.
pub base: FeePerGas,
base: FeePerGas,
}

impl GasPrice {
Expand All @@ -70,6 +70,31 @@ impl GasPrice {
.min(U256::from(self.base).saturating_add(self.tip.into()))
.into()
}

pub fn max(&self) -> FeePerGas {
self.max
}

pub fn tip(&self) -> FeePerGas {
self.tip
}

/// Creates a new instance limiting maxFeePerGas to a reasonable multiple of
/// the current base fee.
pub fn new(max: FeePerGas, tip: FeePerGas, base: FeePerGas) -> Self {
// We multiply a fixed factor of the current base fee per
// gas, which is chosen to be the maximum possible increase to the base
// fee (max 12.5% per block) over 12 blocks, also including the "tip".
const MAX_FEE_FACTOR: f64 = 4.2;
Self {
max: FeePerGas(std::cmp::min(
max.0,
base.mul_ceil(MAX_FEE_FACTOR).add(tip).0,
)),
tip,
base,
}
}
}

/// Implements multiplication of a gas price by a floating point number.
Expand Down
10 changes: 5 additions & 5 deletions crates/driver/src/infra/blockchain/gas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,11 @@ impl GasPriceEstimator {
}
None => estimate,
};
eth::GasPrice {
max: self.max_fee_per_gas.into(),
tip: eth::U256::from_f64_lossy(estimate.max_priority_fee_per_gas).into(),
base: eth::U256::from_f64_lossy(estimate.base_fee_per_gas).into(),
}
eth::GasPrice::new(
self.max_fee_per_gas.into(),
eth::U256::from_f64_lossy(estimate.max_priority_fee_per_gas).into(),
eth::U256::from_f64_lossy(estimate.base_fee_per_gas).into(),
)
})
.map_err(Error::GasPrice)
}
Expand Down
4 changes: 2 additions & 2 deletions crates/driver/src/infra/mempool/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ impl Inner {
.from(solver.account().clone())
.to(tx.to.into())
.gas_price(ethcontract::GasPrice::Eip1559 {
max_fee_per_gas: gas.price.max.into(),
max_priority_fee_per_gas: gas.price.tip.into(),
max_fee_per_gas: gas.price.max().into(),
max_priority_fee_per_gas: gas.price.tip().into(),
})
.data(tx.input.into())
.value(tx.value.0)
Expand Down
7 changes: 5 additions & 2 deletions crates/driver/src/tests/cases/multiple_drivers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,13 @@ async fn separate_deadline() {
.pool(ab_pool())
.order(ab_order())
.solution(ab_solution())
.solver(test_solver().name("second").solving_time_share(0.5))
.solvers(vec![
test_solver().name("first"),
test_solver().name("second").solving_time_share(0.5),
])
.done()
.await;

test.solve().await.ok();
test.solve_with_solver("first").await.ok();
test.solve_with_solver("second").await.ok();
}
39 changes: 32 additions & 7 deletions crates/driver/src/tests/cases/solver_balance.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,46 @@
use crate::tests::{
setup,
setup::{ab_order, ab_pool, ab_solution},
use crate::{
domain::eth,
tests::{
setup,
setup::{ab_order, ab_pool, ab_solution, test_solver},
},
};

/// Test that the `/solve` request errors when solver balance is too low.
#[tokio::test]
#[ignore]
async fn test() {
async fn test_unfunded() {
let test = setup()
.pool(ab_pool())
.order(ab_order())
.solution(ab_solution())
.defund_solvers()
.solvers(vec![test_solver()
.name("unfunded")
.balance(eth::U256::zero())])
.done()
.await;

let solve = test.solve().await;

let solve = test.solve_with_solver("unfunded").await;
solve.ok().empty();
}

/// Test that the `/solve` request succeeds when the solver has just enough
/// funds
#[tokio::test]
#[ignore]
async fn test_just_enough_funded() {
let test = setup()
.pool(ab_pool())
.order(ab_order())
.solution(ab_solution())
.solvers(vec![test_solver()
.name("barely_funded")
// The solution uses ~500k gas units
// With gas costs <20gwei, 0.01 ETH should suffice
.balance(eth::U256::exp10(16))])
.done()
.await;

test.solve_with_solver("barely_funded").await.ok();
test.settle_with_solver("barely_funded").await.ok().await;
}
26 changes: 12 additions & 14 deletions crates/driver/src/tests/setup/blockchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,20 +282,18 @@ impl Blockchain {
)
.await
.unwrap();
if config.funded {
wait_for(
&web3,
web3.eth()
.send_transaction(web3::types::TransactionRequest {
from: primary_address(&web3).await,
to: Some(config.address()),
value: Some(balance / 5),
..Default::default()
}),
)
.await
.unwrap();
}
wait_for(
&web3,
web3.eth()
.send_transaction(web3::types::TransactionRequest {
from: primary_address(&web3).await,
to: Some(config.address()),
value: Some(config.balance),
..Default::default()
}),
)
.await
.unwrap();
}

let domain_separator =
Expand Down
29 changes: 14 additions & 15 deletions crates/driver/src/tests/setup/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,8 @@ impl Default for Order {
pub struct Solver {
/// A human readable identifier of the solver
name: String,
/// Should the solver be funded with ETH? True by default.
funded: bool,
/// How much ETH balance should the solver be funded with? 1 ETH by default.
balance: eth::U256,
/// The private key for this solver.
private_key: ethcontract::PrivateKey,
/// The slippage for this solver.
Expand All @@ -247,7 +247,7 @@ pub struct Solver {
pub fn test_solver() -> Solver {
Solver {
name: solver::NAME.to_owned(),
funded: true,
balance: eth::U256::exp10(18),
private_key: ethcontract::PrivateKey::from_slice(
hex::decode("a131a35fb8f614b31611f4fe68b6fc538b0febd2f75cd68e1282d8fd45b63326")
.unwrap(),
Expand Down Expand Up @@ -285,6 +285,10 @@ impl Solver {
..self
}
}

pub fn balance(self, balance: eth::U256) -> Self {
Self { balance, ..self }
}
}

#[derive(Debug, Clone, PartialEq)]
Expand Down Expand Up @@ -549,16 +553,8 @@ impl Setup {
self
}

pub fn solver(mut self, solver: Solver) -> Self {
self.solvers.push(solver);
self
}

/// Don't fund the solver account with any ETH.
pub fn defund_solvers(mut self) -> Self {
self.solvers
.iter_mut()
.for_each(|solver| solver.funded = false);
pub fn solvers(mut self, solvers: Vec<Solver>) -> Self {
self.solvers = solvers;
self
}

Expand Down Expand Up @@ -761,6 +757,10 @@ impl Test {

/// Call the /settle endpoint.
pub async fn settle(&self) -> Settle {
self.settle_with_solver(solver::NAME).await
}

pub async fn settle_with_solver(&self, solver_name: &str) -> Settle {
let old_balances = self.balances().await;
let old_block = self
.blockchain
Expand All @@ -774,8 +774,7 @@ impl Test {
.client
.post(format!(
"http://{}/{}/settle",
self.driver.addr,
solver::NAME
self.driver.addr, solver_name
))
.json(&driver::settle_req())
.send()
Expand Down

0 comments on commit e6e1495

Please sign in to comment.