From 9ee996bf78ba66eb1c1ce8d5b186d8079b512476 Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Wed, 4 Sep 2024 22:36:18 +0800 Subject: [PATCH 1/3] Introduce `BlockGap` Previously, block gaps could only be created by warp sync, but with upcoming changes (#5406), block gaps will also be generated by fast sync, thus `BlockGapType` is needed. This refactor converts the existing `(NumberFor, NumberFor)` into a dedicated `BlockGap>` struct. This change is purely structural and does not alter existing logic, but it lays the groundwork for future changes. --- substrate/client/consensus/babe/src/lib.rs | 6 +- substrate/client/db/src/lib.rs | 61 +++++++++++-------- substrate/client/db/src/utils.rs | 5 +- .../network/sync/src/strategy/chain_sync.rs | 4 +- substrate/client/service/src/client/client.rs | 5 +- .../primitives/blockchain/src/backend.rs | 40 +++++++++--- 6 files changed, 79 insertions(+), 42 deletions(-) diff --git a/substrate/client/consensus/babe/src/lib.rs b/substrate/client/consensus/babe/src/lib.rs index 9770b16871e1..4cf66302ec85 100644 --- a/substrate/client/consensus/babe/src/lib.rs +++ b/substrate/client/consensus/babe/src/lib.rs @@ -1146,7 +1146,9 @@ where let info = self.client.info(); let number = *block.header.number(); - if info.block_gap.map_or(false, |(s, e)| s <= number && number <= e) || block.with_state() { + if info.block_gap.map_or(false, |gap| gap.start <= number && number <= gap.end) || + block.with_state() + { // Verification for imported blocks is skipped in two cases: // 1. When importing blocks below the last finalized block during network initial // synchronization. @@ -1420,7 +1422,7 @@ where // Skip babe logic if block already in chain or importing blocks during initial sync, // otherwise the check for epoch changes will error because trying to re-import an // epoch change or because of missing epoch data in the tree, respectively. - if info.block_gap.map_or(false, |(s, e)| s <= number && number <= e) || + if info.block_gap.map_or(false, |gap| gap.start <= number && number <= gap.end) || block_status == BlockStatus::InChain { // When re-importing existing block strip away intermediates. diff --git a/substrate/client/db/src/lib.rs b/substrate/client/db/src/lib.rs index eadb26254a18..16c225d1f00f 100644 --- a/substrate/client/db/src/lib.rs +++ b/substrate/client/db/src/lib.rs @@ -61,6 +61,7 @@ use codec::{Decode, Encode}; use hash_db::Prefix; use sc_client_api::{ backend::NewBlockState, + blockchain::{BlockGap, BlockGapType}, leaves::{FinalizationOutcome, LeafSet}, utils::is_descendent_of, IoInfo, MemoryInfo, MemorySize, UsageInfo, @@ -522,7 +523,7 @@ impl BlockchainDb { } } - fn update_block_gap(&self, gap: Option<(NumberFor, NumberFor)>) { + fn update_block_gap(&self, gap: Option>>) { let mut meta = self.meta.write(); meta.block_gap = gap; } @@ -1671,34 +1672,44 @@ impl Backend { ); } - if let Some((mut start, end)) = block_gap { - if number == start { - start += One::one(); - utils::insert_number_to_key_mapping( - &mut transaction, - columns::KEY_LOOKUP, - number, - hash, - )?; - if start > end { - transaction.remove(columns::META, meta_keys::BLOCK_GAP); - block_gap = None; - debug!(target: "db", "Removed block gap."); - } else { - block_gap = Some((start, end)); - debug!(target: "db", "Update block gap. {block_gap:?}"); - transaction.set( - columns::META, - meta_keys::BLOCK_GAP, - &(start, end).encode(), - ); - } - block_gap_updated = true; + if let Some(mut gap) = block_gap { + match gap.gap_type { + BlockGapType::MissingHeaderAndBody => + if number == gap.start { + gap.start += One::one(); + utils::insert_number_to_key_mapping( + &mut transaction, + columns::KEY_LOOKUP, + number, + hash, + )?; + if gap.start > gap.end { + transaction.remove(columns::META, meta_keys::BLOCK_GAP); + block_gap = None; + debug!(target: "db", "Removed block gap."); + } else { + block_gap = Some(gap); + debug!(target: "db", "Update block gap. {block_gap:?}"); + transaction.set( + columns::META, + meta_keys::BLOCK_GAP, + &gap.encode(), + ); + } + block_gap_updated = true; + }, + BlockGapType::MissingBody => { + unreachable!("Unsupported block gap. TODO: https://github.com/paritytech/polkadot-sdk/issues/5406") + }, } } else if number > best_num + One::one() && number > One::one() && self.blockchain.header(parent_hash)?.is_none() { - let gap = (best_num + One::one(), number - One::one()); + let gap = BlockGap { + start: best_num + One::one(), + end: number - One::one(), + gap_type: BlockGapType::MissingHeaderAndBody, + }; transaction.set(columns::META, meta_keys::BLOCK_GAP, &gap.encode()); block_gap = Some(gap); block_gap_updated = true; diff --git a/substrate/client/db/src/utils.rs b/substrate/client/db/src/utils.rs index b532e0d46662..8b44f0a3e177 100644 --- a/substrate/client/db/src/utils.rs +++ b/substrate/client/db/src/utils.rs @@ -25,6 +25,7 @@ use log::{debug, info}; use crate::{Database, DatabaseSource, DbHash}; use codec::Decode; +use sc_client_api::blockchain::BlockGap; use sp_database::Transaction; use sp_runtime::{ generic::BlockId, @@ -73,8 +74,8 @@ pub struct Meta { pub genesis_hash: H, /// Finalized state, if any pub finalized_state: Option<(H, N)>, - /// Block gap, start and end inclusive, if any. - pub block_gap: Option<(N, N)>, + /// Block gap, if any. + pub block_gap: Option>, } /// A block lookup key: used for canonical lookup from block number to hash diff --git a/substrate/client/network/sync/src/strategy/chain_sync.rs b/substrate/client/network/sync/src/strategy/chain_sync.rs index 21e474048625..f29ed1b083e8 100644 --- a/substrate/client/network/sync/src/strategy/chain_sync.rs +++ b/substrate/client/network/sync/src/strategy/chain_sync.rs @@ -44,7 +44,7 @@ use crate::{ use codec::Encode; use log::{debug, error, info, trace, warn}; use prometheus_endpoint::{register, Gauge, PrometheusError, Registry, U64}; -use sc_client_api::{BlockBackend, ProofProvider}; +use sc_client_api::{blockchain::BlockGap, BlockBackend, ProofProvider}; use sc_consensus::{BlockImportError, BlockImportStatus, IncomingBlock}; use sc_network_common::sync::message::{ BlockAnnounce, BlockAttributes, BlockData, BlockRequest, BlockResponse, Direction, FromBlock, @@ -1381,7 +1381,7 @@ where } } - if let Some((start, end)) = info.block_gap { + if let Some(BlockGap { start, end, .. }) = info.block_gap { debug!(target: LOG_TARGET, "Starting gap sync #{start} - #{end}"); self.gap_sync = Some(GapSync { best_queued_number: start - One::one(), diff --git a/substrate/client/service/src/client/client.rs b/substrate/client/service/src/client/client.rs index 22defd7c5514..8b699c7faffd 100644 --- a/substrate/client/service/src/client/client.rs +++ b/substrate/client/service/src/client/client.rs @@ -604,9 +604,8 @@ where } let info = self.backend.blockchain().info(); - let gap_block = info - .block_gap - .map_or(false, |(start, _)| *import_headers.post().number() == start); + let gap_block = + info.block_gap.map_or(false, |gap| *import_headers.post().number() == gap.start); // the block is lower than our last finalized block so it must revert // finality, refusing import. diff --git a/substrate/primitives/blockchain/src/backend.rs b/substrate/primitives/blockchain/src/backend.rs index fd0c5795cbfd..d7386a71a0d1 100644 --- a/substrate/primitives/blockchain/src/backend.rs +++ b/substrate/primitives/blockchain/src/backend.rs @@ -17,6 +17,7 @@ //! Substrate blockchain trait +use codec::{Decode, Encode}; use parking_lot::RwLock; use sp_runtime::{ generic::BlockId, @@ -109,7 +110,7 @@ pub trait ForkBackend: for block in tree_route.retracted() { expanded_forks.insert(block.hash); } - continue + continue; }, Err(_) => { // There are cases when blocks are missing (e.g. warp-sync). @@ -196,7 +197,7 @@ pub trait Backend: let info = self.info(); if info.finalized_number > *base_header.number() { // `base_header` is on a dead fork. - return Ok(None) + return Ok(None); } self.leaves()? }; @@ -207,7 +208,7 @@ pub trait Backend: // go backwards through the chain (via parent links) loop { if current_hash == base_hash { - return Ok(Some(leaf_hash)) + return Ok(Some(leaf_hash)); } let current_header = self @@ -216,7 +217,7 @@ pub trait Backend: // stop search in this chain once we go below the target's block number if current_header.number() < base_header.number() { - break + break; } current_hash = *current_header.parent_hash(); @@ -266,7 +267,7 @@ pub trait Backend: // If we have only one leaf there are no forks, and we can return early. if finalized_block_number == Zero::zero() || leaves.len() == 1 { - return Ok(DisplacedLeavesAfterFinalization::default()) + return Ok(DisplacedLeavesAfterFinalization::default()); } // Store hashes of finalized blocks for quick checking later, the last block is the @@ -332,7 +333,7 @@ pub trait Backend: elapsed = ?now.elapsed(), "Added genesis leaf to displaced leaves." ); - continue + continue; } debug!( @@ -539,6 +540,29 @@ impl DisplacedLeavesAfterFinalization { } } +/// Represents the type of block gaps that may result from either warp sync or fast sync. +#[derive(Debug, Clone, Copy, Eq, PartialEq, Encode, Decode)] +pub enum BlockGapType { + /// Both the header and body are missing, as a result of warp sync. + MissingHeaderAndBody, + /// The block body is missing, as a result of fast sync. + MissingBody, +} + +/// Represents the block gap resulted by warp sync or fast sync. +/// +/// A block gap is a range of blocks where either the bodies, or both headers and bodies are +/// missing. +#[derive(Debug, Clone, Copy, Eq, PartialEq, Encode, Decode)] +pub struct BlockGap { + /// The starting block number of the gap (inclusive). + pub start: N, + /// The ending block number of the gap (inclusive). + pub end: N, + /// The type of gap. + pub gap_type: BlockGapType, +} + /// Blockchain info #[derive(Debug, Eq, PartialEq, Clone)] pub struct Info { @@ -556,8 +580,8 @@ pub struct Info { pub finalized_state: Option<(Block::Hash, <::Header as HeaderT>::Number)>, /// Number of concurrent leave forks. pub number_leaves: usize, - /// Missing blocks after warp sync. (start, end). - pub block_gap: Option<(NumberFor, NumberFor)>, + /// Missing blocks after warp sync or fast sync. + pub block_gap: Option>>, } /// Block status. From f45211a10875f166084ab01a21952b9e7f45a6ca Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Wed, 4 Sep 2024 23:19:12 +0800 Subject: [PATCH 2/3] Handle compatibility for BlockGap structure update This commit introduces changes to address compatibility issues resulting from the transition from the old block gap representation `(NumberFor, NumberFor)` to the new `BlockGap>` structure. --- substrate/client/db/src/lib.rs | 12 +++++++++ substrate/client/db/src/utils.rs | 44 +++++++++++++++++++++++++++----- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/substrate/client/db/src/lib.rs b/substrate/client/db/src/lib.rs index 16c225d1f00f..4559a01e57e3 100644 --- a/substrate/client/db/src/lib.rs +++ b/substrate/client/db/src/lib.rs @@ -92,6 +92,7 @@ use sp_state_machine::{ StorageValue, UsageInfo as StateUsageInfo, }; use sp_trie::{cache::SharedTrieCache, prefixed_key, MemoryDB, MerkleValue, PrefixedMemoryDB}; +use utils::BLOCK_GAP_CURRENT_VERSION; // Re-export the Database trait so that one can pass an implementation of it. pub use sc_state_db::PruningMode; @@ -1685,6 +1686,7 @@ impl Backend { )?; if gap.start > gap.end { transaction.remove(columns::META, meta_keys::BLOCK_GAP); + transaction.remove(columns::META, meta_keys::BLOCK_GAP_VERSION); block_gap = None; debug!(target: "db", "Removed block gap."); } else { @@ -1695,6 +1697,11 @@ impl Backend { meta_keys::BLOCK_GAP, &gap.encode(), ); + transaction.set( + columns::META, + meta_keys::BLOCK_GAP_VERSION, + &BLOCK_GAP_CURRENT_VERSION.encode(), + ); } block_gap_updated = true; }, @@ -1711,6 +1718,11 @@ impl Backend { gap_type: BlockGapType::MissingHeaderAndBody, }; transaction.set(columns::META, meta_keys::BLOCK_GAP, &gap.encode()); + transaction.set( + columns::META, + meta_keys::BLOCK_GAP_VERSION, + &BLOCK_GAP_CURRENT_VERSION.encode(), + ); block_gap = Some(gap); block_gap_updated = true; debug!(target: "db", "Detected block gap {block_gap:?}"); diff --git a/substrate/client/db/src/utils.rs b/substrate/client/db/src/utils.rs index 8b44f0a3e177..0b591c967e60 100644 --- a/substrate/client/db/src/utils.rs +++ b/substrate/client/db/src/utils.rs @@ -25,11 +25,14 @@ use log::{debug, info}; use crate::{Database, DatabaseSource, DbHash}; use codec::Decode; -use sc_client_api::blockchain::BlockGap; +use sc_client_api::blockchain::{BlockGap, BlockGapType}; use sp_database::Transaction; use sp_runtime::{ generic::BlockId, - traits::{Block as BlockT, Header as HeaderT, UniqueSaturatedFrom, UniqueSaturatedInto, Zero}, + traits::{ + Block as BlockT, Header as HeaderT, NumberFor, UniqueSaturatedFrom, UniqueSaturatedInto, + Zero, + }, }; use sp_trie::DBValue; @@ -39,6 +42,9 @@ pub const NUM_COLUMNS: u32 = 13; /// Meta column. The set of keys in the column is shared by full && light storages. pub const COLUMN_META: u32 = 0; +/// Current block gap version. +pub const BLOCK_GAP_CURRENT_VERSION: u32 = 1; + /// Keys of entries in COLUMN_META. pub mod meta_keys { /// Type of storage (full or light). @@ -51,6 +57,8 @@ pub mod meta_keys { pub const FINALIZED_STATE: &[u8; 6] = b"fstate"; /// Block gap. pub const BLOCK_GAP: &[u8; 3] = b"gap"; + /// Block gap version. + pub const BLOCK_GAP_VERSION: &[u8; 7] = b"gap_ver"; /// Genesis block hash. pub const GENESIS_HASH: &[u8; 3] = b"gen"; /// Leaves prefix list key. @@ -198,7 +206,7 @@ fn open_database_at( open_kvdb_rocksdb::(path, db_type, create, *cache_size)?, DatabaseSource::Custom { db, require_create_flag } => { if *require_create_flag && !create { - return Err(OpenDbError::DoesNotExist) + return Err(OpenDbError::DoesNotExist); } db.clone() }, @@ -365,7 +373,7 @@ pub fn check_database_type( return Err(OpenDbError::UnexpectedDbType { expected: db_type, found: stored_type.to_owned(), - }) + }); }, None => { let mut transaction = Transaction::new(); @@ -516,9 +524,31 @@ where } else { None }; - let block_gap = db - .get(COLUMN_META, meta_keys::BLOCK_GAP) - .and_then(|d| Decode::decode(&mut d.as_slice()).ok()); + let block_gap = match db + .get(COLUMN_META, meta_keys::BLOCK_GAP_VERSION) + .and_then(|d| u32::decode(&mut d.as_slice()).ok()) + { + None => { + let old_block_gap: Option<(NumberFor, NumberFor)> = db + .get(COLUMN_META, meta_keys::BLOCK_GAP) + .and_then(|d| Decode::decode(&mut d.as_slice()).ok()); + + old_block_gap.map(|(start, end)| BlockGap { + start, + end, + gap_type: BlockGapType::MissingHeaderAndBody, + }) + }, + Some(version) => match version { + BLOCK_GAP_CURRENT_VERSION => db + .get(COLUMN_META, meta_keys::BLOCK_GAP) + .and_then(|d| Decode::decode(&mut d.as_slice()).ok()), + v => + return Err(sp_blockchain::Error::Backend(format!( + "Unsupported block gap DB version: {v}" + ))), + }, + }; debug!(target: "db", "block_gap={:?}", block_gap); Ok(Meta { From 3a15713ce8a665f6a66e3a9632fa948bcb6478ca Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Thu, 5 Sep 2024 00:06:29 +0800 Subject: [PATCH 3/3] Add prdoc --- prdoc/pr_5592.prdoc | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 prdoc/pr_5592.prdoc diff --git a/prdoc/pr_5592.prdoc b/prdoc/pr_5592.prdoc new file mode 100644 index 000000000000..9d51917db7b1 --- /dev/null +++ b/prdoc/pr_5592.prdoc @@ -0,0 +1,26 @@ +title: Introduce `BlockGap` + +doc: + - audience: Node Dev + description: | + This is the first step towards https://github.com/paritytech/polkadot-sdk/issues/5406, + refactoring the representation of block gap. This refactor converts the existing + `(NumberFor, NumberFor)` into a dedicated `BlockGap>` + struct. This change is purely structural and does not alter existing logic, but lays + the groundwork for the follow-up PR. The compatibility concern in the database caused + by the new structure transition is addressed as well. + + The `BlockGap` refactoring results in breaking changes in the `Info` structure returned + in `client.info()`. + +crates: + - name: sc-consensus-babe + bump: none + - name: sc-client-db + bump: none + - name: sc-network-sync + bump: none + - name: sc-service + bump: none + - name: sp-blockchain + bump: major