Fix invalid batch transaction (paritytech#1957)

* fix invalid batch transaction

* RaceState is now trait

* clippy

Author: svyatonik

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

@@ -91,18 +91,21 @@ impl<AccountId> TaggedAccount<AccountId> {
 }
 
 /// Batch call builder.
-pub trait BatchCallBuilder<Call>: Send {
+pub trait BatchCallBuilder<Call>: Clone + Send {
 	/// Create batch call from given calls vector.
 	fn build_batch_call(&self, _calls: Vec<Call>) -> Call;
 }
 
 /// Batch call builder constructor.
-pub trait BatchCallBuilderConstructor<Call> {
+pub trait BatchCallBuilderConstructor<Call>: Clone {
+	/// Call builder, used by this constructor.
+	type CallBuilder: BatchCallBuilder<Call>;
 	/// Create a new instance of a batch call builder.
-	fn new_builder() -> Option<Box<dyn BatchCallBuilder<Call>>>;
+	fn new_builder() -> Option<Self::CallBuilder>;
 }
 
 /// Batch call builder based on `pallet-utility`.
+#[derive(Clone)]
 pub struct UtilityPalletBatchCallBuilder<C: Chain>(PhantomData<C>);
 
 impl<C: Chain> BatchCallBuilder<C::Call> for UtilityPalletBatchCallBuilder<C>
@@ -118,14 +121,25 @@ impl<C: Chain> BatchCallBuilderConstructor<C::Call> for UtilityPalletBatchCallBu
 where
 	C: ChainWithUtilityPallet,
 {
-	fn new_builder() -> Option<Box<dyn BatchCallBuilder<C::Call>>> {
-		Some(Box::new(Self(Default::default())))
+	type CallBuilder = Self;
+
+	fn new_builder() -> Option<Self::CallBuilder> {
+		Some(Self(Default::default()))
 	}
 }
 
-/// A `BatchCallBuilderConstructor` that always returns `None`.
+// A `BatchCallBuilderConstructor` that always returns `None`.
 impl<Call> BatchCallBuilderConstructor<Call> for () {
-	fn new_builder() -> Option<Box<dyn BatchCallBuilder<Call>>> {
+	type CallBuilder = ();
+	fn new_builder() -> Option<Self::CallBuilder> {
 		None
 	}
 }
+
+// Dummy `BatchCallBuilder` implementation that must never be used outside
+// of the `impl BatchCallBuilderConstructor for ()` code.
+impl<Call> BatchCallBuilder<Call> for () {
+	fn build_batch_call(&self, _calls: Vec<Call>) -> Call {
+		unreachable!("never called, because ()::new_builder() returns None; qed")
+	}
+}

bridges/relays/lib-substrate-relay/src/messages_lane.rs

@@ -111,15 +111,26 @@ pub struct MessagesRelayParams<P: SubstrateMessageLane> {
 
 /// Batch transaction that brings headers + and messages delivery/receiving confirmations to the
 /// source node.
+#[derive(Clone)]
 pub struct BatchProofTransaction<SC: Chain, TC: Chain, B: BatchCallBuilderConstructor<CallOf<SC>>> {
-	builder: Box<dyn BatchCallBuilder<CallOf<SC>>>,
+	builder: B::CallBuilder,
 	proved_header: HeaderIdOf<TC>,
 	prove_calls: Vec<CallOf<SC>>,
 
 	/// Using `fn() -> B` in order to avoid implementing `Send` for `B`.
 	_phantom: PhantomData<fn() -> B>,
 }
 
+impl<SC: Chain, TC: Chain, B: BatchCallBuilderConstructor<CallOf<SC>>> std::fmt::Debug
+	for BatchProofTransaction<SC, TC, B>
+{
+	fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
+		fmt.debug_struct("BatchProofTransaction")
+			.field("proved_header", &self.proved_header)
+			.finish()
+	}
+}
+
 impl<SC: Chain, TC: Chain, B: BatchCallBuilderConstructor<CallOf<SC>>>
 	BatchProofTransaction<SC, TC, B>
 {

bridges/relays/messages/src/message_lane_loop.rs

@@ -111,7 +111,7 @@ pub struct NoncesSubmitArtifacts<T> {
 
 /// Batch transaction that already submit some headers and needs to be extended with
 /// messages/delivery proof before sending.
-pub trait BatchTransaction<HeaderId>: Send {
+pub trait BatchTransaction<HeaderId>: Debug + Send {
 	/// Header that was required in the original call and which is bundled within this
 	/// batch transaction.
 	fn required_header_id(&self) -> HeaderId;
@@ -121,7 +121,7 @@ pub trait BatchTransaction<HeaderId>: Send {
 #[async_trait]
 pub trait SourceClient<P: MessageLane>: RelayClient {
 	/// Type of batch transaction that submits finality and message receiving proof.
-	type BatchTransaction: BatchTransaction<TargetHeaderIdOf<P>>;
+	type BatchTransaction: BatchTransaction<TargetHeaderIdOf<P>> + Clone;
 	/// Transaction tracker to track submitted transactions.
 	type TransactionTracker: TransactionTracker<HeaderId = SourceHeaderIdOf<P>>;
 
@@ -186,7 +186,7 @@ pub trait SourceClient<P: MessageLane>: RelayClient {
 #[async_trait]
 pub trait TargetClient<P: MessageLane>: RelayClient {
 	/// Type of batch transaction that submits finality and messages proof.
-	type BatchTransaction: BatchTransaction<SourceHeaderIdOf<P>>;
+	type BatchTransaction: BatchTransaction<SourceHeaderIdOf<P>> + Clone;
 	/// Transaction tracker to track submitted transactions.
 	type TransactionTracker: TransactionTracker<HeaderId = TargetHeaderIdOf<P>>;
 
@@ -1212,6 +1212,9 @@ pub(crate) mod tests {
 			original_data,
 			Arc::new(|_| {}),
 			Arc::new(move |data: &mut TestClientData| {
+				data.source_state.best_self =
+					HeaderId(data.source_state.best_self.0 + 1, data.source_state.best_self.1 + 1);
+				data.source_state.best_finalized_self = data.source_state.best_self;
 				if let Some(target_to_source_header_required) =
 					data.target_to_source_header_required.take()
 				{
@@ -1223,6 +1226,10 @@ pub(crate) mod tests {
 			}),
 			Arc::new(|_| {}),
 			Arc::new(move |data: &mut TestClientData| {
+				data.target_state.best_self =
+					HeaderId(data.target_state.best_self.0 + 1, data.target_state.best_self.1 + 1);
+				data.target_state.best_finalized_self = data.target_state.best_self;
+
 				if let Some(source_to_target_header_required) =
 					data.source_to_target_header_required.take()
 				{

bridges/relays/messages/src/message_race_delivery.rs

@@ -322,13 +322,19 @@ where
 		self.strategy.is_empty()
 	}
 
-	fn required_source_header_at_target(
+	fn required_source_header_at_target<RS: RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>>>(
 		&self,
 		current_best: &SourceHeaderIdOf<P>,
+		race_state: RS,
 	) -> Option<SourceHeaderIdOf<P>> {
+		// we have already submitted something - let's wait until it is mined
+		if race_state.nonces_submitted().is_some() {
+			return None
+		}
+
 		let has_nonces_to_deliver = !self.strategy.is_empty();
 		let header_required_for_messages_delivery =
-			self.strategy.required_source_header_at_target(current_best);
+			self.strategy.required_source_header_at_target(current_best, race_state);
 		let header_required_for_reward_confirmations_delivery = self
 			.latest_confirmed_nonces_at_source
 			.back()
@@ -381,10 +387,10 @@ where
 		self.strategy.source_nonces_updated(at_block, nonces)
 	}
 
-	fn best_target_nonces_updated(
+	fn best_target_nonces_updated<RS: RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>>>(
 		&mut self,
 		nonces: TargetClientNonces<DeliveryRaceTargetNoncesData>,
-		race_state: &mut RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>, P::MessagesProof>,
+		race_state: &mut RS,
 	) {
 		// best target nonces must always be ge than finalized target nonces
 		let latest_nonce = nonces.latest_nonce;
@@ -396,13 +402,13 @@ where
 		)
 	}
 
-	fn finalized_target_nonces_updated(
+	fn finalized_target_nonces_updated<RS: RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>>>(
 		&mut self,
 		nonces: TargetClientNonces<DeliveryRaceTargetNoncesData>,
-		race_state: &mut RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>, P::MessagesProof>,
+		race_state: &mut RS,
 	) {
 		if let Some(ref best_finalized_source_header_id_at_best_target) =
-			race_state.best_finalized_source_header_id_at_best_target
+			race_state.best_finalized_source_header_id_at_best_target()
 		{
 			let oldest_header_number_to_keep = best_finalized_source_header_id_at_best_target.0;
 			while self
@@ -426,13 +432,13 @@ where
 		)
 	}
 
-	async fn select_nonces_to_deliver(
+	async fn select_nonces_to_deliver<RS: RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>>>(
 		&self,
-		race_state: RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>, P::MessagesProof>,
+		race_state: RS,
 	) -> Option<(RangeInclusive<MessageNonce>, Self::ProofParameters)> {
 		let best_target_nonce = self.strategy.best_at_target()?;
 		let best_finalized_source_header_id_at_best_target =
-			race_state.best_finalized_source_header_id_at_best_target.clone()?;
+			race_state.best_finalized_source_header_id_at_best_target()?;
 		let latest_confirmed_nonce_at_source = self
 			.latest_confirmed_nonces_at_source
 			.iter()
@@ -576,20 +582,29 @@ impl<SourceChainBalance: std::fmt::Debug> NoncesRange for MessageDetailsMap<Sour
 
 #[cfg(test)]
 mod tests {
-	use crate::message_lane_loop::{
-		tests::{
-			header_id, TestMessageLane, TestMessagesProof, TestSourceChainBalance,
-			TestSourceClient, TestSourceHeaderId, TestTargetClient, TestTargetHeaderId,
+	use crate::{
+		message_lane_loop::{
+			tests::{
+				header_id, TestMessageLane, TestMessagesBatchTransaction, TestMessagesProof,
+				TestSourceChainBalance, TestSourceClient, TestSourceHeaderId, TestTargetClient,
+				TestTargetHeaderId,
+			},
+			MessageDetails,
 		},
-		MessageDetails,
+		message_race_loop::RaceStateImpl,
 	};
 
 	use super::*;
 
 	const DEFAULT_DISPATCH_WEIGHT: Weight = Weight::from_parts(1, 0);
 	const DEFAULT_SIZE: u32 = 1;
 
-	type TestRaceState = RaceState<TestSourceHeaderId, TestTargetHeaderId, TestMessagesProof>;
+	type TestRaceState = RaceStateImpl<
+		TestSourceHeaderId,
+		TestTargetHeaderId,
+		TestMessagesProof,
+		TestMessagesBatchTransaction,
+	>;
 	type TestStrategy =
 		MessageDeliveryStrategy<TestMessageLane, TestSourceClient, TestTargetClient>;
 
@@ -617,12 +632,13 @@ mod tests {
 	}
 
 	fn prepare_strategy() -> (TestRaceState, TestStrategy) {
-		let mut race_state = RaceState {
+		let mut race_state = RaceStateImpl {
 			best_finalized_source_header_id_at_source: Some(header_id(1)),
 			best_finalized_source_header_id_at_best_target: Some(header_id(1)),
 			best_target_header_id: Some(header_id(1)),
 			best_finalized_target_header_id: Some(header_id(1)),
 			nonces_to_submit: None,
+			nonces_to_submit_batch: None,
 			nonces_submitted: None,
 		};
 
@@ -964,14 +980,17 @@ mod tests {
 		);
 		// nothing needs to be delivered now and we don't need any new headers
 		assert_eq!(strategy.select_nonces_to_deliver(state.clone()).await, None);
-		assert_eq!(strategy.required_source_header_at_target(&header_id(1)), None);
+		assert_eq!(strategy.required_source_header_at_target(&header_id(1), state.clone()), None);
 
 		// now let's generate two more nonces [24; 25] at the soruce;
 		strategy.source_nonces_updated(header_id(2), source_nonces(24..=25, 19, 0));
 		//
 		// - so now we'll need to relay source block#2 to be able to accept messages [24; 25].
 		assert_eq!(strategy.select_nonces_to_deliver(state.clone()).await, None);
-		assert_eq!(strategy.required_source_header_at_target(&header_id(1)), Some(header_id(2)));
+		assert_eq!(
+			strategy.required_source_header_at_target(&header_id(1), state.clone()),
+			Some(header_id(2))
+		);
 
 		// let's relay source block#2
 		state.best_finalized_source_header_id_at_source = Some(header_id(2));
@@ -982,18 +1001,18 @@ mod tests {
 		// and ask strategy again => still nothing to deliver, because parallel confirmations
 		// race need to be pushed further
 		assert_eq!(strategy.select_nonces_to_deliver(state.clone()).await, None);
-		assert_eq!(strategy.required_source_header_at_target(&header_id(2)), None);
+		assert_eq!(strategy.required_source_header_at_target(&header_id(2), state.clone()), None);
 
 		// let's confirm messages [20; 23]
 		strategy.source_nonces_updated(header_id(2), source_nonces(24..=25, 23, 0));
 
 		// and ask strategy again => now we have everything required to deliver remaining
 		// [24; 25] nonces and proof of [20; 23] confirmation
 		assert_eq!(
-			strategy.select_nonces_to_deliver(state).await,
+			strategy.select_nonces_to_deliver(state.clone()).await,
 			Some(((24..=25), proof_parameters(true, 2))),
 		);
-		assert_eq!(strategy.required_source_header_at_target(&header_id(2)), None);
+		assert_eq!(strategy.required_source_header_at_target(&header_id(2), state), None);
 	}
 
 	#[async_std::test]
@@ -1025,6 +1044,7 @@ mod tests {
 	#[test]
 	#[allow(clippy::reversed_empty_ranges)]
 	fn no_source_headers_required_at_target_if_lanes_are_empty() {
+		let (state, _) = prepare_strategy();
 		let mut strategy = TestStrategy {
 			max_unrewarded_relayer_entries_at_target: 4,
 			max_unconfirmed_nonces_at_target: 4,
@@ -1053,7 +1073,7 @@ mod tests {
 			strategy.latest_confirmed_nonces_at_source,
 			VecDeque::from([(source_header_id, 0)])
 		);
-		assert_eq!(strategy.required_source_header_at_target(&source_header_id), None);
+		assert_eq!(strategy.required_source_header_at_target(&source_header_id, state), None);
 	}
 
 	#[async_std::test]