feat: add bootnode.rs

[varex83]

Closes #130

[varex83]

@claude review this PR

[claude]

Claude finished @varex83's task in 3m 53s —— View job

---

PR Review: feat: add bootnode.rs

Tasks:

• Gather context and understand request
• Read changed files
• Check Go reference implementation (not locally available; reviewed structurally against AGENTS.md and Go patterns)
• Analyze for correctness, safety, and style issues
• Post comprehensive review

---

Summary

This PR adds bootnode.rs to the p2p crate, implementing relay resolution from HTTP(S) URLs and direct multiaddrs, along with MutablePeer to peer.rs for thread-safe peer state management. The overall structure is good and follows the project patterns, but there are several correctness, naming, and style issues to address before merge.

---

Findings

[High] resolve_relay exits permanently on first persistent error

Impact: If query_relay_addrs returns a hard failure during startup (e.g., server unreachable), the background task exits and the relay becomes permanently unresolvable for the lifetime of the node — without a restart.

Evidence: crates/p2p/src/bootnode.rs:188-193

Err(e) => {
    tracing::error!(err = %e, url = %raw_url, "Failed resolving relay addresses from URL");
    return;  // ← exits forever
}

query_relay_addrs already exhausts retries, so this is understandable, but continuous relay polling should loop back even after errors. The Go pattern typically uses a continue or loops again with backoff rather than exiting the goroutine.

Recommendation: Instead of return, log the error and continue to the next tokio::select! (sleep + retry next poll interval). Fix this →

---

[Medium] NotSingleRelayId error variant is dead code

Impact: BootnodeError::NotSingleRelayId is defined (line 87) but never returned anywhere — only a tracing::error! log is emitted at line 204-209. This means callers cannot programmatically handle this condition.

Evidence: crates/p2p/src/bootnode.rs:85-87 (defined), and crates/p2p/src/bootnode.rs:204-209 (only logged)

// Error is defined but this code only logs:
tracing::error!(n = infos.len(), "Failed resolving a single relay ID from addresses");

Recommendation: Either return the error (return Err(BootnodeError::NotSingleRelayId(infos.len()))) or remove the variant from the enum. Fix this →

---

[Medium] reqwest::Client::new() created on every retry cycle

Impact: A new HTTP client (with connection pool) is created every time query_relay_addrs is called (i.e., every 2 minutes per relay). This throws away the connection pool and TLS session cache on every poll, which defeats the purpose of using reqwest.

Evidence: crates/p2p/src/bootnode.rs:252

let client = reqwest::Client::new();

Recommendation: Accept the client as a parameter, or create it once in resolve_relay and pass it down to each call. Fix this →

---

[Medium] std::sync::Mutex in async context — violates AGENTS.md

Impact: MutablePeer uses std::sync::Mutex (peer.rs:8, 139). Per AGENTS.md: "Prefer Tokio sync primitives (tokio::sync::*) over std::sync::* when tasks may .await". While the lock is not held across .await points here, the async tasks that call mutable.set() and mutable.peer() violate this project requirement.

Evidence: crates/p2p/src/peer.rs:5-8, 139

use std::sync::{Arc, Mutex};
// ...
inner: Arc<Mutex<MutablePeerInner>>,

Recommendation: Replace with tokio::sync::Mutex and make set, peer, and subscribe async (or use RwLock if you need concurrent reads). Fix this →

---

[Medium] URL scheme detection is fragile (starts_with("http"))

