bench: Add benchmark for block and Epoch Processing - #121
Conversation
Summary of ChangesHello @guha-rahul, 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 performance testing suite by introducing detailed benchmarks for both block and epoch processing within the state transition logic. These new benchmarks will help in identifying performance bottlenecks and optimizing critical operations. Additionally, it includes a crucial bug fix in the attestation data signing root calculation, which in turn allowed for the re-enabling of proposer and signature verification in the state transition spec tests, improving test coverage and reliability. The Fulu fork epoch configuration was also updated. Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces new benchmarks for process_block and process_epoch operations within the state transition logic, specifically for the Fulu fork. These benchmarks, located in bench/state_transition/process_block.zig and bench/state_transition/process_epoch.zig, load real mainnet state and block data from SSZ files to measure the performance of individual block and epoch processing steps. A critical configuration update was made in src/config/chain/networks/mainnet.zig, setting FULU_FORK_EPOCH to 411392 to enable the Fulu fork on the mainnet, which is essential for the new benchmarks and Fulu-related logic. A critical bug fix was also implemented in src/state_transition/signature_sets/indexed_attestation.zig, changing computeEpochAtSlot to computeStartSlotAtEpoch to ensure the getDomain function receives the correct Slot type for attestation data signing root calculation. To enhance test coverage and correctness, verify_proposer and verify_signatures were enabled in the spec tests (test/spec/runner/transition.zig), validating the signature verification mechanisms, especially after the bug fix. The build system (build.zig and zbuild.zon) was updated to include these new benchmark executables and their respective test targets.
| .ELECTRA_FORK_EPOCH = 364032, | ||
| .FULU_FORK_VERSION = b(4, "0x06000000"), | ||
| .FULU_FORK_EPOCH = std.math.maxInt(u64), | ||
| .FULU_FORK_EPOCH = 411392, |
There was a problem hiding this comment.
Changing FULU_FORK_EPOCH from std.math.maxInt(u64) to a concrete value 411392 is a critical configuration update. This enables the Fulu fork on the mainnet configuration, which is essential for the new benchmarks and any Fulu-related logic to function correctly. This is a functional change, not a bug, and is necessary for the Fulu benchmarks to run correctly.
|
|
||
| pub fn getAttestationDataSigningRoot(cached_state: *const CachedBeaconStateAllForks, data: *const AttestationData, out: *[32]u8) !void { | ||
| const slot = computeEpochAtSlot(data.target.epoch); | ||
| const slot = computeStartSlotAtEpoch(data.target.epoch); |
There was a problem hiding this comment.
This change from computeEpochAtSlot to computeStartSlotAtEpoch is a critical bug fix. The getDomain function expects a Slot type for its message_slot parameter, but data.target.epoch is an Epoch type. computeStartSlotAtEpoch correctly converts the epoch number to its starting slot number, ensuring the getDomain function receives the expected input. This directly addresses the bug mentioned in the PR description.
| .verify_proposer = true, | ||
| .verify_signatures = true, |
There was a problem hiding this comment.
Enabling verify_proposer and verify_signatures in the spec tests is a significant improvement for test coverage and correctness. This change ensures that the state transition logic is thoroughly validated, especially in light of the bug fix in getAttestationDataSigningRoot mentioned in the PR description. It indicates increased confidence in the correctness of the signature verification mechanisms.
|
@spiral-ladder zbench has a character limit for benchmark names, So i was abbreviating some of the names of the benchmark names. |
I see! Wasn't aware of the 22-character limit, thanks for letting me know. |
spiral-ladder
left a comment
There was a problem hiding this comment.
Looking great - gave a pass through
| pub fn slotFromStateBytes(state_bytes: []const u8) ?Slot { | ||
| if (state_bytes.len < 48) return null; | ||
| return std.mem.readInt(u64, state_bytes[40..48], .little); | ||
| } | ||
|
|
||
| /// Read slot from raw SignedBeaconBlock SSZ bytes (offset 100) | ||
| pub fn slotFromBlockBytes(block_bytes: []const u8) ?Slot { | ||
| if (block_bytes.len < 108) return null; | ||
| return std.mem.readInt(u64, block_bytes[100..108], .little); | ||
| } |
There was a problem hiding this comment.
2 points here:
-
I think given that these are only utils used in benchmarking we should just keep them in
bench/state_transitionrather than put them inconfig/fork.zig. We can move these functions if we ever use them outside of benchmarking. -
I think we can assume we're always going to be loading valid bytes here and if not we should be error-ing instead anyway:
| pub fn slotFromStateBytes(state_bytes: []const u8) ?Slot { | |
| if (state_bytes.len < 48) return null; | |
| return std.mem.readInt(u64, state_bytes[40..48], .little); | |
| } | |
| /// Read slot from raw SignedBeaconBlock SSZ bytes (offset 100) | |
| pub fn slotFromBlockBytes(block_bytes: []const u8) ?Slot { | |
| if (block_bytes.len < 108) return null; | |
| return std.mem.readInt(u64, block_bytes[100..108], .little); | |
| } | |
| /// Read slot from raw BeaconState SSZ bytes (offset 40) | |
| pub fn slotFromStateBytes(state_bytes: []const u8) Slot { | |
| std.debug.assert(state_bytes.len >= 48); | |
| return std.mem.readInt(u64, state_bytes[40..48], .little); | |
| } | |
| /// Read slot from raw SignedBeaconBlock SSZ bytes (offset 100) | |
| pub fn slotFromBlockBytes(block_bytes: []const u8) Slot { | |
| std.debug.assert(block_bytes.len >= 108); | |
| return std.mem.readInt(u64, block_bytes[100..108], .little); | |
| } |
There was a problem hiding this comment.
moved these and the loadState and loadBlock to a utils file inside bench
| // Actual processEpoch function | ||
| try bench.addParam("process_epoch", &ProcessEpochBench{ .cached_state = cached_state }, .{}); |
There was a problem hiding this comment.
What does "Actual processEpoch function" mean?
There was a problem hiding this comment.
I meant the non segmented one. Is "continuous" a better name or something more descriptive?
There was a problem hiding this comment.
I don't think we need a special name, just non-segmented vs segmented will do.
| // Actual processEpoch function | ||
| try bench.addParam("process_epoch", &ProcessEpochBench{ .cached_state = cached_state }, .{}); |
There was a problem hiding this comment.
I don't think we need a special name, just non-segmented vs segmented will do.
spiral-ladder
left a comment
There was a problem hiding this comment.
Thanks again for the cleanup! Just a few more loose ends. There's still some things to cleanup but we can leave those for a followup PR.
| const cloned = self.cached_state.clone(allocator) catch unreachable; | ||
| defer { | ||
| cloned.deinit(); | ||
| allocator.destroy(cloned); | ||
| } |
There was a problem hiding this comment.
We can save this for a future PR, but we should probably move this clone (and other clones) out of the run definition since cloning should not be part of the cost of benchmarking processXXX
spiral-ladder
left a comment
There was a problem hiding this comment.
LGTM, thanks for the PR!
tracks #102
getAttestationDataSigningRoot