feat: Pubsub.SeenMessagesTTL #9372

smrz2001 · 2022-10-26T20:26:56Z

Here's a PR for this issue, @lidel.

go.mod

lidel

lgtm, but this needs tests – if we expose this setting, we must be sure it continues working across any future refactors (both Kubo and go-libp2p)

lidel · 2022-11-09T20:55:50Z

core/node/groups.go

@@ -77,6 +77,7 @@ func LibP2P(bcfg *BuildCfg, cfg *config.Config) fx.Option {
 		pubsubOptions = append(
 			pubsubOptions,
 			pubsub.WithMessageSigning(!cfg.Pubsub.DisableSigning),
+			pubsub.WithSeenMessagesTTL(cfg.Pubsub.SeenMessagesTTL.WithDefault(pubsub.TimeCacheDuration)),


This PR needs a basic regression test that ensures Pubsub.SeenMessagesTTL set by the user is respected.
Unsure what is the best way to test this, you'd need to set it too low and then observe duplicates?

@smrz2001 we have some prior art in test/sharness/t0320-pubsub.sh if that helps.

Hey @lidel, it took me a bit to get everything figured out and setup with sharness. After playing around with it a bit, I'm realizing that there isn't really a good way to induce duplicate pubsub messages. The API (rightly) doesn't allow setting a message ID explicitly.

func (t *Topic) Publish(ctx context.Context, data []byte, opts ...PubOpt) error { . . . return t.p.val.PushLocal(&Message{m, "", t.p.host.ID(), nil, pub.local}) }

The only option I can currently come up with is to use tc netem via command line or code to simulate network traffic duplication.

Does this seems like a viable approach? Not sure what the restrictions are around what tests are/aren't allowed to do. I've used tc netem before so I can figure this out, if acceptable.

Thank you for looking into this. I hoped there is an easier way, but I guess we've hit an edge case on this one.

We already have some helpers for sharness in ./test/dependencies, such as pollEndpoint, so that is a potential path forward (golang helper is a bit more future-proof, could be reused when we move away from the sharness). Using tc netem is also an option, if we can get it to work on CI.

@marten-seemann @vyzo is there a better / less painful way to test WithSeenMessagesTTL / simulate duplicated messages?

I'm thinking aloud here but I could also make a libp2p-sim tool under test/dependencies that connects to libp2p like a normal node but is capable of sending spurious messages. Might be useful in other contexts too perhaps.

probably not, we do go out of our way to supress them.

What do you think about having a libp2p-sim that implements it's own Publish and can send dups?

This and libp2p/go-libp2p-pubsub#502 both sound sensible 👍

@lidel, would it be possible to do a regular Golang unit test here instead of adding to sharness?

@lidel, after a lot of hair pulling, I was unable to set up a libp2p simulator but I was able to figure out how to write a Go test under test/integration that does the job, IMO. Would it work to keep the test there? Let me know what you think.

FYI, this test uncovered another pubsub/cache bug, which I will fix under this issue.

Thank you for doing due diligence on this, yes, your approach is acceptable: this is the best we can do given tools at hand, and it still acts as an early warning if go-libp2p APIs change, or a regression is introduced.

Thank you for driving this @smrz2001, and being patient with test work. I'm merging this, so we can include it in Kubo 0.18.0-rc1 (ETA second week of December, or beginning of January).

Excellent, thanks so much, @lidel!! 🙏🏼🙏🏼

I'll try to get the other bugs fixed soon as well, and will update the dependencies and tests accordingly.

docs/config.md

config/pubsub.go

lidel

Thank you for driving this @smrz2001, and being patient with test work.
I'm merging this, so we can include it in Kubo 0.18.0-rc1 (ETA second week of December, or beginning of January).

test/integration/pubsub_msg_seen_cache_test.go

lidel · 2022-11-28T21:52:09Z

core/node/groups.go

@@ -77,6 +77,7 @@ func LibP2P(bcfg *BuildCfg, cfg *config.Config) fx.Option {
 		pubsubOptions = append(
 			pubsubOptions,
 			pubsub.WithMessageSigning(!cfg.Pubsub.DisableSigning),
+			pubsub.WithSeenMessagesTTL(cfg.Pubsub.SeenMessagesTTL.WithDefault(pubsub.TimeCacheDuration)),


Thank you for doing due diligence on this, yes, your approach is acceptable: this is the best we can do given tools at hand, and it still acts as an early warning if go-libp2p APIs change, or a regression is introduced.

smrz2001 commented Oct 26, 2022

View reviewed changes

go.mod Show resolved Hide resolved

lidel self-assigned this Oct 26, 2022

lidel self-requested a review October 26, 2022 20:58

smrz2001 force-pushed the feat/seen-messages-ttl-config branch from 5a9aa6c to c74d8c0 Compare November 4, 2022 15:56

lidel force-pushed the feat/seen-messages-ttl-config branch from c74d8c0 to 27a4bd1 Compare November 9, 2022 19:36

lidel changed the title ~~feat: make pubsub seen messages cache ttl configurable~~ feat: Pubsub.SeenMessagesTTL Nov 9, 2022

lidel requested changes Nov 9, 2022

View reviewed changes

lidel reviewed Nov 9, 2022

View reviewed changes

docs/config.md Show resolved Hide resolved

lidel reviewed Nov 16, 2022

View reviewed changes

config/pubsub.go Outdated Show resolved Hide resolved

smrz2001 and others added 4 commits November 28, 2022 15:18

feat: make pubsub seen messages cache ttl configurable

186a314

docs(config): Pubsub.SeenMessagesTTL

7a22190

chore(docs): add more details for Pubsub.SeenMessagesTTL

77eead2

fix(test): add tests for Pubsub.SeenMessagesTTL

4b8eafa

smrz2001 force-pushed the feat/seen-messages-ttl-config branch from 3c73341 to 4b8eafa Compare November 28, 2022 20:19

go mod tidy

a62272f

lidel approved these changes Nov 28, 2022

View reviewed changes

chore: clarify pubsub tests

c97c89b

lidel enabled auto-merge (squash) November 28, 2022 22:03

lidel merged commit 51f60ea into ipfs:master Nov 28, 2022

smrz2001 deleted the feat/seen-messages-ttl-config branch November 28, 2022 23:51

smrz2001 mentioned this pull request Feb 27, 2023

Pubsub flood due to same message propagated multiple times #9665

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Pubsub.SeenMessagesTTL #9372

feat: Pubsub.SeenMessagesTTL #9372

smrz2001 commented Oct 26, 2022 •

edited by lidel

Loading

lidel left a comment •

edited

Loading

lidel Nov 9, 2022 •

edited

Loading

smrz2001 Nov 16, 2022

lidel Nov 16, 2022 •

edited

Loading

smrz2001 Nov 16, 2022

vyzo Nov 16, 2022

lidel Nov 16, 2022

smrz2001 Nov 16, 2022

smrz2001 Nov 28, 2022

lidel Nov 28, 2022 •

edited

Loading

smrz2001 Nov 28, 2022

lidel left a comment

lidel Nov 28, 2022 •

edited

Loading

feat: Pubsub.SeenMessagesTTL #9372

feat: Pubsub.SeenMessagesTTL #9372

Conversation

smrz2001 commented Oct 26, 2022 • edited by lidel Loading

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

smrz2001 Nov 16, 2022

Choose a reason for hiding this comment

lidel Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

smrz2001 Nov 16, 2022

Choose a reason for hiding this comment

vyzo Nov 16, 2022

Choose a reason for hiding this comment

lidel Nov 16, 2022

Choose a reason for hiding this comment

smrz2001 Nov 16, 2022

Choose a reason for hiding this comment

smrz2001 Nov 28, 2022

Choose a reason for hiding this comment

lidel Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

smrz2001 Nov 28, 2022

Choose a reason for hiding this comment

lidel left a comment

Choose a reason for hiding this comment

lidel Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

smrz2001 commented Oct 26, 2022 •

edited by lidel

Loading

lidel left a comment •

edited

Loading

lidel Nov 9, 2022 •

edited

Loading

lidel Nov 16, 2022 •

edited

Loading

lidel Nov 28, 2022 •

edited

Loading

lidel Nov 28, 2022 •

edited

Loading