Skip to content
This repository has been archived by the owner on Oct 31, 2024. It is now read-only.

No side-effects in getters #39

Closed
wants to merge 8 commits into from
Closed

No side-effects in getters #39

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e4b3b57
add: Add a dedup_signers
dvdplm Mar 25, 2024
89be29c
fix: broken benches
dvdplm Mar 25, 2024
26e2573
Do sorting and deduplication when adding signers
dvdplm Mar 26, 2024
20859c1
chore: appease clippy
dvdplm Mar 27, 2024
0789ebf
Update src/error.rs
dvdplm Mar 27, 2024
c06d735
Review feedback
dvdplm Mar 27, 2024
5b118d3
chore: appease clippy
dvdplm Mar 27, 2024
b011285
Merge branch 'main' into dp-no-side-effects-in-getters
dvdplm Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benches/sign.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
SignatureAggregator,
};
use ice_frost::testing::Secp256k1Sha256;
use ice_frost::CipherSuite;

Check warning on line 20 in benches/sign.rs

View workflow job for this annotation

GitHub Actions / Bitrot check (stable, ubuntu-latest) 

unused import: `ice_frost::CipherSuite`

Check failure on line 20 in benches/sign.rs

View workflow job for this annotation

GitHub Actions / Clippy

unused import: `ice_frost::CipherSuite` 

error: unused import: `ice_frost::CipherSuite`
  --> benches/sign.rs:20:5
   |
20 | use ice_frost::CipherSuite;
   |     ^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unused_imports)]`

type ParticipantDKG = Participant<Secp256k1Sha256>;
type Coeff = Coefficients<Secp256k1Sha256>;
Expand Down Expand Up @@ -164,8 +164,8 @@
);
}

let signers = aggregator.get_signers().clone();

Check failure on line 167 in benches/sign.rs

View workflow job for this annotation

GitHub Actions / Bitrot check (stable, ubuntu-latest) 

no method named `get_signers` found for struct `SignatureAggregator` in the current scope

Check failure on line 167 in benches/sign.rs

View workflow job for this annotation

GitHub Actions / Clippy

no method named `get_signers` found for struct `ice_frost::sign::SignatureAggregator` in the current scope 

error[E0599]: no method named `get_signers` found for struct `ice_frost::sign::SignatureAggregator` in the current scope
   --> benches/sign.rs:167:30
    |
167 |     let signers = aggregator.get_signers().clone();
    |                              ^^^^^^^^^^^
    |
help: there is a method `signers` with a similar name
    |
167 |     let signers = aggregator.signers().clone();
    |                              ~~~~~~~
let message_hash = Secp256k1Sha256::h4(&message[..]);
let message_hash = Secp25dedup_signersh4(&message[..]);

Check failure on line 168 in benches/sign.rs

View workflow job for this annotation

GitHub Actions / Bitrot check (stable, ubuntu-latest) 

cannot find function, tuple struct or tuple variant `Secp25dedup_signersh4` in this scope

Check failure on line 168 in benches/sign.rs

View workflow job for this annotation

GitHub Actions / Clippy

cannot find function, tuple struct or tuple variant `Secp25dedup_signersh4` in this scope 

error[E0425]: cannot find function, tuple struct or tuple variant `Secp25dedup_signersh4` in this scope
   --> benches/sign.rs:168:24
    |
168 |     let message_hash = Secp25dedup_signersh4(&message[..]);
    |                        ^^^^^^^^^^^^^^^^^^^^^ not found in this scope
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
let message_hash_copy = message_hash;

let p1_sk = participants_secret_keys[0].clone();
Expand Down
4 changes: 2 additions & 2 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1455,7 +1455,7 @@
//! #
//! # aggregator.include_signer(1, alice_public_comshares.commitments[0], &alice_public_key);
//! # aggregator.include_signer(3, carol_public_comshares.commitments[0], &carol_public_key);
//! let signers = aggregator.get_signers();
//! let signers = aggregator.signers();
//! # Ok(()) }
//! # fn main() { assert!(do_test().is_ok()); }
//! ```
Expand Down Expand Up @@ -1530,7 +1530,7 @@
//! # aggregator.include_signer(1, alice_public_comshares.commitments[0], &alice_public_key);
//! # aggregator.include_signer(3, carol_public_comshares.commitments[0], &carol_public_key);
//! #
//! # let signers = aggregator.get_signers();
//! # let signers = aggregator.signers();
//! # let message_hash = Secp256k1Sha256::h4(&message[..]);
//!
//! let alice_partial = alice_secret_key.sign(
Expand Down
98 changes: 57 additions & 41 deletions src/sign/signature.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,18 +475,33 @@ impl<C: CipherSuite> SignatureAggregator<C, Initial<'_>> {
.insert(public_key.index, public_key.share);
}

/// Get the list of partipating signers.
/// Sort and deduplicate partipating [`Signer`]s and check that there are
/// enough unique signers given the [`ThresholdParameters`].
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
///
/// # Returns
///
/// A `&Vec<Signer>` of the participating signers in this round.
pub fn get_signers(&'_ mut self) -> &'_ Vec<Signer<C>> {
/// [`Ok`] if the set of signers is still valid for the [`ThresholdParameters`] of the instance, otherwise [`Err`] .
pub fn dedup_signers(&'_ mut self) -> FrostResult<C, ()> {
self.state.signers.sort();
self.state.signers.dedup();

// Sanity check
assert!(self.state.signers.len() <= self.state.parameters.n as usize);
if self.state.signers.len() <= self.state.parameters.n as usize {
Ok(())
} else {
Err(Error::InvalidNumberOfParticipants(
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
self.state.signers.len(),
self.state.parameters.n,
))
}
}

/// Get the list of participating [`Signer`]s.
///
/// # Returns
///
/// A slice of [`Signer`]s.
pub fn signers(&self) -> &[Signer<C>] {
&self.state.signers
}

Expand All @@ -503,7 +518,7 @@ impl<C: CipherSuite> SignatureAggregator<C, Initial<'_>> {
/// A sorted [`Vec`] of unique [`Signer`]s who have yet to contribute their
/// partial signatures.
#[must_use]
pub fn get_remaining_signers(&self) -> Vec<Signer<C>> {
pub fn remaining_signers(&self) -> Vec<Signer<C>> {
let mut remaining_signers = Vec::with_capacity(self.state.signers.len());

for signer in &self.state.signers {
Expand Down Expand Up @@ -541,7 +556,7 @@ impl<C: CipherSuite> SignatureAggregator<C, Initial<'_>> {
/// s.t. \(( t \le t' \le n \)).
pub fn finalize(mut self) -> FrostResult<C, SignatureAggregator<C, Finalized<C>>> {
let mut misbehaving_participants = Vec::new();
let remaining_signers = self.get_remaining_signers();
let remaining_signers = self.remaining_signers();

// We're reporting missing partial signatures which
// could possibly be the fault of the aggregator, but here we explicitly
Expand All @@ -556,7 +571,7 @@ impl<C: CipherSuite> SignatureAggregator<C, Initial<'_>> {
}

// Ensure that our new state is ordered and deduplicated.
self.state.signers = self.get_signers().clone();
self.dedup_signers()?;

for signer in &self.state.signers {
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
if self
Expand Down Expand Up @@ -731,7 +746,7 @@ mod test {

aggregator.include_signer(1, p1_public_comshares.commitments[0], &p1_sk.to_public());

let signers = aggregator.get_signers();
aggregator.dedup_signers().unwrap();
let message_hash = Secp256k1Sha256::h4(&message[..]);

let p1_partial = p1_sk
Expand All @@ -740,7 +755,7 @@ mod test {
&group_key,
&mut p1_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();

Expand Down Expand Up @@ -772,7 +787,7 @@ mod test {

aggregator.include_signer(1, p1_public_comshares.commitments[0], &p1_sk.to_public());

let signers = aggregator.get_signers();
aggregator.dedup_signers().unwrap();
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
let message_hash = Secp256k1Sha256::h4(&message[..]);

let p1_partial = p1_sk
Expand All @@ -781,7 +796,7 @@ mod test {
&group_key,
&mut p1_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();

Expand Down Expand Up @@ -809,7 +824,7 @@ mod test {

aggregator.include_signer(1, p1_public_comshares.commitments[0], &p1_sk.to_public());

let signers = aggregator.get_signers();
aggregator.dedup_signers().unwrap();
let message_hash = Secp256k1Sha256::h4(&message[..]);

let p1_partial = p1_sk
Expand All @@ -818,7 +833,7 @@ mod test {
&group_key,
&mut p1_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();

Expand Down Expand Up @@ -854,7 +869,7 @@ mod test {
aggregator.include_signer(3, p3_public_comshares.commitments[0], &p3_sk.to_public());
aggregator.include_signer(4, p4_public_comshares.commitments[0], &p4_sk.to_public());

let signers = aggregator.get_signers();
aggregator.dedup_signers().unwrap();
let message_hash = Secp256k1Sha256::h4(&message[..]);

let p1_partial = p1_sk
Expand All @@ -863,7 +878,7 @@ mod test {
&group_key,
&mut p1_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();
let p3_partial = p3_sk
Expand All @@ -872,7 +887,7 @@ mod test {
&group_key,
&mut p3_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();
let p4_partial = p4_sk
Expand All @@ -881,7 +896,7 @@ mod test {
&group_key,
&mut p4_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();

Expand Down Expand Up @@ -915,7 +930,7 @@ mod test {
aggregator.include_signer(1, p1_public_comshares.commitments[0], &p1_sk.to_public());
aggregator.include_signer(2, p2_public_comshares.commitments[0], &p2_sk.to_public());

let signers = aggregator.get_signers();
aggregator.dedup_signers().unwrap();
let message_hash = Secp256k1Sha256::h4(&message[..]);

let p1_partial = p1_sk
Expand All @@ -924,7 +939,7 @@ mod test {
&group_key,
&mut p1_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();
let p2_partial = p2_sk
Expand All @@ -933,7 +948,7 @@ mod test {
&group_key,
&mut p2_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();

Expand Down Expand Up @@ -973,7 +988,7 @@ mod test {
aggregator.include_signer(1, d1_public_comshares.commitments[0], &d1_sk.to_public());
aggregator.include_signer(2, d2_public_comshares.commitments[0], &d2_sk.to_public());

let signers = aggregator.get_signers();
aggregator.dedup_signers().unwrap();
let message_hash = Secp256k1Sha256::h4(&message[..]);

let d1_partial = d1_sk
Expand All @@ -982,7 +997,7 @@ mod test {
&group_key,
&mut d1_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();
let d2_partial = d2_sk
Expand All @@ -991,7 +1006,7 @@ mod test {
&group_key,
&mut d2_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();

Expand Down Expand Up @@ -1019,7 +1034,7 @@ mod test {
aggregator.include_signer(1, s1_public_comshares.commitments[0], &s1_sk.to_public());
aggregator.include_signer(2, s2_public_comshares.commitments[0], &s2_sk.to_public());

let signers = aggregator.get_signers();
aggregator.dedup_signers().unwrap();
let message_hash = Secp256k1Sha256::h4(&message[..]);

let s1_partial = s1_sk
Expand All @@ -1028,7 +1043,7 @@ mod test {
&group_key,
&mut s1_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();
let s2_partial = s2_sk
Expand All @@ -1037,7 +1052,7 @@ mod test {
&group_key,
&mut s2_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();

Expand Down Expand Up @@ -1086,7 +1101,7 @@ mod test {
aggregator.include_signer(1, d1_public_comshares.commitments[0], &d1_sk.to_public());
aggregator.include_signer(2, d2_public_comshares.commitments[0], &d2_sk.to_public());

let signers = aggregator.get_signers();
aggregator.dedup_signers().unwrap();
let message_hash = Secp256k1Sha256::h4(&message[..]);

let d1_partial = d1_sk
Expand All @@ -1095,7 +1110,7 @@ mod test {
&group_key,
&mut d1_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();
let d2_partial = d2_sk
Expand All @@ -1104,7 +1119,7 @@ mod test {
&group_key,
&mut d2_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();

Expand Down Expand Up @@ -1135,7 +1150,7 @@ mod test {
aggregator.include_signer(2, s2_public_comshares.commitments[0], &s2_sk.to_public());
aggregator.include_signer(3, s3_public_comshares.commitments[0], &s3_sk.to_public());

let signers = aggregator.get_signers();
aggregator.dedup_signers().unwrap();
let message_hash = Secp256k1Sha256::h4(&message[..]);

let s1_partial = s1_sk
Expand All @@ -1144,7 +1159,7 @@ mod test {
&group_key,
&mut s1_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();
let s2_partial = s2_sk
Expand All @@ -1153,7 +1168,7 @@ mod test {
&group_key,
&mut s2_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();
let s3_partial = s3_sk
Expand All @@ -1162,7 +1177,7 @@ mod test {
&group_key,
&mut s3_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();

Expand Down Expand Up @@ -1213,7 +1228,8 @@ mod test {
aggregator.include_signer(1, p1_public_comshares.commitments[0], &p1_sk.to_public());
aggregator.include_signer(2, p2_public_comshares.commitments[0], &p2_sk.to_public());

let signers = aggregator.get_signers();
aggregator.dedup_signers().unwrap();
let signers = aggregator.state.signers;

// The signers should be deduplicated.
assert!(signers.len() == 2);
Expand Down Expand Up @@ -1493,7 +1509,7 @@ mod test {
&p5_secret_key.to_public(),
);

let signers = aggregator.get_signers();
aggregator.dedup_signers().unwrap();
let message_hash = Secp256k1Sha256::h4(&message[..]);

let p2_partial = p2_secret_key
Expand All @@ -1502,7 +1518,7 @@ mod test {
&group_key,
&mut p2_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();
let p3_partial = p3_secret_key
Expand All @@ -1511,7 +1527,7 @@ mod test {
&group_key,
&mut p3_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();
let p5_partial = p5_secret_key
Expand All @@ -1520,7 +1536,7 @@ mod test {
&group_key,
&mut p5_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();

Expand Down Expand Up @@ -1555,7 +1571,7 @@ mod test {
aggregator.include_signer(1, p1_public_comshares.commitments[0], &p1_sk.to_public());
aggregator.include_signer(2, p2_public_comshares.commitments[0], &p2_sk.to_public());

let signers = aggregator.get_signers();
aggregator.dedup_signers().unwrap();
let message_hash = Secp256k1Sha256::h4(&message[..]);

let p1_partial = p1_sk
Expand All @@ -1564,7 +1580,7 @@ mod test {
&group_key,
&mut p1_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();
let p2_partial = p2_sk
Expand All @@ -1573,7 +1589,7 @@ mod test {
&group_key,
&mut p2_secret_comshares,
0,
signers,
&aggregator.state.signers,
)
.unwrap();

Expand Down
Loading
Loading