Emit dispatch message when whole message (payload) has invalid encodi…

…ng (paritytech#664) * emit dispatch message when whole message (payload) has invalid encoding * DispatchMessage::dispatch accepts Result
serban300 · Apr 8, 2024 · 12cc234 · 12cc234
1 parent 1d366c8
commit 12cc234
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 17 deletions.
diff --git a/bridges/bin/runtime-common/src/messages.rs b/bridges/bin/runtime-common/src/messages.rs
@@ -381,7 +381,9 @@ pub mod target {
 		for FromBridgedChainMessageDispatch<B, ThisRuntime, ThisCallDispatchInstance>
 	where
 		ThisCallDispatchInstance: frame_support::traits::Instance,
-		ThisRuntime: pallet_bridge_call_dispatch::Config<ThisCallDispatchInstance>,
+		ThisRuntime: pallet_bridge_call_dispatch::Config<ThisCallDispatchInstance, MessageId = (LaneId, MessageNonce)>,
+		<ThisRuntime as pallet_bridge_call_dispatch::Config<ThisCallDispatchInstance>>::Event:
+			From<pallet_bridge_call_dispatch::RawEvent<(LaneId, MessageNonce), ThisCallDispatchInstance>>,
 		pallet_bridge_call_dispatch::Module<ThisRuntime, ThisCallDispatchInstance>:
 			bp_message_dispatch::MessageDispatch<
 				(LaneId, MessageNonce),
@@ -402,13 +404,12 @@ pub mod target {
 		}
 
 		fn dispatch(message: DispatchMessage<Self::DispatchPayload, BalanceOf<BridgedChain<B>>>) {
-			if let Ok(payload) = message.data.payload {
-				pallet_bridge_call_dispatch::Module::<ThisRuntime, ThisCallDispatchInstance>::dispatch(
-					B::INSTANCE,
-					(message.key.lane_id, message.key.nonce),
-					payload.0,
-				);
-			}
+			let message_id = (message.key.lane_id, message.key.nonce);
+			pallet_bridge_call_dispatch::Module::<ThisRuntime, ThisCallDispatchInstance>::dispatch(
+				B::INSTANCE,
+				message_id,
+				message.data.payload.map_err(drop).map(|payload| payload.0),
+			);
 		}
 	}
 

diff --git a/bridges/modules/call-dispatch/src/lib.rs b/bridges/modules/call-dispatch/src/lib.rs
@@ -148,6 +148,8 @@ decl_event!(
 	pub enum Event<T, I = DefaultInstance> where
 		<T as Config<I>>::MessageId
 	{
+		/// Message has been rejected before reaching dispatch.
+		MessageRejected(InstanceId, MessageId),
 		/// Message has been rejected by dispatcher because of spec version mismatch.
 		/// Last two arguments are: expected and passed spec version.
 		MessageVersionSpecMismatch(InstanceId, MessageId, SpecVersion, SpecVersion),
@@ -183,7 +185,17 @@ impl<T: Config<I>, I: Instance> MessageDispatch<T::MessageId> for Module<T, I> {
 		message.weight
 	}
 
-	fn dispatch(bridge: InstanceId, id: T::MessageId, message: Self::Message) {
+	fn dispatch(bridge: InstanceId, id: T::MessageId, message: Result<Self::Message, ()>) {
+		// emit special even if message has been rejected by external component
+		let message = match message {
+			Ok(message) => message,
+			Err(_) => {
+				frame_support::debug::trace!("Message {:?}/{:?}: rejected before actual dispatch", bridge, id);
+				Self::deposit_event(RawEvent::MessageRejected(bridge, id));
+				return;
+			}
+		};
+
 		// verify spec version
 		// (we want it to be the same, because otherwise we may decode Call improperly)
 		let expected_version = <T as frame_system::Config>::Version::get().spec_version;
@@ -491,7 +503,7 @@ mod tests {
 			message.spec_version = BAD_SPEC_VERSION;
 
 			System::set_block_number(1);
-			CallDispatch::dispatch(bridge, id, message);
+			CallDispatch::dispatch(bridge, id, Ok(message));
 
 			assert_eq!(
 				System::events(),
@@ -519,7 +531,7 @@ mod tests {
 			message.weight = 0;
 
 			System::set_block_number(1);
-			CallDispatch::dispatch(bridge, id, message);
+			CallDispatch::dispatch(bridge, id, Ok(message));
 
 			assert_eq!(
 				System::events(),
@@ -547,7 +559,7 @@ mod tests {
 			);
 
 			System::set_block_number(1);
-			CallDispatch::dispatch(bridge, id, message);
+			CallDispatch::dispatch(bridge, id, Ok(message));
 
 			assert_eq!(
 				System::events(),
@@ -560,6 +572,26 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn should_emit_event_for_rejected_messages() {
+		new_test_ext().execute_with(|| {
+			let bridge = b"ethb".to_owned();
+			let id = [0; 4];
+
+			System::set_block_number(1);
+			CallDispatch::dispatch(bridge, id, Err(()));
+
+			assert_eq!(
+				System::events(),
+				vec![EventRecord {
+					phase: Phase::Initialization,
+					event: TestEvent::call_dispatch(Event::<TestRuntime>::MessageRejected(bridge, id)),
+					topics: vec![],
+				}],
+			);
+		});
+	}
+
 	#[test]
 	fn should_dispatch_bridge_message_from_root_origin() {
 		new_test_ext().execute_with(|| {
@@ -568,7 +600,7 @@ mod tests {
 			let message = prepare_root_message(Call::System(<frame_system::Call<TestRuntime>>::remark(vec![1, 2, 3])));
 
 			System::set_block_number(1);
-			CallDispatch::dispatch(bridge, id, message);
+			CallDispatch::dispatch(bridge, id, Ok(message));
 
 			assert_eq!(
 				System::events(),
@@ -591,7 +623,7 @@ mod tests {
 			let message = prepare_target_message(call);
 
 			System::set_block_number(1);
-			CallDispatch::dispatch(bridge, id, message);
+			CallDispatch::dispatch(bridge, id, Ok(message));
 
 			assert_eq!(
 				System::events(),
@@ -614,7 +646,7 @@ mod tests {
 			let message = prepare_source_message(call);
 
 			System::set_block_number(1);
-			CallDispatch::dispatch(bridge, id, message);
+			CallDispatch::dispatch(bridge, id, Ok(message));
 
 			assert_eq!(
 				System::events(),

diff --git a/bridges/primitives/message-dispatch/src/lib.rs b/bridges/primitives/message-dispatch/src/lib.rs
@@ -41,6 +41,9 @@ pub trait MessageDispatch<MessageId> {
 	///
 	/// `id` is a short unique identifier of the message.
 	///
-	/// Returns post-dispatch (actual) message weight.
-	fn dispatch(bridge: InstanceId, id: MessageId, message: Self::Message);
+	/// If message is `Ok`, then it should be dispatched. If it is `Err`, then it's just
+	/// a sign that some other component has rejected the message even before it has
+	/// reached `dispatch` method (right now this may only be caused if we fail to decode
+	/// the whole message).
+	fn dispatch(bridge: InstanceId, id: MessageId, message: Result<Self::Message, ()>);
 }