Compression integration (draft) #1134
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
77
to
97
| fn commit_compressed( | ||
| poly: &MultilinearPolynomial<ark_bn254::Fr>, | ||
| setup: &Self::ProverSetup, | ||
| ) -> (Self::CompressedCommitment, Self::OpeningProofHint) { | ||
| let _span = trace_span!("DoryCommitmentScheme::commit").entered(); | ||
|
|
||
| let num_cols = DoryGlobals::get_num_columns(); | ||
| let num_rows = DoryGlobals::get_max_num_rows(); | ||
| let sigma = num_cols.log_2(); | ||
| let nu = num_rows.log_2(); | ||
|
|
||
| todo!() | ||
| // let (tier_2, row_commitments) = <MultilinearPolynomial<ark_bn254::Fr> as Polynomial< | ||
| // ArkFr, | ||
| // >>::commit_compressed::<ArkBn254, JoltG1Routines>( | ||
| // poly, nu, sigma, setup | ||
| // ) | ||
| // .expect("commitment should succeed"); | ||
|
|
||
| // (tier_2, row_commitments) | ||
| } |
Contributor
There was a problem hiding this comment.
The commit_compressed method currently contains a todo!() statement that will panic at runtime if called. Before this PR can be merged, this method needs to be fully implemented. Consider uncommenting and completing the code below the todo!() statement, which appears to be the intended implementation path. This is particularly important since the compressed commitment functionality is a core part of this integration.
Suggested change
| fn commit_compressed( | |
| poly: &MultilinearPolynomial<ark_bn254::Fr>, | |
| setup: &Self::ProverSetup, | |
| ) -> (Self::CompressedCommitment, Self::OpeningProofHint) { | |
| let _span = trace_span!("DoryCommitmentScheme::commit").entered(); | |
| let num_cols = DoryGlobals::get_num_columns(); | |
| let num_rows = DoryGlobals::get_max_num_rows(); | |
| let sigma = num_cols.log_2(); | |
| let nu = num_rows.log_2(); | |
| todo!() | |
| // let (tier_2, row_commitments) = <MultilinearPolynomial<ark_bn254::Fr> as Polynomial< | |
| // ArkFr, | |
| // >>::commit_compressed::<ArkBn254, JoltG1Routines>( | |
| // poly, nu, sigma, setup | |
| // ) | |
| // .expect("commitment should succeed"); | |
| // (tier_2, row_commitments) | |
| } | |
| fn commit_compressed( | |
| poly: &MultilinearPolynomial<ark_bn254::Fr>, | |
| setup: &Self::ProverSetup, | |
| ) -> (Self::CompressedCommitment, Self::OpeningProofHint) { | |
| let _span = trace_span!("DoryCommitmentScheme::commit").entered(); | |
| let num_cols = DoryGlobals::get_num_columns(); | |
| let num_rows = DoryGlobals::get_max_num_rows(); | |
| let sigma = num_cols.log_2(); | |
| let nu = num_rows.log_2(); | |
| let (tier_2, row_commitments) = <MultilinearPolynomial<ark_bn254::Fr> as Polynomial< | |
| ArkFr, | |
| >>::commit_compressed::<ArkBn254, JoltG1Routines>(poly, nu, sigma, setup) | |
| .expect("commitment should succeed"); | |
| (tier_2, row_commitments) | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Cargo.toml
Outdated
| ark-ec = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout" } | ||
| ark-serialize = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout" } | ||
| allocative = { git = "https://github.com/facebookexperimental/allocative", rev = "85b773d85d526d068ce94724ff7a7b81203fc95e" } | ||
| dory-pcs = { git = "https://github.com/a16z/dory", branch = "dev/twist-shout" } |
Contributor
There was a problem hiding this comment.
Inconsistency between Cargo.toml and Cargo.lock for dory-pcs dependency. Cargo.toml adds a patch pointing to a16z/dory branch dev/twist-shout, but Cargo.lock shows the dependency is resolved from JuI3s/dory branch compression-integration. This mismatch will cause build issues and dependency resolution failures.
# Cargo.toml line 130 adds:
dory-pcs = { git = "https://github.com/a16z/dory", branch = "dev/twist-shout" }
# But Cargo.lock shows:
source = "git+https://github.com/JuI3s/dory?branch=compression-integration#..."Update Cargo.toml to match the actual dependency being used:
dory-pcs = { git = "https://github.com/JuI3s/dory", branch = "compression-integration" }
Suggested change
| dory-pcs = { git = "https://github.com/a16z/dory", branch = "dev/twist-shout" } | |
| dory-pcs = { git = "https://github.com/JuI3s/dory", branch = "compression-integration" } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Comment on lines
136
to
140
| # Cryptography and Math | ||
| ark-bn254 = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", default-features = false } | ||
| ark-grumpkin = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", default-features = false } | ||
| ark-grumpkin = { git = "https://github.com/JuI3s/arkworks-algebra", branch = "compression", default-features = false } | ||
| ark-ec = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", default-features = false } | ||
| ark-ff = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", default-features = false } |
Contributor
There was a problem hiding this comment.
Inconsistent arkworks-algebra sources will cause version conflicts. Most dependencies use a16z/arkworks-algebra branch dev/twist-shout (lines 136, 138-140) while ark-grumpkin uses JuI3s/arkworks-algebra branch compression (line 137). Since these packages are tightly coupled, mixing sources will lead to type incompatibilities and compilation failures.
# Line 136: a16z repo
ark-bn254 = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", ... }
# Line 137: JuI3s repo (different!)
ark-grumpkin = { git = "https://github.com/JuI3s/arkworks-algebra", branch = "compression", ... }
# Lines 138-140: back to a16z repo
ark-ec = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", ... }All arkworks dependencies should use the same repository and branch to ensure compatibility.
Suggested change
| # Cryptography and Math | |
| ark-bn254 = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", default-features = false } | |
| ark-grumpkin = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", default-features = false } | |
| ark-grumpkin = { git = "https://github.com/JuI3s/arkworks-algebra", branch = "compression", default-features = false } | |
| ark-ec = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", default-features = false } | |
| ark-ff = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", default-features = false } | |
| # Cryptography and Math | |
| ark-bn254 = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", default-features = false } | |
| ark-grumpkin = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", default-features = false } | |
| ark-ec = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", default-features = false } | |
| ark-ff = { git = "https://github.com/a16z/arkworks-algebra", branch = "dev/twist-shout", default-features = false } | |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
JuI3s
force-pushed
the
compression-integration
branch
from
6489b2a to
d1b6d0c
Compare
JuI3s
force-pushed
the
compression-integration
branch
from
d1b6d0c to
b31cb94
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.