Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions .github/workflows/go-harness.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
name: Go simnet harness

on:
push:
branches: ["main"]
paths:

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.

nit: both jobs share this paths filter including **/*.rs, **/Cargo.toml, and Cargo.lock. The charon-simnet job builds no Rust and only exercises charon in-process, so it re-runs on every Rust-only change that can't affect it. Only pluto-simnet legitimately needs the Rust paths. Splitting the filters per job would avoid redundant runs; a unified filter is a reasonable simplicity tradeoff if you'd rather keep it.

- "**/*.rs"
- "**/Cargo.toml"
- "Cargo.lock"
- "test-infra/harness/**"
- ".github/workflows/go-harness.yml"
pull_request:
types: [synchronize, opened, reopened, ready_for_review]
paths:
- "**/*.rs"
- "**/Cargo.toml"
- "Cargo.lock"
- "test-infra/harness/**"
- ".github/workflows/go-harness.yml"
workflow_dispatch:

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

permissions:
contents: read

jobs:
# Fast signal: all-charon simnet scenarios (in-process and via the beacon
# gateway). No Rust build required.
charon-simnet:
name: Charon simnet scenarios
runs-on: ubuntu-24.04
timeout-minutes: 15
steps:
- name: Checkout
uses: actions/checkout@v6

- name: Set up Go
uses: actions/setup-go@v5
with:
go-version-file: test-infra/harness/go.mod
cache-dependency-path: test-infra/harness/go.sum

- name: Run charon simnet tests
working-directory: test-infra/harness
run: go test -v -timeout=12m -run 'TestSimnetAttesterCharon' ./...

# Full suite with the pluto binary available. The pluto/mixed scenarios
# self-skip until `pluto run` lands, then activate automatically.
pluto-simnet:
name: Pluto simnet scenarios
runs-on: ubuntu-24.04
timeout-minutes: 30
steps:
- name: Checkout
uses: actions/checkout@v6

- name: Set up Go
uses: actions/setup-go@v5
with:
go-version-file: test-infra/harness/go.mod
cache-dependency-path: test-infra/harness/go.sum

- name: Cache cargo registry and target
uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2

- name: Update apt package list
run: sudo apt-get update

- name: Install protobuf
uses: awalsh128/cache-apt-pkgs-action@acb598e5ddbc6f68a970c5da0688d2f3a9f04d05 # v1.6.0
with:
packages: protobuf-compiler=3.21.12*
version: 3.21.12

- name: Install oas3-gen
run: cargo install oas3-gen@0.24.0

- name: Build pluto
run: cargo build -p pluto-cli

- name: Run simnet tests
working-directory: test-infra/harness
env:
PLUTO_BIN: ${{ github.workspace }}/target/debug/pluto
run: go test -v -timeout=20m ./...
6 changes: 6 additions & 0 deletions crates/cluster/src/distvalidator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,12 @@ pub struct DistValidatorV1x8orLater {
pub pub_shares: Vec<Vec<u8>>,

/// Deposit data defines the deposit data to activate a validator.
///
/// Charon serializes this field with `omitempty`, so a lock created
/// without deposit data (e.g. simnet fixtures) omits it entirely.
/// Default to an empty list to stay compatible, matching charon's
/// deserializer which tolerates the absent field.
#[serde(default)]
pub partial_deposit_data: Vec<DepositData>,

/// Builder registration is the pre-generated signed validator builder
Expand Down
5 changes: 5 additions & 0 deletions crates/p2p/src/p2p.rs
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,11 @@ impl<B: NetworkBehaviour> Node<B> {
self.swarm.add_external_address(addr);
}

/// Removes a previously added external address from the peer store.
pub fn remove_external_address(&mut self, addr: &Multiaddr) {
self.swarm.remove_external_address(addr);
}

/// Returns the global context.
pub fn p2p_context(&self) -> &P2PContext {
&self.p2p_context
Expand Down
19 changes: 18 additions & 1 deletion crates/relay-server/src/p2p.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,15 @@ pub async fn run_relay_p2p_node(
let mut external_addrs = external_tcp_multiaddrs(&config.p2p_config)?;
external_addrs.extend(external_udp_multiaddrs(&config.p2p_config)?);

// Advertise the manually configured external addresses to libp2p so they
// are folded into the circuit reservations we hand out. Without at least
// one reachable address, rust-libp2p relay clients reject the reservation
// with `NoAddressesInReservation`; auto-detected listen addresses are
// added below as they are confirmed.
for addr in &external_addrs {
node.add_external_address(addr.clone());

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.

Manually-configured external addresses bypass private-address filtering, and rust-libp2p has no go-libp2p reservation backstop.

This loop calls add_external_address for every entry in external_addrs (built from operator-supplied --p2p-external-ip/--p2p-external-hostname) with no filter_private_addrs/is-private gate. In rust-libp2p, add_external_addressExternalAddrConfirmed feeds libp2p-relay's AcceptReservationReq { addrs } verbatim; libp2p-relay does not filter non-public addresses out of circuit reservations. go-libp2p does: makeReservationMsg (circuitv2/relay/relay.go) unconditionally skips !manet.IsPublicAddr(addr).

Net effect: a misconfigured private/loopback external addr is advertised inside circuit reservations under Pluto, whereas charon/go-libp2p would silently drop it. The auto-detected AddrUpdate::Add path (line 138) is correctly gated by filter_private_addrs, so only the manual path is affected. Impact is low (it adds addresses rather than breaking connectivity, and only under misconfiguration), but consider mirroring go-libp2p's is-public guard here or documenting the divergence.

}

let enr_server_handle = tokio::spawn(enr_server(
server_errors.clone(),
config.clone(),
Expand Down Expand Up @@ -120,15 +129,23 @@ pub async fn run_relay_p2p_node(
event = node.select_next_some() => {
let address_update = handle_swarm_event(&event, config.filter_private_addrs);

// Update listener address list
// Keep the ENR listener list and the libp2p external addresses
// (which back the circuit reservations we issue) in sync, so
// reserving clients always receive at least one reachable
// address for the same set we advertise over `/enr`.
match address_update {
AddrUpdate::Add(address) => {
node.add_external_address(address.clone());
listeners.write().await.push(address);

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.

nit: listeners.push(address) on AddrUpdate::Add is unconditional. If libp2p ever re-emits NewListenAddr for the same address (interface flaps / re-listens), listeners accumulates duplicates. This is masked downstream — web.rs's advertised-addrs dedups the external∪listeners union via a HashSet, and swarm.add_external_address dedups internally — so there's no user-visible drift. Correctness is fine; a dedup-on-push (or a set) would just be tidier for a long-lived relay.

}
AddrUpdate::Remove(address) => {
node.remove_external_address(&address);
listeners.write().await.retain(|a| *a != address);
}
AddrUpdate::RemoveAll(addresses) => {
for address in &addresses {
node.remove_external_address(address);
}
listeners
.write()
.await
Expand Down
6 changes: 4 additions & 2 deletions crates/tracing/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ pub struct TracingConfig {
/// Enables metrics logging. If not - no metrics logging is enabled.
pub metrics: bool,

/// Overrides the environment filter. If not - the environment filter is
/// used.
/// Filter directive applied unless `RUST_LOG` is set in the environment,
/// which always takes precedence. When neither is set, the built-in
/// default (`info`) is used. Populated from `--log-level` /
/// `CHARON_LOG_LEVEL` by the CLI.
pub override_env_filter: Option<String>,
}

Expand Down
64 changes: 58 additions & 6 deletions crates/tracing/src/init.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use std::str::FromStr;

use base64::{Engine as _, engine::general_purpose::STANDARD as BASE64};
use percent_encoding::percent_decode_str;
use tracing_loki::{BackgroundTask, BackgroundTaskController, url::Url};
Expand Down Expand Up @@ -44,10 +42,13 @@ pub struct LokiInit {

/// Initializes the tracing subscriber.
pub fn init(config: &TracingConfig) -> Result<Option<LokiInit>> {
let env_filter = if let Some(override_env_filter) = config.override_env_filter.as_ref() {
EnvFilter::from_str(override_env_filter).unwrap_or_else(|_| default_env_filter())
} else {
EnvFilter::try_from_env("RUST_LOG").unwrap_or_else(|_| default_env_filter())
let rust_log = std::env::var("RUST_LOG").ok();
let env_filter = match resolve_filter_directive(
rust_log.as_deref(),
config.override_env_filter.as_deref(),
) {
Some(directive) => EnvFilter::try_new(&directive).unwrap_or_else(|_| default_env_filter()),

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.

EnvFilter::try_new(&directive).unwrap_or_else(|_| default_env_filter()) swallows a parse error and silently drops to info. This preserves the pre-existing behavior (not a regression), but it's poor observability for exactly the fine-grained escape-hatch path the new doc comment advertises: an operator who typos RUST_LOG=infp,pluto_p2p=debug gets plain info with zero signal. Since tracing isn't initialized yet here, consider an eprintln! to stderr on the error arm before falling back. Not blocking.

None => default_env_filter(),
};

let console_config = config.console.clone().unwrap_or_default();
Expand Down Expand Up @@ -125,10 +126,61 @@ fn default_env_filter() -> EnvFilter {
EnvFilter::new("info")
}

/// Selects the tracing filter directive using this precedence:
///
/// 1. `RUST_LOG` from the environment — the standard runtime escape hatch for

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.

RUST_LOG winning over an explicitly-passed --log-level inverts charon's precedence. Charon resolves as explicit --log-level flag > CHARON_LOG_LEVEL env > default (cmd/cmd.go bindFlags short-circuits on f.Changed), so an explicit flag always beats the env var there.

The change is defensible — RUST_LOG is the tracing-subscriber standard and supports per-target directives (info,pluto_p2p=debug) that charon's single-scalar level can't express, and charon has no RUST_LOG. But it's a genuine precedence divergence for that env var; the doc comment should state explicitly that this intentionally differs from charon (where the explicit flag wins over the env).

/// fine-grained, per-target filtering (e.g. `info,pluto_p2p=debug`) — when
/// set to a non-empty value;
/// 2. the config override, populated from `--log-level` / `CHARON_LOG_LEVEL`;
/// 3. `None`, meaning the caller should fall back to [`default_env_filter`].
///
/// `RUST_LOG` intentionally wins over `--log-level` so operators can crank up
/// specific targets without rebuilding or changing the flag the process was
/// launched with. Takes the `RUST_LOG` value as an argument rather than reading
/// the environment directly so the precedence logic is unit-testable.
fn resolve_filter_directive(
rust_log: Option<&str>,
override_env_filter: Option<&str>,
) -> Option<String> {
if let Some(rust_log) = rust_log
&& !rust_log.trim().is_empty()
{
return Some(rust_log.to_string());

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.

nit: both Some(rust_log.to_string()) and override_env_filter.map(str::to_string) convert &str -> String via ToString (Display-based) rather than ToOwned. to_owned() / str::to_owned is marginally more idiomatic and is what clippy's str_to_string lint prefers. Purely stylistic; flag only if that lint is enabled.

}

override_env_filter.map(str::to_string)

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.

Asymmetric blank-string handling: rust_log is rejected when trim().is_empty() (so Some(" ") falls through to the override), but override_env_filter is returned verbatim with no equivalent guard. A blank --log-level/CHARON_LOG_LEVEL (Some(" ")) is therefore forwarded to EnvFilter::try_new, fails to parse, and silently falls back to info — instead of cleanly returning None and using the default. The override is CLI-sourced so blank is unlikely, but the asymmetry is surprising given both are Option<&str> directives. Either apply the same trim().is_empty() filter to the override, or document the difference.

}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn rust_log_takes_precedence_over_override() {
assert_eq!(
resolve_filter_directive(Some("info,pluto_p2p=debug"), Some("warn")),
Some("info,pluto_p2p=debug".to_string())
);
}

#[test]
fn override_used_when_rust_log_absent_or_blank() {

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.

nit: resolve_filter_directive tests cover blank/empty rust_log but not a blank override (e.g. resolve_filter_directive(None, Some(" "))), which currently returns Some(" ") rather than None per the asymmetry above. Adding that case would pin the intended contract and catch a future change to the override's empty-handling. The existing cases mirror the precedence rules clearly otherwise.

assert_eq!(
resolve_filter_directive(None, Some("debug")),
Some("debug".to_string())
);
assert_eq!(
resolve_filter_directive(Some(" "), Some("debug")),
Some("debug".to_string())
);
}

#[test]
fn none_when_neither_set() {
assert_eq!(resolve_filter_directive(None, None), None);
assert_eq!(resolve_filter_directive(Some(""), None), None);
}

#[test]
fn basic_auth_extracted_from_user_and_password() {
let url = Url::parse("https://alice:s3cr3t@loki.example.com/push").unwrap();
Expand Down
107 changes: 107 additions & 0 deletions test-infra/harness/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
# Go simnet harness

A Go test harness that runs simnet-style end-to-end duty tests over
distributed-validator clusters mixing **charon** nodes (in-process, reusing
charon's own `app.Run` and test utilities) and **pluto** nodes (subprocesses
of the pluto binary). It reuses charon `v1.7.1` (the pluto parity reference)
as a library: `cluster.NewForT` fixtures, `testutil/beaconmock`,
`testutil/validatormock` and `testutil/relay`.

## Architecture

```text
┌────────────────────────── Go test process ──────────────────────────┐
│ │
deterministic │ relay (in-process libp2p) │
cluster fixture ─▶ │ │
(lock, p2p keys, │ beaconmock (shared chain state, deterministic duties) │
BLS shares) │ ▲ ▲ ▲ │
│ │ Go │ HTTP │ HTTP │
│ charon app.Run gateway[0..k] ◀── capture ── gateway[k..n] │
│ (in-process, ▲ ▲ │
│ real p2p) │ beacon API │ beacon API │
└───────────────────┼─────────────────────────────┼────────────────────┘
│ │
charon app.Run pluto run (subprocess)
(gateway mode) ▲
│ validator API
validatormock (harness-driven)
```

Key design points, mirroring charon's `testutil/integration` simnet:

- **One shared beaconmock, one HTTP gateway per node.** Charon's beaconmock
implements dynamic behaviour (duties, validators) on its Go client
interface only; its HTTP server serves static stubs. The gateway fronts
the shared mock over real HTTP so external processes get a complete
beacon API, and captures submissions per node for assertions.
- **Real p2p partial-signature exchange.** Upstream simnet uses an
in-memory ParSigEx transport; the harness omits it so out-of-process
nodes can participate in QBFT consensus and threshold signing.
- **Assertions at the beacon boundary.** Every node must submit an
attestation for the same slot, and all payloads must be identical after
JSON normalization (they are group-signed threshold aggregates). For the
in-process baseline, upstream-style `BroadcastCallback` assertions are
used instead.

## Scenarios

| Test | Cluster | Purpose |
|---|---|---|
| `TestSimnetAttesterCharonInProcess` | 3× charon (in-process bmock) | Baseline; validates fixture/relay/assertion plumbing |
| `TestSimnetAttesterCharonViaGateway` | 3× charon via gateway | Proves the gateway serves a complete beacon API over HTTP |
| `TestSimnetAttesterPluto` | 3× pluto | Pure-pluto duty e2e (skips until `pluto run` lands) |
| `TestSimnetAttesterMixed` | 2× charon + 2× pluto, threshold 3 | Cross-implementation interop; threshold forces both implementations to participate in every duty |

## Running

```bash
cd test-infra/harness

# Charon-only scenarios (no pluto binary needed), ~10s total:
go test -v -run 'TestSimnetAttesterCharon' ./...

# Full suite; pluto scenarios skip unless PLUTO_BIN supports `pluto run`:
PLUTO_BIN=../../target/debug/pluto go test -v ./...
```

The pluto scenarios probe `$PLUTO_BIN run --help` and skip cleanly until the
`run` command exists, so they are safe to keep enabled in CI.

### Validating the subprocess path today

The subprocess runner passes `charon run`-shaped flags, so a charon binary
can stand in for pluto to exercise the entire subprocess path (on-disk
fixture layout, flags, readiness polling, HTTP-driven validator mock,
capture assertions):

```bash
PLUTO_BIN=/path/to/charon go test -v -run TestSimnetAttesterMixed ./...
```

## Requirements on `pluto run`

For the pluto scenarios to activate, `pluto run` needs to accept the
charon-equivalent flags the harness passes (see `pluto.go`):
`--lock-file`, `--private-key-file`, `--beacon-node-endpoints`,
`--validator-api-address`, `--monitoring-address`, `--p2p-tcp-address`,
`--p2p-relays` — and to serve the validator API on the configured address
(readiness is detected by TCP connect).

## Versioning

Charon is pinned to the pluto parity reference (`v1.7.1`) in `go.mod`. The
two `replace` directives are copied from charon's own `go.mod` (Go does not
propagate a dependency's replaces) and must be kept in sync when bumping
charon. Bump deliberately alongside the parity target, not to track charon
main.

## Extending

- More duty types: add scenarios passing different beaconmock options
(upstream simnet's proposer/sync-committee configurations translate
directly; see `simnetBMockOpts` and charon's `TestSimnetDuties`).
- Fault injection: start N nodes but kill/withhold one below threshold,
asserting duties still complete (compose-smoke style).
- The gateway logs any non-GET request it reverse-proxies; if a future
client needs an endpoint dynamically, the log line names it.
Loading
Loading