fix(anvil): Fix AccessList generation

[ngotchac]

Motivation

The generated AccessList via createAccessList RPC call wasn't actually correct:

• It included the coinbase address (for which info was fetched, and thus added to the state diff)
• It didn't take into account for the to address whether storage was read or not
• The precompiles that should be excluded was a fixed list, not dependent on the fork
• Although not necessarily an issue, it kind of depended on revm implementation regarding the returned state-diff, which returned read accounts even though they weren't changed

Solution

I implemented a custom tracer which checks for some opcodes was address/storage slot will be read/written to.
The trace was implemented based on Geth's one: https://github.com/ethereum/go-ethereum/blob/23ac8df15302bbde098cab6d711abdd24843d66a/eth/tracers/logger/access_list_tracer.go#L140-L160

It also returns the second execution's gas-used, instead of a gas estimation (which should be higher than the gas-used, since it has to include refunds).

[mattsse]

thanks so much, this is indefinitely better than before.

Accesslists are a bit too cursed for me -.-

love the tracer impl.

only have smol nits

[mattsse]

can this even return an error?
I couldn't find any ?, so perhaps we just return the Env?

[ngotchac]

Definitely! I thought this was the kind of stuff clippy would catch...
It's done :)

[mattsse]

can just flip the clone here?
clone access_list.0 at L800, so we don't need to unwrap here?

[ngotchac]

Yep, was trying to save a clone but it shouldn't matter that much here...

[ngotchac]

thanks so much, this is indefinitely better than before.

Accesslists are a bit too cursed for me -.-

love the tracer impl.

only have smol nits

Ehe, yeah I agree, AccessLists are actually really not trivial, and there is no official spec on how to generate them... Comparing my approach here with Geth's, they are running the access list generation in an infinite loop until there is no more changes; that's wild!!

Also another thing, there seems to be a few tests failing, and this is because ethers-rs is actually using the GasUsed output of the eth_createAccessList call in order to decide whether to use them or not, comparing it to a standard gas estimate: cf. https://github.com/gakonst/ethers-rs/blob/4655c4448139af865df9d7c4d06cec0f67eb8651/ethers-providers/src/provider.rs#L359-L377

The issue is that the GasUsed returned on my branch is the real gas-used, and not the gas-limit that should be used to send the transactions. If using this value as the gas limit, the transaction will most likely run out of gas since refunds occur at the end (hence the tests failing).

So I'm not sure what the approach should be here:

1. Revert (part of) my changes so that the gas-used is the new gas estimate (which makes it different from other implementations, at least Geth and Nevermind, and doesn't really indicate the gas savings)
2. Update ethers-rs in order to not use this gas-used value as a gas-limit

I'd go with (2), but then there's not real way to compare gas usages, except by tracing the transaction I guess.

WDYT?

[mattsse]

The issue is that the GasUsed returned on my branch is the real gas-used, and not the gas-limit that should be used to send the transactions

right, the geth docs even say

The method eth_createAccessList returns list of addresses and storage keys used by the transaction, plus the gas consumed when the access list is added.

so I guess the safe way would be to always estimate if gas is not set, so your option 2

there's also:

#2330

which gives some reason as to not use accesslist at all

cc @gakonst

[ngotchac]

Yeah I think the auto access-list generation should be disabled in ethers, since it can always be done manually. The tricky situation here is that, whether it's manually or automatically, there's no way to compare gas usages between with and without AL; except by using traces. I would have thought that the eth_createAccessList would return a GasSaved field for this purpose.

And BTW (to address a point in #2330), access-list can definitely save gas even if the calling contract is doing [1, 23] storage accesses, if it itself calls another contract.

[mattsse]

needs ethers bump,
tested successfully locally

[ngotchac]

needs ethers bump, tested successfully locally

Included #2872 to make sure the tests pass

[mattsse]

awesome, ty so much

[sambacha]

Yeah I think the auto access-list generation should be disabled in ethers, since it can always be done manually. The tricky situation here is that, whether it's manually or automatically, there's no way to compare gas usages between with and without AL; except by using traces. I would have thought that the eth_createAccessList would return a GasSaved field for this purpose.

And BTW (to address a point in #2330), access-list can definitely save gas even if the calling contract is doing [1, 23] storage accesses, if it itself calls another contract.

The main purpose of Access Lists is not to save gas, but to protect from bricking contracts post EIP-2929. As the EIP motivation statement puts it:

Mitigates contract breakage risks introduced by EIP-2929, as transactions could pre-specify and pre-pay for the accounts and storage slots that the transaction plans to access; as a result, in the actual execution, the SLOAD and EXT* opcodes would only cost 100 gas: low enough that it would not only prevent breakage due to that EIP but also “unstuck” any contracts that became stuck due to EIP 1884.

Thank you for fixing this @ngotchac and for attending to and reviewing this @mattsse