diff --git a/Cargo.lock b/Cargo.lock index 6f84132a..d80efae9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.189.0" +version = "0.190.0" dependencies = [ "async-trait", "axum", diff --git a/Cargo.toml b/Cargo.toml index 08e7541a..1005fc3d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.189.0" +version = "0.190.0" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over diff --git a/README.md b/README.md index e46da825..00880387 100644 --- a/README.md +++ b/README.md @@ -301,6 +301,15 @@ CORS reflects `chrome-extension://` and the local page origins `http://localhost / `http://127.0.0.1` / `http://127.0.0.2` / `http://[::1]` (with or without a `:port`), so the extension and any page served from a canonical local name can call it. +### `dign network-info` — this node's own posture, from the terminal + +`dign network-info` prints the node's peer id, network id, effective L2 genesis, listen address, +reachability, and the addresses it advertises, in the node's own IPv6-first order. It reads the +open `dig.getNetworkInfo` surface, so it needs **no control token and no elevation** — on a `.deb` +install the control token is `0600 root:root`, and asking "what is my node's address" should not +require `sudo` for a read the node already performs for any peer that dials it. A field the node +did not report prints as `unknown` rather than as a plausible default. + ## Machine-readable contracts (agent-friendly) ### CLI `--json` @@ -332,6 +341,7 @@ typed `ExitCode` enum in `src/cli.rs`: | 4 | `SERVICE_FAILED` | A service operation failed (register/start/stop/uninstall). | | 5 | `BIND_FAILED` | `run`: could not bind the loopback address. | | 6 | `IO_ERROR` | Other I/O error. | +| 12 | `NODE_UNREACHABLE` | The node did not answer; the operation was not measured. | ### JSON-RPC error-code catalogue diff --git a/SPEC.md b/SPEC.md index f0d62d3a..d1962685 100644 --- a/SPEC.md +++ b/SPEC.md @@ -2401,12 +2401,29 @@ flood of never-attached `begin`s cannot grow engine state without bound. `install` · `uninstall` · `start` · `stop` (each accepting `--scope `, §9.1) · `status` · `pair` (§7.11) · `open` (§8.5) · the **control-parity** subcommands `info` · `config` · `cache` · `stores` · `sync` · `updater` · -`subscriptions` (§8.6) · `peers` (§8.7) · `logs` (§11). +`subscriptions` (§8.6) · `peers` (§8.7) · `network-info` (§8.8) · `logs` (§11). The `dign` alias binary (§2.1a) exposes this SAME subcommand set with the SAME semantics — `dign ` is equivalent to `dig-node ` in every respect except the reported program name. +### 8.8. `network-info` — this node's own network posture (#303) + +`network-info` prints this node's `peer_id`, network id, effective L2 genesis, listen address, +reachability, and its advertised candidate addresses in the node's own advertisement order, which +is IPv6-first (§5.2). The order MUST be passed through untouched: re-sorting would hide a node +whose IPv6 advertisement is missing, which is the fault an operator runs the command to find. An +absent field MUST render as `unknown` and MUST NOT be filled with a plausible default — a +fabricated `direct` or an invented `0.0.0.0` reads exactly like a measurement. + +**It reads the OPEN surface and is NOT token-gated, deliberately.** It is the one documented +exception to §8.6's rule that a CLI subcommand presents the master control token: it calls +`dig.getNetworkInfo`, whose body this node already hands any peer that dials it, so a loopback +caller learns nothing a stranger does not. Gating it would buy no confidentiality while costing +real availability — on a `.deb` install the control token is `0600 root:root` (§7.11/#501), so an +ordinary user asking "what is my node's address" would be told to elevate for a read the network +performs for free. This is a property to PRESERVE, not an oversight to tighten later. + ### 8.6. Control-parity subcommands (#426) For EVERY gated `control.*` method the DIG Chrome extension drives (§7), the CLI exposes an @@ -2415,7 +2432,9 @@ extension drives it from a browser. Each subcommand is a THIN dispatch — it ca `control.*` method over the node's loopback endpoint, presenting the MASTER control token (`X-Dig-Control-Token`, read WITHOUT minting — §7.11/#501); no CLI logic is forked from the control plane. A mutating CLI control is therefore gated by the identical capability as the WS surface (the -on-disk master token = local-machine control), never an unauthenticated backdoor. +on-disk master token = local-machine control), never an unauthenticated backdoor. The one +documented exception is `network-info` (§8.8), which reads an OPEN, already-public surface and is +token-free by design; it is not a control-parity subcommand and this rule does not reach it. - `info` → `control.status` — the rich node status (version, uptime, cache, hosted-store + cached-capsule counts, §21 sync availability). DISTINCT from `status` (§8.3), which is an @@ -2620,10 +2639,43 @@ exit `1` (`NOT_SERVING`) so scripts can gate on liveness; the JSON result carrie | 4 | `SERVICE_FAILED` | A service-manager operation failed. | | 5 | `BIND_FAILED` | `run`: could not bind the loopback address. | | 6 | `IO_ERROR` | Other I/O error. | +| 12 | `NODE_UNREACHABLE` | The node did not answer; the operation was not measured. | + +I/O-error mapping, in the order `ExitCode::from_io_error` matches — every arm, because a partial +list reads as complete and the omitted arms are exactly the ones a caller gets wrong: +`PermissionDenied` → 3; `AddrInUse`/`AddrNotAvailable` → 5; `InvalidInput` → 2 (a bad argument +surfaced as an I/O error is still a usage error); `ConnectionRefused` → 12; anything else → 6. -I/O-error mapping: `PermissionDenied` → 3; `AddrInUse`/`AddrNotAvailable` → 5; anything else → 6. Numeric values and symbolic names are a stable contract and MUST NOT be renumbered. +**The occupied numbers span the whole DIG command line, not this CLI alone (MUST).** `dign` and +dig-app's `diga` deliberately share one numbering so a caller sees one surface across both +(`dig-app-core/src/gateway/outcome.rs`), so a code is available only if it is unoccupied +ECOSYSTEM-WIDE. Absence from the table above does NOT make a number free — this repo has already +paid for that reasoning once in the JSON-RPC error space, where `-32015` was taken as "the next +free code" from the owning crate's own list and collided with a released `METADATA_TOO_LARGE`, +forcing a yank. The full occupied set, measured, is: + +| Code | `dign` (this CLI) | `diga` (dig-app gateway) | +|---|---|---| +| 0 | `OK` | `OK` | +| 1 | `NOT_SERVING` | — | +| 2 | `USAGE` | `USAGE` | +| 3 | `PERMISSION_DENIED` | — | +| 4 | `SERVICE_FAILED` | — | +| 5 | `BIND_FAILED` | — | +| 6 | `IO_ERROR` | `IO_ERROR` | +| 7 | — | `NOT_CONNECTED` | +| 8 | — | `ENGINE_ERROR` | +| 9 | — | `LOCKED` | +| 10 | — | `NOT_FOUND` | +| 11 | — | `DENIED` | +| 12 | `NODE_UNREACHABLE` | — | + +0–11 were therefore taken before this CLI added a code, and 12 is the first free number. A new +code MUST be drawn from 13 upward and MUST re-check BOTH tables first; 126, 127 and 128+n are +reserved by the shell and MUST NOT be used. + --- ## 9. OS-service contract diff --git a/USER_JOURNEY.md b/USER_JOURNEY.md index eda4eaa7..4bfd4b67 100644 --- a/USER_JOURNEY.md +++ b/USER_JOURNEY.md @@ -163,7 +163,7 @@ every subcommand (machine output to stdout, prose to stderr). - **`--json`** on the CLI: success → `{ ok:true, action, service, version, …result }`; failure → `{ ok:false, error:{ code, exit_code, message, hint } }`. - **Exit-code table** (documented in the README + `src/cli.rs`): `0 OK`, `1 NOT_SERVING`, - `2 USAGE`, `3 PERMISSION_DENIED`, `4 SERVICE_FAILED`, `5 BIND_FAILED`, `6 IO_ERROR`. + `2 USAGE`, `3 PERMISSION_DENIED`, `4 SERVICE_FAILED`, `5 BIND_FAILED`, `6 IO_ERROR`, `12 NODE_UNREACHABLE`. - **Stable JSON-RPC error codes** (UPPER_SNAKE in `error.data.code`): `PARSE_ERROR` (-32700), `INVALID_REQUEST` (-32600), `METHOD_NOT_FOUND` (-32601), `INVALID_PARAMS` (-32602), `DISPATCH_FAILED` (-32000, shell), `UPSTREAM_ERROR` (-32010, shell), and the control-plane codes diff --git a/crates/dig-node-service/src/cli.rs b/crates/dig-node-service/src/cli.rs index e4a454cd..e0421baf 100644 --- a/crates/dig-node-service/src/cli.rs +++ b/crates/dig-node-service/src/cli.rs @@ -19,6 +19,7 @@ //! | 4 | SERVICE_FAILED | A service operation failed (register/start/stop).| //! | 5 | BIND_FAILED | `run`: could not bind the loopback address. | //! | 6 | IO_ERROR | Other I/O error. | +//! | 12 | NODE_UNREACHABLE | The node did not answer; nothing was measured. | use serde_json::{json, Value}; @@ -43,6 +44,14 @@ pub enum ExitCode { BindFailed, /// 6 — any other I/O error. IoError, + /// 12 — the node did not answer, so the operation was never measured (dig-node#407). + /// + /// Deliberately NOT `IO_ERROR`: that code says an I/O operation was attempted and failed, + /// which is a claim about the requested operation. An unreachable node is a failure to + /// MEASURE, not a measured failure, and the two must not share an encoding -- the restart + /// window an update opens is exactly when the difference matters, and a caller that cannot + /// tell them apart learns to ignore both. + NodeUnreachable, } impl ExitCode { @@ -56,6 +65,7 @@ impl ExitCode { ExitCode::ServiceFailed => 4, ExitCode::BindFailed => 5, ExitCode::IoError => 6, + ExitCode::NodeUnreachable => 12, } } @@ -69,6 +79,7 @@ impl ExitCode { ExitCode::ServiceFailed => "SERVICE_FAILED", ExitCode::BindFailed => "BIND_FAILED", ExitCode::IoError => "IO_ERROR", + ExitCode::NodeUnreachable => "NODE_UNREACHABLE", } } @@ -86,6 +97,9 @@ impl ExitCode { } ExitCode::BindFailed => "run: could not bind the loopback address.", ExitCode::IoError => "Other I/O error.", + ExitCode::NodeUnreachable => { + "The node did not answer; the operation was not measured (it may be restarting)." + } } } @@ -99,6 +113,9 @@ impl ExitCode { // A bad argument surfaced as `InvalidInput` (e.g. `dig-node open` rejecting a // non-DIG/malformed link) is a USAGE error, not a generic I/O failure. InvalidInput => ExitCode::Usage, + // The node did not answer. Nothing was measured about the requested operation, so + // this must not be reported as an I/O failure OF that operation (#407). + ConnectionRefused => ExitCode::NodeUnreachable, _ => ExitCode::IoError, } } @@ -113,6 +130,7 @@ impl ExitCode { ExitCode::ServiceFailed, ExitCode::BindFailed, ExitCode::IoError, + ExitCode::NodeUnreachable, ] } } @@ -179,6 +197,86 @@ pub fn error_envelope(action: &str, exit: ExitCode, message: &str, hint: Option< mod tests { use super::*; + /// #407 -- an unreachable node must not be reported as an I/O failure OF the request. + /// + /// The two arms are asserted TOGETHER because the property is a distinction, not an outcome: + /// a "fix" that renamed code 6, or that mapped every failure to the new code, satisfies + /// either arm alone. `Other` is the genuine-I/O-failure control and must stay `IO_ERROR`. + #[test] + fn an_unreachable_node_is_distinguished_from_a_measured_io_failure() { + let unreachable = + std::io::Error::new(std::io::ErrorKind::ConnectionRefused, "node not answering"); + let measured = std::io::Error::other("the disk gave up mid-write"); + + assert_eq!( + ExitCode::from_io_error(&unreachable), + ExitCode::NodeUnreachable + ); + assert_eq!(ExitCode::from_io_error(&measured), ExitCode::IoError); + assert_ne!( + ExitCode::from_io_error(&unreachable).code(), + ExitCode::from_io_error(&measured).code(), + "a failure to measure and a measured failure must not share an exit code" + ); + } + + /// **No `dign` code may collide with a `diga` code that means something else.** + /// + /// The two command lines deliberately share ONE numbering so a caller sees one surface + /// (`dig-app-core/src/gateway/outcome.rs` says so in its own doc comment), which means a + /// number is free only if it is unoccupied ECOSYSTEM-WIDE. `NODE_UNREACHABLE` was first + /// assigned 7 by reading this file's own table, where 7 genuinely was the next number -- + /// and 7 is `NOT_CONNECTED` on the other side. The identical reasoning already cost a yank + /// in the JSON-RPC error space (`-32015` vs a released `METADATA_TOO_LARGE`), so the guard + /// is a test rather than a note. + /// + /// The `diga` map is transcribed rather than imported: dig-node MUST NOT take a dependency + /// on dig-app (it is the engine, not a consumer of its own client). That makes this fixture + /// the drift risk, so it names the file it was read from and the SPEC carries the same table. + #[test] + fn no_exit_code_collides_with_the_dig_app_gateway_numbering() { + // Read from modules/apps/dig-app/crates/dig-app-core/src/gateway/outcome.rs. + const DIGA: &[(u8, &str)] = &[ + (0, "OK"), + (2, "USAGE"), + (6, "IO_ERROR"), + (7, "NOT_CONNECTED"), + (8, "ENGINE_ERROR"), + (9, "LOCKED"), + (10, "NOT_FOUND"), + (11, "DENIED"), + ]; + + for code in ExitCode::all() { + if let Some((_, diga_name)) = DIGA.iter().find(|(n, _)| *n == code.code()) { + assert_eq!( + code.name(), + *diga_name, + "exit {} is `{}` here and `{}` in the dig-app gateway -- a shared number must carry the SAME meaning on both command lines, or a caller branching on it is reading two different failures as one", + code.code(), + code.name(), + diga_name + ); + } + } + } + + /// The catalogue is the machine-readable contract (§6.2), and a code missing from it is + /// invisible to every consumer that enumerates rather than guesses. + #[test] + fn the_new_code_is_in_the_catalogue_with_a_stable_name_and_number() { + assert!(ExitCode::all().contains(&ExitCode::NodeUnreachable)); + assert_eq!(ExitCode::NodeUnreachable.code(), 12); + assert_eq!(ExitCode::NodeUnreachable.name(), "NODE_UNREACHABLE"); + // Every code's number is distinct -- an added arm that reused 6 would read as success + // against the two assertions above if either were relaxed. + let mut codes: Vec = ExitCode::all().iter().map(|c| c.code()).collect(); + codes.sort_unstable(); + let before = codes.len(); + codes.dedup(); + assert_eq!(codes.len(), before, "two exit codes share a number"); + } + #[test] fn exit_codes_are_unique_and_upper_snake() { let mut codes = std::collections::HashSet::new(); diff --git a/crates/dig-node-service/src/control.rs b/crates/dig-node-service/src/control.rs index e0d60b63..95c673df 100644 --- a/crates/dig-node-service/src/control.rs +++ b/crates/dig-node-service/src/control.rs @@ -429,11 +429,9 @@ pub fn control_token_remedy_for(path: &Path) -> String { Ok(_) => format!( "the presented control token was not accepted. Ensure the node and this command resolve the SAME state dir ({dir}) — if you set DIG_NODE_STATE_DIR it must match on both the node and this command." ), - Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => format!( - "the node's control token at {} exists but is NOT readable by your account — the node runs as a service under a different account (Windows LocalSystem / a root daemon). Re-run this command elevated (Administrator on Windows, sudo on Unix), or reinstall the current dig-node so the service grants your account read access to {} (`dig-node uninstall` then an elevated `dig-node install`, then `dig-node start`).", - path.display(), - dir - ), + Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { + permission_denied_remedy(path, &dir) + } // Absent (NotFound) — the node has not minted one here yet. Either it is not running, // or a STALE older build (installed before the machine-wide state dir) is running and // never mints the token at this path; reinstalling the current dig-node fixes the latter. @@ -444,6 +442,56 @@ pub fn control_token_remedy_for(path: &Path) -> String { } } +/// The remedy for a control token that EXISTS but this account cannot read (dig-node#403). +/// +/// The two platforms genuinely differ, and a single sentence covering both was false on one of +/// them. On Windows the installer establishes an ACL that keeps an explicit read grant for the +/// interactive install-user, so "reinstall and the service will grant your account read access" +/// is accurate there. On Unix the `.deb` creates `/var/lib/dig-node` as `0700 root:root` with the +/// token `0600 root:root`, and reinstalling changes NOTHING — an operator following that advice +/// loops through an uninstall/install cycle and arrives back at the same denial. +/// +/// # Why the fix is not to loosen the mode +/// +/// The natural server shape — node as a system service, app as a user Agent — cannot read this +/// file, and the tempting repairs both widen a privilege boundary. This token is the MASTER +/// capability: it authorizes pairing administration (mint, list, revoke) and `chiaPeers.add`, +/// which grants chain authority over the node's wallet replica. A `dig` group at `0640` hands +/// that, permanently and to every future member, in exchange for a convenience; and it outlives +/// the app that motivated it. +/// +/// The node already has the right primitive for a client that cannot read a file: PAIRING +/// ([`crate::pairing`]), built for exactly this and currently used by the MV3 extension. It +/// yields a SCOPED, REVOCABLE per-client token that cannot mint or revoke pairings and cannot +/// grant chain authority, gated on a local approval the operator performs once with the master +/// token they already hold. The principal admitted is one approved client on this host, with +/// mutation rights minus the master tier — strictly less than a group grant, and revocable +/// without touching a file mode. +fn permission_denied_remedy(path: &Path, dir: &str) -> String { + remedy_for_unreadable_token(path, dir, cfg!(unix)) +} + +/// The pure core of [`permission_denied_remedy`], with the platform as an ARGUMENT. +/// +/// A `cfg!(unix)` branch is only ever exercised on the half of the fleet that compiles it, so the +/// sentence shown to Ubuntu operators would be untested on the machine most likely to be running +/// these tests. Passing the platform in makes both branches assertable everywhere. +fn remedy_for_unreadable_token(path: &Path, dir: &str, unix: bool) -> String { + let elevated = format!( + "the node's control token at {} exists but is NOT readable by your account — the node runs as a service under a different account (Windows LocalSystem / a root daemon). Re-run this command elevated (Administrator on Windows, sudo on Unix)", + path.display() + ); + if unix { + format!( + "{elevated}. For a program that must keep running as an ordinary user (the dig-app Agent on a server), do NOT widen the mode on this file — it is the master capability. Pair a scoped, revocable token for that client instead: `sudo dign pair` LISTS the pending requests and `sudo dign pair approve ` approves one -- the bare verb only lists, it approves nothing -- and the token the client receives cannot mint or revoke pairings and cannot grant chain authority. Revoke it any time with `sudo dign pair revoke `." + ) + } else { + format!( + "{elevated}, or reinstall the current dig-node so the service grants your account read access to {dir} (`dig-node uninstall` then an elevated `dig-node install`, then `dig-node start`)." + ) + } +} + /// Read the master control token WITHOUT creating one — the OPERATOR-side load (`dig-node /// pair` / any local control CLI, #501). It must NEVER mint a token: minting a fresh token /// the running node does not trust is the exact original bug (the CLI wrote its own token to @@ -5717,6 +5765,57 @@ mod tests { /// The remedy hint names the concrete token path and, when the token is absent from /// the caller's perspective, tells them to start the node — never the old generic /// "" wording. + #[test] + /// dig-node#403 -- the unreadable-token remedy must not promise a grant the platform never + /// performs. On Unix the `.deb` leaves the token 0600 root:root and reinstalling changes + /// nothing, so the reinstall clause sent an operator round a loop that cannot succeed. + /// + /// Asserted BOTH ways rather than only on the presence of the new advice: a remedy that + /// appended the pairing sentence while keeping the false reinstall clause reads as fixed and + /// still contains the dead end. + fn the_unreadable_token_remedy_offers_a_scoped_credential_not_a_wider_file() { + let path = Path::new("/var/lib/dig-node/control-token"); + let unix = remedy_for_unreadable_token(path, "/var/lib/dig-node", true); + let windows = remedy_for_unreadable_token(path, "/var/lib/dig-node", false); + + assert!(unix.contains("elevated"), "{unix}"); + // The APPROVING verb, in full. `contains("dign pair")` is satisfied by every WRONG + // string as its own substring -- including the one this row was rewritten to catch, + // which claimed a bare `sudo dign pair` "approves the client's pending request" while + // `entrypoint.rs` maps a bare `pair` to `PairAction::List`. A remedy that names a verb + // which does not approve is the same dead end the reinstall clause was. + assert!( + unix.contains("`sudo dign pair approve `"), + "the remedy must name the verb that actually approves: {unix}" + ); + // Fails if the verb reverts. The row above passes on any string containing the correct + // command, including one that ALSO reasserts the false claim beside it; this one is + // one-sided against the specific wrong attribution, so the two cannot both be vacuous. + assert!( + !unix.contains("`sudo dign pair` approves"), + "the bare verb only lists; attributing approval to it is the dead end: {unix}" + ); + assert!( + unix.contains("revoke"), + "a grant with no stated revocation is a permanent one: {unix}" + ); + assert!( + !unix.contains("uninstall"), + "reinstalling does not grant read access on Unix; advising it is the dead end: {unix}" + ); + + // Windows genuinely DOES keep an explicit read grant for the interactive install-user, + // so its reinstall clause is accurate and must survive. This is the truthful control: a + // fix that simply deleted the clause everywhere would pass the Unix assertions alone. + assert!(windows.contains("uninstall"), "{windows}"); + + // Never, on either platform, advise widening the mode of the master capability. + for r in [&unix, &windows] { + assert!(!r.contains("chmod"), "{r}"); + assert!(!r.contains("0640"), "{r}"); + } + } + #[test] fn control_token_remedy_names_a_concrete_path() { let remedy = control_token_remedy(); diff --git a/crates/dig-node-service/src/control_cli.rs b/crates/dig-node-service/src/control_cli.rs index 5d18ecd7..3e3ddb41 100644 --- a/crates/dig-node-service/src/control_cli.rs +++ b/crates/dig-node-service/src/control_cli.rs @@ -400,10 +400,37 @@ impl ControlAction { /// `--json`). Transport / node errors surface as `io::Error` for the differentiated exit code. pub fn run(config: &Config, action: ControlAction) -> std::io::Result { let method = action.method(); - let result = call_control(config, method, action.wire_params())?; + let result = call_control(config, method, action.wire_params()) + .map_err(|e| explain_unreachable(method, e))?; Ok(Outcome::new(summarize(method, &result), result)) } +/// Restate an unreachable-node failure as what it is: the operation was never measured (#407). +/// +/// Only the `ConnectionRefused` class is touched, and only its MESSAGE -- the kind is preserved +/// so [`crate::cli::ExitCode::from_io_error`] still resolves it to `NODE_UNREACHABLE`. Any other +/// error passes through untouched, because a node that answered and refused has measured +/// something and its own words are the accurate ones. +/// +/// `control.updater.*` gets a sharper sentence because it has a specific, EXPECTED cause. A +/// successful update installs new bytes and cycles the service, so the pass that just succeeded +/// is itself why the node stopped answering. Reporting that as a failure is the cry-wolf case +/// the epic's silent-staged-install policy cannot afford: under a policy where nothing blocks and +/// nothing asks, the status surface is all an operator has. +fn explain_unreachable(method: &str, e: std::io::Error) -> std::io::Error { + if e.kind() != std::io::ErrorKind::ConnectionRefused { + return e; + } + let context = if method.starts_with("control.updater.") { + " — the update pass may have completed and restarted the node, which is the normal end \ + of a successful update. This did NOT observe a failed update; it observed nothing. \ + Re-run once the service is back." + } else { + " — nothing was measured about this request; it never reached the node." + }; + std::io::Error::new(e.kind(), format!("{e}{context}")) +} + /// Every `control.*` method reachable from a `dig-node` CLI verb — the union of the /// control-parity actions here and the `control.pairing.*` methods `dig-node pair` drives /// (#280). The drift test asserts this COVERS [`crate::control::CONTROL_METHODS`], so a new @@ -1495,6 +1522,60 @@ fn render_record(record: &crate::collateral::StoredRecord) -> String { #[cfg(test)] mod tests { + + /// #407 -- `dign updater check-now` probing inside the restart window reported IO_ERROR, + /// which reads to an operator exactly like an update that broke the node. It is the opposite: + /// a successful pass installs new bytes and cycles the service, so the restart is the normal + /// END of the thing that succeeded. + /// + /// The fixture varies the ERROR KIND against a fixed method, and the method against a fixed + /// kind, because the nearest wrong implementation is "treat every updater failure as a + /// restart" -- which would swallow a real decline and passes any assertion that only checks + /// the happy restart case. + #[test] + fn an_unreachable_updater_probe_says_it_measured_nothing() { + let e = std::io::Error::new(std::io::ErrorKind::ConnectionRefused, "could not reach"); + let out = explain_unreachable("control.updater.checkNow", e); + + assert_eq!( + out.kind(), + std::io::ErrorKind::ConnectionRefused, + "the kind carries the exit code and must survive" + ); + let msg = out.to_string(); + assert!(msg.contains("restarted the node"), "{msg}"); + assert!(msg.contains("observed nothing"), "{msg}"); + } + + /// The control that makes the test above load-bearing: a node that ANSWERED and declined has + /// measured something, and its own words are the accurate ones. They must pass through + /// untouched -- no restart story bolted onto a real failure. + #[test] + fn a_genuine_decline_is_not_reframed_as_a_restart() { + let e = + std::io::Error::other("dig-node: dig-updater declined the request: no such channel"); + let out = explain_unreachable("control.updater.checkNow", e); + + assert_eq!(out.kind(), std::io::ErrorKind::Other); + let msg = out.to_string(); + assert!(msg.contains("declined the request"), "{msg}"); + assert!( + !msg.contains("restarted the node"), + "a measured failure must not be excused: {msg}" + ); + } + + /// A non-updater verb hitting the same unreachable node gets the general statement, not the + /// update story -- there is no update pass to attribute the silence to. + #[test] + fn a_non_updater_verb_gets_the_general_unreachable_statement() { + let e = std::io::Error::new(std::io::ErrorKind::ConnectionRefused, "could not reach"); + let out = explain_unreachable("control.cache.get", e); + + let msg = out.to_string(); + assert!(msg.contains("never reached the node"), "{msg}"); + assert!(!msg.contains("update pass"), "{msg}"); + } use super::*; use crate::control::CONTROL_METHODS; diff --git a/crates/dig-node-service/src/control_client.rs b/crates/dig-node-service/src/control_client.rs index e4e4eede..c107a142 100644 --- a/crates/dig-node-service/src/control_client.rs +++ b/crates/dig-node-service/src/control_client.rs @@ -47,36 +47,51 @@ pub fn call_control(config: &Config, method: &str, params: Value) -> std::io::Re let rt = tokio::runtime::Builder::new_current_thread() .enable_all() .build()?; - rt.block_on(call_async(addr, &token, method, params)) + rt.block_on(call_async(addr, Some(&token), method, params)) } -/// POST one JSON-RPC control method with the master token; return its `result` (or `{}` when -/// the node omits one). A transport failure = the node isn't running; a JSON-RPC `error` = -/// the node rejected the call (e.g. a method it does not implement → METHOD_NOT_FOUND). +/// Call one OPEN (non-`control.*`) node method and return its `result` object -- same transport, +/// same direct-to-loopback pinning, NO control token. +/// +/// The loopback server gates the `control.*` plane fail-closed and leaves the ordinary `dig.*` +/// read surface open, so presenting a token here would not change what the node answers. It would +/// change who can ASK: on a `.deb` install the master token is `0600 root:root` (#501), so +/// routing an open read through [`call_control`] makes it fail with an elevation remedy for a +/// question the node answers to any peer that dials it. Use this for reads the node already +/// publishes, and [`call_control`] for everything that is genuinely privileged. +pub fn call_open(config: &Config, method: &str, params: Value) -> std::io::Result { + let addr = config.bind_addr(); + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build()?; + rt.block_on(call_async(addr, None, method, params)) +} + +/// POST one JSON-RPC method, with the master token when one is supplied; return its `result` +/// (or `{}` when the node omits one). A transport failure = the node isn't running; a JSON-RPC +/// `error` = the node rejected the call (e.g. a method it does not implement → METHOD_NOT_FOUND). async fn call_async( addr: std::net::SocketAddr, - token: &str, + token: Option<&str>, method: &str, params: Value, ) -> std::io::Result { let client = build_control_client().map_err(std::io::Error::other)?; let url = format!("http://{addr}/"); let body = json!({ "jsonrpc": "2.0", "id": 1, "method": method, "params": params }); - let resp = client - .post(&url) - .header(control::CONTROL_TOKEN_HEADER, token) - .json(&body) - .send() - .await - .map_err(|e| { - std::io::Error::new( - std::io::ErrorKind::ConnectionRefused, - format!( - "could not reach the dig-node at {url}: {e} — is it running? \ + let mut req = client.post(&url); + if let Some(token) = token { + req = req.header(control::CONTROL_TOKEN_HEADER, token); + } + let resp = req.json(&body).send().await.map_err(|e| { + std::io::Error::new( + std::io::ErrorKind::ConnectionRefused, + format!( + "could not reach the dig-node at {url}: {e} — is it running? \ Start it with `dig-node run` (or `dig-node start` for the service)." - ), - ) - })?; + ), + ) + })?; let v: Value = resp.json().await.map_err(std::io::Error::other)?; if let Some(err) = v.get("error") { let msg = err diff --git a/crates/dig-node-service/src/entrypoint.rs b/crates/dig-node-service/src/entrypoint.rs index 10dd6191..5a25e791 100644 --- a/crates/dig-node-service/src/entrypoint.rs +++ b/crates/dig-node-service/src/entrypoint.rs @@ -13,6 +13,7 @@ //! start Start the installed service. //! stop Stop the running service. //! status Report whether the node is serving (probes /health). +//! network-info This node's own network posture (peer id, addresses, reachability). //! //! With no subcommand, the binary runs in the foreground (equivalent to `run`), so a //! bare invocation just serves — the least-surprise default for a localhost endpoint. @@ -35,6 +36,7 @@ use clap::{CommandFactory, FromArgMatches, Parser, Subcommand}; use crate::cli::{error_envelope, success_envelope, ExitCode, Outcome}; use crate::config::Config; use crate::control_cli::{self, ControlAction}; +use crate::network_info; use crate::open; use crate::pair::{self, PairAction}; use crate::peers::{self, BanState, PeersAction}; @@ -185,6 +187,13 @@ enum Command { #[command(subcommand)] action: Option, }, + /// This node's own network posture: peer id, network + genesis, advertised addresses + /// (IPv6-first, §5.2), reachability and relay reservation. + /// + /// Answers the question `peers` cannot: `peers` describes who this node is TALKING to, this + /// describes how this node is REACHABLE. Reads the node's open `dig.getNetworkInfo` surface, + /// so it needs no control token — every field here is already published to any peer. + NetworkInfo, /// View + manage the node's peer connections — parity with the extension's peer surface. /// With no sub-action, lists the live peer status (running flag, connected count, relay, and — /// on a newer node — the per-peer list with addresses shown IPv6-first per §5.2). @@ -750,6 +759,7 @@ impl Command { Command::Spends { .. } => "spends", Command::Updater { .. } => "updater", Command::Subscriptions { .. } => "subscriptions", + Command::NetworkInfo => "network-info", Command::Peers { .. } => "peers", Command::Collateral { .. } => "collateral", Command::Mirror { .. } => "mirror", @@ -904,6 +914,7 @@ pub fn run() -> std::process::ExitCode { action, json, ), + Command::NetworkInfo => render(network_info::run(&config), action, json), Command::Peers { action: cmd } => match peers_action(cmd) { Ok(a) => render(peers::run(&config, a), action, json), Err(e) => emit_error(&e, action, json), @@ -1407,6 +1418,41 @@ fn run_service(config: Config) -> std::io::Result<()> { mod tests { use super::*; + /// **The bare `pair` verb LISTS; only `pair approve ` approves.** + /// + /// This exists because a remedy string in `control.rs` told an operator that `sudo dign pair` + /// "approves the client's pending request". It does not, and nothing could see the mistake: + /// the remedy's own test asserted `contains("dign pair")`, which the wrong sentence satisfies + /// as its own substring. Pinning the mapping HERE means the claim is measured against the + /// parser rather than against a phrase, so the two cannot drift apart silently. + #[test] + fn the_bare_pair_verb_lists_and_only_approve_approves() { + let pair_action = |argv: &[&str]| match Cli::try_parse_from(argv) + .expect("the verb parses") + .command + .expect("a subcommand was given") + { + Command::Pair { action } => match action { + None | Some(PairCommand::List) => PairAction::List, + Some(PairCommand::Approve { pairing_id }) => PairAction::Approve { pairing_id }, + Some(PairCommand::Revoke { token_id }) => PairAction::Revoke { token_id }, + }, + _ => panic!("expected a pair command from {argv:?}"), + }; + + assert!( + matches!(pair_action(&["dig-node", "pair"]), PairAction::List), + "a bare `pair` must remain a LIST -- the remedy text depends on it" + ); + assert!( + matches!( + pair_action(&["dig-node", "pair", "approve", "abc123"]), + PairAction::Approve { ref pairing_id } if pairing_id == "abc123" + ), + "`pair approve ` must approve, and must carry the id through" + ); + } + /// **`dign mirror bond-states --after` sends the cursor to the node.** /// /// Asserted on the WIRE params rather than on the selected method, because a parser that diff --git a/crates/dig-node-service/src/lib.rs b/crates/dig-node-service/src/lib.rs index 60850b62..ba78a5cc 100644 --- a/crates/dig-node-service/src/lib.rs +++ b/crates/dig-node-service/src/lib.rs @@ -78,6 +78,10 @@ pub mod meta; /// `(store, root, epoch)`, and its disappearance drives reclaim of the collateral. The node signs /// those spends itself with its own operator wallet, scoped by construction. See [`mirror`]. pub mod mirror; +/// `dign network-info` (#303): this node's OWN network posture -- peer id, network + genesis, +/// advertised addresses (IPv6-first, §5.2), reachability and relay reservation. Reads the node's +/// OPEN `dig.getNetworkInfo` surface, so it needs no control token. See [`network_info`]. +pub mod network_info; /// `dig-node open ` (#389): the OS scheme-handler target the /// installer registers for `chia://` + `urn:dig:chia:`. Strictly validates the untrusted /// handler argument, then opens the user's default browser at the resolving URL. See [`open`]. diff --git a/crates/dig-node-service/src/logging.rs b/crates/dig-node-service/src/logging.rs index 75989af3..f4420440 100644 --- a/crates/dig-node-service/src/logging.rs +++ b/crates/dig-node-service/src/logging.rs @@ -60,6 +60,36 @@ pub fn run_context() -> RunContext { } } +/// The sentence printed when the rolling log FILE could not be opened and this run is +/// console-only. +/// +/// A named constant rather than an inline literal for two reasons. It is the only part of the +/// degrade path a test can hold onto -- `init` installs a global subscriber, so the emission +/// itself is not assertable in-process. And it was shipped carrying two runs of ~22 literal +/// spaces from a lost string continuation, which no test could see and which renders in an +/// operator's log as a line that looks corrupted at the exact moment they are trying to work out +/// whether logging is broken. +/// +/// `concat!` rather than a `\`-continued literal, and that is not a style preference: the first +/// repair here DID use continuations, and `cargo fmt` rejoined them and materialised the leading +/// indentation back into the string -- reintroducing the very defect, silently, between writing +/// the fix and running it. `the_announcement_text_has_no_lost_string_continuation` caught it. +/// `concat!` has no whitespace a formatter can reinterpret. +pub const FILE_LOGGING_DEGRADED: &str = concat!( + "the rolling log FILE could not be opened; this run logs to the console ONLY. ", + "Nothing is being written to that directory -- an empty log directory here means ", + "logging was denied, not that the node was quiet." +); + +/// Whether a run must ANNOUNCE that file logging degraded, given the sink's error (if any). +/// +/// Pure, and separated from [`init`] for the same reason `remedy_for_unreadable_token` takes its +/// platform as an argument: the branch that matters runs on the half of the fleet where the log +/// directory is denied, which is not the machine the tests run on. +pub fn degrade_announcement(file_error: Option<&str>) -> Option<&str> { + file_error +} + /// Install the shared logging stack for a SERVE run (SPEC §1) and hold the guard for the /// process lifetime. Idempotent + best-effort: a second call (e.g. a test that serves twice /// in one process) is a silent no-op. @@ -82,6 +112,19 @@ pub fn init(run_context: RunContext) { } match dig_logging::init(service(run_context)) { Ok(guard) => { + // ANNOUNCE a degrade, do not merely record it (dig-node#392). `control.status` has + // carried `file_error` since the 0.2.0 uplift, but nothing was said at the moment it + // happened -- so on a non-admin run denied `C:\ProgramData\DigNetwork\logs` the node + // came up console-only in silence, and an operator later found an empty log directory + // with no way to tell "nothing went wrong" from "logging never started". The console + // layer IS installed on this path, which is precisely why the warning reaches someone. + if let Some(reason) = degrade_announcement(guard.file_error()) { + tracing::warn!( + dir = %guard.log_dir().display(), + reason = %reason, + "{FILE_LOGGING_DEGRADED}" + ); + } // A `set` race (two serve paths initialising at once) is benign: the first guard // wins and stays live; a losing guard is dropped, which only detaches a writer // that was never wired into the global subscriber. @@ -184,6 +227,58 @@ mod tests { assert!(set_level("debug").is_err()); } + /// **A live file sink announces NOTHING.** The control, and the row that makes the next one + /// load-bearing: an implementation that warned unconditionally would satisfy the announce + /// assertion below while shouting on every healthy start. + #[test] + fn a_live_file_sink_announces_no_degrade() { + assert_eq!(degrade_announcement(None), None); + } + + /// **A failed file sink announces, and carries the sink's own reason through.** + /// + /// The reason is asserted by VALUE, not merely by presence: a warning that says logging + /// degraded without saying why sends an operator to guess between a denied directory, a full + /// disk and a bad path -- three different remedies. + #[test] + fn a_failed_file_sink_announces_and_names_the_reason() { + assert_eq!( + degrade_announcement(Some("Access is denied. (os error 5)")), + Some("Access is denied. (os error 5)") + ); + } + + /// **The announcement text carries no run of collapsed whitespace.** + /// + /// This is not style. The message shipped with two runs of ~22 literal spaces, left behind + /// when a Rust string continuation lost its trailing backslash -- the compiler is perfectly + /// happy, every other test stays green, and the only witness is an operator reading a line + /// that looks corrupted at the moment they are trying to establish whether logging works. + #[test] + fn the_announcement_text_has_no_lost_string_continuation() { + assert!( + !FILE_LOGGING_DEGRADED.contains(" "), + "a run of consecutive spaces means a continuation lost its backslash: {FILE_LOGGING_DEGRADED:?}" + ); + } + + /// **The announcement distinguishes "denied" from "quiet".** + /// + /// The whole point of #392: an empty log directory is ambiguous, and the warning exists to + /// resolve the ambiguity rather than to record that something happened. A message that said + /// only "file logging failed" would pass a presence check and leave the ambiguity intact. + #[test] + fn the_announcement_says_an_empty_directory_means_denied_not_quiet() { + assert!( + FILE_LOGGING_DEGRADED.contains("console ONLY"), + "{FILE_LOGGING_DEGRADED}" + ); + assert!( + FILE_LOGGING_DEGRADED.contains("logging was denied, not that the node was quiet"), + "the message must resolve the empty-directory ambiguity: {FILE_LOGGING_DEGRADED}" + ); + } + #[test] fn health_reports_file_logging_off_and_names_the_reason() { // The degraded case the 0.2.0 uplift exists for: the subscriber IS installed (console diff --git a/crates/dig-node-service/src/network_info.rs b/crates/dig-node-service/src/network_info.rs new file mode 100644 index 00000000..384be8b7 --- /dev/null +++ b/crates/dig-node-service/src/network_info.rs @@ -0,0 +1,134 @@ +//! `dign network-info` — this node's own network posture (dig-node#303). +//! +//! The node has always ANSWERED this question: `dig.getNetworkInfo` is served over the loopback +//! JSON-RPC surface and returns the node's `peer_id`, network id, effective L2 genesis, advertised +//! candidate addresses (IPv6-first, §5.2), reachability and relay reservation. What did not exist +//! was a way to ASK it from the command line — `dig-node network-info` was reported as producing +//! empty output on three healthy fleet boxes, which is what an unrecognised subcommand looks like +//! once a shell has swallowed the usage text. `peers` on the same boxes in the same session +//! answered, so the data was there and only the verb was missing. +//! +//! # Why this reads the OPEN surface rather than a `control.*` method +//! +//! Everything here is already published to strangers: the same `dig.getNetworkInfo` body is what +//! this node hands any peer that dials it, so a loopback caller learns nothing a peer does not. +//! Reading it through the token-gated control plane would therefore buy no confidentiality while +//! costing real availability — on a `.deb` install the control token is `0600 root:root` +//! (#501), so an ordinary user asking "what is my node's address" would be told to elevate for a +//! read the network performs for free. It is served token-free for that reason, deliberately, and +//! that is a property to preserve rather than an oversight to tighten later. + +use serde_json::{json, Value}; + +use crate::cli::Outcome; +use crate::config::Config; +use crate::control_client::call_open; + +/// Run `network-info`: read `dig.getNetworkInfo` from the running node and render it. +pub fn run(config: &Config) -> std::io::Result { + let result = call_open(config, "dig.getNetworkInfo", json!({}))?; + Ok(Outcome::new(format_network_info(&result), result)) +} + +/// Render the node's network posture as an operator-friendly block. PURE over the RPC result. +/// +/// Every field is rendered from what the node actually returned: an absent field prints as +/// `unknown` rather than as a plausible default, because a fabricated `direct` or an invented +/// `0.0.0.0` reads exactly like a measurement and would be acted on as one. +fn format_network_info(result: &Value) -> String { + let text = |key: &str| { + result[key] + .as_str() + .map_or_else(|| "unknown".to_string(), str::to_string) + }; + + let mut out = format!( + "dig-node network info:\n peer id {}\n network {}\n genesis {}", + text("peer_id"), + text("network_id"), + text("genesis"), + ); + out.push_str(&format!("\n listen addr {}", text("listen_addr"))); + out.push_str(&format!("\n reachability {}", text("reachability"))); + + // The advertised candidates in the order the node advertises them, which is IPv6-first (§5.2). + // Reordering here would hide a node whose IPv6 advertisement is missing — the exact fault an + // operator runs this command to find — so the order is passed through untouched. + let candidates: Vec<&str> = result["candidate_addresses"] + .as_array() + .map(|a| a.iter().filter_map(Value::as_str).collect()) + .unwrap_or_default(); + if candidates.is_empty() { + out.push_str("\n candidates none advertised (this node is not dialable by peers)"); + } else { + out.push_str("\n candidates:"); + for addr in candidates { + out.push_str(&format!("\n • {addr}")); + } + } + + if let Some(url) = result["relay"]["url"].as_str() { + let reserved = result["relay"]["reserved"].as_bool().unwrap_or(false); + out.push_str(&format!( + "\n relay {url} — reservation {}", + if reserved { "held" } else { "none" } + )); + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The whole posture renders, and the candidate order the node chose survives verbatim. + #[test] + fn renders_posture_and_preserves_the_advertised_candidate_order() { + let s = format_network_info(&json!({ + "peer_id": "aa11", + "network_id": "mainnet", + "genesis": "ccd5bb71183532bff220ba46c268991a00000000000000000000000000000000", + "listen_addr": "[2001:db8::5]:9444", + "reachability": "relayed", + "candidate_addresses": ["[2001:db8::5]:9444", "203.0.113.7:9444"], + "relay": { "url": "wss://relay.dig.net:443", "reserved": true }, + })); + assert!(s.contains("aa11")); + assert!(s.contains("mainnet")); + assert!(s.contains("relayed")); + assert!(s.contains("relay.dig.net")); + assert!(s.contains("reservation held")); + // Two candidates, IPv6 first — asserted by POSITION, so a re-sort is visible here. A + // `contains` on each address alone would pass under any ordering, including one that + // demoted the IPv6 candidate the §5.2 policy exists to keep first. + let v6 = s.find("[2001:db8::5]:9444").unwrap(); + let v4 = s.find("203.0.113.7:9444").unwrap(); + assert!(v6 < v4, "advertised order must pass through untouched: {s}"); + } + + /// A node advertising nothing says so. The tempting alternative — printing the listen address + /// as though it were a candidate — would report a dialable endpoint that no peer was ever + /// offered, which is the failure this verb exists to expose. + #[test] + fn no_candidates_is_stated_not_silently_omitted() { + let s = format_network_info(&json!({ + "peer_id": "bb22", + "candidate_addresses": [], + })); + assert!(s.contains("none advertised"), "{s}"); + assert!(s.contains("not dialable"), "{s}"); + } + + /// A field the node did not send prints as `unknown`. Nobody observed a reachability here, and + /// `direct` — the value a plain `unwrap_or_default` on a string would never produce, but which + /// a hand-written default reaches for — would be a claim about the network drawn from nothing. + #[test] + fn an_absent_field_prints_unknown_rather_than_a_plausible_default() { + let s = format_network_info(&json!({ "peer_id": "cc33" })); + assert!(s.contains("unknown"), "{s}"); + assert!( + !s.contains("direct"), + "a missing reachability must not read as direct: {s}" + ); + } +} diff --git a/crates/dig-node-service/src/peers.rs b/crates/dig-node-service/src/peers.rs index 815ee131..fa473ad7 100644 --- a/crates/dig-node-service/src/peers.rs +++ b/crates/dig-node-service/src/peers.rs @@ -173,15 +173,17 @@ fn format_counts(result: &Value) -> String { /// Render `control.peerStatus` as an operator-friendly summary. Shows the running flag, the /// connected count, and the relay reservation; when a newer node fills the optional per-peer /// list it prints each peer with its addresses ordered IPv6-first (§5.2). +/// +/// An EMPTY per-peer list is reported by cause rather than by symptom (#304): zero connected +/// peers is stated as such, and the version-gap note is kept for the only state that actually +/// implies one — peers this node counts but cannot enumerate. fn format_status(result: &Value) -> String { let running = result["running"].as_bool().unwrap_or(false); if !running { return "dig-node: peer network is not running (no connected peers).".to_string(); } - let mut out = format!( - "dig-node peer network: running · {} connected peer(s)", - result["connected_peers"].as_u64().unwrap_or(0), - ); + let connected = result["connected_peers"].as_u64().unwrap_or(0); + let mut out = format!("dig-node peer network: running · {connected} connected peer(s)"); if let Some(url) = result["relay"]["url"].as_str() { let reserved = result["relay"]["reserved"].as_bool().unwrap_or(false); out.push_str(&format!( @@ -191,10 +193,23 @@ fn format_status(result: &Value) -> String { } let mut peers = result["peers"].as_array().cloned().unwrap_or_default(); if peers.is_empty() { - // Honest degradation: no per-peer list (no peer network running, or an older node build). - out.push_str( - "\n (this node build reports a count only — a per-peer list needs a newer node)", - ); + // An empty list has TWO causes and they need different words (#304). Conflating them + // told a first-run user on the newest node that exists to go and upgrade -- advice that + // is false, and false in the direction that sends them looking for a release that does + // not exist. The connected count distinguishes the two: a node reporting zero connected + // peers has an empty list because it HAS no peers, while a node reporting peers it + // cannot enumerate is the genuine version gap the note was written for. + if connected == 0 { + out.push_str( + "\n no peers connected yet — a node finds peers within a minute or two of \ + starting. If it stays at zero, check outbound connectivity on the peer port \ + and try `dign peers connect
` with a known peer.", + ); + } else { + out.push_str( + "\n (this node build reports a count only — a per-peer list needs a newer node)", + ); + } } else { // Render peers with IPv6-addressed ones first (§5.2 display policy). peers.sort_by_key(|p| is_ipv4(p["address"].as_str().unwrap_or(""))); @@ -253,6 +268,42 @@ mod tests { assert!(s.contains("newer node")); } + /// #304 -- a node with zero peers must not be told to upgrade. Measured on 0.138.0, the + /// newest build that existed, where zero peers was the correct state: the CLI printed "a + /// per-peer list needs a newer node", sending a first-run user after a release that does not + /// exist. The fixture varies ONLY the connected count against the test above, which keeps a + /// truthful control: 4 connected peers with no list IS a version gap and still says so, so a + /// fix that merely reworded the string unconditionally fails there rather than here. + #[test] + fn zero_peers_says_so_instead_of_blaming_the_node_version() { + let s = format_status(&json!({ + "running": true, + "connected_peers": 0, + "relay": { "url": "wss://relay.dig.net:443", "reserved": false }, + })); + assert!( + !s.contains("newer node"), + "zero peers is not a version gap: {s}" + ); + assert!(s.contains("no peers connected"), "{s}"); + // Actionable, not merely honest: the message must name something to try. + assert!(s.contains("peers connect"), "{s}"); + } + + /// The same state reported by a node that DOES fill the list -- an explicitly empty array + /// rather than an absent key. It is still zero peers, and must read identically; the two + /// spellings of "no peers" must not diverge into two different messages. + #[test] + fn an_explicitly_empty_peer_array_reads_the_same_as_an_absent_one() { + let s = format_status(&json!({ + "running": true, + "connected_peers": 0, + "peers": [], + })); + assert!(!s.contains("newer node"), "{s}"); + assert!(s.contains("no peers connected"), "{s}"); + } + #[test] fn peer_line_shows_id_via_direction_and_address() { let peer = json!({ diff --git a/packaging/linux/build-deb.sh b/packaging/linux/build-deb.sh index e7a81d3c..05d03330 100755 --- a/packaging/linux/build-deb.sh +++ b/packaging/linux/build-deb.sh @@ -73,8 +73,8 @@ printf '/etc/dig-node/dig-node.env\n' > "$STAGE/DEBIAN/conffiles" # --- maintainer scripts ----------------------------------------------------- # postinst: pre-create the restrictive machine-wide state dir (#501 — root-owned 0700 so -# the control token is not world-readable), enable+start the service, register the scheme -# handler as the system default. +# the control token is not world-readable), enable+start the service, cycle it on upgrade +# (#305), and register the scheme handler as the system default. cat > "$STAGE/DEBIAN/postinst" <<'EOF' #!/bin/sh set -e @@ -127,6 +127,25 @@ case "$1" in else systemctl enable --now net.dignetwork.dig-node.service || true fi + # dig-node#305 — an UPGRADE must cycle the unit, or the old process keeps serving. + # + # `enable --now` above is a no-op on a unit that is already enabled and running, so before + # this the upgrade replaced /usr/bin/dig-node and left the OLD binary executing. That is + # silent in both directions an operator would check: `dig-node --version` reads the on-disk + # image and reports the NEW version immediately, and `systemctl is-active` reports active + # because the old process is genuinely healthy. Only MainPID moves, and nobody reads it. A + # security fix shipped through the .deb therefore did not take effect on upgrade. + # + # dpkg passes the previously-configured version as $2 only on an upgrade, so this is scoped + # to the case that needs it: on a first install `enable --now` has already started the new + # binary and a restart would be redundant churn. + # + # `try-restart` rather than `restart`: it cycles the unit ONLY if it is already running, so + # a node the operator deliberately stopped — including one held back by the #317 + # no-autostart marker — stays stopped across every future upgrade. + if [ -n "${2:-}" ]; then + systemctl try-restart net.dignetwork.dig-node.service || true + fi fi # Register the chia:// handler as the system default + refresh the desktop DB. if command -v update-desktop-database >/dev/null 2>&1; then diff --git a/scripts/tests/deb-contents.test.sh b/scripts/tests/deb-contents.test.sh index 147d0c8d..730cba9b 100644 --- a/scripts/tests/deb-contents.test.sh +++ b/scripts/tests/deb-contents.test.sh @@ -99,13 +99,18 @@ echo "systemctl $*" >> "$SYSTEMCTL_LOG" STUB chmod 0755 "$TMP/stub/systemctl" -# run_postinst -> echoes the recorded systemctl invocations +# run_postinst [old-version] -> echoes the recorded systemctl invocations +# +# dpkg passes the PREVIOUSLY-CONFIGURED version as $2 on an upgrade and nothing at all on a +# first install. That argument is the only thing distinguishing the two runs, so it is a +# parameter here rather than being fixed to the install case. run_postinst() { local root="$1" + local old_version="${2:-}" export SYSTEMCTL_LOG="$root/systemctl.log" : > "$SYSTEMCTL_LOG" mkdir -p "$root/etc" "$root/var/lib" "$root/usr/share/applications" - PATH="$TMP/stub:$PATH" DIG_NODE_PKG_ROOT="$root" sh "$TMP/ctl/postinst" configure \ + PATH="$TMP/stub:$PATH" DIG_NODE_PKG_ROOT="$root" sh "$TMP/ctl/postinst" configure $old_version \ >"$root/postinst.out" 2>&1 cat "$SYSTEMCTL_LOG" } @@ -151,6 +156,81 @@ else fail "the marker suppressed daemon-reload — the unit is not registered and cannot be started later" fi +# --- 3. dig-node#305: an UPGRADE must cycle the running service ---------------------------- +# `systemctl enable --now` is a no-op on a unit that is already enabled and running, so the +# upgrade path replaced the binary on disk and left the OLD process serving. Both checks a +# person would naturally run reported success: `dig-node --version` reads the ON-DISK binary, +# and `systemctl is-active` reports the genuinely-healthy OLD process. Only MainPID showed it. +# +# The fixture is an UPGRADE (`configure `) with a truthful control beside it (the +# first-install run above, `configure` with no old version). One argument separates them, and +# it is the only argument dpkg varies -- so a "fix" that restarts unconditionally passes the +# upgrade assertion and is caught by the control at the end of this section. +UPGRADE_ROOT="$TMP/upgrade"; mkdir -p "$UPGRADE_ROOT" +UPGRADE_LOG="$(run_postinst "$UPGRADE_ROOT" "0.1.0")" +if grep -qE 'systemctl (try-restart|try-reload-or-restart) net\.dignetwork\.dig-node\.service' <<<"$UPGRADE_LOG"; then + ok "an upgrade cycles the service, so the new binary is the one running (#305)" +else + fail "an upgrade does not restart the service -- the old binary keeps running silently (#305)" +fi + +# `try-restart` is load-bearing, not a spelling preference: a plain `restart` would START a node +# the operator had deliberately stopped, and would defeat the #317 no-autostart marker on every +# subsequent upgrade. Asserting the SEMANTICS means a later simplification to `restart` fails. +if grep -qE 'systemctl (restart|start|reload-or-restart) net\.dignetwork\.dig-node\.service' <<<"$UPGRADE_LOG"; then + fail "the upgrade uses an unconditional restart -- it would start a node the operator stopped (#305)" +else + ok "the upgrade restart is conditional on the unit already running (#305)" +fi + +# The #317 marker must survive EVERY future upgrade, not just the install that created it. The +# postinst comment reasons that `try-restart` gives this for free -- it cycles a unit only if it +# is already running, so a node held back by the marker stays stopped -- but reasoning is not a +# test, and the marker+upgrade combination was never exercised by either section above: section 2 +# runs the marker on a FIRST install, and the upgrade run above carries no marker. +# +# What is asserted is what the stub can see: the argv the postinst emits. `try-restart`'s runtime +# conditionality belongs to systemd and cannot be observed here, so the load-bearing claim is that +# an upgrade under the marker emits NO unconditional starter -- a `start`, a `restart`, or an +# `enable --now` would each start a node the operator deliberately stopped, and each is a change +# a later simplification could plausibly make. +MARKED_UPGRADE_ROOT="$TMP/marked-upgrade"; mkdir -p "$MARKED_UPGRADE_ROOT/etc/dig-node" +touch "$MARKED_UPGRADE_ROOT/etc/dig-node/no-autostart" +MARKED_UPGRADE_LOG="$(run_postinst "$MARKED_UPGRADE_ROOT" "0.1.0")" +if grep -qE 'systemctl (start|restart|reload-or-restart|enable) ' <<<"$MARKED_UPGRADE_LOG"; then + fail "an upgrade under the no-autostart marker starts the node the operator stopped (#317/#305)" +else + ok "an upgrade under the no-autostart marker starts nothing (#317 survives upgrades)" +fi + +# ...and it still CYCLES a running unit. The #305 fix must not be lost under the marker: an +# operator who removed the marker and started the node by hand is running a node like any other, +# and the next upgrade must replace its binary. This row and the one above are one-sided in +# opposite directions, so neither can be satisfied by the postinst simply doing nothing. +if grep -qE 'systemctl try-restart net\.dignetwork\.dig-node\.service' <<<"$MARKED_UPGRADE_LOG"; then + ok "the marked upgrade still cycles an already-running unit (#305 preserved under #317)" +else + fail "the marker suppressed try-restart -- an upgrade would leave the old binary serving (#305)" +fi + +# The truthful control for the row above. Its assertion is a NEGATIVE, which an empty log would +# satisfy for the wrong reason -- a postinst that did nothing at all under the marker would look +# identical. `daemon-reload` proves the configure branch really ran. +if grep -q 'daemon-reload' <<<"$MARKED_UPGRADE_LOG"; then + ok "the marked upgrade really ran its configure branch (so the check above measured something)" +else + fail "the marked upgrade emitted no daemon-reload -- the no-start check above proves nothing" +fi + +# The control. A FIRST install already starts the node via `enable --now`, so a restart there +# would be redundant; its ABSENCE is what proves the assertion above measured the upgrade +# argument rather than a restart bolted onto every configure. +if grep -qE 'systemctl [a-z-]*restart' <<<"$CONTROL_LOG"; then + fail "a first install restarts the service -- the upgrade check above proves nothing" +else + ok "a first install does not restart (so the upgrade check is measuring the upgrade)" +fi + # The redirect seam must default to the real root. A postinst that defaulted DIG_NODE_PKG_ROOT to # anything non-empty would pass every check above while installing into a scratch directory on a # real host.