[stable2509] Backport #10082 (#10087)

Backport #10082 into `stable2509` from alexggh.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

Signed-off-by: Alexandru Gheorghe <[email protected]>
Co-authored-by: Alexandru Gheorghe <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: 3 people

polkadot/node/network/approval-distribution/src/lib.rs

@@ -684,6 +684,8 @@ enum InvalidAssignmentError {
 enum InvalidVoteError {
 	// The candidate index was out of bounds.
 	CandidateIndexOutOfBounds,
+	// The candidate hash was not found in the block's candidate list.
+	CandidateHashNotFound,
 	// The validator index was out of bounds.
 	ValidatorIndexOutOfBounds,
 	// The signature of the vote was invalid.
@@ -2032,7 +2034,7 @@ impl State {
 		))
 		.check_signature(
 			&pubkey,
-			*candidate_hashes.first().unwrap(),
+			*candidate_hashes.first().ok_or(InvalidVoteError::CandidateHashNotFound)?,
 			entry.session,
 			&vote.signature,
 		)
@@ -2408,10 +2410,22 @@ impl State {
 	) -> Vec<IndirectSignedApprovalVoteV2> {
 		let mut sanitized_approvals = Vec::new();
 		for approval in approval.into_iter() {
-			if approval.candidate_indices.len() as usize > MAX_BITFIELD_SIZE {
+			let has_no_approved_candidates = approval.candidate_indices.first_one().is_none();
+			if approval.candidate_indices.len() as usize > MAX_BITFIELD_SIZE ||
+				has_no_approved_candidates
+			{
 				// Punish the peer for the invalid message.
-				modify_reputation(&mut self.reputation, sender, peer_id, COST_OVERSIZED_BITFIELD)
-					.await;
+				modify_reputation(
+					&mut self.reputation,
+					sender,
+					peer_id,
+					if has_no_approved_candidates {
+						COST_INVALID_MESSAGE
+					} else {
+						COST_OVERSIZED_BITFIELD
+					},
+				)
+				.await;
 				gum::debug!(
 					target: LOG_TARGET,
 					block_hash = ?approval.block_hash,

polkadot/node/network/approval-distribution/src/tests.rs

@@ -4446,3 +4446,94 @@ fn subsystem_accepts_tranche0_duplicate_assignments() {
 		},
 	);
 }
+
+#[test]
+fn test_empty_bitfield_gets_rejected_early() {
+	let peers = make_peers_and_authority_ids(15);
+	let peer_a = peers.get(0).unwrap().0;
+	let parent_hash = Hash::repeat_byte(0xFF);
+	let hash = Hash::repeat_byte(0xAA);
+	let candidate_hash = polkadot_primitives::CandidateHash(Hash::repeat_byte(0xBB));
+
+	let _ = test_harness(
+		Arc::new(MockAssignmentCriteria { tranche: Ok(0) }),
+		Arc::new(SystemClock {}),
+		state_without_reputation_delay(),
+		|mut virtual_overseer| async move {
+			let overseer = &mut virtual_overseer;
+
+			// Setup peer
+			setup_peer_with_view(overseer, &peer_a, view![hash], ValidationVersion::V3).await;
+
+			let mut keystore = LocalKeystore::in_memory();
+			let session = dummy_session_info_valid(1, &mut keystore, 1);
+
+			// Setup block with one candidate
+			let meta = BlockApprovalMeta {
+				hash,
+				parent_hash,
+				number: 1,
+				candidates: vec![(candidate_hash, 0.into(), 0.into())],
+				slot: 1.into(),
+				session: 1,
+				vrf_story: RelayVRFStory(Default::default()),
+			};
+			overseer_send(overseer, ApprovalDistributionMessage::NewBlocks(vec![meta])).await;
+
+			// Setup gossip topology
+			let peers_with_optional_peer_id = peers
+				.iter()
+				.map(|(peer_id, authority)| (Some(*peer_id), authority.clone()))
+				.collect_vec();
+			setup_gossip_topology(
+				overseer,
+				make_gossip_topology(1, &peers_with_optional_peer_id, &[0], &[2], 1),
+			)
+			.await;
+
+			// Send assignment first
+			let validator_index = ValidatorIndex(0);
+			let candidate_index = 0u32;
+			let cert = fake_assignment_cert_v2(hash, validator_index, CoreIndex(0).into());
+			let assignments = vec![(cert.clone(), candidate_index.into())];
+			let msg = protocol_v3::ApprovalDistributionMessage::Assignments(assignments);
+			send_message_from_peer_v3(overseer, &peer_a, msg).await;
+			provide_session(overseer, session.clone()).await;
+
+			// Should receive the assignment
+			assert_matches!(
+				overseer_recv(overseer).await,
+				AllMessages::ApprovalVoting(ApprovalVotingMessage::ImportAssignment(_, _))
+			);
+			expect_reputation_change(overseer, &peer_a, BENEFIT_VALID_MESSAGE_FIRST).await;
+
+			// Create an approval with empty candidate_indices is rejected early
+			let mut candidate_indices: CandidateBitfield = vec![0].try_into().unwrap();
+			candidate_indices.inner_mut().clear();
+
+			let normal_approval = IndirectSignedApprovalVoteV2 {
+				block_hash: hash,
+				candidate_indices: candidate_indices.clone(),
+				validator: validator_index,
+				signature: signature_for(
+					&keystore,
+					&session,
+					vec![candidate_hash],
+					validator_index,
+				),
+			};
+
+			let approval_to_send = normal_approval;
+
+			// Send the approval
+			let msg =
+				protocol_v3::ApprovalDistributionMessage::Approvals(vec![approval_to_send.clone()]);
+			send_message_from_peer_v3(overseer, &peer_a, msg).await;
+
+			// Expect rejection due to invalid message
+			expect_reputation_change(overseer, &peer_a, COST_INVALID_MESSAGE).await;
+
+			virtual_overseer
+		},
+	);
+}

polkadot/node/primitives/src/approval/mod.rs

@@ -296,7 +296,6 @@ pub mod v2 {
 		}
 
 		/// For testing purpose, we want a inner mutable ref.
-		#[cfg(test)]
 		pub fn inner_mut(&mut self) -> &mut BitVec<u8, bitvec::order::Lsb0> {
 			&mut self.0
 		}

prdoc/pr_10082.prdoc

@@ -0,0 +1,9 @@
+title: 'approval-distribution: improve test coverage'
+doc:
+- audience: Node Dev
+  description: Add few more test in approval-distribution to improve coverage.
+crates:
+- name: polkadot-approval-distribution
+  bump: patch
+- name: polkadot-node-primitives
+  bump: minor