feat(network): add peer manager struct - not used yet

[nagmo-starkware]

Pull Request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build-related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

• Yes
• No

Other information

---

This change is 

[nagmo-starkware]

• feat(network): add peer manager struct - not used yet #1874 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @nagmo-starkware and the rest of your teammates on Graphite

[ShahakShama]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @nagmo-starkware)

---

crates/papyrus_network/src/peer_manager/mod.rs line 24 at r1 (raw file):

    query_to_peer_map: HashMap<QueryId, PeerId>,
    config: PeerManagerConfig,
    last_peer_idx: usize,

idx -> index

---

crates/papyrus_network/src/peer_manager/mod.rs line 36 at r1 (raw file):

pub(crate) enum PeerManagerError {
    #[error("No such peer: {0}")]
    NoSuchPeer(PeerId),

I like this. We should rename our other errors in the network to NoSuch*

---

crates/papyrus_network/src/peer_manager/mod.rs line 43 at r1 (raw file):

impl Default for PeerManagerConfig {
    fn default() -> Self {
        Self { target_num_for_peers: 100, blacklist_timeout: Duration::MAX }

Please check that adding Duration::MAX to an Instant won't cause overflow

---

crates/papyrus_network/src/peer_manager/mod.rs line 57 at r1 (raw file):

    }

    fn add_peer(&mut self, mut peer: P) {

pub fn

---

crates/papyrus_network/src/peer_manager/mod.rs line 67 at r1 (raw file):

    }

    fn assign_peer_to_query(&mut self, query_id: QueryId) -> Option<(PeerId, Multiaddr)> {

pub fn

---

crates/papyrus_network/src/peer_manager/mod.rs line 71 at r1 (raw file):

            return None;
        }
        let peer = self.peers.iter().skip(self.last_peer_idx).find(|(_, peer)| !peer.is_blocked());

If you didn't find in the range [last_peer_idx, peers.len()), you should look in the range [0, last_peer_idx)

---

crates/papyrus_network/src/peer_manager/mod.rs line 79 at r1 (raw file):

    }

    fn report_peer(

pub fn

---

crates/papyrus_network/src/peer_manager/mod.rs line 92 at r1 (raw file):

    }