Impact: Any relay string starting with the characters "http" (e.g., a hypothetical httpXYZ:// scheme) would be treated as an HTTP URL. The correct guards should be starts_with("http://") or starts_with("https://").

Evidence: crates/p2p/src/bootnode.rs:110-111

if relay_addr.starts_with("http") {
    if !relay_addr.starts_with("https") {

Recommendation: Change to starts_with("http://") || starts_with("https://") or parse with url::Url first and check the scheme. Fix this →

---

[Medium] No IPv6 support in multi_addr_from_enr_str

Impact: record.ip() only returns IPv4 addresses. If a relay ENR only has an IPv6 address (ip6 field), the function will return EnrNoIp and fail silently. Go's multiAddrFromENRStr checks both ip4 and ip6.

Evidence: crates/p2p/src/bootnode.rs:336

let ip = record.ip().ok_or(BootnodeError::EnrNoIp)?;

Recommendation: Check record.ip6() as a fallback when record.ip() is None before returning EnrNoIp.

---

[Medium] TimeoutQueryingRelayAddresses name misrepresents its use

Impact: The variant is used to signal cancellation, not a timeout. This is confusing to callers and makes it harder to distinguish between actual timeouts and cancellations.

Evidence: crates/p2p/src/bootnode.rs:59, 265-266

#[error("timeout querying relay addresses")]
TimeoutQueryingRelayAddresses,
// ...
if cancel.is_cancelled() {
    return Err(BootnodeError::TimeoutQueryingRelayAddresses);  // ← actually cancelled
}

Recommendation: Rename to Cancelled or QueryCancelled, or add a separate Cancelled variant.

---

[Low] Redundant clone() in MutablePeer::set

Impact: peer.clone() is called once to store in inner.peer and then called again for each subscriber unnecessarily. Since subscribers take &Peer, the already-stored value can be borrowed.

Evidence: crates/p2p/src/peer.rs:193-194

inner.peer = Some(peer.clone());
inner.subs.iter().for_each(|sub| sub(&peer.clone()));  // ← second clone unnecessary

Recommendation: Use inner.peer.as_deref().unwrap() or just &peer after the first clone(). Fix this →

---

[Low] Non-200 HTTP response uses wrong error variant

Impact: On a non-200 status code, InvalidRelayUrl is returned (line 284). This is semantically wrong — the URL is valid; the server returned an error. This also means non-200 responses will stop retries (since InvalidRelayUrl is not TimeoutQueryingRelayAddresses), but a clearer error variant would improve observability.

Evidence: crates/p2p/src/bootnode.rs:280-285

tracing::warn!(status_code = resp.status().as_u16(), ...);
return Err(BootnodeError::InvalidRelayUrl);  // ← reuses the wrong variant

---

[Low] Duplicate logic between addr_info_from_p2p_addr and addr_infos_from_p2p_addrs

Impact: addr_info_from_p2p_addr (private, in bootnode.rs:370-390) duplicates the peer ID extraction and transport addr stripping logic already present in addr_infos_from_p2p_addrs (public, in peer.rs:265-291).

Recommendation: addr_info_from_p2p_addr can be implemented as addr_infos_from_p2p_addrs(&[addr.clone()])?.into_iter().next().ok_or(...).

---

[Low] P2PConfig::relays type mismatch with new_relays

Impact: P2PConfig::relays is Vec<Multiaddr> (config.rs:60) while new_relays takes &[String]. These should be consistent or there needs to be a documented conversion point.

Evidence: crates/p2p/src/config.rs:60 vs crates/p2p/src/bootnode.rs:106

---

[Low] No tests for bootnode.rs

Impact: There are no unit tests for multi_addr_from_enr_str, addr_info_from_p2p_addr, or the parsing logic. Per AGENTS.md: "For hashing/serialization parity, generate Go-derived test vectors and hardcode them as Rust fixtures."

Recommendation: Add tests for at minimum multi_addr_from_enr_str with a known ENR string, and addr_info_from_p2p_addr with a known multiaddr string.

---

Parity Matrix

Component	Go	Rust	Match	Notes
HTTP relay URL detection	checks scheme explicitly	starts_with("http")	partial	fragile; should match Go's URL parsing
ENR → QUIC-v1 multiaddr	UDP port → /udp/{}/quic-v1	same	yes
ENR → TCP multiaddr	TCP port → /tcp/{}	same	yes
Retry backoff	expbackoff.FastConfig: 100ms/5s/1.6x	same via backon	yes
Poll interval	2 minutes	RELAY_POLL_INTERVAL = 120s	yes
Resolve timeout	1 minute	BOOTNODE_RESOLVE_TIMEOUT = 60s	yes
IPv4 support	yes	yes	yes
IPv6 support	yes (ip6 field)	no	no	missing record.ip6() fallback
Charon-Cluster header	yes	yes	yes
MutablePeer threading	Go interface + goroutines	Arc<Mutex<...>>	partial	should use tokio::sync::Mutex
resolve_relay loop on error	continues polling	exits task	no	Go keeps the goroutine alive

---

Tests

Tests were not run as this is a review-only pass.

---

Open Questions

1. Should new_relays return an error or succeed empty when cancellation fires during the 60-second wait? Currently returns TimeoutResolvingBootnodeEnr on cancellation which is misleading.
2. Is the P2PConfig::relays: Vec<Multiaddr> intentionally different from new_relays's &[String]? If both are used for relay configuration, one of them should be updated for consistency.

[emlautarom1]

LGTM, some small things only.

[iamquang95]

Question: Go only add addresses to peerstore, this Dial will explicitly send event to dial, which then leads to multiple dials. Not sure it's what you want here

[varex83]

I've tried using NewExternalAddrOfPeer instead but what it does is just notifying other behaviour, so I think dialing is the only option (or at least for now)

[iamquang95]

no usage

[emlautarom1]

LGTM, only nits from me.

[emlautarom1]

The ENR is available at http://localhost:8888/enr

Suggested change

	//! This returns the relay's ENR or multiaddrs that clients can use to connect.
	//! This returns the relay's multiaddrs that clients can use to connect.

[emlautarom1]

This is the same code as in pluto_p2p::utils::multi_addrs_via_relay right?

[emlautarom1]

nit: println! instead of tracing

[emlautarom1]

nit: no need to clone

Suggested change

	.header("Charon-Cluster", &lock_hash_hex_owned)
	.header("Charon-Cluster", &*lock_hash_hex)