From 8b06a6c83a0c55133318ad2d8333bd449d56784e Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 14 Sep 2021 15:13:15 -0500 Subject: [PATCH 1/7] approval-voting: processed wakeups can also update approval state --- node/core/approval-voting/src/lib.rs | 113 ++++++++++++++------------- 1 file changed, 58 insertions(+), 55 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 529523b5b8f9..1e9f66011dcd 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -694,6 +694,7 @@ where woken_block, woken_candidate, tick, + &subsystem.metrics, )? } next_msg = ctx.recv().fuse() => { @@ -1710,14 +1711,14 @@ fn check_and_import_approval( None }; - let mut actions = import_checked_approval( + let mut actions = advance_approval_state( state, db, &metrics, block_entry, approved_candidate_hash, candidate_entry, - ApprovalSource::Remote(approval.validator), + ApprovalStateTransition::RemoteApproval(approval.validator), ); actions.extend(inform_disputes_action); @@ -1725,41 +1726,54 @@ fn check_and_import_approval( Ok((actions, t)) } -enum ApprovalSource { - Remote(ValidatorIndex), - Local(ValidatorIndex, ValidatorSignature), +enum ApprovalStateTransition { + RemoteApproval(ValidatorIndex), + LocalApproval(ValidatorIndex, ValidatorSignature), + WakeupProcessed, } -impl ApprovalSource { - fn validator_index(&self) -> ValidatorIndex { +impl ApprovalStateTransition { + fn validator_index(&self) -> Option { + match *self { + ApprovalStateTransition::RemoteApproval(v) + | ApprovalStateTransition::LocalApproval(v, _) => Some(v), + ApprovalStateTransition::WakeupProcessed => None, + } + } + + fn is_remote_approval(&self) -> bool { match *self { - ApprovalSource::Remote(v) | ApprovalSource::Local(v, _) => v, + ApprovalStateTransition::RemoteApproval(_) => true, + ApprovalStateTransition::LocalApproval(_, _) => false, + ApprovalStateTransition::WakeupProcessed => false, } } - fn is_remote(&self) -> bool { + fn is_local_approval(&self) -> bool { match *self { - ApprovalSource::Remote(_) => true, - ApprovalSource::Local(_, _) => false, + ApprovalStateTransition::RemoteApproval(_) => false, + ApprovalStateTransition::LocalApproval(_, _) => true, + ApprovalStateTransition::WakeupProcessed => false, } } } -// Import an approval vote which is already checked to be valid and corresponding to an assigned -// validator on the candidate and block. This updates the block entry and candidate entry as +// Advance the approval state, either by importing an approval vote which is already checked to be valid and corresponding to an assigned +// validator on the candidate and block, or by noting that there are no further wakeups or tranches needed. This updates the block entry and candidate entry as // necessary and schedules any further wakeups. -fn import_checked_approval( +fn advance_approval_state( state: &State, db: &mut OverlayedBackend<'_, impl Backend>, metrics: &Metrics, mut block_entry: BlockEntry, candidate_hash: CandidateHash, mut candidate_entry: CandidateEntry, - source: ApprovalSource, + transition: ApprovalStateTransition, ) -> Vec { - let validator_index = source.validator_index(); + let validator_index = transition.validator_index(); - let already_approved_by = candidate_entry.mark_approval(validator_index); + let already_approved_by = validator_index.as_ref() + .map(|v| candidate_entry.mark_approval(*v)); let candidate_approved_in_block = block_entry.is_candidate_approved(&candidate_hash); // Check for early exits. @@ -1771,17 +1785,13 @@ fn import_checked_approval( // If the block was approved, but the validator hadn't approved it yet, we should still hold // onto the approval vote on-disk in case we restart and rebroadcast votes. Otherwise, our // assignment might manifest as a no-show. - match source { - ApprovalSource::Remote(_) => { - // We don't store remote votes, so we can early exit as long at the candidate is - // already concluded under the block i.e. we don't need more approvals. - if candidate_approved_in_block { - return Vec::new() - } - }, - ApprovalSource::Local(_, _) => { - // We never early return on the local validator. - }, + if !transition.is_local_approval() { + // We don't store remote votes and there's nothing to store for processed wakeups, + // so we can early exit as long at the candidate is already concluded under the + // block i.e. we don't need more approvals. + if candidate_approved_in_block { + return Vec::new() + } } let mut actions = Vec::new(); @@ -1852,7 +1862,7 @@ fn import_checked_approval( approval_entry.mark_approved(); } - if let ApprovalSource::Local(_, ref sig) = source { + if let ApprovalStateTransition::LocalApproval(_, ref sig) = transition { approval_entry.import_approval_sig(sig.clone()); } @@ -1867,11 +1877,11 @@ fn import_checked_approval( // We have no need to write the candidate entry if // - // 1. The source is remote, as we don't store anything new in the approval entry. + // 1. The approver is remote, as we don't store anything new in the approval entry. // 2. The candidate is not newly approved, as we haven't altered the approval entry's // approved flag with `mark_approved` above. - // 3. The source had already approved the candidate, as we haven't altered the bitfield. - if !source.is_remote() || newly_approved || !already_approved_by { + // 3. The approver, if any, had already approved the candidate, as we haven't altered the bitfield. + if !transition.is_remote_approval() || newly_approved || already_approved_by.unwrap_or(false) { // In all other cases, we need to write the candidate entry. db.write_candidate_entry(candidate_entry); } @@ -1918,6 +1928,7 @@ fn process_wakeup( relay_block: Hash, candidate_hash: CandidateHash, expected_tick: Tick, + metrics: &Metrics, ) -> SubsystemResult> { let _span = jaeger::Span::from_encodable( (relay_block, candidate_hash, expected_tick), @@ -2040,28 +2051,20 @@ fn process_wakeup( } } - let approval_entry = candidate_entry - .approval_entry(&relay_block) - .expect("this function returned earlier if not available; qed"); - - // Although we ran this earlier in the function, we need to run again because we might have - // imported our own assignment, which could change things. - let tranches_to_approve = approval_checking::tranches_to_approve( - &approval_entry, - candidate_entry.approvals(), - tranche_now, - block_tick, - no_show_duration, - session_info.needed_approvals as _, - ); - - actions.extend(schedule_wakeup_action( - &approval_entry, - relay_block, - block_entry.block_number(), + // Although we checked approval earlier in this function, + // this wakeup might have advanced the state to approved via + // a no-show that was immediately covered and therefore + // we need to check for that and advance the state on-disk. + // + // Note that this function also schedules a wakeup as necessary. + actions.extend(advance_approval_state( + state, + db, + metrics, + block_entry, candidate_hash, - block_tick, - tranches_to_approve, + candidate_entry, + ApprovalStateTransition::WakeupProcessed, )); Ok(actions) @@ -2436,14 +2439,14 @@ async fn issue_approval( None }; - let mut actions = import_checked_approval( + let mut actions = advance_approval_state( state, db, metrics, block_entry, candidate_hash, candidate_entry, - ApprovalSource::Local(validator_index as _, sig.clone()), + ApprovalStateTransition::LocalApproval(validator_index as _, sig.clone()), ); metrics.on_approval_produced(); From 296c9288916e3f38f602874b9e155214a752ba14 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 14 Sep 2021 15:24:37 -0500 Subject: [PATCH 2/7] fmt changes --- node/core/approval-voting/src/lib.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 1e9f66011dcd..379cbfa89e55 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -1735,8 +1735,8 @@ enum ApprovalStateTransition { impl ApprovalStateTransition { fn validator_index(&self) -> Option { match *self { - ApprovalStateTransition::RemoteApproval(v) - | ApprovalStateTransition::LocalApproval(v, _) => Some(v), + ApprovalStateTransition::RemoteApproval(v) | + ApprovalStateTransition::LocalApproval(v, _) => Some(v), ApprovalStateTransition::WakeupProcessed => None, } } @@ -1772,8 +1772,7 @@ fn advance_approval_state( ) -> Vec { let validator_index = transition.validator_index(); - let already_approved_by = validator_index.as_ref() - .map(|v| candidate_entry.mark_approval(*v)); + let already_approved_by = validator_index.as_ref().map(|v| candidate_entry.mark_approval(*v)); let candidate_approved_in_block = block_entry.is_candidate_approved(&candidate_hash); // Check for early exits. @@ -1881,7 +1880,10 @@ fn advance_approval_state( // 2. The candidate is not newly approved, as we haven't altered the approval entry's // approved flag with `mark_approved` above. // 3. The approver, if any, had already approved the candidate, as we haven't altered the bitfield. - if !transition.is_remote_approval() || newly_approved || already_approved_by.unwrap_or(false) { + if !transition.is_remote_approval() || + newly_approved || + already_approved_by.unwrap_or(false) + { // In all other cases, we need to write the candidate entry. db.write_candidate_entry(candidate_entry); } From 78bfa2604202704196d11ce727da69cb63eb369c Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 14 Sep 2021 20:06:19 -0500 Subject: [PATCH 3/7] reverse broken if condition --- node/core/approval-voting/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 379cbfa89e55..8e9de3237e7f 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -1882,7 +1882,7 @@ fn advance_approval_state( // 3. The approver, if any, had already approved the candidate, as we haven't altered the bitfield. if !transition.is_remote_approval() || newly_approved || - already_approved_by.unwrap_or(false) + !already_approved_by.unwrap_or(true) { // In all other cases, we need to write the candidate entry. db.write_candidate_entry(candidate_entry); From e3105ac04aa8c559a0ec6d097aaeb34aac20e04e Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 14 Sep 2021 20:08:13 -0500 Subject: [PATCH 4/7] further correct condition --- node/core/approval-voting/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 8e9de3237e7f..8d9298c13517 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -1874,13 +1874,14 @@ fn advance_approval_state( status.required_tranches, )); - // We have no need to write the candidate entry if + // We have no need to write the candidate entry if all of the following + // are trueL // - // 1. The approver is remote, as we don't store anything new in the approval entry. + // 1. This is not a local approval, as we don't store anything new in the approval entry. // 2. The candidate is not newly approved, as we haven't altered the approval entry's // approved flag with `mark_approved` above. // 3. The approver, if any, had already approved the candidate, as we haven't altered the bitfield. - if !transition.is_remote_approval() || + if transition.is_local_approval() || newly_approved || !already_approved_by.unwrap_or(true) { From f0ef443e5e999dead6bcec9366f6418f2fe1da6c Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 14 Sep 2021 21:08:41 -0500 Subject: [PATCH 5/7] add test --- node/core/approval-voting/src/lib.rs | 9 +- node/core/approval-voting/src/tests.rs | 193 +++++++++++++++++++++++++ 2 files changed, 194 insertions(+), 8 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 8d9298c13517..e3572637f3aa 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -1726,6 +1726,7 @@ fn check_and_import_approval( Ok((actions, t)) } +#[derive(Debug)] enum ApprovalStateTransition { RemoteApproval(ValidatorIndex), LocalApproval(ValidatorIndex, ValidatorSignature), @@ -1741,14 +1742,6 @@ impl ApprovalStateTransition { } } - fn is_remote_approval(&self) -> bool { - match *self { - ApprovalStateTransition::RemoteApproval(_) => true, - ApprovalStateTransition::LocalApproval(_, _) => false, - ApprovalStateTransition::WakeupProcessed => false, - } - } - fn is_local_approval(&self) -> bool { match *self { ApprovalStateTransition::RemoteApproval(_) => false, diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index 9819991641c5..9aefb2ddb010 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -2555,3 +2555,196 @@ fn subsystem_assignment_not_triggered_if_at_maximum_but_clock_is_before_with_dri should_be_triggered: |_| false, }); } + +#[test] +fn pre_covers_dont_stall_approval() { + // A, B are tranche 0. + // C is tranche 1. + // + // All assignments imported at once, and B, C approvals imported immediately. + // A no-shows, leading to being covered by C. + // Technically, this is an approved block, but it will be approved + // when the no-show timer hits, not as a response to an approval vote. + // + // Note that we have 6 validators, otherwise the 2nd approval triggers + // the >1/3 insta-approval condition. + + let assignment_criteria = + Box::new(MockAssignmentCriteria::check_only(move |validator_index| match validator_index { + ValidatorIndex(0 | 1) => Ok(0), + ValidatorIndex(2) => Ok(1), + ValidatorIndex(_) => Err(criteria::InvalidAssignment), + })); + + let config = HarnessConfigBuilder::default().assignment_criteria(assignment_criteria).build(); + let store = config.backend(); + test_harness(config, |test_harness| async move { + let TestHarness { + mut virtual_overseer, + clock, + sync_oracle_handle: _sync_oracle_handle, + .. + } = test_harness; + + let block_hash = Hash::repeat_byte(0x01); + let validator_index_a = ValidatorIndex(0); + let validator_index_b = ValidatorIndex(1); + let validator_index_c = ValidatorIndex(2); + + let validators = vec![ + Sr25519Keyring::Alice, + Sr25519Keyring::Bob, + Sr25519Keyring::Charlie, + Sr25519Keyring::Dave, + Sr25519Keyring::Eve, + Sr25519Keyring::One, + ]; + let session_info = SessionInfo { + validators: validators.iter().map(|v| v.public().into()).collect(), + validator_groups: vec![ + vec![ValidatorIndex(0), ValidatorIndex(1)], + vec![ValidatorIndex(2), ValidatorIndex(5)], + vec![ValidatorIndex(3), ValidatorIndex(4)], + ], + needed_approvals: 2, + discovery_keys: validators.iter().map(|v| v.public().into()).collect(), + assignment_keys: validators.iter().map(|v| v.public().into()).collect(), + n_cores: validators.len() as _, + zeroth_delay_tranche_width: 5, + relay_vrf_modulo_samples: 3, + n_delay_tranches: 50, + no_show_slots: 2, + }; + + let candidate_descriptor = make_candidate(1.into(), &block_hash); + let candidate_hash = candidate_descriptor.hash(); + + let head: Hash = ChainBuilder::GENESIS_HASH; + let mut builder = ChainBuilder::new(); + let slot = Slot::from(1 as u64); + builder.add_block( + block_hash, + head, + 1, + BlockConfig { + slot, + candidates: Some(vec![(candidate_descriptor, CoreIndex(0), GroupIndex(0))]), + session_info: Some(session_info), + }, + ); + builder.build(&mut virtual_overseer).await; + + let candidate_index = 0; + + let rx = check_and_import_assignment( + &mut virtual_overseer, + block_hash, + candidate_index, + validator_index_a, + ) + .await; + + assert_eq!(rx.await, Ok(AssignmentCheckResult::Accepted),); + + let rx = check_and_import_assignment( + &mut virtual_overseer, + block_hash, + candidate_index, + validator_index_b, + ) + .await; + + assert_eq!(rx.await, Ok(AssignmentCheckResult::Accepted),); + + let rx = check_and_import_assignment( + &mut virtual_overseer, + block_hash, + candidate_index, + validator_index_c, + ) + .await; + + assert_eq!(rx.await, Ok(AssignmentCheckResult::Accepted),); + + let session_index = 1; + let sig_b = sign_approval(Sr25519Keyring::Bob, candidate_hash, session_index); + + let rx = check_and_import_approval( + &mut virtual_overseer, + block_hash, + candidate_index, + validator_index_b, + candidate_hash, + session_index, + false, + true, + Some(sig_b), + ) + .await; + + assert_eq!(rx.await, Ok(ApprovalCheckResult::Accepted),); + + let sig_c = sign_approval(Sr25519Keyring::Charlie, candidate_hash, session_index); + let rx = check_and_import_approval( + &mut virtual_overseer, + block_hash, + candidate_index, + validator_index_c, + candidate_hash, + session_index, + false, + true, + Some(sig_c), + ) + .await; + + assert_eq!(rx.await, Ok(ApprovalCheckResult::Accepted),); + + // Sleep to ensure we get a consistent read on the database. + // + // NOTE: Since the response above occurs before writing to the database, we are somewhat + // breaking the external consistency of the API by reaching into the database directly. + // Under normal operation, this wouldn't be necessary, since all requests are serialized by + // the event loop and we write at the end of each pass. However, if the database write were + // to fail, a downstream subsystem may expect for this candidate to be approved, and + // possibly take further actions on the assumption that the candidate is approved, when + // that may not be the reality from the database's perspective. This could be avoided + // entirely by having replies processed after database writes, but that would constitute a + // larger refactor and incur a performance penalty. + futures_timer::Delay::new(Duration::from_millis(100)).await; + + // The candidate should not be approved. + let candidate_entry = store.load_candidate_entry(&candidate_hash).unwrap().unwrap(); + assert!(!candidate_entry.approval_entry(&block_hash).unwrap().is_approved()); + assert!(clock.inner.lock().has_wakeup(20)); + + // Wait for the no-show timer to observe the approval from + // tranche 0 and set a wakeup for tranche 1. + clock.inner.lock().set_tick(20); + + // Sleep to ensure we get a consistent read on the database. + futures_timer::Delay::new(Duration::from_millis(100)).await; + + // The next wakeup should observe the assignment & approval from + // tranche 1, and the no-show from tranche 0 should be immediately covered. + assert_eq!(clock.inner.lock().next_wakeup(), Some(31)); + clock.inner.lock().set_tick(31); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::ChainSelection(ChainSelectionMessage::Approved(b_hash)) => { + assert_eq!(b_hash, block_hash); + } + ); + + // The candidate and block should now be approved. + let candidate_entry = store.load_candidate_entry(&candidate_hash).unwrap().unwrap(); + assert!(candidate_entry.approval_entry(&block_hash).unwrap().is_approved()); + assert!(clock.inner.lock().next_wakeup().is_none()); + + let block_entry = store.load_block_entry(&block_hash).unwrap().unwrap(); + assert!(block_entry.is_fully_approved()); + + virtual_overseer + }); +} From 2751fc01aee115a192fbe2a090f24ad57fae8c04 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Wed, 15 Sep 2021 15:29:15 +0200 Subject: [PATCH 6/7] fmt --- node/core/approval-voting/src/lib.rs | 4 +--- node/core/approval-voting/src/tests.rs | 7 ++++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index e3572637f3aa..577a803431ff 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -1874,9 +1874,7 @@ fn advance_approval_state( // 2. The candidate is not newly approved, as we haven't altered the approval entry's // approved flag with `mark_approved` above. // 3. The approver, if any, had already approved the candidate, as we haven't altered the bitfield. - if transition.is_local_approval() || - newly_approved || - !already_approved_by.unwrap_or(true) + if transition.is_local_approval() || newly_approved || !already_approved_by.unwrap_or(true) { // In all other cases, we need to write the candidate entry. db.write_candidate_entry(candidate_entry); diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index 9aefb2ddb010..c40d62ecedf1 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -2569,12 +2569,13 @@ fn pre_covers_dont_stall_approval() { // Note that we have 6 validators, otherwise the 2nd approval triggers // the >1/3 insta-approval condition. - let assignment_criteria = - Box::new(MockAssignmentCriteria::check_only(move |validator_index| match validator_index { + let assignment_criteria = Box::new(MockAssignmentCriteria::check_only( + move |validator_index| match validator_index { ValidatorIndex(0 | 1) => Ok(0), ValidatorIndex(2) => Ok(1), ValidatorIndex(_) => Err(criteria::InvalidAssignment), - })); + }, + )); let config = HarnessConfigBuilder::default().assignment_criteria(assignment_criteria).build(); let store = config.backend(); From fd5bab0a91ce8ca8f91e51f75a65738f22a0c7e0 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 15 Sep 2021 12:58:36 -0500 Subject: [PATCH 7/7] Update node/core/approval-voting/src/lib.rs Co-authored-by: Andronik Ordian --- node/core/approval-voting/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index af4ea2d26b07..b7588710b1a3 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -1868,7 +1868,7 @@ fn advance_approval_state( )); // We have no need to write the candidate entry if all of the following - // are trueL + // is true: // // 1. This is not a local approval, as we don't store anything new in the approval entry. // 2. The candidate is not newly approved, as we haven't altered the approval entry's