TODOs#7880
Open
sstanculeanu wants to merge 12 commits into
Open
Conversation
sstanculeanu
added
the
ignore-for-release-notes
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/testnet-fixes #7880 +/- ##
======================================================
- Coverage 77.62% 77.60% -0.02%
======================================================
Files 885 885
Lines 125425 125495 +70
======================================================
+ Hits 97357 97395 +38
- Misses 21585 21614 +29
- Partials 6483 6486 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…hash should be the last execution result roothash
miiu96
reviewed
Jun 18, 2026
|
|
||
| func (bp *baseProcessor) recreateTrieIfNeeded() error { | ||
| rootHash := bp.blockChain.GetCurrentBlockRootHash() | ||
| prevHeader := bp.blockChain.GetCurrentBlockHeader() |
Contributor
There was a problem hiding this comment.
maybe you can extract this code in a new function ?
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes a number of stale TODOs by implementing the missing logic and wiring, mainly around Supernova/headerV3 execution results handling, gas-capacity-aware inclusion estimation, and using real runtime components (chain handler + tx processor) in the external transaction API.
Changes:
- Add a max block gas capacity cap to
ExecutionResultInclusionEstimatorand wire it from economics data in block processor creation. - Strengthen block proposal sanity checks by verifying header execution results, and update trie recreation to use the last execution result root hash.
- Wire real
ChainHandlerandTransactionProcessorinto the external transaction API (replacing disabled placeholders) and expand test coverage/stubs accordingly.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| process/estimator/executionResultInclusionEstimator.go | Add gas-capacity-aware inclusion limiting for execution results. |
| process/estimator/executionResultInclusionEstimator_test.go | Update constructor calls; add test for gas-capacity cap. |
| process/block/shardblockProposal.go | Verify header execution results during shard block proposal creation. |
| process/block/shardblockProposal_test.go | Adjust proposal tests for headerV3 execution results verification and updated estimator ctor. |
| process/block/shardblock.go | Remove outdated TODOs related to Supernova/outport/final block info. |
| process/block/metablockProposal.go | Remove outdated epoch-start trigger TODO. |
| process/block/metablock.go | Remove outdated Supernova TODO in commit path. |
| process/block/metablock_test.go | Update estimator ctor call signature in meta tests. |
| process/block/export_test.go | Update estimator ctor call signature in export tests. |
| process/block/baseProcess.go | Recreate trie using last execution result root hash derived from previous header. |
| process/block/baseProcess_test.go | Update chain handler stubs to match new trie recreation behavior and estimator ctor. |
| node/external/transactionAPI/check.go | Validate new required args: ChainHandler and TxProcessor. |
| node/external/transactionAPI/apiTransactionProcessor.go | Store/use real chain handler + tx processor for selection and block info. |
| node/external/transactionAPI/apiTransactionProcessor_test.go | Extend mocks and add coverage for nil chain handler; update constructors. |
| node/external/transactionAPI/apiTransactionArgs.go | Add ChainHandler and TxProcessor to API transaction processor args. |
| node/chainSimulator/components/processComponents.go | Expose TransactionProcessor() from process components holder. |
| integrationTests/vm/staking/metaBlockProcessorCreator.go | Update estimator ctor signature; minor formatting alignment fix. |
| integrationTests/testSyncNode.go | Update estimator ctor signature; minor formatting alignment fix. |
| integrationTests/testProcessorNodeWithTestWebServer.go | Provide chain handler + tx processor to API transaction processor in integration setup. |
| integrationTests/testProcessorNode.go | Update estimator ctor signature; pass headers pool into epoch economics args. |
| integrationTests/testFullNode.go | Update estimator ctor signature; pass headers pool into epoch economics args. |
| integrationTests/mock/processComponentsStub.go | Add TransactionProcessor() to integration test process components stub. |
| factory/state/stateComponents.go | Remove outdated TODO about account adapter root hash behavior. |
| factory/processing/processComponentsHandler.go | Add nil-check and accessor for transaction processor in managed process components. |
| factory/processing/processComponents.go | Carry transaction processor through processComponents struct. |
| factory/processing/blockProcessorCreator.go | Compute and pass max gas capacity to inclusion estimator; pass headers pool to epoch economics; expose tx processor. |
| factory/mock/processComponentsStub.go | Add TransactionProcessor() to factory mock. |
| factory/interface.go | Extend ProcessComponentsHolder with TransactionProcessor(). |
| factory/api/apiResolverFactory.go | Wire real chain handler + tx processor into API transaction processor creation. |
| errors/errors.go | Add ErrNilTransactionProcessor for component validation. |
| epochStart/metachain/epochStartData.go | Remove stale TODO on error handling note. |
| epochStart/metachain/economics.go | Add headers pool dependency and use it to find the epoch-change-proposed header via execution results. |
| epochStart/metachain/economics_test.go | Add headers pool stub; test nil headers pool; extend v3 economics args coverage. |
| consensus/broadcast/shardChainMessenger.go | Remove stale determinism TODO. |
| config/config.go | Remove stale TODO in estimator config comment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+609
to
+611
| maxBlockGasCapacity := pcf.coreData.EconomicsData().MaxGasLimitPerBlock(pcf.bootstrapComponents.ShardCoordinator().SelfId()) | ||
| overestimationFactor := pcf.coreData.EconomicsData().BlockCapacityOverestimationFactor() | ||
| maxBlockGasCapacity = maxBlockGasCapacity * overestimationFactor / 100 |
Comment on lines
+1198
to
+1200
| maxBlockGasCapacity := pcf.coreData.EconomicsData().MaxGasLimitPerBlock(pcf.bootstrapComponents.ShardCoordinator().SelfId()) | ||
| overestimationFactor := pcf.coreData.EconomicsData().BlockCapacityOverestimationFactor() | ||
| maxBlockGasCapacity = maxBlockGasCapacity * overestimationFactor / 100 |
| ProposedInRound: 0, | ||
| } | ||
| //second execution result finishes in round 4, allignment will take its finish time as base. third small enough to fit so all accepted | ||
| // second execution result finishes in round 4, allignment will take its finish time as base. third small enough to fit so all accepted |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reasoning behind the pull request
Proposed changes
// TODO add also max estimated block gas capacity+// TODO add also max estimated block gas capacity - used gas must be lower than this-> implemented, now uses the block capacity from economics data// TODO: check if miniblocks and txs are set in a deterministic way (check if there are map iterations that can generate non-deterministic results)-> checked with @ssd04 , should be safe to remove todo// todo: extract the epoch change proposal execution result here-> fixed, now all execution results are checked to find the proper epoch changed proposed header// TODO: analyse error handling here-> checked with @ssd04 , this is old code and should be safe as before// TODO use the right object, not a disabled one-> fixed, integrated the real transactions processor// TODO use the right information below-> fixed, now data is fetched from chain handler// TODO adjust this method if needed for Supernova-> removed todo, execution state should be correctly loaded at bootstrap from the headers// TODO: the trigger would need to be changed upon commit of a block with the epoch start results-> removed todo, already handled// TODO: set proper finalized header in outport-> checked with @BeniaminDrasovean , proper data is already set// TODO: maybe rename this to reflect last execution results-> removed todo, not necessarily needed// todo: check empty mini blocks vs nil. Same for block.Body.MiniBlocks-> removed todo, GetMiniBlockHeaderHandlers and GetMiniBlocks already normalizez empty slice to nil// TODO: sanity check use the verify execution results method-> fixed, added call to VerifyHeaderExecutionResults// TODO: this account adapter may be required to be changed as the roothash should be the last execution result roothash-> fixed, now trie is recreated everywhereTesting 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?