Fix op-geth bootnode peering#396
Fix op-geth bootnode peering#396canercidam wants to merge 3 commits intoimprove-discovery-with-bootnodefrom
Conversation
|
Claude finished @canercidam's task in 1m 27s —— View job PR Review: Fix op-geth bootnode peering
SummaryThis PR re-enables bootnode peering for op-geth by uncommenting the Issues Found
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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) | ||
| } |
There was a problem hiding this comment.
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).
| 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 " | |
| } |
playground/components.go
Outdated
| 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) |
There was a problem hiding this comment.
Fragile host resolution and hardcoded port.
A few concerns with this approach:
-
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. -
host.docker.internalvia/etc/hostsparsing: This awk command assumeshost.docker.internalis in/etc/hosts(Docker Desktop adds it, but on Linux Docker Engine it may not be present withoutextra_hostsin docker-compose). If the entry is missing,awksilently returns empty, producing a malformed enode URL (enode://<id>@:30303) which could cause op-geth to fail or behave unexpectedly at startup. -
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--netrestrictor by using Docker'sextra_hoststo ensure a stable IP mapping, rather than runtime shell parsing?
|
Claude finished @canercidam's task in 2m 26s —— View job PR Review: Fix op-geth bootnode peering
SummaryThe latest push ( Remaining IssueHardcoded port See inline comment for details and suggestions. Other components solve this by using
|
playground/components.go
Outdated
| 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) |
There was a problem hiding this comment.
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.
|
Claude finished @dvush's task in 1m 52s —— View job PR Review: Fix op-geth bootnode peering
SummaryThe latest push ( AnalysisWhat works well: Using Remaining concernHardcoded port Note: Previous review comments about the host port diverging were overstated for this specific case — both services are in Docker, so the container port (always |
| var trustedPeers string | ||
| if ctx.Bootnode != nil { | ||
| // TODO: Figure out the port dynamically. | ||
| trustedPeers = fmt.Sprintf("--bootnodes enode://%s@$(getent hosts bootnode | awk '{print $1}'):30303 --discovery.v4 ", ctx.Bootnode.ID) |
There was a problem hiding this comment.
Nit: Hardcoded 30303 is correct today but fragile.
Since both op-geth and bootnode are Docker services, the container port is used (not the host port), and the bootnode's default is 30303 (components.go:1085). So this works. However, if someone changes the bootnode's default port, this will silently break.
Consider extracting the bootnode default port as a constant shared between the bootnode component and this line, or using the {{Bootnode}} template and performing hostname-to-IP resolution in the shell (since /bin/sh is already the entrypoint):
// Example: resolve the hostname from the {{Bootnode}} template output at runtime
trustedPeers = fmt.Sprintf("--bootnodes $(echo '%s' | sed 's/@[^:]*/@'$(getent hosts bootnode | awk '{print $1}')'/' ) --discovery.v4 ", ctx.Bootnode.Connect())This way the port comes from the template system automatically.
2 participants
No description provided.