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

internal/ethapi: Set basefee for AccessList based on given block, not chain tip #30538

Merged
merged 6 commits into from
Nov 8, 2024

Conversation

jwasinger
Copy link
Contributor

@jwasinger jwasinger commented Oct 2, 2024

The specification for eth_accessList takes a BlockNumberOrHash as an optional parameter (arguably, any other state than latest/pending is useless). Before this PR, we always default to sourcing the gas tip based on a value that would be appropriate for the state at the head of the chain.

However, when the base fee of a historical target block exceeds this value, the access list creation fails. This PR covers this (niche) error case.

Several functions for filling call/transaction parameters:

  • setDefaults fills in the default values for transaction fields based on the current state. It is also responsible for setting blob fields and chain id, both of which are irrelevant in the the context of access list creation.
  • setFeeDefaults fills tx fee fields with sensible values based the hard fork at the current block.
  • CallDefaults sanitizes fields relevant to calls, ensuring they are not nil.

In this implementation, I modify access list creation to call setFeeDefaults instead of setDefaults. setFeeDefaults now sets fee fields based on a provided header (only relevant for post 1559 dynamic txs which compute the base fee based on parent header). I also compute set the nonce based on what it historically was at the given header, and call CallDefaults.

The only functionality that the new logic doesn't cover by not calling setDefaults is setting blob tx fields and gas estimation: both of which are irrelevant and not used by access list creation.

@jwasinger jwasinger changed the title internal/ethapi: fix AccessList dynamic fee calculation internal/ethapi: Set basefee for AccessList based on given block, not chain tip Oct 2, 2024
@s1na
Copy link
Contributor

s1na commented Oct 3, 2024

Yeah I think it makes sense. We shouldn't need to estimate actual gas prices or gas limit. Sure those can influence EVM execution but the user can pass in those if they care/their contract depends on it.

Only one thing I noticed: not filling the nonce causes the To address computation possibly wrong for create txes.

@jwasinger
Copy link
Contributor Author

jwasinger commented Oct 4, 2024

Only one thing I noticed: not filling the nonce causes the To address computation possibly wrong for create txes.

So this would mean that this is broken in all eth_call variants?

@jwasinger
Copy link
Contributor Author

jwasinger commented Oct 4, 2024

It's not broken. The reason is that the nonce during creation is sourced from the state of the account, not what's in the transaction args.

@jwasinger jwasinger marked this pull request as ready for review October 4, 2024 11:59
@s1na
Copy link
Contributor

s1na commented Oct 4, 2024

It's not broken. The reason is that the nonce during creation is sourced from the state of the account, not what's in the transaction args.

Yep for eth_call it's not broken but here it will be when using CallDefaults. It's because CreateAccessList uses the nonce from tx args and passes that to the tracer. We could either here use account state, or modify the tracer to use OnEnter && level == 0 to get the to address.

@jwasinger
Copy link
Contributor Author

jwasinger commented Oct 8, 2024

If the provided nonce was not correct, the transaction will fail in TransactionArgs.TransitionDb. So I think the current code should be okay (?)

args.Nonce = &nonce
}
blockCtx := core.NewEVMBlockContext(header, NewChainContext(ctx, b), nil)
if err := args.CallDefaults(b.RPCGasCap(), blockCtx.BaseFee, b.ChainConfig().ChainID); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

the setDefaults has a lot of logic handling blobs that are passed in the args, and that will become broken if we just change this

Copy link
Contributor

@fjl fjl Oct 8, 2024

Choose a reason for hiding this comment

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

We could just call setBlobTxSidecar in CallDefaults. But I'd say it's OK not to support them here.

Copy link
Contributor

@holiman holiman Oct 8, 2024

Choose a reason for hiding this comment

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

But setDefaults will set the fee defaults on the latest block.

	if err := args.setFeeDefaults(ctx, b); err != nil {
		return err
	}

And regardless of what order we invoke it, it will use the latest header, and do

	if err := args.setCancunFeeDefaults(ctx, head, b); err != nil {
		return err
	}

SO it will apply cancun checks according to tip, regardless of what the given block is.

Copy link
Contributor

@holiman holiman Oct 8, 2024

Choose a reason for hiding this comment

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

How about something like this

// setFeeDefaults fills in default fee values for unspecified tx fields.
func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) error {
	return setFeeDefaultsUsingThisHeader(b.CurrentHeader())
}
func (args *TransactionArgs) setFeeDefaultsUsingThisHeader(ctx context.Context, b Backend, head *types.Header) error {
	// Sanity check the EIP-4844 fee parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

the setDefaults has a lot of logic handling blobs that are passed in the args, and that will become broken if we just change this

The logic there is to fill in blob proofs and commitments if they were not provided. Those matter only when submitting a tx. Since for a simulation all that can be accessed via evm is blob hashes (which still can be passed in via TransactionArgs).

All in all I think I agree with @jwasinger that CallDefaults is a better validation function to use here. All the methods that involve signing and submitting a tx use setDefaults while all the "simulation" methods use CallDefaults.

…with proper values. modify setFeeDefaults to take a historical header value (doesn't work if EIP-1559 is disabled. everything other than AccessLists, which were introduced after/same-time (?) as 1559, calls setFeeDefaults with current block header.
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

@holiman
Copy link
Contributor

holiman commented Nov 8, 2024

@jwasinger please update the top-level PR description (I'm assuming the code has changed since that was written?), so that it's an accurate description. This is a typical such PR that we might revisit three years from now, when trying to figure out why something works in a particular way. So it helps if the desc is up to date with what the PR does.

Other than that, let's merge on green IMO

@holiman holiman added this to the 1.14.12 milestone Nov 8, 2024
@holiman holiman merged commit 0fc9cca into ethereum:master Nov 8, 2024
2 of 3 checks passed
@jwasinger jwasinger deleted the fix-access-list-dynamic-fee branch November 8, 2024 16:56
@jwasinger
Copy link
Contributor Author

Done. I tried to summarize it as succinctly as possible but it got a bit long :)

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.

4 participants