Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(test):sign result #53

Merged
merged 17 commits into from
Apr 28, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
3 changes: 2 additions & 1 deletion src/tests/mock.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::proto;
use proto::message_out::SignResult;
use std::collections::HashMap;
use tokio::sync::mpsc;

Expand All @@ -16,7 +17,7 @@ pub(super) trait Party: Sync + Send {
channels: SenderReceiver,
delivery: Deliverer,
my_uid: &str,
);
) -> SignResult;
async fn shutdown(mut self);
fn get_db_path(&self) -> std::path::PathBuf;
}
Expand Down
93 changes: 82 additions & 11 deletions src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,13 @@ use std::convert::TryFrom;
mod mock;
mod tofnd_party;

use crate::proto;
use crate::proto::{
self,
message_out::{
sign_result::SignResultData::{Criminals, Signature},
SignResult,
},
};
use mock::{Deliverer, Party};
use tofnd_party::TofndParty;

Expand All @@ -30,6 +36,7 @@ struct TestCase {
share_counts: Vec<u32>,
threshold: usize,
signer_indices: Vec<usize>,
criminal_list: Vec<usize>,
#[cfg(feature = "malicious")]
malicious_types: Vec<MaliciousType>,
}
Expand All @@ -42,11 +49,13 @@ impl TestCase {
threshold: usize,
signer_indices: Vec<usize>,
) -> TestCase {
let criminal_list = vec![];
TestCase {
uid_count,
share_counts,
threshold,
signer_indices,
criminal_list,
}
}

Expand All @@ -58,11 +67,18 @@ impl TestCase {
signer_indices: Vec<usize>,
sdaveas marked this conversation as resolved.
Show resolved Hide resolved
malicious_types: Vec<MaliciousType>,
) -> TestCase {
let mut criminal_list = Vec::new();
for (i, m) in malicious_types.iter().enumerate() {
if !matches!(m, Honest) {
criminal_list.push(i);
}
}
TestCase {
uid_count,
share_counts,
threshold,
signer_indices,
criminal_list,
malicious_types,
}
}
Expand Down Expand Up @@ -128,6 +144,43 @@ impl InitParties {
}
}

fn check_results(results: Vec<SignResult>, sign_indices: &[usize], expected_criminals: &[usize]) {
assert_eq!(sign_indices.len(), results.len());
let first = &results[sign_indices[0]];

// if no criminals were defined, check that all parties produced the same output
if let Some(Signature(_)) = first.sign_result_data {
sdaveas marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(
expected_criminals.len(),
0,
"Expected criminals but didn't discover any",
);
for (i, result) in results.iter().enumerate() {
assert_eq!(
first, result,
"party {} didn't produce the expected result",
i
);
}
}

// if criminals were defined, check that all parties found all of them
if let Some(Criminals(criminal_list)) = first.sign_result_data.clone() {
// get criminal list
let mut criminals = criminal_list.criminals;
// remove duplicates
criminals.dedup();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you ever observe duplicates? We should probably fail the test here if this list contains duplicates.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, there is a single entry for each share of the malicious party.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, more investigation is needed here. Each party runs multiple shares, but this code breaks on the first occurrence of SignResult, so we should only ever see one copy of the result. (Hence we should not have duplicates in the criminals list.) Perhaps we should switch to a wait group here similar to what's in production code. In any case, we don't have an explanation for why you are observing duplicates in the criminals list.

Copy link
Contributor

@sdaveas sdaveas Apr 26, 2021

Choose a reason for hiding this comment

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

The result contains the uid of the criminal one time for each of his shares. I think it's logical because all shares that belong to the malicious party are registered as individual criminals, and multiple shares map to the same uid.

Example: if a malicious party A has shares with tofn indices 0,1,2, the criminal_list will contain [0:Malicious, 1:Malicious, 2:Malicious] (which correspond to [tofn_index, CrimeType] for each pair).

In tofnd, we examine the uid of the criminal, rather than his tofn share indices. This means that the criminal vector will become [A:Malicious, A:Malicious, A:Malicious] (which correspond to [tofnd_index, CrimeType] for each pair).

Copy link
Contributor

Choose a reason for hiding this comment

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

The conversion from tofnd to tofn index happens here:

let (criminal_index, _) = map_tofn_to_tofnd_idx(c.index, all_share_counts)
.expect("failure to recover tofnd party index from tofn share index");
ProtoCriminal {
party_uid: participant_uids[criminal_index].clone(),
crime_type: ProtoCrimeType::from(c.crime_type) as i32, // why `as i32`? https://github.com/danburkert/prost#enumerations
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! As discussed, we need to decide how to report the criminals list to axelar-core:

  • Criminals[A:0, A:1, B:0, B:1], or
  • Criminals[A, B]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related: we also need to decide how tofnd should implement malicious behaviour over multiple subshares:

  • all subshares do the same malicious behaviour
  • only one subshare is malicious (eg. the 0th subshare), all other's are honest
  • something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Option 1 is the simplest case for the current codebase in tofnd. Let's start with that and see if we need to fallback to option 2?

// check that we are left with as many criminals as expected
assert!(criminals.len() == expected_criminals.len());
// check that every criminal was in the "expected" list
for res in criminals.iter().zip(expected_criminals).into_iter() {
let criminal_uid = format!("{}", (b'A' + *res.1 as u8) as char);
assert_eq!(res.0.party_uid, criminal_uid);
}
// println!("criminals: {:?}", criminals);
}
}

#[traced_test]
#[tokio::test]
async fn basic_keygen_and_sign() {
Expand All @@ -139,6 +192,7 @@ async fn basic_keygen_and_sign() {
let party_share_counts = &test_case.share_counts;
let threshold = test_case.threshold;
let sign_participant_indices = &test_case.signer_indices;
let expected_criminals = &test_case.criminal_list;

// get malicious types only when we are in malicious mode
#[cfg(feature = "malicious")]
Expand Down Expand Up @@ -174,7 +228,7 @@ async fn basic_keygen_and_sign() {

// println!("sign: participants {:?}", sign_participant_indices);
let new_sig_uid = "Gus-test-sig";
let parties = execute_sign(
let sign_output = execute_sign(
sdaveas marked this conversation as resolved.
Show resolved Hide resolved
parties,
&party_uids,
sign_participant_indices,
Expand All @@ -184,8 +238,13 @@ async fn basic_keygen_and_sign() {
)
.await;

let parties = sign_output.0;

delete_dbs(&parties);
shutdown_parties(parties).await;

let results = sign_output.1;
check_results(results, &sign_participant_indices, &expected_criminals);
}
}

Expand All @@ -199,6 +258,7 @@ async fn restart_one_party() {
let party_share_counts = &test_case.share_counts;
let threshold = test_case.threshold;
let sign_participant_indices = &test_case.signer_indices;
let expected_criminals = &test_case.criminal_list;

// get malicious types only when we are in malicious mode
#[cfg(feature = "malicious")]
Expand Down Expand Up @@ -256,7 +316,7 @@ async fn restart_one_party() {

// println!("sign: participants {:?}", sign_participant_indices);
let new_sig_uid = "Gus-test-sig";
let parties = execute_sign(
let sign_output = execute_sign(
parties,
&party_uids,
&sign_participant_indices,
Expand All @@ -266,8 +326,13 @@ async fn restart_one_party() {
)
.await;

let parties = sign_output.0;

delete_dbs(&parties);
shutdown_parties(parties).await;

let results = sign_output.1;
check_results(results, &sign_participant_indices, &expected_criminals);
}
}

Expand Down Expand Up @@ -375,7 +440,7 @@ async fn execute_sign(
key_uid: &str,
new_sig_uid: &str,
msg_to_sign: &[u8],
) -> Vec<impl Party> {
) -> (Vec<impl Party>, Vec<proto::message_out::SignResult>) {
let participant_uids: Vec<String> = sign_participant_indices
.iter()
.map(|&i| party_uids[i].clone())
Expand All @@ -402,20 +467,26 @@ async fn execute_sign(

// execute the protocol in a spawn
let handle = tokio::spawn(async move {
party
let result = party
.execute_sign(init, channel_pair, delivery, &participant_uid)
.await;
party
(party, result)
});
sign_join_handles.push((participant_index, handle));
}

let mut results = Vec::new();
// move participants back into party_options
for (i, h) in sign_join_handles {
party_options[i] = Some(h.await.unwrap());
let handle = h.await.unwrap();
party_options[i] = Some(handle.0);
results.push(handle.1);
}
party_options
.into_iter()
.map(|o| o.unwrap())
.collect::<Vec<_>>()
(
party_options
.into_iter()
.map(|o| o.unwrap())
.collect::<Vec<_>>(),
results,
)
}
15 changes: 10 additions & 5 deletions src/tests/tofnd_party.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::{mock::SenderReceiver, Deliverer, InitParty, Party};
use crate::{addr, gg20, proto};
use proto::message_out::SignResult;
use std::convert::TryFrom;
use std::path::Path;
use tokio::{net::TcpListener, sync::oneshot, task::JoinHandle};
Expand Down Expand Up @@ -125,7 +126,7 @@ impl Party for TofndParty {
channels: SenderReceiver,
mut delivery: Deliverer,
my_uid: &str,
) {
) -> SignResult {
let my_display_name = format!("{}:{}", my_uid, self.server_port); // uid:port
let (sign_server_incoming, rx) = channels;
let mut sign_server_outgoing = self
Expand All @@ -142,16 +143,17 @@ impl Party for TofndParty {
})
.unwrap();

let mut sign_completed = false;
// use Option of SignResult to avoid giving a default value to SignResult
let mut result: Option<SignResult> = None;
sdaveas marked this conversation as resolved.
Show resolved Hide resolved
while let Some(msg) = sign_server_outgoing.message().await.unwrap() {
let msg_type = msg.data.as_ref().expect("missing data");

match msg_type {
proto::message_out::Data::Traffic(_) => {
delivery.deliver(&msg, &my_uid).await;
}
proto::message_out::Data::SignResult(_) => {
sign_completed = true;
proto::message_out::Data::SignResult(res) => {
result = Some(res.clone());
println!("party [{}] sign finished!", my_display_name);
break;
}
Expand All @@ -161,8 +163,11 @@ impl Party for TofndParty {
),
};
}
assert!(sign_completed, "sign failure to complete");
// fail test if socket was closed before I received the result
assert!(result.is_some(), "sign failure to complete");
println!("party [{}] sign execution complete", my_display_name);

result.unwrap().clone() // it's safe to unwrap here
}

async fn shutdown(mut self) {
Expand Down