Prevent building invalid blocks (#430)

* Commit extrinsics changes

* Removed panic=abort

* Commit when needed

* Resotre default hook for the native call

* Revert test

* Proper test

* Sorted errors and fixed wasm build

substrate/Cargo.toml

@@ -91,4 +91,5 @@ is-it-maintained-issue-resolution = { repository = "paritytech/polkadot" }
 is-it-maintained-open-issues = { repository = "paritytech/polkadot" }
 
 [profile.release]
-panic = "abort"
+# Substrate runtime requires unwinding.
+panic = "unwind" 

substrate/substrate/cli/src/panic_hook.rs

@@ -64,5 +64,6 @@ fn panic_hook(info: &PanicInfo) {
 	);
 
 	let _ = writeln!(stderr, "{}", ABOUT_PANIC);
+	::std::process::exit(1);
 }
 

substrate/substrate/client/src/block_builder.rs

@@ -22,6 +22,7 @@ use state_machine::{self, native_when_possible};
 use runtime_primitives::traits::{Header as HeaderT, Hash, Block as BlockT, One, HashFor};
 use runtime_primitives::generic::BlockId;
 use {backend, error, Client, CallExecutor};
+use runtime_primitives::{ApplyResult, ApplyOutcome};
 
 /// Utility for building new (valid) blocks from a stream of extrinsics.
 pub struct BlockBuilder<B, E, Block> where
@@ -68,6 +69,7 @@ impl<B, E, Block> BlockBuilder<B, E, Block> where
 		);
 
 		executor.call_at_state(&state, &mut changes, "initialise_block", &header.encode(), native_when_possible())?;
+		changes.commit_prospective();
 
 		Ok(BlockBuilder {
 			header,
@@ -83,9 +85,22 @@ impl<B, E, Block> BlockBuilder<B, E, Block> where
 	/// the error. Otherwise, it will return a mutable reference to self (in order to chain).
 	pub fn push(&mut self, xt: <Block as BlockT>::Extrinsic) -> error::Result<()> {
 		match self.executor.call_at_state(&self.state, &mut self.changes, "apply_extrinsic", &xt.encode(), native_when_possible()) {
-			Ok(_) => {
-				self.extrinsics.push(xt);
-				Ok(())
+			Ok((result, _)) => {
+				match ApplyResult::decode(&mut result.as_slice()) {
+					Some(Ok(ApplyOutcome::Success)) | Some(Ok(ApplyOutcome::Fail)) => {
+						self.extrinsics.push(xt);
+						self.changes.commit_prospective();
+						Ok(())
+					}
+					Some(Err(e)) => {
+						self.changes.discard_prospective();
+						Err(error::ErrorKind::ApplyExtinsicFailed(e).into())
+					}
+					None => {
+						self.changes.discard_prospective();
+						Err(error::ErrorKind::CallResultDecode("apply_extrinsic").into())
+					}
+				}
 			}
 			Err(e) => {
 				self.changes.discard_prospective();

substrate/substrate/client/src/client.rs

@@ -592,4 +592,31 @@ mod tests {
 		assert_eq!(client.using_environment(|| test_runtime::system::balance_of(Keyring::Alice.to_raw_public().into())).unwrap(), 958);
 		assert_eq!(client.using_environment(|| test_runtime::system::balance_of(Keyring::Ferdie.to_raw_public().into())).unwrap(), 42);
 	}
+
+	#[test]
+	fn block_builder_does_not_include_invalid() {
+		let client = test_client::new();
+
+		let mut builder = client.new_block().unwrap();
+
+		builder.push(sign_tx(Transfer {
+			from: Keyring::Alice.to_raw_public().into(),
+			to: Keyring::Ferdie.to_raw_public().into(),
+			amount: 42,
+			nonce: 0,
+		})).unwrap();
+
+		assert!(builder.push(sign_tx(Transfer {
+			from: Keyring::Eve.to_raw_public().into(),
+			to: Keyring::Alice.to_raw_public().into(),
+			amount: 42,
+			nonce: 0,
+		})).is_err());
+
+		client.justify_and_import(BlockOrigin::Own, builder.bake().unwrap()).unwrap();
+
+		assert_eq!(client.info().unwrap().chain.best_number, 1);
+		assert!(client.state_at(&BlockId::Number(1)).unwrap() != client.state_at(&BlockId::Number(0)).unwrap());
+		assert_eq!(client.body(&BlockId::Number(1)).unwrap().unwrap().len(), 1)
+	}
 }

substrate/substrate/client/src/error.rs

@@ -19,6 +19,7 @@
 use std;
 use state_machine;
 use primitives::hexdisplay::HexDisplay;
+use runtime_primitives::ApplyError;
 
 error_chain! {
 	errors {
@@ -34,6 +35,12 @@ error_chain! {
 			display("UnknownBlock: {}", &*h),
 		}
 
+		/// Applying extrinsic error.
+		ApplyExtinsicFailed(e: ApplyError) {
+			description("Extrinsic error"),
+			display("Extrinsic error: {:?}", e),
+		}
+
 		/// Execution error.
 		Execution(e: Box<state_machine::Error>) {
 			description("execution error"),
@@ -111,6 +118,12 @@ error_chain! {
 			description("remote fetch failed"),
 			display("Remote data fetch has been failed"),
 		}
+
+		/// Error decoding call result.
+		CallResultDecode(method: &'static str) {
+			description("Error decoding call result")
+			display("Error decoding call result of {}", method)
+		}
 	}
 }
 
@@ -139,4 +152,4 @@ impl Error {
 	}
 }
 
-impl state_machine::Error for Error {}
+impl state_machine::Error for Error {}

substrate/substrate/executor/src/native_executor.rs

@@ -80,7 +80,11 @@ fn fetch_cached_runtime_version<'a, E: Externalities>(
 fn safe_call<F, U>(f: F) -> Result<U>
 	where F: ::std::panic::UnwindSafe + FnOnce() -> U
 {
-	::std::panic::catch_unwind(f).map_err(|_| ErrorKind::Runtime.into())
+	// Substrate uses custom panic hook that terminates process on panic. Disable it for the native call.
+	let hook = ::std::panic::take_hook();
+	let result = ::std::panic::catch_unwind(f).map_err(|_| ErrorKind::Runtime.into());
+	::std::panic::set_hook(hook);
+	result
 }
 
 /// Set up the externalities and safe calling environment to execute calls to a native runtime.

substrate/substrate/runtime/executive/src/lib.rs

@@ -355,4 +355,15 @@ mod tests {
 			});
 		});
 	}
+
+	#[test]
+	fn bad_extrinsic_not_inserted() {
+		let mut t = new_test_ext();
+		let xt = primitives::testing::TestXt((1, 42, Call::transfer(33.into(), 69)));
+		with_externalities(&mut t, || {
+			Executive::initialise_block(&Header::new(1, H256::default(), H256::default(), [69u8; 32].into(), Digest::default()));
+			assert!(Executive::apply_extrinsic(xt).is_err());
+			assert_eq!(<system::Module<Test>>::extrinsic_index(), 0);
+		});
+	}
 }

substrate/substrate/state-machine/src/lib.rs

@@ -342,11 +342,9 @@ pub fn execute_using_consensus_failure_handler<
 
 	match result {
 		Ok(x) => {
-			overlay.commit_prospective();
 			Ok(x)
 		}
 		Err(e) => {
-			overlay.discard_prospective();
 			Err(Box::new(e))
 		}
 	}

substrate/substrate/test-runtime/src/system.rs

@@ -21,6 +21,7 @@ use rstd::prelude::*;
 use runtime_io::{storage_root, enumerated_trie_root};
 use runtime_support::storage::{self, StorageValue, StorageMap};
 use runtime_primitives::traits::{Hash as HashT, BlakeTwo256};
+use runtime_primitives::{ApplyError, ApplyOutcome, ApplyResult};
 use codec::{KeyedVec, Encode};
 use super::{AccountId, BlockNumber, Extrinsic, H256 as Hash, Block, Header};
 
@@ -72,7 +73,7 @@ pub fn execute_block(block: Block) {
 	assert!(header.extrinsics_root == txs_root, "Transaction trie root must be valid.");
 
 	// execute transactions
-	block.extrinsics.iter().for_each(execute_transaction_backend);
+	block.extrinsics.iter().for_each(|e| { execute_transaction_backend(e).map_err(|_| ()).expect("Extrinsic error"); });
 
 	// check storage root.
 	let storage_root = storage_root().into();
@@ -82,11 +83,11 @@ pub fn execute_block(block: Block) {
 
 /// Execute a transaction outside of the block execution function.
 /// This doesn't attempt to validate anything regarding the block.
-pub fn execute_transaction(utx: Extrinsic) {
+pub fn execute_transaction(utx: Extrinsic) -> ApplyResult {
 	let extrinsic_index = ExtrinsicIndex::get();
 	ExtrinsicData::insert(extrinsic_index, utx.encode());
 	ExtrinsicIndex::put(extrinsic_index + 1);
-	execute_transaction_backend(&utx);
+	execute_transaction_backend(&utx)
 }
 
 /// Finalise the block.
@@ -109,13 +110,13 @@ pub fn finalise_block() -> Header {
 	}
 }
 
-fn execute_transaction_backend(utx: &Extrinsic) {
+fn execute_transaction_backend(utx: &Extrinsic) -> ApplyResult {
 	use runtime_primitives::traits::BlindCheckable;
 
 	// check signature
 	let utx = match utx.clone().check() {
 		Ok(tx) => tx,
-		Err(_) => panic!("All transactions should be properly signed"),
+		Err(_) => return Err(ApplyError::BadSignature),
 	};
 
 	let tx: ::Transfer = utx.transfer;
@@ -123,7 +124,9 @@ fn execute_transaction_backend(utx: &Extrinsic) {
 	// check nonce
 	let nonce_key = tx.from.to_keyed_vec(NONCE_OF);
 	let expected_nonce: u64 = storage::get_or(&nonce_key, 0);
-	assert!(tx.nonce == expected_nonce, "All transactions should have the correct nonce");
+	if !(tx.nonce == expected_nonce) {
+		return Err(ApplyError::Stale)
+	}
 
 	// increment nonce in storage
 	storage::put(&nonce_key, &(expected_nonce + 1));
@@ -133,11 +136,14 @@ fn execute_transaction_backend(utx: &Extrinsic) {
 	let from_balance: u64 = storage::get_or(&from_balance_key, 0);
 
 	// enact transfer
-	assert!(tx.amount <= from_balance, "All transactions should transfer at most the sender balance");
+	if !(tx.amount <= from_balance) {
+		return Err(ApplyError::CantPay)
+	}
 	let to_balance_key = tx.to.to_keyed_vec(BALANCE_OF);
 	let to_balance: u64 = storage::get_or(&to_balance_key, 0);
 	storage::put(&from_balance_key, &(from_balance - tx.amount));
 	storage::put(&to_balance_key, &(to_balance + tx.amount));
+	Ok(ApplyOutcome::Success)
 }
 
 #[cfg(feature = "std")]