From 5eb24c6a1be149a83b9a5f926f841c670ed38e80 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 13 Apr 2023 15:46:16 +0300 Subject: [PATCH 01/42] Replace `RollingSessionWindow` with `RuntimeInfo` - initial commit --- node/core/approval-voting/src/import.rs | 95 +++---- node/core/approval-voting/src/lib.rs | 329 +++++++++++++++++------- 2 files changed, 288 insertions(+), 136 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index f3a571a7133d..2be583fb4107 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -40,10 +40,7 @@ use polkadot_node_subsystem::{ }, overseer, RuntimeApiError, SubsystemError, SubsystemResult, }; -use polkadot_node_subsystem_util::{ - determine_new_blocks, - rolling_session_window::{RollingSessionWindow, SessionWindowUpdate}, -}; +use polkadot_node_subsystem_util::determine_new_blocks; use polkadot_primitives::{ BlockNumber, CandidateEvent, CandidateHash, CandidateReceipt, ConsensusLog, CoreIndex, GroupIndex, Hash, Header, SessionIndex, @@ -62,6 +59,7 @@ use crate::{ criteria::{AssignmentCriteria, OurAssignment}, persisted_entries::CandidateEntry, time::{slot_number_to_tick, Tick}, + SessionInfoProvider, }; use super::{State, LOG_TARGET}; @@ -78,7 +76,7 @@ struct ImportedBlockInfo { } struct ImportedBlockInfoEnv<'a> { - session_window: &'a Option, + runtime_info: &'a mut Option, // this is required just for the `earliest_session()` assignment_criteria: &'a (dyn AssignmentCriteria + Send + Sync), keystore: &'a LocalKeystore, } @@ -160,11 +158,7 @@ async fn imported_block_info( return Err(ImportedBlockInfoError::FutureCancelled("SessionIndexForChild", error)), }; - if env - .session_window - .as_ref() - .map_or(true, |s| session_index < s.earliest_session()) - { + if env.runtime_info.as_ref().map_or(true, |s| session_index < s.earliest_session) { gum::debug!( target: LOG_TARGET, "Block {} is from ancient session {}. Skipping", @@ -212,10 +206,21 @@ async fn imported_block_info( } }; - let session_info = match env.session_window.as_ref().and_then(|s| s.session_info(session_index)) - { - Some(s) => s, - None => { + // todo: refactor this + let session_info = if let Some(session_info_provider) = env.runtime_info { + session_info_provider + .runtime_info + .get_session_info_by_index(ctx.sender(), block_hash, session_index) + .await + .map_err(|_| ImportedBlockInfoError::SessionInfoUnavailable) + } else { + gum::debug!(target: LOG_TARGET, "SessionInfoProvider unavailable for block {}", block_hash,); + return Err(ImportedBlockInfoError::SessionInfoUnavailable) + }; + + let session_info = match session_info { + Ok(extended_session_info) => &extended_session_info.session_info, + Err(_) => { gum::debug!(target: LOG_TARGET, "Session info unavailable for block {}", block_hash,); return Err(ImportedBlockInfoError::SessionInfoUnavailable) @@ -360,25 +365,24 @@ pub(crate) async fn handle_new_head( }; // Update session info based on most recent head. - match state.cache_session_info_for_head(ctx, head).await { - Err(e) => { - gum::debug!( - target: LOG_TARGET, - ?head, - ?e, - "Could not cache session info when processing head.", - ); - return Ok(Vec::new()) - }, - Ok(Some(a @ SessionWindowUpdate::Advanced { .. })) => { - gum::info!( - target: LOG_TARGET, - update = ?a, - "Advanced session window for approvals", - ); - }, - Ok(_) => {}, - } + state.cache_session_info_for_head(ctx, head).await; + // Err(e) => { + // gum::debug!( + // target: LOG_TARGET, + // ?head, + // ?e, + // "Could not cache session info when processing head.", + // ); + // return Ok(Vec::new()) + // }, + // Ok(Some(a @ SessionWindowUpdate::Advanced { .. })) => { + // gum::info!( + // target: LOG_TARGET, + // update = ?a, + // "Advanced session window for approvals", + // ); + // }, + // Ok(_) => {},s // If we've just started the node and are far behind, // import at most `MAX_HEADS_LOOK_BACK` blocks. @@ -407,7 +411,7 @@ pub(crate) async fn handle_new_head( let mut imported_blocks_and_info = Vec::with_capacity(new_blocks.len()); for (block_hash, block_header) in new_blocks.into_iter().rev() { let env = ImportedBlockInfoEnv { - session_window: &state.session_window, + runtime_info: &mut state.session_info, assignment_criteria: &*state.assignment_criteria, keystore: &state.keystore, }; @@ -461,11 +465,16 @@ pub(crate) async fn handle_new_head( force_approve, } = imported_block_info; - let session_info = state - .session_window - .as_ref() - .and_then(|s| s.session_info(session_index)) - .expect("imported_block_info requires session info to be available; qed"); + // todo: refactor this + let session_info = &state + .session_info + .as_mut() + .map(|s| &mut s.runtime_info) + .expect("imported_block_info requires session info to be available; qed") + .get_session_info_by_index(ctx.sender(), head, session_index) + .await + .map_err(|e| SubsystemError::FromOrigin { origin: "", source: e.into() })? + .session_info; let (block_tick, no_show_duration) = { let block_tick = slot_number_to_tick(state.slot_duration_millis, slot); @@ -782,7 +791,7 @@ pub(crate) mod tests { let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - session_window: &Some(session_window), + runtime_info: &Some(session_window), assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; @@ -888,7 +897,7 @@ pub(crate) mod tests { let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - session_window: &Some(session_window), + runtime_info: &Some(session_window), assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; @@ -987,7 +996,7 @@ pub(crate) mod tests { let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - session_window: &session_window, + runtime_info: &session_window, assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; @@ -1083,7 +1092,7 @@ pub(crate) mod tests { let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - session_window: &session_window, + runtime_info: &session_window, assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 6a7ae10bfa2a..9c6f71c78f16 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -27,7 +27,7 @@ use polkadot_node_primitives::{ approval::{ BlockApprovalMeta, DelayTranche, IndirectAssignmentCert, IndirectSignedApprovalVote, }, - ValidationResult, + ValidationResult, DISPUTE_WINDOW, }; use polkadot_node_subsystem::{ errors::RecoveryError, @@ -42,11 +42,10 @@ use polkadot_node_subsystem::{ SubsystemSender, }; use polkadot_node_subsystem_util::{ + self, database::Database, metrics::{self, prometheus}, - rolling_session_window::{ - DatabaseParams, RollingSessionWindow, SessionWindowUpdate, SessionsUnavailable, - }, + runtime::{Config as RuntimeInfoConfig, RuntimeInfo}, TimeoutExt, }; use polkadot_primitives::{ @@ -638,70 +637,176 @@ impl CurrentlyCheckingSet { } } +// TODO: better name +struct SessionInfoProvider { + runtime_info: RuntimeInfo, + last_consecutive_cached_session: SessionIndex, + earliest_session: SessionIndex, +} + struct State { - session_window: Option, + session_info: Option, keystore: Arc, slot_duration_millis: u64, clock: Box, assignment_criteria: Box, - // Require for `RollingSessionWindow`. - db_config: DatabaseConfig, - db: Arc, spans: HashMap, } #[overseer::contextbounds(ApprovalVoting, prefix = self::overseer)] impl State { - fn session_info(&self, i: SessionIndex) -> Option<&SessionInfo> { - self.session_window.as_ref().and_then(|w| w.session_info(i)) + async fn session_info( + &mut self, + sender: &mut Sender, + relay_parent: Hash, + session_index: SessionIndex, + ) -> Option<&SessionInfo> + where + Sender: SubsystemSender, + { + match &mut self.session_info { + Some(session_info_provider) => match session_info_provider + .runtime_info + .get_session_info_by_index(sender, relay_parent, session_index) + .await + { + Ok(extended_info) => Some(&extended_info.session_info), + Err(_) => { + // todo log error + None + }, + }, + + None => None, + } } - /// Bring `session_window` up to date. - pub async fn cache_session_info_for_head( - &mut self, - ctx: &mut Context, - head: Hash, - ) -> Result, SessionsUnavailable> + /// If `head` is in a new session - cache it + pub async fn cache_session_info_for_head(&mut self, ctx: &mut Context, head: Hash) where ::Sender: Sized + Send, { - let session_window = self.session_window.take(); - match session_window { + match self.session_info.take() { None => { - let sender = ctx.sender().clone(); - self.session_window = Some( - RollingSessionWindow::new( - sender, - head, - DatabaseParams { - db: self.db.clone(), - db_column: self.db_config.col_session_data, + let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }); + + let head_session_idx = + match runtime_info.get_session_index_for_child(ctx.sender(), head).await { + Ok(session_idx) => session_idx, + Err(err) => { + gum::debug!( + target: LOG_TARGET, + ?head, + ?err, + "Error getting session index for head. Won't cache any sessions" + ); + return }, - ) - .await?, - ); - Ok(None) + }; + + let mut gap_in_cache = false; + let mut last_consecutive_cached_session = 0; + // TODO: fix this -> start of the windoow should be no less than the last finalized block + for idx in head_session_idx.saturating_sub(DISPUTE_WINDOW.get())..=head_session_idx + { + if let Err(err) = + runtime_info.get_session_info_by_index(ctx.sender(), head, idx).await + { + gap_in_cache = true; + gum::debug!( + target: LOG_TARGET, + ?err, + session = idx, + "Can cache session. Moving on." + ); + continue + } + + if !gap_in_cache { + last_consecutive_cached_session = idx; + } + } + + // TODO: fix this + let earliest_session = 0; + self.session_info = Some(SessionInfoProvider { + runtime_info, + last_consecutive_cached_session, + earliest_session, + }); }, - Some(mut session_window) => { - let r = session_window - .cache_session_info_for_head(ctx.sender(), head) + Some(mut session_info_provider) => { + let head_session_idx = match session_info_provider + .runtime_info + .get_session_index_for_child(ctx.sender(), head) .await - .map(Option::Some); - self.session_window = Some(session_window); - r + { + Ok(session_idx) => session_idx, + Err(err) => { + gum::debug!( + target: LOG_TARGET, + ?head, + ?err, + "Error getting session index for head. Won't cache any sessions" + ); + return + }, + }; + + let mut gap_in_cache = false; + if head_session_idx > session_info_provider.last_consecutive_cached_session { + for idx in + session_info_provider.last_consecutive_cached_session + 1..=head_session_idx + { + if let Err(err) = session_info_provider + .runtime_info + .get_session_info_by_index(ctx.sender(), head, idx) + .await + { + gap_in_cache = true; + gum::debug!( + target: LOG_TARGET, + ?err, + session = idx, + "Can cache session. Moving on." + ); + continue + } + + if !gap_in_cache { + session_info_provider.last_consecutive_cached_session = idx; + } + } + } + + // TODO: update `earliest_session` + + self.session_info = Some(session_info_provider); }, } } // Compute the required tranches for approval for this block and candidate combo. // Fails if there is no approval entry for the block under the candidate or no candidate entry // under the block, or if the session is out of bounds. - fn approval_status<'a, 'b>( - &'a self, + async fn approval_status( + &'a mut self, + sender: &mut Sender, block_entry: &'a BlockEntry, candidate_entry: &'b CandidateEntry, - ) -> Option<(&'b ApprovalEntry, ApprovalStatus)> { - let session_info = match self.session_info(block_entry.session()) { - Some(s) => s, + ) -> Option<(&'b ApprovalEntry, ApprovalStatus)> + where + Sender: SubsystemSender, + { + // We can't borrow the session here. Only get copies of what's needed. + let (no_show_slots, needed_approvals) = match self + .session_info(sender, block_entry.parent_hash(), block_entry.session()) + .await + { + Some(s) => (s.no_show_slots, s.needed_approvals), None => { gum::warn!( target: LOG_TARGET, @@ -715,10 +820,8 @@ impl State { let tranche_now = self.clock.tranche_now(self.slot_duration_millis, block_entry.slot()); let block_tick = slot_number_to_tick(self.slot_duration_millis, block_entry.slot()); - let no_show_duration = slot_number_to_tick( - self.slot_duration_millis, - Slot::from(u64::from(session_info.no_show_slots)), - ); + let no_show_duration = + slot_number_to_tick(self.slot_duration_millis, Slot::from(u64::from(no_show_slots))); if let Some(approval_entry) = candidate_entry.approval_entry(&block_hash) { let required_tranches = approval_checking::tranches_to_approve( @@ -727,7 +830,7 @@ impl State { tranche_now, block_tick, no_show_duration, - session_info.needed_approvals as _, + needed_approvals as _, ); let status = ApprovalStatus { required_tranches, block_tick, tranche_now }; @@ -779,13 +882,11 @@ where } let mut state = State { - session_window: None, + session_info: None, keystore: subsystem.keystore, slot_duration_millis: subsystem.slot_duration_millis, clock, assignment_criteria, - db_config: subsystem.db_config, - db: subsystem.db, spans: HashMap::new(), }; @@ -811,12 +912,13 @@ where (_tick, woken_block, woken_candidate) = wakeups.next(&*state.clock).fuse() => { subsystem.metrics.on_wakeup(); process_wakeup( + &mut ctx, &mut state, &mut overlayed_db, woken_block, woken_candidate, &subsystem.metrics, - )? + ).await? } next_msg = ctx.recv().fuse() => { let mut actions = handle_from_overseer( @@ -1260,15 +1362,16 @@ async fn handle_from_overseer( FromOrchestra::Communication { msg } => match msg { ApprovalVotingMessage::CheckAndImportAssignment(a, claimed_core, res) => { let (check_outcome, actions) = - check_and_import_assignment(state, db, a, claimed_core)?; + check_and_import_assignment(ctx.sender(), state, db, a, claimed_core).await?; let _ = res.send(check_outcome); actions }, ApprovalVotingMessage::CheckAndImportApproval(a, res) => - check_and_import_approval(state, db, metrics, a, |r| { + check_and_import_approval(ctx.sender(), state, db, metrics, a, |r| { let _ = res.send(r); - })? + }) + .await? .0, ApprovalVotingMessage::ApprovedAncestor(target, lower_bound, res) => { let mut approved_ancestor_span = state @@ -1738,12 +1841,16 @@ fn schedule_wakeup_action( maybe_action } -fn check_and_import_assignment( - state: &State, +async fn check_and_import_assignment( + sender: &mut Sender, + state: &mut State, db: &mut OverlayedBackend<'_, impl Backend>, assignment: IndirectAssignmentCert, candidate_index: CandidateIndex, -) -> SubsystemResult<(AssignmentCheckResult, Vec)> { +) -> SubsystemResult<(AssignmentCheckResult, Vec)> +where + Sender: SubsystemSender, +{ let tick_now = state.clock.tick_now(); let mut check_and_import_assignment_span = state @@ -1766,7 +1873,10 @@ fn check_and_import_assignment( )), }; - let session_info = match state.session_info(block_entry.session()) { + let session_info = match state + .session_info(sender, block_entry.parent_hash(), block_entry.session()) + .await + { Some(s) => s, None => return Ok(( @@ -1822,10 +1932,11 @@ fn check_and_import_assignment( )), }; + let config = &criteria::Config::from(session_info); let res = state.assignment_criteria.check_assignment_cert( claimed_core_index, assignment.validator, - &criteria::Config::from(session_info), + config, block_entry.relay_vrf_story(), &assignment.cert, approval_entry.backing_group(), @@ -1877,7 +1988,9 @@ fn check_and_import_assignment( let mut actions = Vec::new(); // We've imported a new approval, so we need to schedule a wake-up for when that might no-show. - if let Some((approval_entry, status)) = state.approval_status(&block_entry, &candidate_entry) { + if let Some((approval_entry, status)) = + state.approval_status(sender, &block_entry, &candidate_entry).await + { actions.extend(schedule_wakeup_action( approval_entry, block_entry.block_hash(), @@ -1895,13 +2008,17 @@ fn check_and_import_assignment( Ok((res, actions)) } -fn check_and_import_approval( - state: &State, +async fn check_and_import_approval( + sender: &mut Sender, + state: &mut State, db: &mut OverlayedBackend<'_, impl Backend>, metrics: &Metrics, approval: IndirectSignedApprovalVote, with_response: impl FnOnce(ApprovalCheckResult) -> T, -) -> SubsystemResult<(Vec, T)> { +) -> SubsystemResult<(Vec, T)> +where + Sender: SubsystemSender, +{ macro_rules! respond_early { ($e: expr) => {{ let t = with_response($e); @@ -1927,14 +2044,15 @@ fn check_and_import_approval( }, }; - let session_info = match state.session_info(block_entry.session()) { - Some(s) => s, - None => { - respond_early!(ApprovalCheckResult::Bad(ApprovalCheckError::UnknownSessionIndex( - block_entry.session() - ),)) - }, - }; + let session_info = + match state.session_info(sender, approval.block_hash, block_entry.session()).await { + Some(s) => s, + None => { + respond_early!(ApprovalCheckResult::Bad(ApprovalCheckError::UnknownSessionIndex( + block_entry.session() + ),)) + }, + }; let approved_candidate_hash = match block_entry.candidate(approval.candidate_index as usize) { Some((_, h)) => *h, @@ -2008,6 +2126,7 @@ fn check_and_import_approval( ); let actions = advance_approval_state( + sender, state, db, &metrics, @@ -2015,11 +2134,15 @@ fn check_and_import_approval( approved_candidate_hash, candidate_entry, ApprovalStateTransition::RemoteApproval(approval.validator), - ); + ) + .await; Ok((actions, t)) } +// TODO? +// fn get_session_info(..) {} + #[derive(Debug)] enum ApprovalStateTransition { RemoteApproval(ValidatorIndex), @@ -2048,15 +2171,19 @@ impl ApprovalStateTransition { // 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 advance_approval_state( - state: &State, +async fn advance_approval_state( + sender: &mut Sender, + state: &mut State, db: &mut OverlayedBackend<'_, impl Backend>, metrics: &Metrics, mut block_entry: BlockEntry, candidate_hash: CandidateHash, mut candidate_entry: CandidateEntry, transition: ApprovalStateTransition, -) -> Vec { +) -> Vec +where + Sender: SubsystemSender, +{ let validator_index = transition.validator_index(); let already_approved_by = validator_index.as_ref().map(|v| candidate_entry.mark_approval(*v)); @@ -2087,7 +2214,7 @@ fn advance_approval_state( let tick_now = state.clock.tick_now(); let (is_approved, status) = if let Some((approval_entry, status)) = - state.approval_status(&block_entry, &candidate_entry) + state.approval_status(sender, &block_entry, &candidate_entry).await { let check = approval_checking::check_approval( &candidate_entry, @@ -2217,8 +2344,10 @@ fn should_trigger_assignment( } } -fn process_wakeup( - state: &State, +#[overseer::contextbounds(ApprovalVoting, prefix = self::overseer)] +async fn process_wakeup( + ctx: &mut Context, + state: &mut State, db: &mut OverlayedBackend<'_, impl Backend>, relay_block: Hash, candidate_hash: CandidateHash, @@ -2243,7 +2372,12 @@ fn process_wakeup( _ => return Ok(Vec::new()), }; - let session_info = match state.session_info(block_entry.session()) { + let slot_duration_millis = state.slot_duration_millis; + let tranche_now = state.clock.tranche_now(slot_duration_millis, block_entry.slot()); + let session_info = match state + .session_info(ctx.sender(), block_entry.parent_hash(), block_entry.session()) + .await + { Some(i) => i, None => { gum::warn!( @@ -2257,12 +2391,12 @@ fn process_wakeup( }, }; - let block_tick = slot_number_to_tick(state.slot_duration_millis, block_entry.slot()); + let block_tick = slot_number_to_tick(slot_duration_millis, block_entry.slot()); let no_show_duration = slot_number_to_tick( - state.slot_duration_millis, + slot_duration_millis, Slot::from(u64::from(session_info.no_show_slots)), ); - let tranche_now = state.clock.tranche_now(state.slot_duration_millis, block_entry.slot()); + span.add_uint_tag("tranche", tranche_now as u64); gum::trace!( target: LOG_TARGET, @@ -2353,15 +2487,19 @@ fn process_wakeup( // 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, - candidate_entry, - ApprovalStateTransition::WakeupProcessed, - )); + actions.extend( + advance_approval_state( + ctx.sender(), + state, + db, + metrics, + block_entry, + candidate_hash, + candidate_entry, + ApprovalStateTransition::WakeupProcessed, + ) + .await, + ); Ok(actions) } @@ -2612,7 +2750,10 @@ async fn issue_approval( }; issue_approval_span.add_int_tag("candidate_index", candidate_index as i64); - let session_info = match state.session_info(block_entry.session()) { + let session_info = match state + .session_info(ctx.sender(), block_entry.parent_hash(), block_entry.session()) + .await + { Some(s) => s, None => { gum::warn!( @@ -2658,7 +2799,7 @@ async fn issue_approval( }; let validator_pubkey = match session_info.validators.get(validator_index) { - Some(p) => p, + Some(p) => p.clone(), // todo: remove this None => { gum::warn!( target: LOG_TARGET, @@ -2697,6 +2838,7 @@ async fn issue_approval( ); let actions = advance_approval_state( + ctx.sender(), state, db, metrics, @@ -2704,7 +2846,8 @@ async fn issue_approval( candidate_hash, candidate_entry, ApprovalStateTransition::LocalApproval(validator_index as _, sig.clone()), - ); + ) + .await; metrics.on_approval_produced(); From b0da049a71e8a2fb13f7535e5a392c898f941afd Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 20 Apr 2023 14:00:49 +0300 Subject: [PATCH 02/42] Fix tests in import --- node/core/approval-voting/src/import.rs | 121 +++++++++++++++++++----- node/subsystem-util/src/runtime/mod.rs | 15 +++ 2 files changed, 112 insertions(+), 24 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index 2be583fb4107..b7bcde4c5def 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -618,13 +618,19 @@ pub(crate) async fn handle_new_head( #[cfg(test)] pub(crate) mod tests { use super::*; - use crate::approval_db::v1::DbBackend; + use crate::{approval_db::v1::DbBackend, RuntimeInfo, RuntimeInfoConfig}; use ::test_helpers::{dummy_candidate_receipt, dummy_hash}; use assert_matches::assert_matches; - use polkadot_node_primitives::approval::{VrfSignature, VrfTranscript}; + use polkadot_node_primitives::{ + approval::{VrfSignature, VrfTranscript}, + DISPUTE_WINDOW, + }; use polkadot_node_subsystem::messages::{AllMessages, ApprovalVotingMessage}; use polkadot_node_subsystem_test_helpers::make_subsystem_context; - use polkadot_node_subsystem_util::database::Database; + use polkadot_node_subsystem_util::{ + database::Database, + runtime::{ExtendedSessionInfo, ValidatorInfo}, + }; use polkadot_primitives::{Id as ParaId, IndexedVec, SessionInfo, ValidatorId, ValidatorIndex}; pub(crate) use sp_consensus_babe::{ digests::{CompatibleDigestItem, PreDigest, SecondaryVRFPreDigest}, @@ -633,7 +639,7 @@ pub(crate) mod tests { use sp_core::{crypto::VrfSigner, testing::TaskExecutor}; use sp_keyring::sr25519::Keyring as Sr25519Keyring; pub(crate) use sp_runtime::{Digest, DigestItem}; - use std::{pin::Pin, sync::Arc}; + use std::{num::NonZeroUsize, pin::Pin, sync::Arc}; use crate::{approval_db::v1::Config as DatabaseConfig, criteria, BlockEntry}; @@ -658,24 +664,40 @@ pub(crate) mod tests { } fn blank_state() -> State { - let db = kvdb_memorydb::create(NUM_COLUMNS); - let db = polkadot_node_subsystem_util::database::kvdb_impl::DbAdapter::new(db, &[]); - let db: Arc = Arc::new(db); State { - session_window: None, + session_info: None, keystore: Arc::new(LocalKeystore::in_memory()), slot_duration_millis: 6_000, clock: Box::new(MockClock::default()), assignment_criteria: Box::new(MockAssignmentCriteria), - db, - db_config: TEST_CONFIG, spans: HashMap::new(), } } - fn single_session_state(index: SessionIndex, info: SessionInfo) -> State { + fn single_session_state(index: SessionIndex, info: SessionInfo, relay_parent: Hash) -> State { + let runtime_info = RuntimeInfo::new_with_cache( + RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }, + vec![( + index, + relay_parent, + ExtendedSessionInfo { + session_info: info, + validator_info: ValidatorInfo { our_group: None, our_index: None }, + }, + )], + ); + + //TODO: is it okay to use index for earliest_session and last_consecutive_cached_session? State { - session_window: Some(RollingSessionWindow::with_session_info(index, vec![info])), + session_info: Some(SessionInfoProvider { + earliest_session: index, + last_consecutive_cached_session: index, + runtime_info, + }), ..blank_state() } } @@ -785,13 +807,30 @@ pub(crate) mod tests { .map(|(r, c, g)| (r.hash(), r.clone(), *c, *g)) .collect::>(); - let session_window = - RollingSessionWindow::with_session_info(session, vec![session_info]); + let session_info_provider = SessionInfoProvider { + runtime_info: RuntimeInfo::new_with_cache( + RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }, + vec![( + session, + hash, + ExtendedSessionInfo { + session_info, + validator_info: ValidatorInfo { our_index: None, our_group: None }, + }, + )], + ), + last_consecutive_cached_session: session, //TODO: ?? + earliest_session: session, //TODO: ?? + }; let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - runtime_info: &Some(session_window), + runtime_info: &mut Some(session_info_provider), assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; @@ -891,13 +930,30 @@ pub(crate) mod tests { .collect::>(); let test_fut = { - let session_window = - RollingSessionWindow::with_session_info(session, vec![session_info]); + let session_info_provider = SessionInfoProvider { + runtime_info: RuntimeInfo::new_with_cache( + RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }, + vec![( + session, + hash, + ExtendedSessionInfo { + session_info, + validator_info: ValidatorInfo { our_index: None, our_group: None }, + }, + )], + ), + last_consecutive_cached_session: session, //TODO: ?? + earliest_session: session, //TODO: ?? + }; let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - runtime_info: &Some(session_window), + runtime_info: &mut Some(session_info_provider), assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; @@ -991,12 +1047,12 @@ pub(crate) mod tests { .collect::>(); let test_fut = { - let session_window = None; + let mut runtime_info = None; let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - runtime_info: &session_window, + runtime_info: &mut runtime_info, assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; @@ -1086,13 +1142,30 @@ pub(crate) mod tests { .map(|(r, c, g)| (r.hash(), r.clone(), *c, *g)) .collect::>(); - let session_window = - Some(RollingSessionWindow::with_session_info(session, vec![session_info])); + let session_info_provider = SessionInfoProvider { + runtime_info: RuntimeInfo::new_with_cache( + RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }, + vec![( + session, + hash, + ExtendedSessionInfo { + session_info, + validator_info: ValidatorInfo { our_index: None, our_group: None }, + }, + )], + ), + last_consecutive_cached_session: session, //TODO: ?? + earliest_session: session, //TODO: ?? + }; let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - runtime_info: &session_window, + runtime_info: &mut Some(session_info_provider), assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; @@ -1229,7 +1302,7 @@ pub(crate) mod tests { .map(|(r, c, g)| CandidateEvent::CandidateIncluded(r, Vec::new().into(), c, g)) .collect::>(); - let mut state = single_session_state(session, session_info); + let mut state = single_session_state(session, session_info, parent_hash); overlay_db.write_block_entry( v1::BlockEntry { block_hash: parent_hash, diff --git a/node/subsystem-util/src/runtime/mod.rs b/node/subsystem-util/src/runtime/mod.rs index 6e06b99bbe03..87122d024fd2 100644 --- a/node/subsystem-util/src/runtime/mod.rs +++ b/node/subsystem-util/src/runtime/mod.rs @@ -118,6 +118,21 @@ impl RuntimeInfo { } } + /// Create an instance with pre-populated cache. Used only for testing + // TODO: why doesn't compile + // #[cfg(test)] + pub fn new_with_cache( + cfg: Config, + data: Vec<(SessionIndex, Hash, ExtendedSessionInfo)>, + ) -> Self { + let mut r = Self::new_with_config(cfg); + for (idx, parent, session) in data { + r.session_index_cache.put(parent, idx); + r.session_info_cache.put(idx, session); + } + r + } + /// Returns the session index expected at any child of the `parent` block. /// This does not return the session index for the `parent` block. pub async fn get_session_index_for_child( From 93e6863364b657249b8c644049e79d76dd77b39e Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 20 Apr 2023 14:43:19 +0300 Subject: [PATCH 03/42] Fix the rest of the tests --- node/core/approval-voting/src/tests.rs | 31 -------------------------- 1 file changed, 31 deletions(-) diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index 9ec0c89d4baa..059ab09bc705 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -815,37 +815,6 @@ async fn import_block( ); if !fork { - assert_matches!( - overseer_recv(overseer).await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockNumber( - s_tx, - )) => { - let _ = s_tx.send(Ok(number)); - } - ); - - assert_matches!( - overseer_recv(overseer).await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockHash( - block_number, - s_tx, - )) => { - assert_eq!(block_number, number); - let _ = s_tx.send(Ok(Some(hashes[number as usize].0))); - } - ); - - assert_matches!( - overseer_recv(overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, hashes[number as usize].0); - let _ = s_tx.send(Ok(number.into())); - } - ); - assert_matches!( overseer_recv(overseer).await, AllMessages::RuntimeApi( From a7b74ccbfd520146765e20cca9641f7e4ae8e213 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 20 Apr 2023 21:59:48 +0300 Subject: [PATCH 04/42] Remove dead code --- node/core/approval-voting/src/import.rs | 17 ----------------- node/subsystem-util/src/runtime/mod.rs | 2 -- 2 files changed, 19 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index b7bcde4c5def..0fbd2f7c5b06 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -366,23 +366,6 @@ pub(crate) async fn handle_new_head( // Update session info based on most recent head. state.cache_session_info_for_head(ctx, head).await; - // Err(e) => { - // gum::debug!( - // target: LOG_TARGET, - // ?head, - // ?e, - // "Could not cache session info when processing head.", - // ); - // return Ok(Vec::new()) - // }, - // Ok(Some(a @ SessionWindowUpdate::Advanced { .. })) => { - // gum::info!( - // target: LOG_TARGET, - // update = ?a, - // "Advanced session window for approvals", - // ); - // }, - // Ok(_) => {},s // If we've just started the node and are far behind, // import at most `MAX_HEADS_LOOK_BACK` blocks. diff --git a/node/subsystem-util/src/runtime/mod.rs b/node/subsystem-util/src/runtime/mod.rs index 87122d024fd2..468ba94449e7 100644 --- a/node/subsystem-util/src/runtime/mod.rs +++ b/node/subsystem-util/src/runtime/mod.rs @@ -119,8 +119,6 @@ impl RuntimeInfo { } /// Create an instance with pre-populated cache. Used only for testing - // TODO: why doesn't compile - // #[cfg(test)] pub fn new_with_cache( cfg: Config, data: Vec<(SessionIndex, Hash, ExtendedSessionInfo)>, From 0f84b42bb460f96957cef7d30c3a675382be30df Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 21 Apr 2023 09:56:03 +0300 Subject: [PATCH 05/42] Fix todos --- node/core/approval-voting/src/lib.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 9c6f71c78f16..b3deb7d54677 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -637,7 +637,6 @@ impl CurrentlyCheckingSet { } } -// TODO: better name struct SessionInfoProvider { runtime_info: RuntimeInfo, last_consecutive_cached_session: SessionIndex, @@ -672,7 +671,12 @@ impl State { { Ok(extended_info) => Some(&extended_info.session_info), Err(_) => { - // todo log error + gum::error!( + target: LOG_TARGET, + session: session_index, + ?relay_parent, + "Can't get SessionInfo" + ); None }, }, From 0e486d487ab5731f8c7ff3a11b3c985927a57f34 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 21 Apr 2023 11:30:02 +0300 Subject: [PATCH 06/42] Simplify session caching --- node/core/approval-voting/src/import.rs | 19 ++++---- node/core/approval-voting/src/lib.rs | 64 ++++++++++++++----------- 2 files changed, 44 insertions(+), 39 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index 0fbd2f7c5b06..870cc52309e2 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -158,7 +158,7 @@ async fn imported_block_info( return Err(ImportedBlockInfoError::FutureCancelled("SessionIndexForChild", error)), }; - if env.runtime_info.as_ref().map_or(true, |s| session_index < s.earliest_session) { + if env.runtime_info.as_ref().map_or(true, |s| session_index < s.earliest_session()) { gum::debug!( target: LOG_TARGET, "Block {} is from ancient session {}. Skipping", @@ -674,11 +674,10 @@ pub(crate) mod tests { )], ); - //TODO: is it okay to use index for earliest_session and last_consecutive_cached_session? State { session_info: Some(SessionInfoProvider { - earliest_session: index, - last_consecutive_cached_session: index, + highest_session_seen: index, + gaps_in_cache: false, runtime_info, }), ..blank_state() @@ -806,8 +805,8 @@ pub(crate) mod tests { }, )], ), - last_consecutive_cached_session: session, //TODO: ?? - earliest_session: session, //TODO: ?? + gaps_in_cache: false, + highest_session_seen: session, }; let header = header.clone(); @@ -929,8 +928,8 @@ pub(crate) mod tests { }, )], ), - last_consecutive_cached_session: session, //TODO: ?? - earliest_session: session, //TODO: ?? + gaps_in_cache: false, + highest_session_seen: session, }; let header = header.clone(); @@ -1141,8 +1140,8 @@ pub(crate) mod tests { }, )], ), - last_consecutive_cached_session: session, //TODO: ?? - earliest_session: session, //TODO: ?? + gaps_in_cache: false, + highest_session_seen: session, }; let header = header.clone(); diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index b3deb7d54677..fd76e2063cdb 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -639,11 +639,18 @@ impl CurrentlyCheckingSet { struct SessionInfoProvider { runtime_info: RuntimeInfo, - last_consecutive_cached_session: SessionIndex, - earliest_session: SessionIndex, + gaps_in_cache: bool, + highest_session_seen: SessionIndex, +} + +impl SessionInfoProvider { + fn earliest_session(&self) -> SessionIndex { + self.highest_session_seen.saturating_sub(DISPUTE_WINDOW.get() - 1) + } } struct State { + // `None` on start-up. Gets initialized/updated on leaf update session_info: Option, keystore: Arc, slot_duration_millis: u64, @@ -673,7 +680,7 @@ impl State { Err(_) => { gum::error!( target: LOG_TARGET, - session: session_index, + session = session_index, ?relay_parent, "Can't get SessionInfo" ); @@ -712,35 +719,28 @@ impl State { }, }; - let mut gap_in_cache = false; - let mut last_consecutive_cached_session = 0; - // TODO: fix this -> start of the windoow should be no less than the last finalized block - for idx in head_session_idx.saturating_sub(DISPUTE_WINDOW.get())..=head_session_idx + let mut gaps_in_cache = false; + for idx in + head_session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1)..=head_session_idx { if let Err(err) = runtime_info.get_session_info_by_index(ctx.sender(), head, idx).await { - gap_in_cache = true; + gaps_in_cache = true; gum::debug!( target: LOG_TARGET, ?err, session = idx, - "Can cache session. Moving on." + "Can't cache session. Moving on." ); continue } - - if !gap_in_cache { - last_consecutive_cached_session = idx; - } } - // TODO: fix this - let earliest_session = 0; self.session_info = Some(SessionInfoProvider { runtime_info, - last_consecutive_cached_session, - earliest_session, + gaps_in_cache, + highest_session_seen: head_session_idx, }); }, Some(mut session_info_provider) => { @@ -761,17 +761,23 @@ impl State { }, }; - let mut gap_in_cache = false; - if head_session_idx > session_info_provider.last_consecutive_cached_session { - for idx in - session_info_provider.last_consecutive_cached_session + 1..=head_session_idx - { + let mut gaps_in_cache = false; + if session_info_provider.gaps_in_cache || + head_session_idx > session_info_provider.highest_session_seen + { + let lower_bound = if session_info_provider.gaps_in_cache { + head_session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1) + } else { + session_info_provider.highest_session_seen + 1 + }; + + for idx in lower_bound..=head_session_idx { if let Err(err) = session_info_provider .runtime_info .get_session_info_by_index(ctx.sender(), head, idx) .await { - gap_in_cache = true; + gaps_in_cache = true; gum::debug!( target: LOG_TARGET, ?err, @@ -780,14 +786,14 @@ impl State { ); continue } - - if !gap_in_cache { - session_info_provider.last_consecutive_cached_session = idx; - } } - } - // TODO: update `earliest_session` + session_info_provider = SessionInfoProvider { + runtime_info: session_info_provider.runtime_info, + gaps_in_cache, + highest_session_seen: head_session_idx, + }; + } self.session_info = Some(session_info_provider); }, From f6a9d98539fbf3b127a8247efb6316d48fafdbb5 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 21 Apr 2023 11:52:45 +0300 Subject: [PATCH 07/42] Comments for `SessionInfoProvider` --- node/core/approval-voting/src/lib.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index fd76e2063cdb..ce4c9e44033b 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -637,9 +637,14 @@ impl CurrentlyCheckingSet { } } +// Wraps `RuntimeInfo` and some metadata. On each new leaf `SessionInfo` is +// cached. `RuntimeInfo` keeps the last `DISPUTE_WINDOW` number of sessions. struct SessionInfoProvider { + // `RuntimeInfo` caches sessions internally. runtime_info: RuntimeInfo, + // Will be true if an error had occurred during the last session caching attempt gaps_in_cache: bool, + // Highest session index seen so far. Also used to calculate the earliest one. highest_session_seen: SessionIndex, } From 2918b42b69376a6af3139e865b055f2db2493595 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 21 Apr 2023 16:26:26 +0300 Subject: [PATCH 08/42] Separate `SessionInfoProvider` from `State` --- node/core/approval-voting/src/import.rs | 45 +++-- node/core/approval-voting/src/lib.rs | 208 ++++++++++++++++-------- 2 files changed, 165 insertions(+), 88 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index 870cc52309e2..4b894824e647 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -330,6 +330,7 @@ pub(crate) async fn handle_new_head( ctx: &mut Context, state: &mut State, db: &mut OverlayedBackend<'_, B>, + session_info_provider: &mut Option, head: Hash, finalized_number: &Option, ) -> SubsystemResult> { @@ -365,7 +366,7 @@ pub(crate) async fn handle_new_head( }; // Update session info based on most recent head. - state.cache_session_info_for_head(ctx, head).await; + state.cache_session_info_for_head(ctx, head, session_info_provider).await; // If we've just started the node and are far behind, // import at most `MAX_HEADS_LOOK_BACK` blocks. @@ -394,7 +395,7 @@ pub(crate) async fn handle_new_head( let mut imported_blocks_and_info = Vec::with_capacity(new_blocks.len()); for (block_hash, block_header) in new_blocks.into_iter().rev() { let env = ImportedBlockInfoEnv { - runtime_info: &mut state.session_info, + runtime_info: session_info_provider, assignment_criteria: &*state.assignment_criteria, keystore: &state.keystore, }; @@ -449,8 +450,7 @@ pub(crate) async fn handle_new_head( } = imported_block_info; // todo: refactor this - let session_info = &state - .session_info + let session_info = &session_info_provider .as_mut() .map(|s| &mut s.runtime_info) .expect("imported_block_info requires session info to be available; qed") @@ -648,7 +648,6 @@ pub(crate) mod tests { fn blank_state() -> State { State { - session_info: None, keystore: Arc::new(LocalKeystore::in_memory()), slot_duration_millis: 6_000, clock: Box::new(MockClock::default()), @@ -657,7 +656,11 @@ pub(crate) mod tests { } } - fn single_session_state(index: SessionIndex, info: SessionInfo, relay_parent: Hash) -> State { + fn single_session_state( + index: SessionIndex, + info: SessionInfo, + relay_parent: Hash, + ) -> (State, SessionInfoProvider) { let runtime_info = RuntimeInfo::new_with_cache( RuntimeInfoConfig { keystore: None, @@ -674,14 +677,12 @@ pub(crate) mod tests { )], ); - State { - session_info: Some(SessionInfoProvider { - highest_session_seen: index, - gaps_in_cache: false, - runtime_info, - }), - ..blank_state() - } + let state = blank_state(); + + let session_info_provider = + SessionInfoProvider { runtime_info, gaps_in_cache: false, highest_session_seen: index }; + + (state, session_info_provider) } struct MockAssignmentCriteria; @@ -1284,7 +1285,8 @@ pub(crate) mod tests { .map(|(r, c, g)| CandidateEvent::CandidateIncluded(r, Vec::new().into(), c, g)) .collect::>(); - let mut state = single_session_state(session, session_info, parent_hash); + let (mut state, session_info_provider) = + single_session_state(session, session_info, parent_hash); overlay_db.write_block_entry( v1::BlockEntry { block_hash: parent_hash, @@ -1306,9 +1308,16 @@ pub(crate) mod tests { let test_fut = { Box::pin(async move { let mut overlay_db = OverlayedBackend::new(&db); - let result = handle_new_head(&mut ctx, &mut state, &mut overlay_db, hash, &Some(1)) - .await - .unwrap(); + let result = handle_new_head( + &mut ctx, + &mut state, + &mut overlay_db, + &mut Some(session_info_provider), + hash, + &Some(1), + ) + .await + .unwrap(); let write_ops = overlay_db.into_write_ops(); db.write(write_ops).unwrap(); diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index ce4c9e44033b..1d22b5e97efd 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -655,8 +655,6 @@ impl SessionInfoProvider { } struct State { - // `None` on start-up. Gets initialized/updated on leaf update - session_info: Option, keystore: Arc, slot_duration_millis: u64, clock: Box, @@ -666,43 +664,16 @@ struct State { #[overseer::contextbounds(ApprovalVoting, prefix = self::overseer)] impl State { - async fn session_info( - &mut self, - sender: &mut Sender, - relay_parent: Hash, - session_index: SessionIndex, - ) -> Option<&SessionInfo> - where - Sender: SubsystemSender, - { - match &mut self.session_info { - Some(session_info_provider) => match session_info_provider - .runtime_info - .get_session_info_by_index(sender, relay_parent, session_index) - .await - { - Ok(extended_info) => Some(&extended_info.session_info), - Err(_) => { - gum::error!( - target: LOG_TARGET, - session = session_index, - ?relay_parent, - "Can't get SessionInfo" - ); - None - }, - }, - - None => None, - } - } - /// If `head` is in a new session - cache it - pub async fn cache_session_info_for_head(&mut self, ctx: &mut Context, head: Hash) - where + pub async fn cache_session_info_for_head( + &mut self, + ctx: &mut Context, + head: Hash, + session_info: &mut Option, + ) where ::Sender: Sized + Send, { - match self.session_info.take() { + match session_info.take() { None => { let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { keystore: None, @@ -742,7 +713,7 @@ impl State { } } - self.session_info = Some(SessionInfoProvider { + *session_info = Some(SessionInfoProvider { runtime_info, gaps_in_cache, highest_session_seen: head_session_idx, @@ -800,16 +771,18 @@ impl State { }; } - self.session_info = Some(session_info_provider); + *session_info = Some(session_info_provider); }, } } + // Compute the required tranches for approval for this block and candidate combo. // Fails if there is no approval entry for the block under the candidate or no candidate entry // under the block, or if the session is out of bounds. async fn approval_status( &'a mut self, sender: &mut Sender, + session_info_provider: &'a mut Option, block_entry: &'a BlockEntry, candidate_entry: &'b CandidateEntry, ) -> Option<(&'b ApprovalEntry, ApprovalStatus)> @@ -817,9 +790,13 @@ impl State { Sender: SubsystemSender, { // We can't borrow the session here. Only get copies of what's needed. - let (no_show_slots, needed_approvals) = match self - .session_info(sender, block_entry.parent_hash(), block_entry.session()) - .await + let (no_show_slots, needed_approvals) = match session_info( + session_info_provider, + sender, + block_entry.parent_hash(), + block_entry.session(), + ) + .await { Some(s) => (s.no_show_slots, s.needed_approvals), None => { @@ -897,7 +874,6 @@ where } let mut state = State { - session_info: None, keystore: subsystem.keystore, slot_duration_millis: subsystem.slot_duration_millis, clock, @@ -905,6 +881,9 @@ where spans: HashMap::new(), }; + // `None` on start-up. Gets initialized/updated on leaf update + let mut session_info_provider: Option = None; + let mut wakeups = Wakeups::default(); let mut currently_checking_set = CurrentlyCheckingSet::default(); let mut approvals_cache = lru::LruCache::new(APPROVAL_CACHE_SIZE); @@ -930,6 +909,7 @@ where &mut ctx, &mut state, &mut overlayed_db, + &mut session_info_provider, woken_block, woken_candidate, &subsystem.metrics, @@ -940,6 +920,7 @@ where &mut ctx, &mut state, &mut overlayed_db, + &mut session_info_provider, &subsystem.metrics, next_msg?, &mut last_finalized_height, @@ -990,6 +971,7 @@ where &mut ctx, &mut state, &mut overlayed_db, + &mut session_info_provider, &subsystem.metrics, &mut wakeups, &mut currently_checking_set, @@ -1036,6 +1018,7 @@ async fn handle_actions( ctx: &mut Context, state: &mut State, overlayed_db: &mut OverlayedBackend<'_, impl Backend>, + session_info_provider: &mut Option, metrics: &Metrics, wakeups: &mut Wakeups, currently_checking_set: &mut CurrentlyCheckingSet, @@ -1066,6 +1049,7 @@ async fn handle_actions( ctx, state, overlayed_db, + session_info_provider, metrics, candidate_hash, approval_request, @@ -1293,6 +1277,7 @@ async fn handle_from_overseer( ctx: &mut Context, state: &mut State, db: &mut OverlayedBackend<'_, impl Backend>, + session_info_provider: &mut Option, metrics: &Metrics, x: FromOrchestra, last_finalized_height: &mut Option, @@ -1306,7 +1291,16 @@ async fn handle_from_overseer( let approval_voting_span = jaeger::PerLeafSpan::new(activated.span, "approval-voting"); state.spans.insert(head, approval_voting_span); - match import::handle_new_head(ctx, state, db, head, &*last_finalized_height).await { + match import::handle_new_head( + ctx, + state, + db, + session_info_provider, + head, + &*last_finalized_height, + ) + .await + { Err(e) => return Err(SubsystemError::with_origin("db", e)), Ok(block_imported_candidates) => { // Schedule wakeups for all imported candidates. @@ -1376,16 +1370,31 @@ async fn handle_from_overseer( }, FromOrchestra::Communication { msg } => match msg { ApprovalVotingMessage::CheckAndImportAssignment(a, claimed_core, res) => { - let (check_outcome, actions) = - check_and_import_assignment(ctx.sender(), state, db, a, claimed_core).await?; + let (check_outcome, actions) = check_and_import_assignment( + ctx.sender(), + state, + db, + session_info_provider, + a, + claimed_core, + ) + .await?; let _ = res.send(check_outcome); actions }, ApprovalVotingMessage::CheckAndImportApproval(a, res) => - check_and_import_approval(ctx.sender(), state, db, metrics, a, |r| { - let _ = res.send(r); - }) + check_and_import_approval( + ctx.sender(), + state, + db, + session_info_provider, + metrics, + a, + |r| { + let _ = res.send(r); + }, + ) .await? .0, ApprovalVotingMessage::ApprovedAncestor(target, lower_bound, res) => { @@ -1860,6 +1869,7 @@ async fn check_and_import_assignment( sender: &mut Sender, state: &mut State, db: &mut OverlayedBackend<'_, impl Backend>, + session_info_provider: &mut Option, assignment: IndirectAssignmentCert, candidate_index: CandidateIndex, ) -> SubsystemResult<(AssignmentCheckResult, Vec)> @@ -1888,9 +1898,13 @@ where )), }; - let session_info = match state - .session_info(sender, block_entry.parent_hash(), block_entry.session()) - .await + let session_info = match session_info( + session_info_provider, + sender, + block_entry.parent_hash(), + block_entry.session(), + ) + .await { Some(s) => s, None => @@ -2003,8 +2017,9 @@ where let mut actions = Vec::new(); // We've imported a new approval, so we need to schedule a wake-up for when that might no-show. - if let Some((approval_entry, status)) = - state.approval_status(sender, &block_entry, &candidate_entry).await + if let Some((approval_entry, status)) = state + .approval_status(sender, session_info_provider, &block_entry, &candidate_entry) + .await { actions.extend(schedule_wakeup_action( approval_entry, @@ -2027,6 +2042,7 @@ async fn check_and_import_approval( sender: &mut Sender, state: &mut State, db: &mut OverlayedBackend<'_, impl Backend>, + session_info_provider: &mut Option, metrics: &Metrics, approval: IndirectSignedApprovalVote, with_response: impl FnOnce(ApprovalCheckResult) -> T, @@ -2059,15 +2075,21 @@ where }, }; - let session_info = - match state.session_info(sender, approval.block_hash, block_entry.session()).await { - Some(s) => s, - None => { - respond_early!(ApprovalCheckResult::Bad(ApprovalCheckError::UnknownSessionIndex( - block_entry.session() - ),)) - }, - }; + let session_info = match session_info( + session_info_provider, + sender, + approval.block_hash, + block_entry.session(), + ) + .await + { + Some(s) => s, + None => { + respond_early!(ApprovalCheckResult::Bad(ApprovalCheckError::UnknownSessionIndex( + block_entry.session() + ),)) + }, + }; let approved_candidate_hash = match block_entry.candidate(approval.candidate_index as usize) { Some((_, h)) => *h, @@ -2144,6 +2166,7 @@ where sender, state, db, + session_info_provider, &metrics, block_entry, approved_candidate_hash, @@ -2190,6 +2213,7 @@ async fn advance_approval_state( sender: &mut Sender, state: &mut State, db: &mut OverlayedBackend<'_, impl Backend>, + session_info_provider: &mut Option, metrics: &Metrics, mut block_entry: BlockEntry, candidate_hash: CandidateHash, @@ -2228,8 +2252,9 @@ where let tick_now = state.clock.tick_now(); - let (is_approved, status) = if let Some((approval_entry, status)) = - state.approval_status(sender, &block_entry, &candidate_entry).await + let (is_approved, status) = if let Some((approval_entry, status)) = state + .approval_status(sender, session_info_provider, &block_entry, &candidate_entry) + .await { let check = approval_checking::check_approval( &candidate_entry, @@ -2364,6 +2389,7 @@ async fn process_wakeup( ctx: &mut Context, state: &mut State, db: &mut OverlayedBackend<'_, impl Backend>, + session_info_provider: &mut Option, relay_block: Hash, candidate_hash: CandidateHash, metrics: &Metrics, @@ -2389,9 +2415,13 @@ async fn process_wakeup( let slot_duration_millis = state.slot_duration_millis; let tranche_now = state.clock.tranche_now(slot_duration_millis, block_entry.slot()); - let session_info = match state - .session_info(ctx.sender(), block_entry.parent_hash(), block_entry.session()) - .await + let session_info = match session_info( + session_info_provider, + ctx.sender(), + block_entry.parent_hash(), + block_entry.session(), + ) + .await { Some(i) => i, None => { @@ -2507,6 +2537,7 @@ async fn process_wakeup( ctx.sender(), state, db, + session_info_provider, metrics, block_entry, candidate_hash, @@ -2724,6 +2755,7 @@ async fn issue_approval( ctx: &mut Context, state: &mut State, db: &mut OverlayedBackend<'_, impl Backend>, + session_info_provider: &mut Option, metrics: &Metrics, candidate_hash: CandidateHash, ApprovalVoteRequest { validator_index, block_hash }: ApprovalVoteRequest, @@ -2765,9 +2797,13 @@ async fn issue_approval( }; issue_approval_span.add_int_tag("candidate_index", candidate_index as i64); - let session_info = match state - .session_info(ctx.sender(), block_entry.parent_hash(), block_entry.session()) - .await + let session_info = match session_info( + session_info_provider, + ctx.sender(), + block_entry.parent_hash(), + block_entry.session(), + ) + .await { Some(s) => s, None => { @@ -2856,6 +2892,7 @@ async fn issue_approval( ctx.sender(), state, db, + session_info_provider, metrics, block_entry, candidate_hash, @@ -2918,3 +2955,34 @@ fn issue_local_invalid_statement( false, )); } + +async fn session_info<'a, Sender>( + session_info_provider: &'a mut Option, + sender: &mut Sender, + relay_parent: Hash, + session_index: SessionIndex, +) -> Option<&'a SessionInfo> +where + Sender: SubsystemSender, +{ + match session_info_provider { + Some(session_info_provider) => match session_info_provider + .runtime_info + .get_session_info_by_index(sender, relay_parent, session_index) + .await + { + Ok(extended_info) => Some(&extended_info.session_info), + Err(_) => { + gum::error!( + target: LOG_TARGET, + session = session_index, + ?relay_parent, + "Can't get SessionInfo" + ); + None + }, + }, + + None => None, + } +} From 837f952b84110a09aa36f2ad0295d88cab441536 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 21 Apr 2023 16:42:44 +0300 Subject: [PATCH 09/42] `cache_session_info_for_head` becomes freestanding function --- node/core/approval-voting/src/import.rs | 3 +- node/core/approval-voting/src/lib.rs | 220 ++++++++++++------------ 2 files changed, 110 insertions(+), 113 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index 4b894824e647..73e58430392b 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -56,6 +56,7 @@ use std::collections::HashMap; use super::approval_db::v1; use crate::{ backend::{Backend, OverlayedBackend}, + cache_session_info_for_head, criteria::{AssignmentCriteria, OurAssignment}, persisted_entries::CandidateEntry, time::{slot_number_to_tick, Tick}, @@ -366,7 +367,7 @@ pub(crate) async fn handle_new_head( }; // Update session info based on most recent head. - state.cache_session_info_for_head(ctx, head, session_info_provider).await; + cache_session_info_for_head(ctx.sender(), head, session_info_provider).await; // If we've just started the node and are far behind, // import at most `MAX_HEADS_LOOK_BACK` blocks. diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 1d22b5e97efd..70efd2b45389 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -664,118 +664,6 @@ struct State { #[overseer::contextbounds(ApprovalVoting, prefix = self::overseer)] impl State { - /// If `head` is in a new session - cache it - pub async fn cache_session_info_for_head( - &mut self, - ctx: &mut Context, - head: Hash, - session_info: &mut Option, - ) where - ::Sender: Sized + Send, - { - match session_info.take() { - None => { - let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { - keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), - }); - - let head_session_idx = - match runtime_info.get_session_index_for_child(ctx.sender(), head).await { - Ok(session_idx) => session_idx, - Err(err) => { - gum::debug!( - target: LOG_TARGET, - ?head, - ?err, - "Error getting session index for head. Won't cache any sessions" - ); - return - }, - }; - - let mut gaps_in_cache = false; - for idx in - head_session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1)..=head_session_idx - { - if let Err(err) = - runtime_info.get_session_info_by_index(ctx.sender(), head, idx).await - { - gaps_in_cache = true; - gum::debug!( - target: LOG_TARGET, - ?err, - session = idx, - "Can't cache session. Moving on." - ); - continue - } - } - - *session_info = Some(SessionInfoProvider { - runtime_info, - gaps_in_cache, - highest_session_seen: head_session_idx, - }); - }, - Some(mut session_info_provider) => { - let head_session_idx = match session_info_provider - .runtime_info - .get_session_index_for_child(ctx.sender(), head) - .await - { - Ok(session_idx) => session_idx, - Err(err) => { - gum::debug!( - target: LOG_TARGET, - ?head, - ?err, - "Error getting session index for head. Won't cache any sessions" - ); - return - }, - }; - - let mut gaps_in_cache = false; - if session_info_provider.gaps_in_cache || - head_session_idx > session_info_provider.highest_session_seen - { - let lower_bound = if session_info_provider.gaps_in_cache { - head_session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1) - } else { - session_info_provider.highest_session_seen + 1 - }; - - for idx in lower_bound..=head_session_idx { - if let Err(err) = session_info_provider - .runtime_info - .get_session_info_by_index(ctx.sender(), head, idx) - .await - { - gaps_in_cache = true; - gum::debug!( - target: LOG_TARGET, - ?err, - session = idx, - "Can cache session. Moving on." - ); - continue - } - } - - session_info_provider = SessionInfoProvider { - runtime_info: session_info_provider.runtime_info, - gaps_in_cache, - highest_session_seen: head_session_idx, - }; - } - - *session_info = Some(session_info_provider); - }, - } - } - // Compute the required tranches for approval for this block and candidate combo. // Fails if there is no approval entry for the block under the candidate or no candidate entry // under the block, or if the session is out of bounds. @@ -2986,3 +2874,111 @@ where None => None, } } + +/// If `head` is in a new session - cache it +async fn cache_session_info_for_head( + sender: &mut Sender, + head: Hash, + session_info: &mut Option, +) where + Sender: SubsystemSender, +{ + match session_info.take() { + None => { + let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }); + + let head_session_idx = + match runtime_info.get_session_index_for_child(sender, head).await { + Ok(session_idx) => session_idx, + Err(err) => { + gum::debug!( + target: LOG_TARGET, + ?head, + ?err, + "Error getting session index for head. Won't cache any sessions" + ); + return + }, + }; + + let mut gaps_in_cache = false; + for idx in head_session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1)..=head_session_idx + { + if let Err(err) = runtime_info.get_session_info_by_index(sender, head, idx).await { + gaps_in_cache = true; + gum::debug!( + target: LOG_TARGET, + ?err, + session = idx, + "Can't cache session. Moving on." + ); + continue + } + } + + *session_info = Some(SessionInfoProvider { + runtime_info, + gaps_in_cache, + highest_session_seen: head_session_idx, + }); + }, + Some(mut session_info_provider) => { + let head_session_idx = match session_info_provider + .runtime_info + .get_session_index_for_child(sender, head) + .await + { + Ok(session_idx) => session_idx, + Err(err) => { + gum::debug!( + target: LOG_TARGET, + ?head, + ?err, + "Error getting session index for head. Won't cache any sessions" + ); + return + }, + }; + + let mut gaps_in_cache = false; + if session_info_provider.gaps_in_cache || + head_session_idx > session_info_provider.highest_session_seen + { + let lower_bound = if session_info_provider.gaps_in_cache { + head_session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1) + } else { + session_info_provider.highest_session_seen + 1 + }; + + for idx in lower_bound..=head_session_idx { + if let Err(err) = session_info_provider + .runtime_info + .get_session_info_by_index(sender, head, idx) + .await + { + gaps_in_cache = true; + gum::debug!( + target: LOG_TARGET, + ?err, + session = idx, + "Can cache session. Moving on." + ); + continue + } + } + + session_info_provider = SessionInfoProvider { + runtime_info: session_info_provider.runtime_info, + gaps_in_cache, + highest_session_seen: head_session_idx, + }; + } + + *session_info = Some(session_info_provider); + }, + } +} From 6f609f3eb52619ed7ec51e3c4ce6c92b76b370c9 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 21 Apr 2023 16:49:20 +0300 Subject: [PATCH 10/42] Remove unneeded `mut` usage --- node/core/approval-voting/src/import.rs | 6 +++--- node/core/approval-voting/src/lib.rs | 20 ++++++++++---------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index 73e58430392b..540375a6c028 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -329,7 +329,7 @@ pub struct BlockImportedCandidates { #[overseer::contextbounds(ApprovalVoting, prefix = self::overseer)] pub(crate) async fn handle_new_head( ctx: &mut Context, - state: &mut State, + state: &State, db: &mut OverlayedBackend<'_, B>, session_info_provider: &mut Option, head: Hash, @@ -1286,7 +1286,7 @@ pub(crate) mod tests { .map(|(r, c, g)| CandidateEvent::CandidateIncluded(r, Vec::new().into(), c, g)) .collect::>(); - let (mut state, session_info_provider) = + let (state, session_info_provider) = single_session_state(session, session_info, parent_hash); overlay_db.write_block_entry( v1::BlockEntry { @@ -1311,7 +1311,7 @@ pub(crate) mod tests { let mut overlay_db = OverlayedBackend::new(&db); let result = handle_new_head( &mut ctx, - &mut state, + &state, &mut overlay_db, &mut Some(session_info_provider), hash, diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 70efd2b45389..19e9c23f443a 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -668,7 +668,7 @@ impl State { // Fails if there is no approval entry for the block under the candidate or no candidate entry // under the block, or if the session is out of bounds. async fn approval_status( - &'a mut self, + &'a self, sender: &mut Sender, session_info_provider: &'a mut Option, block_entry: &'a BlockEntry, @@ -795,7 +795,7 @@ where subsystem.metrics.on_wakeup(); process_wakeup( &mut ctx, - &mut state, + &state, &mut overlayed_db, &mut session_info_provider, woken_block, @@ -857,7 +857,7 @@ where if handle_actions( &mut ctx, - &mut state, + &state, &mut overlayed_db, &mut session_info_provider, &subsystem.metrics, @@ -904,7 +904,7 @@ where #[overseer::contextbounds(ApprovalVoting, prefix = self::overseer)] async fn handle_actions( ctx: &mut Context, - state: &mut State, + state: &State, overlayed_db: &mut OverlayedBackend<'_, impl Backend>, session_info_provider: &mut Option, metrics: &Metrics, @@ -1051,7 +1051,7 @@ async fn handle_actions( fn distribution_messages_for_activation( db: &OverlayedBackend<'_, impl Backend>, - state: &mut State, + state: &State, ) -> SubsystemResult> { let all_blocks: Vec = db.load_all_blocks()?; @@ -1755,7 +1755,7 @@ fn schedule_wakeup_action( async fn check_and_import_assignment( sender: &mut Sender, - state: &mut State, + state: &State, db: &mut OverlayedBackend<'_, impl Backend>, session_info_provider: &mut Option, assignment: IndirectAssignmentCert, @@ -1928,7 +1928,7 @@ where async fn check_and_import_approval( sender: &mut Sender, - state: &mut State, + state: &State, db: &mut OverlayedBackend<'_, impl Backend>, session_info_provider: &mut Option, metrics: &Metrics, @@ -2099,7 +2099,7 @@ impl ApprovalStateTransition { // necessary and schedules any further wakeups. async fn advance_approval_state( sender: &mut Sender, - state: &mut State, + state: &State, db: &mut OverlayedBackend<'_, impl Backend>, session_info_provider: &mut Option, metrics: &Metrics, @@ -2275,7 +2275,7 @@ fn should_trigger_assignment( #[overseer::contextbounds(ApprovalVoting, prefix = self::overseer)] async fn process_wakeup( ctx: &mut Context, - state: &mut State, + state: &State, db: &mut OverlayedBackend<'_, impl Backend>, session_info_provider: &mut Option, relay_block: Hash, @@ -2641,7 +2641,7 @@ async fn launch_approval( #[overseer::contextbounds(ApprovalVoting, prefix = self::overseer)] async fn issue_approval( ctx: &mut Context, - state: &mut State, + state: &State, db: &mut OverlayedBackend<'_, impl Backend>, session_info_provider: &mut Option, metrics: &Metrics, From 82a0ebc0aeb0a0b5a70abaec80262ef827eaf102 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 24 Apr 2023 11:19:02 +0300 Subject: [PATCH 11/42] fn session_info -> fn get_session_info() to avoid name clashes. The function also tries to initialize `SessionInfoProvider` --- node/core/approval-voting/src/lib.rs | 64 +++++++++++++++++----------- 1 file changed, 40 insertions(+), 24 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 19e9c23f443a..dd69931550be 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -678,7 +678,7 @@ impl State { Sender: SubsystemSender, { // We can't borrow the session here. Only get copies of what's needed. - let (no_show_slots, needed_approvals) = match session_info( + let (no_show_slots, needed_approvals) = match get_session_info( session_info_provider, sender, block_entry.parent_hash(), @@ -1786,7 +1786,7 @@ where )), }; - let session_info = match session_info( + let session_info = match get_session_info( session_info_provider, sender, block_entry.parent_hash(), @@ -1963,7 +1963,7 @@ where }, }; - let session_info = match session_info( + let session_info = match get_session_info( session_info_provider, sender, approval.block_hash, @@ -2303,7 +2303,7 @@ async fn process_wakeup( let slot_duration_millis = state.slot_duration_millis; let tranche_now = state.clock.tranche_now(slot_duration_millis, block_entry.slot()); - let session_info = match session_info( + let session_info = match get_session_info( session_info_provider, ctx.sender(), block_entry.parent_hash(), @@ -2685,7 +2685,7 @@ async fn issue_approval( }; issue_approval_span.add_int_tag("candidate_index", candidate_index as i64); - let session_info = match session_info( + let session_info = match get_session_info( session_info_provider, ctx.sender(), block_entry.parent_hash(), @@ -2844,7 +2844,7 @@ fn issue_local_invalid_statement( )); } -async fn session_info<'a, Sender>( +async fn get_session_info<'a, Sender>( session_info_provider: &'a mut Option, sender: &mut Sender, relay_parent: Hash, @@ -2853,25 +2853,41 @@ async fn session_info<'a, Sender>( where Sender: SubsystemSender, { - match session_info_provider { - Some(session_info_provider) => match session_info_provider - .runtime_info - .get_session_info_by_index(sender, relay_parent, session_index) - .await - { - Ok(extended_info) => Some(&extended_info.session_info), - Err(_) => { - gum::error!( - target: LOG_TARGET, - session = session_index, - ?relay_parent, - "Can't get SessionInfo" - ); - None - }, - }, + // If `SessionInfoProvider` is not initialized - try to initialize it first + if session_info_provider.is_none() { + cache_session_info_for_head(sender, relay_parent, session_info_provider).await; + } + + if session_info_provider.is_none() { + // `cache_session_info_for_head` should initialize `session_info_provider` + // no matter what so log an error if it is still `None` + gum::error!( + target: LOG_TARGET, + session = session_index, + ?relay_parent, + "SessionInfoProvider is not initialized after caching sessions" + ); + return None + } - None => None, + // And then process the request + match session_info_provider + .as_mut() + .expect("Checked that session_info_provider is Some on the line above. qed.") + .runtime_info + .get_session_info_by_index(sender, relay_parent, session_index) + .await + { + Ok(extended_info) => Some(&extended_info.session_info), + Err(_) => { + gum::error!( + target: LOG_TARGET, + session = session_index, + ?relay_parent, + "Can't get SessionInfo" + ); + None + }, } } From dca728782f378d9cd036b7ac40565d57c6d07f28 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 24 Apr 2023 11:40:47 +0300 Subject: [PATCH 12/42] Fix SessionInfo retrieval --- node/core/approval-voting/src/import.rs | 62 +++++++++++++------------ 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index 540375a6c028..10e4c9f1579e 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -58,6 +58,7 @@ use crate::{ backend::{Backend, OverlayedBackend}, cache_session_info_for_head, criteria::{AssignmentCriteria, OurAssignment}, + get_session_info, persisted_entries::CandidateEntry, time::{slot_number_to_tick, Tick}, SessionInfoProvider, @@ -207,26 +208,19 @@ async fn imported_block_info( } }; - // todo: refactor this - let session_info = if let Some(session_info_provider) = env.runtime_info { - session_info_provider - .runtime_info - .get_session_info_by_index(ctx.sender(), block_hash, session_index) - .await - .map_err(|_| ImportedBlockInfoError::SessionInfoUnavailable) - } else { - gum::debug!(target: LOG_TARGET, "SessionInfoProvider unavailable for block {}", block_hash,); - return Err(ImportedBlockInfoError::SessionInfoUnavailable) - }; - - let session_info = match session_info { - Ok(extended_session_info) => &extended_session_info.session_info, - Err(_) => { - gum::debug!(target: LOG_TARGET, "Session info unavailable for block {}", block_hash,); - - return Err(ImportedBlockInfoError::SessionInfoUnavailable) - }, - }; + let session_info = + match get_session_info(env.runtime_info, ctx.sender(), block_hash, session_index).await { + Some(session_info) => session_info, + None => { + gum::error!( + target: LOG_TARGET, + relay_parent = ?block_hash, + session = session_index, + "Session info unavailable" + ); + return Err(ImportedBlockInfoError::SessionInfoUnavailable) + }, + }; let (assignments, slot, relay_vrf_story) = { let unsafe_vrf = approval_types::babe_unsafe_vrf_info(&block_header); @@ -450,15 +444,25 @@ pub(crate) async fn handle_new_head( force_approve, } = imported_block_info; - // todo: refactor this - let session_info = &session_info_provider - .as_mut() - .map(|s| &mut s.runtime_info) - .expect("imported_block_info requires session info to be available; qed") - .get_session_info_by_index(ctx.sender(), head, session_index) - .await - .map_err(|e| SubsystemError::FromOrigin { origin: "", source: e.into() })? - .session_info; + let session_info = match get_session_info( + session_info_provider, + ctx.sender(), + head, + session_index, + ) + .await + { + Some(session_info) => session_info, + None => { + gum::error!( + target: LOG_TARGET, + session = session_index, + ?head, + "Can't get session info for the new head" + ); + return Ok(Vec::new()) + }, + }; let (block_tick, no_show_duration) = { let block_tick = slot_number_to_tick(state.slot_duration_millis, slot); From 360b96d67f425fb0d0ae937268fdfa370d4ddb2a Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 24 Apr 2023 14:07:31 +0300 Subject: [PATCH 13/42] Code cleanup --- node/core/approval-voting/src/lib.rs | 32 +++++++++++----------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index dd69931550be..c0e8b75fddab 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -677,8 +677,7 @@ impl State { where Sender: SubsystemSender, { - // We can't borrow the session here. Only get copies of what's needed. - let (no_show_slots, needed_approvals) = match get_session_info( + let session_info = match get_session_info( session_info_provider, sender, block_entry.parent_hash(), @@ -686,7 +685,7 @@ impl State { ) .await { - Some(s) => (s.no_show_slots, s.needed_approvals), + Some(s) => s, None => { gum::warn!( target: LOG_TARGET, @@ -700,8 +699,10 @@ impl State { let tranche_now = self.clock.tranche_now(self.slot_duration_millis, block_entry.slot()); let block_tick = slot_number_to_tick(self.slot_duration_millis, block_entry.slot()); - let no_show_duration = - slot_number_to_tick(self.slot_duration_millis, Slot::from(u64::from(no_show_slots))); + let no_show_duration = slot_number_to_tick( + self.slot_duration_millis, + Slot::from(u64::from(session_info.no_show_slots)), + ); if let Some(approval_entry) = candidate_entry.approval_entry(&block_hash) { let required_tranches = approval_checking::tranches_to_approve( @@ -710,7 +711,7 @@ impl State { tranche_now, block_tick, no_show_duration, - needed_approvals as _, + session_info.needed_approvals as _, ); let status = ApprovalStatus { required_tranches, block_tick, tranche_now }; @@ -771,7 +772,6 @@ where // `None` on start-up. Gets initialized/updated on leaf update let mut session_info_provider: Option = None; - let mut wakeups = Wakeups::default(); let mut currently_checking_set = CurrentlyCheckingSet::default(); let mut approvals_cache = lru::LruCache::new(APPROVAL_CACHE_SIZE); @@ -1849,11 +1849,10 @@ where )), }; - let config = &criteria::Config::from(session_info); let res = state.assignment_criteria.check_assignment_cert( claimed_core_index, assignment.validator, - config, + &criteria::Config::from(session_info), block_entry.relay_vrf_story(), &assignment.cert, approval_entry.backing_group(), @@ -2066,9 +2065,6 @@ where Ok((actions, t)) } -// TODO? -// fn get_session_info(..) {} - #[derive(Debug)] enum ApprovalStateTransition { RemoteApproval(ValidatorIndex), @@ -2301,8 +2297,6 @@ async fn process_wakeup( _ => return Ok(Vec::new()), }; - let slot_duration_millis = state.slot_duration_millis; - let tranche_now = state.clock.tranche_now(slot_duration_millis, block_entry.slot()); let session_info = match get_session_info( session_info_provider, ctx.sender(), @@ -2324,12 +2318,12 @@ async fn process_wakeup( }, }; - let block_tick = slot_number_to_tick(slot_duration_millis, block_entry.slot()); + let block_tick = slot_number_to_tick(state.slot_duration_millis, block_entry.slot()); let no_show_duration = slot_number_to_tick( - slot_duration_millis, + state.slot_duration_millis, Slot::from(u64::from(session_info.no_show_slots)), ); - + let tranche_now = state.clock.tranche_now(state.slot_duration_millis, block_entry.slot()); span.add_uint_tag("tranche", tranche_now as u64); gum::trace!( target: LOG_TARGET, @@ -2738,7 +2732,7 @@ async fn issue_approval( }; let validator_pubkey = match session_info.validators.get(validator_index) { - Some(p) => p.clone(), // todo: remove this + Some(p) => p, None => { gum::warn!( target: LOG_TARGET, @@ -2884,7 +2878,7 @@ where target: LOG_TARGET, session = session_index, ?relay_parent, - "Can't get SessionInfo" + "Can't obtain SessionInfo" ); None }, From caa25293ef736b5d1cf5e4d5cdf3f8e16cad7dba Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 25 Apr 2023 16:46:02 +0300 Subject: [PATCH 14/42] Don't wrap `SessionInfoProvider` in an `Option` --- node/core/approval-voting/src/import.rs | 75 +++-- node/core/approval-voting/src/lib.rs | 355 ++++++++++-------------- 2 files changed, 188 insertions(+), 242 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index 10e4c9f1579e..ec03a312ba39 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -56,9 +56,7 @@ use std::collections::HashMap; use super::approval_db::v1; use crate::{ backend::{Backend, OverlayedBackend}, - cache_session_info_for_head, criteria::{AssignmentCriteria, OurAssignment}, - get_session_info, persisted_entries::CandidateEntry, time::{slot_number_to_tick, Tick}, SessionInfoProvider, @@ -78,7 +76,7 @@ struct ImportedBlockInfo { } struct ImportedBlockInfoEnv<'a> { - runtime_info: &'a mut Option, // this is required just for the `earliest_session()` + runtime_info: &'a mut SessionInfoProvider, // this is required just for the `earliest_session()` assignment_criteria: &'a (dyn AssignmentCriteria + Send + Sync), keystore: &'a LocalKeystore, } @@ -160,7 +158,7 @@ async fn imported_block_info( return Err(ImportedBlockInfoError::FutureCancelled("SessionIndexForChild", error)), }; - if env.runtime_info.as_ref().map_or(true, |s| session_index < s.earliest_session()) { + if env.runtime_info.earliest_session().map_or(true, |s| session_index < s) { gum::debug!( target: LOG_TARGET, "Block {} is from ancient session {}. Skipping", @@ -209,7 +207,7 @@ async fn imported_block_info( }; let session_info = - match get_session_info(env.runtime_info, ctx.sender(), block_hash, session_index).await { + match env.runtime_info.get_session_info(ctx.sender(), block_hash, session_index).await { Some(session_info) => session_info, None => { gum::error!( @@ -325,7 +323,7 @@ pub(crate) async fn handle_new_head( ctx: &mut Context, state: &State, db: &mut OverlayedBackend<'_, B>, - session_info_provider: &mut Option, + session_info_provider: &mut SessionInfoProvider, head: Hash, finalized_number: &Option, ) -> SubsystemResult> { @@ -361,7 +359,7 @@ pub(crate) async fn handle_new_head( }; // Update session info based on most recent head. - cache_session_info_for_head(ctx.sender(), head, session_info_provider).await; + session_info_provider.cache_session_info_for_head(ctx.sender(), head).await; // If we've just started the node and are far behind, // import at most `MAX_HEADS_LOOK_BACK` blocks. @@ -444,25 +442,19 @@ pub(crate) async fn handle_new_head( force_approve, } = imported_block_info; - let session_info = match get_session_info( - session_info_provider, - ctx.sender(), - head, - session_index, - ) - .await - { - Some(session_info) => session_info, - None => { - gum::error!( - target: LOG_TARGET, - session = session_index, - ?head, - "Can't get session info for the new head" - ); - return Ok(Vec::new()) - }, - }; + let session_info = + match session_info_provider.get_session_info(ctx.sender(), head, session_index).await { + Some(session_info) => session_info, + None => { + gum::error!( + target: LOG_TARGET, + session = session_index, + ?head, + "Can't get session info for the new head" + ); + return Ok(Vec::new()) + }, + }; let (block_tick, no_show_duration) = { let block_tick = slot_number_to_tick(state.slot_duration_millis, slot); @@ -684,8 +676,11 @@ pub(crate) mod tests { let state = blank_state(); - let session_info_provider = - SessionInfoProvider { runtime_info, gaps_in_cache: false, highest_session_seen: index }; + let session_info_provider = SessionInfoProvider { + runtime_info, + gaps_in_cache: false, + highest_session_seen: Some(index), + }; (state, session_info_provider) } @@ -795,7 +790,7 @@ pub(crate) mod tests { .map(|(r, c, g)| (r.hash(), r.clone(), *c, *g)) .collect::>(); - let session_info_provider = SessionInfoProvider { + let mut session_info_provider = SessionInfoProvider { runtime_info: RuntimeInfo::new_with_cache( RuntimeInfoConfig { keystore: None, @@ -812,13 +807,13 @@ pub(crate) mod tests { )], ), gaps_in_cache: false, - highest_session_seen: session, + highest_session_seen: Some(session), }; let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - runtime_info: &mut Some(session_info_provider), + runtime_info: &mut session_info_provider, assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; @@ -918,7 +913,7 @@ pub(crate) mod tests { .collect::>(); let test_fut = { - let session_info_provider = SessionInfoProvider { + let mut session_info_provider = SessionInfoProvider { runtime_info: RuntimeInfo::new_with_cache( RuntimeInfoConfig { keystore: None, @@ -935,13 +930,13 @@ pub(crate) mod tests { )], ), gaps_in_cache: false, - highest_session_seen: session, + highest_session_seen: Some(session), }; let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - runtime_info: &mut Some(session_info_provider), + runtime_info: &mut session_info_provider, assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; @@ -1035,7 +1030,7 @@ pub(crate) mod tests { .collect::>(); let test_fut = { - let mut runtime_info = None; + let mut runtime_info = SessionInfoProvider::new(); let header = header.clone(); Box::pin(async move { @@ -1130,7 +1125,7 @@ pub(crate) mod tests { .map(|(r, c, g)| (r.hash(), r.clone(), *c, *g)) .collect::>(); - let session_info_provider = SessionInfoProvider { + let mut session_info_provider = SessionInfoProvider { runtime_info: RuntimeInfo::new_with_cache( RuntimeInfoConfig { keystore: None, @@ -1147,13 +1142,13 @@ pub(crate) mod tests { )], ), gaps_in_cache: false, - highest_session_seen: session, + highest_session_seen: Some(session), }; let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - runtime_info: &mut Some(session_info_provider), + runtime_info: &mut session_info_provider, assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; @@ -1290,7 +1285,7 @@ pub(crate) mod tests { .map(|(r, c, g)| CandidateEvent::CandidateIncluded(r, Vec::new().into(), c, g)) .collect::>(); - let (state, session_info_provider) = + let (state, mut session_info_provider) = single_session_state(session, session_info, parent_hash); overlay_db.write_block_entry( v1::BlockEntry { @@ -1317,7 +1312,7 @@ pub(crate) mod tests { &mut ctx, &state, &mut overlay_db, - &mut Some(session_info_provider), + &mut session_info_provider, hash, &Some(1), ) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index c0e8b75fddab..7d36ee16545f 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -645,12 +645,138 @@ struct SessionInfoProvider { // Will be true if an error had occurred during the last session caching attempt gaps_in_cache: bool, // Highest session index seen so far. Also used to calculate the earliest one. - highest_session_seen: SessionIndex, + highest_session_seen: Option, } impl SessionInfoProvider { - fn earliest_session(&self) -> SessionIndex { - self.highest_session_seen.saturating_sub(DISPUTE_WINDOW.get() - 1) + fn new() -> Self { + SessionInfoProvider { + runtime_info: RuntimeInfo::new_with_config(RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }), + gaps_in_cache: false, + highest_session_seen: None, + } + } + + fn earliest_session(&self) -> Option { + self.highest_session_seen.map(|s| s.saturating_sub(DISPUTE_WINDOW.get() - 1)) + } + + async fn cache_session_info_for_head(&mut self, sender: &mut Sender, head: Hash) + where + Sender: SubsystemSender, + { + match self.highest_session_seen { + None => { + let head_session_idx = + match self.runtime_info.get_session_index_for_child(sender, head).await { + Ok(session_idx) => session_idx, + Err(err) => { + gum::debug!( + target: LOG_TARGET, + ?head, + ?err, + "Error getting session index for head. Won't cache any sessions" + ); + return + }, + }; + + for idx in + head_session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1)..=head_session_idx + { + if let Err(err) = + self.runtime_info.get_session_info_by_index(sender, head, idx).await + { + self.gaps_in_cache = true; + gum::debug!( + target: LOG_TARGET, + ?err, + session = idx, + "Can't cache session. Moving on." + ); + continue + } + } + + self.highest_session_seen = Some(head_session_idx); + }, + Some(highest_session_seen) => { + let head_session_idx = + match self.runtime_info.get_session_index_for_child(sender, head).await { + Ok(session_idx) => session_idx, + Err(err) => { + gum::debug!( + target: LOG_TARGET, + ?head, + ?err, + "Error getting session index for head. Won't cache any sessions" + ); + return + }, + }; + + if self.gaps_in_cache || head_session_idx > highest_session_seen { + let lower_bound = if self.gaps_in_cache { + head_session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1) + } else { + highest_session_seen + 1 + }; + + for idx in lower_bound..=head_session_idx { + if let Err(err) = + self.runtime_info.get_session_info_by_index(sender, head, idx).await + { + self.gaps_in_cache = true; + gum::debug!( + target: LOG_TARGET, + ?err, + session = idx, + "Can cache session. Moving on." + ); + continue + } + } + + self.highest_session_seen = Some(head_session_idx); + } + }, + } + } + + async fn get_session_info<'a, Sender>( + &'a mut self, + sender: &mut Sender, + relay_parent: Hash, + session_index: SessionIndex, + ) -> Option<&'a SessionInfo> + where + Sender: SubsystemSender, + { + // If this session is new - perform caching + if self.highest_session_seen.map_or(true, |s| session_index > s) { + self.cache_session_info_for_head(sender, relay_parent).await; + } + + match self + .runtime_info + .get_session_info_by_index(sender, relay_parent, session_index) + .await + { + Ok(extended_info) => Some(&extended_info.session_info), + Err(_) => { + gum::error!( + target: LOG_TARGET, + session = session_index, + ?relay_parent, + "Can't obtain SessionInfo" + ); + None + }, + } } } @@ -670,20 +796,16 @@ impl State { async fn approval_status( &'a self, sender: &mut Sender, - session_info_provider: &'a mut Option, + session_info_provider: &'a mut SessionInfoProvider, block_entry: &'a BlockEntry, candidate_entry: &'b CandidateEntry, ) -> Option<(&'b ApprovalEntry, ApprovalStatus)> where Sender: SubsystemSender, { - let session_info = match get_session_info( - session_info_provider, - sender, - block_entry.parent_hash(), - block_entry.session(), - ) - .await + let session_info = match session_info_provider + .get_session_info(sender, block_entry.parent_hash(), block_entry.session()) + .await { Some(s) => s, None => { @@ -771,7 +893,7 @@ where }; // `None` on start-up. Gets initialized/updated on leaf update - let mut session_info_provider: Option = None; + let mut session_info_provider = SessionInfoProvider::new(); let mut wakeups = Wakeups::default(); let mut currently_checking_set = CurrentlyCheckingSet::default(); let mut approvals_cache = lru::LruCache::new(APPROVAL_CACHE_SIZE); @@ -906,7 +1028,7 @@ async fn handle_actions( ctx: &mut Context, state: &State, overlayed_db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut Option, + session_info_provider: &mut SessionInfoProvider, metrics: &Metrics, wakeups: &mut Wakeups, currently_checking_set: &mut CurrentlyCheckingSet, @@ -1165,7 +1287,7 @@ async fn handle_from_overseer( ctx: &mut Context, state: &mut State, db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut Option, + session_info_provider: &mut SessionInfoProvider, metrics: &Metrics, x: FromOrchestra, last_finalized_height: &mut Option, @@ -1757,7 +1879,7 @@ async fn check_and_import_assignment( sender: &mut Sender, state: &State, db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut Option, + session_info_provider: &mut SessionInfoProvider, assignment: IndirectAssignmentCert, candidate_index: CandidateIndex, ) -> SubsystemResult<(AssignmentCheckResult, Vec)> @@ -1786,13 +1908,9 @@ where )), }; - let session_info = match get_session_info( - session_info_provider, - sender, - block_entry.parent_hash(), - block_entry.session(), - ) - .await + let session_info = match session_info_provider + .get_session_info(sender, block_entry.parent_hash(), block_entry.session()) + .await { Some(s) => s, None => @@ -1929,7 +2047,7 @@ async fn check_and_import_approval( sender: &mut Sender, state: &State, db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut Option, + session_info_provider: &mut SessionInfoProvider, metrics: &Metrics, approval: IndirectSignedApprovalVote, with_response: impl FnOnce(ApprovalCheckResult) -> T, @@ -1962,13 +2080,9 @@ where }, }; - let session_info = match get_session_info( - session_info_provider, - sender, - approval.block_hash, - block_entry.session(), - ) - .await + let session_info = match session_info_provider + .get_session_info(sender, approval.block_hash, block_entry.session()) + .await { Some(s) => s, None => { @@ -2097,7 +2211,7 @@ async fn advance_approval_state( sender: &mut Sender, state: &State, db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut Option, + session_info_provider: &mut SessionInfoProvider, metrics: &Metrics, mut block_entry: BlockEntry, candidate_hash: CandidateHash, @@ -2273,7 +2387,7 @@ async fn process_wakeup( ctx: &mut Context, state: &State, db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut Option, + session_info_provider: &mut SessionInfoProvider, relay_block: Hash, candidate_hash: CandidateHash, metrics: &Metrics, @@ -2297,13 +2411,9 @@ async fn process_wakeup( _ => return Ok(Vec::new()), }; - let session_info = match get_session_info( - session_info_provider, - ctx.sender(), - block_entry.parent_hash(), - block_entry.session(), - ) - .await + let session_info = match session_info_provider + .get_session_info(ctx.sender(), block_entry.parent_hash(), block_entry.session()) + .await { Some(i) => i, None => { @@ -2637,7 +2747,7 @@ async fn issue_approval( ctx: &mut Context, state: &State, db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut Option, + session_info_provider: &mut SessionInfoProvider, metrics: &Metrics, candidate_hash: CandidateHash, ApprovalVoteRequest { validator_index, block_hash }: ApprovalVoteRequest, @@ -2679,13 +2789,9 @@ async fn issue_approval( }; issue_approval_span.add_int_tag("candidate_index", candidate_index as i64); - let session_info = match get_session_info( - session_info_provider, - ctx.sender(), - block_entry.parent_hash(), - block_entry.session(), - ) - .await + let session_info = match session_info_provider + .get_session_info(ctx.sender(), block_entry.parent_hash(), block_entry.session()) + .await { Some(s) => s, None => { @@ -2837,158 +2943,3 @@ fn issue_local_invalid_statement( false, )); } - -async fn get_session_info<'a, Sender>( - session_info_provider: &'a mut Option, - sender: &mut Sender, - relay_parent: Hash, - session_index: SessionIndex, -) -> Option<&'a SessionInfo> -where - Sender: SubsystemSender, -{ - // If `SessionInfoProvider` is not initialized - try to initialize it first - if session_info_provider.is_none() { - cache_session_info_for_head(sender, relay_parent, session_info_provider).await; - } - - if session_info_provider.is_none() { - // `cache_session_info_for_head` should initialize `session_info_provider` - // no matter what so log an error if it is still `None` - gum::error!( - target: LOG_TARGET, - session = session_index, - ?relay_parent, - "SessionInfoProvider is not initialized after caching sessions" - ); - return None - } - - // And then process the request - match session_info_provider - .as_mut() - .expect("Checked that session_info_provider is Some on the line above. qed.") - .runtime_info - .get_session_info_by_index(sender, relay_parent, session_index) - .await - { - Ok(extended_info) => Some(&extended_info.session_info), - Err(_) => { - gum::error!( - target: LOG_TARGET, - session = session_index, - ?relay_parent, - "Can't obtain SessionInfo" - ); - None - }, - } -} - -/// If `head` is in a new session - cache it -async fn cache_session_info_for_head( - sender: &mut Sender, - head: Hash, - session_info: &mut Option, -) where - Sender: SubsystemSender, -{ - match session_info.take() { - None => { - let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { - keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), - }); - - let head_session_idx = - match runtime_info.get_session_index_for_child(sender, head).await { - Ok(session_idx) => session_idx, - Err(err) => { - gum::debug!( - target: LOG_TARGET, - ?head, - ?err, - "Error getting session index for head. Won't cache any sessions" - ); - return - }, - }; - - let mut gaps_in_cache = false; - for idx in head_session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1)..=head_session_idx - { - if let Err(err) = runtime_info.get_session_info_by_index(sender, head, idx).await { - gaps_in_cache = true; - gum::debug!( - target: LOG_TARGET, - ?err, - session = idx, - "Can't cache session. Moving on." - ); - continue - } - } - - *session_info = Some(SessionInfoProvider { - runtime_info, - gaps_in_cache, - highest_session_seen: head_session_idx, - }); - }, - Some(mut session_info_provider) => { - let head_session_idx = match session_info_provider - .runtime_info - .get_session_index_for_child(sender, head) - .await - { - Ok(session_idx) => session_idx, - Err(err) => { - gum::debug!( - target: LOG_TARGET, - ?head, - ?err, - "Error getting session index for head. Won't cache any sessions" - ); - return - }, - }; - - let mut gaps_in_cache = false; - if session_info_provider.gaps_in_cache || - head_session_idx > session_info_provider.highest_session_seen - { - let lower_bound = if session_info_provider.gaps_in_cache { - head_session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1) - } else { - session_info_provider.highest_session_seen + 1 - }; - - for idx in lower_bound..=head_session_idx { - if let Err(err) = session_info_provider - .runtime_info - .get_session_info_by_index(sender, head, idx) - .await - { - gaps_in_cache = true; - gum::debug!( - target: LOG_TARGET, - ?err, - session = idx, - "Can cache session. Moving on." - ); - continue - } - } - - session_info_provider = SessionInfoProvider { - runtime_info: session_info_provider.runtime_info, - gaps_in_cache, - highest_session_seen: head_session_idx, - }; - } - - *session_info = Some(session_info_provider); - }, - } -} From 7665ecdf0638258a89fc4ec2ebcba68f30a3f8ee Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov <610755+tdimitrov@users.noreply.github.com> Date: Thu, 27 Apr 2023 16:08:46 +0300 Subject: [PATCH 15/42] Remove `earliest_session()` --- node/core/approval-voting/src/import.rs | 31 ++++++++++++++----------- node/core/approval-voting/src/lib.rs | 6 +---- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index ec03a312ba39..be52b99c1210 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -76,7 +76,7 @@ struct ImportedBlockInfo { } struct ImportedBlockInfoEnv<'a> { - runtime_info: &'a mut SessionInfoProvider, // this is required just for the `earliest_session()` + runtime_info: &'a mut SessionInfoProvider, assignment_criteria: &'a (dyn AssignmentCriteria + Send + Sync), keystore: &'a LocalKeystore, } @@ -94,8 +94,8 @@ enum ImportedBlockInfoError { #[error(transparent)] ApprovalError(approval_types::ApprovalError), - #[error("block is from an ancient session")] - BlockFromAncientSession, + #[error("block is already finalized")] + BlockAlreadyFinalized, #[error("session info unavailable")] SessionInfoUnavailable, @@ -111,6 +111,7 @@ async fn imported_block_info( env: ImportedBlockInfoEnv<'_>, block_hash: Hash, block_header: &Header, + last_finalized_height: &Option, ) -> Result { // Ignore any runtime API errors - that means these blocks are old and finalized. // Only unfinalized blocks factor into the approval voting process. @@ -158,15 +159,17 @@ async fn imported_block_info( return Err(ImportedBlockInfoError::FutureCancelled("SessionIndexForChild", error)), }; - if env.runtime_info.earliest_session().map_or(true, |s| session_index < s) { + // If we can't determine if the block is finalized or not - try processing it + if last_finalized_height.map_or(false, |finalized| block_header.number < finalized) { gum::debug!( target: LOG_TARGET, - "Block {} is from ancient session {}. Skipping", + session = session_index, + finalized = ?last_finalized_height, + "Block {} is either finalized or last finalized height is unknown. Skipping", block_hash, - session_index ); - return Err(ImportedBlockInfoError::BlockFromAncientSession) + return Err(ImportedBlockInfoError::BlockAlreadyFinalized) } session_index @@ -393,7 +396,7 @@ pub(crate) async fn handle_new_head( keystore: &state.keystore, }; - match imported_block_info(ctx, env, block_hash, &block_header).await { + match imported_block_info(ctx, env, block_hash, &block_header, finalized_number).await { Ok(i) => imported_blocks_and_info.push((block_hash, block_header, i)), Err(error) => { // It's possible that we've lost a race with finality. @@ -818,7 +821,8 @@ pub(crate) mod tests { keystore: &LocalKeystore::in_memory(), }; - let info = imported_block_info(&mut ctx, env, hash, &header).await.unwrap(); + let info = + imported_block_info(&mut ctx, env, hash, &header, &Some(4)).await.unwrap(); assert_eq!(info.included_candidates, included_candidates); assert_eq!(info.session_index, session); @@ -941,7 +945,7 @@ pub(crate) mod tests { keystore: &LocalKeystore::in_memory(), }; - let info = imported_block_info(&mut ctx, env, hash, &header).await; + let info = imported_block_info(&mut ctx, env, hash, &header, &Some(4)).await; assert_matches!(info, Err(ImportedBlockInfoError::VrfInfoUnavailable)); }) @@ -1040,9 +1044,9 @@ pub(crate) mod tests { keystore: &LocalKeystore::in_memory(), }; - let info = imported_block_info(&mut ctx, env, hash, &header).await; + let info = imported_block_info(&mut ctx, env, hash, &header, &Some(6)).await; - assert_matches!(info, Err(ImportedBlockInfoError::BlockFromAncientSession)); + assert_matches!(info, Err(ImportedBlockInfoError::BlockAlreadyFinalized)); }) }; @@ -1153,7 +1157,8 @@ pub(crate) mod tests { keystore: &LocalKeystore::in_memory(), }; - let info = imported_block_info(&mut ctx, env, hash, &header).await.unwrap(); + let info = + imported_block_info(&mut ctx, env, hash, &header, &Some(4)).await.unwrap(); assert_eq!(info.included_candidates, included_candidates); assert_eq!(info.session_index, session); diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 7d36ee16545f..db6324d0cd4f 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -661,10 +661,6 @@ impl SessionInfoProvider { } } - fn earliest_session(&self) -> Option { - self.highest_session_seen.map(|s| s.saturating_sub(DISPUTE_WINDOW.get() - 1)) - } - async fn cache_session_info_for_head(&mut self, sender: &mut Sender, head: Hash) where Sender: SubsystemSender, @@ -1307,7 +1303,7 @@ async fn handle_from_overseer( db, session_info_provider, head, - &*last_finalized_height, + last_finalized_height, ) .await { From 3f2b85c5f4709f7c01808b5390caa874870f653f Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov <610755+tdimitrov@users.noreply.github.com> Date: Fri, 28 Apr 2023 11:19:52 +0300 Subject: [PATCH 16/42] Remove pre-caching -> wip --- node/core/approval-voting/src/import.rs | 161 ++++++++--------- node/core/approval-voting/src/lib.rs | 229 ++++++++---------------- 2 files changed, 144 insertions(+), 246 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index be52b99c1210..d0f6df421376 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -40,7 +40,7 @@ use polkadot_node_subsystem::{ }, overseer, RuntimeApiError, SubsystemError, SubsystemResult, }; -use polkadot_node_subsystem_util::determine_new_blocks; +use polkadot_node_subsystem_util::{determine_new_blocks, runtime::RuntimeInfo}; use polkadot_primitives::{ BlockNumber, CandidateEvent, CandidateHash, CandidateReceipt, ConsensusLog, CoreIndex, GroupIndex, Hash, Header, SessionIndex, @@ -57,9 +57,9 @@ use super::approval_db::v1; use crate::{ backend::{Backend, OverlayedBackend}, criteria::{AssignmentCriteria, OurAssignment}, + get_session_info, persisted_entries::CandidateEntry, time::{slot_number_to_tick, Tick}, - SessionInfoProvider, }; use super::{State, LOG_TARGET}; @@ -76,7 +76,7 @@ struct ImportedBlockInfo { } struct ImportedBlockInfoEnv<'a> { - runtime_info: &'a mut SessionInfoProvider, + runtime_info: &'a mut RuntimeInfo, assignment_criteria: &'a (dyn AssignmentCriteria + Send + Sync), keystore: &'a LocalKeystore, } @@ -210,7 +210,7 @@ async fn imported_block_info( }; let session_info = - match env.runtime_info.get_session_info(ctx.sender(), block_hash, session_index).await { + match get_session_info(env.runtime_info, ctx.sender(), block_hash, session_index).await { Some(session_info) => session_info, None => { gum::error!( @@ -326,7 +326,7 @@ pub(crate) async fn handle_new_head( ctx: &mut Context, state: &State, db: &mut OverlayedBackend<'_, B>, - session_info_provider: &mut SessionInfoProvider, + session_info_provider: &mut RuntimeInfo, head: Hash, finalized_number: &Option, ) -> SubsystemResult> { @@ -361,9 +361,6 @@ pub(crate) async fn handle_new_head( } }; - // Update session info based on most recent head. - session_info_provider.cache_session_info_for_head(ctx.sender(), head).await; - // If we've just started the node and are far behind, // import at most `MAX_HEADS_LOOK_BACK` blocks. let lower_bound_number = header.number.saturating_sub(MAX_HEADS_LOOK_BACK); @@ -445,19 +442,25 @@ pub(crate) async fn handle_new_head( force_approve, } = imported_block_info; - let session_info = - match session_info_provider.get_session_info(ctx.sender(), head, session_index).await { - Some(session_info) => session_info, - None => { - gum::error!( - target: LOG_TARGET, - session = session_index, - ?head, - "Can't get session info for the new head" - ); - return Ok(Vec::new()) - }, - }; + let session_info = match get_session_info( + session_info_provider, + ctx.sender(), + head, + session_index, + ) + .await + { + Some(session_info) => session_info, + None => { + gum::error!( + target: LOG_TARGET, + session = session_index, + ?head, + "Can't get session info for the new head" + ); + return Ok(Vec::new()) + }, + }; let (block_tick, no_show_duration) = { let block_tick = slot_number_to_tick(state.slot_duration_millis, slot); @@ -660,7 +663,7 @@ pub(crate) mod tests { index: SessionIndex, info: SessionInfo, relay_parent: Hash, - ) -> (State, SessionInfoProvider) { + ) -> (State, RuntimeInfo) { let runtime_info = RuntimeInfo::new_with_cache( RuntimeInfoConfig { keystore: None, @@ -677,15 +680,7 @@ pub(crate) mod tests { )], ); - let state = blank_state(); - - let session_info_provider = SessionInfoProvider { - runtime_info, - gaps_in_cache: false, - highest_session_seen: Some(index), - }; - - (state, session_info_provider) + (blank_state(), runtime_info) } struct MockAssignmentCriteria; @@ -793,25 +788,21 @@ pub(crate) mod tests { .map(|(r, c, g)| (r.hash(), r.clone(), *c, *g)) .collect::>(); - let mut session_info_provider = SessionInfoProvider { - runtime_info: RuntimeInfo::new_with_cache( - RuntimeInfoConfig { - keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), + let mut session_info_provider = RuntimeInfo::new_with_cache( + RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }, + vec![( + session, + hash, + ExtendedSessionInfo { + session_info, + validator_info: ValidatorInfo { our_index: None, our_group: None }, }, - vec![( - session, - hash, - ExtendedSessionInfo { - session_info, - validator_info: ValidatorInfo { our_index: None, our_group: None }, - }, - )], - ), - gaps_in_cache: false, - highest_session_seen: Some(session), - }; + )], + ); let header = header.clone(); Box::pin(async move { @@ -917,25 +908,21 @@ pub(crate) mod tests { .collect::>(); let test_fut = { - let mut session_info_provider = SessionInfoProvider { - runtime_info: RuntimeInfo::new_with_cache( - RuntimeInfoConfig { - keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), + let mut session_info_provider = RuntimeInfo::new_with_cache( + RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }, + vec![( + session, + hash, + ExtendedSessionInfo { + session_info, + validator_info: ValidatorInfo { our_index: None, our_group: None }, }, - vec![( - session, - hash, - ExtendedSessionInfo { - session_info, - validator_info: ValidatorInfo { our_index: None, our_group: None }, - }, - )], - ), - gaps_in_cache: false, - highest_session_seen: Some(session), - }; + )], + ); let header = header.clone(); Box::pin(async move { @@ -1034,7 +1021,11 @@ pub(crate) mod tests { .collect::>(); let test_fut = { - let mut runtime_info = SessionInfoProvider::new(); + let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }); let header = header.clone(); Box::pin(async move { @@ -1129,25 +1120,21 @@ pub(crate) mod tests { .map(|(r, c, g)| (r.hash(), r.clone(), *c, *g)) .collect::>(); - let mut session_info_provider = SessionInfoProvider { - runtime_info: RuntimeInfo::new_with_cache( - RuntimeInfoConfig { - keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), + let mut session_info_provider = RuntimeInfo::new_with_cache( + RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }, + vec![( + session, + hash, + ExtendedSessionInfo { + session_info, + validator_info: ValidatorInfo { our_index: None, our_group: None }, }, - vec![( - session, - hash, - ExtendedSessionInfo { - session_info, - validator_info: ValidatorInfo { our_index: None, our_group: None }, - }, - )], - ), - gaps_in_cache: false, - highest_session_seen: Some(session), - }; + )], + ); let header = header.clone(); Box::pin(async move { @@ -1291,7 +1278,7 @@ pub(crate) mod tests { .collect::>(); let (state, mut session_info_provider) = - single_session_state(session, session_info, parent_hash); + single_session_state(session, session_info.clone(), parent_hash); overlay_db.write_block_entry( v1::BlockEntry { block_hash: parent_hash, diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index db6324d0cd4f..f61dae4293c9 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -637,142 +637,29 @@ impl CurrentlyCheckingSet { } } -// Wraps `RuntimeInfo` and some metadata. On each new leaf `SessionInfo` is -// cached. `RuntimeInfo` keeps the last `DISPUTE_WINDOW` number of sessions. -struct SessionInfoProvider { - // `RuntimeInfo` caches sessions internally. - runtime_info: RuntimeInfo, - // Will be true if an error had occurred during the last session caching attempt - gaps_in_cache: bool, - // Highest session index seen so far. Also used to calculate the earliest one. - highest_session_seen: Option, -} - -impl SessionInfoProvider { - fn new() -> Self { - SessionInfoProvider { - runtime_info: RuntimeInfo::new_with_config(RuntimeInfoConfig { - keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), - }), - gaps_in_cache: false, - highest_session_seen: None, - } - } - - async fn cache_session_info_for_head(&mut self, sender: &mut Sender, head: Hash) - where - Sender: SubsystemSender, - { - match self.highest_session_seen { - None => { - let head_session_idx = - match self.runtime_info.get_session_index_for_child(sender, head).await { - Ok(session_idx) => session_idx, - Err(err) => { - gum::debug!( - target: LOG_TARGET, - ?head, - ?err, - "Error getting session index for head. Won't cache any sessions" - ); - return - }, - }; - - for idx in - head_session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1)..=head_session_idx - { - if let Err(err) = - self.runtime_info.get_session_info_by_index(sender, head, idx).await - { - self.gaps_in_cache = true; - gum::debug!( - target: LOG_TARGET, - ?err, - session = idx, - "Can't cache session. Moving on." - ); - continue - } - } - - self.highest_session_seen = Some(head_session_idx); - }, - Some(highest_session_seen) => { - let head_session_idx = - match self.runtime_info.get_session_index_for_child(sender, head).await { - Ok(session_idx) => session_idx, - Err(err) => { - gum::debug!( - target: LOG_TARGET, - ?head, - ?err, - "Error getting session index for head. Won't cache any sessions" - ); - return - }, - }; - - if self.gaps_in_cache || head_session_idx > highest_session_seen { - let lower_bound = if self.gaps_in_cache { - head_session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1) - } else { - highest_session_seen + 1 - }; - - for idx in lower_bound..=head_session_idx { - if let Err(err) = - self.runtime_info.get_session_info_by_index(sender, head, idx).await - { - self.gaps_in_cache = true; - gum::debug!( - target: LOG_TARGET, - ?err, - session = idx, - "Can cache session. Moving on." - ); - continue - } - } - - self.highest_session_seen = Some(head_session_idx); - } - }, - } - } - - async fn get_session_info<'a, Sender>( - &'a mut self, - sender: &mut Sender, - relay_parent: Hash, - session_index: SessionIndex, - ) -> Option<&'a SessionInfo> - where - Sender: SubsystemSender, +async fn get_session_info<'a, Sender>( + runtime_info: &'a mut RuntimeInfo, + sender: &mut Sender, + relay_parent: Hash, + session_index: SessionIndex, +) -> Option<&'a SessionInfo> +where + Sender: SubsystemSender, +{ + match runtime_info + .get_session_info_by_index(sender, relay_parent, session_index) + .await { - // If this session is new - perform caching - if self.highest_session_seen.map_or(true, |s| session_index > s) { - self.cache_session_info_for_head(sender, relay_parent).await; - } - - match self - .runtime_info - .get_session_info_by_index(sender, relay_parent, session_index) - .await - { - Ok(extended_info) => Some(&extended_info.session_info), - Err(_) => { - gum::error!( - target: LOG_TARGET, - session = session_index, - ?relay_parent, - "Can't obtain SessionInfo" - ); - None - }, - } + Ok(extended_info) => Some(&extended_info.session_info), + Err(_) => { + gum::error!( + target: LOG_TARGET, + session = session_index, + ?relay_parent, + "Can't obtain SessionInfo" + ); + None + }, } } @@ -792,16 +679,20 @@ impl State { async fn approval_status( &'a self, sender: &mut Sender, - session_info_provider: &'a mut SessionInfoProvider, + session_info_provider: &'a mut RuntimeInfo, block_entry: &'a BlockEntry, candidate_entry: &'b CandidateEntry, ) -> Option<(&'b ApprovalEntry, ApprovalStatus)> where Sender: SubsystemSender, { - let session_info = match session_info_provider - .get_session_info(sender, block_entry.parent_hash(), block_entry.session()) - .await + let session_info = match get_session_info( + session_info_provider, + sender, + block_entry.parent_hash(), + block_entry.session(), + ) + .await { Some(s) => s, None => { @@ -889,7 +780,11 @@ where }; // `None` on start-up. Gets initialized/updated on leaf update - let mut session_info_provider = SessionInfoProvider::new(); + let mut session_info_provider = RuntimeInfo::new_with_config(RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }); let mut wakeups = Wakeups::default(); let mut currently_checking_set = CurrentlyCheckingSet::default(); let mut approvals_cache = lru::LruCache::new(APPROVAL_CACHE_SIZE); @@ -1024,7 +919,7 @@ async fn handle_actions( ctx: &mut Context, state: &State, overlayed_db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut SessionInfoProvider, + session_info_provider: &mut RuntimeInfo, metrics: &Metrics, wakeups: &mut Wakeups, currently_checking_set: &mut CurrentlyCheckingSet, @@ -1283,7 +1178,7 @@ async fn handle_from_overseer( ctx: &mut Context, state: &mut State, db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut SessionInfoProvider, + session_info_provider: &mut RuntimeInfo, metrics: &Metrics, x: FromOrchestra, last_finalized_height: &mut Option, @@ -1875,7 +1770,7 @@ async fn check_and_import_assignment( sender: &mut Sender, state: &State, db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut SessionInfoProvider, + session_info_provider: &mut RuntimeInfo, assignment: IndirectAssignmentCert, candidate_index: CandidateIndex, ) -> SubsystemResult<(AssignmentCheckResult, Vec)> @@ -1904,9 +1799,13 @@ where )), }; - let session_info = match session_info_provider - .get_session_info(sender, block_entry.parent_hash(), block_entry.session()) - .await + let session_info = match get_session_info( + session_info_provider, + sender, + block_entry.parent_hash(), + block_entry.session(), + ) + .await { Some(s) => s, None => @@ -2043,7 +1942,7 @@ async fn check_and_import_approval( sender: &mut Sender, state: &State, db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut SessionInfoProvider, + session_info_provider: &mut RuntimeInfo, metrics: &Metrics, approval: IndirectSignedApprovalVote, with_response: impl FnOnce(ApprovalCheckResult) -> T, @@ -2076,9 +1975,13 @@ where }, }; - let session_info = match session_info_provider - .get_session_info(sender, approval.block_hash, block_entry.session()) - .await + let session_info = match get_session_info( + session_info_provider, + sender, + approval.block_hash, + block_entry.session(), + ) + .await { Some(s) => s, None => { @@ -2207,7 +2110,7 @@ async fn advance_approval_state( sender: &mut Sender, state: &State, db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut SessionInfoProvider, + session_info_provider: &mut RuntimeInfo, metrics: &Metrics, mut block_entry: BlockEntry, candidate_hash: CandidateHash, @@ -2383,7 +2286,7 @@ async fn process_wakeup( ctx: &mut Context, state: &State, db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut SessionInfoProvider, + session_info_provider: &mut RuntimeInfo, relay_block: Hash, candidate_hash: CandidateHash, metrics: &Metrics, @@ -2407,9 +2310,13 @@ async fn process_wakeup( _ => return Ok(Vec::new()), }; - let session_info = match session_info_provider - .get_session_info(ctx.sender(), block_entry.parent_hash(), block_entry.session()) - .await + let session_info = match get_session_info( + session_info_provider, + ctx.sender(), + block_entry.parent_hash(), + block_entry.session(), + ) + .await { Some(i) => i, None => { @@ -2743,7 +2650,7 @@ async fn issue_approval( ctx: &mut Context, state: &State, db: &mut OverlayedBackend<'_, impl Backend>, - session_info_provider: &mut SessionInfoProvider, + session_info_provider: &mut RuntimeInfo, metrics: &Metrics, candidate_hash: CandidateHash, ApprovalVoteRequest { validator_index, block_hash }: ApprovalVoteRequest, @@ -2785,9 +2692,13 @@ async fn issue_approval( }; issue_approval_span.add_int_tag("candidate_index", candidate_index as i64); - let session_info = match session_info_provider - .get_session_info(ctx.sender(), block_entry.parent_hash(), block_entry.session()) - .await + let session_info = match get_session_info( + session_info_provider, + ctx.sender(), + block_entry.parent_hash(), + block_entry.session(), + ) + .await { Some(s) => s, None => { From 19e9588d1af849ab553f175b782e68983e3f8927 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 2 May 2023 14:44:04 +0300 Subject: [PATCH 17/42] Fix some tests and code cleanup --- node/core/approval-voting/src/import.rs | 161 ++++++++++++------------ node/core/approval-voting/src/tests.rs | 43 +++---- node/subsystem-util/src/runtime/mod.rs | 13 -- 3 files changed, 93 insertions(+), 124 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index d0f6df421376..6e5996df39fa 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -613,10 +613,7 @@ pub(crate) mod tests { }; use polkadot_node_subsystem::messages::{AllMessages, ApprovalVotingMessage}; use polkadot_node_subsystem_test_helpers::make_subsystem_context; - use polkadot_node_subsystem_util::{ - database::Database, - runtime::{ExtendedSessionInfo, ValidatorInfo}, - }; + use polkadot_node_subsystem_util::database::Database; use polkadot_primitives::{Id as ParaId, IndexedVec, SessionInfo, ValidatorId, ValidatorIndex}; pub(crate) use sp_consensus_babe::{ digests::{CompatibleDigestItem, PreDigest, SecondaryVRFPreDigest}, @@ -659,26 +656,12 @@ pub(crate) mod tests { } } - fn single_session_state( - index: SessionIndex, - info: SessionInfo, - relay_parent: Hash, - ) -> (State, RuntimeInfo) { - let runtime_info = RuntimeInfo::new_with_cache( - RuntimeInfoConfig { - keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), - }, - vec![( - index, - relay_parent, - ExtendedSessionInfo { - session_info: info, - validator_info: ValidatorInfo { our_group: None, our_index: None }, - }, - )], - ); + fn single_session_state() -> (State, RuntimeInfo) { + let runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }); (blank_state(), runtime_info) } @@ -788,21 +771,11 @@ pub(crate) mod tests { .map(|(r, c, g)| (r.hash(), r.clone(), *c, *g)) .collect::>(); - let mut session_info_provider = RuntimeInfo::new_with_cache( - RuntimeInfoConfig { - keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), - }, - vec![( - session, - hash, - ExtendedSessionInfo { - session_info, - validator_info: ValidatorInfo { our_index: None, our_group: None }, - }, - )], - ); + let mut session_info_provider = RuntimeInfo::new_with_config(RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }); let header = header.clone(); Box::pin(async move { @@ -867,6 +840,20 @@ pub(crate) mod tests { })); } ); + + assert_matches!( + handle.recv().await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request( + req_block_hash, + RuntimeApiRequest::SessionInfo(idx, si_tx), + ) + ) => { + assert_eq!(session, idx); + assert_eq!(req_block_hash, hash); + si_tx.send(Ok(Some(session_info.clone()))).unwrap(); + } + ); }); futures::executor::block_on(futures::future::join(test_fut, aux_fut)); @@ -908,21 +895,11 @@ pub(crate) mod tests { .collect::>(); let test_fut = { - let mut session_info_provider = RuntimeInfo::new_with_cache( - RuntimeInfoConfig { - keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), - }, - vec![( - session, - hash, - ExtendedSessionInfo { - session_info, - validator_info: ValidatorInfo { our_index: None, our_group: None }, - }, - )], - ); + let mut session_info_provider = RuntimeInfo::new_with_config(RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }); let header = header.clone(); Box::pin(async move { @@ -981,6 +958,20 @@ pub(crate) mod tests { })); } ); + + assert_matches!( + handle.recv().await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request( + req_block_hash, + RuntimeApiRequest::SessionInfo(idx, si_tx), + ) + ) => { + assert_eq!(session, idx); + assert_eq!(req_block_hash, hash); + si_tx.send(Ok(Some(session_info.clone()))).unwrap(); + } + ); }); futures::executor::block_on(futures::future::join(test_fut, aux_fut)); @@ -1120,21 +1111,11 @@ pub(crate) mod tests { .map(|(r, c, g)| (r.hash(), r.clone(), *c, *g)) .collect::>(); - let mut session_info_provider = RuntimeInfo::new_with_cache( - RuntimeInfoConfig { - keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), - }, - vec![( - session, - hash, - ExtendedSessionInfo { - session_info, - validator_info: ValidatorInfo { our_index: None, our_group: None }, - }, - )], - ); + let mut session_info_provider = RuntimeInfo::new_with_config(RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }); let header = header.clone(); Box::pin(async move { @@ -1199,6 +1180,20 @@ pub(crate) mod tests { })); } ); + + assert_matches!( + handle.recv().await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request( + req_block_hash, + RuntimeApiRequest::SessionInfo(idx, si_tx), + ) + ) => { + assert_eq!(session, idx); + assert_eq!(req_block_hash, hash); + si_tx.send(Ok(Some(session_info.clone()))).unwrap(); + } + ); }); futures::executor::block_on(futures::future::join(test_fut, aux_fut)); @@ -1277,8 +1272,7 @@ pub(crate) mod tests { .map(|(r, c, g)| CandidateEvent::CandidateIncluded(r, Vec::new().into(), c, g)) .collect::>(); - let (state, mut session_info_provider) = - single_session_state(session, session_info.clone(), parent_hash); + let (state, mut session_info_provider) = single_session_state(); overlay_db.write_block_entry( v1::BlockEntry { block_hash: parent_hash, @@ -1343,17 +1337,6 @@ pub(crate) mod tests { } ); - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(c_tx), - )) => { - assert_eq!(h, hash); - let _ = c_tx.send(Ok(session)); - } - ); - // determine_new_blocks exits early as the parent_hash is in the DB assert_matches!( @@ -1399,6 +1382,20 @@ pub(crate) mod tests { } ); + assert_matches!( + handle.recv().await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request( + req_block_hash, + RuntimeApiRequest::SessionInfo(idx, si_tx), + ) + ) => { + assert_eq!(session, idx); + assert_eq!(req_block_hash, hash); + si_tx.send(Ok(Some(session_info.clone()))).unwrap(); + } + ); + assert_matches!( handle.recv().await, AllMessages::ApprovalDistribution(ApprovalDistributionMessage::NewBlocks( diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index 059ab09bc705..8c1fcf08985f 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -799,36 +799,7 @@ async fn import_block( h_tx.send(Ok(Some(new_header.clone()))).unwrap(); } ); - - assert_matches!( - overseer_recv(overseer).await, - AllMessages::RuntimeApi( - RuntimeApiMessage::Request( - req_block_hash, - RuntimeApiRequest::SessionIndexForChild(s_tx) - ) - ) => { - let hash = &hashes[number as usize]; - assert_eq!(req_block_hash, hash.0); - s_tx.send(Ok(number.into())).unwrap(); - } - ); - if !fork { - assert_matches!( - overseer_recv(overseer).await, - AllMessages::RuntimeApi( - RuntimeApiMessage::Request( - req_block_hash, - RuntimeApiRequest::SessionInfo(idx, si_tx), - ) - ) => { - assert_eq!(number, idx); - assert_eq!(req_block_hash, *new_head); - si_tx.send(Ok(Some(session_info.clone()))).unwrap(); - } - ); - let mut _ancestry_step = 0; if gap { assert_matches!( @@ -929,6 +900,20 @@ async fn import_block( } ); } else { + assert_matches!( + overseer_recv(overseer).await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request( + req_block_hash, + RuntimeApiRequest::SessionInfo(idx, si_tx), + ) + ) => { + // assert_eq!(session, idx); + // assert_eq!(req_block_hash, hashes[(number-1) as usize].0); + si_tx.send(Ok(Some(session_info.clone()))).unwrap(); + } + ); + assert_matches!( overseer_recv(overseer).await, AllMessages::ApprovalDistribution( diff --git a/node/subsystem-util/src/runtime/mod.rs b/node/subsystem-util/src/runtime/mod.rs index 468ba94449e7..6e06b99bbe03 100644 --- a/node/subsystem-util/src/runtime/mod.rs +++ b/node/subsystem-util/src/runtime/mod.rs @@ -118,19 +118,6 @@ impl RuntimeInfo { } } - /// Create an instance with pre-populated cache. Used only for testing - pub fn new_with_cache( - cfg: Config, - data: Vec<(SessionIndex, Hash, ExtendedSessionInfo)>, - ) -> Self { - let mut r = Self::new_with_config(cfg); - for (idx, parent, session) in data { - r.session_index_cache.put(parent, idx); - r.session_info_cache.put(idx, session); - } - r - } - /// Returns the session index expected at any child of the `parent` block. /// This does not return the session index for the `parent` block. pub async fn get_session_index_for_child( From 70d136f32837bdd9b9f1ec77c57eb1d4ad120bb7 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 2 May 2023 18:33:33 +0300 Subject: [PATCH 18/42] Fix all tests --- node/core/approval-voting/src/tests.rs | 28 ++++++++++++++------------ 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index 8c1fcf08985f..cd1a267af3ed 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -900,19 +900,21 @@ async fn import_block( } ); } else { - assert_matches!( - overseer_recv(overseer).await, - AllMessages::RuntimeApi( - RuntimeApiMessage::Request( - req_block_hash, - RuntimeApiRequest::SessionInfo(idx, si_tx), - ) - ) => { - // assert_eq!(session, idx); - // assert_eq!(req_block_hash, hashes[(number-1) as usize].0); - si_tx.send(Ok(Some(session_info.clone()))).unwrap(); - } - ); + if !fork { + assert_matches!( + overseer_recv(overseer).await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request( + req_block_hash, + RuntimeApiRequest::SessionInfo(idx, si_tx), + ) + ) => { + // assert_eq!(session, idx); + // assert_eq!(req_block_hash, hashes[(number-1) as usize].0); + si_tx.send(Ok(Some(session_info.clone()))).unwrap(); + } + ); + } assert_matches!( overseer_recv(overseer).await, From 6e348c958f62da8b92647e8667dcc1793d1b19ea Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 2 May 2023 20:45:30 +0300 Subject: [PATCH 19/42] Fixes in tests --- node/core/approval-voting/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index cd1a267af3ed..9e8295710da0 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -909,8 +909,8 @@ async fn import_block( RuntimeApiRequest::SessionInfo(idx, si_tx), ) ) => { - // assert_eq!(session, idx); - // assert_eq!(req_block_hash, hashes[(number-1) as usize].0); + // Make sure all SessionInfo calls are not made for the leaf (but for its relay parent) + assert_ne!(req_block_hash, hashes[(number-1) as usize].0); si_tx.send(Ok(Some(session_info.clone()))).unwrap(); } ); From 4fa4403322eabc68c576b6863bc73de07062004a Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 2 May 2023 20:55:29 +0300 Subject: [PATCH 20/42] Fix comments, variable names and small style changes --- node/core/approval-voting/src/import.rs | 29 +++++++++++++------------ node/core/approval-voting/src/tests.rs | 1 + 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index 6e5996df39fa..38666f0ee49b 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -159,7 +159,7 @@ async fn imported_block_info( return Err(ImportedBlockInfoError::FutureCancelled("SessionIndexForChild", error)), }; - // If we can't determine if the block is finalized or not - try processing it + // We can't determine if the block is finalized or not - try processing it if last_finalized_height.map_or(false, |finalized| block_header.number < finalized) { gum::debug!( target: LOG_TARGET, @@ -657,13 +657,14 @@ pub(crate) mod tests { } fn single_session_state() -> (State, RuntimeInfo) { - let runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { - keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), - }); - - (blank_state(), runtime_info) + ( + blank_state(), + RuntimeInfo::new_with_config(RuntimeInfoConfig { + keystore: None, + session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) + .expect("DISPUTE_WINDOW can't be 0; qed."), + }), + ) } struct MockAssignmentCriteria; @@ -771,7 +772,7 @@ pub(crate) mod tests { .map(|(r, c, g)| (r.hash(), r.clone(), *c, *g)) .collect::>(); - let mut session_info_provider = RuntimeInfo::new_with_config(RuntimeInfoConfig { + let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { keystore: None, session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) .expect("DISPUTE_WINDOW can't be 0; qed."), @@ -780,7 +781,7 @@ pub(crate) mod tests { let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - runtime_info: &mut session_info_provider, + runtime_info: &mut runtime_info, assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; @@ -895,7 +896,7 @@ pub(crate) mod tests { .collect::>(); let test_fut = { - let mut session_info_provider = RuntimeInfo::new_with_config(RuntimeInfoConfig { + let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { keystore: None, session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) .expect("DISPUTE_WINDOW can't be 0; qed."), @@ -904,7 +905,7 @@ pub(crate) mod tests { let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - runtime_info: &mut session_info_provider, + runtime_info: &mut runtime_info, assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; @@ -1111,7 +1112,7 @@ pub(crate) mod tests { .map(|(r, c, g)| (r.hash(), r.clone(), *c, *g)) .collect::>(); - let mut session_info_provider = RuntimeInfo::new_with_config(RuntimeInfoConfig { + let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { keystore: None, session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) .expect("DISPUTE_WINDOW can't be 0; qed."), @@ -1120,7 +1121,7 @@ pub(crate) mod tests { let header = header.clone(); Box::pin(async move { let env = ImportedBlockInfoEnv { - runtime_info: &mut session_info_provider, + runtime_info: &mut runtime_info, assignment_criteria: &MockAssignmentCriteria, keystore: &LocalKeystore::in_memory(), }; diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index 9e8295710da0..034098392b31 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -901,6 +901,7 @@ async fn import_block( ); } else { if !fork { + // SessionInfo won't be called for forks - it's already cached assert_matches!( overseer_recv(overseer).await, AllMessages::RuntimeApi( From dc0542acf8341909c4969170c08c5aa338a0ea7c Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 2 May 2023 22:51:03 +0300 Subject: [PATCH 21/42] Fix a warning --- node/core/approval-voting/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index 034098392b31..d7e19a8c09f3 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -907,7 +907,7 @@ async fn import_block( AllMessages::RuntimeApi( RuntimeApiMessage::Request( req_block_hash, - RuntimeApiRequest::SessionInfo(idx, si_tx), + RuntimeApiRequest::SessionInfo(_, si_tx), ) ) => { // Make sure all SessionInfo calls are not made for the leaf (but for its relay parent) From 233478e1987886c58e8f9030a6fa221d43035f82 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 5 May 2023 11:18:27 +0300 Subject: [PATCH 22/42] impl From for NonZeroUsize --- node/core/approval-voting/src/import.rs | 17 ++++++----------- node/core/approval-voting/src/lib.rs | 3 +-- node/primitives/src/lib.rs | 8 +++++++- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index 38666f0ee49b..8de52e648cc7 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -622,7 +622,7 @@ pub(crate) mod tests { use sp_core::{crypto::VrfSigner, testing::TaskExecutor}; use sp_keyring::sr25519::Keyring as Sr25519Keyring; pub(crate) use sp_runtime::{Digest, DigestItem}; - use std::{num::NonZeroUsize, pin::Pin, sync::Arc}; + use std::{pin::Pin, sync::Arc}; use crate::{approval_db::v1::Config as DatabaseConfig, criteria, BlockEntry}; @@ -661,8 +661,7 @@ pub(crate) mod tests { blank_state(), RuntimeInfo::new_with_config(RuntimeInfoConfig { keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), + session_cache_lru_size: DISPUTE_WINDOW.into(), }), ) } @@ -774,8 +773,7 @@ pub(crate) mod tests { let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), + session_cache_lru_size: DISPUTE_WINDOW.into(), }); let header = header.clone(); @@ -898,8 +896,7 @@ pub(crate) mod tests { let test_fut = { let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), + session_cache_lru_size: DISPUTE_WINDOW.into(), }); let header = header.clone(); @@ -1015,8 +1012,7 @@ pub(crate) mod tests { let test_fut = { let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), + session_cache_lru_size: DISPUTE_WINDOW.into(), }); let header = header.clone(); @@ -1114,8 +1110,7 @@ pub(crate) mod tests { let mut runtime_info = RuntimeInfo::new_with_config(RuntimeInfoConfig { keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), + session_cache_lru_size: DISPUTE_WINDOW.into(), }); let header = header.clone(); diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index f61dae4293c9..043ecd923c27 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -782,8 +782,7 @@ where // `None` on start-up. Gets initialized/updated on leaf update let mut session_info_provider = RuntimeInfo::new_with_config(RuntimeInfoConfig { keystore: None, - session_cache_lru_size: NonZeroUsize::new(DISPUTE_WINDOW.get() as usize) - .expect("DISPUTE_WINDOW can't be 0; qed."), + session_cache_lru_size: DISPUTE_WINDOW.into(), }); let mut wakeups = Wakeups::default(); let mut currently_checking_set = CurrentlyCheckingSet::default(); diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs index 18e043be9c4d..1177dbc17caa 100644 --- a/node/primitives/src/lib.rs +++ b/node/primitives/src/lib.rs @@ -22,7 +22,7 @@ #![deny(missing_docs)] -use std::pin::Pin; +use std::{num::NonZeroUsize, pin::Pin}; use bounded_vec::BoundedVec; use futures::Future; @@ -143,6 +143,12 @@ impl SessionWindowSize { } } +impl From for NonZeroUsize { + fn from(value: SessionWindowSize) -> Self { + NonZeroUsize::new(value.get() as usize).expect("SessionWindowSize can't be 0. qed.") + } +} + /// The cumulative weight of a block in a fork-choice rule. pub type BlockWeight = u32; From d27220ad881ac7a58806504fde9954c50a010a32 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Sat, 6 May 2023 09:52:40 +0300 Subject: [PATCH 23/42] Fix logging for `get_session_info` - remove redundant logs and decrease log level to DEBUG --- node/core/approval-voting/src/import.rs | 20 ++--------------- node/core/approval-voting/src/lib.rs | 29 +++---------------------- 2 files changed, 5 insertions(+), 44 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index 8de52e648cc7..d65a7ddb100f 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -212,15 +212,7 @@ async fn imported_block_info( let session_info = match get_session_info(env.runtime_info, ctx.sender(), block_hash, session_index).await { Some(session_info) => session_info, - None => { - gum::error!( - target: LOG_TARGET, - relay_parent = ?block_hash, - session = session_index, - "Session info unavailable" - ); - return Err(ImportedBlockInfoError::SessionInfoUnavailable) - }, + None => return Err(ImportedBlockInfoError::SessionInfoUnavailable), }; let (assignments, slot, relay_vrf_story) = { @@ -451,15 +443,7 @@ pub(crate) async fn handle_new_head( .await { Some(session_info) => session_info, - None => { - gum::error!( - target: LOG_TARGET, - session = session_index, - ?head, - "Can't get session info for the new head" - ); - return Ok(Vec::new()) - }, + None => return Ok(Vec::new()), }; let (block_tick, no_show_duration) = { diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 043ecd923c27..18b8746ca317 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -652,7 +652,7 @@ where { Ok(extended_info) => Some(&extended_info.session_info), Err(_) => { - gum::error!( + gum::debug!( target: LOG_TARGET, session = session_index, ?relay_parent, @@ -695,14 +695,7 @@ impl State { .await { Some(s) => s, - None => { - gum::warn!( - target: LOG_TARGET, - "Unknown session info for {}", - block_entry.session() - ); - return None - }, + None => return None, }; let block_hash = block_entry.block_hash(); @@ -2318,16 +2311,7 @@ async fn process_wakeup( .await { Some(i) => i, - None => { - gum::warn!( - target: LOG_TARGET, - "Missing session info for live block {} in session {}", - relay_block, - block_entry.session(), - ); - - return Ok(Vec::new()) - }, + None => return Ok(Vec::new()), }; let block_tick = slot_number_to_tick(state.slot_duration_millis, block_entry.slot()); @@ -2701,13 +2685,6 @@ async fn issue_approval( { Some(s) => s, None => { - gum::warn!( - target: LOG_TARGET, - "Missing session info for live block {} in session {}", - block_hash, - block_entry.session(), - ); - metrics.on_approval_error(); return Ok(Vec::new()) }, From 2ed7dfd7c0964f102db9634daa3ba69acb80d4b1 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 9 May 2023 11:55:20 +0300 Subject: [PATCH 24/42] Code review feedback --- node/core/approval-voting/src/import.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index 67ba48e70702..1ea2687a0246 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -209,11 +209,9 @@ async fn imported_block_info( } }; - let session_info = - match get_session_info(env.runtime_info, ctx.sender(), block_hash, session_index).await { - Some(session_info) => session_info, - None => return Err(ImportedBlockInfoError::SessionInfoUnavailable), - }; + let session_info = get_session_info(env.runtime_info, ctx.sender(), block_hash, session_index) + .await + .ok_or(ImportedBlockInfoError::SessionInfoUnavailable)?; let (assignments, slot, relay_vrf_story) = { let unsafe_vrf = approval_types::babe_unsafe_vrf_info(&block_header); From 96ffefedfe476d188fd0129706bb8365d5515459 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 9 May 2023 13:25:46 +0300 Subject: [PATCH 25/42] Storage migration removing `COL_SESSION_WINDOW_DATA` from parachains db --- node/core/approval-voting/src/lib.rs | 7 +-- node/service/src/lib.rs | 2 - node/service/src/parachains_db/mod.rs | 40 +++++++++-------- node/service/src/parachains_db/upgrade.rs | 52 ++++++++++++++++++++++- 4 files changed, 75 insertions(+), 26 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 18b8746ca317..232662ce53ff 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -116,8 +116,6 @@ const LOG_TARGET: &str = "parachain::approval-voting"; pub struct Config { /// The column family in the DB where approval-voting data is stored. pub col_approval_data: u32, - /// The of the DB where rolling session info is stored. - pub col_session_data: u32, /// The slot duration of the consensus algorithm, in milliseconds. Should be evenly /// divisible by 500. pub slot_duration_millis: u64, @@ -357,10 +355,7 @@ impl ApprovalVotingSubsystem { keystore, slot_duration_millis: config.slot_duration_millis, db, - db_config: DatabaseConfig { - col_approval_data: config.col_approval_data, - col_session_data: config.col_session_data, - }, + db_config: DatabaseConfig { col_approval_data: config.col_approval_data }, mode: Mode::Syncing(sync_oracle), metrics, } diff --git a/node/service/src/lib.rs b/node/service/src/lib.rs index d25b0e1a0767..d19471ef4623 100644 --- a/node/service/src/lib.rs +++ b/node/service/src/lib.rs @@ -897,7 +897,6 @@ where let approval_voting_config = ApprovalVotingConfig { col_approval_data: parachains_db::REAL_COLUMNS.col_approval_data, - col_session_data: parachains_db::REAL_COLUMNS.col_session_window_data, slot_duration_millis: slot_duration.as_millis() as u64, }; @@ -921,7 +920,6 @@ where let dispute_coordinator_config = DisputeCoordinatorConfig { col_dispute_data: parachains_db::REAL_COLUMNS.col_dispute_coordinator_data, - col_session_data: parachains_db::REAL_COLUMNS.col_session_window_data, }; let rpc_handlers = service::spawn_tasks(service::SpawnTasksParams { diff --git a/node/service/src/parachains_db/mod.rs b/node/service/src/parachains_db/mod.rs index 918aecd25e76..86f30e1c2143 100644 --- a/node/service/src/parachains_db/mod.rs +++ b/node/service/src/parachains_db/mod.rs @@ -46,6 +46,18 @@ pub(crate) mod columns { pub const ORDERED_COL: &[u32] = &[COL_AVAILABILITY_META, COL_CHAIN_SELECTION_DATA, COL_DISPUTE_COORDINATOR_DATA]; } + + pub mod v3 { + pub const NUM_COLUMNS: u32 = 5; + pub const COL_AVAILABILITY_DATA: u32 = 0; + pub const COL_AVAILABILITY_META: u32 = 1; + pub const COL_APPROVAL_DATA: u32 = 2; + pub const COL_CHAIN_SELECTION_DATA: u32 = 3; + pub const COL_DISPUTE_COORDINATOR_DATA: u32 = 4; + + pub const ORDERED_COL: &[u32] = + &[COL_AVAILABILITY_META, COL_CHAIN_SELECTION_DATA, COL_DISPUTE_COORDINATOR_DATA]; + } } /// Columns used by different subsystems. @@ -62,19 +74,16 @@ pub struct ColumnsConfig { pub col_chain_selection_data: u32, /// The column used by dispute coordinator for data. pub col_dispute_coordinator_data: u32, - /// The column used for session window data. - pub col_session_window_data: u32, } /// The real columns used by the parachains DB. #[cfg(any(test, feature = "full-node"))] pub const REAL_COLUMNS: ColumnsConfig = ColumnsConfig { - col_availability_data: columns::v2::COL_AVAILABILITY_DATA, - col_availability_meta: columns::v2::COL_AVAILABILITY_META, - col_approval_data: columns::v2::COL_APPROVAL_DATA, - col_chain_selection_data: columns::v2::COL_CHAIN_SELECTION_DATA, - col_dispute_coordinator_data: columns::v2::COL_DISPUTE_COORDINATOR_DATA, - col_session_window_data: columns::v2::COL_SESSION_WINDOW_DATA, + col_availability_data: columns::v3::COL_AVAILABILITY_DATA, + col_availability_meta: columns::v3::COL_AVAILABILITY_META, + col_approval_data: columns::v3::COL_APPROVAL_DATA, + col_chain_selection_data: columns::v3::COL_CHAIN_SELECTION_DATA, + col_dispute_coordinator_data: columns::v3::COL_DISPUTE_COORDINATOR_DATA, }; #[derive(PartialEq)] @@ -126,16 +135,13 @@ pub fn open_creating_rocksdb( let _ = db_config .memory_budget - .insert(columns::v2::COL_AVAILABILITY_DATA, cache_sizes.availability_data); - let _ = db_config - .memory_budget - .insert(columns::v2::COL_AVAILABILITY_META, cache_sizes.availability_meta); + .insert(columns::v3::COL_AVAILABILITY_DATA, cache_sizes.availability_data); let _ = db_config .memory_budget - .insert(columns::v2::COL_APPROVAL_DATA, cache_sizes.approval_data); + .insert(columns::v3::COL_AVAILABILITY_META, cache_sizes.availability_meta); let _ = db_config .memory_budget - .insert(columns::v2::COL_SESSION_WINDOW_DATA, cache_sizes.session_data); + .insert(columns::v3::COL_APPROVAL_DATA, cache_sizes.approval_data); let path_str = path .to_str() @@ -146,7 +152,7 @@ pub fn open_creating_rocksdb( let db = Database::open(&db_config, &path_str)?; let db = polkadot_node_subsystem_util::database::kvdb_impl::DbAdapter::new( db, - columns::v2::ORDERED_COL, + columns::v3::ORDERED_COL, ); Ok(Arc::new(db)) @@ -166,12 +172,12 @@ pub fn open_creating_paritydb( std::fs::create_dir_all(&path_str)?; upgrade::try_upgrade_db(&path, DatabaseKind::ParityDB)?; - let db = parity_db::Db::open_or_create(&upgrade::paritydb_version_2_config(&path)) + let db = parity_db::Db::open_or_create(&upgrade::paritydb_version_3_config(&path)) .map_err(|err| io::Error::new(io::ErrorKind::Other, format!("{:?}", err)))?; let db = polkadot_node_subsystem_util::database::paritydb_impl::DbAdapter::new( db, - columns::v2::ORDERED_COL, + columns::v3::ORDERED_COL, ); Ok(Arc::new(db)) } diff --git a/node/service/src/parachains_db/upgrade.rs b/node/service/src/parachains_db/upgrade.rs index c52bd21c0573..40a1451872e4 100644 --- a/node/service/src/parachains_db/upgrade.rs +++ b/node/service/src/parachains_db/upgrade.rs @@ -28,7 +28,7 @@ type Version = u32; const VERSION_FILE_NAME: &'static str = "parachain_db_version"; /// Current db version. -const CURRENT_VERSION: Version = 2; +const CURRENT_VERSION: Version = 3; #[derive(thiserror::Error, Debug)] pub enum Error { @@ -58,6 +58,8 @@ pub(crate) fn try_upgrade_db(db_path: &Path, db_kind: DatabaseKind) -> Result<() Some(0) => migrate_from_version_0_to_1(db_path, db_kind)?, // 1 -> 2 migration Some(1) => migrate_from_version_1_to_2(db_path, db_kind)?, + // 2 -> 3 migration + Some(2) => migrate_from_version_2_to_3(db_path, db_kind)?, // Already at current version, do nothing. Some(CURRENT_VERSION) => (), // This is an arbitrary future version, we don't handle it. @@ -127,6 +129,18 @@ fn migrate_from_version_1_to_2(path: &Path, db_kind: DatabaseKind) -> Result<(), }) } +fn migrate_from_version_2_to_3(path: &Path, db_kind: DatabaseKind) -> Result<(), Error> { + gum::info!(target: LOG_TARGET, "Migrating parachains db from version 2 to version 3 ..."); + match db_kind { + DatabaseKind::ParityDB => paritydb_migrate_from_version_2_to_3(path), + DatabaseKind::RocksDB => rocksdb_migrate_from_version_2_to_3(path), + } + .and_then(|result| { + gum::info!(target: LOG_TARGET, "Migration complete! "); + Ok(result) + }) +} + /// Migration from version 0 to version 1: /// * the number of columns has changed from 3 to 5; fn rocksdb_migrate_from_version_0_to_1(path: &Path) -> Result<(), Error> { @@ -160,6 +174,22 @@ fn rocksdb_migrate_from_version_1_to_2(path: &Path) -> Result<(), Error> { Ok(()) } +fn rocksdb_migrate_from_version_2_to_3(path: &Path) -> Result<(), Error> { + use kvdb_rocksdb::{Database, DatabaseConfig}; + + let db_path = path + .to_str() + .ok_or_else(|| super::other_io_error("Invalid database path".into()))?; + let db_cfg = DatabaseConfig::with_columns(super::columns::v1::NUM_COLUMNS); + let mut db = Database::open(&db_cfg, db_path)?; + + // TODO: how to remove the column?? + db.remove_last_column() + .map_err(|e| other_io_error(format!("Error removing column {:?}", e)))?; + + Ok(()) +} + // This currently clears columns which had their configs altered between versions. // The columns to be changed are constrained by the `allowed_columns` vector. fn paritydb_fix_columns( @@ -239,6 +269,17 @@ pub(crate) fn paritydb_version_2_config(path: &Path) -> parity_db::Options { options } +/// Database configuration for version 3. +pub(crate) fn paritydb_version_3_config(path: &Path) -> parity_db::Options { + let mut options = + parity_db::Options::with_columns(&path, super::columns::v3::NUM_COLUMNS as u8); + for i in columns::v3::ORDERED_COL { + options.columns[*i as usize].btree_index = true; + } + + options +} + /// Database configuration for version 0. This is useful just for testing. #[cfg(test)] pub(crate) fn paritydb_version_0_config(path: &Path) -> parity_db::Options { @@ -278,6 +319,15 @@ fn paritydb_migrate_from_version_1_to_2(path: &Path) -> Result<(), Error> { Ok(()) } +/// Migration from version 2 to version 3: +/// - clear any columns used by `RollingSessionWindow` +fn paritydb_migrate_from_version_2_to_3(path: &Path) -> Result<(), Error> { + parity_db::clear_column(path, super::columns::v2::COL_SESSION_WINDOW_DATA as u8) + .map_err(|e| other_io_error(format!("Error clearing COL_SESSION_WINDOW_DATA {:?}", e)))?; + + Ok(()) +} + #[cfg(test)] mod tests { use super::{columns::v2::*, *}; From 81acb359095e31541790a0f86ea12aeb5ebece13 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 9 May 2023 15:30:33 +0300 Subject: [PATCH 26/42] Remove `col_session_data` usages --- node/core/approval-voting/src/approval_db/v1/mod.rs | 2 -- node/core/approval-voting/src/lib.rs | 6 ++---- node/core/dispute-coordinator/src/db/v1.rs | 4 +--- node/core/dispute-coordinator/src/lib.rs | 7 +------ node/service/src/lib.rs | 1 - 5 files changed, 4 insertions(+), 16 deletions(-) diff --git a/node/core/approval-voting/src/approval_db/v1/mod.rs b/node/core/approval-voting/src/approval_db/v1/mod.rs index d2a13ad54550..2a839d623011 100644 --- a/node/core/approval-voting/src/approval_db/v1/mod.rs +++ b/node/core/approval-voting/src/approval_db/v1/mod.rs @@ -154,8 +154,6 @@ pub type Bitfield = BitVec; pub struct Config { /// The column family in the database where data is stored. pub col_approval_data: u32, - /// The column of the database where rolling session window data is stored. - pub col_session_data: u32, } /// Details pertaining to our assignment on a block. diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index 232662ce53ff..f5e888c7c538 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -364,10 +364,8 @@ impl ApprovalVotingSubsystem { /// Revert to the block corresponding to the specified `hash`. /// The operation is not allowed for blocks older than the last finalized one. pub fn revert_to(&self, hash: Hash) -> Result<(), SubsystemError> { - let config = approval_db::v1::Config { - col_approval_data: self.db_config.col_approval_data, - col_session_data: self.db_config.col_session_data, - }; + let config = + approval_db::v1::Config { col_approval_data: self.db_config.col_approval_data }; let mut backend = approval_db::v1::DbBackend::new(self.db.clone(), config); let mut overlay = OverlayedBackend::new(&backend); diff --git a/node/core/dispute-coordinator/src/db/v1.rs b/node/core/dispute-coordinator/src/db/v1.rs index aa67781ddd25..691497ef9e97 100644 --- a/node/core/dispute-coordinator/src/db/v1.rs +++ b/node/core/dispute-coordinator/src/db/v1.rs @@ -206,8 +206,6 @@ fn candidate_votes_session_prefix(session: SessionIndex) -> [u8; 15 + 4] { pub struct ColumnConfiguration { /// The column in the key-value DB where data is stored. pub col_dispute_data: u32, - /// The column in the key-value DB where session data is stored. - pub col_session_data: u32, } /// Tracked votes on candidates, for the purposes of dispute resolution. @@ -378,7 +376,7 @@ mod tests { let db = kvdb_memorydb::create(1); let db = polkadot_node_subsystem_util::database::kvdb_impl::DbAdapter::new(db, &[0]); let store = Arc::new(db); - let config = ColumnConfiguration { col_dispute_data: 0, col_session_data: 1 }; + let config = ColumnConfiguration { col_dispute_data: 0 }; DbBackend::new(store, config, Metrics::default()) } diff --git a/node/core/dispute-coordinator/src/lib.rs b/node/core/dispute-coordinator/src/lib.rs index 7379b392f312..02bb6ef9ecda 100644 --- a/node/core/dispute-coordinator/src/lib.rs +++ b/node/core/dispute-coordinator/src/lib.rs @@ -127,16 +127,11 @@ pub struct DisputeCoordinatorSubsystem { pub struct Config { /// The data column in the store to use for dispute data. pub col_dispute_data: u32, - /// The data column in the store to use for session data. - pub col_session_data: u32, } impl Config { fn column_config(&self) -> db::v1::ColumnConfiguration { - db::v1::ColumnConfiguration { - col_dispute_data: self.col_dispute_data, - col_session_data: self.col_session_data, - } + db::v1::ColumnConfiguration { col_dispute_data: self.col_dispute_data } } } diff --git a/node/service/src/lib.rs b/node/service/src/lib.rs index d19471ef4623..36e23a378fe7 100644 --- a/node/service/src/lib.rs +++ b/node/service/src/lib.rs @@ -1515,7 +1515,6 @@ fn revert_chain_selection(db: Arc, hash: Hash) -> sp_blockchain::R fn revert_approval_voting(db: Arc, hash: Hash) -> sp_blockchain::Result<()> { let config = approval_voting_subsystem::Config { col_approval_data: parachains_db::REAL_COLUMNS.col_approval_data, - col_session_data: parachains_db::REAL_COLUMNS.col_session_window_data, slot_duration_millis: Default::default(), }; From 14eb540e3c9aeed339deae7b86e853e5e9c375e5 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 11 May 2023 15:27:09 +0300 Subject: [PATCH 27/42] Storage migration clearing columns w/o removing them --- node/service/src/parachains_db/mod.rs | 7 +- node/service/src/parachains_db/upgrade.rs | 90 +++++++++++++++++++++-- 2 files changed, 89 insertions(+), 8 deletions(-) diff --git a/node/service/src/parachains_db/mod.rs b/node/service/src/parachains_db/mod.rs index 86f30e1c2143..e3cefee40a88 100644 --- a/node/service/src/parachains_db/mod.rs +++ b/node/service/src/parachains_db/mod.rs @@ -36,9 +36,9 @@ pub(crate) mod columns { pub mod v2 { pub const NUM_COLUMNS: u32 = 6; - pub const COL_AVAILABILITY_DATA: u32 = 0; + // pub const COL_AVAILABILITY_DATA: u32 = 0; pub const COL_AVAILABILITY_META: u32 = 1; - pub const COL_APPROVAL_DATA: u32 = 2; + // pub const COL_APPROVAL_DATA: u32 = 2; pub const COL_CHAIN_SELECTION_DATA: u32 = 3; pub const COL_DISPUTE_COORDINATOR_DATA: u32 = 4; pub const COL_SESSION_WINDOW_DATA: u32 = 5; @@ -48,12 +48,13 @@ pub(crate) mod columns { } pub mod v3 { - pub const NUM_COLUMNS: u32 = 5; + pub const NUM_COLUMNS: u32 = 6; pub const COL_AVAILABILITY_DATA: u32 = 0; pub const COL_AVAILABILITY_META: u32 = 1; pub const COL_APPROVAL_DATA: u32 = 2; pub const COL_CHAIN_SELECTION_DATA: u32 = 3; pub const COL_DISPUTE_COORDINATOR_DATA: u32 = 4; + // pub const COL_SESSION_WINDOW_DATA: u32 = 5; // not used pub const ORDERED_COL: &[u32] = &[COL_AVAILABILITY_META, COL_CHAIN_SELECTION_DATA, COL_DISPUTE_COORDINATOR_DATA]; diff --git a/node/service/src/parachains_db/upgrade.rs b/node/service/src/parachains_db/upgrade.rs index 40a1451872e4..e34fac133a37 100644 --- a/node/service/src/parachains_db/upgrade.rs +++ b/node/service/src/parachains_db/upgrade.rs @@ -175,17 +175,22 @@ fn rocksdb_migrate_from_version_1_to_2(path: &Path) -> Result<(), Error> { } fn rocksdb_migrate_from_version_2_to_3(path: &Path) -> Result<(), Error> { + use kvdb::{DBOp, DBTransaction}; use kvdb_rocksdb::{Database, DatabaseConfig}; let db_path = path .to_str() .ok_or_else(|| super::other_io_error("Invalid database path".into()))?; - let db_cfg = DatabaseConfig::with_columns(super::columns::v1::NUM_COLUMNS); - let mut db = Database::open(&db_cfg, db_path)?; + let db_cfg = DatabaseConfig::with_columns(super::columns::v2::NUM_COLUMNS); + let db = Database::open(&db_cfg, db_path)?; - // TODO: how to remove the column?? - db.remove_last_column() - .map_err(|e| other_io_error(format!("Error removing column {:?}", e)))?; + // Wipe all entries in one operation. + let ops = vec![DBOp::DeletePrefix { + col: super::columns::v2::COL_SESSION_WINDOW_DATA, + prefix: kvdb::DBKey::from_slice(b""), + }]; + + db.write(DBTransaction { ops })?; Ok(()) } @@ -259,6 +264,7 @@ pub(crate) fn paritydb_version_1_config(path: &Path) -> parity_db::Options { } /// Database configuration for version 2. +#[cfg(test)] pub(crate) fn paritydb_version_2_config(path: &Path) -> parity_db::Options { let mut options = parity_db::Options::with_columns(&path, super::columns::v2::NUM_COLUMNS as u8); @@ -466,4 +472,78 @@ mod tests { Some("0xdeadb00b".as_bytes().to_vec()) ); } + + #[test] + fn test_paritydb_migrate_2_to_3() { + use parity_db::Db; + + let db_dir = tempfile::tempdir().unwrap(); + let path = db_dir.path(); + let test_key = b"1337"; + + // We need to properly set db version for upgrade to work. + fs::write(version_file_path(path), "2").expect("Failed to write DB version"); + + { + let db = Db::open_or_create(&paritydb_version_2_config(&path)).unwrap(); + + // Write some dummy data + db.commit(vec![( + COL_SESSION_WINDOW_DATA as u8, + test_key.to_vec(), + Some(b"0xdeadb00b".to_vec()), + )]) + .unwrap(); + + assert_eq!(db.num_columns(), columns::v2::NUM_COLUMNS as u8); + } + + try_upgrade_db(&path, DatabaseKind::ParityDB).unwrap(); + + let db = Db::open(&paritydb_version_3_config(&path)).unwrap(); + + assert_eq!(db.num_columns(), columns::v3::NUM_COLUMNS as u8); + + // ensure column is empty + assert!(db.get(COL_SESSION_WINDOW_DATA as u8, &test_key.to_vec()).unwrap().is_none()); + } + + #[test] + fn test_rocksdb_migrate_2_to_3() { + use kvdb::{DBKey, DBOp}; + use kvdb_rocksdb::{Database, DatabaseConfig}; + use polkadot_node_subsystem_util::database::{ + kvdb_impl::DbAdapter, DBTransaction, KeyValueDB, + }; + + let db_dir = tempfile::tempdir().unwrap(); + let db_path = db_dir.path().to_str().unwrap(); + let db_cfg = DatabaseConfig::with_columns(super::columns::v2::NUM_COLUMNS); + let db = Database::open(&db_cfg, db_path).unwrap(); + assert_eq!(db.num_columns(), super::columns::v2::NUM_COLUMNS as u32); + + // We need to properly set db version for upgrade to work. + fs::write(version_file_path(db_dir.path()), "2").expect("Failed to write DB version"); + { + let db = DbAdapter::new(db, columns::v2::ORDERED_COL); + db.write(DBTransaction { + ops: vec![DBOp::Insert { + col: COL_DISPUTE_COORDINATOR_DATA, + key: DBKey::from_slice(b"1234"), + value: b"0xdeadb00b".to_vec(), + }], + }) + .unwrap(); + } + + try_upgrade_db(&db_dir.path(), DatabaseKind::RocksDB).unwrap(); + + let db_cfg = DatabaseConfig::with_columns(super::columns::v3::NUM_COLUMNS); + let db = Database::open(&db_cfg, db_path).unwrap(); + + assert_eq!(db.num_columns(), super::columns::v3::NUM_COLUMNS); + + // Ensure data is actually removed + assert!(db.get(COL_SESSION_WINDOW_DATA, b"1337").unwrap().is_none()); + } } From 0606eeeb6de146e21fddf978b6b99c87cf30f7f0 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 11 May 2023 16:01:59 +0300 Subject: [PATCH 28/42] Remove session data column usages from `approval-voting` and `dispute-coordinator` tests --- node/core/approval-voting/src/approval_db/v1/tests.rs | 4 +--- node/core/approval-voting/src/import.rs | 4 +--- node/core/approval-voting/src/tests.rs | 7 +------ node/core/dispute-coordinator/src/tests.rs | 2 +- 4 files changed, 4 insertions(+), 13 deletions(-) diff --git a/node/core/approval-voting/src/approval_db/v1/tests.rs b/node/core/approval-voting/src/approval_db/v1/tests.rs index 0d30cc8c0cdc..e4bab5b69464 100644 --- a/node/core/approval-voting/src/approval_db/v1/tests.rs +++ b/node/core/approval-voting/src/approval_db/v1/tests.rs @@ -28,12 +28,10 @@ use std::{collections::HashMap, sync::Arc}; use ::test_helpers::{dummy_candidate_receipt, dummy_candidate_receipt_bad_sig, dummy_hash}; const DATA_COL: u32 = 0; -const SESSION_DATA_COL: u32 = 1; const NUM_COLUMNS: u32 = 2; -const TEST_CONFIG: Config = - Config { col_approval_data: DATA_COL, col_session_data: SESSION_DATA_COL }; +const TEST_CONFIG: Config = Config { col_approval_data: DATA_COL }; fn make_db() -> (DbBackend, Arc) { let db = kvdb_memorydb::create(NUM_COLUMNS); diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index 1ea2687a0246..bd9dd03a4f2a 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -609,12 +609,10 @@ pub(crate) mod tests { use crate::{approval_db::v1::Config as DatabaseConfig, criteria, BlockEntry}; const DATA_COL: u32 = 0; - const SESSION_DATA_COL: u32 = 1; const NUM_COLUMNS: u32 = 2; - const TEST_CONFIG: DatabaseConfig = - DatabaseConfig { col_approval_data: DATA_COL, col_session_data: SESSION_DATA_COL }; + const TEST_CONFIG: DatabaseConfig = DatabaseConfig { col_approval_data: DATA_COL }; #[derive(Default)] struct MockClock; diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index d7e19a8c09f3..c75aded022cb 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -14,8 +14,6 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use crate::tests::test_constants::TEST_CONFIG; - use super::*; use polkadot_node_primitives::{ approval::{ @@ -115,12 +113,10 @@ fn make_sync_oracle(val: bool) -> (Box, TestSyncOracleHan pub mod test_constants { use crate::approval_db::v1::Config as DatabaseConfig; const DATA_COL: u32 = 0; - const SESSION_DATA_COL: u32 = 1; pub(crate) const NUM_COLUMNS: u32 = 2; - pub(crate) const TEST_CONFIG: DatabaseConfig = - DatabaseConfig { col_approval_data: DATA_COL, col_session_data: SESSION_DATA_COL }; + pub(crate) const TEST_CONFIG: DatabaseConfig = DatabaseConfig { col_approval_data: DATA_COL }; } struct MockSupportsParachains; @@ -493,7 +489,6 @@ fn test_harness>( Config { col_approval_data: test_constants::TEST_CONFIG.col_approval_data, slot_duration_millis: SLOT_DURATION_MILLIS, - col_session_data: TEST_CONFIG.col_session_data, }, Arc::new(db), Arc::new(keystore), diff --git a/node/core/dispute-coordinator/src/tests.rs b/node/core/dispute-coordinator/src/tests.rs index b685660d62b7..caeccffd424a 100644 --- a/node/core/dispute-coordinator/src/tests.rs +++ b/node/core/dispute-coordinator/src/tests.rs @@ -213,7 +213,7 @@ impl Default for TestState { let db = kvdb_memorydb::create(1); let db = polkadot_node_subsystem_util::database::kvdb_impl::DbAdapter::new(db, &[]); let db = Arc::new(db); - let config = Config { col_dispute_data: 0, col_session_data: 1 }; + let config = Config { col_dispute_data: 0 }; let genesis_header = Header { parent_hash: Hash::zero(), From 6a2a9e2787925fa30e55c9c18cde78f79bf35ecd Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 12 May 2023 16:21:33 +0300 Subject: [PATCH 29/42] Add some test cases from `RollingSessionWindow` to `dispute-coordinator` tests --- node/core/dispute-coordinator/src/tests.rs | 178 ++++++++++++++++++++- 1 file changed, 176 insertions(+), 2 deletions(-) diff --git a/node/core/dispute-coordinator/src/tests.rs b/node/core/dispute-coordinator/src/tests.rs index caeccffd424a..028aac54480f 100644 --- a/node/core/dispute-coordinator/src/tests.rs +++ b/node/core/dispute-coordinator/src/tests.rs @@ -33,6 +33,7 @@ use polkadot_node_subsystem_util::database::Database; use polkadot_node_primitives::{ DisputeMessage, DisputeStatus, SignedDisputeStatement, SignedFullStatement, Statement, + DISPUTE_WINDOW, }; use polkadot_node_subsystem::{ messages::{ @@ -211,7 +212,7 @@ impl Default for TestState { make_keystore(vec![Sr25519Keyring::Alice.to_seed()].into_iter()).into(); let db = kvdb_memorydb::create(1); - let db = polkadot_node_subsystem_util::database::kvdb_impl::DbAdapter::new(db, &[]); + let db = polkadot_node_subsystem_util::database::kvdb_impl::DbAdapter::new(db, &[0]); let db = Arc::new(db); let config = Config { col_dispute_data: 0 }; @@ -327,9 +328,11 @@ impl TestState { assert_eq!(h, block_hash); let _ = tx.send(Ok(session)); + let first_expected_session = session.saturating_sub(DISPUTE_WINDOW.get() - 1); + // Queries for session caching - see `handle_startup` if self.known_session.is_none() { - for i in 0..=session { + for i in first_expected_session..=session { assert_matches!( overseer_recv(virtual_overseer).await, AllMessages::RuntimeApi(RuntimeApiMessage::Request( @@ -3379,3 +3382,174 @@ fn informs_chain_selection_when_dispute_concluded_against() { }) }); } + +// On startup `SessionInfo` cache should be populated +#[test] +fn session_info_caching_on_startup_works() { + 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; + + test_state + }) + }); +} + +// Underflow means that no more than `DISPUTE_WINDOW` sessions should be fetched on startup +#[test] +fn session_info_caching_doesnt_underflow() { + test_harness(|mut test_state, mut virtual_overseer| { + Box::pin(async move { + let session = DISPUTE_WINDOW.get() + 1; + + test_state.handle_resume_sync(&mut virtual_overseer, session).await; + + test_state + }) + }); +} + +// Cached `SessionInfo` shouldn't be re-requested from the runtime +#[test] +fn session_info_is_requested_only_once() { + 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; + + // This leaf activation shouldn't fetch `SessionInfo` because the session is already cached + test_state + .activate_leaf_at_session( + &mut virtual_overseer, + session, + 3, + vec![make_candidate_included_event(make_valid_candidate_receipt())], + ) + .await; + + // This leaf activation should fetch `SessionInfo` because the session is new + test_state + .activate_leaf_at_session( + &mut virtual_overseer, + session + 1, + 4, + vec![make_candidate_included_event(make_valid_candidate_receipt())], + ) + .await; + + assert_matches!( + virtual_overseer.recv().await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _, + RuntimeApiRequest::SessionInfo(session_index, tx), + )) => { + assert_eq!(session_index, 2); + let _ = tx.send(Ok(Some(test_state.session_info()))); + } + ); + test_state + }) + }); +} + +// Big jump means the new session we see with a leaf update is at least a `DISPUTE_WINDOW` bigger than +// the already known one. In this case The whole `DISPUTE_WINDOW` should be fetched. +#[test] +fn session_info_big_jump_works() { + test_harness(|mut test_state, mut virtual_overseer| { + Box::pin(async move { + let session_on_startup = 1; + + test_state.handle_resume_sync(&mut virtual_overseer, session_on_startup).await; + + // This leaf activation shouldn't fetch `SessionInfo` because the session is already cached + test_state + .activate_leaf_at_session( + &mut virtual_overseer, + session_on_startup, + 3, + vec![make_candidate_included_event(make_valid_candidate_receipt())], + ) + .await; + + let session_after_jump = session_on_startup + DISPUTE_WINDOW.get() + 10; + // This leaf activation should cache all missing `SessionInfo`s + test_state + .activate_leaf_at_session( + &mut virtual_overseer, + session_after_jump, + 4, + vec![make_candidate_included_event(make_valid_candidate_receipt())], + ) + .await; + + let first_expected_session = + session_after_jump.saturating_sub(DISPUTE_WINDOW.get() - 1); + for expected_idx in first_expected_session..=session_after_jump { + assert_matches!( + virtual_overseer.recv().await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _, + RuntimeApiRequest::SessionInfo(session_index, tx), + )) => { + assert_eq!(session_index, expected_idx); + let _ = tx.send(Ok(Some(test_state.session_info()))); + } + ); + } + test_state + }) + }); +} + +// Small jump means the new session we see with a leaf update is at less than last known one + `DISPUTE_WINDOW`. In this +// case fetching should start from last known one + 1. +#[test] +fn session_info_small_jump_works() { + test_harness(|mut test_state, mut virtual_overseer| { + Box::pin(async move { + let session_on_startup = 1; + + test_state.handle_resume_sync(&mut virtual_overseer, session_on_startup).await; + + // This leaf activation shouldn't fetch `SessionInfo` because the session is already cached + test_state + .activate_leaf_at_session( + &mut virtual_overseer, + session_on_startup, + 3, + vec![make_candidate_included_event(make_valid_candidate_receipt())], + ) + .await; + + let session_after_jump = session_on_startup + DISPUTE_WINDOW.get() - 1; + // This leaf activation should cache all missing `SessionInfo`s + test_state + .activate_leaf_at_session( + &mut virtual_overseer, + session_after_jump, + 4, + vec![make_candidate_included_event(make_valid_candidate_receipt())], + ) + .await; + + let first_expected_session = session_on_startup + 1; + for expected_idx in first_expected_session..=session_after_jump { + assert_matches!( + virtual_overseer.recv().await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _, + RuntimeApiRequest::SessionInfo(session_index, tx), + )) => { + assert_eq!(session_index, expected_idx); + let _ = tx.send(Ok(Some(test_state.session_info()))); + } + ); + } + test_state + }) + }); +} From 0f94664ec9f3a7e3737a30291195990e1e7065fc Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 12 May 2023 16:22:48 +0300 Subject: [PATCH 30/42] Fix formatting in initialized.rs --- .../dispute-coordinator/src/initialized.rs | 101 +++++++++--------- node/network/approval-distribution/src/lib.rs | 10 +- 2 files changed, 55 insertions(+), 56 deletions(-) diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index c0eb029f4d0f..6644bebc4776 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -214,62 +214,61 @@ impl Initialized { gum::trace!(target: LOG_TARGET, "Waiting for message"); let mut overlay_db = OverlayedBackend::new(backend); let default_confirm = Box::new(|| Ok(())); - let confirm_write = match MuxedMessage::receive(ctx, &mut self.participation_receiver) - .await? - { - MuxedMessage::Participation(msg) => { - gum::trace!(target: LOG_TARGET, "MuxedMessage::Participation"); - let ParticipationStatement { - session, - candidate_hash, - candidate_receipt, - outcome, - } = self.participation.get_participation_result(ctx, msg).await?; - if let Some(valid) = outcome.validity() { - gum::trace!( - target: LOG_TARGET, - ?session, - ?candidate_hash, - ?valid, - "Issuing local statement based on participation outcome." - ); - self.issue_local_statement( - ctx, - &mut overlay_db, + let confirm_write = + match MuxedMessage::receive(ctx, &mut self.participation_receiver).await? { + MuxedMessage::Participation(msg) => { + gum::trace!(target: LOG_TARGET, "MuxedMessage::Participation"); + let ParticipationStatement { + session, candidate_hash, candidate_receipt, - session, - valid, - clock.now(), - ) - .await?; - } else { - gum::warn!(target: LOG_TARGET, ?outcome, "Dispute participation failed"); - } - default_confirm - }, - MuxedMessage::Subsystem(msg) => match msg { - FromOrchestra::Signal(OverseerSignal::Conclude) => return Ok(()), - FromOrchestra::Signal(OverseerSignal::ActiveLeaves(update)) => { - gum::trace!(target: LOG_TARGET, "OverseerSignal::ActiveLeaves"); - self.process_active_leaves_update( - ctx, - &mut overlay_db, - update, - clock.now(), - ) - .await?; + outcome, + } = self.participation.get_participation_result(ctx, msg).await?; + if let Some(valid) = outcome.validity() { + gum::trace!( + target: LOG_TARGET, + ?session, + ?candidate_hash, + ?valid, + "Issuing local statement based on participation outcome." + ); + self.issue_local_statement( + ctx, + &mut overlay_db, + candidate_hash, + candidate_receipt, + session, + valid, + clock.now(), + ) + .await?; + } else { + gum::warn!(target: LOG_TARGET, ?outcome, "Dispute participation failed"); + } default_confirm }, - FromOrchestra::Signal(OverseerSignal::BlockFinalized(_, n)) => { - gum::trace!(target: LOG_TARGET, "OverseerSignal::BlockFinalized"); - self.scraper.process_finalized_block(&n); - default_confirm + MuxedMessage::Subsystem(msg) => match msg { + FromOrchestra::Signal(OverseerSignal::Conclude) => return Ok(()), + FromOrchestra::Signal(OverseerSignal::ActiveLeaves(update)) => { + gum::trace!(target: LOG_TARGET, "OverseerSignal::ActiveLeaves"); + self.process_active_leaves_update( + ctx, + &mut overlay_db, + update, + clock.now(), + ) + .await?; + default_confirm + }, + FromOrchestra::Signal(OverseerSignal::BlockFinalized(_, n)) => { + gum::trace!(target: LOG_TARGET, "OverseerSignal::BlockFinalized"); + self.scraper.process_finalized_block(&n); + default_confirm + }, + FromOrchestra::Communication { msg } => + self.handle_incoming(ctx, &mut overlay_db, msg, clock.now()).await?, }, - FromOrchestra::Communication { msg } => - self.handle_incoming(ctx, &mut overlay_db, msg, clock.now()).await?, - }, - }; + }; if !overlay_db.is_empty() { let ops = overlay_db.into_write_ops(); diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index c8ad21ad406e..82f62502e474 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -1270,11 +1270,11 @@ impl State { let candidate_entry = match block_entry.candidates.get(index as usize) { None => { gum::debug!( - target: LOG_TARGET, - ?hash, - ?index, - "`get_approval_signatures`: could not find candidate entry for given hash and index!" - ); + target: LOG_TARGET, + ?hash, + ?index, + "`get_approval_signatures`: could not find candidate entry for given hash and index!" + ); continue }, Some(e) => e, From db120f60371b43feb45eae7ab530e865203e5b33 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 12 May 2023 16:27:13 +0300 Subject: [PATCH 31/42] Fix a corner case in `SessionInfo` caching for `dispute-coordinator` --- node/core/dispute-coordinator/src/initialized.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index 6644bebc4776..df301c5fad04 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -305,13 +305,12 @@ impl Initialized { Ok(session_idx) if self.gaps_in_cache || session_idx > self.highest_session_seen => { - // If error has occurred during last session caching - fetch the whole window - // Otherwise - cache only the new sessions - let lower_bound = if self.gaps_in_cache { - session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1) - } else { - self.highest_session_seen + 1 - }; + // Fetch the last `DISPUTE_WINDOW` number of sessions unless there are no gaps in + // cache and we are not missing too many `SessionInfo`s + let mut lower_bound = session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1); + if !self.gaps_in_cache && self.highest_session_seen > lower_bound { + lower_bound = self.highest_session_seen + 1 + } // There is a new session. Perform a dummy fetch to cache it. for idx in lower_bound..=session_idx { From 0ffd17ff44ba8be0559fce3eb41b037a0709bda2 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 12 May 2023 16:33:31 +0300 Subject: [PATCH 32/42] Remove `RollingSessionWindow` ;( --- node/subsystem-util/src/lib.rs | 2 - .../src/rolling_session_window.rs | 1532 ----------------- 2 files changed, 1534 deletions(-) delete mode 100644 node/subsystem-util/src/rolling_session_window.rs diff --git a/node/subsystem-util/src/lib.rs b/node/subsystem-util/src/lib.rs index 6c16cf396c40..1444bc0a2bf1 100644 --- a/node/subsystem-util/src/lib.rs +++ b/node/subsystem-util/src/lib.rs @@ -65,8 +65,6 @@ pub mod reexports { pub use polkadot_overseer::gen::{SpawnedSubsystem, Spawner, Subsystem, SubsystemContext}; } -/// A rolling session window cache. -pub mod rolling_session_window; /// Convenient and efficient runtime info access. pub mod runtime; diff --git a/node/subsystem-util/src/rolling_session_window.rs b/node/subsystem-util/src/rolling_session_window.rs deleted file mode 100644 index 18364491849a..000000000000 --- a/node/subsystem-util/src/rolling_session_window.rs +++ /dev/null @@ -1,1532 +0,0 @@ -// Copyright (C) Parity Technologies (UK) Ltd. -// This file is part of Polkadot. - -// Polkadot is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// Polkadot is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with Polkadot. If not, see . - -//! A rolling window of sessions and cached session info, updated by the state of newly imported blocks. -//! -//! This is useful for consensus components which need to stay up-to-date about recent sessions but don't -//! care about the state of particular blocks. - -use super::database::{DBTransaction, Database}; -use kvdb::{DBKey, DBOp}; - -use parity_scale_codec::{Decode, Encode}; -pub use polkadot_node_primitives::{new_session_window_size, SessionWindowSize}; -use polkadot_primitives::{BlockNumber, Hash, SessionIndex, SessionInfo}; -use std::sync::Arc; - -use futures::channel::oneshot; -use polkadot_node_subsystem::{ - errors::{ChainApiError, RuntimeApiError}, - messages::{ChainApiMessage, RuntimeApiMessage, RuntimeApiRequest}, - overseer, -}; - -// The window size is equal to the `approval-voting` and `dispute-coordinator` constants that -// have been obsoleted. -const SESSION_WINDOW_SIZE: SessionWindowSize = new_session_window_size!(6); -const LOG_TARGET: &str = "parachain::rolling-session-window"; -const STORED_ROLLING_SESSION_WINDOW: &[u8] = b"Rolling_session_window"; - -/// Sessions unavailable in state to cache. -#[derive(Debug, Clone, thiserror::Error)] -pub enum SessionsUnavailableReason { - /// Runtime API subsystem was unavailable. - #[error(transparent)] - RuntimeApiUnavailable(#[from] oneshot::Canceled), - /// The runtime API itself returned an error. - #[error(transparent)] - RuntimeApi(#[from] RuntimeApiError), - /// The chain API itself returned an error. - #[error(transparent)] - ChainApi(#[from] ChainApiError), - /// Missing session info from runtime API for given `SessionIndex`. - #[error("Missing session index {0:?}")] - Missing(SessionIndex), - /// Missing last finalized block number. - #[error("Missing last finalized block number")] - MissingLastFinalizedBlock, - /// Missing last finalized block hash. - #[error("Missing last finalized block hash")] - MissingLastFinalizedBlockHash(BlockNumber), -} - -/// Information about the sessions being fetched. -#[derive(Debug, Clone)] -pub struct SessionsUnavailableInfo { - /// The desired window start. - pub window_start: SessionIndex, - /// The desired window end. - pub window_end: SessionIndex, - /// The block hash whose state the sessions were meant to be drawn from. - pub block_hash: Hash, -} - -/// Sessions were unavailable to fetch from the state for some reason. -#[derive(Debug, thiserror::Error, Clone)] -#[error("Sessions unavailable: {kind:?}, info: {info:?}")] -pub struct SessionsUnavailable { - /// The error kind. - #[source] - kind: SessionsUnavailableReason, - /// The info about the session window, if any. - info: Option, -} - -/// An indicated update of the rolling session window. -#[derive(Debug, PartialEq, Clone)] -pub enum SessionWindowUpdate { - /// The session window was just advanced from one range to a new one. - Advanced { - /// The previous start of the window (inclusive). - prev_window_start: SessionIndex, - /// The previous end of the window (inclusive). - prev_window_end: SessionIndex, - /// The new start of the window (inclusive). - new_window_start: SessionIndex, - /// The new end of the window (inclusive). - new_window_end: SessionIndex, - }, - /// The session window was unchanged. - Unchanged, -} - -/// A structure to store rolling session database parameters. -#[derive(Clone)] -pub struct DatabaseParams { - /// Database reference. - pub db: Arc, - /// The column which stores the rolling session info. - pub db_column: u32, -} -/// A rolling window of sessions and cached session info. -pub struct RollingSessionWindow { - earliest_session: SessionIndex, - session_info: Vec, - window_size: SessionWindowSize, - // The option is just to enable some approval-voting tests to force feed sessions - // in the window without dealing with the DB. - db_params: Option, -} - -/// The rolling session data we persist in the database. -#[derive(Encode, Decode, Default)] -struct StoredWindow { - earliest_session: SessionIndex, - session_info: Vec, -} - -impl RollingSessionWindow { - /// Initialize a new session info cache with the given window size. - /// Invariant: The database always contains the earliest session. Then, - /// we can always extend the session info vector using chain state. - pub async fn new( - mut sender: Sender, - block_hash: Hash, - db_params: DatabaseParams, - ) -> Result - where - Sender: overseer::SubsystemSender - + overseer::SubsystemSender, - { - // At first, determine session window start using the chain state. - let session_index = get_session_index_for_child(&mut sender, block_hash).await?; - let earliest_non_finalized_block_session = - Self::earliest_non_finalized_block_session(&mut sender).await?; - - // This will increase the session window to cover the full unfinalized chain. - let on_chain_window_start = std::cmp::min( - session_index.saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - earliest_non_finalized_block_session, - ); - - // Fetch session information from DB. - let maybe_stored_window = Self::db_load(db_params.clone()); - - // Get the DB stored sessions and recompute window start based on DB data. - let (mut window_start, stored_sessions) = - if let Some(mut stored_window) = maybe_stored_window { - // Check if DB is ancient. - if earliest_non_finalized_block_session > - stored_window.earliest_session + stored_window.session_info.len() as u32 - { - // If ancient, we scrap it and fetch from chain state. - stored_window.session_info.clear(); - } - - // The session window might extend beyond the last finalized block, but that's fine as we'll prune it at - // next update. - let window_start = if stored_window.session_info.len() > 0 { - // If there is at least one entry in db, we always take the DB as source of truth. - stored_window.earliest_session - } else { - on_chain_window_start - }; - - (window_start, stored_window.session_info) - } else { - (on_chain_window_start, Vec::new()) - }; - - // Compute the amount of sessions missing from the window that will be fetched from chain state. - let sessions_missing_count = session_index - .saturating_sub(window_start) - .saturating_add(1) - .saturating_sub(stored_sessions.len() as u32); - - // Extend from chain state. - let sessions = if sessions_missing_count > 0 { - match extend_sessions_from_chain_state( - stored_sessions, - &mut sender, - block_hash, - &mut window_start, - session_index, - ) - .await - { - Err(kind) => Err(SessionsUnavailable { - kind, - info: Some(SessionsUnavailableInfo { - window_start, - window_end: session_index, - block_hash, - }), - }), - Ok(sessions) => Ok(sessions), - }? - } else { - // There are no new sessions to be fetched from chain state. - stored_sessions - }; - - Ok(Self { - earliest_session: window_start, - session_info: sessions, - window_size: SESSION_WINDOW_SIZE, - db_params: Some(db_params), - }) - } - - // Load session information from the parachains db. - fn db_load(db_params: DatabaseParams) -> Option { - match db_params.db.get(db_params.db_column, STORED_ROLLING_SESSION_WINDOW).ok()? { - None => None, - Some(raw) => { - let maybe_decoded = StoredWindow::decode(&mut &raw[..]).map(Some); - match maybe_decoded { - Ok(decoded) => decoded, - Err(err) => { - gum::warn!( - target: LOG_TARGET, - ?err, - "Failed decoding db entry; will start with onchain session infos and self-heal DB entry on next update." - ); - None - }, - } - }, - } - } - - // Saves/Updates all sessions in the database. - // TODO: https://github.com/paritytech/polkadot/issues/6144 - fn db_save(&mut self, stored_window: StoredWindow) { - if let Some(db_params) = self.db_params.as_ref() { - match db_params.db.write(DBTransaction { - ops: vec![DBOp::Insert { - col: db_params.db_column, - key: DBKey::from_slice(STORED_ROLLING_SESSION_WINDOW), - value: stored_window.encode(), - }], - }) { - Ok(_) => {}, - Err(err) => { - gum::warn!(target: LOG_TARGET, ?err, "Failed writing db entry"); - }, - } - } - } - - /// Initialize a new session info cache with the given window size and - /// initial data. - /// This is only used in `approval voting` tests. - pub fn with_session_info( - earliest_session: SessionIndex, - session_info: Vec, - ) -> Self { - RollingSessionWindow { - earliest_session, - session_info, - window_size: SESSION_WINDOW_SIZE, - db_params: None, - } - } - - /// Access the session info for the given session index, if stored within the window. - pub fn session_info(&self, index: SessionIndex) -> Option<&SessionInfo> { - if index < self.earliest_session { - None - } else { - self.session_info.get((index - self.earliest_session) as usize) - } - } - - /// Access the index of the earliest session. - pub fn earliest_session(&self) -> SessionIndex { - self.earliest_session - } - - /// Access the index of the latest session. - pub fn latest_session(&self) -> SessionIndex { - self.earliest_session + (self.session_info.len() as SessionIndex).saturating_sub(1) - } - - /// Returns `true` if `session_index` is contained in the window. - pub fn contains(&self, session_index: SessionIndex) -> bool { - session_index >= self.earliest_session() && session_index <= self.latest_session() - } - - async fn earliest_non_finalized_block_session( - sender: &mut Sender, - ) -> Result - where - Sender: overseer::SubsystemSender - + overseer::SubsystemSender, - { - let last_finalized_height = { - let (tx, rx) = oneshot::channel(); - sender.send_message(ChainApiMessage::FinalizedBlockNumber(tx)).await; - match rx.await { - Ok(Ok(number)) => number, - Ok(Err(e)) => - return Err(SessionsUnavailable { - kind: SessionsUnavailableReason::ChainApi(e), - info: None, - }), - Err(err) => { - gum::warn!( - target: LOG_TARGET, - ?err, - "Failed fetching last finalized block number" - ); - return Err(SessionsUnavailable { - kind: SessionsUnavailableReason::MissingLastFinalizedBlock, - info: None, - }) - }, - } - }; - - let (tx, rx) = oneshot::channel(); - // We want to get the session index for the child of the last finalized block. - sender - .send_message(ChainApiMessage::FinalizedBlockHash(last_finalized_height, tx)) - .await; - let last_finalized_hash_parent = match rx.await { - Ok(Ok(maybe_hash)) => maybe_hash, - Ok(Err(e)) => - return Err(SessionsUnavailable { - kind: SessionsUnavailableReason::ChainApi(e), - info: None, - }), - Err(err) => { - gum::warn!(target: LOG_TARGET, ?err, "Failed fetching last finalized block hash"); - return Err(SessionsUnavailable { - kind: SessionsUnavailableReason::MissingLastFinalizedBlockHash( - last_finalized_height, - ), - info: None, - }) - }, - }; - - // Get the session in which the last finalized block was authored. - if let Some(last_finalized_hash_parent) = last_finalized_hash_parent { - let session = - match get_session_index_for_child(sender, last_finalized_hash_parent).await { - Ok(session_index) => session_index, - Err(err) => { - gum::warn!( - target: LOG_TARGET, - ?err, - ?last_finalized_hash_parent, - "Failed fetching session index" - ); - return Err(err) - }, - }; - - Ok(session) - } else { - return Err(SessionsUnavailable { - kind: SessionsUnavailableReason::MissingLastFinalizedBlockHash( - last_finalized_height, - ), - info: None, - }) - } - } - - /// When inspecting a new import notification, updates the session info cache to match - /// the session of the imported block's child. - /// - /// this only needs to be called on heads where we are directly notified about import, as sessions do - /// not change often and import notifications are expected to be typically increasing in session number. - /// - /// some backwards drift in session index is acceptable. - pub async fn cache_session_info_for_head( - &mut self, - sender: &mut Sender, - block_hash: Hash, - ) -> Result - where - Sender: overseer::SubsystemSender - + overseer::SubsystemSender, - { - let session_index = get_session_index_for_child(sender, block_hash).await?; - let latest = self.latest_session(); - - // Either cached or ancient. - if session_index <= latest { - return Ok(SessionWindowUpdate::Unchanged) - } - - let earliest_non_finalized_block_session = - Self::earliest_non_finalized_block_session(sender).await?; - - let old_window_start = self.earliest_session; - let old_window_end = latest; - - // Ensure we keep sessions up to last finalized block by adjusting the window start. - // This will increase the session window to cover the full unfinalized chain. - let window_start = std::cmp::min( - session_index.saturating_sub(self.window_size.get() - 1), - earliest_non_finalized_block_session, - ); - - // Never look back past earliest session, since if sessions beyond were not needed or available - // in the past remains valid for the future (window only advances forward). - let mut window_start = std::cmp::max(window_start, self.earliest_session); - - let mut sessions = self.session_info.clone(); - let sessions_out_of_window = window_start.saturating_sub(old_window_start) as usize; - - let sessions = if sessions_out_of_window < sessions.len() { - // Drop sessions based on how much the window advanced. - sessions.split_off((window_start as usize).saturating_sub(old_window_start as usize)) - } else { - // Window has jumped such that we need to fetch all sessions from on chain. - Vec::new() - }; - - match extend_sessions_from_chain_state( - sessions, - sender, - block_hash, - &mut window_start, - session_index, - ) - .await - { - Err(kind) => Err(SessionsUnavailable { - kind, - info: Some(SessionsUnavailableInfo { - window_start, - window_end: session_index, - block_hash, - }), - }), - Ok(s) => { - let update = SessionWindowUpdate::Advanced { - prev_window_start: old_window_start, - prev_window_end: old_window_end, - new_window_start: window_start, - new_window_end: session_index, - }; - - self.session_info = s; - - // we need to account for this case: - // window_start ................................... session_index - // old_window_start ........... latest - let new_earliest = std::cmp::max(window_start, old_window_start); - self.earliest_session = new_earliest; - - // Update current window in DB. - self.db_save(StoredWindow { - earliest_session: self.earliest_session, - session_info: self.session_info.clone(), - }); - Ok(update) - }, - } - } -} - -// Returns the session index expected at any child of the `parent` block. -// -// Note: We could use `RuntimeInfo::get_session_index_for_child` here but it's -// cleaner to just call the runtime API directly without needing to create an instance -// of `RuntimeInfo`. -async fn get_session_index_for_child( - sender: &mut impl overseer::SubsystemSender, - block_hash: Hash, -) -> Result { - let (s_tx, s_rx) = oneshot::channel(); - - // We're requesting session index of a child to populate the cache in advance. - sender - .send_message(RuntimeApiMessage::Request( - block_hash, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) - .await; - - match s_rx.await { - Ok(Ok(s)) => Ok(s), - Ok(Err(e)) => - return Err(SessionsUnavailable { - kind: SessionsUnavailableReason::RuntimeApi(e), - info: None, - }), - Err(e) => - return Err(SessionsUnavailable { - kind: SessionsUnavailableReason::RuntimeApiUnavailable(e), - info: None, - }), - } -} - -/// Attempts to extend db stored sessions with sessions missing between `start` and up to `end_inclusive`. -/// Runtime session info fetching errors are ignored if that doesn't create a gap in the window. -async fn extend_sessions_from_chain_state( - stored_sessions: Vec, - sender: &mut impl overseer::SubsystemSender, - block_hash: Hash, - window_start: &mut SessionIndex, - end_inclusive: SessionIndex, -) -> Result, SessionsUnavailableReason> { - // Start from the db sessions. - let mut sessions = stored_sessions; - // We allow session fetch failures only if we won't create a gap in the window by doing so. - // If `allow_failure` is set to true here, fetching errors are ignored until we get a first session. - let mut allow_failure = sessions.is_empty(); - - let start = *window_start + sessions.len() as u32; - - for i in start..=end_inclusive { - let (tx, rx) = oneshot::channel(); - sender - .send_message(RuntimeApiMessage::Request( - block_hash, - RuntimeApiRequest::SessionInfo(i, tx), - )) - .await; - - match rx.await { - Ok(Ok(Some(session_info))) => { - // We do not allow failure anymore after having at least 1 session in window. - allow_failure = false; - sessions.push(session_info); - }, - Ok(Ok(None)) if !allow_failure => return Err(SessionsUnavailableReason::Missing(i)), - Ok(Ok(None)) => { - // Handle `allow_failure` true. - // If we didn't get the session, we advance window start. - *window_start += 1; - gum::debug!( - target: LOG_TARGET, - session = ?i, - "Session info missing from runtime." - ); - }, - Ok(Err(e)) if !allow_failure => return Err(SessionsUnavailableReason::RuntimeApi(e)), - Err(canceled) if !allow_failure => - return Err(SessionsUnavailableReason::RuntimeApiUnavailable(canceled)), - Ok(Err(err)) => { - // Handle `allow_failure` true. - // If we didn't get the session, we advance window start. - *window_start += 1; - gum::debug!( - target: LOG_TARGET, - session = ?i, - ?err, - "Error while fetching session information." - ); - }, - Err(err) => { - // Handle `allow_failure` true. - // If we didn't get the session, we advance window start. - *window_start += 1; - gum::debug!( - target: LOG_TARGET, - session = ?i, - ?err, - "Channel error while fetching session information." - ); - }, - }; - } - - Ok(sessions) -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::database::kvdb_impl::DbAdapter; - use assert_matches::assert_matches; - use polkadot_node_subsystem::{ - messages::{AllMessages, AvailabilityRecoveryMessage}, - SubsystemContext, - }; - use polkadot_node_subsystem_test_helpers::make_subsystem_context; - use polkadot_primitives::Header; - use sp_core::testing::TaskExecutor; - - const SESSION_DATA_COL: u32 = 0; - - const NUM_COLUMNS: u32 = 1; - - fn dummy_db_params() -> DatabaseParams { - let db = kvdb_memorydb::create(NUM_COLUMNS); - let db = DbAdapter::new(db, &[]); - let db: Arc = Arc::new(db); - DatabaseParams { db, db_column: SESSION_DATA_COL } - } - - fn dummy_session_info(index: SessionIndex) -> SessionInfo { - SessionInfo { - validators: Default::default(), - discovery_keys: Vec::new(), - assignment_keys: Vec::new(), - validator_groups: Default::default(), - n_cores: index as _, - zeroth_delay_tranche_width: index as _, - relay_vrf_modulo_samples: index as _, - n_delay_tranches: index as _, - no_show_slots: index as _, - needed_approvals: index as _, - active_validator_indices: Vec::new(), - dispute_period: 6, - random_seed: [0u8; 32], - } - } - - fn cache_session_info_test( - expected_start_session: SessionIndex, - session: SessionIndex, - window: Option, - expect_requests_from: SessionIndex, - db_params: Option, - ) -> RollingSessionWindow { - let db_params = db_params.unwrap_or(dummy_db_params()); - - let header = Header { - digest: Default::default(), - extrinsics_root: Default::default(), - number: 5, - state_root: Default::default(), - parent_hash: Default::default(), - }; - - let finalized_header = Header { - digest: Default::default(), - extrinsics_root: Default::default(), - number: 0, - state_root: Default::default(), - parent_hash: Default::default(), - }; - - let pool = TaskExecutor::new(); - let (mut ctx, mut handle) = - make_subsystem_context::(pool.clone()); - - let hash = header.hash(); - - let sender = ctx.sender(); - - let test_fut = { - Box::pin(async move { - let window = match window { - None => - RollingSessionWindow::new(sender.clone(), hash, db_params).await.unwrap(), - Some(mut window) => { - window.cache_session_info_for_head(sender, hash).await.unwrap(); - window - }, - }; - assert_eq!(window.earliest_session, expected_start_session); - assert_eq!( - window.session_info, - (expected_start_session..=session).map(dummy_session_info).collect::>(), - ); - - window - }) - }; - - let aux_fut = Box::pin(async move { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, hash); - let _ = s_tx.send(Ok(session)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockNumber( - s_tx, - )) => { - let _ = s_tx.send(Ok(finalized_header.number)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockHash( - block_number, - s_tx, - )) => { - assert_eq!(block_number, finalized_header.number); - let _ = s_tx.send(Ok(Some(finalized_header.hash()))); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, finalized_header.hash()); - let _ = s_tx.send(Ok(session)); - } - ); - - for i in expect_requests_from..=session { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionInfo(j, s_tx), - )) => { - assert_eq!(h, hash); - assert_eq!(i, j); - let _ = s_tx.send(Ok(Some(dummy_session_info(i)))); - } - ); - } - }); - - let (window, _) = futures::executor::block_on(futures::future::join(test_fut, aux_fut)); - window - } - - #[test] - fn cache_session_info_start_empty_db() { - let db_params = dummy_db_params(); - - let window = cache_session_info_test( - (10 as SessionIndex).saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - 10, - None, - (10 as SessionIndex).saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - Some(db_params.clone()), - ); - - let window = cache_session_info_test( - (11 as SessionIndex).saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - 11, - Some(window), - 11, - None, - ); - assert_eq!(window.session_info.len(), SESSION_WINDOW_SIZE.get() as usize); - - cache_session_info_test( - (11 as SessionIndex).saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - 12, - None, - 12, - Some(db_params), - ); - } - - #[test] - fn cache_session_info_first_early() { - cache_session_info_test(0, 1, None, 0, None); - } - - #[test] - fn cache_session_info_does_not_underflow() { - let window = RollingSessionWindow { - earliest_session: 1, - session_info: vec![dummy_session_info(1)], - window_size: SESSION_WINDOW_SIZE, - db_params: Some(dummy_db_params()), - }; - - cache_session_info_test(1, 2, Some(window), 2, None); - } - - #[test] - fn cache_session_window_contains() { - let window = RollingSessionWindow { - earliest_session: 10, - session_info: vec![dummy_session_info(1)], - window_size: SESSION_WINDOW_SIZE, - db_params: Some(dummy_db_params()), - }; - - assert!(!window.contains(0)); - assert!(!window.contains(10 + SESSION_WINDOW_SIZE.get())); - assert!(!window.contains(11)); - assert!(!window.contains(10 + SESSION_WINDOW_SIZE.get() - 1)); - } - - #[test] - fn cache_session_info_first_late() { - cache_session_info_test( - (100 as SessionIndex).saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - 100, - None, - (100 as SessionIndex).saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - None, - ); - } - - #[test] - fn cache_session_info_jump() { - let window = RollingSessionWindow { - earliest_session: 50, - session_info: vec![ - dummy_session_info(50), - dummy_session_info(51), - dummy_session_info(52), - ], - window_size: SESSION_WINDOW_SIZE, - db_params: Some(dummy_db_params()), - }; - - cache_session_info_test( - (100 as SessionIndex).saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - 100, - Some(window), - (100 as SessionIndex).saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - None, - ); - } - - #[test] - fn cache_session_info_roll_full() { - let start = 99 - (SESSION_WINDOW_SIZE.get() - 1); - let window = RollingSessionWindow { - earliest_session: start, - session_info: (start..=99).map(dummy_session_info).collect(), - window_size: SESSION_WINDOW_SIZE, - db_params: Some(dummy_db_params()), - }; - - cache_session_info_test( - (100 as SessionIndex).saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - 100, - Some(window), - 100, // should only make one request. - None, - ); - } - - #[test] - fn cache_session_info_roll_many_full_db() { - let db_params = dummy_db_params(); - let start = 97 - (SESSION_WINDOW_SIZE.get() - 1); - let window = RollingSessionWindow { - earliest_session: start, - session_info: (start..=97).map(dummy_session_info).collect(), - window_size: SESSION_WINDOW_SIZE, - db_params: Some(db_params.clone()), - }; - - cache_session_info_test( - (100 as SessionIndex).saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - 100, - Some(window), - 98, - None, - ); - - // We expect the session to be populated from DB, and only fetch 101 from on chain. - cache_session_info_test( - (100 as SessionIndex).saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - 101, - None, - 101, - Some(db_params.clone()), - ); - - // Session warps in the future. - let window = cache_session_info_test(195, 200, None, 195, Some(db_params)); - - assert_eq!(window.session_info.len(), SESSION_WINDOW_SIZE.get() as usize); - } - - #[test] - fn cache_session_info_roll_many_full() { - let start = 97 - (SESSION_WINDOW_SIZE.get() - 1); - let window = RollingSessionWindow { - earliest_session: start, - session_info: (start..=97).map(dummy_session_info).collect(), - window_size: SESSION_WINDOW_SIZE, - db_params: Some(dummy_db_params()), - }; - - cache_session_info_test( - (100 as SessionIndex).saturating_sub(SESSION_WINDOW_SIZE.get() - 1), - 100, - Some(window), - 98, - None, - ); - } - - #[test] - fn cache_session_info_roll_early() { - let start = 0; - let window = RollingSessionWindow { - earliest_session: start, - session_info: (0..=1).map(dummy_session_info).collect(), - window_size: SESSION_WINDOW_SIZE, - db_params: Some(dummy_db_params()), - }; - - cache_session_info_test( - 0, - 2, - Some(window), - 2, // should only make one request. - None, - ); - } - - #[test] - fn cache_session_info_roll_many_early() { - let start = 0; - let window = RollingSessionWindow { - earliest_session: start, - session_info: (0..=1).map(dummy_session_info).collect(), - window_size: SESSION_WINDOW_SIZE, - db_params: Some(dummy_db_params()), - }; - - let actual_window_size = window.session_info.len() as u32; - - cache_session_info_test(0, 3, Some(window), actual_window_size, None); - } - - #[test] - fn db_load_works() { - // Session index of the tip of our fake test chain. - let session: SessionIndex = 100; - let genesis_session: SessionIndex = 0; - - let header = Header { - digest: Default::default(), - extrinsics_root: Default::default(), - number: 5, - state_root: Default::default(), - parent_hash: Default::default(), - }; - - let finalized_header = Header { - digest: Default::default(), - extrinsics_root: Default::default(), - number: 0, - state_root: Default::default(), - parent_hash: Default::default(), - }; - - let finalized_header_clone = finalized_header.clone(); - - let hash: sp_core::H256 = header.hash(); - let db_params = dummy_db_params(); - let db_params_clone = db_params.clone(); - - let pool = TaskExecutor::new(); - let (mut ctx, mut handle) = make_subsystem_context::<(), _>(pool.clone()); - - let test_fut = { - let sender = ctx.sender().clone(); - Box::pin(async move { - let mut rsw = - RollingSessionWindow::new(sender.clone(), hash, db_params_clone).await.unwrap(); - - let session_info = rsw.session_info.clone(); - let earliest_session = rsw.earliest_session(); - - assert_eq!(earliest_session, 0); - assert_eq!(session_info.len(), 101); - - rsw.db_save(StoredWindow { earliest_session, session_info }); - }) - }; - - let aux_fut = Box::pin(async move { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, hash); - let _ = s_tx.send(Ok(session)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockNumber( - s_tx, - )) => { - let _ = s_tx.send(Ok(finalized_header.number)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockHash( - block_number, - s_tx, - )) => { - assert_eq!(block_number, finalized_header.number); - let _ = s_tx.send(Ok(Some(finalized_header.hash()))); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, finalized_header.hash()); - let _ = s_tx.send(Ok(0)); - } - ); - - // Unfinalized chain starts at geneisis block, so session 0 is how far we stretch. - for i in genesis_session..=session { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionInfo(j, s_tx), - )) => { - assert_eq!(h, hash); - assert_eq!(i, j); - let _ = s_tx.send(Ok(Some(dummy_session_info(i)))); - } - ); - } - }); - - futures::executor::block_on(futures::future::join(test_fut, aux_fut)); - - let pool = TaskExecutor::new(); - let (mut ctx, mut handle) = make_subsystem_context::<(), _>(pool.clone()); - - let test_fut = { - Box::pin(async move { - let sender = ctx.sender().clone(); - let res = RollingSessionWindow::new(sender, hash, db_params).await; - let rsw = res.unwrap(); - assert_eq!(rsw.earliest_session, 0); - assert_eq!(rsw.session_info.len(), 101); - }) - }; - - let aux_fut = Box::pin(async move { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, hash); - let _ = s_tx.send(Ok(session)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockNumber( - s_tx, - )) => { - let _ = s_tx.send(Ok(finalized_header_clone.number)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockHash( - block_number, - s_tx, - )) => { - assert_eq!(block_number, finalized_header_clone.number); - let _ = s_tx.send(Ok(Some(finalized_header_clone.hash()))); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, finalized_header_clone.hash()); - let _ = s_tx.send(Ok(0)); - } - ); - }); - - futures::executor::block_on(futures::future::join(test_fut, aux_fut)); - } - - #[test] - fn cache_session_fails_for_gap_in_window() { - // Session index of the tip of our fake test chain. - let session: SessionIndex = 100; - let genesis_session: SessionIndex = 0; - - let header = Header { - digest: Default::default(), - extrinsics_root: Default::default(), - number: 5, - state_root: Default::default(), - parent_hash: Default::default(), - }; - - let finalized_header = Header { - digest: Default::default(), - extrinsics_root: Default::default(), - number: 0, - state_root: Default::default(), - parent_hash: Default::default(), - }; - - let pool = TaskExecutor::new(); - let (mut ctx, mut handle) = make_subsystem_context::<(), _>(pool.clone()); - - let hash = header.hash(); - - let test_fut = { - let sender = ctx.sender().clone(); - Box::pin(async move { - let res = RollingSessionWindow::new(sender, hash, dummy_db_params()).await; - - assert!(res.is_err()); - }) - }; - - let aux_fut = Box::pin(async move { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, hash); - let _ = s_tx.send(Ok(session)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockNumber( - s_tx, - )) => { - let _ = s_tx.send(Ok(finalized_header.number)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockHash( - block_number, - s_tx, - )) => { - assert_eq!(block_number, finalized_header.number); - let _ = s_tx.send(Ok(Some(finalized_header.hash()))); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, finalized_header.hash()); - let _ = s_tx.send(Ok(0)); - } - ); - - // Unfinalized chain starts at geneisis block, so session 0 is how far we stretch. - // First 50 sessions are missing. - for i in genesis_session..=50 { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionInfo(j, s_tx), - )) => { - assert_eq!(h, hash); - assert_eq!(i, j); - let _ = s_tx.send(Ok(None)); - } - ); - } - // next 10 sessions are present - for i in 51..=60 { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionInfo(j, s_tx), - )) => { - assert_eq!(h, hash); - assert_eq!(i, j); - let _ = s_tx.send(Ok(Some(dummy_session_info(i)))); - } - ); - } - // gap of 1 session - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionInfo(j, s_tx), - )) => { - assert_eq!(h, hash); - assert_eq!(61, j); - let _ = s_tx.send(Ok(None)); - } - ); - }); - - futures::executor::block_on(futures::future::join(test_fut, aux_fut)); - } - - #[test] - fn any_session_stretch_with_failure_allowed_for_unfinalized_chain() { - // Session index of the tip of our fake test chain. - let session: SessionIndex = 100; - let genesis_session: SessionIndex = 0; - - let header = Header { - digest: Default::default(), - extrinsics_root: Default::default(), - number: 5, - state_root: Default::default(), - parent_hash: Default::default(), - }; - - let finalized_header = Header { - digest: Default::default(), - extrinsics_root: Default::default(), - number: 0, - state_root: Default::default(), - parent_hash: Default::default(), - }; - - let pool = TaskExecutor::new(); - let (mut ctx, mut handle) = make_subsystem_context::<(), _>(pool.clone()); - - let hash = header.hash(); - - let test_fut = { - let sender = ctx.sender().clone(); - Box::pin(async move { - let res = RollingSessionWindow::new(sender, hash, dummy_db_params()).await; - assert!(res.is_ok()); - let rsw = res.unwrap(); - // Since first 50 sessions are missing the earliest should be 50. - assert_eq!(rsw.earliest_session, 50); - assert_eq!(rsw.session_info.len(), 51); - }) - }; - - let aux_fut = Box::pin(async move { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, hash); - let _ = s_tx.send(Ok(session)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockNumber( - s_tx, - )) => { - let _ = s_tx.send(Ok(finalized_header.number)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockHash( - block_number, - s_tx, - )) => { - assert_eq!(block_number, finalized_header.number); - let _ = s_tx.send(Ok(Some(finalized_header.hash()))); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, finalized_header.hash()); - let _ = s_tx.send(Ok(0)); - } - ); - - // Unfinalized chain starts at geneisis block, so session 0 is how far we stretch. - // We also test if failure is allowed for 50 first missing sessions. - for i in genesis_session..=session { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionInfo(j, s_tx), - )) => { - assert_eq!(h, hash); - assert_eq!(i, j); - - let _ = s_tx.send(Ok(if i < 50 { - None - } else { - Some(dummy_session_info(i)) - })); - } - ); - } - }); - - futures::executor::block_on(futures::future::join(test_fut, aux_fut)); - } - - #[test] - fn any_session_unavailable_for_caching_means_no_change() { - let session: SessionIndex = 6; - let start_session = session.saturating_sub(SESSION_WINDOW_SIZE.get() - 1); - - let header = Header { - digest: Default::default(), - extrinsics_root: Default::default(), - number: 5, - state_root: Default::default(), - parent_hash: Default::default(), - }; - - let finalized_header = Header { - digest: Default::default(), - extrinsics_root: Default::default(), - number: 0, - state_root: Default::default(), - parent_hash: Default::default(), - }; - - let pool = TaskExecutor::new(); - let (mut ctx, mut handle) = make_subsystem_context::<(), _>(pool.clone()); - - let hash = header.hash(); - - let test_fut = { - let sender = ctx.sender().clone(); - Box::pin(async move { - let res = RollingSessionWindow::new(sender, hash, dummy_db_params()).await; - assert!(res.is_err()); - }) - }; - - let aux_fut = Box::pin(async move { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, hash); - let _ = s_tx.send(Ok(session)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockNumber( - s_tx, - )) => { - let _ = s_tx.send(Ok(finalized_header.number)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockHash( - block_number, - s_tx, - )) => { - assert_eq!(block_number, finalized_header.number); - let _ = s_tx.send(Ok(Some(finalized_header.hash()))); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, finalized_header.hash()); - let _ = s_tx.send(Ok(session)); - } - ); - - for i in start_session..=session { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionInfo(j, s_tx), - )) => { - assert_eq!(h, hash); - assert_eq!(i, j); - - let _ = s_tx.send(Ok(if i == session { - None - } else { - Some(dummy_session_info(i)) - })); - } - ); - } - }); - - futures::executor::block_on(futures::future::join(test_fut, aux_fut)); - } - - #[test] - fn request_session_info_for_genesis() { - let session: SessionIndex = 0; - - let header = Header { - digest: Default::default(), - extrinsics_root: Default::default(), - number: 0, - state_root: Default::default(), - parent_hash: Default::default(), - }; - - let pool = TaskExecutor::new(); - let (mut ctx, mut handle) = make_subsystem_context::<(), _>(pool.clone()); - - let hash = header.hash(); - - let test_fut = { - Box::pin(async move { - let sender = ctx.sender().clone(); - let window = - RollingSessionWindow::new(sender, hash, dummy_db_params()).await.unwrap(); - - assert_eq!(window.earliest_session, session); - assert_eq!(window.session_info, vec![dummy_session_info(session)]); - }) - }; - - let aux_fut = Box::pin(async move { - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, hash); - let _ = s_tx.send(Ok(session)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockNumber( - s_tx, - )) => { - let _ = s_tx.send(Ok(header.number)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::ChainApi(ChainApiMessage::FinalizedBlockHash( - block_number, - s_tx, - )) => { - assert_eq!(block_number, header.number); - let _ = s_tx.send(Ok(Some(header.hash()))); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionIndexForChild(s_tx), - )) => { - assert_eq!(h, header.hash()); - let _ = s_tx.send(Ok(session)); - } - ); - - assert_matches!( - handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - h, - RuntimeApiRequest::SessionInfo(s, s_tx), - )) => { - assert_eq!(h, hash); - assert_eq!(s, session); - - let _ = s_tx.send(Ok(Some(dummy_session_info(s)))); - } - ); - }); - - futures::executor::block_on(futures::future::join(test_fut, aux_fut)); - } -} From 7d83f95e84533a42f80751d37b1e7fb7cf83f59b Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 12 May 2023 18:05:33 +0300 Subject: [PATCH 33/42] Revert "Fix formatting in initialized.rs" This reverts commit 0f94664ec9f3a7e3737a30291195990e1e7065fc. --- .../dispute-coordinator/src/initialized.rs | 101 +++++++++--------- node/network/approval-distribution/src/lib.rs | 10 +- 2 files changed, 56 insertions(+), 55 deletions(-) diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index df301c5fad04..f2956938f562 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -214,61 +214,62 @@ impl Initialized { gum::trace!(target: LOG_TARGET, "Waiting for message"); let mut overlay_db = OverlayedBackend::new(backend); let default_confirm = Box::new(|| Ok(())); - let confirm_write = - match MuxedMessage::receive(ctx, &mut self.participation_receiver).await? { - MuxedMessage::Participation(msg) => { - gum::trace!(target: LOG_TARGET, "MuxedMessage::Participation"); - let ParticipationStatement { - session, + let confirm_write = match MuxedMessage::receive(ctx, &mut self.participation_receiver) + .await? + { + MuxedMessage::Participation(msg) => { + gum::trace!(target: LOG_TARGET, "MuxedMessage::Participation"); + let ParticipationStatement { + session, + candidate_hash, + candidate_receipt, + outcome, + } = self.participation.get_participation_result(ctx, msg).await?; + if let Some(valid) = outcome.validity() { + gum::trace!( + target: LOG_TARGET, + ?session, + ?candidate_hash, + ?valid, + "Issuing local statement based on participation outcome." + ); + self.issue_local_statement( + ctx, + &mut overlay_db, candidate_hash, candidate_receipt, - outcome, - } = self.participation.get_participation_result(ctx, msg).await?; - if let Some(valid) = outcome.validity() { - gum::trace!( - target: LOG_TARGET, - ?session, - ?candidate_hash, - ?valid, - "Issuing local statement based on participation outcome." - ); - self.issue_local_statement( - ctx, - &mut overlay_db, - candidate_hash, - candidate_receipt, - session, - valid, - clock.now(), - ) - .await?; - } else { - gum::warn!(target: LOG_TARGET, ?outcome, "Dispute participation failed"); - } + session, + valid, + clock.now(), + ) + .await?; + } else { + gum::warn!(target: LOG_TARGET, ?outcome, "Dispute participation failed"); + } + default_confirm + }, + MuxedMessage::Subsystem(msg) => match msg { + FromOrchestra::Signal(OverseerSignal::Conclude) => return Ok(()), + FromOrchestra::Signal(OverseerSignal::ActiveLeaves(update)) => { + gum::trace!(target: LOG_TARGET, "OverseerSignal::ActiveLeaves"); + self.process_active_leaves_update( + ctx, + &mut overlay_db, + update, + clock.now(), + ) + .await?; default_confirm }, - MuxedMessage::Subsystem(msg) => match msg { - FromOrchestra::Signal(OverseerSignal::Conclude) => return Ok(()), - FromOrchestra::Signal(OverseerSignal::ActiveLeaves(update)) => { - gum::trace!(target: LOG_TARGET, "OverseerSignal::ActiveLeaves"); - self.process_active_leaves_update( - ctx, - &mut overlay_db, - update, - clock.now(), - ) - .await?; - default_confirm - }, - FromOrchestra::Signal(OverseerSignal::BlockFinalized(_, n)) => { - gum::trace!(target: LOG_TARGET, "OverseerSignal::BlockFinalized"); - self.scraper.process_finalized_block(&n); - default_confirm - }, - FromOrchestra::Communication { msg } => - self.handle_incoming(ctx, &mut overlay_db, msg, clock.now()).await?, + FromOrchestra::Signal(OverseerSignal::BlockFinalized(_, n)) => { + gum::trace!(target: LOG_TARGET, "OverseerSignal::BlockFinalized"); + self.scraper.process_finalized_block(&n); + default_confirm }, - }; + FromOrchestra::Communication { msg } => + self.handle_incoming(ctx, &mut overlay_db, msg, clock.now()).await?, + }, + }; if !overlay_db.is_empty() { let ops = overlay_db.into_write_ops(); diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index 82f62502e474..c8ad21ad406e 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -1270,11 +1270,11 @@ impl State { let candidate_entry = match block_entry.candidates.get(index as usize) { None => { gum::debug!( - target: LOG_TARGET, - ?hash, - ?index, - "`get_approval_signatures`: could not find candidate entry for given hash and index!" - ); + target: LOG_TARGET, + ?hash, + ?index, + "`get_approval_signatures`: could not find candidate entry for given hash and index!" + ); continue }, Some(e) => e, From 39ae1910287c2de0fde8b18ac04a1a4623d77737 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 16 May 2023 11:48:04 +0300 Subject: [PATCH 34/42] v2 to v3 migration drops `COL_DISPUTE_COORDINATOR_DATA` instead of clearing it --- node/service/src/parachains_db/mod.rs | 4 ++-- node/service/src/parachains_db/upgrade.rs | 23 ++++------------------- 2 files changed, 6 insertions(+), 21 deletions(-) diff --git a/node/service/src/parachains_db/mod.rs b/node/service/src/parachains_db/mod.rs index e3cefee40a88..f3b4ba7ef10d 100644 --- a/node/service/src/parachains_db/mod.rs +++ b/node/service/src/parachains_db/mod.rs @@ -41,6 +41,7 @@ pub(crate) mod columns { // pub const COL_APPROVAL_DATA: u32 = 2; pub const COL_CHAIN_SELECTION_DATA: u32 = 3; pub const COL_DISPUTE_COORDINATOR_DATA: u32 = 4; + #[cfg(test)] pub const COL_SESSION_WINDOW_DATA: u32 = 5; pub const ORDERED_COL: &[u32] = @@ -48,13 +49,12 @@ pub(crate) mod columns { } pub mod v3 { - pub const NUM_COLUMNS: u32 = 6; + pub const NUM_COLUMNS: u32 = 5; pub const COL_AVAILABILITY_DATA: u32 = 0; pub const COL_AVAILABILITY_META: u32 = 1; pub const COL_APPROVAL_DATA: u32 = 2; pub const COL_CHAIN_SELECTION_DATA: u32 = 3; pub const COL_DISPUTE_COORDINATOR_DATA: u32 = 4; - // pub const COL_SESSION_WINDOW_DATA: u32 = 5; // not used pub const ORDERED_COL: &[u32] = &[COL_AVAILABILITY_META, COL_CHAIN_SELECTION_DATA, COL_DISPUTE_COORDINATOR_DATA]; diff --git a/node/service/src/parachains_db/upgrade.rs b/node/service/src/parachains_db/upgrade.rs index e34fac133a37..86bf93de251e 100644 --- a/node/service/src/parachains_db/upgrade.rs +++ b/node/service/src/parachains_db/upgrade.rs @@ -175,22 +175,15 @@ fn rocksdb_migrate_from_version_1_to_2(path: &Path) -> Result<(), Error> { } fn rocksdb_migrate_from_version_2_to_3(path: &Path) -> Result<(), Error> { - use kvdb::{DBOp, DBTransaction}; use kvdb_rocksdb::{Database, DatabaseConfig}; let db_path = path .to_str() .ok_or_else(|| super::other_io_error("Invalid database path".into()))?; let db_cfg = DatabaseConfig::with_columns(super::columns::v2::NUM_COLUMNS); - let db = Database::open(&db_cfg, db_path)?; - - // Wipe all entries in one operation. - let ops = vec![DBOp::DeletePrefix { - col: super::columns::v2::COL_SESSION_WINDOW_DATA, - prefix: kvdb::DBKey::from_slice(b""), - }]; + let mut db = Database::open(&db_cfg, db_path)?; - db.write(DBTransaction { ops })?; + db.remove_last_column()?; Ok(()) } @@ -264,7 +257,6 @@ pub(crate) fn paritydb_version_1_config(path: &Path) -> parity_db::Options { } /// Database configuration for version 2. -#[cfg(test)] pub(crate) fn paritydb_version_2_config(path: &Path) -> parity_db::Options { let mut options = parity_db::Options::with_columns(&path, super::columns::v2::NUM_COLUMNS as u8); @@ -328,9 +320,8 @@ fn paritydb_migrate_from_version_1_to_2(path: &Path) -> Result<(), Error> { /// Migration from version 2 to version 3: /// - clear any columns used by `RollingSessionWindow` fn paritydb_migrate_from_version_2_to_3(path: &Path) -> Result<(), Error> { - parity_db::clear_column(path, super::columns::v2::COL_SESSION_WINDOW_DATA as u8) - .map_err(|e| other_io_error(format!("Error clearing COL_SESSION_WINDOW_DATA {:?}", e)))?; - + parity_db::Db::drop_last_column(&mut paritydb_version_2_config(path)) + .map_err(|e| other_io_error(format!("Error removing COL_SESSION_WINDOW_DATA {:?}", e)))?; Ok(()) } @@ -503,9 +494,6 @@ mod tests { let db = Db::open(&paritydb_version_3_config(&path)).unwrap(); assert_eq!(db.num_columns(), columns::v3::NUM_COLUMNS as u8); - - // ensure column is empty - assert!(db.get(COL_SESSION_WINDOW_DATA as u8, &test_key.to_vec()).unwrap().is_none()); } #[test] @@ -542,8 +530,5 @@ mod tests { let db = Database::open(&db_cfg, db_path).unwrap(); assert_eq!(db.num_columns(), super::columns::v3::NUM_COLUMNS); - - // Ensure data is actually removed - assert!(db.get(COL_SESSION_WINDOW_DATA, b"1337").unwrap().is_none()); } } From 84c134aaea71c273b933e0e8315588c805ca8c10 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 16 May 2023 17:20:20 +0300 Subject: [PATCH 35/42] Fix `NUM_COLUMNS` in `approval-voting` --- node/core/approval-voting/src/approval_db/v1/tests.rs | 2 +- node/core/approval-voting/src/tests.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/node/core/approval-voting/src/approval_db/v1/tests.rs b/node/core/approval-voting/src/approval_db/v1/tests.rs index e4bab5b69464..07d8242b772e 100644 --- a/node/core/approval-voting/src/approval_db/v1/tests.rs +++ b/node/core/approval-voting/src/approval_db/v1/tests.rs @@ -29,7 +29,7 @@ use ::test_helpers::{dummy_candidate_receipt, dummy_candidate_receipt_bad_sig, d const DATA_COL: u32 = 0; -const NUM_COLUMNS: u32 = 2; +const NUM_COLUMNS: u32 = 1; const TEST_CONFIG: Config = Config { col_approval_data: DATA_COL }; diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index c75aded022cb..f58e60c6a487 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -114,7 +114,7 @@ pub mod test_constants { use crate::approval_db::v1::Config as DatabaseConfig; const DATA_COL: u32 = 0; - pub(crate) const NUM_COLUMNS: u32 = 2; + pub(crate) const NUM_COLUMNS: u32 = 1; pub(crate) const TEST_CONFIG: DatabaseConfig = DatabaseConfig { col_approval_data: DATA_COL }; } From c7439c587c37b57d788ba2d821d402bbc57dba32 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 19 May 2023 00:25:15 +0300 Subject: [PATCH 36/42] Use `columns::v3::NUM_COLUMNS` when opening db --- node/service/src/parachains_db/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/service/src/parachains_db/mod.rs b/node/service/src/parachains_db/mod.rs index f3b4ba7ef10d..a4c3422dfe28 100644 --- a/node/service/src/parachains_db/mod.rs +++ b/node/service/src/parachains_db/mod.rs @@ -132,7 +132,7 @@ pub fn open_creating_rocksdb( let path = root.join("parachains").join("db"); - let mut db_config = DatabaseConfig::with_columns(columns::v2::NUM_COLUMNS); + let mut db_config = DatabaseConfig::with_columns(columns::v3::NUM_COLUMNS); let _ = db_config .memory_budget From 2f6b33b6f719e1c7c085897a097cef07f075e3e8 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 May 2023 13:35:00 +0300 Subject: [PATCH 37/42] Update node/service/src/parachains_db/upgrade.rs Co-authored-by: Andrei Sandu <54316454+sandreim@users.noreply.github.com> --- node/service/src/parachains_db/upgrade.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/service/src/parachains_db/upgrade.rs b/node/service/src/parachains_db/upgrade.rs index 86bf93de251e..ad4c4eebd595 100644 --- a/node/service/src/parachains_db/upgrade.rs +++ b/node/service/src/parachains_db/upgrade.rs @@ -318,7 +318,7 @@ fn paritydb_migrate_from_version_1_to_2(path: &Path) -> Result<(), Error> { } /// Migration from version 2 to version 3: -/// - clear any columns used by `RollingSessionWindow` +/// - drop the column used by `RollingSessionWindow` fn paritydb_migrate_from_version_2_to_3(path: &Path) -> Result<(), Error> { parity_db::Db::drop_last_column(&mut paritydb_version_2_config(path)) .map_err(|e| other_io_error(format!("Error removing COL_SESSION_WINDOW_DATA {:?}", e)))?; From 0ccfe2cc985fece4be2c40d1a06bd5d538d380f7 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 May 2023 14:00:14 +0300 Subject: [PATCH 38/42] Don't write in `COL_DISPUTE_COORDINATOR_DATA` for `test_rocksdb_migrate_2_to_3` --- node/service/src/parachains_db/upgrade.rs | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/node/service/src/parachains_db/upgrade.rs b/node/service/src/parachains_db/upgrade.rs index ad4c4eebd595..3c1c39e01076 100644 --- a/node/service/src/parachains_db/upgrade.rs +++ b/node/service/src/parachains_db/upgrade.rs @@ -498,31 +498,18 @@ mod tests { #[test] fn test_rocksdb_migrate_2_to_3() { - use kvdb::{DBKey, DBOp}; use kvdb_rocksdb::{Database, DatabaseConfig}; - use polkadot_node_subsystem_util::database::{ - kvdb_impl::DbAdapter, DBTransaction, KeyValueDB, - }; let db_dir = tempfile::tempdir().unwrap(); let db_path = db_dir.path().to_str().unwrap(); let db_cfg = DatabaseConfig::with_columns(super::columns::v2::NUM_COLUMNS); - let db = Database::open(&db_cfg, db_path).unwrap(); - assert_eq!(db.num_columns(), super::columns::v2::NUM_COLUMNS as u32); + { + let db = Database::open(&db_cfg, db_path).unwrap(); + assert_eq!(db.num_columns(), super::columns::v2::NUM_COLUMNS as u32); + } // We need to properly set db version for upgrade to work. fs::write(version_file_path(db_dir.path()), "2").expect("Failed to write DB version"); - { - let db = DbAdapter::new(db, columns::v2::ORDERED_COL); - db.write(DBTransaction { - ops: vec![DBOp::Insert { - col: COL_DISPUTE_COORDINATOR_DATA, - key: DBKey::from_slice(b"1234"), - value: b"0xdeadb00b".to_vec(), - }], - }) - .unwrap(); - } try_upgrade_db(&db_dir.path(), DatabaseKind::RocksDB).unwrap(); From ab7197df2c1d6a2786e9af06692d746b807a08c8 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 25 May 2023 14:00:44 +0300 Subject: [PATCH 39/42] Fix `NUM+COLUMNS` in approval_voting --- node/core/approval-voting/src/import.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/approval-voting/src/import.rs b/node/core/approval-voting/src/import.rs index bd9dd03a4f2a..e33caed49c5f 100644 --- a/node/core/approval-voting/src/import.rs +++ b/node/core/approval-voting/src/import.rs @@ -610,7 +610,7 @@ pub(crate) mod tests { const DATA_COL: u32 = 0; - const NUM_COLUMNS: u32 = 2; + const NUM_COLUMNS: u32 = 1; const TEST_CONFIG: DatabaseConfig = DatabaseConfig { col_approval_data: DATA_COL }; #[derive(Default)] From 44fe75a4d0a2805460a70fa9282fcd4b20e0bfff Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 29 May 2023 11:31:08 +0300 Subject: [PATCH 40/42] Fix formatting --- node/service/src/parachains_db/upgrade.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/service/src/parachains_db/upgrade.rs b/node/service/src/parachains_db/upgrade.rs index 3c1c39e01076..d508f94e3f57 100644 --- a/node/service/src/parachains_db/upgrade.rs +++ b/node/service/src/parachains_db/upgrade.rs @@ -318,7 +318,7 @@ fn paritydb_migrate_from_version_1_to_2(path: &Path) -> Result<(), Error> { } /// Migration from version 2 to version 3: -/// - drop the column used by `RollingSessionWindow` +/// - drop the column used by `RollingSessionWindow` fn paritydb_migrate_from_version_2_to_3(path: &Path) -> Result<(), Error> { parity_db::Db::drop_last_column(&mut paritydb_version_2_config(path)) .map_err(|e| other_io_error(format!("Error removing COL_SESSION_WINDOW_DATA {:?}", e)))?; From bbbd5ee973a5ef633e27d41a2c17c6f0207ea758 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 29 May 2023 14:14:17 +0300 Subject: [PATCH 41/42] Fix columns usage --- node/service/src/parachains_db/mod.rs | 9 +-------- node/service/src/parachains_db/upgrade.rs | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/node/service/src/parachains_db/mod.rs b/node/service/src/parachains_db/mod.rs index a4c3422dfe28..519afbe0ccd1 100644 --- a/node/service/src/parachains_db/mod.rs +++ b/node/service/src/parachains_db/mod.rs @@ -36,16 +36,9 @@ pub(crate) mod columns { pub mod v2 { pub const NUM_COLUMNS: u32 = 6; - // pub const COL_AVAILABILITY_DATA: u32 = 0; - pub const COL_AVAILABILITY_META: u32 = 1; - // pub const COL_APPROVAL_DATA: u32 = 2; - pub const COL_CHAIN_SELECTION_DATA: u32 = 3; - pub const COL_DISPUTE_COORDINATOR_DATA: u32 = 4; + #[cfg(test)] pub const COL_SESSION_WINDOW_DATA: u32 = 5; - - pub const ORDERED_COL: &[u32] = - &[COL_AVAILABILITY_META, COL_CHAIN_SELECTION_DATA, COL_DISPUTE_COORDINATOR_DATA]; } pub mod v3 { diff --git a/node/service/src/parachains_db/upgrade.rs b/node/service/src/parachains_db/upgrade.rs index d508f94e3f57..6041a093ef9b 100644 --- a/node/service/src/parachains_db/upgrade.rs +++ b/node/service/src/parachains_db/upgrade.rs @@ -249,7 +249,7 @@ fn paritydb_fix_columns( pub(crate) fn paritydb_version_1_config(path: &Path) -> parity_db::Options { let mut options = parity_db::Options::with_columns(&path, super::columns::v1::NUM_COLUMNS as u8); - for i in columns::v2::ORDERED_COL { + for i in columns::v3::ORDERED_COL { options.columns[*i as usize].btree_index = true; } @@ -260,7 +260,7 @@ pub(crate) fn paritydb_version_1_config(path: &Path) -> parity_db::Options { pub(crate) fn paritydb_version_2_config(path: &Path) -> parity_db::Options { let mut options = parity_db::Options::with_columns(&path, super::columns::v2::NUM_COLUMNS as u8); - for i in columns::v2::ORDERED_COL { + for i in columns::v3::ORDERED_COL { options.columns[*i as usize].btree_index = true; } @@ -283,8 +283,8 @@ pub(crate) fn paritydb_version_3_config(path: &Path) -> parity_db::Options { pub(crate) fn paritydb_version_0_config(path: &Path) -> parity_db::Options { let mut options = parity_db::Options::with_columns(&path, super::columns::v1::NUM_COLUMNS as u8); - options.columns[super::columns::v2::COL_AVAILABILITY_META as usize].btree_index = true; - options.columns[super::columns::v2::COL_CHAIN_SELECTION_DATA as usize].btree_index = true; + options.columns[super::columns::v3::COL_AVAILABILITY_META as usize].btree_index = true; + options.columns[super::columns::v3::COL_CHAIN_SELECTION_DATA as usize].btree_index = true; options } @@ -299,7 +299,7 @@ fn paritydb_migrate_from_version_0_to_1(path: &Path) -> Result<(), Error> { paritydb_fix_columns( path, paritydb_version_1_config(path), - vec![super::columns::v2::COL_DISPUTE_COORDINATOR_DATA], + vec![super::columns::v3::COL_DISPUTE_COORDINATOR_DATA], )?; Ok(()) @@ -327,7 +327,10 @@ fn paritydb_migrate_from_version_2_to_3(path: &Path) -> Result<(), Error> { #[cfg(test)] mod tests { - use super::{columns::v2::*, *}; + use super::{ + columns::{v2::COL_SESSION_WINDOW_DATA, v3::*}, + *, + }; #[test] fn test_paritydb_migrate_0_to_1() { @@ -422,7 +425,7 @@ mod tests { // We need to properly set db version for upgrade to work. fs::write(version_file_path(db_dir.path()), "1").expect("Failed to write DB version"); { - let db = DbAdapter::new(db, columns::v2::ORDERED_COL); + let db = DbAdapter::new(db, columns::v3::ORDERED_COL); db.write(DBTransaction { ops: vec![DBOp::Insert { col: COL_DISPUTE_COORDINATOR_DATA, @@ -440,7 +443,7 @@ mod tests { assert_eq!(db.num_columns(), super::columns::v2::NUM_COLUMNS); - let db = DbAdapter::new(db, columns::v2::ORDERED_COL); + let db = DbAdapter::new(db, columns::v3::ORDERED_COL); assert_eq!( db.get(COL_DISPUTE_COORDINATOR_DATA, b"1234").unwrap(), From 4b45afbad739945e72b13559fc39e7355c7474ed Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 29 May 2023 14:45:24 +0300 Subject: [PATCH 42/42] Clarification comments about the different db versions --- node/core/approval-voting/src/approval_db/v1/mod.rs | 6 ++++++ node/core/dispute-coordinator/src/db/v1.rs | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/node/core/approval-voting/src/approval_db/v1/mod.rs b/node/core/approval-voting/src/approval_db/v1/mod.rs index 2a839d623011..c31389269d2e 100644 --- a/node/core/approval-voting/src/approval_db/v1/mod.rs +++ b/node/core/approval-voting/src/approval_db/v1/mod.rs @@ -15,6 +15,12 @@ // along with Polkadot. If not, see . //! Version 1 of the DB schema. +//! +//! Note that the version here differs from the actual version of the parachains +//! database (check `CURRENT_VERSION` in `node/service/src/parachains_db/upgrade.rs`). +//! The code in this module implements the way approval voting works with +//! its data in the database. Any breaking changes here will still +//! require a db migration (check `node/service/src/parachains_db/upgrade.rs`). use parity_scale_codec::{Decode, Encode}; use polkadot_node_primitives::approval::{AssignmentCert, DelayTranche}; diff --git a/node/core/dispute-coordinator/src/db/v1.rs b/node/core/dispute-coordinator/src/db/v1.rs index 691497ef9e97..2d14f5151003 100644 --- a/node/core/dispute-coordinator/src/db/v1.rs +++ b/node/core/dispute-coordinator/src/db/v1.rs @@ -15,6 +15,12 @@ // along with Polkadot. If not, see . //! `V1` database for the dispute coordinator. +//! +//! Note that the version here differs from the actual version of the parachains +//! database (check `CURRENT_VERSION` in `node/service/src/parachains_db/upgrade.rs`). +//! The code in this module implements the way dispute coordinator works with +//! the dispute data in the database. Any breaking changes here will still +//! require a db migration (check `node/service/src/parachains_db/upgrade.rs`). use polkadot_node_primitives::DisputeStatus; use polkadot_node_subsystem_util::database::{DBTransaction, Database};