Follow-up on #1536 (#1549)

* Make RelayStrategy::final_decision() sync

Signed-off-by: Serban Iorga <serban@parity.io>

* Move logic from RelayStrategy to RelayReference

Signed-off-by: Serban Iorga <serban@parity.io>

* Rename RelayStrategy::final_decision()

Signed-off-by: Serban Iorga <serban@parity.io>

bridges/relays/messages/src/relay_strategy/altruistic_strategy.rs

@@ -23,7 +23,7 @@ use crate::{
 	message_lane_loop::{
 		SourceClient as MessageLaneSourceClient, TargetClient as MessageLaneTargetClient,
 	},
-	relay_strategy::{RationalStrategy, RelayReference, RelayStrategy},
+	relay_strategy::{RelayReference, RelayStrategy},
 };
 
 /// The relayer doesn't care about rewards.
@@ -40,13 +40,20 @@ impl RelayStrategy for AltruisticStrategy {
 		&mut self,
 		reference: &mut RelayReference<P, SourceClient, TargetClient>,
 	) -> bool {
-		// we don't care about costs and rewards, but we want to report unprofitable transactions
+		// We don't care about costs and rewards, but we want to report unprofitable transactions.
-		// => let rational strategy fill required fields
+		if let Err(e) = reference.update_cost_and_reward().await {
-		let _ = RationalStrategy.decide(reference).await;
+			log::debug!(
+				target: "bridge",
+				"Failed to update transaction cost and reward: {:?}. \
+				The `unprofitable_delivery_transactions` metric will be inaccurate",
+				e,
+			);
+		}
+
 		true
 	}
 
-	async fn final_decision<
+	fn on_final_decision<
 		P: MessageLane,
 		SourceClient: MessageLaneSourceClient<P>,
 		TargetClient: MessageLaneTargetClient<P>,
