refactor: remove V2Secp256k1 variant

[gilcu3]

Closes #2442

The curve will be removed from DomainConfig in a follow-up #3113

[claude]

PR title type suggestion: This PR removes a variant from the codebase, which is code restructuring/cleanup rather than adding new functionality. The type prefix should probably be refactor: or chore: instead of feat:.
Suggested title: refactor: remove V2Secp256k1 variant

[claude]

Code Review

CRITICAL: Missing deserialization compat for KeyshareData V2Secp256k1

File: crates/node/src/keyshare.rs

The V2Secp256k1 variant was removed from KeyshareData, which derives Serialize, Deserialize. Existing MPC nodes that have a DamgardEtAl (robust ECDSA) domain will have keyshares persisted in both permanent storage (JSON) and temporary storage with the serde tag V2Secp256k1. After upgrading to this binary, deserializing those keyshares will fail because the variant no longer exists.

This affects permanent key storage (PermanentKeyshareData loaded via serde_json from_slice) and temporary key storage (individual keyshare files).

Suggested fix: Add a serde(alias = V2Secp256k1) attribute to the Secp256k1 variant so existing persisted V2Secp256k1 data deserializes into Secp256k1. This works because both variants stored the same inner type (ecdsa KeygenOutput), making the alias a safe lossless migration. Without this, any node with a DamgardEtAl domain will fail to start after upgrade.

One merge-blocking issue: the missing serde deserialization compat for KeyshareData V2Secp256k1 will break all existing nodes with robust ECDSA domains on upgrade.

[gilcu3]

@claude review

please check again, we know no node in testnet or devnet has the V2Secp256k1 variant stored

[claude]

Code Review

Reviewed the full diff. The V2Secp256k1 removal is well-handled: contract Borsh migration uses OldCurve, JSON compat path is correct (V2Secp256k1 was never emitted in JSON), and the coordinator now routes keyshares by protocol from the domain registry rather than by the keyshare data discriminant.

One item worth noting (not blocking given the author's confirmation that no deployed node has V2Secp256k1 keyshares):

KeyshareData serde compat (acknowledged non-issue): The V2Secp256k1 variant is removed from KeyshareData (which uses serde). If any node did have persisted V2Secp256k1 keyshares, deserialization would fail. The author has confirmed this doesn't apply to any testnet/devnet node, so this is acceptable.

No critical issues found. The changes are consistent and well-structured:

• Contract Borsh migration properly handled via OldCurve in v3_9_1_state.rs
• JSON deserialization compat via DomainConfigCompat correctly updated with exhaustive match
• Coordinator keyshare routing now uses protocol from contract domain registry — more correct than inferring from keyshare data discriminant
• Halting on routing invariant violation (routing_invariant_violated → HaltUntilInterrupted) is a safe defensive measure
• All test updates are mechanical and consistent

✅ Approved

[kevindeforth]

compared it against #2442 (comment)

These refactors are pretty hard to follow and considering #2970 is still open, I just hope we don't miss anything.

[gilcu3]

compared it against #2442 (comment)

These refactors are pretty hard to follow and considering #2970 is still open, I just hope we don't miss anything.

Indeed I think I should tackle #2970 asap, I was thinking on it yesterday :) Thanks for the reminder!

[gilcu3]

@claude review (I want to see if we get better reviews now)

[kevindeforth]

Thanks!

[gilcu3]

@claude review (lets see how the new model compares to the old one)

[claude]

Pull request overview

This PR removes the V2Secp256k1 variant from Curve (both internal mpc_primitives::domain::Curve and the DTO surface) now that the Protocol enum carries the disambiguation between classic ECDSA (CaitSith) and robust ECDSA (DamgardEtAl) on the Secp256k1 curve. It also drops KeyshareData::V2Secp256k1, rewires coordinator/mpc_client routing to dispatch on Protocol instead of Curve, preserves Borsh state migration via a private OldCurve enum, and renames the devnet CLI flag --schemes → --protocols.

Changes:

• Drop Curve::V2Secp256k1 everywhere (DTO + primitives), plus the From<Curve> for Protocol migration helper that became ambiguous after the removal.
• Drop KeyshareData::V2Secp256k1; both classic and robust ECDSA keyshares now persist as KeyshareData::Secp256k1, with disambiguation done via the contract's DomainConfig.protocol.
• coordinator.rs: route keyshares to ECDSA / robust-ECDSA / EdDSA / CKD providers using a domain_id → protocol map sourced from the contract's running state, bailing on any (domain_id, keyshare_kind, protocol) inconsistency.
• mpc_client.rs: the runtime dispatch table is keyed on Protocol instead of Curve.
• key_events.rs: keygen and resharing branches keyed on domain.protocol.
• v3_9_1_state.rs: introduce a private OldCurve (with V2Secp256k1 index 3) so legacy Borsh state still decodes; the From<OldDomainConfig> impl maps V2Secp256k1 → (Secp256k1, DamgardEtAl).
• state.rs (DTO compat): explicit exhaustive match for the JSON protocol fallback now that From<Curve> for Protocol is gone.
• Add domains: Vec<DomainConfig> to ContractRunningState so the coordinator can read protocols.
• Remove is_valid_curve_for_purpose and InvalidCurvePurposeCombination; consolidate validation through validate_domain_consistency + is_valid_protocol_for_purpose.
• Devnet CLI: --schemes → --protocols; README updated; make_payload and parallel-sign call routing key on Protocol.

Reviewed changes

Per-file summary

File	Description
crates/primitives/src/domain.rs	Remove Curve::V2Secp256k1 and the now-ambiguous From<Curve> for Protocol impl + tests.
crates/contract/src/lib.rs	Switch payload-shape and test setup dispatch from Curve to Protocol; rename helper to make_public_key_for_curve; parametrize tests over both ECDSA protocols.
crates/contract/src/primitives/domain.rs	Drop is_valid_curve_for_purpose + V2Secp256k1 carve-out from validate_domain_consistency; tests updated.
crates/contract/src/primitives/test_utils.rs	ALL_CURVES/NUM_CURVES → ALL_PROTOCOLS/NUM_PROTOCOLS; generators iterate by protocol.
crates/contract/src/state/{running,initializing,resharing}.rs	Validate added domains via validate_domain_consistency; tests updated.
crates/contract/src/v3_9_1_state.rs	New private OldCurve enum used in OldDomainConfig; migration maps each old curve to (curve, protocol).
crates/contract/src/snapshots/...borsh_schema...snap, tests/snapshots/abi...snap	Snapshots regenerated to reflect the smaller Curve enum.
crates/contract/src/errors.rs	Drop InvalidCurvePurposeCombination.
crates/contract/tests/sandbox/**	Sandbox setup builders, constants, and tests switched to protocol-keyed APIs.
crates/near-mpc-contract-interface/src/types/state.rs	Explicit Curve → Protocol fallback in DomainConfigCompat.
crates/node/src/coordinator.rs	Keyshare routing uses domain_to_protocol from running state; bails on unknown domain or curve/keyshare mismatch.
crates/node/src/indexer/participants.rs	ContractRunningState gains a domains vector so coordinator can read protocols.
crates/node/src/key_events.rs	Keygen and resharing match on domain.protocol; DamgardEtAl now stores KeyshareData::Secp256k1.
crates/node/src/keyshare.rs	Drop KeyshareData::V2Secp256k1 variant.
crates/node/src/mpc_client.rs	domain_to_curve → domain_to_protocol; runtime dispatch on protocol.
crates/node/src/tests/**, crates/e2e-tests/tests/**	Test setups updated to use Protocol::* and to look up domains by protocol.
crates/devnet/{cli,contracts,loadtest,mpc}.rs, crates/devnet/README.md	CLI flag rename --schemes → --protocols; payload selection keyed on Protocol; README updated.

Findings

No blocking issues found. The serde-compat concern for persisted KeyshareData::V2Secp256k1 and the Borsh state migration were already discussed and addressed. The unresolved coordinator review thread is also addressed in the diff (anyhow::bail! is used, no panic).

Non-blocking (suggestions / follow-ups):

• crates/devnet/src/cli.rs:280 — Renaming the user-facing --schemes flag to --protocols is a breaking change for any operator script invoking mpc-devnet ... vote-add-domains. Devnet is internal so this is likely fine, but worth a heads-up to operators along with the rollout. The README update at crates/devnet/README.md:170 already covers the new usage.
• crates/node/src/coordinator.rs:567-579 — The keyshare-routing block bails on any inconsistency, but bailing returns Err, which the supervisor logs and then re-spawns the running job on the next contract state change (vs. HaltUntilInterrupted). For a true invariant violation (e.g. local DB has a Bls12381 keyshare under a Secp256k1 domain), continuous retries are noisy and won't recover. Consider returning Ok(MpcJobResult::HaltUntilInterrupted) for these specific mismatches so the node halts cleanly until intervention. Not blocking — the path should be unreachable in practice.
• crates/near-mpc-contract-interface/src/types/state.rs:122-138 — The DTO Curve no longer accepts V2Secp256k1 from JSON. If any external consumer (off-chain tool, monitoring script) cached old contract state view-method JSON containing "curve": "V2Secp256k1", it would now fail to deserialize. As noted by the author this never reached testnet/devnet, so this is just a documentation/release-notes nudge.
• crates/node/src/key_events.rs:241-256, crates/node/src/coordinator.rs:567 — Both CaitSith and DamgardEtAl now persist as KeyshareData::Secp256k1, so the on-disk keyshare bytes alone no longer self-describe their protocol. Correctness now depends on the contract's DomainConfig.protocol for that domain remaining consistent with what was stored. The contract validates this via validate_domain_consistency, so this is fine — just a coupling worth being aware of for future schema changes.

✅ Approved

[anodar]

Looks like docs/foreign-chain-transactions.md and docs/design/domain-separation.md still mention V2Secp256k1.

[gilcu3]

Looks like docs/foreign-chain-transactions.md and docs/design/domain-separation.md still mention V2Secp256k1.

for the latter it makes sense to keep it, for the former we need to check, but given that it is anyway not used there we can solve that later when that doc is updated after the latest design changes