Simon/snapshots #218

SimonRastikian · 2025-11-11T19:02:16Z

Closes #145

codecov-commenter · 2025-11-11T19:04:40Z

Codecov Report

❌ Patch coverage is 76.31579% with 72 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.60%. Comparing base (4bee396) to head (1faa085).

Files with missing lines	Patch %	Lines
src/test_utils/run_protocol.rs	54.66%	29 Missing and 5 partials ⚠️
src/test_utils/snapshot.rs	88.61%	23 Missing ⚠️
src/test_utils/participant_simulation.rs	0.00%	15 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                     @@
##           simon/test_utils_folder     #218      +/-   ##
===========================================================
- Coverage                    87.95%   87.60%   -0.35%     
===========================================================
  Files                           47       49       +2     
  Lines                         9064     9365     +301     
  Branches                      9064     9365     +301     
===========================================================
+ Hits                          7972     8204     +232     
- Misses                         645      709      +64     
- Partials                       447      452       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gilcu3

I did not find any big issue, but did have a lot of suggestions, therefore marking the review as "Request changes". Most of them should be quite easy to solve though.

Once that is done, I can do a quick pass again and approve.

src/test_utils/run_protocol.rs

src/test_utils/snapshot.rs

gilcu3 · 2025-11-19T08:45:09Z

src/test_utils/snapshot.rs

+        let max_malicious = 2;
+        let participants = generate_participants(5);
+
+        let f = Polynomial::generate_polynomial(None, max_malicious, &mut OsRng).unwrap();


same, please lets use deterministic tests

gilcu3 · 2025-11-19T08:46:29Z

src/test_utils/snapshot.rs

+    }
+
+    #[test]
+    fn test_snapshot_on_ecdsa_protocol() {


I think this test does not belong to this file, as here we are just implementing general snapshot structs. Not sure what the proper place would be though.

gilcu3 · 2025-11-19T08:47:37Z

src/test_utils/snapshot.rs

+    }
+
+    #[test]
+    fn test_snapshot_on_ecdsa_protocol() {


as far I can see this only runs presign, so the function name is a bit misleading

gilcu3 · 2025-11-19T08:52:21Z

src/test_utils/run_protocol.rs

+pub fn run_protocol_with_snapshots<T>(
+    mut ps: Vec<(Participant, Box<dyn Protocol<Output = T>>)>,
+) -> Result<(Vec<(Participant, T)>, ProtocolSnapshot), ProtocolError> {
+    // Get the participants
+    let participants: Vec<_> = ps.iter().map(|(p, _)| *p).collect();
+    // Build the snapshot
+    let mut protocol_snapshots = ProtocolSnapshot::new_empty(participants);
+
+    // Compute the participants indices
+    let indices: HashMap<Participant, usize> =
+        ps.iter().enumerate().map(|(i, (p, _))| (*p, i)).collect();
+
+    let size = ps.len();
+    let mut out = Vec::with_capacity(size);
+    while out.len() < size {
+        for i in 0..size {
+            while {
+                let action = ps[i].1.poke()?;
+                match action {
+                    Action::Wait => false,
+                    Action::SendMany(m) => {
+                        for j in 0..size {
+                            if i == j {
+                                continue;
+                            }
+                            let from = ps[i].0;
+                            let to = ps[j].0;
+                            protocol_snapshots
+                                .push_message(to, from, m.clone())
+                                .ok_or_else(|| {
+                                    ProtocolError::Other(
+                                        "Participant not found in snapshot".to_string(),
+                                    )
+                                })?;
+                            ps[j].1.message(from, m.clone());
+                        }
+                        true
+                    }
+                    Action::SendPrivate(to, m) => {
+                        let from = ps[i].0;
+                        protocol_snapshots
+                            .push_message(to, from, m.clone())
+                            .ok_or_else(|| {
+                                ProtocolError::Other(
+                                    "Participant not found in snapshot".to_string(),
+                                )
+                            })?;
+                        ps[indices[&to]].1.message(from, m);
+                        true
+                    }
+                    Action::Return(r) => {
+                        out.push((ps[i].0, r));
+                        false
+                    }
+                }
+            } {}
+        }
+    }
+    Ok((out, protocol_snapshots))
+}


This function has an almost identical implementation to the previous one. It would be nice to refactor them so that the logic is only implemented once (in a third function)

gilcu3 · 2025-11-19T08:56:04Z

src/test_utils/mockrng.rs

 use rand_chacha::{rand_core::SeedableRng, ChaCha12Rng};

 /// Used for deterministic Rngs and only in testing
+#[derive(Debug, Clone, PartialEq, Eq)]


This feels a bit strange, but I guess if they are implemented for ChaCha12Rng then its nice for us to have them as well

I'm generally happy to see many traits being eagerly derived as per https://rust-lang.github.io/api-guidelines/interoperability.html?highlight=trait#types-eagerly-implement-common-traits-c-common-traits

netrome

Some nits from my side, but overall I think it looks good. I agree with the general comments about test organization from Reynaldo.

The only blockers I see are:

Some docstrings are a bit confusing right now.
The tests that use OsRng should be made deterministic.

netrome · 2025-11-19T10:32:51Z

src/test_utils/mockrng.rs

 use rand_chacha::{rand_core::SeedableRng, ChaCha12Rng};

 /// Used for deterministic Rngs and only in testing
+#[derive(Debug, Clone, PartialEq, Eq)]


I'm generally happy to see many traits being eagerly derived as per https://rust-lang.github.io/api-guidelines/interoperability.html?highlight=trait#types-eagerly-implement-common-traits-c-common-traits

netrome · 2025-11-19T10:36:18Z

src/test_utils/snapshot.rs

+
+use crate::{participants::Participant, protocol::MessageData};
+
+/// Constructor for specific (sender, messages) pairs transmitted during a protocol run


This is not a constructor. Is the docstring stale or would it be sufficient to just say something like

Suggested change

/// Constructor for specific (sender, messages) pairs transmitted during a protocol run

/// A single received message during a protocol run

netrome · 2025-11-19T10:39:15Z

src/test_utils/snapshot.rs

+    }
+}
+
+/// Registers a particular participant's view of the received messages


Nit: "Registers" sounds confusing here since this is a data structure, not a behavior.

Suggested change

/// Registers a particular participant's view of the received messages

/// A participant's view of their received messages

src/test_utils/snapshot.rs

netrome · 2025-11-19T10:47:06Z

src/test_utils/snapshot.rs

+    }
+
+    #[test]
+    fn test_snapshot_on_ecdsa_protocol() {


If I understand this correctly, the purpose of the test is to verify that the protocol is deterministic. How about naming it something like:

Suggested change

fn test_snapshot_on_ecdsa_protocol() {

fn ecdsa_presign__should_return_same_snapshot_when_executed_twice() {

SimonRastikian added 2 commits November 11, 2025 20:00

Snapshotting all

f7a34c0

Cargo fmt

3720e8f

SimonRastikian self-assigned this Nov 11, 2025

SimonRastikian linked an issue Nov 11, 2025 that may be closed by this pull request

[Benchmark] [Participant Simulation] Protocols Derandomization: Networking Layer Change #145

Open

SimonRastikian force-pushed the simon/snapshots branch from 1faa085 to 3720e8f Compare November 14, 2025 12:30

This was referenced Nov 17, 2025

Investigate the possibility of simulating multiparty protocols with a single thread #224

Open

chore: added deterministic tests #225

Open

gilcu3 requested changes Nov 19, 2025

View reviewed changes

netrome requested changes Nov 19, 2025

View reviewed changes


		use crate::{participants::Participant, protocol::MessageData};

		/// Constructor for specific (sender, messages) pairs transmitted during a protocol run

	/// Constructor for specific (sender, messages) pairs transmitted during a protocol run
	/// A single received message during a protocol run

	/// Registers a particular participant's view of the received messages
	/// A participant's view of their received messages

	fn test_snapshot_on_ecdsa_protocol() {
	fn ecdsa_presign__should_return_same_snapshot_when_executed_twice() {

Simon/snapshots #218

Are you sure you want to change the base?

Simon/snapshots #218

Conversation

SimonRastikian commented Nov 11, 2025

Uh oh!

codecov-commenter commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gilcu3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

netrome 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Nov 11, 2025 •

edited

Loading