Remove some expect() statements (paritytech#2123)

* Return error on save_message()

Prevent unexpected failures.

* Remove RefCell

* Fix spellcheck

* CI fixes

Author: serban300

bridges/modules/messages/src/benchmarking.rs

@@ -17,8 +17,8 @@
 //! Messages pallet benchmarking.
 
 use crate::{
-	inbound_lane::InboundLaneStorage, inbound_lane_storage, outbound_lane,
-	weights_ext::EXPECTED_DEFAULT_MESSAGE_LENGTH, Call, OutboundLanes,
+	inbound_lane::InboundLaneStorage, outbound_lane, weights_ext::EXPECTED_DEFAULT_MESSAGE_LENGTH,
+	Call, OutboundLanes, RuntimeInboundLaneStorage,
 };
 
 use bp_messages::{
@@ -443,11 +443,12 @@ benchmarks_instance_pallet! {
 
 fn send_regular_message<T: Config<I>, I: 'static>() {
 	let mut outbound_lane = outbound_lane::<T, I>(T::bench_lane_id());
-	outbound_lane.send_message(vec![]);
+	outbound_lane.send_message(vec![]).expect("We craft valid messages");
 }
 
 fn receive_messages<T: Config<I>, I: 'static>(nonce: MessageNonce) {
-	let mut inbound_lane_storage = inbound_lane_storage::<T, I>(T::bench_lane_id());
+	let mut inbound_lane_storage =
+		RuntimeInboundLaneStorage::<T, I>::from_lane_id(T::bench_lane_id());
 	inbound_lane_storage.set_data(InboundLaneData {
 		relayers: vec![UnrewardedRelayer {
 			relayer: T::bridged_relayer_id(),

bridges/modules/messages/src/inbound_lane.rs

@@ -40,7 +40,7 @@ pub trait InboundLaneStorage {
 	/// Return maximal number of unconfirmed messages in inbound lane.
 	fn max_unconfirmed_messages(&self) -> MessageNonce;
 	/// Get lane data from the storage.
-	fn data(&self) -> InboundLaneData<Self::Relayer>;
+	fn get_or_init_data(&mut self) -> InboundLaneData<Self::Relayer>;
 	/// Update lane data in the storage.
 	fn set_data(&mut self, data: InboundLaneData<Self::Relayer>);
 }
@@ -117,17 +117,17 @@ impl<S: InboundLaneStorage> InboundLane<S> {
 		InboundLane { storage }
 	}
 
-	/// Returns storage reference.
-	pub fn storage(&self) -> &S {
-		&self.storage
+	/// Returns `mut` storage reference.
+	pub fn storage_mut(&mut self) -> &mut S {
+		&mut self.storage
 	}
 
 	/// Receive state of the corresponding outbound lane.
 	pub fn receive_state_update(
 		&mut self,
 		outbound_lane_data: OutboundLaneData,
 	) -> Option<MessageNonce> {
-		let mut data = self.storage.data();
+		let mut data = self.storage.get_or_init_data();
 		let last_delivered_nonce = data.last_delivered_nonce();
 
 		if outbound_lane_data.latest_received_nonce > last_delivered_nonce {
@@ -170,7 +170,7 @@ impl<S: InboundLaneStorage> InboundLane<S> {
 		nonce: MessageNonce,
 		message_data: DispatchMessageData<Dispatch::DispatchPayload>,
 	) -> ReceivalResult<Dispatch::DispatchLevelResult> {
-		let mut data = self.storage.data();
+		let mut data = self.storage.get_or_init_data();
 		let is_correct_message = nonce == data.last_delivered_nonce() + 1;
 		if !is_correct_message {
 			return ReceivalResult::InvalidNonce
@@ -252,7 +252,7 @@ mod tests {
 				None,
 			);
 
-			assert_eq!(lane.storage.data().last_confirmed_nonce, 0);
+			assert_eq!(lane.storage.get_or_init_data().last_confirmed_nonce, 0);
 		});
 	}
 
@@ -270,7 +270,7 @@ mod tests {
 				}),
 				Some(3),
 			);
-			assert_eq!(lane.storage.data().last_confirmed_nonce, 3);
+			assert_eq!(lane.storage.get_or_init_data().last_confirmed_nonce, 3);
 
 			assert_eq!(
 				lane.receive_state_update(OutboundLaneData {
@@ -279,7 +279,7 @@ mod tests {
 				}),
 				None,
 			);
-			assert_eq!(lane.storage.data().last_confirmed_nonce, 3);
+			assert_eq!(lane.storage.get_or_init_data().last_confirmed_nonce, 3);
 		});
 	}
 
@@ -290,9 +290,9 @@ mod tests {
 			receive_regular_message(&mut lane, 1);
 			receive_regular_message(&mut lane, 2);
 			receive_regular_message(&mut lane, 3);
-			assert_eq!(lane.storage.data().last_confirmed_nonce, 0);
+			assert_eq!(lane.storage.get_or_init_data().last_confirmed_nonce, 0);
 			assert_eq!(
-				lane.storage.data().relayers,
+				lane.storage.get_or_init_data().relayers,
 				vec![unrewarded_relayer(1, 3, TEST_RELAYER_A)]
 			);
 
@@ -303,9 +303,9 @@ mod tests {
 				}),
 				Some(2),
 			);
-			assert_eq!(lane.storage.data().last_confirmed_nonce, 2);
+			assert_eq!(lane.storage.get_or_init_data().last_confirmed_nonce, 2);
 			assert_eq!(
-				lane.storage.data().relayers,
+				lane.storage.get_or_init_data().relayers,
 				vec![unrewarded_relayer(3, 3, TEST_RELAYER_A)]
 			);
 
@@ -316,16 +316,16 @@ mod tests {
 				}),
 				Some(3),
 			);
-			assert_eq!(lane.storage.data().last_confirmed_nonce, 3);
-			assert_eq!(lane.storage.data().relayers, vec![]);
+			assert_eq!(lane.storage.get_or_init_data().last_confirmed_nonce, 3);
+			assert_eq!(lane.storage.get_or_init_data().relayers, vec![]);
 		});
 	}
 
 	#[test]
 	fn receive_status_update_works_with_batches_from_relayers() {
 		run_test(|| {
 			let mut lane = inbound_lane::<TestRuntime, _>(TEST_LANE_ID);
-			let mut seed_storage_data = lane.storage.data();
+			let mut seed_storage_data = lane.storage.get_or_init_data();
 			// Prepare data
 			seed_storage_data.last_confirmed_nonce = 0;
 			seed_storage_data.relayers.push_back(unrewarded_relayer(1, 1, TEST_RELAYER_A));
@@ -341,9 +341,9 @@ mod tests {
 				}),
 				Some(3),
 			);
-			assert_eq!(lane.storage.data().last_confirmed_nonce, 3);
+			assert_eq!(lane.storage.get_or_init_data().last_confirmed_nonce, 3);
 			assert_eq!(
-				lane.storage.data().relayers,
+				lane.storage.get_or_init_data().relayers,
 				vec![
 					unrewarded_relayer(4, 4, TEST_RELAYER_B),
 					unrewarded_relayer(5, 5, TEST_RELAYER_C)
@@ -364,7 +364,7 @@ mod tests {
 				),
 				ReceivalResult::InvalidNonce
 			);
-			assert_eq!(lane.storage.data().last_delivered_nonce(), 0);
+			assert_eq!(lane.storage.get_or_init_data().last_delivered_nonce(), 0);
 		});
 	}
 
@@ -470,7 +470,7 @@ mod tests {
 				ReceivalResult::Dispatched(dispatch_result(0))
 			);
 			assert_eq!(
-				lane.storage.data().relayers,
+				lane.storage.get_or_init_data().relayers,
 				vec![
 					unrewarded_relayer(1, 1, TEST_RELAYER_A),
 					unrewarded_relayer(2, 2, TEST_RELAYER_B),
@@ -508,7 +508,7 @@ mod tests {
 		run_test(|| {
 			let mut lane = inbound_lane::<TestRuntime, _>(TEST_LANE_ID);
 			receive_regular_message(&mut lane, 1);
-			assert_eq!(lane.storage.data().last_delivered_nonce(), 1);
+			assert_eq!(lane.storage.get_or_init_data().last_delivered_nonce(), 1);
 		});
 	}
 

bridges/modules/messages/src/lib.rs

@@ -67,7 +67,7 @@ use bp_runtime::{BasicOperatingMode, ChainId, OwnedBridgeModule, PreComputedSize
 use codec::{Decode, Encode, MaxEncodedLen};
 use frame_support::{dispatch::PostDispatchInfo, ensure, fail, traits::Get};
 use sp_runtime::traits::UniqueSaturatedFrom;
-use sp_std::{cell::RefCell, marker::PhantomData, prelude::*};
+use sp_std::{marker::PhantomData, prelude::*};
 
 mod inbound_lane;
 mod outbound_lane;
@@ -319,7 +319,7 @@ pub mod pallet {
 
 				// subtract extra storage proof bytes from the actual PoV size - there may be
 				// less unrewarded relayers than the maximal configured value
-				let lane_extra_proof_size_bytes = lane.storage().extra_proof_size_bytes();
+				let lane_extra_proof_size_bytes = lane.storage_mut().extra_proof_size_bytes();
 				actual_weight = actual_weight.set_proof_size(
 					actual_weight.proof_size().saturating_sub(lane_extra_proof_size_bytes),
 				);
@@ -332,7 +332,7 @@ pub mod pallet {
 							"Received lane {:?} state update: latest_confirmed_nonce={}. Unrewarded relayers: {:?}",
 							lane_id,
 							updated_latest_confirmed_nonce,
-							UnrewardedRelayersState::from(&lane.storage().data()),
+							UnrewardedRelayersState::from(&lane.storage_mut().get_or_init_data()),
 						);
 					}
 				}
@@ -531,12 +531,12 @@ pub mod pallet {
 		NotOperatingNormally,
 		/// The outbound lane is inactive.
 		InactiveOutboundLane,
-		/// The message is too large to be sent over the bridge.
-		MessageIsTooLarge,
 		/// Message has been treated as invalid by chain verifier.
 		MessageRejectedByChainVerifier(VerificationError),
 		/// Message has been treated as invalid by lane verifier.
 		MessageRejectedByLaneVerifier(VerificationError),
+		/// Message has been treated as invalid by the pallet logic.
+		MessageRejectedByPallet(VerificationError),
 		/// Submitter has failed to pay fee for delivering and dispatching messages.
 		FailedToWithdrawMessageFee,
 		/// The transaction brings too many messages.
@@ -727,11 +727,9 @@ fn send_message<T: Config<I>, I: 'static>(
 	// finally, save message in outbound storage and emit event
 	let encoded_payload = payload.encode();
 	let encoded_payload_len = encoded_payload.len();
-	ensure!(
-		encoded_payload_len <= T::MaximalOutboundPayloadSize::get() as usize,
-		Error::<T, I>::MessageIsTooLarge
-	);
-	let nonce = lane.send_message(encoded_payload);
+	let nonce = lane
+		.send_message(encoded_payload)
+		.map_err(Error::<T, I>::MessageRejectedByPallet)?;
 
 	log::trace!(
 		target: LOG_TARGET,
@@ -761,18 +759,7 @@ fn ensure_normal_operating_mode<T: Config<I>, I: 'static>() -> Result<(), Error<
 fn inbound_lane<T: Config<I>, I: 'static>(
 	lane_id: LaneId,
 ) -> InboundLane<RuntimeInboundLaneStorage<T, I>> {
-	InboundLane::new(inbound_lane_storage::<T, I>(lane_id))
-}
-
-/// Creates new runtime inbound lane storage.
-fn inbound_lane_storage<T: Config<I>, I: 'static>(
-	lane_id: LaneId,
-) -> RuntimeInboundLaneStorage<T, I> {
-	RuntimeInboundLaneStorage {
-		lane_id,
-		cached_data: RefCell::new(None),
-		_phantom: Default::default(),
-	}
+	InboundLane::new(RuntimeInboundLaneStorage::from_lane_id(lane_id))
 }
 
 /// Creates new outbound lane object, backed by runtime storage.
@@ -785,10 +772,17 @@ fn outbound_lane<T: Config<I>, I: 'static>(
 /// Runtime inbound lane storage.
 struct RuntimeInboundLaneStorage<T: Config<I>, I: 'static = ()> {
 	lane_id: LaneId,
-	cached_data: RefCell<Option<InboundLaneData<T::InboundRelayer>>>,
+	cached_data: Option<InboundLaneData<T::InboundRelayer>>,
 	_phantom: PhantomData<I>,
 }
 
+impl<T: Config<I>, I: 'static> RuntimeInboundLaneStorage<T, I> {
+	/// Creates new runtime inbound lane storage.
+	fn from_lane_id(lane_id: LaneId) -> RuntimeInboundLaneStorage<T, I> {
+		RuntimeInboundLaneStorage { lane_id, cached_data: None, _phantom: Default::default() }
+	}
+}
+
 impl<T: Config<I>, I: 'static> RuntimeInboundLaneStorage<T, I> {
 	/// Returns number of bytes that may be subtracted from the PoV component of
 	/// `receive_messages_proof` call, because the actual inbound lane state is smaller than the
@@ -798,9 +792,9 @@ impl<T: Config<I>, I: 'static> RuntimeInboundLaneStorage<T, I> {
 	/// `MaxUnrewardedRelayerEntriesAtInboundLane` constant from the pallet configuration. The PoV
 	/// of the call includes the maximal size of inbound lane state. If the actual size is smaller,
 	/// we may subtract extra bytes from this component.
-	pub fn extra_proof_size_bytes(&self) -> u64 {
+	pub fn extra_proof_size_bytes(&mut self) -> u64 {
 		let max_encoded_len = StoredInboundLaneData::<T, I>::max_encoded_len();
-		let relayers_count = self.data().relayers.len();
+		let relayers_count = self.get_or_init_data().relayers.len();
 		let actual_encoded_len =
 			InboundLaneData::<T::InboundRelayer>::encoded_size_hint(relayers_count)
 				.unwrap_or(usize::MAX);
@@ -823,26 +817,20 @@ impl<T: Config<I>, I: 'static> InboundLaneStorage for RuntimeInboundLaneStorage<
 		T::MaxUnconfirmedMessagesAtInboundLane::get()
 	}
 
-	fn data(&self) -> InboundLaneData<T::InboundRelayer> {
-		match self.cached_data.clone().into_inner() {
-			Some(data) => data,
+	fn get_or_init_data(&mut self) -> InboundLaneData<T::InboundRelayer> {
+		match self.cached_data {
+			Some(ref data) => data.clone(),
 			None => {
 				let data: InboundLaneData<T::InboundRelayer> =
 					InboundLanes::<T, I>::get(self.lane_id).into();
-				*self.cached_data.try_borrow_mut().expect(
-					"we're in the single-threaded environment;\
-						we have no recursive borrows; qed",
-				) = Some(data.clone());
+				self.cached_data = Some(data.clone());
 				data
 			},
 		}
 	}
 
 	fn set_data(&mut self, data: InboundLaneData<T::InboundRelayer>) {
-		*self.cached_data.try_borrow_mut().expect(
-			"we're in the single-threaded environment;\
-				we have no recursive borrows; qed",
-		) = Some(data.clone());
+		self.cached_data = Some(data.clone());
 		InboundLanes::<T, I>::insert(self.lane_id, StoredInboundLaneData::<T, I>(data))
 	}
 }
@@ -872,15 +860,17 @@ impl<T: Config<I>, I: 'static> OutboundLaneStorage for RuntimeOutboundLaneStorag
 			.map(Into::into)
 	}
 
-	fn save_message(&mut self, nonce: MessageNonce, message_payload: MessagePayload) {
+	fn save_message(
+		&mut self,
+		nonce: MessageNonce,
+		message_payload: MessagePayload,
+	) -> Result<(), VerificationError> {
 		OutboundMessages::<T, I>::insert(
 			MessageKey { lane_id: self.lane_id, nonce },
-			StoredMessagePayload::<T, I>::try_from(message_payload).expect(
-				"save_message is called after all checks in send_message; \
-					send_message checks message size; \
-					qed",
-			),
+			StoredMessagePayload::<T, I>::try_from(message_payload)
+				.map_err(|_| VerificationError::MessageTooLarge)?,
 		);
+		Ok(())
 	}
 
 	fn remove_message(&mut self, nonce: &MessageNonce) {
@@ -1128,7 +1118,9 @@ mod tests {
 					TEST_LANE_ID,
 					message_payload.clone(),
 				),
-				Error::<TestRuntime, ()>::MessageIsTooLarge,
+				Error::<TestRuntime, ()>::MessageRejectedByPallet(
+					VerificationError::MessageTooLarge
+				),
 			);
 
 			// let's check that we're able to send `MAX_OUTBOUND_PAYLOAD_SIZE` messages
@@ -2097,10 +2089,10 @@ mod tests {
 		fn storage(relayer_entries: usize) -> RuntimeInboundLaneStorage<TestRuntime, ()> {
 			RuntimeInboundLaneStorage {
 				lane_id: Default::default(),
-				cached_data: RefCell::new(Some(InboundLaneData {
+				cached_data: Some(InboundLaneData {
 					relayers: vec![relayer_entry(); relayer_entries].into_iter().collect(),
 					last_confirmed_nonce: 0,
-				})),
+				}),
 				_phantom: Default::default(),
 			}
 		}