Skip to content

fix(bench): make sure benches do not fail by fixing determinism - #3635

Merged
gilcu3 merged 1 commit into
mainfrom
3516-simulated-replay-benches-silently-measure-time-to-error-instead-of-protocol-runtime
Jun 23, 2026
Merged

fix(bench): make sure benches do not fail by fixing determinism#3635
gilcu3 merged 1 commit into
mainfrom
3516-simulated-replay-benches-silently-measure-time-to-error-instead-of-protocol-runtime

Conversation

@gilcu3

@gilcu3 gilcu3 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Closes #3516

Comment on lines 297 to +381
@@ -377,6 +378,7 @@ fn robust_run_sign(
}

let (results, metrics) = run_simulation(protocols, latency);
assert_eq!(results.len(), participants.len());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are broken window fixes. Not strictly needed, but better to have them in case a participant does not finish

Base automatically changed from 3633-move-ckd-bench-helpers-out-of-the-frost_eddsa-bench-utils-module to main June 22, 2026 07:42
@gilcu3
gilcu3 force-pushed the 3516-simulated-replay-benches-silently-measure-time-to-error-instead-of-protocol-runtime branch from 9e7165a to 7091f38 Compare June 22, 2026 07:42
@gilcu3
gilcu3 marked this pull request as ready for review June 22, 2026 07:44
@claude

claude Bot commented Jun 22, 2026

Copy link
Copy Markdown

PR title type suggestion: This PR only changes benchmark files (test infrastructure), so the type prefix should probably be test: instead of fix:.
Suggested title: test(bench): make sure benches do not fail by fixing determinism

@claude

claude Bot commented Jun 22, 2026

Copy link
Copy Markdown

Pull request overview

Fixes the simulated-replay benches in crates/threshold-signatures/benches/ so they fail loudly when the replay diverges, instead of silently timing the path to an error. The previous code passed the Result of run_simulated_protocol/run_protocol straight to criterion which dropped errors, and rebuilt the real participant's RNG via fragile order-dependent reconstructions (MockCryptoRng::seed_from_u64(42) + N next_u64() calls). The new approach records each per-participant seed in a HashMap<Participant, u64> during snapshot capture and rebuilds the RNG via a small participant_rng helper. Resolves #3516.

Changes:

  • Add .expect("...") on every run_simulated_protocol/run_protocol call inside iter_batched closures across the advanced_* and naive_* benches.
  • Introduce participant_rng(&seeds, participant) and a seeds: HashMap<Participant, u64> field on the various Prepared* structs (PreparedPresig, PreparedDkgPackage, PreparedCkdPackage, FrostEd25519SigV1, OTECDSAPreparedTriples, RobustECDSAPreparedPresig).
  • Replace handwritten RNG-replay loops in advanced_ot_based_ecdsa.rs, advanced_robust_ecdsa.rs, and advanced_eddsa_frost_sign_v2.rs with participant_rng(&preps.seeds, real_participant).
  • Add assert_eq!(results.len(), participants.len()) after run_simulation in simulate_ckd.rs, simulate_ecdsa.rs, and simulate_frost.rs to catch participants that silently dropped out.
  • Convert prepare_dkg's return from Vec<(Participant, ...)> to a struct PreparedDkgPackage { protocols, seeds }; simulate_dkg.rs and advanced_dkg.rs updated to destructure.

Reviewed changes

