Cosmetics (paritytech#2124)

* Cosmetics

* Address PR comment

Author: serban300

modules/messages/src/lib.rs

@@ -59,9 +59,9 @@ use bp_messages::{
 		DeliveryPayments, DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages,
 		SourceHeaderChain,
 	},
-	total_unrewarded_messages, DeliveredMessages, InboundLaneData, InboundMessageDetails, LaneId,
-	MessageKey, MessageNonce, MessagePayload, MessagesOperatingMode, OutboundLaneData,
-	OutboundMessageDetails, UnrewardedRelayersState, VerificationError,
+	DeliveredMessages, InboundLaneData, InboundMessageDetails, LaneId, MessageKey, MessageNonce,
+	MessagePayload, MessagesOperatingMode, OutboundLaneData, OutboundMessageDetails,
+	UnrewardedRelayersState, VerificationError,
 };
 use bp_runtime::{BasicOperatingMode, ChainId, OwnedBridgeModule, PreComputedSize, Size};
 use codec::{Decode, Encode, MaxEncodedLen};
@@ -437,21 +437,8 @@ pub mod pallet {
 
 					Error::<T, I>::InvalidMessagesDeliveryProof
 				})?;
-
-			// verify that the relayer has declared correct `lane_data::relayers` state
-			// (we only care about total number of entries and messages, because this affects call
-			// weight)
-			ensure!(
-				total_unrewarded_messages(&lane_data.relayers).unwrap_or(MessageNonce::MAX) ==
-					relayers_state.total_messages &&
-					lane_data.relayers.len() as MessageNonce ==
-						relayers_state.unrewarded_relayer_entries,
-				Error::<T, I>::InvalidUnrewardedRelayersState
-			);
-			// the `last_delivered_nonce` field may also be used by the signed extension. Even
-			// though providing wrong value isn't critical, let's also check it here.
 			ensure!(
-				lane_data.last_delivered_nonce() == relayers_state.last_delivered_nonce,
+				relayers_state.is_valid(&lane_data),
 				Error::<T, I>::InvalidUnrewardedRelayersState
 			);
 
@@ -975,9 +962,9 @@ mod tests {
 			))),
 			UnrewardedRelayersState {
 				unrewarded_relayer_entries: 1,
+				messages_in_oldest_entry: 1,
 				total_messages: 1,
 				last_delivered_nonce: 1,
-				..Default::default()
 			},
 		));
 
@@ -1341,9 +1328,9 @@ mod tests {
 				single_message_delivery_proof,
 				UnrewardedRelayersState {
 					unrewarded_relayer_entries: 1,
+					messages_in_oldest_entry: 1,
 					total_messages: 1,
 					last_delivered_nonce: 1,
-					..Default::default()
 				},
 			);
 			assert_ok!(result);
@@ -1381,9 +1368,9 @@ mod tests {
 				two_messages_delivery_proof,
 				UnrewardedRelayersState {
 					unrewarded_relayer_entries: 2,
+					messages_in_oldest_entry: 1,
 					total_messages: 2,
 					last_delivered_nonce: 2,
-					..Default::default()
 				},
 			);
 			assert_ok!(result);
@@ -1746,9 +1733,9 @@ mod tests {
 				TestMessagesDeliveryProof(messages_1_and_2_proof),
 				UnrewardedRelayersState {
 					unrewarded_relayer_entries: 1,
+					messages_in_oldest_entry: 2,
 					total_messages: 2,
 					last_delivered_nonce: 2,
-					..Default::default()
 				},
 			));
 			// second tx with message 3
@@ -1757,9 +1744,9 @@ mod tests {
 				TestMessagesDeliveryProof(messages_3_proof),
 				UnrewardedRelayersState {
 					unrewarded_relayer_entries: 1,
+					messages_in_oldest_entry: 1,
 					total_messages: 1,
 					last_delivered_nonce: 3,
-					..Default::default()
 				},
 			));
 		});

primitives/messages/src/lib.rs

@@ -191,6 +191,17 @@ impl<RelayerId> InboundLaneData<RelayerId> {
 			.map(|entry| entry.messages.end)
 			.unwrap_or(self.last_confirmed_nonce)
 	}
+
+	/// Returns the total number of messages in the `relayers` vector,
+	/// saturating in case of underflow or overflow.
+	pub fn total_unrewarded_messages(&self) -> MessageNonce {
+		let relayers = &self.relayers;
+		match (relayers.front(), relayers.back()) {
+			(Some(front), Some(back)) =>
+				(front.messages.begin..=back.messages.end).saturating_len(),
+			_ => 0,
+		}
+	}
 }
 
 /// Outbound message details, returned by runtime APIs.
@@ -287,7 +298,7 @@ impl DeliveredMessages {
 
 	/// Return total count of delivered messages.
 	pub fn total_messages(&self) -> MessageNonce {
-		(self.begin..=self.end).checked_len().unwrap_or(0)
+		(self.begin..=self.end).saturating_len()
 	}
 
 	/// Note new dispatched message.
@@ -318,16 +329,23 @@ pub struct UnrewardedRelayersState {
 	pub last_delivered_nonce: MessageNonce,
 }
 
+impl UnrewardedRelayersState {
+	// Verify that the relayers state corresponds with the `InboundLaneData`.
+	pub fn is_valid<RelayerId>(&self, lane_data: &InboundLaneData<RelayerId>) -> bool {
+		self == &lane_data.into()
+	}
+}
+
 impl<RelayerId> From<&InboundLaneData<RelayerId>> for UnrewardedRelayersState {
 	fn from(lane: &InboundLaneData<RelayerId>) -> UnrewardedRelayersState {
 		UnrewardedRelayersState {
 			unrewarded_relayer_entries: lane.relayers.len() as _,
 			messages_in_oldest_entry: lane
 				.relayers
 				.front()
-				.and_then(|entry| (entry.messages.begin..=entry.messages.end).checked_len())
+				.map(|entry| entry.messages.total_messages())
 				.unwrap_or(0),
-			total_messages: total_unrewarded_messages(&lane.relayers).unwrap_or(MessageNonce::MAX),
+			total_messages: lane.total_unrewarded_messages(),
 			last_delivered_nonce: lane.last_delivered_nonce(),
 		}
 	}
@@ -357,24 +375,6 @@ impl Default for OutboundLaneData {
 	}
 }
 
-/// Returns total number of messages in the `InboundLaneData::relayers` vector.
-///
-/// Returns `None` if there are more messages that `MessageNonce` may fit (i.e. `MessageNonce + 1`).
-pub fn total_unrewarded_messages<RelayerId>(
-	relayers: &VecDeque<UnrewardedRelayer<RelayerId>>,
-) -> Option<MessageNonce> {
-	match (relayers.front(), relayers.back()) {
-		(Some(front), Some(back)) => {
-			if let Some(difference) = back.messages.end.checked_sub(front.messages.begin) {
-				difference.checked_add(1)
-			} else {
-				Some(0)
-			}
-		},
-		_ => Some(0),
-	}
-}
-
 /// Calculate the number of messages that the relayers have delivered.
 pub fn calc_relayers_rewards<AccountId>(
 	messages_relayers: VecDeque<UnrewardedRelayer<AccountId>>,
@@ -447,20 +447,19 @@ mod tests {
 
 	#[test]
 	fn total_unrewarded_messages_does_not_overflow() {
-		assert_eq!(
-			total_unrewarded_messages(
-				&vec![
-					UnrewardedRelayer { relayer: 1, messages: DeliveredMessages::new(0) },
-					UnrewardedRelayer {
-						relayer: 2,
-						messages: DeliveredMessages::new(MessageNonce::MAX)
-					},
-				]
-				.into_iter()
-				.collect()
-			),
-			None,
-		);
+		let lane_data = InboundLaneData {
+			relayers: vec![
+				UnrewardedRelayer { relayer: 1, messages: DeliveredMessages::new(0) },
+				UnrewardedRelayer {
+					relayer: 2,
+					messages: DeliveredMessages::new(MessageNonce::MAX),
+				},
+			]
+			.into_iter()
+			.collect(),
+			last_confirmed_nonce: 0,
+		};
+		assert_eq!(lane_data.total_unrewarded_messages(), MessageNonce::MAX);
 	}
 
 	#[test]

primitives/runtime/src/lib.rs

@@ -35,7 +35,7 @@ pub use chain::{
 	UnderlyingChainProvider,
 };
 pub use frame_support::storage::storage_prefix as storage_value_final_key;
-use num_traits::{CheckedAdd, CheckedSub, One};
+use num_traits::{CheckedAdd, CheckedSub, One, SaturatingAdd, Zero};
 pub use storage_proof::{
 	record_all_keys as record_all_trie_keys, Error as StorageProofError,
 	ProofSize as StorageProofSize, RawStorageProof, StorageProofChecker,
@@ -527,17 +527,27 @@ impl<T> Debug for StrippableError<T> {
 pub trait RangeInclusiveExt<Idx> {
 	/// Computes the length of the `RangeInclusive`, checking for underflow and overflow.
 	fn checked_len(&self) -> Option<Idx>;
+	/// Computes the length of the `RangeInclusive`, saturating in case of underflow or overflow.
+	fn saturating_len(&self) -> Idx;
 }
 
 impl<Idx> RangeInclusiveExt<Idx> for RangeInclusive<Idx>
 where
-	Idx: CheckedSub + CheckedAdd + One,
+	Idx: CheckedSub + CheckedAdd + SaturatingAdd + One + Zero,
 {
 	fn checked_len(&self) -> Option<Idx> {
 		self.end()
 			.checked_sub(self.start())
 			.and_then(|len| len.checked_add(&Idx::one()))
 	}
+
+	fn saturating_len(&self) -> Idx {
+		let len = match self.end().checked_sub(self.start()) {
+			Some(len) => len,
+			None => return Idx::zero(),
+		};
+		len.saturating_add(&Idx::one())
+	}
 }
 
 #[cfg(test)]

relays/lib-substrate-relay/src/messages_target.rs

@@ -31,8 +31,8 @@ use crate::{
 use async_std::sync::Arc;
 use async_trait::async_trait;
 use bp_messages::{
-	storage_keys::inbound_lane_data_key, total_unrewarded_messages, InboundLaneData, LaneId,
-	MessageNonce, UnrewardedRelayersState,
+	storage_keys::inbound_lane_data_key, InboundLaneData, LaneId, MessageNonce,
+	UnrewardedRelayersState,
 };
 use bridge_runtime_common::messages::source::FromBridgedChainMessagesDeliveryProof;
 use messages_relay::{
@@ -45,7 +45,7 @@ use relay_substrate_client::{
 };
 use relay_utils::relay_loop::Client as RelayClient;
 use sp_core::Pair;
-use std::{collections::VecDeque, convert::TryFrom, ops::RangeInclusive};
+use std::{convert::TryFrom, ops::RangeInclusive};
 
 /// Message receiving proof returned by the target Substrate node.
 pub type SubstrateMessagesDeliveryProof<C> =
@@ -197,20 +197,9 @@ where
 		id: TargetHeaderIdOf<MessageLaneAdapter<P>>,
 	) -> Result<(TargetHeaderIdOf<MessageLaneAdapter<P>>, UnrewardedRelayersState), SubstrateError>
 	{
-		let inbound_lane_data = self.inbound_lane_data(id).await?;
-		let last_delivered_nonce =
-			inbound_lane_data.as_ref().map(|data| data.last_delivered_nonce()).unwrap_or(0);
-		let relayers = inbound_lane_data.map(|data| data.relayers).unwrap_or_else(VecDeque::new);
-		let unrewarded_relayers_state = bp_messages::UnrewardedRelayersState {
-			unrewarded_relayer_entries: relayers.len() as _,
-			messages_in_oldest_entry: relayers
-				.front()
-				.map(|entry| 1 + entry.messages.end - entry.messages.begin)
-				.unwrap_or(0),
-			total_messages: total_unrewarded_messages(&relayers).unwrap_or(MessageNonce::MAX),
-			last_delivered_nonce,
-		};
-		Ok((id, unrewarded_relayers_state))
+		let inbound_lane_data =
+			self.inbound_lane_data(id).await?.unwrap_or(InboundLaneData::default());
+		Ok((id, (&inbound_lane_data).into()))
 	}
 
 	async fn prove_messages_receiving(