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

[tinniaru3005]

Summary

Centralized the cross-service ad cleanup into one helper (removeAdvertisementEverywhere), so a shared advertisement gets removed from every service list in one explicit pass instead of relying on each service's prune loop to lazily stumble onto it.

Follow-up to #2845

Affected Areas

• Gossipsub

• Transports

• Peer Management / Discovery

• Protocol Logic

• Build / Tooling

• Other

Compatibility & Downstream Validation

Reference PRs / branches / commits demonstrating successful integration:

• Nimbus:

• Waku:

• Codex:

Impact on Library Users

Risk Assessment

References

Additional Notes

[AlbertoSoutullo]

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]

good point. working on this

[tinniaru3005]

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.

[AlbertoSoutullo]

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

[AlbertoSoutullo]

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...