Skip to content

fix(types): drop the signer-local feature gating a mandatory dependency - #301

Closed
JspIIV wants to merge 1 commit into
circlefin:mainfrom
JspIIV:fix/consensus-types-no-default-features
Closed

fix(types): drop the signer-local feature gating a mandatory dependency#301
JspIIV wants to merge 1 commit into
circlefin:mainfrom
JspIIV:fix/consensus-types-no-default-features

Conversation

@JspIIV

@JspIIV JspIIV commented Sep 1, 2026

Copy link
Copy Markdown

Closes #236.

What

arc-consensus-types does not build with --no-default-features:

$ cargo check -p arc-consensus-types --no-default-features
error[E0432]: unresolved import `malachitebft_signing_ed25519`
  --> crates/types/src/proposal_part.rs:26:5
error[E0432]: unresolved import `malachitebft_signing_ed25519`
  --> crates/types/src/codec/proto.rs:28:5
error[E0432]: unresolved import `malachitebft_signing_ed25519`
  --> crates/types/src/signing.rs:21:9
error[E0432]: unresolved import `malachitebft_signing_ed25519`
  --> crates/types/src/ssz/v1/vote.rs:21:5

signer-local marks malachitebft-signing-ed25519 optional, but those four modules import it unconditionally, so the crate cannot compile without it.

The feature is not optionality

It gates a dependency that is mandatory in practice, and nothing selects it:

  • no crate in the workspace names arc-consensus-types/signer-local — the only signer-local matches elsewhere are alloy-signer-local, an unrelated crate
  • there is no cfg(feature = "signer-local") anywhere in the tree

So the feature has exactly one effect: it makes --no-default-features fail. Making the dependency mandatory removes that without changing what any consumer gets, since default turned it on for everyone already.

Checks

command before after
cargo check -p arc-consensus-types --no-default-features fails, 4 × E0432 passes
cargo check -p arc-consensus-types passes passes
cargo check -p arc-consensus-db passes passes
cargo check -p arc-signer passes passes
cargo check -p arc-remote-signer passes passes
cargo check -p arc-node-consensus-cli passes passes

Two things I could not exercise, both failing identically on an untouched tree here:

arc-consensus-types does not build with --no-default-features:

    error[E0432]: unresolved import `malachitebft_signing_ed25519`
      --> crates/types/src/proposal_part.rs:26:5
      --> crates/types/src/codec/proto.rs:28:5
      --> crates/types/src/signing.rs:21:9
      --> crates/types/src/ssz/v1/vote.rs:21:5

signer-local marks malachitebft-signing-ed25519 optional, but those four
modules import it unconditionally, so the crate cannot compile without it.
The feature offers no optionality: nothing in the workspace names
arc-consensus-types/signer-local, and there is no cfg(feature = "signer-local")
anywhere in the tree.

Make the dependency mandatory, which is what it already is, and remove the
feature along with the default that enabled it.
@osr21

osr21 commented Sep 1, 2026

Copy link
Copy Markdown

