Propagate message verification errors (#2114)

* Propagate message verification errors

* Replace parse_finalized_storage_proof() with storage_proof_checker()

* small fixes

* fix comment

bridges/modules/grandpa/src/lib.rs

@@ -1212,11 +1212,8 @@ mod tests {
 	fn parse_finalized_storage_proof_rejects_proof_on_unknown_header() {
 		run_test(|| {
 			assert_noop!(
-				Pallet::<TestRuntime>::parse_finalized_storage_proof(
-					Default::default(),
-					vec![],
-					|_| (),
-				),
+				Pallet::<TestRuntime>::storage_proof_checker(Default::default(), vec![],)
+					.map(|_| ()),
 				bp_header_chain::HeaderChainError::UnknownHeader,
 			);
 		});
@@ -1235,8 +1232,7 @@ mod tests {
 			<ImportedHeaders<TestRuntime>>::insert(hash, header.build());
 
 			assert_ok!(
-				Pallet::<TestRuntime>::parse_finalized_storage_proof(hash, storage_proof, |_| (),),
-				(),
+				Pallet::<TestRuntime>::storage_proof_checker(hash, storage_proof).map(|_| ())
 			);
 		});
 	}

bridges/modules/messages/src/lib.rs

@@ -61,7 +61,7 @@ use bp_messages::{
 	},
 	total_unrewarded_messages, DeliveredMessages, InboundLaneData, InboundMessageDetails, LaneId,
 	MessageKey, MessageNonce, MessagePayload, MessagesOperatingMode, OutboundLaneData,
-	OutboundMessageDetails, UnrewardedRelayersState,
+	OutboundMessageDetails, UnrewardedRelayersState, VerificationError,
 };
 use bp_runtime::{BasicOperatingMode, ChainId, OwnedBridgeModule, PreComputedSize, Size};
 use codec::{Decode, Encode, MaxEncodedLen};
@@ -557,9 +557,9 @@ pub mod pallet {
 		/// The message is too large to be sent over the bridge.
 		MessageIsTooLarge,
 		/// Message has been treated as invalid by chain verifier.
-		MessageRejectedByChainVerifier,
+		MessageRejectedByChainVerifier(VerificationError),
 		/// Message has been treated as invalid by lane verifier.
-		MessageRejectedByLaneVerifier,
+		MessageRejectedByLaneVerifier(VerificationError),
 		/// Submitter has failed to pay fee for delivering and dispatching messages.
 		FailedToWithdrawMessageFee,
 		/// The transaction brings too many messages.
@@ -732,7 +732,7 @@ fn send_message<T: Config<I>, I: 'static>(
 			err,
 		);
 
-		Error::<T, I>::MessageRejectedByChainVerifier
+		Error::<T, I>::MessageRejectedByChainVerifier(err)
 	})?;
 
 	// now let's enforce any additional lane rules
@@ -746,7 +746,7 @@ fn send_message<T: Config<I>, I: 'static>(
 				err,
 			);
 
-			Error::<T, I>::MessageRejectedByLaneVerifier
+			Error::<T, I>::MessageRejectedByLaneVerifier(err)
 		},
 	)?;
 
@@ -918,7 +918,7 @@ impl<T: Config<I>, I: 'static> OutboundLaneStorage for RuntimeOutboundLaneStorag
 fn verify_and_decode_messages_proof<Chain: SourceHeaderChain, DispatchPayload: Decode>(
 	proof: Chain::MessagesProof,
 	messages_count: u32,
-) -> Result<ProvedMessages<DispatchMessage<DispatchPayload>>, Chain::Error> {
+) -> Result<ProvedMessages<DispatchMessage<DispatchPayload>>, VerificationError> {
 	// `receive_messages_proof` weight formula and `MaxUnconfirmedMessagesAtInboundLane` check
 	// guarantees that the `message_count` is sane and Vec<Message> may be allocated.
 	// (tx with too many messages will either be rejected from the pool, or will fail earlier)
@@ -1177,7 +1177,9 @@ mod tests {
 					TEST_LANE_ID,
 					PAYLOAD_REJECTED_BY_TARGET_CHAIN,
 				),
-				Error::<TestRuntime, ()>::MessageRejectedByChainVerifier,
+				Error::<TestRuntime, ()>::MessageRejectedByChainVerifier(VerificationError::Other(
+					mock::TEST_ERROR
+				)),
 			);
 		});
 	}
@@ -1190,7 +1192,9 @@ mod tests {
 			message.reject_by_lane_verifier = true;
 			assert_noop!(
 				send_message::<TestRuntime, ()>(RuntimeOrigin::signed(1), TEST_LANE_ID, message,),
-				Error::<TestRuntime, ()>::MessageRejectedByLaneVerifier,
+				Error::<TestRuntime, ()>::MessageRejectedByLaneVerifier(VerificationError::Other(
+					mock::TEST_ERROR
+				)),
 			);
 		});
 	}

bridges/modules/messages/src/mock.rs

@@ -27,7 +27,7 @@ use bp_messages::{
 		ProvedLaneMessages, ProvedMessages, SourceHeaderChain,
 	},
 	DeliveredMessages, InboundLaneData, LaneId, Message, MessageKey, MessageNonce, MessagePayload,
-	OutboundLaneData, UnrewardedRelayer, UnrewardedRelayersState,
+	OutboundLaneData, UnrewardedRelayer, UnrewardedRelayersState, VerificationError,
 };
 use bp_runtime::{messages::MessageDispatchResult, Size};
 use codec::{Decode, Encode};
@@ -295,13 +295,11 @@ impl Size for TestMessagesDeliveryProof {
 pub struct TestTargetHeaderChain;
 
 impl TargetHeaderChain<TestPayload, TestRelayer> for TestTargetHeaderChain {
-	type Error = &'static str;
-
 	type MessagesDeliveryProof = TestMessagesDeliveryProof;
 
-	fn verify_message(payload: &TestPayload) -> Result<(), Self::Error> {
+	fn verify_message(payload: &TestPayload) -> Result<(), VerificationError> {
 		if *payload == PAYLOAD_REJECTED_BY_TARGET_CHAIN {
-			Err(TEST_ERROR)
+			Err(VerificationError::Other(TEST_ERROR))
 		} else {
 			Ok(())
 		}
@@ -309,8 +307,8 @@ impl TargetHeaderChain<TestPayload, TestRelayer> for TestTargetHeaderChain {
 
 	fn verify_messages_delivery_proof(
 		proof: Self::MessagesDeliveryProof,
-	) -> Result<(LaneId, InboundLaneData<TestRelayer>), Self::Error> {
-		proof.0.map_err(|_| TEST_ERROR)
+	) -> Result<(LaneId, InboundLaneData<TestRelayer>), VerificationError> {
+		proof.0.map_err(|_| VerificationError::Other(TEST_ERROR))
 	}
 }
 
@@ -319,18 +317,16 @@ impl TargetHeaderChain<TestPayload, TestRelayer> for TestTargetHeaderChain {
 pub struct TestLaneMessageVerifier;
 
 impl LaneMessageVerifier<RuntimeOrigin, TestPayload> for TestLaneMessageVerifier {
-	type Error = &'static str;
-
 	fn verify_message(
 		_submitter: &RuntimeOrigin,
 		_lane: &LaneId,
 		_lane_outbound_data: &OutboundLaneData,
 		payload: &TestPayload,
-	) -> Result<(), Self::Error> {
+	) -> Result<(), VerificationError> {
 		if !payload.reject_by_lane_verifier {
 			Ok(())
 		} else {
-			Err(TEST_ERROR)
+			Err(VerificationError::Other(TEST_ERROR))
 		}
 	}
 }
@@ -400,15 +396,16 @@ impl DeliveryConfirmationPayments<AccountId> for TestDeliveryConfirmationPayment
 pub struct TestSourceHeaderChain;
 
 impl SourceHeaderChain for TestSourceHeaderChain {
-	type Error = &'static str;
-
 	type MessagesProof = TestMessagesProof;
 
 	fn verify_messages_proof(
 		proof: Self::MessagesProof,
 		_messages_count: u32,
-	) -> Result<ProvedMessages<Message>, Self::Error> {
-		proof.result.map(|proof| proof.into_iter().collect()).map_err(|_| TEST_ERROR)
+	) -> Result<ProvedMessages<Message>, VerificationError> {
+		proof
+			.result
+			.map(|proof| proof.into_iter().collect())
+			.map_err(|_| VerificationError::Other(TEST_ERROR))
 	}
 }
 

bridges/modules/parachains/src/lib.rs

@@ -91,6 +91,8 @@ pub mod pallet {
 		BoundedStorageValue<<T as Config<I>>::MaxParaHeadDataSize, ParaStoredHeaderData>;
 	/// Weight info of the given parachains pallet.
 	pub type WeightInfoOf<T, I> = <T as Config<I>>::WeightInfo;
+	type GrandpaPalletOf<T, I> =
+		pallet_bridge_grandpa::Pallet<T, <T as Config<I>>::BridgesGrandpaPalletInstance>;
 
 	#[pallet::event]
 	#[pallet::generate_deposit(pub(super) fn deposit_event)]
@@ -125,14 +127,8 @@ pub mod pallet {
 		UnknownRelayChainBlock,
 		/// The number of stored relay block is different from what the relayer has provided.
 		InvalidRelayChainBlockNumber,
-		/// Error generated by a method defined in `bp-header-chain`.
-		HeaderChain(HeaderChainError),
-		/// Given parachain head is unknown.
-		UnknownParaHead,
-		/// The storage proof doesn't contains storage root. So it is invalid for given header.
-		StorageRootMismatch,
-		/// Failed to extract state root from given parachain head.
-		FailedToExtractStateRoot,
+		/// Parachain heads storage proof is invalid.
+		HeaderChainStorageProof(HeaderChainError),
 		/// Error generated by the `OwnedBridgeModule` trait.
 		BridgeModule(bp_runtime::OwnedBridgeModuleError),
 	}
@@ -337,112 +333,113 @@ pub mod pallet {
 				parachains.len() as _,
 			);
 
-			pallet_bridge_grandpa::Pallet::<T, T::BridgesGrandpaPalletInstance>::parse_finalized_storage_proof(
+			let mut storage = GrandpaPalletOf::<T, I>::storage_proof_checker(
 				relay_block_hash,
 				parachain_heads_proof.0,
-				move |mut storage| {
-					for (parachain, parachain_head_hash) in parachains {
-						let parachain_head = match Pallet::<T, I>::read_parachain_head(&mut storage, parachain) {
-							Ok(Some(parachain_head)) => parachain_head,
-							Ok(None) => {
-								log::trace!(
-									target: LOG_TARGET,
-									"The head of parachain {:?} is None. {}",
-									parachain,
-									if ParasInfo::<T, I>::contains_key(parachain) {
-										"Looks like it is not yet registered at the source relay chain"
-									} else {
-										"Looks like it has been deregistered from the source relay chain"
-									},
-								);
-								Self::deposit_event(Event::MissingParachainHead { parachain });
-								continue;
-							},
-							Err(e) => {
-								log::trace!(
-									target: LOG_TARGET,
-									"The read of head of parachain {:?} has failed: {:?}",
-									parachain,
-									e,
-								);
-								Self::deposit_event(Event::MissingParachainHead { parachain });
-								continue;
-							},
-						};
+			)
+			.map_err(Error::<T, I>::HeaderChainStorageProof)?;
 
-						// if relayer has specified invalid parachain head hash, ignore the head
-						// (this isn't strictly necessary, but better safe than sorry)
-						let actual_parachain_head_hash = parachain_head.hash();
-						if parachain_head_hash != actual_parachain_head_hash {
-							log::trace!(
-								target: LOG_TARGET,
-								"The submitter has specified invalid parachain {:?} head hash: {:?} vs {:?}",
-								parachain,
-								parachain_head_hash,
-								actual_parachain_head_hash,
-							);
-							Self::deposit_event(Event::IncorrectParachainHeadHash {
-								parachain,
-								parachain_head_hash,
-								actual_parachain_head_hash,
-							});
-							continue;
-						}
-
-						// convert from parachain head into stored parachain head data
-						let parachain_head_data = match T::ParaStoredHeaderDataBuilder::try_build(
+			for (parachain, parachain_head_hash) in parachains {
+				let parachain_head = match Self::read_parachain_head(&mut storage, parachain) {
+					Ok(Some(parachain_head)) => parachain_head,
+					Ok(None) => {
+						log::trace!(
+							target: LOG_TARGET,
+							"The head of parachain {:?} is None. {}",
 							parachain,
-							&parachain_head,
-						) {
-							Some(parachain_head_data) => parachain_head_data,
-							None => {
-								log::trace!(
+							if ParasInfo::<T, I>::contains_key(parachain) {
+								"Looks like it is not yet registered at the source relay chain"
+							} else {
+								"Looks like it has been deregistered from the source relay chain"
+							},
+						);
+						Self::deposit_event(Event::MissingParachainHead { parachain });
+						continue
+					},
+					Err(e) => {
+						log::trace!(
+							target: LOG_TARGET,
+							"The read of head of parachain {:?} has failed: {:?}",
+							parachain,
+							e,
+						);
+						Self::deposit_event(Event::MissingParachainHead { parachain });
+						continue
+					},
+				};
+
+				// if relayer has specified invalid parachain head hash, ignore the head
+				// (this isn't strictly necessary, but better safe than sorry)
+				let actual_parachain_head_hash = parachain_head.hash();
+				if parachain_head_hash != actual_parachain_head_hash {
+					log::trace!(
+						target: LOG_TARGET,
+						"The submitter has specified invalid parachain {:?} head hash: \
+								{:?} vs {:?}",
+						parachain,
+						parachain_head_hash,
+						actual_parachain_head_hash,
+					);
+					Self::deposit_event(Event::IncorrectParachainHeadHash {
+						parachain,
+						parachain_head_hash,
+						actual_parachain_head_hash,
+					});
+					continue
+				}
+
+				// convert from parachain head into stored parachain head data
+				let parachain_head_data =
+					match T::ParaStoredHeaderDataBuilder::try_build(parachain, &parachain_head) {
+						Some(parachain_head_data) => parachain_head_data,
+						None => {
+							log::trace!(
 									target: LOG_TARGET,
 									"The head of parachain {:?} has been provided, but it is not tracked by the pallet",
 									parachain,
 								);
-								Self::deposit_event(Event::UntrackedParachainRejected { parachain });
-								continue;
-							},
-						};
+							Self::deposit_event(Event::UntrackedParachainRejected { parachain });
+							continue
+						},
+					};
 
-						let update_result: Result<_, ()> = ParasInfo::<T, I>::try_mutate(parachain, |stored_best_head| {
-							let artifacts = Pallet::<T, I>::update_parachain_head(
-								parachain,
-								stored_best_head.take(),
-								relay_block_number,
-								parachain_head_data,
-								parachain_head_hash,
-							)?;
-							*stored_best_head = Some(artifacts.best_head);
-							Ok(artifacts.prune_happened)
-						});
+				let update_result: Result<_, ()> =
+					ParasInfo::<T, I>::try_mutate(parachain, |stored_best_head| {
+						let artifacts = Pallet::<T, I>::update_parachain_head(
+							parachain,
+							stored_best_head.take(),
+							relay_block_number,
+							parachain_head_data,
+							parachain_head_hash,
+						)?;
+						*stored_best_head = Some(artifacts.best_head);
+						Ok(artifacts.prune_happened)
+					});
 
-						// we're refunding weight if update has not happened and if pruning has not happened
-						let is_update_happened = matches!(update_result, Ok(_));
-						if !is_update_happened {
-							actual_weight = actual_weight
-								.saturating_sub(WeightInfoOf::<T, I>::parachain_head_storage_write_weight(T::DbWeight::get()));
-						}
-						let is_prune_happened = matches!(update_result, Ok(true));
-						if !is_prune_happened {
-							actual_weight = actual_weight
-								.saturating_sub(WeightInfoOf::<T, I>::parachain_head_pruning_weight(T::DbWeight::get()));
-						}
-					}
+				// we're refunding weight if update has not happened and if pruning has not happened
+				let is_update_happened = matches!(update_result, Ok(_));
+				if !is_update_happened {
+					actual_weight = actual_weight.saturating_sub(
+						WeightInfoOf::<T, I>::parachain_head_storage_write_weight(
+							T::DbWeight::get(),
+						),
+					);
+				}
+				let is_prune_happened = matches!(update_result, Ok(true));
+				if !is_prune_happened {
+					actual_weight = actual_weight.saturating_sub(
+						WeightInfoOf::<T, I>::parachain_head_pruning_weight(T::DbWeight::get()),
+					);
+				}
+			}
 
-					// even though we may have accepted some parachain heads, we can't allow relayers to submit
-					// proof with unused trie nodes
-					// => treat this as an error
-					//
-					// (we can throw error here, because now all our calls are transactional)
-					storage.ensure_no_unused_nodes()
-				},
-			)
-			.and_then(|r| r.map_err(HeaderChainError::StorageProof))
-			.map_err(|e| {
-				log::trace!(target: LOG_TARGET, "Parachain heads storage proof is invalid: {:?}", e);
-				Error::<T, I>::HeaderChain(e)
+			// even though we may have accepted some parachain heads, we can't allow relayers to
+			// submit proof with unused trie nodes
+			// => treat this as an error
+			//
+			// (we can throw error here, because now all our calls are transactional)
+			storage.ensure_no_unused_nodes().map_err(|e| {
+				Error::<T, I>::HeaderChainStorageProof(HeaderChainError::StorageProof(e))
 			})?;
 
 			Ok(PostDispatchInfo { actual_weight: Some(actual_weight), pays_fee: Pays::Yes })
@@ -1438,7 +1435,7 @@ pub(crate) mod tests {
 			// try to import head#5 of parachain#1 at relay chain block #0
 			assert_noop!(
 				import_parachain_1_head(0, Default::default(), parachains, proof),
-				Error::<TestRuntime>::HeaderChain(HeaderChainError::StorageProof(
+				Error::<TestRuntime>::HeaderChainStorageProof(HeaderChainError::StorageProof(
 					StorageProofError::StorageRootMismatch
 				))
 			);