From c0529f23eab1b601bc0778072d3737b63613defc Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 21:17:28 -0400 Subject: [PATCH] feat(cosim): make the v2.4.2 acceptance gate executable, and it found a defect The plan states v2.4.2's gate as prose: "hash-checkpoint agrees with full capture". Checkpoints are an APPROXIMATION of "where do these two runs first differ", traded for four orders of magnitude of disk, and the entire scheme is worthless if the approximation can disagree with the answer. So the answer is now computed directly by first_full_capture_difference, and the contract the approximation must honour is localisation_is_consistent -- a function, not a sentence in a plan document. The contract is narrow deliberately, because a looser reading of it is satisfiable by a broken implementation: identical streams -> must report Identical. Anything else is a FALSE POSITIVE, and a gate that cries wolf gets switched off. a real difference at k -> must NOT report Identical. That FALSE NEGATIVE is the failure that matters: it passes a wrong DUT. ... reported as Diverged -> the named window must CONTAIN k. a real difference at k -> Inconclusive is an honest "I could not answer". identical streams -> Inconclusive is not. The third line is the one worth the effort, and it is the one a weaker gate omits. A divergence report naming the wrong window sends a full-capture re-run somewhere nothing is wrong, spends the debugging budget, and comes back "no problem here" -- which reads as evidence the DUT is fine. The sweep drives 331 cases: every run length around the interval boundary (1, 2, 4095, 4096, 4097, 8192, 8193, 10000, 12288), a corruption at every position for short runs and a randomised sweep for long ones, and a different observable field perturbed each time so it is not silently exercising one field forever. The PRNG is eight lines rather than a dependency: this crate emits the goldens an external implementation is verified against and its lockfile is now committed, so each dependency added here is one more thing that can move underneath a provenance artifact. IT FOUND A REAL DEFECT ON ITS FIRST RUN, AT len = 1 A divergence at cycle ZERO was reported in a window that did not contain it. Divergence::after_cycle was a u64 in which 0 meant both "no prior checkpoint" and "cycle zero" -- a sentinel colliding with a legitimate value. So the first window read as (0, 0], which is empty, and a full-capture re-run of it would have found nothing. "Nothing found" reads as evidence the DUT is fine. Fixed by removing the collision rather than special-casing it: after_cycle is Option, None meaning the first window, whose span begins wherever the run began. Divergence::contains is offered so call sites do not reimplement a boundary that is half-open at one end and open-ended at the other -- the same reasoning that put is_single_scanline in rustynes-probe's divergence module. window_len returns Option for the same honesty: for the first window the span starts where the run started, and a checkpoint stream carries no evidence that it started at cycle 0. The answer is "unknown", not an assumed through_cycle + 1. checkpoint_diff prints that case open-ended: DIVERGED at checkpoint 0 window to re-run with full capture: cycles from the start of the run through 4103 inclusive rather than "(0, 4103]", which would have excluded cycle 0. Note this defect was NOT reachable through the CLI's normal use before the sweep existed -- every hand-written test put its corruption in a later window, because that is what a person picks. len = 1 is the case a hand-written test set omits, and it is the only one that exposes the collision. Both halves are demonstrated to fail. A one-character mutation to Divergence::contains (`<=` to `<`) reddens two tests, not one. One local #[allow] added with its reason: clippy::match_same_arms wants the two `false` arms of the gate predicate merged. They stay apart because they name two DIFFERENT failure modes and the comments on them are the point of the function -- merging deletes exactly the distinction the predicate exists to draw. Gates. The emulation core is untouched; no file under crates/rustynes-{cpu,ppu,apu,mappers,core} changes, so AccuracyCoin 141/141 and nestest 0-diff hold by construction. fmt (workspace and the excluded crate), clippy on both, rustdoc -D warnings on both, markdownlint on both changed documents. 126 workspace suites / 2223 passed / 0 failed 4 excluded-crate suites / 39 passed / 0 failed (was 35) --- CHANGELOG.md | 41 +++ .../rustynes-cosim/src/bin/checkpoint_diff.rs | 23 +- crates/rustynes-cosim/src/checkpoint.rs | 341 +++++++++++++++++- docs/mister.md | 30 ++ 4 files changed, 418 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e656f434..3bbdc646 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,47 @@ cycle-accurate core later replaced. second, which is indistinguishable from "never armed"; `Oracle::take_irq_artifacts` derives both from one take. +- **The v2.4.2 acceptance gate, executable: the hash checkpoints agree with + full capture.** Checkpoints are an *approximation* of "where do these two runs + first differ", traded for four orders of magnitude of disk. The whole scheme + is worthless if the approximation can disagree with the answer, so + `first_full_capture_difference` computes the answer directly and + `localisation_is_consistent` states the contract the approximation must honour + — as a function rather than as prose in a plan. + + The contract is narrow on purpose, because a looser reading is satisfiable by + a broken implementation. Identical streams must report `Identical` (a **false + positive** gets a gate switched off). A real difference must never report + `Identical` (a **false negative** passes a wrong DUT). And when it reports a + divergence, **the named window must contain the difference** — a report naming + the wrong window sends a full-capture re-run somewhere nothing is wrong, + spends the debugging budget, and returns "no problem here", which reads as + evidence the DUT is fine. `Inconclusive` is acceptable for a real difference + and never for identical streams. + + A sweep drives **331 cases**: every run length around the interval boundary + (1, 2, 4095, 4096, 4097, 8192, 8193, 10 000, 12 288), a corruption at every + position for short runs and a randomised sweep for long ones, and a different + observable field perturbed each time so it is not silently exercising one + field. Both the gate predicate and the sweep are demonstrated to fail — a + one-character mutation to `Divergence::contains` reddens two tests. + +- **A divergence at cycle zero was reported in a window that did not contain + it**, found by that sweep at `len = 1` — the degenerate case a hand-written + test set omits. `Divergence::after_cycle` was a `u64` in which `0` meant both + "no prior checkpoint" and "cycle zero", so the first window read as `(0, 0]`, + which is empty. A full-capture re-run of it would have found nothing, and + "nothing found" reads as evidence the DUT is fine. + + `after_cycle` is now `Option`, which removes the sentinel collision + rather than special-casing it, and `Divergence::contains` is offered so call + sites do not reimplement a boundary that is half-open at one end and + open-ended at the other. `window_len` returns `Option`: for the first + window the span begins wherever the run began, and a checkpoint stream carries + no evidence that it began at cycle 0 — so the honest answer is "unknown", not + an assumed `through_cycle + 1`. `checkpoint_diff` prints that window + open-ended rather than as `(0, N]`. + ### Fixed - **The excluded crate's lockfile was silently gitignored, so CI re-resolved it diff --git a/crates/rustynes-cosim/src/bin/checkpoint_diff.rs b/crates/rustynes-cosim/src/bin/checkpoint_diff.rs index 8746329f..8b1f5ac4 100644 --- a/crates/rustynes-cosim/src/bin/checkpoint_diff.rs +++ b/crates/rustynes-cosim/src/bin/checkpoint_diff.rs @@ -50,17 +50,26 @@ fn main() -> ExitCode { ExitCode::SUCCESS } Comparison::Diverged(d) => { + // The first window is printed open-ended rather than as `(0, N]`. + // Cycle 0 is a real cycle, so a `0` lower bound would exclude the + // very cycle that differs when the run diverges immediately -- and + // an operator would re-run a window that cannot contain the + // problem, then read "nothing found" as the DUT being fine. + let window = match (d.after_cycle, d.window_len()) { + (Some(after), Some(len)) => { + format!("cycles ({after}, {}] ({len} cycles)", d.through_cycle) + } + _ => format!( + "cycles from the start of the run through {} inclusive", + d.through_cycle + ), + }; println!( "DIVERGED at checkpoint {}\n \ - window to re-run with full capture: cycles ({}, {}] ({} cycles)\n \ + window to re-run with full capture: {}\n \ reference hash = {:#018x}\n \ candidate hash = {:#018x}", - d.index, - d.after_cycle, - d.through_cycle, - d.window_len(), - d.reference_hash, - d.candidate_hash, + d.index, window, d.reference_hash, d.candidate_hash, ); ExitCode::from(1) } diff --git a/crates/rustynes-cosim/src/checkpoint.rs b/crates/rustynes-cosim/src/checkpoint.rs index adfd1c10..7f62ded7 100644 --- a/crates/rustynes-cosim/src/checkpoint.rs +++ b/crates/rustynes-cosim/src/checkpoint.rs @@ -353,8 +353,17 @@ pub enum InconclusiveReason { pub struct Divergence { /// Index of the first differing checkpoint. pub index: usize, - /// Last cycle known to agree. Zero when the very first window differs. - pub after_cycle: u64, + /// Last cycle known to agree, or `None` when the **first** window differs + /// and so nothing is known to agree. + /// + /// This is an `Option` rather than a zero sentinel, and the distinction is + /// not decorative: cycle 0 is a real cycle. With `0` meaning both "no prior + /// checkpoint" and "cycle zero", the window `(0, through]` excludes cycle 0 + /// — so a divergence *at* the very first cycle reported a window that did + /// not contain it, and a full-capture re-run of that window would have + /// found nothing. Caught by the acceptance sweep at `len = 1`, which is + /// exactly the degenerate case a hand-written test set omits. + pub after_cycle: Option, /// Last cycle of the first differing window. pub through_cycle: u64, /// The reference hash at that checkpoint. @@ -364,10 +373,34 @@ pub struct Divergence { } impl Divergence { - /// Cycles in the window a full-capture re-run must cover. + /// Whether `cycle` falls in the window a full-capture re-run must cover. + /// + /// Offered rather than left to call sites because the boundary is half-open + /// on one end and open-ended on the other, and that is easy to get wrong in + /// exactly the direction that hides a bug — the same reason + /// `divergence::is_single_scanline` exists in `rustynes-probe`. + #[must_use] + pub const fn contains(&self, cycle: u64) -> bool { + match self.after_cycle { + // The first window: everything up to and including `through_cycle`. + None => cycle <= self.through_cycle, + Some(after) => after < cycle && cycle <= self.through_cycle, + } + } + + /// Cycles in the window a full-capture re-run must cover, or `None` for the + /// first window, whose start is wherever the run began rather than a cycle + /// this comparison observed. + /// + /// `None` rather than an assumed `through_cycle + 1`: that answer is only + /// right if the run started at cycle 0, and a checkpoint stream carries no + /// evidence that it did. #[must_use] - pub const fn window_len(&self) -> u64 { - self.through_cycle - self.after_cycle + pub const fn window_len(&self) -> Option { + match self.after_cycle { + None => None, + Some(after) => Some(self.through_cycle - after), + } } } @@ -398,9 +431,9 @@ pub fn compare(reference: &[Checkpoint], candidate: &[Checkpoint]) -> Comparison return Comparison::Diverged(Divergence { index: i, after_cycle: if i == 0 { - 0 + None } else { - reference[i - 1].through_cycle + Some(reference[i - 1].through_cycle) }, through_cycle: reference[i].through_cycle, reference_hash: reference[i].hash, @@ -422,6 +455,82 @@ pub fn compare(reference: &[Checkpoint], candidate: &[Checkpoint]) -> Comparison } } +/// The first cycle at which two full-capture record streams differ. +/// +/// This is the answer checkpoints are an *approximation* of, and it exists so +/// the approximation can be checked against it rather than trusted. +/// +/// Returns `None` when the streams are identical over their common prefix +/// **and** the same length. A shorter stream is a difference: a run that +/// stopped early differs from one that did not, at the first cycle it no longer +/// has. +#[must_use] +pub fn first_full_capture_difference( + reference: &[Observable], + candidate: &[Observable], +) -> Option { + let common = reference.len().min(candidate.len()); + for i in 0..common { + if reference[i] != candidate[i] { + return Some(reference[i].cpu_cycle); + } + } + if reference.len() == candidate.len() { + return None; + } + // The longer stream's first unmatched record is where they part. + let longer = if reference.len() > candidate.len() { + reference + } else { + candidate + }; + Some(longer[common].cpu_cycle) +} + +/// Whether a [`Comparison`] correctly localises a known full-capture +/// difference. +/// +/// **This is the v2.4.2 acceptance gate**, expressed as a function rather than +/// as prose in a plan, so it is executable. +/// +/// The contract checkpoints have to honour is narrow and worth stating exactly, +/// because a looser reading of it is satisfiable by a broken implementation: +/// +/// - If full capture says the streams are identical, the comparison must say +/// `Identical`. Anything else is a **false positive**, and a gate that cries +/// wolf gets switched off. +/// - If full capture says they first differ at cycle `k`, the comparison must +/// not say `Identical` — that is a **false negative**, which is the failure +/// that matters, because it passes a wrong DUT. +/// - When it says `Diverged`, the reported window must **contain `k`**. A +/// divergence report that names the wrong window sends a full-capture re-run +/// somewhere nothing is wrong, which is worse than no report: it spends the +/// debugging budget and returns "no problem here". +/// - `Inconclusive` is acceptable for a real difference — it is an honest "I +/// could not answer" — but never for identical streams. +// Clippy wants the two `false` arms merged. They stay apart because they name +// two DIFFERENT failure modes, and the comments on them are the point of the +// function: a false positive gets a gate switched off, while a false negative +// passes a wrong DUT. Merging them into one pattern deletes exactly the +// distinction this predicate exists to draw. +#[allow(clippy::match_same_arms)] +#[must_use] +pub const fn localisation_is_consistent( + comparison: &Comparison, + first_difference: Option, +) -> bool { + match (comparison, first_difference) { + (Comparison::Identical { .. }, None) => true, + // A false positive: the streams agree and the gate says otherwise. + (Comparison::Identical { .. }, Some(_)) => false, + // The failure that matters: a real difference reported as agreement. + (Comparison::Diverged(_) | Comparison::Inconclusive { .. }, None) => false, + (Comparison::Diverged(d), Some(k)) => d.contains(k), + // Honest refusal on a real difference. + (Comparison::Inconclusive { .. }, Some(_)) => true, + } +} + /// Serialize a checkpoint stream: repeated `(u64 through_cycle, u64 hash)`, /// little-endian, no header. /// @@ -621,13 +730,15 @@ mod tests { match compare(&reference, &candidate) { Comparison::Diverged(d) => { assert_eq!(d.index, 1, "cycle 5000 falls in the second window"); - assert_eq!(d.after_cycle, 4095); + assert_eq!(d.after_cycle, Some(4095)); assert_eq!(d.through_cycle, 8191); - assert_eq!(d.window_len(), 4096); + assert_eq!(d.window_len(), Some(4096)); assert!( - (d.after_cycle..=d.through_cycle).contains(&5000), + d.contains(5000), "the reported window must contain the corrupted cycle" ); + assert!(!d.contains(4095), "the window is open at its start"); + assert!(d.contains(8191), "and closed at its end"); } other => panic!("expected a divergence, got {other:?}"), } @@ -817,6 +928,216 @@ mod tests { let _ = Hasher::new(0); } + /// A tiny deterministic PRNG. Deliberately not a dependency: this crate + /// emits the goldens an external implementation is verified against, and + /// its lockfile is committed, so every dependency added here is one more + /// thing that can move underneath a provenance artifact. + struct Lcg(u64); + impl Lcg { + fn next(&mut self) -> u64 { + self.0 = self + .0 + .wrapping_mul(6_364_136_223_846_793_005) + .wrapping_add(1_442_695_040_888_963_407); + self.0 >> 11 + } + fn below(&mut self, n: u64) -> u64 { + self.next() % n + } + } + + fn hash_stream(records: &[Observable], interval: u64) -> Vec { + let mut h = Hasher::new(interval); + for r in records { + h.push(r); + } + h.finish() + } + + /// Perturb one observable field of one record, chosen by the PRNG so the + /// sweep is not silently exercising the same field every time. + fn perturb(r: &mut Observable, which: u64) { + match which % 8 { + 0 => r.pc ^= 1, + 1 => r.bus_addr ^= 1, + 2 => r.bus_data ^= 1, + 3 => r.bus_access = if r.bus_access == 1 { 2 } else { 1 }, + 4 => r.put_cycle = !r.put_cycle, + 5 => r.nmi_line = !r.nmi_line, + 6 => r.irq_line_at_low = !r.irq_line_at_low, + _ => r.irq_line_at_high = !r.irq_line_at_high, + } + } + + /// **The v2.4.2 acceptance gate: the hash checkpoints agree with full + /// capture.** + /// + /// Checkpoints are an approximation of "where do these two runs first + /// differ", traded for four orders of magnitude of disk. The whole scheme + /// is worthless if the approximation can disagree with the answer, so this + /// sweeps a corruption across every position in a run and asserts the + /// checkpoint comparison localises what full capture finds — never missing + /// a real difference, never inventing one, and never naming a window that + /// does not contain it. + /// + /// The last of those three is the one worth the effort. A divergence report + /// naming the wrong window sends a full-capture re-run somewhere nothing is + /// wrong, spends the debugging budget, and returns "no problem here" — which + /// reads as evidence the DUT is fine. + #[test] + fn checkpoints_localise_what_full_capture_finds() { + let mut rng = Lcg(0x5EED_1234_ABCD_0001); + let mut cases = 0; + + // Lengths chosen around the interval boundary on purpose: exactly one + // window, one short of a boundary, exactly on one, and several with a + // partial tail. + for &len in &[1u64, 2, 4095, 4096, 4097, 8192, 8193, 10_000, 12_288] { + let reference: Vec = (0..len).map(obs).collect(); + let ref_ck = hash_stream(&reference, DEFAULT_INTERVAL); + + // Identical must never report a difference. + assert!( + localisation_is_consistent( + &compare(&ref_ck, &hash_stream(&reference, DEFAULT_INTERVAL)), + first_full_capture_difference(&reference, &reference), + ), + "len {len}: identical streams were not reported identical" + ); + cases += 1; + + // Then a corruption at every position for short runs, and a + // randomised sweep for long ones. + let positions: Vec = if len <= 16 { + (0..len).collect() + } else { + let mut v = vec![ + 0, + 1, + len - 2, + len - 1, + DEFAULT_INTERVAL - 1, + DEFAULT_INTERVAL, + ]; + v.retain(|&p| p < len); + for _ in 0..40 { + v.push(rng.below(len)); + } + v + }; + + for pos in positions { + let mut candidate = reference.clone(); + perturb( + &mut candidate[usize::try_from(pos).expect("fits")], + rng.next(), + ); + + let k = first_full_capture_difference(&reference, &candidate); + assert_eq!( + k, + Some(pos), + "len {len}: full capture should first differ at the perturbed cycle" + ); + + let cmp = compare(&ref_ck, &hash_stream(&candidate, DEFAULT_INTERVAL)); + assert!( + localisation_is_consistent(&cmp, k), + "len {len}, corrupted cycle {pos}: checkpoints did not localise \ + the difference full capture found.\n comparison = {cmp:?}" + ); + // For a same-length corruption the answer must be a located + // window, not a refusal -- `Inconclusive` here would mean the + // gate degraded to "I could not tell" on the one case it exists + // to handle. + assert!( + matches!(cmp, Comparison::Diverged(_)), + "len {len}, cycle {pos}: expected a located window, got {cmp:?}" + ); + cases += 1; + } + } + assert!(cases > 300, "the sweep must actually sweep; ran {cases}"); + eprintln!("checkpoint/full-capture agreement: {cases} cases"); + } + + /// The gate predicate must itself be able to fail, or the sweep above is + /// asserting nothing. Each arm is driven directly. + #[test] + fn the_gate_predicate_rejects_each_way_of_being_wrong() { + let identical = Comparison::Identical { checkpoints: 3 }; + let diverged = Comparison::Diverged(Divergence { + index: 1, + after_cycle: Some(4095), + through_cycle: 8191, + reference_hash: 1, + candidate_hash: 2, + }); + let inconclusive = Comparison::Inconclusive { + reason: InconclusiveReason::Empty, + }; + + // Correct outcomes. + assert!(localisation_is_consistent(&identical, None)); + assert!(localisation_is_consistent(&diverged, Some(5000))); + assert!(localisation_is_consistent(&diverged, Some(8191))); + assert!(localisation_is_consistent(&inconclusive, Some(5000))); + + // False negative: a real difference reported as agreement. The one that + // passes a wrong DUT. + assert!(!localisation_is_consistent(&identical, Some(5000))); + // False positive: agreement reported as a divergence. + assert!(!localisation_is_consistent(&diverged, None)); + assert!(!localisation_is_consistent(&inconclusive, None)); + // Right answer, wrong window -- the case that wastes a re-run. + assert!(!localisation_is_consistent(&diverged, Some(4095))); + assert!(!localisation_is_consistent(&diverged, Some(8192))); + } + + /// The degenerate case the acceptance sweep found: a divergence at cycle + /// **zero**, in the first window. + /// + /// With `after_cycle` as a plain `u64`, "no prior checkpoint" and "cycle 0" + /// were the same value, so the window `(0, 0]` was empty and did not + /// contain the very cycle that differed. A full-capture re-run of it would + /// have found nothing and read as evidence the DUT was fine. + #[test] + fn a_divergence_at_cycle_zero_is_inside_its_own_window() { + let a = vec![obs(0)]; + let mut b = vec![obs(0)]; + b[0].bus_data ^= 1; + + let cmp = compare(&hash_stream(&a, 1), &hash_stream(&b, 1)); + let Comparison::Diverged(d) = cmp else { + panic!("expected a divergence, got {cmp:?}"); + }; + assert_eq!(d.after_cycle, None, "nothing precedes the first window"); + assert_eq!(d.through_cycle, 0); + assert!( + d.contains(0), + "the window must contain the cycle that differs" + ); + assert_eq!( + d.window_len(), + None, + "the first window's start is where the run began, which a checkpoint \ + stream carries no evidence of" + ); + assert!(localisation_is_consistent(&cmp, Some(0))); + } + + /// A truncated run differs from a complete one, and full capture must say + /// so — otherwise the sweep's "identical" arm is satisfiable by a stream + /// that simply stops. + #[test] + fn a_shorter_stream_differs_at_its_first_missing_cycle() { + let long: Vec = (0..100).map(obs).collect(); + let short: Vec = (0..60).map(obs).collect(); + assert_eq!(first_full_capture_difference(&long, &short), Some(60)); + assert_eq!(first_full_capture_difference(&short, &long), Some(60)); + assert_eq!(first_full_capture_difference(&long, &long), None); + } + #[test] fn access_codes_are_stable() { assert_eq!(Observable::access_code(true, false, false), 0); diff --git a/docs/mister.md b/docs/mister.md index 4ef82f71..09f12ef4 100644 --- a/docs/mister.md +++ b/docs/mister.md @@ -294,6 +294,36 @@ different cycles cover different spans, so a hash difference between them says nothing, and calling it a divergence would send a full-capture re-run at a window where nothing is wrong. +#### The gate: checkpoints agree with full capture + +Checkpoints approximate "where do these two runs first differ", traded for four +orders of magnitude of disk, and the scheme is worthless if the approximation +can disagree with the answer. `first_full_capture_difference` computes the answer +directly; `localisation_is_consistent` states the contract, as a function rather +than as prose here. + +| full capture says | the comparison must | +|---|---| +| identical | report `Identical` - anything else is a false positive, and a gate that cries wolf gets switched off | +| first differs at `k` | **not** report `Identical` - that false negative passes a wrong DUT | +| first differs at `k`, and it reports `Diverged` | name a window **containing `k`** | +| first differs at `k` | `Inconclusive` is an acceptable, honest refusal | +| identical | `Inconclusive` is **not** acceptable | + +The third row is the one worth the effort. A divergence report naming the wrong +window sends a full-capture re-run somewhere nothing is wrong, spends the +debugging budget, and returns "no problem here" - which reads as evidence the DUT +is fine. + +A sweep drives 331 cases across every run length around the interval boundary, +corrupting a different observable field at a different position each time. It +found a real defect on its first run: **a divergence at cycle zero was reported +in a window that did not contain it.** `after_cycle` was a `u64` in which `0` +meant both "no prior checkpoint" and "cycle zero", so the first window read as +`(0, 0]` - empty. It is now `Option`, and `Divergence::contains` is offered +so call sites do not reimplement a boundary that is half-open at one end and +open-ended at the other. + #### A capacity-limited trace refuses rather than truncating `IrqTrace::push` silently drops records once it reaches the capacity it was armed