Rating improvement hard fork stuck shard #3545

mariusmihaic · 2021-10-27T15:08:33Z

Added MaxConsecutiveRoundsOfRatingDecrease , which represents the max number of consecutive rounds in which a validator's rating can be decrease in case of not processing/signing a block. If, for instance, a shard gets stuck for more rounds than this value, then we should stop decreasing validator's rating so they won't get jailed.
Added StopDecreasingValidatorRatingWhenStuckEnableEpoch which represents the epoch when we should stop decreasing validator's rating if, for instance, a shard gets stuck
Added simple logic to check if enable epoch flag is set AND missed rounds > MaxConsecutiveRoundsOfRatingDecrease => do not decrease validator rating
Added unit test

codecov · 2021-10-27T15:17:29Z

Codecov Report

Merging #3545 (2aa7a26) into development (8aa73b5) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           development    #3545      +/-   ##
===============================================
+ Coverage        73.94%   73.95%   +0.01%     
===============================================
  Files              581      581              
  Lines            74780    74794      +14     
===============================================
+ Hits             55293    55311      +18     
+ Misses           15077    15075       -2     
+ Partials          4410     4408       -2

Impacted Files	Coverage Δ
factory/processComponents.go	`62.67% <100.00%> (+0.08%)`	⬆️
process/peer/process.go	`72.99% <100.00%> (+0.46%)`	⬆️
process/block/baseProcess.go	`68.71% <0.00%> (+0.25%)`	⬆️
p2p/libp2p/netMessenger.go	`75.00% <0.00%> (+0.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1eef8df...2aa7a26. Read the comment docs.

SebastianMarian · 2021-10-28T10:12:34Z

cmd/node/config/enableEpochs.toml

@@ -144,6 +144,9 @@
    # RemoveNonUpdatedStorageEnableEpoch represents the epoch when the backward compatibility for removing non updated storage is enabled
    RemoveNonUpdatedStorageEnableEpoch = 2

+    # StopDecreasingValidatorRatingWhenStuckEpoch represents the epoch when we should stop decreasing validator's rating if, for instance, a shard gets stuck
+    StopDecreasingValidatorRatingWhenStuckEpoch = 1000000


StopDecreasingValidatorRatingWhenStuckEnableEpoch to keep the same pattern

Renamed it for consistency

SebastianMarian · 2021-10-28T10:13:04Z

config/epochConfig.go

@@ -61,6 +61,7 @@ type EnableEpochs struct {
 	FixOOGReturnCodeEnableEpoch                 uint32
 	RemoveNonUpdatedStorageEnableEpoch          uint32
 	DeleteDelegatorAfterClaimRewardsEnableEpoch uint32
+	StopDecreasingValidatorRatingWhenStuckEpoch uint32


StopDecreasingValidatorRatingWhenStuckEnableEpoch to keep the same pattern

Renamed it for consistency

SebastianMarian · 2021-10-28T10:15:43Z

process/peer/process.go

+	SwitchJailWaitingEnableEpoch                uint32
+	BelowSignedThresholdEnableEpoch             uint32
+	StakingV2EnableEpoch                        uint32
+	StopDecreasingValidatorRatingWhenStuckEpoch uint32


StopDecreasingValidatorRatingWhenStuckEnableEpoch to keep the same pattern

Renamed it for consistency

SebastianMarian · 2021-10-28T10:16:00Z

process/peer/process.go

+	jailedEnableEpoch                           uint32
+	belowSignedThresholdEnableEpoch             uint32
+	stakingV2EnableEpoch                        uint32
+	stopDecreasingValidatorRatingWhenStuckEpoch uint32


stopDecreasingValidatorRatingWhenStuckEpoch to keep the same pattern

Renamed it for consistency

SebastianMarian · 2021-10-28T10:16:22Z

process/peer/process.go

+	stopDecreasingValidatorRatingWhenStuckEpoch uint32
+	flagJailedEnabled                           atomic.Flag
+	flagStakingV2Enabled                        atomic.Flag
+	flagStopDecreasingValidatorRating           atomic.Flag


flagStopDecreasingValidatorRatingEnabled to keep the same pattern

Renamed it for consistency

…stuck_shard

AdoAdoAdo · 2021-10-28T11:38:27Z

cmd/node/config/enableEpochs.toml

@@ -144,6 +144,9 @@
    # RemoveNonUpdatedStorageEnableEpoch represents the epoch when the backward compatibility for removing non updated storage is enabled
    RemoveNonUpdatedStorageEnableEpoch = 2

+    # StopDecreasingValidatorRatingWhenStuckEnableEpoch represents the epoch when we should stop decreasing validator's rating if, for instance, a shard gets stuck
+    StopDecreasingValidatorRatingWhenStuckEnableEpoch = 1000000


can we have this enabled though?
you can set it to 2, then we can try to test.

Yes, changed it as requested

AdoAdoAdo · 2021-10-28T11:43:37Z

process/peer/process.go

@@ -1230,4 +1243,6 @@ func (vs *validatorStatistics) EpochConfirmed(epoch uint32, _ uint64) {
 	log.Debug("validatorStatistics: jailed", "enabled", vs.flagJailedEnabled.IsSet())
 	vs.flagStakingV2Enabled.Toggle(epoch > vs.stakingV2EnableEpoch)
 	log.Debug("validatorStatistics: stakingV2", vs.flagStakingV2Enabled.IsSet())
+	vs.flagStopDecreasingValidatorRatingEnabled.Toggle(epoch >= vs.stopDecreasingValidatorRatingWhenStuckEnableEpoch)
+	log.Debug("validatorStatistics: stop decreasing validator rating", vs.flagStopDecreasingValidatorRatingEnabled.IsSet())


can you log also the maximum number of rounds?
in case we miss initializing it in the DTO and it ends up default = 0, there will never be any rating decrease.

Very good point!
Added a log for maxConsecutiveRoundsOfRatingDecrease as well.
Also, I've added a check on constructor to check if maxConsecutiveRoundsOfRatingDecrease == 0 => return error

raduchis · 2021-10-28T11:40:00Z

cmd/node/config/config.toml

@@ -23,6 +23,11 @@
   # GenesisMaxNumberOfShards represents the maximum number of shards to be created at genesis (excluding metaChain shard)
   GenesisMaxNumberOfShards  = 3

+   # MaxConsecutiveRoundsOfRatingDecrease represents the max number of consecutive rounds in which a validator's rating


#MaxConsecutiveRoundsOfRatingDecrease representes the max number of consecutive rounds in which a block is not proposed on a shard and the validators' rating could be decreased. (...) then we should stop decreasing validator's rating so they won't get jailed => then the validators rating decrease should stop...

Changed the description as suggested !

raduchis · 2021-10-28T11:47:28Z

process/peer/process_test.go

@@ -1333,6 +1337,43 @@ func TestValidatorStatisticsProcessor_CheckForMissedBlocksNoMissedBlocks(t *test
 	assert.False(t, computeValidatorGroupCalled)
 }

+func TestValidatorStatisticsProcessor_CheckForMissedBlocksMissedRoundsGreaterThanMaxConsecutiveRoundsOfRatingDecrease(t *testing.T) {


Check for actual rating decrease before and after fix is missing.

Good point. I've changed the test.

…stuck_shard

cmd/node/config/config.toml

process/errors.go

…stuck_shard # Conflicts: # config/epochConfig.go

…stuck_shard # Conflicts: # cmd/node/config/enableEpochs.toml # config/epochConfig.go

…stuck_shard

gabi-vuls

System test passed.

FEAT: Add initial flags in config files

3067cf8

SebastianMarian reviewed Oct 28, 2021

View reviewed changes

mariusmihaic added 3 commits October 28, 2021 13:34

FIX: Add flag + tests

2f1b4c3

FIX: Some comments + review findings

3e4a45a

Merge branch 'development' into EN-10889-Rating_improvement_hardfork_…

4b66f7e

…stuck_shard

mariusmihaic marked this pull request as ready for review October 28, 2021 11:01

mariusmihaic self-assigned this Oct 28, 2021

raduchis self-requested a review October 28, 2021 11:15

SebastianMarian previously approved these changes Oct 28, 2021

View reviewed changes

AdoAdoAdo requested changes Oct 28, 2021

View reviewed changes

raduchis requested changes Oct 28, 2021

View reviewed changes

FIX: Review findings

cd48fc2

mariusmihaic dismissed SebastianMarian’s stale review via cd48fc2 October 29, 2021 10:31

Merge branch 'development' into EN-10889-Rating_improvement_hardfork_…

4410fd0

…stuck_shard

mariusmihaic requested review from raduchis and AdoAdoAdo November 1, 2021 09:23

raduchis requested changes Nov 1, 2021

View reviewed changes

cmd/node/config/config.toml Outdated Show resolved Hide resolved

cmd/node/config/config.toml Outdated Show resolved Hide resolved

process/errors.go Outdated Show resolved Hide resolved

FIX: Review findings

410550f

raduchis previously approved these changes Nov 1, 2021

View reviewed changes

AdoAdoAdo previously approved these changes Nov 1, 2021

View reviewed changes

SebastianMarian previously approved these changes Nov 2, 2021

View reviewed changes

Merge branch 'development' into EN-10889-Rating_improvement_hardfork_…

d1d3d9f

…stuck_shard # Conflicts: # config/epochConfig.go

mariusmihaic dismissed stale reviews from SebastianMarian, AdoAdoAdo, and raduchis via d1d3d9f November 8, 2021 14:10

AdoAdoAdo previously approved these changes Nov 8, 2021

View reviewed changes

SebastianMarian previously approved these changes Nov 8, 2021

View reviewed changes

raduchis previously approved these changes Nov 8, 2021

View reviewed changes

FIX: Log

80a1304

mariusmihaic dismissed stale reviews from raduchis, SebastianMarian, and AdoAdoAdo via 80a1304 November 10, 2021 09:21

mariusmihaic and others added 2 commits November 12, 2021 15:39

Merge branch 'development' into EN-10889-Rating_improvement_hardfork_…

9163ccc

…stuck_shard # Conflicts: # cmd/node/config/enableEpochs.toml # config/epochConfig.go

Merge branch 'development' into EN-10889-Rating_improvement_hardfork_…

2aa7a26

…stuck_shard

AdoAdoAdo approved these changes Nov 12, 2021

View reviewed changes

raduchis approved these changes Nov 12, 2021

View reviewed changes

gabi-vuls approved these changes Nov 12, 2021

View reviewed changes

SebastianMarian approved these changes Nov 12, 2021

View reviewed changes

mariusmihaic merged commit 6c0d7c2 into development Nov 12, 2021

mariusmihaic deleted the EN-10889-Rating_improvement_hardfork_stuck_shard branch November 12, 2021 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rating improvement hard fork stuck shard #3545

Rating improvement hard fork stuck shard #3545

mariusmihaic commented Oct 27, 2021 •

edited

codecov bot commented Oct 27, 2021 •

edited

SebastianMarian Oct 28, 2021

mariusmihaic Oct 28, 2021

SebastianMarian Oct 28, 2021

mariusmihaic Oct 28, 2021

SebastianMarian Oct 28, 2021

mariusmihaic Oct 28, 2021

SebastianMarian Oct 28, 2021

mariusmihaic Oct 28, 2021

SebastianMarian Oct 28, 2021

mariusmihaic Oct 28, 2021

AdoAdoAdo Oct 28, 2021

mariusmihaic Oct 29, 2021

AdoAdoAdo Oct 28, 2021

mariusmihaic Oct 29, 2021

raduchis Oct 28, 2021

mariusmihaic Oct 29, 2021

raduchis Oct 28, 2021

mariusmihaic Oct 29, 2021

gabi-vuls left a comment

Rating improvement hard fork stuck shard #3545

Rating improvement hard fork stuck shard #3545

Conversation

mariusmihaic commented Oct 27, 2021 • edited

codecov bot commented Oct 27, 2021 • 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

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

gabi-vuls left a comment

Choose a reason for hiding this comment

mariusmihaic commented Oct 27, 2021 •

edited

codecov bot commented Oct 27, 2021 •

edited