AleoNet / snarkVM

A Virtual Machine for Zero-Knowledge Executions
https://snarkvm.org
Apache License 2.0
1.08k stars 1.5k forks source link

Introduce deployment variable limit and cost #2431

Closed vicsn closed 7 months ago

vicsn commented 7 months ago

Motivation

Even though we limit program size and number of constraints, @d0cd identified that it is possible for someone to create huge constants in programs. During deployment verification, an attacker can make a validator take forever or use a lot of memory, without paying for it, because we don't limit or price constants.

The solution is to limit constants similarly to how we limit the number of constraints.

Some design considerations:

Test Plan

Adding two tests, one testing the variable limit, and one testing manipulation of reported variables.

Related PRs

Similar to: https://github.com/AleoHQ/snarkVM/pull/2271

howardwu commented 7 months ago

You'll need to reflect TestnetCircuit changes and TestnetV0 changes now as well.

vicsn commented 7 months ago

Good questions.

It's said that 300k should be sufficient but what's the basis of that assumption of sufficiency?

The limit is 1M, same as the constraint limit, because synthesizing variables and constraints for our existing gadgets go hand in hand. If you grep in snarkVM for Count:: you'll see all of our existing gadgets allocate roughly the same amount of variables as constraints (actually a bit less variables, because multiple constraints typically link the same variable). Compared to before, the only usage which this design limits is someone trying to generate large constants.

My main concern with this is that how the circuit creates variables under the hood is effectively opaque to most users (minus those who are familiar with the circuits) and will create a bad UX as users pull out their hair trying to find out why their deployments are failing.

This was also the case for the constraint limit. If you have alternative design ideas let me know.

I agree we should report the reason of failure: https://github.com/AleoHQ/snarkOS/issues/2962

howardwu commented 7 months ago

The design should have variable count in the verifying key. It is error prone to have it defined as an accessory, like in this PR.

AFAIK, the insert_variable_count is NOT called everywhere insert_verifying_key is called.

It would be helpful to know why this design was chosen, and whether it was validated on a snarkOS devnet.

vicsn commented 7 months ago

It would be helpful to know why this design was chosen, and whether it was validated on a snarkOS devnet.

I did consider it. From this PR's README: "we cannot add num_constants to CircuitInfo, constants are not a known quantity at the Varuna level. That's why I added the value to the Deployment object." Adding more context:

But go wild if you want to do it anyway.

whether it was validated on a snarkOS devnet.

Not yet, otherwise I would/should have mentioned in the PR README or one of the comments.

AFAIK, the insert_variable_count is NOT called everywhere insert_verifying_key is called.

I see, that looks like a potential issue. A simpler way around could be to unify just those 2 functions

howardwu commented 7 months ago

Thanks for the insight. We're currently discussing using https://github.com/AleoHQ/snarkVM/pull/2444 as the replacement PR.

howardwu commented 7 months ago

Closing in favor of #2444