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

refactor: replace AccessList with alloy version #1552

Merged
merged 5 commits into from
Jun 22, 2024

Conversation

Wodann
Copy link
Contributor

@Wodann Wodann commented Jun 20, 2024

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.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this would be great and would get rid of a bunch of allocs in reth

crates/primitives/Cargo.toml Show resolved Hide resolved
crates/interpreter/src/gas/calc.rs Outdated Show resolved Hide resolved
crates/revm/src/journaled_state.rs Show resolved Hide resolved
rakita

This comment was marked as resolved.

@DaniPopes

This comment was marked as resolved.

@rakita

This comment was marked as resolved.

Copy link
Collaborator

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

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

@Wodann
Copy link
Contributor Author

Wodann commented Jun 20, 2024

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
Copy link
Contributor Author

Wodann commented Jun 20, 2024

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 😁

crates/interpreter/src/gas/calc.rs Outdated Show resolved Hide resolved
examples/generate_block_traces.rs Show resolved Hide resolved
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@Wodann Wodann requested a review from mattsse June 21, 2024 21:41
@rakita
Copy link
Member

rakita commented Jun 22, 2024

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.

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm

@rakita
Copy link
Member

rakita commented Jun 22, 2024

Failed tests not related to this PR

@rakita rakita merged commit 58ab5e3 into bluealloy:main Jun 22, 2024
20 of 26 checks passed
This was referenced Jun 22, 2024
@Wodann Wodann deleted the refactor/access-list branch June 25, 2024 14:57
This was referenced Jun 27, 2024
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