Use gas price in place of effective gas price for initial balance check

[gd-0]

The prelimary balance (LackOfFundForGasLimit) check revm does is different to Geth

revm checks: effective_price * gas + value >= caller_balance

revm/crates/revm/src/evm_impl.rs

Line 115 in 2e4e800

.checked_mul(effective_gas_price)

geth checks: gas_fee_cap * gas + value >= caller_balance.
https://github.com/ethereum/go-ethereum/blob/3a79a99f804c4bd2002fe52768c965c4be8106ea/core/state_transition.go#L221

This change should align the revm check with Geth.

[gakonst]

ah wow good catch, "ethtests/GeneralStateTests/stEIP2930/coinbaseT2.json" failed: Test:LONDON:0, Root missmatched, Expected: 0x1c8f398c6a243ffec2e8f63ef816e48c3af0fbbafc875837c75bb78b555f40aa got:0xe6ef536faafc284934886eb803b318d7b6cf41f9e31f7a76ce5f9a9e1fd45503 is this related?

[gd-0]

ah wow good catch, "ethtests/GeneralStateTests/stEIP2930/coinbaseT2.json" failed: Test:LONDON:0, Root missmatched, Expected: 0x1c8f398c6a243ffec2e8f63ef816e48c3af0fbbafc875837c75bb78b555f40aa got:0xe6ef536faafc284934886eb803b318d7b6cf41f9e31f7a76ce5f9a9e1fd45503 is this related?

I think it might be actually. It looks like Geth checks the fee_cap but only subtracts the effective_price*gas from the acc balance: https://github.com/ethereum/go-ethereum/blob/3a79a99f804c4bd2002fe52768c965c4be8106ea/core/state_transition.go#L233
The change I implemented means that the revm will subtract fee_cap*gas from the account.

<- Ignore this. st.gasPrice() isn't the effective price.

[rakita]

For history:

Will check failing test

[rakita]

@gd-0 additionally this needs to be changed:

revm/crates/revm/src/evm_impl.rs

Line 266 in 2e4e800

let effective_gas_price = self.data.env.effective_gas_price();

we need to use gas_price

And after gas_price is used this needs to be changed to continue having the same functionality:

revm/crates/revm/src/evm_impl.rs

Line 289 in 2e4e800

effective_gas_price.saturating_sub(basefee)

to have effective_gas_price - base_fee deduction.

[gd-0]

@rakita I've implemented the change but looks like the tests are still failing. Maybe I misunderstood what you meant?

[rakita]

@rakita I've implemented the change but looks like the tests are still failing. Maybe I misunderstood what you meant?

I said the incorrect thing, didn't take into account that, in finalization, we need to return the difference of (gas_price-effective_price)*gas_limit

Suggested change

	.saturating_add(self.data.env.tx.gas_price * U256::from(gas.remaining() + gas_refunded));
	.saturating_add((self.data.env.tx.gas_price-effective_gas_price) * self.data.env.tx.gas_limit)
	.saturating_add(effective_gas_price * U256::from(gas.remaining() + gas_refunded));

[gd-0]

I'm still not quite sure what you mean. I tried applying this change and the tests are still failing.
Also when checking Geth it looks like gasPrice is used? https://github.com/ethereum/go-ethereum/blob/3a79a99f804c4bd2002fe52768c965c4be8106ea/core/state_transition.go#L400

[rakita]

At the start:
we have done it as: balance -= min(base_fee+tip,max_gas_price)*gas_limit => balance -= effective*gas_limit
and now we are doing: balance -= max_gas_price * gas_limit

In finalize we need to return it back:
previously it was: balance += effective*(remaining_gas+gas_refund)
but now it needs to take into account additional gas that we subbed, and it would become:
balance +=(gas_price-effective_price)*gas_limit+effective*(remaining-refunded)

it can be done as you did below, as do this only for the balance check. Not sure why it panics, will need to look at at detail

[gd-0]

aha gotcha. That makes sense, thanks :D

Geth does it the way I did it below by only subtracting the effective price from the account: https://github.com/ethereum/go-ethereum/blob/0c9eb8c9a47285526078beef4ab5aed5a90ab938/core/state_transition.go#L217 so this seems like the best way to go imo? *note st.gasPrice here is effective_gas_price: https://github.com/ethereum/go-ethereum/blob/0c9eb8c9a47285526078beef4ab5aed5a90ab938/core/types/transaction.go#L633

[rakita]

yeah, I agree it is better to do as your latest, it is best to be aligned with geth with this kind of sensitive things.

[gd-0]

All tests are passing now! The test was failing because we should only use gas_price for the balance check, the amount that is actually subbed from the account is the effective_price*gas_limit.

The only other thing that I noticed was that it looks like the value transfer is only included in the gas checks if the tx type is 2: https://github.com/ethereum/go-ethereum/blob/3a79a99f804c4bd2002fe52768c965c4be8106ea/core/state_transition.go#L222

To fix this I adapted the code to this:

        let mut balance_check = U256::from(gas_limit)
            .checked_mul(self.data.env.tx.gas_price)
            .ok_or(EVMError::Transaction(
                InvalidTransaction::OverflowPaymentInTransaction,
            ))?;

        if self.data.env.tx.gas_priority_fee.is_some() {
            balance_check += value;
        }

        // Check if account has enough balance for gas_limit*gas_price and value transfer.
        // Transfer will be done inside `*_inner` functions.
        if balance_check > *caller_balance && !disable_balance_check {
            return Err(InvalidTransaction::LackOfFundForGasLimit.into());
        }

But now the tests are failing with this error:

thread '<unnamed>' panicked at 'Internal return flags should remain internal OutOfFund', /revm/crates/revm/src/evm_impl.rs:230:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Maybe I am interpreting this wrong? What do you guys think?