From 9b28a5453a83e21b71f18401f3662078fbd0de1b Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Wed, 4 Sep 2024 18:50:26 +0800 Subject: [PATCH] Avoid updating the block gap when it's unchanged (#5540) There are basically three commits in this PR. Since all these commits essentially have no logical changes, I packed them into one PR. Review by per-commit is recommended. - The first commit avoids unnecessarily updating the block gap storage when the value remains unchanged, as discovered when I worked on https://github.com/paritytech/polkadot-sdk/issues/5406. - The second commit is purely about format string style changes but deletes ~10 lines of code, which slightly helps me look into this file :P - The third commit is added to avoid the unnecessary block gap update in `BlockchainDb`. --------- Co-authored-by: Davide Galassi --- prdoc/pr_5540.prdoc | 12 +++ substrate/client/db/src/lib.rs | 156 +++++++++++++++------------------ 2 files changed, 82 insertions(+), 86 deletions(-) create mode 100644 prdoc/pr_5540.prdoc diff --git a/prdoc/pr_5540.prdoc b/prdoc/pr_5540.prdoc new file mode 100644 index 000000000000..2c00714c4f86 --- /dev/null +++ b/prdoc/pr_5540.prdoc @@ -0,0 +1,12 @@ +title: Avoid unnecessary block gap updates + +doc: + - audience: Node Dev + description: | + Previously, the block gap storage in database and state in `BlockchainDb` could be updated even if no changes occurred. + This commit refines the logic to ensure updates only occur when the block gap value actually changes, reducing unnecessary + writes and enhancing overall efficiency. + +crates: + - name: sc-client-db + bump: none diff --git a/substrate/client/db/src/lib.rs b/substrate/client/db/src/lib.rs index ba0cbc09d53d..eadb26254a18 100644 --- a/substrate/client/db/src/lib.rs +++ b/substrate/client/db/src/lib.rs @@ -538,7 +538,7 @@ impl BlockchainDb { fn insert_justifications_if_pinned(&self, hash: Block::Hash, justification: Justification) { let mut cache = self.pinned_blocks_cache.write(); if !cache.contains(hash) { - return + return; } let justifications = Justifications::from(justification); @@ -551,7 +551,7 @@ impl BlockchainDb { fn insert_persisted_justifications_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { let mut cache = self.pinned_blocks_cache.write(); if !cache.contains(hash) { - return Ok(()) + return Ok(()); } let justifications = self.justifications_uncached(hash)?; @@ -565,7 +565,7 @@ impl BlockchainDb { fn insert_persisted_body_if_pinned(&self, hash: Block::Hash) -> ClientResult<()> { let mut cache = self.pinned_blocks_cache.write(); if !cache.contains(hash) { - return Ok(()) + return Ok(()); } let body = self.body_uncached(hash)?; @@ -594,8 +594,7 @@ impl BlockchainDb { Ok(justifications) => Ok(Some(justifications)), Err(err) => return Err(sp_blockchain::Error::Backend(format!( - "Error decoding justifications: {}", - err + "Error decoding justifications: {err}" ))), }, None => Ok(None), @@ -610,10 +609,7 @@ impl BlockchainDb { match Decode::decode(&mut &body[..]) { Ok(body) => return Ok(Some(body)), Err(err) => - return Err(sp_blockchain::Error::Backend(format!( - "Error decoding body: {}", - err - ))), + return Err(sp_blockchain::Error::Backend(format!("Error decoding body: {err}"))), } } @@ -636,8 +632,7 @@ impl BlockchainDb { let ex = Block::Extrinsic::decode(&mut input).map_err( |err| { sp_blockchain::Error::Backend(format!( - "Error decoding indexed extrinsic: {}", - err + "Error decoding indexed extrinsic: {err}" )) }, )?; @@ -645,8 +640,7 @@ impl BlockchainDb { }, None => return Err(sp_blockchain::Error::Backend(format!( - "Missing indexed transaction {:?}", - hash + "Missing indexed transaction {hash:?}" ))), }; }, @@ -655,12 +649,11 @@ impl BlockchainDb { }, } } - return Ok(Some(body)) + return Ok(Some(body)); }, Err(err) => return Err(sp_blockchain::Error::Backend(format!( - "Error decoding body list: {}", - err + "Error decoding body list: {err}", ))), } } @@ -672,7 +665,7 @@ impl sc_client_api::blockchain::HeaderBackend for Blockcha fn header(&self, hash: Block::Hash) -> ClientResult> { let mut cache = self.header_cache.lock(); if let Some(result) = cache.get_refresh(&hash) { - return Ok(result.clone()) + return Ok(result.clone()); } let header = utils::read_header( &*self.db, @@ -724,7 +717,7 @@ impl sc_client_api::blockchain::Backend for BlockchainDb ClientResult>> { let cache = self.pinned_blocks_cache.read(); if let Some(result) = cache.body(&hash) { - return Ok(result.clone()) + return Ok(result.clone()); } self.body_uncached(hash) @@ -733,7 +726,7 @@ impl sc_client_api::blockchain::Backend for BlockchainDb ClientResult> { let cache = self.pinned_blocks_cache.read(); if let Some(result) = cache.justifications(&hash) { - return Ok(result.clone()) + return Ok(result.clone()); } self.justifications_uncached(hash) @@ -778,8 +771,7 @@ impl sc_client_api::blockchain::Backend for BlockchainDb transactions.push(t), None => return Err(sp_blockchain::Error::Backend(format!( - "Missing indexed transaction {:?}", - hash + "Missing indexed transaction {hash:?}", ))), } } @@ -787,7 +779,7 @@ impl sc_client_api::blockchain::Backend for BlockchainDb - Err(sp_blockchain::Error::Backend(format!("Error decoding body list: {}", err))), + Err(sp_blockchain::Error::Backend(format!("Error decoding body list: {err}"))), } } } @@ -810,8 +802,7 @@ impl HeaderMetadata for BlockchainDb { }) .ok_or_else(|| { ClientError::UnknownBlock(format!( - "Header was not found in the database: {:?}", - hash + "Header was not found in the database: {hash:?}", )) }) }, @@ -858,7 +849,7 @@ impl BlockImportOperation { } if count > 0 { - log::debug!(target: "sc_offchain", "Applied {} offchain indexing changes.", count); + log::debug!(target: "sc_offchain", "Applied {count} offchain indexing changes."); } } @@ -877,7 +868,7 @@ impl BlockImportOperation { state_version: StateVersion, ) -> ClientResult { if storage.top.keys().any(|k| well_known_keys::is_child_storage_key(k)) { - return Err(sp_blockchain::Error::InvalidState) + return Err(sp_blockchain::Error::InvalidState); } let child_delta = storage.children_default.values().map(|child_content| { @@ -1011,7 +1002,7 @@ impl sp_state_machine::Storage> for StorageDb Backend { if meta.best_number.saturating_sub(best_number).saturated_into::() > self.canonicalization_delay { - return Err(sp_blockchain::Error::SetHeadTooOld) + return Err(sp_blockchain::Error::SetHeadTooOld); } let parent_exists = @@ -1307,7 +1298,7 @@ impl Backend { (&r.number, &r.hash) ); - return Err(sp_blockchain::Error::NotInFinalizedChain) + return Err(sp_blockchain::Error::NotInFinalizedChain); } retracted.push(r.hash); @@ -1349,10 +1340,9 @@ impl Backend { *header.parent_hash() != last_finalized { return Err(sp_blockchain::Error::NonSequentialFinalization(format!( - "Last finalized {:?} not parent of {:?}", - last_finalized, + "Last finalized {last_finalized:?} not parent of {:?}", header.hash() - ))) + ))); } Ok(()) } @@ -1429,10 +1419,10 @@ impl Backend { hash_to_canonicalize, to_canonicalize.saturated_into(), ) { - return Ok(()) + return Ok(()); } - trace!(target: "db", "Canonicalize block #{} ({:?})", to_canonicalize, hash_to_canonicalize); + trace!(target: "db", "Canonicalize block #{to_canonicalize} ({hash_to_canonicalize:?})"); let commit = self.storage.state_db.canonicalize_block(&hash_to_canonicalize).map_err( sp_blockchain::Error::from_state_db::< sc_state_db::Error, @@ -1456,6 +1446,8 @@ impl Backend { (meta.best_number, meta.finalized_hash, meta.finalized_number, meta.block_gap) }; + let mut block_gap_updated = false; + let mut current_transaction_justifications: HashMap = HashMap::new(); let mut finalized_blocks = operation.finalized_blocks.into_iter().peekable(); @@ -1623,13 +1615,8 @@ impl Backend { let is_best = pending_block.leaf_state.is_best(); debug!( target: "db", - "DB Commit {:?} ({}), best={}, state={}, existing={}, finalized={}", - hash, - number, - is_best, + "DB Commit {hash:?} ({number}), best={is_best}, state={}, existing={existing_header}, finalized={finalized}", operation.commit_state, - existing_header, - finalized, ); self.state_usage.merge_sm(operation.old_state.usage_info()); @@ -1693,19 +1680,20 @@ impl Backend { 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(), - ); + 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; } } else if number > best_num + One::one() && number > One::one() && self.blockchain.header(parent_hash)?.is_none() @@ -1713,7 +1701,8 @@ impl Backend { let gap = (best_num + One::one(), number - One::one()); transaction.set(columns::META, meta_keys::BLOCK_GAP, &gap.encode()); block_gap = Some(gap); - debug!(target: "db", "Detected block gap {:?}", block_gap); + block_gap_updated = true; + debug!(target: "db", "Detected block gap {block_gap:?}"); } } @@ -1747,9 +1736,8 @@ impl Backend { }); } else { return Err(sp_blockchain::Error::UnknownBlock(format!( - "Cannot set head {:?}", - set_head - ))) + "Cannot set head {set_head:?}", + ))); } } @@ -1759,7 +1747,7 @@ impl Backend { // Code beyond this point can't fail. if let Some((header, hash)) = imported { - trace!(target: "db", "DB Commit done {:?}", hash); + trace!(target: "db", "DB Commit done {hash:?}"); let header_metadata = CachedHeaderMetadata::from(&header); self.blockchain.insert_header_metadata(header_metadata.hash, header_metadata); cache_header(&mut self.blockchain.header_cache.lock(), hash, Some(header)); @@ -1768,7 +1756,9 @@ impl Backend { for m in meta_updates { self.blockchain.update_meta(m); } - self.blockchain.update_block_gap(block_gap); + if block_gap_updated { + self.blockchain.update_block_gap(block_gap); + } Ok(()) } @@ -1875,7 +1865,7 @@ impl Backend { transaction: &mut Transaction, id: BlockId, ) -> ClientResult<()> { - debug!(target: "db", "Removing block #{}", id); + debug!(target: "db", "Removing block #{id}"); utils::remove_from_db( transaction, &*self.storage.db, @@ -1909,8 +1899,7 @@ impl Backend { }, Err(err) => return Err(sp_blockchain::Error::Backend(format!( - "Error decoding body list: {}", - err + "Error decoding body list: {err}", ))), } } @@ -2138,14 +2127,14 @@ impl sc_client_api::backend::Backend for Backend { if number > self.blockchain.info().finalized_number || (hash != last_finalized && !is_descendent_of(&hash, &last_finalized)?) { - return Err(ClientError::NotInFinalizedChain) + return Err(ClientError::NotInFinalizedChain); } let justifications = if let Some(mut stored_justifications) = self.blockchain.justifications(hash)? { if !stored_justifications.append(justification) { - return Err(ClientError::BadJustification("Duplicate consensus engine ID".into())) + return Err(ClientError::BadJustification("Duplicate consensus engine ID".into())); } stored_justifications } else { @@ -2230,13 +2219,12 @@ impl sc_client_api::backend::Backend for Backend { let mut revert_blocks = || -> ClientResult> { for c in 0..n.saturated_into::() { if number_to_revert.is_zero() { - return Ok(c.saturated_into::>()) + return Ok(c.saturated_into::>()); } let mut transaction = Transaction::new(); let removed = self.blockchain.header(hash_to_revert)?.ok_or_else(|| { sp_blockchain::Error::UnknownBlock(format!( - "Error reverting to {}. Block header not found.", - hash_to_revert, + "Error reverting to {hash_to_revert}. Block header not found.", )) })?; let removed_hash = removed.hash(); @@ -2246,7 +2234,7 @@ impl sc_client_api::backend::Backend for Backend { if prev_number == best_number { best_hash } else { *removed.parent_hash() }; if !self.have_state_at(prev_hash, prev_number) { - return Ok(c.saturated_into::>()) + return Ok(c.saturated_into::>()); } match self.storage.state_db.revert_one() { @@ -2342,23 +2330,21 @@ impl sc_client_api::backend::Backend for Backend { let best_hash = self.blockchain.info().best_hash; if best_hash == hash { - return Err(sp_blockchain::Error::Backend(format!("Can't remove best block {:?}", hash))) + return Err(sp_blockchain::Error::Backend(format!("Can't remove best block {hash:?}"))); } let hdr = self.blockchain.header_metadata(hash)?; if !self.have_state_at(hash, hdr.number) { return Err(sp_blockchain::Error::UnknownBlock(format!( - "State already discarded for {:?}", - hash - ))) + "State already discarded for {hash:?}", + ))); } let mut leaves = self.blockchain.leaves.write(); if !leaves.contains(hdr.number, hash) { return Err(sp_blockchain::Error::Backend(format!( - "Can't remove non-leaf block {:?}", - hash - ))) + "Can't remove non-leaf block {hash:?}", + ))); } let mut transaction = Transaction::new(); @@ -2398,7 +2384,7 @@ impl sc_client_api::backend::Backend for Backend { if let Some(outcome) = remove_outcome { leaves.undo().undo_remove(outcome); } - return Err(e.into()) + return Err(e.into()); } self.blockchain().remove_header_metadata(hash); Ok(()) @@ -2420,7 +2406,7 @@ impl sc_client_api::backend::Backend for Backend { .build(); let state = RefTrackingState::new(db_state, self.storage.clone(), None); - return Ok(RecordStatsState::new(state, None, self.state_usage.clone())) + return Ok(RecordStatsState::new(state, None, self.state_usage.clone())); } } @@ -2446,8 +2432,7 @@ impl sc_client_api::backend::Backend for Backend { Ok(RecordStatsState::new(state, Some(hash), self.state_usage.clone())) } else { Err(sp_blockchain::Error::UnknownBlock(format!( - "State already discarded for {:?}", - hash + "State already discarded for {hash:?}", ))) } }, @@ -2512,16 +2497,14 @@ impl sc_client_api::backend::Backend for Backend { self.storage.state_db.pin(&hash, number.saturated_into::(), hint).map_err( |_| { sp_blockchain::Error::UnknownBlock(format!( - "Unable to pin: state already discarded for `{:?}`", - hash + "Unable to pin: state already discarded for `{hash:?}`", )) }, )?; } else { return Err(ClientError::UnknownBlock(format!( - "Can not pin block with hash `{:?}`. Block not found.", - hash - ))) + "Can not pin block with hash `{hash:?}`. Block not found.", + ))); } if self.blocks_pruning != BlocksPruning::KeepAll { @@ -4226,8 +4209,9 @@ pub(crate) mod tests { match pruning_mode { // we can only revert to blocks for which we have state, if pruning is enabled // then the last state available will be that of the latest finalized block - BlocksPruning::Some(_) => - assert_eq!(backend.blockchain().info().finalized_number, 8), + BlocksPruning::Some(_) => { + assert_eq!(backend.blockchain().info().finalized_number, 8) + }, // otherwise if we're not doing state pruning we can revert past finalized blocks _ => assert_eq!(backend.blockchain().info().finalized_number, 5), }