Conversation
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
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 refactors the codebase to implement a more robust and type-safe approach to handling different Ethereum Beacon Chain forks. By leveraging Zig's compile-time generics, the changes introduce specialized types for each fork, enhancing type safety and enabling compiler optimizations. This refactoring centralizes fork-related logic within a new 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
|
There was a problem hiding this comment.
Code Review
This is an impressive and large-scale refactoring that introduces compile-time generics for handling fork-specific logic. The new fork_types module with Any* and Fork* types is a great architectural improvement that enhances type safety and likely performance. The changes are consistent and significantly improve the maintainability of the codebase, especially with the removal of the old types directory and the simplification of tests. I've found one critical issue that will cause a compilation error and a couple of style guide violations regarding function length that should be addressed.
| switch (self.*) { | ||
| .bellatrix => |*state| try state.setValue("latest_execution_payload_header", &header.bellatrix), | ||
| .capella => |*state| try state.setValue("latest_execution_payload_header", &header.capella), |
There was a problem hiding this comment.
The switch statement is not exhaustive. It's missing cases for .deneb, .electra, .fulu, and an else case. This will result in a compilation error.
Similar to how latestExecutionPayloadHeader handles these forks by mapping them to .deneb (since they share the same payload header structure), you should do the same here.
switch (self.*) {
.bellatrix => |*state| try state.setValue("latest_execution_payload_header", &header.bellatrix),
.capella => |*state| try state.setValue("latest_execution_payload_header", &header.capella),
.deneb, .electra, .fulu => |*state| try state.setValue("latest_execution_payload_header", &header.deneb),
else => return error.InvalidAtFork,
}
| pub fn deserialize(allocator: std.mem.Allocator, block_type: BlockType, fork_seq: ForkSeq, bytes: []const u8) !AnySignedBeaconBlock { | ||
| switch (fork_seq) { | ||
| .phase0 => { | ||
| if (block_type != .full) return error.InvalidBlockTypeForFork; | ||
| const signed_block = try allocator.create(ct.phase0.SignedBeaconBlock.Type); | ||
| errdefer allocator.destroy(signed_block); | ||
| signed_block.* = ct.phase0.SignedBeaconBlock.default_value; | ||
| try ct.phase0.SignedBeaconBlock.deserializeFromBytes(allocator, bytes, signed_block); | ||
| return .{ .phase0 = signed_block }; | ||
| }, | ||
| .altair => { | ||
| if (block_type != .full) return error.InvalidBlockTypeForFork; | ||
| const signed_block = try allocator.create(ct.altair.SignedBeaconBlock.Type); | ||
| errdefer allocator.destroy(signed_block); | ||
| signed_block.* = ct.altair.SignedBeaconBlock.default_value; | ||
| try ct.altair.SignedBeaconBlock.deserializeFromBytes(allocator, bytes, signed_block); | ||
| return .{ .altair = signed_block }; | ||
| }, | ||
| .bellatrix => { | ||
| if (block_type == .full) { | ||
| const signed_block = try allocator.create(ct.bellatrix.SignedBeaconBlock.Type); | ||
| errdefer allocator.destroy(signed_block); | ||
| signed_block.* = ct.bellatrix.SignedBeaconBlock.default_value; | ||
| try ct.bellatrix.SignedBeaconBlock.deserializeFromBytes(allocator, bytes, signed_block); | ||
| return .{ .full_bellatrix = signed_block }; | ||
| } else { | ||
| const signed_block = try allocator.create(ct.bellatrix.SignedBlindedBeaconBlock.Type); | ||
| errdefer allocator.destroy(signed_block); | ||
| signed_block.* = ct.bellatrix.SignedBlindedBeaconBlock.default_value; | ||
| try ct.bellatrix.SignedBlindedBeaconBlock.deserializeFromBytes(allocator, bytes, signed_block); | ||
| return .{ .blinded_bellatrix = signed_block }; | ||
| } | ||
| }, | ||
| .capella => { | ||
| if (block_type == .full) { | ||
| const signed_block = try allocator.create(ct.capella.SignedBeaconBlock.Type); | ||
| errdefer allocator.destroy(signed_block); | ||
| signed_block.* = ct.capella.SignedBeaconBlock.default_value; | ||
| try ct.capella.SignedBeaconBlock.deserializeFromBytes(allocator, bytes, signed_block); | ||
| return .{ .full_capella = signed_block }; | ||
| } else { | ||
| const signed_block = try allocator.create(ct.capella.SignedBlindedBeaconBlock.Type); | ||
| errdefer allocator.destroy(signed_block); | ||
| signed_block.* = ct.capella.SignedBlindedBeaconBlock.default_value; | ||
| try ct.capella.SignedBlindedBeaconBlock.deserializeFromBytes(allocator, bytes, signed_block); | ||
| return .{ .blinded_capella = signed_block }; | ||
| } | ||
| }, | ||
| .deneb => { | ||
| if (block_type == .full) { | ||
| const signed_block = try allocator.create(ct.deneb.SignedBeaconBlock.Type); | ||
| errdefer allocator.destroy(signed_block); | ||
| signed_block.* = ct.deneb.SignedBeaconBlock.default_value; | ||
| try ct.deneb.SignedBeaconBlock.deserializeFromBytes(allocator, bytes, signed_block); | ||
| return .{ .full_deneb = signed_block }; | ||
| } else { | ||
| const signed_block = try allocator.create(ct.deneb.SignedBlindedBeaconBlock.Type); | ||
| errdefer allocator.destroy(signed_block); | ||
| signed_block.* = ct.deneb.SignedBlindedBeaconBlock.default_value; | ||
| try ct.deneb.SignedBlindedBeaconBlock.deserializeFromBytes(allocator, bytes, signed_block); | ||
| return .{ .blinded_deneb = signed_block }; | ||
| } | ||
| }, | ||
| .electra => { | ||
| if (block_type == .full) { | ||
| const signed_block = try allocator.create(ct.electra.SignedBeaconBlock.Type); | ||
| errdefer allocator.destroy(signed_block); | ||
| signed_block.* = ct.electra.SignedBeaconBlock.default_value; | ||
| try ct.electra.SignedBeaconBlock.deserializeFromBytes(allocator, bytes, signed_block); | ||
| return .{ .full_electra = signed_block }; | ||
| } else { | ||
| const signed_block = try allocator.create(ct.electra.SignedBlindedBeaconBlock.Type); | ||
| errdefer allocator.destroy(signed_block); | ||
| signed_block.* = ct.electra.SignedBlindedBeaconBlock.default_value; | ||
| try ct.electra.SignedBlindedBeaconBlock.deserializeFromBytes(allocator, bytes, signed_block); | ||
| return .{ .blinded_electra = signed_block }; | ||
| } | ||
| }, | ||
| .fulu => { | ||
| if (block_type == .full) { | ||
| const signed_block = try allocator.create(ct.fulu.SignedBeaconBlock.Type); | ||
| errdefer allocator.destroy(signed_block); | ||
| signed_block.* = ct.fulu.SignedBeaconBlock.default_value; | ||
| try ct.fulu.SignedBeaconBlock.deserializeFromBytes(allocator, bytes, signed_block); | ||
| return .{ .full_fulu = signed_block }; | ||
| } else { | ||
| const signed_block = try allocator.create(ct.fulu.SignedBlindedBeaconBlock.Type); | ||
| errdefer allocator.destroy(signed_block); | ||
| signed_block.* = ct.fulu.SignedBlindedBeaconBlock.default_value; | ||
| try ct.fulu.SignedBlindedBeaconBlock.deserializeFromBytes(allocator, bytes, signed_block); | ||
| return .{ .blinded_fulu = signed_block }; | ||
| } | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
This function exceeds the 70-line limit specified in the style guide (line 104). Please refactor it to be shorter. You could extract the logic for each fork into a separate helper function.
References
- Functions should not exceed a hard limit of 70 lines to reduce complexity and improve readability. (link)
| pub fn serialize(self: AnySignedBeaconBlock, allocator: std.mem.Allocator) ![]u8 { | ||
| switch (self) { | ||
| .phase0 => |signed_block| { | ||
| const out = try allocator.alloc(u8, ct.phase0.SignedBeaconBlock.serializedSize(signed_block)); | ||
| errdefer allocator.free(out); | ||
| _ = ct.phase0.SignedBeaconBlock.serializeIntoBytes(signed_block, out); | ||
| return out; | ||
| }, | ||
| .altair => |signed_block| { | ||
| const out = try allocator.alloc(u8, ct.altair.SignedBeaconBlock.serializedSize(signed_block)); | ||
| errdefer allocator.free(out); | ||
| _ = ct.altair.SignedBeaconBlock.serializeIntoBytes(signed_block, out); | ||
| return out; | ||
| }, | ||
| .full_bellatrix => |signed_block| { | ||
| const out = try allocator.alloc(u8, ct.bellatrix.SignedBeaconBlock.serializedSize(signed_block)); | ||
| errdefer allocator.free(out); | ||
| _ = ct.bellatrix.SignedBeaconBlock.serializeIntoBytes(signed_block, out); | ||
| return out; | ||
| }, | ||
| .blinded_bellatrix => |signed_block| { | ||
| const out = try allocator.alloc(u8, ct.bellatrix.SignedBlindedBeaconBlock.serializedSize(signed_block)); | ||
| errdefer allocator.free(out); | ||
| _ = ct.bellatrix.SignedBlindedBeaconBlock.serializeIntoBytes(signed_block, out); | ||
| return out; | ||
| }, | ||
| .full_capella => |signed_block| { | ||
| const out = try allocator.alloc(u8, ct.capella.SignedBeaconBlock.serializedSize(signed_block)); | ||
| errdefer allocator.free(out); | ||
| _ = ct.capella.SignedBeaconBlock.serializeIntoBytes(signed_block, out); | ||
| return out; | ||
| }, | ||
| .blinded_capella => |signed_block| { | ||
| const out = try allocator.alloc(u8, ct.capella.SignedBlindedBeaconBlock.serializedSize(signed_block)); | ||
| errdefer allocator.free(out); | ||
| _ = ct.capella.SignedBlindedBeaconBlock.serializeIntoBytes(signed_block, out); | ||
| return out; | ||
| }, | ||
| .full_deneb => |signed_block| { | ||
| const out = try allocator.alloc(u8, ct.deneb.SignedBeaconBlock.serializedSize(signed_block)); | ||
| errdefer allocator.free(out); | ||
| _ = ct.deneb.SignedBeaconBlock.serializeIntoBytes(signed_block, out); | ||
| return out; | ||
| }, | ||
| .blinded_deneb => |signed_block| { | ||
| const out = try allocator.alloc(u8, ct.deneb.SignedBlindedBeaconBlock.serializedSize(signed_block)); | ||
| errdefer allocator.free(out); | ||
| _ = ct.deneb.SignedBlindedBeaconBlock.serializeIntoBytes(signed_block, out); | ||
| return out; | ||
| }, | ||
| .full_electra => |signed_block| { | ||
| const out = try allocator.alloc(u8, ct.electra.SignedBeaconBlock.serializedSize(signed_block)); | ||
| errdefer allocator.free(out); | ||
| _ = ct.electra.SignedBeaconBlock.serializeIntoBytes(signed_block, out); | ||
| return out; | ||
| }, | ||
| .blinded_electra => |signed_block| { | ||
| const out = try allocator.alloc(u8, ct.electra.SignedBlindedBeaconBlock.serializedSize(signed_block)); | ||
| errdefer allocator.free(out); | ||
| _ = ct.electra.SignedBlindedBeaconBlock.serializeIntoBytes(signed_block, out); | ||
| return out; | ||
| }, | ||
| .full_fulu => |signed_block| { | ||
| const out = try allocator.alloc(u8, ct.fulu.SignedBeaconBlock.serializedSize(signed_block)); | ||
| errdefer allocator.free(out); | ||
| _ = ct.fulu.SignedBeaconBlock.serializeIntoBytes(signed_block, out); | ||
| return out; | ||
| }, | ||
| .blinded_fulu => |signed_block| { | ||
| const out = try allocator.alloc(u8, ct.fulu.SignedBlindedBeaconBlock.serializedSize(signed_block)); | ||
| errdefer allocator.free(out); | ||
| _ = ct.fulu.SignedBlindedBeaconBlock.serializeIntoBytes(signed_block, out); | ||
| return out; | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
This function exceeds the 70-line limit specified in the style guide (line 104). Please refactor it to be shorter, for example by extracting the logic for each fork into a helper function.
References
- Functions should not exceed a hard limit of 70 lines to reduce complexity and improve readability. (link)
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cae60996ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Chen Kai <281165273grape@gmail.com>
| @@ -0,0 +1,24 @@ | |||
| const ct = @import("consensus_types"); | |||
|
|
|||
| pub const AnyAttestations = union(enum) { | |||
There was a problem hiding this comment.
Is there a benefit to the Any prefix for this (and all other types)? Most data structures differ by fork types anyway, so I'm not sure if the Any prefix is helpful
Feels like we can either go with:
ForkBeaconStateversusBeaconState, orBeaconStateversusAnyBeaconState
There was a problem hiding this comment.
Preference for 2. or keeping as is
| full_bellatrix: *ct.bellatrix.SignedBeaconBlock.Type, | ||
| blinded_bellatrix: *ct.bellatrix.SignedBlindedBeaconBlock.Type, |
There was a problem hiding this comment.
nit: I personally prefer <name>_<variant> for naming, think it makes more sense naming wise for fork to come before the type of block since we have alphabetical ordering.
| full_bellatrix: *ct.bellatrix.SignedBeaconBlock.Type, | |
| blinded_bellatrix: *ct.bellatrix.SignedBlindedBeaconBlock.Type, | |
| bellatrix_full: *ct.bellatrix.SignedBeaconBlock.Type, | |
| bellatrix_blinded: *ct.bellatrix.SignedBlindedBeaconBlock.Type, |
There was a problem hiding this comment.
One thing that needs to be more consistent in this PR is the ordering between block type and fork.
Its not clear that fork needs to be first, nor vice versa.
I think that block type being primary reads slightly better, and would like to see (bt, f) everywhere. Eg BeaconBlockBody(.full, .fulu)
There was a problem hiding this comment.
made that update, now (bt, f) is consistently everywhere.
Signed-off-by: Chen Kai <281165273grape@gmail.com>
spiral-ladder
left a comment
There was a problem hiding this comment.
lgtm w some comments
| }; | ||
| } | ||
|
|
||
| pub fn BeaconBlock(comptime bt: BlockType, comptime f: ForkSeq) type { |
There was a problem hiding this comment.
nit and maybe in a followup PR: should we capitalize comptime types? eg. BT and F
There was a problem hiding this comment.
I think we should only capitalize types
| } | ||
|
|
||
| pub fn nextWithdrawalIndex(self: *Self) !u64 { | ||
| if (comptime (f == .phase0 or f == .altair or f == .bellatrix)) return error.InvalidAtFork; |
There was a problem hiding this comment.
can we use lt() for these and the other checks? Seems pretty long and unreadable
There was a problem hiding this comment.
yeah, current impl isn't great. (currently unused codepath)
| var state_eth1_data_view = try state.eth1Data(); | ||
| var state_eth1_data: Eth1Data = undefined; | ||
| try state_eth1_data_view.toValue(allocator, &state_eth1_data); | ||
| try state_eth1_data_view.toValue(undefined, &state_eth1_data); |
There was a problem hiding this comment.
i know this is because this doesn't require an allocator but this seems like a smell maybe for a future refactor
There was a problem hiding this comment.
yes this is annoying, this is why the ssz types are split into fixed and variable types, so that we avoid this ^. (eg ct.phase0.Checkpoint.deserializeFromBytes doesn't take an allocator bc of the FixedContainerType function signature).
Our tree view code is not splt between fixed and variable, so we have situations like this...
There was a problem hiding this comment.
would there be any value in having toValue and toValueAlloc?
fork_typesmoduleAny*types. OnlyAnySignedBeaconBlockandAnyBeaconStateare currently usedForkTypes(f)(for all types) andFork*for block and state, eg:ForkBeaconState(comptime f: ForkSeq)comptime f: ForkSeqinternally. The top-level stfn takesAnyBeaconState/CachedBeaconStateandAnySignedBeaconBlock, and then does a single dispatch into fork-specific code.CachedBeaconStatefrom being passed deeply into utility functions and only pass the pieces required per function, Eg: converting functions likegetFinalityDelay(cached_state: *CachedBeaconState) !u64intogetFinalityDelay(current_epoch: u64, finalized_epoch: u64) u64