From a419a942a81570eea7416f54d9d44c0b3ed3dbb9 Mon Sep 17 00:00:00 2001 From: Serban Iorga Date: Thu, 18 Aug 2022 15:33:17 +0300 Subject: [PATCH] Follow-up on #1536 (#1549) * Make RelayStrategy::final_decision() sync Signed-off-by: Serban Iorga * Move logic from RelayStrategy to RelayReference Signed-off-by: Serban Iorga * Rename RelayStrategy::final_decision() Signed-off-by: Serban Iorga --- .../src/relay_strategy/altruistic_strategy.rs | 22 +++-- .../relay_strategy/enforcement_strategy.rs | 2 +- .../src/relay_strategy/mix_strategy.rs | 6 +- .../relays/messages/src/relay_strategy/mod.rs | 90 ++++++++++++++++- .../src/relay_strategy/rational_strategy.rs | 98 ++----------------- 5 files changed, 116 insertions(+), 102 deletions(-) diff --git a/bridges/relays/messages/src/relay_strategy/altruistic_strategy.rs b/bridges/relays/messages/src/relay_strategy/altruistic_strategy.rs index b66129117873e..7e00678e1317f 100644 --- a/bridges/relays/messages/src/relay_strategy/altruistic_strategy.rs +++ b/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, ) -> 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

, TargetClient: MessageLaneTargetClient

, @@ -55,10 +62,11 @@ impl RelayStrategy for AltruisticStrategy { reference: &RelayReference, ) { 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, diff --git a/bridges/relays/messages/src/relay_strategy/enforcement_strategy.rs b/bridges/relays/messages/src/relay_strategy/enforcement_strategy.rs index 5d231462e86d9..bb4192d45ab05 100644 --- a/bridges/relays/messages/src/relay_strategy/enforcement_strategy.rs +++ b/bridges/relays/messages/src/relay_strategy/enforcement_strategy.rs @@ -214,7 +214,7 @@ impl EnforcementStrategy { 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 diff --git a/bridges/relays/messages/src/relay_strategy/mix_strategy.rs b/bridges/relays/messages/src/relay_strategy/mix_strategy.rs index d00c27196787c..331f77b012d22 100644 --- a/bridges/relays/messages/src/relay_strategy/mix_strategy.rs +++ b/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

, TargetClient: MessageLaneTargetClient

, @@ -65,8 +65,8 @@ impl RelayStrategy for MixStrategy { reference: &RelayReference, ) { 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), } } } diff --git a/bridges/relays/messages/src/relay_strategy/mod.rs b/bridges/relays/messages/src/relay_strategy/mod.rs index da615f34e8659..7bfd74c4790f8 100644 --- a/bridges/relays/messages/src/relay_strategy/mod.rs +++ b/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

, TargetClient: MessageLaneTargetClient

, @@ -66,6 +67,14 @@ pub trait RelayStrategy: 'static + Clone + Send + Sync { ); } +/// Total cost of mesage delivery and confirmation. +struct MessagesDeliveryCost { + /// 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, } +impl< + P: MessageLane, + SourceClient: MessageLaneSourceClient

, + TargetClient: MessageLaneTargetClient

, + > RelayReference +{ + /// 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, 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, diff --git a/bridges/relays/messages/src/relay_strategy/rational_strategy.rs b/bridges/relays/messages/src/relay_strategy/rational_strategy.rs index f45f01725c631..1791670fa45a8 100644 --- a/bridges/relays/messages/src/relay_strategy/rational_strategy.rs +++ b/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, ) -> 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

, TargetClient: MessageLaneTargetClient

, @@ -118,40 +73,3 @@ impl RelayStrategy for RationalStrategy { // anything here } } - -/// Total cost of mesage delivery and confirmation. -struct MessagesDeliveryCost { - /// 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

, - TargetClient: MessageLaneTargetClient

, ->( - reference: &RelayReference, -) -> Result, 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 }) -}