fix(coordinator): prevent assign halo2 tasks to openvm provers #1616
Conversation
WalkthroughThis pull request modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as LoginClient
participant Logic as LoginLogic
participant Config as Configuration
Client->>Logic: Invoke NewLoginLogic()
Logic->>Config: Retrieve HighVersionCircuit.ForkName
alt ForkName ≠ "euclid"
Logic->>Logic: Append ForkName to highHardForks
else ForkName == "euclid"
Logic->>Logic: Skip appending fork name
end
Logic->>Logic: Populate proverVersionHardForkMap
Logic->>Client: Return initialized LoginLogic object
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package zstd: could not load export data: no export data for "github.com/scroll-tech/da-codec/encoding/zstd"" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
coordinator/internal/logic/auth/login.go (2)
45-47: Consider adding a comment explaining the special case for "euclid"This conditional check excludes "euclid" from the high hard forks list, which aligns with the PR objective to prevent halo2 tasks from being assigned to openvm provers. However, the reason for this exclusion isn't immediately clear from the code itself.
Adding a comment would improve maintainability and make the intent clearer for future developers. For example:
+ // Skip adding "euclid" to highHardForks to prevent assigning halo2 tasks to openvm provers if cfg.ProverManager.Verifier.LowVersionCircuit.ForkName != "euclid" { highHardForks = append(highHardForks, cfg.ProverManager.Verifier.LowVersionCircuit.ForkName) }
45-47: Consider making "euclid" a constant instead of a hardcoded stringUsing a hardcoded string for "euclid" might cause maintenance issues if this value needs to change in the future. Consider defining a constant for this value at the package level:
+ // EuclidForkName represents the Euclid fork + const EuclidForkName = "euclid" // Then in the conditional: - if cfg.ProverManager.Verifier.LowVersionCircuit.ForkName != "euclid" { + if cfg.ProverManager.Verifier.LowVersionCircuit.ForkName != EuclidForkName {This makes the code more maintainable and ensures consistency if this value is used elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
coordinator/internal/logic/auth/login.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (1)
coordinator/internal/logic/auth/login.go (1)
45-47: Verify test coverage for this conditional logicThis change introduces a conditional that alters the behavior of task assignments between provers. Ensure there are appropriate tests verifying that:
- The "euclid" fork name is correctly excluded from highHardForks
- Halo2 tasks are not assigned to openvm provers after this change
If tests don't exist, consider adding them to validate this important condition.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1616 +/- ##
===========================================
+ Coverage 42.10% 42.15% +0.04%
===========================================
Files 222 222
Lines 17739 17741 +2
===========================================
+ Hits 7469 7478 +9
+ Misses 9560 9555 -5
+ Partials 710 708 -2
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:
|
yiweichi
changed the title
Co-authored-by: yiweichi <yiweichi@users.noreply.github.com>
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?
To prevent coordinator assign halo2 tasks to openvm prover, specially handle the corner case.
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
Bug Fixes
New Features