fixing a bug for rescue/solidity transcript #70

zhenfeizhang · 2022-05-23T19:18:43Z

Description

closes: #XXXX

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (main)
Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
Wrote unit tests
Updated relevant documentation in the code
Added a relevant changelog entry to the Pending section in CHANGELOG.md
Re-reviewed Files changed in the GitHub PR explorer

CLAassistant · 2022-06-29T23:10:35Z

All committers have signed the CLA.

alxiong · 2022-12-13T15:54:16Z

this is a stale PR. iirc, the original motivation was just defense in depth in the light of trailofbits' disclosure on FrozenHeart where some FS transcripts forget taking in all the public inputs (especially the vk), but our library has always hashed all PI into our transcript, and adding a domain separator (the label) for each field may not have a security impact, just good-to-have.

Adding this to SolidityTranscript will increase the gas cost for PlonkVerifier.

I personally don't find this necessary from security perspective, am I missing anything?

Just want to bring this out for discussion. @EspressoSystems/jellyfish

mrain · 2022-12-16T15:22:30Z

Is there any reference for old discussion about this issue?

Also is the gas cost increase going to be significant if we add those domain separators? @alxiong

alxiong · 2022-12-19T16:47:47Z

Is there any reference for old discussion about this issue?

Sorry, I can't recall. @chancharles92 do you have anything to add beyond my comments above?

Also is the gas cost increase going to be significant if we add those domain separators?

yes, it's non-neglible cuz we are hashing using rescue which is expensive in Solidity (in absolute gas cost, but somewhat manageable in relative gas cost), the more pressing problem is the contract bytecode size will increase. I remember we have a lots of trouble trying to keep CAPE contract under size limit.

chancharles92 · 2022-12-19T17:20:52Z

Is there any reference for old discussion about this issue?

Sorry, I can't recall. @chancharles92 do you have anything to add beyond my comments above?

My impression is that Zhenfei mentioned that our SNARK/Circuit transcript implementations are secure, but I don't remember clearly whether the solidity transcript is correct or not.

chancharles92 · 2023-01-17T16:08:12Z

Found the context here: https://www.notion.so/espressosys/Potential-vulnerability-on-Jellyfish-s-transcript-91e810dd59c849aba7bab4ef9baaffb9 @mrain @alxiong

TL; DR: it's not a bug in Jellyfish because Jellyfish code has an implicit data length field. But for general usage (e.g. if some external users want to use it and want to support dynamic data length fields), it'll be better to patch this PR.

…nto bug-disclaimer

alxiong · 2023-01-18T07:29:52Z

Thanks for finding the notion page. I have a question, so it's probably a good idea to fix "rescue transcript". But is it necessary to also add label to "solidity transcript"? The scenario described where 3 BN254 points turn out to be identical to 2 BLS12-381 points are not possible in our CAPE contract.

Also I remember Aztec's plonk verifier contract doesn't add label, neither does matter-lab.

chancharles92 · 2023-01-18T14:00:17Z

Thanks for finding the notion page. I have a question, so it's probably a good idea to fix "rescue transcript". But is it necessary to also add label to "solidity transcript"? The scenario described where 3 BN254 points turn out to be identical to 2 BLS12-381 points are not possible in our CAPE contract.

Also I remember Aztec's plonk verifier contract doesn't add label, neither does matter-lab.

Yes, check TL; DR. We didn't need to merge this PR now. OTOH, fixing the "rescue transcript" also has some costs because the SNARK circuit will have more constraints.

mrain · 2023-01-18T15:28:09Z

Thanks for finding the notion page. I have a question, so it's probably a good idea to fix "rescue transcript". But is it necessary to also add label to "solidity transcript"? The scenario described where 3 BN254 points turn out to be identical to 2 BLS12-381 points are not possible in our CAPE contract.
Also I remember Aztec's plonk verifier contract doesn't add label, neither does matter-lab.

Yes, check TL; DR. We didn't need to merge this PR now. OTOH, fixing the "rescue transcript" also has some costs because the SNARK circuit will have more constraints.

I pushed a commit for fixing the rescue transcript. Please check @alxiong @chancharles92

chancharles92 · 2023-01-18T15:35:22Z

Thanks for finding the notion page. I have a question, so it's probably a good idea to fix "rescue transcript". But is it necessary to also add label to "solidity transcript"? The scenario described where 3 BN254 points turn out to be identical to 2 BLS12-381 points are not possible in our CAPE contract.
Also I remember Aztec's plonk verifier contract doesn't add label, neither does matter-lab.

Yes, check TL; DR. We didn't need to merge this PR now. OTOH, fixing the "rescue transcript" also has some costs because the SNARK circuit will have more constraints.

I pushed a commit for fixing the rescue transcript. Please check @alxiong @chancharles92

If we fix the rescue transcript, should we also sync the change in the circuit? E.g., in append_variable(), we should also append the label constant.
https://github.com/EspressoSystems/jellyfish/blob/main/plonk/src/circuit/transcript.rs#L84

(It's strange that the tests didn't fail after the changes.)

mrain · 2023-01-18T18:47:56Z

Thanks for finding the notion page. I have a question, so it's probably a good idea to fix "rescue transcript". But is it necessary to also add label to "solidity transcript"? The scenario described where 3 BN254 points turn out to be identical to 2 BLS12-381 points are not possible in our CAPE contract.
Also I remember Aztec's plonk verifier contract doesn't add label, neither does matter-lab.

Yes, check TL; DR. We didn't need to merge this PR now. OTOH, fixing the "rescue transcript" also has some costs because the SNARK circuit will have more constraints.

I pushed a commit for fixing the rescue transcript. Please check @alxiong @chancharles92

If we fix the rescue transcript, should we also sync the change in the circuit? E.g., in append_variable(), we should also append the label constant. https://github.com/EspressoSystems/jellyfish/blob/main/plonk/src/circuit/transcript.rs#L84

(It's strange that the tests didn't fail after the changes.)

Sure we should. But it's not easy in the current gadget design. I'll try to do it today.

chancharles92 · 2023-01-18T18:52:54Z

Sure we should. But it's not easy in the current gadget design. I'll try to do it today.

Thanks, it's not in a hurry as we do not plan to merge this PR very soon.

COULD NOT PASS THE TESTS. Details are described in the comments. Pls search for "TODO".

mrain · 2023-01-18T22:57:28Z

plonk/src/transcript/rescue.rs

-        let mut f = bytes_to_field_elements(&msg);
-        self.transcript.append(&mut f);
+    /// Append the message to the transcript.
+    /// TODO(Chengyu): fix the bug here, current design brings trouble to the


I updated the gadget implementation for rescue transcript. However, I encountered trouble passing the tests. The reason is described here.
I would also suggest remove this from the current release plan.
@alxiong @chancharles92

Agree, we can include this change in a patch release afterwards.

mrain · 2023-01-19T02:12:04Z

Also could someone check the CI here? It failed test on my local computer. But CI passed.

chancharles92 · 2023-01-19T02:18:08Z

Also could someone check the CI here? It failed test on my local computer. But CI passed.

@Ancient123 Can we do similar changes in Jellyfish as we did to the HyperPlonk repo?

Ancient123 · 2023-01-19T15:40:15Z

Also could someone check the CI here? It failed test on my local computer. But CI passed.

@Ancient123 Can we do similar changes in Jellyfish as we did to the HyperPlonk repo?

This is part of the reason I consider it bad practice to call testing scripts versus having them embedded in the github build.yml.

…nto bug-disclaimer

zhenfeizhang added 3 commits May 23, 2022 13:51

add a disclaimer for bug

6151981

fix bug in solidity transcript

8b64887

clean up

3c6ccda

zhenfeizhang mentioned this pull request Jun 6, 2022

add bug disclaimer EspressoSystems/cape#1112

Merged

alxiong added this to the Release 0.2.0 milestone Dec 13, 2022

alxiong self-assigned this Dec 19, 2022

mrain added 2 commits January 17, 2023 14:03

Merge branch 'main' of https://github.com/EspressoSystems/jellyfish i…

8db2575

…nto bug-disclaimer

Fixes rescue transcript

20487d2

mrain changed the title ~~fixing a bug for solidity transcript~~ fixing a bug for rescue/solidity transcript Jan 18, 2023

"Fix" the gadget for rescue transcript.

5284b9c

COULD NOT PASS THE TESTS. Details are described in the comments. Pls search for "TODO".

mrain reviewed Jan 18, 2023

View reviewed changes

mrain removed this from the Release 0.2.0 milestone Jan 19, 2023

Merge branch 'main' of https://github.com/EspressoSystems/jellyfish i…

3248574

…nto bug-disclaimer

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing a bug for rescue/solidity transcript #70

fixing a bug for rescue/solidity transcript #70

zhenfeizhang commented May 23, 2022

CLAassistant commented Jun 29, 2022 •

edited

Loading

alxiong commented Dec 13, 2022 •

edited

Loading

mrain commented Dec 16, 2022

alxiong commented Dec 19, 2022

chancharles92 commented Dec 19, 2022

chancharles92 commented Jan 17, 2023 •

edited

Loading

alxiong commented Jan 18, 2023

chancharles92 commented Jan 18, 2023

mrain commented Jan 18, 2023

chancharles92 commented Jan 18, 2023 •

edited

Loading

mrain commented Jan 18, 2023

chancharles92 commented Jan 18, 2023

mrain Jan 18, 2023

alxiong Jan 19, 2023

mrain commented Jan 19, 2023

chancharles92 commented Jan 19, 2023

Ancient123 commented Jan 19, 2023

fixing a bug for rescue/solidity transcript #70

Are you sure you want to change the base?

fixing a bug for rescue/solidity transcript #70

Conversation

zhenfeizhang commented May 23, 2022

Description

CLAassistant commented Jun 29, 2022 • edited Loading

alxiong commented Dec 13, 2022 • edited Loading

mrain commented Dec 16, 2022

alxiong commented Dec 19, 2022

chancharles92 commented Dec 19, 2022

chancharles92 commented Jan 17, 2023 • edited Loading

alxiong commented Jan 18, 2023

chancharles92 commented Jan 18, 2023

mrain commented Jan 18, 2023

chancharles92 commented Jan 18, 2023 • edited Loading

mrain commented Jan 18, 2023

chancharles92 commented Jan 18, 2023

mrain Jan 18, 2023

Choose a reason for hiding this comment

alxiong Jan 19, 2023

Choose a reason for hiding this comment

mrain commented Jan 19, 2023

chancharles92 commented Jan 19, 2023

Ancient123 commented Jan 19, 2023

CLAassistant commented Jun 29, 2022 •

edited

Loading

alxiong commented Dec 13, 2022 •

edited

Loading

chancharles92 commented Jan 17, 2023 •

edited

Loading

chancharles92 commented Jan 18, 2023 •

edited

Loading