Skip to content
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.

Sign up for GitHub

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

Jump to bottom

FIX: Possible backwards incompatibilities fixes #6110

Merged
merged 6 commits into from
Apr 16, 2024
Merged

FIX: Possible backwards incompatibilities fixes #6110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
760a767
FIX: Possible backwards incompatibilities fixes
mariusmihaic Apr 8, 2024
a47a099
Merge branch 'fix-backwards-compatibility-problem-2024.04.03' into MX…
mariusmihaic Apr 11, 2024
c655353
FEAT: Extra safety measure checks
mariusmihaic Apr 11, 2024
f8151b1
CLN: AddNewValidator to trie
mariusmihaic Apr 12, 2024
f5e038b
Merge branch 'rc/v1.7.0' into MX-15351-backwards-compat-issues
mariusmihaic Apr 12, 2024
e1d0f11
FIX: Remove StakingQueueEnabled flag
mariusmihaic Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion common/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1011,7 +1011,6 @@ const (
StakingV4Step1Flag core.EnableEpochFlag = "StakingV4Step1Flag"
StakingV4Step2Flag core.EnableEpochFlag = "StakingV4Step2Flag"
StakingV4Step3Flag core.EnableEpochFlag = "StakingV4Step3Flag"
StakingQueueFlag core.EnableEpochFlag = "StakingQueueFlag"
StakingV4StartedFlag core.EnableEpochFlag = "StakingV4StartedFlag"
AlwaysMergeContextsInEEIFlag core.EnableEpochFlag = "AlwaysMergeContextsInEEIFlag"
// all new flags must be added to createAllFlagsMap method, as part of enableEpochsHandler allFlagsDefined
Expand Down
6 changes: 0 additions & 6 deletions common/enablers/enableEpochsHandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,12 +713,6 @@ func (handler *enableEpochsHandler) createAllFlagsMap() {
},
activationEpoch: handler.enableEpochsConfig.StakingV4Step3EnableEpoch,
},
common.StakingQueueFlag: {
isActiveInEpoch: func(epoch uint32) bool {
return epoch < handler.enableEpochsConfig.StakingV4Step1EnableEpoch
},
activationEpoch: handler.enableEpochsConfig.StakingV4Step1EnableEpoch,
},
common.StakingV4StartedFlag: {
isActiveInEpoch: func(epoch uint32) bool {
return epoch >= handler.enableEpochsConfig.StakingV4Step1EnableEpoch
Expand Down
9 changes: 0 additions & 9 deletions common/enablers/enableEpochsHandler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,13 +192,6 @@ func TestEnableEpochsHandler_IsFlagEnabled(t *testing.T) {
handler.EpochConfirmed(cfg.SetGuardianEnableEpoch+1, 0)
require.True(t, handler.IsFlagEnabled(common.SetGuardianFlag))

handler.EpochConfirmed(cfg.StakingV4Step1EnableEpoch-1, 0)
require.True(t, handler.IsFlagEnabled(common.StakingQueueFlag))
handler.EpochConfirmed(cfg.StakingV4Step1EnableEpoch, 0)
require.False(t, handler.IsFlagEnabled(common.StakingQueueFlag))
handler.EpochConfirmed(cfg.StakingV4Step1EnableEpoch+1, 0)
require.False(t, handler.IsFlagEnabled(common.StakingQueueFlag))

handler.EpochConfirmed(cfg.StakingV4Step1EnableEpoch-1, 0)
require.False(t, handler.IsFlagEnabled(common.StakingV4StartedFlag))
handler.EpochConfirmed(cfg.StakingV4Step1EnableEpoch, 0)
Expand Down Expand Up @@ -318,7 +311,6 @@ func TestEnableEpochsHandler_IsFlagEnabled(t *testing.T) {
require.False(t, handler.IsFlagEnabled(common.StakingV4Step1Flag))
require.True(t, handler.IsFlagEnabled(common.StakingV4Step2Flag))
require.True(t, handler.IsFlagEnabled(common.StakingV4Step3Flag))
require.False(t, handler.IsFlagEnabled(common.StakingQueueFlag))
require.True(t, handler.IsFlagEnabled(common.StakingV4StartedFlag))
require.True(t, handler.IsFlagEnabled(common.AlwaysMergeContextsInEEIFlag))
}
Expand Down Expand Up @@ -434,7 +426,6 @@ func TestEnableEpochsHandler_GetActivationEpoch(t *testing.T) {
require.Equal(t, cfg.StakingV4Step1EnableEpoch, handler.GetActivationEpoch(common.StakingV4Step1Flag))
require.Equal(t, cfg.StakingV4Step2EnableEpoch, handler.GetActivationEpoch(common.StakingV4Step2Flag))
require.Equal(t, cfg.StakingV4Step3EnableEpoch, handler.GetActivationEpoch(common.StakingV4Step3Flag))
require.Equal(t, cfg.StakingV4Step1EnableEpoch, handler.GetActivationEpoch(common.StakingQueueFlag))
require.Equal(t, cfg.StakingV4Step1EnableEpoch, handler.GetActivationEpoch(common.StakingV4StartedFlag))
require.Equal(t, cfg.AlwaysMergeContextsInEEIEnableEpoch, handler.GetActivationEpoch(common.AlwaysMergeContextsInEEIFlag))
}
Expand Down
103 changes: 66 additions & 37 deletions epochStart/metachain/legacySystemSCs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ func (s *legacySystemSCProcessor) processLegacy(
return err
}

if s.enableEpochsHandler.IsFlagEnabled(common.StakingQueueFlag) {
if !s.enableEpochsHandler.IsFlagEnabled(common.StakingV4StartedFlag) {
err = s.stakeNodesFromQueue(validatorsInfoMap, numUnStaked, nonce, common.NewList)
if err != nil {
return err
Expand Down Expand Up @@ -292,8 +292,7 @@ func (s *legacySystemSCProcessor) unStakeNodesWithNotEnoughFunds(
stakingV4Enabled := s.enableEpochsHandler.IsFlagEnabled(common.StakingV4StartedFlag)
validatorLeaving := validatorInfo.ShallowClone()
validatorLeaving.SetListAndIndex(string(common.LeavingList), validatorLeaving.GetIndex(), stakingV4Enabled)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the bool stakingV4Enabled should be removed from the SetListAndIndex.
I think it should be called SetListAndIndexUpdatePrevious and should always update the previous list and index.

For the previous/backwards compatible version just the SetList should be called on a different if branch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my point of view, this would create too much overhead without any noticeable benefit.
This func is already backwards compatible and is better to only have one func, so that it is not mistakenly used after stakingV4, since we rely that previous values are always updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the benefit would be a cleaner code: in the future SetListAndIndex will be used always with the updatePrevious. All future calls will get a "legacy" argument always true.
could remain like this and be refactored in the future.

err = s.replaceValidators(validatorInfo, validatorLeaving, validatorsInfoMap)
log.LogIfError(err)
s.replaceValidators(validatorInfo, validatorLeaving, validatorsInfoMap)
}

err = s.updateDelegationContracts(mapOwnersKeys)
Expand Down Expand Up @@ -430,16 +429,28 @@ func (s *legacySystemSCProcessor) fillStakingDataForNonEligible(validatorsInfoMa
}

if deleteCalled {
err := validatorsInfoMap.SetValidatorsInShard(shId, newList)
if err != nil {
return err
}
s.setValidatorsInShard(validatorsInfoMap, shId, newList)
raduchis marked this conversation as resolved.
Show resolved Hide resolved
}
}

return nil
}

func (s *legacySystemSCProcessor) setValidatorsInShard(
validatorsInfoMap state.ShardValidatorsInfoMapHandler,
shardID uint32,
validators []state.ValidatorInfoHandler,
) {
err := validatorsInfoMap.SetValidatorsInShard(shardID, validators)
if err == nil {
return
}

// this should never happen, but replace them anyway, as in old legacy code
log.Error("legacySystemSCProcessor.setValidatorsInShard", "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there is an error on the validatorsInfoMap.SetValidatorsInShard(shardID, validators) with StakingV4 activated, should not an error be returned, and not try the legacy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to return error in that case and block execution. Not a strong opinion if we should return error, though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the new version will be never used here... if it fails for any reason, the old version will be used, and the new checks are ignored

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err := validatorsInfoMap.SetValidatorsInShard(shardID, validators) is actually the new version

validatorsInfoMap.SetValidatorsInShardUnsafe(shardID, validators)
}

func (s *legacySystemSCProcessor) prepareStakingDataForEligibleNodes(validatorsInfoMap state.ShardValidatorsInfoMapHandler) error {
eligibleNodes, err := getEligibleNodeKeys(validatorsInfoMap)
if err != nil {
Expand Down Expand Up @@ -587,14 +598,21 @@ func (s *legacySystemSCProcessor) updateMaxNodes(validatorsInfoMap state.ShardVa
return err
}

if s.enableEpochsHandler.IsFlagEnabled(common.StakingQueueFlag) {
sw.Start("stakeNodesFromQueue")
err = s.stakeNodesFromQueue(validatorsInfoMap, maxNumberOfNodes-prevMaxNumberOfNodes, nonce, common.NewList)
sw.Stop("stakeNodesFromQueue")
if err != nil {
return err
}
if s.enableEpochsHandler.IsFlagEnabled(common.StakingV4StartedFlag) {
return nil
}

if maxNumberOfNodes < prevMaxNumberOfNodes {
return epochStart.ErrInvalidMaxNumberOfNodes
}

sw.Start("stakeNodesFromQueue")
err = s.stakeNodesFromQueue(validatorsInfoMap, maxNumberOfNodes-prevMaxNumberOfNodes, nonce, common.NewList)
sw.Stop("stakeNodesFromQueue")
if err != nil {
return err
}

return nil
}

Expand Down Expand Up @@ -752,8 +770,7 @@ func (s *legacySystemSCProcessor) stakingToValidatorStatistics(
}

newValidatorInfo := s.validatorInfoCreator.PeerAccountToValidatorInfo(account)
err = s.replaceValidators(jailedValidator, newValidatorInfo, validatorsInfoMap)
log.LogIfError(err)
s.replaceValidators(jailedValidator, newValidatorInfo, validatorsInfoMap)

return blsPubKey, nil
}
Expand All @@ -762,14 +779,21 @@ func (s *legacySystemSCProcessor) replaceValidators(
old state.ValidatorInfoHandler,
new state.ValidatorInfoHandler,
validatorsInfoMap state.ShardValidatorsInfoMapHandler,
) error {
stakingV4Enabled := s.enableEpochsHandler.IsFlagEnabled(common.StakingV4StartedFlag)
if stakingV4Enabled {
return validatorsInfoMap.Replace(old, new)
) {
// legacy code
if !s.enableEpochsHandler.IsFlagEnabled(common.StakingV4StartedFlag) {
_ = validatorsInfoMap.ReplaceValidatorByKey(old.GetPublicKey(), new, old.GetShardId())
return
}

_ = validatorsInfoMap.ReplaceValidatorByKey(old.GetPublicKey(), new, old.GetShardId())
return nil
// try with new code which does extra validity checks.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not "try" something... are the new checks used in other places? how is the error handling there? if any new check fails and we revert to the old... we could just do the old...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.Replace is used in 2 other places (after stakingV4 is enabled), where we correctly call the functiona and return error if exists, since that is new code and I tried to correctly use the func

// if this also fails, do legacy code
if err := validatorsInfoMap.Replace(old, new); err != nil {
log.Error("legacySystemSCProcessor.replaceValidators", "error", err)

replaced := validatorsInfoMap.ReplaceValidatorByKey(old.GetPublicKey(), new, old.GetShardId())
log.Debug("legacySystemSCProcessor.replaceValidators", "old", old.GetPublicKey(), "new", new.GetPublicKey(), "was replace successful", replaced)
}
}

func isValidator(validator state.ValidatorInfoHandler) bool {
Expand Down Expand Up @@ -1219,11 +1243,6 @@ func (s *legacySystemSCProcessor) addNewlyStakedNodesToValidatorTrie(
return err
}

err = peerAcc.SetBLSPublicKey(blsKey)
if err != nil {
return err
}

peerAcc.SetListAndIndex(peerAcc.GetShardId(), string(list), uint32(nonce), s.enableEpochsHandler.IsFlagEnabled(common.StakingV4StartedFlag))
peerAcc.SetTempRating(s.startRating)
peerAcc.SetUnStakedEpoch(common.DefaultUnstakedEpoch)
Expand All @@ -1244,22 +1263,32 @@ func (s *legacySystemSCProcessor) addNewlyStakedNodesToValidatorTrie(
AccumulatedFees: big.NewInt(0),
}

existingValidator := validatorsInfoMap.GetValidator(validatorInfo.GetPublicKey())
// This fix is not be backwards incompatible
if !check.IfNil(existingValidator) && s.enableEpochsHandler.IsFlagEnabled(common.StakingV4StartedFlag) {
err = validatorsInfoMap.Delete(existingValidator)
if err != nil {
return err
}
err = s.addNewValidator(validatorsInfoMap, validatorInfo)
if err != nil {
return err
}
}

return nil
}

func (s *legacySystemSCProcessor) addNewValidator(
validatorsInfoMap state.ShardValidatorsInfoMapHandler,
validatorInfo state.ValidatorInfoHandler,
) error {
if !s.enableEpochsHandler.IsFlagEnabled(common.StakingV4StartedFlag) {
return validatorsInfoMap.Add(validatorInfo)
}

err = validatorsInfoMap.Add(validatorInfo)
existingValidator := validatorsInfoMap.GetValidator(validatorInfo.GetPublicKey())
if !check.IfNil(existingValidator) {
err := validatorsInfoMap.Delete(existingValidator)
if err != nil {
return err
}
}

return nil
return validatorsInfoMap.Add(validatorInfo)
}

func (s *legacySystemSCProcessor) initESDT() error {
Expand All @@ -1277,7 +1306,7 @@ func (s *legacySystemSCProcessor) extractConfigFromESDTContract() ([][]byte, err
CallerAddr: s.endOfEpochCallerAddress,
Arguments: [][]byte{},
CallValue: big.NewInt(0),
GasProvided: math.MaxUint64,
GasProvided: math.MaxInt64,
raduchis marked this conversation as resolved.
Show resolved Hide resolved
},
Function: "getContractConfig",
RecipientAddr: vm.ESDTSCAddress,
Expand Down
1 change: 0 additions & 1 deletion epochStart/metachain/systemSCs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ func NewSystemSCProcessor(args ArgsNewEpochStartSystemSCProcessing) (*systemSCPr
common.SaveJailedAlwaysFlag,
common.StakingV4Step1Flag,
common.StakingV4Step2Flag,
common.StakingQueueFlag,
common.StakingV4StartedFlag,
common.DelegationSmartContractFlagInSpecificEpochOnly,
common.GovernanceFlagInSpecificEpochOnly,
Expand Down
4 changes: 4 additions & 0 deletions epochStart/metachain/systemSCs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2326,6 +2326,10 @@ func TestSystemSCProcessor_LegacyEpochConfirmedCorrectMaxNumNodesAfterNodeRestar
args.EpochNotifier.CheckEpoch(&block.Header{Epoch: 6, Nonce: 6})
require.True(t, s.flagChangeMaxNodesEnabled.IsSet())
err = s.processLegacy(validatorsInfoMap, 6, 6)
require.Equal(t, epochStart.ErrInvalidMaxNumberOfNodes, err)

args.EnableEpochsHandler.(*enableEpochsHandlerMock.EnableEpochsHandlerStub).AddActiveFlags(common.StakingV4StartedFlag)
err = s.processLegacy(validatorsInfoMap, 6, 6)
require.Nil(t, err)
require.Equal(t, nodesConfigEpoch6.MaxNumNodes, s.maxNodes)

Expand Down
1 change: 1 addition & 0 deletions state/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ type ShardValidatorsInfoMapHandler interface {
Replace(old ValidatorInfoHandler, new ValidatorInfoHandler) error
ReplaceValidatorByKey(oldBlsKey []byte, new ValidatorInfoHandler, shardID uint32) bool
SetValidatorsInShard(shardID uint32, validators []ValidatorInfoHandler) error
SetValidatorsInShardUnsafe(shardID uint32, validators []ValidatorInfoHandler)
}

// ValidatorInfoHandler defines which data shall a validator info hold.
Expand Down
8 changes: 8 additions & 0 deletions state/validatorsInfoMap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,14 @@ func (vi *shardValidatorsInfoMap) SetValidatorsInShard(shardID uint32, validator
return nil
}

// SetValidatorsInShardUnsafe resets all validators saved in a specific shard with the provided ones.
// It does not check that provided validators are in the same shard as provided shard id.
func (vi *shardValidatorsInfoMap) SetValidatorsInShardUnsafe(shardID uint32, validators []ValidatorInfoHandler) {
vi.mutex.Lock()
vi.valInfoMap[shardID] = validators
vi.mutex.Unlock()
}

// Delete will delete the provided validator from the internally stored map, if found.
// The validators slice at the corresponding shardID key will be re-sliced, without reordering
func (vi *shardValidatorsInfoMap) Delete(validator ValidatorInfoHandler) error {
Expand Down
Loading