From 3c419c62220c79aa4fac05c83a0acc3eb46734c2 Mon Sep 17 00:00:00 2001 From: Geoff Stuart Date: Tue, 3 Dec 2024 08:44:20 -0500 Subject: [PATCH] Rough rip-out of churn tracker --- .../validator-manager/ValidatorManager.sol | 100 +++--------------- .../interfaces/IValidatorManager.sol | 12 ++- .../tests/PoAValidatorManagerTests.t.sol | 14 +-- .../tests/PoSValidatorManagerTests.t.sol | 6 +- 4 files changed, 26 insertions(+), 106 deletions(-) diff --git a/contracts/validator-manager/ValidatorManager.sol b/contracts/validator-manager/ValidatorManager.sol index 6df7be3a2..027cf2a21 100644 --- a/contracts/validator-manager/ValidatorManager.sol +++ b/contracts/validator-manager/ValidatorManager.sol @@ -9,10 +9,10 @@ import {ValidatorMessages} from "./ValidatorMessages.sol"; import { InitialValidator, IValidatorManager, + IChurnTracker, PChainOwner, ConversionData, Validator, - ValidatorChurnPeriod, ValidatorManagerSettings, ValidatorRegistrationInput, ValidatorStatus @@ -38,12 +38,9 @@ abstract contract ValidatorManager is Initializable, ContextUpgradeable, IValida struct ValidatorManagerStorage { /// @notice The l1ID associated with this validator manager. bytes32 _l1ID; - /// @notice The number of seconds after which to reset the churn tracker. - uint64 _churnPeriodSeconds; - /// @notice The maximum churn rate allowed per churn period. - uint8 _maximumChurnPercentage; + uint64 _totalWeight; /// @notice The churn tracker used to track the amount of stake added or removed in the churn period. - ValidatorChurnPeriod _churnTracker; + IChurnTracker _churnTracker; /// @notice Maps the validationID to the registration message such that the message can be re-sent if needed. mapping(bytes32 => bytes) _pendingRegisterValidationMessages; /// @notice Maps the validationID to the validator information. @@ -119,16 +116,7 @@ abstract contract ValidatorManager is Initializable, ContextUpgradeable, IValida { ValidatorManagerStorage storage $ = _getValidatorManagerStorage(); $._l1ID = settings.l1ID; - - if ( - settings.maximumChurnPercentage > MAXIMUM_CHURN_PERCENTAGE_LIMIT - || settings.maximumChurnPercentage == 0 - ) { - revert InvalidMaximumChurnPercentage(settings.maximumChurnPercentage); - } - - $._maximumChurnPercentage = settings.maximumChurnPercentage; - $._churnPeriodSeconds = settings.churnPeriodSeconds; + $._churnTracker = IChurnTracker(settings.churnTracker); } modifier initializedValidatorSet() { @@ -187,13 +175,7 @@ abstract contract ValidatorManager is Initializable, ContextUpgradeable, IValida validationID, initialValidator.nodeID, initialValidator.weight ); } - $._churnTracker.totalWeight = totalWeight; - - // Rearranged equation for totalWeight < (100 / $._maximumChurnPercentage) - // Total weight must be above this value in order to not trigger churn limits with an added/removed weight of 1. - if (totalWeight * $._maximumChurnPercentage < 100) { - revert InvalidTotalWeight(totalWeight); - } + $._churnTracker.setTotalWeight(totalWeight); // Verify that the sha256 hash of the L1 conversion data matches with the Warp message's conversionID. bytes32 conversionID = ValidatorMessages.unpackSubnetToL1ConversionMessage( @@ -246,11 +228,6 @@ abstract contract ValidatorManager is Initializable, ContextUpgradeable, IValida revert InvalidRegistrationExpiry(input.registrationExpiry); } - // Ensure the new validator doesn't overflow the total weight - if (uint256(weight) + uint256($._churnTracker.totalWeight) > type(uint64).max) { - revert InvalidTotalWeight(weight); - } - _validatePChainOwner(input.remainingBalanceOwner); _validatePChainOwner(input.disableOwner); @@ -266,8 +243,8 @@ abstract contract ValidatorManager is Initializable, ContextUpgradeable, IValida revert NodeAlreadyRegistered(input.nodeID); } - // Check that adding this validator would not exceed the maximum churn rate. - _checkAndUpdateChurnTracker(weight, 0); + // Check that changing the validator weight would not exceed the maximum churn rate. + $._churnTracker.checkAndUpdateChurnTracker(weight, 0); (bytes32 validationID, bytes memory registerL1ValidatorMessage) = ValidatorMessages .packRegisterL1ValidatorMessage( @@ -503,6 +480,11 @@ abstract contract ValidatorManager is Initializable, ContextUpgradeable, IValida return warpMessage; } + function _getChurnPeriodSeconds() internal returns (uint64) { + ValidatorManagerStorage storage $ = _getValidatorManagerStorage(); + return $._churnTracker.getChurnPeriodSeconds(); + } + function _setValidatorWeight( bytes32 validationID, uint64 newWeight @@ -511,7 +493,7 @@ abstract contract ValidatorManager is Initializable, ContextUpgradeable, IValida uint64 validatorWeight = $._validationPeriods[validationID].weight; // Check that changing the validator weight would not exceed the maximum churn rate. - _checkAndUpdateChurnTracker(newWeight, validatorWeight); + $._churnTracker.checkAndUpdateChurnTracker(newWeight, validatorWeight); uint64 nonce = _incrementAndGetNonce(validationID); @@ -531,60 +513,4 @@ abstract contract ValidatorManager is Initializable, ContextUpgradeable, IValida return (nonce, messageID); } - - function _getChurnPeriodSeconds() internal view returns (uint64) { - return _getValidatorManagerStorage()._churnPeriodSeconds; - } - - /** - * @dev Helper function to check if the stake weight to be added or removed would exceed the maximum stake churn - * rate for the past churn period. If the churn rate is exceeded, the function will revert. If the churn rate is - * not exceeded, the function will update the churn tracker with the new weight. - */ - function _checkAndUpdateChurnTracker( - uint64 newValidatorWeight, - uint64 oldValidatorWeight - ) private { - ValidatorManagerStorage storage $ = _getValidatorManagerStorage(); - - uint64 weightChange; - if (newValidatorWeight > oldValidatorWeight) { - weightChange = newValidatorWeight - oldValidatorWeight; - } else { - weightChange = oldValidatorWeight - newValidatorWeight; - } - - uint256 currentTime = block.timestamp; - ValidatorChurnPeriod memory churnTracker = $._churnTracker; - - if ( - churnTracker.startedAt == 0 - || currentTime >= churnTracker.startedAt + $._churnPeriodSeconds - ) { - churnTracker.churnAmount = weightChange; - churnTracker.startedAt = currentTime; - churnTracker.initialWeight = churnTracker.totalWeight; - } else { - // Churn is always additive whether the weight is being added or removed. - churnTracker.churnAmount += weightChange; - } - - // Rearranged equation of maximumChurnPercentage >= currentChurnPercentage to avoid integer division truncation. - if ($._maximumChurnPercentage * churnTracker.initialWeight < churnTracker.churnAmount * 100) - { - revert MaxChurnRateExceeded(churnTracker.churnAmount); - } - - // Two separate calculations because we're using uints and (newValidatorWeight - oldValidatorWeight) could underflow. - churnTracker.totalWeight += newValidatorWeight; - churnTracker.totalWeight -= oldValidatorWeight; - - // Rearranged equation for totalWeight < (100 / $._maximumChurnPercentage) - // Total weight must be above this value in order to not trigger churn limits with an added/removed weight of 1. - if (churnTracker.totalWeight * $._maximumChurnPercentage < 100) { - revert InvalidTotalWeight(churnTracker.totalWeight); - } - - $._churnTracker = churnTracker; - } } diff --git a/contracts/validator-manager/interfaces/IValidatorManager.sol b/contracts/validator-manager/interfaces/IValidatorManager.sol index e0c2280de..8b621b4bc 100644 --- a/contracts/validator-manager/interfaces/IValidatorManager.sol +++ b/contracts/validator-manager/interfaces/IValidatorManager.sol @@ -57,8 +57,7 @@ struct ValidatorChurnPeriod { */ struct ValidatorManagerSettings { bytes32 l1ID; - uint64 churnPeriodSeconds; - uint8 maximumChurnPercentage; + address churnTracker; } /** @@ -94,6 +93,15 @@ struct ValidatorRegistrationInput { PChainOwner disableOwner; } +interface IChurnTracker { + function checkAndUpdateChurnTracker( + uint64 newValidatorWeight, + uint64 oldValidatorWeight + ) external; + function getChurnPeriodSeconds() external returns (uint64); + function setTotalWeight(uint64) external; +} + /** * @notice Interface for Validator Manager contracts that implement Subnet-only Validator management. */ diff --git a/contracts/validator-manager/tests/PoAValidatorManagerTests.t.sol b/contracts/validator-manager/tests/PoAValidatorManagerTests.t.sol index 0602df452..92fe2f7e1 100644 --- a/contracts/validator-manager/tests/PoAValidatorManagerTests.t.sol +++ b/contracts/validator-manager/tests/PoAValidatorManagerTests.t.sol @@ -36,12 +36,7 @@ contract PoAValidatorManagerTest is ValidatorManagerTest { app = new PoAValidatorManager(ICMInitializable.Disallowed); vm.expectRevert(abi.encodeWithSelector(Initializable.InvalidInitialization.selector)); app.initialize( - ValidatorManagerSettings({ - l1ID: DEFAULT_L1_ID, - churnPeriodSeconds: DEFAULT_CHURN_PERIOD, - maximumChurnPercentage: DEFAULT_MAXIMUM_CHURN_PERCENTAGE - }), - address(this) + ValidatorManagerSettings({l1ID: DEFAULT_L1_ID, churnTracker: address(0)}), address(this) ); } @@ -112,12 +107,7 @@ contract PoAValidatorManagerTest is ValidatorManagerTest { function _setUp() internal override returns (IValidatorManager) { app = new PoAValidatorManager(ICMInitializable.Allowed); app.initialize( - ValidatorManagerSettings({ - l1ID: DEFAULT_L1_ID, - churnPeriodSeconds: DEFAULT_CHURN_PERIOD, - maximumChurnPercentage: DEFAULT_MAXIMUM_CHURN_PERCENTAGE - }), - address(this) + ValidatorManagerSettings({l1ID: DEFAULT_L1_ID, churnTracker: address(0)}), address(this) ); validatorManager = app; diff --git a/contracts/validator-manager/tests/PoSValidatorManagerTests.t.sol b/contracts/validator-manager/tests/PoSValidatorManagerTests.t.sol index b338f38d5..77f015b00 100644 --- a/contracts/validator-manager/tests/PoSValidatorManagerTests.t.sol +++ b/contracts/validator-manager/tests/PoSValidatorManagerTests.t.sol @@ -2452,11 +2452,7 @@ abstract contract PoSValidatorManagerTest is ValidatorManagerTest { function _defaultPoSSettings() internal pure returns (PoSValidatorManagerSettings memory) { return PoSValidatorManagerSettings({ - baseSettings: ValidatorManagerSettings({ - l1ID: DEFAULT_L1_ID, - churnPeriodSeconds: DEFAULT_CHURN_PERIOD, - maximumChurnPercentage: DEFAULT_MAXIMUM_CHURN_PERCENTAGE - }), + baseSettings: ValidatorManagerSettings({l1ID: DEFAULT_L1_ID, churnTracker: address(0)}), minimumStakeAmount: DEFAULT_MINIMUM_STAKE_AMOUNT, maximumStakeAmount: DEFAULT_MAXIMUM_STAKE_AMOUNT, minimumStakeDuration: DEFAULT_MINIMUM_STAKE_DURATION,