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

[HackerOne-2354265] Restrict number of imports, program depth, and call depth. #2352

Merged
merged 15 commits into from
Feb 15, 2024

Conversation

d0cd
Copy link
Contributor

@d0cd d0cd commented Feb 12, 2024

This PR,

  • introduces a limit for the number of imports in a program.
  • checks that each function in a program makes at most MAX_TRANSITIONS - 1 calls.
  • introduces an upper bound in import depth
  • caches get_number_of_function_calls.

@raychu86 raychu86 changed the title [Fix] Restrict number of imports, program depth, and call depth. [HackerOne-2354265] Restrict number of imports, program depth, and call depth. Feb 12, 2024
Comment on lines 164 to 166
const MAX_PROGRAM_DEPTH: usize = 1024;
/// The maximum number of imports.
const MAX_IMPORTS: usize = 2048;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide some context on why these depths and imports are where they're at?

Based on our design, I believe we're constraining other parameters already at the 32 mark. It would make sense to constraint both of these parameters alongside the 32 mark.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MAX_PROGRAM_DEPTH was decided based off of Ethereum's MAX_CALL_DEPTH.
I went with this as a healthy upper bound. I see no issue with restricting MAX_PROGRAM_DEPTH to 32.

MAX_IMPORTS should be larger because a user may want to create a program that reads the mappings of a large number of other programs.

Copy link
Contributor Author

@d0cd d0cd Feb 13, 2024

Choose a reason for hiding this comment

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

Ah actually, suppose that program1 reads program2 reads ... reads program32
If we program0 to reads program1, then we need a greater MAX_PROGRAM_DEPTH.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline, waiting for a change to 64 and 64

@randomsleep
Copy link
Contributor

One question less related this issue: Would it be better to set a larger 'TRANSACTION_DEPTH', (e.g. 8)? This will make it easier to increase 'MAX_TRANSITION' in the future. We can still keep 'MAX_TRANSITION' as 32 for now.

@howardwu
Copy link
Contributor

One question less related this issue: Would it be better to set a larger 'TRANSACTION_DEPTH', (e.g. 8)? This will make it easier to increase 'MAX_TRANSITION' in the future. We can still keep 'MAX_TRANSITION' as 32 for now.

This has problems because it will change the Merklization to be much slower, and require a larger inclusion circuit for proving. Ultimately, these impact runtime for transaction generation.

@howardwu howardwu merged commit 6e182c6 into mainnet Feb 15, 2024
78 checks passed
@howardwu howardwu deleted the fix/import-depth branch February 15, 2024 00:06
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.

3 participants