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

Vesting account operations don't have represented gas prices #3817

Closed
ValarDragon opened this issue Mar 6, 2019 · 9 comments
Closed

Vesting account operations don't have represented gas prices #3817

ValarDragon opened this issue Mar 6, 2019 · 9 comments
Labels
T: Performance Performance improvements T: Security

Comments

@ValarDragon
Copy link
Contributor

ValarDragon commented Mar 6, 2019

Vesting accounts operations don't charge more gas, however they do more computation than standard accountss. To process fees, we have to check the "spendable coins", which requires calls to getVestedCoins and getSpendableCoins https://github.com/cosmos/cosmos-sdk/blob/develop/x/auth/account.go#L398 https://github.com/cosmos/cosmos-sdk/blob/develop/x/auth/account.go#L221. Then note that if you do a bank transaction with these, this same computation gets done twice, but for free both times.

As far as transactions go, these are computationally expensive (Far more than a bank tx for instance). Lets parameterize this with the number of coin types stored in the vesting account (not appearing in the fee) While one is in the vesting time period, getVestedCoins does a high precision decimal division, and then performs a linear number of decimal multiplications on every considered coin type's amount (a big int) by this decimal scalar.

Then getSpendableCoins does n^2 searches on the coin types, and a linear number of big int additions / subtractions.

This is certainly lower total time than signature verifications with a single coin type, so it may not matter near-term, but if a vesting account with many coin types gets added, then this gets increasingly expensive. (Certain transactions also call get spendable coins multiple times, e.g. bank, which increases the importance of this problem) This is something that is not at all reflected in gas price, and it can become expensive as number of coin types increases.

As a side note, the n^2 search can be made linear with structural refactoring.

@fedekunze fedekunze added the T: Performance Performance improvements label Apr 5, 2019
@fedekunze
Copy link
Collaborator

great analysis @ValarDragon.

How should we proceed to fix this ? Could you provide more context on the action items ?

@ValarDragon
Copy link
Contributor Author

Vesting account operations should have parameterized gas charges. I don't know what is the cleanest way to do that in the code.

@alexanderbez
Copy link
Contributor

Before we used to charge gas directly in the bank keeper, so it would've been easier. But we've since removed direct gas consumption calls. Thoughts @jaekwon ?

@cwgoes
Copy link
Contributor

cwgoes commented Jan 9, 2020

Is this still the case? Relevant to #5492.

@alexanderbez
Copy link
Contributor

Yes, this is still the case @cwgoes

@github-actions
Copy link
Contributor

This issue 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 Jul 17, 2020
@cwgoes cwgoes removed the stale label Jul 17, 2020
@cwgoes
Copy link
Contributor

cwgoes commented Jul 17, 2020

Still applicable.

@elias-orijtech
Copy link
Contributor

Gentle ping. Still relevant? If so, anybody motivated to sketch a design for fixing it?

@alexanderbez
Copy link
Contributor

Given that account abstractions are in the hot seat, this issue might become irrelevant as vesting will no longer be a concrete account type most likely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: Performance Performance improvements T: Security
Projects
None yet
Development

No branches or pull requests

6 participants