From d17247fa18961abf1751f8bccc279cdbbbe029da Mon Sep 17 00:00:00 2001 From: skhomuti Date: Wed, 29 Nov 2023 22:40:19 +0500 Subject: [PATCH] removed target limit setting in case of stuck keys in favor of additional check in vetKeys --- src/CSModule.sol | 58 ++++++------------ test/CSModule.t.sol | 143 +++++--------------------------------------- 2 files changed, 31 insertions(+), 170 deletions(-) diff --git a/src/CSModule.sol b/src/CSModule.sol index fdf09f3b..bee00f66 100644 --- a/src/CSModule.sol +++ b/src/CSModule.sol @@ -25,8 +25,6 @@ struct NodeOperator { bool active; uint256 targetLimit; bool isTargetLimitActive; - uint256 targetLimitForUnstuck; - bool isTargetLimitActiveForUnstuck; uint256 stuckPenaltyEndTimestamp; uint256 totalExitedKeys; uint256 totalAddedKeys; @@ -114,8 +112,9 @@ contract CSModuleBase { error SameAddress(); error AlreadyProposed(); error InvalidVetKeysPointer(); + error TargetLimitExceeded(); + error StuckKeysPresent(); error InvalidTargetLimit(); - error IncreasingTargetLimitWhenStuckKeys(); error StuckKeysHigherThanTotalDeposited(); error QueueLookupNoLimit(); @@ -623,7 +622,7 @@ contract CSModule is IStakingModule, CSModuleBase { } // @notice update stuck validators count by StakingRouter - // Presence of stuck validators leads to setting target limit to total deposited keys + // Presence of stuck validators leads to stop vetting for the node operator // to prevent further deposits and clean batches from the deposit queue. // @param nodeOperatorIds - bytes packed array of node operator ids // @param stuckValidatorsCounts - bytes packed array of stuck validators counts @@ -654,30 +653,6 @@ contract CSModule is IStakingModule, CSModuleBase { if (stuckValidatorsCount == no.stuckValidatorsCount) continue; no.stuckValidatorsCount = stuckValidatorsCount; - - if (stuckValidatorsCount > 0) { - // @dev the oracle can decrease target limit but can't increase it - // after unstuck, target limit will be set to previous value - no.isTargetLimitActiveForUnstuck = no.isTargetLimitActive; - no.targetLimitForUnstuck = no.targetLimit; - if ( - !no.isTargetLimitActive || - no.targetLimit > no.totalDepositedKeys - ) { - _setTargetLimit( - nodeOperatorId, - true, - no.totalDepositedKeys - ); - } - } else { - _setTargetLimit( - nodeOperatorId, - no.isTargetLimitActiveForUnstuck, - no.targetLimitForUnstuck - ); - } - emit StuckSigningKeysCountChanged( nodeOperatorId, stuckValidatorsCount @@ -710,11 +685,6 @@ contract CSModule is IStakingModule, CSModuleBase { uint256 targetLimit ) external onlyExistingNodeOperator(nodeOperatorId) onlyStakingRouter { // TODO sanity checks? - if (_nodeOperators[nodeOperatorId].stuckValidatorsCount > 0) { - _nodeOperators[nodeOperatorId] - .isTargetLimitActiveForUnstuck = isTargetLimitActive; - _nodeOperators[nodeOperatorId].targetLimitForUnstuck = targetLimit; - } _setTargetLimit(nodeOperatorId, isTargetLimitActive, targetLimit); _incrementModuleNonce(); } @@ -722,8 +692,6 @@ contract CSModule is IStakingModule, CSModuleBase { // @notice update target limits with event emission // target limit decreasing (or appearing) must unvet node operator's keys from the queue // @dev it's not expected (yet) that target limit can be disabled with some value. - // in case of stuck keys, the previous limit stored in the targetLimitForUnstuck field - // hence, don't have to store non-zero value in the targetLimit field function _setTargetLimit( uint256 nodeOperatorId, bool isTargetLimitActive, @@ -734,8 +702,6 @@ contract CSModule is IStakingModule, CSModuleBase { revert InvalidTargetLimit(); NodeOperator storage no = _nodeOperators[nodeOperatorId]; - if (no.stuckValidatorsCount > 0 && targetLimit > no.totalDepositedKeys) - revert IncreasingTargetLimitWhenStuckKeys(); if ( no.isTargetLimitActive == isTargetLimitActive && @@ -777,10 +743,7 @@ contract CSModule is IStakingModule, CSModuleBase { if (vetKeysPointer <= no.totalVettedKeys) revert InvalidVetKeysPointer(); if (vetKeysPointer > no.totalAddedKeys) revert InvalidVetKeysPointer(); - if ( - no.isTargetLimitActive && - vetKeysPointer > (no.totalExitedKeys + no.targetLimit) - ) revert InvalidVetKeysPointer(); + _validateVetKeys(nodeOperatorId, vetKeysPointer); uint64 count = SafeCast.toUint64(vetKeysPointer - no.totalVettedKeys); uint64 start = SafeCast.toUint64(no.totalVettedKeys); @@ -801,6 +764,19 @@ contract CSModule is IStakingModule, CSModuleBase { _incrementModuleNonce(); } + function _validateVetKeys( + uint256 nodeOperatorId, + uint64 vetKeysPointer + ) internal view { + NodeOperator storage no = _nodeOperators[nodeOperatorId]; + + if ( + no.isTargetLimitActive && + vetKeysPointer > (no.totalExitedKeys + no.targetLimit) + ) revert TargetLimitExceeded(); + if (no.stuckValidatorsCount > 0) revert StuckKeysPresent(); + } + function unvetKeys( uint256 nodeOperatorId ) diff --git a/test/CSModule.t.sol b/test/CSModule.t.sol index 25b8828c..12372aab 100644 --- a/test/CSModule.t.sol +++ b/test/CSModule.t.sol @@ -807,7 +807,20 @@ contract CsmVetKeys is CSMCommon { csm.vetKeys(noId, 1); csm.updateTargetValidatorsLimits(noId, true, 1); - vm.expectRevert(InvalidVetKeysPointer.selector); + vm.expectRevert(TargetLimitExceeded.selector); + csm.vetKeys(noId, 2); + } + + function test_vetKeys_RevertWhenStuckKeysPresent() public { + uint256 noId = createNodeOperator(2); + csm.vetKeys(noId, 1); + csm.obtainDepositData(1, ""); + csm.updateStuckValidatorsCount( + bytes.concat(bytes8(0x0000000000000000)), + bytes.concat(bytes16(0x00000000000000000000000000000001)) + ); + + vm.expectRevert(StuckKeysPresent.selector); csm.vetKeys(noId, 2); } } @@ -1123,20 +1136,6 @@ contract CsmUpdateTargetValidatorsLimits is CSMCommon { vm.expectRevert(InvalidTargetLimit.selector); csm.updateTargetValidatorsLimits(noId, false, 10); } - - function test_updateTargetValidatorsLimits_RevertWhenTargetLimitIsGreaterThanDepositedWhenStuck() - public - { - uint256 noId = createNodeOperator(2); - csm.vetKeys(noId, 2); - csm.obtainDepositData(2, ""); - csm.updateStuckValidatorsCount( - bytes.concat(bytes8(0x0000000000000000)), - bytes.concat(bytes16(0x00000000000000000000000000000002)) - ); - vm.expectRevert(IncreasingTargetLimitWhenStuckKeys.selector); - csm.updateTargetValidatorsLimits(noId, true, 4); - } } contract CsmUpdateStuckValidatorsCount is CSMCommon { @@ -1145,8 +1144,6 @@ contract CsmUpdateStuckValidatorsCount is CSMCommon { csm.vetKeys(noId, 3); csm.obtainDepositData(1, ""); - vm.expectEmit(true, true, true, true, address(csm)); - emit TargetValidatorsCountChanged(noId, true, 1); vm.expectEmit(true, true, false, true, address(csm)); emit StuckSigningKeysCountChanged(noId, 1); csm.updateStuckValidatorsCount( @@ -1160,12 +1157,6 @@ contract CsmUpdateStuckValidatorsCount is CSMCommon { 1, "stuckValidatorsCount not increased" ); - assertEq( - summary.targetValidatorsCount, - summary.totalDepositedValidators, - "targetValidatorsCount should be limited to totalDepositedValidators" - ); - assertTrue(summary.isTargetLimitActive, "isTargetLimitActive is false"); } function test_updateStuckValidatorsCount_Unstuck() public { @@ -1178,8 +1169,6 @@ contract CsmUpdateStuckValidatorsCount is CSMCommon { bytes.concat(bytes16(0x00000000000000000000000000000001)) ); - vm.expectEmit(true, true, true, true, address(csm)); - emit TargetValidatorsCountChanged(noId, false, 0); vm.expectEmit(true, true, false, true, address(csm)); emit StuckSigningKeysCountChanged(noId, 0); csm.updateStuckValidatorsCount( @@ -1192,94 +1181,6 @@ contract CsmUpdateStuckValidatorsCount is CSMCommon { 0, "stuckValidatorsCount should be zero" ); - assertEq( - summary.targetValidatorsCount, - 0, - "targetValidatorsCount should be zero" - ); - assertFalse( - summary.isTargetLimitActive, - "isTargetLimitActive should be false" - ); - } - - function test_updateStuckValidatorsCount_DecreaseLimit() public { - uint256 noId = createNodeOperator(); - csm.vetKeys(noId, 1); - csm.obtainDepositData(1, ""); - csm.updateTargetValidatorsLimits(noId, true, 2); - - vm.expectEmit(true, true, true, true, address(csm)); - emit TargetValidatorsCountChanged(noId, true, 1); - csm.updateStuckValidatorsCount( - bytes.concat(bytes8(0x0000000000000000)), - bytes.concat(bytes16(0x00000000000000000000000000000001)) - ); - } - - function test_updateStuckValidatorsCount_ReturnToPrevLimit() public { - uint256 noId = createNodeOperator(); - csm.vetKeys(noId, 1); - csm.obtainDepositData(1, ""); - csm.updateTargetValidatorsLimits(noId, true, 2); - - csm.updateStuckValidatorsCount( - bytes.concat(bytes8(0x0000000000000000)), - bytes.concat(bytes16(0x00000000000000000000000000000001)) - ); - - vm.expectEmit(true, true, true, true, address(csm)); - emit TargetValidatorsCountChanged(noId, true, 2); - csm.updateStuckValidatorsCount( - bytes.concat(bytes8(0x0000000000000000)), - bytes.concat(bytes16(0x00000000000000000000000000000000)) - ); - NodeOperatorSummary memory summary = getNodeOperatorSummary(noId); - assertEq( - summary.stuckValidatorsCount, - 0, - "stuckValidatorsCount should be zero" - ); - assertEq( - summary.targetValidatorsCount, - 2, - "targetValidatorsCount should return to prev value" - ); - assertTrue( - summary.isTargetLimitActive, - "isTargetLimitActive should remain true" - ); - } - - function test_updateStuckValidatorsCount_ReturnToPrevZeroLimit() public { - uint256 noId = createNodeOperator(); - csm.vetKeys(noId, 1); - csm.obtainDepositData(1, ""); - csm.updateTargetValidatorsLimits(noId, true, 0); - - csm.updateStuckValidatorsCount( - bytes.concat(bytes8(0x0000000000000000)), - bytes.concat(bytes16(0x00000000000000000000000000000001)) - ); - - vm.recordLogs(); - csm.updateStuckValidatorsCount( - bytes.concat(bytes8(0x0000000000000000)), - bytes.concat(bytes16(0x00000000000000000000000000000000)) - ); - Vm.Log[] memory logs = vm.getRecordedLogs(); - assertEq(logs.length, 1); - - NodeOperatorSummary memory summary = getNodeOperatorSummary(noId); - assertEq( - summary.targetValidatorsCount, - 0, - "targetValidatorsCount should not change" - ); - assertTrue( - summary.isTargetLimitActive, - "isTargetLimitActive should remain true" - ); } function test_updateStuckValidatorsCount_RevertWhenNoNodeOperator() public { @@ -1330,20 +1231,4 @@ contract CsmUpdateStuckValidatorsCount is CSMCommon { Vm.Log[] memory logs = vm.getRecordedLogs(); assertEq(logs.length, 0); } - - function test_updateStuckValidatorsCount_NoEventWhenLimitSame() public { - uint256 noId = createNodeOperator(); - csm.vetKeys(noId, 1); - csm.obtainDepositData(1, ""); - csm.updateTargetValidatorsLimits(noId, true, 1); - - vm.recordLogs(); - csm.updateStuckValidatorsCount( - bytes.concat(bytes8(0x0000000000000000)), - bytes.concat(bytes16(0x00000000000000000000000000000001)) - ); - Vm.Log[] memory logs = vm.getRecordedLogs(); - // only StuckSigningKeysCountChanged - assertEq(logs.length, 1); - } }