refactor! clean up ContentInfo type, remove unexpected paths #1528
Conversation
KolbyML
force-pushed
the
fix-content-info
branch
from
1a606d4 to
b8e3d73
Compare
KolbyML
force-pushed
the
fix-content-info
branch
from
b8e3d73 to
8e817d5
Compare
|
nit since it's a breaking change, it should use a Also, it would be good to get that breaking change updated in glados as soon as possible (and Hive, but I assume that was already on your mind). |
| @@ -49,9 +49,7 @@ pub struct TraceGossipInfo { | |||
| #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] | |||
There was a problem hiding this comment.
nit: The documentation should be updated (it's not longer used for GetContent)
ethportal-api/src/types/portal.rs
Outdated
| pub utp_transfer: bool, | ||
| } | ||
|
|
||
| /// Parsed response for TraceRecursiveFindContent endpoint |
There was a problem hiding this comment.
nit: Isn't this TraceGetContent now?
portal-bridge/src/bridge/era1.rs
Outdated
| @@ -176,7 +176,7 @@ impl Era1Bridge { | |||
| let hunter_threshold = (content_keys_to_sample.len() as u64 * threshold / 100) as usize; | |||
| for content_key in content_keys_to_sample { | |||
| let result = self.portal_client.get_content(content_key.clone()).await; | |||
| if let Ok(ContentInfo::Content { .. }) = result { | |||
| if let Ok(GetContentInfo { .. }) = result { | |||
There was a problem hiding this comment.
Because GetContentInfo is not enum (as this result type used to be), I believe that this check can be simplified to:
if result.is_ok() {Also in other places below.
portal-bridge/src/gossip.rs
Outdated
| @@ -61,7 +61,7 @@ async fn beacon_trace_gossip( | |||
| } | |||
| // if not, make rfc request to see if data is available on network | |||
| let result = BeaconNetworkApiClient::get_content(&client, content_key.clone()).await; | |||
| if let Ok(ContentInfo::Content { .. }) = result { | |||
| if let Ok(GetContentInfo { .. }) = result { | |||
src/bin/poll_latest.rs
Outdated
There was a problem hiding this comment.
I think that many of changes in this file are not needed. While it does preserve original functionality, I think original implementation was wrong (because it wasn't handling errors correctly).
Meaning, if GetContent fails (content not available) it should keep retrying, right?
So timeout and backoff should still be used.
In my opinion, in audit_content_key we should have something like:
while Instant::now() - timestamp < timeout {
match client.get_content(content_key.clone()).await {
Ok(_) => return Ok(Instant::now()),
_ => {
attempts += 1;
let sleep_time = match backoff {
Backoff::Exponential => attempts * 2,
Backoff::Linear(delay) => delay,
};
sleep(Duration::from_secs(sleep_time)).await;
}
}
}which is closer to the original implementation.
Otherwise, if you think new implementation is correct, than you should cleanup more of this file (remove Backoff, update documentation on how to run it, etc).
There was a problem hiding this comment.
The new implementation is the same as the old implementation, but I agree that your example is most likely what was originally intended
KolbyML
changed the title
portal-bridge/src/bridge/era1.rs
Outdated
| @@ -368,7 +368,7 @@ impl Era1Bridge { | |||
| let header_hash = block_tuple.header.header.hash(); | |||
| let header_content_key = HistoryContentKey::new_block_header_by_hash(header_hash); | |||
| let header_content_info = portal_client.get_content(header_content_key.clone()).await; | |||
| if let Ok(ContentInfo::Content { .. }) = header_content_info { | |||
| if let Ok(GetContentInfo { .. }) = header_content_info { | |||
There was a problem hiding this comment.
nit: this could also be header_content_info.is_ok().
Also in 3 more places in this file below.
| pub enum ContentInfo { | ||
| #[serde(rename_all = "camelCase")] | ||
| ConnectionId { connection_id: u16 }, | ||
| pub enum FindContentInfo { |
There was a problem hiding this comment.
The documentation is still not updated.
BTW, sorry I misspoke about the conventional commits standard. For next time: it is supposed to be to add a |
What was wrong?
fixes #1526 (comment)
Basically the
ContentInfotype we were using for ourFindContentandGetContentRPC endpoints were leaking information implementation details not related to the endpointsHow was it fixed?
Split
ContentInfointoFindContentInfoGetContentInfoand only expose the datatypes specified in the
portal-network-specsjson-rpc specification