improve weight per gas calculation

[koushiro]

related PR: moonbeam-foundation/moonbeam#2075

[boundless-forest]

How did you get this BLOCK_GAS_LIMIT? There seems to be a dead loop in your solution. When you calculate the BLOCK_GAS_LIMIT, you also need WeightPerGas.

[koushiro]

actually, you need to know BLOCK_GAS_LIMIT firstly, then calculate the weight per gas.
if not that, how did you get the original WEIGHT_PER_GAS?

[boundless-forest]

I remembered that the const value was estimated by the benchmark.
https://github.com/paritytech/frontier/blob/master/frame/evm/src/benchmarking.rs

[sorpaas]

It's just reversed order -- previously we get block gas limit from weight per gas. Now we get weight per gas from block gas limit.

Both are equivalent, but I'd honestly like the new approach slightly more. The thing is -- one can get a much more accurate estimate of the overall block gas limit through benchmarking (because it's aggregated), than weight per gas, and thus fixing the former would be better.

[boundless-forest]

I'm working on #994 , an issue that has some relevance to the current pr.

previously we get block gas limit from weight per gas.

block_gas_limit was previously calculated from block_max_weight and weight_per_gas. block_max_weight is known in runtime and weight_per_gas can be obtained from pallet_evm's benchmark.

block_gas_limit = block_max_weight / weight_per_gas.

one can get a much more accurate estimate of the overall block gas limit through benchmarking (because it's aggregated)

Can you go a bit deeper into your idea? @sorpaas

Benchmark the maximum value of transaction gas that a block of fixed weight size can hold? That doesn't seem to work, the Ethereum::transact() call's weight calculation requires weight_per_gas, and the weigth_per_gas calculation requires block_gas_limit, which is like a loop.

[sorpaas]

I just mean they are equivalent. When you know block_gas_limit, you know weight_per_gas, and vice versa. The formula is always fixed:

• block_gas_limit = block_max_weight / weight_per_gas
• weight_per_gas = block_max_weight / block_gas_limit

We just need to get any of the value. Previously we get weight_per_gas first and then calculate block_gas_limit. After this PR we get block_gas_limit first and then calculate weight_per_gas.

I don't see any loops -- we use pallet_evm::Config::WeightPerGas for transaction / execution related gas calculation through the WeightGasMapping. pallet_evm::Config::BlockGasLimit is not used for them, and instead is only used for populating the Ethereum block gas limit. However, indeed, the config value BlockGasLimit should probably belong to pallet_ethereum instead of pallet_evm.

Regarding benchmarking, the normal benchmark process for Substrate is different than benchmarking BlockGasLimit/WeightPerGas. Substrate has strict definition of what weight is (10^12 Weight as 1 second of computation on the physical machine used for benchmarking), and the whole Substrate benchmarking process is based on that. However, for BlockGasLimit/WeightPerGas, the process is different -- we have to figure out the maximum allowable value through a trial and error process.

• Give an initial block gas limit / weight per gas, execute a series of test blocks, and see if it can finishes in target time.
• If it finishes significantly faster, increase the value, otherwise, decrease the value, and then repeat.

This is why I say the new method (fixing BlockGasLimit instead of WeightPerGas) is better. Technically the old and new method will get the same result (because it's equivalent). However, using BlockGasLimit is aggregated, and more intuitive because that's the actual value that we're comparing against in benchmarking.