[MegaRedHand]

First gloas step of the builder work: the validator client publishes signed proposer preferences (fee recipient, target gas limit) for its gloas proposals. Stacked on #705.

Why: from gloas, beacon-APIs makes SignedProposerPreferences the source of a proposer's fee recipient and gas limit, superseding prepare_beacon_proposer. A proposer without preferences also gets no trustless bids: p2p bids for its slot are ignored.

What it adds

Piece	Behavior
Publishing	Every slot, for the current and next epoch that are gloas epochs (so warm-up starts one epoch before the fork). Fetches v2 proposer duties, whose dependent_root is the shuffling dependent root the preferences must carry; the duty loop keeps v1. Signs each own duty whose slot has not started and which was not yet published for that (slot, dependent_root). Submits one preference per request to every node, so one refusal does not hold back the others. A reorg that moves the dependent root re-publishes
Unconditional	Not gated on --builder-proposals, as in Lighthouse: from gloas the preferences carry the fee recipient for every proposal. Validators with no fee recipient are skipped, with one warning per epoch
Signing domain	DOMAIN_PROPOSER_PREFERENCES at the fork version of proposal_epoch - 1, which is what the gossip rule verifies against. The validator guide's head-state signer differs only for a proposal in a fork's first epoch signed during that epoch; ours is the one peers following the gossip rule accept. ethlambda's beacon node accepts both
Eth-Consensus-Version	The target epoch's fork (gloas)
Gloas boost factor	With --builder-proposals, produceBlockV4's body carries the configured builder_boost_factor, so pooled p2p bids can win on value. Without it the factor stays 0
Metrics	ethlambda_validator_proposer_preferences_published_total, ..._failures_total (preferences a node refused or did not take), ..._duty_fetch_failures_total (failed v2 duty fetches). A refusal is warned once per preference, then logged at debug

Known limitations

• Lighthouse at the fork boundary (unverified). It is reported to sign at the proposal epoch's fork version. If its gossip validation also verifies at that version, Lighthouse peers would reject our preferences for the first gloas epoch. Ours follow the spec's gossip rule.
• A preference cannot be replaced for the same slot and dependent root, so a fee recipient changed after publishing applies from the next dependent root on.

Tests

• Unit: the v2 path; per-entry submission; refusal isolation; reorg re-publish; the pre-gloas gate; started slots; missing fee recipients; single-flight polling; the fork-version choice on both sides of the boundary; the boost factor in the gloas body.
• End to end through ethlambda's own beacon node: preferences signed by the validator client from its own v2 duties pass the node's gossip rules and are gossiped.

make lint passes, as do the validator lib, rpc lib and binary tests.

[github-actions]

🤖 Claude Code Review

Review of PR 707: validator client publishes signed proposer preferences

I read the diff and the checked-out preferences.rs, signing.rs and the fork-choice helper. I did not build the PR or run its tests. The change looks correct and well tested, with only minor points.

Correctness

• The DependentRoot::V2 refactor in rpc/src/beacon/validator.rs is behavior-preserving. compute_shuffling_dependent_slot is start_slot(epoch.saturating_sub(MIN_SEED_LOOKAHEAD)).saturating_sub(1), which matches the old inline code when MIN_SEED_LOOKAHEAD is 1. Sharing it with the state transition also removes a drift risk.
• sign_proposer_preferences signs at the fork version of proposal_epoch - MIN_SEED_LOOKAHEAD, matching the gossip rule. Both sides of the fork boundary are tested. The PR documents the one case where this differs from the validator guide.
• publish_epoch skips started slots, sends one preference per request, and remembers each success by (slot, dependent_root). Pruning is keyed correctly. Poisoned mutexes are recovered, and InFlightGuard clears the flag on panic.
• The mock now bumps proposer_duties_v2_calls only after guard(), so failed calls are not counted. The mock's doc comments say the same.

Minor points

