From e432248af0af703818ff7e6a5dd4478c31115ee2 Mon Sep 17 00:00:00 2001 From: Andreas Doerr Date: Tue, 13 Oct 2020 12:23:58 +0200 Subject: [PATCH] Add ChainTime associated type (#410) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * add HeaderTimestamp associated type * use Header Timestamp * rename HeaderTimestamp to ChainTime * add unit test * deal with clippy * Apply suggestions from code review Commit review suggestions Co-authored-by: Tomasz Drwięga * code review * cargo fmt * get rid of additional test runtime * unit test asserts against concrete import context Co-authored-by: Tomasz Drwięga --- bridges/bin/rialto/runtime/src/kovan.rs | 14 +++- bridges/bin/rialto/runtime/src/lib.rs | 2 + bridges/bin/rialto/runtime/src/rialto_poa.rs | 14 +++- bridges/modules/ethereum/Cargo.toml | 1 + bridges/modules/ethereum/src/error.rs | 3 + bridges/modules/ethereum/src/import.rs | 24 +++++-- bridges/modules/ethereum/src/lib.rs | 24 +++++++ bridges/modules/ethereum/src/mock.rs | 14 +++- bridges/modules/ethereum/src/verification.rs | 74 +++++++++++++++++--- 9 files changed, 154 insertions(+), 16 deletions(-) diff --git a/bridges/bin/rialto/runtime/src/kovan.rs b/bridges/bin/rialto/runtime/src/kovan.rs index d0b59478d22ea..bc2252bbb9594 100644 --- a/bridges/bin/rialto/runtime/src/kovan.rs +++ b/bridges/bin/rialto/runtime/src/kovan.rs @@ -21,7 +21,8 @@ use bp_header_chain::BaseHeaderChain; use frame_support::RuntimeDebug; use hex_literal::hex; use pallet_bridge_eth_poa::{ - AuraConfiguration, PruningStrategy as BridgePruningStrategy, ValidatorsConfiguration, ValidatorsSource, + AuraConfiguration, ChainTime as TChainTime, PruningStrategy as BridgePruningStrategy, ValidatorsConfiguration, + ValidatorsSource, }; use sp_std::prelude::*; @@ -134,6 +135,17 @@ impl BridgePruningStrategy for PruningStrategy { } } +/// PoA Header timestamp verification against `Timestamp` pallet. +#[derive(Default, RuntimeDebug)] +pub struct ChainTime; + +impl TChainTime for ChainTime { + fn is_timestamp_ahead(&self, timestamp: u64) -> bool { + let now = super::Timestamp::now(); + timestamp > now + } +} + /// The Kovan Blockchain as seen by the runtime. pub struct KovanBlockchain; diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs index a693e8897397a..77a23cbf8f4ad 100644 --- a/bridges/bin/rialto/runtime/src/lib.rs +++ b/bridges/bin/rialto/runtime/src/lib.rs @@ -234,6 +234,7 @@ impl pallet_bridge_eth_poa::Trait for Runtime { type FinalityVotesCachingInterval = rialto_poa::FinalityVotesCachingInterval; type ValidatorsConfiguration = rialto_poa::BridgeValidatorsConfiguration; type PruningStrategy = rialto_poa::PruningStrategy; + type ChainTime = rialto_poa::ChainTime; type OnHeadersSubmitted = (); } @@ -243,6 +244,7 @@ impl pallet_bridge_eth_poa::Trait for Runtime { type FinalityVotesCachingInterval = kovan::FinalityVotesCachingInterval; type ValidatorsConfiguration = kovan::BridgeValidatorsConfiguration; type PruningStrategy = kovan::PruningStrategy; + type ChainTime = kovan::ChainTime; type OnHeadersSubmitted = (); } diff --git a/bridges/bin/rialto/runtime/src/rialto_poa.rs b/bridges/bin/rialto/runtime/src/rialto_poa.rs index 4839579b04e05..324179b55d88c 100644 --- a/bridges/bin/rialto/runtime/src/rialto_poa.rs +++ b/bridges/bin/rialto/runtime/src/rialto_poa.rs @@ -23,7 +23,8 @@ use bp_header_chain::BaseHeaderChain; use frame_support::RuntimeDebug; use hex_literal::hex; use pallet_bridge_eth_poa::{ - AuraConfiguration, PruningStrategy as TPruningStrategy, ValidatorsConfiguration, ValidatorsSource, + AuraConfiguration, ChainTime as TChainTime, PruningStrategy as TPruningStrategy, ValidatorsConfiguration, + ValidatorsSource, }; use sp_std::prelude::*; @@ -109,6 +110,17 @@ impl TPruningStrategy for PruningStrategy { } } +/// ChainTime provider +#[derive(Default)] +pub struct ChainTime; + +impl TChainTime for ChainTime { + fn is_timestamp_ahead(&self, timestamp: u64) -> bool { + let now = super::Timestamp::now(); + timestamp > now + } +} + /// The Rialto PoA Blockchain as seen by the runtime. pub struct RialtoBlockchain; diff --git a/bridges/modules/ethereum/Cargo.toml b/bridges/modules/ethereum/Cargo.toml index eb8393f4905ae..83be6d79639a4 100644 --- a/bridges/modules/ethereum/Cargo.toml +++ b/bridges/modules/ethereum/Cargo.toml @@ -26,6 +26,7 @@ sp-std = { version = "2.0", default-features = false } [dev-dependencies] libsecp256k1 = { version = "0.3.4", features = ["hmac"] } +hex-literal = "0.3" [features] default = ["std"] diff --git a/bridges/modules/ethereum/src/error.rs b/bridges/modules/ethereum/src/error.rs index 0a9ca57f8c135..50dccd6ea2ced 100644 --- a/bridges/modules/ethereum/src/error.rs +++ b/bridges/modules/ethereum/src/error.rs @@ -62,6 +62,8 @@ pub enum Error { UnsignedTooFarInTheFuture = 19, /// Trying to finalize sibling of finalized block. TryingToFinalizeSibling = 20, + /// Header timestamp is ahead of on-chain timestamp + HeaderTimestampIsAhead = 21, } impl Error { @@ -88,6 +90,7 @@ impl Error { Error::TransactionsReceiptsMismatch => "Invalid transactions receipts provided", Error::UnsignedTooFarInTheFuture => "The unsigned header is too far in future", Error::TryingToFinalizeSibling => "Trying to finalize sibling of finalized block", + Error::HeaderTimestampIsAhead => "Header timestamp is ahead of on-chain timestamp", } } diff --git a/bridges/modules/ethereum/src/import.rs b/bridges/modules/ethereum/src/import.rs index 977ffd6718159..1b41c3a8b20ad 100644 --- a/bridges/modules/ethereum/src/import.rs +++ b/bridges/modules/ethereum/src/import.rs @@ -18,7 +18,7 @@ use crate::error::Error; use crate::finality::finalize_blocks; use crate::validators::{Validators, ValidatorsConfiguration}; use crate::verification::{is_importable_header, verify_aura_header}; -use crate::{AuraConfiguration, ChangeToEnact, PruningStrategy, Storage}; +use crate::{AuraConfiguration, ChainTime, ChangeToEnact, PruningStrategy, Storage}; use bp_eth_poa::{AuraHeader, HeaderId, Receipt}; use sp_std::{collections::btree_map::BTreeMap, prelude::*}; @@ -31,13 +31,16 @@ use sp_std::{collections::btree_map::BTreeMap, prelude::*}; /// we have NOT imported. /// Returns error if fatal error has occured during import. Some valid headers may be /// imported in this case. -pub fn import_headers( +/// TODO: update me (https://github.com/paritytech/parity-bridges-common/issues/415) +#[allow(clippy::too_many_arguments)] +pub fn import_headers( storage: &mut S, pruning_strategy: &mut PS, aura_config: &AuraConfiguration, validators_config: &ValidatorsConfiguration, submitter: Option, headers: Vec<(AuraHeader, Option>)>, + chain_time: &CT, finalized_headers: &mut BTreeMap, ) -> Result<(u64, u64), Error> { let mut useful = 0; @@ -50,6 +53,7 @@ pub fn import_headers( validators_config, submitter.clone(), header, + chain_time, receipts, ); @@ -79,20 +83,23 @@ pub type FinalizedHeaders = Vec<(HeaderId, Option<::Submitter>) /// has returned true. /// /// Returns imported block id and list of all finalized headers. -pub fn import_header( +/// TODO: update me (https://github.com/paritytech/parity-bridges-common/issues/415) +#[allow(clippy::too_many_arguments)] +pub fn import_header( storage: &mut S, pruning_strategy: &mut PS, aura_config: &AuraConfiguration, validators_config: &ValidatorsConfiguration, submitter: Option, header: AuraHeader, + chain_time: &CT, receipts: Option>, ) -> Result<(HeaderId, FinalizedHeaders), Error> { // first check that we are able to import this header at all let (header_id, finalized_id) = is_importable_header(storage, &header)?; // verify header - let import_context = verify_aura_header(storage, aura_config, submitter, &header)?; + let import_context = verify_aura_header(storage, aura_config, submitter, &header, chain_time)?; // check if block schedules new validators let validators = Validators::new(validators_config); @@ -195,6 +202,7 @@ mod tests { &test_validators_config(), None, Default::default(), + &(), None, ), Err(Error::AncientHeader), @@ -215,6 +223,7 @@ mod tests { &test_validators_config(), None, header.clone(), + &(), None, ) .map(|_| ()), @@ -228,6 +237,7 @@ mod tests { &test_validators_config(), None, header, + &(), None, ) .map(|_| ()), @@ -254,6 +264,7 @@ mod tests { &validators_config, None, header, + &(), None ) .map(|_| ()), @@ -291,6 +302,7 @@ mod tests { &validators_config, Some(100), header, + &(), None, ) .unwrap(); @@ -320,6 +332,7 @@ mod tests { &validators_config, Some(101), header11.clone(), + &(), Some(vec![validators_change_receipt(latest_block_id.hash)]), ) .unwrap(); @@ -345,6 +358,7 @@ mod tests { &validators_config, Some(102), header, + &(), None, ) .unwrap(); @@ -374,6 +388,7 @@ mod tests { &validators_config, Some(103), header, + &(), None, ) .unwrap(); @@ -404,6 +419,7 @@ mod tests { )), None, header, + &(), None, ) .map(|_| id) diff --git a/bridges/modules/ethereum/src/lib.rs b/bridges/modules/ethereum/src/lib.rs index afc5c681a39a4..2328864957ad0 100644 --- a/bridges/modules/ethereum/src/lib.rs +++ b/bridges/modules/ethereum/src/lib.rs @@ -326,6 +326,25 @@ pub trait PruningStrategy: Default { fn pruning_upper_bound(&mut self, best_number: u64, best_finalized_number: u64) -> u64; } +/// ChainTime represents the runtime on-chain time +pub trait ChainTime: Default { + /// Is a header timestamp ahead of the current on-chain time. + /// + /// Check whether `timestamp` is ahead (i.e greater than) the current on-chain + /// time. If so, return `true`, `false` otherwise. + fn is_timestamp_ahead(&self, timestamp: u64) -> bool; +} + +/// ChainTime implementation for the empty type. +/// +/// This implementation will allow a runtime without the timestamp pallet to use +/// the empty type as its ChainTime associated type. +impl ChainTime for () { + fn is_timestamp_ahead(&self, _: u64) -> bool { + false + } +} + /// Callbacks for header submission rewards/penalties. pub trait OnHeadersSubmitted { /// Called when valid headers have been submitted. @@ -366,6 +385,8 @@ pub trait Trait: frame_system::Trait { type FinalityVotesCachingInterval: Get>; /// Headers pruning strategy. type PruningStrategy: PruningStrategy; + /// Header timestamp verification against current on-chain time. + type ChainTime: ChainTime; /// Handler for headers submission result. type OnHeadersSubmitted: OnHeadersSubmitted; @@ -385,6 +406,7 @@ decl_module! { &T::ValidatorsConfiguration::get(), None, header, + &T::ChainTime::default(), receipts, ).map_err(|e| e.msg())?; } @@ -406,6 +428,7 @@ decl_module! { &T::ValidatorsConfiguration::get(), Some(submitter.clone()), headers_with_receipts, + &T::ChainTime::default(), &mut finalized_headers, ); @@ -531,6 +554,7 @@ impl, I: Instance> frame_support::unsigned::ValidateUnsigned for Mod &T::ValidatorsConfiguration::get(), &pool_configuration(), header, + &T::ChainTime::default(), receipts.as_ref(), ); diff --git a/bridges/modules/ethereum/src/mock.rs b/bridges/modules/ethereum/src/mock.rs index 2c46c7c5407d9..1255cc671962b 100644 --- a/bridges/modules/ethereum/src/mock.rs +++ b/bridges/modules/ethereum/src/mock.rs @@ -18,7 +18,7 @@ pub use crate::test_utils::{insert_header, validator_utils::*, validators_change pub use bp_eth_poa::signatures::secret_to_address; use crate::validators::{ValidatorsConfiguration, ValidatorsSource}; -use crate::{AuraConfiguration, GenesisConfig, PruningStrategy, Trait}; +use crate::{AuraConfiguration, ChainTime, GenesisConfig, PruningStrategy, Trait}; use bp_eth_poa::{Address, AuraHeader, H256, U256}; use frame_support::{impl_outer_origin, parameter_types, weights::Weight}; use secp256k1::SecretKey; @@ -83,6 +83,7 @@ impl Trait for TestRuntime { type ValidatorsConfiguration = TestValidatorsConfiguration; type FinalityVotesCachingInterval = TestFinalityVotesCachingInterval; type PruningStrategy = KeepSomeHeadersBehindBest; + type ChainTime = ConstChainTime; type OnHeadersSubmitted = (); } @@ -168,3 +169,14 @@ impl PruningStrategy for KeepSomeHeadersBehindBest { best_number.saturating_sub(self.0) } } + +/// Constant chain time +#[derive(Default)] +pub struct ConstChainTime; + +impl ChainTime for ConstChainTime { + fn is_timestamp_ahead(&self, timestamp: u64) -> bool { + let now = i32::max_value() as u64 / 2; + timestamp > now + } +} diff --git a/bridges/modules/ethereum/src/verification.rs b/bridges/modules/ethereum/src/verification.rs index c9592c567efe9..90d3f2a4748b6 100644 --- a/bridges/modules/ethereum/src/verification.rs +++ b/bridges/modules/ethereum/src/verification.rs @@ -16,7 +16,7 @@ use crate::error::Error; use crate::validators::{Validators, ValidatorsConfiguration}; -use crate::{AuraConfiguration, ImportContext, PoolConfiguration, ScheduledChange, Storage}; +use crate::{AuraConfiguration, ChainTime, ImportContext, PoolConfiguration, ScheduledChange, Storage}; use bp_eth_poa::{ public_to_address, step_validator, Address, AuraHeader, HeaderId, Receipt, SealedEmptyStep, H256, H520, U128, U256, }; @@ -46,19 +46,20 @@ pub fn is_importable_header(storage: &S, header: &AuraHeader) -> Res /// Try accept unsigned aura header into transaction pool. /// /// Returns required and provided tags. -pub fn accept_aura_header_into_pool( +pub fn accept_aura_header_into_pool( storage: &S, config: &AuraConfiguration, validators_config: &ValidatorsConfiguration, pool_config: &PoolConfiguration, header: &AuraHeader, + chain_time: &CT, receipts: Option<&Vec>, ) -> Result<(Vec, Vec), Error> { // check if we can verify further let (header_id, _) = is_importable_header(storage, header)?; // we can always do contextless checks - contextless_checks(config, header)?; + contextless_checks(config, header, chain_time)?; // we want to avoid having same headers twice in the pool // => we're strict about receipts here - if we need them, we require receipts to be Some, @@ -153,14 +154,15 @@ pub fn accept_aura_header_into_pool( } /// Verify header by Aura rules. -pub fn verify_aura_header( +pub fn verify_aura_header( storage: &S, config: &AuraConfiguration, submitter: Option, header: &AuraHeader, + chain_time: &CT, ) -> Result, Error> { // let's do the lightest check first - contextless_checks(config, header)?; + contextless_checks(config, header, chain_time)?; // the rest of checks requires access to the parent header let context = storage.import_context(submitter, &header.parent_hash).ok_or_else(|| { @@ -180,7 +182,11 @@ pub fn verify_aura_header( } /// Perform basic checks that only require header itself. -fn contextless_checks(config: &AuraConfiguration, header: &AuraHeader) -> Result<(), Error> { +fn contextless_checks( + config: &AuraConfiguration, + header: &AuraHeader, + chain_time: &CT, +) -> Result<(), Error> { let expected_seal_fields = expected_header_seal_fields(config, header); if header.seal.len() != expected_seal_fields { return Err(Error::InvalidSealArity); @@ -207,6 +213,10 @@ fn contextless_checks(config: &AuraConfiguration, header: &AuraHeader) -> Result return Err(Error::TimestampOverflow); } + if chain_time.is_timestamp_ahead(header.timestamp) { + return Err(Error::HeaderTimestampIsAhead); + } + Ok(()) } @@ -358,7 +368,7 @@ mod tests { use super::*; use crate::mock::{ insert_header, run_test_with_genesis, test_aura_config, validator, validator_address, validators_addresses, - validators_change_receipt, AccountId, HeaderBuilder, TestRuntime, GAS_LIMIT, + validators_change_receipt, AccountId, ConstChainTime, HeaderBuilder, TestRuntime, GAS_LIMIT, }; use crate::validators::ValidatorsSource; use crate::DefaultInstance; @@ -366,8 +376,9 @@ mod tests { pool_configuration, BridgeStorage, FinalizedBlock, Headers, HeadersByNumber, NextValidatorsSetId, ScheduledChanges, ValidatorsSet, ValidatorsSets, }; - use bp_eth_poa::{compute_merkle_root, rlp_encode, TransactionOutcome, H520}; + use bp_eth_poa::{compute_merkle_root, rlp_encode, TransactionOutcome, H520, U256}; use frame_support::{StorageMap, StorageValue}; + use hex_literal::hex; use secp256k1::SecretKey; use sp_runtime::transaction_validity::TransactionTag; @@ -381,7 +392,7 @@ mod tests { fn verify_with_config(config: &AuraConfiguration, header: &AuraHeader) -> Result, Error> { run_test_with_genesis(genesis(), TOTAL_VALIDATORS, |_| { let storage = BridgeStorage::::new(); - verify_aura_header(&storage, &config, None, header) + verify_aura_header(&storage, &config, None, header, &ConstChainTime::default()) }) } @@ -414,6 +425,7 @@ mod tests { &validators_config, &pool_configuration(), &header, + &(), receipts.as_ref(), ) }) @@ -554,6 +566,50 @@ mod tests { assert_ne!(default_verify(&header), Err(Error::TimestampOverflow)); } + #[test] + fn verifies_chain_time() { + // expected import context after verification + let expect = ImportContext:: { + submitter: None, + parent_hash: hex!("6e41bff05578fc1db17f6816117969b07d2217f1f9039d8116a82764335991d3").into(), + parent_header: genesis(), + parent_total_difficulty: U256::zero(), + parent_scheduled_change: None, + validators_set_id: 0, + validators_set: ValidatorsSet { + validators: vec![ + hex!("dc5b20847f43d67928f49cd4f85d696b5a7617b5").into(), + hex!("897df33a7b3c62ade01e22c13d48f98124b4480f").into(), + hex!("05c987b34c6ef74e0c7e69c6e641120c24164c2d").into(), + ], + signal_block: None, + enact_block: HeaderId { + number: 0, + hash: hex!("6e41bff05578fc1db17f6816117969b07d2217f1f9039d8116a82764335991d3").into(), + }, + }, + last_signal_block: None, + }; + + // header is behind + let header = HeaderBuilder::with_parent(&genesis()) + .timestamp(i32::max_value() as u64 / 2 - 100) + .sign_by(&validator(1)); + assert_eq!(default_verify(&header).unwrap(), expect); + + // header is ahead + let header = HeaderBuilder::with_parent(&genesis()) + .timestamp(i32::max_value() as u64 / 2 + 100) + .sign_by(&validator(1)); + assert_eq!(default_verify(&header), Err(Error::HeaderTimestampIsAhead)); + + // header has same timestamp as ConstChainTime + let header = HeaderBuilder::with_parent(&genesis()) + .timestamp(i32::max_value() as u64 / 2) + .sign_by(&validator(1)); + assert_eq!(default_verify(&header).unwrap(), expect); + } + #[test] fn verifies_parent_existence() { // when there's no parent in the storage