-
Notifications
You must be signed in to change notification settings - Fork 183
Check banned users on order arrival #4696
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| //! Fan-out of orders as they arrive in the orderbook. | ||
| //! | ||
| //! Producers (currently the Postgres order listener) publish orders as soon as | ||
| //! they show up, consumers subscribe independently so work that would | ||
| //! otherwise happen on the auction cut's critical path (warming the banned | ||
| //! users cache, ...) can start early. | ||
|
|
||
| use {crate::domain::OrderUid, tokio::sync::broadcast, tokio_stream::wrappers::BroadcastStream}; | ||
|
|
||
| /// How many arrivals a consumer may fall behind before it starts missing | ||
| /// orders. Consumers are best effort, so lagging is not fatal. In mainnet we're | ||
| /// (at the time of writing) running around ~1 order/second, so 16 should be | ||
| /// plenty space for new orders coming in. | ||
| const CAPACITY: usize = 16; | ||
|
|
||
| /// Publishing end of the order arrival fan-out. | ||
| #[derive(Clone)] | ||
| pub struct Notifier(broadcast::Sender<OrderUid>); | ||
|
|
||
| impl Notifier { | ||
| pub fn new() -> Self { | ||
| Self(broadcast::Sender::new(CAPACITY)) | ||
| } | ||
|
|
||
| /// Publishes a newly arrived order. Arrivals published while nobody is | ||
| /// subscribed are dropped. | ||
| pub fn publish(&self, order: OrderUid) { | ||
| if let Err(_) = self.0.send(order) { | ||
| tracing::error!("failed to send order uid to subscribers"); | ||
| } | ||
| } | ||
|
|
||
| /// Stream of the orders arriving from now on, ending once all publishers | ||
| /// are gone. | ||
| /// | ||
| /// A consumer that can't keep up is told how many orders it missed | ||
| /// ([`BroadcastStreamRecvError::Lagged`]) instead of slowing down the | ||
| /// publisher or its fellow consumers. | ||
| pub fn subscribe(&self) -> BroadcastStream<OrderUid> { | ||
| BroadcastStream::new(self.0.subscribe()) | ||
| } | ||
| } | ||
|
|
||
| impl Default for Notifier { | ||
| fn default() -> Self { | ||
| Self::new() | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use { | ||
| super::*, | ||
| tokio_stream::{StreamExt, wrappers::errors::BroadcastStreamRecvError}, | ||
| }; | ||
|
|
||
| fn order(n: usize) -> OrderUid { | ||
| let mut uid = [0u8; 56]; | ||
| uid[..size_of::<usize>()].copy_from_slice(&n.to_le_bytes()); | ||
| OrderUid(uid) | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn every_subscriber_sees_every_arrival() { | ||
| let arrivals = Notifier::new(); | ||
| let (mut first, mut second) = (arrivals.subscribe(), arrivals.subscribe()); | ||
|
|
||
| arrivals.publish(order(1)); | ||
| arrivals.publish(order(2)); | ||
|
|
||
| assert_eq!(first.next().await, Some(Ok(order(1)))); | ||
| assert_eq!(first.next().await, Some(Ok(order(2)))); | ||
| assert_eq!(second.next().await, Some(Ok(order(1)))); | ||
| assert_eq!(second.next().await, Some(Ok(order(2)))); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn lagging_subscriber_is_told_what_it_missed() { | ||
| let arrivals = Notifier::new(); | ||
| let mut subscription = arrivals.subscribe(); | ||
|
|
||
| for n in 0..=CAPACITY { | ||
| arrivals.publish(order(n)); | ||
| } | ||
|
|
||
| // The oldest arrival got dropped instead of stalling the publisher. | ||
| assert_eq!( | ||
| subscription.next().await, | ||
| Some(Err(BroadcastStreamRecvError::Lagged(1))) | ||
| ); | ||
| assert_eq!(subscription.next().await, Some(Ok(order(1)))); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| pub use order_validation::banned::*; | ||
| use { | ||
| crate::domain::order_notify, | ||
| futures::StreamExt, | ||
| std::sync::Arc, | ||
| tokio_stream::wrappers::errors::BroadcastStreamRecvError, | ||
| }; | ||
|
|
||
| /// Spawns a task that warms the cache with the owner of every arriving order | ||
| /// so the auction cut doesn't pay for the remote lookup on its critical path. | ||
| /// | ||
| /// Best effort: the receiver of an order is not part of its UID, so it remains | ||
| /// a cut time lookup. | ||
| pub fn spawn_cache_prewarming(arrivals: &order_notify::Notifier, users: Arc<Users>) { | ||
| let mut arrivals = arrivals.subscribe(); | ||
| tokio::spawn(async move { | ||
| while let Some(arrival) = arrivals.next().await { | ||
| let owner = match arrival { | ||
| Ok(order) => order.owner(), | ||
| Err(BroadcastStreamRecvError::Lagged(skipped)) => { | ||
| tracing::debug!(skipped, "lagged behind new orders, skipping prewarming"); | ||
| continue; | ||
| } | ||
| }; | ||
| // Owners we already know about make up the bulk of the arrivals. | ||
| if users.cached(&owner).is_none() { | ||
|
Contributor
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. At this point there is not much point anymore in doing the cached check, right? |
||
| users.banned([owner]).await; | ||
| } | ||
| } | ||
| tracing::error!("banned users cache prewarming task terminated unexpectedly"); | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,8 @@ | ||
| pub mod api; | ||
| pub mod banned; | ||
| pub mod blockchain; | ||
| pub mod persistence; | ||
| pub mod shadow; | ||
| pub mod solvers; | ||
|
|
||
| pub use { | ||
| blockchain::Ethereum, | ||
| order_validation::banned, | ||
| persistence::Persistence, | ||
| solvers::Driver, | ||
| }; | ||
| pub use {blockchain::Ethereum, persistence::Persistence, solvers::Driver}; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ use { | |
| Unscored, | ||
| winner_selection::{self, Ranking}, | ||
| }, | ||
| order_notify, | ||
| settlement::{ExecutionEnded, ExecutionStarted}, | ||
| }, | ||
| infra::{ | ||
|
|
@@ -129,6 +130,7 @@ impl RunLoop { | |
| trusted_tokens: AutoUpdatingTokenList, | ||
| probes: Probes, | ||
| maintenance: MaintenanceSync, | ||
| order_notifier: order_notify::Notifier, | ||
| ) -> Self { | ||
| let max_winners = config.max_winners_per_auction.get(); | ||
| let weth = eth.contracts().wrapped_native_token(); | ||
|
|
@@ -137,7 +139,7 @@ impl RunLoop { | |
| let wake_notify = Arc::new(tokio::sync::Notify::new()); | ||
|
|
||
| // Spawn background tasks to listen for events | ||
| persistence.spawn_order_listener(wake_notify.clone()); | ||
| persistence.spawn_order_listener(wake_notify.clone(), order_notifier); | ||
|
Contributor
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. Feels like the actual DB notify listener should now be spawned inside the constructor of the new component, no? Also I think all the things that get done when an order gets posted should live in this struct instead of spreading that over many different background tasks created in infra files (e.g. ban list warming, balance cache warming, fast path handling). |
||
| Self::spawn_block_listener(eth.current_block().clone(), wake_notify.clone()); | ||
|
|
||
| Self { | ||
|
|
||
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.
This currently only looks at the order's owner but not the receiver. For the other 2 use cases (balance cache warming, order fast path) we both need to look up the order. So probably best to already fetch the order from the DB in the new
order_notify.