feat(prover): make ProverEngine mockable to speed up CI #381
Conversation
WalkthroughThis change introduces a trait-based abstraction for the prover engine, enabling dynamic selection and mocking of prover implementations. Proof representation is unified into a serializable Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant MockEngine as MockProverEngine
participant Prover as Prover
participant Sequencer as Sequencer
participant Syncer as Syncer
Test->>MockEngine: create_mock_engine()
Test->>Prover: Prover::new_with_engine(..., MockEngine, ...)
Prover->>Sequencer: initialize with MockEngine
Prover->>Syncer: initialize with MockEngine
Test->>Prover: perform test actions
Prover->>MockEngine: prove_epoch(...) / verify_proof(...)
MockEngine-->>Prover: returns mocked proofs/results
Prover-->>Test: test assertions
sequenceDiagram
participant Prover as Prover
participant Engine as ProverEngine (trait)
participant SP1 as SP1ProverEngine
participant DB as Database
Prover->>Engine: prove_epoch(epoch, batch, db)
Engine->>SP1: (if SP1ProverEngine) prove_epoch(...)
SP1->>DB: (may fetch data for recursive proof)
SP1-->>Engine: returns (SuccinctProof, SuccinctProof)
Engine-->>Prover: returns (snark, stark)
Prover->>Engine: verify_proof(proof)
Engine-->>Prover: verification result
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (11)
⏰ 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). (4)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/node_types/prover/src/prover/tests/mod.rs (1)
52-64: Mock engine implementation is appropriate for testing.The mock returns default proofs for successful batch verification and always succeeds for
verify_proof.Note the TODO comment about mocking verifiable epochs - this may need attention if more sophisticated test scenarios are required.
The TODO on line 61 suggests potential future work. Would you like me to create an issue to track implementing more sophisticated VerifiableEpoch mocking?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/da/Cargo.toml(1 hunks)crates/da/src/lib.rs(6 hunks)crates/node_types/lightclient/src/lib.rs(1 hunks)crates/node_types/lightclient/src/lightclient.rs(8 hunks)crates/node_types/lightclient/src/tests/mod.rs(3 hunks)crates/node_types/prover/Cargo.toml(1 hunks)crates/node_types/prover/src/prover/mod.rs(3 hunks)crates/node_types/prover/src/prover/tests/mod.rs(8 hunks)crates/node_types/prover/src/prover_engine/engine.rs(1 hunks)crates/node_types/prover/src/prover_engine/mod.rs(1 hunks)crates/node_types/prover/src/prover_engine/sp1_prover.rs(6 hunks)crates/node_types/prover/src/sequencer.rs(2 hunks)crates/node_types/prover/src/syncer.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Follow Rust Coding Standards Use rustfmt with project settings (merge...
**/*.rs: Follow Rust Coding Standards
Use rustfmt with project settings (merge_imports=true, imports_granularity="Crate", max_width=100)
Error handling: Use Result types with descriptive error messages
Naming: follow Rust conventions (snake_case for functions/variables, CamelCase for types)
Documentation: Add comments for public APIs and complex logic
File organization: Group related functionality in modules
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
crates/node_types/prover/src/prover_engine/mod.rscrates/node_types/lightclient/src/lib.rscrates/node_types/prover/src/syncer.rscrates/node_types/prover/src/prover_engine/engine.rscrates/node_types/prover/src/sequencer.rscrates/node_types/prover/src/prover/mod.rscrates/node_types/prover/src/prover/tests/mod.rscrates/da/src/lib.rscrates/node_types/prover/src/prover_engine/sp1_prover.rscrates/node_types/lightclient/src/tests/mod.rscrates/node_types/lightclient/src/lightclient.rs
🧠 Learnings (14)
📓 Common learnings
Learnt from: distractedm1nd
PR: deltadevsde/prism#146
File: crates/common/src/tree.rs:114-126
Timestamp: 2024-11-01T07:52:07.324Z
Learning: In `UpdateProof::verify` within `crates/common/src/tree.rs`, the `old_hashchain` and `hashchain_after_update` are different hashchains, so caching their serialized values is not feasible.
crates/da/Cargo.toml (3)
Learnt from: jns-ps
PR: deltadevsde/prism#165
File: crates/serde/src/binary.rs:9-20
Timestamp: 2024-12-06T06:03:33.008Z
Learning: The project uses bincode version 1.3.3, so suggestions should be compatible with that version.
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/*.rs : Use rustfmt with project settings (merge_imports=true, imports_granularity="Crate", max_width=100)
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/*.rs : Follow Rust Coding Standards
crates/node_types/prover/Cargo.toml (1)
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
crates/node_types/prover/src/prover_engine/mod.rs (2)
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/*.rs : File organization: Group related functionality in modules
crates/node_types/lightclient/src/lib.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#193
File: crates/node_types/lightclient/src/lightclient.rs:68-74
Timestamp: 2025-01-08T09:35:23.121Z
Learning: In the LightClient implementation, signature verification failures should trigger a panic as clients must not proceed with potentially compromised data. This is a security measure rather than an error handling concern.
crates/node_types/prover/src/syncer.rs (3)
Learnt from: sebasti810
PR: deltadevsde/prism#342
File: crates/cli/src/main.rs:152-155
Timestamp: 2025-06-17T07:14:03.223Z
Learning: In the Prism codebase, the verifying_key field in NetworkConfig was changed from Option<VerifyingKey> to VerifyingKey because it's always required by all node types (LightClient, Prover, FullNode) and a sensible default (generated key) can be provided when not explicitly configured.
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
Learnt from: distractedm1nd
PR: deltadevsde/prism#146
File: crates/common/src/tree.rs:114-126
Timestamp: 2024-11-01T07:52:07.324Z
Learning: In `UpdateProof::verify` within `crates/common/src/tree.rs`, the `old_hashchain` and `hashchain_after_update` are different hashchains, so caching their serialized values is not feasible.
crates/node_types/prover/src/prover_engine/engine.rs (2)
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
Learnt from: distractedm1nd
PR: deltadevsde/prism#146
File: crates/common/src/tree.rs:114-126
Timestamp: 2024-11-01T07:52:07.324Z
Learning: In `UpdateProof::verify` within `crates/common/src/tree.rs`, the `old_hashchain` and `hashchain_after_update` are different hashchains, so caching their serialized values is not feasible.
crates/node_types/prover/src/sequencer.rs (2)
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
Learnt from: distractedm1nd
PR: deltadevsde/prism#146
File: crates/common/src/tree.rs:114-126
Timestamp: 2024-11-01T07:52:07.324Z
Learning: In `UpdateProof::verify` within `crates/common/src/tree.rs`, the `old_hashchain` and `hashchain_after_update` are different hashchains, so caching their serialized values is not feasible.
crates/node_types/prover/src/prover/mod.rs (2)
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
Learnt from: sebasti810
PR: deltadevsde/prism#342
File: crates/cli/src/main.rs:152-155
Timestamp: 2025-06-17T07:14:03.223Z
Learning: In the Prism codebase, the verifying_key field in NetworkConfig was changed from Option<VerifyingKey> to VerifyingKey because it's always required by all node types (LightClient, Prover, FullNode) and a sensible default (generated key) can be provided when not explicitly configured.
crates/node_types/prover/src/prover/tests/mod.rs (4)
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/common/src/test_ops.rs:27-44
Timestamp: 2024-10-29T08:34:32.445Z
Learning: In `crates/common/src/test_ops.rs`, methods in test code are deliberately kept short for brevity, even if this results in abbreviated method names.
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/{tests,**/*_test}.rs : Include tests for new functionality
Learnt from: sebasti810
PR: deltadevsde/prism#342
File: crates/cli/src/main.rs:152-155
Timestamp: 2025-06-17T07:14:03.223Z
Learning: In the Prism codebase, the verifying_key field in NetworkConfig was changed from Option<VerifyingKey> to VerifyingKey because it's always required by all node types (LightClient, Prover, FullNode) and a sensible default (generated key) can be provided when not explicitly configured.
crates/da/src/lib.rs (3)
Learnt from: distractedm1nd
PR: deltadevsde/prism#146
File: crates/common/src/tree.rs:114-126
Timestamp: 2024-11-01T07:52:07.324Z
Learning: In `UpdateProof::verify` within `crates/common/src/tree.rs`, the `old_hashchain` and `hashchain_after_update` are different hashchains, so caching their serialized values is not feasible.
Learnt from: distractedm1nd
PR: deltadevsde/prism#146
File: crates/common/src/hashchain.rs:164-174
Timestamp: 2024-11-01T07:51:25.629Z
Learning: In `crates/common/src/hashchain.rs`, extracting duplicated key validation logic between different operation types is not feasible without refactoring the underlying enum structs to use the same type.
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
crates/node_types/prover/src/prover_engine/sp1_prover.rs (4)
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
Learnt from: distractedm1nd
PR: deltadevsde/prism#146
File: crates/common/src/tree.rs:114-126
Timestamp: 2024-11-01T07:52:07.324Z
Learning: In `UpdateProof::verify` within `crates/common/src/tree.rs`, the `old_hashchain` and `hashchain_after_update` are different hashchains, so caching their serialized values is not feasible.
Learnt from: distractedm1nd
PR: deltadevsde/prism#150
File: crates/common/src/keys.rs:27-31
Timestamp: 2024-10-29T15:02:10.475Z
Learning: In the Rust codebase of `prism`, within the `VerifyingKey::as_bytes` method in `crates/common/src/keys.rs`, returning a `Vec<u8>` is necessary because the underlying key types (`Secp256k1VerifyingKey` and `Ed25519VerifyingKey`) provide their byte representations by value. Returning a `&[u8]` would involve borrowing from a temporary value, which is not feasible.
Learnt from: sebasti810
PR: deltadevsde/prism#342
File: crates/cli/src/main.rs:152-155
Timestamp: 2025-06-17T07:14:03.223Z
Learning: In the Prism codebase, the verifying_key field in NetworkConfig was changed from Option<VerifyingKey> to VerifyingKey because it's always required by all node types (LightClient, Prover, FullNode) and a sensible default (generated key) can be provided when not explicitly configured.
crates/node_types/lightclient/src/tests/mod.rs (3)
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/common/src/test_ops.rs:27-44
Timestamp: 2024-10-29T08:34:32.445Z
Learning: In `crates/common/src/test_ops.rs`, methods in test code are deliberately kept short for brevity, even if this results in abbreviated method names.
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/{tests,**/*_test}.rs : Include tests for new functionality
crates/node_types/lightclient/src/lightclient.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#193
File: crates/node_types/lightclient/src/lightclient.rs:68-74
Timestamp: 2025-01-08T09:35:23.121Z
Learning: In the LightClient implementation, signature verification failures should trigger a panic as clients must not proceed with potentially compromised data. This is a security measure rather than an error handling concern.
🧬 Code Graph Analysis (2)
crates/node_types/prover/src/prover_engine/engine.rs (1)
crates/node_types/prover/src/prover_engine/sp1_prover.rs (3)
verification_keys(37-49)prove_epoch(51-64)verify_proof(67-75)
crates/node_types/lightclient/src/lightclient.rs (4)
crates/da/src/events.rs (3)
recv(188-190)new(111-114)send(171-179)crates/node_types/prover/src/prover/mod.rs (2)
run(213-276)new(122-130)crates/node_types/wasm-lightclient/src/worker.rs (2)
run(90-122)new(52-88)crates/telemetry/src/metrics_registry.rs (2)
new(32-59)get_metrics(103-111)
🔇 Additional comments (29)
crates/da/Cargo.toml (1)
37-37: LGTM: Clean dependency additionThe addition of
bincodeas a workspace dependency is appropriate for the new proof serialization requirements. Using workspace dependencies ensures version consistency across the codebase.crates/node_types/prover/Cargo.toml (1)
34-34: LGTM: Supports the mockable ProverEngine objectiveThe addition of
mockallas a workspace dependency is appropriate for creating mock implementations of theProverEnginetrait, which aligns perfectly with the PR objective of speeding up CI tests.crates/node_types/prover/src/prover_engine/mod.rs (1)
1-2: LGTM: Clean module organizationThe module declarations follow Rust conventions and provide good separation of concerns between the trait definition (
engine) and its implementation (sp1_prover). This structure supports the abstraction needed for mockable ProverEngine.crates/node_types/prover/src/syncer.rs (2)
11-11: LGTM: Import path reflects new module structureThe updated import path correctly reflects the new module organization where
ProverEngineis now in theenginesubmodule.
23-23: LGTM: Trait object enables dependency injectionThe change from
Arc<ProverEngine>toArc<dyn ProverEngine>enables dynamic dispatch and dependency injection, which is essential for the mockable ProverEngine objective. This allows substituting mock implementations in tests while maintaining the same interface.Also applies to: 34-34
crates/node_types/lightclient/src/tests/mod.rs (4)
147-150: Good addition of synchronization point.Adding explicit wait for
BackwardsSyncStartedevent ensures the test properly synchronizes with the async backward sync flow before proceeding.
302-302: Function rename looks good.Removing the
_v1suffix simplifies the test name.
305-305: Proper event channel subscription setup.Creating a separate subscriber for DA layer events allows isolated event handling.
336-339: Consistent synchronization pattern.Waiting for
BackwardsSyncStartedon the DA layer subscriber ensures proper test synchronization with the refactored event-driven backward sync flow.crates/node_types/prover/src/prover_engine/engine.rs (1)
1-33: Well-designed trait abstraction for the prover engine.The
ProverEnginetrait provides a clean abstraction with:
- Proper async support via
async_trait- Mock generation via
automockfor testing- Clear method signatures and documentation
- Appropriate trait bounds (
Send + Sync)- Test-only method properly gated with
#[cfg(test)]This design successfully enables the mockable prover engine objective stated in the PR.
crates/node_types/prover/src/sequencer.rs (3)
17-17: Import path correctly updated.The import now references the trait from its new module location.
117-117: Proper use of trait object for flexibility.The parameter type change to
Arc<dyn ProverEngine>enables injection of different prover engine implementations.
124-131: Correct adaptation to new proof structure.The code properly destructures the
(snark, stark)tuple and uses the newSuccinctProoffields inFinalizedEpoch.crates/node_types/prover/src/prover/mod.rs (4)
21-21: Import correctly includes both trait and implementation.The import provides access to both the
ProverEnginetrait andSP1ProverEngineimplementation.
113-113: Field type properly abstracted to trait object.Using
Arc<dyn ProverEngine>enables runtime polymorphism for different prover implementations.
128-130: Backward compatibility preserved.The original
newconstructor maintains the API while internally using the new abstraction.
132-138: Clean dependency injection pattern.The
new_with_engineconstructor enables injection of anyProverEngineimplementation, supporting both production and test scenarios.crates/node_types/prover/src/prover/tests/mod.rs (4)
1-11: Imports correctly updated for mock support.The imports include
MockProverEngineand the necessary proof types for the new abstraction.
33-50: Test helper properly integrated with mock engine.The
create_test_proverfunction correctly creates and uses a mock engine vianew_with_engine.
33-33: Performance improvement in test setup.Reducing the duration from higher values to 50ms for
InMemoryDataAvailabilityLayerwill speed up test execution.Also applies to: 296-296, 370-371, 507-507
300-312: Consistent use of mock engine across all tests.All test setups have been properly updated to use
new_with_enginewith the mock implementation, successfully isolating tests from the real prover engine.Also applies to: 336-343, 377-389, 394-408, 514-522, 543-551
crates/node_types/prover/src/prover_engine/sp1_prover.rs (2)
35-76: LGTM! Clean trait implementation.The async trait implementation for
ProverEngineis well-structured with proper error handling and return types matching the newSuccinctProofabstraction.
97-121: Proof conversion looks correct.The method properly generates both SNARK and STARK proofs and converts them to the unified
SuccinctProoftype using thetry_into()conversion.crates/da/src/lib.rs (2)
30-49: Well-designed cross-platform proof abstraction.The
SuccinctProofstruct provides a clean abstraction for handling proofs across wasm32 and non-wasm32 environments. The documentation clearly explains the purpose and usage of each field.
127-130: Consistent migration to SuccinctProof fields.The
FinalizedEpochstruct and its methods have been properly updated to use the newSuccinctProofabstraction. The verification and commitment extraction logic correctly accesses the proof data from the unified structure.Also applies to: 171-189, 213-217
crates/node_types/lightclient/src/lightclient.rs (4)
31-68: Excellent macro design for event handling.The
select_with_cancellation!andawait_event!macros effectively reduce boilerplate while maintaining proper error handling and cancellation support. This improves code maintainability and consistency across event handling patterns.
129-150: Platform-appropriate concurrency patterns.Good use of
JoinSetfor non-wasm32 targets enabling better task management and graceful shutdown, while keeping the wasm32 implementation simple withjoin!.
256-329: Clean refactoring of backward sync logic.The conversion to a dedicated async method with bounded search depth and consistent event handling improves code clarity and maintainability. The use of the new event macros ensures proper cancellation handling throughout the sync process.
339-340: Good addition of cooperative yielding.The
yield_now()call prevents the backward search loop from monopolizing the executor on non-wasm32 targets, improving overall task scheduling fairness.
distractedm1nd
force-pushed
the
proverengine-mock
branch
from
bb28dc4 to
c0562a0
Compare
distractedm1nd
force-pushed
the
proverengine-mock
branch
from
98a0d9e to
e1910d9
Compare
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
Tests are now blazing fast, but coverage will take a hit. But it is unsustainable to be calling sp1
client::initandclient::executeSummary by CodeRabbit
New Features
Refactor
Tests
Chores