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-2300725] Limit the number of allowed constraints for deployments #2271

Merged
merged 40 commits into from
Feb 9, 2024

Conversation

vicsn
Copy link
Contributor

@vicsn vicsn commented Dec 29, 2023

Motivation

Large deployments may take a lot of time to synthesize, so as an initial stopgap we can limit their size, as measurement by the number of constraints.

Note that the current approach panics on failure, but because check_transaction_basic is called from snarkOS in a tokio task, this panic is gracefully handled. Returning a Result would cascade to 100 other functions using the Circuit api.

Test Plan

  • Manually test deploying to a validator via RPC. Prevents large deployments succesfully. Even with a manipulated verifying_key.
  • Check that the synthesizer benchmarks don't take a big hit
  • Add a test deploying a program which is too big.

Related PRs

https://github.com/AleoHQ/snarkOS/pull/2961

Copy link
Contributor

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

Left a few comments and suggestions.

@vicsn vicsn changed the title Limit the number of allowed constraints for deployments WIP: Limit the number of allowed constraints for deployments Jan 10, 2024
@vicsn vicsn changed the title WIP: Limit the number of allowed constraints for deployments Limit the number of allowed constraints for deployments Jan 10, 2024
@vicsn
Copy link
Contributor Author

vicsn commented Jan 22, 2024

@evan-schott can you help finish the two tests in 01b95d2 and look at fixing any broken CI tests? We can discuss further in person.

@evan-schott
Copy link
Contributor

@evan-schott can you help finish the two tests in 01b95d2 and look at fixing any broken CI tests? We can discuss further in person.

I will investigate

@raychu86
Copy link
Contributor

Is there a reason more CI tests are failing? Do we need to resample anything?

@vicsn
Copy link
Contributor Author

vicsn commented Feb 2, 2024

LGTM!

Comment on lines +385 to +389
// If in 'CheckDeployment' mode, set the expected constraint limit.
if let CallStack::CheckDeployment(_, _, _, constraint_limit) = &registers.call_stack() {
A::set_constraint_limit(*constraint_limit);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced this logic is necessary.

When triggering call, no logic above this resets the constraint limit AFAIK.

@howardwu howardwu merged commit ee6b2da into mainnet Feb 9, 2024
64 of 78 checks passed
@howardwu howardwu deleted the limit_deployment_num_constraints branch February 9, 2024 02:26
@raychu86 raychu86 changed the title Limit the number of allowed constraints for deployments [HackerOne-2300725] Limit the number of allowed constraints for deployments Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants