From e77129edeaebba5617e6d8b4a4ab0bd342a4adaf Mon Sep 17 00:00:00 2001 From: Parth Desai Date: Fri, 9 Feb 2024 17:55:26 +0530 Subject: [PATCH] addressing low hanging review comments + minor cleanups --- Cargo.lock | 1 - pulsar/src/config.rs | 9 +-- pulsar/src/utils.rs | 4 -- sdk/dsn/src/builder.rs | 11 ---- sdk/farmer/src/lib.rs | 30 +++++---- sdk/node/src/builder.rs | 12 ++-- sdk/node/src/domains/builder.rs | 2 +- sdk/node/src/lib.rs | 24 ++++--- sdk/subspace-sdk/examples/mini-farmer.rs | 15 +---- sdk/substrate/Cargo.toml | 1 - sdk/substrate/src/lib.rs | 83 ++++-------------------- 11 files changed, 55 insertions(+), 137 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0fdba8dd..e8bcd124 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10057,7 +10057,6 @@ dependencies = [ "sc-storage-monitor", "sdk-utils", "serde", - "serde_json", "sp-runtime", "subspace-service", ] diff --git a/pulsar/src/config.rs b/pulsar/src/config.rs index 14c1998e..c5b8ac74 100644 --- a/pulsar/src/config.rs +++ b/pulsar/src/config.rs @@ -11,7 +11,7 @@ use subspace_sdk::node::{DomainConfigBuilder, DsnBuilder, NetworkBuilder, Node, use subspace_sdk::{chain_spec, ByteSize, FarmDescription, PublicKey}; use tracing::instrument; -use crate::utils::{provider_storage_dir_getter, IntoEyre}; +use crate::utils::IntoEyre; /// defaults for the user config file pub(crate) const DEFAULT_FARM_SIZE: ByteSize = ByteSize::gb(2); @@ -52,10 +52,7 @@ impl NodeConfig { ChainConfig::Gemini3h => { let mut node = Node::gemini_3h() .network(NetworkBuilder::gemini_3h().name(name)) - .dsn( - DsnBuilder::gemini_3h() - .provider_storage_path(provider_storage_dir_getter()), - ) + .dsn(DsnBuilder::gemini_3h()) .sync_from_dsn(true) .enable_subspace_block_relay(true); if enable_domains { @@ -77,7 +74,7 @@ impl NodeConfig { ChainConfig::DevNet => { let mut node = Node::devnet() .network(NetworkBuilder::devnet().name(name)) - .dsn(DsnBuilder::devnet().provider_storage_path(provider_storage_dir_getter())) + .dsn(DsnBuilder::devnet()) .sync_from_dsn(true) .enable_subspace_block_relay(true); if enable_domains { diff --git a/pulsar/src/utils.rs b/pulsar/src/utils.rs index 6cef31bc..eea3ff7c 100644 --- a/pulsar/src/utils.rs +++ b/pulsar/src/utils.rs @@ -149,10 +149,6 @@ pub(crate) fn node_directory_getter() -> PathBuf { data_dir_getter().join("node") } -pub(crate) fn provider_storage_dir_getter() -> PathBuf { - node_directory_getter().join("provider-storage") -} - fn data_dir_getter() -> PathBuf { dirs::data_dir().expect("data folder must be present in every major OS").join("pulsar") } diff --git a/sdk/dsn/src/builder.rs b/sdk/dsn/src/builder.rs index 3286d82f..12393ad8 100644 --- a/sdk/dsn/src/builder.rs +++ b/sdk/dsn/src/builder.rs @@ -84,10 +84,6 @@ pub struct PendingOutConnections(#[derivative(Default(value = "150"))] pub u32); #[builder(pattern = "immutable", build_fn(private, name = "_build"), name = "DsnBuilder")] #[non_exhaustive] pub struct Dsn { - /// Listen on some address for other nodes - #[builder(default, setter(into, strip_option))] - #[serde(default, skip_serializing_if = "sdk_utils::is_default")] - pub provider_storage_path: Option, /// Listen on some address for other nodes #[builder(default, setter(into))] #[serde(default, skip_serializing_if = "sdk_utils::is_default")] @@ -125,11 +121,6 @@ pub struct Dsn { #[builder(setter(into), default)] #[serde(default, skip_serializing_if = "sdk_utils::is_default")] pub pending_out_connections: PendingOutConnections, - /// Defines target total (in and out) connection number for DSN that - /// should be maintained. - #[builder(setter(into), default)] - #[serde(default, skip_serializing_if = "sdk_utils::is_default")] - pub target_connections: TargetConnections, } sdk_utils::generate_builder!(Dsn); @@ -237,14 +228,12 @@ impl Dsn { listen_addresses, reserved_nodes, allow_non_global_addresses_in_dht, - provider_storage_path: _, in_connections: InConnections(max_established_incoming_connections), out_connections: OutConnections(max_established_outgoing_connections), pending_in_connections: PendingInConnections(max_pending_incoming_connections), pending_out_connections: PendingOutConnections(max_pending_outgoing_connections), boot_nodes, external_addresses, - .. } = self; let bootstrap_nodes = boot_nodes.into_iter().map(Into::into).collect::>(); diff --git a/sdk/farmer/src/lib.rs b/sdk/farmer/src/lib.rs index 67f06dd4..609a742b 100644 --- a/sdk/farmer/src/lib.rs +++ b/sdk/farmer/src/lib.rs @@ -40,6 +40,7 @@ use subspace_farmer::utils::{ all_cpu_cores, create_plotting_thread_pool_manager, thread_pool_core_indices, }; use subspace_farmer::{Identity, KNOWN_PEERS_CACHE_SIZE}; +use subspace_farmer_components::plotting::PlottedSector; use subspace_farmer_components::sector::{sector_size, SectorMetadataChecksummed}; use subspace_networking::libp2p::kad::RecordKey; use subspace_networking::utils::multihash::ToMultihash; @@ -343,20 +344,12 @@ async fn create_readers_and_pieces( Ok(readers_and_pieces) } -fn handler_on_sector_update( - sector_update: &SectorUpdate, +fn handler_on_sector_plotted( + plotted_sector: &PlottedSector, + maybe_old_plotted_sector: &Option, disk_farm_index: usize, readers_and_pieces: Arc>>, ) { - let (plotted_sector, maybe_old_plotted_sector) = match sector_update { - SectorUpdate::Plotting(SectorPlottingDetails::Finished { - plotted_sector, - old_plotted_sector, - .. - }) => (plotted_sector, old_plotted_sector), - _ => return, - }; - let disk_farm_index = disk_farm_index .try_into() .expect("More than 256 farms are not supported, this is checked above already; qed"); @@ -632,8 +625,19 @@ impl Config { sector_plotting_handler_ids.push(single_disk_farm.on_sector_update(Arc::new( move |(_plotted_sector, sector_update)| { let _span_guard = span.enter(); - handler_on_sector_update( - sector_update, + + let (plotted_sector, maybe_old_plotted_sector) = match sector_update { + SectorUpdate::Plotting(SectorPlottingDetails::Finished { + plotted_sector, + old_plotted_sector, + .. + }) => (plotted_sector, old_plotted_sector), + _ => return, + }; + + handler_on_sector_plotted( + plotted_sector, + maybe_old_plotted_sector, disk_farm_index, readers_and_pieces.clone(), ) diff --git a/sdk/node/src/builder.rs b/sdk/node/src/builder.rs index 9c361ccc..32ba0ebe 100644 --- a/sdk/node/src/builder.rs +++ b/sdk/node/src/builder.rs @@ -8,8 +8,7 @@ use derive_more::{Deref, DerefMut, Display, From}; use sc_service::BlocksPruning; use sdk_dsn::{Dsn, DsnBuilder}; use sdk_substrate::{ - Base, BaseBuilder, NetworkBuilder, OffchainWorkerBuilder, PruningMode, Role, RpcBuilder, - StorageMonitor, + Base, BaseBuilder, NetworkBuilder, PruningMode, Role, RpcBuilder, StorageMonitor, }; use sdk_utils::ByteSize; use serde::{Deserialize, Serialize}; @@ -90,7 +89,7 @@ pub struct Config { /// Proof of time entropy #[builder(setter(into), default)] #[serde(default, skip_serializing_if = "sdk_utils::is_default")] - pub pot_external_entropy: Option>, + pub pot_external_entropy: Option, } impl Config { @@ -119,7 +118,6 @@ impl Builder { .network(NetworkBuilder::dev()) .dsn(DsnBuilder::dev()) .rpc(RpcBuilder::dev()) - .offchain_worker(OffchainWorkerBuilder::dev()) } /// Gemini 3g configuration @@ -128,9 +126,8 @@ impl Builder { .network(NetworkBuilder::gemini_3h()) .dsn(DsnBuilder::gemini_3h()) .rpc(RpcBuilder::gemini_3h()) - .offchain_worker(OffchainWorkerBuilder::gemini_3h()) .role(Role::Authority) - .state_pruning(PruningMode::ArchiveAll) + .state_pruning(PruningMode::ArchiveCanonical) .blocks_pruning(BlocksPruning::Some(256)) } @@ -140,9 +137,8 @@ impl Builder { .network(NetworkBuilder::devnet()) .dsn(DsnBuilder::devnet()) .rpc(RpcBuilder::devnet()) - .offchain_worker(OffchainWorkerBuilder::devnet()) .role(Role::Authority) - .state_pruning(PruningMode::ArchiveAll) + .state_pruning(PruningMode::ArchiveCanonical) .blocks_pruning(BlocksPruning::Some(256)) } diff --git a/sdk/node/src/domains/builder.rs b/sdk/node/src/domains/builder.rs index 1a14b6c9..8600218d 100644 --- a/sdk/node/src/domains/builder.rs +++ b/sdk/node/src/domains/builder.rs @@ -128,7 +128,7 @@ impl DomainConfigBuilder { /// Dev chain configuration pub fn dev() -> Self { - Self::new().chain_id("dev").domain_id(DomainId::new(0)).dev_key_seed("//Alice") + Self::new().chain_id("dev").domain_id(DomainId::new(0)) } /// Gemini 3g configuration diff --git a/sdk/node/src/lib.rs b/sdk/node/src/lib.rs index 496f917b..032d3fd7 100644 --- a/sdk/node/src/lib.rs +++ b/sdk/node/src/lib.rs @@ -30,6 +30,7 @@ use sc_utils::mpsc::tracing_unbounded; use sdk_dsn::{DsnOptions, DsnShared}; use sdk_traits::Farmer; use sdk_utils::{DestructorSet, MultiaddrWithPeerId, PublicKey, TaskOutput}; +use serde_json::Value; use sp_consensus::SyncOracle; use sp_consensus_subspace::digests::PreDigest; use sp_core::traits::SpawnEssentialNamed; @@ -70,28 +71,31 @@ const SEGMENT_HEADERS_NUMBER_LIMIT: u64 = MAX_SEGMENT_HEADERS_PER_REQUEST as u64 fn pot_external_entropy( consensus_chain_config: &Configuration, - config_pot_external_entropy: Option>, + maybe_pot_external_entropy: Option, ) -> Result, sc_service::Error> { let maybe_chain_spec_pot_external_entropy = consensus_chain_config .chain_spec .properties() .get("potExternalEntropy") - .map(|d| serde_json::from_value(d.clone())) - .transpose() - .map_err(|error| { - sc_service::Error::Other(format!("Failed to decode PoT initial key: {error:?}")) - })? - .flatten(); + .map(|d| match d.clone() { + Value::String(s) => Ok(s), + Value::Null => Ok(String::new()), + _ => Err(sc_service::Error::Other("Failed to decode PoT initial key".to_string())), + }) + .transpose()?; if maybe_chain_spec_pot_external_entropy.is_some() - && config_pot_external_entropy.is_some() - && maybe_chain_spec_pot_external_entropy != config_pot_external_entropy + && maybe_pot_external_entropy.is_some() + && maybe_chain_spec_pot_external_entropy != maybe_pot_external_entropy { tracing::warn!( "--pot-external-entropy CLI argument was ignored due to chain spec having a different \ explicit value" ); } - Ok(maybe_chain_spec_pot_external_entropy.or(config_pot_external_entropy).unwrap_or_default()) + Ok(maybe_chain_spec_pot_external_entropy + .or(maybe_pot_external_entropy) + .unwrap_or_default() + .into_bytes()) } impl Config { diff --git a/sdk/subspace-sdk/examples/mini-farmer.rs b/sdk/subspace-sdk/examples/mini-farmer.rs index ae3f78e9..d94b74dd 100644 --- a/sdk/subspace-sdk/examples/mini-farmer.rs +++ b/sdk/subspace-sdk/examples/mini-farmer.rs @@ -76,18 +76,9 @@ async fn main() -> anyhow::Result<()> { let node_dir = base_path.join("node"); let node = match chain { - Chain::Gemini3h => Node::gemini_3h().dsn( - subspace_sdk::node::DsnBuilder::gemini_3h() - .provider_storage_path(node_dir.join("provider_storage")), - ), - Chain::Devnet => Node::devnet().dsn( - subspace_sdk::node::DsnBuilder::devnet() - .provider_storage_path(node_dir.join("provider_storage")), - ), - Chain::Dev => Node::dev().dsn( - subspace_sdk::node::DsnBuilder::dev() - .provider_storage_path(node_dir.join("provider_storage")), - ), + Chain::Gemini3h => Node::gemini_3h().dsn(subspace_sdk::node::DsnBuilder::gemini_3h()), + Chain::Devnet => Node::devnet().dsn(subspace_sdk::node::DsnBuilder::devnet()), + Chain::Dev => Node::dev().dsn(subspace_sdk::node::DsnBuilder::dev()), } .role(node::Role::Authority); diff --git a/sdk/substrate/Cargo.toml b/sdk/substrate/Cargo.toml index 7e3dc21a..7e7023eb 100644 --- a/sdk/substrate/Cargo.toml +++ b/sdk/substrate/Cargo.toml @@ -18,6 +18,5 @@ sc-state-db = { version = "0.10.0-dev", git = "https://github.com/subspace/polka sc-storage-monitor = { version = "0.1.0", git = "https://github.com/subspace/polkadot-sdk", rev = "d6b500960579d73c43fc4ef550b703acfa61c4c8", default-features = false } sdk-utils = { path = "../utils" } serde = { version = "1", features = ["derive"] } -serde_json = "1" sp-runtime = { version = "24.0.0", git = "https://github.com/subspace/polkadot-sdk", rev = "d6b500960579d73c43fc4ef550b703acfa61c4c8" } subspace-service = { git = "https://github.com/subspace/subspace", rev = "e8ed377ecee4e1992ffb731d67cd04bf444e6382" } diff --git a/sdk/substrate/src/lib.rs b/sdk/substrate/src/lib.rs index e786627d..f4e87a1b 100644 --- a/sdk/substrate/src/lib.rs +++ b/sdk/substrate/src/lib.rs @@ -64,10 +64,6 @@ pub struct Base { #[builder(setter(into), default)] #[serde(default, skip_serializing_if = "sdk_utils::is_default")] pub network: Network, - /// Offchain worker settings - #[builder(setter(into), default)] - #[serde(default, skip_serializing_if = "sdk_utils::is_default")] - pub offchain_worker: OffchainWorker, /// Enable color for substrate informant #[builder(default)] #[serde(default, skip_serializing_if = "sdk_utils::is_default")] @@ -76,10 +72,6 @@ pub struct Base { #[builder(default)] #[serde(default, skip_serializing_if = "sdk_utils::is_default")] pub telemetry: Vec<(Multiaddr, u8)>, - /// Dev key seed - #[builder(setter(strip_option), default)] - #[serde(default, skip_serializing_if = "sdk_utils::is_default")] - pub dev_key_seed: Option, } #[doc(hidden)] @@ -124,14 +116,10 @@ macro_rules! derive_base { rpc: $crate::Rpc, /// Network settings network: $crate::Network, - /// Offchain worker settings - offchain_worker: $crate::OffchainWorker, /// Enable color for substrate informant informant_enable_color: bool, /// Additional telemetry endpoints telemetry: Vec<(sdk_utils::Multiaddr, u8)>, - /// Dev key seed - dev_key_seed: String }); } } @@ -152,6 +140,7 @@ impl Base { let Self { force_authoring, + role, blocks_pruning, state_pruning, impl_name: ImplName(impl_name), @@ -164,21 +153,18 @@ impl Base { cors: rpc_cors, methods: rpc_methods, max_subs_per_conn: rpc_max_subs_per_conn, - .. }, network, informant_enable_color, telemetry, - .. } = self; let base_path = BasePath::new(directory.as_ref()); let config_dir = base_path.config_dir(chain_spec.id()); let network = { - let Network { - listen_addresses, boot_nodes, force_synced, name, allow_private_ip, .. - } = network; + let Network { listen_addresses, boot_nodes, force_synced, name, allow_private_ip } = + network; let name = name.unwrap_or_else(|| { names::Generator::with_naming(names::Name::Numbered) .next() @@ -213,25 +199,8 @@ impl Base { } }; - // HACK: Tricky way to add extra endpoints as we can't push into telemetry - // endpoints let telemetry_endpoints = match chain_spec.telemetry_endpoints() { - Some(endpoints) => { - let Ok(serde_json::Value::Array(extra_telemetry)) = - serde_json::to_value(&telemetry) - else { - unreachable!("Will always return an array") - }; - let Ok(serde_json::Value::Array(telemetry)) = serde_json::to_value(endpoints) - else { - unreachable!("Will always return an array") - }; - - serde_json::from_value(serde_json::Value::Array( - telemetry.into_iter().chain(extra_telemetry).collect::>(), - )) - .expect("Serialization is always valid") - } + Some(endpoints) => endpoints.clone(), None => sc_service::config::TelemetryEndpoints::new( telemetry.into_iter().map(|(endpoint, n)| (endpoint.to_string(), n)).collect(), ) @@ -263,7 +232,7 @@ impl Base { informant_output_format: sc_informant::OutputFormat { enable_color: informant_enable_color, }, - farmer: self.role == Role::Authority, + farmer: role == Role::Authority, } .into() } @@ -297,14 +266,6 @@ pub struct Rpc { #[builder(default)] #[serde(default, skip_serializing_if = "sdk_utils::is_default")] pub methods: RpcMethods, - /// Maximum payload of a rpc request - #[builder(setter(strip_option), default)] - #[serde(default, skip_serializing_if = "sdk_utils::is_default")] - pub max_request_size: Option, - /// Maximum payload of a rpc request - #[builder(setter(strip_option), default)] - #[serde(default, skip_serializing_if = "sdk_utils::is_default")] - pub max_response_size: Option, /// Maximum allowed subscriptions per rpc connection #[builder(default)] #[serde(default, skip_serializing_if = "sdk_utils::is_default")] @@ -323,8 +284,6 @@ impl RpcBuilder { .addr(SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), port)) .port(port) .max_connections(100) - .max_request_size(10 * 1024) - .max_response_size(10 * 1024) .max_subs_per_conn(Some(100)) } @@ -356,18 +315,10 @@ impl RpcBuilder { #[builder(pattern = "owned", build_fn(private, name = "_build"), name = "NetworkBuilder")] #[non_exhaustive] pub struct Network { - /// Listen on some address for other nodes - #[builder(default)] - #[serde(default, skip_serializing_if = "sdk_utils::is_default")] - pub enable_mdns: bool, /// Listen on some address for other nodes #[builder(default)] #[serde(default, skip_serializing_if = "sdk_utils::is_default")] pub allow_private_ip: bool, - /// Allow non globals in network DHT - #[builder(default)] - #[serde(default, skip_serializing_if = "sdk_utils::is_default")] - pub allow_non_globals_in_dht: bool, /// Listen on some address for other nodes #[builder(default)] #[serde(default, skip_serializing_if = "sdk_utils::is_default")] @@ -384,10 +335,6 @@ pub struct Network { #[builder(setter(into, strip_option), default)] #[serde(default, skip_serializing_if = "sdk_utils::is_default")] pub name: Option, - /// Client id for telemetry (default is `{IMPL_NAME}/v{IMPL_VERSION}`) - #[builder(setter(into, strip_option), default)] - #[serde(default, skip_serializing_if = "sdk_utils::is_default")] - pub client_id: Option, } impl NetworkBuilder { @@ -398,22 +345,18 @@ impl NetworkBuilder { /// Gemini 3g configuration pub fn gemini_3h() -> Self { - Self::default() - .listen_addresses(vec![ - "/ip6/::/tcp/30333".parse().expect("hardcoded value is true"), - "/ip4/0.0.0.0/tcp/30333".parse().expect("hardcoded value is true"), - ]) - .enable_mdns(true) + Self::default().listen_addresses(vec![ + "/ip6/::/tcp/30333".parse().expect("hardcoded value is true"), + "/ip4/0.0.0.0/tcp/30333".parse().expect("hardcoded value is true"), + ]) } /// Dev network configuration pub fn devnet() -> Self { - Self::default() - .listen_addresses(vec![ - "/ip6/::/tcp/30333".parse().expect("hardcoded value is true"), - "/ip4/0.0.0.0/tcp/30333".parse().expect("hardcoded value is true"), - ]) - .enable_mdns(true) + Self::default().listen_addresses(vec![ + "/ip6/::/tcp/30333".parse().expect("hardcoded value is true"), + "/ip4/0.0.0.0/tcp/30333".parse().expect("hardcoded value is true"), + ]) } }