Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ban solvers based on the settlements success rate #3263

Open
wants to merge 8 commits into
base: notify-banned-solvers
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 50 additions & 9 deletions crates/autopilot/src/arguments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,22 +253,63 @@ pub struct Arguments {

#[derive(Debug, clap::Parser)]
pub struct DbBasedSolverParticipationGuardConfig {
/// Enables or disables the solver participation guard
/// The time-to-live for the solver participation blacklist cache.
#[clap(long, env, default_value = "5m", value_parser = humantime::parse_duration)]
pub solver_blacklist_cache_ttl: Duration,

#[clap(flatten)]
pub non_settling_validator_config: NonSettlingValidatorConfig,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please could you add comments?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does the term "validator" come into play here? Aren't these solvers?


#[clap(flatten)]
pub low_settling_validator_config: LowSettlingValidatorConfig,
}

#[derive(Debug, clap::Parser)]
pub struct NonSettlingValidatorConfig {
/// Enables search of non-settling solvers.
#[clap(
id = "db_enabled",
long = "db-based-solver-participation-guard-enabled",
env = "DB_BASED_SOLVER_PARTICIPATION_GUARD_ENABLED",
id = "non_settling_solvers_blacklisting_enabled",
long = "non-settling-solvers-blacklisting-enabled",
env = "NON_SETTLING_SOLVERS_BLACKLISTING_ENABLED",
default_value = "true"
)]
pub enabled: bool,

/// The time-to-live for the solver participation blacklist cache.
#[clap(long, env, default_value = "5m", value_parser = humantime::parse_duration)]
pub solver_blacklist_cache_ttl: Duration,
/// The number of last auctions to check solver participation eligibility.
#[clap(
id = "non_settling_last_auctions_participation_count",
long = "non-settling-last-auctions-participation-count",
env = "NON_SETTLING_LAST_AUCTIONS_PARTICIPATION_COUNT",
default_value = "3"
)]
pub last_auctions_participation_count: u32,
}

#[derive(Debug, clap::Parser)]
pub struct LowSettlingValidatorConfig {
/// Enables search of non-settling solvers.
#[clap(
id = "low_settling_solvers_blacklisting_enabled",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is id used for?

long = "low-settling-solvers-blacklisting-enabled",
env = "LOW_SETTLING_SOLVERS_BLACKLISTING_ENABLED",
default_value = "true"
)]
pub enabled: bool,

/// The number of last auctions to check solver participation eligibility.
#[clap(long, env, default_value = "3")]
pub solver_last_auctions_participation_count: u32,
#[clap(
id = "low_settling_last_auctions_participation_count",
long = "low-settling-last-auctions-participation-count",
env = "LOW_SETTLING_LAST_AUCTIONS_PARTICIPATION_COUNT",
default_value = "100"
)]
pub last_auctions_participation_count: u32,

/// A minimum success rate for a solver to be considered eligible for
/// participation in the competition. Otherwise, the solver will be
/// banned.
#[clap(long, env, default_value = "0.1")]
pub solver_min_settlement_success_rate: f64,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a solver has not won a single auction in the last n blocks? Then technically their success rate is 0 so they should be blocked. Does it make more sense to express this config in terms of a maximum failure rate instead?

}

impl std::fmt::Display for Arguments {
Expand Down
22 changes: 22 additions & 0 deletions crates/autopilot/src/database/competition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,4 +163,26 @@ impl super::Postgres {
.await
.context("solver_competition::find_non_settling_solvers")
}

pub async fn find_low_settling_solvers(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you add a comment here too? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DB queries tend to not have any comments. A comment on the caller side was initially added.

&self,
last_auctions_count: u32,
current_block: u64,
min_success_ratio: f64,
) -> anyhow::Result<Vec<Address>> {
let mut ex = self.pool.acquire().await.context("acquire")?;
let _timer = super::Metrics::get()
.database_queries
.with_label_values(&["find_low_settling_solvers"])
.start_timer();

database::solver_competition::find_low_settling_solvers(
&mut ex,
last_auctions_count,
current_block,
min_success_ratio,
)
.await
.context("solver_competition::find_low_settling_solvers")
}
}
200 changes: 136 additions & 64 deletions crates/autopilot/src/domain/competition/participation_guard/db.rs
Original file line number Diff line number Diff line change
@@ -1,28 +1,36 @@
use {
crate::{
arguments::{
DbBasedSolverParticipationGuardConfig,
LowSettlingValidatorConfig,
NonSettlingValidatorConfig,
},
database::Postgres,
domain::{eth, Metrics},
infra,
infra::{self, solvers::dto},
},
ethrpc::block_stream::CurrentBlockWatcher,
futures::future::join_all,
std::{
collections::HashMap,
collections::{HashMap, HashSet},
sync::Arc,
time::{Duration, Instant},
},
tokio::join,
};

