feat: priority queue #394
feat: priority queue #394
Conversation
| @@ -62,10 +62,12 @@ contract CSModule is | |||
| //////////////////////// | |||
| // State variables below | |||
| //////////////////////// | |||
| /// @dev DEPRECATED. Moved to CSParametersRegistry | |||
| uint256 internal _keyRemovalCharge; | |||
| mapping(uint256 queuePriority => QueueLib.Queue queue) queueByPriority; | |||
There was a problem hiding this comment.
we need to nullify it in the init_v2
There was a problem hiding this comment.
Not necessary, but the point is valid.
| return 40; // Default priority, should be greater than LEGACY_QUEUE_PRIOIRITY. | ||
| } | ||
|
|
||
| function isEligibleForPriorityQueue( |
There was a problem hiding this comment.
I think it can be a single parameter maxKeysPerOperatorInPriorityQueue which tells that it's not eligible if no value is set for the curve
| @@ -85,6 +87,9 @@ contract CSModule is | |||
| uint64 private _depositableValidatorsCount; | |||
| uint64 private _nodeOperatorsCount; | |||
|
|
|||
| // TODO: Place instead of publicRelease. | |||
There was a problem hiding this comment.
We plan to keep public release since it also limits the number of keys per-operator
| uint256 curveId = accounting.getBondCurveId(nodeOperatorId); | ||
| NodeOperator storage no = _nodeOperators[nodeOperatorId]; | ||
|
|
||
| if (PARAMETERS_REGISTRY.isEligibleForPriorityQueue(curveId) && |
There was a problem hiding this comment.
Just use priority. Ne need for a separate flag here
| @@ -85,6 +87,9 @@ contract CSModule is | |||
| uint64 private _depositableValidatorsCount; | |||
| uint64 private _nodeOperatorsCount; | |||
|
|
|||
| // TODO: Place instead of publicRelease. | |||
| uint256[] internal _depositPriorities; | |||
There was a problem hiding this comment.
It should be public to be able to fetch the resulting queue off-chain
| NodeOperator storage no = _nodeOperators[nodeOperatorId]; | ||
|
|
||
| if (PARAMETERS_REGISTRY.isEligibleForPriorityQueue(curveId) && | ||
| PARAMETERS_REGISTRY.maxKeysPerOperatorInPriorityQueue() > no.totalAddedKeys) { |
There was a problem hiding this comment.
This condition seems off. I think we need to have {queuePriority, maxKeysInPriotiryQueue} and place up to maxKeysInPriotiryQueue to the queue with queuePriority the rest to the normal queue
There was a problem hiding this comment.
Sorry, I didn't get it. Can you give an example?
There was a problem hiding this comment.
NO has 100 keys waiting to be placed in the queue. This is a new NO and they had never uploaded keys before. queuePriority = 5, maxKeysInPriotiryQueue = 20. We need to place 20 keys to the queue with queuePriority = 5 and the rest to the default queue
There was a problem hiding this comment.
The other example. NO already has 10 keys added and enqueued in the past and 30 more waiting to be enqueued. queuePriority = 5, maxKeysInPriotiryQueue = 20. We need to add 10 keys to the queue with queuePriority = 5 and the rest to the default queue
| /// @dev Legacy queue (priority=10), that we will probably may be able to remove in the future. | ||
| QueueLib.Queue public legacyQueue; | ||
|
|
||
| uint256 private constant LEGACY_QUEUE_PRIOIRITY = 10; |
There was a problem hiding this comment.
Why not make it zero and put the others on top? In the current approach, you effectively limit the number of priority queues. It might work only if we want to have de-prioritized queues, but do we?
There was a problem hiding this comment.
Like for big NOs with even less priority to enter?
There was a problem hiding this comment.
Yeah. But I think we want to keep the default one with the lowest priority. Hence, the proposal is to make it's priority = 0 and sort priorities in the descending orger
| for (uint256 p; p < _depositPriorities.length; ++p) { | ||
| QueueLib.Queue storage q = _getQueue(_depositPriorities[p]); | ||
| uint128 qLen = q.length(); | ||
|
|
||
| if (index >= qLen) { | ||
| index -= qLen; | ||
| continue; | ||
| } | ||
|
|
||
| return q.at(index); | ||
| } | ||
|
|
||
| return Batch.wrap(0); |
There was a problem hiding this comment.
I did not get this method. I also suggest having a queuePriority as a param here.
There was a problem hiding this comment.
It gets a view to a queue linearly. E.g. we have 2 queues [...]*50 + [...]*50, and we query index 69, which is mapped to the second queue's 19th element. We can achieve the same result off-chain, just with different sets of getters. This one is more unified.
No description provided.