Fix block data cleaning from pools #2259

SebastianMarian · 2020-08-30T20:29:16Z

Canceled the removal of block data at the commit time as this approach was forgotten here and its scope was to test an old issue which was proven later to be from somewhere else. As now, there are situations when block data (miniblocks and transactions) are intentionally broadcast with delay, the call of the removeBlockDataFromPool method at commit time, could remove from pool only partial data. Because of this, when the cleanupPools method is called for the headers behind final, the already removed block data at the commit time for earlier blocks, broke some links between all the components from these blocks and they could not be removed anymore completely from pools.

…ach was forgotten here and its scope was to test an old issue which was proven later to be from somewhere else. As now, there are situations when block data (miniblocks and transactions) are intentionally broadcast with delay, the call of the removeBlockDataFromPool method at commit time, could remove from pool only partial data. Because of this, when the cleanupPools method is called for the headers behind final, the already removed block data at the commit time for earlier blocks, broke some links between all the components from these blocks and they could not be removed anymore completely from pools.

LucianMincu

System tests failed

iulianpascalau · 2020-08-31T12:08:47Z

cmd/node/config/config.toml

@@ -541,7 +541,7 @@

 [BlockSizeThrottleConfig]
    MinSizeInBytes = 104857 # 104857 is 10% from 1MB
-    MaxSizeInBytes = 943718 # 943718 is 90% from 1MB
+    MaxSizeInBytes = 891289 # 943718 is 85% from 1MB


…uted transaction is of type smart contract call/deploy - made p2p `buffer too large` error message carry more info - reverted throttling config changes

sasurobert · 2020-08-31T17:59:37Z

process/block/preprocess/transactions.go

@@ -961,7 +961,12 @@ func (txs *transactions) createAndProcessMiniBlocksFromMe(
 		}

 		miniBlock.TxHashes = append(miniBlock.TxHashes, txHash)
-		txs.blockSizeComputation.AddNumTxs(1)
+		numTxs := 1
+		if core.IsSmartContractAddress(tx.RcvAddr) {


this is not the correct way to resolve it. Add blockSizeComputation component to smart contract result post processor and call AddNumTxs there when new cross shard SCRs are generated. It is not ensured that one tx = one scr

When we reach the postprocessor it is already too late as the transactions are already executed and we need to include the scr(s) as well. Indeed, it is not ensured that one tx = one scr but at least it will estimate better.

Actually you will have nothing in post processor when you create a miniblock from me to meta with 27k stake SC calls. But at destination, in this case in meta, could result 54k txs (from which 27k are SCR). Because you need to process at least one mb completely this could result in a stuck situation. So, you also need to multiply at source, with 2 or more (in terms of size, depending of how many SCR could exist), all the SC calls when you create mbs from me. I think that this situation on normal cases should be solved by gas limit and in case of an invalid SC call, adding only one additional tx which simulates the scr created at destination, should be enough as in this case would be only one SCR for each invalid SC call?

Added block size computation taking in consideration also the post process mbs and txs when creating mbs dest me

…st processor miniblocks and transactions

…ls' into Fix-block-data-cleaning-from-pools # Conflicts: # process/block/preprocess/transactions.go

sasurobert

add unit test

sasurobert · 2020-09-01T08:11:52Z

process/block/preprocess/transactions.go

 		txs.blockSizeComputation.AddNumTxs(1)
+		txs.blockSizeComputation.AddNumTxs(numScCalls)


numScCalls here is always 1. call txs.blockSizeComputation.AddNumTxs(1) instead of numSCCalls++. Although this protection should be only for cross shard and per shard.

sasurobert · 2020-09-01T08:13:35Z

process/coordinator/process.go

+
+		mbs := interimProc.CreateAllInterMiniBlocks()
+		for _, mb := range mbs {
+			numTxs += len(mb.TxHashes)


do not add intra shard miniblocks.

process/block/preprocess/transactions.go

process/coordinator/process.go

iulianpascalau · 2020-09-01T17:22:28Z

process/coordinator/process.go

+	numTxs := 0
+	numCrossShardScCalls := 0
+
+	allTxs := make(map[string]data.TransactionHandler)


some unit tests would have been great

Would be added of course. Just waiting for the system test results

…ethod

…ers to how many txs of sc call type, could be included in one block

…ls' into Fix-block-data-cleaning-from-pools

iulianpascalau · 2020-09-03T11:46:51Z

core/constants.go

@@ -505,3 +505,7 @@ const MaxWaitingTimeToReceiveRequestedItem = 5 * time.Second
 // DefaultLogProfileIdentifier represents the default log profile used when the logviewer/termui applications do not
 // need to change the current logging profile
 const DefaultLogProfileIdentifier = "[default log profile]"
+
+// MultiplyFactorForScCall specifies the multiply factor, in terms of number, which should be used for how many sc calls
+// could be included in one block (normal txs -> aprox. 27000, sc calls -> aprox. 2700 with value of 9 set below)


comment or value need to be adjusted

iulianpascalau · 2020-09-03T11:48:03Z

process/block/preprocess/blockSizeComputation_test.go

-		{numNewMiniBlocks: 1, numNewTxs: 27757, expected: true, name: "with miniblocks 1 and txs 20078"},
-		{numNewMiniBlocks: 2, numNewTxs: 27756, expected: true, name: "with miniblocks 2 and txs 20077"},
+		{numNewMiniBlocks: 1, numNewTxs: 27756, expected: false, name: "with miniblocks 1 and txs 27756"},
+		{numNewMiniBlocks: 1, numNewTxs: 27757, expected: true, name: "with miniblocks 1 and txs 27757"},


iulianpascalau · 2020-09-03T11:49:59Z

process/block/preprocess/smartContractResults.go

@@ -476,6 +477,43 @@ func (scr *smartContractResults) ProcessMiniBlock(miniBlock *block.MiniBlock, ha
 			&totalGasConsumedInSelfShard)

 		if err != nil {
+			//TODO: Remove this after testing


can't remove this as it is used for the debug print L499

This was added to serves the prints and also the prints would been removed now

iulianpascalau · 2020-09-03T15:36:57Z

process/block/preprocess/validatorInfoPreProcessor.go

@@ -145,6 +151,13 @@ func (vip *validatorInfoPreprocessor) ProcessMiniBlock(miniBlock *block.MiniBloc
 		return nil, process.ErrValidatorInfoMiniBlockNotFromMeta
 	}

+	if vip.blockSizeComputation.IsMaxBlockSizeWithoutThrottleReached(1, len(miniBlock.TxHashes)) {


We need another function in the blocksize estimator that will better handle the peer changes as those are not hashes. cc @sasurobert

Those are taken as it is. It is hard to estimate what is the actual value

Please add a TODO to fix the estimator. It is not complicated to do so in a separate PR.

The good part is that until then we can reduce if needed the MaxSizeInBytes from [BlockSizeThrottleConfig] in config.toml to 85% from 90% and in this way we should avoid any kind of "message too large" from p2p

sasurobert

CreateSCRIfError is wrongly constructed right now. It is an old code, which is not up to date. Will fix it in separate PR.

sasurobert · 2020-09-03T15:46:07Z

process/block/preprocess/validatorInfoPreProcessor.go

@@ -145,6 +151,13 @@ func (vip *validatorInfoPreprocessor) ProcessMiniBlock(miniBlock *block.MiniBloc
 		return nil, process.ErrValidatorInfoMiniBlockNotFromMeta
 	}

+	if vip.blockSizeComputation.IsMaxBlockSizeWithoutThrottleReached(1, len(miniBlock.TxHashes)) {


Those are taken as it is. It is hard to estimate what is the actual value

AdoAdoAdo · 2020-09-03T16:28:15Z

process/block/preprocess/transactions.go

+			if !firstCrossShardScCallFound {
+				numNewMiniBlocks++
+			}
+			numNewTxs += 1 * core.MultiplyFactorForScCall


1*core.MultiplyFactorForScCall
why 1*x instead of simply x ?

Is it supposed to be changed later to a different value? then use a constant.

I just kept an old pattern when 1 was a variable named numTxs. I will change.

AdoAdoAdo · 2020-09-03T16:29:36Z

process/block/preprocess/transactions.go

+				txs.blockSizeComputation.AddNumMiniBlocks(1)
+			}
+			//we need to increment this as to account for the corresponding SCR hash
+			txs.blockSizeComputation.AddNumTxs(1 * core.MultiplyFactorForScCall)


1*core.MultiplyFactorForScCall ?

I just kept an old pattern when 1 was a variable named numTxs. I will change.

LucianMincu

System tests passed.

SebastianMarian added the type:feature New feature or request label Aug 30, 2020

SebastianMarian self-assigned this Aug 30, 2020

* Put back some log levels

eb69453

iulianpascalau self-requested a review August 31, 2020 07:25

iulianpascalau previously approved these changes Aug 31, 2020

View reviewed changes

AdoAdoAdo previously approved these changes Aug 31, 2020

View reviewed changes

sasurobert previously approved these changes Aug 31, 2020

View reviewed changes

LucianMincu requested changes Aug 31, 2020

View reviewed changes

* Adjusted max allowed size in bytes for body and the whole message

518a6bf

SebastianMarian dismissed stale reviews from sasurobert, AdoAdoAdo, and iulianpascalau via 518a6bf August 31, 2020 11:25

Merge branch 'development' into Fix-block-data-cleaning-from-pools

cdeb77d

iulianpascalau previously approved these changes Aug 31, 2020

View reviewed changes

sasurobert previously approved these changes Aug 31, 2020

View reviewed changes

AdoAdoAdo previously approved these changes Aug 31, 2020

View reviewed changes

iulianpascalau mentioned this pull request Aug 31, 2020

Fixing buffer too large #2262

Closed

- fixed buffer to large error by counting the SCR hash when an exec…

9e0cbba

…uted transaction is of type smart contract call/deploy - made p2p `buffer too large` error message carry more info - reverted throttling config changes

iulianpascalau dismissed stale reviews from AdoAdoAdo, sasurobert, and themself via 9e0cbba August 31, 2020 15:13

sasurobert requested changes Aug 31, 2020

View reviewed changes

iulianpascalau and others added 4 commits August 31, 2020 22:27

- added a new protection in tx prepocessor

0a9d3d5

* Fixed block size computation, taking into consideration also the po…

1247f38

…st processor miniblocks and transactions

* Fixed block size computation, taking into consideration also the po…

025217a

…st processor miniblocks and transactions

Merge remote-tracking branch 'origin/Fix-block-data-cleaning-from-poo…

2e9bb58

…ls' into Fix-block-data-cleaning-from-pools # Conflicts: # process/block/preprocess/transactions.go

sasurobert requested changes Sep 1, 2020

View reviewed changes

* Fixed after review

273d73b

iulianpascalau reviewed Sep 1, 2020

View reviewed changes

SebastianMarian dismissed iulianpascalau’s stale review via 7c9bb2f September 2, 2020 14:12

SebastianMarian added 7 commits September 2, 2020 17:27

* Fixed a wrong shardID which has been given to MaxGasLimitPerBlock m…

9480cc3

…ethod

* Fixed a wrong shardID which has been given to MaxGasLimitPerBlock m…

595e2b9

…ethod

* Added unit tets

7a49849

* Added more prints

2dc7726

* Added multiply constant, in terms of number, for sc calls. This ref…

5e14031

…ers to how many txs of sc call type, could be included in one block

* Added multiply constant, in terms of number, for sc calls. This ref…

c614b66

…ers to how many txs of sc call type, could be included in one block

Merge remote-tracking branch 'origin/Fix-block-data-cleaning-from-poo…

4210c2a

…ls' into Fix-block-data-cleaning-from-pools

iulianpascalau reviewed Sep 3, 2020

View reviewed changes

SebastianMarian added 2 commits September 3, 2020 14:58

* Added block size computation handler aslo in peer block preprocessor

416a97d

* Fixed after review

8de540f

iulianpascalau previously approved these changes Sep 3, 2020

View reviewed changes

iulianpascalau self-requested a review September 3, 2020 15:37

sasurobert previously approved these changes Sep 3, 2020

View reviewed changes

* Added TODO

a56c36e

SebastianMarian dismissed stale reviews from sasurobert and iulianpascalau via a56c36e September 3, 2020 16:17

sasurobert previously approved these changes Sep 3, 2020

View reviewed changes

iulianpascalau previously approved these changes Sep 3, 2020

View reviewed changes

AdoAdoAdo requested changes Sep 3, 2020

View reviewed changes

* Fixed after review

ec6008a

SebastianMarian dismissed stale reviews from iulianpascalau and sasurobert via ec6008a September 3, 2020 17:10

sasurobert approved these changes Sep 3, 2020

View reviewed changes

SebastianMarian added the type:bug Something isn't working label Sep 3, 2020

AdoAdoAdo approved these changes Sep 3, 2020

View reviewed changes

iulianpascalau approved these changes Sep 3, 2020

View reviewed changes

LucianMincu approved these changes Sep 3, 2020

View reviewed changes

LucianMincu merged commit a2c2833 into development Sep 3, 2020

LucianMincu deleted the Fix-block-data-cleaning-from-pools branch September 3, 2020 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix block data cleaning from pools #2259

Fix block data cleaning from pools #2259

SebastianMarian commented Aug 30, 2020

LucianMincu left a comment

iulianpascalau Aug 31, 2020

sasurobert Aug 31, 2020

iulianpascalau Aug 31, 2020

SebastianMarian Aug 31, 2020 •

edited

SebastianMarian Aug 31, 2020

sasurobert left a comment

sasurobert Sep 1, 2020

SebastianMarian Sep 1, 2020

sasurobert Sep 1, 2020

SebastianMarian Sep 1, 2020

iulianpascalau Sep 1, 2020

SebastianMarian Sep 1, 2020

iulianpascalau Sep 3, 2020

SebastianMarian Sep 3, 2020

iulianpascalau Sep 3, 2020

iulianpascalau Sep 3, 2020

SebastianMarian Sep 3, 2020

iulianpascalau Sep 3, 2020

sasurobert Sep 3, 2020

iulianpascalau Sep 3, 2020

SebastianMarian Sep 3, 2020

sasurobert left a comment

sasurobert Sep 3, 2020

AdoAdoAdo Sep 3, 2020

SebastianMarian Sep 3, 2020

AdoAdoAdo Sep 3, 2020

SebastianMarian Sep 3, 2020

LucianMincu left a comment

		txs.blockSizeComputation.AddNumTxs(1)
		txs.blockSizeComputation.AddNumTxs(numScCalls)

Fix block data cleaning from pools #2259

Fix block data cleaning from pools #2259

Conversation

SebastianMarian commented Aug 30, 2020

LucianMincu left a 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 Aug 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasurobert left a 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

sasurobert left a 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

SebastianMarian Aug 31, 2020 •

edited