@@ -55,10 +62,11 @@ impl RelayStrategy for AltruisticStrategy {
 		reference: &RelayReference<P, SourceClient, TargetClient>,
 	) {
 		if let Some(ref metrics) = reference.metrics {
-			if reference.total_cost > reference.total_reward {
+			if !reference.is_profitable() {
 				log::debug!(
 					target: "bridge",
-					"The relayer has submitted unprofitable {} -> {} message delivery trabsaction with {} messages: total cost = {:?}, total reward = {:?}",
+					"The relayer has submitted unprofitable {} -> {} message delivery transaction \
+					with {} messages: total cost = {:?}, total reward = {:?}",
 					P::SOURCE_NAME,
 					P::TARGET_NAME,
 					reference.index + 1,

bridges/relays/messages/src/relay_strategy/enforcement_strategy.rs

@@ -214,7 +214,7 @@ impl<Strategy: RelayStrategy> EnforcementStrategy<Strategy> {
 
 			let selected_max_nonce =
 				hard_selected_begin_nonce + hard_selected_count as MessageNonce - 1;
-			self.strategy.final_decision(&relay_reference).await;
+			self.strategy.on_final_decision(&relay_reference);
 			Some(selected_max_nonce)
 		} else {
 			None

bridges/relays/messages/src/relay_strategy/mix_strategy.rs

@@ -56,7 +56,7 @@ impl RelayStrategy for MixStrategy {
 		}
 	}
 
-	async fn final_decision<
+	fn on_final_decision<
 		P: MessageLane,
 		SourceClient: MessageLaneSourceClient<P>,
 		TargetClient: MessageLaneTargetClient<P>,
@@ -65,8 +65,8 @@ impl RelayStrategy for MixStrategy {
 		reference: &RelayReference<P, SourceClient, TargetClient>,
 	) {
 		match self.relayer_mode {
-			RelayerMode::Altruistic => AltruisticStrategy.final_decision(reference).await,
+			RelayerMode::Altruistic => AltruisticStrategy.on_final_decision(reference),
-			RelayerMode::Rational => RationalStrategy.final_decision(reference).await,
+			RelayerMode::Rational => RationalStrategy.on_final_decision(reference),
 		}
 	}
 }

bridges/relays/messages/src/relay_strategy/mod.rs

@@ -18,6 +18,7 @@
 
 use async_trait::async_trait;
 use bp_messages::{MessageNonce, Weight};
+use sp_arithmetic::traits::Saturating;
 use std::ops::Range;
 
 use crate::{
@@ -56,7 +57,7 @@ pub trait RelayStrategy: 'static + Clone + Send + Sync {
 	) -> bool;
 
 	/// Notification that the following maximal nonce has been selected for the delivery.
-	async fn final_decision<
+	fn on_final_decision<
 		P: MessageLane,
 		SourceClient: MessageLaneSourceClient<P>,
 		TargetClient: MessageLaneTargetClient<P>,
@@ -66,6 +67,14 @@ pub trait RelayStrategy: 'static + Clone + Send + Sync {
 	);
 }
 
+/// Total cost of mesage delivery and confirmation.
+struct MessagesDeliveryCost<SourceChainBalance> {
+	/// Cost of message delivery transaction.
+	pub delivery_transaction_cost: SourceChainBalance,
+	/// Cost of confirmation delivery transaction.
+	pub confirmation_transaction_cost: SourceChainBalance,
+}
+
 /// Reference data for participating in relay
 pub struct RelayReference<
 	P: MessageLane,
@@ -107,6 +116,85 @@ pub struct RelayReference<
 	pub details: MessageDetails<P::SourceChainBalance>,
 }
 
+impl<
+		P: MessageLane,
+		SourceClient: MessageLaneSourceClient<P>,
+		TargetClient: MessageLaneTargetClient<P>,
+	> RelayReference<P, SourceClient, TargetClient>
+{
+	/// Returns whether the current `RelayReference` is profitable.
+	pub fn is_profitable(&self) -> bool {
+		self.total_reward >= self.total_cost
+	}
+
+	async fn estimate_messages_delivery_cost(
+		&self,
+	) -> Result<MessagesDeliveryCost<P::SourceChainBalance>, TargetClient::Error> {
+		// technically, multiple confirmations will be delivered in a single transaction,
+		// meaning less loses for relayer. But here we don't know the final relayer yet, so
+		// we're adding a separate transaction for every message. Normally, this cost is covered
+		// by the message sender. Probably reconsider this?
+		let confirmation_transaction_cost =
+			self.lane_source_client.estimate_confirmation_transaction().await;
+
+		let delivery_transaction_cost = self
+			.lane_target_client
+			.estimate_delivery_transaction_in_source_tokens(
+				self.hard_selected_begin_nonce..=
+					(self.hard_selected_begin_nonce + self.index as MessageNonce),
+				self.selected_prepaid_nonces,
+				self.selected_unpaid_weight,
+				self.selected_size as u32,
+			)
+			.await?;
+
+		Ok(MessagesDeliveryCost { confirmation_transaction_cost, delivery_transaction_cost })
+	}
+
+	async fn update_cost_and_reward(&mut self) -> Result<(), TargetClient::Error> {
+		let prev_is_profitable = self.is_profitable();
+		let prev_total_cost = self.total_cost;
+		let prev_total_reward = self.total_reward;
+
+		let MessagesDeliveryCost { confirmation_transaction_cost, delivery_transaction_cost } =
+			self.estimate_messages_delivery_cost().await?;
+		self.total_confirmations_cost =
+			self.total_confirmations_cost.saturating_add(confirmation_transaction_cost);
+		self.total_reward = self.total_reward.saturating_add(self.details.reward);
+		self.total_cost = self.total_confirmations_cost.saturating_add(delivery_transaction_cost);
+
+		if prev_is_profitable && !self.is_profitable() {
+			// if it is the first message that makes reward less than cost, let's log it
+			log::debug!(
+				target: "bridge",
+				"Message with nonce {} (reward = {:?}) changes total cost {:?}->{:?} and makes it larger than \
+				total reward {:?}->{:?}",
+				self.nonce,
+				self.details.reward,
+				prev_total_cost,
+				self.total_cost,
+				prev_total_reward,
+				self.total_reward,
+			);
+		} else if !prev_is_profitable && self.is_profitable() {
+			// if this message makes batch profitable again, let's log it
+			log::debug!(
+				target: "bridge",
+				"Message with nonce {} (reward = {:?}) changes total cost {:?}->{:?} and makes it less than or \
+				equal to the total reward {:?}->{:?} (again)",
+				self.nonce,
+				self.details.reward,
+				prev_total_cost,
+				self.total_cost,
+				prev_total_reward,
+				self.total_reward,
+			);
+		}
+
+		Ok(())
+	}
+}
+
 /// Relay reference data
 pub struct RelayMessagesBatchReference<
 	P: MessageLane,

bridges/relays/messages/src/relay_strategy/rational_strategy.rs

@@ -17,9 +17,6 @@
 //! Rational relay strategy
 
 use async_trait::async_trait;
-use num_traits::SaturatingAdd;
-
-use bp_messages::MessageNonce;
 
 use crate::{
 	message_lane::MessageLane,
@@ -44,60 +41,18 @@ impl RelayStrategy for RationalStrategy {
 		&mut self,
 		reference: &mut RelayReference<P, SourceClient, TargetClient>,
 	) -> bool {
-		let total_cost = match estimate_messages_delivery_cost(reference).await {
+		if let Err(e) = reference.update_cost_and_reward().await {
-			Ok(total_cost) => total_cost,
-			Err(err) => {
-				log::debug!(
-					target: "bridge",
-					"Failed to estimate delivery transaction cost: {:?}. No nonces selected for delivery",
-					err,
-				);
-
-				return false
-			},
-		};
-
-		// if it is the first message that makes reward less than cost, let's log it
-		// if this message makes batch profitable again, let's log it
-		let MessagesDeliveryCost { confirmation_transaction_cost, delivery_transaction_cost } =
-			total_cost;
-		let is_total_reward_less_than_cost = reference.total_reward < reference.total_cost;
-		let prev_total_cost = reference.total_cost;
-		let prev_total_reward = reference.total_reward;
-		reference.total_confirmations_cost = reference
-			.total_confirmations_cost
-			.saturating_add(&confirmation_transaction_cost);
-		reference.total_reward = reference.total_reward.saturating_add(&reference.details.reward);
-		reference.total_cost =
-			reference.total_confirmations_cost.saturating_add(&delivery_transaction_cost);
-		if !is_total_reward_less_than_cost && reference.total_reward < reference.total_cost {
 			log::debug!(
 				target: "bridge",
-				"Message with nonce {} (reward = {:?}) changes total cost {:?}->{:?} and makes it larger than \
+				"Failed to update transaction cost and reward: {:?}. No nonces selected for delivery",
-				total reward {:?}->{:?}",
+				e,
-				reference.nonce,
-				reference.details.reward,
-				prev_total_cost,
-				reference.total_cost,
-				prev_total_reward,
-				reference.total_reward,
-			);
-		} else if is_total_reward_less_than_cost && reference.total_reward >= reference.total_cost {
-			log::debug!(
-				target: "bridge",
-				"Message with nonce {} (reward = {:?}) changes total cost {:?}->{:?} and makes it less than or \
-				equal to the total reward {:?}->{:?} (again)",
-				reference.nonce,
-				reference.details.reward,
-				prev_total_cost,
-				reference.total_cost,
-				prev_total_reward,
-				reference.total_reward,
 			);
+
+			return false
 		}
 
-		// Rational relayer never want to lose his funds
+		// Rational relayer never wants to lose his funds.
-		if reference.total_reward >= reference.total_cost {
+		if reference.is_profitable() {
 			reference.selected_reward = reference.total_reward;
 			reference.selected_cost = reference.total_cost;
 			return true
@@ -106,7 +61,7 @@ impl RelayStrategy for RationalStrategy {
 		false
 	}
 
-	async fn final_decision<
+	fn on_final_decision<
 		P: MessageLane,
 		SourceClient: MessageLaneSourceClient<P>,
 		TargetClient: MessageLaneTargetClient<P>,
@@ -118,40 +73,3 @@ impl RelayStrategy for RationalStrategy {
 		// anything here
 	}
 }
-
-/// Total cost of mesage delivery and confirmation.
-struct MessagesDeliveryCost<SourceChainBalance> {
-	/// Cost of message delivery transaction.
-	pub delivery_transaction_cost: SourceChainBalance,
-	/// Cost of confirmation delivery transaction.
-	pub confirmation_transaction_cost: SourceChainBalance,
-}
-
-/// Returns cost of message delivery and confirmation delivery transactions
-async fn estimate_messages_delivery_cost<
-	P: MessageLane,
-	SourceClient: MessageLaneSourceClient<P>,
-	TargetClient: MessageLaneTargetClient<P>,
->(
-	reference: &RelayReference<P, SourceClient, TargetClient>,
-) -> Result<MessagesDeliveryCost<P::SourceChainBalance>, TargetClient::Error> {
-	// technically, multiple confirmations will be delivered in a single transaction,
-	// meaning less loses for relayer. But here we don't know the final relayer yet, so
-	// we're adding a separate transaction for every message. Normally, this cost is covered
-	// by the message sender. Probably reconsider this?
-	let confirmation_transaction_cost =
-		reference.lane_source_client.estimate_confirmation_transaction().await;
-
-	let delivery_transaction_cost = reference
-		.lane_target_client
-		.estimate_delivery_transaction_in_source_tokens(
-			reference.hard_selected_begin_nonce..=
-				(reference.hard_selected_begin_nonce + reference.index as MessageNonce),
-			reference.selected_prepaid_nonces,
-			reference.selected_unpaid_weight,
-			reference.selected_size as u32,
-		)
-		.await?;
-
-	Ok(MessagesDeliveryCost { confirmation_transaction_cost, delivery_transaction_cost })
-}