feat(beacon): gloas builder market: bid and preference gossip, endpoints, building on bids

[MegaRedHand]

Motivation

Gloas makes payload building a market: builders gossip signed bids on execution_payload_bid, proposers gossip signed proposer_preferences (fee recipient, gas target) so builders can bid for their slots, and a proposer may build on the best bid instead of its own payload. Our node subscribed to neither topic, served none of the endpoints (lighthouse and prysm validator clients already call POST /eth/v1/validator/proposer_preferences and got a 404), and always self-built. This adds the node side of the market. Stacked on #667.

Changes

Piece	What
Gossip	execution_payload_bid and proposer_preferences under gloas digests: every rule of gloas p2p-interface.md, seen caches, their own validation permit pool, relayed on accept. Never queued, never sent to the chain actor.
State	One SharedBuilderMarket shared by p2p and rpc: proposer preferences per proposal slot, the best bids per slot and parent, and the known execution payloads the bid rules check parents against (gossip envelopes plus the node's own). Bounded and pruned.
Endpoints	POST /eth/v1/beacon/execution_payload_bids (JSON or SSZ, gossip-validated, published), POST /eth/v1/validator/proposer_preferences (verified, cached, published), POST /eth/v1/beacon/states/{state_id}/builders (filterable builder registry).
produceBlockV4	Honors BuilderConfig: the best gossiped bid competes with the local build under min_bid and builder_boost_factor (local wins ties, shouldOverrideBuilder respected). A winning bid returns the block only (no envelope, Eth-Execution-Payload-Included: false); the builder releases the envelope. A bid that fails to assemble falls back to the local build.
Self-build	From gloas, the fee recipient and gas target come from the proposer's signed preferences, then prepare_beacon_proposer, as beacon-APIs requires.
Types	Builder integers are quoted in JSON (they were bare, also in the gloas state JSON); preferences deserialize.

Phase 2 (not here): the builder API (bids fetched from BuilderConfig.builders URLs, POST /eth/v1/validator/builder_preferences, forwarding the block to the winning builder). That route is a 404 for now.

Deviations (in docs/spec_deviations.md)

• Preference signatures are accepted under the spec's lookahead-state domain and the schedule's fork at P - MIN_SEED_LOOKAHEAD and at P, which coincide outside a fork's first epoch: lighthouse signs with the fork at P, the spec with P - 1, and refusing either would drop honest preferences at the gloas boundary.
• Stateful checks read cached states only (a miss is IGNORE), and the head fast path serves preferences whose dependent block's state was evicted.
• A pre-gloas parent's payload counts as known.

Validation

• Spec gossip vectors v1.7.0-beta.2 mainnet: gossip_execution_payload_bid 45/45, gossip_proposer_preferences 27/27 (previously ignored). Minimal not run locally.
• Unit tests for the market, every rule (including fulu-parent boundary cases and the domains), assembly on a bid (process_block accepts it), topics, decode, verdict settle, publish, bid selection, BuilderConfig, and router tests driving every endpoint and produceBlockV4 against a fake execution client.
• --lib: state-transition 544, types 221, p2p 312, rpc 234, engine 27, validator 269, blockchain 236, storage 152; clippy and fmt clean.

[github-actions]

🤖 Claude Code Review

Review of PR 669: gloas builder market (bids, proposer preferences, produceBlockV4 bid selection)

Coverage: I only saw part of the diff. The saved copy was cut at about 60 KB. I read builder_market.rs, gloas_block_production.rs, and execution_payload_bid.rs up to the stateful rules (roughly the first 1,430 lines). I did not read the rest of the stateful rules, the proposer-preferences gossip, the p2p wiring, the RPC endpoints, or the produceBlockV4 selection logic. Nothing below speaks to those. I did not build the code or run any tests.

Overall

What I read is well structured. The cheap/stateful split follows the existing envelope pattern. The bounds are explicit: seen keys per slot, bids per parent, preferences and known payloads. The "never queue, cached state only, a miss means IGNORE" policy is documented as a deviation. The lock-poison handling is reasonable, since every mutation is a single insert or remove.

Findings

1. Possible wrong exit set in parent_payload_exits (execution_payload_bid.rs, ~1318-1329).

   • The market lookup correctly requires beacon_block_root == bid.parent_block_root.
   • The fallback does not. It loads the stored envelope for bid.parent_block_root and returns its builder_exits without checking that envelope.payload.block_hash == bid.parent_block_hash.
   • If the bid builds on the parent's EMPTY branch (the grandparent's payload hash), the parent block's own envelope may still be stored. This happens when the envelope arrives late.
   • Rule 21 would then judge the bid against exits from a payload it does not build on. That could wrongly REJECT an honest builder or wrongly accept a bid.
   • Fix: compare the envelope's block hash to bid.parent_block_hash before using it. Otherwise return None.

