Skip to content

Fix op-geth bootnode peering #396

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dvush merged 3 commits into from
Mar 10, 2026
Merged

Fix op-geth bootnode peering #396

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1bbac9
fix op-geth bootnode peering
canercidam Mar 6, 2026
4f160da
add --nodiscover back conditionally
canercidam Mar 6, 2026
03b4590
op-geth peers
dvush Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions playground/components.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,12 +379,13 @@ func (o *OpGeth) Apply(ctx *ExContext) *Component {
component := NewComponent("op-geth")
o.Enode = ctx.Output.GetEnodeAddr()

// TODO: this does not work for op-geth because hostnames are not allowed
// in geth bootnode config (node will break on startup if we remove --nodiscover below)
// var trustedPeers string
// if ctx.Bootnode != nil {
// trustedPeers = fmt.Sprintf("--bootnodes %s ", ctx.Bootnode.Connect())
// }
var trustedPeers string
if ctx.Bootnode != nil {
// TODO: Figure out the port dynamically.
trustedPeers = fmt.Sprintf("--bootnodes enode://%s@$(awk '/host.docker.internal/ {print $1; exit}' /etc/hosts):30303 ", ctx.Bootnode.ID)
Copy link
Contributor

@claude claude bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fragile host resolution and hardcoded port.

A few concerns with this approach:

  1. Hardcoded port 30303: The TODO acknowledges this but it's worth noting that if the bootnode's port assignment changes, this will silently break peering.

  2. host.docker.internal via /etc/hosts parsing: This awk command assumes host.docker.internal is in /etc/hosts (Docker Desktop adds it, but on Linux Docker Engine it may not be present without extra_hosts in docker-compose). If the entry is missing, awk silently returns empty, producing a malformed enode URL (enode://<id>@:30303) which could cause op-geth to fail or behave unexpectedly at startup.

  3. Why not use the hostname directly? Other components use ctx.Bootnode.Connect() which resolves via Docker DNS. The original TODO says geth doesn't allow hostnames in bootnode config — could this be worked around with --netrestrict or by using Docker's extra_hosts to ensure a stable IP mapping, rather than runtime shell parsing?

Copy link
Contributor

@claude claude bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Hardcoded port 30303 won't match the actual host port.

The bootnode's host port is dynamically allocated via reservePort and may differ from the container-internal default 30303 (e.g., due to portOffset or port conflicts). Since this enode URL targets host.docker.internal (the Docker host IP), it needs the host port, not the container port — the host port is what's mapped back to the bootnode container.

Other components avoid this by using ctx.Bootnode.Connect() which resolves through the template system ({{Service ...}}) and correctly picks the right port. Since geth can't accept hostnames in enode URLs, you're bypassing the template system, but the port still needs to be dynamic.

Consider using the existing {{Bootnode}} template function (local_runner.go:506) combined with shell-based hostname-to-IP resolution, since op-geth already uses /bin/sh as entrypoint. Or add a new template helper that emits just the bootnode IP and host port.

} else {
trustedPeers = "--nodiscover "
}
Comment on lines +382 to +388
Copy link
Contributor

@claude claude bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Missing --nodiscover when no bootnode is configured.

When ctx.Bootnode == nil, trustedPeers is empty and --nodiscover was removed unconditionally. This means op-geth will attempt peer discovery even without a bootnode, which is inconsistent with every other component in this file (op-reth, reth, op-rbuilder, flashblocks-rpc all add --disable-discovery in the else branch).

Suggested change
var trustedPeers string
if ctx.Bootnode != nil {
// TODO: Figure out the port dynamically.
trustedPeers = fmt.Sprintf("--bootnodes enode://%s@$(awk '/host.docker.internal/ {print $1; exit}' /etc/hosts):30303 ", ctx.Bootnode.ID)
}
var trustedPeers string
if ctx.Bootnode != nil {
// TODO: Figure out the port dynamically.
trustedPeers = fmt.Sprintf("--bootnodes enode://%s@$(awk '/host.docker.internal/ {print $1; exit}' /etc/hosts):30303 ", ctx.Bootnode.ID)
} else {
trustedPeers = "--nodiscover "
}


svc := component.NewService("op-geth").
WithImage("us-docker.pkg.dev/oplabs-tools-artifacts/images/op-geth").
Expand All @@ -409,7 +410,6 @@ func (o *OpGeth) Apply(ctx *ExContext) *Component {
"--ws.origins \"*\" "+
"--ws.api debug,eth,txpool,net,engine,miner "+
"--syncmode full "+
"--nodiscover "+
"--maxpeers 5 "+
"--rpc.allow-unprotected-txs "+
"--authrpc.addr 0.0.0.0 "+
Expand All @@ -420,7 +420,7 @@ func (o *OpGeth) Apply(ctx *ExContext) *Component {
"--state.scheme hash "+
"--port "+`{{Port "rpc" 30303}} `+
"--nodekey /data/p2p_key.txt "+
// trustedPeers+
trustedPeers+
"--metrics "+
"--metrics.addr 0.0.0.0 "+
"--metrics.port "+`{{Port "metrics" 6061}}`,
Expand Down
Loading