Skip to content

Gloas alpha spec 8#9315

Open
eserilev wants to merge 48 commits into
sigp:unstablefrom
eserilev:gloas-alpha-spec-8
Open

Gloas alpha spec 8#9315
eserilev wants to merge 48 commits into
sigp:unstablefrom
eserilev:gloas-alpha-spec-8

Conversation

@eserilev
Copy link
Copy Markdown
Member

@eserilev eserilev requested a review from jxs as a code owner May 18, 2026 12:20
@eserilev eserilev added the gloas label May 18, 2026
@eserilev eserilev added the ready-for-review The code is ready for review label May 18, 2026
@mergify
Copy link
Copy Markdown

mergify Bot commented May 18, 2026

Some required checks have failed. Could you please take a look @eserilev? 🙏

@mergify mergify Bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 18, 2026
@eserilev eserilev added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels May 19, 2026
Comment on lines +104 to +108
// Set a non-zero gas_limit on latest_execution_payload_bid so the gas limit
// compatibility check doesn't reject all bids at genesis.
if let Ok(bid) = state.latest_execution_payload_bid_mut() {
bid.gas_limit = 30_000_000;
}
Copy link
Copy Markdown
Member

@michaelsproul michaelsproul May 20, 2026

Choose a reason for hiding this comment

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

I wonder if this should be part of the interop spec for Gloas genesis. It seems quite important.

Copy link
Copy Markdown
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Partial review, will complete shortly.

Comment thread consensus/types/src/core/chain_spec.rs
Comment thread beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs Outdated
Comment thread beacon_node/beacon_chain/src/beacon_chain.rs Outdated
Comment thread beacon_node/beacon_chain/src/beacon_chain.rs Outdated
Comment thread beacon_node/execution_layer/src/test_utils/mock_builder.rs
parent_beacon_block_root,
}),
// V4 maps to V3 for SSE (slot_number is not part of the SSE spec)
// V4 maps to V3 for SSE (slot_number/target_gas_limit are not part of the SSE spec)
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.

Should probably fix this at the spec level (cc @chong-he 🙏 )

Comment thread consensus/fork_choice/src/fork_choice.rs
Comment thread consensus/fork_choice/src/fork_choice.rs Outdated
Copy link
Copy Markdown
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Have reviewed everything except the consensus changes now.

Comment thread consensus/proto_array/src/proto_array.rs Outdated
Comment thread consensus/proto_array/src/proto_array.rs
Comment thread consensus/proto_array/src/proto_array.rs Outdated
Comment thread consensus/proto_array/src/proto_array.rs Outdated
Comment thread consensus/proto_array/src/proto_array.rs Outdated
Comment thread consensus/proto_array/src/proto_array.rs Outdated
Comment thread consensus/proto_array/src/proto_array.rs
Comment thread testing/ef_tests/src/cases/fork_choice.rs Outdated
Comment thread testing/ef_tests/src/cases/fork_choice.rs Outdated
@mergify
Copy link
Copy Markdown

mergify Bot commented May 20, 2026

Some required checks have failed. Could you please take a look @eserilev? 🙏

@mergify mergify Bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 20, 2026
@eserilev eserilev requested a review from pawanjay176 May 20, 2026 14:41

if proposal_epoch < current_epoch || proposal_epoch > current_epoch.saturating_add(1u64) {
if proposal_epoch < current_epoch
|| proposal_epoch > current_epoch.saturating_add(spec.min_seed_lookahead)
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.

ethereum/consensus-specs#5215 also says that

is_valid_proposal_slot(state, preferences) returns True, where
state is the checkpoint state at the epoch compute_epoch_at_slot(preferences.proposal_slot) - MIN_SEED_LOOKAHEAD and
the root preferences.dependent_root.

We use the head_state here which seems correct and given our previous discussion on not wanting to accept all preferences that force us to load old states. I think that is fine. Do you think this changes?

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 also need to update BeaconState::is_valid_proposal_slot to use min_seed_lookahead right?

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.

The PR also changes it in get_upcoming_proposal_slots but I don't see any direct usage of it in our codebase. This spec change is so annoying.

Copy link
Copy Markdown
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

I have reviewed the non fork choice changes and they look good. Mostly nits

execution_layer
.get_proposer_gas_limit(proposer_index)
.await
.unwrap_or(parent_gas_limit),
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.

Shouldn't this be the default that we have configured in lighthouse instead of the parent gas limit?

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.

Yeah agree, I think there's no harm in setting a higher gas limit here, because this is just the target. It also solves the problem I found which is that this isn't actually the parent_gas_limit anyway:

#9315 (comment)

"No proposer gas limit configured, falling back to parent gas limit"
);
}
proposer_gas_limit.or(Some(pre_payload_attributes.parent_gas_limit))
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 couldn't find the relevant part of the spec where we fall back to the parent gas limit instead of the configured default in lighthouse

Copy link
Copy Markdown
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks good. I think maybe we just add a TODO for the parent gas limit issue I raised, seeing as it is only used as a fallback, and even then is only used as a target gas limit (it can't break anything I don't think).

Comment on lines +5087 to +5091
cached_head
.snapshot
.beacon_state
.latest_execution_payload_bid()?
.gas_limit
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 this is wrong in the case where we build on the Full variant of the parent. The payload has not yet been applied to state in this case, so its gas limit won't be stored in state.latest_execution_payload_bid.

I think we would need to thread the proposer_head_payload_status through here, or maybe just the proposer head payload envelope, and then load the gas limit from that envelope.

@michaelsproul
Copy link
Copy Markdown
Member

Actually, Pawan's suggestion is even cleaner. Get rid of parent_gas_limit from PrePayloadAttributes entirely and just use the process-level default gas limit as fallback.

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

Labels

gloas waiting-on-author The reviewer has suggested changes and awaits thier implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants