Skip to content

Commit

Permalink
Merge pull request #963 from andyleiserson/debug-assertions
Browse files Browse the repository at this point in the history
Turn off debug assertions in benchmark configs; enable strings as steps based on `cfg(test)`
  • Loading branch information
andyleiserson authored Mar 7, 2024
2 parents 951bff3 + c2cbeec commit 603cebe
Show file tree
Hide file tree
Showing 8 changed files with 38 additions and 23 deletions.
4 changes: 0 additions & 4 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,8 @@ members = ["ipa-core", "ipa-macros"]
incremental = true
lto = "thin"

[profile.bench]
debug-assertions = true

[profile.bench-dhat]
inherits = "bench"
debug-assertions = false
incremental = true
lto = "thin"
debug = 1
9 changes: 4 additions & 5 deletions ipa-core/src/ff/curve_points.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,11 +197,7 @@ mod test {
use typenum::U32;

use crate::{
ff::{
curve_points::{NonCanonicalEncoding, RP25519},
ec_prime_field::Fp25519,
Serializable,
},
ff::{curve_points::RP25519, ec_prime_field::Fp25519, Serializable},
secret_sharing::SharedValue,
};

Expand Down Expand Up @@ -258,7 +254,10 @@ mod test {
}

#[test]
#[cfg(debug_assertions)]
fn non_canonical() {
use crate::ff::curve_points::NonCanonicalEncoding;

const ZERO: u128 = 0;
// 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF is not a valid Ristretto point
let buf: [u8; 32] = unsafe { std::mem::transmute([!ZERO, !ZERO]) };
Expand Down
6 changes: 6 additions & 0 deletions ipa-core/src/ff/galois_field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -530,8 +530,14 @@ macro_rules! bit_array_impl {
}

#[test]
#[cfg(debug_assertions)]
#[should_panic(expected = "index < usize::try_from")]
pub fn out_of_count_index() {
// With debug assertions enabled, this test will panic on any out-of-bounds
// access. Without debug assertions, it will not panic on access to the unused
// bits for non-multiple-of-8 bitwidths. Enable the test only with debug
// assertions, rather than try to do something conditioned on the bit width.

let s = $name::try_from(1_u128).unwrap();
// Below assert doesn't matter. The indexing should panic
assert_eq!(s[<$name>::BITS as usize], false);
Expand Down
6 changes: 3 additions & 3 deletions ipa-core/src/protocol/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ mod tests {
telemetry::metrics::{
BYTES_SENT, INDEXED_PRSS_GENERATED, RECORDS_SENT, SEQUENTIAL_PRSS_GENERATED,
},
test_fixture::{Reconstruct, Runner, TestWorld, TestWorldConfig},
test_fixture::{Reconstruct, Runner, TestExecutionStep, TestWorld, TestWorldConfig},
};

trait ReplicatedLeftValue<F: Field> {
Expand Down Expand Up @@ -391,7 +391,7 @@ mod tests {
let input_size = input.len();
let snapshot = world.metrics_snapshot();
let metrics_step = Gate::default()
.narrow(&TestWorld::execution_step(0))
.narrow(&TestExecutionStep::Iter(0))
.narrow("metrics");

// for semi-honest protocols, amplification factor per helper is 1.
Expand Down Expand Up @@ -448,7 +448,7 @@ mod tests {
.await;

let metrics_step = Gate::default()
.narrow(&TestWorld::execution_step(0))
.narrow(&TestExecutionStep::Iter(0))
// TODO: leaky abstraction, test world should tell us the exact step
.narrow(&MaliciousProtocol)
.narrow("metrics");
Expand Down
6 changes: 4 additions & 2 deletions ipa-core/src/protocol/step/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@ pub trait StepNarrow<S: Step + ?Sized> {
pub trait Step: AsRef<str> {}

// In test code, allow a string (or string reference) to be used as a `Step`.
#[cfg(any(feature = "test-fixture", debug_assertions))]
// Note: Since the creation of the `derive(Step)` macro, hardly any code is
// required to define a step. Doing so is highly encouraged, even in tests.
#[cfg(test)]
impl Step for String {}

#[cfg(any(feature = "test-fixture", debug_assertions))]
#[cfg(test)]
impl Step for str {}

/// A step generator for bitwise secure operations.
Expand Down
9 changes: 8 additions & 1 deletion ipa-core/src/test_fixture/circuit.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::{array, num::NonZeroUsize};

use futures::{future::join3, stream, StreamExt};
use ipa_macros::Step;
use rand::distributions::{Distribution, Standard};

use crate::{
Expand Down Expand Up @@ -121,6 +122,12 @@ pub async fn arithmetic<F, const N: usize>(
assert_eq!(sum, u128::from(width));
}

#[derive(Step)]
enum Step {
#[dynamic(1024)]
Stripe(usize),
}

async fn circuit<'a, F, const N: usize>(
ctx: SemiHonestContext<'a>,
record_id: RecordId,
Expand All @@ -134,7 +141,7 @@ where
{
assert_eq!(b.len(), usize::from(depth));
for (stripe_ix, stripe) in b.iter().enumerate() {
let stripe_ctx = ctx.narrow(&format!("s{stripe_ix}"));
let stripe_ctx = ctx.narrow(&Step::Stripe(stripe_ix));
a = a.multiply(stripe, stripe_ctx, record_id).await.unwrap();
}

Expand Down
2 changes: 1 addition & 1 deletion ipa-core/src/test_fixture/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use rand::{distributions::Standard, prelude::Distribution, rngs::mock::StepRng};
use rand_core::{CryptoRng, RngCore};
pub use sharing::{get_bits, into_bits, Reconstruct, ReconstructArr};
#[cfg(feature = "in-memory-infra")]
pub use world::{Runner, TestWorld, TestWorldConfig};
pub use world::{Runner, TestExecutionStep, TestWorld, TestWorldConfig};

use crate::{
ff::{Field, U128Conversions},
Expand Down
19 changes: 12 additions & 7 deletions ipa-core/src/test_fixture/world.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::{fmt::Debug, io::stdout, iter::zip};

use async_trait::async_trait;
use futures::{future::join_all, Future};
use ipa_macros::Step;
use rand::{distributions::Standard, prelude::Distribution, rngs::StdRng};
use rand_core::{RngCore, SeedableRng};
use tracing::{Instrument, Level, Span};
Expand Down Expand Up @@ -31,6 +32,14 @@ use crate::{
},
};

// This is used by the metrics tests in `protocol::context`. It otherwise would/should not be pub.
#[derive(Step)]
pub enum TestExecutionStep {
/// Provides a unique per-iteration context in tests.
#[dynamic(1024)]
Iter(usize),
}

/// Test environment for protocols to run tests that require communication between helpers.
/// For now the messages sent through it never leave the test infra memory perimeter, so
/// there is no need to associate each of them with `QueryId`, but this API makes it possible
Expand Down Expand Up @@ -139,7 +148,7 @@ impl TestWorld {
zip(&self.participants, &self.gateways)
.map(|(participant, gateway)| {
SemiHonestContext::new(participant, gateway)
.narrow(&Self::execution_step(execution))
.narrow(&TestExecutionStep::Iter(execution))
})
.collect::<Vec<_>>()
.try_into()
Expand All @@ -155,7 +164,8 @@ impl TestWorld {
let execution = self.executions.fetch_add(1, Ordering::Relaxed);
zip(&self.participants, &self.gateways)
.map(|(participant, gateway)| {
MaliciousContext::new(participant, gateway).narrow(&Self::execution_step(execution))
MaliciousContext::new(participant, gateway)
.narrow(&TestExecutionStep::Iter(execution))
})
.collect::<Vec<_>>()
.try_into()
Expand All @@ -167,11 +177,6 @@ impl TestWorld {
self.metrics_handle.snapshot()
}

#[must_use]
pub fn execution_step(execution: usize) -> String {
format!("run-{execution}")
}

pub fn gateway(&self, role: Role) -> &Gateway {
&self.gateways[role]
}
Expand Down

0 comments on commit 603cebe

Please sign in to comment.