fix(types): make malachitebft-signing-ed25519 a required dependency

[mehmetkr-31]

Fixes #236.

Summary

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
  --> crates/types/src/codec/proto.rs:28:5
  --> crates/types/src/signing.rs:21:9
  --> crates/types/src/ssz/v1/vote.rs:21:5

The signer-local feature gated dep:malachitebft-signing-ed25519, but those four modules import it unconditionally and feature = "signer-local" never appears in a #[cfg] anywhere in the workspace. The feature offered no real choice — with it the crate builds, without it the build breaks on unresolved imports.

The dependency was never really optional

signing.rs re-exports the dependency's types unconditionally:

pub use malachitebft_signing_ed25519::{Ed25519, PrivateKey, PublicKey, Signature};

So Ed25519, PrivateKey, PublicKey and Signature are part of this crate's public API regardless of the feature. A feature cannot meaningfully gate a dependency whose types the API surface hard-requires — the choice was never between an optional and a required dependency, only between declaring the truth and not. Raised by @osr21 on #236 and by @JspIIV on this PR, independently.

Change

Make the dependency required, and drop signer-local together with the now-empty default. No crate in the workspace requested the feature explicitly, so nothing needs updating alongside this.

Testing

• cargo check -p arc-consensus-types --no-default-features — now compiles (fails on main).
• cargo check --workspace --all-features — passes, no regression.
• cargo test -p arc-consensus-types — 192 tests pass.
• cargo fmt --all --check — clean.
• Cargo.lock unchanged.

Two things worth your call

Feature-name compatibility. Removing the name means an external consumer spelling features = ["signer-local"] would hit an unknown-feature error — though no such consumer can exist today, since the crate does not build without it. If you would rather keep the spelling working, signer-local = [] as a no-op alias gives the same fix; I am happy to switch. A third option is keeping the feature and adding a compile_error! guard the way arc-signer does, which turns the failure into a readable message but leaves a flag that can only ever be on.

Overlap with #231. That PR also edits the [features] table in crates/types/Cargo.toml (it declares the arbitrary feature's own dependencies, per #233). The two changes are independent and touch adjacent lines, so whichever lands second may need a trivial rebase — I will handle it, just let me know if you would prefer them combined into one PR instead.

[osr21]

Reviewed the diff against the verification I did on #236 — this is the right resolution and the mechanics are complete:

• The dependency edit is behavior-preserving for every existing consumer: previously all in-workspace consumers got the dep via default = ["signer-local"] (nobody used default-features = false on this crate); now they get it unconditionally. Same compiled graph, same ["rand", "serde"] features on the dep — which is why Cargo.lock is untouched, and that untouched lockfile is itself evidence the change only moves declarations, not resolution.
• Dropping the now-empty default rather than leaving default = [] is right — an empty default table is noise that invites someone to wonder what used to live there. Git history answers that better.

On the two calls you flagged:

1. Removal without the alias — as argued in more detail on #236, the no-op alias protects a consumer class that provably cannot exist. Any external crate spelling features = ["signer-local"] necessarily also compiled the four unconditional imports, which only ever built with the feature on — so no one depends on the off-state, and the on-state is now simply the crate's permanent state. The compile_error! route is worse than both alternatives: arc-signer earns its guard by having a genuine either/or between providers, while a guard here would formalize a flag with exactly one legal value. One additional data point for the record that settles it beyond the build argument: signing.rs re-exports the dep's types (pub use malachitebft_signing_ed25519::{Ed25519, PrivateKey, PublicKey, Signature}), so the "optional" dependency's types were unconditionally part of this crate's public API all along. A feature cannot meaningfully gate a dependency the crate's API surface hard-requires — removal is the only honest declaration.

2. Keep the PRs separate; land #231 first if there's a choice. The two changes fix opposite halves of the same defect class (#233: feature-on doesn't build standalone; #236: feature-off doesn't build at all), and separate PRs keep each one revertible against its own issue. The rebase collision is one adjacent-line merge either way. Mild preference for #231 landing first only because its diff adds to the [features] table this PR shrinks — rebasing an addition over a deletion is marginally less error-prone than the reverse, and both are trivial.

Worth noting the verification here quietly demonstrates the gap #233's proposed cargo-hack CI job would close: --no-default-features per crate is now load-bearing correctness (this PR is the proof) and nothing in CI exercises it. If that job gets filed, this crate is the test case for both directions.

