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

ADR 0006: Consensus Governance #3423

Merged
merged 1 commit into from
Nov 11, 2020
Merged

Conversation

kostko
Copy link
Member

@kostko kostko commented Oct 16, 2020

@kostko kostko added the c:docs/adr Category: documentation/ADR label Oct 16, 2020
@kostko kostko force-pushed the kostko/docs/adr-consensus-governance branch 3 times, most recently from 184c157 to 58a3856 Compare October 19, 2020 08:35
@kostko kostko force-pushed the kostko/docs/adr-consensus-governance branch from 58a3856 to 4de1301 Compare October 20, 2020 16:00
@kostko kostko marked this pull request as ready for review October 20, 2020 16:04
@kostko kostko force-pushed the kostko/docs/adr-consensus-governance branch from 4de1301 to dda1c1a Compare October 26, 2020 16:59
### Consensus Parameters

This proposal introduces the following new consensus parameters in the
governance module:
Copy link

Choose a reason for hiding this comment

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

Here we can specify: "Since these are consensus parameters, they need to be specified in the genesis file and will be the same across all proposals."

This will help clarify the nature of the parameters.

## Decision

This proposal introduces a minimal on-chain governance mechanism where anyone
can submit governance proposals and the validators can vote where one base unit
Copy link

Choose a reason for hiding this comment

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

Maybe we can explicitly point out that this includes both self-delegations and delegations from other users, since conceptually sometimes people think about delegation as primarily in the "delegations from other users" case.

We can just add parentheses at the end: "(including both self-delegations and delegations from other users)."


// UpgradeProposal is an upgrade proposal.
type UpgradeProposal struct {
upgrade.Descriptor
Copy link
Member

Choose a reason for hiding this comment

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

Is #2596 a prerequisite for this ADR? Or maybe at least a "good to have", so voters can actually confirm the proposed upgrade identifier (hash) matches a specific source code. (i guess alternatively, although not ideal, we could use something like hash of the source code?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's right. So in theory the hash could also be a Git commit or a hash of a manifest containing further information.

@kostko kostko force-pushed the kostko/docs/adr-consensus-governance branch from dda1c1a to 12588f8 Compare October 27, 2020 09:16

- The set of pending upgrades is checked to make sure that no upgrades are
currently pending. If there is an existing upgrade pending, the method call
fails with `ErrUpgradeAlreadyPending`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little odd: we prevent new proposals when one is pending, but as long as active proposals have not been voted on and moved to pending state, additional proposals can be made. Is the intent to prevent two pending proposals from becoming ... "in use" (i.e., approved and switch-over epoch has passed, so presumably all node operators have picked up the implementation or implemented the proposal themselves)? What happens (if anything) if two or more active proposals go into pending state and their switch-over epochs are close to each other, e.g., within upgrade_min_epochs_diff of each other?

Not a critical issue, but just wanted clarification / trying to understand the intent.

Copy link
Member Author

Choose a reason for hiding this comment

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

we prevent new proposals when one is pending, but as long as active proposals have not been voted on and moved to pending state, additional proposals can be made

I think this is just a misunderstanding, there are two things to an upgrade:

  • First a governance upgrade proposal is created and it goes through the voting process.
  • Only if the vote passes is an upgrade pending.

So this does not refer to other pending governance upgrade proposals but to already passed pending upgrades.

What happens (if anything) if two or more active proposals go into pending state and their switch-over epochs are close to each other, e.g., within upgrade_min_epochs_diff of each other?

See the Proposal Content Execution section below where there is another check when the passed governance upgrade proposal is executed. Only one upgrade can be pending and if two upgrade proposals are passed, the second one will fail to execute.

@kostko kostko force-pushed the kostko/docs/adr-consensus-governance branch from 12588f8 to 8e76033 Compare October 28, 2020 20:12

This proposal adds the following consensus layer state in the governance module:

- **Next proposal identifier (`0x80`)**
Copy link
Member

Choose a reason for hiding this comment

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

If we follow the existing convention, I think these identifiers will start at 0x90 (and the governance app id will be 0x09) since 0x08 is in use by the new improved beacon already.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, so I looked at the beacon when deciding on this and it uses the prefix from the previous beacon/epochtime apps which seems like the right thing to do:

// epochCurrentKeyFmt is the current epoch key format.
//
// Value is CBOR-serialized epoch time state.
epochCurrentKeyFmt = keyformat.New(0x30)
// epochFutureKeyFmt is the future epoch key format.
//
// Value is CBOR-serialized epoch time state.
epochFutureKeyFmt = keyformat.New(0x31)
// beaconKeyFmt is the random beacon key format.
//
// Value is raw random beacon.
beaconKeyFmt = keyformat.New(0x40)
// parametersKeyFmt is the key format used for consensus parameters.
//
// Value is CBOR-serialized beacon.ConsensusParameters.
parametersKeyFmt = keyformat.New(0x41)

The app id for the beacon should be changed to 0x04 to be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Ok yeah I also saw the comment in the beacon PR 👍

@kostko kostko force-pushed the kostko/docs/adr-consensus-governance branch from 8e76033 to 60fdaba Compare November 10, 2020 16:44
@kostko kostko mentioned this pull request Nov 10, 2020
21 tasks
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

my summary:

this adds on-chain yes/no/abstain voting for "proposals." entities can vote, and their votes are counted proportional to their stake if they are a validator at the time the proposal is set to close. as an optimization for stable validator committees, they must also be validator at the time they vote. they can also change their vote leading up to the proposal close time.

this adds a set of proposal types for managing network upgrades. the on-chain representation of a proposed upgrade https://github.com/oasisprotocol/oasis-core/blob/v20.12.1/go/upgrade/api/api.go#L66 is mostly opaque, other than the epoch when it happens. this type and the mechanisms around it are already part of the codebase.

@kostko kostko force-pushed the kostko/docs/adr-consensus-governance branch from 60fdaba to 8098873 Compare November 11, 2020 09:15
@kostko kostko merged commit a94d831 into master Nov 11, 2020
@kostko kostko deleted the kostko/docs/adr-consensus-governance branch November 11, 2020 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:docs/adr Category: documentation/ADR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants