avoid redundancy in native code compatibility check

polkadot/api/src/lib.rs

@@ -85,6 +85,7 @@ impl From<client::error::Error> for Error {
 	}
 }
 
+/// A builder for blocks.
 pub trait BlockBuilder: Sized {
 	/// Push a non-inherent transaction.
 	fn push_transaction(&mut self, transaction: UncheckedTransaction) -> Result<()>;
@@ -93,40 +94,60 @@ pub trait BlockBuilder: Sized {
 	fn bake(self) -> Block;
 }
 
+/// A checked block identifier.
+pub trait CheckedBlockId {
+	/// Yield the underlying block ID.
+	fn block_id(&self) -> &BlockId;
+}
+
 /// Trait encapsulating the Polkadot API.
 ///
 /// All calls should fail when the exact runtime is unknown.
 pub trait PolkadotApi {
+	/// A checked block ID. Used to avoid redundancy of code check.
+	type CheckedBlockId: CheckedBlockId;
+	/// The type used to build blocks.
 	type BlockBuilder: BlockBuilder;
 
+	/// Check whether requests at the given block ID can be served.
+	///
+	/// It should not be possible to instantiate this type without going
+	/// through this function.
+	fn check_id(&self, id: BlockId) -> Result<Self::CheckedBlockId>;
+
 	/// Get session keys at a given block.
-	fn session_keys(&self, at: &BlockId) -> Result<Vec<SessionKey>>;
+	fn session_keys(&self, at: &Self::CheckedBlockId) -> Result<Vec<SessionKey>>;
 
 	/// Get validators at a given block.
-	fn validators(&self, at: &BlockId) -> Result<Vec<AccountId>>;
+	fn validators(&self, at: &Self::CheckedBlockId) -> Result<Vec<AccountId>>;
 
 	/// Get the authority duty roster at a block.
-	fn duty_roster(&self, at: &BlockId) -> Result<DutyRoster>;
+	fn duty_roster(&self, at: &Self::CheckedBlockId) -> Result<DutyRoster>;
 
 	/// Get the timestamp registered at a block.
-	fn timestamp(&self, at: &BlockId) -> Result<Timestamp>;
+	fn timestamp(&self, at: &Self::CheckedBlockId) -> Result<Timestamp>;
 
 	/// Evaluate a block and see if it gives an error.
-	fn evaluate_block(&self, at: &BlockId, block: Block) -> Result<()>;
+	fn evaluate_block(&self, at: &Self::CheckedBlockId, block: Block) -> Result<()>;
 
 	/// Create a block builder on top of the parent block.
-	fn build_block(&self, parent: &BlockId, timestamp: u64) -> Result<Self::BlockBuilder>;
+	fn build_block(&self, parent: &Self::CheckedBlockId, timestamp: u64) -> Result<Self::BlockBuilder>;
+}
+
+/// A checked block ID used for the substrate-client implementation of CheckedBlockId;
+#[derive(Debug, Clone, Copy)]
+pub struct CheckedId(BlockId);
+
+impl CheckedBlockId for CheckedId {
+	fn block_id(&self) -> &BlockId {
+		&self.0
+	}
 }
 
 // set up the necessary scaffolding to execute the runtime.
 macro_rules! with_runtime {
 	($client: ident, $at: expr, $exec: expr) => {{
-		// bail if the code is not the same as the natively linked.
-		if $client.code_at($at)? != LocalDispatch::native_equivalent() {
-			bail!(ErrorKind::UnknownRuntime);
-		}
-
-		$client.state_at($at).map_err(Error::from).and_then(|state| {
+		$client.state_at($at.block_id()).map_err(Error::from).and_then(|state| {
 			let mut changes = Default::default();
 			let mut ext = state_machine::Ext {
 				overlay: &mut changes,
@@ -141,33 +162,40 @@ macro_rules! with_runtime {
 impl<B: Backend> PolkadotApi for Client<B, NativeExecutor<LocalDispatch>>
 	where ::client::error::Error: From<<<B as Backend>::State as state_machine::backend::Backend>::Error>
 {
+	type CheckedBlockId = CheckedId;
 	type BlockBuilder = ClientBlockBuilder<B::State>;
 
-	fn session_keys(&self, at: &BlockId) -> Result<Vec<SessionKey>> {
+	fn check_id(&self, id: BlockId) -> Result<CheckedId> {
+		// bail if the code is not the same as the natively linked.
+		if self.code_at(&id)? != LocalDispatch::native_equivalent() {
+			bail!(ErrorKind::UnknownRuntime);
+		}
+
+		Ok(CheckedId(id))
+	}
+
+	fn session_keys(&self, at: &CheckedId) -> Result<Vec<SessionKey>> {
 		with_runtime!(self, at, ::runtime::consensus::authorities)
 	}
 
-	fn validators(&self, at: &BlockId) -> Result<Vec<AccountId>> {
+	fn validators(&self, at: &CheckedId) -> Result<Vec<AccountId>> {
 		with_runtime!(self, at, ::runtime::session::validators)
 	}
 
-	fn duty_roster(&self, at: &BlockId) -> Result<DutyRoster> {
+	fn duty_roster(&self, at: &CheckedId) -> Result<DutyRoster> {
 		with_runtime!(self, at, ::runtime::parachains::calculate_duty_roster)
 	}
 
-	fn timestamp(&self, at: &BlockId) -> Result<Timestamp> {
+	fn timestamp(&self, at: &CheckedId) -> Result<Timestamp> {
 		with_runtime!(self, at, ::runtime::timestamp::get)
 	}
 
-	fn evaluate_block(&self, at: &BlockId, block: Block) -> Result<()> {
+	fn evaluate_block(&self, at: &CheckedId, block: Block) -> Result<()> {
 		with_runtime!(self, at, || ::runtime::system::internal::execute_block(block))
 	}
 
-	fn build_block(&self, parent: &BlockId, timestamp: Timestamp) -> Result<Self::BlockBuilder> {
-		if self.code_at(parent)? != LocalDispatch::native_equivalent() {
-			bail!(ErrorKind::UnknownRuntime);
-		}
-
+	fn build_block(&self, parent: &CheckedId, timestamp: Timestamp) -> Result<Self::BlockBuilder> {
+		let parent = parent.block_id();
 		let header = Header {
 			parent_hash: self.block_hash_from_id(parent)?.ok_or(ErrorKind::UnknownBlock(*parent))?,
 			number: self.block_number_from_id(parent)?.ok_or(ErrorKind::UnknownBlock(*parent))? + 1,
@@ -316,23 +344,24 @@ mod tests {
 	#[test]
 	fn gets_session_and_validator_keys() {
 		let client = client();
-		assert_eq!(client.session_keys(&BlockId::Number(0)).unwrap(), validators());
-		assert_eq!(client.validators(&BlockId::Number(0)).unwrap(), validators());
+		let id = client.check_id(BlockId::Number(0)).unwrap();
+		assert_eq!(client.session_keys(&id).unwrap(), validators());
+		assert_eq!(client.validators(&id).unwrap(), validators());
 	}
 
 	#[test]
 	fn build_block() {
 		let client = client();
 
-		let block_builder = client.build_block(&BlockId::Number(0), 1_000_000).unwrap();
+		let id = client.check_id(BlockId::Number(0)).unwrap();
+		let block_builder = client.build_block(&id, 1_000_000).unwrap();
 		let block = block_builder.bake();
 
 		assert_eq!(block.header.number, 1);
 	}
 
 	#[test]
-	fn cannot_build_block_on_unknown_parent() {
-		let client = client();
-		assert!(client.build_block(&BlockId::Number(100), 1_000_000).is_err());
+	fn fails_to_check_id_for_unknown_block() {
+		assert!(client().check_id(BlockId::Number(100)).is_err());
 	}
 }

polkadot/consensus/src/lib.rs

@@ -465,7 +465,9 @@ impl<C: PolkadotApi, N: Network> bft::ProposerFactory for ProposerFactory<C, N>
 
 	fn init(&self, parent_header: &SubstrateHeader, authorities: &[AuthorityId], sign_with: Arc<ed25519::Pair>) -> Result<Self::Proposer, Error> {
 		let parent_hash = parent_header.hash();
-		let duty_roster = self.client.duty_roster(&BlockId::Hash(parent_hash))?;
+
+		let checked_id = self.client.check_id(BlockId::Hash(parent_hash))?;
+		let duty_roster = self.client.duty_roster(&checked_id)?;
 
 		let group_info = make_group_info(duty_roster, authorities)?;
 		let table = Arc::new(SharedTable::new(group_info, sign_with, parent_hash));
@@ -474,6 +476,7 @@ impl<C: PolkadotApi, N: Network> bft::ProposerFactory for ProposerFactory<C, N>
 		// TODO [PoC-2]: kick off collation process.
 		Ok(Proposer {
 			parent_hash,
+			parent_id: checked_id,
 			_table: table,
 			_router: router,
 			client: self.client.clone(),
@@ -490,8 +493,9 @@ fn current_timestamp() -> Timestamp {
 }
 
 /// The Polkadot proposer logic.
-pub struct Proposer<C, R> {
+pub struct Proposer<C: PolkadotApi, R> {
 	parent_hash: HeaderHash,
+	parent_id: C::CheckedBlockId,
 	client: Arc<C>,
 	_table: Arc<SharedTable>,
 	_router: R,
@@ -505,7 +509,7 @@ impl<C: PolkadotApi, R: TableRouter> bft::Proposer for Proposer<C, R> {
 	fn propose(&self) -> Result<SubstrateBlock, Error> {
 		// TODO: handle case when current timestamp behind that in state.
 		let polkadot_block = self.client.build_block(
-			&BlockId::Hash(self.parent_hash),
+			&self.parent_id,
 			current_timestamp()
 		)?.bake();
 
@@ -519,7 +523,7 @@ impl<C: PolkadotApi, R: TableRouter> bft::Proposer for Proposer<C, R> {
 
 	// TODO: certain kinds of errors here should lead to a misbehavior report.
 	fn evaluate(&self, proposal: &SubstrateBlock) -> Result<bool, Error> {
-		evaluate_proposal(proposal, &*self.client, current_timestamp(), &self.parent_hash)
+		evaluate_proposal(proposal, &*self.client, current_timestamp(), &self.parent_hash, &self.parent_id)
 	}
 }
 
@@ -528,6 +532,7 @@ fn evaluate_proposal<C: PolkadotApi>(
 	client: &C,
 	now: Timestamp,
 	parent_hash: &HeaderHash,
+	parent_id: &C::CheckedBlockId,
 ) -> Result<bool, Error> {
 	const MAX_TIMESTAMP_DRIFT: Timestamp = 4;
 
@@ -551,6 +556,6 @@ fn evaluate_proposal<C: PolkadotApi>(
 	}
 
 	// execute the block.
-	client.evaluate_block(&BlockId::Hash(*parent_hash), proposal)?;
+	client.evaluate_block(parent_id, proposal)?;
 	Ok(true)
 }