Public extrinsic apply doesn't panic (#178)

* Merge remote-tracking branch 'origin/master' into gav-xts-dont-panic

* Update wasm.

* consensus, session and staking all panic-safe.

* Democracy doesn't panic in apply.

* Fix tests.

* Extra helper macro, council depanicked.

* Fix one test.

* Fix up all council tests. No panics!

* Council voting depanicked.

* utilise hygene

substrate/demo/cli/src/lib.rs

@@ -115,7 +115,8 @@ pub fn run<I, T>(args: I) -> error::Result<()> where
 				staking: Some(StakingConfig {
 					current_era: 0,
 					intentions: vec![],
-					transaction_fee: 100,
+					transaction_base_fee: 100,
+					transaction_byte_fee: 1,
 					balances: vec![(god_key.clone(), 1u64 << 63)].into_iter().collect(),
 					validator_count: 12,
 					sessions_per_era: 24,	// 24 hours per era.

substrate/demo/executor/src/lib.rs

@@ -82,8 +82,9 @@ mod tests {
 	#[test]
 	fn panic_execution_with_foreign_code_gives_error() {
 		let mut t: TestExternalities = map![
-			twox_128(&<staking::FreeBalance<Concrete>>::key_for(*Alice)).to_vec() => vec![68u8, 0, 0, 0, 0, 0, 0, 0],
-			twox_128(<staking::TransactionFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
+			twox_128(&<staking::FreeBalance<Concrete>>::key_for(*Alice)).to_vec() => vec![69u8, 0, 0, 0, 0, 0, 0, 0],
+			twox_128(<staking::TransactionBaseFee<Concrete>>::key()).to_vec() => vec![70u8; 8],
+			twox_128(<staking::TransactionByteFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
 			twox_128(&<system::BlockHash<Concrete>>::key_for(0)).to_vec() => vec![0u8; 32]
 		];
 
@@ -96,8 +97,9 @@ mod tests {
 	#[test]
 	fn panic_execution_with_native_equivalent_code_gives_error() {
 		let mut t: TestExternalities = map![
-			twox_128(&<staking::FreeBalance<Concrete>>::key_for(*Alice)).to_vec() => vec![68u8, 0, 0, 0, 0, 0, 0, 0],
-			twox_128(<staking::TransactionFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
+			twox_128(&<staking::FreeBalance<Concrete>>::key_for(*Alice)).to_vec() => vec![69u8, 0, 0, 0, 0, 0, 0, 0],
+			twox_128(<staking::TransactionBaseFee<Concrete>>::key()).to_vec() => vec![70u8; 8],
+			twox_128(<staking::TransactionByteFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
 			twox_128(&<system::BlockHash<Concrete>>::key_for(0)).to_vec() => vec![0u8; 32]
 		];
 
@@ -111,7 +113,8 @@ mod tests {
 	fn successful_execution_with_native_equivalent_code_gives_ok() {
 		let mut t: TestExternalities = map![
 			twox_128(&<staking::FreeBalance<Concrete>>::key_for(*Alice)).to_vec() => vec![111u8, 0, 0, 0, 0, 0, 0, 0],
-			twox_128(<staking::TransactionFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
+			twox_128(<staking::TransactionBaseFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
+			twox_128(<staking::TransactionByteFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
 			twox_128(&<system::BlockHash<Concrete>>::key_for(0)).to_vec() => vec![0u8; 32]
 		];
 
@@ -130,7 +133,8 @@ mod tests {
 	fn successful_execution_with_foreign_code_gives_ok() {
 		let mut t: TestExternalities = map![
 			twox_128(&<staking::FreeBalance<Concrete>>::key_for(*Alice)).to_vec() => vec![111u8, 0, 0, 0, 0, 0, 0, 0],
-			twox_128(<staking::TransactionFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
+			twox_128(<staking::TransactionBaseFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
+			twox_128(<staking::TransactionByteFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
 			twox_128(&<system::BlockHash<Concrete>>::key_for(0)).to_vec() => vec![0u8; 32]
 		];
 
@@ -162,7 +166,8 @@ mod tests {
 				intentions: vec![Alice.into(), Bob.into(), Charlie.into()],
 				validator_count: 3,
 				bonding_duration: 0,
-				transaction_fee: 1,
+				transaction_base_fee: 1,
+				transaction_byte_fee: 0,
 			}),
 			democracy: Some(Default::default()),
 			council: Some(Default::default()),
@@ -197,7 +202,7 @@ mod tests {
 		construct_block(
 			1,
 			[69u8; 32].into(),
-			hex!("a63d59c6a7347cd7a1dc1ec139723b531f0ac450e39b1c532d5ca69ff74ad811").into(),
+			hex!("76b0393b4958d3cb98bb51d9f4edb316af48485142b8721e94c3b52c75ec3243").into(),
 			vec![Extrinsic {
 				signed: Alice.into(),
 				index: 0,
@@ -210,7 +215,7 @@ mod tests {
 		construct_block(
 			2,
 			block1().1,
-			hex!("1c3623b2e3f7e43752debb9015bace4f6931593579b5af34457b931315f5e2ab").into(),
+			hex!("8ae9828a5988459d35fb428086170dead660176ee0766e89bc1a4b48153d4e88").into(),
 			vec![
 				Extrinsic {
 					signed: Bob.into(),
@@ -267,8 +272,9 @@ mod tests {
 	#[test]
 	fn panic_execution_gives_error() {
 		let mut t: TestExternalities = map![
-			twox_128(&<staking::FreeBalance<Concrete>>::key_for(*Alice)).to_vec() => vec![68u8, 0, 0, 0, 0, 0, 0, 0],
-			twox_128(<staking::TransactionFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
+			twox_128(&<staking::FreeBalance<Concrete>>::key_for(*Alice)).to_vec() => vec![69u8, 0, 0, 0, 0, 0, 0, 0],
+			twox_128(<staking::TransactionBaseFee<Concrete>>::key()).to_vec() => vec![70u8; 8],
+			twox_128(<staking::TransactionByteFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
 			twox_128(&<system::BlockHash<Concrete>>::key_for(0)).to_vec() => vec![0u8; 32]
 		];
 
@@ -283,7 +289,8 @@ mod tests {
 	fn successful_execution_gives_ok() {
 		let mut t: TestExternalities = map![
 			twox_128(&<staking::FreeBalance<Concrete>>::key_for(*Alice)).to_vec() => vec![111u8, 0, 0, 0, 0, 0, 0, 0],
-			twox_128(<staking::TransactionFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
+			twox_128(<staking::TransactionBaseFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
+			twox_128(<staking::TransactionByteFee<Concrete>>::key()).to_vec() => vec![0u8; 8],
 			twox_128(&<system::BlockHash<Concrete>>::key_for(0)).to_vec() => vec![0u8; 32]
 		];
 

substrate/polkadot/cli/src/cli.yml

@@ -65,6 +65,6 @@ args:
   - chain:
       long: chain
       value_name: CHAIN_SPEC
-      help: Specify the chain specification (one of dev, local or poc-1)
+      help: Specify the chain specification (one of dev, local or poc-2)
       takes_value: true
 subcommands:

substrate/polkadot/cli/src/lib.rs

@@ -110,7 +110,7 @@ impl substrate_rpc::system::SystemApi for Configuration {
 		Ok(match self.0.chain_spec {
 			ChainSpec::Development => "dev",
 			ChainSpec::LocalTestnet => "local",
-			ChainSpec::PoC1Testnet => "poc-1",
+			ChainSpec::PoC2Testnet => "poc-2",
 		}.into())
 	}
 }
@@ -174,14 +174,14 @@ pub fn run<I, T>(args: I) -> error::Result<()> where
 	match matches.value_of("chain") {
 		Some("dev") => config.chain_spec = ChainSpec::Development,
 		Some("local") => config.chain_spec = ChainSpec::LocalTestnet,
-		Some("poc-1") => config.chain_spec = ChainSpec::PoC1Testnet,
+		Some("poc-2") => config.chain_spec = ChainSpec::PoC2Testnet,
 		None => (),
 		Some(unknown) => panic!("Invalid chain name: {}", unknown),
 	}
 	info!("Chain specification: {}", match config.chain_spec {
 		ChainSpec::Development => "Development",
 		ChainSpec::LocalTestnet => "Local Testnet",
-		ChainSpec::PoC1Testnet => "PoC-1 Testnet",
+		ChainSpec::PoC2Testnet => "PoC-2 Testnet",
 	});
 
 	config.roles = role;

substrate/polkadot/service/src/config.rs

@@ -28,8 +28,8 @@ pub enum ChainSpec {
 	Development,
 	/// Whatever the current runtime is, with simple Alice/Bob auths.
 	LocalTestnet,
-	/// The PoC-1 testnet.
-	PoC1Testnet,
+	/// The PoC-2 testnet.
+	PoC2Testnet,
 }
 
 /// Service configuration.

substrate/polkadot/service/src/lib.rs

@@ -141,7 +141,7 @@ pub struct ChainConfig {
 	boot_nodes: Vec<String>,
 }
 
-fn poc_1_testnet_config() -> ChainConfig {
+fn poc_2_testnet_config() -> ChainConfig {
 	let initial_authorities = vec![
 		hex!["82c39b31a2b79a90f8e66e7a77fdb85a4ed5517f2ae39f6a80565e8ecae85cf5"].into(),
 		hex!["4de37a07567ebcbf8c64568428a835269a566723687058e017b6d69db00a77e7"].into(),
@@ -164,7 +164,8 @@ fn poc_1_testnet_config() -> ChainConfig {
 		staking: Some(StakingConfig {
 			current_era: 0,
 			intentions: initial_authorities.clone(),
-			transaction_fee: 100,
+			transaction_base_fee: 100,
+			transaction_byte_fee: 1,
 			balances: endowed_accounts.iter().map(|&k|(k, 1u128 << 60)).collect(),
 			validator_count: 12,
 			sessions_per_era: 24,	// 24 hours per era.
@@ -222,7 +223,8 @@ fn testnet_config(initial_authorities: Vec<AuthorityId>) -> ChainConfig {
 		staking: Some(StakingConfig {
 			current_era: 0,
 			intentions: initial_authorities.clone(),
-			transaction_fee: 1,
+			transaction_base_fee: 1,
+			transaction_byte_fee: 0,
 			balances: endowed_accounts.iter().map(|&k|(k, (1u128 << 60))).collect(),
 			validator_count: 2,
 			sessions_per_era: 5,
@@ -364,7 +366,7 @@ impl<B, E> Service<B, E>
 		let ChainConfig { genesis_config, boot_nodes } = match config.chain_spec {
 			ChainSpec::Development => development_config(),
 			ChainSpec::LocalTestnet => local_testnet_config(),
-			ChainSpec::PoC1Testnet => poc_1_testnet_config(),
+			ChainSpec::PoC2Testnet => poc_2_testnet_config(),
 		};
 		config.network.boot_nodes.extend(boot_nodes);
 

substrate/substrate/runtime-support/src/lib.rs

@@ -44,3 +44,62 @@ mod hashable;
 pub use self::storage::{StorageVec, StorageList, StorageValue, StorageMap};
 pub use self::hashable::Hashable;
 pub use self::dispatch::{Parameter, Dispatchable, Callable, AuxDispatchable, AuxCallable, IsSubType, IsAuxSubType};
+pub use runtime_io::print;
+
+#[macro_export]
+macro_rules! fail {
+	( $y:expr ) => {{
+		$crate::print($y);
+		return;
+	}}
+}
+
+#[macro_export]
+macro_rules! ensure {
+	( $x:expr, $y:expr ) => {{
+		if !$x {
+			fail!($y);
+		}
+	}};
+	($x:expr) => {{
+		if !$x {
+			$crate::print("Bailing! Cannot ensure: ");
+			$crate::print(stringify!($x));
+			return;
+		}
+	}}
+}
+
+#[macro_export]
+macro_rules! ensure_unwrap {
+	($x:expr, $y: expr) => {
+		if let Some(v) = $x {
+			v
+		} else {
+			fail!{$y}
+		}
+	}
+}
+
+#[macro_export]
+macro_rules! ensure_unwrap_err {
+	($x:expr, $y: expr) => {
+		if let Err(v) = $x {
+			v
+		} else {
+			fail!{$y}
+		}
+	}
+}
+
+#[macro_export]
+#[cfg(feature = "std")]
+macro_rules! assert_noop {
+	( $( $x:tt )* ) => {
+		let h = runtime_io::storage_root();
+		{
+			$( $x )*
+		}
+		assert_eq!(h, runtime_io::storage_root());
+	}
+}

substrate/substrate/runtime/council/src/lib.rs

@@ -224,8 +224,8 @@ impl<T: Trait> Module<T> {
 	/// Set candidate approvals. Approval slots stay valid as long as candidates in those slots
 	/// are registered.
 	fn set_approvals(aux: &T::PublicAux, votes: Vec<bool>, index: VoteIndex) {
-		assert!(!Self::presentation_active());
-		assert_eq!(index, Self::vote_index());
+		ensure!(!Self::presentation_active());
+		ensure!(index == Self::vote_index());
 		if !<LastActiveOf<T>>::exists(aux.ref_into()) {
 			// not yet a voter - deduct bond.
 			<staking::Module<T>>::reserve_balance(aux.ref_into(), Self::voting_bond());
@@ -245,16 +245,16 @@ impl<T: Trait> Module<T> {
 	///
 	/// May be called by anyone. Returns the voter deposit to `signed`.
 	fn reap_inactive_voter(aux: &T::PublicAux, signed_index: u32, who: T::AccountId, who_index: u32, assumed_vote_index: VoteIndex) {
-		assert!(!Self::presentation_active(), "cannot reap during presentation period");
-		assert!(Self::voter_last_active(aux.ref_into()).is_some(), "reaper must be a voter");
-		let last_active = Self::voter_last_active(&who).expect("target for inactivity cleanup must be active");
-		assert!(assumed_vote_index == Self::vote_index(), "vote index not current");
-		assert!(last_active < assumed_vote_index - Self::inactivity_grace_period(), "cannot reap during grace perid");
+		ensure!(!Self::presentation_active(), "cannot reap during presentation period");
+		ensure!(Self::voter_last_active(aux.ref_into()).is_some(), "reaper must be a voter");
+		let last_active = ensure_unwrap!(Self::voter_last_active(&who), "target for inactivity cleanup must be active");
+		ensure!(assumed_vote_index == Self::vote_index(), "vote index not current");
+		ensure!(last_active < assumed_vote_index - Self::inactivity_grace_period(), "cannot reap during grace perid");
 		let voters = Self::voters();
 		let signed_index = signed_index as usize;
 		let who_index = who_index as usize;
-		assert!(signed_index < voters.len() && &voters[signed_index] == aux.ref_into(), "bad reporter index");
-		assert!(who_index < voters.len() && voters[who_index] == who, "bad target index");
+		ensure!(signed_index < voters.len() && &voters[signed_index] == aux.ref_into(), "bad reporter index");
+		ensure!(who_index < voters.len() && voters[who_index] == who, "bad target index");
 
 		// will definitely kill one of signed or who now.
 
@@ -263,8 +263,8 @@ impl<T: Trait> Module<T> {
 			.any(|(&appr, addr)|
 				appr &&
 				*addr != T::AccountId::default() &&
-				Self::candidate_reg_info(addr)
-					.expect("all items in candidates list are registered").0 <= last_active);
+				Self::candidate_reg_info(addr).map_or(false, |x| x.0 <= last_active)/*defensive only: all items in candidates list are registered*/
+			);
 
 		Self::remove_voter(
 			if valid { &who } else { aux.ref_into() },
@@ -280,12 +280,12 @@ impl<T: Trait> Module<T> {
 
 	/// Remove a voter. All votes are cancelled and the voter deposit is returned.
 	fn retract_voter(aux: &T::PublicAux, index: u32) {
-		assert!(!Self::presentation_active(), "cannot retract when presenting");
-		assert!(<LastActiveOf<T>>::exists(aux.ref_into()), "cannot retract non-voter");
+		ensure!(!Self::presentation_active(), "cannot retract when presenting");
+		ensure!(<LastActiveOf<T>>::exists(aux.ref_into()), "cannot retract non-voter");
 		let voters = Self::voters();
 		let index = index as usize;
-		assert!(index < voters.len(), "retraction index invalid");
-		assert!(&voters[index] == aux.ref_into(), "retraction index mismatch");
+		ensure!(index < voters.len(), "retraction index invalid");
+		ensure!(&voters[index] == aux.ref_into(), "retraction index mismatch");
 		Self::remove_voter(aux.ref_into(), index, voters);
 		<staking::Module<T>>::unreserve_balance(aux.ref_into(), Self::voting_bond());
 	}
@@ -294,18 +294,19 @@ impl<T: Trait> Module<T> {
 	///
 	/// Account must have enough transferrable funds in it to pay the bond.
 	fn submit_candidacy(aux: &T::PublicAux, slot: u32) {
-		assert!(!Self::is_a_candidate(aux.ref_into()), "duplicate candidate submission");
-		assert!(<staking::Module<T>>::deduct_unbonded(aux.ref_into(), Self::candidacy_bond()), "candidate has not enough funds");
+		ensure!(!Self::is_a_candidate(aux.ref_into()), "duplicate candidate submission");
+		ensure!(<staking::Module<T>>::can_deduct_unbonded(aux.ref_into(), Self::candidacy_bond()), "candidate has not enough funds");
 
 		let slot = slot as usize;
 		let count = Self::candidate_count() as usize;
 		let candidates = Self::candidates();
-		assert!(
+		ensure!(
 			(slot == count && count == candidates.len()) ||
 			(slot < candidates.len() && candidates[slot] == T::AccountId::default()),
 			"invalid candidate slot"
 		);
 
+		<staking::Module<T>>::deduct_unbonded(aux.ref_into(), Self::candidacy_bond());
 		let mut candidates = candidates;
 		if slot == candidates.len() {
 			candidates.push(aux.ref_into().clone());
@@ -321,23 +322,22 @@ impl<T: Trait> Module<T> {
 	/// Only works if the `block_number >= current_vote().0` and `< current_vote().0 + presentation_duration()``
 	/// `signed` should have at least
 	fn present_winner(aux: &T::PublicAux, candidate: T::AccountId, total: T::Balance, index: VoteIndex) {
-		assert_eq!(index, Self::vote_index(), "index not current");
-		let (_, _, expiring) = Self::next_finalise()
-			.expect("cannot present outside of presentation period");
+		ensure!(index == Self::vote_index(), "index not current");
+		let (_, _, expiring) = ensure_unwrap!(Self::next_finalise(), "cannot present outside of presentation period");
 		let stakes = Self::snapshoted_stakes();
 		let voters = Self::voters();
 		let bad_presentation_punishment = Self::present_slash_per_voter() * T::Balance::sa(voters.len());
-		assert!(<staking::Module<T>>::can_slash(aux.ref_into(), bad_presentation_punishment), "presenter must have sufficient slashable funds");
+		ensure!(<staking::Module<T>>::can_slash(aux.ref_into(), bad_presentation_punishment), "presenter must have sufficient slashable funds");
 
-		let mut leaderboard = Self::leaderboard().expect("leaderboard must exist while present phase active");
-		assert!(total > leaderboard[0].0, "candidate not worthy of leaderboard");
+		let mut leaderboard = ensure_unwrap!(Self::leaderboard(), "leaderboard must exist while present phase active");
+		ensure!(total > leaderboard[0].0, "candidate not worthy of leaderboard");
 
 		if let Some(p) = Self::active_council().iter().position(|&(ref c, _)| c == &candidate) {
-			assert!(p < expiring.len(), "candidate must not form a duplicated member if elected");
+			ensure!(p < expiring.len(), "candidate must not form a duplicated member if elected");
 		}
 
 		let (registered_since, candidate_index): (VoteIndex, u32) =
-			Self::candidate_reg_info(&candidate).expect("presented candidate must be current");
+			ensure_unwrap!(Self::candidate_reg_info(&candidate), "presented candidate must be current");
 		let actual_total = voters.iter()
 			.zip(stakes.iter())
 			.filter_map(|(voter, stake)|
@@ -440,8 +440,8 @@ impl<T: Trait> Module<T> {
 	/// Clears all presented candidates, returning the bond of the elected ones.
 	fn finalise_tally() {
 		<SnapshotedStakes<T>>::kill();
-		let (_, coming, expiring): (T::BlockNumber, u32, Vec<T::AccountId>) = <NextFinalise<T>>::take()
-			.expect("finalise can only be called after a tally is started.");
+		let (_, coming, expiring): (T::BlockNumber, u32, Vec<T::AccountId>) =
+			ensure_unwrap!(<NextFinalise<T>>::take(), "finalise can only be called after a tally is started.");
 		let leaderboard: Vec<(T::Balance, T::AccountId)> = <Leaderboard<T>>::take().unwrap_or_default();
 		let new_expiry = <system::Module<T>>::block_number() + Self::term_duration();
 
@@ -476,7 +476,7 @@ impl<T: Trait> Module<T> {
 			.rev()
 			.take_while(|&(b, _)| !b.is_zero())
 			.skip(coming as usize)
-			.map(|(_, a)| { let i = Self::candidate_reg_info(&a).expect("runner up must be registered").1; (a, i) });
+			.filter_map(|(_, a)| Self::candidate_reg_info(&a).map(|i| (a, i.1)));
 		let mut count = 0u32;
 		for (address, slot) in runners_up {
 			new_candidates[slot as usize] = address;
@@ -626,7 +626,8 @@ mod tests {
 			intentions: vec![],
 			validator_count: 2,
 			bonding_duration: 0,
-			transaction_fee: 0,
+			transaction_base_fee: 0,
+			transaction_byte_fee: 0,
 		}.build_externalities());
 		t.extend(democracy::GenesisConfig::<Test>{
 			launch_period: 1,
@@ -760,55 +761,50 @@ mod tests {
 	}
 
 	#[test]
-	#[should_panic(expected = "invalid candidate slot")]
-	fn candidate_submission_not_using_free_slot_should_panic() {
-		let mut t = new_test_ext_with_candidate_holes();
-
-		with_externalities(&mut t, || {
+	fn candidate_submission_not_using_free_slot_should_not_work() {
+		with_externalities(&mut new_test_ext_with_candidate_holes(), || {
 			System::set_block_number(1);
-			Council::submit_candidacy(&4, 3);
+			assert_noop!{Council::submit_candidacy(&4, 3)};	// gives "invalid candidate slot"
 		});
 	}
 
 	#[test]
-	#[should_panic(expected = "invalid candidate slot")]
-	fn bad_candidate_slot_submission_should_panic() {
+	fn bad_candidate_slot_submission_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(1);
 			assert_eq!(Council::candidates(), Vec::<u64>::new());
-			Council::submit_candidacy(&1, 1);
+			assert_noop!{Council::submit_candidacy(&1, 1)};	// gives "invalid candidate slot"
 		});
 	}
 
 	#[test]
-	#[should_panic(expected = "invalid candidate slot")]
-	fn non_free_candidate_slot_submission_should_panic() {
+	fn non_free_candidate_slot_submission_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(1);
 			assert_eq!(Council::candidates(), Vec::<u64>::new());
 			Council::submit_candidacy(&1, 0);
-			Council::submit_candidacy(&2, 0);
+			assert_eq!(Council::candidates(), vec![1]);
+			assert_noop!{Council::submit_candidacy(&2, 0)};	// gives "invalid candidate slot"
 		});
 	}
 
 	#[test]
-	#[should_panic(expected = "duplicate candidate submission")]
-	fn dupe_candidate_submission_should_panic() {
+	fn dupe_candidate_submission_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(1);
 			assert_eq!(Council::candidates(), Vec::<u64>::new());
 			Council::submit_candidacy(&1, 0);
-			Council::submit_candidacy(&1, 1);
+			assert_eq!(Council::candidates(), vec![1]);
+			assert_noop!{Council::submit_candidacy(&1, 1)};	// gives "duplicate candidate submission"
 		});
 	}
 
 	#[test]
-	#[should_panic(expected = "candidate has not enough funds")]
-	fn poor_candidate_submission_should_panic() {
+	fn poor_candidate_submission_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(1);
 			assert_eq!(Council::candidates(), Vec::<u64>::new());
-			Council::submit_candidacy(&7, 0);
+			assert_noop!{Council::submit_candidacy(&7, 0)};	// gives "candidate has not enough funds"
 		});
 	}
 
@@ -906,36 +902,34 @@ mod tests {
 	}
 
 	#[test]
-	#[should_panic(expected = "retraction index mismatch")]
-	fn invalid_retraction_index_should_panic() {
+	fn invalid_retraction_index_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(1);
 			Council::submit_candidacy(&3, 0);
 			Council::set_approvals(&1, vec![true], 0);
 			Council::set_approvals(&2, vec![true], 0);
-			Council::retract_voter(&1, 1);
+			assert_eq!(Council::voters(), vec![1, 2]);
+			assert_noop!{Council::retract_voter(&1, 1)};	// gives "retraction index mismatch"
 		});
 	}
 
 	#[test]
-	#[should_panic(expected = "retraction index invalid")]
-	fn overflow_retraction_index_should_panic() {
+	fn overflow_retraction_index_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(1);
 			Council::submit_candidacy(&3, 0);
 			Council::set_approvals(&1, vec![true], 0);
-			Council::retract_voter(&1, 1);
+			assert_noop!{Council::retract_voter(&1, 1)};	// gives "retraction index invalid"
 		});
 	}
 
 	#[test]
-	#[should_panic(expected = "cannot retract non-voter")]
-	fn non_voter_retraction_should_panic() {
+	fn non_voter_retraction_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(1);
 			Council::submit_candidacy(&3, 0);
 			Council::set_approvals(&1, vec![true], 0);
-			Council::retract_voter(&2, 0);
+			assert_noop!{Council::retract_voter(&2, 0)};	// gives "cannot retract non-voter"
 		});
 	}
 
@@ -1028,8 +1022,7 @@ mod tests {
 	}
 
 	#[test]
-	#[should_panic(expected = "candidate must not form a duplicated member if elected")]
-	fn presenting_for_double_election_should_panic() {
+	fn presenting_for_double_election_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(4);
 			Council::submit_candidacy(&2, 0);
@@ -1046,7 +1039,7 @@ mod tests {
 			Council::end_block(System::block_number());
 
 			System::set_block_number(10);
-			Council::present_winner(&4, 2, 11, 1);
+			assert_noop!{Council::present_winner(&4, 2, 11, 1)};	// gives "candidate must not form a duplicated member if elected"
 		});
 	}
 
@@ -1088,8 +1081,7 @@ mod tests {
 	}
 
 	#[test]
-	#[should_panic(expected = "bad reporter index")]
-	fn retracting_inactive_voter_with_bad_reporter_index_should_panic() {
+	fn retracting_inactive_voter_with_bad_reporter_index_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(4);
 			Council::submit_candidacy(&2, 0);
@@ -1109,17 +1101,16 @@ mod tests {
 			Council::present_winner(&4, 5, 38, 1);
 			Council::end_block(System::block_number());
 
-			Council::reap_inactive_voter(&2,
+			assert_noop!{Council::reap_inactive_voter(&2,
 				42,
 				2, Council::voters().iter().position(|&i| i == 2).unwrap() as u32,
 				2
-			);
+			)};	// given "bad reporter index"
 		});
 	}
 
 	#[test]
-	#[should_panic(expected = "bad target index")]
-	fn retracting_inactive_voter_with_bad_target_index_should_panic() {
+	fn retracting_inactive_voter_with_bad_target_index_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(4);
 			Council::submit_candidacy(&2, 0);
@@ -1139,11 +1130,11 @@ mod tests {
 			Council::present_winner(&4, 5, 38, 1);
 			Council::end_block(System::block_number());
 
-			Council::reap_inactive_voter(&2,
+			assert_noop!{Council::reap_inactive_voter(&2,
 				Council::voters().iter().position(|&i| i == 2).unwrap() as u32,
 				2, 42,
 				2
-			);
+			)};	// gives "bad target index"
 		});
 	}
 
@@ -1190,8 +1181,7 @@ mod tests {
 	}
 
 	#[test]
-	#[should_panic(expected = "reaper must be a voter")]
-	fn attempting_to_retract_inactive_voter_by_nonvoter_should_panic() {
+	fn attempting_to_retract_inactive_voter_by_nonvoter_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(4);
 			Council::submit_candidacy(&2, 0);
@@ -1211,17 +1201,16 @@ mod tests {
 			Council::present_winner(&4, 5, 41, 1);
 			Council::end_block(System::block_number());
 
-			Council::reap_inactive_voter(&4,
+			assert_noop!{Council::reap_inactive_voter(&4,
 				0,
 				2, Council::voters().iter().position(|&i| i == 2).unwrap() as u32,
 				2
-			);
+			)};	// gives "reaper must be a voter"
 		});
 	}
 
 	#[test]
-	#[should_panic(expected = "candidate not worthy of leaderboard")]
-	fn presenting_loser_should_panic() {
+	fn presenting_loser_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(4);
 			Council::submit_candidacy(&1, 0);
@@ -1241,7 +1230,7 @@ mod tests {
 			Council::present_winner(&4, 3, 21, 0);
 			Council::present_winner(&4, 4, 31, 0);
 			Council::present_winner(&4, 5, 41, 0);
-			Council::present_winner(&4, 2, 11, 0);
+			assert_noop!{Council::present_winner(&4, 2, 11, 0)};	// gives "candidate not worthy of leaderboard"
 		});
 	}
 
@@ -1278,18 +1267,16 @@ mod tests {
 	}
 
 	#[test]
-	#[should_panic(expected = "cannot present outside of presentation period")]
-	fn present_panics_outside_of_presentation_period() {
+	fn present_outside_of_presentation_period_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(4);
 			assert!(!Council::presentation_active());
-			Council::present_winner(&5, 5, 1, 0);
+			assert_noop!{Council::present_winner(&5, 5, 1, 0)};	// gives "cannot present outside of presentation period"
 		});
 	}
 
 	#[test]
-	#[should_panic(expected = "index not current")]
-	fn present_panics_with_invalid_vote_index() {
+	fn present_with_invalid_vote_index_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(4);
 			Council::submit_candidacy(&2, 0);
@@ -1299,13 +1286,12 @@ mod tests {
 			Council::end_block(System::block_number());
 
 			System::set_block_number(6);
-			Council::present_winner(&4, 2, 11, 1);
+			assert_noop!{Council::present_winner(&4, 2, 11, 1)};	// gives "index not current"
 		});
 	}
 
 	#[test]
-	#[should_panic(expected = "presenter must have sufficient slashable funds")]
-	fn present_panics_when_presenter_is_poor() {
+	fn present_when_presenter_is_poor_should_not_work() {
 		with_externalities(&mut new_test_ext(false), || {
 			System::set_block_number(4);
 			assert!(!Council::presentation_active());
@@ -1318,7 +1304,7 @@ mod tests {
 
 			System::set_block_number(6);
 			assert_eq!(Staking::balance(&1), 1);
-			Council::present_winner(&1, 1, 30, 0);
+			assert_noop!{Council::present_winner(&1, 1, 30, 0)};	// gives "presenter must have sufficient slashable funds"
 		});
 	}
 

substrate/substrate/runtime/council/src/voting.rs

@@ -75,12 +75,12 @@ impl<T: Trait> Module<T> {
 	// Dispatch
 	fn propose(aux: &T::PublicAux, proposal: Box<T::Proposal>) {
 		let expiry = <system::Module<T>>::block_number() + Self::voting_period();
-		assert!(Self::will_still_be_councillor_at(aux.ref_into(), expiry));
+		ensure!(Self::will_still_be_councillor_at(aux.ref_into(), expiry));
 
 		let proposal_hash = T::Hashing::hash_of(&proposal);
 
-		assert!(!<ProposalOf<T>>::exists(proposal_hash), "No duplicate proposals allowed");
-		assert!(!Self::is_vetoed(&proposal_hash));
+		ensure!(!<ProposalOf<T>>::exists(proposal_hash), "No duplicate proposals allowed");
+		ensure!(!Self::is_vetoed(&proposal_hash));
 
 		let mut proposals = Self::proposals();
 		proposals.push((expiry, proposal_hash));
@@ -102,14 +102,14 @@ impl<T: Trait> Module<T> {
 	}
 
 	fn veto(aux: &T::PublicAux, proposal_hash: T::Hash) {
-		assert!(Self::is_councillor(aux.ref_into()), "only councillors may veto council proposals");
-		assert!(<ProposalVoters<T>>::exists(&proposal_hash), "proposal must exist to be vetoed");
+		ensure!(Self::is_councillor(aux.ref_into()), "only councillors may veto council proposals");
+		ensure!(<ProposalVoters<T>>::exists(&proposal_hash), "proposal must exist to be vetoed");
 
 		let mut existing_vetoers = Self::veto_of(&proposal_hash)
 			.map(|pair| pair.1)
 			.unwrap_or_else(Vec::new);
-		let insert_position = existing_vetoers.binary_search(aux.ref_into())
-			.expect_err("a councillor may not veto a proposal twice");
+		let insert_position = ensure_unwrap_err!(existing_vetoers.binary_search(aux.ref_into()),
+			"a councillor may not veto a proposal twice");
 		existing_vetoers.insert(insert_position, aux.ref_into().clone());
 		Self::set_veto_of(&proposal_hash, <system::Module<T>>::block_number() + Self::cooloff_period(), existing_vetoers);
 
@@ -163,8 +163,7 @@ impl<T: Trait> Module<T> {
 			Some(&(expiry, hash)) if expiry == n => {
 				// yes this is horrible, but fixing it will need substantial work in storage.
 				Self::set_proposals(&proposals[1..].to_vec());
-				let proposal = <ProposalOf<T>>::take(hash).expect("all queued proposal hashes must have associated proposals");
-				Some((proposal, hash))
+				<ProposalOf<T>>::take(hash).map(|p| (p, hash))	/* defensive only: all queued proposal hashes must have associated proposals*/
 			}
 			_ => None,
 		}
@@ -311,8 +310,7 @@ mod tests {
 	}
 
 	#[test]
-	#[should_panic]
-	fn double_veto_should_panic() {
+	fn double_veto_should_not_work() {
 		with_externalities(&mut new_test_ext(true), || {
 			System::set_block_number(1);
 			let proposal = bonding_duration_proposal(42);
@@ -322,13 +320,12 @@ mod tests {
 
 			System::set_block_number(3);
 			CouncilVoting::propose(&1, Box::new(proposal.clone()));
-			CouncilVoting::veto(&2, hash);
+			assert_noop!{CouncilVoting::veto(&2, hash)};
 		});
 	}
 
 	#[test]
-	#[should_panic]
-	fn retry_in_cooloff_should_panic() {
+	fn retry_in_cooloff_should_not_work() {
 		with_externalities(&mut new_test_ext(true), || {
 			System::set_block_number(1);
 			let proposal = bonding_duration_proposal(42);
@@ -337,7 +334,7 @@ mod tests {
 			CouncilVoting::veto(&2, hash);
 
 			System::set_block_number(2);
-			CouncilVoting::propose(&1, Box::new(proposal.clone()));
+			assert_noop!{CouncilVoting::propose(&1, Box::new(proposal.clone()))};
 		});
 	}
 
@@ -447,12 +444,11 @@ mod tests {
 	}
 
 	#[test]
-	#[should_panic]
-	fn propose_by_public_should_panic() {
+	fn propose_by_public_should_not_work() {
 		with_externalities(&mut new_test_ext(true), || {
 			System::set_block_number(1);
 			let proposal = bonding_duration_proposal(42);
-			CouncilVoting::propose(&4, Box::new(proposal));
+			assert_noop!{CouncilVoting::propose(&4, Box::new(proposal))};
 		});
 	}
 }

substrate/substrate/runtime/democracy/src/lib.rs

@@ -102,7 +102,7 @@ impl<T: Trait> Module<T> {
 
 	// exposed immutables.
 
-	/// Get the amount locked in support of `proposal`; false if proposal isn't a valid proposal
+	/// Get the amount locked in support of `proposal`; `None` if proposal isn't a valid proposal
 	/// index.
 	pub fn locked_for(proposal: PropIndex) -> Option<T::Balance> {
 		Self::deposit_of(proposal).map(|(d, l)| d * T::Balance::sa(l.len()))
@@ -135,7 +135,7 @@ impl<T: Trait> Module<T> {
 	/// Get the voters for the current proposal.
 	pub fn tally(ref_index: ReferendumIndex) -> (T::Balance, T::Balance) {
 		Self::voters_for(ref_index).iter()
-			.map(|a| (<staking::Module<T>>::balance(a), Self::vote_of((ref_index, a.clone())).expect("all items come from `voters`; for an item to be in `voters` there must be a vote registered; qed")))
+			.map(|a| (<staking::Module<T>>::balance(a), Self::vote_of((ref_index, a.clone())).unwrap_or(false)/*defensive only: all items come from `voters`; for an item to be in `voters` there must be a vote registered; qed*/))
 			.map(|(bal, vote)| if vote { (bal, Zero::zero()) } else { (Zero::zero(), bal) })
 			.fold((Zero::zero(), Zero::zero()), |(a, b), (c, d)| (a + c, b + d))
 	}
@@ -144,8 +144,8 @@ impl<T: Trait> Module<T> {
 
 	/// Propose a sensitive action to be taken.
 	fn propose(aux: &T::PublicAux, proposal: Box<T::Proposal>, value: T::Balance) {
-		assert!(value >= Self::minimum_deposit());
-		assert!(<staking::Module<T>>::deduct_unbonded(aux.ref_into(), value));
+		ensure!(value >= Self::minimum_deposit());
+		ensure!(<staking::Module<T>>::deduct_unbonded(aux.ref_into(), value));
 
 		let index = Self::public_prop_count();
 		<PublicPropCount<T>>::put(index + 1);
@@ -158,21 +158,23 @@ impl<T: Trait> Module<T> {
 
 	/// Propose a sensitive action to be taken.
 	fn second(aux: &T::PublicAux, proposal: PropIndex) {
-		let mut deposit = Self::deposit_of(proposal).expect("can only second an existing proposal");
-		assert!(<staking::Module<T>>::deduct_unbonded(aux.ref_into(), deposit.0));
-
-		deposit.1.push(aux.ref_into().clone());
-		<DepositOf<T>>::insert(proposal, deposit);
+		if let Some(mut deposit) = Self::deposit_of(proposal) {
+			ensure!(<staking::Module<T>>::deduct_unbonded(aux.ref_into(), deposit.0));
+			deposit.1.push(aux.ref_into().clone());
+			<DepositOf<T>>::insert(proposal, deposit);
+		} else {
+			fail!("can only second an existing proposal");
+		}
 	}
 
 	/// Vote in a referendum. If `approve_proposal` is true, the vote is to enact the proposal;
 	/// false would be a vote to keep the status quo..
 	fn vote(aux: &T::PublicAux, ref_index: ReferendumIndex, approve_proposal: bool) {
 		if !Self::is_active_referendum(ref_index) {
-			panic!("vote given for invalid referendum.")
+			fail!("vote given for invalid referendum.")
 		}
 		if <staking::Module<T>>::balance(aux.ref_into()).is_zero() {
-			panic!("transactor must have balance to signal approval.");
+			fail!("transactor must have balance to signal approval.");
 		}
 		if !<VoteOf<T>>::exists(&(ref_index, aux.ref_into().clone())) {
 			let mut voters = Self::voters_for(ref_index);
@@ -184,7 +186,7 @@ impl<T: Trait> Module<T> {
 
 	/// Start a referendum.
 	fn start_referendum(proposal: Box<T::Proposal>, vote_threshold: VoteThreshold) {
-		Self::inject_referendum(<system::Module<T>>::block_number() + Self::voting_period(), *proposal, vote_threshold);
+		let _ = Self::inject_referendum(<system::Module<T>>::block_number() + Self::voting_period(), *proposal, vote_threshold);
 	}
 
 	/// Remove a referendum.
@@ -196,7 +198,7 @@ impl<T: Trait> Module<T> {
 
 	/// Start a referendum. Can be called directly by the council.
 	pub fn internal_start_referendum(proposal: T::Proposal, vote_threshold: VoteThreshold) {
-		<Module<T>>::inject_referendum(<system::Module<T>>::block_number() + <Module<T>>::voting_period(), proposal, vote_threshold);
+		let _ = <Module<T>>::inject_referendum(<system::Module<T>>::block_number() + <Module<T>>::voting_period(), proposal, vote_threshold);
 	}
 
 	/// Remove a referendum. Can be called directly by the council.
@@ -211,15 +213,15 @@ impl<T: Trait> Module<T> {
 		end: T::BlockNumber,
 		proposal: T::Proposal,
 		vote_threshold: VoteThreshold
-	) -> ReferendumIndex {
+	) -> Result<ReferendumIndex, &'static str> {
 		let ref_index = Self::referendum_count();
 		if ref_index > 0 && Self::referendum_info(ref_index - 1).map(|i| i.0 > end).unwrap_or(false) {
-			panic!("Cannot inject a referendum that ends earlier than preceeding referendum");
+			Err("Cannot inject a referendum that ends earlier than preceeding referendum")?
 		}
 
 		<ReferendumCount<T>>::put(ref_index + 1);
 		<ReferendumInfoOf<T>>::insert(ref_index, (end, proposal, vote_threshold));
-		ref_index
+		Ok(ref_index)
 	}
 
 	/// Remove all info on a referendum.
@@ -232,23 +234,25 @@ impl<T: Trait> Module<T> {
 	}
 
 	/// Current era is ending; we should finish up any proposals.
-	fn end_block(now: T::BlockNumber) {
+	fn end_block(now: T::BlockNumber) -> Result<(), &'static str> {
 		// pick out another public referendum if it's time.
 		if (now % Self::launch_period()).is_zero() {
 			let mut public_props = Self::public_props();
 			if let Some((winner_index, _)) = public_props.iter()
 				.enumerate()
-				.max_by_key(|x| Self::locked_for((x.1).0).expect("All current public proposals have an amount locked"))
+				.max_by_key(|x| Self::locked_for((x.1).0).unwrap_or_else(Zero::zero)/*defensive only: All current public proposals have an amount locked*/)
 			{
 				let (prop_index, proposal, _) = public_props.swap_remove(winner_index);
-				let (deposit, depositors): (T::Balance, Vec<T::AccountId>) =
-					<DepositOf<T>>::take(prop_index).expect("depositors always exist for current proposals");
-				// refund depositors
-				for d in &depositors {
-					<staking::Module<T>>::refund(d, deposit);
+				if let Some((deposit, depositors)) = <DepositOf<T>>::take(prop_index) {//: (T::Balance, Vec<T::AccountId>) =
+					// refund depositors
+					for d in &depositors {
+						<staking::Module<T>>::refund(d, deposit);
+					}
+					<PublicProps<T>>::put(public_props);
+					Self::inject_referendum(now + Self::voting_period(), proposal, VoteThreshold::SuperMajorityApprove)?;
+				} else {
+					Err("depositors always exist for current proposals")?
 				}
-				<PublicProps<T>>::put(public_props);
-				Self::inject_referendum(now + Self::voting_period(), proposal, VoteThreshold::SuperMajorityApprove);
 			}
 		}
 
@@ -262,12 +266,15 @@ impl<T: Trait> Module<T> {
 			}
 			<NextTally<T>>::put(index + 1);
 		}
+		Ok(())
 	}
 }
 
 impl<T: Trait> Executable for Module<T> {
 	fn execute() {
-		Self::end_block(<system::Module<T>>::block_number());
+		if let Err(e) = Self::end_block(<system::Module<T>>::block_number()) {
+			runtime_io::print(e);
+		}
 	}
 }
 
@@ -388,7 +395,8 @@ mod tests {
 			intentions: vec![],
 			validator_count: 2,
 			bonding_duration: 3,
-			transaction_fee: 0,
+			transaction_base_fee: 0,
+			transaction_byte_fee: 0,
 		}.build_externalities());
 		t.extend(GenesisConfig::<Test>{
 			launch_period: 1,
@@ -437,7 +445,7 @@ mod tests {
 		with_externalities(&mut new_test_ext(), || {
 			System::set_block_number(1);
 			propose_sessions_per_era(1, 2, 1);
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 
 			System::set_block_number(2);
 			let r = 0;
@@ -448,7 +456,7 @@ mod tests {
 			assert_eq!(Democracy::vote_of((r, 1)), Some(true));
 			assert_eq!(Democracy::tally(r), (10, 0));
 
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 			Staking::check_new_era();
 
 			assert_eq!(Staking::era_length(), 2);
@@ -479,7 +487,7 @@ mod tests {
 			Democracy::second(&5, 0);
 			Democracy::second(&5, 0);
 			Democracy::second(&5, 0);
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 			assert_eq!(Staking::balance(&1), 10);
 			assert_eq!(Staking::balance(&2), 20);
 			assert_eq!(Staking::balance(&5), 50);
@@ -487,35 +495,35 @@ mod tests {
 	}
 
 	#[test]
-	#[should_panic]
 	fn proposal_with_deposit_below_minimum_should_panic() {
 		with_externalities(&mut new_test_ext(), || {
 			System::set_block_number(1);
 			propose_sessions_per_era(1, 2, 0);
+			assert_eq!(Democracy::locked_for(0), None);
 		});
 	}
 
 	#[test]
-	#[should_panic]
 	fn poor_proposer_should_panic() {
 		with_externalities(&mut new_test_ext(), || {
 			System::set_block_number(1);
 			propose_sessions_per_era(1, 2, 11);
+			assert_eq!(Democracy::locked_for(0), None);
 		});
 	}
 
 	#[test]
-	#[should_panic]
 	fn poor_seconder_should_panic() {
 		with_externalities(&mut new_test_ext(), || {
 			System::set_block_number(1);
 			propose_sessions_per_era(2, 2, 11);
 			Democracy::second(&1, 0);
+			assert_eq!(Democracy::locked_for(0), Some(11));
 		});
 	}
 
 	fn propose_bonding_duration(who: u64, value: u64, locked: u64) {
-			Democracy::propose(&who, Box::new(Proposal::Staking(staking::PrivCall::set_bonding_duration(value))), locked);
+		Democracy::propose(&who, Box::new(Proposal::Staking(staking::PrivCall::set_bonding_duration(value))), locked);
 	}
 
 	#[test]
@@ -525,23 +533,23 @@ mod tests {
 			propose_bonding_duration(1, 2, 2);
 			propose_bonding_duration(1, 4, 4);
 			propose_bonding_duration(1, 3, 3);
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 
 			System::set_block_number(1);
 			Democracy::vote(&1, 0, true);
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 			Staking::check_new_era();
 			assert_eq!(Staking::bonding_duration(), 4);
 
 			System::set_block_number(2);
 			Democracy::vote(&1, 1, true);
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 			Staking::check_new_era();
 			assert_eq!(Staking::bonding_duration(), 3);
 
 			System::set_block_number(3);
 			Democracy::vote(&1, 2, true);
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 			Staking::check_new_era();
 			assert_eq!(Staking::bonding_duration(), 2);
 		});
@@ -555,14 +563,14 @@ mod tests {
 	fn simple_passing_should_work() {
 		with_externalities(&mut new_test_ext(), || {
 			System::set_block_number(1);
-			let r = Democracy::inject_referendum(1, sessions_per_era_proposal(2), VoteThreshold::SuperMajorityApprove);
+			let r = Democracy::inject_referendum(1, sessions_per_era_proposal(2), VoteThreshold::SuperMajorityApprove).unwrap();
 			Democracy::vote(&1, r, true);
 
 			assert_eq!(Democracy::voters_for(r), vec![1]);
 			assert_eq!(Democracy::vote_of((r, 1)), Some(true));
 			assert_eq!(Democracy::tally(r), (10, 0));
 
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 			Staking::check_new_era();
 
 			assert_eq!(Staking::era_length(), 2);
@@ -573,11 +581,11 @@ mod tests {
 	fn cancel_referendum_should_work() {
 		with_externalities(&mut new_test_ext(), || {
 			System::set_block_number(1);
-			let r = Democracy::inject_referendum(1, sessions_per_era_proposal(2), VoteThreshold::SuperMajorityApprove);
+			let r = Democracy::inject_referendum(1, sessions_per_era_proposal(2), VoteThreshold::SuperMajorityApprove).unwrap();
 			Democracy::vote(&1, r, true);
 			Democracy::cancel_referendum(r);
 
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 			Staking::check_new_era();
 
 			assert_eq!(Staking::era_length(), 1);
@@ -588,14 +596,14 @@ mod tests {
 	fn simple_failing_should_work() {
 		with_externalities(&mut new_test_ext(), || {
 			System::set_block_number(1);
-			let r = Democracy::inject_referendum(1, sessions_per_era_proposal(2), VoteThreshold::SuperMajorityApprove);
+			let r = Democracy::inject_referendum(1, sessions_per_era_proposal(2), VoteThreshold::SuperMajorityApprove).unwrap();
 			Democracy::vote(&1, r, false);
 
 			assert_eq!(Democracy::voters_for(r), vec![1]);
 			assert_eq!(Democracy::vote_of((r, 1)), Some(false));
 			assert_eq!(Democracy::tally(r), (0, 10));
 
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 			Staking::check_new_era();
 
 			assert_eq!(Staking::era_length(), 1);
@@ -606,7 +614,7 @@ mod tests {
 	fn controversial_voting_should_work() {
 		with_externalities(&mut new_test_ext(), || {
 			System::set_block_number(1);
-			let r = Democracy::inject_referendum(1, sessions_per_era_proposal(2), VoteThreshold::SuperMajorityApprove);
+			let r = Democracy::inject_referendum(1, sessions_per_era_proposal(2), VoteThreshold::SuperMajorityApprove).unwrap();
 			Democracy::vote(&1, r, true);
 			Democracy::vote(&2, r, false);
 			Democracy::vote(&3, r, false);
@@ -616,7 +624,7 @@ mod tests {
 
 			assert_eq!(Democracy::tally(r), (110, 100));
 
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 			Staking::check_new_era();
 
 			assert_eq!(Staking::era_length(), 2);
@@ -627,13 +635,13 @@ mod tests {
 	fn controversial_low_turnout_voting_should_work() {
 		with_externalities(&mut new_test_ext(), || {
 			System::set_block_number(1);
-			let r = Democracy::inject_referendum(1, sessions_per_era_proposal(2), VoteThreshold::SuperMajorityApprove);
+			let r = Democracy::inject_referendum(1, sessions_per_era_proposal(2), VoteThreshold::SuperMajorityApprove).unwrap();
 			Democracy::vote(&5, r, false);
 			Democracy::vote(&6, r, true);
 
 			assert_eq!(Democracy::tally(r), (60, 50));
 
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 			Staking::check_new_era();
 
 			assert_eq!(Staking::era_length(), 1);
@@ -647,14 +655,14 @@ mod tests {
 			assert_eq!(Staking::total_stake(), 210);
 
 			System::set_block_number(1);
-			let r = Democracy::inject_referendum(1, sessions_per_era_proposal(2), VoteThreshold::SuperMajorityApprove);
+			let r = Democracy::inject_referendum(1, sessions_per_era_proposal(2), VoteThreshold::SuperMajorityApprove).unwrap();
 			Democracy::vote(&4, r, true);
 			Democracy::vote(&5, r, false);
 			Democracy::vote(&6, r, true);
 
 			assert_eq!(Democracy::tally(r), (100, 50));
 
-			Democracy::end_block(System::block_number());
+			assert_eq!(Democracy::end_block(System::block_number()), Ok(()));
 			Staking::check_new_era();
 
 			assert_eq!(Staking::era_length(), 2);

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

@@ -97,7 +97,7 @@ impl<
 
 		// execute transactions
 		let (header, extrinsics) = block.deconstruct();
-		extrinsics.into_iter().for_each(Self::apply_extrinsic_inner);
+		extrinsics.into_iter().for_each(Self::apply_extrinsic_no_note);
 
 		// post-transactional book-keeping.
 		Finalisation::execute();
@@ -116,33 +116,42 @@ impl<
 		<system::Module<System>>::finalise()
 	}
 
-	/// Apply outside of the block execution function.
+	/// Apply extrinsic outside of the block execution function.
 	/// This doesn't attempt to validate anything regarding the block, but it builds a list of uxt
 	/// hashes.
 	pub fn apply_extrinsic(uxt: Block::Extrinsic) {
-		<system::Module<System>>::note_extrinsic(uxt.encode());
-		Self::apply_extrinsic_inner(uxt);
+		let encoded = uxt.encode();
+		let encoded_len = encoded.len();
+		<system::Module<System>>::note_extrinsic(encoded);
+		Self::apply_extrinsic_no_note_with_len(uxt, encoded_len);
 	}
 
-	/// Apply outside of the block execution function.
-	/// This doesn't attempt to validate anything regarding the block.
-	fn apply_extrinsic_inner(uxt: Block::Extrinsic) {
+	/// Apply an extrinsic inside the block execution function.
+	fn apply_extrinsic_no_note(uxt: Block::Extrinsic) {
+		let l = uxt.encode().len();
+		Self::apply_extrinsic_no_note_with_len(uxt, l);
+	}
+
+	/// 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) {
 		// Verify the signature is good.
 		let xt = match uxt.check() {
 			Ok(xt) => xt,
-			Err(_) => panic!("All transactions should be properly signed"),
+			Err(_) => panic!("All extrinsics should be properly signed"),
 		};
 
 		if xt.sender() != &Default::default() {
 			// check index
 			let expected_index = <system::Module<System>>::account_index(xt.sender());
-			assert!(xt.index() == &expected_index, "All transactions should have the correct nonce");
+			assert!(xt.index() == &expected_index, "All extrinsics should have the correct nonce");
+
+			// pay any fees.
+			assert!(Payment::make_payment(xt.sender(), encoded_len), "All extrinsics should have sender able to pay their fees");
+
+			// AUDIT: Under no circumstances may this function panic from here onwards.
 
 			// increment nonce in storage
 			<system::Module<System>>::inc_account_index(xt.sender());
-
-			// pay any fees.
-			Payment::make_payment(xt.sender());
 		}
 
 		// decode parameters and dispatch
@@ -213,7 +222,8 @@ mod tests {
 			intentions: vec![],
 			validator_count: 0,
 			bonding_duration: 0,
-			transaction_fee: 10,
+			transaction_base_fee: 10,
+			transaction_byte_fee: 0,
 		}.build_externalities());
 		let xt = primitives::testing::TestXt((1, 0, Call::transfer(2, 69)));
 		with_externalities(&mut t, || {
@@ -239,7 +249,7 @@ mod tests {
 				header: Header {
 					parent_hash: [69u8; 32].into(),
 					number: 1,
-					state_root: hex!("aa0cff04242e55fc780861b890aa8deba555f6ed95bd8fa575dfd80864f3b93e").into(),
+					state_root: hex!("1d43ef0fcabb78d925093fe22e50cc9ca5d182d189a3407c778e5fca714177dd").into(),
 					extrinsics_root: hex!("56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421").into(),
 					digest: Digest { logs: vec![], },
 				},
@@ -273,7 +283,7 @@ mod tests {
 				header: Header {
 					parent_hash: [69u8; 32].into(),
 					number: 1,
-					state_root: hex!("aa0cff04242e55fc780861b890aa8deba555f6ed95bd8fa575dfd80864f3b93e").into(),
+					state_root: hex!("1d43ef0fcabb78d925093fe22e50cc9ca5d182d189a3407c778e5fca714177dd").into(),
 					extrinsics_root: [0u8; 32].into(),
 					digest: Digest { logs: vec![], },
 				},

substrate/substrate/runtime/primitives/src/traits.rs

@@ -35,12 +35,13 @@ pub trait Verify {
 
 /// Simple payment making trait, operating on a single generic `AccountId` type.
 pub trait MakePayment<AccountId> {
-	/// Make some sort of payment concerning `who`.
-	fn make_payment(who: &AccountId);
+	/// Make some sort of payment concerning `who` for an extrinsic (transaction) of encoded length
+	/// `encoded_len` bytes. Return true iff the payment was successful.
+	fn make_payment(who: &AccountId, encoded_len: usize) -> bool;
 }
 
 impl<T> MakePayment<T> for () {
-	fn make_payment(_: &T) {}
+	fn make_payment(_: &T, _: usize) -> bool { true }
 }
 
 /// Extensible conversion trait. Generic over both source and destination types.

substrate/substrate/runtime/staking/src/contract.rs

@@ -205,6 +205,7 @@ pub(crate) fn execute<'a, 'b: 'a, T: Trait>(
 		let account = e.account().clone();
 		if let Some(commit_state) =
 			Module::<T>::effect_transfer(&account, &transfer_to, value, e.account_db())
+				.map_err(|_| sandbox::Error::Execution)?
 		{
 			e.account_db_mut().merge(commit_state);
 		}
@@ -231,6 +232,7 @@ pub(crate) fn execute<'a, 'b: 'a, T: Trait>(
 		let account = e.account().clone();
 		if let Some(commit_state) =
 			Module::<T>::effect_create(&account, &code, value, e.account_db())
+				.map_err(|_| sandbox::Error::Execution)?
 		{
 			e.account_db_mut().merge(commit_state);
 		}

substrate/substrate/runtime/staking/src/lib.rs

@@ -52,7 +52,7 @@ use rstd::cell::RefCell;
 use rstd::collections::btree_map::{BTreeMap, Entry};
 use codec::Slicable;
 use runtime_support::{StorageValue, StorageMap, Parameter};
-use primitives::traits::{Zero, One, Bounded, RefInto, SimpleArithmetic, Executable, MakePayment};
+use primitives::traits::{Zero, One, Bounded, RefInto, SimpleArithmetic, Executable, MakePayment, As};
 
 mod contract;
 #[cfg(test)]
@@ -122,8 +122,10 @@ decl_storage! {
 	pub SessionsPerEra get(sessions_per_era): b"sta:spe" => required T::BlockNumber;
 	// The total amount of stake on the system.
 	pub TotalStake get(total_stake): b"sta:tot" => required T::Balance;
-	// The fee to be paid for making a transaction.
-	pub TransactionFee get(transaction_fee): b"sta:fee" => required T::Balance;
+	// The fee to be paid for making a transaction; the base.
+	pub TransactionBaseFee get(transaction_base_fee): b"sta:basefee" => required T::Balance;
+	// The fee to be paid for making a transaction; the per-byte portion.
+	pub TransactionByteFee get(transaction_byte_fee): b"sta:bytefee" => required T::Balance;
 
 	// The current era index.
 	pub CurrentEra get(current_era): b"sta:era" => required T::BlockNumber;
@@ -165,12 +167,22 @@ impl<T: Trait> Module<T> {
 		Self::free_balance(who) + Self::reserved_balance(who)
 	}
 
-	/// Some result as `slash(who, value)` (but without the side-effects) asuming there are no
+	/// Some result as `slash(who, value)` (but without the side-effects) assuming there are no
 	/// balance changes in the meantime.
 	pub fn can_slash(who: &T::AccountId, value: T::Balance) -> bool {
 		Self::balance(who) >= value
 	}
 
+	/// Same result as `deduct_unbonded(who, value)` (but without the side-effects) assuming there
+	/// are no balance changes in the meantime.
+	pub fn can_deduct_unbonded(who: &T::AccountId, value: T::Balance) -> bool {
+		if let LockStatus::Liquid = Self::unlock_block(who) {
+			Self::free_balance(who) >= value
+		} else {
+			false
+		}
+	}
+
 	/// The block at which the `who`'s funds become entirely liquid.
 	pub fn unlock_block(who: &T::AccountId) -> LockStatus<T::BlockNumber> {
 		match Self::bondage(who) {
@@ -183,7 +195,7 @@ impl<T: Trait> Module<T> {
 	/// Create a smart-contract account.
 	pub fn create(aux: &T::PublicAux, code: &[u8], value: T::Balance) {
 		// commit anything that made it this far to storage
-		if let Some(commit) = Self::effect_create(aux.ref_into(), code, value, &DirectAccountDb) {
+		if let Ok(Some(commit)) = Self::effect_create(aux.ref_into(), code, value, &DirectAccountDb) {
 			<AccountDb<T>>::merge(&mut DirectAccountDb, commit);
 		}
 	}
@@ -194,7 +206,7 @@ impl<T: Trait> Module<T> {
 	/// TODO: probably want to state gas-limit and gas-price.
 	fn transfer(aux: &T::PublicAux, dest: T::AccountId, value: T::Balance) {
 		// commit anything that made it this far to storage
-		if let Some(commit) = Self::effect_transfer(aux.ref_into(), &dest, value, &DirectAccountDb) {
+		if let Ok(Some(commit)) = Self::effect_transfer(aux.ref_into(), &dest, value, &DirectAccountDb) {
 			<AccountDb<T>>::merge(&mut DirectAccountDb, commit);
 		}
 	}
@@ -205,7 +217,7 @@ impl<T: Trait> Module<T> {
 	fn stake(aux: &T::PublicAux) {
 		let mut intentions = <Intentions<T>>::get();
 		// can't be in the list twice.
-		assert!(intentions.iter().find(|&t| t == aux.ref_into()).is_none(), "Cannot stake if already staked.");
+		ensure!(intentions.iter().find(|&t| t == aux.ref_into()).is_none(), "Cannot stake if already staked.");
 		intentions.push(aux.ref_into().clone());
 		<Intentions<T>>::put(intentions);
 		<Bondage<T>>::insert(aux.ref_into(), T::BlockNumber::max_value());
@@ -218,11 +230,11 @@ impl<T: Trait> Module<T> {
 		let mut intentions = <Intentions<T>>::get();
 		if let Some(position) = intentions.iter().position(|t| t == aux.ref_into()) {
 			intentions.swap_remove(position);
+			<Intentions<T>>::put(intentions);
+			<Bondage<T>>::insert(aux.ref_into(), Self::current_era() + Self::bonding_duration());
 		} else {
-			panic!("Cannot unstake if not already staked.");
+			fail!("Cannot unstake if not already staked.");
 		}
-		<Intentions<T>>::put(intentions);
-		<Bondage<T>>::insert(aux.ref_into(), Self::current_era() + Self::bonding_duration());
 	}
 
 	// PRIV DISPATCH
@@ -280,11 +292,14 @@ impl<T: Trait> Module<T> {
 	}
 
 	/// Moves `value` from balance to reserved balance.
-	pub fn reserve_balance(who: &T::AccountId, value: T::Balance) {
+	pub fn reserve_balance(who: &T::AccountId, value: T::Balance) -> bool {
 		let b = Self::free_balance(who);
-		assert!(b >= value);
+		if b < value {
+			return false;
+		}
 		<FreeBalance<T>>::insert(who, b - value);
 		<ReservedBalance<T>>::insert(who, Self::reserved_balance(who) + value);
+		true
 	}
 
 	/// Moves `value` from reserved balance to balance.
@@ -536,26 +551,28 @@ impl<T: Trait> Module<T> {
 		code: &[u8],
 		value: T::Balance,
 		account_db: &DB,
-	) -> Option<State<T>> {
+	) -> Result<Option<State<T>>, &'static str> {
 		let from_balance = account_db.get_balance(transactor);
 		// TODO: a fee.
-		assert!(from_balance >= value);
+		if from_balance < value {
+			return Err("balance too low to send value");
+		}
 
 		let dest = T::DetermineContractAddress::contract_address_for(code, transactor);
 
 		// early-out if degenerate.
 		if &dest == transactor {
-			return None;
+			return Ok(None);
 		}
 
 		let mut local = BTreeMap::new();
 
 		// two inserts are safe
-		assert!(&dest != transactor);
+		// note that we now know that `&dest != transactor` due to early-out before.
 		local.insert(dest, ChangeEntry { balance: Some(value), code: Some(code.to_vec()), storage: Default::default() });
 		local.insert(transactor.clone(), ChangeEntry::balance_changed(from_balance - value));
 
-		Some(local)
+		Ok(Some(local))
 	}
 
 	fn effect_transfer<DB: AccountDb<T>>(
@@ -563,13 +580,19 @@ impl<T: Trait> Module<T> {
 		dest: &T::AccountId,
 		value: T::Balance,
 		account_db: &DB,
-	) -> Option<State<T>> {
+	) -> Result<Option<State<T>>, &'static str> {
 		let from_balance = account_db.get_balance(transactor);
-		assert!(from_balance >= value);
+		if from_balance < value {
+			return Err("balance too low to send value");
+		}
 
 		let to_balance = account_db.get_balance(dest);
-		assert!(<Bondage<T>>::get(transactor) <= <Bondage<T>>::get(dest));
-		assert!(to_balance + value > to_balance); // no overflow
+		if <Bondage<T>>::get(transactor) > <Bondage<T>>::get(dest) {
+			return Err("bondage too high to send value");
+		}
+		if to_balance + value <= to_balance {
+			return Err("destination balance too high to receive value");
+		}
 
 		// TODO: a fee, based upon gaslimit/gasprice.
 		let gas_limit = 100_000;
@@ -594,20 +617,23 @@ impl<T: Trait> Module<T> {
 			contract::execute(&dest_code, dest, &mut overlay, gas_limit).is_ok()
 		};
 
-		if should_commit {
+		Ok(if should_commit {
 			Some(overlay.into_state())
 		} else {
 			None
-		}
+		})
 	}
 }
 
 impl<T: Trait> MakePayment<T::AccountId> for Module<T> {
-	fn make_payment(transactor: &T::AccountId) {
+	fn make_payment(transactor: &T::AccountId, encoded_len: usize) -> bool {
 		let b = Self::free_balance(transactor);
-		let transaction_fee = Self::transaction_fee();
-		assert!(b >= transaction_fee, "attempt to transact without enough funds to pay fee");
+		let transaction_fee = Self::transaction_base_fee() + Self::transaction_byte_fee() * <T::Balance as As<usize>>::sa(encoded_len);
+		if b < transaction_fee {
+			return false;
+		}
 		<FreeBalance<T>>::insert(transactor, b - transaction_fee);
+		true
 	}
 }
 
@@ -628,7 +654,8 @@ pub struct GenesisConfig<T: Trait> {
 	pub intentions: Vec<T::AccountId>,
 	pub validator_count: u64,
 	pub bonding_duration: T::BlockNumber,
-	pub transaction_fee: T::Balance,
+	pub transaction_base_fee: T::Balance,
+	pub transaction_byte_fee: T::Balance,
 }
 
 #[cfg(any(feature = "std", test))]
@@ -642,7 +669,8 @@ impl<T: Trait> GenesisConfig<T> where T::AccountId: From<u64> {
 			intentions: vec![T::AccountId::from(1), T::AccountId::from(2), T::AccountId::from(3)],
 			validator_count: 3,
 			bonding_duration: T::BlockNumber::sa(0),
-			transaction_fee: T::Balance::sa(0),
+			transaction_base_fee: T::Balance::sa(0),
+			transaction_byte_fee: T::Balance::sa(0),
 		}
 	}
 
@@ -663,7 +691,8 @@ impl<T: Trait> GenesisConfig<T> where T::AccountId: From<u64> {
 			intentions: vec![T::AccountId::from(1), T::AccountId::from(2), T::AccountId::from(3)],
 			validator_count: 3,
 			bonding_duration: T::BlockNumber::sa(0),
-			transaction_fee: T::Balance::sa(1),
+			transaction_base_fee: T::Balance::sa(1),
+			transaction_byte_fee: T::Balance::sa(0),
 		}
 	}
 }
@@ -679,7 +708,8 @@ impl<T: Trait> Default for GenesisConfig<T> {
 			intentions: vec![],
 			validator_count: 0,
 			bonding_duration: T::BlockNumber::sa(1000),
-			transaction_fee: T::Balance::sa(0),
+			transaction_base_fee: T::Balance::sa(0),
+			transaction_byte_fee: T::Balance::sa(0),
 		}
 	}
 }
@@ -697,7 +727,8 @@ impl<T: Trait> primitives::BuildExternalities for GenesisConfig<T> {
 			twox_128(<SessionsPerEra<T>>::key()).to_vec() => self.sessions_per_era.encode(),
 			twox_128(<ValidatorCount<T>>::key()).to_vec() => self.validator_count.encode(),
 			twox_128(<BondingDuration<T>>::key()).to_vec() => self.bonding_duration.encode(),
-			twox_128(<TransactionFee<T>>::key()).to_vec() => self.transaction_fee.encode(),
+			twox_128(<TransactionBaseFee<T>>::key()).to_vec() => self.transaction_base_fee.encode(),
+			twox_128(<TransactionByteFee<T>>::key()).to_vec() => self.transaction_byte_fee.encode(),
 			twox_128(<CurrentEra<T>>::key()).to_vec() => self.current_era.encode(),
 			twox_128(<TotalStake<T>>::key()).to_vec() => total_stake.encode()
 		];
@@ -854,12 +885,12 @@ mod tests {
 	}
 
 	#[test]
-	#[should_panic]
 	fn staking_balance_transfer_when_bonded_panics() {
 		with_externalities(&mut new_test_ext(1, 3, 1, false), || {
 			<FreeBalance<Test>>::insert(1, 111);
 			Staking::stake(&1);
 			Staking::transfer(&1, 2, 69);
+			assert_eq!(Staking::balance(&1), 111);
 		});
 	}
 
@@ -880,13 +911,13 @@ mod tests {
 		});
 	}
 
-	#[test]
 	#[should_panic]
 	fn staking_balance_transfer_when_reserved_panics() {
 		with_externalities(&mut new_test_ext(1, 3, 1, false), || {
 			<FreeBalance<Test>>::insert(1, 111);
 			Staking::reserve_balance(&1, 69);
 			Staking::transfer(&1, 2, 69);
+			assert_eq!(Staking::balance(&1), 111);
 		});
 	}
 

substrate/substrate/runtime/staking/src/mock.rs

@@ -68,7 +68,8 @@ pub fn new_test_ext(session_length: u64, sessions_per_era: u64, current_era: u64
 		intentions: vec![],
 		validator_count: 2,
 		bonding_duration: 3,
-		transaction_fee: 0,
+		transaction_base_fee: 0,
+		transaction_byte_fee: 0,
 	}.build_externalities());
 	t
 }