feat: the CLOAK privacy solution - #1737
Conversation
Sorry, something went wrong.
WalkthroughBumps project version to v4.6.3 and threads Validium mode, a version byte, and optional decryption key through coordinator, provertasks, libzkp FFI, Rust proving/verifier crates, rollup watcher/relayer, and tests; adds L1-message fetching and switches several L2 clients from ethclient to rpc.Client. Changes
Sequence Diagram(s)sequenceDiagram
participant API as Coordinator API
participant Config as Config
participant Verifier as Verifier
participant Utils as Utils
participant LibZKP as LibZKP
API->>Config: read cfg.L2.ValidiumMode
API->>Verifier: NewVerifier(cfg.Verifier, validiumMode)
Verifier->>Utils: Version(forkName, validiumMode)
Utils-->>Verifier: version (uint8)
Verifier->>LibZKP: init(ver_n)
LibZKP-->>Verifier: ready
Verifier-->>API: initialized
sequenceDiagram
participant Prover as ProverTask
participant Config as Config
participant Sequencer as Sequencer (cfg)
participant LibZKP as LibZKP
Prover->>Config: validiumMode()
alt validiumMode == true
Prover->>Sequencer: request Sequencer.DecryptionKey (hex)
Sequencer-->>Prover: hex key
Prover->>Prover: decode hex -> decryptionKey bytes
end
Prover->>LibZKP: GenerateUniversalTask(..., decryptionKey)
LibZKP-->>Prover: task result
sequenceDiagram
participant Watcher as L2Watcher
participant RPC as rpc.Client
participant Chain as L2 Node
participant DB as DB
Watcher->>RPC: scroll_getL1MessagesInBlock(block)
RPC-->>Watcher: encrypted L1 messages
Watcher->>Watcher: validate counts & queue indexes
Watcher->>DB: store block with replaced txs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labelsbump-version Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)coordinator/internal/logic/verifier/mock.go (1)📜 Review details11-12: Consider naming the ignored parameter for clarity.
The second parameter is intentionally ignored in this mock implementation, but naming it (e.g., useValidiumMode) would improve readability and make the relationship to the real verifier implementation more explicit. Additionally, update the function comment to document the new parameter.
Apply this diff to improve clarity:
-// NewVerifier Sets up a mock verifier. -func NewVerifier(cfg *config.VerifierConfig, _ bool) (*Verifier, error) { +// NewVerifier Sets up a mock verifier. +// The useValidiumMode parameter is ignored in this mock implementation. +func NewVerifier(cfg *config.VerifierConfig, useValidiumMode bool) (*Verifier, error) {
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between bdddfbd and ea5fec8.
📒 Files selected for processing (1)- coordinator/internal/logic/verifier/mock.go (1 hunks)
coordinator/internal/logic/verifier/verifier.go (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). (7)coordinator/internal/config/config.go (1)
- NewVerifier (77-105)
coordinator/internal/logic/verifier/types.go (1)
- VerifierConfig (75-79)
- Verifier (11-17)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
coordinator/internal/logic/verifier/mock.go (1)22-40: LGTM!
The mock verification methods remain unchanged and continue to provide appropriate test doubles for the real verifier. The simple logic (returning success except for explicitly invalid proofs) is suitable for mock testing purposes.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)build/dockerfiles/coordinator-proxy.Dockerfile (1)📜 Review details.github/workflows/docker.yml (1)20-22: Consider minimizing installed utilities for production readiness.
The utilities (vim, netcat-openbsd, net-tools, curl, jq) are useful for debugging but add unnecessary bloat for a production image. Consider creating separate debug and production variants, or removing unused tools if they're not actively needed by operators.
399-401: Consider adding multi-platform build support.
Most other jobs in this workflow specify platforms: linux/amd64,linux/arm64 (see gas_oracle, rollup_relayer, blob_uploader, etc.). The coordinator-proxy job omits this. Verify whether single-platform deployment is intentional (like coordinator-api) or if multi-platform support should be added for consistency.
If multi-platform support is desired, add to the build configuration:
- name: Build and push uses: docker/build-push-action@v4 env: ECR_REGISTRY: ${{ steps.login-ecr.outputs.registry }} REPOSITORY: coordinator-proxy IMAGE_TAG: ${{ github.ref_name }} with: context: . file: ./build/dockerfiles/coordinator-proxy.Dockerfile + platforms: linux/amd64,linux/arm64 push: true tags: | scrolltech/${{ env.REPOSITORY }}:${{ env.IMAGE_TAG }} ${{ env.ECR_REGISTRY }}/${{ env.REPOSITORY }}:${{ env.IMAGE_TAG }}
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between a7543d4 and 833a2f5.
📒 Files selected for processing (5)- .github/workflows/docker.yml (1 hunks)
- build/dockerfiles/coordinator-proxy.Dockerfile (1 hunks)
- build/dockerfiles/coordinator-proxy.Dockerfile.dockerignore (1 hunks)
- coordinator/internal/utils/codec_validium.go (1 hunks)
- crates/libzkp_c/src/lib.rs (1 hunks)
- coordinator/internal/utils/codec_validium.go
crates/libzkp_c/src/utils.rs (1)🪛 actionlint (1.7.8) .github/workflows/docker.ymlcrates/l2geth/src/rpc_client.rs (1)
- c_char_to_str (3-6)
crates/l2geth/src/lib.rs (1)
- get_client (81-87)
crates/libzkp/src/lib.rs (1)
- get_client (14-19)
- checkout_chunk_task (40-49)
370-370: the runner of "docker/setup-qemu-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
372-372: the runner of "docker/setup-buildx-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
374-374: the runner of "docker/login-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
393-393: the runner of "docker/build-push-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (7)- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
build/dockerfiles/coordinator-proxy.Dockerfile.dockerignore (1)build/dockerfiles/coordinator-proxy.Dockerfile (1)1-8: LGTM!
The .dockerignore patterns are appropriate and follow Docker best practices, keeping the build context lean.
crates/libzkp_c/src/lib.rs (2)1-26: LGTM!
The multi-stage build is well-structured. The CGO_LDFLAGS propagation to the final image is correct and necessary for runtime compatibility. The version check on Line 24 provides basic validation of the compiled binary.
155-156: LGTM: Function signature follows FFI conventions
The addition of decryption_key and decryption_key_len parameters follows the same pattern as expected_vk / expected_vk_len, maintaining consistency with the existing FFI design.
176-176: LGTM: Function call correctly updated
The call to libzkp::checkout_chunk_task has been properly updated to include the decryption_key parameter, matching the updated function signature shown in the relevant code snippets.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)rollup/internal/controller/watcher/l2_watcher.go (1)🧹 Nitpick comments (1)132-140: Critical: Don't assume L1 message txs occupy the first slots.
The code counts L1 message transactions by iterating through all transactions (lines 107-112) but then assumes the first count transactions are L1 messages. This is incorrect—user transactions can precede L1 message transactions in a block. Accessing blockTxs[ii].AsL1MessageTx() when blockTxs[ii] is not an L1 message will cause a panic or incorrect behavior.
Apply this diff to collect actual L1 message indices first:
- for ii := 0; ii < count; ii++ { - // sanity check - if blockTxs[ii].AsL1MessageTx().QueueIndex != txs[ii].AsL1MessageTx().QueueIndex { - return fmt.Errorf("L1 message queue index mismatch at index %d: expected %d, got %d", ii, blockTxs[ii].AsL1MessageTx().QueueIndex, txs[ii].AsL1MessageTx().QueueIndex) - } - - log.Info("Replacing L1 message tx in validium mode", "index", ii, "queueIndex", txs[ii].AsL1MessageTx().QueueIndex, "decryptedTxHash", blockTxs[ii].Hash().Hex(), "originalTxHash", txs[ii].Hash().Hex()) - blockTxs[ii] = txs[ii] - } + l1Indices := make([]int, 0, count) + for idx, tx := range blockTxs { + if tx.IsL1MessageTx() { + l1Indices = append(l1Indices, idx) + } + } + if len(l1Indices) != len(txs) { + return fmt.Errorf("L1 message count mismatch: expected %d, got %d", len(l1Indices), len(txs)) + } + for i, idx := range l1Indices { + if blockTxs[idx].AsL1MessageTx().QueueIndex != txs[i].AsL1MessageTx().QueueIndex { + return fmt.Errorf("L1 message queue index mismatch at position %d: expected %d, got %d", idx, blockTxs[idx].AsL1MessageTx().QueueIndex, txs[i].AsL1MessageTx().QueueIndex) + } + log.Info("Replacing L1 message tx in validium mode", "position", idx, "queueIndex", txs[i].AsL1MessageTx().QueueIndex, "decryptedTxHash", blockTxs[idx].Hash().Hex(), "originalTxHash", txs[i].Hash().Hex()) + blockTxs[idx] = txs[i] + }
rollup/internal/controller/watcher/l2_watcher_test.go (1)📜 Review details52-74: Consider using a dynamic block selection.
The test uses a hardcoded block hash (0xc80cf12883341827d71c08f734ba9a9d6da7e59eb16921d26e6706887e552c74) that may not exist on the target network specified by RPC_ENDPOINT_URL. Consider fetching a recent block dynamically or documenting which network this hash corresponds to.
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 833a2f5 and 94dbf57.
📒 Files selected for processing (6)- common/testcontainers/testcontainers.go (2 hunks)
- rollup/cmd/permissionless_batches/app/app.go (2 hunks)
- rollup/cmd/rollup_relayer/app/app.go (5 hunks)
- rollup/internal/controller/watcher/l2_watcher.go (5 hunks)
- rollup/internal/controller/watcher/l2_watcher_test.go (3 hunks)
- rollup/internal/controller/watcher/watcher_test.go (3 hunks)
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/cmd/rollup_relayer/app/app.go
- rollup/internal/controller/watcher/l2_watcher.go
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/cmd/rollup_relayer/app/app.go
- rollup/cmd/permissionless_batches/app/app.go
rollup/internal/controller/watcher/l2_watcher.go (2)rollup/cmd/rollup_relayer/app/app.go (5)
- NewL2WatcherClient (45-64)
- L2WatcherClient (23-42)
rollup/internal/config/l2.go (1)rollup/cmd/permissionless_batches/app/app.go (4)rollup/internal/controller/relayer/l2_relayer.go (1)
- L2Config (10-29)
rollup/internal/config/relayer.go (1)
- NewLayer2Relayer (121-206)
rollup/internal/controller/watcher/l2_watcher.go (1)
- RelayerConfig (63-88)
rollup/internal/utils/confirmation.go (1)
- NewL2WatcherClient (45-64)
- GetLatestConfirmedBlockNumber (18-56)
rollup/internal/config/l2.go (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). (8)rollup/internal/controller/watcher/l2_watcher.go (1)
- L2Config (10-29)
rollup/internal/config/config.go (1)
- NewL2WatcherClient (45-64)
rollup/internal/config/relayer.go (1)
- Config (20-25)
- RelayerConfig (63-88)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
rollup/cmd/rollup_relayer/app/app.go (5)rollup/internal/controller/watcher/watcher_test.go (1)15-15: LGTM: Import addition is correct.
The rpc package import is necessary for the new rpc.Dial pattern used at line 73.
73-77: LGTM: Dual client pattern correctly implemented.
The switch from ethclient.Dial to rpc.Dial + ethclient.NewClient wrapper is necessary because different downstream APIs have different client type requirements:
- NewL2WatcherClient requires the raw *rpc.Client
- NewLayer2Relayer and GetLatestConfirmedBlockNumber require *ethclient.Client
This pattern correctly satisfies both requirements.
105-105: LGTM: Correct client type passed to NewLayer2Relayer.
The change to pass l2ethClient (*ethclient.Client) instead of l2client correctly matches the function signature which expects *ethclient.Client.
119-119: LGTM: Watcher initialization updated correctly for Validium mode.
The changes correctly:
- Pass the raw *rpc.Client (l2client) which matches the expected function signature
- Add the validiumMode boolean parameter from configuration, properly positioned in the parameter list
This aligns with the CLOAK/Validium feature additions.
149-149: LGTM: Correct client type passed to GetLatestConfirmedBlockNumber.
The change to pass l2ethClient correctly satisfies the ethClient interface expected by GetLatestConfirmedBlockNumber.
rollup/cmd/permissionless_batches/app/app.go (1)67-69: LGTM!
The refactor to obtain an RPC client first and then wrap it in an ethclient is clean and aligns with the broader RPC-based client handling changes across the PR.
rollup/internal/controller/watcher/l2_watcher_test.go (1)94-100: LGTM!
The switch to RPC-based client creation and the addition of the ValidiumMode parameter align correctly with the configuration structure and the broader changes in this PR.
rollup/internal/controller/watcher/l2_watcher.go (1)24-24: LGTM!
The updated constructor calls correctly pass the validiumMode parameter and use the RPC-based client, aligning with the broader refactor.
Also applies to: 46-49
44-64: LGTM!
The constructor correctly accepts an RPC client, wraps it in an ethclient, stores the RPC client reference for raw calls, and captures the validiumMode flag for conditional logic.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)coordinator/Makefile (1)📜 Review details40-40: Inconsistent path resolution: $(CURDIR) used only in localsetup, not other build targets.
Lines 40 and 42 correctly switch to $(CURDIR) for robust path resolution, but lines 29, 32, 35, 49, 52 (and test at line 18) still use $(PWD). $(CURDIR) is more reliable in scenarios involving symlinks or nested make invocations. Consider applying this consistently across all build targets.
To ensure consistent path resolution, consider applying this pattern to all targets:
coordinator_api: $(LIBZKP_PATH) ## Builds the Coordinator api instance. - go build -ldflags "-X scroll-tech/common/version.ZkVersion=${ZK_VERSION}" -o $(PWD)/build/bin/coordinator_api ./cmd/api + go build -ldflags "-X scroll-tech/common/version.ZkVersion=${ZK_VERSION}" -o $(CURDIR)/build/bin/coordinator_api ./cmd/api coordinator_cron: - go build -ldflags "-X scroll-tech/common/version.ZkVersion=${ZK_VERSION}" -o $(PWD)/build/bin/coordinator_cron ./cmd/cron + go build -ldflags "-X scroll-tech/common/version.ZkVersion=${ZK_VERSION}" -o $(CURDIR)/build/bin/coordinator_cron ./cmd/cron coordinator_tool: - go build -ldflags "-X scroll-tech/common/version.ZkVersion=${ZK_VERSION}" -o $(PWD)/build/bin/coordinator_tool ./cmd/tool + go build -ldflags "-X scroll-tech/common/version.ZkVersion=${ZK_VERSION}" -o $(CURDIR)/build/bin/coordinator_tool ./cmd/toolSimilarly, update mock targets (lines 49, 52), test target (line 18), and lint target (line 61).
Also applies to: 42-42
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 94dbf57 and 261de3d.
📒 Files selected for processing (4)- coordinator/Makefile (1 hunks)
- tests/prover-e2e/Makefile (2 hunks)
- tests/prover-e2e/README.md (1 hunks)
- tests/prover-e2e/cloak-xen/genesis.json (1 hunks)
- tests/prover-e2e/README.md
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
coordinator/Makefile (1)tests/prover-e2e/Makefile (2)40-40: Verify -fL flags are appropriate for config copying in local setup.
The -f (force overwrite) and -L (follow symlinks) flags are reasonable for a setup workflow, but ensure this behavior aligns with your local development practices. If config.json may be a symlink to an external config or if overwriting should be guarded, consider the implications.
tests/prover-e2e/cloak-xen/genesis.json (1)28-45: Verify GOOSE migration directory and double-up invocation logic.
Line 44 invokes ${GOOSE_CMD} up and then line 45 invokes GOOSE_MIGRATION_DIR=conf ${GOOSE_CMD} up-to 100. This runs two separate migration commands:
- One with the default migration directory (current directory or GOOSE_MIGRATION_DIR env default)
- One with migrations from conf directory up to version 100
Confirm this dual-invocation is intentional. If migrations were moved to conf/, consider consolidating to a single command:
${GOOSE_CMD} up - GOOSE_MIGRATION_DIR=conf ${GOOSE_CMD} up-to 100 + GOOSE_MIGRATION_DIR=conf ${GOOSE_CMD} upOtherwise, document why two separate migrations are needed.
52-56: conf/ is a symbolic link—not a concrete directory with subdirectories.
The Makefile correctly documents this on lines 17–18: conf is a target that instructs users to link either sepolia or cloak-xen as conf. Once this symlink is created (e.g., ln -s sepolia conf), the conf/config.json path resolves correctly. The config files exist at sepolia/config.json and cloak-xen/config.json and contain valid configurations. The codec flags (7 for Euclid, 8 for default) are properly passed as CLI arguments, not expected in the config file. The setup is intentional and correct.
Likely an incorrect or invalid review comment.
1-123: Confirm genesis state root and L1 configuration values for validium test environment.
Verification confirms that the genesis.json file is properly configured for validium mode testing:
✓ config.json explicitly sets "validium_mode": true, confirming this is the intended test environment
✓ useZktrie is correctly set to false for validium mode
✓ JSON structure is valid and all required genesis fields are present
✓ Pre-allocated system contracts and accounts are properly configuredHowever, the following values require manual verification:
- genesisStateRoot (0x08d535cc60f40af5dd3b31e0998d7567c2d568b224bed2ba26070aeb078d1339) and missingHeaderFieldsSHA256 (0x9062e2fa1200dca63bee1d18d429572f134f5f0c98cb4852f62fc394e33cf6e6) are not validated against test execution code. Confirm these hashes match the expected computed values from the account allocations.
- L1 configuration addresses (l1MessageQueueV2Address: 0xb5D7c8Ec8C7A72a0ed9782cB67Cc68b3915389a1, scrollChainAddress: 0x283Eff377d7f38955c2aD1414DAE94e9A9b3270F) should match the L1 test environment setup if applicable for this validium test.
Ensure these values are consistent with the test harness and any L1 deployment configuration before merging.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)crates/libzkp/src/tasks/batch.rs (1)🧹 Nitpick comments (5)common/types/message/message.go (1)347-348: Uncomment the assertion if the method is available.
The commented-out assertion should be uncommented to verify that the computed batch hash matches the provided one, as BatchHeaderValidium should have a batch_hash() method available.
Apply this diff:
- // Sanity: computed batch hash equals the provided one (if method available) - // assert_eq!(v.header.batch_hash(), expected_batch_hash); + // Sanity: computed batch hash equals the provided one + assert_eq!(v.header.batch_hash(), expected_batch_hash);116-128: Add UnmarshalJSON method to maintain round-trip compatibility.
The RawBytes type has a custom MarshalJSON that outputs a numeric array, but lacks the corresponding UnmarshalJSON. This breaks round-trip marshaling in Go and will cause failures when coordinator or other Go components try to unmarshal these payloads.
A detailed implementation was suggested in a previous review. Please implement the UnmarshalJSON method that accepts both the new numeric-array format and legacy base64 strings for backward compatibility.
coordinator/internal/logic/provertask/batch_prover_task.go (3)📜 Review detailscrates/libzkp/src/tasks/batch.rs (1)260-260: Remove commented code.
The commented-out variable declaration serves no purpose and should be removed to keep the codebase clean.
Apply this diff:
- // var chunkInfos []*message.ChunkInfo
331-336: Document and validate the hardcoded byte offsets for KZG data extraction.
The hardcoded offsets (64:112, 112:160) used to extract KzgCommitment and KzgProof from BlobDataProof are fragile and difficult to maintain. While the comment describes the memory layout, these magic numbers should be defined as named constants.
Consider defining constants at the package level:
const ( BlobDataProofKzgCommitmentOffset = 64 BlobDataProofKzgCommitmentEnd = 112 BlobDataProofKzgProofOffset = 112 BlobDataProofKzgProofEnd = 160 )Then use them:
-taskDetail.KzgProof = &message.Byte48{Big: hexutil.Big(*new(big.Int).SetBytes(dbBatch.BlobDataProof[112:160]))} -taskDetail.KzgCommitment = &message.Byte48{Big: hexutil.Big(*new(big.Int).SetBytes(dbBatch.BlobDataProof[64:112]))} +taskDetail.KzgCommitment = &message.Byte48{Big: hexutil.Big(*new(big.Int).SetBytes(dbBatch.BlobDataProof[BlobDataProofKzgCommitmentOffset:BlobDataProofKzgCommitmentEnd]))} +taskDetail.KzgProof = &message.Byte48{Big: hexutil.Big(*new(big.Int).SetBytes(dbBatch.BlobDataProof[BlobDataProofKzgProofOffset:BlobDataProofKzgProofEnd]))}
274-282: Fix variable naming inconsistency.
The variable taskBytesWithchunkProofs has inconsistent casing. The 'c' in "chunk" should be capitalized for consistency.
Apply this diff:
- taskBytesWithchunkProofs, err := json.Marshal(taskDetail) + taskBytesWithChunkProofs, err := json.Marshal(taskDetail) if err != nil { return nil, fmt.Errorf("failed to marshal chunk proofs, taskID:%s err:%w", task.TaskID, err) } taskMsg := &coordinatorType.GetTaskSchema{ TaskID: task.TaskID, TaskType: int(message.ProofTypeBatch), - TaskData: string(taskBytesWithchunkProofs), + TaskData: string(taskBytesWithChunkProofs), HardForkName: hardForkName, }common/types/message/message.go (1)219-226: Improve panic message for better debugging.
The panic message "unexpected header type" could be more informative. Consider including the actual header type to aid debugging.
Apply this diff:
match &self.batch_header { BatchHeaderV::Validium(h) => assert_eq!( h.header.batch_hash(), h.batch_hash, "calculated batch hash match which from coordinator" ), - _ => panic!("unexpected header type"), + _ => panic!("unexpected header type: expected Validium, found {}", self.batch_header), }138-138: Remove commented-out deprecated field.
Based on previous review discussions confirming this field is deprecated with no references, the commented-out TxBytes line should be removed to keep the codebase clean.
Apply this diff:
IsPadding bool `json:"is_padding"` - // TxBytes []byte `json:"tx_bytes"` TxBytesHash common.Hash `json:"tx_data_digest"`
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between ff3da9a and 3ee6639.
📒 Files selected for processing (4)- common/types/message/message.go (4 hunks)
- coordinator/internal/logic/provertask/batch_prover_task.go (2 hunks)
- crates/libzkp/src/lib.rs (2 hunks)
- crates/libzkp/src/tasks/batch.rs (3 hunks)
- crates/libzkp/src/lib.rs
common/types/message/message.go (1)coordinator/internal/logic/provertask/batch_prover_task.go (6)coordinator/internal/utils/version.go (1)
- ChunkInfo (131-148)
crates/libzkp/src/tasks/bundle.rs (1)
- Version (16-40)
crates/libzkp/src/tasks/chunk.rs (1)
- precheck_and_build_metadata (60-72)
crates/libzkp/src/tasks/batch/utils.rs (4)
- precheck_and_build_metadata (185-190)
crates/libzkp/src/tasks.rs (1)
- to_blob (27-41)
- blob_to_kzg_commitment (44-48)
- get_versioned_hash (54-58)
- get_kzg_proof (66-74)
- check_aggregation_proofs (24-36)
common/types/message/message.go (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). (16)coordinator/internal/types/get_task.go (1)
- OpenVMChunkProof (180-189)
- BatchTaskDetail (94-105)
- Byte48 (51-53)
rollup/internal/orm/batch.go (2)
- GetTaskSchema (12-19)
coordinator/internal/orm/batch.go (2)
- Batch (25-75)
- Batch (83-85)
coordinator/internal/utils/version.go (1)
- Batch (18-72)
- Batch (80-82)
coordinator/internal/utils/codec_validium.go (2)
- Version (16-40)
- CodecVersion (10-10)
- FromVersion (38-40)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
coordinator/internal/logic/provertask/batch_prover_task.go (2)crates/libzkp/src/tasks/batch.rs (3)312-317: Verify the early return behavior for unsupported codec versions.
When the codec version is outside the expected range (V3, V4, V6, V7, V8), the function returns a partially initialized taskDetail (with only Version, ChunkProofs, and ForkName populated, but missing BatchHeader and KZG-related fields). A past review questioned why this doesn't error.
Please confirm whether returning a partial taskDetail is the intended behavior or if this should return an error instead.
299-299: Verification complete: version() method is correctly implemented.
The bp.version() method on BatchProverTask is inherited from BaseProverTask and correctly delegates to utils.Version(hardForkName, b.validiumMode()) at line 72 of coordinator/internal/logic/provertask/prover_task.go. The validiumMode() helper properly retrieves the ValidiumMode setting from config. Version byte calculation is consistent across the codebase.
common/types/message/message.go (5)23-87: LGTM! Good use of Display for error messages.
The BatchHeaderValidiumWithHash struct and the Validium variant addition to BatchHeaderV are well-structured. The Display implementation is particularly useful for providing informative error messages in the unreachable! macros throughout the code.
140-274: LGTM! Version-driven logic is well-structured.
The version-driven approach to handle Validium vs non-Validium paths is well-implemented with appropriate assertions and checks. The reference header construction based on (domain, stf_version) tuples provides good type safety, and the block hash zeroing for the Scroll domain is correctly implemented.
276-288: LGTM! Version parameter correctly threaded through.
The precheck_and_build_metadata method correctly passes the version to check_aggregation_proofs, which is consistent with the version-driven validation approach introduced in this PR.
41-48: LGTM! Version field additions support version-driven processing.
The Version and PostMsgQueueHash fields added to ChunkTaskDetail are well-placed and consistent with the version-driven approach introduced across the codebase.
94-105: LGTM! Pointer types for optional KZG fields are appropriate.
The changes to BatchTaskDetail are well-structured:
- Version field supports version-driven processing
- KZG fields changed to pointers (*Byte48) with omitempty correctly models their optional nature
- ChallengeDigest as interface{} provides flexibility for different hash types
144-148: LGTM! New fields support block hash tracking and encryption.
The additions of PrevBlockhash, PostBlockhash, and EncryptionKey to ChunkInfo align with:
- The block hash handling logic in the Rust code (zeroing for Scroll domain)
- The CLOAK privacy solution mentioned in the PR title
200-211: LGTM! EncryptionKey field appropriately added.
The EncryptionKey field addition to OpenVMBatchInfo is correctly implemented. As clarified in past reviews, this holds the public encryption key (not the secret decryption key), which is appropriate for these public data structures.
261-272: LGTM! EncryptionKey consistently propagated.
The EncryptionKey field is correctly added to OpenVMBundleInfo, maintaining consistency with the similar addition to OpenVMBatchInfo and supporting the CLOAK privacy feature.
Sorry, something went wrong.
Purpose or design rationale of this PR
Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has tag in common/version.go been updated or have you added bump-version label to this PR?
Breaking change label
Does this PR have the breaking-change label?
Summary by CodeRabbit
New Features
Configuration Updates
Chores