2. Gossip path can run a full epoch transition (cached_checkpoint_state, ~1285-1292).

   • When a bid's epoch differs from the parent state's epoch, the code clones the state and calls process_slots to the epoch boundary.
   • The cost of that transition is not bounded by what has been checked at that point.
   • It runs on a blocking thread, and I did not see how many are allowed at once. Only the bounded permit pool, the bid-seen rules, and the preferences check come before it.
   • Please confirm the permit pool is small enough that repeated unseen-parent/epoch combinations cannot starve verdicts. Caching the result already helps.
   • If bids from a past-epoch parent are rare, consider checking cache-or-skip before advancing.

3. record_bid leaves an empty slot entry when the cap is hit (builder_market.rs, 208-211).

   • pool.slots.entry(bid_slot).or_default() runs before the MAX_SEEN_BID_KEYS_PER_SLOT check. Today the seen.len() check cannot fire on a fresh entry, so this is harmless.
   • The entry is created before the later split_off, so an out-of-order low-slot bid is inserted and then possibly pruned in the same call. It still returns true (accepted and relayed) even though it was just dropped.
   • Compute keep_from first and reject slots below it.

4. record_preferences mutates before it can return false (builder_market.rs, 292-302).

   • It prunes everything below current_slot before inserting, and returns cache.contains_key(&key) to cover the case where the new key is itself stale or over the cap.
   • That is correct, but a stale message performs a prune and a no-op insert. The "false" result means both "duplicate" and "dropped by the cap". Callers may treat these differently, for example IGNORE versus still relaying.
   • It would be worth documenting which case callers can distinguish.

5. Minor.

   • bid_is_includable only applies the exited_by_parent check when parent_is_full, which is right.
   • It does not check execution_payment == 0. process_block will catch it, so this is only a missed pre-filter.

Tests

Coverage of the market and bid assembly is good. It includes the truncated best-value bar, the per-slot key cap, the LRU eviction, and the exit-by-parent case. Add a test for Point 1 where the parent envelope exists but the bid builds on the grandparent's payload hash.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

• I found 2 correctness concerns worth addressing; otherwise the overall structure looks careful, especially the split between cheap/stateful gossip checks and the bounded in-memory market.

1. Potential underflow in gas-limit compatibility check

• In crates/blockchain/state_transition/src/beacon/gossip/execution_payload_bid.rs:59, let min_gas_limit = parent_gas_limit - max_difference; can underflow in debug builds when parent_gas_limit == 0 because max_difference becomes 0 and the subtraction is safe there, but for very small values like 1, max_difference is still 0; the real hazard is more generally that this branch relies on non-obvious invariants and uses plain subtraction right after a saturating calculation. Given this is consensus-path validation code, I’d strongly prefer saturating_sub here for robustness and clarity:
  • let min_gas_limit = parent_gas_limit.saturating_sub(max_difference);
• Even if current execution payload rules make tiny gas limits unreachable in practice, avoiding raw subtraction in consensus validation is safer and more idiomatic.

2. produceBlockV4 can return 503 even when a usable bid exists

• In crates/net/rpc/src/beacon/gloas_proposal.rs:257, the early check uses market.has_bids_for_slot(slot) to decide whether a node without an execution client can serve the request.

