FIX: Possible backwards incompatibilities fixes #6110
Conversation
…-15351-backwards-compat-issues
| @@ -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) | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // this should never happen, but replace them anyway, as in old legacy code | ||
| log.Error("legacySystemSCProcessor.setValidatorsInShard", "error", err) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I didn't want to return error in that case and block execution. Not a strong opinion if we should return error, though
There was a problem hiding this comment.
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
There was a problem hiding this comment.
err := validatorsInfoMap.SetValidatorsInShard(shardID, validators) is actually the new version
| return epochStart.ErrInvalidMaxNumberOfNodes | ||
| } | ||
| } | ||
|
|
||
| if s.enableEpochsHandler.IsFlagEnabled(common.StakingQueueFlag) { |
There was a problem hiding this comment.
don't think this flag is needed... on the old version of the systemSC this was not here.
There was a problem hiding this comment.
It is mandatory to be there, that flag says:
- if there is still a staking queue (so staking v4 not started)
- stake nodes from queue
We don't want to have anything to do with the queue once stakingV4 is started
There was a problem hiding this comment.
after a closer inspection, it looks like the
s.enableEpochsHandler.IsFlagEnabled(common.StakingQueueFlag) equals !s.enableEpochsHandler.IsFlagEnabled(common.StakingV4StartedFlag)
it could be refactored so that only the StakingV4StartedFlag is used
There was a problem hiding this comment.
Removed StakingQueueFlag
| } | ||
|
|
||
| _ = validatorsInfoMap.ReplaceValidatorByKey(old.GetPublicKey(), new, old.GetShardId()) | ||
| return nil | ||
| // try with new code which does extra validity checks. |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
.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
There was a problem hiding this comment.
Normal allin test: v1.6.15-dev-config-eb2e06c06d -> MX-15351-backwards-compat--e1d0f11464
--- Specific errors ---
block hash does not match 368
wrong nonce in block 316
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0
/------/
--- Statistics ---
Nr. of all ERRORS: 0
Nr. of all WARNS: 229
Nr. of new ERRORS: 0
Nr. of new WARNS: 8
Nr. of PANICS: 0
/------/
--- ERRORS ---
/------/
--- WARNINGS ---
/------/
Reasoning behind the pull request