Audit/final commit #1191
Audit/final commit #1191
Conversation
style: added comment
style: removed unused variable
fix: fixed L-05
style: cleaner computation nb BSB commitments
style: fixed typos
fix: fixed l-02
fix: fixed M-04
There was a problem hiding this comment.
Looks good to me. I have two suggestions essentially, one is just a something nice to have but the second one imo would introduce an invalid comment. But I'm not sure if we can fix it as you changes are already approved.
However, I don't know what to do with the merge conflict - it there seem to have already modifications in the PLONK template and maybe it would apply nicely. In any case I think it needs to be resolved manually because otherwise the merge would mess up the PLONK Solidity template.
| @@ -222,6 +223,17 @@ contract PlonkVerifier { | |||
| revert(ptError, 0x64) | |||
| } | |||
|
|
|||
| /// Called when an operation on Bn254 fails | |||
| /// @dev for instance when calling EcMul on a point not on Bn254. | |||
There was a problem hiding this comment.
Comment invalid? The error is for mod exp, but comment is about EcMul?
There was a problem hiding this comment.
right it's a mistake
| {{ range $index, $element := .CommitmentConstraintIndexes}} | ||
| mstore(add(mPtr, {{ hex (add 544 (mul $index 64)) }}), VK_QCP_{{ $index }}_X) | ||
| mstore(add(mPtr, {{ hex (add 576 (mul $index 64)) }}), VK_QCP_{{ $index }}_Y) | ||
| mstore(add(mPtr, {{ hex (add $offset (mul $index 64)) }}), VK_QCP_{{ $index }}_X) |
There was a problem hiding this comment.
I would do similarly as before {{ $offset = add $offset 0x20 }} and then use $offset in the next line.
| @@ -73,7 +71,7 @@ func UnmarshalSolidity(s []byte, nbCommits int) Proof { | |||
| // uint256[] selector_commit_api_at_zeta; | |||
| // uint256[] wire_committed_commitments; | |||
| for i := 0; i < nbCommits; i++ { | |||
| proof.BatchedProof.ClaimedValues[7+i].SetBytes(s[offset : offset+fr_size]) | |||
| proof.BatchedProof.ClaimedValues[6+i].SetBytes(s[offset : offset+fr_size]) | |||
There was a problem hiding this comment.
if I'm not mistaken, UnmarshalSolidity is untested in our codebase --> can you add a simple round trip test with the MarshalSolidity func to ensure non regression / offset errors ?
There was a problem hiding this comment.
so I discussed a bit with OZ about that, in fact UnmarshalSolidity was used for debugging only (at the time there was an issue on testnet and I had to check the proof that was given by linea). At the time MarshalSolidity serialised all the elements of the proof. Now there has been an optimisation such that we don't serialise the opening of the linearised polynomial, it is recomputed in the verifier. To recompute this value we need the public inputs. So UnmarshalSolidity would have a weird signature (we would need to pass the public inputs). I told OZ that this function should be used internally for debugging only, I don't think we should keep it in the repo
Description
Final commit fixing Openzeppelin findings
Type of change
Checklist:
golangci-lintdoes not output errors locally