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

eth_createAccessList fails when using block_hash and not setting txn fees #30145

Open
DragonDev1906 opened this issue Jul 11, 2024 · 10 comments · May be fixed by #30538
Open

eth_createAccessList fails when using block_hash and not setting txn fees #30145

DragonDev1906 opened this issue Jul 11, 2024 · 10 comments · May be fixed by #30538
Assignees
Labels

Comments

@DragonDev1906
Copy link
Contributor

DragonDev1906 commented Jul 11, 2024

System information

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

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:
    // Retrieve the execution context
    db, header, err := b.StateAndHeaderByNumberOrHash(ctx, blockNrOrHash)
    if db == nil || err != nil {
    return nil, 0, nil, err
    }
    // Ensure any missing fields are filled, extract the recipient and input data
    if err := args.setDefaults(ctx, b, true); err != nil {
    return nil, 0, nil, err
    }
  • The part of the implementation that sets this:
    func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) error {
    head := b.CurrentHeader()
  • Where the field is set:
    tip, err := b.SuggestGasTipCap(ctx)
    if err != nil {
    return err
    }
    args.MaxPriorityFeePerGas = (*hexutil.Big)(tip)

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): #25319

@DragonDev1906
Copy link
Contributor Author

DragonDev1906 commented Jul 12, 2024

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:

@SangIlMo
Copy link
Contributor

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

@DragonDev1906
Copy link
Contributor Author

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:

// Retrieve the execution context
db, header, err := b.StateAndHeaderByNumberOrHash(ctx, blockNrOrHash)
if db == nil || err != nil {
return nil, 0, nil, err
}
// Ensure any missing fields are filled, extract the recipient and input data
if err := args.setDefaults(ctx, b, true); err != nil {
return nil, 0, nil, err
}

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.

@SangIlMo
Copy link
Contributor

SangIlMo commented Jul 23, 2024

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.

go-ethereum/core/vm/evm.go

Lines 136 to 158 in ef583e9

func NewEVM(blockCtx BlockContext, txCtx TxContext, statedb StateDB, chainConfig *params.ChainConfig, config Config) *EVM {
// 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)
if config.NoBaseFee {
if txCtx.GasPrice.BitLen() == 0 {
blockCtx.BaseFee = new(big.Int)
}
if txCtx.BlobFeeCap != nil && txCtx.BlobFeeCap.BitLen() == 0 {
blockCtx.BlobBaseFee = new(big.Int)
}
}
evm := &EVM{
Context: blockCtx,
TxContext: txCtx,
StateDB: statedb,
Config: config,
chainConfig: chainConfig,
chainRules: chainConfig.Rules(blockCtx.BlockNumber, blockCtx.Random != nil, blockCtx.Time),
}
evm.interpreter = NewEVMInterpreter(evm)
return evm
}

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.

@DragonDev1906
Copy link
Contributor Author

// 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:

func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) error {
head := b.CurrentHeader()

if isLondon {
// London is active, set maxPriorityFeePerGas and maxFeePerGas.
if err := args.setLondonFeeDefaults(ctx, head, b); err != nil {
return err
}
} else {
if args.MaxFeePerGas != nil || args.MaxPriorityFeePerGas != nil {
return errors.New("maxFeePerGas and maxPriorityFeePerGas are not valid before London is active")
}
// London not active, set gas price.
price, err := b.SuggestGasTipCap(ctx)
if err != nil {
return err
}
args.GasPrice = (*hexutil.Big)(price)
}
return nil

func (args *TransactionArgs) setLondonFeeDefaults(ctx context.Context, head *types.Header, b Backend) error {
// Set maxPriorityFeePerGas if it is missing.
if args.MaxPriorityFeePerGas == nil {
tip, err := b.SuggestGasTipCap(ctx)
if err != nil {
return err
}
args.MaxPriorityFeePerGas = (*hexutil.Big)(tip)
}
// Set maxFeePerGas if it is missing.
if args.MaxFeePerGas == nil {
// Set the max fee to be 2 times larger than the previous block's base fee.
// The additional slack allows the tx to not become invalidated if the base
// fee is rising.
val := new(big.Int).Add(
args.MaxPriorityFeePerGas.ToInt(),
new(big.Int).Mul(head.BaseFee, big.NewInt(2)),
)
args.MaxFeePerGas = (*hexutil.Big)(val)
}
// Both EIP-1559 fee parameters are now set; sanity check them.
if args.MaxFeePerGas.ToInt().Cmp(args.MaxPriorityFeePerGas.ToInt()) < 0 {
return fmt.Errorf("maxFeePerGas (%v) < maxPriorityFeePerGas (%v)", args.MaxFeePerGas, args.MaxPriorityFeePerGas)
}
return nil
}

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:

msg := args.ToMessage(header.BaseFee)

func (args *TransactionArgs) ToMessage(baseFee *big.Int) *core.Message {
var (
gasPrice *big.Int
gasFeeCap *big.Int
gasTipCap *big.Int
)
if baseFee == nil {
gasPrice = args.GasPrice.ToInt()
gasFeeCap, gasTipCap = gasPrice, gasPrice
} else {
// A basefee is provided, necessitating 1559-type execution
if args.GasPrice != nil {
// User specified the legacy gas field, convert to 1559 gas typing
gasPrice = args.GasPrice.ToInt()
gasFeeCap, gasTipCap = gasPrice, gasPrice
} else {
// User specified 1559 gas fields (or none), use those
gasFeeCap = args.MaxFeePerGas.ToInt()
gasTipCap = args.MaxPriorityFeePerGas.ToInt()
// Backfill the legacy gasPrice for EVM execution, unless we're all zeroes
gasPrice = new(big.Int)
if gasFeeCap.BitLen() > 0 || gasTipCap.BitLen() > 0 {
gasPrice = math.BigMin(new(big.Int).Add(gasTipCap, baseFee), gasFeeCap)
}
}
}

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.

go-ethereum/core/vm/evm.go

Lines 136 to 147 in ef583e9

func NewEVM(blockCtx BlockContext, txCtx TxContext, statedb StateDB, chainConfig *params.ChainConfig, config Config) *EVM {
// 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)
if config.NoBaseFee {
if txCtx.GasPrice.BitLen() == 0 {
blockCtx.BaseFee = new(big.Int)
}
if txCtx.BlobFeeCap != nil && txCtx.BlobFeeCap.BitLen() == 0 {
blockCtx.BlobBaseFee = new(big.Int)
}
}

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

@DragonDev1906
Copy link
Contributor Author

DragonDev1906 commented Jul 24, 2024

I just add the testCase for covering the error gasPrice is less than the baseFee. Plz check this PR #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 ( 👍 ) 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)

@SangIlMo
Copy link
Contributor

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.

@DragonDev1906
Copy link
Contributor Author

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.

@holiman
Copy link
Contributor

holiman commented Oct 8, 2024

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.

@DragonDev1906
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants
@karalabe @holiman @DragonDev1906 @SangIlMo and others