Revert "Header-only sync for old forks (#3942)" (#4022)

This reverts commit ac78c90.

Author: gavofyork

core/client/db/src/lib.rs

@@ -894,12 +894,6 @@ impl<Block: BlockT<Hash=H256>> Backend<Block> {
 		inmem
 	}
 
-	/// Returns total numbet of blocks (headers) in the block DB.
-	#[cfg(feature = "test-helpers")]
-	pub fn blocks_count(&self) -> u64 {
-		self.blockchain.db.iter(columns::HEADER).count() as u64
-	}
-
 	/// Read (from storage or cache) changes trie config.
 	///
 	/// Currently changes tries configuration is set up once (at genesis) and could not
@@ -1121,7 +1115,7 @@ impl<Block: BlockT<Hash=H256>> Backend<Block> {
 			);
 
 			transaction.put(columns::HEADER, &lookup_key, &pending_block.header.encode());
-			if let Some(body) = &pending_block.body {
+			if let Some(body) = pending_block.body {
 				transaction.put(columns::BODY, &lookup_key, &body.encode());
 			}
 			if let Some(justification) = pending_block.justification {
@@ -1133,26 +1127,21 @@ impl<Block: BlockT<Hash=H256>> Backend<Block> {
 				transaction.put(columns::META, meta_keys::GENESIS_HASH, hash.as_ref());
 			}
 
-			let finalized = if pending_block.body.is_some() {
-				let mut changeset: state_db::ChangeSet<Vec<u8>> = state_db::ChangeSet::default();
-				for (key, (val, rc)) in operation.db_updates.drain() {
-					if rc > 0 {
-						changeset.inserted.push((key, val.to_vec()));
-					} else if rc < 0 {
-						changeset.deleted.push(key);
-					}
+			let mut changeset: state_db::ChangeSet<Vec<u8>> = state_db::ChangeSet::default();
+			for (key, (val, rc)) in operation.db_updates.drain() {
+				if rc > 0 {
+					changeset.inserted.push((key, val.to_vec()));
+				} else if rc < 0 {
+					changeset.deleted.push(key);
 				}
-				let number_u64 = number.saturated_into::<u64>();
-				let commit = self.storage.state_db.insert_block(&hash, number_u64, &pending_block.header.parent_hash(), changeset)
-					.map_err(|e: state_db::Error<io::Error>| client::error::Error::from(format!("State database error: {:?}", e)))?;
-				apply_state_commit(&mut transaction, commit);
-
-				// Check if need to finalize. Genesis is always finalized instantly.
-				let finalized = number_u64 == 0 || pending_block.leaf_state.is_final();
-				finalized
-			} else {
-				false
-			};
+			}
+			let number_u64 = number.saturated_into::<u64>();
+			let commit = self.storage.state_db.insert_block(&hash, number_u64, &pending_block.header.parent_hash(), changeset)
+				.map_err(|e: state_db::Error<io::Error>| client::error::Error::from(format!("State database error: {:?}", e)))?;
+			apply_state_commit(&mut transaction, commit);
+
+			// Check if need to finalize. Genesis is always finalized instantly.
+			let finalized = number_u64 == 0 || pending_block.leaf_state.is_final();
 
 			let header = &pending_block.header;
 			let is_best = pending_block.leaf_state.is_best();
@@ -1592,7 +1581,7 @@ mod tests {
 		};
 		let mut op = backend.begin_operation().unwrap();
 		backend.begin_state_operation(&mut op, block_id).unwrap();
-		op.set_block_data(header, Some(Vec::new()), None, NewBlockState::Best).unwrap();
+		op.set_block_data(header, None, None, NewBlockState::Best).unwrap();
 		op.update_changes_trie((changes_trie_update, ChangesTrieCacheAction::Clear)).unwrap();
 		backend.commit_operation(op).unwrap();
 

core/client/src/client.rs

@@ -927,39 +927,22 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 			BlockOrigin::Genesis | BlockOrigin::NetworkInitialSync | BlockOrigin::File => false,
 		};
 
-		let storage_changes = match &body {
-			Some(body) => {
-				self.backend.begin_state_operation(&mut operation.op, BlockId::Hash(parent_hash))?;
-
-				// ensure parent block is finalized to maintain invariant that
-				// finality is called sequentially.
-				if finalized {
-					self.apply_finality_with_block_hash(operation, parent_hash, None, info.best_hash, make_notifications)?;
-				}
+		self.backend.begin_state_operation(&mut operation.op, BlockId::Hash(parent_hash))?;
 
-				// FIXME #1232: correct path logic for when to execute this function
-				let (storage_update, changes_update, storage_changes) = self.block_execution(
-					&operation.op,
-					&import_headers,
-					origin,
-					hash,
-					&body,
-				)?;
+		// ensure parent block is finalized to maintain invariant that
+		// finality is called sequentially.
+		if finalized {
+			self.apply_finality_with_block_hash(operation, parent_hash, None, info.best_hash, make_notifications)?;
+		}
 
-				operation.op.update_cache(new_cache);
-				if let Some(storage_update) = storage_update {
-					operation.op.update_db_storage(storage_update)?;
-				}
-				if let Some(storage_changes) = storage_changes.clone() {
-					operation.op.update_storage(storage_changes.0, storage_changes.1)?;
-				}
-				if let Some(Some(changes_update)) = changes_update {
-					operation.op.update_changes_trie(changes_update)?;
-				}
-				storage_changes
-			},
-			None => Default::default()
-		};
+		// FIXME #1232: correct path logic for when to execute this function
+		let (storage_update, changes_update, storage_changes) = self.block_execution(
+			&operation.op,
+			&import_headers,
+			origin,
+			hash,
+			body.clone(),
+		)?;
 
 		let is_new_best = finalized || match fork_choice {
 			ForkChoiceStrategy::LongestChain => import_headers.post().number() > &info.best_number,
@@ -994,6 +977,17 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 			leaf_state,
 		)?;
 
+		operation.op.update_cache(new_cache);
+		if let Some(storage_update) = storage_update {
+			operation.op.update_db_storage(storage_update)?;
+		}
+		if let Some(storage_changes) = storage_changes.clone() {
+			operation.op.update_storage(storage_changes.0, storage_changes.1)?;
+		}
+		if let Some(Some(changes_update)) = changes_update {
+			operation.op.update_changes_trie(changes_update)?;
+		}
+
 		operation.op.insert_aux(aux)?;
 
 		if make_notifications {
@@ -1020,7 +1014,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 		import_headers: &PrePostHeader<Block::Header>,
 		origin: BlockOrigin,
 		hash: Block::Hash,
-		body: &[Block::Extrinsic],
+		body: Option<Vec<Block::Extrinsic>>,
 	) -> error::Result<(
 		Option<StorageUpdate<B, Block>>,
 		Option<Option<ChangesUpdate<Block>>>,
@@ -1058,7 +1052,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 
 				let encoded_block = <Block as BlockT>::encode_from(
 					import_headers.pre(),
-					body,
+					&body.unwrap_or_default()
 				);
 
 				let (_, storage_update, changes_update) = self.executor
@@ -1529,7 +1523,7 @@ impl<'a, B, E, Block, RA> consensus::BlockImport<Block> for &'a Client<B, E, Blo
 		&mut self,
 		block: BlockCheckParams<Block>,
 	) -> Result<ImportResult, Self::Error> {
-		let BlockCheckParams { hash, number, parent_hash, header_only } = block;
+		let BlockCheckParams { hash, number, parent_hash } = block;
 
 		if let Some(h) = self.fork_blocks.as_ref().and_then(|x| x.get(&number)) {
 			if &hash != h  {
@@ -1547,9 +1541,7 @@ impl<'a, B, E, Block, RA> consensus::BlockImport<Block> for &'a Client<B, E, Blo
 			.map_err(|e| ConsensusError::ClientImport(e.to_string()))?
 		{
 			BlockStatus::InChainWithState | BlockStatus::Queued => {},
-			BlockStatus::Unknown => return Ok(ImportResult::UnknownParent),
-			BlockStatus::InChainPruned if header_only => {},
-			BlockStatus::InChainPruned => return Ok(ImportResult::MissingState),
+			BlockStatus::Unknown | BlockStatus::InChainPruned => return Ok(ImportResult::UnknownParent),
 			BlockStatus::KnownBad => return Ok(ImportResult::KnownBad),
 		}
 
@@ -1561,6 +1553,7 @@ impl<'a, B, E, Block, RA> consensus::BlockImport<Block> for &'a Client<B, E, Blo
 			BlockStatus::KnownBad => return Ok(ImportResult::KnownBad),
 		}
 
+
 		Ok(ImportResult::imported(false))
 	}
 }

core/consensus/common/src/block_import.rs

@@ -35,15 +35,11 @@ pub enum ImportResult {
 	KnownBad,
 	/// Block parent is not in the chain.
 	UnknownParent,
-	/// Parent state is missing.
-	MissingState,
 }
 
 /// Auxiliary data associated with an imported block result.
 #[derive(Debug, Default, PartialEq, Eq)]
 pub struct ImportedAux {
-	/// Only the header has been imported. Block body verification was skipped.
-	pub header_only: bool,
 	/// Clear all pending justification requests.
 	pub clear_justification_requests: bool,
 	/// Request a justification for the given block.
@@ -102,8 +98,6 @@ pub struct BlockCheckParams<Block: BlockT> {
 	pub number: NumberFor<Block>,
 	/// Parent hash of the block that we verify.
 	pub parent_hash: Block::Hash,
-	/// Don't check state availability
-	pub header_only: bool,
 }
 
 /// Data required to import a Block.

core/consensus/common/src/import_queue.rs

@@ -203,10 +203,6 @@ pub fn import_single_block<B: BlockT, V: Verifier<B>>(
 				Ok(BlockImportResult::ImportedKnown(number))
 			},
 			Ok(ImportResult::Imported(aux)) => Ok(BlockImportResult::ImportedUnknown(number, aux, peer.clone())),
-			Ok(ImportResult::MissingState) => {
-				debug!(target: "sync", "Parent state is missing for {}: {:?}, parent: {:?}", number, hash, parent_hash);
-				Err(BlockImportError::UnknownParent)
-			},
 			Ok(ImportResult::UnknownParent) => {
 				debug!(target: "sync", "Block with unknown parent {}: {:?}, parent: {:?}", number, hash, parent_hash);
 				Err(BlockImportError::UnknownParent)
@@ -221,12 +217,7 @@ pub fn import_single_block<B: BlockT, V: Verifier<B>>(
 			}
 		}
 	};
-	match import_error(import_handle.check_block(BlockCheckParams {
-		hash,
-		number,
-		parent_hash,
-		header_only: block.body.is_none(),
-	}))? {
+	match import_error(import_handle.check_block(BlockCheckParams { hash, number, parent_hash }))? {
 		BlockImportResult::ImportedUnknown { .. } => (),
 		r => return Ok(r), // Any other successful result means that the block is already imported.
 	}

core/finality-grandpa/src/light_import.rs

@@ -680,7 +680,6 @@ pub mod tests {
 			bad_justification: false,
 			needs_finality_proof: false,
 			is_new_best: true,
-			header_only: false,
 		}));
 	}
 
@@ -693,7 +692,6 @@ pub mod tests {
 			bad_justification: false,
 			needs_finality_proof: false,
 			is_new_best: true,
-			header_only: false,
 		}));
 	}
 
@@ -707,7 +705,6 @@ pub mod tests {
 			bad_justification: false,
 			needs_finality_proof: true,
 			is_new_best: true,
-			header_only: false,
 		}));
 	}
 
@@ -724,7 +721,6 @@ pub mod tests {
 				bad_justification: false,
 				needs_finality_proof: true,
 				is_new_best: false,
-				header_only: false,
 			},
 		));
 	}

core/finality-grandpa/src/tests.rs

@@ -984,7 +984,6 @@ fn allows_reimporting_change_blocks() {
 			bad_justification: false,
 			needs_finality_proof: false,
 			is_new_best: true,
-			header_only: false,
 		}),
 	);