-
Notifications
You must be signed in to change notification settings - Fork 23
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
[Sort Multi Bit] Upgrade to malicious + semi honest ipa now calls multi bit sort #396
Conversation
b8bf517
to
1bc381a
Compare
This will have some small conflicts with #387. https://github.com/andyleiserson/ipa/commits/multi_bit may be useful for resolving them. Commits b80bfe4 and 173ac7b are the substantive changes, and the commit between those two is a merge with the conflicts resolved. (One thing I am doing in #387 is removing the MaliciousUpgradeInput step. Since each of the upgrades is done with a separate MaliciousValidator, it's not necessary to narrow the context used for the upgrade -- an upgrade context is created as part of the malicious validator creation.) |
@@ -274,7 +276,7 @@ where | |||
assert_eq!(sort_keys.len(), num_bits as usize); | |||
|
|||
let upgraded_sort_keys = m_ctx | |||
.upgrade_vector(&MaliciousUpgradeInput, sort_keys[0].clone()) | |||
.upgrade_vector(&MaliciousUpgradeInput(0), sort_keys[0].clone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there's a reason these functions take a reference to sort_keys
. It looks like the callers have an owned Vec anyways, and the upgrade functions want to take the raw input by value, so it may be possible to change the argument type to this function and get rid of this clone and several like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Callers own the vector but it is used in the next step after sorting. We use the sort keys to sort and then use them to apply sort on combined_match_keys_and_sidecar_data.
Great to see upgrade_vector go away! The only reason I included (0) was to run it across bits in malicious_multi
Let me wait for you to land the code and then I will make the changes. Since there might be some code-restructure I can re-use from your PR |
@@ -274,7 +276,7 @@ where | |||
assert_eq!(sort_keys.len(), num_bits as usize); | |||
|
|||
let upgraded_sort_keys = m_ctx | |||
.upgrade_vector(&MaliciousUpgradeInput, sort_keys[0].clone()) | |||
.upgrade_vector(&MaliciousUpgradeInput(0), sort_keys[0].clone()) | |||
.await?; | |||
let bit_0_permutation = | |||
bit_permutation(m_ctx_0.narrow(&BitPermutationStep), &upgraded_sort_keys).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't you be removing this?
let mut malicious_validator = MaliciousValidator::new(sh_ctx.narrow(&MaliciousUpgradeContext)); | ||
let mut m_ctx = malicious_validator.context(); | ||
let m_ctx_0 = m_ctx.narrow(&Sort(0)); | ||
assert_eq!(sort_keys.len(), num_bits as usize); | ||
|
||
let last_bit_num = std::cmp::min(num_multi_bits, num_bits); | ||
|
||
let upgraded_sort_keys = try_join_all((0..last_bit_num).zip(repeat(m_ctx.clone())).map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of duplicate code here between this and the semi-honest version. This introduces the risk of a bug when they get out of sync.
let (randoms_for_shuffle0, randoms_for_shuffle1, revealed) = ( | ||
revealed_and_random_permutations | ||
.randoms_for_shuffle | ||
.0 | ||
.as_slice(), | ||
revealed_and_random_permutations | ||
.randoms_for_shuffle | ||
.1 | ||
.as_slice(), | ||
revealed_and_random_permutations.revealed.as_slice(), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to do this destructuring when revealed_and_random_permutations
is originally created back up on line 184?
1bc381a
to
98fabe4
Compare
let upgraded_sort_keys = try_join_all( | ||
(0..last_bit_num) | ||
.zip(repeat(m_ctx.clone())) | ||
.map(|(i, m_ctx)| async move { m_ctx.upgrade(sort_keys[i as usize].clone()).await }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andyleiserson this still will fail since we are using the same context across bits for the same record with the error "Generated randomness for index '0' twice using the same key 'protocol/run-0/sort/malicious_upgrade_context/upgrade_input' ". Any ideas how to resolve this without narrowing context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking (based on an earlier revision of this PR) was to add an upgrade impl for Vec<Vec<Replicated>>
and then use that to upgrade everything at once rather than doing multiple upgrades.
Does that make sense? I'm not sure if something changed since I looked at it or if there's some other reason I missed that that won't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code was earlier written to just work on one vector at a time and so I am doing some refactoring to let the sort code take in vec<vec<vec>> so that each vec<vec> can be processed in one iteration
) -> Result<Vec<Replicated<F>>, Error> | ||
where | ||
F: Field, | ||
{ | ||
let mut malicious_validator = None; | ||
let sort_ctx: &dyn Context<F> = if malicious_setting { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akoshelev need your help in checking how to make this work. It would be nice to have malicious and semi honest sort work from the same function since we can avoid duplication and mistakes in upgrades by this approach.
Code still does not compile because we need share upgrade
let malicious_validator = Some(MaliciousValidator::new( | ||
ctx.narrow(&MaliciousUpgradeContext), | ||
)); | ||
let ctx = malicious_validator.unwrap().context(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not:
let malicious_validator = Some(MaliciousValidator::new( | |
ctx.narrow(&MaliciousUpgradeContext), | |
)); | |
let ctx = malicious_validator.unwrap().context(); | |
let malicious_validator = MaliciousValidator::new( | |
ctx.narrow(&MaliciousUpgradeContext), | |
); | |
let ctx = malicious_validator.context(); |
You added an Option<T>
wrapper and then unwrapped it immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were trying to combine the two sorts i.e. malicious and semi honest in the same function hence introduced an option for malicious_validator. Will revisit later
Closing this since this needs a bit of a refactor to use upgrade on a slice of num_multi_bits per record |
No description provided.