From df11df83197071d81a47528fd62a03d629fbf37a Mon Sep 17 00:00:00 2001 From: Serban Iorga Date: Tue, 9 May 2023 11:48:24 +0300 Subject: [PATCH] Remove some `expect()` statements (#2123) * Return error on save_message() Prevent unexpected failures. * Remove RefCell * Fix spellcheck * CI fixes --- bridges/modules/messages/src/benchmarking.rs | 9 +- bridges/modules/messages/src/inbound_lane.rs | 42 +++++----- bridges/modules/messages/src/lib.rs | 82 +++++++++---------- bridges/modules/messages/src/outbound_lane.rs | 50 ++++++----- 4 files changed, 92 insertions(+), 91 deletions(-) diff --git a/bridges/modules/messages/src/benchmarking.rs b/bridges/modules/messages/src/benchmarking.rs index aab8855a729a8..04f64b53b305f 100644 --- a/bridges/modules/messages/src/benchmarking.rs +++ b/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, I: 'static>() { let mut outbound_lane = outbound_lane::(T::bench_lane_id()); - outbound_lane.send_message(vec![]); + outbound_lane.send_message(vec![]).expect("We craft valid messages"); } fn receive_messages, I: 'static>(nonce: MessageNonce) { - let mut inbound_lane_storage = inbound_lane_storage::(T::bench_lane_id()); + let mut inbound_lane_storage = + RuntimeInboundLaneStorage::::from_lane_id(T::bench_lane_id()); inbound_lane_storage.set_data(InboundLaneData { relayers: vec![UnrewardedRelayer { relayer: T::bridged_relayer_id(), diff --git a/bridges/modules/messages/src/inbound_lane.rs b/bridges/modules/messages/src/inbound_lane.rs index 5ec4444dbdfbe..2912c89cd1bee 100644 --- a/bridges/modules/messages/src/inbound_lane.rs +++ b/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; + fn get_or_init_data(&mut self) -> InboundLaneData; /// Update lane data in the storage. fn set_data(&mut self, data: InboundLaneData); } @@ -117,9 +117,9 @@ impl InboundLane { 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. @@ -127,7 +127,7 @@ impl InboundLane { &mut self, outbound_lane_data: OutboundLaneData, ) -> Option { - 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 InboundLane { nonce: MessageNonce, message_data: DispatchMessageData, ) -> ReceivalResult { - 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,8 +316,8 @@ 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![]); }); } @@ -325,7 +325,7 @@ mod tests { fn receive_status_update_works_with_batches_from_relayers() { run_test(|| { let mut lane = inbound_lane::(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::(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); }); } diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs index 02255e15082f6..307de89d54eca 100644 --- a/bridges/modules/messages/src/lib.rs +++ b/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, 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::::MessageIsTooLarge - ); - let nonce = lane.send_message(encoded_payload); + let nonce = lane + .send_message(encoded_payload) + .map_err(Error::::MessageRejectedByPallet)?; log::trace!( target: LOG_TARGET, @@ -761,18 +759,7 @@ fn ensure_normal_operating_mode, I: 'static>() -> Result<(), Error< fn inbound_lane, I: 'static>( lane_id: LaneId, ) -> InboundLane> { - InboundLane::new(inbound_lane_storage::(lane_id)) -} - -/// Creates new runtime inbound lane storage. -fn inbound_lane_storage, I: 'static>( - lane_id: LaneId, -) -> RuntimeInboundLaneStorage { - 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, I: 'static>( /// Runtime inbound lane storage. struct RuntimeInboundLaneStorage, I: 'static = ()> { lane_id: LaneId, - cached_data: RefCell>>, + cached_data: Option>, _phantom: PhantomData, } +impl, I: 'static> RuntimeInboundLaneStorage { + /// Creates new runtime inbound lane storage. + fn from_lane_id(lane_id: LaneId) -> RuntimeInboundLaneStorage { + RuntimeInboundLaneStorage { lane_id, cached_data: None, _phantom: Default::default() } + } +} + impl, I: 'static> RuntimeInboundLaneStorage { /// 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, I: 'static> RuntimeInboundLaneStorage { /// `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::::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::::encoded_size_hint(relayers_count) .unwrap_or(usize::MAX); @@ -823,26 +817,20 @@ impl, I: 'static> InboundLaneStorage for RuntimeInboundLaneStorage< T::MaxUnconfirmedMessagesAtInboundLane::get() } - fn data(&self) -> InboundLaneData { - match self.cached_data.clone().into_inner() { - Some(data) => data, + fn get_or_init_data(&mut self) -> InboundLaneData { + match self.cached_data { + Some(ref data) => data.clone(), None => { let data: InboundLaneData = InboundLanes::::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) { - *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::::insert(self.lane_id, StoredInboundLaneData::(data)) } } @@ -872,15 +860,17 @@ impl, 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::::insert( MessageKey { lane_id: self.lane_id, nonce }, - StoredMessagePayload::::try_from(message_payload).expect( - "save_message is called after all checks in send_message; \ - send_message checks message size; \ - qed", - ), + StoredMessagePayload::::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::::MessageIsTooLarge, + Error::::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 { 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(), } } diff --git a/bridges/modules/messages/src/outbound_lane.rs b/bridges/modules/messages/src/outbound_lane.rs index bc4fdfab9195d..0cf0c4ccb425c 100644 --- a/bridges/modules/messages/src/outbound_lane.rs +++ b/bridges/modules/messages/src/outbound_lane.rs @@ -20,6 +20,7 @@ use crate::{Config, LOG_TARGET}; use bp_messages::{ DeliveredMessages, LaneId, MessageNonce, MessagePayload, OutboundLaneData, UnrewardedRelayer, + VerificationError, }; use codec::{Decode, Encode}; use frame_support::{ @@ -42,7 +43,11 @@ pub trait OutboundLaneStorage { #[cfg(test)] fn message(&self, nonce: &MessageNonce) -> Option; /// Save outbound message in the storage. - fn save_message(&mut self, nonce: MessageNonce, message_payload: MessagePayload); + fn save_message( + &mut self, + nonce: MessageNonce, + message_payload: MessagePayload, + ) -> Result<(), VerificationError>; /// Remove outbound message from the storage. fn remove_message(&mut self, nonce: &MessageNonce); } @@ -85,15 +90,18 @@ impl OutboundLane { /// Send message over lane. /// /// Returns new message nonce. - pub fn send_message(&mut self, message_payload: MessagePayload) -> MessageNonce { + pub fn send_message( + &mut self, + message_payload: MessagePayload, + ) -> Result { let mut data = self.storage.data(); let nonce = data.latest_generated_nonce + 1; data.latest_generated_nonce = nonce; - self.storage.save_message(nonce, message_payload); + self.storage.save_message(nonce, message_payload)?; self.storage.set_data(data); - nonce + Ok(nonce) } /// Confirm messages delivery. @@ -209,7 +217,7 @@ mod tests { }, outbound_lane, }; - use frame_support::weights::constants::RocksDbWeight; + use frame_support::{assert_ok, weights::constants::RocksDbWeight}; use sp_std::ops::RangeInclusive; fn unrewarded_relayers( @@ -230,9 +238,9 @@ mod tests { ) -> Result, ReceivalConfirmationError> { run_test(|| { let mut lane = outbound_lane::(TEST_LANE_ID); - lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); - lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); - lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); + assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); + assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); + assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 0); let result = lane.confirm_delivery(3, latest_received_nonce, relayers); @@ -247,7 +255,7 @@ mod tests { run_test(|| { let mut lane = outbound_lane::(TEST_LANE_ID); assert_eq!(lane.storage.data().latest_generated_nonce, 0); - assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), 1); + assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), Ok(1)); assert!(lane.storage.message(&1).is_some()); assert_eq!(lane.storage.data().latest_generated_nonce, 1); }); @@ -257,9 +265,9 @@ mod tests { fn confirm_delivery_works() { run_test(|| { let mut lane = outbound_lane::(TEST_LANE_ID); - assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), 1); - assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), 2); - assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), 3); + assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), Ok(1)); + assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), Ok(2)); + assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), Ok(3)); assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 0); assert_eq!( @@ -275,9 +283,9 @@ mod tests { fn confirm_delivery_rejects_nonce_lesser_than_latest_received() { run_test(|| { let mut lane = outbound_lane::(TEST_LANE_ID); - lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); - lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); - lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); + assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); + assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); + assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 0); assert_eq!( @@ -359,9 +367,9 @@ mod tests { ); assert_eq!(lane.storage.data().oldest_unpruned_nonce, 1); // when nothing is confirmed, nothing is pruned - lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); - lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); - lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); + assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); + assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); + assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); assert!(lane.storage.message(&1).is_some()); assert!(lane.storage.message(&2).is_some()); assert!(lane.storage.message(&3).is_some()); @@ -403,9 +411,9 @@ mod tests { fn confirm_delivery_detects_when_more_than_expected_messages_are_confirmed() { run_test(|| { let mut lane = outbound_lane::(TEST_LANE_ID); - lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); - lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); - lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); + assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); + assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); + assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); assert_eq!( lane.confirm_delivery(0, 3, &unrewarded_relayers(1..=3)), Err(ReceivalConfirmationError::TryingToConfirmMoreMessagesThanExpected),