Skip to content
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

[feat] make assembly block Solidity memory safe #50

Merged

Conversation

jonathanpwang
Copy link
Contributor

Previously the assembly block for snark verifier started mstore from memory offset 0. This does not respect Solidity's memory layout where 0x40 = 64 is reserved as a free memory pointer. See https://docs.soliditylang.org/en/latest/assembly.html#memory-safety

We make the assembly block memory safe by changing EvmLoader::new to start ptr from MEM_PTR_START = 0x80, and check in the assembly block preamble that mload(0x40) = 0x80. This means that the memory between 0 and 0x80 is never used.

@han0110 The one thing I was a little unsure of was the common_scalar in EvmTranscript: is common_scalar only called exactly once at the very beginning on the transcript_initial_repr and so it's special case mload to 0x80 (formerly 0x00)?

Copy link
Collaborator

@han0110 han0110 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

The one thing I was a little unsure of was the common_scalar in EvmTranscript: is common_scalar only called exactly once at the very beginning on the transcript_initial_repr and so it's special case mload to 0x80 (formerly 0x00)?

Yes exactly, it's preserved for transcript_repr (it's a stupid design and should be fix tho..)

@han0110 han0110 merged commit bd61f8d into privacy-scaling-explorations:main Oct 30, 2023
2 checks passed
@jonathanpwang jonathanpwang deleted the feat/memory-safe branch December 9, 2023 00:04
zemse pushed a commit to zemse/snark-verifier that referenced this pull request Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants