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

Author: coriolinus

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(
+					let ((t1, s1), (t2, s2)) = vdv.deconstruct::<TableContext>();
+					let t1 = table_statement_to_primitive(t1);
+					let t2 = table_statement_to_primitive(t2);
+
+					SignedFullStatement::new(
+						t1,
+						validator_index,
+						s1,
 						&test_state.signing_context,
-						&test_state.validator_public[s1.validator_index() as usize],
-					).unwrap();
+						&test_state.validator_public[validator_index as usize],
+					).expect("signature must be valid");
 
-					s2.check_signature(
+					SignedFullStatement::new(
+						t2,
+						validator_index,
+						s2,
 						&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");
 				}
 			);
 
@@ -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(
+					let ((t1, s1), (t2, s2)) = vdv.deconstruct::<TableContext>();
+					let t1 = table_statement_to_primitive(t1);
+					let t2 = table_statement_to_primitive(t2);
+
+					SignedFullStatement::new(
+						t1,
+						validator_index,
+						s1,
 						&test_state.signing_context,
-						&test_state.validator_public[s1.validator_index() as usize],
-					).unwrap();
+						&test_state.validator_public[validator_index as usize],
+					).expect("signature must be valid");
 
-					s2.check_signature(
+					SignedFullStatement::new(
+						t2,
+						validator_index,
+						s2,
 						&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");
 				}
 			);
 		});
@@ -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);

node/primitives/src/lib.rs

@@ -25,17 +25,9 @@
 use futures::Future;
 use parity_scale_codec::{Decode, Encode};
 use polkadot_primitives::v1::{
-	Hash, CommittedCandidateReceipt, CandidateReceipt, CompactStatement,
-	EncodeAs, Signed, SigningContext, ValidatorIndex, ValidatorId,
-	UpwardMessage, ValidationCode, PersistedValidationData,
-	HeadData, PoV, CollatorPair, Id as ParaId, OutboundHrmpMessage, CandidateCommitments, CandidateHash,
-};
-use polkadot_statement_table::{
-	generic::{
-		ValidityDoubleVote as TableValidityDoubleVote,
-		MultipleCandidates as TableMultipleCandidates,
-	},
-	v1::Misbehavior as TableMisbehavior,
+	CandidateCommitments, CandidateHash, CollatorPair, CommittedCandidateReceipt, CompactStatement,
+	EncodeAs, Hash, HeadData, Id as ParaId, OutboundHrmpMessage, PersistedValidationData, PoV,
+	Signed, UpwardMessage, ValidationCode,
 };
 use std::pin::Pin;
 
@@ -105,36 +97,6 @@ impl EncodeAs<CompactStatement> for Statement {
 /// Only the compact `SignedStatement` is suitable for submission to the chain.
 pub type SignedFullStatement = Signed<Statement, CompactStatement>;
 
-/// A misbehaviour report.
-#[derive(Debug, Clone)]
-pub enum MisbehaviorReport {
-	/// These validator nodes disagree on this candidate's validity, please figure it out
-	///
-	/// Most likely, the list of statments all agree except for the final one. That's not
-	/// guaranteed, though; if somehow we become aware of lots of
-	/// statements disagreeing about the validity of a candidate before taking action,
-	/// this message should be dispatched with all of them, in arbitrary order.
-	///
-	/// This variant is also used when our own validity checks disagree with others'.
-	CandidateValidityDisagreement(CandidateReceipt, Vec<SignedFullStatement>),
-	/// I've noticed a peer contradicting itself about a particular candidate
-	SelfContradiction(CandidateReceipt, SignedFullStatement, SignedFullStatement),
-	/// This peer has seconded more than one parachain candidate for this relay parent head
-	DoubleVote(SignedFullStatement, SignedFullStatement),
-}
-
-/// A utility struct used to convert `TableMisbehavior` to `MisbehaviorReport`s.
-pub struct FromTableMisbehavior {
-	/// Index of the validator.
-	pub id: ValidatorIndex,
-	/// The misbehavior reported by the table.
-	pub report: TableMisbehavior,
-	/// Signing context.
-	pub signing_context: SigningContext,
-	/// Misbehaving validator's public key.
-	pub key: ValidatorId,
-}
-
 /// Candidate invalidity details
 #[derive(Debug)]
 pub enum InvalidCandidate {
@@ -170,102 +132,6 @@ pub enum ValidationResult {
 	Invalid(InvalidCandidate),
 }
 
-impl std::convert::TryFrom<FromTableMisbehavior> for MisbehaviorReport {
-	type Error = ();
-
-	fn try_from(f: FromTableMisbehavior) -> Result<Self, Self::Error> {
-		match f.report {
-			TableMisbehavior::ValidityDoubleVote(
-				TableValidityDoubleVote::IssuedAndValidity((c, s1), (d, s2))
-			) => {
-				let receipt = c.clone();
-				let signed_1 = SignedFullStatement::new(
-					Statement::Seconded(c),
-					f.id,
-					s1,
-					&f.signing_context,
-					&f.key,
-				).ok_or(())?;
-				let signed_2 = SignedFullStatement::new(
-					Statement::Valid(d),
-					f.id,
-					s2,
-					&f.signing_context,
-					&f.key,
-				).ok_or(())?;
-
-				Ok(MisbehaviorReport::SelfContradiction(receipt.to_plain(), signed_1, signed_2))
-			}
-			TableMisbehavior::ValidityDoubleVote(
-				TableValidityDoubleVote::IssuedAndInvalidity((c, s1), (d, s2))
-			) => {
-				let receipt = c.clone();
-				let signed_1 = SignedFullStatement::new(
-					Statement::Seconded(c),
-					f.id,
-					s1,
-					&f.signing_context,
-					&f.key,
-				).ok_or(())?;
-				let signed_2 = SignedFullStatement::new(
-					Statement::Invalid(d),
-					f.id,
-					s2,
-					&f.signing_context,
-					&f.key,
-				).ok_or(())?;
-
-				Ok(MisbehaviorReport::SelfContradiction(receipt.to_plain(), signed_1, signed_2))
-			}
-			TableMisbehavior::ValidityDoubleVote(
-				TableValidityDoubleVote::ValidityAndInvalidity(c, s1, s2)
-			) => {
-				let signed_1 = SignedFullStatement::new(
-					Statement::Valid(c.hash()),
-					f.id,
-					s1,
-					&f.signing_context,
-					&f.key,
-				).ok_or(())?;
-				let signed_2 = SignedFullStatement::new(
-					Statement::Invalid(c.hash()),
-					f.id,
-					s2,
-					&f.signing_context,
-					&f.key,
-				).ok_or(())?;
-
-				Ok(MisbehaviorReport::SelfContradiction(c.to_plain(), signed_1, signed_2))
-			}
-			TableMisbehavior::MultipleCandidates(
-				TableMultipleCandidates {
-					first,
-					second,
-				}
-			) => {
-				let signed_1 = SignedFullStatement::new(
-					Statement::Seconded(first.0),
-					f.id,
-					first.1,
-					&f.signing_context,
-					&f.key,
-				).ok_or(())?;
-
-				let signed_2 = SignedFullStatement::new(
-					Statement::Seconded(second.0),
-					f.id,
-					second.1,
-					&f.signing_context,
-					&f.key,
-				).ok_or(())?;
-
-				Ok(MisbehaviorReport::DoubleVote(signed_1, signed_2))
-			}
-			_ => Err(()),
-		}
-	}
-}
-
 /// The output of a collator.
 ///
 /// This differs from `CandidateCommitments` in two ways: