Skip to content
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

refactor: perform local db content lookup at OverlayService level #1637

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

njgheorghita
Copy link
Collaborator

@njgheorghita njgheorghita commented Jan 15, 2025

What was wrong?

Fixes #1329
This feature is needed to more efficiently perform beacon sync inside hive tests.

How was it fixed?

  • Perform local store content lookup at OverlayService level rather than at JsonRpc level.

To-Do

@njgheorghita njgheorghita marked this pull request as ready for review January 15, 2025 21:38
Comment on lines 2364 to 2380
// Lookup content locally before querying the network.
if let Ok(Some(content)) = self.store.read().get(&target) {
let local_enr = self.local_enr();
let mut query_trace = QueryTrace::new(&local_enr, target.content_id().into());
query_trace.node_responded_with_content(&local_enr);
query_trace.content_validated(local_enr.into());
if let Some(callback) = callback {
let _ = callback.send(Ok((
RawContentValue::from(content),
false,
Some(query_trace),
)));
return;
} else {
warn!("Local content found but no callback to return it to overlay protocol, continuing with network query & ignoring local content.");
}
}
Copy link
Member

@KolbyML KolbyML Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Lookup content locally before querying the network.
if let Ok(Some(content)) = self.store.read().get(&target) {
let local_enr = self.local_enr();
let mut query_trace = QueryTrace::new(&local_enr, target.content_id().into());
query_trace.node_responded_with_content(&local_enr);
query_trace.content_validated(local_enr.into());
if let Some(callback) = callback {
let _ = callback.send(Ok((
RawContentValue::from(content),
false,
Some(query_trace),
)));
return;
} else {
warn!("Local content found but no callback to return it to overlay protocol, continuing with network query & ignoring local content.");
}
}
// Lookup content locally before querying the network.
if let Ok(Some(content)) = self.store.read().get(&target) {
let local_enr = self.local_enr();
let mut query_trace = QueryTrace::new(&local_enr, target.content_id().into());
query_trace.node_responded_with_content(&local_enr);
query_trace.content_validated(local_enr.into());
if let Some(callback) = callback {
let _ = callback.send(Ok((
RawContentValue::from(content),
false,
Some(query_trace),
)));
return;
} else {
warn!("Local content found but no callback to return it to overlay protocol, continuing with network query & ignoring local content.");
}
}

I think we should remove the Option on the callback, the option is only used for testing, which tastes weird, also we should remove this warn! and check if there is a channel, before checking local storage as this is an expensive operation.

But as I said before we should just remove the option, I don't think we should have special arguments just for writing unit tests.

It doesn't make sense to do a storage lookup, if we aren't going to use it.

@@ -2369,9 +2358,27 @@ impl<
target: TContentKey,
callback: Option<oneshot::Sender<RecursiveFindContentResult>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
callback: Option<oneshot::Sender<RecursiveFindContentResult>>,
callback: oneshot::Sender<RecursiveFindContentResult>,

I don't think this should be an option

@njgheorghita njgheorghita requested a review from KolbyML January 16, 2025 18:10
Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: this PR looks very good

@njgheorghita njgheorghita merged commit c0e6d13 into ethereum:master Jan 16, 2025
11 checks passed
@njgheorghita njgheorghita deleted the overlay-service-db-lookup branch January 16, 2025 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

portalnet/overlay: FindContent query doesn't check for the content in database before making network requests
2 participants