Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: compile time errors for conflicting flags, clean up flags in sdk #1805

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions crates/prover/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ required-features = ["export-tests"]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like how the prover crate now has a "cuda" and "network feature that don't do anything at all. On second thought, it's even kind of weird that the ProverMode lives in the prover crate in the first place. I feel like the prover crate should pretty much solely be concerned with generating different kinds of proofs on-device, and not really think about anything else.

cc @ctian1 @ratankaliani for second opinions

@leruaa why did you need to move the provermode here in the first place?


[features]
cuda = []
network = []
native-gnark = ["sp1-recursion-gnark-ffi/native"]
export-tests = ["dep:test-artifacts"]
debug = ["sp1-core-machine/debug"]
2 changes: 2 additions & 0 deletions crates/prover/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,9 @@ impl SP1Bn254ProofData {
pub enum ProverMode {
#[default]
Cpu,
#[cfg(feature = "cuda")]
Cuda,
#[cfg(feature = "network")]
Network,
#[value(skip)]
Mock,
Expand Down
6 changes: 5 additions & 1 deletion crates/sdk/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ alloy-signer = { version = "0.5", optional = true }
alloy-signer-local = { version = "0.5", optional = true }
alloy-primitives = { version = "0.8", optional = true }
backoff = { version = "0.4", features = ["tokio"], optional = true }
conflicting = "0.1.0"

[dev-dependencies]
test-artifacts = { workspace = true }
Expand All @@ -62,6 +63,7 @@ native-gnark = ["sp1-prover/native-gnark"]
# TODO: Once alloy has a 1.* release, we can likely remove this feature flag, as there will be less
# dependency resolution issues.
network = [
"sp1-prover/network",
"dep:prost",
"dep:alloy-sol-types",
"dep:tokio",
Expand All @@ -71,7 +73,9 @@ network = [
"dep:twirp",
"dep:reqwest-middleware",
]

network-v2 = [
"sp1-prover/network",
"dep:prost",
"dep:alloy-sol-types",
"dep:alloy-signer",
Expand All @@ -85,7 +89,7 @@ network-v2 = [
"dep:tonic",
"dep:backoff",
]
cuda = ["sp1-cuda"]
cuda = ["sp1-cuda", "sp1-prover/cuda"]

[build-dependencies]
vergen = { version = "8", default-features = false, features = [
Expand Down
23 changes: 11 additions & 12 deletions crates/sdk/src/install.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use cfg_if::cfg_if;
use std::path::PathBuf;

#[cfg(any(feature = "network", feature = "network-v2"))]
Expand Down Expand Up @@ -42,18 +41,18 @@ pub fn try_install_circuit_artifacts(artifacts_type: &str) -> PathBuf {
build_dir.display()
);
} else {
cfg_if! {
if #[cfg(any(feature = "network", feature = "network-v2"))] {
println!(
"[sp1] {} circuit artifacts for version {} do not exist at {}. downloading...",
artifacts_type,
SP1_CIRCUIT_VERSION,
build_dir.display()
);
install_circuit_artifacts(build_dir.clone(), artifacts_type);
}
}
#[cfg(any(feature = "network", feature = "network-v2"))]
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: no action needed, but I think it's kind of weird how the network feature is, in the rest of the sdk, responsible for the network prover, but now it's responsible for accessing the network for getting artifacts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh youre saying that the artifacts actually shouldn't be gated because all modes mauy need them?

Not actually sure just kept the original functionality here

println!(
"[sp1] {} circuit artifacts for version {} do not exist at {}. downloading...",
artifacts_type,
SP1_CIRCUIT_VERSION,
build_dir.display()
);
install_circuit_artifacts(build_dir.clone(), artifacts_type);
};
}

build_dir
}

Expand Down
96 changes: 27 additions & 69 deletions crates/sdk/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@
SP1VerifyingKey,
};

#[cfg(any(feature = "network", feature = "network-v2"))]
use conflicting::conflicting;

/// A client for interacting with SP1.
pub struct ProverClient {
/// The underlying prover implementation.
Expand All @@ -71,6 +74,7 @@
/// std::env::set_var("SP1_PROVER", "local");
/// let client = ProverClient::new();
/// ```
#[deprecated = "Use `ProverClient::builder` instead"]
pub fn new() -> Self {
#[allow(unreachable_code)]
match env::var("SP1_PROVER").unwrap_or("local".to_string()).to_lowercase().as_str() {
Expand Down Expand Up @@ -185,18 +189,18 @@
///
/// let client = ProverClient::network(private_key, rpc_url, skip_simulation);
/// ```
#[cfg(any(feature = "network", feature = "network-v2"))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to fix these CI failures before merging. Maybe we need to check sp1-sdk using both the network and network-v2 features ...

Honestly, if we deprecate network v1 in the next couple months maybe we should just deal with these sins until then.

pub fn network(private_key: String, rpc_url: Option<String>, skip_simulation: bool) -> Self {
cfg_if! {
if #[cfg(feature = "network-v2")] {
conflicting! {

Check failure on line 194 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Test (ARM)

"network" conflicts with "network-v2"

Check failure on line 194 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Test (ARM)

"network-v2" conflicts with "network"

Check failure on line 194 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Formatting & Clippy

"network" conflicts with "network-v2"

Check failure on line 194 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Formatting & Clippy

"network-v2" conflicts with "network"

Check failure on line 194 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Test (x86-64)

"network" conflicts with "network-v2"

Check failure on line 194 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Test (x86-64)

"network-v2" conflicts with "network"
"network" => {
Self {

Check failure on line 196 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Test (ARM)

mismatched types

Check failure on line 196 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Formatting & Clippy

mismatched types

Check failure on line 196 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Test (x86-64)

mismatched types
prover: Box::new(NetworkProverV2::new(&private_key, rpc_url, skip_simulation)),
prover: Box::new(NetworkProverV1::new(&private_key, rpc_url, skip_simulation)),
}
} else if #[cfg(feature = "network")] {
},
"network-v2" => {
Self {
prover: Box::new(NetworkProverV1::new(&private_key, rpc_url, skip_simulation)),
prover: Box::new(NetworkProverV2::new(&private_key, rpc_url, skip_simulation)),
}
} else {
panic!("network feature is not enabled")
}
}
}
Expand Down Expand Up @@ -315,16 +319,19 @@

impl Default for ProverClient {
fn default() -> Self {
Self::new()
Self::cpu()
}
}

/// Builder type for [`ProverClient`].
#[derive(Debug, Default)]
pub struct ProverClientBuilder {
mode: Option<ProverMode>,
#[cfg(any(feature = "network", feature = "network-v2"))]
private_key: Option<String>,
#[cfg(any(feature = "network", feature = "network-v2"))]
rpc_url: Option<String>,
#[cfg(any(feature = "network", feature = "network-v2"))]
skip_simulation: bool,
}

Expand All @@ -336,18 +343,21 @@
}

/// Sets the private key.
#[cfg(any(feature = "network", feature = "network-v2"))]
pub fn private_key(mut self, private_key: String) -> Self {
self.private_key = Some(private_key);
self
}

/// Sets the RPC URL.
#[cfg(any(feature = "network", feature = "network-v2"))]
pub fn rpc_url(mut self, rpc_url: String) -> Self {
self.rpc_url = Some(rpc_url);
self
}

/// Skips simulation.
#[cfg(any(feature = "network", feature = "network-v2"))]
pub fn skip_simulation(mut self) -> Self {
self.skip_simulation = true;
self
Expand All @@ -357,29 +367,22 @@
pub fn build(self) -> ProverClient {
match self.mode.expect("The prover mode is required") {
ProverMode::Cpu => ProverClient::cpu(),
ProverMode::Cuda => {
cfg_if! {
if #[cfg(feature = "cuda")] {
ProverClient::cuda()
} else {
panic!("cuda feature is not enabled")
}
}
}
#[cfg(feature = "cuda")]
ProverMode::Cuda => ProverClient::cuda(),
#[cfg(any(feature = "network", feature = "network-v2"))]
ProverMode::Network => {
let private_key = self.private_key.expect("The private key is required");

cfg_if! {
if #[cfg(feature = "network-v2")] {
conflicting! {

Check failure on line 376 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Test (ARM)

"network" conflicts with "network-v2"

Check failure on line 376 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Test (ARM)

"network-v2" conflicts with "network"

Check failure on line 376 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Formatting & Clippy

"network" conflicts with "network-v2"

Check failure on line 376 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Formatting & Clippy

"network-v2" conflicts with "network"

Check failure on line 376 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Test (x86-64)

"network" conflicts with "network-v2"

Check failure on line 376 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Test (x86-64)

"network-v2" conflicts with "network"
"network" => {
ProverClient {

Check failure on line 378 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Test (ARM)

mismatched types

Check failure on line 378 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Formatting & Clippy

mismatched types

Check failure on line 378 in crates/sdk/src/lib.rs

View workflow job for this annotation

GitHub Actions / Test (x86-64)

mismatched types
prover: Box::new(NetworkProverV2::new(&private_key, self.rpc_url, self.skip_simulation)),
prover: Box::new(NetworkProverV1::new(&private_key, self.rpc_url, self.skip_simulation)),
}
} else if #[cfg(feature = "network")] {
},
"network-v2" => {
ProverClient {
prover: Box::new(NetworkProverV1::new(&private_key, self.rpc_url, self.skip_simulation)),
prover: Box::new(NetworkProverV2::new(&private_key, self.rpc_url, self.skip_simulation)),
}
} else {
panic!("network feature is not enabled")
}
}
}
Expand All @@ -388,51 +391,6 @@
}
}

/// Builder type for network prover.
#[cfg(any(feature = "network", feature = "network-v2"))]
#[derive(Debug, Default)]
pub struct NetworkProverBuilder {
private_key: Option<String>,
rpc_url: Option<String>,
skip_simulation: bool,
}

impl NetworkProverBuilder {
/// Sets the private key.
pub fn private_key(mut self, private_key: String) -> Self {
self.private_key = Some(private_key);
self
}

/// Sets the RPC URL.
pub fn rpc_url(mut self, rpc_url: String) -> Self {
self.rpc_url = Some(rpc_url);
self
}

/// Skips simulation.
pub fn skip_simulation(mut self) -> Self {
self.skip_simulation = true;
self
}

/// Creates a new [NetworkProverV1].
#[cfg(feature = "network")]
pub fn build(self) -> NetworkProverV1 {
let private_key = self.private_key.expect("The private key is required");

NetworkProverV1::new(&private_key, self.rpc_url, self.skip_simulation)
}

/// Creates a new [NetworkProverV2].
#[cfg(feature = "network-v2")]
pub fn build_v2(self) -> NetworkProverV2 {
let private_key = self.private_key.expect("The private key is required");

NetworkProverV2::new(&private_key, self.rpc_url, self.skip_simulation)
}
}

/// Utility method for blocking on an async function.
///
/// If we're already in a tokio runtime, we'll block in place. Otherwise, we'll create a new
Expand Down
8 changes: 1 addition & 7 deletions crates/sdk/src/network-v2/prover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ use std::time::{Duration, Instant};
use crate::{
network_v2::client::NetworkClient,
network_v2::proto::network::{ProofMode, ProofStatus, ProofStrategy},
NetworkProverBuilder, Prover, SP1Context, SP1ProofKind, SP1ProofWithPublicValues,
SP1ProvingKey, SP1VerifyingKey,
Prover, SP1Context, SP1ProofKind, SP1ProofWithPublicValues, SP1ProvingKey, SP1VerifyingKey,
};
use anyhow::Result;
use backoff::{future::retry, ExponentialBackoff};
Expand Down Expand Up @@ -41,11 +40,6 @@ impl NetworkProver {
Self { client, local_prover, skip_simulation }
}

/// Creates a new network prover builder. See [`NetworkProverBuilder`] for more details.
pub fn builder() -> NetworkProverBuilder {
NetworkProverBuilder::default()
}

/// Requests a proof from the prover network, returning the request ID.
pub async fn request_proof(
&self,
Expand Down
8 changes: 1 addition & 7 deletions crates/sdk/src/network/prover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ use crate::{
client::NetworkClient,
proto::network::{ProofMode, ProofStatus},
},
NetworkProverBuilder, Prover, SP1Context, SP1ProofKind, SP1ProofWithPublicValues,
SP1ProvingKey, SP1VerifyingKey,
Prover, SP1Context, SP1ProofKind, SP1ProofWithPublicValues, SP1ProvingKey, SP1VerifyingKey,
};
use anyhow::Result;
use sp1_core_machine::io::SP1Stdin;
Expand Down Expand Up @@ -39,11 +38,6 @@ impl NetworkProver {
Self { client: NetworkClient::new(private_key, rpc_url), local_prover, skip_simulation }
}

/// Creates a new network prover builder. See [`NetworkProverBuilder`] for more details.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think either @ctian1 or @ratankaliani needs this I don't remember who

pub fn builder() -> NetworkProverBuilder {
NetworkProverBuilder::default()
}

/// Requests a proof from the prover network, returning the proof ID.
pub async fn request_proof(
&self,
Expand Down
Loading