Check banned users on order arrival - #4696
Conversation
|
Did this also have a measurable latency reduction when building auctions? Fetching banned users happens concurrently with other things so I might have 0 net impact if the other things we do concurrently always take longer than the ban check. |
|
|
||
| // Spawn background tasks to listen for events | ||
| persistence.spawn_order_listener(wake_notify.clone()); | ||
| persistence.spawn_order_listener(wake_notify.clone(), banned_users); |
There was a problem hiding this comment.
Feels like the concept of handling order as they get put into the orderbook will become more relevant and should probably get its own dedicated domain level component.
That could then be used for:
- pre-fetch ban status
- pre-fetch balances
- initiate fast path handling for the fast path feature currently in the works
| // Best effort: the receiver (distinct from the | ||
| // owner in ~3% of orders) is not derivable from | ||
| // the payload and remains a cut-time lookup |
There was a problem hiding this comment.
Seems like a classical AI comment where context from the implementation thread gets added to the code base as if it makes sense universally.
In this case the code is not even looking at the order owner so this comment makes 0 sense.
There was a problem hiding this comment.
🤦 I remember vetting this comment, then refactored and it got LLM'd
| let (sender, mut receiver) = mpsc::channel(QUEUE_SIZE); | ||
| tokio::spawn(async move { | ||
| while let Some(address) = receiver.recv().await { | ||
| let mut batch = HashSet::from([address]); |
There was a problem hiding this comment.
batching requests feels like unnecessary complexity at the moment. Did you evaluate how frequently new orders get placed?
Additionally I think the number of NEW owners/receivers is a lot lower still. If you expose a sync API on the banned users cache that only responds with cached data you can probably discard the majority of updates outright so that batching is really not needed.
There was a problem hiding this comment.
I added the batching to avoid launching multiple tasks per order, like when we get spammed and get a bunch of orders followed by a bunch of cancellations
The alternative actually involved adding more code to handle a single order instead of multiple ones, so code-wise this is slightly simpler
I can put the filter up though, before it even gets sent to the batch
There was a problem hiding this comment.
I added the batching to avoid launching multiple tasks per order
You can still have the 1 dedicated background task without batching, though. AFAICS none of the downstream components actually have any significant batch optimizations (hermod just has a single address API, the RPC calls automatically get batched, and the caches are using moka which is optimized for concurrent accesses).
Are there any significant downsides of just channel.for_each(banned_users.banned(user)) or something simple like that?
There was a problem hiding this comment.
Ah I see what you're saying, yes, we can keep a single task reading off the queue, the other points about the RPCs are valid too
| let deadline = tokio::time::sleep(BATCH_DELAY); | ||
| tokio::pin!(deadline); | ||
| loop { | ||
| tokio::select! { | ||
| () = &mut deadline => break, | ||
| next = receiver.recv() => { | ||
| let Some(address) = next else { break }; | ||
| batch.insert(address); | ||
| if batch.len() >= BATCH_SIZE { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Nice, didn't know about this one
This is part of the "update solvable orders" phase, sadly, it didn't have a measurable impact |
Keep in mind that real world improvements should be the measurement of success - especially for PRs introducing non-trivial changes. In this case I believe there is an argument to be made to add this component now because we already have a few additions to that in mind but without those additions I don't think this PR should be merged. |
0741b43 to
8619593
Compare
| } | ||
| }; | ||
| // Owners we already know about make up the bulk of the arrivals. | ||
| if users.cached(&owner).is_none() { |
There was a problem hiding this comment.
At this point there is not much point anymore in doing the cached check, right?
The regular banned() function already first looks in the cache.
| tokio::spawn(async move { | ||
| while let Some(arrival) = arrivals.next().await { | ||
| let owner = match arrival { | ||
| Ok(order) => order.owner(), |
There was a problem hiding this comment.
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.
|
|
||
| // Spawn background tasks to listen for events | ||
| persistence.spawn_order_listener(wake_notify.clone(), banned_users); | ||
| persistence.spawn_order_listener(wake_notify.clone(), order_notifier); |
There was a problem hiding this comment.
Feels like the actual DB notify listener should now be spawned inside the constructor of the new component, no?
What I'd imagine is sth like:
pub struct NewOrderListener;
impl NewOrderListener {
pub fn new() -> Self {
// spawn DB NOTIFY listener and wire it up
}
}
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).
2 participants
Description
We're doing all the user ban checks when performing auction cutting, however, we can check them eagerly when the autopilot gets notified of new orders; this works because we can extract the owner from the order UID. As such we can check on order arrival leading to the cache being pre-warmed when the auction cutting happens.
Changes
How to test
Tested in staging to ensure nothing broke and in prod:
The overall latency was reduced as one can see based on less bucket variance, there are still some spikes as receivers are still searched later on, since the order UID doesn't carry info on them.
Heatmap panel: