misbehavior: report multiple offenses per validator as necessary (#2222)

* use proper descriptive generic type names

* cleanup

* Table stores a list of detected misbehavior per authority

* add Table::drain_misbehaviors_for

* WIP: unify misbehavior types; report multiple misbehaviors per validator

Code checks, but tests don't yet pass.

* update drain_misbehaviors: return authority id as well as specific misbehavior

* enable unchecked construction of Signed structs in tests

* remove test-features feature & unnecessary generic

* fix backing tests

This took a while to figure out, because where we'd previously been
passing around `SignedFullStatement`s, we now needed to construct
those on the fly within the test, to take advantage of the signature-
checking in the constructor. That, in turn, necessitated changing the
iterable type of `drain_misbehaviors` to return the validator index,
and passing that validator index along within the misbehavior report.

Once that was sorted, however, it became relatively straightforward:
just needed to add appropriate methods to deconstruct the misbehavior
reports, and then we could construct the signed statements directly.

* fix bad merge

polkadot/node/core/backing/src/lib.rs

@@ -19,7 +19,6 @@
 #![deny(unused_crate_dependencies)]
 
 use std::collections::{HashMap, HashSet};
-use std::convert::TryFrom;
 use std::pin::Pin;
 use std::sync::Arc;
 
@@ -28,13 +27,12 @@ use futures::{channel::{mpsc, oneshot}, Future, FutureExt, SinkExt, StreamExt};
 
 use sp_keystore::SyncCryptoStorePtr;
 use polkadot_primitives::v1::{
-	CommittedCandidateReceipt, BackedCandidate, Id as ParaId, ValidatorId,
-	ValidatorIndex, SigningContext, PoV, CandidateHash,
-	CandidateDescriptor, AvailableData, ValidatorSignature, Hash, CandidateReceipt,
-	CoreState, CoreIndex, CollatorId, ValidityAttestation, CandidateCommitments,
+	AvailableData, BackedCandidate, CandidateCommitments, CandidateDescriptor, CandidateHash,
+	CandidateReceipt, CollatorId, CommittedCandidateReceipt, CoreIndex, CoreState, Hash, Id as ParaId,
+	PoV, SigningContext, ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation,
 };
 use polkadot_node_primitives::{
-	FromTableMisbehavior, Statement, SignedFullStatement, MisbehaviorReport, ValidationResult,
+	Statement, SignedFullStatement, ValidationResult,
 };
 use polkadot_subsystem::{
 	JaegerSpan, PerLeafSpan,
@@ -60,8 +58,9 @@ use statement_table::{
 	Context as TableContextTrait,
 	Table,
 	v1::{
+		SignedStatement as TableSignedStatement,
 		Statement as TableStatement,
-		SignedStatement as TableSignedStatement, Summary as TableSummary,
+		Summary as TableSummary,
 	},
 };
 use thiserror::Error;
@@ -145,8 +144,6 @@ struct CandidateBackingJob {
 	/// The candidates that are includable, by hash. Each entry here indicates
 	/// that we've sent the provisioner the backed candidate.
 	backed: HashSet<CandidateHash>,
-	/// We have already reported misbehaviors for these validators.
-	reported_misbehavior_for: HashSet<ValidatorIndex>,
 	keystore: SyncCryptoStorePtr,
 	table: Table<TableContext>,
 	table_context: TableContext,
@@ -644,36 +641,17 @@ impl CandidateBackingJob {
 	}
 
 	/// Check if there have happened any new misbehaviors and issue necessary messages.
-	///
-	/// TODO: Report multiple misbehaviors (https://github.com/paritytech/polkadot/issues/1387)
 	#[tracing::instrument(level = "trace", skip(self), fields(subsystem = LOG_TARGET))]
 	async fn issue_new_misbehaviors(&mut self) -> Result<(), Error> {
-		let mut reports = Vec::new();
-
-		for (k, v) in self.table.get_misbehavior().iter() {
-			if !self.reported_misbehavior_for.contains(k) {
-				self.reported_misbehavior_for.insert(*k);
-
-				let f = FromTableMisbehavior {
-					id: *k,
-					report: v.clone(),
-					signing_context: self.table_context.signing_context.clone(),
-					key: self.table_context.validators[*k as usize].clone(),
-				};
-
-				if let Ok(report) = MisbehaviorReport::try_from(f) {
-					let message = ProvisionerMessage::ProvisionableData(
-						self.parent,
-						ProvisionableData::MisbehaviorReport(self.parent, report),
-					);
-
-					reports.push(message);
-				}
-			}
-		}
-
-		for report in reports.drain(..) {
-			self.send_to_provisioner(report).await?
+		// collect the misbehaviors to avoid double mutable self borrow issues
+		let misbehaviors: Vec<_> = self.table.drain_misbehaviors().collect();
+		for (validator_id, report) in misbehaviors {
+			self.send_to_provisioner(
+				ProvisionerMessage::ProvisionableData(
+					self.parent,
+					ProvisionableData::MisbehaviorReport(self.parent, validator_id, report)
+				)
+			).await?
 		}
 
 		Ok(())
@@ -1086,7 +1064,6 @@ impl util::JobTrait for CandidateBackingJob {
 				seconded: None,
 				unbacked_candidates: HashMap::new(),
 				backed: HashSet::new(),
-				reported_misbehavior_for: HashSet::new(),
 				keystore,
 				table: Table::default(),
 				table_context,
@@ -1199,9 +1176,7 @@ mod tests {
 	use super::*;
 	use assert_matches::assert_matches;
 	use futures::{future, Future};
-	use polkadot_primitives::v1::{
-		ScheduledCore, BlockData, PersistedValidationData, HeadData, GroupRotationInfo,
-	};
+	use polkadot_primitives::v1::{BlockData, GroupRotationInfo, HeadData, PersistedValidationData, ScheduledCore};
 	use polkadot_subsystem::{
 		messages::{RuntimeApiRequest, RuntimeApiMessage},
 		ActiveLeavesUpdate, FromOverseer, OverseerSignal,
@@ -1210,12 +1185,23 @@ mod tests {
 	use sp_keyring::Sr25519Keyring;
 	use sp_application_crypto::AppKey;
 	use sp_keystore::{CryptoStore, SyncCryptoStore};
+	use statement_table::v1::Misbehavior;
 	use std::collections::HashMap;
 
 	fn validator_pubkeys(val_ids: &[Sr25519Keyring]) -> Vec<ValidatorId> {
 		val_ids.iter().map(|v| v.public().into()).collect()
 	}
 
+	fn table_statement_to_primitive(
+		statement: TableStatement,
+	) -> Statement {
+		match statement {
+			TableStatement::Candidate(committed_candidate_receipt) => Statement::Seconded(committed_candidate_receipt),
+			TableStatement::Valid(candidate_hash) => Statement::Valid(candidate_hash),
+			TableStatement::Invalid(candidate_hash) => Statement::Invalid(candidate_hash),
+		}
+	}
+
 	struct TestState {
 		chain_ids: Vec<ParaId>,
 		keystore: SyncCryptoStorePtr,
@@ -1950,19 +1936,30 @@ mod tests {
 						_,
 						ProvisionableData::MisbehaviorReport(
 							relay_parent,
-							MisbehaviorReport::SelfContradiction(_, s1, s2),
+							validator_index,
+							Misbehavior::ValidityDoubleVote(vdv),
 						)
 					)
 				) if relay_parent == test_state.relay_parent => {
-					s1.check_signature(
-						&test_state.signing_context,
-						&test_state.validator_public[s1.validator_index() as usize],
-					).unwrap();
+					let ((t1, s1), (t2, s2)) = vdv.deconstruct::<TableContext>();
+					let t1 = table_statement_to_primitive(t1);
+					let t2 = table_statement_to_primitive(t2);
 
-					s2.check_signature(
+					SignedFullStatement::new(
+						t1,
+						validator_index,
+						s1,
 						&test_state.signing_context,
-						&test_state.validator_public[s2.validator_index() as usize],
-					).unwrap();
+						&test_state.validator_public[validator_index as usize],
+					).expect("signature must be valid");
+
+					SignedFullStatement::new(
+						t2,
+						validator_index,
+						s2,
+						&test_state.signing_context,
+						&test_state.validator_public[validator_index as usize],
+					).expect("signature must be valid");
 				}
 			);
 
@@ -1979,19 +1976,30 @@ mod tests {
 						_,
 						ProvisionableData::MisbehaviorReport(
 							relay_parent,
-							MisbehaviorReport::SelfContradiction(_, s1, s2),
+							validator_index,
+							Misbehavior::ValidityDoubleVote(vdv),
 						)
 					)
 				) if relay_parent == test_state.relay_parent => {
-					s1.check_signature(
-						&test_state.signing_context,
-						&test_state.validator_public[s1.validator_index() as usize],
-					).unwrap();
+					let ((t1, s1), (t2, s2)) = vdv.deconstruct::<TableContext>();
+					let t1 = table_statement_to_primitive(t1);
+					let t2 = table_statement_to_primitive(t2);
 
-					s2.check_signature(
+					SignedFullStatement::new(
+						t1,
+						validator_index,
+						s1,
 						&test_state.signing_context,
-						&test_state.validator_public[s2.validator_index() as usize],
-					).unwrap();
+						&test_state.validator_public[validator_index as usize],
+					).expect("signature must be valid");
+
+					SignedFullStatement::new(
+						t2,
+						validator_index,
+						s2,
+						&test_state.signing_context,
+						&test_state.validator_public[validator_index as usize],
+					).expect("signature must be valid");
 				}
 			);
 		});
@@ -2464,6 +2472,7 @@ mod tests {
 	#[test]
 	fn candidate_backing_reorders_votes() {
 		use sp_core::Encode;
+		use std::convert::TryFrom;
 
 		let relay_parent = [1; 32].into();
 		let para_id = ParaId::from(10);