1. preferences.rs:269-310: each preference is sent in turn, and each send goes to every node. A client with many keys and a slow node could take a while per poll. The in_flight guard keeps polls from stacking, but a poll that overruns skips the next slot's poll. That could cost preferences for the slots after it, which gossip IGNOREs once they have started. A handful per epoch is fine, so this only matters at high key counts. Sending with bounded concurrency (join_all or buffer_unordered) would remove the risk cheaply.
2. preferences.rs:183-198: a failed duties fetch is counted and logged at debug!, and also warns once per epoch. That is intentional, but fetch_warned is pruned with retain(>= epoch), so a node that stays down warns once per target epoch. That is acceptable.
3. published is keyed on (slot, dependent_root) only. A fee recipient or gas limit changed later is not re-sent for the same root, which the PR documents under "No replacement". Fine.
4. http.rs: builder_config formats the JSON by hand. That is safe because the factor is a u64, but serde_json::json! would be more idiomatic. This is a nit.
5. lib.rs: the spawn of preferences.poll(...) is a detached task, so panics are swallowed apart from the guard reset. The guard makes this safe.

Tests
Coverage is thorough: v2 path, per-entry submission, refusal isolation, reorg re-publish, the pre-gloas gate, started slots, missing fee recipients, single-flight, fork-version choice, the boost factor on the wire, and an end-to-end run against ethlambda's own beacon node. The Lighthouse fork-boundary interop is flagged as unverified, which is honest.

No blocking issues.

---

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

[github-actions]

🤖 Codex Code Review

I reviewed the PR diff and the touched Rust paths in context. Overall this looks careful and the added tests materially improve confidence around proposer-preferences signing and v2 duties. I did not find a clear consensus-breaking bug in the new logic.

• No blocking correctness/security issue stands out in the added proposer-preferences flow.
• The main behavior change — signing DOMAIN_PROPOSER_PREFERENCES using the epoch before the proposal and sourcing dependent_root from v2 proposer duties — is well-motivated and covered by tests in crates/validator/src/signing.rs:335, crates/validator/src/preferences.rs:183, and crates/net/rpc/src/beacon/builder_market_tests.rs:1065.

Findings

• Minor robustness concern: PreferencesService keeps published, refusal_warned, no_fee_warned, and duty_fetch_warned behind std::sync::Mutex in async code (crates/validator/src/preferences.rs:99). If any of those locks are ever held across future .await refactors, this becomes a latent footgun. It looks safe today, but I’d consider tokio::sync primitives or extra care/comments around lock scope for maintainability.
• Potential operator-confusion/API concern: the CLI help now says --builder-proposals “boost factor weighs p2p bids in produceBlockV4” even though the option still sounds boolean and gloas publishing is now unconditional (bin/ethlambda/src/validator.rs:80). The implementation seems intentional, but the UX semantics have shifted enough that this may surprise operators. Not a correctness bug, but worth ensuring docs/help text make it crystal clear that on gloas this flag only influences boost factor, not whether proposer preferences are published.
• Test gap, low severity: the end-to-end test explicitly notes it does not pin the fork-boundary version choice (crates/net/rpc/src/beacon/builder_market_tests.rs:1072). Since fork-boundary domain mismatches are exactly the kind of subtle consensus-adjacent bug that can reappear later, I’d keep an eye on whether the unit coverage in crates/validator/src/signing.rs:335 is enough, or add an integration-style boundary case if practical.

What looks good

• Correct use of v2 proposer duties for dependent_root, which matches the gossip validation contract and avoids a likely source of silent rejection (crates/validator/src/beacon_node/http.rs:642, crates/validator/src/preferences.rs:183).
• Good defensive behavior around blinded responses when builder proposals are off; refusing a blinded block in that mode avoids signing something this client cannot publish (crates/validator/src/beacon_node/http.rs:668).
• Sensible deduplication semantics keyed by (proposal_slot, dependent_root), which handles reorgs without suppressing valid resubmission (crates/validator/src/preferences.rs:99).
• Strong test coverage for HTTP encoding/decoding, gossip acceptance, duplicate handling, and signature-domain behavior.

Looks good to me aside from the minor maintainability/UX notes above.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which implements proposer preferences for the Gloas fork. Let me analyze the changes systematically.

Overall Assessment

This is a well-structured PR adding Gloas fork proposer preferences support. The core logic is sound, but I've identified several issues ranging from minor to potentially serious.

