Ethanfrey/4938 better gas estimation

[ethanfrey]

Description

This is a WIP - investigation to share.

Closes #4938

Findings

To reproduce:

cd x/bank
go test -v . -run TestSendGas

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 := Simulate(tx)
simGas1 := Simulate(tx2)

Check(tx)
Check(tx2)

resimGas0 := Simulate(tx)
resimGas1 := SImulate(tx2)

simGas0 - simGas1 = 300 (txbytes)

resimGas0 - resimGas1 = 300 (txBytes)

simGas0 - resimGas0 = 4400 (why?)

< --- consumed 135 gas for 'ReadPerByte' ---
< --- consumed 2000 gas for 'WriteFlat' ---
< --- consumed 2550 gas for 'WritePerByte' ---
---
> --- consumed 261 gas for 'ReadPerByte' ---
39c37
< --- consumed 255 gas for 'ReadPerByte' ---
---
> --- consumed 261 gas for 'ReadPerByte' ---
51c49
< --- consumed 255 gas for 'ReadPerByte' ---
---
> --- consumed 261 gas for 'ReadPerByte' ---
54c52
< --- consumed 255 gas for 'ReadPerByte' ---
---
> --- consumed 261 gas for 'ReadPerByte' ---
56c54
< --- consumed 255 gas for 'ReadPerByte' ---
---
> --- consumed 261 gas for 'ReadPerByte' ---
77c75
< Sim 0 used: 55951
---
> Re-Sim 0 (after check) used: 51551

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.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[ethanfrey]

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 ******************* NEW LIMITED ***************** with all the *** is actually related to the transaction delivery. That is still a lot. And everything else going on in this block???

[alexanderbez]

Thanks for looking into this @ethanfrey.

Trying to grok this, and correct me if I'm wrong, simulating (with the current code) tx after CheckTx yields a significantly smaller estimation compared to before CheckTx (diff of 4400)? The 4400 diff here comes from a WriteFlat and a WritePerByte that occurred before CheckTx, correct? In other words, the large discrepancy comes from the fact that we're not writing something after CheckTx has happened?

AFAICT, simulate (via query) and CheckTx run nearly identical execution paths and actually share the same state (checkState). My guess would be that executing simulate (via query) yields some writes to checkState, which are then cached, so that on the subsequent CheckTx, there isn't a need to write something. This is the only thing I can think of...

But my original proposal was to use a separate (third) state object, queryState where I believe you'd end up writing, so the gas estimation should be near the same if not identical?

[alexanderbez]

Also, I don't see any commits of the actual proposal...

[ethanfrey]

Trying to grok this, and correct me if I'm wrong, simulating (with the current code) tx after CheckTx yields a significantly smaller estimation compared to before CheckTx (diff of 4400)? The 4400 diff here comes from a WriteFlat and a WritePerByte that occurred before CheckTx, correct? In other words, the large discrepancy comes from the fact that we're not writing something after CheckTx has happened?

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:

1. eab1252 - uses a separate simulate state to avoid CheckTx interference
2. f74ab1c - implements the proposed fix (msCache.Write() for runTxSimulate as well)

In both cases I still get the issue that:

pregas := app.Simulate(tx)
pregas2 := app.Simulate(tx)
app.Check(tx)
pregas3 := app.Simulate(tx)

pregas == pregas2 (good) but pregas != pregas3 (not so cool)

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:

go test ./x/bank -v -count 1 -run TestSendGas

[clevinson]

@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?

[ethanfrey]

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)

[alexanderbez]

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.

[ethanfrey]

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