From 4f52a2318df69eb927b5d67b697c224fec20a377 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 1 Sep 2026 20:55:08 -0700 Subject: [PATCH 01/19] chore(mirror): open the peer-binding lane for #473 Stub anchor so a session cap cannot lose the lane. Activation of the bond verifier follows. Co-Authored-By: Claude --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index effdd8e9..44380d78 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.236.0" +version = "0.244.0" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over From 99a792b0bfd921bef27a800a3e521b47d2f8640b Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 1 Sep 2026 21:41:24 -0700 Subject: [PATCH 02/19] feat(mirror): activate bond promotion on the coin's own peer declaration The bond verifier shipped in #467 was inert by construction: `peer_declaration` returned `NotReadable` unconditionally, so `Bonded` was unreachable, `verdict_for` short-circuited before any chain read, and every holder got one verdict. That was the correct posture while nothing could bind a coin to a claimant -- a coin proves that *a* bond exists and never that the peer offering the record holds it, and promoting on the chain half alone would rank a stranger republishing a public coin id first at zero collateral. `dig-mirror-coin` 0.8.0 supplies the missing half. A coin's owner may declare `dig-peer:<64-hex>` in the memo tail; only the owner's key can produce the spend that writes it, so the term is an owner attestation carried by executed on-chain code. `peer_declaration` now delegates to that crate's typed accessor rather than parsing the tail here, because a second parser for a security-critical format makes a divergence a silent authorization difference instead of a compile error. Promotion now requires BOTH bindings: coin -> content via `MirrorCoin::advertises`, and coin -> peer id via the declaration. `PeerDeclaration::NotReadable` is removed. It described a situation that no longer exists, and a variant nothing constructs is a state the type claims to model and does not. The address-substitution residual, resolved ---------------------------------------------------------------- The declaration binds coin -> peer id, never peer id -> address, so a record carrying an honest holder's peer id, that holder's real coin id and an ATTACKER's addresses satisfies every check here and IS promoted. SPEC 25.6a previously required closing that with an authoritative-record restriction, on the stated grounds that "a dialler is not by itself a backstop, because peer ids are derived from the presented certificate rather than pinned against the dialled identity". That premise is false for every path dig-node dials on: the download path makes the record's own `provider_peer_id` the `PeerTarget` pin, dig-nat passes it to dig-tls, and the verifier fails the handshake with `peer_id mismatch: expected .., got ..`. dig-peer re-checks after connect, and fetched content is merkle-verified against the caller's own requested root regardless. The attacker buys a refused connection, not a redirected reader. The restriction as written is also not implementable at this layer, and that is worth recording rather than rediscovering: dig-dht really does keep authoritative and hearsay records in two separate stores, but erases the distinction in `merge_dedup_by_provider` before `find_providers` returns, and a locator restricted to authoritative records would return almost nothing -- that store holds keys this node is k-closest to, not content it wants. What this layer owes instead is a BOUND, and it is added here: at most one record is promoted per claimed peer id, so one stolen identity cannot spend the whole verified budget. A duplicate falls back to the baseline tier it would have occupied with no verifier at all, never below it, so the lattice stays credit-only. Also corrects `peer.rs`'s note asserting no dial pins a peer id. The narrow fact behind it -- dig-gossip's legacy rustls outbound does not pin, and every `expected_peer_id` there is `#[cfg(test)]` -- is true; the generalisation to every dial was not, and dig-node never dials on that path. Closes #473 Closes #466 Co-Authored-By: Claude --- crates/dig-node-core/src/mirror_bond.rs | 59 ++++- crates/dig-node-core/src/peer.rs | 19 +- crates/dig-node-service/Cargo.toml | 2 +- .../src/mirror/bond_verify.rs | 208 +++++++++++------- 4 files changed, 190 insertions(+), 98 deletions(-) diff --git a/crates/dig-node-core/src/mirror_bond.rs b/crates/dig-node-core/src/mirror_bond.rs index 1741474d..6fa3ea42 100644 --- a/crates/dig-node-core/src/mirror_bond.rs +++ b/crates/dig-node-core/src/mirror_bond.rs @@ -5,19 +5,36 @@ //! at no cost, bonding nothing. Until this module existed nothing anywhere read that field against a //! chain, so the collateral economy's one economic guarantee was unenforced end to end. //! -//! # This layer is INERT today, and that is the safe posture +//! # This layer is LIVE, and what makes promotion sound //! -//! No [`MirrorBondVerifier`] this node can build returns `Bonded` yet, because a coin proves that -//! *a* bond exists and never that the *claimant* holds it, and nothing here can yet bind a mirror -//! coin's owner to a DHT peer id. So every holder receives the same verdict and the ranking below is -//! a no-op on every slate: no reordering, and the host's implementation declines the chain read -//! rather than paying for an answer this module would discard. +//! Promotion requires TWO independent bindings, and neither is sufficient alone. //! -//! Promoting on the chain half alone is the alternative, and it is an attack: coin ids travel the -//! DHT in cleartext by design, so a stranger republishing an honest holder's id would rank first at -//! zero collateral. Withholding credit from everyone is the only posture that is neither exploitable -//! nor expensive. The binding is ; when it lands -//! this module needs no change. +//! 1. **coin -> content**, from `MirrorCoin::advertises`: the coin declares exactly this +//! `(store, root, epoch)`, with the hint recomputed from the coin's own lineage proof. +//! 2. **coin -> peer id**, from the coin's owner-written `dig-peer:` declaration +//! (`dig-mirror-coin` 0.8.0, its `SPEC.md` §5.1). Memos are written by the spend that creates the +//! coin and only the owner's key can produce that spend, so the term is an owner attestation +//! carried by executed on-chain code. +//! +//! Without (2), promotion is itself an attack: coin ids travel the DHT in cleartext by design, so a +//! stranger republishing an honest holder's id would rank first at zero collateral. That is why this +//! module shipped inert until dig-node#473 supplied the declaration. +//! +//! # The binding this module does NOT make, and who does +//! +//! (2) binds a coin to a `peer_id`. It does **not** bind that `peer_id` to the addresses beside it +//! in the provider record, and no chain read can: a provider record is unsigned, and dig-dht says so +//! outright. A record carrying an honest holder's peer id, that holder's real coin id, and an +//! ATTACKER's addresses therefore satisfies everything above and IS promoted here. +//! +//! It is refused one layer down, by the transport, which pins the claimed `peer_id` against the +//! certificate the far end actually presents — `dig-download`'s `provider_peer_id` becomes the +//! `PeerTarget` pin, enforced in `dig-tls`'s verifier as `peer_id mismatch: expected …, got …`, with +//! `dig-peer` re-checking it after connect. So the attacker buys a failed handshake, not a served +//! byte, and the content is merkle-verified against the caller's own requested root regardless. +//! +//! What this layer owes in return is a BOUND: at most one record is promoted per claimed peer id, so +//! a single stolen identity cannot occupy every promoted slot. See `SPEC.md` §25.6a. //! //! # What lives here, and what deliberately does not //! @@ -193,6 +210,7 @@ impl ProviderLocator for BondRankingLocator { // not to: a comparison-driven lookup would read the same coin O(n log n) times. let mut ranked: Vec<(u8, ProviderRecord)> = Vec::with_capacity(found.len()); let mut verified = 0usize; + let mut promoted_peers: std::collections::HashSet = std::collections::HashSet::new(); for record in found { let claimed = record.unverified_mirror_coin_id_bytes(); // A holder that claims nothing, and every record past the budget, keeps its place with @@ -205,6 +223,23 @@ impl ProviderLocator for BondRankingLocator { let verdict = verifier .verify(content, &record.provider_peer_id, claimed) .await; + let mut rank = credit_rank(verdict); + // At most ONE record is promoted per claimed peer id. A coin declares one peer and a + // peer needs its own collateralised coin, so promotion is meant to cost collateral -- + // but nothing stops a stranger republishing one honest holder's peer id and coin id + // across the whole slate with addresses of its choosing. Each copy satisfies the + // declaration check on the strength of the same single bond, and without this the + // budget's worth of promoted slots could all be spent on one stolen identity. + // + // This is a BOUND, not a punishment: a duplicate falls back to the baseline tier it + // would have occupied with no verifier at all, never below it, so the lattice stays + // credit-only. It also costs an honest holder nothing -- a peer that legitimately + // announces twice keeps its first record promoted. + if rank == credit_rank(BondVerdict::Bonded) + && !promoted_peers.insert(record.provider_peer_id.clone()) + { + rank = credit_rank(BondVerdict::Unverified); + } if verdict == BondVerdict::Unbonded { // Worth an operator's attention and nobody's ban list: this record's own pointer // disproves its own claim. Logged and NOT demoted — the record may be a stranger's @@ -214,7 +249,7 @@ impl ProviderLocator for BondRankingLocator { "located holder's claimed mirror coin does not bond this content; no promotion" ); } - ranked.push((credit_rank(verdict), record)); + ranked.push((rank, record)); } // STABLE, so holders sharing a tier keep the order their source gave them. The download diff --git a/crates/dig-node-core/src/peer.rs b/crates/dig-node-core/src/peer.rs index eded5913..0fa52bc7 100644 --- a/crates/dig-node-core/src/peer.rs +++ b/crates/dig-node-core/src/peer.rs @@ -860,12 +860,19 @@ pub use crate::shared::identity::{install_crypto_provider, load_or_generate_node /// peer-RPC server, the DHT dials, and the gossip pool). Without this the pool would present a cert /// hashing to a different peer_id than the one this node advertises (#1532). /// -/// **A dialler is not guaranteed to notice.** `dig-gossip` DERIVES a peer id from the presented SPKI -/// and does not compare it against the one it dialled — every `expected_peer_id` occurrence in that -/// crate is inside `#[cfg(test)]` (DIG-Network/dig-gossip#85). So a split identity is not caught by a -/// fail-closed handshake in the general case; where pinning does happen it is the dialler's own -/// (`dig-tls::pin_and_bind`, used by the ping path). Do not rely on a mismatch being refused. dig-gossip only READS these files (`dig_peer_protocol::load_ssl_cert`), so pointing at the -/// canonical identity files can never clobber them. +/// **A dialler DOES notice, on every path this node is dialled over.** An earlier version of this +/// note said the opposite, and it was wrong in a way worth stating: it generalised one true fact +/// about `dig-gossip` into a claim about every dial. The true fact is narrow — `dig-gossip`'s legacy +/// rustls outbound derives a peer id from the presented SPKI without comparing it to the one it +/// dialled, and every `expected_peer_id` in that crate is inside `#[cfg(test)]` +/// (DIG-Network/dig-gossip#85). But dig-node never dials on that path. Its dials go through +/// `dig-nat`, which passes the expected id to `dig-tls`; the verifier compares it against the leaf +/// the far end presents and fails the handshake with `peer_id mismatch: expected <..>, got <..>`, +/// and `dig-peer` re-checks it after connect. The download path pins the same way, from a provider +/// record's own `provider_peer_id`. So a split identity IS caught fail-closed here, which is exactly +/// why keeping these files canonical matters. dig-gossip only READS them +/// (`dig_peer_protocol::load_ssl_cert`), so pointing at the canonical identity files can never +/// clobber them. /// /// [`node_cert_dir`]: crate::seams::key_mgmt::key_manager::KeyManager::node_cert_dir fn gossip_identity_paths(node_cert_dir: &std::path::Path) -> (String, String) { diff --git a/crates/dig-node-service/Cargo.toml b/crates/dig-node-service/Cargo.toml index 873101b0..669ca5ea 100644 --- a/crates/dig-node-service/Cargo.toml +++ b/crates/dig-node-service/Cargo.toml @@ -107,7 +107,7 @@ dig-mirror-collateral = "0.3" # The chain half of the same model: `census` counts the collateralised network at a block height # and hands `dig-mirror-collateral` the three integers its controller consumes (dig-node#400). # Without it a node could only ever record epoch 1, which is derivable from nothing. -dig-mirror-coin = "0.7" +dig-mirror-coin = "0.8" # The canonical `ChainSource` trait `dig-mirror-coin`'s census is generic over. Declared, not # implemented: `chia-query` already provides the implementation this node uses diff --git a/crates/dig-node-service/src/mirror/bond_verify.rs b/crates/dig-node-service/src/mirror/bond_verify.rs index 821ee1f7..78e34e53 100644 --- a/crates/dig-node-service/src/mirror/bond_verify.rs +++ b/crates/dig-node-service/src/mirror/bond_verify.rs @@ -105,11 +105,9 @@ impl VerdictKey { /// What a mirror coin's advertised terms say about the peer claiming it. /// -/// `DeclaresThisPeer` and `Silent` are matched but not yet constructed: nothing can construct them -/// until [`peer_declaration`] has a typed source for the binding, which is exactly the promotion gate -/// described there. They are written now so the shape of the answer is fixed before the source -/// arrives, and so the call site reads as the full decision rather than a placeholder. -#[allow(dead_code)] +/// Two answers, not three. Until `dig-mirror-coin` 0.8.0 there was a `NotReadable` state for "this +/// node has no way to read a declaration at all"; the typed accessor removed the situation, so the +/// variant was removed with it rather than left as a case nothing constructs. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum PeerDeclaration { /// The coin's owner declared this exact peer, in code the chain executed. @@ -117,36 +115,30 @@ pub(crate) enum PeerDeclaration { /// The coin declares some other peer, or none. Credit is withheld — never subtracted, because /// the record naming this coin may be a stranger's lie ABOUT the coin's real holder. Silent, - /// This node cannot read the declaration at all, so it knows nothing either way. - NotReadable, } /// Whether `advertised_terms` — the free tail of a mirror coin's memo — declares `claiming_peer_id`. /// -/// **This is deliberately unreadable today, and that is the promotion gate.** The tail is arbitrary -/// UTF-8 and `MirrorCoin::urls()` already hands it over, so a `dig-peer:<64-hex>` term COULD be -/// parsed here — and must not be. `dig-mirror-coin` 0.8.0 is about to own that format with a typed -/// accessor; parsing it here would create a second parser for a security-critical format, in the -/// consumer, where a divergence between the two would be a silent authorization difference rather -/// than a compile error (CLAUDE.md 2.0, centralize rival implementations). +/// The answer comes from `dig-mirror-coin`'s typed accessor and is NOT parsed here. A second parser +/// for this format, living in the consumer, would make a divergence between the two a silent +/// authorization difference rather than a compile error (CLAUDE.md §2.0, centralize rival +/// implementations). Everything the format means — exact prefix matching, byte-wise peer-id +/// comparison, and the rule that a coin carrying two declarations declares NOBODY — is stated once, +/// in that crate's `SPEC.md` §5.1. /// -/// So promotion is switched OFF until that accessor exists: with no sound source for the -/// coin -> peer binding, `Bonded` cannot be established, and the layer withholds credit from -/// everyone rather than granting it on a check it cannot make. Nothing is demoted by this -/// (mirror_bond's lattice is credit-only), so the interim behaviour is exactly the behaviour of a -/// node with no verifier at all. -/// -/// Replacing the body with the 0.8.0 accessor turns promotion on, and three things MUST land in -/// that same change, never after: the authoritative-record restriction on dig-node#466, the -/// claiming peer id staying part of [`VerdictKey`] (a peer-agnostic key would serve one peer's -/// earned `Bonded` to a stranger republishing the same public coin id), and the cost analysis on -/// [`declaration_source_is_readable`], whose short-circuit lifts itself the moment this function -/// can answer. +/// **What a `DeclaresThisPeer` establishes.** Memos are written by the spend that creates the coin +/// and only the owner's key can produce that spend, so the term is an owner attestation carried by +/// executed on-chain code. It binds the coin to a `peer_id`. It does NOT bind that `peer_id` to the +/// addresses travelling beside it in the provider record — see `SPEC.md` §25.6a for why that gap is +/// closed by the transport rather than here. pub(crate) fn peer_declaration( - _advertised_terms: &[String], - _claiming_peer_id: &str, + advertised_terms: &[String], + claiming_peer_id: &str, ) -> PeerDeclaration { - PeerDeclaration::NotReadable + match dig_mirror_coin::declared_peer(advertised_terms) { + Some(declared) if declared.names(claiming_peer_id) => PeerDeclaration::DeclaresThisPeer, + _ => PeerDeclaration::Silent, + } } /// Whether [`peer_declaration`] can bind a coin to a peer AT ALL — probed through the real @@ -320,7 +312,7 @@ pub fn verdict_for( }; match peer_declaration(mirror.urls(), claiming_peer_id) { PeerDeclaration::DeclaresThisPeer => BondVerdict::Bonded, - PeerDeclaration::Silent | PeerDeclaration::NotReadable => BondVerdict::Unverified, + PeerDeclaration::Silent => BondVerdict::Unverified, } } @@ -565,11 +557,11 @@ mod tests { }; use std::sync::atomic::{AtomicUsize, Ordering as AtomicOrdering}; - /// A chain that counts every read reaching it and answers nothing. + /// A chain that counts every read reaching it and answers "no such coin". /// - /// Answering nothing is deliberate: the property under test is that the source is not consulted - /// AT ALL, so a double that could satisfy a read would let a short-circuit that merely fails - /// fast look identical to one that never asks. + /// Answering nothing is deliberate. It makes the read COUNT the only variable: a claim against + /// this source is disproven at the first step, so any count above one is a retry loop or a + /// second question, both of which this module owes the network not to perform. struct CountingChain { reads: Arc, } @@ -677,31 +669,35 @@ mod tests { let terms = vec![format!("dig-peer:{claiming_peer_id}")]; match peer_declaration(&terms, claiming_peer_id) { PeerDeclaration::DeclaresThisPeer => BondVerdict::Bonded, - PeerDeclaration::Silent | PeerDeclaration::NotReadable => BondVerdict::Unverified, + PeerDeclaration::Silent => BondVerdict::Unverified, } } } - /// **Proves (dig-node#466, HIGH finding 2 — the residual the credit-only lattice does NOT - /// close):** a hearsay record naming an honest holder's peer id AND its real coin id, but - /// carrying the ATTACKER's addresses, is not promoted — so the address a redirected reader - /// dials is unchanged by it. + /// **Proves (dig-node#473):** a record carrying an honest holder's peer id and real coin id but + /// the ATTACKER's addresses IS promoted here — and the promotion is bounded to ONE slot however + /// many copies of it a slate contains. /// - /// **Catches:** the hole neither other test can see. Every field of this record is honest - /// except the one that matters, so a coin-id check passes, a peer-id check passes, and the peer - /// id is IDENTICAL in the passing and failing versions of the code — which is why the assertion - /// is on the addresses. `stop_on_providers` means one answer can be the whole slate, so a - /// promotion here puts the attacker's host first for every reader that trusts the ranking, and - /// the dial does not pin the peer id (DIG-Network/dig-gossip#85) to catch it afterwards. + /// **This test previously asserted the opposite, and it was wrong for a measured reason.** Its + /// premise was that "the dial does not pin the peer id (dig-gossip#85) to catch it afterwards". + /// That is true only of `dig-gossip`'s legacy rustls outbound, which dig-node never dials on: + /// every dial it makes passes the expected id through `dig-nat` to `dig-tls`, whose verifier + /// fails the handshake with `peer_id mismatch`. So the attacker's addresses buy a refused + /// connection, not a redirected reader, and the honest promotion is not a hole. /// - /// The verifier is driven through the REAL [`peer_declaration`] gate rather than a hand-written - /// verdict, so this test measures production's answer: with no sound coin -> peer binding - /// available, nothing is promoted at all. + /// **Catches:** the bound going missing. The coin binds coin -> peer id and never peer id -> + /// address, so nothing in this layer can tell the honest holder's record from the attacker's + /// copy of it — both name the same peer and the same real coin. What this layer CAN do is refuse + /// to spend more than one promoted slot on one claimed peer id, which is what keeps a single + /// stolen identity from filling the whole verified budget. The slate below carries three copies + /// and one ordinary holder, so a missing dedup shows up as three promotions instead of one. #[tokio::test] - async fn an_honest_peer_id_with_attacker_addresses_is_not_promoted() { + async fn one_stolen_identity_cannot_occupy_more_than_one_promoted_slot() { let slate = Slate(vec![ - holder_at(0xCC, None, "honest.example"), // an ordinary holder, no pointer - holder_at(0xAA, Some([0x01; 32]), "attacker.example"), + holder_at(0xCC, None, "honest-no-pointer.example"), + holder_at(0xAA, Some([0x01; 32]), "attacker-1.example"), + holder_at(0xAA, Some([0x01; 32]), "attacker-2.example"), + holder_at(0xAA, Some([0x01; 32]), "attacker-3.example"), ]); let slot = bond_verifier_slot(); let _ = slot.set(Arc::new(EveryChainCheckPasses)); @@ -712,9 +708,13 @@ mod tests { assert_eq!( hosts, - vec!["honest.example", "attacker.example"], - "a record whose peer id and coin id are both honest must still not promote the \ - addresses it chose for itself" + vec![ + "attacker-1.example", + "honest-no-pointer.example", + "attacker-2.example", + "attacker-3.example", + ], + "exactly one copy is promoted; the rest fall back to baseline in source order, never below it" ); } @@ -755,19 +755,16 @@ mod tests { ); } - /// **Proves (dig-node#466, security F1):** no chain is read at all while nothing can bind a - /// coin to a peer. + /// **Proves (dig-node#466 / #473):** a wrong pointer costs the verifier exactly ONE chain read, + /// with no retry loop — the cost the ticket requires be borne by the publisher, not the reader. /// - /// **Catches:** the amplifier. The production `ChainSource` reaches `api.coinset.org`, and a - /// `Bonded` that degrades to `Unverified` is the one verdict the cache refuses to hold — so - /// each read is re-paid on every locate, up to the locate budget, for an answer the stable - /// credit-only sort provably discards. The counting source makes the absence of that egress an - /// assertion rather than a claim. - /// - /// The count is asserted at zero AND the verdict at `Unverified`, so a short-circuit that - /// changed the answer would fail here rather than pass quietly. + /// **Catches:** the amplifier. The production `ChainSource` reaches `api.coinset.org`, the coin + /// id is chosen by whoever wrote the provider record, and `Unbonded` is a verdict a stranger can + /// elicit for free. A retry, or a second question asked of a claim already disproven, multiplies + /// attacker-directed egress by the locate budget. The count is asserted alongside the verdict so + /// a change that stopped reading altogether would fail here rather than pass quietly. #[test] - fn nothing_is_read_from_the_chain_while_the_declaration_has_no_source() { + fn a_disproven_pointer_costs_exactly_one_chain_read() { let reads = Arc::new(AtomicUsize::new(0)); let source = CountingChain { reads: Arc::clone(&reads), @@ -785,34 +782,87 @@ mod tests { assert_eq!( verdict, - BondVerdict::Unverified, - "withholding credit is the answer the short-circuit must preserve" + BondVerdict::Unbonded, + "the chain answered and there is no such coin, which disproves the claim" ); assert_eq!( reads.load(AtomicOrdering::Relaxed), - 0, - "a verdict that cannot be `Bonded` must cost no chain read" + 1, + "one read settles it; anything more is a retry loop paid for by the reader" ); + } + + /// **Proves (dig-node#473):** the declaration source is LIVE, so the pre-read short-circuit no + /// longer withholds every verdict — and the probe that lifts it is a genuine fail-closed + /// self-test rather than a switch someone must remember to flip. + /// + /// **Catches:** a silent regression in the format agreement. The probe asks the production + /// [`peer_declaration`] for the one term a coin owned by `probe_peer` would carry. If a future + /// `dig-mirror-coin` changed the declaration format, or the accessor stopped answering, this + /// goes false and `verdict_for` returns to withholding credit from everyone — the safe + /// direction — instead of promoting on a check it can no longer make. Asserting it TRUE here is + /// what makes that failure visible as a red test rather than as silently inert ranking. + #[test] + fn the_declaration_source_is_live_and_its_probe_is_a_fail_closed_self_test() { assert!( - !declaration_source_is_readable(), - "control: the short-circuit is active precisely because the source is unreadable — \ - when 0.8.0's accessor lands this flips and the reads resume" + declaration_source_is_readable(), + "the typed accessor must answer, or promotion is unreachable for every input" + ); + + let peer = "aa".repeat(32); + assert_eq!( + peer_declaration(&[format!("dig-peer:{peer}")], &peer), + PeerDeclaration::DeclaresThisPeer, + "control: the probe and the production path are the same function" ); } + /// **Proves (dig-node#473):** only the coin's own declaration of THIS claimant promotes it. + /// + /// **Catches:** the binding degrading to "some coin bonds this content", which is the weaker + /// question a stranger republishing a public coin id passes. Every row shares one claimant and + /// varies only what the coin says, so a check that ignored the terms would answer identically + /// for all of them. #[test] - fn no_visible_term_promotes_a_claim_before_the_typed_accessor_exists() { + fn only_the_coins_own_declaration_of_this_claimant_promotes_it() { let peer = "aa".repeat(32); - for terms in [ - vec![], + let other = "bb".repeat(32); + + let promotes = [ vec![format!("dig-peer:{peer}")], - vec![format!("dig-peer:{}", "bb".repeat(32))], - vec!["https://mirror.example/store".to_string()], - ] { - assert_ne!( + vec![ + "https://mirror.example/store".to_string(), + format!("dig-peer:{peer}"), + ], + // The owner wrote the id in the other case; it denotes the same SHA-256. + vec![format!("dig-peer:{}", peer.to_uppercase())], + ]; + for terms in promotes { + assert_eq!( peer_declaration(&terms, &peer), PeerDeclaration::DeclaresThisPeer, - "promotion must stay unreachable until the binding has a typed source; terms {terms:?}" + "terms {terms:?}" + ); + } + + let withholds = [ + vec![], + vec!["https://mirror.example/store".to_string()], + // Someone else's coin, republished by this claimant. + vec![format!("dig-peer:{other}")], + // Two declarations name nobody -- one coin's collateral must not back two peers. + vec![format!("dig-peer:{peer}"), format!("dig-peer:{other}")], + // Prefix lookalikes are ordinary advertised strings. + vec![format!("xdig-peer:{peer}")], + vec![format!("dig-peers:{peer}")], + // A payload that is not a peer id. + vec!["dig-peer:nope".to_string()], + ]; + for terms in withholds { + assert_eq!( + peer_declaration(&terms, &peer), + PeerDeclaration::Silent, + "terms {terms:?}" ); } } From b506a4f9f131a6bab3e5a65b85d4fe6de04e98fd Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 1 Sep 2026 21:53:23 -0700 Subject: [PATCH 03/19] docs(spec): 25.6a states the pin as the remedy, and two status bullets stop lying Three normative corrections in the section a reimplementation of the bond layer would be built from. No behaviour changes. 25.6a required an authoritative-record restriction and dismissed the alternative because "a dialler is not by itself a backstop, since peer ids are derived from the presented certificate rather than pinned against the dialled identity". That was FALSE. It generalised one true narrow fact -- dig-gossip's legacy rustls outbound does not pin, and every `expected_peer_id` there is test-only -- into a claim about every dial. dig-node never dials on that path: the download path makes a record's own `provider_peer_id` the pinned dial target, dig-nat passes it to dig-tls, and the verifier refuses the handshake on a mismatch. The restriction the clause preferred is also not available at the layer that ranks, and requiring it as though it were is worse than not requiring it. A DHT keeps attributed and hearsay records apart but flattens them into one untagged list when answering a lookup, and a reader's records for content it wants are overwhelmingly hearsay -- the attributed store covers the keys a node is closest to, not what it fetches. Restricting the locator to attributed records would return almost nothing. What the ranking layer owes instead is now stated as a MUST: at most one record promoted per claimed peer id per locate, so one stolen identity cannot occupy every promoted slot on the strength of a single bond. Credit-only is explicitly preserved. The two status bullets were separately stale. Verification is no longer inert. And the DHT pointer IS attached -- the `dig-dht ^0.13`/`0.15` semver split that blocked it is resolved, the announce passes a coin id (`dht.rs:493`), and `SnapshotMirrorPointers` is installed at `server.rs:2169`. A bullet saying a mirror coin id never reaches the DHT would have made the whole verification path read as unreachable. Refs #473 Co-Authored-By: Claude --- SPEC.md | 71 ++++++++++++++++++++---------- crates/dig-node-service/Cargo.toml | 2 +- 2 files changed, 49 insertions(+), 24 deletions(-) diff --git a/SPEC.md b/SPEC.md index 0d1719f5..1c3dca03 100644 --- a/SPEC.md +++ b/SPEC.md @@ -8332,24 +8332,24 @@ itself (SYSTEM.md §4.1). > by name BEFORE any chain read (dig-node#426). **RECLAIMS are implemented** and are supported at `fee = 0` with > no fee coins, which is §25.4.4 — and are never gated on any funding read, including the > committed-coin read. -> * **§25.10's verification of OTHER peers' claims is BUILT BUT INERT — no claim is verified on a -> running node today.** The mechanism is present and wired: `dig-node-core`'s `mirror_bond` (the +> * **§25.10's verification of OTHER peers' claims is LIVE.** `dig-node-core`'s `mirror_bond` (the > three verdicts and the ranking locator, installed inside `NodeContent::new`) and -> `dig-node-service`'s `mirror/bond_verify.rs` (the chain read, installed on the running node by -> `spawn_bond_verifier_install`). But this node has no sound source for the coin-to-peer binding -> §25.6a requires, so `bonded` is unreachable for every input, the chain read is short-circuited -> before it is paid, and every holder receives the same verdict — the ranking is a no-op on every -> slate. **A reader must not take this bullet as saying collateral is enforced; it is not.** The -> binding is tracked as , and promotion -> becomes reachable the moment it lands, with no further change here. What is verified once it does -> is a peer's claim; this node still attaches no pointer of its own, per the next bullet. -> * **§25.6's DHT pointer is not attached.** `ProviderRecord::unverified_mirror_coin_id` lives in -> dig-dht 0.15, and `dig-download` 0.21.0 and `dig-peer-selector` 0.10.0 both require -> `dig-dht ^0.13` — semver-incompatible on a `0.x` line, so taking 0.15 here would resolve two -> dig-dht lines. Tracked as . +> `dig-node-service`'s `mirror/bond_verify.rs` (the chain read, installed by +> `spawn_bond_verifier_install`) are wired, and `peer_declaration` now reads `dig-mirror-coin` +> 0.8.0's typed `dig-peer:` accessor, so `bonded` is reachable and a promoted holder has proven both +> halves §25.6a requires. **What a reader MUST NOT infer is that any coin on chain carries a +> declaration yet.** The format is new, and a coin acquires one only when its owner recreates it at +> an epoch boundary; until an owner does, that holder is `unverified` and sits at baseline. That is a +> stated degradation and not a lie about enforcement: no honest holder is ranked below where no +> verifier at all would put it. +> * **§25.6's DHT pointer IS attached.** This bullet previously said it was not, blocked on a +> `dig-dht ^0.13` / `0.15` semver split; that split is resolved, `dig-node-core` takes dig-dht 0.15, +> and the announce passes a coin id (`seams/dig_peer/dht.rs:493`, +> `announce_provider_with_collateral`). The production pointer source is +> `mirror/pointers.rs`'s `SnapshotMirrorPointers`, installed at `server.rs:2169`. A reader MUST still +> treat the field as the untrusted claim its name says it is. > -> So a reader MUST NOT infer that any coin is CREATED at this head, and MUST NOT infer that a mirror -> coin id reaches the DHT. +> So a reader MUST NOT infer that any coin is CREATED at this head. > > **A clause not named in the list above MUST be read as pending, whatever its grammatical voice**, > and the list is to be read NARROWLY: where an entry names a file or a function, it satisfies the @@ -8768,13 +8768,38 @@ to an address, and a provider record's `peer_id`-to-address association is unaut that no chain read can settle. Such a record satisfies the declaration check and would be promoted on the strength of somebody else's bond. -Closing that requires a separate restriction, which a node performing promotion MUST apply: promote -only from a record whose `peer_id`-to-address association is itself authoritative — a first-hand -announce from the peer being ranked, not a slate forwarded by a third party — or defer the credit -until the dialled identity has been checked against the claimed `peer_id`. A dialler is not by itself -a backstop, because peer ids are derived from the presented certificate rather than pinned against -the dialled identity; the residual an unrestricted implementation carries is traffic redirection, not -a stolen bond. +Closing that requires the promoted address to be checked against the promoted identity. **A node +performing promotion MUST dial with the claimed `peer_id` PINNED**, refusing the connection when the +certificate the far end presents hashes to anything else. This is the operative remedy here and it +holds: the download path makes a record's own `provider_peer_id` the dial target's pinned id, the +NAT dialler passes it into the TLS client config, and the verifier fails the handshake with +`peer_id mismatch`. So the attacker in the paragraph above buys a refused connection rather than a +redirected reader, and the content a reader does accept is merkle-verified against the root it asked +for regardless. + +**An earlier version of this clause said a dialler is not by itself a backstop, "because peer ids are +derived from the presented certificate rather than pinned against the dialled identity". That was +false**, and is recorded here because a reader reasoning from it would over-trust the alternative. It +generalised one true fact — `dig-gossip`'s legacy rustls outbound does not pin, and every +`expected_peer_id` in that crate is test-only — into a claim about every dial. A node MUST NOT rank on +a declaration if its own transport lacks the pin; a node whose transport has it MUST NOT be required +to do more. + +The alternative that clause offered — promote only from a record whose `peer_id`-to-address +association is authoritative, a first-hand announce rather than a forwarded slate — is **NOT +available at the layer that ranks, and a specification MUST NOT require it as though it were.** A DHT +keeps attributed and hearsay records apart, but flattens them into one untagged list when answering a +lookup, and the records a reader holds for content it actually wants are overwhelmingly the hearsay +ones: the attributed store is populated for the keys a node is closest to, not for what it fetches. A +locator restricted to attributed records would return almost nothing, which degrades discovery +without improving safety the pin already provides. + +What the ranking layer MUST do instead is BOUND the credit: **at most one record is promoted per +claimed `peer_id` in one locate.** The declaration binds a coin to a peer id and cannot distinguish an +honest holder's record from a stranger's copy of it, so without this a single stolen identity could +occupy every promoted slot on the strength of one bond. A record refused promotion by this bound +takes the baseline tier it would have had with no verifier at all, never below it — the credit-only +rule above is not weakened by it. **One locate is bounded work.** The size of a located set is chosen by whoever answered the lookup, so a node MUST bound the number of bonds it reads against a chain per locate, verifying in source diff --git a/crates/dig-node-service/Cargo.toml b/crates/dig-node-service/Cargo.toml index 669ca5ea..d4674876 100644 --- a/crates/dig-node-service/Cargo.toml +++ b/crates/dig-node-service/Cargo.toml @@ -332,7 +332,7 @@ chia-protocol = "0.36.1" # hand-written `CoinRecord` cannot exercise the authentication path the census runs, so the probe # would assert its property against a fixture that could not exhibit it. # -# Every version here is the one `dig-mirror-coin` 0.7 itself compiles against, and the whole set +# Every version here is the one `dig-mirror-coin` 0.8 itself compiles against, and the whole set # moves together: the chia ceiling is not one number, and a crate split across two chia lines # compiles only until something crosses a public signature. chia-puzzle-types = "0.36.1" From 4f5ff49c5e3b6395c1c3f94643e56060438f347c Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 1 Sep 2026 22:27:01 -0700 Subject: [PATCH 04/19] fix(mirror): bound promotion by peer IDENTITY, and write this node's own declaration Three findings from the adversarial gate, one of them a live zero-cost attack on the bound added earlier in this branch. The bound was keyed on the raw wire string ---------------------------------------------------------------- `promoted_peers` used `record.provider_peer_id` verbatim, while every check that GRANTS a promotion is case-insensitive: the coin's declaration compares 32 decoded bytes, and the TLS pin compares 32 bytes of certificate hash. A peer id is fixed-length hex, so one identity has many spellings. So a stranger answering one lookup could return eight records carrying an honest holder's peer id in eight different hex cases, each with its own addresses. Each passes `advertises`, each passes `declares_peer`, each is a distinct `String` -- eight promotions and eight chain reads, consuming the whole `MAX_VERIFIED_PER_LOCATE` budget on the strength of one bond the attacker does not hold. Exactly the outcome the bound was written to prevent. Both the bound and `VerdictKey`'s claiming-peer component are now keyed on the ASCII-lowercased id. dig-dht applies this same normalisation to the neighbouring `unverified_mirror_coin_id`, for the reason its own doc gives: without it "dedup and equality would split on presentation". The wire-level gap is filed as DIG-Network/dig-dht#27 -- it also affects self-exclusion and the union address merge, both pre-existing. The regression test was vacuous when first written, and that is worth recording: with the honest holder LAST in the slate, a promoted respelling and a baseline one land in the same position under a stable sort, so it passed with the fix reverted. Moving the honest record between the two spellings makes the behaviours differ. Now revert-proven -- reverting the fix fails exactly one test, with the attacker's respelling ahead of the honest holder. A coin this node creates now names this node ---------------------------------------------------------------- `MirrorAdvertisement` gained the field, and dig-node had to answer it. A coin that declares nobody can never be promoted by any reader, so a node creating one pays collateral for a claim nothing can credit to it -- the feature would have been vacuous for every coin this node makes. `Node::own_peer_id` is threaded to the create, re-read per pass rather than captured at spawn, because the mirror task starts beside the peer network rather than after it. `None` still creates the coin and warns; refusing would leave a node unable to bond at all before its network is up. Two stale claims ---------------------------------------------------------------- `verdict_for`'s own doc still said "nothing is promoted today" and "No chain is read at all", in the commit that makes both false. And `peer.rs:2475` was an uncorrected twin of the dialler claim fixed at `:863` -- same refuted assertion, same file, in the code that configures the gossip pool. Fixing one copy of a false normative claim and leaving the other is how it comes back. Refs #473 Co-Authored-By: Claude --- crates/dig-node-core/src/lib.rs | 9 +++ crates/dig-node-core/src/mirror_bond.rs | 10 ++- crates/dig-node-core/src/peer.rs | 17 ++++- .../src/mirror/bond_verify.rs | 76 ++++++++++++++++--- .../dig-node-service/src/mirror/lifecycle.rs | 33 ++++++++ crates/dig-node-service/src/mirror/spends.rs | 7 ++ crates/dig-node-service/src/server.rs | 5 ++ .../tests/mirror_advertised_urls.rs | 6 ++ .../tests/mirror_intra_pass_reservation.rs | 6 ++ 9 files changed, 157 insertions(+), 12 deletions(-) diff --git a/crates/dig-node-core/src/lib.rs b/crates/dig-node-core/src/lib.rs index 5f007110..18cc88ef 100644 --- a/crates/dig-node-core/src/lib.rs +++ b/crates/dig-node-core/src/lib.rs @@ -4916,6 +4916,15 @@ impl Node { ) -> Option> { self.mirror_pointers.get().cloned() } + + /// This node's own `peer_id`, once its peer network has started. + /// + /// The mirror lifecycle needs it to write the peer declaration into a coin it creates: a coin + /// that names no peer bonds content for nobody in particular and can never be promoted, so a + /// node creating one is paying collateral for a claim no reader can credit to it. + pub fn own_peer_id(&self) -> Option { + self.peer_status.peer_id() + } } /// The COMPOSITION-ROOT upcasts (#1285 W1c — the locked "Option A" shape). `Node` stays ONE diff --git a/crates/dig-node-core/src/mirror_bond.rs b/crates/dig-node-core/src/mirror_bond.rs index 6fa3ea42..370eb915 100644 --- a/crates/dig-node-core/src/mirror_bond.rs +++ b/crates/dig-node-core/src/mirror_bond.rs @@ -210,6 +210,14 @@ impl ProviderLocator for BondRankingLocator { // not to: a comparison-driven lookup would read the same coin O(n log n) times. let mut ranked: Vec<(u8, ProviderRecord)> = Vec::with_capacity(found.len()); let mut verified = 0usize; + // Keyed on the ASCII-LOWERCASED peer id, never the raw wire string. A peer id is fixed-length + // hex, so its two spellings denote one identity -- and every check that GRANTS the promotion + // already knows that: the coin's declaration compares 32 decoded bytes, and the TLS pin + // compares 32 bytes of certificate hash. A set keyed on the text would therefore admit the + // same peer once per hex spelling, and one stolen identity could fill every promoted slot at + // zero collateral by varying case alone. dig-dht applies exactly this normalisation to the + // neighbouring `unverified_mirror_coin_id` field, for the reason its own doc gives: without + // it "dedup and equality would split on presentation". let mut promoted_peers: std::collections::HashSet = std::collections::HashSet::new(); for record in found { let claimed = record.unverified_mirror_coin_id_bytes(); @@ -236,7 +244,7 @@ impl ProviderLocator for BondRankingLocator { // credit-only. It also costs an honest holder nothing -- a peer that legitimately // announces twice keeps its first record promoted. if rank == credit_rank(BondVerdict::Bonded) - && !promoted_peers.insert(record.provider_peer_id.clone()) + && !promoted_peers.insert(record.provider_peer_id.to_ascii_lowercase()) { rank = credit_rank(BondVerdict::Unverified); } diff --git a/crates/dig-node-core/src/peer.rs b/crates/dig-node-core/src/peer.rs index 0fa52bc7..7786b137 100644 --- a/crates/dig-node-core/src/peer.rs +++ b/crates/dig-node-core/src/peer.rs @@ -229,6 +229,16 @@ impl PeerStatus { Arc::new(PeerStatus::default()) } + /// This node's own `peer_id`, once the peer network has started. + /// + /// `None` before start-up. A caller that must NAME this node — writing the peer declaration into + /// a mirror coin, say — has to tolerate that: the identity exists on disk from the first boot, + /// but this status only learns it when the network comes up, so an early caller MUST treat + /// `None` as "not yet" rather than as "this node has no identity". + pub fn peer_id(&self) -> Option { + self.peer_id.lock().unwrap().clone() + } + /// Mark the peer network running under `peer_id` (clears the last error). pub fn set_running(&self, peer_id: String) { self.running.store(true, Ordering::Relaxed); @@ -2472,8 +2482,11 @@ async fn run_peer_network(node: Arc) -> Result<(), String> { // pool's cert/key at the persisted `NodeCert` files themselves (dig-gossip only READS them); // letting the GossipService mint its OWN throwaway cert would hash to a DIFFERENT peer_id and // a dial to this node can reach a DIFFERENT identity than the one advertised (#1532 — the - // identity split). Not every dialler refuses that: dig-gossip derives the peer id and does not - // pin it (DIG-Network/dig-gossip#85), so this must be right at the source rather than caught. + // identity split). A pinning dialler DOES refuse that -- dig-gossip's production dials go + // through dig-nat, which pins the expected id in dig-tls -- but getting it right at the source + // is still what this does, because a caught mismatch is a refused connection rather than a + // working one. (dig-gossip's own legacy rustls outbound does not pin, DIG-Network/dig-gossip#85, + // but nothing here dials on it.) // `cfg.peer_id` is set to that same identity so the pool's self-dial guard + handshake agree. // The address book (`peers.json`) stays under `peer-net/`; only the identity is shared. let gossip_dir = node.peer_cert_dir(); diff --git a/crates/dig-node-service/src/mirror/bond_verify.rs b/crates/dig-node-service/src/mirror/bond_verify.rs index 78e34e53..898d5fdb 100644 --- a/crates/dig-node-service/src/mirror/bond_verify.rs +++ b/crates/dig-node-service/src/mirror/bond_verify.rs @@ -91,8 +91,13 @@ impl VerdictKey { epoch: u64, claiming_peer_id: &str, ) -> Self { + // ASCII-lowercased before hashing. A peer id is fixed-length hex, so its two spellings are + // one identity, and `peer_declaration` already treats them as one because it compares + // decoded bytes. Hashing the raw text would give each spelling its own cache entry, so a + // stranger could multiply the chain reads one claim costs simply by varying case -- turning + // a memo into an amplifier against `api.coinset.org`. let mut hasher = chia_sha2::Sha256::new(); - hasher.update(claiming_peer_id.as_bytes()); + hasher.update(claiming_peer_id.to_ascii_lowercase().as_bytes()); VerdictKey { coin_id, store_launcher_id: store.to_bytes(), @@ -273,17 +278,19 @@ fn chain_bond_verdict_and_coin( /// The full verdict: the chain half, then **whose bond it is**. /// -/// A valid, fully-collateralised coin bonding exactly this content still says nothing about the -/// peer offering the record — every field of that record, the coin id included, was chosen by -/// whoever answered the lookup. Only the coin's own owner-written declaration of a peer closes that, -/// and this node cannot read one yet (see [`peer_declaration`]), so nothing is promoted today. +/// A valid, fully-collateralised coin bonding exactly this content still says nothing about the peer +/// offering the record — every field of that record, the coin id included, was chosen by whoever +/// answered the lookup. Only the coin's own owner-written declaration of a peer closes that, and +/// [`peer_declaration`] reads it, so a `Bonded` here means BOTH halves held: the coin bonds this +/// content, and the coin's owner named this claimant. /// /// Credit is withheld, never subtracted: a record naming this coin may be a stranger's lie ABOUT the /// coin's real holder, and demoting on it is what would make that lie pay. /// -/// **No chain is read at all while the ownership half has no source** (see -/// [`declaration_source_is_readable`]): with `Bonded` unreachable, the reads would be paid for an -/// answer this function is about to discard. +/// **A node with no censused requirement for the epoch cannot promote anyone**, because it cannot +/// price a bond — `required_collateral` is then `None` and the verdict degrades to `Unverified` +/// rather than to `Bonded`. Detection of a false claim still works there (the binding is checked +/// first, deliberately), but certification does not. pub fn verdict_for( source: &S, store_launcher_id: Bytes32, @@ -714,7 +721,7 @@ mod tests { "attacker-2.example", "attacker-3.example", ], - "exactly one copy is promoted; the rest fall back to baseline in source order, never below it" + "exactly one copy is promoted; the rest fall back to baseline in source order, never below it" ); } @@ -755,6 +762,57 @@ mod tests { ); } + /// **Proves (dig-node#473, adversarial gate):** the promotion bound is keyed on the peer's + /// IDENTITY, not on the text a record happened to spell it with. + /// + /// **Catches:** the bound being defeated at zero cost. Everything that GRANTS a promotion + /// compares bytes — the coin's declaration decodes the hex, and the TLS pin compares certificate + /// hashes — so a peer id in upper case and the same id in lower case are one peer to every check + /// that matters. A bound keyed on the raw string is not: a stranger returning one honest + /// holder's peer id in eight different hex spellings, each with its own addresses, would have + /// eight distinct keys, eight promotions, and eight chain reads, and would occupy the whole + /// verified budget on the strength of a single bond it does not hold. + /// + /// The two records below differ ONLY in the case of that one field, so a set keyed on the text + /// promotes both and a set keyed on the identity promotes one. + #[tokio::test] + async fn the_promotion_bound_is_not_defeated_by_respelling_one_peer_id() { + let mut shouted = holder_at(0xAA, Some([0x01; 32]), "attacker.example"); + shouted.provider_peer_id = shouted.provider_peer_id.to_uppercase(); + assert_ne!( + shouted.provider_peer_id, + holder_at(0xAA, None, "x").provider_peer_id, + "the fixture must actually differ as TEXT, or it proves nothing" + ); + + // The honest holder sits BETWEEN the two spellings, and that placement is the whole test. + // With it last, a promoted respelling and a baseline one land in the same position under a + // stable sort and the two behaviours are indistinguishable -- the test would pass either + // way. Here, promoting the respelling moves it AHEAD of the honest record; bounding it + // leaves the honest record in front. + let slate = Slate(vec![ + holder_at(0xAA, Some([0x01; 32]), "attacker-lower.example"), + holder_at(0xCC, None, "honest.example"), + shouted, + ]); + let slot = bond_verifier_slot(); + let _ = slot.set(Arc::new(EveryChainCheckPasses)); + let locator = BondRankingLocator::new(Arc::new(slate), slot); + + let got = locator.find_providers(&capsule()).await.expect("located"); + let hosts: Vec = got.iter().map(|r| r.addresses[0].host.clone()).collect(); + + assert_eq!( + hosts, + vec![ + "attacker-lower.example", + "honest.example", + "attacker.example", + ], + "one identity earns one promoted slot however it is spelled; the respelling stays at baseline, BEHIND the honest holder it would otherwise have jumped" + ); + } + /// **Proves (dig-node#466 / #473):** a wrong pointer costs the verifier exactly ONE chain read, /// with no retry loop — the cost the ticket requires be borne by the publisher, not the reader. /// diff --git a/crates/dig-node-service/src/mirror/lifecycle.rs b/crates/dig-node-service/src/mirror/lifecycle.rs index 30892fd4..3279c1d0 100644 --- a/crates/dig-node-service/src/mirror/lifecycle.rs +++ b/crates/dig-node-service/src/mirror/lifecycle.rs @@ -147,6 +147,9 @@ pub struct NodeMirrorEffects<'a, S: ChainSource> { committed_coin_ids: Result>, PassError>, /// Where this node advertises its stores can be fetched from. Empty means it cannot advertise. advertised_urls: Vec, + /// This node's own `peer_id`, written into every coin it creates so a reader can credit the bond + /// to it. `None` before the peer network has started. + own_peer_id: Option, /// The chain, for the owned-coin scan and for the reclaim spends. source: &'a S, /// This node's operator puzzle hash — a public value, derived once at bring-up. @@ -170,6 +173,33 @@ pub struct NodeMirrorEffects<'a, S: ChainSource> { } impl<'a, S: ChainSource> NodeMirrorEffects<'a, S> { + /// The peer declaration to write into a coin this node creates. + /// + /// `None` when the peer network has not started, or -- which should not happen -- when the id it + /// reported is not a well-formed peer id. Both are warned about rather than silently dropped: a + /// coin created without a declaration locks real collateral for an epoch and can never be + /// promoted by any reader, which is a worse outcome than the create being retried next pass. + fn declared_peer(&self) -> Option { + let Some(peer_id) = self.own_peer_id.as_deref() else { + tracing::warn!( + target: "mirror", + "creating a mirror coin before the peer network reported an identity: the coin will name no peer and no reader can credit its bond to this node" + ); + return None; + }; + match dig_mirror_coin::PeerDeclaration::from_hex(peer_id) { + Ok(declaration) => Some(declaration), + Err(error) => { + tracing::warn!( + target: "mirror", + %error, + "this node reported a peer id that is not a well-formed peer id; the coin will name no peer" + ); + None + } + } + } + /// Assemble the effects for one pass from readings the scheduler has already taken. #[allow(clippy::too_many_arguments)] pub fn new( @@ -177,6 +207,7 @@ impl<'a, S: ChainSource> NodeMirrorEffects<'a, S> { dig_balance: Result, committed_coin_ids: Result, PassError>, advertised_urls: Vec, + own_peer_id: Option, source: &'a S, owner_puzzle_hash: Bytes32, signer: Option<&'a MirrorSigner>, @@ -191,6 +222,7 @@ impl<'a, S: ChainSource> NodeMirrorEffects<'a, S> { // of the audit record, and within-pass accumulation is this type's business. committed_coin_ids: committed_coin_ids.map(std::cell::RefCell::new), advertised_urls, + own_peer_id, source, owner_puzzle_hash, signer, @@ -472,6 +504,7 @@ impl MirrorEffects for NodeMirrorEffects<'_, S> { root_hash, num_bigint::BigInt::from(epoch), self.advertised_urls.clone(), + self.declared_peer(), amount_dig_base_units, dig_coins, signer.synthetic_key(), diff --git a/crates/dig-node-service/src/mirror/spends.rs b/crates/dig-node-service/src/mirror/spends.rs index bf58dbc4..acec2815 100644 --- a/crates/dig-node-service/src/mirror/spends.rs +++ b/crates/dig-node-service/src/mirror/spends.rs @@ -170,6 +170,7 @@ pub fn build_create( root_hash: Bytes32, epoch: BigInt, urls: Vec, + declared_peer: Option, collateral_dig_base_units: u64, dig_coins: Vec, synthetic_key: PublicKey, @@ -178,6 +179,12 @@ pub fn build_create( ) -> Result { let spends = dig_mirror_coin::create( MirrorAdvertisement { + // The peer this collateral stands behind. A coin that names nobody bonds content for no + // one in particular: no reader can credit it to this node, so the collateral buys + // discovery weight it will never receive. `None` still creates the coin -- refusing + // would leave a node unable to bond at all before its peer network is up -- and the + // caller logs it. + declared_peer, store_launcher_id, root_hash, epoch: epoch.clone(), diff --git a/crates/dig-node-service/src/server.rs b/crates/dig-node-service/src/server.rs index 5e3991d9..1a0d1183 100644 --- a/crates/dig-node-service/src/server.rs +++ b/crates/dig-node-service/src/server.rs @@ -2906,6 +2906,11 @@ fn spawn_mirror_passes( // any chain read rather than staking collateral on an advertisement // nobody can act on. advertised_urls.clone(), + // Re-read per pass rather than captured at spawn: this task starts + // beside the peer network rather than after it, so a value read once + // could be `None` for the life of the node and every coin it created + // would name nobody. + node.own_peer_id(), &source, owner_puzzle_hash, signer_ref, diff --git a/crates/dig-node-service/tests/mirror_advertised_urls.rs b/crates/dig-node-service/tests/mirror_advertised_urls.rs index 22358baf..217de465 100644 --- a/crates/dig-node-service/tests/mirror_advertised_urls.rs +++ b/crates/dig-node-service/tests/mirror_advertised_urls.rs @@ -246,6 +246,9 @@ fn the_configured_urls_reach_the_coin_in_the_operators_order() { Ok(PER_COIN), Ok(HashSet::new()), urls, + // No peer declaration: this fixture is about URLs and reservations, not about the + // coin naming a peer. `None` is what a node writes before its peer network is up. + None, &chain, signer.owner_puzzle_hash(), Some(&signer), @@ -311,6 +314,9 @@ fn an_all_rejected_value_refuses_and_spends_nothing() { Ok(PER_COIN), Ok(HashSet::new()), urls, + // No peer declaration: this fixture is about URLs and reservations, not about the + // coin naming a peer. `None` is what a node writes before its peer network is up. + None, &chain, signer.owner_puzzle_hash(), Some(&signer), diff --git a/crates/dig-node-service/tests/mirror_intra_pass_reservation.rs b/crates/dig-node-service/tests/mirror_intra_pass_reservation.rs index d08d4fab..9bcad1b5 100644 --- a/crates/dig-node-service/tests/mirror_intra_pass_reservation.rs +++ b/crates/dig-node-service/tests/mirror_intra_pass_reservation.rs @@ -232,6 +232,9 @@ fn two_creates_in_one_pass_select_disjoint_coins() { // Non-empty: `create` refuses before any chain read without one, and a probe that tripped // that refusal would assert nothing about coin selection. vec!["https://mirror.example/dig".to_string()], + // No peer declaration: this fixture is about URLs and reservations, not about the + // coin naming a peer. `None` is what a node writes before its peer network is up. + None, &chain, signer.owner_puzzle_hash(), Some(&signer), @@ -285,6 +288,9 @@ fn the_only_coin_funds_one_create_and_the_second_refuses() { Ok(PER_COIN), Ok(HashSet::new()), vec!["https://mirror.example/dig".to_string()], + // No peer declaration: this fixture is about URLs and reservations, not about the + // coin naming a peer. `None` is what a node writes before its peer network is up. + None, &chain, signer.owner_puzzle_hash(), Some(&signer), From 6349be77f5689f7f81d269760c8e33062c5038f0 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 10:28:34 -0700 Subject: [PATCH 05/19] chore(mirror): salvage fix-lane WIP from the 15:11Z session cap -- uncompiled Uncommitted work left in the dead lane's worktree (7 files, +753/-41). Never compiled or tested by the lane that wrote it; the resuming implementer verifies it first. Co-Authored-By: Claude --- Cargo.lock | 6 +- crates/dig-node-core/src/mirror_bond.rs | 67 ++- .../src/mirror/bond_verify.rs | 433 +++++++++++++++++- .../dig-node-service/src/mirror/lifecycle.rs | 145 +++++- crates/dig-node-service/src/mirror/runner.rs | 10 + crates/dig-node-service/src/mirror/spends.rs | 16 +- .../tests/mirror_bond_verify.rs | 117 +++++ 7 files changed, 753 insertions(+), 41 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fea707be..3fe3b545 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2880,9 +2880,7 @@ dependencies = [ [[package]] name = "dig-mirror-coin" -version = "0.7.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f53968cacd4bbb5be4540aab0940b24a70e73b0b26f05cdfca9c8ccc6778e053" +version = "0.8.0" dependencies = [ "chia-bls 0.36.1", "chia-protocol 0.36.1", @@ -3031,7 +3029,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.236.0" +version = "0.244.0" dependencies = [ "async-trait", "axum", diff --git a/crates/dig-node-core/src/mirror_bond.rs b/crates/dig-node-core/src/mirror_bond.rs index 370eb915..9922603e 100644 --- a/crates/dig-node-core/src/mirror_bond.rs +++ b/crates/dig-node-core/src/mirror_bond.rs @@ -163,6 +163,30 @@ pub type BondVerifierSlot = Arc>>; pub const MAX_VERIFIED_PER_LOCATE: usize = 8; /// Promotion tier for a verdict — `Bonded` first, everything else in one baseline tier. +/// A peer id from an untrusted provider record, in a shape that is safe to put in a log field. +/// +/// `provider_peer_id` is a bare `String` carried off an unsigned provider record with no +/// wire-boundary normalisation, so it may be any length and hold any UTF-8 — newlines and control +/// characters included, which is what makes an unbounded log field a log-INJECTION surface rather +/// than merely a noisy one: a peer that spells its id with an embedded newline writes a second line +/// into this node's log, and an operator reading that log cannot tell it from one this node wrote. +/// +/// An honest peer id is exactly 64 hex characters, so keeping only hex digits, lowercasing them and +/// taking the first [`PEER_ID_HEX_LEN`] discards nothing real. The lowercasing matches the +/// normalisation the promotion bound above applies for the same reason: two spellings are one +/// identity, so two log lines about them should read as one peer. +fn peer_id_for_log(provider_peer_id: &str) -> String { + provider_peer_id + .chars() + .filter(char::is_ascii_hexdigit) + .map(|c| c.to_ascii_lowercase()) + .take(PEER_ID_HEX_LEN) + .collect() +} + +/// The length of a peer id in hex characters — 32 bytes, so 64. +const PEER_ID_HEX_LEN: usize = 64; + fn credit_rank(verdict: BondVerdict) -> u8 { match verdict { BondVerdict::Bonded => 0, @@ -253,7 +277,7 @@ impl ProviderLocator for BondRankingLocator { // disproves its own claim. Logged and NOT demoted — the record may be a stranger's // lie ABOUT an honest holder, and demoting on it is what would make that lie pay. tracing::debug!( - peer = %record.provider_peer_id, + peer = %peer_id_for_log(&record.provider_peer_id), "located holder's claimed mirror coin does not bond this content; no promotion" ); } @@ -276,6 +300,47 @@ mod tests { use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Mutex; + /// **Proves (dig-node#501, security round 1, LOW 5):** peer-supplied text reaching a log field + /// is bounded and normalised. + /// + /// **Catches:** log injection through an unsigned field. `provider_peer_id` is a bare `String` + /// with no wire-boundary normalisation, so a peer that spells its id with an embedded newline + /// writes a whole second line into this node's log, and an operator reading it cannot tell that + /// line from one the node wrote itself. The fixture carries a newline, a carriage return, an + /// ANSI escape and 200 characters of padding — each a separate thing the old field would have + /// emitted verbatim — and the honest row is the control, so a function that simply returned the + /// empty string would not pass. + #[test] + fn a_peer_id_in_a_log_field_is_bounded_and_normalised() { + let honest = "aa".repeat(32); + assert_eq!( + peer_id_for_log(&honest), + honest, + "control: an honest 64-hex id must survive unchanged, or the field says nothing" + ); + assert_eq!( + peer_id_for_log(&honest.to_uppercase()), + honest, + "two spellings are one identity, exactly as the promotion bound treats them" + ); + + let hostile = format!("dead\nbeef\r\u{1b}[31m peer=trusted {}", "f".repeat(200)); + let logged = peer_id_for_log(&hostile); + assert!( + !logged.chars().any(char::is_control), + "a control character would forge a log line: {logged:?}" + ); + assert!( + logged.chars().all(|c| c.is_ascii_hexdigit()), + "only hex digits can be part of a peer id: {logged:?}" + ); + assert!( + logged.len() <= PEER_ID_HEX_LEN, + "the field is bounded at one peer id's worth, got {} characters", + logged.len() + ); + } + const STORE: [u8; 32] = [0x11; 32]; const ROOT: [u8; 32] = [0x22; 32]; diff --git a/crates/dig-node-service/src/mirror/bond_verify.rs b/crates/dig-node-service/src/mirror/bond_verify.rs index 898d5fdb..ecbd4eb6 100644 --- a/crates/dig-node-service/src/mirror/bond_verify.rs +++ b/crates/dig-node-service/src/mirror/bond_verify.rs @@ -59,6 +59,61 @@ const VERDICT_TTL: Duration = Duration::from_secs(600); /// earned simply by rotating coin ids, which converts a memoisation into an amplifier. const MAX_CACHED_VERDICTS: usize = 1024; +/// The most locate-triggered verifications this PROCESS will pay for in one burst. +/// +/// Each one costs up to two blocking HTTPS reads through the node's ONE shared `ChiaQuery` client — +/// the same client the wallet, the collateral census and the mirror spends read through. So this +/// ceiling is not about fairness between requestors; it is about this node keeping its own chain +/// access when a stranger directs traffic at it. +/// +/// The inbound admission gate (`allow_miss_lookup`, burst 16 at 4/sec) is PER REQUESTOR over up to +/// 4,096 self-minted identities and has no aggregate cap, so it cannot bound this. Sixteen is +/// therefore chosen against the *outbound* cost, not the inbound one: two locates' worth of a full +/// slate, after which verification degrades to `Unverified` — which is where every record already +/// sits when no verifier is installed at all. +const VERIFICATION_BURST: u32 = 16; + +/// How fast [`VERIFICATION_BURST`] refills, in verifications per second. +/// +/// One per second, deliberately slower than the inbound refill it cannot control: an attacker who +/// sustains the inbound rate gets a *constant* trickle of outbound reads rather than a multiple of +/// its own request rate. An honest node's own reads — a capsule it is downloading — are answered +/// from the burst and then from the verdict cache. +const VERIFICATION_REFILL_PER_SEC: f64 = 1.0; + +/// The most verifications in flight at once, so a burst cannot fan out CONCURRENTLY onto the shared +/// client even while it is within the rate ceiling. +/// +/// A rate bound alone still permits sixteen simultaneous blocking reads, which is what a connection +/// pool experiences as an outage rather than as load. +const MAX_CONCURRENT_VERIFICATIONS: usize = 4; + +/// The most DISTINCT claimed coin ids one claiming peer may spend chain reads on, per +/// [`CLAIMANT_LEDGER_WINDOW`], without ever proving a bond. +/// +/// This is the bound that answers the fabricated-coin-id case specifically. [`VerdictKey`] includes +/// the coin id — it must, or a stranger republishing a public coin id would inherit its holder's +/// verdict — so eight records carrying eight *invented* coin ids miss the cache eight times by +/// construction, and no cache design can fix that. What CAN be bounded is how many invented ids one +/// claimed identity is allowed to be wrong about before this node stops asking on its behalf. +/// +/// Four, because an honest holder needs ONE: a peer publishes the coin it created. A rollover can +/// make it briefly two (the old coin and the new one). Anything beyond that is a peer that does not +/// know which coin it holds, and its records are worth no more chain reads than a stranger's. +const MAX_UNPROVEN_COINS_PER_CLAIMANT: usize = 4; + +/// How long a claimant's unproven-coin ledger is remembered. Matched to [`VERDICT_TTL`] so a peer +/// that genuinely rotates coins is forgiven on the same clock a cached verdict expires on. +const CLAIMANT_LEDGER_WINDOW: Duration = VERDICT_TTL; + +/// The most claimants tracked in that ledger at once. +/// +/// A claiming peer id is attacker-chosen and unbounded in supply, so the ledger MUST be bounded. +/// Once it is full of live entries an UNKNOWN claimant is refused rather than admitted, which is +/// the fail-closed direction: the cost of being wrong is a holder sitting at the baseline tier it +/// would occupy with no verifier at all, never a wrong promotion. +const MAX_TRACKED_CLAIMANTS: usize = 512; + /// A verdict is only ever cached for the exact question it answered. /// /// The coin id alone is not the key: one coin bonds one `(store, root, epoch)`, so caching by coin @@ -323,6 +378,173 @@ pub fn verdict_for( } } +/// Whether a chain read may be spent on one claim, right now. +/// +/// **Why this exists.** Before promotion went live, `verdict_for` returned at +/// `declaration_source_is_readable()` and a locate cost zero chain reads. Activating it turned one +/// cheap inbound token into up to `MAX_VERIFIED_PER_LOCATE` verifications, each two outbound HTTPS +/// reads, on a client shared with the wallet — and the inbound gate that admits the locate is +/// per-requestor over self-minted identities, so it bounds nothing in aggregate. This type is the +/// aggregate bound (dig-node#501, security round 1, HIGH). +/// +/// Two independent limits, because they answer two different attacks: +/// +/// * a **process-wide token bucket** ([`VERIFICATION_BURST`] at [`VERIFICATION_REFILL_PER_SEC`]), +/// which bounds total outbound egress however many identities the traffic is spread across; +/// * a **per-claimant distinct-coin ledger** ([`MAX_UNPROVEN_COINS_PER_CLAIMANT`]), which bounds +/// the fabricated-coin-id case the verdict cache structurally cannot absorb. +/// +/// Exhaustion of either returns [`BondVerdict::Unverified`] having read NOTHING. That is a +/// degradation and not a refusal of service: `Unverified` and `Unbonded` share a rank, the sort is +/// stable, and the located slate is returned unchanged — the exact behaviour of a node with no +/// verifier installed. **The read path is never blocked and no holder is ever ranked below where it +/// started**, so an attacker who exhausts the budget denies promotion, not content. +struct ReadAdmission { + state: Mutex, +} + +/// [`ReadAdmission`]'s interior, held under one lock so the two limits are decided atomically — +/// a token spent on a claim the ledger was about to refuse would be a leak. +struct AdmissionState { + /// Whole verifications available now. Fractional so a sub-second refill is not rounded away. + tokens: f64, + /// When `tokens` was last brought up to date. + refilled_at: Instant, + burst: f64, + refill_per_sec: f64, + /// Per claimant: when its window opened, and the distinct coin ids it has spent reads on + /// without proving a bond. Keyed on the SHA-256 of the lowercased peer id, for the reason + /// [`VerdictKey`] hashes it — the key must be fixed-size against an attacker-chosen string, and + /// two hex spellings must be one identity. + unproven: HashMap<[u8; 32], (Instant, std::collections::HashSet<[u8; 32]>)>, +} + +impl ReadAdmission { + /// A budget of the given size. Parameterised so a test can exhaust one in a few calls rather + /// than by replicating production's arithmetic. + fn new(burst: u32, refill_per_sec: f64) -> Self { + ReadAdmission { + state: Mutex::new(AdmissionState { + tokens: f64::from(burst), + refilled_at: Instant::now(), + burst: f64::from(burst), + refill_per_sec, + unproven: HashMap::new(), + }), + } + } + + /// The one budget every locate on this node draws from. + /// + /// Process-wide rather than per verifier: the constraint being protected is the single shared + /// `ChiaQuery` client, which is a property of the process, so a budget scoped to anything + /// narrower would be several budgets against one resource. + fn shared() -> &'static ReadAdmission { + static SHARED: std::sync::OnceLock = std::sync::OnceLock::new(); + SHARED.get_or_init(|| ReadAdmission::new(VERIFICATION_BURST, VERIFICATION_REFILL_PER_SEC)) + } + + /// Whether a chain read may be spent on `claimed_coin_id` for `claiming_peer_id`. + /// + /// Returning `false` costs the network nothing and this node nothing; returning `true` spends a + /// token, so it is called exactly once per verification and never as a peek. + fn admit(&self, claiming_peer_id: &str, claimed_coin_id: [u8; 32]) -> bool { + let Ok(mut state) = self.state.lock() else { + // A poisoned budget cannot be shown to have capacity, so it has none. + return false; + }; + + let now = Instant::now(); + let elapsed = now.saturating_duration_since(state.refilled_at).as_secs_f64(); + state.tokens = (state.tokens + elapsed * state.refill_per_sec).min(state.burst); + state.refilled_at = now; + + let claimant = claimant_key(claiming_peer_id); + state + .unproven + .retain(|_, (opened, _)| opened.elapsed() < CLAIMANT_LEDGER_WINDOW); + let known = state.unproven.contains_key(&claimant); + if !known && state.unproven.len() >= MAX_TRACKED_CLAIMANTS { + return false; + } + if let Some((_, coins)) = state.unproven.get(&claimant) { + // A coin id this claimant has already been read about is not a new question: the cache + // entry for it has simply expired, and re-asking is what keeps a verdict fresh. Only + // DISTINCT unproven ids count against the ledger. + if !coins.contains(&claimed_coin_id) && coins.len() >= MAX_UNPROVEN_COINS_PER_CLAIMANT { + return false; + } + } + + if state.tokens < 1.0 { + return false; + } + state.tokens -= 1.0; + state + .unproven + .entry(claimant) + .or_insert_with(|| (now, std::collections::HashSet::new())) + .1 + .insert(claimed_coin_id); + true + } + + /// Forgive a claimant's ledger: it has proven a bond, so it is not a peer guessing at coin ids. + /// + /// The process-wide bucket is deliberately NOT refunded. A proven bond says the claimant is + /// honest; it says nothing about this node's chain access, which is the thing the bucket exists + /// to protect. + fn record_proven(&self, claiming_peer_id: &str) { + if let Ok(mut state) = self.state.lock() { + state.unproven.remove(&claimant_key(claiming_peer_id)); + } + } +} + +/// A claiming peer id as a fixed-size ledger key: lowercased, then hashed. +/// +/// The same normalisation [`VerdictKey`] applies, for the same two reasons — the key must be +/// fixed-size against an attacker-chosen string, and a peer id's two hex spellings denote one +/// identity, so a ledger keyed on the raw text would give a stranger a fresh allowance per spelling. +fn claimant_key(claiming_peer_id: &str) -> [u8; 32] { + let mut hasher = chia_sha2::Sha256::new(); + hasher.update(claiming_peer_id.to_ascii_lowercase().as_bytes()); + hasher.finalize() +} + +/// [`verdict_for`], but only if the budget allows the reads it would perform. +/// +/// **This is the composition production uses**, so a test that drives it is exercising the real +/// ordering rather than re-deriving it: the budget is consulted BEFORE the source is touched, so a +/// refused claim reads nothing, and a proven bond forgives its claimant's ledger. +fn admitted_verdict_for( + admission: &ReadAdmission, + source: &S, + store_launcher_id: Bytes32, + root_hash: Bytes32, + epoch: &BigInt, + required_collateral: Option, + claiming_peer_id: &str, + claimed_coin_id: Bytes32, +) -> BondVerdict { + if !admission.admit(claiming_peer_id, claimed_coin_id.to_bytes()) { + return BondVerdict::Unverified; + } + let verdict = verdict_for( + source, + store_launcher_id, + root_hash, + epoch, + required_collateral, + claiming_peer_id, + claimed_coin_id, + ); + if verdict == BondVerdict::Bonded { + admission.record_proven(claiming_peer_id); + } + verdict +} + /// The memo of definite verdicts, keyed on the exact question each one answered. /// /// Its own type, rather than two fields on the verifier, so the key/lookup/eviction rules can be @@ -353,9 +575,26 @@ impl VerdictCache { return; }; if entries.len() >= MAX_CACHED_VERDICTS { - // Evict one arbitrary entry, not the map. `HashMap` iteration order is unspecified, so - // the victim is not attacker-selectable either; the cost of a wrong guess is one chain - // read, never a wrong answer. + // Expired first. Eviction should reclaim what is already worthless before it touches + // anything live, and a full map is the only moment worth paying the scan for. + entries.retain(|_, (taken, _)| taken.elapsed() < VERDICT_TTL); + } + if entries.len() >= MAX_CACHED_VERDICTS { + // Still full of LIVE entries, so admitting this one costs an honest verdict. Only a + // `Bonded` is worth that trade, and only a `Bonded` is expensive to obtain: it requires + // a coin that exists, is fully collateralised, and declares its claimant. + // + // **An `Unbonded` is refused admission rather than allowed to evict.** The previous + // version evicted one arbitrary entry per insert, which read as conservative but is + // paced by the attacker: `Unbonded` is the verdict a stranger elicits for FREE by + // naming coin ids that do not exist, so at an attacker-chosen insert rate the map turns + // over entirely in well under a minute and every honest `Bonded` this node earned is + // discarded. Refusing the cheap insert instead means a flood of invented coin ids + // cannot displace a single earned verdict — it only re-reads its own claims, which + // `ReadAdmission` is separately bounding. + if verdict != BondVerdict::Bonded { + return; + } if let Some(victim) = entries.keys().next().copied() { entries.remove(&victim); } @@ -401,10 +640,23 @@ impl ChainBondVerifier { return BondVerdict::Unverified; }; + // The concurrency ceiling, and `try_acquire` rather than `acquire`: a queue of tasks + // waiting for a permit is an unbounded backlog of attacker-directed work held on this + // node's heap, and the honest answer when the budget is saturated is available immediately + // -- `Unverified`, the tier every record occupies with no verifier installed. + static IN_FLIGHT: std::sync::OnceLock = std::sync::OnceLock::new(); + let Ok(_permit) = IN_FLIGHT + .get_or_init(|| tokio::sync::Semaphore::new(MAX_CONCURRENT_VERIFICATIONS)) + .try_acquire() + else { + return BondVerdict::Unverified; + }; + // `ChainSource` is blocking, so the read leaves the async worker rather than parking it. let epoch_big = BigInt::from(epoch); let verdict = tokio::task::block_in_place(|| { - verdict_for( + admitted_verdict_for( + ReadAdmission::shared(), &source, store, root, @@ -850,6 +1102,179 @@ mod tests { ); } + /// **Proves (dig-node#501, security round 1, HIGH):** locate-triggered verification spends a + /// PROCESS-WIDE budget of chain reads, so spreading the traffic across identities does not + /// multiply this node's outbound egress. + /// + /// **Catches:** the aggregate cap going missing — the defect this PR introduced by lifting the + /// pre-read short-circuit. The inbound gate that admits a locate is per-requestor (burst 16 at + /// 4/sec) over up to 4,096 self-minted identities, so it bounds nothing in aggregate; every + /// admitted locate then verified up to eight records at two HTTPS reads each, through the ONE + /// `ChiaQuery` client the node's own wallet, census and spends read through. + /// + /// Each row here uses a DISTINCT claimant, so the per-claimant ledger cannot be what bites and + /// the number asserted is the aggregate bucket's. The count is asserted from the chain double + /// itself rather than from the admission's own bookkeeping — an admission that returned `false` + /// while still reading would pass a test that only counted refusals. + #[test] + fn locate_triggered_chain_reads_are_capped_process_wide() { + const BURST: u32 = 4; + const CLAIMS: u32 = 20; + + let reads = Arc::new(AtomicUsize::new(0)); + let source = CountingChain { + reads: Arc::clone(&reads), + }; + // No refill during the test: a rate the test cannot outrun would make the cap unobservable. + let admission = ReadAdmission::new(BURST, 0.0); + + let mut verdicts = Vec::new(); + for claim in 0..CLAIMS { + verdicts.push(admitted_verdict_for( + &admission, + &source, + Bytes32::new(STORE), + Bytes32::new(ROOT), + &BigInt::from(7u64), + Some(1), + // A fresh claimant per row, each a well-formed 64-hex id. + &format!("{claim:064x}"), + Bytes32::new([0x33; 32]), + )); + } + + assert_eq!( + reads.load(AtomicOrdering::Relaxed), + BURST as usize, + "the budget is the ceiling on chain reads, whatever the claim rate" + ); + assert_eq!( + verdicts + .iter() + .filter(|v| **v == BondVerdict::Unbonded) + .count(), + BURST as usize, + "control: every admitted claim was genuinely answered, so the cap is not hiding a \ + function that stopped reading altogether" + ); + assert!( + verdicts[BURST as usize..] + .iter() + .all(|v| *v == BondVerdict::Unverified), + "past the budget a record stays at the tier it would occupy with no verifier at all -- \ + withheld credit, never a demotion, and never a blocked read" + ); + } + + /// **Proves (dig-node#501, security round 1, HIGH):** one claimant cannot spend an unbounded + /// number of chain reads on coin ids it invents. + /// + /// **Catches:** the case the verdict cache structurally cannot absorb. [`VerdictKey`] includes + /// the coin id and MUST — without it a stranger republishing a public coin id would inherit its + /// holder's `Bonded` — so a slate of records carrying invented coin ids misses the cache once + /// per invented id, by construction. No cache design fixes that; only a bound on how many + /// unproven ids one claimed identity is allowed does. + /// + /// The budget here is deliberately large, so the number asserted can only be the per-claimant + /// ledger's. A single-claimant test against the aggregate bucket alone would pass with no + /// ledger at all. + #[test] + fn fabricated_coin_ids_from_one_claimant_stop_costing_chain_reads() { + let reads = Arc::new(AtomicUsize::new(0)); + let source = CountingChain { + reads: Arc::clone(&reads), + }; + let admission = ReadAdmission::new(1_000, 0.0); + let claimant = "aa".repeat(32); + + for invented in 0..32u8 { + admitted_verdict_for( + &admission, + &source, + Bytes32::new(STORE), + Bytes32::new(ROOT), + &BigInt::from(7u64), + Some(1), + &claimant, + Bytes32::new([invented; 32]), + ); + } + + assert_eq!( + reads.load(AtomicOrdering::Relaxed), + MAX_UNPROVEN_COINS_PER_CLAIMANT, + "a claimant that never proves a bond gets a fixed allowance of invented coin ids" + ); + + // The control, and the reason the bound is not simply a per-peer denial: a DIFFERENT + // claimant is unaffected by this one's exhaustion. + admitted_verdict_for( + &admission, + &source, + Bytes32::new(STORE), + Bytes32::new(ROOT), + &BigInt::from(7u64), + Some(1), + &"bb".repeat(32), + Bytes32::new([0x01; 32]), + ); + assert_eq!( + reads.load(AtomicOrdering::Relaxed), + MAX_UNPROVEN_COINS_PER_CLAIMANT + 1, + "the ledger is per claimant; one peer exhausting its allowance must not silence another" + ); + } + + /// **Proves (dig-node#501, security round 1, HIGH, second half):** a flood of cheap negative + /// verdicts cannot displace a verdict this node earned. + /// + /// **Catches:** attacker-paced eviction. The previous `remember` evicted one arbitrary entry per + /// insert past [`MAX_CACHED_VERDICTS`], which reads as conservative and is not: `Unbonded` is + /// the verdict a stranger elicits for FREE by naming coin ids that do not exist, so at an + /// attacker-chosen insert rate the whole map turns over in under a minute and every earned + /// `Bonded` is discarded — the "memoisation into an amplifier" the constant's own doc warns + /// about, reached one entry at a time instead of all at once. + /// + /// The honest entries are counted rather than sampled, so an implementation that dropped one + /// per cheap insert would fail here rather than pass on the entry the test happened to check. + #[test] + fn a_flood_of_cheap_negatives_cannot_evict_an_earned_verdict() { + let store = Bytes32::new(STORE); + let root = Bytes32::new(ROOT); + let cache = VerdictCache::default(); + + let earned: Vec = (0..MAX_CACHED_VERDICTS) + .map(|n| VerdictKey::new([n as u8; 32], store, root, 7, &format!("{n:064x}"))) + .collect(); + for key in &earned { + cache.remember(*key, BondVerdict::Bonded); + } + let held = earned.iter().filter(|k| cache.get(k).is_some()).count(); + assert_eq!( + held, MAX_CACHED_VERDICTS, + "control: the map really is full of live earned verdicts before the flood" + ); + + let liar = "ff".repeat(32); + for invented in 0..64u8 { + cache.remember( + VerdictKey::new([invented; 32], store, root, 7, &liar), + BondVerdict::Unbonded, + ); + } + + assert_eq!( + earned.iter().filter(|k| cache.get(k).is_some()).count(), + MAX_CACHED_VERDICTS, + "not one earned verdict may be traded for a negative that cost the publisher nothing" + ); + assert_eq!( + cache.get(&VerdictKey::new([0x07; 32], store, root, 7, &liar)), + None, + "the cheap negative is refused admission rather than admitted at an honest entry's cost" + ); + } + /// **Proves (dig-node#473):** the declaration source is LIVE, so the pre-read short-circuit no /// longer withholds every verdict — and the probe that lifts it is a genuine fail-closed /// self-test rather than a switch someone must remember to flip. diff --git a/crates/dig-node-service/src/mirror/lifecycle.rs b/crates/dig-node-service/src/mirror/lifecycle.rs index 3279c1d0..d0078aff 100644 --- a/crates/dig-node-service/src/mirror/lifecycle.rs +++ b/crates/dig-node-service/src/mirror/lifecycle.rs @@ -172,32 +172,44 @@ pub struct NodeMirrorEffects<'a, S: ChainSource> { resolved: std::cell::RefCell>, } +/// The declaration a create must carry, or the reason this pass must not create at all. +/// +/// **A create with no declaration is REFUSED, not degraded (dig-node#501, security round 1).** A +/// coin created without one locks real collateral for a whole epoch and can never be credited to +/// this node by any reader — `verdict_for` promotes on the coin's own declaration of the claimant, +/// so an undeclared coin buys discovery weight it will never receive. The money is spent either +/// way; only the benefit is lost. +/// +/// Deferring costs a pass. The scheduler runs again, and a node whose peer network has not yet +/// reported an identity almost always has one by the next pass. An epoch of locked $DIG against one +/// deferred pass is not a close call, which is why this fails closed — and why +/// [`super::spends::build_create`] takes a declaration rather than an `Option`, so the refusal is +/// structural instead of a rule a later caller could forget. +fn declaration_for_create( + own_peer_id: Option<&str>, +) -> Result { + let Some(peer_id) = own_peer_id else { + return Err(PassError::Identity( + "the peer network has not reported an identity yet, so a coin created now would name \ + no peer and no reader could credit its bond to this node" + .into(), + )); + }; + dig_mirror_coin::PeerDeclaration::from_hex(peer_id).map_err(|error| { + PassError::Identity(format!( + "this node reported a peer id that is not a well-formed peer id ({error}), so a coin \ + created now would name no peer" + )) + }) +} + impl<'a, S: ChainSource> NodeMirrorEffects<'a, S> { - /// The peer declaration to write into a coin this node creates. + /// The peer declaration to write into a coin this node creates, or the reason there is none. /// - /// `None` when the peer network has not started, or -- which should not happen -- when the id it - /// reported is not a well-formed peer id. Both are warned about rather than silently dropped: a - /// coin created without a declaration locks real collateral for an epoch and can never be - /// promoted by any reader, which is a worse outcome than the create being retried next pass. - fn declared_peer(&self) -> Option { - let Some(peer_id) = self.own_peer_id.as_deref() else { - tracing::warn!( - target: "mirror", - "creating a mirror coin before the peer network reported an identity: the coin will name no peer and no reader can credit its bond to this node" - ); - return None; - }; - match dig_mirror_coin::PeerDeclaration::from_hex(peer_id) { - Ok(declaration) => Some(declaration), - Err(error) => { - tracing::warn!( - target: "mirror", - %error, - "this node reported a peer id that is not a well-formed peer id; the coin will name no peer" - ); - None - } - } + /// Delegates to the free [`declaration_for_create`] so the decision can be driven directly by a + /// test without assembling a whole effects struct around it. + fn declaration_for_create(&self) -> Result { + declaration_for_create(self.own_peer_id.as_deref()) } /// Assemble the effects for one pass from readings the scheduler has already taken. @@ -481,6 +493,18 @@ impl MirrorEffects for NodeMirrorEffects<'_, S> { // `sign_and_broadcast` takes the set mutably to record what this bundle consumed, and a // borrow still live across that call is a runtime panic on the money path rather than a // compile error — so the scope is the guarantee, and it is deliberately narrow. + // BEFORE any coin is selected: a coin created with no declaration would lock this + // collateral for an epoch uncreditably, so the pass defers rather than spends (see + // `declaration_for_create`). + let declaration = self.declaration_for_create().inspect_err(|error| { + tracing::warn!( + target: "mirror", + %error, + store_id = %bond.store_id, + "not creating mirror collateral this pass" + ); + })?; + let dig_coins = { let committed = committed.borrow(); funding::select_operator_dig_cats( @@ -504,7 +528,7 @@ impl MirrorEffects for NodeMirrorEffects<'_, S> { root_hash, num_bigint::BigInt::from(epoch), self.advertised_urls.clone(), - self.declared_peer(), + declaration, amount_dig_base_units, dig_coins, signer.synthetic_key(), @@ -861,6 +885,77 @@ pub fn journal() -> SpendJournal { mod tests { use super::*; + /// **Proves (dig-node#501, security round 1, MEDIUM 3):** a node that cannot name ITSELF does + /// not create a mirror coin. + /// + /// **Catches:** the create-side guard failing OPEN on money. Before this, both reasons a + /// declaration could be absent produced a `None` that `build_create` cheerfully turned into a + /// coin — so a host whose peer network never reported an identity locked a full epoch's + /// collateral, every pass, in a bond no reader could ever credit to it. The rustdoc said so and + /// the code did the opposite. + /// + /// All three rows are asserted, and the `Ok` row is the control: a test that only asserted the + /// refusals would pass against a function that refused unconditionally, which would stop this + /// node bonding anything at all. + #[test] + fn a_create_is_refused_when_this_node_cannot_name_itself() { + let honest = "aa".repeat(32); + let declaration = declaration_for_create(Some(&honest)) + .expect("control: a well-formed peer id yields a declaration and the create proceeds"); + assert!( + declaration.names(&honest), + "control: the declaration written into the coin must name THIS node" + ); + + for absent in [None, Some("not-a-peer-id"), Some("")] { + let refusal = declaration_for_create(absent) + .expect_err("a create with no declaration locks uncreditable collateral"); + assert!( + matches!(refusal, PassError::Identity(_)), + "the refusal is its own cause, not a wallet failure an operator would go debug: \ + got {refusal}" + ); + } + } + + /// **Proves (dig-node#501, review round 2, finding 2):** this module's operator-facing refusal + /// text is not corrupted by a lost line continuation. + /// + /// **Catches:** exactly what shipped at `59a0331`. The two `warn!` literals these messages + /// replace carried an 18- and a 22-space run from an eaten `\`, so the emitted line read + /// "the coin will  … name no peer". It compiles, no assertion looked at the text, and + /// the mangled and correct forms are indistinguishable in a normal diff view — so nothing but a + /// test over the VALUE can see it. Asserted over the rendered message rather than over the + /// source, because that is what an operator actually reads, and it also covers the `Display` + /// prefix the message is glued to. + #[test] + fn the_refusal_messages_read_as_sentences() { + let messages: Vec = [None, Some("not-a-peer-id")] + .into_iter() + .map(|peer| { + declaration_for_create(peer) + .expect_err("both rows refuse") + .to_string() + }) + .collect(); + + assert_eq!(messages.len(), 2, "both refusal literals must be exercised"); + for message in &messages { + assert!( + !message.contains(" "), + "a run of two spaces is an eaten line continuation, not prose: {message:?}" + ); + assert!( + !message.chars().any(char::is_control), + "no control character belongs in an operator-facing line: {message:?}" + ); + assert!( + message.contains("no peer"), + "control: the message must still say what went wrong: {message:?}" + ); + } + } + /// Bring-up with no operator wallet yields NO signer, and says which of the two reasons it was. /// /// The two branches are asserted separately because they collapse to the same `None` signer and diff --git a/crates/dig-node-service/src/mirror/runner.rs b/crates/dig-node-service/src/mirror/runner.rs index 7a6b528a..e07331b2 100644 --- a/crates/dig-node-service/src/mirror/runner.rs +++ b/crates/dig-node-service/src/mirror/runner.rs @@ -88,6 +88,13 @@ pub enum PassError { /// `Display` delegates, so every existing consumer that only renders a `PassError` is /// unaffected: the message an operator sees is the `FundingError`'s own. Funding(super::funding::FundingError), + /// This node cannot yet name ITSELF, so a create would lock uncreditable collateral. + /// + /// Its own variant rather than a [`PassError::Wallet`] because the wallet is fine and the + /// operator has nothing to fix: the peer network has simply not reported an identity yet, and + /// the next pass usually has one. Rendering it as a wallet failure would send an operator to + /// debug a wallet that is working (dig-node#501, security round 1). + Identity(String), } impl std::fmt::Display for PassError { @@ -97,6 +104,9 @@ impl std::fmt::Display for PassError { PassError::Chain(cause) => write!(f, "the chain source could not be read: {cause}"), PassError::Wallet(cause) => write!(f, "the operator wallet could not act: {cause}"), PassError::Funding(cause) => write!(f, "{cause}"), + PassError::Identity(cause) => { + write!(f, "this node cannot declare its own peer identity: {cause}") + } } } } diff --git a/crates/dig-node-service/src/mirror/spends.rs b/crates/dig-node-service/src/mirror/spends.rs index acec2815..94582405 100644 --- a/crates/dig-node-service/src/mirror/spends.rs +++ b/crates/dig-node-service/src/mirror/spends.rs @@ -170,7 +170,7 @@ pub fn build_create( root_hash: Bytes32, epoch: BigInt, urls: Vec, - declared_peer: Option, + declared_peer: dig_mirror_coin::PeerDeclaration, collateral_dig_base_units: u64, dig_coins: Vec, synthetic_key: PublicKey, @@ -179,12 +179,14 @@ pub fn build_create( ) -> Result { let spends = dig_mirror_coin::create( MirrorAdvertisement { - // The peer this collateral stands behind. A coin that names nobody bonds content for no - // one in particular: no reader can credit it to this node, so the collateral buys - // discovery weight it will never receive. `None` still creates the coin -- refusing - // would leave a node unable to bond at all before its peer network is up -- and the - // caller logs it. - declared_peer, + // The peer this collateral stands behind, and NOT an `Option`. A coin that names + // nobody bonds content for no one in particular: no reader can credit it to this node, + // so the collateral is locked for an epoch and buys discovery weight it will never + // receive. Taking a declaration by value is what makes that unspendable rather than + // merely discouraged -- `dig_mirror_coin::create` accepts `None`, so a signature that + // passed the `Option` through would leave the money decision to whichever caller + // remembered it. `lifecycle::declaration_for_create` is where the refusal is decided. + declared_peer: Some(declared_peer), store_launcher_id, root_hash, epoch: epoch.clone(), diff --git a/crates/dig-node-service/tests/mirror_bond_verify.rs b/crates/dig-node-service/tests/mirror_bond_verify.rs index 8e422ab5..0597a90a 100644 --- a/crates/dig-node-service/tests/mirror_bond_verify.rs +++ b/crates/dig-node-service/tests/mirror_bond_verify.rs @@ -411,3 +411,120 @@ fn a_coin_that_passes_every_chain_check_is_still_not_promoted_to_a_claimant() { "a bond proven on chain must not promote a peer the coin does not name" ); } + +/// The world for the promotion rows: two honestly published, fully collateralised mirror coins of +/// `store_a()` at `root_1()`, differing in **which peer each one declares** — plus one that declares +/// the holder but bonds `root_2()`. +/// +/// **Two different wallets rather than two coins of one wallet, and that is a fixture requirement +/// with a reason.** `creating_spend` derives a coin's parent from `(owner, asset, amount)`, so two +/// same-amount advertisements by one wallet are the SAME coin and the second silently overwrites +/// the first's creating spend. Using distinct owners keeps the collateral, the store, the root and +/// the epoch identical across the two rows, so the only thing the promotion decision can see +/// differing between them is the declared peer id. Each coin's hint is recomputed from its own +/// lineage owner, so both are honest publications. +fn declaring_bonds(holder: &str, stranger: &str) -> (Chain, Bytes32, Bytes32, Bytes32) { + let declares_holder = wallet(3); + let declares_stranger = wallet(4); + let wrong_root = wallet(5); + + let (spend_holder, coin_holder) = creating_spend( + &declares_holder, + &mirror_memos( + &declares_holder, + store_a(), + root_1(), + &["https://holder.example", &format!("dig-peer:{holder}")], + ), + ); + let (spend_stranger, coin_stranger) = creating_spend( + &declares_stranger, + &mirror_memos( + &declares_stranger, + store_a(), + root_1(), + &["https://holder.example", &format!("dig-peer:{stranger}")], + ), + ); + let (spend_wrong, coin_wrong) = creating_spend( + &wrong_root, + &mirror_memos( + &wrong_root, + store_a(), + root_2(), + &["https://holder.example", &format!("dig-peer:{holder}")], + ), + ); + + let chain = Chain::holding(&[ + (spend_holder, coin_holder), + (spend_stranger, coin_stranger), + (spend_wrong, coin_wrong), + ]); + ( + chain, + coin_holder.coin_id(), + coin_stranger.coin_id(), + coin_wrong.coin_id(), + ) +} + +/// **Proves (dig-node#466 / #473, review round 2, finding 3):** promotion through `verdict_for` +/// requires BOTH bindings — the coin bonds the requested content, and the coin's own owner-written +/// declaration names the peer claiming it — against a chain holding real coins. +/// +/// **Catches:** exactly what the suite could not see at `59a0331`. Every `Bonded` in the unit tests +/// arrives through a double that builds the declaration term FROM the claiming peer id, so it +/// returns `Bonded` for every claimant by construction and cannot represent a coin that declares +/// someone else. A `verdict_for` that promoted on "some coin bonds this content" — the weaker +/// question a stranger republishing a public coin id passes — would have satisfied that suite in +/// full. +/// +/// The three rows share store, root, epoch, collateral and required collateral, and are asked with +/// the same claimant. Row 2 differs from row 1 only in the declared peer id, and row 3 only in the +/// generation the coin bonds — so each verdict is attributable to one property and to nothing else. +/// Row 3 also exercises `advertises` against a coin that EXISTS, which no prior test did: the +/// existing negative names a coin the chain does not hold, which returns before that step. +#[test] +fn only_a_coin_that_declares_the_claimant_promotes_it() { + let holder = "aa".repeat(32); + let stranger = "bb".repeat(32); + let (chain, declares_holder, declares_stranger, bonds_another_root) = + declaring_bonds(&holder, &stranger); + + let promote = |coin, root| { + verdict_for( + &chain, + store_a(), + root, + &epoch(), + Some(COLLATERAL), + &holder, + coin, + ) + }; + + assert_eq!( + promote(declares_holder, root_1()), + BondVerdict::Bonded, + "the coin bonds this content and its owner named this claimant, so both halves hold" + ); + assert_eq!( + promote(declares_stranger, root_1()), + BondVerdict::Unverified, + "an equally valid bond that names a DIFFERENT peer promotes nobody: credit is withheld, \ + never subtracted, because this record may be a lie about that coin's real holder" + ); + assert_eq!( + promote(bonds_another_root, root_1()), + BondVerdict::Unbonded, + "a real coin that declares the claimant but advertises another generation disproves the \ + claim -- the step that catches it is `advertises`, on a coin that exists" + ); + assert_eq!( + promote(bonds_another_root, root_2()), + BondVerdict::Bonded, + "control: that same coin promotes this claimant for the generation it actually bonds, so \ + the row above is attributable to the triple and not to a fixture too broken to verify" + ); +} From 8bf536c2f64f01663d10113e953b5d5883278b78 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 16:30:17 -0700 Subject: [PATCH 06/19] fix(mirror): restore the spaces a lost continuation baked into six operator messages Six operator-facing literals carried the source indentation of a `\`-line-continuation that had been collapsed away, so an operator reading the log saw a 14-18 space gap in the middle of a sentence. The text is what was always meant; only the run is removed. Found by scanning every literal in the mirror modules rather than the one site the review named -- the reviewer reported it in `lifecycle.rs`, and the defect was actually in `advertise.rs` (3), `pass.rs` (2) and `runner.rs` (1). A defect class named at one site is not a defect class swept. Co-Authored-By: Claude --- crates/dig-node-service/src/mirror/advertise.rs | 6 +++--- crates/dig-node-service/src/mirror/pass.rs | 4 ++-- crates/dig-node-service/src/mirror/runner.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/dig-node-service/src/mirror/advertise.rs b/crates/dig-node-service/src/mirror/advertise.rs index 9e409a7e..0cd4ff1c 100644 --- a/crates/dig-node-service/src/mirror/advertise.rs +++ b/crates/dig-node-service/src/mirror/advertise.rs @@ -91,10 +91,10 @@ pub fn configured_urls() -> Vec { for (entry, why) in &advertised.rejected { let reason = match why { Rejection::NotAbsolute => { - "it is not an absolute URL with a scheme and a host, so it names no way to reach anything" + "it is not an absolute URL with a scheme and a host, so it names no way to reach anything" } Rejection::ThisMachineOnly => { - "its host can only mean this machine, so every reader would resolve it to themselves" + "its host can only mean this machine, so every reader would resolve it to themselves" } }; tracing::warn!( @@ -113,7 +113,7 @@ pub fn configured_urls() -> Vec { } else { tracing::info!( target: "mirror", - "no {ADVERTISE_URLS_ENV} entry is publishable, so this node advertises nothing and creates no mirror coin (SPEC.md 25.10)" + "no {ADVERTISE_URLS_ENV} entry is publishable, so this node advertises nothing and creates no mirror coin (SPEC.md 25.10)" ); } diff --git a/crates/dig-node-service/src/mirror/pass.rs b/crates/dig-node-service/src/mirror/pass.rs index 5e82b666..4a08e8c6 100644 --- a/crates/dig-node-service/src/mirror/pass.rs +++ b/crates/dig-node-service/src/mirror/pass.rs @@ -791,7 +791,7 @@ mod tests { epoch: NOW_EPOCH, amount_dig_base_units: REQUIRED, }, - "the coin is still on chain and still locking money, switch or no switch -- and the reclaim carrying it home is the more precise thing to say than `Bonded`" + "the coin is still on chain and still locking money, switch or no switch -- and the reclaim carrying it home is the more precise thing to say than `Bonded`" ); assert_eq!( state("bb", "22"), @@ -1064,7 +1064,7 @@ mod tests { assert_eq!( d.states, vec![(bond("aa", "11"), BondState::Pending)], - "not `Bonded` -- nothing is advertising it yet -- and not `Reclaiming`, which would describe last epoch's money while the question is about this epoch's capsule" + "not `Bonded` -- nothing is advertising it yet -- and not `Reclaiming`, which would describe last epoch's money while the question is about this epoch's capsule" ); } } diff --git a/crates/dig-node-service/src/mirror/runner.rs b/crates/dig-node-service/src/mirror/runner.rs index e07331b2..5a4d7950 100644 --- a/crates/dig-node-service/src/mirror/runner.rs +++ b/crates/dig-node-service/src/mirror/runner.rs @@ -1381,7 +1381,7 @@ mod tests { assert_eq!( report.states, vec![(bond("aa", "11"), BondState::FundsUnknown)], - "not `Unfunded` -- this pass has no evidence the wallet is short, only that it could not be read" + "not `Unfunded` -- this pass has no evidence the wallet is short, only that it could not be read" ); } From dc262d1e4c72f7cbd976aaf71a0d10726ab508e1 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 16:30:18 -0700 Subject: [PATCH 07/19] style(mirror): rustfmt the salvaged hunks The salvage commit was written into the tree without ever being built or formatted, so two lines it introduced were over width. Formatting only -- no behaviour change. Co-Authored-By: Claude --- crates/dig-node-core/src/mirror_bond.rs | 3 ++- crates/dig-node-service/src/mirror/bond_verify.rs | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/dig-node-core/src/mirror_bond.rs b/crates/dig-node-core/src/mirror_bond.rs index 9922603e..ed059cef 100644 --- a/crates/dig-node-core/src/mirror_bond.rs +++ b/crates/dig-node-core/src/mirror_bond.rs @@ -242,7 +242,8 @@ impl ProviderLocator for BondRankingLocator { // zero collateral by varying case alone. dig-dht applies exactly this normalisation to the // neighbouring `unverified_mirror_coin_id` field, for the reason its own doc gives: without // it "dedup and equality would split on presentation". - let mut promoted_peers: std::collections::HashSet = std::collections::HashSet::new(); + let mut promoted_peers: std::collections::HashSet = + std::collections::HashSet::new(); for record in found { let claimed = record.unverified_mirror_coin_id_bytes(); // A holder that claims nothing, and every record past the budget, keeps its place with diff --git a/crates/dig-node-service/src/mirror/bond_verify.rs b/crates/dig-node-service/src/mirror/bond_verify.rs index ecbd4eb6..7afffc71 100644 --- a/crates/dig-node-service/src/mirror/bond_verify.rs +++ b/crates/dig-node-service/src/mirror/bond_verify.rs @@ -455,7 +455,9 @@ impl ReadAdmission { }; let now = Instant::now(); - let elapsed = now.saturating_duration_since(state.refilled_at).as_secs_f64(); + let elapsed = now + .saturating_duration_since(state.refilled_at) + .as_secs_f64(); state.tokens = (state.tokens + elapsed * state.refill_per_sec).min(state.burst); state.refilled_at = now; From 018be034dd13029129ca4658b27abc7deb48a069 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 16:30:18 -0700 Subject: [PATCH 08/19] docs(spec): 25.6a states the aggregate read bound, and stops promising the eviction we removed Two coherence gaps between 25.6a and the code that now ships under it. The per-locate read bound was specified; the AGGREGATE bound was not. A per-locate ceiling bounds nothing on its own, because the gate admitting a locate is per-requestor over self-minted identities -- an adversary multiplies the ceiling by as many identities as it cares to mint. The clause now requires both limits the implementation holds (a process-wide verification budget and a per-claimant distinct-unproven-coin ledger), requires them to be consulted before the chain is touched, and states that exhaustion degrades to `unverified` rather than refusing service. The eviction clause said overflow "MUST evict rather than clear". The implementation deliberately does less than that: an `unbonded` is refused admission to a cache full of live entries rather than allowed to displace a `bonded`, because `unbonded` is the verdict a stranger elicits for free and per-insert eviction is therefore paced by the attacker. The clause said the code did something it no longer does. Also documents a `clippy::too_many_arguments` allow on `admitted_verdict_for`: its parameter list mirrors `verdict_for`'s exactly so that a transposition of one of the four opaque 32-byte arguments stays visible at the call site. Co-Authored-By: Claude --- SPEC.md | 45 ++++++++++++++++++- .../src/mirror/bond_verify.rs | 7 +++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/SPEC.md b/SPEC.md index e66c4303..cdcf4ff9 100644 --- a/SPEC.md +++ b/SPEC.md @@ -8936,6 +8936,37 @@ rule above is not weakened by it. so a node MUST bound the number of bonds it reads against a chain per locate, verifying in source order and leaving the remainder at baseline. +**A per-locate bound is not by itself sufficient, and a node MUST NOT rely on one.** The gate that +admits a locate is per-requestor over self-minted identities, so it bounds nothing in aggregate: an +adversary spreads its locates across fresh identities and multiplies the per-locate ceiling by as +many as it cares to mint. A node performing promotion therefore MUST additionally hold an +AGGREGATE bound on the chain reads promotion causes, and that bound MUST be consulted BEFORE the +chain is touched, so that a refused claim costs nothing. + +Two limits are required, because they answer two different attacks: + +- a **process-wide** budget on verifications, which bounds this node's total outbound read volume + however many identities the traffic is spread across. It is process-wide rather than per-verifier + because the resource being protected — the node's shared chain client — is a property of the + process, so any narrower scope would be several budgets against one resource; +- a **per-claimant** limit on DISTINCT coin ids the claimant has caused reads for without ever + proving a bond, which bounds the fabricated-coin-id case. A coin id the claimant has already been + read about MUST NOT count again: that is a cache entry expiring and being refreshed, not a new + question. A claimant that proves a bond MUST have this ledger forgiven; the process-wide budget + MUST NOT be refunded, since a proven bond says the claimant is honest and says nothing about this + node's chain access. + +Exhausting either limit yields `unverified` having read nothing. **This is a degradation and never a +refusal of service**: `unverified` and `unbonded` share a rank, the sort is stable, and the located +slate is returned unchanged — precisely the behaviour of a node with no verifier installed. The read +path is never blocked and no holder is ever ranked below where it started, so an adversary who +exhausts the budget denies promotion, not content. + +Both limits are keyed on the claiming peer id LOWERCASED before use. A peer id is fixed-length hex, +so its two spellings denote one identity, and every check that GRANTS promotion already treats them +as one — the coin's declaration compares decoded bytes and the TLS pin compares certificate-hash +bytes. A limit keyed on the raw wire text would hand a stranger a fresh allowance per spelling. + The verification is performed in the ORDER §25.6 states, with one refinement that is normative: the `advertises` binding is checked BEFORE the collateral magnitude. A node that has not censused the epoch cannot price a bond, and checking magnitude first would make every verdict on such a node @@ -8971,8 +9002,18 @@ to be remembered. Only DEFINITE verdicts are cached: `unverified` records this node's own momentary inability to look, and holding it would keep an outage in force after it had ended. The cache is keyed partly on attacker-chosen input, so it MUST -be bounded, and overflow MUST evict rather than clear — clearing would let a stranger discard every -verdict a node has earned by rotating coin ids. +be bounded, and overflow MUST NOT clear — clearing would let a stranger discard every verdict a node +has earned by rotating coin ids. + +**Nor may overflow evict indiscriminately.** Reclaiming already-expired entries first is always +correct. But when the cache is full of LIVE entries, admitting a new one costs an honest verdict, and +only `bonded` is worth that trade: `bonded` is expensive to obtain — it requires a coin that exists, +is fully collateralised, and declares its claimant — whereas `unbonded` is the verdict a stranger +elicits for FREE by naming coin ids that do not exist. A policy of evicting one arbitrary entry per +insert reads as conservative but is paced by the attacker, who at a chosen insert rate turns the +whole cache over in well under a minute and discards every earned verdict. An `unbonded` MUST +therefore be refused admission to a cache full of live entries rather than allowed to displace a +`bonded`. ### 25.7. Consent, the switch, and revocation diff --git a/crates/dig-node-service/src/mirror/bond_verify.rs b/crates/dig-node-service/src/mirror/bond_verify.rs index 7afffc71..2ca899c5 100644 --- a/crates/dig-node-service/src/mirror/bond_verify.rs +++ b/crates/dig-node-service/src/mirror/bond_verify.rs @@ -519,6 +519,13 @@ fn claimant_key(claiming_peer_id: &str) -> [u8; 32] { /// **This is the composition production uses**, so a test that drives it is exercising the real /// ordering rather than re-deriving it: the budget is consulted BEFORE the source is touched, so a /// refused claim reads nothing, and a proven bond forgives its claimant's ledger. +/// +/// The parameter list is `verdict_for`'s, in `verdict_for`'s order, with the budget in front. That +/// is deliberate and is why the lint is allowed here rather than satisfied by grouping: four of the +/// arguments are opaque 32-byte values, so the one mistake this wrapper could make is transposing +/// two of them, and a signature that mirrors the wrapped function exactly makes such a transposition +/// visible at the call site below instead of hiding it inside a re-packing struct. +#[allow(clippy::too_many_arguments)] fn admitted_verdict_for( admission: &ReadAdmission, source: &S, From 21b0e6a4c2f039750ab755dc16ba208b53191335 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 16:30:19 -0700 Subject: [PATCH 09/19] fix(mirror): the bond path takes its own corroboration floor and keeps the panic backstop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dig-node#513 items 1 and 5, both of which need the file #506 created. Item 1 -- `CORROBORATION_FLOOR` is two, so two agreeing peers were a full quorum for a `Bonded` verdict. That constant is two for a LIVENESS reason belonging to the sync path: it writes the wallet's replica, and demanding more peers than a thin network offers is what froze a user's node for hours. The bond path writes nothing -- a refused round yields `Unverified`, the tier every record occupies with no verifier installed -- so refusing is free there and the floor can be higher. `BOND_CORROBORATION_FLOOR = 3` is therefore a SEPARATE constant, applied through `tally_with_floor` and selected by the bond path via `CorroboratedChainSource::requiring_corroboration`. It binds in BOTH dimensions -- answers and agreement -- so a wide round in which two voices agree does not buy its way past it. The cache is not consulted above the default floor: a cached row records the answer a round settled on, never how many peers settled it, and the sync path fills that cache at two. Item 5 -- this adapter replaces `chia-query`'s bridge on the bond path and had dropped its `guard_panics` backstop, keeping only the runtime-flavour check. That catches the misuse we can name and nothing else; a panic crossing a `ChainSource` method would unwind out of a `block_in_place` inside a locate. Restored, asserted through `block_on` rather than on the helper, so deleting it from the path turns the test red. Item 2 (memoising `Unverified`) stays rejected by design: `VerdictCache::remember` refuses `Unverified` and the path is bounded by `ReadAdmission` instead. Also merges origin/main and bumps dig-mirror-coin 0.8 -> 0.9 (§2.4b). Refs dig-node#513 Co-Authored-By: Claude --- crates/dig-node-service/Cargo.toml | 2 +- .../src/mirror/bond_verify.rs | 9 ++ .../tests/mirror_bond_corroboration.rs | 99 +++++++++++++ .../src/sage/corroborated_source.rs | 136 +++++++++++++++++- crates/dig-wallet/src/sage/peer_reads.rs | 50 ++++++- crates/dig-wallet/src/sage/quorum.rs | 43 +++++- crates/dig-wallet/src/sage/quorum/tests.rs | 62 ++++++++ 7 files changed, 386 insertions(+), 15 deletions(-) diff --git a/crates/dig-node-service/Cargo.toml b/crates/dig-node-service/Cargo.toml index 56dde7cd..45cf3d6b 100644 --- a/crates/dig-node-service/Cargo.toml +++ b/crates/dig-node-service/Cargo.toml @@ -107,7 +107,7 @@ dig-mirror-collateral = "0.3" # The chain half of the same model: `census` counts the collateralised network at a block height # and hands `dig-mirror-collateral` the three integers its controller consumes (dig-node#400). # Without it a node could only ever record epoch 1, which is derivable from nothing. -dig-mirror-coin = "0.8" +dig-mirror-coin = "0.9" # The canonical `ChainSource` trait `dig-mirror-coin`'s census is generic over. Declared, not # implemented: `chia-query` already provides the implementation this node uses diff --git a/crates/dig-node-service/src/mirror/bond_verify.rs b/crates/dig-node-service/src/mirror/bond_verify.rs index 9f43202c..76f74650 100644 --- a/crates/dig-node-service/src/mirror/bond_verify.rs +++ b/crates/dig-node-service/src/mirror/bond_verify.rs @@ -653,9 +653,18 @@ impl ChainBondVerifier { // // Re-read per call rather than held from bring-up, matching the mirror pass: a transport // built once would make a node that started offline one that never verifies again. + // + // At the BOND floor, not the sync one (dig-node#513). `CORROBORATION_FLOOR` is two + // because the sync path writes the wallet's replica and a refused round stalls it; this + // path writes nothing, so a refusal costs `Unverified` -- the tier every record occupies + // with no verifier installed -- while believing a two-voice round sells a promotion for + // the price of two peers. let Ok(source) = self .chain .corroborated_chain_source(tokio::runtime::Handle::current()) + .map(|source| { + source.requiring_corroboration(dig_wallet::sage::quorum::BOND_CORROBORATION_FLOOR) + }) else { return BondVerdict::Unverified; }; diff --git a/crates/dig-node-service/tests/mirror_bond_corroboration.rs b/crates/dig-node-service/tests/mirror_bond_corroboration.rs index 521d60b3..0a06c7e0 100644 --- a/crates/dig-node-service/tests/mirror_bond_corroboration.rs +++ b/crates/dig-node-service/tests/mirror_bond_corroboration.rs @@ -177,6 +177,13 @@ async fn source_over(views: Vec>) -> CorroboratedChainSource { CorroboratedChainSource::new(reads, tokio::runtime::Handle::current()) } +/// The same source, asked at the BOND floor -- what `ChainBondVerifier` actually uses. +async fn bond_floor_source_over(views: Vec>) -> CorroboratedChainSource { + source_over(views) + .await + .requiring_corroboration(dig_wallet::sage::quorum::BOND_CORROBORATION_FLOOR) +} + // --------------------------------------------------------------------------- // The fixtures // --------------------------------------------------------------------------- @@ -470,3 +477,95 @@ fn a_single_source_bonds_the_fabricated_coin() { "the fabricated coin passes every internal-consistency check; only corroboration catches it" ); } + +// --------------------------------------------------------------------------- +// dig-node#513 item 1 -- the bond path's own corroboration floor +// --------------------------------------------------------------------------- + +/// **Proves (dig-node#513 item 1):** a round only TWO peers answered does not bond, because the +/// bond path demands `BOND_CORROBORATION_FLOOR` and not the sync path's +/// `CORROBORATION_FLOOR = 2`. +/// +/// **Catches:** the floor being inherited rather than chosen. `CORROBORATION_FLOOR` is two for a +/// liveness reason that belongs to the SYNC path -- a refused round stalls the wallet's replica -- +/// and at two a pair of colluding peers is a full quorum, so a promotion costs an attacker two +/// voices. On this path a refusal costs nothing, so the floor can be higher and must be. +/// +/// **Why the fixture is shaped this way:** four peers are drawn and two are silent, so the round +/// is *thin* rather than *dishonest*; the two that speak hold the GENUINE bond and agree with each +/// other perfectly. Nothing but the floor can refuse it. The control below is the identical +/// fixture at the default floor, differing in exactly one dimension -- the floor -- so this pair +/// cannot be satisfied by a harness that simply never bonds. +#[tokio::test(flavor = "multi_thread")] +async fn a_two_peer_round_does_not_bond_at_the_bond_floor() { + let honest = honest_bond(); + let view = ChainView::holding(std::slice::from_ref(&honest)); + + let source = bond_floor_source_over(vec![Some(view.clone()), Some(view), None, None]).await; + + assert_eq!( + verdict(&source, honest.1.coin_id()), + BondVerdict::Unverified, + "two agreeing peers bonded a coin on the path where refusing is free" + ); +} + +/// **Proves:** the control for the case above -- the SAME two-peer round DOES bond at the default +/// floor, so the refusal there is the bond floor and not the thinness of the fixture. +/// +/// It also pins the sync path's floor from the other side: `CORROBORATION_FLOOR` must stay at two, +/// because raising it is what froze a user's replica, and a change that raised the shared constant +/// to satisfy the case above would turn this control red. +#[tokio::test(flavor = "multi_thread")] +async fn the_same_two_peer_round_still_bonds_at_the_default_floor() { + let honest = honest_bond(); + let view = ChainView::holding(std::slice::from_ref(&honest)); + + let source = source_over(vec![Some(view.clone()), Some(view), None, None]).await; + + assert_eq!( + verdict(&source, honest.1.coin_id()), + BondVerdict::Bonded, + "fixture: a two-peer round must bond at the default floor, or the case above proves nothing" + ); +} + +/// **Proves:** the bond floor is not bought by DIALLING wider. Three peers answer, but only two of +/// them agree, and the round is refused. +/// +/// **Catches:** a floor enforced only on how many peers ANSWERED. Under that reading a round of +/// twelve in which two voices agree clears a floor of three, which is the exact shape -- two +/// colluding peers plus noise -- the floor exists to refuse. `tally_with_floor` therefore applies +/// it to the AGREEING count too. +#[tokio::test(flavor = "multi_thread")] +async fn a_wide_round_in_which_only_two_peers_agree_does_not_bond() { + let honest = honest_bond(); + let forged = fabricated_bond(); + let view = ChainView::holding(std::slice::from_ref(&honest)); + let dissent = ChainView::holding(std::slice::from_ref(&forged)); + + let source = bond_floor_source_over(vec![Some(view.clone()), Some(view), Some(dissent)]).await; + + assert_eq!( + verdict(&source, honest.1.coin_id()), + BondVerdict::Unverified, + "two agreeing voices among three answers cleared a floor of three" + ); +} + +/// **Proves:** the control for the whole floor -- at the bond floor, a round of THREE agreeing +/// peers still bonds a genuine coin. The floor is a floor, not a wall. +#[tokio::test(flavor = "multi_thread")] +async fn three_agreeing_peers_do_bond_at_the_bond_floor() { + let honest = honest_bond(); + let view = ChainView::holding(std::slice::from_ref(&honest)); + + let source = + bond_floor_source_over(vec![Some(view.clone()), Some(view.clone()), Some(view)]).await; + + assert_eq!( + verdict(&source, honest.1.coin_id()), + BondVerdict::Bonded, + "the bond floor refused a round that met it exactly" + ); +} diff --git a/crates/dig-wallet/src/sage/corroborated_source.rs b/crates/dig-wallet/src/sage/corroborated_source.rs index 8e0dd02a..535a27d0 100644 --- a/crates/dig-wallet/src/sage/corroborated_source.rs +++ b/crates/dig-wallet/src/sage/corroborated_source.rs @@ -60,13 +60,35 @@ use super::peer_reads::PeerCorroboratedReads; pub struct CorroboratedChainSource { reads: Arc, handle: tokio::runtime::Handle, + /// How many peers must corroborate each read. [`super::quorum::CORROBORATION_FLOOR`] unless a + /// caller asked for more via [`CorroboratedChainSource::requiring_corroboration`]. + floor: usize, } impl CorroboratedChainSource { /// A source over `reads`, bridging each blocking call onto `handle`. #[must_use] pub fn new(reads: Arc, handle: tokio::runtime::Handle) -> Self { - Self { reads, handle } + Self { + reads, + handle, + floor: super::quorum::CORROBORATION_FLOOR, + } + } + + /// The same source, refusing any read fewer than `floor` peers corroborate. + /// + /// The seam a caller whose verdict RANKS a peer uses -- dig-node's mirror bond path passes + /// [`super::quorum::BOND_CORROBORATION_FLOOR`] here. It is a caller's choice rather than this + /// type's default because the two callers pay opposite prices for a refusal: the sync path + /// stalls a replica, the bond path merely declines a promotion. + /// + /// A `floor` below [`super::quorum::CORROBORATION_FLOOR`] is not honoured; this can only + /// tighten. + #[must_use] + pub fn requiring_corroboration(mut self, floor: usize) -> Self { + self.floor = floor.max(super::quorum::CORROBORATION_FLOOR); + self } /// Drives one corroborated read to completion from a synchronous caller. @@ -86,12 +108,33 @@ impl CorroboratedChainSource { .to_string(), )) } - Ok(_) => Ok(tokio::task::block_in_place(|| self.handle.block_on(fut))), - Err(_) => Ok(self.handle.block_on(fut)), + Ok(_) => guard_panics(|| tokio::task::block_in_place(|| self.handle.block_on(fut))), + Err(_) => guard_panics(|| self.handle.block_on(fut)), } } } +/// Runs `f`, converting a panic into a [`ChainSourceError::Transport`] so the synchronous +/// [`ChainSource`] boundary never unwinds (dig-node#513). +/// +/// `chia-query`'s own bridge carries this backstop (`provider_registry::bridge::guard_panics`) and +/// this adapter, which replaces that bridge on the bond path, must not be weaker than the thing it +/// replaces. The runtime-flavour check above catches the misuse we can NAME; this catches the ones +/// we cannot. A panic crossing a `ChainSource` method has no defined behaviour for the caller -- +/// on the bond path it would unwind out of a `block_in_place` inside a locate, taking a read-path +/// task with it, which turns a chain hiccup into a denial of the read the verdict was decorating. +/// +/// `AssertUnwindSafe` because the future is consumed exactly once and a panic leaves no observable +/// half-mutated state behind this facade -- the same reasoning `chia-query` records for its own. +fn guard_panics(f: impl FnOnce() -> T) -> Result { + std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)).map_err(|_| { + ChainSourceError::Transport( + "corroborated chain source caught a panic while blocking on the async runtime" + .to_string(), + ) + }) +} + /// The one spelling of a coin id [`PeerCorroboratedReads`] keys its cache on: lowercase hex, no /// `0x`. `hex::encode` of the 32 bytes produces exactly that, so the key can never drift from the /// bytes the caller asked about. @@ -188,7 +231,11 @@ impl ChainSource for CorroboratedChainSource { fn coin_record(&self, coin_id: Bytes32) -> Result, Self::Error> { let key = key_for(coin_id); let answer = self - .block_on(async { self.reads.coin_record_by_id(&key).await })? + .block_on(async { + self.reads + .coin_record_by_id_at_floor(&key, self.floor) + .await + })? .map_err(|e| ChainSourceError::Transport(e.to_string()))?; answer .as_ref() @@ -201,7 +248,7 @@ impl ChainSource for CorroboratedChainSource { fn coin_spend(&self, coin_id: Bytes32) -> Result, Self::Error> { let key = key_for(coin_id); let answer = self - .block_on(async { self.reads.coin_spend(&key).await })? + .block_on(async { self.reads.coin_spend_at_floor(&key, self.floor).await })? .map_err(|e| ChainSourceError::Transport(e.to_string()))?; answer .as_ref() @@ -268,3 +315,82 @@ impl ChainSource for CorroboratedChainSource { )) } } + +#[cfg(test)] +mod tests { + //! dig-node#513 item 5 -- this adapter replaces `chia-query`'s bridge on the bond path, and + //! must not be weaker than the thing it replaces. + + use std::sync::Arc; + + use super::*; + use crate::sage::peer_reads::{CoinPeer, PeerSample}; + use crate::wallet_db::WalletDb; + + /// A sample that holds no peers -- enough to build a source, since these cases never read. + struct NoPeers; + + #[async_trait::async_trait] + impl PeerSample for NoPeers { + async fn draw(&self) -> Vec> { + Vec::new() + } + } + + async fn source() -> CorroboratedChainSource { + let db = WalletDb::open_in_memory() + .await + .expect("in-memory wallet db"); + let reads = Arc::new(PeerCorroboratedReads::new(Arc::new(NoPeers), db)); + CorroboratedChainSource::new(reads, tokio::runtime::Handle::current()) + } + + /// PROPERTY: a panic raised while blocking becomes a `ChainSourceError`, and does NOT unwind + /// out of the synchronous trait boundary. + /// + /// NEAREST WRONG IMPLEMENTATION: the runtime-flavour check alone, which is what this adapter + /// shipped with. It catches the misuse we can NAME and nothing else, while the bridge it + /// replaces (`chia_query::provider_registry::bridge::guard_panics`) catches the rest. The + /// assertion is on the RETURN, not on `catch_unwind` being present: a test that merely called + /// `guard_panics` directly would pass with the backstop deleted from `block_on`, so this drives + /// it through the real path. + #[tokio::test(flavor = "multi_thread")] + async fn a_panic_while_blocking_becomes_an_error_rather_than_unwinding() { + let source = source().await; + let previous = std::panic::take_hook(); + std::panic::set_hook(Box::new(|_| {})); + + let outcome: Result<(), ChainSourceError> = + source.block_on(async { panic!("a read panicked") }); + + std::panic::set_hook(previous); + assert!( + matches!(outcome, Err(ChainSourceError::Transport(_))), + "a panic crossed the ChainSource boundary instead of becoming an error" + ); + } + + /// PROPERTY: the control -- the guarded path still RETURNS an ordinary value, so the case above + /// is not satisfied by a `block_on` that errs unconditionally. + #[tokio::test(flavor = "multi_thread")] + async fn an_ordinary_read_still_returns_its_value_through_the_guard() { + let source = source().await; + assert_eq!(source.block_on(async { 7u32 }).ok(), Some(7)); + } + + /// PROPERTY: the bond floor is a caller's choice and can only TIGHTEN. + #[tokio::test(flavor = "multi_thread")] + async fn the_corroboration_floor_can_only_tighten() { + let strict = source() + .await + .requiring_corroboration(super::super::quorum::BOND_CORROBORATION_FLOOR); + assert_eq!(strict.floor, super::super::quorum::BOND_CORROBORATION_FLOOR); + + let relaxed = source().await.requiring_corroboration(1); + assert_eq!( + relaxed.floor, + super::super::quorum::CORROBORATION_FLOOR, + "a caller talked the source down to a single source" + ); + } +} diff --git a/crates/dig-wallet/src/sage/peer_reads.rs b/crates/dig-wallet/src/sage/peer_reads.rs index 92e081c7..232c59f8 100644 --- a/crates/dig-wallet/src/sage/peer_reads.rs +++ b/crates/dig-wallet/src/sage/peer_reads.rs @@ -228,12 +228,34 @@ impl PeerCorroboratedReads { /// `Ok(None)` means the peers AGREED there is no such coin. Failing to assemble agreement is /// an `Err`. pub async fn coin_record_by_id(&self, coin_id: &str) -> Result> { + self.coin_record_by_id_at_floor(coin_id, quorum::CORROBORATION_FLOOR) + .await + } + + /// [`Self::coin_record_by_id`], demanding `floor` corroborating peers instead of the default + /// [`quorum::CORROBORATION_FLOOR`]. + /// + /// # The cache is not consulted above the default floor + /// + /// A cached row records the ANSWER a past round settled on and not how many peers settled it, + /// so serving one to a caller that asked for stronger corroboration would answer a question + /// the cache cannot answer -- and would make the stricter floor vacuous, since the ordinary + /// sync path fills that cache at the default floor. The stricter caller therefore always asks + /// the peers. It still WRITES what it learns, because a row corroborated by more peers is not + /// less true for the readers that wanted fewer. + pub async fn coin_record_by_id_at_floor( + &self, + coin_id: &str, + floor: usize, + ) -> Result> { let id = normalized(coin_id); let parsed = parse_coin_id(&id)?; let now = self.clock.now_unix(); - if let Some(coin) = self.cached_coin_record_by_id(coin_id).await? { - return Ok(Some(coin)); + if floor <= quorum::CORROBORATION_FLOOR { + if let Some(coin) = self.cached_coin_record_by_id(coin_id).await? { + return Ok(Some(coin)); + } } let peers = self.sample.draw().await; @@ -249,7 +271,7 @@ impl PeerCorroboratedReads { } } - let verdict = quorum::tally(&responses); + let verdict = quorum::tally_with_floor(&responses, floor); let Some(answer) = verdict.corroborated() else { return Err(no_corroboration("coin record", &id, &verdict)); }; @@ -333,12 +355,28 @@ impl PeerCorroboratedReads { /// `Ok(None)` means the peers AGREED the coin is unspent or unknown. Failing to assemble /// agreement is an `Err`, because a lineage walk reads absence as *this is the tip*. pub async fn coin_spend(&self, coin_id: &str) -> Result> { + self.coin_spend_at_floor(coin_id, quorum::CORROBORATION_FLOOR) + .await + } + + /// [`Self::coin_spend`], demanding `floor` corroborating peers. + /// + /// The cache is bypassed above the default floor for the reason + /// [`Self::coin_record_by_id_at_floor`] states, and it applies with more force here: a spend + /// row has no TTL, so one written by a thin round would answer every stricter caller forever. + pub async fn coin_spend_at_floor( + &self, + coin_id: &str, + floor: usize, + ) -> Result> { let id = normalized(coin_id); let parsed = parse_coin_id(&id)?; let now = self.clock.now_unix(); - if let Some(spend) = self.cached_coin_spend(coin_id).await? { - return Ok(Some(spend)); + if floor <= quorum::CORROBORATION_FLOOR { + if let Some(spend) = self.cached_coin_spend(coin_id).await? { + return Ok(Some(spend)); + } } let peers = self.sample.draw().await; @@ -352,7 +390,7 @@ impl PeerCorroboratedReads { } } - let verdict = quorum::tally(&responses); + let verdict = quorum::tally_with_floor(&responses, floor); let Some(answer) = verdict.corroborated() else { return Err(no_corroboration("coin spend", &id, &verdict)); }; diff --git a/crates/dig-wallet/src/sage/quorum.rs b/crates/dig-wallet/src/sage/quorum.rs index 5a5146e0..2d97d8a6 100644 --- a/crates/dig-wallet/src/sage/quorum.rs +++ b/crates/dig-wallet/src/sage/quorum.rs @@ -171,6 +171,25 @@ const _: () = assert!( /// of this node may reasonably overturn it in either direction. pub const CORROBORATION_FLOOR: usize = 2; +/// The corroboration floor a **bond verdict** demands, above [`CORROBORATION_FLOOR`]. +/// +/// Three, and the reason it may be higher here is that the two paths pay opposite prices for +/// refusing. [`CORROBORATION_FLOOR`] is pinned at two because the sync path writes the wallet's +/// replica: a round it refuses is a round the replica does not advance, and demanding more peers +/// than a thin network offers is how a user's node froze for hours. **The bond path writes +/// nothing.** A refused round there yields `unverified`, which shares a rank with `unbonded` and +/// leaves the located slate exactly as a node with no verifier installed would return it, so the +/// cost of refusing is zero and the cost of believing a thin round is a promotion an attacker +/// bought for the price of two voices. +/// +/// It is deliberately a SEPARATE constant rather than a raised [`CORROBORATION_FLOOR`]: raising +/// the shared one would re-create the frozen replica, which is the incident that fixed it at two. +/// +/// Applied through [`tally_with_floor`], which enforces it in BOTH dimensions — at least this many +/// peers must answer, and at least this many must give the same answer — so it cannot be satisfied +/// by a wide round in which only two voices agree. +pub const BOND_CORROBORATION_FLOOR: usize = 3; + /// How many distinct peers one round DIALS before narrowing (dig_ecosystem#2827). /// /// Over-subscribing then pulling back is what makes a round resilient to the ordinary case that @@ -760,14 +779,32 @@ pub struct Response { /// cheaper for an attacker to capture, and [`sybil_success_probability`] will tell you by how /// much. pub fn tally(responses: &[Response]) -> Verdict { + tally_with_floor(responses, CORROBORATION_FLOOR) +} + +/// [`tally`], against a caller-chosen corroboration floor. +/// +/// `floor` is applied in BOTH dimensions: fewer than `floor` answers is +/// [`Verdict::Insufficient`], and an answer given by fewer than `floor` peers is +/// [`Verdict::Split`] however wide the round was. Enforcing only the first would let a +/// twelve-peer round in which two voices agree pass a floor of three, which is the shape the +/// stricter floor exists to refuse. +/// +/// A floor BELOW [`CORROBORATION_FLOOR`] is not honoured -- the never-one-source rule is not a +/// caller's to relax -- so this can only ever be stricter than [`tally`], never weaker. +pub fn tally_with_floor( + responses: &[Response], + floor: usize, +) -> Verdict { + let floor = floor.max(CORROBORATION_FLOOR); let answered = responses.len(); - if answered < CORROBORATION_FLOOR { + if answered < floor { return Verdict::Insufficient { answered, - required: CORROBORATION_FLOOR, + required: floor, }; } - let required = required_agreement(answered); + let required = required_agreement(answered).max(floor); let mut counts: HashMap<&A, usize> = HashMap::new(); for r in responses { diff --git a/crates/dig-wallet/src/sage/quorum/tests.rs b/crates/dig-wallet/src/sage/quorum/tests.rs index e7d719c9..88a7005a 100644 --- a/crates/dig-wallet/src/sage/quorum/tests.rs +++ b/crates/dig-wallet/src/sage/quorum/tests.rs @@ -926,3 +926,65 @@ fn a_thinner_round_is_measurably_easier_to_capture() { floor={at_floor} sample={at_sample} hold={at_hold}" ); } + +// --------------------------------------------------------------------------- +// dig-node#513 item 1 -- a caller-chosen, strictly tighter corroboration floor +// --------------------------------------------------------------------------- + +/// PROPERTY: `tally_with_floor` refuses a round of two at a floor of three, and accepts the round +/// of three -- the bound pinned from BOTH sides, so it cannot pass by refusing everything. +#[test] +fn the_bond_floor_is_pinned_from_both_sides() { + let two = responses(&[("a", 1), ("b", 1)]); + let three = responses(&[("a", 1), ("b", 1), ("c", 1)]); + + assert_eq!( + tally_with_floor(&two, BOND_CORROBORATION_FLOOR), + Verdict::Insufficient { + answered: 2, + required: BOND_CORROBORATION_FLOOR, + }, + "one under the bond floor must not corroborate" + ); + assert!( + tally_with_floor(&three, BOND_CORROBORATION_FLOOR) + .corroborated() + .is_some(), + "at the bond floor exactly, a unanimous round must still corroborate" + ); +} + +/// PROPERTY: the floor binds the AGREEING count, not merely how many peers answered. +/// +/// NEAREST WRONG IMPLEMENTATION: enforcing `floor` only on `responses.len()`. Under that, twelve +/// answers of which two agree clears a floor of three -- two colluding peers plus noise, which is +/// precisely the round the floor exists to refuse. `required_agreement(3)` is 3 already, so the +/// fixture is widened to a size where the ratio alone would permit two. +#[test] +fn a_wide_round_does_not_buy_its_way_past_the_bond_floor() { + let scattered = responses(&[("a", 1), ("b", 1), ("c", 2), ("d", 3), ("e", 4), ("f", 5)]); + + assert!( + matches!( + tally_with_floor(&scattered, BOND_CORROBORATION_FLOOR), + Verdict::Split { .. } + ), + "two agreeing voices in a six-peer round cleared a floor of three" + ); +} + +/// PROPERTY: the floor can only TIGHTEN. A caller asking for one source is given +/// `CORROBORATION_FLOOR` anyway -- the never-one-source rule is not a caller's to relax. +#[test] +fn a_floor_below_the_shared_one_is_not_honoured() { + let one = responses(&[("a", 1)]); + + assert_eq!( + tally_with_floor(&one, 1), + Verdict::Insufficient { + answered: 1, + required: CORROBORATION_FLOOR, + }, + "a caller talked the tally down to a single source" + ); +} From e7a33506b885a1dc87e21addf8143d596ea83497 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 16:30:20 -0700 Subject: [PATCH 10/19] fix(mirror): adopt dig-mirror-coin 0.9.0 in the lock and correct the WalletDb test import The manifest already declared "0.9"; the lock still resolved 0.8.0, so the two disagreed. Resolve the lock to 0.9.0 so the declared and resolved versions agree. The corroborated_source test module imported WalletDb from a `crate::wallet_db` path that does not exist; the type lives at `crate::sage::db::WalletDb`. Co-Authored-By: Claude --- Cargo.lock | 4 ++-- crates/dig-wallet/src/sage/corroborated_source.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 190be45f..3e4b5763 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2880,9 +2880,9 @@ dependencies = [ [[package]] name = "dig-mirror-coin" -version = "0.8.0" +version = "0.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "eceab37a1234805e0043c30d096f5da38f5b86581032f465d8a2db807cf10b64" +checksum = "2ac3c72506f13eef2cd6dccce66fd715191e2fc7b751f3b682332d92bf545812" dependencies = [ "chia-bls 0.36.1", "chia-protocol 0.36.1", diff --git a/crates/dig-wallet/src/sage/corroborated_source.rs b/crates/dig-wallet/src/sage/corroborated_source.rs index 535a27d0..64c26e65 100644 --- a/crates/dig-wallet/src/sage/corroborated_source.rs +++ b/crates/dig-wallet/src/sage/corroborated_source.rs @@ -325,7 +325,7 @@ mod tests { use super::*; use crate::sage::peer_reads::{CoinPeer, PeerSample}; - use crate::wallet_db::WalletDb; + use crate::sage::db::WalletDb; /// A sample that holds no peers -- enough to build a source, since these cases never read. struct NoPeers; From 56dfeac51af90838083ee2b70045b5657be120ec Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 16:30:21 -0700 Subject: [PATCH 11/19] build(deps): resolve dig-mirror-coin to 0.9.0 after the main merge The manifest declares "0.9"; the post-merge lock carried 0.7.0 from main. Re-resolve so declared and resolved agree on one line. Co-Authored-By: Claude --- Cargo.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b827726e..3e4b5763 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2880,9 +2880,9 @@ dependencies = [ [[package]] name = "dig-mirror-coin" -version = "0.7.0" +version = "0.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f53968cacd4bbb5be4540aab0940b24a70e73b0b26f05cdfca9c8ccc6778e053" +checksum = "2ac3c72506f13eef2cd6dccce66fd715191e2fc7b751f3b682332d92bf545812" dependencies = [ "chia-bls 0.36.1", "chia-protocol 0.36.1", @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.252.4" +version = "0.253.0" dependencies = [ "async-trait", "axum", From b4ab5be670a06875352401b1ebdc773811b1a454 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 20:06:49 -0700 Subject: [PATCH 12/19] style(wallet): order the corroborated_source test imports as rustfmt wants The only `cargo fmt --all -- --check` diff on this branch: `db` must precede `peer_reads`. Fixed by hand rather than with `cargo fmt --all`, which has rewritten thousands of untouched lines on a sibling branch. --- crates/dig-wallet/src/sage/corroborated_source.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/dig-wallet/src/sage/corroborated_source.rs b/crates/dig-wallet/src/sage/corroborated_source.rs index 64c26e65..2e274514 100644 --- a/crates/dig-wallet/src/sage/corroborated_source.rs +++ b/crates/dig-wallet/src/sage/corroborated_source.rs @@ -324,8 +324,8 @@ mod tests { use std::sync::Arc; use super::*; - use crate::sage::peer_reads::{CoinPeer, PeerSample}; use crate::sage::db::WalletDb; + use crate::sage::peer_reads::{CoinPeer, PeerSample}; /// A sample that holds no peers -- enough to build a source, since these cases never read. struct NoPeers; From 274b3350d467c2c446d4673cebeacb9900fb20df Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 20:07:08 -0700 Subject: [PATCH 13/19] fix(mirror): supply a peer id in the create fixtures the identity guard now refuses Three tests went red on this branch for one reason: `create` now refuses before selecting any coin when the node has reported no peer id, because a coin naming no peer locks collateral for an epoch that no reader could ever credit. The fixtures predate that guard and passed `None`, whose comment ("what a node writes before its peer network is up") described a state that is no longer a supported input to a create. Each of the three now passes a well-formed id built as `"a1".repeat(32)`, so its length is right by construction rather than by counting 64 characters in a literal. The guard itself is unchanged and correct: it fails closed on money, which is the direction a wrong answer should fail in. The fourth call site keeps `None` deliberately. `an_all_rejected_value_refuses_and_spends_nothing` refuses at the advertisement guard, which returns before the identity guard is consulted, so its `None` is never reached. That test previously asserted only `is_err()`, which this PR's second early return makes ambiguous -- it would pass just as happily if the identity guard were reordered ahead of the URL one, leaving the URL guard it exists for unexercised. It now names the expected cause. --- .../tests/mirror_advertised_urls.rs | 28 +++++++++++++------ .../tests/mirror_intra_pass_reservation.rs | 14 ++++++---- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/crates/dig-node-service/tests/mirror_advertised_urls.rs b/crates/dig-node-service/tests/mirror_advertised_urls.rs index 217de465..f6f8268c 100644 --- a/crates/dig-node-service/tests/mirror_advertised_urls.rs +++ b/crates/dig-node-service/tests/mirror_advertised_urls.rs @@ -246,9 +246,11 @@ fn the_configured_urls_reach_the_coin_in_the_operators_order() { Ok(PER_COIN), Ok(HashSet::new()), urls, - // No peer declaration: this fixture is about URLs and reservations, not about the - // coin naming a peer. `None` is what a node writes before its peer network is up. - None, + // A well-formed peer id, because `create` now refuses without one: a coin naming no peer + // locks collateral no reader could credit. This fixture is about URL ORDER, so it must + // reach the advertisement rather than stop at the identity guard. `repeat` rather than a + // 64-character literal so the length is right by construction instead of by counting. + Some("a1".repeat(32)), &chain, signer.owner_puzzle_hash(), Some(&signer), @@ -314,8 +316,10 @@ fn an_all_rejected_value_refuses_and_spends_nothing() { Ok(PER_COIN), Ok(HashSet::new()), urls, - // No peer declaration: this fixture is about URLs and reservations, not about the - // coin naming a peer. `None` is what a node writes before its peer network is up. + // Deliberately absent, and deliberately UNREACHED: the advertisement guard returns before + // `declaration_for_create` is consulted, so this row still refuses for the URL reason. It + // is left as `None` so that a future reordering putting the identity guard first changes + // the REASON, which the assertion below now names explicitly. None, &chain, signer.owner_puzzle_hash(), @@ -325,10 +329,18 @@ fn an_all_rejected_value_refuses_and_spends_nothing() { runtime.handle().clone(), ); - let refused = effects.create(&bond(0xB2, 0xD4), EPOCH, PER_COIN); + let reason = effects + .create(&bond(0xB2, 0xD4), EPOCH, PER_COIN) + .expect_err( + "a mirror with nowhere to fetch from is not a mirror, so the create must refuse", + ); + // WHICH refusal, not merely that one happened. `create` now has a second early return -- the + // peer-identity guard -- and this fixture carries no declaration, so a bare `is_err()` would + // pass just as happily if the identity guard were reordered ahead of the advertisement one, + // leaving the URL guard this test exists for completely unexercised. assert!( - refused.is_err(), - "a mirror with nowhere to fetch from is not a mirror, so the create must refuse" + reason.to_string().contains("at least one URL"), + "the refusal must name the missing advertisement rather than any other cause: {reason}" ); assert!( broadcast_bytes(&broadcaster).is_empty(), diff --git a/crates/dig-node-service/tests/mirror_intra_pass_reservation.rs b/crates/dig-node-service/tests/mirror_intra_pass_reservation.rs index 9bcad1b5..02a2140d 100644 --- a/crates/dig-node-service/tests/mirror_intra_pass_reservation.rs +++ b/crates/dig-node-service/tests/mirror_intra_pass_reservation.rs @@ -232,9 +232,10 @@ fn two_creates_in_one_pass_select_disjoint_coins() { // Non-empty: `create` refuses before any chain read without one, and a probe that tripped // that refusal would assert nothing about coin selection. vec!["https://mirror.example/dig".to_string()], - // No peer declaration: this fixture is about URLs and reservations, not about the - // coin naming a peer. `None` is what a node writes before its peer network is up. - None, + // A well-formed peer id, for the same reason the URL list is non-empty: `create` refuses + // before selecting any coin without one, and a probe that stopped at the identity guard + // would assert nothing about DISJOINTNESS, which is the property under test. + Some("a1".repeat(32)), &chain, signer.owner_puzzle_hash(), Some(&signer), @@ -288,9 +289,10 @@ fn the_only_coin_funds_one_create_and_the_second_refuses() { Ok(PER_COIN), Ok(HashSet::new()), vec!["https://mirror.example/dig".to_string()], - // No peer declaration: this fixture is about URLs and reservations, not about the - // coin naming a peer. `None` is what a node writes before its peer network is up. - None, + // A well-formed peer id: `create` refuses before selecting any coin without one, and this + // fixture needs the FIRST create to genuinely reach coin selection so that the second one + // has something already reserved to collide with. + Some("a1".repeat(32)), &chain, signer.owner_puzzle_hash(), Some(&signer), From f9370bf5bcf3cb7b1f25cfefc005120b4e36abe8 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 20:32:44 -0700 Subject: [PATCH 14/19] fix(mirror): guard the advertise literals a test could not reach The earlier pin for this defect class was real but covered the wrong module. `the_refusal_messages_read_as_sentences` drives `declaration_for_create`, so it asserts over `lifecycle.rs`'s own two refusal literals -- which were never the corrupted ones. The three genuinely broken runtime lines are inline `tracing` literals in this file with no test reachability at all, so they sat behind a green test that appeared to cover them. That is worse than the original defect, because it looks discharged. The two rejection reasons move into `rejection_reason()` and the two info lines into `ADVERTISING_AT_CONFIGURED_URLS` and `nothing_publishable()`, so a test can reach the rendered text. `nothing_publishable` is a function rather than a const because it names the environment variable and `concat!` cannot take a const; spelling the variable a second time as a literal would be a second source of truth for the same name. The walk is exhaustive BY CONSTRUCTION: a match maps each `Rejection` variant to the name a failure prints, so a new variant fails to compile until it is named in the walk. `rejection_reason`'s own match would force a new variant to be GIVEN a message, but nothing would force that message into the sweep meant to check it -- a gate over an enumeration can only check the enumeration it was handed. --- .../dig-node-service/src/mirror/advertise.rs | 117 ++++++++++++++++-- 1 file changed, 104 insertions(+), 13 deletions(-) diff --git a/crates/dig-node-service/src/mirror/advertise.rs b/crates/dig-node-service/src/mirror/advertise.rs index 0cd4ff1c..cd32d44e 100644 --- a/crates/dig-node-service/src/mirror/advertise.rs +++ b/crates/dig-node-service/src/mirror/advertise.rs @@ -85,18 +85,45 @@ pub fn advertised_urls_from_env() -> Advertised { /// The warnings are emitted HERE rather than at the call site because this is the only place that /// knows WHY an entry was dropped; a caller handed a shortened list could only report that some /// entry was missing, which is not something an operator can act on. +/// Why one configured entry is not advertised, in the words an operator reads. +/// +/// Split out of [`configured_urls`] so a test can walk EVERY variant and assert over the rendered +/// text. That is not tidiness: all three of this module's operator-facing lines shipped with 14- to +/// 18-space runs baked into the middle of a sentence, left behind when a `\` string continuation +/// lost its backslash. Such a literal compiles, no caller inspects it, and the mangled and correct +/// forms are indistinguishable in a diff — so only a test over the VALUE can see it, and it has to +/// reach these literals rather than a neighbouring module's. +fn rejection_reason(why: &Rejection) -> &'static str { + match why { + Rejection::NotAbsolute => { + "it is not an absolute URL with a scheme and a host, so it names no way to reach anything" + } + Rejection::ThisMachineOnly => { + "its host can only mean this machine, so every reader would resolve it to themselves" + } + } +} + +/// What this node reports when at least one entry survived and will be published. +const ADVERTISING_AT_CONFIGURED_URLS: &str = + "advertising this node's stores at the operator-configured URLs, in the configured order"; + +/// What this node reports when no configured entry may be published. +/// +/// A function rather than a `const` because it names the environment variable, and `concat!` cannot +/// take a `const`. Spelling the variable a second time as a literal would be a second source of +/// truth for the same name — exactly the drift this module's tests exist to catch. +fn nothing_publishable() -> String { + format!( + "no {ADVERTISE_URLS_ENV} entry is publishable, so this node advertises nothing and creates no mirror coin (SPEC.md 25.10)" + ) +} + pub fn configured_urls() -> Vec { let advertised = advertised_urls_from_env(); for (entry, why) in &advertised.rejected { - let reason = match why { - Rejection::NotAbsolute => { - "it is not an absolute URL with a scheme and a host, so it names no way to reach anything" - } - Rejection::ThisMachineOnly => { - "its host can only mean this machine, so every reader would resolve it to themselves" - } - }; + let reason = rejection_reason(why); tracing::warn!( target: "mirror", entry = %entry, @@ -108,13 +135,10 @@ pub fn configured_urls() -> Vec { tracing::info!( target: "mirror", urls = ?advertised.accepted, - "advertising this node's stores at the operator-configured URLs, in the configured order" + "{ADVERTISING_AT_CONFIGURED_URLS}" ); } else { - tracing::info!( - target: "mirror", - "no {ADVERTISE_URLS_ENV} entry is publishable, so this node advertises nothing and creates no mirror coin (SPEC.md 25.10)" - ); + tracing::info!(target: "mirror", "{}", nothing_publishable()); } advertised.accepted @@ -217,6 +241,73 @@ fn is_this_machine_only_v4(ip: Ipv4Addr) -> bool { mod tests { use super::*; + /// **Every operator-facing line this module emits reads as a sentence.** + /// + /// All three of them shipped corrupted: a `\` string continuation lost its backslash and baked + /// 14 to 18 literal spaces into the middle of each sentence. Nothing else can catch that. The + /// compiler is happy, no caller inspects the text, the two forms are indistinguishable in a + /// diff, and the only witness is an operator reading a line that looks broken at the moment + /// they are trying to work out why nothing is being advertised. + /// + /// The guard walks the whole SET rather than a chosen example, and asserts the count, so + /// deleting a message from the walk fails here instead of silently shrinking the sweep. A + /// sibling guard over `lifecycle.rs`'s two refusals exists and is NOT a substitute: it cannot + /// reach these literals, which is how three corrupted runtime lines sat behind a green test + /// that appeared to cover them. + #[test] + fn every_operator_facing_line_reads_as_a_sentence() { + let lines: Vec<(&str, String)> = [Rejection::NotAbsolute, Rejection::ThisMachineOnly] + .iter() + .map(|why| { + // Exhaustive BY CONSTRUCTION, and the name it yields is what a failure prints. A + // guard over an enumeration can only check the enumeration it was handed: + // `rejection_reason`'s own match forces a new `Rejection` variant to be GIVEN a + // message, but nothing would force that message into the sweep meant to check it, + // so it would ship unguarded by the very test that exists to guard it. This match + // fails to compile until the new variant is named here too. + let name = match why { + Rejection::NotAbsolute => "Rejection::NotAbsolute", + Rejection::ThisMachineOnly => "Rejection::ThisMachineOnly", + }; + (name, rejection_reason(why).to_string()) + }) + .chain([ + ( + "ADVERTISING_AT_CONFIGURED_URLS", + ADVERTISING_AT_CONFIGURED_URLS.to_string(), + ), + ("nothing_publishable", nothing_publishable()), + ]) + .collect(); + + assert_eq!( + lines.len(), + 4, + "every line this module emits must be walked, never a subset: {lines:?}" + ); + for (name, line) in &lines { + assert!( + !line.contains(" "), + "{name}: a run of two spaces is an eaten line continuation, not prose: {line:?}" + ); + assert!( + !line.chars().any(char::is_control), + "{name}: no control character belongs in an operator-facing line: {line:?}" + ); + // Control: a fixture too empty to exhibit the property must not pass. Both assertions + // above are satisfied by "" and by a single word. + assert!( + line.split_whitespace().count() >= 8, + "{name}: control -- each line must still be a sentence saying what happened: {line:?}" + ); + } + assert!( + lines[3].1.contains(ADVERTISE_URLS_ENV), + "control: the no-entry line must name the variable an operator has to set: {:?}", + lines[3].1 + ); + } + /// The refusal that exists today survives an unset value: no URL means no advertisement, which /// is what makes `create` decline rather than publish somewhere unreachable. #[test] From 024afa2d1e1c826ac20668c5f7802c338b3fae6b Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 20:33:04 -0700 Subject: [PATCH 15/19] test(mirror): control row 2 so its Unverified is attributable to the declaration The test claimed row 2 "differs only in the declared peer id", which was the reviewer's condition for it proving anything. On the fixture as built it does not: row 1 is minted by wallet(3) and row 2 by wallet(4), so they differ in owner puzzle hash AND declared peer id. That matters because `verdict_for` reaches `Unverified` from two disjoint places -- the chain half producing no coin, and `PeerDeclaration::Silent` at the final match. A row-2 coin malformed anywhere in the chain half would satisfy the assertion while proving nothing about the declaration: the same vacuity already closed for row 3 by its fourth row, and left open for row 2. The distinct owners are load-bearing and stay: `creating_spend` derives a coin's parent from (owner, asset, amount), so one wallet cannot publish two same-amount advertisements without the second overwriting the first. So rather than collapsing the rows, row 2's coin is asked the question it should answer positively -- same coin, same root, claimant `stranger` -- and must return `Bonded`. That proves the entire chain half passes, leaving the declaration as the only thing row 2 can be attributable to. The doc no longer states one-field difference as a property of the fixture. --- .../tests/mirror_bond_verify.rs | 40 ++++++++++++++++--- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/crates/dig-node-service/tests/mirror_bond_verify.rs b/crates/dig-node-service/tests/mirror_bond_verify.rs index 0597a90a..510de556 100644 --- a/crates/dig-node-service/tests/mirror_bond_verify.rs +++ b/crates/dig-node-service/tests/mirror_bond_verify.rs @@ -481,10 +481,22 @@ fn declaring_bonds(holder: &str, stranger: &str) -> (Chain, Bytes32, Bytes32, By /// full. /// /// The three rows share store, root, epoch, collateral and required collateral, and are asked with -/// the same claimant. Row 2 differs from row 1 only in the declared peer id, and row 3 only in the -/// generation the coin bonds — so each verdict is attributable to one property and to nothing else. -/// Row 3 also exercises `advertises` against a coin that EXISTS, which no prior test did: the -/// existing negative names a coin the chain does not hold, which returns before that step. +/// the same claimant. Row 2 differs from row 1 in the declared peer id, and row 3 in the generation +/// the coin bonds. Row 3 also exercises `advertises` against a coin that EXISTS, which no prior +/// test did: the existing negative names a coin the chain does not hold, which returns before that +/// step. +/// +/// **Each negative carries a control, because a negative verdict alone is not evidence.** +/// `verdict_for` reaches `Unverified` from two disjoint places — the chain half producing no coin, +/// and `PeerDeclaration::Silent` at the final match — and reaches `Unbonded` from a coin that is +/// merely unverifiable as easily as from one that genuinely bonds elsewhere. So rows 2 and 3 are +/// each followed by a row proving that the SAME coin verifies completely when asked the question it +/// should answer positively. Without those, either negative would be satisfied by a fixture too +/// broken to verify, and the test would measure nothing while appearing to measure the decision. +/// Note that rows 1 and 2 are necessarily minted by different wallets: `creating_spend` derives a +/// coin's parent from `(owner, asset, amount)`, so one wallet cannot publish two same-amount +/// advertisements. "Differs only in the declared peer id" is therefore a statement about what the +/// DECISION can see, which row 2's control is what actually establishes. #[test] fn only_a_coin_that_declares_the_claimant_promotes_it() { let holder = "aa".repeat(32); @@ -492,17 +504,18 @@ fn only_a_coin_that_declares_the_claimant_promotes_it() { let (chain, declares_holder, declares_stranger, bonds_another_root) = declaring_bonds(&holder, &stranger); - let promote = |coin, root| { + let promote_as = |coin, root, claimant: &str| { verdict_for( &chain, store_a(), root, &epoch(), Some(COLLATERAL), - &holder, + claimant, coin, ) }; + let promote = |coin, root| promote_as(coin, root, &holder); assert_eq!( promote(declares_holder, root_1()), @@ -527,4 +540,19 @@ fn only_a_coin_that_declares_the_claimant_promotes_it() { "control: that same coin promotes this claimant for the generation it actually bonds, so \ the row above is attributable to the triple and not to a fixture too broken to verify" ); + // Control for row 2, and it is load-bearing for the same reason row 4 is. `verdict_for` + // reaches `Unverified` from TWO disjoint places: the chain half failing to produce a coin + // (unreadable source, hint or lineage mismatch, admission exhausted), and `PeerDeclaration:: + // Silent` at the final match. Row 2's coin is minted by a DIFFERENT wallet from row 1's -- + // unavoidably, since `creating_spend` derives the parent from `(owner, asset, amount)` and one + // wallet cannot publish two same-amount advertisements -- so "differs only in the declared + // peer id" is a claim about the DECISION's inputs, not about the fixture's construction. + // Without this row, a `declares_stranger` coin broken anywhere in the chain half would satisfy + // row 2 while proving nothing whatever about the declaration. + assert_eq!( + promote_as(declares_stranger, root_1(), &stranger), + BondVerdict::Bonded, + "control: row 2's coin passes the ENTIRE chain half and promotes the peer it actually \ + names, so row 2's Unverified is attributable to the declaration and to nothing else" + ); } From 2b9adcd4dbce406a8b2bb1c26de61475833507e1 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 20:36:12 -0700 Subject: [PATCH 16/19] docs(mirror): keep configured_urls' doc attached to configured_urls Lifting the message helpers put them BETWEEN `configured_urls`'s doc comment and its signature, so the doc block silently reattached to `rejection_reason` and the public function was left undocumented. Nothing catches this: it compiles, rustfmt and clippy are clean, and the rendered docs simply describe the wrong item. Moved the helpers above the doc block instead. --- .../dig-node-service/src/mirror/advertise.rs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/dig-node-service/src/mirror/advertise.rs b/crates/dig-node-service/src/mirror/advertise.rs index cd32d44e..c8fd9990 100644 --- a/crates/dig-node-service/src/mirror/advertise.rs +++ b/crates/dig-node-service/src/mirror/advertise.rs @@ -75,16 +75,6 @@ pub fn advertised_urls_from_env() -> Advertised { parse_advertised_urls(&std::env::var(ADVERTISE_URLS_ENV).unwrap_or_default()) } -/// The URLs this node will publish, with every rejected entry reported to the operator. -/// -/// This is the whole operator surface as the mirror scheduler consumes it: one call, at bring-up, -/// yielding the list `create` advertises. An empty answer is the honest default rather than an -/// error — `NodeMirrorEffects::create` refuses by name on it, before any chain read, so a node with -/// nothing to advertise stakes nothing. -/// -/// The warnings are emitted HERE rather than at the call site because this is the only place that -/// knows WHY an entry was dropped; a caller handed a shortened list could only report that some -/// entry was missing, which is not something an operator can act on. /// Why one configured entry is not advertised, in the words an operator reads. /// /// Split out of [`configured_urls`] so a test can walk EVERY variant and assert over the rendered @@ -119,6 +109,16 @@ fn nothing_publishable() -> String { ) } +/// The URLs this node will publish, with every rejected entry reported to the operator. +/// +/// This is the whole operator surface as the mirror scheduler consumes it: one call, at bring-up, +/// yielding the list `create` advertises. An empty answer is the honest default rather than an +/// error — `NodeMirrorEffects::create` refuses by name on it, before any chain read, so a node with +/// nothing to advertise stakes nothing. +/// +/// The warnings are emitted HERE rather than at the call site because this is the only place that +/// knows WHY an entry was dropped; a caller handed a shortened list could only report that some +/// entry was missing, which is not something an operator can act on. pub fn configured_urls() -> Vec { let advertised = advertised_urls_from_env(); From 56b35988d647920e5d607ce35bc65d4cb8cda014 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Wed, 2 Sep 2026 20:45:18 -0700 Subject: [PATCH 17/19] test(mirror): derive the guard's count from Rejection::ALL and cover the warn wrapper My own comment overclaimed, in the direction this guard exists to prevent. The exhaustive match is on the TYPE, so a new `Rejection` variant genuinely cannot compile without being named -- but naming is not walking. Add a variant, add the two match arms, and the array literal still compiles, `lines.len()` is still 4, and the hard-coded `assert_eq!(lines.len(), 4)` still passes while the new operator-facing message ships unguarded. A literal count does not merely fail to prevent that; it cements it. The walk is now driven from `Rejection::ALL`, declared beside the enum, with the expected count DERIVED as `Rejection::ALL.len() + 2` so the walk and the list cannot drift. `ALL`'s doc states what is actually enforced -- the match forces a developer into this module and forces the variant to be given a message; nothing forces it into the array, and the derived length is what ties them together -- rather than repeating the stronger claim. Also folds in a line that was outside the walk while the guard's name claimed `every`: the warn wrapper is operator-facing prose in its own right, so it becomes `not_advertised()` and the walk asserts the whole rendered sentence rather than the reason fragment it embeds. --- .../dig-node-service/src/mirror/advertise.rs | 49 ++++++++++++++----- 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/crates/dig-node-service/src/mirror/advertise.rs b/crates/dig-node-service/src/mirror/advertise.rs index c8fd9990..05fcb222 100644 --- a/crates/dig-node-service/src/mirror/advertise.rs +++ b/crates/dig-node-service/src/mirror/advertise.rs @@ -50,6 +50,22 @@ pub enum Rejection { ThisMachineOnly, } +impl Rejection { + /// Every variant, so the operator-message guard can walk the set rather than a chosen example. + /// + /// Declared beside the enum, not spelled inside the test. A walk built from an array literal in + /// the test would still compile, still be the same length, and still pass after a variant was + /// added — so the new variant's message would ship unguarded by the very test written to guard + /// it, which is the shape of gap this module already shipped once. + /// + /// **What is and is not enforced, stated precisely because the looser claim was wrong.** The + /// `match` in [`rejection_reason`] is exhaustive, so a new variant CANNOT compile without a + /// developer editing this module and giving it a message. Nothing in Rust then forces that + /// variant into this array — but the guard asserts its walk against `ALL.len()`, so the walk and + /// this list cannot drift apart, and this is the one place to add it. + pub const ALL: [Rejection; 2] = [Rejection::NotAbsolute, Rejection::ThisMachineOnly]; +} + /// What [`parse_advertised_urls`] made of the operator's value. #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct Advertised { @@ -94,6 +110,15 @@ fn rejection_reason(why: &Rejection) -> &'static str { } } +/// The whole line an operator sees when one entry is dropped, the reason included. +/// +/// The wrapper around the reason is operator-facing prose too, so it belongs in the guard's walk. +/// Checking only the reason would leave the sentence it is embedded in unchecked while the guard +/// claims to cover every line this module emits. +fn not_advertised(reason: &str) -> String { + format!("{ADVERTISE_URLS_ENV} entry is not advertised: {reason}") +} + /// What this node reports when at least one entry survived and will be published. const ADVERTISING_AT_CONFIGURED_URLS: &str = "advertising this node's stores at the operator-configured URLs, in the configured order"; @@ -123,11 +148,11 @@ pub fn configured_urls() -> Vec { let advertised = advertised_urls_from_env(); for (entry, why) in &advertised.rejected { - let reason = rejection_reason(why); tracing::warn!( target: "mirror", entry = %entry, - "{ADVERTISE_URLS_ENV} entry is not advertised: {reason}" + "{}", + not_advertised(rejection_reason(why)) ); } @@ -256,20 +281,22 @@ mod tests { /// that appeared to cover them. #[test] fn every_operator_facing_line_reads_as_a_sentence() { - let lines: Vec<(&str, String)> = [Rejection::NotAbsolute, Rejection::ThisMachineOnly] + // Driven from `Rejection::ALL`, and the expected count is DERIVED from it rather than + // written as a literal. A hard-coded `4` would actively cement a subset: add a variant, + // give it a message, leave it out of `ALL`, and a literal count still matches while the new + // line ships unchecked. The `match` below only forces the variant to be NAMED — that is + // what makes it visible, not what makes it walked — so the derived length is what actually + // ties the two together. + let lines: Vec<(&str, String)> = Rejection::ALL .iter() .map(|why| { - // Exhaustive BY CONSTRUCTION, and the name it yields is what a failure prints. A - // guard over an enumeration can only check the enumeration it was handed: - // `rejection_reason`'s own match forces a new `Rejection` variant to be GIVEN a - // message, but nothing would force that message into the sweep meant to check it, - // so it would ship unguarded by the very test that exists to guard it. This match - // fails to compile until the new variant is named here too. let name = match why { Rejection::NotAbsolute => "Rejection::NotAbsolute", Rejection::ThisMachineOnly => "Rejection::ThisMachineOnly", }; - (name, rejection_reason(why).to_string()) + // The wrapper, not the bare reason: that is the line an operator actually reads, + // and it contains the reason, so this covers both. + (name, not_advertised(rejection_reason(why))) }) .chain([ ( @@ -282,7 +309,7 @@ mod tests { assert_eq!( lines.len(), - 4, + Rejection::ALL.len() + 2, "every line this module emits must be walked, never a subset: {lines:?}" ); for (name, line) in &lines { From a91abf57a4140e8fb46ac6ae3b6b4e75c0221605 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Thu, 3 Sep 2026 07:15:52 -0700 Subject: [PATCH 18/19] fix(mirror): drop the stale None declared_peer field the merge duplicated The merge with origin/main spliced this branch's new `Some(declared_peer)` field alongside main's unmodified `declared_peer: None` block for the same struct literal -- a silent, non-conflicting 3-way merge that left the field specified twice (E0062), because dig-node#473 (this branch) inserted its field near the top of the literal while main's untouched block still carried the pre-#473 field lower down. Removed the stale block; the kept comment and `Some(declared_peer)` are this branch's real implementation. Co-Authored-By: Claude --- crates/dig-node-service/src/mirror/spends.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/crates/dig-node-service/src/mirror/spends.rs b/crates/dig-node-service/src/mirror/spends.rs index 347e3f82..94582405 100644 --- a/crates/dig-node-service/src/mirror/spends.rs +++ b/crates/dig-node-service/src/mirror/spends.rs @@ -191,13 +191,6 @@ pub fn build_create( root_hash, epoch: epoch.clone(), urls, - // No peer declaration: this reproduces exactly what dig-mirror-coin 0.7 wrote, which - // had no declared_peer concept at all, so the bump changes nothing about the coins this - // node creates. It is a deliberate choice rather than a default -- the crate made the - // field required precisely so a consumer cannot inherit one silently. Binding this - // collateral to the node's own DIG peer id is dig-node#473, which owns that decision and - // the signature change it needs. - declared_peer: None, collateral: collateral_dig_base_units, }, dig_coins, From f94969efa525d7882c9091a68bd6400112989fd5 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Thu, 3 Sep 2026 07:37:50 -0700 Subject: [PATCH 19/19] fix(mirror): close the continuation-guard exclusion #501 was granted Empty EXCLUDED_DIRS now that this PR own its six lost-string-continuation sites are already fixed elsewhere in this diff (mirror/advertise.rs, mirror/pass.rs, mirror/runner.rs). Verified with the guard itself: no_lost_string_continuation_leaves_a_multi_space_run_mid_sentence passes scanning every directory under src, with files_scanned > 20 and zero offenses. Co-Authored-By: Claude --- crates/dig-node-service/src/continuation_guard.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/crates/dig-node-service/src/continuation_guard.rs b/crates/dig-node-service/src/continuation_guard.rs index 67a8eb19..2e40b9f1 100644 --- a/crates/dig-node-service/src/continuation_guard.rs +++ b/crates/dig-node-service/src/continuation_guard.rs @@ -11,11 +11,10 @@ use std::path::Path; -/// Files under `mirror/` are owned by dig-node#501 (a separate open PR repairs six -/// sites of this same class there). Scanning them here would either duplicate that -/// fix or fail this branch on work this PR does not own. Delete this exclusion once -/// #501 merges. -const EXCLUDED_DIRS: &[&str] = &["mirror"]; +/// No directory under `src` is exempt from the crate-wide scan (dig-node#501 closed +/// the exclusion this list used to carry for `mirror/`, once that directory's own +/// six lost-continuation sites were fixed). +const EXCLUDED_DIRS: &[&str] = &[]; /// `service.rs`'s `sc qc` parser test pins a byte-identical copy of real `sc.exe` /// output; its fixed-width `LABEL : value` columns are deliberate alignment the