-
Notifications
You must be signed in to change notification settings - Fork 197
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.
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
Nodes coordinator with staking v4 #3883
Changes from 24 commits
479692d
8c1ed21
40805f9
d87f063
fe9db50
34b4f01
9664050
54087d9
55e09b3
337a353
6e7b730
d6cf445
e63f85b
82bf91e
3ca3f89
be3bc70
b4993df
e306d99
0c6ae5e
9815093
0807341
4fddf7b
044e8e7
8dbcf97
068c23a
ccea211
04b6888
eca5854
09d6a4a
fb6a3b9
9f32944
8f17265
d58e550
7dd0593
6092f80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ type NodesShufflerArgs struct { | |
MaxNodesEnableConfig []config.MaxNodesChangeConfig | ||
BalanceWaitingListsEnableEpoch uint32 | ||
WaitingListFixEnableEpoch uint32 | ||
StakingV4EnableEpoch uint32 | ||
} | ||
|
||
type shuffleNodesArg struct { | ||
|
@@ -32,6 +33,7 @@ type shuffleNodesArg struct { | |
unstakeLeaving []Validator | ||
additionalLeaving []Validator | ||
newNodes []Validator | ||
auction []Validator | ||
randomness []byte | ||
distributor ValidatorsDistributor | ||
nodesMeta uint32 | ||
|
@@ -40,6 +42,7 @@ type shuffleNodesArg struct { | |
maxNodesToSwapPerShard uint32 | ||
flagBalanceWaitingLists bool | ||
flagWaitingListFix bool | ||
flagStakingV4 bool | ||
} | ||
|
||
// TODO: Decide if transaction load statistics will be used for limiting the number of shards | ||
|
@@ -61,6 +64,8 @@ type randHashShuffler struct { | |
flagBalanceWaitingLists atomic.Flag | ||
waitingListFixEnableEpoch uint32 | ||
flagWaitingListFix atomic.Flag | ||
stakingV4EnableEpoch uint32 | ||
flagStakingV4 atomic.Flag | ||
} | ||
|
||
// NewHashValidatorsShuffler creates a validator shuffler that uses a hash between validator key and a given | ||
|
@@ -85,10 +90,12 @@ func NewHashValidatorsShuffler(args *NodesShufflerArgs) (*randHashShuffler, erro | |
availableNodesConfigs: configs, | ||
balanceWaitingListsEnableEpoch: args.BalanceWaitingListsEnableEpoch, | ||
waitingListFixEnableEpoch: args.WaitingListFixEnableEpoch, | ||
stakingV4EnableEpoch: args.StakingV4EnableEpoch, | ||
} | ||
|
||
log.Debug("randHashShuffler: enable epoch for balance waiting list", "epoch", rxs.balanceWaitingListsEnableEpoch) | ||
log.Debug("randHashShuffler: enable epoch for waiting waiting list", "epoch", rxs.waitingListFixEnableEpoch) | ||
log.Debug("randHashShuffler: enable epoch for staking v4", "epoch", rxs.stakingV4EnableEpoch) | ||
|
||
rxs.UpdateParams(args.NodesShard, args.NodesMeta, args.Hysteresis, args.Adaptivity) | ||
|
||
|
@@ -176,6 +183,7 @@ func (rhs *randHashShuffler) UpdateNodeLists(args ArgsUpdateNodes) (*ResUpdateNo | |
unstakeLeaving: args.UnStakeLeaving, | ||
additionalLeaving: args.AdditionalLeaving, | ||
newNodes: args.NewNodes, | ||
auction: args.Auction, | ||
randomness: args.Rand, | ||
nodesMeta: nodesMeta, | ||
nodesPerShard: nodesPerShard, | ||
|
@@ -184,6 +192,7 @@ func (rhs *randHashShuffler) UpdateNodeLists(args ArgsUpdateNodes) (*ResUpdateNo | |
maxNodesToSwapPerShard: rhs.activeNodesConfig.NodesToShufflePerShard, | ||
flagBalanceWaitingLists: rhs.flagBalanceWaitingLists.IsSet(), | ||
flagWaitingListFix: rhs.flagWaitingListFix.IsSet(), | ||
flagStakingV4: rhs.flagStakingV4.IsSet(), | ||
}) | ||
} | ||
|
||
|
@@ -288,16 +297,24 @@ func shuffleNodes(arg shuffleNodesArg) (*ResUpdateNodes, error) { | |
log.Warn("distributeValidators newNodes failed", "error", err) | ||
} | ||
|
||
err = arg.distributor.DistributeValidators(newWaiting, shuffledOutMap, arg.randomness, arg.flagBalanceWaitingLists) | ||
if err != nil { | ||
log.Warn("distributeValidators shuffledOut failed", "error", err) | ||
if arg.flagStakingV4 { | ||
err = distributeValidators(newWaiting, arg.auction, arg.randomness, false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is hard to understand the difference between the 2 versions. Rename this function to distributeValidatorsUsingAuction - or something more clear. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not think that creating an extra function here would bring any benefit, since the only thing it would do is to call I think the way to go here is to add an explanatory comment |
||
if err != nil { | ||
log.Warn("distributeValidators auction list failed", "error", err) | ||
} | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not see why there is log.Warn everywhere - this errors are critical - so they should return error. Add a task to refactor this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RIght, I was also thinking the same when I saw this, but decided to be consistent with the current implementation. |
||
err = arg.distributor.DistributeValidators(newWaiting, shuffledOutMap, arg.randomness, arg.flagBalanceWaitingLists) | ||
if err != nil { | ||
log.Warn("distributeValidators shuffledOut failed", "error", err) | ||
} | ||
} | ||
|
||
actualLeaving, _ := removeValidatorsFromList(allLeaving, stillRemainingInLeaving, len(stillRemainingInLeaving)) | ||
|
||
return &ResUpdateNodes{ | ||
Eligible: newEligible, | ||
Waiting: newWaiting, | ||
ShuffledOut: shuffledOutMap, | ||
Leaving: actualLeaving, | ||
StillRemaining: stillRemainingInLeaving, | ||
}, nil | ||
|
@@ -779,8 +796,12 @@ func (rhs *randHashShuffler) UpdateShufflerConfig(epoch uint32) { | |
|
||
rhs.flagBalanceWaitingLists.SetValue(epoch >= rhs.balanceWaitingListsEnableEpoch) | ||
log.Debug("balanced waiting lists", "enabled", rhs.flagBalanceWaitingLists.IsSet()) | ||
|
||
rhs.flagWaitingListFix.SetValue(epoch >= rhs.waitingListFixEnableEpoch) | ||
log.Debug("waiting list fix", "enabled", rhs.flagWaitingListFix.IsSet()) | ||
|
||
rhs.flagStakingV4.SetValue(epoch >= rhs.stakingV4EnableEpoch) | ||
log.Debug("staking v4", "enabled", rhs.flagStakingV4.IsSet()) | ||
} | ||
|
||
func (rhs *randHashShuffler) sortConfigs() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not unmarshall 2 times - push the epoch here - make a component which is NodesCoordinatorRegistryFactory - which knows depending on the epoch which registry has to be created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good suggestion. I've done this similar to how
CreateHeader
is done, which creates eitherHeader
/HeaderV2
, but your suggestion is way better.Therefore, I've done a pretty big refactored and created a
NodesCoordinatorRegistryFactory
component