refactor: evict inactive services in the checkpoint #141
Conversation
kupermind
commented
Nov 21, 2023
kupermind
commented
- Evict inactive services in the checkpoint.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## downtime_check #141 +/- ##
==================================================
+ Coverage 98.69% 99.80% +1.11%
==================================================
Files 19 19
Lines 1535 1577 +42
Branches 290 292 +2
==================================================
+ Hits 1515 1574 +59
+ Misses 20 3 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| enum ServiceStakingState { | ||
| Unstaked, | ||
| Staked, | ||
| Evicted | ||
| } |
There was a problem hiding this comment.
Giving a bit more information about service staking statuses
| // Get the number of evicted services | ||
| for (uint256 i = 0; i < totalNumServices; ++i) { | ||
| if (evictServiceIds[i] > 0) { | ||
| numEvictServices++; | ||
| } | ||
| } |
There was a problem hiding this comment.
The input array is always of a size of all the staked services, and thus for the event exact number of evicted services must be calculated.
| serviceId = evictServiceIds[i]; | ||
| serviceIds[sCounter] = serviceId; | ||
|
|
||
| ServiceInfo storage sInfo = mapServiceInfo[serviceId]; | ||
| owners[sCounter] = sInfo.owner; | ||
| multisigs[sCounter] = sInfo.multisig; | ||
| serviceInactivity[sCounter] = sInfo.inactivity; | ||
| serviceIndexes[sCounter] = i; |
There was a problem hiding this comment.
Get all the data of each evicted service.
|
|
||
| // Evict services from the global set of staked services | ||
| uint256 idx; | ||
| for (uint256 i = numEvictServices - 1; i > 0; --i) { |
There was a problem hiding this comment.
When evicting, we go backwards, as if we go forward all the indexes will be changed with every deletion.
| uint256[] memory finalEligibleServiceIds; | ||
| uint256[] memory finalEligibleServiceRewards; | ||
| evictServiceIds = new uint256[](serviceIds.length); | ||
| uint256 curServiceId; |
There was a problem hiding this comment.
For the event, we need a precise number of eligible services.
Also, evict services are allocated to the size of the all services in the set of staked services.
| // Get the overall continuous service inactivity | ||
| uint256 inactivity = mapServiceInfo[curServiceId].inactivity + serviceInactivity[i]; | ||
| mapServiceInfo[curServiceId].inactivity = inactivity; | ||
| // Check for the maximum allowed inactivity time | ||
| if (inactivity > maxAllowedInactivity) { | ||
| // Evict a service if it has been inactive for more than a maximum allowed inactivity time | ||
| evictServiceIds[i] = curServiceId; | ||
| } |
There was a problem hiding this comment.
If inactivity is bigger than the allowed one, the service assigned to evicted services list.
|
@mariapiamo I've added requested tests |
test/ServiceStaking.js
Outdated
| // Approve services | ||
| await serviceRegistry.approve(serviceStaking.address, serviceId + i); | ||
|
|
||
| // Stake the first service |
| event ServiceUnstaked(uint256 indexed serviceId, address indexed owner, address indexed multisig, uint256[] nonces, | ||
| uint256 reward, uint256 tsStart); | ||
| event ServiceEvicted(uint256 indexed serviceId, address indexed owner, address indexed multisig, uint256 inactivity); | ||
| event ServiceStaked(uint256 indexed epoch, uint256 indexed serviceId, address indexed multisig, address owner, |
There was a problem hiding this comment.
why is owner not indexed?
There was a problem hiding this comment.
It's max of 3 indexed fields. I've changed that now, not indexing the epoch.
| event ServiceStaked(uint256 indexed epoch, uint256 indexed serviceId, address indexed multisig, address owner, | ||
| uint256[] nonces); | ||
| event Checkpoint(uint256 indexed epoch, uint256 availableRewards, uint256[] serviceIds, uint256[] rewards); | ||
| event ServiceUnstaked(uint256 indexed epoch, uint256 indexed serviceId, address indexed multisig, address owner, |
There was a problem hiding this comment.
see above
why tsStart? What's the point of emitting this here?
| event Checkpoint(uint256 indexed epoch, uint256 availableRewards, uint256[] serviceIds, uint256[] rewards); | ||
| event ServiceUnstaked(uint256 indexed epoch, uint256 indexed serviceId, address indexed multisig, address owner, | ||
| uint256[] nonces, uint256 reward, uint256 tsStart); | ||
| event ServicesEvicted(uint256 indexed epoch, uint256[] serviceIds, address[] owners, address[] multisigs, |
There was a problem hiding this comment.
order of multisigs and owners need to be reverted to be symmetric with the other events
| /// @return Eligible service rewards. | ||
| /// @return success True, if the checkpoint was successful. | ||
| /// @return All staking service Ids (including evicted ones during within a current epoch). | ||
| /// @return All staking updated nonces (including evicted ones during within a current epoch). |
| /// @return success True, if the checkpoint was successful. | ||
| /// @return All staking service Ids (including evicted ones during within a current epoch). | ||
| /// @return All staking updated nonces (including evicted ones during within a current epoch). | ||
| /// @return Set of eligible service Ids. |
There was a problem hiding this comment.
why do we no longer return the nonces?
There was a problem hiding this comment.
We return nonces, we don't return the success flag. No need to return that, just can check for the serviceIds length.
