Dump log revamp (#21)

jfldde · web-flow · commit e85ecd3dca51 · 2024-10-10T08:41:15.000+01:00
* Impl LogProvider on config directly

* Don't dump logs if node not present in test_case
diff --git a/src/bitcoin.rs b/src/bitcoin.rs
@@ -1,6 +1,7 @@
 use std::{
     collections::HashSet,
-    path::PathBuf,
+    fs::File,
+    process::Stdio,
     sync::Arc,
     time::{Duration, Instant},
 };
@@ -17,10 +18,10 @@ use super::{
     config::BitcoinConfig,
     docker::DockerEnv,
     framework::TestContext,
-    traits::{LogProvider, NodeT, Restart, SpawnOutput},
+    traits::{NodeT, Restart, SpawnOutput},
     Result,
 };
-use crate::node::NodeKind;
+use crate::{log_provider::LogPathProvider, node::NodeKind};
 
 pub const FINALITY_DEPTH: u64 = 8;
 
@@ -185,10 +186,19 @@ impl NodeT for BitcoinNode {
         let args = config.args();
         debug!("Running bitcoind with args : {args:?}");
 
+        info!(
+            "Bitcoin debug.log available at : {}",
+            config.log_path().display()
+        );
+
+        let stderr_path = config.stderr_path();
+        let stderr_file = File::create(stderr_path).context("Failed to create stderr file")?;
+
         Command::new("bitcoind")
             .args(&args)
             .kill_on_drop(true)
             .envs(config.env.clone())
+            .stderr(Stdio::from(stderr_file))
             .spawn()
             .context("Failed to spawn bitcoind process")
             .map(SpawnOutput::Child)
@@ -258,7 +268,7 @@ impl Restart for BitcoinNode {
 
     async fn start(&mut self, config: Option<Self::Config>) -> Result<()> {
         if let Some(config) = config {
-            self.config = config
+            self.config = config;
         }
         self.spawn_output = Self::spawn(&self.config, &self.docker_env).await?;
 
@@ -271,16 +281,6 @@ impl Restart for BitcoinNode {
     }
 }
 
-impl LogProvider for BitcoinNode {
-    fn kind(&self) -> NodeKind {
-        NodeKind::Bitcoin
-    }
-
-    fn log_path(&self) -> PathBuf {
-        self.config.data_dir.join("regtest").join("debug.log")
-    }
-}
-
 pub struct BitcoinNodeCluster {
     inner: Vec<BitcoinNode>,
 }
@@ -361,7 +361,7 @@ async fn wait_for_rpc_ready(client: &Client, timeout: Option<Duration>) -> Resul
             Ok(_) => return Ok(()),
             Err(e) => {
                 trace!("[wait_for_rpc_ready] error {e}");
-                sleep(Duration::from_millis(500)).await
+                sleep(Duration::from_millis(500)).await;
             }
         }
     }
diff --git a/src/citrea_config/rollup.rs b/src/citrea_config/rollup.rs
@@ -86,7 +86,7 @@ const fn default_max_subscriptions_per_connection() -> u32 {
 pub struct StorageConfig {
     /// Path that can be utilized by concrete rollup implementation
     pub path: PathBuf,
-    /// File descriptor limit for RocksDB
+    /// File descriptor limit for `RocksDB`
     pub db_max_open_files: Option<i32>,
 }
 
diff --git a/src/citrea_config/sequencer.rs b/src/citrea_config/sequencer.rs
@@ -29,7 +29,7 @@ impl Default for SequencerConfig {
             deposit_mempool_fetch_limit: 10,
             block_production_interval_ms: 100,
             da_update_interval_ms: 100,
-            mempool_conf: Default::default(),
+            mempool_conf: SequencerMempoolConfig::default(),
         }
     }
 }
@@ -57,11 +57,11 @@ pub struct SequencerMempoolConfig {
 impl Default for SequencerMempoolConfig {
     fn default() -> Self {
         Self {
-            pending_tx_limit: 100000,
+            pending_tx_limit: 100_000,
             pending_tx_size: 200,
-            queue_tx_limit: 100000,
+            queue_tx_limit: 100_000,
             queue_tx_size: 200,
-            base_fee_tx_limit: 100000,
+            base_fee_tx_limit: 100_000,
             base_fee_tx_size: 200,
             max_account_slots: 16,
         }
diff --git a/src/client.rs b/src/client.rs
@@ -86,7 +86,7 @@ impl Client {
         self.client
             .get_sequencer_commitments_on_slot_by_hash(hash)
             .await
-            .map_err(|e| e.into())
+            .map_err(Into::into)
     }
 
     pub async fn ledger_get_head_soft_confirmation_height(&self) -> Result<u64> {
diff --git a/src/config/bitcoin.rs b/src/config/bitcoin.rs
@@ -3,6 +3,8 @@ use std::path::PathBuf;
 use bitcoin::Network;
 use tempfile::TempDir;
 
+use crate::{log_provider::LogPathProvider, node::NodeKind};
+
 #[derive(Debug, Clone)]
 pub struct BitcoinConfig {
     pub p2p_port: u16,
@@ -62,3 +64,17 @@ impl BitcoinConfig {
         .concat()
     }
 }
+
+impl LogPathProvider for BitcoinConfig {
+    fn kind() -> NodeKind {
+        NodeKind::Bitcoin
+    }
+
+    fn log_path(&self) -> PathBuf {
+        self.data_dir.join("regtest").join("debug.log")
+    }
+
+    fn stderr_path(&self) -> PathBuf {
+        self.data_dir.join("stderr.log")
+    }
+}
diff --git a/src/config/mod.rs b/src/config/mod.rs
@@ -22,7 +22,10 @@ pub use crate::citrea_config::{
     rollup::{FullNodeConfig, RollupPublicKeys, RpcConfig, RunnerConfig, StorageConfig},
     sequencer::SequencerConfig,
 };
-use crate::node::{Config, NodeKind};
+use crate::{
+    log_provider::LogPathProvider,
+    node::{Config, NodeKind},
+};
 
 #[derive(Clone, Debug)]
 pub struct FullL2NodeConfig<T> {
@@ -96,3 +99,20 @@ where
         &self.rollup
     }
 }
+
+impl<T: Clone + Serialize> LogPathProvider for FullL2NodeConfig<T>
+where
+    FullL2NodeConfig<T>: Config,
+{
+    fn kind() -> NodeKind {
+        Self::node_kind()
+    }
+
+    fn log_path(&self) -> PathBuf {
+        self.dir().join("stdout.log")
+    }
+
+    fn stderr_path(&self) -> PathBuf {
+        self.dir().join("stderr.log")
+    }
+}
diff --git a/src/config/rollup.rs b/src/config/rollup.rs
@@ -66,7 +66,7 @@ impl From<BitcoinConfig> for BitcoinServiceConfig {
             node_password: v.rpc_password,
             network: v.network,
             da_private_key: None,
-            tx_backup_dir: "".to_string(),
+            tx_backup_dir: String::new(),
         }
     }
 }
diff --git a/src/framework.rs b/src/framework.rs
@@ -4,16 +4,15 @@ use bitcoincore_rpc::RpcApi;
 use tracing::{debug, info};
 
 use super::{
-    bitcoin::BitcoinNodeCluster,
-    config::TestConfig,
-    docker::DockerEnv,
-    full_node::FullNode,
-    node::NodeKind,
-    sequencer::Sequencer,
-    traits::{LogProvider, LogProviderErased, NodeT},
-    Result,
+    bitcoin::BitcoinNodeCluster, config::TestConfig, docker::DockerEnv, full_node::FullNode,
+    node::NodeKind, sequencer::Sequencer, traits::NodeT, Result,
+};
+use crate::{
+    batch_prover::BatchProver,
+    light_client_prover::LightClientProver,
+    log_provider::{LogPathProvider, LogPathProviderErased},
+    utils::tail_file,
 };
-use crate::{batch_prover::BatchProver, light_client_prover::LightClientProver, utils::tail_file};
 
 pub struct TestContext {
     pub config: TestConfig,
@@ -41,7 +40,6 @@ pub struct TestFramework {
     pub batch_prover: Option<BatchProver>,
     pub light_client_prover: Option<LightClientProver>,
     pub full_node: Option<FullNode>,
-    show_logs: bool,
     pub initial_da_height: u64,
 }
 
@@ -64,15 +62,13 @@ impl TestFramework {
 
         let bitcoin_nodes = BitcoinNodeCluster::new(&ctx).await?;
 
-        // tokio::time::sleep(std::time::Duration::from_secs(30)).await;
         Ok(Self {
             bitcoin_nodes,
             sequencer: None,
             batch_prover: None,
             light_client_prover: None,
             full_node: None,
             ctx,
-            show_logs: true,
             initial_da_height: 0,
         })
     }
@@ -103,36 +99,31 @@ impl TestFramework {
         Ok(())
     }
 
-    fn get_nodes_as_log_provider(&self) -> Vec<&dyn LogProviderErased> {
-        vec![
-            self.bitcoin_nodes.get(0).map(LogProvider::as_erased),
-            self.sequencer.as_ref().map(LogProvider::as_erased),
-            self.full_node.as_ref().map(LogProvider::as_erased),
-            self.batch_prover.as_ref().map(LogProvider::as_erased),
-            self.light_client_prover
-                .as_ref()
-                .map(LogProvider::as_erased),
-        ]
-        .into_iter()
-        .flatten()
-        .collect()
-    }
-
-    pub fn show_log_paths(&self) {
-        if self.show_logs {
-            info!(
-                "Logs available at {}",
-                self.ctx.config.test_case.dir.display()
-            );
-
-            for node in self.get_nodes_as_log_provider() {
-                info!(
-                    "{} logs available at : {}",
-                    node.kind(),
-                    node.log_path().display()
-                );
-            }
-        }
+    fn get_nodes_as_log_provider(&self) -> Vec<&dyn LogPathProviderErased> {
+        let test_case = &self.ctx.config.test_case;
+
+        self.ctx
+            .config
+            .bitcoin
+            .iter()
+            .map(LogPathProvider::as_erased)
+            .map(Option::Some)
+            .chain(vec![
+                test_case
+                    .with_sequencer
+                    .then(|| LogPathProvider::as_erased(&self.ctx.config.sequencer)),
+                test_case
+                    .with_full_node
+                    .then(|| LogPathProvider::as_erased(&self.ctx.config.full_node)),
+                test_case
+                    .with_batch_prover
+                    .then(|| LogPathProvider::as_erased(&self.ctx.config.batch_prover)),
+                test_case
+                    .with_light_client_prover
+                    .then(|| LogPathProvider::as_erased(&self.ctx.config.light_client_prover)),
+            ])
+            .flatten()
+            .collect()
     }
 
     pub fn dump_log(&self) -> Result<()> {
@@ -142,11 +133,11 @@ impl TestFramework {
             .ok()
             .and_then(|v| v.parse::<usize>().ok())
             .unwrap_or(25);
-        for node in self.get_nodes_as_log_provider() {
-            debug!("{} logs (last {n_lines} lines):", node.kind());
-            if let Err(e) = tail_file(&node.log_path(), n_lines) {
-                eprint!("{e}");
-            }
+        for provider in self.get_nodes_as_log_provider() {
+            println!("{} logs (last {n_lines} lines):", provider.kind());
+            let _ = tail_file(&provider.log_path(), n_lines);
+            println!("{} stderr logs (last {n_lines} lines):", provider.kind());
+            let _ = tail_file(&provider.stderr_path(), n_lines);
         }
         Ok(())
     }
diff --git a/src/lib.rs b/src/lib.rs
@@ -7,6 +7,7 @@ mod docker;
 pub mod framework;
 pub mod full_node;
 pub mod light_client_prover;
+mod log_provider;
 pub mod node;
 pub mod sequencer;
 pub mod test_case;
diff --git a/src/log_provider.rs b/src/log_provider.rs
@@ -0,0 +1,35 @@
+use std::path::PathBuf;
+
+use crate::node::NodeKind;
+
+pub trait LogPathProvider {
+    fn kind() -> NodeKind;
+    fn log_path(&self) -> PathBuf;
+    fn stderr_path(&self) -> PathBuf;
+    fn as_erased(&self) -> &dyn LogPathProviderErased
+    where
+        Self: Sized,
+    {
+        self
+    }
+}
+
+pub trait LogPathProviderErased {
+    fn kind(&self) -> NodeKind;
+    fn log_path(&self) -> PathBuf;
+    fn stderr_path(&self) -> PathBuf;
+}
+
+impl<T: LogPathProvider> LogPathProviderErased for T {
+    fn kind(&self) -> NodeKind {
+        T::kind()
+    }
+
+    fn log_path(&self) -> PathBuf {
+        LogPathProvider::log_path(self)
+    }
+
+    fn stderr_path(&self) -> PathBuf {
+        LogPathProvider::stderr_path(self)
+    }
+}
diff --git a/src/node.rs b/src/node.rs
diff --git a/src/test_case.rs b/src/test_case.rs
diff --git a/src/traits.rs b/src/traits.rs
diff --git a/src/utils.rs b/src/utils.rs

Original file line number	Diff line number	Diff line change
`@@ -86,7 +86,7 @@ const fn default_max_subscriptions_per_connection() -> u32 {`
`86`	`86`	`pub struct StorageConfig {`
`87`	`87`	`/// Path that can be utilized by concrete rollup implementation`
`88`	`88`	`pub path: PathBuf,`
`89`		`- /// File descriptor limit for RocksDB`
	`89`	+ /// File descriptor limit for `RocksDB`
`90`	`90`	`pub db_max_open_files: Option<i32>,`
`91`	`91`	`}`
`92`	`92`
Original file line number	Diff line number	Diff line change
`@@ -29,7 +29,7 @@ impl Default for SequencerConfig {`
`29`	`29`	`deposit_mempool_fetch_limit: 10,`
`30`	`30`	`block_production_interval_ms: 100,`
`31`	`31`	`da_update_interval_ms: 100,`
`32`		`- mempool_conf: Default::default(),`
	`32`	`+ mempool_conf: SequencerMempoolConfig::default(),`
`33`	`33`	`}`
`34`	`34`	`}`
`35`	`35`	`}`
`@@ -57,11 +57,11 @@ pub struct SequencerMempoolConfig {`
`57`	`57`	`impl Default for SequencerMempoolConfig {`
`58`	`58`	`fn default() -> Self {`
`59`	`59`	`Self {`
`60`		`- pending_tx_limit: 100000,`
	`60`	`+ pending_tx_limit: 100_000,`
`61`	`61`	`pending_tx_size: 200,`
`62`		`- queue_tx_limit: 100000,`
	`62`	`+ queue_tx_limit: 100_000,`
`63`	`63`	`queue_tx_size: 200,`
`64`		`- base_fee_tx_limit: 100000,`
	`64`	`+ base_fee_tx_limit: 100_000,`
`65`	`65`	`base_fee_tx_size: 200,`
`66`	`66`	`max_account_slots: 16,`
`67`	`67`	`}`
Original file line number	Diff line number	Diff line change
`@@ -86,7 +86,7 @@ impl Client {`
`86`	`86`	`self.client`
`87`	`87`	`.get_sequencer_commitments_on_slot_by_hash(hash)`
`88`	`88`	`.await`
`89`		`- .map_err(\|e\| e.into())`
	`89`	`+ .map_err(Into::into)`
`90`	`90`	`}`
`91`	`91`
`92`	`92`	`pub async fn ledger_get_head_soft_confirmation_height(&self) -> Result<u64> {`
Original file line number	Diff line number	Diff line change
`@@ -66,7 +66,7 @@ impl From<BitcoinConfig> for BitcoinServiceConfig {`
`66`	`66`	`node_password: v.rpc_password,`
`67`	`67`	`network: v.network,`
`68`	`68`	`da_private_key: None,`
`69`		`- tx_backup_dir: "".to_string(),`
	`69`	`+ tx_backup_dir: String::new(),`
`70`	`70`	`}`
`71`	`71`	`}`
`72`	`72`	`}`