    fn report_query(

pub fn

---

crates/papyrus_network/src/peer_manager/mod.rs line 105 at r1 (raw file):

            }
        } else {
            Err(PeerManagerError::NoSuchPeer(PeerId::random()))

Make a different variant called NoSuchQuery

---

crates/papyrus_network/src/peer_manager/mod.rs line 109 at r1 (raw file):

    }

    fn more_peers_needed(&self) -> bool {

pub fn

---

crates/papyrus_network/src/peer_manager/mod.rs line 110 at r1 (raw file):

    fn more_peers_needed(&self) -> bool {
        self.peers.len() < self.config.target_num_for_peers

Add TODO to consider counting only non blocked peers

---

crates/papyrus_network/src/peer_manager/peer.rs line 13 at r1 (raw file):

    fn update_reputation(&mut self, reason: ReputationModifier) -> Result<(), PeerError>;

    fn get_id(&self) -> PeerId;

Remove the get, it doesn't correspond to rust style guide
Additionally, change the id to peer_id. It's a term from libp2p (The second point is relevant even if you disagree on the first)

---

crates/papyrus_network/src/peer_manager/peer.rs line 15 at r1 (raw file):

    fn get_id(&self) -> PeerId;

    fn get_multiaddr(&self) -> Multiaddr;

Remove the get, it doesn't correspond to rust style guide

---

crates/papyrus_network/src/peer_manager/peer.rs line 17 at r1 (raw file):

    fn get_multiaddr(&self) -> Multiaddr;

    fn set_timeout_duration(&mut self, duration: Duration);

Why not put it as part of the constructor fn new(duration: Duration) -> Self and then the timeout_duration will be non option and you can erase PeerError

---

crates/papyrus_network/src/peer_manager/peer.rs line 30 at r1 (raw file):

#[derive(Clone)]
pub(crate) struct Peer {
    id: PeerId,

id -> peer_id

---

crates/papyrus_network/src/peer_manager/peer.rs line 32 at r1 (raw file):

    id: PeerId,
    multiaddr: Multiaddr,
    timed_out_until: Option<time::Instant>,

Consider importing Instant. I don't see a difference between Instant and Duration

---

crates/papyrus_network/src/peer_manager/peer.rs line 38 at r1 (raw file):

#[allow(dead_code)]
impl Peer {
    pub fn new(id: PeerId, multiaddr: Multiaddr) -> Self {

id -> peer_id

---

crates/papyrus_network/src/peer_manager/peer.rs line 45 at r1 (raw file):

impl PeerTrait for Peer {
    fn update_reputation(&mut self, _reason: ReputationModifier) -> Result<(), PeerError> {
        if let Some(timeout_duration) = self.timeout_duration {

Consider using let else

---

crates/papyrus_network/src/peer_manager/peer.rs line 65 at r1 (raw file):

    fn is_blocked(&self) -> bool {
        if let Some(timed_out_until) = self.timed_out_until {

Consider self.timed_out_until.map(|timed_out_until| timed_out_until > time::Instant::now()).unwrap_or(false)
Both are valid. Choose whatever you think is cleaner

---

crates/papyrus_network/src/peer_manager/test.rs line 1 at r1 (raw file):

use std::time::Duration;

Add test with a low blacklist_timeout in the config and check that the peer becomes unblocked after you sleep for blacklist_timeout

---

crates/papyrus_network/src/peer_manager/test.rs line 33 at r1 (raw file):

    // Verify that the peers are assigned in a round-robin fashion
    match res1.unwrap().0 {

Isn't it always guaranteed that the first peer will be the first?

---

crates/papyrus_network/src/peer_manager/test.rs line 46 at r1 (raw file):

}

#[test]

Add test where there are no blocked peers. You can do it in this test if you want

---

crates/papyrus_network/src/peer_manager/test.rs line 178 at r1 (raw file):

    let peer_id = PeerId::random();
    let mut peer = MockPeerTrait::new();
    let mut mockall_seq = mockall::Sequence::new();

Reminding you to erase this if we decide erasing set_timeout_duration :)

[nagmo-starkware]

Reviewable status: 1 of 6 files reviewed, 22 unresolved discussions (waiting on @ShahakShama)

---

crates/papyrus_network/src/peer_manager/mod.rs line 24 at r1 (raw file):

Previously, ShahakShama wrote…

idx -> index

Done.

---

crates/papyrus_network/src/peer_manager/mod.rs line 43 at r1 (raw file):

Previously, ShahakShama wrote…

Please check that adding Duration::MAX to an Instant won't cause overflow

moved to chrono time to support all OSs (previous version works only on linux without panic)

---

crates/papyrus_network/src/peer_manager/mod.rs line 57 at r1 (raw file):

Previously, ShahakShama wrote…

pub fn

Done.

---

crates/papyrus_network/src/peer_manager/mod.rs line 67 at r1 (raw file):

Previously, ShahakShama wrote…

pub fn

Done.

---

crates/papyrus_network/src/peer_manager/mod.rs line 71 at r1 (raw file):

Previously, ShahakShama wrote…

If you didn't find in the range [last_peer_idx, peers.len()), you should look in the range [0, last_peer_idx)

Done. added a test for that

---

crates/papyrus_network/src/peer_manager/mod.rs line 79 at r1 (raw file):

Previously, ShahakShama wrote…

pub fn

Done.

---

crates/papyrus_network/src/peer_manager/mod.rs line 92 at r1 (raw file):

Previously, ShahakShama wrote…

pub fn

Done.

---

crates/papyrus_network/src/peer_manager/mod.rs line 105 at r1 (raw file):

Previously, ShahakShama wrote…

Make a different variant called NoSuchQuery

Done.

---

crates/papyrus_network/src/peer_manager/mod.rs line 109 at r1 (raw file):

Previously, ShahakShama wrote…

pub fn

Done.

---

crates/papyrus_network/src/peer_manager/mod.rs line 110 at r1 (raw file):

Previously, ShahakShama wrote…

Add TODO to consider counting only non blocked peers

Done.

---

crates/papyrus_network/src/peer_manager/peer.rs line 13 at r1 (raw file):

Previously, ShahakShama wrote…

Remove the get, it doesn't correspond to rust style guide
Additionally, change the id to peer_id. It's a term from libp2p (The second point is relevant even if you disagree on the first)

Done.

---

crates/papyrus_network/src/peer_manager/peer.rs line 15 at r1 (raw file):

Previously, ShahakShama wrote…

Remove the get, it doesn't correspond to rust style guide

Done.

---

crates/papyrus_network/src/peer_manager/peer.rs line 17 at r1 (raw file):

Previously, ShahakShama wrote…

Why not put it as part of the constructor fn new(duration: Duration) -> Self and then the timeout_duration will be non option and you can erase PeerError

because peer is public interface and I don't want the consumers to have the ability to put whatever timeout they want. I want the peer manager to have control of it.

---

crates/papyrus_network/src/peer_manager/peer.rs line 30 at r1 (raw file):

Previously, ShahakShama wrote…

id -> peer_id

Done.

---

crates/papyrus_network/src/peer_manager/peer.rs line 32 at r1 (raw file):

Previously, ShahakShama wrote…

Consider importing Instant. I don't see a difference between Instant and Duration

not relevant anymore

---

crates/papyrus_network/src/peer_manager/peer.rs line 38 at r1 (raw file):

Previously, ShahakShama wrote…

id -> peer_id

Done.

---

crates/papyrus_network/src/peer_manager/peer.rs line 45 at r1 (raw file):

Previously, ShahakShama wrote…

Consider using let else

I like this syntax better

---

crates/papyrus_network/src/peer_manager/peer.rs line 65 at r1 (raw file):

Previously, ShahakShama wrote…

Consider self.timed_out_until.map(|timed_out_until| timed_out_until > time::Instant::now()).unwrap_or(false)
Both are valid. Choose whatever you think is cleaner

I think this is more readable

---

crates/papyrus_network/src/peer_manager/test.rs line 1 at r1 (raw file):

Previously, ShahakShama wrote…

Add test with a low blacklist_timeout in the config and check that the peer becomes unblocked after you sleep for blacklist_timeout

Done.

---

crates/papyrus_network/src/peer_manager/test.rs line 33 at r1 (raw file):

Previously, ShahakShama wrote…

Isn't it always guaranteed that the first peer will be the first?

no it's stored in a hashmap and there's no guaranty of the order of iteration

---

crates/papyrus_network/src/peer_manager/test.rs line 46 at r1 (raw file):

Previously, ShahakShama wrote…

Add test where there are no blocked peers. You can do it in this test if you want

isn't it the previous test peer_assignment_round_robin?

---

crates/papyrus_network/src/peer_manager/test.rs line 178 at r1 (raw file):

Previously, ShahakShama wrote…

Reminding you to erase this if we decide erasing set_timeout_duration :)

:)

[ShahakShama]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nagmo-starkware)

---

crates/papyrus_network/src/peer_manager/mod.rs line 43 at r1 (raw file):

Previously, nagmo-starkware wrote…

moved to chrono time to support all OSs (previous version works only on linux without panic)

Could you add a comment above the use statement stating why we use chrono and not std?

---

crates/papyrus_network/src/peer_manager/peer.rs line 17 at r1 (raw file):

Previously, nagmo-starkware wrote…

because peer is public interface and I don't want the consumers to have the ability to put whatever timeout they want. I want the peer manager to have control of it.

You can make the new function pub(super)

---

crates/papyrus_network/src/peer_manager/test.rs line 46 at r1 (raw file):

Previously, nagmo-starkware wrote…

isn't it the previous test peer_assignment_round_robin?

Sorry, I meant where all peers are blocked in my original comment

---

crates/papyrus_network/src/peer_manager/test.rs line 83 at r2 (raw file):

    peer.update_reputation(ReputationModifier::Bad {}).unwrap();
    assert!(peer.is_blocked());
    sleep(time::Duration::from_secs(1)).await;

Does this test take 1 second? If yes, reduce the timeout to 10/50 milliseconds and anyway, extract the time to a constant at the start of the test

[ShahakShama]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nagmo-starkware)

---

crates/papyrus_network/src/peer_manager/test.rs line 178 at r1 (raw file):

Previously, nagmo-starkware wrote…

:)

Still reminding because that discussion is open

[nagmo-starkware]

Reviewable status: 3 of 6 files reviewed, 5 unresolved discussions (waiting on @ShahakShama)

---

crates/papyrus_network/src/peer_manager/mod.rs line 43 at r1 (raw file):

Previously, ShahakShama wrote…

Could you add a comment above the use statement stating why we use chrono and not std?

I think no one will ever give a shit but sure...

---

crates/papyrus_network/src/peer_manager/peer.rs line 17 at r1 (raw file):

Previously, ShahakShama wrote…

You can make the new function pub(super)

no.. I do want the ability to create new peers just not to set the timeout duration.
once you add a peer you pass ownership to the peer manager which then is setting the duration

---

crates/papyrus_network/src/peer_manager/test.rs line 46 at r1 (raw file):

Previously, ShahakShama wrote…

Sorry, I meant where all peers are blocked in my original comment

given you can block a peer it's the same functionality as having no peers.. I don't see the value in it

---

crates/papyrus_network/src/peer_manager/test.rs line 83 at r2 (raw file):

Previously, ShahakShama wrote…

Does this test take 1 second? If yes, reduce the timeout to 10/50 milliseconds and anyway, extract the time to a constant at the start of the test

Done.

[ShahakShama]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nagmo-starkware)

[ShahakShama]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nagmo-starkware)