Fix delivery transaction estimation used by rational relayer (#1109)

* fix delivery transaction estimation in greedy relayer

* fixed typo

* improve logging

* improve logging

* fmt

* fix compilation

* fmt

* Update relays/lib-substrate-relay/src/messages_target.rs

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* review

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

bridges/relays/lib-substrate-relay/Cargo.toml

@@ -40,8 +40,9 @@ sp-finality-grandpa = { git = "https://github.com/paritytech/substrate", branch
 sp-runtime = { git = "https://github.com/paritytech/substrate", branch = "master" }
 
 [dev-dependencies]
-relay-millau-client = { path = "../client-millau" }
-relay-rialto-client = { path = "../client-rialto" }
-bp-rialto = { path = "../../primitives/chain-rialto" }
 bp-millau = { path = "../../primitives/chain-millau" }
+bp-rococo = { path = "../../primitives/chain-rococo" }
+bp-wococo = { path = "../../primitives/chain-wococo" }
+relay-rococo-client = { path = "../client-rococo" }
+relay-wococo-client = { path = "../client-wococo" }
 rialto-runtime = { path = "../../bin/rialto/runtime" }

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

@@ -88,6 +88,13 @@ pub trait SubstrateMessageLane: 'static + Clone + Send + Sync {
 	/// Name of the messages pallet as it is declared in the `construct_runtime!()` at target chain.
 	const MESSAGE_PALLET_NAME_AT_TARGET: &'static str;
 
+	/// Extra weight of the delivery transaction at the target chain, that is paid to cover
+	/// dispatch fee payment.
+	///
+	/// If dispatch fee is paid at the source chain, then this weight is refunded by the
+	/// delivery transaction.
+	const PAY_INBOUND_DISPATCH_FEE_WEIGHT_AT_TARGET_CHAIN: Weight;
+
 	/// Source chain.
 	type SourceChain: Chain;
 	/// Target chain.

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

@@ -24,7 +24,6 @@ use crate::on_demand_headers::OnDemandHeadersRelay;
 
 use async_trait::async_trait;
 use bp_messages::{LaneId, MessageNonce, UnrewardedRelayersState};
-use bp_runtime::messages::DispatchFeePayment;
 use bridge_runtime_common::messages::{
 	source::FromBridgedChainMessagesDeliveryProof, target::FromBridgedChainMessagesProof,
 };
@@ -43,7 +42,10 @@ use relay_substrate_client::{
 };
 use relay_utils::{relay_loop::Client as RelayClient, BlockNumberBase, HeaderId};
 use sp_core::Bytes;
-use sp_runtime::{traits::Header as HeaderT, DeserializeOwned};
+use sp_runtime::{
+	traits::{AtLeast32BitUnsigned, Header as HeaderT},
+	DeserializeOwned,
+};
 use std::ops::RangeInclusive;
 
 /// Intermediate message proof returned by the source Substrate node. Includes everything
@@ -121,6 +123,7 @@ where
 	>,
 	<P::MessageLane as MessageLane>::TargetHeaderNumber: Decode,
 	<P::MessageLane as MessageLane>::TargetHeaderHash: Decode,
+	<P::MessageLane as MessageLane>::SourceChainBalance: AtLeast32BitUnsigned,
 {
 	async fn state(&self) -> Result<SourceClientState<P::MessageLane>, SubstrateError> {
 		// we can't continue to deliver confirmations if source node is out of sync, because
@@ -264,6 +267,7 @@ where
 				prepare_dummy_messages_delivery_proof::<P::SourceChain, P::TargetChain>(),
 			))
 			.await
+			.map(|fee| fee.inclusion_fee())
 			.unwrap_or_else(|_| BalanceOf::<P::SourceChain>::max_value())
 	}
 }
@@ -397,7 +401,7 @@ fn make_message_details_map<C: Chain>(
 				dispatch_weight: details.dispatch_weight,
 				size: details.size as _,
 				reward: details.delivery_and_dispatch_fee,
-				dispatch_fee_payment: DispatchFeePayment::AtSourceChain,
+				dispatch_fee_payment: details.dispatch_fee_payment,
 			},
 		);
 		expected_nonce = details.nonce + 1;
