Get fees from the previous header of epoch change proposed for metrics#7654
Conversation
…ch-rewards-metric
…ch-rewards-metric
AdoAdoAdo
changed the base branch from
MX-17438-fix-end-of-epoch-rewards-metric
to
feat/supernova-async-exec
…f-epoch-change-proposed
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/supernova-async-exec #7654 +/- ##
=============================================================
- Coverage 77.59% 77.56% -0.04%
=============================================================
Files 877 877
Lines 121752 121784 +32
=============================================================
- Hits 94476 94458 -18
- Misses 21008 21051 +43
- Partials 6268 6275 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…f-epoch-change-proposed
| prevHashOfEpochChangeProposed []byte, | ||
| executionResults []data.BaseExecutionResultHandler, | ||
| ) { | ||
| ) bool { |
There was a problem hiding this comment.
for accumulated fees we need the execution result that has GetHeaderHash equal to the prevHash of epoch change propose block.
…f-epoch-change-proposed
There was a problem hiding this comment.
Pull request overview
This pull request refactors how economics metrics (accumulated fees and dev rewards) are retrieved for epoch changes in v3 headers (Supernova activation). Instead of getting fees from the last execution result of an epoch start block, the code now gets fees from the execution result of the previous header of the epoch change proposed block.
Changes:
- Refactored metrics collection logic to fetch fees from the previous header of epoch change proposed blocks for v3 headers
- Moved and split the
saveEpochStartEconomicsMetricsfunction from common package to baseProcessor, with specialized handling for v3 headers - Removed obsolete helper functions
GetAccumulatedFeesInEpochandGetDeveloperFeesInEpochfrom the common package along with their tests
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| process/block/shardblockProposal.go | Added saveEpochStartEconomicsIfNeeded and setEpochStartMetricsV3FromExecutionResults methods to handle fee metrics for shard processors |
| process/block/shardblock.go | Updated call to use instance method instead of package-level function |
| process/block/metrics.go | Removed obsolete saveEpochStartEconomicsMetrics function |
| process/block/metablockProposal.go | Added saveEpochStartEconomicsMetricsV3IfNeeded to set fee metrics when processing epoch change proposed blocks |
| process/block/metablock.go | Updated calls to use instance method instead of package-level function |
| process/block/baseProcess.go | Added saveEpochStartEconomicsMetrics method with v3-aware logic |
| node/chainSimulator/chainSimulator_test.go | Added integration test to verify economics metrics are properly set across epochs |
| epochStart/bootstrap/process.go | Refactored metrics setting logic for bootstrap with v3-specific handling, improved syncing logic for intermediate blocks |
| common/common_test.go | Removed tests for deleted helper functions |
| common/common.go | Removed obsolete helper functions that were replaced by new implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for currentNonce > metaEpochChangeProposedHeader.GetNonce() { | ||
| intermHeader, err := sp.getHeaderFromHash(true, currentHash, core.MetachainShardId) | ||
| if err != nil { | ||
| // if a header is not found, return here to close the loop. should no be blocking | ||
| return | ||
| } | ||
| currentNonce = intermHeader.GetNonce() | ||
|
|
||
| updatedMetrics := sp.saveEconomicsMetricFromExecutionResults(intermHeader.GetExecutionResultsHandlers(), metaEpochChangeProposedHeader.GetPrevHash()) | ||
| if updatedMetrics { | ||
| return | ||
| } | ||
|
|
||
| currentHash = intermHeader.GetPrevHash() | ||
| } |
There was a problem hiding this comment.
Potential infinite loop detected in the loop logic. The loop condition checks if currentNonce > metaEpochChangeProposedHeader.GetNonce(), and inside the loop, currentNonce is updated with intermHeader.GetNonce(). However, if the intermHeader has the same or higher nonce as the current value (which could happen if getHeaderFromHash retrieves an unexpected header or if there's a chain inconsistency), the loop will never terminate. Consider adding a safeguard to ensure forward progress, such as checking that the new nonce is strictly less than the current nonce before updating, or adding a maximum iteration counter.
There was a problem hiding this comment.
this should not happen
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
sstanculeanu
dismissed stale reviews from axenteoctavian and AdoAdoAdo
via
41133ca
sstanculeanu
deleted the
get-metrics-from-prev-of-epoch-change-proposed
branch
Reasoning behind the pull request
Proposed changes
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?