go-ethereum icon indicating copy to clipboard operation
go-ethereum copied to clipboard

`eth_createAccessList` fails when using block_hash and not setting txn fees

Open DragonDev1906 opened this issue 1 year ago • 10 comments

System information

Geth version: current master (also happens in releases) Commit hash : cf0378499f1bcae65c093c58cd6ca8225e91b125

Expected behaviour

eth_createAccessList to succeed when given a block hash (that wasn't pruned) and a basic transaction (only to and input) that does not revert.

Actual behaviour

Returns the following error:

code: -32000
message: "failed to apply transaction: 0x3e983326c3827cf62805f3dab45de2d23ce3c839310d823feac120f538a1a286 err: max fee per gas less than block base fee: address 0x0000000000000000000000000000000000000000, maxFeePerGas: 27907986728, baseFee: 28235336045", data: None

Note that this does not always happen:

Relevant files

  • The call to set_defaults that doesn't get the block that was requested: https://github.com/ethereum/go-ethereum/blob/bcaf3747f8443c6f8042f4f058f74404bb8c0a22/internal/ethapi/api.go#L1506-L1515
  • The part of the implementation that sets this: https://github.com/ethereum/go-ethereum/blob/bcaf3747f8443c6f8042f4f058f74404bb8c0a22/internal/ethapi/transaction_args.go#L186-L187
  • Where the field is set: https://github.com/ethereum/go-ethereum/blob/bcaf3747f8443c6f8042f4f058f74404bb8c0a22/internal/ethapi/transaction_args.go#L268-L272

The cause of this bug:

When requesting the access list for anything but the most recent block and not specifying fee details, Go-Ethereum automatically adds the fee values using the most recent block (rather than the specified one). When the block base-fee changed durin gthat time the resulting maxFeePerGas can (and does often enough to be a problem) be below the baseFee of the block we're actually executing against, thus causing this error.

Additional Information

Potentially related (but I think it's a different issue/cause): https://github.com/ethereum/go-ethereum/issues/25319

DragonDev1906 avatar Jul 11 '24 14:07 DragonDev1906

This issue has been preview on the Pull Requests. To resolve, Contact the Ethereum support page to initiate a chat with the live agent on the chat button to get more information about your request via ETH Support Thanks for posting @DragonDev1906

Before someone clicks that link:

  • It does look like spam: https://supportethofficial.web.app/
  • The GTHubDevi account that posted this was created yesterday

DragonDev1906 avatar Jul 12 '24 06:07 DragonDev1906

I check this issue, and find out you catch well the reason why the error (you issued) occurred. It is because the gasPrice is less than baseFee at that point of the request (by the latest block)!

But IMO, I don't think it is a bug.. because it is just a moment the error comes out and if you try again with updated gasPrice then it will succeed. So, this api(eth_createAccessList) is just working well based on latest block(user requested)'s baseFee and doesn't have to (perhaps could not) know which block number the users actually requests when they not specify the block number.

I just add the testCase for covering the error gasPrice is less than the baseFee. Plz check this PR #30194

SangIlMo avatar Jul 20 '24 08:07 SangIlMo

doesn't have to (perhaps could not) know which block number the users actually requests when they not specify the block number.

Yes, and if the user does not specify the block number everything works as it should. This issue is ONLY about the case when the user DOES specify the block number and DOES NOT set txn.gasPrice manually. Maybe I wasn't clear about that in the issue description.

It is because the gasPrice is less than baseFee at that point of the request (by the latest block)!

Yes, that's the reason for this error and is the intended behavior (it is also what your test checks).

and if you try again with updated gasPrice then it will succeed

That's correct. What I'm trying to describe in this issue is that when my request does not contain a gasPrice at all (null value/empty field), Go-Ethereum calls args.setDefaults to attempt to fill in any missing values (including the gasPrice).

Since I, the caller, never set the gasPrice and there is logic for this case (args.setDefaults) it doesn't make sense to "try again with updated gasPrice" by sending another request, which contains a gas price. It would make more sense if either

  • Go-Ethereum would fill in the gasPrice with a value that actually works, as at that point Go-Ethereum knows the block it is running against OR
  • Go-Ethereum would try again by itself with a working value (again, I have never specified the gasPrice, so it doesn't make much sense for me to send a second request) OR
  • Remove args.setDefaults and document which values are required to be present in a request.

In my opinion the first option is the cleanest and since the block number/hash is known in the api function (blockNrOrHash) it shouldn't be difficult to do so either: https://github.com/ethereum/go-ethereum/blob/bcaf3747f8443c6f8042f4f058f74404bb8c0a22/internal/ethapi/api.go#L1506-L1515

At the moment the caller (via json rpc) has to have the block's header to fill in the gasPrice value, just to prevent args.setDefaults from filling in a bad value that results in this error.

DragonDev1906 avatar Jul 22 '24 09:07 DragonDev1906

This issue is ONLY about the case when the user DOES specify the block number and DOES NOT set txn.gasPrice manually

When gasPrice is not set, the baseFee is also initialized by this code. So, the error cannot be occurred like my test code (updated). And without gasPrice, I think it seems no matter to add block height or not.

https://github.com/ethereum/go-ethereum/blob/ef583e9d18a98dfea8dad9f027c9d8a989591579/core/vm/evm.go#L136-L158

Actually eth_createAccessList call might not need the gasPrice, because it just provides only the accessLists by the transaction and is not really executed by the node.

SangIlMo avatar Jul 23 '24 08:07 SangIlMo

// If basefee tracking is disabled (eth_call, eth_estimateGas, etc), and no // gas prices were specified, lower the basefee to 0 to avoid breaking EVM // invariants (basefee < feecap)

This doesn't always manage to avoid breaking this invariant (as evident by the error message), so either

  • gas prices are specified by something other than the one using the api OR
  • config.NoBaseFee == false OR
  • the code this comment is referring to is broken.

Unless I'm mistaken the following code causes the first of these conditions

This sets gasPrice or MaxFeePerGas using the latest header: https://github.com/ethereum/go-ethereum/blob/bcaf3747f8443c6f8042f4f058f74404bb8c0a22/internal/ethapi/transaction_args.go#L186-L187 https://github.com/ethereum/go-ethereum/blob/bcaf3747f8443c6f8042f4f058f74404bb8c0a22/internal/ethapi/transaction_args.go#L226-L242 https://github.com/ethereum/go-ethereum/blob/bcaf3747f8443c6f8042f4f058f74404bb8c0a22/internal/ethapi/transaction_args.go#L265-L290

This computes the gasPrice using a different header. During the transition this could even mean that the transaction arguments default to post-London and the gas price is calculated pre-London (though that likely wouldn't be a significant problem with eth_createAccessList:

https://github.com/ethereum/go-ethereum/blob/bcaf3747f8443c6f8042f4f058f74404bb8c0a22/internal/ethapi/api.go#L1543 https://github.com/ethereum/go-ethereum/blob/7ee9a6e89f59cee21b5852f5f6ffa2bcfc05a25f/internal/ethapi/transaction_args.go#L424-L449

And finally, this code would set default for GasPrice, but it is never called because the previous code has already set a (wrong) default for it. https://github.com/ethereum/go-ethereum/blob/ef583e9d18a98dfea8dad9f027c9d8a989591579/core/vm/evm.go#L136-L147

Thus not preventing the max fee per gas less than block base fee error.

DragonDev1906 avatar Jul 24 '24 08:07 DragonDev1906

I just add the testCase for covering the error gasPrice is less than the baseFee. Plz check this PR https://github.com/ethereum/go-ethereum/pull/30194

I'm not sure if the situation described above can happen in your test case. It certainly won't always happen. As far as I can tell it only happens when:

  • You call ec.CreateAccessList without fee parameters ( :+1: ) AND
  • During processing a new block is minted and is the new chain head (might happen in the test case, but probably only rarely) AND
  • That block has a higher basFee than its parent (makes it even more unlikely to happen in the test case).

For context: I've had this happen only occasionally, perhaps once in 200-500 requests (so far only tested it on Sepolia)

DragonDev1906 avatar Jul 24 '24 09:07 DragonDev1906

Unless I'm mistaken the following code causes the first of these conditions

I think your code analysis is correct. As you know the api is using config := vm.Config{Tracer: tracer.Hooks(), NoBaseFee: true} in createAccessList, and according to the NewEVM() code I mentioned, baseFee is always initialized if gasPrice is not set. And I think you pointed out well that gasPrice is always filled with default.

But I would add one more thing: if gasPrice is nil, gasPrice is not filled with the default value, but with the MaxFeePerGas value according to EIP1559 (by the IsLondon). So if the user does not specify a gasPrice, baseFee is eventually initialized because the nil value is retained, because of the NoBaseFee == true setting. (You may already know) Anyway, I don't think this is the point of this issue.

To say exactly what we are discussing, "Should the createAccessList api reset the gasPrice for the block baseFee for incorrect gasPrice input from the user?".

I think the answer you're looking for is “YES", but I'd say “NO". gasPrice is what the user is willing to pay, and I think it's problematic to arbitrarily modify that in the API. So more fundamentally, I think of this case as "incorrect user input" and the current API is handling the error well.

So when I first posted the testcase code, I arbitrarily set a low value for gasPrice to show that the API is handling the error "correctly".

I think this is a clear disagreement between us. To summarize, I think the answer is "no" for the following reasons

  • gasPrice is an explicit amount that the user is willing to pay, so modifying it directly in the API can lead to more errors.
  • It's the user's responsibility to enter the gasPrice correctly, and the current API takes this into account (as shown in the test code).
  • I think the current setting of NoBaseFee == true (createAccessList does not consider baseFee and it is not actually needed) supports the above two comments.

SangIlMo avatar Jul 27 '24 05:07 SangIlMo

I think the main issue is that NoBaseFee == true (which I've overlooked during my initial analysis) isn't honered here, or it's scope does not include the check against the gasPrice.

Unless not setting a gasPrice (i.e. nil/null) in the json is explicitly incorrect usage of the API, for which I've so far found no evidence or documentation. I'm completely on your side that if the api user does specify a gasPrice != nil that is wrong it should result in this error. But when not specifying one at all it should either be explicitly forbidden (and documented) - perhaps with a different error message that is always returned in that case - or it should work.

I'd argue that a gasPrice = 0 is different from a "didn't give us a value" (null), as in the first case the user explicitly told Go-Ethereum to set it to 0, while the second is more a "set it to whatever works" (from the viewpoint of the json-api user.

The current behavior (for gasPrice == nil) is way too unpredictable and effectively a race condition.

DragonDev1906 avatar Jul 29 '24 10:07 DragonDev1906

eth_createAccessList to succeed when given a block hash (that wasn't pruned) and a basic transaction (only to and input) that does not revert.

What is it you want to achieve by this? I don't understand the usecase, please elaborate.

holiman avatar Oct 08 '24 12:10 holiman

What is it you want to achieve by this? I don't understand the usecase, please elaborate.

Execution of EVM view functions in a resource limited environment without trusting the Go-Ethereum node. To avoid having too many round trips I need the list of storage slots + accounts accessed by the EVM (=> eth_createAccessList) I'm first fetching all the storage and a merkle proof which are checked against the blocks state_root.

Since those merkle proofs need to be checked against the block hash I don't want Go-Ethereum to choose the block hash (it still is one of the most recent ones), thus making it easier to combine the storage proofs with the correct block.

Since I'm only executing EVM view functions I don't really care about anything related to fees (only a gas-limit to avoid DoS), which is how I found this bug. But I still have to provide the fees just so Go-Ethereum doesn't randomly fail to evaluate the view function, returning an error instead of the list of storage slots accessed.

For me that isn't a big issue (I have the fee information that is needed to avoid this bug), but random fails is still not what you'd expect when the api allows requests that don't include those fields.

DragonDev1906 avatar Oct 08 '24 15:10 DragonDev1906