Skip to content

feat(STF): implement Gloas Fork - #462

Closed
guha-rahul wants to merge 36 commits into
ChainSafe:mainfrom
guha-rahul:gloas
Closed

feat(STF): implement Gloas Fork #462
guha-rahul wants to merge 36 commits into
ChainSafe:mainfrom
guha-rahul:gloas

Conversation

@guha-rahul

@guha-rahul guha-rahul commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

This pr implements

@guha-rahul
guha-rahul requested a review from a team as a code owner July 2, 2026 10:09
@guha-rahul
guha-rahul marked this pull request as draft July 5, 2026 10:35

@spiral-ladder spiral-ladder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first pass, mostly on utils/gloas.zig

Comment thread src/constants/root.zig Outdated
Comment thread src/state_transition/utils/gloas.zig
Comment thread src/state_transition/utils/gloas.zig Outdated
Comment thread src/state_transition/utils/gloas.zig Outdated
}

pub fn canBuilderCoverBid(allocator: Allocator, state: *BeaconState(.gloas), builder_index: u64, bid_amount: u64) !bool {
var builders = try state.inner.get("builders");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var builders = try state.inner.get("builders");
var builders = try state.inner.getReadonly("builders");

Comment thread src/state_transition/utils/gloas.zig Outdated
Comment thread src/state_transition/utils/pending_deposits_lookup.zig Outdated
Comment thread src/state_transition/utils/pending_deposits_lookup.zig
Comment thread src/state_transition/cache/epoch_cache.zig Outdated
var data: PtcWindowEpochCacheData = undefined;

for (0..preset.SLOTS_PER_EPOCH) |i| {
try ptc_window.getValue(undefined, i, &data.previous[i]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of scope of this PR, but would be nice if we had getReadonlyByRange for this usecase

Comment thread src/state_transition/cache/epoch_cache.zig Outdated
@spiral-ladder

Copy link
Copy Markdown
Member

One overarching thought that is out of scope of this PR is that I still kinda dislike the whole dumping of everything into utils/gloas.zig. We should probably separate the functions meaningfully by name. With that said that's not required here, in fact the current form makes it easier to review, but I'd like to have a followup PR to rearrange stuff eventually

@spiral-ladder

Copy link
Copy Markdown
Member

#464 might've broken nextValue for this PR - refactored it to make use of some comptime facts to be more explicit, see PR for details

@GrapeBaBa

Copy link
Copy Markdown
Contributor

I suggest to add some OOM/Double free fault injection test using the test_allocators and also run process_epoch/process_block bench before and after this change to see if any regression

@guha-rahul
guha-rahul marked this pull request as ready for review August 11, 2026 13:55
@guha-rahul

Copy link
Copy Markdown
Contributor Author

I suggest to add some OOM/Double free fault injection test using the test_allocators and also run process_epoch/process_block bench before and after this change to see if any regression

I did the benchmarking and found near to no regresssions.

@spiral-ladder spiral-ladder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed process_withdrawals.zig and utils/gloas.zig

Comment thread src/state_transition/block/process_withdrawals.zig
Comment thread src/state_transition/block/process_withdrawals.zig
const processed_partial_withdrawals_count = expected_withdrawals_result.processed_partial_withdrawals_count;
const expected_withdrawals = expected_withdrawals_result.withdrawals.items;
const num_withdrawals = expected_withdrawals.len;
// [New in EIP-7732] Return early if parent block is empty

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do without the [new in ...] comments

Suggested change
// [New in EIP-7732] Return early if parent block is empty

Comment on lines +103 to +105
const latest_withdrawal = if (expected_withdrawals.len > 0) expected_withdrawals[expected_withdrawals.len - 1] else null;
if (latest_withdrawal) |lw| {
try state.setNextWithdrawalIndex(lw.index + 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably skip the optional here

Suggested change
const latest_withdrawal = if (expected_withdrawals.len > 0) expected_withdrawals[expected_withdrawals.len - 1] else null;
if (latest_withdrawal) |lw| {
try state.setNextWithdrawalIndex(lw.index + 1);
if (expected_withdrawals.len > 0) {
try state.setNextWithdrawalIndex(expected_withdrawals[expected_withdrawals.len - 1].index + 1);
}

later below expected_withdrawals.len == preset.MAX_WITHDRAWALS_PER_PAYLOAD check necessarily means expected_withdrawals.len > 0

Comment on lines -85 to +125
/// Called by the block proposer to find a list of withdrawals to include in the block.
///
/// This list is assumed to be bounded by `preset.MAX_WITHDRAWALS_PER_PAYLOAD`.
///
/// Caller should deinit `withdrawal_balances` with .deinit() after use.
// Consumer should deinit WithdrawalsResult with .deinit() after use

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this removed? it's also not a doc comment (triple slash instead of double slash)

/// Convert a builder index to a flagged validator index for use in Withdrawal containers.
/// Spec: https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.1/specs/gloas/beacon-chain.md#new-convert_builder_index_to_validator_index
pub fn convertBuilderIndexToValidatorIndex(builder_index: u64) u64 {
return if (hasBuilderIndexFlag(builder_index)) builder_index else builder_index | c.BUILDER_INDEX_FLAG;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is idempotent so we don't need the if check

Suggested change
return if (hasBuilderIndexFlag(builder_index)) builder_index else builder_index | c.BUILDER_INDEX_FLAG;
return builder_index | c.BUILDER_INDEX_FLAG;

/// Convert a flagged validator index back to a builder index.
/// Spec: https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.1/specs/gloas/beacon-chain.md#new-convert_validator_index_to_builder_index
pub fn convertValidatorIndexToBuilderIndex(validator_index: u64) u64 {
return if (hasBuilderIndexFlag(validator_index)) validator_index & ~c.BUILDER_INDEX_FLAG else validator_index;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return if (hasBuilderIndexFlag(validator_index)) validator_index & ~c.BUILDER_INDEX_FLAG else validator_index;
return validator_index & ~c.BUILDER_INDEX_FLAG;


/// Check if a validator index represents a builder (has the builder flag set).
/// Spec: https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.1/specs/gloas/beacon-chain.md#new-is_builder_index
pub fn isBuilderIndex(validator_index: u64) bool {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just wrapping hasBuilderIndexFlag? We should move that here instead to align with spec

};

var domain: ct.primitive.Domain.Type = undefined;
computeDomain(c.DOMAIN_BUILDER_DEPOSIT, config.chain.GENESIS_FORK_VERSION, c.ZERO_HASH, &domain) catch return false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit long, add a trailing comma and fmt

Suggested change
computeDomain(c.DOMAIN_BUILDER_DEPOSIT, config.chain.GENESIS_FORK_VERSION, c.ZERO_HASH, &domain) catch return false;
computeDomain(c.DOMAIN_BUILDER_DEPOSIT, config.chain.GENESIS_FORK_VERSION, c.ZERO_HASH, &domain,) catch return false;

Comment on lines +251 to +252

pub fn isAttestationSameSlotRootCache(root_cache: *RootCache(.gloas), data: *const ct.phase0.AttestationData.Type) !bool {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should maybe even be in root_cache.zig

Suggested change
pub fn isAttestationSameSlotRootCache(root_cache: *RootCache(.gloas), data: *const ct.phase0.AttestationData.Type) !bool {
/// Use cached block roots to avoid repeated state root lookups while matching the spec's is_attestation_same_slot behavior.
pub fn isAttestationSameSlotRootCache(root_cache: *RootCache(.gloas), data: *const ct.phase0.AttestationData.Type) !bool {

@spiral-ladder

spiral-ladder commented Aug 23, 2026

Copy link
Copy Markdown
Member

We should also keep this aligned with the spec test version in https://github.com/ChainSafe/lodestar/blob/62b30e7337e3bbc1098037b0e856ef6b2f106466/spec-tests-version.json

edit: but this will need #99

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants