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

performance issue for gov/tally #11422

Closed
4 tasks
carameleon opened this issue Mar 21, 2022 · 4 comments
Closed
4 tasks

performance issue for gov/tally #11422

carameleon opened this issue Mar 21, 2022 · 4 comments
Labels

Comments

@carameleon
Copy link

carameleon commented Mar 21, 2022

Summary of Bug

This is a result query to full node. ( ~ 1TB data)

func TestGetProposals(t *testing.T) {
	queryClient := cli.GetGovQueryClient()
	request := govtypes.QueryTallyResultRequest{ProposalId: 63}

	tallyResp, err := queryClient.TallyResult(context.Background(), &request)
	require.NoError(t, err)

	t.Log(tallyResp.Tally)
}
=== RUN   TestGetProposals
    /cosmos/client/gov_test.go:20: "yes": "23472585477427"
        abstain: "20014268669628"
        "no": "2690448414295"
        no_with_veto: "43925427829"

--- PASS: TestGetProposals (311.73s)

it takes time 5mins to get gov/tally about single prop (63)

pruned node1 (under 10GB ./data)

takes 20 secs to get gov/tally for prop 62 ,63

pruned node2 (115G ./data)

Mar 14 16:02:23 start proposal update : 62
Mar 14 16:04:07 finish proposal update : 62

Mar 21 08:07:31 start updating proposals : 62, 63
Mar 21 08:14:27 finish update proposals : 62, 63

It takes 7mins to get gov/tally about prop 62, 63.
That means node will stop for 7mins to complete return gov/tally.
In my memory, It has been happening since prop 60.(means node slow down clearly)

I'm wondering

  • is there any approach to get gov/tally result faster.
  • will be improved upcoming mainnet upgrade?

Version

gaiad v6.0.0

Steps to Reproduce

try to get tally result using gRPC.

    queryClient := govtypes.NewQueryClient(c.GRPC)
    request := govtypes.QueryTallyResultRequest{ProposalId: resp.Proposal.ProposalId}
    tally, err := queryClient.TallyResult(context.Background(), &request)

For Admin Use

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

Are these for proposals that are still in their voting periods? If so, the cost would come from Keeper#Tally which iterates over all bonded validators and iterates over all votes, which I imagine incurs a majority of the cost, although I don't have evidence to prove this.

That being that case, idk what we can really do here. Hope that SMT and/or latest IAVL improvements from Osmosis helps?

@ValarDragon
Copy link
Contributor

ValarDragon commented Mar 23, 2022

We're testing what we hope to be the last fix for IAVL stability, hopefully we can work on upstreaming it after that. I think thats the only thing that can be done to fix this computation overhead :(

We could build accumulators into the state machine, which is probably good long term, but the root cause of slowness here is definitely IAVL & bad levelDB configs.

I think on top of the osmosis work, getting a roadmap of levelDB things that we should try in prod to help speed things up is a great move. I know @ValNodes and @marbar have been working on both code to compact more leveldb data for speed + playing around with configs.

@fig666
Copy link

fig666 commented Mar 23, 2022

  • the incentive comes from the gov. treasury toll of over paid deposits and refunds from contributions of all disbanded users of ended projects from github's database.

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.
@tac0turtle tac0turtle moved this to 👀 To Do in Cosmos-SDK Nov 16, 2023
@julienrbrt
Copy link
Member

Given that TallyResut just call keeper.Tally, closing this as dup of #10353, so we have one tracking issue for the improvements.

@github-project-automation github-project-automation bot moved this from 👀 To Do to 🥳 Done in Cosmos-SDK Feb 28, 2024
@tac0turtle tac0turtle removed this from Cosmos-SDK Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants