feat: garble vm #191
feat: garble vm #191
Conversation
There was a problem hiding this comment.
Good work.
I left some qs, including security-critical ones.
@sinui0
| R: Send + 'static, | ||
| W: Fn(&T) -> usize + Send + 'static, | ||
| { | ||
| let item_count = items.len(); |
There was a problem hiding this comment.
Do we want to return an early error here if item_count == 0 ?
| @@ -200,6 +222,27 @@ impl<Io> MTContext<Io> { | |||
| .first() | |||
| .expect("child thread should be available")) | |||
| } | |||
|
|
|||
| async fn get_children(&mut self, count: usize) -> Result<&[Handle<Self>], ContextError> { | |||
There was a problem hiding this comment.
Is this fn allowed to return more children than count? If so, could we add a comment, otherwise such behaviour looks like an error.
| // | ||
| // Only prove MACs for output data and evaluator's inputs. | ||
| let provable_input = match self.role { | ||
| Role::Generator => input - self.vis.visible(), |
There was a problem hiding this comment.
Is there a reason why this not input.intersection(self.vis.blind()) like above?
| idx_outputs |= output.to_range(); | ||
| true | ||
| } else { | ||
| false |
There was a problem hiding this comment.
Shouldn't we also do
idx_outputs |= output.to_range();
before returning false?
There was a problem hiding this comment.
I agree, makes sense and I tested it. Without this fix the vm preprocess call hangs. With this fix it proceeds.
There was a problem hiding this comment.
Please open a PR if you've identified a fix for a bug
| .scope_boxed() | ||
| }); | ||
|
|
||
| while !self.call_stack.is_empty() { |
There was a problem hiding this comment.
Since the loop will end up draining the entire callstack, what is the purpose of receiving the garbled circuits layer-by-layer? We can just receive the garbled circuits for the entirety of the callstack in one go.
There was a problem hiding this comment.
It's because we process them in parallel and this ensures no interdependencies to facilitate that. The generator is streaming the gates over separate IO threads
| }); | ||
| } | ||
|
|
||
| let expected = hasher.tccr(Block::from((id as u128).to_be_bytes()), *mac.as_block()); |
There was a problem hiding this comment.
I think we should use a strong hash here. Fixed-key AES TCCR was not proven to have strong hash properties like e.g. collision resistance.
|
|
||
| /// Adjusts the truth value of the corresponding MAC. | ||
| #[inline] | ||
| pub fn adjust(&mut self, adjust: bool, delta: &Delta) { |
There was a problem hiding this comment.
I had a hard time parsing what this method tries to achieve. Could you describe the adjust argument?
| #[inline] | ||
| pub fn new(delta: Delta) -> Self { | ||
| let mut public_one = Key(MAC_ONE ^ delta.as_block()); | ||
| public_one.0.set_lsb(false); |
There was a problem hiding this comment.
iiuc, at this point the LSB will always be 0, why do we need to set it here?
MAC_ONE's LSB is always 1, delta's LSB is always 1 and so the Key's LSB will always be zero.
|
|
||
| /// Allocates memory with the given keys. | ||
| /// | ||
| /// The provided keys are marked as used. |
There was a problem hiding this comment.
The keys are not marked as used here.
This PR implements a semi-honest GC VM, including a lot of the core functionality which can be applied to implement the coming quicksilver VM.