• But has_bids_for_slot in crates/blockchain/state_transition/src/beacon/builder_market.rs:189 only checks whether any bid exists for the slot, not whether there is an includable bid for the prepared (parent_root, parent_hash) or one above min_bid.

• This can lead to a false positive in one direction today, and more importantly a false negative/poor API contract after future refactors: the endpoint may proceed past the guard, only to fail later because no usable bid matches the prepared parent. The guard should be aligned with actual selection criteria, ideally after prepare(...), by checking bids_for(slot, prepared.head_root, prepared.inputs.head_block_hash) and then applying the same bid-selection filter.

• At minimum, the current 503 condition/message is looser than what block production actually requires.

• A couple of smaller notes:

  • crates/blockchain/state_transition/src/beacon/builder_market.rs:148 sorts the full bid vector on every insert. Since MAX_BIDS_PER_PARENT is only 16, this is acceptable, but if bid churn is expected to be high, a binary insertion plus truncation would reduce per-insert work.
  • The lock poisoning recovery in crates/blockchain/state_transition/src/beacon/builder_market.rs:111 is a reasonable tradeoff here, and I like that the comment explicitly documents the assumption.

• Overall, aside from the two points above, the PR looks solid: the gossip verdict plumbing, duplicate/idempotency handling, and explicit spec-deviation documentation are all well thought out.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

🤖 Kimi Code Review

I'll review this PR systematically, focusing on correctness, security, performance, and Rust best practices for consensus-critical code.

Overview

This PR implements the Gloas builder market: gossip validation and Beacon API endpoints for execution_payload_bid and proposer_preferences, plus bid selection logic for produceBlockV4. It's a substantial change (~3,500 lines) touching p2p, RPC, state transition, and consensus containers.

---

Critical Issues

1. Race condition in bid recording (builder_market.rs:95-120)

