core: balanceCheck should always include msg.Value

[zhiqiangxu]

Currently balanceCheck only includes msg.Value when msg.GasFeeCap!=nil.

I think balanceCheck should always include msg.Value no matter whether msg.GasPrice or msg.GasFeeCap is chosen as the effective gas price.

[holiman]

During TransitionDb, we do check that the value transfer can happen,

	if !value.IsZero() && !st.evm.Context.CanTransfer(st.state, msg.From, value) {
		return nil, fmt.Errorf("%w: address %v", ErrInsufficientFundsForTransfer, msg.From.Hex())
	}

However, the balancecheck is a bit special. We have two "prices" for gas, the gas fee cap and gas tip cap. Now, it's consensus-decided that the sender must be able to cover the highest of those two. So if the sender says he's ready to pay a large tip (but in the end doesn't have to), he still needs to have the funds to cover for that.

So the balanceCheck is simply the checking of this claim. We check "can sender cover both msg value and the highest promised tip price?".

So the change you are proposing is not wrong per se, but afaict it is not required either.

[zhiqiangxu]

I think the semantic should be that: the user balance should be enough to cover both msg.value and gas price x gas limit?

[holiman]

I think the semantic should be that: the user balance should be enough to cover both msg.value and gas price x gas limit?

Yes.

But there are two cases:

• The balance-check,
• The actual charging of money from the account.

In the simple case, we can just charge money and everything is fine. It works or it does not.

In the complicated case, we need to check (but not charge) a higher amount first.

[zhiqiangxu]

Yes there're two cases, and balanceCheck corresponds the balance-check case, so I think it should include msg.value.

BTW, when will msg.GasFeeCap be nil?

[holiman]

BTW, when will msg.GasFeeCap be nil?

On legacy-transactions which has the old style gas pricing

[zhiqiangxu]

It seems for legacy tx, GasFeeCap is just an alias of GasPrice: source. So it won't be nil as long as GasPrice isn't nil.

[holiman]

Right, might be only via eth_call, func (args *TransactionArgs) ToMessage(baseFee *big.Int) *core.Message that leaves the field as nil.

[zhiqiangxu]

It seems it's only possible to be nil when it goes to this branch, but in that case, GasPrice will be nil as well.

[fjl]

I think the change here is correct. The code just looks very odd as-written, with msg.Value only being deducted when GasFeeCap is non-nil. The case where it's nil doesn't happen in practice during block execution.

[holiman]

LGTM