[Feat] Galileo v2 #1771
[Feat] Galileo v2 #1771
Conversation
WalkthroughAdds GalileoV2 / CodecV10 support across coordinator, rollup, and prover; renames Rust batch header API to include V10 and updates accessors; removes a prover config field; bumps multiple Go/Rust dependency pins; adds sepolia-galileoV2 e2e environment and updates integration fetch/import tooling and test infra. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (10)
🔇 Additional comments (6)
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 |
|
local e2e test has passed with following configurations:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1771 +/- ##
===========================================
- Coverage 36.59% 36.49% -0.11%
===========================================
Files 247 248 +1
Lines 21239 21310 +71
===========================================
+ Hits 7773 7777 +4
- Misses 12637 12705 +68
+ Partials 829 828 -1
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
coordinator/internal/utils/version.go (1)
25-35: New galileov2 → stfVersion 10 mapping fits existing patternAdding:
case "galileo": stfVersion = 9 case "galileov2": stfVersion = 10after lowercasing
hardForkNamecleanly extends support for the GalileoV2 fork and stays consistent with the existing naming scheme. This should play nicely with config values like"galileoV2"thanks tostrings.ToLower.If this mapping grows further, consider hoisting these magic numbers into named constants or a small lookup map for easier maintenance, but it’s not blocking for this change.
Cargo.toml (1)
20-26: Consider pinning zkvm deps to a tag/commit instead of a feature branchPointing
scroll-zkvm-prover,scroll-zkvm-verifier, andscroll-zkvm-typesat thefeat/galileo-v2branch is fine while iterating, but it makes builds non‑reproducible once merged intodevelop. When the fork stabilizes, consider switching these to a tagged release or explicit commit hash; the sbv crates already follow that pattern withscroll-v91.2.tests/prover-e2e/sepolia-galileoV2/config.json (1)
2-13: DB DSN credentials are dev‑friendly but may trigger secret scannersThe hard‑coded DSN (
postgres://dev:dev@localhost:5432/...) is clearly for local/test use, but tools like Checkov will still flag it as a credential. If this path is scanned in CI and causes noise or failures, consider either:
- Moving the DSN to an environment variable that tests read, or
- Using a placeholder value here and letting your test harness substitute the real DSN.
From a functionality perspective, the
validium_mode: false+codec_version: 10wiring for the GalileoV2 e2e setup looks consistent.zkvm-prover/Makefile (1)
1-1: Addtest_e2e_run_gputo the.PHONYdeclaration.The new
test_e2e_run_gputarget should be declared as phony to prevent conflicts with any potential file of the same name and to ensure Make always executes the recipe.-.PHONY: prover prover_cpu lint tests_binary test_e2e_run test_run +.PHONY: prover prover_cpu lint tests_binary test_e2e_run test_e2e_run_gpu test_runtests/prover-e2e/Makefile (1)
12-14:.OPTIONALis non-standard; consider.SECONDARYor removing the directive.
.OPTIONALis not a standard GNU Make special target. If the intent is to prevent errors when no SQL files exist, thewildcardfunction already handles this gracefully by returning an empty string. Consider removing line 13 or using a more explicit approach.BLOCK_PRE_MIGRATIONS := $(wildcard conf/*.sql) -.OPTIONAL: $(BLOCK_PRE_MIGRATIONS) -If the goal is to mark these as intermediate files that shouldn't be deleted, use
.SECONDARYinstead:.SECONDARY: $(BLOCK_PRE_MIGRATIONS)crates/libzkp/src/tasks/batch.rs (1)
69-91: Accessor method renames improve clarity.The
to_zkvm_batch_header_*naming convention better conveys the transformation intent compared tomust_*. However, note thatto_zkvm_batch_header_v7_to_v9on line 76 handles V10 as well (per the match arm on line 78), so the name could be slightly misleading.Consider renaming for consistency with the enum variant:
- pub fn to_zkvm_batch_header_v7_to_v9(&self) -> &BatchHeaderV7 { + pub fn to_zkvm_batch_header_v7_to_v10(&self) -> &BatchHeaderV7 {rollup/tests/integration_tool/imports.go (1)
45-58: LGTM! Block import parameter addition is well-integrated.The new
blocksparameter and conditional insertion logic cleanly extends the import workflow to support externally-fetched blocks.Consider enhancing the log message for better observability:
if len(blocks) > 0 { - log.Info("import block") + log.Info("importing blocks", "count", len(blocks)) blockOrm := orm.NewL2Block(db) if err := blockOrm.InsertL2Blocks(ctx, blocks); err != nil { return nil, err } }rollup/tests/integration_tool/main.go (1)
171-177: Use camelCase for variable naming per Go conventions.The variable
import_blocksuses snake_case, which is not idiomatic Go.- var import_blocks []*encoding.Block + var importBlocks []*encoding.Block if cfg.FetchConfig != nil { - import_blocks, err = fetchAndStoreBlocks(ctx.Context, beginBlk, endBlk) + importBlocks, err = fetchAndStoreBlocks(ctx.Context, beginBlk, endBlk) if err != nil { return err } }Also update the call at line 192:
- ret, err := importData(ctx.Context, beginBlk, endBlk, import_blocks, chkNum, batchNum, bundleNum, seed) + ret, err := importData(ctx.Context, beginBlk, endBlk, importBlocks, chkNum, batchNum, bundleNum, seed)rollup/tests/integration_tool/block_fetching.go (1)
71-74: Inconsistent error variable naming.Use
errconsistently instead of introducingerr3:- withdrawRoot, err3 := ethCli.StorageAt(ctx, cfg.L2MessageQueueAddress, cfg.WithdrawTrieRootSlot, big.NewInt(int64(number))) - if err3 != nil { - return nil, fmt.Errorf("failed to get withdrawRoot: %v. number: %v", err3, number) + withdrawRoot, err := ethCli.StorageAt(ctx, fetchCfg.L2MessageQueueAddress, fetchCfg.WithdrawTrieRootSlot, big.NewInt(int64(number))) + if err != nil { + return nil, fmt.Errorf("failed to get withdrawRoot: %v. number: %v", err, number) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
Cargo.lockis excluded by!**/*.lockbridge-history-api/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcoordinator/go.sumis excluded by!**/*.sumdatabase/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sumtests/integration-test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
Cargo.toml(1 hunks)Makefile(1 hunks)bridge-history-api/go.mod(2 hunks)common/go.mod(2 hunks)coordinator/build/setup_releases.sh(1 hunks)coordinator/conf/config.json(1 hunks)coordinator/go.mod(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(1 hunks)coordinator/internal/utils/version.go(1 hunks)crates/libzkp/src/tasks/batch.rs(6 hunks)crates/prover-bin/src/prover.rs(0 hunks)database/go.mod(1 hunks)rollup/go.mod(1 hunks)rollup/internal/controller/blob_uploader/blob_uploader.go(1 hunks)rollup/internal/controller/relayer/l2_relayer.go(3 hunks)rollup/internal/utils/utils.go(1 hunks)rollup/tests/integration_tool/block_fetching.go(1 hunks)rollup/tests/integration_tool/imports.go(1 hunks)rollup/tests/integration_tool/main.go(7 hunks)tests/integration-test/go.mod(1 hunks)tests/prover-e2e/Makefile(3 hunks)tests/prover-e2e/cloak-xen/.make.env(0 hunks)tests/prover-e2e/cloak-xen/config.json(1 hunks)tests/prover-e2e/prepare/dump_block_records.sql(1 hunks)tests/prover-e2e/sepolia-feynman/.make.env(0 hunks)tests/prover-e2e/sepolia-feynman/config.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-galileoV2/.make.env(1 hunks)tests/prover-e2e/sepolia-galileoV2/config.json(1 hunks)tests/prover-e2e/sepolia-galileoV2/config.template.json(1 hunks)tests/prover-e2e/sepolia-galileoV2/genesis.json(1 hunks)zkvm-prover/Makefile(1 hunks)zkvm-prover/config.json.template(1 hunks)
💤 Files with no reviewable changes (3)
- tests/prover-e2e/cloak-xen/.make.env
- tests/prover-e2e/sepolia-feynman/.make.env
- crates/prover-bin/src/prover.rs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-18T06:49:24.796Z
Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1746
File: rollup/internal/controller/sender/sender.go:637-640
Timestamp: 2025-10-18T06:49:24.796Z
Learning: In the file `rollup/internal/controller/sender/sender.go`, the resubmission logic in `createReplacingTransaction` does not convert V0 blob sidecars to V1 when resubmitting transactions after the Fusaka upgrade. This is an accepted edge case because it's unlikely to occur, geth is expected to handle it gracefully, and manual recovery is available if needed.
Applied to files:
rollup/internal/controller/relayer/l2_relayer.go
📚 Learning: 2025-07-29T16:38:24.647Z
Learnt from: colinlyguo
Repo: scroll-tech/scroll PR: 1714
File: rollup/internal/controller/relayer/l2_relayer.go:1548-1555
Timestamp: 2025-07-29T16:38:24.647Z
Learning: In rollup/internal/controller/relayer/l2_relayer.go, the validateBatchFields function should error out when GetBatchByHash fails to find a parent batch. This is intentional behavior - missing parent batches represent genuine error conditions that should halt batch submission processing. Genesis batch handling occurs separately from normal batch validation flow.
Applied to files:
rollup/internal/controller/relayer/l2_relayer.go
📚 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-galileoV2/config.template.json
📚 Learning: 2024-10-20T15:30:18.084Z
Learnt from: colinlyguo
Repo: scroll-tech/scroll PR: 1530
File: tests/integration-test/go.mod:21-21
Timestamp: 2024-10-20T15:30:18.084Z
Learning: In the `tests/integration-test` folder, updates to import statements or dependencies are not necessary since it's only used for testing purposes.
Applied to files:
tests/integration-test/go.mod
🧬 Code graph analysis (3)
rollup/internal/controller/relayer/l2_relayer.go (3)
coordinator/internal/utils/codec_validium.go (1)
CodecVersion(10-10)rollup/internal/orm/batch.go (2)
Batch(25-75)Batch(83-85)coordinator/internal/orm/batch.go (2)
Batch(18-72)Batch(80-82)
rollup/tests/integration_tool/main.go (3)
database/config.go (1)
DBConfig(12-19)coordinator/internal/config/config.go (1)
Config(57-63)coordinator/internal/utils/codec_validium.go (1)
CodecVersion(10-10)
rollup/tests/integration_tool/imports.go (2)
common/database/db.go (1)
InitDB(48-80)database/config.go (1)
DBConfig(12-19)
🪛 Checkov (3.2.334)
tests/prover-e2e/sepolia-galileoV2/config.json
[medium] 4-5: Basic Auth Credentials
(CKV_SECRET_4)
tests/prover-e2e/sepolia-galileoV2/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). (9)
- GitHub Check: test
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (34)
tests/prover-e2e/prepare/dump_block_records.sql (1)
28-28: Block range update is correct for sepolia-galileoV2 test environment.The WHERE clause correctly filters for blocks 15206780–15206809 (30 blocks), which matches the test data in
tests/prover-e2e/sepolia-galileo/00100_import_blocks.sql. The old block range (20278000–20278050) has no remaining references in the codebase.coordinator/internal/logic/provertask/batch_prover_task.go (1)
312-320: CodecV10 acceptance in switch looks correct; please confirm BlobDataProof layout compatibilityAdding
encoding.CodecV10to the allowed set in this switch cleanly extends support for V10 in the non‑validium path and keeps the error handling behavior unchanged. This matches the PR’s objective to roll out CodecV10.One thing to double‑check: this path assumes the
BlobDataProofmemory layout (z, y, kzg_commitment, kzg_proof with the current hard‑coded byte offsets) is identical for all listed codec versions. Please confirm that CodecV10 uses the same layout; if it doesn’t, we’d need a version‑aware extraction instead of the current shared offsets.tests/prover-e2e/sepolia-galileo/.make.env (1)
1-2: No verification issues found. The block range insepolia-galileois independently configured test data. Each.make.envfile (galileo, galileoV2, feynman, cloak-xen) uses its own isolated block range from the respective testnet to populate the local e2e test database. Block numbers across different configurations do not need to follow any particular ordering.Likely an incorrect or invalid review comment.
Makefile (1)
3-3: L2GETH_TAG bump looks consistent; just confirm tag–commit alignment
L2GETH_TAGnow points toscroll-v5.9.17, which is used by theupdatetarget to pingithub.com/scroll-tech/go-ethereumacross modules. That’s aligned with the go-ethereum version bumps elsewhere; just make sure this tag actually corresponds to the same commit/pseudo-version you use in the variousgo.modfiles so you don’t get drift betweengo getand the checked-in versions.tests/prover-e2e/sepolia-feynman/config.json (1)
8-9: Explicit codec_version for Feynman is reasonable; ensure readers are updatedAdding
"codec_version": 8alongside"validium_mode": falsemakes the environment self-describing. Please double-check that whatever component parses this config now usescodec_version(and no longer relies on an externalCODEC_VERSIONenv or default) so Feynman runs with codec v8 as intended.database/go.mod (1)
11-11: go-ethereum bump matches the rest of the repo; verify DB paths still behaveUpdating
github.com/scroll-tech/go-ethereumhere tov1.10.14-0.20251128092113-8629f088d78fkeeps the database module aligned with the other services. Please just ensure you’ve run the full database test suite (and any migration/CLI flows that touch geth types) against this version to catch subtle API or type changes.tests/integration-test/go.mod (1)
8-9: Test harness deps aligned with main modules; just re-run integration testsBumping
github.com/scroll-tech/da-codectov0.10.0andgithub.com/scroll-tech/go-ethereumtov1.10.14-0.20251128092113-8629f088d78fkeeps the integration-test module in sync with the main services. Since this is tests-only, the risk is low but you’ll still want to re-run the integration suite to catch any subtle behavior changes in the new codec / geth versions. Based on learnings, this folder is non-critical, so no blockers from my side.tests/prover-e2e/sepolia-galileo/config.json (1)
8-9: Explicit codec_version for Galileo aligns with the new codec matrixIntroducing
"codec_version": 9under"validium_mode": falsemakes the Galileo prover-e2e config self-contained and consistent with the new codec-versioned environments. Please ensure the prover/coordinator stack that reads this config now relies oncodec_versionhere (and doesn’t still depend on a separateCODEC_VERSIONenv or hardcoded default), so Galileo scenarios actually run with codec v9.tests/prover-e2e/cloak-xen/config.json (1)
8-9: Codec version wiring for cloak‑xen looks consistentAdding
"codec_version": 8alongsidevalidium_mode: trueis consistent with the broader codec‑versioned configs; no issues from this file alone.rollup/internal/controller/blob_uploader/blob_uploader.go (1)
147-178: CodecV10 sharing the V7–V9 batch layout looks correct; update comment for clarityExtending the
CodecV7/8/9branch to includeencoding.CodecV10is consistent as long as V10 uses the same DABatchfields (parent hash, chunks, L1 queue hashes, and collectedBlocks). You might also want to update theallBlockscomment (“collect blocks for CodecV7”) to mention V7–V10 to avoid confusion later.coordinator/go.mod (1)
12-13: Dependency bumps look fine; rely on CI to confirm compatibilityBumping
github.com/scroll-tech/da-codectov0.10.0andgithub.com/scroll-tech/go-ethereumto the new pseudo‑version is straightforward here; just ensure coordinator CI (and any integration tests exercising CodecV10 / GalileoV2 paths) pass so we catch any upstream API or behavior changes.bridge-history-api/go.mod (1)
13-25: Version bumps look straightforward; double‑check the go‑ethereum hotfix behaviorUpgrading
github.com/scroll-tech/da-codecandgithub.com/scroll-tech/go-ethereumhere and updating thereplacetov1.10.14-0.20251128092359-25d5bf6b817bis mechanically fine. Given the explicit “header hash incompatibility” hotfix note, please confirm that this new go‑ethereum version still includes the required fix (or an equivalent upstream change) so bridge‑history behavior remains correct.zkvm-prover/Makefile (1)
65-66: LGTM!The new
test_e2e_run_gputarget correctly mirrors the existingtest_e2e_runtarget while adding CUDA support via--features cuda. The implementation maintains consistency with the existing pattern.tests/prover-e2e/sepolia-galileoV2/config.template.json (2)
19-24: Template credentials acknowledged.The
dev:devcredentials in the DSN are appropriate for a test configuration template. Based on learnings, placeholders in configuration files are intentionally left for users to replace when deploying. The Checkov static analysis finding (CKV_SECRET_4) is a false positive in this context.
1-40: New galileoV2 test configuration template looks good.The configuration structure properly defines prover_manager settings with galileoV2 fork support, appropriate test database settings, and placeholder values for user-provided secrets. This aligns with the broader PR objective of adding galileoV2 support to the prover-e2e test infrastructure.
common/go.mod (2)
18-18: LGTM!The go-ethereum dependency update is consistent with the rollup/go.mod changes.
187-187: LGTM!The da-codec indirect dependency update to v0.10.0 aligns with the direct dependency in rollup/go.mod.
tests/prover-e2e/sepolia-galileoV2/genesis.json (2)
1-111: LGTM! Genesis configuration for sepolia-galileoV2 test environment.The genesis file properly defines:
- Chain ID 534351 (Scroll Sepolia)
- Complete fork progression through galileoV2
- Scroll-specific L1 configuration pointing to Sepolia
- System contract deployments at 0x5300... addresses
- Pre-funded test accounts
This aligns well with the galileoV2 test infrastructure being introduced.
23-25: Fork timestamp progression is correct.The galileoV2Time (1764244000) appropriately follows galileoTime (1764236000) with an 8000-second gap (~2.2 hours). The timing is intentional and suitable for the test environment staging of the galileoV2 upgrade.
tests/prover-e2e/Makefile (2)
31-34: LGTM!The
migration_blockstarget correctly gates pre-migration execution based on the presence of SQL files. The conditionalifneqblock ensures the goose command only runs when migrations exist.
63-64: LGTM!The unified
import_datatarget simplifies the workflow by using a single config-driven approach instead of separate codec-specific targets. The dependency chain properly ensures migrations run before data import.crates/libzkp/src/tasks/batch.rs (4)
31-34: Documentation update correctly reflects V10 support.The updated comments clearly explain the relationship between STF versions (v6-v10) from the golang side and the batch header types (v6, v7, validium) used in zkvm-prover's witness definition.
43-47: Enum variant rename clarifies STF version range.The rename from
V7_V8_V9toV7_to_V10accurately reflects that this variant now handles STF versions v7 through v10, as GalileoV2 introduces STFVersion::V10.
160-165: Fork validation correctly extended for GalileoV2.The assertion now properly validates that
V7_to_V10headers are only used with EuclidV2, Feynman, Galileo, or GalileoV2 forks, aligning with the STF version support matrix.
246-249: STF version handling correctly extended to V10.V10 is correctly mapped to the same
ReferenceHeader::V7_V8_V9variant since the da-codec structure remains identical across these STF versions. The version byte in the batch header differentiates the STF version while the header format stays consistent.rollup/go.mod (1)
18-19: Dependency version updates are consistent across all Go modules.The da-codec upgrade to v0.10.0 is uniformly applied across five modules (rollup, tests/integration-test, common, bridge-history-api, coordinator), and the go-ethereum commit
8629f088d78fis consistently pinned across six modules. The one exception in bridge-history-api (using commit25d5bf6b817bvia a documented replace directive for PR #1133) is intentional and properly annotated.These updates align with the GalileoV2 and CodecV10 support objectives mentioned in the PR.
rollup/internal/utils/utils.go (1)
189-198: LGTM! CodecV10 correctly added to validium header version mapping.The addition of
encoding.CodecV10to the version=1 branch is consistent with the existing V8/V9 handling pattern for validium mode.rollup/internal/controller/relayer/l2_relayer.go (3)
491-517: LGTM! CodecV10 correctly added to ProcessPendingBatches.The extension of the codec version switch to include
CodecV10follows the existing pattern for V7-V9 handling in both validium and non-validium paths.
749-764: LGTM! CodecV10 correctly added to finalizeBundle.The codec version check extension maintains consistency with the ProcessPendingBatches changes and properly handles both validium and non-validium finalization paths.
1052-1059: LGTM! CodecV10 correctly added to validium commit payload construction.The extension is consistent with the corresponding change in
rollup/internal/utils/utils.go:encodeBatchHeaderValidium, ensuring CodecV10 is treated identically to V8/V9 for validium mode (version = 1).rollup/tests/integration_tool/main.go (2)
82-97: LGTM! Configuration structures are well-designed.The
fetchConfigstruct provides appropriate fields for RPC-based block fetching, and the optionalFetchConfigfield in the main config allows backward compatibility with existing configurations.
144-160: LGTM! CodecV10 support correctly added.The codec version switch properly handles CodecV10 (case 10) alongside existing versions.
rollup/tests/integration_tool/block_fetching.go (2)
75-80: LGTM! Block construction is correct.The encoding.Block is properly constructed with the block header, converted transactions, and withdraw root.
55-68: Unsafe assumption about L1 message transaction positioning in block.The replacement logic (lines 60-68) assumes L1 message transactions occupy indices
0tocount-1inblockTxs, but the counting logic (lines 35-40) counts L1 messages throughout all transactions without recording their positions. If L1 messages can appear at arbitrary positions within a block, this would attempt to replace non-L1 transactions or callAsL1MessageTx()on wrong transaction types. Either build a map of actual L1 message positions or add explicit validation that all L1 messages appear at the start of the block.This same issue exists in
rollup/internal/controller/watcher/l2_watcher.go.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rollup/tests/integration_tool/main.go (1)
162-169: Minor: error message for end block parse is misleadingWhen parsing
endBlk, the error message still says"invalid begin block number", which can be confusing when troubleshooting CLI input. Consider changing it to mention"end block number"instead.
♻️ Duplicate comments (1)
tests/prover-e2e/sepolia-galileoV2/config.template.json (1)
29-29: Fix typo in placeholder: "serach" should be "search".- "endpoint": "<serach for a public rpc endpoint like alchemy>" + "endpoint": "<search for a public rpc endpoint like alchemy>"
🧹 Nitpick comments (3)
tests/prover-e2e/sepolia-galileoV2/config.template.json (1)
21-22: Consider using environment variables for database credentials instead of hardcoding.While this is a template file intended for local development, hardcoded credentials (even dev credentials) create unnecessary security surface. This template should be clearer that users must update these values before any deployment, or better yet, use environment variable placeholders.
As a reference, you could refactor the DSN to use environment variables:
- "dsn": "postgres://dev:dev@localhost/scroll?sslmode=disable", + "dsn": "postgres://<POSTGRES_USER>:<POSTGRES_PASSWORD>@<POSTGRES_HOST>/scroll?sslmode=disable",Then add a comment or documentation instructing users to replace the placeholders, or use actual environment variable references if the configuration loader supports them.
rollup/tests/integration_tool/main.go (2)
22-25: CodecVersion mapping is correct; consider small helper for reuseThe mapping from
cfg.CodecVersion6–10 toencoding.CodecV6–CodecV10, with0falling back to the global defaultCodecV9, is logically consistent and guarded with an explicit error on unsupported values. If you end up needing this mapping elsewhere, consider extracting a small helper (e.g.,codecFromInt(int) (encoding.CodecVersion, error)) to avoid duplicating the switch.Also applies to: 144-160
190-206: JSON result output is clear; consider a small test around itMarshalling
retwithjson.MarshalIndentand writing tooutputPathwith0600permissions is a clean way to expose deterministic output from this tool. Given Codecov is flagging missing coverage in this file, it might be worth adding a minimal test (or golden‑file style test) aroundimportData’s returned structure and the file write path, to guard future refactors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rollup/tests/integration_tool/block_fetching.go(1 hunks)rollup/tests/integration_tool/main.go(8 hunks)tests/prover-e2e/sepolia-galileoV2/.make.env(1 hunks)tests/prover-e2e/sepolia-galileoV2/config.template.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/prover-e2e/sepolia-galileoV2/.make.env
- rollup/tests/integration_tool/block_fetching.go
🧰 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-galileoV2/config.template.json
🧬 Code graph analysis (1)
rollup/tests/integration_tool/main.go (4)
coordinator/internal/types/auth.go (1)
Hash(134-141)database/config.go (1)
DBConfig(12-19)coordinator/internal/config/config.go (1)
Config(57-63)coordinator/internal/utils/codec_validium.go (1)
CodecVersion(10-10)
🪛 Checkov (3.2.334)
tests/prover-e2e/sepolia-galileoV2/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). (10)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: test
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (4)
rollup/tests/integration_tool/main.go (4)
12-20: Newcommonimport usage looks appropriateThe added
github.com/scroll-tech/go-ethereum/commonimport is only used in the config struct for addresses/hashes, keeping this dependency localized to the integration tool. No issues here.
82-97: Config and fetchConfig wiring is coherentUsing a dedicated
fetchConfigand a pointer fieldFetchConfig *fetchConfiginconfigcleanly gates the optional fetch‑and‑store behavior, and addingCodecVersionat config level makes codec selection explicit and testable. This struct layout and JSON tagging look solid.
123-136: newConfig refactor is straightforward and safeUnmarshalling into a local
loadCfgand returning it directly is idiomatic and keepscfginitialization contained to theBeforehook. Error propagation is clear; no concerns.
171-177: Conditional fetch path using FetchConfig is sensibleGuarding
fetchAndStoreBlocksbehindcfg.FetchConfig != nilgives a clear feature flag for the fetch‑and‑store mode, and passing the resulting slice intoimportDataviaimport_blocks(nil when not fetching) aligns with Go’s safe handling of nil slices. This control flow looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/libzkp/src/tasks/batch.rs (3)
31-34: Tighten wording around STF→header mappingThe comment is clearer than before but still a bit ambiguous (“v7-v10 (current)” and “v7 (current)”). Consider explicitly naming the Rust types and the STF range they cover to avoid reintroducing the earlier confusion around V9 vs V10.
-/// We distinguish the header type in golang side according to the STF -/// version, i.e. v6, v7-v10 (current), and validium -/// And adapt it to the corresponding batch header type used in zkvm-prover's witness -/// definition, i.e. v6, v7 (current), and validium +/// We distinguish the header type on the Go side according to the STF +/// version, i.e. v6, v7–v10 (current), and validium. +/// These map to the batch header types used in zkvm-prover's witness +/// definition: [`BatchHeaderV6`] for STF v6, [`BatchHeaderV7`] for STF v7–v10, +/// and validium.
69-91: Accessors retain previous invariants; API rename is reasonableThe new
to_zkvm_batch_header_*accessors mirror the oldmust_*helpers, with the same invariant enforcement viaunreachable!()and clearer naming for the V7–V10 case.If this crate has external consumers, you might optionally alias the old names to these (and deprecate them) to ease upgrades, but functionally this looks fine.
160-165: Fork sanity check correctly extended to GalileoV2Including
ForkName::GalileoV2in thematches!and expected list keeps the da‑codec@v7/8/9/10 header guardrails in place for the new fork.Very minor nit: for consistency, you could use
{:?}forfoundas well, but that’s cosmetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/libzkp/src/tasks/batch.rs(5 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). (9)
- GitHub Check: test
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (2)
crates/libzkp/src/tasks/batch.rs (2)
43-48: V7_to_V10 variant and Display/batch_hash wiring look consistentRenaming the variant to
V7_to_V10and printing"V7 - V10"aligns the type and Display with the STF range, and forwardingbatch_hash()toBatchHeaderV7preserves existing behavior.Also applies to: 54-55, 64-65
234-252: ReferenceHeader mapping cleanly reuses V7_V8_V9 for STF v10Handling
(Domain::Scroll, STFVersion::V7 | V8 | V9 | V10)viaReferenceHeader::V7_V8_V9and the sharedto_zkvm_batch_header_v7_to_v10()accessor matches the “codec unchanged” comment and keeps V10 on the existing path. The v6 and validium arms also line up with the new accessors.
Summary by CodeRabbit
New Features
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.