-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Ethanfrey/4938 better gas estimation #6635
Ethanfrey/4938 better gas estimation #6635
Conversation
4ee54da
to
8c53084
Compare
I got curious as to where all the reads/writes came from for the 50k gas for a simple send (and why the two were different). Here is some logging of the gas-charged writes I would love an eye from someone of the core team to explain what all this is - and then I can make better sense of the differences between deliver and simulate. Note; only the section under |
Thanks for looking into this @ethanfrey. Trying to grok this, and correct me if I'm wrong, simulating (with the current code) AFAICT, But my original proposal was to use a separate (third) state object, |
Also, I don't see any commits of the actual proposal... |
8c53084
to
eab1252
Compare
Yes, that is right. I reverted some changes digging into gas stuff to see if I messed them up. I am pushing it back to a version that:
In both cases I still get the issue that: pregas := app.Simulate(tx)
pregas2 := app.Simulate(tx)
app.Check(tx)
pregas3 := app.Simulate(tx)
If I can't even fix this minor issue, I doubt I can get consistent simulation results. To reproduce, please check out this branch and do the following:
|
@ethanfrey thanks for digging into this. If you're not planning on further work on this can you close the PR, or note in the description that this is not being investigated further? |
Bez wanted to see what I got to. I'd be happy for him to close this pr after he takes a look (and maybe figures out some error in my test code that makes everything interact) |
Yes pls keep this PR around. I'd like to get to the bottom of this. As much as I hoped this would be an easy win for us, I just don't think we have the bandwidth to get this into launchpad. We obviously have some things we can do here, but just not for launchpad. Please keep the PR open, I'll review and test accordingly. |
I thought it would be easier too. I can rebase to master when master is stable, then keep this as an investigation for (post)stargate |
Description
This is a WIP - investigation to share.
Closes #4938
Findings
To reproduce:
I made a test in bank to simulate tx and submit it. The gas numbers from simulation were (very slightly) higher than the real gas numbers from deliver. I tried two tx and there was a clear difference due to the first having the public key and slightly larger txbytes.
However, I tried running:
simGas0 - simGas1 = 300 (txbytes)
resimGas0 - resimGas1 = 300 (txBytes)
simGas0 - resimGas0 = 4400
(why?)It seems like something being read a lot is slightly smaller (and why do we read it so much)?
It also seems like there is a large write that is not triggered after a CheckTx has run:
consumed 2000 gas for 'WriteFlat' --- consumed 2550 gas for 'WritePerByte'
.This is all a bit more confusing than the original issue suggests and I think writing the cache will not do anything (I did add a separate simulateState, that was parallel to checkState, but that didn't help - got some sequence errors when I removed a level of caching even.
Anyway... if someone else wants to wrack their brains....
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes