Repository navigation
feat(validator): publish proposer preferences (gloas) - #707
MegaRedHand wants to merge 32 commits into
Conversation
🤖 Claude Code ReviewReview of PR 707: validator client publishes signed proposer preferencesI read the diff and the checked-out Correctness
Minor points
Tests No blocking issues. Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code ReviewI 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.
Findings
What looks good
Looks good to me aside from the minor maintainability/UX notes above. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewI'll review this PR which implements proposer preferences for the Gloas fork. Let me analyze the changes systematically. Overall AssessmentThis 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 Issues1. Potential Division by Zero in
|
| 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
ae1a3d2 to
5931f19
Compare
5931f19 to
652bfc0
Compare
…nces Signed proposer preferences must carry the shuffling dependent root, which only v2 of the proposer duties endpoint reports (v1's root is one epoch later). Add both calls to the beacon node trait, with the fallback sending preferences to every node since each one gossips what it accepts.
…main The gossip validator verifies a preference at the fork version of the epoch before its proposal, while the validator guide signs with the head state. The two differ only in the first epoch of a fork, where the gossip rule is what peers accept, so sign for that one.
From gloas a proposer's fee recipient and gas limit travel in signed proposer preferences, and a node ignores trustless bids for a slot whose proposer published none. A per-slot service signs one per owned proposal in the current and next gloas epoch, keyed by (slot, dependent root) so a reorg re-publishes, and sends it to every node.
…oas bids by the boost factor Wire the preferences service into the slot loop, unconditionally since from gloas they carry every proposal's fee recipient. produceBlockV4 now sends the configured builder_boost_factor in its body, so with --builder-proposals a pooled p2p bid can win on value; without it the factor stays 0 as before.
…'s gossip rules The signing domain is the part of the preferences flow most likely to drift between the signer and the verifier, so drive the validator client's own signer and HTTP client against the real route and gossip rules.
Document the per-slot preferences, their two counters, the signing domain departure from the validator guide, and that the boost factor now also weighs gloas p2p bids.
… their reporting A node answers 400 for a whole call when any entry fails, and one it holds for the same slot and root with other content is such a failure, so a batch kept every entry unpublished and re-sent until that slot passed (a restart after a fee recipient change triggers it). Send each preference alone and remember each success. Also: a failed v2 duties fetch no longer warns "duty likely missed" twice a slot (the service warns once per epoch and counts it), the fork header is the target epoch's, the poll is spawned ahead of the proposal, the signing-domain explanation is corrected (get_domain reads state.fork, so the guide and the gossip rule differ only for a proposal in a fork's first epoch signed during it), v2 dependent slot reuses the state transition's definition, and the end-to-end test now reads its duty and dependent root through the client.
A preference a node keeps refusing is retried every slot until its slot passes, which warned up to two epochs of slots per entry. Warn on the first refusal of each (slot, dependent root), log repeats at debug, and keep counting every failure. Reword the failures metric to what it now counts.
…split the fetch counter try_all warned per node on every refusal, so a preference a node kept refusing still warned every slot despite the per-entry dedup; add a quiet variant for it. Poll after the proposal rather than before, so the next epoch duties fetch does not compete with block production. Count failed v2 duty fetches on their own counter, since a syncing node is not a refused preference.
From gloas the proposer preferences carry each validator's fee recipient and target gas limit, so the keymanager's `feerecipient` and `gas_limit` reach the node through them, from the next dependent root on, whether or not `--builder-proposals` is set. The README's duty list, written on main by #716, gains the preferences, which are published every slot.
652bfc0 to
34a914f
Compare
|
Security read of this PR, with the signing side and the network/node side reviewed separately. No critical or high issues: the message is signed under 1. Bound and check the duties before signing ( The v2 duties are taken as they come: no cap on the entry count and no check that 2. A node that refused once never receives the preference again (
Smaller
Tests I'd add: duties at slots outside the target epoch are dropped, and a list of hundreds of entries is bounded; a node that refused while another accepted receives the preference on a later poll; a key removed between the duties fetch and signing is skipped without losing the others; a preference signed during the first gloas epoch itself passing the node's rules end to end (the gossip test has gloas at epoch 0, so the boundary is unit-pinned only). Checked and clean: pre-gloas target epochs skipped before any fetch; started slots skipped, matching the node's IGNORE; one request per preference, so one refusal cannot hold back the others; an empty list never reaches the wire; |
|
/review |
🤖 Gemini Code ReviewThis PR looks great. The implementation of A few highlights:
No issues found — ready to merge. Automated review by Gemini · gemini-4-argon · custom prompt |
🤖 Codex Code Review
Tests could not run: Rustup attempted to write to its read-only temporary directory. Automated review by OpenAI Codex · gpt-6.1-sol · custom prompt |
🤖 Claude Code ReviewReview: proposer preferences in the validator client (PR 707)Overall this is well-structured. Single-flight polling, per-entry submits, per- Correctness / consensus1. A non-zero gloas boost factor lets a p2p bid win, but the proposal path is unchanged.
2. Unconditional publishing combined with factor
3. The signing domain deviates from
4. Verify that the
Behaviour5. Only the first node's duties are signed against.
6. Pre-fork submissions carry a gloas
Minor
Nice work extracting Automated review by Claude · claude-opus-5-5 · custom prompt |
🤖 Kimi Code ReviewFinding
Otherwise, the change looks well considered: the v2 dependent-root calculation now shares the state-transition helper, the fork-boundary signing domain is explicitly tested, single-flight cleanup is panic-safe, and one-preference-per-request avoids batch-wide rejection poisoning unrelated entries. Automated review by Kimi (Moonshot AI) · kimi-k3.1-pro · custom prompt |
Claude-Session: https://claude.ai/code/session_011vghNneoZ1Ta9W6JNqzjxm # Conflicts: # crates/validator/src/beacon_node/fallback.rs
…k version consensus-specs v1.7.0-beta.4 signs (`get_signed_proposer_preferences`) and verifies (`validate_proposer_preferences_gossip`) proposer preferences with `compute_fork_version(proposal_epoch)`, and Lighthouse, Prysm, Teku, Lodestar and Nimbus all do the same. The validator client still signed by beta.2's gossip rule, the fork of the epoch before the proposal's, which differs only in the first epoch of a fork; on a devnet Lighthouse refused every one of its first-gloas-epoch preferences as a bad signature. - Signer: `SigningContext::sign_proposer_preferences` now uses the proposal epoch's fork. Its tests assert gloas's version and not fulu's for the first and the second gloas epoch. - Node: `proposer_preferences_domains` still accepts all three candidates, but tries the proposal epoch's first; beta.2's two are kept until the fixtures (pinned to beta.2) move past it. - New end-to-end test with fulu before gloas: the validator client's preference for the first gloas epoch passes the node's gossip rules and verifies under gloas's version, not fulu's. - The route comment and docs no longer say a validator client names fulu, or that the client signs by the gossip rule's epoch-before domain. Claude-Session: https://claude.ai/code/session_011vghNneoZ1Ta9W6JNqzjxm
…holds them
Security review of the proposer preferences service:
- Bound the v2 duties before signing: drop entries outside the target
epoch and keep the first per slot, so a node's answer costs at most
SLOTS_PER_EPOCH signatures, sends and remembered keys per epoch.
- A preference is done only once every node accepted it. The fallback
now answers Delivery::{Everywhere, Partly}; a partly delivered
preference is kept as signed and re-sent to every node each slot, so a
node that was syncing still gets it, and a fee recipient change in
between cannot split the nodes. Counted and logged once, at the first
acceptance.
- Read the fee recipient and gas limit in one settings snapshot.
- Use the shared InFlight guard, and fix InFlight::enter: then_some
built the guard eagerly, so a refused enter dropped it and cleared the
running run's flag.
Claude-Session: https://claude.ai/code/session_011vghNneoZ1Ta9W6JNqzjxm
|
Thanks for the reviews. The changes are in 68cb7e0, 7edb397 and 3d3220b, on top of #705's (merged in at 382fa78). I checked each point against Lighthouse, Prysm, Teku, Lodestar and Nimbus. That check turned up one thing nobody had flagged, so it comes first. The signing domain: we were the odd one outconsensus-specs v1.7.0-beta.4 signs and verifies proposer preferences with
All five clients do the same: Lighthouse This client signed by beta.2's rule instead:
@pablodeymo's security review
Smaller points:
Tests: added:
Automated reviews
|
|
Second pass, on The first-round follow-ups are all in: I found no logic error in the happy path. What is left is degraded-mode and default behaviour. Four I'd settle before merging. 1. The 60M default now steers our own payload ( Every validator with a fee recipient signs 2. A restarted node loses its preferences for up to two epochs (
3. One slow node delays every send ( Each preference is its own 4. The comment ties Already in the description, still open
Smaller
Simplifications, take or leave
|
One map from (slot, dependent_root) to the signed message and its stage replaces published/partly_published/refusal_warned. It keeps the message after every node accepted it, which the rest needs: - A beacon node holds preferences in memory only, so a restarted one had none for up to two epochs. Every kept preference whose slot is ahead is now sent again, as signed, once an epoch, at the poll of the epoch's last slot, so the next epoch's first slot is covered: a node restarted mid-epoch lacks only the rest of that epoch's. A re-send some node misses is sent each slot from then on; it is never counted as published again. - Sends were one awaited request after another, so one node that does not answer delayed the last of 25 by minutes. Each preference is still its own request, but all of a poll's go out at once, and both target epochs are fetched and sent concurrently: a hung node costs one timeout per poll. With the slot clock, a slot that started while the poll waited is not sent, and one that started in flight is not reported as refused. - Partial delivery is counted: the failures counter takes every send some node refused or did not answer, re-sends included, and the preferences newly held by only some nodes are warned about once per poll. - The pre-sign fee recipient check accepts the address of a preference a node accepted for the slot, which a gloas node pays in preference to the current one, and its hint names the dependent-root delay. Also: the poll gates on gloas and on an empty key set before reading the store or fetching duties, sign's warning puts slot first, and the HTTP client and the mock share one proposer duties lookup for v1 and v2.
From gloas the proposer preferences carry each validator's target gas limit, and produceBlockV4 builds this client's own payload toward it, with or without --builder-proposals. The default was the fixed 60M, so on a network running elsewhere (the devnet ran 200M) every default proposal pulled the limit toward 60M, and only a keymanager override, lost on restart, could change it. --gas-limit (default 60000000, under Lighthouse's flag name) sets the default every validator without an override gets: builder registrations before gloas, proposer preferences from it. Unlike Lighthouse's, it does not require --builder-proposals, since the preferences are published either way. Zero is refused. The keymanager's gas_limit endpoints serve and fall back to it. A default configuration still pulls a non-60M chain toward 60M. Reading the network config's GAS_LIMIT_SCHEDULE, which Lighthouse tries before its own 60M fallback, is left for later.
produceBlockV4's body sent min_bid 0 while this PR turns the boost factor on, and choose_payload applies the floor to the pooled p2p bids: with --prefer-builder-proposals (u64::MAX) any 1-gwei bid beat the local payload, and with the default factor so did any bid while the local payload's value is 0. --builder-min-bid (gwei, default 0, requires --builder-proposals) is sent as that min_bid. Gloas only; produceBlockV3 has no such parameter.
|
Thanks. The changes are in 9fed8e4, af7caeb and 320eb7b. The four
Already in the description
Smaller
Simplifications
TestsAdded:
Not covered: a refusal that arrives after its slot started, and the proposal service reading the preferences, which is three lines of wiring.
|
Brings in #705's second builder-proposals review (one registration send lock, per-node batching, the producer carried with the block, the proposal deadline in propose_by) and #704's refusal of block production while syncing. Conflicts resolved by keeping both sides: - BeaconNodeApi and the fallback gain register_validators_in_batches beside submit_proposer_preferences. - ProposalService keeps builder_min_bid and drops last_blinded_slot, which the producer carried with the block replaces. - The mock keeps the preference delay and proposers_at, and takes the blinded publish gate in place of its delay. - cli.md's fee recipient and gas limit rows keep the gloas text and say the registration usually goes at the next slot.
|
Third pass, on the three response commits ( The four are in and do what the reply says: Three things I'd settle before merging. All are small. 1. The 60M default is wrong on both built-in gloas networks ( Sepolia's and Platåberget's 2. The keymanager still takes a zero gas limit (
3. A builder's bid can be signed with Without the flag the client sends a factor of
Fine as follow-ups
Smaller
Accepted as they are
|
…works POST gas_limit refuses 0, as --gas-limit does: from gloas the value is signed into a proposer preference that cannot be replaced for its dependent root, and the node builds toward it. --gas-limit's help, docs/cli.md and the README name Sepolia and Platåberget, whose GAS_LIMIT_SCHEDULE is 200000000, until #795 reads the schedule. with_default_gas_limit sets the field in place through get_mut().
…osals Without the flag a gloas block request carries a boost factor of 0, the local payload unless it cannot be built, so a node whose execution client fails at the slot may build on a pooled p2p bid. The client signs that block, as Lighthouse, Prysm and Teku do (Lodestar refuses it only in its opt-in executiononly mode). builder_config's 'self-build only, as before' said otherwise; proposal.rs's module doc, the CLI help, docs/cli.md and the README now say so, and a_builders_bid_gets_no_envelope_from_this_client pins it. Revisiting the default itself is #796.
- accepted_fee_recipients takes the duty's validator index, so after a reorg that moves a slot between two of this client's validators, a block paying the other one's address no longer passes the fee recipient check. - The node answers 200, not 400, for an identical preference gossip recorded while its HTTP copy was being validated: validate's AlreadySeen and a refused record_preferences both re-read the held entry. AlreadySeen is validate's first rule, so the earlier pre-check is folded into it. - The 'No beacon node accepted' warning, and the module doc, name the restart with other settings, where the nodes keep and build with the preference signed before it.
|
Thanks. The changes are in e7decb6, ebc5a76 and 86d3b17. The three
Follow-ups
Smaller
|
The once-an-epoch re-send ran at the poll of the epoch's last slot. Polls are single-flight, so a poll held up behind a slow node skips the next one, and when that was the last slot's, the round ran at the next epoch's first slot, which had already started: a node restarted mid-epoch then lacked that slot's preference. The round now starts at the second-to-last slot and still runs at the last when that poll is skipped. A duty whose pubkey does not parse is now logged at debug rather than skipped silently.
Without --builder-proposals a gloas request carries a boost factor of 0, and a node whose execution client fails at the slot may still build on a pooled p2p bid, which the client signs. The floor that bounds that bid could not be set: clap required --builder-proposals for --builder-min-bid. It no longer does, and the value moves from BuilderProposals to ValidatorConfig::builder_min_bid, so it reaches produceBlockV4's min_bid either way.
# Conflicts: # docs/cli.md
From the bots' review of the proposer preferences flow. At a real fork every preference for the first gloas epoch is signed and sent in the epoch before it, so the node runs the gossip rules on a fulu head. The existing boundary test put the head inside the first gloas epoch. `World::before_the_gloas_fork` has a fulu head at slot 32 with gloas at epoch 2, and the new test submits a preference for slot 64 the way the validator client does: the duty and dependent root from the v2 duties route (read from fulu's proposer lookahead), signed, posted, and accepted by `validate`. With `fixed_proposer` answering `None` for a fulu state, this test fails and the gloas-head ones still pass. `submit_as_the_validator_client` now takes the proposal slot. Also: - `proposer_duties_v2`'s trait doc says the fallback fails over quietly there, which only a comment in `fallback.rs` said. - A README line in the validator client section is rewrapped to the paragraph's width.
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
SignedProposerPreferencesthe source of a proposer's fee recipient and gas limit, supersedingprepare_beacon_proposer. A proposer without preferences also gets no trustless bids: p2p bids for its slot are ignored.What it adds
dependent_rootis the shuffling dependent root the preferences must carry; the duty loop keeps v1. Keeps only duties inside the target epoch, one per slot, so a node's answer cannot make it sign more than an epoch's worth. Signs each own duty whose slot has not started and which not every node holds yet for that(slot, dependent_root). Sends one preference per request to every node, all of a poll's at once (both target epochs' fetches and sends too), so one refusal does not hold back the others and a hung node costs a poll one timeout. A preference only some nodes accepted is re-sent, as first signed, each slot until every node holds it. One every node holds is re-sent once an epoch, at the epoch's second-to-last slot (its last, if a slow node made the client skip that poll), for a node that restarted. A reorg that moves the dependent root re-publishes. With the slot clock, a slot that started while the poll waited is not sent--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 epochDOMAIN_PROPOSER_PREFERENCESat the fork version of the proposal epoch, as consensus-specs v1.7.0-beta.4 signs and verifies it, and as Lighthouse, Prysm, Teku, Lodestar and Nimbus do. ethlambda's beacon node also still accepts beta.2's domain (the fork of the epoch before the proposal's), since our fixtures are pinned to beta.2Eth-Consensus-Version--builder-proposals,produceBlockV4's body carries the configuredbuilder_boost_factor, so pooled p2p bids can win on value, and--builder-min-bid(gwei, default0) is sent asmin_bidwith or without it. Without it the factor stays0: the local payload unless it cannot be built, so a node whose execution client fails at the slot may still build on a p2p bid, and the client signs that block, as Lighthouse, Prysm and Teku do (#796 revisits the default)--gas-limit60000000), for registrations before gloas and preferences from it, whichproduceBlockV4builds this client's own payload toward. Not tied to--builder-proposalsethlambda_validator_proposer_preferences_published_total,..._failures_total(sends some node refused or did not answer, re-sends included),..._duty_fetch_failures_total(failed v2 duty fetches). A refusal by every node is warned once per preference, then logged at debug; preferences newly held by only some nodes are warned once per pollKnown limitations
GAS_LIMIT_SCHEDULEis not read, so a default configuration on a chain whose gas limit is not 60M still pulls toward 60M until--gas-limitis set (validator: take the default target gas limit from GAS_LIMIT_SCHEDULE #795). Sepolia and Platåberget schedule 200M, and the docs say to pass--gas-limit 200000000there.Tests
--gas-limitand--builder-min-bidparsing; the fee recipient check's accepted addresses, the proposer's own only; a zero gas limit refused by the keymanager.make lintpasses, as do the validator lib, rpc lib and binary tests. The second-round changes (once-an-epoch re-send, concurrent sends,--gas-limit,--builder-min-bid, the fee recipient check) are not devnet-tested yet.Devnet results
Kurtosis (ethereum-package) with this branch's head (
18a907cf), Lighthouse and Gethglamsterdam-devnet-8, and buildoor as the builder. Fulu from genesis, gloas at epoch 6. Three nodes: Lighthouse alone; ethlambda's validator client on a Lighthouse beacon node (--builder-proposals --prefer-builder-proposals, 40% of stake); ethlambda for both (--builder-proposals). All three stayed on one head through the fork.Works
(slot, dependent_root), 0 failures through ethlambda's beacon node, which validated, pooled and gossiped every preference.0, and the circuit breaker.Fails at the fork boundary
BadSignature, then accepted the same client's epoch-7 and epoch-8 ones. The client retried each refused preference every slot until its slot started (749 failures).Published proposer preferences to gossipsubat info, because the publish error is logged only at debug (swarm_adapter.rs). Possible fixes: re-send the pooled preferences at the fork, or publish the warm-up epoch's preferences closer to it.Together, on a network with Lighthouse, none of our validators had a preference for the first gloas epoch.
Not tested
builder_boost_factorinproduceBlockV4): buildoor's builder was never onboarded (the builder registry was empty on both clients), so there were no bids and every gloas block was self-built. Both beacon nodes accepted the request body.--builder-proposals, the reorg re-publish, and validators with no fee recipient.Found along the way, not from this PR (same on
main)committee_indexfromattestation_dataand sendspayload_attestation_data?slot=, as beacon-APIsmastersays. Lighthouseglamsterdam-devnet-8requirescommittee_indexand serves/payload_attestation_data/{slot}, answering 400 to both. The client on Lighthouse missed every attestation from the fork on: target participation fell from 97% to 58%, and justification stopped.--gas-limit: it registered (and from gloas preferred) 60M unless set per validator through the keymanager. On a 200M chain every builder bid failed the gas-limit check. Added in this PR, since from gloas the preference also steers the node's own payload.getPayloadright afterforkchoiceUpdated), so any bid beats a local build, and boost factors above0cannot be told apart.--builder-min-bidnow sets a floor.glamsterdam-devnet-8's gloas blocks pay the zero address, which our client flags as a fee recipient mismatch.https://claude.ai/code/session_011vghNjfTQmZNRCjPkRs1wP