@@ -411,12 +415,12 @@ fn make_message_details_map<C: Chain>(
 mod tests {
 	use super::*;
 	use bp_runtime::messages::DispatchFeePayment;
-	use relay_millau_client::Millau;
-	use relay_rialto_client::Rialto;
+	use relay_rococo_client::Rococo;
+	use relay_wococo_client::Wococo;
 
 	fn message_details_from_rpc(
 		nonces: RangeInclusive<MessageNonce>,
-	) -> Vec<bp_messages::MessageDetails<bp_rialto::Balance>> {
+	) -> Vec<bp_messages::MessageDetails<bp_wococo::Balance>> {
 		nonces
 			.into_iter()
 			.map(|nonce| bp_messages::MessageDetails {
@@ -432,7 +436,7 @@ mod tests {
 	#[test]
 	fn make_message_details_map_succeeds_if_no_messages_are_missing() {
 		assert_eq!(
-			make_message_details_map::<relay_rialto_client::Rialto>(message_details_from_rpc(1..=3), 1..=3,).unwrap(),
+			make_message_details_map::<Wococo>(message_details_from_rpc(1..=3), 1..=3,).unwrap(),
 			vec![
 				(
 					1,
@@ -470,7 +474,7 @@ mod tests {
 	#[test]
 	fn make_message_details_map_succeeds_if_head_messages_are_missing() {
 		assert_eq!(
-			make_message_details_map::<relay_rialto_client::Rialto>(message_details_from_rpc(2..=3), 1..=3,).unwrap(),
+			make_message_details_map::<Wococo>(message_details_from_rpc(2..=3), 1..=3,).unwrap(),
 			vec![
 				(
 					2,
@@ -501,7 +505,7 @@ mod tests {
 		let mut message_details_from_rpc = message_details_from_rpc(1..=3);
 		message_details_from_rpc.remove(1);
 		assert!(matches!(
-			make_message_details_map::<relay_rialto_client::Rialto>(message_details_from_rpc, 1..=3,),
+			make_message_details_map::<Wococo>(message_details_from_rpc, 1..=3,),
 			Err(SubstrateError::Custom(_))
 		));
 	}
@@ -509,7 +513,7 @@ mod tests {
 	#[test]
 	fn make_message_details_map_fails_if_tail_messages_are_missing() {
 		assert!(matches!(
-			make_message_details_map::<relay_rialto_client::Rialto>(message_details_from_rpc(1..=2), 1..=3,),
+			make_message_details_map::<Wococo>(message_details_from_rpc(1..=2), 1..=3,),
 			Err(SubstrateError::Custom(_))
 		));
 	}
@@ -517,15 +521,15 @@ mod tests {
 	#[test]
 	fn make_message_details_map_fails_if_all_messages_are_missing() {
 		assert!(matches!(
-			make_message_details_map::<relay_rialto_client::Rialto>(vec![], 1..=3),
+			make_message_details_map::<Wococo>(vec![], 1..=3),
 			Err(SubstrateError::Custom(_))
 		));
 	}
 
 	#[test]
 	fn prepare_dummy_messages_delivery_proof_works() {
-		let expected_minimal_size = Rialto::MAXIMAL_ENCODED_ACCOUNT_ID_SIZE + Millau::STORAGE_PROOF_OVERHEAD;
-		let dummy_proof = prepare_dummy_messages_delivery_proof::<Rialto, Millau>();
+		let expected_minimal_size = Wococo::MAXIMAL_ENCODED_ACCOUNT_ID_SIZE + Rococo::STORAGE_PROOF_OVERHEAD;
+		let dummy_proof = prepare_dummy_messages_delivery_proof::<Wococo, Rococo>();
 		assert!(
 			dummy_proof.1.encode().len() as u32 > expected_minimal_size,
 			"Expected proof size at least {}. Got: {}",

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

@@ -29,7 +29,7 @@ use bridge_runtime_common::messages::{
 	source::FromBridgedChainMessagesDeliveryProof, target::FromBridgedChainMessagesProof,
 };
 use codec::{Decode, Encode};
-use frame_support::weights::Weight;
+use frame_support::weights::{Weight, WeightToFeePolynomial};
 use messages_relay::message_lane::MessageLane;
 use messages_relay::{
 	message_lane::{SourceHeaderIdOf, TargetHeaderIdOf},
@@ -37,11 +37,11 @@ use messages_relay::{
 };
 use num_traits::{Bounded, Zero};
 use relay_substrate_client::{
-	BalanceOf, BlockNumberOf, Chain, Client, Error as SubstrateError, HashOf, HeaderOf, IndexOf,
+	BalanceOf, BlockNumberOf, Chain, Client, Error as SubstrateError, HashOf, HeaderOf, IndexOf, WeightToFeeOf,
 };
 use relay_utils::{relay_loop::Client as RelayClient, BlockNumberBase, HeaderId};
 use sp_core::Bytes;
-use sp_runtime::{DeserializeOwned, FixedPointNumber, FixedU128};
+use sp_runtime::{traits::Saturating, DeserializeOwned, FixedPointNumber, FixedU128};
 use std::{convert::TryFrom, ops::RangeInclusive};
 
 /// Message receiving proof returned by the target Substrate node.
@@ -118,7 +118,6 @@ where
 	BlockNumberOf<P::TargetChain>: Copy,
 	HeaderOf<P::TargetChain>: DeserializeOwned,
 	BlockNumberOf<P::TargetChain>: BlockNumberBase,
-
 	P::MessageLane: MessageLane<
 		MessagesProof = SubstrateMessagesProof<P::SourceChain>,
 		MessagesReceivingProof = SubstrateMessagesReceivingProof<P::TargetChain>,
@@ -244,6 +243,7 @@ where
 	async fn estimate_delivery_transaction_in_source_tokens(
 		&self,
 		nonces: RangeInclusive<MessageNonce>,
+		total_prepaid_nonces: MessageNonce,
 		total_dispatch_weight: Weight,
 		total_size: u32,
 	) -> Result<<P::MessageLane as MessageLane>::SourceChainBalance, SubstrateError> {
@@ -258,27 +258,88 @@ where
 					P::SourceChain::NAME,
 				))
 			})?;
+
+		// Prepare 'dummy' delivery transaction - we only care about its length and dispatch weight.
+		let delivery_tx = self.lane.make_messages_delivery_transaction(
+			Zero::zero(),
+			HeaderId(Default::default(), Default::default()),
+			nonces.clone(),
+			prepare_dummy_messages_proof::<P::SourceChain>(nonces.clone(), total_dispatch_weight, total_size),
+		);
+		let delivery_tx_fee = self.client.estimate_extrinsic_fee(delivery_tx).await?;
+		let inclusion_fee_in_target_tokens = delivery_tx_fee.inclusion_fee();
+
+		// The pre-dispatch cost of delivery transaction includes additional fee to cover dispatch fee payment
+		// (Currency::transfer in regular deployment). But if message dispatch has already been paid
+		// at the Source chain, the delivery transaction will refund relayer with this additional cost.
+		// But `estimate_extrinsic_fee` obviously just returns pre-dispatch cost of the transaction. So
+		// if transaction delivers prepaid message, then it may happen that pre-dispatch cost is larger
+		// than reward and `Rational` relayer will refuse to deliver this message.
+		//
+		// The most obvious solution would be to deduct total weight of dispatch fee payments from the
+		// `total_dispatch_weight` and use regular `estimate_extrinsic_fee` call. But what if
+		// `total_dispatch_weight` is less than total dispatch fee payments weight? Weight is strictly
+		// positive, so we can't use this option.
+		//
+		// Instead we'll be directly using `WeightToFee` and `NextFeeMultiplier` of the Target chain.
+		// This requires more knowledge of the Target chain, but seems there's no better way to solve
+		// this now.
+		let expected_refund_in_target_tokens = if total_prepaid_nonces != 0 {
+			const WEIGHT_DIFFERENCE: Weight = 100;
+
+			let larger_dispatch_weight = total_dispatch_weight.saturating_add(WEIGHT_DIFFERENCE);
+			let larger_delivery_tx_fee = self
+				.client
+				.estimate_extrinsic_fee(self.lane.make_messages_delivery_transaction(
+					Zero::zero(),
+					HeaderId(Default::default(), Default::default()),
+					nonces.clone(),
+					prepare_dummy_messages_proof::<P::SourceChain>(nonces.clone(), larger_dispatch_weight, total_size),
+				))
+				.await?;
+
+			compute_prepaid_messages_refund::<P>(
+				total_prepaid_nonces,
+				compute_fee_multiplier::<P::TargetChain>(
+					delivery_tx_fee.adjusted_weight_fee,
+					total_dispatch_weight,
+					larger_delivery_tx_fee.adjusted_weight_fee,
+					larger_dispatch_weight,
+				),
+			)
+		} else {
+			Zero::zero()
+		};
+
+		let delivery_fee_in_source_tokens = convert_target_tokens_to_source_tokens::<P::SourceChain, P::TargetChain>(
+			FixedU128::from_float(conversion_rate),
+			inclusion_fee_in_target_tokens.saturating_sub(expected_refund_in_target_tokens),
+		);
+
 		log::trace!(
 			target: "bridge",
-			"Using conversion rate {} when converting from {} tokens to {} tokens",
-			conversion_rate,
-			P::TargetChain::NAME,
-			P::SourceChain::NAME,
+			"Estimated {} -> {} messages delivery transaction.\n\t\
+				Total nonces: {:?}\n\t\
+				Prepaid messages: {}\n\t\
+				Total messages size: {}\n\t\
+				Total messages dispatch weight: {}\n\t\
+				Inclusion fee (in {1} tokens): {:?}\n\t\
+				Expected refund (in {1} tokens): {:?}\n\t\
+				{1} -> {0} conversion rate: {:?}\n\t\
+				Expected delivery tx fee (in {0} tokens): {:?}",
+				P::SourceChain::NAME,
+				P::TargetChain::NAME,
+				nonces,
+				total_prepaid_nonces,
+				total_size,
+				total_dispatch_weight,
+				inclusion_fee_in_target_tokens,
+				expected_refund_in_target_tokens,
+				conversion_rate,
+				delivery_fee_in_source_tokens,
 		);
-		Ok(
-			convert_target_tokens_to_source_tokens::<P::SourceChain, P::TargetChain>(
-				FixedU128::from_float(conversion_rate),
-				self.client
-					.estimate_extrinsic_fee(self.lane.make_messages_delivery_transaction(
-						Zero::zero(),
-						HeaderId(Default::default(), Default::default()),
-						nonces.clone(),
-						prepare_dummy_messages_proof::<P::SourceChain>(nonces, total_dispatch_weight, total_size),
-					))
-					.await
-					.unwrap_or_else(|_| <P::TargetChain as Chain>::Balance::max_value()),
-			),
-		)
+
+		Ok(delivery_fee_in_source_tokens)
 	}
 }
 
@@ -316,17 +377,110 @@ where
 		.unwrap_or_else(|_| SC::Balance::max_value())
 }
 
+/// Compute fee multiplier that is used by the chain, given couple of fees for transactions
+/// that are only differ in dispatch weights.
+///
+/// This function assumes that standard transaction payment pallet is used by the chain.
+/// The only fee component that depends on dispatch weight is the `adjusted_weight_fee`.
+///
+/// **WARNING**: this functions will only be accurate if weight-to-fee conversion function
+/// is linear. For non-linear polynomials the error will grow with `weight_difference` growth.
+/// So better to use smaller differences.
+fn compute_fee_multiplier<C: Chain>(
+	smaller_adjusted_weight_fee: BalanceOf<C>,
+	smaller_tx_weight: Weight,
+	larger_adjusted_weight_fee: BalanceOf<C>,
+	larger_tx_weight: Weight,
+) -> FixedU128 {
+	let adjusted_weight_fee_difference = larger_adjusted_weight_fee.saturating_sub(smaller_adjusted_weight_fee);
+	let smaller_tx_unadjusted_weight_fee = WeightToFeeOf::<C>::calc(&smaller_tx_weight);
+	let larger_tx_unadjusted_weight_fee = WeightToFeeOf::<C>::calc(&larger_tx_weight);
+	FixedU128::saturating_from_rational(
+		adjusted_weight_fee_difference,
+		larger_tx_unadjusted_weight_fee.saturating_sub(smaller_tx_unadjusted_weight_fee),
+	)
+}
+
+/// Compute fee that will be refunded to the relayer because dispatch of `total_prepaid_nonces`
+/// messages has been paid at the source chain.
+fn compute_prepaid_messages_refund<P: SubstrateMessageLane>(
+	total_prepaid_nonces: MessageNonce,
+	fee_multiplier: FixedU128,
+) -> BalanceOf<P::TargetChain> {
+	fee_multiplier.saturating_mul_int(WeightToFeeOf::<P::TargetChain>::calc(
+		&P::PAY_INBOUND_DISPATCH_FEE_WEIGHT_AT_TARGET_CHAIN.saturating_mul(total_prepaid_nonces),
+	))
+}
+
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use relay_millau_client::Millau;
-	use relay_rialto_client::Rialto;
+	use relay_rococo_client::{Rococo, SigningParams as RococoSigningParams};
+	use relay_wococo_client::{SigningParams as WococoSigningParams, Wococo};
+
+	#[derive(Clone)]
+	struct TestSubstrateMessageLane;
+
+	impl SubstrateMessageLane for TestSubstrateMessageLane {
+		type MessageLane = crate::messages_lane::SubstrateMessageLaneToSubstrate<
+			Rococo,
+			RococoSigningParams,
+			Wococo,
+			WococoSigningParams,
+		>;
+
+		const OUTBOUND_LANE_MESSAGE_DETAILS_METHOD: &'static str = "";
+		const OUTBOUND_LANE_LATEST_GENERATED_NONCE_METHOD: &'static str = "";
+		const OUTBOUND_LANE_LATEST_RECEIVED_NONCE_METHOD: &'static str = "";
+
+		const INBOUND_LANE_LATEST_RECEIVED_NONCE_METHOD: &'static str = "";
+		const INBOUND_LANE_LATEST_CONFIRMED_NONCE_METHOD: &'static str = "";
+		const INBOUND_LANE_UNREWARDED_RELAYERS_STATE: &'static str = "";
+
+		const BEST_FINALIZED_SOURCE_HEADER_ID_AT_TARGET: &'static str = "";
+		const BEST_FINALIZED_TARGET_HEADER_ID_AT_SOURCE: &'static str = "";
+
+		const MESSAGE_PALLET_NAME_AT_SOURCE: &'static str = "";
+		const MESSAGE_PALLET_NAME_AT_TARGET: &'static str = "";
+
+		const PAY_INBOUND_DISPATCH_FEE_WEIGHT_AT_TARGET_CHAIN: Weight = 100_000;
+
+		type SourceChain = Rococo;
+		type TargetChain = Wococo;
+
+		fn source_transactions_author(&self) -> bp_rococo::AccountId {
+			unreachable!()
+		}
+
+		fn make_messages_receiving_proof_transaction(
+			&self,
+			_transaction_nonce: <Rococo as Chain>::Index,
+			_generated_at_block: TargetHeaderIdOf<Self::MessageLane>,
+			_proof: <Self::MessageLane as MessageLane>::MessagesReceivingProof,
+		) -> Bytes {
+			unreachable!()
+		}
+
+		fn target_transactions_author(&self) -> bp_wococo::AccountId {
+			unreachable!()
+		}
+
+		fn make_messages_delivery_transaction(
+			&self,
+			_transaction_nonce: <Wococo as Chain>::Index,
+			_generated_at_header: SourceHeaderIdOf<Self::MessageLane>,
+			_nonces: RangeInclusive<MessageNonce>,
+			_proof: <Self::MessageLane as MessageLane>::MessagesProof,
+		) -> Bytes {
+			unreachable!()
+		}
+	}
 
 	#[test]
 	fn prepare_dummy_messages_proof_works() {
 		const DISPATCH_WEIGHT: Weight = 1_000_000;
 		const SIZE: u32 = 1_000;
-		let dummy_proof = prepare_dummy_messages_proof::<Rialto>(1..=10, DISPATCH_WEIGHT, SIZE);
+		let dummy_proof = prepare_dummy_messages_proof::<Rococo>(1..=10, DISPATCH_WEIGHT, SIZE);
 		assert_eq!(dummy_proof.0, DISPATCH_WEIGHT);
 		assert!(
 			dummy_proof.1.encode().len() as u32 > SIZE,
@@ -339,16 +493,47 @@ mod tests {
 	#[test]
 	fn convert_target_tokens_to_source_tokens_works() {
 		assert_eq!(
-			convert_target_tokens_to_source_tokens::<Rialto, Millau>((150, 100).into(), 1_000),
+			convert_target_tokens_to_source_tokens::<Rococo, Wococo>((150, 100).into(), 1_000),
 			1_500
 		);
 		assert_eq!(
-			convert_target_tokens_to_source_tokens::<Rialto, Millau>((50, 100).into(), 1_000),
+			convert_target_tokens_to_source_tokens::<Rococo, Wococo>((50, 100).into(), 1_000),
 			500
 		);
 		assert_eq!(
-			convert_target_tokens_to_source_tokens::<Rialto, Millau>((100, 100).into(), 1_000),
+			convert_target_tokens_to_source_tokens::<Rococo, Wococo>((100, 100).into(), 1_000),
 			1_000
 		);
 	}
+
+	#[test]
+	fn compute_fee_multiplier_returns_sane_results() {
+		let multiplier = FixedU128::saturating_from_rational(1, 1000);
+
+		let smaller_weight = 1_000_000;
+		let smaller_adjusted_weight_fee = multiplier.saturating_mul_int(WeightToFeeOf::<Rococo>::calc(&smaller_weight));
+
+		let larger_weight = smaller_weight + 200_000;
+		let larger_adjusted_weight_fee = multiplier.saturating_mul_int(WeightToFeeOf::<Rococo>::calc(&larger_weight));
+
+		assert_eq!(
+			compute_fee_multiplier::<Rococo>(
+				smaller_adjusted_weight_fee,
+				smaller_weight,
+				larger_adjusted_weight_fee,
+				larger_weight,
+			),
+			multiplier,
+		);
+	}
+
+	#[test]
+	fn compute_prepaid_messages_refund_returns_sane_results() {
+		assert!(
+			compute_prepaid_messages_refund::<TestSubstrateMessageLane>(
+				10,
+				FixedU128::saturating_from_rational(110, 100),
+			) > (10 * TestSubstrateMessageLane::PAY_INBOUND_DISPATCH_FEE_WEIGHT_AT_TARGET_CHAIN).into()
+		);
+	}
 }

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

@@ -420,10 +420,10 @@ fn on_demand_headers_relay_name<SourceChain: Chain, TargetChain: Chain>() -> Str
 mod tests {
 	use super::*;
 
-	type TestChain = relay_millau_client::Millau;
+	type TestChain = relay_rococo_client::Rococo;
 
-	const AT_SOURCE: Option<bp_millau::BlockNumber> = Some(10);
-	const AT_TARGET: Option<bp_millau::BlockNumber> = Some(1);
+	const AT_SOURCE: Option<bp_rococo::BlockNumber> = Some(10);
+	const AT_TARGET: Option<bp_rococo::BlockNumber> = Some(1);
 
 	#[async_std::test]
 	async fn mandatory_headers_scan_range_selects_range_if_too_many_headers_are_missing() {