feat(v2.4.1): "Fabric" — RustyNES as a co-simulation oracle for an FPGA device-under-test

[doublegate]

RustyNES as a co-simulation oracle for an FPGA device-under-test

Opens the v2.4.1 → v2.5.0 "Fabric" line (ADR 0037):
a new NES core written in SystemVerilog from public hardware documentation, in a
sibling RustyNES_MiSTer repository, verified against this emulator. This PR
lands the half that lives here — the boundary between the two — plus the decision
record, the execution plan, and the research archive behind them.

RustyNES is not being ported to FPGA, and cannot be

A MiSTer core is SystemVerilog compiled by Quartus 17.0.2 into a Cyclone V
bitstream. Rust does not become a bitstream, and high-level synthesis of a
cycle-accurate emulator's control flow does not produce usable hardware. What is
buildable is a new implementation with RustyNES as its verification oracle —
the one role it is uniquely equipped for, and the reason to attempt this here.

The reference firewall extends to HDL accordingly: NES_MiSTer and fpganes
rtl/ are strict black boxes, instantiable as opaque modules to compare
outputs, never readable as source.

What lands

crates/rustynes-cosim is a pure wrapper. No core API, no core behaviour change,
outside workspace.default-members (verified through cargo metadata, not
assumed). Two surfaces: a narrow C ABI a Verilator testbench links, and the safe
Rust API a golden-export binary uses. nes_golden_export emits five formats — the
CpuBootTrace binary, the per-cycle IRQ/bus CSV, a pre-palette u16 index
framebuffer, the 2 KiB work RAM the AccuracyCoin RAM decoder reads, and a
provenance manifest.

Replay, not lockstep, because Nes exposes run_frame() and
step_instruction() and nothing finer — cycle-lockstep would mean new core API on
the hot path, and would gain nothing, since the determinism contract already makes
a pre-recorded trace exactly the trace a lockstep run produces.

The framebuffer is exported pre-palette on purpose: a palette difference must
not be able to masquerade as a rendering difference — the failure mode v2.3.8
"Parallax" was built to prevent.

Two findings the crate was not looking for

No CI invocation had ever enabled cpu-boot-trace or irq-timing-trace for
clippy, so those two rustynes-core modules had never passed the lint gate.
Turning them on surfaced six pre-existing errors, all fixed. --workspace --all-targets reads as exhaustive and is not: it covers each crate's default
feature set, so a cfg-gated module is invisible to it. This crate enables both
features unconditionally — a build without them would export empty goldens,
an absence of signal that reads exactly like agreement.

The first run_frame() after power-on advances zero cycles. The PPU is
constructed at dot 340 of the pre-render line, so the 7-cycle reset sequence ticks
past the frame wrap and leaves frame_complete latched. Measured, not inferred:
frame 0 advances the cycle counter by 0, frames 1..3 by ~29,780 each. Every other
caller runs thousands of frames so it is invisible to them — but a bare
for _ in 0..n loop would emit an (n−1)-frame golden under a manifest claiming n.
Oracle::advance_frames gates on the frame counter, with a jam bail-out, and
the quirk is pinned by a test that names it.

The rung-0 gate, shown able to fail

tests/null_dut_self_diff.rs feeds RustyNES's own golden back in as if it were the
DUT and requires zero divergences — plus a one-bit corruption being caught
(a comparator that always agrees passes a self-diff trivially), and five frames
being five NTSC frames' worth of cycles in six run_frame() calls.

Verified against the real cpu_boot_trace_diff CLI: All 5464 aligned records match, exit 0; and on a one-bit corruption it reports the divergence at cycle 561,
PC $C419, naming the field and both values. The recorded ROM SHA-256 matches an
independent sha256sum, and 148905 cycles ÷ 29780.5 = exactly 5.000 frames
where a bare loop gives 4.0.

Scope, stated rather than discovered

