-
Notifications
You must be signed in to change notification settings - Fork 89
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
Driver fetches order's app-data #3242
base: main
Are you sure you want to change the base?
Conversation
inner: Arc<Mutex<Inner>>, | ||
app_data_retriever: Arc<order::app_data::AppDataRetriever>, |
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.
app_data_retriever
doesn't require the internal lock, so it sits separately to be able to execute app-data and balance fetching tasks in parallel.
// According to statistics, the average size of the app-data is ~800 bytes. With | ||
// this constant, the approximate size of the cache will be ~1.6 MB. | ||
const CACHE_SIZE: u64 = 2_000; |
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.
The value can be increased or become configurable. The total amount of unique app-data atm is ~97k, while there is 4.3M of orders.
} | ||
} | ||
|
||
impl Clone for FetchingError { |
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 is required to satisfy BoxRequestSharing
constraints.
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.
Nice and thoughtful implementation. Only couple of comments.
@@ -50,6 +51,42 @@ pub struct Order { | |||
pub quote: Option<Quote>, | |||
} | |||
|
|||
/// The app data associated with an order. |
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.
Now that order/app_data.rs
is a thing it seems more fitting to put all these appdata related types there.
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.
Which types exactly? The whole AppDataRetriever
? I ended up creating a separate file for the AppDataRetriever
and keeping mod.rs
to contain domain structs only because the latter is already quite big and hard to read.
// Filter out orders that failed to fetch app data. | ||
prioritized_orders.retain_mut(|order| { |
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.
Filtering orders without the full appdata right away seems a bit dangerous. At the moment we don't need the parsed data yet so keeping orders where we weren't able to fetch the full appdata would not be a regression to the status quo.
Also I'm still a bit worried how much time these requests will take even if there aren't so many unique appdata hashes. To avoid bad surprises I'd like to see this feature gated.
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.
Filtering orders without the full appdata right away seems a bit dangerous.
It filters out orders that only had flashloan data in the order, but for some reason, we didn't receive the full data from the API. I think orders shouldn't be included because they will most likely fail to execute without flashloan hints sent to solvers, right?
To avoid bad surprises I'd like to see this feature gated.
Makes sense. Will add metrics and a config.
/// updated orders. | ||
pub async fn process(&self, auction: Auction, solver: ð::H160) -> Auction { | ||
let (mut app_data_by_order, mut prioritized_orders) = join( | ||
self.collect_orders_app_data(&auction), |
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.
The majority of solvers are connected to a single driver. AuctionProcessor::process()
gets called for all of them. Under the hood prioritize_orders()
will only ever run once per auction since it turned out that this overhead is too much especially given that all this would be duplicated work.
I think collecting order app data should also be implemented such that it only runs once per auction. Even if it's already using request sharing logic internally I think it would be good to extend the AuctionProcessor
code such that one would naturally continue using the mutexed processing introduced in prioritize_orders
.
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.
it would be good to extend the AuctionProcessor code such that one would naturally continue using the mutexed processing introduced in prioritize_orders.
Using the same mutex means we won't be able to execute both balance filtering and app data collecting in parallel, right? I wanted to avoid that. Nvm, probably, your suggestion is still a better option.
@MartinquaXD what is the most important reason not to make app-data part of the auction (I don't fully understand the argument ☝️)? AFAIS the current solution also has a number of cons:
|
Description
As part of the flashloans support, the driver needs be able to retrieve the order's full app data in order to send flashloans hints to solvers(#3216). This PR introduces an app-data retriever that works as follows:
BoxRequestSharing
, which helps to avoid request duplication.Order
struct, responses with404 Not Found
are also cached asNone
. There is a small amount of orders with empty full app data in the DB, but they still exist.Implementation details and considerations
app_data
among all the orders, so there is no need for a TTL cache since the cache is expected to be hit for the majority of orders.2000
capacity, approximately equal to ~1.5MB of memory.domain::Order
struct. To avoid creating new order structs, theAppData
is converted into an enum, which gets updated accordingly.Rate limiting
The following is only valid for colocated solvers since the API is not rate-limited for the services within the same k8s cluster.
SQL query
This image represents the following data: The left column shows how many unique app data entries have a single auction, whereas the right shows how many auctions have the corresponding amount of unique app data.
The orderbook API RPS is 5. Only 0.05% of auctions have more than 5 unique app data entries. That means there is at most a 0.05% chance that a driver hits the RPS for a single auction, where the actual probability is even lower since some of the hashes will more likely be already cached. That is why the current implementation doesn't contain a rate-limiting mechanism.
Changes
orderbook-url
.How to test
All the current tests pass. New driver tests can be added only once the driver starts sending the collected data to solvers(#3216).
Related Issues
Fixes #3215