Skip to content

Commit

Permalink
Merge branch 'consensus-fix-review' into bf-N06
Browse files Browse the repository at this point in the history
  • Loading branch information
@brunoffranca
brunoffranca committed Oct 14, 2024
2 parents 41c8e99 + 0963893 commit 4e63a5d
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 28 deletions.
75 changes: 49 additions & 26 deletions l2-contracts/contracts/ConsensusRegistry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,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,6 +42,11 @@ 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();
Expand Down Expand Up @@ -69,6 +80,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 Down Expand Up @@ -136,7 +153,7 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
return;
}

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

emit AttesterDeactivated(_nodeOwner);
Expand All @@ -153,7 +170,7 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
return;
}

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

emit ValidatorDeactivated(_nodeOwner);
Expand All @@ -170,7 +187,7 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
return;
}

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

emit AttesterActivated(_nodeOwner);
Expand All @@ -187,7 +204,7 @@ contract ConsensusRegistry is IConsensusRegistry, Initializable, Ownable2StepUpg
return;
}

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

emit ValidatorActivated(_nodeOwner);
Expand All @@ -204,9 +221,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 @@ -216,15 +233,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 @@ -234,22 +254,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 @@ -258,7 +281,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 @@ -272,22 +295,22 @@ 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) {
) external onlyOwner {
_verifyInputSecp256k1PublicKey(_pubKey);
_verifyNodeOwnerExists(_nodeOwner);
(Node storage node, bool deleted) = _getNodeAndDeleteIfRequired(_nodeOwner);
Expand All @@ -301,7 +324,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 @@ -329,9 +352,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 @@ -354,9 +377,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 @@ -383,7 +406,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 @@ -422,21 +445,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 @@ -445,13 +468,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 Down
8 changes: 6 additions & 2 deletions l2-contracts/contracts/interfaces/IConsensusRegistry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ pragma solidity ^0.8.20;
interface IConsensusRegistry {
/// @dev Represents a consensus node.
/// @param attesterLastUpdateCommit The latest `attestersCommit` where the node's attester attributes were updated.
/// @param validatorLastUpdateCommit The latest `validatorsCommit` where the node's validator attributes were updated.
/// @param nodeOwnerIdx Index of the node owner within the array of node owners.
/// @param attesterLatest Attester attributes to read if `node.attesterLastUpdateCommit` < `attestersCommit`.
/// @param attesterSnapshot Attester attributes to read if `node.attesterLastUpdateCommit` == `attestersCommit`.
/// @param validatorLastUpdateCommit The latest `validatorsCommit` where the node's validator attributes were updated.
/// @param validatorLatest Validator attributes to read if `node.validatorLastUpdateCommit` < `validatorsCommit`.
/// @param validatorSnapshot Validator attributes to read if `node.validatorLastUpdateCommit` == `validatorsCommit`.
/// @param nodeOwnerIdx Index of the node owner within the array of node owners.
struct Node {
uint32 attesterLastUpdateCommit;
uint32 validatorLastUpdateCommit;
Expand Down Expand Up @@ -104,6 +104,8 @@ interface IConsensusRegistry {
error InvalidInputBLS12_381PublicKey();
error InvalidInputBLS12_381Signature();
error InvalidInputSecp256k1PublicKey();
error ZeroAttesterWeight();
error ZeroValidatorWeight();

event NodeAdded(
address indexed nodeOwner,
Expand Down Expand Up @@ -168,4 +170,6 @@ interface IConsensusRegistry {
function getAttesterCommittee() external view returns (CommitteeAttester[] memory);

function getValidatorCommittee() external view returns (CommitteeValidator[] memory);

function numNodes() external view returns (uint256);
}

0 comments on commit 4e63a5d

Please sign in to comment.