Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: balanceCheck should always include msg.Value #29762

Merged
merged 1 commit into from
May 15, 2024

Conversation

zhiqiangxu
Copy link
Contributor

@zhiqiangxu zhiqiangxu commented May 13, 2024

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
Copy link
Contributor

holiman commented May 13, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

holiman commented May 13, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

holiman commented May 13, 2024

BTW, when will msg.GasFeeCap be nil?

On legacy-transactions which has the old style gas pricing

@zhiqiangxu
Copy link
Contributor Author

zhiqiangxu commented May 13, 2024

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
Copy link
Contributor

holiman commented May 13, 2024

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

@zhiqiangxu
Copy link
Contributor Author

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
Copy link
Contributor

fjl commented May 14, 2024

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.

core/state_transition.go Outdated Show resolved Hide resolved
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fjl fjl merged commit 7ed52c9 into ethereum:master May 15, 2024
4 checks passed
@fjl fjl added this to the 1.14.4 milestone May 15, 2024
@fjl fjl removed the status:triage label May 15, 2024
jorgemmsilva pushed a commit to iotaledger/go-ethereum that referenced this pull request Jun 17, 2024
It's a bit confusing to add msg.value into the balanceCheck within the conditional.
No impact on block validation since GasFeeCap is always set when processing transactions.
stwiname pushed a commit to subquery/data-node-go-ethereum that referenced this pull request Sep 9, 2024
It's a bit confusing to add msg.value into the balanceCheck within the conditional.
No impact on block validation since GasFeeCap is always set when processing transactions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants