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

feat: expire messages from the cache based on last seen time #513

Merged
merged 6 commits into from
Jan 24, 2023

Conversation

smrz2001
Copy link
Contributor

@smrz2001 smrz2001 commented Jan 6, 2023

This PR attempts to fix #502.

It also includes a thread-safe copy of the original time cache implementation.

Both the "first seen" (original) and "last seen" (new) implementations have been abstracted behind the TimeCache interface in a new timecache package.

cc @vyzo

blacklist.go Show resolved Hide resolved
go.mod Show resolved Hide resolved
pubsub.go Outdated Show resolved Hide resolved
@vyzo vyzo self-requested a review January 10, 2023 13:52
@vyzo
Copy link
Collaborator

vyzo commented Jan 10, 2023

thanks, i was on vacation. I will take a look soon.

@BigLep
Copy link

BigLep commented Jan 12, 2023

@vyzo : after this is reviewed/merged, can you cut a new release? (I see this repo isn't part of unified ci: https://github.com/protocol/.github/blob/master/configs/go.json). I'm asking so it can be bubbled up to Kubo for https://github.com/ipfs/kubo/pull/9372/files#diff-a75543978c37023721256291000855b02598e5d72668e203f7372cc4b8c159cfR222

@vyzo
Copy link
Collaborator

vyzo commented Jan 12, 2023

sure

@BigLep
Copy link

BigLep commented Jan 13, 2023

@vyzo : I know you're juggling a lot. Would it be helpful if someone from the EngRes libp2p Stewards team takes a look?

lidel added a commit to smrz2001/kubo that referenced this pull request Jan 13, 2023
@vyzo
Copy link
Collaborator

vyzo commented Jan 13, 2023

It has been a little hectic in fvm land recently, but i do have some time to look at this during the weekend.

@vyzo
Copy link
Collaborator

vyzo commented Jan 13, 2023

also, feel free to add steward reviewers, it would help disseminate knowledge.

lidel
lidel previously requested changes Jan 16, 2023
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@vyzo I eyeballed it, but any chance you take a look at this week?

@smrz2001 quick ask: unify default strategy (right now it is different https://github.com/libp2p/go-libp2p-pubsub/pull/513/files#r1071344615)

@vyzo
Copy link
Collaborator

vyzo commented Jan 16, 2023

yes, yes, my weekend plan didnt materialize, but i have it on the list.

@smrz2001
Copy link
Contributor Author

@vyzo I eyeballed it, but any chance you take a look at this week?

@smrz2001 quick ask: unify default strategy (right now it is different https://github.com/libp2p/go-libp2p-pubsub/pull/513/files#r1071344615)

Fixed! Thanks, @lidel.

smrz2001 added a commit to smrz2001/kubo that referenced this pull request Jan 16, 2023
@BigLep
Copy link

BigLep commented Jan 20, 2023

@MarcoPolo @marten-seemann @Wondertan : any chance you can please help take a look at this? I'd like to get this over the line to get @smrz2001 and team unblocked.

@Wondertan
Copy link
Contributor

Happy to review this on the weekends if it will be relevant until then.

Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

A couple of issues to decide and a blocker: please dont change the default behaviour!

timecache/first_seen_cache.go Show resolved Hide resolved
timecache/last_seen_cache.go Show resolved Hide resolved
timecache/last_seen_cache.go Show resolved Hide resolved
@vyzo
Copy link
Collaborator

vyzo commented Jan 23, 2023

Please retain the default behaviour of first seen.

@oed
Copy link

oed commented Jan 23, 2023

Please retain the default behaviour of first seen.

@vyzo you said previously (#502 (comment)) that it should be changed. This was the point of the bug that was reported.

@vyzo
Copy link
Collaborator

vyzo commented Jan 23, 2023

Yes, i know, but i had achange of heart regarding the default behaviour.

It is true that it is desirable to use the last seen behaviour for many applications.

However, some of the biggest users of this library are blockchains which can quite easily post reject and deduplicate messages once the timecache period has expired, because of consensus.
That is, after two minutes almost all blockchains can reject older messages reappearing and replays with their consensus based validators (nonce, block height, and so on).

For these applications, changing the default under their feet will not bring any tangible benefit for the increased memory usage.
At the very least i think we should be conservative and let the user choose.

Note that we can also consider making last seen the default a bit further down the line, say in a release or two.
Also, maturity of implementation concerns.

Does this make sense?

@oed
Copy link

oed commented Jan 23, 2023

It does make sense. Thanks for explaining your reasoning @vyzo

Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

ok, lets merge it and follow up for background gc.

@vyzo vyzo requested a review from lidel January 23, 2023 20:50
@vyzo
Copy link
Collaborator

vyzo commented Jan 23, 2023

@lidel you are blocking merge.

@lidel lidel dismissed their stale review January 23, 2023 21:19

unblocking

smrz2001 added a commit to smrz2001/kubo that referenced this pull request Jan 23, 2023
libp2p/go-libp2p-pubsub#513
use last seen time cache implementation
@smrz2001
Copy link
Contributor Author

ok, lets merge it and follow up for background gc.

Great, thanks so much @vyzo and @lidel!

I opened #515 for the GC follow-up discussion.

@vyzo vyzo merged commit 973fef5 into libp2p:master Jan 24, 2023
@smrz2001 smrz2001 deleted the feature/res-849-writing-code-for-libp2p branch January 24, 2023 00:25
lidel added a commit to ipfs/kubo that referenced this pull request Jan 26, 2023
* feat: expire messages from the cache based on last seen time
* docs: Pubsub.SeenMessagesStrategy

Ref. libp2p/go-libp2p-pubsub#513

Co-authored-by: Marcin Rataj <lidel@lidel.org>
galargh pushed a commit to ipfs/kubo that referenced this pull request Jan 30, 2023
* feat: expire messages from the cache based on last seen time
* docs: Pubsub.SeenMessagesStrategy

Ref. libp2p/go-libp2p-pubsub#513

Co-authored-by: Marcin Rataj <lidel@lidel.org>
hannahhoward pushed a commit to filecoin-project/kubo-api-client that referenced this pull request Jun 19, 2023
* feat: expire messages from the cache based on last seen time
* docs: Pubsub.SeenMessagesStrategy

Ref. libp2p/go-libp2p-pubsub#513

Co-authored-by: Marcin Rataj <lidel@lidel.org>
hannahhoward pushed a commit to filecoin-project/kubo-api-client that referenced this pull request Jun 19, 2023
* feat: expire messages from the cache based on last seen time
* docs: Pubsub.SeenMessagesStrategy

Ref. libp2p/go-libp2p-pubsub#513

Co-authored-by: Marcin Rataj <lidel@lidel.org>
yhassanzadeh13 added a commit to yhassanzadeh13/go-libp2p-pubsub that referenced this pull request Jul 3, 2023
* feat: expire messages from the cache based on last seen time (libp2p#513)

* feat: expire messages from the cache based on last seen time

* chore: minor renaming

* fix: messages should not be found after expiration

* chore: editorial

* fix: use new time cache strategy consistently

* fix: default to old time cache and add todo for background gc

* chore: update to go-libp2p v0.25 (libp2p#517)

* Update to go-libp2p v0.25

* Use go 1.19

* chore: update go version and dependencies (libp2p#516)

* fix(timecache): remove panic in first seen cache on Add (libp2p#522)

* Refactor timecache implementations (libp2p#523)

* reimplement timecache for sane and performant behaviour

* remove seenMessagesMx, take advantage of new tc api

* fix timecache tests

* fix typo

* store expiry, don't make life difficult

* refactor common background sweep procedure for both impls

* add godocs to TimeCache

* Default validator support (libp2p#525)

* add default validator support

* add an implementation for basic seqno as nonce validation

* missing return

* the nonce belongs to the origin peer

* add note about rust predicament

* add seqno validator tests

* minor test tweak, ensure at least 1ms before replay

* Fix Memory Leak In New Timecache Implementations (libp2p#528)

* fix bug

* add for last seen cache

* chore: Update .github/workflows/stale.yml [skip ci]

* chore: Update .github/workflows/stale.yml [skip ci]

* upgrades libp2p version

* upgrades libp2p version

---------

Co-authored-by: Mohsin Zaidi <2236875+smrz2001@users.noreply.github.com>
Co-authored-by: Marco Munizaga <git@marcopolo.io>
Co-authored-by: RichΛrd <info@richardramos.me>
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
Co-authored-by: vyzo <vyzo@hackzen.org>
Co-authored-by: Nishant Das <nishdas93@gmail.com>
Co-authored-by: GitHub <noreply@github.com>
yhassanzadeh13 added a commit to yhassanzadeh13/go-libp2p-pubsub that referenced this pull request Feb 20, 2024
* feat: expire messages from the cache based on last seen time (libp2p#513)

* feat: expire messages from the cache based on last seen time

* chore: minor renaming

* fix: messages should not be found after expiration

* chore: editorial

* fix: use new time cache strategy consistently

* fix: default to old time cache and add todo for background gc

* chore: update to go-libp2p v0.25 (libp2p#517)

* Update to go-libp2p v0.25

* Use go 1.19

* chore: update go version and dependencies (libp2p#516)

* fix(timecache): remove panic in first seen cache on Add (libp2p#522)

* Refactor timecache implementations (libp2p#523)

* reimplement timecache for sane and performant behaviour

* remove seenMessagesMx, take advantage of new tc api

* fix timecache tests

* fix typo

* store expiry, don't make life difficult

* refactor common background sweep procedure for both impls

* add godocs to TimeCache

* Default validator support (libp2p#525)

* add default validator support

* add an implementation for basic seqno as nonce validation

* missing return

* the nonce belongs to the origin peer

* add note about rust predicament

* add seqno validator tests

* minor test tweak, ensure at least 1ms before replay

* Fix Memory Leak In New Timecache Implementations (libp2p#528)

* fix bug

* add for last seen cache

* chore: Update .github/workflows/stale.yml [skip ci]

* chore: Update .github/workflows/stale.yml [skip ci]

* bump golang.org/x/net from 0.4.0 to 0.7.0 (libp2p#520)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.4.0 to 0.7.0.
- [Release notes](https://github.com/golang/net/releases)
- [Commits](golang/net@v0.4.0...v0.7.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: topicscore params can't be set for dynamically subscribed topic (libp2p#540)

* fix: topicscore params can't be set for a topic subscribed after gossipsub is initialized

* chore:address review comments

* Revert "fix: topicscore params can't be set for dynamically subscribed topic (libp2p#540)" (libp2p#541)

This reverts commit aa5fd79.

* remove usage of deprecated peerid.Pretty method (libp2p#542)

* chore: update go-libp2p to v0.32 (libp2p#548)

* chore: Update .github/workflows/stale.yml [skip ci]

* make tidy

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Mohsin Zaidi <2236875+smrz2001@users.noreply.github.com>
Co-authored-by: Marco Munizaga <git@marcopolo.io>
Co-authored-by: RichΛrd <info@richardramos.me>
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
Co-authored-by: vyzo <vyzo@hackzen.org>
Co-authored-by: Nishant Das <nishdas93@gmail.com>
Co-authored-by: GitHub <noreply@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Prem Chaitanya Prathi <chaitanyaprem@gmail.com>
Co-authored-by: Sukun <sukunrt@gmail.com>
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.

seenMessages TTL should be based on last observed time, not first
6 participants