From 6a5df502236d8878681bc05ef35e3398dcb056ca Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 4 Oct 2022 11:31:44 +0200 Subject: [PATCH 1/9] Fix indentation + add warning on participation errors. --- .../dispute-coordinator/src/initialized.rs | 131 +++++++++--------- 1 file changed, 67 insertions(+), 64 deletions(-) diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index 5f29245f33f8..1e9cc2f8c883 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -198,59 +198,62 @@ impl Initialized { gum::trace!(target: LOG_TARGET, "Waiting for message"); let mut overlay_db = OverlayedBackend::new(backend); let default_confirm = Box::new(|| Ok(())); - let confirm_write = - match MuxedMessage::receive(ctx, &mut self.participation_receiver).await? { - MuxedMessage::Participation(msg) => { - gum::trace!(target: LOG_TARGET, "MuxedMessage::Participation"); - let ParticipationStatement { - session, + let confirm_write = match MuxedMessage::receive(ctx, &mut self.participation_receiver) + .await? + { + MuxedMessage::Participation(msg) => { + gum::trace!(target: LOG_TARGET, "MuxedMessage::Participation"); + let ParticipationStatement { + session, + candidate_hash, + candidate_receipt, + outcome, + } = self.participation.get_participation_result(ctx, msg).await?; + if let Some(valid) = outcome.validity() { + gum::trace!( + target: LOG_TARGET, + ?session, + ?candidate_hash, + ?valid, + "Issuing local statement based on participation outcome." + ); + self.issue_local_statement( + ctx, + &mut overlay_db, candidate_hash, candidate_receipt, - outcome, - } = self.participation.get_participation_result(ctx, msg).await?; - if let Some(valid) = outcome.validity() { - gum::trace!( - target: LOG_TARGET, - ?session, - ?candidate_hash, - ?valid, - "Issuing local statement based on participation outcome." - ); - self.issue_local_statement( - ctx, - &mut overlay_db, - candidate_hash, - candidate_receipt, - session, - valid, - clock.now(), - ) - .await?; - } + session, + valid, + clock.now(), + ) + .await?; + } else { + gum::warn!(target: LOG_TARGET, ?outcome, "Dispute participation failed"); + } + default_confirm + }, + MuxedMessage::Subsystem(msg) => match msg { + FromOrchestra::Signal(OverseerSignal::Conclude) => return Ok(()), + FromOrchestra::Signal(OverseerSignal::ActiveLeaves(update)) => { + gum::trace!(target: LOG_TARGET, "OverseerSignal::ActiveLeaves"); + self.process_active_leaves_update( + ctx, + &mut overlay_db, + update, + clock.now(), + ) + .await?; default_confirm }, - MuxedMessage::Subsystem(msg) => match msg { - FromOrchestra::Signal(OverseerSignal::Conclude) => return Ok(()), - FromOrchestra::Signal(OverseerSignal::ActiveLeaves(update)) => { - gum::trace!(target: LOG_TARGET, "OverseerSignal::ActiveLeaves"); - self.process_active_leaves_update( - ctx, - &mut overlay_db, - update, - clock.now(), - ) - .await?; - default_confirm - }, - FromOrchestra::Signal(OverseerSignal::BlockFinalized(_, n)) => { - gum::trace!(target: LOG_TARGET, "OverseerSignal::BlockFinalized"); - self.scraper.process_finalized_block(&n); - default_confirm - }, - FromOrchestra::Communication { msg } => - self.handle_incoming(ctx, &mut overlay_db, msg, clock.now()).await?, + FromOrchestra::Signal(OverseerSignal::BlockFinalized(_, n)) => { + gum::trace!(target: LOG_TARGET, "OverseerSignal::BlockFinalized"); + self.scraper.process_finalized_block(&n); + default_confirm }, - }; + FromOrchestra::Communication { msg } => + self.handle_incoming(ctx, &mut overlay_db, msg, clock.now()).await?, + }, + }; if !overlay_db.is_empty() { let ops = overlay_db.into_write_ops(); @@ -391,19 +394,19 @@ impl Initialized { CompactStatement::Valid(_) => ValidDisputeStatementKind::BackingValid(relay_parent), }; - debug_assert!( - SignedDisputeStatement::new_checked( + debug_assert!( + SignedDisputeStatement::new_checked( DisputeStatement::Valid(valid_statement_kind), candidate_hash, session, validator_public.clone(), validator_signature.clone(), - ).is_ok(), - "Scraped backing votes had invalid signature! candidate: {:?}, session: {:?}, validator_public: {:?}", - candidate_hash, - session, - validator_public, - ); + ).is_ok(), + "Scraped backing votes had invalid signature! candidate: {:?}, session: {:?}, validator_public: {:?}", + candidate_hash, + session, + validator_public, + ); let signed_dispute_statement = SignedDisputeStatement::new_unchecked_from_trusted_source( DisputeStatement::Valid(valid_statement_kind), @@ -485,20 +488,20 @@ impl Initialized { }) .cloned()?; - debug_assert!( - SignedDisputeStatement::new_checked( + debug_assert!( + SignedDisputeStatement::new_checked( dispute_statement.clone(), candidate_hash, session, validator_public.clone(), validator_signature.clone(), - ).is_ok(), - "Scraped dispute votes had invalid signature! candidate: {:?}, session: {:?}, dispute_statement: {:?}, validator_public: {:?}", - candidate_hash, - session, + ).is_ok(), + "Scraped dispute votes had invalid signature! candidate: {:?}, session: {:?}, dispute_statement: {:?}, validator_public: {:?}", + candidate_hash, + session, dispute_statement, - validator_public, - ); + validator_public, + ); Some(( SignedDisputeStatement::new_unchecked_from_trusted_source( From 55d8d01deed71b2a1c8025af6e75b4970c517723 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 4 Oct 2022 11:49:38 +0200 Subject: [PATCH 2/9] Don't vote invalid on internal errors. --- node/core/dispute-coordinator/src/participation/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/dispute-coordinator/src/participation/mod.rs b/node/core/dispute-coordinator/src/participation/mod.rs index 874f37e63213..f458580b7820 100644 --- a/node/core/dispute-coordinator/src/participation/mod.rs +++ b/node/core/dispute-coordinator/src/participation/mod.rs @@ -357,7 +357,7 @@ async fn participate( err, ); - send_result(&mut result_sender, req, ParticipationOutcome::Invalid).await; + send_result(&mut result_sender, req, ParticipationOutcome::Error).await; }, Ok(Ok(ValidationResult::Invalid(invalid))) => { From 8d078cf9ab7ac4b5d799f3a95aa87680ceb08bbd Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 4 Oct 2022 13:46:11 +0200 Subject: [PATCH 3/9] Don't dispute on code compression error. --- node/core/candidate-validation/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index c3775ba1c453..98ef585e74bd 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -527,8 +527,9 @@ async fn validate_candidate_exhaustive( Err(e) => { gum::info!(target: LOG_TARGET, ?para_id, err=?e, "Invalid candidate (validation code)"); - // If the validation code is invalid, the candidate certainly is. - return Ok(ValidationResult::Invalid(InvalidCandidate::CodeDecompressionFailure)) + // Code already passed pre-checking, if decompression fails now this most likley means + // some local corruption happened. + return Err(ValidationFailed("Code decompression failed".to_string())) }, }; From 84a135ac290e02b7bf7cf34d296da14d22ce13de Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 4 Oct 2022 14:14:08 +0200 Subject: [PATCH 4/9] Remove CodeDecompressionError --- node/core/candidate-validation/src/tests.rs | 4 ++-- node/primitives/src/lib.rs | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs index ecac13d1440d..797fd5142e14 100644 --- a/node/core/candidate-validation/src/tests.rs +++ b/node/core/candidate-validation/src/tests.rs @@ -671,7 +671,7 @@ fn compressed_code_works() { } #[test] -fn code_decompression_failure_is_invalid() { +fn code_decompression_failure_is_error() { 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]); @@ -714,7 +714,7 @@ fn code_decompression_failure_is_invalid() { &Default::default(), )); - assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::CodeDecompressionFailure))); + assert_matches!(v, Err(_)); } #[test] diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs index 4551ce9855e3..ce18455c911a 100644 --- a/node/primitives/src/lib.rs +++ b/node/primitives/src/lib.rs @@ -219,8 +219,6 @@ pub enum InvalidCandidate { ParamsTooLarge(u64), /// Code size is over the limit. CodeTooLarge(u64), - /// Code does not decompress correctly. - CodeDecompressionFailure, /// PoV does not decompress correctly. PoVDecompressionFailure, /// Validation function returned invalid data. From f81357b7e6127ea23ae58cc54aa8fc1cd445b108 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 6 Oct 2022 11:34:33 +0200 Subject: [PATCH 5/9] Candidate not invalid if PVF preparation fails. Instead: Report error. --- node/core/candidate-validation/src/lib.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index 98ef585e74bd..cb39c273fa3e 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -561,7 +561,6 @@ async fn validate_candidate_exhaustive( match result { Err(ValidationError::InternalError(e)) => Err(ValidationFailed(e)), - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout)) => Ok(ValidationResult::Invalid(InvalidCandidate::Timeout)), Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::WorkerReportedError(e))) => @@ -571,8 +570,7 @@ async fn validate_candidate_exhaustive( "ambiguous worker death".to_string(), ))), Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::PrepareError(e))) => - Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(e))), - + Err(ValidationFailed(e)), Ok(res) => if res.head_data.hash() != candidate_receipt.descriptor.para_head { gum::info!(target: LOG_TARGET, ?para_id, "Invalid candidate (para_head)"); From 7051858dfa47c35a81c8a943f92a1ac9cdec029d Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 6 Oct 2022 16:57:39 +0200 Subject: [PATCH 6/9] Fix malus --- node/malus/src/variants/common.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/node/malus/src/variants/common.rs b/node/malus/src/variants/common.rs index e112aa49f83e..e9194e7d1736 100644 --- a/node/malus/src/variants/common.rs +++ b/node/malus/src/variants/common.rs @@ -61,8 +61,6 @@ pub enum FakeCandidateValidationError { ParamsTooLarge, /// Code size is over the limit. CodeTooLarge, - /// Code does not decompress correctly. - CodeDecompressionFailure, /// PoV does not decompress correctly. POVDecompressionFailure, /// Validation function returned invalid data. @@ -88,8 +86,6 @@ impl Into for FakeCandidateValidationError { FakeCandidateValidationError::Timeout => InvalidCandidate::Timeout, FakeCandidateValidationError::ParamsTooLarge => InvalidCandidate::ParamsTooLarge(666), FakeCandidateValidationError::CodeTooLarge => InvalidCandidate::CodeTooLarge(666), - FakeCandidateValidationError::CodeDecompressionFailure => - InvalidCandidate::CodeDecompressionFailure, FakeCandidateValidationError::POVDecompressionFailure => InvalidCandidate::PoVDecompressionFailure, FakeCandidateValidationError::BadReturn => InvalidCandidate::BadReturn, From 5159879990d39e547795c01aef9834cb53ee57b7 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 6 Oct 2022 17:02:09 +0200 Subject: [PATCH 7/9] Add clarifying comment. --- node/core/candidate-validation/src/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index cb39c273fa3e..baa4b46ad927 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -570,6 +570,10 @@ async fn validate_candidate_exhaustive( "ambiguous worker death".to_string(), ))), Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::PrepareError(e))) => + // In principle if preparation of the `WASM` fails, the current candidate can not be the + // reason for that. So we can't say whether it is invalid or not in addition with + // pre-checking enabled only valid runtimes should ever get enacted, so we can be + // reasonably sure that this is some local problem on the current node. Err(ValidationFailed(e)), Ok(res) => if res.head_data.hash() != candidate_receipt.descriptor.para_head { From a88f697abd8a8f2e377e29539d49b3954bcd243d Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 6 Oct 2022 17:15:44 +0200 Subject: [PATCH 8/9] cargo fmt --- node/core/candidate-validation/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index baa4b46ad927..f95f4cb4f6b3 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -570,10 +570,10 @@ async fn validate_candidate_exhaustive( "ambiguous worker death".to_string(), ))), Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::PrepareError(e))) => - // In principle if preparation of the `WASM` fails, the current candidate can not be the - // reason for that. So we can't say whether it is invalid or not in addition with - // pre-checking enabled only valid runtimes should ever get enacted, so we can be - // reasonably sure that this is some local problem on the current node. + // In principle if preparation of the `WASM` fails, the current candidate can not be the + // reason for that. So we can't say whether it is invalid or not in addition with + // pre-checking enabled only valid runtimes should ever get enacted, so we can be + // reasonably sure that this is some local problem on the current node. Err(ValidationFailed(e)), Ok(res) => if res.head_data.hash() != candidate_receipt.descriptor.para_head { From 3198c8d0694f54789848d1a76979d6fd48d8796d Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 11 Oct 2022 13:17:45 +0200 Subject: [PATCH 9/9] Fix indentation. --- node/core/candidate-validation/src/lib.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index f95f4cb4f6b3..2a73dbb441e6 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -569,12 +569,13 @@ async fn validate_candidate_exhaustive( Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError( "ambiguous worker death".to_string(), ))), - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::PrepareError(e))) => - // In principle if preparation of the `WASM` fails, the current candidate can not be the - // reason for that. So we can't say whether it is invalid or not in addition with - // pre-checking enabled only valid runtimes should ever get enacted, so we can be - // reasonably sure that this is some local problem on the current node. - Err(ValidationFailed(e)), + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::PrepareError(e))) => { + // In principle if preparation of the `WASM` fails, the current candidate can not be the + // reason for that. So we can't say whether it is invalid or not in addition with + // pre-checking enabled only valid runtimes should ever get enacted, so we can be + // reasonably sure that this is some local problem on the current node. + Err(ValidationFailed(e)) + }, Ok(res) => if res.head_data.hash() != candidate_receipt.descriptor.para_head { gum::info!(target: LOG_TARGET, ?para_id, "Invalid candidate (para_head)");