refactor(coordinator): simplify logic post-Euclid #1652
Conversation
WalkthroughThis change removes the entire legacy prover Rust codebase, configuration, and related build/test infrastructure. It eliminates all references to legacy proof types and interfaces (e.g., Halo2 proofs) from the Go codebase, replacing them with exclusive use of new OpenVM proof types. Configuration and control logic are simplified by dropping support for low-version circuits, mock mode, and legacy proof abstractions. Changes
Sequence Diagram(s)sequenceDiagram
participant Coordinator
participant Verifier
participant DB
participant Prover
Coordinator->>Verifier: Verify OpenVMChunkProof / OpenVMBatchProof / OpenVMBundleProof
Verifier-->>Coordinator: Verification result (no legacy VKs, only OpenVM)
Coordinator->>DB: Store or retrieve OpenVM proof (pointer types)
Prover->>Coordinator: Submit OpenVM proof (no legacy types)
Coordinator-->>Prover: Acknowledge proof receipt
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package zstd: could not load export data: no export data for "github.com/scroll-tech/da-codec/encoding/zstd"" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1652 +/- ##
===========================================
+ Coverage 40.68% 40.91% +0.23%
===========================================
Files 225 225
Lines 18419 18303 -116
===========================================
- Hits 7493 7488 -5
+ Misses 10195 10088 -107
+ Partials 731 727 -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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
coordinator/internal/logic/provertask/bundle_prover_task.go (1)
203-210: 🛠️ Refactor suggestionAvoid passing a pointer-to-pointer to
json.Unmarshal
proofis already a pointer. Passing&proof(i.e.**OpenVMBatchProof) forcesencoding/jsonto allocate an additional struct and assign it toproof, which is unnecessary and makes the code harder to read.- proof := &message.OpenVMBatchProof{} - if encodeErr := json.Unmarshal(batch.Proof, &proof); encodeErr != nil { + proof := &message.OpenVMBatchProof{} + if encodeErr := json.Unmarshal(batch.Proof, proof); encodeErr != nil {(Same applies everywhere this pattern appears.)
♻️ Duplicate comments (1)
coordinator/internal/logic/verifier/verifier_test.go (1)
74-83: Same signature mismatch for chunk proof helperPlease apply the identical fix here:
-func readChunkProof(filePat string, as *assert.Assertions) types.OpenVMChunkProof { +func readChunkProof(filePat string, as *assert.Assertions) *types.OpenVMChunkProof { @@ - proof := &types.OpenVMChunkProof{} - as.NoError(json.Unmarshal(byt, proof)) - - return proof + proof := &types.OpenVMChunkProof{} + as.NoError(json.Unmarshal(byt, proof)) + return proof }
🧹 Nitpick comments (8)
coordinator/test/mock_prover.go (2)
210-221: Minor: default case for unsupported task types
submitProofsilently leavesproofasnilif an unexpectedTaskTypeis received.
Adding adefault:that fails the test early makes debugging easier.switch proverTaskSchema.TaskType { @@ proof = encodeData +default: + t.Fatalf("unsupported task type %d", proverTaskSchema.TaskType) }
225-235: Duplicated code – factor into a helperThe chunk/batch branches that craft an invalid proof are identical except for the proof struct type. Extracting a small helper reduces duplication and future maintenance cost.
coordinator/internal/logic/provertask/batch_prover_task.go (2)
203-206: Avoid double-pointer unmarshalling
proofis already a pointer; passing&prooftojson.Unmarshalyields a**message.OpenVMChunkProof.
While Go’sencoding/jsontolerates this, it is unnecessary and obscures intent.- proof := &message.OpenVMChunkProof{} - if encodeErr := json.Unmarshal(chunk.Proof, &proof); encodeErr != nil { + proof := &message.OpenVMChunkProof{} + if encodeErr := json.Unmarshal(chunk.Proof, proof); encodeErr != nil {
258-259: UnusedhardForkNameparameter – clarify or drop
hardForkNameis accepted but ignored becauseForkNameis hard-coded tomessage.EuclidV2ForkNameForProver.
Either use the parameter:- ForkName: message.EuclidV2ForkNameForProver, + ForkName: hardForkName,or delete it from the signature and call sites to avoid future confusion.
coordinator/internal/logic/auth/login.go (1)
98-101: Redundant length check afterstrings.Split
strings.Splitalways returns at least one element, solen(proverVersionSplits)==0is impossible.
This guard can be removed to simplify the code.coordinator/internal/logic/verifier/verifier.go (1)
164-195: Potential temp-file race when multiple verifier instances start
loadOpenVMVksdumps VKs to a fixed filename in/tmp(openVmVk.json).
Parallel starts (e.g., multiple coordinator replicas) could race on this file.
Consider usingos.CreateTempto obtain a unique filename:- tempFile := path.Join(os.TempDir(), "openVmVk.json") + f, err := os.CreateTemp("", "openVmVk_*.json") + if err != nil { return err } + tempFile := f.Name() + f.Close()common/types/message/message.go (2)
101-108: Addomitemptyto slice-typed JSON fields to prevent unwantednullvalues
ChunkProofs(and other byte / slice fields in this struct) default tonullwhen the slice isnil.
Down-stream services that previously consumed an absent field (""in JSON) may now receive the literal valuenull, which can break strict decoders or cause subtle logic branches.- ChunkProofs []*OpenVMChunkProof `json:"chunk_proofs"` + ChunkProofs []*OpenVMChunkProof `json:"chunk_proofs,omitempty"`Consider doing the same for
BlobBytes,ChunkInfos, etc., if they may be empty at marshaling time.
114-116: Mirror theomitemptychange forBatchProofsFor consistency with the suggestion above, and to keep the marshalled JSON surface identical between task types, mark
BatchProofsasomitemptyas well:- BatchProofs []*OpenVMBatchProof `json:"batch_proofs"` + BatchProofs []*OpenVMBatchProof `json:"batch_proofs,omitempty"`This avoids emitting
nullwhen a bundle task is constructed before proofs are attached.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
.github/workflows/prover.yml(0 hunks)common/types/message/message.go(1 hunks)coordinator/cmd/api/app/mock_app.go(0 hunks)coordinator/cmd/tool/tool.go(1 hunks)coordinator/conf/config.json(0 hunks)coordinator/internal/config/config.go(0 hunks)coordinator/internal/controller/api/controller.go(1 hunks)coordinator/internal/logic/auth/login.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(2 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(3 hunks)coordinator/internal/logic/provertask/chunk_prover_task.go(1 hunks)coordinator/internal/logic/provertask/prover_task.go(0 hunks)coordinator/internal/logic/submitproof/proof_receiver.go(1 hunks)coordinator/internal/logic/verifier/mock.go(1 hunks)coordinator/internal/logic/verifier/types.go(0 hunks)coordinator/internal/logic/verifier/verifier.go(3 hunks)coordinator/internal/logic/verifier/verifier_test.go(1 hunks)coordinator/internal/types/auth_test.go(2 hunks)coordinator/internal/types/prover.go(3 hunks)coordinator/test/api_test.go(0 hunks)coordinator/test/mock_prover.go(2 hunks)prover/Cargo.toml(0 hunks)prover/Makefile(0 hunks)prover/config.json(0 hunks)prover/print_halo2gpu_version.sh(0 hunks)prover/print_high_zkevm_version.sh(0 hunks)prover/rust-toolchain(0 hunks)prover/rustfmt.toml(0 hunks)prover/src/config.rs(0 hunks)prover/src/main.rs(0 hunks)prover/src/prover.rs(0 hunks)prover/src/types.rs(0 hunks)prover/src/utils.rs(0 hunks)prover/src/zk_circuits_handler.rs(0 hunks)prover/src/zk_circuits_handler/common.rs(0 hunks)prover/src/zk_circuits_handler/darwin.rs(0 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(0 hunks)rollup/internal/controller/relayer/l2_relayer.go(3 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(1 hunks)rollup/internal/orm/batch.go(3 hunks)rollup/internal/orm/bundle.go(3 hunks)rollup/internal/orm/orm_test.go(4 hunks)rollup/tests/rollup_test.go(2 hunks)
💤 Files with no reviewable changes (23)
- prover/rust-toolchain
- coordinator/cmd/api/app/mock_app.go
- prover/rustfmt.toml
- coordinator/internal/config/config.go
- coordinator/conf/config.json
- prover/config.json
- prover/print_halo2gpu_version.sh
- coordinator/test/api_test.go
- prover/print_high_zkevm_version.sh
- coordinator/internal/logic/provertask/prover_task.go
- prover/Makefile
- prover/src/config.rs
- coordinator/internal/logic/verifier/types.go
- prover/Cargo.toml
- .github/workflows/prover.yml
- prover/src/main.rs
- prover/src/utils.rs
- prover/src/zk_circuits_handler/common.rs
- prover/src/types.rs
- prover/src/zk_circuits_handler.rs
- prover/src/prover.rs
- prover/src/zk_circuits_handler/darwin_v2.rs
- prover/src/zk_circuits_handler/darwin.rs
🧰 Additional context used
🧬 Code Graph Analysis (14)
rollup/internal/controller/relayer/l2_relayer_test.go (1)
common/types/message/message.go (1)
OpenVMBundleProof(250-259)
coordinator/internal/logic/provertask/chunk_prover_task.go (1)
common/types/message/message.go (1)
EuclidV2ForkNameForProver(17-17)
coordinator/cmd/tool/tool.go (1)
common/types/message/message.go (1)
OpenVMBatchProof(190-199)
coordinator/internal/types/auth_test.go (1)
coordinator/internal/types/prover.go (2)
ProverType(19-19)ProverTypeOpenVM(42-42)
rollup/tests/rollup_test.go (1)
common/types/message/message.go (2)
OpenVMBatchProof(190-199)OpenVMBundleProof(250-259)
coordinator/internal/logic/verifier/verifier_test.go (1)
common/types/message/message.go (2)
OpenVMBatchProof(190-199)OpenVMChunkProof(158-166)
coordinator/internal/logic/provertask/bundle_prover_task.go (1)
common/types/message/message.go (2)
OpenVMBatchProof(190-199)EuclidV2ForkNameForProver(17-17)
rollup/internal/controller/relayer/l2_relayer.go (3)
common/types/message/message.go (1)
OpenVMBundleProof(250-259)rollup/internal/orm/batch.go (2)
Batch(22-72)Batch(80-82)rollup/internal/orm/chunk.go (2)
Chunk(20-64)Chunk(72-74)
coordinator/internal/logic/submitproof/proof_receiver.go (1)
common/types/message/message.go (3)
OpenVMChunkProof(158-166)OpenVMBatchProof(190-199)OpenVMBundleProof(250-259)
rollup/internal/orm/batch.go (3)
coordinator/internal/orm/batch.go (2)
Batch(18-72)Batch(80-82)tests/integration-test/orm/batch.go (2)
Batch(17-60)Batch(68-70)common/types/message/message.go (1)
OpenVMBatchProof(190-199)
rollup/internal/orm/orm_test.go (1)
common/types/message/message.go (1)
OpenVMBundleProof(250-259)
coordinator/test/mock_prover.go (2)
common/types/message/message.go (3)
OpenVMChunkProof(158-166)OpenVMBatchProof(190-199)OpenVMProof(146-149)coordinator/internal/logic/verifier/types.go (1)
InvalidTestProof(8-8)
coordinator/internal/logic/verifier/mock.go (2)
coordinator/internal/logic/verifier/types.go (2)
Verifier(11-14)InvalidTestProof(8-8)common/types/message/message.go (3)
OpenVMChunkProof(158-166)OpenVMBatchProof(190-199)OpenVMBundleProof(250-259)
coordinator/internal/logic/verifier/verifier.go (2)
coordinator/internal/logic/verifier/types.go (1)
Verifier(11-14)common/types/message/message.go (3)
OpenVMBatchProof(190-199)OpenVMChunkProof(158-166)OpenVMBundleProof(250-259)
🪛 golangci-lint (1.64.8)
coordinator/internal/logic/provertask/batch_prover_task.go
257-257: cannot use chunkProofs (variable of type []message.OpenVMChunkProof) as []*message.OpenVMChunkProof value in struct literal
(typecheck)
🪛 GitHub Check: tests
coordinator/internal/logic/provertask/batch_prover_task.go
[failure] 257-257:
cannot use chunkProofs (variable of type []message.OpenVMChunkProof) as []*message.OpenVMChunkProof value in struct literal
coordinator/internal/logic/verifier/mock.go
[failure] 34-34:
proof.VmProof undefined (type *message.OpenVMBundleProof has no field or method VmProof)
🪛 GitHub Actions: Coordinator
coordinator/internal/logic/provertask/batch_prover_task.go
[error] 225-225: Type mismatch error: cannot use chunkProofs (type []*message.OpenVMChunkProof) as []message.OpenVMChunkProof in argument to bp.getBatchTaskDetail
coordinator/internal/logic/verifier/mock.go
[error] 34-34: Compilation error: proof.VmProof undefined (type *message.OpenVMBundleProof has no field or method VmProof)
🪛 GitHub Actions: Integration
coordinator/internal/logic/verifier/mock.go
[error] 34-34: proof.VmProof undefined (type *message.OpenVMBundleProof has no field or method VmProof)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (28)
coordinator/internal/controller/api/controller.go (1)
29-29: Simplified logging for verifier mapsThe logging has been streamlined to only include the OpenVMVkMap, removing references to legacy verification key maps (like ChunkVKMap, BatchVKMap, BundleVkMap) that are no longer needed. This change aligns with the broader refactoring to remove legacy proof types and focus exclusively on OpenVM proofs.
rollup/internal/controller/relayer/l2_relayer_test.go (1)
151-152: Updated to use OpenVMBundleProof instead of legacy proof typeThe test now uses the new
OpenVMBundleProofstruct with a simpler structure, setting only theVkfield. This aligns with the codebase's migration from legacy Halo2 proof abstractions to OpenVM proof types.coordinator/internal/logic/provertask/chunk_prover_task.go (1)
194-194: Simplified ForkName assignmentThe conditional logic for assigning the fork name has been removed, directly setting it to
message.EuclidV2ForkNameForProver("euclidv2"). This simplification is part of the broader refactoring to standardize on the Euclid V2 fork and remove support for legacy circuit versions.coordinator/internal/types/auth_test.go (2)
25-25: Updated to use OpenVM prover typeReplaced legacy
ProverTypeBatchwithProverTypeOpenVMto align with the codebase's migration to exclusively use OpenVM proof types.
67-67: Updated to use OpenVM prover typeReplaced legacy
ProverTypeChunkwithProverTypeOpenVMto align with the codebase's migration to exclusively use OpenVM proof types.rollup/tests/rollup_test.go (2)
150-152: Proof type updated to OpenVMBatchProofThe test is now using the new
OpenVMBatchProoftype which replaces the legacyHalo2BatchProoftype. This change is part of the refactoring to simplify the codebase post-Euclid by removing all legacy proof types.The new proof type requires only the
Vkfield compared to the previous implementation that included additional fields likeRawProofandInstances.
163-165: Proof type updated to OpenVMBundleProofSimilar to the batch proof change above, the test now uses the simplified
OpenVMBundleProoftype instead of the legacyHalo2BundleProof. This continues the standardization on OpenVM proof types throughout the codebase.coordinator/cmd/tool/tool.go (1)
65-73: Updated proof type from BatchProof interface to concrete OpenVMBatchProofThe code now uses a slice of concrete
*message.OpenVMBatchProoftypes instead of an interface type. This change:
- Simplifies the code by removing the need for factory functions that handle different proof types
- Makes the type more explicit, improving code readability
- Aligns with the broader refactoring to eliminate legacy proof abstractions
The instantiation is also simplified by directly creating an empty struct rather than relying on a factory function that took fork information as a parameter.
rollup/internal/orm/orm_test.go (2)
454-464: Test updated to use OpenVMBundleProofThe test now uses the new
OpenVMBundleProoftype instead of the legacyHalo2BundleProoftype. The assertion is also updated to use theProof()method rather than accessing internal fields directly, following good object-oriented practices of encapsulation.This test update ensures proper verification of the database ORM layer with the new proof types.
475-490: Updated test case for UpdateProofAndProvingStatusByHashThe test for
UpdateProofAndProvingStatusByHashnow properly uses theOpenVMBundleProoftype and its associated methods. This ensures that proof persistence and retrieval work correctly with the new proof structure.The change maintains the same test verification logic while adapting to the new proof type interface.
coordinator/internal/logic/submitproof/proof_receiver.go (3)
174-178: Simplified chunk proof creationThe code now directly instantiates an
OpenVMChunkProofinstead of using a factory function that required hard fork information. This simplification:
- Removes conditional logic based on hard fork names
- Makes the code more straightforward and easier to understand
- Aligns with the broader refactoring to use OpenVM proof types exclusively
The hard fork name is still used for verification later, maintaining compatibility with the verification process.
180-185: Simplified batch proof creationSimilar to the chunk proof change, the code now directly creates an
OpenVMBatchProofinstead of using a factory function. This continues the pattern of simplification across all proof types.The proof unmarshalling and verification process remains unchanged, ensuring that functionality is preserved while the code structure is improved.
186-191: Simplified bundle proof creationCompleting the pattern, bundle proof creation is also simplified to use direct instantiation of
OpenVMBundleProof. This change maintains consistency with the other proof type changes.This refactoring successfully removes all legacy proof factory functions while maintaining the core verification functionality.
coordinator/internal/logic/provertask/bundle_prover_task.go (1)
214-216: Hard-coding the fork name may break future forks
ForkNameis now hard-coded tomessage.EuclidV2ForkNameForProver, ignoringhardForkNamederived earlier.
If another fork is enabled, bundle provers will still advertiseeuclidv2, leading to mismatched VK selection or verification failure.Consider:
- ForkName: message.EuclidV2ForkNameForProver, + ForkName: hardForkName,(or document why the constant is always correct).
coordinator/internal/types/prover.go (1)
46-53:MakeProverTypestill returns deprecated variants
ProofTypeBundle/Batchmap toProverTypeBatchDeprecated, andProofTypeChunkmaps toProverTypeChunkDeprecated.
If the intention is to phase these out in favour ofProverTypeOpenVM, update the mapping (or add a TODO) to avoid propagating “deprecated” types throughout new code paths.rollup/internal/controller/relayer/l2_relayer.go (3)
729-729: Type change from interface to concrete typeChanged
aggProoffrommessage.BundleProofinterface to a pointer to the concretemessage.OpenVMBundleProofstruct. This aligns with the broader refactoring to remove legacy proof interfaces and standardize on OpenVM proof types.
1052-1052: Parameter type changed to pointer to concrete typeChanged parameter type from
message.BundleProofinterface to*message.OpenVMBundleProof. This completes the refactoring to remove legacy proof types across the codebase.
1080-1080: Parameter type changed to pointer to concrete typeChanged parameter type from
message.BundleProofinterface to*message.OpenVMBundleProof. This completes the refactoring to remove legacy proof types across the codebase.rollup/internal/orm/batch.go (3)
124-124: Return type changed from interface to concrete pointer typeChanged return type from
message.BatchProofinterface to*message.OpenVMBatchProof. This change is part of the broader refactoring to remove legacy proof interfaces in favor of concrete OpenVM proof types.
135-135: Variable type changed to pointer to concrete typeChanged variable declaration from using the interface type to using a pointer to the concrete
message.OpenVMBatchProofstruct. This change aligns with the return type modification and simplifies the implementation.
439-439: Parameter type changed to pointer to concrete typeChanged parameter type from
message.BatchProofinterface to*message.OpenVMBatchProof. This change completes the refactoring to use concrete OpenVM proof types throughout the codebase.rollup/internal/orm/bundle.go (3)
137-137: Return type changed from interface to concrete pointer typeChanged return type from
message.BundleProofinterface to*message.OpenVMBundleProof. This change aligns with the broader refactoring to remove legacy proof interfaces in favor of concrete OpenVM proof types.
148-148: Variable type changed to pointer to concrete typeChanged variable declaration from using the interface type to using a pointer to the concrete
message.OpenVMBundleProofstruct. This aligns with the return type modification and simplifies the implementation.
259-259: Parameter type changed to pointer to concrete typeChanged parameter type from
message.BundleProofinterface to*message.OpenVMBundleProof. This change completes the refactoring to use concrete OpenVM proof types throughout the codebase.coordinator/internal/logic/verifier/mock.go (3)
13-13: Simplified NewVerifier function returnThe function now only returns the Verifier with configuration, removing the previously returned verification key maps that were used for legacy proof types. This simplification aligns with the removal of legacy proof abstractions.
17-18: Parameter type changed and condition updated for OpenVM proofChanged parameter type from
message.ChunkProofto*message.OpenVMChunkProofand updated the condition to check for invalid proof by examiningproof.VmProof.Proof.
25-26: Parameter type changed and condition updated for OpenVM proofChanged parameter type from
message.BatchProofto*message.OpenVMBatchProofand updated the condition to check for invalid proof by examiningproof.VmProof.Proof.coordinator/internal/logic/provertask/batch_prover_task.go (1)
225-229: Update call after type fixAfter changing the parameter type of
getBatchTaskDetail(next comment), the call site stays valid, but ensure you re-rungo vetto confirm no further mismatches remain.🧰 Tools
🪛 golangci-lint (1.64.8)
225-225: cannot use chunkProofs (variable of type []*message.OpenVMChunkProof) as []message.OpenVMChunkProof value in argument to bp.getBatchTaskDetail
(typecheck)
🪛 GitHub Check: tests
[failure] 225-225:
cannot use chunkProofs (variable of type []*message.OpenVMChunkProof) as []message.OpenVMChunkProof value in argument to bp.getBatchTaskDetail🪛 GitHub Actions: Coordinator
[error] 225-225: Type mismatch error: cannot use chunkProofs (type []*message.OpenVMChunkProof) as []message.OpenVMChunkProof in argument to bp.getBatchTaskDetail
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CONTRIBUTING.md (1)
14-14: Fix grammar and improve clarity in the new contribution guideline sentence.The sentence is currently a bit long, contains a stray “to” (“contribute to by adding…”), and is missing punctuation. Consider splitting it and tightening the wording for readability:
-Did you know there are many ways of contributing to Scroll? If you are looking to contribute to by adding Scroll to existing Dev Tools or by doing integrations please go to the [Contribute to Scroll](https://github.com/scroll-tech/contribute-to-scroll) repo instead. This repository covers the Scroll infrastructure and smart contracts, if you want to contribute to these areas continue reading this document. +Did you know there are many ways to contribute to Scroll? If you're looking to add Scroll to existing Dev Tools or integrations, please go to the [Contribute to Scroll](https://github.com/scroll-tech/contribute-to-scroll) repo instead. + +This repository covers the Scroll infrastructure and smart contracts. If you want to contribute to these areas, continue reading this document.🧰 Tools
🪛 LanguageTool
[typographical] ~14-~14: It seems that a comma is missing.
Context: ...sting Dev Tools or by doing integrations please go to the [Contribute to Scroll](https:...(IF_PLEASE_COMMA)
[uncategorized] ~14-~14: A comma might be missing here.
Context: ...cts, if you want to contribute to these areas continue reading this document. ## Iss...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
coordinator/internal/logic/provertask/batch_prover_task.go (1)
260-265: Return a typed constant instead of a raw string & keep room for future forksThe fork gate keeps only
EuclidV2Fork; all other values throw an error, which is fine today but brittle for future forks and hard-codes the log level toError.Consider using a
switchwith explicit typed constants and logging unknown forks atWarnbefore erroring. This makes the intent clearer and easier to extend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CONTRIBUTING.md(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(2 hunks)coordinator/internal/logic/verifier/mock.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(3 hunks)coordinator/test/api_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- coordinator/test/api_test.go
- coordinator/internal/logic/verifier/mock.go
- coordinator/internal/logic/verifier/verifier.go
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md
[typographical] ~14-~14: It seems that a comma is missing.
Context: ...sting Dev Tools or by doing integrations please go to the [Contribute to Scroll](https:...
(IF_PLEASE_COMMA)
[uncategorized] ~14-~14: A comma might be missing here.
Context: ...cts, if you want to contribute to these areas continue reading this document. ## Iss...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (1)
coordinator/internal/logic/provertask/batch_prover_task.go (1)
210-221: Defensive-checkproof.MetaDatato prevent panics
proof.MetaData(and the nestedChunkInfo) originate from external JSON and may legally benull.
Dereferencing it without a check will panic and crash the coordinator.if proof.MetaData == nil || proof.MetaData.ChunkInfo == nil { return nil, fmt.Errorf("missing MetaData in chunk proof, taskID:%s chunkHash:%s", task.TaskID, chunk.Hash) }Place the guard before constructing
chunkInfo.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
rollup/tests/rollup_test.go (1)
176-176:⚠️ Potential issueVariable 'bundleProof' is used but not defined.
This line references a variable
bundleProofthat is not defined in the visible code. It should use theproofvariable defined on line 172.- err = bundleOrm.UpdateProofAndProvingStatusByHash(context.Background(), bundle.Hash, bundleProof, types.ProvingTaskVerified, 100) + err = bundleOrm.UpdateProofAndProvingStatusByHash(context.Background(), bundle.Hash, proof, types.ProvingTaskVerified, 100)🧰 Tools
🪛 golangci-lint (1.64.8)
176-176: undefined: bundleProof
(typecheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
coordinator/cmd/api/app/mock_app.go(1 hunks)coordinator/internal/config/config.go(0 hunks)coordinator/internal/logic/verifier/types.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(3 hunks)coordinator/internal/logic/verifier/verifier_test.go(2 hunks)coordinator/test/api_test.go(13 hunks)coordinator/test/mock_prover.go(2 hunks)rollup/internal/controller/relayer/l2_relayer.go(3 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(1 hunks)rollup/internal/orm/batch.go(3 hunks)rollup/internal/orm/bundle.go(3 hunks)rollup/internal/orm/orm_test.go(4 hunks)rollup/tests/bridge_test.go(1 hunks)rollup/tests/rollup_test.go(3 hunks)
💤 Files with no reviewable changes (1)
- coordinator/internal/config/config.go
✅ Files skipped from review due to trivial changes (1)
- rollup/tests/bridge_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- coordinator/cmd/api/app/mock_app.go
- rollup/internal/controller/relayer/l2_relayer_test.go
- coordinator/internal/logic/verifier/types.go
- coordinator/test/mock_prover.go
- coordinator/internal/logic/verifier/verifier_test.go
- rollup/internal/orm/orm_test.go
- rollup/internal/orm/batch.go
- rollup/internal/controller/relayer/l2_relayer.go
- coordinator/test/api_test.go
- rollup/internal/orm/bundle.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
coordinator/internal/logic/verifier/verifier.go (2)
coordinator/internal/logic/verifier/types.go (1)
Verifier(11-14)common/types/message/message.go (3)
OpenVMBatchProof(190-199)OpenVMChunkProof(158-166)OpenVMBundleProof(250-259)
rollup/tests/rollup_test.go (1)
common/types/message/message.go (3)
OpenVMBatchProof(190-199)OpenVMBundleProof(250-259)OpenVMEvmProof(152-155)
🪛 golangci-lint (1.64.8)
rollup/tests/rollup_test.go
172-172: declared and not used: proof
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (11)
coordinator/internal/logic/verifier/verifier.go (7)
93-94: Method signature refactored to use OpenVM-specific proof typeThe update to use
*message.OpenVMBatchProofinstead of the previous proof type is consistent with the PR objective to remove legacy Halo2 prover logic and simplify to OpenVM-only. The implementation correctly handles marshaling and verification with the updated type.
112-113: Method signature refactored to use OpenVM-specific proof typeThe update to use
*message.OpenVMChunkProofinstead of the previous proof type is consistent with the PR objective to remove legacy Halo2 prover logic and simplify to OpenVM-only. The implementation correctly handles marshaling and verification with the updated type.
132-132: Method signature refactored to use OpenVM-specific proof typeThe update to use
*message.OpenVMBundleProofinstead of the previous proof type is consistent with the PR objective to remove legacy Halo2 prover logic and simplify to OpenVM-only. The implementation correctly handles marshaling and verification with the updated type.
46-48: Rust configuration structure simplifiedThe
rustVerifierConfigstruct has been correctly simplified to remove theLowVersionCircuitfield, retaining only theHighVersionCircuitconfiguration. This matches the overall goal of removing the legacy prover logic.
50-54: Rust verifier configuration constructor simplifiedThe configuration constructor now only initializes the high version circuit configuration, which is consistent with the removal of low version circuit functionality. This simplification aligns with the refactoring objective.
62-91: Verifier initialization focused on OpenVMThe
NewVerifierfunction has been simplified to only initialize and load OpenVM verification keys for Euclid and EuclidV2 forks, removing all low version circuit and mock mode handling. This streamlined approach aligns perfectly with the PR's objective to simplify coordinator logic post-Euclid.
162-194: Retained OpenVM verification key loading logicThe
loadOpenVMVksfunction has been preserved while removing legacy key loading methods. This function correctly handles the loading of verification keys from the Rust library and populates the OpenVMVkMap for verification.rollup/tests/rollup_test.go (4)
151-154: Correctly updated to use OpenVMBatchProof.The code has been successfully refactored to use the new OpenVM proof types, replacing the legacy Halo2 proof types. The monkey-patching for the SanityCheck method remains consistent with the old implementation.
156-156: LGTM: Successfully updated batch proof initialization.The initialization has been properly updated to use the new OpenVMBatchProof type.
167-170: Successfully updated to use OpenVMBundleProof.The monkey-patching implementation has been correctly updated to use the new OpenVMBundleProof type.
11-11: LGTM: Import maintained appropriately.The gomonkey import is correctly maintained, which is necessary for the monkey-patching used to override the SanityCheck methods.
colinlyguo
force-pushed
the
refactor-simplify-coordinator
branch
3 times, most recently
from
4dd5b28 to
c08f005
Compare
colinlyguo
changed the title
colinlyguo
force-pushed
the
refactor-simplify-coordinator
branch
from
c08f005 to
05c07a6
Compare
colinlyguo
force-pushed
the
refactor-simplify-coordinator
branch
from
05c07a6 to
62bb661
Compare
Thegaram
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
14-14: Punctuation nit: add missing commas and improve sentence flow
The sentence at line 14 combines several clauses without proper separators, which can make it harder to read. Splitting into shorter sentences and inserting commas will clarify the intent.- If you are looking to contribute by adding Scroll to existing Dev Tools or by doing integrations please go to the [Contribute to Scroll](https://github.com/scroll-tech/contribute-to-scroll) repo instead. This repository covers the Scroll infrastructure and smart contracts, if you want to contribute to these areas continue reading this document. + If you are looking to contribute by adding Scroll to existing Dev Tools or by doing integrations, please go to the [Contribute to Scroll](https://github.com/scroll-tech/contribute-to-scroll) repo instead. + This repository covers the Scroll infrastructure and smart contracts. If you want to contribute to these areas, continue reading this document.🧰 Tools
🪛 LanguageTool
[typographical] ~14-~14: It seems that a comma is missing.
Context: ...sting Dev Tools or by doing integrations please go to the [Contribute to Scroll](https:...(IF_PLEASE_COMMA)
[uncategorized] ~14-~14: A comma might be missing here.
Context: ...cts, if you want to contribute to these areas continue reading this document. ## Iss...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
.github/workflows/prover.yml(0 hunks)CONTRIBUTING.md(1 hunks)common/libzkp/impl/src/lib.rs(0 hunks)common/types/message/message.go(1 hunks)common/version/version.go(1 hunks)coordinator/cmd/api/app/mock_app.go(1 hunks)coordinator/cmd/tool/tool.go(1 hunks)coordinator/conf/config.json(1 hunks)coordinator/internal/config/config.go(0 hunks)coordinator/internal/config/config_test.go(1 hunks)coordinator/internal/controller/api/controller.go(1 hunks)coordinator/internal/logic/auth/login.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(2 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(3 hunks)coordinator/internal/logic/provertask/chunk_prover_task.go(1 hunks)coordinator/internal/logic/provertask/prover_task.go(0 hunks)coordinator/internal/logic/submitproof/proof_receiver.go(1 hunks)coordinator/internal/logic/verifier/mock.go(1 hunks)coordinator/internal/logic/verifier/types.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(3 hunks)coordinator/internal/logic/verifier/verifier_test.go(2 hunks)coordinator/internal/types/auth_test.go(2 hunks)coordinator/internal/types/prover.go(3 hunks)coordinator/test/api_test.go(13 hunks)coordinator/test/mock_prover.go(2 hunks)prover/Cargo.toml(0 hunks)prover/Makefile(0 hunks)prover/config.json(0 hunks)prover/print_halo2gpu_version.sh(0 hunks)prover/print_high_zkevm_version.sh(0 hunks)prover/rust-toolchain(0 hunks)prover/rustfmt.toml(0 hunks)prover/src/config.rs(0 hunks)prover/src/main.rs(0 hunks)prover/src/prover.rs(0 hunks)prover/src/types.rs(0 hunks)prover/src/utils.rs(0 hunks)prover/src/zk_circuits_handler.rs(0 hunks)prover/src/zk_circuits_handler/common.rs(0 hunks)prover/src/zk_circuits_handler/darwin.rs(0 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(0 hunks)rollup/internal/controller/relayer/l2_relayer.go(3 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(1 hunks)rollup/internal/orm/batch.go(3 hunks)rollup/internal/orm/bundle.go(3 hunks)rollup/internal/orm/orm_test.go(4 hunks)rollup/tests/bridge_test.go(1 hunks)rollup/tests/rollup_test.go(3 hunks)
💤 Files with no reviewable changes (20)
- prover/rust-toolchain
- coordinator/internal/config/config.go
- prover/print_high_zkevm_version.sh
- coordinator/internal/logic/provertask/prover_task.go
- common/libzkp/impl/src/lib.rs
- prover/rustfmt.toml
- prover/src/config.rs
- prover/Cargo.toml
- prover/src/zk_circuits_handler/common.rs
- prover/config.json
- prover/src/main.rs
- prover/print_halo2gpu_version.sh
- prover/Makefile
- prover/src/utils.rs
- prover/src/types.rs
- .github/workflows/prover.yml
- prover/src/zk_circuits_handler.rs
- prover/src/zk_circuits_handler/darwin_v2.rs
- prover/src/prover.rs
- prover/src/zk_circuits_handler/darwin.rs
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🚧 Files skipped from review as they are similar to previous changes (23)
- coordinator/internal/controller/api/controller.go
- coordinator/cmd/api/app/mock_app.go
- rollup/tests/bridge_test.go
- coordinator/conf/config.json
- coordinator/internal/logic/provertask/chunk_prover_task.go
- coordinator/internal/types/auth_test.go
- coordinator/internal/logic/verifier/types.go
- rollup/tests/rollup_test.go
- rollup/internal/orm/orm_test.go
- rollup/internal/controller/relayer/l2_relayer_test.go
- coordinator/internal/logic/submitproof/proof_receiver.go
- coordinator/cmd/tool/tool.go
- coordinator/internal/logic/verifier/verifier_test.go
- coordinator/internal/types/prover.go
- coordinator/internal/logic/auth/login.go
- rollup/internal/orm/batch.go
- rollup/internal/orm/bundle.go
- common/types/message/message.go
- coordinator/test/api_test.go
- coordinator/test/mock_prover.go
- rollup/internal/controller/relayer/l2_relayer.go
- coordinator/internal/logic/verifier/mock.go
- coordinator/internal/logic/provertask/batch_prover_task.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
coordinator/internal/logic/provertask/bundle_prover_task.go (2)
common/types/message/message.go (1)
OpenVMBatchProof(190-199)coordinator/internal/types/auth.go (1)
Hash(134-141)
coordinator/internal/logic/verifier/verifier.go (2)
coordinator/internal/logic/verifier/types.go (1)
Verifier(11-14)common/types/message/message.go (3)
OpenVMBatchProof(190-199)OpenVMChunkProof(158-166)OpenVMBundleProof(250-259)
🪛 LanguageTool
CONTRIBUTING.md
[typographical] ~14-~14: It seems that a comma is missing.
Context: ...sting Dev Tools or by doing integrations please go to the [Contribute to Scroll](https:...
(IF_PLEASE_COMMA)
[uncategorized] ~14-~14: A comma might be missing here.
Context: ...cts, if you want to contribute to these areas continue reading this document. ## Iss...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🔇 Additional comments (7)
coordinator/internal/config/config_test.go (1)
18-27: Configuration structure simplification looks good.The updated test configuration correctly reflects the removal of halo2 prover logic and focuses solely on the openvm prover, with appropriate settings for the new external_prover_threshold and bundle_collection_time_sec fields. The nested high_version_circuit structure provides a cleaner organization for verifier configurations.
coordinator/internal/logic/provertask/bundle_prover_task.go (3)
203-210: Good simplification of proof types.Replacing generic
message.BatchProofinterfaces with concrete*message.OpenVMBatchProoftypes simplifies the codebase and makes the intent clearer. This change aligns well with the PR's goal of removing legacy proof abstractions.
218-221: Improved error handling for unsupported fork names.Adding explicit error handling for unsupported fork names is a good improvement. This ensures that only the supported
euclidV2fork name is accepted, preventing potential issues with invalid configurations.
231-231: Simplified MsgQueueHash assignment.The MsgQueueHash field is now unconditionally assigned, which simplifies the code by removing conditional logic. This is consistent with the PR's goal of simplifying the coordinator logic.
coordinator/internal/logic/verifier/verifier.go (3)
93-94: Improved type safety with concrete OpenVM proof types.Changing the parameter type from the generic
message.BatchProofinterface to the concrete*message.OpenVMBatchProoftype improves type safety and makes the expected input clearer. This change aligns with the PR's goal of simplifying the codebase by standardizing on OpenVM proof types.
112-113: Consistent use of concrete OpenVM proof types.Similar to the batch proof verification, using the concrete
*message.OpenVMChunkProoftype instead of the interface improves type safety and clarity. This is a good change that reinforces the standardization on OpenVM proof types.
132-132: Standardized on OpenVM bundle proof type.Changing to the concrete
*message.OpenVMBundleProoftype completes the standardization of proof types across all verification methods. This is consistent with the other changes and improves the overall cohesion of the codebase.
Purpose or design rationale of this PR
This PR removes halo2 prover logic and only keeps openvm prover logic.
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Summary by CodeRabbit