fix(fn-dsa): shake256x4 could not be built per-crate — missing feature forwarding - #76
Open
Nexlab-One wants to merge 3 commits into
Open
fix(fn-dsa): shake256x4 could not be built per-crate — missing feature forwarding#76Nexlab-One wants to merge 3 commits into
Nexlab-One wants to merge 3 commits into
Conversation
🔍 Pull Request SummaryGenerated: Wed Jul 29 12:54:03 UTC 2026 📋 Validation Results
✅ Overall Status: PASSED🔒 Security Checklist
📝 Review NotesPlease review the security implications of this change carefully. ✅ Automated validation passed! This PR is ready for review. |
🔒 Security Validation ReportGenerated: Wed Jul 29 12:57:46 UTC 2026 📊 Summary
✅ Overall Security Status: PASSEDAll critical security validations passed successfully. 🔍 DetailsThis report covers:
📋 Next Steps✅ Security validation passed. Code is ready for deployment. ✅ Security validation passed! This code meets all security requirements. |
…built
`cargo check -p lib-q-fn-dsa-kgen --features shake256x4` failed with E0433 and
`-p lib-q-fn-dsa-sign` with E0425: the feature flips both crates' call sites onto
`fn_dsa_comm::shake::SHAKE256x4`, but they declared `shake256x4 = []`, so -comm —
where that type is itself `#[cfg(feature = "shake256x4")]` — was never compiled
with the feature. rustc said so explicitly ("found an item that was configured
out ... gated behind the `shake256x4` feature"). The type was never missing; the
manifest wiring was.
WHY IT SURVIVED SINCE IMPORT
The only feature coverage in CI is workspace-wide (`--all-features`, ci.yml:76 /
cd.yml:78). There the `lib-q-fn-dsa` facade — which does forward — enables the
flag on -comm, and feature unification hands that same -comm build to every leaf,
so a leaf that forwards nothing still compiles. Only per-crate selection
(`-p <leaf> --features shake256x4`, `-p <leaf> --all-features`) exposes it, and no
CI row ever did that. The defect is inherited: upstream fn-dsa 0.3.0 declares the
same empty features in fn-dsa-kgen/fn-dsa-sign and only works because its facade
enables all three. A future re-vendor must not "restore" the empty form.
WHY FIX THE WIRING RATHER THAN REMOVE THE FEATURE
`SHAKE256x4` is present and complete (portable + dedicated AVX2 backend), and it
is verified, not assumed: comm's own oracle (fn-dsa-comm/src/shake.rs) compares it
against four 64-bit-interleaved SHAKE256 streams over all 301 seed lengths
0..=300 and passes on BOTH dispatch paths (`--features shake256x4` and
`--features shake256x4,no_avx2`), and fn-dsa-kgen's `gauss::sample_tv` shake256x4
vectors are byte-identical to upstream 0.3.0's. Deleting a working,
upstream-faithful, test-covered feature to fix a two-line manifest bug would be
the wrong trade. Enabling the forward cannot regress any build that compiles
today: such a build already had comm/shake256x4 on, or it would not have compiled.
REGRESSION COVERAGE (both were proven to fail on the reverted manifest)
- lib-q-fn-dsa/fn-dsa-kgen/tests/feature_forwarding.rs reads the manifests as
data, so `cargo test -p lib-q-fn-dsa-kgen` — which the CI workspace pass runs,
the crate is a member and is not in the --exclude list — catches a missing
forward without having to build the broken configuration. It cannot pass
vacuously: the crate pairs it must evaluate are asserted, so a manifest reshape
that blinds the parser fails the test instead of silently skipping it.
- .github/actions/test-fn-dsa/action.yml gains the compile-level counterpart:
per-crate `cargo check --features shake256x4` / `--all-features`, plus the
SHAKE256x4 equivalence oracle on both dispatch paths and the feature-on keygen
and signing tests. One `-p` per invocation — merging them re-unifies the
features and tests nothing.
NOT FIXED HERE
`no_avx2` has the same missing forward in -kgen/-sign/-vrfy. It is not
build-breaking (it gates internal dispatch, not a cross-crate item); adding the
forward would change which SHAKE code path existing per-crate builds select, which
is a behaviour change that belongs with the portable-keygen work, not a build fix.
The guard test is deliberately scoped to `shake256x4` and documents this.
Verified on x86_64 Windows with the pinned toolchain (rustc 1.99.0-nightly
89c61a754 2026-07-23). No non-x86_64 target was built; the change is manifest-only
and target-independent by construction, and the portable dispatch path was
exercised on x86_64 via `no_avx2`.
… so untruthfully
The shake256x4 guard added in fe7cfbd inspected only DIRECT entries of the
`shake256x4` feature list. Cargo does not: a feature entry with no `/` names
another feature of the same crate, which cargo expands in turn. So this wiring
in fn-dsa-kgen/Cargo.toml
_comm_x4 = ["lib-q-fn-dsa-comm/shake256x4"]
shake256x4 = ["_comm_x4"]
is correct — `cargo check -p lib-q-fn-dsa-kgen --features shake256x4` exits 0 —
yet the guard failed it (exit 101), and the failure text read:
`cargo check -p lib-q-fn-dsa-kgen --features shake256x4` will fail to compile.
That sentence was asserted flatly and was false in that state. The test runs no
compiler; it was predicting an outcome it had never observed, and predicting it
wrong. That is the worse half of the bug: a false positive costs one confused
afternoon, but a guard that states a falsehood about WHY it failed teaches the
next person to distrust it and delete it — taking the real defect's only
detector with it.
WHAT CHANGED
Both available remedies, because they fix different faults and the second costs
nothing once the first is in place:
1. `resolve_feature` walks the feature graph within a manifest transitively
(worklist + `seen` set), so indirect wiring is accepted exactly as cargo
accepts it. Entries naming another crate (`dep/feat`, `dep?/feat`) or an
optional dep (`dep:name`) are terminal. `seen` makes a cyclic manifest
terminate instead of hanging; cargo diagnoses cycles itself.
2. The failure message now states only what was inspected — the resolved entry
set, and that none of it forwards — then names what it did NOT do ("NOT
checked: whether anything compiles. This test runs no build"), points at the
`cargo check` counterpart in .github/actions/test-fn-dsa/action.yml, and
gives the cross-crate cfg reasoning as rationale rather than as a prediction
about this particular build.
Transitive resolution alone would have left the message free to lie the next
time it fired; an honest message alone would have left the false positive.
NOT WEAKENED
The defect that was live on main is still caught. Verified in both directions on
the real manifest, restored afterwards:
shake256x4 = ["lib-q-fn-dsa-comm/shake256x4"] guard 0 check 0
_comm_x4 = [...]; shake256x4 = ["_comm_x4"] guard 0 check 0 (was 101)
shake256x4 = [] guard 101 check 101 (E0433)
The guard's verdict now matches the compiler's on all three. Eight unit tests
over synthetic manifests pin both directions permanently, so the next person does
not have to hand-edit and hand-restore Cargo.toml to trust it: empty list, wrong
target feature and dangling indirection are rejected; direct, indirect, multi-hop
and weak (`dep?/feat`) forwards are accepted; cycles terminate.
REQUIRED_PAIRS gains ("lib-q-fn-dsa", "lib-q-fn-dsa-alg"), the outermost link in
the chain, so every crate in the `cargo check -p <crate> --features shake256x4`
sweep is now pinned as must-be-evaluated. Confirmed non-vacuous: breaking that
forward to `[]` fails the guard with the forwarding message.
Re-confirmed green on the pinned toolchain (nightly-2026-07-24), x86_64 Windows:
`cargo check -p <crate> --features shake256x4` exits 0 for -comm, -kgen, -sign,
-alg and lib-q-fn-dsa. That sweep is not vacuous — cargo errors on an undeclared
feature ("the package 'lib-q-fn-dsa' does not contain this feature"), so exit 0
means the feature really was selected. `cargo clippy -p lib-q-fn-dsa-kgen
--all-targets` and `cargo fmt --check` are clean.
…r errors "Why the forward is required" claimed every missing shake256x4 forward gets "configured out" as E0433/E0425. That's only true for pairs where the dependent's own code directly names an item the dependency gates behind the same flag (kgen/sign naming -comm's SHAKE256x4). For alg->kgen, alg->sign, alg->comm, and lib-q-fn-dsa->alg it isn't: the build succeeds silently and the crate keeps un-forwarded behavior instead — a correctness bug with no compiler diagnostic. The lib-q-fn-dsa->alg case is sharper still: that crate has zero shake256x4 cfg sites of its own, so the old text's "turns on this crate's own #[cfg(...)] code" premise didn't even apply. Verified: emptying alg's forward to -kgen or -sign compiles clean but fails `cargo test -p lib-q-fn-dsa-alg --features shake256x4 test_kat` on a byte mismatch, not a compile error. Emptying alg->comm alone produces no failure at all in isolation (kgen/sign's own forwards already turn comm's feature on transitively). Emptying the top facade's forward compiles clean and passes every existing test — lib-q-fn-dsa/src has no shake256x4 cfg sites and no KAT test exists at that layer to catch the divergence. The guard logic and REQUIRED_PAIRS list are unchanged; only the inline rationale text changes.
Nexlab-One
force-pushed
the
fix/shake256x4-feature
branch
from
July 29, 2026 13:05
69b6582 to
3011382
Compare
🔒 Security Validation ReportGenerated: Wed Jul 29 13:46:56 UTC 2026 📊 Summary
✅ Overall Security Status: PASSEDAll critical security validations passed successfully. 🔍 DetailsThis report covers:
📋 Next Steps✅ Security validation passed. Code is ready for deployment. ✅ Security validation passed! This code meets all security requirements. |
🔍 Pull Request SummaryGenerated: Wed Jul 29 13:47:13 UTC 2026 📋 Validation Results
✅ Overall Status: PASSED🔒 Security Checklist
📝 Review NotesPlease review the security implications of this change carefully. ✅ Automated validation passed! This PR is ready for review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cargo check -p lib-q-fn-dsa-kgen --features shake256x4failed to compile onmain:shake256x4is not a stray flag — it is wired end to end (lib-q-fn-dsa->-alg->-comm/-kgen/-sign), so any consumer enabling it hit a hard build error.The actual cause (not the obvious one)
SHAKE256x4does exist, atfn-dsa-comm/src/shake.rs:809, behind#[cfg(feature = "shake256x4")]. rustc says so in anote:attached to that very error —note: found an item that was configured out.The defect is missing per-crate feature forwarding:
lib-q-fn-dsa-kgendeclaredshake256x4 = []instead of forwarding tolib-q-fn-dsa-comm/shake256x4, so building that crate alone turned on the call sites while leaving the implementation configured out.That also explains why the workspace-wide
--all-featuresclippy step never caught it: at workspace scope, feature unification enableslib-q-fn-dsa-comm/shake256x4through the umbrella chain, so the symbol is present and everything compiles. The break is visible only in a per-crate build, which no CI job performed.What this changes
Forward the feature per-crate across the chain, add a manifest guard test, and add a CI step that builds each crate individually with the feature so a future per-crate wiring break is caught.
A guard defect found in review and fixed
The guard "Why the forward is required" rationale asserted that every pair fails with
E0433/E0425(items configured out). Checking each pair individually showed the reality is more varied — and more interesting:kgen->comm,sign->commalg->kgen,alg->signalg->commlib-q-fn-dsa->algSilently wrong cryptographic output without a compile error is the more alarming case, and the rationale now says so instead of flattening everything to "E0433". Guard logic is unchanged — only the explanation, which was asserting a compile outcome it had not verified.
Verified
cargo check --features shake256x4now succeeds for all five crates in the chain:-comm,-kgen,-sign,-alg, and thelib-q-fn-dsaumbrella. Guard test: 9 passed. Confirmed both directions — the guard still fails on a genuinely broken manifest, and no longer false-positives on a transitively-correct one.Not checked
SHAKE256x4is correct. This PR makes a declared feature buildable; it does not audit the 4-way SHAKE256 PRNG itself, and no CI row exercises FN-DSA keygen under it.