Fix estimate gas for old runtimes

[librelois]

This PR #1257 introduced a regression that make the client not compatible with old runtimes.

Old runtimes doesn't check properly non transactional evm calls, old runtimes rely on the fact that max_fee_per_gas is None to consider the evm call non transactional.

It introduced a bug in Moonbeam where eth_call and estimate_gas against past blocks produce the BalanceLow error.
Because we can't change old runtimes, the client should not set max_fee_per_gas as Some(zero), otherwize old runtimes will assume that the evm call is transactional.

I submitted a PR upstream (#1404) to fix it for eth_call, but we have the same bug with estimate_gas, this PR move the fix into the fee_details function itself.

[boundless-forest]

The current fee_details in the estimate_gas function is problematic in the case of absence of gas_price or request_max_fee, request_priority in the estimate gas call request. If the estimate call request doesn't include the price fields, it will pass None to the runtime call, leading to a mismatch TransactionData proof_size_base(less), and going to the problem 1 above. If the proof size is plays a key role in the transaction gas estimate, it's problematic. The estimated gas is less than the actual execution gas. I fixed this by give U256::zero instead of None in the fee_details.

I rethink this issue again and this is not a totally correct patch. It will break the current runtime functionality. See the comment I wrote in the previous PR. Changing max_fee_per_gas to None indeed solves the old runtime eth_call and estimate_gas issue, but also breaks the latest runtime functionality. This lead to the eth_call or estimate_gas proof_size not match the actual transaction, futher then cause transaction failure in some cases(proof_size nominate the final gas result).

Because of this, I submited #1412 to revert your previous PR.

The current final gas is the max(evm gas, proof_size gas). This makes the things much more complicated and brings more Ethereum compatible issues. I've received several times that the contract developer complains that the contract gas used sometimes is weird, different from the Ethereum, sometimes it's the same. Take the proof_size cost into the fee is the design of the polkadot sdk parachain to prevent the abuse of parachain POV size. It makes sense. But for the Frontier, the Ethereum compatibility is a high priority that we should consider. To prevent the POV size abuse or attack, a better way is to restrict the single transaction proof_size. Reject or charge the extra fee for the transactions with proof_size over the limit, which can ensure that most of the frontier transactions keep the same behaviour as the Ethereum.

[librelois]

but also breaks the latest runtime functionality. This lead to the eth_call or estimate_gas proof_size not match the actual transaction, further then cause transaction failure in some cases(proof_size nominate the final gas result).

It's better to overestimate a bit the proof size on estimate_gas than to fail with BalanceTooLow error.

Also, can you point to the piece of code that compute the proof size badly in that case? Because it's not part of frontier
codebase, on moonbeam we alwqays account a little bit more for estimate or offchain calls, and it not problematic in practice: https://github.com/moonbeam-foundation/moonbeam/blob/perm-runtime-2801/runtime/common/src/apis.rs#L274-L289

What you should have done in your original PR is to create a new version of the runtime API so that the client can adapt to each runtime. You can do that on a new PR, but for runtimes published in the meantime, I don't see an ideal solution. Do you have an idea?
In the absence of an ideal solution, I think it's preferable to slightly overestimate the proof size rather than return a BalanceTooLow error.

[boundless-forest]

Also, can you point to the piece of code that compute the proof size badly in that case?

• proof_size calculate in the call api:

  frontier/template/runtime/src/lib.rs

  Line 803 in e7fccf9

  let transaction_data = TransactionData::new(

• ethereum actual execute:

  frontier/frame/ethereum/src/lib.rs

  Line 722 in e7fccf9

  let transaction_data: TransactionData = transaction.into();

• how the TransactionData calculate the proof_size_base_cost

  frontier/primitives/ethereum/src/lib.rs

  Line 81 in e7fccf9

  pub fn proof_size_base_cost(&self) -> u64 {

Pass Some(0) and None, the calculated proof_size_base_cost is slightly different.

What you should have done in your original PR is to create a new version of the runtime API so that the client can adapt to each runtime. You can do that on a new PR, but for runtimes published in the meantime, I don't see an ideal solution. Do you have an idea?

Yes, that's the way to do it. I didn't realise it would break this at that time. No ideas either.

[boundless-forest]

In the absence of an ideal solution, I think it's preferable to slightly overestimate the proof size rather than return a BalanceTooLow error.

I agree with that. Would you mind picking the Moonbeam runtime call estimated_transaction_len to the frontier template node runtime in this PR? This ensures that the proof_size in estimate_gas is always greater than the actual usage, and that no transaction fails due to out of gas. Change this in the template runtime to allow frontier users to use correct behaviour.

[librelois]

Would you mind picking the Moonbeam runtime call estimated_transaction_len to the frontier template node runtime in this PR?

Done

[boundless-forest]

Sorry for the delay in replying(just come back from holiday). I come up with another idea is that we can pass default value to the TransactionData::new(xxx) for these field with Option<T> type, such as max_fee_per_gas and max_priority_fee_per_gas, instead of None. This way can also meet our needs and looks cleaner. What do you think? @librelois