core/vm: set basefee to 0 internally on eth_call #28470

karalabe · 2023-11-06T13:40:04Z

After shipping 1559, we've added support for running eth_call (and it's derivatives) with a NoBaseFee vm.config flag. This is needed because people usually don't set gas prices on eth calls, so all the usual checks around maxfee > basefee, etc cannot pass. https://github.com/ethereum/go-ethereum/blob/master/core/state_transition.go#L293

One place we didn't handle is the basefee opcode when running the EVM. Currently that returns a non-zero number, even if NoBaseFee was requested. This poses an interesting issue:

tx.gasprice - block.basefee;

This above Solidity code reverts if ran with a gasprice of 0. That cannot happen for transaction execution, so the live network isn't impacted. And accessing the basefee in an eth_call seems pointless so people don't really hit it.

However, running a gas estimation on the above code will always fail if the user does not provide a gas price to the estimator. Whilst it really makes sense to provide a gas price (since tx execution might depend on it), if we allow 0 priced calls/estimations, then we must set the basefee to 0 too. Otherwise we create an impossible scenario (basefee > total fee), and all bets are off when the execution starts out with junk state.

FWIW, it would also make sense to consider if we want to disallow gas estimation with 0 price, since it feels wrong. My gut feeling is yes, but nobody knows what we're breaking (especially since ethclient itself fogot to forward the gas fields until this week).

holiman · 2023-11-06T15:00:44Z

core/vm/eips.go

+	if interpreter.evm.Config.NoBaseFee {
+		scope.Stack.push(new(uint256.Int))


Instead of checking for NoBaseFee in the production opcode, can't we just set the interpreter.evm.Context.BaseFee to a new(uint256.Int) in the NoBaseFee-situation?

I did consider that too. The only catch is that some code paths feed us the context directly, and we're not copying currently, so setting the basefee would modify the passed in context. We'd need to add some copying to avoid that.

Like this

diff --git a/core/vm/eips.go b/core/vm/eips.go index 44d4ba9cff..35f0a3f7c2 100644 --- a/core/vm/eips.go +++ b/core/vm/eips.go @@ -215,12 +215,8 @@ func opTstore(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]b // opBaseFee implements BASEFEE opcode func opBaseFee(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byte, error) { - if interpreter.evm.Config.NoBaseFee { - scope.Stack.push(new(uint256.Int)) - } else { - baseFee, _ := uint256.FromBig(interpreter.evm.Context.BaseFee) - scope.Stack.push(baseFee) - } + baseFee, _ := uint256.FromBig(interpreter.evm.Context.BaseFee) + scope.Stack.push(baseFee) return nil, nil } diff --git a/core/vm/evm.go b/core/vm/evm.go index 2c6cc7d484..1f84ff7660 100644 --- a/core/vm/evm.go +++ b/core/vm/evm.go @@ -133,6 +133,9 @@ func NewEVM(blockCtx BlockContext, txCtx TxContext, statedb StateDB, chainConfig chainConfig: chainConfig, chainRules: chainConfig.Rules(blockCtx.BlockNumber, blockCtx.Random != nil, blockCtx.Time), } + if config.NoBaseFee { + evm.Context.BaseFee = new(big.Int) + } evm.interpreter = NewEVMInterpreter(evm) return evm }

I did consider that too.

Ah I didn't see this comment. But I don't think you are quite correct -- the blockCtx is passed by value, not ref, in the path I used.

Yeah, I think Martin's approach is better, BaseFee is an environment context.

We can also leave a TODO for BlobBaseFee, we might face the same problem for this opcode too.

I'll switch to Martin's approach, but we need to be aware that BaseFee form the context is used in many locations, each potentially becoming a footgun with 0.

@rjl493456442 Wrt the blob fee, I think we can piggie-back on the same flag. BaseFee, BlobBaseFee, the concept is the same and setting it to zero should also be the same.

Ugh, no, Martin's fix isn't good (neither is mine).

Currently the logic is that we only disable base fee checks if 1) NoBaseFee is set and 2) gas prices are 0. Both our suggested fixes unilaterally nuke it out always, which is bad.

Martin's proposal cannot work because you don't know during construction time if tx will be run with or without gas price. My solution could work if I pulled the gas price up too, but it's a bit ugh. Maybe a cleaner solution is to set/rest the field before actually executing the bytecode for each tx individually. .Will look into that.

Maybe Martin's does work. The EVM is reconstructed for each tx.

MariusVanDerWijden · 2023-11-06T16:01:46Z

I thought the whole idea is that basefee is never nil.
In core/state_transition we say

// This will panic if baseFee is nil, but basefee presence is verified
// as part of header validation.

In my opinion the issue is not with the opcode then but with the way we construct the evm.Context without a valid basefee

edit: Ahh nvm, now I get it, thanks Martin

holiman · 2023-11-06T16:15:54Z

I thought the whole idea is that basefee is never nil.

Yeah the problem isn't that it's nil, it's non-nil (and quite large). So the problem is that basefee is larger than gasprice

holiman · 2023-11-07T10:05:10Z

core/state_transition.go

+			// Skip the checks if gas fields are zero and blobBaseFee was explicitly disabled (eth_call)
+			if !st.evm.Config.NoBaseFee && msg.BlobGasFeeCap.BitLen() > 0 {


The way this became changed now, the check is skipped if msg.BlobGasFeeCap == 0. And msg comes from the transaction we're about to execute. In other words, we omit a check depending on either

If a debug-setting is enabled, OR

A certain field in a transaction is zero.

It's not obvious to me why it is safe to do number 2)

Shit, good catch. Should have been an ||

holiman · 2023-11-07T10:39:19Z

core/state_transition.go

@@ -327,7 +327,7 @@ func (st *StateTransition) preCheck() error {
 	if st.evm.ChainConfig().IsCancun(st.evm.Context.BlockNumber, st.evm.Context.Time) {
 		if st.blobGasUsed() > 0 {
 			// Skip the checks if gas fields are zero and blobBaseFee was explicitly disabled (eth_call)
-			if !st.evm.Config.NoBaseFee && msg.BlobGasFeeCap.BitLen() > 0 {
+			if !st.evm.Config.NoBaseFee || msg.BlobGasFeeCap.BitLen() > 0 {


I had to, again, look at it for a while before my brain parsed it out fully. Yeah it's ok now, but I'd prefer having it rewritten for clarity:

// skip checks if basefee was disabled and blobBaseFee is zero (eth_call) skipCheck := st.evm.Config.NoBaseFee && msg.BlobGasFeeCap.BitLen() == 0 if !skipCheck {

Fair enough

Pushed this fix and also added it to the 1559 basefee

karalabe · 2023-11-07T13:58:40Z

I've removed the check for blobfee > 2^256. We don't need it because we use uint256 in blobtxs, so it cannot happen, apart from the last mile converstion to big.Int internally. We should start converting more things to uint256 as it would get rid of these cornercases.

holiman

LGTM

* core/vm: set basefee to 0 internally on eth_call * core: nicer 0-basefee, make it work for blob fees too * internal/ethapi: make tests a bit more complex * core: fix blob fee checker * core: make code a bit more readable * core: fix some test error strings * core/vm: Get rid of weird comment * core: dict wrong typo

)" This reverts commit 0a87754.

hkalodner · 2023-12-08T04:30:57Z

I wanted to note that this change broke gas estimation in our code when called without a gas price set for the following

 uint256 submissionFee = calculateRetryableSubmissionFee(data.length, block.basefee);

    function calculateRetryableSubmissionFee(uint256 dataLength, uint256 baseFee)
        public
        view
        override(AbsInbox, IInboxBase)
        returns (uint256)
    {
        // Use current block basefee if baseFee parameter is 0
        return (1400 + 6 * dataLength) * (baseFee == 0 ? block.basefee : baseFee);
    }

When the transaction is estimated, block.basefee now returns 0 which differs significantly from on-chain execution leading to incorrect estimation. We have some workarounds so this isn't a blocker for us, but it seems to generally make estimation execution differ more from on-chain execution than the previous behavior.

I'm wondering whether or not you considered resolving the tx.gasprice - block.basefee issue by setting tx.gasprice = block.basefee = current base fee instead of setting both equal to 0. That would solve the same issue without setting the base fee to 0, though could definitely be missing other factors.

As I mentioned we have a workaround, but this lead to some breakage so I thought I'd see if there was interest in alternative options.

s1na · 2023-12-08T06:46:26Z

by setting tx.gasprice = block.basefee = current base fee instead of setting both equal to 0

This will lead to bad UI. As accounts would need funds for fee so state overrides always have to be used.

However IF that's ok for you you can already set the gasPrice field of the transaction to the current block's base fee or higher and it should work.

A crucial note here is: block base fee is set to 0 only if tx.gasPrice is also 0.

karalabe · 2023-12-08T07:40:36Z

@hkalodner As @s1na also mentioned, you should really be doing gas pricing first and then estimation. Estimation needs good prices anyway and if the prices are non zero, then estimate will also use the real basefee.

karalabe · 2023-12-08T07:41:31Z

E.g. in our abigen generated code we do tx.gasprice = eth_gasPrice; and then do eth_estimateGas.

* core/vm: set basefee to 0 internally on eth_call * core: nicer 0-basefee, make it work for blob fees too * internal/ethapi: make tests a bit more complex * core: fix blob fee checker * core: make code a bit more readable * core: fix some test error strings * core/vm: Get rid of weird comment * core: dict wrong typo

core/vm: set basefee to 0 internally on eth_call

849c0d4

karalabe added this to the 1.13.5 milestone Nov 6, 2023

karalabe requested review from holiman and rjl493456442 as code owners November 6, 2023 13:40

holiman reviewed Nov 6, 2023

View reviewed changes

karalabe added 2 commits November 7, 2023 11:55

core: nicer 0-basefee, make it work for blob fees too

de24a7a

internal/ethapi: make tests a bit more complex

5f39342

holiman reviewed Nov 7, 2023

View reviewed changes

core: fix blob fee checker

9da87f8

holiman reviewed Nov 7, 2023

View reviewed changes

karalabe added 2 commits November 7, 2023 12:42

core: make code a bit more readable

2cd4ccf

core: fix some test error strings

7aff365

karalabe added 2 commits November 7, 2023 16:14

core/vm: Get rid of weird comment

4544026

core: dict wrong typo

ff2d1d7

marioevz mentioned this pull request Nov 7, 2023

Zero max_fee_per_blob_gas test is ineffective ethereum/execution-spec-tests#343

Closed

holiman approved these changes Nov 8, 2023

View reviewed changes

karalabe merged commit 470dba8 into ethereum:master Nov 8, 2023
2 checks passed

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "core/vm: set basefee to 0 internally on eth_call (ethereum#28470

ebad970

)" This reverts commit 0a87754.

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "core/vm: set basefee to 0 internally on eth_call (ethereum#28470

576266c

)" This reverts commit 0a87754.

BrewTestBot mentioned this pull request Nov 14, 2023

ethereum 1.13.5 Homebrew/homebrew-core#154261

Merged

roberto-bayardo mentioned this pull request Dec 12, 2023

fix swapped fee/tip parameters to estimateGas ethereum-optimism/optimism#8572

Merged

gzeoneth mentioned this pull request Dec 14, 2023

fix: gas estimation issue OffchainLabs/nitro-contracts#101

Closed

nksazonov mentioned this pull request Mar 12, 2024

Gas limit estimation is broken by go-ethereum@1.13.5 pimlicolabs/alto#160

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/vm: set basefee to 0 internally on eth_call #28470

core/vm: set basefee to 0 internally on eth_call #28470

karalabe commented Nov 6, 2023

holiman Nov 6, 2023

karalabe Nov 6, 2023

holiman Nov 6, 2023

holiman Nov 6, 2023

rjl493456442 Nov 7, 2023

karalabe Nov 7, 2023

karalabe Nov 7, 2023

karalabe Nov 7, 2023

karalabe Nov 7, 2023

MariusVanDerWijden commented Nov 6, 2023 •

edited

Loading

holiman commented Nov 6, 2023

holiman Nov 7, 2023

karalabe Nov 7, 2023

holiman Nov 7, 2023 •

edited

Loading

karalabe Nov 7, 2023

karalabe Nov 7, 2023

karalabe commented Nov 7, 2023

holiman left a comment

hkalodner commented Dec 8, 2023

s1na commented Dec 8, 2023

karalabe commented Dec 8, 2023

karalabe commented Dec 8, 2023

		if interpreter.evm.Config.NoBaseFee {
		scope.Stack.push(new(uint256.Int))

		// Skip the checks if gas fields are zero and blobBaseFee was explicitly disabled (eth_call)
		if !st.evm.Config.NoBaseFee && msg.BlobGasFeeCap.BitLen() > 0 {

core/vm: set basefee to 0 internally on eth_call #28470

core/vm: set basefee to 0 internally on eth_call #28470

Conversation

karalabe commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariusVanDerWijden commented Nov 6, 2023 • edited Loading

holiman commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karalabe commented Nov 7, 2023

holiman left a comment

Choose a reason for hiding this comment

hkalodner commented Dec 8, 2023

s1na commented Dec 8, 2023

karalabe commented Dec 8, 2023

karalabe commented Dec 8, 2023

MariusVanDerWijden commented Nov 6, 2023 •

edited

Loading

holiman Nov 7, 2023 •

edited

Loading