test manipulated bit multiplication fails verification #1181
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1181 +/- ##
==========================================
+ Coverage 92.01% 92.05% +0.03%
==========================================
Files 195 195
Lines 29130 29224 +94
==========================================
+ Hits 26805 26901 +96
+ Misses 2325 2323 -2 ☔ View full report in Codecov by Sentry. |
| let mut prss_left = prss.left_arr().clone(); | ||
| if ctx.role() == cheater { | ||
| prss_left += <<F as Vectorizable<N>>::Array>::from_fn(|_| F::ONE); | ||
| }; | ||
|
|
||
| let z = | ||
| multiplication_protocol(&ctx, record_id, a, b, &prss_left, prss.right_arr()).await?; |
There was a problem hiding this comment.
I find it just adds an extra level of confusion that we achieve this bit flip specifically through manipulating the prss... I would rather just mutate z after computing it.
| let segment = Segment::from_entries( | ||
| F::as_segment_entry(a.left_arr()), | ||
| F::as_segment_entry(a.right_arr()), | ||
| F::as_segment_entry(b.left_arr()), | ||
| F::as_segment_entry(b.right_arr()), | ||
| F::as_segment_entry(prss.left_arr()), | ||
| F::as_segment_entry(prss.right_arr()), | ||
| F::as_segment_entry(z.right_arr()), | ||
| ); | ||
|
|
||
| // add segment to the batch that needs to be verified by the dzkp prover and verifiers | ||
| ctx.push(record_id, segment); | ||
|
|
||
| Ok(z) | ||
| } |
There was a problem hiding this comment.
This approach leads to a lot of code duplication. Why not just invoke the normal protocol (which already does all of this segment entry stuff), and just mutate the result (apply an additive attack)?
There was a problem hiding this comment.
I agree. These tests are running against reference implementation and not intended to be formal verification methods that explore all possible states the system can be it. There are better tools than our poor in-memory infra for that.
Modelling the state of malicious adversaries does not seem very useful - there are infinite combinatorial states they can be it. The threat model allows them to delay response infinitely, respond with errors, run Byzantine attacks etc. Instead of focusing on what malicious adversaries can do, it may be easier to shift our attention to what honest helpers can see from such entities. This greatly simplifies testing imo, because there are only a few things a malicious actor can do:
- It may never respond to multiplication requests
- It can send correct values and tamper the proof
- It can send corrupted values and proof for correct values
- It can send corrupted values and proof that matches them
For the first 3 we don't need to duplicate protocols (#1202 covers 2 and 3 and 1 will be covered after #1205). I could be wrong, but test here seems to be checking 3) so we could leverage our new infrastructure capabilities and avoid copy-pasting the segment construction logic.
Both @danielmasny and @andyleiserson expressed interest in covering 4) which is an interesting exercise. I think code duplication is probably wrong for it in the long term as well - we need to figure out how to do it without cloning our MPC circuits.
| let [(_, s_1), (_, s_2), (v_3, s_3)] = world | ||
| .malicious((), |ctx, ()| async move { | ||
| let [a, b, prss] = all_combination_of_inputs(ctx.role(), i); | ||
| let validator = ctx.narrow(&TestStep::Counter(i)).dzkp_validator(10); |
There was a problem hiding this comment.
I don't know what this "10" is / does.
| let [a, b, prss] = all_combination_of_inputs(ctx.role(), i); | ||
| let validator = ctx.narrow(&TestStep::Counter(i)).dzkp_validator(10); | ||
| let mctx = validator.context(); | ||
| let product = multiply_with_cheater( |
There was a problem hiding this comment.
I think I would rather have the other two parties call the normal protocol, and only one call multiply_with_cheater. I realize that only the person with the correct role is going to actually cheat, but this seems riskier than just testing the verifiers running the normal code.
| .await; | ||
|
|
||
| // H1 cheats means H3 fails | ||
| // since always verifier on the left of the cheating prover fails |
There was a problem hiding this comment.
This makes zero sense to me. I cannot think of any reason why just one verifier should fail.
Both verifiers should be checking that all of the b shares sum to zero. If it fails for one, it should fail for both.
There was a problem hiding this comment.
lets sync on this once you are back next week.
No description provided.