From 2f64af76d882f685b633f45600b25bbf5f41f130 Mon Sep 17 00:00:00 2001 From: Michael Sun <35479365+8sunyuan@users.noreply.github.com> Date: Fri, 8 Dec 2023 17:12:39 -0500 Subject: [PATCH] Feat(M2-Mainnet): StakeRegistry pull updates per quorum (#62) * feat: initial implementation w/o timestamp * feat: enforce stake updates for quorum in sigcheck * fix: update operator stake for just one quorum Additionally - renamed updateOperatorsPerQuorum to updateOperatorsForQuorum - sort by operator addresses instead of operatorIds * fix: requested changes and optimizations - sorted by ascending address instead of operatorId - sanitization checks on quorumNumbers, added quorumsAllExist modifier - performing `OperatorStatus.REGISTERED` checks now - Removed uneccesary bitmapToBytes calls and doing bytes slicing for quorumNumbers - using internal function and scoping to fix stack-too-deep * refactor: move quorumUpdateTimestamp to reg coord * refactor: remove modifier in favor of helper method (#64) * fix: initializedQuorumBitmap fix small off by 1 error, for example of quorumCount = 1,3 1 << 1 = 2, bit rep is 10 1 << 1 - 1 = 1, bit rep is 01 1 << 3 = 8, bit rep is 1000 1 << 3 - 1 = 7, bit rep is 0111 We should be subtracting by 1 instead of 2 here to get the bitmap * fix: bitshifting error, needs to shift first before subtracting * fix: moved 1 weeks to a constant for now May change this in the future to be configurable * chore: require statement * refactor: move shared logic to `_updateOperator` * refactor: added status check to `_updateOperator` * feat: Delegation.withdrawalDelayBlocks in sigcheck * fix: use blocknumber instead of timestamp * fix: unused constant and require error * feat: timestamp requirement is able to be toggled * fix: interface imports * chore: renamed to staleStakesForbidden * chore: typo with BlockNumber * fix: rebase remove deleted file * fix: prevOperatorAddress not being updated --------- Co-authored-by: Alex <18387287+wadealexc@users.noreply.github.com> --- src/BLSSignatureChecker.sol | 32 ++++++- src/RegistryCoordinator.sol | 119 +++++++++++++++++++----- src/interfaces/IBLSSignatureChecker.sol | 9 +- src/interfaces/IRegistryCoordinator.sol | 8 +- test/mocks/RegistryCoordinatorMock.sol | 2 + 5 files changed, 141 insertions(+), 29 deletions(-) diff --git a/src/BLSSignatureChecker.sol b/src/BLSSignatureChecker.sol index 3b79e6f4..d0469d93 100644 --- a/src/BLSSignatureChecker.sol +++ b/src/BLSSignatureChecker.sol @@ -4,7 +4,7 @@ pragma solidity =0.8.12; import {IBLSSignatureChecker} from "src/interfaces/IBLSSignatureChecker.sol"; import {IRegistryCoordinator} from "src/interfaces/IRegistryCoordinator.sol"; import {IBLSApkRegistry} from "src/interfaces/IBLSApkRegistry.sol"; -import {IStakeRegistry} from "src/interfaces/IStakeRegistry.sol"; +import {IStakeRegistry, IDelegationManager, IServiceManager} from "src/interfaces/IStakeRegistry.sol"; import {BitmapUtils} from "src/libraries/BitmapUtils.sol"; import {BN254} from "src/libraries/BN254.sol"; @@ -26,11 +26,23 @@ contract BLSSignatureChecker is IBLSSignatureChecker { IRegistryCoordinator public immutable registryCoordinator; IStakeRegistry public immutable stakeRegistry; IBLSApkRegistry public immutable blsApkRegistry; + IDelegationManager public immutable delegation; + IServiceManager public immutable serviceManager; + /// @notice If true, check the staleness of the operator stakes and that its within the delegation withdrawalDelayBlocks window. + bool public staleStakesForbidden; + + modifier onlyServiceManagerOwner { + require(msg.sender == serviceManager.owner(), "BLSSignatureChecker.onlyServiceManagerOwner: caller is not the service manager owner"); + _; + } constructor(IRegistryCoordinator _registryCoordinator) { - registryCoordinator = IRegistryCoordinator(_registryCoordinator); + registryCoordinator = _registryCoordinator; stakeRegistry = _registryCoordinator.stakeRegistry(); blsApkRegistry = _registryCoordinator.blsApkRegistry(); + delegation = stakeRegistry.delegation(); + serviceManager = stakeRegistry.serviceManager(); + staleStakesForbidden = true; } /** @@ -72,6 +84,12 @@ contract BLSSignatureChecker is IBLSSignatureChecker { // loop through every quorumNumber and keep track of the apk BN254.G1Point memory apk = BN254.G1Point(0, 0); for (uint i = 0; i < quorumNumbers.length; i++) { + if (staleStakesForbidden) { + require( + registryCoordinator.quorumUpdateBlockNumber(uint8(quorumNumbers[i])) + delegation.withdrawalDelayBlocks() <= block.number, + "BLSSignatureChecker.checkSignatures: StakeRegistry updates must be within withdrawalDelayBlocks window" + ); + } require( bytes24(nonSignerStakesAndSignature.quorumApks[i].hashG1Point()) == blsApkRegistry.getApkHashAtBlockNumberAndIndex( @@ -202,4 +220,14 @@ contract BLSSignatureChecker is IBLSSignatureChecker { PAIRING_EQUALITY_CHECK_GAS ); } + + /** + * ServiceManager owner can either enforce or not that operator stakes are staler + * than the delegation.withdrawalDelayBlocks() window. + * @param value to toggle staleStakesForbidden + */ + function setStaleStakesForbidden(bool value) external onlyServiceManagerOwner { + staleStakesForbidden = value; + emit StaleStakesForbiddenUpdate(value); + } } diff --git a/src/RegistryCoordinator.sol b/src/RegistryCoordinator.sol index 615879f3..54458cd1 100644 --- a/src/RegistryCoordinator.sol +++ b/src/RegistryCoordinator.sol @@ -67,6 +67,9 @@ contract RegistryCoordinator is EIP712, Initializable, IRegistryCoordinator, ISo mapping(address => OperatorInfo) internal _operatorInfo; /// @notice whether the salt has been used for an operator churn approval mapping(bytes32 => bool) public isChurnApproverSaltUsed; + /// @notice mapping from quorum number to the latest block that all quorums were updated all at once + mapping(uint8 => uint256) public quorumUpdateBlockNumber; + /// @notice the dynamic-length array of the registries this coordinator is coordinating address[] public registries; @@ -257,31 +260,70 @@ contract RegistryCoordinator is EIP712, Initializable, IRegistryCoordinator, ISo */ function updateOperators(address[] calldata operators) external onlyWhenNotPaused(PAUSED_UPDATE_OPERATOR) { for (uint256 i = 0; i < operators.length; i++) { - address operator = operators[i]; - OperatorInfo storage operatorInfo = _operatorInfo[operator]; + OperatorInfo memory operatorInfo = _operatorInfo[operator]; bytes32 operatorId = operatorInfo.operatorId; - // Only update operators currently registered for at least one quorum - if (operatorInfo.status != OperatorStatus.REGISTERED) { - continue; - } - + // Update the operator's stake for their active quorums uint192 currentBitmap = _currentOperatorBitmap(operatorId); - bytes memory currentQuorums = BitmapUtils.bitmapToBytesArray(currentBitmap); + bytes memory quorumsToUpdate = BitmapUtils.bitmapToBytesArray(currentBitmap); + _updateOperator(operator, operatorInfo, quorumsToUpdate); + } + } - /** - * Update the operator's stake for their active quorums. The stakeRegistry returns a bitmap - * of quorums where the operator no longer meets the minimum stake, and should be deregistered. - */ - uint192 quorumsToRemove = stakeRegistry.updateOperatorStake(operator, operatorId, currentQuorums); + /** + * @notice Updates the stakes of all operators for each of the specified quorums in the StakeRegistry. Each quorum also + * has their StakeRegistry.quorumTimestamp updated. which is meant to keep track of when operators were last all updated at once. + * @param operatorsPerQuorum is an array of arrays of operators to update for each quorum. Note that each nested array + * of operators must be sorted in ascending address order to ensure that all operators in the quorum are updated + * @param quorumNumbers is an array of quorum numbers to update + * @dev This method is used to update the stakes of all operators in a quorum at once, rather than individually. Performs + * sanitization checks on the input array lengths, quorumNumbers existing, and that quorumNumbers are ordered. Function must + * also not be paused by the PAUSED_UPDATE_OPERATOR flag. + */ + function updateOperatorsForQuorum( + address[][] calldata operatorsPerQuorum, + bytes calldata quorumNumbers + ) external onlyWhenNotPaused(PAUSED_UPDATE_OPERATOR) { + uint192 quorumBitmap = uint192(BitmapUtils.orderedBytesArrayToBitmap(quorumNumbers)); + require(_quorumsAllExist(quorumBitmap), "RegistryCoordinator.updateOperatorsForQuorum: some quorums do not exist"); + require( + operatorsPerQuorum.length == quorumNumbers.length, + "RegistryCoordinator.updateOperatorsForQuorum: input length mismatch" + ); - if (!quorumsToRemove.isEmpty()) { - _deregisterOperator({ - operator: operator, - quorumNumbers: BitmapUtils.bitmapToBytesArray(quorumsToRemove) - }); + for (uint256 i = 0; i < quorumNumbers.length; ++i) { + uint8 quorumNumber = uint8(quorumNumbers[i]); + address[] calldata currQuorumOperators = operatorsPerQuorum[i]; + require( + currQuorumOperators.length == indexRegistry.totalOperatorsForQuorum(quorumNumber), + "RegistryCoordinator.updateOperatorsForQuorum: number of updated operators does not match quorum total" + ); + address prevOperatorAddress = address(0); + // Update stakes for each operator in this quorum + for (uint256 j = 0; j < currQuorumOperators.length; ++j) { + address operator = currQuorumOperators[j]; + OperatorInfo memory operatorInfo = _operatorInfo[operator]; + bytes32 operatorId = operatorInfo.operatorId; + { + uint192 currentBitmap = _currentOperatorBitmap(operatorId); + require( + BitmapUtils.numberIsInBitmap(currentBitmap, quorumNumber), + "RegistryCoordinator.updateOperatorsForQuorum: operator not in quorum" + ); + // Require check is to prevent duplicate operators and that all quorum operators are updated + require( + operator > prevOperatorAddress, + "RegistryCoordinator.updateOperatorsForQuorum: operators array must be sorted in ascending address order" + ); + } + _updateOperator(operator, operatorInfo, quorumNumbers[i:i+1]); + prevOperatorAddress = operator; } + + // Update timestamp that all operators in quorum have been updated all at once + quorumUpdateBlockNumber[quorumNumber] = block.number; + emit QuorumBlockNumberUpdated(quorumNumber, block.number); } } @@ -388,6 +430,7 @@ contract RegistryCoordinator is EIP712, Initializable, IRegistryCoordinator, ISo uint192 quorumsToAdd = uint192(BitmapUtils.orderedBytesArrayToBitmap(quorumNumbers, quorumCount)); uint192 currentBitmap = _currentOperatorBitmap(operatorId); require(!quorumsToAdd.isEmpty(), "RegistryCoordinator._registerOperator: bitmap cannot be 0"); + require(_quorumsAllExist(quorumsToAdd), "RegistryCoordinator._registerOperator: some quorums do not exist"); require(quorumsToAdd.noBitsInCommon(currentBitmap), "RegistryCoordinator._registerOperator: operator already registered for some quorums being registered for"); uint192 newBitmap = uint192(currentBitmap.plus(quorumsToAdd)); @@ -475,6 +518,7 @@ contract RegistryCoordinator is EIP712, Initializable, IRegistryCoordinator, ISo uint192 quorumsToRemove = uint192(BitmapUtils.orderedBytesArrayToBitmap(quorumNumbers, quorumCount)); uint192 currentBitmap = _currentOperatorBitmap(operatorId); require(!quorumsToRemove.isEmpty(), "RegistryCoordinator._deregisterOperator: bitmap cannot be 0"); + require(_quorumsAllExist(quorumsToRemove), "RegistryCoordinator._deregisterOperator: some quorums do not exist"); require(quorumsToRemove.isSubsetOf(currentBitmap), "RegistryCoordinator._deregisterOperator: operator is not registered for specified quorums"); uint192 newBitmap = uint192(currentBitmap.minus(quorumsToRemove)); @@ -498,6 +542,29 @@ contract RegistryCoordinator is EIP712, Initializable, IRegistryCoordinator, ISo indexRegistry.deregisterOperator(operatorId, quorumNumbers); } + /** + * @notice update operator stake for specified quorumsToUpdate, and deregister if necessary + * does nothing if operator is not registered for any quorums. + */ + function _updateOperator( + address operator, + OperatorInfo memory operatorInfo, + bytes memory quorumsToUpdate + ) internal { + if (operatorInfo.status != OperatorStatus.REGISTERED) { + return; + } + bytes32 operatorId = operatorInfo.operatorId; + uint192 quorumsToRemove = stakeRegistry.updateOperatorStake(operator, operatorId, quorumsToUpdate); + + if (!quorumsToRemove.isEmpty()) { + _deregisterOperator({ + operator: operator, + quorumNumbers: BitmapUtils.bitmapToBytesArray(quorumsToRemove) + }); + } + } + /** * @notice Returns the stake threshold required for an incoming operator to replace an existing operator * The incoming operator must have more stake than the return value. @@ -594,6 +661,14 @@ contract RegistryCoordinator is EIP712, Initializable, IRegistryCoordinator, ISo } } + /** + * @notice Returns true iff all of the bits in `quorumBitmap` belong to initialized quorums + */ + function _quorumsAllExist(uint192 quorumBitmap) internal view returns (bool) { + uint192 initializedQuorumBitmap = uint192((1 << quorumCount) - 1); + return quorumBitmap.isSubsetOf(initializedQuorumBitmap); + } + /// @notice Get the most recent bitmap for the operator, returning an empty bitmap if /// the operator is not registered. function _currentOperatorBitmap(bytes32 operatorId) internal view returns (uint192) { @@ -706,13 +781,7 @@ contract RegistryCoordinator is EIP712, Initializable, IRegistryCoordinator, ISo /// @notice Returns the current quorum bitmap for the given `operatorId` or 0 if the operator is not registered for any quorum function getCurrentQuorumBitmap(bytes32 operatorId) external view returns (uint192) { - uint256 quorumBitmapHistoryLength = _operatorBitmapHistory[operatorId].length; - // the first part of this if statement is met if the operator has never registered. - // the second part is met if the operator has previously registered, but is currently deregistered - if (quorumBitmapHistoryLength == 0 || _operatorBitmapHistory[operatorId][quorumBitmapHistoryLength - 1].nextUpdateBlockNumber != 0) { - return 0; - } - return _operatorBitmapHistory[operatorId][quorumBitmapHistoryLength - 1].quorumBitmap; + return _currentOperatorBitmap(operatorId); } /// @notice Returns the length of the quorum bitmap history for the given `operatorId` diff --git a/src/interfaces/IBLSSignatureChecker.sol b/src/interfaces/IBLSSignatureChecker.sol index 85b324f1..ffc73a7b 100644 --- a/src/interfaces/IBLSSignatureChecker.sol +++ b/src/interfaces/IBLSSignatureChecker.sol @@ -3,7 +3,7 @@ pragma solidity =0.8.12; import {IRegistryCoordinator} from "src/interfaces/IRegistryCoordinator.sol"; import {IBLSApkRegistry} from "src/interfaces/IBLSApkRegistry.sol"; -import {IStakeRegistry} from "src/interfaces/IStakeRegistry.sol"; +import {IStakeRegistry, IDelegationManager, IServiceManager} from "src/interfaces/IStakeRegistry.sol"; import {BN254} from "src/libraries/BN254.sol"; @@ -38,12 +38,19 @@ interface IBLSSignatureChecker { // total amount staked by all operators in each quorum uint96[] totalStakeForQuorum; } + + // EVENTS + + /// @notice Emitted when `staleStakesForbiddenUpdate` is set. Value only set by serviceManagerOwner. + event StaleStakesForbiddenUpdate(bool value); // CONSTANTS & IMMUTABLES function registryCoordinator() external view returns (IRegistryCoordinator); function stakeRegistry() external view returns (IStakeRegistry); function blsApkRegistry() external view returns (IBLSApkRegistry); + function delegation() external view returns (IDelegationManager); + function serviceManager() external view returns (IServiceManager); /** * @notice This function is called by disperser when it has aggregated all the signatures of the operators diff --git a/src/interfaces/IRegistryCoordinator.sol b/src/interfaces/IRegistryCoordinator.sol index ca6f8629..bd6fecd3 100644 --- a/src/interfaces/IRegistryCoordinator.sol +++ b/src/interfaces/IRegistryCoordinator.sol @@ -24,6 +24,9 @@ interface IRegistryCoordinator { event EjectorUpdated(address prevEjector, address newEjector); + /// @notice emitted when all the operators for a quorum are updated at once + event QuorumBlockNumberUpdated(uint8 indexed quorumNumber, uint256 blocknumber); + // DATA STRUCTURES enum OperatorStatus { @@ -134,4 +137,7 @@ interface IRegistryCoordinator { /// @notice Returns the number of registries function numRegistries() external view returns (uint256); -} \ No newline at end of file + + /// @notice returns the blocknumber the quorum was last updated all at once for all operators + function quorumUpdateBlockNumber(uint8 quorumNumber) external view returns (uint256); +} diff --git a/test/mocks/RegistryCoordinatorMock.sol b/test/mocks/RegistryCoordinatorMock.sol index 87cbb436..89e4feee 100644 --- a/test/mocks/RegistryCoordinatorMock.sol +++ b/test/mocks/RegistryCoordinatorMock.sol @@ -58,4 +58,6 @@ contract RegistryCoordinatorMock is IRegistryCoordinator { function registerOperator(bytes memory quorumNumbers, bytes calldata) external {} function deregisterOperator(bytes calldata quorumNumbers, bytes calldata) external {} + + function quorumUpdateBlockNumber(uint8 quorumNumber) external view returns (uint256) {} }