Allow both consensus and runtime to limit block building (#1581)

* Limit block size in runtime,

* Add test for basic authorship.

* Store length of extrinsics instead of computing it.

* Don't rely on note_extrinsic

* Use hashed version of storage and write test.

* Recompile runtime.

substrate/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" }

substrate/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<Block: BlockT> {
 	/// Push an extrinsic onto the block. Fails if the extrinsic is invalid.
@@ -144,6 +142,7 @@ impl<C, A> consensus_common::Environment<<C as AuthoringApi>::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<Block: BlockT, C, A: txpool::ChainApi> {
 	parent_id: BlockId<Block>,
 	parent_number: <<Block as BlockT>::Header as HeaderT>::Number,
 	transaction_pool: Arc<TransactionPool<A>>,
+	now: Box<Fn() -> time::Instant>,
 }
 
 impl<Block, C, A> consensus_common::Proposer<<C as AuthoringApi>::Block> for Proposer<Block, C, A> where
@@ -169,10 +169,12 @@ impl<Block, C, A> consensus_common::Proposer<<C as AuthoringApi>::Block> for Pro
 	type Create = Result<<C as AuthoringApi>::Block, error::Error>;
 	type Error = error::Error;
 
-	fn propose(&self, inherent_data: InherentData)
+	fn propose(&self, inherent_data: InherentData, max_duration: time::Duration)
 		-> Result<<C as AuthoringApi>::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<Block, C, A> Proposer<Block, C, A>	where
 	A: txpool::ChainApi<Block=Block>,
 	client::error::Error: From<<C as AuthoringApi>::Error>,
 {
-	fn propose_with(&self, inherent_data: InherentData)
+	fn propose_with(&self, inherent_data: InherentData, deadline: time::Instant)
 		-> Result<<C as AuthoringApi>::Block, error::Error>
 	{
 		use runtime_primitives::traits::BlakeTwo256;
@@ -193,16 +195,21 @@ impl<Block, C, A> Proposer<Block, C, A>	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<Block, C, A> Proposer<Block, C, A>	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);
+	}
+
+}

substrate/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: <Block as BlockT>::Extrinsic) -> error::Result<()> {
 		use crate::runtime_api::ApiExt;
 

substrate/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<B, C, E, I, SO, Error>(
 						};
 
 						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<TestBlock, Error>;
 
-		fn propose(&self, _: InherentData) -> Result<TestBlock, Error> {
+		fn propose(&self, _: InherentData, _: Duration) -> Result<TestBlock, Error> {
 			self.1.new_block().unwrap().bake().map_err(|e| e.into())
 		}
 	}

substrate/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<Block: BlockT>(
 	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() {

substrate/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<B: Block> {
 	/// Future that resolves to a committed proposal.
 	type Create: IntoFuture<Item=B, Error=Self::Error>;
 	/// 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.

substrate/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";
 

substrate/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 {

substrate/core/test-runtime/src/lib.rs

@@ -226,7 +226,7 @@ impl_runtime_apis! {
 		}
 
 		fn inherent_extrinsics(_data: InherentData) -> Vec<<Block as BlockT>::Extrinsic> {
-			unimplemented!()
+			vec![]
 		}
 
 		fn check_inherents(_block: Block, _data: InherentData) -> CheckInherentsResult {