Implement gossip.SetWithBloomFilter #4632

StephenButtolph · 2025-12-01T16:28:51Z

Why this should be merged

Part of #4621

This allows VM implementations an alternative to the gossip.BloomFilter implementation. By using this implementation, users do not need to manage a bloom filter on their own.

How this works

Provides a generic implementation similar to the implementations in the X-chain and the P-chain.

To speed up development, this PR does not include migrating the existing VMs to this new implementation.

How this was tested

Updated the integration test of the gossip package to utilize the simplified API.

Need to be documented in RELEASES.md?

No.

network/p2p/gossip/set_test.go

network/p2p/gossip/bloom.go

network/p2p/gossip/set.go

go.mod

JonathanOppenheimer

Someone else more knowledgeable should also review.

ARR4N

No need for me to re-review the requested changes.

ARR4N · 2025-12-02T10:47:53Z

network/p2p/gossip/bloom.go

-	// is later modified.
-	salt := b.salt
-	return bloomBytes, salt[:]
+func (b *BloomFilter) BloomFilter() (*bloom.Filter, ids.ID) {


For my understanding: why ids.ID? It seems like an odd overloading of the meaning of a type.

I don't think there was a reason in the old code - I think we just went with hash = ids.ID. It would probably make more sense to make a type alias for salt or just use plain []byte

I returned an array rather than a slice to avoid needing to do the copy.. It could just be [32]byte... We just typically use ids.ID throughout the repo for these... We could probably clean it up later

ARR4N · 2025-12-02T10:59:00Z

network/p2p/gossip/gossip.go

 }

