fix: slashable window boundaries #965
Conversation
8sunyuan
force-pushed
the
fix-slashable-windows
branch
from
4a8ea67 to
ca92a20
Compare
| // if the slashableUntil block is in the future, simply read current slashing factors | ||
| // still possible however for the slashing factors to change before the withdrawal is completable | ||
| // and the shares withdrawn to be less | ||
| if (slashableUntil > uint32(block.number)) { |
There was a problem hiding this comment.
this should be >=
alternatively, making it uint32(block.number) > slashableUntil and switching the inners of the conditionals is cleaner
There was a problem hiding this comment.
_getSlashingFactors gets the current slashing factor, so the logic here is that if slashableUntil is at some future block, then we simply read the current slashing factors. Else we read it at the exact slashableUntil block
There was a problem hiding this comment.
Its clearer if we go >=, but the underlying logic is still the same
There was a problem hiding this comment.
I get now what you guys are saying now and agreed.
Updated now e6c66cd
| allocation.effectBlock = uint32(block.number) + DEALLOCATION_DELAY; | ||
| // deallocations are slashable in the window [block.number, block.number + deallocationDelay] | ||
| // therefore, the effectBlock is set to the block right after the slashable window | ||
| allocation.effectBlock = uint32(block.number) + DEALLOCATION_DELAY + 1; |
There was a problem hiding this comment.
Note: every comparison to effectBlock is < effectBlock. There is some minor difference in naming (and resulting comparisons) because we do:
- < effectBlock
- <= slashableUntil
Nomenclature does make sense here though
| // slashableUntil is block inclusive so we need to check if the current block is strictly greater than the slashableUntil block | ||
| // meaning the withdrawal can be completed. | ||
| uint32 slashableUntil = withdrawal.startBlock + MIN_WITHDRAWAL_DELAY_BLOCKS; | ||
| require(slashableUntil < uint32(block.number), WithdrawalDelayNotElapsed()); |
There was a problem hiding this comment.
test_Revert_WhenWithdrawalDelayNotPassed validates this
| uint256 curCumulativeScaledShares = _cumulativeScaledSharesHistory[operator][strategy].latest(); | ||
| uint256 prevCumulativeScaledShares = _cumulativeScaledSharesHistory[operator][strategy].upperLookup({ | ||
| key: uint32(block.number) - MIN_WITHDRAWAL_DELAY_BLOCKS | ||
| key: uint32(block.number) - MIN_WITHDRAWAL_DELAY_BLOCKS - 1 |
8sunyuan
force-pushed
the
fix-slashable-windows
branch
from
ab8550d to
e6c66cd
Compare
Fixing slashable windows here by ensuring that from
[block.number, block.number + MIN_WITHDRAWAL_DELAY_BLOCKS] and
[block.number, block.number + DEALLOCATION_DELAY] are both the same window.
Since we want these two values
MIN_WITHDRAWAL_DELAY_BLOCKSandDEALLOCATION_DELAYto be configured to be the same in deployments, we make the following changes:withdrawal.startBlock + MIN_WITHDRAWAL_DELAY_BLOCKSwhich is the last block the staker's withdrawal is slashable for. That means the first block they can complete withdraw is the block following hence the strict inequalityFor deallocations, we have effectBlock set as
block.number + DEALLOCATION_DELAYcurrently but that is the block that the deallocation is no longer slashable and can be completed. We fix this to ensure the slashable window of[block.number, block.number + DEALLOCATION_DELAY]by making theeffectBlock = block.number + DEALLOCATION_DELAY + 1.For
DelegationManager._getSlashedSharesInQueuewe want also ensure a slashable window of [block.number - MIN_WITHDRAWAL_DELAY_BLOCKS, block.number] including blocknumberblock.number - MIN_WITHDRAWAL_DELAY_BLOCKS. So since a subtraction is done ofcurCumulativeScaledShares - prevCumulativeScaledShares, we make the snapshot.lookup of prevCumulativeScaledShares to be-1to include that block itself.