-
Notifications
You must be signed in to change notification settings - Fork 623
chore(zkvm-circuit): upgrade to 0.1.0-rc.6 #1610
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
chore(zkvm-circuit): upgrade to 0.1.0-rc.6 #1610
Conversation
WalkthroughThis pull request comprises extensive updates across multiple modules. The changes include version bumps in Go and Rust toolchains, dependency upgrades, new Dockerfiles, and significant restructuring in proof handling logic. Notably, the darwin verifiers were removed in favor of a new EuclidVerifier, and several interfaces and data types were updated from pointer to value semantics. Additionally, a new Rust-based zkvm-prover project was introduced alongside configuration and build script enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant L as libzkp Interface
participant EU as EuclidVerifier
participant FS as File System
C->>L: Call dump_vk(fork_name, file)
L->>EU: _dump_vk(fork_name, file)
EU->>FS: Write verification keys to file
FS-->>EU: File written
EU-->>L: Return verification keys
L-->>C: Complete dump_vk call
sequenceDiagram
participant CLI as Prover CLI
participant MP as Main (zkvm-prover)
participant CP as Config Parser
participant LP as LocalProver Service
CLI->>MP: Parse command-line arguments
MP->>CP: Load configuration file
CP-->>MP: Return configuration data
MP->>LP: Initialize LocalProver with config
LP->>LP: Set active circuit handler based on hard fork
LP->>LP: Process prove request asynchronously
LP-->>MP: Return prove response
MP-->>CLI: Output proof result
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package zstd: could not load export data: no export data for "github.com/scroll-tech/da-codec/encoding/zstd"" ✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## omerfirmak/euclid-prover #1610 +/- ##
============================================================
- Coverage 42.12% 42.12% -0.01%
============================================================
Files 222 222
Lines 17739 17739
============================================================
- Hits 7473 7472 -1
- Misses 9557 9558 +1
Partials 709 709
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
common/libzkp/impl/src/verifier.rs (1)
47-59
: 💡 Verification agent🧩 Analysis chain
Simplified verifier initialization.
The initialization logic has been significantly simplified, directly creating an EuclidVerifier from the assets path rather than the previous complex configuration process. This makes the code more maintainable.
Let's check if VERIFIER_HIGH is populated correctly by examining the usage:
🏁 Script executed:
#!/bin/bash # Check if VERIFIER_HIGH is used properly in the codebase rg "VERIFIER_HIGH" common/libzkp/impl/src/Length of output: 234
Action Required: Verify Global Verifier Initialization
The simplified initialization appears intended to create an instance of
EuclidVerifier
using the provided assets path and associate it with the high-version verifier. However, while the static variable declared isVERIFIER_HIGH
, theinit
function is settingVERIFIER_LOW
. Therg
search confirmed thatVERIFIER_HIGH
is indeed referenced in a conditional check (if let Some(verifier) = VERIFIER_HIGH.get() {...}
), but it isn’t being initialized in the shown snippet.Please review and correct the inconsistency by ensuring that either:
- The initialization sets
VERIFIER_HIGH
if that’s the intended global, or- Update the conditional check to reference
VERIFIER_LOW
if that is correct.
🧹 Nitpick comments (32)
coordinator/internal/orm/batch.go (1)
86-87
: Add a code comment explaining the codec_version filter.You've added a filter to exclude batches with
codec_version = 5
in the SQL query. This is likely related to the zkvm-circuit upgrade to 0.1.0-rc.6, but the purpose of this filter isn't immediately clear from the code.Consider adding a comment explaining why this specific codec version is being excluded:
sql := fmt.Sprintf("SELECT * FROM batch WHERE proving_status = %d AND total_attempts < %d AND active_attempts < %d AND chunk_proofs_status = %d AND codec_version != 5 AND batch.deleted_at IS NULL ORDER BY batch.index LIMIT 1;", + // Excluding codec_version 5 as it's not compatible with zkvm-circuit 0.1.0-rc.6 int(types.ProvingTaskUnassigned), maxTotalAttempts, maxActiveAttempts, int(types.ChunkProofsStatusReady))
coordinator/internal/orm/chunk.go (1)
77-78
: Add a code comment explaining the codec_version filter.Similar to the batch.go file, you've added a filter to exclude chunks with
codec_version = 5
in the SQL query. The purpose of this filter isn't immediately clear without additional context.Consider adding a comment explaining why this specific codec version is being excluded:
sql := fmt.Sprintf("SELECT * FROM chunk WHERE proving_status = %d AND total_attempts < %d AND active_attempts < %d AND end_block_number <= %d AND codec_version != 5 AND chunk.deleted_at IS NULL ORDER BY chunk.index LIMIT 1;", + // Excluding codec_version 5 as it's not compatible with zkvm-circuit 0.1.0-rc.6 int(types.ProvingTaskUnassigned), maxTotalAttempts, maxActiveAttempts, height)
common/libzkp/interface/libzkp.h (1)
11-12
: Ensure adequate documentation for new API functionThe newly added
dump_vk
function lacks documentation comments explaining its purpose, parameters, and expected usage. Consider adding documentation to maintain API clarity.char verify_chunk_proof(char* proof, char* fork_name); +// dump_vk writes the verification key for the specified fork to the given file path +// Parameters: +// - fork_name: Name of the fork to dump the verification key for +// - file: File path to write the verification key to void dump_vk(char* fork_name, char* file);rollup/internal/config/l2.go (1)
50-50
: Good addition of batch chunk limiting configurationThe addition of
MaxChunksPerBatch
provides important control over batch processing in the rollup system, which aligns well with the zkvm-circuit upgrade goals.Consider adding a brief comment explaining the purpose of this parameter and any recommended value ranges.
MaxUncompressedBatchBytesSize uint64 `json:"max_uncompressed_batch_bytes_size"` +// MaxChunksPerBatch limits the number of chunks that can be included in a single batch MaxChunksPerBatch int `json:"max_chunks_per_batch"`
zkvm-prover/print_halo2gpu_version.sh (1)
13-13
: Improve regex pattern robustnessThe grep pattern for extracting the halo2gpu path might be fragile if the config file format changes.
Consider using a more robust approach, such as:
-halo2gpu_path=$(grep -Po '(?<=paths = \[")([^"]*)' $config_file) +halo2gpu_path=$(awk -F'"' '/paths = \[/{print $2}' $config_file)Or better yet, use a package like
toml
orcargo metadata
to parse the configuration properly.zkvm-prover/src/zk_circuits_handler.rs (1)
1-12
: New CircuitsHandler trait looks good, but missing documentationThe trait definition is well-structured and follows Rust best practices using async_trait, but it lacks documentation comments.
Consider adding rustdoc comments to document the trait and its methods:
+/// Handler for ZK circuit operations +/// +/// This trait defines the interface for handling verification keys and proof generation +/// operations for different proof types. #[async_trait] pub trait CircuitsHandler: Sync + Send { + /// Retrieves the verification key for the specified proof type + /// + /// # Arguments + /// * `task_type` - The type of proof to get the verification key for + /// + /// # Returns + /// The verification key as a byte vector, or None if not available async fn get_vk(&self, task_type: ProofType) -> Option<Vec<u8>>; + /// Generates proof data for the given prove request + /// + /// # Arguments + /// * `prove_request` - The request containing all necessary information for proof generation + /// + /// # Returns + /// The proof data as a string, or an error if proof generation fails async fn get_proof_data(&self, prove_request: ProveRequest) -> Result<String>; }build/dockerfiles/prover.Dockerfile (3)
15-15
: Specify target directory for source code copyThe COPY command doesn't specify a destination directory, which means it will copy to the current working directory (likely the root). This could lead to unexpected behavior.
-COPY ./zkvm-prover . +COPY ./zkvm-prover /app +WORKDIR /app
17-17
: Consider capturing build errorsThe cargo build command doesn't capture or handle potential build failures explicitly.
-RUN cargo build --release +RUN cargo build --release && echo "Build completed successfully"
19-23
: Consider security best practices for runtime containerThe runtime container runs as root by default and lacks security constraints.
Consider adding:
FROM ubuntu:24.04 AS runtime COPY --from=builder /app/target/release/prover /usr/local/bin/ +# Create non-root user +RUN useradd -m prover +USER prover + +# Set health check +HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 CMD [ "prover", "--version" ] + ENTRYPOINT ["prover"]zkvm-prover/Cargo.toml (1)
30-47
: Consider organizing dependencies by purposeThe dependencies section would benefit from organization by purpose (e.g., networking, crypto, serialization) with comments explaining each group's purpose. This would improve maintainability as the project grows.
zkvm-prover/src/main.rs (2)
28-47
: Improve error handling and shutdown processThe main function runs the prover but doesn't handle the result from
prover.run()
. Consider these improvements:
- Properly handle the result from
run()
- Implement graceful shutdown on signals
- Add more detailed error logging
#[tokio::main] async fn main() -> anyhow::Result<()> { init_tracing(); + + // Set up signal handlers for graceful shutdown + let shutdown = tokio::signal::ctrl_c(); let args = Args::parse(); if args.version { println!("version is {}", get_version()); std::process::exit(0); } let cfg = LocalProverConfig::from_file(args.config_file)?; let sdk_config = cfg.sdk_config.clone(); let local_prover = LocalProver::new(cfg); let prover = ProverBuilder::new(sdk_config, local_prover).build().await?; - prover.run().await; + // Run the prover and handle any errors + match prover.run().await { + Ok(_) => log::info!("Prover completed successfully"), + Err(e) => { + log::error!("Prover failed: {}", e); + return Err(e.into()); + } + } Ok(()) }
12-26
: Consider enhancing CLI argument handlingThe CLI arguments are minimal but functional. Consider adding:
- Environment variable fallbacks for configuration options
- Validation for configuration file existence
- More descriptive help messages
#[derive(Parser, Debug)] #[clap(disable_version_flag = true)] struct Args { /// Path of config file - #[arg(long = "config", default_value = "conf/config.json")] + #[arg( + long = "config", + default_value = "conf/config.json", + env = "PROVER_CONFIG", + help = "Path to the prover configuration JSON file" + )] config_file: String, /// Version of this prover - #[arg(short, long, action = ArgAction::SetTrue)] + #[arg( + short, + long, + action = ArgAction::SetTrue, + help = "Display the prover version and exit" + )] version: bool, /// Path of log file - #[arg(long = "log.file")] + #[arg( + long = "log.file", + env = "PROVER_LOG_FILE", + help = "Path to store logs (if not specified, logs to stdout)" + )] log_file: Option<String>, }coordinator/internal/logic/provertask/bundle_prover_task.go (1)
224-231
: Changed from pointer semantics to value semantics for batch proofs.The code now uses value semantics ([]message.BatchProof) instead of pointer semantics ([]*message.BatchProof) for batch proofs. This change reduces indirection and potentially simplifies memory management.
Ensure this change is consistent across all related code to avoid mixing pointer and value semantics. Run tests to verify the behavior remains consistent.
common/libzkp/impl/src/verifier/euclid.rs (4)
1-14
: Introduce documentation comments for EuclidVerifier.
Adding doc comments for the struct fields (e.g., chunk, batch, and bundle verifiers) can help others understand their roles faster.
16-31
: Review setup paths for EuclidVerifier.
The constructor sets up three verifiers from the provided paths. If these paths can be user-defined, consider adding validation or error handling for missing or invalid files in lieu of panicking.
33-53
: Handle potential JSON errors more gracefully.
When deserializing proofs, capturing serde errors distinctly (rather than unwrapping) can deliver clearer feedback to callers.
55-64
: Consider alternative VK encoding or encryption.
Base64 encoding is convenient, but if there's concern about sensitive data, evaluating a different format or encrypting the file might be beneficial.zkvm-prover/src/zk_circuits_handler/euclid.rs (2)
12-16
: Consider adding documentation for each prover field.Providing a brief comment for
chunk_prover
,batch_prover
, andbundle_prover
fields can help new contributors quickly understand their responsibilities and relationships with the rest of the codebase.
60-92
: Avoid blocking concurrency with a single mutex.Multiple proof requests for “Chunk,” “Batch,” or “Bundle” tasks will contend on the same lock. If performance or parallelism is critical, consider per-prover locks or a lock-free design to reduce blocking.
coordinator/internal/logic/verifier/mock.go (2)
19-24
: Use consistent logging or comments for mock behavior.When returning a mock verification result, logging or clarifying comments can help other maintainers quickly grasp that these verifications are placeholders. Consistent messaging avoids confusion across different mock methods.
27-32
: Check the performance impact of repeated string conversion.Casting the proof to string (
string(proof.Proof())
) is fine for a mock, but if proofs grow large, it might have overhead. It's minor in this mock code but worth noting if the logic expands in the future.coordinator/internal/logic/verifier/verifier.go (2)
60-64
: Document howrustVkDump
values are produced.Since
dump_vk
presumably writes a JSON file containing these fields, a comment clarifying the origin and structure ofchunk_vk
,batch_vk
, andbundle_vk
supports maintainability.
102-109
: Ensure error handling coverage forloadLowVersionVKs
andloadOpenVMVks
.Breaking changes in these methods (e.g., invalid file paths) might propagate. Verify you handle or log errors thoroughly, especially given reliance on external file I/O and FFI calls.
coordinator/internal/logic/provertask/batch_prover_task.go (1)
218-239
: Updated proof handling to use value semanticsThe code now uses value semantics instead of pointer semantics for chunk proofs, consistent with the changes in other files. The type check for
ChunkInfo
access has been improved to use proper type assertion.Consider adding an error check for the type assertion:
- if haloProot, ok := proof.(*message.Halo2ChunkProof); ok { - if haloProot.ChunkInfo != nil { - chunkInfo.TxBytes = haloProot.ChunkInfo.TxBytes - } + if haloProot, ok := proof.(*message.Halo2ChunkProof); ok { + if haloProot.ChunkInfo != nil { + chunkInfo.TxBytes = haloProot.ChunkInfo.TxBytes + } + } else { + log.Debug("chunk proof is not a Halo2ChunkProof", "chunk_hash", chunk.Hash) }rollup/internal/controller/relayer/l2_relayer.go (2)
208-211
: Improve error diagnostic with contextual details
Currently, the returned error does not include contextual information (e.g., chunk vs. block). Including it in the error message or logs would greatly aid debugging.208 if err = r.l2BlockOrm.InsertL2Blocks(r.ctx, chunk.Blocks); err != nil { - return fmt.Errorf("failed to insert genesis block: %v", err) + return fmt.Errorf("failed to insert genesis block for chunk %d: %w", + 0, // or chunk index if available + err, + ) }
530-556
: Fix typographical error
A minor typo is present in the log statement at line 544 ("firsr"). This can be distracting and should be corrected in all affected logs.- log.Error("failed to get firsr unfinalized chunk", "chunk index", firstUnfinalizedBatch.StartChunkIndex) + log.Error("failed to get first unfinalized chunk", "chunk index", firstUnfinalizedBatch.StartChunkIndex)zkvm-prover/src/prover.rs (4)
23-41
: Consider adding config validations
For production environments, consider validating required fields inLocalProverConfig
(e.g., missing circuit entries).
49-55
: Single active proving task
Storingcurrent_task
inLocalProver
suggests only one proving job can run at a time. If multiple concurrent tasks are desired later, plan for additional concurrency or scheduling logic.
57-131
: Implementation of the ProvingService trait
The flow ofprove
→do_prove
andquery_task
is straightforward. However, be mindful that a second.prove()
call while a first is in progress would overwritecurrent_task
.
133-190
: Concurrency approach
Usingtokio::task::spawn_blocking
is a well-established way to offload CPU-bound proof logic. Just confirm that blocking tasks don’t hamper performance if you plan to scale or handle multiple simultaneous proofs.common/types/message/message.go (2)
49-52
: Consider replacinginterface{}
with a more specific type.
Usinginterface{}
forBatchHeader
can lead to extra type assertions or reflection overhead. If there's a known structure or set of possible structures, a more explicit type might improve code safety and readability.
246-253
: Consider a non-panic error handling strategy.
Currently, marshaling errors trigger a panic. Replace it with an error return or a logged error to prevent unexpected process termination.func (p *OpenVMChunkProof) Proof() []byte { - proofJson, err := json.Marshal(p.VmProof) - if err != nil { - panic(fmt.Sprint("marshaling error", err)) - } + proofJson, err := json.Marshal(p.VmProof) + if err != nil { + // handle error gracefully, e.g. return nil or log the error + return nil + } return proofJson }
🛑 Comments failed to post (9)
zkvm-prover/print_halo2gpu_version.sh (3)
20-20: 🛠️ Refactor suggestion
Add error handling for directory operations
Similar to the previous comment, the
popd
command should include error handling.-popd +popd || { echo "Error: Unable to return to previous directory"; exit 1; }📝 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.popd || { echo "Error: Unable to return to previous directory"; exit 1; }
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 20-20: Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
(SC2164)
15-15: 🛠️ Refactor suggestion
Add error handling for directory operations
The
pushd
command lacks error handling which could lead to unexpected behavior if the directory doesn't exist or can't be accessed.-pushd $halo2gpu_path +pushd $halo2gpu_path || { echo "Error: Cannot change directory to $halo2gpu_path"; exit 1; }📝 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.pushd $halo2gpu_path || { echo "Error: Cannot change directory to $halo2gpu_path"; exit 1; }
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 15-15: Use 'pushd ... || exit' or 'pushd ... || return' in case pushd fails.
(SC2164)
17-18: 🛠️ Refactor suggestion
Add error handling for git operations
The git command lacks error handling if the directory is not a git repository or if the command fails for other reasons.
-commit_hash=$(git log --pretty=format:%h -n 1) -echo "${commit_hash:0:7}" +if ! commit_hash=$(git log --pretty=format:%h -n 1 2>/dev/null); then + echo "Error: Failed to get git commit hash" + exit 1 +fi +echo "${commit_hash:0:7}"📝 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.if ! commit_hash=$(git log --pretty=format:%h -n 1 2>/dev/null); then echo "Error: Failed to get git commit hash" exit 1 fi echo "${commit_hash:0:7}"
build/dockerfiles/prover.Dockerfile (1)
21-21:
⚠️ Potential issueFix source path for binary copy
The path to the built binary seems incorrect. Given the build context, it should be relative to the current directory.
-COPY --from=builder /target/release/prover /usr/local/bin/ +COPY --from=builder /app/target/release/prover /usr/local/bin/📝 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.COPY --from=builder /app/target/release/prover /usr/local/bin/
common/libzkp/impl/Cargo.toml (1)
17-18: 💡 Verification agent
🧩 Analysis chain
Confirm tag availability for Euclid-based crates.
Make sure the tagv0.1.0-rc.6
exists and is stable inscroll-tech/zkvm-prover.git
.
🏁 Script executed:
#!/bin/bash # Checking if tag v0.1.0-rc.6 is present in the GitHub repository for scroll-zkvm-prover. gh release view v0.1.0-rc.6 --repo scroll-tech/zkvm-prover || echo "Tag v0.1.0-rc.6 not found."Length of output: 141
Action Required: Update Euclid-based Crates Tag
The tag
v0.1.0-rc.6
specified incommon/libzkp/impl/Cargo.toml
could not be found in thescroll-tech/zkvm-prover.git
repository. Please update the tag reference to a valid and stable release to ensure the crates rely on an existing version.
- Location:
common/libzkp/impl/Cargo.toml
, lines 17–18.- Action: Verify the correct tag (e.g., by checking the repository's release list) and update both
euclid_prover
andeuclid_verifier
dependencies accordingly.zkvm-prover/src/zk_circuits_handler/euclid.rs (1)
49-57: 🛠️ Refactor suggestion
Handle potential lock poisoning gracefully.
This code calls
self.try_lock().unwrap()
, which will panic if the lock is poisoned. Consider usinglock().await
or handling the error more gracefully in production environments to avoid crashing the entire process.coordinator/internal/logic/verifier/verifier.go (1)
232-264:
⚠️ Potential issueVerify concurrency implications of writing and removing the temp file.
If this function is invoked concurrently for the same
forkName
, race conditions could arise when multiple threads read, write, and remove the same file. Consider including unique naming, or synchronizing calls if concurrency is possible.- tempFile := path.Join(os.TempDir(), "openVmVk.json") + tempFile := path.Join(os.TempDir(), fmt.Sprintf("openVmVk_%s.json", forkName))📝 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.func (v *Verifier) loadOpenVMVks(forkName string) error { tempFile := path.Join(os.TempDir(), fmt.Sprintf("openVmVk_%s.json", forkName)) defer func() { if err := os.Remove(tempFile); err != nil { log.Error("failed to remove temp file", "err", err) } }() forkNameCStr := C.CString(forkName) defer C.free(unsafe.Pointer(forkNameCStr)) tempFileCStr := C.CString(tempFile) defer C.free(unsafe.Pointer(tempFileCStr)) C.dump_vk(forkNameCStr, tempFileCStr) f, err := os.Open(tempFile) if err != nil { return err } byt, err := io.ReadAll(f) if err != nil { return err } var dump rustVkDump if err := json.Unmarshal(byt, &dump); err != nil { return err } v.OpenVMVkMap[dump.Chunk] = struct{}{} v.OpenVMVkMap[dump.Batch] = struct{}{} v.OpenVMVkMap[dump.Bundle] = struct{}{} return nil }
common/types/message/message.go (2)
356-382:
⚠️ Potential issueInsufficient length check for
Instances
.
Though you verifylen(pf.Proof)%32 == 0
, you do not confirmpf.Instances
has ≥384 bytes. This omission can cause a runtime slice error inProof()
.
324-354:
⚠️ Potential issuePotential out-of-bounds slice risk at line 351.
p.EvmProof.Instances[:384]
can panic iflen(p.EvmProof.Instances) < 384
. Add a length check inProof()
or within the sanity check logic before slicing.func (p *OpenVMBundleProof) Proof() []byte { + if len(p.EvmProof.Instances) < 384 { + // handle this error scenario, e.g. log or return a partial result + panic("Instances slice too short, must be at least 384 bytes") + } proofBytes := make([]byte, 0, 384+len(p.EvmProof.Proof)) proofBytes = append(proofBytes, p.EvmProof.Instances[:384]...) return append(proofBytes, p.EvmProof.Proof...) }📝 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.// OpenVMBundleProof includes the proof info that are required for verification of a bundle of batch proofs. type OpenVMBundleProof struct { MetaData struct { BundleInfo *OpenVMBundleInfo `json:"bundle_info"` BunndlePIHash common.Hash `json:"bundle_pi_hash"` } `json:"metadata"` EvmProof *OpenVMEvmProof `json:"proof"` Vk []byte `json:"vk,omitempty"` GitVersion string `json:"git_version,omitempty"` } // Proof returns the proof bytes that are eventually passed as calldata for on-chain bundle proof verification. // // There are 12 accumulators for a SNARK proof. The accumulators are the first 12 elements of the EvmProof's // Instances field. The remaining items in Instances are supplied on-chain by the ScrollChain contract. // // The structure of these bytes is: // | byte index start | byte length | value | description | // |------------------|----------------|----------|---------------------| // | 0 | 32 | accs[0] | accumulator 1 | // | 32 | 32 | accs[1] | accumulator 2 | // | 32*i ... | 32 | accs[i] | accumulator i ... | // | 352 | 32 | accs[11] | accumulator 12 | // | 384 | dynamic | proof | proof bytes | func (p *OpenVMBundleProof) Proof() []byte { if len(p.EvmProof.Instances) < 384 { // handle this error scenario, e.g. log or return a partial result panic("Instances slice too short, must be at least 384 bytes") } proofBytes := make([]byte, 0, 384+len(p.EvmProof.Proof)) proofBytes = append(proofBytes, p.EvmProof.Instances[:384]...) return append(proofBytes, p.EvmProof.Proof...) }
Purpose or design rationale of this PR
Related commit in
scroll-proving-sdk
: scroll-tech/scroll-proving-sdk@9524a42Summary by CodeRabbit
New Features
Improvements