Follow-up on paritytech#1536 (paritytech#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
- // => let rational strategy fill required fields
- let _ = RationalStrategy.decide(reference).await;
+ // We don't care about costs and rewards, but we want to report unprofitable transactions.
+ if let Err(e) = reference.update_cost_and_reward().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::Rational => RationalStrategy.final_decision(reference).await,
+ RelayerMode::Altruistic => AltruisticStrategy.on_final_decision(reference),
+ 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 {
- 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 {
+ if let Err(e) = reference.update_cost_and_reward().await {
  log::debug!(
  target: "bridge",
- "Message with nonce {} (reward = {:?}) changes total cost {:?}->{:?} and makes it larger than \
- total reward {:?}->{:?}",
- 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,
+ "Failed to update transaction cost and reward: {:?}. No nonces selected for delivery",
+ e,
  );
+
+ return false
  }
 
- // Rational relayer never want to lose his funds
- if reference.total_reward >= reference.total_cost {
+ // Rational relayer never wants to lose his funds.
+ 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 })
-}