refactor(blend): establish new connections, or deny new streams #989

youngjoon-lee · 2025-01-21T00:10:01Z

1. What does this PR implement?

This PR is the last one of refactoring connection management.
https://www.notion.so/Connection-Management-in-Libp2p-1708f96fb65c8019a6edd2a3b89ff8e2?pvs=4#1768f96fb65c80e58f68ea5300deba40

This PR adds the following actions:

Dial new peers if needed.
Deny a stream of a newly established connection in certain conditions.

The detailed logic is:

Behaviour maintains three sets of peers (healthy, unhealthy, and malicious) based on the output of connection monitoring added in the PR refactor(blend): perform conn monitoring in the ConnectionHandler level #987.
Behaviour determines whether new connections need to be established, based on the number of peers in those three sets. If so, the Behaviour emits an event to the Backend, so that the Backend can select random peers and request Swarm to dial them.
Behaviour denies the stream of connections, if max_peering_degree reached or if the peer has been detected as malicious. It doesn't deny the connection itself (even though libp2p provides an API for that explicitly), because we want to make our behaviour/handler have control on only streams, not connections.

Why Backend selects random peers? not by Behaviour?

We still don't know how to handle membership updates. I want to make membership live in the Backend because we may have more control and flexibility on the Backend rather than Behaviour.
Also, someone who wants to use the Behaviour may have a different kind of membership. So, if possible, I want to decouple Behaviour with membership.

As I noted in Notion, I've found that we don't actually need to define our own ConnectionManager abstraction. However, please feel free to share your opinion if you think it's better to define the abstraction for both Blend and DA. I think, their requirements are quite different.

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:

1. Description added.
2. Context and links to Specification document(s) added.
3. Main contact(s) (developers and specification authors) added
4. Implementation and Specification are 100% in sync including changes. This is critical.
5. Link PR to a specific milestone.

danielSanchezQ · 2025-01-22T10:23:51Z

nomos-blend/network/src/behaviour.rs

