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

[zk-token-sdk] Restrict Edwards and Ristretto multiscalar multiplication vector length to at most 512 #34763

Merged
merged 6 commits into from
Jan 18, 2024
Merged

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Jan 12, 2024

Problem

The curve25519 syscalls have an operation called the multiscalar multiplication (MSM), which computes the inner product of a vector scalar and curve points. The larger the vectors are, the more expensive the MSM operation becomes. The assigned compute units for MSM's do take this growing cost into account, but there is no concrete bound on the size of the vectors.

Theoretically, an attacker could craft a number of txs with very large MSM inputs, which could lead to either:

  • all validators spending more than 400ms to compute all the MSMs
  • or some validators take more than 400ms and others less

It would be good to define the max size of the vectors for MSM for safety. For most practical applications, MSM of vector length 512 should be sufficient.

Summary of Changes

  • Add a max cap of 512 vector length for the multiscalar multiplication syscall.
  • The curve25519 syscalls are already activated on testnet and devnet, but not yet on MB. I created a new feature id and replaced the old feature id.

Fixes #

@samkim-crypto samkim-crypto added the work in progress This isn't quite right yet label Jan 12, 2024
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9468996) 81.7% compared to head (694c344) 81.8%.
Report is 10 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34763   +/-   ##
=======================================
  Coverage    81.7%    81.8%           
=======================================
  Files         825      825           
  Lines      223126   223164   +38     
=======================================
+ Hits       182507   182585   +78     
+ Misses      40619    40579   -40     

@samkim-crypto samkim-crypto added v1.17 PRs that should be backported to v1.17 and removed work in progress This isn't quite right yet labels Jan 12, 2024
Copy link
Contributor

mergify bot commented Jan 12, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

programs/bpf_loader/src/syscalls/mod.rs Outdated Show resolved Hide resolved
zk-token-sdk/src/curve25519/edwards.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

ok, lgtm now. r+. let's give @Lichtso 'til EOW to take a pass. ping me again friday evening if he hasn't had a chance and i'll approve

@samkim-crypto samkim-crypto merged commit 7321859 into solana-labs:master Jan 18, 2024
45 checks passed
mergify bot pushed a commit that referenced this pull request Jan 18, 2024
…ion vector length to at most 512 (#34763)

* restrict curve25519 multiscalar multiplication vector length to 512

* add syscall tests for msm vector length

* add new feature gate `curve25519_restrict_msm_length`

* update tests for feature new gate

* Update programs/bpf_loader/src/syscalls/mod.rs

Co-authored-by: Trent Nelson <[email protected]>

* remove length guard on the multisicalar mult lib function

---------

Co-authored-by: Trent Nelson <[email protected]>
(cherry picked from commit 7321859)

# Conflicts:
#	sdk/src/feature_set.rs
samkim-crypto added a commit that referenced this pull request Jan 20, 2024
…iplication vector length to at most 512 (backport of #34763) (#34849)

* [zk-token-sdk] Restrict Edwards and Ristretto multiscalar multiplication vector length to at most 512 (#34763)

* restrict curve25519 multiscalar multiplication vector length to 512

* add syscall tests for msm vector length

* add new feature gate `curve25519_restrict_msm_length`

* update tests for feature new gate

* Update programs/bpf_loader/src/syscalls/mod.rs

Co-authored-by: Trent Nelson <[email protected]>

* remove length guard on the multisicalar mult lib function

---------

Co-authored-by: Trent Nelson <[email protected]>
(cherry picked from commit 7321859)

# Conflicts:
#	sdk/src/feature_set.rs

* resolve conflict

---------

Co-authored-by: samkim-crypto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants