feat(gossip): make BloomFilter completely thread-safe and introduce ResetIfNeeded() method

[ARR4N]

Why this should be merged

Since gossip.ResetBloomFilterIfNeeded() wasn't thread-safe, users were required to take out their own lock. However the check for "IfNeeded" only requires a read lock (avoiding unnecessarily locking out other goroutines), but this isn't possible externally. Without this, all new SAE transactions received over RPC will be handled sequentially.

Since this change no longer requires consumers to hold a hold while resetting, it could result in a race between Marshal() and refilling after a reset, resulting in marshalling an empty filter. The new BloomFilter.ResetIfNeeded() method therefore also accepts an iterator that it uses to refill the filter while still holding the lock.

While I could have exposed an IsResetNeeded() bool method, this would force every consumer to implement the same logic. I also made func(*BloomFilter, ...) functions into methods on the type as part of a general tidying of the file.

How this works

Adds a sync.RWMutex into BloomFilter. Methods that were already thread-safe are considered reads while resetting is considered a write.

How this was tested

Existing UT for general functionality. Actually testing the thread safety is difficult (is it even possible?) so I simply clobber all the methods in an attempt to trigger go test -race errors. Refilling has a new UT.

Need to be documented in RELEASES.md?

No

[Copilot]

Pull request overview

This PR makes the BloomFilter type completely thread-safe by introducing internal locking, eliminating the need for external synchronization. The key improvement is the addition of a ResetIfNeeded() method that uses read-write locking to efficiently check whether a reset is needed (with a read lock) before acquiring a write lock for the actual reset operation.

Key changes:

• Added sync.RWMutex to BloomFilter to protect concurrent access during resets
• Introduced ResetIfNeeded() method with double-checked locking pattern for efficient thread-safe resets
• Converted package-level functions to methods on BloomFilter for better encapsulation

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
network/p2p/gossip/bloom.go	Added RWMutex synchronization to BloomFilter, converted functions to methods, and implemented ResetIfNeeded() with double-checked locking
network/p2p/gossip/bloom_test.go	Added concurrency stress test to verify thread-safety with race detector

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[StephenButtolph]

Does this refactor encourage misuse of this bloom filter?

Specifically, it seems like this change does not expect for the bloom filter to be refilled atomically with its reset. This code certainly can be cleaned up... But adding this locking behavior doesn't seem useful (when using the bloom filter correctly).

If the bloom filter is accessed in a racy manner (Specifically Marshal being called concurrently with ResetIfNeeded followed by the expected Add calls), wouldn't we send a (potentially empty) bloom filter even though we may have a large number of items still in the set?

[ARR4N]

Yeah I see what you mean. I think the best way to address that along with my intended improvement is to add a afterReset func() argument, which is called while still holding the write lock, and expect refills to be in there. WDYT?

EDIT: I implemented it in the latest commit (7118ad6) because it's easy to revert but does a better job of communicating my thinking.

[ARR4N]

I changed from a general callback to an iter.Seq[Gossipable] to reduce the scope to exactly what's needed.