Skip to content
This repository has been archived by the owner on Mar 12, 2024. It is now read-only.

optimize quorum #222

Closed
wants to merge 2 commits into from
Closed

optimize quorum #222

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ab7574
feat(contracts): optimize quorum
ZzzzHui Aug 21, 2023
c41df42
fix(contracts): quick fix
ZzzzHui Aug 22, 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
80 changes: 42 additions & 38 deletions onchain/rollups/contracts/consensus/quorum/Quorum.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,32 +3,44 @@

pragma solidity ^0.8.8;

import {AccessControlEnumerable} from "@openzeppelin/contracts/access/AccessControlEnumerable.sol";
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
import {PaymentSplitter} from "@openzeppelin/contracts/finance/PaymentSplitter.sol";

guidanoli marked this conversation as resolved.
Show resolved Hide resolved
import {AbstractConsensus} from "../AbstractConsensus.sol";
import {IConsensus} from "../IConsensus.sol";
import {IHistory} from "../../history/IHistory.sol";

/// @title Quorum consensus
/// @notice A consensus model controlled by a small set of addresses, the validators.
/// @dev This contract uses several OpenZeppelin contracts:
/// `AccessControlEnumerable`, `EnumerableSet`, and `PaymentSplitter`.
/// For more information on those, please consult OpenZeppelin's official documentation.
Comment on lines -15 to -17
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's good to mention that we're using OpenZeppelin's contracts, because they are trustworthy.

contract Quorum is AbstractConsensus, AccessControlEnumerable, PaymentSplitter {
using EnumerableSet for EnumerableSet.AddressSet;

/// @notice The validator role.
/// @dev Only validators can submit claims.
bytes32 public constant VALIDATOR_ROLE = keccak256("VALIDATOR_ROLE");
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 use AccessControl, this variable would still be in use.


/// In this version, the validator set is immutable.
/// @dev This contract uses OpenZeppelin `PaymentSplitter`.
/// For more information on `PaymentSplitter`, please consult OpenZeppelin's official documentation.
contract Quorum is AbstractConsensus, PaymentSplitter {
/// @notice The history contract.
/// @dev See the `getHistory` function.
IHistory internal immutable history;

/// @notice For each claim, the set of validators that agree
/// that it should be submitted to the history contract.
mapping(bytes => EnumerableSet.AddressSet) internal yeas;
// Quorum members
// Map an address to true if it's a validator
mapping(address => bool) public validators;
Copy link
Contributor

@guidanoli guidanoli Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is very similar to how AccessControl is implemented. Maybe we should use it instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The main idea is to reduce AccessControlEnumerable to only something like AccessControl. But the reason why I didn't use AccessControl is because AccessControl is most useful when there are different roles, while we only have one role, validator. So it doesn't make sense to me to do 1 extra storage read (through this variable) each time when we want to read the validators.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't imply an extra storage read.
It just adds a new keccak in the calculation of the storage slot where the value resides.
Either way, I think it is still worth it to use AccessControl because it has been tested thoroughly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right there's no extra storage read. Sorry about my mistake.
But you see, all we need is a mapping(address => bool), set some addresses map to true in the constructor, plus we used it for the modifier. This is simple enough that it doesn't make much of a difference using existing codes or not, since it's only 2 lines of codes plus a modifier.

Another advantage is that it can be more flexible if we want to optimize further. If the current version is ok, I'm creating another PR based on the current version for the last step of optimization. We can decide on this after the other PR, ok? If the other PR is not gonna be merged, then we can use AccessControl if you think it's better.

Copy link
Contributor

Choose a reason for hiding this comment

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

But you see, all we need is a mapping(address => bool), set some addresses map to true in the constructor, plus we used it for the modifier.

You're right, but this logic has already been implemented. There is no need to re-implement it. Besides, for anyone reading this code, they will need to understand what you're trying to achieve with this mapping, the modifier, etc. But if you use AccessControl (which many smart contract developers are already familiar with), the code will be much clearer, and, most importantly, more trustworthy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another advantage is that it can be more flexible if we want to optimize further.

That is true. But we need to find the balance between performance and simplicity.

uint256 public immutable quorumSize;

// Quorum votes
struct Voted {
uint256 count;
// Map an address to true if it has voted yea
mapping(address => bool) voted;
}
// Map a claim to struct Voted
mapping(bytes => Voted) internal yeas;

/// @notice Raised if not a validator
error OnlyValidator();
modifier onlyValidator() {
if (!validators[msg.sender]) {
revert OnlyValidator();
}
_;
}

/// @notice Construct a Quorum consensus
/// @param _validators the list of validators
Expand All @@ -39,17 +51,15 @@ contract Quorum is AbstractConsensus, AccessControlEnumerable, PaymentSplitter {
uint256[] memory _shares,
IHistory _history
) PaymentSplitter(_validators, _shares) {
// Iterate through the array of validators,
// and grant to each the validator role.
// Add the array of validators into the quorum
for (uint256 i; i < _validators.length; ++i) {
grantRole(VALIDATOR_ROLE, _validators[i]);
validators[_validators[i]] = true;
guidanoli marked this conversation as resolved.
Show resolved Hide resolved
}

// Set history.
quorumSize = _validators.length;
history = _history;
}

/// @notice Submits a claim for voting.
/// @notice Vote for a claim to be submitted.
/// If this is the claim that reaches the majority, then
/// the claim is submitted to the history contract.
/// The encoding of `_claimData` might vary depending on the
Expand All @@ -58,26 +68,16 @@ contract Quorum is AbstractConsensus, AccessControlEnumerable, PaymentSplitter {
/// @dev Can only be called by a validator,
/// and the `Quorum` contract must have ownership over
/// its current history contract.
function submitClaim(
bytes calldata _claimData
) external onlyRole(VALIDATOR_ROLE) {
// Get the set of validators in favour of the claim
EnumerableSet.AddressSet storage claimYeas = yeas[_claimData];
function submitClaim(bytes calldata _claimData) external onlyValidator {
Voted storage claimYeas = yeas[_claimData];

// Add the message sender to such set.
// If the `add` function returns `true`,
// then the message sender was not in the set.
if (claimYeas.add(msg.sender)) {
// Get the number of validators in favour of the claim,
// taking into account the message sender as well.
uint256 numOfVotesInFavour = claimYeas.length();
// If the msg.sender hasn't submitted the same claim before
if (!claimYeas.voted[msg.sender]) {
claimYeas.voted[msg.sender] = true;

// Get the number of validators in the quorum.
uint256 quorumSize = getRoleMemberCount(VALIDATOR_ROLE);

// If this claim has now just over half of the quorum's approval,
// If this claim has now just over half of the quorum's votes,
// then we can submit it to the history contract.
if (numOfVotesInFavour == 1 + quorumSize / 2) {
if (++claimYeas.count == 1 + quorumSize / 2) {
history.submitClaim(_claimData);
}
}
Expand All @@ -89,6 +89,10 @@ contract Quorum is AbstractConsensus, AccessControlEnumerable, PaymentSplitter {
return history;
}

/// @notice Get a claim from the current history.
/// The encoding of `_proofContext` might vary depending on the
/// implementation of the current history contract.
/// @inheritdoc IConsensus
function getClaim(
address _dapp,
bytes calldata _proofContext
Expand Down