change check for meta notarizing shard headers#7658
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modifies the check for meta nodes notarizing shard headers during epoch change transitions. The main change adds a validation to prevent shard headers from being notarized between the time an epoch change is proposed and when the epoch start block is created.
Changes:
- Added
GetEpochChangeProposed()method to theEpochStartTriggerHandlerinterface and all implementing types - Introduced new error
ErrShardHeadersShouldNotBeNotarizedto signal invalid shard header notarization during epoch transitions - Modified meta block proposal creation logic to check the epoch change proposed state from the trigger handler instead of checking if it's epoch start
- Added validation in
checkShardHeadersValidityAndFinalityProposalto reject blocks with shard headers when epoch change is proposed
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| process/interface.go | Added GetEpochChangeProposed() method to EpochStartTriggerHandler interface |
| epochStart/interface.go | Added GetEpochChangeProposed() method to TriggerHandler interface and reordered imports |
| process/errors.go | Added new error ErrShardHeadersShouldNotBeNotarized |
| epochStart/metachain/trigger.go | Implemented GetEpochChangeProposed() with proper locking to return the epoch change proposed flag |
| epochStart/shardchain/trigger.go | Implemented GetEpochChangeProposed() to always return false for shard chain triggers |
| epochStart/bootstrap/disabled/disabledEpochStartTrigger.go | Implemented stub GetEpochChangeProposed() returning false |
| process/block/metablockProposal.go | Changed condition at line 114 to use GetEpochChangeProposed() and added validation at lines 981-984 to prevent notarizing shard headers during epoch transition |
| process/block/metablockProposal_test.go | Updated test name and assertion to reflect the change from IsEpochStart() to GetEpochChangeProposed() |
| testscommon/epochStartTriggerStub.go | Added GetEpochChangeProposedCalled field and implemented stub method |
| process/mock/endOfEpochTriggerStub.go | Added GetEpochChangeProposedCalled field and implemented stub method |
| node/mock/endOfEpochTriggerStub.go | Added GetEpochChangeProposedCalled field and implemented stub method |
| integrationTests/mock/endOfEpochTriggerStub.go | Added GetEpochChangeProposedCalled field and implemented stub method |
| factory/mock/epochStartTriggerStub.go | Added GetEpochChangeProposedCalled field and implemented stub method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| shouldNotHaveShardHeaders := metaHeaderHandler.IsStartOfEpochBlock() || metaHeaderHandler.IsEpochChangeProposed() || mp.epochStartTrigger.GetEpochChangeProposed() | ||
| if len(usedShardHeaders.orderedShardHeaders) > 0 && shouldNotHaveShardHeaders { | ||
| return fmt.Errorf("%w : between epoch change proposed and epoch start block", process.ErrShardHeadersShouldNotBeNotarized) | ||
| } |
There was a problem hiding this comment.
The new validation logic that checks if shard headers should not be notarized during epoch change is missing test coverage. There should be test cases in Test_checkShardHeadersValidityAndFinalityProposal that verify:
- When GetEpochChangeProposed() returns true and there are shard headers present, the error ErrShardHeadersShouldNotBeNotarized is returned
- When IsStartOfEpochBlock() returns true and there are shard headers present, the error is returned
- When IsEpochChangeProposed() returns true on the meta header and there are shard headers present, the error is returned
The base branch was changed.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/supernova-async-exec #7658 +/- ##
=============================================================
- Coverage 77.59% 77.58% -0.01%
=============================================================
Files 877 877
Lines 121752 121761 +9
=============================================================
- Hits 94476 94474 -2
- Misses 21008 21017 +9
- Partials 6268 6270 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Reasoning behind the pull request
This PR modifies the check for meta nodes notarizing shard headers during epoch change transitions. The main change adds a validation to prevent shard headers from being notarized between the time an epoch change is proposed and when the epoch start block is created.
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?