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

Add Governance Documentation #80

Merged
merged 7 commits into from Feb 3, 2018
Merged

Add Governance Documentation #80

merged 7 commits into from Feb 3, 2018

Conversation

gamarin2
Copy link
Contributor

@gamarin2 gamarin2 commented Jan 6, 2018

Markdown version of Governance MVP doc

  • No new design choice has been introduced. However, I have refactored much of the document to make it clearer and more coherent
  • I have added structs and pseudocode
    • If pseudocode is not useful, I can remove it
    • Please review types used in structs, as I am not very familiar with best practices
    • If the whole implementation section is unneeded in this doc, I can remove it
  • There are notes throughout the doc. They often indicate a point that needs clarification.

gamarin2 added 3 commits January 6, 2018 21:16
- Markdown version of Governance MVP doc
- No new design choice has been introduced. However, I have refactored much of the document to make it clearer and more coherent
- I have added structs and pseudocode
    - If pseudocode is not useful, I can remove it
    - Please review types used in structs, as I am not very familiar with best practices
    - If the whole implementation section is unneeded in this doc, I can remove it
- There are notes throughout the doc. They often indicate a point that needs clarification.
@jaekwon jaekwon self-requested a review January 10, 2018 18:05
@@ -0,0 +1,619 @@
# Governance documentation

*Disclaimer: This is work in progress. Mechanisms are susceptible to change*

Choose a reason for hiding this comment

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

Last dot missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok


### Proposal filter (minimum deposit)

To prevent spam, proposals must be submitted with a deposit in Atoms. Voting period will not start as long as the proposal's deposit is inferior to the minimum deposit parameter `MinDeposit`.

Choose a reason for hiding this comment

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

is smaller than the minimum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok


To prevent spam, proposals must be submitted with a deposit in Atoms. Voting period will not start as long as the proposal's deposit is inferior to the minimum deposit parameter `MinDeposit`.

When a proposal is submitted, it has to be acompagnied by a deposit that must be strictly positive but that can be inferior to `MinDeposit`. Indeed, the submitter need not pay for the entire deposit on its own. If a proposal's deposit is strictly inferior to `MinDeposit`, other Atom holders can increase the proposal's deposit by sending a `TxDeposit` transaction. Once the proposals's deposit reaches `minDeposit`, it enters voting period.

Choose a reason for hiding this comment

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

accompagnied

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok


There are two instances where Atom holders that deposited can claim back their deposit:
- If the proposal is accepted
- If the proposal's deposit does not reach `MinDeposit` for a period longer than `mMxDepositPeriod` (initial value: 2 months). Then the proposal is considered closed and nobody can deposit on it anymore.

Choose a reason for hiding this comment

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

Do we really want to allow people to get their atoms back for a failed deposit? I think it might be simpler to just burn those. That would get rid of an extra parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also and especially for people who deposited on accepted proposals. The idea is to incentivize people to deposit on good proposals. I'm 100% in favour of keeping it at least if the proposal is accepted (other platforms such as Dfinity do it also).

As for proposals that failed to reach MinDeposit, it's not that complicate once you have TxClaimDeposit. The idea is to not disincentivize people to deposit. They might get scared to deposit if they think there's a chance deposit will never reach MinDeposit. I think it's always good to advertise that there is a way to get your money back.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm with adrian here, I'd like to burden the people who are pushing for proposals a bit... I'm okay if not that many proposals get put forth, less wasted time reading bad proposals

- If the proposal is accepted
- If the proposal's deposit does not reach `MinDeposit` for a period longer than `mMxDepositPeriod` (initial value: 2 months). Then the proposal is considered closed and nobody can deposit on it anymore.

In such instances, Atom holders that deposited can send a `TxClaimDeposit` transaction to retrieve their share of the deposit.

Choose a reason for hiding this comment

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

And would get rid of this tx type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not correct, you would still need this transaction type to claim deposits on accepted proposals


### Participants

*Participants* are users that have the right to vote. On the Cosmos Hub, participants are bonded Atom holders. Unbonded Atom holders and other users do not get the right to participate in governance. However, they can submit and deposit on proposals.

Choose a reason for hiding this comment

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

Can you deposit bonded atoms on a proposal? The answer should probably be no.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No


Initially, the threshold is set at 50% with a possibility to veto if more than 1/3rd of votes (excluding `Abstain` votes) are `NoWithVeto` votes. This means that proposals are accepted is the ratio of `Yes` votes to `No` votes at the end of the voting period is superior to 50% and if the number of `NoWithVeto` votes is inferior to 1/3rd of total votes (excluding `Abstain`).

`Urgent` proposals also work with the aforementioned threshold, except there is another condition that can accelerate the acceptance of the proposal. Namely, if the ratio of `Yes` votes to `InitTotalVotingPower` exceeds 2/3, `UrgentProposal` will be immediately accepted, even if the `Voting period` is not finished. `InitTotalVotingPower` is the total voting power of all bonded Atom holders at the moment when the vote opens.

Choose a reason for hiding this comment

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

I feel like this should be applicable to all proposal. Once a proposal has >2/3 it will always be accepted so we might just end the vote here.

Copy link
Contributor Author

@gamarin2 gamarin2 Jan 16, 2018

Choose a reason for hiding this comment

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

No it won't be because of the way we implement it. If validators vote very fast there could be >2/3 yes vote, because they carry the vote of their delegators. When delegators, who will likely be slower to connect and vote, cast their tx, they could override the vote of validators and change the outcome.

It is true that if >2/3rd of validators collude, they can censor votes. However, assuming that validators are honest, it is possible that first validators will vote on an option and then delegators on the other. We don't want to prevent that scenario from happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh interesting... okay yeah I'm with gautier on this one... HOWEVER I think we should eliminate the urgent proposal type altogether I've outlined previously

Choose a reason for hiding this comment

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

I agree with removing the urgent proposal type. It adds confusion and since the rules are then hardwired in the blockchain, it invites people to try to game it.

Leaving one simple and safe on-chain voting mechanism is best. If there are emergencies, they can be handled off-chain, as is currently the case with almost every blockchain.

Choose a reason for hiding this comment

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

Note that the "one process" rule is the same as urgent.
If you just redefine the "one process" to always act like "regular" (no early end), then there is no problem.
If you really need to go fast and break things, do it off-chain.

Copy link
Contributor

Choose a reason for hiding this comment

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

yup totally in agreement - let's rip it out!

If a delegator does not vote, it will inherit its validator vote.

- If the delegator votes before its validator, it will not inherit from the validator's vote.
- If the delegator votes after its validaotor, it will override its validator vote with its own vote. If the proposal is a `Urgent` proposal, it is possible that the vote will close before delegators have a chance to react and override their validator's vote. This is not a problem, as `Urgent` proposals require more than 2/3rd of the total voting power to pass before the end of the voting period. If more than 2/3rd of validators collude, they can censor the votes of delegators anyway. This is an inherent limit of byzantine fault tolerant systems.

Choose a reason for hiding this comment

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

Let's take out the last sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok


Validators are required to vote on all proposals to ensure that results have legitimacy. Voting is part of validators' directives and failure to do it will result in a penalty.

If a validator’s address is not in the list of addresses that voted on a proposal and if the vote is closed (i.e. `MinDeposit` was reached and `Voting period` is over), then any user of the Hub can submit a `TxGovernancePenalty` that will result in a partial slash `GovernancePenalty` of the validator's stake. The user that submitted the `TxGovernancePenalty` transaction will receive `GovernanceReward` from the `GovernancePenalty`.

Choose a reason for hiding this comment

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

Why have a user submit a transaction. Just penalise the validator automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feels computationally expensive to me. I have Ethereum reflexes, to me state mutation should always (or almost) be triggered by users sending a tx and paying the associated fee.
I'm not against automatic punishment but computational cost needs to be evaluated.

Choose a reason for hiding this comment

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

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 iterating through the validator set for each governance proposal is not a problem... Iterating over the validator set is only really a problem if it needs to happen every block, given that proposals are infrequent relatively speaking we should be OK to iterate through the validator set.

