Skip to content

Commit

Permalink
fix: cleaner sign share request implementation (#114)
Browse files Browse the repository at this point in the history 
* Better sign share function
* update the test
* remove extraneous assert; fix boolean cache name; use short name when logging vars
* test SignatureShareRequest with no NonceResponse and out of range signer_id
---------
Co-authored-by: Joey Yandle <[email protected]>
  • Loading branch information
@djordon
djordon authored Jan 14, 2025
1 parent 69c0bb9 commit 612c023
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 79 deletions.
53 changes: 46 additions & 7 deletions src/state_machine/coordinator/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1117,19 +1117,58 @@ pub mod test {
}
}

// add NonceResponses so there's more than the number of signers
let messages = outbound_messages.clone();
let result = feedback_messages_with_errors(&mut coordinators, &mut signers, &messages);
assert!(result.is_ok());

// test request with no NonceResponses
let mut packet = outbound_messages[0].clone();
if let Message::SignatureShareRequest(ref mut request) = packet.msg {
for _ in 0..1000 {
request
.nonce_responses
.push(request.nonce_responses.first().unwrap().clone());
}
request.nonce_responses.clear();
} else {
panic!("failed to match message");
}

// Send the SignatureShareRequest message to all signers and share their responses with the coordinator and signers
// Send the SignatureShareRequest message to all signers and share
// their responses with the coordinator and signers
let result = feedback_messages_with_errors(&mut coordinators, &mut signers, &[packet]);
if !matches!(
result,
Err(StateMachineError::Signer(SignerError::InvalidNonceResponse))
) {
panic!("Should have received signer invalid nonce response error, got {result:?}");
}

// test request with a duplicate NonceResponse
let mut packet = outbound_messages[0].clone();
if let Message::SignatureShareRequest(ref mut request) = packet.msg {
request
.nonce_responses
.push(request.nonce_responses[0].clone());
} else {
panic!("failed to match message");
}

// Send the SignatureShareRequest message to all signers and share
// their responses with the coordinator and signers
let result = feedback_messages_with_errors(&mut coordinators, &mut signers, &[packet]);
if !matches!(
result,
Err(StateMachineError::Signer(SignerError::InvalidNonceResponse))
) {
panic!("Should have received signer invalid nonce response error, got {result:?}");
}

// test request with an out of range signer_id
let mut packet = outbound_messages[0].clone();
if let Message::SignatureShareRequest(ref mut request) = packet.msg {
request.nonce_responses[0].signer_id = num_signers;
} else {
panic!("failed to match message");
}

// Send the SignatureShareRequest message to all signers and share
// their responses with the coordinator and signers
let result = feedback_messages_with_errors(&mut coordinators, &mut signers, &[packet]);
if !matches!(
result,
Expand Down
128 changes: 56 additions & 72 deletions src/state_machine/signer/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -563,91 +563,75 @@ impl<SignerType: SignerTrait> Signer<SignerType> {
sign_request: &SignatureShareRequest,
rng: &mut R,
) -> Result<Vec<Message>, Error> {
let mut msgs = vec![];

let signer_ids = sign_request
.nonce_responses
.iter()
.map(|nr| nr.signer_id)
.collect::<Vec<u32>>();

let signer_id_set = sign_request
.nonce_responses
.iter()
.map(|nr| nr.signer_id)
.collect::<BTreeSet<u32>>();

if signer_ids.len() != signer_id_set.len()
// The expected usage is that Signer IDs start at zero and
// increment by one until self.total_signers - 1. So the checks
// here should be sufficient for catching empty signer ID sets,
// duplicate signer IDs, or unknown signer IDs.
let is_invalid_request = sign_request.nonce_responses.len() != signer_id_set.len()
|| signer_id_set.is_empty()
|| signer_id_set.len() > self.total_signers.try_into().unwrap()
|| signer_id_set.last().unwrap() > &self.total_signers
{
warn!(
?signer_ids,
"Got SignatureShareRequest with invalid NonceResponse"
);
|| signer_id_set.last() >= Some(&self.total_signers);

if is_invalid_request {
warn!("received an invalid SignatureShareRequest");
return Err(Error::InvalidNonceResponse);
} else {
debug!(?signer_ids, "Got SignatureShareRequest");
}

for signer_id in &signer_ids {
if *signer_id == self.signer_id {
let key_ids: Vec<u32> = sign_request
.nonce_responses
.iter()
.flat_map(|nr| nr.key_ids.iter().copied())
.collect::<Vec<u32>>();
let nonces = sign_request
.nonce_responses
.iter()
.flat_map(|nr| nr.nonces.clone())
.collect::<Vec<PublicNonce>>();
let signature_shares = match sign_request.signature_type {
SignatureType::Taproot(m) => self.signer.sign_taproot(
&sign_request.message,
&signer_ids,
&key_ids,
&nonces,
m,
),
SignatureType::Schnorr => self.signer.sign_schnorr(
&sign_request.message,
&signer_ids,
&key_ids,
&nonces,
),
SignatureType::Frost => {
self.signer
.sign(&sign_request.message, &signer_ids, &key_ids, &nonces)
}
};
debug!(signer_id = %self.signer_id, "received a valid SignatureShareRequest");

if signer_id_set.contains(&self.signer_id) {
let key_ids: Vec<u32> = sign_request
.nonce_responses
.iter()
.flat_map(|nr| nr.key_ids.iter().copied())
.collect::<Vec<u32>>();
let nonces = sign_request
.nonce_responses
.iter()
.flat_map(|nr| nr.nonces.clone())
.collect::<Vec<PublicNonce>>();

let signer_ids = signer_id_set.into_iter().collect::<Vec<_>>();
let msg = &sign_request.message;
let signature_shares = match sign_request.signature_type {
SignatureType::Taproot(merkle_root) => {
self.signer
.sign_taproot(msg, &signer_ids, &key_ids, &nonces, merkle_root)
}
SignatureType::Schnorr => {
self.signer
.sign_schnorr(msg, &signer_ids, &key_ids, &nonces)
}
SignatureType::Frost => self.signer.sign(msg, &signer_ids, &key_ids, &nonces),
};

self.signer.gen_nonces(rng);
self.signer.gen_nonces(rng);

let response = SignatureShareResponse {
dkg_id: sign_request.dkg_id,
sign_id: sign_request.sign_id,
sign_iter_id: sign_request.sign_iter_id,
signer_id: *signer_id,
signature_shares,
};
info!(
%signer_id,
dkg_id = %sign_request.dkg_id,
sign_id = %sign_request.sign_id,
sign_iter_id = %sign_request.sign_iter_id,
"sending SignatureShareResponse"
);

let response = Message::SignatureShareResponse(response);

msgs.push(response);
} else {
debug!("SignatureShareRequest for {} dropped.", signer_id);
}
let response = SignatureShareResponse {
dkg_id: sign_request.dkg_id,
sign_id: sign_request.sign_id,
sign_iter_id: sign_request.sign_iter_id,
signer_id: self.signer_id,
signature_shares,
};
info!(
signer_id = %self.signer_id,
dkg_id = %sign_request.dkg_id,
sign_id = %sign_request.sign_id,
sign_iter_id = %sign_request.sign_iter_id,
"sending SignatureShareResponse"
);

Ok(vec![Message::SignatureShareResponse(response)])
} else {
debug!(signer_id = %self.signer_id, "signer not included in SignatureShareRequest");
Ok(Vec::new())
}
Ok(msgs)
}

fn dkg_begin<R: RngCore + CryptoRng>(
Expand Down

0 comments on commit 612c023

Please sign in to comment.