-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat(M2-Mainnet): StakeRegistry pull updates per quorum #62
Changes from all commits
0a90b61
00e2e2d
bb6c802
f3049d9
23b0698
30ebab1
dc14fc4
8b92066
634138e
2896b31
db4957f
4117e5f
b8c042b
4be4e1d
ef74445
e31b89c
39c943b
213a957
e313d34
037f184
5dc29f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prefer rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? it's public so it matches the getter we're really inconsistent about this throughout both repos 😓 |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
8sunyuan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// 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; | ||
} | ||
Comment on lines
+554
to
+556
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made an earlier comment that we should have this check for both methods and that it didn't cost extra because we're already SLOADing the operatorId But I'm realizing that's actually not the same slot - the If we want to save an extra SLOAD for each operator updated in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorta ambivalent - I like the readability but IIRC gas was a concern so I'm fine with this concept if we feel it's worth it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For weekly avs-sync, 200 operators in quorum0 and 60 operators in 9 remaining quorums, that's ~150,000 gas from the SLOADS. Maybe worth considering removing the check but I also prefer the explicitness |
||
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` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we just store
delegation.withdrawalDelayBlocks()
as an immutable if we've taken out the ability to change it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DelegationManager could still get upgraded and reinitialize
withdrawalDelayBlocks
again so I'm assuming its better we just read off of DelegationManager