From 8eacdb54de2089e32f07aa79a2314ee1cc8049d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Sun, 18 Aug 2019 18:05:16 +0200 Subject: [PATCH] client: refuse to import blocks that revert finality (#3432) * client: don't import blocks that revert finality * client: test import of blocks that revert finality * client: replace tempdir with tempfile in test --- substrate/Cargo.lock | 1 + substrate/core/client/Cargo.toml | 1 + substrate/core/client/src/client.rs | 129 ++++++++++++++++++++++++++-- 3 files changed, 124 insertions(+), 7 deletions(-) diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index cebf379d89..fc3be28b20 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -4366,6 +4366,7 @@ dependencies = [ "substrate-telemetry 2.0.0", "substrate-test-runtime-client 2.0.0", "substrate-trie 2.0.0", + "tempfile 3.1.0 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] diff --git a/substrate/core/client/Cargo.toml b/substrate/core/client/Cargo.toml index d917427a50..49524e3be4 100644 --- a/substrate/core/client/Cargo.toml +++ b/substrate/core/client/Cargo.toml @@ -29,6 +29,7 @@ sr-api-macros = { path = "../sr-api-macros" } [dev-dependencies] env_logger = "0.6" +tempfile = "3.1" test-client = { package = "substrate-test-runtime-client", path = "../test-runtime/client" } kvdb-memorydb = { git = "https://github.com/paritytech/parity-common", rev="b0317f649ab2c665b7987b8475878fc4d2e1f81d" } diff --git a/substrate/core/client/src/client.rs b/substrate/core/client/src/client.rs index 0360c532d2..c6234dc8d1 100644 --- a/substrate/core/client/src/client.rs +++ b/substrate/core/client/src/client.rs @@ -916,10 +916,26 @@ impl Client where blockchain::BlockStatus::Unknown => {}, } - let (last_best, last_best_number) = { - let info = self.backend.blockchain().info(); - (info.best_hash, info.best_number) - }; + let info = self.backend.blockchain().info(); + + // the block is lower than our last finalized block so it must revert + // finality, refusing import. + if *import_headers.post().number() <= info.finalized_number { + return Err(error::Error::NotInFinalizedChain); + } + + // find tree route from last finalized to given block. + let route_from_finalized = crate::blockchain::tree_route( + self.backend.blockchain(), + BlockId::Hash(info.finalized_hash), + BlockId::Hash(parent_hash), + )?; + + // the block being imported retracts the last finalized block, refusing to + // import. + if !route_from_finalized.retracted().is_empty() { + return Err(error::Error::NotInFinalizedChain); + } // this is a fairly arbitrary choice of where to draw the line on making notifications, // but the general goal is to only make notifications when we are already fully synced @@ -934,16 +950,23 @@ impl Client where // ensure parent block is finalized to maintain invariant that // finality is called sequentially. if finalized { - self.apply_finality_with_block_hash(operation, parent_hash, None, last_best, make_notifications)?; + self.apply_finality_with_block_hash(operation, parent_hash, None, info.best_hash, make_notifications)?; } // FIXME #1232: correct path logic for when to execute this function - let (storage_update,changes_update,storage_changes) = self.block_execution(&operation.op, &import_headers, origin, hash, body.clone())?; + let (storage_update, changes_update, storage_changes) = self.block_execution( + &operation.op, + &import_headers, + origin, + hash, + body.clone(), + )?; let is_new_best = finalized || match fork_choice { - ForkChoiceStrategy::LongestChain => import_headers.post().number() > &last_best_number, + ForkChoiceStrategy::LongestChain => import_headers.post().number() > &info.best_number, ForkChoiceStrategy::Custom(v) => v, }; + let leaf_state = if finalized { crate::backend::NewBlockState::Final } else if is_new_best { @@ -1871,6 +1894,7 @@ pub(crate) mod tests { use test_client::{ prelude::*, client::backend::Backend as TestBackend, + client_db::{Backend, DatabaseSettings, PruningMode}, runtime::{self, Block, Transfer, RuntimeApi, TestAPI}, }; @@ -2790,4 +2814,95 @@ pub(crate) mod tests { client.set_head(BlockId::hash(b1.hash())).unwrap(); assert_eq!(950, current_balance()); } + + #[test] + fn doesnt_import_blocks_that_revert_finality() { + let _ = env_logger::try_init(); + let tmp = tempfile::tempdir().unwrap(); + + // we need to run with archive pruning to avoid pruning non-canonical + // states + let backend = Arc::new(Backend::new( + DatabaseSettings { + cache_size: None, + state_cache_size: 1 << 20, + state_cache_child_ratio: None, + path: tmp.path().into(), + pruning: PruningMode::ArchiveAll, + }, + u64::max_value(), + ).unwrap()); + + let client = TestClientBuilder::with_backend(backend).build(); + + // -> C1 + // / + // G -> A1 -> A2 + // \ + // -> B1 -> B2 -> B3 + + let a1 = client.new_block_at(&BlockId::Number(0), Default::default()) + .unwrap().bake().unwrap(); + client.import(BlockOrigin::Own, a1.clone()).unwrap(); + + let a2 = client.new_block_at(&BlockId::Hash(a1.hash()), Default::default()) + .unwrap().bake().unwrap(); + client.import(BlockOrigin::Own, a2.clone()).unwrap(); + + let mut b1 = client.new_block_at(&BlockId::Number(0), Default::default()).unwrap(); + + // needed to make sure B1 gets a different hash from A1 + b1.push_transfer(Transfer { + from: AccountKeyring::Alice.into(), + to: AccountKeyring::Ferdie.into(), + amount: 1, + nonce: 0, + }).unwrap(); + let b1 = b1.bake().unwrap(); + client.import(BlockOrigin::Own, b1.clone()).unwrap(); + + let b2 = client.new_block_at(&BlockId::Hash(b1.hash()), Default::default()) + .unwrap().bake().unwrap(); + client.import(BlockOrigin::Own, b2.clone()).unwrap(); + + // we will finalize A2 which should make it impossible to import a new + // B3 at the same height but that doesnt't include it + client.finalize_block(BlockId::Hash(a2.hash()), None, false).unwrap(); + + let b3 = client.new_block_at(&BlockId::Hash(b2.hash()), Default::default()) + .unwrap().bake().unwrap(); + + let import_err = client.import(BlockOrigin::Own, b3).err().unwrap(); + let expected_err = ConsensusError::ClientImport( + error::Error::NotInFinalizedChain.to_string() + ); + + assert_eq!( + import_err.to_string(), + expected_err.to_string(), + ); + + // adding a C1 block which is lower than the last finalized should also + // fail (with a cheaper check that doesn't require checking ancestry). + let mut c1 = client.new_block_at(&BlockId::Number(0), Default::default()).unwrap(); + + // needed to make sure C1 gets a different hash from A1 and B1 + c1.push_transfer(Transfer { + from: AccountKeyring::Alice.into(), + to: AccountKeyring::Ferdie.into(), + amount: 2, + nonce: 0, + }).unwrap(); + let c1 = c1.bake().unwrap(); + + let import_err = client.import(BlockOrigin::Own, c1).err().unwrap(); + let expected_err = ConsensusError::ClientImport( + error::Error::NotInFinalizedChain.to_string() + ); + + assert_eq!( + import_err.to_string(), + expected_err.to_string(), + ); + } }