Refactor and cleanup gossipsub #373

NIC619 · 2019-12-03T08:12:56Z

How was it fixed?

Mainly refactoring the Gossipsub heartbeat.

some refactors, remove unnecessary check and peer selection filter
don't send the control messages at once but instead group the messages for the same peer and send it at the end of the heartbeat
add tests for individual mesh_heartbeat and gossip_heartbeat

And some other bug fixes and remove outdated TODOs.

Cute Animal Picture

(source: https://www.maxpixels.net/Mammals-Cute-Pet-Dog-Animal-Kingdom-Portrait-3071189)

NIC619

Leave some notes regarding the TODOs removed.

NIC619 · 2019-12-03T10:04:16Z

libp2p/pubsub/gossipsub.py

@@ -106,7 +109,6 @@ def attach(self, pubsub: Pubsub) -> None:
        logger.debug("attached to pusub")

        # Start heartbeat now that we have a pubsub instance
-        # TODO: Start after delay


This is fixed by adding heartbeat_initial_delay.

NIC619 · 2019-12-03T10:05:17Z

libp2p/pubsub/gossipsub.py

@@ -127,8 +129,9 @@ def add_peer(self, peer_id: ID, protocol_id: TProtocol) -> None:
            #   instance in multistream-select, but it is not the protocol that gossipsub supports.
            #   In this case, probably we registered gossipsub to a wrong `protocol_id`
            #   in multistream-select, or wrong versions.
-            # TODO: Better handling


Not much to be handled here. This code base should not be reached.

NIC619 · 2019-12-03T10:10:38Z

libp2p/pubsub/gossipsub.py

@@ -218,15 +235,9 @@ def _get_peers_to_send(

            # gossipsub peers
            in_topic_gossipsub_peers: List[ID] = None
-            # TODO: Do we need to check `topic in self.pubsub.my_topics`?


Checked in L226

NIC619 · 2019-12-03T10:15:47Z

libp2p/pubsub/gossipsub.py

        # Note: the comments here are the exact pseudocode from the spec
        for topic in self.fanout:
-            # If time since last published > ttl
+            # Delete topic entry if it's not in `pubsub.peer_topics`
+            # or if it's time-since-last-published > ttl
            # TODO: there's no way time_since_last_publish gets set anywhere yet


This will be handled in another PR.

NIC619 · 2019-12-03T10:16:21Z

libp2p/pubsub/gossipsub.py

        for topic in self.mesh:
            msg_ids = self.mcache.window(topic)
            if msg_ids:
-                # TODO: Make more efficient, possibly using a generator?


Seems irrelevant now IMO.

NIC619 · 2019-12-03T10:16:57Z

libp2p/pubsub/pubsub.py

@@ -78,7 +78,6 @@ class Pubsub:

    topic_validators: Dict[str, TopicValidator]

-    # TODO: Be sure it is increased atomically everytime.


Seems to be the case.

NIC619 · 2019-12-03T10:17:29Z

libp2p/pubsub/pubsub.py

@@ -300,7 +299,6 @@ def get_msg_validators(self, msg: rpc_pb2.Message) -> Tuple[TopicValidator, ...]
            logger.debug("Fail to add new peer %s: stream closed", peer_id)
            del self.peers[peer_id]
            return
-        # TODO: Check EOF of this stream.


It's checked right above.

NIC619 · 2019-12-03T10:19:02Z

libp2p/pubsub/pubsub.py

-        """
-        Continuously read from peer queue and each time a new peer is found,
-        open a stream to the peer using a supported pubsub protocol
-        TODO: Handle failure for when the peer does not support any of the


Related to the unreachable code path mentioned above.

NIC619 · 2019-12-04T05:12:28Z

This should be ready for review. Tasks like heartbeat silently crashed have make debugging the tests fairly harder but this problem should be fixed in #365 once we run each part as a service so that crashed child task would terminate the service as well.

libp2p/pubsub/gossipsub.py

tests/pubsub/test_gossipsub.py

Co-Authored-By: Chih Cheng Liang <chihchengliang@gmail.com>

ralexstokes

kept this pretty high-level but overall looks good!

mhchia · 2019-12-05T15:39:09Z

libp2p/pubsub/gossipsub.py

+        for topic in self.mesh:
+            if peer_id in self.mesh[topic]:
+                # Delete the entry if no other peers left
+                self.mesh[topic].remove(peer_id)


Note: I'd think we should change Lists to Set for better performance some time.

libp2p/pubsub/gossipsub.py

mhchia · 2019-12-06T07:55:31Z

libp2p/pubsub/gossipsub.py

+        ihave_msgs: List[rpc_pb2.ControlIHave],
+        graft_msgs: List[rpc_pb2.ControlGraft],
+        prune_msgs: List[rpc_pb2.ControlPrune],
+    ) -> rpc_pb2.ControlMessage:


Am I understanding it correctly that this change is for piggybacking?

I would say it's just packing the messages together and send them at once. Will open an issue for piggybacking.

use defaultdict and init control message

mhchia

LGTM! Nice work.

Co-Authored-By: Kevin Mai-Husan Chia <mhchia@users.noreply.github.com>

mhchia · 2019-12-07T08:17:15Z

libp2p/pubsub/gossipsub.py

+                ):
+                    if topic in self.pubsub.peer_topics:
+                        # Combine fanout peers with selected peers
+                        fanout_peers += self._get_in_topic_gossipsub_peers_from_minus(


Note: one thing I'm thinking is to have methods like _get_fanout_peers(topic), _get_mesh_peers(topic), and _get_ peers_to_emit_ihave_to, to make the code more readable.

moshemalawach · 2020-05-14T14:23:21Z

Very nice! I used gossipsub in my implementation before... TL;DR, what should I change?

ralexstokes · 2020-05-14T14:34:36Z

@moshemalawach i'm not sure a user of the code in this PR will need to update anything beyond any API changes introduced.

note that we will be migrating to gossipsub v1.1 over the next few months -- v1.1 is backwards compatible w/ v1.0 but does add a variety of "hardening" features to make the overall mesh more secure.

NIC619 added 12 commits November 30, 2019 17:12

Cleanup TODOs in pubsub

e59ac6a

Del entry if no more peers subscribe to the topic

0a52a05

Cleanup outdated TODOs in gossipsub

50fd0ac

Fix: move heartbeat delay to heartbeat

0672f5a

Add gossipsub heartbeat_initial_delay

c2d8896

Remove unneccessary filter and check in gossipsub

357341e

Add missing cleanup in gossipsub remove_peer

a7e0c5d

Fix lint and add check in fanout heartbeat

920cf64

Remove unneccessary check in gossip heartbeat

ab1500c

Group messages for peer in heartbeat

5efdf4c

Add test for mesh heartbeat

8dec0b1

Add test for gossip heartbeat

b405fd7

NIC619 requested review from ralexstokes and mhchia December 3, 2019 08:12

Extend wait time for test to pass

60bd469

NIC619 commented Dec 3, 2019

View reviewed changes

NIC619 added 5 commits December 3, 2019 18:45

Add back some comment and TODO. Add comment to tests

ea6cd30

Fix var access before assignment

bb15c81

Fix list deletion and add list remove check

a9abf1e

Fix: should not remove topic if no peers

c08b237

Update the sleep time in test_handle_prune

8e59122

NIC619 force-pushed the refactor_and_cleanup_gossipsub branch from fdcd481 to 8e59122 Compare December 3, 2019 15:11

NIC619 requested a review from ChihChengLiang December 4, 2019 04:57

ChihChengLiang reviewed Dec 4, 2019

View reviewed changes

libp2p/pubsub/gossipsub.py Outdated Show resolved Hide resolved

tests/pubsub/test_gossipsub.py Outdated Show resolved Hide resolved

tests/pubsub/test_gossipsub.py Outdated Show resolved Hide resolved

NIC619 added 3 commits December 5, 2019 14:35

Refactor _get_peers_to_send

e6813da

Apply PR feedback: correct the comment in test

fae3798

Remove unnecessary check and fix test

67f02c5

NIC619 force-pushed the refactor_and_cleanup_gossipsub branch from ca0ce52 to 67f02c5 Compare December 5, 2019 07:10

Apply suggestions from code review

b4900d5

Co-Authored-By: Chih Cheng Liang <chihchengliang@gmail.com>

Fix lint after applying suggestion

db0017d

ralexstokes approved these changes Dec 5, 2019

View reviewed changes

mhchia reviewed Dec 6, 2019

View reviewed changes

Apply PR feedback:

2d3bfc8

use defaultdict and init control message

mhchia approved these changes Dec 6, 2019

View reviewed changes

Update libp2p/pubsub/gossipsub.py

a675da5

Co-Authored-By: Kevin Mai-Husan Chia <mhchia@users.noreply.github.com>

mhchia reviewed Dec 7, 2019

View reviewed changes

NIC619 merged commit ad37922 into libp2p:master Dec 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and cleanup gossipsub #373

Refactor and cleanup gossipsub #373

NIC619 commented Dec 3, 2019 •

edited

Loading

NIC619 left a comment

NIC619 Dec 3, 2019

NIC619 Dec 3, 2019

NIC619 Dec 3, 2019

NIC619 Dec 3, 2019

NIC619 Dec 3, 2019

NIC619 Dec 3, 2019

NIC619 Dec 3, 2019

NIC619 Dec 3, 2019

NIC619 commented Dec 4, 2019

ralexstokes left a comment

mhchia Dec 5, 2019

mhchia Dec 6, 2019

NIC619 Dec 7, 2019

mhchia left a comment

mhchia Dec 7, 2019

moshemalawach commented May 14, 2020

ralexstokes commented May 14, 2020

		@@ -78,7 +78,6 @@ class Pubsub:

		topic_validators: Dict[str, TopicValidator]

		# TODO: Be sure it is increased atomically everytime.

Refactor and cleanup gossipsub #373

Refactor and cleanup gossipsub #373

Conversation

NIC619 commented Dec 3, 2019 • edited Loading

How was it fixed?

Cute Animal Picture

NIC619 left a 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

NIC619 commented Dec 4, 2019

ralexstokes left a 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

mhchia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moshemalawach commented May 14, 2020

ralexstokes commented May 14, 2020

NIC619 commented Dec 3, 2019 •

edited

Loading