Introduce "default init" and "unload" methods for Ipv8Component #6402

drew2a · 2021-10-01T12:32:33Z

Based on #6396

class Ipv8Component(RestfulComponent):
    ipv8: IPv8

    def initialise_community_by_default(self, community):
        community.bootstrappers.append(self.make_bootstrapper())

        # Value of `target_peers` must not be equal to the value of `max_peers` for this community.
        # This causes a deformed network topology and makes it harder for peers to connect to others.
        # More information: https://github.com/Tribler/py-ipv8/issues/979#issuecomment-896643760
        random_walk_max_peers = max(10, community.max_peers - 10)
        self.ipv8.add_strategy(community, RandomWalk(community), random_walk_max_peers)

    async def unload_community(self, community):
        await self.ipv8.unload_overlay(community)

qstokkink

Nice 👍

In my opinion this is good to merge as-is. However, I would urge you to consider implementing my one suggestion already in this PR.

qstokkink · 2021-10-02T07:02:21Z

src/tribler-core/tribler_core/components/ipv8.py

        ipv8 = self.ipv8
        community = DiscoveryCommunity(self.peer, ipv8.endpoint, ipv8.network, max_peers=100)
+        community.bootstrappers.append(self.make_bootstrapper())


nit: seems like you can replace this with self.initialise_community_by_default(community) and also shed the ipv8.add_strategy(community, RandomWalk(community), 20).
Same for _init_dht_discovery_community. Loses 2 LOC overall.

Agree. Is 20 target peers is necessary for the RandomWalk strategy for DiscoveryCommunity?

Why I asking is because if I replace it by self.initialise_community_by_default(community), then target_peers become max_peers-10 == 90

Short answer: yes, 20 is a good idea.

