Update persistent networking parameters manager. #1762

shamil-gadelshin · 2023-08-04T13:46:38Z

This PR contains two changes to the persistent networking parameters manager:

add ignore list to skip address adding (to prevent bootstrap nodes to get there)
add address removed event to remove address from Kademlia when the networking parameters manager reports an obsolete address

Code contributor checklist:

I have read, understood and followed contributing guide

nazar-pc · 2023-08-05T23:42:31Z

crates/subspace-networking/src/behavior/persistent_parameters.rs

@@ -62,6 +76,13 @@ pub trait NetworkingParametersRegistry: Send + Sync {

    /// Enables Clone implementation for `Box<dyn NetworkingParametersRegistry>`
    fn clone_box(&self) -> Box<dyn NetworkingParametersRegistry>;
+
+    /// Triggers when we removed the peer address from the permanent storage. Returns optional


I think this should be a bit more specific, we don't really remove it from persistent storage when this event is triggered. Maybe call it on_unreachable or something of that sort?

Hmm, I like on_unreachable_address but NetworkingParametersRegistry only knows about address removal nothing about the connectivity. I could add the intended usage comment though.

But it does have a timer and it does essentially check for how long address was unreachable.

I updated the comment.

I still think we need to remove it. Current name is misleading because address is literally not removed from anywhere.

Ok. I'll change to on_unreachable_address

nazar-pc · 2023-08-05T23:46:29Z

crates/subspace-networking/src/behavior/persistent_parameters.rs

+                                last_address
+                            };
+
+                            address_removed_events.push(address_removed);


Why not calling even handlers from here? Looks like you're collecting them into vector here just to iterate it afterwards. Or you can turn this for_each into filter_map and return iterator of these things without collecting first.

It's easier to test this way. I don't expect a lot of elements in this vector.

crates/subspace-networking/src/node_runner.rs

nazar-pc · 2023-08-05T23:53:42Z

crates/subspace-networking/src/node_runner.rs

+        if event.last_address {
+            self.swarm
+                .behaviour_mut()
+                .kademlia
+                .remove_peer(&event.peer_id);
+        } else {
+            self.swarm
+                .behaviour_mut()
+                .kademlia
+                .remove_address(&event.peer_id, &event.address);
+        }


Just curious, will Kademlia really keep a peer in routing table that we know no address of? Also maybe you can query Kademlia for list of addresses rather than sending last_address with event? Just looks like an awkward API.

Yes, it does. However, I think there is a bug here anyway. The problem will arise when we remove known addresses that are unresponsive and remove other addresses (via remove_peer) that we don't know about because we didn't dial them previously. I changed the logic to handle the UnroutablePeer event instead.

Okay, that makes sense and I think we should then replace handle_removed_address_event with handle_removed_peer, but even then it seems problematic that we may not remember all the peers in Kademlia anyway, thus unable to remove them. Should known peers be unbounded in memory then and bounded when written to disk (by unbounded I mean bounded by Kademlia externally)?

I'm not sure I followed here. I moved remove_peer call from there the UnroutablePeer handler. This way I both remove unreachable addresses and handler unroutable peers. Am I missing something?

What I meant is that our known peers cache is limited in size. It seems possible that we will have some peers added to Kademlia, but not in known peers. If that is the case, they will never be removed from Kademlia and we do not have event about things evicted from LRU cache of known peers.

Can we derive cache limits in your pending PR from the Kademlia internals?

Not dynamically, but if we have fixed Kademlia parameters and we know upper bound then yes. I agree this PR is an improvement, I'm just trying to understand whether it is a 100% solution to the problem.

crates/subspace-networking/src/utils.rs

crates/subspace-networking/src/behavior/persistent_parameters.rs

nazar-pc · 2023-08-06T00:02:30Z

crates/subspace-networking/src/node_runner.rs

@@ -397,10 +398,16 @@ where
                .kademlia
                .remove_peer(&event.peer_id);
        } else {
+            // Remove both versions of the address


Is it really possible to have both? It seems odd to not have identity in the address, how would you establish secure connection to someone you don't know identity of?

Technically, it's possible: https://github.com/libp2p/rust-libp2p/blob/f95f39f634d08dad8df72d7ecf92312f9ad2d826/swarm/src/dial_opts.rs#L79

We don't use this feature though. AFAIK, in each case, when we need to establish a connection with the peer we always have a function signature with both PeerId and Multiaddress.

I wouldn't handle it then if it is not used

I meant when we use (implicitly) address without P2p(peer_id) protocol we have PeerId known. But it's Kademlia internals. I saw both types of addresses in the logs this is why I remove both types as well.

Well, I'm not sure when we use address without Peer ID, this is why I'm confused.

We use addresses without Peer ID when we add them as self-reported addresses to Kademlia. They become self-reported when they are taken from listening and external addresses lists on the other peer.

But we do not add our own address to our Kademlia, do we? I don't think I follow you here.

Actually, we do (implicitly) - when we add external addresses and specify listening addresses. When we request providers - Kademlia fills providers with those addresses (both lists chained).

nazar-pc · 2023-08-07T12:39:55Z

crates/subspace-networking/src/behavior/persistent_parameters.rs

@@ -62,6 +76,13 @@ pub trait NetworkingParametersRegistry: Send + Sync {

    /// Enables Clone implementation for `Box<dyn NetworkingParametersRegistry>`
    fn clone_box(&self) -> Box<dyn NetworkingParametersRegistry>;
+
+    /// Triggers when we removed the peer address from the permanent storage. Returns optional


But it does have a timer and it does essentially check for how long address was unreachable.

crates/subspace-networking/src/behavior/persistent_parameters.rs

crates/subspace-networking/src/node_runner.rs

nazar-pc · 2023-08-07T12:44:43Z

crates/subspace-networking/src/node_runner.rs

@@ -397,10 +398,16 @@ where
                .kademlia
                .remove_peer(&event.peer_id);
        } else {
+            // Remove both versions of the address


I wouldn't handle it then if it is not used

nazar-pc · 2023-08-07T12:48:42Z

crates/subspace-networking/src/node_runner.rs

+        if event.last_address {
+            self.swarm
+                .behaviour_mut()
+                .kademlia
+                .remove_peer(&event.peer_id);
+        } else {
+            self.swarm
+                .behaviour_mut()
+                .kademlia
+                .remove_address(&event.peer_id, &event.address);
+        }


Okay, that makes sense and I think we should then replace handle_removed_address_event with handle_removed_peer, but even then it seems problematic that we may not remember all the peers in Kademlia anyway, thus unable to remove them. Should known peers be unbounded in memory then and bounded when written to disk (by unbounded I mean bounded by Kademlia externally)?

# Conflicts: # crates/subspace-networking/src/behavior/tests.rs # crates/subspace-networking/src/bin/subspace-bootstrap-node/main.rs # crates/subspace-networking/src/create.rs

nazar-pc · 2023-08-08T08:58:16Z

crates/subspace-networking/src/node_runner.rs

+        if event.last_address {
+            self.swarm
+                .behaviour_mut()
+                .kademlia
+                .remove_peer(&event.peer_id);
+        } else {
+            self.swarm
+                .behaviour_mut()
+                .kademlia
+                .remove_address(&event.peer_id, &event.address);
+        }


Not dynamically, but if we have fixed Kademlia parameters and we know upper bound then yes. I agree this PR is an improvement, I'm just trying to understand whether it is a 100% solution to the problem.

nazar-pc · 2023-08-08T09:00:05Z

crates/subspace-networking/src/node_runner.rs

@@ -397,10 +398,16 @@ where
                .kademlia
                .remove_peer(&event.peer_id);
        } else {
+            // Remove both versions of the address


But we do not add our own address to our Kademlia, do we? I don't think I follow you here.

shamil-gadelshin added 3 commits August 4, 2023 20:42

networking: Add an event for peer address removal.

43cfbc3

Add bootstrap node filter on address persistence.

96c498f

networking: Remove both versions of address from DHT

b14d4bc

shamil-gadelshin added the networking Subspace networking (DSN) label Aug 4, 2023

shamil-gadelshin self-assigned this Aug 4, 2023

shamil-gadelshin requested review from nazar-pc and rg3l3dr as code owners August 4, 2023 13:46

nazar-pc reviewed Aug 6, 2023

View reviewed changes

networking: Refactor persistent parameters manager.

6627fb3

nazar-pc reviewed Aug 7, 2023

View reviewed changes

shamil-gadelshin added 2 commits August 8, 2023 14:56

networking: Refactor persistent parameters manager.

619e669

Merge branch 'main' into remove-address-from-kademlia

6af3530

# Conflicts: # crates/subspace-networking/src/behavior/tests.rs # crates/subspace-networking/src/bin/subspace-bootstrap-node/main.rs # crates/subspace-networking/src/create.rs

shamil-gadelshin requested a review from nazar-pc August 8, 2023 08:28

networking: Apply review suggestions.

2060c47

nazar-pc approved these changes Aug 8, 2023

View reviewed changes

shamil-gadelshin enabled auto-merge August 8, 2023 09:03

nazar-pc disabled auto-merge August 8, 2023 11:46

nazar-pc merged commit 1e781eb into main Aug 8, 2023

nazar-pc deleted the remove-address-from-kademlia branch August 8, 2023 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update persistent networking parameters manager. #1762

Update persistent networking parameters manager. #1762

shamil-gadelshin commented Aug 4, 2023

nazar-pc Aug 5, 2023

shamil-gadelshin Aug 7, 2023

nazar-pc Aug 7, 2023

shamil-gadelshin Aug 8, 2023

nazar-pc Aug 8, 2023

shamil-gadelshin Aug 8, 2023

nazar-pc Aug 5, 2023

shamil-gadelshin Aug 7, 2023

nazar-pc Aug 5, 2023

shamil-gadelshin Aug 7, 2023

nazar-pc Aug 7, 2023

shamil-gadelshin Aug 8, 2023

nazar-pc Aug 8, 2023

shamil-gadelshin Aug 8, 2023

nazar-pc Aug 8, 2023

nazar-pc Aug 6, 2023

shamil-gadelshin Aug 7, 2023

nazar-pc Aug 7, 2023

shamil-gadelshin Aug 8, 2023

nazar-pc Aug 8, 2023

shamil-gadelshin Aug 8, 2023

nazar-pc Aug 8, 2023

shamil-gadelshin Aug 8, 2023 •

edited

Loading

nazar-pc Aug 7, 2023

nazar-pc Aug 7, 2023

nazar-pc Aug 7, 2023

nazar-pc Aug 8, 2023

nazar-pc Aug 8, 2023

Update persistent networking parameters manager. #1762

Update persistent networking parameters manager. #1762

Conversation

shamil-gadelshin commented Aug 4, 2023

Code contributor checklist:

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamil-gadelshin Aug 8, 2023 • 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

Choose a reason for hiding this comment

shamil-gadelshin Aug 8, 2023 •

edited

Loading