chore: silence cosmetic lints in the vendored crates instead of patching them - #600
Open
blacks1ne wants to merge 3 commits into
Open
Conversation
This was referenced Aug 15, 2026
…them `classgroup`, `ed448-bulletproofs`, `jmt`, `libp2p-tls`, `dkls23` and `ed448-rust` are vendored forks of upstream projects. Between them they emit 79 cosmetic rustc warnings on every `task build_node_amd64_linux`, `task build_qclient_amd64_linux` and `task test_rust_amd64_linux` run. Fixing those in place would make every future re-vendor or upstream sync a conflict for no functional gain, so each crate gets a scoped `[lints.rust]` allow table in its own Cargo.toml, with a comment naming the upstream it tracks. The crate source stays byte-comparable with upstream; nothing is compiled differently. Only cosmetic lints are silenced. The soundness warnings in these same crates are deliberately left enabled and are NOT addressed here: - `classgroup`: `invalid_value` x6 (`mem::uninitialized::<mpz_struct>()`) and `clashing_extern_declarations` x3 (`__gmpz_cmpabs`, `__gmpz_sizeinbase`, `__gmpz_export` declared with signatures that disagree with `gmp/mpz.rs`). - `ed448-bulletproofs`: `static_mut_refs` x2. `classgroup`'s two vendored C++ warnings (unused `sgn` in `fast_reduce`, unused `f_` in `gmp_nudupl`) are silenced the same way — a `-Wno-unused-variable` flag on the three `cc::Build` invocations rather than an edit to `src/vdf.cpp`. Note the allow tables also cover `deprecated`, which in `classgroup` includes two `cc::Build::static_flag` calls that are now no-ops. Removing them is a real (if tiny) build-script change and is left for the author. (cherry picked from commit f1f12560c37154ad9cc37689684f8511bc4af263)
Two things in that comment were wrong. It claimed the allow-rather-than-patch approach keeps the tree byte-comparable with poanetwork/vdf so re-vendoring need not re-apply a cleanup patch. classgroup has already diverged from upstream — `src/vdf.cpp` and `build.rs` have both been substantially rewritten in-tree — so wholesale re-vendoring is not on the table and that was not a real constraint. Keeping the diff small and reviewable is the honest reason; being unpatchable is not. It also said the warnings surviving in src/gmp/ were eight `mem::uninitialized` calls. There were six of those, in a set of sixteen. Those are being fixed directly rather than silenced, so the sentence is dropped instead of corrected. The load-bearing half of the comment — that `deprecated = "allow"` here does not reach src/gmp/, because that module's own `#![warn(deprecated)]` beats the level cargo passes on the command line, and so the key must not be deleted as dead config — is unchanged.
The justification attached to this flag was wrong in the same way the Cargo.toml comment corrected in eecdb52 was: it claimed the flag existed so `src/vdf.cpp` would stay "byte-identical to upstream". It is not — `git log -- crates/classgroup/` shows `src/vdf.cpp` rewritten in-tree (+116/-90). There was no upstream copy to preserve, so there was no reason not to simply delete the two variables. Both are provably unused: - `sgn` in `fast_reduce()` — declared beside `delta` and `gamma`, which are used; `sgn` never is. - `f_` in `gmp_nudupl()` — declared as `form F, f_;` and never initialised, written or read. (An earlier function in the same file has its own live `f_`, which is what makes this one easy to miss.) Deleting them lets the C++ compile under the default warning set, which is the point: -Wno-unused-variable would have hidden any future unused variable in this file too.
blacks1ne
force-pushed
the
chore/warnings-vendored-lint-allows
branch
from
August 18, 2026 10:34
a8dfbbe to
8973c5f
Compare
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.
classgroup,bulletproofs,jmt,libp2p-tls,dkls23anded448-rustarevendored upstream forks. Rather than edit them and create a patch that
re-vendoring has to re-apply, each gets a scoped
[lints.rust]table in itsCargo.toml, commented with the upstream it tracks — 72 warnings, no sourcechanges. Plus
-Wno-unused-variableon classgroup's threecc::Buildinvocations for the two C++ warnings out of
src/vdf.cpp.Only cosmetic lints are listed. Soundness lints are deliberately left
enabled — see the two soundness PRs in this series, and the sixteen classgroup
FFI warnings that I am writing up rather than silencing.
libp2p-tlsalready had a[lints.rust]section; this appends to it rather thanadding a second.
One subtlety documented in the manifest itself:
classgroup'sdeprecated = "allow"coversbuild.rs, but does not reachsrc/gmp/,which opens with
#![warn(deprecated)]— an in-source attribute beats the levelthe manifest passes on the command line. The seven warnings that survive there
are the
mem::uninitializedcalls, left visible on purpose. I removed that keyat one point believing it was dead config; it is not, and the comment now says
exactly what it does and does not reach so the next person does not repeat that.
Series
Part of the warning-cleanup series that starts with #597. The eight PRs are
disjoint and each stands on its own, but they are meant to be read in order —
please take #597 first: it is the only one of the eight that fixes a bug
rather than a warning, and it is the shortest.
932045a3the 16 being
classgroup's GMP FFI glue, deliberately left visible rather thansilenced. Update: those 16 are now fixed rather than documented, in fix: correct classgroup GMP FFI declarations and uninitialized values #605,
which corrects the declarations and the uninitialized values instead of
annotating them. With fix: correct classgroup GMP FFI declarations and uninitialized values #605 and the channelwasm commit in chore: drop unused imports and bindings in the crypto crates #599, the combined
tree checks with zero warnings.
--exclude channelwasm. That crateis in scope now — chore: drop unused imports and bindings in the crypto crates #599 covers it — so the exclusion is gone.)
Happy to re-pace these, drop any of them, or squash the set into a single PR if
you would rather review it in one pass — just say which.
Drafted with Claude Code; every site was read individually and the reasoning is
in the commit message.