Keep max num nodes limit #2995
Keep max num nodes limit #2995
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2995 +/- ##
==========================================
- Coverage 75.06% 75.06% -0.01%
==========================================
Files 629 629
Lines 60655 60776 +121
==========================================
+ Hits 45531 45619 +88
- Misses 11020 11040 +20
- Partials 4104 4117 +13
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| for i := len(waitingListData.blsKeys) - 1; i >= 0; i-- { | ||
| stakedData := waitingListData.stakedDataList[i] |
There was a problem hiding this comment.
just to make sure, is the len(waitingListData.blsKeys) enforced to always be equal or less than len(waitingListData.stakedDataList) ?
If not enforced, I would add a check before the loop that the two are equal and error otherwise, to avoid panics.
There was a problem hiding this comment.
I would still add a check, just in case, before the loop.
if len(waitingListData.blsKeys) != len(waitingListData.stakedDataList) return an error
| if numQualified < numRegisteredKeys { | ||
| mapCheckedOwners[string(owner)] = false | ||
| return false, nil | ||
| if numQualified >= numRegisteredKeys { |
There was a problem hiding this comment.
why not numQualified > numRegisteredKeys?
why do we set qualifiedToStake to true if the registered keys are equal to number of qualified nodes?
wouldn't this cause a stake of an aditional node from staking queue in function stakeNodesFromQueue when not needed?
There was a problem hiding this comment.
can you please add a test to validate that we do not stake an aditional node from staking queue if we already have exactly numQualified nodes active and an aditional node in the staking queue on the first position?
There was a problem hiding this comment.
The test is already added. All the nodes from the additional queue need those 2500eGLD - thus if numQualified == numRegistered - it means the validator has enough funds to have all of its nodes active.
epochStart: initialize ownerStats for unstaked at eoe
| @@ -1183,12 +1191,57 @@ func (s *systemSCProcessor) updateSystemSCContractsCode(contractMetadata []byte) | |||
| return nil | |||
| } | |||
|
|
|||
| func (s *systemSCProcessor) cleanAdditionalQueue() error { | |||
| vmInput := &vmcommon.ContractCallInput{ | |||
There was a problem hiding this comment.
please add this:
sw := core.NewStopWatch()
sw.Start("systemSCProcessor")
defer func() {
sw.Stop("systemSCProcessor")
log.Info("systemSCProcessor.cleanAdditionalQueue time measurements", sw.GetMeasurements()...)
}()| @@ -1543,32 +1545,31 @@ func (s *stakingSC) resetLastUnJailedFromQueue(args *vmcommon.ContractCallInput) | |||
|
|
|||
| func (s *stakingSC) cleanAdditionalQueueNotEnoughFunds( | |||
There was a problem hiding this comment.
cleanAdditionalQueueNotEnoughFundsReturningUnstaked is a better name now
There was a problem hiding this comment.
that is too long.
| if s.flagCorrectNumNodesToStake.IsSet() { | ||
| err := s.cleanAdditionalQueue() | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
should we consider this a critical error? Shouldn't we just log.Error the error?
There was a problem hiding this comment.
it here we have a critical error - the contracts will not work as expected.
There was a problem hiding this comment.
ok, makes sense. Returning error here will avoid of having an inconsistent state. The metachain will halt and we have to do the fixes so it will resume the operation.
Clean waiting list nodes when no nodes are qualified
Keep the maximum number of nodes property always