pub fn record_bid(&self, signed: gloas::SignedExecutionPayloadBid) -> bool {
    let mut pool = lock(&self.bids);
    if pool.check(&signed.message).is_err() {  // check under lock
        return false;
    }
    // ... gap: another thread can record between check and insert
    slot.seen.insert((parent, signed.message.builder_index));  // insert under same lock

The check and insert are under the same lock, which is correct. However, check_bid_seen at line 90 is a separate public method that takes the lock independently. The race is: thread A calls check_bid_seen (passes), thread B calls record_bid (succeeds), thread A then calls record_bid (should fail but check is re-run under lock so it's safe). Actually re-reading: record_bid re-runs check under the lock, so this is fine. The separate check_bid_seen is only used by gossip's cheap checks which then call record_bid if stateful checks pass, so there's a window but it's bounded by gossipsub's single-threaded delivery per topic.

Verdict: Acceptable given gossipsub semantics, but document the assumption.

2. Integer division precision loss in bid comparison (bid_selection.rs:52-56)

const WEI_PER_WEIGHTED_GWEI: u128 = 10_000_000;  // 1e9 / 100

let weighted_local = local.value_wei / WEI_PER_WEIGHTED_GWEI;

The comment says wei / 1e9 * 100 == wei / 1e7, but this is floor division. The spec's floor(wei / 1e7) is correctly implemented, but verify this matches the reference test vectors.

Line 52-56, crates/net/rpc/src/beacon/bid_selection.rs

3. Potential panic in lock helper (builder_market.rs:82-86)

fn lock<T>(mutex: &Mutex<T>) -> MutexGuard<'_, T> {
    mutex.lock().unwrap_or_else(|poisoned| poisoned.into_inner())
}

This silently ignores poison. The comment says "every mutation here is a single insert or remove, so the data is still consistent" — this is incorrect for record_bid which does multiple operations (insert to seen, update best_value, push to bids, sort, truncate). If poisoned mid-mutation, best_value and bids can diverge.

File: crates/blockchain/state_transition/src/beacon/builder_market.rs, lines 82-86

Fix: Use parking_lot::Mutex which doesn't poison, or make lock return Result and propagate. For a consensus client, I'd prefer parking_lot or explicit recovery.

4. BLS signature verification without proper error handling (execution_payload_bid.rs:395-402)

if !matches!(
    verify_execution_payload_bid_signature(&state, signed),
    Ok(true)
) {
    return Err(reject(RejectReason::BadSignature));
}

verify_execution_payload_bid_signature can return Err for reasons other than bad signature (e.g., invalid public key format). This conflates all errors with BadSignature.

File: crates/blockchain/state_transition/src/beacon/gossip/execution_payload_bid.rs, lines 395-402

Fix: Distinguish Ok(false) (bad signature) from Err (internal error → IgnoreReason::Internal).

5. SSZ size check bypass for JSON bids (bids.rs:108-110)

if encoding == BodyEncoding::Ssz && body.len() > MAX_SIGNED_EXECUTION_PAYLOAD_BID_SIZE {
    return ApiError::BadRequest("the SignedExecutionPayloadBid exceeds its size bound")
        .into_response();
}

JSON bids aren't size-checked. A malicious JSON bid could be arbitrarily large before deserialization. The SSZ bound is 196,932 bytes; JSON with hex strings could be ~2x larger.

File: crates/net/rpc/src/beacon/bids.rs, lines 108-110

Fix: Apply a size limit regardless of encoding, or check body.len() against a generous JSON upper bound.

---

Security Issues

6. No replay protection for bid publication (bids.rs:147-157)

if !market.record_bid(bid.clone()) {
    return bad_request(describe(&Outcome::Ignore(IgnoreReason::AlreadySeen)));
}
match p2p.publish_execution_payload_bid(bid) {

An attacker can submit a valid bid, get it recorded, then resubmit after it expires from seen (slot advances, prune happens) but while still in bids. The contains_bid check at line 97 catches exact duplicates, but if the bid was pruned from seen due to slot advancement yet remains in bids (different pruning), record_bid returns false with AlreadySeen even though contains_bid would return true.

Actually re-reading: record_bid prunes slots below bid.slot - 1, and contains_bid checks bids directly. The seen set is what record_bid checks first. If a bid is in bids but its slot was pruned from seen, record_bid would create a new SlotBids entry and succeed. But contains_bid at line 97 catches this before record_bid is called.

Verdict: The contains_bid check protects against exact resubmission, but the race between contains_bid (read lock) and record_bid (write lock) is benign due to record_bid's re-check.

7. Preferences validation doesn't check slot clock bound (proposer_preferences.rs:55-58)

if is_past_slot(&config, preferences.proposal_slot, now_ms) {
    return Err(Outcome::Ignore(IgnoreReason::SlotStarted));
}

This uses MAXIMUM_GOSSIP_CLOCK_DISPARITY (500ms) but preferences should arguably be rejected, not ignored, once the slot has started — they're useless for block production.

File: crates/blockchain/state_transition/src/beacon/gossip/proposer_preferences.rs, line 55-58

This matches the spec's is_past_slot → IGNORE, so it's correct per spec. Not a security issue.

8. Builder market permits not rate-limited per peer (verdict.rs:481-485)

Validated::ExecutionPayloadBid { .. } | Validated::ProposerPreferences(_) => {
    &server.builder_validation_permits
}

A single peer can exhaust all 32 builder permits. No per-peer rate limiting.

File: crates/net/p2p/src/beacon/verdict.rs, lines 481-485

Mitigation: Consider governor or similar per-peer limits, though this is standard for the codebase's permit model.

---

Correctness Issues (Consensus-Critical)

9. is_gas_limit_target_compatible edge case (execution_payload_bid.rs:47-62)

let max_difference = (parent_gas_limit / 1024).saturating_sub(1);

When parent_gas_limit < 1024, max_difference saturates to 0. The test at line 271 verifies is_gas_limit_target_compatible(1023, 1023, 5000) is true, but what about parent_gas_limit = 0? The EIP-1559 spec says max_difference = max(1, parent_gas_limit / 1024 - 1) or similar? Need to verify against consensus spec.

File: crates/blockchain/state_transition/src/beacon/gossip/execution_payload_bid.rs, lines 47-62

The test at line 275 is_gas_limit_target_compatible(0, 0, 100) passes, but is_gas_limit_target_compatible(0, 1, 100) would return false (max_difference = 0, target > max, so gas_limit must equal max = 0). This seems correct.

10. dependent_root_at saturates to state's own block (proposer_preferences.rs:304-312)

pub fn dependent_root_at(
    state: &BeaconState,
    state_block_root: Root,
    proposal_slot: Slot,
) -> Option<Root> {
    let dependent_slot = compute_shuffling_dependent_slot(compute_epoch_at_slot(proposal_slot));
    ancestor_at(state, state_block_root, dependent_slot)
}

When dependent_slot is before the state's genesis, ancestor_at returns state_block_root (saturates). The test at line 570 verifies this. But is this correct for the spec? The spec's ancestor_at returns the block at exactly that slot, or the earliest ancestor if before genesis. Need to verify this matches the reference implementation.

File: crates/blockchain/state_transition/src/beacon/gossip/proposer_preferences.rs, lines 304-312

11. Fork choice head payload status may be stale (execution_payload_bid.rs:187-196)

let node = match (store.head().ok(), store.head_payload_status()) {
    (Some(root), Some(payload_status)) => ForkChoiceNode { root, payload_status },
    _ => fork_choice::get_head_node(store, &config).map_err(|_| internal())?,
};

If head() returns Ok but head_payload_status() is None, it falls back to get_head_node. But what if head_payload_status() is Some for a different head than store.head()? This could happen during a reorg where the head is updated but payload status isn't yet.

File: crates/blockchain/state_transition/src/beacon/gossip/execution_payload_bid.rs, lines 187-196

Fix: Verify the payload status corresponds to the head root, or always use get_head_node for consistency.

12. Bid signature domain uses None for epoch (builder_market.rs:226-236)

let domain = get_domain(state, constants::DOMAIN_BEACON_BUILDER, None);

The spec's get_domain with None uses compute_epoch_at_slot(state.slot()). For a bid at slot 64 (epoch 2) with parent state at slot 32 (epoch 1), if we use parent_state (epoch 1) directly, the domain is epoch 1's. But the bid should be signed with the bid's slot epoch? Verify against spec.

Actually, sign_bid in test_support uses get_domain(state, DOMAIN_BEACON_BUILDER, None) where state is the parent state. The spec's verify_execution_payload_bid_signature likely uses the advanced state. Need to check if stateful_checks at line 320 uses parent_state or state (advanced):

let state = if proposal_epoch == get_current_epoch(&parent_state) {
    parent_state.clone()
} else {
    cached_checkpoint_state(...)
};
// ...
if !matches!(verify_execution_payload_bid_signature(&state, signed), Ok(true))

So it uses the advanced state. The test support should match. This is test-only code, but if tests pass with parent state domain, either the domains coincide (same epoch) or the test is only testing within-epoch bids.

File: crates/blockchain/state_transition/src/beacon/builder_market.rs, lines 226-236

---

Performance Issues

13. record_bid sorts entire bid vector (builder_market.rs:112-117)

entry.bids.push(signed);
entry.bids.sort_by(|a, b| {
    b.message.value.cmp(&a.message.value)
        .then(a.message.builder_index.cmp(&b.message.builder_index))
});
entry.bids.truncate(MAX_BIDS_PER_PARENT);

Insertion sort on every bid: O(n log n) per insert, n ≤ 16. Acceptable for small n, but could use BinaryHeap or maintain sorted order on insert.

File: crates/blockchain/state_transition/src/beacon/builder_market.rs, lines 112-117

Suggestion: Use binary_search_by + insert for O(n) instead of O(n log n), or keep as-is given small bound.

14. proposer_preferences_domains allocates Vec on every call (proposer_preferences.rs:325-345)

pub fn proposer_preferences_domains(...) -> Vec<Domain> {
    // ...
    let mut distinct: Vec<Domain> = Vec::with_capacity(domains.len());
    for domain in domains {
        if !distinct.contains(&domain) {
            distinct.push(domain);
        }
    }
    distinct
}

Called for every preferences validation. At most 3 elements, so negligible. But contains is O(n²) for n=3, trivial.

File: crates/blockchain/state_transition/src/beacon/gossip/proposer_preferences.rs, lines 325-345

15. State clone in cached_checkpoint_state (execution_payload_bid.rs:248-261)

let state = if state.slot() < target_slot {
    let mut advanced = (*state).clone();
    stf::process_slots(&mut advanced, target_slot, &store.config()).ok()?;
    Arc::new(advanced)
} else {
    state
};

Clones entire state (~MBs) for checkpoint advancement. This is the standard pattern in the codebase, but note that process_slots is called on a blocking thread.

File: crates/blockchain/state_transition/src/beacon/gossip/execution_payload_bid.rs, lines 248-261

---

Rust Best Practices

16. Missing Send bound on SharedBuilderMarket (`builder_market.rs:35)

pub type SharedBuilderMarket = Arc<BuilderMarket>;

BuilderMarket contains Mutex<BidPool>, Mutex<PreferencesCache>, Mutex<LruCache<...>>. std::sync::Mutex is Send if T: Send. BTreeMap, BTreeSet, LruCache are Send. This is fine, but consider parking_lot::Mutex for better performance and no poisoning.

17. unwrap() in const context (`builder_market.rs:43)

pub const KNOWN_PAYLOADS_CAPACITY: NonZeroUsize = NonZeroUsize::new(256).unwrap();

This is fine (const-evaluated, never fails), but const_assert! or expect with message is clearer.

18. Large enum variant without boxing (`gloas_proposal.rs:376-386)

enum Production {
    Local(Produced),  // large
    Bid { block: BeaconBlock, bid: SignedExecutionPayloadBid },  // also large
}

The #[allow(clippy::large_enum_variant)] is justified by "short-lived", but Production is returned from produce() which is async and may be held across await points.

File: crates/net/rpc/src/beacon/gloas_proposal.rs, lines 376-386

---

Code Quality / Maintainability

19. Inconsistent error message format (bids.rs:55-58, proposer_preferences.rs)

pub(crate) fn describe(outcome: &Outcome) -> String {
    let (outcome, reason) = outcome.labels();
    format!("{outcome}: {reason}")
}

This produces "reject: bad_signature" but the test at builder_market_tests.rs:875 expects "reject: bad_signature". However, IgnoreReason::AlreadySeen produces "ignore: already_seen". The spec deviations doc says the API has no separate status for IGNORE, so this is intentional. But the format should be documented.

20. Dead code in BuilderConfig (`builder_config.rs:40-48)

pub(crate) struct BuilderEntry {
    // ...
    #[serde(with = "ethlambda_types::beacon::serde_helpers::quoted_or_bare")]
    pub(crate) max_execution_payment: u64,
    // ...
}

max_execution_payment is decoded but never used (builder API phase 2). Document with #[allow(dead_code)] or // TODO(phase2).

21. Test helper pollution (`gloas_block_production.rs:1173-1363)

The bid_assembly_tests module duplicates test support from builder_market::test_support. Consider consolidating.

---

Documentation Issues

22. Spec deviation doc outdated (docs/spec_deviations.md)

The PR updates this from "self-build only" to "gossip and API only", but the README.md or CLAUDE.md may still reference old behavior. The PR updates CLAUDE.md correctly.

---

Summary Table

Priority	Item	File	Line	Issue
Critical	3	builder_market.rs	82-86	Poison-ignoring lock helper
Critical	4	execution_payload_bid.rs	395-402	BLS error conflation
High	5	bids.rs	108-110	Missing JSON size limit
High	11	execution_payload_bid.rs	187-196	Stale payload status
Medium	2	bid_selection.rs	52-56	Verify division matches spec
Medium	9	execution_payload_bid.rs	47-62	Verify gas limit edge cases
Medium	13	builder_market.rs	112-117	Sorting performance
Low	18	gloas_proposal.rs	376-386	Large enum variant
Low	20	builder_config.rs	40-48	Unused field

The PR is well-structured and extensively tested. The main concerns are the

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt