Fix bug estimate used gas and estimation for WeightV2

[tgmichel]

Solves #1100

Some changes that we included on Moonbeam were missing upstream:

• Currently pallet-ethereum PostDispatchInfo included the legacy/standard used gas but the block includes the effective gas data. This is an issue when producing blocks, as we hint the block builder way more available resources left than actually are.
• Gas estimation using binary search need to be updated to WeightV2 (basically the ethereum call runtime api).

[arturgontijo]

Just tested this one and it indeed fixes #1100, thanks!

Let me try to add a test to it so we avoid missing that in the future.

[sorpaas]

LGTM. Just a grumble.

[arturgontijo]

Hey @tgmichel I'm have hard time adding a test for this one =/

What I've done so far was to add this one to the frame/ethereum/tests/legacy.rs (but my idea is to create a separated file to include the other Transactions types too):

#[test]
fn validated_block_gas_limit_works() {
	let (pairs, mut ext) = new_test_ext_with_initial_balance(1, 100_000_000_000);
	let alice = &pairs[0];

	let mut transaction = legacy_erc20_creation_unsigned_transaction();

	ext.execute_with(|| {

		System::set_block_number(1);

		// Probe transaction, to get its real gas_used.
		let signed = transaction.sign(&alice.private_key);
		assert_ok!(crate::ValidatedTransaction::<Test>::apply(alice.address, signed));

		Ethereum::on_finalize(System::block_number());

		let block = <CurrentBlock<Test>>::get().unwrap();
		let mut next_block_gas_used = U256::zero();

		System::set_block_number(2);

		// Being sure that we send sufficient transactions to surpass block_gas_limit.
		while next_block_gas_used <= (block.header.gas_limit + block.header.gas_used) {
			let signed = transaction.sign(&alice.private_key);
			assert_ok!(crate::ValidatedTransaction::<Test>::apply(alice.address, signed));
			transaction.nonce += U256::one();
			next_block_gas_used += block.header.gas_used;
		}

		Ethereum::on_finalize(System::block_number());

		let block = <CurrentBlock<Test>>::get().unwrap();
		assert_eq!(block.header.gas_used, block.header.gas_limit);
	});
}

I left the assert_eq!(...) so it can outputs the values for inspection:

---- tests::legacy::validated_block_gas_limit_works stdout ----
thread 'tests::legacy::validated_block_gas_limit_works' panicked at 'assertion failed: `(left == right)`
  left: `151119462`,
 right: `150000000`', frame/ethereum/src/tests/legacy.rs:549:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

As Ethereum::store_block() (called by Ethereum::on_finalize()) is not checking for cumulative_gas <= block_gas_limit I think that is done one step earlier, by the pool logic maybe? Am I right on this one?

If so I'll move the test to somewhere else, maybe adding it to primitives/evm/src/validation.rs.

[tgmichel]

@arturgontijo thank you, I thought you already had a working test sorry :|

IMO if we want to verify that configured block gas limit works as expected with WeightV2 we need to test two type of transactions: ref time heavy, and proof size heavy, so in either case we make sure we account the right UsedGas component. I think this types of tests are easier on typescript.

[arturgontijo]

@tgmichel no man, still studying the whole code base.

Regarding the TS test, I see that we have a TODO that could be exactly what we need:
https://github.com/paritytech/frontier/blob/master/ts-tests/tests/test-gas.ts#L75

Do you mind writing the test(s) for it? I think you would do way way faster than me, avoiding this being blocked. =)

[tgmichel]

Tests pass locally but fail on github machine, caused by the MAX_PROPOSAL_DURATION harcoded value to 10 seconds in substrate (which IMO makes no sense, to hardcode the value that is):
https://github.com/paritytech/substrate/blob/ebe8dcd3be6583faea1ca0b38441a39e2e02ce23/client/consensus/manual-seal/src/seal_block.rs#L127

10 seconds is not enough in our case, where we need to process 75M gas worth of transactions in the CI. Deadline is reached while applying the transaction pool iterator, and an arbitrary amount of transactions are included instead reaching the configured limit.

[arturgontijo]

Does heavier transactions help here?
I mean, transactions that spend more than 21_000 gas?

Something like this:

    mapping(address => uint) public map;
    
    // n=1      30k
    // n=10     37k
    // n=100    100k
    // n=250    205k
    // n=500    380k
    // n=1000   745k
    function storageLoop(
        uint16 n,
        address _to,
        uint _amount
    ) public {
        for (uint16 i = 0; i < n; i++) {
            map[_to] += _amount;
        }
    }

[AurevoirXavier]

When can we merge this? I hope it can be included in the polkadot-v0.9.43 branch. :)

[sorpaas]

There are issues with tests which need to be fixed first.

[tgmichel]

I will work on fixing the tests today

[koushiro]

@sorpaas we should cherry-pick this PR to polkadot-v0.9.43 branch.

[bernardoaraujor]

@sorpaas we should cherry-pick this PR to polkadot-v0.9.43 branch.

also to polkadot-v0.9.42 branch:

• bring PR #1101 to polkadot-v0.9.42 release branch #1113
• bring PR #1101 to polkadot-v0.9.43 release branch #1114