diff --git a/ipa-core/benches/ct/dzkp.rs b/ipa-core/benches/ct/dzkp.rs index 2e5b87997..294d45af1 100644 --- a/ipa-core/benches/ct/dzkp.rs +++ b/ipa-core/benches/ct/dzkp.rs @@ -66,7 +66,7 @@ fn benchmark_proof(c: &mut Criterion) { (a, b) }, |(a, b): (Vec, Vec)| async move { - TestWorld::default() + let _ = TestWorld::default() .malicious((a.into_iter(), b.into_iter()), |ctx, (a, b)| async move { let batch_size = non_zero_prev_power_of_two( TARGET_PROOF_SIZE / usize::try_from(BA::BITS).unwrap(), diff --git a/ipa-core/src/bin/helper.rs b/ipa-core/src/bin/helper.rs index e82598dec..9e3f14f46 100644 --- a/ipa-core/src/bin/helper.rs +++ b/ipa-core/src/bin/helper.rs @@ -291,7 +291,7 @@ async fn server(args: ServerArgs, logging_handle: LoggingHandle) -> Result<(), B join(server_handle, shard_server_handle).await; - [query_runtime, http_runtime].map(Runtime::shutdown_background); + let _ = [query_runtime, http_runtime].map(Runtime::shutdown_background); Ok(()) } diff --git a/ipa-core/src/bin/ipa_bench/models.rs b/ipa-core/src/bin/ipa_bench/models.rs index 04256d27c..bb8e9bbfc 100644 --- a/ipa-core/src/bin/ipa_bench/models.rs +++ b/ipa-core/src/bin/ipa_bench/models.rs @@ -133,19 +133,6 @@ pub enum GenericReport { }, } -#[derive(Serialize, Deserialize)] -enum QueryType { - SourceFanout, - TriggerFanout, -} - -#[derive(Serialize, Deserialize)] -enum Node { - Helper1, - Helper2, - Helper3, -} - #[cfg(all(test, unit_test))] mod tests { use super::{Epoch, EventTimestamp}; diff --git a/ipa-core/src/cli/clientconf.rs b/ipa-core/src/cli/clientconf.rs index 453eb2f09..67104d7f0 100644 --- a/ipa-core/src/cli/clientconf.rs +++ b/ipa-core/src/cli/clientconf.rs @@ -63,7 +63,7 @@ pub fn setup(args: ConfGenArgs) -> Result<(), BoxError> { .map(|(id, (host, (port, shard_port)))| { let id: u8 = u8::try_from(id).unwrap() + 1; HelperClientConf { - host: host.to_string(), + host: host.clone(), port, shard_port, tls_cert_file: args.keys_dir.helper_tls_cert(id), diff --git a/ipa-core/src/cli/config_parse.rs b/ipa-core/src/cli/config_parse.rs index 1efdf0e67..26e8068c1 100644 --- a/ipa-core/src/cli/config_parse.rs +++ b/ipa-core/src/cli/config_parse.rs @@ -361,8 +361,8 @@ pub fn sharded_server_from_toml_str( let url = myself.url.to_string(); let pos = url.rfind(':'); let port = shard_port.expect("Shard port should be set"); - let new_url = if pos.is_some() { - format!("{}{port}", &url[..=pos.unwrap()]) + let new_url = if let Some(pos) = pos { + format!("{}{port}", &url[..=pos]) } else { format!("{}:{port}", &url) }; @@ -374,7 +374,7 @@ pub fn sharded_server_from_toml_str( }; Ok((mpc_network, shard_network)) } else { - return Err(Error::MissingShardUrls(missing_urls)); + Err(Error::MissingShardUrls(missing_urls)) } } diff --git a/ipa-core/src/cli/keygen.rs b/ipa-core/src/cli/keygen.rs index a0ddec88f..dd3afbdb5 100644 --- a/ipa-core/src/cli/keygen.rs +++ b/ipa-core/src/cli/keygen.rs @@ -103,11 +103,11 @@ pub fn keygen_tls(args: &KeygenArgs, rng: &mut R) -> Result< fn keygen_matchkey(args: &KeygenArgs, mut rng: &mut R) -> Result<(), BoxError> { let keypair = crate::hpke::KeyPair::r#gen(&mut rng); - if args.mk_public_key.is_some() && args.mk_private_key.is_some() { - create_new(args.mk_public_key.as_ref().unwrap())? - .write_all(hex::encode(keypair.pk_bytes()).as_bytes())?; - create_new(args.mk_private_key.as_ref().unwrap())? - .write_all(hex::encode(keypair.sk_bytes()).as_bytes())?; + if let (Some(mk_public_key), Some(mk_private_key)) = + (args.mk_public_key.as_ref(), args.mk_private_key.as_ref()) + { + create_new(mk_public_key)?.write_all(hex::encode(keypair.pk_bytes()).as_bytes())?; + create_new(mk_private_key)?.write_all(hex::encode(keypair.sk_bytes()).as_bytes())?; } Ok(()) diff --git a/ipa-core/src/cli/metric_collector.rs b/ipa-core/src/cli/metric_collector.rs index 2cf678f6c..e1fae888d 100644 --- a/ipa-core/src/cli/metric_collector.rs +++ b/ipa-core/src/cli/metric_collector.rs @@ -8,9 +8,9 @@ use tokio::runtime::Builder; /// Holds a reference to metrics controller and producer pub struct CollectorHandle { - thread_handle: JoinHandle<()>, + thread_handle: Option>, /// This will be used once we start consuming metrics - controller: MetricsCollectorController, + controller: Option, producer: MetricsProducer, } @@ -26,16 +26,23 @@ pub fn install_collector() -> io::Result { tracing::info!("Metrics engine is enabled"); Ok(CollectorHandle { - thread_handle: handle, - controller, + thread_handle: Some(handle), + controller: Some(controller), producer, }) } impl Drop for CollectorHandle { fn drop(&mut self) { - if !thread::panicking() && !self.thread_handle.is_finished() { - tracing::warn!("Metrics thread is still running"); + if thread::panicking() { + return; // avoid potential deadlock during panic unwind + } + // Drop controller first to disconnect the command channel. + // This causes the collector thread's event_loop to exit. + drop(self.controller.take()); + // Wait for the collector thread to finish. + if let Some(handle) = self.thread_handle.take() { + let _ = handle.join(); } } } @@ -61,7 +68,12 @@ impl CollectorHandle { /// If metrics is not initialized #[must_use] pub fn scrape_metrics(&self) -> Vec { - let mut store = self.controller.snapshot().expect("Metrics must be set up"); + let mut store = self + .controller + .as_ref() + .expect("Metrics must be set up") + .snapshot() + .expect("Metrics snapshot failed"); let mut buff = Vec::new(); store.export(&mut buff); diff --git a/ipa-core/src/cli/playbook/mod.rs b/ipa-core/src/cli/playbook/mod.rs index 513a91cc9..95cb5bdb0 100644 --- a/ipa-core/src/cli/playbook/mod.rs +++ b/ipa-core/src/cli/playbook/mod.rs @@ -222,7 +222,7 @@ pub async fn make_clients( // Note: This closure is only called when the selected action uses clients. let clients = IpaHttpClient::from_conf(&IpaRuntime::current(), &network, &ClientIdentity::None); - wait_for_servers(wait, &[clients.clone()]).await; + wait_for_servers(wait, std::slice::from_ref(&clients)).await; (clients, network) } diff --git a/ipa-core/src/cli/test_setup.rs b/ipa-core/src/cli/test_setup.rs index b6ef5cdeb..3639b6f2e 100644 --- a/ipa-core/src/cli/test_setup.rs +++ b/ipa-core/src/cli/test_setup.rs @@ -174,7 +174,7 @@ fn make_client_configs( keygen(&keygen_args)?; Ok(HelperClientConf { - host: localhost.to_string(), + host: localhost.clone(), port: mpc_port, shard_port, tls_cert_file: keygen_args.tls_cert, diff --git a/ipa-core/src/error.rs b/ipa-core/src/error.rs index f9d55bcee..ee9700f8f 100644 --- a/ipa-core/src/error.rs +++ b/ipa-core/src/error.rs @@ -22,13 +22,14 @@ use crate::{ /// * `ipa::ff::Error`, for finite field routines /// * `ipa::net::Error`, for the HTTP transport /// * `ipa::app::Error`, for the report collector query APIs -#[derive(Error, Debug)] +#[derive(Error, Debug, Default)] pub enum Error { #[error("already exists")] AlreadyExists, #[error("already setup")] AlreadySetup, #[error("internal")] + #[default] Internal, #[error("invalid id found: {0}")] InvalidId(String), @@ -106,12 +107,6 @@ pub enum Error { DuplicateBytes(usize), } -impl Default for Error { - fn default() -> Self { - Self::Internal - } -} - impl Error { #[must_use] pub fn path_parse_error(source: &str) -> Error { diff --git a/ipa-core/src/helpers/buffers/ordering_sender.rs b/ipa-core/src/helpers/buffers/ordering_sender.rs index f69288a49..73df74500 100644 --- a/ipa-core/src/helpers/buffers/ordering_sender.rs +++ b/ipa-core/src/helpers/buffers/ordering_sender.rs @@ -9,7 +9,7 @@ use std::{ task::{Context, Poll}, }; -use futures::{Future, Stream, task::Waker}; +use futures::{Future, task::Waker}; use crate::{ helpers::{Message, buffers::circular::CircularBuf}, @@ -493,11 +493,13 @@ impl Future for Close<'_> { /// the next stream that happens to be polled. Ordinarily streams require a /// mutable reference so that they have exclusive access to the underlying state. /// To avoid that happening, don't make more than one stream. +#[cfg(all(test, any(unit_test, feature = "shuttle")))] pub struct OrderedStream> { sender: B, } -impl + Unpin> Stream for OrderedStream { +#[cfg(all(test, any(unit_test, feature = "shuttle")))] +impl + Unpin> futures::Stream for OrderedStream { type Item = Vec; fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { diff --git a/ipa-core/src/helpers/stream/chunks.rs b/ipa-core/src/helpers/stream/chunks.rs index 4f7d150ee..5df92c8de 100644 --- a/ipa-core/src/helpers/stream/chunks.rs +++ b/ipa-core/src/helpers/stream/chunks.rs @@ -277,7 +277,7 @@ where ChunkType::Partial(remainder_len), )) } else { - return None; + None } } } diff --git a/ipa-core/src/helpers/transport/handler.rs b/ipa-core/src/helpers/transport/handler.rs index b36a1e3b4..1da76dd2f 100644 --- a/ipa-core/src/helpers/transport/handler.rs +++ b/ipa-core/src/helpers/transport/handler.rs @@ -177,7 +177,7 @@ pub enum Error { } /// Trait for custom-handling different request types made against MPC helper parties. -/// There is a limitation for RPITIT that traits can't be made object-safe, hence the use of async_trait +/// There is a limitation for RPITIT that traits can't be made object-safe, hence the use of `async_trait` #[async_trait] pub trait RequestHandler: Send + Sync { /// Handle the incoming request with metadata/headers specified in [`Addr`] and body encoded as diff --git a/ipa-core/src/helpers/transport/receive.rs b/ipa-core/src/helpers/transport/receive.rs index 299433a1c..4e31c1539 100644 --- a/ipa-core/src/helpers/transport/receive.rs +++ b/ipa-core/src/helpers/transport/receive.rs @@ -1,5 +1,5 @@ use std::{ - pin::{Pin, pin}, + pin::Pin, task::{Context, Poll}, }; diff --git a/ipa-core/src/net/http_serde.rs b/ipa-core/src/net/http_serde.rs index 2777cf7cb..695de27e0 100644 --- a/ipa-core/src/net/http_serde.rs +++ b/ipa-core/src/net/http_serde.rs @@ -69,12 +69,6 @@ pub mod echo { } pub mod metrics { - - use serde::{Deserialize, Serialize}; - - #[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize)] - pub struct Request {} - pub const AXUM_PATH: &str = "/metrics"; } diff --git a/ipa-core/src/net/server/mod.rs b/ipa-core/src/net/server/mod.rs index 6e4a538df..f4fc7af0e 100644 --- a/ipa-core/src/net/server/mod.rs +++ b/ipa-core/src/net/server/mod.rs @@ -670,7 +670,7 @@ mod e2e_tests { } /// Ensures that server tracks number of requests it received and emits a corresponding metric. - /// In order for this test not to be flaky, we rely on tokio::test macro to set up a + /// In order for this test not to be flaky, we rely on `tokio::test` macro to set up a /// new runtime per test (which it currently does) and set up metric recorders per thread (done /// by this test). It is also tricky to make it work in a multi-threaded environment - I haven't /// tested that, so better to stick with default behavior of tokio:test macro diff --git a/ipa-core/src/protocol/basics/reshare.rs b/ipa-core/src/protocol/basics/reshare.rs index 57a14c3d0..9f9b46bfb 100644 --- a/ipa-core/src/protocol/basics/reshare.rs +++ b/ipa-core/src/protocol/basics/reshare.rs @@ -29,8 +29,8 @@ use crate::{ /// 1. While calculating for a helper, we call pseudo random secret sharing (prss) to get random values which match /// with those generated by other helpers (say `rand_left`, `rand_right`) /// `to_helper.left` knows `rand_left` (named r1) and `to_helper.right` knows `rand_right` (named r0) -/// 2. `to_helper.left` calculates part1 = (a1 + a2) - r2 = Same as (input.left() + input.right()) - r1 from helper POV -/// `to_helper.right` calculates part2 = (a3 - r3) = Same as (input.left() - r0) from helper POV +/// 2. `to_helper.left` calculates part1 = (a1 + a2) - r2 = Same as (`input.left()` + `input.right()`) - r1 from helper POV +/// `to_helper.right` calculates part2 = (a3 - r3) = Same as (`input.left()` - r0) from helper POV /// 3. `to_helper.left` and `to_helper.right` exchange their calculated shares /// 4. Everyone sets their shares /// `to_helper.left` = (part1 + part2, `rand_left`) = (part1 + part2, r1) diff --git a/ipa-core/src/protocol/boolean/step.rs b/ipa-core/src/protocol/boolean/step.rs index 0128cb037..746de045c 100644 --- a/ipa-core/src/protocol/boolean/step.rs +++ b/ipa-core/src/protocol/boolean/step.rs @@ -19,4 +19,5 @@ pub struct TwoHundredFiftySixBitOpStep(usize); #[cfg(test)] #[derive(CompactStep)] #[step(count = 256, name = "bit")] +#[allow(dead_code)] // used as a type parameter, not constructed directly pub struct DefaultBitStep(usize); diff --git a/ipa-core/src/protocol/context/dzkp_validator.rs b/ipa-core/src/protocol/context/dzkp_validator.rs index 4ede763c0..4a732ad14 100644 --- a/ipa-core/src/protocol/context/dzkp_validator.rs +++ b/ipa-core/src/protocol/context/dzkp_validator.rs @@ -684,7 +684,7 @@ pub trait DZKPValidator: Send + Sync { fn context(&self) -> Self::Context; /// Sets the validator's total number of records field. This is required when using - /// the validate_record API, if it wasn't already set on the context used to create + /// the `validate_record` API, if it wasn't already set on the context used to create /// the validator. fn set_total_records>(&mut self, total_records: T); @@ -860,7 +860,7 @@ impl<'a, B: ShardBinding> DZKPValidator for MaliciousDZKPValidator<'a, B> { } /// `is_verified` checks that there are no `MultiplicationInputs` that have not been verified. - /// This function is called by drop() to ensure that the validator is safe to be dropped. + /// This function is called by `drop()` to ensure that the validator is safe to be dropped. /// /// ## Errors /// Errors when there are `MultiplicationInputs` that have not been verified. diff --git a/ipa-core/src/protocol/context/validator.rs b/ipa-core/src/protocol/context/validator.rs index ff5c7b7c7..41a11bd43 100644 --- a/ipa-core/src/protocol/context/validator.rs +++ b/ipa-core/src/protocol/context/validator.rs @@ -423,7 +423,7 @@ mod tests { /// This is the simplest arithmetic circuit that allows us to test all of the pieces of this validator /// A - /// \ - /// Mult_Gate -> A*B + /// `Mult_Gate` -> A*B /// / /// B - /// diff --git a/ipa-core/src/protocol/ipa_prf/boolean_ops/comparison_and_subtraction_sequential.rs b/ipa-core/src/protocol/ipa_prf/boolean_ops/comparison_and_subtraction_sequential.rs index 5df9ca0f2..ece7c4c63 100644 --- a/ipa-core/src/protocol/ipa_prf/boolean_ops/comparison_and_subtraction_sequential.rs +++ b/ipa-core/src/protocol/ipa_prf/boolean_ops/comparison_and_subtraction_sequential.rs @@ -375,7 +375,7 @@ mod test { const BENCH_COUNT: usize = 131_072; #[test] - #[ignore] // benchmark + #[ignore = "benchmark"] #[cfg(not(coverage))] fn semi_honest_compare_gt_novec() { run(|| async move { @@ -430,7 +430,7 @@ mod test { } #[test] - #[ignore] // benchmark + #[ignore = "benchmark"] #[cfg(not(coverage))] fn semi_honest_compare_gt_vec() { run(|| async move { diff --git a/ipa-core/src/protocol/ipa_prf/oprf_padding/mod.rs b/ipa-core/src/protocol/ipa_prf/oprf_padding/mod.rs index bbe092b70..6fe861e2f 100644 --- a/ipa-core/src/protocol/ipa_prf/oprf_padding/mod.rs +++ b/ipa-core/src/protocol/ipa_prf/oprf_padding/mod.rs @@ -582,7 +582,7 @@ mod tests { } #[test] - #[ignore] + #[ignore = "manual execution only"] pub fn table_of_padding_parameters() { // see output https://docs.google.com/spreadsheets/d/1N0WEUkarP_6nd-7W8O9r-Xurh9OImESgAC1Jd_6OfWw/edit?gid=0#gid=0 let epsilon_values = [0.01, 0.1, 1.0, 5.0, 10.0]; diff --git a/ipa-core/src/query/processor.rs b/ipa-core/src/query/processor.rs index c8312316c..a53f595fc 100644 --- a/ipa-core/src/query/processor.rs +++ b/ipa-core/src/query/processor.rs @@ -1128,9 +1128,9 @@ mod tests { use crate::{helpers::query::CompareStatusRequest, protocol::QueryId}; /// * From the standpoint of leader shard in Helper 1 - /// * On query_status + /// * On `query_status` /// - /// The min state should be returned. In this case, if I, as leader, am in AwaitingInputs + /// The min state should be returned. In this case, if I, as leader, am in `AwaitingInputs` /// state and shards report that they are further ahead (Completed and Running), then my /// state is returned. #[tokio::test] @@ -1188,9 +1188,9 @@ mod tests { } /// * From the standpoint of leader shard in Helper 1 - /// * On query_status + /// * On `query_status` /// - /// If one of my shards hasn't received the query yet (NoSuchQuery) the leader should + /// If one of my shards hasn't received the query yet (`NoSuchQuery`) the leader should /// return an error despite other shards returning their status #[tokio::test] #[should_panic( diff --git a/ipa-core/src/query/runner/reshard_tag.rs b/ipa-core/src/query/runner/reshard_tag.rs index d10c31dac..cb83d67ff 100644 --- a/ipa-core/src/query/runner/reshard_tag.rs +++ b/ipa-core/src/query/runner/reshard_tag.rs @@ -1,5 +1,5 @@ use std::{ - pin::{Pin, pin}, + pin::Pin, task::{Context, Poll}, }; diff --git a/ipa-core/src/test_fixture/sharing.rs b/ipa-core/src/test_fixture/sharing.rs index b42ce5c54..29aeb0e5f 100644 --- a/ipa-core/src/test_fixture/sharing.rs +++ b/ipa-core/src/test_fixture/sharing.rs @@ -212,6 +212,7 @@ impl Reconstruct<()> for [(); 3] { fn reconstruct(&self) {} } +#[allow(dead_code)] // used as a trait bound; not all feature combinations call its methods pub trait ValidateMalicious { fn validate(&self, r: F::ExtendedField); } diff --git a/ipa-step-test/src/lib.rs b/ipa-step-test/src/lib.rs index 3789e6fcf..8ea549fd0 100644 --- a/ipa-step-test/src/lib.rs +++ b/ipa-step-test/src/lib.rs @@ -1,5 +1,8 @@ +#[cfg(test)] mod basic_step; +#[cfg(test)] mod complex_step; +#[cfg(test)] mod module; #[cfg(test)]