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

added : TimedCache module #518

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mystical-prog
Copy link
Contributor

@mystical-prog mystical-prog commented Feb 24, 2025

What was wrong?

There was a gap in Pubsub implementation wherein there was a simple LRU based cache, instead we now have a built-in timed_cache python module which will now handle the seen_messages cache in Pubsub and it's derived protocols.

ref: https://github.com/libp2p/go-libp2p-pubsub/tree/master/timecache

@seetadev
Copy link

seetadev commented Feb 24, 2025

@mystical-prog : Excellent, Vraj :) Thank you for sharing the pull request. Looking forward to integrating and testing it with py-libp2p soon. Wish to share some key features expected in the pull request for Gossipsub in py-libp2p as discussed in the specification meeting focused on Gossipsub across libp2p modules:

✅ Gossipsub Core Implementation – Implements the core message propagation and peer scoring mechanisms.
✅ Mesh and Gossip Peering – Establishes the mesh network topology for efficient message dissemination and applies gossip propagation for improved redundancy.
✅ Validation Hooks – Introduces message validation to filter spam and ensure network integrity.
✅ Interoperability – Aligns with libp2p’s Gossipsub specs, allowing seamless integration with other libp2p implementations.

@mystical-prog
Copy link
Contributor Author

Hey @seetadev I am creating a TODO list and will keep on updating it as I find gaps, currently I have found these two gaps and I am looking forward to close before Monday!

@seetadev
Copy link

seetadev commented Mar 1, 2025

@mystical-prog : That sounds like a great plan! 🚀 Thanks for proactively identifying and tracking these gaps. Keeping an updated TODO list will definitely help streamline the process and ensure we cover all the missing pieces efficiently.

I appreciate your initiative in closing these gaps before Monday—that will be super helpful in keeping the momentum going! Looking forward to seeing the updates!🔥

Hey @seetadev I am creating a TODO list and will keep on updating it as I find gaps, currently I have found these two gaps and I am looking forward to close before Monday!

@mystical-prog mystical-prog changed the title Upgrade Gossipsub to V1.1 added : TimedCache module Mar 3, 2025
@mystical-prog
Copy link
Contributor Author

@seetadev
I would like to change this PR to be dedicated for timed_cache module as it is a significant change, I will similarly open small and step-by-step PRs which will help us achieve the larger goal of upgrading Gossipsub to v1.1

@mystical-prog mystical-prog marked this pull request as ready for review March 4, 2025 15:04
@mystical-prog
Copy link
Contributor Author

hey @pacrob @seetadev
I believe this PR is ready to be reviewed, will open up more such smaller PRs for the larger goal of upgrading Gossipsub to v1.1

@seetadev seetadev requested review from seetadev and pacrob March 10, 2025 21:20
@seetadev
Copy link

@mystical-prog : Hi Vraj. This looks great. Kindly share a small demo video (upto 2-4 mins ) explaining the PR, integration with libp2p and also a walkthrough on the code. This will also help other devs to extend on your work.

I will wait for @pacrob 's and @dhuseby 's feedback. Will merge once I hear the feedback from them.

@seetadev seetadev requested a review from dhuseby March 10, 2025 21:24
@mystical-prog
Copy link
Contributor Author

Sure, I will try to share the video ASAP

@pacrob
Copy link
Member

pacrob commented Mar 12, 2025

This looks good, @mystical-prog, thanks! Just a thought:

TimedCache is not a module of libp2p (in the libp2p sense, not the python sense), right? It's more of a tool/utility that is used within other modules? What do you think about placing it within the libp2p/tools folder?

Also, I see you've tested the changes via floodsub and pubsub, but some direct testing of your expectations for both FirstSeenCache and LastSeenCache would be appropriate.

@@ -0,0 +1,51 @@
import threading
Copy link
Member

Choose a reason for hiding this comment

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

I think renaming the file to base_timed_cache.py fits better.

import time


class TimedCache:
Copy link
Member

@pacrob pacrob Mar 12, 2025

Choose a reason for hiding this comment

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

This would be a good place to use ABC, like:

from abc import ABC

class BaseTimedCache(ABC)

You can then decorate the add and has methods with @abstractmethod and you don't have to worry about raising anything, it's handled by the ABC.

@mystical-prog
Copy link
Contributor Author

Hey @pacrob

I will look into the changes suggested by you and get back to you ASAP

@mystical-prog
Copy link
Contributor Author

I have made the suggested changes @pacrob
Let me know if any further modifications need to be made

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

Successfully merging this pull request may close these issues.

3 participants