So in line with adrians thinking I'd edit to not include a Tx for punishment here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm ok but is it possible to trigger the iteration exactly VotingPeriod blocks after the proposal reaches MinDeposit. Like when MinDeposit is reached the app will perform the iteration in BeginBlock 2 weeks after, without iterating over all proposals?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible, when adding a deposit you check is MinDeposit is reached, if it is, then you add the proposal ID to a ProposalProcessingQueue where the oldest element is checked each round... this is a similar to unbonding for the staking module, we can reuse the queue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll modify the spec accordingly then


### Governance key and governance address

Validators can make use of an additional slot where they can designate a `Governance PubKey`. By default, a validator's `Governance PubKey` will be the same as its main PubKey. Validators can change this `Governance PubKey` by sending a `Change Governance PubKey` transaction signed by their main `Consensus PubKey`. From there, they will be able to sign vote using the `Governance PrivKey` associated with their `Governance PubKey`. The `Governance PubKey` can be changed at any moment.

Choose a reason for hiding this comment

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

We should enforce for these two keys to be different. Otherwise the default is a security weakness. It's like leaving password as admin on your router.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm can you give an explicit example where this would cause a security issue?

Choose a reason for hiding this comment

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

In the default case. The validation key now needs to be available to also sign votes. This means it is easier duplicated or the validator needs to have hot access to the HSM.

Most likely the key will be duplicated. A validator should never have the validation key in a hot HSM to sign proposals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok then why not force the validator to specify a Governance PubKey in its TxBond, meaning there is no default choice? If the validator then inputs the same PubKey as his Consensus PubKey, it's its problem...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I can build this logic into the staking module... I guess there is some overlap, there should only really be one candidate object but it should support both staking and governance

Choose a reason for hiding this comment

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

Probably it should be "signed by their main Consensus PrivKey instead of Consensus PubKey.

@adrianbrink
Copy link

This looks great. I've left a couple of comments. I'll check everything after Implementation in the next round.

gamarin2 added 2 commits January 16, 2018 21:23
Some comments of Adrian should be discussed before changing specs. These include:

- Getting rid of `TxClaimDeposit`
- No differentiation between `SoftwareUpgradeProposal` and `PlainTextProposal`
- Always accept if >2/3 vote `Yes` (which means no `Urgent` proposals anymore)
- Penalize validator automatically
- Enforce `MainKey` and `GovernanceKey` are always different
### Proposal types

In the initial version of the governance module, there are two types of proposal:
- `PlainTextProposal`. All the proposals that do not involve a modification of the source code go under this type. For example, an opinion poll would use a proposal of type `PlainTextProposal`
Copy link
Contributor

Choose a reason for hiding this comment

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

What is an opinion poll in this context? I can see proposals which outline the intention to upgrade the protocol in a certain way whichout actually providing the code... is this what you mean? we should go into greater detail on this idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see proposals which outline the intention to upgrade the protocol in a certain way whichout actually providing the code...

Yes good example. Actually PlainTextProposals encompass everything that is not an immediate proposition to update the protocol. Another example could be "Scaling Tendermint: roadmap A or roadmap B?" (like Casper FFG vs Casper CBC)

- `Regular`
- `Urgent`

