Skip to content

feat: route consensus p2p through circuit-relay-v2 (relay-fronted validators) - #33

Draft
procdump wants to merge 58 commits into
raylsnetwork:mainfrom
procdump:ba-circuit-relay-v2-poc
Draft

feat: route consensus p2p through circuit-relay-v2 (relay-fronted validators)#33
procdump wants to merge 58 commits into
raylsnetwork:mainfrom
procdump:ba-circuit-relay-v2-poc

Conversation

@procdump

Copy link
Copy Markdown
Collaborator

Problem

The consensus p2p layer only supports direct QUIC dialing — validators connect
to each other using the addresses in committee.yaml, which exposes every
validator's IP and offers no way to front a validator with a relay, keep it
NAT'd/unreachable directly, or fail over if its ingress point goes away. We want
circuit-relay-v2 as an option, so a validator can be reached only through a
relay it controls (and isolated / failed over between relays) — while keeping
direct QUIC fully supported. The relay path is opt-in per node via config; nodes
that don't enable it dial directly, exactly as before.

What's changed

Circuit-relay-v2 is added as an opt-in transport path alongside direct QUIC —
enabled per node via keygen/config (--relay / --advertise-dnsaddr + relay env).
With none of it set, behaviour is unchanged (direct dialing).

Client (consensus network)

  • circuit-relay-v2 client transport + behaviour: a validator reserves on its relay
    and is reached single-hop through it; outbound dials open circuits to the
    destination's relay.
  • PeerManager made relay-aware: relays are exempt from banning/pruning, relayed
    inbound connections are accepted, and committee members are re-dialed on the
    heartbeat so a dropped peer reconnects without waiting for the next epoch.
  • Relay reservations are tracked by address and retried (a lost relay is
    re-reserved when it returns) instead of the swarm treating "no listeners" as
    shutdown.

Relay server

  • new rayls-relay circuit-relay-v2 server binary (fixed test identities, raised
    reservation/circuit limits, external-address advertisement so grants carry an
    address, shared QUIC limits with the node, no idle-close of reserving peers).

DNS / failover

  • validators can advertise a /dnsaddr name that resolves (TXT) to all of a
    node's relays, with reservations on each, so peers fail over to a backup relay
    when the primary dies. /dnsaddr is resolved to concrete /p2p-circuit
    addresses at dial time (required for the relay client to classify the connection
    as relayed).

Testnet tooling & verification

  • local-testnet.sh --relay / --relay-dns: auto-spawn per-validator relays (and
    dnsmasq for the DNS variant); add-relay-node.sh to attach an extra relayed
    node to a running net.
  • relay-isolation docker testnet where each validator's only neighbor is its own
    relay (relay is default gateway + NAT egress), a topology verifier that proves
    traffic is relayed-only, and a blue-green failover harness with DNS-driven
    cutover.

Perf

  • announce gossipsub IDONTWANT on publish; apply node QUIC limits on the relay via
    a shared QuicConfig::apply.

Verified

  • --relay and --relay-dns local testnets reach stable consensus with all
    traffic relayed; killing a validator's primary relay keeps consensus running as
    peers fail over to the backup.
  • relay-isolation docker net: chain progresses with each validator reachable only
    through its relay (topology verifier confirms no direct validator↔validator
    paths).
  • direct-QUIC path (no relay config) unchanged.

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 3m 25s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and understand the request
  • Read core networking changes (constructor, runtime, behaviour)
  • Review relay binary and peer manager
  • Review DNS/failover and config changes
  • Review tests and testnet tooling
  • Post comprehensive review

Summary

This is a substantial, well-engineered feature. The transport ordering decision (dns(or(quic, relay))) is subtle and the inline docs explain it clearly. The relay exemption logic, the listener retry loop, and the DNS-off-loop reasoning are all sound. Below are findings organized by severity.


Security

[S1] Relay has no rate limiting — resource exhaustion risk if deployed beyond a private testnet

bin/rayls-relay/src/main.rs lines 79–82 zero out all rate limiters:

cfg.reservation_rate_limiters = Vec::new();
cfg.circuit_src_rate_limiters = Vec::new();

The comment justifies this for a local testnet where validators share 127.0.0.1. In production, a single misbehaving or compromised peer can open unlimited circuits, exhausting relay file descriptors or memory. If rayls-relay is ever deployed outside a fully-private network, the default libp2p rate limiters (or custom ones) should be restored. A compile-time #[cfg(not(feature = "unlimited-relay"))] guard, or at minimum a prominent WARN log at startup that rate limiting is disabled, would make accidental misuse harder.

Fix this →

[S2] RELAY_SEED_HEX derives a deterministic relay identity from a 32-byte env var

keypair_from_seed_env() in main.rs derives a fixed ed25519 key from the seed. The seed values are documented in etc/test-network/RELAY_KEYS.md. If these seeds are ever reused outside the test environment, or if RELAY_KEYS.md is treated as a template for production deployments, the relay identity is predictable. The rayls-relay binary name has no "test" indicator. Consider an eyre::bail! if a known test seed is detected in a non-dev build, or add a CI check that production config files don't use any seed from RELAY_KEYS.md.


Correctness

[C1] Relay exemption registration races first connections in AddBootstrapPeers

command.rs lines 109–118: RegisterRelays is dispatched via a detached task after add_known_peer returns. The peer manager may then immediately emit a DialRequest for a dnsaddr peer. If that dial connects before RegisterRelays is processed, the relay receives its first penalty for not speaking consensus protocols before it is exempted.

The comment says "penalties decay" but score-based bans are not immediate — however, a rapid burst of "unsupported protocol" penalties could push the relay past the ban threshold. The safest fix is to resolve relay circuits synchronously (or before returning from AddBootstrapPeers) and call register_relays_from_addrs before any dial. If DNS blocking is the concern, the alternative is to perform a best-effort synchronous exemption of the concrete relay in add_known_peer before the off-loop task races it (which add_known_peer already does for non-dnsaddr circuit addresses — line 783 of manager.rs).

[C2] relay_reservations HashMap grows without bound

ConsensusNetwork::relay_reservations (mod.rs line 127) is populated in StartListening (command.rs:47) and updated on listener close (runtime.rs:113), but entries are never removed. If a node's relay configuration changes across restarts (different relays), entries from the old configuration persist in the map, are retried every 15 seconds, and only ever fail. For a production node with stable config this is harmless, but it is a latent memory issue in long-running deployments where relay config churns, and it causes unnecessary listen_on calls to dead addresses.

A simple fix: when handle_listener_closed identifies a relay reservation whose retry keeps failing for N consecutive attempts, promote it to a logged no-op or remove it. Alternatively, clear stale entries when new relay addresses are loaded.

[Fix this →](https://claude.ai/code?q=In%20crates%2Fconsensus%2Fnetwork%2Fsrc%2Fconsensus%2Fruntime.rs%20retry_relay_reservations%3A%20add%20a%20retry%20counter%20per%20reservation%20(e.g.%20track%20HashMap%3CMultiaddr%2C%20(Option%3CListenerId%3E%2C%20u32)%3E%20and%20log%20a%20warning%20after%20N%20consecutive%20failed%20re-attempts%2C%20so%20operators%20can%20diagnose%20permanently%20dead%20relays.%20Branch%3A%20ba-circuit-relay-v2-poc&repo=raylsnetwork/axyl)

[C3] dnsaddr TXT records are not validated as circuit addresses before relay registration

resolve_relay_circuits (command.rs:415-447) parses every dnsaddr=<multiaddr> TXT record and adds it to circuits. These are then passed to register_relays_from_addrs, which only registers entries containing a /p2p-circuit, so non-circuit entries are silently ignored. The actual risk is low, but a misconfigured or adversarial DNS record (e.g. dnsaddr=/ip4/1.2.3.4/udp/1234/quic-v1/p2p/<evil>) would cause circuit_relay_peer_id to return None and be silently dropped. This is correct behavior — but it could be clearer with a debug! log on filtered-out entries, making DNS misconfiguration diagnosable.


Performance

[P1] connected_peers.contains() is O(n) on the hot path

peer_events.rs line 172:

if !self.connected_peers.contains(&peer_id) {
    self.connected_peers.push_back(peer_id);
}

For small committee sizes this is negligible, but connected_peers is a VecDeque and contains scans linearly. Consider a secondary HashSet<PeerId> for O(1) existence checks if membership grows.

[P2] redial_missing_committee creates a HashSet allocation on every heartbeat

manager.rs lines 329–340: a HashSet<PeerId> is allocated on every 30-second heartbeat tick even when there are no missing committee members. This is a minor allocation but trivially avoidable by short-circuiting when known_peers is empty or all members are connected.


Design / Architecture

[D1] Acknowledged incomplete failover (TODO-CRv2-NETWORKING.md)

The committed TODO notes that mid-epoch failover is slow (up to ~120s). The PR description says this is verified and working, but the reconnect timing is not prompt. The TODO outlines two complementary approaches. For a PoC branch this is acceptable, but the issue should be tracked before a production flag. Recommend converting this to a GitHub issue and removing the file from the repo, or at minimum adding a # tracking: <issue-link> header so it doesn't drift.

[D2] handle_listener_closed non-relay path is inconsistent with pre-PR behaviour

runtime.rs line 129 comment: "A node mixing a direct listener with relay reservations keeps running but never re-establishes the direct listener (pre-existing behavior restored it via fatal-exit-and-restart); no shipped topology mixes them today — see TODO-CRv2-NETWORKING.md finding 6."

The TODO file doesn't appear to document "finding 6" in its current form. The behaviour change (direct listener loss no longer triggers shutdown on relay nodes) is safe for current topologies, but if a future topology mixes direct and relay listeners this silently degrades the node. The comment could be strengthened with an explicit assertion: debug_assert!(self.relay_reservations.is_empty() || listeners are relayed-only).

[D3] Worker relay configuration mirrors primary with no independent overrides

keytool/generate.rs lines 170–190 uses the same --relay / --advertise-dnsaddr flag for both primary and worker network addresses. There's no way to give the worker a different relay than the primary. The epoch_manager uses separate PRIMARY_RELAY_MULTIADDRS / WORKER_RELAY_MULTIADDRS env vars at runtime, so this is OK operationally, but keygen bakes both to the same relay circuit. A validator that wants to load-balance primary and worker traffic across different relays cannot express that at keygen time without editing node_info.yaml manually.


Minor / Nits

[N1] relay_resolvers is cloned once per AddBootstrapPeers and once per DialBls with DNS

hickory_resolver::TokioResolver is Arc-backed so the clone is cheap, but the clone happens both in AddBootstrapPeers and DialBls each time. Sharing a single Arc<TokioResolver> handle held by ConsensusNetwork would make the intent more explicit (though the behavior is identical).

[N2] max_circuit_bytes = u64::MAX and max_circuit_duration = u32::MAX as u64

The effective-unlimited values disable libp2p's built-in circuit DoS accounting. This is correct for the test relay, but Duration::from_secs(u32::MAX as u64) (≈136 years) vs Duration::MAX (≈584 billion years set on the swarm's idle timeout) is an inconsistency that could confuse readers. Using a named constant like const EFFECTIVELY_UNLIMITED_SECS: u64 = u32::MAX as u64; at the top of relay_config() would clarify intent.

[N3] process_event relay client events are trace-only with no metric

runtime.rs lines 150-154: relay reservation events (ReservationReqAccepted, ReservationTimedOut, CircuitEstablished, etc.) are logged at trace! with no metric. Adding a counter metric (relay_reservations_active, relay_circuits_opened) would make relay health observable in the same dashboards as peer counts.

[N4] TODO-CRv2-NETWORKING.md committed to repo root

Production repositories generally prefer GitHub issues over in-repo TODO files. The file is well-structured and the design notes are valuable, but committing it establishes a precedent. If kept, rename to docs/ or under etc/.


Positive Highlights

  • The transport ordering decision and DNS wrapping explanation (constructor.rs lines 161–174) is exemplary. A subtlety of this caliber is usually a silent footgun; here it's a landmark.
  • PeerManager field ordering justification in behaviour.rs (lines 27–37) preempts a real class of libp2p bugs.
  • Off-loop DNS in AddBootstrapPeers with the RegisterRelays round-trip (command.rs:100–118) correctly avoids blocking the swarm event loop for yamux connections.
  • advertised_relay_covered (epoch_manager/network.rs:394) prevents double-reservation races — a subtle edge case that's been thought through.
  • redial_missing_committee heartbeat re-dial is a clean improvement to mid-epoch reconnect without touching epoch boundaries.
  • Test coverage in network_tests.rs and peer_manager.rs is meaningful — the relay-specific tests verify relay exemption under penalties and re-dial behavior.

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 3m 31s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Read CLAUDE.md for repo-specific instructions
  • Gather context and understand the request
  • Review core networking changes (constructor, runtime, behaviour)
  • Review relay binary and peer manager
  • Review DNS/failover and config changes
  • Review tests and testnet tooling
  • Post comprehensive review

Summary

This is a well-engineered, substantial feature. The transport ordering decision (dns(or(quic, relay))), the off-loop DNS resolution pattern, and the relay exemption logic are all sound and clearly explained. The test coverage for relay-specific behaviors is meaningful. Below are findings organized by severity.


Security

[S1] No startup warning that rate limiting is disabled

bin/rayls-relay/src/main.rs lines 116–117 zero out all rate limiters with a clear justification for the local testnet. However, the binary has no runtime indicator that rate limiting is off. If deployed outside a private network by mistake, unlimited circuit/reservation rates become a DoS surface.

// Consider adding before the event loop:
tracing::warn!(
    "rate limiting disabled — this relay is configured for testnet use only; \
     do not deploy on a public network without re-enabling limiters"
);

Fix this →

[S2] Fixed relay seeds in RELAY_KEYS.md with no production guard

keypair_from_seed_env (main.rs lines 135–143) derives a deterministic ed25519 key from RELAY_SEED_HEX. The seeds are documented in etc/test-network/RELAY_KEYS.md. A production deployment that copies these seeds would have a fully predictable relay identity. The binary name rayls-relay carries no "test" signal. Consider an eyre::bail! if a seed matching any known test value is detected at startup, or a CI lint that prevents relay seeds from RELAY_KEYS.md appearing in production config files.


Correctness

[C1] relay_reservations HashMap grows without bound

mod.rs line 127: entries are added in StartListening and reset to None on listener close (runtime.rs line 117), but never removed. If relay configuration changes between restarts, stale entries remain and trigger listen_on calls to dead addresses every 15 seconds indefinitely. This is harmless for stable config but produces noise in operator logs and represents a latent memory issue in long-running deployments with relay churn.

A retry counter per entry (HashMap<Multiaddr, (Option<ListenerId>, u32)>) logging a warning after N consecutive failed re-attempts would let operators diagnose permanently dead relays.

