Approve block announcements of backed blocks (#394)

* Approve block announcements of backed blocks If we receive a block announcement without a statement attached that matches the latest backed block, it is valid and we need to approve the block announcement to download the block. * Fix tests * Approve block announcement if it comes from the best known block * Fetch backed block only when required
2026-06-12 05:21:08 +00:00 · 2021-04-12 13:54:07 +02:00
parent dfdee4cb52
commit b35deaed95
2 changed files with 129 additions and 45 deletions
@@ -34,6 +34,7 @@ use sp_runtime::{
 };

 use polkadot_node_primitives::{SignedFullStatement, Statement};
+use polkadot_parachain::primitives::HeadData;
 use polkadot_primitives::v1::{
 	Block as PBlock, CandidateReceipt, CompactStatement, Hash as PHash, Id as ParaId,
 	OccupiedCoreAssumption, ParachainHost, SignedStatement, SigningContext,
@@ -55,7 +56,7 @@ use wait_on_relay_chain_block::WaitOnRelayChainBlock;
 mod tests;
 mod wait_on_relay_chain_block;

-const LOG_TARGET: &str = "cumulus-network";
+const LOG_TARGET: &str = "sync::cumulus";

 type BoxedError = Box<dyn std::error::Error + Send>;

@@ -102,9 +103,7 @@ impl BlockAnnounceData {
 			return Err(Validation::Failure { disconnect: true });
 		}

-		if polkadot_parachain::primitives::HeadData(encoded_header).hash()
-			!= self.receipt.descriptor.para_head
-		{
+		if HeadData(encoded_header).hash() != self.receipt.descriptor.para_head {
 			tracing::debug!(
 				target: LOG_TARGET,
 				"Receipt para head hash doesn't match the hash of the header in the block announcement",
@@ -223,19 +222,19 @@ impl TryFrom<SignedFullStatement> for BlockAnnounceData {
 /// chain. If it is at the tip, it is required to provide a justification or otherwise we reject
 /// it. However, if the announcement is for a block below the tip the announcement is accepted
 /// as it probably comes from a node that is currently syncing the chain.
-pub struct BlockAnnounceValidator<Block, P, B, BCE> {
+pub struct BlockAnnounceValidator<Block, R, B, BCE> {
 	phantom: PhantomData<Block>,
-	relay_chain_client: Arc<P>,
+	relay_chain_client: Arc<R>,
 	relay_chain_backend: Arc<B>,
 	para_id: ParaId,
 	relay_chain_sync_oracle: Box<dyn SyncOracle + Send>,
 	wait_on_relay_chain_block: WaitOnRelayChainBlock<B, BCE>,
 }

-impl<Block, P, B, BCE> BlockAnnounceValidator<Block, P, B, BCE> {
+impl<Block, R, B, BCE> BlockAnnounceValidator<Block, R, B, BCE> {
 	/// Create a new [`BlockAnnounceValidator`].
 	pub fn new(
-		relay_chain_client: Arc<P>,
+		relay_chain_client: Arc<R>,
 		para_id: ParaId,
 		relay_chain_sync_oracle: Box<dyn SyncOracle + Send>,
 		relay_chain_backend: Arc<B>,
@@ -255,14 +254,54 @@ impl<Block, P, B, BCE> BlockAnnounceValidator<Block, P, B, BCE> {
 	}
 }

-impl<Block: BlockT, P, B, BCE> BlockAnnounceValidator<Block, P, B, BCE>
+impl<Block: BlockT, R, B, BCE> BlockAnnounceValidator<Block, R, B, BCE>
 where
-	P: ProvideRuntimeApi<PBlock> + Send + Sync + 'static,
-	P::Api: ParachainHost<PBlock>,
+	R: ProvideRuntimeApi<PBlock> + Send + Sync + 'static,
+	R::Api: ParachainHost<PBlock>,
 	B: Backend<PBlock> + 'static,
 	// Rust bug: https://github.com/rust-lang/rust/issues/24159
 	sc_client_api::StateBackendFor<B, PBlock>: sc_client_api::StateBackend<HashFor<PBlock>>,
 {
+	/// Get the included block of the given parachain in the relay chain.
+	fn included_block(
+		relay_chain_client: &R,
+		block_id: &BlockId<PBlock>,
+		para_id: ParaId,
+	) -> Result<Block::Header, BoxedError> {
+		let validation_data = relay_chain_client
+			.runtime_api()
+			.persisted_validation_data(block_id, para_id, OccupiedCoreAssumption::TimedOut)
+			.map_err(|e| Box::new(BlockAnnounceError(format!("{:?}", e))) as Box<_>)?
+			.ok_or_else(|| {
+				Box::new(BlockAnnounceError(
+					"Could not find parachain head in relay chain".into(),
+				)) as Box<_>
+			})?;
+		let para_head =
+			Block::Header::decode(&mut &validation_data.parent_head.0[..]).map_err(|e| {
+				Box::new(BlockAnnounceError(format!(
+					"Failed to decode parachain head: {:?}",
+					e
+				))) as Box<_>
+			})?;
+
+		Ok(para_head)
+	}
+
+	/// Get the backed block hash of the given parachain in the relay chain.
+	fn backed_block_hash(
+		relay_chain_client: &R,
+		block_id: &BlockId<PBlock>,
+		para_id: ParaId,
+	) -> Result<Option<PHash>, BoxedError> {
+		let candidate_receipt = relay_chain_client
+			.runtime_api()
+			.candidate_pending_availability(block_id, para_id)
+			.map_err(|e| Box::new(BlockAnnounceError(format!("{:?}", e))) as Box<_>)?;
+
+		Ok(candidate_receipt.map(|cr| cr.descriptor.para_head))
+	}
+
 	/// Handle a block announcement with empty data (no statement) attached to it.
 	fn handle_empty_block_announce_data(
 		&self,
@@ -278,32 +317,30 @@ where
 			let runtime_api_block_id = BlockId::Hash(relay_chain_info.best_hash);
 			let block_number = header.number();

-			let local_validation_data = relay_chain_client
-				.runtime_api()
-				.persisted_validation_data(
-					&runtime_api_block_id,
-					para_id,
-					OccupiedCoreAssumption::TimedOut,
-				)
-				.map_err(|e| Box::new(BlockAnnounceError(format!("{:?}", e))) as Box<_>)?
-				.ok_or_else(|| {
-					Box::new(BlockAnnounceError(
-						"Could not find parachain head in relay chain".into(),
-					)) as Box<_>
-				})?;
-			let parent_head = Block::Header::decode(&mut &local_validation_data.parent_head.0[..])
-				.map_err(|e| {
-					Box::new(BlockAnnounceError(format!(
-						"Failed to decode parachain head: {:?}",
-						e
-					))) as Box<_>
-				})?;
-			let known_best_number = parent_head.number();
+			let best_head =
+				Self::included_block(&*relay_chain_client, &runtime_api_block_id, para_id)?;
+			let known_best_number = best_head.number();
+			let backed_block = ||
+				Self::backed_block_hash(&*relay_chain_client, &runtime_api_block_id, para_id);

-			if block_number >= known_best_number {
-				tracing::trace!(
-					target: "cumulus-network",
-					"validation failed because a justification is needed if the block at the top of the chain."
+			if best_head == header {
+				tracing::debug!(
+					target: LOG_TARGET,
+					"Announced block matches best block.",
+				);
+
+				Ok(Validation::Success { is_new_best: true })
+			} else if Some(HeadData(header.encode()).hash()) == backed_block()? {
+				tracing::debug!(
+					target: LOG_TARGET,
+					"Announced block matches latest backed block.",
+				);
+
+				Ok(Validation::Success { is_new_best: true })
+			} else if block_number >= known_best_number {
+				tracing::debug!(
+					target: LOG_TARGET,
+					"Validation failed because a justification is needed if the block at the top of the chain."
 				);

 				Ok(Validation::Failure { disconnect: false })
@@ -501,7 +538,7 @@ impl<Block: BlockT> WaitToAnnounce<Block> {
 		self.spawner.spawn(
 			"cumulus-wait-to-announce",
 			async move {
-				tracing::trace!(
+				tracing::debug!(
 					target: "cumulus-network",
 					"waiting for announce block in a background task...",
 				);
@@ -509,7 +546,7 @@ impl<Block: BlockT> WaitToAnnounce<Block> {
 				wait_to_announce::<Block>(block_hash, pov_hash, announce_block, signed_stmt_recv)
 					.await;

-				tracing::trace!(
+				tracing::debug!(
 					target: "cumulus-network",
 					"block announcement finished",
 				);
@@ -15,7 +15,7 @@
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.

 use super::*;
-use cumulus_test_service::runtime::{Block, Header};
+use cumulus_test_service::runtime::{Block, Header, Hash};
 use futures::{executor::block_on, poll, task::Poll};
 use polkadot_node_primitives::{SignedFullStatement, Statement};
 use polkadot_primitives::v1::{
@@ -37,6 +37,7 @@ use sp_keyring::Sr25519Keyring;
 use sp_keystore::{testing::KeyStore, SyncCryptoStore, SyncCryptoStorePtr};
 use sp_runtime::RuntimeAppPublic;
 use std::collections::BTreeMap;
+use parking_lot::Mutex;

 fn check_error(error: crate::BoxedError, check_error: impl Fn(&BlockAnnounceError) -> bool) {
 	let error = *error
@@ -173,6 +174,7 @@ fn invalid_if_no_data_exceeds_best_known_number() {
 	let mut validator = make_validator_and_api().0;
 	let header = Header {
 		number: 1,
+		state_root: Hash::random(),
 		..default_header()
 	};
 	let res = block_on(validator.validate(&header, &[]));
@@ -184,6 +186,18 @@ fn invalid_if_no_data_exceeds_best_known_number() {
 	);
 }

+#[test]
+fn valid_if_no_data_and_block_matches_best_known_block() {
+	let mut validator = make_validator_and_api().0;
+	let res = block_on(validator.validate(&default_header(), &[]));
+
+	assert_eq!(
+		res.unwrap(),
+		Validation::Success { is_new_best: true },
+		"validation is successful when the block hash matches the best known block",
+	);
+}
+
 #[test]
 fn check_statement_is_encoded_correctly() {
 	let mut validator = make_validator_and_api().0;
@@ -331,13 +345,33 @@ fn relay_parent_not_imported_when_block_announce_is_processed() {
 	});
 }

+/// Ensures that when we receive a block announcement without a statement included, while the block
+/// is not yet included by the node checking the announcement, but the node is already backed.
+#[test]
+fn block_announced_without_statement_and_block_only_backed() {
+	block_on(async move {
+		let (mut validator, api) = make_validator_and_api();
+		api.data.lock().has_pending_availability = true;
+
+		let header = default_header();
+
+		let validation = validator.validate(&header, &[]);
+
+		assert!(matches!(
+			validation.await,
+			Ok(Validation::Success { is_new_best: true })
+		));
+	});
+}
+
 #[derive(Default)]
 struct ApiData {
 	validators: Vec<ValidatorId>,
+	has_pending_availability: bool,
 }

 struct TestApi {
-	data: Arc<ApiData>,
+	data: Arc<Mutex<ApiData>>,
 	relay_client: Arc<PClient>,
 	relay_backend: Arc<PBackend>,
 }
@@ -348,9 +382,10 @@ impl TestApi {
 		let relay_backend = builder.backend();

 		Self {
-			data: Arc::new(ApiData {
+			data: Arc::new(Mutex::new(ApiData {
 				validators: vec![Sr25519Keyring::Alice.public().into()],
-			}),
+				has_pending_availability: false,
+			})),
 			relay_client: Arc::new(builder.build()),
 			relay_backend,
 		}
@@ -359,7 +394,7 @@ impl TestApi {

 #[derive(Default)]
 struct RuntimeApi {
-	data: Arc<ApiData>,
+	data: Arc<Mutex<ApiData>>,
 }

 impl ProvideRuntimeApi<PBlock> for TestApi {
@@ -376,7 +411,7 @@ impl ProvideRuntimeApi<PBlock> for TestApi {
 sp_api::mock_impl_runtime_apis! {
 	impl ParachainHost<PBlock> for RuntimeApi {
 		fn validators(&self) -> Vec<ValidatorId> {
-			self.data.validators.clone()
+			self.data.lock().validators.clone()
 		}

 		fn validator_groups(&self) -> (Vec<Vec<ValidatorIndex>>, GroupRotationInfo<BlockNumber>) {
@@ -407,7 +442,19 @@ sp_api::mock_impl_runtime_apis! {
 		}

 		fn candidate_pending_availability(&self, _: ParaId) -> Option<CommittedCandidateReceipt<PHash>> {
-			None
+			if self.data.lock().has_pending_availability {
+				Some(CommittedCandidateReceipt {
+					descriptor: CandidateDescriptor {
+						para_head: polkadot_parachain::primitives::HeadData(
+							default_header().encode(),
+						).hash(),
+						..Default::default()
+					},
+					..Default::default()
+				})
+			} else {
+				None
+			}
 		}

 		fn candidate_events(&self) -> Vec<CandidateEvent<PHash>> {