Quorum consensus

[guidanoli]

Closes cartesi/rollups-contracts#48

[guidanoli]

Here, I'm using three really handy OpenZeppelin contracts:

• AccessControlEnumerable
• EnumerableSet
• PaymentSplitter (suggested by @pedroargento)

[pedroargento]

I know that we have a limited set of validators, but I'm still not comfortable pushing transfers like this in a for loop.

Maybe we can use the payment splitter from Open Zeppelin.

[guidanoli]

True, I will take a look into that. Thanks!

[pedroargento]

I think this should be a parameter instead of a majority.

Also, should not be x/2 + 1? If there are three validators and we want the majority it results in a 1 of 3.

What happens when the quorum was reached and another validator publish the claim? Is there a risk of trying to add multiple times to history?

[tuler]

if quorumSize is 3 you need 2, division is integer, right? And it's >, not >=

[pedroargento]

That is true. I misread as >=

[pedroargento]

Actually, if we change to == we solve the problem of including multiple times to the history. It would only add to History when the second validator (in this 2 of 3 example) post and ignore future validators.

[guidanoli]

The current implementation of History prevents its owner from claiming the same data twice. :-)

[guidanoli]

But I see what you mean. Maybe we should change this to numOfVotesInFavour == 1 + quorumSize / 2?

[tuler]

I think the tricky part is that there is no coordination between the multiple nodes. They will submit claims at the exact same time. What would happen in that case? Imagine the "same" transaction, coming from multiple nodes, in the same block. Would it work?

[guidanoli]

Rebased and applied @pedroargento suggestion to use PaymentSplitter.

[guidanoli]

Applied @pedroargento's suggestion of using == instead of > for checking majority, and avoid submitting the same claim to the history twice.

[pedroargento]

Doesn't it make sense to make the number of necessary votes a parameter? Maybe one DApp wants to have a super majority or wants to accept only 2 signatures even with a lot of possible validators.

[guidanoli]

Doesn't it make sense to make the number of necessary votes a parameter? Maybe one DApp wants to have a super majority or wants to accept only 2 signatures even with a lot of possible validators.

If we see a demand for such fine-grained configuration, sure.
For now, a simple majority scheme should suffice for an MVP.

[guidanoli]

I think the tricky part is that there is no coordination between the multiple nodes. They will submit claims at the exact same time. What would happen in that case? Imagine the "same" transaction, coming from multiple nodes, in the same block. Would it work?

Great point. What will happen now that I've changed > to ==, is that it will only submit a claim once. Of course, it needs testing, but it should work (in my head).

[ZzzzHui]

By using PaymentSplitter, the set of validators and shares are fixed at the constructor and cannot be changed later on. Do we wanna assume the quorum to be fixed?

[guidanoli]

By using PaymentSplitter, the set of validators and shares are fixed at the constructor and cannot be changed later on. Do we wanna assume the quorum to be fixed?

Yes, the PaymentSplitter contract assumes the set of shares is constant.

[ZzzzHui]

I think we can optimize the codes a bit:

1. Currently, we call AccessControlEnumerable.grantRole in the constructor to add members into the quorum. AccessControlEnumerable is a combination of AccessControl and EnumerableSet. Since we don't have a dynamic quorum, we don't actually need to maintain a EnumerableSet. With the current codes, the only time we call this EnumerableSet is to check the size of the quorum, which can simply be done with a uint256. As for access control, we can simplify to a mapping(address => bool) members to indicate whether it's a member or not. The complete set of members is already shown in the constructor parameter, and we could emit an event if needed.
2. The 2nd place that we use EnumerableSet is to record the set of members that voted yeas. Similarly, we don't actually need to maintain the set, we only need to record who has already voted yea. So instead of using this struct from EnumerableSet

struct Set {
        bytes32[] _values;
        mapping(bytes32 => uint256) _indexes;
    }

, we can simplify it to

struct Yeas {
        uint256 count;
        mapping(address => bool) voted;
}

Furthermore, if we want to optimize further. We can use a uint256 and split it into 2 parts. | who_has_voted(248 bit) | count(8 bit) | This way we can record up to 248 members, including who has voted and the current count of how many has voted. If there could be more than 248 members, then we could make it an array(mapping) of uint256s. P.S., in part 1, we mentioned using mapping(address => bool) members to indicate whether it's a member. Now it becomes mapping(address => uint256) members to indicate a member's bit position (>0) in the uint256 struct, with 0 indicating not a member.
The drawback is the code becomes more complex for sure. Let me know what you think and we can work on this together :)

[xdaniortega]

Good code work!

Here are my thoughts:
We trying to define here is a multi-signature consensus model so on the definition of the members of the signature validator is a bit inaccurate as members (or just a shared/distributed key) may not be validating the the claim and just signing. We could rename it to MEMBER_ROLE or SIGNER_ROLE.

I also remember that someone mentioned that Quorum can bring confusion to what this consensus model is really doing, and maybe the discussion can be retaken here. We can consider simplifying the name of this model to multisig or similar.

[guidanoli]

@xdaniortega Thanks for the feedback!
And let us bring the discussion to the original issue, so that everyone can see and participate. :-)

[guidanoli]

This PR has been migrated to cartesi/rollups-contracts#49.