epochStart: fix pending mininBlocks for scheduled #3717

AdoAdoAdo · 2022-01-21T17:31:43Z

No description provided.

…o fix-processed-mbs-scheduled

iulianpascalau · 2022-01-26T16:07:02Z

epochStart/bootstrap/shardStorageHandler.go

+	return pendingMbHashes
+}
+
+func (ssh *shardStorageHandler) updateProcessedMiniBlocksForScheduled(


unit tests for all the new methods?

iulianpascalau · 2022-01-26T16:07:16Z

epochStart/bootstrap/shardStorageHandler.go

+	processedMbHashes := make([][]byte, 0)
+	miniBlocksDstMe := getAllMiniBlocksWithDst(metaBlock, destShardID)
+	for hash, mb := range miniBlocksDstMe {
+		if _, hashExists := pendingMBsMap[hash]; hashExists {


multiple lines

# Conflicts: # dataRetriever/chainStorer_test.go

…on component

…eduled

raduchis · 2022-01-27T09:42:15Z

epochStart/bootstrap/shardStorageHandler.go

+	return pendingMbHashes
+}
+
+func (ssh *shardStorageHandler) updateProcessedMiniBlocksForScheduled(


raduchis · 2022-01-27T09:45:19Z

epochStart/bootstrap/shardStorageHandler.go

+	noPendingMbs := make(map[string]struct{})
+	remainingPendingMiniBlocks := make([]bootstrapStorage.PendingMiniBlocksInfo, 0)
+	for index, metaBlockHash := range referencedMetaBlockHashes {
+		if index == 0 {


index == 0 means that this is the oldest block and thats why there are no pending, everything is processed?

First referenced metablock in the notarized shard block header will be pending when trying to re-process the notarized shard block header.
we want here to remove only the follow up referenced MetaHeaders pending mbs from initial pending list, as these will be naturally added when processing the notarized shard block.

raduchis · 2022-01-27T10:05:41Z

epochStart/bootstrap/shardStorageHandler.go

+		if metaBlock.ShardInfo[i].ShardID == destId {
+			continue
+		}
+
 		for _, val := range metaBlock.ShardInfo[i].ShardMiniBlockHeaders {
 			isCrossShardDestMe := val.ReceiverShardID == destId && val.SenderShardID != destId


should the isCrossShardDestMe := (val.ReceiverShardID == destId || **val.ReceiverShardID == core.AllShardId**) && val.SenderShardID != destId from line 616 be used also here?

the name of the function here is a bit misleading

this intent of this method is to only returns the miniBlocks with destID as receiver shard, but only ones that are pending to destId shard due to their inclusion in this metablock header.

the miniBlocks with core.AllShardId as destination become pending for a shard only when included in the MetaBlock.MiniBlockHeaders, not when processed by other shards.

renamed to getNewPendingMiniBlocksForDst

raduchis · 2022-01-27T10:17:51Z

process/block/preprocess/gasComputation.go

 	gc.mutGasProvided.Unlock()
 }

 // SetGasProvidedAsScheduled sets gas provided as scheduled for a given hash
 func (gc *gasComputation) SetGasProvidedAsScheduled(gasProvided uint64, hash []byte) {
 	gc.mutGasProvided.Lock()
 	gc.gasProvidedAsScheduled[string(hash)] = gasProvided
+	gc.txHashesWithGasProvidedAsScheduledSinceLastReset = append(gc.txHashesWithGasProvidedAsScheduledSinceLastReset, hash)


no need for temporary debug also for this line?

//TODO: Remove it or set log level to Trace log.Debug("gasComputation.SetGasProvidedAsScheduled", "tx hash", hash, "gas gasProvided", gasProvided)

Actually the problem happens only with the gas refunded and gas penalized

raduchis · 2022-01-27T10:21:01Z

process/block/preprocess/transactions.go

 			}
+
+			txs.gasHandler.RestoreGasSinceLastReset()


the logic before for scheduledMode called only removedTheGasProvidedAsScheduled. Now there will be a reset for all gascomputations. Is it ok?

The mechanism has changed and the Reset method which is called before starting to process, resets all four lists of tx hashes for gasProvided, gasProvidedAsScheduled, gasRefunded and gasPenalized. Then, when processing is started the Set method for gasProvided, gasProvidedAsScheduled, gasRefunded and gasPenalized is called accordingly with the current mode (scheduled or not). So, the RestoreGasSinceLastReset will have nothing to restore for gasProvidedAsScheduled when the scheduled mode is false and vice-versa, will have nothing to restore for gasProvided, gasRefunded and gasPenalized when the scheduled mode is true. In case of scheduled mode set on true, the transaction is not really executed, just the basic checks are done and because of that the only thing which could be calculated is gasProvidedAsScheduled. Afterwards, the scheduled txs are executed and resulted scrs, accumulated fees, developer fees, etc., are added in the next block.

* Optimized time used for scheduled txs execution

…o fix-processed-mbs-scheduled

…round duration)

…k/elrond-go into fix-processed-mbs-scheduled

iulianpascalau · 2022-01-27T16:55:19Z

cmd/node/config/config.toml

@@ -672,7 +672,7 @@
 # Consensus type which will be used (the current implementation can manage "bn" and "bls")
 # When consensus type is "bls" the multisig hasher type should be "blake2b"
 [Consensus]
-   ScheduledExecutionMilliseconds = 1500
+   ScheduledExecutionMilliseconds = 6000 # round time duration in milliseconds


This is confusing. It supposed to mean "maximum duration to execute scheduled transactions" or a "general timestamp" at which you start counting.

Removed as we don't need it anymore after the refactor of the txs scheduled execution timing mechanism

iulianpascalau · 2022-01-27T17:01:54Z

process/block/preprocess/gasComputation.go

-	mutGasRefunded         sync.RWMutex
-	gasPenalized           map[string]uint64
-	mutGasPenalized        sync.RWMutex
+	gasProvided                                      map[string]uint64


refactoring idea for next PRs in order to avoid having a lot of fields along with their protective mutexes (which, improperly used can cause deadlocks):
create ConcurrentSafeBytesSlice or ConcurrentSafeStringUint64Map that can internally hold the mutexes and have some exported functions that can safely lock/unlock the mutexes

Yep, should be done this way

iulianpascalau · 2022-01-27T17:03:23Z

process/block/preprocess/gasComputation.go

 	gc.mutGasRefunded.Unlock()
+
+	//TODO: Remove it or set log level to Trace


resolve TODO?

We are hunting a rare situation, so I will keep this log on Trace, which anyway should not appear too often, just to catch this error which seems to come from sc execution

iulianpascalau · 2022-01-27T17:03:33Z

process/block/preprocess/gasComputation.go

 	gc.mutGasPenalized.Unlock()
+
+	//TODO: Remove it or set log level to Trace


resolve TODO?

We are hunting a rare situation, so I will keep this log on Trace, which anyway should not appear too often, just to catch this error which seems to come from sc execution

iulianpascalau · 2022-01-27T17:04:04Z

process/block/preprocess/smartContractResults.go

-			scr.gasHandler.RemoveGasRefunded(processedTxHashes)
-			scr.gasHandler.RemoveGasPenalized(processedTxHashes)
+			for _, hash := range processedTxHashes {
+				//TODO: Remove it or set log level to Trace


resolve TODO?

We are hunting a rare situation, so I will keep this log on Trace, which anyway should not appear too often, just to catch this error which seems to come from sc execution

iulianpascalau · 2022-01-27T17:04:13Z

process/block/preprocess/transactions.go

-				txs.gasHandler.RemoveGasRefunded(processedTxHashes)
-				txs.gasHandler.RemoveGasPenalized(processedTxHashes)
+			for _, hash := range processedTxHashes {
+				//TODO: Remove it or set log level to Trace


resolve TODO?

We are hunting a rare situation, so I will keep this log on Trace, which anyway should not appear too often, just to catch this error which seems to come from sc execution

* Fixed situation when proposer would not include SCRs from previous block which had scheduled txs execution, because it is running out of time even before starting the creation of the block

…k/elrond-go into fix-processed-mbs-scheduled

…o fix-processed-mbs-scheduled

iulianpascalau · 2022-01-28T11:45:48Z

process/block/shardblock.go

-	log.Debug("elapsed time to create mbs to me",
-		"time [s]", elapsedTime,
-	)
+	log.Debug("elapsed time to create mbs to me", "time [s]", elapsedTime)


maybe remove [s] ? the time will contain the measuring unit by default

iulianpascalau · 2022-01-28T11:51:27Z

process/block/shardblock.go

-	log.Debug("elapsed time to create mbs from me",
-		"time [s]", elapsedTime,
-	)
+	log.Debug("elapsed time to create mbs from me", "time [s]", elapsedTime)


iulianpascalau · 2022-01-28T11:54:14Z

process/track/baseBlockTrack.go

@@ -828,7 +828,7 @@ func getCrossShardMiniblockKeys(miniBlockHdrs []data.MiniBlockHeaderHandler, sel
 	keys := make([][]byte, 0)
 	for _, miniBlockHdr := range miniBlockHdrs {
 		receiverShard := miniBlockHdr.GetReceiverShardID()
-		receiverIsSelfShard := receiverShard == selfShardID || receiverShard == core.AllShardId && processingShard == core.MetachainShardId
+		receiverIsSelfShard := (receiverShard == selfShardID || receiverShard == core.AllShardId) && processingShard == core.MetachainShardId


…k/elrond-go into fix-processed-mbs-scheduled

…o fix-processed-mbs-scheduled

LucianMincu

System tests passed.

AdoAdoAdo and others added 2 commits January 21, 2022 19:30

epochStart: fix pending mininBlocks for scheduled

268fa2f

* Added some guard conditions, removed unused code, fixed unit tests

c2f54c6

SebastianMarian previously approved these changes Jan 21, 2022

View reviewed changes

* Updated pending mini blocks for scheduled as well

f1bf7bb

SebastianMarian dismissed their stale review via f1bf7bb January 21, 2022 19:47

SebastianMarian and others added 8 commits January 22, 2022 21:46

* Fixed wrong block type set for scheduled SCRs

5113ade

* Added more logs

ba2d924

* Fixed extension of persister life

9e460f9

* Fixed Get from pruning storer

08a857b

* Fixed unit test

aabbc14

fix revert to genesis block

2c00d5a

* Changed SnapshotsGoroutineNum from 10 to 200

bf07a69

Merge remote-tracking branch 'origin/fix-processed-mbs-scheduled' int…

e5dd736

…o fix-processed-mbs-scheduled

iulianpascalau reviewed Jan 26, 2022

View reviewed changes

SebastianMarian and others added 7 commits January 26, 2022 22:37

* Fixed edge cases of reverting gas provided, refunded and penalized

9fea6d8

Merge branch 'development' into fix-processed-mbs-scheduled

387dd90

# Conflicts: # dataRetriever/chainStorer_test.go

* Added some logs

668ec5e

* Refactored some variables/methods names

dbb22e4

* Added unit tests for new functionality implemented in gas computati…

f7b0c61

…on component

Merge branch 'feat/scheduled-sc-execution' into fix-processed-mbs-sch…

9f799c0

…eduled

epochStart: extract common code

68dbbfd

raduchis reviewed Jan 27, 2022

View reviewed changes

SebastianMarian and others added 7 commits January 27, 2022 13:10

* Added unit tests for UpdatePendingMiniBlocksForScheduled

43bca42

* Optimized time used for scheduled txs execution

Merge remote-tracking branch 'origin/fix-processed-mbs-scheduled' int…

69d6ebc

…o fix-processed-mbs-scheduled

* Changed ScheduledExecutionMilliseconds value to 6000 milliseconds (…

82966e1

…round duration)

epochStart: add some debug logs

2987539

Merge branch 'fix-processed-mbs-scheduled' of github.com:ElrondNetwor…

6ca0444

…k/elrond-go into fix-processed-mbs-scheduled

epochStart: log data

7f94d37

epochStart: fix remove hashes and add some unit tests

e6bc23c

iulianpascalau requested changes Jan 27, 2022

View reviewed changes

SebastianMarian and others added 7 commits January 28, 2022 12:38

* Fixed after review

95f966d

* Fixed situation when proposer would not include SCRs from previous block which had scheduled txs execution, because it is running out of time even before starting the creation of the block

epochStart: sync previous to first referenced meta block

59ab8d7

Merge branch 'fix-processed-mbs-scheduled' of github.com:ElrondNetwor…

57c365c

…k/elrond-go into fix-processed-mbs-scheduled

epochStart: remove commented code

6a256f0

epochStart: renaming function

26e258c

* Refactored some variables/methods names and rearrange some code

f34d0cf

Merge remote-tracking branch 'origin/fix-processed-mbs-scheduled' int…

5c40182

…o fix-processed-mbs-scheduled

iulianpascalau previously approved these changes Jan 28, 2022

View reviewed changes

AdoAdoAdo added 2 commits January 28, 2022 14:03

epochStart, process: cleanup some logs

1dca107

Merge branch 'fix-processed-mbs-scheduled' of github.com:ElrondNetwor…

e82fe1c

…k/elrond-go into fix-processed-mbs-scheduled

AdoAdoAdo dismissed iulianpascalau’s stale review via e82fe1c January 28, 2022 12:03

iulianpascalau previously approved these changes Jan 28, 2022

View reviewed changes

raduchis previously approved these changes Jan 28, 2022

View reviewed changes

SebastianMarian added 2 commits January 28, 2022 14:25

* Fixed after review

b70b901

Merge remote-tracking branch 'origin/fix-processed-mbs-scheduled' int…

7c7647d

…o fix-processed-mbs-scheduled

SebastianMarian dismissed stale reviews from raduchis and iulianpascalau via 7c7647d January 28, 2022 12:29

* Fixed receiver shard condition for AllShardsId

888e7b7

raduchis approved these changes Jan 28, 2022

View reviewed changes

iulianpascalau approved these changes Jan 28, 2022

View reviewed changes

LucianMincu approved these changes Jan 28, 2022

View reviewed changes

LucianMincu merged commit 8c07de7 into feat/scheduled-sc-execution Jan 28, 2022

LucianMincu deleted the fix-processed-mbs-scheduled branch January 28, 2022 19:51

		gc.mutGasRefunded.Unlock()

		//TODO: Remove it or set log level to Trace

		gc.mutGasPenalized.Unlock()

		//TODO: Remove it or set log level to Trace

epochStart: fix pending mininBlocks for scheduled #3717

epochStart: fix pending mininBlocks for scheduled #3717

Conversation

AdoAdoAdo commented Jan 21, 2022

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

AdoAdoAdo Jan 28, 2022 • 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

SebastianMarian Jan 28, 2022 • 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

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

LucianMincu left a comment

Choose a reason for hiding this comment

AdoAdoAdo Jan 28, 2022 •

edited

SebastianMarian Jan 28, 2022 •

edited