These two categories are strictly identical except that `Urgent` proposals can be accepted faster if a certain condition is met. For more information, see [Threshold](#threshold) section.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need the urgent message type... If it's urgent the validators can just push it through on their own accord without governance and then create a vote post-mortum, this non-governance mechanism effectively covers urgent changes @jaekwon What do you think about urgent proposals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I think it's good to have a public and open process, so that decisions have legitimacy. As far as I remember jae thought the same. It's not that complicated to implement.

Copy link
Contributor

@jaekwon jaekwon Jan 19, 2018

Choose a reason for hiding this comment

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

I think flagging a proposal as "urgent" is fine, because it can wake up validators, red flashing lights and sirens, paging on-call devops etc. If it wasn't actually urgent, then we'll maybe figure out some punishment for it later. The whitepaper specifies or hints at the ability to say "burn that deposit, this proposal is junk", and that would also work for "burn that deposit, this wasn't urgent".

Otherwise, proposals should work the same whether urgent or not. It's not considered having "passed" unless it receives >50% of the votes, accounting for delegation power and any delegator overrides (manual vote to override the default vote of the validator(s)). If the delegators are too late in showing up for their vote, then perhaps it was (actually) an urgent matter, and it passed despite the lack of delegators' support.

If we require a waiting period before a proposal is decided to have "passed" in order to let the delegators voice their opinions, then we lose the opportunity to respond to urgent matters. So there must be an implicit contract that for urgent matters (regardless of the "urgent" flag), the validators have to make a best-case judgement call. Perhaps, how to resolve the delegator-lag issue should be specified in the proposal, so that validators can judge for themselves what they're actually voting for. Until we see some options it seems to me that it's better to leave this open for now.

In any case, whether a proposal is marked "urgent" or not should be cosmetic, and have no effect on the process of actually voting or deciding.

For example, a Byzantine proposer can propose the vote for a really urgent matter, but not flag the proposal as "urgent". Now, should voters vote yes for that, or vote yes for the alternative (newer) proposal that is marked "urgent"? The solution to this conundrum/wrinkle-in-logic is to ignore the flag. But it could still be useful to wake up devops in the middle of the night.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the existence of an "urgent" flag may confuse participants about the true urgency of proposals, maybe it's better to leave it out, and have a convention of including the "URGENT" text in the proposal (or in the future, comments). I dunno.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm yeah interesting about the ambiguity of use of the urgent flag... Maybe the urgentness of a proposal could be determined not by the person who submits the proposal but by the people that are voting for the proposal. If people begin to consitently vote a propsal as urgent then it would automatically take on the urgent type... I think this could avoid some of the problems outlined... but it's also a bit more confusing..

I guess my comment is we still need an out of band process for validators to change sensistive issues as well such as the example with the Monero upgrade in the past... this can be dealt with seperately however, and is maybe an edge case outside of the standard "urgent" proposal being outlined... but then again maybe not? I feel like the majority "urgent" proposals will be security patches and should be sensitive as to not alert potential malicious actors who could take advantage of a bug before it's patched... am I wrong? what other types of urgent proposals can you guys think of which would really merit not going out of protocol and not waiting two weeks??? @jaekwon @gamarin2

Copy link
Contributor

Choose a reason for hiding this comment

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

@gamarin2 You're missing a point here, under two processes there is still an issue with delegators BEING at risk of large validators censoring them if the large validator submits an urgent proposal... I proposed a solution which addresses this under a one process:

SO HERE IS THE SOLUTION: Whenever you are voting something as "urgent" it's only your personal stake which counts towards making the proposal urgent, so if the network has generally low-self bond ratios for validators a proposal could not simply be pushed through by the validators because it's only their small stake which would count towards calling the proposal urgent... the network would then need to wait for some threshold of the delegators to agree that the proposal is urgent before it would take off before the end of the voting period

But yes there is some inherent tradeoff between voting time and proposal urgency, I think that It shouldn't be a problem for the network to coordinate quickly under and urgent proposal and push it to through with an urgent vote using the above mechanism. OR if something is super extremely tremendously important for the decision to push through out of band

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gamarin2 You're missing a point here, under two processes there is still an issue with delegators BEING at risk of large validators sensoring them if the large validator submits an urgent proposal...

No, please re-read my previous post, I've considered this point, cf

OR re-submit the proposal as urgent if they wanted to decide without letting delegators participate

We can't do anything about that. However, two processes allow us to have more clarity about what's going on. If there are two processes, it will be clear to everyone that someone re-submitted a regular proposal as urgent and, if it is accepted as urgent but not as regular, then we clearly know there is a divergence between validators and delegators. Then, delegators can identify which validators are trying to bypass them (by voting Yes on the urgent proposal) and move away from them. Again, it's a question of clarity.

And if we have inheritance and validator's punishment, it's because we want validators to be active and step up when needed. So it wouldn't make sense to me to deprive them from their delegated voting power when proposals are urgent.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gamarin2 are you referring to a comment like this?

I still think the two processes option is the better because it's more clear and actually less ambiguous. If they don't deem the proposal urgent, validators could vote early without the fear of shutting down the vote and making their delegators unhappy. Likewise, delegators could vote late without the fear of being too late because some validator with sufficient voting power shut down the vote for whatever personal motive.

I agree with the logic, but I still think it's confusing as heck and suspect that it may be difficult to uptake. Two processes has less clarity, think about being a delegator and seeing two identical proposals one marked as urgent and one as not urgent, totally bonkers - I don't think most people will even know how to act under this situation, it's non-intuitive.

If we do something good with the UI or have explicit proposals which are "non-urgent" and their goal is to overturn an existing "urgent" proposal which has just gone through, then yes it would achieve the same effect - I still see this as a poor choice however...

why can't the validators just go out of band if a proposal is so so urgent that they can't wait for some basic loose coordination of the network (delegators and validators) to vote that YES a proposal is in fact urgent and should be passed in less than 2 weeks... This is probably very rare situations anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the logic, but I still think it's confusing as heck and suspect that it may be difficult to uptake. Two processes has less clarity, think about being a delegator and seeing two identical proposals one marked as urgent and one as not urgent, totally bonkers - I don't think most people will even know how to act under this situation, it's non-intuitive.

This problem exists even if we have only one process. Someone can submit the same proposal twice. If we have two processes and what you say happens, then I think it's a good way of finding out if validators and delegators converge in opinion, which is the only stable state of the network.

why can't the validators just go out of band if a proposal is so so urgent that they can't wait for some basic loose coordination of the network (delegators and validators) to vote that YES a proposal is in fact urgent and should be passed in less than 2 weeks... This is probably very rare situations anyways.

I don't think we can cover every situation. We are just trying to find the best way to proceed for, say, 90% of proposals. For example, if there is a bug that affects the governance code itself, then validators will have no choice but to coordinate off-chain. But these are extreme case. I believe many proposals will be just fine going through the regular process and full voting period. For these proposals it's good that validators and delegators can vote whenever they want without fearing timing issues and power imbalance.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gamarin2: This problem exists even if we have only one process. Someone can submit the same proposal twice. If we have two processes and what you say happens, then I think it's a good way of finding out if validators and delegators converge in opinion, which is the only stable state of the network.

If there is only 1 process, then there's less need for duplicate proposals. The beauty of the 1 process system is that whether a proposal is urgent is decided by the voters themselves.

If a delegator does not vote, it will inherit its validator vote.

- If the delegator votes before its validator, it will not inherit from the validator's vote.
- If the delegator votes after its validaotor, it will override its validator vote with its own vote. If the proposal is a `Urgent` proposal, it is possible that the vote will close before delegators have a chance to react and override their validator's vote. This is not a problem, as `Urgent` proposals require more than 2/3rd of the total voting power to pass before the end of the voting period. If more than 2/3rd of validators collude, they can censor the votes of delegators anyway.
Copy link
Contributor

Choose a reason for hiding this comment

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

This section will change if we remove urgent vote type


### Switch

Once a block contains more than 2/3rd *precommits* where a common `SoftwareUpgradeProposal` is signaled, all the nodes (including validator nodes, non-validating full nodes and light-nodes) are expected to switch to the new version of the software.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious if it should be a bit higher than 2/3rd.... consider:

  • if it's an incompatible hard fork and exactly 2/3rds are signalling
  • all non signalling validators will be submitting invalid responses
  • if a single validator goes offline from the upgraded subset, the entire hub will halt

Why don't we do something higher than 2/3 say 3/4

thoughts?

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 makes sense to me. Should be just a bit higher then, not too much. Waiting for other people to weigh in before modifying the spec @jaekwon @adrianbrink @sunnya97

Choose a reason for hiding this comment

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

Yes, I think at least 3/4 should be ready.
I mean, bitcoin was aiming for 90% for segwit.

Once a vote passes and some validators refuse to upgrade, in an attempt to veto with eg. 26%, people can re-delegate to ensure that the validators who already upgraded have at least 3/4 power... this upgrade process err on safety rather than speed.


Once a block contains more than 2/3rd *precommits* where a common `SoftwareUpgradeProposal` is signaled, all the nodes (including validator nodes, non-validating full nodes and light-nodes) are expected to switch to the new version of the software.

*Note: Not clear how the flip is handled programatically*
Copy link
Contributor

Choose a reason for hiding this comment

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

I also think we should maybe introduce a punishment for validators who do not switch within a certain period of time once governance has accepted.... this will help ease the above scenario too!

Copy link
Contributor Author

@gamarin2 gamarin2 Jan 19, 2018

Choose a reason for hiding this comment

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

They're already punished for being offline, and will eventually get unbonded if they don't react

Copy link
Contributor

Choose a reason for hiding this comment

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

But maybe they don't go offline if they don't upgrade, they're just always be submitting invalid blocks or voting against an upgraded proposers blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But more than 2/3rd validators upgraded so the chain won't halt. And the one who did not upgraded will fail to propose and/or have their precommit missing from the blocks that get committed, and will thus be punished

Copy link
Contributor

Choose a reason for hiding this comment

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

I am under the impression that they could still propose an invalid block if they haven't upgraded their protocol... I'm not sure there is an explicit way to prevent this here (maybe?) @ebuchman

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this block will not be accepted because more than 2/3rd validators think it's invalid. So they will not propose valid blocks until they upgrade, and their precommit will not be inlcuded until they upgrade -> they will be punished

Copy link

@milosevic milosevic Jan 30, 2018

Choose a reason for hiding this comment

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

If a block contains 2/3rd precommits for a software upgrade, then the next proposal should be with the new software version, i.e., the proposal is considered invalid if it does not have the new version. This require having version as an additional field in the Proposal message, but we should probably have this for various reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened an issue in the SDK for this, probably needs to be a solution across SDK/Tendermint cosmos/cosmos-sdk#403


```Go
type Procedure struct {
VotingPeriod uint64 // Length of the voting period. Initial value: 2 weeks
Copy link
Contributor

Choose a reason for hiding this comment

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

switch all uint64 to int64 as per new conventions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

}
```

Each `Proposal` is identified by its unique `proposalID`.
Copy link
Contributor

Choose a reason for hiding this comment

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

is the proposalID the key in the KVStore? I noticed it's not included in the proposal struct... We should add discussion surrounding indexing and the governance state... talk to bucky about organization of the specs

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 for me ProposalID is the key. @sunnya97 is doing the implementation, so he should lead the discussion

Deposit uint64 // Current deposit on this proposal. Initial value is set at InitialDeposit
SubmitBlock uint64 // Height of the block where TxSubmitProposal was included
VotingStartBlock uint64 // Height of the block where MinDeposit was reached. -1 if MinDeposit is not reached.
Votes map[string]uint64 // Votes for each option (Yes, No, NoWithVeto, Abstain)
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use a map, the type should probably be either an array of int64 of a new struct VoteRes

type VoteRes struct {
    Yes, No, NoWithVeto, Abstain int64
}

Copy link
Contributor

Choose a reason for hiding this comment

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

(can't use maps in a merkle store due to non-determinism)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sunnya97 what's your opinion? Array of new struct?

Choose a reason for hiding this comment

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

What are we mapping from? So what would the string be? Validator/Delegator keys?

Also, does the proposal struct handle the overriding by delegators and can people update their votes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • string would be each option Yes No NoWithVeto Abstain.
  • overriding is done when processing TxVote
  • In MVP there is no vote update

Choose a reason for hiding this comment

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

With this structure we can't attribute who submitted what vote though? How are we going to update the votes when processing TxVote?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are 4 additional lists linked to each proposalID. One of them is VotersList where you can find every pubkey that voted on which option and associated to which validator.

Originally I wanted a mapping PubKey -> {option, valPubKey} instead of voterList but apparently not possible in Go. So I put lists as general struct but surely this needs to be adapted as @rigelrozanski said. Probably @sunnya97 should have a say in this as he is going to implement it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@adrianbrink @gamarin2 We don't need to be able to dicern who voted for what... we just need to know that nobody is voting twice and that the sum of the votes made... right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to be able to dicern:

  • Who voted under which validator (the same PubKey can vote twice if it has bonded Atoms to two distincts validators)
  • What option did each validator vote for

- `DepositorList`: List of addresses that deposited on the proposal with their associated deposit
- `VotersList`: List of addresses that voted **under each validator** with their associated option
- `InitVotingPowerList`: Snapshot of validators' voting power **when proposal enters voting period** (only saves validators whose voting power is >0).
- `MinusesList`: List of minuses for each validator. Used to compute validators' voting power when they cast a vote.
Copy link
Contributor

Choose a reason for hiding this comment

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

We likely probably not be holding lists, but rather tallies of the voting power which has voted and in which way, and then have a unique item in the store for each item if they need to be referenced... we should never need to iterate through any of these lists :) - There are many parrellels to this architecture and the staking spec... In the staking spec we don't hold a list of the bonds to a validator, only the sum of the bonds in the validator, and then a bunch of unique bonds in the store, but no list of it all... neat aye, we should do something like that here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah initially I wanted mappings but AFAIK you can't map PubKey to base type in Go. I'm too unexperienced in Go to know the best way to circumvent that fact. All I want is to link ProposalID to a mapping where I can find an info by giving a PubKey. Don't need to iterate then. @sunnya97 probably knows better than me on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, let's hear it sunny! this is an important implementation detail

Once a proposal is submitted, if `Proposal.Deposit < ActiveProcedure.MinDeposit`, Atom holders can send `TxDeposit` transactions to increase the proposal's deposit.

```Go

Copy link
Contributor

Choose a reason for hiding this comment

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

can delete these extra spaces in the spec within the code seperators hehe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

A `TxSubmitProposal` transaction can be handled according to the following pseudocode

```
// PSEUDOCODE //
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 we'd be better not including the pseudo code in the spec (still keep a copy though!) but just a short description of any of the programmatic nuances should be sufficient... This comment applies to all the pseudocode sections

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll leave it until @sunnya97 reviews it and then remove it in a later commit.

- **`BountyProposals`:** If accepted, a `BountyProposal` creates an open bounty. The `BountyProposal` specifies how many Atoms will be given upon completion. These Atoms will be taken from the `reserve pool`. After a `BountyProposal` is accepted by governance, anybody can submit a `SoftwareUpgradeProposal` with the code to claim the bounty. Note that once a `BountyProposal` is accepted, the corresponding funds in the `reserve pool` are locked so that payment can always be honored. In order to link a `SoftwareUpgradeProposal` to an open bounty, the submitter of the `SoftwareUpgradeProposal` will use the `Proposal.LinkedProposal` attribute. If a `SoftwareUpgradeProposal` linked to an open bounty is accepted by governance, the funds that were reserved are automatically transferred to the submitter.
- **Complex delegation:** Delegators could choose other representatives than their validators. Ultimately, the chain of representatives would always end up to a validator, but delegators could inherit the vote of their chosen representative before they inherit the vote of their validator. In other words, they would only inherit the vote of their validator if their other appointed representative did not vote.
- **`ParameterProposals` and `WhitelistProposals`:** These proposals would automatically change pre-defined parameters and whitelists. Upon acceptance, these proposals would not require validators to do the signal and switch process.
- **Better process for proposal review:** There would be two parts to `proposal.Deposit`, one for anti-spam (same as in MVP) and an other one to reward third party auditors.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also more complex voting styles? such as ranked voting??? <- big fan of this idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ranked voting is cool but not near future. These are things next in line just after MVP

type TxVote struct {
ProposalID uint // proposalID of the proposal
Option string // option from OptionSet chosen by the voter
ValidatorPubKey crypto.PubKey // PubKey of the validator voter wants to tie its vote to
Copy link
Contributor

Choose a reason for hiding this comment

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

We should draw more distintion here so folks know it's not the block signing key... I'd like ValGovernPubKey or something like that... get's confusing with TxVote as well... I think should maybe be called TxGovVote

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it is the block signing key. I did that under the assumption that GovernancePubKey would be findable using MainPubKey but not the opposite. If this is the case, we need the main pub key in order to know how many Atoms are bonded to it. If this is not the case, then I can change the type and the associated pseudocode.

Ok for TxGovVote and other type names.

Finally, if the proposal is accepted or `MinDeposit` was not reached before the end of the `MaximumDepositPeriod`, then Atom holders can send `TxClaimDeposit` transaction to claim their deposits.

```Go
type TxClaimDeposit struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

TxGovClaimDeposit? - maybe these transaction names should reference the fact that they apply to goverenace throughout?

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, done

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

Looking good! Let's iron through some of these comments

@ebuchman ebuchman mentioned this pull request Jan 26, 2018
11 tasks
@jaekwon jaekwon requested a review from sunnya97 January 31, 2018 17:33
@rigelrozanski
Copy link
Contributor

@gamarin2 we should add in discussion surrouding labelling a proposal a duplicate proposal from:
a) one that has already been voted on historically
b) one that is currently in the hopper

Also I was just thinking that it should be OKAY to re-propose an old rejected proposal if a significant number of voting power has changed from the previous vote (aka if the network changes hands old issues may be acceptable even if they were not acceptable earlier).

Copy link
Member

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

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

Some thoughts from review here.

I'll merge this and have opened new issue #82 to continue discussion.

As for the spec structure itself, the overview stuff is great, but we should try and structure it like:

State

For each data structure that will be stored in the AVL tree, describe it and it's fields

Transactions

For each transaction type, describe the Tx, how it is validated, and the effect of the tx on the state


There are two instances where Atom holders that deposited can claim back their deposit:
- If the proposal is accepted
- If the proposal's deposit does not reach `MinDeposit` for a period longer than `mMxDepositPeriod` (initial value: 2 months). Then the proposal is considered closed and nobody can deposit on it anymore.
Copy link
Member

Choose a reason for hiding this comment

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

mMx?


Threshold is defined as the minimum ratio of `Yes` votes to `No` votes for the proposal to be accepted.

Initially, the threshold is set at 50% with a possibility to veto if more than 1/3rd of votes (excluding `Abstain` votes) are `NoWithVeto` votes. This means that proposals are accepted is the ratio of `Yes` votes to `No` votes at the end of the voting period is superior to 50% and if the number of `NoWithVeto` votes is inferior to 1/3rd of total votes (excluding `Abstain`).
Copy link
Member

Choose a reason for hiding this comment

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

language needs to be fixed. If threshold is minimum ratio of Yes:No and threshold is 50%, means 1:2...

So either it's "minimum proportion of Yes votes" or some other fixed description but I don't think its the ratio of Yes to No.

The same language needs to be fixed in a variety of places


Threshold is defined as the minimum ratio of `Yes` votes to `No` votes for the proposal to be accepted.

Initially, the threshold is set at 50% with a possibility to veto if more than 1/3rd of votes (excluding `Abstain` votes) are `NoWithVeto` votes. This means that proposals are accepted is the ratio of `Yes` votes to `No` votes at the end of the voting period is superior to 50% and if the number of `NoWithVeto` votes is inferior to 1/3rd of total votes (excluding `Abstain`).
Copy link
Member

Choose a reason for hiding this comment

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

Is there a cost to NoWithVeto? Why not use it every time ?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Interesting - yeah should maybe cost a nominal amount more

Copy link
Contributor

Choose a reason for hiding this comment

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

but are the funds returned if everyone else also voted no with veto? The community shouldn't get collectively punished if somebody submits an outrageous proposal which must be veto'd


After a `SoftwareUpgradeProposal` is accepted, validators are expected to download and install the new version of the software while continuing to run the previous version. Once a validator has downloaded and installed the upgrade, it will start signaling to the network that it is ready to switch by including the proposal's `proposalID` in its *precommits*.(*Note: Confirmation that we want it in the precommit?*)

Note: There is only one signal slot per *precommit*. If several `SoftwareUpgradeProposals` are accepted in a short timeframe, a pipeline will form and they will be implemented one after the other in the order that they were accepted.
Copy link
Member

Choose a reason for hiding this comment

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

Needs discussion

MinDeposit int64 // Minimum deposit for a proposal to enter voting period.
OptionSet []string // Options available to voters. {Yes, No, NoWithVeto, Abstain}
ProposalTypes []string // Types available to submitters. {PlainTextProposal, SoftwareUpgradeProposal}
Threshold int64 // Minimum value of Yes votes to No votes ratio for proposal to pass. Initial value: 0.5
Copy link
Member

Choose a reason for hiding this comment

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

int64 but initial value is 0.5 ? relates to my comments above re fixing the definition here

Copy link
Contributor

Choose a reason for hiding this comment

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

use sdk.Rational (will be in sdk types directory)

@ebuchman
Copy link
Member

ebuchman commented Feb 3, 2018

Re pseudo code, I think it's ok but would be more useful to have the organization I mentioned and perhaps some pseudo code in each section related to that sections data structures

@ebuchman ebuchman merged commit f757482 into cosmos:master Feb 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants