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

[DON'T MERGE] feat(x/gov): add governors #16

Draft
wants to merge 47 commits into
base: main
Choose a base branch
from

Conversation

giunatale
Copy link
Collaborator

@giunatale giunatale commented Sep 17, 2024

  • You can only delegate to a single governor, and 100% of your governance VP or nothing, but you can always override the vote.
  • Governors election is done on "estimated" VP exactly like validators, so only top X (configurable param) active governors by VP are counted for voting
  • Governors can set their status between active and inactive but can change it only once every 4 weeks. Active governors can be elected (inactive governors are skipped) but the base account has automatically a self-delegation of all its VP to the governor and cannot redelegate. Inactive governors can re-delegate.
  • Governors VP is "estimated" (I think it's an exact computation as I think I cover everything) using x/staking hooks so when slashing/bond/unbond/redel happen it triggers a VP update.
  • It should be ok computationally and also ok in how data is stored for easy retrieval, however maybe not optimized for storage itself as there is the need for a couple of additional indexes. Need to also check how much the governor VP computation is accurate because it might allow to further simplify VP computation (skip part of it) in tally
  • I also added an invariant but I am not sure how that impacts performance
  • Currently missing all tests, code is also itself untested, it was a weekend project. No guarantees on quality or if it works at all (or as intended).

Based on discussions as recorded in https://gist.github.com/giunatale/95e9b43f6e265ba32b29e2769f7b8a37

EDIT:

  • Governors need to have at least minSelfDelegation (param) tokens bonded in x/staking to be elegible for tally (active governors always require to "self-delegate" the governance VP from their base account)
  • Added another invariant on governors delegations consistency with cumulative virtual validator shares stored

Only sketching out how the tally would work for now.
Lots of stuff is left undefined but the strategy is the one outlined in https://gist.github.com/giunatale/95e9b43f6e265ba32b29e2769f7b8a37?permalink_comment_id=5067400#gistcomment-5067400 but with the restriction of being able to delegate a percentage of its bonded tokens to at most one governor (https://gist.github.com/giunatale/95e9b43f6e265ba32b29e2769f7b8a37?permalink_comment_id=5187246#gistcomment-5187246)

It assumes that every time someone redelegates, undelegates or adds to its delegations a hook is called to check if there is a governor delegated to in x/gov, and the governor's total shares updated accordingly.
The governor's total shares are a collection of share amounts for different validators.
can only delegate all or nothing to governors now
x/gov/keeper/tally.go Dismissed Show resolved Hide resolved
x/gov/keeper/tally.go Fixed Show fixed Hide fixed
x/gov/keeper/invariants.go Fixed Show fixed Hide fixed
@giunatale
Copy link
Collaborator Author

giunatale commented Sep 25, 2024

An alternative system that does not make use of the aggregate VP is also available at:

In brief:

  • It removes the aggregate VP (and related indexing) for governors, so no need to loop over all involved governors in the slashing hook (which is removed here)
  • Governor election is simplified to happen only among governors that voted, so if a governor had a lot of delegations but did not vote it basically gave up its spot. Actually since there is no need to limit the number of governors with this system the max governors param is removed. Only account for active governors that voted and meet the minimum self delegation requirement.
  • Since there is no aggregate VP number, the optimization for quorum is not possible

Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Nov 18, 2024
@arlai-mk
Copy link

Hi, thanks for your work on this, it really starts to look like the idea I tried to work on in March/April 2023 here (except I called governors "representatives"): https://github.com/arlai-mk/cosmos-sdk/tree/arlai/gov-complex-delegation

May I ask why the need to restrict to one governor only, with 100% of your VP, and not ability to spread your VP across multiple governors.
My original idea was to spread by percentage: you say that you want to delegate your Governance VP to X governors, with let's say 40% to governor #1, 30% to governor #2, and 30% to governor #3. This way, for each staking/slashing event, you can still recalculate the VP that is delegated to each governor.
I reckon it slightly complexifies the computations, but as you will need to recalculate anyway, I don't think the overhead is too big for a much better UX (ability to delegate to multiple governors).

Thank you.

@giunatale
Copy link
Collaborator Author

giunatale commented Nov 18, 2024

Thank you for your question. The reason is tally performance. With such a simplification the impact on tally is limited while complexity would potentially add another term if we allow multiple delegations. Look at https://gist.github.com/giunatale/95e9b43f6e265ba32b29e2769f7b8a37, and the comments because there are corrections along the way. Happy to explain more if needed.

Also, ultimately in practice there is no need to really spread your governance delegations if they are independent from staking and are purely related to governance, and you can freely redelegate at any times and override with your vote. For staking, spreading your delegations is important cause it helps distribute stake and achieve a higher nakamoto coefficient. But looking at the governance system independently, why would you need to have more than one representative? Other delegated governance systems (e.g. Optimism) also don't have this. And if you think about governance delegations as purely a system to facilitate governance, I doubt that adding multi-delegations really adds much. In the end, if you have a nuanced opinion, then you should vote yourself. The delegations/governor system is there to help people that don't have time/knowledge and I think it's better to ask them to find one governor that they like instead of a pletora.

Also, I invite you to actually look at https://github.com/giunatale/atomone/tree/governors-no-aggregate-vp because imho is the better approach (for more info check the comment from me above) and is more complete with tests and a bunch of fixes.

EDIT: btw the percentage is also what I had originally in mind. But again the tally complexity that way adds another term O(number of votes x validators x governors).

@github-actions github-actions bot removed the Stale label Nov 19, 2024
tbruyelle added a commit that referenced this pull request Dec 10, 2024
Spotted by `make vulncheck`

Last seen in
https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:14

```
Vulnerability #1: GO-2024-3279
    ASA-2024-010: cosmossdk.io/math: Mismatched bit-length validation in sdk.Int
    and sdk.Dec can lead to panic
  More info: https://pkg.go.dev/vuln/GO-2024-3279
  Module: cosmossdk.io/math
    Found in: cosmossdk.io/[email protected]
    Fixed in: cosmossdk.io/[email protected]
    Example traces found:
Error:       #1: app/export.go:209:63: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.ApplyAndReturnValidatorSetUpdates, which calls math.Int.Add
Error:       #2: app/genesis_account.go:31:33: app.SimGenesisAccount.Validate calls types.Coins.IsAnyNil, which eventually calls math.Int.BigInt
Error:       #3: x/gov/types/v1beta1/gov.pb.go:732:20: v1beta1.TallyResult.Equal calls math.Int.Equal
Error:       #4: x/gov/keeper/deposit.go:180:97: keeper.Keeper.AddDeposit calls types.Coins.IsAllGTE, which calls math.Int.GT
Error:       #5: ante/gov_vote_ante.go:46:[14](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:15): ante.GovVoteDecorator.AnteHandle calls types.ChainAnteDecorators, which eventually calls math.Int.GTE
Error:       #6: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.Int.Int64
Error:       #7: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.Int.IsInt64
Error:       #8: x/gov/keeper/deposit.go:145:28: keeper.Keeper.AddDeposit calls types.NewCoin, which eventually calls math.Int.IsNegative
Error:       #9: x/gov/types/v1/msgs.go:101:30: types.MsgSubmitProposal.ValidateBasic calls types.MsgCancelUnbondingDelegation.ValidateBasic, which calls math.Int.IsPositive
Error:       #10: x/gov/keeper/tally.go:58:23: keeper.Keeper.HasReachedQuorum calls math.Int.IsZero
Error:       #11: x/gov/keeper/deposit.go:[15](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:16)5:20: keeper.Keeper.AddDeposit calls types.Coin.IsGTE, which calls math.Int.LT
Error:       #12: x/gov/keeper/deposit.go:63:37: keeper.DeleteAndBurnDeposits calls keeper.BaseKeeper.BurnCoins, which eventually calls math.Int.Marshal
Error:       #13: x/gov/types/v1beta1/gov.pb.go:994:38: v1beta1.TallyResult.MarshalToSizedBuffer calls math.Int.MarshalTo
Error:       #14: app/export.go:11:2: app.init calls staking.init, which eventually calls math.Int.Mul
Error:       #15: x/gov/keeper/deposit.go:[16](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:17)8:54: keeper.Keeper.AddDeposit calls keeper.BaseKeeper.SendCoinsFromAccountToModule, which eventually calls math.Int.Neg
Error:       #16: app/export.go:209:63: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.ApplyAndReturnValidatorSetUpdates, which eventually calls math.Int.Quo
Error:       #[17](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:18): ante/gov_vote_ante.go:46:14: ante.GovVoteDecorator.AnteHandle calls types.ChainAnteDecorators, which eventually calls math.Int.QuoRaw
Error:       #[18](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:19): x/gov/types/v1beta1/msgs.go:167:29: v1beta1.MsgDeposit.ValidateBasic calls types.Coins.IsAnyNegative, which eventually calls math.Int.Sign
Error:       #[19](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:20): x/gov/types/v1beta1/gov.pb.go:1322:16: v1beta1.TallyResult.Size calls math.Int.Size
Error:       #[20](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:21): x/gov/types/v1/tally.go:12:27: types.NewTallyResult calls math.Int.String
Error:       #[21](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:22): app/export.go:209:63: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.ApplyAndReturnValidatorSetUpdates, which calls math.Int.Sub
Error:       #[22](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:23): x/gov/keeper/deposit.go:145:73: keeper.Keeper.AddDeposit calls math.Int.ToLegacyDec
Error:       #23: x/gov/types/v1beta1/gov.pb.go:2141:29: v1beta1.TallyResult.Unmarshal calls math.Int.Unmarshal
Error:       #24: x/gov/types/v1beta1/msgs.go:263:32: v1beta1.MsgVoteWeighted.ValidateBasic calls math.LegacyDec.Add
Error:       #25: app/sim/sim_state.go:203:34: sim.AppStateRandomizedFn calls module.SimulationManager.GenerateGenesisStates, which eventually calls math.LegacyDec.BigInt
Error:       #26: ante/gov_vote_ante.go:46:14: ante.GovVoteDecorator.AnteHandle calls types.ChainAnteDecorators, which eventually calls math.LegacyDec.Ceil
Error:       #27: x/gov/types/v1beta1/params.go:68:[24](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:25): v1beta1.TallyParams.Equal calls math.LegacyDec.Equal
Error:       #28: x/gov/client/cli/query.go:624:15: cli.GetCmdQueryProposer calls fmt.Sprintf, which eventually calls math.LegacyDec.Format
Error:       #29: x/gov/types/v1beta1/msgs.go:270:19: v1beta1.MsgVoteWeighted.ValidateBasic calls math.LegacyDec.GT
Error:       #30: x/gov/keeper/tally.go:93:26: keeper.Keeper.HasReachedQuorum calls math.LegacyDec.GTE
Error:       #31: x/gov/types/v1/params.go:129:22: types.Params.ValidateBasic calls math.LegacyDec.IsNegative
Error:       #32: x/gov/types/v1/msgs.go:101:30: types.MsgSubmitProposal.ValidateBasic calls types.MsgUpdateParams.ValidateBasic, which calls math.LegacyDec.IsNil
Error:       #33: x/gov/types/v1/params.go:140:26: types.Params.ValidateBasic calls math.LegacyDec.IsPositive
Error:       #34: x/gov/keeper/deposit.go:138:28: keeper.Keeper.AddDeposit calls math.LegacyDec.IsZero
Error:       #35: x/gov/types/v1beta1/msgs.go:274:19: v1beta1.MsgVoteWeighted.ValidateBasic calls math.LegacyDec.LT
Error:       #36: app/export.go:96:53: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.WithdrawDelegationRewards, which eventually calls math.LegacyDec.LTE
Error:       #37: app/app.go:[25](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:26)4:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.LegacyDec.Marshal
Error:       #38: x/gov/types/v1beta1/gov.pb.go:1187:41: v1beta1.TallyParams.MarshalToSizedBuffer calls math.LegacyDec.MarshalTo
Error:       #39: x/gov/keeper/deposit.go:145:79: keeper.Keeper.AddDeposit calls math.LegacyDec.Mul
Error:       #40: x/gov/keeper/tally.go:1[26](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:27):31: keeper.tallyVotes calls keeper.Keeper.IterateDelegations, which eventually calls math.LegacyDec.MulInt
Error:       #41: app/export.go:96:53: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.WithdrawDelegationRewards, which eventually calls math.LegacyDec.MulInt64
Error:       #42: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.LegacyDec.MulTruncate
Error:       #43: app/export.go:151:60: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Hooks.BeforeDelegationCreated, which eventually calls math.LegacyDec.Neg
Error:       #44: x/gov/keeper/tally.go:88:39: keeper.Keeper.HasReachedQuorum calls math.LegacyDec.Quo
Error:       #45: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.LegacyDec.QuoInt
Error:       #46: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.LegacyDec.QuoRoundUp
Error:       #47: app/export.go:96:53: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.WithdrawDelegationRewards, which eventually calls math.LegacyDec.QuoTruncate
Error:       #48: x/gov/keeper/deposit.go:246:112: keeper.Keeper.validateInitialDeposit calls math.LegacyDec.RoundInt
Error:       #49: x/gov/simulation/genesis.go:84:52: simulation.GenTallyParamsConstitutionalThreshold calls math.LegacyDec.RoundInt64
Error:       #50: x/gov/types/v1beta1/gov.pb.go:1392:19: v1beta1.TallyParams.Size calls math.LegacyDec.Size
Error:       #51: x/gov/keeper/msg_server.go:[27](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:28)4:29: keeper.legacyMsgServer.VoteWeighted calls math.LegacyDec.String
Error:       #52: x/gov/keeper/tally.go:41:25: keeper.Keeper.Tally calls math.LegacyDec.Sub
Error:       #53: x/gov/keeper/deposit.go:145:108: keeper.Keeper.AddDeposit calls math.LegacyDec.TruncateInt
Error:       #54: cmd/atomoned/main.go:16:26: atomoned.main calls cmd.Execute, which eventually calls math.LegacyDec.TruncateInt64
Error:       #55: x/gov/types/v1beta1/gov.pb.go:2680:32: v1beta1.TallyParams.Unmarshal calls math.LegacyDec.Unmarshal
Error:       #56: app/export.go:96:53: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.WithdrawDelegationRewards, which eventually calls math.LegacyMinDec
Error:       #57: x/gov/simulation/genesis.go:73:37: simulation.GenMinDepositRatio calls math.LegacyMustNewDecFromStr
Error:       #58: x/gov/types/v1beta1/msgs.go:257:34: v1beta1.MsgVoteWeighted.ValidateBasic calls math.LegacyNewDec
Error:       #59: app/sim/sim_state.go:203:34: sim.AppStateRandomizedFn calls module.SimulationManager.GenerateGenesisStates, which eventually calls math.LegacyNewDecFromBigIntWithPrec
Error:       #60: x/gov/keeper/tally.go:88:64: keeper.Keeper.HasReachedQuorum calls math.LegacyNewDecFromInt
Error:       #61: x/gov/keeper/grpc_query.go:406:35: keeper.legacyQueryServer.Params calls math.LegacyNewDecFromStr
Error:       #62: x/gov/simulation/genesis.go:85:27: simulation.GenTallyParamsConstitutionalThreshold calls math.LegacyNewDecWithPrec
Error:       #63: x/gov/types/v1/params.go:132:32: types.Params.ValidateBasic calls math.LegacyOneDec
Error:       #64: app/export.go:96:53: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.WithdrawDelegationRewards, which eventually calls math.LegacySmallestDec
Error:       #65: x/gov/keeper/tally.go:41:77: keeper.Keeper.Tally calls math.LegacyZeroDec
Error:       #66: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.MaxInt
Error:       #67: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.MinInt
Error:       #68: app/helpers/test_helpers.go:79:69: helpers.Setup calls math.NewInt
Error:       #69: x/gov/keeper/proposal.go:72:24: keeper.Keeper.SubmitProposal calls baseapp.RegisterService, which eventually calls math.NewIntFromBigInt
Error:       #70: x/gov/migrations/v3/convert.go:73:35: migrations.ConvertToLegacyTallyResult calls math.NewIntFromString
Error:       #71: x/gov/types/v1beta1/codec.go:7:2: v1beta1.init calls types.init, which calls math.NewIntFromUint64
Error:       #72: app/sim/sim_state.go:203:34: sim.AppStateRandomizedFn calls module.SimulationManager.GenerateGenesisStates, which eventually calls math.OneInt
Error:       #73: x/gov/types/v1beta1/tally.go:55:36: v1beta1.EmptyTallyResult calls math.ZeroInt
Error:       #74: app/sim/sim_state.go:205:[31](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:32): sim.AppStateRandomizedFn calls json.Marshal, which eventually calls math.init
Error:       #75: x/gov/types/v1beta1/genesis.go:6:2: v1beta1.init calls math.init
```
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.

2 participants