Conversation
|
Claude finished @SozinM's task in 4m 7s —— View job ReviewPosted 5 inline comments. Summary of the most important issues:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| func localMevBoostRelayURL(endpoint string) (string, bool) { | ||
| envSkBytes, err := hexutil.Decode(mevboostrelay.DefaultSecretKey) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
| secretKey, err := bls.SecretKeyFromBytes(envSkBytes[:]) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
| blsPublicKey, err := bls.PublicKeyFromSecretKey(secretKey) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
| publicKey, err := utils.BlsPublicKeyToPublicKey(blsPublicKey) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
|
|
||
| return ConnectRaw(endpoint, "http", "http", publicKey.String()), true | ||
| } |
There was a problem hiding this comment.
All MevBoostRelay instances in this repo run with mevboostrelay.DefaultSecretKey (see mev-boost-relay/cmd/main.go — the CLI exposes no --api-secret-key flag), so localMevBoostRelayURL returns the same BLS pubkey for every endpoint passed to it. In the new l1-multi-builder recipe this means mev-boost is configured with N URLs that all share one pubkey. mev-boost dedupes/aggregates by pubkey internally, so users will not actually see N independent relays — the multi-relay topology silently degenerates to one. Either expose --api-secret-key on the relay CLI and assign per-relay keys here (similar to how rbuilder gets a per-builder RelaySecretKey), or document this limitation prominently.
| // TODO: Handle error | ||
| ctx.Output.WriteFile("rbuilder-config.toml", rbuilderConfigToml) | ||
| config := defaultRbuilderConfigToml | ||
| if r.ServiceName != "" || len(r.RelayEndpoints) > 0 || r.BeaconNode != "" || r.ExecutionNode != "" || | ||
| r.RelaySecretKey != "" || r.ExtraData != "" || r.JSONRPCPort != 0 || r.RedactedPort != 0 || r.FullMetricsPort != 0 { | ||
| config = r.configTOML() | ||
| } | ||
| ctx.Output.WriteFile(configArtifact, config) |
There was a problem hiding this comment.
This "if any field was set, regenerate the TOML programmatically; otherwise use the embedded template" gate is fragile: the template and configTOML() are now divergent (e.g. live_builders = ["mgp-ordering"] in the template vs ["mp-ordering"] here, no [[builders]] block in the template, coinbase_secret_key with vs without 0x prefix, cl_node_url as a string vs an array), so equivalent inputs produce different rbuilder behavior depending on whether any field was set. Adding a new field also requires remembering to extend this || chain. Prefer one source of truth — either always go through configTOML() (with the template removed/inlined as defaults), or template both code paths. Also, the // TODO: Handle error comment at the top has no error to handle.
| for i := 1; i <= l.normalizedBuilderCount(); i++ { | ||
| component.AddComponent(ctx, &Rbuilder{ | ||
| ServiceName: fmt.Sprintf("rbuilder-%d", i), | ||
| RelayEndpoints: relayServices, | ||
| RelaySecretKey: fmt.Sprintf("0x%064x", i), | ||
| ExtraData: fmt.Sprintf("Playground Builder %d", i), | ||
| JSONRPCPort: 8645 + i - 1, | ||
| RedactedPort: 6061 + i - 1, | ||
| FullMetricsPort: 6060 + i - 1, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Each rbuilder runs in its own container and therefore its own network namespace, so these per-instance internal port offsets (8645+i-1, 6061+i-1, 6060+i-1) aren't needed to avoid collisions and just create three more chances to misconfigure. You can leave the rbuilder defaults (Rbuilder.jsonRPCPort/redactedPort/fullMetricsPort) for every instance — they only collide if exposed to the host, and that should be expressed through the {{Port "name" default}} template rather than hardcoded ints.
Also, the RelaySecretKey is derived as fmt.Sprintf("0x%064x", i), producing trivial scalars 0x…0001, 0x…0002, …. Some BLS libraries reject low-entropy / boundary scalars; even where accepted these are recognizably weak keys. For a testing harness this is probably fine, but at minimum hash the index (e.g. keccak256(fmt.Sprintf("rbuilder-%d", i))) so the keys aren't visually trivial.
| func (l *L1MultiBuilderRecipe) normalizedBuilderCount() int { | ||
| if l.builderCount > 0 { | ||
| return l.builderCount | ||
| } | ||
| return defaultL1MultiBuilderCount | ||
| } | ||
|
|
||
| func (l *L1MultiBuilderRecipe) normalizedRelayCount() int { | ||
| if l.relayCount > 0 { | ||
| return l.relayCount | ||
| } | ||
| return defaultL1MultiRelayCount | ||
| } |
There was a problem hiding this comment.
normalizedBuilderCount/normalizedRelayCount silently rewrite 0 (and any negative value) to the default. Passing --builders 0 or --relays 0 will quietly run 2 of each instead of failing or running zero. Either validate the flag and return an error, or change the condition to >= 0 so explicit zero is honored. Same comment for normalizedBlockTime — --block-time 0 is silently replaced.
| func (r *Rbuilder) configTOML() string { | ||
| relayEndpoints := r.relayEndpoints() | ||
| relayNames := make([]string, 0, len(relayEndpoints)) | ||
| for _, relay := range relayEndpoints { | ||
| relayNames = append(relayNames, strconv.Quote(relay)) | ||
| } | ||
|
|
||
| var b strings.Builder | ||
| fmt.Fprintf(&b, "log_json = false\n") | ||
| fmt.Fprintf(&b, "log_level = \"info,rbuilder=debug\"\n") | ||
| fmt.Fprintf(&b, "redacted_telemetry_server_port = %d\n", r.redactedPort()) | ||
| fmt.Fprintf(&b, "redacted_telemetry_server_ip = \"0.0.0.0\"\n") | ||
| fmt.Fprintf(&b, "full_telemetry_server_port = %d\n", r.fullMetricsPort()) | ||
| fmt.Fprintf(&b, "full_telemetry_server_ip = \"0.0.0.0\"\n\n") | ||
| fmt.Fprintf(&b, "chain = \"/data/genesis.json\"\n") | ||
| fmt.Fprintf(&b, "reth_datadir = \"/data_reth\"\n") | ||
| fmt.Fprintf(&b, "el_node_ipc_path = \"/data_reth/reth.ipc\"\n") | ||
| fmt.Fprintf(&b, "coinbase_secret_key = \"0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80\"\n") | ||
| fmt.Fprintf(&b, "relay_secret_key = %s\n", strconv.Quote(r.relaySecretKey())) | ||
| fmt.Fprintf(&b, "cl_node_url = [%s]\n", strconv.Quote(fmt.Sprintf("http://%s:3500", r.beaconNode()))) | ||
| fmt.Fprintf(&b, "jsonrpc_server_port = %d\n", r.jsonRPCPort()) | ||
| fmt.Fprintf(&b, "jsonrpc_server_ip = \"0.0.0.0\"\n") | ||
| fmt.Fprintf(&b, "extra_data = %s\n\n", strconv.Quote(r.extraData())) | ||
| fmt.Fprintf(&b, "ignore_cancellable_orders = true\n") | ||
| fmt.Fprintf(&b, "root_hash_use_sparse_trie = true\n") | ||
| fmt.Fprintf(&b, "root_hash_compare_sparse_trie = false\n") | ||
| fmt.Fprintf(&b, "slot_delta_to_start_bidding_ms = -20000\n\n") | ||
| fmt.Fprintf(&b, "live_builders = [\"mp-ordering\"]\n") | ||
| fmt.Fprintf(&b, "enabled_relays = [%s]\n\n", strings.Join(relayNames, ", ")) | ||
|
|
||
| for i, relay := range relayEndpoints { | ||
| fmt.Fprintf(&b, "[[relays]]\n") | ||
| fmt.Fprintf(&b, "name = %s\n", strconv.Quote(relay)) | ||
| fmt.Fprintf(&b, "url = %s\n", strconv.Quote(fmt.Sprintf("http://%s:5555", relay))) | ||
| fmt.Fprintf(&b, "priority = %d\n", i) | ||
| fmt.Fprintf(&b, "use_ssz_for_submit = false\n") | ||
| fmt.Fprintf(&b, "use_gzip_for_submit = false\n") | ||
| fmt.Fprintf(&b, "mode = \"full\"\n\n") | ||
| } | ||
|
|
||
| fmt.Fprintf(&b, "[[builders]]\n") | ||
| fmt.Fprintf(&b, "name = \"mp-ordering\"\n") | ||
| fmt.Fprintf(&b, "algo = \"ordering-builder\"\n") | ||
| fmt.Fprintf(&b, "discard_txs = true\n") | ||
| fmt.Fprintf(&b, "sorting = \"max-profit\"\n") | ||
| fmt.Fprintf(&b, "failed_order_retries = 1\n") | ||
| fmt.Fprintf(&b, "drop_failed_orders = true\n") | ||
|
|
||
| return b.String() | ||
| } |
There was a problem hiding this comment.
Building TOML by hand with fmt.Fprintf is fragile: anything in r.extraData() containing a " or backslash is only safe because strconv.Quote is used here, but other call sites (e.g. relay names directly interpolated into URLs on line 902) don't get the same treatment, and a future edit can easily inject a string field without quoting. Prefer encoding via github.com/pelletier/go-toml/v2 or BurntSushi/toml and a typed struct — that also makes the divergence with rbuilder-config.toml.tmpl visible. The hardcoded :3500 in cl_node_url (line 888) is also brittle: if the lighthouse beacon's http port template changes, this silently breaks.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude finished @SozinM's task in 4m 20s —— View job PR ReviewPosted 6 inline comments. Most issues raised in the prior review are still present. Most important:
Smaller nits:
|
| func localMevBoostRelayURL(endpoint string) (string, bool) { | ||
| envSkBytes, err := hexutil.Decode(mevboostrelay.DefaultSecretKey) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
| secretKey, err := bls.SecretKeyFromBytes(envSkBytes[:]) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
| blsPublicKey, err := bls.PublicKeyFromSecretKey(secretKey) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
| publicKey, err := utils.BlsPublicKeyToPublicKey(blsPublicKey) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
|
|
||
| return ConnectRaw(endpoint, "http", "http", publicKey.String()), true | ||
| } |
There was a problem hiding this comment.
Multi-relay topology silently degenerates to one relay. MevBoostRelay.Apply (line 647) doesn't pass any per-relay secret key to the relay binary, so every instance runs with mevboostrelay.DefaultSecretKey. localMevBoostRelayURL then encodes the same BLS pubkey into every URL passed to mev-boost, and mev-boost dedupes by pubkey — so --relays N ends up looking like one relay to the boost layer. This defeats the recipe's purpose.
This was raised in the previous review and is still unaddressed. Either:
- Expose
--api-secret-keyon the relay CLI and assign per-relay keys (mirroring how rbuilder gets a per-builderRelaySecretKey), or - Document this limitation prominently and reduce the default
relaysto 1.
| // TODO: Handle error | ||
| ctx.Output.WriteFile("rbuilder-config.toml", rbuilderConfigToml) | ||
| config := defaultRbuilderConfigToml | ||
| if r.ServiceName != "" || len(r.RelayEndpoints) > 0 || r.BeaconNode != "" || r.ExecutionNode != "" || | ||
| r.RelaySecretKey != "" || r.ExtraData != "" || r.JSONRPCPort != 0 || r.RedactedPort != 0 || r.FullMetricsPort != 0 { | ||
| config = r.configTOML() | ||
| } | ||
| ctx.Output.WriteFile(configArtifact, config) |
There was a problem hiding this comment.
Two non-equivalent code paths for the same component. The "any field set → regenerate, otherwise use template" gate switches between TOMLs with materially different content:
| field | template | configTOML() |
|---|---|---|
live_builders |
["mgp-ordering"] (note: mgp, no matching [[builders]]) |
["mp-ordering"] (with [[builders]] name = "mp-ordering") |
cl_node_url |
string "http://beacon:3500" |
array ["http://beacon:3500"] |
relay_secret_key |
unprefixed 5eae... |
0x... prefixed |
enabled_relays |
["playground"] |
named after the relay services |
coinbase_secret_key |
no 0x |
0x prefix |
| telemetry/extra_data/ignore_cancellable_orders | absent | present |
So Rbuilder{} (no fields) and Rbuilder{ServiceName: "rbuilder"} produce two materially different rbuilder configurations. Adding any new field also requires extending this || chain — easy to miss.
Pick one source of truth: either always call configTOML() (delete the template, or inline its values as defaults), or template both code paths through the same generator. Also, the // TODO: Handle error on line 925 refers to nothing — defaultRbuilderConfigToml is an embedded string and WriteFile already returns nothing handle-able here.
| func (r *Rbuilder) configTOML() string { | ||
| relayEndpoints := r.relayEndpoints() | ||
| relayNames := make([]string, 0, len(relayEndpoints)) | ||
| for _, relay := range relayEndpoints { | ||
| relayNames = append(relayNames, strconv.Quote(relay)) | ||
| } | ||
|
|
||
| var b strings.Builder | ||
| fmt.Fprintf(&b, "log_json = false\n") | ||
| fmt.Fprintf(&b, "log_level = \"info,rbuilder=debug\"\n") | ||
| fmt.Fprintf(&b, "redacted_telemetry_server_port = %d\n", r.redactedPort()) | ||
| fmt.Fprintf(&b, "redacted_telemetry_server_ip = \"0.0.0.0\"\n") | ||
| fmt.Fprintf(&b, "full_telemetry_server_port = %d\n", r.fullMetricsPort()) | ||
| fmt.Fprintf(&b, "full_telemetry_server_ip = \"0.0.0.0\"\n\n") | ||
| fmt.Fprintf(&b, "chain = \"/data/genesis.json\"\n") | ||
| fmt.Fprintf(&b, "reth_datadir = \"/data_reth\"\n") | ||
| fmt.Fprintf(&b, "el_node_ipc_path = \"/data_reth/reth.ipc\"\n") | ||
| fmt.Fprintf(&b, "coinbase_secret_key = \"0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80\"\n") | ||
| fmt.Fprintf(&b, "relay_secret_key = %s\n", strconv.Quote(r.relaySecretKey())) | ||
| fmt.Fprintf(&b, "cl_node_url = [%s]\n", strconv.Quote(fmt.Sprintf("http://%s:3500", r.beaconNode()))) | ||
| fmt.Fprintf(&b, "jsonrpc_server_port = %d\n", r.jsonRPCPort()) | ||
| fmt.Fprintf(&b, "jsonrpc_server_ip = \"0.0.0.0\"\n") | ||
| fmt.Fprintf(&b, "extra_data = %s\n\n", strconv.Quote(r.extraData())) | ||
| fmt.Fprintf(&b, "ignore_cancellable_orders = true\n") | ||
| fmt.Fprintf(&b, "root_hash_use_sparse_trie = true\n") | ||
| fmt.Fprintf(&b, "root_hash_compare_sparse_trie = false\n") | ||
| fmt.Fprintf(&b, "slot_delta_to_start_bidding_ms = -20000\n\n") | ||
| fmt.Fprintf(&b, "live_builders = [\"mp-ordering\"]\n") | ||
| fmt.Fprintf(&b, "enabled_relays = [%s]\n\n", strings.Join(relayNames, ", ")) | ||
|
|
||
| for i, relay := range relayEndpoints { | ||
| fmt.Fprintf(&b, "[[relays]]\n") | ||
| fmt.Fprintf(&b, "name = %s\n", strconv.Quote(relay)) | ||
| fmt.Fprintf(&b, "url = %s\n", strconv.Quote(fmt.Sprintf("http://%s:5555", relay))) | ||
| fmt.Fprintf(&b, "priority = %d\n", i) | ||
| fmt.Fprintf(&b, "use_ssz_for_submit = false\n") | ||
| fmt.Fprintf(&b, "use_gzip_for_submit = false\n") | ||
| fmt.Fprintf(&b, "mode = \"full\"\n\n") | ||
| } | ||
|
|
||
| fmt.Fprintf(&b, "[[builders]]\n") | ||
| fmt.Fprintf(&b, "name = \"mp-ordering\"\n") | ||
| fmt.Fprintf(&b, "algo = \"ordering-builder\"\n") | ||
| fmt.Fprintf(&b, "discard_txs = true\n") | ||
| fmt.Fprintf(&b, "sorting = \"max-profit\"\n") | ||
| fmt.Fprintf(&b, "failed_order_retries = 1\n") | ||
| fmt.Fprintf(&b, "drop_failed_orders = true\n") | ||
|
|
||
| return b.String() | ||
| } |
There was a problem hiding this comment.
Hand-rolled TOML with quoting inconsistencies and a hidden coupling.
github.com/BurntSushi/tomlis already a transitive dep (go.mod); use a TOML encoder + a typed config struct rather thanfmt.Fprintfwith mixedstrconv.Quote/ raw literals. As written, a future edit that interpolates an unquoted string field (e.g. extra_data, name) is a quoting bug waiting to happen.- Line 888 hardcodes
:3500for the beacon URL. The lighthouse beacon'shttpport is declared as{{Port "http" 3500}}(line 555), so a default change there silently breaks rbuilder. UseConnect(r.beaconNode(), "http")instead — the same template substitution that other components use. - Line 902 hardcodes
:5555for the relay URL the same way; preferConnect(relay, "http"). - Line 886 (
coinbase_secret_key) hardcodes the first prefunded account secret. If the prefunded account list is ever reordered, this drifts silently.
SozinM
changed the title
- Per-relay BLS keys: expose --api-secret-key on mev-boost-relay, give each MevBoostRelay a distinct hash-derived scalar (reduced mod BLS12-381 order) and embed the matching pubkey in mev-boost's --relay URL. Multi-relay topology no longer collapses to one pubkey. - Drop rbuilder-config.toml.tmpl; Rbuilder always emits config via a typed struct + BurntSushi/toml encoder. Single source of truth. - Replace hardcoded :3500 / :5555 in TOML with port constants shared by lighthouse beacon and mev-boost-relay component declarations. - Drop per-builder JSONRPCPort/RedactedPort/FullMetricsPort offsets; each rbuilder uses defaults in its own netns, host allocator handles host-side collisions. - Replace 0x...0001 BLS scalars with keccak256-derived secrets reduced to the curve order so blst accepts them. - Validate --builders / --relays / --block-time after flag parsing so explicit zero/negative values fail loudly instead of being silently rewritten to defaults. - Restore buildernetSigningTransport in test_tx.go (unrelated to this recipe; the FlowProxy auth header should not be removed here). - Pass GENESIS_TIMESTAMP to mev-boost so getHeader is not skipped as past lateInSlotTimeMs; without this the validator falls back to a reth-built block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @SozinM's task in 5m 23s —— View job PR ReviewPosted 5 inline comments. The latest commit ( Most important
Smaller nits
|
| func (l *L1MultiBuilderRecipe) builderCountOrDefault() int { | ||
| if l.builderCount > 0 { | ||
| return l.builderCount | ||
| } | ||
| return defaultL1MultiBuilderCount | ||
| } | ||
|
|
||
| func (l *L1MultiBuilderRecipe) relayCountOrDefault() int { | ||
| if l.relayCount > 0 { | ||
| return l.relayCount | ||
| } | ||
| return defaultL1MultiRelayCount | ||
| } | ||
|
|
||
| func (l *L1MultiBuilderRecipe) blockTimeOrDefault() time.Duration { | ||
| if l.blockTime > 0 { | ||
| return l.blockTime | ||
| } | ||
| return time.Duration(defaultL1BlockTimeSeconds) * time.Second | ||
| } |
There was a problem hiding this comment.
Validate() runs only inside runIt, but generate and RecipeToYAML skip it.
main.go:786 does if v, ok := recipe.(interface{ Validate() error }); ok { ... } only on the start path. generate l1-multi-builder --stdout calls RecipeToYAML → recipe.Apply(...) directly, which uses builderCountOrDefault/relayCountOrDefault/blockTimeOrDefault. Those silently rewrite 0 and any negative value back to the default — which is exactly the behavior Validate() was added to forbid.
Repro:
playground generate l1-multi-builder --builders -1 --stdout # produces 2 builders
playground start l1-multi-builder --builders -1 # errors, as expected
Either:
- Call
Validate()fromRecipeToYAML(or wire it intogenerateCmd.RunE), or - Drop the
*OrDefaulthelpers entirely and usel.builderCount/l.relayCount/l.blockTimedirectly —Validate()already guarantees they're > 0 on the start path, and theflag.IntVardefaults handle the unset case at flag-parse time.
The current shape leaves the helpers as defensive dead code that re-introduces the very behavior the validator forbids.
| LogJSON bool `toml:"log_json"` | ||
| LogLevel string `toml:"log_level"` | ||
| RedactedTelemetryServerIP string `toml:"redacted_telemetry_server_ip"` | ||
| RedactedTelemetryServerPt int `toml:"redacted_telemetry_server_port"` |
There was a problem hiding this comment.
Field name RedactedTelemetryServerPt looks like a truncated Port; every other port field in this struct is spelled …Port (FullTelemetryServerPort, JSONRPCServerPort). Not externally visible (the toml tag is right) but jarring for the next reader.
| RedactedTelemetryServerPt int `toml:"redacted_telemetry_server_port"` | |
| RedactedTelemetryServerPort int `toml:"redacted_telemetry_server_port"` |
| for _, endpoint := range m.RelayEndpoints { | ||
| if endpoint == "mev-boost-relay" { | ||
| // creating relay url with public key since mev-boost requires it | ||
| envSkBytes, err := hexutil.Decode(mevboostrelay.DefaultSecretKey) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| secretKey, err := bls.SecretKeyFromBytes(envSkBytes[:]) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| blsPublicKey, err := bls.PublicKeyFromSecretKey(secretKey) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| publicKey, err := utils.BlsPublicKeyToPublicKey(blsPublicKey) | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| relayURL := ConnectRaw("mev-boost-relay", "http", "http", publicKey.String()) | ||
| if endpoint.URL != "" { | ||
| args = append(args, "--relay", endpoint.URL) | ||
| continue | ||
| } | ||
| if relayURL, ok := localMevBoostRelayURL(endpoint.Service, endpoint.SecretKey); ok { | ||
| args = append(args, "--relay", relayURL) | ||
| } else { | ||
| args = append(args, "--relay", Connect(endpoint, "http")) | ||
| continue | ||
| } | ||
| args = append(args, "--relay", Connect(endpoint.Service, "http")) | ||
| } |
There was a problem hiding this comment.
Silent fallback hides bad-secret errors.
When localMevBoostRelayURL returns ok=false (the secret fails to decode or BLS rejects it), the code falls through to Connect(endpoint.Service, "http"), which produces a URL with no pubkey@host segment. mev-boost requires the 0xpubkey@ prefix and will reject the relay outright at startup — far from the line that built the bad URL.
Two cleaner options:
- Return the error from
localMevBoostRelayURLand propagate it up (panic inApply, like rbuilder already does onconfigTOMLfailure), so a malformed secret fails at manifest-build time with a clear message. - Drop the fallback line and let an empty-
URL/empty-Serviceendpoint be a hard error.
The current shape only matters if Service != "" and SecretKey is malformed — which is unreachable for the in-tree recipes today, but is a foot-gun for anyone adding a new caller.
| // MevBoostRelayEndpoint identifies a relay reachable by mev-boost. | ||
| // | ||
| // Exactly one form should be set: | ||
| // - URL: a fully-formed `scheme://pubkey@host:port` URL (used for external | ||
| // relays). | ||
| // - Service: the docker-compose service name of a local relay; SecretKey is | ||
| // the BLS secret used to derive the pubkey embedded in the URL (defaults | ||
| // to mevboostrelay.DefaultSecretKey). | ||
| type MevBoostRelayEndpoint struct { | ||
| URL string | ||
| Service string | ||
| SecretKey string | ||
| } |
There was a problem hiding this comment.
The doc says "Exactly one form should be set", but nothing enforces it. If a caller sets both URL and Service, URL wins silently (Apply checks URL != "" first). For a struct documented as a discriminated union, either:
- enforce it: panic / return an error if both are set, or
- collapse the API — e.g. one constructor
MevBoostRelayEndpointFromService(svc, secret)and anotherMevBoostRelayEndpointFromURL(url), instead of a flat struct.
This isn't urgent (current callers use one form), but the struct's contract is currently honor-system.
| latestPlaygroundUtilsTag = "cc6f172493d7ef6b88a5b7895f4b8619806c99f9" | ||
| ) | ||
|
|
||
| // In-container default ports shared between component args and artifact files | ||
| // (e.g. rbuilder TOML). Components must keep their `{{Port "name" default}}` | ||
| // declarations in sync with these constants so URLs in artifact files resolve | ||
| // to the correct in-container port. | ||
| const ( | ||
| lighthouseBeaconHTTPPort = 3500 | ||
| mevBoostRelayHTTPPort = 5555 | ||
| rbuilderJSONRPCPort = 8645 | ||
| rbuilderRedactedPort = 6061 |
There was a problem hiding this comment.
These constants line up the in-container default ports between the component args and the TOML/URL strings that other components hand out, but the link is one-way.
If a user overrides e.g. mev-boost-relay-1:http via the {{Port "name" default}} machinery (or via --override), the relay binds to the new port, but the rbuilder TOML still emits http://mev-boost-relay-1:5555 because configTOML uses the constant. Same for the lighthouse beacon URL.
Consider documenting that "in-container ports are not user-overridable for these components" (and adding a guard in the override layer if you want), or — long-term — exposing in-container ports to other components through the same template substitution that already powers Connect() for cross-service URLs. The constants are an improvement over the prior hardcoded literals; just be aware they sidestep the override layer.
- Run Validate() on every recipe entry point: RecipeToYAML and ValidateRecipe both invoke it now, so `playground generate l1-multi-builder --builders -1 --stdout` and `playground validate` fail loudly instead of silently substituting the default. - Drop *OrDefault helpers from L1MultiBuilderRecipe; GetBaseRecipes calls Flags() once on each recipe so pflag's IntVar/DurationVar defaults populate the struct fields, and Validate() now guards every consumer path. - Propagate localMevBoostRelayURL errors instead of silently falling back to a `Connect(...)` URL with no `0xpubkey@` prefix that mev-boost rejects far from the offending line. - Enforce the URL xor Service contract on MevBoostRelayEndpoint with a validate() method called from MevBoost.Apply; tests in components_test.go cover all combinations. - Rename rbuilderConfig.RedactedTelemetryServerPt to RedactedTelemetryServerPort to match every other …Port field. - Document the in-container port constants' interaction with the manifest's port allocator and `--override`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude finished @SozinM's task in 6m 28s —— View job PR ReviewThe latest two commits ( Most important
Smaller
|
| // Validate checks the recipe's flag-backed fields. Called from every entry | ||
| // point that consumes the recipe (cook/start, generate, validate) so that | ||
| // explicit zero/negative values fail loudly instead of being silently | ||
| // rewritten to defaults. GetBaseRecipes registers Flags() once on each | ||
| // instance so the struct zero never reaches Validate in normal usage. | ||
| func (l *L1MultiBuilderRecipe) Validate() error { | ||
| if l.builderCount < 1 { | ||
| return fmt.Errorf("--builders must be >= 1, got %d", l.builderCount) | ||
| } | ||
| if l.relayCount < 1 { | ||
| return fmt.Errorf("--relays must be >= 1, got %d", l.relayCount) | ||
| } | ||
| if l.blockTime <= 0 { | ||
| return fmt.Errorf("--block-time must be > 0, got %s", l.blockTime) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Validate() is bypassed when this recipe is wrapped in a YAMLRecipe.
runIt (main.go:786) and RecipeToYAML (recipe_to_yaml.go:14–19) both probe the outer recipe via interface{ Validate() error }. *YAMLRecipe doesn't implement Validate(), so a YAML file like
base: l1-multi-builder
base_args: ["--builders", "-1"]silently produces an empty manifest (no rbuilders, no relays) on both playground start and playground generate instead of erroring with --builders must be >= 1.
The structural fix lives in recipe_yaml.go — add a one-line forwarder:
func (y *YAMLRecipe) Validate() error {
if v, ok := y.baseRecipe.(interface{ Validate() error }); ok {
return v.Validate()
}
return nil
}Worth doing in this PR since this is the first recipe to ship a Validate() method, so this gap was invisible before.
| // defaultRbuilderRelaySecretKey is the BLS secret rbuilder uses to sign bids | ||
| // when no override is given. Single-builder L1 setups use this; multi-builder | ||
| // recipes derive distinct keys per builder. | ||
| const defaultRbuilderRelaySecretKey = "0x25295f0d1d592a90b333e26e85149708208e9f8e8bc18f6c77bd62f8ad7a6866" | ||
|
|
||
| // builderCoinbaseSecretKey is the EOA secret used as the rbuilder coinbase. It | ||
| // matches staticPrefundedAccounts[0] so the builder has funds to pay the | ||
| // proposer in playground genesis. | ||
| const builderCoinbaseSecretKey = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80" |
There was a problem hiding this comment.
Implicit, untested coupling between builderCoinbaseSecretKey and staticPrefundedAccounts[0].
The comment notes the coupling, but nothing enforces it: if staticPrefundedAccounts (artifacts.go:209) is ever reordered or the first entry replaced, the rbuilder coinbase silently becomes an unfunded account and bids never get included. There is no compile-time link and no test.
Two cheap options:
- Replace the constant with
staticPrefundedAccounts[0]directly (most direct — couples them at the source) and rename the constant to live next to that slice. - Add a
TestBuilderCoinbaseMatchesPrefundedthat assertsbuilderCoinbaseSecretKey == staticPrefundedAccounts[0], so reordering the prefunded list trips CI.
| "--api-listen-port", fmt.Sprintf(`{{Port "http" %d}}`, mevBoostRelayHTTPPort), | ||
| "--beacon-client-addr", Connect(m.BeaconClient, "http"), | ||
| ) | ||
|
|
||
| if m.SecretKey != "" { | ||
| service.WithArgs("--api-secret-key", m.SecretKey) | ||
| } |
There was a problem hiding this comment.
The relay <-> mev-boost-endpoint pairing relies on callers manually keeping the two SecretKey fields in sync — MevBoostRelay.SecretKey (passed via --api-secret-key here) and MevBoostRelayEndpoint.SecretKey (used to derive the pubkey embedded in the URL handed to mev-boost). The recipe assigns them from the same relaySecrets[i] slice, but nothing in the type system links them, so a future caller that sets one but not the other would silently produce a relay whose bid signatures fail mev-boost's pubkey check.
Since the only valid configuration is "both equal" or "both empty", consider collapsing the two: e.g. take the relay's resolved SecretKey (with the DefaultSecretKey fallback applied here) and have the recipe build endpoints from MevBoostRelay instances rather than from a parallel string slice. That removes the foot-gun and the duplicated relaySecrets[i] plumbing in recipe_l1_multi_builder.go:122–137.
Also: --api-secret-key lands the BLS scalar in docker inspect, the rendered playground.yaml, and any saved manifest. Fine for a deterministic test harness, just worth knowing.
| mevBoost := manifest.MustGetService("mev-boost") | ||
| mevBoostArgs := strings.Join(mevBoost.Args, " ") | ||
| require.Contains(t, mevBoostArgs, `{{Service "mev-boost-relay-1" "http" "http" "0x`) | ||
| require.Contains(t, mevBoostArgs, `{{Service "mev-boost-relay-2" "http" "http" "0x`) |
There was a problem hiding this comment.
These assertions verify each --relay URL has the 0x prefix, but not that the embedded pubkeys actually differ across relays. Since the whole point of plumbing per-relay --api-secret-key was so mev-boost sees distinct pubkeys, an assertion on that property would lock it in:
relay1URL := extractRelayArg(mevBoost.Args, "mev-boost-relay-1")
relay2URL := extractRelayArg(mevBoost.Args, "mev-boost-relay-2")
require.NotEqual(t, relay1URL, relay2URL, "mev-boost must see distinct relay pubkeys")A regression that re-shared the secret (e.g. if someone later collapses relaySecrets[i]) would otherwise still pass this test as long as the URL format is intact.
1 participant
Summary
l1-multi-builderbase recipe that runs one L1 stack with configurable counts of rbuilder and mev-boost-relay services.MevBoostRelayandRbuilderreusable with distinct service names, preservepidin generated YAML, and generate docs for the new recipe.Validation
go test ./playground -run 'TestL1MultiBuilder|TestConvertServiceToYAML'go test ./...go run main.go generate l1-multi-builder --stdoutmake generate-docsmake build