From d8eea5426032b89e886c42b6b95174bac9f52e15 Mon Sep 17 00:00:00 2001 From: iho Date: Thu, 9 Jul 2026 11:13:27 +0300 Subject: [PATCH 1/2] fix: fall back to defaults for missing config keys Allow omitting non-required keys such as log_to_file without parse failure. LoggingConfig, ServerConfig, StratumServerConfig, and ConfigMembers now use serde defaults aligned with Default impls. Addresses #3002. --- config/src/config.rs | 59 +++++++++++++++++++++++++++++++ config/src/types.rs | 5 ++- servers/src/common/types.rs | 69 +++++++++++++++++++++++++++++++++++-- util/src/logger.rs | 47 ++++++++++++++++++++----- 4 files changed, 168 insertions(+), 12 deletions(-) diff --git a/config/src/config.rs b/config/src/config.rs index 969085d9f2..b555b0ae69 100644 --- a/config/src/config.rs +++ b/config/src/config.rs @@ -590,3 +590,62 @@ fn test_api_secret_paths_config_file() { foreign_api_secret_exists, ); } + +/// Missing logging keys must fall back to defaults (issue #3002). +#[test] +fn test_logging_config_missing_keys_use_defaults() { + // Omitting log_to_file previously failed to parse; it must default to true. + let toml = r#" +[server] +db_root = "chain_data" +api_http_addr = "127.0.0.1:3413" + +[logging] +log_to_stdout = false +"#; + let members: ConfigMembers = toml::from_str(toml).expect("minimal config should parse"); + let logging = members.logging.expect("logging section present"); + assert_eq!(logging.log_to_stdout, false); + assert_eq!(logging.log_to_file, true); // default + assert_eq!(logging.log_file_append, true); // default + assert_eq!(format!("{:?}", logging.stdout_log_level), "Warn"); + assert_eq!(format!("{:?}", logging.file_log_level), "Info"); +} + +/// Entire [logging] section can be omitted. +#[test] +fn test_logging_section_optional() { + let toml = r#" +[server] +db_root = "chain_data" +api_http_addr = "127.0.0.1:3413" +"#; + let members: ConfigMembers = toml::from_str(toml).expect("config without logging should parse"); + assert!(members.logging.is_none()); + assert_eq!(members.server.db_root, "chain_data"); + assert_eq!(members.server.api_http_addr, "127.0.0.1:3413"); + // Other server fields use defaults + assert_eq!(members.server.run_tui, Some(true)); +} + +/// Partial [server.stratum_mining_config] uses stratum defaults. +#[test] +fn test_stratum_partial_config_defaults() { + let toml = r#" +[server] +db_root = "chain_data" + +[server.stratum_mining_config] +enable_stratum_server = true +"#; + let members: ConfigMembers = toml::from_str(toml).expect("partial stratum config should parse"); + let stratum = members + .server + .stratum_mining_config + .expect("stratum section"); + assert_eq!(stratum.enable_stratum_server, Some(true)); + assert_eq!(stratum.attempt_time_per_block, 15); + assert_eq!(stratum.minimum_share_difficulty, 1); + assert_eq!(stratum.wallet_listener_url, "http://127.0.0.1:3415"); + assert!(!stratum.burn_reward); +} diff --git a/config/src/types.rs b/config/src/types.rs index 2b71d99ce6..a397ae093f 100644 --- a/config/src/types.rs +++ b/config/src/types.rs @@ -87,12 +87,15 @@ pub struct GlobalConfig { /// internal state that we don't necessarily /// want serialised or deserialised #[derive(Clone, Debug, Serialize, Deserialize, PartialEq)] +#[serde(default)] pub struct ConfigMembers { /// Config file version (None == version 1) + #[serde(default)] pub config_file_version: Option, /// Server config #[serde(default)] pub server: ServerConfig, - /// Logging config + /// Logging config (optional section; missing keys use LoggingConfig defaults) + #[serde(default)] pub logging: Option, } diff --git a/servers/src/common/types.rs b/servers/src/common/types.rs index 445a57edab..c29551870f 100644 --- a/servers/src/common/types.rs +++ b/servers/src/common/types.rs @@ -138,23 +138,32 @@ impl Default for ChainValidationMode { /// Full server configuration, aggregating configurations required for the /// different components. +/// +/// Missing optional keys fall back to [`Default`] (see #3002). #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(default)] pub struct ServerConfig { /// Directory under which the rocksdb stores will be created + #[serde(default = "default_db_root")] pub db_root: String, /// Network address for the Rest API HTTP server. + #[serde(default = "default_api_http_addr")] pub api_http_addr: String, /// Location of secret for basic auth on Rest API HTTP and V2 Owner API server. + #[serde(default = "default_api_secret_path")] pub api_secret_path: Option, /// Location of secret for basic auth on v2 Foreign API server. + #[serde(default = "default_foreign_api_secret_path")] pub foreign_api_secret_path: Option, /// TLS certificate file + #[serde(default)] pub tls_certificate_file: Option, /// TLS certificate private key file + #[serde(default)] pub tls_certificate_key: Option, /// Setup the server for tests, testnet or mainnet @@ -170,23 +179,29 @@ pub struct ServerConfig { pub chain_validation_mode: ChainValidationMode, /// Whether this node is a full archival node or a fast-sync, pruned node + #[serde(default = "default_archive_mode")] pub archive_mode: Option, /// Whether to skip the sync timeout on startup /// (To assist testing on solo chains) + #[serde(default = "default_skip_sync_wait")] pub skip_sync_wait: Option, /// Whether to run the TUI /// if enabled, this will disable logging to stdout + #[serde(default = "default_run_tui")] pub run_tui: Option, /// Whether to run the test miner (internal, cuckoo 16) + #[serde(default = "default_run_test_miner")] pub run_test_miner: Option, /// Test miner wallet URL + #[serde(default)] pub test_miner_wallet_url: Option, /// Configuration for the peer-to-peer server + #[serde(default)] pub p2p_config: p2p::P2PConfig, /// Transaction pool configuration @@ -206,6 +221,31 @@ pub struct ServerConfig { pub webhook_config: WebHooksConfig, } +fn default_db_root() -> String { + "grin_chain".to_string() +} +fn default_api_http_addr() -> String { + "127.0.0.1:3413".to_string() +} +fn default_api_secret_path() -> Option { + Some(".api_secret".to_string()) +} +fn default_foreign_api_secret_path() -> Option { + Some(".foreign_api_secret".to_string()) +} +fn default_archive_mode() -> Option { + Some(false) +} +fn default_skip_sync_wait() -> Option { + Some(false) +} +fn default_run_tui() -> Option { + Some(true) +} +fn default_run_test_miner() -> Option { + Some(false) +} + fn default_future_time_limit() -> u64 { DEFAULT_FUTURE_TIME_LIMIT } @@ -238,36 +278,59 @@ impl Default for ServerConfig { /// Stratum (Mining server) configuration #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(default)] pub struct StratumServerConfig { /// Run a stratum mining server (the only way to communicate to mine this /// node via grin-miner + #[serde(default = "default_enable_stratum_server")] pub enable_stratum_server: Option, /// If enabled, the address and port to listen on + #[serde(default = "default_stratum_server_addr")] pub stratum_server_addr: Option, /// How long to wait before stopping the miner, recollecting transactions /// and starting again + #[serde(default = "default_attempt_time_per_block")] pub attempt_time_per_block: u32, /// Minimum difficulty for worker shares + #[serde(default = "default_minimum_share_difficulty")] pub minimum_share_difficulty: u64, /// Base address to the HTTP wallet receiver + #[serde(default = "default_wallet_listener_url")] pub wallet_listener_url: String, /// Attributes the reward to a random private key instead of contacting the /// wallet receiver. Mostly used for tests. + #[serde(default)] pub burn_reward: bool, } +fn default_attempt_time_per_block() -> u32 { + 15 +} +fn default_minimum_share_difficulty() -> u64 { + 1 +} +fn default_wallet_listener_url() -> String { + "http://127.0.0.1:3415".to_string() +} +fn default_enable_stratum_server() -> Option { + Some(false) +} +fn default_stratum_server_addr() -> Option { + Some("127.0.0.1:3416".to_string()) +} + impl Default for StratumServerConfig { fn default() -> StratumServerConfig { StratumServerConfig { - wallet_listener_url: "http://127.0.0.1:3415".to_string(), + wallet_listener_url: default_wallet_listener_url(), burn_reward: false, - attempt_time_per_block: 15, - minimum_share_difficulty: 1, + attempt_time_per_block: default_attempt_time_per_block(), + minimum_share_difficulty: default_minimum_share_difficulty(), enable_stratum_server: Some(false), stratum_server_addr: Some("127.0.0.1:3416".to_string()), } diff --git a/util/src/logger.rs b/util/src/logger.rs index dc1bf78aab..bc817f5c63 100644 --- a/util/src/logger.rs +++ b/util/src/logger.rs @@ -60,39 +60,70 @@ pub struct LogEntry { } /// Logging config +/// +/// Missing keys in `grin-server.toml` fall back to these defaults (see #3002). #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(default)] pub struct LoggingConfig { /// whether to log to stdout + #[serde(default = "default_true")] pub log_to_stdout: bool, /// logging level for stdout + #[serde(default = "default_stdout_log_level")] pub stdout_log_level: Level, /// whether to log to file + #[serde(default = "default_true")] pub log_to_file: bool, /// log file level + #[serde(default = "default_file_log_level")] pub file_log_level: Level, /// Log file path + #[serde(default = "default_log_file_path")] pub log_file_path: String, /// Whether to append to log or replace + #[serde(default = "default_true")] pub log_file_append: bool, /// Size of the log in bytes to rotate over (optional) + #[serde(default = "default_log_max_size")] pub log_max_size: Option, /// Number of the log files to rotate over (optional) + #[serde(default = "default_log_max_files")] pub log_max_files: Option, /// Whether the tui is running (optional) + #[serde(default)] pub tui_running: Option, } +fn default_true() -> bool { + true +} +fn default_stdout_log_level() -> Level { + Level::Warn +} +fn default_file_log_level() -> Level { + Level::Info +} +fn default_log_file_path() -> String { + String::from("grin.log") +} +fn default_log_max_size() -> Option { + Some(1024 * 1024 * 16) // 16 megabytes default +} +fn default_log_max_files() -> Option { + Some(DEFAULT_ROTATE_LOG_FILES) +} + impl Default for LoggingConfig { fn default() -> LoggingConfig { LoggingConfig { - log_to_stdout: true, - stdout_log_level: Level::Warn, - log_to_file: true, - file_log_level: Level::Info, - log_file_path: String::from("grin.log"), - log_file_append: true, - log_max_size: Some(1024 * 1024 * 16), // 16 megabytes default - log_max_files: Some(DEFAULT_ROTATE_LOG_FILES), + log_to_stdout: default_true(), + stdout_log_level: default_stdout_log_level(), + log_to_file: default_true(), + file_log_level: default_file_log_level(), + log_file_path: default_log_file_path(), + log_file_append: default_true(), + log_max_size: default_log_max_size(), + log_max_files: default_log_max_files(), tui_running: None, } } From 98b73d44c837193f89b4d70d99da04b0317371e4 Mon Sep 17 00:00:00 2001 From: iho Date: Sun, 2 Aug 2026 05:29:05 +0300 Subject: [PATCH 2/2] Address review: chain-aware defaults, logging, unknown keys Default missing [logging] to Some(LoggingConfig::default()) so startup does not unwrap None. Load configs by deep-merging onto for_chain defaults so testnet/usernet get the right API and P2P ports. Reject unknown fields so typos like archive_mod fail to parse. --- config/src/config.rs | 124 ++++++++++++++++++++++++++++++------ config/src/types.rs | 10 ++- p2p/src/types.rs | 1 + pool/src/types.rs | 2 + servers/src/common/types.rs | 8 ++- util/src/logger.rs | 2 +- 6 files changed, 120 insertions(+), 27 deletions(-) diff --git a/config/src/config.rs b/config/src/config.rs index b555b0ae69..dad33c69cd 100644 --- a/config/src/config.rs +++ b/config/src/config.rs @@ -212,6 +212,59 @@ pub fn initial_setup_server(chain_type: &global::ChainTypes) -> Result { + for (key, value) in overlay_map { + match base_map.get_mut(&key) { + Some(base_value) => merge_toml(base_value, value), + None => { + base_map.insert(key, value); + } + } + } + } + (base, overlay) => { + *base = overlay; + } + } +} + +/// Parse config TOML using chain-specific defaults for missing keys. +/// +/// 1. Read `server.chain_type` (default Mainnet) for the defaults base. +/// 2. Start from `GlobalConfig::for_chain` serialized to a TOML table. +/// 3. Deep-merge the file contents on top so only present keys override. +/// 4. Deserialize with `deny_unknown_fields` so typos fail loudly. +fn parse_config_members(toml_str: &str) -> Result { + let chain_type = { + let prelim: ChainTypeConfig = toml::from_str(toml_str).map_err(|e| e.to_string())?; + prelim.server.and_then(|s| s.chain_type).unwrap_or_default() + }; + + let defaults = GlobalConfig::for_chain(&chain_type) + .members + .expect("for_chain always sets members"); + let defaults_toml = + toml::to_string(&defaults).map_err(|e| format!("serialize defaults: {}", e))?; + let mut base: toml::Value = + toml::from_str(&defaults_toml).map_err(|e| format!("defaults as value: {}", e))?; + let overlay: toml::Value = toml::from_str(toml_str).map_err(|e| e.to_string())?; + merge_toml(&mut base, overlay); + + let mut members: ConfigMembers = base + .try_into() + .map_err(|e: toml::de::Error| e.to_string())?; + // Guarantee logging is present after load (startup unwraps it). + if members.logging.is_none() { + members.logging = Some(LoggingConfig::default()); + } + Ok(members) +} + /// Returns the defaults, as strewn throughout the code impl Default for ConfigMembers { fn default() -> ConfigMembers { @@ -299,27 +352,19 @@ impl GlobalConfig { /// Read config fn read_config(mut self) -> Result { let config_file_path = self.config_file_path.as_ref().unwrap(); + let path_str = config_file_path.to_str().unwrap().to_string(); let contents = fs::read_to_string(config_file_path)?; let migrated = GlobalConfig::migrate_config_file_version_none_to_2(contents.clone()) - .map_err(|e| { - ConfigError::ParseError(config_file_path.to_str().unwrap().to_string(), e) - })?; + .map_err(|e| ConfigError::ParseError(path_str.clone(), e))?; if contents != migrated { fs::write(config_file_path, &migrated)?; } let fixed = GlobalConfig::fix_warning_level(migrated); - let decoded: Result = toml::from_str(&fixed); - match decoded { - Ok(gc) => { - self.members = Some(gc); - Ok(self) - } - Err(e) => Err(ConfigError::ParseError( - self.config_file_path.unwrap().to_str().unwrap().to_string(), - format!("{}", e), - )), - } + let members = + parse_config_members(&fixed).map_err(|e| ConfigError::ParseError(path_str, e))?; + self.members = Some(members); + Ok(self) } /// Update paths @@ -603,8 +648,8 @@ api_http_addr = "127.0.0.1:3413" [logging] log_to_stdout = false "#; - let members: ConfigMembers = toml::from_str(toml).expect("minimal config should parse"); - let logging = members.logging.expect("logging section present"); + let members = parse_config_members(toml).expect("minimal config should parse"); + let logging = members.logging.expect("logging always present after load"); assert_eq!(logging.log_to_stdout, false); assert_eq!(logging.log_to_file, true); // default assert_eq!(logging.log_file_append, true); // default @@ -612,7 +657,7 @@ log_to_stdout = false assert_eq!(format!("{:?}", logging.file_log_level), "Info"); } -/// Entire [logging] section can be omitted. +/// Entire [logging] section can be omitted (defaults applied; no panic on unwrap). #[test] fn test_logging_section_optional() { let toml = r#" @@ -620,8 +665,9 @@ fn test_logging_section_optional() { db_root = "chain_data" api_http_addr = "127.0.0.1:3413" "#; - let members: ConfigMembers = toml::from_str(toml).expect("config without logging should parse"); - assert!(members.logging.is_none()); + let members = parse_config_members(toml).expect("config without logging should parse"); + assert!(members.logging.is_some()); + assert_eq!(members.logging.as_ref().unwrap().log_to_file, true); assert_eq!(members.server.db_root, "chain_data"); assert_eq!(members.server.api_http_addr, "127.0.0.1:3413"); // Other server fields use defaults @@ -638,7 +684,7 @@ db_root = "chain_data" [server.stratum_mining_config] enable_stratum_server = true "#; - let members: ConfigMembers = toml::from_str(toml).expect("partial stratum config should parse"); + let members = parse_config_members(toml).expect("partial stratum config should parse"); let stratum = members .server .stratum_mining_config @@ -649,3 +695,41 @@ enable_stratum_server = true assert_eq!(stratum.wallet_listener_url, "http://127.0.0.1:3415"); assert!(!stratum.burn_reward); } + +/// Missing keys use chain-specific defaults from `for_chain`, not always mainnet. +#[test] +fn test_chain_specific_defaults_for_testnet() { + let toml = r#" +[server] +chain_type = "Testnet" +db_root = "my_testnet_data" +"#; + let members = parse_config_members(toml).expect("testnet config should parse"); + assert_eq!(members.server.chain_type, global::ChainTypes::Testnet); + assert_eq!(members.server.db_root, "my_testnet_data"); + // From GlobalConfig::for_chain(Testnet), not mainnet ServerConfig::default() + assert_eq!(members.server.api_http_addr, "127.0.0.1:13413"); + assert_eq!(members.server.p2p_config.port, TESTNET_PEER_PORT); + let stratum = members.server.stratum_mining_config.unwrap(); + assert_eq!( + stratum.stratum_server_addr, + Some("127.0.0.1:13416".to_owned()) + ); + assert_eq!(stratum.wallet_listener_url, "http://127.0.0.1:13415"); +} + +/// Typos / unknown keys are rejected rather than silently defaulted. +#[test] +fn test_unknown_config_key_rejected() { + let toml = r#" +[server] +db_root = "chain_data" +archive_mod = true +"#; + let err = parse_config_members(toml).expect_err("typo should fail"); + assert!( + err.contains("unknown field") || err.contains("archive_mod"), + "unexpected error: {}", + err + ); +} diff --git a/config/src/types.rs b/config/src/types.rs index a397ae093f..d7bcd149fe 100644 --- a/config/src/types.rs +++ b/config/src/types.rs @@ -87,7 +87,7 @@ pub struct GlobalConfig { /// internal state that we don't necessarily /// want serialised or deserialised #[derive(Clone, Debug, Serialize, Deserialize, PartialEq)] -#[serde(default)] +#[serde(default, deny_unknown_fields)] pub struct ConfigMembers { /// Config file version (None == version 1) #[serde(default)] @@ -95,7 +95,11 @@ pub struct ConfigMembers { /// Server config #[serde(default)] pub server: ServerConfig, - /// Logging config (optional section; missing keys use LoggingConfig defaults) - #[serde(default)] + /// Logging config. Omitted section falls back to defaults (never `None` after load). + #[serde(default = "default_logging")] pub logging: Option, } + +fn default_logging() -> Option { + Some(LoggingConfig::default()) +} diff --git a/p2p/src/types.rs b/p2p/src/types.rs index 03acd8ecdd..cd3615e9dc 100644 --- a/p2p/src/types.rs +++ b/p2p/src/types.rs @@ -399,6 +399,7 @@ impl PeerAddr { /// Configuration for the peer-to-peer server. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(default, deny_unknown_fields)] pub struct P2PConfig { pub host: IpAddr, pub port: u16, diff --git a/pool/src/types.rs b/pool/src/types.rs index 88a39af2ff..35ecc0b301 100644 --- a/pool/src/types.rs +++ b/pool/src/types.rs @@ -47,6 +47,7 @@ const DANDELION_ALWAYS_STEM_OUR_TXS: bool = true; /// Configuration for "Dandelion". /// Note: shared between p2p and pool. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(default, deny_unknown_fields)] pub struct DandelionConfig { /// Length of each "epoch". #[serde(default = "default_dandelion_epoch_secs")] @@ -100,6 +101,7 @@ fn default_dandelion_always_stem_our_txs() -> bool { /// Transaction pool configuration #[derive(Clone, Debug, Serialize, Deserialize, PartialEq)] +#[serde(default, deny_unknown_fields)] pub struct PoolConfig { /// Base fee for a transaction to be accepted by the pool. The transaction /// weight is computed from its number of inputs, outputs and kernels and diff --git a/servers/src/common/types.rs b/servers/src/common/types.rs index c29551870f..eafdc3ca38 100644 --- a/servers/src/common/types.rs +++ b/servers/src/common/types.rs @@ -139,9 +139,10 @@ impl Default for ChainValidationMode { /// Full server configuration, aggregating configurations required for the /// different components. /// -/// Missing optional keys fall back to [`Default`] (see #3002). +/// Missing optional keys fall back to chain-aware defaults on load (see #3002). +/// Unknown keys are rejected so typos are not silently ignored. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -#[serde(default)] +#[serde(default, deny_unknown_fields)] pub struct ServerConfig { /// Directory under which the rocksdb stores will be created #[serde(default = "default_db_root")] @@ -278,7 +279,7 @@ impl Default for ServerConfig { /// Stratum (Mining server) configuration #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -#[serde(default)] +#[serde(default, deny_unknown_fields)] pub struct StratumServerConfig { /// Run a stratum mining server (the only way to communicate to mine this /// node via grin-miner @@ -339,6 +340,7 @@ impl Default for StratumServerConfig { /// Web hooks configuration #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(default, deny_unknown_fields)] pub struct WebHooksConfig { /// url to POST transaction data when a new transaction arrives from a peer pub tx_received_url: Option, diff --git a/util/src/logger.rs b/util/src/logger.rs index bc817f5c63..c32e877725 100644 --- a/util/src/logger.rs +++ b/util/src/logger.rs @@ -63,7 +63,7 @@ pub struct LogEntry { /// /// Missing keys in `grin-server.toml` fall back to these defaults (see #3002). #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -#[serde(default)] +#[serde(default, deny_unknown_fields)] pub struct LoggingConfig { /// whether to log to stdout #[serde(default = "default_true")]