-
Notifications
You must be signed in to change notification settings - Fork 126
feat(l1): cache block hash lookup for BLOCKHASH opcode #5434
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
base: main
Are you sure you want to change the base?
Conversation
Lines of code reportTotal lines added: Detailed view |
| .get_canonical_block_hash_sync(block_number) | ||
| .map_err(|err| EvmError::DB(err.to_string()))? | ||
| { | ||
| block_hash_cache.insert(block_number, hash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this grows infinitely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only used for the BLOCKHASH opcode implementation, which checks that the requested block number is less than 256 blocks away from the current block, so it should only grow up to 256
crates/blockchain/vm.rs
Outdated
| // We use this when executing blocks in batches, as we will only add the blocks at the end | ||
| // And may need to access hashes of blocks previously executed in the batch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the PR is not open yet but we should update this comment, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch!
|
I suggest we try preloading the 256 hashes in an array/vector first. For the most part they could live in memory (they take 8kiB for the canonical chain, with some extra for reorgs and forks) already, and even when they don't, the common case is blocks in the canonical chain, meaning they can be resolved with a single query to the canonical hashes table, and the rest require a bounded (65, I think, for forks starting at a non-finalized block that should be at most 64 blocks long, plus one for the intersection with the canonical chain). |
I implemented this on #5443 but the benchmark results are worse than this solution. |
I'd argue it doesn't really implement what I said, as it's making a get to the DB for each block, which I'm saying we should avoid. It's also using a hashmap rather than a vector, and hashing is not free either. |
Motivation
Addressing #5344 by using the existing
block_hash_cacheto store block hashes looked up duringBLOCKHASHopcode execution to speed up subsequent lookupsDescription
Benchmark Results: x33,6 improvement on BlockHash EEST gas benchmark: 1080.92 Mgas/s vs 32.18 Gas/s on main (7b4a0ba)
Closes #5344