Simon/code refactor #22

SimonRastikian · 2025-08-01T10:12:15Z

This part concerns code refactoring from PR #15.
Closes #6

…nts)

… protocol folder

Copilot

Pull Request Overview

This PR implements a major code refactoring as described in the title, addressing issue #6. The refactoring consolidates cryptographic functionality, removes deprecated modules, and restructures the codebase around FROST-based implementations while eliminating the legacy cait-sith specific curve trait.

Key changes include:

Removal of deprecated modules (serde.rs, proof modules, triple generation)
Consolidation of cryptographic functionality under crypto/ module structure
Migration from custom CSCurve trait to FROST library types
Restructuring of ECDSA implementation to use FROST primitives

Reviewed Changes

Copilot reviewed 47 out of 51 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/serde.rs	Complete removal of serialization utilities module
src/proofs/dlogeq.rs	Removal of discrete log equality proof implementation
src/proofs/dlog.rs	Removal of discrete log proof implementation
src/participants.rs	Updated to use FROST types and consolidated Lagrange coefficient computation
src/lib.rs	Simplified module structure removing deprecated components
src/generic_dkg.rs	Refactored to use new crypto module structure and FROST polynomials
src/eddsa/test.rs	Updated import paths for hash utilities
src/eddsa/sign.rs	Updated import paths and method names for FROST compatibility
src/ecdsa/test.rs	Simplified test utilities and updated type definitions
src/ecdsa/mod.rs	Major restructuring to define ECDSA types using FROST primitives

src/participants.rs

src/generic_dkg.rs

src/crypto/README.md

src/crypto/proofs/dlog.rs

src/crypto/proofs/dlogeq.rs

src/ecdsa/ot_based_ecdsa/triples/mta.rs

src/generic_dkg.rs

src/protocol/internal.rs

src/ecdsa/ot_based_ecdsa/triples/generation.rs

kevindeforth

Partial review. Some things that I noticed:

some of the protocols have no reference pseudocode (e.g. ecdsa/ot_based_ecdsa/triples/mta)
quite a bit of dead code.

src/protocol/mod.rs

src/participants.rs

kevindeforth · 2025-08-04T12:30:50Z

src/ecdsa/ot_based_ecdsa/triples/batch_random_ot.rs

 }

 /// Run the batch random OT protocol between two parties.
 #[allow(dead_code)]


is this code still used? Why don't we delete it?

Used only in unit test, moved there.

This created extra "deadcode".
This dead code is not fully dead as:

it is tested

it is used in the MPC repo

Should be 100% used when benchmarking the implementation.
I push this problem to be solved in [Task] Benchmark implementation #8 and Fix remaining clippy issues and Erase #allow() #12

For the moment I kept allow deadcode for clippy

How is this used in the MPC repo if the function is private to the crate?

@netrome not sure what you're asking here. But if it is about run_batch_random_ot function then this has been moved to test module. This code version is outdated see https://github.com/near/threshold-signatures/pull/22/files#diff-828c7600c9ca003925b4117ddedc488adc75289ad104297547edefabda01be56R352-R378

src/ecdsa/ot_based_ecdsa/triples/mta.rs

src/ecdsa/ot_based_ecdsa/triples/mod.rs

src/ecdsa/ot_based_ecdsa/triples/test.rs

src/participants.rs

src/crypto/polynomials.rs

src/crypto/README.md

src/crypto/commit.rs

src/crypto/random.rs

src/crypto/hash.rs

src/crypto/commit.rs

src/protocol/internal.rs

…an error

kevindeforth

Thank you @SimonRastikian for all the hard work!

This is a solid improvement to the code base!

I approve, conditional on:

The minor comments (especially the ones related to panics) from this review to be addressed;
the remaining unresolved conversations to be followed-up with fixes, or by opening issues for them;
@netrome approving to merge.

Disclaimer: I did not properly review the cryptographic protocols. I assumed those were re-factor, not changed. So please lmk if you want me to dive into those as well.

