diff --git a/crates/hotshot/src/tasks/mod.rs b/crates/hotshot/src/tasks/mod.rs index 5ed9503891..40802e91be 100644 --- a/crates/hotshot/src/tasks/mod.rs +++ b/crates/hotshot/src/tasks/mod.rs @@ -2,7 +2,7 @@ /// Provides trait to create task states from a `SystemContextHandle` pub mod task_state; -use std::{sync::Arc, time::Duration}; +use std::{collections::HashSet, sync::Arc, time::Duration}; use async_broadcast::broadcast; use async_compatibility_layer::art::{async_sleep, async_spawn}; @@ -25,6 +25,8 @@ use hotshot_task_impls::{ }; use hotshot_types::{ constants::EVENT_CHANNEL_SIZE, + data::QuorumProposal, + message::Proposal, message::{Messages, VersionedMessage}, traits::{ network::ConnectedNetwork, @@ -408,6 +410,82 @@ impl> EventTransformerState> { + /// Store events from previous views + pub validated_proposals: Vec>, + /// How many times current node has been elected leader and sent proposal + pub total_proposals_from_node: u64, + /// Which proposals to be dishonest at + pub dishonest_at_proposal_numbers: HashSet, + /// How far back to look for a QC + pub view_look_back: usize, + /// Phantom + pub _phantom: std::marker::PhantomData, +} + +/// Add method that will handle `QuorumProposalSend` events +/// If we have previous proposals stored and the total_proposals_from_node matches a value specified in dishonest_at_proposal_numbers +/// Then send out the event with the modified proposal that has an older QC +impl> DishonestLeader { + /// When a leader is sending a proposal this method will mock a dishonest leader + /// We accomplish this by looking back a number of specified views and using that cached proposals QC + fn handle_proposal_send_event( + &self, + event: &HotShotEvent, + proposal: &Proposal>, + sender: &TYPES::SignatureKey, + ) -> HotShotEvent { + let length = self.validated_proposals.len(); + if !self + .dishonest_at_proposal_numbers + .contains(&self.total_proposals_from_node) + || length == 0 + { + return event.clone(); + } + + // Grab proposal from specified view look back + let proposal_from_look_back = if length - 1 < self.view_look_back { + // If look back is too far just take the first proposal + self.validated_proposals[0].clone() + } else { + let index = (self.validated_proposals.len() - 1) - self.view_look_back; + self.validated_proposals[index].clone() + }; + + // Create a dishonest proposal by using the old proposals qc + let mut dishonest_proposal = proposal.clone(); + dishonest_proposal.data.justify_qc = proposal_from_look_back.justify_qc; + + HotShotEvent::QuorumProposalSend(dishonest_proposal, sender.clone()) + } +} + +#[async_trait] +impl + std::fmt::Debug> + EventTransformerState for DishonestLeader +{ + async fn recv_handler(&mut self, event: &HotShotEvent) -> Vec> { + vec![event.clone()] + } + + async fn send_handler(&mut self, event: &HotShotEvent) -> Vec> { + match event { + HotShotEvent::QuorumProposalSend(proposal, sender) => { + self.total_proposals_from_node += 1; + return vec![self.handle_proposal_send_event(event, proposal, sender)]; + } + HotShotEvent::QuorumProposalValidated(proposal, _) => { + self.validated_proposals.push(proposal.clone()); + } + _ => {} + } + vec![event.clone()] + } +} + /// adds tasks for sending/receiving messages to/from the network. pub async fn add_network_tasks>( handle: &mut SystemContextHandle, diff --git a/crates/testing/src/consistency_task.rs b/crates/testing/src/consistency_task.rs index 4b9ecc0f22..01c868a5a1 100644 --- a/crates/testing/src/consistency_task.rs +++ b/crates/testing/src/consistency_task.rs @@ -148,7 +148,7 @@ pub struct ConsistencyTask { /// A map from node ids to (leaves keyed on view number) pub consensus_leaves: NetworkMap, /// safety task requirements - pub safety_properties: OverallSafetyPropertiesDescription, + pub safety_properties: OverallSafetyPropertiesDescription, } #[async_trait] diff --git a/crates/testing/src/overall_safety_task.rs b/crates/testing/src/overall_safety_task.rs index add9853a14..3a27e6edbc 100644 --- a/crates/testing/src/overall_safety_task.rs +++ b/crates/testing/src/overall_safety_task.rs @@ -71,6 +71,11 @@ pub enum OverallSafetyTaskErr { failed_views: HashSet, }, + /// mismatched expected failed view vs actual failed view + InconsistentFailedViews { + expected_failed_views: HashSet, + actual_failed_views: HashSet, + }, } /// Data availability task state @@ -80,13 +85,42 @@ pub struct OverallSafetyTask, /// configure properties - pub properties: OverallSafetyPropertiesDescription, + pub properties: OverallSafetyPropertiesDescription, /// error pub error: Option>>, /// sender to test event channel pub test_sender: Sender, } +impl> OverallSafetyTask { + async fn handle_view_failure(&mut self, num_failed_views: usize, view_number: TYPES::Time) { + let expected_view_to_fail = &mut self.properties.expected_views_to_fail; + + self.ctx.failed_views.insert(view_number); + if self.ctx.failed_views.len() > num_failed_views { + let _ = self.test_sender.broadcast(TestEvent::Shutdown).await; + self.error = Some(Box::new(OverallSafetyTaskErr::::TooManyFailures { + failed_views: self.ctx.failed_views.clone(), + })); + } else if !expected_view_to_fail.is_empty() { + match expected_view_to_fail.get(&view_number) { + Some(_) => { + expected_view_to_fail.insert(view_number, true); + } + None => { + let _ = self.test_sender.broadcast(TestEvent::Shutdown).await; + self.error = Some(Box::new( + OverallSafetyTaskErr::::InconsistentFailedViews { + expected_failed_views: expected_view_to_fail.keys().cloned().collect(), + actual_failed_views: self.ctx.failed_views.clone(), + }, + )); + } + } + } + } +} + #[async_trait] impl> TestTaskState for OverallSafetyTask @@ -95,14 +129,15 @@ impl> TestTaskState /// Handles an event from one of multiple receivers. async fn handle_event(&mut self, (message, id): (Self::Event, usize)) -> Result<()> { - let OverallSafetyPropertiesDescription { + let OverallSafetyPropertiesDescription:: { check_leaf, check_block, num_failed_views, num_successful_views, threshold_calculator, transaction_threshold, - }: OverallSafetyPropertiesDescription = self.properties.clone(); + .. + }: OverallSafetyPropertiesDescription = self.properties.clone(); let Event { view_number, event } = message; let key = match event { EventType::Error { error } => { @@ -168,14 +203,9 @@ impl> TestTaskState return Ok(()); } ViewStatus::Failed => { - self.ctx.failed_views.insert(view_number); - if self.ctx.failed_views.len() > num_failed_views { - let _ = self.test_sender.broadcast(TestEvent::Shutdown).await; - self.error = - Some(Box::new(OverallSafetyTaskErr::::TooManyFailures { - failed_views: self.ctx.failed_views.clone(), - })); - } + self.handle_view_failure(num_failed_views, view_number) + .await; + return Ok(()); } ViewStatus::Err(e) => { @@ -189,13 +219,8 @@ impl> TestTaskState } } else if view.check_if_failed(threshold, len) { view.status = ViewStatus::Failed; - self.ctx.failed_views.insert(view_number); - if self.ctx.failed_views.len() > num_failed_views { - let _ = self.test_sender.broadcast(TestEvent::Shutdown).await; - self.error = Some(Box::new(OverallSafetyTaskErr::::TooManyFailures { - failed_views: self.ctx.failed_views.clone(), - })); - } + self.handle_view_failure(num_failed_views, view_number) + .await; return Ok(()); } Ok(()) @@ -206,14 +231,15 @@ impl> TestTaskState return TestResult::Fail(e.clone()); } - let OverallSafetyPropertiesDescription { + let OverallSafetyPropertiesDescription:: { check_leaf: _, check_block: _, num_failed_views: num_failed_rounds_total, num_successful_views, threshold_calculator: _, transaction_threshold: _, - }: OverallSafetyPropertiesDescription = self.properties.clone(); + expected_views_to_fail, + }: OverallSafetyPropertiesDescription = self.properties.clone(); let num_incomplete_views = self.ctx.round_results.len() - self.ctx.successful_views.len() @@ -232,6 +258,18 @@ impl> TestTaskState })); } + if !expected_views_to_fail + .values() + .all(|&view_failed| view_failed) + { + return TestResult::Fail(Box::new( + OverallSafetyTaskErr::::InconsistentFailedViews { + actual_failed_views: self.ctx.failed_views.clone(), + expected_failed_views: expected_views_to_fail.keys().cloned().collect(), + }, + )); + } + // We should really be able to include a check like this: // // if self.ctx.failed_views.len() < num_failed_rounds_total { @@ -495,7 +533,7 @@ impl RoundResult { /// cross node safety properties #[derive(Clone)] -pub struct OverallSafetyPropertiesDescription { +pub struct OverallSafetyPropertiesDescription { /// required number of successful views pub num_successful_views: usize, /// whether or not to check the leaf @@ -512,9 +550,11 @@ pub struct OverallSafetyPropertiesDescription { /// threshold calculator. Given number of live and total nodes, provide number of successes /// required to mark view as successful pub threshold_calculator: Arc usize + Send + Sync>, + /// pass in the views that we expect to fail + pub expected_views_to_fail: HashMap, } -impl std::fmt::Debug for OverallSafetyPropertiesDescription { +impl std::fmt::Debug for OverallSafetyPropertiesDescription { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("OverallSafetyPropertiesDescription") .field("num successful views", &self.num_successful_views) @@ -522,11 +562,12 @@ impl std::fmt::Debug for OverallSafetyPropertiesDescription { .field("check_block", &self.check_block) .field("num_failed_rounds_total", &self.num_failed_views) .field("transaction_threshold", &self.transaction_threshold) + .field("expected views to fail", &self.expected_views_to_fail) .finish_non_exhaustive() } } -impl Default for OverallSafetyPropertiesDescription { +impl Default for OverallSafetyPropertiesDescription { fn default() -> Self { Self { num_successful_views: 50, @@ -536,6 +577,7 @@ impl Default for OverallSafetyPropertiesDescription { transaction_threshold: 0, // very strict threshold_calculator: Arc::new(|_num_live, num_total| 2 * num_total / 3 + 1), + expected_views_to_fail: HashMap::new(), } } } diff --git a/crates/testing/src/test_builder.rs b/crates/testing/src/test_builder.rs index d2726aa25f..39a76071a6 100644 --- a/crates/testing/src/test_builder.rs +++ b/crates/testing/src/test_builder.rs @@ -67,7 +67,7 @@ pub struct TestDescription> { /// Size of the non-staked DA committee for the test pub da_non_staked_committee_size: usize, /// overall safety property description - pub overall_safety_properties: OverallSafetyPropertiesDescription, + pub overall_safety_properties: OverallSafetyPropertiesDescription, /// spinning properties pub spinning_properties: SpinningTaskDescription, /// txns timing @@ -230,13 +230,14 @@ impl> TestDescription { num_nodes_with_stake, num_nodes_without_stake, start_nodes: num_nodes_with_stake, - overall_safety_properties: OverallSafetyPropertiesDescription { + overall_safety_properties: OverallSafetyPropertiesDescription:: { num_successful_views: 50, check_leaf: true, check_block: true, num_failed_views: 15, transaction_threshold: 0, threshold_calculator: Arc::new(|_active, total| (2 * total / 3 + 1)), + expected_views_to_fail: HashMap::new(), }, timing_data: TimingData { next_view_timeout: 2000, @@ -263,13 +264,14 @@ impl> TestDescription { num_nodes_with_stake, num_nodes_without_stake, start_nodes: num_nodes_with_stake, - overall_safety_properties: OverallSafetyPropertiesDescription { + overall_safety_properties: OverallSafetyPropertiesDescription:: { num_successful_views: 20, check_leaf: true, check_block: true, num_failed_views: 8, transaction_threshold: 0, threshold_calculator: Arc::new(|_active, total| (2 * total / 3 + 1)), + expected_views_to_fail: HashMap::new(), }, timing_data: TimingData { start_delay: 120_000, diff --git a/crates/testing/tests/tests_1/test_with_failures_2.rs b/crates/testing/tests/tests_1/test_with_failures_2.rs index cb9acd9dc5..c41d362c24 100644 --- a/crates/testing/tests/tests_1/test_with_failures_2.rs +++ b/crates/testing/tests/tests_1/test_with_failures_2.rs @@ -1,6 +1,5 @@ // TODO: Remove this after integration #![allow(unused_imports)] - use hotshot_example_types::{ node_types::{Libp2pImpl, MemoryImpl, PushCdnImpl}, state_types::TestTypes, @@ -8,9 +7,17 @@ use hotshot_example_types::{ use hotshot_macros::cross_tests; use hotshot_testing::{ block_builder::SimpleBuilderImplementation, + completion_task::{CompletionTaskDescription, TimeBasedCompletionTaskDescription}, spinning_task::{ChangeNode, SpinningTaskDescription, UpDown}, test_builder::TestDescription, }; +use hotshot_types::data::ViewNumber; +use hotshot_types::traits::node_implementation::ConsensusTime; +use std::{collections::HashMap, time::Duration}; +use std::collections::HashSet; + +#[cfg(async_executor_impl = "async-std")] +use {hotshot::tasks::DishonestLeader, hotshot_testing::test_builder::Behaviour, std::rc::Rc}; // Test that a good leader can succeed in the view directly after view sync #[cfg(not(feature = "dependency-tasks"))] cross_tests!( @@ -52,3 +59,45 @@ cross_tests!( metadata } ); + +#[cfg(async_executor_impl = "async-std")] +cross_tests!( + TestName: dishonest_leader, + Impls: [MemoryImpl], + Types: [TestTypes], + Ignore: false, + Metadata: { + let behaviour = Rc::new(|node_id| { + let dishonest_leader = DishonestLeader:: { + dishonest_at_proposal_numbers: HashSet::from([2, 3]), + validated_proposals: Vec::new(), + total_proposals_from_node: 0, + view_look_back: 1, + _phantom: std::marker::PhantomData + }; + match node_id { + 2 => Behaviour::Byzantine(Box::new(dishonest_leader)), + _ => Behaviour::Standard, + } + }); + + let mut metadata = TestDescription { + // allow more time to pass in CI + completion_task_description: CompletionTaskDescription::TimeBasedCompletionTaskBuilder( + TimeBasedCompletionTaskDescription { + duration: Duration::from_secs(60), + }, + ), + behaviour, + ..TestDescription::default() + }; + + metadata.overall_safety_properties.num_failed_views = 2; + metadata.num_nodes_with_stake = 5; + metadata.overall_safety_properties.expected_views_to_fail = HashMap::from([ + (ViewNumber::new(7), false), + (ViewNumber::new(12), false) + ]); + metadata + }, +);