From 6874af6ffac5a8b1b2eeb612574c6039d5b828ac Mon Sep 17 00:00:00 2001 From: Park Juhyung Date: Thu, 16 Apr 2020 14:12:43 +0900 Subject: [PATCH 1/4] Remove verify_block_seal from the ConsensusEngine The function was empty. We will make Foundry verify seal in a header soon. When verifying a seal, we should not use ConsensusEngine. Since the Tendermint ConsensusEngine uses a global lock, ConsensusEngine::verify_block_seal would not utilize multi threads. --- core/src/consensus/mod.rs | 5 ----- core/src/consensus/solo/mod.rs | 7 +------ core/src/verification/queue/kind.rs | 26 +++++--------------------- core/src/verification/queue/mod.rs | 4 ++-- core/src/verification/verification.rs | 10 +--------- 5 files changed, 9 insertions(+), 43 deletions(-) diff --git a/core/src/consensus/mod.rs b/core/src/consensus/mod.rs index 9a87c2225d..a6f440884d 100644 --- a/core/src/consensus/mod.rs +++ b/core/src/consensus/mod.rs @@ -144,11 +144,6 @@ pub trait ConsensusEngine: Sync + Send { Ok(()) } - /// Phase 2 verification. Perform costly checks such as transaction signatures. Returns either a null `Ok` or a general error detailing the problem with import. - fn verify_block_seal(&self, _header: &Header) -> Result<(), Error> { - Ok(()) - } - /// Phase 3 verification. Check block information against parent. Returns either a null `Ok` or a general error detailing the problem with import. /// The verification must be conducted only with the two headers' information because it does not guarantee whether the two corresponding bodies have been imported. fn verify_block_family(&self, _header: &Header, _parent: &Header) -> Result<(), Error> { diff --git a/core/src/consensus/solo/mod.rs b/core/src/consensus/solo/mod.rs index 633d9c1d4d..2bac4bd872 100644 --- a/core/src/consensus/solo/mod.rs +++ b/core/src/consensus/solo/mod.rs @@ -130,17 +130,12 @@ impl ConsensusEngine for Solo { mod tests { use crate::scheme::Scheme; use ctypes::Header; - use primitives::H520; #[test] fn fail_to_verify() { let engine = Scheme::new_test_solo().engine; - let mut header: Header = Header::default(); + let header: Header = Header::default(); assert!(engine.verify_header_basic(&header).is_ok()); - - header.set_seal(vec![::rlp::encode(&H520::default())]); - - assert!(engine.verify_block_seal(&header).is_ok()); } } diff --git a/core/src/verification/queue/kind.rs b/core/src/verification/queue/kind.rs index f34761fe44..0794d65aef 100644 --- a/core/src/verification/queue/kind.rs +++ b/core/src/verification/queue/kind.rs @@ -68,11 +68,7 @@ pub trait Kind: 'static + Sized + Send + Sync { fn create(input: Self::Input, engine: &dyn CodeChainEngine) -> Result; /// Attempt to verify the `Unverified` item using the given engine. - fn verify( - unverified: Self::Unverified, - engine: &dyn CodeChainEngine, - check_seal: bool, - ) -> Result; + fn verify(unverified: Self::Unverified) -> Result; fn signal() -> ClientIoMessage; } @@ -117,16 +113,8 @@ pub mod headers { Ok(input) } - fn verify( - un: Self::Unverified, - engine: &dyn CodeChainEngine, - check_seal: bool, - ) -> Result { - if check_seal { - engine.verify_block_seal(&un).map(|_| un) - } else { - Ok(un) - } + fn verify(un: Self::Unverified) -> Result { + Ok(un) } fn signal() -> ClientIoMessage { @@ -172,13 +160,9 @@ pub mod blocks { } } - fn verify( - un: Self::Unverified, - engine: &dyn CodeChainEngine, - check_seal: bool, - ) -> Result { + fn verify(un: Self::Unverified) -> Result { let hash = un.hash(); - match verify_block_seal(un.header, un.bytes, engine, check_seal) { + match verify_block_seal(un.header, un.bytes) { Ok(verified) => Ok(verified), Err(e) => { cwarn!(CLIENT, "Stage 2 block verification failed for {}: {:?}", hash, e); diff --git a/core/src/verification/queue/mod.rs b/core/src/verification/queue/mod.rs index e6622fad80..a56513a711 100644 --- a/core/src/verification/queue/mod.rs +++ b/core/src/verification/queue/mod.rs @@ -185,7 +185,7 @@ impl VerificationQueue { fn verify( verification: &Verification, - engine: &dyn CodeChainEngine, + _engine: &dyn CodeChainEngine, ready_signal: &QueueSignal, empty: &SCondvar, more_to_verify: &SCondvar, @@ -232,7 +232,7 @@ impl VerificationQueue { }; let hash = item.hash(); - let is_ready = match K::verify(item, engine, verification.check_seal) { + let is_ready = match K::verify(item) { Ok(verified) => { let mut verifying = verification.verifying.lock(); let mut idx = None; diff --git a/core/src/verification/verification.rs b/core/src/verification/verification.rs index a66b0370c1..45a42f9888 100644 --- a/core/src/verification/verification.rs +++ b/core/src/verification/verification.rs @@ -154,15 +154,7 @@ fn verify_transactions_root( /// Phase 2 verification. Perform costly checks such as transaction signatures and block nonce for ethash. /// Still operates on a individual block /// Returns a `PreverifiedBlock` structure populated with transactions -pub fn verify_block_seal( - header: Header, - bytes: Bytes, - engine: &dyn CodeChainEngine, - check_seal: bool, -) -> Result { - if check_seal { - engine.verify_block_seal(&header)?; - } +pub fn verify_block_seal(header: Header, bytes: Bytes) -> Result { let transactions: Vec<_> = BlockView::new(&bytes).transactions().into_iter().map(TryInto::try_into).collect::>()?; Ok(PreverifiedBlock { From 49be244298bb0e015d78a5b92e4d3a47d1577637 Mon Sep 17 00:00:00 2001 From: Park Juhyung Date: Thu, 16 Apr 2020 14:24:01 +0900 Subject: [PATCH 2/4] Remove unused check_seal variable The VerificationQueue does not read the variable. --- core/src/client/importer.rs | 12 +++--------- core/src/verification/queue/mod.rs | 13 +++---------- 2 files changed, 6 insertions(+), 19 deletions(-) diff --git a/core/src/client/importer.rs b/core/src/client/importer.rs index 51db44e2ec..e02b414b16 100644 --- a/core/src/client/importer.rs +++ b/core/src/client/importer.rs @@ -64,15 +64,9 @@ impl Importer { message_channel: IoChannel, miner: Arc, ) -> Result { - let block_queue = BlockQueue::new( - &config.queue, - engine.clone(), - message_channel.clone(), - config.verifier_type.verifying_seal(), - ); - - let header_queue = - HeaderQueue::new(&config.queue, engine.clone(), message_channel, config.verifier_type.verifying_seal()); + let block_queue = BlockQueue::new(&config.queue, engine.clone(), message_channel.clone()); + + let header_queue = HeaderQueue::new(&config.queue, engine.clone(), message_channel); Ok(Importer { import_lock: Mutex::new(()), diff --git a/core/src/verification/queue/mod.rs b/core/src/verification/queue/mod.rs index a56513a711..cf3b6aa4f5 100644 --- a/core/src/verification/queue/mod.rs +++ b/core/src/verification/queue/mod.rs @@ -114,12 +114,7 @@ impl QueueSignal { } impl VerificationQueue { - pub fn new( - config: &Config, - engine: Arc, - message_channel: IoChannel, - check_seal: bool, - ) -> Self { + pub fn new(config: &Config, engine: Arc, message_channel: IoChannel) -> Self { let verification = Arc::new(Verification { unverified: Mutex::new(VecDeque::new()), verifying: Mutex::new(VecDeque::new()), @@ -130,7 +125,6 @@ impl VerificationQueue { verifying: AtomicUsize::new(0), verified: AtomicUsize::new(0), }, - check_seal, more_to_verify_mutex: SMutex::new(()), }); let deleting = Arc::new(AtomicBool::new(false)); @@ -486,7 +480,6 @@ struct Verification { verified: Mutex>, bad: Mutex>, sizes: Sizes, - check_seal: bool, more_to_verify_mutex: SMutex<()>, } @@ -513,7 +506,7 @@ mod tests { let engine = scheme.engine; let config = Config::default(); - BlockQueue::new(&config, engine, IoChannel::disconnected(), true) + BlockQueue::new(&config, engine, IoChannel::disconnected()) } #[test] @@ -523,7 +516,7 @@ mod tests { let engine = scheme.engine; let config = Config::default(); - let _ = BlockQueue::new(&config, engine, IoChannel::disconnected(), true); + let _ = BlockQueue::new(&config, engine, IoChannel::disconnected()); } #[test] From 026bf274822a576c470717c09b84c2e2aaff8f7b Mon Sep 17 00:00:00 2001 From: Park Juhyung Date: Thu, 16 Apr 2020 14:39:47 +0900 Subject: [PATCH 3/4] Remove verification_type Foundry was using only the VerifierType::Canon. --- core/src/client/config.rs | 5 +---- core/src/client/importer.rs | 3 ++- core/src/verification/mod.rs | 36 ------------------------------------ 3 files changed, 3 insertions(+), 41 deletions(-) diff --git a/core/src/client/config.rs b/core/src/client/config.rs index f64b679609..e1afbb6097 100644 --- a/core/src/client/config.rs +++ b/core/src/client/config.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU Affero General Public License // along with this program. If not, see . -use crate::verification::{QueueConfig, VerifierType}; +use crate::verification::QueueConfig; use kvdb_rocksdb::CompactionProfile; use std::path::Path; use std::str::FromStr; @@ -71,8 +71,6 @@ pub struct ClientConfig { pub db_compaction: DatabaseCompactionProfile, /// State db cache-size. pub state_cache_size: usize, - /// Type of block verifier used by client. - pub verifier_type: VerifierType, } impl Default for ClientConfig { @@ -84,7 +82,6 @@ impl Default for ClientConfig { db_cache_size: Default::default(), db_compaction: Default::default(), state_cache_size: DEFAULT_STATE_CACHE_SIZE as usize * mb, - verifier_type: Default::default(), } } } diff --git a/core/src/client/importer.rs b/core/src/client/importer.rs index e02b414b16..e0a160081d 100644 --- a/core/src/client/importer.rs +++ b/core/src/client/importer.rs @@ -36,6 +36,7 @@ use std::borrow::Borrow; use std::collections::HashSet; use std::iter::FromIterator; use std::sync::Arc; +use verification::CanonVerifier; pub struct Importer { /// Lock used during block import @@ -70,7 +71,7 @@ impl Importer { Ok(Importer { import_lock: Mutex::new(()), - verifier: verification::new(config.verifier_type), + verifier: Box::new(CanonVerifier), block_queue, header_queue, miner, diff --git a/core/src/verification/mod.rs b/core/src/verification/mod.rs index 711a614ab9..1149560c24 100644 --- a/core/src/verification/mod.rs +++ b/core/src/verification/mod.rs @@ -26,39 +26,3 @@ pub use self::noop_verifier::NoopVerifier; pub use self::queue::{BlockQueue, Config as QueueConfig}; pub use self::verification::*; pub use self::verifier::Verifier; - -/// Verifier type. -#[derive(Debug, PartialEq, Clone, Copy)] -pub enum VerifierType { - /// Verifies block normally. - Canon, - /// Verifies block normally, but skips seal verification. - CanonNoSeal, - /// Does not verify block at all. - /// Used in tests. - Noop, -} - -impl VerifierType { - /// Check if seal verification is enabled for this verifier type. - pub fn verifying_seal(self) -> bool { - match self { - VerifierType::Canon => true, - VerifierType::Noop | VerifierType::CanonNoSeal => false, - } - } -} - -impl Default for VerifierType { - fn default() -> Self { - VerifierType::Canon - } -} - -/// Create a new verifier based on type. -pub fn new(v: VerifierType) -> Box { - match v { - VerifierType::Canon | VerifierType::CanonNoSeal => Box::new(CanonVerifier), - VerifierType::Noop => Box::new(NoopVerifier), - } -} From 2e6c1e8c15f9c2ffbb211ee3f0c6ff79e75527b5 Mon Sep 17 00:00:00 2001 From: Park Juhyung Date: Thu, 16 Apr 2020 14:41:22 +0900 Subject: [PATCH 4/4] Remove Verifier trait Foundry is using only CanonVerifier. We are not using the `Verifier` abstraction. --- core/src/client/importer.rs | 7 ++-- core/src/verification/canon_verifier.rs | 45 ------------------------- core/src/verification/mod.rs | 4 --- core/src/verification/noop_verifier.rs | 44 ------------------------ core/src/verification/verifier.rs | 20 ++++++++--- 5 files changed, 18 insertions(+), 102 deletions(-) delete mode 100644 core/src/verification/canon_verifier.rs delete mode 100644 core/src/verification/noop_verifier.rs diff --git a/core/src/client/importer.rs b/core/src/client/importer.rs index e0a160081d..29f213f391 100644 --- a/core/src/client/importer.rs +++ b/core/src/client/importer.rs @@ -24,7 +24,7 @@ use crate::miner::{Miner, MinerService}; use crate::service::ClientIoMessage; use crate::types::BlockId; use crate::verification::queue::{BlockQueue, HeaderQueue}; -use crate::verification::{self, PreverifiedBlock, Verifier}; +use crate::verification::{PreverifiedBlock, Verifier}; use crate::views::{BlockView, HeaderView}; use cio::IoChannel; use ctypes::header::{Header, Seal}; @@ -36,14 +36,13 @@ use std::borrow::Borrow; use std::collections::HashSet; use std::iter::FromIterator; use std::sync::Arc; -use verification::CanonVerifier; pub struct Importer { /// Lock used during block import pub import_lock: Mutex<()>, // FIXME Maybe wrap the whole `Importer` instead? /// Used to verify blocks - pub verifier: Box, + pub verifier: Verifier, /// Queue containing pending blocks pub block_queue: BlockQueue, @@ -71,7 +70,7 @@ impl Importer { Ok(Importer { import_lock: Mutex::new(()), - verifier: Box::new(CanonVerifier), + verifier: Verifier, block_queue, header_queue, miner, diff --git a/core/src/verification/canon_verifier.rs b/core/src/verification/canon_verifier.rs deleted file mode 100644 index ee4f75e2b5..0000000000 --- a/core/src/verification/canon_verifier.rs +++ /dev/null @@ -1,45 +0,0 @@ -// Copyright 2018-2020 Kodebox, Inc. -// This file is part of CodeChain. -// -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU Affero General Public License as -// published by the Free Software Foundation, either version 3 of the -// License, or (at your option) any later version. -// -// This program 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 Affero General Public License for more details. -// -// You should have received a copy of the GNU Affero General Public License -// along with this program. If not, see . - -use super::verification; -use super::Verifier; -use crate::consensus::CodeChainEngine; -use crate::error::Error; -use ctypes::{CommonParams, Header}; - -/// A canonial verifier -- this does full verification. -pub struct CanonVerifier; - -impl Verifier for CanonVerifier { - fn verify_block_family( - &self, - block: &[u8], - header: &Header, - parent: &Header, - engine: &dyn CodeChainEngine, - common_params: &CommonParams, - ) -> Result<(), Error> { - verification::verify_block_family(block, header, parent, engine, common_params) - } - - fn verify_block_final(&self, expected: &Header, got: &Header) -> Result<(), Error> { - verification::verify_block_final(expected, got) - } - - fn verify_block_external(&self, header: &Header, engine: &dyn CodeChainEngine) -> Result<(), Error> { - engine.verify_block_external(header) - } -} diff --git a/core/src/verification/mod.rs b/core/src/verification/mod.rs index 1149560c24..0c9642c63e 100644 --- a/core/src/verification/mod.rs +++ b/core/src/verification/mod.rs @@ -14,15 +14,11 @@ // You should have received a copy of the GNU Affero General Public License // along with this program. If not, see . -mod canon_verifier; -mod noop_verifier; pub mod queue; #[cfg_attr(feature = "cargo-clippy", allow(clippy::module_inception))] mod verification; mod verifier; -pub use self::canon_verifier::CanonVerifier; -pub use self::noop_verifier::NoopVerifier; pub use self::queue::{BlockQueue, Config as QueueConfig}; pub use self::verification::*; pub use self::verifier::Verifier; diff --git a/core/src/verification/noop_verifier.rs b/core/src/verification/noop_verifier.rs deleted file mode 100644 index 982e0c28e8..0000000000 --- a/core/src/verification/noop_verifier.rs +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright 2018-2020 Kodebox, Inc. -// This file is part of CodeChain. -// -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU Affero General Public License as -// published by the Free Software Foundation, either version 3 of the -// License, or (at your option) any later version. -// -// This program 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 Affero General Public License for more details. -// -// You should have received a copy of the GNU Affero General Public License -// along with this program. If not, see . - -use super::Verifier; -use crate::consensus::CodeChainEngine; -use crate::error::Error; -use ctypes::{CommonParams, Header}; - -/// A no-op verifier -- this will verify everything it's given immediately. -pub struct NoopVerifier; - -impl Verifier for NoopVerifier { - fn verify_block_family( - &self, - _block: &[u8], - _: &Header, - _t: &Header, - _: &dyn CodeChainEngine, - _common_params: &CommonParams, - ) -> Result<(), Error> { - Ok(()) - } - - fn verify_block_final(&self, _expected: &Header, _got: &Header) -> Result<(), Error> { - Ok(()) - } - - fn verify_block_external(&self, _header: &Header, _engine: &dyn CodeChainEngine) -> Result<(), Error> { - Ok(()) - } -} diff --git a/core/src/verification/verifier.rs b/core/src/verification/verifier.rs index 3bb2a8fd4e..5767fd6369 100644 --- a/core/src/verification/verifier.rs +++ b/core/src/verification/verifier.rs @@ -14,24 +14,34 @@ // You should have received a copy of the GNU Affero General Public License // along with this program. If not, see . +use super::verification; use crate::consensus::CodeChainEngine; use crate::error::Error; use ctypes::{CommonParams, Header}; /// Should be used to verify blocks. -pub trait Verifier: Send + Sync { +pub struct Verifier; + +impl Verifier { /// Verify a block relative to its parent and uncles. - fn verify_block_family( + pub fn verify_block_family( &self, block: &[u8], header: &Header, parent: &Header, engine: &dyn CodeChainEngine, common_params: &CommonParams, - ) -> Result<(), Error>; + ) -> Result<(), Error> { + verification::verify_block_family(block, header, parent, engine, common_params) + } /// Do a final verification check for an enacted header vs its expected counterpart. - fn verify_block_final(&self, expected: &Header, got: &Header) -> Result<(), Error>; + pub fn verify_block_final(&self, expected: &Header, got: &Header) -> Result<(), Error> { + verification::verify_block_final(expected, got) + } + /// Verify a block, inspecting external state. - fn verify_block_external(&self, header: &Header, engine: &dyn CodeChainEngine) -> Result<(), Error>; + pub fn verify_block_external(&self, header: &Header, engine: &dyn CodeChainEngine) -> Result<(), Error> { + engine.verify_block_external(header) + } }