+                if self.should_deny_stream(&peer_id) {
+                    // Notify the corresponding [`ConnectionHandler`] to close the stream.
+                    self.events.push_back(ToSwarm::NotifyHandler {
+                        peer_id,
+                        handler: NotifyHandler::One(connection_id),
+                        event: FromBehaviour::CloseSubstreams,
+                    });


This goes before the calls of handle_established_outbound/inbound_connection right? Anyway, we should check if by the time this event is processed we didn't open a stream already. Because we are opening streams for any in/out-bound connections.
Is that the case?

Since they don't have a proper documentation, I investigated their code.

This ConnectionEstablished event is triggered AFTER the handle_established_outbound/inbound_connection is called and the ConnectionHandler is returned from that.

It means that the FromBehaviour::CloseSubstreams that I'm scheduling here will be defintely delivered to the ConnectionHandler. If the ConnectionHandler already established inbound/outbound substreams, they will be replaced with the Dropped state, as I implemented in #988. Even if they're not initialized yet, they'll be set as Dropped and are never going to be initalized.

I still think in this case it is easier to just reject the connection in the handle_stablished_outbound/inboundfrom this behaviour anaway. Accepting a connection just to get it closed feels weird. Maybe I'm missing something here.

Okay. That was my initial implementation when I opened this PR, but changed it while having discussions in other PRs. Rejecting the connection itself is the recommended way by libp2p, as far as I understand. However, as I've been saying, if we have to consider the case where multiple behaviours share a connection, things are complicated, and libp2p don't give us a clear approach. We need to decide.

Let's use the way libp2p recommends. I will revert the handle_established_outbound/inbound_connection to deny the connection.

4680e94 I just updated this PR by using libp2p-allow-block-list and libp2p-connection-limits. It would be better to review this PR itself again, but it might be also easy to review the specific commit if you want.

AlejandroCabeza · 2025-01-23T11:19:15Z

nomos-blend/network/src/peer_sets.rs

+    pub(crate) fn set_malicious(&mut self, peer_id: PeerId) {
+        self.negotiated_healthy_peers.remove(&peer_id);
+        self.negotiated_unhealthy_peers.remove(&peer_id);
+        self.malicious_peers.insert(peer_id);
+    }


If when setting a peer as malicious they are removed from healthy and unhealthy; shouldn't it happen the other way around? E.g.: Shouldn't set_unhealthy remove from malicious_peers?

Ah, good point. Actually, in the current spec, any peer who has been set as malicious can't be removed from the malicious set. But, yeah, as you said, we should handle the case when the peer is in the malicious set at the moment when set_unhealthy is called. I will re-think about this state machine and refine it.

I removed this peer_sets.rs because I found and use libp2p-allow-block-list and libp2p-connection-limits in the end. 4680e94
They simplify this PR a lot. Instead of maintaining the malicious_peers set, now we add malicious peers to the libp2p block list, so that the connections from them or to them can be denied / closed.
It would be better to review this PR itself again, but it might be also easy to review the specific commit if you want.

I'll go over it again, no worries :)

youngjoon-lee · 2025-01-25T02:28:42Z

nomos-blend/network/Cargo.toml

@@ -7,7 +7,7 @@ edition = "2021"
 cached = "0.53.1"
 futures = "0.3.30"
 futures-timer = "3.0.3"
-libp2p = "0.53"
+libp2p = "0.55"


To use all functions of libp2p-allow-block-list, we need to upgrade libp2p to 0.55: libp2p/rust-libp2p@e63975d.

youngjoon-lee · 2025-01-25T02:30:45Z

nomos-libp2p/src/lib.rs

-    pub fn unsubscribe(&mut self, topic: &str) -> Result<bool, PublishError> {
+    pub fn unsubscribe(&mut self, topic: &str) -> bool {


This is because the unsubscribe API has changed in libp2p v0.55

youngjoon-lee · 2025-01-25T02:35:40Z

nomos-services/blend/src/backends/libp2p.rs

+    // TODO: Instead of holding the membership, we just want a way to get the list of addresses.
+    membership: Membership<PeerId, SphinxMessage>,
+    rng: R,
+    peering_degree: usize,


I'll update this to u16 by rebasing this branch to the #987, after all changes are reviewed completely.

youngjoon-lee · 2025-01-25T02:48:17Z

Dockerfile

-FROM rust:1.82.0-slim-bookworm AS builder
+FROM rust:1.84.0-slim-bookworm AS builder


libp2p v0.55 requires Rust >=1.83.

Why libp2p v0.55? Please see #989 (comment)

youngjoon-lee added the blend label Jan 21, 2025

youngjoon-lee added this to the Iteration 9 milestone Jan 21, 2025

youngjoon-lee requested review from danielSanchezQ and bacv January 21, 2025 00:10

youngjoon-lee self-assigned this Jan 21, 2025

This was linked to issues Jan 21, 2025

Don't accept inbound connection establishments more than peering_degree #939

Open

Refactor connection management #990

Open

youngjoon-lee changed the title ~~refactor(blend): establish more connections or deny connections~~ refactor(blend): establish new connections or deny connections Jan 21, 2025

youngjoon-lee changed the title ~~refactor(blend): establish new connections or deny connections~~ refactor(blend): establish or deny new connections Jan 21, 2025

youngjoon-lee mentioned this pull request Jan 21, 2025

refactor(blend): perform conn monitoring in the ConnectionHandler level #987

Open

5 tasks

youngjoon-lee changed the title ~~refactor(blend): establish or deny new connections~~ refactor(blend): establish new connections, or deny new streams. Jan 22, 2025

youngjoon-lee changed the title ~~refactor(blend): establish new connections, or deny new streams.~~ refactor(blend): establish new connections, or deny new streams Jan 22, 2025

youngjoon-lee force-pushed the blend-drop-conns branch from b79429d to 018e6cf Compare January 22, 2025 08:08

youngjoon-lee force-pushed the blend-new-conn branch from d9e2d21 to d276e59 Compare January 22, 2025 08:12

youngjoon-lee force-pushed the blend-drop-conns branch from 018e6cf to 15a6973 Compare January 22, 2025 08:39

youngjoon-lee force-pushed the blend-new-conn branch from d276e59 to 15ab4af Compare January 22, 2025 08:39

danielSanchezQ reviewed Jan 22, 2025

View reviewed changes

youngjoon-lee mentioned this pull request Jan 22, 2025

refactor(blend): drop streams if malicious or on error #988

Open

5 tasks

youngjoon-lee force-pushed the blend-drop-conns branch from 15a6973 to 21cf051 Compare January 23, 2025 05:31

refactor(blend): establish new connections, or deny new streams

6e56328

youngjoon-lee force-pushed the blend-new-conn branch from 15ab4af to 6e56328 Compare January 23, 2025 05:33

AlejandroCabeza reviewed Jan 23, 2025

View reviewed changes

use libp2p-connection-limits and libp2p-allow-block-list

4680e94

youngjoon-lee requested review from danielSanchezQ and AlejandroCabeza January 25, 2025 02:21

youngjoon-lee commented Jan 25, 2025

View reviewed changes

use rust 1.84 for libp2p 0.55

b527360

youngjoon-lee commented Jan 25, 2025

View reviewed changes

AlejandroCabeza approved these changes Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(blend): establish new connections, or deny new streams #989

refactor(blend): establish new connections, or deny new streams #989

youngjoon-lee commented Jan 21, 2025 •

edited

Loading

danielSanchezQ Jan 22, 2025

youngjoon-lee Jan 22, 2025

danielSanchezQ Jan 23, 2025

youngjoon-lee Jan 24, 2025 •

edited

Loading

youngjoon-lee Jan 24, 2025 •

edited

Loading

youngjoon-lee Jan 25, 2025 •

edited

Loading

AlejandroCabeza Jan 23, 2025

youngjoon-lee Jan 24, 2025 •

edited

Loading

youngjoon-lee Jan 25, 2025 •

edited

Loading

AlejandroCabeza Jan 27, 2025

youngjoon-lee Jan 25, 2025

youngjoon-lee Jan 25, 2025

youngjoon-lee Jan 25, 2025

youngjoon-lee Jan 25, 2025 •

edited

Loading

		pub fn unsubscribe(&mut self, topic: &str) -> Result<bool, PublishError> {
		pub fn unsubscribe(&mut self, topic: &str) -> bool {

		FROM rust:1.82.0-slim-bookworm AS builder
		FROM rust:1.84.0-slim-bookworm AS builder

refactor(blend): establish new connections, or deny new streams #989

Are you sure you want to change the base?

refactor(blend): establish new connections, or deny new streams #989

Conversation

youngjoon-lee commented Jan 21, 2025 • edited Loading

1. What does this PR implement?

2. Does the code have enough context to be clearly understood?

3. Who are the specification authors and who is accountable for this PR?

4. Is the specification accurate and complete?

5. Does the implementation introduce changes in the specification?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngjoon-lee Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

youngjoon-lee Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

youngjoon-lee Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngjoon-lee Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

youngjoon-lee Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngjoon-lee Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

youngjoon-lee commented Jan 21, 2025 •

edited

Loading

youngjoon-lee Jan 24, 2025 •

edited

Loading

youngjoon-lee Jan 24, 2025 •

edited

Loading

youngjoon-lee Jan 25, 2025 •

edited

Loading

youngjoon-lee Jan 24, 2025 •

edited

Loading

youngjoon-lee Jan 25, 2025 •

edited

Loading

youngjoon-lee Jan 25, 2025 •

edited

Loading