Fix edge case empty waiting list #5061

mariusmihaic · 2023-03-07T11:29:34Z

Reasoning behind the pull request

With stakingV4, shuffled out nodes from eligible are sent to auction, instead of waiting list. This causes nodes to spend one epoch in auction before they are distributed (if selected based on topUP) to waiting. However, in case we have: nodes in waiting < max waiting list size, after shuffling out nodes, there is no need to send nodes to auction, since they will be selected anyway. They can be directly sent to waiting list after shuffling, because next epoch they will be selected anyway. This also fixes some edge cases when waiting lists are almost empty and nodes would be forced to spend extra epochs(auction+waiting) before being sent to eligible.

Proposed changes

In case number of active nodes <= max number of nodes, distribute all shuffled out nodes to waiting instead of auction

Testing procedure

Integration test for edge cases.

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?

codecov-commenter · 2023-03-07T11:38:43Z

Codecov Report

❗ No coverage uploaded for pull request base (feat/staking-v4@8237658). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head a9f8608 differs from pull request most recent head 09be726. Consider uploading reports for the commit 09be726 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@                Coverage Diff                 @@
##             feat/staking-v4    #5061   +/-   ##
==================================================
  Coverage                   ?   70.76%           
==================================================
  Files                      ?      681           
  Lines                      ?    87805           
  Branches                   ?        0           
==================================================
  Hits                       ?    62136           
  Misses                     ?    21065           
  Partials                   ?     4604

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

sasurobert · 2023-03-09T09:59:06Z

integrationTests/vm/staking/stakingV4_test.go

+
+		currNodesConfig = node.NodesConfig
+		require.Len(t, getAllPubKeys(currNodesConfig.eligible), 8)
+		require.Len(t, getAllPubKeys(currNodesConfig.waiting), 4)


duplicated code for checking a lot of requires. Make a separate funciton.

Right, should've done this before.
Refactored to reuse functionality with new functions

sasurobert · 2023-03-09T09:59:46Z

vm/systemSmartContracts/delegation.go

@@ -2883,22 +2883,6 @@ func (d *delegation) executeStakeAndUpdateStatus(
 	return vmcommon.Ok
 }