Independently verified this at head d0427ff — the change is correct, and I can add a few data points to the record (I ran the same verification on #236 back on Aug 8):

  • The four unconditional imports check out, and there is a fifth usage the PR body doesn't count: a test-module import in codec/proto.rs (~line 900) — same story, no #[cfg]. More importantly, signing.rs does a pub use re-export (Ed25519, PrivateKey, PublicKey, Signature), so the "optional" dependency's types have been an unconditional part of this crate's public API all along. That settles the design question in favor of a required dependency: a feature can't meaningfully gate a dep whose types the crate re-exports unconditionally.
  • No workspace consumer names signer-local, none sets default-features = false on arc-consensus-types, and there is no cfg(feature = "signer-local") anywhere in the tree — all confirmed by fresh grep at current main. The org-wide signer-local code-search hits are all alloy-signer-local, matching the PR's claim.
  • On the semver question bug: arc-consensus-types does not build with --no-default-features; signer-local gates a mandatory dependency #236 raised (external consumers spelling features = ["signer-local"]): the workspace sets publish = false, the crate is 0.0.1 and unpublished, and a GitHub-wide Cargo.toml search finds no external consumers — so dropping the feature name outright is safe; a no-op alias would preserve a spelling nobody uses.

One thing the PR body should acknowledge: this duplicates #237, the open PR by the #236 reporter — the issue body itself says "Opened as #237". The material difference today is that #237 has gone stale into a merge conflict (mergeable: false — it predates the byzantine feature being added to this same [features] block), while this PR applies cleanly to current main and adds the per-crate check table. How to resolve that is the maintainers' and authors' call — #237's conflict is a one-line rebase away, so the fair options are pinging #237 for a rebase or landing this one with credit to #236/#237. What shouldn't happen is the two sitting as silent parallel duplicates.

Context on why this survived unnoticed: no CI workflow exercises --no-default-features for any crate (no cargo-hack / feature-matrix step), so nothing guards this class of regression — worth keeping in mind whichever PR lands.

@JspIIV

JspIIV commented Sep 1, 2026

Copy link
Copy Markdown
Author

Thanks for the verification, and for catching the duplication — that one is on me. I worked from #236 and missed the "Opened as #237" line in the issue body, so I did not see that @mehmetkr-31 had already published the same change on Aug 8, three weeks before this.

#237 is the same fix in the same file. It came first and it should get the credit.

The only difference is that it now conflicts, and only because the byzantine feature was added to that same [features] block afterwards. That is a one-line rebase, so my preference is that #237 is rebased and landed and this is closed. I will close it myself as soon as #237 moves, or immediately if a maintainer would rather not have two open.

If it is useful in the meantime, the two extra findings from your comment are worth carrying over to #237 whichever way it goes:

Per-crate check results from this branch, in case they save anyone a rerun on #237: arc-consensus-types passes with --no-default-features and with defaults, and arc-consensus-db, arc-signer, arc-remote-signer and arc-node-consensus-cli all still build. --all-features still fails on arbitrary, which is #233 and untouched by either PR.

@osr21

osr21 commented Sep 1, 2026

Copy link
Copy Markdown

Clean handling — and events have already met your closing condition: #237 was rebased about nine minutes after this comment (e68f4eb), re-verified over there, and its approval carries over. It merges cleanly onto current main, so nothing is waiting on it but maintainer attention. Closing this one now loses nothing.

One measurement that makes your "saves anyone a rerun" offer exact rather than approximate: I diffed the two heads directly — git diff between e68f4eb and d0427ff is empty across the entire tree. Post-rebase, the two PRs don't just make the same change; they produce byte-identical results. Every per-crate check result from this branch therefore transfers to #237 verbatim, including yours above.

On the residual --all-features failure: right that it's #233 and untouched by either PR — and for the record, its fix is also open, rebased today, and approved (#231, the arbitrary feature declarations). Together #237 + #231 are what make arc-consensus-types build standalone across the feature matrix. One sequencing note measured today rather than assumed: #231 and #237 conflict in both orders — #231 edits the line directly below the default = ["signer-local"] line #237 deletes, so the adjacent edits collapse into one hunk (conflict markers posted on #231). Trivial keep-both resolution, but whichever lands second goes red on mergeability first.

So the full map for maintainers is now: #237 lands #236's fix, #231 lands #233's, one trivial rebase between them in either order, and #240 is the guard that keeps the class closed. This PR served its purpose — it forced the rebase check that got #237 unstuck.

@JspIIV

JspIIV commented Sep 1, 2026

Copy link
Copy Markdown
Author

Closing as promised — #237 has been rebased and is mergeable, so this has nothing left to add.

Thanks @mehmetkr-31 for the quick rebase, and @osr21 for the diff check confirming the two heads are identical. #237 is the one to land: it was opened three weeks earlier, it carries its approval, and it is the reporter's own fix.

@JspIIV JspIIV closed this Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: arc-consensus-types does not build with --no-default-features; signer-local gates a mandatory dependency

2 participants