Reduce max gas limit per mini block #3475

SebastianMarian · 2021-09-28T15:12:24Z

Implemented gas limit calculation in cross mbs dest me taking into account also gas refunded
Implemented gas limit calculation in cross mbs dest me taking into account also gas penalized
Reduced max gas limit per cross shard mini block from me
Registered VM to gas schedule notifier

Testing scenario:

Start a normal system test
In epoch 2, OptimizeGasUsedInCrossMiniBlocks flag will be activated
Start txgen from this branch and with this param:
txgen branch: temp-increase-gas-erc20-esdt
txgen starting command: ./start.sh start 100 base
After 1-2 epochs we should check the logs for "gas consumed, refunded and penalized info" messages and we should have in some of them "total gas consumed" higher than 1.5 billion, but "total gas consumed" - "total gas refunded" - "total gas penalized" always lower than 1.5 billion.
Also we should find messages like this "transactions.splitMiniBlockIfNeeded: gas limit exceeded" when mini blocks with gas higher than 500 mil. will be split and more mini blocks with lower gas

…on in self shard

…(10 min to 1 hour)

…1 hour to 10 min)

# Conflicts: # config/tomlConfig_test.go

…odeRunner.go

…-gas-limit-per-mini-block # Conflicts: # process/block/preprocess/transactions.go # process/mock/gasHandlerMock.go

codecov · 2021-09-29T13:45:15Z

Codecov Report

Merging #3475 (c880fe4) into development (b573b38) will increase coverage by 0.02%.
The diff coverage is 79.46%.

❗ Current head c880fe4 differs from pull request most recent head c6a21c7. Consider uploading reports for the commit c6a21c7 to get more accurate results

@@               Coverage Diff               @@
##           development    #3475      +/-   ##
===============================================
+ Coverage        73.85%   73.87%   +0.02%     
===============================================
  Files              581      581              
  Lines            73907    74193     +286     
===============================================
+ Hits             54583    54812     +229     
- Misses           14945    14993      +48     
- Partials          4379     4388       +9

Impacted Files	Coverage Δ
node/nodeRunner.go	`0.00% <0.00%> (ø)`
process/economics/testEconomicsData.go	`0.00% <0.00%> (ø)`
genesis/process/shardGenesisBlockCreator.go	`43.09% <66.66%> (+0.15%)`	⬆️
process/economics/economicsData.go	`71.17% <69.29%> (-3.91%)`	⬇️
process/block/preprocess/smartContractResults.go	`69.75% <70.00%> (+1.00%)`	⬆️
process/coordinator/process.go	`75.06% <74.46%> (-0.07%)`	⬇️
process/block/preprocess/transactions.go	`55.69% <76.00%> (+2.55%)`	⬆️
process/block/preprocess/basePreProcess.go	`77.11% <88.63%> (+3.64%)`	⬆️
process/block/postprocess/basePostProcess.go	`78.90% <94.11%> (+3.70%)`	⬆️
factory/blockProcessorCreator.go	`80.33% <100.00%> (+0.10%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d93b2b...c6a21c7. Read the comment docs.

…ross shard mini block from me is reached

# Conflicts: # cmd/node/config/enableEpochs.toml # config/epochConfig.go # config/tomlConfig_test.go # genesis/process/shardGenesisBlockCreator.go # integrationTests/testProcessorNode.go # node/nodeRunner.go # p2p/libp2p/discovery/continuousKadDhtDiscoverer.go # p2p/libp2p/discovery/optimizedKadDhtDiscoverer.go # process/smartContract/process.go

# Conflicts: # factory/blockProcessorCreator.go # genesis/process/metaGenesisBlockCreator.go # genesis/process/shardGenesisBlockCreator.go # integrationTests/testProcessorNode.go # process/smartContract/process.go

…ed-in-cross-dest-me-mbs

…mit-per-mini-block

iulianpascalau

minor stuff

iulianpascalau · 2021-10-06T05:19:46Z

cmd/node/config/economics.toml

-    MaxGasLimitPerBlock     = "1500000000"
-    MaxGasLimitPerMetaBlock = "15000000000"
+    GasLimitSettings = [
+        {EnableEpoch = 0, MaxGasLimitPerBlock = "1500000000", MaxGasLimitPerMiniBlock = "1500000000", MaxGasLimitPerMetaBlock = "15000000000", MaxGasLimitPerMetaMiniBlock = "15000000000", MinGasLimit = "50000"},


MaxGasLimitPerMetaMiniBlock = "15000000000" is this ok? It is a ok for the metachain to produce such a large miniblock? Who can consume this?

This is the way it was worked until now. Gas per mini block = Gas per block = 1.5 bil. or 15 bil. in meta.

The difference is that nobody will create miniblocks with gas limit > 1.5 bil. gas, even if they are for meta. The max gas limit of 15 bil. is used only by meta, to accept in one created block, many miniblocks from different shards to himself, until the cumulative gas from each of them reached the 15 bil.

And the most important thing is to keep the backward compatibility, so the mini block gas limit should be equal with block gas limit until the activation epoch for this PR

Ok, but on the next line we have this: MaxGasLimitPerMetaMiniBlock = "5000000000" - 5 billion. How can we interpret this?

WIth the new version of MaxGasLimit configs, the max gas limit per mini blocks in shards and meta will be reduced at 30% from the value used in the first verison. In meta from 15 bil. to 5 bil. and in shards from 1.5 bil to 500 mil.

Anyway the concept of max gas limit per mini block in META does not have a real use case for now. Instead, max gas limit per block is used when a block is created, filled with stuff up to 15 bil. gas. The only use case of gas per mini block, either if the proposer is in shard or in meta, is when it creates a cross shard mini block and it takes into consideration the gas limit per mini block into receiver shard. Actually in this case the shard proposer theoretically could create a mini block to meta which consumes max 5 bil. gas in meta after the activation epoch. Until then the mini block could have even 15 bil. gas.

5 billion gas in meta in one miniblock.....that is very dangerous as it might stop the metachain. I think we should put the MaxGasLimitPerMetaBlock to the exact value of MaxGasLimitPerMiniBlock

epochStart/mock/economicsHandlerStub.go

genesis/process/disabled/epochNotifier.go

node/mock/txFeeHandlerStub.go

process/common.go

process/block/preprocess/transactions.go

integrationTests/testProcessorNode.go

iulianpascalau · 2021-10-06T06:37:18Z

process/economics/economicsData.go

+	conversionBase := 10
+	bitConversionSize := 64
+	for _, gasLimitSetting := range economics.FeeSettings.GasLimitSettings {
+		minGasLimit, err := strconv.ParseUint(gasLimitSetting.MinGasLimit, conversionBase, bitConversionSize)


duplicated code on L256-L279 with the setGasLimitSetting function.

It is different. This one is done in local variables just for checking the validity of the received parameters on New.. method, before the object is created. In setGasLimitSetting the convertion is done directly into members variables at epoch change. This conversion part could not be extracted into a single method which would satisfy both situations.

Yes because the code is written in such way. Could have been refactored as to have only one parse function. I will make a PR that refactors this exact part and you will be one of the reviewers :D

process/economics/economicsData.go

genesis/process/disabled/epochNotifier.go

process/common.go

sasurobert · 2021-10-06T07:46:50Z

process/coordinator/process.go

@@ -976,6 +1026,7 @@ func (tc *transactionCoordinator) VerifyCreatedBlockTransactions(hdr data.Header
 		}(interimProc)
 	}

+	tc.mutInterimProcessors.RUnlock()
 	wg.Wait()


unlock should be after wg.Wait

Hm, I just took the mechanism from one method above -> IsDataPreparedForProcessing. I have done the same thing also there.

Actually the rlock - runlock is used only for the iteration. After the for loop is done, the mutex can be released. I've solved these kind of situation by having the locking held only to have a copy of the slice. Then, I could have iterated on the copied slice with no problems even if the locking was released.

process/block/preprocess/basePreProcess.go

sasurobert · 2021-10-06T09:39:51Z

process/block/preprocess/transactions.go

@@ -901,6 +914,12 @@ func (txs *transactions) createAndProcessMiniBlocksFromMe(
 		totalTimeUsedForComputeGasConsumed += elapsedTime
 		if err != nil {
 			log.Trace("createAndProcessMiniBlocksFromMe.computeGasConsumed", "error", err)
+			isTxTargetedForDeletion := errors.Is(err, process.ErrMaxGasLimitPerOneTxInReceiverShardIsReached)


we could add this at interceptor level as well.

WIll be added in another PR. I already talked with @iulianpascalau about this earlier protection mechanism

process/block/preprocess/transactions.go

process/coordinator/process.go

…ock' into reduce-max-gas-limit-per-mini-block

iulianpascalau · 2021-10-06T15:36:51Z

process/coordinator/process.go

@@ -976,6 +1026,7 @@ func (tc *transactionCoordinator) VerifyCreatedBlockTransactions(hdr data.Header
 		}(interimProc)
 	}

+	tc.mutInterimProcessors.RUnlock()
 	wg.Wait()


Actually the rlock - runlock is used only for the iteration. After the for loop is done, the mutex can be released. I've solved these kind of situation by having the locking held only to have a copy of the slice. Then, I could have iterated on the copied slice with no problems even if the locking was released.

iulianpascalau · 2021-10-06T15:38:16Z

process/coordinator/process.go

 	wg.Wait()
+	tc.mutRequestedTxs.RUnlock()


this is a good change 👍

iulianpascalau · 2021-10-06T15:39:30Z

testscommon/economicsmocks/economicsHandlerMock.go

@@ -8,35 +8,37 @@ import (

 // EconomicsHandlerMock -
 type EconomicsHandlerMock struct {


so many economic mocks

# Conflicts: # process/smartContract/process.go

LucianMincu

System tests passed.

SebastianMarian added 14 commits September 19, 2021 18:12

* Optimized gas consumption in cross shard mini blocks with destinati…

cc7a5bc

…on in self shard

* Refactored duplicated code

1b1704a

* Refactored method name

ea56770

* Increased number of rounds to keep unprocessed txs from 100 to 600 …

8e70ab5

…(10 min to 1 hour)

* Reverted number of rounds to keep unprocessed txs from 600 to 100 (…

45a5b80

…1 hour to 10 min)

* Fixes after review

c875d80

Merge branch 'development' into optimize-gas-used-in-cross-dest-me-mbs

2be9ed0

# Conflicts: # config/tomlConfig_test.go

* Merged with dev

c6551ae

* Added log for OptimizeGasUsedInCrossMiniBlocksEnableEpoch flag in n…

05bbf6a

…odeRunner.go

* Added logs for gas consumed and gas refunded

72e3db6

* Changed OptimizeGasUsedInCrossMiniBlocksEnableEpoch flag, from 7 to 2

413113e

* Changed some logs level to trace

6328582

* Changed condition for warn message

440f2b9

* Implemented gas penalized

a68f51e

SebastianMarian added the type:feature New feature or request label Sep 28, 2021

SebastianMarian self-assigned this Sep 28, 2021

SebastianMarian added 2 commits September 29, 2021 16:28

* Added MaxGasLimitPerMiniBlock functionality

f042a2f

Merge branch 'optimize-gas-used-in-cross-dest-me-mbs' into reduce-max…

da395c1

…-gas-limit-per-mini-block # Conflicts: # process/block/preprocess/transactions.go # process/mock/gasHandlerMock.go

SebastianMarian added 7 commits September 29, 2021 18:52

* Implemented mini blocks splitting when max gas limit accepted per c…

fb864ef

…ross shard mini block from me is reached

* Fixed bad name replacing

431e2c6

* Fixed shardID parameter for MaxGasLimitPerBlock method

abbbcc6

* Fixed gas refunded / consumed roll back when gas limit is exceeded

5eacebd

* Fixed after merge and revert last change

d41bd5f

* Reformated code

23ab47e

SebastianMarian changed the title ~~[WIP] Reduce max gas limit per mini block~~ Reduce max gas limit per mini block Sep 30, 2021

SebastianMarian marked this pull request as ready for review September 30, 2021 13:08

SebastianMarian added 2 commits September 30, 2021 18:28

Merge branch 'development' into reduce-max-gas-limit-per-mini-block

9584626

* Refactored some print messages

b13a51a

SebastianMarian and others added 5 commits October 3, 2021 22:12

* Guarded and fixed some situations in splitMiniBlockIfNeeded methods

6a92fff

Merge branch 'development' into reduce-max-gas-limit-per-mini-block

1e87446

# Conflicts: # factory/blockProcessorCreator.go # genesis/process/metaGenesisBlockCreator.go # genesis/process/shardGenesisBlockCreator.go # integrationTests/testProcessorNode.go # process/smartContract/process.go

* Fixed after merge and after previous review done in optimize-gas-us…

6c6af7c

…ed-in-cross-dest-me-mbs

Merge branch 'registerVM-gasSchedule-notifier' into reduce-max-gas-li…

20bea5f

…mit-per-mini-block

Merge branch 'development' into reduce-max-gas-limit-per-mini-block

edecc09

iulianpascalau self-requested a review October 6, 2021 05:17

iulianpascalau reviewed Oct 6, 2021

View reviewed changes

sasurobert reviewed Oct 6, 2021

View reviewed changes

SebastianMarian and others added 4 commits October 6, 2021 16:54

Merge branch 'development' into reduce-max-gas-limit-per-mini-block

d7061c5

* Fixed after review

34c3475

Merge remote-tracking branch 'origin/reduce-max-gas-limit-per-mini-bl…

d13ab9c

…ock' into reduce-max-gas-limit-per-mini-block

* Added more check for nil in mocks

e9cf70a

iulianpascalau reviewed Oct 6, 2021

View reviewed changes

sasurobert previously approved these changes Oct 6, 2021

View reviewed changes

* Adjusted gas limit in economics.toml

5ec84f9

SebastianMarian dismissed sasurobert’s stale review via 5ec84f9 October 6, 2021 17:11

iulianpascalau previously approved these changes Oct 6, 2021

View reviewed changes

Merge branch 'development' into reduce-max-gas-limit-per-mini-block

d6b229a

sasurobert previously approved these changes Oct 7, 2021

View reviewed changes

sasurobert and others added 3 commits October 7, 2021 14:11

Merge branch 'development' into reduce-max-gas-limit-per-mini-block

3c1928d

Merge branch 'development' into reduce-max-gas-limit-per-mini-block

13109a8

# Conflicts: # process/smartContract/process.go

* Fix after merge

6580158

SebastianMarian dismissed stale reviews from sasurobert and iulianpascalau via 6580158 October 8, 2021 07:04

iulianpascalau approved these changes Oct 8, 2021

View reviewed changes

sasurobert approved these changes Oct 8, 2021

View reviewed changes

Merge branch 'development' into reduce-max-gas-limit-per-mini-block

c6a21c7

LucianMincu approved these changes Oct 8, 2021

View reviewed changes

LucianMincu merged commit 1229c24 into development Oct 8, 2021

LucianMincu deleted the reduce-max-gas-limit-per-mini-block branch October 8, 2021 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce max gas limit per mini block #3475

Reduce max gas limit per mini block #3475

SebastianMarian commented Sep 28, 2021 •

edited

codecov bot commented Sep 29, 2021 •

edited

iulianpascalau left a comment

iulianpascalau Oct 6, 2021

SebastianMarian Oct 6, 2021

SebastianMarian Oct 6, 2021 •

edited

SebastianMarian Oct 6, 2021

iulianpascalau Oct 6, 2021

SebastianMarian Oct 6, 2021

SebastianMarian Oct 6, 2021

iulianpascalau Oct 6, 2021

iulianpascalau Oct 6, 2021

SebastianMarian Oct 6, 2021 •

edited

iulianpascalau Oct 6, 2021

sasurobert Oct 6, 2021

SebastianMarian Oct 6, 2021 •

edited

iulianpascalau Oct 6, 2021 •

edited

sasurobert Oct 6, 2021

SebastianMarian Oct 6, 2021

iulianpascalau Oct 6, 2021 •

edited

iulianpascalau Oct 6, 2021

iulianpascalau Oct 6, 2021

LucianMincu left a comment

		@@ -8,35 +8,37 @@ import (

		// EconomicsHandlerMock -
		type EconomicsHandlerMock struct {

Reduce max gas limit per mini block #3475

Reduce max gas limit per mini block #3475

Conversation

SebastianMarian commented Sep 28, 2021 • edited

codecov bot commented Sep 29, 2021 • edited

Codecov Report

iulianpascalau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianMarian Oct 6, 2021 • 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

SebastianMarian Oct 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianMarian Oct 6, 2021 • edited

Choose a reason for hiding this comment

iulianpascalau Oct 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iulianpascalau Oct 6, 2021 • edited

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 commented Sep 28, 2021 •

edited

codecov bot commented Sep 29, 2021 •

edited

SebastianMarian Oct 6, 2021 •

edited

SebastianMarian Oct 6, 2021 •

edited

SebastianMarian Oct 6, 2021 •

edited

iulianpascalau Oct 6, 2021 •

edited

iulianpascalau Oct 6, 2021 •

edited