Breaking Changes with MPC node (@netrome):
There are a few breaking changes with respect to the node. I did not investigate in detail. Seems like most of them are imports that changed, and maybe a few visibility issues. But there is a risk of surprises.

```bash error[E0432]: unresolved import `threshold_signatures::ecdsa::sign` --> node/src/indexer/types.rs:16:34 | 16 | use threshold_signatures::ecdsa::sign::FullSignature; | ^^^^ could not find `sign` in `ecdsa`

error[E0432]: unresolved import threshold_signatures::ecdsa::triples
--> node/src/primitives.rs:8:34
|
8 | use threshold_signatures::ecdsa::triples::TripleGenerationOutput;
| ^^^^^^^ could not find triples in ecdsa

error[E0432]: unresolved import threshold_signatures::ecdsa::presign
--> node/src/providers/ecdsa/presign.rs:21:34
|
21 | use threshold_signatures::ecdsa::presign::{presign, PresignArguments, PresignOutput};
| ^^^^^^^ could not find presign in ecdsa

error[E0432]: unresolved import threshold_signatures::ecdsa::triples
--> node/src/providers/ecdsa/presign.rs:22:34
|
22 | use threshold_signatures::ecdsa::triples::TripleGenerationOutput;
| ^^^^^^^ could not find triples in ecdsa

error[E0432]: unresolved import threshold_signatures::ecdsa::presign
--> node/src/providers/ecdsa/sign.rs:17:34
|
17 | use threshold_signatures::ecdsa::presign::PresignOutput;
| ^^^^^^^ could not find presign in ecdsa

error[E0432]: unresolved import threshold_signatures::ecdsa::sign
--> node/src/providers/ecdsa/sign.rs:18:34
|
18 | use threshold_signatures::ecdsa::sign::FullSignature;
| ^^^^ could not find sign in ecdsa

error[E0432]: unresolved import threshold_signatures::ecdsa::triples
--> node/src/providers/ecdsa/triple.rs:18:34
|
18 | use threshold_signatures::ecdsa::triples::TripleGenerationOutput;
| ^^^^^^^ could not find triples in ecdsa

error[E0432]: unresolved import threshold_signatures::ecdsa::sign
--> node/src/providers/ecdsa/mod.rs:28:34
|
28 | use threshold_signatures::ecdsa::sign::FullSignature;
| ^^^^ could not find sign in ecdsa

error[E0433]: failed to resolve: could not find sign in ecdsa
--> node/src/providers/ecdsa/sign.rs:152:53
|
152 | let protocol = threshold_signatures::ecdsa::sign::sign(
| ^^^^ could not find sign in ecdsa
|
help: consider importing one of these modules
|
1 + use threshold_signatures::ecdsa::ot_based_ecdsa::sign;
|
1 + use threshold_signatures::eddsa::sign;
|
help: if you import sign, refer to it directly
|
152 - let protocol = threshold_signatures::ecdsa::sign::sign(
152 + let protocol = sign::sign(
|

error[E0433]: failed to resolve: could not find triples in ecdsa
--> node/src/providers/ecdsa/triple.rs:225:53
|
225 | let protocol = threshold_signatures::ecdsa::triples::generate_triple_many::<Secp256k1, N>(
| ^^^^^^^ could not find triples in ecdsa
|
help: consider importing this module
|
1 + use threshold_signatures::ecdsa::ot_based_ecdsa::triples;
|
help: if you import triples, refer to it directly
|
225 - let protocol = threshold_signatures::ecdsa::triples::generate_triple_many::<Secp256k1, N>(
225 + let protocol = triples::generate_triple_many::<Secp256k1, N>(
|

error[E0599]: no method named num_owned_ready found for struct Arc<TripleStorage> in the current scope
--> node/src/providers/ecdsa/triple.rs:80:68
|
80 | metrics::MPC_OWNED_NUM_TRIPLES_ONLINE.set(triple_store.num_owned_ready() as i64);
| ^^^^^^^^^^^^^^^ method not found in Arc<TripleStorage>

error[E0599]: no method named num_owned_offline found for struct Arc<TripleStorage> in the current scope
--> node/src/providers/ecdsa/triple.rs:82:35
|
82 | .set(triple_store.num_owned_offline() as i64);
| ^^^^^^^^^^^^^^^^^ method not found in Arc<TripleStorage>

error[E0599]: no method named num_owned found for struct Arc<TripleStorage> in the current scope
--> node/src/providers/ecdsa/triple.rs:83:49
|
83 | let my_triples_count = triple_store.num_owned();
| ^^^^^^^^^
|
help: there is a method to_owned with a similar name
|
83 - let my_triples_count = triple_store.num_owned();
83 + let my_triples_count = triple_store.to_owned();
|

error[E0599]: no method named generate_and_reserve_id_range found for struct Arc<TripleStorage> in the current scope
--> node/src/providers/ecdsa/triple.rs:111:22
|
110 | let id_start = triple_store
| ___________-
111 | | .generate_and_reserve_id_range(SUPPORTED_TRIPLE_GENERATION_BATCH_SIZE as u32);
| | -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ method not found in Arc<TripleStorage>
| ||
|

error[E0599]: no method named add_owned found for struct Arc<TripleStorage> in the current scope
--> node/src/providers/ecdsa/triple.rs:137:38
|
137 | triple_store.add_owned(id_start.add_to_counter(i as u32)?, paired_triple);
| ^^^^^^^^^
|
help: there is a method to_owned with a similar name, but with different arguments
--> /home/kevin/.rustup/toolchains/1.86.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/borrow.rs:58:5
|
58 | fn to_owned(&self) -> Self::Owned;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0599]: no method named maybe_discard_owned found for struct Arc<TripleStorage> in the current scope
--> node/src/providers/ecdsa/triple.rs:155:30
|
155 | triple_store.maybe_discard_owned(32).await;
| ^^^^^^^^^^^^^^^^^^^ method not found in Arc<TripleStorage>

error[E0599]: no method named take_owned found for struct Arc<TripleStorage> in the current scope
--> node/src/providers/ecdsa/presign.rs:106:75
|
106 | let (paired_triple_id, (triple0, triple1)) = triple_store.take_owned().await;
| ^^^^^^^^^^
|
help: there is a method to_owned with a similar name
|
106 - let (paired_triple_id, (triple0, triple1)) = triple_store.take_owned().await;
106 + let (paired_triple_id, (triple0, triple1)) = triple_store.to_owned().await;
|

error[E0599]: no method named take_unowned found for struct Arc<TripleStorage> in the current scope
--> node/src/providers/ecdsa/presign.rs:257:52
|
257 | let (triple0, triple1) = self.triple_store.take_unowned(self.paired_triple_id)?;
| ^^^^^^^^^^^^
|
help: there is a method to_owned with a similar name, but with different arguments
--> /home/kevin/.rustup/toolchains/1.86.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/borrow.rs:58:5
|
58 | fn to_owned(&self) -> Self::Owned;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0599]: no method named add_unowned found for struct Arc<TripleStorage> in the current scope
--> node/src/providers/ecdsa/triple.rs:269:35
|
269 | self.out_triple_store.add_unowned(
| ----------------------^^^^^^^^^^^ method not found in Arc<TripleStorage>

Some errors have detailed explanations: E0432, E0433, E0599.
For more information about an error, try rustc --explain E0432.

</details>

kevindeforth · 2025-08-18T07:53:11Z

src/crypto/commitment.rs

+}
+
+#[cfg(test)]
+mod test {


Follow-up: Add checks against known expected values to catch future regressions.

kevindeforth · 2025-08-18T08:12:23Z

src/crypto/hash.rs

+#[cfg(test)]
+pub(crate) use test::scalar_hash;
+
+use crate::protocol::ProtocolError;


lets move the import to the top of the file

kevindeforth · 2025-08-18T08:28:57Z

src/crypto/polynomials.rs

+    if let Some(x) = x {
+        for x_j in points_set.iter() {
+            if *x_i == *x_j {
+                contains_i = true;


Here and on line 404, we can do an additional check: if contains_i is already true, then return an error, as the input requirements are not met.

src/crypto/polynomials.rs

src/protocol/internal.rs

src/participants.rs

kevindeforth · 2025-08-18T10:38:39Z

src/participants.rs

The existing code has a few panics and the style could be improved for readability.
Something like the following:

pub fn put(&mut self, participant: Participant, data: T) { let i = self.participants.indices.get(&participant); if i.is_none() { return; } let i = *i.unwrap(); if self.data[i].is_some() { return; } self.data[i] = Some(data); self.count += 1; }

could be simplified to

pub fn put(&mut self, participant: Participant, data: T) { if let Some(&i) = self.participants.indices.get(&participant) { if self.data[i].is_none() { self.data[i] = Some(data); self.count += 1; } } }

It might also be worth to investigate if we could simplify a few things by relying on standard library structs, but I haven't looked into that enough to know if that's the case.

src/ecdsa/ot_based_ecdsa/triples/test.rs

netrome · 2025-08-18T13:33:19Z

Breaking Changes with MPC node (@netrome):
There are a few breaking changes with respect to the node. I did not investigate in detail. Seems like most of them are imports that changed, and maybe a few visibility issues. But there is a risk of surprises.

I believe these are tackled in near/mpc#867

kevindeforth · 2025-08-18T13:35:28Z

Breaking Changes with MPC node (@netrome):
There are a few breaking changes with respect to the node. I did not investigate in detail. Seems like most of them are imports that changed, and maybe a few visibility issues. But there is a risk of surprises.

I believe these are tackled in near/mpc#867

Ah, thanks for signaling I was just getting started on that! 😄

netrome

Skimmed this now. I created a follow-up for the missing lagrange coefficient tests. Two panics spotted by @kevindeforth remain to be addressed. Please fix these @SimonRastikian and ping me once that's done and I'll approve so we can get this merged.

netrome · 2025-08-18T17:34:22Z

src/crypto/polynomials.rs

+/// lamda_i(x) = \prod_j (x - x_j)/(x_i - x_j)
+/// where j != i
+/// If x is None then consider it as 0
+pub fn compute_lagrange_coefficient<C: Ciphersuite>(


In the interest of maintaining our sanity, let's tackle these as a follow-up. Created #40

src/participants.rs

src/protocol/internal.rs

SimonRastikian · 2025-08-19T07:48:09Z

@netrome I addressed the two comments of yours.
Although I do not find it very clean to have a sending channel fail, I am unsure for the moment what is the best approach to deal with this.

netrome

Thank you for fixing the last panics. Let's get this merged!

netrome · 2025-08-19T08:50:09Z

Although I do not find it very clean to have a sending channel fail, I am unsure for the moment what is the best approach to deal with this.

Can you elaborate? Is this due to the fallible serialization being used in the send functions, and if so - would it be possible to formulate these in a non-fallible way?

SimonRastikian · 2025-08-19T08:57:18Z

Yes it is due to the fallible serialization. I currently do not know if there is a way to do this without failing, but understanding the communication layer is to be done. See task #39.

Simon Rastikian added 9 commits August 1, 2025 11:47

Cryptography tools and utilities

6de4ca6

Removed files non-necessary

138f834

Unused file (replaced by crypto folder)

ce573b7

refactor ot-based ecdsa (changing types and reorganizing file placeme…

2cebf76

…nts)

Linking files

48a2d74

reworking generic dkg, linking files and moving reliable_broadcast to…

63b14d8

… protocol folder

Adjusting libraries

946ac5f

No robust_ecdsa crate

3b70c94

Adjusting EdDSA

b4fda96

SimonRastikian requested review from gilcu3, kevindeforth, kuksag and netrome August 1, 2025 10:12

SimonRastikian self-assigned this Aug 1, 2025

SimonRastikian mentioned this pull request Aug 1, 2025

Simon/passive robust ecdsa #15

Closed

pbeza requested a review from Copilot August 1, 2025 10:38

Copilot AI reviewed Aug 1, 2025

View reviewed changes

src/participants.rs Outdated Show resolved Hide resolved

src/participants.rs Outdated Show resolved Hide resolved

src/generic_dkg.rs Outdated Show resolved Hide resolved

gilcu3 requested changes Aug 4, 2025

View reviewed changes

kevindeforth reviewed Aug 4, 2025

View reviewed changes

kevindeforth reviewed Aug 5, 2025

View reviewed changes

src/crypto/commit.rs Outdated Show resolved Hide resolved

kevindeforth reviewed Aug 5, 2025

View reviewed changes

src/protocol/internal.rs Show resolved Hide resolved

SimonRastikian and others added 7 commits August 5, 2025 22:09

Copilot suggestions about collect

2502320

Removing this file for the moment, to be added in another PR

dd666c6

Making Strings Constant

3fcfe5f

Moved Deadcode to where it belongs

73c7413

Restructuring a test that should be in unit test

cd84aa2

Allowing dead code and running cargo fmt

5744ad5

Improve readability Secp256 instead of C

cb149c7

SimonRastikian added 6 commits August 15, 2025 16:35

No panic in hash

cf1d40e

Unreachable is now Error

07ce057

Now all functions in poly check if the polynomial if empty and raise …

290af98

…an error

test_extend_with_identity

0e0e1d8

ProtocolError instead of panic in dlog.rs and dlogeq.rs

6520f60

Test generate polynomial degree and poitn

8f62487

gilcu3 mentioned this pull request Aug 18, 2025

Integrate threshold-signature refactor in MPC node near/mpc#858

Closed

Making some frost libraries public so that the mpc repo can use them.

5fa4caf

SimonRastikian force-pushed the simon/code-refactor branch from 6aab5d9 to 5fa4caf Compare August 18, 2025 11:33

gilcu3 mentioned this pull request Aug 18, 2025

chore: integrate ts#22 refactor near/mpc#867

Merged

kevindeforth approved these changes Aug 18, 2025

View reviewed changes

SimonRastikian mentioned this pull request Aug 18, 2025

[Task] Investigate and Improve the communication layer #39

Open

netrome requested changes Aug 18, 2025

View reviewed changes

SimonRastikian added 2 commits August 19, 2025 09:16

No panic for lagrange

1b4f6d8

No panic on send message

61b8da6

fmt + clippy

646bd24

SimonRastikian force-pushed the simon/code-refactor branch from 5db6be6 to 646bd24 Compare August 19, 2025 07:50

netrome approved these changes Aug 19, 2025

View reviewed changes

gilcu3 self-requested a review August 19, 2025 09:04

gilcu3 approved these changes Aug 19, 2025

View reviewed changes

SimonRastikian merged commit 5fd9fe7 into main Aug 19, 2025
1 check passed

This was referenced Aug 19, 2025

Simplify error handling #43

Closed

Implement an Optimization for multi-lagrange coefficient computation #41

Closed

SimonRastikian deleted the simon/code-refactor branch September 4, 2025 08:14

SimonRastikian mentioned this pull request Sep 18, 2025

Simon/rerandomization api ot ecdsa #86

Merged

Simon/code refactor #22

Simon/code refactor #22

Uh oh!

Conversation

SimonRastikian commented Aug 1, 2025 • edited by gilcu3 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevindeforth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonRastikian Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevindeforth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SimonRastikian commented Aug 1, 2025 •

edited by gilcu3

Loading

SimonRastikian Aug 6, 2025 •

edited

Loading

netrome commented Aug 18, 2025 •

edited

Loading

SimonRastikian commented Aug 19, 2025 •

edited

Loading