Introduce deployment variable limit and cost #2431

vicsn · 2024-04-10T19:36:05Z

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:

I'm limiting total number of variables, not just constants, as defense in depth. Our lack of variable limitation was nagging me anyway. Speed to synthesize is the same for constants v.s. variables (for their respective worst case opcode keccak v.s. psd)
I'm pricing in variables at the same rate as constraints, to avoid DoS from small programs with lots of constants.
credits.aleo uses ~150k variables, the biggest popular program 300k variables, so (1 << 20) should be a sufficient limit
verification of a deployment with max constants of (1 << 20) takes ~2 seconds in debug mode, so likely 0.02-0.2 second in release).
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. Given that we price deployments by their size, the cost for deployments is increased by 0.01 aleo credit per function. Technically, we could only track the constants and not the total number of variables in the deployment, but I fear its less understandable for outside developers.

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 · 2024-04-12T22:41:35Z

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

…tant calculations

…in a program

…g over those limits Lower MAX_DEPLOYMENT_VARIABLES Initialize circuit gadget constants before synthesis Update test expectation

synthesizer/process/src/stack/helpers/synthesize.rs

ljedrz · 2024-04-15T10:31:13Z

console/network/src/lib.rs

@@ -118,7 +118,9 @@ pub trait Network:
    /// The cost in microcredits per constraint for the deployment transaction.
    const SYNTHESIS_FEE_MULTIPLIER: u64 = 25; // 25 microcredits per constraint
    /// The maximum number of constraints in a deployment.
-    const MAX_DEPLOYMENT_LIMIT: u64 = 1 << 20; // 1,048,576 constraints
+    const MAX_DEPLOYMENT_CONSTRAINTS: u64 = 1 << 20; // 1,048,576 constraints


if the limit is 1 << 20, is it reasonable to assume that we would never expect 1 << 64 to be supported? u32 might be a better pick in that case, and IIRC that could have a performance impact on some of the R1CS operations (if the Variable was also capped at 1 << 32)

Constraints won't exceed u32 anytime soon, especially as our SRS only supports 1 << 28. Perhaps constant variables might end up being more. The circuit crate stores most data as u32, and its only a small amount of counters anyway, so at this point its not worth the rewrite I think.

ljedrz · 2024-04-15T10:33:52Z

ledger/store/src/transaction/deployment.rs

@@ -49,6 +49,8 @@ pub trait DeploymentStorage<N: Network>: Clone + Send + Sync {
    type VerifyingKeyMap: for<'a> Map<'a, (ProgramID<N>, Identifier<N>, u16), VerifyingKey<N>>;
    /// The mapping of `(program ID, function name, edition)` to `certificate`.
    type CertificateMap: for<'a> Map<'a, (ProgramID<N>, Identifier<N>, u16), Certificate<N>>;
+    /// The mapping of `(program ID, function name, edition)` to `variable count`.
+    type VariableCountMap: for<'a> Map<'a, (ProgramID<N>, Identifier<N>, u16), u64>;


this is not strictly related to this PR, but I wonder if, in case of several maps containing composite/large keys, it would make sense to have composite values too (e.g. a single map of (ProgramID<N>, Identifier<N>, u16) to (VerifyingKey<N>, Certificate<N>, u64)), reducing the number of queries and storage space dedicated to keys

Indeed that sounds like an area worthy of optimization

iamalwaysuncomfortable

I understand the defense-in-depth argument, however is putting a variable limit maybe going too far.

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

If there are programs which need more variables to more innocent arithmetic operations, this prevents them from operating - this problem will be especially acute for people writing programs in Leo as it tends to lead to many more variables than programs written natively in Aleo instructions.

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.

vicsn · 2024-04-15T14:06:54Z

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 · 2024-04-29T17:19:53Z

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 · 2024-04-29T19:43:14Z

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:

The CircuitVerifyingKey is from the algorithms crate, so it is also semantically not clean to add constants in there which are not relevant in that crate. I.e. we'd have to set them to 0 or None for any tests local to the Varuna crate.
Another option is the VerifyingKey in the synthesizer crate. I didn't want to mess with that abstraction either, as it is used as a thin wrapper.

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 · 2024-04-29T19:50:42Z

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

howardwu · 2024-04-29T22:53:18Z

Closing in favor of #2444

vicsn requested review from d0cd and evan-schott April 10, 2024 19:36

vicsn force-pushed the introduce_variable_limit branch 2 times, most recently from 5de1115 to d2f8d1e Compare April 12, 2024 11:45

vicsn marked this pull request as ready for review April 12, 2024 11:45

vicsn requested a review from ljedrz April 12, 2024 14:56

vicsn added 4 commits April 15, 2024 11:55

Introduce variable limit for deployed functions, to prevent long cons…

22c1504

…tant calculations

Ensure MAX_DEPLOYMENT_VARIABLES limits the total number of variables …

15fabe7

…in a program

Track num_variables for each function in a Deployment to prevent goin…

88b5ffb

…g over those limits Lower MAX_DEPLOYMENT_VARIABLES Initialize circuit gadget constants before synthesis Update test expectation

Add variable limit to testnet

1db96dd

vicsn force-pushed the introduce_variable_limit branch from d2f8d1e to 1db96dd Compare April 15, 2024 10:22

ljedrz reviewed Apr 15, 2024

View reviewed changes

synthesizer/process/src/stack/helpers/synthesize.rs Show resolved Hide resolved

ljedrz reviewed Apr 15, 2024

View reviewed changes

iamalwaysuncomfortable reviewed Apr 15, 2024

View reviewed changes

vicsn added 3 commits April 16, 2024 11:58

Count variables in the entire circuit - not just the scope

4053412

Correctly assert the number of variables of inital circuits

84d05f8

Price in num variables to avoid DoS by cheap const programs

9fb9a60

vicsn changed the title ~~Introduce variable limit~~ Introduce variable limit and cost Apr 17, 2024

vicsn changed the title ~~Introduce variable limit and cost~~ Introduce deployment variable limit and cost Apr 17, 2024

Update test expectations

4a7940e

howardwu mentioned this pull request Apr 29, 2024

Adds a variable limit for deployments #2444

Merged

howardwu closed this Apr 29, 2024

vicsn mentioned this pull request May 6, 2024

[Feature] Print number of variables when calling leo run ProvableHQ/leo#27934

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce deployment variable limit and cost #2431

Introduce deployment variable limit and cost #2431

vicsn commented Apr 10, 2024 •

edited

Loading

howardwu commented Apr 12, 2024

ljedrz Apr 15, 2024 •

edited

Loading

vicsn Apr 15, 2024

ljedrz Apr 15, 2024 •

edited

Loading

vicsn Apr 15, 2024

iamalwaysuncomfortable left a comment •

edited

Loading

vicsn commented Apr 15, 2024

howardwu commented Apr 29, 2024

vicsn commented Apr 29, 2024 •

edited

Loading

howardwu commented Apr 29, 2024

howardwu commented Apr 29, 2024

Introduce deployment variable limit and cost #2431

Introduce deployment variable limit and cost #2431

Conversation

vicsn commented Apr 10, 2024 • edited Loading

Motivation

Test Plan

Related PRs

howardwu commented Apr 12, 2024

ljedrz Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

vicsn Apr 15, 2024

Choose a reason for hiding this comment

ljedrz Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

vicsn Apr 15, 2024

Choose a reason for hiding this comment

iamalwaysuncomfortable left a comment • edited Loading

Choose a reason for hiding this comment

vicsn commented Apr 15, 2024

howardwu commented Apr 29, 2024

vicsn commented Apr 29, 2024 • edited Loading

howardwu commented Apr 29, 2024

howardwu commented Apr 29, 2024

vicsn commented Apr 10, 2024 •

edited

Loading

ljedrz Apr 15, 2024 •

edited

Loading

ljedrz Apr 15, 2024 •

edited

Loading

iamalwaysuncomfortable left a comment •

edited

Loading

vicsn commented Apr 29, 2024 •

edited

Loading