v2.5.0 is scoped to "the 6502 rung closes" — the harness plus a cycle-exact
6502, gated on nestest 0-diff and per-cycle bus equality. A from-scratch
cycle-accurate NES core is 7–13 months FTE against a two-to-four-week window at
demonstrated cadence, so PPU, APU and MiSTer integration become the v2.6–v2.9
programme.

Two risks accepted in writing: NES_MiSTer scores 121/125 on AccuracyCoin
where real Famicom AV hardware also scores ~121/125, so there is no published
accuracy headroom and the core may be declined as a duplicate (Retro Remake is
the planned fallback home); and the oracle can be wrong, since 141/141 is not
"matches silicon", so every rung is labelled by whether it has an independent
oracle.

Verification

AccuracyCoin 141/141 (100.00%, RAM decoder) and nestest 0-diff — verified,
not asserted, since rustynes-core changes. Workspace 2246 passed / 129 suites / 0 failed. Full clippy matrix, both wasm32 combinations, no_std thumbv7em,
rustdoc with warnings as errors. Three new assertions proven by mutation, including
one caught as a hang rather than a failure.

🤖 Generated with Claude Code

https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f9bba444-2aef-4ca2-90fd-16823cf4289d

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[github-actions]

Antigravity review (Gemini via Ultra)

Adds rustynes-cosim, a C ABI and CLI for exporting deterministic NES emulation state as a verification oracle for a SystemVerilog DUT, and excludes it from the Cargo workspace to avoid forcing diagnostic trace features into the core builds.

Blocking issues

• Missing C ABI capability for correct frame advancement: The PR correctly notes that the first run_frame() after power-on is swallowed by the reset latch, and adds Oracle::advance_frames(n) to safely handle this for the Rust CLI. However, the C API only exposes rn_run_frame and lacks any way to query the frame counter (rn_frame), query the CPU jam state (rn_is_jammed), or advance multiple frames robustly (rn_advance_frames). A Verilator C++ testbench using the C API will be forced to write a bare rn_run_frame loop and will inevitably hit the exact off-by-one frame desync and infinite-loop bugs documented in the PR.

Suggestions

• crates/rustynes-cosim/src/lib.rs (around line 828 in rn_open): Oracle::new returns a Result with an error message on failure, but rn_open maps this to a flat NULL pointer, destroying the diagnostic context. Consider adding a way for the C API to retrieve the last error message (or returning an error code via an out-pointer) to align with the no-swallowed-errors principle correctly applied in write_error_code.
• crates/rustynes-cosim/src/bin/nes_golden_export.rs: File I/O operations and ROM parsing use .unwrap_or_else(|e| panic!(...)). Per the project style guide (typed results over panics), consider returning a Result from main instead of panicking on user-provided file paths.

Nitpicks

• crates/rustynes-cosim/src/bin/nes_golden_export.rs: frames is parsed as a u32 and then cast via u64::from() when calling advance_frames. You can parse it directly as u64.
• In AGENTS.md and scripts/agy-review-selftest.sh, "behaviour" and "behavior" are used inconsistently when describing the Antigravity reviewer fix.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-20 19:03 UTC

Antigravity review (Gemini via Ultra)

Adds rustynes-cosim to expose the emulation core via a C ABI and a CLI tool, generating simulation goldens to verify an independent FPGA HDL implementation.

Blocking issues

