diff --git a/polkadot/node/core/approval-voting/src/approval_checking.rs b/polkadot/node/core/approval-voting/src/approval_checking.rs index d8384f88d4..4bdf3ffb6e 100644 --- a/polkadot/node/core/approval-voting/src/approval_checking.rs +++ b/polkadot/node/core/approval-voting/src/approval_checking.rs @@ -66,6 +66,8 @@ pub enum Check { Unapproved, /// The candidate is approved, with the given amount of no-shows. Approved(usize), + /// The candidate is approved by one third of all validators. + ApprovedOneThird, } impl Check { @@ -74,6 +76,15 @@ impl Check { match *self { Check::Unapproved => false, Check::Approved(_) => true, + Check::ApprovedOneThird => true, + } + } + + /// The number of known no-shows in this computation. + pub fn known_no_shows(&self) -> usize { + match *self { + Check::Approved(n) => n, + _ => 0, } } } @@ -84,16 +95,17 @@ pub fn check_approval( approval: &ApprovalEntry, required: RequiredTranches, ) -> Check { + + // any set of size f+1 contains at least one honest node. If at least one + // honest node approves, the candidate should be approved. + let approvals = candidate.approvals(); + if 3 * approvals.count_ones() > approvals.len() { + return Check::ApprovedOneThird; + } + match required { RequiredTranches::Pending { .. } => Check::Unapproved, - RequiredTranches::All => { - let approvals = candidate.approvals(); - if 3 * approvals.count_ones() > 2 * approvals.len() { - Check::Approved(0) - } else { - Check::Unapproved - } - } + RequiredTranches::All => Check::Unapproved, RequiredTranches::Exact { needed, tolerated_missing, .. } => { // whether all assigned validators up to `needed` less no_shows have approved. // e.g. if we had 5 tranches and 1 no-show, we would accept all validators in @@ -410,33 +422,6 @@ mod tests { ).is_approved()); } - #[test] - fn all_requires_supermajority() { - let mut candidate: CandidateEntry = approval_db::v1::CandidateEntry { - candidate: Default::default(), - session: 0, - block_assignments: Default::default(), - approvals: bitvec![BitOrderLsb0, u8; 0; 10], - }.into(); - - for i in 0..6 { - candidate.mark_approval(ValidatorIndex(i)); - } - - let approval_entry = approval_db::v1::ApprovalEntry { - tranches: Vec::new(), - assignments: bitvec![BitOrderLsb0, u8; 1; 10], - our_assignment: None, - backing_group: GroupIndex(0), - approved: false, - }.into(); - - assert!(!check_approval(&candidate, &approval_entry, RequiredTranches::All).is_approved()); - - candidate.mark_approval(ValidatorIndex(6)); - assert!(check_approval(&candidate, &approval_entry, RequiredTranches::All).is_approved()); - } - #[test] fn exact_takes_only_assignments_up_to() { let mut candidate: CandidateEntry = approval_db::v1::CandidateEntry { @@ -446,7 +431,70 @@ mod tests { approvals: bitvec![BitOrderLsb0, u8; 0; 10], }.into(); - for i in 0..6 { + for i in 0..3 { + candidate.mark_approval(ValidatorIndex(i)); + } + + let approval_entry = approval_db::v1::ApprovalEntry { + tranches: vec![ + approval_db::v1::TrancheEntry { + tranche: 0, + assignments: (0..2).map(|i| (ValidatorIndex(i), 0.into())).collect(), + }, + approval_db::v1::TrancheEntry { + tranche: 1, + assignments: (2..5).map(|i| (ValidatorIndex(i), 1.into())).collect(), + }, + approval_db::v1::TrancheEntry { + tranche: 2, + assignments: (5..10).map(|i| (ValidatorIndex(i), 0.into())).collect(), + }, + ], + assignments: bitvec![BitOrderLsb0, u8; 1; 10], + our_assignment: None, + backing_group: GroupIndex(0), + approved: false, + }.into(); + + assert!(check_approval( + &candidate, + &approval_entry, + RequiredTranches::Exact { + needed: 0, + tolerated_missing: 0, + next_no_show: None, + }, + ).is_approved()); + assert!(!check_approval( + &candidate, + &approval_entry, + RequiredTranches::Exact { + needed: 1, + tolerated_missing: 0, + next_no_show: None, + }, + ).is_approved()); + assert!(check_approval( + &candidate, + &approval_entry, + RequiredTranches::Exact { + needed: 1, + tolerated_missing: 2, + next_no_show: None, + }, + ).is_approved()); + } + + #[test] + fn one_honest_node_always_approves() { + let mut candidate: CandidateEntry = approval_db::v1::CandidateEntry { + candidate: Default::default(), + session: 0, + block_assignments: Default::default(), + approvals: bitvec![BitOrderLsb0, u8; 0; 10], + }.into(); + + for i in 0..3 { candidate.mark_approval(ValidatorIndex(i)); } @@ -471,32 +519,56 @@ mod tests { approved: false, }.into(); - assert!(check_approval( - &candidate, - &approval_entry, - RequiredTranches::Exact { - needed: 1, - tolerated_missing: 0, - next_no_show: None, - }, - ).is_approved()); + let exact_all = RequiredTranches::Exact { + needed: 10, + tolerated_missing: 0, + next_no_show: None, + }; + + let pending_all = RequiredTranches::Pending { + considered: 5, + next_no_show: None, + maximum_broadcast: 8, + clock_drift: 12, + }; + assert!(!check_approval( &candidate, &approval_entry, - RequiredTranches::Exact { - needed: 2, - tolerated_missing: 0, - next_no_show: None, - }, + RequiredTranches::All, ).is_approved()); + + assert!(!check_approval( + &candidate, + &approval_entry, + exact_all.clone(), + ).is_approved()); + + assert!(!check_approval( + &candidate, + &approval_entry, + pending_all.clone(), + ).is_approved()); + + // This creates a set of 4/10 approvals, which is always an approval. + candidate.mark_approval(ValidatorIndex(3)); + assert!(check_approval( &candidate, &approval_entry, - RequiredTranches::Exact { - needed: 2, - tolerated_missing: 4, - next_no_show: None, - }, + RequiredTranches::All, + ).is_approved()); + + assert!(check_approval( + &candidate, + &approval_entry, + exact_all, + ).is_approved()); + + assert!(check_approval( + &candidate, + &approval_entry, + pending_all, ).is_approved()); } diff --git a/polkadot/node/core/approval-voting/src/lib.rs b/polkadot/node/core/approval-voting/src/lib.rs index 87eda0ef97..4ee997d77d 100644 --- a/polkadot/node/core/approval-voting/src/lib.rs +++ b/polkadot/node/core/approval-voting/src/lib.rs @@ -1316,7 +1316,7 @@ fn check_and_apply_full_approval( required_tranches.clone(), ); - if let approval_checking::Check::Approved(no_shows) = check { + if check.is_approved() { tracing::trace!( target: LOG_TARGET, ?candidate_hash, @@ -1324,6 +1324,8 @@ fn check_and_apply_full_approval( "Candidate approved under block.", ); + let no_shows = check.known_no_shows(); + let was_approved = block_entry.is_fully_approved(); newly_approved.push(*block_hash); diff --git a/polkadot/node/core/approval-voting/src/tests.rs b/polkadot/node/core/approval-voting/src/tests.rs index 40d84d698a..9b9c153919 100644 --- a/polkadot/node/core/approval-voting/src/tests.rs +++ b/polkadot/node/core/approval-voting/src/tests.rs @@ -869,7 +869,52 @@ fn check_and_apply_full_approval_does_not_load_cached_block_from_db() { } #[test] -fn assignment_triggered_by_all_with_less_than_supermajority() { +fn assignment_triggered_by_all_with_less_than_threshold() { + let block_hash = Hash::repeat_byte(0x01); + + let mut candidate_entry: CandidateEntry = { + let approval_entry = approval_db::v1::ApprovalEntry { + tranches: Vec::new(), + backing_group: GroupIndex(0), + our_assignment: Some(approval_db::v1::OurAssignment { + cert: garbage_assignment_cert( + AssignmentCertKind::RelayVRFModulo { sample: 0 } + ), + tranche: 1, + validator_index: ValidatorIndex(4), + triggered: false, + }), + assignments: bitvec::bitvec![BitOrderLsb0, u8; 0; 4], + approved: false, + }; + + approval_db::v1::CandidateEntry { + candidate: Default::default(), + session: 1, + block_assignments: vec![(block_hash, approval_entry)].into_iter().collect(), + approvals: bitvec::bitvec![BitOrderLsb0, u8; 0; 4], + }.into() + }; + + // 1-of-4 + candidate_entry + .approval_entry_mut(&block_hash) + .unwrap() + .import_assignment(0, ValidatorIndex(0), 0); + + candidate_entry.mark_approval(ValidatorIndex(0)); + + let tranche_now = 1; + assert!(should_trigger_assignment( + candidate_entry.approval_entry(&block_hash).unwrap(), + &candidate_entry, + RequiredTranches::All, + tranche_now, + )); +} + +#[test] +fn assignment_not_triggered_by_all_with_threshold() { let block_hash = Hash::repeat_byte(0x01); let mut candidate_entry: CandidateEntry = { @@ -910,63 +955,6 @@ fn assignment_triggered_by_all_with_less_than_supermajority() { candidate_entry.mark_approval(ValidatorIndex(0)); candidate_entry.mark_approval(ValidatorIndex(1)); - let tranche_now = 1; - assert!(should_trigger_assignment( - candidate_entry.approval_entry(&block_hash).unwrap(), - &candidate_entry, - RequiredTranches::All, - tranche_now, - )); -} - -#[test] -fn assignment_not_triggered_by_all_with_supermajority() { - let block_hash = Hash::repeat_byte(0x01); - - let mut candidate_entry: CandidateEntry = { - let approval_entry = approval_db::v1::ApprovalEntry { - tranches: Vec::new(), - backing_group: GroupIndex(0), - our_assignment: Some(approval_db::v1::OurAssignment { - cert: garbage_assignment_cert( - AssignmentCertKind::RelayVRFModulo { sample: 0 } - ), - tranche: 1, - validator_index: ValidatorIndex(4), - triggered: false, - }), - assignments: bitvec::bitvec![BitOrderLsb0, u8; 0; 4], - approved: false, - }; - - approval_db::v1::CandidateEntry { - candidate: Default::default(), - session: 1, - block_assignments: vec![(block_hash, approval_entry)].into_iter().collect(), - approvals: bitvec::bitvec![BitOrderLsb0, u8; 0; 4], - }.into() - }; - - // 3-of-4 - candidate_entry - .approval_entry_mut(&block_hash) - .unwrap() - .import_assignment(0, ValidatorIndex(0), 0); - - candidate_entry - .approval_entry_mut(&block_hash) - .unwrap() - .import_assignment(0, ValidatorIndex(1), 0); - - candidate_entry - .approval_entry_mut(&block_hash) - .unwrap() - .import_assignment(0, ValidatorIndex(2), 0); - - candidate_entry.mark_approval(ValidatorIndex(0)); - candidate_entry.mark_approval(ValidatorIndex(1)); - candidate_entry.mark_approval(ValidatorIndex(2)); - let tranche_now = 1; assert!(!should_trigger_assignment( candidate_entry.approval_entry(&block_hash).unwrap(), diff --git a/polkadot/roadmap/implementers-guide/src/node/approval/approval-voting.md b/polkadot/roadmap/implementers-guide/src/node/approval/approval-voting.md index b2b9a243b2..dd6994caea 100644 --- a/polkadot/roadmap/implementers-guide/src/node/approval/approval-voting.md +++ b/polkadot/roadmap/implementers-guide/src/node/approval/approval-voting.md @@ -360,8 +360,9 @@ Likewise, when considering how many tranches to take, the no-show depth should b #### Check Approval * Check whether a candidate is approved under a particular block. * Requires `(block_entry, candidate_entry, approval_entry, n_tranches)` + * If we have `3 * n_approvals > n_validators`, return true. This is because any set with f+1 validators must have at least one honest validator, who has approved the candidate. * If `n_tranches` is `RequiredTranches::Pending`, return false - * If `n_tranches` is `RequiredTranches::All`, then we return `3 * n_approvals > 2 * n_validators`. + * If `n_tranches` is `RequiredTranches::All`, return false. * If `n_tranches` is `RequiredTranches::Exact { tranche, tolerated_missing, .. }`, then we return whether all assigned validators up to `tranche` less `tolerated_missing` have approved. e.g. if we had 5 tranches and 1 tolerated missing, we would accept only if all but 1 of assigned validators in tranches 0..=5 have approved. In that example, we also accept all validators in tranches 0..=5 having approved, but that would indicate that the `RequiredTranches` value was incorrectly constructed, so it is not realistic. `tolerated_missing` actually represents covered no-shows. If there are more missing approvals than there are tolerated missing, that indicates that there are some assignments which are not yet no-shows, but may become no-shows, and we should wait for the validators to either approve or become no-shows. ### Time