From 7fd44764be9df4d3d9eb6a9e3d913890fab0581d Mon Sep 17 00:00:00 2001 From: Al Liu Date: Wed, 20 Sep 2023 23:07:50 +0800 Subject: [PATCH 01/10] Add supports to persist the block for testing happy path --- nomos-core/Cargo.toml | 8 ++++++ nomos-core/src/block/debug_happy.rs | 39 +++++++++++++++++++++++++++++ nomos-core/src/block/mod.rs | 3 +++ tests/Cargo.toml | 2 +- 4 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 nomos-core/src/block/debug_happy.rs diff --git a/nomos-core/Cargo.toml b/nomos-core/Cargo.toml index e671b14be..9793e52ab 100644 --- a/nomos-core/Cargo.toml +++ b/nomos-core/Cargo.toml @@ -21,6 +21,11 @@ bincode = "1.3" once_cell = "1.0" indexmap = { version = "1.9", features = ["serde"] } +# The below three crates, only used for debug-happy feature, need to be removed in the future. +parking_lot = { version = "0.12", optional = true } +tempfile = { version = "3.8", optional = true } +serde_json = { version = "1", optional = true } + [dev-dependencies] rand = "0.8" tokio = { version = "1.23", features = ["macros", "rt"] } @@ -30,3 +35,6 @@ tokio = { version = "1.23", features = ["macros", "rt"] } default = [] raptor = ["raptorq"] mock = [] + +# This feature needs to be removed when we have fixed the nomos node bug +debug-happy = ["parking_lot", "tempfile", "serde_json"] \ No newline at end of file diff --git a/nomos-core/src/block/debug_happy.rs b/nomos-core/src/block/debug_happy.rs new file mode 100644 index 000000000..e65bafdc9 --- /dev/null +++ b/nomos-core/src/block/debug_happy.rs @@ -0,0 +1,39 @@ +use std::{ + fs::File, + hash::Hash, + sync::OnceLock, +}; +use consensus_engine::{Qc, AggregateQc, View}; +use parking_lot::Mutex; + +static TEMP_DEBUG_FILE: OnceLock> = OnceLock::new(); + +pub fn debug_file() -> &'static Mutex { + // Unwrap because we will only use this function in tests, panic anyway. + TEMP_DEBUG_FILE.get_or_init(|| Mutex::new(tempfile::tempfile().unwrap())) +} + +impl Drop for super::Block { + fn drop(&mut self) { + let header = self.header(); + // We need to persist the block to disk before dropping it + // if there is a aggregated qc when testing happy path + if let Qc::Aggregated(qcs) = &header.parent_qc { + #[derive(serde::Serialize)] + struct Info<'a> { + id: String, + view: View, + qcs: &'a AggregateQc, + } + + let mut file = debug_file().lock(); + let info = Info { + id: format!("{}", header.id), + view: header.view, + qcs, + }; + // Use pretty print to make it easier to read, because we need the this for debugging. + serde_json::to_writer_pretty(&mut *file, &info).unwrap(); + } + } +} \ No newline at end of file diff --git a/nomos-core/src/block/mod.rs b/nomos-core/src/block/mod.rs index 2e4a79059..1106b3422 100644 --- a/nomos-core/src/block/mod.rs +++ b/nomos-core/src/block/mod.rs @@ -107,3 +107,6 @@ impl< result } } + +#[cfg(feature = "debug-happy")] +mod debug_happy; \ No newline at end of file diff --git a/tests/Cargo.toml b/tests/Cargo.toml index 9c3c1fdd5..7dcde4793 100644 --- a/tests/Cargo.toml +++ b/tests/Cargo.toml @@ -11,7 +11,7 @@ nomos-network = { path = "../nomos-services/network" } nomos-log = { path = "../nomos-services/log" } nomos-http = { path = "../nomos-services/http", features = ["http"] } overwatch-rs = { git = "https://github.com/logos-co/Overwatch", branch = "main" } -nomos-core = { path = "../nomos-core" } +nomos-core = { path = "../nomos-core", features = ["debug-happy"] } consensus-engine = { path = "../consensus-engine", features = ["serde"] } nomos-mempool = { path = "../nomos-services/mempool", features = ["mock"] } nomos-da = { path = "../nomos-services/data-availability" } From bac5bfc506ac7645792c960f1ff868b9c85c81d7 Mon Sep 17 00:00:00 2001 From: Al Liu Date: Wed, 20 Sep 2023 23:11:25 +0800 Subject: [PATCH 02/10] fix fmt --- nomos-core/src/block/debug_happy.rs | 54 +++++++++++++---------------- nomos-core/src/block/mod.rs | 2 +- 2 files changed, 26 insertions(+), 30 deletions(-) diff --git a/nomos-core/src/block/debug_happy.rs b/nomos-core/src/block/debug_happy.rs index e65bafdc9..21bfa38cc 100644 --- a/nomos-core/src/block/debug_happy.rs +++ b/nomos-core/src/block/debug_happy.rs @@ -1,39 +1,35 @@ -use std::{ - fs::File, - hash::Hash, - sync::OnceLock, -}; -use consensus_engine::{Qc, AggregateQc, View}; +use consensus_engine::{AggregateQc, Qc, View}; use parking_lot::Mutex; +use std::{fs::File, hash::Hash, sync::OnceLock}; static TEMP_DEBUG_FILE: OnceLock> = OnceLock::new(); pub fn debug_file() -> &'static Mutex { - // Unwrap because we will only use this function in tests, panic anyway. - TEMP_DEBUG_FILE.get_or_init(|| Mutex::new(tempfile::tempfile().unwrap())) + // Unwrap because we will only use this function in tests, panic anyway. + TEMP_DEBUG_FILE.get_or_init(|| Mutex::new(tempfile::tempfile().unwrap())) } impl Drop for super::Block { - fn drop(&mut self) { - let header = self.header(); - // We need to persist the block to disk before dropping it - // if there is a aggregated qc when testing happy path - if let Qc::Aggregated(qcs) = &header.parent_qc { - #[derive(serde::Serialize)] - struct Info<'a> { - id: String, - view: View, - qcs: &'a AggregateQc, - } + fn drop(&mut self) { + let header = self.header(); + // We need to persist the block to disk before dropping it + // if there is a aggregated qc when testing happy path + if let Qc::Aggregated(qcs) = &header.parent_qc { + #[derive(serde::Serialize)] + struct Info<'a> { + id: String, + view: View, + qcs: &'a AggregateQc, + } - let mut file = debug_file().lock(); - let info = Info { - id: format!("{}", header.id), - view: header.view, - qcs, - }; - // Use pretty print to make it easier to read, because we need the this for debugging. - serde_json::to_writer_pretty(&mut *file, &info).unwrap(); + let mut file = debug_file().lock(); + let info = Info { + id: format!("{}", header.id), + view: header.view, + qcs, + }; + // Use pretty print to make it easier to read, because we need the this for debugging. + serde_json::to_writer_pretty(&mut *file, &info).unwrap(); + } } - } -} \ No newline at end of file +} diff --git a/nomos-core/src/block/mod.rs b/nomos-core/src/block/mod.rs index 1106b3422..2ccb0c69a 100644 --- a/nomos-core/src/block/mod.rs +++ b/nomos-core/src/block/mod.rs @@ -109,4 +109,4 @@ impl< } #[cfg(feature = "debug-happy")] -mod debug_happy; \ No newline at end of file +mod debug_happy; From 22f7b9ae3e2a17414bd1e4391b113e6fe351f0bc Mon Sep 17 00:00:00 2001 From: Al Liu Date: Wed, 20 Sep 2023 23:37:11 +0800 Subject: [PATCH 03/10] Call persist explicitly --- nomos-core/Cargo.toml | 8 ------ nomos-core/src/block/debug_happy.rs | 35 ----------------------- nomos-core/src/block/mod.rs | 3 -- tests/Cargo.toml | 1 + tests/src/tests/happy.rs | 44 +++++++++++++++++++++++++---- 5 files changed, 40 insertions(+), 51 deletions(-) delete mode 100644 nomos-core/src/block/debug_happy.rs diff --git a/nomos-core/Cargo.toml b/nomos-core/Cargo.toml index 9793e52ab..e671b14be 100644 --- a/nomos-core/Cargo.toml +++ b/nomos-core/Cargo.toml @@ -21,11 +21,6 @@ bincode = "1.3" once_cell = "1.0" indexmap = { version = "1.9", features = ["serde"] } -# The below three crates, only used for debug-happy feature, need to be removed in the future. -parking_lot = { version = "0.12", optional = true } -tempfile = { version = "3.8", optional = true } -serde_json = { version = "1", optional = true } - [dev-dependencies] rand = "0.8" tokio = { version = "1.23", features = ["macros", "rt"] } @@ -35,6 +30,3 @@ tokio = { version = "1.23", features = ["macros", "rt"] } default = [] raptor = ["raptorq"] mock = [] - -# This feature needs to be removed when we have fixed the nomos node bug -debug-happy = ["parking_lot", "tempfile", "serde_json"] \ No newline at end of file diff --git a/nomos-core/src/block/debug_happy.rs b/nomos-core/src/block/debug_happy.rs deleted file mode 100644 index 21bfa38cc..000000000 --- a/nomos-core/src/block/debug_happy.rs +++ /dev/null @@ -1,35 +0,0 @@ -use consensus_engine::{AggregateQc, Qc, View}; -use parking_lot::Mutex; -use std::{fs::File, hash::Hash, sync::OnceLock}; - -static TEMP_DEBUG_FILE: OnceLock> = OnceLock::new(); - -pub fn debug_file() -> &'static Mutex { - // Unwrap because we will only use this function in tests, panic anyway. - TEMP_DEBUG_FILE.get_or_init(|| Mutex::new(tempfile::tempfile().unwrap())) -} - -impl Drop for super::Block { - fn drop(&mut self) { - let header = self.header(); - // We need to persist the block to disk before dropping it - // if there is a aggregated qc when testing happy path - if let Qc::Aggregated(qcs) = &header.parent_qc { - #[derive(serde::Serialize)] - struct Info<'a> { - id: String, - view: View, - qcs: &'a AggregateQc, - } - - let mut file = debug_file().lock(); - let info = Info { - id: format!("{}", header.id), - view: header.view, - qcs, - }; - // Use pretty print to make it easier to read, because we need the this for debugging. - serde_json::to_writer_pretty(&mut *file, &info).unwrap(); - } - } -} diff --git a/nomos-core/src/block/mod.rs b/nomos-core/src/block/mod.rs index 2ccb0c69a..2e4a79059 100644 --- a/nomos-core/src/block/mod.rs +++ b/nomos-core/src/block/mod.rs @@ -107,6 +107,3 @@ impl< result } } - -#[cfg(feature = "debug-happy")] -mod debug_happy; diff --git a/tests/Cargo.toml b/tests/Cargo.toml index 7dcde4793..925d1d16c 100644 --- a/tests/Cargo.toml +++ b/tests/Cargo.toml @@ -30,6 +30,7 @@ reqwest = { version = "0.11", features = ["json"] } nomos-libp2p = { path = "../nomos-libp2p", optional = true } tempfile = "3.6" serde_yaml = "0.9" +serde_json = "1" tokio = "1" futures = "0.3" async-trait = "0.1" diff --git a/tests/src/tests/happy.rs b/tests/src/tests/happy.rs index b93f0d55d..d83842515 100644 --- a/tests/src/tests/happy.rs +++ b/tests/src/tests/happy.rs @@ -1,13 +1,44 @@ -use consensus_engine::View; +use consensus_engine::{AggregateQc, Block, Qc, View}; use fraction::{Fraction, One}; use futures::stream::{self, StreamExt}; -use std::collections::HashSet; +use std::fs::OpenOptions; use std::time::Duration; +use std::{collections::HashSet, fs::File}; use tests::{MixNode, Node, NomosNode, SpawnConfig}; const TARGET_VIEW: View = View::new(20); -async fn happy_test(nodes: Vec) { +pub fn persist(name: &'static str, blocks: impl Iterator) { + // We need to persist the block to disk before dropping it + // if there is a aggregated qc when testing happy path + if let Qc::Aggregated(qcs) = &block.parent_qc { + #[derive(serde::Serialize)] + struct Info<'a> { + id: String, + view: View, + qcs: &'a AggregateQc, + } + + let mut file = OpenOptions::new() + .create(true) + .truncate(true) + .read(true) + .write(true) + .open(format!("{name}.json")) + .unwrap(); + let infos = blocks + .map(|b| Info { + id: format!("{}", header.id), + view: header.view, + qcs, + }) + .collect::>(); + // Use pretty print to make it easier to read, because we need the this for debugging. + serde_json::to_writer_pretty(&mut *file, &infos).unwrap(); + } +} + +async fn happy_test(name: &'static str, nodes: Vec) { let timeout = std::time::Duration::from_secs(20); let timeout = tokio::time::sleep(timeout); tokio::select! { @@ -43,6 +74,9 @@ async fn happy_test(nodes: Vec) { .unwrap() }) .collect::>(); + + // persist the blocks for debugging + persist(name, infos.iter().flat_map(|i| i.safe_blocks.values())); assert_eq!(blocks.len(), 1); } @@ -57,7 +91,7 @@ async fn two_nodes_happy() { mixnet_topology, }) .await; - happy_test(nodes).await; + happy_test("two_nodes", nodes).await; } #[tokio::test] @@ -71,5 +105,5 @@ async fn ten_nodes_happy() { mixnet_topology, }) .await; - happy_test(nodes).await; + happy_test("ten_nodes", nodes).await; } From 866d7162f0244674fb87a19ad07c14dbbc8026e0 Mon Sep 17 00:00:00 2001 From: Al Liu Date: Wed, 20 Sep 2023 23:40:47 +0800 Subject: [PATCH 04/10] fix Cargo --- tests/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Cargo.toml b/tests/Cargo.toml index 925d1d16c..bf60ad09c 100644 --- a/tests/Cargo.toml +++ b/tests/Cargo.toml @@ -11,7 +11,7 @@ nomos-network = { path = "../nomos-services/network" } nomos-log = { path = "../nomos-services/log" } nomos-http = { path = "../nomos-services/http", features = ["http"] } overwatch-rs = { git = "https://github.com/logos-co/Overwatch", branch = "main" } -nomos-core = { path = "../nomos-core", features = ["debug-happy"] } +nomos-core = { path = "../nomos-core" } consensus-engine = { path = "../consensus-engine", features = ["serde"] } nomos-mempool = { path = "../nomos-services/mempool", features = ["mock"] } nomos-da = { path = "../nomos-services/data-availability" } From 490f35a8bb0d5d7851d9a387333b2fde7684f26c Mon Sep 17 00:00:00 2001 From: Al Liu Date: Thu, 21 Sep 2023 11:33:38 +0800 Subject: [PATCH 05/10] Report info --- tests/src/tests/happy.rs | 44 +++++++++++++--------------------------- 1 file changed, 14 insertions(+), 30 deletions(-) diff --git a/tests/src/tests/happy.rs b/tests/src/tests/happy.rs index d83842515..106e46224 100644 --- a/tests/src/tests/happy.rs +++ b/tests/src/tests/happy.rs @@ -8,34 +8,10 @@ use tests::{MixNode, Node, NomosNode, SpawnConfig}; const TARGET_VIEW: View = View::new(20); -pub fn persist(name: &'static str, blocks: impl Iterator) { - // We need to persist the block to disk before dropping it - // if there is a aggregated qc when testing happy path - if let Qc::Aggregated(qcs) = &block.parent_qc { - #[derive(serde::Serialize)] - struct Info<'a> { - id: String, - view: View, - qcs: &'a AggregateQc, - } - - let mut file = OpenOptions::new() - .create(true) - .truncate(true) - .read(true) - .write(true) - .open(format!("{name}.json")) - .unwrap(); - let infos = blocks - .map(|b| Info { - id: format!("{}", header.id), - view: header.view, - qcs, - }) - .collect::>(); - // Use pretty print to make it easier to read, because we need the this for debugging. - serde_json::to_writer_pretty(&mut *file, &infos).unwrap(); - } +#[derive(serde::Serialize)] +struct Info { + id: String, + view: View, } async fn happy_test(name: &'static str, nodes: Vec) { @@ -75,8 +51,16 @@ async fn happy_test(name: &'static str, nodes: Vec) { }) .collect::>(); - // persist the blocks for debugging - persist(name, infos.iter().flat_map(|i| i.safe_blocks.values())); + // try to see if we have invalid blocks + let invalid_blocks = infos.iter().flat_map(|i| i.safe_blocks.values().filter_map(|b| match &b.parent_qc { + Qc::Standard(_) => None, + Qc::Aggregated(_) => Some(Info { + id: b.id.to_string(), + view: b.view, + }), + })).collect::>(); + + assert!(invalid_blocks.is_empty(), "{}", serde_json::to_string_pretty(&invalid_blocks).unwrap()); assert_eq!(blocks.len(), 1); } From fcfc39d2de5cdffb26d4d90f196d6c9fff93fd79 Mon Sep 17 00:00:00 2001 From: Al Liu Date: Thu, 21 Sep 2023 11:33:59 +0800 Subject: [PATCH 06/10] fix fmt --- tests/src/tests/happy.rs | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tests/src/tests/happy.rs b/tests/src/tests/happy.rs index 106e46224..290e75e8e 100644 --- a/tests/src/tests/happy.rs +++ b/tests/src/tests/happy.rs @@ -52,15 +52,24 @@ async fn happy_test(name: &'static str, nodes: Vec) { .collect::>(); // try to see if we have invalid blocks - let invalid_blocks = infos.iter().flat_map(|i| i.safe_blocks.values().filter_map(|b| match &b.parent_qc { - Qc::Standard(_) => None, - Qc::Aggregated(_) => Some(Info { - id: b.id.to_string(), - view: b.view, - }), - })).collect::>(); + let invalid_blocks = infos + .iter() + .flat_map(|i| { + i.safe_blocks.values().filter_map(|b| match &b.parent_qc { + Qc::Standard(_) => None, + Qc::Aggregated(_) => Some(Info { + id: b.id.to_string(), + view: b.view, + }), + }) + }) + .collect::>(); - assert!(invalid_blocks.is_empty(), "{}", serde_json::to_string_pretty(&invalid_blocks).unwrap()); + assert!( + invalid_blocks.is_empty(), + "{}", + serde_json::to_string_pretty(&invalid_blocks).unwrap() + ); assert_eq!(blocks.len(), 1); } From 4a5032a92db549be07166dbf391fe83c069c76e9 Mon Sep 17 00:00:00 2001 From: Al Liu Date: Thu, 21 Sep 2023 11:51:31 +0800 Subject: [PATCH 07/10] fix CI --- tests/Cargo.toml | 1 + tests/src/tests/happy.rs | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Cargo.toml b/tests/Cargo.toml index bf60ad09c..aad45ba0b 100644 --- a/tests/Cargo.toml +++ b/tests/Cargo.toml @@ -29,6 +29,7 @@ waku-bindings = { version = "0.1.1", optional = true } reqwest = { version = "0.11", features = ["json"] } nomos-libp2p = { path = "../nomos-libp2p", optional = true } tempfile = "3.6" +serde = { version = "1", features = ["derive"] } serde_yaml = "0.9" serde_json = "1" tokio = "1" diff --git a/tests/src/tests/happy.rs b/tests/src/tests/happy.rs index 290e75e8e..b9650fbc4 100644 --- a/tests/src/tests/happy.rs +++ b/tests/src/tests/happy.rs @@ -1,9 +1,8 @@ use consensus_engine::{AggregateQc, Block, Qc, View}; use fraction::{Fraction, One}; use futures::stream::{self, StreamExt}; -use std::fs::OpenOptions; use std::time::Duration; -use std::{collections::HashSet, fs::File}; +use std::collections::HashSet; use tests::{MixNode, Node, NomosNode, SpawnConfig}; const TARGET_VIEW: View = View::new(20); From 223f9efc5677ceacc1ceb4053179ea7e14c06471 Mon Sep 17 00:00:00 2001 From: Al Liu Date: Thu, 21 Sep 2023 11:53:27 +0800 Subject: [PATCH 08/10] fix fmt --- tests/src/tests/happy.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/tests/happy.rs b/tests/src/tests/happy.rs index b9650fbc4..bb75d9647 100644 --- a/tests/src/tests/happy.rs +++ b/tests/src/tests/happy.rs @@ -1,8 +1,8 @@ use consensus_engine::{AggregateQc, Block, Qc, View}; use fraction::{Fraction, One}; use futures::stream::{self, StreamExt}; -use std::time::Duration; use std::collections::HashSet; +use std::time::Duration; use tests::{MixNode, Node, NomosNode, SpawnConfig}; const TARGET_VIEW: View = View::new(20); From 9d2e481e1fe9c872c79d49dbda7db7796809c4bb Mon Sep 17 00:00:00 2001 From: Al Liu Date: Thu, 21 Sep 2023 13:51:16 +0800 Subject: [PATCH 09/10] fix warnings --- tests/src/tests/happy.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/src/tests/happy.rs b/tests/src/tests/happy.rs index bb75d9647..bbc425819 100644 --- a/tests/src/tests/happy.rs +++ b/tests/src/tests/happy.rs @@ -1,4 +1,4 @@ -use consensus_engine::{AggregateQc, Block, Qc, View}; +use consensus_engine::{Qc, View}; use fraction::{Fraction, One}; use futures::stream::{self, StreamExt}; use std::collections::HashSet; @@ -13,7 +13,7 @@ struct Info { view: View, } -async fn happy_test(name: &'static str, nodes: Vec) { +async fn happy_test(nodes: Vec) { let timeout = std::time::Duration::from_secs(20); let timeout = tokio::time::sleep(timeout); tokio::select! { @@ -83,7 +83,7 @@ async fn two_nodes_happy() { mixnet_topology, }) .await; - happy_test("two_nodes", nodes).await; + happy_test(nodes).await; } #[tokio::test] @@ -97,5 +97,5 @@ async fn ten_nodes_happy() { mixnet_topology, }) .await; - happy_test("ten_nodes", nodes).await; + happy_test(nodes).await; } From 58ccb270516fc5ed9c4df8a19401fa256742def0 Mon Sep 17 00:00:00 2001 From: Al Liu Date: Mon, 25 Sep 2023 14:37:22 +0800 Subject: [PATCH 10/10] Add node id to the report --- tests/src/tests/happy.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/src/tests/happy.rs b/tests/src/tests/happy.rs index bbc425819..114b80c50 100644 --- a/tests/src/tests/happy.rs +++ b/tests/src/tests/happy.rs @@ -9,7 +9,8 @@ const TARGET_VIEW: View = View::new(20); #[derive(serde::Serialize)] struct Info { - id: String, + node_id: String, + block_id: String, view: View, } @@ -57,7 +58,8 @@ async fn happy_test(nodes: Vec) { i.safe_blocks.values().filter_map(|b| match &b.parent_qc { Qc::Standard(_) => None, Qc::Aggregated(_) => Some(Info { - id: b.id.to_string(), + node_id: i.id.to_string(), + block_id: b.id.to_string(), view: b.view, }), })