-func (d *delegation) getConfigStatusAndGlobalFund() (*DelegationConfig, *DelegationContractStatus, *GlobalFundData, error) {


Not used, remained here from a rc->feat merge

Not used, I think it was some merging "leftovers" from an rc->feat

sasurobert · 2023-03-09T13:37:57Z

sharding/nodesCoordinator/hashValidatorShuffler.go

+		// Distribute validators from SHUFFLED OUT -> WAITING
+		err = arg.distributor.DistributeValidators(newWaiting, shuffledOutMap, arg.randomness, arg.flagBalanceWaitingLists)
+		if err != nil {
+			log.Warn("distributeValidators shuffledOut failed", "error", err)


should return error. why only log.Warn in case of critical errors ?

same for all cases.

Did some digging + tests regarding this and it seems like we could safely return error here and it should also be backwards compatible.
Refactored to return error

sasurobert · 2023-03-09T13:41:09Z

sharding/nodesCoordinator/hashValidatorShuffler.go

@@ -283,6 +285,30 @@ func shuffleNodes(arg shuffleNodesArg) (*ResUpdateNodes, error) {

 	shuffledOutMap, newEligible := shuffleOutNodes(newEligible, numToRemove, arg.randomness)

+	numShuffled := getNumPubKeys(shuffledOutMap)


make a separate function for the new code.

Added separate function with little refactor

sasurobert

We should really continue refactoring shuffleNodes function into a full fedged component.

raduchis · 2023-03-16T19:19:48Z

sharding/nodesCoordinator/hashValidatorShuffler.go

@@ -285,26 +297,42 @@ func shuffleNodes(arg shuffleNodesArg) (*ResUpdateNodes, error) {

 	err = moveMaxNumNodesToMap(newEligible, newWaiting, arg.nodesMeta, arg.nodesPerShard)
 	if err != nil {
-		log.Warn("moveNodesToMap failed", "error", err)
+		return nil, fmt.Errorf("moveNodesToMap failed, error: %w", err)


this could be backwards incompatible and is not protected by a flag (before it would just log.warn and now it returns error)

This is related to my comment from here.
An error here could never happend, that's why it was only treated as a warn;
This case never manifested on mainnet anyway, it's safe to refactor it

raduchis · 2023-03-16T21:47:55Z

sharding/nodesCoordinator/hashValidatorShuffler.go

 		// Distribute validators from SHUFFLED OUT -> WAITING
 		err = arg.distributor.DistributeValidators(newWaiting, shuffledOutMap, arg.randomness, arg.flagBalanceWaitingLists)
 		if err != nil {
-			log.Warn("distributeValidators shuffledOut failed", "error", err)
+			return nil, fmt.Errorf("distributeValidators shuffled out failed, error: %w", err)


also backwards incompatibility here?

This is related to my comment from here.
An error here could never happend, that's why it was only treated as a warn;
This case never manifested on mainnet anyway, it's safe to refactor it

mariusmihaic added 3 commits March 6, 2023 14:14

FEAT: Add integration test which fails for now

5bca1bb

FEAT: Add todo workflow

6d4b2f8

FEAT: Possible solution for easy case

26e8245

mariusmihaic added 3 commits March 7, 2023 18:31

FEAT: Possible solution

9712fc0

FIX: Broken condition for impossible case

7217487

FEAT: Continue integration edge case testing

c68293a

mariusmihaic self-assigned this Mar 8, 2023

mariusmihaic added 2 commits March 8, 2023 18:19

FEAT: Extend edge case testing

9f27284

CLN: Comments

8f7f754

mariusmihaic marked this pull request as ready for review March 9, 2023 08:43

mariusmihaic changed the title ~~Mx 13933 fix edge case empty waiting list~~ Fix edge case empty waiting list Mar 9, 2023

FIX: Typo

b56b74c

sasurobert self-requested a review March 9, 2023 09:07

sasurobert reviewed Mar 9, 2023

View reviewed changes

mariusmihaic added 5 commits March 13, 2023 11:34

FIX: Refactor integration tests

db83ac2

CLN: Move test functionalities

77b331d

CLN: Create new func for shouldDistributeShuffledToWaitingInStakingV4

1afecd5

CLN: Refactor error handling + new nodes in shuffler

c26f690

FIX: Return error if moveMaxNumNodesToMap fails

09be726

sasurobert approved these changes Mar 14, 2023

View reviewed changes

raduchis self-requested a review March 15, 2023 10:50

raduchis reviewed Mar 16, 2023

View reviewed changes

raduchis approved these changes Mar 20, 2023

View reviewed changes

mariusmihaic merged commit c27357d into feat/staking-v4 Mar 20, 2023

mariusmihaic deleted the MX-13933-fix-edge-case-empty-waiting-list branch March 20, 2023 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix edge case empty waiting list #5061

Fix edge case empty waiting list #5061

mariusmihaic commented Mar 7, 2023 •

edited

Loading

codecov-commenter commented Mar 7, 2023 •

edited

Loading

sasurobert Mar 9, 2023

mariusmihaic Mar 13, 2023

sasurobert Mar 9, 2023

mariusmihaic Mar 10, 2023

mariusmihaic Mar 13, 2023

sasurobert Mar 9, 2023

sasurobert Mar 9, 2023

mariusmihaic Mar 13, 2023 •

edited

Loading

sasurobert Mar 9, 2023

mariusmihaic Mar 13, 2023

sasurobert left a comment

raduchis Mar 16, 2023

mariusmihaic Mar 20, 2023

raduchis Mar 16, 2023

mariusmihaic Mar 20, 2023

		@@ -283,6 +285,30 @@ func shuffleNodes(arg shuffleNodesArg) (*ResUpdateNodes, error) {

		shuffledOutMap, newEligible := shuffleOutNodes(newEligible, numToRemove, arg.randomness)

		numShuffled := getNumPubKeys(shuffledOutMap)

Fix edge case empty waiting list #5061

Fix edge case empty waiting list #5061

Conversation

mariusmihaic commented Mar 7, 2023 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov-commenter commented Mar 7, 2023 • edited Loading

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

mariusmihaic Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasurobert left a 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

mariusmihaic commented Mar 7, 2023 •

edited

Loading

codecov-commenter commented Mar 7, 2023 •

edited

Loading

mariusmihaic Mar 13, 2023 •

edited

Loading