diff --git a/Cargo.lock b/Cargo.lock
index ad75224fefdc47bc2f3c21b789464bbae9ca050c..aad15fe033d1c42f13825d8d9c2b1a5801a4fdd4 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -19855,7 +19855,6 @@ name = "sp-blockchain"
version = "28.0.0"
dependencies = [
"futures",
- "log",
"parity-scale-codec",
"parking_lot 0.12.3",
"schnellru",
@@ -19866,6 +19865,7 @@ dependencies = [
"sp-runtime",
"sp-state-machine",
"thiserror",
+ "tracing",
]
[[package]]
diff --git a/prdoc/pr_4997.prdoc b/prdoc/pr_4997.prdoc
new file mode 100644
index 0000000000000000000000000000000000000000..25620a7e63ea817e9009cf557748deb63d921f2e
--- /dev/null
+++ b/prdoc/pr_4997.prdoc
@@ -0,0 +1,20 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Do not crash on block gap in displaced_leaves_after_finalizing
+
+doc:
+ - audience:
+ - Node Operator
+ - Node Dev
+ description: |
+ After recent changes, crashes where occuring when calculating displaced branches after a block was finalized.
+ The reason are block gaps in the finalized chain. When encountering unknown blocks, the node was panicking.
+ This PR introduces changes to tolerate unknown blocks. Leafs that are separated by a gap from the to-be-finalized
+ block are not marked as displaced.
+
+crates:
+- name: sc-client-db
+ bump: none
+- name: sp-blockchain
+ bump: patch
diff --git a/substrate/client/db/src/lib.rs b/substrate/client/db/src/lib.rs
index e95cd9e4ad5fd3e527f3f870aaf40948b238c9a2..acd165d916135008749e1e04d1b3e3b919819a0b 100644
--- a/substrate/client/db/src/lib.rs
+++ b/substrate/client/db/src/lib.rs
@@ -2613,6 +2613,35 @@ pub(crate) mod tests {
Ok(header.hash())
}
+ pub fn insert_disconnected_header(
+ backend: &Backend<Block>,
+ number: u64,
+ parent_hash: H256,
+ extrinsics_root: H256,
+ best: bool,
+ ) -> H256 {
+ use sp_runtime::testing::Digest;
+
+ let digest = Digest::default();
+ let header =
+ Header { number, parent_hash, state_root: Default::default(), digest, extrinsics_root };
+
+ let mut op = backend.begin_operation().unwrap();
+
+ op.set_block_data(
+ header.clone(),
+ Some(vec![]),
+ None,
+ None,
+ if best { NewBlockState::Best } else { NewBlockState::Normal },
+ )
+ .unwrap();
+
+ backend.commit_operation(op).unwrap();
+
+ header.hash()
+ }
+
pub fn insert_header_no_head(
backend: &Backend<Block>,
number: u64,
@@ -3112,6 +3141,123 @@ pub(crate) mod tests {
}
}
+ #[test]
+ fn displaced_leaves_after_finalizing_works_with_disconnect() {
+ // In this test we will create a situation that can typically happen after warp sync.
+ // The situation looks like this:
+ // g -> <unimported> -> a3 -> a4
+ // Basically there is a gap of unimported blocks at some point in the chain.
+ let backend = Backend::<Block>::new_test(1000, 100);
+ let blockchain = backend.blockchain();
+ let genesis_number = 0;
+ let genesis_hash =
+ insert_header(&backend, genesis_number, Default::default(), None, Default::default());
+
+ let a3_number = 3;
+ let a3_hash = insert_disconnected_header(
+ &backend,
+ a3_number,
+ H256::from([200; 32]),
+ H256::from([1; 32]),
+ true,
+ );
+
+ let a4_number = 4;
+ let a4_hash =
+ insert_disconnected_header(&backend, a4_number, a3_hash, H256::from([2; 32]), true);
+ {
+ let displaced =
+ blockchain.displaced_leaves_after_finalizing(a3_hash, a3_number).unwrap();
+ assert_eq!(blockchain.leaves().unwrap(), vec![a4_hash, genesis_hash]);
+ assert_eq!(displaced.displaced_leaves, vec![]);
+ assert_eq!(displaced.displaced_blocks, vec![]);
+ }
+
+ {
+ let displaced =
+ blockchain.displaced_leaves_after_finalizing(a4_hash, a4_number).unwrap();
+ assert_eq!(blockchain.leaves().unwrap(), vec![a4_hash, genesis_hash]);
+ assert_eq!(displaced.displaced_leaves, vec![]);
+ assert_eq!(displaced.displaced_blocks, vec![]);
+ }
+
+ // Import block a1 which has the genesis block as parent.
+ // g -> a1 -> <unimported> -> a3(f) -> a4
+ let a1_number = 1;
+ let a1_hash = insert_disconnected_header(
+ &backend,
+ a1_number,
+ genesis_hash,
+ H256::from([123; 32]),
+ false,
+ );
+ {
+ let displaced =
+ blockchain.displaced_leaves_after_finalizing(a3_hash, a3_number).unwrap();
+ assert_eq!(blockchain.leaves().unwrap(), vec![a4_hash, a1_hash]);
+ assert_eq!(displaced.displaced_leaves, vec![]);
+ assert_eq!(displaced.displaced_blocks, vec![]);
+ }
+
+ // Import block b1 which has the genesis block as parent.
+ // g -> a1 -> <unimported> -> a3(f) -> a4
+ // \-> b1
+ let b1_number = 1;
+ let b1_hash = insert_disconnected_header(
+ &backend,
+ b1_number,
+ genesis_hash,
+ H256::from([124; 32]),
+ false,
+ );
+ {
+ let displaced =
+ blockchain.displaced_leaves_after_finalizing(a3_hash, a3_number).unwrap();
+ assert_eq!(blockchain.leaves().unwrap(), vec![a4_hash, a1_hash, b1_hash]);
+ assert_eq!(displaced.displaced_leaves, vec![]);
+ assert_eq!(displaced.displaced_blocks, vec![]);
+ }
+
+ // If branch of b blocks is higher in number than a branch, we
+ // should still not prune disconnected leafs.
+ // g -> a1 -> <unimported> -> a3(f) -> a4
+ // \-> b1 -> b2 ----------> b3 ----> b4 -> b5
+ let b2_number = 2;
+ let b2_hash =
+ insert_disconnected_header(&backend, b2_number, b1_hash, H256::from([40; 32]), false);
+ let b3_number = 3;
+ let b3_hash =
+ insert_disconnected_header(&backend, b3_number, b2_hash, H256::from([41; 32]), false);
+ let b4_number = 4;
+ let b4_hash =
+ insert_disconnected_header(&backend, b4_number, b3_hash, H256::from([42; 32]), false);
+ let b5_number = 5;
+ let b5_hash =
+ insert_disconnected_header(&backend, b5_number, b4_hash, H256::from([43; 32]), false);
+ {
+ let displaced =
+ blockchain.displaced_leaves_after_finalizing(a3_hash, a3_number).unwrap();
+ assert_eq!(blockchain.leaves().unwrap(), vec![b5_hash, a4_hash, a1_hash]);
+ assert_eq!(displaced.displaced_leaves, vec![]);
+ assert_eq!(displaced.displaced_blocks, vec![]);
+ }
+
+ // Even though there is a disconnect, diplace should still detect
+ // branches above the block gap.
+ // /-> c4
+ // g -> a1 -> <unimported> -> a3 -> a4(f)
+ // \-> b1 -> b2 ----------> b3 -> b4 -> b5
+ let c4_number = 4;
+ let c4_hash =
+ insert_disconnected_header(&backend, c4_number, a3_hash, H256::from([44; 32]), false);
+ {
+ let displaced =
+ blockchain.displaced_leaves_after_finalizing(a4_hash, a4_number).unwrap();
+ assert_eq!(blockchain.leaves().unwrap(), vec![b5_hash, a4_hash, c4_hash, a1_hash]);
+ assert_eq!(displaced.displaced_leaves, vec![(c4_number, c4_hash)]);
+ assert_eq!(displaced.displaced_blocks, vec![c4_hash]);
+ }
+ }
#[test]
fn displaced_leaves_after_finalizing_works() {
let backend = Backend::<Block>::new_test(1000, 100);
@@ -3156,6 +3302,15 @@ pub(crate) mod tests {
assert_eq!(displaced_a3.displaced_leaves, vec![]);
assert_eq!(displaced_a3.displaced_blocks, vec![]);
}
+ {
+ // Finalized block is above leaves and not imported yet.
+ // We will not be able to make a connection,
+ // nothing can be marked as displaced.
+ let displaced =
+ blockchain.displaced_leaves_after_finalizing(H256::from([57; 32]), 10).unwrap();
+ assert_eq!(displaced.displaced_leaves, vec![]);
+ assert_eq!(displaced.displaced_blocks, vec![]);
+ }
// fork from genesis: 2 prong.
let b1_number = 1;
diff --git a/substrate/primitives/blockchain/Cargo.toml b/substrate/primitives/blockchain/Cargo.toml
index aedd720612c33d35722eca2f3815f562e15f23c0..bd0daaf63c055934ab99c4087eb7d27db8ae41c1 100644
--- a/substrate/primitives/blockchain/Cargo.toml
+++ b/substrate/primitives/blockchain/Cargo.toml
@@ -19,7 +19,6 @@ targets = ["x86_64-unknown-linux-gnu"]
[dependencies]
codec = { features = ["derive"], workspace = true }
futures = { workspace = true }
-log = { workspace = true, default-features = true }
parking_lot = { workspace = true, default-features = true }
schnellru = { workspace = true }
thiserror = { workspace = true }
@@ -29,3 +28,4 @@ sp-consensus = { workspace = true, default-features = true }
sp-database = { workspace = true, default-features = true }
sp-runtime = { workspace = true, default-features = true }
sp-state-machine = { workspace = true, default-features = true }
+tracing = { workspace = true, default-features = true }
diff --git a/substrate/primitives/blockchain/src/backend.rs b/substrate/primitives/blockchain/src/backend.rs
index a928217d5885454b704c527354cabb5ba49af923..2accd4dad12c5bc579408ae432a5bf579ea8d57c 100644
--- a/substrate/primitives/blockchain/src/backend.rs
+++ b/substrate/primitives/blockchain/src/backend.rs
@@ -17,7 +17,6 @@
//! Substrate blockchain trait
-use log::warn;
use parking_lot::RwLock;
use sp_runtime::{
generic::BlockId,
@@ -25,6 +24,7 @@ use sp_runtime::{
Justifications,
};
use std::collections::{btree_set::BTreeSet, HashMap, VecDeque};
+use tracing::{debug, warn};
use crate::{
error::{Error, Result},
@@ -228,6 +228,7 @@ pub trait Backend<Block: BlockT>:
//
// FIXME #1558 only issue this warning when not on a dead fork
warn!(
+ target: crate::LOG_TARGET,
"Block {:?} exists in chain but not found when following all leaves backwards",
base_hash,
);
@@ -254,16 +255,35 @@ pub trait Backend<Block: BlockT>:
) -> std::result::Result<DisplacedLeavesAfterFinalization<Block>, Error> {
let leaves = self.leaves()?;
+ debug!(
+ target: crate::LOG_TARGET,
+ ?leaves,
+ %finalized_block_hash,
+ ?finalized_block_number,
+ "Checking for displaced leaves after finalization."
+ );
+
// 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())
}
- // Store hashes of finalized blocks for quick checking later, the last block if the
+ // Store hashes of finalized blocks for quick checking later, the last block is the
// finalized one
let mut finalized_chain = VecDeque::new();
- finalized_chain
- .push_front(MinimalBlockMetadata::from(&self.header_metadata(finalized_block_hash)?));
+ let current_finalized = match self.header_metadata(finalized_block_hash) {
+ Ok(metadata) => metadata,
+ Err(Error::UnknownBlock(_)) => {
+ debug!(
+ target: crate::LOG_TARGET,
+ hash = ?finalized_block_hash,
+ "Tried to fetch unknown block, block ancestry has gaps."
+ );
+ return Ok(DisplacedLeavesAfterFinalization::default());
+ },
+ Err(e) => Err(e)?,
+ };
+ finalized_chain.push_front(MinimalBlockMetadata::from(¤t_finalized));
// Local cache is a performance optimization in case of finalized block deep below the
// tip of the chain with a lot of leaves above finalized block
@@ -273,6 +293,7 @@ pub trait Backend<Block: BlockT>:
displaced_leaves: Vec::with_capacity(leaves.len()),
displaced_blocks: Vec::with_capacity(leaves.len()),
};
+
let mut displaced_blocks_candidates = Vec::new();
for leaf_hash in leaves {
@@ -306,11 +327,11 @@ pub trait Backend<Block: BlockT>:
continue;
}
- // Otherwise the whole leaf branch needs to be pruned, track it all the way to the
- // point of branching from the finalized chain
- result.displaced_leaves.push((leaf_number, leaf_hash));
- result.displaced_blocks.extend(displaced_blocks_candidates.drain(..));
- result.displaced_blocks.push(current_header_metadata.hash);
+ // We reuse `displaced_blocks_candidates` to store the current metadata.
+ // This block is not displaced if there is a gap in the ancestry. We
+ // check for this gap later.
+ displaced_blocks_candidates.push(current_header_metadata.hash);
+
// Collect the rest of the displaced blocks of leaf branch
for distance_from_finalized in 1_u32.. {
// Find block at `distance_from_finalized` from finalized block
@@ -318,9 +339,22 @@ pub trait Backend<Block: BlockT>:
match finalized_chain.iter().rev().nth(distance_from_finalized as usize) {
Some(header) => (header.number, header.hash),
None => {
- let metadata = MinimalBlockMetadata::from(&self.header_metadata(
- finalized_chain.front().expect("Not empty; qed").parent,
- )?);
+ let to_fetch = finalized_chain.front().expect("Not empty; qed");
+ let metadata = match self.header_metadata(to_fetch.parent) {
+ Ok(metadata) => metadata,
+ Err(Error::UnknownBlock(_)) => {
+ debug!(
+ target: crate::LOG_TARGET,
+ distance_from_finalized,
+ hash = ?to_fetch.parent,
+ number = ?to_fetch.number,
+ "Tried to fetch unknown block, block ancestry has gaps."
+ );
+ break;
+ },
+ Err(e) => Err(e)?,
+ };
+ let metadata = MinimalBlockMetadata::from(&metadata);
let result = (metadata.number, metadata.hash);
finalized_chain.push_front(metadata);
result
@@ -336,11 +370,13 @@ pub trait Backend<Block: BlockT>:
let parent_hash = current_header_metadata.parent;
if finalized_chain_block_hash == parent_hash {
// Reached finalized chain, nothing left to do
+ result.displaced_blocks.extend(displaced_blocks_candidates.drain(..));
+ result.displaced_leaves.push((leaf_number, leaf_hash));
break;
}
// Store displaced block and look deeper for block on finalized chain
- result.displaced_blocks.push(parent_hash);
+ displaced_blocks_candidates.push(parent_hash);
current_header_metadata =
MinimalBlockMetadata::from(&self.header_metadata(parent_hash)?);
}
diff --git a/substrate/primitives/blockchain/src/lib.rs b/substrate/primitives/blockchain/src/lib.rs
index eabbbcf50d9f208c19d76ef81c8736e314e61a71..305b7f6afec198f260a9dbc331fb573612fb74c0 100644
--- a/substrate/primitives/blockchain/src/lib.rs
+++ b/substrate/primitives/blockchain/src/lib.rs
@@ -24,3 +24,5 @@ mod header_metadata;
pub use backend::*;
pub use error::*;
pub use header_metadata::*;
+
+const LOG_TARGET: &str = "db::blockchain";