New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New enable epochs handler functionality #5417
New enable epochs handler functionality #5417
Conversation
@@ -33,6 +36,9 @@ func NewEnableEpochsHandler(enableEpochsConfig config.EnableEpochs, epochNotifie | |||
|
|||
// EpochConfirmed is called whenever a new epoch is confirmed | |||
func (handler *enableEpochsHandler) EpochConfirmed(epoch uint32, _ uint64) { | |||
atomic.StoreUint32(&handler.currentEpoch, epoch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is concurrent safe. However, I was wondering if making it a critical section will help when we can have concurrent EpochConfirmed calls and the internal flags will have non-deterministic values.
We should never have concurrent EpochConfirmed calls but we never know how the code will change,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added mutex
return epoch >= handler.enableEpochsConfig.SwitchHysteresisForMinNodesEnableEpoch | ||
} | ||
|
||
// IsSwitchHysteresisForMinNodesFlagEnabledInSpecificEpochOnly returns true if SwitchHysteresisForMinNodesEnableEpoch is the provided epoch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
} | ||
|
||
// IsSaveToSystemAccountFlagEnabledInEpoch returns true if OptimizeNFTStoreEnableEpoch is lower than the provided epoch | ||
// this is a duplicate for OptimizeNFTStoreEnableEpoch needed for consistency into vm-common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need the OptimizeNFTStoreEnableEpoch value?
Valid also for OptimizeNFTStoreEnableEpoch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the moment I just duplicated all the methods and then integrate one by one.. unused methods will be removed in further PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great!
@@ -140,109 +141,111 @@ func TestNewEnableEpochsHandler_EpochConfirmed(t *testing.T) { | |||
handler, _ := NewEnableEpochsHandler(cfg, &epochNotifier.EpochNotifierStub{}) | |||
require.False(t, check.IfNil(handler)) | |||
|
|||
handler.EpochConfirmed(math.MaxUint32, 0) | |||
currentEpoch := uint32(math.MaxUint32 - 1) | |||
handler.EpochConfirmed(currentEpoch, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this call and change all the other calls to use currentEpoch instead of math.MaxUint32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@@ -259,107 +262,108 @@ func TestNewEnableEpochsHandler_EpochConfirmed(t *testing.T) { | |||
|
|||
handler.EpochConfirmed(epoch, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be removed
assert.True(t, handler.IsTransferToMetaFlagEnabled()) | ||
assert.True(t, handler.IsESDTNFTImprovementV1FlagEnabled()) | ||
assert.True(t, handler.FixDelegationChangeOwnerOnAccountEnabled()) | ||
assert.True(t, handler.IsSCDeployFlagEnabledInEpoch(math.MaxUint32)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test against the epoch value, not math.MaxUint32
@@ -371,107 +375,108 @@ func TestNewEnableEpochsHandler_EpochConfirmed(t *testing.T) { | |||
|
|||
handler.EpochConfirmed(epoch, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required
assert.False(t, handler.IsScheduledMiniBlocksFlagEnabled()) | ||
assert.False(t, handler.IsCorrectJailedNotUnStakedEmptyQueueFlagEnabled()) | ||
assert.False(t, handler.IsDoNotReturnOldBlockInBlockchainHookFlagEnabled()) | ||
assert.False(t, handler.IsSCDeployFlagEnabledInEpoch(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test against epoch, not 0
// IsCorrectLastUnJailedFlagEnabledInEpoch returns true if correctLastUnJailedFlag is lower than the provided epoch | ||
func (handler *enableEpochsHandler) IsCorrectLastUnJailedFlagEnabledInEpoch(epoch uint32) bool { | ||
return epoch >= handler.enableEpochsConfig.CorrectLastUnjailedEnableEpoch | ||
} | ||
|
||
// IsCorrectLastUnJailedFlagEnabledInSpecificEpochOnly returns true if correctLastUnJailedCurrentEpochFlag is the provided epoch | ||
func (handler *enableEpochsHandler) IsCorrectLastUnJailedFlagEnabledInSpecificEpochOnly(epoch uint32) bool { | ||
return epoch == handler.enableEpochsConfig.CorrectLastUnjailedEnableEpoch | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// IsCorrectLastUnJailedFlagEnabledInEpoch returns true if correctLastUnJailedFlag is lower than the provided epoch | |
func (handler *enableEpochsHandler) IsCorrectLastUnJailedFlagEnabledInEpoch(epoch uint32) bool { | |
return epoch >= handler.enableEpochsConfig.CorrectLastUnjailedEnableEpoch | |
} | |
// IsCorrectLastUnJailedFlagEnabledInSpecificEpochOnly returns true if correctLastUnJailedCurrentEpochFlag is the provided epoch | |
func (handler *enableEpochsHandler) IsCorrectLastUnJailedFlagEnabledInSpecificEpochOnly(epoch uint32) bool { | |
return epoch == handler.enableEpochsConfig.CorrectLastUnjailedEnableEpoch | |
} | |
// IsCorrectLastUnJailedFlagEnabledInEpoch returns true if CorrectLastUnjailedEnableEpoch is lower than the provided epoch | |
func (handler *enableEpochsHandler) IsCorrectLastUnJailedFlagEnabledInEpoch(epoch uint32) bool { | |
return epoch >= handler.enableEpochsConfig.CorrectLastUnjailedEnableEpoch | |
} | |
// IsCorrectLastUnJailedFlagEnabledInSpecificEpochOnly returns true if CorrectLastUnjailedEnableEpoch is the provided epoch | |
func (handler *enableEpochsHandler) IsCorrectLastUnJailedFlagEnabledInSpecificEpochOnly(epoch uint32) bool { | |
return epoch == handler.enableEpochsConfig.CorrectLastUnjailedEnableEpoch | |
} |
change comments to have the same pattern as for the others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, missed them
74e1f27
into
feat/refactor_enable_epochs_handler
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:
feat
branch created?feat
branch merging, do all satellite projects have a proper tag insidego.mod
?