Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove BandwidthCommunity #7798

Merged
merged 1 commit into from
Jan 5, 2024
Merged

Remove BandwidthCommunity #7798

merged 1 commit into from
Jan 5, 2024

Conversation

drew2a
Copy link
Contributor

@drew2a drew2a commented Jan 4, 2024

This PR removes the BandwidthCommunity and closes #7336. It also contains a migration for removing bandwidth.db files.

I've decided to leave the BandwidthCrawler as an example of a simple crawler for Tribler, for the next generation of developers or PhD students.

PR / scripttest_draft / run (pull_request_target) is failing because, due to security reasons, the content of YML files is always taken from the target branch. Therefore, the checks for the current PR use the old (unchanged) scripttest.yml. After merging, this check should be correct.

Closes #5212
Closes #4939
Closes #4255
Closes #3731

@drew2a drew2a force-pushed the feature/7336 branch 2 times, most recently from 309f475 to 23803f1 Compare January 5, 2024 11:20
@drew2a drew2a requested a review from egbertbouman January 5, 2024 11:21
@drew2a drew2a marked this pull request as ready for review January 5, 2024 11:21
@drew2a drew2a requested a review from a team January 5, 2024 11:21
@drew2a
Copy link
Contributor Author

drew2a commented Jan 5, 2024

@egbertbouman, I'm requesting your review as it turns out that the BandwidthCommunity was intertwined with the TunnelCommunity. I've tried to be as careful as possible, but it's hard to grasp all the logic from TunnelCommunity and verify on my own that my changes are correct. However, all tests have passed, and anonymous downloading is working (I've tried it both on new downloads and existing ones).

Copy link
Member

@egbertbouman egbertbouman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. A couple of things to note:

  • BalanceRequestCache can be removed, as well as the cache check in should_join_circuit.
  • Since messages/cells have been removed, but old peers are still sending these messages, I'm guessing there is a lot of log spam now.
  • While the network updates to the new code, there will be a bias toward people that are running the old code (e.g. new peers only compete for random slots, whereas old peers compete for all slots). Not really a problem anyway.

@drew2a
Copy link
Contributor Author

drew2a commented Jan 5, 2024

I've removed BalanceRequestCache and the cache check in should_join_circuit: d6edc77 (I'll swap the commits before merge).

Since messages/cells have been removed, but old peers are still sending these messages, I'm guessing there is a lot of log spam now.

Yes. But as we've seen that the network updates to the new version quite quickly (it takes a couple of weeks), then it is probably worth it to wait without any temporary warning suppression.

While the network updates to the new code, there will be a bias toward people that are running the old code (e.g. new peers only compete for random slots, whereas old peers compete for all slots). Not really a problem anyway.

Agreed. Also, I'll note here just in case, that I've increased the random slot count from 5 to 20 as it seems fair to keep the same amount of slots as before (random_slots (5) + competing_slots (15)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants