Bug fix/sync multiple tries #1864

sasurobert · 2020-06-04T10:17:45Z

sync multiple tries at the same time - on go routines.

The base branch was changed.

SebastianMarian · 2020-06-04T10:45:47Z

data/syncer/userAccountsSyncer.go

+	u.syncerMutex.Lock()
+	dataTrie, err := trie.NewTrie(u.trieStorageManager, u.marshalizer, u.hasher, u.maxTrieLevelInMemory)
+	if err != nil {
+		return err


u.syncerMutex.Unlock() before return

SebastianMarian · 2020-06-04T10:46:19Z

data/syncer/userAccountsSyncer.go

+	u.dataTries[string(rootHash)] = dataTrie
+	trieSyncer, err := trie.NewTrieSyncer(u.requestHandler, u.cacher, dataTrie, u.shardId, factory.AccountTrieNodesTopic)
+	if err != nil {
+		return err


u.syncerMutex.Unlock() before return

SebastianMarian · 2020-06-04T10:52:13Z

data/syncer/userAccountsSyncer.go

+			}
+		}(rootHash)
+
+		errMutex.Lock()


Missing errMutex.Unlock() after line 128. I suggest to replace the code between lines 123-128 with:

errMutex.RLock() returnErr := errFound errMutex.RUnlock() if returnErr != nil { return returnErr }

SebastianMarian · 2020-06-04T10:54:07Z

dataRetriever/requestHandlers/requestHandler.go

@@ -28,6 +31,10 @@ type resolverRequestHandler struct {
 	sweepTime             time.Time
 	requestInterval       time.Duration
 	mutSweepTime          sync.Mutex
+
+	trieHashAccumulator [][]byte


trieHashesAcumulator ?

SebastianMarian · 2020-06-04T11:02:42Z

dataRetriever/requestHandlers/requestHandler.go

+
+	rrh.trieHashAccumulator = append(rrh.trieHashAccumulator, unrequestedHashes...)
+	elapsedTime := time.Since(rrh.lastTrieRequestTime)
+	if len(rrh.trieHashAccumulator) < minHashesToRequest && elapsedTime < timeToAccumulateTrieHashes {


What if the last bulk of trieHashes is smaller than 10? Let say 8. This will make the method to return in this point. It would be called again for the same 8 hashes after a while?

If yes, they would be added again in rrh.trieHashAccumulator, and now we have 16, but they are duplicates.

If not, they would be not requested anymore.

I think you should either call: rrh.addRequestedItems(rrh.trieHashAccumulator) before return or better rrh.trieHashAccumulator = rrh.trieHashAccumulator[:0] and add them again in the next call

There are also other exit points below so I think you should put this line: rrh.trieHashAccumulator = rrh.trieHashAccumulator[:0], in a defer method above and remove it from line 400

the request trie hashes method is called in a continuous way at every second. Thus the accumulated hashes will be surely requested.

Then you need only to manage the duplicate hashes added in rrh.trieHashAccumulator after each call

done with MAP.

SebastianMarian · 2020-06-04T11:09:52Z

dataRetriever/requestHandlers/requestHandler.go

+		return
+	}
+
+	rrh.lastTrieRequestTime = time.Now()


Move this to line 391 where you are sure that the rrh.requestHashesWithDataSplit(rrh.trieHashAccumulator, trieResolver) would be executed

I would make requestHashesWithDataSplit to return error at least if the SplitDataInChunks call fails. And set the rrh.lastTrieRequestTime = time.Now() only if the request has been done with success.

SebastianMarian · 2020-06-04T11:47:35Z

dataRetriever/requestHandlers/requestHandler.go

@@ -18,6 +18,9 @@ var _ epochStart.RequestHandler = (*resolverRequestHandler)(nil)

 var log = logger.GetOrCreate("dataretriever/requesthandlers")

+const minHashesToRequest = 10


Please replace line 456 from addRequestedItems method with continue instead return. This will fix the situation when the call of rrh.requestedItemsHandler.Add for a key, fails for whatever reason, and because of this the all others keys would not be added in the requested items list.

SebastianMarian · 2020-06-04T13:02:23Z

dataRetriever/requestHandlers/requestHandler.go

@@ -150,7 +152,10 @@ func (rrh *resolverRequestHandler) requestHashesWithDataSplit(
 				"batch size", len(batch),
 			)
 		}
+		return err


Move this return after line 153, or better remove it at all so some of them would be requested

SebastianMarian · 2020-06-04T13:12:19Z

dataRetriever/requestHandlers/requestHandler.go

@@ -121,15 +121,17 @@ func (rrh *resolverRequestHandler) requestByHashes(destShardID uint32, hashes []

 	rrh.whiteList.Add(unrequestedHashes)

-	go rrh.requestHashesWithDataSplit(unrequestedHashes, txResolver)
+	go func() {


You can replace 124-126 with:

err = rrh.requestHashesWithDataSplit(unrequestedHashes, txResolver) if err != nil { return }

As all the exported methods from this file, which call requestByHashes method, are called already on go routines

SebastianMarian · 2020-06-04T13:13:36Z

dataRetriever/requestHandlers/requestHandler.go

+		index++
+	}
+
+	rrh.whiteList.Add(itemsToRequest)


Move this in line 411 to not add duplicate items if multiple calls are done and all of them exit in line 382, 399 or 405

moved addRequestedItems - whitelisting would leave to the top.

SebastianMarian · 2020-06-04T13:14:33Z

dataRetriever/requestHandlers/requestHandler.go

+	}
+
+	rrh.whiteList.Add(itemsToRequest)
+	rrh.addRequestedItems(itemsToRequest)


Move this in line 416 to be done only if the request of these items is done

moved addRequestedItems - whitelisting would leave to the top.

SebastianMarian · 2020-06-04T13:22:49Z

data/syncer/userAccountsSyncer.go

-		errMutex.Lock()
-		returnErr := errFound
-		errMutex.Unlock()
+	errMutex.Lock()


You can replace 124-132 with:

errMutex.RLock() defer errMutex.RUnlock() return errFound

consensus/mock/cacherMock.go

data/mock/cacherMock.go

dataRetriever/mock/cacherStub.go

data/mock/uint64CacherStub.go

storage/txcache/txCache.go

SebastianMarian · 2020-06-04T19:17:58Z

storage/txcache/txCache.go

 	log.Error("TxCache.RegisterHandler is not implemented")
 }

+// RegisterHandler is not implemented


UnRegisterHandler

storage/txcache/txCache.go

update/mock/cacherMock.go

update/mock/cacherStub.go

iulianpascalau · 2020-06-04T19:29:51Z

core/common.go

@@ -12,3 +16,10 @@ func EmptyChannel(ch chan bool) int {
 		}
 	}
 }
+
+// UniqueIdentifier returns a unique string identifier


might add ...of 32 bytes long
Since this will contain unprintable elements, I guess (for the sake of the re usability) to change the return type in []byte

data/mock/cacherMock.go

SebastianMarian · 2020-06-04T20:20:12Z

dataRetriever/requestHandlers/requestHandler.go

@@ -140,7 +138,6 @@ func (rrh *resolverRequestHandler) requestHashesWithDataSplit(
 			"num txs", len(unrequestedHashes),
 			"max txs to request", rrh.maxTxsToRequest,
 		)
-		return err


You can call "return" here

would leave as it is - the next for could do nothing as is a range over a nil.

…ore hand for metachain.

LucianMincu

System tests passed.

sasurobert added 2 commits June 4, 2020 13:02

start multiple trie syncers

40ea2ed

small fixes

43d4ef9

sasurobert changed the base branch from release-candidate to bugFix/recreate-trie-after-shuffle-out June 4, 2020 10:22

iulianpascalau self-requested a review June 4, 2020 10:22

SebastianMarian self-requested a review June 4, 2020 10:22

iulianpascalau previously approved these changes Jun 4, 2020

View reviewed changes

clean accumulated trie hashes

6d720b3

Base automatically changed from bugFix/recreate-trie-after-shuffle-out to release-candidate June 4, 2020 10:43

Merge branch 'release-candidate' into bugFix/sync-multiple-tries

0d73b65

SebastianMarian reviewed Jun 4, 2020

View reviewed changes

sasurobert added 2 commits June 4, 2020 15:44

small fixes.

b26eb67

error only last

47cbc1a

SebastianMarian reviewed Jun 4, 2020

View reviewed changes

fix after review.

bfcef6f

SebastianMarian previously approved these changes Jun 4, 2020

View reviewed changes

go routine start

96c0346

sasurobert dismissed SebastianMarian’s stale review via 96c0346 June 4, 2020 15:18

sasurobert added 2 commits June 4, 2020 18:40

waitgroup

d1a6c27

register and unregister handler

0ca11a3

SebastianMarian reviewed Jun 4, 2020

View reviewed changes

iulianpascalau reviewed Jun 4, 2020

View reviewed changes

SebastianMarian reviewed Jun 4, 2020

View reviewed changes

iulianpascalau assigned sasurobert Jun 4, 2020

iulianpascalau added the type:bug Something isn't working label Jun 4, 2020

sasurobert added 3 commits June 5, 2020 09:44

no need to register for intercepted nodes

bcd2e00

create trie DB only for shard in case of requests - as it is done bef…

075fa3b

…ore hand for metachain.

comment changes.

21eeac9

iulianpascalau approved these changes Jun 5, 2020

View reviewed changes

SebastianMarian approved these changes Jun 5, 2020

View reviewed changes

LucianMincu approved these changes Jun 5, 2020

View reviewed changes

LucianMincu merged commit 5f2bb92 into release-candidate Jun 5, 2020

LucianMincu deleted the bugFix/sync-multiple-tries branch June 5, 2020 13:02

		@@ -18,6 +18,9 @@ var _ epochStart.RequestHandler = (*resolverRequestHandler)(nil)

		var log = logger.GetOrCreate("dataretriever/requesthandlers")

		const minHashesToRequest = 10

Bug fix/sync multiple tries #1864

Bug fix/sync multiple tries #1864

Conversation

sasurobert commented Jun 4, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianMarian Jun 4, 2020 • edited

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

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

SebastianMarian Jun 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucianMincu left a comment

Choose a reason for hiding this comment

SebastianMarian Jun 4, 2020 •

edited

SebastianMarian Jun 4, 2020 •

edited