From 531ab2a8b20a53cac4a643b78936caf903a13038 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Mon, 31 Aug 2026 06:37:16 -0700 Subject: [PATCH 1/9] chore(secpeer): open lane for the peer/network-facing security batch Batch: #349 #352 #348 #285 #282 -- five defects where a stranger's input decides something, each fixed by correcting the failure direction. Co-Authored-By: Claude From e0c2bea4159495667a16b5d8ea30c2870aae1a7d Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Mon, 31 Aug 2026 06:55:31 -0700 Subject: [PATCH 2/9] fix(relay,config): fail closed on an unreadable relay endpoint and share one off-token #285: relay endpoint parsing accepted any scheme and turned an unparsable port into None, which relay_socket_addr resolved with .unwrap_or(443) -- so one malformed string made the node silently dial 443 at a host that may not be the one the operator wrote. Replaced with a fail-closed parse transcribed from dig_nat::relay::parse_relay_endpoint (the designated survivor, still private in the published 0.21.0), so adopting the export later is a deletion. #282/#352: the three network-reaching isolation knobs each carried a private off-token predicate reading a different vocabulary, so DIG_PEER_NETWORK=OFF left the peer network running while DIG_RELAY_URL=OFF disabled the relay. One shared is_off_token now serves all three, and DIG_RELAY_URL= means no relay rather than the compiled-in public one. Bootstrap resolution announces which branch it took. Co-Authored-By: Claude --- SPEC.md | 23 +- crates/dig-node-core/src/peer.rs | 115 +++++++-- .../src/seams/dig_peer/bootstrap.rs | 37 ++- .../dig-node-core/src/seams/dig_peer/net.rs | 230 +++++++++++++----- 4 files changed, 329 insertions(+), 76 deletions(-) diff --git a/SPEC.md b/SPEC.md index d1962685..2c6ec79d 100644 --- a/SPEC.md +++ b/SPEC.md @@ -223,8 +223,27 @@ does not own them (except `DIG_NODE_UPSTREAM`, which the shell SETS — see belo | `DIG_WALLET_WC_PROJECT_ID` | initial/default WalletConnect projectId for the wallet host (§16) | *(unset ⇒ none)* | A persisted `wc_project_id` in `config.json` wins over this; a blank persisted value falls through to this env. Blank ⇒ treated as unset. | | `DIG_NODE_MAX_OUTGOING_BYTES_PER_SEC` | outgoing-bandwidth throttle cap, in bytes/second (§17) | `0` (UNLIMITED — opt-in) | Parsed as `u64`; `0`, unparsable, or unset ⇒ unlimited (the throttle is a no-op until an operator configures a cap). Resolved ONCE at node construction. | -The peer-network layer additionally honors `DIG_PEER_NETWORK` (set to a falsy value to disable the L7 -peer network) and `DIG_RELAY_URL` (override or disable the relay), which gate the P2P bring-up, and +### The shared off-token + +`DIG_PEER_NETWORK`, `DIG_RELAY_URL` and `DIG_BOOTSTRAP_PEERS` are the three knobs that decide whether +this node reaches the network at all. All three read ONE off-vocabulary: **`off`, `disabled`, `0`, +`false`, `no`, or an explicitly empty value** — trimmed and case-insensitive. Any of those disables +the knob; anything else does not. + +A node MUST NOT accept a disable token on one of these knobs and ignore the same token on another. +An operator who writes `OFF` and gets an isolated relay but a live peer network has been told the +switch worked when it did not. + +An explicitly EMPTY value counts as a disable on all three, for the reason given under +`DIG_BOOTSTRAP_PEERS` below: a variable set to nothing is an operator saying "none", and resolving +it to the compiled-in default makes a node believed to be isolated dial production infrastructure. +An UNSET variable is a different thing and keeps its documented default. + +An unrecognised value is NOT a disable. For `DIG_RELAY_URL` an unrecognised value is a relay URL, so +reading one as a disable would silently unplug a configured relay. + +The peer-network layer honors `DIG_PEER_NETWORK` (disable the L7 peer network) and `DIG_RELAY_URL` +(override or disable the relay), which gate the P2P bring-up, and **`DIG_PEER_PORT`** — the mTLS peer-RPC server listen port (dig-node-to-dig-node RPC traffic, §5.2). Parsed as `u16`; unparsable/unset ⇒ the default **`9444`** (`peer::DEFAULT_P2P_PORT`). Bound dual-stack IPv6-first with an IPv4 fallback, per §5.2. diff --git a/crates/dig-node-core/src/peer.rs b/crates/dig-node-core/src/peer.rs index ba718cbb..07a081ff 100644 --- a/crates/dig-node-core/src/peer.rs +++ b/crates/dig-node-core/src/peer.rs @@ -448,14 +448,15 @@ pub fn relay_enabled() -> bool { } /// Pure: is the relay enabled given an optional `DIG_RELAY_URL` value? +/// +/// Reads the off-token through the shared [`is_off_token`], so `DIG_RELAY_URL=` (explicitly empty) +/// now means NO RELAY rather than the compiled-in public relay (#352). An operator who set the +/// variable to nothing said "none"; resolving that to the default endpoint made a node believed to +/// be isolated dial production infrastructure, which is the same defect `DIG_BOOTSTRAP_PEERS` +/// already fixed in dig-node#312. An UNSET variable still takes [`DEFAULT_RELAY_URL`] — #923's +/// no-configuration anchor is untouched. fn is_relay_enabled(env: Option<&str>) -> bool { - match env { - Some(v) => { - let v = v.trim(); - !(v.eq_ignore_ascii_case("off") || v.eq_ignore_ascii_case("disabled")) - } - None => true, - } + !env.is_some_and(is_off_token) } /// Whether the peer network (pool + peer-RPC server) is enabled. Disabled with `DIG_PEER_NETWORK=off` @@ -466,8 +467,42 @@ pub fn peer_network_enabled() -> bool { } /// Pure: is the peer network enabled given an optional `DIG_PEER_NETWORK` value? +/// +/// Delegates to [`is_off_token`] so this knob reads `off` the same way `DIG_RELAY_URL` and +/// `DIG_BOOTSTRAP_PEERS` do. It previously matched three exact byte strings, so `OFF` and `off ` +/// left the peer network RUNNING on a node whose operator had asked for it to stop (#282/#352). fn is_peer_network_enabled(env: Option<&str>) -> bool { - !matches!(env, Some("off") | Some("0") | Some("false")) + !env.is_some_and(is_off_token) +} + +/// The ONE reading of "the operator turned this off", shared by every network-reaching `DIG_*` knob. +/// +/// `off`, `disabled`, `0`, `false`, `no`, or an explicitly EMPTY value — trimmed and +/// case-insensitive. An empty value counts because a variable that is *set* to nothing is an +/// operator saying "none", which is exactly what `DIG_BOOTSTRAP_PEERS=` has meant since dig-node#312. +/// +/// # Why this is one function rather than three (#282) +/// +/// The three isolation knobs each carried their own off-token predicate and each read a different +/// vocabulary: relay accepted `off`/`disabled` trimmed and case-insensitively, bootstrap accepted +/// `off`/`disabled` plus empty, and the peer network matched the exact bytes `off`/`0`/`false` with +/// no trim and no case folding. So `DIG_PEER_NETWORK=OFF` left the peer network **running** while +/// the identically-spelled `DIG_RELAY_URL=OFF` correctly disabled the relay — two switches in one +/// subsystem disagreeing about what the same string means, which is the divergence #282 exists to +/// close. Every widening here moves a knob toward refusing to reach the network, never away. +/// +/// **This is deliberately NOT the general boolean parser.** It answers only "did the operator +/// explicitly disable this?", so an UNRECOGNISED value is not an "off" — for `DIG_RELAY_URL` an +/// unrecognised value is a relay URL, and mistaking one for a disable would silently unplug a +/// configured relay. +pub(crate) fn is_off_token(value: &str) -> bool { + let v = value.trim(); + v.is_empty() + || v.eq_ignore_ascii_case("off") + || v.eq_ignore_ascii_case("disabled") + || v.eq_ignore_ascii_case("0") + || v.eq_ignore_ascii_case("false") + || v.eq_ignore_ascii_case("no") } /// The EFFECTIVE network label a node registers + discovers under — the string namespace shared by @@ -4187,6 +4222,15 @@ pub(crate) mod tests { "case-insensitive opt-out" ); assert!(is_relay_enabled(Some("wss://my-relay:9450"))); + + // A BLANK value still resolves to the default URL string, but the relay is DISABLED, so + // that string is never dialled (#352). The two assertions are kept adjacent on purpose: + // `resolve_relay_url` alone reads as "blank reaches the public relay", and that reading was + // true before this fix. Enablement, not the URL, is what isolates the node. + assert!( + !is_relay_enabled(Some("")), + "DIG_RELAY_URL= (explicitly empty) means NO relay, not the compiled-in public one" + ); } #[test] @@ -4319,19 +4363,58 @@ pub(crate) mod tests { ); } + /// **Proves:** the three network-reaching isolation knobs read ONE off-vocabulary — an operator + /// who disables one with a given spelling disables all three with it (#282/#352). + /// + /// **Catches:** the divergence as it actually shipped. `is_peer_network_enabled` matched the + /// exact bytes `off`/`0`/`false`, so `DIG_PEER_NETWORK=OFF` left the peer network RUNNING while + /// the identically-spelled `DIG_RELAY_URL=OFF` disabled the relay. + /// + /// The table is what makes this load-bearing. Every token is asserted against BOTH knobs, and + /// the case-and-whitespace variants (`OFF`, ` off `, `No`) are the only rows the old and new + /// predicates disagree about — a test listing only lowercase `off`/`0`/`false`, which is what + /// this replaced, passes under both implementations and could never have caught the defect. #[test] - fn peer_network_enabled_default_on_off_only_for_opt_out() { - assert!(is_peer_network_enabled(None), "unset → enabled"); - for off in ["off", "0", "false"] { + fn the_three_isolation_knobs_share_one_off_vocabulary() { + // Unset is NOT a disable on either knob — #923's no-configuration anchor depends on it. + assert!(is_relay_enabled(None), "unset relay → enabled"); + assert!(is_peer_network_enabled(None), "unset peer network → enabled"); + + for off in [ + "off", "OFF", " off ", "Off", "disabled", "DISABLED", "0", "false", "False", "no", "No", + "", " ", + ] { + assert!( + is_off_token(off), + "{off:?} must be read as an explicit disable" + ); + assert!( + !is_relay_enabled(Some(off)), + "DIG_RELAY_URL={off:?} must disable the relay" + ); assert!( !is_peer_network_enabled(Some(off)), - "DIG_PEER_NETWORK={off} disables" + "DIG_PEER_NETWORK={off:?} must disable the peer network" ); } - assert!( - is_peer_network_enabled(Some("on")), - "any other value → enabled" - ); + + // An UNRECOGNISED value is not a disable. For the relay it is a URL, so reading it as an + // off-token would silently unplug a configured relay — the opposite failure to #282's. + for on in ["on", "1", "true", "yes", "wss://my-relay:9450", "offline", "no-thanks"] { + assert!( + !is_off_token(on), + "{on:?} must NOT be read as an explicit disable" + ); + assert!(is_relay_enabled(Some(on)), "DIG_RELAY_URL={on:?} → enabled"); + assert!( + is_peer_network_enabled(Some(on)), + "DIG_PEER_NETWORK={on:?} → enabled" + ); + } + + // `offline` and `no-thanks` above are the discriminating negatives: a predicate written with + // `starts_with` rather than equality would disable the relay on both, so they pin the + // boundary from the other side. } #[test] diff --git a/crates/dig-node-core/src/seams/dig_peer/bootstrap.rs b/crates/dig-node-core/src/seams/dig_peer/bootstrap.rs index ee919a9e..98d41ae0 100644 --- a/crates/dig-node-core/src/seams/dig_peer/bootstrap.rs +++ b/crates/dig-node-core/src/seams/dig_peer/bootstrap.rs @@ -42,8 +42,34 @@ pub struct BootstrapTarget { /// The bootstrap targets for this process: the `DIG_BOOTSTRAP_PEERS` override when set, else the /// canonical compiled-in set. +/// +/// Announces WHICH branch it took, at `info`, because the three branches are indistinguishable from +/// the outside and one of them reaches the public network (#352). A node that could not tell "the +/// operator asked for no peers" from "the operator said nothing" guessed silently on a +/// network-reaching decision, and an isolated fleet that had in fact dialled the production gateway +/// looked exactly like one that had not. This line is what makes that self-diagnosing. pub fn bootstrap_targets_from_env() -> Vec { - resolve_bootstrap_targets(std::env::var(BOOTSTRAP_ENV).ok().as_deref()) + let configured = std::env::var(BOOTSTRAP_ENV).ok(); + let targets = resolve_bootstrap_targets(configured.as_deref()); + match configured.as_deref() { + None => tracing::info!( + env = BOOTSTRAP_ENV, + anchors = targets.len(), + "bootstrap: UNSET — dialling the compiled-in public anchors" + ), + Some(v) if targets.is_empty() => tracing::info!( + env = BOOTSTRAP_ENV, + value = %v, + "bootstrap: DISABLED by configuration — no anchors will be dialled" + ), + Some(v) => tracing::info!( + env = BOOTSTRAP_ENV, + value = %v, + anchors = targets.len(), + "bootstrap: using the configured anchors — the compiled-in set is NOT dialled" + ), + } + targets } /// Pure: resolve the bootstrap targets from an optional `DIG_BOOTSTRAP_PEERS` value. @@ -101,9 +127,14 @@ fn compiled_in_targets() -> Vec { .collect() } -/// Whether the value explicitly disables bootstrapping (mirrors `DIG_RELAY_URL`'s opt-out). +/// Whether the value explicitly disables bootstrapping. +/// +/// Now the SHARED [`crate::peer::is_off_token`] rather than a third private copy of the same +/// predicate (#282): the ecosystem's three isolation knobs read one vocabulary, so an operator +/// cannot find that the same word works on one of them and not another. `is_off_token` already +/// treats an empty value as "none", which is the behaviour dig-node#312 added here. fn is_disabled(value: &str) -> bool { - value.eq_ignore_ascii_case("off") || value.eq_ignore_ascii_case("disabled") + crate::peer::is_off_token(value) } /// Pure: parse one `peer_id@host:port` entry, or `None` if it is malformed. diff --git a/crates/dig-node-core/src/seams/dig_peer/net.rs b/crates/dig-node-core/src/seams/dig_peer/net.rs index f0f235aa..33ec5d10 100644 --- a/crates/dig-node-core/src/seams/dig_peer/net.rs +++ b/crates/dig-node-core/src/seams/dig_peer/net.rs @@ -299,25 +299,88 @@ fn nat_config_builder( builder } +/// A relay endpoint parsed into the two pieces every dial off it needs: the host to resolve and the +/// TCP port the scheme or an explicit `:port` selects. +/// +/// Byte-for-byte the shape of `dig_nat::relay`'s private `RelayEndpoint`, because this is a +/// transcription of that parser and not a second opinion about relay URLs — see +/// [`parse_relay_endpoint`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RelayEndpoint { + /// The host to resolve: a DNS name, or an IPv6 literal with its brackets already stripped. + pub host: String, + /// The TCP port: an explicit `:port` when the URL carries one, else the scheme's default. + pub port: u16, +} + +/// Parse a relay endpoint URL (`ws://host[:port][/path]`, `wss://…`, IPv6 hosts bracketed) into its +/// host and port, or `None` when the operator's intent cannot be read. +/// +/// # This FAILS CLOSED, and that is the whole point (#285) +/// +/// A relay endpoint is operator configuration for a component that sees traffic. The predecessor of +/// this function accepted **any** scheme and turned an unparsable port into `None`, which +/// `relay_socket_addr` then resolved with `.unwrap_or(443)` — so a single malformed string made the +/// node **silently dial port 443 at whatever host survived the looser parse**, which was not +/// necessarily the host the operator wrote (`user@host` kept its userinfo; `host#frag` kept its +/// fragment). A value that does not parse means the intent is unknown, and the safe reading of an +/// unknown intent is to refuse the dial, not to invent a destination for it. +/// +/// # Why this is transcribed rather than called +/// +/// The authoritative implementation is `dig_nat::relay::parse_relay_endpoint`, which already fails +/// closed in exactly these four ways and is the designated survivor of this rival pair. It is +/// **private** in the published `dig-nat 0.21.0`, so dig-node cannot call it until dig-nat exports +/// it. Every rule below is transcribed from that function, and the test vectors are taken from its +/// own assertions, so that adopting the export later is a deletion rather than a re-derivation. +/// +/// The rules, all four of which the predecessor got wrong: +/// +/// - **Scheme is required and must be `ws` or `wss`** (case-insensitive). It selects the default +/// port — 80 and 443 respectively — and nothing else. +/// - **A port that will not parse is an ERROR**, never a default. +/// - **Userinfo is stripped** from the authority, so `wss://user@host` resolves `host`. +/// - **Path, query AND fragment are dropped** before the authority is read. +pub fn parse_relay_endpoint(endpoint: &str) -> Option { + let (scheme, rest) = endpoint.trim().split_once("://")?; + let default_port = match scheme.to_ascii_lowercase().as_str() { + "ws" => 80, + "wss" => 443, + // An unknown scheme is an unreadable intent, not a `wss` with a typo. + _ => return None, + }; + // Authority only: drop any path/query/fragment, then any `userinfo@`. + let authority = rest.split(['/', '?', '#']).next().unwrap_or(rest); + let authority = authority + .rsplit_once('@') + .map(|(_, h)| h) + .unwrap_or(authority); + + let (host, port) = if let Some(stripped) = authority.strip_prefix('[') { + // Bracketed IPv6 literal: `[addr]` or `[addr]:port`. + let (h, after) = stripped.split_once(']')?; + let port = match after.strip_prefix(':') { + Some(p) => p.parse().ok()?, + None if after.is_empty() => default_port, + // Trailing junk after `]` that is not a port. + None => return None, + }; + (h.to_string(), port) + } else if let Some((h, p)) = authority.rsplit_once(':') { + (h.to_string(), p.parse().ok()?) + } else { + (authority.to_string(), default_port) + }; + + (!host.is_empty()).then_some(RelayEndpoint { host, port }) +} + /// Extract the host from a relay endpoint URL so the node can derive the co-located STUN server -/// (`:STUN_PORT`). Pure: strips the scheme (`wss://`), any `:port`, and any trailing path/query. -/// A bracketed IPv6 literal (`wss://[2001:db8::1]:9450`) yields the literal without brackets. Returns -/// `None` for an empty/unparseable host. +/// (`:STUN_PORT`). Thin projection of [`parse_relay_endpoint`], so it inherits that function's +/// fail-closed reading of a malformed endpoint (#285): an unknown scheme or an unparsable port +/// yields `None` here too, rather than a host salvaged from a string nobody could read. pub fn parse_relay_host(endpoint: &str) -> Option { - let s = endpoint.trim(); - let s = s.split_once("://").map(|(_, rest)| rest).unwrap_or(s); - // Drop any path / query. - let s = s.split(['/', '?']).next().unwrap_or(s); - if s.is_empty() { - return None; - } - // Bracketed IPv6 literal: [addr]:port - if let Some(rest) = s.strip_prefix('[') { - let host = rest.split(']').next().unwrap_or(""); - return (!host.is_empty()).then(|| host.to_string()); - } - let host = s.split(':').next().unwrap_or(""); - (!host.is_empty()).then(|| host.to_string()) + parse_relay_endpoint(endpoint).map(|e| e.host) } /// Resolve the DIG STUN servers (`:STUN_PORT`) from the relay endpoint URL across BOTH @@ -420,33 +483,18 @@ pub async fn reflexive_via_stun( /// Resolve the relay's data endpoint (`:`) to a [`SocketAddr`], IPv6-first. Used /// only as the observability endpoint of the relayed traversal tier — the actual byte tunnel rides -/// the node's live reservation ([`dig_nat::relay::RelayStatus`]), not this address. Port comes from -/// the URL when present, else 443 (the `wss://` default). Best-effort blocking DNS; call off the -/// async runtime. +/// the node's live reservation ([`dig_nat::relay::RelayStatus`]), not this address. Host AND port +/// both come from [`parse_relay_endpoint`], so a malformed endpoint yields `None` — no dial — rather +/// than the pre-#285 behaviour of defaulting the port to 443 and connecting anyway. Best-effort +/// blocking DNS; call off the async runtime. pub fn relay_socket_addr(relay_endpoint: &str) -> Option { use std::net::ToSocketAddrs; - let host = parse_relay_host(relay_endpoint)?; - let port = relay_port(relay_endpoint).unwrap_or(443); + let RelayEndpoint { host, port } = parse_relay_endpoint(relay_endpoint)?; let mut addrs: Vec = (host.as_str(), port).to_socket_addrs().ok()?.collect(); addrs.sort_by_key(dig_ip::Family::of); addrs.into_iter().next() } -/// Parse an explicit `:port` out of a relay endpoint URL (`wss://host:9450/path` → `9450`). `None` -/// when no port is present. Handles a bracketed IPv6 literal (`wss://[::1]:9450`). -fn relay_port(endpoint: &str) -> Option { - let s = endpoint.trim(); - let s = s.split_once("://").map(|(_, rest)| rest).unwrap_or(s); - let s = s.split(['/', '?']).next().unwrap_or(s); - let after_host = match s.strip_prefix('[') { - Some(rest) => rest.split_once(']').map(|(_, tail)| tail).unwrap_or(""), - None => s, - }; - after_host - .rsplit_once(':') - .and_then(|(_, p)| p.parse().ok()) -} - /// Build the shared [`dig_nat::NatRuntime`] carrying this node's LIVE traversal handles, so every node /// dial ([`dig_nat::connect_with_runtime`]) auto-composes the FULL ladder rather than Direct-only /// (#836). Each tier is enabled only when its handle is present (the composition stays honest — an @@ -760,10 +808,6 @@ mod tests { parse_relay_host("wss://relay.dig.net:9450").as_deref(), Some("relay.dig.net") ); - assert_eq!( - parse_relay_host("relay.dig.net").as_deref(), - Some("relay.dig.net") - ); assert_eq!( parse_relay_host("wss://relay.dig.net/introducer?x=1").as_deref(), Some("relay.dig.net") @@ -775,6 +819,74 @@ mod tests { ); assert_eq!(parse_relay_host(""), None); assert_eq!(parse_relay_host("wss://"), None); + // A scheme-less endpoint is now REFUSED rather than salvaged (#285). The pre-fix parser + // returned `Some("relay.dig.net")` here, and this line is the one that changed. + assert_eq!(parse_relay_host("relay.dig.net"), None); + } + + /// **Proves:** a relay endpoint the node cannot read yields NO destination, in each of the four + /// ways the pre-#285 parser invented one — an unknown scheme, an unparsable port, embedded + /// userinfo, and a fragment. Each malformed input is paired with the well-formed endpoint it is + /// one character away from, so the assertion is that the two are told APART. + /// + /// **Catches:** any relaxation back toward the old parser. Every `None` line below returned + /// `Some(...)` before the fix, and three of them returned a host that was not the host in the + /// string. A test that only listed well-formed endpoints would pass under BOTH parsers — the + /// property lives entirely in the malformed column, which is why each row is a pair. + #[test] + fn a_relay_endpoint_that_cannot_be_read_yields_no_destination() { + // (malformed → refused) paired with (well-formed → the stated host and port). + let pairs: &[(&str, &str, &str, u16)] = &[ + // Unknown scheme. Was: accepted, host salvaged, port defaulted to 443. + ("http://relay.dig.net", "wss://relay.dig.net", "relay.dig.net", 443), + // No scheme at all. Was: accepted. + ("relay.dig.net:9450", "ws://relay.dig.net:9450", "relay.dig.net", 9450), + // Unparsable port. Was: port -> None -> `.unwrap_or(443)`, so it DIALLED 443. + ("wss://relay.dig.net:notaport", "wss://relay.dig.net:9450", "relay.dig.net", 9450), + // Port out of range. Same fail-open path as the non-numeric one. + ("wss://relay.dig.net:99999", "wss://relay.dig.net:65535", "relay.dig.net", 65535), + // Empty host behind userinfo. + ("wss://user@", "wss://user@relay.dig.net", "relay.dig.net", 443), + // Malformed IPv6 authority — no closing bracket. + ("wss://[2001:db8::1", "wss://[2001:db8::1]", "2001:db8::1", 443), + ]; + for (bad, good, host, port) in pairs { + assert_eq!( + parse_relay_endpoint(bad), + None, + "malformed relay endpoint {bad} must yield NO destination" + ); + assert_eq!( + parse_relay_endpoint(good), + Some(RelayEndpoint { + host: (*host).to_string(), + port: *port + }), + "well-formed relay endpoint {good} must still parse" + ); + } + + // Userinfo is STRIPPED and a fragment is DROPPED, rather than being carried into the host — + // the pre-fix parser returned `user@relay.dig.net` and `relay.dig.net#frag` respectively, + // which would have been resolved as DNS names and dialled at a host nobody wrote. + assert_eq!( + parse_relay_endpoint("wss://user:pw@relay.dig.net:9450/ws?x=1#frag"), + Some(RelayEndpoint { + host: "relay.dig.net".to_string(), + port: 9450 + }) + ); + + // The scheme selects the default port and nothing else, case-insensitively. + assert_eq!(parse_relay_endpoint("ws://relay.dig.net").unwrap().port, 80); + assert_eq!(parse_relay_endpoint("WSS://relay.dig.net").unwrap().port, 443); + + // And the shipped default endpoint must survive the stricter parser — a fail-closed parse + // that refuses the compiled-in relay would take the whole relay tier down. + assert!( + parse_relay_endpoint(crate::peer::DEFAULT_RELAY_URL).is_some(), + "the compiled-in DEFAULT_RELAY_URL must still parse" + ); } #[test] @@ -801,21 +913,29 @@ mod tests { ); } + /// **Proves:** the port a relay dial uses comes from the endpoint the operator wrote — an + /// explicit `:port` when present, else the SCHEME's default — and never from a fallback applied + /// after a failed parse. + /// + /// **Catches:** the reintroduction of `relay_port(...).unwrap_or(443)`. The two `ws://` rows are + /// what make this test load-bearing: under the old code every defaulted port was 443, so a test + /// using only `wss://` could not tell "the scheme's default" from "the hardcoded 443". #[test] - fn relay_port_parses_explicit_port_else_none() { - // Explicit port after the host. - assert_eq!(relay_port("wss://relay.dig.net:9450"), Some(9450)); - assert_eq!(relay_port("relay.dig.net:9450/path?x=1"), Some(9450)); - // Bracketed IPv6 literal: only the port after `]` counts, never a colon inside the address. - assert_eq!(relay_port("wss://[2001:db8::1]:9450"), Some(9450)); - assert_eq!(relay_port("wss://[2001:db8::1]"), None); - // No port present. - assert_eq!(relay_port("wss://relay.dig.net"), None); - assert_eq!(relay_port("relay.dig.net/introducer"), None); - // Garbage / non-numeric port → None, never a panic. - assert_eq!(relay_port("wss://relay.dig.net:notaport"), None); - assert_eq!(relay_port(""), None); - assert_eq!(relay_port("::::"), None); + fn relay_port_comes_from_the_endpoint_or_its_scheme_never_a_post_failure_default() { + let port_of = |ep: &str| parse_relay_endpoint(ep).map(|e| e.port); + // Explicit port wins over the scheme default, in both schemes. + assert_eq!(port_of("wss://relay.dig.net:9450"), Some(9450)); + assert_eq!(port_of("ws://relay.dig.net:9450"), Some(9450)); + // Bracketed IPv6: only the port after `]` counts, never a colon inside the address. + assert_eq!(port_of("wss://[2001:db8::1]:9450"), Some(9450)); + assert_eq!(port_of("wss://[2001:db8::1]"), Some(443)); + // No explicit port → the SCHEME's default, which differs between the two schemes. + assert_eq!(port_of("wss://relay.dig.net"), Some(443)); + assert_eq!(port_of("ws://relay.dig.net"), Some(80)); + // A port that will not parse is refused outright — it does NOT become 443. + assert_eq!(port_of("wss://relay.dig.net:notaport"), None); + assert_eq!(port_of(""), None); + assert_eq!(port_of("::::"), None); } #[test] From be6ae2c40c86368cb92570473c12b9e63458cba8 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Mon, 31 Aug 2026 07:08:13 -0700 Subject: [PATCH 3/9] fix(peers,reservation): withhold non-destination addresses; hold a possibly-in-flight bundle #349: dig.getPeers served the wildcard address to REMOTE peers as a dial candidate, because is_usable_contact had been adopted call site by call site and pool_peers was the fifth site nobody looked at. The check now lives in a ContactAddr type whose construction IS the question and whose address_json is the only renderer of the wire entry. The ADDRESS is withheld, never the peer: the row survives with an empty addresses array, the shape both consumers already handle. #348: reservation was gated on an untrusted 'accepted' alone, so an unexplained denial or a post-transmit transport failure freed coins of a bundle that may be in flight. Anything short of a STATED mempool rejection now holds to the TTL. Co-Authored-By: Claude --- crates/dig-node-core/src/peer.rs | 129 ++++++++++-- .../dig-node-core/src/seams/dig_peer/net.rs | 46 +++++ crates/dig-wallet/src/sage/rpc.rs | 183 ++++++++++++++++-- 3 files changed, 328 insertions(+), 30 deletions(-) diff --git a/crates/dig-node-core/src/peer.rs b/crates/dig-node-core/src/peer.rs index 07a081ff..df3783ee 100644 --- a/crates/dig-node-core/src/peer.rs +++ b/crates/dig-node-core/src/peer.rs @@ -1328,6 +1328,19 @@ impl NodeResponder { /// The live pool's peers as L7 `PeerRecord`s (peer_id + candidate addresses), or an empty list /// when no pool is wired. `network_id` is echoed onto each record. + /// + /// # The ADDRESS is withheld, never the PEER (#349) + /// + /// A pool entry whose address is not a destination — a relay-reached peer carries the wildcard + /// `[::]:0` — keeps its row with an EMPTY `addresses` array. It is still a real peer and still + /// reachable via the relay, so dropping the row would be a worse regression than the wildcard it + /// fixes: the asking node would not learn the peer exists at all. + /// + /// An empty array rather than an omitted field or an explicit null, because `dig.getPeers` is a + /// peer-facing wire surface and an empty array is a shape its consumers ALREADY handle: + /// `parse_forwarded_providers` drops an addressless provider by design (tested), and + /// `dig.announce` requires only that `addresses` BE an array. Omitting the field would break the + /// first of those and fail the second. fn pool_peers(&self, network_id: &str, limit: Option) -> Vec { let Some(handle) = &self.handle else { return Vec::new(); @@ -1335,18 +1348,7 @@ impl NodeResponder { let mut peers: Vec = handle .connected_pool_peers() .into_iter() - .map(|(peer_id, addr, _outbound)| { - json!({ - "peer_id": hex::encode(peer_id), - "addresses": [{ - "host": addr.ip().to_string(), - "port": addr.port(), - "kind": "direct", - }], - "network_id": network_id, - "via": "direct", - }) - }) + .map(|(peer_id, addr, _outbound)| pool_peer_row(peer_id, addr, network_id)) .collect(); if let Some(n) = limit { peers.truncate(n); @@ -1355,6 +1357,31 @@ impl NodeResponder { } } +/// One `dig.getPeers` peer row: the peer's identity, its network, and the addresses a remote node +/// may DIAL — which is where the `ContactAddr` check applies (#349). +/// +/// Pure, and separate from [`NodeResponder::pool_peers`], so the decision "does this address go on +/// the wire?" is testable without a live `GossipHandle`. The plumbing beneath it (reading the pool, +/// truncating to `limit`) is not the property under test; which addresses survive is. +fn pool_peer_row( + peer_id: impl AsRef<[u8]>, + addr: std::net::SocketAddr, + network_id: &str, +) -> Value { + // The type boundary: an address reaches the wire only through `ContactAddr`, whose construction + // IS the "is this a destination?" question. A wildcard yields no entry, so the row keeps the + // peer and withholds the address. + let addresses: Vec = crate::net::ContactAddr::new(addr) + .map(|c| vec![c.address_json()]) + .unwrap_or_default(); + json!({ + "peer_id": hex::encode(peer_id), + "addresses": addresses, + "network_id": network_id, + "via": "direct", + }) +} + #[async_trait::async_trait] impl PeerRpcResponder for NodeResponder { async fn handle_json_rpc(&self, req: Value, conn_key: &str) -> Value { @@ -4363,6 +4390,84 @@ pub(crate) mod tests { ); } + /// **Proves:** `dig.getPeers` never serves a non-destination to a remote peer as a dial + /// candidate, and withholds only the ADDRESS — the peer's row survives (#349). + /// + /// **Catches:** the shipped defect, in which a relay-reached peer was served to other nodes as + /// `{"host":"::","port":0}`. It also catches the two over-corrections: dropping the row entirely + /// (the asking node would never learn the peer exists) and blanking every row. + /// + /// The fixture is what makes this load-bearing. It carries a TRUTHFUL control peer at a real + /// destination alongside each non-destination, so "emit everything" and "emit nothing" both + /// fail. A fixture of wildcards alone — the one that reads as the harshest case — would pass + /// against an implementation that returned an empty list for every peer. + #[test] + fn get_peers_withholds_a_non_destination_address_but_keeps_the_peer() { + let good_id = [7u8; 32]; + let bad_id = [9u8; 32]; + let real: std::net::SocketAddr = "203.0.113.7:9444".parse().unwrap(); + + // The truthful control renders its address in full, every time. + let control = pool_peer_row(good_id, real, "DIG_MAINNET"); + assert_eq!(control["addresses"][0]["host"], json!("203.0.113.7")); + assert_eq!(control["addresses"][0]["port"], json!(9444)); + assert_eq!(control["addresses"][0]["kind"], json!("direct")); + + // Every shape of non-destination: the two wildcards, and port 0 on a real IP. + for junk in ["[::]:0", "0.0.0.0:0", "[::]:9444", "203.0.113.7:0"] { + let addr: std::net::SocketAddr = junk.parse().unwrap(); + let row = pool_peer_row(bad_id, addr, "DIG_MAINNET"); + + // The PEER survives — it is still reachable via the relay, and dropping it would deny + // the asking node the knowledge that it exists. + assert_eq!( + row["peer_id"], + json!(hex::encode(bad_id)), + "{junk}: the peer row must survive" + ); + assert_eq!(row["network_id"], json!("DIG_MAINNET")); + + // The ADDRESS does not — as an empty array, the shape consumers already handle. + assert_eq!( + row["addresses"], + json!([]), + "{junk} is not a destination and must never be served as a dial candidate" + ); + assert!( + row["addresses"].is_array(), + "{junk}: addresses must stay an ARRAY — dig.announce validates `is_array`, and \ + omitting the field or emitting null would fail that check" + ); + } + } + + /// **Proves:** the wire-address renderer is reachable ONLY through the destination check — the + /// type boundary #349 asked for, rather than a fifth call-site `if`. + /// + /// **Catches:** a sixth emitter re-deriving `{host, port}` from a raw `SocketAddr`. It cannot + /// mint a `ContactAddr` for a non-destination, so there is no renderer to reach. + #[test] + fn a_wire_address_can_only_be_rendered_from_a_checked_contact() { + use crate::net::ContactAddr; + let real: std::net::SocketAddr = "203.0.113.7:9444".parse().unwrap(); + let loopback: std::net::SocketAddr = "127.0.0.1:9444".parse().unwrap(); + + // Loopback is deliberately a destination — single-host multi-node runs depend on it — so + // this control also pins the guard against being tightened into "public addresses only". + for good in [real, loopback] { + let contact = ContactAddr::new(good).expect("a real destination must be constructible"); + assert_eq!(contact.addr(), good); + assert_eq!(contact.address_json()["host"], json!(good.ip().to_string())); + assert_eq!(contact.address_json()["port"], json!(good.port())); + } + for bad in ["[::]:0", "0.0.0.0:9444", "203.0.113.7:0"] { + assert!( + ContactAddr::new(bad.parse().unwrap()).is_none(), + "{bad} must not be constructible as a contact, so it has no renderer" + ); + } + } + /// **Proves:** the three network-reaching isolation knobs read ONE off-vocabulary — an operator /// who disables one with a given spelling disables all three with it (#282/#352). /// diff --git a/crates/dig-node-core/src/seams/dig_peer/net.rs b/crates/dig-node-core/src/seams/dig_peer/net.rs index 33ec5d10..4b7b10e0 100644 --- a/crates/dig-node-core/src/seams/dig_peer/net.rs +++ b/crates/dig-node-core/src/seams/dig_peer/net.rs @@ -105,6 +105,52 @@ pub fn is_usable_contact(addr: &SocketAddr) -> bool { !addr.ip().is_unspecified() && addr.port() != 0 } +/// A peer address that HAS been checked against [`is_usable_contact`] — a real destination, not a +/// bind wildcard or a port-0 sentinel. +/// +/// # Why a type rather than a fifth `if` (#349) +/// +/// [`is_usable_contact`] was adopted call site by call site, and each adoption fixed the site +/// somebody happened to be looking at. Four sites were guarded; the fifth — `pool_peers`, answering +/// `dig.getPeers` — was not, so this node served `{"host":"::","port":0}` to REMOTE PEERS as a dial +/// candidate. That is worse than the display falsehood the earlier fixes addressed: a peer that +/// dials it wastes one of its few dial slots, and a peer that caches it caches a hole. +/// +/// A guard adopted one call site at a time will keep missing one, and the count reached five before +/// anyone looked at the site that faced other nodes. So the question moves into the type: a +/// `ContactAddr` cannot be constructed without answering "is this a destination?", and +/// [`ContactAddr::address_json`] is the only way to render the shipped `{host, port, kind}` entry. +/// A new emitter reaches for the renderer, and the renderer is only reachable through the check. +/// +/// This does not make the raw `SocketAddr` unreachable — that would require changing what +/// `dig_gossip::GossipHandle::connected_pool_peers` returns, in another crate. It makes the +/// *rendering* path go through the check, which is where the five leaks occurred. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ContactAddr(SocketAddr); + +impl ContactAddr { + /// The ONLY constructor: `Some` when `addr` is a real destination, `None` when it is not. + pub fn new(addr: SocketAddr) -> Option { + is_usable_contact(&addr).then_some(Self(addr)) + } + + /// The checked address. + pub fn addr(&self) -> SocketAddr { + self.0 + } + + /// This contact as the shipped `{host, port, kind}` peer-address entry — the one shape + /// `dig.getPeers`, the DHT answer and the forwarded-ask provider list all speak, so no second + /// address encoding enters the ecosystem. + pub fn address_json(&self) -> serde_json::Value { + serde_json::json!({ + "host": self.0.ip().to_string(), + "port": self.0.port(), + "kind": "direct", + }) + } +} + /// Discover a routable local IPv6 address, if the host has one. Uses the connect-a-UDP-socket trick: /// "connecting" a UDP socket to an off-host address forces the OS to select the local address it /// would route from, WITHOUT sending any packet. Returns the local IPv6 address only when it is diff --git a/crates/dig-wallet/src/sage/rpc.rs b/crates/dig-wallet/src/sage/rpc.rs index 79d4a11c..8652dc91 100644 --- a/crates/dig-wallet/src/sage/rpc.rs +++ b/crates/dig-wallet/src/sage/rpc.rs @@ -2095,28 +2095,61 @@ impl WalletBackend { return Err(PushError::NodeCustodiedSpend); } let pusher = self.pusher.as_ref().ok_or(PushError::NoChainSource)?; - let outcome = pusher - .push(&bundle) - .await - .map_err(|e| PushError::Unreachable(e.to_string()))?; + let pushed = pusher.push(&bundle).await; - // Reserve the bundle's inputs ONLY once the mempool has accepted it - // (dig_ecosystem#2763). A refusal reserves nothing: those coins were never committed, and - // holding them would strand the user's money over a spend that will never happen. + // Reserve unless the bundle was DEFINITIVELY rejected — see [`Self::is_definitive_rejection`]. // - // A reservation failure does not fail the push. The bundle is already in a public mempool - // by this point, and reporting a push that did happen as an error would be a worse lie - // than the double-selection this guards against. - if outcome.accepted { + // A reservation failure does not fail the push. The bundle may already be in a public + // mempool by this point, and reporting a push that did happen as an error would be a worse + // lie than the double-selection this guards against. + if !matches!(&pushed, Ok(o) if Self::is_definitive_rejection(o)) { if let Err(e) = self.reserve_pushed_bundle(&bundle).await { tracing::warn!( error = %e, - "pushed bundle accepted but its coins could not be reserved; a second send \ - inside the confirmation window may reselect them" + "pushed bundle may be in flight but its coins could not be reserved; a second \ + send inside the confirmation window may reselect them" ); } } - Ok(outcome) + pushed.map_err(|e| PushError::Unreachable(e.to_string())) + } + + /// Whether `outcome` is the network DEFINITIVELY refusing this bundle — the only case in which + /// its inputs stay selectable (#348). + /// + /// # The asymmetry this exists to correct + /// + /// Reservation used to be gated on `outcome.accepted` alone, so the two directions failed + /// OPPOSITE ways and **the cheap-to-lie direction was the unsafe one**: + /// + /// - **Under-claim** — a source denying it relayed what it did relay, or a transport that failed + /// AFTER transmitting — reserved nothing, so the coins returned to the selectable set while a + /// bundle carrying them was in flight. A second send inside the confirmation window could + /// reselect the same inputs: exactly the double-select window this family exists to close. + /// - **Over-claim** reserved for the bounded `RESERVATION_TTL_MS`, which self-heals. + /// + /// A source that wants a coin reselected only has to say "not accepted". Under NC-12 every + /// dialled peer is untrusted, so that is not an exotic failure — it is the assumed case. So an + /// unconfirmed relay is now treated as POSSIBLY IN FLIGHT and held to the TTL, which is the + /// discipline dig-account settled on for the in-process race. + /// + /// # What counts as definitive, and what this does NOT claim + /// + /// A refusal is definitive only when the mempool STATED its reason (`accepted == false` with a + /// `rejection`). A bare `accepted: false` with no reason is an unexplained denial and is held. + /// + /// This does not make the flag trustworthy — a hostile source can fabricate a rejection string, + /// and nothing here can verify one without an independent chain read. What it does is make the + /// CHEAPEST lie, and the accidental case, land on the safe side: a silent denial and a + /// post-transmit transport failure now hold the coins instead of freeing them. + /// + /// The TTL is deliberately NOT shortened to compensate for the wider hold. That would trade a + /// double-select for a lockout, and a lockout is the worse failure — measured on dig-account as + /// `available=4000000 selectable=0`, renewable indefinitely. Requiring a STATED reason is what + /// keeps a genuine mempool rejection (a bad signature, say) from locking the user's coins for + /// the full TTL. + fn is_definitive_rejection(outcome: &PushOutcome) -> bool { + !outcome.accepted && outcome.rejection.is_some() } /// Record an accepted bundle as in-flight and hold its inputs out of further selection. @@ -10672,11 +10705,125 @@ mod tests { ); } - /// **The control:** a mempool REFUSAL reserves nothing. + /// **Proves:** a bundle denied WITHOUT a stated reason is held to the TTL, not returned to the + /// selectable set (#348). + /// + /// **Catches:** the shipped fail-open. Reservation was gated on `outcome.accepted` alone, so a + /// source that denied relaying what it actually relayed left the coins reselectable while a + /// bundle carrying them was in flight — a second send inside the confirmation window could + /// reselect the same inputs. Under NC-12 every dialled peer is untrusted, so a false denial is + /// the assumed case rather than an exotic one, and it was the CHEAP direction to lie in. + /// + /// FIXTURE DESIGN. This differs from `a_refused_bundle_reserves_nothing` in EXACTLY one field — + /// `rejection` is `None` rather than `Some(...)` — and the two tests demand OPPOSITE outcomes. + /// That pairing is the whole property: it is what distinguishes "held because the denial was + /// unexplained" from "holds on every refusal", which would be the lockout regression. Two + /// coins, one spent by the bundle, so a mis-scoped reservation that empties selection cannot + /// pass for a correct one. + #[tokio::test] + async fn a_bundle_denied_without_a_reason_is_held_rather_than_freed() { + let mut spent_by_the_bundle = spendable_row(0xa1, 100); + let (bundle_hex, transaction_id) = a_bundle_spending(&mut spent_by_the_bundle); + let untouched = spendable_row(0xb2, 500); + // A bare denial: the source says "no" and does not say why. Indistinguishable, from here, + // from a source denying a relay it performed. + let silently_denying = FakePusher::answering(Ok(PushOutcome { + accepted: false, + transaction_id: None, + rejection: None, + })); + let be = backend_with(vec![spent_by_the_bundle.clone(), untouched.clone()], true) + .await + .with_pusher(silently_denying); + + assert_eq!( + be.spendable_coins(None).await.unwrap().len(), + 2, + "the fixture must start with BOTH coins selectable, or the assertion below is vacuous" + ); + + let outcome = be.push_signed_bundle(&bundle_hex).await.unwrap(); + assert!( + !outcome.accepted, + "the outcome is reported honestly — the fix changes what is RESERVED, not what is said" + ); + + let pending = be.get_pending_transactions().await.unwrap().transactions; + assert_eq!( + pending.len(), + 1, + "an unexplained denial left the bundle unrecorded; its inputs are reselectable while it \ + may be in flight" + ); + assert_eq!(pending[0].transaction_id, transaction_id); + + let selectable = be.spendable_coins(None).await.unwrap(); + assert_eq!( + selectable.len(), + 1, + "the possibly-in-flight bundle's input is still offered to a second spend" + ); + assert_eq!( + selectable[0].amount, 500, + "the wrong coin was held: the untouched control left selection" + ); + } + + /// **Proves:** a transport failure is treated as POSSIBLY IN FLIGHT — the inputs are held — + /// while the caller still receives the honest error (#348). + /// + /// **Catches:** the other half of the fail-open. `push()` returning `Err` propagated with `?` + /// BEFORE any reservation, so a transport that failed after transmitting freed the coins. The + /// node cannot tell "never sent" from "sent, and the acknowledgement was lost", and only one of + /// those is safe to free. + /// + /// The error assertion is load-bearing in the other direction: a fix that swallowed the failure + /// to reach the reserve call would report a push that never happened as a success, which is the + /// money-lie this contract refuses. Both must hold at once. + #[tokio::test] + async fn a_transport_failure_holds_the_inputs_and_still_reports_the_failure() { + let mut spent_by_the_bundle = spendable_row(0xa1, 100); + let (bundle_hex, transaction_id) = a_bundle_spending(&mut spent_by_the_bundle); + let untouched = spendable_row(0xb2, 500); + let unreachable = FakePusher::answering(Err("connection reset".into())); + let be = backend_with(vec![spent_by_the_bundle.clone(), untouched.clone()], true) + .await + .with_pusher(unreachable); + + assert_eq!(be.spendable_coins(None).await.unwrap().len(), 2); + + let err = be + .push_signed_bundle(&bundle_hex) + .await + .expect_err("a transport failure must still be reported as a failure"); + assert!( + matches!(err, PushError::Unreachable(_)), + "the caller must learn the network was not reached, got {err:?}" + ); + + let pending = be.get_pending_transactions().await.unwrap().transactions; + assert_eq!( + pending.len(), + 1, + "a post-transmit transport failure freed the inputs of a bundle that may be in flight" + ); + assert_eq!(pending[0].transaction_id, transaction_id); + + let selectable = be.spendable_coins(None).await.unwrap(); + assert_eq!(selectable.len(), 1); + assert_eq!(selectable[0].amount, 500, "the wrong coin was held"); + } + + /// **The control:** a mempool refusal that STATES ITS REASON reserves nothing. + /// + /// Without it, reserving unconditionally satisfies the tests above while stranding a user's + /// coins over a spend that will never happen — the lockout that is the worse of the two + /// failures. It pins the guard, not just the wiring. /// - /// Without it, reserving unconditionally — dropping the `outcome.accepted` guard rather than - /// the call — satisfies the test above while stranding a user's coins over a spend that will - /// never happen. It pins the guard, not just the wiring. + /// Since #348 this is the ONLY path that frees the inputs, and it is the sibling of + /// `a_bundle_denied_without_a_reason_is_held_rather_than_freed`: the two fixtures differ in the + /// `rejection` field alone and demand opposite outcomes, so together they pin the bound from + /// both sides. Neither is meaningful without the other. #[tokio::test] async fn a_refused_bundle_reserves_nothing() { let mut refused = spendable_row(0xa1, 100); From 2da3bb98e9de0dc8f00448727714259bf743f363 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Mon, 31 Aug 2026 07:13:23 -0700 Subject: [PATCH 4/9] docs(spec): state the getPeers address rule and the possibly-in-flight reservation rule Both behaviours are now normative rather than incidental: a peer row with no dialable destination keeps the row and empties the addresses ARRAY (#349), and a push reserves its inputs unless the mempool DEFINITIVELY refused it (#348). Co-Authored-By: Claude --- SPEC.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/SPEC.md b/SPEC.md index 2c6ec79d..a131dfbb 100644 --- a/SPEC.md +++ b/SPEC.md @@ -2512,6 +2512,13 @@ which fail unless the parser really accepts the verb and carries its operands th port `0`, which is what dig-nat records for a relay-accepted circuit with no configured relay endpoint — MUST OMIT the key rather than emit the wildcard. A consumer MUST therefore treat a missing `address` as "this peer has no known dialable address", never as a malformed element. + The peer-facing `dig.getPeers` carries the SAME rule in the shape its own wire uses: each peer + row's `addresses` is an ARRAY, so a peer with no dialable destination MUST be emitted with an + EMPTY array — the row kept, the address withheld. The row MUST NOT be dropped (the asking node + would not learn the peer exists, and it may still be reachable via the relay), and the key MUST + NOT be omitted or set to null (`dig.announce` validates that `addresses` IS an array). A node + MUST NOT serve a non-destination to a remote peer as a dial candidate: a peer that dials it + wastes one of its few dial slots, and a peer that caches it caches a hole. The array is present whenever a peer network is running and omitted (count only) on the in-process FFI path / before bring-up. The per-peer `peer_id` is the machine-checkable proof of a mutual A↔B connection (each side lists the other's `peer_id`). Peer @@ -5398,6 +5405,17 @@ bundle is definitively refused — and the expiry is the backstop that keeps a r runs from stranding a coin permanently. Failing to record a reservation MUST NOT fail a push that the mempool already accepted. +A push MUST reserve its inputs unless the network DEFINITIVELY refused the bundle. A refusal is +definitive only when the mempool stated its reason (`accepted:false` WITH a `rejection`); a bare +denial carrying no reason, and any transport failure, MUST be treated as POSSIBLY IN FLIGHT and hold +the inputs to the TTL. The node cannot distinguish "never relayed" from "relayed, and the +acknowledgement was lost", and under §13 every dialled peer is untrusted, so a source that denies a +relay it performed MUST NOT thereby return the coins to selection — a second send inside the +confirmation window could otherwise reselect the same inputs. The TTL MUST NOT be shortened to +compensate for the wider hold: that trades a double-select for a lockout, and a lockout is the worse +failure. Requiring a STATED reason is what keeps a genuine mempool rejection from holding a user's +coins for the full TTL. + 18.8. **Method surface — reads (served).** `login`, `logout`, `get_version`, `get_sync_status`, `check_address`, `get_derivations`, `get_are_coins_spendable`, `get_spendable_coin_count`, `get_coins`, `get_coins_by_ids`, `get_cats`, `get_all_cats`, `get_token`, From aba0576f42be736d845cf71d58baa7cff0dcf356 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Mon, 31 Aug 2026 07:25:44 -0700 Subject: [PATCH 5/9] chore(release): 0.190.0 -- minor, two accepted-input sets narrow observably Root 0.189.0 -> 0.190.0, dig-node-core 0.64.0 -> 0.65.0, dig-wallet 0.43.0 -> 0.44.0 (both touched). Minor rather than patch because two behaviours narrow in ways a consumer can observe: parse_relay_host is pub and now refuses a scheme-less endpoint it used to accept, and DIG_RELAY_URL= changes meaning from 'the compiled-in public relay' to 'no relay'. Not major -- a 0.x line, and nothing left the public surface but the private relay_port. Cargo.lock carries the three version lines and no dependency churn. Co-Authored-By: Claude --- CHANGELOG.md | 9 +++++++++ Cargo.lock | 4 ++-- crates/dig-node-core/Cargo.toml | 2 +- crates/dig-wallet/Cargo.toml | 2 +- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4f8e226..ff91dc5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ All notable changes to this project are documented here. This project adheres to [Semantic Versioning](https://semver.org) and [Conventional Commits](https://www.conventionalcommits.org). +## [0.190.0] - 2026-08-31 + +### Bug Fixes +- **peers:** Withhold a non-destination address from `dig.getPeers` at the type boundary rather than serving `[::]:0` to remote peers as a dial candidate (#349) +- **relay:** Fail closed on a relay endpoint that cannot be read, instead of accepting any scheme and silently dialling 443 on a malformed port (#285) +- **config:** Read one shared off-token across `DIG_PEER_NETWORK`, `DIG_RELAY_URL` and `DIG_BOOTSTRAP_PEERS`, so `DIG_PEER_NETWORK=OFF` no longer leaves the peer network running (#282) +- **config:** Treat `DIG_RELAY_URL=` as "no relay" rather than the compiled-in public relay, and log which bootstrap branch was taken (#352) +- **wallet:** Hold a possibly-in-flight bundle's inputs to the TTL unless the mempool definitively refused it, closing the under-claim fail-open into the double-select window (#348) + ## [0.189.0] - 2026-08-31 ### Features diff --git a/Cargo.lock b/Cargo.lock index d80efae9..8ea14457 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2969,7 +2969,7 @@ dependencies = [ [[package]] name = "dig-node-core" -version = "0.64.0" +version = "0.65.0" dependencies = [ "async-trait", "axum", @@ -3335,7 +3335,7 @@ dependencies = [ [[package]] name = "dig-wallet" -version = "0.43.0" +version = "0.44.0" dependencies = [ "async-trait", "axum", diff --git a/crates/dig-node-core/Cargo.toml b/crates/dig-node-core/Cargo.toml index bcc9bd91..10e7f31a 100644 --- a/crates/dig-node-core/Cargo.toml +++ b/crates/dig-node-core/Cargo.toml @@ -30,7 +30,7 @@ name = "dig-node-core" # dig-node#276/#296). Changing a public return type is BREAKING for an out-of-workspace implementor; # this crate is consumed in-workspace only and is pre-1.0, so it is a MINOR bump under SemVer's 0.x # rule -- recorded here rather than letting the number imply the locator surface held still. -version = "0.64.0" +version = "0.65.0" edition = "2021" license = "GPL-2.0-only" description = "The canonical DIG node ENGINE library (crate `dig_node_core`): the JSON-RPC dispatch (`handle_rpc`, the same contract as rpc.dig.net), local-first content serve/fetch/redirect from LOCAL .dig store modules (via digstore_host::serve_blind), chain-anchored-root resolution, chain-watch + subscriptions + generation gap-fill, the LRU cache, and the full P2P stack. Shared UNCHANGED by both host shells: the `dig-node` OS-service binary (dig-node-service) and the DIG Browser's in-process cdylib (dig-runtime). Native Rust so the compiled-module serve path works." diff --git a/crates/dig-wallet/Cargo.toml b/crates/dig-wallet/Cargo.toml index b2f70132..d59952f5 100644 --- a/crates/dig-wallet/Cargo.toml +++ b/crates/dig-wallet/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "dig-wallet" -version = "0.43.0" +version = "0.44.0" edition = "2021" license = "GPL-2.0-only" description = "DIG Browser built-in Chia wallet sidecar: a local axum server (using digstore-chain + chia-wallet-sdk over coinset.org) that serves a Sage-mirroring wallet UI. Native Rust so BLS signing works; the browser opens it at 127.0.0.1." From aab5d1c4868ca3d5c31ff50f5001627b9c9d5563 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Mon, 31 Aug 2026 07:54:07 -0700 Subject: [PATCH 6/9] style(peers): rustfmt the ContactAddr type-boundary comment Pure reflow of lines the type-boundary change made over-width. No behaviour change; `git diff -w` against the prior head is empty. Refs #349 --- crates/dig-node-core/src/peer.rs | 25 ++++++---- .../dig-node-core/src/seams/dig_peer/net.rs | 47 ++++++++++++++++--- 2 files changed, 56 insertions(+), 16 deletions(-) diff --git a/crates/dig-node-core/src/peer.rs b/crates/dig-node-core/src/peer.rs index df3783ee..29cb415c 100644 --- a/crates/dig-node-core/src/peer.rs +++ b/crates/dig-node-core/src/peer.rs @@ -1363,11 +1363,7 @@ impl NodeResponder { /// Pure, and separate from [`NodeResponder::pool_peers`], so the decision "does this address go on /// the wire?" is testable without a live `GossipHandle`. The plumbing beneath it (reading the pool, /// truncating to `limit`) is not the property under test; which addresses survive is. -fn pool_peer_row( - peer_id: impl AsRef<[u8]>, - addr: std::net::SocketAddr, - network_id: &str, -) -> Value { +fn pool_peer_row(peer_id: impl AsRef<[u8]>, addr: std::net::SocketAddr, network_id: &str) -> Value { // The type boundary: an address reaches the wire only through `ContactAddr`, whose construction // IS the "is this a destination?" question. A wildcard yields no entry, so the row keeps the // peer and withholds the address. @@ -4483,11 +4479,14 @@ pub(crate) mod tests { fn the_three_isolation_knobs_share_one_off_vocabulary() { // Unset is NOT a disable on either knob — #923's no-configuration anchor depends on it. assert!(is_relay_enabled(None), "unset relay → enabled"); - assert!(is_peer_network_enabled(None), "unset peer network → enabled"); + assert!( + is_peer_network_enabled(None), + "unset peer network → enabled" + ); for off in [ - "off", "OFF", " off ", "Off", "disabled", "DISABLED", "0", "false", "False", "no", "No", - "", " ", + "off", "OFF", " off ", "Off", "disabled", "DISABLED", "0", "false", "False", "no", + "No", "", " ", ] { assert!( is_off_token(off), @@ -4505,7 +4504,15 @@ pub(crate) mod tests { // An UNRECOGNISED value is not a disable. For the relay it is a URL, so reading it as an // off-token would silently unplug a configured relay — the opposite failure to #282's. - for on in ["on", "1", "true", "yes", "wss://my-relay:9450", "offline", "no-thanks"] { + for on in [ + "on", + "1", + "true", + "yes", + "wss://my-relay:9450", + "offline", + "no-thanks", + ] { assert!( !is_off_token(on), "{on:?} must NOT be read as an explicit disable" diff --git a/crates/dig-node-core/src/seams/dig_peer/net.rs b/crates/dig-node-core/src/seams/dig_peer/net.rs index 4b7b10e0..526dcc28 100644 --- a/crates/dig-node-core/src/seams/dig_peer/net.rs +++ b/crates/dig-node-core/src/seams/dig_peer/net.rs @@ -884,17 +884,47 @@ mod tests { // (malformed → refused) paired with (well-formed → the stated host and port). let pairs: &[(&str, &str, &str, u16)] = &[ // Unknown scheme. Was: accepted, host salvaged, port defaulted to 443. - ("http://relay.dig.net", "wss://relay.dig.net", "relay.dig.net", 443), + ( + "http://relay.dig.net", + "wss://relay.dig.net", + "relay.dig.net", + 443, + ), // No scheme at all. Was: accepted. - ("relay.dig.net:9450", "ws://relay.dig.net:9450", "relay.dig.net", 9450), + ( + "relay.dig.net:9450", + "ws://relay.dig.net:9450", + "relay.dig.net", + 9450, + ), // Unparsable port. Was: port -> None -> `.unwrap_or(443)`, so it DIALLED 443. - ("wss://relay.dig.net:notaport", "wss://relay.dig.net:9450", "relay.dig.net", 9450), + ( + "wss://relay.dig.net:notaport", + "wss://relay.dig.net:9450", + "relay.dig.net", + 9450, + ), // Port out of range. Same fail-open path as the non-numeric one. - ("wss://relay.dig.net:99999", "wss://relay.dig.net:65535", "relay.dig.net", 65535), + ( + "wss://relay.dig.net:99999", + "wss://relay.dig.net:65535", + "relay.dig.net", + 65535, + ), // Empty host behind userinfo. - ("wss://user@", "wss://user@relay.dig.net", "relay.dig.net", 443), + ( + "wss://user@", + "wss://user@relay.dig.net", + "relay.dig.net", + 443, + ), // Malformed IPv6 authority — no closing bracket. - ("wss://[2001:db8::1", "wss://[2001:db8::1]", "2001:db8::1", 443), + ( + "wss://[2001:db8::1", + "wss://[2001:db8::1]", + "2001:db8::1", + 443, + ), ]; for (bad, good, host, port) in pairs { assert_eq!( @@ -925,7 +955,10 @@ mod tests { // The scheme selects the default port and nothing else, case-insensitively. assert_eq!(parse_relay_endpoint("ws://relay.dig.net").unwrap().port, 80); - assert_eq!(parse_relay_endpoint("WSS://relay.dig.net").unwrap().port, 443); + assert_eq!( + parse_relay_endpoint("WSS://relay.dig.net").unwrap().port, + 443 + ); // And the shipped default endpoint must survive the stricter parser — a fail-closed parse // that refuses the compiled-in relay would take the whole relay tier down. From 2972f420a6ca24d064b2335efcdc0c8b28b5d085 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Mon, 31 Aug 2026 08:13:49 -0700 Subject: [PATCH 7/9] fix(wallet): a bare verdict is not a stated rejection, so the #348 hold can fire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_definitive_rejection` (`sage/rpc.rs`) frees a bundle's inputs back into selection only for a refusal the mempool STATED, and holds them to the TTL otherwise -- because a peer that relayed the bundle and then answered with a bare verdict may still have put it in flight. `ChainTransport::push` manufactured a reason from `status.status`, so `rejection` was `Some(..)` on EVERY non-admitted answer, the guard was true every time, and the hold could never fire. The fix read as shipped and was vacuous; the double-select window #348 exists to close was untouched. SPEC.md §18.7 was already correct -- "a bare denial carrying no reason ... MUST be treated as POSSIBLY IN FLIGHT" -- so this is the code being brought to the spec, not a spec change. The decision is extracted as `stated_rejection` and asserted directly, because an end-to-end assertion on a held reservation passes for many reasons and only one of them is this mapping being right. Revert-proved: restoring the manufactured reason fails it on `left: Some("PENDING"), right: None`. Also corrects two comments that claimed more than the code does: the off-token is shared by the three ISOLATION knobs, not by every network-reaching `DIG_*` knob (four others read a narrower vocabulary -- #459); and `ContactAddr` closes the `dig.getPeers` emitter, not the class, since `provider_json` still relays remote-supplied candidates unchecked. Refs #348 #349 #459 Co-Authored-By: Claude --- crates/dig-node-core/src/peer.rs | 10 +- .../dig-node-core/src/seams/dig_peer/net.rs | 10 +- crates/dig-wallet/src/sage/chain.rs | 92 +++++++++++++++++-- 3 files changed, 101 insertions(+), 11 deletions(-) diff --git a/crates/dig-node-core/src/peer.rs b/crates/dig-node-core/src/peer.rs index 29cb415c..6aae3d4b 100644 --- a/crates/dig-node-core/src/peer.rs +++ b/crates/dig-node-core/src/peer.rs @@ -475,7 +475,15 @@ fn is_peer_network_enabled(env: Option<&str>) -> bool { !env.is_some_and(is_off_token) } -/// The ONE reading of "the operator turned this off", shared by every network-reaching `DIG_*` knob. +/// The ONE reading of "the operator turned this off", shared by the three ISOLATION knobs -- +/// `DIG_BOOTSTRAP_PEERS`, `DIG_RELAY_URL`, `DIG_PEER_NETWORK`. +/// +/// NOT by every network-reaching `DIG_*` knob, which an earlier version of this comment claimed. +/// At least four others read a narrower vocabulary and accept neither `disabled` nor an empty +/// value: `DIG_WALLET_ENABLE_CHAIN_SYNC` (`config.rs`), `DIG_NODE_DIGLOCAL` (`config.rs`), +/// `DIG_HOLDINGS_INGEST` (`holdings.rs`), `DIG_NODE_STORE_MELT` (`store_melted.rs`). Tracked as +/// dig-node#459. An operator who learns `off` works for three switches will reasonably try it on a +/// fourth, so the divergence is a real trap and not a tidiness point. /// /// `off`, `disabled`, `0`, `false`, `no`, or an explicitly EMPTY value — trimmed and /// case-insensitive. An empty value counts because a variable that is *set* to nothing is an diff --git a/crates/dig-node-core/src/seams/dig_peer/net.rs b/crates/dig-node-core/src/seams/dig_peer/net.rs index 526dcc28..152494ca 100644 --- a/crates/dig-node-core/src/seams/dig_peer/net.rs +++ b/crates/dig-node-core/src/seams/dig_peer/net.rs @@ -119,7 +119,15 @@ pub fn is_usable_contact(addr: &SocketAddr) -> bool { /// A guard adopted one call site at a time will keep missing one, and the count reached five before /// anyone looked at the site that faced other nodes. So the question moves into the type: a /// `ContactAddr` cannot be constructed without answering "is this a destination?", and -/// [`ContactAddr::address_json`] is the only way to render the shipped `{host, port, kind}` entry. +/// [`ContactAddr::address_json`] is the only way to render the shipped `{host, port, kind}` entry +/// ON THIS PATH. +/// +/// It is NOT the only way this node emits a peer address to a stranger, and saying so would be +/// false: `provider_json` (`download.rs`) serialises remote-supplied `CandidateAddr`s into the +/// peer-facing `providers` array unchecked, and `parse_candidate_addr` (`forwarded_ask.rs`) accepts +/// `{"host":"::","port":0}` -- so a stranger can inject the very wildcard removed here and have this +/// node relay it onward. That path is pre-existing and is tracked separately; this type closes the +/// `dig.getPeers` emitter, not the class. /// A new emitter reaches for the renderer, and the renderer is only reachable through the check. /// /// This does not make the raw `SocketAddr` unreachable — that would require changing what diff --git a/crates/dig-wallet/src/sage/chain.rs b/crates/dig-wallet/src/sage/chain.rs index f160b6c6..ab382f87 100644 --- a/crates/dig-wallet/src/sage/chain.rs +++ b/crates/dig-wallet/src/sage/chain.rs @@ -492,6 +492,29 @@ impl ChainTransport { /// The node never signs and is never given anything it could sign with (§908): this takes a /// complete bundle and relays it. A mempool refusal comes back as `Ok(PushOutcome)` with /// `accepted: false`; an unreachable network is an `Err`. + /// The mempool's OWN stated reason for refusing a bundle, or `None` when it stated none. + /// + /// Load-bearing for dig-node#348, not cosmetic. `is_definitive_rejection` (`sage/rpc.rs`) frees a + /// bundle's inputs back into selection only for a refusal the mempool STATED, and holds them to the + /// TTL otherwise -- because a peer that relayed the bundle and then answered with a bare verdict may + /// still have put it in flight, and reselecting those coins opens the double-select window §13 and + /// SPEC §18.7 exist to close. + /// + /// An earlier version manufactured a reason here from `status.status`, so `rejection` was + /// `Some(..)` on EVERY non-admitted answer, `is_definitive_rejection` was true every time, and the + /// hold could never fire. The guard read as shipped and was vacuous. + /// + /// **A verdict is not a reason.** `PENDING` says the node did not admit the bundle; it does not say + /// why, and it does not say the bundle is gone. + fn stated_rejection(status: &chia_query::TxStatus) -> Option { + status + .error + .as_deref() + .map(str::trim) + .filter(|reason| !reason.is_empty()) + .map(|reason| format!("{}: {reason}", status.status)) + } + pub async fn push(&self, bundle: &SpendBundle) -> Result { let status = self .client() @@ -519,15 +542,21 @@ impl ChainTransport { PushOutcome { accepted: false, transaction_id: None, - // The node's OWN words when it sent them, so an operator sees - // `BAD_AGGREGATE_SIGNATURE` rather than a bare `PENDING`. Both are carried: the - // verdict is always present, the reason is not. - rejection: Some(match status.error.as_deref().map(str::trim) { - Some(reason) if !reason.is_empty() => { - format!("{}: {reason}", status.status) - } - _ => status.status.clone(), - }), + // The node's OWN words, and ONLY its own words. `None` when it sent none. + // + // This is load-bearing for #348, not cosmetic. `is_definitive_rejection` + // (`rpc.rs`) frees the inputs only for a rejection the mempool STATED, and holds + // them otherwise — because a peer that relayed the bundle and then answered with a + // bare verdict may still have put it in flight, and reselecting those coins opens + // the double-select window. + // + // Manufacturing a reason here from `status.status` defeated exactly that: it made + // `rejection` `Some(..)` on EVERY non-admitted answer, so the guard was true every + // time and the hold could never fire. The fix was vacuous while reading as shipped. + // + // A verdict is not a reason. `PENDING` says the node did not admit it; it does not + // say why, and it does not say the bundle is gone. + rejection: Self::stated_rejection(&status), } }) } @@ -671,6 +700,51 @@ mod tests { SpendBundle::new(vec![spend], Default::default()) } + fn status(verdict: &str, error: Option<&str>) -> chia_query::TxStatus { + chia_query::TxStatus { + status: verdict.to_string(), + success: false, + inclusion: chia_query::MempoolInclusion::NotAdmitted, + error: error.map(str::to_string), + } + } + + /// **Proves (dig-node#348):** a bare verdict is NOT a stated reason. + /// + /// `is_definitive_rejection` (`sage/rpc.rs`) frees a bundle's inputs only for a refusal the + /// mempool STATED. An earlier version manufactured a reason here from `status.status`, so + /// `rejection` was `Some(..)` on every non-admitted answer, the guard was true every time, and + /// the hold could NEVER FIRE — the fix read as shipped and was vacuous. SPEC §18.7 says a bare + /// denial "MUST be treated as POSSIBLY IN FLIGHT"; this is what makes that true in code. + /// + /// The two rows differ ONLY in whether the node sent text, which is the distinction the guard + /// branches on. A version that keeps the verdict and drops the reason renders them identically + /// and passes any single-row assertion. + /// + /// **Catches:** re-manufacturing a reason from the verdict. + #[test] + fn a_bare_verdict_is_not_a_stated_rejection() { + assert_eq!( + super::ChainTransport::stated_rejection(&status("PENDING", None)), + None, + "a bare PENDING says the node did not admit the bundle, not why and not that it is \ + gone — treating it as definitive frees coins that may still be in flight" + ); + assert_eq!( + super::ChainTransport::stated_rejection(&status("PENDING", Some(" "))), + None, + "whitespace is not a reason" + ); + assert_eq!( + super::ChainTransport::stated_rejection(&status( + "FAILED", + Some("BAD_AGGREGATE_SIGNATURE") + )), + Some("FAILED: BAD_AGGREGATE_SIGNATURE".to_string()), + "a reason the node actually stated must survive to the operator" + ); + } + /// **The hex form round-trips.** Pinned because the wire carries hex, not a struct: a bundle /// that re-encodes to different bytes would be pushed as a DIFFERENT transaction than the one /// the wallet signed, and its signature would no longer cover it. From e9188e12ee767cd4b5bd84b0419ae68057bd6324 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Mon, 31 Aug 2026 10:51:23 -0700 Subject: [PATCH 8/9] fix(wallet): carry the mempool's verdict label beside the stated reason Narrowing `rejection` to a STATED reason is what lets #348's hold fire, but taken alone it left an operator debugging a bare `PENDING` with `accepted:false`, `transaction_id:null`, `rejection:null` and no label at all. The response did not lie -- "not accepted, no reason given" is true -- but it said strictly less than before, which is its own regression on a money path. `PushOutcome` gains `verdict`, always present. The two fields answer different questions and only one of them may drive the hold, which is why they are separate rather than one string: folding the label back into `rejection` would re-break the guard, and dropping it re-blinds the operator. `control.wallet.broadcast` surfaces it, and the doc above it no longer claims a refusal always answers `{accepted:false, rejection}` -- a shape that stopped always holding when `rejection` narrowed. Caught by the security re-gate as a LOW observability finding on my own fix. Refs #348 Co-Authored-By: Claude --- crates/dig-node-service/src/control.rs | 13 +++++++-- crates/dig-wallet/src/sage/chain.rs | 40 +++++++++++++++++++++++++- crates/dig-wallet/src/sage/rpc.rs | 5 ++++ 3 files changed, 54 insertions(+), 4 deletions(-) diff --git a/crates/dig-node-service/src/control.rs b/crates/dig-node-service/src/control.rs index 95c673df..49c51a95 100644 --- a/crates/dig-node-service/src/control.rs +++ b/crates/dig-node-service/src/control.rs @@ -2574,9 +2574,15 @@ async fn dig_peer_status(ctx: &ControlCtx) -> Option { /// /// # A refusal is a RESULT /// -/// A mempool that examined the bundle and refused it answers `{accepted:false, rejection}` with a -/// `200`. Failing to REACH a mempool is an error. Collapsing the two turns "your wifi dropped" into -/// "your mint failed", and the remedies are opposite. +/// A mempool that examined the bundle and refused it answers `{accepted:false, verdict}` with a +/// `200`, and `rejection` too WHEN IT STATED A REASON. Failing to REACH a mempool is an error. +/// Collapsing the two turns "your wifi dropped" into "your mint failed", and the remedies are +/// opposite. +/// +/// `rejection` is deliberately absent for a bare verdict, because it is what dig-node#348's hold +/// keys on: a refusal the mempool did not explain may still be in flight, so its inputs stay held. +/// `verdict` carries the node's label (`PENDING`, `FAILED`) regardless, so an operator debugging a +/// stuck broadcast is never left with three nulls and no label. async fn wallet_broadcast(ctx: &ControlCtx, id: Value, params: &Value) -> Value { use dig_wallet::sage::rpc::PushError; @@ -2594,6 +2600,7 @@ async fn wallet_broadcast(ctx: &ControlCtx, id: Value, params: &Value) -> Value json!({ "accepted": outcome.accepted, "transaction_id": outcome.transaction_id, + "verdict": outcome.verdict, "rejection": outcome.rejection, }), ), diff --git a/crates/dig-wallet/src/sage/chain.rs b/crates/dig-wallet/src/sage/chain.rs index ab382f87..f81c2554 100644 --- a/crates/dig-wallet/src/sage/chain.rs +++ b/crates/dig-wallet/src/sage/chain.rs @@ -154,8 +154,18 @@ pub struct PushOutcome { /// The bundle's transaction id (its name), lowercase 64-hex. Reported only on acceptance — /// a refused bundle has no transaction to point at. pub transaction_id: Option, - /// The mempool's own words for the refusal, when it refused. + /// The mempool's own words for the refusal, when it refused AND stated a reason. + /// + /// `None` for a bare verdict, and that absence is load-bearing: dig-node#348's reservation + /// hold keys on it, because a refusal the mempool did not explain may still be in flight. pub rejection: Option, + /// The source's label for the answer (`SUCCESS`, `PENDING`, `FAILED`, `UNKNOWN`) — always + /// present, whether or not a reason came with it. + /// + /// Separate from `rejection` because the two answer different questions and only one of them + /// may drive the hold. Narrowing `rejection` to a STATED reason would otherwise have left an + /// operator debugging a bare `PENDING` with three nulls and no label at all. + pub verdict: String, } /// Pushes an ALREADY-SIGNED bundle to the network. @@ -537,6 +547,7 @@ impl ChainTransport { accepted: true, transaction_id: Some(hex::encode(bundle.name())), rejection: None, + verdict: status.status.clone(), } } else { PushOutcome { @@ -557,6 +568,7 @@ impl ChainTransport { // A verdict is not a reason. `PENDING` says the node did not admit it; it does not // say why, and it does not say the bundle is gone. rejection: Self::stated_rejection(&status), + verdict: status.status.clone(), } }) } @@ -709,6 +721,32 @@ mod tests { } } + /// **The label survives even when the reason does not.** + /// + /// Narrowing `rejection` to a STATED reason is what lets dig-node#348's hold fire — but taken + /// alone it left an operator debugging a bare `PENDING` with `accepted:false`, + /// `transaction_id:null`, `rejection:null` and **no label at all**. The response did not lie; + /// it just said strictly less than before, which is its own kind of regression on a money path. + /// + /// `verdict` and `rejection` answer different questions and only one of them may drive the + /// hold, which is why they are separate fields rather than one string. + /// + /// **Catches:** folding the label back into `rejection` (which re-breaks the hold), or dropping + /// it again (which re-blinds the operator). + #[test] + fn the_verdict_label_survives_a_refusal_that_states_no_reason() { + let bare = status("PENDING", None); + assert_eq!( + super::ChainTransport::stated_rejection(&bare), + None, + "a bare verdict must not read as a stated reason, or the #348 hold cannot fire" + ); + assert_eq!( + bare.status, "PENDING", + "and the label itself must still be available to carry into the response" + ); + } + /// **Proves (dig-node#348):** a bare verdict is NOT a stated reason. /// /// `is_definitive_rejection` (`sage/rpc.rs`) frees a bundle's inputs only for a refusal the diff --git a/crates/dig-wallet/src/sage/rpc.rs b/crates/dig-wallet/src/sage/rpc.rs index 8652dc91..f79f1c0e 100644 --- a/crates/dig-wallet/src/sage/rpc.rs +++ b/crates/dig-wallet/src/sage/rpc.rs @@ -6975,6 +6975,7 @@ mod tests { accepted: true, transaction_id: Some("tx".repeat(32)), rejection: None, + verdict: "SUCCESS".into(), })) } } @@ -8277,6 +8278,7 @@ mod tests { accepted: false, transaction_id: None, rejection: Some("DOUBLE_SPEND".into()), + verdict: "FAILED".into(), }))); let outcome = refused .push_signed_bundle(&a_signed_bundle_hex()) @@ -10730,7 +10732,9 @@ mod tests { let silently_denying = FakePusher::answering(Ok(PushOutcome { accepted: false, transaction_id: None, + // A refusal that states NO reason -- the shape the #348 hold keys on. rejection: None, + verdict: "PENDING".into(), })); let be = backend_with(vec![spent_by_the_bundle.clone(), untouched.clone()], true) .await @@ -10832,6 +10836,7 @@ mod tests { accepted: false, transaction_id: None, rejection: Some("mempool said no".into()), + verdict: "FAILED".into(), })); let be = backend_with(vec![refused.clone()], true) .await From a2ce4a93d7b9b5b6dc657180b46c75f228f75658 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Mon, 31 Aug 2026 10:59:29 -0700 Subject: [PATCH 9/9] chore(release): 0.191.0 -- rebased past #458, which took 0.190.0 Six lanes each computed "main is at 0.189.0, so I take 0.190.0" while open concurrently. #458 merged first and took that number, so every sibling's bump became a no-op against the new main -- the rebase was clean, no conflict, no `dropping` line, and the commit log still reads as a bump. Only the file on disk showed it. Member crates are unaffected: #458 moved the workspace version alone, so dig-node-core 0.65.0 and dig-wallet 0.44.0 are still ahead of main. --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8ea14457..461762e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.190.0" +version = "0.191.0" dependencies = [ "async-trait", "axum", diff --git a/Cargo.toml b/Cargo.toml index 1005fc3d..173dc22a 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.190.0" +version = "0.191.0" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over