Block size proposal limit checks#7661
Conversation
process/block/gasConsumption.go
Outdated
| return lastMiniBlockIndex, 0, err | ||
| } | ||
|
|
||
| gc.blockSizeComputation.AddNumMiniBlocks(1) |
There was a problem hiding this comment.
should this be added also inside the if from L137, for rewards and peer changes?
There was a problem hiding this comment.
i was thinking initially that these miniblocks has to be included anyway, added increase limits though
process/block/gasConsumption.go
Outdated
| return lastMiniBlockIndex, 0, nil | ||
| } | ||
|
|
||
| func (gc *gasConsumption) increaseBlockSizeLimits( |
There was a problem hiding this comment.
maybe rename it to something like addMiniBlockToBlockSizeComputation or similar?
| doubleTransactionsDetector process.DoubleTransactionDetector | ||
| processedMiniBlocksTracker process.ProcessedMiniBlocksTracker | ||
| enableEpochsHandler common.EnableEpochsHandler | ||
| enableRoundsHandler common.EnableRoundsHandler |
There was a problem hiding this comment.
no, it was part of the old code, removed
process/block/gasConsumption.go
Outdated
| break | ||
| } | ||
|
|
||
| gc.blockSizeComputation.AddNumMiniBlocks(1) |
There was a problem hiding this comment.
I'd move this inside if !mbsLimitReached, at L296. This way blockSizeComputation gets updated when miniblocks are added via direct call on AddIncomingMiniBlocks, but also when appending pending ones.
and remove it from L157 this way
There was a problem hiding this comment.
Pull request overview
This pull request adds block size limit checks for execution results to complement existing gas-based limits. It updates the block size computation component to track execution result sizes and enforces these limits during block proposal creation.
Changes:
- Moved
LastExecutionResultForInclusionfromprocess/estimatortocommonpackage for better code organization - Extended
BlockSizeComputationHandlerinterface with execution result tracking methods (AddNumExecRes,DecNumExecRes,IsMaxExecResSizeReached) - Added
MaxExecResSizeInBytesconfiguration field toBlockSizeThrottleConfigwith default value of 10% of 1MB - Integrated execution result size checks in
ExecutionResultInclusionEstimatorandgasConsumptioncomponents - Updated all test files and integration tests to use the new constructor signatures
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| config/config.go | Added MaxExecResSizeInBytes field to BlockSizeThrottleConfig |
| cmd/node/config/config.toml | Added configuration value for execution result size limit |
| common/dtos.go | Moved LastExecutionResultForInclusion struct from estimator package |
| process/interface.go | Updated InclusionEstimator interface signature to use common.LastExecutionResultForInclusion |
| process/block/preprocess/interfaces.go | Extended BlockSizeComputationHandler interface with execution result methods |
| process/block/preprocess/blockSizeComputation.go | Implemented execution result size tracking and limit checking |
| process/block/preprocess/blockSizeComputation_test.go | Added tests for execution result size computation methods |
| process/estimator/executionResultInclusionEstimator.go | Integrated blockSizeComputation for execution result size checks |
| process/estimator/executionResultInclusionEstimator_test.go | Updated tests to pass blockSizeComputation parameter |
| process/block/gasConsumption.go | Added block size limit checks alongside gas limit checks |
| process/block/gasConsumption_test.go | Updated test arguments with blockSizeComputation |
| testscommon/blockSizeComputationStub.go | Extended stub with execution result methods |
| testscommon/generalConfig.go | Added test configuration for new limits |
| genesis/process/disabled/blockSizeComputationHandler.go | Added no-op implementations for new methods |
| factory/processing/blockProcessorCreator.go | Updated block processor creation to pass MaxExecResSizeInBytes |
| Multiple test files | Updated to use new constructor signatures and pass blockSizeComputation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if erie.blockSizeComputation.IsMaxExecResSizeReached(1) { | ||
| log.Debug("ExecutionResultInclusionEstimator: estimated max size reached", | ||
| "currentIndex", pendingExecutionIndex) | ||
| return pendingExecutionIndex | ||
| } | ||
| erie.blockSizeComputation.AddNumExecRes(1) |
There was a problem hiding this comment.
The new execution result size limit functionality (IsMaxExecResSizeReached check at line 172) is not covered by any tests. All tests in this file use BlockSizeComputationStub which returns false by default. Consider adding test cases that verify the behavior when the execution result size limit is reached, similar to how MaxResultsPerBlock is tested.
| if erie.blockSizeComputation.IsMaxExecResSizeReached(1) { | ||
| log.Debug("ExecutionResultInclusionEstimator: estimated max size reached", | ||
| "currentIndex", pendingExecutionIndex) | ||
| return pendingExecutionIndex | ||
| } | ||
| erie.blockSizeComputation.AddNumExecRes(1) |
There was a problem hiding this comment.
The ExecutionResultInclusionEstimator's Decide method mutates the blockSizeComputation state by calling AddNumExecRes. This is problematic because Decide is called both during block proposal creation (in addExecutionResultsOnHeader) and during block verification (in checkInclusionEstimationForExecutionResults). During verification, we should not mutate shared state as multiple verifications might be happening concurrently, or a verification might interfere with proposal creation. Consider either: 1) making the size check read-only in the estimator and moving the mutation to the caller, or 2) using a separate blockSizeComputation instance for verification vs proposal.
There was a problem hiding this comment.
will add a separate check flow for this case, will trigger calculation encapsulated in this call
There was a problem hiding this comment.
updated to trigger calculation separately on each call
| roundHandler RoundHandler | ||
| // TODO add also max estimated block gas capacity - used gas must be lower than this | ||
| blockSizeComputation blockSizeComputationHandler | ||
| // blockSizeComputation blockSizeComputationHandler |
| } | ||
|
|
||
| var err error | ||
| ers.execResSize, err = ers.generateDummyExecutionResultSize(marshalizer, 10) |
There was a problem hiding this comment.
i kept the same estimation as for block, as in blockSizeComputation - currently i think on shard we have up to 5 miniblocks and on meta up to 9
There was a problem hiding this comment.
to be discussed, there is also the TODO with the option to have a size estimation calculated dynamically based on real time data, in case we'll have more shards in the future
| [BlockSizeThrottleConfig] | ||
| MinSizeInBytes = 104857 # 104857 is 10% from 1MB | ||
| MaxSizeInBytes = 943718 # 943718 is 90% from 1MB | ||
| MaxExecResSizeInBytes = 104857 # 104857 is 10% from 1MB |
There was a problem hiding this comment.
what is the average size of an execution result? I am curious what would this limitation mean in concrete number of execution results
There was a problem hiding this comment.
this limit will allow up to 200 execution results
There was a problem hiding this comment.
this is based on the dummy created execution results with 10 miniblocks
| dummyHash := make([]byte, 32) | ||
| _, _ = rand.Reader.Read(dummyHash) | ||
|
|
||
| executionResult := &block.ExecutionResult{ |
There was a problem hiding this comment.
maybe use a meta excution result instead? I think that one has more fields.
| return ers, nil | ||
| } | ||
|
|
||
| func (ers *execResSizeComputation) generateDummyExecutionResultSize( |
There was a problem hiding this comment.
can you generate the size estimation based on the larger result? (I think the metachain result is larger).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/supernova-async-exec #7661 +/- ##
=============================================================
- Coverage 77.51% 77.51% -0.01%
=============================================================
Files 880 882 +2
Lines 122576 122753 +177
=============================================================
+ Hits 95016 95146 +130
- Misses 21233 21263 +30
- Partials 6327 6344 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| // DecNumMiniBlocks decrements the provided value to numMiniBlocks in a concurrent safe manner | ||
| func (bsc *blockSizeComputation) DecNumMiniBlocks(numMiniBlocks int) { | ||
| atomic.AddInt32(&bsc.numMiniBlocks, -int32(numMiniBlocks)) |
|
|
||
| // DecNumTxs decrements the provided value to numTxs in a concurrent safe manner | ||
| func (bsc *blockSizeComputation) DecNumTxs(numTxs int) { | ||
| atomic.AddInt32(&bsc.numTxs, -int32(numTxs)) |
| GasUsed: 1234, | ||
| }, | ||
| ValidatorStatsRootHash: dummyHash, | ||
| AccumulatedFeesInEpoch: big.NewInt(0), |
There was a problem hiding this comment.
maybe have larger values for the bigints, fully denominated egld values
There was a problem hiding this comment.
updated to use bigger values
| mbType := miniBlocks[i].GetTypeInt32() | ||
| if mbType == int32(block.RewardsBlock) || mbType == int32(block.PeerBlock) { | ||
| // rewards and validator info have 0 gas limit, thus they should be included anyway without checking their transactions | ||
| gc.addMiniBlockToBlockSizeComputation(miniBlocks[i]) |
There was a problem hiding this comment.
seems like there is no check for the peer + rewards mbs, if we have reached the size limit before adding more.
There was a problem hiding this comment.
there is no check on gas consumption, so did not add for size limit; they are counted for size but with no check
Reasoning behind the pull request
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?