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: Low waiting list edge case in StakingV4Step2 #5111

Merged
merged 3 commits into from Apr 3, 2023
Merged

FIX: Low waiting list edge case in StakingV4Step2 #5111

merged 3 commits into from Apr 3, 2023

Conversation

mariusmihaic
Copy link
Contributor

@mariusmihaic mariusmihaic commented Mar 21, 2023 •
edited

Reasoning behind the pull request

  • If there are not enough active nodes in the network and a new owner stakes his nodes in staking v4 step1, his auction nodes would have spend 2 epochs in auction instead of one (stakingV4 step2 + step3).

Proposed changes

  • Send auction list nodes in stakingV4 step2 directly to auction if waiting list is too low

Testing procedure

!!! For both 1. & 2. we should have numNodes (active + waiting) <= 46 !!!

Meanwhile, nodes config should be unchanged,e.g.:

    MaxNodesChangeEnableEpoch = [
        { EpochEnable = 0, MaxNumNodes = 36, NodesToShufflePerShard = 4 },
        { EpochEnable = 1, MaxNumNodes = 56, NodesToShufflePerShard = 2 },
        # Staking v4 configuration, where:
        # - Enable epoch = StakingV4Step3EnableEpoch
        # - NodesToShufflePerShard = same as previous entry in MaxNodesChangeEnableEpoch
        # - MaxNumNodes = (MaxNumNodesFromPreviousEpochEnable - (numOfShards+1)*NodesToShufflePerShard)
        { EpochEnable = 6, MaxNumNodes = 48, NodesToShufflePerShard = 2 },
  1. Start a testnet with feat/staking-v4 ( before fix )
  • In epoch StakingV4Step1EnableEpoch = 4 stake 2 nodes. Both nodes should be in auction. Check validator/auction
  • In epoch StakingV4Step2EnableEpoch = 5 the nodes above should still be in auction.
  • In epoch StakingV4Step2EnableEpoch = 6 those nodes should have been distributed to waiting list
  1. Start a testnet with this branch: MX-13983-fix-edge-case-auction-stakingv4step2 ( after fix )
  • In epoch StakingV4Step1EnableEpoch = 4 stake 2 nodes. Both nodes should be in auction. Check validator/auction
  • In epoch StakingV4Step2EnableEpoch = 5 the nodes above should be distributed to waiting list.
  • In epoch StakingV4Step2EnableEpoch = 6 those nodes should still be in waiting(or already eligibile, depending on the configuration)

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?

@mariusmihaic mariusmihaic self-assigned this Mar 21, 2023
@sasurobert sasurobert self-requested a review March 21, 2023 10:41
sasurobert
sasurobert previously approved these changes Mar 21, 2023
View reviewed changes
raduchis
raduchis requested changes Mar 21, 2023
View reviewed changes
integrationTests/vm/staking/stakingV4_test.go Outdated
// Epoch = 0, before staking v4, owner2 stakes 2 nodes
// - maxNumNodes = 20
// - activeNumNodes = 10
// Newly staked nodes should be sent tu new list
Copy link
Contributor

Choose a reason for hiding this comment

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

tu -> to

sharding/nodesCoordinator/hashValidatorShuffler.go
@@ -325,7 +326,7 @@ func shuffleNodes(arg shuffleNodesArg) (*ResUpdateNodes, error) {
}
}

if shouldDistributeShuffledToWaiting(shuffledNodesCfg) {
if !arg.flagStakingV4Step2 || lowWaitingList {
Copy link
Contributor

Choose a reason for hiding this comment

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

if lowWaitingList is true, it will distributeValidators 2 times? Once on line 318 and once on line 329?

sharding/nodesCoordinator/hashValidatorShuffler.go
if !shuffledNodesCfg.flagStakingV4Step2 {
return true
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

some unitTest should have failed here. Can you please add an unit test?

@codecov
Copy link

codecov bot commented Apr 3, 2023 •
edited

Codecov Report

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

❗ Current head 0486632 differs from pull request most recent head 26f5249. Consider uploading reports for the commit 26f5249 to get more accurate results

Additional details and impacted files 
@@                Coverage Diff                 @@
##             feat/staking-v4    #5111   +/-   ##
==================================================
  Coverage                   ?   71.03%           
==================================================
  Files                      ?      690           
  Lines                      ?    88960           
  Branches                   ?        0           
==================================================
  Hits                       ?    63197           
  Misses                     ?    21078           
  Partials                   ?     4685

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.

@mariusmihaic mariusmihaic merged commit 528757f into feat/staking-v4 Apr 3, 2023
4 checks passed
@mariusmihaic mariusmihaic deleted the MX-13983-fix-edge-case-auction-stakingv4step2 branch April 3, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sasurobert sasurobert sasurobert approved these changes

@raduchis raduchis raduchis approved these changes

Assignees

@mariusmihaic mariusmihaic

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mariusmihaic @sasurobert @raduchis