openspecs for the NS API, NS API agent and gateway probe#6945
openspecs for the NS API, NS API agent and gateway probe#6945mmsinclair wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughAdds a complete specification set for the node-status subsystem, covering architecture, gateway probing, agent workflows, HTTP APIs, monitoring, persistence, test-run orchestration, ticketbooks, and end-to-end lifecycle behavior. ChangesNode status subsystem
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nym-gateway-probe/docs/gateway-probe.md`:
- Around line 87-94: Correct the wg-mix row in the test-mode table so Mixnet
ping is marked disabled, while keeping WireGuard enabled and LP/SOCKS5 disabled.
Align the documentation with TestMode::mixnet_tests(), which only enables mixnet
tests for Core and All.
In `@openspec/specs/gateway-probe/spec.md`:
- Around line 55-57: Clarify the credential-mode requirement so the exactly-one
CLI selection applies only to non-agent runs. In the agent-run path, such as
RunAgent, explicitly allow credentials to be imported from supplied
AttachedTicketMaterials without requiring --use-mock-ecash or --mnemonic, while
preserving the existing mnemonic and mock-ecash behavior for other probe runs.
In `@openspec/specs/node-status-api-http/spec.md`:
- Around line 77-89: Update get_gateway_list and its callers to propagate
gateways-table read failures as the established internal HTTP error, returning
status 500 instead of panicking. Remove the unrecoverable panic behavior and add
a specification scenario covering a gateway read failure and the expected HTTP
500 response.
In `@openspec/specs/node-status-api-persistence/spec.md`:
- Around line 75-79: Update the timestamp persistence contract in the
specification and related database models/migrations to use i64-compatible Unix
timestamp storage, including the required migration. If retaining i32 is
intentional, instead define the supported timestamp range and add tests
enforcing it; do not leave the year-2038 behavior unspecified.
In `@openspec/specs/node-status-api-ticketbook/spec.md`:
- Around line 26-38: Extend the refill requirement around aggregation failure to
define the recovery contract: identify the component that retries persisted
pending/recovery rows, specify the transition to completed after wallet
aggregation succeeds, and require idempotency or deposit identification so
retries cannot issue duplicate funds. Add scenarios covering retry, successful
reconciliation, and duplicate-prevention behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4dd0cd23-3c2d-4df6-a2ed-d96a1518c2d1
📒 Files selected for processing (10)
nym-gateway-probe/docs/gateway-probe.mdopenspec/specs/architecture/spec.mdopenspec/specs/gateway-probe/spec.mdopenspec/specs/node-status-agent/spec.mdopenspec/specs/node-status-api-http/spec.mdopenspec/specs/node-status-api-monitoring/spec.mdopenspec/specs/node-status-api-persistence/spec.mdopenspec/specs/node-status-api-testruns/spec.mdopenspec/specs/node-status-api-ticketbook/spec.mdopenspec/specs/overview/spec.md
| | Test mode | Mixnet ping | WireGuard | LP | SOCKS5 | | ||
| |---|:---:|:---:|:---:|:---:| | ||
| | `core` (alias `mixnet`) | ✅ | ✅ | — | — | | ||
| | `all` | ✅ | ✅ | ✅ | ✅ | | ||
| | `wg-mix` | ✅ | ✅ | — | — | | ||
| | `wg-lp` | — | ✅ | ✅ | — | | ||
| | `lp-only` | — | — | ✅ | — | | ||
| | `socks5-only` | — | — | — | ✅ | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the wg-mix test-mode table.
The wg-mix row currently says Mixnet ping is enabled, but TestMode::mixnet_tests() only enables mixnet tests for Core and All. WgMix requires a mixnet client for WireGuard, but does not run the mixnet ping phase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nym-gateway-probe/docs/gateway-probe.md` around lines 87 - 94, Correct the
wg-mix row in the test-mode table so Mixnet ping is marked disabled, while
keeping WireGuard enabled and LP/SOCKS5 disabled. Align the documentation with
TestMode::mixnet_tests(), which only enables mixnet tests for Core and All.
| ### Requirement: The probe SHALL acquire or import ecash credentials according to credential mode | ||
|
|
||
| Exactly one credential mode MUST be selected via the CLI arg group: `--use-mock-ecash` or `--mnemonic`. In mnemonic mode, if the credential store holds fewer than one ticketbook, the probe MUST acquire bandwidth for `V1MixnetEntry`, `V1WireguardEntry`, and `V1WireguardExit`. Mock-ecash mode MUST use a mock bandwidth controller and acquire nothing. In agent runs, credentials MUST instead be imported from supplied `AttachedTicketMaterials`. Bandwidth acquisition MUST retry (up to 50 attempts, 1s backoff) and MUST treat "account sequence mismatch" as retryable. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the agent credential exception explicit.
This currently requires exactly one CLI credential mode, but RunAgent is defined to import AttachedTicketMaterials instead. If interpreted literally, agent runs would still require --use-mock-ecash or --mnemonic, conflicting with the agent submission contract.
Suggested clarification
-Exactly one credential mode MUST be selected via the CLI arg group: `--use-mock-ecash` or `--mnemonic`.
+For non-agent runs, exactly one credential mode MUST be selected via the CLI arg group: `--use-mock-ecash` or `--mnemonic`.
+`RunAgent` MUST instead require and import the supplied `AttachedTicketMaterials` and MUST NOT acquire credentials from the local mnemonic/mock-ecash modes.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Requirement: The probe SHALL acquire or import ecash credentials according to credential mode | |
| Exactly one credential mode MUST be selected via the CLI arg group: `--use-mock-ecash` or `--mnemonic`. In mnemonic mode, if the credential store holds fewer than one ticketbook, the probe MUST acquire bandwidth for `V1MixnetEntry`, `V1WireguardEntry`, and `V1WireguardExit`. Mock-ecash mode MUST use a mock bandwidth controller and acquire nothing. In agent runs, credentials MUST instead be imported from supplied `AttachedTicketMaterials`. Bandwidth acquisition MUST retry (up to 50 attempts, 1s backoff) and MUST treat "account sequence mismatch" as retryable. | |
| ### Requirement: The probe SHALL acquire or import ecash credentials according to credential mode | |
| For non-agent runs, exactly one credential mode MUST be selected via the CLI arg group: `--use-mock-ecash` or `--mnemonic`. In mnemonic mode, if the credential store holds fewer than one ticketbook, the probe MUST acquire bandwidth for `V1MixnetEntry`, `V1WireguardEntry`, and `V1WireguardExit`. Mock-ecash mode MUST use a mock bandwidth controller and acquire nothing. In agent runs, credentials MUST instead be imported from supplied `AttachedTicketMaterials` and MUST NOT acquire credentials from the local mnemonic/mock-ecash modes. Bandwidth acquisition MUST retry (up to 50 attempts, 1s backoff) and MUST treat "account sequence mismatch" as retryable. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/specs/gateway-probe/spec.md` around lines 55 - 57, Clarify the
credential-mode requirement so the exactly-one CLI selection applies only to
non-agent runs. In the agent-run path, such as RunAgent, explicitly allow
credentials to be imported from supplied AttachedTicketMaterials without
requiring --use-mock-ecash or --mnemonic, while preserving the existing mnemonic
and mock-ecash behavior for other probe runs.
| ### Requirement: The API SHALL map error conditions to typed HTTP statuses | ||
|
|
||
| Errors MUST map as: invalid input → 400, unauthorized → 401, "no testruns available" → 503, "no delegations for node" → 404, and internal errors → 500 (with optional logging). Summary reads MUST require all summary keys to be present, returning 500 otherwise. | ||
|
|
||
| #### Scenario: No testruns available | ||
| - **WHEN** an internal testrun request finds nothing to assign | ||
| - **THEN** the API returns HTTP 503 | ||
|
|
||
| ## Technical Notes | ||
|
|
||
| - **Implementation**: `http/server.rs` (`start_http_api`, binds `0.0.0.0:{http_port}`), `http/api/mod.rs` (router + CORS + request logging), `http/state.rs` (`AppState`, `HttpCache`), `http/models/mod.rs` (`Gateway`, `DVpnGateway`, `ExtendedNymNode`, `Service`, `SessionStats`, scoring), `http/models/gw_probe/` (probe deserialization + scoring + `socks5_calc.rs`), `http/error.rs`. | ||
| - **Known behavior**: `get_gateway_list` panics if the gateways table read fails (treated as unrecoverable). | ||
| - **Framework**: axum + tower-http; OpenAPI via utoipa/utoipa-swagger-ui/utoipauto. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not leave gateway read failures as a panic.
Lines 77-79 promise that internal errors return HTTP 500, but Line 88 documents that get_gateway_list panics when the gateways-table read fails. A transient database failure could therefore terminate the API instead of returning the documented typed error. Propagate the failure as an internal HTTP error and add a scenario covering this path.
🧰 Tools
🪛 LanguageTool
[grammar] ~82-~82: Ensure spelling is correct
Context: ...struns available - WHEN an internal testrun request finds nothing to assign - **THE...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/specs/node-status-api-http/spec.md` around lines 77 - 89, Update
get_gateway_list and its callers to propagate gateways-table read failures as
the established internal HTTP error, returning status 500 instead of panicking.
Remove the unrecoverable panic behavior and add a specification scenario
covering a gateway read failure and the expected HTTP 500 response.
| ## Technical Notes | ||
|
|
||
| - **Implementation**: `db/mod.rs` (`Storage::init`, pool, migrations), `db/models.rs` (DTOs, `TestRunStatus`/`TestRunKind` enums, self-described de-helpers with `#[serde(default)]` for forward-compat, ports-check normalization), `db/queries/*` (per-table modules). | ||
| - **Engine**: PostgreSQL only, via `sqlx` (`runtime-tokio-rustls`) with compile-time-checked `query!`/`query_scalar!` macros; migrations in `./migrations_pg`. | ||
| - **Known risks**: several unix timestamps are stored as `i32` (year-2038 truncation risk); `get_raw_node_stats` reads the second-most-recent row (`LIMIT 1 OFFSET 1`) as the delta baseline. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Turn the timestamp overflow risk into an explicit contract.
The specification acknowledges that several Unix timestamps use i32, creating a year-2038 truncation risk, but defines no mitigation or supported range. Require i64 storage/migration (or explicitly document and test the supported lifetime) before this persistence contract is considered complete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/specs/node-status-api-persistence/spec.md` around lines 75 - 79,
Update the timestamp persistence contract in the specification and related
database models/migrations to use i64-compatible Unix timestamp storage,
including the required migration. If retaining i32 is intentional, instead
define the supported timestamp range and add tests enforcing it; do not leave
the year-2038 behavior unspecified.
| ### Requirement: Refill SHALL respect shutdown between chunks and persist a recovery row on aggregation failure | ||
|
|
||
| Deposits MUST be made in chunks bounded by `max_concurrent_deposits`, checking for shutdown *between* chunks so that an in-flight chain operation is not abandoned mid-deposit. If wallet aggregation from the quorum fails, the manager MUST persist a pending/recovery row rather than losing the deposit. | ||
|
|
||
| #### Scenario: Shutdown honored between chunks | ||
| - **GIVEN** a multi-chunk refill in progress | ||
| - **WHEN** shutdown is signalled between chunks | ||
| - **THEN** the current in-flight chain operation completes and no new chunk is started | ||
|
|
||
| #### Scenario: Aggregation failure persisted for recovery | ||
| - **GIVEN** a completed on-chain deposit whose quorum wallet aggregation fails | ||
| - **WHEN** the failure occurs | ||
| - **THEN** a pending/recovery row is persisted so the ticketbook can be recovered |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Specify how aggregation-recovery rows are reconciled.
Persisting a pending row after a completed deposit is not sufficient unless the specification also defines who retries it, how it transitions to completed, and how duplicate deposits are prevented. Without that recovery contract, the next refill can strand the deposit or issue additional funds unnecessarily.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/specs/node-status-api-ticketbook/spec.md` around lines 26 - 38,
Extend the refill requirement around aggregation failure to define the recovery
contract: identify the component that retries persisted pending/recovery rows,
specify the transition to completed after wallet aggregation succeeds, and
require idempotency or deposit identification so retries cannot issue duplicate
funds. Add scenarios covering retry, successful reconciliation, and
duplicate-prevention behavior.
| - **Assignment is gated.** The NS API only assigns a test run if the request is authenticated and *fresh* (timestamp within the freshness window, default 120s), the in-progress count is below `agent_max_count`, and there are enough ecash ticketbooks of every buffered type. It claims the **oldest queued** run for a bonded gateway (performance > 0) atomically with `FOR UPDATE ... SKIP LOCKED`, so two agents never get the same run. | ||
| - **Ticket materials pay for the tests.** The NS API's ticketbook manager keeps a buffer of threshold-signed ecash ticketbooks; on assignment it attaches materials the probe imports to pay for mixnet/WireGuard bandwidth during the run. | ||
| - **Log capture is bracketed.** The agent captures only the tracing output emitted between `start()` and `stop_and_drain()` — i.e. during the probe run — and submits it alongside the result. | ||
| - **Submission fans out with version asymmetry.** The primary server gets the v1 endpoint (log truncated to 1024 bytes); secondary servers get the v2 endpoint (full log + gateway identity key). Per-server failures are logged but don't abort the others. |
There was a problem hiding this comment.
I guess that's something we want to fix, i.e. they should be using the same endpoint, no?
| - **Ticket materials pay for the tests.** The NS API's ticketbook manager keeps a buffer of threshold-signed ecash ticketbooks; on assignment it attaches materials the probe imports to pay for mixnet/WireGuard bandwidth during the run. | ||
| - **Log capture is bracketed.** The agent captures only the tracing output emitted between `start()` and `stop_and_drain()` — i.e. during the probe run — and submits it alongside the result. | ||
| - **Submission fans out with version asymmetry.** The primary server gets the v1 endpoint (log truncated to 1024 bytes); secondary servers get the v2 endpoint (full log + gateway identity key). Per-server failures are logged but don't abort the others. | ||
| - **Submission is validated.** The NS API only completes a run if it is in-progress and the submitted `assigned_at_utc` matches the stored assignment. On the v2 endpoint an unknown test-run id causes the API to create an "external" gateway + run before recording the result. |
There was a problem hiding this comment.
yes, that bit is quite problematic and I think it could use a bit more of A1 explanation. essentially if some test takes too long, it will internally time out and is going to get re-assigned to a different agent. However, when the original one comes back around with the result, it will get rejected
|
|
||
| ### Requirement: The agent SHALL run as a one-shot CLI, looped externally | ||
|
|
||
| The agent process MUST parse its arguments once, execute a single subcommand, and exit. Continuous operation MUST be achieved by an external loop (the container `entrypoint.sh` runs it in a `while true` loop with a `SLEEP_TIME` interval). The agent MUST NOT implement its own daemon loop or internal retry across invocations. |
There was a problem hiding this comment.
honestly, if we'd be moving gateway proxy things around, I'd loosen that requirement or maybe completely remove it.
|
|
||
| ## Requirements | ||
|
|
||
| ### Requirement: The probe SHALL expose four run modes selected by subcommand |
There was a problem hiding this comment.
just thinking aloud here, I don't think we need the complexity of having to support all the modes. @simonwicky can correct me if I'm wrong, but from what I recall, in particular, RunLocal was quite annoying to update to "work" with the current structure and I'm not sure it even supports all the features, like LP
|
|
||
| ### Requirement: Test phases SHALL run in a fixed order with the mixnet client disconnected before LP/SOCKS5 | ||
|
|
||
| `do_probe_test` MUST sequence phases as: mixnet ping → WireGuard (holding the mixnet client) → **disconnect the mixnet client** → LP registration → SOCKS5. Each phase MUST be gated by its `TestMode` predicate, and a failure in one phase MUST NOT prevent later phases (SOCKS5 errors in particular are logged and non-fatal); WG/LP results fall back to defaults on failure. |
There was a problem hiding this comment.
is this actually what we want? I think it exposes an issue after socks5 has been introduced. since socks5 does require a mixnet client, it should perform that test before disconnecting and doing LP...
|
|
||
| ### Requirement: The network monitor SHALL periodically snapshot the directory, geodata, and delegations | ||
|
|
||
| The monitor MUST run on a loop (default refresh 300s; 60s retry on failure). Each cycle MUST: check the ipinfo bandwidth allowance, fetch described nodes v2 from `nym-api`, classify gateways by declared entry/exit-IPR role, fetch bonded/basic/mixing nodes and families, write nym-nodes and families to the database, refresh geodata for every node (geolocating each IP via ipinfo, caching the first success with TTL `geodata_ttl`, else an empty location), build and write gateway records (including an `explorer_pretty_bond` with location and performance), refresh per-node delegations from `nyxd`, and write the network summary (8 summary keys plus `NetworkSummary`). A `run_once` early-exit mode MUST write only nodes and families. |
There was a problem hiding this comment.
do we really have to do a ipinfo/geolocation lookup every 300s? seems like a huge overkill. how about only do it if
A) it's a new gateway we know nothing about
B) its IP address has updated
C) once a day/week otherwise
also, we should probably move it to a separate, dedicated service (that would eventually submit it to contracts)
|
|
||
| ### Requirement: The packet-stats scraper SHALL fetch per-node stats and accumulate daily deltas | ||
|
|
||
| The node (packet-stats) scraper MUST run every 1 hour with concurrency from `packet_stats_max_concurrent_tasks` (default 10). Per node it MUST fetch `/stats` (parsing both legacy-mixnode and nym-node formats) and, for entry/exit gateways, fetch bridge client-params. Results MUST be batch-stored, and daily deltas MUST be computed against the previous raw stats row; on a counter decrease (node restart) it MUST use the current value rather than a negative delta. |
There was a problem hiding this comment.
this should really do some post-processing and remove any outliers, because remember those are the Sve... self-declared endpoints that could easily be polluted
| #### Scenario: Gateway node written to both tables | ||
| - **GIVEN** a scraped node whose key is a bonded gateway | ||
| - **WHEN** its description is stored | ||
| - **THEN** the description is written to both the nym-node and gateway description tables |
There was a problem hiding this comment.
I think this is unnecessary complexity and probably source of problems
|
|
||
| ### Requirement: The background queuer SHALL enqueue idempotent probe test runs and re-queue stale ones | ||
|
|
||
| The test-run queuer MUST loop: re-queue stale in-progress test runs (those assigned before the stale window, default 7200s), then iterate all gateways ordered by last-testrun time, filter to bonded gateways, and queue a probe run per gateway, then sleep the refresh interval (default 450s). Enqueue MUST be idempotent — if a non-complete Probe run already exists for a gateway it MUST be reused rather than duplicated; otherwise a new `Queued`/`Probe` row is inserted. Unknown gateways MUST be rejected. |
There was a problem hiding this comment.
" if a non-complete Probe run already exists for a gateway it MUST be reused rather than duplicated;"
I wonder if it would be better if it was duplicated after all. worst case, we'd just have an additional test run information
|
|
||
| ## Purpose | ||
|
|
||
| This overview describes a three-part subsystem that measures and reports the health of the Nym mixnet. The **node status API** persists a snapshot of the network and serves it over HTTP, while also orchestrating gateway "test runs". External **agents** poll the API for assigned test runs, execute network diagnostics against a target gateway using the **gateway probe** library, and submit signed results back. Together they produce the connectivity/performance data that powers dVPN gateway directories and network dashboards. |
There was a problem hiding this comment.
that measures and reports the health of the Nym mixnet
well, not just the mixnet, but also, and mostly, dvpn
4 participants
Adds openspecs for the:
This is ahead of work to refactor them and change how the tests are done
This change is
Summary by CodeRabbit