From 4f01975fd2e9ff58dfe38b0f53906a6b0e2f2d86 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 16 Jun 2022 18:30:47 +0200 Subject: [PATCH] Don't import backing statements directly into the dispute coordinator. This also gets rid of a redundant signature check. Both should have some impact on backing performance. Reasoning (aka why this is fine): For the signature check: As mentioned, it is a redundant check. The signature has already been checked at this point. This is even made obvious by the used types. The smart constructor is not perfect as discussed [here](https://github.com/paritytech/polkadot/issues/3455), but still a reasonable security. For not importing to the dispute-coordinator: This should be fine as the dispute coordinator does scrape backing votes from chain. This suffices in practice as a super majority of validators must have seen a backing fork in order for a candidate to get included and only included candidates pose a threat to our system. The import from chain is preferable over direct import of backing votes for two reasons: 1. The import is batched, greatly improving import performance. (All backing votes for a candidate are imported with a single import.) 2. We do less work in general as not every candidate for which statements are gossiped might actually make it on a chain. The dispute coordinator as with the previous implementation would still import and keep those votes around for six sessions. While redundancy is good for reliability in the event of bugs, this also comes at a non negible cost. The dispute-coordinator right now is the subsystem with the highest load, despite the fact that it should not be doing much during mormal operation. We'll see on Versi how much of a performance improvement this PR provides. --- node/core/backing/src/lib.rs | 89 +---------------- node/core/backing/src/tests.rs | 172 --------------------------------- 2 files changed, 4 insertions(+), 257 deletions(-) diff --git a/node/core/backing/src/lib.rs b/node/core/backing/src/lib.rs index a189b5955c89..78cbb4787eb7 100644 --- a/node/core/backing/src/lib.rs +++ b/node/core/backing/src/lib.rs @@ -31,15 +31,15 @@ use futures::{ use error::{Error, FatalResult}; use polkadot_node_primitives::{ - AvailableData, InvalidCandidate, PoV, SignedDisputeStatement, SignedFullStatement, Statement, - ValidationResult, BACKING_EXECUTION_TIMEOUT, + AvailableData, InvalidCandidate, PoV, SignedFullStatement, Statement, ValidationResult, + BACKING_EXECUTION_TIMEOUT, }; use polkadot_node_subsystem::{ jaeger, messages::{ AvailabilityDistributionMessage, AvailabilityStoreMessage, CandidateBackingMessage, - CandidateValidationMessage, CollatorProtocolMessage, DisputeCoordinatorMessage, - ProvisionableData, ProvisionerMessage, RuntimeApiRequest, StatementDistributionMessage, + CandidateValidationMessage, CollatorProtocolMessage, ProvisionableData, ProvisionerMessage, + RuntimeApiRequest, StatementDistributionMessage, }, overseer, ActiveLeavesUpdate, FromOrchestra, OverseerSignal, PerLeafSpan, SpawnedSubsystem, Stage, SubsystemError, @@ -380,7 +380,6 @@ async fn handle_active_leaves_update( let job = CandidateBackingJob { parent, - session_index, assignment, required_collator, issued_statements: HashSet::new(), @@ -411,8 +410,6 @@ struct JobAndSpan { struct CandidateBackingJob { /// The hash of the relay parent on top of which this job is doing it's work. parent: Hash, - /// The session index this corresponds to. - session_index: SessionIndex, /// The `ParaId` assigned to this validator assignment: Option, /// The collator required to author the candidate, if any. @@ -783,8 +780,6 @@ async fn validate_and_make_available( tx_command.send((relay_parent, make_command(res))).await.map_err(Into::into) } -struct ValidatorIndexOutOfBounds; - #[overseer::contextbounds(CandidateBacking, prefix = self::overseer)] impl CandidateBackingJob { async fn handle_validated_candidate_command( @@ -1014,21 +1009,6 @@ impl CandidateBackingJob { ) }; - if let Err(ValidatorIndexOutOfBounds) = self - .dispatch_new_statement_to_dispute_coordinator(ctx.sender(), candidate_hash, &statement) - .await - { - gum::warn!( - target: LOG_TARGET, - session_index = ?self.session_index, - relay_parent = ?self.parent, - validator_index = statement.validator_index().0, - "Supposedly 'Signed' statement has validator index out of bounds." - ); - - return Ok(None) - } - let stmt = primitive_statement_to_table(statement); let summary = self.table.import_statement(&self.table_context, stmt); @@ -1083,67 +1063,6 @@ impl CandidateBackingJob { Ok(summary) } - /// The dispute coordinator keeps track of all statements by validators about every recent - /// candidate. - /// - /// When importing a statement, this should be called access the candidate receipt either - /// from the statement itself or from the underlying statement table in order to craft - /// and dispatch the notification to the dispute coordinator. - /// - /// This also does bounds-checking on the validator index and will return an error if the - /// validator index is out of bounds for the current validator set. It's expected that - /// this should never happen due to the interface of the candidate backing subsystem - - /// the networking component responsible for feeding statements to the backing subsystem - /// is meant to check the signature and provenance of all statements before submission. - async fn dispatch_new_statement_to_dispute_coordinator( - &self, - sender: &mut impl overseer::CandidateBackingSenderTrait, - candidate_hash: CandidateHash, - statement: &SignedFullStatement, - ) -> Result<(), ValidatorIndexOutOfBounds> { - // Dispatch the statement to the dispute coordinator. - let validator_index = statement.validator_index(); - let signing_context = - SigningContext { parent_hash: self.parent, session_index: self.session_index }; - - let validator_public = match self.table_context.validators.get(validator_index.0 as usize) { - None => return Err(ValidatorIndexOutOfBounds), - Some(v) => v, - }; - - let maybe_candidate_receipt = match statement.payload() { - Statement::Seconded(receipt) => Some(receipt.to_plain()), - Statement::Valid(candidate_hash) => { - // Valid statements are only supposed to be imported - // once we've seen at least one `Seconded` statement. - self.table.get_candidate(&candidate_hash).map(|c| c.to_plain()) - }, - }; - - let maybe_signed_dispute_statement = SignedDisputeStatement::from_backing_statement( - statement.as_unchecked(), - signing_context, - validator_public.clone(), - ) - .ok(); - - if let (Some(candidate_receipt), Some(dispute_statement)) = - (maybe_candidate_receipt, maybe_signed_dispute_statement) - { - sender - .send_message(DisputeCoordinatorMessage::ImportStatements { - candidate_hash, - candidate_receipt, - session: self.session_index, - statements: vec![(dispute_statement, validator_index)], - pending_confirmation: None, - }) - .await; - } - - Ok(()) - } - async fn handle_second_msg( &mut self, root_span: &jaeger::Span, diff --git a/node/core/backing/src/tests.rs b/node/core/backing/src/tests.rs index 0243c68c7c4c..9b392cf956a5 100644 --- a/node/core/backing/src/tests.rs +++ b/node/core/backing/src/tests.rs @@ -273,34 +273,6 @@ async fn test_startup(virtual_overseer: &mut VirtualOverseer, test_state: &TestS ); } -async fn test_dispute_coordinator_notifications( - virtual_overseer: &mut VirtualOverseer, - candidate_hash: CandidateHash, - session: SessionIndex, - validator_indices: Vec, -) { - for validator_index in validator_indices { - assert_matches!( - virtual_overseer.recv().await, - AllMessages::DisputeCoordinator( - DisputeCoordinatorMessage::ImportStatements { - candidate_hash: c_hash, - candidate_receipt: c_receipt, - session: s, - statements, - pending_confirmation: None, - } - ) => { - assert_eq!(c_hash, candidate_hash); - assert_eq!(c_receipt.hash(), c_hash); - assert_eq!(s, session); - assert_eq!(statements.len(), 1); - assert_eq!(statements[0].1, validator_index); - } - ) - } -} - // Test that a `CandidateBackingMessage::Second` issues validation work // and in case validation is successful issues a `StatementDistributionMessage`. #[test] @@ -364,14 +336,6 @@ fn backing_second_works() { } ); - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate.hash(), - test_state.session(), - vec![ValidatorIndex(0)], - ) - .await; - assert_matches!( virtual_overseer.recv().await, AllMessages::StatementDistribution( @@ -469,14 +433,6 @@ fn backing_works() { virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate_a_hash, - test_state.session(), - vec![ValidatorIndex(2)], - ) - .await; - // Sending a `Statement::Seconded` for our assignment will start // validation process. The first thing requested is the PoV. assert_matches!( @@ -526,14 +482,6 @@ fn backing_works() { } ); - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate_a_hash, - test_state.session(), - vec![ValidatorIndex(0)], - ) - .await; - assert_matches!( virtual_overseer.recv().await, AllMessages::Provisioner( @@ -560,14 +508,6 @@ fn backing_works() { virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate_a_hash, - test_state.session(), - vec![ValidatorIndex(5)], - ) - .await; - virtual_overseer .send(FromOrchestra::Signal(OverseerSignal::ActiveLeaves( ActiveLeavesUpdate::stop_work(test_state.relay_parent), @@ -664,14 +604,6 @@ fn backing_works_while_validation_ongoing() { CandidateBackingMessage::Statement(test_state.relay_parent, signed_a.clone()); virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate_a.hash(), - test_state.session(), - vec![ValidatorIndex(2)], - ) - .await; - // Sending a `Statement::Seconded` for our assignment will start // validation process. The first thing requested is PoV from the // `PoVDistribution`. @@ -711,14 +643,6 @@ fn backing_works_while_validation_ongoing() { virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate_a.hash(), - test_state.session(), - vec![ValidatorIndex(5)], - ) - .await; - // Candidate gets backed entirely by other votes. assert_matches!( virtual_overseer.recv().await, @@ -738,14 +662,6 @@ fn backing_works_while_validation_ongoing() { virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate_a.hash(), - test_state.session(), - vec![ValidatorIndex(3)], - ) - .await; - let (tx, rx) = oneshot::channel(); let msg = CandidateBackingMessage::GetBackedCandidates( test_state.relay_parent, @@ -845,14 +761,6 @@ fn backing_misbehavior_works() { virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate_a_hash, - test_state.session(), - vec![ValidatorIndex(2)], - ) - .await; - assert_matches!( virtual_overseer.recv().await, AllMessages::AvailabilityDistribution( @@ -898,14 +806,6 @@ fn backing_misbehavior_works() { } ); - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate_a_hash, - test_state.session(), - vec![ValidatorIndex(0)], - ) - .await; - assert_matches!( virtual_overseer.recv().await, AllMessages::Provisioner( @@ -937,14 +837,6 @@ fn backing_misbehavior_works() { virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate_a_hash, - test_state.session(), - vec![ValidatorIndex(2)], - ) - .await; - assert_matches!( virtual_overseer.recv().await, AllMessages::Provisioner( @@ -1087,14 +979,6 @@ fn backing_dont_second_invalid() { } ); - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate_b.hash(), - test_state.session(), - vec![ValidatorIndex(0)], - ) - .await; - assert_matches!( virtual_overseer.recv().await, AllMessages::StatementDistribution( @@ -1163,14 +1047,6 @@ fn backing_second_after_first_fails_works() { virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate.hash(), - test_state.session(), - vec![ValidatorIndex(2)], - ) - .await; - // Subsystem requests PoV and requests validation. assert_matches!( virtual_overseer.recv().await, @@ -1297,14 +1173,6 @@ fn backing_works_after_failed_validation() { virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate.hash(), - test_state.session(), - vec![ValidatorIndex(2)], - ) - .await; - // Subsystem requests PoV and requests validation. assert_matches!( virtual_overseer.recv().await, @@ -1615,14 +1483,6 @@ fn retry_works() { CandidateBackingMessage::Statement(test_state.relay_parent, signed_a.clone()); virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate.hash(), - test_state.session(), - vec![ValidatorIndex(2)], - ) - .await; - // Subsystem requests PoV and requests validation. // We cancel - should mean retry on next backing statement. assert_matches!( @@ -1642,14 +1502,6 @@ fn retry_works() { CandidateBackingMessage::Statement(test_state.relay_parent, signed_b.clone()); virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate.hash(), - test_state.session(), - vec![ValidatorIndex(3)], - ) - .await; - // Not deterministic which message comes first: for _ in 0u32..2 { match virtual_overseer.recv().await { @@ -1674,14 +1526,6 @@ fn retry_works() { CandidateBackingMessage::Statement(test_state.relay_parent, signed_c.clone()); virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate.hash(), - test_state.session(), - vec![ValidatorIndex(5)], - ) - .await; - assert_matches!( virtual_overseer.recv().await, AllMessages::AvailabilityDistribution( @@ -1806,14 +1650,6 @@ fn observes_backing_even_if_not_validator() { virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate_a_hash, - test_state.session(), - vec![ValidatorIndex(0), ValidatorIndex(5)], - ) - .await; - assert_matches!( virtual_overseer.recv().await, AllMessages::Provisioner( @@ -1831,14 +1667,6 @@ fn observes_backing_even_if_not_validator() { virtual_overseer.send(FromOrchestra::Communication { msg: statement }).await; - test_dispute_coordinator_notifications( - &mut virtual_overseer, - candidate_a_hash, - test_state.session(), - vec![ValidatorIndex(2)], - ) - .await; - virtual_overseer .send(FromOrchestra::Signal(OverseerSignal::ActiveLeaves( ActiveLeavesUpdate::stop_work(test_state.relay_parent),