Optimized peer authentication messages management #4420

iulianpascalau · 2022-08-31T11:02:51Z

Description of the reasoning behind the pull request

optimized peer authentication messages management by indexing the messages in the pool by their public key. In this way unnecessary get operations are not done on the resolvers.
simplified interceptedHeartbeat.go
fixed timeCacher implementation to use a put function instead of upsert as to properly override existing values with newer ones
removed unnecessary function from peerShardMapper that was previously used just in the peer authentication resolver that was simplified

Proposed Changes

code simplification as explained above

Testing procedure

standard testing procedure

codecov-commenter · 2022-08-31T16:45:05Z

Codecov Report

Merging #4420 (3b74080) into rc/2022-july (8f960bd) will increase coverage by 0.01%.
The diff coverage is 95.08%.

@@               Coverage Diff                @@
##           rc/2022-july    #4420      +/-   ##
================================================
+ Coverage         73.92%   73.94%   +0.01%     
================================================
  Files               677      677              
  Lines             86884    86877       -7     
================================================
+ Hits              64230    64241      +11     
+ Misses            17855    17841      -14     
+ Partials           4799     4795       -4

Impacted Files	Coverage Δ
...esolverscontainer/baseResolversContainerFactory.go	`86.72% <ø> (-0.19%)`	⬇️
...esolverscontainer/metaResolversContainerFactory.go	`82.32% <ø> (-0.09%)`	⬇️
...solverscontainer/shardResolversContainerFactory.go	`87.87% <ø> (-0.08%)`	⬇️
sharding/networksharding/peerShardMapper.go	`85.24% <ø> (+0.55%)`	⬆️
process/heartbeat/interceptedHeartbeat.go	`82.66% <50.00%> (-2.15%)`	⬇️
factory/processComponents.go	`53.56% <66.66%> (+<0.01%)`	⬆️
...aRetriever/resolvers/peerAuthenticationResolver.go	`96.34% <100.00%> (+1.04%)`	⬆️
epochStart/bootstrap/process.go	`85.61% <100.00%> (-0.02%)`	⬇️
heartbeat/sender/peerAuthenticationSender.go	`92.92% <100.00%> (+0.22%)`	⬆️
process/heartbeat/interceptedPeerAuthentication.go	`98.50% <100.00%> (+6.02%)`	⬆️
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

storage/timecache/timeCacheCore.go

mariusmihaic · 2022-08-31T18:24:17Z

storage/timecache/timeCacher.go

+		return
+	}
+
+	return


This func always returns nothing. Is this intended?

Also, could've used logIfError

please check that the function has named returned parameters. This is go idiomatic. log.LogIfError does not add sufficient context.
Removed one return, thus

mariusmihaic · 2022-08-31T18:31:37Z

storage/timecache/timeCacheCore.go

@@ -58,7 +58,7 @@ func (tcc *timeCacheCore) upsert(key string, value interface{}, duration time.Du
 }

 // put will add the key, value and provided duration, overriding values if the data already existed
