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

x/gov: Change proposal state storage, so votes do not read static data #12688

Closed
4 tasks
ValarDragon opened this issue Jul 22, 2022 · 6 comments · Fixed by #13576
Closed
4 tasks

x/gov: Change proposal state storage, so votes do not read static data #12688

ValarDragon opened this issue Jul 22, 2022 · 6 comments · Fixed by #13576
Assignees
Labels

Comments

@ValarDragon
Copy link
Contributor

ValarDragon commented Jul 22, 2022

Summary

A serious problem for cosmwasm upload governance proposals, is that the x/gov module has every vote, read the entire governance proposal. Votes should not require reading static data from a proposal, e.g. the Content. The current design makes votes for wasm upload proposals require 1million+ gas.

This applies in SDK v0.46 with the message based governance module as well. This is because proposals are written in state with the entire proposal content at one state entry.

Thank you @ethanfrey for pointing this out

Problem Definition

Fix gas consumption for custom governance proposals. This applies outside of just cosmwasm proposals, and even applies in the 46 framing of Messages in the proposal.

I'd classify the current proto structure of having the content in the same key in state as a bug.

Proposal

Change the proto structure of governance proposals to separate out static proposal content into another struct, and that is written in state at a different key than the core mutable data. (Both still indexed by governance proposal ID)

Then in votes, we do not read the static content.
I'd even argue that we should consider having three state entries per proposal:

  • Proposal Static Metadata
  • Proposal Content for execution
  • Proposal vote data

And adjust the static metadata to not have the content in the same entry, as the content can have extremely high length


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@ValarDragon ValarDragon changed the title Change proposal state storage, so votes do not read static data x/gov: Change proposal state storage, so votes do not read static data Jul 22, 2022
@ethanfrey
Copy link
Contributor

I agree with this strongly. It has been an issue with both Osmosis and Statgaze. Especially as some clients fix a cost for voting, less than 1 million obviously.

I believe this same issue hits proposals with very long text descriptions, albeit an order of magnitude less, still much unnecessary reading and writing and gas burn

@tac0turtle
Copy link
Member

This sounds good to me. This would also coincide with the idea of using the file system to store votes instead of the tree.

@alexanderbez
Copy link
Contributor

This would also coincide with the idea of using the file system to store votes instead of the tree.

I think we'd need to use some sort of Merkle tree, like a Merkle BTree (see osmosis-labs#294), because we need this data to still be state sync-able, but we don't need proofs for them. Opposed to flat files, which cannot be state synced AFAIK.

@tac0turtle
Copy link
Member

Flat files can be state syncable. The same way CosmWasm contract code is state syncable.

@ethanfrey
Copy link
Contributor

This can also be a two step process. First to pull votes into separate (iavl) struct, which solve the immediate problem.

Second, later, to pull them out of iavl tree completely if that is needed for performance (I would have good benchmarks of real data to show this is a slowdown and the solution would make a benefit)

@alexanderbez
Copy link
Contributor

Flat files can be state syncable. The same way CosmWasm contract code is state syncable.

ohh, well this isn't what @ValarDragon told me :P

@facundomedica facundomedica moved this to 📝 Todo in Cosmos-SDK Oct 19, 2022
@facundomedica facundomedica moved this from 📝 Todo to 💪 In Progress in Cosmos-SDK Oct 19, 2022
@facundomedica facundomedica moved this from 💪 In Progress to 👀 Needs Review in Cosmos-SDK Oct 26, 2022
Repository owner moved this from 👀 Needs Review to 👏 Done in Cosmos-SDK Oct 26, 2022
@tac0turtle tac0turtle removed this from Cosmos-SDK Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@alexanderbez @ethanfrey @ValarDragon @facundomedica @tac0turtle and others