search

bluealloy / revm

Rust implementation of the Ethereum Virtual Machine.
https://bluealloy.github.io/revm/
MIT License
1.61k stars 540 forks source link

refactor: replace AccessList with alloy version #1552

Closed Wodann closed 3 months ago

Wodann commented 3 months ago

The replaces the custom AccessListItem definition with the alloy version. The alloy version uses a B256 instead of a U256, which is conform the Ethereum spec.

This avoids type conversions from when interacting with revm, where currently B256 storage indices need to be converted to U256 values using big-Endian conversion.

I think it's worth considering changing the account storage to match this.

DaniPopes commented 3 months ago

Access list U256 does not touch the stack, it goes to storage and storage is U256. The conversion is essentially free in the context of SLOAD/SSTORE where 99% of the time is spent in hashmaps and DB, I would not block this change on that.

alloy-eips is also a primitive/small crate, it has essentially the same dependencies as revm-primitives if not less.

rakita commented 3 months ago

Access list U256 does not touch the stack, it goes to storage and storage is U256. The conversion is essentially free in the context of SLOAD/SSTORE where 99% of the time is spent in hashmaps and DB, I would not block this change on that.

True it touches storage and storage is U256 as the stack is U256. Again, it is strange to depend on alloy, and would like to see alloy including revm-primitives for example to add From<>

Wodann commented 3 months ago

U256 is there as the stack is in U256 and we remove conversion between B256/U256 inside revm, this can be done outside of it.

I see your point w.r.t the SLOAD/SSTORE instructions popping a U256 from the stack and wanting to use that to access fn storage. I'm happy to leave that as is, without conversion to B256.

would like to see alloy including revm-primitives for example to add From<>

I'm also happy to move the AccessList and AccessListItem types to revm-primitives. Either is fine for us, as long as we can use the same type across the board.

The intent of this PR is to avoid the conversion fn (Vec<(Address, Vec<B256>) -> Vec<(Address, Vec<U256>)> to configure transactions.

Especially in combination with the trait Transaction proposed in the chain-specific configuration PR, this creates the ability to bypass the conversion of From<ThirdPartyTransaction> for TxEnv.

Instead we can directly use ThirdPartyTransaction in revm to query the transaction parameters for execution. This philosophy does require using the same types at the API level between Ethereum protocol and Ethereum VM. Otherwise people will always need to create two types: the chain transaction type and a chain "transaction environment" type.

Please let me know which changes are reasonable to get this PR across the finish line - if at all possible.

Wodann commented 3 months ago

Thanks, we resolved our disputes internally and decided to include this, one last nit for the test changes

Perfect. That voids the need for my previous message 😁

rakita commented 3 months ago

I am still of the stance that AccessList should be inside this repo, and alloy/eips to depend on revm-primitives, revm-primitives is the lowest crate in the repo and the crate and can be published at any time. In the end Transaction (and its parts) without evm does not exist.

But having it in one or other repo is a maintenance problem, the user problem is having two different types and is something that would be good to solve. As I said internally I am okay to step back and include alloy/eips.

rakita commented 3 months ago

Failed tests not related to this PR