Long answer: The DiscoveryCommunity is a meta-community that all peers join to quickly synchronize the other Communities they are a part of. This also forms a secondary bootstrap mechanism (which is why you generally don't want to deviate from the default parameterization of your Community and use a custom Network()). Actively walking to some peers in the DiscoveryCommunity will allow you to get a little bit of diversity in your connections. However, walking to too many peers here will just give you a lot of peers you do not share Communities with. 20 seems to be a good number (also considering the network topologies of the other Communities this Community shares its peer pool with). You could even set this to 0. However, 90 may produce too many open connections if overlay identifiers start diverging in the network.

Got it. What if we will rewrite initialise_community_by_default this way:

def initialise_community_by_default(self, community, default_random_walk_max_peers=20): community.bootstrappers.append(self.make_bootstrapper()) # Value of `target_peers` must not be equal to the value of `max_peers` for the community. # This causes a deformed network topology and makes it harder for peers to connect to others. # More information: https://github.com/Tribler/py-ipv8/issues/979#issuecomment-896643760 # # Then: # random_walk_max_peers should be less than community.max_peers: random_walk_max_peers = min(default_random_walk_max_peers, community.max_peers - 10) # random_walk_max_peers should be greater than 0 random_walk_max_peers = max(0, random_walk_max_peers) self.ipv8.add_strategy(community, RandomWalk(community), random_walk_max_peers)

Seems good to me 👍

Implemented

qstokkink · 2021-10-02T07:06:11Z

src/tribler-core/tribler_core/components/bandwidth_accounting/bandwidth_accounting_component.py

+        database = BandwidthDatabase(database_path, self._ipv8_component.peer.public_key.key_to_bin())
+        self.community = bandwidth_cls(self._ipv8_component.peer,
+                                       self._ipv8_component.ipv8.endpoint,
+                                       self._ipv8_component.ipv8.network,


thought/nit: We're injecting self._ipv8_component.ipv8.network as a dependency and then the component itself is overwriting its input with a Network() of its own creation. We should, at some point (probably not this PR) move the Network() overwrite to this file (same for PopularityCommunity).

Yes, probably. For me purpose of overwriting a Network is unclear.
It is true for PopularityCommunity and GigachannelCommunity (as successors of RemoteQueryCommunity)
cc: @ichorid

(I see it as out of the scope of this PR)

GigaChannelCommunity walks and deletes peers aggressively, so creates a separate Network instance to prevent interfering with other communities.

The reason for a "clean" Network() is the introduction of bias through custom parameterization of Communities. If you bias your peer discovery mechanism, you get a self-reinforcing effect that essentially forces some peers out of your network. It is not a problem if one peer deviates, but if you force a default on the entire network you will start seeing this. We've learned this the hard way #2894 (comment) (yes, we sinkhole attacked our own network) Therefore, for every Community that uses custom walks (including peer removal) or custom parameterization, we use a new Network.

Implemented here: #6408

qstokkink · 2021-10-02T07:20:54Z

src/tribler-core/tribler_core/components/tests/test_tribler_components.py

+    # test dht disabled
+    tribler_config.dht.enabled = True
+    session = Session(tribler_config, [MasterKeyComponent(), RESTComponent(), Ipv8Component()])
+    with session:
+        await session.start()
+
+        comp = Ipv8Component.instance()
+        assert comp.dht_discovery_community
+
+    # test discovery_community enabled
+    tribler_config.gui_test_mode = False
+    tribler_config.discovery_community.enabled = True
+    session = Session(tribler_config, [MasterKeyComponent(), RESTComponent(), Ipv8Component()])
+    with session:
+        await session.start()
+
+        comp = Ipv8Component.instance()
+        assert comp._peer_discovery_community


suggestion: This test now breaks with the Arrange/Act/Assert pattern: what I see here is Arrange/Assert/Arrange/Assert/Arrange/Assert. By splitting this code at the comments (turning # test dht disabled into async def test_ipv8_component_dht_disabled() and # test discovery_community enabled into async def test_ipv8_component_discovery_enabled()) you can follow the AAA pattern with very little effort.

Agree! Will implement it soon.

Implemented

sonarqubecloud · 2021-10-05T09:29:14Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

qstokkink

praise: Thanks for going out of your way to even implement the nit and optional suggestion. Looks good 👍

drew2a force-pushed the fix/ipv8_community_bootstrapper branch 2 times, most recently from 4f05665 to fd8cf86 Compare October 1, 2021 12:52

drew2a changed the title ~~Introduce default init and unload methods~~ Introduce "default init" and "unload" methods for Ipv8Component Oct 1, 2021

drew2a force-pushed the fix/ipv8_community_bootstrapper branch 2 times, most recently from bc9b0b8 to 99beb20 Compare October 1, 2021 18:25

drew2a marked this pull request as ready for review October 1, 2021 18:37

drew2a requested review from a team, kozlovsky and qstokkink and removed request for a team and kozlovsky October 1, 2021 18:37

qstokkink previously approved these changes Oct 2, 2021

View reviewed changes

drew2a added 2 commits October 5, 2021 11:27

Introduce init and unload methods in ipv8_component

9658a23

PR fixes

8daf601

drew2a dismissed qstokkink’s stale review via 8daf601 October 5, 2021 09:27

drew2a force-pushed the fix/ipv8_community_bootstrapper branch from 99beb20 to 8daf601 Compare October 5, 2021 09:27

qstokkink approved these changes Oct 5, 2021

View reviewed changes

drew2a merged commit 8263c60 into Tribler:main Oct 5, 2021

drew2a deleted the fix/ipv8_community_bootstrapper branch October 5, 2021 10:08

drew2a mentioned this pull request Oct 5, 2021

Adding tag community, database and REST API endpoints #6396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce "default init" and "unload" methods for Ipv8Component #6402

Introduce "default init" and "unload" methods for Ipv8Component #6402

drew2a commented Oct 1, 2021

qstokkink left a comment

qstokkink Oct 2, 2021

drew2a Oct 4, 2021

qstokkink Oct 4, 2021

drew2a Oct 4, 2021

qstokkink Oct 5, 2021

drew2a Oct 5, 2021

qstokkink Oct 2, 2021

drew2a Oct 4, 2021

ichorid Oct 4, 2021

qstokkink Oct 4, 2021 •

edited

Loading

drew2a Oct 5, 2021

qstokkink Oct 2, 2021

drew2a Oct 4, 2021

drew2a Oct 5, 2021

sonarqubecloud bot commented Oct 5, 2021

qstokkink left a comment •

edited

Loading

Introduce "default init" and "unload" methods for Ipv8Component #6402

Introduce "default init" and "unload" methods for Ipv8Component #6402

Conversation

drew2a commented Oct 1, 2021

qstokkink 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

Choose a reason for hiding this comment

qstokkink Oct 4, 2021 • 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

sonarqubecloud bot commented Oct 5, 2021

qstokkink left a comment • edited Loading

Choose a reason for hiding this comment

qstokkink Oct 4, 2021 •

edited

Loading

qstokkink left a comment •

edited

Loading