Refactor PeerManager to introduce Connectivity Module #6858
Conversation
|
|
diegomrsantos
force-pushed
the
pm-connectivity-to-unstable
branch
from
472cf2e to
6d4bb6f
Compare
diegomrsantos
changed the title
diegomrsantos
changed the title
There was a problem hiding this comment.
This looks good to me. Seems like a good way to split things up.
Historically there have been some sneaky bugs in the peer manager when we try to modify its workings. It usually requires a bit more testing than most other parts of code.
We are currently in the planning of making a testnet release for an upcomming fork. We'll have to decide whether to put this in with electra or leave it for later.
Have you done any testing on lighthouse for this?
| if wanted_peers > 0 { | ||
| self.events | ||
| .push(PeerManagerEvent::DiscoverPeers(wanted_peers)); | ||
| // debug!(self.log, |
There was a problem hiding this comment.
Do we still need this log?
I notice this is down lower in the file
There was a problem hiding this comment.
I'm very sorry about that. I decided to make the code more similar to the original one. Please retake a look.
| .update_min_ttl(&peer_id, *min_ttl); | ||
| } | ||
| if self.dial_peer(enr.clone()) { | ||
| //debug!(self.log, "Added discovered ENR peer to dial queue"; "peer_id" => %peer_id); |
| } | ||
| } | ||
|
|
||
| /// This function checks the status of our current peers and optionally requests a discovery |
There was a problem hiding this comment.
These comments are now longer correct. This function returns a usize and doesn't request any discoveries. Maybe we need to change the name of the function to better reflect this change of logic.
| /// Peers that have been returned by discovery requests that are suitable for dialing are | ||
| /// returned here. | ||
| /// | ||
| /// This function decides whether or not to dial these peers. |
There was a problem hiding this comment.
These comments should describe what the return value is used for as this logic has changed here and it might be easier for future people to understand when looking at what these functions do
diegomrsantos
force-pushed
the
pm-connectivity-to-unstable
branch
4 times, most recently
from
e3e4d4f to
8dd9692
Compare
diegomrsantos
force-pushed
the
pm-connectivity-to-unstable
branch
from
8dd9692 to
abd0f51
Compare
Issue Addressed
#6860
Proposed Changes
Introduction of Connectivity Struct:
NetworkGlobalsProvider Trait:
Refactoring PeerManager:
Removed direct use of discovery_enabled and network global operations from PeerManager.
File Changes:
Additional Info
The plan is to continue refactoring PeerManager and introducing new modules.