From 5a66b5b9303dcbcf7dc4c1aff8427d8f06c7d83d Mon Sep 17 00:00:00 2001 From: Marcin S Date: Fri, 3 Feb 2023 15:00:13 +0100 Subject: [PATCH 01/18] Add `manifest_import_returns_ok_true` test --- .../src/vstaging/grid.rs | 70 ++++++++++++++++++- 1 file changed, 67 insertions(+), 3 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index 939851939de9..f104d7df9c5a 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -365,7 +365,7 @@ impl GridTracker { Some(g) => g, }; - // advertise onwards ad accept received advertisements + // advertise onwards and accept received advertisements let sending_group_manifests = group_topology.sending.iter().map(|v| (*v, ManifestKind::Full)); @@ -1518,14 +1518,78 @@ mod tests { ); } - // TODO [now]: test that senders can provide manifests in acknowledgement + #[test] + fn senders_can_provide_manifests_in_acknowledgement() { + todo!() + } // TODO [now]: check that pending communication is set correctly when receiving a manifest on a confirmed candidate // It should also overwrite any existing `Full` ManifestKind // TODO [now]: check that pending communication is cleared correctly in `manifest_sent_to` - // TODO [now]: test a scenario where manifest import returns `Ok(true)`. + // Test a scenario where manifest import returns `Ok(true)`. + #[test] + fn manifest_import_returns_ok_true() { + let mut tracker = GridTracker::default(); + let session_topology = SessionTopologyView { + group_views: vec![( + GroupIndex(0), + GroupSubView { + sending: HashSet::new(), + receiving: vec![ValidatorIndex(0)].into_iter().collect(), + }, + )] + .into_iter() + .collect(), + }; + + let groups = Groups::new( + vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), + &[ + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + ], + ); + + let candidate_hash = CandidateHash(Hash::repeat_byte(42)); + + let group_index = GroupIndex(0); + let group_size = 3; + + assert_eq!(groups.get_size_and_backing_threshold(group_index), Some((group_size, 2))); + + let local_knowledge = StatementFilter::new(group_size); + + // Add the candidate as backed. + tracker.add_backed_candidate( + &session_topology, + candidate_hash, + group_index, + group_size, + local_knowledge + ); + + // Import manifest. + assert_matches!( + tracker.import_manifest( + &session_topology, + &groups, + candidate_hash, + 3, + ManifestSummary { + claimed_parent_hash: Hash::repeat_byte(0), + claimed_group_index: group_index, + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, + ManifestKind::Full, + ValidatorIndex(0), + ), + Ok(true) + ); + } // TODO [now]: test that pending statements are updated after manifest exchange From 8e95fba91b50905a3d98e79fdb4aa00cac40f1b3 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Fri, 3 Feb 2023 15:00:52 +0100 Subject: [PATCH 02/18] cargo fmt --- .../src/vstaging/grid.rs | 59 ++++++++++--------- .../src/vstaging/mod.rs | 47 ++++++++++----- 2 files changed, 63 insertions(+), 43 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index f104d7df9c5a..fd2a73b42639 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -299,15 +299,14 @@ impl GridTracker { // to `pending_statements` for this validator. confirmed.manifest_received_from(sender, remote_knowledge); if let Some(pending_statements) = confirmed.pending_statements(sender) { - self.pending_statements - .entry(sender) - .or_default() - .extend(decompose_statement_filter( + self.pending_statements.entry(sender).or_default().extend( + decompose_statement_filter( groups, claimed_group_index, candidate_hash, &pending_statements, - )); + ), + ); } } else { // received prevents conflicting manifests so this is max 1 per validator. @@ -407,15 +406,14 @@ impl GridTracker { c.manifest_sent_to(validator_index, local_knowledge); if let Some(pending_statements) = c.pending_statements(validator_index) { - self.pending_statements - .entry(validator_index) - .or_default() - .extend(decompose_statement_filter( + self.pending_statements.entry(validator_index).or_default().extend( + decompose_statement_filter( groups, c.group_index, candidate_hash, &pending_statements, - )); + ), + ); } } @@ -471,8 +469,10 @@ impl GridTracker { &self, validator_index: ValidatorIndex, ) -> Vec<(ValidatorIndex, CompactStatement)> { - let mut v = self.pending_statements - .get(&validator_index).map(|x| x.iter().cloned().collect()) + let mut v = self + .pending_statements + .get(&validator_index) + .map(|x| x.iter().cloned().collect()) .unwrap_or(Vec::new()); v.sort_by_key(|(_, s)| match s { @@ -551,21 +551,25 @@ impl GridTracker { originator: ValidatorIndex, statement: &CompactStatement, ) { - let (g, c_h, kind, in_group) = match extract_statement_and_group_info(groups, originator, statement) { - None => return, - Some(x) => x, - }; + let (g, c_h, kind, in_group) = + match extract_statement_and_group_info(groups, originator, statement) { + None => return, + Some(x) => x, + }; let known = match self.confirmed_backed.get_mut(&c_h) { None => return, Some(x) => x, }; - if !known.note_fresh_statement(in_group, kind) { return } + if !known.note_fresh_statement(in_group, kind) { + return + } // Add to `pending_statements` for all valdiators we communicate with // who have exchanged manifests. - let all_group_validators = session_topology.group_views + let all_group_validators = session_topology + .group_views .get(&g) .into_iter() .flat_map(|g| g.sending.iter().chain(g.receiving.iter())); @@ -623,18 +627,22 @@ fn extract_statement_and_group_info( Some((group, *candidate_hash, statement_kind, index_in_group)) } -fn decompose_statement_filter<'a> ( +fn decompose_statement_filter<'a>( groups: &'a Groups, group_index: GroupIndex, candidate_hash: CandidateHash, statement_filter: &'a StatementFilter, ) -> impl Iterator + 'a { groups.get(group_index).into_iter().flat_map(move |g| { - let s = statement_filter.seconded_in_group.iter_ones() + let s = statement_filter + .seconded_in_group + .iter_ones() .map(|i| g[i].clone()) .map(move |i| (i, CompactStatement::Seconded(candidate_hash))); - let v = statement_filter.validated_in_group.iter_ones() + let v = statement_filter + .validated_in_group + .iter_ones() .map(|i| g[i].clone()) .map(move |i| (i, CompactStatement::Valid(candidate_hash))); @@ -1005,10 +1013,7 @@ impl KnownBackedCandidate { .unwrap_or(false) } - fn pending_statements( - &self, - validator: ValidatorIndex, - ) -> Option { + fn pending_statements(&self, validator: ValidatorIndex) -> Option { // existence of both remote & local knowledge indicate we have exchanged // manifests. // then, everything that is not in the remote knowledge is pending, and we @@ -1027,8 +1032,6 @@ impl KnownBackedCandidate { !remote.validated_in_group.clone(), }) } - - } #[cfg(test)] @@ -1568,7 +1571,7 @@ mod tests { candidate_hash, group_index, group_size, - local_knowledge + local_knowledge, ); // Import manifest. diff --git a/node/network/statement-distribution/src/vstaging/mod.rs b/node/network/statement-distribution/src/vstaging/mod.rs index d8d2c44f9ecc..03177006d091 100644 --- a/node/network/statement-distribution/src/vstaging/mod.rs +++ b/node/network/statement-distribution/src/vstaging/mod.rs @@ -676,9 +676,12 @@ fn pending_statement_network_message( originator: ValidatorIndex, compact: CompactStatement, ) -> Option<(Vec, net_protocol::VersionedValidationProtocol)> { - statement_store.validator_statement(originator, compact) + statement_store + .validator_statement(originator, compact) .map(|s| s.as_unchecked().clone()) - .map(|signed| protocol_vstaging::StatementDistributionMessage::Statement(relay_parent, signed)) + .map(|signed| { + protocol_vstaging::StatementDistributionMessage::Statement(relay_parent, signed) + }) .map(|msg| (vec![peer.clone()], Versioned::VStaging(msg).into())) } @@ -712,7 +715,9 @@ async fn send_pending_cluster_statements( }) .collect::>(); - if network_messages.is_empty() { return } + if network_messages.is_empty() { + return + } ctx.send_message(NetworkBridgeTxMessage::SendValidationMessages(network_messages)) .await; @@ -781,7 +786,12 @@ async fn send_pending_grid_messages( .expect("determined to be some earlier in this function; qed") .grid_tracker; - grid.manifest_sent_to(groups, peer_validator_id, candidate_hash, local_knowledge.clone()); + grid.manifest_sent_to( + groups, + peer_validator_id, + candidate_hash, + local_knowledge.clone(), + ); messages.push(( vec![peer_id.clone()], @@ -814,15 +824,16 @@ async fn send_pending_grid_messages( // otherwise, we might receive statements while the grid peer is "out of view" and then // not send them when they get back "in view". problem! { - let grid_tracker = &mut relay_parent_state.local_validator.as_mut() + let grid_tracker = &mut relay_parent_state + .local_validator + .as_mut() .expect("checked earlier; qed") .grid_tracker; let pending_statements = grid_tracker.all_pending_statements_for(peer_validator_id); - let extra_statements = pending_statements - .into_iter() - .filter_map(|(originator, compact)| { + let extra_statements = + pending_statements.into_iter().filter_map(|(originator, compact)| { let res = pending_statement_network_message( &relay_parent_state.statement_store, relay_parent, @@ -846,7 +857,9 @@ async fn send_pending_grid_messages( messages.extend(extra_statements); } - if messages.is_empty() { return } + if messages.is_empty() { + return + } ctx.send_message(NetworkBridgeTxMessage::SendValidationMessages(messages)).await; } @@ -1674,7 +1687,12 @@ async fn provide_candidate_to_grid( grid::ManifestKind::Acknowledgement => ack_peers.push(p), } - local_validator.grid_tracker.manifest_sent_to(&per_session.groups, v, candidate_hash, filter.clone()); + local_validator.grid_tracker.manifest_sent_to( + &per_session.groups, + v, + candidate_hash, + filter.clone(), + ); post_statements.extend( post_acknowledgement_statement_messages( v, @@ -1971,11 +1989,10 @@ fn post_acknowledgement_statement_messages( group_index: GroupIndex, candidate_hash: CandidateHash, ) -> Vec { - let sending_filter = - match grid_tracker.pending_statements_for(recipient, candidate_hash) { - None => return Vec::new(), - Some(f) => f, - }; + let sending_filter = match grid_tracker.pending_statements_for(recipient, candidate_hash) { + None => return Vec::new(), + Some(f) => f, + }; let mut messages = Vec::new(); for statement in From 0c551bdd11c935a9baf1c12666714874e1508cee Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 5 Feb 2023 14:45:50 +0100 Subject: [PATCH 03/18] Add pending_communication_receiving_manifest_on_confirmed_candidate --- .../src/vstaging/grid.rs | 143 ++++++++++++++---- 1 file changed, 116 insertions(+), 27 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index fd2a73b42639..47d151946d29 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -1440,7 +1440,7 @@ mod tests { GroupIndex(0), GroupSubView { sending: HashSet::new(), - receiving: vec![ValidatorIndex(0)].into_iter().collect(), + receiving: HashSet::from([ValidatorIndex(0)]), }, )] .into_iter() @@ -1526,21 +1526,99 @@ mod tests { todo!() } - // TODO [now]: check that pending communication is set correctly when receiving a manifest on a confirmed candidate - // It should also overwrite any existing `Full` ManifestKind + // Check that pending communication is set correctly when receiving a manifest on a confirmed candidate. + // + // It should also overwrite any existing `Full` ManifestKind. + #[test] + fn pending_communication_receiving_manifest_on_confirmed_candidate() { + let validator_index = ValidatorIndex(0); + + let mut tracker = GridTracker::default(); + let session_topology = SessionTopologyView { + group_views: vec![( + GroupIndex(0), + GroupSubView { + sending: HashSet::from([validator_index]), + receiving: HashSet::from([ValidatorIndex(1)]), + }, + )] + .into_iter() + .collect(), + }; + + let groups = Groups::new( + vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), + &[ + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + ], + ); + + let candidate_hash = CandidateHash(Hash::repeat_byte(42)); + let group_index = GroupIndex(0); + let group_size = 3; + let local_knowledge = StatementFilter::new(group_size); + + assert_eq!(groups.get_size_and_backing_threshold(group_index), Some((group_size, 2))); + + // Manifest should not be pending yet. + let pending_manifest = tracker.is_manifest_pending_for(validator_index, &candidate_hash); + assert_eq!(pending_manifest, None); + + // Add the candidate as backed. + tracker.add_backed_candidate( + &session_topology, + candidate_hash, + group_index, + group_size, + local_knowledge.clone(), + ); + + // Manifest should be pending as `Full`. + let pending_manifest = tracker.is_manifest_pending_for(validator_index, &candidate_hash); + assert_eq!(pending_manifest, Some(ManifestKind::Full)); + + // Note the manifest as 'sent' to validator 0. + tracker.manifest_sent_to(&groups, validator_index, candidate_hash, local_knowledge); + + // Import manifest. + // + // Should overwrite existing `Full` manifest. + let ack = tracker.import_manifest( + &session_topology, + &groups, + candidate_hash, + 3, + ManifestSummary { + claimed_parent_hash: Hash::repeat_byte(0), + claimed_group_index: group_index, + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, + ManifestKind::Acknowledgement, + validator_index, + ); + assert_matches!(ack, Ok(false)); - // TODO [now]: check that pending communication is cleared correctly in `manifest_sent_to` + let pending_manifest = tracker.is_manifest_pending_for(validator_index, &candidate_hash); + assert_eq!(pending_manifest, None); + } - // Test a scenario where manifest import returns `Ok(true)`. + // Check that pending communication is cleared correctly in `manifest_sent_to` + // + // Also test a scenario where manifest import returns `Ok(true)`. #[test] - fn manifest_import_returns_ok_true() { + fn pending_communication_is_cleared() { + let validator_index = ValidatorIndex(0); + let mut tracker = GridTracker::default(); let session_topology = SessionTopologyView { group_views: vec![( GroupIndex(0), GroupSubView { sending: HashSet::new(), - receiving: vec![ValidatorIndex(0)].into_iter().collect(), + receiving: HashSet::from([validator_index]), }, )] .into_iter() @@ -1557,41 +1635,52 @@ mod tests { ); let candidate_hash = CandidateHash(Hash::repeat_byte(42)); - let group_index = GroupIndex(0); let group_size = 3; + let local_knowledge = StatementFilter::new(group_size); assert_eq!(groups.get_size_and_backing_threshold(group_index), Some((group_size, 2))); - let local_knowledge = StatementFilter::new(group_size); - // Add the candidate as backed. tracker.add_backed_candidate( &session_topology, candidate_hash, group_index, group_size, - local_knowledge, + local_knowledge.clone(), ); + // Manifest should not be pending yet. + let pending_manifest = tracker.is_manifest_pending_for(validator_index, &candidate_hash); + assert_eq!(pending_manifest, None); + // Import manifest. - assert_matches!( - tracker.import_manifest( - &session_topology, - &groups, - candidate_hash, - 3, - ManifestSummary { - claimed_parent_hash: Hash::repeat_byte(0), - claimed_group_index: group_index, - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], - }, - ManifestKind::Full, - ValidatorIndex(0), - ), - Ok(true) + let ack = tracker.import_manifest( + &session_topology, + &groups, + candidate_hash, + 3, + ManifestSummary { + claimed_parent_hash: Hash::repeat_byte(0), + claimed_group_index: group_index, + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, + ManifestKind::Full, + validator_index, ); + assert_matches!(ack, Ok(true)); + + // Acknowledgement manifest should be pending. + let pending_manifest = tracker.is_manifest_pending_for(validator_index, &candidate_hash); + assert_eq!(pending_manifest, Some(ManifestKind::Acknowledgement)); + + // Note the candidate as advertised. + tracker.manifest_sent_to(&groups, validator_index, candidate_hash, local_knowledge); + + // Pending manifest should be cleared. + let pending_manifest = tracker.is_manifest_pending_for(validator_index, &candidate_hash); + assert_eq!(pending_manifest, None); } // TODO [now]: test that pending statements are updated after manifest exchange From 443df48a05234a81e9a96235adf8298925d4965e Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 5 Feb 2023 15:38:23 +0100 Subject: [PATCH 04/18] Add `senders_can_provide_manifests_in_acknowledgement` test --- .../src/vstaging/grid.rs | 60 ++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index 47d151946d29..3b1d21f29323 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -1523,7 +1523,65 @@ mod tests { #[test] fn senders_can_provide_manifests_in_acknowledgement() { - todo!() + let validator_index = ValidatorIndex(0); + + let mut tracker = GridTracker::default(); + let session_topology = SessionTopologyView { + group_views: vec![( + GroupIndex(0), + GroupSubView { + sending: HashSet::from([validator_index]), + receiving: HashSet::from([ValidatorIndex(1)]), + }, + )] + .into_iter() + .collect(), + }; + + let groups = Groups::new( + vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), + &[ + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + ], + ); + + let candidate_hash = CandidateHash(Hash::repeat_byte(42)); + let group_index = GroupIndex(0); + let group_size = 3; + let local_knowledge = StatementFilter::new(group_size); + + assert_eq!(groups.get_size_and_backing_threshold(group_index), Some((group_size, 2))); + + // Add the candidate as backed. + tracker.add_backed_candidate( + &session_topology, + candidate_hash, + group_index, + group_size, + local_knowledge.clone(), + ); + + // Note the manifest as 'sent' to validator 0. + tracker.manifest_sent_to(&groups, validator_index, candidate_hash, local_knowledge); + + // Import manifest. + let ack = tracker.import_manifest( + &session_topology, + &groups, + candidate_hash, + 3, + ManifestSummary { + claimed_parent_hash: Hash::repeat_byte(0), + claimed_group_index: group_index, + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, + ManifestKind::Acknowledgement, + validator_index, + ); + assert_matches!(ack, Ok(false)); } // Check that pending communication is set correctly when receiving a manifest on a confirmed candidate. From 28258c3ad82637e0968a77e742fc9e71ecd5e6c8 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 5 Feb 2023 17:45:00 +0100 Subject: [PATCH 05/18] Add a couple of tests for pending statements --- .../src/vstaging/grid.rs | 171 +++++++++++++++++- 1 file changed, 166 insertions(+), 5 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index 3b1d21f29323..61c5ac6b2cfc 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -40,7 +40,7 @@ use super::{groups::Groups, LOG_TARGET}; /// /// In the case that this group is the group that we are locally assigned to, /// the 'receiving' side will be empty. -#[derive(PartialEq)] +#[derive(Debug, PartialEq)] struct GroupSubView { sending: HashSet, receiving: HashSet, @@ -566,7 +566,7 @@ impl GridTracker { return } - // Add to `pending_statements` for all valdiators we communicate with + // Add to `pending_statements` for all validators we communicate with // who have exchanged manifests. let all_group_validators = session_topology .group_views @@ -827,7 +827,7 @@ enum StatementKind { /// Bitfields indicating the statements that are known or undesired /// about a candidate. -#[derive(Clone)] +#[derive(Debug, Clone, PartialEq)] pub struct StatementFilter { /// Seconded statements. '1' is known or undesired. pub seconded_in_group: BitVec, @@ -1741,9 +1741,170 @@ mod tests { assert_eq!(pending_manifest, None); } - // TODO [now]: test that pending statements are updated after manifest exchange + #[test] + fn pending_statements_are_updated_after_manifest_exchange() { + let mut tracker = GridTracker::default(); + + let validator_index = ValidatorIndex(0); + let candidate_hash = CandidateHash(Hash::repeat_byte(42)); + let group_size = 3; + + // Should start with no pending statements. + assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); + assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); + + let statement_filter = StatementFilter::new(group_size); + + todo!() + } + + #[test] + fn invalid_fresh_statement_import() { + let validator_index = ValidatorIndex(0); + + let mut tracker = GridTracker::default(); + let session_topology = SessionTopologyView { + group_views: vec![( + GroupIndex(0), + GroupSubView { + sending: HashSet::new(), + receiving: HashSet::from([validator_index]), + }, + )] + .into_iter() + .collect(), + }; + + let groups = Groups::new( + vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), + &[ + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + ], + ); + + let candidate_hash = CandidateHash(Hash::repeat_byte(42)); + let group_index = GroupIndex(0); + let group_size = 3; + let local_knowledge = StatementFilter::new(group_size); + + // Should start with no pending statements. + assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); + assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); + + // Import fresh statement. Candidate not backed. + let statement = CompactStatement::Seconded(candidate_hash); + tracker.learned_fresh_statement(&groups, &session_topology, ValidatorIndex(1), &statement); + + assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); + assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); + + // Add the candidate as backed. + tracker.add_backed_candidate( + &session_topology, + candidate_hash, + group_index, + group_size, + local_knowledge.clone(), + ); + + // Import fresh statement. Unknown group for validator index. + let statement = CompactStatement::Seconded(candidate_hash); + tracker.learned_fresh_statement(&groups, &session_topology, ValidatorIndex(1), &statement); + + assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); + assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); + } + + #[test] + fn pending_statements_updated_when_importing_fresh_statement() { + let validator_index = ValidatorIndex(0); - // TODO [now]: test that pending statements are updated when importing a fresh statement + let mut tracker = GridTracker::default(); + let session_topology = SessionTopologyView { + group_views: vec![( + GroupIndex(0), + GroupSubView { + sending: HashSet::new(), + receiving: HashSet::from([validator_index]), + }, + )] + .into_iter() + .collect(), + }; + + let groups = Groups::new( + vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), + &[ + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + ], + ); + + let candidate_hash = CandidateHash(Hash::repeat_byte(42)); + let group_index = GroupIndex(0); + let group_size = 3; + let local_knowledge = StatementFilter::new(group_size); + + // Should start with no pending statements. + assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); + assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); + + // Add the candidate as backed. + tracker.add_backed_candidate( + &session_topology, + candidate_hash, + group_index, + group_size, + local_knowledge.clone(), + ); + + // Import fresh statement. + let ack = tracker.import_manifest( + &session_topology, + &groups, + candidate_hash, + 3, + ManifestSummary { + claimed_parent_hash: Hash::repeat_byte(0), + claimed_group_index: group_index, + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, + ManifestKind::Full, + validator_index, + ); + tracker.manifest_sent_to(&groups, validator_index, candidate_hash, local_knowledge); + let statement = CompactStatement::Seconded(candidate_hash); + tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); + + // There should be pending statements now. + let statements = StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + }; + assert_eq!( + tracker.pending_statements_for(validator_index, candidate_hash), + Some(statements.clone()) + ); + assert_eq!( + tracker.all_pending_statements_for(validator_index), + vec![(ValidatorIndex(0), CompactStatement::Seconded(candidate_hash))] + ); + + // After successful import, try importing again. Nothing should change. + tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); + assert_eq!( + tracker.pending_statements_for(validator_index, candidate_hash), + Some(statements) + ); + assert_eq!( + tracker.all_pending_statements_for(validator_index), + vec![(ValidatorIndex(0), CompactStatement::Seconded(candidate_hash))] + ); + } // TODO [now]: test that pending statements respect remote knowledge From 5f6145c9581d36a09f0c8c49e77d2beb1785a6c1 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 5 Feb 2023 18:51:05 +0100 Subject: [PATCH 06/18] Add `pending_statements_cleared_when_sending` test --- .../src/vstaging/grid.rs | 123 +++++++++++++++++- 1 file changed, 121 insertions(+), 2 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index 61c5ac6b2cfc..5658a91fb090 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -819,7 +819,7 @@ fn updating_ensure_within_seconding_limit( true } -#[derive(Clone, Copy)] +#[derive(Debug, Clone, Copy)] enum StatementKind { Seconded, Valid, @@ -863,6 +863,7 @@ impl StatementFilter { } } +#[derive(Debug, Clone)] struct MutualKnowledge { // Knowledge they have about the candidate. `Some` only if they // have advertised or requested the candidate. @@ -875,6 +876,7 @@ struct MutualKnowledge { // A utility struct for keeping track of metadata about candidates // we have confirmed as having been backed. +#[derive(Debug, Clone)] struct KnownBackedCandidate { group_index: GroupIndex, local_knowledge: StatementFilter, @@ -1908,5 +1910,122 @@ mod tests { // TODO [now]: test that pending statements respect remote knowledge - // TODO [now]: test that pending statements are cleared when sending/receiving. + #[test] + fn pending_statements_cleared_when_sending() { + let validator_index = ValidatorIndex(0); + let counterparty = ValidatorIndex(1); + + let mut tracker = GridTracker::default(); + let session_topology = SessionTopologyView { + group_views: vec![( + GroupIndex(0), + GroupSubView { + sending: HashSet::new(), + receiving: HashSet::from([validator_index, counterparty]), + }, + )] + .into_iter() + .collect(), + }; + + let groups = Groups::new( + vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), + &[ + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + ], + ); + + let candidate_hash = CandidateHash(Hash::repeat_byte(42)); + let group_index = GroupIndex(0); + let group_size = 3; + let local_knowledge = StatementFilter::new(group_size); + + // Should start with no pending statements. + assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); + assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); + + // Add the candidate as backed. + tracker.add_backed_candidate( + &session_topology, + candidate_hash, + group_index, + group_size, + local_knowledge.clone(), + ); + + // Import statement for originator. + let ack = tracker.import_manifest( + &session_topology, + &groups, + candidate_hash, + 3, + ManifestSummary { + claimed_parent_hash: Hash::repeat_byte(0), + claimed_group_index: group_index, + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, + ManifestKind::Full, + validator_index, + ); + tracker.manifest_sent_to(&groups, validator_index, candidate_hash, local_knowledge.clone()); + let statement = CompactStatement::Seconded(candidate_hash); + tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); + + // Import statement for counterparty. + let ack = tracker.import_manifest( + &session_topology, + &groups, + candidate_hash, + 3, + ManifestSummary { + claimed_parent_hash: Hash::repeat_byte(0), + claimed_group_index: group_index, + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, + ManifestKind::Full, + counterparty, + ); + tracker.manifest_sent_to(&groups, counterparty, candidate_hash, local_knowledge); + let statement = CompactStatement::Seconded(candidate_hash); + tracker.learned_fresh_statement(&groups, &session_topology, counterparty, &statement); + + // There should be pending statements now. + let statements = StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + }; + assert_eq!( + tracker.pending_statements_for(validator_index, candidate_hash), + Some(statements.clone()) + ); + assert_eq!( + tracker.all_pending_statements_for(validator_index), + vec![(ValidatorIndex(0), CompactStatement::Seconded(candidate_hash))] + ); + assert_eq!( + tracker.pending_statements_for(counterparty, candidate_hash), + Some(statements.clone()) + ); + assert_eq!( + tracker.all_pending_statements_for(counterparty), + vec![(ValidatorIndex(0), CompactStatement::Seconded(candidate_hash))] + ); + + // Note sending or receiving of a direct statement. + tracker.sent_or_received_direct_statement( + &groups, + validator_index, + counterparty, + &statement, + ); + + // There should be no pending statements now (for the counterparty). + assert_eq!(tracker.all_pending_statements_for(counterparty), vec![]); + // TODO: There are still `pending_statements_for`, is this correct? + // assert_eq!(tracker.pending_statements_for(counterparty, candidate_hash), None); + } } From e656d7b86f76f2220ab0731b2c09e3b091f093a4 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 5 Feb 2023 19:26:15 +0100 Subject: [PATCH 07/18] Add `pending_statements_respect_remote_knowledge` test --- .../src/vstaging/grid.rs | 83 ++++++++++++++++++- 1 file changed, 81 insertions(+), 2 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index 5658a91fb090..2371c0d0055f 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -682,7 +682,7 @@ pub enum ManifestImportError { Disallowed, } -/// The knowledge we are awawre of counterparties having of manifests. +/// The knowledge we are aware of counterparties having of manifests. #[derive(Default)] struct ReceivedManifests { received: HashMap, @@ -1908,7 +1908,86 @@ mod tests { ); } - // TODO [now]: test that pending statements respect remote knowledge + #[test] + fn pending_statements_respect_remote_knowledge() { + let validator_index = ValidatorIndex(0); + + let mut tracker = GridTracker::default(); + let session_topology = SessionTopologyView { + group_views: vec![( + GroupIndex(0), + GroupSubView { + sending: HashSet::new(), + receiving: HashSet::from([validator_index]), + }, + )] + .into_iter() + .collect(), + }; + + let groups = Groups::new( + vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), + &[ + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + AuthorityDiscoveryPair::generate().0.public(), + ], + ); + + let candidate_hash = CandidateHash(Hash::repeat_byte(42)); + let group_index = GroupIndex(0); + let group_size = 3; + let local_knowledge = StatementFilter::new(group_size); + + // Should start with no pending statements. + assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); + assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); + + // Add the candidate as backed. + tracker.add_backed_candidate( + &session_topology, + candidate_hash, + group_index, + group_size, + local_knowledge.clone(), + ); + + // Import fresh statement. + let ack = tracker.import_manifest( + &session_topology, + &groups, + candidate_hash, + 3, + ManifestSummary { + claimed_parent_hash: Hash::repeat_byte(0), + claimed_group_index: group_index, + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], + }, + ManifestKind::Full, + validator_index, + ); + tracker.manifest_sent_to(&groups, validator_index, candidate_hash, local_knowledge); + let statement = CompactStatement::Seconded(candidate_hash); + tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); + let statement = CompactStatement::Valid(candidate_hash); + tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); + + // The pending statements should respect the remote knowledge (meaning the Seconded + // statement is ignored). + let statements = StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], + }; + assert_eq!( + tracker.pending_statements_for(validator_index, candidate_hash), + Some(statements.clone()) + ); + assert_eq!( + tracker.all_pending_statements_for(validator_index), + vec![(ValidatorIndex(0), CompactStatement::Valid(candidate_hash))] + ); + } #[test] fn pending_statements_cleared_when_sending() { From 393a2b72954aff6e4fc541a39dc11de340f626a8 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 5 Feb 2023 19:41:33 +0100 Subject: [PATCH 08/18] Refactor group creation in tests --- .../src/vstaging/grid.rs | 121 ++++-------------- 1 file changed, 23 insertions(+), 98 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index 2371c0d0055f..50028f7784a4 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -1044,6 +1044,14 @@ mod tests { use sp_authority_discovery::AuthorityPair as AuthorityDiscoveryPair; use sp_core::crypto::Pair as PairT; + fn dummy_groups(group_size: usize) -> Groups { + let groups = vec![(0..(group_size as u32)).map(ValidatorIndex).collect()].into(); + let mut discovery_keys = vec![]; + (0..group_size).map(|_| discovery_keys.push(AuthorityDiscoveryPair::generate().0.public())); + + Groups::new(groups, &discovery_keys) + } + #[test] fn topology_empty_for_no_index() { let base_topology = SessionGridTopology::new( @@ -1268,14 +1276,7 @@ mod tests { .collect(), }; - let groups = Groups::new( - vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), - &[ - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - ], - ); + let groups = dummy_groups(3); let candidate_hash = CandidateHash(Hash::repeat_byte(42)); @@ -1337,14 +1338,7 @@ mod tests { .collect(), }; - let groups = Groups::new( - vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), - &[ - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - ], - ); + let groups = dummy_groups(3); let candidate_hash = CandidateHash(Hash::repeat_byte(42)); @@ -1402,14 +1396,7 @@ mod tests { .collect(), }; - let groups = Groups::new( - vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), - &[ - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - ], - ); + let groups = dummy_groups(3); let candidate_hash = CandidateHash(Hash::repeat_byte(42)); @@ -1449,14 +1436,7 @@ mod tests { .collect(), }; - let groups = Groups::new( - vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), - &[ - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - ], - ); + let groups = dummy_groups(3); let candidate_hash = CandidateHash(Hash::repeat_byte(42)); @@ -1540,21 +1520,12 @@ mod tests { .collect(), }; - let groups = Groups::new( - vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), - &[ - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - ], - ); - let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; let local_knowledge = StatementFilter::new(group_size); - assert_eq!(groups.get_size_and_backing_threshold(group_index), Some((group_size, 2))); + let groups = dummy_groups(group_size); // Add the candidate as backed. tracker.add_backed_candidate( @@ -1606,21 +1577,12 @@ mod tests { .collect(), }; - let groups = Groups::new( - vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), - &[ - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - ], - ); - let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; let local_knowledge = StatementFilter::new(group_size); - assert_eq!(groups.get_size_and_backing_threshold(group_index), Some((group_size, 2))); + let groups = dummy_groups(group_size); // Manifest should not be pending yet. let pending_manifest = tracker.is_manifest_pending_for(validator_index, &candidate_hash); @@ -1685,21 +1647,12 @@ mod tests { .collect(), }; - let groups = Groups::new( - vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), - &[ - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - ], - ); - let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; let local_knowledge = StatementFilter::new(group_size); - assert_eq!(groups.get_size_and_backing_threshold(group_index), Some((group_size, 2))); + let groups = dummy_groups(group_size); // Add the candidate as backed. tracker.add_backed_candidate( @@ -1777,20 +1730,13 @@ mod tests { .collect(), }; - let groups = Groups::new( - vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), - &[ - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - ], - ); - let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; let local_knowledge = StatementFilter::new(group_size); + let groups = dummy_groups(group_size); + // Should start with no pending statements. assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); @@ -1836,20 +1782,13 @@ mod tests { .collect(), }; - let groups = Groups::new( - vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), - &[ - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - ], - ); - let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; let local_knowledge = StatementFilter::new(group_size); + let groups = dummy_groups(group_size); + // Should start with no pending statements. assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); @@ -1925,20 +1864,13 @@ mod tests { .collect(), }; - let groups = Groups::new( - vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), - &[ - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - ], - ); - let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; let local_knowledge = StatementFilter::new(group_size); + let groups = dummy_groups(group_size); + // Should start with no pending statements. assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); @@ -2007,20 +1939,13 @@ mod tests { .collect(), }; - let groups = Groups::new( - vec![vec![ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)]].into(), - &[ - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - AuthorityDiscoveryPair::generate().0.public(), - ], - ); - let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; let local_knowledge = StatementFilter::new(group_size); + let groups = dummy_groups(group_size); + // Should start with no pending statements. assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); From ebdb99ccd12bd9c888fe6e60dd3112ff4561e2d0 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 5 Feb 2023 19:51:43 +0100 Subject: [PATCH 09/18] Clarify docs --- node/network/statement-distribution/src/vstaging/grid.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index 50028f7784a4..bd47e21f62f4 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -865,10 +865,10 @@ impl StatementFilter { #[derive(Debug, Clone)] struct MutualKnowledge { - // Knowledge they have about the candidate. `Some` only if they + // Knowledge the remote peers have about the candidate. `Some` only if they // have advertised or requested the candidate. remote_knowledge: Option, - // Knowledge we have indicated to them about the candidate. + // Knowledge we have indicated to the remote peers about the candidate. // `Some` only if we have advertised or requested the candidate // from them. local_knowledge: Option, From 0ec0004c2fa401246ea05a811d7ad9328b5d2bd0 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Tue, 7 Feb 2023 19:08:22 +0100 Subject: [PATCH 10/18] Address some review comments --- .../src/vstaging/grid.rs | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index bd47e21f62f4..bdc31cbfef7b 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -863,14 +863,16 @@ impl StatementFilter { } } +/// Knowledge that a remote peer has about a candidate, and that they have about us concerning the +/// candidate. #[derive(Debug, Clone)] struct MutualKnowledge { - // Knowledge the remote peers have about the candidate. `Some` only if they - // have advertised or requested the candidate. + /// Knowledge the remote peer has about the candidate. `Some` only if they + /// have advertised or requested the candidate. remote_knowledge: Option, - // Knowledge we have indicated to the remote peers about the candidate. - // `Some` only if we have advertised or requested the candidate - // from them. + /// Knowledge we have indicated to the remote peer about the candidate. + /// `Some` only if we have advertised or requested the candidate + /// from them. local_knowledge: Option, } @@ -2019,7 +2021,7 @@ mod tests { vec![(ValidatorIndex(0), CompactStatement::Seconded(candidate_hash))] ); - // Note sending or receiving of a direct statement. + tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); tracker.sent_or_received_direct_statement( &groups, validator_index, @@ -2028,8 +2030,10 @@ mod tests { ); // There should be no pending statements now (for the counterparty). + assert_eq!( + tracker.pending_statements_for(counterparty, candidate_hash), + Some(StatementFilter::new(group_size)) + ); assert_eq!(tracker.all_pending_statements_for(counterparty), vec![]); - // TODO: There are still `pending_statements_for`, is this correct? - // assert_eq!(tracker.pending_statements_for(counterparty, candidate_hash), None); } } From ea17edb0202d5b6bafd31a75e874801c55cd5b8a Mon Sep 17 00:00:00 2001 From: Marcin S Date: Fri, 10 Feb 2023 11:58:18 +0100 Subject: [PATCH 11/18] Make some clarifications --- .../src/vstaging/grid.rs | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index 382b9e674488..377c06f93fd1 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -14,8 +14,11 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -//! Utilities for handling distribution of backed candidates along -//! the grid. +//! Utilities for handling distribution of backed candidates along the grid (outside the group to +//! the rest of the network). +//! +//! The grid uses the gossip topology defined in [`polkadot_node_network_protocol::grid_topology`]. +//! It defines how messages and statements are forwarded between validators. use polkadot_node_network_protocol::{ grid_topology::SessionGridTopology, vstaging::StatementFilter, PeerId, @@ -70,8 +73,8 @@ impl SessionTopologyView { /// Build a view of the topology for the session. /// For groups that we are part of: we receive from nobody and send to our X/Y peers. -/// For groups that we are not part of: we receive from any validator in the group we share a slice with. -/// and send to the corresponding X/Y slice. +/// For groups that we are not part of: we receive from any validator in the group we share a slice with +/// and send to the corresponding X/Y slice in the other dimension. /// For any validators we don't share a slice with, we receive from the nodes /// which share a slice with them. pub fn build_session_topology<'a>( @@ -311,7 +314,7 @@ impl GridTracker { } /// Add a new backed candidate to the tracker. This yields - /// an iterator of validators which we should either advertise to + /// a list of validators which we should either advertise to /// or signal that we know the candidate, along with the corresponding /// type of manifest we should send. pub fn add_backed_candidate( @@ -853,8 +856,8 @@ impl FilterQuery for StatementFilter { } } -/// Knowledge that a remote peer has about a candidate, and that they have about us concerning the -/// candidate. +/// Knowledge that we have about a remote peer concerning a candidate, and that they have about us +/// concerning the candidate. #[derive(Debug, Clone)] struct MutualKnowledge { /// Knowledge the remote peer has about the candidate. `Some` only if they @@ -1733,9 +1736,9 @@ mod tests { assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); - // Import fresh statement. Candidate not backed. + // Try to import fresh statement. Candidate not backed. let statement = CompactStatement::Seconded(candidate_hash); - tracker.learned_fresh_statement(&groups, &session_topology, ValidatorIndex(1), &statement); + tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); @@ -1749,7 +1752,7 @@ mod tests { local_knowledge.clone(), ); - // Import fresh statement. Unknown group for validator index. + // Try to import fresh statement. Unknown group for validator index. let statement = CompactStatement::Seconded(candidate_hash); tracker.learned_fresh_statement(&groups, &session_topology, ValidatorIndex(1), &statement); @@ -1795,6 +1798,7 @@ mod tests { ); // Import fresh statement. + let ack = tracker.import_manifest( &session_topology, &groups, @@ -1809,6 +1813,7 @@ mod tests { ManifestKind::Full, validator_index, ); + assert_matches!(ack, Ok(true)); tracker.manifest_sent_to(&groups, validator_index, candidate_hash, local_knowledge); let statement = CompactStatement::Seconded(candidate_hash); tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); @@ -1828,6 +1833,7 @@ mod tests { ); // After successful import, try importing again. Nothing should change. + tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); assert_eq!( tracker.pending_statements_for(validator_index, candidate_hash), @@ -1885,8 +1891,8 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: group_index, - seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], }, ManifestKind::Full, validator_index, @@ -1898,7 +1904,7 @@ mod tests { tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); // The pending statements should respect the remote knowledge (meaning the Seconded - // statement is ignored). + // statement is ignored, but not the Valid statement). let statements = StatementFilter { seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], From 2ec395f21bba34b9a2661be8bc5eabc08a4896d4 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Fri, 10 Feb 2023 12:09:09 +0100 Subject: [PATCH 12/18] Fix post-merge errors --- .../src/vstaging/grid.rs | 124 ++++++++++++------ 1 file changed, 82 insertions(+), 42 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index 377c06f93fd1..efea58e3bd47 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -1145,8 +1145,10 @@ mod tests { let expected_manifest_summary = ManifestSummary { claimed_parent_hash: Hash::repeat_byte(2), claimed_group_index: GroupIndex(0), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + }, }; knowledge @@ -1179,7 +1181,7 @@ mod tests { // conflicting seconded statements bitfield let mut s = expected_manifest_summary.clone(); - s.seconded_in_group = bitvec::bitvec![u8, Lsb0; 0, 1, 0]; + s.statement_knowledge.seconded_in_group = bitvec::bitvec![u8, Lsb0; 0, 1, 0]; assert_matches!( knowledge.import_received(3, 2, CandidateHash(Hash::repeat_byte(1)), s,), Err(ManifestImportError::Conflicting) @@ -1188,7 +1190,7 @@ mod tests { // conflicting valid statements bitfield let mut s = expected_manifest_summary.clone(); - s.validated_in_group = bitvec::bitvec![u8, Lsb0; 0, 1, 0]; + s.statement_knowledge.validated_in_group = bitvec::bitvec![u8, Lsb0; 0, 1, 0]; assert_matches!( knowledge.import_received(3, 2, CandidateHash(Hash::repeat_byte(1)), s,), Err(ManifestImportError::Conflicting) @@ -1206,8 +1208,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0xA), claimed_group_index: GroupIndex(0), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + }, }, ) .unwrap(); @@ -1220,8 +1224,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0xB), claimed_group_index: GroupIndex(0), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + }, }, ) .unwrap(); @@ -1234,8 +1240,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0xC), claimed_group_index: GroupIndex(0), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 1], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + } }, ), Err(ManifestImportError::Overflow) @@ -1249,8 +1257,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0xC), claimed_group_index: GroupIndex(0), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + }, }, ) .unwrap(); @@ -1288,8 +1298,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: GroupIndex(0), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + } }, ManifestKind::Full, ValidatorIndex(1), @@ -1308,8 +1320,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: GroupIndex(1), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + } }, ManifestKind::Full, ValidatorIndex(0), @@ -1348,8 +1362,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: GroupIndex(0), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0, 1], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + } }, ManifestKind::Full, ValidatorIndex(0), @@ -1366,8 +1382,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: GroupIndex(0), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1, 0], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1, 0], + } }, ManifestKind::Full, ValidatorIndex(0), @@ -1406,8 +1424,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: GroupIndex(0), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 1], + } }, ManifestKind::Full, ValidatorIndex(0), @@ -1448,8 +1468,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: GroupIndex(0), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + } }, ManifestKind::Full, ValidatorIndex(0), @@ -1468,8 +1490,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: GroupIndex(0), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], + } }, ManifestKind::Full, ValidatorIndex(0), @@ -1488,8 +1512,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: GroupIndex(0), - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + } }, ManifestKind::Full, ValidatorIndex(0), @@ -1543,8 +1569,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: group_index, - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, }, ManifestKind::Acknowledgement, validator_index, @@ -1610,8 +1638,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: group_index, - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, }, ManifestKind::Acknowledgement, validator_index, @@ -1671,8 +1701,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: group_index, - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, }, ManifestKind::Full, validator_index, @@ -1807,8 +1839,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: group_index, - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, }, ManifestKind::Full, validator_index, @@ -1891,8 +1925,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: group_index, - seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + }, }, ManifestKind::Full, validator_index, @@ -1966,8 +2002,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: group_index, - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, }, ManifestKind::Full, validator_index, @@ -1985,8 +2023,10 @@ mod tests { ManifestSummary { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: group_index, - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, }, ManifestKind::Full, counterparty, From aba75568c49076ceec8b8eb9407f76c011f13124 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Fri, 10 Feb 2023 12:46:22 +0100 Subject: [PATCH 13/18] Clarify test `senders_can_provide_manifests_in_acknowledgement` --- .../statement-distribution/src/vstaging/grid.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index efea58e3bd47..2f4f2f142b6b 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -1524,6 +1524,8 @@ mod tests { ); } + // Test that when we add a candidate as backed and advertise it to the sending group, they can + // provide an acknowledgement manifest in response. #[test] fn senders_can_provide_manifests_in_acknowledgement() { let validator_index = ValidatorIndex(0); @@ -1549,18 +1551,23 @@ mod tests { let groups = dummy_groups(group_size); // Add the candidate as backed. - tracker.add_backed_candidate( + let receivers = tracker.add_backed_candidate( &session_topology, candidate_hash, group_index, group_size, local_knowledge.clone(), ); + // Validator 0 is in the sending group. Advertise onward to it. + // + // Validator 1 is in the receiving group, but we have not received from it, so we're not + // expected to send it an acknowledgement. + assert_eq!(receivers, vec![(validator_index, ManifestKind::Full)]); // Note the manifest as 'sent' to validator 0. tracker.manifest_sent_to(&groups, validator_index, candidate_hash, local_knowledge); - // Import manifest. + // Import manifest of kind `Acknowledgement` from validator 0. let ack = tracker.import_manifest( &session_topology, &groups, From 351c7eeb6cfcb3fc2918fc47e9b2a443c998412e Mon Sep 17 00:00:00 2001 From: Marcin S Date: Fri, 10 Feb 2023 13:22:56 +0100 Subject: [PATCH 14/18] Try writing `pending_statements_are_updated_after_manifest_exchange` --- .../src/vstaging/grid.rs | 121 +++++++++++++++++- 1 file changed, 114 insertions(+), 7 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index 2f4f2f142b6b..5e9ff8e5a87b 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -1661,7 +1661,7 @@ mod tests { // Check that pending communication is cleared correctly in `manifest_sent_to` // - // Also test a scenario where manifest import returns `Ok(true)`. + // Also test a scenario where manifest import returns `Ok(true)` (should acknowledge). #[test] fn pending_communication_is_cleared() { let validator_index = ValidatorIndex(0); @@ -1699,7 +1699,8 @@ mod tests { let pending_manifest = tracker.is_manifest_pending_for(validator_index, &candidate_hash); assert_eq!(pending_manifest, None); - // Import manifest. + // Import manifest. The candidate is confirmed backed and we are expected to receive from + // validator 0, so send it an acknowledgement. let ack = tracker.import_manifest( &session_topology, &groups, @@ -1730,21 +1731,127 @@ mod tests { assert_eq!(pending_manifest, None); } + /// A manifest exchange means that both `manifest_sent_to` and `manifest_received_from` have + /// been invoked. + /// + /// In practice, it means that one of three things have happened: + /// + /// - They announced, we acknowledged + /// + /// - We announced, they acknowledged + /// + /// - We announced, they announced (not sure if this can actually happen; it would happen if 2 + /// nodes had each other in their sending set and they sent manifests at the same time. The + /// code accounts for this anyway) #[test] fn pending_statements_are_updated_after_manifest_exchange() { + let send_to = ValidatorIndex(0); + let receive_from = ValidatorIndex(1); + let mut tracker = GridTracker::default(); + let session_topology = SessionTopologyView { + group_views: vec![( + GroupIndex(0), + GroupSubView { + sending: HashSet::from([send_to]), + receiving: HashSet::from([receive_from]), + }, + )] + .into_iter() + .collect(), + }; - let validator_index = ValidatorIndex(0); let candidate_hash = CandidateHash(Hash::repeat_byte(42)); + let group_index = GroupIndex(0); let group_size = 3; + let local_knowledge = StatementFilter::new(group_size); + + let groups = dummy_groups(group_size); // Should start with no pending statements. - assert_eq!(tracker.pending_statements_for(validator_index, candidate_hash), None); - assert_eq!(tracker.all_pending_statements_for(validator_index), vec![]); + assert_eq!(tracker.pending_statements_for(send_to, candidate_hash), None); + assert_eq!(tracker.all_pending_statements_for(send_to), vec![]); + assert_eq!(tracker.pending_statements_for(receive_from, candidate_hash), None); + assert_eq!(tracker.all_pending_statements_for(receive_from), vec![]); + + let receivers = tracker.add_backed_candidate( + &session_topology, + candidate_hash, + group_index, + group_size, + local_knowledge.clone(), + ); + assert_eq!(receivers, vec![(send_to, ManifestKind::Full)]); + + // Test receiving followed by sending an ack. + + let ack = tracker.import_manifest( + &session_topology, + &groups, + candidate_hash, + 3, + ManifestSummary { + claimed_parent_hash: Hash::repeat_byte(0), + claimed_group_index: group_index, + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, + }, + ManifestKind::Full, + receive_from, + ); + assert_matches!(ack, Ok(true)); + tracker.manifest_sent_to(&groups, receive_from, candidate_hash, local_knowledge.clone()); + + // There should be pending statements now. + let statements = StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + }; + assert_eq!( + tracker.pending_statements_for(receive_from, candidate_hash), + Some(statements.clone()) + ); + assert_eq!( + tracker.all_pending_statements_for(receive_from), + vec![(ValidatorIndex(0), CompactStatement::Seconded(candidate_hash))] + ); - let statement_filter = StatementFilter::new(group_size); + // Test sending followed by receiving an ack. - todo!() + tracker.manifest_sent_to(&groups, send_to, candidate_hash, local_knowledge.clone()); + let ack = tracker.import_manifest( + &session_topology, + &groups, + candidate_hash, + 3, + ManifestSummary { + claimed_parent_hash: Hash::repeat_byte(0), + claimed_group_index: group_index, + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], + }, + }, + ManifestKind::Acknowledgement, + send_to, + ); + assert_matches!(ack, Ok(false)); + + // There should be pending statements now. + let statements = StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + }; + assert_eq!( + tracker.pending_statements_for(send_to, candidate_hash), + Some(statements.clone()) + ); + assert_eq!( + tracker.all_pending_statements_for(send_to), + vec![(send_to, CompactStatement::Seconded(candidate_hash))] + ); } #[test] From 3dcdd25f502ba99b4ec910c56a2c66f39f0a8fa1 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 12 Feb 2023 15:49:27 +0100 Subject: [PATCH 15/18] Document "seconding limit" and `reject_overflowing_manifests` test --- .../statement-distribution/src/vstaging/grid.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index 5e9ff8e5a87b..c8bf8cd95ddd 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -303,7 +303,7 @@ impl GridTracker { ); } } else { - // received prevents conflicting manifests so this is max 1 per validator. + // `received` prevents conflicting manifests so this is max 1 per validator. self.unconfirmed .entry(candidate_hash) .or_default() @@ -797,6 +797,9 @@ impl ReceivedManifests { // updates validator-seconded records but only if the new statements // are OK. returns `true` if alright and `false` otherwise. +// +// The seconding limit is a per-validator limit. It ensures an upper bound on the total number of +// candidates entering the system. fn updating_ensure_within_seconding_limit( seconded_counts: &mut HashMap>, group_index: GroupIndex, @@ -1197,6 +1200,8 @@ mod tests { ); } + // Make sure we don't import manifests that would put a validator in a group over the limit of + // candidates they are allowed to second (aka seconding limit). #[test] fn reject_overflowing_manifests() { let mut knowledge = ReceivedManifests::default(); @@ -1232,6 +1237,7 @@ mod tests { ) .unwrap(); + // Reject a seconding validator that is already at the seconding limit. assert_matches!( knowledge.import_received( 3, @@ -1241,7 +1247,7 @@ mod tests { claimed_parent_hash: Hash::repeat_byte(0xC), claimed_group_index: GroupIndex(0), statement_knowledge: StatementFilter { - seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 1], + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], } }, @@ -1249,6 +1255,7 @@ mod tests { Err(ManifestImportError::Overflow) ); + // Don't reject validators that have seconded less than the limit so far. knowledge .import_received( 3, From 336a4c389f730ad2bd46442219ffa8f1341ae9fa Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 12 Feb 2023 21:31:09 +0100 Subject: [PATCH 16/18] Test that seconding counts are not updated for validators on error --- node/network/statement-distribution/src/vstaging/grid.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index c8bf8cd95ddd..be4f51fbab1c 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -1237,7 +1237,8 @@ mod tests { ) .unwrap(); - // Reject a seconding validator that is already at the seconding limit. + // Reject a seconding validator that is already at the seconding limit. Seconding counts for + // the validators should not be applied. assert_matches!( knowledge.import_received( 3, @@ -1247,7 +1248,7 @@ mod tests { claimed_parent_hash: Hash::repeat_byte(0xC), claimed_group_index: GroupIndex(0), statement_knowledge: StatementFilter { - seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 1], validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], } }, From c2ef184ec33138add37391fb942526d2d31e4626 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Wed, 15 Feb 2023 11:15:03 +0100 Subject: [PATCH 17/18] Fix tests --- .../src/vstaging/grid.rs | 43 ++++++++++++------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index d61f29226d7e..f466ddb732c3 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -1561,7 +1561,7 @@ mod tests { let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; - let local_knowledge = StatementFilter::new(group_size); + let local_knowledge = StatementFilter::blank(group_size); let groups = dummy_groups(group_size); @@ -1625,7 +1625,7 @@ mod tests { let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; - let local_knowledge = StatementFilter::new(group_size); + let local_knowledge = StatementFilter::blank(group_size); let groups = dummy_groups(group_size); @@ -1697,7 +1697,7 @@ mod tests { let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; - let local_knowledge = StatementFilter::new(group_size); + let local_knowledge = StatementFilter::blank(group_size); let groups = dummy_groups(group_size); @@ -1779,7 +1779,7 @@ mod tests { let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; - let local_knowledge = StatementFilter::new(group_size); + let local_knowledge = StatementFilter::blank(group_size); let groups = dummy_groups(group_size); @@ -1889,7 +1889,7 @@ mod tests { let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; - let local_knowledge = StatementFilter::new(group_size); + let local_knowledge = StatementFilter::blank(group_size); let groups = dummy_groups(group_size); @@ -1941,7 +1941,7 @@ mod tests { let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; - let local_knowledge = StatementFilter::new(group_size); + let local_knowledge = StatementFilter::blank(group_size); let groups = dummy_groups(group_size); @@ -2008,6 +2008,8 @@ mod tests { ); } + // After learning fresh statements, we should not generate pending statements for knowledge that + // the validator already has. #[test] fn pending_statements_respect_remote_knowledge() { let validator_index = ValidatorIndex(0); @@ -2028,7 +2030,7 @@ mod tests { let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; - let local_knowledge = StatementFilter::new(group_size); + let local_knowledge = StatementFilter::blank(group_size); let groups = dummy_groups(group_size); @@ -2055,18 +2057,27 @@ mod tests { claimed_parent_hash: Hash::repeat_byte(0), claimed_group_index: group_index, statement_knowledge: StatementFilter { - seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], }, }, ManifestKind::Full, validator_index, ); + assert_matches!(ack, Ok(true)); tracker.manifest_sent_to(&groups, validator_index, candidate_hash, local_knowledge); - let statement = CompactStatement::Seconded(candidate_hash); - tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); - let statement = CompactStatement::Valid(candidate_hash); - tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); + tracker.learned_fresh_statement( + &groups, + &session_topology, + validator_index, + &CompactStatement::Seconded(candidate_hash), + ); + tracker.learned_fresh_statement( + &groups, + &session_topology, + validator_index, + &CompactStatement::Valid(candidate_hash), + ); // The pending statements should respect the remote knowledge (meaning the Seconded // statement is ignored, but not the Valid statement). @@ -2105,7 +2116,7 @@ mod tests { let candidate_hash = CandidateHash(Hash::repeat_byte(42)); let group_index = GroupIndex(0); let group_size = 3; - let local_knowledge = StatementFilter::new(group_size); + let local_knowledge = StatementFilter::blank(group_size); let groups = dummy_groups(group_size); @@ -2123,7 +2134,7 @@ mod tests { ); // Import statement for originator. - let ack = tracker.import_manifest( + tracker.import_manifest( &session_topology, &groups, candidate_hash, @@ -2144,7 +2155,7 @@ mod tests { tracker.learned_fresh_statement(&groups, &session_topology, validator_index, &statement); // Import statement for counterparty. - let ack = tracker.import_manifest( + tracker.import_manifest( &session_topology, &groups, candidate_hash, @@ -2197,7 +2208,7 @@ mod tests { // There should be no pending statements now (for the counterparty). assert_eq!( tracker.pending_statements_for(counterparty, candidate_hash), - Some(StatementFilter::new(group_size)) + Some(StatementFilter::blank(group_size)) ); assert_eq!(tracker.all_pending_statements_for(counterparty), vec![]); } From 5a08ced675644695f0bd7cab5de93e16a24a9d59 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Wed, 15 Feb 2023 12:11:36 +0100 Subject: [PATCH 18/18] Fix manifest exchange test --- .../src/vstaging/grid.rs | 153 ++++++++++-------- 1 file changed, 85 insertions(+), 68 deletions(-) diff --git a/node/network/statement-distribution/src/vstaging/grid.rs b/node/network/statement-distribution/src/vstaging/grid.rs index f466ddb732c3..eecd6218f121 100644 --- a/node/network/statement-distribution/src/vstaging/grid.rs +++ b/node/network/statement-distribution/src/vstaging/grid.rs @@ -1783,12 +1783,7 @@ mod tests { let groups = dummy_groups(group_size); - // Should start with no pending statements. - assert_eq!(tracker.pending_statements_for(send_to, candidate_hash), None); - assert_eq!(tracker.all_pending_statements_for(send_to), vec![]); - assert_eq!(tracker.pending_statements_for(receive_from, candidate_hash), None); - assert_eq!(tracker.all_pending_statements_for(receive_from), vec![]); - + // Confirm the candidate. let receivers = tracker.add_backed_candidate( &session_topology, candidate_hash, @@ -1798,75 +1793,97 @@ mod tests { ); assert_eq!(receivers, vec![(send_to, ManifestKind::Full)]); - // Test receiving followed by sending an ack. - - let ack = tracker.import_manifest( - &session_topology, + // Learn a statement from a different validator. + tracker.learned_fresh_statement( &groups, - candidate_hash, - 3, - ManifestSummary { - claimed_parent_hash: Hash::repeat_byte(0), - claimed_group_index: group_index, - statement_knowledge: StatementFilter { - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], - }, - }, - ManifestKind::Full, - receive_from, + &session_topology, + ValidatorIndex(2), + &CompactStatement::Seconded(candidate_hash), ); - assert_matches!(ack, Ok(true)); - tracker.manifest_sent_to(&groups, receive_from, candidate_hash, local_knowledge.clone()); - // There should be pending statements now. - let statements = StatementFilter { - seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], - }; - assert_eq!( - tracker.pending_statements_for(receive_from, candidate_hash), - Some(statements.clone()) - ); - assert_eq!( - tracker.all_pending_statements_for(receive_from), - vec![(ValidatorIndex(0), CompactStatement::Seconded(candidate_hash))] - ); + // Test receiving followed by sending an ack. + { + // Should start with no pending statements. + assert_eq!(tracker.pending_statements_for(receive_from, candidate_hash), None); + assert_eq!(tracker.all_pending_statements_for(receive_from), vec![]); + let ack = tracker.import_manifest( + &session_topology, + &groups, + candidate_hash, + 3, + ManifestSummary { + claimed_parent_hash: Hash::repeat_byte(0), + claimed_group_index: group_index, + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 1], + }, + }, + ManifestKind::Full, + receive_from, + ); + assert_matches!(ack, Ok(true)); + + // Send ack now. + tracker.manifest_sent_to( + &groups, + receive_from, + candidate_hash, + local_knowledge.clone(), + ); + + // There should be pending statements now. + assert_eq!( + tracker.pending_statements_for(receive_from, candidate_hash), + Some(StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + }) + ); + assert_eq!( + tracker.all_pending_statements_for(receive_from), + vec![(ValidatorIndex(2), CompactStatement::Seconded(candidate_hash))] + ); + } // Test sending followed by receiving an ack. + { + // Should start with no pending statements. + assert_eq!(tracker.pending_statements_for(send_to, candidate_hash), None); + assert_eq!(tracker.all_pending_statements_for(send_to), vec![]); - tracker.manifest_sent_to(&groups, send_to, candidate_hash, local_knowledge.clone()); - let ack = tracker.import_manifest( - &session_topology, - &groups, - candidate_hash, - 3, - ManifestSummary { - claimed_parent_hash: Hash::repeat_byte(0), - claimed_group_index: group_index, - statement_knowledge: StatementFilter { - seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], + tracker.manifest_sent_to(&groups, send_to, candidate_hash, local_knowledge.clone()); + let ack = tracker.import_manifest( + &session_topology, + &groups, + candidate_hash, + 3, + ManifestSummary { + claimed_parent_hash: Hash::repeat_byte(0), + claimed_group_index: group_index, + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 0], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], + }, }, - }, - ManifestKind::Acknowledgement, - send_to, - ); - assert_matches!(ack, Ok(false)); - - // There should be pending statements now. - let statements = StatementFilter { - seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 0, 0], - validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], - }; - assert_eq!( - tracker.pending_statements_for(send_to, candidate_hash), - Some(statements.clone()) - ); - assert_eq!( - tracker.all_pending_statements_for(send_to), - vec![(send_to, CompactStatement::Seconded(candidate_hash))] - ); + ManifestKind::Acknowledgement, + send_to, + ); + assert_matches!(ack, Ok(false)); + + // There should be pending statements now. + assert_eq!( + tracker.pending_statements_for(send_to, candidate_hash), + Some(StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + }) + ); + assert_eq!( + tracker.all_pending_statements_for(send_to), + vec![(ValidatorIndex(2), CompactStatement::Seconded(candidate_hash))] + ); + } } #[test]