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

Protection check for stakingV4 configs #4983

Merged
merged 6 commits into from
Feb 15, 2023
Merged

Protection check for stakingV4 configs #4983

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ba6d253
FEAT: Add first version of checking stakingV4 config
mariusmihaic Feb 13, 2023
070ac93
Merge branch 'feat/staking-v4' into MX-13669-stakingv4-epochs-protection
mariusmihaic Feb 13, 2023
e631a64
FEAT: Move config checker in separate file
mariusmihaic Feb 14, 2023
187972c
FEAT: Add unit tests for configChecker.go
mariusmihaic Feb 14, 2023
32181f0
FIX: Unit test
mariusmihaic Feb 14, 2023
e67fd44
FIX: After review
mariusmihaic Feb 14, 2023
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
48 changes: 16 additions & 32 deletions config/configChecker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,55 +2,41 @@ package config

import (
"fmt"

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

var log = logger.GetOrCreate("configChecker")

// SanityCheckEnableEpochsStakingV4 checks if the enable epoch configs for stakingV4 are set correctly
func SanityCheckEnableEpochsStakingV4(cfg *Configs) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

add comment to public function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, added it

enableEpochsCfg := cfg.EpochConfig.EnableEpochs
err := checkStakingV4EpochsOrder(enableEpochsCfg)
if err != nil {
return err
if !areStakingV4StepsInOrder(enableEpochsCfg) {
return errStakingV4StepsNotInOrder
}

numOfShards := cfg.GeneralConfig.GeneralSettings.GenesisMaxNumberOfShards
return checkStakingV4MaxNodesChangeCfg(enableEpochsCfg, numOfShards)
}

func checkStakingV4EpochsOrder(enableEpochsCfg EnableEpochs) error {
stakingV4StepsInOrder := (enableEpochsCfg.StakingV4Step1EnableEpoch < enableEpochsCfg.StakingV4Step2EnableEpoch) &&
(enableEpochsCfg.StakingV4Step2EnableEpoch < enableEpochsCfg.StakingV4Step3EnableEpoch)

if !stakingV4StepsInOrder {
return errStakingV4StepsNotInOrder
}

stakingV4StepsInExpectedOrder := (enableEpochsCfg.StakingV4Step1EnableEpoch == enableEpochsCfg.StakingV4Step2EnableEpoch-1) &&
func areStakingV4StepsInOrder(enableEpochsCfg EnableEpochs) bool {
return (enableEpochsCfg.StakingV4Step1EnableEpoch == enableEpochsCfg.StakingV4Step2EnableEpoch-1) &&
(enableEpochsCfg.StakingV4Step2EnableEpoch == enableEpochsCfg.StakingV4Step3EnableEpoch-1)
if !stakingV4StepsInExpectedOrder {
log.Warn("staking v4 enable epoch steps should be in cardinal order " +
"(e.g.: StakingV4Step1EnableEpoch = 2, StakingV4Step2EnableEpoch = 3, StakingV4Step3EnableEpoch = 4)" +
"; can leave them as they are for playground purposes" +
", but DO NOT use them in production, since system's behavior is undefined")
}

return nil
}

func checkStakingV4MaxNodesChangeCfg(enableEpochsCfg EnableEpochs, numOfShards uint32) error {
maxNodesChangeCfg := enableEpochsCfg.MaxNodesChangeEnableEpoch
if len(maxNodesChangeCfg) <= 1 {
return errNotEnoughMaxNodesChanges
}

maxNodesConfigAdaptedForStakingV4 := false

for idx, currMaxNodesChangeCfg := range enableEpochsCfg.MaxNodesChangeEnableEpoch {
for idx, currMaxNodesChangeCfg := range maxNodesChangeCfg {
if currMaxNodesChangeCfg.EpochEnable == enableEpochsCfg.StakingV4Step3EnableEpoch {

maxNodesConfigAdaptedForStakingV4 = true

if idx == 0 {
log.Warn(fmt.Sprintf("found config change in MaxNodesChangeEnableEpoch for StakingV4Step3EnableEpoch = %d, ", enableEpochsCfg.StakingV4Step3EnableEpoch) +
"but no previous config change entry in MaxNodesChangeEnableEpoch, DO NOT use this config in production")
return fmt.Errorf("found config change in MaxNodesChangeEnableEpoch for StakingV4Step3EnableEpoch = %d, but %w ",
enableEpochsCfg.StakingV4Step3EnableEpoch, errNoMaxNodesConfigBeforeStakingV4)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

could have dropped the else branch improving the code readability.

prevMaxNodesChange := enableEpochsCfg.MaxNodesChangeEnableEpoch[idx-1]
prevMaxNodesChange := maxNodesChangeCfg[idx-1]
err := checkMaxNodesChangedCorrectly(prevMaxNodesChange, currMaxNodesChangeCfg, numOfShards)
if err != nil {
return err
Expand All @@ -70,9 +56,7 @@ func checkStakingV4MaxNodesChangeCfg(enableEpochsCfg EnableEpochs, numOfShards u

func checkMaxNodesChangedCorrectly(prevMaxNodesChange MaxNodesChangeConfig, currMaxNodesChange MaxNodesChangeConfig, numOfShards uint32) error {
if prevMaxNodesChange.NodesToShufflePerShard != currMaxNodesChange.NodesToShufflePerShard {
log.Warn("previous MaxNodesChangeEnableEpoch.NodesToShufflePerShard != MaxNodesChangeEnableEpoch.NodesToShufflePerShard" +
" with EnableEpoch = StakingV4Step3EnableEpoch; can leave them as they are for playground purposes," +
" but DO NOT use them in production, since this will influence rewards")
return errMismatchNodesToShuffle
}

totalShuffled := (numOfShards + 1) * prevMaxNodesChange.NodesToShufflePerShard
Expand Down
43 changes: 35 additions & 8 deletions config/configChecker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func TestSanityCheckEnableEpochsStakingV4(t *testing.T) {
require.Equal(t, errStakingV4StepsNotInOrder, err)
})

t.Run("staking v4 steps not in cardinal order, should work", func(t *testing.T) {
t.Run("staking v4 steps not in cardinal order, should return error", func(t *testing.T) {
t.Parallel()

cfg := generateCorrectConfig()
Expand All @@ -77,22 +77,22 @@ func TestSanityCheckEnableEpochsStakingV4(t *testing.T) {
cfg.EpochConfig.EnableEpochs.StakingV4Step2EnableEpoch = 3
cfg.EpochConfig.EnableEpochs.StakingV4Step3EnableEpoch = 6
err := SanityCheckEnableEpochsStakingV4(cfg)
require.Nil(t, err)
require.Equal(t, errStakingV4StepsNotInOrder, err)

cfg.EpochConfig.EnableEpochs.StakingV4Step1EnableEpoch = 1
cfg.EpochConfig.EnableEpochs.StakingV4Step2EnableEpoch = 2
cfg.EpochConfig.EnableEpochs.StakingV4Step3EnableEpoch = 6
err = SanityCheckEnableEpochsStakingV4(cfg)
require.Nil(t, err)
require.Equal(t, errStakingV4StepsNotInOrder, err)

cfg.EpochConfig.EnableEpochs.StakingV4Step1EnableEpoch = 1
cfg.EpochConfig.EnableEpochs.StakingV4Step2EnableEpoch = 5
cfg.EpochConfig.EnableEpochs.StakingV4Step3EnableEpoch = 6
err = SanityCheckEnableEpochsStakingV4(cfg)
require.Nil(t, err)
require.Equal(t, errStakingV4StepsNotInOrder, err)
})

t.Run("no previous config for max nodes change, should work", func(t *testing.T) {
t.Run("no previous config for max nodes change, should return error", func(t *testing.T) {
t.Parallel()

cfg := generateCorrectConfig()
Expand All @@ -105,14 +105,19 @@ func TestSanityCheckEnableEpochsStakingV4(t *testing.T) {
}

err := SanityCheckEnableEpochsStakingV4(cfg)
require.Nil(t, err)
require.Equal(t, errNotEnoughMaxNodesChanges, err)
})

t.Run("no max nodes config change for StakingV4Step3EnableEpoch, should return error", func(t *testing.T) {
t.Parallel()

cfg := generateCorrectConfig()
cfg.EpochConfig.EnableEpochs.MaxNodesChangeEnableEpoch = []MaxNodesChangeConfig{
{
EpochEnable: 1,
MaxNumNodes: 56,
NodesToShufflePerShard: 2,
},
{
EpochEnable: 444,
MaxNumNodes: 48,
Expand All @@ -126,15 +131,37 @@ func TestSanityCheckEnableEpochsStakingV4(t *testing.T) {
require.True(t, strings.Contains(err.Error(), "6"))
})

t.Run("stakingV4 config for max nodes changed with different nodes to shuffle, should work", func(t *testing.T) {
t.Run("max nodes config change for StakingV4Step3EnableEpoch has no previous config change, should return error", func(t *testing.T) {
t.Parallel()

cfg := generateCorrectConfig()
cfg.EpochConfig.EnableEpochs.MaxNodesChangeEnableEpoch = []MaxNodesChangeConfig{
{
EpochEnable: cfg.EpochConfig.EnableEpochs.StakingV4Step3EnableEpoch,
MaxNumNodes: 48,
NodesToShufflePerShard: 2,
},
{
EpochEnable: 444,
MaxNumNodes: 56,
NodesToShufflePerShard: 2,
},
}

err := SanityCheckEnableEpochsStakingV4(cfg)
require.NotNil(t, err)
require.ErrorIs(t, err, errNoMaxNodesConfigBeforeStakingV4)
})

t.Run("stakingV4 config for max nodes changed with different nodes to shuffle, should return error", func(t *testing.T) {
t.Parallel()

cfg := generateCorrectConfig()
cfg.EpochConfig.EnableEpochs.MaxNodesChangeEnableEpoch[1].NodesToShufflePerShard = 2
cfg.EpochConfig.EnableEpochs.MaxNodesChangeEnableEpoch[2].NodesToShufflePerShard = 4

err := SanityCheckEnableEpochsStakingV4(cfg)
require.Nil(t, err)
require.ErrorIs(t, err, errMismatchNodesToShuffle)
})

t.Run("stakingV4 config for max nodes changed with wrong max num nodes, should return error", func(t *testing.T) {
Expand Down
8 changes: 7 additions & 1 deletion config/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ package config

import "errors"

var errStakingV4StepsNotInOrder = errors.New("staking v4 enable epochs are not in ascending order; expected StakingV4Step1EnableEpoch < StakingV4Step2EnableEpoch < StakingV4Step3EnableEpoch")
var errStakingV4StepsNotInOrder = errors.New("staking v4 enable epoch steps should be in cardinal order(e.g.: StakingV4Step1EnableEpoch = 2, StakingV4Step2EnableEpoch = 3, StakingV4Step3EnableEpoch = 4)")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good and comprehensive errors.


var errNotEnoughMaxNodesChanges = errors.New("not enough entries in MaxNodesChangeEnableEpoch config; expected one entry before stakingV4 and another one starting StakingV4Step3EnableEpoch")

var errNoMaxNodesConfigBeforeStakingV4 = errors.New("no previous config change entry in MaxNodesChangeEnableEpoch before entry with EpochEnable = StakingV4Step3EnableEpoch")

var errMismatchNodesToShuffle = errors.New("previous MaxNodesChangeEnableEpoch.NodesToShufflePerShard != MaxNodesChangeEnableEpoch.NodesToShufflePerShard with EnableEpoch = StakingV4Step3EnableEpoch")

var errNoMaxNodesConfigChangeForStakingV4 = errors.New("no MaxNodesChangeEnableEpoch config found for EpochEnable = StakingV4Step3EnableEpoch")