From 9b28a5453a83e21b71f18401f3662078fbd0de1b Mon Sep 17 00:00:00 2001
From: Liu-Cheng Xu <xuliuchengxlc@gmail.com>
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 <davxy@datawok.net>
---
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 00000000000..2c00714c4f8
--- /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 ba0cbc09d53..eadb26254a1 100644
--- a/substrate/client/db/src/lib.rs
+++ b/substrate/client/db/src/lib.rs
@@ -538,7 +538,7 @@ impl<Block: BlockT> BlockchainDb<Block> {
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<Block: BlockT> BlockchainDb<Block> {
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<Block: BlockT> BlockchainDb<Block> {
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<Block: BlockT> BlockchainDb<Block> {
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<Block: BlockT> BlockchainDb<Block> {
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<Block: BlockT> BlockchainDb<Block> {
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<Block: BlockT> BlockchainDb<Block> {
},
None =>
return Err(sp_blockchain::Error::Backend(format!(
- "Missing indexed transaction {:?}",
- hash
+ "Missing indexed transaction {hash:?}"
))),
};
},
@@ -655,12 +649,11 @@ impl<Block: BlockT> BlockchainDb<Block> {
},
}
}
- 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<Block: BlockT> sc_client_api::blockchain::HeaderBackend<Block> for Blockcha
fn header(&self, hash: Block::Hash) -> ClientResult<Option<Block::Header>> {
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<Block: BlockT> sc_client_api::blockchain::Backend<Block> for BlockchainDb<B
fn body(&self, hash: Block::Hash) -> ClientResult<Option<Vec<Block::Extrinsic>>> {
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<Block: BlockT> sc_client_api::blockchain::Backend<Block> for BlockchainDb<B
fn justifications(&self, hash: Block::Hash) -> ClientResult<Option<Justifications>> {
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<Block: BlockT> sc_client_api::blockchain::Backend<Block> for BlockchainDb<B
Some(t) => transactions.push(t),
None =>
return Err(sp_blockchain::Error::Backend(format!(
- "Missing indexed transaction {:?}",
- hash
+ "Missing indexed transaction {hash:?}",
))),
}
}
@@ -787,7 +779,7 @@ impl<Block: BlockT> sc_client_api::blockchain::Backend<Block> for BlockchainDb<B
Ok(Some(transactions))
},
Err(err) =>
- 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<Block: BlockT> HeaderMetadata<Block> for BlockchainDb<Block> {
})
.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<Block: BlockT> BlockImportOperation<Block> {
}
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<Block: BlockT> BlockImportOperation<Block> {
state_version: StateVersion,
) -> ClientResult<Block::Hash> {
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<Block: BlockT> sp_state_machine::Storage<HashingFor<Block>> for StorageDb<B
} else {
self.state_db.get(key.as_ref(), self)
}
- .map_err(|e| format!("Database backend error: {:?}", e))
+ .map_err(|e| format!("Database backend error: {e:?}"))
}
}
@@ -1288,7 +1279,7 @@ impl<Block: BlockT> Backend<Block> {
if meta.best_number.saturating_sub(best_number).saturated_into::<u64>() >
self.canonicalization_delay
{
- return Err(sp_blockchain::Error::SetHeadTooOld)
+ return Err(sp_blockchain::Error::SetHeadTooOld);
}
let parent_exists =
@@ -1307,7 +1298,7 @@ impl<Block: BlockT> Backend<Block> {
(&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<Block: BlockT> Backend<Block> {
*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<Block: BlockT> Backend<Block> {
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<sp_database::error::DatabaseError>,
@@ -1456,6 +1446,8 @@ impl<Block: BlockT> Backend<Block> {
(meta.best_number, meta.finalized_hash, meta.finalized_number, meta.block_gap)
};
+ let mut block_gap_updated = false;
+
let mut current_transaction_justifications: HashMap<Block::Hash, Justification> =
HashMap::new();
let mut finalized_blocks = operation.finalized_blocks.into_iter().peekable();
@@ -1623,13 +1615,8 @@ impl<Block: BlockT> Backend<Block> {
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<Block: BlockT> Backend<Block> {
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<Block: BlockT> Backend<Block> {
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<Block: BlockT> Backend<Block> {
});
} else {
return Err(sp_blockchain::Error::UnknownBlock(format!(
- "Cannot set head {:?}",
- set_head
- )))
+ "Cannot set head {set_head:?}",
+ )));
}
}
@@ -1759,7 +1747,7 @@ impl<Block: BlockT> Backend<Block> {
// 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<Block: BlockT> Backend<Block> {
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<Block: BlockT> Backend<Block> {
transaction: &mut Transaction<DbHash>,
id: BlockId<Block>,
) -> 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<Block: BlockT> Backend<Block> {
},
Err(err) =>
return Err(sp_blockchain::Error::Backend(format!(
- "Error decoding body list: {}",
- err
+ "Error decoding body list: {err}",
))),
}
}
@@ -2138,14 +2127,14 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
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<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
let mut revert_blocks = || -> ClientResult<NumberFor<Block>> {
for c in 0..n.saturated_into::<u64>() {
if number_to_revert.is_zero() {
- return Ok(c.saturated_into::<NumberFor<Block>>())
+ return Ok(c.saturated_into::<NumberFor<Block>>());
}
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<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
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::<NumberFor<Block>>())
+ return Ok(c.saturated_into::<NumberFor<Block>>());
}
match self.storage.state_db.revert_one() {
@@ -2342,23 +2330,21 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
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<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
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<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
.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<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
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<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
self.storage.state_db.pin(&hash, number.saturated_into::<u64>(), 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),
}
--
GitLab