diff --git a/Cargo.lock b/Cargo.lock index 7e6736743d539..6771530327c8f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3421,6 +3421,7 @@ dependencies = [ "substrate-consensus-common 0.1.0", "substrate-inherents 0.1.0", "substrate-primitives 0.1.0", + "substrate-test-client 0.1.0", "substrate-transaction-pool 0.1.0", ] diff --git a/core/basic-authorship/Cargo.toml b/core/basic-authorship/Cargo.toml index e7f6bf7351a32..82187db8031f8 100644 --- a/core/basic-authorship/Cargo.toml +++ b/core/basic-authorship/Cargo.toml @@ -14,3 +14,6 @@ consensus_common = { package = "substrate-consensus-common", path = "../../core/ primitives = { package = "substrate-primitives", path = "../../core/primitives" } inherents = { package = "substrate-inherents", path = "../inherents" } transaction_pool = { package = "substrate-transaction-pool", path = "../../core/transaction-pool" } + +[dev-dependencies] +test-client = { package = "substrate-test-client", path = "../../core/test-client" } diff --git a/core/basic-authorship/src/basic_authorship.rs b/core/basic-authorship/src/basic_authorship.rs index aa46919224437..41625273cb35d 100644 --- a/core/basic-authorship/src/basic_authorship.rs +++ b/core/basic-authorship/src/basic_authorship.rs @@ -17,28 +17,26 @@ //! A consensus proposer for "basic" chains which use the primitive inherent-data. // FIXME #1021 move this into substrate-consensus-common -// -use std::{sync::Arc, self}; +// +use std::{self, time, sync::Arc}; -use log::{info, trace}; +use log::{info, debug, trace}; use client::{ self, error, Client as SubstrateClient, CallExecutor, block_builder::api::BlockBuilder as BlockBuilderApi, runtime_api::{Core, ApiExt} }; -use codec::{Decode, Encode}; +use codec::Decode; use consensus_common::{self, evaluation}; use primitives::{H256, Blake2Hasher}; use runtime_primitives::traits::{ Block as BlockT, Hash as HashT, Header as HeaderT, ProvideRuntimeApi, AuthorityIdFor }; use runtime_primitives::generic::BlockId; +use runtime_primitives::ApplyError; use transaction_pool::txpool::{self, Pool as TransactionPool}; use inherents::InherentData; -// block size limit. -const MAX_TRANSACTIONS_SIZE: usize = 4 * 1024 * 1024; - /// Build new blocks. pub trait BlockBuilder { /// Push an extrinsic onto the block. Fails if the extrinsic is invalid. @@ -144,6 +142,7 @@ impl consensus_common::Environment<::Block> for Propose parent_id: id, parent_number: *parent_header.number(), transaction_pool: self.transaction_pool.clone(), + now: Box::new(time::Instant::now), }; Ok(proposer) @@ -157,6 +156,7 @@ pub struct Proposer { parent_id: BlockId, parent_number: <::Header as HeaderT>::Number, transaction_pool: Arc>, + now: Box time::Instant>, } impl consensus_common::Proposer<::Block> for Proposer where @@ -169,10 +169,12 @@ impl consensus_common::Proposer<::Block> for Pro type Create = Result<::Block, error::Error>; type Error = error::Error; - fn propose(&self, inherent_data: InherentData) + fn propose(&self, inherent_data: InherentData, max_duration: time::Duration) -> Result<::Block, error::Error> { - self.propose_with(inherent_data) + // leave some time for evaluation and block finalisation (33%) + let deadline = (self.now)() + max_duration - max_duration / 3; + self.propose_with(inherent_data, deadline) } } @@ -183,7 +185,7 @@ impl Proposer where A: txpool::ChainApi, client::error::Error: From<::Error>, { - fn propose_with(&self, inherent_data: InherentData) + fn propose_with(&self, inherent_data: InherentData, deadline: time::Instant) -> Result<::Block, error::Error> { use runtime_primitives::traits::BlakeTwo256; @@ -193,16 +195,21 @@ impl Proposer where inherent_data, |block_builder| { let mut unqueue_invalid = Vec::new(); - let mut pending_size = 0; let pending_iterator = self.transaction_pool.ready(); for pending in pending_iterator { - let encoded_size = pending.data.encode().len(); - if pending_size + encoded_size >= MAX_TRANSACTIONS_SIZE { break } + if (self.now)() > deadline { + debug!("Consensus deadline reached when pushing block transactions, proceeding with proposing."); + break; + } match block_builder.push_extrinsic(pending.data.clone()) { Ok(()) => { - pending_size += encoded_size; + debug!("[{:?}] Pushed to the block.", pending.hash); + } + Err(error::Error(error::ErrorKind::ApplyExtrinsicFailed(ApplyError::FullBlock), _)) => { + debug!("Block is full, proceed with proposing."); + break; } Err(e) => { trace!(target: "transaction-pool", "Invalid transaction: {}", e); @@ -237,3 +244,60 @@ impl Proposer where Ok(substrate_block) } } + +#[cfg(test)] +mod tests { + use super::*; + + use codec::Encode; + use std::cell::RefCell; + use consensus_common::{Environment, Proposer}; + use test_client::keyring::Keyring; + use test_client::{self, runtime::{Extrinsic, Transfer}}; + + fn extrinsic(nonce: u64) -> Extrinsic { + let tx = Transfer { + amount: Default::default(), + nonce, + from: Keyring::Alice.to_raw_public().into(), + to: Default::default(), + }; + let signature = Keyring::from_raw_public(tx.from.to_fixed_bytes()).unwrap().sign(&tx.encode()).into(); + Extrinsic::Transfer(tx, signature) + } + + #[test] + fn should_cease_building_block_when_deadline_is_reached() { + // given + let client = Arc::new(test_client::new()); + let chain_api = transaction_pool::ChainApi::new(client.clone()); + let txpool = Arc::new(TransactionPool::new(Default::default(), chain_api)); + + txpool.submit_at(&BlockId::number(0), vec![extrinsic(0), extrinsic(1)]).unwrap(); + + let proposer_factory = ProposerFactory { + client: client.clone(), + transaction_pool: txpool.clone(), + }; + + let mut proposer = proposer_factory.init( + &client.header(&BlockId::number(0)).unwrap().unwrap(), + &[] + ).unwrap(); + + // when + let cell = RefCell::new(time::Instant::now()); + proposer.now = Box::new(move || { + let new = *cell.borrow() + time::Duration::from_secs(2); + cell.replace(new) + }); + let deadline = time::Duration::from_secs(3); + let block = proposer.propose(Default::default(), deadline).unwrap(); + + // then + // block should have some extrinsics although we have some more in the pool. + assert_eq!(block.extrinsics().len(), 1); + assert_eq!(txpool.ready().count(), 2); + } + +} diff --git a/core/client/src/block_builder/block_builder.rs b/core/client/src/block_builder/block_builder.rs index 4df12d94e2afb..3f1ed5aa14ff9 100644 --- a/core/client/src/block_builder/block_builder.rs +++ b/core/client/src/block_builder/block_builder.rs @@ -76,9 +76,9 @@ where }) } - /// Push onto the block's list of extrinsics. This will ensure the extrinsic - /// can be validly executed (by executing it); if it is invalid, it'll be returned along with - /// the error. Otherwise, it will return a mutable reference to self (in order to chain). + /// Push onto the block's list of extrinsics. + /// + /// This will ensure the extrinsic can be validly executed (by executing it); pub fn push(&mut self, xt: ::Extrinsic) -> error::Result<()> { use crate::runtime_api::ApiExt; diff --git a/core/consensus/aura/src/lib.rs b/core/consensus/aura/src/lib.rs index 38b5ce53a4460..a531bd8bba1c8 100644 --- a/core/consensus/aura/src/lib.rs +++ b/core/consensus/aura/src/lib.rs @@ -48,7 +48,7 @@ use inherents::{InherentDataProviders, InherentData, RuntimeString}; use futures::{Stream, Future, IntoFuture, future::{self, Either}}; use tokio::timer::Timeout; use slots::Slots; -use ::log::{warn, debug, log, info, trace}; +use ::log::{warn, debug, info, trace}; use srml_aura::{ InherentType as AuraInherent, AuraInherentData, @@ -291,10 +291,9 @@ pub fn start_aura( }; let remaining_duration = slot_info.remaining_duration(); - // deadline our production to approx. the end of the - // slot + // deadline our production to approx. the end of the slot Timeout::new( - proposer.propose(slot_info.inherent_data).into_future(), + proposer.propose(slot_info.inherent_data, remaining_duration).into_future(), remaining_duration, ) } else { @@ -710,7 +709,7 @@ mod tests { type Error = Error; type Create = Result; - fn propose(&self, _: InherentData) -> Result { + fn propose(&self, _: InherentData, _: Duration) -> Result { self.1.new_block().unwrap().bake().map_err(|e| e.into()) } } diff --git a/core/consensus/common/src/evaluation.rs b/core/consensus/common/src/evaluation.rs index a122017806cf9..c0fdcc45f1af2 100644 --- a/core/consensus/common/src/evaluation.rs +++ b/core/consensus/common/src/evaluation.rs @@ -16,7 +16,7 @@ //! Block evaluation and evaluation errors. -use super::MAX_TRANSACTIONS_SIZE; +use super::MAX_BLOCK_SIZE; use parity_codec::Encode; use runtime_primitives::traits::{Block as BlockT, Header as HeaderT, As}; @@ -43,7 +43,7 @@ error_chain! { description("Proposal exceeded the maximum size."), display( "Proposal exceeded the maximum size of {} by {} bytes.", - MAX_TRANSACTIONS_SIZE, size.saturating_sub(MAX_TRANSACTIONS_SIZE) + MAX_BLOCK_SIZE, size.saturating_sub(MAX_BLOCK_SIZE) ), } } @@ -61,12 +61,8 @@ pub fn evaluate_initial( let proposal = Block::decode(&mut &encoded[..]) .ok_or_else(|| ErrorKind::BadProposalFormat)?; - let transactions_size = proposal.extrinsics().iter().fold(0, |a, tx| { - a + Encode::encode(tx).len() - }); - - if transactions_size > MAX_TRANSACTIONS_SIZE { - bail!(ErrorKind::ProposalTooLarge(transactions_size)) + if encoded.len() > MAX_BLOCK_SIZE { + bail!(ErrorKind::ProposalTooLarge(encoded.len())) } if *parent_hash != *proposal.header().parent_hash() { diff --git a/core/consensus/common/src/lib.rs b/core/consensus/common/src/lib.rs index 73278ca5790fe..ccbafdb86369a 100644 --- a/core/consensus/common/src/lib.rs +++ b/core/consensus/common/src/lib.rs @@ -27,6 +27,7 @@ #![recursion_limit="128"] use std::sync::Arc; +use std::time::Duration; use runtime_primitives::generic::BlockId; use runtime_primitives::traits::{AuthorityIdFor, Block}; @@ -40,7 +41,7 @@ pub mod import_queue; pub mod evaluation; // block size limit. -const MAX_TRANSACTIONS_SIZE: usize = 4 * 1024 * 1024; +const MAX_BLOCK_SIZE: usize = 4 * 1024 * 1024 + 512; pub use self::error::{Error, ErrorKind}; pub use block_import::{BlockImport, JustificationImport, ImportBlock, BlockOrigin, ImportResult, ForkChoiceStrategy}; @@ -76,7 +77,7 @@ pub trait Proposer { /// Future that resolves to a committed proposal. type Create: IntoFuture; /// Create a proposal. - fn propose(&self, inherent_data: InherentData) -> Self::Create; + fn propose(&self, inherent_data: InherentData, max_duration: Duration) -> Self::Create; } /// An oracle for when major synchronization work is being undertaken. diff --git a/core/primitives/src/storage.rs b/core/primitives/src/storage.rs index 0a14b14756b12..76ea307905996 100644 --- a/core/primitives/src/storage.rs +++ b/core/primitives/src/storage.rs @@ -71,6 +71,9 @@ pub mod well_known_keys { /// Current extrinsic index (u32) is stored under this key. pub const EXTRINSIC_INDEX: &'static [u8] = b":extrinsic_index"; + /// Sum of all lengths of executed extrinsics (u32). + pub const ALL_EXTRINSICS_LEN: &'static [u8] = b":all_extrinsics_len"; + /// Changes trie configuration is stored under this key. pub const CHANGES_TRIE_CONFIG: &'static [u8] = b":changes_trie"; diff --git a/core/sr-primitives/src/lib.rs b/core/sr-primitives/src/lib.rs index 10ba97a6cc772..224fa6c991c66 100644 --- a/core/sr-primitives/src/lib.rs +++ b/core/sr-primitives/src/lib.rs @@ -70,6 +70,7 @@ use traits::{Verify, Lazy}; /// A String that is a `&'static str` on `no_std` and a `Cow<'static, str>` on `std`. #[cfg(feature = "std")] pub type RuntimeString = ::std::borrow::Cow<'static, str>; +/// A String that is a `&'static str` on `no_std` and a `Cow<'static, str>` on `std`. #[cfg(not(feature = "std"))] pub type RuntimeString = &'static str; @@ -297,6 +298,8 @@ pub enum ApplyError { Future = 2, /// Sending account had too low a balance. CantPay = 3, + /// Block is full, no more extrinsics can be applied. + FullBlock = 255, } impl codec::Encode for ApplyError { diff --git a/core/test-runtime/src/lib.rs b/core/test-runtime/src/lib.rs index 260beb39d5dd1..a19c966969441 100644 --- a/core/test-runtime/src/lib.rs +++ b/core/test-runtime/src/lib.rs @@ -226,7 +226,7 @@ impl_runtime_apis! { } fn inherent_extrinsics(_data: InherentData) -> Vec<::Extrinsic> { - unimplemented!() + vec![] } fn check_inherents(_block: Block, _data: InherentData) -> CheckInherentsResult { diff --git a/core/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.compact.wasm b/core/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.compact.wasm index 00c4d4ce9f0f7..24b6ef4ef6720 100644 Binary files a/core/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.compact.wasm and b/core/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.compact.wasm differ diff --git a/node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm b/node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm index 01f64762a5e60..2d410ff28cddd 100644 Binary files a/node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm and b/node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm differ diff --git a/srml/council/src/lib.rs b/srml/council/src/lib.rs index 60320c0738dc1..c630c587d32ce 100644 --- a/srml/council/src/lib.rs +++ b/srml/council/src/lib.rs @@ -28,7 +28,6 @@ extern crate hex_literal; extern crate parity_codec as codec; #[macro_use] extern crate parity_codec_derive; extern crate substrate_primitives; -#[cfg_attr(not(feature = "std"), macro_use)] extern crate sr_std as rstd; extern crate sr_io as runtime_io; #[macro_use] extern crate srml_support; diff --git a/srml/democracy/src/lib.rs b/srml/democracy/src/lib.rs index 6f9bc4630657c..f37e1e22340a7 100644 --- a/srml/democracy/src/lib.rs +++ b/srml/democracy/src/lib.rs @@ -23,7 +23,6 @@ extern crate substrate_primitives; #[macro_use] extern crate parity_codec_derive; -#[cfg_attr(not(feature = "std"), macro_use)] extern crate sr_std as rstd; #[macro_use] extern crate srml_support; diff --git a/srml/executive/src/lib.rs b/srml/executive/src/lib.rs index fb902ab49e63e..27595404ac957 100644 --- a/srml/executive/src/lib.rs +++ b/srml/executive/src/lib.rs @@ -25,7 +25,6 @@ extern crate parity_codec_derive; #[cfg_attr(test, macro_use)] extern crate srml_support as runtime_support; -#[cfg_attr(not(feature = "std"), macro_use)] extern crate sr_std as rstd; extern crate sr_io as runtime_io; extern crate parity_codec as codec; @@ -54,11 +53,14 @@ use primitives::{ApplyOutcome, ApplyError}; use primitives::transaction_validity::{TransactionValidity, TransactionPriority, TransactionLongevity}; mod internal { + pub const MAX_TRANSACTIONS_SIZE: u32 = 4 * 1024 * 1024; + pub enum ApplyError { BadSignature(&'static str), Stale, Future, CantPay, + FullBlock, } pub enum ApplyOutcome { @@ -144,34 +146,40 @@ impl< pub fn apply_extrinsic(uxt: Block::Extrinsic) -> result::Result { let encoded = uxt.encode(); let encoded_len = encoded.len(); - >::note_extrinsic(encoded); - match Self::apply_extrinsic_no_note_with_len(uxt, encoded_len) { + match Self::apply_extrinsic_with_len(uxt, encoded_len, Some(encoded)) { Ok(internal::ApplyOutcome::Success) => Ok(ApplyOutcome::Success), Ok(internal::ApplyOutcome::Fail(_)) => Ok(ApplyOutcome::Fail), Err(internal::ApplyError::CantPay) => Err(ApplyError::CantPay), Err(internal::ApplyError::BadSignature(_)) => Err(ApplyError::BadSignature), Err(internal::ApplyError::Stale) => Err(ApplyError::Stale), Err(internal::ApplyError::Future) => Err(ApplyError::Future), + Err(internal::ApplyError::FullBlock) => Err(ApplyError::FullBlock), } } /// Apply an extrinsic inside the block execution function. fn apply_extrinsic_no_note(uxt: Block::Extrinsic) { let l = uxt.encode().len(); - match Self::apply_extrinsic_no_note_with_len(uxt, l) { + match Self::apply_extrinsic_with_len(uxt, l, None) { Ok(internal::ApplyOutcome::Success) => (), Ok(internal::ApplyOutcome::Fail(e)) => runtime_io::print(e), Err(internal::ApplyError::CantPay) => panic!("All extrinsics should have sender able to pay their fees"), Err(internal::ApplyError::BadSignature(_)) => panic!("All extrinsics should be properly signed"), Err(internal::ApplyError::Stale) | Err(internal::ApplyError::Future) => panic!("All extrinsics should have the correct nonce"), + Err(internal::ApplyError::FullBlock) => panic!("Extrinsics should not exceed block limit"), } } /// Actually apply an extrinsic given its `encoded_len`; this doesn't note its hash. - fn apply_extrinsic_no_note_with_len(uxt: Block::Extrinsic, encoded_len: usize) -> result::Result { + fn apply_extrinsic_with_len(uxt: Block::Extrinsic, encoded_len: usize, to_note: Option>) -> result::Result { // Verify the signature is good. let xt = uxt.check(&Default::default()).map_err(internal::ApplyError::BadSignature)?; + // Check the size of the block if that extrinsic is applied. + if >::all_extrinsics_len() + encoded_len as u32 > internal::MAX_TRANSACTIONS_SIZE { + return Err(internal::ApplyError::FullBlock); + } + if let (Some(sender), Some(index)) = (xt.sender(), xt.index()) { // check index let expected_index = >::account_nonce(sender); @@ -188,10 +196,16 @@ impl< >::inc_account_nonce(sender); } + // make sure to `note_extrinsic` only after we know it's going to be executed + // to prevent it from leaking in storage. + if let Some(encoded) = to_note { + >::note_extrinsic(encoded); + } + // decode parameters and dispatch let (f, s) = xt.deconstruct(); let r = f.dispatch(s.into()); - >::note_applied_extrinsic(&r); + >::note_applied_extrinsic(&r, encoded_len as u32); r.map(|_| internal::ApplyOutcome::Success).or_else(|e| Ok(internal::ApplyOutcome::Fail(e))) } @@ -415,4 +429,35 @@ mod tests { assert_eq!(>::extrinsic_index(), Some(0)); }); } + + #[test] + fn block_size_limit_enforced() { + let run_test = |should_fail: bool| { + let mut t = new_test_ext(); + let xt = primitives::testing::TestXt(Some(1), 0, Call::transfer(33, 69)); + let xt2 = primitives::testing::TestXt(Some(1), 1, Call::transfer(33, 69)); + let encoded = xt2.encode(); + let len = if should_fail { (internal::MAX_TRANSACTIONS_SIZE - 1) as usize } else { encoded.len() }; + with_externalities(&mut t, || { + Executive::initialise_block(&Header::new(1, H256::default(), H256::default(), [69u8; 32].into(), Digest::default())); + assert_eq!(>::all_extrinsics_len(), 0); + + Executive::apply_extrinsic(xt).unwrap(); + let res = Executive::apply_extrinsic_with_len(xt2, len, Some(encoded)); + + if should_fail { + assert!(res.is_err()); + assert_eq!(>::all_extrinsics_len(), 28); + assert_eq!(>::extrinsic_index(), Some(1)); + } else { + assert!(res.is_ok()); + assert_eq!(>::all_extrinsics_len(), 56); + assert_eq!(>::extrinsic_index(), Some(2)); + } + }); + }; + + run_test(false); + run_test(true); + } } diff --git a/srml/system/src/lib.rs b/srml/system/src/lib.rs index c47c970a98692..640a1d2bf47c7 100644 --- a/srml/system/src/lib.rs +++ b/srml/system/src/lib.rs @@ -210,6 +210,7 @@ decl_storage! { pub AccountNonce get(account_nonce): map T::AccountId => T::Index; ExtrinsicCount: Option; + AllExtrinsicsLen: Option; pub BlockHash get(block_hash) build(|_| vec![(T::BlockNumber::zero(), [69u8; 32])]): map T::BlockNumber => T::Hash; ExtrinsicData get(extrinsic_data): map u32 => Vec; RandomSeed get(random_seed) build(|_| [0u8; 32]): T::Hash; @@ -283,6 +284,11 @@ impl Module { storage::unhashed::get(well_known_keys::EXTRINSIC_INDEX) } + /// Gets a total length of all executed extrinsics. + pub fn all_extrinsics_len() -> u32 { + >::get().unwrap_or_default() + } + /// Start the execution of a particular block. pub fn initialise(number: &T::BlockNumber, parent_hash: &T::Hash, txs_root: &T::Hash) { // populate environment. @@ -299,6 +305,7 @@ impl Module { pub fn finalise() -> T::Header { >::kill(); >::kill(); + >::kill(); let number = >::take(); let parent_hash = >::take(); @@ -385,19 +392,25 @@ impl Module { /// Note what the extrinsic data of the current extrinsic index is. If this is called, then /// ensure `derive_extrinsics` is also called before block-building is completed. + /// + /// NOTE this function is called only when the block is being constructed locally. + /// `execute_block` doesn't note any extrinsics. pub fn note_extrinsic(encoded_xt: Vec) { >::insert(Self::extrinsic_index().unwrap_or_default(), encoded_xt); } /// To be called immediately after an extrinsic has been applied. - pub fn note_applied_extrinsic(r: &Result<(), &'static str>) { + pub fn note_applied_extrinsic(r: &Result<(), &'static str>, encoded_len: u32) { Self::deposit_event(match r { Ok(_) => Event::ExtrinsicSuccess, Err(_) => Event::ExtrinsicFailed, }.into()); let next_extrinsic_index = Self::extrinsic_index().unwrap_or_default() + 1u32; + let total_length = encoded_len.saturating_add(Self::all_extrinsics_len()); + storage::unhashed::put(well_known_keys::EXTRINSIC_INDEX, &next_extrinsic_index); + >::put(&total_length); } /// To be called immediately after `note_applied_extrinsic` of the last extrinsic of the block @@ -500,8 +513,8 @@ mod tests { System::initialise(&2, &[0u8; 32].into(), &[0u8; 32].into()); System::deposit_event(42u16); - System::note_applied_extrinsic(&Ok(())); - System::note_applied_extrinsic(&Err("")); + System::note_applied_extrinsic(&Ok(()), 0); + System::note_applied_extrinsic(&Err(""), 0); System::note_finished_extrinsics(); System::deposit_event(3u16); System::finalise();