From 093d5371a6c39d272257e6b82cd076f88a401b9d Mon Sep 17 00:00:00 2001 From: Joao pedro Santos Date: Mon, 6 Nov 2023 14:01:16 -0300 Subject: [PATCH 01/15] remove retry from backers on failed candidate validation --- polkadot/node/core/approval-voting/src/lib.rs | 4 +- .../node/core/approval-voting/src/tests.rs | 2 +- polkadot/node/core/backing/src/lib.rs | 4 +- polkadot/node/core/backing/src/tests/mod.rs | 20 +- .../src/tests/prospective_parachains.rs | 2 +- .../node/core/candidate-validation/src/lib.rs | 60 ++++-- .../core/candidate-validation/src/tests.rs | 192 +++++++++--------- .../src/participation/mod.rs | 16 +- .../src/participation/tests.rs | 8 +- polkadot/node/malus/src/variants/common.rs | 8 +- .../node/overseer/examples/minimal-example.rs | 4 +- polkadot/node/overseer/src/tests.rs | 24 +-- polkadot/node/subsystem-types/src/messages.rs | 10 +- polkadot/primitives/src/lib.rs | 50 ++--- polkadot/primitives/src/v6/executor_params.rs | 10 +- polkadot/primitives/src/v6/mod.rs | 6 +- .../src/configuration/benchmarking.rs | 6 +- 17 files changed, 225 insertions(+), 201 deletions(-) diff --git a/polkadot/node/core/approval-voting/src/lib.rs b/polkadot/node/core/approval-voting/src/lib.rs index e7cbde5673921..7ae65f11893dd 100644 --- a/polkadot/node/core/approval-voting/src/lib.rs +++ b/polkadot/node/core/approval-voting/src/lib.rs @@ -54,7 +54,7 @@ use polkadot_node_subsystem_util::{ }; use polkadot_primitives::{ ApprovalVote, BlockNumber, CandidateHash, CandidateIndex, CandidateReceipt, DisputeStatement, - ExecutorParams, GroupIndex, Hash, PvfExecTimeoutKind, SessionIndex, SessionInfo, + ExecutorParams, GroupIndex, Hash, PvfExecKind, SessionIndex, SessionInfo, ValidDisputeStatementKind, ValidatorId, ValidatorIndex, ValidatorPair, ValidatorSignature, }; use sc_keystore::LocalKeystore; @@ -2867,7 +2867,7 @@ async fn launch_approval( candidate.clone(), available_data.pov, executor_params, - PvfExecTimeoutKind::Approval, + PvfExecKind::Approval, val_tx, )) .await; diff --git a/polkadot/node/core/approval-voting/src/tests.rs b/polkadot/node/core/approval-voting/src/tests.rs index 13213c158cdb2..bef6e167d9e04 100644 --- a/polkadot/node/core/approval-voting/src/tests.rs +++ b/polkadot/node/core/approval-voting/src/tests.rs @@ -2704,7 +2704,7 @@ async fn handle_double_assignment_import( assert_matches!( overseer_recv(virtual_overseer).await, - AllMessages::CandidateValidation(CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, _, timeout, tx)) if timeout == PvfExecTimeoutKind::Approval => { + AllMessages::CandidateValidation(CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, _, timeout, tx)) if timeout == PvfExecKind::Approval => { tx.send(Ok(ValidationResult::Valid(Default::default(), Default::default()))) .unwrap(); } diff --git a/polkadot/node/core/backing/src/lib.rs b/polkadot/node/core/backing/src/lib.rs index 27b5972d98274..9b891f886c908 100644 --- a/polkadot/node/core/backing/src/lib.rs +++ b/polkadot/node/core/backing/src/lib.rs @@ -106,7 +106,7 @@ use polkadot_node_subsystem_util::{ use polkadot_primitives::{ BackedCandidate, CandidateCommitments, CandidateHash, CandidateReceipt, CommittedCandidateReceipt, CoreIndex, CoreState, ExecutorParams, Hash, Id as ParaId, - PersistedValidationData, PvfExecTimeoutKind, SigningContext, ValidationCode, ValidatorId, + PersistedValidationData, PvfExecKind, SigningContext, ValidationCode, ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation, }; use sp_keystore::KeystorePtr; @@ -566,7 +566,7 @@ async fn request_candidate_validation( candidate_receipt, pov, executor_params, - PvfExecTimeoutKind::Backing, + PvfExecKind::Backing, tx, )) .await; diff --git a/polkadot/node/core/backing/src/tests/mod.rs b/polkadot/node/core/backing/src/tests/mod.rs index 35d17f3d905e4..3718418f560a4 100644 --- a/polkadot/node/core/backing/src/tests/mod.rs +++ b/polkadot/node/core/backing/src/tests/mod.rs @@ -33,7 +33,7 @@ use polkadot_node_subsystem::{ }; use polkadot_node_subsystem_test_helpers as test_helpers; use polkadot_primitives::{ - CandidateDescriptor, GroupRotationInfo, HeadData, PersistedValidationData, PvfExecTimeoutKind, + CandidateDescriptor, GroupRotationInfo, HeadData, PersistedValidationData, PvfExecKind, ScheduledCore, SessionIndex, LEGACY_MIN_BACKING_VOTES, }; use sp_application_crypto::AppCrypto; @@ -351,7 +351,7 @@ async fn assert_validate_from_exhaustive( ) if _pvd == *pvd && _validation_code == *validation_code && *_pov == *pov && &candidate_receipt.descriptor == candidate.descriptor() && - timeout == PvfExecTimeoutKind::Backing && + timeout == PvfExecKind::Backing && candidate.commitments.hash() == candidate_receipt.commitments_hash => { tx.send(Ok(ValidationResult::Valid( @@ -557,7 +557,7 @@ fn backing_works() { ) if _pvd == pvd && _validation_code == validation_code && *_pov == pov && &candidate_receipt.descriptor == candidate_a.descriptor() && - timeout == PvfExecTimeoutKind::Backing && + timeout == PvfExecKind::Backing && candidate_a_commitments_hash == candidate_receipt.commitments_hash => { tx.send(Ok( @@ -736,7 +736,7 @@ fn backing_works_while_validation_ongoing() { ) if _pvd == pvd && _validation_code == validation_code && *_pov == pov && &candidate_receipt.descriptor == candidate_a.descriptor() && - timeout == PvfExecTimeoutKind::Backing && + timeout == PvfExecKind::Backing && candidate_a_commitments_hash == candidate_receipt.commitments_hash => { // we never validate the candidate. our local node @@ -897,7 +897,7 @@ fn backing_misbehavior_works() { ) if _pvd == pvd && _validation_code == validation_code && *_pov == pov && &candidate_receipt.descriptor == candidate_a.descriptor() && - timeout == PvfExecTimeoutKind::Backing && + timeout == PvfExecKind::Backing && candidate_a_commitments_hash == candidate_receipt.commitments_hash => { tx.send(Ok( @@ -1064,7 +1064,7 @@ fn backing_dont_second_invalid() { ) if _pvd == pvd_a && _validation_code == validation_code_a && *_pov == pov_block_a && &candidate_receipt.descriptor == candidate_a.descriptor() && - timeout == PvfExecTimeoutKind::Backing && + timeout == PvfExecKind::Backing && candidate_a.commitments.hash() == candidate_receipt.commitments_hash => { tx.send(Ok(ValidationResult::Invalid(InvalidCandidate::BadReturn))).unwrap(); @@ -1104,7 +1104,7 @@ fn backing_dont_second_invalid() { ) if pvd == pvd_b && _validation_code == validation_code_b && *_pov == pov_block_b && &candidate_receipt.descriptor == candidate_b.descriptor() && - timeout == PvfExecTimeoutKind::Backing && + timeout == PvfExecKind::Backing && candidate_b.commitments.hash() == candidate_receipt.commitments_hash => { tx.send(Ok( @@ -1231,7 +1231,7 @@ fn backing_second_after_first_fails_works() { ) if _pvd == pvd && _validation_code == validation_code && *_pov == pov && &candidate_receipt.descriptor == candidate.descriptor() && - timeout == PvfExecTimeoutKind::Backing && + timeout == PvfExecKind::Backing && candidate.commitments.hash() == candidate_receipt.commitments_hash => { tx.send(Ok(ValidationResult::Invalid(InvalidCandidate::BadReturn))).unwrap(); @@ -1375,7 +1375,7 @@ fn backing_works_after_failed_validation() { ) if _pvd == pvd && _validation_code == validation_code && *_pov == pov && &candidate_receipt.descriptor == candidate.descriptor() && - timeout == PvfExecTimeoutKind::Backing && + timeout == PvfExecKind::Backing && candidate.commitments.hash() == candidate_receipt.commitments_hash => { tx.send(Err(ValidationFailed("Internal test error".into()))).unwrap(); @@ -1641,7 +1641,7 @@ fn retry_works() { ) if _pvd == pvd && _validation_code == validation_code && *_pov == pov && &candidate_receipt.descriptor == candidate.descriptor() && - timeout == PvfExecTimeoutKind::Backing && + timeout == PvfExecKind::Backing && candidate.commitments.hash() == candidate_receipt.commitments_hash ); virtual_overseer diff --git a/polkadot/node/core/backing/src/tests/prospective_parachains.rs b/polkadot/node/core/backing/src/tests/prospective_parachains.rs index b79515ed37a6e..bce779deea8e0 100644 --- a/polkadot/node/core/backing/src/tests/prospective_parachains.rs +++ b/polkadot/node/core/backing/src/tests/prospective_parachains.rs @@ -239,7 +239,7 @@ async fn assert_validate_seconded_candidate( &_validation_code == validation_code && &*_pov == pov && &candidate_receipt.descriptor == candidate.descriptor() && - timeout == PvfExecTimeoutKind::Backing && + timeout == PvfExecKind::Backing && candidate.commitments.hash() == candidate_receipt.commitments_hash => { tx.send(Ok(ValidationResult::Valid( diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs index 93db7d11cee8b..9e2b1fa3453cc 100644 --- a/polkadot/node/core/candidate-validation/src/lib.rs +++ b/polkadot/node/core/candidate-validation/src/lib.rs @@ -46,10 +46,10 @@ use polkadot_parachain_primitives::primitives::{ use polkadot_primitives::{ executor_params::{ DEFAULT_APPROVAL_EXECUTION_TIMEOUT, DEFAULT_BACKING_EXECUTION_TIMEOUT, - DEFAULT_LENIENT_PREPARATION_TIMEOUT, DEFAULT_PRECHECK_PREPARATION_TIMEOUT, + DEFAULT_LENIENT_PREPARATION_TIMEOUT, DEFAULT_PRECHECK_PREPARATION_TIMEOUT, self, }, CandidateCommitments, CandidateDescriptor, CandidateReceipt, ExecutorParams, Hash, - OccupiedCoreAssumption, PersistedValidationData, PvfExecTimeoutKind, PvfPrepTimeoutKind, + OccupiedCoreAssumption, PersistedValidationData, PvfExecKind, PvfPrepTimeoutKind, ValidationCode, ValidationCodeHash, }; @@ -498,7 +498,7 @@ async fn validate_from_chain_state( candidate_receipt: CandidateReceipt, pov: Arc, executor_params: ExecutorParams, - exec_timeout_kind: PvfExecTimeoutKind, + exec_timeout_kind: PvfExecKind, metrics: &Metrics, ) -> Result where @@ -554,7 +554,7 @@ async fn validate_candidate_exhaustive( candidate_receipt: CandidateReceipt, pov: Arc, executor_params: ExecutorParams, - exec_timeout_kind: PvfExecTimeoutKind, + exec_timeout_kind: PvfExecKind, metrics: &Metrics, ) -> Result { let _timer = metrics.time_validate_candidate_exhaustive(); @@ -613,15 +613,37 @@ async fn validate_candidate_exhaustive( relay_parent_storage_root: persisted_validation_data.relay_parent_storage_root, }; - let result = validation_backend - .validate_candidate_with_retry( - raw_validation_code.to_vec(), - pvf_exec_timeout(&executor_params, exec_timeout_kind), - exec_timeout_kind, - params, - executor_params, - ) - .await; + let result = match exec_timeout_kind { + PvfExecKind::Backing => { + let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepTimeoutKind::Lenient); + let exec_timeout = pvf_exec_timeout(&executor_params, exec_timeout_kind); + let pvf = PvfPrepData::from_code( + raw_validation_code.to_vec(), + executor_params, + prep_timeout, + PrepareJobKind::Compilation, + ); + + validation_backend + .validate_candidate( + pvf, + exec_timeout, + params.encode(), + ) + .await + }, + PvfExecKind::Approval => { + validation_backend + .validate_candidate_with_retry( + raw_validation_code.to_vec(), + pvf_exec_timeout(&executor_params, exec_timeout_kind), + exec_timeout_kind, + params, + executor_params, + ) + .await + } + }; if let Err(ref error) = result { gum::info!(target: LOG_TARGET, ?para_id, ?error, "Failed to validate candidate"); @@ -710,7 +732,7 @@ trait ValidationBackend { &mut self, raw_validation_code: Vec, exec_timeout: Duration, - exec_timeout_kind: PvfExecTimeoutKind, + exec_timeout_kind: PvfExecKind, params: ValidationParams, executor_params: ExecutorParams, ) -> Result { @@ -733,8 +755,8 @@ trait ValidationBackend { } let retry_delay = match exec_timeout_kind { - PvfExecTimeoutKind::Backing => PVF_BACKING_EXECUTION_RETRY_DELAY, - PvfExecTimeoutKind::Approval => PVF_APPROVAL_EXECUTION_RETRY_DELAY, + PvfExecKind::Backing => PVF_BACKING_EXECUTION_RETRY_DELAY, + PvfExecKind::Approval => PVF_APPROVAL_EXECUTION_RETRY_DELAY, }; // Allow limited retries for each kind of error. @@ -868,12 +890,12 @@ fn pvf_prep_timeout(executor_params: &ExecutorParams, kind: PvfPrepTimeoutKind) } } -fn pvf_exec_timeout(executor_params: &ExecutorParams, kind: PvfExecTimeoutKind) -> Duration { +fn pvf_exec_timeout(executor_params: &ExecutorParams, kind: PvfExecKind) -> Duration { if let Some(timeout) = executor_params.pvf_exec_timeout(kind) { return timeout } match kind { - PvfExecTimeoutKind::Backing => DEFAULT_BACKING_EXECUTION_TIMEOUT, - PvfExecTimeoutKind::Approval => DEFAULT_APPROVAL_EXECUTION_TIMEOUT, + PvfExecKind::Backing => DEFAULT_BACKING_EXECUTION_TIMEOUT, + PvfExecKind::Approval => DEFAULT_APPROVAL_EXECUTION_TIMEOUT, } } diff --git a/polkadot/node/core/candidate-validation/src/tests.rs b/polkadot/node/core/candidate-validation/src/tests.rs index af530a20c4e0c..327af62222b29 100644 --- a/polkadot/node/core/candidate-validation/src/tests.rs +++ b/polkadot/node/core/candidate-validation/src/tests.rs @@ -430,14 +430,14 @@ fn candidate_validation_ok_is_ok() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data.clone(), - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecTimeoutKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data.clone(), + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -480,16 +480,16 @@ fn candidate_validation_bad_return_is_invalid() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Err( + MockValidateCandidateBackend::with_hardcoded_result(Err( ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout), )), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecTimeoutKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -545,17 +545,17 @@ fn candidate_validation_one_ambiguous_error_is_valid() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![ + MockValidateCandidateBackend::with_hardcoded_result_list(vec![ Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), Ok(validation_result), ]), - validation_data.clone(), - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecTimeoutKind::Backing, - &Default::default(), + validation_data.clone(), + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -598,17 +598,17 @@ fn candidate_validation_multiple_ambiguous_errors_is_invalid() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![ + MockValidateCandidateBackend::with_hardcoded_result_list(vec![ Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), ]), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecTimeoutKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -645,20 +645,20 @@ fn candidate_validation_retry_internal_errors() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![ + MockValidateCandidateBackend::with_hardcoded_result_list(vec![ Err(InternalValidationError::HostCommunication("foo".into()).into()), // Throw an AWD error, we should still retry again. Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), // Throw another internal error. Err(InternalValidationError::HostCommunication("bar".into()).into()), ]), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecTimeoutKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Err(ValidationFailed(s)) if s.contains("bar")); @@ -694,20 +694,20 @@ fn candidate_validation_retry_panic_errors() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![ + MockValidateCandidateBackend::with_hardcoded_result_list(vec![ Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("foo".into()))), // Throw an AWD error, we should still retry again. Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), // Throw another panic error. Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("bar".into()))), ]), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecTimeoutKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(s))) if s == "bar".to_string()); @@ -742,16 +742,16 @@ fn candidate_validation_timeout_is_internal_error() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Err( + MockValidateCandidateBackend::with_hardcoded_result(Err( ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout), )), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecTimeoutKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::Timeout))); @@ -789,14 +789,14 @@ fn candidate_validation_commitment_hash_mismatch_is_invalid() { }; let result = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecTimeoutKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -836,16 +836,16 @@ fn candidate_validation_code_mismatch_is_invalid() { let (_ctx, _ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Err( + MockValidateCandidateBackend::with_hardcoded_result(Err( ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout), )), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecTimeoutKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -895,14 +895,14 @@ fn compressed_code_works() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecTimeoutKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Ok(ValidationResult::Valid(_, _))); @@ -946,14 +946,14 @@ fn code_decompression_failure_is_error() { let (_ctx, _ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecTimeoutKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Err(_)); @@ -998,14 +998,14 @@ fn pov_decompression_failure_is_invalid() { let (_ctx, _ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecTimeoutKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::PoVDecompressionFailure))); diff --git a/polkadot/node/core/dispute-coordinator/src/participation/mod.rs b/polkadot/node/core/dispute-coordinator/src/participation/mod.rs index 35d07b411e8f6..1187a098ae44f 100644 --- a/polkadot/node/core/dispute-coordinator/src/participation/mod.rs +++ b/polkadot/node/core/dispute-coordinator/src/participation/mod.rs @@ -32,7 +32,7 @@ use polkadot_node_subsystem::{ }; use polkadot_node_subsystem_util::runtime::get_validation_code_by_hash; use polkadot_primitives::{ - BlockNumber, CandidateHash, CandidateReceipt, Hash, PvfExecTimeoutKind, SessionIndex, + BlockNumber, CandidateHash, CandidateReceipt, Hash, PvfExecKind, SessionIndex, }; use crate::LOG_TARGET; @@ -381,13 +381,13 @@ async fn participate( let (validation_tx, validation_rx) = oneshot::channel(); sender .send_message(CandidateValidationMessage::ValidateFromExhaustive( - available_data.validation_data, - validation_code, - req.candidate_receipt().clone(), - available_data.pov, - req.executor_params(), - PvfExecTimeoutKind::Approval, - validation_tx, + available_data.validation_data, + validation_code, + req.candidate_receipt().clone(), + available_data.pov, + req.executor_params(), + PvfExecKind::Approval, + validation_tx, )) .await; diff --git a/polkadot/node/core/dispute-coordinator/src/participation/tests.rs b/polkadot/node/core/dispute-coordinator/src/participation/tests.rs index ee6b950720e5f..9e50aa22540f6 100644 --- a/polkadot/node/core/dispute-coordinator/src/participation/tests.rs +++ b/polkadot/node/core/dispute-coordinator/src/participation/tests.rs @@ -116,7 +116,7 @@ pub async fn participation_full_happy_path( ctx_handle.recv().await, AllMessages::CandidateValidation( CandidateValidationMessage::ValidateFromExhaustive(_, _, candidate_receipt, _, _, timeout, tx) - ) if timeout == PvfExecTimeoutKind::Approval => { + ) if timeout == PvfExecKind::Approval => { if expected_commitments_hash != candidate_receipt.commitments_hash { tx.send(Ok(ValidationResult::Invalid(InvalidCandidate::CommitmentsHashMismatch))).unwrap(); } else { @@ -451,7 +451,7 @@ fn cast_invalid_vote_if_validation_fails_or_is_invalid() { ctx_handle.recv().await, AllMessages::CandidateValidation( CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, _, timeout, tx) - ) if timeout == PvfExecTimeoutKind::Approval => { + ) if timeout == PvfExecKind::Approval => { tx.send(Ok(ValidationResult::Invalid(InvalidCandidate::Timeout))).unwrap(); }, "overseer did not receive candidate validation message", @@ -488,7 +488,7 @@ fn cast_invalid_vote_if_commitments_dont_match() { ctx_handle.recv().await, AllMessages::CandidateValidation( CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, _, timeout, tx) - ) if timeout == PvfExecTimeoutKind::Approval => { + ) if timeout == PvfExecKind::Approval => { tx.send(Ok(ValidationResult::Invalid(InvalidCandidate::CommitmentsHashMismatch))).unwrap(); }, "overseer did not receive candidate validation message", @@ -525,7 +525,7 @@ fn cast_valid_vote_if_validation_passes() { ctx_handle.recv().await, AllMessages::CandidateValidation( CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, _, timeout, tx) - ) if timeout == PvfExecTimeoutKind::Approval => { + ) if timeout == PvfExecKind::Approval => { tx.send(Ok(ValidationResult::Valid(dummy_candidate_commitments(None), PersistedValidationData::default()))).unwrap(); }, "overseer did not receive candidate validation message", diff --git a/polkadot/node/malus/src/variants/common.rs b/polkadot/node/malus/src/variants/common.rs index 365b2f16ac21b..6849e9e594f06 100644 --- a/polkadot/node/malus/src/variants/common.rs +++ b/polkadot/node/malus/src/variants/common.rs @@ -30,7 +30,7 @@ use polkadot_node_subsystem::{ use polkadot_primitives::{ CandidateCommitments, CandidateDescriptor, CandidateReceipt, PersistedValidationData, - PvfExecTimeoutKind, + PvfExecKind, }; use futures::channel::oneshot; @@ -90,10 +90,10 @@ impl FakeCandidateValidation { } } - fn should_misbehave(&self, timeout: PvfExecTimeoutKind) -> bool { + fn should_misbehave(&self, timeout: PvfExecKind) -> bool { match timeout { - PvfExecTimeoutKind::Backing => self.includes_backing(), - PvfExecTimeoutKind::Approval => self.includes_approval(), + PvfExecKind::Backing => self.includes_backing(), + PvfExecKind::Approval => self.includes_approval(), } } } diff --git a/polkadot/node/overseer/examples/minimal-example.rs b/polkadot/node/overseer/examples/minimal-example.rs index cffdfd9f8aa14..5e1b294370752 100644 --- a/polkadot/node/overseer/examples/minimal-example.rs +++ b/polkadot/node/overseer/examples/minimal-example.rs @@ -32,7 +32,7 @@ use polkadot_overseer::{ gen::{FromOrchestra, SpawnedSubsystem}, HeadSupportsParachains, SubsystemError, }; -use polkadot_primitives::{CandidateReceipt, Hash, PvfExecTimeoutKind}; +use polkadot_primitives::{CandidateReceipt, Hash, PvfExecKind}; struct AlwaysSupportsParachains; @@ -77,7 +77,7 @@ impl Subsystem1 { candidate_receipt, PoV { block_data: BlockData(Vec::new()) }.into(), Default::default(), - PvfExecTimeoutKind::Backing, + PvfExecKind::Backing, tx, ); ctx.send_message(msg).await; diff --git a/polkadot/node/overseer/src/tests.rs b/polkadot/node/overseer/src/tests.rs index c17613fb7ea5d..c813839fb5563 100644 --- a/polkadot/node/overseer/src/tests.rs +++ b/polkadot/node/overseer/src/tests.rs @@ -29,8 +29,8 @@ use polkadot_node_subsystem_types::messages::{ NetworkBridgeEvent, ReportPeerMessage, RuntimeApiRequest, }; use polkadot_primitives::{ - CandidateHash, CandidateReceipt, CollatorPair, Id as ParaId, InvalidDisputeStatementKind, - PvfExecTimeoutKind, SessionIndex, ValidDisputeStatementKind, ValidatorIndex, + CandidateHash, CandidateReceipt, CollatorPair, Id as ParaId, InvalidDisputeStatementKind, + PvfExecKind, SessionIndex, ValidDisputeStatementKind, ValidatorIndex, }; use crate::{ @@ -103,11 +103,11 @@ where let (tx, _) = oneshot::channel(); ctx.send_message(CandidateValidationMessage::ValidateFromChainState( - candidate_receipt, - PoV { block_data: BlockData(Vec::new()) }.into(), - Default::default(), - PvfExecTimeoutKind::Backing, - tx, + candidate_receipt, + PoV { block_data: BlockData(Vec::new()) }.into(), + Default::default(), + PvfExecKind::Backing, + tx, )) .await; c += 1; @@ -801,11 +801,11 @@ fn test_candidate_validation_msg() -> CandidateValidationMessage { }; CandidateValidationMessage::ValidateFromChainState( - candidate_receipt, - pov, - Default::default(), - PvfExecTimeoutKind::Backing, - sender, + candidate_receipt, + pov, + Default::default(), + PvfExecKind::Backing, + sender, ) } diff --git a/polkadot/node/subsystem-types/src/messages.rs b/polkadot/node/subsystem-types/src/messages.rs index ce4fa6633dc8b..53a7b98170c29 100644 --- a/polkadot/node/subsystem-types/src/messages.rs +++ b/polkadot/node/subsystem-types/src/messages.rs @@ -47,7 +47,7 @@ use polkadot_primitives::{ CoreState, DisputeState, ExecutorParams, GroupIndex, GroupRotationInfo, Hash, Header as BlockHeader, Id as ParaId, InboundDownwardMessage, InboundHrmpMessage, MultiDisputeStatementSet, OccupiedCoreAssumption, PersistedValidationData, PvfCheckStatement, - PvfExecTimeoutKind, SessionIndex, SessionInfo, SignedAvailabilityBitfield, + PvfExecKind, SessionIndex, SessionInfo, SignedAvailabilityBitfield, SignedAvailabilityBitfields, ValidationCode, ValidationCodeHash, ValidatorId, ValidatorIndex, ValidatorSignature, }; @@ -147,8 +147,8 @@ pub enum CandidateValidationMessage { CandidateReceipt, Arc, ExecutorParams, - /// Execution timeout - PvfExecTimeoutKind, + /// Execution timeout and retry + PvfExecKind, oneshot::Sender>, ), /// Validate a candidate with provided, exhaustive parameters for validation. @@ -166,8 +166,8 @@ pub enum CandidateValidationMessage { CandidateReceipt, Arc, ExecutorParams, - /// Execution timeout - PvfExecTimeoutKind, + /// Execution timeout and retry + PvfExecKind, oneshot::Sender>, ), /// Try to compile the given validation code and send back diff --git a/polkadot/primitives/src/lib.rs b/polkadot/primitives/src/lib.rs index 4ba8b8b031fcc..fe7613e049956 100644 --- a/polkadot/primitives/src/lib.rs +++ b/polkadot/primitives/src/lib.rs @@ -34,31 +34,31 @@ pub mod runtime_api; // Current primitives not requiring versioning are exported here. // Primitives requiring versioning must not be exported and must be referred by an exact version. pub use v6::{ - async_backing, byzantine_threshold, check_candidate_backing, collator_signature_payload, - effective_minimum_backing_votes, executor_params, metric_definitions, slashing, - supermajority_threshold, well_known_keys, AbridgedHostConfiguration, AbridgedHrmpChannel, - AccountId, AccountIndex, AccountPublic, ApprovalVote, AssignmentId, AsyncBackingParams, - AuthorityDiscoveryId, AvailabilityBitfield, BackedCandidate, Balance, BlakeTwo256, Block, - BlockId, BlockNumber, CandidateCommitments, CandidateDescriptor, CandidateEvent, CandidateHash, - CandidateIndex, CandidateReceipt, CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, - CollatorId, CollatorSignature, CommittedCandidateReceipt, CompactStatement, ConsensusLog, - CoreIndex, CoreState, DisputeState, DisputeStatement, DisputeStatementSet, DownwardMessage, - EncodeAs, ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, - ExplicitDisputeStatement, GroupIndex, GroupRotationInfo, Hash, HashT, HeadData, Header, - HorizontalMessages, HrmpChannelId, Id, InboundDownwardMessage, InboundHrmpMessage, IndexedVec, - InherentData, InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, Nonce, - OccupiedCore, OccupiedCoreAssumption, OutboundHrmpMessage, ParathreadClaim, ParathreadEntry, - PersistedValidationData, PvfCheckStatement, PvfExecTimeoutKind, PvfPrepTimeoutKind, - RuntimeMetricLabel, RuntimeMetricLabelValue, RuntimeMetricLabelValues, RuntimeMetricLabels, - RuntimeMetricOp, RuntimeMetricUpdate, ScheduledCore, ScrapedOnChainVotes, SessionIndex, - SessionInfo, Signature, Signed, SignedAvailabilityBitfield, SignedAvailabilityBitfields, - SignedStatement, SigningContext, Slot, UncheckedSigned, UncheckedSignedAvailabilityBitfield, - UncheckedSignedAvailabilityBitfields, UncheckedSignedStatement, UpgradeGoAhead, - UpgradeRestriction, UpwardMessage, ValidDisputeStatementKind, ValidationCode, - ValidationCodeHash, ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation, - ValidityError, ASSIGNMENT_KEY_TYPE_ID, LEGACY_MIN_BACKING_VOTES, LOWEST_PUBLIC_ID, - MAX_CODE_SIZE, MAX_HEAD_DATA_SIZE, MAX_POV_SIZE, ON_DEMAND_DEFAULT_QUEUE_MAX_SIZE, - PARACHAINS_INHERENT_IDENTIFIER, PARACHAIN_KEY_TYPE_ID, + async_backing, byzantine_threshold, check_candidate_backing, collator_signature_payload, + effective_minimum_backing_votes, executor_params, metric_definitions, slashing, + supermajority_threshold, well_known_keys, AbridgedHostConfiguration, AbridgedHrmpChannel, + AccountId, AccountIndex, AccountPublic, ApprovalVote, AssignmentId, AsyncBackingParams, + AuthorityDiscoveryId, AvailabilityBitfield, BackedCandidate, Balance, BlakeTwo256, Block, + BlockId, BlockNumber, CandidateCommitments, CandidateDescriptor, CandidateEvent, CandidateHash, + CandidateIndex, CandidateReceipt, CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, + CollatorId, CollatorSignature, CommittedCandidateReceipt, CompactStatement, ConsensusLog, + CoreIndex, CoreState, DisputeState, DisputeStatement, DisputeStatementSet, DownwardMessage, + EncodeAs, ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, + ExplicitDisputeStatement, GroupIndex, GroupRotationInfo, Hash, HashT, HeadData, Header, + HorizontalMessages, HrmpChannelId, Id, InboundDownwardMessage, InboundHrmpMessage, IndexedVec, + InherentData, InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, Nonce, + OccupiedCore, OccupiedCoreAssumption, OutboundHrmpMessage, ParathreadClaim, ParathreadEntry, + PersistedValidationData, PvfCheckStatement, PvfExecKind, PvfPrepTimeoutKind, + RuntimeMetricLabel, RuntimeMetricLabelValue, RuntimeMetricLabelValues, RuntimeMetricLabels, + RuntimeMetricOp, RuntimeMetricUpdate, ScheduledCore, ScrapedOnChainVotes, SessionIndex, + SessionInfo, Signature, Signed, SignedAvailabilityBitfield, SignedAvailabilityBitfields, + SignedStatement, SigningContext, Slot, UncheckedSigned, UncheckedSignedAvailabilityBitfield, + UncheckedSignedAvailabilityBitfields, UncheckedSignedStatement, UpgradeGoAhead, + UpgradeRestriction, UpwardMessage, ValidDisputeStatementKind, ValidationCode, + ValidationCodeHash, ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation, + ValidityError, ASSIGNMENT_KEY_TYPE_ID, LEGACY_MIN_BACKING_VOTES, LOWEST_PUBLIC_ID, + MAX_CODE_SIZE, MAX_HEAD_DATA_SIZE, MAX_POV_SIZE, ON_DEMAND_DEFAULT_QUEUE_MAX_SIZE, + PARACHAINS_INHERENT_IDENTIFIER, PARACHAIN_KEY_TYPE_ID, }; #[cfg(feature = "std")] diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index bb9980f687962..782445fa66fb2 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -21,7 +21,7 @@ //! by the first element of the vector). Decoding to a usable semantics structure is //! done in `polkadot-node-core-pvf`. -use crate::{BlakeTwo256, HashT as _, PvfExecTimeoutKind, PvfPrepTimeoutKind}; +use crate::{BlakeTwo256, HashT as _, PvfExecKind, PvfPrepTimeoutKind}; use parity_scale_codec::{Decode, Encode}; use polkadot_core_primitives::Hash; use scale_info::TypeInfo; @@ -104,7 +104,7 @@ pub enum ExecutorParam { /// Always ensure that `backing_timeout` < `approval_timeout`. /// When absent, the default values will be used. #[codec(index = 6)] - PvfExecTimeout(PvfExecTimeoutKind, u64), + PvfExecTimeout(PvfExecKind, u64), /// Enables WASM bulk memory proposal #[codec(index = 7)] WasmExtBulkMemory, @@ -186,7 +186,7 @@ impl ExecutorParams { } /// Returns a PVF execution timeout, if any - pub fn pvf_exec_timeout(&self, kind: PvfExecTimeoutKind) -> Option { + pub fn pvf_exec_timeout(&self, kind: PvfExecKind) -> Option { for param in &self.0 { if let ExecutorParam::PvfExecTimeout(k, timeout) = param { if kind == *k { @@ -246,8 +246,8 @@ impl ExecutorParams { PvfPrepTimeoutKind::Lenient => "PvfPrepTimeoutKind::Lenient", }, PvfExecTimeout(kind, _) => match kind { - PvfExecTimeoutKind::Backing => "PvfExecTimeoutKind::Backing", - PvfExecTimeoutKind::Approval => "PvfExecTimeoutKind::Approval", + PvfExecKind::Backing => "PvfExecKind::Backing", + PvfExecKind::Approval => "PvfExecKind::Approval", }, WasmExtBulkMemory => "WasmExtBulkMemory", }; diff --git a/polkadot/primitives/src/v6/mod.rs b/polkadot/primitives/src/v6/mod.rs index 9371b3db406b3..013a3a781e67b 100644 --- a/polkadot/primitives/src/v6/mod.rs +++ b/polkadot/primitives/src/v6/mod.rs @@ -1794,13 +1794,15 @@ pub enum PvfPrepTimeoutKind { Lenient, } -/// Type discriminator for PVF execution timeouts +/// Type discriminator for PVF execution timeouts and retries #[derive(Encode, Decode, TypeInfo, Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] -pub enum PvfExecTimeoutKind { +pub enum PvfExecKind { /// The amount of time to spend on execution during backing. + /// Retry is disabled to reduce the chance of nondeterministic blocks getting backed and honest backers getting slashed. Backing, /// The amount of time to spend on execution during approval or disputes. + /// With retry enabled. /// /// This should be much longer than the backing execution timeout to ensure that in the /// absence of extremely large disparities between hardware, blocks that pass backing are diff --git a/polkadot/runtime/parachains/src/configuration/benchmarking.rs b/polkadot/runtime/parachains/src/configuration/benchmarking.rs index d9d11ab56e496..09648f3d54235 100644 --- a/polkadot/runtime/parachains/src/configuration/benchmarking.rs +++ b/polkadot/runtime/parachains/src/configuration/benchmarking.rs @@ -17,7 +17,7 @@ use crate::configuration::*; use frame_benchmarking::{benchmarks, BenchmarkError, BenchmarkResult}; use frame_system::RawOrigin; -use primitives::{ExecutorParam, ExecutorParams, PvfExecTimeoutKind, PvfPrepTimeoutKind}; +use primitives::{ExecutorParam, ExecutorParams, PvfExecKind, PvfPrepTimeoutKind}; use sp_runtime::traits::One; benchmarks! { @@ -43,8 +43,8 @@ benchmarks! { ExecutorParam::PrecheckingMaxMemory(2 * 1024 * 1024 * 1024), ExecutorParam::PvfPrepTimeout(PvfPrepTimeoutKind::Precheck, 60_000), ExecutorParam::PvfPrepTimeout(PvfPrepTimeoutKind::Lenient, 360_000), - ExecutorParam::PvfExecTimeout(PvfExecTimeoutKind::Backing, 2_000), - ExecutorParam::PvfExecTimeout(PvfExecTimeoutKind::Approval, 12_000), + ExecutorParam::PvfExecTimeout(PvfExecKind::Backing, 2_000), + ExecutorParam::PvfExecTimeout(PvfExecKind::Approval, 12_000), ][..])) set_config_with_perbill {}: set_on_demand_fee_variability(RawOrigin::Root, Perbill::from_percent(100)) From 186b5d91674122bb0025980b71989e60601a487b Mon Sep 17 00:00:00 2001 From: Joao pedro Santos Date: Mon, 6 Nov 2023 14:09:09 -0300 Subject: [PATCH 02/15] rename PvfExecTimeoutKind to PvfExecKind on executor params --- polkadot/primitives/src/v6/executor_params.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index 782445fa66fb2..38b8a2aad87e6 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -45,7 +45,7 @@ pub const PRECHECK_MEM_MAX_LO: u64 = 256 * 1024 * 1024; pub const PRECHECK_MEM_MAX_HI: u64 = 16 * 1024 * 1024 * 1024; // Default PVF timeouts. Must never be changed! Use executor environment parameters to adjust them. -// See also `PvfPrepTimeoutKind` and `PvfExecTimeoutKind` docs. +// See also `PvfPrepTimeoutKind` and `PvfExecKind` docs. /// Default PVF preparation timeout for prechecking requests. pub const DEFAULT_PRECHECK_PREPARATION_TIMEOUT: Duration = Duration::from_secs(60); @@ -311,15 +311,15 @@ impl ExecutorParams { } if let (Some(backing), Some(approval)) = ( - seen.get("PvfExecTimeoutKind::Backing") + seen.get("PvfExecKind::Backing") .or(Some(&DEFAULT_BACKING_EXECUTION_TIMEOUT_MS)), - seen.get("PvfExecTimeoutKind::Approval") + seen.get("PvfExecKind::Approval") .or(Some(&DEFAULT_APPROVAL_EXECUTION_TIMEOUT_MS)), ) { if *backing >= *approval { return Err(IncompatibleValues( - "PvfExecTimeoutKind::Backing", - "PvfExecTimeoutKind::Approval", + "PvfExecKind::Backing", + "PvfExecKind::Approval", )) } } From 11ed7251d34c15d1308ddd371b1029c6f44539d4 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Mon, 6 Nov 2023 18:23:39 +0000 Subject: [PATCH 03/15] ".git/.scripts/commands/fmt/fmt.sh" --- .../node/core/candidate-validation/src/lib.rs | 31 ++- .../core/candidate-validation/src/tests.rs | 192 +++++++++--------- .../src/participation/mod.rs | 16 +- polkadot/node/overseer/src/tests.rs | 24 +-- polkadot/primitives/src/lib.rs | 50 ++--- polkadot/primitives/src/v6/executor_params.rs | 8 +- polkadot/primitives/src/v6/mod.rs | 3 +- 7 files changed, 157 insertions(+), 167 deletions(-) diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs index 9e2b1fa3453cc..96f56ec858d32 100644 --- a/polkadot/node/core/candidate-validation/src/lib.rs +++ b/polkadot/node/core/candidate-validation/src/lib.rs @@ -45,8 +45,8 @@ use polkadot_parachain_primitives::primitives::{ }; use polkadot_primitives::{ executor_params::{ - DEFAULT_APPROVAL_EXECUTION_TIMEOUT, DEFAULT_BACKING_EXECUTION_TIMEOUT, - DEFAULT_LENIENT_PREPARATION_TIMEOUT, DEFAULT_PRECHECK_PREPARATION_TIMEOUT, self, + self, DEFAULT_APPROVAL_EXECUTION_TIMEOUT, DEFAULT_BACKING_EXECUTION_TIMEOUT, + DEFAULT_LENIENT_PREPARATION_TIMEOUT, DEFAULT_PRECHECK_PREPARATION_TIMEOUT, }, CandidateCommitments, CandidateDescriptor, CandidateReceipt, ExecutorParams, Hash, OccupiedCoreAssumption, PersistedValidationData, PvfExecKind, PvfPrepTimeoutKind, @@ -624,25 +624,18 @@ async fn validate_candidate_exhaustive( PrepareJobKind::Compilation, ); - validation_backend - .validate_candidate( - pvf, - exec_timeout, - params.encode(), - ) - .await + validation_backend.validate_candidate(pvf, exec_timeout, params.encode()).await }, - PvfExecKind::Approval => { + PvfExecKind::Approval => validation_backend - .validate_candidate_with_retry( - raw_validation_code.to_vec(), - pvf_exec_timeout(&executor_params, exec_timeout_kind), - exec_timeout_kind, - params, - executor_params, - ) - .await - } + .validate_candidate_with_retry( + raw_validation_code.to_vec(), + pvf_exec_timeout(&executor_params, exec_timeout_kind), + exec_timeout_kind, + params, + executor_params, + ) + .await, }; if let Err(ref error) = result { diff --git a/polkadot/node/core/candidate-validation/src/tests.rs b/polkadot/node/core/candidate-validation/src/tests.rs index 327af62222b29..8a207723439cf 100644 --- a/polkadot/node/core/candidate-validation/src/tests.rs +++ b/polkadot/node/core/candidate-validation/src/tests.rs @@ -430,14 +430,14 @@ fn candidate_validation_ok_is_ok() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data.clone(), - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data.clone(), + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -480,16 +480,16 @@ fn candidate_validation_bad_return_is_invalid() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Err( + MockValidateCandidateBackend::with_hardcoded_result(Err( ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout), )), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -545,17 +545,17 @@ fn candidate_validation_one_ambiguous_error_is_valid() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![ + MockValidateCandidateBackend::with_hardcoded_result_list(vec![ Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), Ok(validation_result), ]), - validation_data.clone(), - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + validation_data.clone(), + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -598,17 +598,17 @@ fn candidate_validation_multiple_ambiguous_errors_is_invalid() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![ + MockValidateCandidateBackend::with_hardcoded_result_list(vec![ Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), ]), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -645,20 +645,20 @@ fn candidate_validation_retry_internal_errors() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![ + MockValidateCandidateBackend::with_hardcoded_result_list(vec![ Err(InternalValidationError::HostCommunication("foo".into()).into()), // Throw an AWD error, we should still retry again. Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), // Throw another internal error. Err(InternalValidationError::HostCommunication("bar".into()).into()), ]), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Err(ValidationFailed(s)) if s.contains("bar")); @@ -694,20 +694,20 @@ fn candidate_validation_retry_panic_errors() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![ + MockValidateCandidateBackend::with_hardcoded_result_list(vec![ Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("foo".into()))), // Throw an AWD error, we should still retry again. Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), // Throw another panic error. Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("bar".into()))), ]), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(s))) if s == "bar".to_string()); @@ -742,16 +742,16 @@ fn candidate_validation_timeout_is_internal_error() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Err( + MockValidateCandidateBackend::with_hardcoded_result(Err( ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout), )), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::Timeout))); @@ -789,14 +789,14 @@ fn candidate_validation_commitment_hash_mismatch_is_invalid() { }; let result = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -836,16 +836,16 @@ fn candidate_validation_code_mismatch_is_invalid() { let (_ctx, _ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Err( + MockValidateCandidateBackend::with_hardcoded_result(Err( ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout), )), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -895,14 +895,14 @@ fn compressed_code_works() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Ok(ValidationResult::Valid(_, _))); @@ -946,14 +946,14 @@ fn code_decompression_failure_is_error() { let (_ctx, _ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Err(_)); @@ -998,14 +998,14 @@ fn pov_decompression_failure_is_invalid() { let (_ctx, _ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::PoVDecompressionFailure))); diff --git a/polkadot/node/core/dispute-coordinator/src/participation/mod.rs b/polkadot/node/core/dispute-coordinator/src/participation/mod.rs index 1187a098ae44f..2babd29116de2 100644 --- a/polkadot/node/core/dispute-coordinator/src/participation/mod.rs +++ b/polkadot/node/core/dispute-coordinator/src/participation/mod.rs @@ -32,7 +32,7 @@ use polkadot_node_subsystem::{ }; use polkadot_node_subsystem_util::runtime::get_validation_code_by_hash; use polkadot_primitives::{ - BlockNumber, CandidateHash, CandidateReceipt, Hash, PvfExecKind, SessionIndex, + BlockNumber, CandidateHash, CandidateReceipt, Hash, PvfExecKind, SessionIndex, }; use crate::LOG_TARGET; @@ -381,13 +381,13 @@ async fn participate( let (validation_tx, validation_rx) = oneshot::channel(); sender .send_message(CandidateValidationMessage::ValidateFromExhaustive( - available_data.validation_data, - validation_code, - req.candidate_receipt().clone(), - available_data.pov, - req.executor_params(), - PvfExecKind::Approval, - validation_tx, + available_data.validation_data, + validation_code, + req.candidate_receipt().clone(), + available_data.pov, + req.executor_params(), + PvfExecKind::Approval, + validation_tx, )) .await; diff --git a/polkadot/node/overseer/src/tests.rs b/polkadot/node/overseer/src/tests.rs index c813839fb5563..3ac23a6b32b66 100644 --- a/polkadot/node/overseer/src/tests.rs +++ b/polkadot/node/overseer/src/tests.rs @@ -29,8 +29,8 @@ use polkadot_node_subsystem_types::messages::{ NetworkBridgeEvent, ReportPeerMessage, RuntimeApiRequest, }; use polkadot_primitives::{ - CandidateHash, CandidateReceipt, CollatorPair, Id as ParaId, InvalidDisputeStatementKind, - PvfExecKind, SessionIndex, ValidDisputeStatementKind, ValidatorIndex, + CandidateHash, CandidateReceipt, CollatorPair, Id as ParaId, InvalidDisputeStatementKind, + PvfExecKind, SessionIndex, ValidDisputeStatementKind, ValidatorIndex, }; use crate::{ @@ -103,11 +103,11 @@ where let (tx, _) = oneshot::channel(); ctx.send_message(CandidateValidationMessage::ValidateFromChainState( - candidate_receipt, - PoV { block_data: BlockData(Vec::new()) }.into(), - Default::default(), - PvfExecKind::Backing, - tx, + candidate_receipt, + PoV { block_data: BlockData(Vec::new()) }.into(), + Default::default(), + PvfExecKind::Backing, + tx, )) .await; c += 1; @@ -801,11 +801,11 @@ fn test_candidate_validation_msg() -> CandidateValidationMessage { }; CandidateValidationMessage::ValidateFromChainState( - candidate_receipt, - pov, - Default::default(), - PvfExecKind::Backing, - sender, + candidate_receipt, + pov, + Default::default(), + PvfExecKind::Backing, + sender, ) } diff --git a/polkadot/primitives/src/lib.rs b/polkadot/primitives/src/lib.rs index fe7613e049956..bab04cb2aa4c2 100644 --- a/polkadot/primitives/src/lib.rs +++ b/polkadot/primitives/src/lib.rs @@ -34,31 +34,31 @@ pub mod runtime_api; // Current primitives not requiring versioning are exported here. // Primitives requiring versioning must not be exported and must be referred by an exact version. pub use v6::{ - async_backing, byzantine_threshold, check_candidate_backing, collator_signature_payload, - effective_minimum_backing_votes, executor_params, metric_definitions, slashing, - supermajority_threshold, well_known_keys, AbridgedHostConfiguration, AbridgedHrmpChannel, - AccountId, AccountIndex, AccountPublic, ApprovalVote, AssignmentId, AsyncBackingParams, - AuthorityDiscoveryId, AvailabilityBitfield, BackedCandidate, Balance, BlakeTwo256, Block, - BlockId, BlockNumber, CandidateCommitments, CandidateDescriptor, CandidateEvent, CandidateHash, - CandidateIndex, CandidateReceipt, CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, - CollatorId, CollatorSignature, CommittedCandidateReceipt, CompactStatement, ConsensusLog, - CoreIndex, CoreState, DisputeState, DisputeStatement, DisputeStatementSet, DownwardMessage, - EncodeAs, ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, - ExplicitDisputeStatement, GroupIndex, GroupRotationInfo, Hash, HashT, HeadData, Header, - HorizontalMessages, HrmpChannelId, Id, InboundDownwardMessage, InboundHrmpMessage, IndexedVec, - InherentData, InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, Nonce, - OccupiedCore, OccupiedCoreAssumption, OutboundHrmpMessage, ParathreadClaim, ParathreadEntry, - PersistedValidationData, PvfCheckStatement, PvfExecKind, PvfPrepTimeoutKind, - RuntimeMetricLabel, RuntimeMetricLabelValue, RuntimeMetricLabelValues, RuntimeMetricLabels, - RuntimeMetricOp, RuntimeMetricUpdate, ScheduledCore, ScrapedOnChainVotes, SessionIndex, - SessionInfo, Signature, Signed, SignedAvailabilityBitfield, SignedAvailabilityBitfields, - SignedStatement, SigningContext, Slot, UncheckedSigned, UncheckedSignedAvailabilityBitfield, - UncheckedSignedAvailabilityBitfields, UncheckedSignedStatement, UpgradeGoAhead, - UpgradeRestriction, UpwardMessage, ValidDisputeStatementKind, ValidationCode, - ValidationCodeHash, ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation, - ValidityError, ASSIGNMENT_KEY_TYPE_ID, LEGACY_MIN_BACKING_VOTES, LOWEST_PUBLIC_ID, - MAX_CODE_SIZE, MAX_HEAD_DATA_SIZE, MAX_POV_SIZE, ON_DEMAND_DEFAULT_QUEUE_MAX_SIZE, - PARACHAINS_INHERENT_IDENTIFIER, PARACHAIN_KEY_TYPE_ID, + async_backing, byzantine_threshold, check_candidate_backing, collator_signature_payload, + effective_minimum_backing_votes, executor_params, metric_definitions, slashing, + supermajority_threshold, well_known_keys, AbridgedHostConfiguration, AbridgedHrmpChannel, + AccountId, AccountIndex, AccountPublic, ApprovalVote, AssignmentId, AsyncBackingParams, + AuthorityDiscoveryId, AvailabilityBitfield, BackedCandidate, Balance, BlakeTwo256, Block, + BlockId, BlockNumber, CandidateCommitments, CandidateDescriptor, CandidateEvent, CandidateHash, + CandidateIndex, CandidateReceipt, CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, + CollatorId, CollatorSignature, CommittedCandidateReceipt, CompactStatement, ConsensusLog, + CoreIndex, CoreState, DisputeState, DisputeStatement, DisputeStatementSet, DownwardMessage, + EncodeAs, ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, + ExplicitDisputeStatement, GroupIndex, GroupRotationInfo, Hash, HashT, HeadData, Header, + HorizontalMessages, HrmpChannelId, Id, InboundDownwardMessage, InboundHrmpMessage, IndexedVec, + InherentData, InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, Nonce, + OccupiedCore, OccupiedCoreAssumption, OutboundHrmpMessage, ParathreadClaim, ParathreadEntry, + PersistedValidationData, PvfCheckStatement, PvfExecKind, PvfPrepTimeoutKind, + RuntimeMetricLabel, RuntimeMetricLabelValue, RuntimeMetricLabelValues, RuntimeMetricLabels, + RuntimeMetricOp, RuntimeMetricUpdate, ScheduledCore, ScrapedOnChainVotes, SessionIndex, + SessionInfo, Signature, Signed, SignedAvailabilityBitfield, SignedAvailabilityBitfields, + SignedStatement, SigningContext, Slot, UncheckedSigned, UncheckedSignedAvailabilityBitfield, + UncheckedSignedAvailabilityBitfields, UncheckedSignedStatement, UpgradeGoAhead, + UpgradeRestriction, UpwardMessage, ValidDisputeStatementKind, ValidationCode, + ValidationCodeHash, ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation, + ValidityError, ASSIGNMENT_KEY_TYPE_ID, LEGACY_MIN_BACKING_VOTES, LOWEST_PUBLIC_ID, + MAX_CODE_SIZE, MAX_HEAD_DATA_SIZE, MAX_POV_SIZE, ON_DEMAND_DEFAULT_QUEUE_MAX_SIZE, + PARACHAINS_INHERENT_IDENTIFIER, PARACHAIN_KEY_TYPE_ID, }; #[cfg(feature = "std")] diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index 38b8a2aad87e6..89490a8dd800d 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -311,16 +311,12 @@ impl ExecutorParams { } if let (Some(backing), Some(approval)) = ( - seen.get("PvfExecKind::Backing") - .or(Some(&DEFAULT_BACKING_EXECUTION_TIMEOUT_MS)), + seen.get("PvfExecKind::Backing").or(Some(&DEFAULT_BACKING_EXECUTION_TIMEOUT_MS)), seen.get("PvfExecKind::Approval") .or(Some(&DEFAULT_APPROVAL_EXECUTION_TIMEOUT_MS)), ) { if *backing >= *approval { - return Err(IncompatibleValues( - "PvfExecKind::Backing", - "PvfExecKind::Approval", - )) + return Err(IncompatibleValues("PvfExecKind::Backing", "PvfExecKind::Approval")) } } diff --git a/polkadot/primitives/src/v6/mod.rs b/polkadot/primitives/src/v6/mod.rs index 013a3a781e67b..744aa5133d2fa 100644 --- a/polkadot/primitives/src/v6/mod.rs +++ b/polkadot/primitives/src/v6/mod.rs @@ -1798,7 +1798,8 @@ pub enum PvfPrepTimeoutKind { #[derive(Encode, Decode, TypeInfo, Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] pub enum PvfExecKind { /// The amount of time to spend on execution during backing. - /// Retry is disabled to reduce the chance of nondeterministic blocks getting backed and honest backers getting slashed. + /// Retry is disabled to reduce the chance of nondeterministic blocks getting backed and honest + /// backers getting slashed. Backing, /// The amount of time to spend on execution during approval or disputes. From ab5eb3f6544795ea4526ac67506ffbcde8621454 Mon Sep 17 00:00:00 2001 From: Joao pedro Santos Date: Tue, 7 Nov 2023 10:25:43 -0300 Subject: [PATCH 04/15] rename PvfPrepKind to PvfPrepKind, remove exec_kind from validate_candidate_with_retry and fix tests --- .../node/core/candidate-validation/src/lib.rs | 35 ++++---- .../core/candidate-validation/src/tests.rs | 82 +------------------ polkadot/node/subsystem-types/src/messages.rs | 4 +- polkadot/primitives/src/lib.rs | 2 +- polkadot/primitives/src/v6/executor_params.rs | 20 ++--- polkadot/primitives/src/v6/mod.rs | 2 +- .../src/configuration/benchmarking.rs | 6 +- 7 files changed, 36 insertions(+), 115 deletions(-) diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs index 9e2b1fa3453cc..cb0694c2f056e 100644 --- a/polkadot/node/core/candidate-validation/src/lib.rs +++ b/polkadot/node/core/candidate-validation/src/lib.rs @@ -49,7 +49,7 @@ use polkadot_primitives::{ DEFAULT_LENIENT_PREPARATION_TIMEOUT, DEFAULT_PRECHECK_PREPARATION_TIMEOUT, self, }, CandidateCommitments, CandidateDescriptor, CandidateReceipt, ExecutorParams, Hash, - OccupiedCoreAssumption, PersistedValidationData, PvfExecKind, PvfPrepTimeoutKind, + OccupiedCoreAssumption, PersistedValidationData, PvfExecKind, PvfPrepKind, ValidationCode, ValidationCodeHash, }; @@ -354,7 +354,7 @@ where return PreCheckOutcome::Invalid }; - let timeout = pvf_prep_timeout(&executor_params, PvfPrepTimeoutKind::Precheck); + let timeout = pvf_prep_timeout(&executor_params, PvfPrepKind::Precheck); let pvf = match sp_maybe_compressed_blob::decompress( &validation_code.0, @@ -498,7 +498,7 @@ async fn validate_from_chain_state( candidate_receipt: CandidateReceipt, pov: Arc, executor_params: ExecutorParams, - exec_timeout_kind: PvfExecKind, + exec_kind: PvfExecKind, metrics: &Metrics, ) -> Result where @@ -518,7 +518,7 @@ where candidate_receipt.clone(), pov, executor_params, - exec_timeout_kind, + exec_kind, metrics, ) .await; @@ -554,7 +554,7 @@ async fn validate_candidate_exhaustive( candidate_receipt: CandidateReceipt, pov: Arc, executor_params: ExecutorParams, - exec_timeout_kind: PvfExecKind, + exec_kind: PvfExecKind, metrics: &Metrics, ) -> Result { let _timer = metrics.time_validate_candidate_exhaustive(); @@ -613,10 +613,10 @@ async fn validate_candidate_exhaustive( relay_parent_storage_root: persisted_validation_data.relay_parent_storage_root, }; - let result = match exec_timeout_kind { + let result = match exec_kind { PvfExecKind::Backing => { - let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepTimeoutKind::Lenient); - let exec_timeout = pvf_exec_timeout(&executor_params, exec_timeout_kind); + let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepKind::Lenient); + let exec_timeout = pvf_exec_timeout(&executor_params, exec_kind); let pvf = PvfPrepData::from_code( raw_validation_code.to_vec(), executor_params, @@ -636,8 +636,7 @@ async fn validate_candidate_exhaustive( validation_backend .validate_candidate_with_retry( raw_validation_code.to_vec(), - pvf_exec_timeout(&executor_params, exec_timeout_kind), - exec_timeout_kind, + pvf_exec_timeout(&executor_params, exec_kind), params, executor_params, ) @@ -723,7 +722,7 @@ trait ValidationBackend { encoded_params: Vec, ) -> Result; - /// Tries executing a PVF. Will retry once if an error is encountered that may have been + /// Tries executing a PVF for the approval subsystem. Will retry once if an error is encountered that may have been /// transient. /// /// NOTE: Should retry only on errors that are a result of execution itself, and not of @@ -732,11 +731,10 @@ trait ValidationBackend { &mut self, raw_validation_code: Vec, exec_timeout: Duration, - exec_timeout_kind: PvfExecKind, params: ValidationParams, executor_params: ExecutorParams, ) -> Result { - let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepTimeoutKind::Lenient); + let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepKind::Lenient); // Construct the PVF a single time, since it is an expensive operation. Cloning it is cheap. let pvf = PvfPrepData::from_code( raw_validation_code, @@ -754,10 +752,7 @@ trait ValidationBackend { return validation_result } - let retry_delay = match exec_timeout_kind { - PvfExecKind::Backing => PVF_BACKING_EXECUTION_RETRY_DELAY, - PvfExecKind::Approval => PVF_APPROVAL_EXECUTION_RETRY_DELAY, - }; + let retry_delay = PVF_APPROVAL_EXECUTION_RETRY_DELAY; // Allow limited retries for each kind of error. let mut num_internal_retries_left = 1; @@ -880,13 +875,13 @@ fn perform_basic_checks( Ok(()) } -fn pvf_prep_timeout(executor_params: &ExecutorParams, kind: PvfPrepTimeoutKind) -> Duration { +fn pvf_prep_timeout(executor_params: &ExecutorParams, kind: PvfPrepKind) -> Duration { if let Some(timeout) = executor_params.pvf_prep_timeout(kind) { return timeout } match kind { - PvfPrepTimeoutKind::Precheck => DEFAULT_PRECHECK_PREPARATION_TIMEOUT, - PvfPrepTimeoutKind::Lenient => DEFAULT_LENIENT_PREPARATION_TIMEOUT, + PvfPrepKind::Precheck => DEFAULT_PRECHECK_PREPARATION_TIMEOUT, + PvfPrepKind::Lenient => DEFAULT_LENIENT_PREPARATION_TIMEOUT, } } diff --git a/polkadot/node/core/candidate-validation/src/tests.rs b/polkadot/node/core/candidate-validation/src/tests.rs index 327af62222b29..c78a6e9bd7bf1 100644 --- a/polkadot/node/core/candidate-validation/src/tests.rs +++ b/polkadot/node/core/candidate-validation/src/tests.rs @@ -496,79 +496,6 @@ fn candidate_validation_bad_return_is_invalid() { assert_matches!(v, ValidationResult::Invalid(InvalidCandidate::Timeout)); } -// Test that we vote valid if we get `AmbiguousWorkerDeath`, retry, and then succeed. -#[test] -fn candidate_validation_one_ambiguous_error_is_valid() { - let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; - - let pov = PoV { block_data: BlockData(vec![1; 32]) }; - let head_data = HeadData(vec![1, 1, 1]); - let validation_code = ValidationCode(vec![2; 16]); - - let descriptor = make_valid_candidate_descriptor( - ParaId::from(1_u32), - dummy_hash(), - validation_data.hash(), - pov.hash(), - validation_code.hash(), - head_data.hash(), - dummy_hash(), - Sr25519Keyring::Alice, - ); - - let check = perform_basic_checks( - &descriptor, - validation_data.max_pov_size, - &pov, - &validation_code.hash(), - ); - assert!(check.is_ok()); - - let validation_result = WasmValidationResult { - head_data, - new_validation_code: Some(vec![2, 2, 2].into()), - upward_messages: Default::default(), - horizontal_messages: Default::default(), - processed_downward_messages: 0, - hrmp_watermark: 0, - }; - - let commitments = CandidateCommitments { - head_data: validation_result.head_data.clone(), - upward_messages: validation_result.upward_messages.clone(), - horizontal_messages: validation_result.horizontal_messages.clone(), - new_validation_code: validation_result.new_validation_code.clone(), - processed_downward_messages: validation_result.processed_downward_messages, - hrmp_watermark: validation_result.hrmp_watermark, - }; - - let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; - - let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![ - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), - Ok(validation_result), - ]), - validation_data.clone(), - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), - )) - .unwrap(); - - assert_matches!(v, ValidationResult::Valid(outputs, used_validation_data) => { - assert_eq!(outputs.head_data, HeadData(vec![1, 1, 1])); - assert_eq!(outputs.upward_messages, Vec::::new()); - assert_eq!(outputs.horizontal_messages, Vec::new()); - assert_eq!(outputs.new_validation_code, Some(vec![2, 2, 2].into())); - assert_eq!(outputs.hrmp_watermark, 0); - assert_eq!(used_validation_data, validation_data); - }); -} - #[test] fn candidate_validation_multiple_ambiguous_errors_is_invalid() { let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; @@ -600,7 +527,6 @@ fn candidate_validation_multiple_ambiguous_errors_is_invalid() { let v = executor::block_on(validate_candidate_exhaustive( MockValidateCandidateBackend::with_hardcoded_result_list(vec![ Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), ]), validation_data, validation_code, @@ -617,7 +543,7 @@ fn candidate_validation_multiple_ambiguous_errors_is_invalid() { // Test that we retry on internal errors. #[test] -fn candidate_validation_retry_internal_errors() { +fn candidate_validation_dont_retry_internal_errors() { let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; let pov = PoV { block_data: BlockData(vec![1; 32]) }; @@ -661,12 +587,12 @@ fn candidate_validation_retry_internal_errors() { &Default::default(), )); - assert_matches!(v, Err(ValidationFailed(s)) if s.contains("bar")); + assert_matches!(v, Err(ValidationFailed(s)) if s.contains("foo")); } // Test that we retry on panic errors. #[test] -fn candidate_validation_retry_panic_errors() { +fn candidate_validation_dont_retry_panic_errors() { let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; let pov = PoV { block_data: BlockData(vec![1; 32]) }; @@ -710,7 +636,7 @@ fn candidate_validation_retry_panic_errors() { &Default::default(), )); - assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(s))) if s == "bar".to_string()); + assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(s))) if s == "foo".to_string()); } #[test] diff --git a/polkadot/node/subsystem-types/src/messages.rs b/polkadot/node/subsystem-types/src/messages.rs index 53a7b98170c29..84bee32126f89 100644 --- a/polkadot/node/subsystem-types/src/messages.rs +++ b/polkadot/node/subsystem-types/src/messages.rs @@ -147,7 +147,7 @@ pub enum CandidateValidationMessage { CandidateReceipt, Arc, ExecutorParams, - /// Execution timeout and retry + /// Execution kind, used for timeouts and retries PvfExecKind, oneshot::Sender>, ), @@ -166,7 +166,7 @@ pub enum CandidateValidationMessage { CandidateReceipt, Arc, ExecutorParams, - /// Execution timeout and retry + /// Execution kind, used for timeouts and retries PvfExecKind, oneshot::Sender>, ), diff --git a/polkadot/primitives/src/lib.rs b/polkadot/primitives/src/lib.rs index fe7613e049956..33ab90fd44d71 100644 --- a/polkadot/primitives/src/lib.rs +++ b/polkadot/primitives/src/lib.rs @@ -48,7 +48,7 @@ pub use v6::{ HorizontalMessages, HrmpChannelId, Id, InboundDownwardMessage, InboundHrmpMessage, IndexedVec, InherentData, InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, Nonce, OccupiedCore, OccupiedCoreAssumption, OutboundHrmpMessage, ParathreadClaim, ParathreadEntry, - PersistedValidationData, PvfCheckStatement, PvfExecKind, PvfPrepTimeoutKind, + PersistedValidationData, PvfCheckStatement, PvfExecKind, PvfPrepKind, RuntimeMetricLabel, RuntimeMetricLabelValue, RuntimeMetricLabelValues, RuntimeMetricLabels, RuntimeMetricOp, RuntimeMetricUpdate, ScheduledCore, ScrapedOnChainVotes, SessionIndex, SessionInfo, Signature, Signed, SignedAvailabilityBitfield, SignedAvailabilityBitfields, diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index 38b8a2aad87e6..a292e536aa4a3 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -21,7 +21,7 @@ //! by the first element of the vector). Decoding to a usable semantics structure is //! done in `polkadot-node-core-pvf`. -use crate::{BlakeTwo256, HashT as _, PvfExecKind, PvfPrepTimeoutKind}; +use crate::{BlakeTwo256, HashT as _, PvfExecKind, PvfPrepKind}; use parity_scale_codec::{Decode, Encode}; use polkadot_core_primitives::Hash; use scale_info::TypeInfo; @@ -45,7 +45,7 @@ pub const PRECHECK_MEM_MAX_LO: u64 = 256 * 1024 * 1024; pub const PRECHECK_MEM_MAX_HI: u64 = 16 * 1024 * 1024 * 1024; // Default PVF timeouts. Must never be changed! Use executor environment parameters to adjust them. -// See also `PvfPrepTimeoutKind` and `PvfExecKind` docs. +// See also `PvfPrepKind` and `PvfExecKind` docs. /// Default PVF preparation timeout for prechecking requests. pub const DEFAULT_PRECHECK_PREPARATION_TIMEOUT: Duration = Duration::from_secs(60); @@ -99,7 +99,7 @@ pub enum ExecutorParam { /// Always ensure that `precheck_timeout` < `lenient_timeout`. /// When absent, the default values will be used. #[codec(index = 5)] - PvfPrepTimeout(PvfPrepTimeoutKind, u64), + PvfPrepTimeout(PvfPrepKind, u64), /// PVF execution timeouts, in millisecond. /// Always ensure that `backing_timeout` < `approval_timeout`. /// When absent, the default values will be used. @@ -174,7 +174,7 @@ impl ExecutorParams { } /// Returns a PVF preparation timeout, if any - pub fn pvf_prep_timeout(&self, kind: PvfPrepTimeoutKind) -> Option { + pub fn pvf_prep_timeout(&self, kind: PvfPrepKind) -> Option { for param in &self.0 { if let ExecutorParam::PvfPrepTimeout(k, timeout) = param { if kind == *k { @@ -242,8 +242,8 @@ impl ExecutorParams { StackNativeMax(_) => "StackNativeMax", PrecheckingMaxMemory(_) => "PrecheckingMaxMemory", PvfPrepTimeout(kind, _) => match kind { - PvfPrepTimeoutKind::Precheck => "PvfPrepTimeoutKind::Precheck", - PvfPrepTimeoutKind::Lenient => "PvfPrepTimeoutKind::Lenient", + PvfPrepKind::Precheck => "PvfPrepKind::Precheck", + PvfPrepKind::Lenient => "PvfPrepKind::Lenient", }, PvfExecTimeout(kind, _) => match kind { PvfExecKind::Backing => "PvfExecKind::Backing", @@ -297,15 +297,15 @@ impl ExecutorParams { } if let (Some(precheck), Some(lenient)) = ( - seen.get("PvfPrepTimeoutKind::Precheck") + seen.get("PvfPrepKind::Precheck") .or(Some(&DEFAULT_PRECHECK_PREPARATION_TIMEOUT_MS)), - seen.get("PvfPrepTimeoutKind::Lenient") + seen.get("PvfPrepKind::Lenient") .or(Some(&DEFAULT_LENIENT_PREPARATION_TIMEOUT_MS)), ) { if *precheck >= *lenient { return Err(IncompatibleValues( - "PvfPrepTimeoutKind::Precheck", - "PvfPrepTimeoutKind::Lenient", + "PvfPrepKind::Precheck", + "PvfPrepKind::Lenient", )) } } diff --git a/polkadot/primitives/src/v6/mod.rs b/polkadot/primitives/src/v6/mod.rs index 013a3a781e67b..7c7f705510eaf 100644 --- a/polkadot/primitives/src/v6/mod.rs +++ b/polkadot/primitives/src/v6/mod.rs @@ -1783,7 +1783,7 @@ impl WellKnownKey { /// Type discriminator for PVF preparation timeouts #[derive(Encode, Decode, TypeInfo, Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] -pub enum PvfPrepTimeoutKind { +pub enum PvfPrepKind { /// For prechecking requests, the time period after which the preparation worker is considered /// unresponsive and will be killed. Precheck, diff --git a/polkadot/runtime/parachains/src/configuration/benchmarking.rs b/polkadot/runtime/parachains/src/configuration/benchmarking.rs index 09648f3d54235..666741850f118 100644 --- a/polkadot/runtime/parachains/src/configuration/benchmarking.rs +++ b/polkadot/runtime/parachains/src/configuration/benchmarking.rs @@ -17,7 +17,7 @@ use crate::configuration::*; use frame_benchmarking::{benchmarks, BenchmarkError, BenchmarkResult}; use frame_system::RawOrigin; -use primitives::{ExecutorParam, ExecutorParams, PvfExecKind, PvfPrepTimeoutKind}; +use primitives::{ExecutorParam, ExecutorParams, PvfExecKind, PvfPrepKind}; use sp_runtime::traits::One; benchmarks! { @@ -41,8 +41,8 @@ benchmarks! { ExecutorParam::StackNativeMax(256 * 1024 * 1024), ExecutorParam::WasmExtBulkMemory, ExecutorParam::PrecheckingMaxMemory(2 * 1024 * 1024 * 1024), - ExecutorParam::PvfPrepTimeout(PvfPrepTimeoutKind::Precheck, 60_000), - ExecutorParam::PvfPrepTimeout(PvfPrepTimeoutKind::Lenient, 360_000), + ExecutorParam::PvfPrepTimeout(PvfPrepKind::Precheck, 60_000), + ExecutorParam::PvfPrepTimeout(PvfPrepKind::Lenient, 360_000), ExecutorParam::PvfExecTimeout(PvfExecKind::Backing, 2_000), ExecutorParam::PvfExecTimeout(PvfExecKind::Approval, 12_000), ][..])) From 364de31b532bc278452264b61594060510b95f67 Mon Sep 17 00:00:00 2001 From: Joao pedro Santos Date: Tue, 7 Nov 2023 11:08:02 -0300 Subject: [PATCH 05/15] close delimiter --- polkadot/node/core/candidate-validation/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs index f5c7eef46ffdc..6820fe1e3d5d6 100644 --- a/polkadot/node/core/candidate-validation/src/lib.rs +++ b/polkadot/node/core/candidate-validation/src/lib.rs @@ -626,7 +626,7 @@ async fn validate_candidate_exhaustive( validation_backend.validate_candidate(pvf, exec_timeout, params.encode()).await }, - PvfExecKind::Approval => + PvfExecKind::Approval => { validation_backend .validate_candidate_with_retry( raw_validation_code.to_vec(), From 056146a9bda1a96b46f0624480203ddd009da724 Mon Sep 17 00:00:00 2001 From: Joao pedro Santos Date: Wed, 8 Nov 2023 07:08:28 -0300 Subject: [PATCH 06/15] refactor candidate-validation tests --- .../node/core/candidate-validation/src/lib.rs | 37 +-- .../core/candidate-validation/src/tests.rs | 298 +++++++++++------- 2 files changed, 207 insertions(+), 128 deletions(-) diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs index 6820fe1e3d5d6..8cdf506b51991 100644 --- a/polkadot/node/core/candidate-validation/src/lib.rs +++ b/polkadot/node/core/candidate-validation/src/lib.rs @@ -46,11 +46,11 @@ use polkadot_parachain_primitives::primitives::{ use polkadot_primitives::{ executor_params::{ DEFAULT_APPROVAL_EXECUTION_TIMEOUT, DEFAULT_BACKING_EXECUTION_TIMEOUT, - DEFAULT_LENIENT_PREPARATION_TIMEOUT, DEFAULT_PRECHECK_PREPARATION_TIMEOUT, self, + DEFAULT_LENIENT_PREPARATION_TIMEOUT, DEFAULT_PRECHECK_PREPARATION_TIMEOUT, }, CandidateCommitments, CandidateDescriptor, CandidateReceipt, ExecutorParams, Hash, - OccupiedCoreAssumption, PersistedValidationData, PvfExecKind, PvfPrepKind, - ValidationCode, ValidationCodeHash, + OccupiedCoreAssumption, PersistedValidationData, PvfExecKind, PvfPrepKind, ValidationCode, + ValidationCodeHash, }; use parity_scale_codec::Encode; @@ -73,12 +73,6 @@ mod tests; const LOG_TARGET: &'static str = "parachain::candidate-validation"; -/// The amount of time to wait before retrying after a retry-able backing validation error. We use a -/// lower value for the backing case, to fit within the lower backing timeout. -#[cfg(not(test))] -const PVF_BACKING_EXECUTION_RETRY_DELAY: Duration = Duration::from_millis(500); -#[cfg(test)] -const PVF_BACKING_EXECUTION_RETRY_DELAY: Duration = Duration::from_millis(200); /// The amount of time to wait before retrying after a retry-able approval validation error. We use /// a higher value for the approval case since we have more time, and if we wait longer it is more /// likely that transient conditions will resolve. @@ -626,16 +620,16 @@ async fn validate_candidate_exhaustive( validation_backend.validate_candidate(pvf, exec_timeout, params.encode()).await }, - PvfExecKind::Approval => { + PvfExecKind::Approval => validation_backend - .validate_candidate_with_retry( - raw_validation_code.to_vec(), - pvf_exec_timeout(&executor_params, exec_kind), - params, - executor_params, - ) - .await - } + .validate_candidate_with_retry( + raw_validation_code.to_vec(), + pvf_exec_timeout(&executor_params, exec_kind), + params, + executor_params, + PVF_APPROVAL_EXECUTION_RETRY_DELAY, + ) + .await, }; if let Err(ref error) = result { @@ -716,8 +710,8 @@ trait ValidationBackend { encoded_params: Vec, ) -> Result; - /// Tries executing a PVF for the approval subsystem. Will retry once if an error is encountered that may have been - /// transient. + /// Tries executing a PVF for the approval subsystem. Will retry once if an error is encountered + /// that may have been transient. /// /// NOTE: Should retry only on errors that are a result of execution itself, and not of /// preparation. @@ -727,6 +721,7 @@ trait ValidationBackend { exec_timeout: Duration, params: ValidationParams, executor_params: ExecutorParams, + retry_delay: Duration, ) -> Result { let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepKind::Lenient); // Construct the PVF a single time, since it is an expensive operation. Cloning it is cheap. @@ -746,8 +741,6 @@ trait ValidationBackend { return validation_result } - let retry_delay = PVF_APPROVAL_EXECUTION_RETRY_DELAY; - // Allow limited retries for each kind of error. let mut num_internal_retries_left = 1; let mut num_awd_retries_left = 1; diff --git a/polkadot/node/core/candidate-validation/src/tests.rs b/polkadot/node/core/candidate-validation/src/tests.rs index c78a6e9bd7bf1..9f4b58aaa80df 100644 --- a/polkadot/node/core/candidate-validation/src/tests.rs +++ b/polkadot/node/core/candidate-validation/src/tests.rs @@ -430,14 +430,14 @@ fn candidate_validation_ok_is_ok() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data.clone(), - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data.clone(), + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -480,27 +480,29 @@ fn candidate_validation_bad_return_is_invalid() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Err( + MockValidateCandidateBackend::with_hardcoded_result(Err( ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout), )), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); assert_matches!(v, ValidationResult::Invalid(InvalidCandidate::Timeout)); } +// Test that we vote valid if we get `AmbiguousWorkerDeath`, retry, and then succeed. #[test] -fn candidate_validation_multiple_ambiguous_errors_is_invalid() { +fn candidate_validation_one_ambiguous_error_is_valid() { let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; let pov = PoV { block_data: BlockData(vec![1; 32]) }; + let head_data = HeadData(vec![1, 1, 1]); let validation_code = ValidationCode(vec![2; 16]); let descriptor = make_valid_candidate_descriptor( @@ -509,7 +511,7 @@ fn candidate_validation_multiple_ambiguous_errors_is_invalid() { validation_data.hash(), pov.hash(), validation_code.hash(), - dummy_hash(), + head_data.hash(), dummy_hash(), Sr25519Keyring::Alice, ); @@ -522,28 +524,53 @@ fn candidate_validation_multiple_ambiguous_errors_is_invalid() { ); assert!(check.is_ok()); - let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; + let validation_result = WasmValidationResult { + head_data, + new_validation_code: Some(vec![2, 2, 2].into()), + upward_messages: Default::default(), + horizontal_messages: Default::default(), + processed_downward_messages: 0, + hrmp_watermark: 0, + }; + + let commitments = CandidateCommitments { + head_data: validation_result.head_data.clone(), + upward_messages: validation_result.upward_messages.clone(), + horizontal_messages: validation_result.horizontal_messages.clone(), + new_validation_code: validation_result.new_validation_code.clone(), + processed_downward_messages: validation_result.processed_downward_messages, + hrmp_watermark: validation_result.hrmp_watermark, + }; + + let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![ + MockValidateCandidateBackend::with_hardcoded_result_list(vec![ Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), + Ok(validation_result), ]), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + validation_data.clone(), + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Approval, + &Default::default(), )) .unwrap(); - assert_matches!(v, ValidationResult::Invalid(InvalidCandidate::ExecutionError(_))); + assert_matches!(v, ValidationResult::Valid(outputs, used_validation_data) => { + assert_eq!(outputs.head_data, HeadData(vec![1, 1, 1])); + assert_eq!(outputs.upward_messages, Vec::::new()); + assert_eq!(outputs.horizontal_messages, Vec::new()); + assert_eq!(outputs.new_validation_code, Some(vec![2, 2, 2].into())); + assert_eq!(outputs.hrmp_watermark, 0); + assert_eq!(used_validation_data, validation_data); + }); } -// Test that we retry on internal errors. #[test] -fn candidate_validation_dont_retry_internal_errors() { +fn candidate_validation_multiple_ambiguous_errors_is_invalid() { let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; let pov = PoV { block_data: BlockData(vec![1; 32]) }; @@ -571,28 +598,95 @@ fn candidate_validation_dont_retry_internal_errors() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![ + MockValidateCandidateBackend::with_hardcoded_result_list(vec![Err( + ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath), + )]), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), + )) + .unwrap(); + + assert_matches!(v, ValidationResult::Invalid(InvalidCandidate::ExecutionError(_))); +} + +// Test that we retry for approval on internal errors. +#[test] +fn candidate_validation_retry_internal_errors() { + let v = candidate_validation_retry_on_error_helper( + PvfExecKind::Approval, + vec![ Err(InternalValidationError::HostCommunication("foo".into()).into()), // Throw an AWD error, we should still retry again. Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), // Throw another internal error. Err(InternalValidationError::HostCommunication("bar".into()).into()), - ]), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), - )); + ], + ); + assert_matches!(v, Err(ValidationFailed(s)) if s.contains("bar")); +} + +// Test that we don't retry for backing on internal errors. +#[test] +fn candidate_validation_dont_retry_internal_errors() { + let v = candidate_validation_retry_on_error_helper( + PvfExecKind::Backing, + vec![ + Err(InternalValidationError::HostCommunication("foo".into()).into()), + // Throw an AWD error, we should still retry again. + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), + // Throw another internal error. + Err(InternalValidationError::HostCommunication("bar".into()).into()), + ], + ); + assert_matches!(v, Err(ValidationFailed(s)) if s.contains("foo")); } -// Test that we retry on panic errors. +// Test that we retry for approval on panic errors. +#[test] +fn candidate_validation_retry_panic_errors() { + let v = candidate_validation_retry_on_error_helper( + PvfExecKind::Approval, + vec![ + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("foo".into()))), + // Throw an AWD error, we should still retry again. + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), + // Throw another panic error. + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("bar".into()))), + ], + ); + + + assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(s))) if s == "bar".to_string()); +} + +// Test that we don't retry for backing on panic errors. #[test] fn candidate_validation_dont_retry_panic_errors() { + let v = candidate_validation_retry_on_error_helper( + PvfExecKind::Backing, + vec![ + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("foo".into()))), + // Throw an AWD error, we should still retry again. + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), + // Throw another panic error. + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("bar".into()))), + ], + ); + + assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(s))) if s == "foo".to_string()); +} + +fn candidate_validation_retry_on_error_helper( + exec_kind: PvfExecKind, + mock_errors: Vec>, +) -> Result { let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; let pov = PoV { block_data: BlockData(vec![1; 32]) }; @@ -619,24 +713,16 @@ fn candidate_validation_dont_retry_panic_errors() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; - let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![ - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("foo".into()))), - // Throw an AWD error, we should still retry again. - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), - // Throw another panic error. - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("bar".into()))), - ]), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + return executor::block_on(validate_candidate_exhaustive( + MockValidateCandidateBackend::with_hardcoded_result_list(mock_errors), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + exec_kind, + &Default::default(), )); - - assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(s))) if s == "foo".to_string()); } #[test] @@ -668,16 +754,16 @@ fn candidate_validation_timeout_is_internal_error() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Err( + MockValidateCandidateBackend::with_hardcoded_result(Err( ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout), )), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::Timeout))); @@ -715,14 +801,14 @@ fn candidate_validation_commitment_hash_mismatch_is_invalid() { }; let result = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -762,16 +848,16 @@ fn candidate_validation_code_mismatch_is_invalid() { let (_ctx, _ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Err( + MockValidateCandidateBackend::with_hardcoded_result(Err( ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout), )), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )) .unwrap(); @@ -821,14 +907,14 @@ fn compressed_code_works() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Ok(ValidationResult::Valid(_, _))); @@ -872,14 +958,14 @@ fn code_decompression_failure_is_error() { let (_ctx, _ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Err(_)); @@ -924,14 +1010,14 @@ fn pov_decompression_failure_is_invalid() { let (_ctx, _ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - validation_data, - validation_code, - candidate_receipt, - Arc::new(pov), - ExecutorParams::default(), - PvfExecKind::Backing, - &Default::default(), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), )); assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::PoVDecompressionFailure))); From b5bd48179e7278a1d99370f261be632a00fbb8ba Mon Sep 17 00:00:00 2001 From: Joao pedro Santos Date: Thu, 9 Nov 2023 15:53:09 -0300 Subject: [PATCH 07/15] fix fmt --- .../core/candidate-validation/src/tests.rs | 2 - polkadot/node/malus/src/variants/common.rs | 10 ++-- polkadot/primitives/src/lib.rs | 50 +++++++++---------- polkadot/primitives/src/v6/executor_params.rs | 5 +- 4 files changed, 31 insertions(+), 36 deletions(-) diff --git a/polkadot/node/core/candidate-validation/src/tests.rs b/polkadot/node/core/candidate-validation/src/tests.rs index 9f4b58aaa80df..557f9d69928c8 100644 --- a/polkadot/node/core/candidate-validation/src/tests.rs +++ b/polkadot/node/core/candidate-validation/src/tests.rs @@ -644,7 +644,6 @@ fn candidate_validation_dont_retry_internal_errors() { ], ); - assert_matches!(v, Err(ValidationFailed(s)) if s.contains("foo")); } @@ -662,7 +661,6 @@ fn candidate_validation_retry_panic_errors() { ], ); - assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(s))) if s == "bar".to_string()); } diff --git a/polkadot/node/malus/src/variants/common.rs b/polkadot/node/malus/src/variants/common.rs index 7b64e519365b9..47a0350185fe0 100644 --- a/polkadot/node/malus/src/variants/common.rs +++ b/polkadot/node/malus/src/variants/common.rs @@ -415,7 +415,7 @@ where candidate_receipt, pov, executor_params, - exec_kind: exec_kind, + exec_kind, response_sender, }, }) @@ -445,7 +445,7 @@ where candidate_receipt, pov, executor_params, - exec_kind: exec_kind, + exec_kind, response_sender, }, }), @@ -479,7 +479,7 @@ where candidate_receipt, pov, executor_params, - exec_kind: exec_kind, + exec_kind, response_sender, }, }) @@ -491,12 +491,12 @@ where candidate_receipt, pov, executor_params, - exec_kind: exec_kind, + exec_kind, response_sender, }, }), } - } + }, msg => Some(msg), } } diff --git a/polkadot/primitives/src/lib.rs b/polkadot/primitives/src/lib.rs index 33ab90fd44d71..2570bcadf606a 100644 --- a/polkadot/primitives/src/lib.rs +++ b/polkadot/primitives/src/lib.rs @@ -34,31 +34,31 @@ pub mod runtime_api; // Current primitives not requiring versioning are exported here. // Primitives requiring versioning must not be exported and must be referred by an exact version. pub use v6::{ - async_backing, byzantine_threshold, check_candidate_backing, collator_signature_payload, - effective_minimum_backing_votes, executor_params, metric_definitions, slashing, - supermajority_threshold, well_known_keys, AbridgedHostConfiguration, AbridgedHrmpChannel, - AccountId, AccountIndex, AccountPublic, ApprovalVote, AssignmentId, AsyncBackingParams, - AuthorityDiscoveryId, AvailabilityBitfield, BackedCandidate, Balance, BlakeTwo256, Block, - BlockId, BlockNumber, CandidateCommitments, CandidateDescriptor, CandidateEvent, CandidateHash, - CandidateIndex, CandidateReceipt, CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, - CollatorId, CollatorSignature, CommittedCandidateReceipt, CompactStatement, ConsensusLog, - CoreIndex, CoreState, DisputeState, DisputeStatement, DisputeStatementSet, DownwardMessage, - EncodeAs, ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, - ExplicitDisputeStatement, GroupIndex, GroupRotationInfo, Hash, HashT, HeadData, Header, - HorizontalMessages, HrmpChannelId, Id, InboundDownwardMessage, InboundHrmpMessage, IndexedVec, - InherentData, InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, Nonce, - OccupiedCore, OccupiedCoreAssumption, OutboundHrmpMessage, ParathreadClaim, ParathreadEntry, - PersistedValidationData, PvfCheckStatement, PvfExecKind, PvfPrepKind, - RuntimeMetricLabel, RuntimeMetricLabelValue, RuntimeMetricLabelValues, RuntimeMetricLabels, - RuntimeMetricOp, RuntimeMetricUpdate, ScheduledCore, ScrapedOnChainVotes, SessionIndex, - SessionInfo, Signature, Signed, SignedAvailabilityBitfield, SignedAvailabilityBitfields, - SignedStatement, SigningContext, Slot, UncheckedSigned, UncheckedSignedAvailabilityBitfield, - UncheckedSignedAvailabilityBitfields, UncheckedSignedStatement, UpgradeGoAhead, - UpgradeRestriction, UpwardMessage, ValidDisputeStatementKind, ValidationCode, - ValidationCodeHash, ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation, - ValidityError, ASSIGNMENT_KEY_TYPE_ID, LEGACY_MIN_BACKING_VOTES, LOWEST_PUBLIC_ID, - MAX_CODE_SIZE, MAX_HEAD_DATA_SIZE, MAX_POV_SIZE, ON_DEMAND_DEFAULT_QUEUE_MAX_SIZE, - PARACHAINS_INHERENT_IDENTIFIER, PARACHAIN_KEY_TYPE_ID, + async_backing, byzantine_threshold, check_candidate_backing, collator_signature_payload, + effective_minimum_backing_votes, executor_params, metric_definitions, slashing, + supermajority_threshold, well_known_keys, AbridgedHostConfiguration, AbridgedHrmpChannel, + AccountId, AccountIndex, AccountPublic, ApprovalVote, AssignmentId, AsyncBackingParams, + AuthorityDiscoveryId, AvailabilityBitfield, BackedCandidate, Balance, BlakeTwo256, Block, + BlockId, BlockNumber, CandidateCommitments, CandidateDescriptor, CandidateEvent, CandidateHash, + CandidateIndex, CandidateReceipt, CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, + CollatorId, CollatorSignature, CommittedCandidateReceipt, CompactStatement, ConsensusLog, + CoreIndex, CoreState, DisputeState, DisputeStatement, DisputeStatementSet, DownwardMessage, + EncodeAs, ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, + ExplicitDisputeStatement, GroupIndex, GroupRotationInfo, Hash, HashT, HeadData, Header, + HorizontalMessages, HrmpChannelId, Id, InboundDownwardMessage, InboundHrmpMessage, IndexedVec, + InherentData, InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, Nonce, + OccupiedCore, OccupiedCoreAssumption, OutboundHrmpMessage, ParathreadClaim, ParathreadEntry, + PersistedValidationData, PvfCheckStatement, PvfExecKind, PvfPrepKind, RuntimeMetricLabel, + RuntimeMetricLabelValue, RuntimeMetricLabelValues, RuntimeMetricLabels, RuntimeMetricOp, + RuntimeMetricUpdate, ScheduledCore, ScrapedOnChainVotes, SessionIndex, SessionInfo, Signature, + Signed, SignedAvailabilityBitfield, SignedAvailabilityBitfields, SignedStatement, + SigningContext, Slot, UncheckedSigned, UncheckedSignedAvailabilityBitfield, + UncheckedSignedAvailabilityBitfields, UncheckedSignedStatement, UpgradeGoAhead, + UpgradeRestriction, UpwardMessage, ValidDisputeStatementKind, ValidationCode, + ValidationCodeHash, ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation, + ValidityError, ASSIGNMENT_KEY_TYPE_ID, LEGACY_MIN_BACKING_VOTES, LOWEST_PUBLIC_ID, + MAX_CODE_SIZE, MAX_HEAD_DATA_SIZE, MAX_POV_SIZE, ON_DEMAND_DEFAULT_QUEUE_MAX_SIZE, + PARACHAINS_INHERENT_IDENTIFIER, PARACHAIN_KEY_TYPE_ID, }; #[cfg(feature = "std")] diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index 0f88aad318b57..b354914a3ad59 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -303,10 +303,7 @@ impl ExecutorParams { .or(Some(&DEFAULT_LENIENT_PREPARATION_TIMEOUT_MS)), ) { if *precheck >= *lenient { - return Err(IncompatibleValues( - "PvfPrepKind::Precheck", - "PvfPrepKind::Lenient", - )) + return Err(IncompatibleValues("PvfPrepKind::Precheck", "PvfPrepKind::Lenient")) } } From 769cef6043c2c75a2192457b7fbfa58241ae2261 Mon Sep 17 00:00:00 2001 From: Joao pedro Santos Date: Fri, 10 Nov 2023 08:51:38 -0300 Subject: [PATCH 08/15] minor fix --- polkadot/node/malus/src/variants/common.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/node/malus/src/variants/common.rs b/polkadot/node/malus/src/variants/common.rs index 47a0350185fe0..20b6654638e7d 100644 --- a/polkadot/node/malus/src/variants/common.rs +++ b/polkadot/node/malus/src/variants/common.rs @@ -401,7 +401,7 @@ where candidate_receipt, pov, executor_params, - exec_kind: exec_kind, + exec_kind, response_sender, .. }, From 5bc81837c180bf31b9664c511dbe33a0559fe85b Mon Sep 17 00:00:00 2001 From: Joao pedro Santos Date: Tue, 14 Nov 2023 09:01:47 -0300 Subject: [PATCH 09/15] add backer dont retry to documentation and refactor candidate-validation test --- .../core/candidate-validation/src/tests.rs | 51 ++++++++----------- .../src/node/utility/pvf-host-and-workers.md | 4 ++ 2 files changed, 26 insertions(+), 29 deletions(-) diff --git a/polkadot/node/core/candidate-validation/src/tests.rs b/polkadot/node/core/candidate-validation/src/tests.rs index 557f9d69928c8..360e42eb82a20 100644 --- a/polkadot/node/core/candidate-validation/src/tests.rs +++ b/polkadot/node/core/candidate-validation/src/tests.rs @@ -496,23 +496,20 @@ fn candidate_validation_bad_return_is_invalid() { assert_matches!(v, ValidationResult::Invalid(InvalidCandidate::Timeout)); } -// Test that we vote valid if we get `AmbiguousWorkerDeath`, retry, and then succeed. -#[test] -fn candidate_validation_one_ambiguous_error_is_valid() { - let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; - - let pov = PoV { block_data: BlockData(vec![1; 32]) }; - let head_data = HeadData(vec![1, 1, 1]); - let validation_code = ValidationCode(vec![2; 16]); - +fn perform_basic_checks_on_valid_candidate( + pov: &PoV, + validation_code: &ValidationCode, + validation_data: &PersistedValidationData, + head_data_hash: Hash +) -> CandidateDescriptor { let descriptor = make_valid_candidate_descriptor( ParaId::from(1_u32), dummy_hash(), validation_data.hash(), pov.hash(), validation_code.hash(), - head_data.hash(), - dummy_hash(), + head_data_hash, + head_data_hash, Sr25519Keyring::Alice, ); @@ -523,6 +520,19 @@ fn candidate_validation_one_ambiguous_error_is_valid() { &validation_code.hash(), ); assert!(check.is_ok()); + descriptor +} + +// Test that we vote valid if we get `AmbiguousWorkerDeath`, retry, and then succeed. +#[test] +fn candidate_validation_one_ambiguous_error_is_valid() { + let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; + + let pov = PoV { block_data: BlockData(vec![1; 32]) }; + let head_data = HeadData(vec![1, 1, 1]); + let validation_code = ValidationCode(vec![2; 16]); + + let descriptor = perform_basic_checks_on_valid_candidate(&pov, &validation_code, &validation_data, head_data.hash()); let validation_result = WasmValidationResult { head_data, @@ -576,24 +586,7 @@ fn candidate_validation_multiple_ambiguous_errors_is_invalid() { let pov = PoV { block_data: BlockData(vec![1; 32]) }; let validation_code = ValidationCode(vec![2; 16]); - let descriptor = make_valid_candidate_descriptor( - ParaId::from(1_u32), - dummy_hash(), - validation_data.hash(), - pov.hash(), - validation_code.hash(), - dummy_hash(), - dummy_hash(), - Sr25519Keyring::Alice, - ); - - let check = perform_basic_checks( - &descriptor, - validation_data.max_pov_size, - &pov, - &validation_code.hash(), - ); - assert!(check.is_ok()); + let descriptor = perform_basic_checks_on_valid_candidate(&pov, &validation_code, &validation_data, dummy_hash()); let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; diff --git a/polkadot/roadmap/implementers-guide/src/node/utility/pvf-host-and-workers.md b/polkadot/roadmap/implementers-guide/src/node/utility/pvf-host-and-workers.md index 52129f9eb80af..bb64dc5896920 100644 --- a/polkadot/roadmap/implementers-guide/src/node/utility/pvf-host-and-workers.md +++ b/polkadot/roadmap/implementers-guide/src/node/utility/pvf-host-and-workers.md @@ -27,6 +27,10 @@ hopefully resolve. We use a more brief delay here (1 second as opposed to 15 minutes for preparation (see above)), because a successful execution must happen in a short amount of time. +If the execution fails during the backing phase, we won't retry to reduce the chance of +supporting nondeterministic candidates. This reduces the chance of nondeterministic blocks +getting backed and honest backers getting slashed. + We currently know of the following specific cases that will lead to a retried execution request: From d69ef1a767d5a00cae6741f9047e01e02cd7bad3 Mon Sep 17 00:00:00 2001 From: "Marcin S." Date: Tue, 14 Nov 2023 19:17:47 +0100 Subject: [PATCH 10/15] Fix tests --- polkadot/node/core/candidate-validation/src/tests.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/polkadot/node/core/candidate-validation/src/tests.rs b/polkadot/node/core/candidate-validation/src/tests.rs index a8f9ddfa8edee..1ef99e15482fa 100644 --- a/polkadot/node/core/candidate-validation/src/tests.rs +++ b/polkadot/node/core/candidate-validation/src/tests.rs @@ -626,7 +626,7 @@ fn candidate_validation_retry_internal_errors() { Err(InternalValidationError::HostCommunication("foo".into()).into()), // Throw an AJD error, we should still retry again. Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousJobDeath( - "baz"into(), + "baz".into(), ))), // Throw another internal error. Err(InternalValidationError::HostCommunication("bar".into()).into()), @@ -658,11 +658,11 @@ fn candidate_validation_retry_panic_errors() { let v = candidate_validation_retry_on_error_helper( PvfExecKind::Approval, vec![ - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("foo".into()))), + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::JobError("foo".into()))), // Throw an AWD error, we should still retry again. Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), // Throw another panic error. - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("bar".into()))), + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::JobError("bar".into()))), ], ); @@ -675,11 +675,11 @@ fn candidate_validation_dont_retry_panic_errors() { let v = candidate_validation_retry_on_error_helper( PvfExecKind::Backing, vec![ - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("foo".into()))), + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::JobError("foo".into()))), // Throw an AWD error, we should still retry again. Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), // Throw another panic error. - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Panic("bar".into()))), + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::JobError("bar".into()))), ], ); From 7025c7b9c61dba76f8b01471b8c5434aa785b25e Mon Sep 17 00:00:00 2001 From: Joao pedro Santos Date: Wed, 15 Nov 2023 17:46:50 -0300 Subject: [PATCH 11/15] fix multiple ambiguous errors test --- .../core/candidate-validation/src/tests.rs | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/polkadot/node/core/candidate-validation/src/tests.rs b/polkadot/node/core/candidate-validation/src/tests.rs index 360e42eb82a20..36c505997f5b6 100644 --- a/polkadot/node/core/candidate-validation/src/tests.rs +++ b/polkadot/node/core/candidate-validation/src/tests.rs @@ -500,7 +500,7 @@ fn perform_basic_checks_on_valid_candidate( pov: &PoV, validation_code: &ValidationCode, validation_data: &PersistedValidationData, - head_data_hash: Hash + head_data_hash: Hash, ) -> CandidateDescriptor { let descriptor = make_valid_candidate_descriptor( ParaId::from(1_u32), @@ -532,7 +532,12 @@ fn candidate_validation_one_ambiguous_error_is_valid() { let head_data = HeadData(vec![1, 1, 1]); let validation_code = ValidationCode(vec![2; 16]); - let descriptor = perform_basic_checks_on_valid_candidate(&pov, &validation_code, &validation_data, head_data.hash()); + let descriptor = perform_basic_checks_on_valid_candidate( + &pov, + &validation_code, + &validation_data, + head_data.hash(), + ); let validation_result = WasmValidationResult { head_data, @@ -586,20 +591,26 @@ fn candidate_validation_multiple_ambiguous_errors_is_invalid() { let pov = PoV { block_data: BlockData(vec![1; 32]) }; let validation_code = ValidationCode(vec![2; 16]); - let descriptor = perform_basic_checks_on_valid_candidate(&pov, &validation_code, &validation_data, dummy_hash()); + let descriptor = perform_basic_checks_on_valid_candidate( + &pov, + &validation_code, + &validation_data, + dummy_hash(), + ); let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( - MockValidateCandidateBackend::with_hardcoded_result_list(vec![Err( - ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath), - )]), + MockValidateCandidateBackend::with_hardcoded_result_list(vec![ + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath)), + ]), validation_data, validation_code, candidate_receipt, Arc::new(pov), ExecutorParams::default(), - PvfExecKind::Backing, + PvfExecKind::Approval, &Default::default(), )) .unwrap(); From 79bf9f57d27727b847e3dc848dc4e19d3884b72b Mon Sep 17 00:00:00 2001 From: Joao pedro Santos Date: Fri, 17 Nov 2023 08:23:55 -0300 Subject: [PATCH 12/15] change PvfPrepKind::Lenient to PvfPrepKind::Prepare --- polkadot/node/core/candidate-validation/src/lib.rs | 6 +++--- polkadot/primitives/src/v6/executor_params.rs | 6 +++--- polkadot/primitives/src/v6/mod.rs | 2 +- .../runtime/parachains/src/configuration/benchmarking.rs | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs index a8ca535ac7859..2b0888cfe94ba 100644 --- a/polkadot/node/core/candidate-validation/src/lib.rs +++ b/polkadot/node/core/candidate-validation/src/lib.rs @@ -612,7 +612,7 @@ async fn validate_candidate_exhaustive( let result = match exec_kind { PvfExecKind::Backing => { - let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepKind::Lenient); + let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepKind::Precheck); let exec_timeout = pvf_exec_timeout(&executor_params, exec_kind); let pvf = PvfPrepData::from_code( raw_validation_code.to_vec(), @@ -731,7 +731,7 @@ trait ValidationBackend { executor_params: ExecutorParams, retry_delay: Duration, ) -> Result { - let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepKind::Lenient); + let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepKind::Prepare); // Construct the PVF a single time, since it is an expensive operation. Cloning it is cheap. let pvf = PvfPrepData::from_code( raw_validation_code, @@ -877,7 +877,7 @@ fn pvf_prep_timeout(executor_params: &ExecutorParams, kind: PvfPrepKind) -> Dura } match kind { PvfPrepKind::Precheck => DEFAULT_PRECHECK_PREPARATION_TIMEOUT, - PvfPrepKind::Lenient => DEFAULT_LENIENT_PREPARATION_TIMEOUT, + PvfPrepKind::Prepare => DEFAULT_LENIENT_PREPARATION_TIMEOUT, } } diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index b354914a3ad59..112a529f62b05 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -243,7 +243,7 @@ impl ExecutorParams { PrecheckingMaxMemory(_) => "PrecheckingMaxMemory", PvfPrepTimeout(kind, _) => match kind { PvfPrepKind::Precheck => "PvfPrepKind::Precheck", - PvfPrepKind::Lenient => "PvfPrepKind::Lenient", + PvfPrepKind::Prepare => "PvfPrepKind::Prepare", }, PvfExecTimeout(kind, _) => match kind { PvfExecKind::Backing => "PvfExecKind::Backing", @@ -299,11 +299,11 @@ impl ExecutorParams { if let (Some(precheck), Some(lenient)) = ( seen.get("PvfPrepKind::Precheck") .or(Some(&DEFAULT_PRECHECK_PREPARATION_TIMEOUT_MS)), - seen.get("PvfPrepKind::Lenient") + seen.get("PvfPrepKind::Prepare") .or(Some(&DEFAULT_LENIENT_PREPARATION_TIMEOUT_MS)), ) { if *precheck >= *lenient { - return Err(IncompatibleValues("PvfPrepKind::Precheck", "PvfPrepKind::Lenient")) + return Err(IncompatibleValues("PvfPrepKind::Precheck", "PvfPrepKind::Prepare")) } } diff --git a/polkadot/primitives/src/v6/mod.rs b/polkadot/primitives/src/v6/mod.rs index b5f6a8eb16dd3..6ebceda848ed8 100644 --- a/polkadot/primitives/src/v6/mod.rs +++ b/polkadot/primitives/src/v6/mod.rs @@ -1791,7 +1791,7 @@ pub enum PvfPrepKind { /// For execution and heads-up requests, the time period after which the preparation worker is /// considered unresponsive and will be killed. More lenient than the timeout for prechecking /// to prevent honest validators from timing out on valid PVFs. - Lenient, + Prepare, } /// Type discriminator for PVF execution timeouts and retries diff --git a/polkadot/runtime/parachains/src/configuration/benchmarking.rs b/polkadot/runtime/parachains/src/configuration/benchmarking.rs index 33f95eb0259e5..67daf1c459884 100644 --- a/polkadot/runtime/parachains/src/configuration/benchmarking.rs +++ b/polkadot/runtime/parachains/src/configuration/benchmarking.rs @@ -42,7 +42,7 @@ benchmarks! { ExecutorParam::WasmExtBulkMemory, ExecutorParam::PrecheckingMaxMemory(2 * 1024 * 1024 * 1024), ExecutorParam::PvfPrepTimeout(PvfPrepKind::Precheck, 60_000), - ExecutorParam::PvfPrepTimeout(PvfPrepKind::Lenient, 360_000), + ExecutorParam::PvfPrepTimeout(PvfPrepKind::Prepare, 360_000), ExecutorParam::PvfExecTimeout(PvfExecKind::Backing, 2_000), ExecutorParam::PvfExecTimeout(PvfExecKind::Approval, 12_000), ][..])) From 7f74004d1ad2ae8dbbbd288e9a643e38c3c03ee2 Mon Sep 17 00:00:00 2001 From: Joao pedro Santos Date: Sat, 18 Nov 2023 09:41:45 -0300 Subject: [PATCH 13/15] minor fix --- polkadot/node/core/candidate-validation/src/lib.rs | 2 +- polkadot/primitives/src/v6/mod.rs | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs index 2b0888cfe94ba..ae828a1791cc4 100644 --- a/polkadot/node/core/candidate-validation/src/lib.rs +++ b/polkadot/node/core/candidate-validation/src/lib.rs @@ -612,7 +612,7 @@ async fn validate_candidate_exhaustive( let result = match exec_kind { PvfExecKind::Backing => { - let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepKind::Precheck); + let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepKind::Prepare); let exec_timeout = pvf_exec_timeout(&executor_params, exec_kind); let pvf = PvfPrepData::from_code( raw_validation_code.to_vec(), diff --git a/polkadot/primitives/src/v6/mod.rs b/polkadot/primitives/src/v6/mod.rs index 6ebceda848ed8..09f1367b89d4b 100644 --- a/polkadot/primitives/src/v6/mod.rs +++ b/polkadot/primitives/src/v6/mod.rs @@ -1781,7 +1781,7 @@ impl WellKnownKey { } } -/// Type discriminator for PVF preparation timeouts +/// Type discriminator for PVF preparation. #[derive(Encode, Decode, TypeInfo, Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] pub enum PvfPrepKind { /// For prechecking requests, the time period after which the preparation worker is considered @@ -1794,10 +1794,11 @@ pub enum PvfPrepKind { Prepare, } -/// Type discriminator for PVF execution timeouts and retries +/// Type discriminator for PVF execution. #[derive(Encode, Decode, TypeInfo, Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] pub enum PvfExecKind { /// The amount of time to spend on execution during backing. + /// /// Retry is disabled to reduce the chance of nondeterministic blocks getting backed and honest /// backers getting slashed. Backing, From 2507cc10f811d89e70702957b86a758cf6e7624b Mon Sep 17 00:00:00 2001 From: Joao pedro Santos Date: Sun, 19 Nov 2023 09:36:16 -0300 Subject: [PATCH 14/15] change doc from PfvPrepKind and PvfExecKind to function --- .../node/core/candidate-validation/src/lib.rs | 21 +++++++++++++++++++ polkadot/primitives/src/v6/mod.rs | 20 ++++-------------- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs index ae828a1791cc4..2f47faa698021 100644 --- a/polkadot/node/core/candidate-validation/src/lib.rs +++ b/polkadot/node/core/candidate-validation/src/lib.rs @@ -611,6 +611,8 @@ async fn validate_candidate_exhaustive( }; let result = match exec_kind { + // Retry is disabled to reduce the chance of nondeterministic blocks getting backed and + // honest backers getting slashed. PvfExecKind::Backing => { let prep_timeout = pvf_prep_timeout(&executor_params, PvfPrepKind::Prepare); let exec_timeout = pvf_exec_timeout(&executor_params, exec_kind); @@ -871,6 +873,15 @@ fn perform_basic_checks( Ok(()) } +/// To determine the amount of timeout time for the pvf execution. +/// +/// Precheck +/// The time period after which the preparation worker is considered +/// unresponsive and will be killed. +/// +/// Prepare +///The time period after which the preparation worker is considered +/// unresponsive and will be killed. fn pvf_prep_timeout(executor_params: &ExecutorParams, kind: PvfPrepKind) -> Duration { if let Some(timeout) = executor_params.pvf_prep_timeout(kind) { return timeout @@ -881,6 +892,16 @@ fn pvf_prep_timeout(executor_params: &ExecutorParams, kind: PvfPrepKind) -> Dura } } +/// To determine the amount of timeout time for the pvf execution. +/// +/// Backing subsystem +/// The amount of time to spend on execution during backing. +/// +/// Approval subsystem +/// The amount of time to spend on execution during approval or disputes. +/// This should be much longer than the backing execution timeout to ensure that in the +/// absence of extremely large disparities between hardware, blocks that pass backing are +/// considered executable by approval checkers or dispute participants. fn pvf_exec_timeout(executor_params: &ExecutorParams, kind: PvfExecKind) -> Duration { if let Some(timeout) = executor_params.pvf_exec_timeout(kind) { return timeout diff --git a/polkadot/primitives/src/v6/mod.rs b/polkadot/primitives/src/v6/mod.rs index 09f1367b89d4b..6eaa688494360 100644 --- a/polkadot/primitives/src/v6/mod.rs +++ b/polkadot/primitives/src/v6/mod.rs @@ -1784,31 +1784,19 @@ impl WellKnownKey { /// Type discriminator for PVF preparation. #[derive(Encode, Decode, TypeInfo, Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] pub enum PvfPrepKind { - /// For prechecking requests, the time period after which the preparation worker is considered - /// unresponsive and will be killed. + /// For prechecking requests. Precheck, - /// For execution and heads-up requests, the time period after which the preparation worker is - /// considered unresponsive and will be killed. More lenient than the timeout for prechecking - /// to prevent honest validators from timing out on valid PVFs. + /// For execution and heads-up requestsd. Prepare, } /// Type discriminator for PVF execution. #[derive(Encode, Decode, TypeInfo, Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] pub enum PvfExecKind { - /// The amount of time to spend on execution during backing. - /// - /// Retry is disabled to reduce the chance of nondeterministic blocks getting backed and honest - /// backers getting slashed. + /// For backing requests. Backing, - - /// The amount of time to spend on execution during approval or disputes. - /// With retry enabled. - /// - /// This should be much longer than the backing execution timeout to ensure that in the - /// absence of extremely large disparities between hardware, blocks that pass backing are - /// considered executable by approval checkers or dispute participants. + /// For approval and dispute request. Approval, } From 7381ce15947f24f2801df466c230f2d111ee7920 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 19 Nov 2023 19:08:51 +0100 Subject: [PATCH 15/15] Update polkadot/primitives/src/v6/mod.rs --- polkadot/primitives/src/v6/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/primitives/src/v6/mod.rs b/polkadot/primitives/src/v6/mod.rs index 6eaa688494360..83b590dc32032 100644 --- a/polkadot/primitives/src/v6/mod.rs +++ b/polkadot/primitives/src/v6/mod.rs @@ -1787,7 +1787,7 @@ pub enum PvfPrepKind { /// For prechecking requests. Precheck, - /// For execution and heads-up requestsd. + /// For execution and heads-up requests. Prepare, }