f+1 validators always approve (#2699)

* f+1 always approves

* guide

* grumbles

* grumbles

* fix test

* fix tests

* Update roadmap/implementers-guide/src/node/approval/approval-voting.md

Co-authored-by: Sergei Shulepov <sergei@parity.io>

Co-authored-by: Sergei Shulepov <sergei@parity.io>

polkadot/node/core/approval-voting/src/approval_checking.rs

@@ -66,6 +66,8 @@ pub enum Check {
 	Unapproved,
 	/// The candidate is approved, with the given amount of no-shows.
 	Approved(usize),
+	/// The candidate is approved by one third of all validators.
+	ApprovedOneThird,
 }
 
 impl Check {
@@ -74,6 +76,15 @@ impl Check {
 		match *self {
 			Check::Unapproved => false,
 			Check::Approved(_) => true,
+			Check::ApprovedOneThird => true,
+		}
+	}
+
+	/// The number of known no-shows in this computation.
+	pub fn known_no_shows(&self) -> usize {
+		match *self {
+			Check::Approved(n) => n,
+			_ => 0,
 		}
 	}
 }
@@ -84,16 +95,17 @@ pub fn check_approval(
 	approval: &ApprovalEntry,
 	required: RequiredTranches,
 ) -> Check {
+
+	// any set of size f+1 contains at least one honest node. If at least one
+	// honest node approves, the candidate should be approved.
+	let approvals = candidate.approvals();
+	if 3 * approvals.count_ones() > approvals.len() {
+		return Check::ApprovedOneThird;
+	}
+
 	match required {
 		RequiredTranches::Pending { .. } => Check::Unapproved,
-		RequiredTranches::All => {
-			let approvals = candidate.approvals();
-			if 3 * approvals.count_ones() > 2 * approvals.len() {
-				Check::Approved(0)
-			} else {
-				Check::Unapproved
-			}
-		}
+		RequiredTranches::All => Check::Unapproved,
 		RequiredTranches::Exact { needed, tolerated_missing, .. } => {
 			// whether all assigned validators up to `needed` less no_shows have approved.
 			// e.g. if we had 5 tranches and 1 no-show, we would accept all validators in
@@ -410,33 +422,6 @@ mod tests {
 		).is_approved());
 	}
 
-	#[test]
-	fn all_requires_supermajority() {
-		let mut candidate: CandidateEntry = approval_db::v1::CandidateEntry {
-			candidate: Default::default(),
-			session: 0,
-			block_assignments: Default::default(),
-			approvals: bitvec![BitOrderLsb0, u8; 0; 10],
-		}.into();
-
-		for i in 0..6 {
-			candidate.mark_approval(ValidatorIndex(i));
-		}
-
-		let approval_entry = approval_db::v1::ApprovalEntry {
-			tranches: Vec::new(),
-			assignments: bitvec![BitOrderLsb0, u8; 1; 10],
-			our_assignment: None,
-			backing_group: GroupIndex(0),
-			approved: false,
-		}.into();
-
-		assert!(!check_approval(&candidate, &approval_entry, RequiredTranches::All).is_approved());
-
-		candidate.mark_approval(ValidatorIndex(6));
-		assert!(check_approval(&candidate, &approval_entry, RequiredTranches::All).is_approved());
-	}
-
 	#[test]
 	fn exact_takes_only_assignments_up_to() {
 		let mut candidate: CandidateEntry = approval_db::v1::CandidateEntry {
@@ -446,7 +431,70 @@ mod tests {
 			approvals: bitvec![BitOrderLsb0, u8; 0; 10],
 		}.into();
 
-		for i in 0..6 {
+		for i in 0..3 {
+			candidate.mark_approval(ValidatorIndex(i));
+		}
+
+		let approval_entry = approval_db::v1::ApprovalEntry {
+			tranches: vec![
+				approval_db::v1::TrancheEntry {
+					tranche: 0,
+					assignments: (0..2).map(|i| (ValidatorIndex(i), 0.into())).collect(),
+				},
+				approval_db::v1::TrancheEntry {
+					tranche: 1,
+					assignments: (2..5).map(|i| (ValidatorIndex(i), 1.into())).collect(),
+				},
+				approval_db::v1::TrancheEntry {
+					tranche: 2,
+					assignments: (5..10).map(|i| (ValidatorIndex(i), 0.into())).collect(),
+				},
+			],
+			assignments: bitvec![BitOrderLsb0, u8; 1; 10],
+			our_assignment: None,
+			backing_group: GroupIndex(0),
+			approved: false,
+		}.into();
+
+		assert!(check_approval(
+			&candidate,
+			&approval_entry,
+			RequiredTranches::Exact {
+				needed: 0,
+				tolerated_missing: 0,
+				next_no_show: None,
+			},
+		).is_approved());
+		assert!(!check_approval(
+			&candidate,
+			&approval_entry,
+			RequiredTranches::Exact {
+				needed: 1,
+				tolerated_missing: 0,
+				next_no_show: None,
+			},
+		).is_approved());
+		assert!(check_approval(
+			&candidate,
+			&approval_entry,
+			RequiredTranches::Exact {
+				needed: 1,
+				tolerated_missing: 2,
+				next_no_show: None,
+			},
+		).is_approved());
+	}
+
+	#[test]
+	fn one_honest_node_always_approves() {
+		let mut candidate: CandidateEntry = approval_db::v1::CandidateEntry {
+			candidate: Default::default(),
+			session: 0,
+			block_assignments: Default::default(),
+			approvals: bitvec![BitOrderLsb0, u8; 0; 10],
+		}.into();
+
+		for i in 0..3 {
 			candidate.mark_approval(ValidatorIndex(i));
 		}
 
@@ -471,32 +519,56 @@ mod tests {
 			approved: false,
 		}.into();
 
-		assert!(check_approval(
-			&candidate,
-			&approval_entry,
-			RequiredTranches::Exact {
-				needed: 1,
-				tolerated_missing: 0,
-				next_no_show: None,
-			},
-		).is_approved());
+		let exact_all = RequiredTranches::Exact {
+			needed: 10,
+			tolerated_missing: 0,
+			next_no_show: None,
+		};
+
+		let pending_all = RequiredTranches::Pending {
+			considered: 5,
+			next_no_show: None,
+			maximum_broadcast: 8,
+			clock_drift: 12,
+		};
+
 		assert!(!check_approval(
 			&candidate,
 			&approval_entry,
-			RequiredTranches::Exact {
-				needed: 2,
-				tolerated_missing: 0,
-				next_no_show: None,
-			},
+			RequiredTranches::All,
 		).is_approved());
+
+		assert!(!check_approval(
+			&candidate,
+			&approval_entry,
+			exact_all.clone(),
+		).is_approved());
+
+		assert!(!check_approval(
+			&candidate,
+			&approval_entry,
+			pending_all.clone(),
+		).is_approved());
+
+		// This creates a set of 4/10 approvals, which is always an approval.
+		candidate.mark_approval(ValidatorIndex(3));
+
 		assert!(check_approval(
 			&candidate,
 			&approval_entry,
-			RequiredTranches::Exact {
-				needed: 2,
-				tolerated_missing: 4,
-				next_no_show: None,
-			},
+			RequiredTranches::All,
+		).is_approved());
+
+		assert!(check_approval(
+			&candidate,
+			&approval_entry,
+			exact_all,
+		).is_approved());
+
+		assert!(check_approval(
+			&candidate,
+			&approval_entry,
+			pending_all,
 		).is_approved());
 	}
 

polkadot/node/core/approval-voting/src/lib.rs

@@ -1316,7 +1316,7 @@ fn check_and_apply_full_approval(
 			required_tranches.clone(),
 		);
 
-		if let approval_checking::Check::Approved(no_shows) = check {
+		if check.is_approved() {
 			tracing::trace!(
 				target: LOG_TARGET,
 				?candidate_hash,
@@ -1324,6 +1324,8 @@ fn check_and_apply_full_approval(
 				"Candidate approved under block.",
 			);
 
+			let no_shows = check.known_no_shows();
+
 			let was_approved = block_entry.is_fully_approved();
 
 			newly_approved.push(*block_hash);

polkadot/node/core/approval-voting/src/tests.rs

@@ -869,7 +869,52 @@ fn check_and_apply_full_approval_does_not_load_cached_block_from_db() {
 }
 
 #[test]
-fn assignment_triggered_by_all_with_less_than_supermajority() {
+fn assignment_triggered_by_all_with_less_than_threshold() {
+	let block_hash = Hash::repeat_byte(0x01);
+
+	let mut candidate_entry: CandidateEntry = {
+		let approval_entry = approval_db::v1::ApprovalEntry {
+			tranches: Vec::new(),
+			backing_group: GroupIndex(0),
+			our_assignment: Some(approval_db::v1::OurAssignment {
+				cert: garbage_assignment_cert(
+					AssignmentCertKind::RelayVRFModulo { sample: 0 }
+				),
+				tranche: 1,
+				validator_index: ValidatorIndex(4),
+				triggered: false,
+			}),
+			assignments: bitvec::bitvec![BitOrderLsb0, u8; 0; 4],
+			approved: false,
+		};
+
+		approval_db::v1::CandidateEntry {
+			candidate: Default::default(),
+			session: 1,
+			block_assignments: vec![(block_hash, approval_entry)].into_iter().collect(),
+			approvals: bitvec::bitvec![BitOrderLsb0, u8; 0; 4],
+		}.into()
+	};
+
+	// 1-of-4
+	candidate_entry
+		.approval_entry_mut(&block_hash)
+		.unwrap()
+		.import_assignment(0, ValidatorIndex(0), 0);
+
+	candidate_entry.mark_approval(ValidatorIndex(0));
+
+	let tranche_now = 1;
+	assert!(should_trigger_assignment(
+		candidate_entry.approval_entry(&block_hash).unwrap(),
+		&candidate_entry,
+		RequiredTranches::All,
+		tranche_now,
+	));
+}
+
+#[test]
+fn assignment_not_triggered_by_all_with_threshold() {
 	let block_hash = Hash::repeat_byte(0x01);
 
 	let mut candidate_entry: CandidateEntry = {
@@ -910,63 +955,6 @@ fn assignment_triggered_by_all_with_less_than_supermajority() {
 	candidate_entry.mark_approval(ValidatorIndex(0));
 	candidate_entry.mark_approval(ValidatorIndex(1));
 
-	let tranche_now = 1;
-	assert!(should_trigger_assignment(
-		candidate_entry.approval_entry(&block_hash).unwrap(),
-		&candidate_entry,
-		RequiredTranches::All,
-		tranche_now,
-	));
-}
-
-#[test]
-fn assignment_not_triggered_by_all_with_supermajority() {
-	let block_hash = Hash::repeat_byte(0x01);
-
-	let mut candidate_entry: CandidateEntry = {
-		let approval_entry = approval_db::v1::ApprovalEntry {
-			tranches: Vec::new(),
-			backing_group: GroupIndex(0),
-			our_assignment: Some(approval_db::v1::OurAssignment {
-				cert: garbage_assignment_cert(
-					AssignmentCertKind::RelayVRFModulo { sample: 0 }
-				),
-				tranche: 1,
-				validator_index: ValidatorIndex(4),
-				triggered: false,
-			}),
-			assignments: bitvec::bitvec![BitOrderLsb0, u8; 0; 4],
-			approved: false,
-		};
-
-		approval_db::v1::CandidateEntry {
-			candidate: Default::default(),
-			session: 1,
-			block_assignments: vec![(block_hash, approval_entry)].into_iter().collect(),
-			approvals: bitvec::bitvec![BitOrderLsb0, u8; 0; 4],
-		}.into()
-	};
-
-	// 3-of-4
-	candidate_entry
-		.approval_entry_mut(&block_hash)
-		.unwrap()
-		.import_assignment(0, ValidatorIndex(0), 0);
-
-	candidate_entry
-		.approval_entry_mut(&block_hash)
-		.unwrap()
-		.import_assignment(0, ValidatorIndex(1), 0);
-
-	candidate_entry
-		.approval_entry_mut(&block_hash)
-		.unwrap()
-		.import_assignment(0, ValidatorIndex(2), 0);
-
-	candidate_entry.mark_approval(ValidatorIndex(0));
-	candidate_entry.mark_approval(ValidatorIndex(1));
-	candidate_entry.mark_approval(ValidatorIndex(2));
-
 	let tranche_now = 1;
 	assert!(!should_trigger_assignment(
 		candidate_entry.approval_entry(&block_hash).unwrap(),

polkadot/roadmap/implementers-guide/src/node/approval/approval-voting.md

@@ -360,8 +360,9 @@ Likewise, when considering how many tranches to take, the no-show depth should b
 #### Check Approval
   * Check whether a candidate is approved under a particular block.
   * Requires `(block_entry, candidate_entry, approval_entry, n_tranches)`
+  * If we have `3 * n_approvals > n_validators`, return true. This is because any set with f+1 validators must have at least one honest validator, who has approved the candidate.
   * If `n_tranches` is `RequiredTranches::Pending`, return false
-  * If `n_tranches` is `RequiredTranches::All`,  then we return `3 * n_approvals > 2 * n_validators`.
+  * If `n_tranches` is `RequiredTranches::All`, return false.
   * If `n_tranches` is `RequiredTranches::Exact { tranche, tolerated_missing, .. }`, then we return whether all assigned validators up to `tranche` less `tolerated_missing` have approved. e.g. if we had 5 tranches and 1 tolerated missing, we would accept only if all but 1 of assigned validators in tranches 0..=5 have approved. In that example, we also accept all validators in tranches 0..=5 having approved, but that would indicate that the `RequiredTranches` value was incorrectly constructed, so it is not realistic. `tolerated_missing` actually represents covered no-shows. If there are more missing approvals than there are tolerated missing, that indicates that there are some assignments which are not yet no-shows, but may become no-shows, and we should wait for the validators to either approve or become no-shows.
 
 ### Time