Skip to content

Commit

Permalink
Consensus fix review (#1031)
Browse files Browse the repository at this point in the history 
Co-authored-by: Bruno França <[email protected]>
  • Loading branch information
@vladbochok @brunoffranca
vladbochok and brunoffranca authored Oct 28, 2024
1 parent 64859dc commit 563056e
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 48 deletions.
137 changes: 97 additions & 40 deletions l2-contracts/contracts/ConsensusRegistry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ pragma solidity 0.8.24;
import {Ownable2StepUpgradeable} from "@openzeppelin/contracts-upgradeable-v4/access/Ownable2StepUpgradeable.sol";
import {Initializable} from "@openzeppelin/contracts-upgradeable-v4/proxy/utils/Initializable.sol";
import {IConsensusRegistry} from "./interfaces/IConsensusRegistry.sol";
import {ZeroAddress} from "./errors/L2ContractErrors.sol";

/// @author Matter Labs
/// @custom:security-contact [email protected]
Expand All @@ -21,8 +22,14 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
/// @dev A mapping of node owners => nodes.
mapping(address => Node) public nodes;
/// @dev A mapping for enabling efficient lookups when checking whether a given attester public key exists.
/// @dev Initially, the mappings mark the public keys used by the attesters in the current committee. However,
/// @dev after calling the changeAttesterKey functions, the mappings might also contain public keys of attesters
/// @dev that will only be part of the committee once the contract owner updates the attestersCommit state variable.
mapping(bytes32 => bool) public attesterPubKeyHashes;
/// @dev A mapping for enabling efficient lookups when checking whether a given validator public key exists.
/// @dev Initially, the mappings mark the public keys used by the validators in the current committee. However,
/// @dev after calling the changeValidatorKey functions, the mappings might also contain public keys of validators
/// @dev that will only be part of the committee once the contract owner updates the validatorsCommit state variable.
mapping(bytes32 => bool) public validatorPubKeyHashes;
/// @dev Counter that increments with each new commit to the attester committee.
uint32 public attestersCommit;
Expand All @@ -36,9 +43,14 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
_;
}

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
_disableInitializers();
}

function initialize(address _initialOwner) external initializer {
if (_initialOwner == address(0)) {
revert InvalidInputNodeOwnerAddress();
revert ZeroAddress();
}
_transferOwnership(_initialOwner);
}
Expand All @@ -47,16 +59,20 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
/// @dev Fails if node owner already exists.
/// @dev Fails if a validator/attester with the same public key already exists.
/// @param _nodeOwner The address of the new node's owner.
/// @param _isValidatorActive A flag stating if the validator starts activated.
/// @param _validatorWeight The voting weight of the validator.
/// @param _validatorPubKey The BLS12-381 public key of the validator.
/// @param _validatorPoP The proof-of-possession (PoP) of the validator's public key.
/// @param _isAttesterActive A flag stating if the attester starts activated.
/// @param _attesterWeight The voting weight of the attester.
/// @param _attesterPubKey The ECDSA public key of the attester.
function add(
address _nodeOwner,
bool _isValidatorActive,
uint32 _validatorWeight,
BLS12_381PublicKey calldata _validatorPubKey,
BLS12_381Signature calldata _validatorPoP,
bool _isAttesterActive,
uint32 _attesterWeight,
Secp256k1PublicKey calldata _attesterPubKey
) external onlyOwner {
Expand All @@ -65,6 +81,12 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
_verifyInputBLS12_381PublicKey(_validatorPubKey);
_verifyInputBLS12_381Signature(_validatorPoP);
_verifyInputSecp256k1PublicKey(_attesterPubKey);
if (_attesterWeight == 0) {
revert ZeroAttesterWeight();
}
if (_validatorWeight == 0) {
revert ZeroValidatorWeight();
}

// Verify storage.
_verifyNodeOwnerDoesNotExist(_nodeOwner);
Expand All @@ -77,8 +99,8 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
nodeOwners.push(_nodeOwner);
nodes[_nodeOwner] = Node({
attesterLatest: AttesterAttr({
active: true,
removed: false,
active: _isAttesterActive,
weight: _attesterWeight,
pubKey: _attesterPubKey
}),
Expand All @@ -90,8 +112,8 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
}),
attesterLastUpdateCommit: attestersCommit,
validatorLatest: ValidatorAttr({
active: true,
removed: false,
active: _isValidatorActive,
weight: _validatorWeight,
pubKey: _validatorPubKey,
proofOfPossession: _validatorPoP
Expand All @@ -111,50 +133,82 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg

emit NodeAdded({
nodeOwner: _nodeOwner,
isValidatorActive: _isValidatorActive,
validatorWeight: _validatorWeight,
validatorPubKey: _validatorPubKey,
validatorPoP: _validatorPoP,
isAttesterActive: _isAttesterActive,
attesterWeight: _attesterWeight,
attesterPubKey: _attesterPubKey
});
}

/// @notice Deactivates a node, preventing it from participating in committees.
/// @notice Deactivates an attester, preventing it from participating in attester committees.
/// @dev Only callable by the contract owner or the node owner.
/// @dev Verifies that the node owner exists in the registry.
/// @param _nodeOwner The address of the node's owner to be inactivated.
function deactivate(address _nodeOwner) external onlyOwnerOrNodeOwner(_nodeOwner) {
function deactivateAttester(address _nodeOwner) external onlyOwnerOrNodeOwner(_nodeOwner) {
_verifyNodeOwnerExists(_nodeOwner);
(Node storage node, bool deleted) = _getNodeAndDeleteIfRequired(_nodeOwner);
if (deleted) {
return;
}

_ensureAttesterSnapshot(node);
_snapshotAttesterIfOutdated(node);
node.attesterLatest.active = false;
_ensureValidatorSnapshot(node);
node.validatorLatest.active = false;

emit NodeDeactivated(_nodeOwner);
emit AttesterDeactivated(_nodeOwner);
}

/// @notice Activates a previously inactive node, allowing it to participate in committees.
/// @notice Deactivates a validator, preventing it from participating in validator committees.
/// @dev Only callable by the contract owner or the node owner.
/// @dev Verifies that the node owner exists in the registry.
/// @param _nodeOwner The address of the node's owner to be inactivated.
function deactivateValidator(address _nodeOwner) external onlyOwnerOrNodeOwner(_nodeOwner) {
_verifyNodeOwnerExists(_nodeOwner);
(Node storage node, bool deleted) = _getNodeAndDeleteIfRequired(_nodeOwner);
if (deleted) {
return;
}

_snapshotValidatorIfOutdated(node);
node.validatorLatest.active = false;

emit ValidatorDeactivated(_nodeOwner);
}

/// @notice Activates a previously inactive attester, allowing it to participate in attester committees.
/// @dev Only callable by the contract owner.
/// @dev Verifies that the node owner exists in the registry.
/// @param _nodeOwner The address of the node's owner to be activated.
function activate(address _nodeOwner) external onlyOwnerOrNodeOwner(_nodeOwner) {
function activateAttester(address _nodeOwner) external onlyOwner {
_verifyNodeOwnerExists(_nodeOwner);
(Node storage node, bool deleted) = _getNodeAndDeleteIfRequired(_nodeOwner);
if (deleted) {
return;
}

_ensureAttesterSnapshot(node);
_snapshotAttesterIfOutdated(node);
node.attesterLatest.active = true;
_ensureValidatorSnapshot(node);

emit AttesterActivated(_nodeOwner);
}

/// @notice Activates a previously inactive validator, allowing it to participate in validator committees.
/// @dev Only callable by the contract owner.
/// @dev Verifies that the node owner exists in the registry.
/// @param _nodeOwner The address of the node's owner to be activated.
function activateValidator(address _nodeOwner) external onlyOwner {
_verifyNodeOwnerExists(_nodeOwner);
(Node storage node, bool deleted) = _getNodeAndDeleteIfRequired(_nodeOwner);
if (deleted) {
return;
}

_snapshotValidatorIfOutdated(node);
node.validatorLatest.active = true;

emit NodeActivated(_nodeOwner);
emit ValidatorActivated(_nodeOwner);
}

/// @notice Removes a node from the registry.
Expand All @@ -168,9 +222,9 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
return;
}

_ensureAttesterSnapshot(node);
_snapshotAttesterIfOutdated(node);
node.attesterLatest.removed = true;
_ensureValidatorSnapshot(node);
_snapshotValidatorIfOutdated(node);
node.validatorLatest.removed = true;

emit NodeRemoved(_nodeOwner);
Expand All @@ -180,15 +234,18 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
/// @dev Only callable by the contract owner.
/// @dev Verifies that the node owner exists in the registry.
/// @param _nodeOwner The address of the node's owner whose validator weight will be changed.
/// @param _weight The new validator weight to assign to the node.
/// @param _weight The new validator weight to assign to the node, must be greater than 0.
function changeValidatorWeight(address _nodeOwner, uint32 _weight) external onlyOwner {
if (_weight == 0) {
revert ZeroValidatorWeight();
}
_verifyNodeOwnerExists(_nodeOwner);
(Node storage node, bool deleted) = _getNodeAndDeleteIfRequired(_nodeOwner);
if (deleted) {
return;
}

_ensureValidatorSnapshot(node);
_snapshotValidatorIfOutdated(node);
node.validatorLatest.weight = _weight;

emit NodeValidatorWeightChanged(_nodeOwner, _weight);
Expand All @@ -198,22 +255,25 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
/// @dev Only callable by the contract owner.
/// @dev Verifies that the node owner exists in the registry.
/// @param _nodeOwner The address of the node's owner whose attester weight will be changed.
/// @param _weight The new attester weight to assign to the node.
/// @param _weight The new attester weight to assign to the node, must be greater than 0.
function changeAttesterWeight(address _nodeOwner, uint32 _weight) external onlyOwner {
if (_weight == 0) {
revert ZeroAttesterWeight();
}
_verifyNodeOwnerExists(_nodeOwner);
(Node storage node, bool deleted) = _getNodeAndDeleteIfRequired(_nodeOwner);
if (deleted) {
return;
}

_ensureAttesterSnapshot(node);
_snapshotAttesterIfOutdated(node);
node.attesterLatest.weight = _weight;

emit NodeAttesterWeightChanged(_nodeOwner, _weight);
}

/// @notice Changes the validator's public key and proof-of-possession in the registry.
/// @dev Only callable by the contract owner or the node owner.
/// @dev Only callable by the contract owner.
/// @dev Verifies that the node owner exists in the registry.
/// @param _nodeOwner The address of the node's owner whose validator key and PoP will be changed.
/// @param _pubKey The new BLS12-381 public key to assign to the node's validator.
Expand All @@ -222,7 +282,7 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
address _nodeOwner,
BLS12_381PublicKey calldata _pubKey,
BLS12_381Signature calldata _pop
) external onlyOwnerOrNodeOwner(_nodeOwner) {
) external onlyOwner {
_verifyInputBLS12_381PublicKey(_pubKey);
_verifyInputBLS12_381Signature(_pop);
_verifyNodeOwnerExists(_nodeOwner);
Expand All @@ -236,22 +296,19 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
bytes32 newHash = _hashValidatorPubKey(_pubKey);
_verifyValidatorPubKeyDoesNotExist(newHash);
validatorPubKeyHashes[newHash] = true;
_ensureValidatorSnapshot(node);
_snapshotValidatorIfOutdated(node);
node.validatorLatest.pubKey = _pubKey;
node.validatorLatest.proofOfPossession = _pop;

emit NodeValidatorKeyChanged(_nodeOwner, _pubKey, _pop);
}

/// @notice Changes the attester's public key of a node in the registry.
/// @dev Only callable by the contract owner or the node owner.
/// @dev Only callable by the contract owner.
/// @dev Verifies that the node owner exists in the registry.
/// @param _nodeOwner The address of the node's owner whose attester public key will be changed.
/// @param _pubKey The new ECDSA public key to assign to the node's attester.
function changeAttesterKey(
address _nodeOwner,
Secp256k1PublicKey calldata _pubKey
) external onlyOwnerOrNodeOwner(_nodeOwner) {
function changeAttesterKey(address _nodeOwner, Secp256k1PublicKey calldata _pubKey) external onlyOwner {
_verifyInputSecp256k1PublicKey(_pubKey);
_verifyNodeOwnerExists(_nodeOwner);
(Node storage node, bool deleted) = _getNodeAndDeleteIfRequired(_nodeOwner);
Expand All @@ -265,7 +322,7 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
_verifyAttesterPubKeyDoesNotExist(newHash);
attesterPubKeyHashes[newHash] = true;

_ensureAttesterSnapshot(node);
_snapshotAttesterIfOutdated(node);
node.attesterLatest.pubKey = _pubKey;

emit NodeAttesterKeyChanged(_nodeOwner, _pubKey);
Expand Down Expand Up @@ -293,9 +350,9 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
emit ValidatorsCommitted(validatorsCommit);
}

/// @notice Returns an array of `AttesterAttr` structs representing the current attester committee.
/// @notice Returns an array of `CommitteeAttester` structs representing the current attester committee.
/// @dev Collects active and non-removed attesters based on the latest commit to the committee.
function getAttesterCommittee() public view returns (CommitteeAttester[] memory) {
function getAttesterCommittee() external view returns (CommitteeAttester[] memory) {
uint256 len = nodeOwners.length;
CommitteeAttester[] memory committee = new CommitteeAttester[](len);
uint256 count = 0;
Expand All @@ -318,9 +375,9 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
return committee;
}

/// @notice Returns an array of `ValidatorAttr` structs representing the current attester committee.
/// @notice Returns an array of `CommitteeValidator` structs representing the current attester committee.
/// @dev Collects active and non-removed validators based on the latest commit to the committee.
function getValidatorCommittee() public view returns (CommitteeValidator[] memory) {
function getValidatorCommittee() external view returns (CommitteeValidator[] memory) {
uint256 len = nodeOwners.length;
CommitteeValidator[] memory committee = new CommitteeValidator[](len);
uint256 count = 0;
Expand All @@ -347,7 +404,7 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
return committee;
}

function numNodes() public view returns (uint256) {
function numNodes() external view returns (uint256) {
return nodeOwners.length;
}

Expand Down Expand Up @@ -386,21 +443,21 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
emit NodeDeleted(_nodeOwner);
}

function _ensureAttesterSnapshot(Node storage _node) private {
function _snapshotAttesterIfOutdated(Node storage _node) private {
if (_node.attesterLastUpdateCommit < attestersCommit) {
_node.attesterSnapshot = _node.attesterLatest;
_node.attesterLastUpdateCommit = attestersCommit;
}
}

function _ensureValidatorSnapshot(Node storage _node) private {
function _snapshotValidatorIfOutdated(Node storage _node) private {
if (_node.validatorLastUpdateCommit < validatorsCommit) {
_node.validatorSnapshot = _node.validatorLatest;
_node.validatorLastUpdateCommit = validatorsCommit;
}
}

function _isNodeOwnerExists(address _nodeOwner) private view returns (bool) {
function _doesNodeOwnerExist(address _nodeOwner) private view returns (bool) {
BLS12_381PublicKey storage pubKey = nodes[_nodeOwner].validatorLatest.pubKey;
if (pubKey.a == bytes32(0) && pubKey.b == bytes32(0) && pubKey.c == bytes32(0)) {
return false;
Expand All @@ -409,13 +466,13 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
}

function _verifyNodeOwnerExists(address _nodeOwner) private view {
if (!_isNodeOwnerExists(_nodeOwner)) {
if (!_doesNodeOwnerExist(_nodeOwner)) {
revert NodeOwnerDoesNotExist();
}
}

function _verifyNodeOwnerDoesNotExist(address _nodeOwner) private view {
if (_isNodeOwnerExists(_nodeOwner)) {
if (_doesNodeOwnerExist(_nodeOwner)) {
revert NodeOwnerExists();
}
}
Expand All @@ -438,7 +495,7 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg

function _verifyInputAddress(address _nodeOwner) private pure {
if (_nodeOwner == address(0)) {
revert InvalidInputNodeOwnerAddress();
revert ZeroAddress();
}
}

Expand Down
Loading

0 comments on commit 563056e

Please sign in to comment.