feat: Enforcing strong typing and one explicit threshold: ReconstructionThr…

[SimonRastikian]

Closes near/threshold-signatures#348
Our long lived issue :)

[claude]

PR title type suggestion: This PR restructures how thresholds are typed and enforces type safety across the codebase. Since the primary intent is improving code structure rather than adding new user-facing functionality, the type prefix should probably be refactor: instead of feat:.

Suggested title: refactor: Enforce strong typing with explicit ReconstructionThreshold type

[claude]

Pull request overview

Renames the single threshold abstraction (ReconstructionLowerBound → ReconstructionThreshold) and removes the MaxMalicious parameter from public APIs, so callers (including the node's robust-ECDSA leader/follower paths) speak in ReconstructionThreshold instead of doing ad-hoc translation. Adds ReconstructionThreshold::max_malicious() / MaxMalicious::reconstruction_threshold() to bridge inside the crypto crate, and introduces mpc_primitives::ReconstructionThreshold for the node's wire-level threshold field (with #[serde(transparent)] to keep JSON compatibility with the prior u64). Deletes the old translate_threshold / get_number_of_signers / compute_thresholds helpers and their tests in the node's robust-ecdsa provider.

Changes:

• crates/threshold-signatures/src/thresholds.rs: renames the public threshold type and adds bidirectional conversions between MaxMalicious and ReconstructionThreshold.
• crates/threshold-signatures/...: replaces every public-facing MaxMalicious parameter in keygen/presign/sign/triples APIs with ReconstructionThreshold.
• crates/node/src/config.rs: changes ParticipantsConfig::threshold from u64 to mpc_primitives::ReconstructionThreshold and adds ts_threshold() helper.
• crates/node/src/providers/robust_ecdsa/*.rs: removes the translate_threshold/get_number_of_signers/compute_thresholds helpers and rewires the leader's background-presign and sign code paths to pass ReconstructionThreshold directly to the crypto crate.
• Benches and tests updated to the new names/types.

Reviewed changes

Per-file summary

File	Description
crates/threshold-signatures/src/thresholds.rs	Renames ReconstructionLowerBound → ReconstructionThreshold; adds max_malicious() and reconstruction_threshold() conversions.
crates/threshold-signatures/src/{lib.rs,dkg.rs,frost*,ecdsa/**}	Propagates the rename through every public signature; switches PresignArguments::max_malicious to threshold: ReconstructionThreshold.
crates/threshold-signatures/src/ecdsa/robust_ecdsa/{presign,sign}.rs	Converts inputs via threshold.max_malicious() and keeps the existing 2*(t-1)+1 == participants.len() invariant check.
crates/node/src/config.rs	Adds mpc_primitives::ReconstructionThreshold field + ts_threshold() helper that produces the threshold-signatures variant.
crates/node/src/providers/robust_ecdsa/{,presign,sign}.rs	Removes the translate_threshold / compute_thresholds helpers + their tests; replaces every previous threshold translation with a direct ts_threshold() call.
crates/node/src/providers/{ecdsa,eddsa,ckd}/...	Pure rename — same semantics, no behavioural change.
crates/threshold-signatures/benches/**	Renames threshold types and updates a few sites that previously consumed MaxMalicious.

Findings

Blocking (must fix before merge):

• crates/threshold-signatures/src/thresholds.rs:46-48 — ReconstructionThreshold::max_malicious() does MaxMalicious(self.0 - 1). The doc comment claims "constructors only produce values ≥ 1", but that is not true: the derive_more::From impl auto-derives From<usize> without validation, and #[derive(Deserialize)] happily accepts 0. On a release build with overflow checks disabled (the default release/release-contract profile in this repo), 0_usize - 1 silently wraps to usize::MAX and the resulting MaxMalicious(usize::MAX) is the value handed to the protocol — debug builds panic instead. The downstream presign() only rejects this accidentally (max_malicious.value() > participants.len()); a future caller that consumes MaxMalicious without that bound check would be wide open to UB-adjacent behaviour. Either return Result<MaxMalicious, ThresholdError> via checked_sub, or remove the auto-derived From<usize>/custom-Deserialize and validate > 0 in a hand-written constructor. The function's "Panics if 0" sentence should not be left as the only guard.

• crates/node/src/providers/robust_ecdsa/presign.rs:89-98 and crates/threshold-signatures/src/ecdsa/robust_ecdsa/presign.rs:55-82 — the leader picks num_signers = threshold.value() and selects exactly that many participants, but presign() now insists participants.len() == 2 * (threshold - 1) + 1. With the contract's MIN_THRESHOLD_ABSOLUTE = 2 and 60% rule (crates/contract/src/primitives/thresholds.rs:48-58), e.g. n = 5, t = 3 works (2·2+1 = 5), but n = 7, t = 5 fails (2·4+1 = 9 ≠ 5), and the old translate_threshold accepted exactly this case by computing max_malicious = (t-1)/2 = 2. After this PR the leader will hit the BadParameters("the number of participants during presigning must be exactly 2*(ReconstructionThreshold - 1)+1 ...") error and never produce a presignature for any configuration where the contract threshold isn't (n+1)/2. Either:

  • the leader needs to pick num_signers = 2 * (threshold - 1) + 1 (and validate it ≤ running_participants.len()), or
  • the unification should keep a per-scheme adapter for robust-ECDSA, or
  • explicitly document that robust-ECDSA now requires t = (n+1)/2 and add a startup check + e2e test that exercises a representative non-trivial robust-ECDSA setup.

  Whichever route is chosen, please add a focused test that constructs a robust-ECDSA PresignArguments with the threshold the contract would actually supply (e.g. t = 5, n = 7) and asserts the leader still produces a valid presignature — the deleted test_translate_threshold_* cases used to cover this region and are not replaced.

• crates/node/src/providers/robust_ecdsa/presign.rs:89-98 — the new .expect("governance threshold does not fit in usize") plus assert!(num_signers <= running_participants.len(), …) panics inside a -> ! background task. The previous code panicked too, but it panicked with a robust-ECDSA-specific invariant failure (compute_thresholds enforced 2·max_malicious+1 <= num_signers and t ≥ 5); now the only guard left here is the participant-count comparison, and the robust-ECDSA-specific constraints (t ≥ 2, ≥5 participants) are silently dropped. Either keep the explicit validation (preferred — it gives a much clearer crash if an operator misconfigures the contract for this scheme), or replace the assert! with tracing::error! + return so the supervisor can recover.

• crates/threshold-signatures/src/thresholds.rs:46, crates/threshold-signatures/src/thresholds.rs:29 — engineering-standards.md "Add tests" — the new max_malicious() and reconstruction_threshold() methods have no unit tests at all (the file no longer contains a #[cfg(test)] mod tests). At minimum, please cover (a) round-trip ReconstructionThreshold::from(n).max_malicious().reconstruction_threshold() == ReconstructionThreshold::from(n) for n ≥ 1, (b) MaxMalicious::from(usize::MAX).reconstruction_threshold() returns IntegerOverflow, and (c) the t = 0 edge case for max_malicious() (which today silently underflows in release).

Non-blocking (nits, follow-ups, suggestions):

• crates/node/src/config.rs:77-83 — ts_threshold() returns anyhow::Result solely to surface a usize::try_from(u64) failure. On the supported 64-bit targets this conversion can't fail, so every call site is forced to add ? for a dead error branch. Consider returning threshold_signatures::ReconstructionThreshold directly (with a debug-only assertion) so the API surface honours "Maintain local reasonability".
• crates/threshold-signatures/src/thresholds.rs:28 — #[allow(dead_code)] // used only by test code under cfg(test) on a pub method is misleading: if the method is genuinely only used by cfg(test) callers, gate it on cfg(test) (or cfg(any(test, feature = "test-utils"))) instead of allow(dead_code); otherwise drop the comment, since the method is part of the public surface.
• crates/threshold-signatures/src/ecdsa/robust_ecdsa/sign.rs:32-35, crates/threshold-signatures/src/ecdsa/robust_ecdsa/presign.rs:58,73,80 — several error strings now read "2*(ReconstructionThreshold - 1)+1 must be …", which leaks an implementation detail (the internal max-malicious derivation) into messages an operator will see. Reword in operator-facing terms, e.g. "signing requires exactly 2·(t-1)+1 participants for the configured reconstruction threshold t".
• crates/node/src/key_events.rs:248, crates/node/src/p2p.rs:1029 — mixing mpc_primitives::ReconstructionThreshold::new(...) and threshold_signatures::ReconstructionThreshold::from(...) in adjacent code is easy to misuse. Consider a single use alias or a node-side helper to keep callers from importing the wrong one.

⚠️ Issues found

[claude]

PR title type suggestion: Based on the changed files, this appears to be a type system refactoring rather than a new feature. The title mentions 'enforcing strong typing,' which is typically a refactoring concern. Consider using refactor: instead of feat:.

[netrome]

Nice stuff, though it seems like there are some ambiguous thresholds remaining. Gotta catch em all - or what are your thoughts here?

[netrome]

Hmm, this type has not been renamed. It feels weird to just cast the reconstruction threshold to this other type here. Shouldn't this also be the ReconstructionThreshold so we don't have any ambiguous "threshold" concepts remaining? Long term I guess this should become the governance threshold, but if this is the case, we should already have the governance threshold in participants_config imo where we now have ReconstructionThreshold.

So I suggest one of two things:

1. Use ReconstructionThreshold everywhere.
2. Rename the existing primitiveThreshold type used in the contract for voting operations to GovernanceThreshold and separate the governance and reconstruction thresholds.

The second option seems like it deserves its own issue, so I'd lean on 1. here.

[netrome]

Isn't this the governance threshold now? Do we want to have a separate notion between governance and reconstruction threshold? Otherwise, could we get rid of the existing Threshold type in our primitives crate favor of just having the ReconstructionThreshold everywhere?