-
Notifications
You must be signed in to change notification settings - Fork 147
feat: add select_random_peers to census #1850
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
Conversation
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.
Good work. Few suggestions.
🚀
content_key: Option<&impl OverlayContentKey>, | ||
content_id: &Option<[u8; 32]>, |
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 this should now be something like:
content_key_and_id: &Option<(&impl OverlayContentKey, [u8; 32])>
and line 26 should be:
let content_key_and_id = content_key.map(|key| (key, key.content_id()));
@@ -108,7 +108,7 @@ impl Network { | |||
/// Selects peers to receive content. |
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 this documentation should clarify what happens in None
is passed, something like:
/// Selects peers to receive content. | |
/// Selects peers to receive content. | |
/// | |
/// If content key is present, only peers interested in content will be returned. | |
/// Otherwise, all peers are eligible. |
@@ -123,7 +123,7 @@ impl<W: Weight> Peers<W> { | |||
} | |||
|
|||
/// Selects peers to receive content. |
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.
Also here
@@ -13,17 +13,20 @@ use super::peer::{Peer, PeerInfo}; | |||
pub trait Weight: Send + Sync { | |||
fn weight( |
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 should have a documentation as well.
What was wrong?
This is a chunk from #1846
select_peers()
take a content_key this doesn't make when gossiping a series of EphemeralHeadersByOffer as we just want working random Enr'sHow was it fixed?
At the top level of
census
add a functionselect_random_peers()
, then just pass an option from there down.