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

Change Tally query to be state machine based #10353

Open
4 tasks
mattverse opened this issue Oct 13, 2021 · 2 comments
Open
4 tasks

Change Tally query to be state machine based #10353

mattverse opened this issue Oct 13, 2021 · 2 comments

Comments

@mattverse
Copy link
Contributor

mattverse commented Oct 13, 2021

Summary

Current Tally query iterates over all the votings in the governance proposals causing latency issues to nodes or services that provide API endpoints to the public. Currently, the Tally query in the cosmos hub has an average response time of ~35.14 seconds, for an individual query, causing additional problems when such query has been made. This problem could be possibly solved by using the state machine to record the state of delegations of voting powers (possibly with the additional usage of staking hooks and other hooks) instead of iterating over all votings each time Tally query has been made.

Problem Definition

Latency issues for services running LCD nodes can be improved.

Proposal

  • Use state machine to record the state of Voting power

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@ValarDragon
Copy link
Contributor

ValarDragon commented Oct 13, 2021

Fully agreed this should be done, with planning for how it will extend to more liquid democracy setups.

We should also benchmark whats going on with the tally function, I suspect that will be a more near-term approach to a 10x improvement. My strong suspicion is that most of the overhead is fixable with fixing some code design patterns that become evident in profiling the relevant code. I suspect that the main culprits are:

Ranked by my understanding of overhead.

tbruyelle added a commit to allinbits/cosmos-sdk-fork that referenced this issue Jul 26, 2023
Although rather complex, the `govKeeper.Tally` function has no unit
tests. This change adds a test that covers around 91% of the code, only
some unexpected errors are not covered.

If this change is accepted, an other will follow to refactor the
function into smaller parts (without changing the test). This will
address the TODO on top, reduce complexity,  improve readability and
reusability.

This test should also help for issues like cosmos#11422 and cosmos#10353. It's more
comfortable to improve performance or completely rewrite the
implementation with a high code coverage.

The `setupGovKeeper` had to be modified because it was registering some
mocks expectations that cannot be overrided. It now takes an additional
variadic argument that can be used to better customize the mocks
expectations. Also, to improve readability, the mocks are all gathered in
a new specific `mocks` struct.
tbruyelle added a commit to allinbits/cosmos-sdk-fork that referenced this issue Jul 26, 2023
Although rather complex, the `govKeeper.Tally` function has no unit
tests. This change adds a test that covers around 91% of the code, only
some unexpected errors are not covered.

If this change is accepted, another will follow to refactor the
function into smaller parts (without changing the test). This will
address the TODO on top, reduce complexity,  improve readability and
reusability.

This test should also help for issues like cosmos#11422 and cosmos#10353. It's more
comfortable to improve performance or completely rewrite the
implementation with a high code coverage.

The `setupGovKeeper` had to be modified because it was registering some
mocks expectations that cannot be overridden. It now takes an additional
variadic argument that can be used to better customize the mocks
expectations. Also, to improve readability, the mocks are all gathered in
a new specific `mocks` struct.
tbruyelle added a commit to allinbits/cosmos-sdk-fork that referenced this issue Jul 26, 2023
Although rather complex, the `govKeeper.Tally` function has no unit
tests. This change adds a test that covers around 91% of the code, only
some unexpected errors are not covered.

If this change is accepted, another will follow to refactor the
function into smaller parts (without changing the test). This will
address the TODO on top, reduce complexity,  improve readability and
reusability.

This test should also help for issues like cosmos#11422 and cosmos#10353. It's more
comfortable to improve performance or completely rewrite the
implementation with a high code coverage.

The `setupGovKeeper` had to be modified because it was registering some
mocks expectations that cannot be overridden. It now takes an additional
variadic argument that can be used to better customize the mocks
expectations. Also, to improve readability, the mocks are all gathered in
a new specific `mocks` struct.
@julienrbrt julienrbrt self-assigned this Jan 23, 2024
@alexanderbez
Copy link
Contributor

alexanderbez commented Feb 23, 2024

Thanks for creating this issue @mattverse! We discussed this at decent length today. My summary of this, based on our convo and this issue, is that there are a few main ways we can tackle this, not necessary mutually exclusive:

  1. Charge an explicit additional gas fee. This ties into the general theme of "multi-dimensional" fees, which we should explore regardless. Note, however, this does NOT address the iteration bottleneck. In fact, you'll probably see less vote participation, which can be sort of a con.
  2. Introduce additional constraints to voting, e.g. feat(x/gov): add min_stake_to_vote parameter #18186. This has the same pros/cons as (1).
  3. Refactor tallying completely. I'll describe my proposal for this below at a high level:

Refactor Tallying Proposal

My proposal is to refactor tallying completely, to completely get rid of tally iteration altogether or at the very least, limit it drastically. One way to achieve this would be to have "lazy" or cumulative tallying, i.e. tally as votes come in. Now, there are two things we want to consider when doing this. First, is that it doesn't seem we can avoid relying on total VP when accumulating votes; does this introduce any game theory attacks?. Secondly, we need to ensure that we can handle changed votes and custom tally functions.

You could achieve this using one or possibly more collections, s.t. you tally based on (proposalID, vote/weight, voter, VP)

Note, you could even introduce (1) and (3) together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📋 Backlog
Development

No branches or pull requests

5 participants