-// It returns if the value existed before this call. It also operates on the locker so the call is concurrent safe
+// It returns true if the value existed before this call. It also operates on the locker so the call is concurrent safe
 func (tcc *timeCacheCore) put(key string, value interface{}, duration time.Duration) (bool, error) {


Why have this func return true if the value existed, since in func (tc *timeCacher) Put you always return false and ignore if this one returns true?

removed bool & optimized

timeCacheCore should normally not care about the existence, nor the logic of its caller (timeCacher). Therefore, it's alright to provide the information to the (any) caller - even if that piece of information isn't used.

andreibancioiu · 2022-08-31T18:31:15Z

storage/timecache/timeCacher.go

@@ -81,7 +81,7 @@ func (tc *timeCacher) Clear() {

 // Put adds a value to the cache. It will always return false since the eviction did not occur
 func (tc *timeCacher) Put(key []byte, value interface{}, _ int) (evicted bool) {
-	_, err := tc.timeCache.upsert(string(key), value, tc.timeCache.defaultSpan)
+	_, err := tc.timeCache.put(string(key), value, tc.timeCache.defaultSpan)


So all callers of timeCacher.Put() will receive this fix. Should be only positive impact, correct?

right, this was the actual fix, a complete rewrite of the existing data

andreibancioiu · 2022-08-31T18:31:34Z

storage/timecache/timeCacher.go

@@ -115,14 +115,15 @@ func (tc *timeCacher) Peek(key []byte) (value interface{}, ok bool) {
 // HasOrAdd checks if a key is in the cache.
 // If key exists, does not update the value. Otherwise, adds the key-value in the cache
 func (tc *timeCacher) HasOrAdd(key []byte, value interface{}, _ int) (has, added bool) {
-	existed, err := tc.timeCache.upsert(string(key), value, tc.timeCache.defaultSpan)
+	var err error
+	has, added, err = tc.timeCache.hasOrAdd(string(key), value, tc.timeCache.defaultSpan)


So all callers of timeCacher.HasOrAdd() will receive this fix. Should be only positive impact, correct?

andreibancioiu · 2022-08-31T18:32:44Z

storage/timecache/timeCacheCore.go

+
+	_, found := tcc.data[key]
+	if found {
+		return found, false, nil


Could have been true (for symmetry).

andreibancioiu · 2022-08-31T18:33:56Z

storage/timecache/timeCacher.go

@@ -115,14 +115,15 @@ func (tc *timeCacher) Peek(key []byte) (value interface{}, ok bool) {
 // HasOrAdd checks if a key is in the cache.
 // If key exists, does not update the value. Otherwise, adds the key-value in the cache
 func (tc *timeCacher) HasOrAdd(key []byte, value interface{}, _ int) (has, added bool) {
-	existed, err := tc.timeCache.upsert(string(key), value, tc.timeCache.defaultSpan)
+	var err error
+	has, added, err = tc.timeCache.hasOrAdd(string(key), value, tc.timeCache.defaultSpan)


has and added are mutually exclusive, right? Bit of redundancy therefore, but it's good to be explicit (as it is now) 👍

yes, they are exclusive. Since I have to implement the already existing and used interface, I had to return something.
Also, beside (true, false) and (false, true) we also have (false, false) when the provided key is nil or empty, so we had to encode 3 states.

sstanculeanu · 2022-08-31T18:43:58Z

storage/timecache/timeCacheCore.go

@@ -57,6 +57,47 @@ func (tcc *timeCacheCore) upsert(key string, value interface{}, duration time.Du
 	return found, nil
 }

+// put will add the key, value and provided duration, overriding values if the data already existed
+// It returns true if the value existed before this call. It also operates on the locker so the call is concurrent safe


Suggested change

// It returns true if the value existed before this call. It also operates on the locker so the call is concurrent safe

// It also operates on the locker so the call is concurrent safe

removed part of the comment

SebastianMarian · 2022-08-31T18:33:59Z

heartbeat/sender/peerAuthenticationSender.go

@@ -170,6 +170,9 @@ func (sender *peerAuthenticationSender) execute() (error, bool) {
 		return err, isTriggered
 	}

+	log.Debug("sending peer authentication message",


Do we keep this on Debug level?

yes, one message at ~2hours. Helps the debugging a lot.

SebastianMarian · 2022-08-31T18:43:41Z

storage/timecache/timeCacheCore.go

+
+	_, found := tcc.data[key]
+	if found {
+		return found, false, nil


return true, false, nil or at L100 return found, true, nil just to keep the same logic ?

already done in last commit.

# Conflicts: # process/heartbeat/interceptedHeartbeat.go

gabi-vuls

System test passed.

iulianpascalau added 3 commits August 31, 2022 14:01

- optimized peer authentication messages management

e3d6c57

- added prints

7a815f6

-fixed existing print

6c65afe

iulianpascalau changed the title ~~- optimized peer authentication messages management~~ Optimized peer authentication messages management Aug 31, 2022

iulianpascalau added 3 commits August 31, 2022 17:49

- fixed tests

e71d3d9

- simplified interceptedHeartbeat.go

a07521c

- fixed timeCacher.go to properly use a put function not upsert

db41f5c

iulianpascalau self-assigned this Aug 31, 2022

iulianpascalau added type:bug Something isn't working area:storage area:processing area:heartbeat impact:medium interested:api-multiversx-com interested:protocol interested:validator size:small type:refactoring complexity:medium labels Aug 31, 2022

iulianpascalau added 2 commits August 31, 2022 19:40

- fixed condition

5243547

- simplified peerShardMapper

7752f8c

sstanculeanu previously approved these changes Aug 31, 2022

View reviewed changes

Merge branch 'rc/2022-july' into optimize-peer-auth-messages-management

809d9c7

mariusmihaic self-requested a review August 31, 2022 17:42

mariusmihaic reviewed Aug 31, 2022

View reviewed changes

storage/timecache/timeCacheCore.go Outdated Show resolved Hide resolved

storage/timecache/timeCacheCore.go Outdated Show resolved Hide resolved

storage/timecache/timeCacheCore.go Outdated Show resolved Hide resolved

- fix after review: minir refactoring + added more unit tests

81024e1

iulianpascalau dismissed sstanculeanu’s stale review via 81024e1 August 31, 2022 18:18

SebastianMarian self-requested a review August 31, 2022 18:28

mariusmihaic reviewed Aug 31, 2022

View reviewed changes

andreibancioiu previously approved these changes Aug 31, 2022

View reviewed changes

- fix after review

e4dc56e

iulianpascalau dismissed andreibancioiu’s stale review via e4dc56e August 31, 2022 18:41

andreibancioiu previously approved these changes Aug 31, 2022

View reviewed changes

mariusmihaic previously approved these changes Aug 31, 2022

View reviewed changes

sstanculeanu reviewed Aug 31, 2022

View reviewed changes

SebastianMarian reviewed Aug 31, 2022

View reviewed changes

- fix after review: fixed comment

440f0ac

iulianpascalau dismissed stale reviews from mariusmihaic and andreibancioiu via 440f0ac August 31, 2022 18:51

sstanculeanu previously approved these changes Aug 31, 2022

View reviewed changes

andreibancioiu previously approved these changes Aug 31, 2022

View reviewed changes

mariusmihaic previously approved these changes Aug 31, 2022

View reviewed changes

SebastianMarian previously approved these changes Aug 31, 2022

View reviewed changes

Merge branch 'rc/2022-july' into optimize-peer-auth-messages-management

3b74080

# Conflicts: # process/heartbeat/interceptedHeartbeat.go

iulianpascalau dismissed stale reviews from SebastianMarian, mariusmihaic, andreibancioiu, and sstanculeanu via 3b74080 September 1, 2022 10:18

mariusmihaic approved these changes Sep 1, 2022

View reviewed changes

sstanculeanu approved these changes Sep 1, 2022

View reviewed changes

andreibancioiu approved these changes Sep 1, 2022

View reviewed changes

SebastianMarian approved these changes Sep 1, 2022

View reviewed changes

gabi-vuls approved these changes Sep 1, 2022

View reviewed changes

gabi-vuls merged commit 341ab3d into rc/2022-july Sep 1, 2022

gabi-vuls deleted the optimize-peer-auth-messages-management branch September 1, 2022 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimized peer authentication messages management #4420

Optimized peer authentication messages management #4420

iulianpascalau commented Aug 31, 2022 •

edited

Loading

codecov-commenter commented Aug 31, 2022 •

edited

Loading

mariusmihaic Aug 31, 2022

mariusmihaic Aug 31, 2022

iulianpascalau Aug 31, 2022

mariusmihaic Aug 31, 2022

iulianpascalau Aug 31, 2022 •

edited

Loading

andreibancioiu Aug 31, 2022 •

edited

Loading

andreibancioiu Aug 31, 2022

iulianpascalau Aug 31, 2022

andreibancioiu Aug 31, 2022

iulianpascalau Aug 31, 2022

andreibancioiu Aug 31, 2022

iulianpascalau Aug 31, 2022

andreibancioiu Aug 31, 2022 •

edited

Loading

iulianpascalau Aug 31, 2022

sstanculeanu Aug 31, 2022 •

edited

Loading

iulianpascalau Aug 31, 2022

SebastianMarian Aug 31, 2022

iulianpascalau Aug 31, 2022

SebastianMarian Aug 31, 2022

SebastianMarian Aug 31, 2022

iulianpascalau Aug 31, 2022

SebastianMarian Aug 31, 2022

gabi-vuls left a comment

+              		return
+              	}
+              	return

	// It returns true if the value existed before this call. It also operates on the locker so the call is concurrent safe
	// It also operates on the locker so the call is concurrent safe

Optimized peer authentication messages management #4420

Optimized peer authentication messages management #4420

Conversation

iulianpascalau commented Aug 31, 2022 • edited Loading

Description of the reasoning behind the pull request

Proposed Changes

Testing procedure

codecov-commenter commented Aug 31, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iulianpascalau Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

andreibancioiu Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabi-vuls left a comment

Choose a reason for hiding this comment

iulianpascalau commented Aug 31, 2022 •

edited

Loading

codecov-commenter commented Aug 31, 2022 •

edited

Loading

iulianpascalau Aug 31, 2022 •

edited

Loading

andreibancioiu Aug 31, 2022 •

edited

Loading

andreibancioiu Aug 31, 2022 •

edited

Loading

sstanculeanu Aug 31, 2022 •

edited

Loading