Approving — minimal, correct, and the feature table now tells the truth.

[JspIIV]

Heads up in case it helps you get this unstuck — your change is still correct, it just conflicts now.

The only conflict is in the same [features] block: byzantine = ["dep:malachitebft-engine-byzantine"] was added after you opened this, so the block on main now reads

[features]
default = ["signer-local"]
arbitrary = ["dep:arbitrary"]
byzantine = ["dep:malachitebft-engine-byzantine"]
signer-local = ["dep:malachitebft-signing-ed25519"]

Keeping byzantine and dropping the other two lines resolves it. Your dependency line is unchanged.

Full disclosure: I opened #301 with the same fix before I noticed yours existed — I worked from #236 and missed the "Opened as #237" line. Yours is three weeks older and should be the one that lands; I have said so there and will close mine once this moves.

One thing worth adding to your description if you rebase: signing.rs re-exports Ed25519, PrivateKey, PublicKey and Signature unconditionally, so those types are already part of the crate's public API. That is the strongest argument that the dependency was never really optional.

[mehmetkr-31]

Rebased onto main after the v0.8.0 sync — head is now e68f4eb, conflict resolved exactly as you described: byzantine kept, the other two lines dropped, dependency line untouched.

Re-verified on the new base rather than assuming the old measurements carried over:

# main (v0.8.0)
$ cargo check -p arc-consensus-types --no-default-features --locked
error[E0432]: unresolved import `malachitebft_signing_ed25519`   (x4)

# this branch
$ cargo check -p arc-consensus-types --no-default-features --locked
Finished `dev` profile in 37.80s

So the bug survived the sync — v0.8.0 touched crates/types/Cargo.toml but only to add byzantine. cargo fmt --all --check clean.

Added your signing.rs point to the description. It is the stronger argument and worth stating plainly: Ed25519, PrivateKey, PublicKey and Signature are re-exported unconditionally, so the "optional" dependency's types have always been part of this crate's public API. A feature cannot meaningfully gate a dependency the API surface hard-requires. @osr21 reached the same conclusion independently on #236, which is decent corroboration.

Thanks for the heads-up and for deferring #301 — genuinely helpful, and it saved me finding the conflict on my own schedule. If this one stalls the way the rest of my open PRs have, do reopen yours rather than letting the fix sit; the fix landing matters more than which PR carries it.

[osr21]

Re-verified the rebase (e68f4eb) — the approval carries over, and the premise re-checks clean on the new base.

Confirmed on main at 66ad2d5:

• The diff is effect-identical to what I approved pre-sync: 1+/3− on crates/types/Cargo.toml only, byzantine kept, the dep made required with the same ["rand", "serde"] features, Cargo.lock untouched — so the lockfile evidence from the original review still stands (declarations move, resolution doesn't).
• The four lib-level unconditional imports are all still there post-sync (proposal_part.rs:26, codec/proto.rs:28, signing.rs:21, ssz/v1/vote.rs:21), and a repo-wide grep still finds zero cfg gating on signer-local. One count worth pinning down for the record: there is a fifth import in codec/proto.rs's #[cfg(test)] module (line 901) — cargo check reports 4 × E0432 because it compiles the lib only; --no-default-features --all-targets would surface 5. Doesn't change anything, but it explains any count mismatch a reviewer might trip over.
• One precision nit on "v0.8.0 touched crates/types/Cargo.toml only to add byzantine": from chore: sync v0.8.0 to arc-node #285's actual patch, the sync also added alloy-eips and the optional malachitebft-engine-byzantine line in [dependencies]. Neither is near your edits, so the premise is intact — just keeping the record exact.

On the #231 overlap — upgrade "may need a trivial rebase" to "will": I merge-tested the pairing on current main in both orders today (details with the conflict hunk on #231). Both directions conflict, because #231 edits the arbitrary line directly below the default = ["signer-local"] line this PR deletes — adjacent edits collapse into one hunk. The resolution is the obvious keep-both, and the changes remain semantically independent; it just won't compose silently in either order, so whoever merges second-in-line should expect the red mergeability flag.

With #301 deferred (on record over there — and credit to @JspIIV for the clean handling), this is the landing vehicle for #236. Nothing further from me: rebased, re-measured, still approved.