Conversation
|
PR title type suggestion: This PR adds benchmark implementations, which should use the Suggested title: |
Code ReviewReviewed the DKG benchmark additions. This PR adds benchmark infrastructure for keygen across three ciphersuites (Secp256k1, Ed25519, BLS12-381) using the existing simulated protocol framework. No critical issues found. The code follows existing benchmark patterns (mirrors the CKD bench structure), touches only benchmark/test code, and introduces no production changes. Minor nit (non-blocking): missing newline at end of ✅ Approved |
|
PR title type suggestion: This PR adds benchmark implementations, so the type prefix should be Suggested title: |
|
PR title type suggestion: This PR adds benchmark implementations. The type prefix should be Suggested title: |
|
PR title type suggestion: This PR adds benchmark implementations and documentation, not performance improvements to the code itself. The type prefix should probably be Suggested title: |
|
PR title type suggestion: This PR adds benchmark implementations and documentation, but doesn't change production code performance. The type prefix should probably be |
|
PR title type suggestion: This PR implements benchmark infrastructure rather than improving performance of existing code. The type should probably be Suggested title: |
|
PR title type suggestion: This PR primarily adds benchmark implementations and documentation rather than improving the performance of the code itself. The type should probably be Suggested title: |
|
PR title type suggestion: This PR adds benchmark implementations, which is testing/measurement code rather than a performance improvement to the actual codebase. The type prefix should probably be Suggested title: |
| @@ -0,0 +1,150 @@ | |||
| #![allow(clippy::indexing_slicing)] | |||
There was a problem hiding this comment.
I couldn't find it, but the clippy has the last word
| fn threshold() -> ReconstructionLowerBound { | ||
| ReconstructionLowerBound::from(*MAX_MALICIOUS + 1) | ||
| } | ||
|
|
||
| fn participants_num() -> usize { | ||
| *MAX_MALICIOUS + 1 | ||
| } | ||
|
|
There was a problem hiding this comment.
Hm I think it would be nice to make these lazy loaded constants like MAX_MALICIOUS. But not super important.
| ); | ||
| criterion_main!(benches); | ||
|
|
||
| /****************************** Helpers ******************************/ |
There was a problem hiding this comment.
Not a fan of these comments. I find it nicer to create sub modules if there's a natural division of the code
There was a problem hiding this comment.
I will create a new issue for this. It's gonna make things simpler to review.
|
PR title type suggestion: This PR adds benchmark implementations and testing infrastructure, not performance improvements to the source code itself. The type prefix should probably be Suggested title: |
|
PR title type suggestion: This PR adds benchmark implementations and test utilities, not performance optimizations. The type prefix should probably be Suggested title: |
|
PR title type suggestion: This PR adds benchmark test implementations, so the type prefix should probably be |
|
PR title type suggestion: This PR adds benchmark implementations and documentation rather than performance improvements to the codebase itself. The type prefix should probably be Suggested title: |
| @@ -0,0 +1,150 @@ | |||
| #![allow(clippy::indexing_slicing)] | |||
There was a problem hiding this comment.
I couldn't find it, but the clippy has the last word
| fn bench_dkg_secp256k1(c: &mut Criterion) { | ||
| let num = participants_num(); | ||
| let max_malicious = *MAX_MALICIOUS; | ||
| let mut sizes = Vec::with_capacity(*SAMPLE_SIZE); | ||
|
|
||
| let mut group = c.benchmark_group("dkg"); | ||
| group.sample_size(*SAMPLE_SIZE); | ||
| group.bench_function( | ||
| format!("dkg_secp256k1_MAX_MALICIOUS_{max_malicious}_PARTICIPANTS_{num}"), | ||
| |b| { | ||
| b.iter_batched( | ||
| || { | ||
| let preps = prepare_simulated_dkg::<Secp256K1Sha256>(threshold()); | ||
| sizes.push(preps.simulator.get_view_size()); | ||
| preps | ||
| }, | ||
| |preps| run_simulated_protocol(preps.participant, preps.protocol, preps.simulator), | ||
| criterion::BatchSize::SmallInput, | ||
| ); | ||
| }, | ||
| ); | ||
| analyze_received_sizes(&sizes, true); |
There was a problem hiding this comment.
nit: we could reuse code for all 3 implementations
| pub app_pk: ckd::ElementG1, | ||
| } | ||
|
|
||
| /********************* DKG *********************/ |
There was a problem hiding this comment.
nit: we can remove this one
Closes near/threshold-signatures#252
We decided not to implement Resharing and Refresh as they are conceptually exactly the same thing.
The results documentation will happen in another PR.
Issue opened #2624