From b14238cb7d63bc69fbbca4e3436fb8c323469995 Mon Sep 17 00:00:00 2001
From: Davide Galassi <davxy@datawok.net>
Date: Wed, 7 Jun 2023 11:17:45 +0200
Subject: [PATCH] Finalized should be before best (#14308)
* Finalized block should not be after best block
* Remove unwrap
* Apply code review suggestion
Co-authored-by: Koute <koute@users.noreply.github.com>
* Add test
---------
Co-authored-by: Koute <koute@users.noreply.github.com>
---
substrate/client/service/src/client/client.rs | 54 ++++++++++---------
.../client/service/test/src/client/mod.rs | 38 +++++++++++++
2 files changed, 67 insertions(+), 25 deletions(-)
diff --git a/substrate/client/service/src/client/client.rs b/substrate/client/service/src/client/client.rs
index 91c59cdbac8..3b1a526154d 100644
--- a/substrate/client/service/src/client/client.rs
+++ b/substrate/client/service/src/client/client.rs
@@ -52,7 +52,7 @@ use sp_api::{
};
use sp_blockchain::{
self as blockchain, Backend as ChainBackend, CachedHeaderMetadata, Error,
- HeaderBackend as ChainHeaderBackend, HeaderMetadata,
+ HeaderBackend as ChainHeaderBackend, HeaderMetadata, Info as BlockchainInfo,
};
use sp_consensus::{BlockOrigin, BlockStatus, Error as ConsensusError};
@@ -717,7 +717,7 @@ where
operation,
parent_hash,
None,
- info.best_hash,
+ &info,
make_notifications,
)?;
}
@@ -907,52 +907,56 @@ where
fn apply_finality_with_block_hash(
&self,
operation: &mut ClientImportOperation<Block, B>,
- block: Block::Hash,
+ hash: Block::Hash,
justification: Option<Justification>,
- best_block: Block::Hash,
+ info: &BlockchainInfo<Block>,
notify: bool,
) -> sp_blockchain::Result<()> {
- // find tree route from last finalized to given block.
- let last_finalized = self.backend.blockchain().last_finalized()?;
-
- if block == last_finalized {
+ if hash == info.finalized_hash {
warn!(
"Possible safety violation: attempted to re-finalize last finalized block {:?} ",
- last_finalized
+ hash,
);
return Ok(())
}
+ // Find tree route from last finalized to given block.
let route_from_finalized =
- sp_blockchain::tree_route(self.backend.blockchain(), last_finalized, block)?;
+ sp_blockchain::tree_route(self.backend.blockchain(), info.finalized_hash, hash)?;
if let Some(retracted) = route_from_finalized.retracted().get(0) {
warn!(
"Safety violation: attempted to revert finalized block {:?} which is not in the \
same chain as last finalized {:?}",
- retracted, last_finalized
+ retracted, info.finalized_hash
);
return Err(sp_blockchain::Error::NotInFinalizedChain)
}
- // If there is only one leaf, best block is guaranteed to be
- // a descendant of the new finalized block. If not,
- // we need to check.
- if self.backend.blockchain().leaves()?.len() > 1 {
+ // We may need to coercively update the best block if there is more than one
+ // leaf or if the finalized block number is greater than last best number recorded
+ // by the backend. This last condition may apply in case of consensus implementations
+ // not always checking this condition.
+ let block_number = self
+ .backend
+ .blockchain()
+ .number(hash)?
+ .ok_or(Error::MissingHeader(format!("{hash:?}")))?;
+ if self.backend.blockchain().leaves()?.len() > 1 || info.best_number < block_number {
let route_from_best =
- sp_blockchain::tree_route(self.backend.blockchain(), best_block, block)?;
+ sp_blockchain::tree_route(self.backend.blockchain(), info.best_hash, hash)?;
- // if the block is not a direct ancestor of the current best chain,
+ // If the block is not a direct ancestor of the current best chain,
// then some other block is the common ancestor.
- if route_from_best.common_block().hash != block {
+ if route_from_best.common_block().hash != hash {
// NOTE: we're setting the finalized block as best block, this might
// be slightly inaccurate since we might have a "better" block
// further along this chain, but since best chain selection logic is
// plugable we cannot make a better choice here. usages that need
// an accurate "best" block need to go through `SelectChain`
// instead.
- operation.op.mark_head(block)?;
+ operation.op.mark_head(hash)?;
}
}
@@ -962,8 +966,8 @@ where
operation.op.mark_finalized(finalize_new.hash, None)?;
}
- assert_eq!(enacted.last().map(|e| e.hash), Some(block));
- operation.op.mark_finalized(block, justification)?;
+ assert_eq!(enacted.last().map(|e| e.hash), Some(hash));
+ operation.op.mark_finalized(hash, justification)?;
if notify {
let finalized =
@@ -985,7 +989,7 @@ where
let header = self
.backend
.blockchain()
- .header(block)?
+ .header(hash)?
.expect("Block to finalize expected to be onchain; qed");
operation.notify_finalized = Some(FinalizeSummary { header, finalized, stale_heads });
@@ -1129,7 +1133,7 @@ where
}
/// Get blockchain info.
- pub fn chain_info(&self) -> blockchain::Info<Block> {
+ pub fn chain_info(&self) -> BlockchainInfo<Block> {
self.backend.blockchain().info()
}
@@ -1896,8 +1900,8 @@ where
justification: Option<Justification>,
notify: bool,
) -> sp_blockchain::Result<()> {
- let last_best = self.backend.blockchain().info().best_hash;
- self.apply_finality_with_block_hash(operation, hash, justification, last_best, notify)
+ let info = self.backend.blockchain().info();
+ self.apply_finality_with_block_hash(operation, hash, justification, &info, notify)
}
fn finalize_block(
diff --git a/substrate/client/service/test/src/client/mod.rs b/substrate/client/service/test/src/client/mod.rs
index e71e211767f..9c490a47a3b 100644
--- a/substrate/client/service/test/src/client/mod.rs
+++ b/substrate/client/service/test/src/client/mod.rs
@@ -2001,3 +2001,41 @@ fn use_dalek_ext_works() {
.verify_ed25519(a1.hash(), zero_ed_sig(), zero_ed_pub(), vec![])
.unwrap());
}
+
+#[test]
+fn finalize_after_best_block_updates_best() {
+ let mut client = substrate_test_runtime_client::new();
+
+ // G -> A1
+ let a1 = client.new_block(Default::default()).unwrap().build().unwrap().block;
+ block_on(client.import(BlockOrigin::Own, a1.clone())).unwrap();
+
+ // A1 -> A2
+ let a2 = client
+ .new_block_at(a1.hash(), Default::default(), false)
+ .unwrap()
+ .build()
+ .unwrap()
+ .block;
+ block_on(client.import(BlockOrigin::Own, a2.clone())).unwrap();
+
+ // A2 -> A3
+ let a3 = client
+ .new_block_at(a2.hash(), Default::default(), false)
+ .unwrap()
+ .build()
+ .unwrap()
+ .block;
+ let (header, extrinsics) = a3.clone().deconstruct();
+ let mut import_params = BlockImportParams::new(BlockOrigin::Own, header);
+ import_params.body = Some(extrinsics);
+ import_params.fork_choice = Some(ForkChoiceStrategy::Custom(false));
+ block_on(client.import_block(import_params)).unwrap();
+
+ assert_eq!(client.chain_info().best_hash, a2.hash());
+
+ client.finalize_block(a3.hash(), None).unwrap();
+
+ assert_eq!(client.chain_info().finalized_hash, a3.hash());
+ assert_eq!(client.chain_info().best_hash, a3.hash());
+}
--
GitLab