Repository navigation
fix: release build, opt-in spindle trace, suite lint after the #1097 layout move - #1115
Conversation
e2b2bcd to
30f704b
Compare
#1097 mapped cfg(test) onto cfg(debug_assertions). Two of those gates landed on items production code also uses, so `cargo check --release` failed in rbitcoin-store (lib) and, once that was patched, rbitcoin-query: - scripthash_head.rs gated `use crate::open_address::{self, MAX_LOAD_DEN, MAX_LOAD_NUM}` while `hash_slot` and the bulk-fill load cap use it. - confirm_run/mod.rs re-exported `expected_bits_extending` and `ensure_spend_abs_layouts`, both debug-only, from ungated `pub use`. No PR job compiles the release profile; release.yml and nix do. `qc` now runs `cargo check --release --workspace --exclude rbitcoin-bench` after clippy, and coverage.test.sh pins that step the way it pins the test job's commands.
`spindle_mark` pushed onto a thread-local Vec on every confirm batch. Under cfg(test) that never ran in a node. Under cfg(debug_assertions) every debug node (core-functional nightly, warnet, `cargo run`) grew the write thread's trace for the life of the process; only the one schedule test ever cleared it. The trace is now opt-in like the other debug hooks: `spindle_trace_arm` clears and starts recording on this thread, `spindle_trace_clear` stops and drops it, and an un-armed `spindle_mark` is a no-op. Co-authored-by: Cursor <cursoragent@cursor.com>
Every tests/suite.rs opened with `#![allow(unused_imports, dead_code, unused_variables)]`, which switched the compiler's warnings off for all test code. With the allow gone the workspace reported ~2000 unused imports (all from the src/ -> tests/ move; `cargo fix` removed them), ~20 unused trait imports (by hand), and one dead helper: the `RBITCOIN_BLOCK_QUEUE_*` env lock in ibd/assign.rs, which #1097 replaced with a config field. No unused variables. Imports used only inside a Windows or Darwin cfg block were checked by hand; none were removed. Co-authored-by: Cursor <cursoragent@cursor.com>
…out move #1097 set the floor to 89 and then 91; the badge kept `--gate 89` in coverage.sh and as the coverage-badge.py default, so a run between 89 and 91 would draw a passing badge on a failing gate. coverage.test.sh now reads FLOOR_PCT from coverage-gate.py and pins both to it. The IGNORE regex fixtures in coverage.test.sh name current files (tests/ and rbitcoin-ibd) instead of the src/*_tests.rs they described before the move. docs/concurrency.md links perf_log.rs where it lives now. Five rustdoc links to ChainHub and IndexMode, which rpc and node cannot resolve since IBD moved to rbitcoin-ibd, are plain code spans. Co-authored-by: Cursor <cursoragent@cursor.com>
…in hooks After #1097 the suite reaches internals through `#[doc(hidden)] pub` and test hooks are `cfg(debug_assertions)`, but CONTRIBUTING principle 11 still said to drop to `pub(crate)`, that `-D warnings` would report dead `pub`, and that a `cfg(test)` hook is a smell. The rule now states the convention and its two limits: a hook is off until a test arms it, and a debug gate never covers an import production code uses (the release check in `qc` pins that). code-shape points at it from the lint section and notes suite roots carry no blanket allow. TESTING.md records that the suites build in dev/test only. rbitcoin-ibd gets the AGENTS.md the other crates have. Co-authored-by: Cursor <cursoragent@cursor.com>
… the layout move Co-authored-by: Cursor <cursoragent@cursor.com>
The layout move turned private modules into `#[doc(hidden)] pub mod`, so rustdoc began checking 37 links it had never resolved. Those modules do not render, so a link there has no reader: they become plain code spans. The four in rendered modules (block flags, script_pool, query::testutil) get a resolving path or drop a target that no longer exists. `cargo doc --workspace --no-deps` exits 0 again. Co-authored-by: Cursor <cursoragent@cursor.com>
…ge floor `cargo check --release --workspace` unifies the consensus `corpus` default onto every crate; nix/rbitcoin.nix and release.yml build `-p rbitcoin-node -p rbitcoin-cli -p rbitcoin-store` without it, so qc checks that shape too. `--exclude rbitcoin-bench` is gone: the bench lib checks in release and its bin needs the `cli` feature, which cargo skips. `cargo doc --workspace --no-deps` runs after, so a hidden-module link cannot rot again. The badge gate had three copies (gate.py, coverage.sh, badge.py) kept in sync by a grep. coverage-gate.py writes `floor` to gate.json; coverage.sh reads it from there and the badge takes `--gate` as required. coverage.test.sh pins each, and now also fails when a fixture path no longer exists instead of matching a regex against a string. Co-authored-by: Cursor <cursoragent@cursor.com>
The test cleared the flag before the first mark, so flipping the initializer to `true` would still pass. libtest gives each test a fresh thread; the first assertion now reads the default. The why-comment on the thread-locals folds into `spindle_trace_arm`'s rustdoc. Co-authored-by: Cursor <cursoragent@cursor.com>
…fact Principle 11 claimed every debug hook is opt-in; the tree has ~12 always-on bounded counters, which the rule now allows, and three debug-only timing constants, which it names as exceptions to retire. TESTING.md owns how CI compiles the release profile (both check shapes, no job runs a release binary; the harnesses build dev). CONTRIBUTING, code-shape, ci.yml, and coverage.test.sh point there instead of restating it. Two TESTING.md neighbours still said `pub(crate)` and `#[cfg(test)]`. rbitcoin-ibd/AGENTS.md notes the net dev-dependency. The changelog fragment splits Fixed from Changed and adds the rustdoc fix. Co-authored-by: Cursor <cursoragent@cursor.com>
30f704b to
16b7afe
Compare
There was a problem hiding this comment.
Adversarial review of #1115 (four independent passes)
Scope of each pass: (1) release profile and every cfg(debug_assertions) site in src/; (2) the suite-lint cleanup (263 test files); (3) completeness against every goal of #1097 and every item in the post-merge review; (4) spindle hook, coverage scripts, docs, changelog. Dispositions below; everything marked fixed landed in the four commits on top of the original six (5b115f780..16b7afeed before rebase).
#1097 goals — all verified achieved after this PR
| Goal | Evidence |
|---|---|
One tests/suite per library, [lib] test = false |
All 15 library crates: test = false, doctest = false, autotests = false, [[test]] name = "suite". rbitcoin-bench is the documented exception. Zero #[test] in non-bench src/. cargo test -p rbitcoin-log --no-run -v shows one rustc per package — the no-double-compile benefit is real. |
test-hooks gone |
No hits in Cargo.toml, CI, scripts, nix. compiled_with_test_hooks() is the kept operator warning on cfg!(debug_assertions). |
IBD in rbitcoin-ibd; rpc/electrum/esplora/sv2 → net only |
None of the four depend on ibd or node. rbitcoin-net dev-depends on ibd for peer_header_sync_journey (now stated in crates/rbitcoin-ibd/AGENTS.md). docs/CRATES.md lists ibd; no doc still places IBD in net. |
One rbitcoin-test suite; per-hub NODE_COMPACT_FILTERS |
[[test]] suite + feature-gated overlay; bit documented at peer.rs:358. |
Post-merge review items
Every item is fixed except one that was only partially fixed before this round: the five rustdoc links the review listed were 5 of 37 — cargo doc --workspace --no-deps still failed. Root cause is #1097: private mods became #[doc(hidden)] pub mod, so rustdoc started resolving links nobody had checked. Fixed: all 37, plus a cargo doc step in qc so it can't regress. nixos-module-runtime had never run on the new layout → label added here.
Findings and dispositions
Release / cfg audit
- fixed — TESTING.md claimed the labeled harnesses "run the shipped binaries"; they
cargo build -p rbitcoin-nodein dev. No CI job executes a release binary. Text now says so. - fixed —
--exclude rbitcoin-benchwas unjustified (bench checks in release; its bin needscli, which cargo skips). Removed. - fixed —
cargo check --release --workspaceis not what ships: resolver unification turns consensuscorpuson; nix/release.yml build-p rbitcoin-node -p rbitcoin-cli -p rbitcoin-storewithout it.qcnow checks both shapes;coverage.test.shpins both. - fixed — Principle 11 said every debug hook is "opt-in"; the tree has ~12 always-on bounded counters (
IDLE_WAITERS,header_contextual_checks,tl_note_*, …). Rule now: bounded counter or opt-in; nothing accumulates unarmed. Audit confirmed the spindleVecwas the only accumulator. - follow-up (not this PR) — Three production constants differ between debug and release:
REDIAL_INTERVAL2 s/250 ms (net/service.rs:636),SAM_RETRY500/50 ms (net/i2p_sam.rs:14),CONSTRAINTS_COOLDOWN1 s/100 ms (sv2/lib.rs:53). Pre-#1097 (556d69d9c), and the functional harness exercises them at 4–10× shipped cadence. Principle 11 names them as exceptions to retire; retiring them is a behaviour change and its own PR. - note —
cargo check≠cargo build --release(no link step:mimalloc,+crt-static). Nobuild.rsor release-profile overrides in the workspace, so check catches the class that broke. Real builds stay tag-time. - verified — The two root-cause fixes are exactly right:
scripthash_head.rsusedprimary_slot/MAX_LOAD_*unconditionally; inconfirm_run/mod.rsonlyexpected_bits_extendingandensure_spend_abs_layoutsare suite-only. Remaining 13 gated imports/re-exports all have gated consumers only. 11 gated struct fields: nonerepr(C)or serialized.
Suite-lint cleanup
- clean — 263 files: every non-
usedeletion accounted for (one dead env-lock helper whose callers left in556d69d9c).#[test]count 2842 → 2843.#![allowin suite roots 15 → 0; noallow/expect/_xrenames added anywhere. Everycfg(windows)/cfg(not(unix))/cfg(target_os = "macos")block was checked against the post-diff imports; Windows and macOS CI are green.
Spindle hook
- fixed — The red test called
spindle_trace_clear()before the first mark, so aCell::new(true)initializer would still pass. First assertion now reads the fresh-thread default. - fixed —
//why-comment folded intospindle_trace_arm's rustdoc. - declined — "
#[doc(hidden)]onspindle_markis redundant sincepub mod writeis hidden." Per-item#[doc(hidden)]is #1097's convention across 3384 sites; one file won't deviate. - verified — Every
spindle_markcall site is on the caller's thread (no spawn inwrite.rs); libtest gives each test its own thread, so no cross-test leak; release body is empty.
Coverage scripts
- fixed — The floor lived in three places (gate.py, coverage.sh, badge.py) held together by a grep.
coverage-gate.pyalready writesfloortogate.json;coverage.shreads it from there and--gateis required on the badge. One owner. - fixed — Fixture paths in
coverage.test.shwere regex-matched as strings, which is how four of them rotted silently. The test nowexists()-checks each. - fixed — A comment landed between the badge-floor lines and the assertion it described.
- declined — "Restore
[rbitcoin_net::ChainHub]links in rpc/node instead of backticks." All five are in#[doc(hidden)]modules, which rustdoc does not render; a link there has no reader. Same rule applied to the other 37: links only in rendered modules. (The original commit message gave the wrong reason — "cannot resolve" — the reviewer was right about that.)
Docs / changelog
- fixed — "Release is check-only; qc runs
cargo check --release" was stated in four places. TESTING.md (Coverage) owns it; CONTRIBUTING, code-shape, ci.yml, coverage.test.sh point there. - fixed —
docs/code-shape.mdrestated principle 11; cut to the one new fact (no#![allow]in suite roots) plus a pointer. - fixed — TESTING.md neighbours still said
pub(crate)(fuzz) and#[cfg(test)] wrapper. - fixed — Changelog fragment split:
Fixed(release build, spindle, badge, rustdoc) /Changed(qc steps, suite lint). - declined — Principle 11 is one long paragraph; splitting it into sub-bullets is a style change to a doc the rest of the file formats the same way.
Residual follow-ups (none block this PR)
- Retire the three debug-only timing constants (above).
- No tool finds dead
#[doc(hidden)] pubitems (review counted 578 referenced only fromtests/); principle 11 makes it a convention. A periodiccargo +nightly udeps-style or-W unreachable_pub-based sweep would make it mechanical. TESTING.md:173cites a dated--libbaseline;scripts/coverage.sh:41IGNORE still carries_tests\.rs$patterns subsumed by/tests/. Harmless.cargo check --all-featuresfails onrbitcoin-logfmt-fuzz-anchor; pre-existing and intentional.
Verification on the final tip (after rebase onto ce95b2fd8): fmt, clippy -D warnings, deny, ast-grep, ci-os-smoke, cargo check --release both shapes, cargo doc --workspace --no-deps, coverage.test.sh (32 pass), full workspace suite (one rbitcoin-test failure traced to ENOSPC on the runner, 4/4 green after freeing disk).
Follow-up to #1097 (suite layout move). Fixes everything the post-merge review found. One commit per step.
What was broken
cfg(test)intocfg(debug_assertions), and two of those gates landed on production imports (rbitcoin-store::scripthash_head,rbitcoin-query::confirm_runre-exports). No PR check compiled release, so it shipped red forrelease.ymland nix.spindle_markrecorded on every batch in debug builds; nothing read it outside two tests.scripts/coverage.test.sh; one dead link indocs/concurrency.md; five broken rustdoc intra-doc links in rpc/node.tests/suite.rscarried#![allow(unused_imports, dead_code, unused_variables)], hiding ~2700 unused imports and dead helpers.pub(crate)+cfg(test);rbitcoin-ibdhad noAGENTS.md.Changes
Four review-round commits sit on top of the six below (see the review comment for dispositions): rustdoc links in
doc(hidden)modules (37) +cargo docinqc;qcrelease check in both shapes without the bench exclusion +gate.jsonas the single floor owner + fixture-existence pins; spindle red test pins default-off; principle 11 scoped to the tree and one owner for the release-check fact.fix: release profile compiles again; qc checks it— drop the cfg on the store import; split the two debug-only re-exports in query; new qc stepcargo check --release --workspace --exclude rbitcoin-bench, pinned byscripts/coverage.test.sh.fix: spindle trace records only after a test arms it— thread-local arm flag,spindle_trace_arm()/spindle_trace_clear(); red testspindle_mark_records_only_while_armedfirst.layout: suite roots drop the blanket allow— all 15 suite roots now lint likesrc;cargo fixremoved the imports, ~20 trait imports and three dead ibd helpers removed by hand. One commit rather than per-crate: 99.8% of the fallout was mechanical.coverage: badge gate follows the 91% floor; stale paths— badge/gate to 91 with a self-test pin; fixture paths; links.docs: principle 11 …— CONTRIBUTING principle 11 + checklist, code-shape Lint paragraph, TESTING release-profile note, newcrates/rbitcoin-ibd/AGENTS.md.changelog.d/release-build-after-layout.md(Fixed).Choices to push back on
cargo checkin two shapes, notcargo build/cargo test --release(suites link debug hooks, so release tests cannot build). Documented in TESTING.md.Not in scope (follow-ups)
REDIAL_INTERVAL,SAM_RETRY,CONSTRAINTS_COOLDOWN) differ from release; named in principle 11 as exceptions to retire.cargo test --releaseis no longer possible by construction; if release-profile test runs are wanted, the hooks need a feature instead ofcfg(debug_assertions).Verification
Per-step and on the rebased tip:
cargo fmt --check,cargo clippy --workspace --all-targets -D warnings,cargo deny check,./scripts/ast-grep.sh,cargo test --workspace,./scripts/ci-os-smoke.sh,cargo check --release(workspace and product shapes),cargo doc --workspace --no-deps— all green.