-
Notifications
You must be signed in to change notification settings - Fork 93
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
base: notify-banned-solvers
Are you sure you want to change the base?
Changes from all commits
805946c
7ab2a38
cca115a
983dc41
160e2d9
597d268
7c87087
df4b77f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
||
#[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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,4 +163,26 @@ impl super::Postgres { | |
.await | ||
.context("solver_competition::find_non_settling_solvers") | ||
} | ||
|
||
pub async fn find_low_settling_solvers( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please can you add a comment here too? 🙏 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||
} | ||
} |
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>>, | ||
} | ||
|
||
|
@@ -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, | ||
})); | ||
|
||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing the debug trace |
||
{ | ||
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"); | ||
} | ||
} | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider moving this match to implementation of the Same for the |
||
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] | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?