Skip to content
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

Merged
merged 21 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
0a90b61
feat: initial implementation w/o timestamp
8sunyuan Nov 9, 2023
00e2e2d
feat: enforce stake updates for quorum in sigcheck
8sunyuan Nov 9, 2023
bb6c802
fix: update operator stake for just one quorum
8sunyuan Nov 9, 2023
f3049d9
fix: requested changes and optimizations
8sunyuan Nov 10, 2023
23b0698
refactor: move quorumUpdateTimestamp to reg coord
8sunyuan Nov 10, 2023
30ebab1
refactor: remove modifier in favor of helper method (#64)
wadealexc Nov 13, 2023
dc14fc4
fix: initializedQuorumBitmap fix
8sunyuan Nov 13, 2023
8b92066
fix: bitshifting error, needs to shift first before subtracting
8sunyuan Nov 16, 2023
634138e
fix: moved 1 weeks to a constant for now
8sunyuan Nov 16, 2023
2896b31
chore: require statement
8sunyuan Nov 16, 2023
db4957f
refactor: move shared logic to `_updateOperator`
8sunyuan Nov 20, 2023
4117e5f
refactor: added status check to `_updateOperator`
8sunyuan Nov 20, 2023
b8c042b
feat: Delegation.withdrawalDelayBlocks in sigcheck
8sunyuan Nov 21, 2023
4be4e1d
fix: use blocknumber instead of timestamp
8sunyuan Nov 21, 2023
ef74445
fix: unused constant and require error
8sunyuan Nov 21, 2023
e31b89c
feat: timestamp requirement is able to be toggled
8sunyuan Nov 30, 2023
39c943b
fix: interface imports
8sunyuan Dec 4, 2023
213a957
chore: renamed to staleStakesForbidden
8sunyuan Dec 6, 2023
e313d34
chore: typo with BlockNumber
8sunyuan Dec 6, 2023
037f184
fix: rebase remove deleted file
8sunyuan Dec 8, 2023
5dc29f6
fix: prevOperatorAddress not being updated
8sunyuan Dec 8, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions src/BLSSignatureChecker.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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;
}

/**
Expand Down Expand Up @@ -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,
Copy link
Contributor

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?

Copy link
Collaborator Author

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

"BLSSignatureChecker.checkSignatures: StakeRegistry updates must be within withdrawalDelayBlocks window"
);
}
require(
bytes24(nonSignerStakesAndSignature.quorumApks[i].hashG1Point()) ==
blsApkRegistry.getApkHashAtBlockNumberAndIndex(
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer rename value to _staleStakesForbidden

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 😓

}
}
119 changes: 94 additions & 25 deletions src/RegistryCoordinator.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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));

Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 operatorId is 32 bytes, so the status is its own SLOAD.

If we want to save an extra SLOAD for each operator updated in updateOperatorsForQuorum, we could remove the REGISTERED check, put that solely in updateOperators, and just pass the operatorId into this helper method rather than the entire struct.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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`
Expand Down
9 changes: 8 additions & 1 deletion src/interfaces/IBLSSignatureChecker.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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
Expand Down
8 changes: 7 additions & 1 deletion src/interfaces/IRegistryCoordinator.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -134,4 +137,7 @@ interface IRegistryCoordinator {

/// @notice Returns the number of registries
function numRegistries() external view returns (uint256);
}

/// @notice returns the blocknumber the quorum was last updated all at once for all operators
function quorumUpdateBlockNumber(uint8 quorumNumber) external view returns (uint256);
}
2 changes: 2 additions & 0 deletions test/mocks/RegistryCoordinatorMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}
}
Loading