feat: Forward ports #40
Conversation
huitseeker
force-pushed
the
forward_ports_35
branch
4 times, most recently
from
ecd94f3 to
3a446e0
Compare
Co-authored-by: John Guibas <jtguibas@gmail.com> Co-authored-by: Tamir Hemo <tamir@succinct.xyz> Co-authored-by: Ubuntu <ubuntu@ip-172-31-74-90.ec2.internal> Co-authored-by: Tarik Moon <tarik@tarikmoon.com> Co-authored-by: Matthias Görgens <matthias.goergens@gmail.com> Co-authored-by: Eugene Rabinovich <eugene@succinct.xyz> Co-authored-by: Chris Tian <chris@succinct.xyz> chore: pk/vk serde/clone (#770) chore: SP1ProvingKey serde (#772) chore(recursion): convert ext2felt to hint (#771) chore(recursion): heap ptr checks (#775) feat: groth16 feature flag (#782) chore(recursion): explicitly don't allow witness and public values related apis in sub-builder (#744) fix(recursion): assert curve bit length in circuit p2_hash (#736) fix(recursion): num2bits fixes (#732) chore(recursion): document IR (#737) chore: logup format (#788) chore: merge main into dev (#801) Co-authored-by: Chris T <chris@succinct.xyz> feat: byte multiplicity channel (#800) fix(core): Fix benches warning for unstable features (#763) fix(core): Remove dummy constraint (#783) chore: Clean up TOML files (#796) chore: remove unused deps (#794) Co-authored-by: Bing <b.ing@keemail.me> Co-authored-by: John Guibas <jtguibas@gmail.com> chore: Make some functions const (#774) Co-authored-by: John Guibas <jtguibas@gmail.com> chore: no remainder range checks if division by 0 (#764) Co-authored-by: Tamir Hemo <tamir@succinct.xyz> chore: upgrade checkout action to version with node20 (#734) feat: plonk prover (#795) Co-authored-by: Ubuntu <ubuntu@ip-172-31-78-118.ec2.internal> Co-authored-by: Chris Tian <chris@succinct.xyz> feat: batch sized recursion (#785) Co-authored-by: Ratan Kaliani <ratankaliani@berkeley.edu> chore: remove unecessary todo chore: get rid of unecessary todo in recursion program fix clippy permutation chore fix clippy hm clippy chore: remove unecessary todos in recursion clippy fixes fix: challenger rate issue fix clippy fix cargo check pin new p3 feat: sp1 core prover opts hm fmt hm fix: install for `verify_plonk_bn254` (#798) feat: update contract artifacts (#802) Co-authored-by: Chris T <chris@succinct.xyz> chore: merge main into dev (#823) fix: update release workflow (#777) feat: release on main (#779) refactor: sdk updates (#784) + make vk/pk serde and Clone feat: switch to ethers (#826) refactor: `prove_plonk` (#827) feat: remove `sp1-zkvm` precompile nightly (#810) fix: update ELF's in tests (#830) docs: simplify quickstart (#819)
huitseeker
force-pushed
the
forward_ports_35
branch
from
4ff5d53 to
cf60599
Compare
huitseeker
force-pushed
the
forward_ports_35
branch
from
2a130d2 to
15e1af7
Compare
There was a problem hiding this comment.
A few comments, but this seems to work great! I'm generating the plonk artifacts and looking into the LC changes to work with this (should be very simple, renaming some struct fields, types and prove_groth16 -> prove_plonk, but I want to test the E2E flow as well)
| pub struct SphinxCoreOpts { | ||
| pub shard_size: usize, | ||
| pub shard_batch_size: usize, | ||
| pub shard_chunking_multiplier: usize, | ||
| pub reconstruct_commitments: bool, | ||
| } | ||
|
|
||
| impl Default for SphinxCoreOpts { | ||
| fn default() -> Self { | ||
| Self { | ||
| shard_size: 1 << 22, | ||
| shard_batch_size: 16, | ||
| shard_chunking_multiplier: 1, | ||
| reconstruct_commitments: false, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I believe this change makes the env vars SHARD_SIZE, SHARD_BATCH_SIZE, SHARD_CHUNKING_MULTIPLIER and RECONSTRUCT_COMMITMENTS no longer take effect. The code that read the env vars was in core/src/utils/env.rs which was deleted. As far as I can tell, this also is still present and affects upstream as of v1.0.6-testnet too.
This was partially fixed for SHARD_SIZE and SHARD_BATCH_SIZE in upstream dev and in particular by this upstream PR succinctlabs/sp1#889
If possible, we probably want to port this commit/PR (or perform an equivalent fix) ASAP (i.e. without waiting for upstream to merge dev into the next release), since we've seen that setting SHARD_BATCH_SIZE=0 increases performance by disabling checkpointing and using more RAM, and without an additional fix that env var would no longer take effect
There was a problem hiding this comment.
Yeah, the previous was strictly better, I think we want to revert the changes partially.
There was a problem hiding this comment.
For reference, SHARD_BATCH_SIZE=0 does not work the same way as it used to anymore, and if you set it will panic with "chunk size must be non-zero" here
As far as I can tell this is also an issue in current upstream dev, but it might be possible to change that particular code to handle SHARD_BATCH_SIZE=0 and revert to previous behavior (I think), hopefully in a minimally intrusive way
EDIT: Filed #44 because I think this is a minor perf regression
huitseeker
force-pushed
the
forward_ports_35
branch
from
b0083f3 to
df866f8
Compare
- Restore the ability to run the entire process in one chunk by setting `SHARD_BATCH_SIZE` to `0`.
huitseeker
force-pushed
the
forward_ports_35
branch
from
3de8e7b to
e8539d6
Compare
3 participants
This ports the following upstream PR:
This contributes to #38. Fixes #44
Important
This also ports Plonk proving, an intrusive addition, and the multiplicity fixes of succinctlabs/sp1#800 (but not those of succinctlabs/sp1#853, which will come in later)
Note
This will need a light client companion PR.