-
Notifications
You must be signed in to change notification settings - Fork 264
feat: basic performance contract integration [within Nym API] #5871
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
feat: basic performance contract integration [within Nym API] #5871
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
…n epoch is not available
23ee559 to
c89267a
Compare
durch
left a comment
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.
I've got one bug, and two suggestion. Apart from fixing the bug, I think implementing the performance optimisation might be worth while, not so sure on the race condition, maybe I'm just a bit pedantic/paranoid here
| Ok(PerformanceContractCacheData { epoch_performance }) | ||
| } | ||
|
|
||
| async fn refresh(&mut self) -> Result<Option<PerformanceContractEpochCacheData>, NyxdError> { |
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.
There might be a race condition in here, thinking of a few scenarios:
Scenario 1: New submission between check and fetch
- Refresher: Gets last_submission with block_height: 1000, epoch_id: 100
- Refresher: Determines it's different from cached (needs update)
- Meanwhile: New data submitted to contract at block_height: 1001, epoch_id: 101
- Refresher: Fetches performance data (might get epoch 101 data)
- Refresher: Stores last_submission as the old value from step 1
- Refresher: Will think there's an update (because stored != actual) but fetch same data
Scenario 2: Epoch transition during refresh
- Refresher: Gets last_submission for epoch 100
- Refresher: Gets current_epoch = 100
- Meanwhile: Epoch transitions to 101
- Refresher: Tries to fetch epoch 100 performance (might fail or get incomplete data)
- System could miss the epoch 101 data entirely
I think we should use the actual fetched data to determine if we have an update, not the metadata about when data was last submitted
Maybe we could fetch both pieces of data at the same time:
// Get current epoch
let current_epoch = self
.mixnet_contract_cache
.current_interval()
.await
.unwrap()
.current_epoch_absolute_id();
// Fetch both submission info and performance data in parallel
let (last_submitted, performance_result) = tokio::join!(
self.nyxd_client.get_last_performance_contract_submission(),
self.nyxd_client.get_full_epoch_performance(current_epoch)
); Then figure out if we've already processed this and update if needed
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.
I think we should use the actual fetched data to determine if we have an update, not the metadata about when data was last submitted
the problem with this approach is that we'd have to fetch a lot of data, so that's why I introduced the metadata to have a quick way of seeing if there has been any updates since the last time it was polled. remember that we really don't care about data from the past epochs and are only interested if the most recent one is updated. on startup we pull the full information for the however many fallback epochs we care about and then in the loop we check "has there been anything new submitted? if so, update current epoch" - if the update was for some previous epochs. tough luck. they should have submitted it earlier (and that data would have only ever been used as fallback anyway). in 99% cases network monitors wouldn't be running behind and would actually be submitting only for the current epoch.
with that in mind, let's consider those race condition scenarios:
- I'm assuming in this scenario the current epoch is
101. so we'll just refresh101instead of getting anything for100. it will be slightly inefficient since we'll just refresh what we already had, but it shouldn't hurt at all. - so epoch will transition to
101and we'll potentially get empty data for the current epoch. but in this instance, when anybody queries for "current" performance, we will fallback to information we already had on100. Also, this is an incredibly unlikely scenario as network monitors are meant to (well, once implemented) submit performance measurements for epochs BEFORE the actual transition takes place
also, fetching those at the same time wouldn't have solved anything as in the world of the internet one of those would have been resolved first anyway (plus get_full_epoch_performance is actually multiple paged queries)
Further builds up on #5833 by integrating the performance contract into nym-api.
By setting
.performance_provider.debug.use_performance_contract_datato true in the api's config, it will attempt to use the contract as the source of node scores for all annotations (and thus endpoints and rewarding). Note that if the contract address is not set or the contract itself is empty, the binary will not start. If the flag is not enabled, the binary will work like before, i.e. it will just use whatever it has in the storage.For now this flag shouldn't be set, for there's one more missing piece of the puzzle to complete the whole thing. A separate process to actually submit aggregated data to the aforementioned contract.
This change is