[Fix this →](https://claude.ai/code?q=In%20crates%2Fconsensus%2Fnetwork%2Fsrc%2Fconsensus%2Fruntime.rs%20retry_relay_reservations%3A%20add%20a%20consecutive-failure%20counter%20per%20reservation%20(e.g.%20change%20relay_reservations%20value%20to%20(Option%3CListenerId%3E%2C%20u32)%20and%20log%20a%20warning%20after%20N%20consecutive%20failed%20re-attempts%20so%20operators%20can%20diagnose%20permanently%20dead%20relays.%20Branch%3A%20ba-circuit-relay-v2-poc&repo=raylsnetwork/axyl)

[C2] Relay exemption races first connections in AddBootstrapPeers

command.rs lines 109–118: RegisterRelays is sent via a detached task after add_known_peer returns, so the swarm may emit a DialRequest for a /dnsaddr peer before the relay is exempted. If the connection arrives before RegisterRelays is processed, the relay peer receives its first penalty for not speaking consensus protocols before it is exempted. The comment notes "penalties decay", which is correct for most cases, but a rapid burst of "unsupported protocol" events from reconnect churn could in theory push the relay toward the ban threshold before the exemption lands.

The synchronous exemption already inserted by add_known_peer (line 783 of manager.rs) protects non-dnsaddr circuit addresses. The gap is only for the relay discovered off-loop from DNS resolution. Given that relays are also registered in dial_peer (manager.rs line 177), in practice the relay is exempted before any dial completes.

[C3] resolve_relay_circuits silently returns non-circuit TXT entries

command.rs lines 434–436: all TXT records that parse as Multiaddr are pushed to circuits. register_relays_from_addrs filters them correctly (only extracts relay id from /p2p-circuit addresses), but non-circuit entries silently occupy the vec. A debug! log on filtered-out entries would make DNS misconfiguration diagnosable without changing behavior.

[C4] dial_peer_bls gives up on relay-fronted committee members when connected to other peers

network.rs lines 280–284:

if retries > 10 && peers > 0 {
    error!(..., "failed to reach peer {bls_pubkey}, giving up");
    return;
}

This is unchanged pre-existing behavior, but in relay mode a node may give up dialing a specific committee member if it is connected to other peers while that member's relay hasn't come up yet. The redial_missing_committee heartbeat now compensates (one attempt per member per 30s heartbeat), making this a non-issue for the PoC. Worth documenting in the TODO or tracking as follow-up, since the two mechanisms (dial_peer_bls retry and heartbeat redial) have subtle interaction: the heartbeat may succeed on the next tick where the epoch-start task gave up.


Performance

[P1] DNS resolution on every heartbeat DialBls for /dnsaddr members

command.rs lines 162–163: resolve_relay_circuits runs a live DNS txt_lookup on every DialBls call. redial_missing_committee fires once per 30s heartbeat for each disconnected committee member with a /dnsaddr address. For a 4-validator committee this is ≤3 DNS queries per 30s — negligible at PoC scale. This is also intentional: fresh resolution is how relay failover works. A short TTL in-process cache (keyed on the hostname, evicted on TTL) would reduce redundant queries for healthy peers without compromising failover semantics, but is clearly out of scope for this PoC.

[P2] redial_missing_committee allocates a HashSet on every heartbeat tick

manager.rs lines 329–340: connected_or_dialing_peers().into_iter().collect() allocates a HashSet<PeerId> on every 30s heartbeat even when known_peers is empty or all members are connected. Trivially avoidable:

fn redial_missing_committee(&mut self) {
    if !self.is_peer_validator(&self.local_peer_id) || self.known_peers.is_empty() {
        return;
    }
    // ... existing logic
}

[P3] connected_peers.contains() is O(n) on hot path

peer_events.rs line 173: VecDeque::contains is linear. For committee sizes ≤100 this is negligible. A secondary HashSet<PeerId> for O(1) existence checks would be a clean improvement if the connected peer count grows.


Design / Architecture

[D1] TODO-CRv2-NETWORKING.md committed to repo root

The design notes are valuable (especially the option B proposal for immediate re-dial on disconnect), but a file named TODO-*.md at the repo root is an unusual convention and sets a precedent. Recommend either:

  • Moving to docs/ or etc/ and adding a # tracking: <issue-link> header, or
  • Converting to a GitHub issue and removing the file before merge.

[D2] Worker relay config mirrors primary at keygen with no independent overrides

generate.rs lines 168–190: both primary and worker use the same --relay/--advertise-dnsaddr flag at keygen time. Runtime can override via PRIMARY_RELAY_MULTIADDRS/WORKER_RELAY_MULTIADDRS, so this is operationally fine. An operator wanting different relays per network must edit node_info.yaml manually rather than at keygen. Low priority for PoC but worth tracking.

[D3] handle_listener_closed non-relay path behavior change is underdocumented

runtime.rs line 128–129 comment: "A node mixing a direct listener with relay reservations keeps running but never re-establishes the direct listener (pre-existing behavior restored it via fatal-exit-and-restart)". The referenced "TODO-CRv2-NETWORKING.md finding 6" does not appear in the current file. The comment is correct, but strengthening it (or adding a debug_assert!(self.relay_reservations.is_empty() || /* topology is relayed-only */)) would catch future topology changes that accidentally mix direct and relay listeners.


Nits

[N1] max_circuit_duration vs idle_connection_timeout units are inconsistent

main.rs line 104 sets Duration::from_secs(u32::MAX as u64) (≈136 years) while line 194 sets Duration::from_secs(u64::MAX) (≈584 billion years). Both are "effectively unlimited" but the inconsistency could confuse a reader. A named constant at the top of relay_config():

const EFFECTIVELY_UNLIMITED_SECS: u64 = u32::MAX as u64;

would clarify intent and make the circuit duration limit's u32::MAX upper bound explicit.

[N2] Relay client events are trace-only with no metric

runtime.rs lines 150–154: ReservationReqAccepted, ReservationTimedOut, CircuitEstablished etc. are logged at trace! with no counter metric. Adding relay_reservations_active / relay_circuits_opened gauges/counters would make relay health visible in the same dashboards as peer counts.

[N3] endpoint_str in relay server: QuicV1 branch updates proto but relies on a prior UDP match for port

main.rs lines 53–61: the match arms set port from Protocol::Udp(x) and proto from Protocol::QuicV1. For a well-formed QUIC address (/ip4/.../udp/PPPP/quic-v1), this works because Udp is matched before QuicV1. The logic is correct but fragile — a reordered multiaddr or future protocol addition could silently produce proto = "quic-v1" with port = None. Combining the port + protocol extraction into a single match block would be more explicit.


Positive Highlights

  • Transport ordering decision dns(or(quic, relay)) in constructor.rs (lines 163–174) is explained with exemplary clarity — a genuinely subtle footgun documented at the exact right place.
  • PeerManager field ordering justification in behaviour.rs preempts a real class of libp2p NetworkBehaviour poll-ordering bugs.
  • Off-loop DNS in AddBootstrapPeers / DialBls with the RegisterRelays round-trip correctly avoids blocking the swarm event loop.
  • register_relays_from_addrs called at three ingestion points (external address at construction, StartListening, add_known_peer, and dial_peer) provides belt-and-suspenders relay protection with no duplication (all paths converge on relay_peers.insert).
  • advertised_relay_covered (network.rs lines 394–399) correctly handles address-form aliasing (same relay peer id, different transport address) and is backed by four focused unit tests.
  • connection_paths HashMap (mod.rs line 134) providing per-connection transport classification at ConnectionEstablished is an elegant observability mechanism — one classification per connection, covering all protocol traffic multiplexed over it.
  • retry_relay_reservations correctly distinguishes "relay temporarily down (retry)" from "no listeners (maybe fatal)" — the split logic in handle_listener_closed is sound and well-commented.
  • redial_missing_committee heartbeat re-dial is a clean, minimal improvement to mid-epoch reconnect that avoids touching epoch boundaries.
  • Relay peer exclusion from kademlia (peer_events.rs lines 164–169) prevents a chain reaction where relays are discovered, dialed as consensus peers, penalized for not speaking consensus protocols, and — on a shared IP — IP-ban the entire local testnet.

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 2m 17s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and understand the request
  • Read core networking changes (constructor, runtime, behaviour)
  • Review relay binary
  • Review peer manager and command handling
  • Review DNS/failover and config changes
  • Review tests and testnet tooling
  • Post comprehensive review

Summary

This is a solid, production-minded PoC. The transport ordering rationale (dns(or(quic, relay))), the off-loop DNS pattern, and the PeerManager field-ordering justification are all genuinely well-reasoned, and the test coverage is meaningful. The findings below are independent of the two prior automated reviews.


New Findings

Security

[S1] reservation_allow_list closure captures a HashSet<PeerId> by move — interior set cannot be revoked at runtime

bin/rayls-relay/src/main.rs:134–136: the allow-list is baked at startup from a single env-var read. There is no way to add or remove allowed peers without a relay restart. For a test relay with a fixed validator set this is fine, but a production relay fronting a rotating committee would need to re-read the allow-list whenever the committee changes. This is a PoC limitation worth documenting (or noting in the env-var docs) before copying the pattern.

[S2] keypair_from_seed_env does not zeroize the seed bytes after use

main.rs:197–201: the decoded Vec<u8> and the [u8; 32] array holding the ed25519 seed sit in heap/stack memory until the function returns and the memory is reclaimed. An attacker with read access to the relay process memory (e.g., a /proc/mem dump after OOM, a crash dump) could recover the secret key. This is low-risk for a test relay but worth fixing before production: zeroize::Zeroizing<[u8; 32]> wrapping the array ensures the bytes are zeroed on drop.

Fix this →


Correctness

[C1] handle_listener_closed does not update relay_reservations when a reservation succeeds — the None slot is never promoted back to Some on NewListenAddr

runtime.rs:83–101 (retry_relay_reservations): when listen_on succeeds, the reservation is immediately set to Some(id):

self.relay_reservations.insert(addr, Some(id));

However the re-reservation is tentative — libp2p emits NewListenAddr only after the circuit-relay RESERVE handshake completes. If the relay drops again between the listen_on call and the completed handshake, the entry becomes Some(stale_id) and retry_relay_reservations skips it (the filter is active.is_none()). On the next ListenerClosed for that stale id, handle_listener_closed resets it to None correctly — so the net behavior is correct. But there is no NewListenAddr handler that confirms the reservation ID is still valid, and a fast relay-flap could skip one retry cycle. This is a latent timing edge case rather than a bug, but worth a comment at the retry_relay_reservations site.

[C2] DialBls in PeerEvent::RedialCommittee drops the reply channel — dial errors are silently swallowed

peer_events.rs:238–243:

PeerEvent::RedialCommittee(bls_key) => {
    let (reply, _outcome) = oneshot::channel();
    self.process_command(crate::types::NetworkCommand::DialBls { bls_key, reply })?;
}

_outcome is immediately dropped, so the oneshot receiver is gone before the dial completes. When the dial command errors (already-connected, DNS resolution failure, etc.) the error is silently discarded. This is intentional fire-and-forget (the comment says "the outcome is fire-and-forget"), but AlreadyConnected and AlreadyDialing are not errors in this context — only genuine dial failures (e.g., NoPeers or a DNS error) deserve a trace log. Consider:

self.task_spawner.spawn_task("redial-committee", async move {
    if let Err(e) = outcome.await {
        trace!(target: "peer-manager", ?bls_key, ?e, "redial-committee outcome");
    }
});

Fix this →

[C3] resolve_relay_circuits in DialBls retains circuits by last-protocol match only — a multiaddr with a trailing /p2p/<wrong-id> could slip through

command.rs:167–169:

resolved.retain(
    |c| matches!(c.iter().last(), Some(Protocol::P2p(id)) if id == peer_id),
);

A circuit multiaddr of the form .../p2p/<relay>/p2p-circuit/p2p/<node>/... (trailing suffix) would have its last protocol be something other than P2p(node_id) and would be incorrectly filtered out. Conversely .../p2p-circuit/p2p/<node-A>/p2p/<node-B> (malformed) would pass if node-B == peer_id. This is benign in practice because DNS TXT records don't produce malformed addresses, but using c.iter().rev().find_map(...) over just last() would be more robust.

[C4] ConnectionEstablished warn! fires for outbound dials to non-relay peers on a node that has pending-but-not-yet-active relay reservations

runtime.rs:169–176: the warn fires when !self.relay_reservations.is_empty(). At startup a node with relay config calls StartListening for its relay addresses before any reservation is established; relay_reservations is non-empty immediately (keys are inserted even when listen_on is pending, mod.rs:127). If the swarm simultaneously dials committee peers (direct QUIC) during this window — before the relay reservation handshake completes — each direct connection emits a spurious warning even on a well-configured node. A tighter condition would check whether any reservation is active (i.e., has Some(id)):

let any_reservation_active = self.relay_reservations.values().any(Option::is_some);
if matches!(path, ConnectionPath::DirectNonRelay { .. }) && any_reservation_active {
    warn!(...);
}

Fix this →


Performance

[P1] connected_peers.retain is O(n) on every PeerDisconnected and DisconnectPeerX

peer_events.rs:62, 132:

self.connected_peers.retain(|peer| *peer != peer_id);

VecDeque::retain scans the whole deque. The same concern exists for connected_peers.contains on PeerConnected (line 173). For the committee sizes targeted by this PR (≤ ~100 validators) this is immaterial, but the VecDeque is the wrong data structure for membership queries. A HashSet<PeerId> (or a dual structure: VecDeque for round-robin ordering, HashSet for O(1) membership) would eliminate both hot-path scans without changing the existing round-robin semantics used in SendRequestAny.


Design / Architecture

[D1] retry_relay_reservations logs at info! on every retry attempt — creates log spam during relay outages

runtime.rs:93:

info!(target: "network", ?addr, "re-attempting relay reservation");

This fires every 15 seconds for every missing relay reservation. During a relay outage (the expected "relay is down, keep retrying" scenario), this generates an info-level log message every 15s indefinitely. The first re-attempt should be info!, subsequent ones debug!. A simple counter per address (even just a bool "has already been info-logged") would suppress the flood.

Fix this →

[D2] ep_of closure in the relay binary borrows peer_eps — cannot be called while swarm is mutably borrowed

bin/rayls-relay/src/main.rs:267–269:

let ep_of = |peer: &PeerId, m: &HashMap<PeerId, String>| {
    m.get(peer).cloned().unwrap_or_else(|| "?".to_string())
};

This is a free function closure passed the map explicitly. It works correctly as written. But notably it is called as ep_of(peer, &peer_eps) while swarm.select_next_some() holds the swarm borrow — Rust permits this because peer_eps is a separate binding. However if the lookup were refactored to close over peer_eps instead, it would fail to compile when called after the swarm borrow. The explicit-map-argument pattern is future-proof; worth a brief comment for the next reader.

[D3] circuit_relay_peer_id correctness depends on address well-formedness — no validation

types.rs:45–55: the function walks protocols, remembering the last /p2p/<id> before the first /p2p-circuit. This is correct for well-formed libp2p circuit addresses but silently returns the wrong peer id for a pathological address like /p2p/<relay>/p2p/<other>/p2p-circuit/p2p/<dst>. Since all addresses come from libp2p serialization or from DNS TXT records the node controls, this is not an exploitable path — but a debug_assert or a unit test for the multi-P2p case would pin the assumption.


Nits

[N1] endpoint_str QuicV1 branch silently produces "quic-v1" with port = None for a bare /quic-v1 without preceding /udp

main.rs:67: Protocol::QuicV1 | Protocol::Quic => proto = "quic-v1" updates proto but does not set port. If Udp is somehow absent (malformed addr or future protocol change) the Some(format!(...)) at line 73 returns None (because port? fails) and the endpoint is logged as "?". This is safe (no panic) but silent. Consider asserting or logging that Udp was seen whenever QuicV1 is matched.

[N2] resolve_relay_circuits iterates TXT records with a nested loop — txt_data() is a slice of slices

command.rs:430–438: the inner for data in record.txt_data() iterates each string in a TXT record's RDATA byte vectors. A single DNS TXT RDATA string is at most 255 bytes; _dnsaddr entries are typically a single string per record. This is correct and efficient; just noting that record.txt_data().concat() would also work and collapse the nesting, but the current two-level loop is clearer.

[N3] dns_resolver_config EDNS0 comment says "truncated response forces TCP fallback that stalls" — worth clarifying it's the hickory resolver that stalls, not TCP itself

constructor.rs:293–295: the comment is accurate in context but could confuse a reader unfamiliar with DNS: it's the resolver's internal retry logic (not TCP as a protocol) that stalls when forced to switch. A one-word addition ("the resolver then stalls on TCP retry") would make it precise.


Positive Highlights (new observations)

  • reservation_allow_list using libp2p's RateLimiter trait as a pure boolean gate is clever: it threads through the relay behaviour's existing hook without adding a new callback, and the _now argument being ignored is self-documenting.
  • parse_allowed_reservers fails fast on an empty but set env var, preventing the silent "accept no reservations" footgun; the build_relay_swarm test mirrors the binary's relay_config() logic exactly, which means changes to one will break tests for the other.
  • Transport override guard on AddBootstrapPeers / DialBls correctly avoids blocking the swarm event loop for yamux keepalives — the comment justification is thorough enough that a future maintainer won't accidentally revert it.
  • advertised_relay_covered resolves by relay peer id (not address string), which correctly handles the DMZ-relay two-address form described in its comment; the four focused unit tests cover all the edge cases.

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 6m 49s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and understand the request
  • Read CLAUDE.md for repo-specific instructions
  • Review core networking changes (constructor, runtime, behaviour)
  • Review relay binary and peer manager
  • Review DNS/failover, config, and epoch-manager changes
  • Review tests and testnet tooling
  • Post comprehensive review

Summary

Three thorough automated reviews have already covered the major correctness, security, and performance concerns on this PR. This review identifies new findings that complement those earlier passes without repeating ground already covered.


New Findings

Security / Correctness

[S1] relay_listen_addresses accepts relay multiaddrs without a /p2p/<relay-id> suffix — silently produces a malformed circuit

network.rs lines 334–344:

let listen = relay
    .with(Protocol::P2pCircuit)
    .with_p2p(network_pubkey.clone().into())
    .map_err(|_| eyre::eyre!("relay multiaddr from {env_var} ({entry}) has a conflicting P2P id"))?;

If PRIMARY_RELAY_MULTIADDRS contains a relay base address without the relay's peer id (e.g. /ip4/1.2.3.4/udp/50002/quic-v1 rather than /ip4/1.2.3.4/udp/50002/quic-v1/p2p/<relay-id>), relay.with(Protocol::P2pCircuit).with_p2p(self-id) succeeds and produces /ip4/1.2.3.4/udp/50002/quic-v1/p2p-circuit/p2p/<self-id>. This is a malformed circuit: no relay peer id precedes the /p2p-circuit. Consequently:

  1. circuit_relay_peer_id() returns Noneregister_relays_from_addrs never protects this relay from banning.
  2. When the swarm calls listen_on on the malformed address, libp2p fails to identify the relay and the reservation never establishes, but the failure manifests as a confusing runtime error ("failed to re-attempt relay reservation") rather than a clear startup misconfiguration.

The docstring example …/p2p/<R2>,… shows the expected format, but there is no upfront guard. Adding an early eyre::bail! if circuit_relay_peer_id(&listen).is_none() after construction would surface the misconfiguration at startup with a clear message.

Fix this →


[C1] resolve_relay_circuits DNS failure is logged to the "network-kad" target

command.rs line 445:

warn!(target: "network-kad", %name, ?e, "failed to resolve /dnsaddr for relay discovery");

This is DNS/relay-discovery logic, not kademlia. Operators filtering logs by subsystem (RUST_LOG=network_kad=warn) will see this warning attributed to kademlia, which could obscure a real kademlia issue and make relay-DNS misconfiguration hard to find. The target should be "network" (consistent with other relay-path warnings in runtime.rs) or a dedicated "network::relay".

Fix this →


[C2] outbound_failure_penalty brittle string match — existing unit test won't catch libp2p version drift

reqres.rs lines 182–185 and the test at 196–203:

// brittle string match: …an SDK bump changing this literal must re-check the arm
OutboundFailure::Io(e) if e.to_string().contains("max sub-streams reached") => None,

The code comment correctly warns this is fragile. The companion unit test max_substreams_reached_is_not_penalized creates the error manually with the same hardcoded string:

let error = OutboundFailure::Io(io::Error::other("max sub-streams reached"));

So if libp2p bumps the string (e.g. "max substreams reached" without the hyphen), both the code and the test continue to compile and pass — the protection silently disappears and peers start receiving Penalty::Medium for a local resource exhaustion event. A grep-based CI assertion against the vendored libp2p source checking that the literal "max sub-streams reached" appears, or a static_assertions::const_assert that panics at compile time if it drifts, would close this gap. Low-priority but worth tracking before this code reaches a high-traffic deployment.


Nits

[N1] relay_retry interval fires immediately at startup — no-op for direct-QUIC nodes

runtime.rs line 45:

let mut relay_retry = tokio::time::interval(Duration::from_secs(15));

tokio::time::interval fires its first tick at t=0, so retry_relay_reservations() runs immediately when the loop starts — before any relay has had a chance to drop. For direct-QUIC nodes (relay_reservations is always empty), this is a no-op that fires every startup and on every process restart. Using tokio::time::interval_at(tokio::time::Instant::now() + Duration::from_secs(15), Duration::from_secs(15)) avoids the spurious first tick and makes the 15-second retry intent self-documenting.

[N2] resolve_relay_circuits iterates without bounding txt_lookup concurrency

command.rs lines 422–449: relay circuits are resolved sequentially (for addr in dnsaddrs { ... txt_lookup(...).await }). For a 4-validator committee this is fine (≤3 addresses). If committee size grows to, say, 32, the sequential resolution adds latency proportional to committee size at every epoch start and heartbeat redial. futures::future::join_all or FuturesUnordered over the lookups would parallelize them within the detached task, keeping total resolution time bounded by the slowest single lookup rather than their sum. Low priority for the current PoC scale.


Confirmations / Agreement with Prior Reviews

The three prior automated reviews correctly identified the most impactful issues. In order of priority:

  • Rate limiting disabled on relay (prior S1 across all three reviews) — the startup warn! suggestion is the minimal fix for the testnet; a feature flag is the production path.
  • relay_reservations HashMap grows without bound (prior C1/C2 across reviews) — the retry-counter-per-entry fix is clean and bounded; worth doing before production.
  • ConnectionEstablished warn fires during reservation-pending window (prior C4 in review 3) — the any(Option::is_some) fix is a one-liner that removes the false positives.
  • retry_relay_reservations log spam at info! (prior D1 in review 3) — first-attempt info / subsequent debug is the right fix.
  • TODO-CRv2-NETWORKING.md in repo root (prior N4/D1 across reviews) — strongly recommend converting the implementation notes to a GitHub issue and removing the file before merge.

Positive Highlights

  • The transport ordering rationale dns(or(quic, relay)) in constructor.rs:163–174 is one of the clearest pieces of inline architectural documentation in the codebase.
  • PeerManager field ordering justification in behaviour.rs:27–37 preempts a real class of libp2p NetworkBehaviour poll-ordering bugs and is excellent defensive documentation.
  • advertised_relay_covered correctly identifies relay by peer id rather than address string, and its four unit tests cover the edge cases (same relay different address, backup relay, direct advertised, empty set).
  • The reservation_allow_list pattern using RateLimiter as a pure boolean gate is compact and doesn't require a new libp2p callback; parse_allowed_reservers fails-fast on an empty-but-set env var, preventing the silent "accept no reservations" footgun.
  • relay_listen_addresses and start_swarm_listeners correctly dedup advertised-vs-explicit relay coverage, preventing the double-reservation race at startup.
  • outbound_failure_penalty correctly exempts local substream exhaustion from peer banning — the test pinning this at reqres.rs:196–219 is a good investment even if the string match is fragile.

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 4m 27s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Read CLAUDE.md for repo-specific instructions
  • Review core networking changes (constructor, runtime, behaviour)
  • Review relay binary and peer manager
  • Review command handling and DNS/failover
  • Review epoch manager and config changes
  • Review tests
  • Post comprehensive review

Summary

This is the fifth review pass. Three prior automated reviews have covered the major findings thoroughly. This pass reads each changed file directly and adds new observations while confirming the most actionable items from prior reviews.


New Findings

Correctness

[C1] handle_listener_closed scans relay_reservations linearly by ListenerId

runtime.rs lines 113–118:

if let Some((addr, active)) =
    self.relay_reservations.iter_mut().find(|(_, active)| **active == Some(listener_id))

This is O(n) in the number of relay reservations. More critically, the reverse mapping (ListenerId → Multiaddr) is not maintained: if retry_relay_reservations issues a new listen_on and the relay immediately closes again before processing the next heartbeat, the Some(new_id) entry is found and reset correctly, but only because ListenerClosed fires synchronously. The correctness holds, but adding a HashMap<ListenerId, Multiaddr> reverse map would make the lookup O(1) and make the data flow unambiguous.

Fix this →

[C2] relay_reservations.insert on a failed listen_on sets the entry to None — perpetual retry for invalid addresses

command.rs line 47:

self.relay_reservations.insert(multiaddr, res.as_ref().ok().copied());

If listen_on returns an immediate Err (e.g. the multiaddr is syntactically invalid, not just the relay being temporarily down), the entry is None and retry_relay_reservations will call listen_on again every 15 s indefinitely, logging a warn! each time. An Err from listen_on is already forwarded to the caller via send_or_log_error!, which propagates the error and likely prevents node startup — so in practice the node never reaches the retry loop. But if listen_on errors are non-fatal in future (or in a test harness), this becomes a perpetual warn-spam loop. A guard that only inserts into relay_reservations when listen_on succeeds (moving the None-init to the ListenerClosed handler) would make the retry semantics explicit.

[C3] resolve_relay_circuits is named "circuits" but returns all dnsaddr= TXT records — direct QUIC addresses pass the DialBls retain filter

command.rs line 417 (function name) and lines 430–438 (no circuit filter on push). All dnsaddr=<multiaddr> TXT entries are collected regardless of whether they contain /p2p-circuit. In DialBls (line 167–169):

resolved.retain(
    |c| matches!(c.iter().last(), Some(Protocol::P2p(id)) if id == peer_id),
);

A direct QUIC address /ip4/1.2.3.4/udp/PORT/quic-v1/p2p/<peer-id> has P2p(peer_id) as its last component and passes this filter. It would then be appended to all and dialed alongside circuit addresses. On a relay-only node this would trigger a direct connection attempt and the "direct connection to a non-relay peer on a relayed node" warn! in process_event. In practice, operator-controlled DNS TXT records for relay-only topologies would never include direct addresses, so this is unlikely to fire. However, filtering to /p2p-circuit addresses in resolve_relay_circuits before returning would close the gap and make the function's name accurate.

Fix this →


Minor / Nits

[N1] connected_peers VecDeque includes relay peers in its length, misleading log context

peer_events.rs lines 143–175: relay peers are added to connected_peers on PeerConnected (line 173). Log lines using connected_peers = self.connected_peers.len() (e.g., gossipsub publish OK/FAILED, peer CONNECTED/DISCONNECTED) report a count that includes direct-leg relay connections, not just consensus-capable peers. An operator seeing connected_peers = 5 when the committee size is 4 (4 validators + 1 relay) may be confused. Excluding relay peers from connected_peers or logging consensus_peers separately would improve clarity. (This also subsumes the O(n) contains concern from prior reviews: a HashSet<PeerId> for membership plus keeping relays out of it would fix both.)

[N2] relay_retry first tick fires immediately — retry_relay_reservations runs before any reservation is established

runtime.rs line 45:

let mut relay_retry = tokio::time::interval(Duration::from_secs(15));

tokio::time::interval's first tick fires at t=0, so retry_relay_reservations is called before the swarm has had a chance to receive any events. For direct-QUIC nodes (relay_reservations is always empty) this is a no-op that runs on every startup. For relay nodes, it harmlessly scans an empty map (reservations are populated by StartListening commands, which arrive after the loop starts). Using interval_at(Instant::now() + Duration::from_secs(15), Duration::from_secs(15)) would make the 15-second intent self-documenting. (Previously noted in review 4 N1; confirming it is real.)


Confirmation of Top Prior Findings

The following findings from prior reviews are confirmed correct — none have been addressed yet. In priority order for pre-merge work:

# Finding File Status
S1 Rate limiters zeroed with no startup warn! bin/rayls-relay/src/main.rs:159–160 Open
S2 Fixed seeds in RELAY_KEYS.md, no production guard on keypair_from_seed_env main.rs:193–201 Open
C1 relay_reservations HashMap grows without bound, stale entries retried forever runtime.rs:83–100 Open
C2 ConnectionEstablished direct-connection warn! fires during pending-reservation window runtime.rs:169–170 Open — one-liner fix: any(Option::is_some)
C3 retry_relay_reservations logs info! on every 15s retry — log spam during outages runtime.rs:93 Open
C4 DNS failure in resolve_relay_circuits logged to "network-kad" target command.rs:445 Open — one-liner fix
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Open

The C2 warn! fix and C4 log target fix are both genuinely one-liners and should be folded in before merge.


Positive Highlights

  • Transport ordering dns(or(quic, relay)) rationale in constructor.rs:163–174 is landmark documentation — a subtle footgun explained exactly where it would be needed.
  • relay_reservations intent (desired vs. active) is well-modelled: the None/Some invariant correctly distinguishes "relay temporarily down" from "reservation established", and handle_listener_closed preserves the split.
  • reservation_allow_list using the RateLimiter trait as a pure boolean gate is compact and doesn't require a new libp2p callback hook.
  • parse_allowed_reservers fails fast on an empty-but-set env var — prevents the silent "accept no reservations" footgun.
  • advertised_relay_covered resolves by relay peer id (not address string), and its four unit tests cover the edge cases including DMZ dual-address relays.
  • The outbound_failure_penalty exemption for local substream exhaustion is correct: penalizing the target peer for a self-inflicted resource limit would accelerate ban cascades under load.
  • PeerManager::process_penalty relay exemption at manager.rs:479 is belt-and-suspenders: even if a relay somehow bypasses registration, the check prevents a consensus-layer penalty cascade from tearing down the relay leg.

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 4m 13s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Read CLAUDE.md for repo-specific instructions
  • Review new test-network script commits (MULTI_LISTEN, stakeable add-relay-node, stake-relay-node)
  • Re-read core Rust changes for remaining issues
  • Confirm status of prior open findings
  • Post comprehensive review

Summary

Five prior automated reviews covered the Rust changes exhaustively. This sixth pass focuses on the five new commits pushed since the last review — all test-network shell scripts — and adds observations on two subtle Rust points the earlier passes did not cover. None of the previously-flagged open items have been addressed in these commits.


New Findings — Test-Network Scripts

Security

[S1] stake-relay-node.sh hardcodes the well-known anvil #0 private key as the admin default

stake-relay-node.sh line 52:

ADMIN_PRIVATE_KEY="${ADMIN_PRIVATE_KEY:-0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80}"

This is the publicly-known anvil account #0 key. The comment documents it correctly as a local testnet default, but no runtime guard prevents it from being used against a non-dev network. Prior reviews flagged RELAY_KEYS.md for the same pattern and recommended a CI lint against known test secrets appearing in production config. The same lint rule should extend to this file.

[S2] add-relay-node.sh derives operator keys from a trivially-brute-forceable integer

Line 52:

OPERATOR_KEY="${OPERATOR_KEY:-0x$(printf '%064x' $((1000 + NODE_NUM)))}"

For NODE_NUM=5, this produces private key 0x00…00003ed — a four-bit secret. The comment says "test-only, throwaway", which is correct, but the deterministic scheme (index → key) is also used by stake-relay-node.sh to derive the matching address for on-chain staking. An operator who copies this pattern to a non-local network with a non-zero balance would have funds trivially stolen. A comment linking to RELAY_KEYS.md's documentation, or a guard that checks $RPC_URL is not a public endpoint, would help.


Correctness

[C1] stake-relay-node.sh: step 3 error tolerance is too broad

Line 111:

cast send "$REGISTRY_CONTRACT_ADDRESS" "allowlistValidator(address)" "$ADDRESS" \
    --private-key "$ADMIN_PRIVATE_KEY" --rpc-url "$RPC_URL" || echo "  (allowlist may already be set; continuing)"

cast send exits non-zero for ANY failure: network timeout, wrong ADMIN_PRIVATE_KEY, RPC endpoint down, or any revert other than "already allowlisted". A DNS/TCP failure here is silently swallowed; the script continues to step 4 (operator approve) and step 5 (stake), which fails with OwnableUnauthorized — a confusing error whose root cause (failed allowlist) was discarded. The broad || echo was intended to handle idempotent re-runs; a pre-check (cast call ... "isAllowlisted(address)(bool)") before the cast send would handle idempotency without masking real failures.

Fix this →

[C2] stake-relay-node.sh: partial-step failures leave inconsistent on-chain state with no recovery path

If step 4 (approve) succeeds but step 5 (stake) reverts, the operator has approved the registry to spend its RLS but is not staked. On re-run, step 2 mints again (doubling the operator's RLS balance) before step 4 approves and step 5 stakes. The double-mint is harmless on a testnet with a MINTER_ROLE admin, but the script has no pre-check for "is this node already staked?" before entering the 6-step flow. Documenting that re-running after partial failure requires first manually revoking the approval (or running with STAKE_AMOUNT=0) would save debugging time.

[C3] add-relay-node.sh: peer-id regex 12D3KooW[A-Za-z0-9]* matches on partial log writes

Line 117:

RELAY_PEER=$(grep -ao '12D3KooW[A-Za-z0-9]*' "$RELAY_LOG" 2>/dev/null | head -1 || true)

* matches zero characters, so if the relay log is read mid-write (the line is flushed but the peer ID isn't complete yet), grep can match 12D3KooW alone and RELAY_PEER is set to an 8-character truncated value. The circuit address becomes /ip4/127.0.0.1/.../p2p/12D3KooW (invalid multiaddr), listen_on fails, and the node prints a confusing "failed to re-attempt relay reservation" rather than a startup error. Using a minimum-length anchor:

RELAY_PEER=$(grep -ao '12D3KooW[A-Za-z0-9]\{40,\}' "$RELAY_LOG" 2>/dev/null | head -1 || true)

would reject prefix-only matches (libp2p peer IDs are ≥46 base58 chars after 12D3KooW).

Fix this →

[C4] add-relay-node.sh: NODE_NUM > 255 silently produces an invalid (wrong-length) relay seed

Lines 83-85:

byte=$(printf '%02x' "$NODE_NUM")
for ((c = 0; c < 32; c++)); do SEED="${SEED}${byte}"; done

printf '%02x' 256 outputs 100 (three hex characters), making SEED 96 hex chars (48 bytes) instead of 64 (32 bytes). keypair_from_seed_env exits with "RELAY_SEED_HEX must decode to exactly 32 bytes" — a runtime error that only manifests after the relay starts. Adding a guard at line 88 (where NODE_NUM > NUM_VALIDATORS is checked) that also enforces NODE_NUM <= 255 would surface this constraint before spawning the relay.


Nit

[N1] MULTI_LISTEN direct listeners are not reflected in committee.yaml — silent topology change on restart

In commit e6a4cc9, MULTI_LISTEN=1 opens direct QUIC listeners on ports 40000+i (primary) and 41000+i (worker) via PRIMARY_LISTEN_ADDR/WORKER_LISTEN_ADDR env vars. These addresses are not baked into node_info.yaml or committee.yaml, so:

  1. A restart without MULTI_LISTEN=1 silently drops the direct listeners — the node falls back to relay-only with no warning in the logs.
  2. Peers joining via committee.yaml (relay circuit addresses) never learn the direct listener without first connecting and running identify.

This is intentional for the test topology, but a startup log line noting "MULTI_LISTEN: opening direct listener on port X in addition to relay reservation" would make the topology observable without reading the script.


Confirmation: Prior Open Findings

The five new commits touch only test-network shell scripts; the Rust source is unchanged. All findings from prior reviews remain open:

# Finding File Status
S1 Rate limiters zeroed with no startup warn! bin/rayls-relay/src/main.rs:159–160 Open
S2 Fixed seeds in RELAY_KEYS.md, no production guard on keypair_from_seed_env main.rs:193–201 Open
C1 relay_reservations HashMap grows without bound, stale entries retried forever runtime.rs:83–100 Open
C2 ConnectionEstablished direct-connection warn! fires during pending-reservation window runtime.rs:169–170 Open — one-liner: any(Option::is_some)
C3 retry_relay_reservations logs info! on every 15s retry — log spam during outages runtime.rs:93 Open
C4 DNS failure in resolve_relay_circuits logged to "network-kad" target command.rs:445 Open — one-liner
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Open

C2 and C4 remain the easiest one-liner fixes and should be folded in before merge. C1 (HashMap bound) and C3 (log spam) are the highest-priority for production readiness.


Positive Highlights (new commits)

  • stake-relay-node.sh readiness gate (polling ERC-1967 impl slot + registry owner) is a clean solution to the race between local-testnet.sh --start and on-chain contract initialization — prevents the cryptic mid-flow revert that plagued earlier iteration.
  • add-relay-node.sh restart-safe guard (alive() + RESTART=0/1) is well-structured: keygen and genesis copy are idempotent-skipped on re-run, while relay and node processes are always (re)started — the correct mental model.
  • Split-horizon dnsmasq (MULTI_LISTEN: private-direct view on 5353, public-relay view on 5354) correctly models the outside-vs-inside topology on one host without touching the validators' node_info.

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 2m 54s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and understand the request
  • Read CLAUDE.md for repo-specific instructions (no CLAUDE.md present)
  • Check new commits since last review pass
  • Review new/changed files
  • Check status of prior open findings
  • Post comprehensive review

Summary

This seventh pass reviews the single new commit pushed since review 6: 676578e docs(test-network): relay + validator onboarding runbook. No Rust source files changed. All seven open findings from prior reviews remain unaddressed.


New Commit: 676578eRELAY-NODE-ONBOARDING.md

This is a clear, accurate end-to-end runbook for the relay-fronted testnet and dynamic validator onboarding. The port table values were verified against add-relay-node.sh — all correct (node-6: RPC 8440, WS 18551, metrics 19105, relay 50005). The startup gotchas and troubleshooting notes document real failure modes encountered during development.

[D1-new] Broken internal link to a non-existent file

RELAY-NODE-ONBOARDING.md line 111:

see [`OPEN-QUESTIONS-committee-staking.md`](../../OPEN-QUESTIONS-committee-staking.md)

../../OPEN-QUESTIONS-committee-staking.md resolves to the repo root, but the file does not exist. This will render as a broken link in GitHub's UI. Either create the file or point the link to an existing resource (e.g. TODO-CRv2-NETWORKING.md at the repo root covers the same open questions).

Fix this →

[N1-new] OPERATOR_KEY trivial-key scheme underdocumented in the runbook

The runbook (line 56) describes the operator key as "deterministic from the index" without warning about its tiny key space. Review 6 flagged this in add-relay-node.sh (S2). The runbook is a natural place to add a one-line warning ("This key is a trivially-brute-forceable test secret; never use against a non-local network") to prevent operators from cargo-culting the pattern.


Prior Open Findings — All Still Open

The Rust source and testnet scripts are unchanged in this commit. The following findings from prior reviews are confirmed still open:

# Finding File One-liner?
S1 Rate limiters zeroed with no startup warn! bin/rayls-relay/src/main.rs:159–160 Yes — add tracing::warn! before the event loop
S2 Fixed seeds in RELAY_KEYS.md, no production guard on keypair_from_seed_env main.rs:193–201 No
C1 relay_reservations HashMap grows without bound, stale entries retried forever runtime.rs:83–100 No — needs retry counter
C2 ConnectionEstablished direct-conn warn! fires during pending-reservation window runtime.rs:169–170 Yes!self.relay_reservations.is_empty()self.relay_reservations.values().any(Option::is_some)
C3 retry_relay_reservations logs info! on every 15s retry — log spam during outages runtime.rs:93 Yes — first attempt info!, subsequent debug!
C4 DNS failure in resolve_relay_circuits logged to "network-kad" target command.rs:445 Yes — change target to "network"
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md No — convert to issue

C2, C3, and C4 are all genuine one-liners. They should be the easiest wins before merge.


Positive Highlights (new commit)

  • The startup gotcha ("genesis is created only once — wipe local-validators/ to regenerate") is exactly the right thing to document; it caused real confusion during development and will save future contributors significant debugging time.
  • The connections_by_path clarification ("cumulative counter, not a live count") prevents a common observability misread.
  • The readiness gate explanation and troubleshooting table for OwnableUnauthorizedAccount / ERC20InsufficientBalance correctly attribute the root cause (RLS proxy not live yet, wrong admin key) rather than just describing the symptom.

@procdump
procdump force-pushed the ba-circuit-relay-v2-poc branch from 676578e to d61bf01 Compare July 17, 2026 10:47
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 3m 35s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and understand the request
  • Read CLAUDE.md for repo-specific instructions (no CLAUDE.md present)
  • Read key changed files and verify open findings
  • Drafted and tested four fixes (push blocked by repo permissions — diffs below)
  • Post comprehensive review

Summary

Seven prior automated reviews covered this PR exhaustively. This eighth pass implements the four easy fixes that were flagged across multiple reviews and remained open, and confirms that two other previously-flagged issues (the broken link in RELAY-NODE-ONBOARDING.md and TODO-CRv2-NETWORKING.md) are either already fixed or still open.


Fixes Implemented (3 files, 8 lines changed)

The changes are committed locally as 9ffd1b0 but cannot be pushed due to repository permissions. The diffs are small enough to apply by hand:

S1 — bin/rayls-relay/src/main.rs: startup warning that rate limiting is disabled

-use tracing::info;
+use tracing::{info, warn};

 let relay_cfg = relay_config()?;
+warn!(
+    "rate limiting disabled — this relay is configured for testnet use only; \
+     do not deploy on a public network without re-enabling reservation/circuit limiters"
+);
 let mut swarm = libp2p::SwarmBuilder::with_existing_identity(key)

Justification: rate limiters are cleared unconditionally in relay_config() (lines 159–160). Without a runtime indicator, a mis-deployed relay on a public network has no visible signal that all DoS accounting is off. The warn! fires once at startup — never silenced by log filters that pass warn and above.


C2 — runtime.rs:170: suppress false-positive during pending-reservation window

-    && !self.relay_reservations.is_empty()
+    && self.relay_reservations.values().any(Option::is_some)

Justification: relay_reservations keys are inserted before listen_on completes (keys are added in StartListening, before the circuit-relay RESERVE handshake). During the startup window, the map is non-empty but no reservation is established. Outbound QUIC dials to committee peers that complete during this window incorrectly trigger the "direct connection to a non-relay peer on a relayed node" warning. Checking any(Option::is_some) pins the warning to nodes that have at least one active reservation, which is the actual intent.


C3 — runtime.rs:93: downgrade periodic retry log from info! to debug!

-    info!(target: "network", ?addr, "re-attempting relay reservation");
+    debug!(target: "network", ?addr, "re-attempting relay reservation");

Justification: handle_listener_closed already emits warn!(… "relay reservation lost; will retry to re-reserve") at the point of loss — that is the actionable operator signal. retry_relay_reservations fires every 15 s for every unestablished reservation; during a sustained relay outage this produces an info-level log every 15 s indefinitely. Operators watching info-level logs during an incident will be flooded. The warn on loss + the debug on each retry is the right split.


C4 — command.rs:445: fix log target for DNS relay resolution failures

-    warn!(target: "network-kad", %name, ?e, "failed to resolve /dnsaddr for relay discovery");
+    warn!(target: "network", %name, ?e, "failed to resolve /dnsaddr for relay discovery");

Justification: this is DNS/relay discovery code, not kademlia. Operators filtering with RUST_LOG=network_kad=warn will see this warning attributed to kademlia, making relay DNS misconfiguration hard to find and potentially obscuring real kademlia warnings.


Still Open (from prior reviews)

# Finding File Priority
S2 Fixed test seeds in RELAY_KEYS.md with no production guard in keypair_from_seed_env main.rs:193–201 Medium
C1 relay_reservations HashMap grows without bound; stale entries retried forever after config churn runtime.rs:83–100 Medium
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Low
script-C1 stake-relay-node.sh step 3 swallows all cast send failures with || echo stake-relay-node.sh:111 Low
script-C3 add-relay-node.sh peer-id regex 12D3KooW[A-Za-z0-9]* matches partial log writes add-relay-node.sh:117 Low

None of the remaining items block the PoC topology described in the PR. S2 and C1 should be tracked before production promotion.


Confirmed Closed

  • D1-new (review 7): broken link to OPEN-QUESTIONS-committee-staking.md — the file has already been updated; the link is no longer present in the current RELAY-NODE-ONBOARDING.md.

Positive Highlights

The overall architecture is sound. Specifically worth preserving as patterns:

  • dns(or(quic, relay)) transport ordering rationale in constructor.rs:163–174 is landmark inline documentation.
  • PeerManager field ordering justification in behaviour.rs:27–37 preempts a real class of libp2p NetworkBehaviour poll-ordering bugs.
  • advertised_relay_covered resolves by relay peer id (not address string), with four focused unit tests covering the edge cases.
  • reservation_allow_list using RateLimiter as a pure boolean gate is compact and avoids a new libp2p callback hook.
  • retry_relay_reservations correctly models "desired but not active" via Option<ListenerId>, keeping relay recovery self-healing without a restart.

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 3m 33s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and read new commits since last review
  • Check status of prior open findings
  • Implement easy one-liner fixes (S1, C2, C3, C4 from prior review) — applied but push blocked by repo permissions; diffs below
  • Review new commit 3766d61 fix(consensus): promote newly-staked validators and grow the committee
  • Post comprehensive update

Fixes Applied Locally (Push Blocked — Apply Manually)

Four fixes from prior reviews were prepared as commit fc2ff34 but cannot be pushed due to repository permissions. They are each 1–4 lines:

S1 — relay startup warning (bin/rayls-relay/src/main.rs)

-use tracing::info;
+use tracing::{info, warn};

 let relay_cfg = relay_config()?;
+warn!(
+    "rate limiting disabled — this relay is configured for testnet use only; \
+     do not deploy on a public network without re-enabling reservation/circuit limiters"
+);
 let mut swarm = libp2p::SwarmBuilder::with_existing_identity(key)

C2 — suppress false-positive direct-conn warn during pending-reservation window (runtime.rs:169–170)

-    && !self.relay_reservations.is_empty()
+    && self.relay_reservations.values().any(Option::is_some)

C3 — downgrade periodic relay-retry log to avoid flood during outages (runtime.rs:93)

-    info!(target: "network", ?addr, "re-attempting relay reservation");
+    debug!(target: "network", ?addr, "re-attempting relay reservation");

C4 — fix log target for DNS relay-resolution failures (command.rs:445)

-    warn!(target: "network-kad", %name, ?e, "failed to resolve /dnsaddr for relay discovery");
+    warn!(target: "network", %name, ?e, "failed to resolve /dnsaddr for relay discovery");

New Commit: 3766d61fix(consensus): promote newly-staked validators and grow the committee

This is a substantive bug fix with two compounding issues correctly identified and fixed. The commit message and inline documentation are thorough; the open questions are honestly called out. Review below.

Correctness

[C1] Bug 1 fix is correct — Observer→CvvInactive promotion at decide_node_mode is sound

network.rs:78: the new arm promotes in_committee && !observer_flag && prior==Observer to CvvInactive("joined-committee"). The invariant is:

  • initial_epoch = false (so prior-mode is meaningful — the node lived through a previous epoch in this process)
  • in_committee = true (on-chain, the node is now a committee member)
  • observer_flag = false (not a deliberately configured observer)
  • prior_mode = Observer (was following as a dynamic observer)

Promoting to CvvInactive is the correct step: the node catches up before voting, then the bridge subscriber requests CvvActive once synced. Arriving at CvvActive directly would be unsafe (it would start proposing/voting immediately). Staying Observer is the confirmed bug (silent committee member counted toward quorum).

The promotion fires exactly once per stake event per process lifetime: after the first epoch boundary where prior==Observer && in_committee, the node transitions to CvvInactive, so at the next epoch boundary prior_mode is CvvInactive and it takes the "prior-mode-inactive" arm instead.

[C2] REVISIT Question 2: re-add within one process lifetime is safe but unverified

The author asks whether in_committee && !observer_flag && prior==Observer can arise from a reason other than "just staked in" (e.g. a node that was previously a committee member, left (unstaked), ran as Observer, then re-staked all within the same process). The promotion direction (CvvInactive) is safe in this case too — CvvInactive is always safer than CvvActive for a returning member. The leave/unstake path isn't verified end-to-end (noted in the open questions), but the mode assignment itself won't cause unsafe behavior (voting before synced).

[C3] REVISIT Question 3: hardfork asymmetry is not a real risk

The author asks whether the promotion (not fork-gated) and the committee-growth that makes a node in_committee (behind DynamicCommitteeSize) can disagree across the fork boundary. They cannot: on testnet/mainnet where DynamicCommitteeSize is Never, the committee size is pinned to the current committee's length via the old next_committee_size path, so a newly staked validator is never added to the next committee (the on-chain shuffle+truncate evicts them), meaning in_committee stays false for the newcomer. The "joined-committee" arm never fires on testnet/mainnet until DynamicCommitteeSize is activated — at which point the committee CAN grow and the promotion is correct. The asymmetry is safe.

[C4] Bug 2 fix: get_active_validators() is a new EVM contract call on the epoch hot path

block.rs:484: get_active_validators() is called on every epoch transition where DynamicCommitteeSize is active (local/devnet). This adds one additional static contract call alongside the pre-existing get_epoch_committee_validators(). On testnet/mainnet (where the fork is Never) this path is unreachable. The additional call is acceptable for the PoC; worth a // one extra contract call per epoch comment if this becomes a production concern.

Design

[D1] DynamicCommitteeSize activation block placeholders need follow-up before any testnet/mainnet deploy

chainspec.rs:

// TODO: choose a testnet activation block before deploy.
(Self::DynamicCommitteeSize, ForkCondition::Never),

Both testnet and mainnet schedules leave the fork as Never. The constants TESTNET_DYNAMIC_COMMITTEE_SIZE_BLOCK and MAINNET_DYNAMIC_COMMITTEE_SIZE_BLOCK are intentionally absent. This is correct for a PoC, but merging the PR with a Never-on-production fork means the validator-onboarding fix is testnet/mainnet-invisible until someone adds those constants and flips the condition. Recommend converting the TODO comments to a GitHub issue that blocks the next testnet deploy.

[D2] DynamicCommitteeSize is unbounded — no upper cap or committee sampling

block.rs:492: return Ok(active_validators.len()) sizes the committee to the full active set with no cap. The commit explicitly calls this out in the open questions: "committee = all active validators; no upper cap / committee sampling for large validator sets." The on-chain _checkCommitteeSize prevents a committee larger than the active validator set, but doesn't impose a maximum. For a testnet with a small, known validator count this is fine. Tracking the upper-cap / BFT-committee-sampling design before reaching any meaningful validator count would be prudent.

Nits

[N1] Test does not cover initial_epoch = true with prior_mode = Observer

The new test test_decide_mode_observer_joins_committee_on_stake correctly tests initial_epoch = false. For completeness, a test with initial_epoch = true, has_local_history = true (a node restarting at the genesis/first epoch boundary that was previously an observer) would confirm it takes "has-local-history" → CvvInactive rather than the "joined-committee" arm. The code is correct (the !initial_epoch guard means initial_epoch = true falls through to the history check), but the coverage gap is worth closing.


Prior Open Findings — Status

# Finding File Status
S1 Relay startup: no warn! that rate limiting is disabled bin/rayls-relay/src/main.rs Fixed locally (apply diff above)
C2 ConnectionEstablished warn fires during pending-reservation window runtime.rs:169–170 Fixed locally (apply diff above)
C3 retry_relay_reservations logs info! every 15s during outages runtime.rs:93 Fixed locally (apply diff above)
C4 DNS fail in resolve_relay_circuits logged to "network-kad" target command.rs:445 Fixed locally (apply diff above)
S2 Fixed seeds in RELAY_KEYS.md, no production guard in keypair_from_seed_env main.rs:193–201 Open
C1 relay_reservations HashMap grows without bound; stale entries retried forever runtime.rs:83–100 Open
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Open
D2 DynamicCommitteeSize testnet/mainnet activation blocks not chosen yet chainspec.rs Open (new)

Positive Highlights (new commit)

  • The two-bug isolation (silent-committee-member vs. committee-eviction-on-join) is accurate: both cause a stall but through different mechanisms, and the commit correctly fixes them independently.
  • The REVISIT block in decide_node_mode is exactly the right way to land a behavior change that has open design questions — the fix is deployed, the caveats are documented at the call site, and reviewers are explicitly directed.
  • The hardfork activation being Never on testnet/mainnet is the prudent default: local/devnet exercises the new behavior end-to-end without risking live networks until the activation block is chosen.
  • DynamicCommitteeSize correctly skipped in apply_genesis_account_changes (no pre-loaded accounts needed, correct continue).

return match prior_mode {
NodeMode::CvvActive => (NodeMode::CvvActive, "prior-mode-active"),
NodeMode::CvvInactive => (NodeMode::CvvInactive, "prior-mode-inactive"),
NodeMode::Observer => (NodeMode::Observer, "prior-mode-observer"),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This switch bothers me.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Do we have to gate this behind the fork as well?

@procdump
procdump force-pushed the ba-circuit-relay-v2-poc branch from 3766d61 to 2954307 Compare July 17, 2026 11:44
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 3m 33s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and read the flagged code
  • Analyze the "switch" concern at network.rs:47–79
  • Post analysis

decide_node_mode — the Observer arm in context (network.rs:46–79)

The match prior_mode block has a structural asymmetry that is worth making explicit:

  • CvvActive and CvvInactive arms are identity functions — they return prior_mode unchanged, i.e., "preserve what we had".
  • Observer is the only arm with novel behavior — it does a real promotion to CvvInactive under an implicit precondition that observer_flag=false was already checked three lines above the match.

This conflation of "preserve" and "promote" in a single match is what makes the block unsettling at first read.


Addressing the three REVISIT questions

Q1 — Is decide_node_mode the right layer?

Yes, conditionally. Two stickiness guards exist:

  • request_mode_transition (consensus_bus.rs:717–720): returns false and no-ops if current == NodeMode::Observer — so automated signals (certifier demotion, state-sync self-demotion) can never promote an Observer.
  • run_mode_transition (transition.rs:309–310): skips the write if prior_mode == Observer && target_mode != Observer.

Both guards are designed to prevent automated promotion of configured watcher nodes. They are not aware of observer_flag. The decide_node_mode function checks observer_flag explicitly at lines 40–42 before the match is reached, so the Observer arm is only reachable when observer_flag=false is guaranteed. The bypass is therefore intentional and correct at this layer.

If you want this to live elsewhere, the closest alternative is an explicit mode_transition signal emitted by the on-chain staking event subscriber. That would thread through request_mode_transition, which currently blocks Observer → anything. You'd need to bypass or extend the stickiness guard there too — so the bypass would just move, not disappear.

Q2 — Can in_committee && !observer_flag && prior==Observer arise for a reason other than "just staked"?

Yes: a node that was removed from the committee (in_committee flips false → Observer) and then re-added within one process lifetime hits this arm. The resulting CvvInactive is actually the correct behavior here too (catch up before proposing, regardless of whether the node was freshly staked or re-admitted). So the false positive has correct semantics.

The worrying case would be a stale committee view causing in_committee to return true incorrectly. But consensus_config.in_committee() is derived from the on-chain state that consensus_config loaded, not a guess — so this reduces to a data consistency problem that affects the wider system, not specifically this arm.

Q3 — DynamicCommitteeSize hardfork interaction

The committee growth that puts the node in_committee is fork-gated; in_committee returning true means the fork has activated on this node's committee view. The promotion in the Observer arm fires downstream of that check, so it cannot trigger before the fork. Disagreement across the fork boundary (two nodes with different committee views) is a broader safety concern, not unique to this arm.


Structural recommendation

The two "preserve" arms could be written more clearly to expose that Observer is the exceptional case:

if !initial_epoch {
    match prior_mode {
        // Identity: active/inactive nodes preserve their prior mode across epochs.
        mode @ (NodeMode::CvvActive | NodeMode::CvvInactive) => {
            return (mode, if matches!(mode, NodeMode::CvvActive) { "prior-mode-active" } else { "prior-mode-inactive" });
        }
        // Promote: in_committee + !observer_flag + prior==Observer means the node was
        // admitted to the committee while running as a dynamic observer. Join as
        // CvvInactive (catch up, then promote to Active via try_rejoin_consensus).
        // observer_flag=false is guaranteed by the check at line 40-42.
        NodeMode::Observer => return (NodeMode::CvvInactive, "joined-committee"),
    }
}

Or more tersely — if the distinct reason strings aren't needed for observability:

if !initial_epoch && prior_mode != NodeMode::Observer {
    return (prior_mode, "prior-mode-preserved");
}
if !initial_epoch {
    // Observer + in_committee + !observer_flag: dynamically admitted to committee.
    return (NodeMode::CvvInactive, "joined-committee");
}

Either form makes the Observer arm's non-identity semantics impossible to skim past.


Bottom line

The logic is sound given observer_flag is already gated. The concern is structural: the current match implies all three arms are doing the same kind of thing (preserving prior mode), hiding that Observer is the only arm with novel behavior. Restructuring to make that asymmetry explicit — or at minimum adding a // only non-identity arm; observer_flag=false guaranteed above comment at the Observer => line — would resolve the unease without touching the semantics.

@procdump
procdump force-pushed the ba-circuit-relay-v2-poc branch from 2954307 to 9122d90 Compare July 17, 2026 11:51
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 4m 5s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and understand the request
  • Read key files (runtime.rs, command.rs, main.rs, network.rs)
  • Apply one-liner fixes (S1, C2, C3, C4)
  • Restructure decide_node_mode switch to address @procdump's concern
  • Commit and push fixes — push blocked by repo permissions; diffs below

Fixes Prepared (commit e8729ff — apply manually)

Push failed with 403. Four changes are ready; all are small enough to apply by hand.

S1 — bin/rayls-relay/src/main.rs: startup warning that rate limiting is disabled

-use tracing::info;
+use tracing::{info, warn};

 let relay_cfg = relay_config()?;
+warn!(
+    "rate limiting disabled — this relay is configured for testnet use only; \
+     do not deploy on a public network without re-enabling reservation/circuit limiters"
+);
 let mut swarm = libp2p::SwarmBuilder::with_existing_identity(key)

Fires once at startup. Any log filter that passes warn (the default) will always surface it, making accidental mis-deployment visible immediately.


C2 — runtime.rs:169: suppress false-positive during pending-reservation window

-    && !self.relay_reservations.is_empty()
+    && self.relay_reservations.values().any(Option::is_some)

relay_reservations keys are inserted as soon as StartListening is processed — before the circuit-relay RESERVE handshake completes. During that window the map is non-empty but values() are all None. Outbound QUIC dials that complete in this window incorrectly triggered the "direct connection to a non-relay peer on a relayed node" warn. The fix ties the warn to at least one active reservation.


C3 — runtime.rs:93: downgrade periodic retry log from info! to debug!

-    info!(target: "network", ?addr, "re-attempting relay reservation");
+    debug!(target: "network", ?addr, "re-attempting relay reservation");

handle_listener_closed already emits warn! at the moment of loss — that is the actionable signal. The retry fires every 15 s per down relay; during a sustained outage this produces info-level noise indefinitely. warn on loss + debug on each retry is the right split.


C4 — command.rs:445: fix log target for DNS relay-resolution failures

-    warn!(target: "network-kad", %name, ?e, "failed to resolve /dnsaddr for relay discovery");
+    warn!(target: "network", %name, ?e, "failed to resolve /dnsaddr for relay discovery");

This is relay/DNS discovery code, not kademlia. Operators filtering with RUST_LOG=network_kad=warn would see relay DNS failures attributed to kademlia, obscuring real kademlia warnings and making relay misconfiguration hard to find.


decide_node_mode switch — addressing @procdump's concern

The concern is correct: the current match prior_mode block looks uniform but is not. CvvActive and CvvInactive are pure identity functions; Observer is the only arm with non-identity semantics (it promotes rather than preserves). That asymmetry is invisible at a glance.

Restructured version (also in the commit):

-    return match prior_mode {
-        NodeMode::CvvActive => (NodeMode::CvvActive, "prior-mode-active"),
-        NodeMode::CvvInactive => (NodeMode::CvvInactive, "prior-mode-inactive"),
-        // ============== XXX / REVISIT ... (30 lines) ==============
-        NodeMode::Observer => (NodeMode::CvvInactive, "joined-committee"),
-    };
+    // CvvActive / CvvInactive are identity: preserve mode across epochs.
+    // Observer is the only non-identity arm: a dynamic observer just admitted to the
+    // committee (in_committee=true, observer_flag=false — both checked above) must be
+    // promoted to CvvInactive so it catches up before proposing/voting. Staying Observer
+    // leaves a silent committee member counted toward quorum but never certifying, which
+    // stalls consensus. The promotion is safe for the re-added case too (CvvInactive is
+    // always safer than CvvActive for a returning member). `decide_node_mode` is the right
+    // layer: the two other Observer-stickiness guards (run_mode_transition,
+    // request_mode_transition) target automated signals, not epoch-boundary mode resolution.
+    return match prior_mode {
+        mode @ (NodeMode::CvvActive | NodeMode::CvvInactive) => {
+            let reason = if matches!(mode, NodeMode::CvvActive) {
+                "prior-mode-active"
+            } else {
+                "prior-mode-inactive"
+            };
+            (mode, reason)
+        }
+        // Only non-identity arm; observer_flag=false guaranteed by the check at line 40.
+        NodeMode::Observer => (NodeMode::CvvInactive, "joined-committee"),
+    };

The mode @ binding explicitly groups the identity cases so the Observer arm cannot be skimmed past as "another identity case". The long REVISIT block is replaced with a settled-question comment — the prior reviews answered all three open questions:

  1. Is decide_node_mode the right layer? Yes. The stickiness guards (run_mode_transition, request_mode_transition) target automated signals, not epoch-boundary mode resolution. This is the correct place.
  2. Can in_committee && !observer_flag && prior==Observer arise for a reason other than "just staked"? Yes (re-added within one lifetime), but CvvInactive is correct in that case too — always safer than CvvActive for a returning member.
  3. DynamicCommitteeSize hardfork interaction? Safe: the fork that grows the committee is Never on testnet/mainnet; on local/devnet the promotion only fires after in_committee flips true from the on-chain state, which is fork-gated.

Still Open (no changes in latest commits)

# Finding File Priority
S2 Fixed test seeds in RELAY_KEYS.md, no production guard in keypair_from_seed_env main.rs Medium
C1 relay_reservations HashMap grows without bound; stale entries retried forever runtime.rs:83–100 Medium
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Low — convert to GH issue
script-C3 add-relay-node.sh peer-id regex 12D3KooW[A-Za-z0-9]* matches partial log writes add-relay-node.sh:117 Low

S2 and C1 are the only items worth addressing before a production promotion.

| RaylsHardFork::TransactionLoadBalancing
| RaylsHardFork::EmptyOutputBlock => continue,
| RaylsHardFork::EmptyOutputBlock
| RaylsHardFork::DynamicCommitteeSize => continue,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

is the committee fix ok to be per block as it should flip at epoch boundary

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 2m 57s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Read CLAUDE.md for repo-specific instructions (none present)
  • Check new commits since last review pass (5713bed, e459a6c)
  • Review new commits
  • Address open review comments from @procdump
  • Post comprehensive review

Responding to @procdump's inline review questions

network.rs:48 — "Do we have to gate this behind the fork as well?"

No explicit fork gate is required — the protection is implicit but real. Without DynamicCommitteeSize active, the committee size is pinned to the current committee's length: shuffle_new_committee enlarges the active set then truncates it back down (randomly evicting an incumbent), so the newcomer's in_committee never flips to true. The Observer → CvvInactive arm at line 78 is therefore unreachable on testnet/mainnet until the fork activates.

That said, adding an explicit guard provides defense-in-depth and makes the intent self-documenting. If you want belt-and-suspenders:

// Only reachable once DynamicCommitteeSize activates (prior to that,
// newly-staked validators are evicted by shuffle_new_committee, so
// in_committee stays false and this arm is unreachable).
NodeMode::Observer if consensus_config.is_dynamic_committee_size_active() =>
    (NodeMode::CvvInactive, "joined-committee"),
NodeMode::Observer => (NodeMode::Observer, "prior-mode-observer"),

The current code is correct without this; the gate is a readability/safety choice rather than a correctness requirement.


hardforks/mod.rs:124 — "is the committee fix ok to be per block as it should flip at epoch boundary"

Yes, the classification is correct. DynamicCommitteeSize being in the continue list means it runs no state migration at block boundaries — apply_genesis_account_changes skips it entirely. "Continuous behavioral fork" here means only: no pre-loaded account state to initialize. The actual behavior change — is_dynamic_committee_size_active_at_block(block_number) toggling the committee-size calculation — is invoked only inside next_committee_size() (block.rs:483), which is called from concludeEpoch execution at epoch boundaries, not on every block. The epoch-boundary semantics are preserved.


New Commits

5713bedfix(bridge): demote to Observer instead of panicking on unfetchable catch-up batch

Correctness — Sound

  • is_batch_fetch_error() correctly covers MissingFetchedBatch and ClientRequestsFailed; the test at subscriber.rs:1428–1432 pins the predicate.
  • CvvInactive → Observer demotion calls request_mode_transition(NodeMode::Observer). The existing Observer-stickiness guards (run_mode_transition, request_mode_transition) only block automated promotion from Observer — they do not block demotion to Observer — so this transition is allowed.
  • Observer follow silent-exit: correct; spawn_subscriber re-arms at the next epoch start.

One thing to be aware of (acknowledged, not a bug)

After the catch-up task demotes to Observer mid-epoch, no follow task is spawned for the remainder of that epoch — spawn_subscriber is called only at epoch transitions. The node is effectively idle (no catch-up, no follow) until the next epoch boundary. This is correctly documented as "survivable degraded state that self-heals once connectivity returns," and is the explicit design intent. Worth keeping the XXX / REVISIT comment as a production tracking marker.

Nit: comment duplication

The full "NB: this is NOT garbage collection" explanation is duplicated in full between the CvvInactive catch-up arm (lines 122–138) and the Observer follow arm (lines 170–177). The follow arm currently cross-references the catch-up arm ("see the catch-up arm above"), but only in the short form. Consolidating to one full comment block + a cross-reference in the other would reduce drift risk when the explanation is updated.


e459a6cfeat(test-network): self-contained single-node lifecycle + chaos-restart tooling

stop-relay-node.sh

Clean and correct. The SIGTERM-wait-forever / SIGTERM-then-kill-9 split between node and relay matches their respective cleanup semantics (stateful vs. stateless).

local-testnet.sh refactoring

The build_relay_env extraction correctly unifies the env construction path between --start and --start-validator, eliminating the previous drift risk. start_relay_pair / stop_relay_pair are idempotent due to the relay_alive guard — clean.

bounce-node.sh

  • is_caught_up polls rayls_nodeStatus over RPC — correct approach.
  • wait_until_caught_up spins indefinitely by design (a node that never recovers should block the loop to surface the failure). This is consistent with the stop_validator indefinite-wait philosophy.
  • One fragile coupling: bounce-node.sh:32 derives the added-node RPC port as $((8545 - (INSTANCE - 1))) with a comment "Keep in sync with add-relay-node.sh if that formula changes." This is a silent-divergence risk. Consider extracting the port derivation to a shared helper (e.g. a function in a sourced common.sh) if add-relay-node.sh changes the formula in future.

Minor: stop_validator indefinite wait is correct but can mask test hangs

local-testnet.sh:139–148: the indefinite SIGTERM wait for graceful shutdown is explicitly intended ("a hung graceful shutdown is a real bug worth catching"). This is the right philosophy for a chaos-testing tool. The 30-second progress log (count % 30 == 0) provides visibility. Fine as-is.


Prior Open Findings — Status Unchanged

The four one-liner fixes (S1, C2, C3, C4) were prepared locally in prior review passes but push was blocked. The Rust source is still unchanged from the last review. For completeness:

# Finding File Status
S1 Relay startup: no warn! that rate limiting is disabled bin/rayls-relay/src/main.rs:159–160 Open — one-liner
C2 ConnectionEstablished warn fires during pending-reservation window runtime.rs:169 Open — one-liner: any(Option::is_some)
C3 retry_relay_reservations logs info! every 15s during outages runtime.rs:93 Open — one-liner: info!debug!
C4 DNS failure in resolve_relay_circuits logged to "network-kad" target command.rs:445 Open — one-liner
S2 Fixed test seeds in RELAY_KEYS.md, no production guard main.rs:193–201 Open
C1 relay_reservations HashMap grows without bound runtime.rs:83–100 Open
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Open

S1, C2, C3, C4 are all genuine one-liners and should be the easiest wins before merge. S2 and C1 are the only items worth addressing before a production promotion.

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 2m 43s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and check new commits since last review
  • Check CLAUDE.md for repo-specific instructions (none present)
  • Review new commits (05c61ee, 506a1ec)
  • Apply pending one-liner fixes (S1, C2, C3, C4) — push blocked; diffs below
  • Post comprehensive review

New Commits Since Last Review

506a1ecchore(batch-tracker): demote per-tx nonce_too_high logging to debug

Correct and unambiguously right. The nonce_too_high_detail loop emitted one warn! per dropped tx; during a single-sender nonce burst this is thousands of log lines. The per-sender nonce_range_for_sender summary already carries the actionable gap info at warn!; the per-tx detail belongs at debug! for when you're actually chasing a specific gap. The commit message explains this clearly. No issues.

05c61eefix(test-network): consistent added-node DNS view + DOWN_SECS knob in bounce-node

Two independent fixes to bounce-node.sh:

DNSMASQ_PORT default correction — The prior default of 5354 (public/relay view) caused a silent topology flip across a bounce: add-relay-node.sh defaults to 5353 (private/direct), so adding a node and then bouncing it changed its committee-resolution view without any operator signal. Correcting the default to 5353 makes add + bounce consistent by default. Operators who want the relay view on both sides now pass DNSMASQ_PORT=5354 explicitly — which is the right place to encode intent.

DOWN_SECS knob — The new sleep "$DOWN_SECS" between stop and restart exercises the catch-up path (node falls behind across epoch boundaries). The comment documents the intended use case accurately: set above ~2× epoch duration to trigger the forward-streamer's idle-probe path. Clean addition. No issues.


Fixes Applied Locally (Push Blocked — Apply Manually)

All four fixes are committed locally as 9d227c2 but cannot be pushed due to repository permissions. They are 1–6 lines each:

S1 — bin/rayls-relay/src/main.rs: startup warning that rate limiting is disabled

-use tracing::info;
+use tracing::{info, warn};

 let relay_cfg = relay_config()?;
+warn!(
+    "rate limiting disabled — this relay is configured for testnet use only; \
+     do not deploy on a public network without re-enabling reservation/circuit limiters"
+);
 let mut swarm = libp2p::SwarmBuilder::with_existing_identity(key)

Rate limiters are zeroed unconditionally in relay_config() (lines 159–160). Without a runtime indicator, a mis-deployed relay on a public network has no visible signal that all DoS accounting is off. This warn! fires once at startup — any log filter that passes warn and above (the default) will always surface it.


C2 — runtime.rs:170: suppress false-positive during pending-reservation window

-    && !self.relay_reservations.is_empty()
+    && self.relay_reservations.values().any(Option::is_some)

relay_reservations keys are inserted as soon as StartListening is processed — before the circuit-relay RESERVE handshake completes. During that window the map is non-empty but all values are None. Outbound QUIC dials that complete in this startup window incorrectly triggered the "direct connection to a non-relay peer on a relayed node" warning. The fix ties the warning to at least one active (fully established) reservation.


C3 — runtime.rs:93: downgrade periodic retry log from info! to debug!

-    info!(target: "network", ?addr, "re-attempting relay reservation");
+    debug!(target: "network", ?addr, "re-attempting relay reservation");

handle_listener_closed already emits warn! at the moment of loss — that is the actionable operator signal. retry_relay_reservations fires every 15 s per unestablished reservation; during a sustained relay outage this generates an info!-level line every 15 s indefinitely. warn! on loss + debug! on each retry is the right split.


C4 — command.rs:445: fix log target for DNS relay-resolution failures

-    warn!(target: "network-kad", %name, ?e, "failed to resolve /dnsaddr for relay discovery");
+    warn!(target: "network", %name, ?e, "failed to resolve /dnsaddr for relay discovery");

This is relay/DNS discovery code, not kademlia. Operators filtering with RUST_LOG=network_kad=warn would see this warning attributed to kademlia, obscuring real kademlia warnings and making relay DNS misconfiguration hard to find.


Prior Open Findings — Status

# Finding File Status
S1 Relay startup: no warn! that rate limiting is disabled bin/rayls-relay/src/main.rs Fixed locally — apply diff above
C2 ConnectionEstablished warn fires during pending-reservation window runtime.rs:169 Fixed locally — apply diff above
C3 retry_relay_reservations logs info! every 15s during outages runtime.rs:93 Fixed locally — apply diff above
C4 DNS failure in resolve_relay_circuits logged to "network-kad" command.rs:445 Fixed locally — apply diff above
S2 Fixed test seeds in RELAY_KEYS.md, no production guard in keypair_from_seed_env main.rs:193–201 Open — medium priority, pre-production
C1 relay_reservations HashMap grows without bound; stale entries retried forever after config churn runtime.rs:83–100 Open — medium priority, pre-production
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Open — convert to GH issue before merge
script-C3 add-relay-node.sh peer-id regex 12D3KooW[A-Za-z0-9]* matches partial log writes add-relay-node.sh:117 Open — low priority

S2 and C1 are the only remaining items worth addressing before a production promotion. D1 (the TODO file) should be converted to a GitHub issue before merge.

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 2m 47s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Check new commits since last review
  • Read CLAUDE.md for repo-specific instructions (none present)
  • Apply pending one-liner fixes (S1, C2, C3, C4) — push blocked by repo permissions; diffs below
  • Review new commit (119ef1f)
  • Post comprehensive update

New Commit: 119ef1fdocs(test-network): clarify bounce-node DNS view per node kind

Clean and correct. This doc-only commit strengthens the chaos-test section of RELAY-NODE-ONBOARDING.md in two ways:

  1. Base validator bounce: adds a comment noting that DNSMASQ_PORT is not honored for base validators because build_relay_env pins them to 5353 (private/direct view). Without this note, an operator would try DNSMASQ_PORT=5354 for a base-validator bounce and wonder why it has no effect.

  2. Added-node bounce: the example now reads ADDED=1 DNSMASQ_PORT=5354 ./fork_test_configs/bounce-node.sh 6 (was ADDED=1 ./fork_test_configs/bounce-node.sh 6). This is a real bug fix: commit 05c61ee changed the bounce default from 5354 to 5353, so the old example would cause an added relay node to silently switch from relay-circuit topology to direct-QUIC topology across a bounce. The explicit DNSMASQ_PORT=5354 makes the intent deterministic and warns operators to use the same view they added the node with.

No issues.


Fixes Applied Locally (Push Blocked — Apply Manually)

Four changes were staged and committed locally as dd43c64 but cannot be pushed due to repository permissions. All are 1–6 lines:

S1 — bin/rayls-relay/src/main.rs: startup warning that rate limiting is disabled

-use tracing::info;
+use tracing::{info, warn};

 let relay_cfg = relay_config()?;
+warn!(
+    "rate limiting disabled — this relay is configured for testnet use only; \
+     do not deploy on a public network without re-enabling reservation/circuit limiters"
+);
 let mut swarm = libp2p::SwarmBuilder::with_existing_identity(key)

Rate limiters are zeroed unconditionally in relay_config(). Any log filter passing warn and above (the default) will always surface this at startup.


C2 — runtime.rs:170: suppress false-positive during pending-reservation window

-    && !self.relay_reservations.is_empty()
+    && self.relay_reservations.values().any(Option::is_some)

relay_reservations keys are inserted before the circuit-relay RESERVE handshake completes — so during the startup window the map is non-empty but all values are None. Outbound QUIC dials that complete in this window incorrectly triggered the "direct connection to a non-relay peer on a relayed node" warning. The fix ties the warning to at least one active reservation.


C3 — runtime.rs:93: downgrade periodic retry log from info! to debug!

-    info!(target: "network", ?addr, "re-attempting relay reservation");
+    debug!(target: "network", ?addr, "re-attempting relay reservation");

handle_listener_closed already emits warn! at the moment of loss — that is the actionable operator signal. The retry fires every 15 s per unestablished reservation; during a sustained outage this generates info!-level noise indefinitely. warn! on loss + debug! on each retry is the right split.


C4 — command.rs:445: fix log target for DNS relay-resolution failures

-    warn!(target: "network-kad", %name, ?e, "failed to resolve /dnsaddr for relay discovery");
+    warn!(target: "network", %name, ?e, "failed to resolve /dnsaddr for relay discovery");

This is relay/DNS discovery code, not kademlia. The wrong target misattributes relay DNS failures to kademlia when operators filter by subsystem.


Remaining Open Findings

# Finding File Priority
S2 Fixed test seeds in RELAY_KEYS.md with no production guard in keypair_from_seed_env bin/rayls-relay/src/main.rs Medium — pre-production
C1 relay_reservations HashMap grows without bound; stale entries retried forever after config churn runtime.rs:83–100 Medium — pre-production
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Low — convert to GH issue before merge
script-C3 add-relay-node.sh peer-id regex 12D3KooW[A-Za-z0-9]* matches partial log writes add-relay-node.sh:117 Low

S2 and C1 are the only items worth addressing before a production promotion. Everything else is unblocked for merge as a PoC.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 3m 39s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Read CLAUDE.md for repo-specific instructions (none present)
  • Check new commits since last review
  • Apply pending one-liner fixes (S1, C2, C3, C4) — push blocked by repo permissions; diffs below
  • Review new commit (15fb572)
  • Post comprehensive review

Fixes Applied Locally (Push Blocked — Apply Manually)

Four changes were committed locally as 3622b21 but cannot be pushed due to repository permissions. All are 1–6 lines:

S1 — bin/rayls-relay/src/main.rs: startup warning that rate limiting is disabled

-use tracing::info;
+use tracing::{info, warn};

 let relay_cfg = relay_config()?;
+warn!(
+    "rate limiting disabled — this relay is configured for testnet use only; \
+     do not deploy on a public network without re-enabling reservation/circuit limiters"
+);
 let mut swarm = libp2p::SwarmBuilder::with_existing_identity(key)

Rate limiters are zeroed unconditionally in relay_config() (lines 159–160). Any log filter passing warn and above (the default) will always surface this at startup, making accidental mis-deployment visible immediately.


C2 — runtime.rs:170: suppress false-positive during pending-reservation window

-    && !self.relay_reservations.is_empty()
+    && self.relay_reservations.values().any(Option::is_some)

relay_reservations keys are inserted before the circuit-relay RESERVE handshake completes — so during the startup window the map is non-empty but all values are None. Outbound QUIC dials completing in this window incorrectly triggered the "direct connection to a non-relay peer on a relayed node" warning. The fix ties the warning to at least one active reservation.


C3 — runtime.rs:93: downgrade periodic retry log from info! to debug!

-    info!(target: "network", ?addr, "re-attempting relay reservation");
+    debug!(target: "network", ?addr, "re-attempting relay reservation");

handle_listener_closed already emits warn! at the moment of loss — that is the actionable signal. The retry fires every 15 s per unestablished reservation; during a sustained outage this generates info!-level noise indefinitely. warn! on loss + debug! on each retry is the right split.


C4 — command.rs:445: fix log target for DNS relay-resolution failures

-    warn!(target: "network-kad", %name, ?e, "failed to resolve /dnsaddr for relay discovery");
+    warn!(target: "network", %name, ?e, "failed to resolve /dnsaddr for relay discovery");

This is relay/DNS discovery code, not kademlia. The wrong target misattributes relay DNS failures to kademlia when operators filter by subsystem (RUST_LOG=network_kad=warn).


New Commit: 15fb572feat(test-network): MULTI_LISTEN_BIND knob, default direct listener to loopback

Correct and a good security improvement. The previous hardcoded 0.0.0.0 for direct listeners meant every MULTI_LISTEN=1 node exposed a direct QUIC port on all interfaces, including any external-facing one. Defaulting to 127.0.0.1 correctly matches the direct dnsaddr records (which already advertised 127.0.0.1), so co-located validators still mesh directly while the listener is never reachable cross-host. The comment and docs update are clear.

[N1] Stale comment at local-testnet.sh:703–704

# MULTI_LISTEN is only meaningful with a relay mode, where the node otherwise has no direct
# listener. The direct listener binds 0.0.0.0, so there's no interface/alias to set up.

The second sentence ("binds 0.0.0.0") is now stale — the binding address is MULTI_LISTEN_BIND (default 127.0.0.1). One word change:

# listener. The listener binds MULTI_LISTEN_BIND (default 127.0.0.1), so no interface/alias setup is needed.

[N2] bounce-node.sh exports MULTI_LISTEN but not MULTI_LISTEN_BIND

bounce-node.sh:52:

export MULTI_LISTEN="${MULTI_LISTEN:-0}"   # local-testnet.sh reads this from the env

MULTI_LISTEN_BIND is not similarly exported. In practice this is harmless: if bounce-node.sh was invoked with MULTI_LISTEN_BIND=0.0.0.0 on the command line, the env var propagates to child bash processes automatically (it's an environment variable, not just a shell variable). But the asymmetry is subtle — a reader seeing the explicit export MULTI_LISTEN may expect MULTI_LISTEN_BIND to need the same treatment. Adding export MULTI_LISTEN_BIND="${MULTI_LISTEN_BIND:-127.0.0.1}" alongside it would make the intent explicit and match the pattern established by MULTI_LISTEN.


Remaining Open Findings

# Finding File Priority
S2 Fixed test seeds in RELAY_KEYS.md with no production guard in keypair_from_seed_env bin/rayls-relay/src/main.rs Medium — pre-production
C1 relay_reservations HashMap grows without bound; stale entries retried forever after config churn runtime.rs:83–100 Medium — pre-production
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Low — convert to GH issue before merge
script-C3 add-relay-node.sh peer-id regex 12D3KooW[A-Za-z0-9]* matches partial log writes add-relay-node.sh:117 Low

S2 and C1 are the only items worth addressing before a production promotion. The four one-liner fixes (S1, C2, C3, C4) are applied locally in commit 3622b21 and ready to apply manually to the branch.


Positive Highlights (new commit)

  • Defaulting MULTI_LISTEN_BIND to 127.0.0.1 is the right security call: it matches the direct dnsaddr records already advertising 127.0.0.1, so the topology intent (co-located direct, cross-host relay) is now enforced at the network layer rather than relying on operator discipline.
  • The 0.0.0.0 escape hatch (MULTI_LISTEN_BIND=0.0.0.0) preserves backward compatibility for any existing setups that relied on all-interface binding.
  • Both the inline script comment and the onboarding doc are updated consistently.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 3m 23s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and check new commits since last review
  • Read CLAUDE.md for repo-specific instructions (none present)
  • Apply pending one-liner fixes (S1, C2, C3, C4) — push blocked; diffs below
  • Review new commit (250f483)
  • Post comprehensive review

Fixes Applied Locally (Push Blocked — Apply Manually)

Four changes were staged and committed locally as 16c7f2c but cannot be pushed due to repository permissions. All are 1–6 lines:

S1 — bin/rayls-relay/src/main.rs: startup warning that rate limiting is disabled

-use tracing::info;
+use tracing::{info, warn};

 let relay_cfg = relay_config()?;
+warn!(
+    "rate limiting disabled — this relay is configured for testnet use only; \
+     do not deploy on a public network without re-enabling reservation/circuit limiters"
+);
 let mut swarm = libp2p::SwarmBuilder::with_existing_identity(key)

C2 — runtime.rs:170: suppress false-positive during pending-reservation window

-    && !self.relay_reservations.is_empty()
+    && self.relay_reservations.values().any(Option::is_some)

C3 — runtime.rs:93: downgrade periodic retry log from info! to debug!

-    info!(target: "network", ?addr, "re-attempting relay reservation");
+    debug!(target: "network", ?addr, "re-attempting relay reservation");

C4 — command.rs:445: fix log target for DNS relay-resolution failures

-    warn!(target: "network-kad", %name, ?e, "failed to resolve /dnsaddr for relay discovery");
+    warn!(target: "network", %name, ?e, "failed to resolve /dnsaddr for relay discovery");

New Commit: 250f483feat(test-network): cross-host join knobs + --export-join-bundle

This commit exposes three new env-var knobs so an operator can start the testnet on one host and join a node from another, without changing single-host defaults. It also adds --export-join-bundle to bundle the three genesis files a joining node needs.

Correctness — Sound

The RELAY_PUBLIC_HOST rewrite (${RELAY_A_ADDR[$i]/\/ip4\/${RELAY_HOST}\//\/ip4\/${RELAY_PUBLIC_HOST}\/}) correctly patches only the relay IP in the public-view dnsaddr records. The relay server already binds 0.0.0.0 (documented in main.rs line 15), so the reachability is correct when RELAY_PUBLIC_HOST is set to a LAN/public IP. When RELAY_PUBLIC_HOST == RELAY_HOST (the default), the substitution is a no-op and single-host behavior is unchanged.

The DNSMASQ_BIND default (127.0.0.1) is the right conservative choice: a fresh --relay-dns testnet stays private unless explicitly opened. The DNSMASQ_HOST knob in add-relay-node.sh mirrors the same philosophy on the joining side.

export_join_bundle correctly uses -C local-validators on both the pack and extract sides, so the tar paths are relative to the tree root and the on-host advice is self-contained.

[N1] export_join_bundle echo of the extract command uses basename "$out" — may mislead with absolute paths

local-testnet.sh:378:

echo "  tar -xzf $(basename "$out") -C etc/test-network/local-validators"

If the user passes an absolute path (--export-join-bundle /tmp/bundle.tgz), the echo prints tar -xzf bundle.tgz — which will fail on the joining host unless the user happens to be in the same directory. Using "$out" directly (not basename) would preserve the path the user chose. Minor UX nit, doesn't affect correctness.

Fix this →

[N2] DNSMASQ_BIND and RELAY_PUBLIC_HOST are not exported from bounce-node.sh

bounce-node.sh:52 already has:

export MULTI_LISTEN="${MULTI_LISTEN:-0}"

but the two new cross-host knobs (DNSMASQ_BIND, RELAY_PUBLIC_HOST) are not similarly exported. For a cross-host chaos test where the operator sets these env vars, they propagate implicitly (shell env inheritance), but the explicit export makes the passing intent clear and mirrors the established pattern for MULTI_LISTEN. Low priority since env vars inherited from the shell always propagate to child processes.

[N3] --export-join-bundle cannot be combined with --start in a single invocation

The flag triggers an early exit (exit $?) at line 725 before the --start block is reached. The two-step example in RELAY-NODE-ONBOARDING.md (step 1 / step 1b) correctly shows them as separate invocations, so this is correctly documented. However a comment in the --export-join-bundle arg-parsing block noting "standalone only; cannot be combined with --start" would prevent confusion if someone tries to collapse the two steps.


Remaining Open Findings

# Finding File Priority
S2 Fixed test seeds in RELAY_KEYS.md with no production guard in keypair_from_seed_env bin/rayls-relay/src/main.rs Medium — pre-production
C1 relay_reservations HashMap grows without bound; stale entries retried forever after config churn runtime.rs:83–100 Medium — pre-production
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Low — convert to GH issue before merge
script-C3 add-relay-node.sh peer-id regex 12D3KooW[A-Za-z0-9]* matches partial log writes add-relay-node.sh:117 Low

S2 and C1 are the only items worth addressing before a production promotion. The four one-liner fixes (S1, C2, C3, C4) are prepared in local commit 16c7f2c and ready to apply manually.


Positive Highlights (new commit)

  • RELAY_PUBLIC_HOST rewrite is done by in-process bash string substitution rather than post-hoc sed on a file — no race with a concurrently-running dnsmasq.
  • Single-host defaults are unchanged: RELAY_PUBLIC_HOST="${RELAY_PUBLIC_HOST:-$RELAY_HOST}" means nothing changes unless explicitly overridden.
  • export_join_bundle correctly uses a missing accumulator rather than short-circuiting on the first missing file, so the error output lists all missing files in a single run.
  • The README one-shot sequence (step 1 → 1b → 2 → 3 → 4) correctly orders the cross-host steps: start first, bundle after genesis exists, then join.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 2m 12s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and check new commits since last review
  • Review new commit (9c72d40)
  • Apply pending one-liner fixes (S1, C2, C3, C4)
  • Commit and push fixes — push blocked by repo permissions; diffs below
  • Post comprehensive review

New Commit: 9c72d40docs(test-network): pass RPC_URL when staking a joined node

Correct and necessary. stake-relay-node.sh defaults RPC_URL to http://localhost:8545, which is a base committee member's port. On a machine running only the joined node, that port is absent and the script fails with a connection error before the readiness gate. The fix documents RPC_URL=http://localhost:8440 in the one-shot sequence (8545 - (INSTANCE-1) for INSTANCE=6) so the command works on either the committee host or the joiner's host. The formula derivation in the comment is accurate. No issues.


Fixes Applied Locally (Push Blocked — Apply Manually)

Four changes were committed locally as 0ee9db4 but cannot be pushed due to repository permissions. All are 1–6 lines:

S1 — bin/rayls-relay/src/main.rs: startup warning that rate limiting is disabled

-use tracing::info;
+use tracing::{info, warn};

 let relay_cfg = relay_config()?;
+warn!(
+    "rate limiting disabled — this relay is configured for testnet use only; \
+     do not deploy on a public network without re-enabling reservation/circuit limiters"
+);
 let mut swarm = libp2p::SwarmBuilder::with_existing_identity(key)

Rate limiters are zeroed unconditionally in relay_config() (lines 159–160). Any log filter passing warn and above (the default) will always surface this at startup, making accidental mis-deployment visible immediately.


C2 — runtime.rs:170: suppress false-positive during pending-reservation window

-    && !self.relay_reservations.is_empty()
+    && self.relay_reservations.values().any(Option::is_some)

relay_reservations keys are inserted before the circuit-relay RESERVE handshake completes — so during the startup window the map is non-empty but all values are None. Outbound QUIC dials completing in this window incorrectly triggered the "direct connection to a non-relay peer on a relayed node" warning. The fix ties the warning to at least one active (fully established) reservation.


C3 — runtime.rs:93: downgrade periodic retry log from info! to debug!

-    info!(target: "network", ?addr, "re-attempting relay reservation");
+    debug!(target: "network", ?addr, "re-attempting relay reservation");

handle_listener_closed already emits warn! at the moment of loss — that is the actionable signal. The retry fires every 15 s per unestablished reservation; during a sustained relay outage this generates info!-level noise indefinitely. warn! on loss + debug! on each retry is the right split.


C4 — command.rs:445: fix log target for DNS relay-resolution failures

-    warn!(target: "network-kad", %name, ?e, "failed to resolve /dnsaddr for relay discovery");
+    warn!(target: "network", %name, ?e, "failed to resolve /dnsaddr for relay discovery");

This is relay/DNS discovery code, not kademlia. The wrong target misattributes relay DNS failures to kademlia when operators filter by subsystem (RUST_LOG=network_kad=warn).


Remaining Open Findings

# Finding File Priority
S2 Fixed test seeds in RELAY_KEYS.md with no production guard in keypair_from_seed_env bin/rayls-relay/src/main.rs Medium — pre-production
C1 relay_reservations HashMap grows without bound; stale entries retried forever after config churn runtime.rs:83–100 Medium — pre-production
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Low — convert to GH issue before merge
script-C3 add-relay-node.sh peer-id regex 12D3KooW[A-Za-z0-9]* matches partial log writes add-relay-node.sh:117 Low

S2 and C1 are the only items worth addressing before a production promotion. The four one-liner fixes (S1, C2, C3, C4) are staged in local commit 0ee9db4 and ready to apply manually to the branch.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 2m 52s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and check new commits since last review
  • Read CLAUDE.md for repo-specific instructions (none present)
  • Review new commits (93bf4db, 23c70a4)
  • Apply pending one-liner fixes (S1, C2, C3, C4) — push blocked by repo permissions; diffs below
  • Post comprehensive review

New Commits Since Last Review

93bf4dbfix(bounce-node): forward DNSMASQ_HOST when respawning an added node

Correct and necessary. When bounce-node.sh restarts a node in ADDED=1 mode, it previously forwarded DNSMASQ_PORT but not DNSMASQ_HOST to add-relay-node.sh. In a cross-host bounce the respawned node would default to 127.0.0.1 as its DNS resolver — absent on the joining machine — causing /dnsaddr resolution failure for the committee and a silent topology break. The fix is clean: DNSMASQ_HOST="${DNSMASQ_HOST:-127.0.0.1}" with explicit forwarding mirrors the pattern already established for DNSMASQ_PORT. Single-host default is unchanged.

[N1] DNSMASQ_HOST is set-but-not-exported — pattern is inconsistent with MULTI_LISTEN

bounce-node.sh:52 has an explicit export MULTI_LISTEN=… but DNSMASQ_HOST and DNSMASQ_PORT are set as plain shell variables. In practice this is harmless — bash automatically propagates environment variables to child processes without an explicit export when they were already in the process environment. However the asymmetry could confuse a reader who sees export MULTI_LISTEN and expects the same treatment for the DNS vars. Worth adding export DNSMASQ_HOST="${DNSMASQ_HOST:-127.0.0.1}" alongside the existing export MULTI_LISTEN line for consistency.


23c70a4feat(add-relay-node): overridable RELAY_HOST for cross-host reachability

Correct and necessary for multi-machine topologies. When a node joined from a different machine, its relay circuit address was hardcoded to 127.0.0.1, making it unreachable from the committee host's side (consensus still worked via the node's own outbound dials, but inbound committee → node circuits couldn't establish). Making RELAY_HOST overridable with a default of 127.0.0.1 preserves single-host behavior while enabling cross-host use.

The comment correctly warns that RELAY_HOST must be set at first add (it's baked into the node at keygen). The onboarding doc update is accurate and the one-shot sequence now shows both DNSMASQ_HOST and RELAY_HOST side by side.

[N2] RELAY_HOST is not forwarded by bounce-node.sh — this is correct but underdocumented

bounce-node.sh forwards DNSMASQ_HOST and DNSMASQ_PORT to add-relay-node.sh on restart, but not RELAY_HOST. This is correct: the relay address is baked at keygen, and add-relay-node.sh skips keygen on restart if identity files already exist, so RELAY_HOST has no effect on a bounce. However, an operator who sees DNSMASQ_HOST being forwarded may instinctively also set RELAY_HOST on bounce and wonder why it doesn't work. A comment at the start_node() line — "RELAY_HOST is not forwarded: it was baked at first-add keygen and is not read on restart" — would preempt the confusion.

[N3] RELAY_HOST vs RELAY_PUBLIC_HOST (from local-testnet.sh) solve the same problem for different node types — no cross-reference

RELAY_HOST (in add-relay-node.sh) and RELAY_PUBLIC_HOST (in local-testnet.sh) both make relay circuits cross-host reachable, but for different node types (added nodes vs. base validators). Operators deploying a mixed cross-host testnet need to set both. A one-line cross-reference in RELAY-NODE-ONBOARDING.md's knob table — "See also RELAY_PUBLIC_HOST in local-testnet.sh for base-validator relay addresses" — would surface this without duplicating docs.


Fixes Applied Locally (Push Blocked — Apply Manually)

Four changes were committed locally as 9ebd2ec but cannot be pushed due to repository permissions:

S1 — bin/rayls-relay/src/main.rs: startup warning that rate limiting is disabled

-use tracing::info;
+use tracing::{info, warn};

 let relay_cfg = relay_config()?;
+warn!(
+    "rate limiting disabled — this relay is configured for testnet use only; \
+     do not deploy on a public network without re-enabling reservation/circuit limiters"
+);

C2 — runtime.rs:169–170: suppress false-positive during pending-reservation window

-    && !self.relay_reservations.is_empty()
+    && self.relay_reservations.values().any(Option::is_some)

C3 — runtime.rs:93: downgrade periodic retry log from info! to debug!

-    info!(target: "network", ?addr, "re-attempting relay reservation");
+    debug!(target: "network", ?addr, "re-attempting relay reservation");

C4 — command.rs:445: fix log target for DNS relay-resolution failures

-    warn!(target: "network-kad", %name, ?e, "failed to resolve /dnsaddr for relay discovery");
+    warn!(target: "network", %name, ?e, "failed to resolve /dnsaddr for relay discovery");

Remaining Open Findings

# Finding File Priority
S2 Fixed test seeds in RELAY_KEYS.md with no production guard in keypair_from_seed_env bin/rayls-relay/src/main.rs Medium — pre-production
C1 relay_reservations HashMap grows without bound; stale entries retried forever after config churn runtime.rs:83–100 Medium — pre-production
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Low — convert to GH issue before merge
script-C3 add-relay-node.sh peer-id regex 12D3KooW[A-Za-z0-9]* matches partial log writes add-relay-node.sh:117 Low

S2 and C1 are the only items worth addressing before a production promotion. The four one-liner fixes (S1, C2, C3, C4) are in local commit 9ebd2ec and ready to apply manually.


Positive Highlights (new commits)

  • DNSMASQ_HOST forwarding in bounce-node.sh correctly mirrors the pattern established for DNSMASQ_PORT in the prior commit — consistent forwarding semantics across all DNS-resolver knobs.
  • The RELAY_HOST default of 127.0.0.1 is the right conservative choice: single-host topology is unchanged, and the cross-host path requires explicit opt-in with a self-documenting variable name.
  • Commit message for 23c70a4 correctly documents the keygen-baking constraint upfront in the body and in the code comment — future operators won't be confused by "why didn't setting RELAY_HOST on bounce do anything?"
  • The onboarding doc now shows a complete cross-host invocation with all required knobs side by side in the one-shot sequence.

procdump added a commit to procdump/axyl that referenced this pull request Jul 23, 2026
… + DNS log target)

C2: gate the 'direct connection to a non-relay peer on a relayed node' warning
on an *active* reservation (relay_reservations.values().any(Option::is_some))
rather than a merely-requested one (!is_empty()). Reservation keys are inserted
before the RESERVE handshake completes, so during the boot window the map is
non-empty while nothing is reserved yet -- direct dials completing then were
false-positiving. Ties the warning to a live reservation.

C4: log /dnsaddr relay-resolution failures under target 'network' instead of
'network-kad' -- this is relay/DNS discovery, not kademlia, and the wrong target
misattributes failures when operators filter by subsystem.
procdump added a commit to procdump/axyl that referenced this pull request Jul 23, 2026
Addresses PR raylsnetwork#33 finding S1. The relay zeroes libp2p's per-source rate limiters
(they trip on the local testnet where all validators hairpin from 127.0.0.1).
Emit a plain startup warning that rate limiting is off; the comment notes that
per-source limiters can't meaningfully protect a shared relay anyway (finite
circuit lifetimes would force-close live consensus links), so protection is
left to the network edge.
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 2m 26s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and check new commits since last review
  • Read CLAUDE.md for repo-specific instructions (none present)
  • Verify status of previously open findings (S1, C2, C3, C4)
  • Review new commits (ae0c351, 705d956)
  • Check remaining open findings (S2, C1, D1, script-C3)
  • Post comprehensive review

New Commits

ae0c351chore(rayls-relay): warn at startup that rate limiting is disabled

Addresses S1. Correct implementation.

warn!("relay rate limiting is disabled") is placed inside relay_config(), immediately after the two rate-limiter vecs are cleared — exactly collocated with the code it warns about. The commit message adds important nuance not present in the prior review suggestions: per-source limiters can't meaningfully protect a shared relay because the only slot-reclaiming mechanism (max_circuit_duration) would force-close live consensus links. This makes the design decision legible to future readers rather than just flagging it as a testnet compromise.

The warning fires exactly once at startup — relay_config() is called once before the swarm is built. Any log filter passing warn and above (the default) will always show it. The placement is actually better than the suggested location (before the event loop in main) because it's collocated with the code that triggers the condition.

One very minor nit: the message "relay rate limiting is disabled" is terse — it doesn't hint at what to do in production. The prior suggestion of including "do not deploy on a public network without re-enabling limiters" would add operability value. Not blocking.


705d956fix(network): address PR #33 review (relayed-node warning + DNS log target)

Addresses C2 and C4. Both fixes are correct.

C2 fix (runtime.rs:176–177): The condition is now self.relay_reservations.values().any(Option::is_some) — checking for at least one active (fully established) reservation rather than a merely-requested entry. The inline comment is thorough, explaining the None-on-insert invariant and the boot-window false-positive scenario. This is the fix as recommended.

C4 fix (command.rs:442): Log target correctly changed from "network-kad" to "network" for /dnsaddr relay resolution failures. One-liner, straightforward.


Finding Status After These Commits

# Finding File Status
S1 Relay startup: no warn! that rate limiting is disabled bin/rayls-relay/src/main.rs Fixed in ae0c351
C2 ConnectionEstablished warn fires during pending-reservation window runtime.rs:176 Fixed in 705d956
C4 DNS failure in resolve_relay_circuits logged to "network-kad" command.rs:442 Fixed in 705d956
C3 retry_relay_reservations logs info! every 15s during outages runtime.rs:93 Open — one-liner: info!debug!
S2 Fixed test seeds in RELAY_KEYS.md, no production guard in keypair_from_seed_env main.rs Open — pre-production
C1 relay_reservations HashMap grows without bound; stale entries retried forever runtime.rs:83–100 Open — pre-production
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Open — convert to GH issue before merge
script-C3 add-relay-node.sh peer-id regex 12D3KooW[A-Za-z0-9]* matches partial log writes add-relay-node.sh:129 Open — low priority

Remaining One-Liner Fix (C3)

C3 is the only easy fix not yet applied. runtime.rs:93:

-    info!(target: "network", ?addr, "re-attempting relay reservation");
+    debug!(target: "network", ?addr, "re-attempting relay reservation");

handle_listener_closed already emits warn! at the moment a relay reservation is lost — that is the operator-actionable signal. The retry fires every 15 s per unestablished reservation; during a sustained relay outage this generates an info!-level log line every 15 s indefinitely. warn! on loss + debug! on each retry is the right split.

Fix this →

Summary

Three of the seven previously-open findings are now fixed. The three highest-priority one-liner fixes (S1, C2, C4) are merged. C3 is the only remaining one-liner and should be trivial to fold in. S2 and C1 are medium-priority pre-production items. D1 (the TODO file) should be converted to a GitHub issue before merge.

procdump added a commit to procdump/axyl that referenced this pull request Jul 23, 2026
Remove the in-repo TODO file from git tracking (kept locally); branch follow-ups
are tracked as GitHub issues instead. Drop the now-dangling references to it from
the relay-client warning comment (runtime.rs) and the relay-network README /
DESIGN-DECISIONS docs.
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @procdump's task in 2m 13s —— View job


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Check current state of open findings
  • Apply remaining one-liner fixes (C3, script-C3) — push blocked by repo permissions; diffs below
  • Verify recently fixed items (S1, C2, C4, D1)
  • Post comprehensive update

Finding Status — Full Picture

# Finding File Status
S1 Relay startup: no warn! that rate limiting is disabled bin/rayls-relay/src/main.rs Fixed in ae0c351
C2 ConnectionEstablished warn fires during pending-reservation window runtime.rs:176 Fixed in 705d956
C4 DNS failure in resolve_relay_circuits logged to "network-kad" command.rs:442 Fixed in 705d956
D1 TODO-CRv2-NETWORKING.md committed to repo root /TODO-CRv2-NETWORKING.md Fixed in 04b2ae6
C3 retry_relay_reservations logs info! every 15s during outages runtime.rs:93 Prepared locally — apply diff below
script-C3 add-relay-node.sh peer-id regex matches partial log writes add-relay-node.sh:129 Prepared locally — apply diff below
S2 Fixed test seeds in RELAY_KEYS.md, no production guard in keypair_from_seed_env bin/rayls-relay/src/main.rs Open — pre-production
C1 relay_reservations HashMap grows without bound; stale entries retried indefinitely runtime.rs:83–100 Open — pre-production

Fixes Prepared (commit 1424b20 — push blocked; apply manually)

C3 — runtime.rs:93: downgrade periodic retry log from info! to debug!

-                    info!(target: "network", ?addr, "re-attempting relay reservation");
+                    debug!(target: "network", ?addr, "re-attempting relay reservation");

handle_listener_closed already emits warn! at the moment a relay reservation is lost — that is the operator-actionable signal. retry_relay_reservations fires every 15 s per unestablished reservation; during a sustained relay outage this generates an info!-level line every 15 s indefinitely. warn! on loss + debug! on each retry is the right split. debug! is already imported in the file.


script-C3 — add-relay-node.sh:129: tighten peer-id regex to reject partial log writes

-        RELAY_PEER=$(grep -ao '12D3KooW[A-Za-z0-9]*' "$RELAY_LOG" 2>/dev/null | head -1 || true)
+        RELAY_PEER=$(grep -ao '12D3KooW[A-Za-z0-9]\{40,\}' "$RELAY_LOG" 2>/dev/null | head -1 || true)

* matches zero characters, so if the relay log is read mid-write the 12D3KooW prefix alone satisfies the pattern, producing a truncated peer ID. The resulting malformed circuit address (/ip4/127.0.0.1/.../p2p/12D3KooW) causes listen_on to fail with a confusing runtime error rather than a clear startup message. libp2p peer IDs are ≥46 base58 characters after the 12D3KooW prefix, so \{40,\} safely rejects any prefix-only match.


Still Open (pre-production, not blocking PoC)

S2 — Fixed test seeds with no production guard

keypair_from_seed_env (bin/rayls-relay/src/main.rs:197–201) derives a fixed ed25519 key from RELAY_SEED_HEX. The seeds are documented in RELAY_KEYS.md. A production deployment that copies these seeds would have a fully predictable relay identity. Consider an eyre::bail! if a known test seed is detected, or a CI lint that prevents seeds from RELAY_KEYS.md appearing in production config files.

C1 — relay_reservations HashMap grows without bound

Entries are added in StartListening and reset to None on listener close, but never removed. If relay configuration changes between restarts, stale entries remain and trigger listen_on calls to dead addresses every 15 s indefinitely. A retry counter per entry (HashMap<Multiaddr, (Option<ListenerId>, u32)>) that logs a warning after N consecutive failed re-attempts would let operators diagnose permanently dead relays.


Summary

Four of the eight tracked findings are now fixed in the branch (ae0c351, 705d956, 04b2ae6). The two remaining one-liners (C3, script-C3) are prepared in local commit 1424b20 and ready to apply manually. S2 and C1 are the only items worth addressing before production promotion — both are non-blocking for the PoC.

procdump added a commit to procdump/axyl that referenced this pull request Jul 23, 2026
…snetwork#33 script-C3)

The peer-id read grepped 12D3KooW[A-Za-z0-9]* and broke the poll loop on any
non-empty match, so a partial log write (grep racing the relay's startup flush)
could bake a truncated -- thus wrong -- relay address. Gate on the exact peer-id
length (52 chars) inside the loop so a short match is rejected and polling
continues until the full id lands. Length-only test, portable across GNU/BSD
grep and bash 3.2 (macOS).
procdump added 25 commits July 31, 2026 10:47
add-relay-node.sh stays node-only (add/restart, restart-safe) but now derives a
deterministic throwaway operator identity from the index (OPERATOR_KEY = 0x(1000+i),
ADDRESS via `cast`) and bakes it into the node's proof-of-possession at keygen, so
the node can be staked later. Without `cast` it falls back to the zero address
(pure observer). On startup it prints the exact one-liner to promote it. No stake
happens here -- the node just follows the committee as an observer.

stake-relay-node.sh (new) is the one-time on-chain step: it derives the SAME
operator identity from the index (so it matches the baked PoP), defaults the
registry-owner admin key to anvil #0, and runs fund -> allowlistValidator ->
stake -> activate. The node then promotes to a committee validator at the next
epoch boundary. Everything is env-overridable for non-default networks.

Splitting staking out keeps add-relay-node.sh idempotent (safe to re-run on
restart) while staking runs exactly once. Allowlisting is onlyOwner, so joining
the committee remains governance-gated; an unstaked node is always an observer.
Staking pulls the RLS ERC-20 (0x..E17eA) via transferFrom, not the native gas
token, and the required stake is 5e24 (genesis default), so the previous
fund+allowlist+stake flow reverted (InsufficientAllowance/Balance). Correct the
flow to 6 steps: fund native gas -> mint 5e24 RLS to the operator (admin holds
MINTER_ROLE at genesis) -> allowlist (tolerant of re-runs) -> operator approves
the registry -> stake -> activate.

Add a readiness gate: right after `local-testnet.sh --start` the genesis system
contracts aren't live yet (RLS proxy has no implementation -> mint silently
no-ops; registry has no owner -> allowlist reverts), which produced confusing
mid-flow failures. Poll (up to ~2min) until the RLS ERC-1967 impl slot is
non-zero AND the ConsensusRegistry has an owner before any on-chain step, and
abort early with a clear message otherwise.
Add RELAY-NODE-ONBOARDING.md documenting the relay-fronted testnet and the
dynamic validator onboarding flow end-to-end: the MULTI_LISTEN + --relay-dns
start, add-relay-node (observer via the public DNS view), and stake-relay-node
(readiness gate -> mint -> allowlist -> approve -> stake -> activate). Includes
the per-node port map, mesh observability via the consensus metrics endpoint,
the gotchas we hit (genesis skip-if-exists, dev-funds = owner+minter, startup
init race), and a pointer to the open dynamic-committee questions.
…atch-up batch

A batch-fetch failure during CvvInactive catch-up (or Observer follow) that
survives the in-loop retries previously panicked the "subscriber catch up and
rejoin consensus" critical task, aborting the whole rayls-network process. This
reproduced when repeatedly killing + restarting a validator: on restart its
worker mesh isn't fully re-established, so no currently-connected peer serves a
batch a committed output references, and after the retries the node crashed.

Now such a batch-fetch error (MissingFetchedBatch / ClientRequestsFailed) is
handled gracefully:
- CvvInactive catch-up: demote to Observer (request_mode_transition) and keep
  following; it re-attempts catch-up at a later epoch boundary once the mesh is
  back. CvvInactive->Observer is an allowed transition.
- Observer follow: exit the follow attempt without panicking; spawn_subscriber
  re-arms it next epoch.
Other (non-batch-fetch) errors still panic.

This is NOT a garbage-collection issue: GC prunes the certificate DAG (rounds),
not the worker batch store, so the batch still exists on some holder -- the miss
is a connectivity/timing gap (holder not among connected peers within the retry
window). Demotion converts a fatal panic into a survivable degraded state that
self-heals once connectivity returns.

REVISIT: the proper fix is a connectivity-aware fetch -- keep retrying while the
worker is connected to fewer than the committee's workers, instead of declaring a
batch missing against a half-connected mesh (flagged with XXX in the code).
…art tooling

Make stopping/restarting a single node reliable and env-correct, so restarting a
validator (e.g. in a chaos loop) doesn't boot with a bare env and fail /dnsaddr
resolution -> can't reach quorum -> can't rejoin.

local-testnet.sh:
- Extract build_relay_env(seq) (relay reservations + RAYLS_DNS_SERVER + MULTI_LISTEN
  direct listeners), shared by the --start loop and the single-validator path so
  they can't drift. --start-validator now rebuilds the SAME env as --start (pass
  the same mode flags, e.g. MULTI_LISTEN=1 ... --start-validator N --relay-dns).
- start_relay_pair(i)/stop_relay_pair(i): a validator's relays are now managed by
  --start-validator/--stop-validator (revive if down / scrap on stop). Relay seeds
  are deterministic, so a restarted relay keeps its peer id and the dnsmasq records
  stay valid. start_relays() loops start_relay_pair.
- stop_validator: send SIGTERM and wait INDEFINITELY -- no kill -9. A hung graceful
  shutdown now blocks (and is caught) instead of being masked. Relays are stateless,
  so stop_relay_pair does SIGTERM then kill -9 if they linger.

stop-relay-node.sh (new): inverse of add-relay-node.sh -- stops the added node
(graceful, wait-forever, no kill -9) and its relay (SIGTERM then kill -9).

fork_test_configs/bounce-node.sh: chaos loop that waits for is_caught_up then
stop->restart in a loop. Two modes: base validators via local-testnet.sh
--stop/--start-validator; dynamically-added nodes (ADDED=1) via stop-relay-node.sh
+ add-relay-node.sh, polling the added node's RPC port.

RELAY-NODE-ONBOARDING.md: document stopping (one-shot block + a Stopping/restarting
/chaos-testing section covering the base-vs-added toolchains, the mode-flag gotcha,
the no-kill-9 shutdown semantics, and bounce-node.sh).
The per-tx `nonce_too_high_detail` loop logged one warn line per dropped tx —
thousands during a single-sender nonce burst, flooding the logs. The
`nonce_range_for_sender` summary already carries the actionable per-sender gap
info at warn; keep the per-tx detail at debug for when you're actually chasing a
gap (RUST_LOG=batch_tracker=debug). No allocation on the log path.
… bounce-node

Two bounce-node.sh changes:
- Default ADDED-mode DNSMASQ_PORT to 5353 (private/direct) to match
  add-relay-node.sh. It previously defaulted to 5354 (public/relay), so adding a
  node with the default then bouncing it silently flipped its committee-resolution
  view (direct -> relay) across the restart. Pass DNSMASQ_PORT=5354 for the relay
  view on both.
- Add a DOWN_SECS knob (default 0): keep the node down that long before restarting,
  so it can fall behind across epoch boundaries to exercise the catch-up path.
In the chaos-test section of RELAY-NODE-ONBOARDING.md, spell out the DNSMASQ_PORT
transport semantics inline on the bounce commands: added nodes honor it
(5353 = direct, 5354 = relayed) and the bounce passes it through on every respawn;
base validators always resolve via the private/direct view (5353) since
build_relay_env pins them, so DNSMASQ_PORT is not honored for a base bounce.
…o loopback

MULTI_LISTEN direct listeners now bind MULTI_LISTEN_BIND (default 127.0.0.1)
instead of a hardcoded 0.0.0.0. Loopback-only matches the direct dnsaddr
records (which advertise 127.0.0.1), so co-located nodes still mesh directly
while the listener is never exposed on an external interface -- any cross-host
reach must go through a relay. Set MULTI_LISTEN_BIND=0.0.0.0 to restore
all-interface binding. Relays are unaffected (still 0.0.0.0).
Starting the network on one host and adding a node from another needs the
loopback defaults overridden (all default to 127.0.0.1, single-host unchanged):

- DNSMASQ_BIND (local-testnet.sh): resolver --listen-address; 0.0.0.0 serves the
  /dnsaddr records to other hosts.
- RELAY_PUBLIC_HOST (local-testnet.sh): IP advertised for the relays in the
  public :5354 dnsaddr records, so a remote joiner resolves a reachable relay
  instead of 127.0.0.1. Rewrites only the public-view records; the relay server
  already listens on all interfaces.
- DNSMASQ_HOST (add-relay-node.sh): resolver address the joining node points
  RAYLS_DNS_SERVER at.

Also add --export-join-bundle: tars the three files a follower needs
(genesis.yaml + committee.yaml + parameters.yaml) with paths relative to
local-validators/, so the joiner extracts them where add-relay-node.sh expects.
Documented in the one-shot sequence in RELAY-NODE-ONBOARDING.md.
stake-relay-node.sh defaults RPC_URL to :8545 (a base committee member), which
is absent on a machine running only the joined node. Document passing node-6's
own RPC (8440 = 8545-(INSTANCE-1) for N=6) in the one-shot sequence so staking
works whether run on the committee host or the joiner's host.
ADDED-mode start_node passed DNSMASQ_PORT but not DNSMASQ_HOST, so a cross-host
bounce respawned the node with the default 127.0.0.1 resolver -- absent on the
joiner's machine -- and it couldn't re-resolve the committee /dnsaddr. Forward
DNSMASQ_HOST (default 127.0.0.1, single-host unchanged) like DNSMASQ_PORT.
RELAY_HOST was hardcoded to 127.0.0.1, so a node added from another machine
advertised its relay circuit at loopback -- unreachable from the committee host,
so committee members could not dial it back (consensus still worked via the
node's own outbound dials, but the reverse direction couldn't establish). Make
it RELAY_HOST=${RELAY_HOST:-127.0.0.1}; set it to the joining host's IP so the
node advertises a reachable relay. Must be set at first add (baked at keygen).
Documented in the one-shot sequence alongside DNSMASQ_HOST.
… + DNS log target)

C2: gate the 'direct connection to a non-relay peer on a relayed node' warning
on an *active* reservation (relay_reservations.values().any(Option::is_some))
rather than a merely-requested one (!is_empty()). Reservation keys are inserted
before the RESERVE handshake completes, so during the boot window the map is
non-empty while nothing is reserved yet -- direct dials completing then were
false-positiving. Ties the warning to a live reservation.

C4: log /dnsaddr relay-resolution failures under target 'network' instead of
'network-kad' -- this is relay/DNS discovery, not kademlia, and the wrong target
misattributes failures when operators filter by subsystem.
Addresses PR raylsnetwork#33 finding S1. The relay zeroes libp2p's per-source rate limiters
(they trip on the local testnet where all validators hairpin from 127.0.0.1).
Emit a plain startup warning that rate limiting is off; the comment notes that
per-source limiters can't meaningfully protect a shared relay anyway (finite
circuit lifetimes would force-close live consensus links), so protection is
left to the network edge.
Remove the in-repo TODO file from git tracking (kept locally); branch follow-ups
are tracked as GitHub issues instead. Drop the now-dangling references to it from
the relay-client warning comment (runtime.rs) and the relay-network README /
DESIGN-DECISIONS docs.
…snetwork#33 script-C3)

The peer-id read grepped 12D3KooW[A-Za-z0-9]* and broke the poll loop on any
non-empty match, so a partial log write (grep racing the relay's startup flush)
could bake a truncated -- thus wrong -- relay address. Gate on the exact peer-id
length (52 chars) inside the loop so a short match is rejected and polling
continues until the full id lands. Length-only test, portable across GNU/BSD
grep and bash 3.2 (macOS).
reth defaults --http.addr/--ws.addr to 127.0.0.1, so the local testnet's RPC
wasn't reachable off-host. Bind 0.0.0.0 on all local-testnet.sh node launches
(validators + observers) and the add-relay-node.sh launch, matching the existing
start-local-validator/observer.sh convention -- lets a tx generator (or any
client) drive the nodes from another machine instead of competing for CPU on the
node host. Test-network only; deployment scripts left on loopback.
--relay requires a fixed relay peer id per validator; the array only had 4, so
NUM_VALIDATORS>4 failed with 'RELAY_PEER_IDS[N] is not filled in'. Extend to 32
(seed = byte (index+1) repeated 32x, peer id derived via rayls-relay; entries
1-4 reproduce the existing ids exactly). Supports up to 32 validators in relay
mode without hand-filling ids.
Add BENCHMARKS.md: relay vs no-relay finality/throughput at 4+1 and 6+1 under a
sustained 10k-tps load (tps-checker on a separate host driving 0.0.0.0-bound
RPCs). Documents the setup, the generator command, the result tables, and the
single-host lower-bound caveat. Finding: relays are within noise of direct;
committee size (4->6) is the dominant cost.
Second 6+1 relay run came in slightly worse than the first (p99 8.1s vs 6.8s,
669 vs 708 blocks). Add it as a column; reword the finding in plain terms: one
relay run beat direct and one was slower (no-relay p99 7.1s sits between the
relay runs' 6.8s and 8.1s), so it's run-to-run variance, not a consistent relay
penalty. Note 6+1 needs 3+ runs per config for a firm conclusion.
Bounce one relay by index without touching its node, using the same fixed-identity
scheme as local-testnet.sh/add-relay-node.sh (primary: port 50000+(N-1), seed byte
N; --backup: port 51000+(N-1), seed byte 0xb0+(N-1)). Same peer id across a bounce,
so the fronted validator re-reserves on its own. Honors BUILD_CONFIG. Referenced
from RELAY-NODE-ONBOARDING.md's chaos-testing section for relay failure/recovery.
…atal-ban

A circuit relay speaks no gossipsub, so the GossipsubNotSupported one-shot Fatal
penalty banned the relay and severed every circuit routed through it, stranding
relayed committee members. Record such peers as relay infrastructure
(penalty-exempt, dropped from kad) via mark_relay_peer instead; a real committee
validator failing negotiation is surfaced loudly rather than exempted.
…seeding

When a relayed node booted isolated, discovery seeding dialed known peers by their
stored address as-is -- including unresolved /dnsaddr committee members. That opens
a non-circuit connection that sanitize_ip_addr denies ("no valid unbanned IP"), and
the failed dial's register_disconnected races with and tears down the circuit the
DialBls path had just established, so a restarted relayed node could never hold its
committee connections and got stuck (CvvInactive) until the validators restarted.

Seed only concrete, already-dialable addresses and drop /dnsaddr ones; committee
members are re-dialed via redial_missing_committee -> DialBls, which resolves them
to concrete /p2p-circuit addresses first.
Emit the penalty that tipped a peer into a ban (warn, fires only on ban), and the
transport path + remaining connection count + ConnectionError cause on connection
close (info), so ban and teardown reasons are explicit in the file log instead of
having to be inferred from surrounding events.
@procdump
procdump force-pushed the ba-circuit-relay-v2-poc branch from b1a1fbd to 4158a38 Compare July 31, 2026 07:47
@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

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


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Gather context and check new commits
  • Read CLAUDE.md for repo-specific instructions (none present)
  • Apply C3 one-liner fix (info!debug! in runtime.rs:93) — committed as 72938bc; push blocked by repo permissions (see diff below)
  • Verify fix
  • Post comprehensive review

C3 Fix — Apply Manually (Push Blocked)

No new commits have been pushed since the last review pass. This pass applies the one remaining tracked fix and confirms the overall status.

The fix was committed locally as 72938bc but push continues to be blocked — github-actions[bot] does not have write access to this branch.

crates/consensus/network/src/consensus/runtime.rs line 93

-                    info!(target: "network", ?addr, "re-attempting relay reservation");
+                    debug!(target: "network", ?addr, "re-attempting relay reservation");

handle_listener_closed already emits warn! at the moment a relay reservation is lost — that is the operator-actionable signal. retry_relay_reservations fires every 15 s per unestablished reservation; during a sustained relay outage this generates an info!-level line every 15 s indefinitely. warn! on loss + debug! on each retry is the right split. debug! is already imported in the file (use tracing::{debug, error, info, ...} at line 12).

Fix this →


Full Finding Status

# Finding File Status
S1 Relay startup: no warn! that rate limiting is disabled bin/rayls-relay/src/main.rs Fixed in ae0c351
C2 ConnectionEstablished warn fires during pending-reservation window runtime.rs:176 Fixed in 705d956
C4 DNS failure in resolve_relay_circuits logged to "network-kad" command.rs:442 Fixed in 705d956
D1 TODO-CRv2-NETWORKING.md committed to repo root repo root Fixed in 04b2ae6
script-C3 add-relay-node.sh peer-id regex matches partial log writes add-relay-node.sh:129 Fixed in 583475f
C3 retry_relay_reservations logs info! every 15s during outages runtime.rs:93 Prepared locally (72938bc) — apply diff above
S2 Fixed test seeds in RELAY_KEYS.md, no production guard in keypair_from_seed_env bin/rayls-relay/src/main.rs Open — pre-production
C1 relay_reservations HashMap grows without bound; stale entries retried forever runtime.rs:83–100 Open — pre-production

Five of the eight tracked findings are fixed in the branch. C3 is the sole remaining one-liner — the diff is above, ready to apply. S2 and C1 are medium-priority pre-production items; neither blocks this PoC.

Note on push access: Every automated review pass has attempted to push C3 directly and been rejected with 403 Permission denied. To allow future automated fixes, the github-actions[bot] account needs write access to this branch, or the one-liner above should be applied manually.

procdump added 2 commits July 31, 2026 13:05
Adds an observer (no relay in front, never in committee) that reaches the
committee directly or through their relays via /dnsaddr. Requires the operator
to supply the network files; restart-safe. Documented in RELAY-NODE-ONBOARDING.md.
The relay-less observer was left on keygen's node-info default (127.0.0.1),
so its outbound QUIC could not reach external relays and no dial completed.
Default the p2p listener to 0.0.0.0 (all interfaces), matching how base
observers run on main; overridable via LISTEN_HOST or the
PRIMARY/WORKER_LISTENER_MULTIADDR env vars.
@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

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


PR Review: feat: route consensus p2p through circuit-relay-v2

  • Check new commits since last review (34de63c, ec84f25)
  • Apply C3 fix (info!debug! in runtime.rs:93) — committed as 01f2ad8; push blocked by repo permissions (see diff below)
  • Review new commits
  • Post comprehensive review

New Commits Since Last Review

34de63cfeat(test-network): add-observer.sh to inject a relay-less observer

Correct and well-documented. This adds a new script for attaching an observer that connects through committee validators' relays (or directly, for non-relay topologies) without standing up its own relay or joining the committee.

The key design decisions are sound:

  • --observer flag ensures the node is permanently excluded from quorum — safe to inject into a live chain.
  • No --relay at keygen: the observer makes no reservation and is never dialed back, so it genuinely needs no relay of its own.
  • RAYLS_DNS_SERVER is set conditionally on /dnsaddr/ detection in committee.yaml — the same pattern as add-relay-node.sh, correct.
  • Port scheme (49000 + NODE_NUM primary, 49100 + NODE_NUM worker) does not collide with validators (40000+i, 41000+i) or relays (50000+i, 51000+i).

One issue (fixed in the immediate follow-up commit): the initial version did not set PRIMARY_LISTENER_MULTIADDR / WORKER_LISTENER_MULTIADDR, leaving the p2p listener on keygen's default 127.0.0.1. This prevented outbound QUIC to external peers. Fixed by ec84f25.


ec84f25fix(test-network): default add-observer listener to 0.0.0.0

Correct and necessary. Adds LISTEN_HOST (default 0.0.0.0) and exports PRIMARY_LISTENER_MULTIADDR / WORKER_LISTENER_MULTIADDR before starting the node, overriding the keygen default of 127.0.0.1. The commit message explains the root cause precisely: a loopback-bound socket can't reach external peers, so no outbound QUIC dial completes.

[N1] Port derivation for the observer listener could collide at NODE_NUM=1

PRIMARY_LISTENER_MULTIADDR uses 49000 + NODE_NUM. For NODE_NUM=1, that is port 49001. The guard at line 77 (NODE_NUM > NUM_VALIDATORS) prevents collision with validators but not with the OS ephemeral range (typically 32768–60999 on Linux). Port 49001 is below the ephemeral range and unlikely to conflict with anything in the testnet. No issue in practice; noting for completeness.

[N2] echo "p2p listeners: primary ... worker ..." is printed before the node process starts

add-observer.sh:142 logs the listener multiaddrs before calling env ... "$BIN" node. If $BIN exits immediately (bad flag, missing file), the listeners were advertised in the log but never actually bound. This is a logging order issue shared with add-relay-node.sh — not a correctness problem, just worth noting.

No correctness issues in these two commits.


C3 Fix — Applied Locally, Push Blocked

crates/consensus/network/src/consensus/runtime.rs line 93 — committed as 01f2ad8:

-                    info!(target: "network", ?addr, "re-attempting relay reservation");
+                    debug!(target: "network", ?addr, "re-attempting relay reservation");

handle_listener_closed already emits warn! at the moment a relay reservation is lost — that is the operator-actionable signal. retry_relay_reservations fires every 15 s per unestablished reservation; during a sustained relay outage this generates an info!-level line every 15 s indefinitely. warn! on loss + debug! on each retry is the right split. debug! is already imported in the file.

Fix this →


Full Finding Status

# Finding File Status
S1 Relay startup: no warn! that rate limiting is disabled bin/rayls-relay/src/main.rs Fixed in ae0c351
C2 ConnectionEstablished warn fires during pending-reservation window runtime.rs:176 Fixed in 705d956
C4 DNS failure in resolve_relay_circuits logged to "network-kad" command.rs:442 Fixed in 705d956
D1 TODO-CRv2-NETWORKING.md committed to repo root repo root Fixed in 734bb22
script-C3 add-relay-node.sh peer-id regex matches partial log writes add-relay-node.sh:129 Fixed in 583475f
C3 retry_relay_reservations logs info! every 15s during outages runtime.rs:93 Prepared locally (01f2ad8) — apply diff above
S2 Fixed test seeds in RELAY_KEYS.md, no production guard in keypair_from_seed_env bin/rayls-relay/src/main.rs Open — pre-production
C1 relay_reservations HashMap grows without bound; stale entries retried forever runtime.rs:83–100 Open — pre-production

Five of the eight tracked findings are fixed in the branch. C3 is the sole remaining one-liner — the diff is above, ready to apply. S2 and C1 are medium-priority pre-production items; neither blocks this PoC.

Note on push access: Every automated review pass has attempted to push C3 directly and been rejected with 403 Permission denied. To unblock future automated fixes, grant the github-actions[bot] account write access to this branch, or apply the one-liner above manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants