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

Fewer calls to narrow and fewer steps to mess up compact gate #1056

Merged
merged 5 commits into from
May 13, 2024
Merged
Changes from 2 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
23 changes: 14 additions & 9 deletions ipa-core/src/protocol/ipa_prf/aggregation/bucket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,13 @@ use crate::{
error::Error,
ff::boolean::Boolean,
helpers::repeat_n,
protocol::{
basics::SecureMul, boolean::and::bool_and, context::Context,
ipa_prf::prf_sharding::BinaryTreeDepthStep, RecordId,
},
protocol::{basics::SecureMul, boolean::and::bool_and, context::Context, RecordId},
secret_sharing::{replicated::semi_honest::AdditiveShare, BitDecomposed, FieldSimd},
};

#[derive(Step)]
pub enum BucketStep {
#[dynamic(256)]
#[dynamic(512)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this MAX_BREAKDOWNS, and if so, can we use the constant here? Or if that will confuse the proc macro, at least document the association somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this should match MAX_BREAKDOWNS, but I can't use a constant here:

ipa_macros::step "dynamic" attribute expects a number of steps (<= 1024) in parentheses: #[dynamic(...)].rustc[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic%20message%20%5B0%5D?0#file%3A%2F%2F%2FUsers%2Fbtsavage%2Fprojects%2Fipa-rust%2Fipa-core%2Fsrc%2Fprotocol%2Fipa_prf%2Faggregation%2Fbucket.rs)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It does not seem to be supported yet? rust-lang/rust#52393

Copy link
Collaborator

Choose a reason for hiding this comment

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

A comment, or moving the definition of MAX_BREAKDOWNS next to the step definition, or doing const_assert_eq!(MAX_BREAKDOWNS, 512) next to the step definition, would be fine.

Bit(usize),
}

Expand Down Expand Up @@ -107,19 +104,26 @@ where

let mut row_contribution = vec![value; breakdown_count];

for (tree_depth, bit_of_bdkey) in bd_key.iter().enumerate().rev() {
// To move a value to one of 2^bd_key_bits buckets requires 2^bd_key_bits - 1 multiplications
// They happen in a tree like fashion:
// 1 multiplication for the first bit
// 2 for the second bit
// 4 for the 3rd bit
// And so on. Simply ordering them sequentially is a functional way
// of enumerating them without creating more step transitions than necessary
let mut multiplication_channel = 0;

for bit_of_bdkey in bd_key.iter().rev() {
let span = step >> 1;
if !robust && span > breakdown_count {
step = span;
continue;
}

let depth_c = ctx.narrow(&BinaryTreeDepthStep::from(tree_depth));

let contributions = ctx
.parallel_join((0..breakdown_count).step_by(step).enumerate().filter_map(
|(i, tree_index)| {
let bucket_c = depth_c.narrow(&BucketStep::from(i));
let bucket_c = ctx.narrow(&BucketStep::from(multiplication_channel + i));

let index_contribution = &row_contribution[tree_index];

Expand All @@ -134,6 +138,7 @@ where
},
))
.await?;
multiplication_channel += contributions.len();

for (index, bdbit_contribution) in contributions.into_iter().enumerate() {
let left_index = index * step;
Expand Down
Loading