refactor(blend): perform conn monitoring in the ConnectionHandler level #987
Conversation
youngjoon-lee
force-pushed
the
blend-individual-conn-monitor
branch
from
cefc609 to
f1a11e3
Compare
| pub peering_degree: usize, | ||
| pub max_peering_degree: usize, |
There was a problem hiding this comment.
Unless we want a huge set of peering degree, we can constrain this a bit more. wdyt?
There was a problem hiding this comment.
That's actually true. In practice, I guess u8 would be enough, but it would be safer to use u16 because it can represent the max number of network ports that OS can use.
There was a problem hiding this comment.
I agree, u16 sounds good.
Are we gonna change this for the crate you found?
There was a problem hiding this comment.
I updated them to u16: 2f4192b.
My investigation on https://docs.rs/libp2p-connection-limits/latest/libp2p_connection_limits/ is not done yet. I will open another PR.
| @@ -149,6 +142,16 @@ where | |||
| panic!("Failed to listen on Blend network: {e:?}"); | |||
| }); | |||
|
|
|||
| // Dial the initial peers randomly selected | |||
| membership | |||
| .choose_remote_nodes(&mut rng, config.peering_degree) | |||
There was a problem hiding this comment.
Where does this method comes from? If we added this to the MembershipHandler trait and I did not realize, this should not be there.
There was a problem hiding this comment.
This method is defined in the Membership struct. This struct is a different one from the membership in DA. Could you elaborate your question more?
|
lgtm (not approving, though, because I believe lack necessary context) |
1. What does this PR implement?
(The base branch of this PR is the PR #986.)
This PR is the 2nd step in the journey of connection management refactoring: https://www.notion.so/Connection-Management-in-Libp2p-1708f96fb65c8019a6edd2a3b89ff8e2?pvs=4#1768f96fb65c80e58f68ea5300deba40.
In the previous implementation, the logic for connection monitoring was handled at the
Behaviourlevel. Specifically, theBehaviourperiodically evaluated the status of ALL peers, disconnected malicious peers, and selected new peers to connect with. However, I've found that this approach is cumbersome. Managing the bulk result from ALL peers was more complex than I expected. Furthermore, peers actually don't need to be monitored within a shared time window; it’s sufficient to monitor each peer individually at its own interval since each node in the network operates independently, without relying on a common epoch.To make the structure simpler, this PR moves the connection monitoring logic from the
Behaviourto theConnectionHandlerlevel. This adjustment aligns with the responsibilities of theConnectionHandler, which already manages data reading from a substream. Under this new structure, when an interval elapses, theConnectionHandlerevaluates the status of its assigned peer and reports the results to theBehaviour. Then, theBehaviourmakes a decision what to do. considering the number of active streams. For example, theBehaviourrequests theSwarmto dial new peers if needed.This approach is well-suited with libp2p's design. Additionally, it simplifies handling peers detected as malicious or unhealthy, as the reports are processed individually, rather than in bulk. Also, now the
Behaviouris much lightweight.It’s important to note that this PR does not yet include logic for closing connections or establishing new ones, just to make your review easier. These features will be implemented in the next PR. I added TODO comment for those.
2. Does the code have enough context to be clearly understood?
Yes
3. Who are the specification authors and who is accountable for this PR?
@youngjoon-lee
4. Is the specification accurate and complete?
Yes
5. Does the implementation introduce changes in the specification?
No
Checklist
Warning
Do not merge the PR if any of the following is missing: