From 16d128782b35f5d6e1dfe3bf5f3349d5e78bc0cc Mon Sep 17 00:00:00 2001 From: "Marcin S." Date: Fri, 17 Nov 2023 15:53:20 +0100 Subject: [PATCH 1/8] statement-distribution: Add tests for incoming acknowledgements `handle_incoming_acknowledgement` was never tested. You can verify that it is covered now at this coverage report: https://app.codecov.io/gh/mrcnski/polkadot-sdk/blob/master/polkadot%2Fnode%2Fnetwork%2Fstatement-distribution%2Fsrc%2Fv2%2Fmod.rs#L2485 --- .../src/v2/tests/grid.rs | 467 +++++++++++++++++- .../src/v2/tests/mod.rs | 91 ++++ 2 files changed, 556 insertions(+), 2 deletions(-) diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs b/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs index 9802db060821..85960bc932e1 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs @@ -44,6 +44,8 @@ fn backed_candidate_leads_to_advertisement() { let local_group_index = local_validator.group_index.unwrap(); let local_para = ParaId::from(local_group_index.0); + let other_group = next_group_index(local_group_index, validator_count, group_size); + let test_leaf = state.make_dummy_leaf(relay_parent); let (candidate, pvd) = make_candidate( @@ -57,8 +59,7 @@ fn backed_candidate_leads_to_advertisement() { let candidate_hash = candidate.hash(); let other_group_validators = state.group_validators(local_group_index, true); - let target_group_validators = - state.group_validators((local_group_index.0 + 1).into(), true); + let target_group_validators = state.group_validators(other_group, true); let v_a = other_group_validators[0]; let v_b = other_group_validators[1]; let v_c = target_group_validators[0]; @@ -665,6 +666,468 @@ fn received_advertisement_after_backing_leads_to_acknowledgement() { }); } +#[test] +fn receive_ack_for_unconfirmed_candidate() { + let validator_count = 6; + let group_size = 3; + let config = TestConfig { + validator_count, + group_size, + local_validator: LocalRole::Validator, + async_backing_params: None, + }; + + test_harness(config, |state, mut overseer| async move { + let peers_to_connect = [ + TestPeerToConnect { local: true, relay_parent_in_view: true }, + TestPeerToConnect { local: true, relay_parent_in_view: false }, + TestPeerToConnect { local: false, relay_parent_in_view: true }, + TestPeerToConnect { local: false, relay_parent_in_view: false }, + ]; + let TestSetupInfo { + local_validator, + local_group, + local_para, + relay_parent, + test_leaf, + peers, + validators, + .. + } = setup_test_and_connect_peers( + &state, + &mut overseer, + validator_count, + group_size, + &peers_to_connect, + ) + .await; + let [peer_a, peer_b, peer_c, peer_d] = peers[..] else { panic!() }; + let [v_a, v_b, v_c, v_d] = validators[..] else { panic!() }; + + let (candidate, pvd) = make_candidate( + relay_parent, + 1, + local_para, + test_leaf.para_data(local_para).head_data.clone(), + vec![4, 5, 6].into(), + Hash::repeat_byte(42).into(), + ); + let candidate_hash = candidate.hash(); + + let ack = BackedCandidateAcknowledgement { + candidate_hash, + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + }, + }; + + // Receive an acknowledgement from a peer before the candidate is confirmed. + { + send_peer_message( + &mut overseer, + peer_c.clone(), + protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack.clone()), + ) + .await; + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_c && r == COST_UNEXPECTED_ACKNOWLEDGEMENT_UNKNOWN_CANDIDATE.into() + ); + } + + overseer + }); +} + +// Test receiving unexpected and expected acknowledgements for a locally confirmed candidate. +#[test] +fn received_acknowledgements_for_locally_confirmed() { + let validator_count = 6; + let group_size = 3; + let config = TestConfig { + validator_count, + group_size, + local_validator: LocalRole::Validator, + async_backing_params: None, + }; + + test_harness(config, |state, mut overseer| async move { + let peers_to_connect = [ + TestPeerToConnect { local: true, relay_parent_in_view: true }, + TestPeerToConnect { local: true, relay_parent_in_view: false }, + TestPeerToConnect { local: false, relay_parent_in_view: true }, + TestPeerToConnect { local: false, relay_parent_in_view: false }, + ]; + let TestSetupInfo { + local_validator, + local_group, + local_para, + relay_parent, + test_leaf, + peers, + validators, + .. + } = setup_test_and_connect_peers( + &state, + &mut overseer, + validator_count, + group_size, + &peers_to_connect, + ) + .await; + let [peer_a, peer_b, peer_c, peer_d] = peers[..] else { panic!() }; + let [v_a, v_b, v_c, v_d] = validators[..] else { panic!() }; + + let (candidate, pvd) = make_candidate( + relay_parent, + 1, + local_para, + test_leaf.para_data(local_para).head_data.clone(), + vec![4, 5, 6].into(), + Hash::repeat_byte(42).into(), + ); + let candidate_hash = candidate.hash(); + + let ack = BackedCandidateAcknowledgement { + candidate_hash, + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + }, + }; + + // Confirm the candidate locally so that we don't send out requests. + { + let statement = state + .sign_full_statement( + local_validator.validator_index, + Statement::Seconded(candidate.clone()), + &SigningContext { parent_hash: relay_parent, session_index: 1 }, + pvd.clone(), + ) + .clone(); + + overseer + .send(FromOrchestra::Communication { + msg: StatementDistributionMessage::Share(relay_parent, statement), + }) + .await; + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendValidationMessage(peers, _)) if peers == vec![peer_a] + ); + + answer_expected_hypothetical_depth_request(&mut overseer, vec![], None, false).await; + } + + // Receive an unexpected acknowledgement from peer D. + { + send_peer_message( + &mut overseer, + peer_d.clone(), + protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack.clone()), + ) + .await; + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_d && r == COST_UNEXPECTED_MANIFEST_DISALLOWED.into() + ); + } + + // Send statement from peer B. + { + let statement = state + .sign_statement( + v_b, + CompactStatement::Seconded(candidate_hash), + &SigningContext { parent_hash: relay_parent, session_index: 1 }, + ) + .as_unchecked() + .clone(); + + send_peer_message( + &mut overseer, + peer_b.clone(), + protocol_v2::StatementDistributionMessage::Statement(relay_parent, statement), + ) + .await; + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_b && r == BENEFIT_VALID_STATEMENT_FIRST.into() => { } + ); + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendValidationMessage(peers, _)) if peers == vec![peer_a] + ); + } + + // Send Backed notification. + { + overseer + .send(FromOrchestra::Communication { + msg: StatementDistributionMessage::Backed(candidate_hash), + }) + .await; + + assert_matches!( + overseer.recv().await, + AllMessages:: NetworkBridgeTx( + NetworkBridgeTxMessage::SendValidationMessage( + peers, + Versioned::V2( + protocol_v2::ValidationProtocol::StatementDistribution( + protocol_v2::StatementDistributionMessage::BackedCandidateManifest(manifest), + ), + ), + ) + ) => { + assert_eq!(peers, vec![peer_c]); + assert_eq!(manifest, BackedCandidateManifest { + relay_parent, + candidate_hash, + group_index: local_group, + para_id: local_para, + parent_head_data_hash: pvd.parent_head.hash(), + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + }, + }); + } + ); + + answer_expected_hypothetical_depth_request(&mut overseer, vec![], None, false).await; + } + + // Receive an unexpected acknowledgement from peer D. + // + // It still shouldn't know this manifest. + { + send_peer_message( + &mut overseer, + peer_d.clone(), + protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack.clone()), + ) + .await; + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_d && r == COST_UNEXPECTED_MANIFEST_DISALLOWED.into() + ); + } + + // Receive an acknowledgement from peer C. + // + // It's OK, we know they know it because we sent them a manifest. + { + send_peer_message( + &mut overseer, + peer_c.clone(), + protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack.clone()), + ) + .await; + } + + overseer + }); +} + +// Test receiving unexpected acknowledgements for a candidate confirmed in a different group. +#[test] +fn received_acknowledgements_for_externally_confirmed() { + let validator_count = 6; + let group_size = 3; + let config = TestConfig { + validator_count, + group_size, + local_validator: LocalRole::Validator, + async_backing_params: None, + }; + + let relay_parent = Hash::repeat_byte(1); + + test_harness(config, |state, mut overseer| async move { + let peers_to_connect = [ + TestPeerToConnect { local: true, relay_parent_in_view: true }, + TestPeerToConnect { local: true, relay_parent_in_view: false }, + TestPeerToConnect { local: false, relay_parent_in_view: true }, + TestPeerToConnect { local: false, relay_parent_in_view: true }, + TestPeerToConnect { local: false, relay_parent_in_view: true }, + ]; + let TestSetupInfo { + local_group, + local_para, + other_group, + other_para, + relay_parent, + test_leaf, + peers, + validators, + .. + } = setup_test_and_connect_peers( + &state, + &mut overseer, + validator_count, + group_size, + &peers_to_connect, + ) + .await; + let [peer_a, peer_b, peer_c, peer_d, peer_e] = peers[..] else { panic!() }; + let [v_a, v_b, v_c, v_d, v_e] = validators[..] else { panic!() }; + + let (candidate, pvd) = make_candidate( + relay_parent, + 1, + other_para, + test_leaf.para_data(other_para).head_data.clone(), + vec![4, 5, 6].into(), + Hash::repeat_byte(42).into(), + ); + let candidate_hash = candidate.hash(); + + let manifest = BackedCandidateManifest { + relay_parent, + candidate_hash, + group_index: other_group, + para_id: other_para, + parent_head_data_hash: pvd.parent_head.hash(), + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 0, 1, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + }, + }; + + let statement_c = state + .sign_statement( + v_c, + CompactStatement::Seconded(candidate_hash), + &SigningContext { parent_hash: relay_parent, session_index: 1 }, + ) + .as_unchecked() + .clone(); + let statement_d = state + .sign_statement( + v_d, + CompactStatement::Seconded(candidate_hash), + &SigningContext { parent_hash: relay_parent, session_index: 1 }, + ) + .as_unchecked() + .clone(); + + // Receive an advertisement from C, confirming the candidate. + { + send_peer_message( + &mut overseer, + peer_c.clone(), + protocol_v2::StatementDistributionMessage::BackedCandidateManifest( + manifest.clone(), + ), + ) + .await; + + // Should send a request to C. + let statements = vec![ + statement_c.clone(), + statement_d.clone(), + state + .sign_statement( + v_e, + CompactStatement::Seconded(candidate_hash), + &SigningContext { parent_hash: relay_parent, session_index: 1 }, + ) + .as_unchecked() + .clone(), + ]; + handle_sent_request( + &mut overseer, + peer_c, + candidate_hash, + StatementFilter::blank(group_size), + candidate.clone(), + pvd.clone(), + statements, + ) + .await; + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_c && r == BENEFIT_VALID_STATEMENT.into() + ); + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_c && r == BENEFIT_VALID_STATEMENT.into() + ); + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_c && r == BENEFIT_VALID_STATEMENT.into() + ); + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_c && r == BENEFIT_VALID_RESPONSE.into() + ); + + answer_expected_hypothetical_depth_request(&mut overseer, vec![], None, false).await; + } + + let ack = BackedCandidateAcknowledgement { + candidate_hash, + statement_knowledge: StatementFilter { + seconded_in_group: bitvec::bitvec![u8, Lsb0; 1, 1, 1], + validated_in_group: bitvec::bitvec![u8, Lsb0; 0, 0, 0], + }, + }; + + // Receive an unexpected acknowledgement from peer D. + { + send_peer_message( + &mut overseer, + peer_d.clone(), + protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack.clone()), + ) + .await; + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_d && r == COST_UNEXPECTED_MANIFEST_PEER_UNKNOWN.into() + ); + } + + // Receive an unexpected acknowledgement from peer A. + { + send_peer_message( + &mut overseer, + peer_a.clone(), + protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack.clone()), + ) + .await; + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_a && r == COST_UNEXPECTED_MANIFEST_DISALLOWED.into() + ); + } + + overseer + }); +} + // Received advertisement after confirmation but before backing leads to nothing. #[test] fn received_advertisement_after_confirmation_before_backing() { diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs index 4e6269775245..d9c09a0a7218 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs @@ -377,6 +377,97 @@ impl TestLeaf { } } +struct TestSetupInfo { + local_validator: TestLocalValidator, + local_group: GroupIndex, + local_para: ParaId, + other_group: GroupIndex, + other_para: ParaId, + relay_parent: Hash, + test_leaf: TestLeaf, + peers: Vec, + validators: Vec, +} + +struct TestPeerToConnect { + local: bool, + relay_parent_in_view: bool, +} + +// TODO: Generalize, use in more places. +/// Sets up some test info that is common to most tests, and connects the requested peers. +async fn setup_test_and_connect_peers( + state: &TestState, + overseer: &mut VirtualOverseer, + validator_count: usize, + group_size: usize, + peers_to_connect: &[TestPeerToConnect], +) -> TestSetupInfo { + let local_validator = state.local.clone().unwrap(); + let local_group = local_validator.group_index.unwrap(); + let local_para = ParaId::from(local_group.0); + + let other_group = next_group_index(local_group, validator_count, group_size); + let other_para = ParaId::from(other_group.0); + + let relay_parent = Hash::repeat_byte(1); + let test_leaf = state.make_dummy_leaf(relay_parent); + + // TODO: change based on `LocalRole`? + let our_group_validators = state.group_validators(local_group, true); + let outside_group_validators = state.group_validators(other_group, true); + + let mut peers = vec![]; + let mut validators = vec![]; + let mut our_group_idx = 0; + let mut outside_group_idx = 0; + for peer_to_connect in peers_to_connect { + let peer = PeerId::random(); + peers.push(peer); + + let v = if peer_to_connect.local { + let v = our_group_validators[our_group_idx]; + our_group_idx += 1; + v + } else { + let v = outside_group_validators[outside_group_idx]; + outside_group_idx += 1; + v + }; + validators.push(v); + + connect_peer( + overseer, + peer.clone(), + Some(vec![state.discovery_id(v)].into_iter().collect()), + ) + .await; + + if peer_to_connect.relay_parent_in_view { + send_peer_view_change(overseer, peer.clone(), view![relay_parent]).await; + } + } + + activate_leaf(overseer, &test_leaf, &state, true).await; + + answer_expected_hypothetical_depth_request(overseer, vec![], Some(relay_parent), false).await; + + // Send gossip topology. + send_new_topology(overseer, state.make_dummy_topology()).await; + + TestSetupInfo { + local_validator, + local_group, + local_para, + other_group, + other_para, + test_leaf, + relay_parent, + peers, + validators, + } +} + async fn activate_leaf( virtual_overseer: &mut VirtualOverseer, leaf: &TestLeaf, From c60465e3d59eba6aa89c412e67702c9fd23fc8c8 Mon Sep 17 00:00:00 2001 From: "Marcin S." Date: Sun, 26 Nov 2023 16:17:37 +0100 Subject: [PATCH 2/8] Refactor a bit (can roll with this in a future PR if it looks good) --- .../src/v2/tests/grid.rs | 187 ++++-------------- .../src/v2/tests/mod.rs | 61 +++++- 2 files changed, 98 insertions(+), 150 deletions(-) diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs b/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs index 85960bc932e1..afd89beecfc1 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs @@ -684,27 +684,18 @@ fn receive_ack_for_unconfirmed_candidate() { TestPeerToConnect { local: false, relay_parent_in_view: true }, TestPeerToConnect { local: false, relay_parent_in_view: false }, ]; - let TestSetupInfo { - local_validator, - local_group, - local_para, - relay_parent, - test_leaf, - peers, - validators, - .. - } = setup_test_and_connect_peers( - &state, - &mut overseer, - validator_count, - group_size, - &peers_to_connect, - ) - .await; - let [peer_a, peer_b, peer_c, peer_d] = peers[..] else { panic!() }; - let [v_a, v_b, v_c, v_d] = validators[..] else { panic!() }; + let TestSetupInfo { local_para, relay_parent, test_leaf, peers, .. } = + setup_test_and_connect_peers( + &state, + &mut overseer, + validator_count, + group_size, + &peers_to_connect, + ) + .await; + let [_, _, peer_c, _] = peers[..] else { panic!() }; - let (candidate, pvd) = make_candidate( + let (candidate, _pvd) = make_candidate( relay_parent, 1, local_para, @@ -723,20 +714,13 @@ fn receive_ack_for_unconfirmed_candidate() { }; // Receive an acknowledgement from a peer before the candidate is confirmed. - { - send_peer_message( - &mut overseer, - peer_c.clone(), - protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack.clone()), - ) - .await; - - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_c && r == COST_UNEXPECTED_ACKNOWLEDGEMENT_UNKNOWN_CANDIDATE.into() - ); - } + send_ack_from_peer(&mut overseer, peer_c, ack.clone()).await; + assert_peer_reported( + &mut overseer, + peer_c, + COST_UNEXPECTED_ACKNOWLEDGEMENT_UNKNOWN_CANDIDATE, + ) + .await; overseer }); @@ -779,7 +763,7 @@ fn received_acknowledgements_for_locally_confirmed() { ) .await; let [peer_a, peer_b, peer_c, peer_d] = peers[..] else { panic!() }; - let [v_a, v_b, v_c, v_d] = validators[..] else { panic!() }; + let [_, v_b, _, _] = validators[..] else { panic!() }; let (candidate, pvd) = make_candidate( relay_parent, @@ -810,11 +794,7 @@ fn received_acknowledgements_for_locally_confirmed() { ) .clone(); - overseer - .send(FromOrchestra::Communication { - msg: StatementDistributionMessage::Share(relay_parent, statement), - }) - .await; + share_statement(&mut overseer, relay_parent, statement).await; assert_matches!( overseer.recv().await, @@ -825,20 +805,8 @@ fn received_acknowledgements_for_locally_confirmed() { } // Receive an unexpected acknowledgement from peer D. - { - send_peer_message( - &mut overseer, - peer_d.clone(), - protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack.clone()), - ) - .await; - - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_d && r == COST_UNEXPECTED_MANIFEST_DISALLOWED.into() - ); - } + send_ack_from_peer(&mut overseer, peer_d, ack.clone()).await; + assert_peer_reported(&mut overseer, peer_d, COST_UNEXPECTED_MANIFEST_DISALLOWED).await; // Send statement from peer B. { @@ -858,11 +826,7 @@ fn received_acknowledgements_for_locally_confirmed() { ) .await; - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_b && r == BENEFIT_VALID_STATEMENT_FIRST.into() => { } - ); + assert_peer_reported(&mut overseer, peer_b, BENEFIT_VALID_STATEMENT_FIRST).await; assert_matches!( overseer.recv().await, @@ -872,11 +836,7 @@ fn received_acknowledgements_for_locally_confirmed() { // Send Backed notification. { - overseer - .send(FromOrchestra::Communication { - msg: StatementDistributionMessage::Backed(candidate_hash), - }) - .await; + back_candidate(&mut overseer, candidate_hash).await; assert_matches!( overseer.recv().await, @@ -911,32 +871,13 @@ fn received_acknowledgements_for_locally_confirmed() { // Receive an unexpected acknowledgement from peer D. // // It still shouldn't know this manifest. - { - send_peer_message( - &mut overseer, - peer_d.clone(), - protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack.clone()), - ) - .await; - - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_d && r == COST_UNEXPECTED_MANIFEST_DISALLOWED.into() - ); - } + send_ack_from_peer(&mut overseer, peer_d, ack.clone()).await; + assert_peer_reported(&mut overseer, peer_d, COST_UNEXPECTED_MANIFEST_DISALLOWED).await; // Receive an acknowledgement from peer C. // // It's OK, we know they know it because we sent them a manifest. - { - send_peer_message( - &mut overseer, - peer_c.clone(), - protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack.clone()), - ) - .await; - } + send_ack_from_peer(&mut overseer, peer_c, ack.clone()).await; overseer }); @@ -954,8 +895,6 @@ fn received_acknowledgements_for_externally_confirmed() { async_backing_params: None, }; - let relay_parent = Hash::repeat_byte(1); - test_harness(config, |state, mut overseer| async move { let peers_to_connect = [ TestPeerToConnect { local: true, relay_parent_in_view: true }, @@ -965,8 +904,6 @@ fn received_acknowledgements_for_externally_confirmed() { TestPeerToConnect { local: false, relay_parent_in_view: true }, ]; let TestSetupInfo { - local_group, - local_para, other_group, other_para, relay_parent, @@ -982,8 +919,8 @@ fn received_acknowledgements_for_externally_confirmed() { &peers_to_connect, ) .await; - let [peer_a, peer_b, peer_c, peer_d, peer_e] = peers[..] else { panic!() }; - let [v_a, v_b, v_c, v_d, v_e] = validators[..] else { panic!() }; + let [peer_a, _, peer_c, peer_d, _] = peers[..] else { panic!() }; + let [_, _, v_c, v_d, v_e] = validators[..] else { panic!() }; let (candidate, pvd) = make_candidate( relay_parent, @@ -1026,14 +963,7 @@ fn received_acknowledgements_for_externally_confirmed() { // Receive an advertisement from C, confirming the candidate. { - send_peer_message( - &mut overseer, - peer_c.clone(), - protocol_v2::StatementDistributionMessage::BackedCandidateManifest( - manifest.clone(), - ), - ) - .await; + send_manifest_from_peer(&mut overseer, peer_c, manifest.clone()).await; // Should send a request to C. let statements = vec![ @@ -1059,27 +989,10 @@ fn received_acknowledgements_for_externally_confirmed() { ) .await; - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_c && r == BENEFIT_VALID_STATEMENT.into() - ); - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_c && r == BENEFIT_VALID_STATEMENT.into() - ); - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_c && r == BENEFIT_VALID_STATEMENT.into() - ); - - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_c && r == BENEFIT_VALID_RESPONSE.into() - ); + assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await; + assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await; + assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await; + assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_RESPONSE).await; answer_expected_hypothetical_depth_request(&mut overseer, vec![], None, false).await; } @@ -1093,36 +1006,12 @@ fn received_acknowledgements_for_externally_confirmed() { }; // Receive an unexpected acknowledgement from peer D. - { - send_peer_message( - &mut overseer, - peer_d.clone(), - protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack.clone()), - ) - .await; - - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_d && r == COST_UNEXPECTED_MANIFEST_PEER_UNKNOWN.into() - ); - } + send_ack_from_peer(&mut overseer, peer_d, ack.clone()).await; + assert_peer_reported(&mut overseer, peer_d, COST_UNEXPECTED_MANIFEST_PEER_UNKNOWN).await; // Receive an unexpected acknowledgement from peer A. - { - send_peer_message( - &mut overseer, - peer_a.clone(), - protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack.clone()), - ) - .await; - - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_a && r == COST_UNEXPECTED_MANIFEST_DISALLOWED.into() - ); - } + send_ack_from_peer(&mut overseer, peer_a, ack.clone()).await; + assert_peer_reported(&mut overseer, peer_a, COST_UNEXPECTED_MANIFEST_DISALLOWED).await; overseer }); diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs index d9c09a0a7218..0c32c97e467d 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs @@ -21,7 +21,8 @@ use crate::*; use polkadot_node_network_protocol::{ grid_topology::TopologyPeerInfo, request_response::{outgoing::Recipient, ReqProtocolNames}, - view, ObservedRole, + v2::{BackedCandidateAcknowledgement, BackedCandidateManifest}, + view, ObservedRole, ReputationChange, }; use polkadot_node_primitives::Statement; use polkadot_node_subsystem::messages::{ @@ -638,6 +639,64 @@ async fn answer_expected_hypothetical_depth_request( ) } +async fn assert_peer_reported( + virtual_overseer: &mut VirtualOverseer, + peer_id: PeerId, + rep_change: impl Into, +) { + assert_matches!( + virtual_overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_id && r == rep_change.into() + ); +} + +async fn share_statement( + virtual_overseer: &mut VirtualOverseer, + relay_parent: Hash, + statement: SignedFullStatementWithPVD, +) { + virtual_overseer + .send(FromOrchestra::Communication { + msg: StatementDistributionMessage::Share(relay_parent, statement), + }) + .await; +} + +async fn back_candidate(virtual_overseer: &mut VirtualOverseer, candidate_hash: CandidateHash) { + virtual_overseer + .send(FromOrchestra::Communication { + msg: StatementDistributionMessage::Backed(candidate_hash), + }) + .await; +} + +async fn send_manifest_from_peer( + virtual_overseer: &mut VirtualOverseer, + peer_id: PeerId, + manifest: BackedCandidateManifest, +) { + send_peer_message( + virtual_overseer, + peer_id, + protocol_v2::StatementDistributionMessage::BackedCandidateManifest(manifest), + ) + .await; +} + +async fn send_ack_from_peer( + virtual_overseer: &mut VirtualOverseer, + peer_id: PeerId, + ack: BackedCandidateAcknowledgement, +) { + send_peer_message( + virtual_overseer, + peer_id, + protocol_v2::StatementDistributionMessage::BackedCandidateKnown(ack), + ) + .await; +} + fn validator_pubkeys(val_ids: &[ValidatorPair]) -> IndexedVec { val_ids.iter().map(|v| v.public().into()).collect() } From d8d5bfab5928b39cc27296ee8ff668da817a913b Mon Sep 17 00:00:00 2001 From: "Marcin S." Date: Sun, 26 Nov 2023 16:45:03 +0100 Subject: [PATCH 3/8] Test receiving another valid ack --- .../network/statement-distribution/src/v2/tests/grid.rs | 8 ++++++-- .../network/statement-distribution/src/v2/tests/mod.rs | 7 +++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs b/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs index afd89beecfc1..c8a62a432d66 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs @@ -794,7 +794,7 @@ fn received_acknowledgements_for_locally_confirmed() { ) .clone(); - share_statement(&mut overseer, relay_parent, statement).await; + send_share_message(&mut overseer, relay_parent, statement).await; assert_matches!( overseer.recv().await, @@ -836,8 +836,9 @@ fn received_acknowledgements_for_locally_confirmed() { // Send Backed notification. { - back_candidate(&mut overseer, candidate_hash).await; + send_backed_message(&mut overseer, candidate_hash).await; + // We should send out a manifest. assert_matches!( overseer.recv().await, AllMessages:: NetworkBridgeTx( @@ -879,6 +880,9 @@ fn received_acknowledgements_for_locally_confirmed() { // It's OK, we know they know it because we sent them a manifest. send_ack_from_peer(&mut overseer, peer_c, ack.clone()).await; + // What happens if we get another valid ack? + send_ack_from_peer(&mut overseer, peer_c, ack.clone()).await; + overseer }); } diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs index 0c32c97e467d..a2f9e8668c3d 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs @@ -651,7 +651,7 @@ async fn assert_peer_reported( ); } -async fn share_statement( +async fn send_share_message( virtual_overseer: &mut VirtualOverseer, relay_parent: Hash, statement: SignedFullStatementWithPVD, @@ -663,7 +663,10 @@ async fn share_statement( .await; } -async fn back_candidate(virtual_overseer: &mut VirtualOverseer, candidate_hash: CandidateHash) { +async fn send_backed_message( + virtual_overseer: &mut VirtualOverseer, + candidate_hash: CandidateHash, +) { virtual_overseer .send(FromOrchestra::Communication { msg: StatementDistributionMessage::Backed(candidate_hash), From 457b82f3fa668e19b80e020229c96a10d1de6148 Mon Sep 17 00:00:00 2001 From: "Marcin S." Date: Sun, 26 Nov 2023 17:12:25 +0100 Subject: [PATCH 4/8] Refactor existing test to show how much nicer the new functions are --- .../src/v2/tests/grid.rs | 131 +++++------------- .../src/v2/tests/mod.rs | 7 +- 2 files changed, 33 insertions(+), 105 deletions(-) diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs b/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs index c8a62a432d66..f2073b5fca5d 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs @@ -430,19 +430,33 @@ fn received_advertisement_after_backing_leads_to_acknowledgement() { async_backing_params: None, }; - let relay_parent = Hash::repeat_byte(1); - let peer_c = PeerId::random(); - let peer_d = PeerId::random(); - let peer_e = PeerId::random(); - test_harness(config, |state, mut overseer| async move { - let local_validator = state.local.clone().unwrap(); - let local_group_index = local_validator.group_index.unwrap(); - - let other_group = next_group_index(local_group_index, validator_count, group_size); - let other_para = ParaId::from(other_group.0); + let peers_to_connect = [ + TestPeerToConnect { local: true, relay_parent_in_view: false }, + TestPeerToConnect { local: true, relay_parent_in_view: false }, + TestPeerToConnect { local: false, relay_parent_in_view: true }, + TestPeerToConnect { local: false, relay_parent_in_view: true }, + TestPeerToConnect { local: false, relay_parent_in_view: true }, + ]; - let test_leaf = state.make_dummy_leaf(relay_parent); + let TestSetupInfo { + other_group, + other_para, + relay_parent, + test_leaf, + peers, + validators, + .. + } = setup_test_and_connect_peers( + &state, + &mut overseer, + validator_count, + group_size, + &peers_to_connect, + ) + .await; + let [_, _, peer_c, peer_d, _] = peers[..] else { panic!() }; + let [_, _, v_c, v_d, v_e] = validators[..] else { panic!() }; let (candidate, pvd) = make_candidate( relay_parent, @@ -454,52 +468,6 @@ fn received_advertisement_after_backing_leads_to_acknowledgement() { ); let candidate_hash = candidate.hash(); - let target_group_validators = state.group_validators(other_group, true); - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; - let v_e = target_group_validators[2]; - - // Connect C, D, E - { - connect_peer( - &mut overseer, - peer_c.clone(), - Some(vec![state.discovery_id(v_c)].into_iter().collect()), - ) - .await; - - connect_peer( - &mut overseer, - peer_d.clone(), - Some(vec![state.discovery_id(v_d)].into_iter().collect()), - ) - .await; - - connect_peer( - &mut overseer, - peer_e.clone(), - Some(vec![state.discovery_id(v_e)].into_iter().collect()), - ) - .await; - - send_peer_view_change(&mut overseer, peer_c.clone(), view![relay_parent]).await; - send_peer_view_change(&mut overseer, peer_d.clone(), view![relay_parent]).await; - send_peer_view_change(&mut overseer, peer_e.clone(), view![relay_parent]).await; - } - - activate_leaf(&mut overseer, &test_leaf, &state, true).await; - - answer_expected_hypothetical_depth_request( - &mut overseer, - vec![], - Some(relay_parent), - false, - ) - .await; - - // Send gossip topology. - send_new_topology(&mut overseer, state.make_dummy_topology()).await; - let manifest = BackedCandidateManifest { relay_parent, candidate_hash, @@ -531,14 +499,7 @@ fn received_advertisement_after_backing_leads_to_acknowledgement() { // Receive an advertisement from C. { - send_peer_message( - &mut overseer, - peer_c.clone(), - protocol_v2::StatementDistributionMessage::BackedCandidateManifest( - manifest.clone(), - ), - ) - .await; + send_manifest_from_peer(&mut overseer, peer_c, manifest.clone()).await; // Should send a request to C. let statements = vec![ @@ -564,37 +525,16 @@ fn received_advertisement_after_backing_leads_to_acknowledgement() { ) .await; - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_c && r == BENEFIT_VALID_STATEMENT.into() - ); - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_c && r == BENEFIT_VALID_STATEMENT.into() - ); - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_c && r == BENEFIT_VALID_STATEMENT.into() - ); - - assert_matches!( - overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_c && r == BENEFIT_VALID_RESPONSE.into() - ); + assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await; + assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await; + assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await; + assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_RESPONSE).await; answer_expected_hypothetical_depth_request(&mut overseer, vec![], None, false).await; } // Receive Backed message. - overseer - .send(FromOrchestra::Communication { - msg: StatementDistributionMessage::Backed(candidate_hash), - }) - .await; + send_backed_message(&mut overseer, candidate_hash).await; // Should send an acknowledgement back to C. { @@ -626,14 +566,7 @@ fn received_advertisement_after_backing_leads_to_acknowledgement() { // Receive a manifest about the same candidate from peer D. { - send_peer_message( - &mut overseer, - peer_d.clone(), - protocol_v2::StatementDistributionMessage::BackedCandidateManifest( - manifest.clone(), - ), - ) - .await; + send_manifest_from_peer(&mut overseer, peer_d, manifest.clone()).await; let expected_ack = BackedCandidateAcknowledgement { candidate_hash, diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs index a2f9e8668c3d..53f4ccbdc8d5 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs @@ -437,12 +437,7 @@ async fn setup_test_and_connect_peers( }; validators.push(v); - connect_peer( - overseer, - peer.clone(), - Some(vec![state.discovery_id(v)].into_iter().collect()), - ) - .await; + connect_peer(overseer, peer, Some(vec![state.discovery_id(v)].into_iter().collect())).await; if peer_to_connect.relay_parent_in_view { send_peer_view_change(overseer, peer.clone(), view![relay_parent]).await; From b3b85b2b84a9a53fe0347b9e06fb33f3c3aab8a7 Mon Sep 17 00:00:00 2001 From: "Marcin S." Date: Mon, 4 Dec 2023 18:28:27 +0100 Subject: [PATCH 5/8] remove unused dep --- Cargo.lock | 1 - polkadot/node/network/statement-distribution/Cargo.toml | 1 - polkadot/node/network/statement-distribution/src/lib.rs | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7854042ddbcf..3a5ca480d895 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13348,7 +13348,6 @@ dependencies = [ "polkadot-node-primitives", "polkadot-node-subsystem", "polkadot-node-subsystem-test-helpers", - "polkadot-node-subsystem-types", "polkadot-node-subsystem-util", "polkadot-primitives", "polkadot-primitives-test-helpers", diff --git a/polkadot/node/network/statement-distribution/Cargo.toml b/polkadot/node/network/statement-distribution/Cargo.toml index bf516e7b7ba9..e251abc445d6 100644 --- a/polkadot/node/network/statement-distribution/Cargo.toml +++ b/polkadot/node/network/statement-distribution/Cargo.toml @@ -16,7 +16,6 @@ sp-keystore = { path = "../../../../substrate/primitives/keystore" } polkadot-node-subsystem = { path = "../../subsystem" } polkadot-node-primitives = { path = "../../primitives" } polkadot-node-subsystem-util = { path = "../../subsystem-util" } -polkadot-node-subsystem-types = { path = "../../subsystem-types" } polkadot-node-network-protocol = { path = "../protocol" } arrayvec = "0.7.4" indexmap = "1.9.1" diff --git a/polkadot/node/network/statement-distribution/src/lib.rs b/polkadot/node/network/statement-distribution/src/lib.rs index 0a80c1491a90..ef1fc7cd78b5 100644 --- a/polkadot/node/network/statement-distribution/src/lib.rs +++ b/polkadot/node/network/statement-distribution/src/lib.rs @@ -19,7 +19,7 @@ //! This is responsible for distributing signed statements about candidate //! validity among validators. -// #![deny(unused_crate_dependencies)] +#![deny(unused_crate_dependencies)] #![warn(missing_docs)] use error::{log_error, FatalResult}; From 27fe1ebb203ba93e6831cbddbe624bb4023f6ee3 Mon Sep 17 00:00:00 2001 From: "Marcin S." Date: Mon, 4 Dec 2023 18:33:28 +0100 Subject: [PATCH 6/8] Rename target_group -> other_group Multiple people expressed confusion over the previous naming. This new scheme just keeps things more consistent and clear. --- .../src/v2/tests/grid.rs | 112 +++++++++--------- .../src/v2/tests/mod.rs | 19 +-- 2 files changed, 66 insertions(+), 65 deletions(-) diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs b/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs index f2073b5fca5d..f51c6685955c 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs @@ -58,12 +58,12 @@ fn backed_candidate_leads_to_advertisement() { ); let candidate_hash = candidate.hash(); - let other_group_validators = state.group_validators(local_group_index, true); - let target_group_validators = state.group_validators(other_group, true); - let v_a = other_group_validators[0]; - let v_b = other_group_validators[1]; - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; + let local_group_validators = state.group_validators(local_group_index, true); + let other_group_validators = state.group_validators(other_group, true); + let v_a = local_group_validators[0]; + let v_b = local_group_validators[1]; + let v_c = other_group_validators[0]; + let v_d = other_group_validators[1]; // peer A is in group, has relay parent in view. // peer B is in group, has no relay parent in view. @@ -275,12 +275,12 @@ fn received_advertisement_before_confirmation_leads_to_request() { ); let candidate_hash = candidate.hash(); - let other_group_validators = state.group_validators(local_group_index, true); - let target_group_validators = state.group_validators(other_group, true); - let v_a = other_group_validators[0]; - let v_b = other_group_validators[1]; - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; + let local_group_validators = state.group_validators(local_group_index, true); + let other_group_validators = state.group_validators(other_group, true); + let v_a = local_group_validators[0]; + let v_b = local_group_validators[1]; + let v_c = other_group_validators[0]; + let v_d = other_group_validators[1]; // peer A is in group, has relay parent in view. // peer B is in group, has no relay parent in view. @@ -990,10 +990,10 @@ fn received_advertisement_after_confirmation_before_backing() { ); let candidate_hash = candidate.hash(); - let target_group_validators = state.group_validators(other_group, true); - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; - let v_e = target_group_validators[2]; + let other_group_validators = state.group_validators(other_group, true); + let v_c = other_group_validators[0]; + let v_d = other_group_validators[1]; + let v_e = other_group_validators[2]; // Connect C, D, E { @@ -1176,10 +1176,10 @@ fn additional_statements_are_shared_after_manifest_exchange() { ); let candidate_hash = candidate.hash(); - let target_group_validators = state.group_validators(other_group, true); - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; - let v_e = target_group_validators[2]; + let other_group_validators = state.group_validators(other_group, true); + let v_c = other_group_validators[0]; + let v_d = other_group_validators[1]; + let v_e = other_group_validators[2]; // Connect C, D, E { @@ -1472,13 +1472,12 @@ fn advertisement_sent_when_peer_enters_relay_parent_view() { ); let candidate_hash = candidate.hash(); - let other_group_validators = state.group_validators(local_group_index, true); - let target_group_validators = - state.group_validators((local_group_index.0 + 1).into(), true); - let v_a = other_group_validators[0]; - let v_b = other_group_validators[1]; - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; + let local_group_validators = state.group_validators(local_group_index, true); + let other_group_validators = state.group_validators((local_group_index.0 + 1).into(), true); + let v_a = local_group_validators[0]; + let v_b = local_group_validators[1]; + let v_c = other_group_validators[0]; + let v_d = other_group_validators[1]; // peer A is in group, has relay parent in view. // peer B is in group, has no relay parent in view. @@ -1695,13 +1694,12 @@ fn advertisement_not_re_sent_when_peer_re_enters_view() { ); let candidate_hash = candidate.hash(); - let other_group_validators = state.group_validators(local_group_index, true); - let target_group_validators = - state.group_validators((local_group_index.0 + 1).into(), true); - let v_a = other_group_validators[0]; - let v_b = other_group_validators[1]; - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; + let local_group_validators = state.group_validators(local_group_index, true); + let other_group_validators = state.group_validators((local_group_index.0 + 1).into(), true); + let v_a = local_group_validators[0]; + let v_b = local_group_validators[1]; + let v_c = other_group_validators[0]; + let v_d = other_group_validators[1]; // peer A is in group, has relay parent in view. // peer B is in group, has no relay parent in view. @@ -1919,10 +1917,10 @@ fn grid_statements_imported_to_backing() { ); let candidate_hash = candidate.hash(); - let target_group_validators = state.group_validators(other_group, true); - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; - let v_e = target_group_validators[2]; + let other_group_validators = state.group_validators(other_group, true); + let v_c = other_group_validators[0]; + let v_d = other_group_validators[1]; + let v_e = other_group_validators[2]; // Connect C, D, E { @@ -2124,12 +2122,12 @@ fn advertisements_rejected_from_incorrect_peers() { ); let candidate_hash = candidate.hash(); - let other_group_validators = state.group_validators(local_group_index, true); - let target_group_validators = state.group_validators(other_group, true); - let v_a = other_group_validators[0]; - let v_b = other_group_validators[1]; - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; + let local_group_validators = state.group_validators(local_group_index, true); + let other_group_validators = state.group_validators(other_group, true); + let v_a = local_group_validators[0]; + let v_b = local_group_validators[1]; + let v_c = other_group_validators[0]; + let v_d = other_group_validators[1]; // peer A is in group, has relay parent in view. // peer B is in group, has no relay parent in view. @@ -2268,9 +2266,9 @@ fn manifest_rejected_with_unknown_relay_parent() { ); let candidate_hash = candidate.hash(); - let target_group_validators = state.group_validators(other_group, true); - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; + let other_group_validators = state.group_validators(other_group, true); + let v_c = other_group_validators[0]; + let v_d = other_group_validators[1]; // peer C is not in group, has relay parent in view. // peer D is not in group, has no relay parent in view. @@ -2370,9 +2368,9 @@ fn manifest_rejected_when_not_a_validator() { ); let candidate_hash = candidate.hash(); - let target_group_validators = state.group_validators(other_group, true); - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; + let other_group_validators = state.group_validators(other_group, true); + let v_c = other_group_validators[0]; + let v_d = other_group_validators[1]; // peer C is not in group, has relay parent in view. // peer D is not in group, has no relay parent in view. @@ -2477,9 +2475,9 @@ fn manifest_rejected_when_group_does_not_match_para() { ); let candidate_hash = candidate.hash(); - let target_group_validators = state.group_validators(other_group, true); - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; + let other_group_validators = state.group_validators(other_group, true); + let v_c = other_group_validators[0]; + let v_d = other_group_validators[1]; // peer C is not in group, has relay parent in view. // peer D is not in group, has no relay parent in view. @@ -2583,10 +2581,10 @@ fn peer_reported_for_advertisement_conflicting_with_confirmed_candidate() { ); let candidate_hash = candidate.hash(); - let target_group_validators = state.group_validators(other_group, true); - let v_c = target_group_validators[0]; - let v_d = target_group_validators[1]; - let v_e = target_group_validators[2]; + let other_group_validators = state.group_validators(other_group, true); + let v_c = other_group_validators[0]; + let v_d = other_group_validators[1]; + let v_e = other_group_validators[2]; // Connect C, D, E { diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs index 53f4ccbdc8d5..aed0c2fa1882 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs @@ -414,25 +414,28 @@ async fn setup_test_and_connect_peers( let relay_parent = Hash::repeat_byte(1); let test_leaf = state.make_dummy_leaf(relay_parent); + // Because we are testing grid mod, the "target" group (the one we communicate with) is usually + // other_group, a non-local group. + // // TODO: change based on `LocalRole`? - let our_group_validators = state.group_validators(local_group, true); - let outside_group_validators = state.group_validators(other_group, true); + let local_group_validators = state.group_validators(local_group, true); + let other_group_validators = state.group_validators(other_group, true); let mut peers = vec![]; let mut validators = vec![]; - let mut our_group_idx = 0; - let mut outside_group_idx = 0; + let mut local_group_idx = 0; + let mut other_group_idx = 0; for peer_to_connect in peers_to_connect { let peer = PeerId::random(); peers.push(peer); let v = if peer_to_connect.local { - let v = our_group_validators[our_group_idx]; - our_group_idx += 1; + let v = local_group_validators[local_group_idx]; + local_group_idx += 1; v } else { - let v = outside_group_validators[outside_group_idx]; - outside_group_idx += 1; + let v = other_group_validators[other_group_idx]; + other_group_idx += 1; v }; validators.push(v); From 168e69bf350cc5b9de5d69d7920949592e57ea3f Mon Sep 17 00:00:00 2001 From: "Marcin S." Date: Mon, 4 Dec 2023 18:39:43 +0100 Subject: [PATCH 7/8] change `assert_peer_reported` to macro --- .../src/v2/tests/grid.rs | 31 +++++++++---------- .../src/v2/tests/mod.rs | 21 ++++++------- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs b/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs index f51c6685955c..116116659cb1 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/grid.rs @@ -525,10 +525,10 @@ fn received_advertisement_after_backing_leads_to_acknowledgement() { ) .await; - assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await; - assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await; - assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await; - assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_RESPONSE).await; + assert_peer_reported!(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT); + assert_peer_reported!(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT); + assert_peer_reported!(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT); + assert_peer_reported!(&mut overseer, peer_c, BENEFIT_VALID_RESPONSE); answer_expected_hypothetical_depth_request(&mut overseer, vec![], None, false).await; } @@ -648,12 +648,11 @@ fn receive_ack_for_unconfirmed_candidate() { // Receive an acknowledgement from a peer before the candidate is confirmed. send_ack_from_peer(&mut overseer, peer_c, ack.clone()).await; - assert_peer_reported( + assert_peer_reported!( &mut overseer, peer_c, COST_UNEXPECTED_ACKNOWLEDGEMENT_UNKNOWN_CANDIDATE, - ) - .await; + ); overseer }); @@ -739,7 +738,7 @@ fn received_acknowledgements_for_locally_confirmed() { // Receive an unexpected acknowledgement from peer D. send_ack_from_peer(&mut overseer, peer_d, ack.clone()).await; - assert_peer_reported(&mut overseer, peer_d, COST_UNEXPECTED_MANIFEST_DISALLOWED).await; + assert_peer_reported!(&mut overseer, peer_d, COST_UNEXPECTED_MANIFEST_DISALLOWED); // Send statement from peer B. { @@ -759,7 +758,7 @@ fn received_acknowledgements_for_locally_confirmed() { ) .await; - assert_peer_reported(&mut overseer, peer_b, BENEFIT_VALID_STATEMENT_FIRST).await; + assert_peer_reported!(&mut overseer, peer_b, BENEFIT_VALID_STATEMENT_FIRST); assert_matches!( overseer.recv().await, @@ -806,7 +805,7 @@ fn received_acknowledgements_for_locally_confirmed() { // // It still shouldn't know this manifest. send_ack_from_peer(&mut overseer, peer_d, ack.clone()).await; - assert_peer_reported(&mut overseer, peer_d, COST_UNEXPECTED_MANIFEST_DISALLOWED).await; + assert_peer_reported!(&mut overseer, peer_d, COST_UNEXPECTED_MANIFEST_DISALLOWED); // Receive an acknowledgement from peer C. // @@ -926,10 +925,10 @@ fn received_acknowledgements_for_externally_confirmed() { ) .await; - assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await; - assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await; - assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT).await; - assert_peer_reported(&mut overseer, peer_c, BENEFIT_VALID_RESPONSE).await; + assert_peer_reported!(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT); + assert_peer_reported!(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT); + assert_peer_reported!(&mut overseer, peer_c, BENEFIT_VALID_STATEMENT); + assert_peer_reported!(&mut overseer, peer_c, BENEFIT_VALID_RESPONSE); answer_expected_hypothetical_depth_request(&mut overseer, vec![], None, false).await; } @@ -944,11 +943,11 @@ fn received_acknowledgements_for_externally_confirmed() { // Receive an unexpected acknowledgement from peer D. send_ack_from_peer(&mut overseer, peer_d, ack.clone()).await; - assert_peer_reported(&mut overseer, peer_d, COST_UNEXPECTED_MANIFEST_PEER_UNKNOWN).await; + assert_peer_reported!(&mut overseer, peer_d, COST_UNEXPECTED_MANIFEST_PEER_UNKNOWN); // Receive an unexpected acknowledgement from peer A. send_ack_from_peer(&mut overseer, peer_a, ack.clone()).await; - assert_peer_reported(&mut overseer, peer_a, COST_UNEXPECTED_MANIFEST_DISALLOWED).await; + assert_peer_reported!(&mut overseer, peer_a, COST_UNEXPECTED_MANIFEST_DISALLOWED); overseer }); diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs index aed0c2fa1882..dabbd279a13e 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs @@ -22,7 +22,7 @@ use polkadot_node_network_protocol::{ grid_topology::TopologyPeerInfo, request_response::{outgoing::Recipient, ReqProtocolNames}, v2::{BackedCandidateAcknowledgement, BackedCandidateManifest}, - view, ObservedRole, ReputationChange, + view, ObservedRole, }; use polkadot_node_primitives::Statement; use polkadot_node_subsystem::messages::{ @@ -637,16 +637,15 @@ async fn answer_expected_hypothetical_depth_request( ) } -async fn assert_peer_reported( - virtual_overseer: &mut VirtualOverseer, - peer_id: PeerId, - rep_change: impl Into, -) { - assert_matches!( - virtual_overseer.recv().await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_id && r == rep_change.into() - ); +#[macro_export] +macro_rules! assert_peer_reported { + ($virtual_overseer:expr, $peer_id:expr, $rep_change:expr ) => { + assert_matches!( + $virtual_overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == $peer_id && r == $rep_change.into() + ); + } } async fn send_share_message( From 37faf84a9838d7a5d4373350302c6e0c027c3174 Mon Sep 17 00:00:00 2001 From: "Marcin S." Date: Mon, 4 Dec 2023 19:51:51 +0100 Subject: [PATCH 8/8] Fix macro --- .../node/network/statement-distribution/src/v2/tests/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs index dabbd279a13e..c34cf20d716c 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/mod.rs @@ -639,7 +639,7 @@ async fn answer_expected_hypothetical_depth_request( #[macro_export] macro_rules! assert_peer_reported { - ($virtual_overseer:expr, $peer_id:expr, $rep_change:expr ) => { + ($virtual_overseer:expr, $peer_id:expr, $rep_change:expr $(,)*) => { assert_matches!( $virtual_overseer.recv().await, AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r)))