---

Critical Issues

1. Potential Division by Zero in builder_boost_factor Documentation vs. Implementation

File: crates/validator/src/beacon_node/http.rs, lines 88-96

The documentation states 0 prefers local payload, but the JSON sends "0" as a string. The spec's produceBlockV4 expects this to be a number, not a string. However, looking more carefully, the format string produces "0" (quoted string), not 0 (number).

fn builder_config(boost_factor: Option<u64>) -> String {
    let factor = boost_factor.unwrap_or(0);
    format!(r#"{{"min_bid":"0","builder_boost_factor":"{factor}","builders":[]}}"#)
}

Issue: builder_boost_factor is serialized as a string ("0", "250"), but the Beacon API spec defines it as a number (uint64). This will cause spec-compliant beacon nodes to reject the request or parse it incorrectly.

Fix: Remove quotes around {factor}:

format!(r#"{{"min_bid":"0","builder_boost_factor":{factor},"builders":[]}}"#)

Similarly, verify whether min_bid should be a string or number per spec.

---

Security Issues

2. Missing Input Validation on builder_boost_factor

File: crates/validator/src/beacon_node/http.rs, line 93

boost_factor is passed directly from CLI/config into JSON without bounds checking. While u64 is inherently bounded, consider whether extremely large values (e.g., u64::MAX) could cause issues in downstream parsing.

Mitigation: The current code is probably safe, but consider capping or documenting maximum sensible values.

---

Correctness Issues

3. Fork Version for Signing Domain at Fork Boundary

File: crates/validator/src/signing.rs, lines 335-342

let proposal_epoch = compute_epoch_at_slot(preferences.proposal_slot);
let domain = self.domain(
    DOMAIN_PROPOSER_PREFERENCES,
    proposal_epoch.saturating_sub(preset::MIN_SEED_LOOKAHEAD),
);

Issue: MIN_SEED_LOOKAHEAD is 4 epochs (mainnet), not 1. This subtracts 4 epochs, not 1.

Wait—let me re-check. The comment says "the epoch before the proposal's" but the code subtracts MIN_SEED_LOOKAHEAD (4).

Looking at the test at line 689:

let preferences = preferences_for(gloas_epoch * preset::SLOTS_PER_EPOCH + 2);

And the test verifies it signs with fulu_fork_version when proposal_epoch == GLOAS_EPOCH. With MIN_SEED_LOOKAHEAD = 4, GLOAS_EPOCH - 4 is still Fulu (assuming Fulu ends at GLOAS_EPOCH - 1).

But what if proposal_epoch == GLOAS_EPOCH + 3? Then proposal_epoch - 4 = GLOAS_EPOCH - 1, still Fulu. Only at GLOAS_EPOCH + 4 does it become Gloas.

This contradicts the comment which says "the epoch before the proposal's." The spec's validate_proposer_preferences_gossip uses the fork at the dependent block's epoch, which for v2 duties is compute_shuffling_dependent_slot(epoch) — that's start_slot(epoch) - 1, i.e., the last slot of the previous epoch, which falls in epoch - 1.

So the correct fork epoch should be proposal_epoch - 1, not proposal_epoch - MIN_SEED_LOOKAHEAD.

File: crates/validator/src/signing.rs, line 339

proposal_epoch.saturating_sub(preset::MIN_SEED_LOOKAHEAD),

Fix: This should be proposal_epoch.saturating_sub(1) to match the actual gossip rule domain.

However, looking at the test a_preference_for_the_first_gloas_epoch_is_signed_under_the_fulu_version — it passes with GLOAS_EPOCH because GLOAS_EPOCH - 4 is still in Fulu territory. But the test a_preference_for_the_second_gloas_epoch_is_signed_under_the_gloas_version uses GLOAS_EPOCH + 1, and GLOAS_EPOCH + 1 - 4 = GLOAS_EPOCH - 3, which is still Fulu!

Wait, let me re-read the test... The test at line 691 only checks that it verifies under gloas_fork_version, but doesn't check that it fails under fulu_fork_version. Let me check...

Actually, looking more carefully at line 720-724:

assert!(verify(
    &pubkey,
    &signature,
    under(context.config.gloas_fork_version)
));

This only asserts it verifies under Gloas, not that it fails under Fulu. So the test doesn't catch the MIN_SEED_LOOKAHEAD issue.

This is a bug: The signing domain uses MIN_SEED_LOOKAHEAD (4) instead of 1, meaning for epochs GLOAS_EPOCH through GLOAS_EPOCH+2, the signature would verify under Fulu in ethlambda but potentially fail against other clients' gossip validation that uses the correct epoch-1 semantics.

Wait—I need to re-read the spec deviation document more carefully. It says:

validate_proposer_preferences_gossip verifies against the dependent block's state advanced to the lookahead start

The "lookahead start" might indeed be MIN_SEED_LOOKAHEAD epochs before. Let me re-check the consensus spec...

Actually, looking at the CLAUDE.md change:

the gossip rule's signing domain

And the spec deviation says:

the gossip rule's state (at the start of the previous epoch)

Hmm, "previous epoch" suggests 1 epoch, not 4. But "lookahead start" in the spec might mean MIN_SEED_LOOKAHEAD.

Given the explicit test and the detailed comment, this might be intentional. However, the comment says "the epoch before the proposal's" which is 1, not 4. This inconsistency between comment and code is confusing and potentially wrong.

Recommendation: Verify against the actual consensus spec for validate_proposer_preferences_gossip. If the spec uses epoch - 1, fix the code. If it uses epoch - MIN_SEED_LOOKAHEAD, fix the comment.

4. compute_shuffling_dependent_slot vs. V1 Definition Consistency

File: crates/net/rpc/src/beacon/validator.rs, lines 540-544

Self::V2 => compute_shuffling_dependent_slot(epoch),

Good: This delegates to the state transition function, preventing drift. But verify that compute_shuffling_dependent_slot is actually exported and correct.

---

Performance Issues

5. Lock Contention in PreferencesService::poll

File: crates/validator/src/preferences.rs, lines 181-186

for target in [epoch, epoch + 1] {
    if self.context.config.fork_at_epoch(target) < ForkName::Gloas {
        continue;
    }
    self.publish_epoch(current_slot, target, &pubkeys, &store).await;
}

The loop processes epochs sequentially. Each publish_epoch awaits v2 duty fetches and submissions. Since the two epochs are independent, they could be parallelized with try_join! or join!.

Mitigation: Low impact (only 2 epochs), but worth considering for high-latency nodes.

6. Per-Preference Sequential Submissions

File: crates/validator/src/preferences.rs, lines 252-280

Preferences are sent one-by-one in a loop with await each iteration. For validators with many proposal slots, this serializes network round-trips.

Mitigation: Consider limited concurrency (e.g., futures::stream::iter(...).buffer_unordered(4)) to parallelize submissions while maintaining per-preference error handling.

---

Rust Best Practices

7. Mutex Poison Recovery Hides Bugs

File: crates/validator/src/preferences.rs, lines 121-125

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

While documented, silently recovering from poisoned mutexes can hide panics in other threads. In a consensus client, this could mask critical failures.

Recommendation: Consider whether parking_lot::Mutex (which doesn't poison) would be cleaner, or at least log when recovering from poison.

8. AtomicBool Ordering Could Be Relaxed

File: crates/validator/src/preferences.rs, line 178

if self.in_flight.swap(true, Ordering::AcqRel) {

AcqRel is stronger than needed for a simple flag. Acquire on load, Release on store would suffice, or even Relaxed since there's no data dependency being synchronized (the actual data goes through Arc clones).

Fix: Use Ordering::Acquire for the load part of swap... actually swap doesn't let you split. Consider compare_exchange_weak(false, true, Ordering::Acquire, Ordering::Relaxed) for better performance on some architectures.

9. std::sync::Mutex in Async Code

File: crates/validator/src/preferences.rs, multiple locations

Using std::sync::Mutex and immediately locking/unlocking in async code is acceptable for very short critical sections, but parking_lot::Mutex or tokio::sync::Mutex would be more idiomatic. The current pattern of lock(&self.published).insert(...) is fine since it doesn't hold across await points.

---

Error Handling & Edge Cases

10. Empty preferences List Early Return

File: crates/validator/src/beacon_node/http.rs, lines 837-839

if preferences.is_empty() {
    return Ok(());
}

Good defensive coding. But verify this matches the spec—does the Beacon API allow empty POSTs? The mock test confirms this behavior, but ensure it's consistent with other similar endpoints.

11. Missing Handling of dependent_root Mismatch

File: crates/validator/src/preferences.rs

The service remembers published preferences by (slot, dependent_root), but what if a node accepts a preference with a different dependent_root than what the service expects? The v2 duties fetch gives one root, but the node's head might have moved on.

Current behavior: The preference would be signed with root A, sent to node with head at root B, and the node's gossip rule would reject it. The service would retry next slot.

Potential issue: If there's a consistent mismatch (e.g., node sees different fork choice), the service would warn once and then debug-log retries, potentially missing proposals silently.

Mitigation: Consider adding metrics or more visible logging for persistent mismatches.

---

Testing Issues

12. Test verifies Function Duplicates Production Logic

File: crates/validator/src/preferences.rs, lines 437-463

The verifies helper reconstructs domain computation manually instead of using SigningContext. This creates a maintenance burden and risk of test-production drift.

Recommendation: Use SigningContext::domain or a test helper that shares code.

13. Missing Test for Fork Boundary Signing

File: crates/validator/src/signing.rs

The tests cover first and second Gloas epochs, but miss:

• Epoch exactly at GLOAS_EPOCH + 3 (where MIN_SEED_LOOKAHEAD subtraction would still yield Fulu if using 4, but Gloas if using 1)
• The transition from Fulu to Gloas during the epoch before Gloas (when signing preferences for GLOAS_EPOCH during GLOAS_EPOCH - 1)

---

Documentation & Comments

14. Inconsistent Terminology: "gloas" vs "Gloas"

Minor: Some comments use lowercase "gloas" for the fork name. Standardize to "Gloas" when referring to the fork.

15. CLAUDE.md Update Is Good but Could Be Clearer

The CLAUDE.md change correctly documents the behavior, but the sentence:

From the epoch before gloas it signs proposer preferences every slot for its proposals in the current and next epoch

Could be misread as "from the epoch before Gloas starts" vs "starting from the epoch before each target epoch." The actual behavior is: starting from GLOAS_EPOCH - 1, sign preferences for slots in GLOAS_EPOCH and GLOAS_EPOCH + 1.

---

Minor Issues

16. builder_config Function Name

File: crates/validator/src/beacon_node/http.rs, line 88

The name builder_config is slightly misleading—it's the self-build configuration, not general builder configuration. produce_block_v4_body or self_build_config might be clearer.

17. Redundant std::slice::from_ref Import Style

File: crates/validator/src/preferences.rs, line 269

std::slice::from_ref(&entry)

Consider importing std::slice::from_ref at the top for consistency, or use &[entry] directly if the trait accepts it.

Actually, looking at the trait:

preferences: &[gloas::SignedProposerPreferences],

std::slice::from_ref(&entry) produces &[SignedProposerPreferences], which is correct. &[&entry] would be wrong. This is fine.

---

Summary Table

Priority	Issue	Location	Fix
Critical	builder_boost_factor serialized as string not number	http.rs:94	Remove quotes: "{factor}" → {factor}
High	Signing domain uses MIN_SEED_LOOKAHEAD (4) vs epoch-1; comment/code mismatch	signing.rs:339	Verify spec, fix code or comment
Medium	Mutex poison recovery hides bugs	preferences.rs:121	Use parking_lot or log recovery
Medium	Sequential epoch processing	preferences.rs:181	Consider join!
Medium	Sequential preference submission	preferences.rs:252	Consider limited parallelism
Low	AtomicBool ordering could be relaxed	preferences.rs:178	compare_exchange_weak
Low	Test logic duplicates production	preferences.rs:437	Share domain computation

The PR is well-tested and the architecture is sound. The string-vs-number JSON serialization is the most critical fix needed before merge.

---

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