feat(STF): implement Gloas Fork #303
Conversation
Summary of ChangesHello, 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 implements the 'Gloas' fork, a significant update to the consensus layer. It introduces new data structures for builder management, payload attestations, and execution payload bids, while refactoring existing state transition logic to support these changes. The update also includes necessary adjustments to the test suite and configuration to align with the latest consensus specifications. 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. 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 the 'gloas' fork, implementing EIP-7732 changes including builder registry management, payload timeliness committees (PTC), and updated withdrawal processing. The changes involve adding new consensus types, updating state transition logic for blocks and epochs, and integrating new signature verification sets. I have reviewed the code and provided feedback on performance optimizations, style guide compliance, and safety assertions. Specifically, I have flagged issues regarding linear scans in the builder registry, unbounded loops in PTC computation, inefficient memory allocation in attestation processing, and naming/typing inconsistencies.
| try builders.setValue(builder_index, &builder); | ||
| } | ||
|
|
||
| pub fn findBuilderIndexByPubkey(allocator: Allocator, state: *BeaconState(.gloas), pubkey: *const BLSPubkey) !?usize { |
There was a problem hiding this comment.
This function performs an BUILDER_REGISTRY_LIMIT), this will become a significant performance bottleneck. Consider implementing a more efficient lookup mechanism, such as a cache or a hash-based index, to maintain mechanical sympathy and performance.
| var random_bytes: [32]u8 = undefined; | ||
| var last_block: u64 = std.math.maxInt(u64); | ||
|
|
||
| while (result_len < preset.PTC_SIZE) { |
There was a problem hiding this comment.
This loop lacks a fixed upper bound, which poses a risk of excessive execution time or infinite loops if the input data (e.g., effective balances) does not satisfy the sampling criteria. The style guide requires all loops to have a fixed upper bound to prevent tail latency spikes and ensure bounded execution.
References
- All loops must have a fixed upper bound to prevent infinite loops or tail latency spikes. (link)
| // TODO: metrics — newSeenAttesters, newSeenAttestersEffectiveBalance, attestationsPerBlock | ||
| var proposer_reward: u64 = 0; | ||
|
|
||
| var builder_weight_map = std.AutoHashMap(u64, u64).init(allocator); |
There was a problem hiding this comment.
Using AutoHashMap with heap allocation inside a block processing function is inefficient. Since the number of indices is small and fixed (2 * preset.SLOTS_PER_EPOCH), a stack-allocated array or a pre-allocated buffer would be more performant and align better with the 'Safety' design goals regarding memory allocation.
References
- Most memory should be statically allocated at startup where possible and optimal to improve performance and safety. (link)
| else | ||
| data.slot % preset.SLOTS_PER_EPOCH; | ||
|
|
||
| var builder_pending_payments = try state.inner.get("builder_pending_payments"); |
There was a problem hiding this comment.
Fetching the builder_pending_payments field from the state inside the attestations loop is inefficient. This should be moved outside the loop to avoid redundant state access operations and potentially shared with the batch write logic after the loop.
| signature: BLSSignature, | ||
| slot: u64, | ||
| ) !void { | ||
| const builderIndex = try findBuilderIndexByPubkey(allocator, state, pubkey); |
There was a problem hiding this comment.
Variable name builderIndex violates the repository style guide, which requires snake_case for variable names.
const builder_index = try findBuilderIndexByPubkey(allocator, state, pubkey);
References
- Use snake_case for file names and variable names. (link)
| const len = try builders.length(); | ||
| const current_epoch = computeEpochAtSlot(try state.slot()); | ||
|
|
||
| var builderIndex: ?usize = null; |
There was a problem hiding this comment.
Use of architecture-specific usize for a variable that represents a registry index is discouraged. The style guide recommends using explicitly-sized types like u32 or u64 for everything.
var builder_index: ?u64 = null;
References
- Use explicitly-sized types like u32 for everything, avoid architecture-specific usize. (link)
| const verify = @import("../utils/bls.zig").verify; | ||
| const getExecutionPayloadBidSigningRoot = @import("../signature_sets/execution_payload_bid.zig").getExecutionPayloadBidSigningRoot; | ||
|
|
||
| pub fn processExecutionPayloadBid( |
There was a problem hiding this comment.
This function lacks assertions for its input arguments. The repository style guide mandates that all function arguments, pre/postconditions, and invariants must be asserted to detect programmer errors early and increase the probability of correctness.
References
- Assert all function arguments and return values, pre/postconditions and invariants. (link)
| } | ||
| // TODO: Gloas - Optimization needed | ||
| const total_payments = @TypeOf(builderPendingPayments.*).length; | ||
| for (0..total_payments) |i| { |
There was a problem hiding this comment.
Performing getValue and setValue on a TreeView inside a loop is inefficient due to the overhead of Merkle tree operations. Consider reading the entire vector into a local buffer, modifying it, and writing it back in a single operation to improve performance.
guha-rahul
changed the title
| const pending_deposits_len = try pending_deposits.length(); | ||
| var pending_it = pending_deposits.iteratorReadonly(0); | ||
|
|
||
| for (0..pending_deposits_len) |_| { |
There was a problem hiding this comment.
@guha-rahul I think we need to more carefully think through the bottlenecks in gloas state transition which even in lodestar itself is not yet optimized since we haven't run it on any network with meaningful load, eg. the code here does 50k signature verifications for each deposit request on mainnet, this is pretty redundant and will not work, ChainSafe/lodestar#9180 has this optimized to only do it once per payload, but even that I think is insufficient, also we need to consider batching here
either way, also what I mentioned in standup, we need to look at the performance issues and come up with improvements / caching etc. I am pretty sure we can port the ts implementation improvements to zig as well
2 participants
This pr implements
v1.7.0-alpha.4