Make candidate validation timeouts configurable (#4001)

* pvf: make execution timeout configurable * guide: add timeouts to candidate validation params * add timeouts to candidate validation messages * fmt * port backing to use the backing pvf timeout * port approval-voting to use the execution timeout * port dispute participation to use the correct timeout * fmt * address grumbles & test failure
paritytech · Oct 4, 2021 · 8962e04 · 8962e04
1 parent 16d6607
commit 8962e04
Show file tree

Hide file tree

Showing 19 changed files with 192 additions and 62 deletions.
diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs
@@ -27,7 +27,7 @@ use polkadot_node_primitives::{
 	approval::{
 		BlockApprovalMeta, DelayTranche, IndirectAssignmentCert, IndirectSignedApprovalVote,
 	},
-	SignedDisputeStatement, ValidationResult,
+	SignedDisputeStatement, ValidationResult, APPROVAL_EXECUTION_TIMEOUT,
 };
 use polkadot_node_subsystem::{
 	errors::RecoveryError,
@@ -2235,6 +2235,7 @@ async fn launch_approval(
 					validation_code,
 					candidate.descriptor.clone(),
 					available_data.pov,
+					APPROVAL_EXECUTION_TIMEOUT,
 					val_tx,
 				)
 				.into(),

diff --git a/node/core/backing/src/lib.rs b/node/core/backing/src/lib.rs
@@ -32,6 +32,7 @@ use futures::{
 
 use polkadot_node_primitives::{
 	AvailableData, PoV, SignedDisputeStatement, SignedFullStatement, Statement, ValidationResult,
+	BACKING_EXECUTION_TIMEOUT,
 };
 use polkadot_node_subsystem_util::{
 	self as util,
@@ -380,7 +381,12 @@ async fn request_candidate_validation(
 	let (tx, rx) = oneshot::channel();
 
 	sender
-		.send_message(CandidateValidationMessage::ValidateFromChainState(candidate, pov, tx))
+		.send_message(CandidateValidationMessage::ValidateFromChainState(
+			candidate,
+			pov,
+			BACKING_EXECUTION_TIMEOUT,
+			tx,
+		))
 		.await;
 
 	match rx.await {

diff --git a/node/core/backing/src/tests.rs b/node/core/backing/src/tests.rs
@@ -317,9 +317,10 @@ fn backing_second_works() {
 				CandidateValidationMessage::ValidateFromChainState(
 					c,
 					pov,
+					timeout,
 					tx,
 				)
-			) if pov == pov && &c == candidate.descriptor() => {
+			) if pov == pov && &c == candidate.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
 				tx.send(Ok(
 					ValidationResult::Valid(CandidateCommitments {
 						head_data: expected_head_data.clone(),
@@ -476,9 +477,10 @@ fn backing_works() {
 				CandidateValidationMessage::ValidateFromChainState(
 					c,
 					pov,
+					timeout,
 					tx,
 				)
-			) if pov == pov && &c == candidate_a.descriptor() => {
+			) if pov == pov && &c == candidate_a.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
 				tx.send(Ok(
 					ValidationResult::Valid(CandidateCommitments {
 						head_data: expected_head_data.clone(),
@@ -669,9 +671,10 @@ fn backing_works_while_validation_ongoing() {
 				CandidateValidationMessage::ValidateFromChainState(
 					c,
 					pov,
+					timeout,
 					tx,
 				)
-			) if pov == pov && &c == candidate_a.descriptor() => {
+			) if pov == pov && &c == candidate_a.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
 				// we never validate the candidate. our local node
 				// shouldn't issue any statements.
 				std::mem::forget(tx);
@@ -834,9 +837,10 @@ fn backing_misbehavior_works() {
 				CandidateValidationMessage::ValidateFromChainState(
 					c,
 					pov,
+					timeout,
 					tx,
 				)
-			) if pov == pov && &c == candidate_a.descriptor() => {
+			) if pov == pov && &c == candidate_a.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
 				tx.send(Ok(
 					ValidationResult::Valid(CandidateCommitments {
 						head_data: expected_head_data.clone(),
@@ -980,9 +984,10 @@ fn backing_dont_second_invalid() {
 				CandidateValidationMessage::ValidateFromChainState(
 					c,
 					pov,
+					timeout,
 					tx,
 				)
-			) if pov == pov && &c == candidate_a.descriptor() => {
+			) if pov == pov && &c == candidate_a.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
 				tx.send(Ok(ValidationResult::Invalid(InvalidCandidate::BadReturn))).unwrap();
 			}
 		);
@@ -1008,9 +1013,10 @@ fn backing_dont_second_invalid() {
 				CandidateValidationMessage::ValidateFromChainState(
 					c,
 					pov,
+					timeout,
 					tx,
 				)
-			) if pov == pov && &c == candidate_b.descriptor() => {
+			) if pov == pov && &c == candidate_b.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
 				tx.send(Ok(
 					ValidationResult::Valid(CandidateCommitments {
 						head_data: expected_head_data.clone(),
@@ -1138,9 +1144,10 @@ fn backing_second_after_first_fails_works() {
 				CandidateValidationMessage::ValidateFromChainState(
 					c,
 					pov,
+					timeout,
 					tx,
 				)
-			) if pov == pov && &c == candidate.descriptor() => {
+			) if pov == pov && &c == candidate.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
 				tx.send(Ok(ValidationResult::Invalid(InvalidCandidate::BadReturn))).unwrap();
 			}
 		);
@@ -1186,6 +1193,7 @@ fn backing_second_after_first_fails_works() {
 					_,
 					pov,
 					_,
+					_,
 				)
 			) => {
 				assert_eq!(&*pov, &pov_to_second);
@@ -1270,9 +1278,10 @@ fn backing_works_after_failed_validation() {
 				CandidateValidationMessage::ValidateFromChainState(
 					c,
 					pov,
+					timeout,
 					tx,
 				)
-			) if pov == pov && &c == candidate.descriptor() => {
+			) if pov == pov && &c == candidate.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
 				tx.send(Err(ValidationFailed("Internal test error".into()))).unwrap();
 			}
 		);
@@ -1646,9 +1655,10 @@ fn retry_works() {
 				CandidateValidationMessage::ValidateFromChainState(
 					c,
 					pov,
+					timeout,
 					_tx,
 				)
-			) if pov == pov && &c == candidate.descriptor()
+			) if pov == pov && &c == candidate.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT
 		);
 		virtual_overseer
 	});

diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs
@@ -48,7 +48,7 @@ use parity_scale_codec::Encode;
 
 use futures::{channel::oneshot, prelude::*};
 
-use std::{path::PathBuf, sync::Arc};
+use std::{path::PathBuf, sync::Arc, time::Duration};
 
 use async_trait::async_trait;
 
@@ -135,6 +135,7 @@ where
 				CandidateValidationMessage::ValidateFromChainState(
 					descriptor,
 					pov,
+					timeout,
 					response_sender,
 				) => {
 					let bg = {
@@ -149,6 +150,7 @@ where
 								validation_host,
 								descriptor,
 								pov,
+								timeout,
 								&metrics,
 							)
 							.await;
@@ -165,6 +167,7 @@ where
 					validation_code,
 					descriptor,
 					pov,
+					timeout,
 					response_sender,
 				) => {
 					let bg = {
@@ -179,6 +182,7 @@ where
 								validation_code,
 								descriptor,
 								pov,
+								timeout,
 								&metrics,
 							)
 							.await;
@@ -322,6 +326,7 @@ async fn validate_from_chain_state<Sender>(
 	validation_host: ValidationHost,
 	descriptor: CandidateDescriptor,
 	pov: Arc<PoV>,
+	timeout: Duration,
 	metrics: &Metrics,
 ) -> Result<ValidationResult, ValidationFailed>
 where
@@ -347,6 +352,7 @@ where
 		validation_code,
 		descriptor.clone(),
 		pov,
+		timeout,
 		metrics,
 	)
 	.await;
@@ -377,6 +383,7 @@ async fn validate_candidate_exhaustive(
 	validation_code: ValidationCode,
 	descriptor: CandidateDescriptor,
 	pov: Arc<PoV>,
+	timeout: Duration,
 	metrics: &Metrics,
 ) -> Result<ValidationResult, ValidationFailed> {
 	let _timer = metrics.time_validate_candidate_exhaustive();
@@ -430,7 +437,7 @@ async fn validate_candidate_exhaustive(
 	};
 
 	let result = validation_backend
-		.validate_candidate(raw_validation_code.to_vec(), params)
+		.validate_candidate(raw_validation_code.to_vec(), timeout, params)
 		.await;
 
 	if let Err(ref e) = result {
@@ -475,6 +482,7 @@ trait ValidationBackend {
 	async fn validate_candidate(
 		&mut self,
 		raw_validation_code: Vec<u8>,
+		timeout: Duration,
 		params: ValidationParams,
 	) -> Result<WasmValidationResult, ValidationError>;
 }
@@ -484,12 +492,14 @@ impl ValidationBackend for ValidationHost {
 	async fn validate_candidate(
 		&mut self,
 		raw_validation_code: Vec<u8>,
+		timeout: Duration,
 		params: ValidationParams,
 	) -> Result<WasmValidationResult, ValidationError> {
 		let (tx, rx) = oneshot::channel();
 		if let Err(err) = self
 			.execute_pvf(
 				Pvf::from_code(raw_validation_code),
+				timeout,
 				params.encode(),
 				polkadot_node_core_pvf::Priority::Normal,
 				tx,

diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs
@@ -341,6 +341,7 @@ impl ValidationBackend for MockValidatorBackend {
 	async fn validate_candidate(
 		&mut self,
 		_raw_validation_code: Vec<u8>,
+		_timeout: Duration,
 		_params: ValidationParams,
 	) -> Result<WasmValidationResult, ValidationError> {
 		self.result.clone()
@@ -384,6 +385,7 @@ fn candidate_validation_ok_is_ok() {
 		validation_code,
 		descriptor,
 		Arc::new(pov),
+		Duration::from_secs(0),
 		&Default::default(),
 	))
 	.unwrap();
@@ -426,6 +428,7 @@ fn candidate_validation_bad_return_is_invalid() {
 		validation_code,
 		descriptor,
 		Arc::new(pov),
+		Duration::from_secs(0),
 		&Default::default(),
 	))
 	.unwrap();
@@ -461,6 +464,7 @@ fn candidate_validation_timeout_is_internal_error() {
 		validation_code,
 		descriptor,
 		Arc::new(pov),
+		Duration::from_secs(0),
 		&Default::default(),
 	));
 
@@ -495,6 +499,7 @@ fn candidate_validation_code_mismatch_is_invalid() {
 		validation_code,
 		descriptor,
 		Arc::new(pov),
+		Duration::from_secs(0),
 		&Default::default(),
 	))
 	.unwrap();
@@ -534,6 +539,7 @@ fn compressed_code_works() {
 		validation_code,
 		descriptor,
 		Arc::new(pov),
+		Duration::from_secs(0),
 		&Default::default(),
 	));
 
@@ -573,6 +579,7 @@ fn code_decompression_failure_is_invalid() {
 		validation_code,
 		descriptor,
 		Arc::new(pov),
+		Duration::from_secs(0),
 		&Default::default(),
 	));
 
@@ -613,6 +620,7 @@ fn pov_decompression_failure_is_invalid() {
 		validation_code,
 		descriptor,
 		Arc::new(pov),
+		Duration::from_secs(0),
 		&Default::default(),
 	));
 

diff --git a/node/core/dispute-participation/src/lib.rs b/node/core/dispute-participation/src/lib.rs
@@ -22,7 +22,7 @@
 
 use futures::{channel::oneshot, prelude::*};
 
-use polkadot_node_primitives::ValidationResult;
+use polkadot_node_primitives::{ValidationResult, APPROVAL_EXECUTION_TIMEOUT};
 use polkadot_node_subsystem::{
 	errors::{RecoveryError, RuntimeApiError},
 	messages::{
@@ -269,11 +269,16 @@ async fn participate(
 
 	// we issue a request to validate the candidate with the provided exhaustive
 	// parameters
+	//
+	// We use the approval execution timeout because this is intended to
+	// be run outside of backing and therefore should be subject to the
+	// same level of leeway.
 	ctx.send_message(CandidateValidationMessage::ValidateFromExhaustive(
 		available_data.validation_data,
 		validation_code,
 		candidate_receipt.descriptor.clone(),
 		available_data.pov,
+		APPROVAL_EXECUTION_TIMEOUT,
 		validation_tx,
 	))
 	.await;

diff --git a/node/core/dispute-participation/src/tests.rs b/node/core/dispute-participation/src/tests.rs
@@ -295,8 +295,8 @@ fn cast_invalid_vote_if_validation_fails_or_is_invalid() {
 			assert_matches!(
 				virtual_overseer.recv().await,
 				AllMessages::CandidateValidation(
-					CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, tx)
-				) => {
+					CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, timeout, tx)
+				) if timeout == APPROVAL_EXECUTION_TIMEOUT => {
 					tx.send(Ok(ValidationResult::Invalid(InvalidCandidate::Timeout))).unwrap();
 				},
 				"overseer did not receive candidate validation message",
@@ -331,8 +331,8 @@ fn cast_invalid_vote_if_validation_passes_but_commitments_dont_match() {
 			assert_matches!(
 				virtual_overseer.recv().await,
 				AllMessages::CandidateValidation(
-					CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, tx)
-				) => {
+					CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, timeout, tx)
+				) if timeout == APPROVAL_EXECUTION_TIMEOUT => {
 					let mut commitments = CandidateCommitments::default();
 					// this should lead to a commitments hash mismatch
 					commitments.processed_downward_messages = 42;
@@ -371,8 +371,8 @@ fn cast_valid_vote_if_validation_passes() {
 			assert_matches!(
 				virtual_overseer.recv().await,
 				AllMessages::CandidateValidation(
-					CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, tx)
-				) => {
+					CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, timeout, tx)
+				) if timeout == APPROVAL_EXECUTION_TIMEOUT => {
 					tx.send(Ok(ValidationResult::Valid(Default::default(), Default::default()))).unwrap();
 				},
 				"overseer did not receive candidate validation message",
@@ -408,8 +408,8 @@ fn failure_to_store_available_data_does_not_preclude_participation() {
 			assert_matches!(
 				virtual_overseer.recv().await,
 				AllMessages::CandidateValidation(
-					CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, tx)
-				) => {
+					CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, timeout, tx)
+				) if timeout == APPROVAL_EXECUTION_TIMEOUT => {
 					tx.send(Err(ValidationFailed("fail".to_string()))).unwrap();
 				},
 				"overseer did not receive candidate validation message",