escape inifinite loop in estimte_gas

[miyao-gmo]

#7074

[parity-cla-bot]

It looks like @miyao-gmo hasn'signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

[miyao-gmo]

[clabot:check]

[parity-cla-bot]

It looks like @miyao-gmo signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[rphmeier]

I don't think this configuration belongs in the chain spec. It's also not really an infinite loop, just one bounded at an arbitrary 1 Trillion gas. We could reasonably set the upper limit to block_gas_limit * 100 or something like that without requiring additional configuration.

[miyao-gmo]

I see.
I agree that upper limit is changed from 1 Trillion gas.
However, I think block_gas_limit * 100 is too large value, so block_gas_limit * 2 is enough.

[rphmeier]

I think it'd be better if we had the upper limit still significantly greater than the block gas limit.

[miyao-gmo]

OK.
I changed PR as you say.

[tomusdrw]

The loggic is quite different now.

Previously:

1. Start with block_gas_limit
2. If that's not enough proceed with UPPER_CEILING.

Currently:

1. Start with block_gas_limit * 100

I think we should keep the starting point at block_gas_limit, since it will converge much faster for valid transactions and fallback to 100 * limit only if the first fails.

[miyao-gmo]

I think a execution time in estimate_gas of a valid transaction with block_gas_limit is a same with block_gas_limit * 100, therefore, I changed to one phase estimate_gas from two phases.

I compared a execution time with block_gas_limit and block_gas_limit * 100.

> var calcValid = function() {
... var start = new Date().getTime();
... sample.validTx.estimateGas(function (err, gas) {
..... var end = new Date().getTime();
..... console.log(end - start);
..... });
... }

The following result is a execution time with block_gas_limit.

> calcValid()
undefined
> 75

The following is with block_gas_limit * 100.

> calcValid()
undefined
> 80

These result shows a execution time in estimate_gas of a valid transaction with block_gas_limit is a same with block_gas_limit * 100.

[tomusdrw]

This time will depend on complexity of the transaction (CPU, IO). You can see that there is even a special case for transfer transactions (they will return immediately, by checking the lower bound first). I'm pretty positive that block_gas_limit will be faster by not running the transaction at least 6 times upfront. So if each transaction run takes 50ms it's a gain of 300ms which is huge.

[miyao-gmo]

I understood.
I will change PR as you say.

By the way, I think block_gas_limit * 100 is so large, but what do you think?

[tomusdrw]

It's pretty big, but afair users requested that estimate gas should work way over block gas limit. It's a significant improvement (in terms of time to fail) over what we have now, so I'm in favour of including this PR (or block_gas_limit * 10) for 1.9 and probably re-visiting it if we see that it still takes too much time for users.

[miyao-gmo]

OK, thank you.
I fixed.

[tomusdrw]

Adding patch label, since we should also "fix" this in stable and beta.

[tomusdrw]

@miyao-gmo Thank you!