New methods enable epochs handler #5491

sstanculeanu · 2023-08-17T07:59:52Z

Reasoning behind the pull request

new less prone to errors approach for EnableEpochsHandler is needed

Proposed changes

implement new methods

Testing procedure

will be tested with feat branch

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

…core-go

…espect with mx-chain-core-go(the method will be removed in a further commit)

AdoAdoAdo · 2023-08-21T07:54:17Z

common/enablers/enableEpochsHandler.go

+// IsFlagDefined checks if a specific flag is supported by the current version of mx-chain-core-go
+func (handler *enableEpochsHandler) IsFlagDefined(flag core.EnableEpochFlag) bool {
+	switch flag {
+	case core.SCDeployFlag,


can we instead initialize on the constructor a map with all flags as keys and here just check that the flag is present in the map?

AdoAdoAdo · 2023-08-21T07:55:54Z

common/interface.go

@@ -289,6 +289,11 @@ type EnableEpochsHandler interface {
 	RefactorPeersMiniBlocksEnableEpoch() uint32
 	GetCurrentEpoch() uint32

+	IsFlagDefined(flag core.EnableEpochFlag) bool
+	IsFlagEnabledInCurrentEpoch(flag core.EnableEpochFlag) bool


can we also rename the IsFlagEnabledInCurrentEpoch into IsFlagEnabled?

iulianpascalau · 2023-08-21T07:51:41Z

common/enablers/enableEpochsHandler.go

@@ -39,6 +41,330 @@ func (handler *enableEpochsHandler) EpochConfirmed(epoch uint32, _ uint64) {
 	handler.epochMut.Unlock()
 }

+// IsFlagDefined checks if a specific flag is supported by the current version of mx-chain-core-go
+func (handler *enableEpochsHandler) IsFlagDefined(flag core.EnableEpochFlag) bool {
+	switch flag {


instead of this switch, we can have the allFlagsDefined as a map instead of array, as discussed in the last meeting

iulianpascalau · 2023-08-21T07:56:05Z

common/enablers/enableEpochsHandler.go

+	case core.BelowSignedThresholdFlag:
+		return epoch >= handler.enableEpochsConfig.BelowSignedThresholdEnableEpoch
+	case core.SwitchHysteresisForMinNodesFlagInSpecificEpochOnly:
+		return epoch == handler.enableEpochsConfig.SwitchHysteresisForMinNodesEnableEpoch


please re-check here, the original code was:

handler.setFlagValue(epoch >= handler.enableEpochsConfig.SwitchHysteresisForMinNodesEnableEpoch, handler.switchHysteresisForMinNodesFlag, "switchHysteresisForMinNodesFlag") handler.setFlagValue(epoch == handler.enableEpochsConfig.SwitchHysteresisForMinNodesEnableEpoch, handler.switchHysteresisForMinNodesCurrentEpochFlag, "switchHysteresisForMinNodesCurrentEpochFlag")

rechecked: switchHysteresisForMinNodesFlag(with >=) was returned on IsSwitchHysteresisForMinNodesFlagEnabled, which was never used, thus I removed it

iulianpascalau · 2023-08-21T07:57:28Z

common/enablers/enableEpochsHandler.go

+	case core.RepairCallbackFlag:
+		return epoch >= handler.enableEpochsConfig.RepairCallbackEnableEpoch
+	case core.ReturnDataToLastTransferFlagAfterEpoch:
+		return epoch > handler.enableEpochsConfig.ReturnDataToLastTransferEnableEpoch


missing check for BalanceWaitingListsEnableEpoch ?

handler.setFlagValue(epoch >= handler.enableEpochsConfig.BalanceWaitingListsEnableEpoch, handler.balanceWaitingListsFlag, "balanceWaitingListsFlag")

rechecked: balanceWaitingListsFlag(with >=) was returned on IsBalanceWaitingListsFlagEnabled, which was never used, thus I removed it

iulianpascalau · 2023-08-21T08:02:17Z

common/enablers/enableEpochsHandler.go

+		return epoch >= handler.enableEpochsConfig.ReDelegateBelowMinCheckEnableEpoch
+	case core.ValidatorToDelegationFlag:
+		return epoch >= handler.enableEpochsConfig.ValidatorToDelegationEnableEpoch
+	case core.IncrementSCRNonceInMultiTransferFlag:


missing check for WaitingListFixEnableEpoch ?

handler.setFlagValue(epoch >= handler.enableEpochsConfig.WaitingListFixEnableEpoch, handler.waitingListFixFlag, "waitingListFixFlag")

rechecked: waitingListFixFlag(with >=) was returned on IsWaitingListFixFlagEnabled, which was never used, thus I removed it

iulianpascalau · 2023-08-21T08:04:32Z

common/enablers/enableEpochsHandler.go

+	case core.IncrementSCRNonceInMultiTransferFlag:
+		return epoch >= handler.enableEpochsConfig.IncrementSCRNonceInMultiTransferEnableEpoch
+	case core.ESDTMultiTransferFlag,
+		core.ESDTNFTImprovementV1Flag:


what is ESDTNFTImprovementV1Flag ?
In the original code I do not see 2 flags related to the same ESDTMultiTransferEnableEpoch. Please re-check

I remember we had a discussion for these methods that rely on the same flag, to better create a new method with an appropriate naming, rather than reuse the same method

This led to the following methods, which are marked as duplicate:
https://github.com/multiversx/mx-chain-go/blob/rc/v1.6.0/common/enablers/epochFlags.go#L634

With this new approach, I had to create a new flag for them

iulianpascalau · 2023-08-21T08:05:57Z

common/enablers/enableEpochsHandler.go

+	case core.ESDTTransferRoleFlag:
+		return epoch >= handler.enableEpochsConfig.ESDTTransferRoleEnableEpoch
+	case core.BuiltInFunctionOnMetaFlag,
+		core.TransferToMetaFlag:


same, is this a new flag? In the original code I do not see 2 flags coupled with the same BuiltInFunctionOnMetaEnableEpoch value

same as above: https://github.com/multiversx/mx-chain-go/blob/rc/v1.6.0/common/enablers/epochFlags.go#L628

iulianpascalau · 2023-08-21T08:08:07Z

common/enablers/enableEpochsHandler.go

+		return epoch >= handler.enableEpochsConfig.CorrectFirstQueuedEpoch
+	case core.DeleteDelegatorAfterClaimRewardsFlag:
+		return epoch >= handler.enableEpochsConfig.DeleteDelegatorAfterClaimRewardsEnableEpoch
+	case core.RemoveNonUpdatedStorageFlag:


missing check for FixOOGReturnCodeEnableEpoch?

handler.setFlagValue(epoch >= handler.enableEpochsConfig.FixOOGReturnCodeEnableEpoch, handler.fixOOGReturnCodeFlag, "fixOOGReturnCodeFlag")

check exists on L360, flag is FixOOGReturnCodeFlag

right, missed it due to the changed position

iulianpascalau · 2023-08-21T08:09:52Z

common/enablers/enableEpochsHandler.go

+	case core.RemoveNonUpdatedStorageFlag:
+		return epoch >= handler.enableEpochsConfig.RemoveNonUpdatedStorageEnableEpoch
+	case core.OptimizeNFTStoreFlag,
+		core.SaveToSystemAccountFlag,


are SaveToSystemAccountFlag, CheckFrozenCollectionFlag, ValueLengthCheckFlag , CheckTransferFlag new flags? I do not see them on the original code coupled to OptimizeNFTStoreEnableEpoch

they are all duplicated as mentioned above:

SaveToSystemAccountFlag

CheckFrozenCollectionFlag

ValueLengthCheckFlag

CheckTransferFlag

iulianpascalau · 2023-08-21T08:20:25Z

common/enablers/enableEpochsHandler.go

+		return epoch >= handler.enableEpochsConfig.MiniBlockPartialExecutionEnableEpoch
+	case core.ManagedCryptoAPIsFlag:
+		return epoch >= handler.enableEpochsConfig.ManagedCryptoAPIsEnableEpoch
+	case core.ESDTMetadataContinuousCleanupFlag,


on the original code, I see this epoch flag tight only to esdtMetadataContinuousCleanupFlag and changeDelegationOwnerFlag. Is this correct?

same as above, duplicated for consistency: https://github.com/multiversx/mx-chain-go/blob/rc/v1.6.0/common/enablers/epochFlags.go#L610

iulianpascalau · 2023-08-21T08:27:08Z

common/enablers/enableEpochsHandler.go

+	case core.FixOOGReturnCodeFlag:
+		return epoch >= handler.enableEpochsConfig.FixOOGReturnCodeEnableEpoch
+	case core.DeterministicSortOnValidatorsInfoFixFlag:
+		return epoch >= handler.enableEpochsConfig.DeterministicSortOnValidatorsInfoEnableEpoch


missing check for DynamicGasCostForDataTrieStorageLoadEnableEpoch ? (might need a new update branch operation)

handler.setFlagValue(epoch >= handler.enableEpochsConfig.DynamicGasCostForDataTrieStorageLoadEnableEpoch, handler.dynamicGasCostForDataTrieStorageLoadFlag, "dynamicGasCostForDataTrieStorageLoadFlag", epoch, handler.enableEpochsConfig.DynamicGasCostForDataTrieStorageLoadEnableEpoch)

indeed this one is missing due to a missing update branch.. added TODO to be fixed after merge from rc/v1.6.0

…hain-core-go

codecov · 2023-08-21T10:12:49Z

Codecov Report

Patch coverage: 92.07% and project coverage change: +0.18% 🎉

Comparison is base (7ff0863) 80.09% compared to head (459af83) 80.27%.
Report is 198 commits behind head on feat/refactor_enable_epochs_handler.

❗ Current head 459af83 differs from pull request most recent head 2eefc86. Consider uploading reports for the commit 2eefc86 to get more accurate results

Additional details and impacted files

@@                           Coverage Diff                           @@
##           feat/refactor_enable_epochs_handler    #5491      +/-   ##
=======================================================================
+ Coverage                                80.09%   80.27%   +0.18%     
=======================================================================
  Files                                      703      706       +3     
  Lines                                    92715    93726    +1011     
=======================================================================
+ Hits                                     74263    75242     +979     
- Misses                                   13161    13195      +34     
+ Partials                                  5291     5289       -2

Files Changed	Coverage Δ
dataRetriever/topicSender/diffPeerListCreator.go	`100.00% <ø> (ø)`
epochStart/bootstrap/fromLocalStorage.go	`49.43% <0.00%> (-0.86%)`	⬇️
epochStart/bootstrap/metaStorageHandler.go	`73.79% <ø> (-0.18%)`	⬇️
epochStart/bootstrap/shardStorageHandler.go	`87.97% <ø> (-0.03%)`	⬇️
factory/data/dataComponents.go	`91.34% <ø> (-0.09%)`	⬇️
factory/state/stateComponents.go	`79.08% <ø> (-0.27%)`	⬇️
node/metrics/metrics.go	`100.00% <ø> (ø)`
node/node.go	`78.41% <0.00%> (ø)`
...tiflood/factory/p2pAntifloodAndBlacklistFactory.go	`73.98% <0.00%> (-3.12%)`	⬇️
storage/factory/storageServiceFactory.go	`81.66% <ø> (ø)`
... and 75 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

iulianpascalau · 2023-08-22T10:45:43Z

common/enablers/enableEpochsHandler.go

@@ -365,6 +371,12 @@ func (handler *enableEpochsHandler) IsFlagEnabledInEpoch(flag core.EnableEpochFl
 	}
 }

+// GetActivationEpoch returns the activation epoch of the provided flag
+func (handler *enableEpochsHandler) GetActivationEpoch(flag core.EnableEpochFlag) uint32 {


interesting, this can be helpful if exposed on the API routes

AdoAdoAdo · 2023-08-25T13:26:23Z

common/interface.go

@@ -289,6 +289,12 @@ type EnableEpochsHandler interface {
 	RefactorPeersMiniBlocksEnableEpoch() uint32
 	GetCurrentEpoch() uint32

+	IsFlagDefined(flag core.EnableEpochFlag) bool


will you remove the ...EnableEpoch() and IsFlag...() methods in a subsequent PR?

exactly, once all of them are replaced with calls to new ones

AdoAdoAdo · 2023-08-25T13:43:15Z

common/enablers/enableEpochsHandler.go

 	"github.com/multiversx/mx-chain-go/config"
 	"github.com/multiversx/mx-chain-go/process"
 	logger "github.com/multiversx/mx-chain-logger-go"
 )

 var log = logger.GetOrCreate("common/enablers")

+var allFlagsDefined = map[core.EnableEpochFlag]struct{}{
+	common.SCDeployFlag:                                       {},


maybe add here the handler for checking if the flag is enabled in the given epoch as value?

updated as discussed

AdoAdoAdo · 2023-08-25T13:44:43Z

state/trackableDataTrie/trackableDataTrie.go

@@ -248,8 +248,7 @@ func (tdt *trackableDataTrie) updateTrie(dtr state.DataTrie) ([]core.TrieData, e
 }

 func (tdt *trackableDataTrie) retrieveValueFromTrie(key []byte) (core.TrieData, uint32, error) {
-	currentEpoch := tdt.enableEpochsHandler.GetCurrentEpoch()
-	if tdt.enableEpochsHandler.IsAutoBalanceDataTriesEnabledInEpoch(currentEpoch) {
+	if tdt.enableEpochsHandler.IsFlagEnabled(common.AutoBalanceDataTriesFlag) {


I think we should add for each subcomponent also the compatibility check, to find out early that the handlers for finding that a flag is enabled are defined in the map.

added TODO for this and I'll add the checks in the further PRs, while replacing the calls(easier to extract exactly what flags the specific subcomponent needs)

implemented new methods for enable epochs handler + updated mx-chain-…

e96b324

…core-go

sstanculeanu added area:processing impact:low interested:protocol type:refactoring complexity:medium labels Aug 17, 2023

sstanculeanu self-assigned this Aug 17, 2023

sstanculeanu added 2 commits August 17, 2023 11:02

added missing file

b09fe57

completely removed usage of IsAutoBalanceDataTriesEnabledInEpoch in r…

598cbcc

…espect with mx-chain-core-go(the method will be removed in a further commit)

sstanculeanu marked this pull request as ready for review August 17, 2023 09:02

iulianpascalau self-requested a review August 21, 2023 07:49

AdoAdoAdo reviewed Aug 21, 2023

View reviewed changes

iulianpascalau reviewed Aug 21, 2023

View reviewed changes

fixes after review + moved the constants at this level + updated mx-c…

ee33cf7

…hain-core-go

iulianpascalau previously approved these changes Aug 22, 2023

View reviewed changes

AdoAdoAdo requested changes Aug 25, 2023

View reviewed changes

fixes after review

2eefc86

sstanculeanu dismissed iulianpascalau’s stale review via 2eefc86 August 25, 2023 14:17

AdoAdoAdo approved these changes Aug 25, 2023

View reviewed changes

iulianpascalau approved these changes Aug 25, 2023

View reviewed changes

sstanculeanu merged commit 55a8449 into feat/refactor_enable_epochs_handler Aug 25, 2023
5 checks passed

sstanculeanu deleted the new_methods_enable_epochs_handler branch August 25, 2023 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New methods enable epochs handler #5491

New methods enable epochs handler #5491

sstanculeanu commented Aug 17, 2023

AdoAdoAdo Aug 21, 2023 •

edited

AdoAdoAdo Aug 21, 2023

iulianpascalau Aug 21, 2023

iulianpascalau Aug 21, 2023

sstanculeanu Aug 21, 2023 •

edited

iulianpascalau Aug 22, 2023

iulianpascalau Aug 21, 2023

sstanculeanu Aug 21, 2023

iulianpascalau Aug 21, 2023

sstanculeanu Aug 21, 2023

iulianpascalau Aug 21, 2023

sstanculeanu Aug 21, 2023

iulianpascalau Aug 22, 2023

iulianpascalau Aug 21, 2023

sstanculeanu Aug 21, 2023

iulianpascalau Aug 22, 2023

iulianpascalau Aug 21, 2023

sstanculeanu Aug 21, 2023 •

edited

iulianpascalau Aug 22, 2023

iulianpascalau Aug 21, 2023

sstanculeanu Aug 21, 2023

iulianpascalau Aug 21, 2023

sstanculeanu Aug 21, 2023

iulianpascalau Aug 21, 2023

sstanculeanu Aug 21, 2023

codecov bot commented Aug 21, 2023 •

edited

iulianpascalau Aug 22, 2023

AdoAdoAdo Aug 25, 2023

sstanculeanu Aug 25, 2023

AdoAdoAdo Aug 25, 2023

sstanculeanu Aug 25, 2023

AdoAdoAdo Aug 25, 2023

sstanculeanu Aug 25, 2023 •

edited

New methods enable epochs handler #5491

New methods enable epochs handler #5491

Conversation

sstanculeanu commented Aug 17, 2023

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

AdoAdoAdo Aug 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu Aug 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu Aug 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 21, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu Aug 25, 2023 • edited

Choose a reason for hiding this comment

AdoAdoAdo Aug 21, 2023 •

edited

sstanculeanu Aug 21, 2023 •

edited

sstanculeanu Aug 21, 2023 •

edited

codecov bot commented Aug 21, 2023 •

edited

sstanculeanu Aug 25, 2023 •

edited