feat(client): validate SSV builder definitions constraints at startup

[shane-moore]

Closes #1279. Based on epbs, which now carries #1282 and #1285; this PR is the last piece of that stack. Deploy only the complete stack.

Problem, Evidence, and Context

Lighthouse accepts up to 64 wire entries, but SSV permits only eight configured builders per validator. Its new per-validator overrides would bypass the original global-only Anchor validation. Excess auth roots can fragment signing quorum even when compilation and startup succeed.

Change Overview

Validate at startup before services run:

• At most eight enabled global entries and eight entries in each explicit validator override.
• An absent per-validator list inherits the bounded global list; an empty list disables direct builders.
• Explicit empty auth and more than 64 builder public keys in a global entry fail, including in disabled entries.
• Disabled placeholder URLs need no hostname-derived auth. Active URL validation remains owned by Lighthouse.
• Startup errors keep the parser's line and column and the filesystem error kind, which are structural and cannot echo input, while still omitting URLs, auth bytes and parser messages.
• Lighthouse's builder_store crate is added to the logging allowlist, so a builder omitted at proposal time (for example a request-auth signing timeout on a cache miss) is logged rather than dropped silently.

Risks, Trade-offs, and Mitigations

Reuse Lighthouse's exported definition types and YAML parser. A small private file wrapper remains because the public store API cannot enumerate every raw override. Validation rereads the file after the store loads it; this existing external-write race is documented, not claimed eliminated.

Startup errors preserve safe categories plus position and error-kind metadata without rendering credentials or auth data. A parser-generated regression test also prevents a second-read YAML error from reflecting a sensitive scalar while asserting the position survives. Signing logs record auth length rather than contents.

Document per-validator configuration, inheritance/disabling and the hostname-default migration. For mixed clients or rolling upgrades, configure explicit identical auth bytes on every operator. Explicitly configuring the standard hostname bytes is valid without negotiating a new custom token; arbitrary custom values still require builder agreement. SIP-94 commit 27fb510 aligns the normative default with this Lighthouse pin. That specification update does not replace mixed-client implementation and runtime verification.

Validation

• cargo test -p client --lib builder_definitions::tests --locked: 11 passed.
• cargo test --release -p client --lib builder_definitions::tests --locked: 11 passed.
• New diagnostic, inactive-URL and parser-redaction tests failed before their respective fixes and passed afterward.
• Tests cover 8/9 entry boundaries, global inheritance, empty overrides, explicit bytes, hostname defaults, disabled policies, builder-key bounds and upstream-store round trips.
• cargo check --workspace --all-targets --locked, make cargo-fmt-check, make lint, make sort, make audit-CI, git diff --check: passed.

No keymanager mutation route is exposed by Anchor, so file startup is the reachable configuration boundary. No runtime reload, auth normalization in the signer, batching, stateless production or envelope-fold implementation is added. See #1285 for the integrated local runtime results and their limitations.

Rollback

Removing this validation reopens the SSV entry-cap and silent-filtering failure modes. Do not deploy a partial revert with direct builders configured; use a previously validated complete build and compatible configuration. This PR adds no database migration; configuration edits apply on restart.

Blockers / Dependencies

#1282 and #1285 are merged. Release the complete stack together. Hosted CI must pass on the refreshed head. The local stub run is not proof of real-builder signature acceptance, bid selection, delivery or mixed-client interoperability.

🤖 Generated with Claude Code

[shane-moore]

@claude, please take a look. just the top commit, as described in pr description

[codecov-commenter]

Codecov Report

❌ Patch coverage is 96.30542% with 15 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (epbs@6c98a38). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
anchor/client/src/builder_definitions.rs	97.02%	12 Missing ⚠️
anchor/client/src/lib.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             epbs    #1283   +/-   ##
=======================================
  Coverage        ?   80.50%           
=======================================
  Files           ?      181           
  Lines           ?    41414           
  Branches        ?        0           
=======================================
  Hits            ?    33342           
  Misses          ?     8072           
  Partials        ?        0           

Flag	Coverage Δ
rust	80.50% <96.30%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude-code-actions-sigp]

Claude finished @shane-moore's task in 24m 7s —— View job

---

Review — PR #1283 (top commit 85596357 only)

• Gather context (diff of top commit vs base)
• Review builder_definitions.rs (module + tests)
• Review lib.rs call site
• Review data_dir.rs change
• Review docs
• Post review feedback

---

Reviewed 85596357 in isolation as requested. This is a tightly-scoped, unusually well-documented change: it layers two SSV-specific constraints (SIP-94 §5 — ≤8 enabled entries, no zero-length resolved auth data) on top of Lighthouse's own BuilderStore validation, and moves the store open earlier so an invalid file fails fast. I found no correctness or security defects worth blocking on.

Correctness — verified

• Fail-fast ordering is real. open_and_validate now runs at lib.rs:155, before processor::spawn (:161) and the sync/doppelganger waits. The old late open at the block-service site was removed; grep confirms exactly one production definition of configured_builders (:155) and one use (:744), no double-open.
• Cap semantics are conservative and safe. The entry cap counts enabled only (validate_ssv_builder_constraints), while the on-wire budget that peers enforce (MAX_REQUEST_AUTH_DISTINCT_ROOTS) is over distinct roots. Since each entry resolves to exactly one auth-data → one root, and disabled entries never reach the wire, enabled ≤ 8 guarantees distinct roots ≤ 8. No under-count path. Entries sharing auth_data only reduce root count, staying under budget.
• Zero-length resolution mirrors the wire. The Some(len) / None → url.to_default_auth_data().len() branch matches what builder_config signs; the three derivation_vector_* tests pin that behavior (omitted → exact URL bytes, trailing-slash distinct, explicit verbatim), so the check can't silently diverge from what actually gets signed.
• Drift guard holds. #[serde(default)] on builders means a renamed key would parse to an empty vec and pass vacuously — but wrapper_round_trip_via_builder_store's second phase inserts 9 entries through the real store and asserts TooManyEnabledEntries, which would fail loudly on a rename/filename drift. That's a genuine CI tripwire, not a vacuous test.

Design / safety notes (non-blocking, all already acknowledged in code)

