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

feat: cache block hashes #71

Merged
merged 3 commits into from
Mar 10, 2022
Merged

Conversation

onbjerg
Copy link
Collaborator

@onbjerg onbjerg commented Mar 7, 2022

Caches block hashes, but also computes a block hash based on the block number if no block hash was returned from the underlying database.

Retrieving block hashes can be expensive (as is the case with our forking backend in Foundry), but it is also important that block hashes for different blocks differ from each other

@draganrakita I was unsure where to put this - we need it for cheats.roll to work correctly in Foundry, but it would also be weird to have a set_block_hash function in the database trait.

Two alternatives I could think of:

  • Allow inspectors to override block hashes
  • Implement our own database in Foundry, although this would pretty much be a 1:1 copy of CacheDB here with this patch

Curious to hear what you think

Caches block hashes, but also computes a block hash based
on the block number if no block hash was returned from the
underlying database.

Retrieving block hashes can be expensive, but it is also
important that block hashes for different blocks
differ from each other
@rakita
Copy link
Member

rakita commented Mar 9, 2022

Having cached hashes seems okay, having custom hash created on H256::zero() seems like that it can be added to ExtDB for CacheDB feels like a small pitfall.

@gakonst
Copy link
Collaborator

gakonst commented Mar 9, 2022

@rakita Agree that doing on CacheDB seems weird. What about doing it on EmptyDB's fn block_hash? When we're forking, we're gonna have a value that corresponds to the number, but when we're not forking we're using EmptyDB, so maybe that's the right place for this logic. @onbjerg https://github.com/bluealloy/revm/blob/main/crates/revm/src/db/in_memory_db.rs#L206

@rakita rakita merged commit df012e8 into bluealloy:main Mar 10, 2022
@onbjerg onbjerg deleted the onbjerg/blockhashes branch March 10, 2022 08:39
@onbjerg onbjerg restored the onbjerg/blockhashes branch March 10, 2022 10:13
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.

3 participants