/// Checks the DB by searching for solvers that won N last consecutive auctions
/// but never settled any of them.
/// and either never settled any of them or their settlement success rate is
/// lower than `min_settlement_success_rate`.
#[derive(Clone)]
pub(super) struct Validator(Arc<Inner>);

struct Inner {
db: Postgres,
banned_solvers: dashmap::DashMap<eth::Address, Instant>,
ttl: Duration,
last_auctions_count: u32,
non_settling_config: NonSettlingValidatorConfig,
low_settling_config: LowSettlingValidatorConfig,
drivers_by_address: HashMap<eth::Address, Arc<infra::Driver>>,
}

Expand All @@ -31,15 +39,15 @@ impl Validator {
db: Postgres,
current_block: CurrentBlockWatcher,
settlement_updates_receiver: tokio::sync::mpsc::UnboundedReceiver<()>,
ttl: Duration,
last_auctions_count: u32,
db_based_validator_config: DbBasedSolverParticipationGuardConfig,
drivers_by_address: HashMap<eth::Address, Arc<infra::Driver>>,
) -> Self {
let self_ = Self(Arc::new(Inner {
db,
banned_solvers: Default::default(),
ttl,
last_auctions_count,
ttl: db_based_validator_config.solver_blacklist_cache_ttl,
non_settling_config: db_based_validator_config.non_settling_validator_config,
low_settling_config: db_based_validator_config.low_settling_validator_config,
drivers_by_address,
}));

Expand All @@ -59,71 +67,89 @@ impl Validator {
tokio::spawn(async move {
while settlement_updates_receiver.recv().await.is_some() {
let current_block = current_block.borrow().number;
match self_
.0
.db
.find_non_settling_solvers(self_.0.last_auctions_count, current_block)
.await
{
Ok(non_settling_solvers) => {
let non_settling_drivers = non_settling_solvers
.into_iter()
.filter_map(|solver| {
let address = eth::Address(solver.0.into());
if let Some(driver) = self_.0.drivers_by_address.get(&address) {
Metrics::get()
.non_settling_solver
.with_label_values(&[&driver.name]);
Some(driver.clone())
} else {
None
}
})
.collect::<Vec<_>>();

let non_settling_solver_names = non_settling_drivers
.iter()
.map(|driver| driver.name.clone())
.collect::<Vec<_>>();

tracing::debug!(solvers = ?non_settling_solver_names, "found non-settling solvers");

let non_settling_drivers = non_settling_drivers
.into_iter()
// Check if solver accepted this feature. This should be removed once a CIP is
// approved.
.filter(|driver| driver.accepts_unsettled_blocking)
.collect::<Vec<_>>();

Self::notify_solvers(&non_settling_drivers);

let now = Instant::now();
for driver in non_settling_drivers {
self_
.0
.banned_solvers
.insert(driver.submission_address, now);
}
}
Err(err) => {
tracing::warn!(?err, "error while searching for non-settling solvers")
}
}

let (non_settling_solvers, mut low_settling_solvers) = join!(
self_.find_non_settling_solvers(current_block),
self_.find_low_settling_solvers(current_block)
);
// Non-settling issue has a higher priority, remove duplicates from low-settling
// solvers.
low_settling_solvers.retain(|solver| !non_settling_solvers.contains(solver));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This .contains() executes a O(n)search for each element of low_settling_solvers. Consider returning a HashSet from find_non_settling_solvers() and find_low_settling_solvers()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both functions already return HashSets. Did I miss anything?


self_.post_process(
&non_settling_solvers,
&dto::notify::Request::UnsettledConsecutiveAuctions,
);
self_.post_process(
&low_settling_solvers,
&dto::notify::Request::LowSettlingRate,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: HighSettleFailureRate might be more intuitive

);
}
});
}

async fn find_non_settling_solvers(&self, current_block: u64) -> HashSet<eth::Address> {
if !self.0.non_settling_config.enabled {
return Default::default();
}

match self
.0
.db
.find_non_settling_solvers(
self.0.non_settling_config.last_auctions_participation_count,
current_block,
)
.await
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing the debug trace tracing::debug!(solvers = ?non_settling_solver_names, "found non-settling solvers");

{
Ok(solvers) => solvers
.into_iter()
.map(|solver| eth::Address(solver.0.into()))
.collect(),
Err(err) => {
tracing::warn!(?err, "error while searching for non-settling solvers");
Default::default()
}
}
}

async fn find_low_settling_solvers(&self, current_block: u64) -> HashSet<eth::Address> {
if !self.0.low_settling_config.enabled {
return Default::default();
}

match self
.0
.db
.find_low_settling_solvers(
self.0.low_settling_config.last_auctions_participation_count,
current_block,
self.0
.low_settling_config
.solver_min_settlement_success_rate,
)
.await
{
Ok(solvers) => solvers
.into_iter()
.map(|solver| eth::Address(solver.0.into()))
.collect(),
Err(err) => {
tracing::warn!(?err, "error while searching for low-settling solvers");
Default::default()
}
}
}

/// Try to notify all the non-settling solvers in a background task.
fn notify_solvers(non_settling_drivers: &[Arc<infra::Driver>]) {
let futures = non_settling_drivers
fn notify_solvers(drivers: &[Arc<infra::Driver>], request: &dto::notify::Request) {
let futures = drivers
.iter()
.cloned()
.map(|driver| {
let request = request.clone();
async move {
if let Err(err) = driver
.notify(&infra::solvers::dto::notify::Request::UnsettledConsecutiveAuctions)
.await
{
if let Err(err) = driver.notify(&request).await {
tracing::debug!(solver = ?driver.name, ?err, "unable to notify external solver");
}
}
Expand All @@ -134,6 +160,52 @@ impl Validator {
join_all(futures).await;
});
}

/// Updates the cache and notifies the solvers.
fn post_process(&self, solvers: &HashSet<eth::Address>, request: &dto::notify::Request) {
if solvers.is_empty() {
return;
}

let drivers = solvers
.iter()
.filter_map(|solver| self.0.drivers_by_address.get(solver).cloned())
.collect::<Vec<_>>();

let log_message = match request {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving this match to implementation of the dto::notify::Request as a new method (ex.: log_message()), this will get the post_process() function cleaner.

Same for the reason match below.

dto::notify::Request::UnsettledConsecutiveAuctions => "found non-settling solvers",
dto::notify::Request::LowSettlingRate => "found low-settling solvers",
};
let solver_names = drivers
.iter()
.map(|driver| driver.name.clone())
.collect::<Vec<_>>();
tracing::debug!(solvers = ?solver_names, log_message);

let reason = match request {
dto::notify::Request::UnsettledConsecutiveAuctions => "non_settling",
dto::notify::Request::LowSettlingRate => "low_settling",
};

for solver in solver_names {
Metrics::get()
.banned_solver
.with_label_values(&[&solver, reason]);
}

let non_settling_drivers = drivers
.into_iter()
// Notify and block only solvers that accept unsettled blocking feature. This should be removed once a CIP is approved.
.filter(|driver| driver.accepts_unsettled_blocking)
.collect::<Vec<_>>();

Self::notify_solvers(&non_settling_drivers, request);

let now = Instant::now();
for driver in non_settling_drivers {
self.0.banned_solvers.insert(driver.submission_address, now);
}
}
}

#[async_trait::async_trait]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,15 @@ impl SolverParticipationGuard {
) -> Self {
let mut validators: Vec<Box<dyn Validator + Send + Sync>> = Vec::new();

if db_based_validator_config.enabled {
let current_block = eth.current_block().clone();
let database_solver_participation_validator = db::Validator::new(
db,
current_block,
settlement_updates_receiver,
db_based_validator_config.solver_blacklist_cache_ttl,
db_based_validator_config.solver_last_auctions_participation_count,
drivers_by_address,
);
validators.push(Box::new(database_solver_participation_validator));
}
let current_block = eth.current_block().clone();
let database_solver_participation_validator = db::Validator::new(
db,
current_block,
settlement_updates_receiver,
db_based_validator_config,
drivers_by_address,
);
validators.push(Box::new(database_solver_participation_validator));

let onchain_solver_participation_validator = onchain::Validator { eth };
validators.push(Box::new(onchain_solver_participation_validator));
Expand Down
4 changes: 2 additions & 2 deletions crates/autopilot/src/domain/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ pub use {
pub struct Metrics {
/// How many times the solver marked as non-settling based on the database
/// statistics.
#[metric(labels("solver"))]
pub non_settling_solver: prometheus::IntCounterVec,
#[metric(labels("solver", "reason"))]
pub banned_solver: prometheus::IntCounterVec,
}

impl Metrics {
Expand Down
Loading
Loading