From 9b71f9991383f411d869251013cfa05b7b6405d6 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 1 Feb 2022 15:02:13 +0100 Subject: [PATCH 01/13] Only bypass spam slots on actualy approval/backing vote import. --- .../src/real/initialized.rs | 31 ++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/initialized.rs b/node/core/dispute-coordinator/src/real/initialized.rs index bb369b28fe99..56c7e5cfac80 100644 --- a/node/core/dispute-coordinator/src/real/initialized.rs +++ b/node/core/dispute-coordinator/src/real/initialized.rs @@ -822,6 +822,15 @@ impl Initialized { .find(|(_, index)| controlled_indices.contains(index)) .is_some(); + // Whether or not the import is coming from approval checking/backing. + // NOTE: Those imports should provide a completely separate API from importing untrusted + // dispute votes, not treating them the same would do away with much of the complexity here + // and would also improve performance. + // + // See https://github.com/paritytech/polkadot/issues/4793 + let mut is_backing_import = true; + let mut is_approval_import = true; + // Update candidate votes. for (statement, val_index) in &statements { if validators @@ -842,6 +851,17 @@ impl Initialized { match statement.statement().clone() { DisputeStatement::Valid(valid_kind) => { self.metrics.on_valid_vote(); + + match valid_kind { + ValidDisputeStatementKind::BackingSeconded(_) => is_approval_import = false, + ValidDisputeStatementKind::BackingValid(_) => is_approval_import = false, + ValidDisputeStatementKind::ApprovalChecking => is_backing_import = false, + ValidDisputeStatementKind::Explicit => { + is_approval_import = false; + is_backing_import = false; + } + } + insert_into_statement_vec( &mut votes.valid, valid_kind, @@ -851,6 +871,10 @@ impl Initialized { }, DisputeStatement::Invalid(invalid_kind) => { self.metrics.on_invalid_vote(); + + is_approval_import = false; + is_backing_import = false; + insert_into_statement_vec( &mut votes.invalid, invalid_kind, @@ -870,11 +894,10 @@ impl Initialized { byzantine_threshold(n_validators); // Potential spam: - if !is_confirmed { + if !is_confirmed && !is_backing_import && !is_approval_import { let mut free_spam_slots = false; - for (statement, index) in statements.iter() { - free_spam_slots |= statement.statement().is_backing() || - self.spam_slots.add_unconfirmed(session, candidate_hash, *index); + for (_, index) in statements.iter() { + free_spam_slots |= self.spam_slots.add_unconfirmed(session, candidate_hash, *index); } // No reporting validator had a free spam slot: if !free_spam_slots { From a0ee0b55d7aca55857ae44463652aa34519bc188 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 1 Feb 2022 17:14:45 +0100 Subject: [PATCH 02/13] Fix spam slot treatment. --- .../dispute-coordinator/src/real/initialized.rs | 5 +++-- .../dispute-coordinator/src/real/spam_slots.rs | 14 +++++++++++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/initialized.rs b/node/core/dispute-coordinator/src/real/initialized.rs index 56c7e5cfac80..b14332040c5c 100644 --- a/node/core/dispute-coordinator/src/real/initialized.rs +++ b/node/core/dispute-coordinator/src/real/initialized.rs @@ -895,9 +895,10 @@ impl Initialized { // Potential spam: if !is_confirmed && !is_backing_import && !is_approval_import { - let mut free_spam_slots = false; + let mut free_spam_slots = true; + // Only allow import if all validators of import have not exceeded their spam slots: for (_, index) in statements.iter() { - free_spam_slots |= self.spam_slots.add_unconfirmed(session, candidate_hash, *index); + free_spam_slots &= self.spam_slots.add_unconfirmed(session, candidate_hash, *index); } // No reporting validator had a free spam slot: if !free_spam_slots { diff --git a/node/core/dispute-coordinator/src/real/spam_slots.rs b/node/core/dispute-coordinator/src/real/spam_slots.rs index 6c8707152a68..74cfcb898ce2 100644 --- a/node/core/dispute-coordinator/src/real/spam_slots.rs +++ b/node/core/dispute-coordinator/src/real/spam_slots.rs @@ -35,7 +35,10 @@ type SpamCount = u32; /// disputes _should_ have been seen as included my enough validators. (Otherwise the candidate /// would not have been available in the first place and could not have been included.) So this is /// really just a fallback mechanism if things go terribly wrong. +#[cfg(not(test))] const MAX_SPAM_VOTES: SpamCount = 50; +#[cfg(test)] +const MAX_SPAM_VOTES: SpamCount = 1; /// Spam slots for raised disputes concerning unknown candidates. pub struct SpamSlots { @@ -91,10 +94,15 @@ impl SpamSlots { if validators.insert(validator) { *c += 1; - true - } else { - false } + // Always true even if validator already voted for that candidate. This is necessary, because + // when providing a dispute vote, a validator has to provide an opposing vote: We don't + // want to increase spam count on the validator of that vote for each message where its + // vote is used. + // + // Limiting the kind of spam that is possible due to this should be taken care of by + // networking (dispute-distribution). + true } /// Clear out spam slots for a given candiate in a session. From 0b256989109af614575fc5c976983292c3927f21 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 3 Feb 2022 19:56:45 +0100 Subject: [PATCH 03/13] More tests. --- .../src/real/spam_slots.rs | 13 +- .../dispute-coordinator/src/real/tests.rs | 416 +++++++++++++++++- 2 files changed, 405 insertions(+), 24 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/spam_slots.rs b/node/core/dispute-coordinator/src/real/spam_slots.rs index 74cfcb898ce2..cc10404fbd4f 100644 --- a/node/core/dispute-coordinator/src/real/spam_slots.rs +++ b/node/core/dispute-coordinator/src/real/spam_slots.rs @@ -92,16 +92,17 @@ impl SpamSlots { } let validators = self.unconfirmed.entry((session, candidate)).or_default(); + // We don't want to increase spam count for multiple votes on the same candidate - see + // below. + // This kind of spam does not do much harm and should be limited enough already by + // networking (dispute-distribution). if validators.insert(validator) { *c += 1; } // Always true even if validator already voted for that candidate. This is necessary, because - // when providing a dispute vote, a validator has to provide an opposing vote: We don't - // want to increase spam count on the validator of that vote for each message where its - // vote is used. - // - // Limiting the kind of spam that is possible due to this should be taken care of by - // networking (dispute-distribution). + // when providing a dispute vote, a validator has to provide an opposing vote - multiple + // validators might use the same opposing vote. + true } diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index f3ff8a260aa5..f85045eef4bd 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -43,7 +43,7 @@ use polkadot_node_subsystem::{ }; use polkadot_node_subsystem_util::TimeoutExt; use sc_keystore::LocalKeystore; -use sp_core::testing::TaskExecutor; +use sp_core::{sr25519::Pair, testing::TaskExecutor, Pair as PairT}; use sp_keyring::Sr25519Keyring; use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr}; @@ -78,12 +78,12 @@ use super::db::v1::DbBackend; const TEST_TIMEOUT: Duration = Duration::from_secs(2); // sets up a keystore with the given keyring accounts. -fn make_keystore(accounts: &[Sr25519Keyring]) -> LocalKeystore { +fn make_keystore<'a>(seeds: impl Iterator) -> LocalKeystore { let store = LocalKeystore::in_memory(); - for s in accounts.iter().copied().map(|k| k.to_seed()) { + for s in seeds { store - .sr25519_generate_new(polkadot_primitives::v1::PARACHAIN_KEY_TYPE_ID, Some(s.as_str())) + .sr25519_generate_new(polkadot_primitives::v1::PARACHAIN_KEY_TYPE_ID, Some(&s)) .unwrap(); } @@ -120,7 +120,7 @@ impl MockClock { } struct TestState { - validators: Vec, + validators: Vec, validator_public: Vec, validator_groups: Vec>, master_keystore: Arc, @@ -133,17 +133,28 @@ struct TestState { impl Default for TestState { fn default() -> TestState { + let p1 = Pair::from_string("//Polka", None).unwrap(); + let p2 = Pair::from_string("//Dot", None).unwrap(); + let p3 = Pair::from_string("//Kusama", None).unwrap(); let validators = vec![ - Sr25519Keyring::Alice, - Sr25519Keyring::Bob, - Sr25519Keyring::Charlie, - Sr25519Keyring::Dave, - Sr25519Keyring::Eve, - Sr25519Keyring::One, - Sr25519Keyring::Ferdie, + (Sr25519Keyring::Alice.pair(), Sr25519Keyring::Alice.to_seed()), + (Sr25519Keyring::Bob.pair(), Sr25519Keyring::Bob.to_seed()), + (Sr25519Keyring::Charlie.pair(), Sr25519Keyring::Charlie.to_seed()), + (Sr25519Keyring::Dave.pair(), Sr25519Keyring::Dave.to_seed()), + (Sr25519Keyring::Eve.pair(), Sr25519Keyring::Eve.to_seed()), + (Sr25519Keyring::One.pair(), Sr25519Keyring::One.to_seed()), + (Sr25519Keyring::Ferdie.pair(), Sr25519Keyring::Ferdie.to_seed()), + // Two more keys needed so disputes are not confirmed already with only 3 statements. + (p1, "//Polka".into()), + (p2, "//Dot".into()), + (p3, "//Kusama".into()), ]; - let validator_public = validators.iter().map(|k| ValidatorId::from(k.public())).collect(); + let validator_public = validators + .clone() + .into_iter() + .map(|k| ValidatorId::from(k.0.public())) + .collect(); let validator_groups = vec![ vec![ValidatorIndex(0), ValidatorIndex(1)], @@ -151,14 +162,15 @@ impl Default for TestState { vec![ValidatorIndex(4), ValidatorIndex(5), ValidatorIndex(6)], ]; - let master_keystore = make_keystore(&validators).into(); - let subsystem_keystore = make_keystore(&[Sr25519Keyring::Alice]).into(); + let master_keystore = make_keystore(validators.iter().map(|v| v.1.clone())).into(); + let subsystem_keystore = + make_keystore(vec![Sr25519Keyring::Alice.to_seed()].into_iter()).into(); let db = Arc::new(kvdb_memorydb::create(1)); let config = Config { col_data: 0 }; TestState { - validators, + validators: validators.into_iter().map(|(pair, _)| pair).collect(), validator_public, validator_groups, master_keystore, @@ -393,6 +405,366 @@ fn make_invalid_candidate_receipt() -> CandidateReceipt { dummy_candidate_receipt_bad_sig(Default::default(), Some(Default::default())) } +#[test] +fn too_many_unconfirmed_statements_are_considered_spam() { + sp_tracing::try_init_simple(); + test_harness(|mut test_state, mut virtual_overseer| { + Box::pin(async move { + let session = 1; + + test_state.handle_resume_sync(&mut virtual_overseer, session).await; + + let candidate_receipt1 = make_valid_candidate_receipt(); + let candidate_hash1 = candidate_receipt1.hash(); + let candidate_receipt2 = make_invalid_candidate_receipt(); + let candidate_hash2 = candidate_receipt2.hash(); + + test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; + + let valid_vote1 = + test_state.issue_statement_with_index(3, candidate_hash1, session, true).await; + + let invalid_vote1 = + test_state.issue_statement_with_index(1, candidate_hash1, session, false).await; + + let valid_vote2 = + test_state.issue_statement_with_index(3, candidate_hash1, session, true).await; + + let invalid_vote2 = + test_state.issue_statement_with_index(2, candidate_hash1, session, false).await; + + let (pending_confirmation, _confirmation_rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash: candidate_hash1, + candidate_receipt: candidate_receipt1.clone(), + session, + statements: vec![ + (valid_vote1, ValidatorIndex(3)), + (invalid_vote1, ValidatorIndex(1)), + ], + pending_confirmation, + }, + }) + .await; + + // Participation has to fail, otherwise the dispute will be confirmed. + participation_missing_availability(&mut virtual_overseer).await; + + { + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ActiveDisputes(tx), + }) + .await; + + assert_eq!(rx.await.unwrap(), vec![(session, candidate_hash1)]); + + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::QueryCandidateVotes( + vec![(session, candidate_hash1)], + tx, + ), + }) + .await; + + let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone(); + assert_eq!(votes.valid.len(), 1); + assert_eq!(votes.invalid.len(), 1); + } + + let (pending_confirmation, confirmation_rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash: candidate_hash2, + candidate_receipt: candidate_receipt2.clone(), + session, + statements: vec![ + (valid_vote2, ValidatorIndex(3)), + (invalid_vote2, ValidatorIndex(2)), + ], + pending_confirmation, + }, + }) + .await; + + { + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::QueryCandidateVotes( + vec![(session, candidate_hash2)], + tx, + ), + }) + .await; + + assert_matches!(rx.await.unwrap().get(0), None); + } + + // Result should be invalid, because it should be considered spam. + assert_matches!(confirmation_rx.await, Ok(ImportStatementsResult::InvalidImport)); + + virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; + + // No more messages expected: + assert!(virtual_overseer.try_recv().await.is_none()); + + test_state + }) + }); +} + +#[test] +fn dispute_gets_confirmed_via_participation() { + sp_tracing::try_init_simple(); + test_harness(|mut test_state, mut virtual_overseer| { + Box::pin(async move { + let session = 1; + + test_state.handle_resume_sync(&mut virtual_overseer, session).await; + + let candidate_receipt1 = make_valid_candidate_receipt(); + let candidate_hash1 = candidate_receipt1.hash(); + let candidate_receipt2 = make_invalid_candidate_receipt(); + let candidate_hash2 = candidate_receipt2.hash(); + + test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; + + let valid_vote1 = + test_state.issue_statement_with_index(3, candidate_hash1, session, true).await; + + let invalid_vote1 = + test_state.issue_statement_with_index(1, candidate_hash1, session, false).await; + + let valid_vote2 = + test_state.issue_statement_with_index(3, candidate_hash1, session, true).await; + + let invalid_vote2 = + test_state.issue_statement_with_index(2, candidate_hash1, session, false).await; + + let (pending_confirmation, _confirmation_rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash: candidate_hash1, + candidate_receipt: candidate_receipt1.clone(), + session, + statements: vec![ + (valid_vote1, ValidatorIndex(3)), + (invalid_vote1, ValidatorIndex(1)), + ], + pending_confirmation, + }, + }) + .await; + + participation_with_distribution(&mut virtual_overseer, &candidate_hash1).await; + + { + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ActiveDisputes(tx), + }) + .await; + + assert_eq!(rx.await.unwrap(), vec![(session, candidate_hash1)]); + + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::QueryCandidateVotes( + vec![(session, candidate_hash1)], + tx, + ), + }) + .await; + + let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone(); + assert_eq!(votes.valid.len(), 2); + assert_eq!(votes.invalid.len(), 1); + } + + let (pending_confirmation, confirmation_rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash: candidate_hash2, + candidate_receipt: candidate_receipt2.clone(), + session, + statements: vec![ + (valid_vote2, ValidatorIndex(3)), + (invalid_vote2, ValidatorIndex(2)), + ], + pending_confirmation, + }, + }) + .await; + + participation_missing_availability(&mut virtual_overseer).await; + + { + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::QueryCandidateVotes( + vec![(session, candidate_hash2)], + tx, + ), + }) + .await; + + let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone(); + assert_eq!(votes.valid.len(), 1); + assert_eq!(votes.invalid.len(), 1); + } + + // Result should be valid, because our node participated, so spam slots are cleared: + assert_matches!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); + + virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; + + // No more messages expected: + assert!(virtual_overseer.try_recv().await.is_none()); + + test_state + }) + }); +} + +#[test] +fn dispute_gets_confirmed_at_byzantine_threshold() { + sp_tracing::try_init_simple(); + test_harness(|mut test_state, mut virtual_overseer| { + Box::pin(async move { + let session = 1; + + test_state.handle_resume_sync(&mut virtual_overseer, session).await; + + let candidate_receipt1 = make_valid_candidate_receipt(); + let candidate_hash1 = candidate_receipt1.hash(); + let candidate_receipt2 = make_invalid_candidate_receipt(); + let candidate_hash2 = candidate_receipt2.hash(); + + test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; + + let valid_vote1 = + test_state.issue_statement_with_index(3, candidate_hash1, session, true).await; + + let invalid_vote1 = + test_state.issue_statement_with_index(1, candidate_hash1, session, false).await; + + let valid_vote1a = + test_state.issue_statement_with_index(4, candidate_hash1, session, true).await; + + let invalid_vote1a = + test_state.issue_statement_with_index(5, candidate_hash1, session, false).await; + + let valid_vote2 = + test_state.issue_statement_with_index(3, candidate_hash1, session, true).await; + + let invalid_vote2 = + test_state.issue_statement_with_index(2, candidate_hash1, session, false).await; + + let (pending_confirmation, _confirmation_rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash: candidate_hash1, + candidate_receipt: candidate_receipt1.clone(), + session, + statements: vec![ + (valid_vote1, ValidatorIndex(3)), + (invalid_vote1, ValidatorIndex(1)), + (valid_vote1a, ValidatorIndex(4)), + (invalid_vote1a, ValidatorIndex(5)), + ], + pending_confirmation, + }, + }) + .await; + + participation_missing_availability(&mut virtual_overseer).await; + + { + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ActiveDisputes(tx), + }) + .await; + + assert_eq!(rx.await.unwrap(), vec![(session, candidate_hash1)]); + + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::QueryCandidateVotes( + vec![(session, candidate_hash1)], + tx, + ), + }) + .await; + + let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone(); + assert_eq!(votes.valid.len(), 2); + assert_eq!(votes.invalid.len(), 2); + } + + let (pending_confirmation, confirmation_rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash: candidate_hash2, + candidate_receipt: candidate_receipt2.clone(), + session, + statements: vec![ + (valid_vote2, ValidatorIndex(3)), + (invalid_vote2, ValidatorIndex(2)), + ], + pending_confirmation, + }, + }) + .await; + + participation_missing_availability(&mut virtual_overseer).await; + + { + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::QueryCandidateVotes( + vec![(session, candidate_hash2)], + tx, + ), + }) + .await; + + let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone(); + assert_eq!(votes.valid.len(), 1); + assert_eq!(votes.invalid.len(), 1); + } + + // Result should be valid, because byzantine threshold has been reached in first + // import, so spam slots are cleared: + assert_matches!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); + + virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; + + // No more messages expected: + assert!(virtual_overseer.try_recv().await.is_none()); + + test_state + }) + }); +} + #[test] fn conflicting_votes_lead_to_dispute_participation() { test_harness(|mut test_state, mut virtual_overseer| { @@ -1154,6 +1526,10 @@ fn resume_dispute_without_local_statement() { test_state.issue_statement_with_index(4, candidate_hash, session, true).await; let valid_vote5 = test_state.issue_statement_with_index(5, candidate_hash, session, true).await; + let valid_vote6 = + test_state.issue_statement_with_index(6, candidate_hash, session, true).await; + let valid_vote7 = + test_state.issue_statement_with_index(7, candidate_hash, session, true).await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1167,6 +1543,8 @@ fn resume_dispute_without_local_statement() { (valid_vote3, ValidatorIndex(3)), (valid_vote4, ValidatorIndex(4)), (valid_vote5, ValidatorIndex(5)), + (valid_vote6, ValidatorIndex(6)), + (valid_vote7, ValidatorIndex(7)), ], pending_confirmation, }, @@ -1277,7 +1655,8 @@ fn resume_dispute_with_local_statement() { fn resume_dispute_without_local_statement_or_local_key() { let session = 1; let mut test_state = TestState::default(); - test_state.subsystem_keystore = make_keystore(&[Sr25519Keyring::Two]).into(); + test_state.subsystem_keystore = + make_keystore(vec![Sr25519Keyring::Two.to_seed()].into_iter()).into(); test_state .resume(|mut test_state, mut virtual_overseer| { Box::pin(async move { @@ -1411,7 +1790,8 @@ fn resume_dispute_with_local_statement_without_local_key() { }) }); // No keys: - test_state.subsystem_keystore = make_keystore(&[Sr25519Keyring::Two]).into(); + test_state.subsystem_keystore = + make_keystore(vec![Sr25519Keyring::Two.to_seed()].into_iter()).into(); // Two should not send a DisputeParticiationMessage::Participate on restart since we gave // her a non existing key. test_state.resume(|test_state, mut virtual_overseer| { From 73a9ab6e6de45c4169f46e9ce1cfda35a98eae64 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 3 Feb 2022 20:37:37 +0100 Subject: [PATCH 04/13] Make sure backing statements import works. --- .../src/real/initialized.rs | 2 +- .../dispute-coordinator/src/real/tests.rs | 375 +++++++++++++----- 2 files changed, 274 insertions(+), 103 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/initialized.rs b/node/core/dispute-coordinator/src/real/initialized.rs index b14332040c5c..4a109989fef2 100644 --- a/node/core/dispute-coordinator/src/real/initialized.rs +++ b/node/core/dispute-coordinator/src/real/initialized.rs @@ -859,7 +859,7 @@ impl Initialized { ValidDisputeStatementKind::Explicit => { is_approval_import = false; is_backing_import = false; - } + }, } insert_into_statement_vec( diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index f85045eef4bd..392fc5acdd6b 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -32,7 +32,7 @@ use futures::{ use kvdb::KeyValueDB; use parity_scale_codec::Encode; -use polkadot_node_primitives::SignedDisputeStatement; +use polkadot_node_primitives::{SignedDisputeStatement, SignedFullStatement, Statement}; use polkadot_node_subsystem::{ messages::{ ChainApiMessage, DisputeCoordinatorMessage, DisputeDistributionMessage, @@ -57,8 +57,8 @@ use polkadot_node_subsystem_test_helpers::{make_subsystem_context, TestSubsystem use polkadot_primitives::{ v1::{ BlakeTwo256, BlockNumber, CandidateCommitments, CandidateHash, CandidateReceipt, Hash, - HashT, Header, MultiDisputeStatementSet, ScrapedOnChainVotes, SessionIndex, ValidatorId, - ValidatorIndex, + HashT, Header, MultiDisputeStatementSet, ScrapedOnChainVotes, SessionIndex, SigningContext, + ValidatorId, ValidatorIndex, }, v2::SessionInfo, }; @@ -334,7 +334,7 @@ impl TestState { } } - async fn issue_statement_with_index( + async fn issue_explicit_statement_with_index( &self, index: usize, candidate_hash: CandidateHash, @@ -351,6 +351,32 @@ impl TestState { .unwrap() } + async fn issue_backing_statement_with_index( + &self, + index: usize, + candidate_hash: CandidateHash, + session: SessionIndex, + ) -> SignedDisputeStatement { + let keystore = self.master_keystore.clone() as SyncCryptoStorePtr; + let validator_id = self.validators[index].public().into(); + let context = + SigningContext { session_index: session, parent_hash: Hash::repeat_byte(0xac) }; + + let statement = SignedFullStatement::sign( + &keystore, + Statement::Valid(candidate_hash), + &context, + ValidatorIndex(index as _), + &validator_id, + ) + .await + .unwrap() + .unwrap() + .into_unchecked(); + + SignedDisputeStatement::from_backing_statement(&statement, context, validator_id).unwrap() + } + fn resume(self, test: F) -> Self where F: FnOnce(TestState, VirtualOverseer) -> BoxFuture<'static, TestState>, @@ -407,7 +433,6 @@ fn make_invalid_candidate_receipt() -> CandidateReceipt { #[test] fn too_many_unconfirmed_statements_are_considered_spam() { - sp_tracing::try_init_simple(); test_harness(|mut test_state, mut virtual_overseer| { Box::pin(async move { let session = 1; @@ -422,16 +447,18 @@ fn too_many_unconfirmed_statements_are_considered_spam() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; let valid_vote1 = - test_state.issue_statement_with_index(3, candidate_hash1, session, true).await; + test_state.issue_backing_statement_with_index(3, candidate_hash1, session).await; - let invalid_vote1 = - test_state.issue_statement_with_index(1, candidate_hash1, session, false).await; + let invalid_vote1 = test_state + .issue_explicit_statement_with_index(1, candidate_hash1, session, false) + .await; let valid_vote2 = - test_state.issue_statement_with_index(3, candidate_hash1, session, true).await; + test_state.issue_backing_statement_with_index(3, candidate_hash1, session).await; - let invalid_vote2 = - test_state.issue_statement_with_index(2, candidate_hash1, session, false).await; + let invalid_vote2 = test_state + .issue_explicit_statement_with_index(2, candidate_hash1, session, false) + .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); virtual_overseer @@ -522,7 +549,6 @@ fn too_many_unconfirmed_statements_are_considered_spam() { #[test] fn dispute_gets_confirmed_via_participation() { - sp_tracing::try_init_simple(); test_harness(|mut test_state, mut virtual_overseer| { Box::pin(async move { let session = 1; @@ -536,17 +562,21 @@ fn dispute_gets_confirmed_via_participation() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - let valid_vote1 = - test_state.issue_statement_with_index(3, candidate_hash1, session, true).await; + let valid_vote1 = test_state + .issue_explicit_statement_with_index(3, candidate_hash1, session, true) + .await; - let invalid_vote1 = - test_state.issue_statement_with_index(1, candidate_hash1, session, false).await; + let invalid_vote1 = test_state + .issue_explicit_statement_with_index(1, candidate_hash1, session, false) + .await; - let valid_vote2 = - test_state.issue_statement_with_index(3, candidate_hash1, session, true).await; + let valid_vote2 = test_state + .issue_explicit_statement_with_index(3, candidate_hash1, session, true) + .await; - let invalid_vote2 = - test_state.issue_statement_with_index(2, candidate_hash1, session, false).await; + let invalid_vote2 = test_state + .issue_explicit_statement_with_index(2, candidate_hash1, session, false) + .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); virtual_overseer @@ -640,7 +670,6 @@ fn dispute_gets_confirmed_via_participation() { #[test] fn dispute_gets_confirmed_at_byzantine_threshold() { - sp_tracing::try_init_simple(); test_harness(|mut test_state, mut virtual_overseer| { Box::pin(async move { let session = 1; @@ -654,23 +683,29 @@ fn dispute_gets_confirmed_at_byzantine_threshold() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - let valid_vote1 = - test_state.issue_statement_with_index(3, candidate_hash1, session, true).await; + let valid_vote1 = test_state + .issue_explicit_statement_with_index(3, candidate_hash1, session, true) + .await; - let invalid_vote1 = - test_state.issue_statement_with_index(1, candidate_hash1, session, false).await; + let invalid_vote1 = test_state + .issue_explicit_statement_with_index(1, candidate_hash1, session, false) + .await; - let valid_vote1a = - test_state.issue_statement_with_index(4, candidate_hash1, session, true).await; + let valid_vote1a = test_state + .issue_explicit_statement_with_index(4, candidate_hash1, session, true) + .await; - let invalid_vote1a = - test_state.issue_statement_with_index(5, candidate_hash1, session, false).await; + let invalid_vote1a = test_state + .issue_explicit_statement_with_index(5, candidate_hash1, session, false) + .await; - let valid_vote2 = - test_state.issue_statement_with_index(3, candidate_hash1, session, true).await; + let valid_vote2 = test_state + .issue_explicit_statement_with_index(3, candidate_hash1, session, true) + .await; - let invalid_vote2 = - test_state.issue_statement_with_index(2, candidate_hash1, session, false).await; + let invalid_vote2 = test_state + .issue_explicit_statement_with_index(2, candidate_hash1, session, false) + .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); virtual_overseer @@ -765,6 +800,108 @@ fn dispute_gets_confirmed_at_byzantine_threshold() { }); } +#[test] +fn backing_statements_import_works_and_no_spam() { + test_harness(|mut test_state, mut virtual_overseer| { + Box::pin(async move { + let session = 1; + + test_state.handle_resume_sync(&mut virtual_overseer, session).await; + + let candidate_receipt = make_valid_candidate_receipt(); + let candidate_hash = candidate_receipt.hash(); + + test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; + + let valid_vote1 = + test_state.issue_backing_statement_with_index(3, candidate_hash, session).await; + + let valid_vote2 = + test_state.issue_backing_statement_with_index(4, candidate_hash, session).await; + + let (pending_confirmation, confirmation_rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash, + candidate_receipt: candidate_receipt.clone(), + session, + statements: vec![ + (valid_vote1, ValidatorIndex(3)), + (valid_vote2, ValidatorIndex(4)), + ], + pending_confirmation, + }, + }) + .await; + assert_matches!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); + + { + // Just backing votes - we should not have any active disputes now. + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ActiveDisputes(tx), + }) + .await; + + assert!(rx.await.unwrap().is_empty()); + + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::QueryCandidateVotes( + vec![(session, candidate_hash)], + tx, + ), + }) + .await; + + let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone(); + assert_eq!(votes.valid.len(), 2); + assert_eq!(votes.invalid.len(), 0); + } + + let candidate_receipt = make_invalid_candidate_receipt(); + let candidate_hash = candidate_receipt.hash(); + + let valid_vote1 = + test_state.issue_backing_statement_with_index(3, candidate_hash, session).await; + + let valid_vote2 = + test_state.issue_backing_statement_with_index(4, candidate_hash, session).await; + + let (pending_confirmation, confirmation_rx) = oneshot::channel(); + // Backing vote import should not have accounted to spam slots, so this should succeed + // as well: + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash, + candidate_receipt: candidate_receipt.clone(), + session, + statements: vec![ + (valid_vote1, ValidatorIndex(3)), + (valid_vote2, ValidatorIndex(4)), + ], + pending_confirmation, + }, + }) + .await; + + // Result should be valid, because our node participated, so spam slots are cleared: + assert_matches!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); + + virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; + + // No more messages expected: + assert!(virtual_overseer.try_recv().await.is_none()); + + test_state + }) + }); +} + #[test] fn conflicting_votes_lead_to_dispute_participation() { test_harness(|mut test_state, mut virtual_overseer| { @@ -778,14 +915,17 @@ fn conflicting_votes_lead_to_dispute_participation() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - let valid_vote = - test_state.issue_statement_with_index(3, candidate_hash, session, true).await; + let valid_vote = test_state + .issue_explicit_statement_with_index(3, candidate_hash, session, true) + .await; - let invalid_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, false).await; + let invalid_vote = test_state + .issue_explicit_statement_with_index(1, candidate_hash, session, false) + .await; - let invalid_vote_2 = - test_state.issue_statement_with_index(2, candidate_hash, session, false).await; + let invalid_vote_2 = test_state + .issue_explicit_statement_with_index(2, candidate_hash, session, false) + .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); virtual_overseer @@ -882,11 +1022,13 @@ fn positive_votes_dont_trigger_participation() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - let valid_vote = - test_state.issue_statement_with_index(2, candidate_hash, session, true).await; + let valid_vote = test_state + .issue_explicit_statement_with_index(2, candidate_hash, session, true) + .await; - let valid_vote_2 = - test_state.issue_statement_with_index(1, candidate_hash, session, true).await; + let valid_vote_2 = test_state + .issue_explicit_statement_with_index(1, candidate_hash, session, true) + .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); virtual_overseer @@ -987,11 +1129,13 @@ fn wrong_validator_index_is_ignored() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - let valid_vote = - test_state.issue_statement_with_index(2, candidate_hash, session, true).await; + let valid_vote = test_state + .issue_explicit_statement_with_index(2, candidate_hash, session, true) + .await; - let invalid_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, false).await; + let invalid_vote = test_state + .issue_explicit_statement_with_index(1, candidate_hash, session, false) + .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1057,11 +1201,13 @@ fn finality_votes_ignore_disputed_candidates() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - let valid_vote = - test_state.issue_statement_with_index(2, candidate_hash, session, true).await; + let valid_vote = test_state + .issue_explicit_statement_with_index(2, candidate_hash, session, true) + .await; - let invalid_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, false).await; + let invalid_vote = test_state + .issue_explicit_statement_with_index(1, candidate_hash, session, false) + .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1153,11 +1299,13 @@ fn supermajority_valid_dispute_may_be_finalized() { let supermajority_threshold = polkadot_primitives::v1::supermajority_threshold(test_state.validators.len()); - let valid_vote = - test_state.issue_statement_with_index(2, candidate_hash, session, true).await; + let valid_vote = test_state + .issue_explicit_statement_with_index(2, candidate_hash, session, true) + .await; - let invalid_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, false).await; + let invalid_vote = test_state + .issue_explicit_statement_with_index(1, candidate_hash, session, false) + .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1179,8 +1327,9 @@ fn supermajority_valid_dispute_may_be_finalized() { let mut statements = Vec::new(); for i in (0..supermajority_threshold - 1).map(|i| i + 3) { - let vote = - test_state.issue_statement_with_index(i, candidate_hash, session, true).await; + let vote = test_state + .issue_explicit_statement_with_index(i, candidate_hash, session, true) + .await; statements.push((vote, ValidatorIndex(i as _))); } @@ -1270,11 +1419,13 @@ fn concluded_supermajority_for_non_active_after_time() { let supermajority_threshold = polkadot_primitives::v1::supermajority_threshold(test_state.validators.len()); - let valid_vote = - test_state.issue_statement_with_index(2, candidate_hash, session, true).await; + let valid_vote = test_state + .issue_explicit_statement_with_index(2, candidate_hash, session, true) + .await; - let invalid_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, false).await; + let invalid_vote = test_state + .issue_explicit_statement_with_index(1, candidate_hash, session, false) + .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1297,8 +1448,9 @@ fn concluded_supermajority_for_non_active_after_time() { let mut statements = Vec::new(); // -2: 1 for already imported vote and one for local vote (which is valid). for i in (0..supermajority_threshold - 2).map(|i| i + 3) { - let vote = - test_state.issue_statement_with_index(i, candidate_hash, session, true).await; + let vote = test_state + .issue_explicit_statement_with_index(i, candidate_hash, session, true) + .await; statements.push((vote, ValidatorIndex(i as _))); } @@ -1365,11 +1517,13 @@ fn concluded_supermajority_against_non_active_after_time() { let supermajority_threshold = polkadot_primitives::v1::supermajority_threshold(test_state.validators.len()); - let valid_vote = - test_state.issue_statement_with_index(2, candidate_hash, session, true).await; + let valid_vote = test_state + .issue_explicit_statement_with_index(2, candidate_hash, session, true) + .await; - let invalid_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, false).await; + let invalid_vote = test_state + .issue_explicit_statement_with_index(1, candidate_hash, session, false) + .await; let (pending_confirmation, confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1395,8 +1549,9 @@ fn concluded_supermajority_against_non_active_after_time() { let mut statements = Vec::new(); // minus 2, because of local vote and one previously imported invalid vote. for i in (0..supermajority_threshold - 2).map(|i| i + 3) { - let vote = - test_state.issue_statement_with_index(i, candidate_hash, session, false).await; + let vote = test_state + .issue_explicit_statement_with_index(i, candidate_hash, session, false) + .await; statements.push((vote, ValidatorIndex(i as _))); } @@ -1462,11 +1617,13 @@ fn resume_dispute_without_local_statement() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - let valid_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, true).await; + let valid_vote = test_state + .issue_explicit_statement_with_index(1, candidate_hash, session, true) + .await; - let invalid_vote = - test_state.issue_statement_with_index(2, candidate_hash, session, false).await; + let invalid_vote = test_state + .issue_explicit_statement_with_index(2, candidate_hash, session, false) + .await; let (pending_confirmation, confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1518,18 +1675,24 @@ fn resume_dispute_without_local_statement() { participation_with_distribution(&mut virtual_overseer, &candidate_hash).await; - let valid_vote0 = - test_state.issue_statement_with_index(0, candidate_hash, session, true).await; - let valid_vote3 = - test_state.issue_statement_with_index(3, candidate_hash, session, true).await; - let valid_vote4 = - test_state.issue_statement_with_index(4, candidate_hash, session, true).await; - let valid_vote5 = - test_state.issue_statement_with_index(5, candidate_hash, session, true).await; - let valid_vote6 = - test_state.issue_statement_with_index(6, candidate_hash, session, true).await; - let valid_vote7 = - test_state.issue_statement_with_index(7, candidate_hash, session, true).await; + let valid_vote0 = test_state + .issue_explicit_statement_with_index(0, candidate_hash, session, true) + .await; + let valid_vote3 = test_state + .issue_explicit_statement_with_index(3, candidate_hash, session, true) + .await; + let valid_vote4 = test_state + .issue_explicit_statement_with_index(4, candidate_hash, session, true) + .await; + let valid_vote5 = test_state + .issue_explicit_statement_with_index(5, candidate_hash, session, true) + .await; + let valid_vote6 = test_state + .issue_explicit_statement_with_index(6, candidate_hash, session, true) + .await; + let valid_vote7 = test_state + .issue_explicit_statement_with_index(7, candidate_hash, session, true) + .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1588,14 +1751,17 @@ fn resume_dispute_with_local_statement() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - let local_valid_vote = - test_state.issue_statement_with_index(0, candidate_hash, session, true).await; + let local_valid_vote = test_state + .issue_explicit_statement_with_index(0, candidate_hash, session, true) + .await; - let valid_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, true).await; + let valid_vote = test_state + .issue_explicit_statement_with_index(1, candidate_hash, session, true) + .await; - let invalid_vote = - test_state.issue_statement_with_index(2, candidate_hash, session, false).await; + let invalid_vote = test_state + .issue_explicit_statement_with_index(2, candidate_hash, session, false) + .await; let (pending_confirmation, confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1667,11 +1833,13 @@ fn resume_dispute_without_local_statement_or_local_key() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - let valid_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, true).await; + let valid_vote = test_state + .issue_explicit_statement_with_index(1, candidate_hash, session, true) + .await; - let invalid_vote = - test_state.issue_statement_with_index(2, candidate_hash, session, false).await; + let invalid_vote = test_state + .issue_explicit_statement_with_index(2, candidate_hash, session, false) + .await; let (pending_confirmation, confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1743,14 +1911,17 @@ fn resume_dispute_with_local_statement_without_local_key() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - let local_valid_vote = - test_state.issue_statement_with_index(0, candidate_hash, session, true).await; + let local_valid_vote = test_state + .issue_explicit_statement_with_index(0, candidate_hash, session, true) + .await; - let valid_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, true).await; + let valid_vote = test_state + .issue_explicit_statement_with_index(1, candidate_hash, session, true) + .await; - let invalid_vote = - test_state.issue_statement_with_index(2, candidate_hash, session, false).await; + let invalid_vote = test_state + .issue_explicit_statement_with_index(2, candidate_hash, session, false) + .await; let (pending_confirmation, confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1833,7 +2004,7 @@ fn issue_local_statement_does_cause_distribution_but_not_duplicate_participation test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; let other_vote = test_state - .issue_statement_with_index(1, candidate_hash, session, !validity) + .issue_explicit_statement_with_index(1, candidate_hash, session, !validity) .await; let (pending_confirmation, confirmation_rx) = oneshot::channel(); From b45d982c70d62b25bb6b87b24521647d29c209b3 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 3 Feb 2022 21:20:10 +0100 Subject: [PATCH 05/13] Aaaaaand the actual proper fix. --- .../src/real/initialized.rs | 37 +++++-------------- .../src/real/spam_slots.rs | 19 ++++++---- .../dispute-coordinator/src/real/tests.rs | 12 +++--- 3 files changed, 28 insertions(+), 40 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/initialized.rs b/node/core/dispute-coordinator/src/real/initialized.rs index 4a109989fef2..2ec925e43808 100644 --- a/node/core/dispute-coordinator/src/real/initialized.rs +++ b/node/core/dispute-coordinator/src/real/initialized.rs @@ -822,15 +822,6 @@ impl Initialized { .find(|(_, index)| controlled_indices.contains(index)) .is_some(); - // Whether or not the import is coming from approval checking/backing. - // NOTE: Those imports should provide a completely separate API from importing untrusted - // dispute votes, not treating them the same would do away with much of the complexity here - // and would also improve performance. - // - // See https://github.com/paritytech/polkadot/issues/4793 - let mut is_backing_import = true; - let mut is_approval_import = true; - // Update candidate votes. for (statement, val_index) in &statements { if validators @@ -852,16 +843,6 @@ impl Initialized { DisputeStatement::Valid(valid_kind) => { self.metrics.on_valid_vote(); - match valid_kind { - ValidDisputeStatementKind::BackingSeconded(_) => is_approval_import = false, - ValidDisputeStatementKind::BackingValid(_) => is_approval_import = false, - ValidDisputeStatementKind::ApprovalChecking => is_backing_import = false, - ValidDisputeStatementKind::Explicit => { - is_approval_import = false; - is_backing_import = false; - }, - } - insert_into_statement_vec( &mut votes.valid, valid_kind, @@ -872,9 +853,6 @@ impl Initialized { DisputeStatement::Invalid(invalid_kind) => { self.metrics.on_invalid_vote(); - is_approval_import = false; - is_backing_import = false; - insert_into_statement_vec( &mut votes.invalid, invalid_kind, @@ -894,13 +872,18 @@ impl Initialized { byzantine_threshold(n_validators); // Potential spam: - if !is_confirmed && !is_backing_import && !is_approval_import { + if !is_confirmed { let mut free_spam_slots = true; - // Only allow import if all validators of import have not exceeded their spam slots: - for (_, index) in statements.iter() { - free_spam_slots &= self.spam_slots.add_unconfirmed(session, candidate_hash, *index); + // Only allow import if all validators voting invalid have not exceeded their spam slots: + for (statement, index) in statements.iter() { + // Disputes can only be triggered via an invalidity stating vote, thus we only + // need to increase spam slots on invalid votes. (If we did not, we would also + // increase spam slots for backing valdiators for example - as validators have to + // provide some opposing vote for dispute-distribution). + free_spam_slots &= statement.statement().indicates_validity() || + self.spam_slots.add_unconfirmed(session, candidate_hash, *index); } - // No reporting validator had a free spam slot: + // Only validity stating votes or validator had free spam slot? if !free_spam_slots { tracing::debug!( target: LOG_TARGET, diff --git a/node/core/dispute-coordinator/src/real/spam_slots.rs b/node/core/dispute-coordinator/src/real/spam_slots.rs index cc10404fbd4f..44e3671edf88 100644 --- a/node/core/dispute-coordinator/src/real/spam_slots.rs +++ b/node/core/dispute-coordinator/src/real/spam_slots.rs @@ -23,18 +23,20 @@ use crate::real::LOG_TARGET; /// Type used for counting potential spam votes. type SpamCount = u32; -/// How many unconfirmed disputes a validator is allowed to be a participant in (per session). +/// How many unconfirmed disputes a validator is allowed to import (per session). /// /// Unconfirmed means: Node has not seen the candidate be included on any chain, it has not cast a /// vote itself on that dispute, the dispute has not yet reached more than a third of /// validator's votes and the including relay chain block has not yet been finalized. /// /// Exact number of `MAX_SPAM_VOTES` is not that important here. It is important that the number is -/// low enough to not cause resource exhaustion, if multiple validators spend their limits. Also -/// if things are working properly, this number cannot really be too low either, as all relevant -/// disputes _should_ have been seen as included my enough validators. (Otherwise the candidate -/// would not have been available in the first place and could not have been included.) So this is -/// really just a fallback mechanism if things go terribly wrong. +/// low enough to not cause resource exhaustion (disk & memory) on the importing validator, even if +/// multiple validators fully make use of their assigned spam slots. +/// +/// Also if things are working properly, this number cannot really be too low either, as all +/// relevant disputes _should_ have been seen as included my enough validators. (Otherwise the +/// candidate would not have been available in the first place and could not have been included.) +/// So this is really just a fallback mechanism if things go terribly wrong. #[cfg(not(test))] const MAX_SPAM_VOTES: SpamCount = 50; #[cfg(test)] @@ -79,7 +81,10 @@ impl SpamSlots { Self { slots, unconfirmed: unconfirmed_disputes } } - /// Add an unconfirmed dispute if free slots are available. + /// Increase a "voting invalid" validator's spam slot. + /// + /// This function should get called for any validator's invalidity vote for any not yet + /// confirmed dispute. pub fn add_unconfirmed( &mut self, session: SessionIndex, diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index 392fc5acdd6b..9a41d15f9946 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -457,7 +457,7 @@ fn too_many_unconfirmed_statements_are_considered_spam() { test_state.issue_backing_statement_with_index(3, candidate_hash1, session).await; let invalid_vote2 = test_state - .issue_explicit_statement_with_index(2, candidate_hash1, session, false) + .issue_explicit_statement_with_index(1, candidate_hash1, session, false) .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); @@ -513,7 +513,7 @@ fn too_many_unconfirmed_statements_are_considered_spam() { session, statements: vec![ (valid_vote2, ValidatorIndex(3)), - (invalid_vote2, ValidatorIndex(2)), + (invalid_vote2, ValidatorIndex(1)), ], pending_confirmation, }, @@ -575,7 +575,7 @@ fn dispute_gets_confirmed_via_participation() { .await; let invalid_vote2 = test_state - .issue_explicit_statement_with_index(2, candidate_hash1, session, false) + .issue_explicit_statement_with_index(1, candidate_hash1, session, false) .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); @@ -630,7 +630,7 @@ fn dispute_gets_confirmed_via_participation() { session, statements: vec![ (valid_vote2, ValidatorIndex(3)), - (invalid_vote2, ValidatorIndex(2)), + (invalid_vote2, ValidatorIndex(1)), ], pending_confirmation, }, @@ -704,7 +704,7 @@ fn dispute_gets_confirmed_at_byzantine_threshold() { .await; let invalid_vote2 = test_state - .issue_explicit_statement_with_index(2, candidate_hash1, session, false) + .issue_explicit_statement_with_index(1, candidate_hash1, session, false) .await; let (pending_confirmation, _confirmation_rx) = oneshot::channel(); @@ -761,7 +761,7 @@ fn dispute_gets_confirmed_at_byzantine_threshold() { session, statements: vec![ (valid_vote2, ValidatorIndex(3)), - (invalid_vote2, ValidatorIndex(2)), + (invalid_vote2, ValidatorIndex(1)), ], pending_confirmation, }, From 22830e407a765318ade15f2a66fbe5501680af3b Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 3 Feb 2022 21:32:59 +0100 Subject: [PATCH 06/13] Better docs + spelling fixes. --- node/core/dispute-coordinator/src/real/spam_slots.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/spam_slots.rs b/node/core/dispute-coordinator/src/real/spam_slots.rs index 44e3671edf88..6d6a259b2ffc 100644 --- a/node/core/dispute-coordinator/src/real/spam_slots.rs +++ b/node/core/dispute-coordinator/src/real/spam_slots.rs @@ -97,21 +97,17 @@ impl SpamSlots { } let validators = self.unconfirmed.entry((session, candidate)).or_default(); - // We don't want to increase spam count for multiple votes on the same candidate - see - // below. - // This kind of spam does not do much harm and should be limited enough already by - // networking (dispute-distribution). if validators.insert(validator) { + // We only increment spam slots once per candidate, as each validator has to provide an + // opposing vote for sending out its own vote. Therefore, receiving multiple votes for + // a single candidate is expected and should not get punished here. *c += 1; } - // Always true even if validator already voted for that candidate. This is necessary, because - // when providing a dispute vote, a validator has to provide an opposing vote - multiple - // validators might use the same opposing vote. true } - /// Clear out spam slots for a given candiate in a session. + /// Clear out spam slots for a given candidate in a session. /// /// This effectively reduces the spam slot count for all validators participating in a dispute /// for that candidate. You should call this function once a dispute became obsolete or got From 48b75be020c977a368c5a3c2d7fbc92ca468212b Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 3 Feb 2022 21:35:08 +0100 Subject: [PATCH 07/13] Fix. --- node/core/dispute-coordinator/src/real/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index 9a41d15f9946..fad00c69c994 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -78,7 +78,7 @@ use super::db::v1::DbBackend; const TEST_TIMEOUT: Duration = Duration::from_secs(2); // sets up a keystore with the given keyring accounts. -fn make_keystore<'a>(seeds: impl Iterator) -> LocalKeystore { +fn make_keystore(seeds: impl Iterator) -> LocalKeystore { let store = LocalKeystore::in_memory(); for s in seeds { From b4c5d89f5bfed6d58b0c7e0f00a0bf790da0ad51 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Fri, 4 Feb 2022 06:35:17 +0100 Subject: [PATCH 08/13] Typo Co-authored-by: Robert Habermeier --- node/core/dispute-coordinator/src/real/spam_slots.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/dispute-coordinator/src/real/spam_slots.rs b/node/core/dispute-coordinator/src/real/spam_slots.rs index 6d6a259b2ffc..d34827e6bc42 100644 --- a/node/core/dispute-coordinator/src/real/spam_slots.rs +++ b/node/core/dispute-coordinator/src/real/spam_slots.rs @@ -34,7 +34,7 @@ type SpamCount = u32; /// multiple validators fully make use of their assigned spam slots. /// /// Also if things are working properly, this number cannot really be too low either, as all -/// relevant disputes _should_ have been seen as included my enough validators. (Otherwise the +/// relevant disputes _should_ have been seen as included by enough validators. (Otherwise the /// candidate would not have been available in the first place and could not have been included.) /// So this is really just a fallback mechanism if things go terribly wrong. #[cfg(not(test))] From 2d73fd17c9eb3e017a8b2be59454c4b17ef0f9d4 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Fri, 4 Feb 2022 06:45:40 +0100 Subject: [PATCH 09/13] Fix doc. --- node/core/dispute-coordinator/src/real/spam_slots.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/node/core/dispute-coordinator/src/real/spam_slots.rs b/node/core/dispute-coordinator/src/real/spam_slots.rs index d34827e6bc42..3739bcd91bac 100644 --- a/node/core/dispute-coordinator/src/real/spam_slots.rs +++ b/node/core/dispute-coordinator/src/real/spam_slots.rs @@ -85,6 +85,8 @@ impl SpamSlots { /// /// This function should get called for any validator's invalidity vote for any not yet /// confirmed dispute. + /// + /// Returns: true if validator still had free spam slots, false otherwise. pub fn add_unconfirmed( &mut self, session: SessionIndex, From 60538c021ed4bf6f1f4b1f147fa5f3bf894b9325 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Mon, 7 Feb 2022 11:53:37 +0100 Subject: [PATCH 10/13] Update node/core/dispute-coordinator/src/real/initialized.rs --- node/core/dispute-coordinator/src/real/initialized.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/dispute-coordinator/src/real/initialized.rs b/node/core/dispute-coordinator/src/real/initialized.rs index 2ec925e43808..11c6a8d9901f 100644 --- a/node/core/dispute-coordinator/src/real/initialized.rs +++ b/node/core/dispute-coordinator/src/real/initialized.rs @@ -878,7 +878,7 @@ impl Initialized { for (statement, index) in statements.iter() { // Disputes can only be triggered via an invalidity stating vote, thus we only // need to increase spam slots on invalid votes. (If we did not, we would also - // increase spam slots for backing valdiators for example - as validators have to + // increase spam slots for backing validators for example - as validators have to // provide some opposing vote for dispute-distribution). free_spam_slots &= statement.statement().indicates_validity() || self.spam_slots.add_unconfirmed(session, candidate_hash, *index); From eae951f70d9d6c6650d168eb6227dc3170adf9e4 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Mon, 7 Feb 2022 18:53:39 +0100 Subject: [PATCH 11/13] minor rewording, line wrap --- .../core/dispute-coordinator/src/real/initialized.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/initialized.rs b/node/core/dispute-coordinator/src/real/initialized.rs index 11c6a8d9901f..096689507759 100644 --- a/node/core/dispute-coordinator/src/real/initialized.rs +++ b/node/core/dispute-coordinator/src/real/initialized.rs @@ -865,7 +865,8 @@ impl Initialized { // Whether or not we know already that this is a good dispute: // - // Note we can only know for sure whether we reached the `byzantine_threshold` after updating candidate votes above, therefore the spam checking is afterwards: + // Note we can only know for sure whether we reached the `byzantine_threshold` after + // updating candidate votes above, therefore the spam checking is afterwards: let is_confirmed = is_included || was_confirmed || is_local || votes.voted_indices().len() > @@ -873,18 +874,19 @@ impl Initialized { // Potential spam: if !is_confirmed { - let mut free_spam_slots = true; - // Only allow import if all validators voting invalid have not exceeded their spam slots: + let mut free_spam_slots_available = true; + // Only allow import if there is at least one validator voting invalid, that didn't exceed + // its spam slot: for (statement, index) in statements.iter() { // Disputes can only be triggered via an invalidity stating vote, thus we only // need to increase spam slots on invalid votes. (If we did not, we would also // increase spam slots for backing validators for example - as validators have to // provide some opposing vote for dispute-distribution). - free_spam_slots &= statement.statement().indicates_validity() || + free_spam_slots_available &= statement.statement().indicates_validity() || self.spam_slots.add_unconfirmed(session, candidate_hash, *index); } // Only validity stating votes or validator had free spam slot? - if !free_spam_slots { + if !free_spam_slots_available { tracing::debug!( target: LOG_TARGET, ?candidate_hash, From e4446926a7933d66007dbc732de4d63e504a8f7d Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 8 Feb 2022 19:15:12 +0100 Subject: [PATCH 12/13] fix test case --- node/core/dispute-coordinator/src/real/initialized.rs | 4 +++- node/core/dispute-coordinator/src/real/spam_slots.rs | 2 +- node/core/dispute-coordinator/src/real/tests.rs | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/initialized.rs b/node/core/dispute-coordinator/src/real/initialized.rs index 68d3492762e1..281b0f1eca9b 100644 --- a/node/core/dispute-coordinator/src/real/initialized.rs +++ b/node/core/dispute-coordinator/src/real/initialized.rs @@ -244,8 +244,10 @@ impl Initialized { if !overlay_db.is_empty() { let ops = overlay_db.into_write_ops(); backend.write(ops)?; - confirm_write()?; } + // even if the changeset was empty, + // otherwise the caller will error. + confirm_write()?; } } diff --git a/node/core/dispute-coordinator/src/real/spam_slots.rs b/node/core/dispute-coordinator/src/real/spam_slots.rs index 3739bcd91bac..b58b812b042a 100644 --- a/node/core/dispute-coordinator/src/real/spam_slots.rs +++ b/node/core/dispute-coordinator/src/real/spam_slots.rs @@ -86,7 +86,7 @@ impl SpamSlots { /// This function should get called for any validator's invalidity vote for any not yet /// confirmed dispute. /// - /// Returns: true if validator still had free spam slots, false otherwise. + /// Returns: `true` if validator still had vacant spam slots, `false` otherwise. pub fn add_unconfirmed( &mut self, session: SessionIndex, diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index 8b65a9b89cd9..9d031a70b22f 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -2157,10 +2157,10 @@ fn redundant_votes_ignored() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; let valid_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, true).await; + test_state.issue_backing_statement_with_index(1, candidate_hash, session).await; let valid_vote_2 = - test_state.issue_statement_with_index(1, candidate_hash, session, true).await; + test_state.issue_backing_statement_with_index(1, candidate_hash, session).await; assert!(valid_vote.validator_signature() != valid_vote_2.validator_signature()); From 97db9e02a9f5f76e39e11f1f815d2e3047421dc1 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Mon, 14 Feb 2022 15:11:20 +0100 Subject: [PATCH 13/13] Fix obsolete comment. --- node/core/dispute-coordinator/src/real/initialized.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/initialized.rs b/node/core/dispute-coordinator/src/real/initialized.rs index 281b0f1eca9b..d9d71c67e9bb 100644 --- a/node/core/dispute-coordinator/src/real/initialized.rs +++ b/node/core/dispute-coordinator/src/real/initialized.rs @@ -894,8 +894,8 @@ impl Initialized { // Potential spam: if !is_confirmed { let mut free_spam_slots_available = true; - // Only allow import if there is at least one validator voting invalid, that didn't exceed - // its spam slot: + // Only allow import if all validators voting invalid, have not exceeded + // their spam slots: for (statement, index) in statements.iter() { // Disputes can only be triggered via an invalidity stating vote, thus we only // need to increase spam slots on invalid votes. (If we did not, we would also