From 5c1ce7dd906500d74c1ca2fd816aab361632b9b7 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 11 May 2021 11:15:16 +0300 Subject: [PATCH 01/10] pass call origin to the message verifier --- Cargo.lock | 2 +- bin/millau/runtime/src/rialto_messages.rs | 1 + bin/rialto/runtime/src/millau_messages.rs | 1 + bin/runtime-common/Cargo.toml | 2 + bin/runtime-common/src/messages.rs | 66 ++++++++++++++++++----- modules/messages/src/instant_payments.rs | 20 +++---- modules/messages/src/lib.rs | 27 ++++++---- modules/messages/src/mock.rs | 16 +++--- primitives/messages/Cargo.toml | 3 +- primitives/messages/src/source_chain.rs | 21 ++++---- 10 files changed, 104 insertions(+), 55 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 454c537e17..45e60bbb1c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -723,7 +723,6 @@ dependencies = [ "bitvec", "bp-runtime", "frame-support", - "frame-system", "impl-trait-for-tuples", "parity-scale-codec", "serde", @@ -879,6 +878,7 @@ dependencies = [ "bp-runtime", "ed25519-dalek", "frame-support", + "frame-system", "hash-db", "pallet-bridge-dispatch", "pallet-bridge-grandpa", diff --git a/bin/millau/runtime/src/rialto_messages.rs b/bin/millau/runtime/src/rialto_messages.rs index 12af2c3285..c98f34c12b 100644 --- a/bin/millau/runtime/src/rialto_messages.rs +++ b/bin/millau/runtime/src/rialto_messages.rs @@ -101,6 +101,7 @@ impl messages::ChainWithMessages for Millau { } impl messages::ThisChainWithMessages for Millau { + type Origin = crate::Origin; type Call = crate::Call; fn is_outbound_lane_enabled(lane: &LaneId) -> bool { diff --git a/bin/rialto/runtime/src/millau_messages.rs b/bin/rialto/runtime/src/millau_messages.rs index bf97478a0a..ad565ef61a 100644 --- a/bin/rialto/runtime/src/millau_messages.rs +++ b/bin/rialto/runtime/src/millau_messages.rs @@ -101,6 +101,7 @@ impl messages::ChainWithMessages for Rialto { } impl messages::ThisChainWithMessages for Rialto { + type Origin = crate::Origin; type Call = crate::Call; fn is_outbound_lane_enabled(lane: &LaneId) -> bool { diff --git a/bin/runtime-common/Cargo.toml b/bin/runtime-common/Cargo.toml index 07fe8910c2..db65a33f41 100644 --- a/bin/runtime-common/Cargo.toml +++ b/bin/runtime-common/Cargo.toml @@ -24,6 +24,7 @@ pallet-bridge-messages = { path = "../../modules/messages", default-features = f # Substrate dependencies frame-support = { git = "https://github.com/paritytech/substrate", branch = "master" , default-features = false } +frame-system = { git = "https://github.com/paritytech/substrate", branch = "master" , default-features = false } pallet-transaction-payment = { git = "https://github.com/paritytech/substrate", branch = "master" , default-features = false } sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" , default-features = false } sp-runtime = { git = "https://github.com/paritytech/substrate", branch = "master" , default-features = false } @@ -39,6 +40,7 @@ std = [ "bp-runtime/std", "codec/std", "frame-support/std", + "frame-system/std", "hash-db/std", "pallet-bridge-dispatch/std", "pallet-bridge-grandpa/std", diff --git a/bin/runtime-common/src/messages.rs b/bin/runtime-common/src/messages.rs index 72249e4f4e..c1ba4b3e2b 100644 --- a/bin/runtime-common/src/messages.rs +++ b/bin/runtime-common/src/messages.rs @@ -22,7 +22,7 @@ use bp_message_dispatch::MessageDispatch as _; use bp_messages::{ - source_chain::{LaneMessageVerifier, Sender}, + source_chain::LaneMessageVerifier, target_chain::{DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages}, InboundLaneData, LaneId, Message, MessageData, MessageKey, MessageNonce, OutboundLaneData, }; @@ -95,6 +95,8 @@ pub struct MessageTransaction { /// This chain that has `pallet-bridge-messages` and `dispatch` modules. pub trait ThisChainWithMessages: ChainWithMessages { + /// Call origin on the chain. + type Origin; /// Call type on the chain. type Call: Encode + Decode; @@ -149,6 +151,7 @@ pub(crate) type SignatureOf = ::Signature; pub(crate) type WeightOf = ::Weight; pub(crate) type BalanceOf = ::Balance; +pub(crate) type OriginOf = ::Origin; pub(crate) type CallOf = ::Call; /// Raw storage proof type (just raw trie nodes). @@ -244,17 +247,26 @@ pub mod source { pub(crate) const TOO_MANY_PENDING_MESSAGES: &str = "Too many pending messages at the lane."; pub(crate) const BAD_ORIGIN: &str = "Unable to match the source origin to expected target origin."; pub(crate) const TOO_LOW_FEE: &str = "Provided fee is below minimal threshold required by the lane."; + pub(crate) const INVALID_SUBMITTER_ORIGIN: &str = "The submitter origin is invalid."; - impl LaneMessageVerifier>, FromThisChainMessagePayload, BalanceOf>> - for FromThisChainMessageVerifier + impl + LaneMessageVerifier< + OriginOf>, + AccountIdOf>, + FromThisChainMessagePayload, + BalanceOf>, + > for FromThisChainMessageVerifier where B: MessageBridge, + // matches requirements from the `frame_system::Config::Origin` + OriginOf>: + Clone + Into>>, OriginOf>>>, AccountIdOf>: PartialEq + Clone, { type Error = &'static str; fn verify_message( - submitter: &Sender>>, + submitter: &OriginOf>, delivery_and_dispatch_fee: &BalanceOf>, lane: &LaneId, lane_outbound_data: &OutboundLaneData, @@ -276,7 +288,13 @@ pub mod source { // Do the dispatch-specific check. We assume that the target chain uses // `Dispatch`, so we verify the message accordingly. - pallet_bridge_dispatch::verify_message_origin(submitter, payload).map_err(|_| BAD_ORIGIN)?; + let raw_origin_or_err: Result>>, OriginOf>> = + submitter.clone().into(); + pallet_bridge_dispatch::verify_message_origin( + &raw_origin_or_err.map_err(|_| INVALID_SUBMITTER_ORIGIN)?, + payload, + ) + .map_err(|_| BAD_ORIGIN)?; let minimal_fee_in_this_tokens = estimate_message_dispatch_and_delivery_fee::(payload, B::RELAYER_FEE_PERCENT)?; @@ -781,6 +799,14 @@ mod tests { #[codec(index = 84)] Mint, } + #[derive(Clone, Debug)] + struct ThisChainOrigin(Result, ()>); + + impl From for Result, ThisChainOrigin> { + fn from(origin: ThisChainOrigin) -> Result, ThisChainOrigin> { + origin.clone().0.map_err(|_| origin) + } + } #[derive(Debug, PartialEq, Decode, Encode)] struct BridgedChainAccountId(u32); @@ -790,6 +816,16 @@ mod tests { struct BridgedChainSignature(u32); #[derive(Debug, PartialEq, Decode, Encode)] enum BridgedChainCall {} + #[derive(Clone, Debug)] + struct BridgedChainOrigin; + + impl From for Result, BridgedChainOrigin> { + fn from( + _origin: BridgedChainOrigin, + ) -> Result, BridgedChainOrigin> { + unreachable!() + } + } macro_rules! impl_wrapped_balance { ($name:ident) => { @@ -867,6 +903,7 @@ mod tests { } impl ThisChainWithMessages for ThisChain { + type Origin = ThisChainOrigin; type Call = ThisChainCall; fn is_outbound_lane_enabled(lane: &LaneId) -> bool { @@ -923,6 +960,7 @@ mod tests { } impl ThisChainWithMessages for BridgedChain { + type Origin = BridgedChainOrigin; type Call = BridgedChainCall; fn is_outbound_lane_enabled(_lane: &LaneId) -> bool { @@ -1050,7 +1088,7 @@ mod tests { // and now check that the verifier checks the fee assert_eq!( source::FromThisChainMessageVerifier::::verify_message( - &Sender::Root, + &ThisChainOrigin(Ok(frame_system::RawOrigin::Root)), &ThisChainBalance(1), TEST_LANE_ID, &test_lane_outbound_data(), @@ -1060,7 +1098,7 @@ mod tests { ); assert!( source::FromThisChainMessageVerifier::::verify_message( - &Sender::Root, + &ThisChainOrigin(Ok(frame_system::RawOrigin::Root)), &ThisChainBalance(1_000_000), TEST_LANE_ID, &test_lane_outbound_data(), @@ -1084,7 +1122,7 @@ mod tests { // and now check that the verifier checks the fee assert_eq!( source::FromThisChainMessageVerifier::::verify_message( - &Sender::Signed(ThisChainAccountId(0)), + &ThisChainOrigin(Ok(frame_system::RawOrigin::Signed(ThisChainAccountId(0)))), &ThisChainBalance(1_000_000), TEST_LANE_ID, &test_lane_outbound_data(), @@ -1094,7 +1132,7 @@ mod tests { ); assert_eq!( source::FromThisChainMessageVerifier::::verify_message( - &Sender::None, + &ThisChainOrigin(Ok(frame_system::RawOrigin::None)), &ThisChainBalance(1_000_000), TEST_LANE_ID, &test_lane_outbound_data(), @@ -1104,7 +1142,7 @@ mod tests { ); assert!( source::FromThisChainMessageVerifier::::verify_message( - &Sender::Root, + &ThisChainOrigin(Ok(frame_system::RawOrigin::Root)), &ThisChainBalance(1_000_000), TEST_LANE_ID, &test_lane_outbound_data(), @@ -1128,7 +1166,7 @@ mod tests { // and now check that the verifier checks the fee assert_eq!( source::FromThisChainMessageVerifier::::verify_message( - &Sender::Signed(ThisChainAccountId(0)), + &ThisChainOrigin(Ok(frame_system::RawOrigin::Signed(ThisChainAccountId(0)))), &ThisChainBalance(1_000_000), TEST_LANE_ID, &test_lane_outbound_data(), @@ -1138,7 +1176,7 @@ mod tests { ); assert!( source::FromThisChainMessageVerifier::::verify_message( - &Sender::Signed(ThisChainAccountId(1)), + &ThisChainOrigin(Ok(frame_system::RawOrigin::Signed(ThisChainAccountId(1)))), &ThisChainBalance(1_000_000), TEST_LANE_ID, &test_lane_outbound_data(), @@ -1152,7 +1190,7 @@ mod tests { fn message_is_rejected_when_sent_using_disabled_lane() { assert_eq!( source::FromThisChainMessageVerifier::::verify_message( - &Sender::Root, + &ThisChainOrigin(Ok(frame_system::RawOrigin::Root)), &ThisChainBalance(1_000_000), b"dsbl", &test_lane_outbound_data(), @@ -1166,7 +1204,7 @@ mod tests { fn message_is_rejected_when_there_are_too_many_pending_messages_at_outbound_lane() { assert_eq!( source::FromThisChainMessageVerifier::::verify_message( - &Sender::Root, + &ThisChainOrigin(Ok(frame_system::RawOrigin::Root)), &ThisChainBalance(1_000_000), TEST_LANE_ID, &OutboundLaneData { diff --git a/modules/messages/src/instant_payments.rs b/modules/messages/src/instant_payments.rs index 524a3765d6..057bcfbde6 100644 --- a/modules/messages/src/instant_payments.rs +++ b/modules/messages/src/instant_payments.rs @@ -20,7 +20,7 @@ //! to the actual relayer in case confirmation is received. use bp_messages::{ - source_chain::{MessageDeliveryAndDispatchPayment, RelayersRewards, Sender}, + source_chain::{MessageDeliveryAndDispatchPayment, RelayersRewards}, MessageNonce, }; use codec::Encode; @@ -46,7 +46,8 @@ pub struct InstantCurrencyPayments _phantom: sp_std::marker::PhantomData<(T, Currency, GetConfirmationFee, RootAccount)>, } -impl MessageDeliveryAndDispatchPayment +impl + MessageDeliveryAndDispatchPayment for InstantCurrencyPayments where T: frame_system::Config, @@ -67,20 +68,21 @@ where } fn pay_delivery_and_dispatch_fee( - submitter: &Sender, + submitter: &T::Origin, fee: &Currency::Balance, relayer_fund_account: &T::AccountId, ) -> Result<(), Self::Error> { let root_account = RootAccount::get(); - let account = match submitter { - Sender::Signed(submitter) => submitter, - Sender::Root | Sender::None => root_account - .as_ref() - .ok_or("Sending messages using Root or None origin is disallowed.")?, + let submitter_account = match submitter.clone().into() { + Ok(frame_system::RawOrigin::Signed(submitter)) => submitter, + Ok(frame_system::RawOrigin::Root) | Ok(frame_system::RawOrigin::None) => { + root_account.ok_or("Sending messages using Root or None origin is disallowed.")? + } + Err(_) => return Err("Invalid message sender origin"), }; Currency::transfer( - account, + &submitter_account, relayer_fund_account, *fee, // it's fine for the submitter to go below Existential Deposit and die. diff --git a/modules/messages/src/lib.rs b/modules/messages/src/lib.rs index a8071e9922..28d1aefcbe 100644 --- a/modules/messages/src/lib.rs +++ b/modules/messages/src/lib.rs @@ -145,9 +145,18 @@ pub trait Config: frame_system::Config { /// Target header chain. type TargetHeaderChain: TargetHeaderChain; /// Message payload verifier. - type LaneMessageVerifier: LaneMessageVerifier; + type LaneMessageVerifier: LaneMessageVerifier< + Self::Origin, + Self::AccountId, + Self::OutboundPayload, + Self::OutboundMessageFee, + >; /// Message delivery payment. - type MessageDeliveryAndDispatchPayment: MessageDeliveryAndDispatchPayment; + type MessageDeliveryAndDispatchPayment: MessageDeliveryAndDispatchPayment< + Self::Origin, + Self::AccountId, + Self::OutboundMessageFee, + >; /// Handler for delivered messages. type OnDeliveryConfirmed: OnDeliveryConfirmed; @@ -318,7 +327,6 @@ decl_module! { delivery_and_dispatch_fee: T::OutboundMessageFee, ) -> DispatchResult { ensure_normal_operating_mode::()?; - let submitter = origin.into().map_err(|_| BadOrigin)?; // let's first check if message can be delivered to target chain T::TargetHeaderChain::verify_message(&payload) @@ -336,7 +344,7 @@ decl_module! { // now let's enforce any additional lane rules let mut lane = outbound_lane::(lane_id); T::LaneMessageVerifier::verify_message( - &submitter, + &origin, &delivery_and_dispatch_fee, &lane_id, &lane.data(), @@ -354,15 +362,14 @@ decl_module! { // let's withdraw delivery and dispatch fee from submitter T::MessageDeliveryAndDispatchPayment::pay_delivery_and_dispatch_fee( - &submitter, + &origin, &delivery_and_dispatch_fee, &Self::relayer_fund_account_id(), ).map_err(|err| { log::trace!( target: "runtime::bridge-messages", - "Message to lane {:?} is rejected because submitter {:?} is unable to pay fee {:?}: {:?}", + "Message to lane {:?} is rejected because submitter is unable to pay fee {:?}: {:?}", lane_id, - submitter, delivery_and_dispatch_fee, err, ); @@ -411,16 +418,14 @@ decl_module! { ensure!(nonce <= lane.data().latest_generated_nonce, Error::::MessageIsNotYetSent); // withdraw additional fee from submitter - let submitter = origin.into().map_err(|_| BadOrigin)?; T::MessageDeliveryAndDispatchPayment::pay_delivery_and_dispatch_fee( - &submitter, + &origin, &additional_fee, &Self::relayer_fund_account_id(), ).map_err(|err| { log::trace!( target: "runtime::bridge-messages", - "Submitter {:?} can't pay additional fee {:?} for the message {:?}/{:?}: {:?}", - submitter, + "Submitter can't pay additional fee {:?} for the message {:?}/{:?}: {:?}", additional_fee, lane_id, nonce, diff --git a/modules/messages/src/mock.rs b/modules/messages/src/mock.rs index 9d361b53cf..deccf32d6a 100644 --- a/modules/messages/src/mock.rs +++ b/modules/messages/src/mock.rs @@ -22,7 +22,7 @@ use crate::Config; use bitvec::prelude::*; use bp_messages::{ source_chain::{ - LaneMessageVerifier, MessageDeliveryAndDispatchPayment, OnDeliveryConfirmed, RelayersRewards, Sender, + LaneMessageVerifier, MessageDeliveryAndDispatchPayment, OnDeliveryConfirmed, RelayersRewards, TargetHeaderChain, }, target_chain::{DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages, SourceHeaderChain}, @@ -283,11 +283,11 @@ impl TargetHeaderChain for TestTargetHeaderChain { #[derive(Debug, Default)] pub struct TestLaneMessageVerifier; -impl LaneMessageVerifier for TestLaneMessageVerifier { +impl LaneMessageVerifier for TestLaneMessageVerifier { type Error = &'static str; fn verify_message( - _submitter: &Sender, + _submitter: &Origin, delivery_and_dispatch_fee: &TestMessageFee, _lane: &LaneId, _lane_outbound_data: &OutboundLaneData, @@ -313,7 +313,8 @@ impl TestMessageDeliveryAndDispatchPayment { /// Returns true if given fee has been paid by given submitter. pub fn is_fee_paid(submitter: AccountId, fee: TestMessageFee) -> bool { - frame_support::storage::unhashed::get(b":message-fee:") == Some((Sender::Signed(submitter), fee)) + let raw_origin: Result, _> = Origin::signed(submitter).into(); + frame_support::storage::unhashed::get(b":message-fee:") == Some((raw_origin.unwrap(), fee)) } /// Returns true if given relayer has been rewarded with given balance. The reward-paid flag is @@ -324,11 +325,11 @@ impl TestMessageDeliveryAndDispatchPayment { } } -impl MessageDeliveryAndDispatchPayment for TestMessageDeliveryAndDispatchPayment { +impl MessageDeliveryAndDispatchPayment for TestMessageDeliveryAndDispatchPayment { type Error = &'static str; fn pay_delivery_and_dispatch_fee( - submitter: &Sender, + submitter: &Origin, fee: &TestMessageFee, _relayer_fund_account: &AccountId, ) -> Result<(), Self::Error> { @@ -336,7 +337,8 @@ impl MessageDeliveryAndDispatchPayment for TestMessag return Err(TEST_ERROR); } - frame_support::storage::unhashed::put(b":message-fee:", &(submitter, fee)); + let raw_origin: Result, _> = submitter.clone().into(); + frame_support::storage::unhashed::put(b":message-fee:", &(raw_origin.unwrap(), fee)); Ok(()) } diff --git a/primitives/messages/Cargo.toml b/primitives/messages/Cargo.toml index b5b68220a4..930850bf72 100644 --- a/primitives/messages/Cargo.toml +++ b/primitives/messages/Cargo.toml @@ -19,16 +19,15 @@ bp-runtime = { path = "../runtime", default-features = false } # Substrate Dependencies frame-support = { git = "https://github.com/paritytech/substrate", branch = "master" , default-features = false } -frame-system = { git = "https://github.com/paritytech/substrate", branch = "master" , default-features = false } sp-std = { git = "https://github.com/paritytech/substrate", branch = "master" , default-features = false } [features] default = ["std"] std = [ + "bitvec/std", "bp-runtime/std", "codec/std", "frame-support/std", - "frame-system/std", "serde", "sp-std/std" ] diff --git a/primitives/messages/src/source_chain.rs b/primitives/messages/src/source_chain.rs index ed69f39c76..b2002bbc8b 100644 --- a/primitives/messages/src/source_chain.rs +++ b/primitives/messages/src/source_chain.rs @@ -22,9 +22,6 @@ use bp_runtime::Size; use frame_support::{weights::Weight, Parameter, RuntimeDebug}; use sp_std::{collections::btree_map::BTreeMap, fmt::Debug}; -/// The sender of the message on the source chain. -pub type Sender = frame_system::RawOrigin; - /// Relayers rewards, grouped by relayer account id. pub type RelayersRewards = BTreeMap>; @@ -77,13 +74,13 @@ pub trait TargetHeaderChain { /// Lane3 until some block, ...), then it may be built using this verifier. /// /// Any fee requirements should also be enforced here. -pub trait LaneMessageVerifier { +pub trait LaneMessageVerifier { /// Error type. type Error: Debug + Into<&'static str>; /// Verify message payload and return Ok(()) if message is valid and allowed to be sent over the lane. fn verify_message( - submitter: &Sender, + submitter: &Origin, delivery_and_dispatch_fee: &Fee, lane: &LaneId, outbound_data: &OutboundLaneData, @@ -104,14 +101,14 @@ pub trait LaneMessageVerifier { /// So to be sure that any non-altruist relayer would agree to deliver message, submitter /// should set `delivery_and_dispatch_fee` to at least (equialent of): sum of fees from (2) /// to (4) above, plus some interest for the relayer. -pub trait MessageDeliveryAndDispatchPayment { +pub trait MessageDeliveryAndDispatchPayment { /// Error type. type Error: Debug + Into<&'static str>; /// Withhold/write-off delivery_and_dispatch_fee from submitter account to /// some relayers-fund account. fn pay_delivery_and_dispatch_fee( - submitter: &Sender, + submitter: &Origin, fee: &Balance, relayer_fund_account: &AccountId, ) -> Result<(), Self::Error>; @@ -188,11 +185,11 @@ impl TargetHeaderChain for ForbidOutboun } } -impl LaneMessageVerifier for ForbidOutboundMessages { +impl LaneMessageVerifier for ForbidOutboundMessages { type Error = &'static str; fn verify_message( - _submitter: &Sender, + _submitter: &Origin, _delivery_and_dispatch_fee: &Fee, _lane: &LaneId, _outbound_data: &OutboundLaneData, @@ -202,11 +199,13 @@ impl LaneMessageVerifier for F } } -impl MessageDeliveryAndDispatchPayment for ForbidOutboundMessages { +impl MessageDeliveryAndDispatchPayment + for ForbidOutboundMessages +{ type Error = &'static str; fn pay_delivery_and_dispatch_fee( - _submitter: &Sender, + _submitter: &Origin, _fee: &Balance, _relayer_fund_account: &AccountId, ) -> Result<(), Self::Error> { From 83bdde1bf92f10e7136f47fef9418a4f9470802a Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 11 May 2021 11:27:57 +0300 Subject: [PATCH 02/10] is_outbound_lane_enabled -> is_message_accepted --- bin/millau/runtime/src/rialto_messages.rs | 2 +- bin/rialto/runtime/src/millau_messages.rs | 2 +- bin/runtime-common/README.md | 5 +++-- bin/runtime-common/src/messages.rs | 16 ++++++++-------- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/bin/millau/runtime/src/rialto_messages.rs b/bin/millau/runtime/src/rialto_messages.rs index c98f34c12b..98fe27ff03 100644 --- a/bin/millau/runtime/src/rialto_messages.rs +++ b/bin/millau/runtime/src/rialto_messages.rs @@ -104,7 +104,7 @@ impl messages::ThisChainWithMessages for Millau { type Origin = crate::Origin; type Call = crate::Call; - fn is_outbound_lane_enabled(lane: &LaneId) -> bool { + fn is_message_accepted(_send_origin: &Self::Origin, lane: &LaneId) -> bool { *lane == [0, 0, 0, 0] || *lane == [0, 0, 0, 1] } diff --git a/bin/rialto/runtime/src/millau_messages.rs b/bin/rialto/runtime/src/millau_messages.rs index ad565ef61a..47107d9aa6 100644 --- a/bin/rialto/runtime/src/millau_messages.rs +++ b/bin/rialto/runtime/src/millau_messages.rs @@ -104,7 +104,7 @@ impl messages::ThisChainWithMessages for Rialto { type Origin = crate::Origin; type Call = crate::Call; - fn is_outbound_lane_enabled(lane: &LaneId) -> bool { + fn is_message_accepted(_send_origin: &Self::Origin, lane: &LaneId) -> bool { *lane == [0, 0, 0, 0] || *lane == [0, 0, 0, 1] } diff --git a/bin/runtime-common/README.md b/bin/runtime-common/README.md index 38a47bfdcc..dcf748218f 100644 --- a/bin/runtime-common/README.md +++ b/bin/runtime-common/README.md @@ -62,8 +62,9 @@ corresponding chain. There is single exception, though (it may be changed in the This trait represents this chain from bridge point of view. Let's review every method of this trait: -- `ThisChainWithMessages::is_outbound_lane_enabled`: is used to check whether given lane accepts - outbound messages. +- `ThisChainWithMessages::is_message_accepted`: is used to check whether given lane accepts + messages. The send-message origin is passed to the function, so you may e.g. verify that only + given pallet is able to send messages over selected lane. - `ThisChainWithMessages::maximal_pending_messages_at_outbound_lane`: you should return maximal number of pending (undelivered) messages from this function. Returning small values would require diff --git a/bin/runtime-common/src/messages.rs b/bin/runtime-common/src/messages.rs index c1ba4b3e2b..994fe4a283 100644 --- a/bin/runtime-common/src/messages.rs +++ b/bin/runtime-common/src/messages.rs @@ -100,8 +100,8 @@ pub trait ThisChainWithMessages: ChainWithMessages { /// Call type on the chain. type Call: Encode + Decode; - /// Are we accepting any messages to the given lane? - fn is_outbound_lane_enabled(lane: &LaneId) -> bool; + /// Do we accept message sent by given origin to given lane? + fn is_message_accepted(origin: &Self::Origin, lane: &LaneId) -> bool; /// Maximal number of pending (not yet delivered) messages at This chain. /// @@ -243,7 +243,7 @@ pub mod source { #[derive(RuntimeDebug)] pub struct FromThisChainMessageVerifier(PhantomData); - pub(crate) const OUTBOUND_LANE_DISABLED: &str = "The outbound message lane is disabled."; + pub(crate) const MESSAGE_REJECTED_BY_OUTBOUND_LANE: &str = "The outbound message lane has rejected the message."; pub(crate) const TOO_MANY_PENDING_MESSAGES: &str = "Too many pending messages at the lane."; pub(crate) const BAD_ORIGIN: &str = "Unable to match the source origin to expected target origin."; pub(crate) const TOO_LOW_FEE: &str = "Provided fee is below minimal threshold required by the lane."; @@ -273,8 +273,8 @@ pub mod source { payload: &FromThisChainMessagePayload, ) -> Result<(), Self::Error> { // reject message if lane is blocked - if !ThisChain::::is_outbound_lane_enabled(lane) { - return Err(OUTBOUND_LANE_DISABLED); + if !ThisChain::::is_message_accepted(submitter, lane) { + return Err(MESSAGE_REJECTED_BY_OUTBOUND_LANE); } // reject message if there are too many pending messages at this lane @@ -906,7 +906,7 @@ mod tests { type Origin = ThisChainOrigin; type Call = ThisChainCall; - fn is_outbound_lane_enabled(lane: &LaneId) -> bool { + fn is_message_accepted(_send_origin: &Self::Origin, lane: &LaneId) -> bool { lane == TEST_LANE_ID } @@ -963,7 +963,7 @@ mod tests { type Origin = BridgedChainOrigin; type Call = BridgedChainCall; - fn is_outbound_lane_enabled(_lane: &LaneId) -> bool { + fn is_message_accepted(_send_origin: &Self::Origin, _lane: &LaneId) -> bool { unreachable!() } @@ -1196,7 +1196,7 @@ mod tests { &test_lane_outbound_data(), ®ular_outbound_message_payload(), ), - Err(source::OUTBOUND_LANE_DISABLED) + Err(source::MESSAGE_REJECTED_BY_OUTBOUND_LANE) ); } From e641a1ff04f5f624412d0b5754f780b296acb8a1 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 11 May 2021 12:50:48 +0300 Subject: [PATCH 03/10] trait SenderOrigin --- bin/millau/runtime/src/lib.rs | 3 +- bin/millau/runtime/src/rialto_messages.rs | 17 ++++++++++-- bin/rialto/runtime/src/lib.rs | 3 +- bin/rialto/runtime/src/millau_messages.rs | 17 ++++++++++-- bin/runtime-common/README.md | 4 ++- modules/messages/src/instant_payments.rs | 30 ++++++++++---------- modules/messages/src/lib.rs | 12 +++++--- modules/messages/src/mock.rs | 12 +++++++- primitives/messages/src/source_chain.rs | 34 +++++++++++++++++------ 9 files changed, 95 insertions(+), 37 deletions(-) diff --git a/bin/millau/runtime/src/lib.rs b/bin/millau/runtime/src/lib.rs index 4913043373..5b0308fb0c 100644 --- a/bin/millau/runtime/src/lib.rs +++ b/bin/millau/runtime/src/lib.rs @@ -376,13 +376,14 @@ impl pallet_bridge_messages::Config for Runtime { type AccountIdConverter = bp_millau::AccountIdConverter; + type SenderOrigin = Origin; type TargetHeaderChain = crate::rialto_messages::Rialto; type LaneMessageVerifier = crate::rialto_messages::ToRialtoMessageVerifier; type MessageDeliveryAndDispatchPayment = pallet_bridge_messages::instant_payments::InstantCurrencyPayments< Runtime, + WithRialtoMessagesInstance, pallet_balances::Pallet, GetDeliveryConfirmationTransactionFee, - RootAccountForPayments, >; type OnDeliveryConfirmed = (); diff --git a/bin/millau/runtime/src/rialto_messages.rs b/bin/millau/runtime/src/rialto_messages.rs index 98fe27ff03..7d8774f4c8 100644 --- a/bin/millau/runtime/src/rialto_messages.rs +++ b/bin/millau/runtime/src/rialto_messages.rs @@ -19,7 +19,7 @@ use crate::Runtime; use bp_messages::{ - source_chain::TargetHeaderChain, + source_chain::{SenderOrigin, TargetHeaderChain}, target_chain::{ProvedMessages, SourceHeaderChain}, InboundLaneData, LaneId, Message, MessageNonce, Parameter as MessagesParameter, }; @@ -104,8 +104,8 @@ impl messages::ThisChainWithMessages for Millau { type Origin = crate::Origin; type Call = crate::Call; - fn is_message_accepted(_send_origin: &Self::Origin, lane: &LaneId) -> bool { - *lane == [0, 0, 0, 0] || *lane == [0, 0, 0, 1] + fn is_message_accepted(send_origin: &Self::Origin, lane: &LaneId) -> bool { + send_origin.linked_account().is_some() && (*lane == [0, 0, 0, 0] || *lane == [0, 0, 0, 1]) } fn maximal_pending_messages_at_outbound_lane() -> MessageNonce { @@ -248,6 +248,17 @@ impl SourceHeaderChain for Rialto { } } +impl SenderOrigin for crate::Origin { + fn linked_account(&self) -> Option { + match self.caller { + crate::OriginCaller::system(frame_system::RawOrigin::Signed(ref submitter)) => Some(submitter.clone()), + crate::OriginCaller::system(frame_system::RawOrigin::Root) + | crate::OriginCaller::system(frame_system::RawOrigin::None) => crate::RootAccountForPayments::get(), + _ => None, + } + } +} + /// Millau -> Rialto message lane pallet parameters. #[derive(RuntimeDebug, Clone, Encode, Decode, PartialEq, Eq)] pub enum MillauToRialtoMessagesParameter { diff --git a/bin/rialto/runtime/src/lib.rs b/bin/rialto/runtime/src/lib.rs index e53beed389..eb2f14fedb 100644 --- a/bin/rialto/runtime/src/lib.rs +++ b/bin/rialto/runtime/src/lib.rs @@ -483,13 +483,14 @@ impl pallet_bridge_messages::Config for Runtime { type AccountIdConverter = bp_rialto::AccountIdConverter; + type SenderOrigin = Origin; type TargetHeaderChain = crate::millau_messages::Millau; type LaneMessageVerifier = crate::millau_messages::ToMillauMessageVerifier; type MessageDeliveryAndDispatchPayment = pallet_bridge_messages::instant_payments::InstantCurrencyPayments< Runtime, + WithMillauMessagesInstance, pallet_balances::Pallet, GetDeliveryConfirmationTransactionFee, - RootAccountForPayments, >; type OnDeliveryConfirmed = (); diff --git a/bin/rialto/runtime/src/millau_messages.rs b/bin/rialto/runtime/src/millau_messages.rs index 47107d9aa6..6fd72a7a48 100644 --- a/bin/rialto/runtime/src/millau_messages.rs +++ b/bin/rialto/runtime/src/millau_messages.rs @@ -19,7 +19,7 @@ use crate::Runtime; use bp_messages::{ - source_chain::TargetHeaderChain, + source_chain::{SenderOrigin, TargetHeaderChain}, target_chain::{ProvedMessages, SourceHeaderChain}, InboundLaneData, LaneId, Message, MessageNonce, Parameter as MessagesParameter, }; @@ -104,8 +104,8 @@ impl messages::ThisChainWithMessages for Rialto { type Origin = crate::Origin; type Call = crate::Call; - fn is_message_accepted(_send_origin: &Self::Origin, lane: &LaneId) -> bool { - *lane == [0, 0, 0, 0] || *lane == [0, 0, 0, 1] + fn is_message_accepted(send_origin: &Self::Origin, lane: &LaneId) -> bool { + send_origin.linked_account().is_some() && (*lane == [0, 0, 0, 0] || *lane == [0, 0, 0, 1]) } fn maximal_pending_messages_at_outbound_lane() -> MessageNonce { @@ -248,6 +248,17 @@ impl SourceHeaderChain for Millau { } } +impl SenderOrigin for crate::Origin { + fn linked_account(&self) -> Option { + match self.caller { + crate::OriginCaller::system(frame_system::RawOrigin::Signed(ref submitter)) => Some(submitter.clone()), + crate::OriginCaller::system(frame_system::RawOrigin::Root) + | crate::OriginCaller::system(frame_system::RawOrigin::None) => crate::RootAccountForPayments::get(), + _ => None, + } + } +} + /// Rialto -> Millau message lane pallet parameters. #[derive(RuntimeDebug, Clone, Encode, Decode, PartialEq, Eq)] pub enum RialtoToMillauMessagesParameter { diff --git a/bin/runtime-common/README.md b/bin/runtime-common/README.md index dcf748218f..5f2298cd78 100644 --- a/bin/runtime-common/README.md +++ b/bin/runtime-common/README.md @@ -64,7 +64,9 @@ This trait represents this chain from bridge point of view. Let's review every m - `ThisChainWithMessages::is_message_accepted`: is used to check whether given lane accepts messages. The send-message origin is passed to the function, so you may e.g. verify that only - given pallet is able to send messages over selected lane. + given pallet is able to send messages over selected lane. **IMPORTANT**: if you assume that the + message must be paid by the sender, you must ensure that the sender origin has linked the account + for paying message delivery and dispatch fee. - `ThisChainWithMessages::maximal_pending_messages_at_outbound_lane`: you should return maximal number of pending (undelivered) messages from this function. Returning small values would require diff --git a/modules/messages/src/instant_payments.rs b/modules/messages/src/instant_payments.rs index 057bcfbde6..7bf40f68ee 100644 --- a/modules/messages/src/instant_payments.rs +++ b/modules/messages/src/instant_payments.rs @@ -20,7 +20,7 @@ //! to the actual relayer in case confirmation is received. use bp_messages::{ - source_chain::{MessageDeliveryAndDispatchPayment, RelayersRewards}, + source_chain::{MessageDeliveryAndDispatchPayment, RelayersRewards, SenderOrigin}, MessageNonce, }; use codec::Encode; @@ -42,19 +42,18 @@ use sp_std::fmt::Debug; /// to the relayer account. /// NOTE It's within relayer's interest to keep their balance above ED as well, to make sure they /// can receive the payment. -pub struct InstantCurrencyPayments { - _phantom: sp_std::marker::PhantomData<(T, Currency, GetConfirmationFee, RootAccount)>, +pub struct InstantCurrencyPayments { + _phantom: sp_std::marker::PhantomData<(T, I, Currency, GetConfirmationFee)>, } -impl - MessageDeliveryAndDispatchPayment - for InstantCurrencyPayments +impl + MessageDeliveryAndDispatchPayment + for InstantCurrencyPayments where - T: frame_system::Config, + T: crate::Config, Currency: CurrencyT, Currency::Balance: From, GetConfirmationFee: Get, - RootAccount: Get>, { type Error = &'static str; @@ -68,17 +67,18 @@ where } fn pay_delivery_and_dispatch_fee( - submitter: &T::Origin, + submitter: &T::SenderOrigin, fee: &Currency::Balance, relayer_fund_account: &T::AccountId, ) -> Result<(), Self::Error> { - let root_account = RootAccount::get(); - let submitter_account = match submitter.clone().into() { - Ok(frame_system::RawOrigin::Signed(submitter)) => submitter, - Ok(frame_system::RawOrigin::Root) | Ok(frame_system::RawOrigin::None) => { - root_account.ok_or("Sending messages using Root or None origin is disallowed.")? + let submitter_account = match submitter.linked_account() { + Some(submitter_account) => submitter_account, + None => { + // message lane verifier has accepted the message before, so this message + // is unpaid **by design** + // => let's just do nothing + return Ok(()); } - Err(_) => return Err("Invalid message sender origin"), }; Currency::transfer( diff --git a/modules/messages/src/lib.rs b/modules/messages/src/lib.rs index 28d1aefcbe..ce6aa9cfb5 100644 --- a/modules/messages/src/lib.rs +++ b/modules/messages/src/lib.rs @@ -48,7 +48,8 @@ use crate::weights::WeightInfo; use bp_messages::{ source_chain::{ - LaneMessageVerifier, MessageDeliveryAndDispatchPayment, OnDeliveryConfirmed, RelayersRewards, TargetHeaderChain, + LaneMessageVerifier, MessageDeliveryAndDispatchPayment, OnDeliveryConfirmed, RelayersRewards, SenderOrigin, + TargetHeaderChain, }, target_chain::{DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages, SourceHeaderChain}, total_unrewarded_messages, DeliveredMessages, InboundLaneData, LaneId, MessageData, MessageKey, MessageNonce, @@ -142,18 +143,20 @@ pub trait Config: frame_system::Config { // Types that are used by outbound_lane (on source chain). + /// Message sender origin. + type SenderOrigin: SenderOrigin + From; /// Target header chain. type TargetHeaderChain: TargetHeaderChain; /// Message payload verifier. type LaneMessageVerifier: LaneMessageVerifier< - Self::Origin, + Self::SenderOrigin, Self::AccountId, Self::OutboundPayload, Self::OutboundMessageFee, >; /// Message delivery payment. type MessageDeliveryAndDispatchPayment: MessageDeliveryAndDispatchPayment< - Self::Origin, + Self::SenderOrigin, Self::AccountId, Self::OutboundMessageFee, >; @@ -342,6 +345,7 @@ decl_module! { })?; // now let's enforce any additional lane rules + let origin = T::SenderOrigin::from(origin); let mut lane = outbound_lane::(lane_id); T::LaneMessageVerifier::verify_message( &origin, @@ -419,7 +423,7 @@ decl_module! { // withdraw additional fee from submitter T::MessageDeliveryAndDispatchPayment::pay_delivery_and_dispatch_fee( - &origin, + &T::SenderOrigin::from(origin), &additional_fee, &Self::relayer_fund_account_id(), ).map_err(|err| { diff --git a/modules/messages/src/mock.rs b/modules/messages/src/mock.rs index deccf32d6a..94242416a6 100644 --- a/modules/messages/src/mock.rs +++ b/modules/messages/src/mock.rs @@ -22,7 +22,7 @@ use crate::Config; use bitvec::prelude::*; use bp_messages::{ source_chain::{ - LaneMessageVerifier, MessageDeliveryAndDispatchPayment, OnDeliveryConfirmed, RelayersRewards, + LaneMessageVerifier, MessageDeliveryAndDispatchPayment, OnDeliveryConfirmed, RelayersRewards, SenderOrigin, TargetHeaderChain, }, target_chain::{DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages, SourceHeaderChain}, @@ -172,6 +172,7 @@ impl Config for TestRuntime { type AccountIdConverter = AccountIdConverter; + type SenderOrigin = Origin; type TargetHeaderChain = TestTargetHeaderChain; type LaneMessageVerifier = TestLaneMessageVerifier; type MessageDeliveryAndDispatchPayment = TestMessageDeliveryAndDispatchPayment; @@ -181,6 +182,15 @@ impl Config for TestRuntime { type MessageDispatch = TestMessageDispatch; } +impl SenderOrigin for Origin { + fn linked_account(&self) -> Option { + match self.caller { + OriginCaller::system(frame_system::RawOrigin::Signed(ref submitter)) => Some(submitter.clone()), + _ => None, + } + } +} + impl Size for TestPayload { fn size_hint(&self) -> u32 { 16 diff --git a/primitives/messages/src/source_chain.rs b/primitives/messages/src/source_chain.rs index b2002bbc8b..73ae6bf11a 100644 --- a/primitives/messages/src/source_chain.rs +++ b/primitives/messages/src/source_chain.rs @@ -22,6 +22,22 @@ use bp_runtime::Size; use frame_support::{weights::Weight, Parameter, RuntimeDebug}; use sp_std::{collections::btree_map::BTreeMap, fmt::Debug}; +/// The sender of the message on the source chain. +pub trait SenderOrigin { + /// Return id of the account that is sending this message. + /// + /// In regular messages configuration, when regular message is sent you'll always get `Some(_)` from + /// this call. This is the account that is paying send costs. However, there are some examples when + /// `None` may be returned from the call: + /// + /// - if the send-message call origin is either `frame_system::RawOrigin::Root` or `frame_system::RawOrigin::None` + /// and your configuration forbids such messages; + /// - if your configuration allows 'unpaid' messages sent by pallets. Then the pallet may just use its + /// own defined origin (not linked to any account) and the message will be accepted. This may be useful for + /// pallets that are sending important system-wide information (like update of runtime version). + fn linked_account(&self) -> Option; +} + /// Relayers rewards, grouped by relayer account id. pub type RelayersRewards = BTreeMap>; @@ -74,13 +90,13 @@ pub trait TargetHeaderChain { /// Lane3 until some block, ...), then it may be built using this verifier. /// /// Any fee requirements should also be enforced here. -pub trait LaneMessageVerifier { +pub trait LaneMessageVerifier { /// Error type. type Error: Debug + Into<&'static str>; /// Verify message payload and return Ok(()) if message is valid and allowed to be sent over the lane. fn verify_message( - submitter: &Origin, + submitter: &SenderOrigin, delivery_and_dispatch_fee: &Fee, lane: &LaneId, outbound_data: &OutboundLaneData, @@ -101,14 +117,14 @@ pub trait LaneMessageVerifier { /// So to be sure that any non-altruist relayer would agree to deliver message, submitter /// should set `delivery_and_dispatch_fee` to at least (equialent of): sum of fees from (2) /// to (4) above, plus some interest for the relayer. -pub trait MessageDeliveryAndDispatchPayment { +pub trait MessageDeliveryAndDispatchPayment { /// Error type. type Error: Debug + Into<&'static str>; /// Withhold/write-off delivery_and_dispatch_fee from submitter account to /// some relayers-fund account. fn pay_delivery_and_dispatch_fee( - submitter: &Origin, + submitter: &SenderOrigin, fee: &Balance, relayer_fund_account: &AccountId, ) -> Result<(), Self::Error>; @@ -185,11 +201,13 @@ impl TargetHeaderChain for ForbidOutboun } } -impl LaneMessageVerifier for ForbidOutboundMessages { +impl LaneMessageVerifier + for ForbidOutboundMessages +{ type Error = &'static str; fn verify_message( - _submitter: &Origin, + _submitter: &SenderOrigin, _delivery_and_dispatch_fee: &Fee, _lane: &LaneId, _outbound_data: &OutboundLaneData, @@ -199,13 +217,13 @@ impl LaneMessageVerifier MessageDeliveryAndDispatchPayment +impl MessageDeliveryAndDispatchPayment for ForbidOutboundMessages { type Error = &'static str; fn pay_delivery_and_dispatch_fee( - _submitter: &Origin, + _submitter: &SenderOrigin, _fee: &Balance, _relayer_fund_account: &AccountId, ) -> Result<(), Self::Error> { From 290cc1bcc031ad8645f2cea7973be5c7977ca0f9 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 30 Sep 2021 13:12:34 +0300 Subject: [PATCH 04/10] only accept messages from token swap pallet to token swap lane --- bin/millau/runtime/src/rialto_messages.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/bin/millau/runtime/src/rialto_messages.rs b/bin/millau/runtime/src/rialto_messages.rs index 2b8151b5bd..4e96a19221 100644 --- a/bin/millau/runtime/src/rialto_messages.rs +++ b/bin/millau/runtime/src/rialto_messages.rs @@ -116,7 +116,17 @@ impl messages::ThisChainWithMessages for Millau { type Call = crate::Call; fn is_message_accepted(send_origin: &Self::Origin, lane: &LaneId) -> bool { - send_origin.linked_account().is_some() && (*lane == [0, 0, 0, 0] || *lane == [0, 0, 0, 1]) + // lanes 0x00000000 && 0x00000001 are accepting any paid messages, while `TokenSwapMessageLane` + // only accepts messages from token swap pallet + let token_swap_dedicated_lane = crate::TokenSwapMessagesLane::get(); + match *lane { + [0, 0, 0, 0] | [0, 0, 0, 1] => send_origin.linked_account().is_some(), + _ if *lane == token_swap_dedicated_lane => matches!( + send_origin.caller, + crate::OriginCaller::BridgeRialtoTokenSwap(pallet_bridge_token_swap::RawOrigin::TokenSwap {..}) + ), + _ => false, + } } fn maximal_pending_messages_at_outbound_lane() -> MessageNonce { @@ -280,6 +290,10 @@ impl SenderOrigin for crate::Origin { crate::OriginCaller::system(frame_system::RawOrigin::Root) | crate::OriginCaller::system(frame_system::RawOrigin::None) => crate::RootAccountForPayments::get(), + crate::OriginCaller::BridgeRialtoTokenSwap(pallet_bridge_token_swap::RawOrigin::TokenSwap { + ref swap_account_at_this_chain, + .. + }) => Some(swap_account_at_this_chain.clone()), _ => None, } } From 23fb1dda2974d9c840077fe9c24ca824664fc35b Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 30 Sep 2021 13:29:25 +0300 Subject: [PATCH 05/10] tests for edge cases of pay_delivery_and_dispatch_fee --- bin/millau/runtime/src/rialto_messages.rs | 18 +++++---- modules/messages/src/instant_payments.rs | 46 ++++++++++++++++++++++- 2 files changed, 56 insertions(+), 8 deletions(-) diff --git a/bin/millau/runtime/src/rialto_messages.rs b/bin/millau/runtime/src/rialto_messages.rs index 4e96a19221..574ed942ce 100644 --- a/bin/millau/runtime/src/rialto_messages.rs +++ b/bin/millau/runtime/src/rialto_messages.rs @@ -116,14 +116,16 @@ impl messages::ThisChainWithMessages for Millau { type Call = crate::Call; fn is_message_accepted(send_origin: &Self::Origin, lane: &LaneId) -> bool { - // lanes 0x00000000 && 0x00000001 are accepting any paid messages, while `TokenSwapMessageLane` - // only accepts messages from token swap pallet + // lanes 0x00000000 && 0x00000001 are accepting any paid messages, while + // `TokenSwapMessageLane` only accepts messages from token swap pallet let token_swap_dedicated_lane = crate::TokenSwapMessagesLane::get(); match *lane { [0, 0, 0, 0] | [0, 0, 0, 1] => send_origin.linked_account().is_some(), _ if *lane == token_swap_dedicated_lane => matches!( send_origin.caller, - crate::OriginCaller::BridgeRialtoTokenSwap(pallet_bridge_token_swap::RawOrigin::TokenSwap {..}) + crate::OriginCaller::BridgeRialtoTokenSwap( + pallet_bridge_token_swap::RawOrigin::TokenSwap { .. } + ) ), _ => false, } @@ -290,10 +292,12 @@ impl SenderOrigin for crate::Origin { crate::OriginCaller::system(frame_system::RawOrigin::Root) | crate::OriginCaller::system(frame_system::RawOrigin::None) => crate::RootAccountForPayments::get(), - crate::OriginCaller::BridgeRialtoTokenSwap(pallet_bridge_token_swap::RawOrigin::TokenSwap { - ref swap_account_at_this_chain, - .. - }) => Some(swap_account_at_this_chain.clone()), + crate::OriginCaller::BridgeRialtoTokenSwap( + pallet_bridge_token_swap::RawOrigin::TokenSwap { + ref swap_account_at_this_chain, + .. + }, + ) => Some(swap_account_at_this_chain.clone()), _ => None, } } diff --git a/modules/messages/src/instant_payments.rs b/modules/messages/src/instant_payments.rs index 2a4e28163f..c32321d930 100644 --- a/modules/messages/src/instant_payments.rs +++ b/modules/messages/src/instant_payments.rs @@ -234,7 +234,9 @@ fn pay_relayer_reward( #[cfg(test)] mod tests { use super::*; - use crate::mock::{run_test, AccountId as TestAccountId, Balance as TestBalance, TestRuntime}; + use crate::mock::{ + run_test, AccountId as TestAccountId, Balance as TestBalance, Origin, TestRuntime, + }; use bp_messages::source_chain::RelayerRewards; type Balances = pallet_balances::Pallet; @@ -253,6 +255,48 @@ mod tests { .collect() } + #[test] + fn pay_delivery_and_dispatch_fee_fails_on_non_zero_fee_and_unknown_payer() { + frame_support::parameter_types! { + const GetConfirmationFee: TestBalance = 0; + }; + + run_test(|| { + let result = InstantCurrencyPayments::< + TestRuntime, + (), + Balances, + GetConfirmationFee, + >::pay_delivery_and_dispatch_fee( + &Origin::root(), + &100, + &RELAYERS_FUND_ACCOUNT, + ); + assert!(result.is_err()); + }); + } + + #[test] + fn pay_delivery_and_dispatch_succeeds_on_zero_fee_and_unknown_payer() { + frame_support::parameter_types! { + const GetConfirmationFee: TestBalance = 0; + }; + + run_test(|| { + let result = InstantCurrencyPayments::< + TestRuntime, + (), + Balances, + GetConfirmationFee, + >::pay_delivery_and_dispatch_fee( + &Origin::root(), + &0, + &RELAYERS_FUND_ACCOUNT, + ); + assert!(result.is_ok()); + }); + } + #[test] fn confirmation_relayer_is_rewarded_if_it_has_also_delivered_messages() { run_test(|| { From 6d937dec88d5f4184530cc960eedfb36a30e4338 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 30 Sep 2021 15:13:31 +0300 Subject: [PATCH 06/10] fixed origin verification --- bin/runtime-common/src/messages.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/bin/runtime-common/src/messages.rs b/bin/runtime-common/src/messages.rs index 30147dd12d..1a865bb8b3 100644 --- a/bin/runtime-common/src/messages.rs +++ b/bin/runtime-common/src/messages.rs @@ -269,7 +269,6 @@ pub mod source { "Unable to match the source origin to expected target origin."; pub(crate) const TOO_LOW_FEE: &str = "Provided fee is below minimal threshold required by the lane."; - pub(crate) const INVALID_SUBMITTER_ORIGIN: &str = "The submitter origin is invalid."; impl LaneMessageVerifier< @@ -314,11 +313,20 @@ pub mod source { frame_system::RawOrigin>>, OriginOf>, > = submitter.clone().into(); - pallet_bridge_dispatch::verify_message_origin( - &raw_origin_or_err.map_err(|_| INVALID_SUBMITTER_ORIGIN)?, - payload, - ) - .map_err(|_| BAD_ORIGIN)?; + match raw_origin_or_err { + Ok(raw_origin) => pallet_bridge_dispatch::verify_message_origin( + &raw_origin, + payload, + ) + .map(drop) + .map_err(|_| BAD_ORIGIN)?, + Err(_) => { + // so what it means that we've failed to convert origin to the `frame_system::RawOrigin`? + // now it means that the custom pallet origin has been used to send the message. Do we need + // to verify it? The answer is no, because pallet may craft any origin (e.g. root) && we + // can't verify whether it is valid, or not. + }, + }; let minimal_fee_in_this_tokens = estimate_message_dispatch_and_delivery_fee::(payload, B::RELAYER_FEE_PERCENT)?; From af2637fcd16b6af6060a07010fe58485555e66e6 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 30 Sep 2021 17:31:43 +0300 Subject: [PATCH 07/10] fmt --- bin/runtime-common/src/messages.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/bin/runtime-common/src/messages.rs b/bin/runtime-common/src/messages.rs index 1a865bb8b3..a52c3b4dd6 100644 --- a/bin/runtime-common/src/messages.rs +++ b/bin/runtime-common/src/messages.rs @@ -314,17 +314,16 @@ pub mod source { OriginOf>, > = submitter.clone().into(); match raw_origin_or_err { - Ok(raw_origin) => pallet_bridge_dispatch::verify_message_origin( - &raw_origin, - payload, - ) - .map(drop) - .map_err(|_| BAD_ORIGIN)?, + Ok(raw_origin) => + pallet_bridge_dispatch::verify_message_origin(&raw_origin, payload) + .map(drop) + .map_err(|_| BAD_ORIGIN)?, Err(_) => { - // so what it means that we've failed to convert origin to the `frame_system::RawOrigin`? - // now it means that the custom pallet origin has been used to send the message. Do we need - // to verify it? The answer is no, because pallet may craft any origin (e.g. root) && we - // can't verify whether it is valid, or not. + // so what it means that we've failed to convert origin to the + // `frame_system::RawOrigin`? now it means that the custom pallet origin has + // been used to send the message. Do we need to verify it? The answer is no, + // because pallet may craft any origin (e.g. root) && we can't verify whether it + // is valid, or not. }, }; From 8d17963efe0cf69b35de7711d7984f88d9b534aa Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Fri, 8 Oct 2021 13:27:56 +0300 Subject: [PATCH 08/10] fix benchmarks compilation --- modules/token-swap/src/benchmarking.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/token-swap/src/benchmarking.rs b/modules/token-swap/src/benchmarking.rs index 0dd8922d15..62854b5485 100644 --- a/modules/token-swap/src/benchmarking.rs +++ b/modules/token-swap/src/benchmarking.rs @@ -18,7 +18,8 @@ use crate::{ swap_account_id, target_account_at_this_chain, BridgedAccountIdOf, BridgedAccountPublicOf, - BridgedAccountSignatureOf, BridgedBalanceOf, Call, Pallet, ThisChainBalance, TokenSwapOf, + BridgedAccountSignatureOf, BridgedBalanceOf, Call, Origin, Pallet, ThisChainBalance, + TokenSwapOf, }; use bp_token_swap::{TokenSwap, TokenSwapState, TokenSwapType}; @@ -42,6 +43,7 @@ pub trait Config: crate::Config { benchmarks_instance_pallet! { where_clause { where + Origin: Into, BridgedAccountPublicOf: Default + Parameter, BridgedAccountSignatureOf: Default, } From 45f4fc8b60d264bc853423552f47ec8ef8f45bd7 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Wed, 26 Jan 2022 11:16:49 +0300 Subject: [PATCH 09/10] fix TODO with None account and non-zero message fee (already covered by tests) --- bin/millau/runtime/src/lib.rs | 4 +--- modules/messages/src/instant_payments.rs | 8 ++++++-- relays/messages/src/message_race_delivery.rs | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/bin/millau/runtime/src/lib.rs b/bin/millau/runtime/src/lib.rs index 9f72b9b9e9..8acf2e5ead 100644 --- a/bin/millau/runtime/src/lib.rs +++ b/bin/millau/runtime/src/lib.rs @@ -65,9 +65,7 @@ pub use frame_support::{ pub use frame_system::Call as SystemCall; pub use pallet_balances::Call as BalancesCall; -pub use pallet_bridge_grandpa::{ - Call as BridgeGrandpaRialtoCall, Call as BridgeGrandpaWestendCall, -}; +pub use pallet_bridge_grandpa::Call as BridgeGrandpaRialtoCall; pub use pallet_bridge_messages::Call as MessagesCall; pub use pallet_sudo::Call as SudoCall; pub use pallet_timestamp::Call as TimestampCall; diff --git a/modules/messages/src/instant_payments.rs b/modules/messages/src/instant_payments.rs index c32321d930..da1f4d38ae 100644 --- a/modules/messages/src/instant_payments.rs +++ b/modules/messages/src/instant_payments.rs @@ -31,6 +31,10 @@ use num_traits::{SaturatingAdd, Zero}; use sp_runtime::traits::Saturating; use sp_std::{collections::vec_deque::VecDeque, fmt::Debug, ops::RangeInclusive}; +/// Error that occurs when message fee is non-zero, but payer is not defined. +const NON_ZERO_MESSAGE_FEE_CANT_BE_PAID_BY_NONE: &str = + "Non-zero message fee can't be paid by "; + /// Instant message payments made in given currency. /// /// The balance is initially reserved in a special `relayers-fund` account, and transferred @@ -72,7 +76,7 @@ where // if we'll accept some message that has declared that the `fee` has been paid but // it isn't actually paid, then it'll lead to problems with delivery confirmation // payments (see `pay_relayer_rewards` && `confirmation_relayer` in particular) - return Err("TODO: add test for me") + return Err(NON_ZERO_MESSAGE_FEE_CANT_BE_PAID_BY_NONE) }, None => { // message lane verifier has accepted the message before, so this message @@ -272,7 +276,7 @@ mod tests { &100, &RELAYERS_FUND_ACCOUNT, ); - assert!(result.is_err()); + assert_eq!(result, Err(NON_ZERO_MESSAGE_FEE_CANT_BE_PAID_BY_NONE)); }); } diff --git a/relays/messages/src/message_race_delivery.rs b/relays/messages/src/message_race_delivery.rs index 3433b683d7..4fd721dae5 100644 --- a/relays/messages/src/message_race_delivery.rs +++ b/relays/messages/src/message_race_delivery.rs @@ -444,7 +444,7 @@ where ); return None - } + }, _ => (), } From efc534ab78813aac7b9c6c868afae4f4d6e58131 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Wed, 26 Jan 2022 12:53:58 +0300 Subject: [PATCH 10/10] revert cargo fmt changes temporarily --- bin/millau/runtime/src/lib.rs | 4 +++- relays/messages/src/message_race_delivery.rs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/bin/millau/runtime/src/lib.rs b/bin/millau/runtime/src/lib.rs index 8acf2e5ead..9f72b9b9e9 100644 --- a/bin/millau/runtime/src/lib.rs +++ b/bin/millau/runtime/src/lib.rs @@ -65,7 +65,9 @@ pub use frame_support::{ pub use frame_system::Call as SystemCall; pub use pallet_balances::Call as BalancesCall; -pub use pallet_bridge_grandpa::Call as BridgeGrandpaRialtoCall; +pub use pallet_bridge_grandpa::{ + Call as BridgeGrandpaRialtoCall, Call as BridgeGrandpaWestendCall, +}; pub use pallet_bridge_messages::Call as MessagesCall; pub use pallet_sudo::Call as SudoCall; pub use pallet_timestamp::Call as TimestampCall; diff --git a/relays/messages/src/message_race_delivery.rs b/relays/messages/src/message_race_delivery.rs index 4fd721dae5..3433b683d7 100644 --- a/relays/messages/src/message_race_delivery.rs +++ b/relays/messages/src/message_race_delivery.rs @@ -444,7 +444,7 @@ where ); return None - }, + } _ => (), }