feat: gossip improvements by sbackend123 · Pull Request #5520 · ethersphere/bee

sbackend123 · 2026-06-26T11:16:55Z

Checklist

I have read the coding guide.
My change requires a documentation update, and I have done it.
I have added tests to cover my changes.
I have filled out the description and linked the related issues.

Description

Adds write coalescing for hive outbound gossip. Single-peer BroadcastPeers calls are buffered per addressee and flushed as one batched message after ~1s (configurable via GossipCoalesceInterval), or immediately when the buffer reaches maxBatchSize (30). Calls with 2+ peers are sent without coalescing

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

#5490

Screenshots (if appropriate):

AI Disclosure

This PR contains code that has been generated by an LLM.
I have reviewed the AI generated code thoroughly.
I possess the technical expertise to responsibly review the code generated in this PR.

Discard buffered gossip on shutdown instead of flushing, restore quit checks in broadcastNow, document the async BroadcastPeers contract, use fixed 100ms jitter, and clean up tests and metrics. Co-authored-by: Cursor <cursoragent@cursor.com>

acud · 2026-07-07T18:20:40Z

+type pendingGossip struct {
+	addressee swarm.Address
+	peers     map[string]swarm.Address // peer bytestring -> address (set semantics)
+	deadline  time.Time


i wonder whether there's a benefit of keeping a deadline per peer. usually, write coalescing is simple enough:

have an interval fire at a constant rate

if new records arrived by interval fire

when new entries arrived, optionally, postpone the sending the sending until the next interval firing (and so also you could extend up to a set upper bound, so that entries don't keep collecting forever but also guarantee that information goes out still relatively quickly)

send all the pending sends

also: usually, when a peer arrives - we gossip that peer to all peers (full nodes) and gossip to that peer all of our connected peers.
this in turn means that sending is almost always involving all connected peers. which in turn also means that the timestamps on the individual pendingGossip entries would be almost identical (making the field even more so redundant)

gacevicljubisa

Nice work.

One idea: instead of deciding what to do based on how many peers are passed in, it might be cleaner to add a separate method like GossipPeer(addressee, peer) on discovery.Driver just for the buffered case. Make BroadcastPeers plain => send now and return an error method, and use GossipPeer as async (fire and forget) in kademlia.go:1082-1090 and flush in startGossipCoalescer worker. This allows you to drop coalesceThreshold. Also, the "buffer is full" logic, you can also move to the background worker with a wakeup channel so buffering never blocks or sends directly.

acud · 2026-08-07T23:01:03Z

+
+	key := addressee.ByteString()
+	peerSet, ok := b.pending[key]
+	if !ok {


since maps leak memory by design, it would be better to:

check whether the b.pending key exists

merge its contents with peers if it does before writing the key to the map

check the max batch size and if the entry really exceeds max batch - return early without writing to the map

finally if we are within the bounds of the max batch - write to map

acud · 2026-08-07T23:04:48Z

+	if err != nil {
+		s.logger.Debug("coalesced gossip flush failed", "addressee", addressee, "reason", reason, "batch_size", len(peers), "error", err)
+	}
+	cancel()


ideally this should be a defer call just after it gets created.

acud · 2026-08-07T23:08:53Z

+			select {
+			case <-ticker.C:
+				for _, batch := range s.gossipBuf.takeAll() {
+					s.flushGossipBatch(batch.addressee, batch.peers, coalesceFlushReasonTimer)


nit - this is a blocking call that makes slower peers to block other peers from getting the information. i would tend to turn this into go s.flushGossipBatch. iirc the latest go compilers make sure the values get copied correctly such that when the iterator changes batch values it doesn't change the underlying value for the goroutines already dispatched with that same variable name. but maybe also putting this into a closure won't hurt too much.

acud · 2026-08-07T23:09:54Z

+)
+
+const (
+	defaultGossipCoalesceInterval = time.Second


nit: i think this can be higher (like 5 sec)? the same for the coalesce threshold - we want to have bigger messages and less often. the timer fires every 5 seconds anyway.

acud · 2026-08-07T23:12:51Z

+			peers:     slices.Collect(maps.Values(peerSet)),
+		})
+	}
+	b.pending = make(map[string]map[string]swarm.Address)


clear(b.pending)

sbackend123 · 2026-08-19T11:55:59Z

Nice work.

One idea: instead of deciding what to do based on how many peers are passed in, it might be cleaner to add a separate method like GossipPeer(addressee, peer) on discovery.Driver just for the buffered case. Make BroadcastPeers plain => send now and return an error method, and use GossipPeer as async (fire and forget) in kademlia.go:1082-1090 and flush in startGossipCoalescer worker. This allows you to drop coalesceThreshold. Also, the "buffer is full" logic, you can also move to the background worker with a wakeup channel so buffering never blocks or sends directly.

Nice idea, but looks like more changes than we need (?)
@acud wdyt?

acud · 2026-08-20T18:32:58Z

wdyt

not sure... also having a buffered vs unbuffered kinda defeats the purpose of batching the writes together. not sure if i see the case of non-buffered broadcast as needed. the thresholds should take care of that already - when the group is big enough - it is urgent enough (and that would always be the case for a peer that connects and gets a bunch of peers via gossip). so we can use the implicit behavior in this case instead of expanding the interfaces. my 2 cents

acud

lgtm

acud · 2026-08-20T18:46:36Z

 	s.metrics.BroadcastPeersPeers.Add(float64(len(peers)))

+	// Already-batched messages go out immediately; single-peer gossips are coalesced.
+	if len(peers) >= coalesceThreshold {


if the addressee already has some peers which are queued for sending through the buffer - they are silently skipped here. not a big issue, can be handled later too. flagging this nevertheless.

also, you might want to gossip, but the rate limiter won't allow you to send the whole batch together because you can't get enough tokens from the bucket. this puts things as a best effort. i'm not sure we should handle all those edge cases right away but they are definitely worth documenting at least inline and perhaps as a follow up issue.

martinconic · 2026-09-01T07:47:33Z

+			select {
+			case <-ticker.C:
+				for _, batch := range s.gossipBuf.takeAll() {
+					go func(batch gossipBatch) {


Untracked goroutine, this will bypass s.wg:

go func(batch) is not wrapped in s.wg.Go(...).

When the node shuts down, Service.Close() closes s.quit and calls s.wg.Wait().

s.wg.Wait() immediately finishes without waiting for the flushGossipBatch goroutines.

Inside flushGossipBatch

ctx, cancel := context.WithTimeout(context.Background(), messageTimeout) // 1 minute timeout!

The orphaned goroutines keep attempting to dial peers and open streams on the libp2p transport for up to 60 seconds after the node has shut down.

martinconic · 2026-09-01T07:50:58Z

Is the following scenario a valid one?

The Scenario
At time = 0s: Single peer PeerA is announced -> buffered in gossipBuf for RemoteNode.
At time = 1s: A batch of 5 peers [P1, P2, P3, P4, P5] is broadcast -> since length is 5 or more, it bypasses the buffer and sends immediately via Stream 1.
RemoteNode receives [P1..P5] before receiving PeerA.
At time = 5s: The timer fires and sends PeerA in Stream 2.

Time 0s: [Buffer: PeerA]
Time 1s: [Immediate Broadcast: P1, P2, P3, P4, P5] ----> Sent in Stream #1
Time 5s: [Coalescer Flush: PeerA] --------------------> Sent in Stream #2

Consequences:
Older gossip arrives after newer gossip.
If P1 was also in the buffer, it gets transmitted twice (Stream 1 + Stream 2).
If RemoteNode disconnects at time = 3s, PeerA is purged on disconnect and never delivered, even though later peers were delivered.

aloknerurkar

I think it now makes more sense to have one bgCtx in the service which is cancelled on quit. There seems to be a lot of cases where we read ctx.Done + s.quit. This can be condensed. Maybe we can do it in a separate PR.

aloknerurkar · 2026-09-01T07:46:57Z

+			select {
+			case <-ticker.C:
+				for _, batch := range s.gossipBuf.takeAll() {
+					go func(batch gossipBatch) {


The goroutines started here are not tracked using the waitgroup. If a shutdown happens in worst case these routines will wait till messageTimeout to exit and keep trying to write on p2p. Currently there is no test to Close mid-flush which is why we don't find this leak in the tests.

We should create these go routines using the waitgroup. Also maybe a test to verify everything closes correctly mid-flush.

aloknerurkar · 2026-09-01T07:50:22Z

 		if !s.outLimiter.Allow(addressee.ByteString(), maxSize) {
+			if coalesced {
+				s.metrics.GossipCoalesceDropped.Add(float64(len(peers)))
+			}


Just checking if its worth to add a debug log here rather than silently dropping.

gacevicljubisa

not sure if i see the case of non-buffered broadcast as needed. the thresholds should take care of that already - when the group is big enough - it is urgent enough

One thing to check: on a small cluster (beekeeper, or any node right after start, since only confirmed-reachable peers count) the introduction list at kademlia.go:1155 has fewer than 5 peers, so it gets buffered and returns nil. Does the Disconnect("failed broadcasting to peer") branch below it ever run then, and what happens to the list if the peer drops before the tick?

And the reason why I suggested GossipPeer is that it lets the caller say "this one can wait" instead of hive guessing from the count. Announce/AnnounceTo use it for the single-peer gossip, BroadcastPeers stays send-now-and-return-the-error.

PR overall looks good to me. Just address the comments regarding the untracked goroutines.

gacevicljubisa · 2026-09-03T11:32:38Z

One more thing at hive.go:199: the batch asks for all its tokens at once, and if the limiter says no the whole batch is dropped, not put back. After an addressee's burst is spent (refill is 1 token/min) nothing gets through until 30 tokens accumulate, ~30 minutes for a full batch, and every batch tried in between is lost.

sbackend123 · 2026-09-06T13:01:30Z

Is the following scenario a valid one?

The Scenario At time = 0s: Single peer PeerA is announced -> buffered in gossipBuf for RemoteNode. At time = 1s: A batch of 5 peers [P1, P2, P3, P4, P5] is broadcast -> since length is 5 or more, it bypasses the buffer and sends immediately via Stream 1. RemoteNode receives [P1..P5] before receiving PeerA. At time = 5s: The timer fires and sends PeerA in Stream 2.
Time 0s: [Buffer: PeerA]
Time 1s: [Immediate Broadcast: P1, P2, P3, P4, P5] ----> Sent in Stream #1
Time 5s: [Coalescer Flush: PeerA] --------------------> Sent in Stream #2
Consequences: Older gossip arrives after newer gossip. If P1 was also in the buffer, it gets transmitted twice (Stream 1 + Stream 2). If RemoteNode disconnects at time = 3s, PeerA is purged on disconnect and never delivered, even though later peers were delivered.

Yes, this scenario is possible, but in my opinion it would only be a problem in a very sparse or unstable topology.
Under normal conditions, the delivery order of peer gossip does not matter. Even if an announcement is lost on disconnect, the peer will most likely be advertised later by other peers, or discovered through subsequent gossip or a handshake.
The point about deduplication is valid, but I would rather not complicate the current solution. Deduplication already exists at the buffer level; we can observe mainnet behavior and extend the implementation later if needed.

aloknerurkar · 2026-09-09T11:26:42Z


 func (s *Service) Close() error {
 	close(s.quit)
+	s.bgCancel()


So idea was to not have both. Right now it seems like we are duplicating the functionality. In some cases we are waiting on s.quit and in some cases we are waiting on bgCtx.Done. This has become more confusing than it was.

If we have bgCtx we can remove s.quit. In broadcastNow function when we are using the caller context we need to merge the cancel functions of both bgCtx and the calling context. That way we obey both cancels. This way we can remove s.quit. But this is not so straightforward, which is why I suggested doing it in a separate PR.

For eg https://pkg.go.dev/context#example-AfterFunc-Merge

martinconic · 2026-09-01T07:54:45Z

 		}

-		// If broadcasting limit is exceeded, return early
 		if !s.outLimiter.Allow(addressee.ByteString(), maxSize) {


Because takeAll() already removed all buffered peers from b.pending, if s.outLimiter.Allow(..., 30) returns false (when available tokens < 30), broadcastNow returns nil and all 30 peers are permanently lost.

Since limitRate is time.Minute (1 token/minute refill), if a bucket is depleted, a node will drop all 30-peer batches for up to 30 minutes.

We should re-enqueue the rejected peers back into the buffer so they can be sent on subsequent ticks or when tokens become available

Yes, we decided not to cover all corner cases now
#5520 (comment)
I can just document it as Elad suggested

martinconic · 2026-09-01T07:55:47Z

+// coalesceThreshold peers are buffered and flushed asynchronously; errors
+// during deferred dispatch are logged but not returned to the caller.
+// Calls with coalesceThreshold or more peers are sent immediately.
 func (s *Service) BroadcastPeers(ctx context.Context, addressee swarm.Address, peers ...swarm.Address) error {


If a caller invokes BroadcastPeers with a context that is already canceled or has a short deadline, the check is bypassed and the peer is buffered anyway, returning nil. We should validate ctx.Err() before staging into memory

sbackend123 added 3 commits June 25, 2026 09:59

feat: ttl cache for gossip deduplication

3c44fa7

feat: send peers in batches

0825424

feat: peer distribution with gradient

646bdb5

github-actions Bot assigned sbackend123 Jun 26, 2026

sbackend123 added 4 commits June 28, 2026 19:44

fix: remove fanout

5fa5cc0

fix: remove deduplication

8cb8fc1

fix: testing

607f400

fix: linter issue

ddb758f

sbackend123 mentioned this pull request Jun 29, 2026

fix: better coverage for distant bins #5521

Merged

7 tasks

sbackend123 and others added 3 commits June 29, 2026 11:48

fix: address gossip coalesce review feedback

be0e1fb

Discard buffered gossip on shutdown instead of flushing, restore quit checks in broadcastNow, document the async BroadcastPeers contract, use fixed 100ms jitter, and clean up tests and metrics. Co-authored-by: Cursor <cursoragent@cursor.com>

fix: clean up

5ec8adf

fix: clean up 2

a7a5a15

sbackend123 marked this pull request as ready for review June 29, 2026 12:43

sbackend123 requested review from acud, gacevicljubisa and martinconic June 29, 2026 12:43

acud reviewed Jul 7, 2026

View reviewed changes

fix: simplify implementation

ea9a08f

sbackend123 requested a review from acud July 16, 2026 11:13

gacevicljubisa reviewed Jul 21, 2026

View reviewed changes

fix: better coverage for distant bins (#5521)

2b276e5

acud requested changes Aug 7, 2026

View reviewed changes

acud reviewed Aug 7, 2026

View reviewed changes

sbackend123 added 3 commits August 17, 2026 22:16

Merge branch 'master' into feat/gossip-improvements

f892fbf

fix: address review comments

6c6fd78

fix: make linter happy

bea3e7d

sbackend123 requested a review from acud August 20, 2026 09:50

acud approved these changes Aug 20, 2026

View reviewed changes

sbackend123 requested a review from gacevicljubisa August 27, 2026 09:55

martinconic reviewed Sep 1, 2026

View reviewed changes

aloknerurkar reviewed Sep 1, 2026

View reviewed changes

gacevicljubisa requested a review from janos September 3, 2026 09:54

gacevicljubisa reviewed Sep 3, 2026

View reviewed changes

fix: address review comments

7dffd3c

sbackend123 requested review from aloknerurkar and martinconic September 3, 2026 21:41

aloknerurkar reviewed Sep 9, 2026

View reviewed changes

martinconic reviewed Sep 10, 2026

View reviewed changes

sbackend123 added 2 commits September 10, 2026 14:37

fix: check context

b42d2af

Merge branch 'master' into feat/gossip-improvements

d58db7d

Uh oh!

Conversation

sbackend123 commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):

AI Disclosure

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gacevicljubisa left a comment • 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!

sbackend123 commented Aug 19, 2026

Uh oh!

acud commented Aug 20, 2026

Uh oh!

acud left a comment

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!

martinconic commented Sep 1, 2026

Uh oh!

aloknerurkar left a comment

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!

gacevicljubisa left a comment

Choose a reason for hiding this comment

Uh oh!

gacevicljubisa commented Sep 3, 2026

Uh oh!

sbackend123 commented Sep 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbackend123 Sep 10, 2026 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sbackend123 commented Jun 26, 2026 •

edited

Loading

gacevicljubisa left a comment •

edited

Loading

sbackend123 Sep 10, 2026 •

edited

Loading