feat: lazy pkey downloader #1977
Conversation
WalkthroughAdds resumable, checksum-verified proving-key downloads and a LazyKeyManager; refactors server, queue workers, tests, and CLI to use the key manager; simplifies prover startup to a parameterless spawn/start with auto-download and a GitHub-release binary downloader; updates CI to build linux/arm64 and Windows .exe artifacts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI (start-prover)
participant FS as Filesystem
participant GH as GitHub Releases
participant Prover as Prover Binary
participant Server as Prover Server
User->>CLI: start-prover --port N [--redis URL]
CLI->>FS: check platform-specific binary path
alt binary missing
CLI->>GH: download release asset (retries, redirects)
GH-->>CLI: binary stream
CLI->>FS: save file, chmod (non-Windows)
end
CLI->>Prover: exec start-prover --port N --auto-download true [--redis URL]
Prover->>Server: initialize
Server->>Server: create LazyKeyManager (DownloadConfig)
opt preload requested
Server->>Server: Preload keys/circuits (may download)
end
User-->>CLI: wait for health
CLI-->>User: prover running
sequenceDiagram
autonumber
participant Client as API Client
participant Server as Prover Server
participant KM as LazyKeyManager
participant Disk as Key Dir
participant Net as Key Source (HTTP)
Client->>Server: request proof(params)
Server->>KM: Get*System(params)
alt cached
KM-->>Server: return in-memory system
else not cached
KM->>Disk: check key file
alt file present & checksum ok
KM->>KM: load into cache
else missing/invalid
opt AutoDownload enabled
KM->>Net: download key (resumable, verify checksum)
Net-->>KM: key bytes
KM->>Disk: persist & verify
KM->>KM: load into cache
end
end
KM-->>Server: return system or error
end
Server->>Server: generate proof
Server-->>Client: return proof
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (24)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sergeytimoshin
changed the title
SwenSchaeferjohann
left a comment
SwenSchaeferjohann
left a comment
There was a problem hiding this comment.
drive-by merge assist
|
@coderabbitai review |
ananas-block
left a comment
ananas-block
left a comment
There was a problem hiding this comment.
Getting this on a fresh install running cargo test-sbf -p compressed-token-test -- --test test_22_transfer
No running prover found with the requested configuration.
Kill existing prover process...
Starting prover in forester-test mode...
prover-darwin-arm64 process exited with code 1
Prover process failed with exit code 1
========== Contents of prover-darwin-arm64.log ==========
Incorrect Usage: flag provided but not defined: -auto-download
NAME:
prover-darwin-arm64 start
USAGE:
prover-darwin-arm64 start [command options]
COMMANDS:
help, h Shows a list of commands or help for one command
OPTIONS:
--json-logging enable JSON logging (default: false)
--prover-address value address for the prover server (default: "0.0.0.0:3001")
--metrics-address value address for the metrics server (default: "0.0.0.0:9998")
--keys-dir value Directory where key files are stored (default: "./proving-keys/")
--circuit value [ --circuit value ] Specify the circuits to enable (inclusion, non-inclusion, combined, append, update, append-test, update-test, address-append, address-append-test)
--run-mode value Specify the running mode (rpc, forester, forester-test, full, or full-test)
--redis-url value Redis URL for queue processing (e.g., redis://localhost:6379)
--queue-only Run only queue workers (no HTTP server) (default: false)
--server-only Run only HTTP server (no queue workers) (default: false)
--help, -h show help
16:25:29 FTL App failed. error="flag provided but not defined: -auto-download"
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/prover-test.yml (1)
106-151: Target real test packages when filtering by-run.
go test -v -run …without any package args only runs tests in the current package (.). Most of the prover tests live in nested packages, so these matrix entries now miss them completely. Add./...so the regex is applied across the module:- go test -v -run TestWorkerSelection -timeout 5m + go test ./... -v -run TestWorkerSelection -timeout 5m - go test -v -run TestBatchOperations -timeout 5m + go test ./... -v -run TestBatchOperations -timeout 5m(Apply the same fix to the Redis suite commands to keep coverage intact.)
♻️ Duplicate comments (1)
.github/actions/setup-and-build-nocheck/action.yml (1)
23-23: Cache key simplification is fine; align validation with lazy keys.You still validate that prover/server/proving-keys exists (Line 51), which may be unnecessary with auto-download. Either relax the check or remove the action if unused.
[ suggest_recommended_refactor ]
🧹 Nitpick comments (8)
prover/server/prover/common/proving_keys_utils.go (1)
287-291: Pre-load key existence with auto-download is the right place.Good early failure. Optional: consider a config switch to verify checksums of already-present keys (can be heavy) for integrity-sensitive deployments.
cli/src/utils/processProverServer.ts (2)
90-111: Ensure executable bit when binary already exists.If an existing binary lacks +x (common after manual copy), spawn may fail. On non-Windows, re-assert 0o755 when the file exists.
async function ensureProverBinary(): Promise<void> { const binaryPath = getProverPathByArch(); const binaryName = getProverNameByArch(); - if (fs.existsSync(binaryPath)) { - return; - } + if (fs.existsSync(binaryPath)) { + if (process.platform !== "win32") { + try { + fs.chmodSync(binaryPath, 0o755); + } catch {} + } + return; + }
136-150: Defaulting to local-rpc and enabling --auto-download by default.This matches the new server behavior. Consider exposing a CLI flag to disable auto-download for air‑gapped environments and pass it through instead of hardcoding "true".
prover/server/integration_test.go (1)
108-122: Replace fixed sleep with readiness check.time.Sleep(1s) is flaky on CI. Poll /health (with timeout) before running tests.
prover/server/prover/common/key_downloader.go (4)
16-21: Make BaseURL configurable via env/flag.Hardcoding a dated bucket path may require rebuilds for key rotations. Allow override (e.g., PROVING_KEYS_BASE_URL) while keeping DefaultBaseURL.
-const ( - DefaultBaseURL = "https://storage.googleapis.com/light-protocol-proving-keys/proving-keys-06-10-25" +const ( + DefaultBaseURL = "https://storage.googleapis.com/light-protocol-proving-keys/proving-keys-06-10-25" ) +func baseURLFromEnv(def string) string { + if v := os.Getenv("PROVING_KEYS_BASE_URL"); v != "" { + return v + } + return def +}And in DefaultDownloadConfig:
- BaseURL: DefaultBaseURL, + BaseURL: baseURLFromEnv(DefaultBaseURL),
41-50: Global checksum cache lacks concurrency safety.Add sync.Once/Mutex to avoid races under concurrent EnsureKeysExist/DownloadKey calls.
+import "sync" type checksumCache struct { checksums map[string]string loaded bool + mu sync.RWMutex + once sync.Once } var globalChecksumCache = &checksumCache{ checksums: make(map[string]string), loaded: false }And guard reads/writes in downloadChecksum and lookups with mu/once.
51-97: Checksum load: scope and parsing are fine; add once+error retention.Wrap with once.Do and store last error to prevent repeated fetches on failures; avoid partial caches.
262-327: Consider optional verification of existing keys at startup.Currently only missing keys are checked; existing files aren’t validated unless re-downloaded. Add a config flag (e.g., VerifyExisting) to optionally verify all present keys on startup for integrity-sensitive runs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/actions/setup-and-build-nocheck/action.yml(2 hunks).github/actions/setup-and-build/action.yml(3 hunks).github/workflows/prover-release.yml(2 hunks).github/workflows/prover-test.yml(3 hunks)cli/package.json(2 hunks)cli/scripts/buildProver.sh(0 hunks)cli/src/utils/downloadProverBinary.ts(1 hunks)cli/src/utils/processProverServer.ts(5 hunks)prover/server/README.md(0 hunks)prover/server/integration_test.go(2 hunks)prover/server/main.go(3 hunks)prover/server/prover/common/key_downloader.go(1 hunks)prover/server/prover/common/proving_keys_utils.go(1 hunks)prover/server/scripts/download_keys.sh(0 hunks)scripts/install.sh(0 hunks)
💤 Files with no reviewable changes (4)
- cli/scripts/buildProver.sh
- prover/server/README.md
- scripts/install.sh
- prover/server/scripts/download_keys.sh
🧰 Additional context used
🧬 Code graph analysis (5)
cli/src/utils/downloadProverBinary.ts (1)
cli/src/psp-utils/download.ts (1)
downloadFile(142-211)
prover/server/integration_test.go (2)
prover/server/prover/common/key_downloader.go (1)
DefaultDownloadConfig(31-39)prover/server/prover/common/proving_keys_utils.go (1)
LoadKeysWithConfig(282-318)
cli/src/utils/processProverServer.ts (1)
cli/src/utils/downloadProverBinary.ts (1)
downloadProverBinary(15-60)
prover/server/prover/common/proving_keys_utils.go (1)
prover/server/prover/common/key_downloader.go (3)
DefaultDownloadConfig(31-39)DownloadConfig(23-29)EnsureKeysExist(329-370)
prover/server/main.go (2)
prover/server/prover/common/key_downloader.go (6)
DefaultBaseURL(17-17)DefaultMaxRetries(18-18)DownloadConfig(23-29)DefaultRetryDelay(19-19)DefaultMaxRetryDelay(20-20)EnsureKeysExist(329-370)prover/server/prover/common/proving_keys_utils.go (2)
GetKeys(112-276)LoadKeysWithConfig(282-318)
⏰ 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: test-with-redis (Failed job status tests, go test -v -run TestFailedJobStatus -timeout 5m)
- GitHub Check: test-no-redis (Batch operations queue routing tests, go test -v -run TestBatchOperations -timeout...
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: lint
- GitHub Check: cli-v2
- GitHub Check: stateless-js-v2
- GitHub Check: stateless-js-v1
🔇 Additional comments (11)
.github/workflows/prover-release.yml (1)
17-34: Thanks for the cross-platform touch-upsPulling
go-versionfrom the module file, expanding the build loop to cover linux/arm64, and appending.exefor Windows keeps the workflow aligned with the new targets without complicating the script. Looks solid.cli/package.json (2)
21-22: Good call excluding heavy assets from the package.Dropping proving-keys and prover-* from publish aligns with lazy downloads and keeps the tarball small. Ensure all runtime paths rely on on-demand downloads (they do via processProverServer).
95-99: Scripts simplification looks fine; confirm release parity.Using the same add-bins step for build and build-release removes divergence. Verify that no prior release-only steps are required anymore (e.g., prover build), given binaries are now downloaded at runtime.
.github/actions/setup-and-build-nocheck/action.yml (1)
137-137: Mirror key change for save step is consistent.Keying both restore/save only off scripts/install.sh is consistent with the new model.
.github/actions/setup-and-build/action.yml (3)
31-31: Cache key scope reduction matches removal of key-download script.Looks good. Keeping the proving-keys path in the cache while not enforcing their presence in validation is consistent with lazy provisioning.
100-100: Removing proving-keys presence check avoids brittle CI.This unblocks runs where keys are fetched on demand.
201-201: Consistent cache key update in save step.No issues.
prover/server/prover/common/proving_keys_utils.go (1)
279-280: Nice wrapper; defaulting to DownloadConfig keeps existing callers working.This preserves API while enabling configurability.
prover/server/integration_test.go (1)
41-48: Switch to config-driven key loading is solid.Using DefaultDownloadConfig with AutoDownload true keeps tests hermetic.
prover/server/prover/common/key_downloader.go (2)
123-261: Resumable download implementation is solid.Exponential backoff, Range support, and progress logging look good.
329-370: EnsureKeysExist behavior is correct.AutoDownload=false case and selective missing-key downloads are implemented cleanly.
sergeytimoshin
force-pushed
the
sergey/prover-lazy-keys
branch
from
699556d to
cc1d339
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
prover/client/src/prover.rs (1)
19-40: Fix lifetime bug inprover_pathselectionThe
#[cfg(feature = "devenv")]arm returns&format!(...), which is a reference to a temporaryString. As soon as the block ends that temporary is dropped, so this code will fail to compile (borrowed value does not live long enough) whenever thedevenvfeature is enabled. Please store the owned string and pass a reference when building the command.- let prover_path: &str = { + let prover_path = { #[cfg(feature = "devenv")] { - &format!("{}/{}", _project_root.trim(), "cli/test_bin/run") + format!("{}/{}", _project_root.trim(), "cli/test_bin/run") } #[cfg(not(feature = "devenv"))] { println!("Running in production mode, using prover binary"); - "light" + "light".to_string() } }; - let command = Command::new(prover_path) + let command = Command::new(&prover_path) .arg("start-prover") .spawn() .expect("Failed to start prover process");sdk-libs/client/src/local_test_validator.rs (1)
6-12: Fix test initializations of LightValidatorConfig
All tests still reference the removedprover_configfield. Update eachLightValidatorConfig { … prover_config: … }to useenable_prover: trueorfalseand removeProverConfigimports.
♻️ Duplicate comments (3)
.github/workflows/prover-test.yml (2)
70-99: Unit test job still points at non-existent package pathRunning
go test ./prover/...fromprover/serverfails withpattern ./prover/...: no matching packages, so the “Unit tests” matrix entry never exercises anything. Switch back to the module root pattern.- - name: "Unit tests" - command: "go test ./prover/... -timeout 60m" + - name: "Unit tests" + command: "go test ./... -timeout 60m"
219-249: Full integration suite still skips release branch pushesThis job only runs for PRs whose base starts with
release, so direct pushes torelease/**(which this workflow listens to) bypass the full integration tests. Extend the condition to allow those push events too.- if: | - github.event_name == 'pull_request' && - github.event.pull_request.draft == false && - startsWith(github.event.pull_request.base.ref, 'release') + if: | + (github.event_name == 'pull_request' && + github.event.pull_request.draft == false && + startsWith(github.event.pull_request.base.ref, 'release')) || + (github.event_name == 'push' && + startsWith(github.ref, 'refs/heads/release/'))cli/src/utils/processProverServer.ts (1)
135-157: Windows ARM64 download still 404s
getProverNameByArch()requestsprover-windows-arm64.exe, but the project’s releases still publish onlyprover-windows-amd64.exe. On Windows ARM64 we’ll now fail every time with “Failed to download prover binary”. Please either ship that asset or add a graceful fallback path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.github/actions/setup-and-build-nocheck/action.yml(2 hunks).github/actions/setup-and-build/action.yml(3 hunks).github/workflows/prover-release.yml(2 hunks).github/workflows/prover-test.yml(3 hunks)cli/package.json(2 hunks)cli/scripts/buildProver.sh(0 hunks)cli/src/commands/start-prover/index.ts(1 hunks)cli/src/utils/downloadProverBinary.ts(1 hunks)cli/src/utils/initTestEnv.ts(1 hunks)cli/src/utils/processProverServer.ts(4 hunks)program-tests/compressed-token-test/tests/test.rs(15 hunks)program-tests/utils/src/e2e_test_env.rs(0 hunks)prover/client/src/prover.rs(2 hunks)prover/client/tests/batch_address_append.rs(2 hunks)prover/client/tests/batch_append.rs(2 hunks)prover/client/tests/batch_update.rs(2 hunks)prover/client/tests/combined.rs(2 hunks)prover/client/tests/inclusion.rs(2 hunks)prover/client/tests/non_inclusion.rs(2 hunks)prover/server/README.md(0 hunks)prover/server/integration_test.go(3 hunks)prover/server/main.go(6 hunks)prover/server/prover/common/key_downloader.go(1 hunks)prover/server/prover/common/lazy_key_manager.go(1 hunks)prover/server/prover/common/proving_keys_utils.go(1 hunks)prover/server/redis_queue_test.go(3 hunks)prover/server/scripts/download_keys.sh(0 hunks)prover/server/server/queue_job.go(8 hunks)prover/server/server/server.go(10 hunks)scripts/install.sh(0 hunks)sdk-libs/client/src/lib.rs(1 hunks)sdk-libs/client/src/local_test_validator.rs(3 hunks)sdk-libs/program-test/src/program_test/config.rs(0 hunks)sdk-libs/program-test/src/program_test/light_program_test.rs(2 hunks)
💤 Files with no reviewable changes (6)
- program-tests/utils/src/e2e_test_env.rs
- scripts/install.sh
- sdk-libs/program-test/src/program_test/config.rs
- cli/scripts/buildProver.sh
- prover/server/scripts/download_keys.sh
- prover/server/README.md
✅ Files skipped from review due to trivial changes (1)
- sdk-libs/client/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/actions/setup-and-build-nocheck/action.yml
- cli/package.json
🧰 Additional context used
🧬 Code graph analysis (20)
prover/server/prover/common/proving_keys_utils.go (1)
prover/server/prover/common/key_downloader.go (3)
DefaultDownloadConfig(31-39)DownloadConfig(23-29)EnsureKeysExist(329-370)
cli/src/utils/downloadProverBinary.ts (1)
cli/src/psp-utils/download.ts (1)
downloadFile(142-211)
prover/client/tests/combined.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
prover/client/tests/batch_append.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
prover/client/tests/batch_update.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
cli/src/commands/start-prover/index.ts (1)
cli/src/utils/processProverServer.ts (1)
startProver(113-133)
cli/src/utils/processProverServer.ts (3)
cli/src/utils/downloadProverBinary.ts (1)
downloadProverBinary(15-60)cli/src/utils/process.ts (1)
killProcessByPort(74-82)cli/src/utils/constants.ts (1)
BASE_PATH(30-30)
cli/src/utils/initTestEnv.ts (1)
cli/src/utils/processProverServer.ts (1)
startProver(113-133)
prover/server/prover/common/lazy_key_manager.go (2)
prover/server/prover/common/key_downloader.go (3)
DownloadConfig(23-29)DefaultDownloadConfig(31-39)DownloadKey(262-327)prover/server/prover/common/proving_keys_utils.go (4)
RunMode(16-16)GetKeys(112-276)Full(22-22)FullTest(23-23)
prover/server/server/queue_job.go (2)
prover/server/prover/common/lazy_key_manager.go (1)
LazyKeyManager(10-17)prover/server/server/queue.go (1)
RedisQueue(14-17)
sdk-libs/program-test/src/program_test/light_program_test.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
prover/client/tests/inclusion.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
prover/client/tests/batch_address_append.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
program-tests/compressed-token-test/tests/test.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
prover/client/tests/non_inclusion.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
prover/server/server/server.go (2)
prover/server/prover/common/lazy_key_manager.go (1)
LazyKeyManager(10-17)prover/server/server/queue.go (1)
RedisQueue(14-17)
prover/server/main.go (5)
prover/server/prover/common/key_downloader.go (6)
DefaultBaseURL(17-17)DefaultMaxRetries(18-18)DownloadConfig(23-29)DefaultRetryDelay(19-19)DefaultMaxRetryDelay(20-20)EnsureKeysExist(329-370)prover/server/prover/common/proving_keys_utils.go (6)
GetKeys(112-276)RunMode(16-16)Forester(19-19)ForesterTest(20-20)Full(22-22)FullTest(23-23)prover/server/prover/common/lazy_key_manager.go (1)
NewLazyKeyManager(19-30)prover/server/server/queue_job.go (3)
NewUpdateQueueWorker(51-61)NewAppendQueueWorker(63-73)NewAddressAppendQueueWorker(75-85)prover/server/server/server.go (2)
RunWithQueue(459-467)Run(582-584)
prover/server/redis_queue_test.go (4)
prover/server/prover/common/lazy_key_manager.go (1)
NewLazyKeyManager(19-30)prover/server/prover/common/key_downloader.go (1)
DefaultDownloadConfig(31-39)prover/server/server/queue_job.go (3)
NewUpdateQueueWorker(51-61)NewAppendQueueWorker(63-73)NewAddressAppendQueueWorker(75-85)prover/server/server/server.go (1)
RunEnhanced(469-580)
prover/client/src/prover.rs (2)
sdk-libs/program-test/src/program_test/light_program_test.rs (1)
new(55-332)sdk-libs/program-test/src/program_test/config.rs (1)
new(56-65)
prover/server/integration_test.go (4)
prover/server/prover/common/proving_keys_utils.go (3)
RunMode(16-16)FullTest(23-23)Full(22-22)prover/server/prover/common/key_downloader.go (1)
DefaultDownloadConfig(31-39)prover/server/prover/common/lazy_key_manager.go (1)
NewLazyKeyManager(19-30)prover/server/server/server.go (2)
Config(623-626)Run(582-584)
⏰ 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). (26)
- GitHub Check: lean-verification
- GitHub Check: integration-test-lightweight-lazy
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: integration-test-lightweight
- GitHub Check: stateless-js-v2
- GitHub Check: cli-v1
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: stateless-js-v1
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: Forester e2e test
- GitHub Check: lint
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client batched-mer...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: cli-v2
🔇 Additional comments (1)
sdk-libs/client/src/local_test_validator.rs (1)
45-47: LGTM! Clearer and more straightforward logic.The simplified check using the boolean flag is more readable than the previous approach and correctly appends
--skip-proverwhen the prover is disabled.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
program-tests/batched-merkle-tree-test/tests/merkle_tree.rs (1)
449-449: Correct usage of the simplified prover API.The no-argument
spawn_prover().awaitcalls are correct. The#[serial]test annotation ensures these calls don't conflict. The prover startup includes health checks with appropriate timeouts.Consider using a test fixture or
once_cellto spawn the prover once for all tests rather than per-test, which could reduce total test execution time:use once_cell::sync::Lazy; static PROVER: Lazy<()> = Lazy::new(|| { tokio::runtime::Runtime::new() .unwrap() .block_on(spawn_prover()); }); #[tokio::test] async fn test_name() { Lazy::force(&PROVER); // ... test logic }Also applies to: 888-888, 1583-1583, 1983-1983
forester/tests/e2e_test.rs (1)
244-256: Correct usage of the simplified validator config and prover API.The change from
prover_config: Nonetoenable_prover: falsealigns with the API simplification. The subsequent manualspawn_prover().awaitcall provides explicit control over prover startup timing, which is appropriate for test orchestration.Consider adding a comment explaining the intentional separation:
init(Some(LightValidatorConfig { enable_indexer: true, enable_prover: false, // Disabled to manually control prover startup below wait_time: 60, sbf_programs: vec![...], limit_ledger_size: None, })) .await; // Spawn prover manually after validator initialization spawn_prover().await;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
forester/tests/e2e_test.rs(2 hunks)forester/tests/legacy/batched_state_async_indexer_test.rs(2 hunks)program-tests/batched-merkle-tree-test/tests/merkle_tree.rs(5 hunks)program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
forester/tests/legacy/batched_state_async_indexer_test.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
forester/tests/e2e_test.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
program-tests/batched-merkle-tree-test/tests/merkle_tree.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
⏰ 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). (26)
- GitHub Check: integration-test-lightweight
- GitHub Check: lean-verification
- GitHub Check: integration-test-lightweight-lazy
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client batched-mer...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v1
- GitHub Check: stateless-js-v2
- GitHub Check: lint
- GitHub Check: cli-v2
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Forester e2e test
🔇 Additional comments (11)
forester/tests/legacy/batched_state_async_indexer_test.rs (1)
14-14: LGTM! Clean API simplification.The removal of
ProverConfigfrom imports and the simplifiedspawn_prover()call align well with the PR's goal of lazy key management. The new implementation handles configuration internally via feature flags, reducing test boilerplate while maintaining functionality.Also applies to: 87-87
program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs (9)
30-30: LGTM! Import correctly updated.The import statement has been properly simplified to reflect the API change where
ProverConfigis no longer part of the public interface.
48-50: LGTM! Prover startup correctly simplified.The call to
spawn_prover()has been properly updated to the new no-argument API. The#[serial]attribute ensures tests don't race during prover initialization.
347-349: LGTM! Consistent API usage.Correctly updated to use the simplified
spawn_prover()API without configuration arguments.
661-663: LGTM! Prover initialization consistent.The no-argument
spawn_prover()call is correctly implemented and consistent with the refactored API.
1261-1263: LGTM! API usage correct.Properly updated to the new
spawn_prover()signature without configuration parameters.
2021-2023: LGTM! Simplified prover startup.The
spawn_prover()call correctly uses the new API without requiring explicit configuration.
2319-2321: LGTM! API update applied correctly.Consistent with the other test functions, this correctly uses the no-argument
spawn_prover()API.
2838-2840: LGTM! Final call site correctly updated.All
spawn_prover()call sites in this file have been consistently updated to the new no-argument API, completing the refactoring.
30-30: Excellent refactoring: API simplified across all test functions.This file demonstrates the successful migration to the new
spawn_prover()API:
- Import simplified: Removed
ProverConfigfrom imports (line 30)- All call sites updated: 7 test functions now use
spawn_prover().awaitwithout configuration arguments- Consistent pattern: All tests use
#[serial]attribute to prevent race conditions during prover initialization- Improved ergonomics: Eliminates the need to construct and pass
ProverConfig::default()at every call siteThe changes align with the PR's goal of introducing lazy key management and centralizing prover configuration internally.
Also applies to: 50-50, 349-349, 663-663, 1263-1263, 2023-2023, 2321-2321, 2840-2840
program-tests/batched-merkle-tree-test/tests/merkle_tree.rs (1)
40-40: LGTM!The simplified import aligns with the no-argument
spawn_prover()API and removes the unusedProverConfig.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
prover/server/integration_test.go (1)
96-132: TestMain gating still has edge cases.The past review concern persists: patterns like
go test -test.run=.*orgo test -test.run=Test.*won't match the string-contains checks for "TestFull", "TestLightweight", or "TestLightweightLazy", leavingrunIntegrationTests=false. Tests then run without a server, causing failures.As suggested in the previous review, simplify by defaulting to server startup whenever tests in this integration-test file run:
Apply this diff:
- runIntegrationTests := false + runIntegrationTests := true isLightweightMode = true - preloadKeys := true + preloadKeys := true for _, arg := range os.Args { if strings.Contains(arg, "-test.run=TestFull") { isLightweightMode = false - runIntegrationTests = true break } if strings.Contains(arg, "-test.run=TestLightweightLazy") { - runIntegrationTests = true preloadKeys = false break } if strings.Contains(arg, "-test.run=TestLightweight") { - runIntegrationTests = true break } } - if !runIntegrationTests { - hasTestRunFlag := false - for _, arg := range os.Args { - if strings.HasPrefix(arg, "-test.run=") { - hasTestRunFlag = true - pattern := strings.TrimPrefix(arg, "-test.run=") - if pattern == "" || pattern == "^Test" || strings.Contains(pattern, "Lightweight") || strings.Contains(pattern, "Full") { - runIntegrationTests = true - } - break - } - } - if !hasTestRunFlag { - runIntegrationTests = true - } - } - - if runIntegrationTests { - if isLightweightMode { - if preloadKeys { - logging.Logger().Info().Msg("Running in lightweight mode - preloading keys") - } else { - logging.Logger().Info().Msg("Running in lazy lightweight mode") - } + if isLightweightMode { + if preloadKeys { + logging.Logger().Info().Msg("Running in lightweight mode - preloading keys") } else { - logging.Logger().Info().Msg("Running in full mode - preloading keys") + logging.Logger().Info().Msg("Running in lazy lightweight mode") } - - StartServerWithPreload(isLightweightMode, preloadKeys) - code := m.Run() - StopServer() - os.Exit(code) } else { - logging.Logger().Info().Msg("Skipping key loading - no integration tests in this run") - os.Exit(m.Run()) + logging.Logger().Info().Msg("Running in full mode - preloading keys") } + + StartServerWithPreload(isLightweightMode, preloadKeys) + code := m.Run() + StopServer() + os.Exit(code) }
🧹 Nitpick comments (2)
prover/server/integration_test.go (2)
46-47: Redundant AutoDownload assignment.
DefaultDownloadConfig()already setsAutoDownload = true(as shown in the provided snippets), making line 47 redundant.Apply this diff to remove the redundant assignment:
downloadConfig := common.DefaultDownloadConfig() - downloadConfig.AutoDownload = true
54-57: Remove unnecessary return after Fatal.
Fatal()terminates execution, so thereturnon line 56 is unreachable and unnecessary.Apply this diff:
err := keyManager.PreloadForRunMode(runMode) if err != nil { logging.Logger().Fatal().Err(err).Msg("Failed to preload proving keys") - return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
prover/server/integration_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
prover/server/integration_test.go (4)
prover/server/prover/common/proving_keys_utils.go (3)
RunMode(16-16)FullTest(23-23)Full(22-22)prover/server/prover/common/key_downloader.go (1)
DefaultDownloadConfig(32-40)prover/server/prover/common/lazy_key_manager.go (1)
NewLazyKeyManager(19-30)prover/server/server/server.go (2)
Config(623-626)Run(582-584)
⏰ 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). (30)
- GitHub Check: test-with-redis (Failed job status tests, go test -v -run TestFailedJobStatus -timeout 5m)
- GitHub Check: lean-verification
- GitHub Check: integration-test-lightweight-lazy
- GitHub Check: test-with-redis (Queue processing flow tests, go test -v -run TestJobProcessingFlow -timeout 5m)
- GitHub Check: test-with-redis (Redis Queue tests, go test -v -run TestRedis -timeout 10m)
- GitHub Check: test-with-redis (Queue cleanup tests, go test -v -run TestCleanup -timeout 5m)
- GitHub Check: integration-test-lightweight
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Forester e2e test
- GitHub Check: lint
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client batched-mer...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: cli-v2
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v1
- GitHub Check: stateless-js-v2
🔇 Additional comments (3)
prover/server/integration_test.go (3)
27-27: LGTM: Server stop tracking.The
serverStoppedflag prevents double-stop issues whenStopServeris called multiple times.
33-35: LGTM: Clean delegation pattern.The refactor maintains backward compatibility while enabling the new preload functionality.
84-91: LGTM: Double-stop protection.The guard prevents issues if
StopServeris called multiple times, which is good defensive programming.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
prover/server/integration_test.go (2)
47-48: Remove redundant AutoDownload assignment.
DefaultDownloadConfig()already setsAutoDownload: true, making line 48 unnecessary.Apply this diff:
downloadConfig := common.DefaultDownloadConfig() - downloadConfig.AutoDownload = true
105-141: The detection logic is more robust but still complex.The enhanced logic correctly handles various test invocation patterns and addresses the concern from the previous review about detecting test modes. However, the two-phase detection (lines 109-124, then 126-141) is intricate.
For this specific file where all tests are
TestLightweight*orTestFull, a simpler approach would be:runIntegrationTests := true // default to running integration tests isLightweightMode := true preloadKeys := true for _, arg := range os.Args { if strings.Contains(arg, "-test.run=TestFull") { isLightweightMode = false break } if strings.Contains(arg, "-test.run=TestLightweightLazy") { preloadKeys = false break } // TestLightweight is the default, no need to check }This would start the server for any test run in this package, which is appropriate since all tests need it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
prover/server/integration_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
prover/server/integration_test.go (4)
prover/server/prover/common/proving_keys_utils.go (3)
RunMode(16-16)FullTest(23-23)Full(22-22)prover/server/prover/common/key_downloader.go (1)
DefaultDownloadConfig(32-40)prover/server/prover/common/lazy_key_manager.go (1)
NewLazyKeyManager(19-30)prover/server/server/server.go (2)
Config(623-626)Run(582-584)
⏰ 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). (30)
- GitHub Check: integration-test-lightweight-lazy
- GitHub Check: test-with-redis (Queue cleanup tests, go test -v -run TestCleanup -timeout 5m)
- GitHub Check: test-with-redis (Queue processing flow tests, go test -v -run TestJobProcessingFlow -timeout 5m)
- GitHub Check: integration-test-lightweight
- GitHub Check: test-with-redis (Failed job status tests, go test -v -run TestFailedJobStatus -timeout 5m)
- GitHub Check: test-with-redis (Redis Queue tests, go test -v -run TestRedis -timeout 10m)
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: lean-verification
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: cli-v1
- GitHub Check: cli-v2
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client batched-mer...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: stateless-js-v2
- GitHub Check: stateless-js-v1
- GitHub Check: lint
- GitHub Check: Forester e2e test
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
🔇 Additional comments (6)
prover/server/integration_test.go (6)
21-22: LGTM! Package-level preloadKeys enables proper test isolation.This addresses the previous review comment by promoting
preloadKeysto package-level scope, allowingTestLightweightLazyto check the value and skip appropriately.
28-28: LGTM! Idempotent StopServer prevents double-stop issues.The
serverStoppedflag correctly guards against callinginstance.RequestStop()andinstance.AwaitStop()multiple times, which could cause panics or undefined behavior.Also applies to: 85-85, 94-100
34-36: LGTM! Clean refactor maintains backward compatibility.Delegating to
StartServerWithPreload(isLightweight, true)preserves the original behavior while enabling the new lazy-loading test mode.
143-152: LGTM! Clear logging helps understand test execution mode.The conditional logging messages clearly indicate whether tests are running in lightweight/full mode and whether keys are preloaded or lazy-loaded.
154-161: LGTM! Proper test lifecycle management.The server is correctly started before running tests and stopped afterward, with proper exit code propagation. The else branch appropriately skips server setup when no integration tests are detected.
172-183: LGTM! Skip logic correctly implemented.This fully addresses the previous review comment by adding the skip check at lines 173-175. The test now only runs when
preloadKeys=false, ensuring it tests lazy key loading in isolation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
prover/client/src/proof_client.rs (1)
24-24: Document the rationale for the 15-minute timeout.The increase from 2 to 15 minutes is substantial and appropriate given the lazy key downloading introduced in this PR. However, consider adding a comment explaining that this timeout accommodates both on-demand key downloads (which can be large) and proof generation time, especially for first-time usage.
Apply this diff to add a clarifying comment:
+// Timeout for proof generation including lazy key downloads on first use. +// Circuit keys can be hundreds of MB; 15 minutes accommodates download + proving time. const DEFAULT_MAX_WAIT_TIME_SECS: u64 = 900;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
prover/client/src/proof_client.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: integration-test-lightweight-lazy
- GitHub Check: integration-test-lightweight
- GitHub Check: lean-verification
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client batched-mer...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: Forester e2e test
- GitHub Check: lint
- GitHub Check: stateless-js-v2
- GitHub Check: cli-v1
- GitHub Check: cli-v2
- GitHub Check: stateless-js-v1
- GitHub Check: system-programs (random-e2e-test, ["cargo-test-sbf -p e2e-test"])
- GitHub Check: system-programs (system-cpi-test-v2-other, ["cargo-test-sbf -p system-cpi-v2-test -- --skip funct...
- GitHub Check: system-programs (light-compressed-token, ["cargo-test-sbf -p compressed-token-test"])
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (system-cpi-test-v2-functional, ["cargo-test-sbf -p system-cpi-v2-test -- functio...
- GitHub Check: Test program-libs-slow
- GitHub Check: system-programs (system-cpi-test-v2-event, ["cargo-test-sbf -p system-cpi-v2-test -- event::parse"])
- GitHub Check: system-programs (account-compression, ["cargo-test-sbf -p account-compression-test"])
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test concurrent-merkle-tree
- GitHub Check: system-programs (light-registry, ["cargo-test-sbf -p registry-test"])
- GitHub Check: system-programs (light-system-program, ["cargo-test-sbf -p system-test"])
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cli/src/utils/downloadProverBinary.ts (2)
131-134: Consider making timeout configurable.The 60-second timeout is hardcoded, which may be insufficient for users with slow internet connections downloading large binaries. Since retry logic is already implemented at the parent level, consider making the timeout configurable or increasing it.
Apply this diff to make timeout configurable through DownloadOptions:
interface DownloadOptions { maxRetries?: number; retryDelay?: number; + timeoutMs?: number; } export async function downloadProverBinary( binaryPath: string, binaryName: string, options: DownloadOptions = {}, ): Promise<void> { - const { maxRetries = 3, retryDelay = 2000 } = options; + const { maxRetries = 3, retryDelay = 2000, timeoutMs = 120000 } = options; // ... for (let attempt = 1; attempt <= maxRetries; attempt++) { try { - await downloadFile(url, binaryPath); + await downloadFile(url, binaryPath, 0, timeoutMs);Then update the
downloadFilesignature:async function downloadFile( url: string, outputPath: string, redirectDepth: number = 0, + timeoutMs: number = 120000, ): Promise<void> { // ... - request.setTimeout(60000, () => { + request.setTimeout(timeoutMs, () => { request.destroy(); reject(new Error("Download timeout")); });
62-136: Consider reusing existing download utility.There's a similar
downloadFilefunction incli/src/psp-utils/download.tsthat uses axios and includes features like better progress bars with cli-progress. While your implementation is simpler and more focused, you might want to evaluate whether reusing the existing utility (possibly with modifications) would reduce maintenance burden.If the existing implementation's dependencies (axios, cli-progress, tar.gz handling) are acceptable, you could refactor to use it:
import { downloadFile } from "../psp-utils/download"; export async function downloadProverBinary( binaryPath: string, binaryName: string, options: DownloadOptions = {}, ): Promise<void> { // ... existing setup code ... for (let attempt = 1; attempt <= maxRetries; attempt++) { try { await downloadFile({ localFilePath: binaryPath, dirPath: path.dirname(binaryPath), url, }); // ... rest of logic ... } } }However, if you intentionally avoided axios or wanted a lighter-weight implementation, the current approach is acceptable.
prover/server/main.go (1)
656-678: Consider documenting preloading impact on startup time.The preloading logic executes synchronously before the server starts accepting requests. If many keys are preloaded (especially with
--preload-keys=all), startup latency could be significant. This fail-fast behavior is likely intentional, but consider documenting the tradeoff between startup time and runtime latency in operational runbooks or help text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli/src/utils/downloadProverBinary.ts(1 hunks)prover/client/src/proof_client.rs(1 hunks)prover/server/main.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prover/client/src/proof_client.rs
🧰 Additional context used
🧬 Code graph analysis (2)
prover/server/main.go (4)
prover/server/prover/common/key_downloader.go (6)
DefaultBaseURL(18-18)DefaultMaxRetries(19-19)DownloadConfig(24-30)DefaultRetryDelay(20-20)DefaultMaxRetryDelay(21-21)EnsureKeysExist(372-413)prover/server/prover/common/lazy_key_manager.go (1)
NewLazyKeyManager(19-30)prover/server/server/queue_job.go (3)
NewUpdateQueueWorker(51-61)NewAppendQueueWorker(63-73)NewAddressAppendQueueWorker(75-85)prover/server/server/server.go (2)
RunWithQueue(459-467)Run(582-584)
cli/src/utils/downloadProverBinary.ts (1)
cli/src/psp-utils/download.ts (1)
downloadFile(142-211)
🔇 Additional comments (3)
cli/src/utils/downloadProverBinary.ts (1)
138-140: LGTM!Simple and effective version getter function.
prover/server/main.go (2)
476-562: LGTM! Well-structured download command.The new
downloadsubcommand provides a clean interface for provisioning and verifying proving keys. The flag defaults are sensible (local-rpc mode, auto-download enabled), and the verify-only mode correctly inverts the AutoDownload setting. Error handling and logging are comprehensive.
634-683: LazyKeyManager is thread-safe
All accesses tomerkleSystems,batchSystems, andloadingInProgressare protected bymu.RLock/RUnlockfor reads andmu.Lock/Unlockfor writes, ensuring proper synchronization.
…ry management in CLI
sergeytimoshin
force-pushed
the
sergey/prover-lazy-keys
branch
from
c3c70f7 to
4adc115
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
prover/client/src/prover.rs (3)
17-31: Fix lifetime and dev/prod path resolutionReturning &str from format! creates a dangling reference. Also, requiring project_root in production is unnecessary. Use String (or Cow) and gate project_root only for devenv.
Apply this diff:
pub async fn spawn_prover() { - if let Some(_project_root) = get_project_root() { - let prover_path: &str = { - #[cfg(feature = "devenv")] - { - &format!("{}/{}", _project_root.trim(), "cli/test_bin/run") - } - #[cfg(not(feature = "devenv"))] - { - println!("Running in production mode, using prover binary"); - "light" - } - }; + #[cfg(feature = "devenv")] + let prover_path = { + let root = get_project_root().expect("Failed to find project root."); + format!("{}/cli/test_bin/run", root.trim()) + }; + #[cfg(not(feature = "devenv"))] + let prover_path = String::from("light"); + tracing::info!("Starting prover using binary: {}", prover_path);
31-47: Don’t block on child; fix atomic and reset flag
- Remove wait_with_output (blocks until the server exits).
- Use compare_exchange to set IS_LOADING atomically.
- Reset IS_LOADING after startup attempt.
Apply this diff:
- if !health_check(10, 1).await && !IS_LOADING.load(Ordering::Relaxed) { - IS_LOADING.store(true, Ordering::Relaxed); - - let command = Command::new(prover_path) - .arg("start-prover") - .spawn() - .expect("Failed to start prover process"); - - let _ = command.wait_with_output(); - - let health_result = health_check(120, 1).await; - if health_result { - info!("Prover started successfully"); - } else { - panic!("Failed to start prover, health check failed."); - } - } - } else { - panic!("Failed to find project root."); - }; + if !health_check(10, 1).await { + if IS_LOADING + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_ok() + { + match Command::new(&prover_path).arg("start-prover").spawn() { + Ok(_child) => { + let ok = health_check(120, 1).await; + IS_LOADING.store(false, Ordering::Release); + if ok { + info!("Prover started successfully"); + } else { + panic!("Failed to start prover, health check failed."); + } + } + Err(e) => { + IS_LOADING.store(false, Ordering::Release); + panic!("Failed to start prover process: {e}"); + } + } + } + }
1-6: Use async sleep and set HTTP timeoutsAvoid blocking std::thread::sleep in async code; set a small HTTP timeout to prevent hanging.
Apply this diff:
use std::{ process::Command, sync::atomic::{AtomicBool, Ordering}, - thread::sleep, time::Duration, }; +use tokio::time::sleep; @@ -pub async fn health_check(retries: usize, timeout: usize) -> bool { - let client = reqwest::Client::new(); +pub async fn health_check(retries: usize, timeout: usize) -> bool { + let client = reqwest::Client::builder() + .timeout(Duration::from_secs(2)) + .build() + .expect("Failed to build HTTP client"); @@ - Err(_) => { - sleep(Duration::from_secs(timeout as u64)); - } + Err(_) => { + sleep(Duration::from_secs(timeout as u64)).await; + }Also applies to: 53-71
♻️ Duplicate comments (6)
cli/src/utils/processProverServer.ts (2)
135-159: Windows ARM64 binary unavailable in releases.The platform/arch mapping includes Windows ARM64, but as noted in the previous review, the release assets don't include
prover-windows-arm64.exe. Users on Windows ARM64 will get a 404 error during download.Either publish the Windows ARM64 asset in releases or add a fallback/error message for unsupported platforms.
Based on past review comments.
90-111: Cached binaries may fail with new flags.The function skips download when a binary exists but doesn't verify that the cached binary supports the
--auto-downloadflag now passed instartProver. A developer with an older binary (pre-dating this PR) will hit an immediate startup failure with an "unknown flag" error.This is the same issue flagged in the previous review. Please implement one of the suggested solutions:
- Embed the prover release tag in the cached filename (e.g.,
prover-${platform}-${arch}-${version}) and force re-download when versions differ- Add a version/capability check by running the binary with
--versionor--helpto detect flag support before reuseBased on past review comments.
.github/workflows/prover-test.yml (2)
70-72: Incorrect package path in unit test command.The test command uses
go test ./prover/...from working directoryprover/server, which fails because there's noprover/subdirectory underprover/server. The entire matrix entry errors out with "pattern ./prover/...: no matching packages".As flagged in the previous review, use
./...to run tests against all packages in the module root:- command: "go test ./prover/... -timeout 60m" + command: "go test ./... -timeout 60m"Based on past review comments.
221-224: Full integration tests skip direct pushes to release branches.The workflow triggers on
pushtorelease/**branches (line 8), but this job'sifcondition restricts execution to pull requests only. Once a PR merges or a release branch is updated directly, the full test suite is skipped entirely.As noted in the previous review, either remove the PR-only guard or add a push condition:
if: | - github.event_name == 'pull_request' && + (github.event_name == 'pull_request' && github.event.pull_request.draft == false && - startsWith(github.event.pull_request.base.ref, 'release') + startsWith(github.event.pull_request.base.ref, 'release')) || + (github.event_name == 'push' && + startsWith(github.ref, 'refs/heads/release'))Based on past review comments.
cli/src/utils/downloadProverBinary.ts (2)
15-60: Missing integrity verification for downloaded binary.The function downloads the prover binary but doesn't verify its integrity or authenticity using a checksum or cryptographic signature. If the download is intercepted (MITM), corrupted during transfer, or the GitHub release is compromised, malicious code could be executed.
As suggested in the previous review, implement one of these approaches:
- Option 1 (Recommended): Store expected SHA256 checksums and verify after download:
+const PROVER_CHECKSUMS: Record<string, string> = { + "prover-linux-amd64": "abc123...", + "prover-darwin-amd64": "def456...", + // ... other binaries +}; + +async function verifyChecksum(filePath: string, expectedChecksum: string): Promise<boolean> { + const crypto = require("crypto"); + const fileBuffer = fs.readFileSync(filePath); + const hash = crypto.createHash("sha256").update(fileBuffer).digest("hex"); + return hash === expectedChecksum; +} + for (let attempt = 1; attempt <= maxRetries; attempt++) { try { await downloadFile(url, binaryPath); + const expectedChecksum = PROVER_CHECKSUMS[binaryName]; + if (expectedChecksum && !(await verifyChecksum(binaryPath, expectedChecksum))) { + fs.unlinkSync(binaryPath); + throw new Error("Checksum verification failed"); + } + if (process.platform !== "win32") {
- Option 2: Download and verify a separate
.sha256file from the same release.Based on past review comments.
62-78: Unbounded redirect recursion allows infinite loops.The redirect handling recursively calls
downloadFilewithout limiting the depth. A malicious server returning an infinite redirect chain could cause a stack overflow, or legitimate circular redirects could cause excessive delays.As suggested in the previous review, add a depth limit:
async function downloadFile( url: string, outputPath: string, + redirectDepth: number = 0, ): Promise<void> { + const MAX_REDIRECTS = 10; + + if (redirectDepth > MAX_REDIRECTS) { + throw new Error(`Too many redirects (max ${MAX_REDIRECTS})`); + } + return new Promise((resolve, reject) => { const protocol = url.startsWith("https") ? https : http; const request = protocol.get(url, (response) => { if ( response.statusCode === 301 || response.statusCode === 302 || response.statusCode === 307 || response.statusCode === 308 ) { const redirectUrl = response.headers.location; if (!redirectUrl) { return reject(new Error("Redirect without location header")); } - return downloadFile(redirectUrl, outputPath).then(resolve, reject); + return downloadFile(redirectUrl, outputPath, redirectDepth + 1).then(resolve, reject); }Based on past review comments.
🧹 Nitpick comments (5)
prover/server/prover/common/proving_keys_utils.go (1)
287-290: Defer checksum download until keys are actually missingTo avoid unnecessary network I/O when all keys already exist, adjust EnsureKeysExist to check for missing files first, and only then fetch checksums and download.
Consider this change in prover/server/prover/common/key_downloader.go:
--- a/prover/server/prover/common/key_downloader.go +++ b/prover/server/prover/common/key_downloader.go @@ func EnsureKeysExist(keys []string, config *DownloadConfig) error { if !config.AutoDownload { for _, key := range keys { if _, err := os.Stat(key); os.IsNotExist(err) { return fmt.Errorf("required key file not found: %s (auto-download disabled)", key) } } return nil } - - if err := downloadChecksum(config); err != nil { - return fmt.Errorf("failed to download checksums: %w", err) - } - - var missingKeys []string - for _, key := range keys { - if _, err := os.Stat(key); os.IsNotExist(err) { - missingKeys = append(missingKeys, key) - } - } + var missingKeys []string + for _, key := range keys { + if _, err := os.Stat(key); os.IsNotExist(err) { + missingKeys = append(missingKeys, key) + } + } if len(missingKeys) > 0 { + if err := downloadChecksum(config); err != nil { + return fmt.Errorf("failed to download checksums: %w", err) + } logging.Logger().Info(). Int("missing_count", len(missingKeys)). Int("total_count", len(keys)). Msg("Found missing key files, will download") @@ } return nil }cli/package.json (1)
21-23: Tighten exclude globs to ensure recursive omissionUsing recursive globs avoids accidental inclusion of nested files.
- "!/bin/proving-keys", - "!/bin/prover-*", + "!/bin/proving-keys/**", + "!/bin/prover-*/**",program-tests/compressed-token-test/tests/test.rs (2)
597-597: Avoid duplicative prover startups across testsTo reduce repetition and flakiness, prefer enabling prover via ProgramTestConfig (with_prover: true) or a single module-level initializer instead of calling spawn_prover() in many tests. LightProgramTest::new already conditionally spawns the prover.
Also applies to: 1253-1253, 1416-1416, 1560-1560, 1636-1636, 2824-2824, 3102-3102, 3466-3466, 3655-3655, 3919-3919, 4213-4213
1332-1336: Track the TODO with an issueRe-exporting these types from light-program-test would simplify tests. Want me to open an issue or submit a PR for the re-export?
prover/server/prover/common/lazy_key_manager.go (1)
63-77: Key string uses CircuitType directlyBe explicit to avoid formatting surprises if CircuitType changes type.
Apply this diff:
- key := fmt.Sprintf("%s_%d_%d", circuitType, treeHeight, batchSize) + key := fmt.Sprintf("%s_%d_%d", string(circuitType), treeHeight, batchSize)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.github/workflows/prover-release.yml(2 hunks).github/workflows/prover-test.yml(3 hunks)cli/package.json(2 hunks)cli/scripts/buildProver.sh(0 hunks)cli/src/commands/start-prover/index.ts(1 hunks)cli/src/utils/downloadProverBinary.ts(1 hunks)cli/src/utils/initTestEnv.ts(1 hunks)cli/src/utils/processProverServer.ts(4 hunks)forester/tests/e2e_test.rs(2 hunks)forester/tests/legacy/batched_state_async_indexer_test.rs(2 hunks)forester/tests/test_batch_append_spent.rs(1 hunks)program-tests/batched-merkle-tree-test/tests/merkle_tree.rs(5 hunks)program-tests/compressed-token-test/tests/test.rs(15 hunks)program-tests/system-cpi-v2-test/tests/event.rs(1 hunks)program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs(8 hunks)program-tests/utils/src/e2e_test_env.rs(0 hunks)prover/client/src/proof_client.rs(1 hunks)prover/client/src/prover.rs(2 hunks)prover/client/tests/batch_address_append.rs(2 hunks)prover/client/tests/batch_append.rs(2 hunks)prover/client/tests/batch_update.rs(2 hunks)prover/client/tests/combined.rs(2 hunks)prover/client/tests/inclusion.rs(2 hunks)prover/client/tests/non_inclusion.rs(2 hunks)prover/server/README.md(0 hunks)prover/server/integration_test.go(3 hunks)prover/server/main.go(6 hunks)prover/server/prover/common/key_downloader.go(1 hunks)prover/server/prover/common/lazy_key_manager.go(1 hunks)prover/server/prover/common/proving_keys_utils.go(1 hunks)prover/server/redis_queue_test.go(3 hunks)prover/server/scripts/download_keys.sh(0 hunks)prover/server/server/queue_job.go(8 hunks)prover/server/server/server.go(10 hunks)sdk-libs/client/src/lib.rs(1 hunks)sdk-libs/client/src/local_test_validator.rs(3 hunks)sdk-libs/program-test/src/program_test/config.rs(0 hunks)sdk-libs/program-test/src/program_test/light_program_test.rs(2 hunks)
💤 Files with no reviewable changes (5)
- sdk-libs/program-test/src/program_test/config.rs
- program-tests/utils/src/e2e_test_env.rs
- prover/server/scripts/download_keys.sh
- prover/server/README.md
- cli/scripts/buildProver.sh
🚧 Files skipped from review as they are similar to previous changes (15)
- prover/client/tests/combined.rs
- prover/client/src/proof_client.rs
- prover/server/prover/common/key_downloader.go
- .github/workflows/prover-release.yml
- program-tests/batched-merkle-tree-test/tests/merkle_tree.rs
- forester/tests/test_batch_append_spent.rs
- program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs
- prover/server/integration_test.go
- sdk-libs/client/src/local_test_validator.rs
- prover/server/server/queue_job.go
- prover/client/tests/batch_update.rs
- forester/tests/legacy/batched_state_async_indexer_test.rs
- prover/server/redis_queue_test.go
- prover/client/tests/batch_append.rs
- cli/src/utils/initTestEnv.ts
🧰 Additional context used
🧬 Code graph analysis (14)
prover/server/prover/common/proving_keys_utils.go (1)
prover/server/prover/common/key_downloader.go (3)
DefaultDownloadConfig(32-40)DownloadConfig(24-30)EnsureKeysExist(372-413)
prover/client/tests/batch_address_append.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
cli/src/utils/downloadProverBinary.ts (1)
cli/src/psp-utils/download.ts (1)
downloadFile(142-211)
prover/client/tests/non_inclusion.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
forester/tests/e2e_test.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
sdk-libs/program-test/src/program_test/light_program_test.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
prover/client/tests/inclusion.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
prover/server/prover/common/lazy_key_manager.go (2)
prover/server/prover/common/key_downloader.go (3)
DownloadConfig(24-30)DefaultDownloadConfig(32-40)DownloadKey(285-370)prover/server/prover/common/proving_keys_utils.go (4)
RunMode(16-16)GetKeys(112-276)Full(22-22)FullTest(23-23)
prover/client/src/prover.rs (2)
sdk-libs/program-test/src/program_test/light_program_test.rs (1)
new(55-332)sdk-libs/program-test/src/program_test/config.rs (1)
new(56-65)
cli/src/utils/processProverServer.ts (3)
cli/src/utils/downloadProverBinary.ts (1)
downloadProverBinary(15-60)cli/src/utils/process.ts (1)
killProcessByPort(74-82)cli/src/utils/constants.ts (1)
BASE_PATH(30-30)
cli/src/commands/start-prover/index.ts (1)
cli/src/utils/processProverServer.ts (1)
startProver(113-133)
prover/server/main.go (5)
prover/server/prover/common/key_downloader.go (6)
DefaultBaseURL(18-18)DefaultMaxRetries(19-19)DownloadConfig(24-30)DefaultRetryDelay(20-20)DefaultMaxRetryDelay(21-21)EnsureKeysExist(372-413)prover/server/prover/common/proving_keys_utils.go (2)
GetKeys(112-276)RunMode(16-16)prover/server/prover/common/lazy_key_manager.go (1)
NewLazyKeyManager(19-30)prover/server/server/queue_job.go (3)
NewUpdateQueueWorker(51-61)NewAppendQueueWorker(63-73)NewAddressAppendQueueWorker(75-85)prover/server/server/server.go (2)
RunWithQueue(459-467)Run(582-584)
prover/server/server/server.go (2)
prover/server/prover/common/lazy_key_manager.go (1)
LazyKeyManager(10-17)prover/server/server/queue.go (1)
RedisQueue(14-17)
program-tests/compressed-token-test/tests/test.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
⏰ 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). (28)
- GitHub Check: integration-test-lightweight
- GitHub Check: integration-test-lightweight-lazy
- GitHub Check: test-with-redis (Queue processing flow tests, go test -v -run TestJobProcessingFlow -timeout 5m)
- GitHub Check: test-with-redis (Redis Queue tests, go test -v -run TestRedis -timeout 10m)
- GitHub Check: lean-verification
- GitHub Check: test-with-redis (Failed job status tests, go test -v -run TestFailedJobStatus -timeout 5m)
- GitHub Check: test-with-redis (Queue cleanup tests, go test -v -run TestCleanup -timeout 5m)
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: Forester e2e test
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: cli-v2
- GitHub Check: stateless-js-v2
- GitHub Check: cli-v1
- GitHub Check: lint
- GitHub Check: stateless-js-v1
🔇 Additional comments (17)
program-tests/system-cpi-v2-test/tests/event.rs (1)
534-534: LGTM! Prover enablement aligns with lazy key loading.Enabling the prover in this test aligns with the PR's introduction of lazy proving-key downloads. Since the test is marked
#[ignore], it won't impact regular CI runs and will only execute when explicitly invoked for generating test data.forester/tests/e2e_test.rs (1)
244-256: LGTM! Clean migration to the new prover API.The changes correctly migrate to the simplified API:
LightValidatorConfignow usesenable_prover: falseinstead ofprover_config: Nonespawn_prover()is now called without arguments and properly awaitedThe pattern of disabling auto-start (
enable_prover: false) and then manually spawning the prover provides explicit control over initialization order for test setup, which is appropriate for this test scenario.prover/client/tests/non_inclusion.rs (1)
3-3: No‑arg spawn_prover adoption looks goodImport and callsite align with the new API; serialized test prevents startup races.
Also applies to: 13-13
prover/client/tests/batch_address_append.rs (1)
9-9: Consistent migration to no‑arg spawn_proverImport and call update match the refactored client API.
Also applies to: 26-26
prover/server/prover/common/proving_keys_utils.go (1)
279-280: Good API shimLoadKeys delegating to LoadKeysWithConfig(DefaultDownloadConfig) preserves the old surface while enabling config-driven downloads.
cli/src/commands/start-prover/index.ts (1)
33-33: API alignmentstartProver(proverPort, redisUrl) matches the simplified startup surface.
cli/package.json (1)
95-95: scripts/copyLocalProgramBinaries.sh is present Verified the add-bins script exists atscripts/copyLocalProgramBinaries.sh; build and build-release will succeed.prover/server/main.go (2)
476-562: LGTM!The new
downloadcommand is well-structured with clear flag definitions, proper configuration handling, and informative logging. The error handling is appropriate, and the flow throughGetKeys→EnsureKeysExistis clean and maintainable.
578-684: LGTM!The preload configuration options (
none,run-mode,all) provide good flexibility for different deployment scenarios. The error handling validates flag combinations appropriately (e.g., requiring--run-modewhen using--preload-keys=run-mode), and the logging provides clear visibility into the key manager's state.prover/server/server/server.go (2)
270-274: LGTM!The refactoring from explicit proving system slices to
LazyKeyManageris clean and consistent. The API changes are clearly intentional as part of the lazy loading feature, and all call sites have been updated accordingly. The handler initialization properly threads thekeyManagerthrough to the proof processing logic.Also applies to: 459-467, 469-488, 582-584
895-898: LGTM!The proof generation methods correctly use
keyManager.GetBatchSystemandkeyManager.GetMerkleSystemwith proper error handling. Each error is wrapped with contextual information (e.g., "batch update: %w", "inclusion proof: %w"), making debugging easier while preserving the original error chain.Also applies to: 918-921, 942-945, 956-965, 999-1008, 1051-1060
program-tests/compressed-token-test/tests/test.rs (2)
47-47: Import looks goodImporting spawn_prover directly aligns with the new API.
5487-5491: Enabling prover in validator config is consistentSetting enable_prover: true here is consistent with the lazy startup path.
sdk-libs/program-test/src/program_test/light_program_test.rs (2)
11-12: Import looks goodUsing spawn_prover from light_prover_client is consistent with the new API.
322-329: Prover startup flow LGTM
- devenv: always spawn (useful for local runs).
- non-devenv: respect config.with_prover flag.
This aligns with the simplified client API.
prover/server/prover/common/lazy_key_manager.go (1)
192-220: Concurrent load coordination LGTMSingle-flight via loadingInProgress chans prevents duplicate loads; release on defer avoids leaks.
prover/client/tests/inclusion.rs (1)
3-4: LGTMImporting and awaiting spawn_prover() before hitting the HTTP endpoint is correct with the new client API.
Also applies to: 13-14
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
.github/workflows/prover-test.yml (2)
70-77: Fix the Go package pattern in the unit-test matrix.
go test ./prover/...still points at a non-existent submodule (we run fromprover/server, there is no nestedprover/folder), so this matrix leg bombs out before running any tests. Switch the pattern to./...so the unit suite actually executes.Apply this diff:
- - name: "Unit tests" - command: "go test ./prover/... -timeout 60m" + - name: "Unit tests" + command: "go test ./... -timeout 60m"
222-224: Keep running full integration tests on release pushes.This job still short-circuits on direct pushes to
release/**because theifguard only admits pull-request events. We lose release-branch coverage the moment a release PR merges (or a commit lands directly). Reintroduce a push clause so release branches keep their full suite.One way to fix it:
- if: | - github.event_name == 'pull_request' && - github.event.pull_request.draft == false && - startsWith(github.event.pull_request.base.ref, 'release') + if: | + (github.event_name == 'pull_request' && + github.event.pull_request.draft == false && + startsWith(github.event.pull_request.base.ref, 'release')) || + (github.event_name == 'push' && + startsWith(github.ref, 'refs/heads/release/'))cli/src/utils/downloadProverBinary.ts (2)
15-60: Add checksum verification for downloaded binary.As previously noted, the function downloads the prover binary but does not verify its integrity or authenticity. This creates a security risk if the download is intercepted, corrupted, or the release is compromised.
66-78: Add redirect depth limit to prevent infinite recursion.As previously noted, the redirect handling recursively calls
downloadFilewithout limiting the depth, which could lead to stack overflow with infinite redirect chains or excessive delays from circular redirects.cli/src/utils/processProverServer.ts (1)
93-111: Refresh the local prover when flags change.As previously noted, the function only checks if the binary file exists (line 97) but doesn't verify it supports the new
--auto-downloadflag (line 124). Developers with older cached binaries will encounter immediate startup failures with "unknown flag" errors.Please version-gate the cache (embed the prover release tag in the filename or store a manifest) or add an explicit capability/version check before deciding to reuse the binary.
🧹 Nitpick comments (4)
cli/src/utils/downloadProverBinary.ts (2)
7-8: Centralize the prover version constant.The prover version is hardcoded here and may need updates when new prover releases are published. Consider moving
PROVER_VERSIONto a shared constants file (e.g.,cli/src/utils/constants.ts) to maintain a single source of truth and reduce the risk of version mismatches across the codebase.Apply this diff to relocate the constant:
In
cli/src/utils/constants.ts:+export const PROVER_VERSION = "1.0.2";In
cli/src/utils/downloadProverBinary.ts:+import { PROVER_VERSION } from "./constants"; + -const PROVER_VERSION = "1.0.2"; const GITHUB_RELEASES_BASE_URL = `https://github.com/Lightprotocol/light-protocol/releases/download/light-prover-v${PROVER_VERSION}`;
62-136: Consider consolidating download utilities.A similar
downloadFilefunction exists incli/src/psp-utils/download.ts(lines 141-210) with overlapping functionality (retry logic, progress reporting, error handling). While the feature sets differ (axios vs. native http, tar.gz extraction vs. none), maintaining two separate implementations increases the maintenance burden.Consider refactoring to share common download logic, or document why separate implementations are needed.
prover/server/prover/common/lazy_key_manager.go (1)
79-138: Consider propagating load errors to waiting goroutines.Lines 87-96: When a goroutine waits for another to complete loading and the load fails, it receives a generic error "loading completed but system not found in cache" rather than the actual failure cause. While the actual error is logged (line 112 or 117), waiting goroutines lack context.
This is a minor UX issue for concurrent loads. Consider storing the error in
loadingInProgressso waiting goroutines can access it:type LazyKeyManager struct { mu sync.RWMutex merkleSystems map[string]*MerkleProofSystem batchSystems map[string]*BatchProofSystem keysDir string downloadConfig *DownloadConfig - loadingInProgress map[string]chan struct{} + loadingInProgress map[string]chan error }Then have the loading goroutine send the error on the channel before closing it, and waiting goroutines can receive and return that error. This is entirely optional since the current approach is functional and logs errors appropriately.
prover/server/integration_test.go (1)
105-162: LGTM! TestMain now starts server for integration tests by default.The test detection logic (lines 105-141) has been improved to address previous concerns about test gating. The current approach:
- Detects specific test patterns (TestFull, TestLightweightLazy, TestLightweight)
- Defaults to running integration tests unless a specific non-integration test is targeted
- Configures preload mode based on the test pattern
This ensures the integration server starts for most test runs while allowing developers to run isolated unit tests without the overhead.
The test detection logic (lines 105-141) is somewhat intricate. Consider adding a brief comment at line 104 explaining the intent:
// Detect which integration test mode to run: // - TestFull: full mode with preload // - TestLightweightLazy: lightweight mode with lazy loading // - TestLightweight: lightweight mode with preload // - Default: run integration tests unless targeting a specific non-integration test
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.github/workflows/prover-release.yml(2 hunks).github/workflows/prover-test.yml(3 hunks)cli/package.json(2 hunks)cli/scripts/buildProver.sh(0 hunks)cli/src/commands/start-prover/index.ts(1 hunks)cli/src/utils/downloadProverBinary.ts(1 hunks)cli/src/utils/initTestEnv.ts(1 hunks)cli/src/utils/processProverServer.ts(4 hunks)forester/tests/e2e_test.rs(2 hunks)forester/tests/legacy/batched_state_async_indexer_test.rs(2 hunks)forester/tests/test_batch_append_spent.rs(1 hunks)program-tests/batched-merkle-tree-test/tests/merkle_tree.rs(5 hunks)program-tests/compressed-token-test/tests/test.rs(15 hunks)program-tests/system-cpi-v2-test/tests/event.rs(1 hunks)program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs(8 hunks)program-tests/utils/src/e2e_test_env.rs(0 hunks)prover/client/src/proof_client.rs(1 hunks)prover/client/src/prover.rs(2 hunks)prover/client/tests/batch_address_append.rs(2 hunks)prover/client/tests/batch_append.rs(2 hunks)prover/client/tests/batch_update.rs(2 hunks)prover/client/tests/combined.rs(2 hunks)prover/client/tests/inclusion.rs(2 hunks)prover/client/tests/non_inclusion.rs(2 hunks)prover/server/README.md(0 hunks)prover/server/integration_test.go(3 hunks)prover/server/main.go(6 hunks)prover/server/prover/common/key_downloader.go(1 hunks)prover/server/prover/common/lazy_key_manager.go(1 hunks)prover/server/prover/common/proving_keys_utils.go(1 hunks)prover/server/redis_queue_test.go(3 hunks)prover/server/scripts/download_keys.sh(3 hunks)prover/server/server/queue_job.go(8 hunks)prover/server/server/server.go(10 hunks)sdk-libs/client/src/lib.rs(1 hunks)sdk-libs/client/src/local_test_validator.rs(3 hunks)sdk-libs/program-test/src/program_test/config.rs(0 hunks)sdk-libs/program-test/src/program_test/light_program_test.rs(2 hunks)
💤 Files with no reviewable changes (4)
- sdk-libs/program-test/src/program_test/config.rs
- program-tests/utils/src/e2e_test_env.rs
- cli/scripts/buildProver.sh
- prover/server/README.md
🚧 Files skipped from review as they are similar to previous changes (13)
- cli/src/utils/initTestEnv.ts
- forester/tests/legacy/batched_state_async_indexer_test.rs
- sdk-libs/client/src/local_test_validator.rs
- forester/tests/test_batch_append_spent.rs
- cli/package.json
- program-tests/system-cpi-v2-test/tests/event.rs
- prover/server/prover/common/key_downloader.go
- cli/src/commands/start-prover/index.ts
- prover/client/tests/batch_append.rs
- prover/client/tests/batch_update.rs
- sdk-libs/client/src/lib.rs
- prover/client/tests/inclusion.rs
- program-tests/batched-merkle-tree-test/tests/merkle_tree.rs
🧰 Additional context used
🧬 Code graph analysis (17)
prover/client/tests/non_inclusion.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
prover/server/redis_queue_test.go (4)
prover/server/prover/common/lazy_key_manager.go (1)
NewLazyKeyManager(19-30)prover/server/prover/common/key_downloader.go (1)
DefaultDownloadConfig(32-40)prover/server/server/queue_job.go (3)
NewUpdateQueueWorker(51-61)NewAppendQueueWorker(63-73)NewAddressAppendQueueWorker(75-85)prover/server/server/server.go (1)
RunEnhanced(469-580)
prover/server/prover/common/lazy_key_manager.go (2)
prover/server/prover/common/key_downloader.go (3)
DownloadConfig(24-30)DefaultDownloadConfig(32-40)DownloadKey(285-370)prover/server/prover/common/proving_keys_utils.go (4)
RunMode(16-16)GetKeys(112-276)Full(22-22)FullTest(23-23)
prover/client/tests/combined.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
forester/tests/e2e_test.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
cli/src/utils/processProverServer.ts (3)
cli/src/utils/downloadProverBinary.ts (1)
downloadProverBinary(15-60)cli/src/utils/process.ts (1)
killProcessByPort(74-82)cli/src/utils/constants.ts (1)
BASE_PATH(30-30)
prover/client/src/prover.rs (2)
sdk-libs/program-test/src/program_test/light_program_test.rs (1)
new(55-332)sdk-libs/program-test/src/program_test/config.rs (1)
new(56-65)
prover/server/integration_test.go (4)
prover/server/prover/common/proving_keys_utils.go (3)
RunMode(16-16)FullTest(23-23)Full(22-22)prover/server/prover/common/key_downloader.go (1)
DefaultDownloadConfig(32-40)prover/server/prover/common/lazy_key_manager.go (1)
NewLazyKeyManager(19-30)prover/server/server/server.go (2)
Config(623-626)Run(582-584)
program-tests/compressed-token-test/tests/test.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
sdk-libs/program-test/src/program_test/light_program_test.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
prover/server/main.go (5)
prover/server/prover/common/key_downloader.go (6)
DefaultBaseURL(18-18)DefaultMaxRetries(19-19)DownloadConfig(24-30)DefaultRetryDelay(20-20)DefaultMaxRetryDelay(21-21)EnsureKeysExist(372-413)prover/server/prover/common/proving_keys_utils.go (2)
GetKeys(112-276)RunMode(16-16)prover/server/prover/common/lazy_key_manager.go (1)
NewLazyKeyManager(19-30)prover/server/server/queue_job.go (3)
NewUpdateQueueWorker(51-61)NewAppendQueueWorker(63-73)NewAddressAppendQueueWorker(75-85)prover/server/server/server.go (2)
RunWithQueue(459-467)Run(582-584)
prover/server/server/queue_job.go (2)
prover/server/prover/common/lazy_key_manager.go (1)
LazyKeyManager(10-17)prover/server/server/queue.go (1)
RedisQueue(14-17)
prover/server/server/server.go (2)
prover/server/prover/common/lazy_key_manager.go (1)
LazyKeyManager(10-17)prover/server/server/queue.go (1)
RedisQueue(14-17)
prover/client/tests/batch_address_append.rs (1)
prover/client/src/prover.rs (1)
spawn_prover(17-51)
cli/src/utils/downloadProverBinary.ts (1)
cli/src/psp-utils/download.ts (1)
downloadFile(142-211)
prover/server/prover/common/proving_keys_utils.go (1)
prover/server/prover/common/key_downloader.go (3)
DefaultDownloadConfig(32-40)DownloadConfig(24-30)EnsureKeysExist(372-413)
🪛 Shellcheck (0.11.0)
prover/server/scripts/download_keys.sh
[warning] 73-73: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 89-89: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 90-90: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 262-262: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 263-263: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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). (26)
- GitHub Check: test-with-redis (Redis Queue tests, go test -v -run TestRedis -timeout 10m)
- GitHub Check: test-with-redis (Queue processing flow tests, go test -v -run TestJobProcessingFlow -timeout 5m)
- GitHub Check: test-with-redis (Queue cleanup tests, go test -v -run TestCleanup -timeout 5m)
- GitHub Check: integration-test-lightweight-lazy
- GitHub Check: test-with-redis (Failed job status tests, go test -v -run TestFailedJobStatus -timeout 5m)
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: Forester e2e test
- GitHub Check: cli-v2
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v1
- GitHub Check: stateless-js-v2
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: lint
🔇 Additional comments (44)
.github/workflows/prover-release.yml (1)
22-34: Build matrix extension looks solid.The expanded loop cleanly targets darwin/linux (amd64, arm64) plus the Windows exe variant, and the extension handling keeps the artifacts aligned with the release list. All good here.
prover/client/src/proof_client.rs (1)
24-24: Confirm default timeout change
Search shows no remaining hardcoded 120s references; all defaults updated to 600s. Validate that 600-second timeout covers expected proof generation and lazy key download durations.cli/src/utils/downloadProverBinary.ts (2)
80-136: Well-implemented download handling.The HTTP status validation, progress reporting, streaming with
pipeline, error cleanup, and timeout configuration are all well done. Once the checksum verification and redirect depth limit are addressed, this will be a solid implementation.
138-140: LGTM!Exposing the prover version through a public API is good practice and enables version checks from calling code.
cli/src/utils/processProverServer.ts (2)
113-133: Simplified startup flow looks good.The signature simplification and removal of
runMode/circuitsparameters aligns well with the lazy key management approach described in the PR objectives. Delegating configuration to the prover itself via--auto-downloadreduces CLI complexity.Assuming the version-gating issue in
ensureProverBinaryis addressed, this implementation is solid.
135-159: LGTM!The platform and architecture normalization logic is correct and follows Go conventions (x64 → amd64, win32 → windows). The binary naming is consistent with the expected asset names in GitHub releases.
sdk-libs/program-test/src/program_test/light_program_test.rs (2)
11-11: LGTM! Clean import simplification.The removal of
ProverConfigfrom the import is consistent with the simplified prover spawning logic.
322-329: No changes required
Thedevenvfeature is opt-in (default = []) and by design always spawns the prover, whileconfig.with_proveris respected whendevenvis disabled.program-tests/system-cpi-v2-test/tests/invoke_cpi_with_read_only.rs (1)
30-30: LGTM! API usage updated correctly.The import and all call sites have been properly updated to use the new parameterless
spawn_prover()signature, removing the now-obsoleteProverConfigdependency.Also applies to: 50-50, 349-349, 663-663, 1263-1263, 2023-2023, 2321-2321, 2840-2840
prover/client/tests/combined.rs (1)
3-3: LGTM! Test updated correctly.The test properly adopts the new parameterless
spawn_prover()API.Also applies to: 13-13
prover/client/tests/non_inclusion.rs (1)
3-3: LGTM! Test migrated to new API.The test correctly uses the simplified
spawn_prover()function without configuration.Also applies to: 13-13
forester/tests/e2e_test.rs (1)
21-21: LGTM! Test configuration migrated correctly.The changes properly reflect the API evolution:
- Removed
ProverConfigimport- Switched from
prover_config: Nonetoenable_prover: false- Updated to parameterless
spawn_prover()callThe refactor maintains the same behavior while simplifying the API surface.
Also applies to: 246-246, 255-255
prover/client/tests/batch_address_append.rs (1)
9-9: LGTM! API simplification is correctly applied.The removal of
ProverConfigfrom the import and the updatedspawn_prover()call signature are consistent with the new parameterless API design shown inprover/client/src/prover.rs.Also applies to: 26-26
program-tests/compressed-token-test/tests/test.rs (4)
47-47: LGTM! Import updated correctly.The import now references only
spawn_prover, consistent with the removal ofProverConfigandProverModefrom the public API.
597-597: LGTM! Prover initialization calls are consistent.All
spawn_prover().awaitcalls follow the new parameterless API. The pattern of calling it once per test function is appropriate, and the implementation's internal deduplication (viaIS_LOADINGflag) prevents redundant spawns.Also applies to: 1253-1253, 1416-1416, 1560-1560, 1635-1635, 2823-2823, 3101-3101, 3465-3465, 3654-3654, 3918-3918, 4212-4212
1332-1336: Track TODO for type re-exports.The TODO comment suggests centralizing type exports from the light-program test module. Consider creating a tracking issue to address this technical debt.
Is there already a tracking issue for consolidating these type re-exports?
4446-4453: LGTM! Validator config correctly enables prover.Setting
enable_prover: trueinLightValidatorConfigis the correct approach for the Photon test, replacing the previous explicit prover configuration pattern.prover/server/prover/common/proving_keys_utils.go (1)
278-290: LGTM! Clean refactor with backward compatibility.The delegation pattern maintains backward compatibility for
LoadKeyswhileLoadKeysWithConfigadds download config support. The pre-loading verification viaEnsureKeysExistensures keys are available before attempting to read them, providing better error messages and auto-download capability.prover/server/prover/common/lazy_key_manager.go (5)
10-30: LGTM! Well-structured lazy loading manager.The
LazyKeyManagerstruct properly encapsulates lazy loading state with:
- Separate caches for merkle and batch systems
- Per-key loading locks to prevent duplicate concurrent loads
- Proper mutex for thread-safe access
- Fallback to
DefaultDownloadConfig()when nil is passed
32-77: LGTM! Efficient cache-first retrieval pattern.Both
GetMerkleSystemandGetBatchSystemfollow a sound pattern:
- Generate deterministic cache key from parameters
- Check cache under read lock (minimizes contention)
- Delegate to load function if not cached
The cache key format is consistent between retrieval (
GetMerkleSystem) and storage (cacheSystemat lines 400-419), ensuring correct cache hits.
192-220: LGTM! Correct concurrent loading coordination.The locking mechanism properly coordinates concurrent loads:
acquireLoadingLockreturns a channel only for the first goroutine to request a key- Subsequent goroutines get nil and must
waitForLoadingon the shared channelreleaseLoadingLockcleans up and signals completionThis pattern prevents duplicate loads while allowing concurrent loads of different keys.
222-275: LGTM! Clear key path mappings.Both
determineMerkleKeyPathanddetermineBatchKeyPathprovide explicit mappings from system parameters to key file paths. Returning empty string for unsupported parameters is handled gracefully by callers (lines 101-104, 155-157) with descriptive errors.The mappings align with the key generation scheme used throughout the codebase (v1 vs v2, specific height/size combinations).
277-438: LGTM! Comprehensive preloading support.The preload methods cover multiple use cases:
PreloadForRunMode: Load keys for a specific run mode (e.g., Full, FullTest)PreloadAll: Load all keys across Full and FullTest modesPreloadCircuits: Load keys for specific circuits or key namesThe
cacheSystemmethod correctly generates cache keys matching the format used byGetMerkleSystem/GetBatchSystem(lines 40-48, 64), ensuring preloaded systems are found in subsequent lookups.
GetStatsprovides useful observability into cache state and ongoing loads.prover/server/integration_test.go (3)
38-91: LGTM! Well-structured lazy loading integration.
StartServerWithPreloadcleanly separates two key loading strategies:
- Preload mode (lines 52-59): Eagerly loads all keys for the run mode via
PreloadForRunMode, failing fast if keys are unavailable- Lazy mode (lines 60-77): Preloads only test circuits via
PreloadCircuits, logging warnings but continuing on failure (keys download on-demand later)This design enables testing both eager and lazy loading paths. The
DefaultDownloadConfig()withAutoDownload: true(lines 47-48) ensures keys are fetched as needed.
94-100: LGTM! Idempotent server shutdown.The
serverStoppedguard (lines 94-96) prevents double-shutdown errors whenStopServeris called multiple times. This is particularly useful in test cleanup paths and deferred shutdown calls.
172-183: LGTM! Lazy loading test properly isolated.
TestLightweightLazycorrectly skips whenpreloadKeysis true (lines 173-175), ensuring it only runs in lazy mode. This addresses the previous review concern about the test lacking skip logic.The test exercises the same functionality as
TestLightweightbut with keys loaded on-demand, providing coverage for both eager and lazy loading code paths.prover/server/redis_queue_test.go (2)
679-699: LGTM! Clean migration to centralized key management.Worker creation (lines 681, 686, 691) now uses the new
LazyKeyManagerinstead of passing explicit proof system slices. The workers will fetch systems on-demand viakeyManager.GetMerkleSystemandkeyManager.GetBatchSystemwhen processing jobs.This centralizes key management and enables lazy loading in queue workers.
848-860: LGTM! Test server startup updated for key manager.The test server initialization (line 859) now passes
keyManagertoRunEnhanced, consistent with the updated signature. UsingDefaultDownloadConfig()ensures tests behave like production with auto-download enabled.prover/server/server/server.go (4)
271-273: LGTM! Simplified handler structure.The
proveHandlerstruct now holds a singlekeyManagerreference instead of multiple proof system slices, runMode, and circuits fields. This centralizes key management and enables lazy loading of proof systems on-demand.
459-584: LGTM! Consistent signature updates across run entry points.All server run functions (
Run,RunWithQueue,RunEnhanced) now accept akeyManagerparameter instead of explicit proof systems, runMode, and circuits. The handler initialization (lines 484-487) correctly wires the keyManager into theproveHandler.This refactor maintains the overall server structure while enabling lazy loading of proof systems.
883-953: LGTM! Batch proof handlers correctly use lazy loading.All batch proof handlers (
batchAddressAppendProof,batchAppendHandler,batchUpdateProof) follow a consistent pattern:
- Parse parameters to extract tree height and batch size
- Call
keyManager.GetBatchSystemto retrieve or load the appropriate proving system- Generate the proof using the system
Error messages wrap underlying errors with context (e.g., "batch update: %w" at line 944), aiding debugging.
955-1085: LGTM! Non-batch proof handlers properly determine versions.The proof handlers (
inclusionProof,nonInclusionProof,combinedProof) correctly usekeyManager.GetMerkleSystemto retrieve or load systems based on request parameters.Version determination logic (lines 994-997, 1046-1049) is sound:
- Address tree height 40 → version 2
- Otherwise → version 1
This aligns with the explicit height checks later (lines 1010, 1025, 1062, 1072) that dispatch to v1 or v2 prover implementations. Error wrapping provides clear context for debugging.
prover/server/server/queue_job.go (7)
33-33: LGTM: Clean refactor to centralized key management.The BaseQueueWorker now uses a shared LazyKeyManager instead of explicit proving system fields. All three constructors (NewUpdateQueueWorker, NewAppendQueueWorker, NewAddressAppendQueueWorker) consistently initialize the keyManager field.
Also applies to: 51-85
266-293: LGTM: Inclusion proof retrieval simplified.The keyManager.GetMerkleSystem call replaces the previous loop-based lookup. Parameters correctly specify state tree configuration with zeros for address tree parameters (inclusion proofs don't use address trees). Error handling is consistent.
295-322: LGTM: Non-inclusion proof retrieval simplified.The keyManager.GetMerkleSystem call with addressTreeHeight and numAddresses parameters (zeros for state tree parameters) correctly retrieves the non-inclusion proof system. Error handling is consistent with other proof types.
324-351: LGTM: Combined proof retrieval simplified.The keyManager.GetMerkleSystem call correctly specifies all four parameters (state tree height/inputs and address tree height/addresses) for combined proofs that need both tree configurations. Error handling is consistent.
353-369: LGTM: Batch update proof retrieval simplified.The keyManager.GetBatchSystem call with circuit type, height, and batch size correctly retrieves the batch update system. Direct proving call is cleaner than the previous loop-based lookup. Error handling is consistent.
371-387: LGTM: Batch append proof retrieval simplified.Consistent with batch update changes. The keyManager.GetBatchSystem call correctly retrieves the batch append system, and the direct proving call eliminates the need for loop-based lookup. Error handling is consistent.
389-406: LGTM Use ofparams.TreeHeightmatches the struct definition; no changes required.prover/server/main.go (5)
476-562: LGTM: Well-structured download command.The new "download" subcommand provides a clean interface for provisioning proving keys. Key aspects:
- Comprehensive flags for run-mode, circuits, keys directory, and download configuration
- Proper error handling at each step (parse run-mode, get keys, ensure keys exist)
- Edge case handled: returns error if no keys are required (lines 541-543)
- Logging is thorough and includes configuration and progress updates
- The
verify-onlyflag correctly inverts toAutoDownload: !verifyOnly(line 526)
578-586: LGTM: Sensible preload and download flags.The new flags provide flexible control over key loading:
preload-keys(none/run-mode/all) allows users to choose lazy vs eager loading strategy, defaulting to lazy ("none")preload-circuitsenables selective preloading of specific circuitsauto-downloaddefaults to true, providing good UX by automatically fetching missing keys- Download configuration flags (URL and retries) use sensible defaults from common package
Also applies to: 602-616
623-684: LGTM: Well-structured key manager initialization.The keyManager initialization and preloading logic is clean and robust:
- DownloadConfig is properly constructed from CLI flags (lines 626-632)
- LazyKeyManager is initialized with the config (line 634)
- Comprehensive logging before and after initialization (lines 640-683)
- Preload logic correctly handles three modes:
- "run-mode": preloads keys for specified run-mode with proper validation (lines 656-663)
- "all": preloads all available keys (lines 664-668)
- "none": skips preloading (lazy load on demand)
- Invalid preload-keys value returns an error (lines 669-671)
- Edge case handled: run-mode preload requires --run-mode flag (lines 657-659)
- Selective circuit preloading supported independently (lines 673-678)
- Stats logged for observability (lines 680-683)
718-747: LGTM: Consistent queue worker initialization.All three queue workers (update, append, address-append) are consistently updated to use the keyManager:
- Each worker is constructed with
redisQueueandkeyManagerparameters (lines 732, 736, 740)- Workers are properly stored in the workers slice for later shutdown (lines 733, 737, 741)
- Each worker is started in a separate goroutine (lines 734, 738, 742)
- Comprehensive logging confirms all workers started (lines 744-746)
756-762: LGTM: Server startup updated for key manager.Both server startup paths consistently use the keyManager:
RunWithQueuecalled with&config, redisQueue, keyManager(line 756)Runcalled with&config, keyManager(line 762)- Proper logging distinguishes between queue-enabled and standalone modes (lines 757-766)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/actions/setup-and-build/action.yml (1)
121-121: Broaden cache invalidation to cover all key-version sourcesGood move hashing Go sources. To avoid stale keys when version/checksum/URLs move to another file, include the entire common folder’s Go files in the hash (or any manifest) so cache invalidates on any key change.
Suggested tweak:
- key: ${{ runner.os }}-proving-keys-${{ inputs.cache-suffix }}${{ inputs.cache-suffix && '-' || '' }}${{ hashFiles('prover/server/prover/common/key_downloader.go', 'prover/server/prover/common/proving_keys_utils.go') }} + key: ${{ runner.os }}-proving-keys-${{ inputs.cache-suffix }}${{ inputs.cache-suffix && '-' || '' }}${{ hashFiles('prover/server/prover/common/*.go') }}Additionally, if the bash script at Lines 123–127 influences which keys are fetched, either include it in the hash or ensure it delegates to the new Go EnsureKeysExist logic so the hash remains the single source of truth.
Please confirm where key URLs/checksums/versions live so we can ensure all such files are included in the hash. If they reside outside common/*.go (e.g., a manifest), add that path to hashFiles as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/actions/setup-and-build/action.yml(1 hunks)prover/server/scripts/download_keys.sh(0 hunks)scripts/devenv/download-gnark-keys.sh(1 hunks)
💤 Files with no reviewable changes (1)
- prover/server/scripts/download_keys.sh
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/devenv/download-gnark-keys.sh (1)
scripts/devenv/shared.sh (1)
download(78-81)
🪛 Shellcheck (0.11.0)
scripts/devenv/download-gnark-keys.sh
[warning] 27-27: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ 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). (24)
- GitHub Check: lean-verification
- GitHub Check: integration-test-lightweight
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: integration-test-lightweight-lazy
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: lint
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
- GitHub Check: stateless-js-v1
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v2
- GitHub Check: cli-v2
- GitHub Check: Forester e2e test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
prover/server/prover/common/proving_keys_utils.go (1)
120-202: Excellent refactor: Cross-platform path handling.Replacing string concatenation with
filepath.Joinfor all key path construction ensures proper path separator handling across Windows, Linux, and macOS. This is a best practice for Go code dealing with file paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
prover/server/prover/common/proving_keys_utils.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
prover/server/prover/common/proving_keys_utils.go (1)
prover/server/prover/common/key_downloader.go (3)
DefaultDownloadConfig(32-40)DownloadConfig(24-30)EnsureKeysExist(372-413)
🪛 Gitleaks (8.28.0)
prover/server/prover/common/proving_keys_utils.go
[high] 156-156: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 157-157: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 158-158: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 160-160: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 161-161: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 162-162: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 164-164: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 165-165: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 166-166: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 167-167: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 169-169: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 170-170: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 171-171: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 172-172: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 174-174: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 175-175: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 177-177: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 178-178: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (25)
- GitHub Check: integration-test-lightweight
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: test-with-redis (Queue processing flow tests, go test -v -run TestJobProcessingFlow -timeout 5m)
- GitHub Check: test-with-redis (Redis Queue tests, go test -v -run TestRedis -timeout 10m)
- GitHub Check: lean-verification
- GitHub Check: stateless-js-v2
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: lint
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: cli-v1
- GitHub Check: stateless-js-v1
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: Forester e2e test
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: cli-v2
🔇 Additional comments (3)
prover/server/prover/common/proving_keys_utils.go (3)
9-9: LGTM! Import addition supports cross-platform path handling.The
path/filepathimport is correctly added to support thefilepath.Joincalls used throughout the file for constructing key file paths.
279-291: Well-designed integration with lazy key management.The refactoring maintains backward compatibility while adding lazy download functionality:
LoadKeysdelegates toLoadKeysWithConfigwith default config, preserving the existing APILoadKeysWithConfigadds pre-loading validation viaEnsureKeysExist, which downloads missing keys when auto-download is enabled- Error handling is appropriate with wrapped errors
This design enables on-demand key downloads without breaking existing callers.
156-178: Static analysis false positives: Not actual API keys.The Gitleaks alerts flagging these lines as "Generic API Key" are false positives. These are file paths for proving key files (e.g.,
v1_combined_26_26_1_1.key), not secret credentials. The tool incorrectly pattern-matched on the.keyfile extension.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/devenv/download-gnark-keys.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: lean-verification
- GitHub Check: integration-test-lightweight
- GitHub Check: integration-test-lightweight-lazy
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: Forester e2e test
- GitHub Check: stateless-js-v2
- GitHub Check: cli-v1
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v2
- GitHub Check: lint
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/src/utils/downloadProverBinary.ts (1)
16-61: Checksum verification is still missing.The previous review flagged the absence of checksum verification as a critical security issue. The downloaded binary is not verified for integrity or authenticity, creating a risk of executing compromised or corrupted code if the download is intercepted, the release is tampered with, or the transfer is corrupted.
As recommended in the previous review, implement checksum verification using one of these approaches:
Option 1 (Recommended): Store expected SHA256 checksums and verify after download:
+import crypto from "crypto"; + +const PROVER_CHECKSUMS: Record<string, string> = { + "light-prover-linux": "abc123...", // Replace with actual checksums + "light-prover-macos": "def456...", + "light-prover.exe": "ghi789...", +}; + +async function verifyChecksum(filePath: string, expectedChecksum: string): Promise<boolean> { + const fileBuffer = fs.readFileSync(filePath); + const hash = crypto.createHash("sha256").update(fileBuffer).digest("hex"); + return hash === expectedChecksum; +} + export async function downloadProverBinary( binaryPath: string, binaryName: string, options: DownloadOptions = {}, ): Promise<void> { // ... existing code ... for (let attempt = 1; attempt <= maxRetries; attempt++) { try { await downloadFile(url, binaryPath); + const expectedChecksum = PROVER_CHECKSUMS[binaryName]; + if (!expectedChecksum) { + throw new Error(`No checksum available for ${binaryName}`); + } + if (!(await verifyChecksum(binaryPath, expectedChecksum))) { + fs.unlinkSync(binaryPath); + throw new Error("Checksum verification failed - binary may be corrupted or tampered"); + } + if (process.platform !== "win32") { fs.chmodSync(binaryPath, 0o755); }Option 2: Download and verify a
.sha256file from the same release, then verify as in Option 1.
🧹 Nitpick comments (3)
cli/src/utils/downloadProverBinary.ts (3)
7-8: Consider externalizing the prover version constant.The
PROVER_VERSIONis hardcoded, requiring code changes whenever the version is updated. Consider sourcing it frompackage.jsonor a dedicated config file for easier maintenance and consistency across the codebase.For example, if the version is defined in
package.jsonunder a field likeproverVersion:import { readFileSync } from "fs"; import { join } from "path"; const packageJson = JSON.parse( readFileSync(join(__dirname, "../../package.json"), "utf-8") ); const PROVER_VERSION = packageJson.proverVersion || "1.0.2";
134-134: Consider wrapping cleanup in try-catch to prevent masking errors.The
fs.unlinkSync(outputPath)on Line 134 could throw if the file is locked or permissions prevent deletion. If this happens, the original pipeline error would be masked by the unlink error.Apply this diff to ensure the original error is always reported:
pipeline(response, fileStream) .then(() => { if (totalBytes > 0) { process.stdout.write("\r Progress: 100% - Download complete\n"); } resolve(); }) .catch((error) => { - fs.unlinkSync(outputPath); + try { + fs.unlinkSync(outputPath); + } catch (cleanupError) { + // Log but don't throw, preserve original error + console.error(`Warning: Failed to cleanup ${outputPath}:`, cleanupError); + } reject(error); });
1-155: Consider consolidating with existing download utility.The codebase now has two download utilities:
- This file:
cli/src/utils/downloadProverBinary.ts(uses nativehttp/https)- Existing:
cli/src/psp-utils/download.ts(usesaxios, has progress bar viacliProgress)While the native approach avoids the
axiosdependency, maintaining two similar implementations creates potential for inconsistencies and duplicated effort (e.g., both would need checksum verification).Consider either:
- Extending
psp-utils/download.tsto handle binary downloads with a simpler interface, or- Extracting common download logic into a shared utility that both can use
This would centralize retry logic, checksum verification, and progress reporting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/src/utils/downloadProverBinary.ts(1 hunks)scripts/devenv/download-gnark-keys.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli/src/utils/downloadProverBinary.ts (1)
cli/src/psp-utils/download.ts (1)
downloadFile(142-211)
⏰ 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). (24)
- GitHub Check: lean-verification
- GitHub Check: integration-test-lightweight-lazy
- GitHub Check: integration-test-lightweight
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v1
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: cli-v2
- GitHub Check: stateless-js-v2
- GitHub Check: lint
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: Forester e2e test
🔇 Additional comments (1)
cli/src/utils/downloadProverBinary.ts (1)
82-88: Redirect depth limit successfully implemented.The redirect depth check properly prevents infinite redirect loops by enforcing the
MAX_REDIRECTSlimit. This addresses the major issue flagged in the previous review.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/devenv/download-gnark-keys.sh (1)
27-40: cd and go run failure paths are correctly handledPrevious concerns are addressed: cd guarded and go run gated with proper exit on failure. LGTM.
🧹 Nitpick comments (3)
scripts/devenv/download-gnark-keys.sh (3)
1-4: Enable strict mode and guard sourcing shared.shImprove safety and fail-fast behavior; also emit a clear error if shared.sh is missing.
#!/usr/bin/env bash +set -Eeuo pipefail +trap 'echo "Error on line $LINENO" >&2' ERR + SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/shared.sh" +source "${SCRIPT_DIR}/shared.sh" || { + echo "Error: cannot source ${SCRIPT_DIR}/shared.sh" >&2 + exit 1 +}
6-11: Make variables local and add a non-git fallback for ROOT_DIRGuard against non-git environments (archives, CI checkouts) and avoid leaking vars.
download_gnark_keys() { - local key_type=${1:-light} - ROOT_DIR="$(git rev-parse --show-toplevel)" - PROVER_DIR="${ROOT_DIR}/prover/server" - KEYS_DIR="${ROOT_DIR}/prover/server/proving-keys" + local key_type=${1:-light} + local RUN_MODE exit_code + local ROOT_DIR + if ! ROOT_DIR="$(git rev-parse --show-toplevel 2>/dev/null)"; then + ROOT_DIR="$(cd "${SCRIPT_DIR}/../.." && pwd)" + fi + local PROVER_DIR="${ROOT_DIR}/prover/server" + local KEYS_DIR="${ROOT_DIR}/prover/server/proving-keys"
25-26: Avoid parsing ls for emptiness checks (ShellCheck SC2012)Use find to test directory non-emptiness; more robust and quiet.
- if [ ! -d "${KEYS_DIR}" ] || [ -z "$(ls -A "${KEYS_DIR}" 2>/dev/null)" ]; then + if [ ! -d "${KEYS_DIR}" ] || [ -z "$(find "${KEYS_DIR}" -mindepth 1 -print -quit 2>/dev/null)" ]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/devenv/download-gnark-keys.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/devenv/download-gnark-keys.sh (1)
scripts/devenv/shared.sh (2)
download(78-81)log(51-51)
⏰ 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). (25)
- GitHub Check: test-with-redis (Failed job status tests, go test -v -run TestFailedJobStatus -timeout 5m)
- GitHub Check: integration-test-lightweight-lazy
- GitHub Check: lean-verification
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: integration-test-lightweight
- GitHub Check: Forester e2e test
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: Test program-libs-slow
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: stateless-js-v2
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v1
- GitHub Check: lint
- GitHub Check: cli-v2
🔇 Additional comments (1)
scripts/devenv/download-gnark-keys.sh (1)
35-35: No action needed: INSTALL_LOG is already defined
The INSTALL_LOG variable is exported in scripts/devenv/shared.sh before any log calls, so the log function will always have a valid target.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cli/src/utils/downloadProverBinary.ts (1)
16-61: Checksum verification still missing.The previously flagged critical security issue remains unaddressed: the downloaded binary is not verified for integrity or authenticity using a checksum or cryptographic signature. Without this verification, malicious or corrupted binaries could be executed.
Please refer to the previous review comment for implementation approaches (storing SHA256 checksums or downloading
.sha256files from the release).
🧹 Nitpick comments (3)
cli/src/utils/downloadProverBinary.ts (3)
146-149: Consider making the download timeout configurable.The 60-second timeout is hardcoded, which may be insufficient for large binaries on slow connections. Consider adding a timeout option to
DownloadOptionsto allow callers to adjust this value based on their needs.Example:
interface DownloadOptions { maxRetries?: number; retryDelay?: number; + timeoutMs?: number; } export async function downloadProverBinary( binaryPath: string, binaryName: string, options: DownloadOptions = {}, ): Promise<void> { - const { maxRetries = 3, retryDelay = 2000 } = options; + const { maxRetries = 3, retryDelay = 2000, timeoutMs = 60000 } = options; // ... - await downloadFile(url, binaryPath); + await downloadFile(url, binaryPath, 0, timeoutMs); } async function downloadFile( url: string, outputPath: string, redirectDepth: number = 0, + timeoutMs: number = 60000, ): Promise<void> { // ... - request.setTimeout(60000, () => { + request.setTimeout(timeoutMs, () => { request.destroy(); reject(new Error("Download timeout")); }); }
126-144: Consider adding existence check before cleanup on line 134.For consistency with the error handler on lines 140-142, consider checking if the file exists before attempting to delete it in the pipeline catch block.
Apply this diff:
pipeline(response, fileStream) .then(() => { if (totalBytes > 0) { process.stdout.write("\r Progress: 100% - Download complete\n"); } resolve(); }) .catch((error) => { - fs.unlinkSync(outputPath); + if (fs.existsSync(outputPath)) { + fs.unlinkSync(outputPath); + } reject(error); });
1-155: Observation: Similar download functionality exists in the codebase.There's another download utility at
cli/src/psp-utils/download.ts(lines 141-210) that uses axios and cliProgress for similar functionality. While your implementation uses native http/https (fewer dependencies) and theirs uses axios (more features), consider whether consolidating or sharing common patterns could reduce maintenance burden.This is just an observation—the current approach is valid, especially if you prefer avoiding axios as a dependency for this specific use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/src/utils/downloadProverBinary.ts(1 hunks)prover/server/main.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
prover/server/main.go (5)
prover/server/prover/common/key_downloader.go (6)
DefaultBaseURL(18-18)DefaultMaxRetries(19-19)DownloadConfig(24-30)DefaultRetryDelay(20-20)DefaultMaxRetryDelay(21-21)EnsureKeysExist(372-413)prover/server/prover/common/proving_keys_utils.go (2)
GetKeys(113-277)RunMode(17-17)prover/server/prover/common/lazy_key_manager.go (1)
NewLazyKeyManager(19-30)prover/server/server/queue_job.go (3)
NewUpdateQueueWorker(51-61)NewAppendQueueWorker(63-73)NewAddressAppendQueueWorker(75-85)prover/server/server/server.go (2)
RunWithQueue(459-467)Run(582-584)
cli/src/utils/downloadProverBinary.ts (1)
cli/src/psp-utils/download.ts (1)
downloadFile(142-211)
🪛 GitHub Actions: PR #1977
prover/server/main.go
[error] 647-647: declared and not used: runMode. Remove the variable or use it.
🪛 GitHub Actions: Test gnark prover
prover/server/main.go
[error] 647-647: Go build error: declared and not used: runMode
⏰ 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). (12)
- GitHub Check: stateless-js-v1
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: cli-v1
- GitHub Check: cli-v2
- GitHub Check: lint
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: stateless-js-v2
- GitHub Check: Forester e2e test
🔇 Additional comments (8)
cli/src/utils/downloadProverBinary.ts (2)
71-93: LGTM! Redirect depth limit correctly implemented.The redirect handling now properly tracks depth and enforces the MAX_REDIRECTS limit before following additional redirects. This prevents infinite redirect loops while allowing legitimate redirect chains.
153-155: LGTM!Simple and appropriate version getter.
prover/server/main.go (6)
476-562: LGTM! Well-structured download command.The new
downloadsubcommand is cleanly implemented with:
- Comprehensive flags for controlling download behavior
- Proper configuration construction and logging
- Validation to ensure keys exist before attempting download
- Clear error messages and appropriate use of
EnsureKeysExist
578-616: LGTM! Well-designed flags for lazy key management.The new flags provide fine-grained control over key preloading and auto-download behavior:
preload-keysdefaults to "none" for true lazy loadingpreload-circuitsallows selective preloadingauto-downloaddefaults totrueas intended per the PR descriptionThe flag design aligns well with the lazy key management architecture.
623-646: LGTM! LazyKeyManager initialization is well-structured.The download configuration and key manager initialization is clean:
- DownloadConfig properly constructed from CLI flags
- LazyKeyManager created with keys directory and download config
- Comprehensive logging of initialization parameters
656-677: LGTM! Preloading logic correctly handles all cases.The preloading logic properly handles:
- "all": preload everything
- "none": lazy load (default)
- Specific run mode: preload keys for that mode
- Specific circuits: preload those circuits
Error messages are clear and helpful.
731-746: LGTM! Workers correctly wired to use LazyKeyManager.All queue workers (update, append, address-append) are properly instantiated with the
keyManagerparameter, consistent with the refactoring to lazy key loading.
755-761: LGTM! Server startup functions correctly use LazyKeyManager.Both
RunWithQueueandRunnow receive thekeyManagerparameter, completing the refactoring from directLoadKeysto the lazy loading model.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/src/utils/downloadProverBinary.ts (1)
16-61: Critical security gap: Downloaded binary lacks integrity verification.The function downloads and executes a binary without verifying its checksum or cryptographic signature. This creates a serious security vulnerability where corrupted, intercepted (MITM), or compromised releases could execute malicious code on user systems.
As noted in previous reviews, implement checksum verification before accepting the downloaded binary. Options include:
- Hardcoded checksums per version/platform (fetch expected checksum, compute file's SHA256, compare)
- Download separate
.sha256file from the release and verify against itThe verification should occur after download (line 37) but before chmod and return, with cleanup on mismatch.
🧹 Nitpick comments (5)
cli/src/utils/downloadProverBinary.ts (2)
7-7: Consider externalizing the prover version.The hardcoded
PROVER_VERSIONconstant requires manual updates for each release. If this version needs to stay synchronized with other parts of the codebase (e.g., package.json), consider reading it from a shared configuration file or package metadata to reduce maintenance overhead and prevent version drift.
63-151: Consider consolidating with existing download utility.There's an existing
downloadFilefunction incli/src/psp-utils/download.tsthat handles similar download operations with axios and a progress bar library. While the implementations differ in approach (axios vs. native http/https), maintaining two separate download utilities increases maintenance burden.Consider:
- Evaluating whether one implementation can serve both use cases
- Extracting common download logic into a shared utility
- Documenting why separate implementations are necessary if they must coexist
This can be deferred to a future refactor if the implementations have sufficiently different requirements.
prover/server/main.go (3)
476-562: Newdownloadcommand looks good; expose backoff knobs and ensure keys dir existsConsider adding retry/backoff flags to match DownloadConfig and pre-create the keys directory for clearer failures.
Apply this diff to add flags and wire them:
@@ Flags: []cli.Flag{ @@ &cli.IntFlag{ Name: "max-retries", Usage: "Maximum number of retries for downloading keys", Value: common.DefaultMaxRetries, }, + &cli.DurationFlag{ + Name: "retry-delay", + Usage: "Initial backoff between retries (e.g., 5s)", + Value: common.DefaultRetryDelay, + }, + &cli.DurationFlag{ + Name: "max-retry-delay", + Usage: "Maximum backoff between retries (e.g., 5m)", + Value: common.DefaultMaxRetryDelay, + }, &cli.BoolFlag{ Name: "verify-only", Usage: "Only verify existing keys without downloading", Value: false, }, }, Action: func(context *cli.Context) error { @@ keysDirPath := context.String("keys-dir") verifyOnly := context.Bool("verify-only") + // Ensure keys directory exists + if err := os.MkdirAll(keysDirPath, 0755); err != nil { + return fmt.Errorf("failed to create keys dir %q: %w", keysDirPath, err) + } + // Configure download settings downloadConfig := &common.DownloadConfig{ BaseURL: context.String("download-url"), MaxRetries: context.Int("max-retries"), - RetryDelay: common.DefaultRetryDelay, - MaxRetryDelay: common.DefaultMaxRetryDelay, + RetryDelay: context.Duration("retry-delay"), + MaxRetryDelay: context.Duration("max-retry-delay"), AutoDownload: !verifyOnly, }Also consider harmonizing flag naming with "start" (e.g., use download-max-retries here too) for consistency.
578-617: Start flags: good additions; consider exposing retry/backoff and tightening UXExpose retry delays to avoid hardcoded defaults and clarify how to pass multiple circuits.
Apply this diff to add flags and clarify usage:
@@ &cli.StringSliceFlag{ Name: "preload-circuits", - Usage: "Preload specific circuits, e.g.: update,append,batch_update_32_500,batch_append_32_500)", + Usage: "Preload specific circuits (repeat flag), e.g.: --preload-circuits update --preload-circuits append", }, @@ &cli.StringFlag{ Name: "download-url", Usage: "Base URL for downloading key files", Value: common.DefaultBaseURL, }, &cli.IntFlag{ Name: "download-max-retries", Usage: "Maximum number of retries for downloading keys", Value: common.DefaultMaxRetries, }, + &cli.DurationFlag{ + Name: "download-retry-delay", + Usage: "Initial backoff between retries (e.g., 5s)", + Value: common.DefaultRetryDelay, + }, + &cli.DurationFlag{ + Name: "download-max-retry-delay", + Usage: "Maximum backoff between retries (e.g., 5m)", + Value: common.DefaultMaxRetryDelay, + },And wire them when building DownloadConfig (see next comment).
Question: Are "start" flags run-mode/circuit still consumed downstream? If not, consider deprecation to reduce confusion.
623-672: Log download config at startup and wire delay flagsMake retry/backoff configurable (if added) and log them for observability.
Apply this diff:
@@ - downloadConfig := &common.DownloadConfig{ - BaseURL: context.String("download-url"), - MaxRetries: context.Int("download-max-retries"), - RetryDelay: common.DefaultRetryDelay, - MaxRetryDelay: common.DefaultMaxRetryDelay, - AutoDownload: context.Bool("auto-download"), - } + downloadConfig := &common.DownloadConfig{ + BaseURL: context.String("download-url"), + MaxRetries: context.Int("download-max-retries"), + RetryDelay: context.Duration("download-retry-delay"), + MaxRetryDelay: context.Duration("download-max-retry-delay"), + AutoDownload: context.Bool("auto-download"), + } @@ - logging.Logger().Info(). + logging.Logger().Info(). Str("preload_keys", preloadKeys). Strs("preload_circuits", preloadCircuits). Str("keys_dir", keysDirPath). + Str("download_url", downloadConfig.BaseURL). + Int("download_max_retries", downloadConfig.MaxRetries). + Dur("retry_delay", downloadConfig.RetryDelay). + Dur("max_retry_delay", downloadConfig.MaxRetryDelay). + Bool("auto_download", downloadConfig.AutoDownload). Msg("Initializing lazy key manager")Optional: pre-validate preloadCircuits values to fail fast on typos (if PreloadCircuits doesn’t already).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/src/utils/downloadProverBinary.ts(1 hunks)prover/server/main.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cli/src/utils/downloadProverBinary.ts (1)
cli/src/psp-utils/download.ts (1)
downloadFile(142-211)
prover/server/main.go (5)
prover/server/prover/common/key_downloader.go (6)
DefaultBaseURL(18-18)DefaultMaxRetries(19-19)DownloadConfig(24-30)DefaultRetryDelay(20-20)DefaultMaxRetryDelay(21-21)EnsureKeysExist(372-413)prover/server/prover/common/proving_keys_utils.go (1)
GetKeys(113-277)prover/server/prover/common/lazy_key_manager.go (1)
NewLazyKeyManager(19-30)prover/server/server/queue_job.go (3)
NewUpdateQueueWorker(51-61)NewAppendQueueWorker(63-73)NewAddressAppendQueueWorker(75-85)prover/server/server/server.go (2)
RunWithQueue(459-467)Run(582-584)
⏰ 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). (27)
- GitHub Check: lean-verification
- GitHub Check: test-with-redis (Queue processing flow tests, go test -v -run TestJobProcessingFlow -timeout 5m)
- GitHub Check: integration-test-lightweight-lazy
- GitHub Check: test-with-redis (Failed job status tests, go test -v -run TestFailedJobStatus -timeout 5m)
- GitHub Check: test-no-redis (Unit tests, go test ./prover/... -timeout 60m)
- GitHub Check: integration-test-lightweight
- GitHub Check: stateless-js-v1
- GitHub Check: cli-v1
- GitHub Check: system-programs (sdk-anchor-test-program, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -...
- GitHub Check: system-programs (sdk-test-program, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v...
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client, cargo test...
- GitHub Check: create_release
- GitHub Check: cli-v2
- GitHub Check: stateless-js-v2
- GitHub Check: system-programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_addre...
- GitHub Check: system-programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-syst...
- GitHub Check: system-programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test...
- GitHub Check: system-programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_c...
- GitHub Check: system-programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test ...
- GitHub Check: system-programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-t...
- GitHub Check: system-programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test -- --skip te...
- GitHub Check: system-programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test",...
- GitHub Check: Forester e2e test
- GitHub Check: Test program-libs-fast
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-slow
- GitHub Check: lint
🔇 Additional comments (5)
cli/src/utils/downloadProverBinary.ts (3)
11-14: LGTM!The interface is well-defined with appropriate optional properties for download configuration.
63-151: LGTM! Redirect depth limit successfully implemented.The
downloadFilefunction now includes proper redirect depth checking (lines 82-88), which addresses the previously flagged major issue about infinite redirect vulnerability. The implementation correctly:
- Tracks redirect depth through recursive calls
- Enforces a maximum of 10 redirects
- Provides clear error messages when the limit is exceeded
Additionally, the function demonstrates solid engineering with appropriate timeout handling (60s), error cleanup, and user-friendly progress reporting.
153-155: LGTM!Simple, clean getter function providing external access to the prover version.
prover/server/main.go (2)
720-735: Queue workers now receive keyManager — LGTMCorrect dependency injection; aligns workers with lazy key loading.
744-751: Server startup now passes keyManager — LGTMSignatures match server.RunWithQueue/Run; integration looks correct.
Summary by CodeRabbit
New Features
Bug Fixes / Changes
Tests
Chores
Documentation