feat: add era reader/writer - #149
Conversation
Summary of ChangesHello @wemeetagain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's capability to interact with historical Ethereum Beacon Chain data by introducing a robust system for reading, writing, and managing 'era' files. This new functionality provides a streamlined approach to accessing historical beacon states and blocks, which is crucial for simplifying testing, benchmarking, and analysis of the state transition function without the need for a full node or extensive additional infrastructure. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new era module for reading and writing era files, which is a significant and well-structured feature. The implementation is mostly solid, with good structure and tests. I've found a few issues related to potential errors, style guide adherence, and code simplification. My main suggestions are to use std.io.copy for file downloading to improve efficiency and maintainability, fix a potential crash in the era writer when handling era 0, and address a type-casting issue in the era reader. I've also pointed out a function that exceeds the length limit from the style guide.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive support for Ethereum consensus layer ERA file format (e2store), enabling reading and writing of historical beacon chain data for testing and benchmarking purposes without requiring a full node. The implementation is primarily transliterated from Lodestar's TypeScript implementation.
- Implements Reader and Writer for ERA files with full validation support
- Adds downloader script for fetching ERA files from nimbus.team
- Updates snappy dependency to support both raw and framed compression modes
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| zbuild.zon | Updates snappy dependency and adds download_era_options configuration with base URL and file list |
| build.zig.zon | Updates snappy package hash for new commit |
| build.zig | Adds era module, download_era_files executable, and integrates era testing infrastructure |
| src/era/root.zig | Module entry point exposing e2s, era, Reader, and Writer components |
| src/era/era.zig | Core ERA file utilities including filename parsing, group indices, and validation functions |
| src/era/e2s.zig | E2Store format implementation with entry reading/writing and SlotIndex serialization |
| src/era/Reader.zig | ERA file reader with methods for reading compressed/serialized blocks and states |
| src/era/Writer.zig | ERA file writer with state machine for sequential block/state writing |
| src/state_transition/types/beacon_state.zig | Adds deserialize and serialize methods to BeaconStateAllForks for all fork variants |
| src/state_transition/types/beacon_block.zig | Adds deserialize, deinit, and serialize methods to SignedBeaconBlock, changes pointers from const to mutable |
| src/config/root.zig | Exports mainnet_genesis_validators_root constant |
| src/config/chain/networks/mainnet.zig | Adds mainnet_genesis_validators_root constant and sets FULU_FORK_EPOCH to 411392 |
| scripts/download_era_files.zig | HTTP downloader script for fetching ERA files with existence checks and streaming writes |
| test/int/era/root.zig | Integration tests validating ERA file reading and round-trip write operations |
| test/int/root.zig | Imports era integration tests |
| test/spec/test_case.zig | Updates snappy import to use .raw namespace |
| test/spec/ssz/test_case.zig | Updates snappy import to use .raw namespace |
| .gitignore | Adds *.era pattern to exclude downloaded ERA files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try reader.validate(allocator); | ||
| } | ||
|
|
||
| test "write an era file from an existing era file" { |
There was a problem hiding this comment.
after zig build run:download_era_files I found it takes > ~70s in my environment to run test:int
maybe drop a comment to instruct people to move downloaded files to another folder to save time
There was a problem hiding this comment.
Or could we have this test be entirely independent of downloading, either by just using a known good era.Reader (preferred) or committing an era file? Seems not great to have to have extra instructions, it's more ideal to be able to zig build test after fetching the repo.
We can do the same for the validation test by writing a known good era file into a tmp dir and then validating it, so we don't have to rely on downloading for the tests, which makes us do stuff like SkipZigTest in the case of errors external to our implementation (download failure, etc.)
There was a problem hiding this comment.
I found it takes > ~70s in my environment to run
test:int
Maybe time to just add another test specifically for slow tests
Seems not great to have to have extra instructions, it's more ideal to be able to zig build test after fetching the repo.
I think we already have precedent to requiring extra instructions for some tests (download and write spec tests).
The problem here is similar. The validation rules are quite strict, and "actually useful" test data is big.
The benefit though, is that these test files are "actually useful" in that they are actually representative of our intended workload.
I think this cost (run an extra script to download a few files) can be offset by leaning in the other direction -- reusing these era files for as many other tasks as possible. For example, it should be straightforward to refactor #6 and #121 (ssz and stfn microbenchmarks) to use the very same era files. Also it should be straightforward to add a script that loads an era file and replays the state transition -- for the purpose of running perf or collecting prometheus metrics from it.
Imo these era files can be like a poor-man's deterministic testing framework. In lieu of the ability to generate test data, or until we have that ability, these era files are the next best thing.
twoeths
left a comment
twoeths
left a comment
There was a problem hiding this comment.
looks good to me after comparing to ChainSafe/lodestar#8035
the tests work well in my environment
I dropped some minor comments
| try reader.validate(allocator); | ||
| } | ||
|
|
||
| test "write an era file from an existing era file" { |
There was a problem hiding this comment.
Or could we have this test be entirely independent of downloading, either by just using a known good era.Reader (preferred) or committing an era file? Seems not great to have to have extra instructions, it's more ideal to be able to zig build test after fetching the repo.
We can do the same for the validation test by writing a known good era file into a tmp dir and then validating it, so we don't have to rely on downloading for the tests, which makes us do stuff like SkipZigTest in the case of errors external to our implementation (download failure, etc.)
eramodule (mostly transliterated from @lodestar/era)Reader/Writerimplementationszbuild run download_era_filesdownload_era_fileshasn't been run)My hope is that era support can be leveraged for simple testing / benchmarking / etc of the stfn without the need for a full node or a lot of additional infra.
PS:
zbuildneeded to be rebuilt to fix a bug with list options.