• Unvalidated external input reaching a sink: rn_write_cpu_boot_trace and rn_write_irq_trace_csv (crates/rustynes-cosim/src/lib.rs) pass the unvalidated path parameter from C directly to std::fs::write. A malicious or buggy testbench could traverse directories (e.g., ../../../etc/passwd) or overwrite arbitrary files.
• Panics on untrusted input: nes_golden_export.rs uses unwrap_or_else(|e| panic!(...)) for file reads, file writes, and ROM parsing. Project rules mandate typed results over panics on external input; refactor main to return a Result and use ?.
• Missing FFI boundary validation: rn_set_buttons casts port: c_uint directly to usize for Nes::set_buttons. If the core expects port < 4, an out-of-bounds C input will trigger a panic across the FFI boundary (Undefined Behavior). Add an explicit bounds check.
• Build breakage on stable Rust:
  • The let-chain syntax (if self.irq_trace.is_some() && let Some(level) = ...) in crates/rustynes-core/src/bus.rs requires the unstable let_chains feature.
  • The .is_multiple_of() method on integers in crates/rustynes-core/src/cpu_boot_trace.rs requires the unstable is_multiple_of feature.
    Revert both to their stable equivalents (nested ifs and %).

Suggestions

• crates/rustynes-cosim/src/bin/nes_golden_export.rs: If the hex crate is in the workspace, hex::encode(digest.as_slice()) is simpler than the manual byte-shifting loop in sha256_hex.

Nitpicks

• The PR title claims v2.4.1, but Cargo.lock shows rustynes-cosim resolving to workspace version 2.3.9.
• The CHANGELOG and PR description claim "The emulation core is untouched", but files in rustynes-core were modified to fix lints.
• Unnecessary spelling churn (behaviour -> behavior) in the scripts/agy-review*.sh comments.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-20 18:28 UTC

Antigravity review (Gemini via Ultra)

This PR introduces the rustynes-cosim crate, providing a C ABI and CLI tool to use RustyNES as a verification oracle for an HDL NES core.

Blocking issues

• Unvalidated external input reaching a sink: In crates/rustynes-cosim/src/lib.rs, rn_write_cpu_boot_trace and rn_write_irq_trace_csv accept unvalidated path strings from C and pass them directly to std::fs::write without checking for path traversal or authorized locations.
• Clear error handling: crates/rustynes-cosim/src/bin/nes_golden_export.rs uses .expect() and .unwrap_or_else(|e| panic!(...)) for file I/O and parsing instead of returning typed Results (e.g., lines 123, 129, 134, 135). This panics on untrusted input.
• Silent failure paths: In crates/rustynes-cosim/src/bin/nes_golden_export.rs line 117, the return value of write!(s, "{b:02x}") is explicitly swallowed with let _ =, violating the error handling rules.
• Correctness/Build Failure: In crates/rustynes-core/src/cpu_boot_trace.rs line 415, body.len().is_multiple_of(RECORD_SIZE) relies on an unstable Nightly API for integers, breaking stable compilation. Revert to body.len() % RECORD_SIZE != 0.

Suggestions

• crates/rustynes-core/src/bus.rs lines 3044-3045: Revert the if-let chain refactor. The original nested if statements already achieved the exact short-circuiting described in the new comment. Modifying it violates the "smallest correct change" rule and relies on newer/unstable syntax.

Nitpicks

• crates/rustynes-cosim/src/bin/nes_golden_export.rs: The manual argument parsing loop is fragile; consider returning a Result instead of immediately exiting the process with usage() during parse failures.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Earlier review rounds (newest first)

Round reviewed at 2026-08-20 18:23 UTC

Antigravity review (Gemini via Ultra)

This PR adds a co-simulation crate to generate and export golden trace vectors for verifying an independent HDL NES core against RustyNES.

Blocking issues

• Feature Unification / Core Modification: rustynes-cosim is added to the workspace members and unconditionally enables cpu-boot-trace and irq-timing-trace on rustynes-core. Cargo's workspace feature unification means these heavy tracing paths will be enabled for all crates during a default or --workspace build, severely degrading emulator performance and violating the "emulation core is untouched" / "absent from the default build" claims.
• Correctness (Filename Corruption): In crates/rustynes-cosim/src/bin/nes_golden_export.rs, base.with_extension("...") is used on a path derived from file_stem(). If the ROM name contains a dot (e.g., Super Mario Bros. 3.nes), with_extension truncates the name at the last dot, resulting in corrupted output filenames. Append to the filename string instead of using path extension replacement.
• Ignored Return Value: In crates/rustynes-cosim/src/bin/nes_golden_export.rs, sha256_hex ignores the result of write!(s, "{b:02x}") using a let _ = binding, explicitly violating the project rule against ignored return values.
• Swallowed Errors: In crates/rustynes-cosim/src/lib.rs, rn_write_cpu_boot_trace and rn_write_irq_trace_csv silently swallow std::io::Error via Err(_) => -3, hiding the actual cause of file write failures (e.g., disk full, permission denied) from the C ABI caller.

