Skip to content

fix(service-disco): centralize cross-service advertisement cleanup #2847

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SionoiS merged 3 commits into from
Jul 17, 2026
Merged

fix(service-disco): centralize cross-service advertisement cleanup #2847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5ce5580
centralize cross-service advertisement cleanup
tinniaru3005 Jul 17, 2026
549a114
fixes
tinniaru3005 Jul 17, 2026
5143295
fix
tinniaru3005 Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 38 additions & 16 deletions libp2p/protocols/service_discovery/registrar.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,20 @@ proc removeAd*(ipTree: IpTree, ad: Advertisement) {.raises: [].} =
proc isExpired(now, ts: Moment, expiry: Duration): bool =
now - ts > expiry

proc removeAdvertisementEverywhere(registrar: Registrar, ad: Advertisement) =
let key = ad.toAdvertisementKey()

registrar.cacheTimestamps.del(key)

for _, sads in registrar.cache.mpairs:
var j = 0
while j < sads.len:
if sads[j].toAdvertisementKey() == key:
registrar.ipTree.removeAd(sads[j])
sads.delete(j)
else:
inc(j)
Comment on lines +57 to +69

@AlbertoSoutullo AlbertoSoutullo Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow me to also jump in here.
From what I see, this removes the ad from ipTree only once, but insertNewAd inserts into ipTree once per service cache insertion afaik.
If insertNewAd was called 3 times, so ipTree.insertAd(ad) was also called 3 times. The new removeAdvertisementEverywhere deletes all 3 cache entries, but only calls registrar.ipTree.removeAd(ad) once.
Unless I am missing something this leaves ipTree counters inflated.
I would either remove from ipTree once per removed cache entry, or change insertion so ipTree is only incremented once per AdvertisementKey.
Also, updateExistingAd can still orphan old copies of a replaced advertisement across other service caches, so the centralized cleanup should be used there too. Does this makes sense?

I think this really requires adding more tests to the codebase.

@tinniaru3005 tinniaru3005 Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. working on this

@tinniaru3005 tinniaru3005 Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

went with your first option — remove from ipTree once per removed cache entry, not once per AdvertisementKey.

Fixed in three places:

  • insertNewAd — IP tree insert is unconditional, once per service acceptance.
  • removeAdvertisementEverywhere — now removes from the IP tree once per matching cache entry found in the sweep, not once total.
  • updateExistingAd centralization.

New tests: same ad across 3 services counts the tree 3x on insert; a shared ad's IP tree entries all clear on removal; an update through one service replaces the stale copy in a second service and keeps the tree balanced through that replacement. All passing.


proc pruneAdsForService(
registrar: Registrar,
serviceId: ServiceId,
Expand All @@ -72,11 +86,7 @@ proc pruneAdsForService(
remove = isExpired(now, ts[], advertExpiry)

if remove:
# Free global state only while this key still owns a timestamp.
if key in registrar.cacheTimestamps:
registrar.ipTree.removeAd(ad)
registrar.cacheTimestamps.del(key)
ads.delete(i)
registrar.removeAdvertisementEverywhere(ad)
inc(expiredCount)
else:
inc(i)
Expand Down Expand Up @@ -324,18 +334,18 @@ proc evictOldestAd*(
return

var emptiedSids: seq[ServiceId] = @[]
var removedFromIpTree = false
disco.registrar.cacheTimestamps.del(oldestKey)

for sid, sads in disco.registrar.cache.mpairs:
var i = 0
var removedFromThisService = false
while i < sads.len:
if sads[i].toAdvertisementKey() == oldestKey:
# Free IP tree / timestamp once; further service lists are orphans.
if not removedFromIpTree:
disco.registrar.ipTree.removeAd(sads[i])
disco.registrar.cacheTimestamps.del(oldestKey)
removedFromIpTree = true
# Each service holding this key was a separate ad_cache admission
# (an advertisement is associated to its service_id) that added
# the IP once - remove it once per matching entry, not once total,
# or the tree stays inflated for every service beyond the first.
disco.registrar.ipTree.removeAd(sads[i])
sads.delete(i)
removedFromThisService = true
else:
Expand Down Expand Up @@ -368,11 +378,17 @@ proc updateExistingAd*(
registrar.cacheTimestamps[existing.toAdvertisementKey()] = now
return false
elif ad.data.seqNo > existing.data.seqNo:
registrar.ipTree.removeAd(existing)
registrar.cacheTimestamps.del(existing.toAdvertisementKey())
let oldKey = existing.toAdvertisementKey()
for _, sads in registrar.cache.mpairs:
for j in 0 ..< sads.len:
if sads[j].toAdvertisementKey() == oldKey:
registrar.ipTree.removeAd(sads[j])
registrar.ipTree.insertAd(ad)
sads[j] = ad

registrar.cacheTimestamps.del(oldKey)
Comment on lines +381 to +389

@AlbertoSoutullo AlbertoSoutullo Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is wrong...
This updates every service cache entry matching the old (peerId, seqNo) to the new advertisement, without checking whether the new advertisement advertises that service.
This can put an invalid ad under a service cache:

oldAd(seq=1) advertises [svcA, svcB]
cache[svcA] = [oldAd]
cache[svcB] = [oldAd]

Then the peer registers:

newAd(seq=2) advertises [svcA]

through svcA.
isValidAdvertisement only checks that newAd advertises the requested serviceId:

if not ad.advertisesService(serviceId):
  return err("message & advertisement service mismatch")

So newAd is valid for svcA, but not necessarily for svcB.
The new updateExistingAd loop then does:

for _, sads in registrar.cache.mpairs:
  for j in 0 ..< sads.len:
    if sads[j].toAdvertisementKey() == oldKey:
      registrar.ipTree.removeAd(sads[j])
      registrar.ipTree.insertAd(ad)
      sads[j] = ad

That means cache[svcB] can now contain newAd, even though newAd.advertisesService(svcB) == false.
This breaks the cache and can later produce invalid GET_ADS results for svcB. The discoverer does defensively reject these via validAds:

if not ad.advertisesService(serviceId):
  error "advert service mismatch", serviceId
  continue

ads[idx] = ad
registrar.cacheTimestamps[ad.toAdvertisementKey()] = now
registrar.ipTree.insertAd(ad)
return true
else:
return false
Expand All @@ -387,10 +403,16 @@ proc insertNewAd*(
## Insert a brand-new advertisement into the cache.
## Evicts the globally oldest entry first if the cache is at capacity.
## Returns true (a new insertion always warrants a metrics update).
if disco.registrar.cacheTimestamps.len.uint64 >= disco.discoConfig.advertCacheCap:
## The IP tree is incremented unconditionally, once per (service, ad)
## admission - the same physical ad accepted for 3 services is 3 separate
## ad_cache admissions, and each one adds the IP once.
let key = ad.toAdvertisementKey()
let isNewKey = key notin disco.registrar.cacheTimestamps
if isNewKey and
disco.registrar.cacheTimestamps.len.uint64 >= disco.discoConfig.advertCacheCap:
evictOldestAd(disco, serviceId, ads)
ads.add(ad)
disco.registrar.cacheTimestamps[ad.toAdvertisementKey()] = now
disco.registrar.cacheTimestamps[key] = now
disco.registrar.ipTree.insertAd(ad)
return true

Expand Down
66 changes: 64 additions & 2 deletions tests/libp2p/service_discovery/test_registrar.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,8 @@ suite "Service Discovery Registrar - Cache Pruning":
registrar.cache[serviceId1] = @[ad]
registrar.cache[serviceId2] = @[ad]
registrar.cacheTimestamps[ad.toAdvertisementKey()] = now - 1000.secs
# Each service's copy is a separate ad_cache admission
registrar.ipTree.insertAd(ad)
registrar.ipTree.insertAd(ad)

pruneExpiredAds(registrar, 900.secs)
Expand Down Expand Up @@ -633,7 +635,7 @@ suite "Service Discovery Registrar - Cache Pruning":
fresh.toAdvertisementKey() in registrar.cacheTimestamps
expired.toAdvertisementKey() notin registrar.cacheTimestamps

test "pruneExpiredAds removes multi-service ad from IP tree only once":
test "pruneExpiredAds removes multi-service ad from IP tree once per service":
let registrar = Registrar.new()
let serviceId1 = makeServiceId(1)
let serviceId2 = makeServiceId(2)
Expand All @@ -643,8 +645,10 @@ suite "Service Discovery Registrar - Cache Pruning":
registrar.cache[serviceId1] = @[ad]
registrar.cache[serviceId2] = @[ad]
registrar.cacheTimestamps[ad.toAdvertisementKey()] = now - 1000.secs
# Each service's copy is a separate ad_cache admission
registrar.ipTree.insertAd(ad)
check registrar.ipTree.root.counter == 1
registrar.ipTree.insertAd(ad)
check registrar.ipTree.root.counter == 2

pruneExpiredAds(registrar, 900.secs)

Expand Down Expand Up @@ -1336,6 +1340,40 @@ suite "Service Discovery Registrar - updateExistingAd":
check currentAd.toAdvertisementKey() in registrar.cacheTimestamps
check staleAd.toAdvertisementKey() notin registrar.cacheTimestamps

test "higher seqNo replaces stale copies in every other service that cached it":
let registrar = Registrar.new()
let serviceId1 = makeServiceId(1)
let serviceId2 = makeServiceId(2)
let privateKey = PrivateKey.random(rng()).get()
let oldAd = makeAdvertisement(
privateKey = privateKey, seqNo = 1, addrs = @[makeMultiAddress("10.0.0.1")]
)
let newAd = makeAdvertisement(
privateKey = privateKey, seqNo = 2, addrs = @[makeMultiAddress("192.168.1.1")]
)

registrar.cache[serviceId1] = @[oldAd]
registrar.cache[serviceId2] = @[oldAd]
registrar.cacheTimestamps[oldAd.toAdvertisementKey()] = initMoment(1000)
# Each service's copy is a separate ad_cache admission
registrar.ipTree.insertAd(oldAd)
registrar.ipTree.insertAd(oldAd)
check registrar.ipTree.root.counter == 2

var ads1 = registrar.cache[serviceId1]
let changed = registrar.updateExistingAd(ads1, 0, newAd, initMoment(2000))
registrar.cache[serviceId1] = ads1

check changed
check registrar.cache[serviceId1].len == 1
check registrar.cache[serviceId1][0].data.seqNo == 2
# serviceId2 was never passed into updateExistingAd directly.
check registrar.cache[serviceId2].len == 1
check registrar.cache[serviceId2][0].data.seqNo == 2
check oldAd.toAdvertisementKey() notin registrar.cacheTimestamps
check newAd.toAdvertisementKey() in registrar.cacheTimestamps
check registrar.ipTree.root.counter == 2

Comment on lines +1343 to +1376

@AlbertoSoutullo AlbertoSoutullo Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test added at does not catch what I mentioned because both oldAd and newAd use the default "test-service" payload while being manually stored under arbitrary makeServiceId(1) / makeServiceId(2) keys. It should use real matching service IDs, then test the case where the new ad drops one of the old services...

suite "Service Discovery Registrar - insertNewAd":
test "inserts ad into cache, IP tree, and timestamps, returns true":
let disco =
Expand All @@ -1354,6 +1392,30 @@ suite "Service Discovery Registrar - insertNewAd":
check disco.registrar.cacheTimestamps[ad.toAdvertisementKey()] == now
check disco.registrar.ipTree.root.counter > 0

test "same ad accepted for three services counts the IP tree once per service":
# ad_cache associates each advertisement to its service_id : the
# same physical ad admitted for 3 services is 3 separate admissions,
# each of which adds the IP once - not one shared contribution.
let disco =
setupServiceDiscoveryNode(discoConfig = ServiceDiscoveryConfig.new(fReturn = 3))
let serviceId1 = makeServiceId(1)
let serviceId2 = makeServiceId(2)
let serviceId3 = makeServiceId(3)
let ad = makeAdvertisement(addrs = @[makeMultiAddress("10.0.0.1")])
let now = initMoment(1000)

var ads1: seq[Advertisement] = @[]
var ads2: seq[Advertisement] = @[]
var ads3: seq[Advertisement] = @[]
discard disco.insertNewAd(serviceId1, ads1, ad, now)
discard disco.insertNewAd(serviceId2, ads2, ad, now)
discard disco.insertNewAd(serviceId3, ads3, ad, now)
disco.registrar.cache[serviceId1] = ads1
disco.registrar.cache[serviceId2] = ads2
disco.registrar.cache[serviceId3] = ads3

check disco.registrar.ipTree.root.counter == 3

test "inserts ad without eviction when cache is under capacity":
let disco = setupServiceDiscoveryNode(
discoConfig = ServiceDiscoveryConfig.new(fReturn = 3, advertExpiry = 900.secs)
Expand Down
Loading