• Two-read race window (store's earlier read vs. this re-read) is documented in the module doc and mitigated by "no Anchor code writes the file, edits apply on restart." Consciously accepted; fine given no in-process writer.
• Error::Store surfaces Lighthouse's error via {0:?}, and lib.rs:158 prints it with {e} (Display). Lighthouse's error may name builder URLs, which "may embed credentials" per the module doc. This matches the pre-existing {e:?} behavior and Lighthouse's own VC load logging, and the SSV-specific variants correctly carry only index/count (never URLs or auth bytes). Parity decision, not a leak regression.
• Fail-fast on over-cap/zero-length halts the node. Intentional loud signal, consistent with go-ssv rejecting at config load and with every other builder_definitions error since feat(validator_store): sign builder request auth #1282.

Testing

Coverage is thorough for a validation module: cap boundary (9 fail / 8+disabled pass, count asserted against the const), both zero-length arms (explicit "0x" and empty-URL default, each with file-order index), the round-trip drift guard, and three derivation vectors through the public builder_config API. Tests use the real BuilderStore via tempfile, so they pin filename + YAML dialect + field encoding rather than a hand-rolled fixture — good choice.

I could not independently re-verify the Lighthouse BuilderStore/BuilderDefinition API surface (the pinned git checkout isn't reachable from this sandbox), so I'm relying on the PR's stated cargo test -p client / make test results for compilation and behavior. The logic above is verified against the diff and the in-repo call sites.

Verdict: LGTM. No changes requested. (I can't formally approve — for security reasons this action can't submit GitHub approvals.)

[shane-moore]

Reviewed the top commit, 85596357, against its direct parent, d1a56cdd.

The SSV entry cap, zero-length auth validation, startup ordering, parser drift guard, and derivation vectors match the current SIP-94, go-ssv, and pinned Lighthouse behavior. I found two issues to resolve before undrafting:

1. builder_pubkeys is missing its startup bound. Anchor's new pass does not inspect the list length, and Lighthouse's load validator also leaves the Vec unbounded. A definition with 65 valid keys passes startup; builder_config later logs and omits that builder because the wire list is capped at 64. Please enforce this in Lighthouse and re-pin, or check it here, then add a 65-key test and document the limit.
2. The security guidance should not say auth_data is non-secret or imply credentials in URLs are safe. The merged keymanager schema explicitly permits auth_data to be a shared secret, while the pinned store logs raw builder URLs and BuilderUrl::Display does not redact them. Treat custom auth data and URL credentials as sensitive. Until those Lighthouse logs are redacted, advise operators not to embed credentials in builder URLs.

All CI checks pass at this head, including the local testnet. The existing draft gates on the Lighthouse stack and issue #1280 remain.

Reviewed by gpt-5.6-sol max.

[shane-moore]

Both review items addressed in 4ca6d8b: startup now rejects any entry with more than 64 builder_pubkeys (TooManyBuilderPubkeys, all entries, bound tracked structurally via MaxBuilderPubkeys::USIZE, with a 64-pass/65-fail boundary test through the real store), and the docs no longer call auth_data non-secret; they now treat agreed auth data as a credential and advise keeping credentials out of builder URLs, since logs print them verbatim. Moving the pubkeys bound into Lighthouse's load validation stays on the re-pin ask list; the local check becomes deletable then.

[shane-moore]

Review by Claude Fable 5.1 (Claude Code), against head 3f01d4a8 with epbs at 6c98a389 and the Lighthouse pin fdcc8658. Findings were folded into the merge commit 9e0d3f65, so this is the record rather than a request.

Scope checked

• Global entries: at the pin, Lighthouse BuilderConfigFile::validate skips disabled entries and, for enabled ones, checks only URL shape and scheme, derivable default auth, and (url, auth) duplicates. It does not reject an explicit empty auth_data and does not bound builder_pubkeys. The per-entry loop here is unfiltered by enabled, so both are caught for every global entry.
• Per-validator overrides: Lighthouse ValidatorBuilderConfig::validate already bounds the list at 64, rejects oversized pubkeys and empty auth, and dedups. This PR adds only the SSV cap of 8 on present lists; None inherits the already-capped enabled globals and Some([]) disables, matching resolved_for.
• SIP-94 at PR #94 head fc4f965d: 8 entries per validator, lowercase ASCII hostname default, zero-length data invalid. go-ssv #2901 head 6fe7a951: still defaults auth to the raw URL bytes and caps at 8, so the mixed-client migration section is accurate as of today.
• Docs claims against the pin: template file on first start, 2048-byte URL and 4096-byte auth bounds, no enabled on override entries, override replaces globals, disabled globals need no derivable hostname. All hold.
• yaml_serde resolves to a single 0.10.4 on both sides. Startup placement is unconditional, as open_or_create already was on the base, so no new failure class on non-Gloas networks.

No correctness or safety defects found. The two items from the 08-27 review (builder_pubkeys bound, auth_data treated as a credential) are addressed at this head.

Three minor items, fixed in 9e0d3f65

1. Startup errors redacted safe metadata along with the sensitive payload: parse errors lost the parser's line and column, open errors lost the io::ErrorKind. Both are structural and cannot echo input, so they are now rendered while URLs, auth bytes, and parser messages stay out.
2. The docs told operators not to rely on Lighthouse builder_store omission logs being visible. That was true because the crate was missing from the logging allowlist, and one path had no other logger: the 200 ms request-auth sign timeout on a cache miss at proposal time, which dropped the builder silently. builder_store is now on the allowlist (its only log sites are error!) and the caveat is removed.
3. bls was added to [dev-dependencies] while already a regular dependency.

Considered and dropped

• Entry cap versus the 8-root gossip budget under overrides: an override list replaces globals, so a validator resolves to at most 8 entries and at most 8 roots.
• The two-read window between the store load and this validation: unchanged from the earlier review; no Anchor writer, no runtime reload, and only a Lighthouse accessor over loaded definitions would close it. The module doc already names that as the deletion path for the wrapper.