From 172830a0c845b8d29e1eadd1cdf521402fefee7a Mon Sep 17 00:00:00 2001
From: Svyatoslav Nikolsky <svyatonik@gmail.com>
Date: Fri, 7 Apr 2023 09:55:48 +0300
Subject: [PATCH] only refund if all bundled messages have been delivered
 (#2019)

---
 bin/runtime-common/src/messages_call_ext.rs   | 17 ++++--
 .../src/refund_relayer_extension.rs           | 59 ++++++++++++++++++-
 2 files changed, 68 insertions(+), 8 deletions(-)

diff --git a/bin/runtime-common/src/messages_call_ext.rs b/bin/runtime-common/src/messages_call_ext.rs
index 588afad106da..e8f992c63891 100644
--- a/bin/runtime-common/src/messages_call_ext.rs
+++ b/bin/runtime-common/src/messages_call_ext.rs
@@ -25,8 +25,11 @@ use sp_runtime::transaction_validity::TransactionValidity;
 /// Generic info about a messages delivery/confirmation proof.
 #[derive(PartialEq, RuntimeDebug)]
 pub struct BaseMessagesProofInfo {
+	/// Message lane, used by the call.
 	pub lane_id: LaneId,
+	/// Nonce of the best message, included in the call.
 	pub best_bundled_nonce: MessageNonce,
+	/// Nonce of the best message, stored by this chain before the call is dispatched.
 	pub best_stored_nonce: MessageNonce,
 }
 
@@ -58,19 +61,23 @@ pub struct CallHelper<T: Config<I>, I: 'static> {
 }
 
 impl<T: Config<I>, I: 'static> CallHelper<T, I> {
-	/// Check if a call delivered proof/confirmation for at least some of the messages that it
-	/// contained.
-	pub fn was_partially_successful(info: &CallInfo) -> bool {
+	/// Returns true if:
+	///
+	/// - call is `receive_messages_proof` and all messages have been delivered;
+	///
+	/// - call is `receive_messages_delivery_proof` and all messages confirmations have been
+	///   received.
+	pub fn was_successful(info: &CallInfo) -> bool {
 		match info {
 			CallInfo::ReceiveMessagesProof(info) => {
 				let inbound_lane_data =
 					pallet_bridge_messages::InboundLanes::<T, I>::get(info.0.lane_id);
-				inbound_lane_data.last_delivered_nonce() > info.0.best_stored_nonce
+				inbound_lane_data.last_delivered_nonce() == info.0.best_bundled_nonce
 			},
 			CallInfo::ReceiveMessagesDeliveryProof(info) => {
 				let outbound_lane_data =
 					pallet_bridge_messages::OutboundLanes::<T, I>::get(info.0.lane_id);
-				outbound_lane_data.latest_received_nonce > info.0.best_stored_nonce
+				outbound_lane_data.latest_received_nonce == info.0.best_bundled_nonce
 			},
 		}
 	}
diff --git a/bin/runtime-common/src/refund_relayer_extension.rs b/bin/runtime-common/src/refund_relayer_extension.rs
index 7be97f19ad2a..9b1e9a3a1a4d 100644
--- a/bin/runtime-common/src/refund_relayer_extension.rs
+++ b/bin/runtime-common/src/refund_relayer_extension.rs
@@ -344,6 +344,16 @@ where
 				finality_proof_info.block_number,
 			) {
 				// we only refund relayer if all calls have updated chain state
+				log::trace!(
+					target: "runtime::bridge",
+					"{} from parachain {} via {:?}: failed to refund relayer {:?}, because \
+					relay chain finality proof has not been accepted",
+					Self::IDENTIFIER,
+					Para::Id::get(),
+					Msgs::Id::get(),
+					relayer,
+				);
+
 				return Ok(())
 			}
 
@@ -366,15 +376,34 @@ where
 				para_proof_info,
 			) {
 				// we only refund relayer if all calls have updated chain state
+				log::trace!(
+					target: "runtime::bridge",
+					"{} from parachain {} via {:?}: failed to refund relayer {:?}, because \
+					parachain finality proof has not been accepted",
+					Self::IDENTIFIER,
+					Para::Id::get(),
+					Msgs::Id::get(),
+					relayer,
+				);
+
 				return Ok(())
 			}
 		}
 
-		// Check if the `ReceiveMessagesProof` call delivered at least some of the messages that
+		// Check if the `ReceiveMessagesProof` call delivered all the messages that
 		// it contained. If this happens, we consider the transaction "helpful" and refund it.
 		let msgs_call_info = call_info.messages_call_info();
-		if !MessagesCallHelper::<Runtime, Msgs::Instance>::was_partially_successful(msgs_call_info)
-		{
+		if !MessagesCallHelper::<Runtime, Msgs::Instance>::was_successful(msgs_call_info) {
+			log::trace!(
+				target: "runtime::bridge",
+				"{} from parachain {} via {:?}: failed to refund relayer {:?}, because \
+				some of messages have not been accepted",
+				Self::IDENTIFIER,
+				Para::Id::get(),
+				Msgs::Id::get(),
+				relayer,
+			);
+
 			return Ok(())
 		}
 
@@ -1032,6 +1061,30 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn post_dispatch_ignores_transaction_that_has_not_delivered_all_messages() {
+		run_test(|| {
+			initialize_environment(200, 200, Default::default(), 150);
+
+			assert_storage_noop!(run_post_dispatch(Some(all_finality_pre_dispatch_data()), Ok(())));
+			assert_storage_noop!(run_post_dispatch(
+				Some(parachain_finality_pre_dispatch_data()),
+				Ok(())
+			));
+			assert_storage_noop!(run_post_dispatch(Some(delivery_pre_dispatch_data()), Ok(())));
+
+			assert_storage_noop!(run_post_dispatch(
+				Some(all_finality_confirmation_pre_dispatch_data()),
+				Ok(())
+			));
+			assert_storage_noop!(run_post_dispatch(
+				Some(parachain_finality_confirmation_pre_dispatch_data()),
+				Ok(())
+			));
+			assert_storage_noop!(run_post_dispatch(Some(confirmation_pre_dispatch_data()), Ok(())));
+		});
+	}
+
 	#[test]
 	fn post_dispatch_refunds_relayer_in_all_finality_batch_with_extra_weight() {
 		run_test(|| {