feat: Pubsub.SeenMessagesStrategy

[smrz2001]

This PR closes #9542 – attempts to apply the fix for libp2p/go-libp2p-pubsub#502 to kubo.

Once an official pubsub release is available, this draft PR can be updated accordingly and marked ready for review.

cc @lidel @BigLep

sending [msg_1] with message ID [MsgId_1] at T0.000000s
received [msg_1] at T0.000726s
sending [msg_2] with message ID [MsgId_1] at T0.002784s
did not receive [msg_2] at T1.011944s
sending [msg_3] with message ID [MsgId_2] at T1.013189s
received [msg_3] at T1.014176s
sending [msg_4] with message ID [MsgId_1] at T9.913194s
did not receive [msg_4] at T10.924792s
sending [msg_5] with message ID [MsgId_1] at T10.930913s
did not receive [msg_5] at T11.944472s
sending [msg_6] with message ID [MsgId_2] at T11.950365s
received [msg_6] at T11.950365s
sending [msg_7] with message ID [MsgId_1] at T21.036650s
received [msg_7] at T21.037893s
sending [msg_8] with message ID [MsgId_3] at T21.039733s
received [msg_8] at T21.039733s
--- PASS: TestMessageSeenCacheTTL (21.66s)

Should also close #9554

[lidel]

FYSA I've pushed this change, so we can see if CI passes against last commit from libp2p/go-libp2p-pubsub#513
(It is late Friday so will do proper review on Monday)

@smrz2001 In the future, you can point at an arbitrary commit from your own fork via:

$  go mod edit -replace github.com/libp2p/go-libp2p-pubsub=github.com/smrz2001/go-libp2p-pubsub@04bfcf58514f59bddb650fec5b2edac018c8c406

[smrz2001]

Thanks so much, @lidel!! I didn't know we could point to fork commits like that!

[smrz2001]

I'll fix any CI issues that come up.

[lidel]

Updated to latest master to include newly added GH version of CI jobs and see if this PR fixes issue described in #9554.

Will check on the result tomorrow.

[smrz2001]

@lidel, since @vyzo requested switching back to the original, "first seen" time cache implementation in the pubsub PR, I updated the kubo code to explicitly use the new, "last seen" implementation.

I could also make this configurable if you suggest. Regardless, I think it would be best to default to the new implementation for kubo, even if configurable.

I could also make it configurable in a follow up PR. Please let me know either way.

[smrz2001]

@lidel, I see the "context deadline timeout" failure on (my previous) flaky pubsub tests that you mentioned might get fixed via this PR.

I'm going to try bumping the timeout to see if that helps. 1 second is probably too small.

[smrz2001]

@lidel, I see the "context deadline timeout" failure on (my previous) flaky pubsub tests that you mentioned might get fixed via this PR.

I'm going to try bumping the timeout to see if that helps. 1 second is probably too small.

Bah, it failed even with the larger value (though it passed just before that with the same value) :(

[lidel]

I could also make this configurable if you suggest. Regardless, I think it would be best to default to the new implementation for kubo, even if configurable.
I could also make it configurable in a follow up PR. Please let me know either way.

Fine to change the default, but please include a configuration option in this PR – @vyzo makes a good point, there may be people who build things based on the old behavior

We should document the change in release notes, allowing them to switch back via optional configration option.

Let's make it future-proof, in case more advanced strategies will be added in the future.
Add *OptionalString to config/pubsub.go, and docs/config.md, something like:

+SeenExpirationStrategy *OptionalString  `json:",omitempty"`

With last-observed being the new implicit default and first-observed being the old one (feel free to pick better names). 🙏

[smrz2001]

@lidel, I just pushed some updates to make the cache implementation configurable. I relied on the enum I added to the pubsub code and used an OptionalInteger for the new config (which for now would be 0 (first seen) or 1 (last seen)). Please let me know if I need to change this. I can just add strings that map to the enum values also.

[smrz2001]

@lidel, I just pushed some updates to make the cache implementation configurable. I relied on the enum I added to the pubsub code and used an OptionalInteger for the new config (which for now would be 0 (first seen) or 1 (last seen)). Please let me know if I need to change this. I can just add strings that map to the enum values also.

Ok, nvm, I changed it to a string.

[BigLep]

@smrz2001 : FYI that @lidel is going to prioritize the review here so that we can wrap this up today/tomorrow and do 0.18.1 tomorrow: #9579 . If you can please be quick on any responses that come from @lidel , that would be great. Thanks!

@lidel : I assume we'll do a changelog addition. 0.18.1 changelog has been started in #9587

[smrz2001]

Thanks so much for the cleanup and tweaks, @lidel 🙏🏼

[lidel]

Thank you @smrz2001 for driving this and being patient with us.
Added some docs and release notes, merging.

0.18.1 release can be tracked in #9579 (ETA Friday or Monday, depends if other item lands on time)