fix: improve discovery via bootnode

[dvush]

Discovery with bootnodes did not work for l2 nodes, this fixes it but there are some todos left.

• we were using "--disable-discovery" and "--bootnode" at the same time which does not make sense so now when bootnode is set we enable discovery
• bootnode is now set to enable v5 protocol by default just in case, v4 still works (always enabled)
• I added --rollup.discovery.v4 to op-reth based nodes because for some reason v5 discovery does not work for them. This is low priority todo to figure this out.
• op-geth discovery does not work because we use hostname instead of ip and geth does not support it for bootnodes. it never worked but because we were disabling discovery it was never run. This is todo.
• I used "--nat", "none" for the reth nodes otherwise it will advertise external IP of the machine to its peers

[claude]

Claude finished @dvush's task in 1m 1s —— View job

---

PR Review: fix: improve discovery via bootnode

• Gather context and read changed files
• Review changes and provide feedback

---

The changes are logical and well-structured. A few observations:

Repeated bootnode pattern could use a helper

The same if ctx.Bootnode != nil { ... } else { --disable-discovery } block with --nat none appears in 4 places (OpRbuilder, FlashblocksRPC, RethEL, OpReth), and 3 of those also add --rollup.discovery.v4. This is a good candidate for a small helper to reduce duplication and ensure consistency, though it's not blocking.

op-geth discovery is fully disabled (acknowledged TODO)

The commented-out code in OpGeth (lines 382-387) documents the hostname limitation well. Just flagging that --nodiscover on line 412 means op-geth nodes won't participate in peer discovery at all, which may affect multi-node setups. The TODO comment covers this.

No issues found with the BootnodeProtocol removal

The BootnodeProtocol type and BootnodeProtocolV5 constant are cleanly removed with no remaining references. Hardcoding --v5 on the bootnode is a reasonable simplification since v4 is always enabled anyway.

Overall this is a clean fix. The logic change from "always disable discovery + optionally set bootnode" to "enable discovery when bootnode is set, disable otherwise" is correct and the --nat none additions prevent advertising external IPs in Docker networks.

[claude]

Claude finished @dvush's task in 1m 21s —— View job

---

PR Review: fix: improve discovery via bootnode

• Gather context and read changed files
• Analyze changes and provide review

Summary: This PR fixes L2 node discovery by removing the contradictory --disable-discovery + --bootnode combination, adds --nat none to prevent advertising external IPs, enables v5 on the bootnode by default, and falls back to --rollup.discovery.v4 for op-reth based nodes.

Observations:

1. Logic is sound. The core fix — making --disable-discovery conditional on ctx.Bootnode == nil instead of unconditional — is correct. Previously, setting --disable-discovery alongside --bootnodes would silently ignore the bootnode, which is clearly a bug.

2. BootnodeProtocol removal is clean. The BootnodeProtocolV5 enum was only used in one place and the v5 flag is now always set, which is a good simplification since v4 still works regardless.

3. op-geth TODO is well-documented. The commented-out code for op-geth with the explanation about hostnames not being supported is clear. This is a known geth limitation — --nodiscover remains hardcoded which is consistent.

4. Duplication across components. Left an inline comment about this — the bootnode args pattern is repeated in 4 places with slight variations. Not blocking, but worth considering a helper.

5. No test changes. The PR doesn't add or modify tests. Given this is a networking/discovery configuration change, it's understandable that it's hard to unit test, but the existing integration tests should cover basic startup.

No blocking issues found. The changes are focused and well-motivated by the PR description.

[claude]

The bootnode discovery pattern is repeated across 4 components (OpRbuilder, FlashblocksRPC, RethEL, OpReth) with slight variations (some include --rollup.discovery.v4, some don't). Consider extracting a helper like applyBootnodeArgs(svc, ctx, isRollup bool) to centralize this logic — it would make it easier to update in one place if the flags change again.

[claude]

Claude finished @dvush's task in 1m 27s —— View job

---

PR Review: fix: improve discovery via bootnode

• Gather context and read changed files
• Review changes and provide feedback

---

The core logic change — conditionally enabling/disabling discovery based on bootnode presence instead of always disabling — is correct and well-motivated. The BootnodeProtocol simplification and --nat none additions are clean.

Issues noted (inline comment posted):

1. Hardcoded service name and port in op-geth (components.go:385): The getent hosts bootnode shell command hardcodes the service name "bootnode" and port 30303, while every other component uses ctx.Bootnode.Connect() / ctx.Bootnode.Service for the name and template syntax for the port. If either changes, op-geth will silently fail to discover peers.

2. Repeated bootnode args pattern: The if ctx.Bootnode != nil { WithArgs(--bootnodes, --nat none, ...) } else { --disable-discovery } block is duplicated across 4 components with minor variations (some add --rollup.discovery.v4, some don't). A helper like applyBootnodeArgs(svc, ctx, isRollup bool) would reduce the risk of future inconsistencies. (Previously noted, still applicable after sync.)

No blocking issues — the approach is sound and the TODOs are well-documented.

[claude]

The service name bootnode is hardcoded here while other components use ctx.Bootnode.Service (via ctx.Bootnode.Connect()). If the bootnode service name ever changes, this will silently break. Also, port 30303 is hardcoded — consider using the template port syntax to stay consistent with the rest of the codebase.

Additionally, embedding $(getent hosts bootnode | awk '{print $1}') in the shell string works because op-geth uses /bin/sh -c, but it couples this component to a shell entrypoint. A comment noting this dependency would help future readers.