Skip to content

Commit 76f79c3

Browse files
SionoiSSionoiS
committed
fix
1 parent 985026a commit 76f79c3

2 files changed

Lines changed: 108 additions & 81 deletions

File tree

‎libp2p/protocols/service_discovery/advertisement_cache.nim‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,6 @@ proc evictOldest(c: AdvertisementCache) =
118118
proc replaceEverywhere(
119119
c: AdvertisementCache, oldKey: AdvertisementKey, ad: Advertisement, now: Moment
120120
) =
121-
## Replace `oldKey` with `ad` in every service that admitted the old key.
122-
## IP tree is adjusted once per membership.
123121
var oldEntry: CachedAd
124122
c.entries.withValue(oldKey, e):
125123
oldEntry = e[]
@@ -130,25 +128,31 @@ proc replaceEverywhere(
130128
let newKey = ad.toAdvertisementKey()
131129
let oldServices = oldEntry.services
132130

131+
var keptServices = initHashSet[ServiceId]()
133132
for serviceId in oldServices:
133+
# Free the old admission's IP contribution for every prior membership.
134134
c.ipTree.removeAd(oldEntry.ad)
135+
if ad.advertisesService(serviceId):
136+
keptServices.incl(serviceId)
137+
else:
138+
c.dropServicePeer(serviceId, peerId)
135139

136140
c.entries.del(oldKey)
137141

142+
if keptServices.len == 0:
143+
return
144+
138145
if newKey in c.entries:
139146
c.entries.withValue(newKey, newEntry):
140147
newEntry[].ad = ad
141148
newEntry[].timestamp = now
142-
for serviceId in oldServices:
149+
for serviceId in keptServices:
143150
newEntry[].services.incl(serviceId)
144151
c.setServicePeer(serviceId, peerId, newKey)
145152
c.ipTree.insertAd(ad)
146153
else:
147-
var services = initHashSet[ServiceId]()
148-
for serviceId in oldServices:
149-
services.incl(serviceId)
150-
c.entries[newKey] = CachedAd(ad: ad, timestamp: now, services: services)
151-
for serviceId in oldServices:
154+
c.entries[newKey] = CachedAd(ad: ad, timestamp: now, services: keptServices)
155+
for serviceId in keptServices:
152156
c.setServicePeer(serviceId, peerId, newKey)
153157
c.ipTree.insertAd(ad)
154158

‎tests/libp2p/service_discovery/test_registrar.nim‎

Lines changed: 96 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -1045,24 +1045,13 @@ suite "Service Discovery Registrar - acceptAdvertisement seqNo handling":
10451045
test "same peer higher seqNo replaces existing ad":
10461046
let disco =
10471047
setupServiceDiscoveryNode(discoConfig = ServiceDiscoveryConfig.new(fReturn = 3))
1048-
let serviceId = makeServiceId()
1048+
let serviceName = "service"
1049+
let serviceId = serviceName.hashServiceId()
10491050
let privateKey = PrivateKey.random(rng()).get()
1050-
let peerId = PeerId.init(privateKey).get()
10511051
let now = Moment.now()
10521052

1053-
let oldAd = SignedExtendedPeerRecord
1054-
.init(
1055-
privateKey,
1056-
ExtendedPeerRecord(peerId: peerId, seqNo: 1, addresses: @[], services: @[]),
1057-
)
1058-
.get()
1059-
1060-
let newAd = SignedExtendedPeerRecord
1061-
.init(
1062-
privateKey,
1063-
ExtendedPeerRecord(peerId: peerId, seqNo: 2, addresses: @[], services: @[]),
1064-
)
1065-
.get()
1053+
let oldAd = makeAdvertisement(serviceName, privateKey = privateKey, seqNo = 1)
1054+
let newAd = makeAdvertisement(serviceName, privateKey = privateKey, seqNo = 2)
10661055

10671056
disco.acceptAdvertisement(now, serviceId, oldAd)
10681057
check disco.registrar.ads.adsForService(serviceId)[0].data.seqNo == 1
@@ -1075,24 +1064,13 @@ suite "Service Discovery Registrar - acceptAdvertisement seqNo handling":
10751064
test "same peer lower seqNo is silently dropped":
10761065
let disco =
10771066
setupServiceDiscoveryNode(discoConfig = ServiceDiscoveryConfig.new(fReturn = 3))
1078-
let serviceId = makeServiceId()
1067+
let serviceName = "service"
1068+
let serviceId = serviceName.hashServiceId()
10791069
let privateKey = PrivateKey.random(rng()).get()
1080-
let peerId = PeerId.init(privateKey).get()
10811070
let now = Moment.now()
10821071

1083-
let newerAd = SignedExtendedPeerRecord
1084-
.init(
1085-
privateKey,
1086-
ExtendedPeerRecord(peerId: peerId, seqNo: 10, addresses: @[], services: @[]),
1087-
)
1088-
.get()
1089-
1090-
let olderAd = SignedExtendedPeerRecord
1091-
.init(
1092-
privateKey,
1093-
ExtendedPeerRecord(peerId: peerId, seqNo: 5, addresses: @[], services: @[]),
1094-
)
1095-
.get()
1072+
let newerAd = makeAdvertisement(serviceName, privateKey = privateKey, seqNo = 10)
1073+
let olderAd = makeAdvertisement(serviceName, privateKey = privateKey, seqNo = 5)
10961074

10971075
disco.acceptAdvertisement(now, serviceId, newerAd)
10981076
disco.acceptAdvertisement(now, serviceId, olderAd)
@@ -1103,9 +1081,10 @@ suite "Service Discovery Registrar - acceptAdvertisement seqNo handling":
11031081
test "different peers each store their own ad":
11041082
let disco =
11051083
setupServiceDiscoveryNode(discoConfig = ServiceDiscoveryConfig.new(fReturn = 3))
1106-
let serviceId = makeServiceId()
1107-
let ad1 = makeAdvertisement($serviceId)
1108-
let ad2 = makeAdvertisement($serviceId)
1084+
let serviceName = "service"
1085+
let serviceId = serviceName.hashServiceId()
1086+
let ad1 = makeAdvertisement(serviceName)
1087+
let ad2 = makeAdvertisement(serviceName)
11091088
let now = Moment.now()
11101089

11111090
disco.acceptAdvertisement(now, serviceId, ad1)
@@ -1116,34 +1095,23 @@ suite "Service Discovery Registrar - acceptAdvertisement seqNo handling":
11161095
test "seqNo replacement updates IP tree correctly":
11171096
let disco =
11181097
setupServiceDiscoveryNode(discoConfig = ServiceDiscoveryConfig.new(fReturn = 3))
1119-
let serviceId = makeServiceId()
1098+
let serviceName = "service"
1099+
let serviceId = serviceName.hashServiceId()
11201100
let privateKey = PrivateKey.random(rng()).get()
1121-
let peerId = PeerId.init(privateKey).get()
11221101
let now = Moment.now()
11231102

1124-
let oldAd = SignedExtendedPeerRecord
1125-
.init(
1126-
privateKey,
1127-
ExtendedPeerRecord(
1128-
peerId: peerId,
1129-
seqNo: 1,
1130-
addresses: @[AddressInfo(address: makeMultiAddress("10.0.0.1"))],
1131-
services: @[],
1132-
),
1133-
)
1134-
.get()
1135-
1136-
let newAd = SignedExtendedPeerRecord
1137-
.init(
1138-
privateKey,
1139-
ExtendedPeerRecord(
1140-
peerId: peerId,
1141-
seqNo: 2,
1142-
addresses: @[AddressInfo(address: makeMultiAddress("10.0.0.2"))],
1143-
services: @[],
1144-
),
1145-
)
1146-
.get()
1103+
let oldAd = makeAdvertisement(
1104+
serviceName,
1105+
privateKey = privateKey,
1106+
seqNo = 1,
1107+
addrs = @[makeMultiAddress("10.0.0.1")],
1108+
)
1109+
let newAd = makeAdvertisement(
1110+
serviceName,
1111+
privateKey = privateKey,
1112+
seqNo = 2,
1113+
addrs = @[makeMultiAddress("10.0.0.2")],
1114+
)
11471115

11481116
disco.acceptAdvertisement(now, serviceId, oldAd)
11491117
let counterAfterFirst = disco.registrar.ads.ipTotal
@@ -1218,10 +1186,11 @@ suite "Service Discovery Registrar - AdvertisementCache put":
12181186

12191187
test "higher seqNo replaces ad, updates timestamps and IP tree, returns AdReplaced":
12201188
let ads = AdvertisementCache.new()
1221-
let serviceId = makeServiceId()
1189+
let serviceName = "service"
1190+
let serviceId = serviceName.hashServiceId()
12221191
let privateKey = PrivateKey.random(rng()).get()
1223-
let oldAd = makeAdvertisement(privateKey = privateKey, seqNo = 1)
1224-
let newAd = makeAdvertisement(privateKey = privateKey, seqNo = 2)
1192+
let oldAd = makeAdvertisement(serviceName, privateKey = privateKey, seqNo = 1)
1193+
let newAd = makeAdvertisement(serviceName, privateKey = privateKey, seqNo = 2)
12251194

12261195
check ads.put(serviceId, oldAd, initMoment(1000), HighCap) == AdInserted
12271196
check ads.put(serviceId, newAd, initMoment(2000), HighCap) == AdReplaced
@@ -1234,13 +1203,20 @@ suite "Service Discovery Registrar - AdvertisementCache put":
12341203

12351204
test "higher seqNo with address swap keeps IP multi-set size stable":
12361205
let ads = AdvertisementCache.new()
1237-
let serviceId = makeServiceId()
1206+
let serviceName = "service"
1207+
let serviceId = serviceName.hashServiceId()
12381208
let privateKey = PrivateKey.random(rng()).get()
12391209
let oldAd = makeAdvertisement(
1240-
privateKey = privateKey, seqNo = 1, addrs = @[makeMultiAddress("10.0.0.1")]
1210+
serviceName,
1211+
privateKey = privateKey,
1212+
seqNo = 1,
1213+
addrs = @[makeMultiAddress("10.0.0.1")],
12411214
)
12421215
let newAd = makeAdvertisement(
1243-
privateKey = privateKey, seqNo = 2, addrs = @[makeMultiAddress("192.168.1.1")]
1216+
serviceName,
1217+
privateKey = privateKey,
1218+
seqNo = 2,
1219+
addrs = @[makeMultiAddress("192.168.1.1")],
12441220
)
12451221

12461222
discard ads.put(serviceId, oldAd, initMoment(1000), HighCap)
@@ -1252,10 +1228,11 @@ suite "Service Discovery Registrar - AdvertisementCache put":
12521228

12531229
test "lower seqNo leaves cache unchanged, returns AdIgnored":
12541230
let ads = AdvertisementCache.new()
1255-
let serviceId = makeServiceId()
1231+
let serviceName = "service"
1232+
let serviceId = serviceName.hashServiceId()
12561233
let privateKey = PrivateKey.random(rng()).get()
1257-
let currentAd = makeAdvertisement(privateKey = privateKey, seqNo = 10)
1258-
let staleAd = makeAdvertisement(privateKey = privateKey, seqNo = 5)
1234+
let currentAd = makeAdvertisement(serviceName, privateKey = privateKey, seqNo = 10)
1235+
let staleAd = makeAdvertisement(serviceName, privateKey = privateKey, seqNo = 5)
12591236

12601237
discard ads.put(serviceId, currentAd, initMoment(1000), HighCap)
12611238
check ads.put(serviceId, staleAd, initMoment(2000), HighCap) == AdIgnored
@@ -1266,14 +1243,23 @@ suite "Service Discovery Registrar - AdvertisementCache put":
12661243

12671244
test "higher seqNo replaces stale copies in every other service that cached it":
12681245
let ads = AdvertisementCache.new()
1269-
let serviceId1 = makeServiceId(1)
1270-
let serviceId2 = makeServiceId(2)
1246+
let serviceName1 = "service-a"
1247+
let serviceName2 = "service-b"
1248+
let serviceId1 = serviceName1.hashServiceId()
1249+
let serviceId2 = serviceName2.hashServiceId()
12711250
let privateKey = PrivateKey.random(rng()).get()
1272-
let oldAd = makeAdvertisement(
1273-
privateKey = privateKey, seqNo = 1, addrs = @[makeMultiAddress("10.0.0.1")]
1251+
let services = @[makeServiceInfo(serviceName1), makeServiceInfo(serviceName2)]
1252+
let oldAd = makeAdvertisementWithServices(
1253+
services,
1254+
privateKey = privateKey,
1255+
seqNo = 1,
1256+
addrs = @[makeMultiAddress("10.0.0.1")],
12741257
)
1275-
let newAd = makeAdvertisement(
1276-
privateKey = privateKey, seqNo = 2, addrs = @[makeMultiAddress("192.168.1.1")]
1258+
let newAd = makeAdvertisementWithServices(
1259+
services,
1260+
privateKey = privateKey,
1261+
seqNo = 2,
1262+
addrs = @[makeMultiAddress("192.168.1.1")],
12771263
)
12781264

12791265
discard ads.put(serviceId1, oldAd, initMoment(1000), HighCap)
@@ -1288,6 +1274,43 @@ suite "Service Discovery Registrar - AdvertisementCache put":
12881274
check newAd.toAdvertisementKey() in ads
12891275
check ads.ipTotal == 2
12901276

1277+
test "higher seqNo drops services the new ad no longer advertises":
1278+
let ads = AdvertisementCache.new()
1279+
let serviceName1 = "service-a"
1280+
let serviceName2 = "service-b"
1281+
let serviceId1 = serviceName1.hashServiceId()
1282+
let serviceId2 = serviceName2.hashServiceId()
1283+
let privateKey = PrivateKey.random(rng()).get()
1284+
let oldAd = makeAdvertisementWithServices(
1285+
@[makeServiceInfo(serviceName1), makeServiceInfo(serviceName2)],
1286+
privateKey = privateKey,
1287+
seqNo = 1,
1288+
addrs = @[makeMultiAddress("10.0.0.1")],
1289+
)
1290+
# Newer ad only claims service-a.
1291+
let newAd = makeAdvertisementWithServices(
1292+
@[makeServiceInfo(serviceName1)],
1293+
privateKey = privateKey,
1294+
seqNo = 2,
1295+
addrs = @[makeMultiAddress("10.0.0.1")],
1296+
)
1297+
1298+
discard ads.put(serviceId1, oldAd, initMoment(1000), HighCap)
1299+
discard ads.put(serviceId2, oldAd, initMoment(1000), HighCap)
1300+
check ads.ipTotal == 2
1301+
check ads.serviceAdCount(serviceId2) == 1
1302+
1303+
check ads.put(serviceId1, newAd, initMoment(2000), HighCap) == AdReplaced
1304+
1305+
check:
1306+
ads.serviceAdCount(serviceId1) == 1
1307+
ads.adsForService(serviceId1)[0].data.seqNo == 2
1308+
ads.serviceAdCount(serviceId2) == 0
1309+
not ads.containsService(serviceId2)
1310+
oldAd.toAdvertisementKey() notin ads
1311+
newAd.toAdvertisementKey() in ads
1312+
ads.ipTotal == 1
1313+
12911314
test "inserts ad into cache, IP tree, and timestamps":
12921315
let ads = AdvertisementCache.new()
12931316
let serviceId = makeServiceId()

0 commit comments

Comments
 (0)