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

Extend threshold types for multisig #139

Closed
ethanfrey opened this issue Nov 13, 2020 · 14 comments · Fixed by #180
Closed

Extend threshold types for multisig #139

ethanfrey opened this issue Nov 13, 2020 · 14 comments · Fixed by #180
Assignees
Labels
multisig Related to a multisig epic
Milestone

Comments

@ethanfrey
Copy link
Member

ethanfrey commented Nov 13, 2020

Builds on #80

Currently the cw3-fixed-multsig only supports AbsoluteCount threshold, and the initial version of cw3-flex-multisig as well. Let us extend cw3-flex-multisig to support all three threshold types defined in cw3.

This should snapshot total_weight at the beginning of the vote, and calculate acceptance criteria based on that. (Note: there may be a subtle difference between the snapshotted member weights - beginning of the block when proposal was created - and the total weight - beginning of the tx when the proposal was created. We can make a follow up issue to see how to resolve that).

These votes (like percentage and quorum) must work properly with significant membership changes during the election.

@ethanfrey ethanfrey added the multisig Related to a multisig epic label Nov 13, 2020
@ethanfrey ethanfrey added this to the v0.4.0 milestone Nov 13, 2020
@ethanfrey ethanfrey self-assigned this Dec 11, 2020
@maurolacy
Copy link
Contributor

Now reviewing.

@maurolacy
Copy link
Contributor

maurolacy commented Dec 15, 2020

When reviewing this I found what I think are some inconsistencies, or maybe a misunderstanding on my part.

Posting here so we can discuss before submitting the review.

When you say:

This should snapshot total_weight at the beginning of the vote, and calculate acceptance criteria based on that. (Note: there may be a subtle difference between the snapshotted member weights - beginning of the block when proposal was created - and the total weight - beginning of the tx when the proposal was created. We can make a follow up issue to see how to resolve that).

That seems contradictory with the comments in the Threshold section. In particular, for AbsolutePercentage:

    /// Declares a percentage of the total weight needed to pass
    /// This implies the percentage is stable, when total_weight changes
    /// eg. at 50.1%, we go from needing 51/100 to needing 101/200

The example in particular, seems to imply that AbsolutePercentage means that the percentage is over the current total weight, not the initial total weight. Numbers are 50.1% of 200 is 100.2, which is 101 with ceiling.

And, looking at the code, you're using in fact the initial (snapshotted) weight.

Probably more important: if the percentage is over the initial weight, then the fairness of the voting scheme is easy to defeat. Just submit a proposal associated to a group with a small number of members (one (the proposer) will suffice), and add members to the group afterwards.

Given that the initial total weight is computed as a sum of the members' weights at proposal creation, any new member or members with enough weight can easily pass the proposal; since new member weights are not considered when computing the "total" weight.

@ethanfrey
Copy link
Member Author

That comment refers not to one proposal but the lifetime of the contract.

Proposal 1 starts with total weight 100. Proposal 2 with total weight 200. In absolute count, they would need the same number of yeses to pass. In absolute percentage, the second one would need more.

Maybe I should clear up the texy

@maurolacy
Copy link
Contributor

The same with the other threshold, i.e ThresholdQuora. The same scheme as above applies, only that now not only with votes, but also with the quorum.

Let's clarify this first, so I can make a useful review.

@ethanfrey
Copy link
Member Author

As to the attack, the last 4 tests thoroughly show that they use the voters weight from the snapshot. (Make a proposal. Change voters, do some votes, ensure the original weights were used)

@ethanfrey
Copy link
Member Author

Every proposal uses snapshots of total weight and individual voter weights (the second is done in the cw4 contract)

@maurolacy
Copy link
Contributor

AH, OK. You're using the snapshotted weights of the members too! That's the root of my confusion: I understood that only the total weight was being snapshotted.

@maurolacy
Copy link
Contributor

Thanks for clarifying... this shoud have been clear after our work in snapshots, but I somehow got confused by the apparently contradictory comments.

@ethanfrey
Copy link
Member Author

I did the whole weight snapshotting a few prs ago. The point was not how voting worked, but that the number of votes needed to pass can change automatically as the group changes. Unlike the first, simple version (only really suited for relatively static groups)

@maurolacy
Copy link
Contributor

That comment refers not to one proposal but the lifetime of the contract.

Proposal 1 starts with total weight 100. Proposal 2 with total weight 200. In absolute count, they would need the same number of yeses to pass. In absolute percentage, the second one would need more.

Maybe I should clear up the texy

Yes, that was really confusing to me.

@ethanfrey
Copy link
Member Author

Happy for any proposals of better documentation. I have the picture in my head and often forget to write it all down.

@maurolacy
Copy link
Contributor

Sure. That will be part of the review.

@maurolacy
Copy link
Contributor

I'm finding it exceedingly difficult to know what is the correct impl in each case.

We'll have to document this better, or after a couple of weeks nobody will understand how it works in detail.

@ethanfrey
Copy link
Member Author

Note for future readers. We did do quite a bit of documentation improvement before merging. See #188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multisig Related to a multisig epic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants