[Feat] Galileo forking #1753
[Feat] Galileo forking #1753
Conversation
WalkthroughProject version bumped to v4.7.1; workspace deps and crates pinned to tags; per-asset Changes
Sequence Diagram(s)sequenceDiagram
actor Coordinator
participant Verifier as Verifier Init
participant LibZKP as libzkp
participant ProverBin as Prover Bin
participant Provint as ProvintTaskExt
rect `#E8F0FF`
note right of Coordinator: Old flow (pre-change)
Coordinator->>Verifier: load VerifierConfig (features)
Verifier->>LibZKP: SetDynamicFeature(features)
LibZKP->>LibZKP: set global dynamic feature
end
rect `#E8FFE8`
note right of Coordinator: New flow (per-asset / per-task)
Coordinator->>Verifier: load AssetConfig (features)
Verifier->>LibZKP: verifier_init(circuits) — parse features into registry
LibZKP->>LibZKP: build FeatureOptions map (per-fork)
ProverBin->>Provint: deserialize input -> ProvintTaskExt (use_openvm_13)
Provint->>ProverBin: return (ProvingTask, TaskConfig)
ProverBin->>LibZKP: gen_universal_*_task(proving_task) (no fork param)
LibZKP->>LibZKP: precheck/into_proving_task_with_precheck -> (Info, pi_hash, ProvingTask)
LibZKP->>ProverBin: return (pi_hash, metadata, proving_task)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-16T02:09:51.657ZApplied to files:
🧬 Code graph analysis (4)crates/libzkp/src/tasks/chunk.rs (3)
crates/libzkp/src/tasks/batch.rs (6)
crates/libzkp/src/tasks.rs (3)
crates/libzkp/src/tasks/bundle.rs (3)
⏰ 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). (4)
🔇 Additional comments (21)
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. Comment |
| ) | ||
|
|
||
| var tag = "v4.6.3" | ||
| var tag = "v4.7.1" |
| /// defination in zkvm-prover's types | ||
| /// We distinguish the header type in golang side according to the codec | ||
| /// version, i.e. v6 - v9 (current), and validium | ||
| /// And adapt it to different header version used in zkvm-prover's witness | ||
| /// defination, i.e. v6- v8 (current), and validium |
There was a problem hiding this comment.
| /// defination in zkvm-prover's types | |
| /// We distinguish the header type in golang side according to the codec | |
| /// version, i.e. v6 - v9 (current), and validium | |
| /// And adapt it to different header version used in zkvm-prover's witness | |
| /// defination, i.e. v6- v8 (current), and validium | |
| /// definition in zkvm-prover's types | |
| /// We distinguish the header type in golang side according to the codec | |
| /// version, i.e. v7 - v9 (current), and validium | |
| /// And adapt it to the corresponding header version used in zkvm-prover's witness | |
| /// definition, i.e. v7 - v8 (current), and validium |
|
|
||
| // additional config dispatched with proving task | ||
| #[derive(Debug, Default)] | ||
| pub(crate) struct TaskConfig { |
There was a problem hiding this comment.
very wierd, I think ProvintTaskExt is enough, don't need to have a new TaskConfig
|
e2e test (for galileo blocks with guest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
common/version/version.go (1)
8-8: Verify the version jump from 4.6.3 to 4.7.1.Skipping version 4.7.0 is unusual. Was this intentional, or should it be 4.7.0 instead?
Cargo.toml (1)
17-17: Version bump aligns with project-wide update.The version bump to 4.7.1 is consistent with the version update in
common/version/version.go. However, as noted in another comment, skipping 4.7.0 is unusual and should be verified as intentional.
🧹 Nitpick comments (9)
tests/prover-e2e/sepolia-galileo/config.json (1)
4-4: Test environment credentials are acceptable; refactoring to env vars is optional.The hardcoded credentials in this test config are intentional. The docker-compose.yml explicitly marks this setup as "for local debugging only" and binds PostgreSQL exclusively to localhost (127.0.0.1:5432). The credentials match the docker-compose environment variables, confirming this is part of the designed local test infrastructure. Since this is a self-contained local-only setup with no production exposure, the hardcoded "dev:dev" credentials are acceptable and consistent across all prover-e2e test configs.
Migrating to environment variables would be a nice refactoring improvement, but is not required for this context.
crates/prover-bin/src/prover.rs (1)
275-303: Handler cache ignores TaskConfig; fine today, but fragile if config ever varies by taskWith the new
(prover_task, task_cfg)tuple, you correctly threadtask_cfgintoUniversalHandler::new. However, the handler cache is still keyed only byvk, so any subsequent task with the samevkwill reuse the existing handler regardless of itsTaskConfig.Given the current design (per-fork features → constant
use_openvm_13per circuit/vk), that’s effectively safe. But if you ever introduce task-level variability foris_openvm_v13(or more TaskConfig fields), the cache will silently give you a handler initialized with stale config.To make this more robust, consider keying the cache by
(vk, is_openvm_v13)or asserting thattask_cfg.is_openvm_v13matches when reusing a handler:- let handler = if let Some(handler) = self.handlers.get(&vk) { + let handler = if let Some(handler) = self.handlers.get(&vk) { handler.clone() } else { … let circuits_handler = Arc::new(Mutex::new(UniversalHandler::new(&asset_path, &task_cfg)?)); self.handlers.insert(vk, circuits_handler.clone()); circuits_handler };(If you keep single-key cache, a runtime assert comparing the stored config vs new
task_cfgwould at least fail loudly if the invariant is ever broken.)crates/libzkp/src/lib.rs (3)
37-56: FeatureOptions parsing is fine; consider skipping empty segments to avoid noisy logs
FeatureOptions::newwalks a colon-separated list and logs a warning for any unrecognized token. If the config ever provides an empty string or trailing colon (e.g."legacy_witness:"), you’ll end up with an “unrecognized dynamic feature” warning for the empty segment.Not a correctness issue, but you can cheaply avoid noise:
- for feat_s in feats.split(':') { - match feat_s.trim().to_lowercase().as_str() { + for feat_s in feats.split(':') { + let feat_s = feat_s.trim(); + if feat_s.is_empty() { + continue; + } + match feat_s.to_lowercase().as_str() { "legacy_witness" => { … } "openvm_13" => { … } s => tracing::warn!("unrecognized dynamic feature: {s}"), } }Overall, the feature registry wiring in
verifier_init(buildingADDITIONAL_FEATURESfromcfg.circuits) looks solid.Also applies to: 247-265
136-193: Version/fork consistency checks and ProvintTaskExt wrapping look good; minor error-message polishThe enhancements in
gen_universal_taskdo a few important things correctly:
- Normalize
task.fork_nameto lowercase and enforce that it matches thefork_name_strargument.- Independently decode
Version::from(task.version)and enforce thatversion.forkagrees with the fork argument as well.- Wrap the resulting
u_taskintoProvintTaskExtand apply per-forkfor_openvm_13_proverfromADDITIONAL_FEATURES, giving a single source of truth for OpenVM 13 flags.This substantially hardens task sanity checking and feature propagation.
The only nit: the panic-catch error message is
"caught panic in chunk task{e}"for all three task types (chunk/batch/bundle); if you care about log clarity, you might want to either use task-specific labels or a generic"caught panic in universal task {e}".Also applies to: 199-218
58-60: set_dynamic_feature is now deprecated; confirm all callers are truly obsoleteTurning
set_dynamic_featureinto a no-op that just logs an error is consistent with moving to config-time features viaverifier_init. However, any existing FFI or external callers relying on runtime feature toggling will now silently stop working apart from the log message.If that’s intentional, consider documenting this deprecation clearly (e.g., in release notes or header comments) and, where feasible, removing or gating the C FFI export in a future breaking release so that consumers don’t assume it still has effect.
crates/libzkp/src/tasks/chunk.rs (1)
212-244: Avoid hard-codingVersion::euclid_v2()in deprecated interpreter pathIn
prepare_task_via_interpret, using a fixedVersion::euclid_v2()when callingbuild_guest_inputmay diverge fromself.versionif this helper is ever used for non-EuclidV2 chunks (e.g., Feynman/Galileo or validium), which could subtly change witness construction during the retry loop.Unless this method is only ever invoked for EuclidV2 tasks, it would be safer and simpler to derive the version from the task itself:
- loop { - let witness = self.build_guest_input(Version::euclid_v2()); + loop { + let witness = self.build_guest_input(Version::from(self.version));This keeps the deprecated path behavior consistent with other call sites.
crates/libzkp/src/tasks/bundle.rs (1)
49-76: Bundle guest input + precheck now correctly version-aware with per-version pi-hash
build_guest_input(&self, version: Version)constructingBundleWitnessandprecheck_and_build_metadataderiving(BundleInfo, pi_hash_by_version(version)), pluscheck_aggregation_proofs(witness.batch_infos, Version::from(self.version)), aligns bundle behavior with batch/chunk. This ensures the pi-hash and adjacency validation are both driven by the same version.If you want a micro-cleanup, you could reuse the local
versionincheck_aggregation_proofsto avoid a secondVersion::from(self.version)call, but it’s not significant.crates/libzkp/src/tasks.rs (2)
38-59:ProvintTaskExtwrapper cleanly adds theuse_openvm_13flag while preserving task shapeFlattening
ProvingTaskintoProvintTaskExtand adding ause_openvm_13bool with a default offalseis a neat way to extend the serialized format without breaking existing consumers. TheFrom<ProvintTaskExt> for ProvingTaskplusProvintTaskExt::new(task)helpers are minimal and sufficient.If you expect frequent construction from plain
ProvingTask, you might also consider animpl From<ProvingTask> for ProvintTaskExtas a minor ergonomics improvement, but it’s optional.
91-105: Bundle universal task: duplicatedbundle_pi_hashreturn vs metadata seems intentional but worth double-checking
gen_universal_bundle_tasknow returnsbundle_pi_hashboth as the first tuple element and embedded inBundleProofMetadata { bundle_info, bundle_pi_hash }. This looks deliberate (e.g., convenience for callers that only care about the hash while keeping it in metadata for downstream uses), but it does mean the same value is carried twice.If there’s no compatibility constraint forcing both, you could consider dropping one of them; otherwise, please confirm callers rely on this shape as-is.
📜 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 (27)
Cargo.toml(1 hunks)common/version/version.go(1 hunks)coordinator/conf/config.json(1 hunks)coordinator/internal/config/config.go(1 hunks)coordinator/internal/logic/libzkp/lib.go(0 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(3 hunks)coordinator/internal/logic/verifier/verifier.go(2 hunks)crates/libzkp/Cargo.toml(1 hunks)crates/libzkp/src/lib.rs(3 hunks)crates/libzkp/src/proofs.rs(0 hunks)crates/libzkp/src/tasks.rs(3 hunks)crates/libzkp/src/tasks/batch.rs(8 hunks)crates/libzkp/src/tasks/bundle.rs(3 hunks)crates/libzkp/src/tasks/chunk.rs(5 hunks)crates/libzkp/src/verifier.rs(1 hunks)crates/prover-bin/Cargo.toml(1 hunks)crates/prover-bin/src/prover.rs(2 hunks)crates/prover-bin/src/zk_circuits_handler/universal.rs(2 hunks)tests/prover-e2e/Makefile(3 hunks)tests/prover-e2e/cloak-xen/.make.env(1 hunks)tests/prover-e2e/prepare/dump_block_records.sql(1 hunks)tests/prover-e2e/sepolia-feynman/.make.env(1 hunks)tests/prover-e2e/sepolia-feynman/genesis.json(1 hunks)tests/prover-e2e/sepolia-galileo/.make.env(1 hunks)tests/prover-e2e/sepolia-galileo/config.json(1 hunks)tests/prover-e2e/sepolia-galileo/genesis.json(1 hunks)zkvm-prover/config.json.template(1 hunks)
💤 Files with no reviewable changes (2)
- coordinator/internal/logic/libzkp/lib.go
- crates/libzkp/src/proofs.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T02:09:51.657Z
Learnt from: noel2004
Repo: scroll-tech/scroll PR: 1736
File: crates/libzkp/src/verifier/universal.rs:35-45
Timestamp: 2025-09-16T02:09:51.657Z
Learning: The verify_stark_proof method in scroll-zkvm-verifier returns Result<(), Error> indicating success/failure, not Result<bool, Error>. When it succeeds, verification passed; when it fails, it returns an error.
Applied to files:
crates/libzkp/src/tasks/bundle.rscrates/libzkp/src/tasks/batch.rs
🧬 Code graph analysis (9)
coordinator/internal/logic/provertask/batch_prover_task.go (1)
coordinator/internal/utils/version.go (1)
Version(16-40)
crates/prover-bin/src/zk_circuits_handler/universal.rs (2)
crates/libzkp/src/lib.rs (1)
new(38-55)crates/libzkp/src/tasks.rs (1)
new(53-58)
crates/prover-bin/src/prover.rs (1)
crates/prover-bin/src/zk_circuits_handler/universal.rs (2)
get_task_from_input(46-53)new(25-38)
crates/libzkp/src/lib.rs (5)
coordinator/internal/types/auth.go (1)
HardForkName(24-24)crates/libzkp/src/tasks.rs (2)
new(53-58)from(47-49)crates/libzkp_c/src/lib.rs (1)
set_dynamic_feature(281-284)crates/libzkp/src/utils.rs (1)
panic_catch(43-53)crates/libzkp/src/verifier.rs (1)
init(61-83)
crates/libzkp/src/verifier.rs (2)
coordinator/internal/config/config.go (1)
VerifierConfig(76-79)coordinator/internal/types/auth.go (1)
HardForkName(24-24)
crates/libzkp/src/tasks/chunk.rs (3)
crates/libzkp/src/lib.rs (1)
witness_use_legacy_mode(16-27)crates/libzkp/src/tasks/batch.rs (3)
build_guest_input(142-282)precheck_and_build_metadata(284-298)try_from(118-138)crates/libzkp/src/tasks/bundle.rs (3)
build_guest_input(49-60)precheck_and_build_metadata(62-76)try_from(82-102)
crates/libzkp/src/tasks/bundle.rs (4)
crates/libzkp/src/tasks/batch.rs (3)
build_guest_input(142-282)precheck_and_build_metadata(284-298)try_from(118-138)crates/libzkp/src/tasks/chunk.rs (3)
build_guest_input(159-177)precheck_and_build_metadata(183-190)try_from(100-116)crates/libzkp/src/tasks.rs (2)
from(47-49)check_aggregation_proofs(24-36)crates/libzkp/src/lib.rs (1)
witness_use_legacy_mode(16-27)
crates/libzkp/src/tasks.rs (3)
crates/libzkp/src/lib.rs (1)
new(38-55)crates/prover-bin/src/zk_circuits_handler/universal.rs (1)
new(25-38)crates/libzkp/src/verifier/universal.rs (1)
new(19-26)
crates/libzkp/src/tasks/batch.rs (5)
coordinator/internal/utils/version.go (1)
Version(16-40)crates/libzkp/src/tasks/bundle.rs (2)
build_guest_input(49-60)precheck_and_build_metadata(62-76)crates/libzkp/src/tasks/chunk.rs (2)
build_guest_input(159-177)precheck_and_build_metadata(183-190)crates/libzkp/src/lib.rs (1)
witness_use_legacy_mode(16-27)crates/libzkp/src/tasks.rs (2)
from(47-49)check_aggregation_proofs(24-36)
🪛 Checkov (3.2.334)
tests/prover-e2e/sepolia-galileo/config.json
[medium] 4-5: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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). (3)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (37)
zkvm-prover/config.json.template (1)
28-32: Configuration addition looks good.The new
galileocircuit entry follows the established pattern from thefeynmanconfiguration and uses appropriate S3 paths and workspace directories for the new hard fork. JSON syntax is valid and properly structured.tests/prover-e2e/prepare/dump_block_records.sql (1)
28-28: Verify the block range aligns with Galileo fork timing on Scroll testnet.The change shifts the block range from early blocks (1-49) to a specific Galileo-era range (20278000-20278050). While this appears intentional for Galileo fork testing, confirm that:
- Block 20278000 is the correct Galileo fork block number on the Scroll testnet environment used by e2e tests
- This block range contains representative blocks needed for comprehensive Galileo fork testing
Additionally, note that the PR checklist indicates regression e2e tests for Feynman blocks (v0.5.2) are still pending. Clarify whether this change replaces Feynman block testing entirely or if a separate configuration should handle both Feynman and Galileo block ranges.
tests/prover-e2e/sepolia-feynman/.make.env (1)
3-3: LGTM. Straightforward environment variable addition for codec version selection.tests/prover-e2e/sepolia-galileo/.make.env (1)
1-3: LGTM. New test environment configuration follows established patterns and aligns codec version (9) with Galileo fork designation.tests/prover-e2e/cloak-xen/.make.env (1)
2-3: LGTM. Codec version 8 correctly assigned for cloak-xen (consistent with pre-Galileo fork tests).tests/prover-e2e/sepolia-galileo/genesis.json (1)
1-110: LGTM. Valid genesis configuration file with proper fork activation sequencing (feynman at 1751271989, galileo at 1751271990) enabling boundary-condition testing.tests/prover-e2e/sepolia-feynman/genesis.json (1)
1-109: LGTM. Genesis configuration correctly omits galileoTime (fork not relevant for feynman-only regression tests). Structure is consistent with paired galileo configuration.tests/prover-e2e/Makefile (4)
12-12: Verify backward compatibility:prepare_dbnow part ofalltarget.Adding
prepare_dbto thealltarget introduces a database migration reset on every run. If developers or CI pipelines rely on incremental builds without full database resets, this is a breaking change. Consider documenting the new workflow or creating a separate target (e.g.,all_fresh) for full resets.
46-48: LGTM. Newprepare_dbtarget cleanly encapsulates migration reset logic; good separation of concerns.
55-65: Verify CODEC_VERSION availability in genericimport_datatarget.The generic
import_datatarget (lines 64–65) depends on${CODEC_VERSION}being set from.make.env. Ensure:
- CODEC_VERSION is always defined in the linked conf/.make.env.
- A sensible default or error message is provided if CODEC_VERSION is missing.
- Documentation or help text clarifies that users should use explicit targets (
import_data_feynman,import_data_galileo) or ensure CODEC_VERSION is set.The explicit codec targets (
import_data_euclid,import_data_feynman,import_data_galileo) are a good safeguard, but the genericimport_datashould be resilient.
67-68: LGTM.reimport_datatarget provides convenient full-reset workflow by chainingprepare_dbandimport_data.coordinator/internal/config/config.go (1)
65-73: LGTM! Feature configuration moved to asset level.The addition of the
Featuresfield toAssetConfigaligns with the PR's objective of supporting per-fork feature configuration, enabling more granular control over features at the asset level rather than the verifier level.crates/libzkp/src/verifier.rs (2)
42-49: LGTM! Features field added with proper defaults.The addition of the
featuresfield toCircuitConfigwithserde(default)ensures backward compatibility when the field is absent in older configurations. This aligns with the per-fork feature handling introduced across the PR.
56-56: LGTM! Visibility adjustment for internal usage.Changing
HardForkNametopub(crate)appropriately limits its visibility to within the crate, supporting the centralized per-fork handling while preventing unnecessary public exposure.Cargo.toml (3)
50-62: LGTM! Consistent patch updates to tagged releases.All revm-related crate patches have been updated from rev-based references to the
scroll-v91tag, ensuring consistent versioning across the dependency tree. This improves build reproducibility.
20-26: Dependency tags verified and accessible.Both tags exist in their upstream repositories:
v0.7.0-rc.4in scroll-tech/zkvm-prover ✓scroll-v91in scroll-tech/stateless-block-verifier ✓The update to tagged releases improves reproducibility as intended.
41-41: I'll use web search to verify the serde_with version constraint and available versions in the 3.x series.I need to inspect the codebase to see how serde_with is actually being used.
Based on my research, serde_with's latest version is 3.16.0, released just days ago. The version constraint change from
3.11.0to3is compatible because:
- Recent releases between 3.15.1 to 3.16.0 show only additions of new features with no breaking changes.
- Semantic versioning dictates that within major version 3, all minor and patch releases are backward compatible. The constraint
3allows any version from 3.0.0 through 3.16.0+.- Projects in the Rust ecosystem (like iceberg-rust) routinely update to the latest 3.x version without issues.
The change from specifying
3.11.0to3is a standard practice that follows semantic versioning and is safe. All features available in 3.11.0 are preserved in newer 3.x versions.
crates/libzkp/src/tasks/batch.rs (6)
29-48: LGTM! BatchHeaderV enum updated to support Galileo fork.The restructured
BatchHeaderVenum now properly handles STF versions V7, V8, and V9 using the sameBatchHeaderV7variant, as the codec for these versions is identical. The documentation clearly explains this design decision.
76-81: LGTM! Accessor method renamed for clarity.The method name
must_v7_v8_v9_headeraccurately reflects that it now handles V7, V8, and V9 STF versions uniformly. The error message also clearly indicates the expected range.
142-165: Comprehensive version validation added.The runtime assertions ensure that the parsed header type matches the version byte, catching configuration errors early. The checks cover:
- Validium domain validation
- V6 fork matching for EuclidV1
- V7/V8/V9 header matching for EuclidV2/Feynman/Galileo
174-187: Verify codec-based blob padding logic.The code now uses padded blob bytes for Codec::V7 but not for Codec::V6. This is a critical difference that affects the challenge digest computation. Please verify that:
- V6 codec intentionally uses unpadded blob bytes
- V7 codec (covering STF V7/V8/V9) requires padding to N_BLOB_BYTES
Based on learnings and the code logic, this appears intentional, but it's worth confirming with the team or documentation that this codec-based branching correctly implements the da-codec specification differences between V6 and V7.
284-298: All callers properly handle the new return type.The verification confirms that all call sites of
precheck_and_build_metadatahave been correctly updated to destructure and handle the new(BatchInfo, B256)tuple return type. The changes ingen_universal_batch_task,gen_universal_chunk_task, andgen_universal_bundle_taskall properly destructure the tuple and use both values appropriately.
231-254: No changes required; ReferenceHeader::V7_V8_V9 variant is correctly used.The verification confirms that the
ReferenceHeader::V7_V8_V9variant exists in the scroll-zkvm-types dependency at tag v0.7.0-rc.4. The code is using the correct variant name, which is properly defined and consistently used throughout the zkvm-prover codebase.coordinator/conf/config.json (1)
11-21: LGTM! Per-fork feature configuration added.The
featuresfield has been added to the Feynman fork configuration with value"legacy_witness:openvm_13", while the Galileo fork has no features specified. This aligns with the per-fork feature system introduced in this PR and allows Feynman blocks to use legacy witness encoding with OpenVM 13 support.crates/prover-bin/Cargo.toml (1)
11-11: LGTM! Prover-bin now depends on libzkp.The addition of the local libzkp dependency enables prover-bin to utilize the extended task handling and feature system introduced in this PR. The use of a path dependency is appropriate for workspace members.
crates/libzkp/Cargo.toml (1)
12-12: Feature verification confirmed.The
scroll-compress-infofeature exists in sbv-primitives at the scroll-v91 tag and is properly defined with all required dependencies. The change is valid.coordinator/internal/logic/verifier/verifier.go (1)
28-33: Per-asset features wiring into rustCircuitConfig looks consistentThe new
Featuresfield onrustCircuitConfigand its population fromcfg.Featurescleanly align the Go-side verifier config with the RustCircuitConfig.featuressemantics. Usingjson:"features,omitempty"ensures we omit the field when no features are configured, which is what the RustOption<String>expects. No issues from this change alone.Also applies to: 47-52
coordinator/internal/logic/provertask/batch_prover_task.go (1)
297-347: Stricter codec version gating is reasonable; consider future-proofingRejecting unsupported
CodecVersionvalues up front and logging the successful decode makes the control flow clearer and avoids undefined behavior on unknown encodings. The behavior (bubble up as internal failure) is consistent with the rest of this method.One thing to keep in mind is that each time a new codec version is introduced in
da-codec, this switch must be updated; otherwise batches using the new codec will start failing with “Unsupported codec version”. That’s fine as long as adding a codec is always accompanied by updating this list, but it’s worth treating that as part of the rollout checklist.crates/prover-bin/src/zk_circuits_handler/universal.rs (1)
14-18: TaskConfig + per-task parsing cleanly propagateuse_openvm_13into the proverThe new
TaskConfigandget_task_from_inputflow look coherent:
ProvintTaskExtis deserialized once from input, converted intoProvingTask, and itsuse_openvm_13bit is captured inTaskConfig.UniversalHandler::newnow consumes&TaskConfigand wiresis_openvm_v13intoProverConfig, which is exactly where this flag belongs.The design keeps the prover’s runtime configuration separate from the task payload while staying minimal (single bool). I don’t see correctness issues here.
Also applies to: 25-34, 46-53
crates/libzkp/src/tasks/chunk.rs (3)
1-9: Version/public-inputs imports match new usageBringing
Version(and related public-inputs types) into this module is consistent with the new version-aware guest-input and metadata APIs; nothing to change here.
97-107: ChunkProvingTask → ProvingTask conversion correctly switched to version-aware witness + legacy toggleUsing
value.build_guest_input(value.version.into())and guarding serialization withwitness_use_legacy_mode(&value.fork_name)?cleanly aligns chunk behavior with batch/bundle: one source of truth for versioned witnesses, plus per-fork legacy encoding. No issues from a correctness standpoint.
159-190: Version-aware chunk witness + metadata/pi-hash handling looks soundThe new
build_guest_input(&self, version: Version)cleanly separates validium vs scroll paths and reuses the sameversionboth for constructing the witness and forChunkInfo::pi_hash_by_version(version)inprecheck_and_build_metadata. This keeps the pi-hash selection in lockstep with the actual witness encoding and matches the pattern used in batch and bundle.crates/libzkp/src/tasks/bundle.rs (2)
2-44: Bundle identifier viametadata.batch_info.batch_hashis consistent with new metadata layoutSwitching
identifier()to readbatch_hashthroughmetadata.batch_infofor the first/last proofs matches the updatedBatchProofMetadata/BundleInfostructure and still enforces non-empty bundles viaBUNDLE_SANITY_MSG. Looks correct.
79-103: BundleProvingTask → ProvingTask conversion matches the new version/legacy patternUsing
value.build_guest_input(value.version.into())andwitness_use_legacy_mode(&value.fork_name)?mirrors the updated chunk and batch conversions and keeps legacy vs modern encoding decisions centralized. No functional issues spotted.crates/libzkp/src/tasks.rs (3)
17-36: Sharedcheck_aggregation_proofsabstraction withMultiVersionPublicInputsis a good centralizationImporting
MultiVersionPublicInputs/Versionhere and routing all aggregation checks throughcheck_aggregation_proofs(usingw[1].validate(&w[0], version)insidepanic_catch) centralizes the “adjacent metadata validation per version” logic and keeps batch/bundle code slimmer. Implementation looks straightforward and correct.
61-76: Chunk universal task generation now correctly uses precomputed pi-hash
gen_universal_chunk_task’s switch tolet (chunk_info, pi_hash) = task.precheck_and_build_metadata()?;and returningpi_hashas the first tuple element keeps the universal interface the same while ensuring the hash comes fromChunkInfo::pi_hash_by_versionrather than any fork-name heuristics. The rest of the metadata (includingchunk_total_gas) is unchanged.
78-89: Batch universal task generation aligned with new(BatchInfo, pi_hash)metadata APIUsing
task.precheck_and_build_metadata()?to obtain(batch_info, batch_pi_hash)and returning(batch_pi_hash, BatchProofMetadata { batch_info }, proving_task)makes batch handling consistent with chunk and bundle: callers always get the per-version pi-hash alongside slimmed-down metadata.Just ensure all call sites that previously treated the leading
B256as “batch pi-hash” (or a differently named value) still interpret it correctly after this refactor.
| pub(crate) fn witness_use_legacy_mode(fork_name: &str) -> eyre::Result<bool> { | ||
| ADDITIONAL_FEATURES | ||
| .get() | ||
| .and_then(|features| features.get(fork_name)) | ||
| .map(|cfg| cfg.legacy_witness_encoding) | ||
| .ok_or_else(|| { | ||
| eyre::eyre!( | ||
| "can not found features setting for unrecognized fork {}", | ||
| fork_name | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Normalize fork_name before indexing ADDITIONAL_FEATURES to avoid case-sensitivity surprises
ADDITIONAL_FEATURES is populated with keys config.fork_name.to_lowercase() in verifier_init, but witness_use_legacy_mode currently looks up the entry with the raw fork_name argument:
pub(crate) fn witness_use_legacy_mode(fork_name: &str) -> eyre::Result<bool> {
ADDITIONAL_FEATURES
.get()
.and_then(|features| features.get(fork_name))
…
}This silently relies on all callers passing a lowercased fork name; otherwise you’ll get an “unrecognized fork” error even though features exist for that fork.
Safer to canonicalize here as well:
pub(crate) fn witness_use_legacy_mode(fork_name: &str) -> eyre::Result<bool> {
- ADDITIONAL_FEATURES
- .get()
- .and_then(|features| features.get(fork_name))
+ let canonical = fork_name.to_lowercase();
+ ADDITIONAL_FEATURES
+ .get()
+ .and_then(|features| features.get(&canonical))
.map(|cfg| cfg.legacy_witness_encoding)
.ok_or_else(|| {
eyre::eyre!(
"can not found features setting for unrecognized fork {}",
fork_name
)
})
}This keeps the external API forgiving while preserving lowercased keys internally.
Also applies to: 29-36, 247-265
🏁 Script executed:
#!/bin/bash
# Verify ADDITIONAL_FEATURES initialization and witness_use_legacy_mode implementation
cd /tmp/repo || exit 1
# Check lines 16-27 in crates/libzkp/src/lib.rs
echo "=== Lines 16-27 in crates/libzkp/src/lib.rs ==="
sed -n '16,27p' crates/libzkp/src/lib.rs
echo -e "\n=== Lines 29-36 in crates/libzkp/src/lib.rs ==="
sed -n '29,36p' crates/libzkp/src/lib.rs
echo -e "\n=== Lines 247-265 in crates/libzkp/src/lib.rs ==="
sed -n '247,265p' crates/libzkp/src/lib.rs
# Search for verifier_init and ADDITIONAL_FEATURES setup
echo -e "\n=== Searching for verifier_init definition ==="
rg -n "fn verifier_init" crates/libzkp/src/
echo -e "\n=== Searching for ADDITIONAL_FEATURES initialization ==="
rg -n "ADDITIONAL_FEATURES" crates/libzkp/src/ -A 3 -B 1Length of output: 59
🏁 Script executed:
#!/bin/bash
# Find and inspect the codebase
echo "=== Current directory ==="
pwd
ls -la | head -20
echo -e "\n=== Searching for libzkp crate ==="
find . -name "lib.rs" -path "*/libzkp/src/*" 2>/dev/null | head -5
echo -e "\n=== Checking if crates/libzkp/src/lib.rs exists ==="
if [ -f "crates/libzkp/src/lib.rs" ]; then
echo "File found. Inspecting lines 16-27:"
sed -n '16,27p' crates/libzkp/src/lib.rs
echo -e "\nLines 29-36:"
sed -n '29,36p' crates/libzkp/src/lib.rs
echo -e "\nLines 247-265:"
sed -n '247,265p' crates/libzkp/src/lib.rs
echo -e "\n=== Searching for ADDITIONAL_FEATURES ==="
rg -n "ADDITIONAL_FEATURES" crates/libzkp/src/ -A 3 -B 1
echo -e "\n=== Searching for verifier_init ==="
rg -n "fn verifier_init" crates/libzkp/src/ -A 10
fiLength of output: 4951
🏁 Script executed:
#!/bin/bash
# Get more context around line 204 and check for other ADDITIONAL_FEATURES usages
echo "=== Lines 195-210 (context for line 204) ==="
sed -n '195,210p' crates/libzkp/src/lib.rs
echo -e "\n=== Search for all .get(fork_name) or .get(&fork_name) patterns ==="
rg -n "\.get\(" crates/libzkp/src/lib.rs -B 2 -A 1 | grep -E "(fork|ADDITIONAL_FEATURES)" -B 2 -A 1
echo -e "\n=== Lines 170-210 to see full context of fork_name normalization ==="
sed -n '170,210p' crates/libzkp/src/lib.rs
echo -e "\n=== Search for callers of witness_use_legacy_mode ==="
rg -n "witness_use_legacy_mode" crates/ -B 1 -A 1Length of output: 4242
🏁 Script executed:
#!/bin/bash
# Check if fork_name in task values is normalized before witness_use_legacy_mode is called
echo "=== Check bundle.rs context around line 84 ==="
sed -n '70,95p' crates/libzkp/src/tasks/bundle.rs
echo -e "\n=== Check batch.rs context around line 120 ==="
sed -n '105,130p' crates/libzkp/src/tasks/batch.rs
echo -e "\n=== Check chunk.rs context around line 102 ==="
sed -n '90,110p' crates/libzkp/src/tasks/chunk.rs
echo -e "\n=== Search for where fork_name is set in task structs ==="
rg -n "fork_name\s*=" crates/libzkp/src/tasks/ -B 1 -A 1 | head -40
echo -e "\n=== Check if fork_name is normalized in task construction ==="
rg -n "\.to_lowercase\(\)" crates/libzkp/src/tasks/ -B 2 -A 1Length of output: 2908
🏁 Script executed:
#!/bin/bash
# Get the complete match statement with all task types
echo "=== Lines 140-195 to see all task type cases ==="
sed -n '140,195p' crates/libzkp/src/lib.rs
echo -e "\n=== Search for all fork_name normalizations in lib.rs ==="
rg -n "to_lowercase" crates/libzkp/src/lib.rs -B 2 -A 2Length of output: 4597
Normalize fork_name in witness_use_legacy_mode to prevent case-sensitivity bugs
The function at line 19 looks up fork_name against ADDITIONAL_FEATURES without lowercasing, creating an inconsistency:
- ADDITIONAL_FEATURES keys are lowercased at population (line 256)
- Line 204 normalizes fork_name before lookup:
features.get(&fork_name)(where fork_name is pre-normalized) - Line 19 does not normalize:
features.get(fork_name)(relying on caller normalization)
While current callers (tasks/bundle.rs:84, tasks/batch.rs:120, tasks/chunk.rs:102) pass pre-normalized fork_names, the function is pub(crate) and should defend against future callers that may not normalize. This matches the defensive pattern already used at line 204.
pub(crate) fn witness_use_legacy_mode(fork_name: &str) -> eyre::Result<bool> {
+ let canonical = fork_name.to_lowercase();
ADDITIONAL_FEATURES
.get()
- .and_then(|features| features.get(fork_name))
+ .and_then(|features| features.get(&canonical))
.map(|cfg| cfg.legacy_witness_encoding)
.ok_or_else(|| {
eyre::eyre!(
"can not found features setting for unrecognized fork {}",
fork_name
)
})
}🤖 Prompt for AI Agents
In crates/libzkp/src/lib.rs around lines 16 to 27, the function
witness_use_legacy_mode looks up ADDITIONAL_FEATURES using the raw fork_name
which can cause case-sensitivity bugs; normalize fork_name (e.g., create a
lowercased String or &str from fork_name.to_lowercase()) before calling
features.get(...) so it matches the lowercased keys populated into
ADDITIONAL_FEATURES, and use that normalized value in the lookup and error
message to ensure the function is defensive against unnormalized callers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/libzkp/src/proofs.rs (1)
256-260: Usingpi_hash_euclidv1aligns test metadata with new PI-hash APIInitializing
bundle_pi_hashwithbundle_info.pi_hash_euclidv1()matches the new per-fork PI-hash helper and keeps the dummy metadata consistent with Euclid‑v1 semantics. The rest of the test still only validates proof/vk serialization, so behavior is unchanged.If you want slightly stronger coverage, you could also assert at the end of the test that
bundle_proof.metadata.bundle_pi_hash == bundle_proof.metadata.bundle_info.pi_hash_euclidv1()to catch accidental mismatches in future refactors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/libzkp/src/proofs.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T02:09:51.657Z
Learnt from: noel2004
Repo: scroll-tech/scroll PR: 1736
File: crates/libzkp/src/verifier/universal.rs:35-45
Timestamp: 2025-09-16T02:09:51.657Z
Learning: The verify_stark_proof method in scroll-zkvm-verifier returns Result<(), Error> indicating success/failure, not Result<bool, Error>. When it succeeds, verification passed; when it fails, it returns an error.
Applied to files:
crates/libzkp/src/proofs.rs
⏰ 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). (3)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (1)
crates/libzkp/src/proofs.rs (1)
218-218: Import narrowing in tests looks correctThe test module now only imports
BundleInfoandEvmProoffromscroll_zkvm_types, which matches what is actually used in this file and keeps the dependency surface minimal. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
Cargo.toml(1 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). (3)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (5)
Cargo.toml (5)
17-17: Version bump 4.6.3 → 4.7.1 appears intentional for Galileo feature release.The skip from 4.7.0 to 4.7.1 aligns with the PR objective. Confirm this does not conflict with any prior 4.7.0 tag or release.
20-22: Tag v0.7.0-rc.5 verified in zkvm-prover repository.The release tag exists and is accessible. Migration from revision to tag-based dependencies is sound and improves reproducibility.
24-26: Tag scroll-v91 verified in stateless-block-verifier repository.The HTTP 200 response confirms the tag exists and is accessible. The migration from
masterbranch to tag-based pinning improves reproducibility. Feature flags are correctly preserved:sbv-primitivesandsbv-coreretain theirscrollfeatures, withsbv-primitivesalso includingrkyv.
34-34: alloy-primitives 1.4.1 update is backwards compatible.There are no breaking changes listed for alloy-primitives v1.4.1. The micro bump from 1.3 to 1.4.1 is a safe, backwards-compatible update, and the
tiny-keccakfeature flag remains properly configured.
50-62: Verify if op-revm should also be patched with scroll-v91 tag.The
scroll-v91tag exists and is accessible. However, the patch section contains only 12 revm crates (not 13 as stated). The workspace at scroll-v91 includes 13 published crates, butop-revmis not patched. Confirm whether this omission is intentional—ifop-revmrequires Scroll-specific changes or depends on patched revm crates, it should likely also referencescroll-v91.
| serde_json = { version = "1.0" } | ||
| serde_derive = "1.0" | ||
| serde_with = "3.11.0" | ||
| serde_with = "3" |
There was a problem hiding this comment.
🧩 Analysis chain
The change from "3.11.0" (exact pin) to "3" (minor version) allows any 3.x release. This loosens reproducibility guarantees and could allow silent updates to incompatible patch versions. Confirm this is intentional and that 3.x releases maintain API compatibility.
If exact pinning is required for reproducibility (recommended for a production prover), revert to:
-serde_with = "3"
+serde_with = "3.11.0"Otherwise, document why serde_with's patch-level flexibility is acceptable in this context.
I'll help you verify this review comment. Let me examine the Cargo.toml file and research the serde_with version compatibility.
Let me examine the Cargo.toml file directly to verify the current state and check the project context.
🏁 Script executed:
#!/bin/bash
# Examine Cargo.toml for serde_with version and project context
cat Cargo.toml | head -50
echo "---"
grep -A2 -B2 "serde_with" Cargo.tomlLength of output: 1733
🏁 Script executed:
#!/bin/bash
# Look for project docs/context mentioning reproducibility or version pinning
find . -type f \( -name "README.md" -o -name "CONTRIBUTING.md" -o -name ".github" \) 2>/dev/null | head -20Length of output: 246
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or docs related to version constraints in Cargo.toml
cat Cargo.toml | grep -i "version\|lock\|pin\|reproducib" Length of output: 688
Confirm version constraint loosening is intentional or revert to exact pin.
The version constraint serde_with = "3" allows any version >=3.0.0, <4.0.0, whereas the previous "3.11.0" was an exact pin. This change does loosen reproducibility guarantees. For a production blockchain prover, reproducible builds require a lockfile to record the state of the world at build time, but relying only on Cargo.lock is fragile.
The rest of the Cargo.toml shows a mixed strategy: some dependencies use exact versions (e.g., metrics = "0.23.0", serde_json = "1.0"), while others permit minor-version flexibility (e.g., serde = "1", alloy = "1"). This inconsistency suggests the change may have been unintentional.
Either pin to "3.11.0" for consistency with the project's critical dependencies or document why serde_with's minor-version flexibility is acceptable here.
🤖 Prompt for AI Agents
Cargo.toml line 41: the dependency serde_with is loosened to "3" which permits
any 3.x version; either revert it to the previous exact pin "3.11.0" to maintain
reproducible builds or, if loosening is intentional, add an inline comment above
the dependency explaining why minor-version flexibility is acceptable and ensure
Cargo.lock is committed and documented in the repo policy; make the change
accordingly and run cargo update/verify to confirm the lockfile state.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
coordinator/Makefile (1)
40-45: Use consistent path variables in Makefile conditionals.The conditional block mixes
$(PWD)(line 41) and$(CURDIR)(line 43), which is unreliable. In Makefiles,$(CURDIR)is the standard and safer choice across all contexts, whereas$(PWD)is shell-dependent and may be unset in nested invocations.Apply this diff to use
$(CURDIR)consistently:-@if [ -f "$(PWD)/conf/config.template.json" ]; then \ - SRC="$(PWD)/conf/config.template.json"; \ +@if [ -f "$(CURDIR)/conf/config.template.json" ]; then \ + SRC="$(CURDIR)/conf/config.template.json"; \tests/prover-e2e/cloak-xen/config.template.json (1)
20-22: Credentials in test template—acceptable context but document intent.Checkov flags the PostgreSQL DSN credentials (
dev:dev) as a security concern. This is a test template with placeholder values, so the flag is a false positive for local/test environments. However, to prevent future confusion and align with secure-by-default principles, consider adding a brief comment clarifying that these are placeholder credentials intended for local development setup.Add an inline comment above the db section (e.g.,
// Template with placeholder credentials for local development only.) to document the intent.coordinator/internal/logic/provertask/batch_prover_task.go (2)
331-331: Consider Debug level for operational detail logs.This Info log will fire on every batch task decode. In high-throughput production environments, this may generate excessive log volume. Unless this information is needed for operational monitoring, Debug level would be more appropriate for implementation details.
Apply this diff if you prefer Debug level:
- log.Info("Decode batchheader bytes to canonical header", "version", batchHeader.Version()) + log.Debug("Decode batchheader bytes to canonical header", "version", batchHeader.Version())
342-342: Consider Debug level for configuration detail logs.Changing this log from Debug to Info means it will appear for every validium batch task. Since this represents a configuration/mode detail rather than an actionable event, Debug level may be more appropriate to reduce log verbosity in production.
Apply this diff if you prefer Debug level:
- log.Info("Apply validium mode for batch proving task") + log.Debug("Apply validium mode for batch proving task")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
coordinator/Makefile(1 hunks)coordinator/build/setup_releases.sh(2 hunks)coordinator/conf/config.json(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(3 hunks)tests/prover-e2e/Makefile(2 hunks)tests/prover-e2e/cloak-xen/.make.env(1 hunks)tests/prover-e2e/cloak-xen/config.template.json(1 hunks)tests/prover-e2e/sepolia-feynman/.make.env(1 hunks)tests/prover-e2e/sepolia-feynman/config.template.json(1 hunks)tests/prover-e2e/sepolia-galileo/.make.env(1 hunks)tests/prover-e2e/sepolia-galileo/config.template.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/prover-e2e/sepolia-galileo/.make.env
- coordinator/conf/config.json
- tests/prover-e2e/sepolia-feynman/.make.env
- tests/prover-e2e/Makefile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-15T08:52:44.176Z
Learnt from: colinlyguo
Repo: scroll-tech/scroll PR: 1645
File: rollup/proposer-tool-config.json:34-40
Timestamp: 2025-04-15T08:52:44.176Z
Learning: In configuration files like `rollup/proposer-tool-config.json`, placeholders such as `<mainnet read db config>` are intentionally left as-is to be replaced by users with their own configuration values when deploying the tool.
Applied to files:
tests/prover-e2e/sepolia-galileo/config.template.json
🧬 Code graph analysis (1)
coordinator/internal/logic/provertask/batch_prover_task.go (1)
coordinator/internal/utils/version.go (1)
Version(16-40)
🪛 Checkov (3.2.334)
tests/prover-e2e/sepolia-galileo/config.template.json
[medium] 21-22: Basic Auth Credentials
(CKV_SECRET_4)
tests/prover-e2e/sepolia-feynman/config.template.json
[medium] 22-23: Basic Auth Credentials
(CKV_SECRET_4)
tests/prover-e2e/cloak-xen/config.template.json
[medium] 21-22: Basic Auth Credentials
(CKV_SECRET_4)
⏰ 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). (3)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (8)
coordinator/build/setup_releases.sh (1)
46-46: Fork filtering logic and logging are well-aligned.The extraction of
fork_namefrom config and the fork-aware logging provide good visibility into which verifier is being processed. This aligns well with the broader per-fork feature handling introduced in the PR.Also applies to: 63-63, 68-68
tests/prover-e2e/cloak-xen/.make.env (1)
2-4: LGTM! Configuration aligns with Feynman regression testing.The additions of
CODEC_VERSION?=8andSCROLL_FORK_NAME=feynmanproperly configure this test fixture for Feynman fork regression testing, which aligns with the PR's checklist item "regression e2e tests: for feynman blocks."Note:
SCROLL_FORK_NAMEintentionally uses=instead of?=, ensuring this test always runs against the Feynman fork regardless of environment overrides, which is appropriate for a dedicated test fixture.tests/prover-e2e/sepolia-feynman/config.template.json (2)
13-15: Configuration correctly reflects per-asset features schema.The template includes the
featuresfield (line 13), which aligns with the PR objective to move per-asset features into configuration and populate them into libzkp at init. The combination offeatures,assets_path, andfork_nameproperly models the new configuration structure.
20-22: Credentials in test template—acceptable context.Checkov flags the PostgreSQL DSN credentials, which is a false positive for this test template. The placeholder credentials (dev:dev) are appropriate for local/test environments.
tests/prover-e2e/sepolia-galileo/config.template.json (3)
12-16: Verify whether thefeaturesfield should be included for Galileo fork.This template omits the
featuresfield that is present in thesepolia-feynmantemplate (lines 13 in that file). Given the PR's focus on per-asset feature handling and the aligned schema in the feynman template, confirm whether Galileo requires different feature flags or if this is an unintended omission.
13-14: Verifyassets_pathnaming convention across forks.The
assets_pathis set to generic"assets"here, whereas thesepolia-feynmantemplate uses"assets_feynman". Confirm whether this difference is intentional based on Galileo fork requirements, or if it should be"assets_galileo"for consistency and clarity.
19-23: Credentials in test template—acceptable context.Checkov flags the PostgreSQL DSN credentials, which is a false positive for this test template. The placeholder credentials (dev:dev) are appropriate for local/test environments.
coordinator/internal/logic/provertask/batch_prover_task.go (1)
317-320: LGTM: Explicit error handling for unsupported codec versions.The explicit error return for unsupported codec versions (instead of falling through) improves error visibility and prevents silent failures.
| # default fork name from env or "galileo" | ||
| SCROLL_FORK_NAME="${SCROLL_FORK_NAME:-galileo}" |
There was a problem hiding this comment.
Add validation to detect when no verifier matches SCROLL_FORK_NAME.
The fork filtering logic is well-structured, but the script exits cleanly even if all verifiers are skipped (i.e., none match SCROLL_FORK_NAME). This could result in silent failures where no assets are downloaded, making it difficult to diagnose configuration mismatches.
Add a counter and validation after the loop to ensure at least one verifier was processed:
+PROCESSED_COUNT=0
+
for ((i=0; i<$VERIFIER_COUNT; i++)); do
# extract assets_path for current verifier
ASSETS_PATH=$(jq -r ".prover_manager.verifier.verifiers[$i].assets_path" "$CONFIG_FILE")
FORK_NAME=$(jq -r ".prover_manager.verifier.verifiers[$i].fork_name" "$CONFIG_FILE")
# skip if this verifier's fork doesn't match the target fork
if [ "$FORK_NAME" != "$SCROLL_FORK_NAME" ]; then
echo "Expect $SCROLL_FORK_NAME, skip current fork ($FORK_NAME)"
continue
fi
+ PROCESSED_COUNT=$((PROCESSED_COUNT + 1))
if [ "$ASSETS_PATH" = "null" ]; then
echo "Warning: Could not find assets_path for verifier $i, skipping..."
continue
fi
# ... rest of loop ...
done
+if [ "$PROCESSED_COUNT" -eq 0 ]; then
+ echo "Error: No verifiers found matching fork '$SCROLL_FORK_NAME'"
+ exit 1
+fi
+
echo "All verifier assets downloaded successfully"Also applies to: 33-33, 35-39
🤖 Prompt for AI Agents
In coordinator/build/setup_releases.sh around lines 9-10 (and likewise where the
verifier loop exists near lines 33 and 35-39), the script can silently exit when
no verifier matches SCROLL_FORK_NAME; add a processed counter increment inside
the verifier loop whenever a verifier is selected/handled, then after the loop
validate that counter > 0 and if not print a clear error to stderr and exit
non-zero (e.g., echo error "no verifier matched
SCROLL_FORK_NAME=$SCROLL_FORK_NAME" >&2; exit 1) to fail fast and avoid silent
no-op runs.
| case 0: | ||
| log.Warn("the codec version is 0, if it is not under integration test we have encountered an error here") | ||
| return taskDetail, nil |
There was a problem hiding this comment.
🧩 Analysis chain
Verify downstream handling of incomplete taskDetail for codec version 0.
Returning taskDetail early means BatchHeader, ChallengeDigest, KzgProof, and KzgCommitment remain uninitialized (nil/zero values). If this path is triggered outside integration tests (as the warning suggests shouldn't happen), downstream prover code may encounter nil pointer dereferences or invalid proof generation.
Consider either:
- Returning an explicit error if version 0 should never occur in production, or
- Verifying that the prover and all downstream consumers gracefully handle a taskDetail with these nil fields
Run the following script to check how taskDetail fields are accessed after being returned:
🏁 Script executed:
#!/bin/bash
# Description: Find usages of BatchTaskDetail fields that may be nil when version is 0
# Search for accesses to BatchHeader field
echo "=== BatchHeader field access ==="
rg -nP --type=go -C3 'taskDetail\.BatchHeader|BatchTaskDetail.*BatchHeader' -g '!**/test/**'
# Search for accesses to ChallengeDigest field
echo -e "\n=== ChallengeDigest field access ==="
rg -nP --type=go -C3 'taskDetail\.ChallengeDigest|BatchTaskDetail.*ChallengeDigest' -g '!**/test/**'
# Search for accesses to KzgProof field
echo -e "\n=== KzgProof field access ==="
rg -nP --type=go -C3 'taskDetail\.KzgProof|BatchTaskDetail.*KzgProof' -g '!**/test/**'Length of output: 3376
🏁 Script executed:
#!/bin/bash
# Description: Find how getBatchTaskDetail is called and how its return value is used
echo "=== Callers of getBatchTaskDetail ==="
rg -nP --type=go 'getBatchTaskDetail\s*\(' -C5 -g '!**/test/**'
echo -e "\n=== How taskDetail is used after returned ==="
# Look for accesses to taskDetail fields in caller contexts
rg -nP --type=go -B2 'taskDetail\.(BatchHeader|ChallengeDigest|KzgProof|KzgCommitment)' -C2 -g '!**/test/**' | head -50Length of output: 4441
🏁 Script executed:
#!/bin/bash
# Description: Check codec version constants and what version 0 means
echo "=== Codec version constants or enum ==="
rg -nP --type=go 'CodecVersion.*0|version.*0.*=' -C3 -g '!**/test/**' | head -40
echo -e "\n=== Batch structure definition ==="
rg -nP --type=go -B5 'type.*Batch.*struct|CodecVersion' -C2 | head -60Length of output: 6936
🏁 Script executed:
#!/bin/bash
# Description: Find the version() function and understand codec version mapping
echo "=== version() function definition ==="
rg -nP --type=go -A15 'func.*version\s*\(' -C2 | grep -A15 'func (bp \*BatchProverTask) version'
echo -e "\n=== Codec version mapping (fork name to version) ==="
rg -nP --type=go 'CodecV0|forking.*hardFork|forkName.*codec|version.*codec' -C3 -g '!**/test/**' | head -80Length of output: 5484
🏁 Script executed:
#!/bin/bash
# Description: Check if downstream prover handles nil BatchHeader/ChallengeDigest/KzgProof
# Search for prover code that processes BatchTaskDetail
echo "=== Prover uses of BatchTaskDetail ==="
rg -nP --type=go 'BatchTaskDetail' -C4 -g '!**/test/**' | head -100Length of output: 3090
🏁 Script executed:
#!/bin/bash
# Description: Find version() method in BatchProverTask
echo "=== version method (broader search) ==="
rg -nP --type=go 'def.*version|func.*version' coordinator/internal/logic/provertask/ -C5
echo -e "\n=== All methods in batch_prover_task.go ==="
rg -nP --type=go '^\s*func \(bp' coordinator/internal/logic/provertask/batch_prover_task.goLength of output: 1617
🏁 Script executed:
#!/bin/bash
# Description: Check fork name to codec version mapping
echo "=== Fork/hardFork name definitions ==="
rg -nP --type=go 'hardForkName|forkName.*=|ForkleName.*=' -C3 -g '!**/test/**' | head -100
echo -e "\n=== Version mapping in code ==="
rg -nP --type=go '"euclidv|galileo|fork.*case' -C2 -g '!**/test/**' | head -80Length of output: 11294
🏁 Script executed:
#!/bin/bash
# Description: Verify if version 0 can reach production data
echo "=== Verify version 0 handling context ==="
cat coordinator/internal/logic/provertask/batch_prover_task.go | sed -n '297,352p'Length of output: 2566
Return an error instead of silently accepting codec version 0.
The code logs a warning about codec version 0 but then returns an incomplete taskDetail with nil BatchHeader, ChallengeDigest, KzgProof, and KzgCommitment fields. This incomplete struct is then JSON marshaled and sent to the prover, which may not handle null values gracefully.
Since the warning explicitly states this shouldn't occur outside integration tests, either:
- Add an explicit error return for codec version 0 in production scenarios, or
- Document that downstream code must handle null/empty values for these fields and verify it does
File: coordinator/internal/logic/provertask/batch_prover_task.go:314-316
Replace the early return with:
return nil, fmt.Errorf("codec version 0 is not supported in production")Or if version 0 must be handled (e.g., for legacy data), ensure all required fields are properly initialized before returning.
🤖 Prompt for AI Agents
In coordinator/internal/logic/provertask/batch_prover_task.go around lines
314-316, the code currently logs a warning for codec version 0 then returns an
empty taskDetail which causes downstream JSON to contain null fields; replace
this early-return behavior with an explicit error return (e.g., return nil,
fmt.Errorf("codec version 0 is not supported in production")) and add the fmt
import, or if codec version 0 must be supported, fully initialize BatchHeader,
ChallengeDigest, KzgProof, and KzgCommitment with valid legacy defaults before
returning so downstream code never receives null values.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1753 +/- ##
===========================================
- Coverage 36.56% 36.47% -0.09%
===========================================
Files 247 247
Lines 21191 21191
===========================================
- Hits 7748 7730 -18
- Misses 12615 12637 +22
+ Partials 828 824 -4
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:
|
| dbBatchCodecVersion := encoding.CodecVersion(dbBatch.CodecVersion) | ||
| switch dbBatchCodecVersion { | ||
| case 0: | ||
| log.Warn("the codec version is 0, if it is not under integration test we have encountered an error here") |
There was a problem hiding this comment.
Does this mean we use codec=0 for integration tests?
There was a problem hiding this comment.
I think it should never be 0 We only use v0 for genesis batch (which is not proven)
| pub fn set_dynamic_feature(_: &str) { | ||
| tracing::error!("call deprecated method"); | ||
| } | ||
|
|
There was a problem hiding this comment.
can we just remove this from libzkp? And all occurrences of it from the scroll monorepo?
| static ADDITIONAL_FEATURES: OnceLock<HashMap<HardForkName, FeatureOptions>> = OnceLock::new(); | ||
|
|
||
| impl FeatureOptions { | ||
| pub fn new(feats: &str) -> Self { |
There was a problem hiding this comment.
Why not instead:
impl FromStr for FeatureOptions| pub(crate) fn witness_use_legacy_mode() -> bool { | ||
| unsafe { LEGACY_WITNESS_ENCODING } | ||
| #[derive(Debug, Default, Clone)] | ||
| struct FeatureOptions { |
There was a problem hiding this comment.
If I'm not wrong: legacy_witness <=> openvm_13 ?
| } | ||
|
|
||
| #[derive(serde::Deserialize, serde::Serialize)] | ||
| pub struct ProvintTaskExt { |
There was a problem hiding this comment.
Maybe you mean ProvingTaskExt ? We should fix this typo
|
|
||
| pub fn get_task_from_input(input: &str) -> Result<ProvingTask> { | ||
| Ok(serde_json::from_str(input)?) | ||
| pub fn get_task_from_input(input: &str) -> Result<(ProvingTask, TaskConfig)> { |
There was a problem hiding this comment.
Why not
| pub fn get_task_from_input(input: &str) -> Result<(ProvingTask, TaskConfig)> { | |
| pub fn get_task_from_input(input: &str) -> Result<ProvingTaskExt> { |
| if decodeErr != nil { | ||
| return nil, fmt.Errorf("failed to decode batch header version %d: %w", dbBatch.CodecVersion, decodeErr) | ||
| } | ||
| log.Info("Decode batchheader bytes to canonical header", "version", batchHeader.Version()) |
| .map(|cfg| cfg.legacy_witness_encoding) | ||
| .ok_or_else(|| { | ||
| eyre::eyre!( | ||
| "can not found features setting for unrecognized fork {}", |
There was a problem hiding this comment.
Do you mean "cannot find feature settings"?
| "login_expire_duration_sec": 3600 | ||
| }, | ||
| "sequencer": { | ||
| "decryption_key": "ad25ba5de9f03fd9e3c81a1ad731c5c7e94ae2641f75ed7d3275312053b40f75" |
There was a problem hiding this comment.
Why do you commit a public network's secret into GitHub.....
This PR induced works required by galileo forking
v0.7.0-rcguestsregression e2e tests:
v0.5.2guests4.6.1andv0.5.2guestsSummary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.