+// PushGossiperSet exposes whether hashes are still included in a set.
+type PushGossiperSet interface {


(subjective) I understand (and agree with) the abstraction of the different set types, but their names feel unintuitive to me. Although they're named for how they're used, I think it would make more sense for them to be named for what they do. This, for example, could be a MembershipVerber (perhaps Reporter or Checker).

If you plan on coming back to this code later then I think it's better to just add a TODO so we don't block SAE on this.

I added TODOs

ARR4N · 2025-12-02T11:00:43Z

network/p2p/gossip/set.go

+	// This value should always be at least as large as the number of items that
+	// can be iterated over with a call to Iterate.


Please elaborate on scenarios in which one might expect Iterate to yield fewer items.

I reworded it to say it should be equal... Really it just must not be smaller.

network/p2p/gossip/set.go

network/p2p/gossip/set_test.go

ARR4N · 2025-12-02T11:25:11Z

network/p2p/gossip/set_test.go

+
+			// Add one to expectedResetCount to account for the initial creation
+			// of the bloom filter.
+			require.Equal(tt.expectedResetCount+1, testutil.ToFloat64(bs.metrics.ResetCount))


Suggested change

require.Equal(tt.expectedResetCount+1, testutil.ToFloat64(bs.metrics.ResetCount))

require.Equal(tt.expectedResetCount+1, testutil.ToFloat64(bs.metrics.ResetCount), "number of resets")

I'm curious on the rationale for these logs in require - isn't this generally self documenting?

I also don't find the logs useful, but adding them doesn't matter to me

ARR4N · 2025-12-02T11:26:40Z

network/p2p/gossip/set_test.go

+			require.Equal(tt.expectedResetCount+1, testutil.ToFloat64(bs.metrics.ResetCount))
+			b, h := bs.BloomFilter()
+			for _, expected := range tt.expected {
+				require.True(bloom.Contains(b, expected[:], h[:]))


Suggested change

require.True(bloom.Contains(b, expected[:], h[:]))

require.Truef(bloom.Contains(b, expected[:], h[:]), "%T.Contains(%#x)", bloom, expected)

Also, assert is more appropriate for testing set membership, to provide a full picture and avoid whack-a-mole debugging. Just saying 😉

I know people talk about whack-a-mole debugging... I have never experienced it myself

network/p2p/gossip/set_test.go

network/p2p/gossip/set.go

joshua-kim · 2025-12-01T20:47:31Z

network/p2p/gossip/gossip.go

+// that were not included in the filter.
+type PullGossiperSet[T Gossipable] interface {
+	// Add adds a value to the set. Returns an error if v was not added.
+	Add(v T) error


nit: I noticed we use this throughout the PR but did we intend to use v here? Should this be t?

I felt like v for value was pretty normal

network/p2p/gossip/gossip_test.go

network/p2p/gossip/set.go

joshua-kim · 2025-12-02T17:05:04Z

network/p2p/gossip/set_test.go

+
+			// Add one to expectedResetCount to account for the initial creation
+			// of the bloom filter.
+			require.Equal(tt.expectedResetCount+1, testutil.ToFloat64(bs.metrics.ResetCount))


I'm curious on the rationale for these logs in require - isn't this generally self documenting?

Copilot

Pull request overview

This PR introduces gossip.BloomSet, a new implementation that simplifies bloom filter management by wrapping a Set implementation and automatically handling bloom filter creation, resetting, and synchronization. This eliminates the need for VM implementations to manually manage bloom filters.

Key changes:

Adds BloomSet as a generic wrapper that manages bloom filters for any Set implementation
Deprecates the existing BloomFilter type in favor of BloomSet
Updates interface definitions to separate concerns between handlers, push gossip, and pull gossip

Reviewed changes

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

Show a summary per file

File	Description
network/p2p/gossip/set.go	Implements new `BloomSet` wrapper with automatic bloom filter management
network/p2p/gossip/set_test.go	Adds comprehensive tests for `BloomSet` including concurrency testing
network/p2p/gossip/gossip.go	Moves `Gossipable` and `Marshaller` definitions, adds new interface types for pull/push gossipers
network/p2p/gossip/handler.go	Updates to use new `HandlerSet` interface instead of full `Set`
network/p2p/gossip/bloom.go	Deprecates existing `BloomFilter` implementation and updates method signatures
network/p2p/gossip/gossipable.go	Removes file (definitions moved to gossip.go)
network/p2p/gossip/gossip_test.go	Updates tests to use `BloomSet` and refactors test doubles
network/p2p/gossip/bloom_test.go	Updates to use new `BloomFilter()` method signature
vms/platformvm/network/gossip.go	Updates to implement new `BloomFilter()` method signature
vms/avm/network/gossip.go	Updates to implement new `BloomFilter()` method signature
graft/coreth/plugin/evm/eth_gossiper.go	Updates to implement new interfaces and method signature
graft/coreth/plugin/evm/atomic/txpool/mempool.go	Updates to implement new interfaces and method signature
graft/coreth/plugin/evm/gossip/handler.go	Updates to use new `HandlerSet` interface
graft/coreth/plugin/evm/tx_gossip_test.go	Simplifies test to use new gossip utilities
graft/coreth/plugin/evm/atomic/vm/tx_gossip_test.go	Simplifies test to use new gossip utilities
go.mod	Updates dependency versions

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

network/p2p/gossip/set.go

joshua-kim · 2025-12-03T16:43:17Z

network/p2p/gossip/set.go

+	DefaultMinTargetElements              = 1000
+	DefaultTargetFalsePositiveProbability = .01
+	DefaultResetFalsePositiveProbability  = .05


Should we expose these under a single DefaultBloomSetConfig? It seems like it would make discoverability easier

I did it this way so that the defaults can't be changed (As we can't expose a const DefaultBloomSetConfig).

StephenButtolph added 3 commits November 26, 2025 12:41

Skip incrementing bloom count after noop Add

c6439f2

Implement gossip.SetWithBloomFilter for usage in the sae vm

5c29684

nits

01e4f33

StephenButtolph requested review from a team and joshua-kim as code owners December 1, 2025 16:28

github-project-automation bot added this to avalanchego Dec 1, 2025

JonathanOppenheimer added the need-subnet-evm-port label Dec 1, 2025

JonathanOppenheimer reviewed Dec 1, 2025

View reviewed changes

network/p2p/gossip/set_test.go Show resolved Hide resolved

fix order

0b09d08

JonathanOppenheimer reviewed Dec 1, 2025

View reviewed changes

network/p2p/gossip/bloom.go Outdated Show resolved Hide resolved

JonathanOppenheimer reviewed Dec 1, 2025

View reviewed changes

network/p2p/gossip/set.go Outdated Show resolved Hide resolved

StephenButtolph added 6 commits December 1, 2025 12:14

add concurrency test

1ca0b4f

fix lint

88f1fb0

Comments

690e1ae

wip

98b2978

wip

3ba684b

ci?

edc2d77

StephenButtolph commented Dec 1, 2025

View reviewed changes

go.mod Show resolved Hide resolved

JonathanOppenheimer approved these changes Dec 1, 2025

View reviewed changes

Base automatically changed from bloom-count-noop to master December 1, 2025 21:04

merged

a8313ed

ARR4N approved these changes Dec 2, 2025

View reviewed changes

joshua-kim reviewed Dec 2, 2025

View reviewed changes

JonathanOppenheimer added the coreth label Dec 2, 2025

Address most PR comments

9d27671

Copilot AI review requested due to automatic review settings December 3, 2025 16:12

Copilot AI reviewed Dec 3, 2025

View reviewed changes

network/p2p/gossip/set.go Outdated Show resolved Hide resolved

StephenButtolph added 3 commits December 3, 2025 11:15

Add TODOs

fddd265

typo

f3bbb0d

manually pass t

fb3b413

lint

cb16af9

joshua-kim approved these changes Dec 3, 2025

View reviewed changes

StephenButtolph added this pull request to the merge queue Dec 3, 2025

Merged via the queue into master with commit 39b7456 Dec 3, 2025
40 checks passed

StephenButtolph deleted the implement-set-with-bloom-filter branch December 3, 2025 17:11

github-project-automation bot moved this to Done 🎉 in avalanchego Dec 3, 2025

		// This value should always be at least as large as the number of items that
		// can be iterated over with a call to Iterate.

	require.Equal(tt.expectedResetCount+1, testutil.ToFloat64(bs.metrics.ResetCount))
	require.Equal(tt.expectedResetCount+1, testutil.ToFloat64(bs.metrics.ResetCount), "number of resets")

	require.True(bloom.Contains(b, expected[:], h[:]))
	require.Truef(bloom.Contains(b, expected[:], h[:]), "%T.Contains(%#x)", bloom, expected)

Implement gossip.SetWithBloomFilter #4632

Implement gossip.SetWithBloomFilter #4632

Uh oh!

Conversation

StephenButtolph commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Need to be documented in RELEASES.md?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JonathanOppenheimer left a comment

Choose a reason for hiding this comment

Uh oh!

ARR4N left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshua-kim Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

StephenButtolph commented Dec 1, 2025 •

edited

Loading

joshua-kim Dec 2, 2025 •

edited

Loading