upgrade 0809: use feynman guest rkyv #1718
Conversation
WalkthroughThis change updates workspace dependencies in Changes
Sequence Diagram(s)sequenceDiagram
participant Task as ProvingTask Conversion
participant Witness as Witness Data
participant Serializer as rkyv_serialize
Task->>Witness: Prepare witness for serialization
Task->>Serializer: Serialize(witness, Some(ForkName::Feynman))
Serializer-->>Task: Serialized witness data (with Feynman fork context)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(1 hunks)crates/libzkp/src/tasks/bundle.rs(1 hunks)crates/libzkp/src/tasks/chunk.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (1)
crates/libzkp/src/tasks/chunk.rs (1)
6-6: Import addition looks correctNew import of ForkName is necessary for fork-aware serialization. No concerns.
| scroll-zkvm-prover-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", tag = "0.5.4", package = "scroll-zkvm-prover" } | ||
| scroll-zkvm-verifier-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", tag = "0.5.4", package = "scroll-zkvm-verifier" } | ||
| scroll-zkvm-types = { git = "https://github.com/scroll-tech/zkvm-prover", tag = "0.5.4" } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pin git dependencies to the exact commit for reproducibility and supply-chain safety
Using a tag is better than a branch, but tags can be retagged. Since the PR references commit 72f9abb00f0ef7c5e46b7856d58bb86cbb332509, prefer rev to guarantee determinism.
Apply this diff:
-scroll-zkvm-prover-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", tag = "0.5.4", package = "scroll-zkvm-prover" }
-scroll-zkvm-verifier-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", tag = "0.5.4", package = "scroll-zkvm-verifier" }
-scroll-zkvm-types = { git = "https://github.com/scroll-tech/zkvm-prover", tag = "0.5.4" }
+scroll-zkvm-prover-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", rev = "72f9abb00f0ef7c5e46b7856d58bb86cbb332509", package = "scroll-zkvm-prover" }
+scroll-zkvm-verifier-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", rev = "72f9abb00f0ef7c5e46b7856d58bb86cbb332509", package = "scroll-zkvm-verifier" }
+scroll-zkvm-types = { git = "https://github.com/scroll-tech/zkvm-prover", rev = "72f9abb00f0ef7c5e46b7856d58bb86cbb332509" }Verification script to confirm the tag’s commit (optional):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
REPO="https://github.com/scroll-tech/zkvm-prover"
TAG="0.5.4"
EXPECTED="72f9abb00f0ef7c5e46b7856d58bb86cbb332509"
echo "Tag -> commit:"
git ls-remote "$REPO" "refs/tags/$TAG" | awk '{print $1, $2}'
echo "Expected: $EXPECTED"
if git ls-remote "$REPO" "refs/tags/$TAG" | grep -q "^$EXPECTED"; then
echo "OK: Tag $TAG points to expected commit."
else
echo "WARN: Tag $TAG does not match expected commit."
fiLength of output: 658
Pin git dependencies to an exact commit using rev
Confirmed that tag 0.5.4 currently points to commit 72f9abb00f0ef7c5e46b7856d58bb86cbb332509. To guard against future retagging and ensure full reproducibility, switch from tag to rev.
• Update in Cargo.toml (around lines 20–22):
-scroll-zkvm-prover-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", tag = "0.5.4", package = "scroll-zkvm-prover" }
-scroll-zkvm-verifier-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", tag = "0.5.4", package = "scroll-zkvm-verifier" }
-scroll-zkvm-types = { git = "https://github.com/scroll-tech/zkvm-prover", tag = "0.5.4" }
+scroll-zkvm-prover-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", rev = "72f9abb00f0ef7c5e46b7856d58bb86cbb332509", package = "scroll-zkvm-prover" }
+scroll-zkvm-verifier-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", rev = "72f9abb00f0ef7c5e46b7856d58bb86cbb332509", package = "scroll-zkvm-verifier" }
+scroll-zkvm-types = { git = "https://github.com/scroll-tech/zkvm-prover", rev = "72f9abb00f0ef7c5e46b7856d58bb86cbb332509" }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| scroll-zkvm-prover-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", tag = "0.5.4", package = "scroll-zkvm-prover" } | |
| scroll-zkvm-verifier-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", tag = "0.5.4", package = "scroll-zkvm-verifier" } | |
| scroll-zkvm-types = { git = "https://github.com/scroll-tech/zkvm-prover", tag = "0.5.4" } | |
| scroll-zkvm-prover-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", rev = "72f9abb00f0ef7c5e46b7856d58bb86cbb332509", package = "scroll-zkvm-prover" } | |
| scroll-zkvm-verifier-euclid = { git = "https://github.com/scroll-tech/zkvm-prover", rev = "72f9abb00f0ef7c5e46b7856d58bb86cbb332509", package = "scroll-zkvm-verifier" } | |
| scroll-zkvm-types = { git = "https://github.com/scroll-tech/zkvm-prover", rev = "72f9abb00f0ef7c5e46b7856d58bb86cbb332509" } |
🤖 Prompt for AI Agents
In Cargo.toml around lines 20 to 22, the git dependencies use the tag "0.5.4"
which can be moved to point to a specific commit for better reproducibility.
Replace the "tag" field with "rev" and set its value to the commit hash
"72f9abb00f0ef7c5e46b7856d58bb86cbb332509" for all three dependencies to pin
them to that exact commit.
| @@ -83,7 +83,7 @@ impl TryFrom<BundleProvingTask> for ProvingTask { | |||
| .into_iter() | |||
| .map(|w_proof| w_proof.proof.into_root_proof().expect("expect root proof")) | |||
| .collect(), | |||
| serialized_witness: vec![witness.rkyv_serialize(None)?.to_vec()], | |||
| serialized_witness: vec![witness.rkyv_serialize(Some(ForkName::Feynman))?.to_vec()], | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Hard-coding Feynman here ignores the task’s provided fork_name; use the actual fork to avoid mismatches
Serialization should respect the task’s fork to keep witness and metadata consistent. Hard-coding Feynman can produce invalid or unverifiable artifacts when fork_name != "feynman".
Apply this diff within the selected line:
- serialized_witness: vec![witness.rkyv_serialize(Some(ForkName::Feynman))?.to_vec()],
+ serialized_witness: vec![witness.rkyv_serialize(Some(fork))?.to_vec()],And add this line before constructing ProvingTask (outside the selected range):
let fork = ForkName::from(value.fork_name.as_str());Optional repo-wide sanity check:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Calls to rkyv_serialize and their fork arg:"
rg --line-number 'rkyv_serialize\(' -A 1
echo
echo "Places using Some(ForkName::Feynman):"
rg --line-number 'Some\(ForkName::Feynman\)'
echo
echo "Places constructing ForkName dynamically:"
rg --line-number 'ForkName::from\('Length of output: 1308
Replace hard-coded Feynman with the task’s actual fork_name
Serialization must use the configured fork to keep witness and metadata in sync. Update both occurrences where ForkName::Feynman is passed to rkyv_serialize:
Files needing changes:
- crates/libzkp/src/tasks/bundle.rs (line 86)
- crates/libzkp/src/tasks/chunk.rs (line 75)
Apply this diff in each file:
- serialized_witness: vec![witness.rkyv_serialize(Some(ForkName::Feynman))?.to_vec()],
+ serialized_witness: vec![witness.rkyv_serialize(Some(fork_name))?.to_vec()],Both modules already bind fork_name earlier (e.g. let fork_name = ForkName::from(self.fork_name.as_str());), so no new variable is needed.
Optional sanity check to confirm all instances:
rg --line-number 'rkyv_serialize\(' -A 1
rg --line-number 'Some\(ForkName::Feynman\)'
rg --line-number 'ForkName::from\('🤖 Prompt for AI Agents
In crates/libzkp/src/tasks/bundle.rs at line 86, replace the hard-coded
ForkName::Feynman argument passed to rkyv_serialize with the existing fork_name
variable to ensure serialization uses the configured fork. Locate the
rkyv_serialize calls and substitute Some(ForkName::Feynman) with
Some(fork_name). Repeat the same change in crates/libzkp/src/tasks/chunk.rs at
line 75. No new variable declaration is needed since fork_name is already
defined earlier in both files.
| @@ -71,7 +72,7 @@ impl TryFrom<ChunkProvingTask> for ProvingTask { | |||
| identifier: value.identifier(), | |||
| fork_name: value.fork_name, | |||
| aggregated_proofs: Vec::new(), | |||
| serialized_witness: vec![witness.rkyv_serialize(None)?.to_vec()], | |||
| serialized_witness: vec![witness.rkyv_serialize(Some(ForkName::Feynman))?.to_vec()], | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Same issue: serialization must use the task’s fork, not a hard-coded Feynman
Align serialization with the provided fork_name to prevent cross-fork inconsistencies and verification failures.
Apply this diff within the selected line:
- serialized_witness: vec![witness.rkyv_serialize(Some(ForkName::Feynman))?.to_vec()],
+ serialized_witness: vec![witness.rkyv_serialize(Some(fork))?.to_vec()],And add this line before constructing ProvingTask (outside the selected range):
let fork = ForkName::from(value.fork_name.as_str());Optional check for consistency across the repo:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "All rkyv_serialize calls and fork argument:"
rg --line-number 'rkyv_serialize\(' -A 1Length of output: 558
Use the provided fork name for serialization instead of hard-coding Feynman
Both crate/libzkp/src/tasks/chunk.rs and crate/libzkp/src/tasks/bundle.rs currently serialize the witness with ForkName::Feynman. This will break cross-fork proofs if a different fork is requested. You need to:
• Extract the fork from value.fork_name before building the task
• Pass that fork into rkyv_serialize instead of ForkName::Feynman
Locations to update:
crates/libzkp/src/tasks/chunk.rs(around line 75)crates/libzkp/src/tasks/bundle.rs(around line 86)
Example diff for both files:
@@ before constructing the ProvingTask
- serialized_witness: vec![witness.rkyv_serialize(Some(ForkName::Feynman))?.to_vec()],
+ serialized_witness: vec![witness.rkyv_serialize(Some(fork))?.to_vec()],Make sure you add, just above the task construction in each file:
let fork = ForkName::from(value.fork_name.as_str());This ensures serialization always uses the fork specified by the caller.
🤖 Prompt for AI Agents
In crates/libzkp/src/tasks/chunk.rs at line 75, replace the hard-coded
ForkName::Feynman in the witness serialization with a dynamic fork value. First,
extract the fork by adding let fork = ForkName::from(value.fork_name.as_str());
just above the task construction, then pass this fork variable into
rkyv_serialize instead of ForkName::Feynman. This change ensures the
serialization respects the requested fork and prevents cross-fork proof issues.
Purpose or design rationale of this PR
using this commit scroll-tech/zkvm-prover@72f9abb
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Chores
Refactor