Propagate message receival confirmation errors (paritytech#2116)

serban300 · web-flow · commit 183778f06c46 · 2023-05-09T11:43:15.000+03:00
* Propagate message receival confirmation errors

* spellcheck
diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs
@@ -48,7 +48,7 @@ pub use weights_ext::{
 
 use crate::{
 	inbound_lane::{InboundLane, InboundLaneStorage},
-	outbound_lane::{OutboundLane, OutboundLaneStorage, ReceivalConfirmationResult},
+	outbound_lane::{OutboundLane, OutboundLaneStorage, ReceivalConfirmationError},
 };
 
 use bp_messages::{
@@ -458,36 +458,13 @@ pub mod pallet {
 			// mark messages as delivered
 			let mut lane = outbound_lane::<T, I>(lane_id);
 			let last_delivered_nonce = lane_data.last_delivered_nonce();
-			let confirmed_messages = match lane.confirm_delivery(
-				relayers_state.total_messages,
-				last_delivered_nonce,
-				&lane_data.relayers,
-			) {
-				ReceivalConfirmationResult::ConfirmedMessages(confirmed_messages) =>
-					Some(confirmed_messages),
-				ReceivalConfirmationResult::NoNewConfirmations => None,
-				ReceivalConfirmationResult::TryingToConfirmMoreMessagesThanExpected(
-					to_confirm_messages_count,
-				) => {
-					log::trace!(
-						target: LOG_TARGET,
-						"Messages delivery proof contains too many messages to confirm: {} vs declared {}",
-						to_confirm_messages_count,
-						relayers_state.total_messages,
-					);
-
-					fail!(Error::<T, I>::TryingToConfirmMoreMessagesThanExpected);
-				},
-				error => {
-					log::trace!(
-						target: LOG_TARGET,
-						"Messages delivery proof contains invalid unrewarded relayers vec: {:?}",
-						error,
-					);
-
-					fail!(Error::<T, I>::InvalidUnrewardedRelayers);
-				},
-			};
+			let confirmed_messages = lane
+				.confirm_delivery(
+					relayers_state.total_messages,
+					last_delivered_nonce,
+					&lane_data.relayers,
+				)
+				.map_err(Error::<T, I>::ReceivalConfirmation)?;
 
 			if let Some(confirmed_messages) = confirmed_messages {
 				// emit 'delivered' event
@@ -568,8 +545,6 @@ pub mod pallet {
 		InvalidMessagesProof,
 		/// Invalid messages delivery proof has been submitted.
 		InvalidMessagesDeliveryProof,
-		/// The bridged chain has invalid `UnrewardedRelayers` in its storage (fatal for the lane).
-		InvalidUnrewardedRelayers,
 		/// The relayer has declared invalid unrewarded relayers state in the
 		/// `receive_messages_delivery_proof` call.
 		InvalidUnrewardedRelayersState,
@@ -578,9 +553,8 @@ pub mod pallet {
 		InsufficientDispatchWeight,
 		/// The message someone is trying to work with (i.e. increase fee) is not yet sent.
 		MessageIsNotYetSent,
-		/// The number of actually confirmed messages is going to be larger than the number of
-		/// messages in the proof. This may mean that this or bridged chain storage is corrupted.
-		TryingToConfirmMoreMessagesThanExpected,
+		/// Error confirming messages receival.
+		ReceivalConfirmation(ReceivalConfirmationError),
 		/// Error generated by the `OwnedBridgeModule` trait.
 		BridgeModule(bp_runtime::OwnedBridgeModuleError),
 	}
@@ -941,13 +915,16 @@ fn verify_and_decode_messages_proof<Chain: SourceHeaderChain, DispatchPayload: D
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use crate::mock::{
-		inbound_unrewarded_relayers_state, message, message_payload, run_test, unrewarded_relayer,
-		AccountId, DbWeight, RuntimeEvent as TestEvent, RuntimeOrigin,
-		TestDeliveryConfirmationPayments, TestDeliveryPayments, TestMessagesDeliveryProof,
-		TestMessagesProof, TestRelayer, TestRuntime, TestWeightInfo, MAX_OUTBOUND_PAYLOAD_SIZE,
-		PAYLOAD_REJECTED_BY_TARGET_CHAIN, REGULAR_PAYLOAD, TEST_LANE_ID, TEST_LANE_ID_2,
-		TEST_LANE_ID_3, TEST_RELAYER_A, TEST_RELAYER_B,
+	use crate::{
+		mock::{
+			inbound_unrewarded_relayers_state, message, message_payload, run_test,
+			unrewarded_relayer, AccountId, DbWeight, RuntimeEvent as TestEvent, RuntimeOrigin,
+			TestDeliveryConfirmationPayments, TestDeliveryPayments, TestMessagesDeliveryProof,
+			TestMessagesProof, TestRelayer, TestRuntime, TestWeightInfo, MAX_OUTBOUND_PAYLOAD_SIZE,
+			PAYLOAD_REJECTED_BY_TARGET_CHAIN, REGULAR_PAYLOAD, TEST_LANE_ID, TEST_LANE_ID_2,
+			TEST_LANE_ID_3, TEST_RELAYER_A, TEST_RELAYER_B,
+		},
+		outbound_lane::ReceivalConfirmationError,
 	};
 	use bp_messages::{BridgeMessagesCall, UnrewardedRelayer, UnrewardedRelayersState};
 	use bp_test_utils::generate_owned_bridge_module_tests;
@@ -1818,7 +1795,9 @@ mod tests {
 					))),
 					UnrewardedRelayersState { last_delivered_nonce: 1, ..Default::default() },
 				),
-				Error::<TestRuntime, ()>::TryingToConfirmMoreMessagesThanExpected,
+				Error::<TestRuntime, ()>::ReceivalConfirmation(
+					ReceivalConfirmationError::TryingToConfirmMoreMessagesThanExpected
+				),
 			);
 		});
 	}
diff --git a/bridges/modules/messages/src/outbound_lane.rs b/bridges/modules/messages/src/outbound_lane.rs
@@ -16,16 +16,18 @@
 
 //! Everything about outgoing messages sending.
 
-use crate::Config;
+use crate::{Config, LOG_TARGET};
 
 use bp_messages::{
 	DeliveredMessages, LaneId, MessageNonce, MessagePayload, OutboundLaneData, UnrewardedRelayer,
 };
+use codec::{Decode, Encode};
 use frame_support::{
 	weights::{RuntimeDbWeight, Weight},
-	BoundedVec, RuntimeDebug,
+	BoundedVec, PalletError, RuntimeDebug,
 };
 use num_traits::Zero;
+use scale_info::TypeInfo;
 use sp_std::collections::vec_deque::VecDeque;
 
 /// Outbound lane storage.
@@ -49,13 +51,8 @@ pub trait OutboundLaneStorage {
 pub type StoredMessagePayload<T, I> = BoundedVec<u8, <T as Config<I>>::MaximalOutboundPayloadSize>;
 
 /// Result of messages receival confirmation.
-#[derive(RuntimeDebug, PartialEq, Eq)]
-pub enum ReceivalConfirmationResult {
-	/// New messages have been confirmed by the confirmation transaction.
-	ConfirmedMessages(DeliveredMessages),
-	/// Confirmation transaction brings no new confirmation. This may be a result of relayer
-	/// error or several relayers running.
-	NoNewConfirmations,
+#[derive(Encode, Decode, RuntimeDebug, PartialEq, Eq, PalletError, TypeInfo)]
+pub enum ReceivalConfirmationError {
 	/// Bridged chain is trying to confirm more messages than we have generated. May be a result
 	/// of invalid bridged chain storage.
 	FailedToConfirmFutureMessages,
@@ -66,7 +63,7 @@ pub enum ReceivalConfirmationResult {
 	/// bridged chain storage.
 	NonConsecutiveUnrewardedRelayerEntries,
 	/// The chain has more messages that need to be confirmed than there is in the proof.
-	TryingToConfirmMoreMessagesThanExpected(MessageNonce),
+	TryingToConfirmMoreMessagesThanExpected,
 }
 
 /// Outbound messages lane.
@@ -105,37 +102,39 @@ impl<S: OutboundLaneStorage> OutboundLane<S> {
 		max_allowed_messages: MessageNonce,
 		latest_delivered_nonce: MessageNonce,
 		relayers: &VecDeque<UnrewardedRelayer<RelayerId>>,
-	) -> ReceivalConfirmationResult {
+	) -> Result<Option<DeliveredMessages>, ReceivalConfirmationError> {
 		let mut data = self.storage.data();
-		if latest_delivered_nonce <= data.latest_received_nonce {
-			return ReceivalConfirmationResult::NoNewConfirmations
+		let confirmed_messages = DeliveredMessages {
+			begin: data.latest_received_nonce.saturating_add(1),
+			end: latest_delivered_nonce,
+		};
+		if confirmed_messages.total_messages() == 0 {
+			return Ok(None)
 		}
-		if latest_delivered_nonce > data.latest_generated_nonce {
-			return ReceivalConfirmationResult::FailedToConfirmFutureMessages
+		if confirmed_messages.end > data.latest_generated_nonce {
+			return Err(ReceivalConfirmationError::FailedToConfirmFutureMessages)
 		}
-		if latest_delivered_nonce - data.latest_received_nonce > max_allowed_messages {
+		if confirmed_messages.total_messages() > max_allowed_messages {
 			// that the relayer has declared correct number of messages that the proof contains (it
 			// is checked outside of the function). But it may happen (but only if this/bridged
 			// chain storage is corrupted, though) that the actual number of confirmed messages if
 			// larger than declared. This would mean that 'reward loop' will take more time than the
 			// weight formula accounts, so we can't allow that.
-			return ReceivalConfirmationResult::TryingToConfirmMoreMessagesThanExpected(
-				latest_delivered_nonce - data.latest_received_nonce,
-			)
+			log::trace!(
+				target: LOG_TARGET,
+				"Messages delivery proof contains too many messages to confirm: {} vs declared {}",
+				confirmed_messages.total_messages(),
+				max_allowed_messages,
+			);
+			return Err(ReceivalConfirmationError::TryingToConfirmMoreMessagesThanExpected)
 		}
 
-		if let Err(e) = ensure_unrewarded_relayers_are_correct(latest_delivered_nonce, relayers) {
-			return e
-		}
+		ensure_unrewarded_relayers_are_correct(confirmed_messages.end, relayers)?;
 
-		let prev_latest_received_nonce = data.latest_received_nonce;
-		data.latest_received_nonce = latest_delivered_nonce;
+		data.latest_received_nonce = confirmed_messages.end;
 		self.storage.set_data(data);
 
-		ReceivalConfirmationResult::ConfirmedMessages(DeliveredMessages {
-			begin: prev_latest_received_nonce + 1,
-			end: latest_delivered_nonce,
-		})
+		Ok(Some(confirmed_messages))
 	}
 
 	/// Prune at most `max_messages_to_prune` already received messages.
@@ -176,27 +175,24 @@ impl<S: OutboundLaneStorage> OutboundLane<S> {
 fn ensure_unrewarded_relayers_are_correct<RelayerId>(
 	latest_received_nonce: MessageNonce,
 	relayers: &VecDeque<UnrewardedRelayer<RelayerId>>,
-) -> Result<(), ReceivalConfirmationResult> {
-	let mut last_entry_end: Option<MessageNonce> = None;
+) -> Result<(), ReceivalConfirmationError> {
+	let mut expected_entry_begin = relayers.front().map(|entry| entry.messages.begin);
 	for entry in relayers {
 		// unrewarded relayer entry must have at least 1 unconfirmed message
 		// (guaranteed by the `InboundLane::receive_message()`)
 		if entry.messages.end < entry.messages.begin {
-			return Err(ReceivalConfirmationResult::EmptyUnrewardedRelayerEntry)
+			return Err(ReceivalConfirmationError::EmptyUnrewardedRelayerEntry)
 		}
 		// every entry must confirm range of messages that follows previous entry range
 		// (guaranteed by the `InboundLane::receive_message()`)
-		if let Some(last_entry_end) = last_entry_end {
-			let expected_entry_begin = last_entry_end.checked_add(1);
-			if expected_entry_begin != Some(entry.messages.begin) {
-				return Err(ReceivalConfirmationResult::NonConsecutiveUnrewardedRelayerEntries)
-			}
+		if expected_entry_begin != Some(entry.messages.begin) {
+			return Err(ReceivalConfirmationError::NonConsecutiveUnrewardedRelayerEntries)
 		}
-		last_entry_end = Some(entry.messages.end);
+		expected_entry_begin = entry.messages.end.checked_add(1);
 		// entry can't confirm messages larger than `inbound_lane_data.latest_received_nonce()`
 		// (guaranteed by the `InboundLane::receive_message()`)
 		if entry.messages.end > latest_received_nonce {
-			return Err(ReceivalConfirmationResult::FailedToConfirmFutureMessages)
+			return Err(ReceivalConfirmationError::FailedToConfirmFutureMessages)
 		}
 	}
 
@@ -231,7 +227,7 @@ mod tests {
 	fn assert_3_messages_confirmation_fails(
 		latest_received_nonce: MessageNonce,
 		relayers: &VecDeque<UnrewardedRelayer<TestRelayer>>,
-	) -> ReceivalConfirmationResult {
+	) -> Result<Option<DeliveredMessages>, ReceivalConfirmationError> {
 		run_test(|| {
 			let mut lane = outbound_lane::<TestRuntime, _>(TEST_LANE_ID);
 			lane.send_message(outbound_message_data(REGULAR_PAYLOAD));
@@ -268,7 +264,7 @@ mod tests {
 			assert_eq!(lane.storage.data().latest_received_nonce, 0);
 			assert_eq!(
 				lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)),
-				ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(1..=3)),
+				Ok(Some(delivered_messages(1..=3))),
 			);
 			assert_eq!(lane.storage.data().latest_generated_nonce, 3);
 			assert_eq!(lane.storage.data().latest_received_nonce, 3);
@@ -286,19 +282,13 @@ mod tests {
 			assert_eq!(lane.storage.data().latest_received_nonce, 0);
 			assert_eq!(
 				lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)),
-				ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(1..=3)),
-			);
-			assert_eq!(
-				lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)),
-				ReceivalConfirmationResult::NoNewConfirmations,
+				Ok(Some(delivered_messages(1..=3))),
 			);
+			assert_eq!(lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)), Ok(None),);
 			assert_eq!(lane.storage.data().latest_generated_nonce, 3);
 			assert_eq!(lane.storage.data().latest_received_nonce, 3);
 
-			assert_eq!(
-				lane.confirm_delivery(1, 2, &unrewarded_relayers(1..=1)),
-				ReceivalConfirmationResult::NoNewConfirmations,
-			);
+			assert_eq!(lane.confirm_delivery(1, 2, &unrewarded_relayers(1..=1)), Ok(None),);
 			assert_eq!(lane.storage.data().latest_generated_nonce, 3);
 			assert_eq!(lane.storage.data().latest_received_nonce, 3);
 		});
@@ -308,7 +298,7 @@ mod tests {
 	fn confirm_delivery_rejects_nonce_larger_than_last_generated() {
 		assert_eq!(
 			assert_3_messages_confirmation_fails(10, &unrewarded_relayers(1..=10),),
-			ReceivalConfirmationResult::FailedToConfirmFutureMessages,
+			Err(ReceivalConfirmationError::FailedToConfirmFutureMessages),
 		);
 	}
 
@@ -323,7 +313,7 @@ mod tests {
 					.chain(unrewarded_relayers(3..=3).into_iter())
 					.collect(),
 			),
-			ReceivalConfirmationResult::FailedToConfirmFutureMessages,
+			Err(ReceivalConfirmationError::FailedToConfirmFutureMessages),
 		);
 	}
 
@@ -339,7 +329,7 @@ mod tests {
 					.chain(unrewarded_relayers(2..=3).into_iter())
 					.collect(),
 			),
-			ReceivalConfirmationResult::EmptyUnrewardedRelayerEntry,
+			Err(ReceivalConfirmationError::EmptyUnrewardedRelayerEntry),
 		);
 	}
 
@@ -354,7 +344,7 @@ mod tests {
 					.chain(unrewarded_relayers(2..=2).into_iter())
 					.collect(),
 			),
-			ReceivalConfirmationResult::NonConsecutiveUnrewardedRelayerEntries,
+			Err(ReceivalConfirmationError::NonConsecutiveUnrewardedRelayerEntries),
 		);
 	}
 
@@ -383,7 +373,7 @@ mod tests {
 			// after confirmation, some messages are received
 			assert_eq!(
 				lane.confirm_delivery(2, 2, &unrewarded_relayers(1..=2)),
-				ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(1..=2)),
+				Ok(Some(delivered_messages(1..=2))),
 			);
 			assert_eq!(
 				lane.prune_messages(RocksDbWeight::get(), RocksDbWeight::get().writes(101)),
@@ -396,7 +386,7 @@ mod tests {
 			// after last message is confirmed, everything is pruned
 			assert_eq!(
 				lane.confirm_delivery(1, 3, &unrewarded_relayers(3..=3)),
-				ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(3..=3)),
+				Ok(Some(delivered_messages(3..=3))),
 			);
 			assert_eq!(
 				lane.prune_messages(RocksDbWeight::get(), RocksDbWeight::get().writes(101)),
@@ -418,15 +408,15 @@ mod tests {
 			lane.send_message(outbound_message_data(REGULAR_PAYLOAD));
 			assert_eq!(
 				lane.confirm_delivery(0, 3, &unrewarded_relayers(1..=3)),
-				ReceivalConfirmationResult::TryingToConfirmMoreMessagesThanExpected(3),
+				Err(ReceivalConfirmationError::TryingToConfirmMoreMessagesThanExpected),
 			);
 			assert_eq!(
 				lane.confirm_delivery(2, 3, &unrewarded_relayers(1..=3)),
-				ReceivalConfirmationResult::TryingToConfirmMoreMessagesThanExpected(3),
+				Err(ReceivalConfirmationError::TryingToConfirmMoreMessagesThanExpected),
 			);
 			assert_eq!(
 				lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)),
-				ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(1..=3)),
+				Ok(Some(delivered_messages(1..=3))),
 			);
 		});
 	}
