From fbacd04c7be2c081a6aaacdf60755121238099ba Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Sun, 6 Feb 2022 14:31:23 -0600 Subject: [PATCH 1/4] ignore duplicate dispute votes --- .../src/real/initialized.rs | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/initialized.rs b/node/core/dispute-coordinator/src/real/initialized.rs index 4e4a8d0298ae..3ffe5d8cc4f8 100644 --- a/node/core/dispute-coordinator/src/real/initialized.rs +++ b/node/core/dispute-coordinator/src/real/initialized.rs @@ -777,14 +777,17 @@ impl Initialized { // There is one exception: A sufficiently sophisticated attacker could prevent // us from seeing the backing votes by witholding arbitrary blocks, and hence we do // not have a `CandidateReceipt` available. - let mut votes = match overlay_db + let (mut votes, mut votes_changed) = match overlay_db .load_candidate_votes(session, &candidate_hash)? .map(CandidateVotes::from) { - Some(votes) => votes, + Some(votes) => (votes, false), None => if let MaybeCandidateReceipt::Provides(candidate_receipt) = candidate_receipt { - CandidateVotes { candidate_receipt, valid: Vec::new(), invalid: Vec::new() } + ( + CandidateVotes { candidate_receipt, valid: Vec::new(), invalid: Vec::new() }, + true, + ) } else { tracing::warn!( target: LOG_TARGET, @@ -839,22 +842,30 @@ impl Initialized { match statement.statement().clone() { DisputeStatement::Valid(valid_kind) => { - self.metrics.on_valid_vote(); - insert_into_statement_vec( + let fresh = insert_into_statement_vec( &mut votes.valid, valid_kind, *val_index, statement.validator_signature().clone(), ); + + if !fresh { continue } + + votes_changed = true; + self.metrics.on_valid_vote(); }, DisputeStatement::Invalid(invalid_kind) => { - self.metrics.on_invalid_vote(); - insert_into_statement_vec( + let fresh = insert_into_statement_vec( &mut votes.invalid, invalid_kind, *val_index, statement.validator_signature().clone(), ); + + if !fresh { continue } + + votes_changed = true; + self.metrics.on_invalid_vote(); }, } } @@ -986,7 +997,10 @@ impl Initialized { overlay_db.write_recent_disputes(recent_disputes); } - overlay_db.write_candidate_votes(session, candidate_hash, votes.into()); + // Only write when votes have changed. + if votes_changed { + overlay_db.write_candidate_votes(session, candidate_hash, votes.into()); + } Ok(ImportStatementsResult::ValidImport) } @@ -1143,18 +1157,21 @@ impl MuxedMessage { } } +// Returns 'true' if no other vote by that validator was already +// present and 'false' otherwise. Same semantics as `HashSet`. fn insert_into_statement_vec( vec: &mut Vec<(T, ValidatorIndex, ValidatorSignature)>, tag: T, val_index: ValidatorIndex, val_signature: ValidatorSignature, -) { +) -> bool { let pos = match vec.binary_search_by_key(&val_index, |x| x.1) { - Ok(_) => return, // no duplicates needed. + Ok(_) => return false, // no duplicates needed. Err(p) => p, }; vec.insert(pos, (tag, val_index, val_signature)); + true } #[derive(Debug, Clone)] From f929676e16f44839043966b412f30cf86dd900a8 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Sun, 6 Feb 2022 14:32:32 -0600 Subject: [PATCH 2/4] fmt and TODOs --- .../dispute-coordinator/src/real/initialized.rs | 14 +++++++++++--- node/core/dispute-coordinator/src/real/tests.rs | 4 ++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/initialized.rs b/node/core/dispute-coordinator/src/real/initialized.rs index 3ffe5d8cc4f8..f17146887af3 100644 --- a/node/core/dispute-coordinator/src/real/initialized.rs +++ b/node/core/dispute-coordinator/src/real/initialized.rs @@ -785,7 +785,11 @@ impl Initialized { None => if let MaybeCandidateReceipt::Provides(candidate_receipt) = candidate_receipt { ( - CandidateVotes { candidate_receipt, valid: Vec::new(), invalid: Vec::new() }, + CandidateVotes { + candidate_receipt, + valid: Vec::new(), + invalid: Vec::new(), + }, true, ) } else { @@ -849,7 +853,9 @@ impl Initialized { statement.validator_signature().clone(), ); - if !fresh { continue } + if !fresh { + continue + } votes_changed = true; self.metrics.on_valid_vote(); @@ -862,7 +868,9 @@ impl Initialized { statement.validator_signature().clone(), ); - if !fresh { continue } + if !fresh { + continue + } votes_changed = true; self.metrics.on_invalid_vote(); diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index f3ff8a260aa5..0b2fc301daef 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -1548,3 +1548,7 @@ fn negative_issue_local_statement_only_triggers_import() { }) }); } + +// TODO [now]: redundant votes aren't written to DB + +// TODO [now]: redundant votes aren't counted in metrics From ea0f900df92ae1a0a25f4f16a19c93c2674678e3 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Sun, 6 Feb 2022 15:02:30 -0600 Subject: [PATCH 3/4] tests --- .../src/real/initialized.rs | 2 +- .../dispute-coordinator/src/real/tests.rs | 114 +++++++++++++++++- 2 files changed, 113 insertions(+), 3 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/initialized.rs b/node/core/dispute-coordinator/src/real/initialized.rs index f17146887af3..67d44511f6ec 100644 --- a/node/core/dispute-coordinator/src/real/initialized.rs +++ b/node/core/dispute-coordinator/src/real/initialized.rs @@ -888,7 +888,7 @@ impl Initialized { // Potential spam: if !is_confirmed { - let mut free_spam_slots = false; + let mut free_spam_slots = statements.is_empty(); for (statement, index) in statements.iter() { free_spam_slots |= statement.statement().is_backing() || self.spam_slots.add_unconfirmed(session, candidate_hash, *index); diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index 0b2fc301daef..53a16601e929 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -1549,6 +1549,116 @@ fn negative_issue_local_statement_only_triggers_import() { }); } -// TODO [now]: redundant votes aren't written to DB +#[test] +fn empty_import_still_writes_candidate_receipt() { + 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 (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash, + candidate_receipt: candidate_receipt.clone(), + session, + statements: Vec::new(), + pending_confirmation: tx, + } + }) + .await; + + rx.await.unwrap(); + + let backend = DbBackend::new(test_state.db.clone(), test_state.config.column_config()); + + let votes = backend.load_candidate_votes(session, &candidate_hash).unwrap().unwrap(); + assert_eq!(votes.invalid.len(), 0); + assert_eq!(votes.valid.len(), 0); + assert_eq!(votes.candidate_receipt, candidate_receipt); + + virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; + assert!(virtual_overseer.try_recv().await.is_none()); + + test_state + }) + }); +} + + +#[test] +fn redundant_votes_ignored() { + 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_vote = + test_state.issue_statement_with_index(1, candidate_hash, session, true).await; + + let valid_vote_2 = + test_state.issue_statement_with_index(1, candidate_hash, session, true).await; + + assert!(valid_vote.validator_signature() != valid_vote_2.validator_signature()); + + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash, + candidate_receipt: candidate_receipt.clone(), + session, + statements: vec![ + (valid_vote.clone(), ValidatorIndex(1)) + ], + pending_confirmation: tx, + } + }) + .await; + + rx.await.unwrap(); + + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash, + candidate_receipt: candidate_receipt.clone(), + session, + statements: vec![ + (valid_vote_2, ValidatorIndex(1)) + ], + pending_confirmation: tx, + } + }) + .await; + + rx.await.unwrap(); -// TODO [now]: redundant votes aren't counted in metrics + let backend = DbBackend::new(test_state.db.clone(), test_state.config.column_config()); + + let votes = backend.load_candidate_votes(session, &candidate_hash).unwrap().unwrap(); + assert_eq!(votes.invalid.len(), 0); + assert_eq!(votes.valid.len(), 1); + assert_eq!(&votes.valid[0].2, valid_vote.validator_signature()); + + virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; + assert!(virtual_overseer.try_recv().await.is_none()); + + test_state + }) + }); +} From 2ae67c8da57c332e8e392986e6af233eeb9dd66f Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Sun, 6 Feb 2022 15:19:26 -0600 Subject: [PATCH 4/4] fmt --- node/core/dispute-coordinator/src/real/tests.rs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index 53a16601e929..aed361f5886f 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -1571,7 +1571,7 @@ fn empty_import_still_writes_candidate_receipt() { session, statements: Vec::new(), pending_confirmation: tx, - } + }, }) .await; @@ -1592,7 +1592,6 @@ fn empty_import_still_writes_candidate_receipt() { }); } - #[test] fn redundant_votes_ignored() { test_harness(|mut test_state, mut virtual_overseer| { @@ -1607,7 +1606,7 @@ 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_statement_with_index(1, candidate_hash, session, true).await; let valid_vote_2 = test_state.issue_statement_with_index(1, candidate_hash, session, true).await; @@ -1621,11 +1620,9 @@ fn redundant_votes_ignored() { candidate_hash, candidate_receipt: candidate_receipt.clone(), session, - statements: vec![ - (valid_vote.clone(), ValidatorIndex(1)) - ], + statements: vec![(valid_vote.clone(), ValidatorIndex(1))], pending_confirmation: tx, - } + }, }) .await; @@ -1638,11 +1635,9 @@ fn redundant_votes_ignored() { candidate_hash, candidate_receipt: candidate_receipt.clone(), session, - statements: vec![ - (valid_vote_2, ValidatorIndex(1)) - ], + statements: vec![(valid_vote_2, ValidatorIndex(1))], pending_confirmation: tx, - } + }, }) .await;