Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/supernova-async-exec #7797 +/- ##
=============================================================
- Coverage 77.54% 77.53% -0.01%
=============================================================
Files 882 882
Lines 123683 123646 -37
=============================================================
- Hits 95908 95870 -38
- Misses 21429 21434 +5
+ Partials 6346 6342 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| NumTxs: numOfTxs, | ||
| NotarizedBlocks: notarizedBlocks, | ||
| MiniBlocks: miniblocks, |
There was a problem hiding this comment.
add activation flag check for this? or maybe better keep it as before, with propored miniblocks and num of txs?
| NumTxs: numOfTxs, | ||
| MiniBlocks: miniblocks, |
There was a problem hiding this comment.
Pull request overview
This PR updates the block API receipts-loading flow to correctly handle protocol differences (incl. Supernova round activation) by threading round/receipts-hash context through the API block processors and receipts repository.
Changes:
- Extends
LoadReceiptsto accept an explicitreceiptsHashand updates all call sites/tests accordingly. - Adds
EnableRoundsHandlerplumbing into the block API processor args and processors; uses it to select the correct storage unit for receipts. - Refactors shard/meta API block conversion to delegate miniblock/tx counting logic to shared helpers and adds an integration-style simulator test for receipts.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| testscommon/receiptsRepositoryStub.go | Updates stub to match new LoadReceipts(receiptsHash, header, headerHash) signature. |
| process/receipts/receiptsRepository.go | Updates repository LoadReceipts signature and storage-key selection input. |
| process/receipts/receiptsRepository_test.go | Adjusts tests to pass receiptsHash explicitly. |
| node/external/blockAPI/interface.go | Updates internal blockAPI receipts repository interface signature. |
| node/external/blockAPI/blockArgs.go | Adds EnableRoundHandler arg (rounds handler) for block API processor construction. |
| node/external/blockAPI/check.go | Validates non-nil rounds handler in args. |
| node/external/blockAPI/baseBlock.go | Uses rounds flag to choose receipts storer unit; passes explicit receipts hash to receipts repo for intrashard miniblocks. |
| node/external/blockAPI/baseBlock_test.go | Updates tests to the new receipts-loading API and handler plumbing. |
| node/external/blockAPI/shardBlock.go | Refactors conversion to use helper methods; updates intrashard receipts miniblock loading signature. |
| node/external/blockAPI/metaBlock.go | Same as shardBlock: refactor + updated intrashard receipts miniblock loading signature. |
| node/external/blockAPI/apiBlockFactory_test.go | Updates block API processor args to include rounds handler. |
| node/chainSimulator/chainSimulator_test.go | Adds a simulator test asserting receipts are exposed correctly across Supernova activation. |
| factory/interface.go | Updates public ReceiptsRepository interface signature. |
| factory/api/apiResolverFactory.go | Wires EnableRoundsHandler() into block API processor args. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| allMbs := append(mbsBeforeExecutionAndCleanup, mbsAfterExecution...) | ||
| intraMb, err := bap.getIntrashardMiniblocksFromReceiptsStorage(blockHeader, headerHash, options) | ||
| receiptsHash := executionResultHandler.GetReceiptsHash() |
There was a problem hiding this comment.
In async execution, receiptsHash := executionResultHandler.GetReceiptsHash() can be nil/empty (e.g. if the field is missing in older execution results / default-initialized). Passing that through to LoadReceipts makes decideStorageKey choose an empty key instead of falling back to the header hash for the empty-receipts case, which can prevent intrashard miniblocks from being loaded. Consider falling back to blockHeader.GetReceiptsHash() (or to bap.emptyReceiptsHash) when len(receiptsHash)==0 before calling getIntrashardMiniblocksFromReceiptsStorage.
| receiptsHash := executionResultHandler.GetReceiptsHash() | |
| receiptsHash := executionResultHandler.GetReceiptsHash() | |
| if len(receiptsHash) == 0 { | |
| // Fallback to the block header receipts hash for older/default async results | |
| receiptsHash = blockHeader.GetReceiptsHash() | |
| } | |
| if len(receiptsHash) == 0 { | |
| // If still empty, use the predefined empty receipts hash | |
| receiptsHash = bap.emptyReceiptsHash | |
| } |
node/external/blockAPI/blockArgs.go
Outdated
| AccountsRepository state.AccountsRepository | ||
| ScheduledTxsExecutionHandler process.ScheduledTxsExecutionHandler | ||
| EnableEpochsHandler common.EnableEpochsHandler | ||
| EnableRoundHandler common.EnableRoundsHandler |
There was a problem hiding this comment.
ArgAPIBlockProcessor introduces an exported field named EnableRoundHandler, but the type and the rest of the codebase use EnableRoundsHandler (plural). This inconsistency is easy to trip over and makes the API args less discoverable; consider renaming the field to EnableRoundsHandler and updating call sites accordingly.
| EnableRoundHandler common.EnableRoundsHandler | |
| EnableRoundsHandler common.EnableRoundsHandler |
| chainSimulatorCommon.CheckGenerateTransactions(t, chainSimulator) | ||
| } | ||
|
|
||
| func TestSimilator_MoveBalanceCheckReceipt(t *testing.T) { |
There was a problem hiding this comment.
The test name TestSimilator_MoveBalanceCheckReceipt is misspelled (Similator). Since Go test selection relies on Test... names, consider correcting it to TestSimulator_MoveBalanceCheckReceipt for consistency and easier discovery.
| func TestSimilator_MoveBalanceCheckReceipt(t *testing.T) { | |
| func TestSimulator_MoveBalanceCheckReceipt(t *testing.T) { |
| require.Equal(te, value, mb.Receipts[0].Value.String()) | ||
| } | ||
| } | ||
| require.True(t, called) |
There was a problem hiding this comment.
Inside checkReceipts, assertions mostly use the te parameter, but the final assertion uses the outer t (require.True(t, called)). This can report failures on the wrong *testing.T (especially if this helper is reused in subtests). Use te consistently for all assertions in this helper.
| require.True(t, called) | |
| require.True(te, called) |
| chainSimulatorCommon.CheckGenerateTransactions(t, chainSimulator) | ||
| } | ||
|
|
||
| func TestSimilator_MoveBalanceCheckReceipt(t *testing.T) { |
There was a problem hiding this comment.
Simulator instead of Similator
Reasoning behind the pull request
fullHistoryPrunningStorerPre-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?