Add another condition to the reject-obsolete-parachain-heads extension (

paritytech#1505)

* add another condition to the reject-obsolete-parachain-heads extension

* add tracing to obsolete-tx-extensions

* fix tests

* extension_rejects_header_from_new_relay_block_with_same_hash

* fmt

* fix benchmarks

bridges/bin/millau/runtime/src/lib.rs

@@ -1035,6 +1035,7 @@ impl_runtime_apis! {
 					pallet_bridge_parachains::RelayBlockNumber,
 					pallet_bridge_parachains::RelayBlockHash,
 					bp_polkadot_core::parachains::ParaHeadsProof,
+					Vec<(bp_polkadot_core::parachains::ParaId, bp_polkadot_core::parachains::ParaHash)>,
 				) {
 					bridge_runtime_common::parachains_benchmarking::prepare_parachain_heads_proof::<Runtime, WithRialtoParachainsInstance>(
 						parachains,

bridges/bin/runtime-common/src/messages_extension.rs

@@ -70,6 +70,14 @@ macro_rules! declare_bridge_reject_obsolete_messages {
 
 							let inbound_lane_data = pallet_bridge_messages::InboundLanes::<$runtime, $instance>::get(&proof.lane);
 							if proof.nonces_end <= inbound_lane_data.last_delivered_nonce() {
+								log::trace!(
+									target: pallet_bridge_messages::LOG_TARGET,
+									"Rejecting obsolete messages delivery transaction: lane {:?}, bundled {:?}, best {:?}",
+									proof.lane,
+									proof.nonces_end,
+									inbound_lane_data.last_delivered_nonce(),
+								);
+
 								return sp_runtime::transaction_validity::InvalidTransaction::Stale.into();
 							}
 
@@ -84,6 +92,14 @@ macro_rules! declare_bridge_reject_obsolete_messages {
 
 							let outbound_lane_data = pallet_bridge_messages::OutboundLanes::<$runtime, $instance>::get(&proof.lane);
 							if latest_delivered_nonce <= outbound_lane_data.latest_received_nonce {
+								log::trace!(
+									target: pallet_bridge_messages::LOG_TARGET,
+									"Rejecting obsolete messages confirmation transaction: lane {:?}, bundled {:?}, best {:?}",
+									proof.lane,
+									latest_delivered_nonce,
+									outbound_lane_data.latest_received_nonce,
+								);
+
 								return sp_runtime::transaction_validity::InvalidTransaction::Stale.into();
 							}
 

bridges/bin/runtime-common/src/parachains_benchmarking.rs

@@ -21,7 +21,7 @@
 use crate::messages_benchmarking::{grow_trie, insert_header_to_grandpa_pallet};
 
 use bp_parachains::parachain_head_storage_key_at_source;
-use bp_polkadot_core::parachains::{ParaHead, ParaHeadsProof, ParaId};
+use bp_polkadot_core::parachains::{ParaHash, ParaHead, ParaHeadsProof, ParaId};
 use bp_runtime::StorageProofSize;
 use codec::Encode;
 use frame_support::traits::Get;
@@ -37,7 +37,7 @@ pub fn prepare_parachain_heads_proof<R, PI>(
 	parachains: &[ParaId],
 	parachain_head_size: u32,
 	size: StorageProofSize,
-) -> (RelayBlockNumber, RelayBlockHash, ParaHeadsProof)
+) -> (RelayBlockNumber, RelayBlockHash, ParaHeadsProof, Vec<(ParaId, ParaHash)>)
 where
 	R: pallet_bridge_parachains::Config<PI>
 		+ pallet_bridge_grandpa::Config<R::BridgesGrandpaPalletInstance>,
@@ -48,6 +48,7 @@ where
 	let parachain_head = ParaHead(vec![0u8; parachain_head_size as usize]);
 
 	// insert all heads to the trie
+	let mut parachain_heads = Vec::with_capacity(parachains.len());
 	let mut storage_keys = Vec::with_capacity(parachains.len());
 	let mut state_root = Default::default();
 	let mut mdb = MemoryDB::default();
@@ -62,6 +63,7 @@ where
 				.map_err(|_| "TrieMut::insert has failed")
 				.expect("TrieMut::insert should not fail in benchmarks");
 			storage_keys.push(storage_key);
+			parachain_heads.push((*parachain, parachain_head.hash()))
 		}
 	}
 	state_root = grow_trie(state_root, &mut mdb, size);
@@ -76,5 +78,5 @@ where
 	let (relay_block_number, relay_block_hash) =
 		insert_header_to_grandpa_pallet::<R, R::BridgesGrandpaPalletInstance>(state_root);
 
-	(relay_block_number, relay_block_hash, ParaHeadsProof(proof))
+	(relay_block_number, relay_block_hash, ParaHeadsProof(proof), parachain_heads)
 }

bridges/modules/grandpa/src/extension.rs

@@ -75,6 +75,13 @@ macro_rules! declare_bridge_reject_obsolete_grandpa_header {
 							if best_finalized_number < bundled_block_number {
 								Ok(sp_runtime::transaction_validity::ValidTransaction::default())
 							} else {
+								log::trace!(
+									target: $crate::LOG_TARGET,
+									"Rejecting obsolete bridged header: bundled {:?}, best {:?}",
+									bundled_block_number,
+									best_finalized_number,
+								);
+
 								sp_runtime::transaction_validity::InvalidTransaction::Stale.into()
 							}
 						},

bridges/modules/grandpa/src/lib.rs

@@ -60,7 +60,7 @@ pub use pallet::*;
 pub use weights::WeightInfo;
 
 /// The target that will be used when publishing logs related to this pallet.
-const LOG_TARGET: &str = "runtime::bridge-grandpa";
+pub const LOG_TARGET: &str = "runtime::bridge-grandpa";
 
 /// Block number of the bridged chain.
 pub type BridgedBlockNumber<T, I> = BlockNumberOf<<T as Config<I>>::BridgedChain>;

bridges/modules/messages/src/lib.rs

@@ -91,7 +91,7 @@ mod mock;
 pub use pallet::*;
 
 /// The target that will be used when publishing logs related to this pallet.
-const LOG_TARGET: &str = "runtime::bridge-messages";
+pub const LOG_TARGET: &str = "runtime::bridge-messages";
 
 #[frame_support::pallet]
 pub mod pallet {

bridges/modules/parachains/src/benchmarking.rs

@@ -21,7 +21,7 @@ use crate::{
 	RelayBlockNumber,
 };
 
-use bp_polkadot_core::parachains::{ParaHeadsProof, ParaId};
+use bp_polkadot_core::parachains::{ParaHash, ParaHeadsProof, ParaId};
 use bp_runtime::StorageProofSize;
 use frame_benchmarking::{account, benchmarks_instance_pallet};
 use frame_system::RawOrigin;
@@ -37,7 +37,7 @@ pub trait Config<I: 'static>: crate::Config<I> {
 		parachains: &[ParaId],
 		parachain_head_size: u32,
 		proof_size: StorageProofSize,
-	) -> (RelayBlockNumber, RelayBlockHash, ParaHeadsProof);
+	) -> (RelayBlockNumber, RelayBlockHash, ParaHeadsProof, Vec<(ParaId, ParaHash)>);
 }
 
 benchmarks_instance_pallet! {
@@ -57,13 +57,13 @@ benchmarks_instance_pallet! {
 
 		let sender = account("sender", 0, 0);
 		let parachains = (1..=p).map(ParaId).collect::<Vec<_>>();
-		let (relay_block_number, relay_block_hash, parachain_heads_proof) = T::prepare_parachain_heads_proof(
+		let (relay_block_number, relay_block_hash, parachain_heads_proof, parachains_heads) = T::prepare_parachain_heads_proof(
 			&parachains,
 			DEFAULT_PARACHAIN_HEAD_SIZE,
 			StorageProofSize::Minimal(0),
 		);
 		let at_relay_block = (relay_block_number, relay_block_hash);
-	}: submit_parachain_heads(RawOrigin::Signed(sender), at_relay_block, parachains.clone(), parachain_heads_proof)
+	}: submit_parachain_heads(RawOrigin::Signed(sender), at_relay_block, parachains_heads, parachain_heads_proof)
 	verify {
 		for parachain in parachains {
 			assert!(crate::Pallet::<T, I>::best_parachain_head(parachain).is_some());
@@ -74,13 +74,13 @@ benchmarks_instance_pallet! {
 	submit_parachain_heads_with_1kb_proof {
 		let sender = account("sender", 0, 0);
 		let parachains = vec![ParaId(1)];
-		let (relay_block_number, relay_block_hash, parachain_heads_proof) = T::prepare_parachain_heads_proof(
+		let (relay_block_number, relay_block_hash, parachain_heads_proof, parachains_heads) = T::prepare_parachain_heads_proof(
 			&parachains,
 			DEFAULT_PARACHAIN_HEAD_SIZE,
 			StorageProofSize::HasExtraNodes(1024),
 		);
 		let at_relay_block = (relay_block_number, relay_block_hash);
-	}: submit_parachain_heads(RawOrigin::Signed(sender), at_relay_block, parachains.clone(), parachain_heads_proof)
+	}: submit_parachain_heads(RawOrigin::Signed(sender), at_relay_block, parachains_heads, parachain_heads_proof)
 	verify {
 		for parachain in parachains {
 			assert!(crate::Pallet::<T, I>::best_parachain_head(parachain).is_some());
@@ -91,13 +91,13 @@ benchmarks_instance_pallet! {
 	submit_parachain_heads_with_16kb_proof {
 		let sender = account("sender", 0, 0);
 		let parachains = vec![ParaId(1)];
-		let (relay_block_number, relay_block_hash, parachain_heads_proof) = T::prepare_parachain_heads_proof(
+		let (relay_block_number, relay_block_hash, parachain_heads_proof, parachains_heads) = T::prepare_parachain_heads_proof(
 			&parachains,
 			DEFAULT_PARACHAIN_HEAD_SIZE,
 			StorageProofSize::HasExtraNodes(16 * 1024),
 		);
 		let at_relay_block = (relay_block_number, relay_block_hash);
-	}: submit_parachain_heads(RawOrigin::Signed(sender), at_relay_block, parachains.clone(), parachain_heads_proof)
+	}: submit_parachain_heads(RawOrigin::Signed(sender), at_relay_block, parachains_heads, parachain_heads_proof)
 	verify {
 		for parachain in parachains {
 			assert!(crate::Pallet::<T, I>::best_parachain_head(parachain).is_some());

bridges/modules/parachains/src/extension.rs

@@ -72,15 +72,34 @@ macro_rules! declare_bridge_reject_obsolete_parachain_header {
 							ref parachains,
 							..
 						}) if parachains.len() == 1 => {
-							let parachain = parachains.get(0).expect("verified by match condition; qed");
+							let (parachain, parachain_head_hash) = parachains.get(0).expect("verified by match condition; qed");
 
 							let bundled_relay_block_number = at_relay_block.0;
 
 							let best_parachain_head = $crate::BestParaHeads::<$runtime, $instance>::get(parachain);
+
 							match best_parachain_head {
 								Some(best_parachain_head) if best_parachain_head.at_relay_block_number
-									>= bundled_relay_block_number =>
-										sp_runtime::transaction_validity::InvalidTransaction::Stale.into(),
+									>= bundled_relay_block_number => {
+									log::trace!(
+										target: $crate::LOG_TARGET,
+										"Rejecting obsolete parachain-head {:?} transaction: bundled relay block number: \
+										{:?} best relay block number: {:?}",
+										parachain,
+										bundled_relay_block_number,
+										best_parachain_head.at_relay_block_number,
+									);
+									sp_runtime::transaction_validity::InvalidTransaction::Stale.into()
+								}
+								Some(best_parachain_head) if best_parachain_head.head_hash == *parachain_head_hash => {
+									log::trace!(
+										target: $crate::LOG_TARGET,
+										"Rejecting obsolete parachain-head {:?} transaction: head hash {:?}",
+										parachain,
+										best_parachain_head.head_hash,
+									);
+									sp_runtime::transaction_validity::InvalidTransaction::Stale.into()
+								}
 								_ => Ok(sp_runtime::transaction_validity::ValidTransaction::default()),
 							}
 						},
@@ -118,7 +137,7 @@ mod tests {
 		mock::{run_test, Call, TestRuntime},
 		BestParaHead, BestParaHeads, RelayBlockNumber,
 	};
-	use bp_polkadot_core::parachains::{ParaHeadsProof, ParaId};
+	use bp_polkadot_core::parachains::{ParaHash, ParaHeadsProof, ParaId};
 	use frame_support::weights::{DispatchClass, DispatchInfo, Pays};
 	use sp_runtime::traits::SignedExtension;
 
@@ -127,7 +146,10 @@ mod tests {
 		Call::Parachains => ()
 	}
 
-	fn validate_submit_parachain_heads(num: RelayBlockNumber, parachains: Vec<ParaId>) -> bool {
+	fn validate_submit_parachain_heads(
+		num: RelayBlockNumber,
+		parachains: Vec<(ParaId, ParaHash)>,
+	) -> bool {
 		BridgeRejectObsoleteParachainHeader
 			.validate(
 				&42,
@@ -147,29 +169,39 @@ mod tests {
 			ParaId(1),
 			BestParaHead {
 				at_relay_block_number: 10,
-				head_hash: Default::default(),
+				head_hash: [1u8; 32].into(),
 				next_imported_hash_position: 0,
 			},
 		);
 	}
 
 	#[test]
-	fn extension_rejects_obsolete_header() {
+	fn extension_rejects_header_from_the_obsolete_relay_block() {
 		run_test(|| {
 			// when current best finalized is #10 and we're trying to import header#5 => tx is
 			// rejected
 			sync_to_relay_header_10();
-			assert!(!validate_submit_parachain_heads(5, vec![ParaId(1)]));
+			assert!(!validate_submit_parachain_heads(5, vec![(ParaId(1), [1u8; 32].into())]));
+		});
+	}
+
+	#[test]
+	fn extension_rejects_header_from_the_same_relay_block() {
+		run_test(|| {
+			// when current best finalized is #10 and we're trying to import header#10 => tx is
+			// rejected
+			sync_to_relay_header_10();
+			assert!(!validate_submit_parachain_heads(10, vec![(ParaId(1), [1u8; 32].into())]));
 		});
 	}
 
 	#[test]
-	fn extension_rejects_same_header() {
+	fn extension_rejects_header_from_new_relay_block_with_same_hash() {
 		run_test(|| {
 			// when current best finalized is #10 and we're trying to import header#10 => tx is
 			// rejected
 			sync_to_relay_header_10();
-			assert!(!validate_submit_parachain_heads(10, vec![ParaId(1)]));
+			assert!(!validate_submit_parachain_heads(20, vec![(ParaId(1), [1u8; 32].into())]));
 		});
 	}
 
@@ -179,7 +211,7 @@ mod tests {
 			// when current best finalized is #10 and we're trying to import header#15 => tx is
 			// accepted
 			sync_to_relay_header_10();
-			assert!(validate_submit_parachain_heads(15, vec![ParaId(1)]));
+			assert!(validate_submit_parachain_heads(15, vec![(ParaId(1), [2u8; 32].into())]));
 		});
 	}
 
@@ -189,7 +221,10 @@ mod tests {
 			// when current best finalized is #10 and we're trying to import header#5, but another
 			// parachain head is also supplied => tx is accepted
 			sync_to_relay_header_10();
-			assert!(validate_submit_parachain_heads(5, vec![ParaId(1), ParaId(2)]));
+			assert!(validate_submit_parachain_heads(
+				5,
+				vec![(ParaId(1), [1u8; 32].into()), (ParaId(2), [1u8; 32].into())]
+			));
 		});
 	}
 }