-
Notifications
You must be signed in to change notification settings - Fork 1.7k
escape inifinite loop in estimte_gas #7075
escape inifinite loop in estimte_gas #7075
Conversation
It looks like @miyao-gmo hasn'signed our Contributor License Agreement, yet.
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 Many thanks, Parity Technologies CLA Bot |
[clabot:check] |
It looks like @miyao-gmo signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
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 |
I see. |
I think it'd be better if we had the upper limit still significantly greater than the block gas limit. |
OK. |
ethcore/src/client/client.rs
Outdated
let initial_upper = env_info.gas_limit; | ||
env_info.gas_limit = UPPER_CEILING.into(); | ||
(initial_upper, env_info) | ||
let upper = env_info.gas_limit * U256::from(100); // enough gas for estimate_gas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loggic is quite different now.
Previously:
- Start with
block_gas_limit
- If that's not enough proceed with
UPPER_CEILING
.
Currently:
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thank you.
I fixed.
Adding patch label, since we should also "fix" this in stable and beta. |
@miyao-gmo Thank you! |
escape inifinite loop in estimte_gas
escape inifinite loop in estimte_gas
* Merge pull request #7075 from miyao-gmo/feature/estimate_gas_limit escape inifinite loop in estimte_gas * Merge pull request #7006 from paritytech/no-uncles Disable uncles by default * Maximum uncle count transition (#7196) * Enable delayed maximum_uncle_count activation. * Fix tests. * Defer kovan HF. * Bump version. * Kovan HF. * Update Kovan HF block. * Fix compilation issues. * Fix aura test. * Add missing byzantium builtins. * Fix tests. * Bump version for installers. * Increase allowed time drift to 10s. (#7238)
#7074