Suggestions

• crates/rustynes-core/src/bus.rs: Verify that if self.irq_trace.is_some() && let Some(level) = ... (let_chains) is actually stabilized in your MSRV. If it's still unstable, this will break the build.
• crates/rustynes-cosim/src/bin/nes_golden_export.rs: You can avoid the fmt::Write error-handling loop in sha256_hex entirely by formatting the digest directly: format!("{:x}", Sha256::digest(data)) (the output type implements LowerHex).

Nitpicks

None found.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

[doublegate]

Feature unification — you were right, and the fix is exclusion

Verified before acting, and the measurement is worth stating because it partly
contradicts the finding's framing.

cargo build --workspace compiled rustynes-core once with the union
(--message-format=json, not inferred):

before: ['cpu-boot-trace', 'debug-hooks', 'default', 'hd-pack', 'irq-timing-trace', 'std']
after:  ['debug-hooks', 'default', 'hd-pack', 'std']

The performance claim was overstated. The instrumented build costs
+1.24% / +1.39% / +1.89% across the three full_frame benches — below this
project's own 3% adoption bar, and it never touched the shipped binary
(-p rustynes-frontend doesn't select this crate) or the perf gate
(cargo bench -p rustynes-core).

The real problem was worse than performance. irq-timing-trace selects a
different for sub_dot in 0..3 loop in Bus::tick_one_cpu_cycle — there are
two, under opposite cfgs. So CI's cargo test --workspace --release --features test-roms, the accuracy battery, was validating a scheduler no user runs.
That's this project's v2.3.4 defect repeating, and it's wrong at any percentage.

The crate is now in [workspace] exclude. Both costs of that are mechanically
closed rather than trusted:

• cosim_manifest_audit.rs asserts every duplicated field and lint still equals
  the workspace's, and that the crate is still excluded — so re-adding it to
  members fails a test. Four mutations, all caught.
• CI gains explicit fmt, clippy and test steps, since --all/--workspace
  no longer reach it.

The clippy step reported a must_use_candidate on its first run that
--workspace had never surfaced — so it isn't ceremony.

The other three findings

Filename corruption — real, fixed. with_extension truncated at the last
dot: Super Mario Bros. 3 → Super Mario Bros.ram.bin. Verified with a two-line
program. Now appends; the test asserts the correct name and that it differs from
with_extension's, so a "simplify this" is caught.

Discarded fmt::Result — fixed, built by hand with nothing fallible.
Your suggested format!("{:x}", …) does not compile: sha2 0.11 returns
hybrid_array::Array<u8, _>, which has no LowerHex — checked against a scratch
crate. Digests are pinned against the known SHA-256 of "" and "abc".

Swallowed io::Error — fixed. Was a flat -3 for every write failure; now
-(100 + errno), with -1..-9 reserved for malformed calls.

One suggestion declined

let-chains "verify they're stable in your MSRV". They are, and this is the
eighth time it has been raised. Stable in edition 2024 on the pinned stable
1.96.0; the identical construct has been in libretro_info_audit.rs on main
since v2.3.5; and the fmt + clippy + rustdoc job has compiled it green on this
branch.

---

AccuracyCoin 141/141 (100.00%, RAM decoder) and nestest 0-diff on the
shipped path, plus the full gate matrix and the excluded crate's own three gates.