Snowbridge Beacon header age check and add linear fee multiplier to e…

…nsure safety margins (#3815)

This is a cherry-pick from master of
#3790 and
#3727

Expected patches for (1.9.0):
snowbridge-pallet-ethereum-client
snowbridge-pallet-inbound-queue
snowbridge-pallet-outbound-queue
snowbridge-outbound-queue-runtime-api
snowbridge-pallet-system
snowbridge-core

---------

Co-authored-by: Vincent Geddes <vincent@snowfork.com>
Co-authored-by: Vincent Geddes <vincent.geddes@hey.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>

bridges/snowbridge/pallets/ethereum-client/src/lib.rs

@@ -130,6 +130,10 @@ pub mod pallet {
 		InvalidExecutionHeaderProof,
 		InvalidAncestryMerkleProof,
 		InvalidBlockRootsRootMerkleProof,
+		/// The gap between the finalized headers is larger than the sync committee period,
+		/// rendering execution headers unprovable using ancestry proofs (blocks root size is
+		/// the same as the sync committee period slots).
+		InvalidFinalizedHeaderGap,
 		HeaderNotFinalized,
 		BlockBodyHashTreeRootFailed,
 		HeaderHashTreeRootFailed,
@@ -398,6 +402,17 @@ pub mod pallet {
 				Error::<T>::IrrelevantUpdate
 			);
 
+			// Verify the finalized header gap between the current finalized header and new imported
+			// header is not larger than the sync committee period, otherwise we cannot do
+			// ancestry proofs for execution headers in the gap.
+			ensure!(
+				latest_finalized_state
+					.slot
+					.saturating_add(config::SLOTS_PER_HISTORICAL_ROOT as u64) >=
+					update.finalized_header.slot,
+				Error::<T>::InvalidFinalizedHeaderGap
+			);
+
 			// Verify that the `finality_branch`, if present, confirms `finalized_header` to match
 			// the finalized checkpoint root saved in the state of `attested_header`.
 			let finalized_block_root: H256 = update

bridges/snowbridge/pallets/ethereum-client/src/tests.rs

@@ -15,7 +15,7 @@ use crate::mock::{
 
 pub use crate::mock::*;
 
-use crate::config::{EPOCHS_PER_SYNC_COMMITTEE_PERIOD, SLOTS_PER_EPOCH};
+use crate::config::{EPOCHS_PER_SYNC_COMMITTEE_PERIOD, SLOTS_PER_EPOCH, SLOTS_PER_HISTORICAL_ROOT};
 use frame_support::{assert_err, assert_noop, assert_ok};
 use hex_literal::hex;
 use primitives::{
@@ -884,6 +884,61 @@ fn submit_execution_header_not_finalized() {
 	});
 }
 
+/// Check that a gap of more than 8192 slots between finalized headers is not allowed.
+#[test]
+fn submit_finalized_header_update_with_too_large_gap() {
+	let checkpoint = Box::new(load_checkpoint_update_fixture());
+	let update = Box::new(load_sync_committee_update_fixture());
+	let mut next_update = Box::new(load_next_sync_committee_update_fixture());
+
+	// Adds 8193 slots, so that the next update is still in the next sync committee, but the
+	// gap between the finalized headers is more than 8192 slots.
+	let slot_with_large_gap = checkpoint.header.slot + SLOTS_PER_HISTORICAL_ROOT as u64 + 1;
+
+	next_update.finalized_header.slot = slot_with_large_gap;
+	// Adding some slots to the attested header and signature slot since they need to be ahead
+	// of the finalized header.
+	next_update.attested_header.slot = slot_with_large_gap + 33;
+	next_update.signature_slot = slot_with_large_gap + 43;
+
+	new_tester().execute_with(|| {
+		assert_ok!(EthereumBeaconClient::process_checkpoint_update(&checkpoint));
+		assert_ok!(EthereumBeaconClient::submit(RuntimeOrigin::signed(1), update.clone()));
+		assert!(<NextSyncCommittee<Test>>::exists());
+		assert_err!(
+			EthereumBeaconClient::submit(RuntimeOrigin::signed(1), next_update.clone()),
+			Error::<Test>::InvalidFinalizedHeaderGap
+		);
+	});
+}
+
+/// Check that a gap of 8192 slots between finalized headers is allowed.
+#[test]
+fn submit_finalized_header_update_with_gap_at_limit() {
+	let checkpoint = Box::new(load_checkpoint_update_fixture());
+	let update = Box::new(load_sync_committee_update_fixture());
+	let mut next_update = Box::new(load_next_sync_committee_update_fixture());
+
+	next_update.finalized_header.slot = checkpoint.header.slot + SLOTS_PER_HISTORICAL_ROOT as u64;
+	// Adding some slots to the attested header and signature slot since they need to be ahead
+	// of the finalized header.
+	next_update.attested_header.slot =
+		checkpoint.header.slot + SLOTS_PER_HISTORICAL_ROOT as u64 + 33;
+	next_update.signature_slot = checkpoint.header.slot + SLOTS_PER_HISTORICAL_ROOT as u64 + 43;
+
+	new_tester().execute_with(|| {
+		assert_ok!(EthereumBeaconClient::process_checkpoint_update(&checkpoint));
+		assert_ok!(EthereumBeaconClient::submit(RuntimeOrigin::signed(1), update.clone()));
+		assert!(<NextSyncCommittee<Test>>::exists());
+		assert_err!(
+			EthereumBeaconClient::submit(RuntimeOrigin::signed(1), next_update.clone()),
+			// The test should pass the InvalidFinalizedHeaderGap check, and will fail at the
+			// next check, the merkle proof, because we changed the next_update slots.
+			Error::<Test>::InvalidHeaderMerkleProof
+		);
+	});
+}
+
 /* IMPLS */
 
 #[test]

bridges/snowbridge/pallets/inbound-queue/src/mock.rs

@@ -173,7 +173,8 @@ parameter_types! {
 	pub Parameters: PricingParameters<u128> = PricingParameters {
 		exchange_rate: FixedU128::from_rational(1, 400),
 		fee_per_gas: gwei(20),
-		rewards: Rewards { local: DOT, remote: meth(1) }
+		rewards: Rewards { local: DOT, remote: meth(1) },
+		multiplier: FixedU128::from_rational(1, 1),
 	};
 }
 

bridges/snowbridge/pallets/outbound-queue/runtime-api/src/lib.rs

@@ -3,18 +3,21 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 
 use frame_support::traits::tokens::Balance as BalanceT;
-use snowbridge_core::outbound::Message;
+use snowbridge_core::{
+	outbound::{Command, Fee},
+	PricingParameters,
+};
 use snowbridge_outbound_queue_merkle_tree::MerkleProof;
 
 sp_api::decl_runtime_apis! {
 	pub trait OutboundQueueApi<Balance> where Balance: BalanceT
 	{
 		/// Generate a merkle proof for a committed message identified by `leaf_index`.
 		/// The merkle root is stored in the block header as a
-		/// `\[`sp_runtime::generic::DigestItem::Other`\]`
+		/// `sp_runtime::generic::DigestItem::Other`
 		fn prove_message(leaf_index: u64) -> Option<MerkleProof>;
 
-		/// Calculate the delivery fee for `message`
-		fn calculate_fee(message: Message) -> Option<Balance>;
+		/// Calculate the delivery fee for `command`
+		fn calculate_fee(command: Command, parameters: Option<PricingParameters<Balance>>) -> Fee<Balance>;
 	}
 }

bridges/snowbridge/pallets/outbound-queue/src/api.rs

@@ -4,8 +4,12 @@
 
 use crate::{Config, MessageLeaves};
 use frame_support::storage::StorageStreamIter;
-use snowbridge_core::outbound::{Message, SendMessage};
+use snowbridge_core::{
+	outbound::{Command, Fee, GasMeter},
+	PricingParameters,
+};
 use snowbridge_outbound_queue_merkle_tree::{merkle_proof, MerkleProof};
+use sp_core::Get;
 
 pub fn prove_message<T>(leaf_index: u64) -> Option<MerkleProof>
 where
@@ -19,12 +23,14 @@ where
 	Some(proof)
 }
 
-pub fn calculate_fee<T>(message: Message) -> Option<T::Balance>
+pub fn calculate_fee<T>(
+	command: Command,
+	parameters: Option<PricingParameters<T::Balance>>,
+) -> Fee<T::Balance>
 where
 	T: Config,
 {
-	match crate::Pallet::<T>::validate(&message) {
-		Ok((_, fees)) => Some(fees.total()),
-		_ => None,
-	}
+	let gas_used_at_most = T::GasMeter::maximum_gas_used_at_most(&command);
+	let parameters = parameters.unwrap_or(T::PricingParameters::get());
+	crate::Pallet::<T>::calculate_fee(gas_used_at_most, parameters)
 }

bridges/snowbridge/pallets/outbound-queue/src/lib.rs

@@ -47,24 +47,37 @@
 //! consume on Ethereum. Using this upper bound, a final fee can be calculated.
 //!
 //! The fee calculation also requires the following parameters:
-//! * ETH/DOT exchange rate
-//! * Ether fee per unit of gas
+//! * Average ETH/DOT exchange rate over some period
+//! * Max fee per unit of gas that bridge is willing to refund relayers for
 //!
 //! By design, it is expected that governance should manually update these
 //! parameters every few weeks using the `set_pricing_parameters` extrinsic in the
 //! system pallet.
 //!
+//! This is an interim measure. Once ETH/DOT liquidity pools are available in the Polkadot network,
+//! we'll use them as a source of pricing info, subject to certain safeguards.
+//!
 //! ## Fee Computation Function
 //!
 //! ```text
 //! LocalFee(Message) = WeightToFee(ProcessMessageWeight(Message))
-//! RemoteFee(Message) = MaxGasRequired(Message) * FeePerGas + Reward
-//! Fee(Message) = LocalFee(Message) + (RemoteFee(Message) / Ratio("ETH/DOT"))
+//! RemoteFee(Message) = MaxGasRequired(Message) * Params.MaxFeePerGas + Params.Reward
+//! RemoteFeeAdjusted(Message) = Params.Multiplier * (RemoteFee(Message) / Params.Ratio("ETH/DOT"))
+//! Fee(Message) = LocalFee(Message) + RemoteFeeAdjusted(Message)
 //! ```
 //!
-//! By design, the computed fee is always going to conservative, to cover worst-case
-//! costs of dispatch on Ethereum. In future iterations of the design, we will optimize
-//! this, or provide a mechanism to asynchronously refund a portion of collected fees.
+//! By design, the computed fee includes a safety factor (the `Multiplier`) to cover
+//! unfavourable fluctuations in the ETH/DOT exchange rate.
+//!
+//! ## Fee Settlement
+//!
+//! On the remote side, in the gateway contract, the relayer accrues
+//!
+//! ```text
+//! Min(GasPrice, Message.MaxFeePerGas) * GasUsed() + Message.Reward
+//! ```
+//! Or in plain english, relayers are refunded for gas consumption, using a
+//! price that is a minimum of the actual gas price, or `Message.MaxFeePerGas`.
 //!
 //! # Extrinsics
 //!
@@ -106,7 +119,7 @@ pub use snowbridge_outbound_queue_merkle_tree::MerkleProof;
 use sp_core::{H256, U256};
 use sp_runtime::{
 	traits::{CheckedDiv, Hash},
-	DigestItem,
+	DigestItem, Saturating,
 };
 use sp_std::prelude::*;
 pub use types::{CommittedMessage, ProcessMessageOriginOf};
@@ -366,8 +379,9 @@ pub mod pallet {
 			// downcast to u128
 			let fee: u128 = fee.try_into().defensive_unwrap_or(u128::MAX);
 
-			// convert to local currency
+			// multiply by multiplier and convert to local currency
 			let fee = FixedU128::from_inner(fee)
+				.saturating_mul(params.multiplier)
 				.checked_div(&params.exchange_rate)
 				.expect("exchange rate is not zero; qed")
 				.into_inner();

bridges/snowbridge/pallets/outbound-queue/src/mock.rs

@@ -77,7 +77,8 @@ parameter_types! {
 	pub Parameters: PricingParameters<u128> = PricingParameters {
 		exchange_rate: FixedU128::from_rational(1, 400),
 		fee_per_gas: gwei(20),
-		rewards: Rewards { local: DOT, remote: meth(1) }
+		rewards: Rewards { local: DOT, remote: meth(1) },
+		multiplier: FixedU128::from_rational(4, 3),
 	};
 }
 

bridges/snowbridge/pallets/outbound-queue/src/test.rs

@@ -268,42 +268,48 @@ fn encode_digest_item() {
 }
 
 #[test]
-fn validate_messages_with_fees() {
+fn test_calculate_fees_with_unit_multiplier() {
 	new_tester().execute_with(|| {
-		let message = mock_message(1000);
-		let (_, fee) = OutboundQueue::validate(&message).unwrap();
+		let gas_used: u64 = 250000;
+		let price_params: PricingParameters<<Test as Config>::Balance> = PricingParameters {
+			exchange_rate: FixedU128::from_rational(1, 400),
+			fee_per_gas: 10000_u32.into(),
+			rewards: Rewards { local: 1_u32.into(), remote: 1_u32.into() },
+			multiplier: FixedU128::from_rational(1, 1),
+		};
+		let fee = OutboundQueue::calculate_fee(gas_used, price_params);
 		assert_eq!(fee.local, 698000000);
-		assert_eq!(fee.remote, 2680000000000);
+		assert_eq!(fee.remote, 1000000);
 	});
 }
 
 #[test]
-fn test_calculate_fees() {
+fn test_calculate_fees_with_multiplier() {
 	new_tester().execute_with(|| {
 		let gas_used: u64 = 250000;
-		let illegal_price_params: PricingParameters<<Test as Config>::Balance> =
-			PricingParameters {
-				exchange_rate: FixedU128::from_rational(1, 400),
-				fee_per_gas: 10000_u32.into(),
-				rewards: Rewards { local: 1_u32.into(), remote: 1_u32.into() },
-			};
-		let fee = OutboundQueue::calculate_fee(gas_used, illegal_price_params);
+		let price_params: PricingParameters<<Test as Config>::Balance> = PricingParameters {
+			exchange_rate: FixedU128::from_rational(1, 400),
+			fee_per_gas: 10000_u32.into(),
+			rewards: Rewards { local: 1_u32.into(), remote: 1_u32.into() },
+			multiplier: FixedU128::from_rational(4, 3),
+		};
+		let fee = OutboundQueue::calculate_fee(gas_used, price_params);
 		assert_eq!(fee.local, 698000000);
-		assert_eq!(fee.remote, 1000000);
+		assert_eq!(fee.remote, 1333333);
 	});
 }
 
 #[test]
 fn test_calculate_fees_with_valid_exchange_rate_but_remote_fee_calculated_as_zero() {
 	new_tester().execute_with(|| {
 		let gas_used: u64 = 250000;
-		let illegal_price_params: PricingParameters<<Test as Config>::Balance> =
-			PricingParameters {
-				exchange_rate: FixedU128::from_rational(1, 1),
-				fee_per_gas: 1_u32.into(),
-				rewards: Rewards { local: 1_u32.into(), remote: 1_u32.into() },
-			};
-		let fee = OutboundQueue::calculate_fee(gas_used, illegal_price_params.clone());
+		let price_params: PricingParameters<<Test as Config>::Balance> = PricingParameters {
+			exchange_rate: FixedU128::from_rational(1, 1),
+			fee_per_gas: 1_u32.into(),
+			rewards: Rewards { local: 1_u32.into(), remote: 1_u32.into() },
+			multiplier: FixedU128::from_rational(1, 1),
+		};
+		let fee = OutboundQueue::calculate_fee(gas_used, price_params.clone());
 		assert_eq!(fee.local, 698000000);
 		// Though none zero pricing params the remote fee calculated here is invalid
 		// which should be avoided

bridges/snowbridge/pallets/system/src/lib.rs

@@ -159,6 +159,7 @@ pub mod pallet {
 		type DefaultPricingParameters: Get<PricingParametersOf<Self>>;
 
 		/// Cost of delivering a message from Ethereum
+		#[pallet::constant]
 		type InboundDeliveryCost: Get<BalanceOf<Self>>;
 
 		type WeightInfo: WeightInfo;
@@ -334,6 +335,7 @@ pub mod pallet {
 			let command = Command::SetPricingParameters {
 				exchange_rate: params.exchange_rate.into(),
 				delivery_cost: T::InboundDeliveryCost::get().saturated_into::<u128>(),
+				multiplier: params.multiplier.into(),
 			};
 			Self::send(PRIMARY_GOVERNANCE_CHANNEL, command, PaysFee::<T>::No)?;
 

bridges/snowbridge/pallets/system/src/mock.rs

@@ -193,7 +193,8 @@ parameter_types! {
 	pub Parameters: PricingParameters<u128> = PricingParameters {
 		exchange_rate: FixedU128::from_rational(1, 400),
 		fee_per_gas: gwei(20),
-		rewards: Rewards { local: DOT, remote: meth(1) }
+		rewards: Rewards { local: DOT, remote: meth(1) },
+		multiplier: FixedU128::from_rational(4, 3)
 	};
 	pub const InboundDeliveryCost: u128 = 1_000_000_000;
 

bridges/snowbridge/primitives/core/src/outbound.rs

@@ -136,6 +136,8 @@ mod v1 {
 			exchange_rate: UD60x18,
 			// Cost of delivering a message from Ethereum to BridgeHub, in ROC/KSM/DOT
 			delivery_cost: u128,
+			// Fee multiplier
+			multiplier: UD60x18,
 		},
 	}
 
@@ -203,10 +205,11 @@ mod v1 {
 					Token::Uint(U256::from(*transfer_asset_xcm)),
 					Token::Uint(*register_token),
 				])]),
-				Command::SetPricingParameters { exchange_rate, delivery_cost } =>
+				Command::SetPricingParameters { exchange_rate, delivery_cost, multiplier } =>
 					ethabi::encode(&[Token::Tuple(vec![
 						Token::Uint(exchange_rate.clone().into_inner()),
 						Token::Uint(U256::from(*delivery_cost)),
+						Token::Uint(multiplier.clone().into_inner()),
 					])]),
 			}
 		}
@@ -273,7 +276,8 @@ mod v1 {
 	}
 }
 
-#[cfg_attr(feature = "std", derive(PartialEq, Debug))]
+#[derive(Clone, Encode, Decode, RuntimeDebug, TypeInfo)]
+#[cfg_attr(feature = "std", derive(PartialEq))]
 /// Fee for delivering message
 pub struct Fee<Balance>
 where
@@ -346,12 +350,13 @@ pub trait GasMeter {
 	/// the command within the message
 	const MAXIMUM_BASE_GAS: u64;
 
+	/// Total gas consumed at most, including verification & dispatch
 	fn maximum_gas_used_at_most(command: &Command) -> u64 {
 		Self::MAXIMUM_BASE_GAS + Self::maximum_dispatch_gas_used_at_most(command)
 	}
 
-	/// Measures the maximum amount of gas a command payload will require to dispatch, AFTER
-	/// validation & verification.
+	/// Measures the maximum amount of gas a command payload will require to *dispatch*, NOT
+	/// including validation & verification.
 	fn maximum_dispatch_gas_used_at_most(command: &Command) -> u64;
 }