From c795389aae8e9fd315edb159f6dacb5fa1b44321 Mon Sep 17 00:00:00 2001 From: garwah <14845405+garwahl@users.noreply.github.com> Date: Tue, 17 Sep 2024 20:01:32 +1000 Subject: [PATCH] fix(cli) Allow Pruning CLI Args to take precedence over TOML configuration (#10774) Co-authored-by: garwah --- Cargo.lock | 1 + crates/config/Cargo.toml | 1 + crates/config/src/config.rs | 79 +++++++++++++++++++++++- crates/node/builder/src/launch/common.rs | 10 ++- 4 files changed, 89 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 40c850026..ecf9e9407 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6581,6 +6581,7 @@ dependencies = [ "humantime-serde", "reth-network-peers", "reth-network-types", + "reth-primitives", "reth-prune-types", "reth-stages-types", "serde", diff --git a/crates/config/Cargo.toml b/crates/config/Cargo.toml index f30056f1e..d8224a3d6 100644 --- a/crates/config/Cargo.toml +++ b/crates/config/Cargo.toml @@ -27,3 +27,4 @@ eyre.workspace = true [dev-dependencies] tempfile.workspace = true reth-network-peers.workspace = true +reth-primitives.workspace = true diff --git a/crates/config/src/config.rs b/crates/config/src/config.rs index 33076afff..24c992fdd 100644 --- a/crates/config/src/config.rs +++ b/crates/config/src/config.rs @@ -392,6 +392,34 @@ impl PruneConfig { pub fn has_receipts_pruning(&self) -> bool { self.segments.receipts.is_some() || !self.segments.receipts_log_filter.is_empty() } + + /// Merges another `PruneConfig` into this one, taking values from the other config if and only + /// if the corresponding value in this config is not set. + pub fn merge(&mut self, other: Option) { + let Some(other) = other else { return }; + + // Merge block_interval + if self.block_interval == 0 { + self.block_interval = other.block_interval; + } + + // Merge the various segment prune modes + self.segments.sender_recovery = + self.segments.sender_recovery.or(other.segments.sender_recovery); + self.segments.transaction_lookup = + self.segments.transaction_lookup.or(other.segments.transaction_lookup); + self.segments.receipts = self.segments.receipts.or(other.segments.receipts); + self.segments.account_history = + self.segments.account_history.or(other.segments.account_history); + self.segments.storage_history = + self.segments.storage_history.or(other.segments.storage_history); + + if self.segments.receipts_log_filter.0.is_empty() && + !other.segments.receipts_log_filter.0.is_empty() + { + self.segments.receipts_log_filter = other.segments.receipts_log_filter; + } + } } /// Helper type to support older versions of Duration deserialization. @@ -415,8 +443,11 @@ where #[cfg(test)] mod tests { use super::{Config, EXTENSION}; + use crate::PruneConfig; use reth_network_peers::TrustedPeer; - use std::{path::Path, str::FromStr, time::Duration}; + use reth_primitives::Address; + use reth_prune_types::{PruneMode, PruneModes, ReceiptsLogPruneConfig}; + use std::{collections::BTreeMap, path::Path, str::FromStr, time::Duration}; fn with_tempdir(filename: &str, proc: fn(&std::path::Path)) { let temp_dir = tempfile::tempdir().unwrap(); @@ -893,6 +924,52 @@ receipts = 'full' assert!(err.contains("invalid value: string \"full\""), "{}", err); } + #[test] + fn test_prune_config_merge() { + let mut config1 = PruneConfig { + block_interval: 5, + segments: PruneModes { + sender_recovery: Some(PruneMode::Full), + transaction_lookup: None, + receipts: Some(PruneMode::Distance(1000)), + account_history: None, + storage_history: Some(PruneMode::Before(5000)), + receipts_log_filter: ReceiptsLogPruneConfig(BTreeMap::from([( + Address::random(), + PruneMode::Full, + )])), + }, + }; + + let config2 = PruneConfig { + block_interval: 10, + segments: PruneModes { + sender_recovery: Some(PruneMode::Distance(500)), + transaction_lookup: Some(PruneMode::Full), + receipts: Some(PruneMode::Full), + account_history: Some(PruneMode::Distance(2000)), + storage_history: Some(PruneMode::Distance(3000)), + receipts_log_filter: ReceiptsLogPruneConfig(BTreeMap::from([ + (Address::random(), PruneMode::Distance(1000)), + (Address::random(), PruneMode::Before(2000)), + ])), + }, + }; + + let original_filter = config1.segments.receipts_log_filter.clone(); + config1.merge(Some(config2)); + + // Check that the configuration has been merged. Any configuration present in config1 + // should not be overwritten by config2 + assert_eq!(config1.block_interval, 5); + assert_eq!(config1.segments.sender_recovery, Some(PruneMode::Full)); + assert_eq!(config1.segments.transaction_lookup, Some(PruneMode::Full)); + assert_eq!(config1.segments.receipts, Some(PruneMode::Distance(1000))); + assert_eq!(config1.segments.account_history, Some(PruneMode::Distance(2000))); + assert_eq!(config1.segments.storage_history, Some(PruneMode::Before(5000))); + assert_eq!(config1.segments.receipts_log_filter, original_filter); + } + #[test] fn test_conf_trust_nodes_only() { let trusted_nodes_only = r"# diff --git a/crates/node/builder/src/launch/common.rs b/crates/node/builder/src/launch/common.rs index 1fc0d8cae..3328ea71d 100644 --- a/crates/node/builder/src/launch/common.rs +++ b/crates/node/builder/src/launch/common.rs @@ -351,8 +351,16 @@ impl LaunchContextWith> { } /// Returns the configured [`PruneConfig`] + /// Any configuration set in CLI will take precedence over those set in toml pub fn prune_config(&self) -> Option { - self.toml_config().prune.clone().or_else(|| self.node_config().prune_config()) + let Some(mut node_prune_config) = self.node_config().prune_config() else { + // No CLI config is set, use the toml config. + return self.toml_config().prune.clone(); + }; + + // Otherwise, use the CLI configuration and merge with toml config. + node_prune_config.merge(self.toml_config().prune.clone()); + Some(node_prune_config) } /// Returns the configured [`PruneModes`], returning the default if no config was available.