From e87fc540b0b6c5379a13f6913a7c4f08515085e1 Mon Sep 17 00:00:00 2001 From: ss-es <155648797+ss-es@users.noreply.github.com> Date: Fri, 16 Aug 2024 11:52:11 -0400 Subject: [PATCH] Add marketplace integration test (#3569) --- crates/example-types/src/block_types.rs | 4 +- crates/example-types/src/node_types.rs | 14 ++ crates/task-impls/src/consensus/handlers.rs | 3 - crates/task-impls/src/events.rs | 5 - crates/task-impls/src/network.rs | 8 +- crates/task-impls/src/quorum_vote/handlers.rs | 3 - crates/testing/src/consistency_task.rs | 138 +++++++++++------- crates/testing/src/predicates/event.rs | 11 -- crates/testing/src/test_builder.rs | 5 +- crates/testing/src/test_runner.rs | 7 + crates/testing/tests/tests_1/test_success.rs | 27 +++- .../tests_1/upgrade_task_with_consensus.rs | 2 - .../tests/tests_1/upgrade_task_with_vote.rs | 1 - crates/types/src/lib.rs | 14 ++ 14 files changed, 150 insertions(+), 92 deletions(-) diff --git a/crates/example-types/src/block_types.rs b/crates/example-types/src/block_types.rs index 556312a05f..1547e46b9b 100644 --- a/crates/example-types/src/block_types.rs +++ b/crates/example-types/src/block_types.rs @@ -29,6 +29,7 @@ use time::OffsetDateTime; use vbs::version::Version; use crate::{ + auction_results_provider_types::TestAuctionResult, node_types::TestTypes, state_types::{TestInstanceState, TestValidatedState}, testable_delay::{DelayConfig, SupportedTraitTypesForAsyncDelay, TestableDelay}, @@ -274,6 +275,7 @@ impl< BlockHeader = Self, BlockPayload = TestBlockPayload, InstanceState = TestInstanceState, + AuctionResult = TestAuctionResult, >, > BlockHeader for TestBlockHeader { @@ -349,7 +351,7 @@ impl< } fn get_auction_results(&self) -> Option { - unimplemented!() + Some(TYPES::AuctionResult { urls: vec![] }) } } diff --git a/crates/example-types/src/node_types.rs b/crates/example-types/src/node_types.rs index 90e1394326..390b5472d2 100644 --- a/crates/example-types/src/node_types.rs +++ b/crates/example-types/src/node_types.rs @@ -146,3 +146,17 @@ impl Versions for TestVersions { type Marketplace = StaticVersion<0, 3>; } + +#[derive(Clone, Debug, Copy)] +pub struct MarketplaceUpgradeTestVersions {} + +impl Versions for MarketplaceUpgradeTestVersions { + type Base = StaticVersion<0, 2>; + type Upgrade = StaticVersion<0, 3>; + const UPGRADE_HASH: [u8; 32] = [ + 1, 0, 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, + 0, 0, + ]; + + type Marketplace = StaticVersion<0, 3>; +} diff --git a/crates/task-impls/src/consensus/handlers.rs b/crates/task-impls/src/consensus/handlers.rs index 187b486ebb..ffca94fdd8 100644 --- a/crates/task-impls/src/consensus/handlers.rs +++ b/crates/task-impls/src/consensus/handlers.rs @@ -550,9 +550,6 @@ pub async fn handle_quorum_proposal_validated< .await; *decided_certificate_lock = Some(cert.clone()); drop(decided_certificate_lock); - let _ = event_stream - .broadcast(Arc::new(HotShotEvent::UpgradeDecided(cert.clone()))) - .await; } let mut consensus = task_state.consensus.write().await; diff --git a/crates/task-impls/src/events.rs b/crates/task-impls/src/events.rs index ee49a73141..17a9c4fb9b 100644 --- a/crates/task-impls/src/events.rs +++ b/crates/task-impls/src/events.rs @@ -183,8 +183,6 @@ pub enum HotShotEvent { UpgradeVoteSend(UpgradeVote), /// Upgrade certificate has been sent to the network UpgradeCertificateFormed(UpgradeCertificate), - /// A HotShot upgrade was decided - UpgradeDecided(UpgradeCertificate), /* Consensus State Update Events */ /// A undecided view has been created and added to the validated state storage. @@ -421,9 +419,6 @@ impl Display for HotShotEvent { "UpgradeCertificateFormed(view_number={:?})", cert.view_number() ), - HotShotEvent::UpgradeDecided(cert) => { - write!(f, "UpgradeDecided(view_number{:?})", cert.view_number()) - } HotShotEvent::QuorumProposalRequest(view_number) => { write!(f, "QuorumProposalRequest(view_number={view_number:?})") } diff --git a/crates/task-impls/src/network.rs b/crates/task-impls/src/network.rs index fe6712a1c4..3ca3771fa4 100644 --- a/crates/task-impls/src/network.rs +++ b/crates/task-impls/src/network.rs @@ -42,7 +42,6 @@ pub fn quorum_filter(event: &Arc>) -> bool | HotShotEvent::QuorumVoteSend(_) | HotShotEvent::DacSend(_, _) | HotShotEvent::TimeoutVoteSend(_) - | HotShotEvent::UpgradeDecided(_) | HotShotEvent::ViewChange(_) ) } @@ -53,7 +52,6 @@ pub fn upgrade_filter(event: &Arc>) -> bool event.as_ref(), HotShotEvent::UpgradeProposalSend(_, _) | HotShotEvent::UpgradeVoteSend(_) - | HotShotEvent::UpgradeDecided(_) | HotShotEvent::ViewChange(_) ) } @@ -64,7 +62,6 @@ pub fn da_filter(event: &Arc>) -> bool { event.as_ref(), HotShotEvent::DaProposalSend(_, _) | HotShotEvent::DaVoteSend(_) - | HotShotEvent::UpgradeDecided(_) | HotShotEvent::ViewChange(_) ) } @@ -73,9 +70,7 @@ pub fn da_filter(event: &Arc>) -> bool { pub fn vid_filter(event: &Arc>) -> bool { !matches!( event.as_ref(), - HotShotEvent::VidDisperseSend(_, _) - | HotShotEvent::UpgradeDecided(_) - | HotShotEvent::ViewChange(_) + HotShotEvent::VidDisperseSend(_, _) | HotShotEvent::ViewChange(_) ) } @@ -89,7 +84,6 @@ pub fn view_sync_filter(event: &Arc>) -> bo | HotShotEvent::ViewSyncPreCommitVoteSend(_) | HotShotEvent::ViewSyncCommitVoteSend(_) | HotShotEvent::ViewSyncFinalizeVoteSend(_) - | HotShotEvent::UpgradeDecided(_) | HotShotEvent::ViewChange(_) ) } diff --git a/crates/task-impls/src/quorum_vote/handlers.rs b/crates/task-impls/src/quorum_vote/handlers.rs index 3a296ad8c7..a19a7ce5be 100644 --- a/crates/task-impls/src/quorum_vote/handlers.rs +++ b/crates/task-impls/src/quorum_vote/handlers.rs @@ -60,9 +60,6 @@ pub(crate) async fn handle_quorum_proposal_validated< .await; *decided_certificate_lock = Some(cert.clone()); drop(decided_certificate_lock); - let _ = sender - .broadcast(Arc::new(HotShotEvent::UpgradeDecided(cert.clone()))) - .await; } let mut consensus_writer = task_state.consensus.write().await; diff --git a/crates/testing/src/consistency_task.rs b/crates/testing/src/consistency_task.rs index 42c30762c1..2d05c7c4df 100644 --- a/crates/testing/src/consistency_task.rs +++ b/crates/testing/src/consistency_task.rs @@ -21,10 +21,6 @@ use crate::{ test_task::{TestResult, TestTaskState}, }; -trait Validatable { - fn valid(&self) -> Result<()>; -} - /// Map from views to leaves for a single node, allowing multiple leaves for each view (because the node may a priori send us multiple leaves for a given view). pub type NodeMap = BTreeMap<::Time, Vec>>; @@ -32,7 +28,7 @@ pub type NodeMap = BTreeMap<::Time, Vec>>; pub type NodeMapSanitized = BTreeMap<::Time, Leaf>; /// Validate that the `NodeMap` only has a single leaf per view. -pub fn sanitize_node_map( +fn sanitize_node_map( node_map: &NodeMap, ) -> Result> { let mut result = BTreeMap::new(); @@ -58,27 +54,25 @@ pub fn sanitize_node_map( Ok(result) } -/// For a NodeLeafMap, we validate that each leaf extends the preceding leaf. -impl Validatable for NodeMapSanitized { - fn valid(&self) -> Result<()> { - let leaf_pairs = self.values().zip(self.values().skip(1)); +/// For a NodeMapSanitized, we validate that each leaf extends the preceding leaf. +fn validate_node_map(node_map: &NodeMapSanitized) -> Result<()> { + let leaf_pairs = node_map.values().zip(node_map.values().skip(1)); - // Check that the child leaf follows the parent, possibly with a gap. - for (parent, child) in leaf_pairs { - ensure!( + // Check that the child leaf follows the parent, possibly with a gap. + for (parent, child) in leaf_pairs { + ensure!( child.justify_qc().view_number >= parent.view_number(), "The node has provided leaf:\n\n{child:?}\n\nbut its quorum certificate points to a view before the most recent leaf:\n\n{parent:?}" ); - if child.justify_qc().view_number == parent.view_number() - && child.justify_qc().data.leaf_commit != parent.commit() - { - bail!("The node has provided leaf:\n\n{child:?}\n\nwhich points to:\n\n{parent:?}\n\nbut the commits do not match."); - } + if child.justify_qc().view_number == parent.view_number() + && child.justify_qc().data.leaf_commit != parent.commit() + { + bail!("The node has provided leaf:\n\n{child:?}\n\nwhich points to:\n\n{parent:?}\n\nbut the commits do not match."); } - - Ok(()) } + + Ok(()) } /// A map from node ids to `NodeMap`s; note that the latter may have multiple leaves per view in principle. @@ -88,7 +82,7 @@ pub type NetworkMap = BTreeMap>; pub type NetworkMapSanitized = BTreeMap>; /// Validate that each node has only produced one unique leaf per view, and produce a `NetworkMapSanitized`. -pub fn sanitize_network_map( +fn sanitize_network_map( network_map: &NetworkMap, ) -> Result> { let mut result = BTreeMap::new(); @@ -104,49 +98,58 @@ pub fn sanitize_network_map( Ok(result) } -impl Validatable for NetworkMap { - fn valid(&self) -> Result<()> { - let sanitized = sanitize_network_map(self)?; - - sanitized.valid() +pub type ViewMap = BTreeMap<::Time, BTreeMap>>; + +// Invert the network map by interchanging the roles of the node_id and view number. +// +// # Errors +// +// Returns an error if any node map is invalid. +fn invert_network_map( + network_map: &NetworkMapSanitized, +) -> Result> { + let mut inverted_map = BTreeMap::new(); + for (node_id, node_map) in network_map.iter() { + validate_node_map(node_map) + .context(format!("Node {node_id} has an invalid leaf history"))?; + + // validate each node's leaf map + for (view, leaf) in node_map.iter() { + let view_map = inverted_map.entry(*view).or_insert(BTreeMap::new()); + view_map.insert(*node_id, leaf.clone()); + } } + + Ok(inverted_map) } -/// For a NetworkLeafMap, we validate that no two nodes have submitted differing leaves for any given view, in addition to the individual NodeLeafMap checks. -impl Validatable for NetworkMapSanitized { - fn valid(&self) -> Result<()> { - // Invert the map by interchanging the roles of the node_id and view number. - let mut inverted_map = BTreeMap::new(); - for (node_id, node_map) in self.iter() { - node_map - .valid() - .context(format!("Node {node_id} has an invalid leaf history"))?; - - // validate each node's leaf map - for (view, leaf) in node_map.iter() { - let view_map = inverted_map.entry(*view).or_insert(BTreeMap::new()); - view_map.insert(*node_id, leaf.clone()); - } - } +/// A view map, sanitized to have exactly one leaf per view. +pub type ViewMapSanitized = BTreeMap<::Time, Leaf>; - for (view, view_map) in inverted_map.iter() { - let mut leaves: Vec<_> = view_map.iter().collect(); +fn sanitize_view_map( + view_map: &ViewMap, +) -> Result> { + let mut result = BTreeMap::new(); - leaves.dedup_by(|(_node_a, leaf_a), (_node_b, leaf_b)| leaf_a == leaf_b); + for (view, leaf_map) in view_map.iter() { + let mut node_leaves: Vec<_> = leaf_map.iter().collect(); - ensure!( - leaves.len() <= 1, - view_map.iter().fold( - format!("The network does not agree on view {view:?}."), - |acc, (node, leaf)| { - format!("{acc}\n\nNode {node} sent us leaf:\n\n{leaf:?}") - } - ) - ); - } + node_leaves.dedup_by(|(_node_a, leaf_a), (_node_b, leaf_b)| leaf_a == leaf_b); - Ok(()) + ensure!( + node_leaves.len() <= 1, + leaf_map.iter().fold( + format!("The network does not agree on view {view:?}."), + |acc, (node, leaf)| { format!("{acc}\n\nNode {node} sent us leaf:\n\n{leaf:?}") } + ) + ); + + if let Some(leaf) = node_leaves.first() { + result.insert(*view, leaf.1.clone()); + } } + + Ok(result) } /// Data availability task state @@ -155,6 +158,29 @@ pub struct ConsistencyTask { pub consensus_leaves: NetworkMap, /// safety task requirements pub safety_properties: OverallSafetyPropertiesDescription, + /// whether we should have seen an upgrade certificate or not + pub ensure_upgrade: bool, +} + +impl ConsistencyTask { + pub fn validate(&self) -> Result<()> { + let sanitized_network_map = sanitize_network_map(&self.consensus_leaves)?; + + let inverted_map = invert_network_map(&sanitized_network_map)?; + + let sanitized_view_map = sanitize_view_map(&inverted_map)?; + + let expected_upgrade = self.ensure_upgrade; + let actual_upgrade = sanitized_view_map.iter().fold(false, |acc, (_view, leaf)| { + acc || leaf.upgrade_certificate().is_some() + }); + + ensure!(expected_upgrade == actual_upgrade, + "Mismatch between expected and actual upgrade. Expected upgrade: {expected_upgrade}. Actual upgrade: {actual_upgrade}" + ); + + Ok(()) + } } #[async_trait] @@ -181,7 +207,7 @@ impl TestTaskState for ConsistencyTask { } fn check(&self) -> TestResult { - if let Err(e) = self.consensus_leaves.valid() { + if let Err(e) = self.validate() { return TestResult::Fail(Box::new(e)); } diff --git a/crates/testing/src/predicates/event.rs b/crates/testing/src/predicates/event.rs index 8b740b8482..d933184c1e 100644 --- a/crates/testing/src/predicates/event.rs +++ b/crates/testing/src/predicates/event.rs @@ -147,17 +147,6 @@ where Box::new(EventPredicate { check, info }) } -pub fn upgrade_decided() -> Box> -where - TYPES: NodeType, -{ - let info = "UpgradeDecided".to_string(); - let check: EventCallback = - Arc::new(move |e: Arc>| matches!(e.as_ref(), UpgradeDecided(_))); - - Box::new(EventPredicate { check, info }) -} - pub fn quorum_vote_send() -> Box> where TYPES: NodeType, diff --git a/crates/testing/src/test_builder.rs b/crates/testing/src/test_builder.rs index 9a654e619e..2cb521b08b 100644 --- a/crates/testing/src/test_builder.rs +++ b/crates/testing/src/test_builder.rs @@ -95,6 +95,8 @@ pub struct TestDescription, V: Ver pub behaviour: Rc Behaviour>, /// Delay config if any to add delays to asynchronous calls pub async_delay_config: DelayConfig, + /// view in which to propose an upgrade + pub upgrade_view: Option, } #[derive(Debug)] @@ -377,6 +379,7 @@ impl, V: Versions> Default }, behaviour: Rc::new(|_| Behaviour::Standard), async_delay_config: DelayConfig::default(), + upgrade_view: None, } } } @@ -509,7 +512,7 @@ where config, marketplace_config: Box::new(|_| MarketplaceConfig:: { auction_results_provider: TestAuctionResultsProvider::::default().into(), - fallback_builder_url: Url::parse("http://localhost").unwrap(), + fallback_builder_url: Url::parse("http://localhost:9999").unwrap(), }), }, metadata: self, diff --git a/crates/testing/src/test_runner.rs b/crates/testing/src/test_runner.rs index 372e3ece3b..6167d23698 100644 --- a/crates/testing/src/test_runner.rs +++ b/crates/testing/src/test_runner.rs @@ -208,6 +208,7 @@ where let consistency_task_state = ConsistencyTask { consensus_leaves: BTreeMap::new(), safety_properties: self.launcher.metadata.overall_safety_properties, + ensure_upgrade: self.launcher.metadata.upgrade_view.is_some(), }; let consistency_task = TestTask::>::new( @@ -403,6 +404,9 @@ where for i in 0..total { let mut config = config.clone(); + if let Some(upgrade_view) = self.launcher.metadata.upgrade_view { + config.set_view_upgrade(upgrade_view); + } let node_id = self.next_node_id; self.next_node_id += 1; tracing::debug!("launch node {}", i); @@ -451,6 +455,8 @@ where marketplace_config.auction_results_provider = new_auction_results_provider.into(); } + marketplace_config.fallback_builder_url = builder_urls.first().unwrap().clone(); + let network_clone = network.clone(); let networks_ready_future = async move { network_clone.wait_for_ready().await; @@ -487,6 +493,7 @@ where // We assign node's public key and stake value rather than read from config file since it's a test let validator_config = ValidatorConfig::generated_from_seed_indexed([0u8; 32], node_id, 1, is_da); + let hotshot = Self::add_node_with_config( node_id, network.clone(), diff --git a/crates/testing/tests/tests_1/test_success.rs b/crates/testing/tests/tests_1/test_success.rs index ef955e13d7..5a2b141cdd 100644 --- a/crates/testing/tests/tests_1/test_success.rs +++ b/crates/testing/tests/tests_1/test_success.rs @@ -8,7 +8,10 @@ use std::{rc::Rc, time::Duration}; use hotshot::tasks::{BadProposalViewDos, DoubleProposeVote}; use hotshot_example_types::{ - node_types::{Libp2pImpl, MemoryImpl, PushCdnImpl, TestConsecutiveLeaderTypes, TestVersions}, + node_types::{ + Libp2pImpl, MarketplaceUpgradeTestVersions, MemoryImpl, PushCdnImpl, + TestConsecutiveLeaderTypes, TestVersions, + }, state_types::TestTypes, testable_delay::{DelayConfig, DelayOptions, DelaySettings, SupportedTraitTypesForAsyncDelay}, }; @@ -39,6 +42,26 @@ cross_tests!( }, ); +cross_tests!( + TestName: test_success_marketplace, + Impls: [MemoryImpl], + Types: [TestTypes], + Versions: [MarketplaceUpgradeTestVersions], + Ignore: false, + Metadata: { + TestDescription { + // allow more time to pass in CI + completion_task_description: CompletionTaskDescription::TimeBasedCompletionTaskBuilder( + TimeBasedCompletionTaskDescription { + duration: Duration::from_secs(60), + }, + ), + upgrade_view: Some(5), + ..TestDescription::default() + } + }, +); + cross_tests!( TestName: test_success_with_async_delay, Impls: [MemoryImpl, Libp2pImpl, PushCdnImpl], @@ -57,7 +80,7 @@ cross_tests!( }; metadata.overall_safety_properties.num_failed_views = 0; - metadata.overall_safety_properties.num_successful_views = 10; + metadata.overall_safety_properties.num_successful_views = 0; let mut config = DelayConfig::default(); let delay_settings = DelaySettings { delay_option: DelayOptions::Random, diff --git a/crates/testing/tests/tests_1/upgrade_task_with_consensus.rs b/crates/testing/tests/tests_1/upgrade_task_with_consensus.rs index d196356455..9c4ada9ae4 100644 --- a/crates/testing/tests/tests_1/upgrade_task_with_consensus.rs +++ b/crates/testing/tests/tests_1/upgrade_task_with_consensus.rs @@ -177,7 +177,6 @@ async fn test_upgrade_task_vote() { exact(ViewChange(ViewNumber::new(6))), validated_state_updated(), quorum_proposal_validated(), - upgrade_decided(), leaf_decided(), ], task_state_asserts: vec![decided_upgrade_certificate()], @@ -604,7 +603,6 @@ async fn test_upgrade_task_blank_blocks() { exact(ViewChange(ViewNumber::new(5))), validated_state_updated(), quorum_proposal_validated(), - upgrade_decided(), leaf_decided(), // This is between versions, but we are receiving a null block and hence should vote affirmatively on it. quorum_vote_send(), diff --git a/crates/testing/tests/tests_1/upgrade_task_with_vote.rs b/crates/testing/tests/tests_1/upgrade_task_with_vote.rs index 5613f66734..7fa5d3bf40 100644 --- a/crates/testing/tests/tests_1/upgrade_task_with_vote.rs +++ b/crates/testing/tests/tests_1/upgrade_task_with_vote.rs @@ -181,7 +181,6 @@ async fn test_upgrade_task_with_vote() { ), Expectations::from_outputs_and_task_states( all_predicates![ - upgrade_decided(), exact(LockedViewUpdated(ViewNumber::new(4))), exact(LastDecidedViewUpdated(ViewNumber::new(3))), leaf_decided(), diff --git a/crates/types/src/lib.rs b/crates/types/src/lib.rs index 566cebeecf..6206497700 100644 --- a/crates/types/src/lib.rs +++ b/crates/types/src/lib.rs @@ -227,3 +227,17 @@ pub struct HotShotConfig { /// Unix time in seconds at which we stop voting on an upgrade. To prevent voting on an upgrade, set stop_voting_time <= start_voting_time. pub stop_voting_time: u64, } + +impl HotShotConfig { + /// Update a hotshot config to have a view-based upgrade. + pub fn set_view_upgrade(&mut self, view: u64) { + self.start_proposing_view = view; + self.stop_proposing_view = view + 1; + self.start_voting_view = view.saturating_sub(1); + self.stop_voting_view = view + 10; + self.start_proposing_time = 0; + self.stop_proposing_time = u64::MAX; + self.start_voting_time = 0; + self.stop_voting_time = u64::MAX; + } +}