Per-file summary
File Description
benches/bench_utils.rs Adds participant_rng helper and seeds field on PreparedPresig.
benches/bench_utils/ckd.rs Records per-participant seed in PreparedCkdPackage.seeds.
benches/bench_utils/dkg.rs Returns new PreparedDkgPackage carrying protocols + seeds.
benches/bench_utils/frost_eddsa.rs ed25519_build_presign_protocols now returns (protocols, seeds); ed25519_prepare_sign_v1 exposes seeds.
benches/bench_utils/ot_based_ecdsa.rs Records seeds for ot_ecdsa_prepare_triples; ot_ecdsa_prepare_presign returns an empty seeds map (no per-participant RNG).
benches/bench_utils/robust_ecdsa.rs Records seeds for robust_ecdsa_prepare_presign.
benches/advanced_dkg.rs Destructures PreparedDkgPackage; uses participant_rng; .expect on replay.
benches/advanced_eddsa_frost_sign_v1.rs Replaces RNG passthrough with participant_rng; .expect on replay.
benches/advanced_eddsa_frost_sign_v2.rs Removes handwritten replay loop; uses participant_rng; .expect on replay.
benches/advanced_ot_based_ecdsa.rs Same refactor for triples setup; .expect on replay.
benches/advanced_robust_ecdsa.rs Same refactor for presign setup; .expect on replay.
benches/ckd.rs .expect on replay; uses participant_rng for coordinator RNG.
benches/naive_ot_based_ecdsa.rs / naive_robust_ecdsa.rs .expect("protocol should complete") on every run_protocol call.
benches/simulate_ckd.rs / simulate_ecdsa.rs / simulate_frost.rs Add assert_eq!(results.len(), participants.len()) after run_simulation.
benches/simulate_dkg.rs Updated to use .protocols on new PreparedDkgPackage.

Findings

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

  • crates/threshold-signatures/benches/bench_utils.rs:79-82 — The doc comment on PreparedPresig::seeds ("empty when the protocol is built from deterministic inputs") describes a quirk of one specific constructor (ot_ecdsa_prepare_presign, which passes HashMap::new() at bench_utils/ot_based_ecdsa.rs:96). Because the field is the same type in either case, a future caller that mistakenly calls participant_rng(&preps.seeds, ...) on an OT-based presign would hit the expect in participant_rng only at runtime. Consider modeling this with two distinct types (or an Option<HashMap<...>>) so the absence is encoded in the type. Acceptable as a follow-up given this is bench-only code.
  • crates/threshold-signatures/benches/bench_utils.rs:40-48participant_rng is generic over BuildHasher, but every caller passes HashMap<Participant, u64> with the default hasher. The generic parameter adds noise without buying anything; could drop S and accept &HashMap<Participant, u64> directly.
  • crates/threshold-signatures/benches/advanced_dkg.rs:106, advanced_eddsa_frost_sign_v1.rs:77, advanced_eddsa_frost_sign_v2.rs:104, advanced_ot_based_ecdsa.rs:161, advanced_robust_ecdsa.rs:123, ckd.rs:84 — The "rebuild the exact rng the real participant used during snapshot capture" comment is repeated verbatim across 6 sites and largely paraphrases the next line (participant_rng(&preps.seeds, ...)). The why — that the simulator's recorded messages only match if the real participant replays its exact internal randomness — would be more valuable in the participant_rng doc comment alone, and the call-site comments could be dropped. (Per engineering-standards.md "Write helpful code comments".)
  • crates/threshold-signatures/benches/simulate_ecdsa.rs:381, simulate_ckd.rs:113, simulate_ckd.rs:151, simulate_frost.rs:161, simulate_frost.rs:193 — As the author already noted on the inline thread, the assert_eq!(results.len(), participants.len()) additions catch the case where a participant drops out entirely. Note that assert!(results.iter().any(|(_, sig)| sig.is_some())) remains too weak (only one valid output is required), so participant failures past assert_eq! can still slip through as None outputs — worth a follow-up to assert quorum size, not just non-emptiness. Not new to this PR.

Approved

@gilcu3
gilcu3 added this pull request to the merge queue Jun 23, 2026
Merged via the queue into main with commit ab5c8fc Jun 23, 2026
18 checks passed
@gilcu3
gilcu3 deleted the 3516-simulated-replay-benches-silently-measure-time-to-error-instead-of-protocol-runtime branch June 23, 2026 06:32
nocktoshi pushed a commit to nocktoshi/mpc that referenced this pull request Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Simulated-replay benches silently measure time-to-error instead of protocol runtime

3 participants