Make on_slot return the block with the post header (#8188)

* Make `on_slot` return the block with the post header

Before this pr `on_slot` returned the pre block. However this is wrong,
because adding some post digest changes the hash of the header. Thus,
we need to make sure to return the correct block that uses the post
header.

* Update primitives/consensus/common/src/block_import.rs

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

substrate/client/consensus/aura/src/lib.rs

@@ -683,7 +683,7 @@ impl<B: BlockT, C, P, CAW> Verifier<B> for AuraVerifier<C, P, CAW> where
 }
 
 fn initialize_authorities_cache<A, B, C>(client: &C) -> Result<(), ConsensusError> where
-	A: Codec,
+	A: Codec + Debug,
 	B: BlockT,
 	C: ProvideRuntimeApi<B> + BlockOf + ProvideCache<B>,
 	C::Api: AuraApi<B, A>,
@@ -719,7 +719,7 @@ fn initialize_authorities_cache<A, B, C>(client: &C) -> Result<(), ConsensusErro
 
 #[allow(deprecated)]
 fn authorities<A, B, C>(client: &C, at: &BlockId<B>) -> Result<Vec<A>, ConsensusError> where
-	A: Codec,
+	A: Codec + Debug,
 	B: BlockT,
 	C: ProvideRuntimeApi<B> + BlockOf + ProvideCache<B>,
 	C::Api: AuraApi<B, A>,
@@ -886,7 +886,7 @@ mod tests {
 	use sc_client_api::BlockchainEvents;
 	use sp_consensus_aura::sr25519::AuthorityPair;
 	use sc_consensus_slots::{SimpleSlotWorker, BackoffAuthoringOnFinalizedHeadLagging};
-	use std::task::Poll;
+	use std::{task::Poll, time::Instant};
 	use sc_block_builder::BlockBuilderProvider;
 	use sp_runtime::traits::Header as _;
 	use substrate_test_runtime_client::{TestClient, runtime::{Header, H256}};
@@ -1124,4 +1124,51 @@ mod tests {
 		assert!(worker.claim_slot(&head, 6.into(), &authorities).is_none());
 		assert!(worker.claim_slot(&head, 7.into(), &authorities).is_some());
 	}
+
+	#[test]
+	fn on_slot_returns_correct_block() {
+		let net = AuraTestNet::new(4);
+
+		let keystore_path = tempfile::tempdir().expect("Creates keystore path");
+		let keystore = LocalKeystore::open(keystore_path.path(), None)
+			.expect("Creates keystore.");
+		SyncCryptoStore::sr25519_generate_new(
+			&keystore,
+			AuthorityPair::ID, Some(&Keyring::Alice.to_seed()),
+		).expect("Key should be created");
+
+		let net = Arc::new(Mutex::new(net));
+
+		let mut net = net.lock();
+		let peer = net.peer(3);
+		let client = peer.client().as_full().expect("full clients are created").clone();
+		let environ = DummyFactory(client.clone());
+
+		let mut worker = AuraWorker {
+			client: client.clone(),
+			block_import: Arc::new(Mutex::new(client.clone())),
+			env: environ,
+			keystore: keystore.into(),
+			sync_oracle: DummyOracle.clone(),
+			force_authoring: false,
+			backoff_authoring_blocks: Option::<()>::None,
+			_key_type: PhantomData::<AuthorityPair>,
+		};
+
+		let head = client.header(&BlockId::Number(0)).unwrap().unwrap();
+
+		let res = futures::executor::block_on(worker.on_slot(
+			head,
+			SlotInfo {
+				slot: 0.into(),
+				timestamp: 0,
+				ends_at: Instant::now() + Duration::from_secs(100),
+				inherent_data: InherentData::new(),
+				duration: 1000,
+			},
+		)).unwrap();
+
+		// The returned block should be imported and we should be able to get its header by now.
+		assert!(client.header(&BlockId::Hash(res.block.hash())).unwrap().is_some());
+	}
 }

substrate/client/consensus/slots/src/lib.rs

@@ -334,7 +334,7 @@ pub trait SimpleSlotWorker<B: BlockT> {
 
 		proposal_work.and_then(move |(proposal, claim)| async move {
 			let (block, storage_proof) = (proposal.block, proposal.proof);
-			let (header, body) = block.clone().deconstruct();
+			let (header, body) = block.deconstruct();
 			let header_num = *header.number();
 			let header_hash = header.hash();
 			let parent_hash = *header.parent_hash();
@@ -342,7 +342,7 @@ pub trait SimpleSlotWorker<B: BlockT> {
 			let block_import_params = block_import_params_maker(
 				header,
 				&header_hash,
-				body,
+				body.clone(),
 				proposal.storage_changes,
 				claim,
 				epoch_data,
@@ -361,6 +361,7 @@ pub trait SimpleSlotWorker<B: BlockT> {
 				"hash_previously" => ?header_hash,
 			);
 
+			let header = block_import_params.post_header();
 			if let Err(err) = block_import.lock().import_block(block_import_params, Default::default()) {
 				warn!(
 					target: logging_target,
@@ -376,7 +377,7 @@ pub trait SimpleSlotWorker<B: BlockT> {
 				);
 			}
 
-			Ok(SlotResult { block, storage_proof })
+			Ok(SlotResult { block: B::new(header, body), storage_proof })
 		}).then(|r| async move {
 			r.map_err(|e| warn!(target: "slots", "Encountered consensus error: {:?}", e)).ok()
 		}).boxed()

substrate/primitives/consensus/common/src/block_import.rs

@@ -193,16 +193,21 @@ impl<Block: BlockT, Transaction> BlockImportParams<Block, Transaction> {
 		if let Some(hash) = self.post_hash {
 			hash
 		} else {
-			if self.post_digests.is_empty() {
-				self.header.hash()
-			} else {
-				let mut hdr = self.header.clone();
-				for digest_item in &self.post_digests {
-					hdr.digest_mut().push(digest_item.clone());
-				}
+			self.post_header().hash()
+		}
+	}
 
-				hdr.hash()
+	/// Get the post header.
+	pub fn post_header(&self) -> Block::Header {
+		if self.post_digests.is_empty() {
+			self.header.clone()
+		} else {
+			let mut hdr = self.header.clone();
+			for digest_item in &self.post_digests {
+				hdr.digest_mut().push(digest_item.clone());
 			}
+
+			hdr
 		}
 	}