Separate autopilot API native price estimator #4044
Conversation
| /// Creates a new CachingNativePriceEstimator with a background maintenance | ||
| /// task. | ||
| /// | ||
| /// The maintenance task periodically refreshes cached prices before they | ||
| /// expire. Use this for the primary estimator in a shared-cache setup. |
There was a problem hiding this comment.
Shouldn't cache maintenance be coupled with the cache itself?
In my head it makes more sense to create a single cache, comes with maintenance by default and when you want more structs to have access to it, hand out references (which you do with the clone on the struct with the Arc)
I'm a bit confused by the separation
There was a problem hiding this comment.
I don't clearly see your idea. Which estimators will the cache maintenance task will be using then?
| /// Main estimator, which runs the background cache maintenance task. | ||
| pub main: Arc<CachingNativePriceEstimator>, | ||
| /// API estimator that shares the cache with the main estimator but doesn't | ||
| /// run background task and might use a different set of sources. |
There was a problem hiding this comment.
How come it can use a different set of sources but not run the background task to update entries according to them?
What am I missing?
There was a problem hiding this comment.
The idea of the API estimators is that a subset of the main estimators. But after thinking more about it, we can probably run a maintenance task with estimators combined from api and main estimators to ensure everything is covered.
There was a problem hiding this comment.
I reworked the approach. It would require starting the review from scratch. Sorry about that.
| /// The main estimator includes all sources and runs the background | ||
| /// maintenance task. The API estimator (if `api_native` is provided) | ||
| /// uses its own set of sources but shares the cache, so it can return | ||
| /// cached prices without triggering requests to excluded sources. |
There was a problem hiding this comment.
The different set of sources is either poorly documented and needs a better explanation or it feels like a footgun / something that may cause a lot of confusion during investigations
There was a problem hiding this comment.
What exactly confuses you? Different sets of estimators are used for the orderbook and auction competition.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant architectural improvement by separating the native price estimator for the Autopilot API and introducing a shared cache with source-aware maintenance. This refactoring effectively reduces memory usage and prevents redundant price fetches. The new NativePriceCache encapsulates caching logic cleanly, simplifying the CachingNativePriceEstimator and improving the overall design. The changes are well-structured and align with the goals outlined in the description. I've found one potential issue that could lead to a panic, which I've detailed in a specific comment.
| /// Creates a new CachingNativePriceEstimator. | ||
| /// | ||
| /// The estimator will use the provided cache for lookups and will fetch | ||
| /// prices on-demand for cache misses. Background maintenance (keeping the |
There was a problem hiding this comment.
Q: should we keep metrics on cache hits / misses?
There was a problem hiding this comment.
Wdym? I think I didn't touch metrics.
There was a problem hiding this comment.
By "keep" I meant like "keep an eye", but I wasn't clear, rephrasing it: should we start tracking cache hits/misses?
There was a problem hiding this comment.
Probably makes sense to build some metrics in a separate PR. Instead of hits/misses, I personally would be interested in cached values that are never accessed later.
| updated_at, | ||
| now, | ||
| Default::default(), | ||
| EstimatorSource::default(), |
There was a problem hiding this comment.
I found it a bit hard to reason about what primary and secondary is supposed to be. Given that this logic is already extremely specific it would probably make sense to just give them specific names?
There was a problem hiding this comment.
Yeah, I was also thinking about that. Which exactly names, for example? They are located in the shared crate, which doesn't know anything about autopilot or orderbook, I assume.
There was a problem hiding this comment.
After suggesting the primary/secondary names, Ilya better explained what's happening with them. I don't have a great idea for "these prices are for queries from A and these prices are for queries from B"
But if this is service just two services, I'd say it makes sense to name them "service_X_source" and "service_Y_source" or something similar
I'd also note that they could stop being in the shared crate if they're just use by those two services 🧹 😅
| /// originally fetched it. The cache's background maintenance task uses | ||
| /// this information to dispatch updates to the appropriate estimator, | ||
| /// ensuring each token is refreshed using the same source that | ||
| /// originally fetched it. |
There was a problem hiding this comment.
This seems to relate to the design considerations you mention in the description:
The source-tracking approach addresses a subtle issue with shared cache maintenance. With two estimators (primary using CoinGecko, secondary without), a naive approach of picking one estimator for all maintenance would fail:
If primary maintains everything: tokens initially fetched via secondary would start hitting CoinGecko during maintenance, defeating the purpose
If secondary maintains everything: tokens originally fetched via CoinGecko couldn't be properly refreshed
By tagging each cached entry with its source and dispatching maintenance accordingly, tokens stay with their original estimator throughout their cache lifetime.
However, I'm don't think this is thought through completely. With the exception for onchain placed orders (😬) all new native prices have to be fetched by an API request originally. Unless I'm missing something once a user places an order for a completely new token the autopilot will continue to update the cache without coingecko (since that is not part of the estimator that originally fetched the price). I suspect this token could only ever be upgraded to be fetched by coingecko after a restart puts it into the cache and marks it as a primary token.
What I think would be closer to what we try to achieve is that we only keep tokens warm that are actually used in the auction. So what I would imagine is this:
- API request for a completely new native price
- autopilot caches it but it's not marked for maintenance yet
- a. user never places an order => maintenance never refetches the token, eventually gets evicted from the cache
b. user places an order => when autopilot fetches the price for building the auction it marks it as "worthy of maintenance"
In that approach there would be only the main estimator (with coingecko) running the maintenance and only for the tokens that were explicitly marked by the autopilot. That deviates from what we currently do (where the API is configured to run the maintenance as well) but I think since we now have a single cache that's shared by everyone and has the prices for all tokens in the auction kept up to date the API probably can stop updating it's cache entirely.
And regarding the issue that you brought somewhere else where it can be an issue where the estimator that handled the orderbook native price request is "more powerful" (i.e. would find native prices that the estimator with coingecko is not able to) I'd like to point out that we could perhaps stick to 1 required native price estimator argument and an optional coingecko argument. Only if coingecko is enabled the need for a second estimator would arise and the autopilot could just add coingecko to the regular estimators.
That way the estimator used for maintaining the cache is always at least as capable as the estimator used to handle API requests.
All that being said I agree with @jmg-duarte that this is very complicated but unfortunately I was also not able to come up with a cleaner idea. 😞
There was a problem hiding this comment.
a. user never places an order => maintenance never refetches the token, eventually gets evicted from the cache
b. user places an order => when autopilot fetches the price for building the auction it marks it as "worthy of maintenance"
My worry here is that if something is placed into the cache, it has to be maintained. Otherwise, our orderbook will be sending stale results until the item expires in the cache. That is valid for the cases where no order was placed, but still, this affects the quote competition, I suppose. Do I miss something, or can we ignore this problem for the quote competition?
There was a problem hiding this comment.
Alright, after discussing it, we can neglect the stale native prices for the quote competition problem and run the maintenance task only for the auction-related native prices because we don't really report those native prices in the quote competition and just use them to ensure the token is "tradable".
Since we would have caches in both the orderbook and the autopilot, that can lead to keeping the price in the cache twice as long in the worst case scenario, e.g., orderbook and autopilot have 10m cache TTL, an orderbook restarts and requests a price from the autopilot that is about to expire in its cache and then the orderbook caches it for another 10m in its own cache.
This can be addresses by disabling the cache on the orderbook side since we run both services on the same k8s node and the latency should be minimal. The connection can be improved by keeping it open or switching to websockets.
Will update the PR.
There was a problem hiding this comment.
My worry here is that if something is placed into the cache, it has to be maintained. Otherwise, our orderbook will be sending stale results until the item expires in the cache.
But that's part of the premise/trade-off of the cache, even with maintenance you will be sending "stale" values, unless they're the value right after the fetching. For example: you just fetched a value, there's a price jump, maintenance didn't occur yet, you'll be serving a "stale" value that was just fetched
I think Martin's idea might be easier to implement using a two-tier cache, one that is kept warm by maintenance and another that isn't kept warm, and values are only evicted or upgraded, some "trace examples":
quote estimation -> no value (L1, L2) -> fetch coingecko -> place in L2
order estimation -> no value (L1) -> value in L2 -> upgrade to L1 (remove from L2 + add to L1)
quote estimation -> value in L2 -> return value (no upgrade)
quote estimation -> value in L1 -> return value (no upgrade)
order estimation -> value in L2 -> return value (upgrade in background)
maintenance thread:
- every X run
- loop over entries
- batch request or similar to coingecko
However, this has an issue, if I get an estimate and actually do a trade with a crappy token, when will it get evicted from L1? It should have some bound and support LRU that ignores the maintenance
I know this is not trivial to implement but it's the cleanest solution I can conjure up right now.
There was a problem hiding this comment.
But that's part of the premise/trade-off of the cache, even with maintenance you will be sending "stale" values, unless they're the value right after the fetching. For example: you just fetched a value, there's a price jump, maintenance didn't occur yet, you'll be serving a "stale" value that was just fetched
Staleness of 10m(cache) or 10s(maintenance update interval) has a huge difference, tho 😄
This comment should probably explain why we can disable the maintenance task for the quote competition native prices.
There was a problem hiding this comment.
Staleness of 10m(cache) or 10s(maintenance update interval) has a huge difference, tho 😄
Yeah I know, I forgot to add that the maintenance interval needs to be balanced with the TTL
Anyways, I hadn't seen this #4044 (comment) because I started writing and it took a while to gather my thoughts 😅 all good
crates/e2e/src/setup/proxy.rs
Outdated
| if let Some(current) = backends.pop_front() { | ||
| backends.push_back(current); | ||
| } | ||
| tracing::info!(?backends, "rotated backends"); |
There was a problem hiding this comment.
Not related to this PR, but should be useful when investigating failures of the flaky local_node_dual_autopilot_only_leader_produces_auctions e2e test.
|
Based on #4044 (comment), the approach was reworked. The PR description is also updated. |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-thought-out refactoring of the native price estimation logic to reduce external API calls. By introducing source-aware caching that distinguishes between actively maintained Auction requests and passively cached Quote requests, you've created a more efficient system. The new NativePriceCache and MaintenanceConfig structs improve modularity and make the caching strategy clear. My review identified a minor opportunity for code simplification and a refactoring opportunity to reduce duplication. A potential bug regarding the handling of approximation_tokens was also noted, which should be addressed in a separate pull request. Overall, this is a high-quality contribution.
| create_missing_entry: Option<EstimatorSource>, | ||
| upgrade_to_source: Option<EstimatorSource>, |
There was a problem hiding this comment.
These arguments are bit confusing to me. Wouldn't a single argument like mark_for_maintenance be sufficient? If the entry is missing - create it. If the entry exists but the token is currently not flagged for maintenance - update the flag.
If you drop the information which estimator originally fetched the price and instead simply frame the logic as "does the token need maintenance or not" the PR could probably shrink by a good amount. Unless I'm missing something this would require this PR to only introduce a few core changes:
- spawn 2 different native price estimators (one with and one without coingecko)
- the required config changes (if any - I mean you could use the currently 1 native price estimator setup and only filter out coingecko from the list to create the second estimator)
- have them share a cache
- only spawn a maintenance task for 1 of them
There was a problem hiding this comment.
That reverts us to the initial PR structure, which I find much more confusing. And I assume @jmg-duarte was under the same impression.
The arguments are also confusing. I need to think more about it since they serve different purposes, and we need both.
There was a problem hiding this comment.
Introduced a CacheLookup enum, which replaces these args.
9bcf4dd (this PR)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-designed refactoring of the native price estimation and caching mechanism. By implementing source-aware caching, distinguishing between auction-related requests (actively maintained) and quote/API requests (cached but not maintained), the changes effectively reduce unnecessary API calls, particularly to CoinGecko. The new NativePriceCache and QuoteCompetitionEstimator provide a clear separation of concerns and improve efficiency. The updates to the PriceEstimatorFactory and various test cases demonstrate a thorough implementation of the new architecture. Overall, this is a solid improvement to the system's performance and maintainability.
| // Ensure all the locks are released and follower has time to step up | ||
| tokio::time::sleep(Duration::from_secs(2)).await; | ||
| onchain.mint_block().await; | ||
| // Ensure the follower has stepped up as leader | ||
| tokio::time::sleep(Duration::from_secs(2)).await; |
There was a problem hiding this comment.
For some reason, on this branch, the test faces a race condition. While I can't reproduce it locally on mac os, this fixes the test flakiness.
There was a problem hiding this comment.
Would the wait_for_condition be useful here? Even if with a shorter timeout
There was a problem hiding this comment.
I wonder which condition we should wait for, then? Creating an order?
There was a problem hiding this comment.
too bad we dont have access to the autopilot API/interface to use that here
we could do wait for is_leader 🤔
but reviewing the test i dont have a better idea than the current code, or do the thing above with much more effort, so i think its ok, but we should leave a note that the sleeps are related to a race condition
There was a problem hiding this comment.
we could do wait for is_leader 🤔
I assume, there is no existing function and we need to query the metrics endpoint for that?
There was a problem hiding this comment.
could also be a solution 💡
There was a problem hiding this comment.
I mean, what was your solution?
There was a problem hiding this comment.
i didnt have one specifically, was kind of trying to brainstorm a solution that didn't require the sleeps, i thought there was an api to check the leader status
there isn't (at least not directly) so your suggestion makes sense
| // Ensure all the locks are released and follower has time to step up | ||
| tokio::time::sleep(Duration::from_secs(2)).await; | ||
| onchain.mint_block().await; | ||
| // Ensure the follower has stepped up as leader | ||
| tokio::time::sleep(Duration::from_secs(2)).await; |
There was a problem hiding this comment.
too bad we dont have access to the autopilot API/interface to use that here
we could do wait for is_leader 🤔
but reviewing the test i dont have a better idea than the current code, or do the thing above with much more effort, so i think its ok, but we should leave a note that the sleeps are related to a race condition
Description
Once we started forwarding native price estimates from the orderbook to autopilot, CoinGecko API usage went up. This happened because the estimator moved to autopilot, which now handles all requests and also relies on CoinGecko.
This PR refactors native price estimation by introducing source-aware caching that distinguishes between auction-related requests (actively maintained) and quote/API requests (cached but not maintained). We can skip updating the latter type of requests since, for the quote competition, they are only used to ensure the token is "tradable" and we don't report those prices to the end user. The maintenance task is only required for the auction competition, where the native price matters.
Changes
Shared source-aware caching
Source-aware maintenance
Design considerations
The source-tracking approach reduces CoinGecko API usage by distinguishing request origins:
How to test
Existing tests.