From 81004eabfd47f94f4cd2a70454d510e0a94b6c41 Mon Sep 17 00:00:00 2001
From: Nikolay Volf <nikvolf@gmail.com>
Date: Thu, 23 Jan 2020 05:41:22 -0800
Subject: [PATCH] Refactor and test spec block rules (#4670)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* Refactor and test spec block rules
* address review
* Update client/src/block_rules.rs
Co-Authored-By: André Silva <andre.beat@gmail.com>
Co-authored-by: André Silva <andre.beat@gmail.com>
---
substrate/bin/node/testing/src/client.rs | 1 +
substrate/client/block-builder/src/lib.rs | 2 -
substrate/client/src/block_rules.rs | 72 +++++++++
substrate/client/src/client.rs | 142 +++++++++++++++---
substrate/client/src/lib.rs | 1 +
substrate/test-utils/client/src/lib.rs | 41 +++--
.../test-utils/runtime/client/src/lib.rs | 7 +-
7 files changed, 225 insertions(+), 41 deletions(-)
create mode 100644 substrate/client/src/block_rules.rs
diff --git a/substrate/bin/node/testing/src/client.rs b/substrate/bin/node/testing/src/client.rs
index 140d1cbfc33..1dddd8ba5ae 100644
--- a/substrate/bin/node/testing/src/client.rs
+++ b/substrate/bin/node/testing/src/client.rs
@@ -57,6 +57,7 @@ pub trait TestClientBuilderExt: Sized {
}
impl TestClientBuilderExt for substrate_test_client::TestClientBuilder<
+ node_primitives::Block,
sc_client::LocalCallExecutor<Backend, Executor>,
Backend,
GenesisParameters,
diff --git a/substrate/client/block-builder/src/lib.rs b/substrate/client/block-builder/src/lib.rs
index 9b403dead44..d0eb8b28926 100644
--- a/substrate/client/block-builder/src/lib.rs
+++ b/substrate/client/block-builder/src/lib.rs
@@ -197,8 +197,6 @@ where
)?;
let parent_hash = self.parent_hash;
- // The unsafe is required because the consume requires that we drop/consume the inner api
- // (what we do here).
let storage_changes = self.api.into_storage_changes(
&state,
changes_trie_state.as_ref(),
diff --git a/substrate/client/src/block_rules.rs b/substrate/client/src/block_rules.rs
new file mode 100644
index 00000000000..e5614511817
--- /dev/null
+++ b/substrate/client/src/block_rules.rs
@@ -0,0 +1,72 @@
+// Copyright 2020 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate. If not, see <http://www.gnu.org/licenses/>.
+
+//! Client fixed chain specification rules
+
+use std::collections::{HashMap, HashSet};
+
+use sp_runtime::{
+ traits::{Block as BlockT, NumberFor},
+};
+
+use sc_client_api::{ForkBlocks, BadBlocks};
+
+/// Chain specification rules lookup result.
+pub enum LookupResult<B: BlockT> {
+ /// Specification rules do not contain any special rules about this block
+ NotSpecial,
+ /// The bock is known to be bad and should not be imported
+ KnownBad,
+ /// There is a specified canonical block hash for the given height
+ Expected(B::Hash)
+}
+
+/// Chain-specific block filtering rules.
+///
+/// This holds known bad blocks and known good forks, and
+/// is usually part of the chain spec.
+pub struct BlockRules<B: BlockT> {
+ bad: HashSet<B::Hash>,
+ forks: HashMap<NumberFor<B>, B::Hash>,
+}
+
+impl<B: BlockT> BlockRules<B> {
+ /// New block rules with provided black and white lists.
+ pub fn new(
+ fork_blocks: ForkBlocks<B>,
+ bad_blocks: BadBlocks<B>,
+ ) -> Self {
+ Self {
+ bad: bad_blocks.unwrap_or(HashSet::new()),
+ forks: fork_blocks.unwrap_or(vec![]).into_iter().collect(),
+ }
+ }
+
+ /// Check if there's any rule affecting the given block.
+ pub fn lookup(&self, number: NumberFor<B>, hash: &B::Hash) -> LookupResult<B> {
+ if let Some(hash_for_height) = self.forks.get(&number) {
+ if hash_for_height != hash {
+ return LookupResult::Expected(hash_for_height.clone());
+ }
+ }
+
+ if self.bad.contains(hash) {
+ return LookupResult::KnownBad;
+ }
+
+ LookupResult::NotSpecial
+ }
+}
diff --git a/substrate/client/src/client.rs b/substrate/client/src/client.rs
index 26b077277f8..a3bbf84f7d7 100644
--- a/substrate/client/src/client.rs
+++ b/substrate/client/src/client.rs
@@ -82,7 +82,7 @@ use sp_blockchain::Error;
use crate::{
call_executor::LocalCallExecutor,
light::{call_executor::prove_execution, fetcher::ChangesProof},
- in_mem, genesis, cht,
+ in_mem, genesis, cht, block_rules::{BlockRules, LookupResult as BlockLookupResult},
};
/// Substrate Client
@@ -94,8 +94,7 @@ pub struct Client<B, E, Block, RA> where Block: BlockT {
finality_notification_sinks: Mutex<Vec<mpsc::UnboundedSender<FinalityNotification<Block>>>>,
// holds the block hash currently being imported. TODO: replace this with block queue
importing_block: RwLock<Option<Block::Hash>>,
- fork_blocks: ForkBlocks<Block>,
- bad_blocks: BadBlocks<Block>,
+ block_rules: BlockRules<Block>,
execution_extensions: ExecutionExtensions<Block>,
_phantom: PhantomData<RA>,
}
@@ -219,8 +218,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
import_notification_sinks: Default::default(),
finality_notification_sinks: Default::default(),
importing_block: Default::default(),
- fork_blocks,
- bad_blocks,
+ block_rules: BlockRules::new(fork_blocks, bad_blocks),
execution_extensions,
_phantom: Default::default(),
})
@@ -1551,32 +1549,25 @@ impl<B, E, Block, RA> sp_consensus::BlockImport<Block> for &Client<B, E, Block,
// Check the block against white and black lists if any are defined
// (i.e. fork blocks and bad blocks respectively)
- let fork_block = self.fork_blocks.as_ref()
- .and_then(|fs| fs.iter().find(|(n, _)| *n == number));
-
- if let Some((_, h)) = fork_block {
- if &hash != h {
+ match self.block_rules.lookup(number, &hash) {
+ BlockLookupResult::KnownBad => {
+ trace!(
+ "Rejecting known bad block: #{} {:?}",
+ number,
+ hash,
+ );
+ return Ok(ImportResult::KnownBad);
+ },
+ BlockLookupResult::Expected(expected_hash) => {
trace!(
"Rejecting block from known invalid fork. Got {:?}, expected: {:?} at height {}",
hash,
- h,
+ expected_hash,
number
);
return Ok(ImportResult::KnownBad);
- }
- }
-
- let bad_block = self.bad_blocks.as_ref()
- .filter(|bs| bs.contains(&hash))
- .is_some();
-
- if bad_block {
- trace!(
- "Rejecting known bad block: #{} {:?}",
- number,
- hash,
- );
- return Ok(ImportResult::KnownBad);
+ },
+ BlockLookupResult::NotSpecial => {}
}
// Own status must be checked first. If the block and ancestry is pruned
@@ -3009,6 +3000,107 @@ pub(crate) mod tests {
);
}
+
+ #[test]
+ fn respects_block_rules() {
+
+ fn run_test(
+ record_only: bool,
+ known_bad: &mut HashSet<H256>,
+ fork_rules: &mut Vec<(u64, H256)>,
+ ) {
+ let mut client = if record_only {
+ TestClientBuilder::new().build()
+ } else {
+ TestClientBuilder::new()
+ .set_block_rules(
+ Some(fork_rules.clone()),
+ Some(known_bad.clone()),
+ )
+ .build()
+ };
+
+ let block_ok = client.new_block_at(&BlockId::Number(0), Default::default(), false)
+ .unwrap().build().unwrap().block;
+
+ let params = BlockCheckParams {
+ hash: block_ok.hash().clone(),
+ number: 0,
+ parent_hash: block_ok.header().parent_hash().clone(),
+ allow_missing_state: false,
+ import_existing: false,
+ };
+ assert_eq!(client.check_block(params).unwrap(), ImportResult::imported(false));
+
+ // this is 0x0d6d6612a10485370d9e085aeea7ec427fb3f34d961c6a816cdbe5cde2278864
+ let mut block_not_ok = client.new_block_at(&BlockId::Number(0), Default::default(), false)
+ .unwrap();
+ block_not_ok.push_storage_change(vec![0], Some(vec![1])).unwrap();
+ let block_not_ok = block_not_ok.build().unwrap().block;
+
+ let params = BlockCheckParams {
+ hash: block_not_ok.hash().clone(),
+ number: 0,
+ parent_hash: block_not_ok.header().parent_hash().clone(),
+ allow_missing_state: false,
+ import_existing: false,
+ };
+ if record_only {
+ known_bad.insert(block_not_ok.hash());
+ } else {
+ assert_eq!(client.check_block(params).unwrap(), ImportResult::KnownBad);
+ }
+
+ // Now going to the fork
+ client.import_as_final(BlockOrigin::Own, block_ok).unwrap();
+
+ // And check good fork
+ let mut block_ok = client.new_block_at(&BlockId::Number(1), Default::default(), false)
+ .unwrap();
+ block_ok.push_storage_change(vec![0], Some(vec![2])).unwrap();
+ let block_ok = block_ok.build().unwrap().block;
+
+ let params = BlockCheckParams {
+ hash: block_ok.hash().clone(),
+ number: 1,
+ parent_hash: block_ok.header().parent_hash().clone(),
+ allow_missing_state: false,
+ import_existing: false,
+ };
+ if record_only {
+ fork_rules.push((1, block_ok.hash().clone()));
+ }
+ assert_eq!(client.check_block(params).unwrap(), ImportResult::imported(false));
+
+ // And now try bad fork
+ let mut block_not_ok = client.new_block_at(&BlockId::Number(1), Default::default(), false)
+ .unwrap();
+ block_not_ok.push_storage_change(vec![0], Some(vec![3])).unwrap();
+ let block_not_ok = block_not_ok.build().unwrap().block;
+
+ let params = BlockCheckParams {
+ hash: block_not_ok.hash().clone(),
+ number: 1,
+ parent_hash: block_not_ok.header().parent_hash().clone(),
+ allow_missing_state: false,
+ import_existing: false,
+ };
+
+ if !record_only {
+ assert_eq!(client.check_block(params).unwrap(), ImportResult::KnownBad);
+ }
+ }
+
+ let mut known_bad = HashSet::new();
+ let mut fork_rules = Vec::new();
+
+ // records what bad_blocks and fork_blocks hashes should be
+ run_test(true, &mut known_bad, &mut fork_rules);
+
+ // enforces rules and actually makes assertions
+ run_test(false, &mut known_bad, &mut fork_rules);
+ }
+
#[test]
fn returns_status_for_pruned_blocks() {
let _ = env_logger::try_init();
diff --git a/substrate/client/src/lib.rs b/substrate/client/src/lib.rs
index 8aa71c5ec5c..4caabfa201f 100644
--- a/substrate/client/src/lib.rs
+++ b/substrate/client/src/lib.rs
@@ -83,6 +83,7 @@ pub mod light;
pub mod leaves;
mod call_executor;
mod client;
+mod block_rules;
pub use sc_client_api::{
blockchain,
diff --git a/substrate/test-utils/client/src/lib.rs b/substrate/test-utils/client/src/lib.rs
index 8603b26d50a..76aa1acd8a9 100644
--- a/substrate/test-utils/client/src/lib.rs
+++ b/substrate/test-utils/client/src/lib.rs
@@ -21,7 +21,10 @@
pub mod client_ext;
pub use sc_client::{blockchain, self};
-pub use sc_client_api::execution_extensions::{ExecutionStrategies, ExecutionExtensions};
+pub use sc_client_api::{
+ execution_extensions::{ExecutionStrategies, ExecutionExtensions},
+ ForkBlocks, BadBlocks,
+};
pub use sc_client_db::{Backend, self};
pub use sp_consensus;
pub use sc_executor::{NativeExecutor, WasmExecutionMethod, self};
@@ -33,7 +36,6 @@ pub use sp_keyring::{
pub use sp_core::{Blake2Hasher, traits::BareCryptoStorePtr};
pub use sp_runtime::{Storage, StorageChild};
pub use sp_state_machine::ExecutionStrategy;
-
pub use self::client_ext::{ClientExt, ClientBlockImportExt};
use std::sync::Arc;
@@ -61,23 +63,25 @@ impl GenesisInit for () {
}
/// A builder for creating a test client instance.
-pub struct TestClientBuilder<Executor, Backend, G: GenesisInit> {
+pub struct TestClientBuilder<Block: BlockT, Executor, Backend, G: GenesisInit> {
execution_strategies: ExecutionStrategies,
genesis_init: G,
child_storage_extension: HashMap<Vec<u8>, StorageChild>,
backend: Arc<Backend>,
_executor: std::marker::PhantomData<Executor>,
keystore: Option<BareCryptoStorePtr>,
+ fork_blocks: ForkBlocks<Block>,
+ bad_blocks: BadBlocks<Block>,
}
impl<Block: BlockT, Executor, G: GenesisInit> Default
- for TestClientBuilder<Executor, Backend<Block>, G> {
+ for TestClientBuilder<Block, Executor, Backend<Block>, G> {
fn default() -> Self {
Self::with_default_backend()
}
}
-impl<Block: BlockT, Executor, G: GenesisInit> TestClientBuilder<Executor, Backend<Block>, G> {
+impl<Block: BlockT, Executor, G: GenesisInit> TestClientBuilder<Block, Executor, Backend<Block>, G> {
/// Create new `TestClientBuilder` with default backend.
pub fn with_default_backend() -> Self {
let backend = Arc::new(Backend::new_test(std::u32::MAX, std::u64::MAX));
@@ -91,7 +95,7 @@ impl<Block: BlockT, Executor, G: GenesisInit> TestClientBuilder<Executor, Backen
}
}
-impl<Executor, Backend, G: GenesisInit> TestClientBuilder<Executor, Backend, G> {
+impl<Block: BlockT, Executor, Backend, G: GenesisInit> TestClientBuilder<Block, Executor, Backend, G> {
/// Create a new instance of the test client builder.
pub fn with_backend(backend: Arc<Backend>) -> Self {
TestClientBuilder {
@@ -101,6 +105,8 @@ impl<Executor, Backend, G: GenesisInit> TestClientBuilder<Executor, Backend, G>
genesis_init: Default::default(),
_executor: Default::default(),
keystore: None,
+ fork_blocks: None,
+ bad_blocks: None,
}
}
@@ -152,8 +158,18 @@ impl<Executor, Backend, G: GenesisInit> TestClientBuilder<Executor, Backend, G>
self
}
+ /// Sets custom block rules.
+ pub fn set_block_rules(mut self,
+ fork_blocks: ForkBlocks<Block>,
+ bad_blocks: BadBlocks<Block>,
+ ) -> Self {
+ self.fork_blocks = fork_blocks;
+ self.bad_blocks = bad_blocks;
+ self
+ }
+
/// Build the test client with the given native executor.
- pub fn build_with_executor<Block, RuntimeApi>(
+ pub fn build_with_executor<RuntimeApi>(
self,
executor: Executor,
) -> (
@@ -167,7 +183,6 @@ impl<Executor, Backend, G: GenesisInit> TestClientBuilder<Executor, Backend, G>
) where
Executor: sc_client::CallExecutor<Block> + 'static,
Backend: sc_client_api::backend::Backend<Block>,
- Block: BlockT,
{
let storage = {
@@ -191,8 +206,8 @@ impl<Executor, Backend, G: GenesisInit> TestClientBuilder<Executor, Backend, G>
self.backend.clone(),
executor,
&storage,
- Default::default(),
- Default::default(),
+ self.fork_blocks,
+ self.bad_blocks,
ExecutionExtensions::new(
self.execution_strategies,
self.keystore.clone(),
@@ -205,13 +220,14 @@ impl<Executor, Backend, G: GenesisInit> TestClientBuilder<Executor, Backend, G>
}
}
-impl<E, Backend, G: GenesisInit> TestClientBuilder<
+impl<Block: BlockT, E, Backend, G: GenesisInit> TestClientBuilder<
+ Block,
sc_client::LocalCallExecutor<Backend, NativeExecutor<E>>,
Backend,
G,
> {
/// Build the test client with the given native executor.
- pub fn build_with_native_executor<Block, RuntimeApi, I>(
+ pub fn build_with_native_executor<RuntimeApi, I>(
self,
executor: I,
) -> (
@@ -226,7 +242,6 @@ impl<E, Backend, G: GenesisInit> TestClientBuilder<
I: Into<Option<NativeExecutor<E>>>,
E: sc_executor::NativeExecutionDispatch + 'static,
Backend: sc_client_api::backend::Backend<Block> + 'static,
- Block: BlockT,
{
let executor = executor.into().unwrap_or_else(||
NativeExecutor::new(WasmExecutionMethod::Interpreted, None)
diff --git a/substrate/test-utils/runtime/client/src/lib.rs b/substrate/test-utils/runtime/client/src/lib.rs
index 7646f4a9602..21cf94dfa67 100644
--- a/substrate/test-utils/runtime/client/src/lib.rs
+++ b/substrate/test-utils/runtime/client/src/lib.rs
@@ -140,7 +140,12 @@ impl substrate_test_client::GenesisInit for GenesisParameters {
}
/// A `TestClient` with `test-runtime` builder.
-pub type TestClientBuilder<E, B> = substrate_test_client::TestClientBuilder<E, B, GenesisParameters>;
+pub type TestClientBuilder<E, B> = substrate_test_client::TestClientBuilder<
+ substrate_test_runtime::Block,
+ E,
+ B,
+ GenesisParameters,
+>;
/// Test client type with `LocalExecutor` and generic Backend.
pub type Client<B> = sc_client::Client<
--
GitLab