From 5c16c95bd5e91f33ed81914a6e4205e3c9c3cb43 Mon Sep 17 00:00:00 2001
From: Lldenaurois <ljdenaurois@gmail.com>
Date: Fri, 18 Jun 2021 20:27:02 -0400
Subject: [PATCH] Follow-up PR: Count no-shows (#3309)

* node/approval-voting: test for invalid validator index in assignments

This commit adds a unit test to show that, currently, validator indexes
greater than n_validators (or the length of the approvals bitvector) are
counted in n_assignments. In the subsequent commit we will correct this
behavior.

* node/approval-voting: ignore invalid validator indexes in n_assignments

This commit ignores any validator assignments whose index is beyond
n_validators. Without this check, an improperly crafted assignment would
be counted towards the approval.

It still remains that n_assignments and count_no_shows inspect the
number of validators and approvals, respectively. Ideally we would
add greater safety around ensuring these two values cannot differ.
---
 .../approval-voting/src/approval_checking.rs  | 58 ++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/polkadot/node/core/approval-voting/src/approval_checking.rs b/polkadot/node/core/approval-voting/src/approval_checking.rs
index 3e0161b609..0843d574fc 100644
--- a/polkadot/node/core/approval-voting/src/approval_checking.rs
+++ b/polkadot/node/core/approval-voting/src/approval_checking.rs
@@ -385,7 +385,10 @@ pub fn tranches_to_approve(
 				return None;
 			}
 
-			let n_assignments = assignments.len();
+			// Count the number of valid validator assignments.
+			let n_assignments = assignments.iter()
+				.filter(|(v_index, _)| v_index.0 < n_validators as u32)
+				.count();
 
 			// count no-shows. An assignment is a no-show if there is no corresponding approval vote
 			// after a fixed duration.
@@ -982,6 +985,59 @@ mod tests {
 		);
 	}
 
+	#[test]
+	fn validator_indexes_out_of_range_are_ignored_in_assignments() {
+		let block_tick = 20;
+		let no_show_duration = 10;
+		let needed_approvals = 3;
+
+		let mut candidate: CandidateEntry = approval_db::v1::CandidateEntry {
+			candidate: Default::default(),
+			session: 0,
+			block_assignments: Default::default(),
+			approvals: bitvec![BitOrderLsb0, u8; 0; 3],
+		}.into();
+
+		for i in 0..3 {
+			candidate.mark_approval(ValidatorIndex(i));
+		}
+
+		let approval_entry = approval_db::v1::ApprovalEntry {
+			tranches: vec![
+				// Assignments with invalid validator indexes.
+				approval_db::v1::TrancheEntry {
+					tranche: 1,
+					assignments: (2..5).map(|i| (ValidatorIndex(i), 1.into())).collect(),
+				},
+			],
+			assignments: bitvec![BitOrderLsb0, u8; 1; 3],
+			our_assignment: None,
+			our_approval_sig: None,
+			backing_group: GroupIndex(0),
+			approved: false,
+		}.into();
+
+		let approvals = bitvec![BitOrderLsb0, u8; 0; 3];
+
+		let tranche_now = 10;
+		assert_eq!(
+			tranches_to_approve(
+				&approval_entry,
+				&approvals,
+				tranche_now,
+				block_tick,
+				no_show_duration,
+				needed_approvals,
+			),
+			RequiredTranches::Pending {
+				considered: 10,
+				next_no_show: None,
+				maximum_broadcast: DelayTranche::max_value(),
+				clock_drift: 0,
+			},
+		);
+	}
+
 	#[test]
 	fn filled_tranche_iterator_yields_sequential_tranches() {
 		const PREFIX: u32 = 10;