diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index f9d8f891fd44..5195f9b48a2a 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -67,6 +67,9 @@ use sp_std::prelude::*; use sp_version::NativeVersion; use sp_version::RuntimeVersion; +// to be able to use Millau runtime in `bridge-runtime-common` tests +pub use bridge_runtime_common; + // A few exports that help ease life for downstream crates. pub use frame_support::{ construct_runtime, parameter_types, @@ -570,6 +573,12 @@ pallet_bridge_parachains::declare_bridge_reject_obsolete_parachain_header! { Call::BridgeRialtoParachains => WithRialtoParachainsInstance } +bridge_runtime_common::declare_bridge_reject_obsolete_messages! { + Runtime, + Call::BridgeRialtoMessages => WithRialtoMessagesInstance, + Call::BridgeRialtoParachainMessages => WithRialtoParachainMessagesInstance +} + /// The address format for describing accounts. pub type Address = AccountId; /// Block header type as expected by this runtime. @@ -592,6 +601,7 @@ pub type SignedExtra = ( pallet_transaction_payment::ChargeTransactionPayment, BridgeRejectObsoleteGrandpaHeader, BridgeRejectObsoleteParachainHeader, + BridgeRejectObsoleteMessages, ); /// The payload being signed in transactions. pub type SignedPayload = generic::SignedPayload; diff --git a/bridges/bin/runtime-common/Cargo.toml b/bridges/bin/runtime-common/Cargo.toml index 198df7f16321..3bf778ee50a0 100644 --- a/bridges/bin/runtime-common/Cargo.toml +++ b/bridges/bin/runtime-common/Cargo.toml @@ -46,6 +46,9 @@ xcm = { git = "https://github.com/paritytech/polkadot", branch = "gav-xcm-v3", d xcm-builder = { git = "https://github.com/paritytech/polkadot", branch = "gav-xcm-v3", default-features = false } xcm-executor = { git = "https://github.com/paritytech/polkadot", branch = "gav-xcm-v3", default-features = false } +[dev-dependencies] +millau-runtime = { path = "../millau/runtime" } + [features] default = ["std"] std = [ diff --git a/bridges/bin/runtime-common/src/lib.rs b/bridges/bin/runtime-common/src/lib.rs index 616a55d43662..9776fd5827d8 100644 --- a/bridges/bin/runtime-common/src/lib.rs +++ b/bridges/bin/runtime-common/src/lib.rs @@ -21,6 +21,7 @@ pub mod messages; pub mod messages_api; pub mod messages_benchmarking; +pub mod messages_extension; pub mod parachains_benchmarking; #[cfg(feature = "integrity-test")] diff --git a/bridges/bin/runtime-common/src/messages.rs b/bridges/bin/runtime-common/src/messages.rs index d302d35c8565..a9a252118b25 100644 --- a/bridges/bin/runtime-common/src/messages.rs +++ b/bridges/bin/runtime-common/src/messages.rs @@ -197,9 +197,6 @@ pub fn transaction_payment( pub mod source { use super::*; - /// Encoded Call of the Bridged chain. We never try to decode it on This chain. - pub type BridgedChainOpaqueCall = Vec; - /// Message payload for This -> Bridged chain messages. pub type FromThisChainMessagePayload = Vec; @@ -523,6 +520,7 @@ pub mod target { pub bridged_header_hash: BridgedHeaderHash, /// A storage trie proof of messages being delivered. pub storage_proof: RawStorageProof, + /// Messages in this proof are sent over this lane. pub lane: LaneId, /// Nonce of the first message being delivered. pub nonces_start: MessageNonce, diff --git a/bridges/bin/runtime-common/src/messages_extension.rs b/bridges/bin/runtime-common/src/messages_extension.rs new file mode 100644 index 000000000000..fd1d9d7a934b --- /dev/null +++ b/bridges/bin/runtime-common/src/messages_extension.rs @@ -0,0 +1,261 @@ +// Copyright 2021 Parity Technologies (UK) Ltd. +// This file is part of Parity Bridges Common. + +// Parity Bridges Common is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Parity Bridges Common is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Parity Bridges Common. If not, see . + +/// Declares a runtime-specific `BridgeRejectObsoleteMessages` and +/// `BridgeRejectObsoleteMessageConfirmations` signed extensions. +/// +/// ## Example +/// +/// ```nocompile +/// bridge_runtime_common::declare_bridge_reject_obsolete_messages!{ +/// Runtime, +/// Call::BridgeRialtoMessages => WithRialtoMessagesInstance, +/// Call::BridgeRialtoParachainMessages => WithRialtoParachainMessagesInstance, +/// } +/// ``` +/// +/// The goal of this extension is to avoid "mining" messages delivery and delivery confirmation +/// transactions, that are delivering outdated messages/confirmations. Without that extension, +/// even honest relayers may lose their funds if there are multiple relays running and submitting +/// the same messages/confirmations. +#[macro_export] +macro_rules! declare_bridge_reject_obsolete_messages { + ($runtime:ident, $($call:path => $instance:ty),*) => { + /// Transaction-with-obsolete-messages check that will reject transaction if + /// it submits obsolete messages/confirmations. + #[derive(Clone, codec::Decode, codec::Encode, Eq, PartialEq, frame_support::RuntimeDebug, scale_info::TypeInfo)] + pub struct BridgeRejectObsoleteMessages; + + impl sp_runtime::traits::SignedExtension for BridgeRejectObsoleteMessages { + const IDENTIFIER: &'static str = "BridgeRejectObsoleteMessages"; + type AccountId = <$runtime as frame_system::Config>::AccountId; + type Call = <$runtime as frame_system::Config>::Call; + type AdditionalSigned = (); + type Pre = (); + + fn additional_signed(&self) -> sp_std::result::Result< + (), + sp_runtime::transaction_validity::TransactionValidityError, + > { + Ok(()) + } + + fn validate( + &self, + _who: &Self::AccountId, + call: &Self::Call, + _info: &sp_runtime::traits::DispatchInfoOf, + _len: usize, + ) -> sp_runtime::transaction_validity::TransactionValidity { + match *call { + $( + $call(pallet_bridge_messages::Call::<$runtime, $instance>::receive_messages_proof { + ref proof, + .. + }) => { + let nonces_end = proof.nonces_end; + + let inbound_lane_data = pallet_bridge_messages::InboundLanes::<$runtime, $instance>::get(&proof.lane); + if proof.nonces_end <= inbound_lane_data.last_delivered_nonce() { + return sp_runtime::transaction_validity::InvalidTransaction::Stale.into(); + } + + Ok(sp_runtime::transaction_validity::ValidTransaction::default()) + }, + $call(pallet_bridge_messages::Call::<$runtime, $instance>::receive_messages_delivery_proof { + ref proof, + ref relayers_state, + .. + }) => { + let latest_delivered_nonce = relayers_state.last_delivered_nonce; + + let outbound_lane_data = pallet_bridge_messages::OutboundLanes::<$runtime, $instance>::get(&proof.lane); + if latest_delivered_nonce <= outbound_lane_data.latest_received_nonce { + return sp_runtime::transaction_validity::InvalidTransaction::Stale.into(); + } + + Ok(sp_runtime::transaction_validity::ValidTransaction::default()) + } + )* + _ => Ok(sp_runtime::transaction_validity::ValidTransaction::default()), + } + } + + fn pre_dispatch( + self, + who: &Self::AccountId, + call: &Self::Call, + info: &sp_runtime::traits::DispatchInfoOf, + len: usize, + ) -> Result { + self.validate(who, call, info, len).map(drop) + } + + fn post_dispatch( + _maybe_pre: Option, + _info: &sp_runtime::traits::DispatchInfoOf, + _post_info: &sp_runtime::traits::PostDispatchInfoOf, + _len: usize, + _result: &sp_runtime::DispatchResult, + ) -> Result<(), sp_runtime::transaction_validity::TransactionValidityError> { + Ok(()) + } + } + }; +} + +#[cfg(test)] +mod tests { + use bp_messages::UnrewardedRelayersState; + use frame_support::weights::{DispatchClass, DispatchInfo, Pays}; + use millau_runtime::{ + bridge_runtime_common::messages::{ + source::FromBridgedChainMessagesDeliveryProof, target::FromBridgedChainMessagesProof, + }, + BridgeRejectObsoleteMessages, Call, Runtime, WithRialtoMessagesInstance, + }; + use sp_runtime::traits::SignedExtension; + + fn deliver_message_10() { + pallet_bridge_messages::InboundLanes::::insert( + [0, 0, 0, 0], + bp_messages::InboundLaneData { relayers: Default::default(), last_confirmed_nonce: 10 }, + ); + } + + fn validate_message_delivery( + nonces_start: bp_messages::MessageNonce, + nonces_end: bp_messages::MessageNonce, + ) -> bool { + BridgeRejectObsoleteMessages + .validate( + &[0u8; 32].into(), + &Call::BridgeRialtoMessages(pallet_bridge_messages::Call::< + Runtime, + WithRialtoMessagesInstance, + >::receive_messages_proof { + relayer_id_at_bridged_chain: [0u8; 32].into(), + messages_count: (nonces_end - nonces_start + 1) as u32, + dispatch_weight: 0, + proof: FromBridgedChainMessagesProof { + bridged_header_hash: Default::default(), + storage_proof: vec![], + lane: [0, 0, 0, 0], + nonces_start, + nonces_end, + }, + }), + &DispatchInfo { weight: 0, class: DispatchClass::Operational, pays_fee: Pays::Yes }, + 0, + ) + .is_ok() + } + + #[test] + fn extension_rejects_obsolete_messages() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + // when current best delivered is message#10 and we're trying to deliver message#5 => tx + // is rejected + deliver_message_10(); + assert!(!validate_message_delivery(8, 9)); + }); + } + + #[test] + fn extension_rejects_same_message() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + // when current best delivered is message#10 and we're trying to import message#10 => tx + // is rejected + deliver_message_10(); + assert!(!validate_message_delivery(8, 10)); + }); + } + + #[test] + fn extension_accepts_new_message() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + // when current best delivered is message#10 and we're trying to deliver message#15 => + // tx is accepted + deliver_message_10(); + assert!(validate_message_delivery(10, 15)); + }); + } + + fn confirm_message_10() { + pallet_bridge_messages::OutboundLanes::::insert( + [0, 0, 0, 0], + bp_messages::OutboundLaneData { + oldest_unpruned_nonce: 0, + latest_received_nonce: 10, + latest_generated_nonce: 10, + }, + ); + } + + fn validate_message_confirmation(last_delivered_nonce: bp_messages::MessageNonce) -> bool { + BridgeRejectObsoleteMessages + .validate( + &[0u8; 32].into(), + &Call::BridgeRialtoMessages(pallet_bridge_messages::Call::< + Runtime, + WithRialtoMessagesInstance, + >::receive_messages_delivery_proof { + proof: FromBridgedChainMessagesDeliveryProof { + bridged_header_hash: Default::default(), + storage_proof: Vec::new(), + lane: [0, 0, 0, 0], + }, + relayers_state: UnrewardedRelayersState { + last_delivered_nonce, + ..Default::default() + }, + }), + &DispatchInfo { weight: 0, class: DispatchClass::Operational, pays_fee: Pays::Yes }, + 0, + ) + .is_ok() + } + + #[test] + fn extension_rejects_obsolete_confirmations() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + // when current best confirmed is message#10 and we're trying to confirm message#5 => tx + // is rejected + confirm_message_10(); + assert!(!validate_message_confirmation(5)); + }); + } + + #[test] + fn extension_rejects_same_confirmation() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + // when current best confirmed is message#10 and we're trying to confirm message#10 => + // tx is rejected + confirm_message_10(); + assert!(!validate_message_confirmation(10)); + }); + } + + #[test] + fn extension_accepts_new_confirmation() { + sp_io::TestExternalities::new(Default::default()).execute_with(|| { + // when current best confirmed is message#10 and we're trying to confirm message#15 => + // tx is accepted + confirm_message_10(); + assert!(validate_message_confirmation(15)); + }); + } +} diff --git a/bridges/modules/messages/src/benchmarking.rs b/bridges/modules/messages/src/benchmarking.rs index 66aa70312cc9..c312528d4bd2 100644 --- a/bridges/modules/messages/src/benchmarking.rs +++ b/bridges/modules/messages/src/benchmarking.rs @@ -493,6 +493,7 @@ benchmarks_instance_pallet! { unrewarded_relayer_entries: 1, messages_in_oldest_entry: 1, total_messages: 1, + last_delivered_nonce: 1, }; let proof = T::prepare_message_delivery_proof(MessageDeliveryProofParams { lane: T::bench_lane_id(), @@ -534,6 +535,7 @@ benchmarks_instance_pallet! { unrewarded_relayer_entries: 1, messages_in_oldest_entry: 2, total_messages: 2, + last_delivered_nonce: 2, }; let mut delivered_messages = DeliveredMessages::new(1, true); delivered_messages.note_dispatched_message(true); @@ -576,6 +578,7 @@ benchmarks_instance_pallet! { unrewarded_relayer_entries: 2, messages_in_oldest_entry: 1, total_messages: 2, + last_delivered_nonce: 2, }; let proof = T::prepare_message_delivery_proof(MessageDeliveryProofParams { lane: T::bench_lane_id(), diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs index a41ea3d1d95a..0f287a9df47e 100644 --- a/bridges/modules/messages/src/lib.rs +++ b/bridges/modules/messages/src/lib.rs @@ -554,6 +554,12 @@ pub mod pallet { relayers_state.unrewarded_relayer_entries, Error::::InvalidUnrewardedRelayersState ); + // the `last_delivered_nonce` field may also be used by the signed extension. Even + // though providing wrong value isn't critical, let's also check it here. + ensure!( + lane_data.last_delivered_nonce() == relayers_state.last_delivered_nonce, + Error::::InvalidUnrewardedRelayersState + ); // mark messages as delivered let mut lane = outbound_lane::(lane_id); @@ -1159,7 +1165,9 @@ mod tests { fn inbound_unrewarded_relayers_state( lane: bp_messages::LaneId, ) -> bp_messages::UnrewardedRelayersState { - let relayers = InboundLanes::::get(&lane).relayers; + let inbound_lane_data = InboundLanes::::get(&lane); + let last_delivered_nonce = inbound_lane_data.last_delivered_nonce(); + let relayers = inbound_lane_data.relayers; bp_messages::UnrewardedRelayersState { unrewarded_relayer_entries: relayers.len() as _, messages_in_oldest_entry: relayers @@ -1167,6 +1175,7 @@ mod tests { .map(|entry| 1 + entry.messages.end - entry.messages.begin) .unwrap_or(0), total_messages: total_unrewarded_messages(&relayers).unwrap_or(MessageNonce::MAX), + last_delivered_nonce, } } @@ -1225,6 +1234,7 @@ mod tests { UnrewardedRelayersState { unrewarded_relayer_entries: 1, total_messages: 1, + last_delivered_nonce: 1, ..Default::default() }, )); @@ -1467,6 +1477,7 @@ mod tests { unrewarded_relayer_entries: 1, messages_in_oldest_entry: 1, total_messages: 1, + last_delivered_nonce: 1, }, ), Error::::Halted, @@ -1522,6 +1533,7 @@ mod tests { unrewarded_relayer_entries: 1, messages_in_oldest_entry: 1, total_messages: 1, + last_delivered_nonce: 1, }, )); }); @@ -1619,6 +1631,7 @@ mod tests { unrewarded_relayer_entries: 2, messages_in_oldest_entry: 1, total_messages: 2, + last_delivered_nonce: 10, }, ); @@ -1654,6 +1667,7 @@ mod tests { unrewarded_relayer_entries: 2, messages_in_oldest_entry: 1, total_messages: 2, + last_delivered_nonce: 11, }, ); }); @@ -1749,6 +1763,7 @@ mod tests { UnrewardedRelayersState { unrewarded_relayer_entries: 1, total_messages: 1, + last_delivered_nonce: 1, ..Default::default() }, )); @@ -1774,6 +1789,7 @@ mod tests { UnrewardedRelayersState { unrewarded_relayer_entries: 2, total_messages: 2, + last_delivered_nonce: 2, ..Default::default() }, )); @@ -1818,6 +1834,7 @@ mod tests { UnrewardedRelayersState { unrewarded_relayer_entries: 1, total_messages: 2, + last_delivered_nonce: 2, ..Default::default() }, ), @@ -1843,6 +1860,33 @@ mod tests { UnrewardedRelayersState { unrewarded_relayer_entries: 2, total_messages: 1, + last_delivered_nonce: 2, + ..Default::default() + }, + ), + Error::::InvalidUnrewardedRelayersState, + ); + + // when last delivered nonce is invalid + assert_noop!( + Pallet::::receive_messages_delivery_proof( + Origin::signed(1), + TestMessagesDeliveryProof(Ok(( + TEST_LANE_ID, + InboundLaneData { + relayers: vec![ + unrewarded_relayer(1, 1, TEST_RELAYER_A), + unrewarded_relayer(2, 2, TEST_RELAYER_B) + ] + .into_iter() + .collect(), + ..Default::default() + } + ))), + UnrewardedRelayersState { + unrewarded_relayer_entries: 2, + total_messages: 2, + last_delivered_nonce: 8, ..Default::default() }, ), @@ -2079,6 +2123,7 @@ mod tests { UnrewardedRelayersState { unrewarded_relayer_entries: 1, total_messages: 2, + last_delivered_nonce: 2, ..Default::default() }, )); @@ -2089,6 +2134,7 @@ mod tests { UnrewardedRelayersState { unrewarded_relayer_entries: 1, total_messages: 1, + last_delivered_nonce: 3, ..Default::default() }, )); @@ -2116,6 +2162,7 @@ mod tests { let relayers_state = UnrewardedRelayersState { unrewarded_relayer_entries: 1, total_messages: 3, + last_delivered_nonce: 3, ..Default::default() }; let pre_dispatch_weight = @@ -2190,7 +2237,7 @@ mod tests { TEST_LANE_ID, InboundLaneData { last_confirmed_nonce: 1, relayers: Default::default() }, ))), - UnrewardedRelayersState::default(), + UnrewardedRelayersState { last_delivered_nonce: 1, ..Default::default() }, ), Error::::TryingToConfirmMoreMessagesThanExpected, ); @@ -2269,6 +2316,7 @@ mod tests { UnrewardedRelayersState { unrewarded_relayer_entries: 1, total_messages: max_messages_to_prune, + last_delivered_nonce: max_messages_to_prune, ..Default::default() }, )); diff --git a/bridges/primitives/messages/src/lib.rs b/bridges/primitives/messages/src/lib.rs index 35df6d01f1cb..8d51a6fa8646 100644 --- a/bridges/primitives/messages/src/lib.rs +++ b/bridges/primitives/messages/src/lib.rs @@ -289,6 +289,11 @@ pub struct UnrewardedRelayersState { pub messages_in_oldest_entry: MessageNonce, /// Total number of messages in the relayers vector. pub total_messages: MessageNonce, + /// Nonce of the latest message that has been delivered to the target chain. + /// + /// This corresponds to the result of the `InboundLaneData::last_delivered_nonce` call + /// at the bridged chain. + pub last_delivered_nonce: MessageNonce, } /// Outbound lane data. diff --git a/bridges/relays/client-millau/src/lib.rs b/bridges/relays/client-millau/src/lib.rs index e0837e50bb72..36ad9df69d69 100644 --- a/bridges/relays/client-millau/src/lib.rs +++ b/bridges/relays/client-millau/src/lib.rs @@ -117,6 +117,7 @@ impl TransactionSignScheme for Millau { pallet_transaction_payment::ChargeTransactionPayment::::from(param.unsigned.tip), millau_runtime::BridgeRejectObsoleteGrandpaHeader, millau_runtime::BridgeRejectObsoleteParachainHeader, + millau_runtime::BridgeRejectObsoleteMessages, ), ( (), @@ -129,6 +130,7 @@ impl TransactionSignScheme for Millau { (), (), (), + (), ), ); let signature = raw_payload.using_encoded(|payload| param.signer.sign(payload)); diff --git a/bridges/relays/lib-substrate-relay/src/messages_source.rs b/bridges/relays/lib-substrate-relay/src/messages_source.rs index 7dc582dd5762..85bd9ea7d0f7 100644 --- a/bridges/relays/lib-substrate-relay/src/messages_source.rs +++ b/bridges/relays/lib-substrate-relay/src/messages_source.rs @@ -474,6 +474,7 @@ fn prepare_dummy_messages_delivery_proof( unrewarded_relayer_entries: 1, messages_in_oldest_entry: 1, total_messages: 1, + last_delivered_nonce: 1, }, FromBridgedChainMessagesDeliveryProof { bridged_header_hash: Default::default(), diff --git a/bridges/relays/lib-substrate-relay/src/messages_target.rs b/bridges/relays/lib-substrate-relay/src/messages_target.rs index 08604e66b72a..00c0c1570541 100644 --- a/bridges/relays/lib-substrate-relay/src/messages_target.rs +++ b/bridges/relays/lib-substrate-relay/src/messages_target.rs @@ -191,11 +191,10 @@ where id: TargetHeaderIdOf>, ) -> Result<(TargetHeaderIdOf>, UnrewardedRelayersState), SubstrateError> { - let relayers = self - .inbound_lane_data(id) - .await? - .map(|data| data.relayers) - .unwrap_or_else(VecDeque::new); + let inbound_lane_data = self.inbound_lane_data(id).await?; + let last_delivered_nonce = + inbound_lane_data.as_ref().map(|data| data.last_delivered_nonce()).unwrap_or(0); + let relayers = inbound_lane_data.map(|data| data.relayers).unwrap_or_else(VecDeque::new); let unrewarded_relayers_state = bp_messages::UnrewardedRelayersState { unrewarded_relayer_entries: relayers.len() as _, messages_in_oldest_entry: relayers @@ -203,6 +202,7 @@ where .map(|entry| 1 + entry.messages.end - entry.messages.begin) .unwrap_or(0), total_messages: total_unrewarded_messages(&relayers).unwrap_or(MessageNonce::MAX), + last_delivered_nonce, }; Ok((id, unrewarded_relayers_state)) } diff --git a/bridges/relays/messages/src/message_lane_loop.rs b/bridges/relays/messages/src/message_lane_loop.rs index 487623096f06..84473f0f27b2 100644 --- a/bridges/relays/messages/src/message_lane_loop.rs +++ b/bridges/relays/messages/src/message_lane_loop.rs @@ -729,6 +729,7 @@ pub(crate) mod tests { unrewarded_relayer_entries: 0, messages_in_oldest_entry: 0, total_messages: 0, + last_delivered_nonce: 0, }, )) } diff --git a/bridges/relays/messages/src/message_race_delivery.rs b/bridges/relays/messages/src/message_race_delivery.rs index dc994364f178..4484d4b7994c 100644 --- a/bridges/relays/messages/src/message_race_delivery.rs +++ b/bridges/relays/messages/src/message_race_delivery.rs @@ -639,6 +639,7 @@ mod tests { unrewarded_relayer_entries: 0, messages_in_oldest_entry: 0, total_messages: 0, + last_delivered_nonce: 0, }, }, }), @@ -954,6 +955,7 @@ mod tests { unrewarded_relayer_entries: 2, messages_in_oldest_entry: 2, total_messages: 2, + last_delivered_nonce: 19, }, }, },