From 04890dc85e5f9dd2403fb87793c0cf4aa16a2172 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 30 Nov 2020 14:49:09 -0500 Subject: [PATCH] Add Derived Account Origins to Dispatcher (#519) * Update some docs * Add derived account origin * Add tests for derived origin * Do a little bit of cleanup * Change Origin type to use AccountIds instead of Public keys * Update (most) tests to use new Origin types * Remove redundant test * Update `runtime-common` tests to use new Origin types * Remove unused import * Fix documentation around origin verification * Update config types to use AccountIds in runtime * Update Origin type used in message relay * Use correct type when verifying message origin * Make CallOrigin docs more consistent * Use AccountIds instead of Public keys in Runtime types * Introduce trait for converting AccountIds * Bring back standalone function for deriving account IDs * Remove AccountIdConverter configuration trait * Remove old bridge_account_id derivation function * Handle target ID decoding errors more gracefully * Update message-lane to use new AccountId derivation * Update merged code to use new Origin types * Use explicit conversion between H256 and AccountIds * Make relayer fund account a config option in `message-lane` pallet * Add note about deriving the same account on different chains * Fix test weight * Use AccountId instead of Public key when signing Calls * Semi-hardcode relayer fund address into Message Lane pallet --- bridges/bin/millau/runtime/src/lib.rs | 5 +- bridges/bin/rialto/runtime/src/lib.rs | 5 +- bridges/bin/runtime-common/src/messages.rs | 12 +- bridges/modules/call-dispatch/Cargo.toml | 3 +- bridges/modules/call-dispatch/src/lib.rs | 372 +++++++++++------- .../message-lane/src/instant_payments.rs | 34 +- bridges/modules/message-lane/src/lib.rs | 18 + bridges/modules/message-lane/src/mock.rs | 23 +- .../primitives/message-dispatch/src/lib.rs | 2 +- .../message-lane/src/source_chain.rs | 13 +- bridges/primitives/millau/src/lib.rs | 8 + bridges/primitives/rialto/src/lib.rs | 9 + bridges/primitives/runtime/Cargo.toml | 2 + bridges/primitives/runtime/src/lib.rs | 51 ++- bridges/relays/substrate/src/main.rs | 15 +- 15 files changed, 376 insertions(+), 196 deletions(-) diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index 2949e695ffde..cbb4a14d5462 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -226,9 +226,10 @@ impl pallet_bridge_call_dispatch::Trait for Runtime { type Event = Event; type MessageId = (bp_message_lane::LaneId, bp_message_lane::MessageNonce); type Call = Call; - type SourceChainAccountPublic = MultiSigner; + type SourceChainAccountId = bp_rialto::AccountId; type TargetChainAccountPublic = MultiSigner; type TargetChainSignature = MultiSignature; + type AccountIdConverter = bp_millau::AccountIdConverter; } impl pallet_grandpa::Trait for Runtime { @@ -336,6 +337,8 @@ impl pallet_message_lane::Trait for Runtime { type InboundMessageFee = bp_rialto::Balance; type InboundRelayer = bp_rialto::AccountId; + type AccountIdConverter = bp_millau::AccountIdConverter; + type TargetHeaderChain = crate::rialto_messages::Rialto; type LaneMessageVerifier = crate::rialto_messages::ToRialtoMessageVerifier; type MessageDeliveryAndDispatchPayment = diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs index cd5f2df34ed9..e239af930ceb 100644 --- a/bridges/bin/rialto/runtime/src/lib.rs +++ b/bridges/bin/rialto/runtime/src/lib.rs @@ -277,9 +277,10 @@ impl pallet_bridge_call_dispatch::Trait for Runtime { type Event = Event; type MessageId = (bp_message_lane::LaneId, bp_message_lane::MessageNonce); type Call = Call; - type SourceChainAccountPublic = MultiSigner; + type SourceChainAccountId = bp_millau::AccountId; type TargetChainAccountPublic = MultiSigner; type TargetChainSignature = MultiSignature; + type AccountIdConverter = bp_rialto::AccountIdConverter; } pub struct DepositInto; @@ -443,6 +444,8 @@ impl pallet_message_lane::Trait for Runtime { type InboundMessageFee = bp_millau::Balance; type InboundRelayer = bp_millau::AccountId; + type AccountIdConverter = bp_rialto::AccountIdConverter; + type TargetHeaderChain = crate::millau_messages::Millau; type LaneMessageVerifier = crate::millau_messages::ToMillauMessageVerifier; type MessageDeliveryAndDispatchPayment = diff --git a/bridges/bin/runtime-common/src/messages.rs b/bridges/bin/runtime-common/src/messages.rs index 9a934ee162da..b26d9435cd6b 100644 --- a/bridges/bin/runtime-common/src/messages.rs +++ b/bridges/bin/runtime-common/src/messages.rs @@ -114,7 +114,7 @@ pub mod source { /// Message payload for This -> Bridged chain messages. pub type FromThisChainMessagePayload = pallet_bridge_call_dispatch::MessagePayload< - SignerOf>, + AccountIdOf>, SignerOf>, SignatureOf>, BridgedChainOpaqueCall, @@ -238,14 +238,14 @@ pub mod target { /// Call origin for Bridged -> This chain messages. pub type FromBridgedChainMessageCallOrigin = pallet_bridge_call_dispatch::CallOrigin< - SignerOf>, + AccountIdOf>, SignerOf>, SignatureOf>, >; /// Decoded Bridged -> This message payload. pub type FromBridgedChainDecodedMessagePayload = pallet_bridge_call_dispatch::MessagePayload< - SignerOf>, + AccountIdOf>, SignerOf>, SignatureOf>, CallOf>, @@ -614,7 +614,7 @@ mod tests { let message_on_bridged_chain = source::FromThisChainMessagePayload:: { spec_version: 1, weight: 100, - origin: pallet_bridge_call_dispatch::CallOrigin::BridgeAccount, + origin: pallet_bridge_call_dispatch::CallOrigin::SourceRoot, call: ThisChainCall::Transfer.encode(), } .encode(); @@ -628,7 +628,7 @@ mod tests { target::FromBridgedChainDecodedMessagePayload:: { spec_version: 1, weight: 100, - origin: pallet_bridge_call_dispatch::CallOrigin::BridgeAccount, + origin: pallet_bridge_call_dispatch::CallOrigin::SourceRoot, call: ThisChainCall::Transfer, } ); @@ -642,7 +642,7 @@ mod tests { let payload = source::FromThisChainMessagePayload:: { spec_version: 1, weight: 100, - origin: pallet_bridge_call_dispatch::CallOrigin::BridgeAccount, + origin: pallet_bridge_call_dispatch::CallOrigin::SourceRoot, call: vec![42], }; diff --git a/bridges/modules/call-dispatch/Cargo.toml b/bridges/modules/call-dispatch/Cargo.toml index e80f98e0adc1..9c25e3a8eb1d 100644 --- a/bridges/modules/call-dispatch/Cargo.toml +++ b/bridges/modules/call-dispatch/Cargo.toml @@ -18,11 +18,11 @@ bp-runtime = { path = "../../primitives/runtime", default-features = false } frame-support = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } frame-system = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } +sp-core = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } sp-std = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } sp-runtime = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } [dev-dependencies] -sp-core = { git = "https://github.com/paritytech/substrate.git", branch = "master" } sp-io = { git = "https://github.com/paritytech/substrate.git", branch = "master" } [features] @@ -32,6 +32,7 @@ std = [ "bp-runtime/std", "frame-support/std", "frame-system/std", + "sp-core/std", "sp-runtime/std", "sp-std/std", ] diff --git a/bridges/modules/call-dispatch/src/lib.rs b/bridges/modules/call-dispatch/src/lib.rs index e7a35a342314..63d3ff25d54a 100644 --- a/bridges/modules/call-dispatch/src/lib.rs +++ b/bridges/modules/call-dispatch/src/lib.rs @@ -16,18 +16,16 @@ //! Runtime module which takes care of dispatching messages received over the bridge. //! -//! The messages are interpreted directly as runtime `Call`s, we attempt to decode -//! them and then dispatch as usualy. -//! To prevent compatibility issues, the calls have to include `spec_version` as well -//! which is being checked before dispatch. -//! -//! In case of succesful dispatch event is emitted. +//! The messages are interpreted directly as runtime `Call`. We attempt to decode +//! them and then dispatch as usual. To prevent compatibility issues, the Calls have +//! to include a `spec_version`. This will be checked before dispatch. In the case of +//! a succesful dispatch an event is emitted. #![cfg_attr(not(feature = "std"), no_std)] #![warn(missing_docs)] use bp_message_dispatch::{MessageDispatch, Weight}; -use bp_runtime::{bridge_account_id, InstanceId, CALL_DISPATCH_MODULE_PREFIX}; +use bp_runtime::{derive_account_id, InstanceId, SourceAccount}; use codec::{Decode, Encode}; use frame_support::{ decl_event, decl_module, decl_storage, @@ -38,37 +36,51 @@ use frame_support::{ }; use frame_system::{ensure_root, ensure_signed, RawOrigin}; use sp_runtime::{ - traits::{BadOrigin, IdentifyAccount, Verify}, + traits::{BadOrigin, Convert, IdentifyAccount, MaybeDisplay, MaybeSerializeDeserialize, Member, Verify}, DispatchResult, }; -use sp_std::{marker::PhantomData, prelude::*}; +use sp_std::{fmt::Debug, marker::PhantomData, prelude::*}; /// Spec version type. pub type SpecVersion = u32; -/// Origin of the call on the target chain. +/// Origin of a Call when it is dispatched on the target chain. +/// +/// The source chain can (and should) verify that the message can be dispatched on the target chain +/// with a particular origin given the source chain's origin. This can be done with the +/// `verify_message_origin()` function. #[derive(RuntimeDebug, Encode, Decode, Clone, PartialEq, Eq)] -pub enum CallOrigin { - /// Call is originated from bridge account, which is (designed to be) specific to - /// the single deployed instance of the messages bridge (message-lane, ...) module. - /// It is assumed that this account is not controlled by anyone and has zero balance - /// (unless someone would make transfer by mistake?). - /// If we trust the source chain to allow sending calls with that origin in case they originate - /// from source chain `root` account (default implementation), `BridgeAccount` represents the - /// source-chain-root origin on the target chain and can be used to send and authorize - /// "control plane" messages between the two runtimes. - BridgeAccount, - /// Call is originated from account, identified by `TargetChainAccountPublic`. The proof - /// that the `SourceChainAccountPublic` controls `TargetChainAccountPublic` is the - /// `TargetChainSignature` over `(Call, SourceChainAccountPublic).encode()`. - /// The source chain must ensure that the message is sent by the owner of - /// `SourceChainAccountPublic` account (use the `fn verify_sending_message()`). - RealAccount(SourceChainAccountPublic, TargetChainAccountPublic, TargetChainSignature), +pub enum CallOrigin { + /// Call is sent by the Root origin on the source chain. On the target chain it is dispatched + /// from a derived account. + /// + /// The derived account represents the source Root account on the target chain. This is useful + /// if the target chain needs some way of knowing that a call came from a priviledged origin on + /// the source chain (maybe to allow a configuration change for example). + SourceRoot, + + /// Call is sent by `SourceChainAccountId` on the source chain. On the target chain it is + /// dispatched from an account controlled by a private key on the target chain. + /// + /// The account can be identified by `TargetChainAccountPublic`. The proof that the + /// `SourceChainAccountId` controls `TargetChainAccountPublic` is the `TargetChainSignature` + /// over `(Call, SourceChainAccountId).encode()`. + TargetAccount(SourceChainAccountId, TargetChainAccountPublic, TargetChainSignature), + + /// Call is sent by the `SourceChainAccountId` on the source chain. On the target chain it is + /// dispatched from a derived account ID. + /// + /// The account ID on the target chain is derived from the source account ID This is useful if + /// you need a way to represent foreign accounts on this chain for call dispatch purposes. + /// + /// Note that the derived account does not need to have a private key on the target chain. This + /// origin can therefore represent proxies, pallets, etc. as well as "regular" accounts. + SourceAccount(SourceChainAccountId), } /// Message payload type used by call-dispatch module. #[derive(RuntimeDebug, Encode, Decode, Clone, PartialEq, Eq)] -pub struct MessagePayload { +pub struct MessagePayload { /// Runtime specification version. We only dispatch messages that have the same /// runtime version. Otherwise we risk to misinterpret encoded calls. pub spec_version: SpecVersion, @@ -76,7 +88,7 @@ pub struct MessagePayload, + pub origin: CallOrigin, /// The call itself. pub call: Call, } @@ -89,8 +101,8 @@ pub trait Trait: frame_system::Trait { /// event with this id + dispatch result. Could be e.g. (LaneId, MessageNonce) if /// it comes from message-lane module. type MessageId: Parameter; - /// Type of account public key on source chain. - type SourceChainAccountPublic: Parameter; + /// Type of account ID on source chain. + type SourceChainAccountId: Parameter + Member + MaybeSerializeDeserialize + Debug + MaybeDisplay + Ord + Default; /// Type of account public key on target chain. type TargetChainAccountPublic: Parameter + IdentifyAccount; /// Type of signature that may prove that the message has been signed by @@ -103,11 +115,14 @@ pub trait Trait: frame_system::Trait { Origin = ::Origin, PostInfo = frame_support::dispatch::PostDispatchInfo, >; + /// A type which can be turned into an AccountId from a 256-bit hash. + /// + /// Used when deriving target chain AccountIds from source chain AccountIds. + type AccountIdConverter: sp_runtime::traits::Convert; } decl_storage! { - trait Store for Module, I: Instance = DefaultInstance> as CallDispatch { - } + trait Store for Module, I: Instance = DefaultInstance> as CallDispatch {} } decl_event!( @@ -124,8 +139,8 @@ decl_event!( MessageSignatureMismatch(InstanceId, MessageId), /// Message has been dispatched with given result. MessageDispatched(InstanceId, MessageId, DispatchResult), - /// Phantom member, never used. - Dummy(PhantomData), + /// Phantom member, never used. Needed to handle multiple pallet instances. + _Dummy(PhantomData), } ); @@ -139,7 +154,7 @@ decl_module! { impl, I: Instance> MessageDispatch for Module { type Message = MessagePayload< - T::SourceChainAccountPublic, + T::SourceChainAccountId, T::TargetChainAccountPublic, T::TargetChainSignature, >::Call, @@ -194,11 +209,14 @@ impl, I: Instance> MessageDispatch for Module { // prepare dispatch origin let origin_account = match message.origin { - CallOrigin::BridgeAccount => bridge_account_id(bridge, CALL_DISPATCH_MODULE_PREFIX), - CallOrigin::RealAccount(source_public, target_public, target_signature) => { + CallOrigin::SourceRoot => { + let encoded_id = derive_account_id::(bridge, SourceAccount::Root); + T::AccountIdConverter::convert(encoded_id) + } + CallOrigin::TargetAccount(source_account_id, target_public, target_signature) => { let mut signed_message = Vec::new(); message.call.encode_to(&mut signed_message); - source_public.encode_to(&mut signed_message); + source_account_id.encode_to(&mut signed_message); let target_account = target_public.into_account(); if !target_signature.verify(&signed_message[..], &target_account) { @@ -215,6 +233,10 @@ impl, I: Instance> MessageDispatch for Module { target_account } + CallOrigin::SourceAccount(source_account_id) => { + let encoded_id = derive_account_id(bridge, SourceAccount::Account(source_account_id)); + T::AccountIdConverter::convert(encoded_id) + } }; // finally dispatch message @@ -238,35 +260,45 @@ impl, I: Instance> MessageDispatch for Module { } } -/// Verify payload of the message at the sending side. -pub fn verify_sending_message< - ThisChainOuterOrigin, - ThisChainAccountId, - SourceChainAccountPublic, +/// Check if the message is allowed to be dispatched on the target chain given the sender's origin +/// on the source chain. +/// +/// For example, if a message is sent from a "regular" account on the source chain it will not be +/// allowed to be dispatched as Root on the target chain. This is a useful check to do on the source +/// chain _before_ sending a message whose dispatch will be rejected on the target chain. +pub fn verify_message_origin< + SourceChainOuterOrigin, + SourceChainAccountId, TargetChainAccountPublic, TargetChainSignature, Call, >( - sender_origin: ThisChainOuterOrigin, - message: &MessagePayload, -) -> Result, BadOrigin> + sender_origin: SourceChainOuterOrigin, + message: &MessagePayload, +) -> Result, BadOrigin> where - ThisChainOuterOrigin: Into, ThisChainOuterOrigin>>, - TargetChainAccountPublic: Clone + IdentifyAccount, - ThisChainAccountId: PartialEq, + SourceChainOuterOrigin: Into, SourceChainOuterOrigin>>, + SourceChainAccountId: PartialEq, { match message.origin { - CallOrigin::BridgeAccount => { + CallOrigin::SourceRoot => { ensure_root(sender_origin)?; Ok(None) } - CallOrigin::RealAccount(ref this_account_public, _, _) => { - let this_chain_account_id = ensure_signed(sender_origin)?; - if this_chain_account_id != this_account_public.clone().into_account() { + CallOrigin::TargetAccount(ref source_account_id, _, _) => { + let source_chain_signer = ensure_signed(sender_origin)?; + if source_chain_signer != *source_account_id { return Err(BadOrigin); } - Ok(Some(this_chain_account_id)) + Ok(Some(source_chain_signer)) + } + CallOrigin::SourceAccount(ref source_account_id) => { + let source_chain_signer = ensure_signed(sender_origin)?; + if source_chain_signer != *source_account_id { + return Err(BadOrigin); + } + Ok(Some(source_chain_signer)) } } } @@ -280,7 +312,7 @@ mod tests { use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup}, - DispatchError, Perbill, + Perbill, }; type AccountId = u64; @@ -311,6 +343,14 @@ mod tests { } } + pub struct AccountIdConverter; + + impl sp_runtime::traits::Convert for AccountIdConverter { + fn convert(hash: H256) -> AccountId { + hash.to_low_u64_ne() + } + } + #[derive(Clone, Eq, PartialEq)] pub struct TestRuntime; @@ -374,10 +414,11 @@ mod tests { impl Trait for TestRuntime { type Event = TestEvent; type MessageId = MessageId; - type SourceChainAccountPublic = TestAccountPublic; + type SourceChainAccountId = AccountId; type TargetChainAccountPublic = TestAccountPublic; type TargetChainSignature = TestSignature; type Call = Call; + type AccountIdConverter = AccountIdConverter; } const TEST_SPEC_VERSION: SpecVersion = 0; @@ -390,33 +431,62 @@ mod tests { sp_io::TestExternalities::new(t) } - fn prepare_bridge_message( + fn prepare_message( + origin: CallOrigin, call: Call, ) -> as MessageDispatch<::MessageId>>::Message { MessagePayload { spec_version: TEST_SPEC_VERSION, weight: TEST_WEIGHT, - origin: CallOrigin::BridgeAccount, + origin, call, } } + fn prepare_root_message( + call: Call, + ) -> as MessageDispatch<::MessageId>>::Message { + prepare_message(CallOrigin::SourceRoot, call) + } + + fn prepare_target_message( + call: Call, + ) -> as MessageDispatch<::MessageId>>::Message { + let origin = CallOrigin::TargetAccount(1, TestAccountPublic(1), TestSignature(1)); + prepare_message(origin, call) + } + + fn prepare_source_message( + call: Call, + ) -> as MessageDispatch<::MessageId>>::Message { + let origin = CallOrigin::SourceAccount(1); + prepare_message(origin, call) + } + #[test] - fn should_succesfuly_dispatch_remark() { + fn should_fail_on_spec_version_mismatch() { new_test_ext().execute_with(|| { - let origin = b"ethb".to_owned(); + let bridge = b"ethb".to_owned(); let id = [0; 4]; - let message = - prepare_bridge_message(Call::System(>::remark(vec![1, 2, 3]))); + + const BAD_SPEC_VERSION: SpecVersion = 99; + let mut message = + prepare_root_message(Call::System(>::remark(vec![1, 2, 3]))); + message.spec_version = BAD_SPEC_VERSION; System::set_block_number(1); - CallDispatch::dispatch(origin, id, message); + CallDispatch::dispatch(bridge, id, message); assert_eq!( System::events(), vec![EventRecord { phase: Phase::Initialization, - event: TestEvent::call_dispatch(Event::::MessageDispatched(origin, id, Ok(()))), + event: TestEvent::call_dispatch(Event::::MessageVersionSpecMismatch( + bridge, + id, + TEST_SPEC_VERSION, + BAD_SPEC_VERSION + )), topics: vec![], }], ); @@ -424,22 +494,24 @@ mod tests { } #[test] - fn should_fail_on_spec_version_mismatch() { + fn should_fail_on_weight_mismatch() { new_test_ext().execute_with(|| { - let origin = b"ethb".to_owned(); + let bridge = b"ethb".to_owned(); let id = [0; 4]; let mut message = - prepare_bridge_message(Call::System(>::remark(vec![1, 2, 3]))); - message.origin = CallOrigin::RealAccount(TestAccountPublic(2), TestAccountPublic(2), TestSignature(1)); + prepare_root_message(Call::System(>::remark(vec![1, 2, 3]))); + message.weight = 0; System::set_block_number(1); - CallDispatch::dispatch(origin, id, message); + CallDispatch::dispatch(bridge, id, message); assert_eq!( System::events(), vec![EventRecord { phase: Phase::Initialization, - event: TestEvent::call_dispatch(Event::::MessageSignatureMismatch(origin, id,)), + event: TestEvent::call_dispatch(Event::::MessageWeightMismatch( + bridge, id, 1973000, 0, + )), topics: vec![], }], ); @@ -447,24 +519,25 @@ mod tests { } #[test] - fn should_fail_on_weight_mismatch() { + fn should_fail_on_signature_mismatch() { new_test_ext().execute_with(|| { - let origin = b"ethb".to_owned(); + let bridge = b"ethb".to_owned(); let id = [0; 4]; - let mut message = - prepare_bridge_message(Call::System(>::remark(vec![1, 2, 3]))); - message.weight = 0; + + let call_origin = CallOrigin::TargetAccount(1, TestAccountPublic(1), TestSignature(99)); + let message = prepare_message( + call_origin, + Call::System(>::remark(vec![1, 2, 3])), + ); System::set_block_number(1); - CallDispatch::dispatch(origin, id, message); + CallDispatch::dispatch(bridge, id, message); assert_eq!( System::events(), vec![EventRecord { phase: Phase::Initialization, - event: TestEvent::call_dispatch(Event::::MessageWeightMismatch( - origin, id, 1973000, 0, - )), + event: TestEvent::call_dispatch(Event::::MessageSignatureMismatch(bridge, id)), topics: vec![], }], ); @@ -472,24 +545,20 @@ mod tests { } #[test] - fn should_fail_on_signature_mismatch() { + fn should_dispatch_bridge_message_from_root_origin() { new_test_ext().execute_with(|| { - let origin = b"ethb".to_owned(); + let bridge = b"ethb".to_owned(); let id = [0; 4]; - let mut message = - prepare_bridge_message(Call::System(>::remark(vec![1, 2, 3]))); - message.weight = 0; + let message = prepare_root_message(Call::System(>::remark(vec![1, 2, 3]))); System::set_block_number(1); - CallDispatch::dispatch(origin, id, message); + CallDispatch::dispatch(bridge, id, message); assert_eq!( System::events(), vec![EventRecord { phase: Phase::Initialization, - event: TestEvent::call_dispatch(Event::::MessageWeightMismatch( - origin, id, 1973000, 0, - )), + event: TestEvent::call_dispatch(Event::::MessageDispatched(bridge, id, Ok(()))), topics: vec![], }], ); @@ -497,104 +566,115 @@ mod tests { } #[test] - fn should_dispatch_bridge_message_from_non_root_origin() { + fn should_dispatch_bridge_message_from_target_origin() { new_test_ext().execute_with(|| { - let origin = b"ethb".to_owned(); let id = [0; 4]; - let message = prepare_bridge_message(Call::System(>::fill_block( - Perbill::from_percent(10), - ))); + let bridge = b"ethb".to_owned(); + + let call = Call::System(>::remark(vec![])); + let message = prepare_target_message(call); System::set_block_number(1); - CallDispatch::dispatch(origin, id, message); + CallDispatch::dispatch(bridge, id, message); assert_eq!( System::events(), vec![EventRecord { phase: Phase::Initialization, - event: TestEvent::call_dispatch(Event::::MessageDispatched( - origin, - id, - Err(DispatchError::BadOrigin) - )), + event: TestEvent::call_dispatch(Event::::MessageDispatched(bridge, id, Ok(()))), topics: vec![], }], ); - }); + }) } #[test] - fn dispatch_supports_different_accounts() { - fn dispatch_suicide(call_origin: CallOrigin) { - let origin = b"ethb".to_owned(); + fn should_dispatch_bridge_message_from_source_origin() { + new_test_ext().execute_with(|| { let id = [0; 4]; - let mut message = prepare_bridge_message(Call::System(>::suicide())); - message.origin = call_origin; + let bridge = b"ethb".to_owned(); - System::set_block_number(1); - CallDispatch::dispatch(origin, id, message); - } + let call = Call::System(>::remark(vec![])); + let message = prepare_source_message(call); - new_test_ext().execute_with(|| { - // 'create' real account - let real_account_id = 1; - System::inc_account_nonce(real_account_id); - // 'create' bridge account - let bridge_account_id: AccountId = bridge_account_id(*b"ethb", CALL_DISPATCH_MODULE_PREFIX); - System::inc_account_nonce(bridge_account_id); - - assert_eq!(System::account_nonce(real_account_id), 1); - assert_eq!(System::account_nonce(bridge_account_id), 1); - - // kill real account - dispatch_suicide(CallOrigin::RealAccount( - TestAccountPublic(real_account_id), - TestAccountPublic(real_account_id), - TestSignature(real_account_id), - )); - assert_eq!(System::account_nonce(real_account_id), 0); - assert_eq!(System::account_nonce(bridge_account_id), 1); + System::set_block_number(1); + CallDispatch::dispatch(bridge, id, message); - // kill bridge account - dispatch_suicide(CallOrigin::BridgeAccount); - assert_eq!(System::account_nonce(real_account_id), 0); - assert_eq!(System::account_nonce(bridge_account_id), 0); - }); + assert_eq!( + System::events(), + vec![EventRecord { + phase: Phase::Initialization, + event: TestEvent::call_dispatch(Event::::MessageDispatched(bridge, id, Ok(()))), + topics: vec![], + }], + ); + }) } #[test] - fn origin_is_checked_when_verify_sending_message() { - let mut message = prepare_bridge_message(Call::System(>::suicide())); + fn origin_is_checked_when_verifying_sending_message_using_source_root_account() { + let call = Call::System(>::remark(vec![])); + let message = prepare_root_message(call); - // when message is sent by root, CallOrigin::BridgeAccount is allowed + // When message is sent by Root, CallOrigin::SourceRoot is allowed assert!(matches!( - verify_sending_message(Origin::from(RawOrigin::Root), &message), + verify_message_origin(Origin::from(RawOrigin::Root), &message), Ok(None) )); - // when message is sent by some real account, CallOrigin::BridgeAccount is not allowed + // when message is sent by some real account, CallOrigin::SourceRoot is not allowed + assert!(matches!( + verify_message_origin(Origin::from(RawOrigin::Signed(1)), &message), + Err(BadOrigin) + )); + } + + #[test] + fn origin_is_checked_when_verifying_sending_message_using_target_account() { + let call = Call::System(>::remark(vec![])); + let message = prepare_target_message(call); + + // When message is sent by Root, CallOrigin::TargetAccount is not allowed assert!(matches!( - verify_sending_message(Origin::from(RawOrigin::Signed(1)), &message), + verify_message_origin(Origin::from(RawOrigin::Root), &message), Err(BadOrigin) )); - // when message is sent by root, CallOrigin::RealAccount is not allowed - message.origin = CallOrigin::RealAccount(TestAccountPublic(2), TestAccountPublic(2), TestSignature(2)); + // When message is sent by some other account, it is rejected assert!(matches!( - verify_sending_message(Origin::from(RawOrigin::Root), &message), + verify_message_origin(Origin::from(RawOrigin::Signed(2)), &message), Err(BadOrigin) )); - // when message is sent by some other account, it is rejected + // When message is sent by a real account, it is allowed to have origin + // CallOrigin::TargetAccount assert!(matches!( - verify_sending_message(Origin::from(RawOrigin::Signed(1)), &message), + verify_message_origin(Origin::from(RawOrigin::Signed(1)), &message), + Ok(Some(1)) + )); + } + + #[test] + fn origin_is_checked_when_verifying_sending_message_using_source_account() { + let call = Call::System(>::remark(vec![])); + let message = prepare_source_message(call); + + // Sending a message from the expected origin account works + assert!(matches!( + verify_message_origin(Origin::from(RawOrigin::Signed(1)), &message), + Ok(Some(1)) + )); + + // If we send a message from a different account, it is rejected + assert!(matches!( + verify_message_origin(Origin::from(RawOrigin::Signed(2)), &message), Err(BadOrigin) )); - // when message is sent real account, it is allowed to have origin CallOrigin::RealAccount + // If we try and send the message from Root, it is also rejected assert!(matches!( - verify_sending_message(Origin::from(RawOrigin::Signed(2)), &message), - Ok(Some(2)) + verify_message_origin(Origin::from(RawOrigin::Root), &message), + Err(BadOrigin) )); } } diff --git a/bridges/modules/message-lane/src/instant_payments.rs b/bridges/modules/message-lane/src/instant_payments.rs index 7db2429e07f1..efc9795b2e15 100644 --- a/bridges/modules/message-lane/src/instant_payments.rs +++ b/bridges/modules/message-lane/src/instant_payments.rs @@ -18,8 +18,7 @@ //! All payments are instant. use bp_message_lane::source_chain::MessageDeliveryAndDispatchPayment; -use bp_runtime::{bridge_account_id, MESSAGE_LANE_MODULE_PREFIX, NO_INSTANCE_ID}; -use codec::Decode; +use codec::Encode; use frame_support::traits::{Currency as CurrencyT, ExistenceRequirement}; use sp_std::fmt::Debug; @@ -33,23 +32,26 @@ impl MessageDeliveryAndDispatchPayment where Currency: CurrencyT, - AccountId: Debug + Default + Decode, + AccountId: Debug + Default + Encode, { type Error = &'static str; - fn pay_delivery_and_dispatch_fee(submitter: &AccountId, fee: &Currency::Balance) -> Result<(), Self::Error> { - Currency::transfer( - submitter, - &relayers_fund_account(), - *fee, - ExistenceRequirement::AllowDeath, - ) - .map_err(Into::into) + fn pay_delivery_and_dispatch_fee( + submitter: &AccountId, + fee: &Currency::Balance, + relayer_fund_account: &AccountId, + ) -> Result<(), Self::Error> { + Currency::transfer(submitter, relayer_fund_account, *fee, ExistenceRequirement::AllowDeath).map_err(Into::into) } - fn pay_relayer_reward(_confirmation_relayer: &AccountId, relayer: &AccountId, reward: &Currency::Balance) { + fn pay_relayer_reward( + _confirmation_relayer: &AccountId, + relayer: &AccountId, + reward: &Currency::Balance, + relayer_fund_account: &AccountId, + ) { let pay_result = Currency::transfer( - &relayers_fund_account(), + &relayer_fund_account, relayer, *reward, ExistenceRequirement::AllowDeath, @@ -73,9 +75,3 @@ where } } } - -/// Return account id of shared relayers-fund account that is storing all fees -/// paid by submitters, until they're claimed by relayers. -fn relayers_fund_account() -> AccountId { - bridge_account_id(NO_INSTANCE_ID, MESSAGE_LANE_MODULE_PREFIX) -} diff --git a/bridges/modules/message-lane/src/lib.rs b/bridges/modules/message-lane/src/lib.rs index 936d590f6410..af822e02a33f 100644 --- a/bridges/modules/message-lane/src/lib.rs +++ b/bridges/modules/message-lane/src/lib.rs @@ -95,6 +95,11 @@ pub trait Trait: frame_system::Trait { /// Identifier of relayer that deliver messages to this chain. Relayer reward is paid on the bridged chain. type InboundRelayer: Parameter; + /// A type which can be turned into an AccountId from a 256-bit hash. + /// + /// Used when deriving the shared relayer fund account. + type AccountIdConverter: sp_runtime::traits::Convert; + // Types that are used by outbound_lane (on source chain). /// Target header chain. @@ -267,6 +272,7 @@ decl_module! { T::MessageDeliveryAndDispatchPayment::pay_delivery_and_dispatch_fee( &submitter, &delivery_and_dispatch_fee, + &relayer_fund_account_id::(), ).map_err(|err| { frame_support::debug::trace!( "Message to lane {:?} is rejected because submitter {:?} is unable to pay fee {:?}: {:?}", @@ -396,6 +402,7 @@ decl_module! { let received_range = lane.confirm_delivery(lane_data.latest_received_nonce); if let Some(received_range) = received_range { Self::deposit_event(RawEvent::MessagesDelivered(lane_id, received_range.0, received_range.1)); + let relayer_fund_account = relayer_fund_account_id::(); // reward relayers that have delivered messages // this loop is bounded by `T::MaxUnconfirmedMessagesAtInboundLane` on the bridged chain @@ -413,6 +420,7 @@ decl_module! { &confirmation_relayer, &relayer, &message_data.fee, + &relayer_fund_account, ); } } @@ -636,6 +644,16 @@ fn verify_and_decode_messages_proof, Fee, Dispatch }) } +/// AccountId of the shared relayer fund account. +/// +/// This account stores all the fees paid by submitters. Relayers are able to claim these +/// funds as at their convenience. +fn relayer_fund_account_id, I: Instance>() -> T::AccountId { + use sp_runtime::traits::Convert; + let encoded_id = bp_runtime::derive_relayer_fund_account_id(bp_runtime::NO_INSTANCE_ID); + T::AccountIdConverter::convert(encoded_id) +} + #[cfg(test)] mod tests { use super::*; diff --git a/bridges/modules/message-lane/src/mock.rs b/bridges/modules/message-lane/src/mock.rs index 398f85f655ff..bc7b15ab14f6 100644 --- a/bridges/modules/message-lane/src/mock.rs +++ b/bridges/modules/message-lane/src/mock.rs @@ -36,6 +36,14 @@ pub type TestPayload = (u64, Weight); pub type TestMessageFee = u64; pub type TestRelayer = u64; +pub struct AccountIdConverter; + +impl sp_runtime::traits::Convert for AccountIdConverter { + fn convert(hash: H256) -> AccountId { + hash.to_low_u64_ne() + } +} + #[derive(Clone, Eq, PartialEq, Debug)] pub struct TestRuntime; @@ -106,6 +114,8 @@ impl Trait for TestRuntime { type InboundMessageFee = TestMessageFee; type InboundRelayer = TestRelayer; + type AccountIdConverter = AccountIdConverter; + type TargetHeaderChain = TestTargetHeaderChain; type LaneMessageVerifier = TestLaneMessageVerifier; type MessageDeliveryAndDispatchPayment = TestMessageDeliveryAndDispatchPayment; @@ -234,7 +244,11 @@ impl TestMessageDeliveryAndDispatchPayment { impl MessageDeliveryAndDispatchPayment for TestMessageDeliveryAndDispatchPayment { type Error = &'static str; - fn pay_delivery_and_dispatch_fee(submitter: &AccountId, fee: &TestMessageFee) -> Result<(), Self::Error> { + fn pay_delivery_and_dispatch_fee( + submitter: &AccountId, + fee: &TestMessageFee, + _relayer_fund_account: &AccountId, + ) -> Result<(), Self::Error> { if frame_support::storage::unhashed::get(b":reject-message-fee:") == Some(true) { return Err(TEST_ERROR); } @@ -243,7 +257,12 @@ impl MessageDeliveryAndDispatchPayment for TestMessag Ok(()) } - fn pay_relayer_reward(_confirmation_relayer: &AccountId, relayer: &AccountId, fee: &TestMessageFee) { + fn pay_relayer_reward( + _confirmation_relayer: &AccountId, + relayer: &AccountId, + fee: &TestMessageFee, + _relayer_fund_account: &AccountId, + ) { let key = (b":relayer-reward:", relayer, fee).encode(); frame_support::storage::unhashed::put(&key, &true); } diff --git a/bridges/primitives/message-dispatch/src/lib.rs b/bridges/primitives/message-dispatch/src/lib.rs index 1877df90bcb3..321e09654a75 100644 --- a/bridges/primitives/message-dispatch/src/lib.rs +++ b/bridges/primitives/message-dispatch/src/lib.rs @@ -39,7 +39,7 @@ pub trait MessageDispatch { /// /// `bridge` indicates instance of deployed bridge where the message came from. /// - /// `id` is a short unique if of the message. + /// `id` is a short unique identifier of the message. /// /// Returns post-dispatch (actual) message weight. fn dispatch(bridge: InstanceId, id: MessageId, message: Self::Message); diff --git a/bridges/primitives/message-lane/src/source_chain.rs b/bridges/primitives/message-lane/src/source_chain.rs index fd188e56b7c9..4bf964c9b041 100644 --- a/bridges/primitives/message-lane/src/source_chain.rs +++ b/bridges/primitives/message-lane/src/source_chain.rs @@ -93,8 +93,17 @@ pub trait MessageDeliveryAndDispatchPayment { /// Withhold/write-off delivery_and_dispatch_fee from submitter account to /// some relayers-fund account. - fn pay_delivery_and_dispatch_fee(submitter: &AccountId, fee: &Balance) -> Result<(), Self::Error>; + fn pay_delivery_and_dispatch_fee( + submitter: &AccountId, + fee: &Balance, + relayer_fund_account: &AccountId, + ) -> Result<(), Self::Error>; /// Pay reward for delivering message to the given relayer account. - fn pay_relayer_reward(confirmation_relayer: &AccountId, relayer: &AccountId, reward: &Balance); + fn pay_relayer_reward( + confirmation_relayer: &AccountId, + relayer: &AccountId, + reward: &Balance, + relayer_fund_account: &AccountId, + ); } diff --git a/bridges/primitives/millau/src/lib.rs b/bridges/primitives/millau/src/lib.rs index c5af6a07ad69..fc71ea7a26db 100644 --- a/bridges/primitives/millau/src/lib.rs +++ b/bridges/primitives/millau/src/lib.rs @@ -136,6 +136,14 @@ pub type AccountSigner = MultiSigner; /// Balance of an account. pub type Balance = u64; +/// Convert a 256-bit hash into an AccountId. +pub struct AccountIdConverter; + +impl sp_runtime::traits::Convert for AccountIdConverter { + fn convert(hash: sp_core::H256) -> AccountId { + hash.to_fixed_bytes().into() + } +} sp_api::decl_runtime_apis! { /// API for querying information about Millau headers from the Bridge Pallet instance. /// diff --git a/bridges/primitives/rialto/src/lib.rs b/bridges/primitives/rialto/src/lib.rs index a9a1342a68fb..c7d365c91bcc 100644 --- a/bridges/primitives/rialto/src/lib.rs +++ b/bridges/primitives/rialto/src/lib.rs @@ -98,6 +98,15 @@ pub type AccountSigner = MultiSigner; /// Balance of an account. pub type Balance = u128; +/// Convert a 256-bit hash into an AccountId. +pub struct AccountIdConverter; + +impl sp_runtime::traits::Convert for AccountIdConverter { + fn convert(hash: sp_core::H256) -> AccountId { + hash.to_fixed_bytes().into() + } +} + sp_api::decl_runtime_apis! { /// API for querying information about Rialto headers from the Bridge Pallet instance. /// diff --git a/bridges/primitives/runtime/Cargo.toml b/bridges/primitives/runtime/Cargo.toml index 2452a9f44e63..a5aaa1cfa644 100644 --- a/bridges/primitives/runtime/Cargo.toml +++ b/bridges/primitives/runtime/Cargo.toml @@ -13,6 +13,7 @@ num-traits = { version = "0.2", default-features = false } # Substrate Dependencies frame-support = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } +sp-core = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } sp-io = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } sp-runtime = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } sp-std = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } @@ -23,6 +24,7 @@ std = [ "codec/std", "frame-support/std", "num-traits/std", + "sp-core/std", "sp-io/std", "sp-runtime/std", "sp-std/std", diff --git a/bridges/primitives/runtime/src/lib.rs b/bridges/primitives/runtime/src/lib.rs index df0975f9815e..926761b4ba3c 100644 --- a/bridges/primitives/runtime/src/lib.rs +++ b/bridges/primitives/runtime/src/lib.rs @@ -18,7 +18,8 @@ #![cfg_attr(not(feature = "std"), no_std)] -use codec::{Decode, Encode}; +use codec::Encode; +use sp_core::hash::H256; use sp_io::hashing::blake2_256; pub use chain::{BlockNumberOf, Chain, HashOf, HasherOf, HeaderOf}; @@ -46,17 +47,45 @@ pub const MESSAGE_LANE_MODULE_PREFIX: &[u8] = b"pallet-bridge/message-lane"; /// to identify deployed instance dynamically. This type is used for that. pub type InstanceId = [u8; 4]; -/// Returns id of account that acts as "system" account of given bridge instance. -/// The `module_prefix` (arbitrary slice) may be used to generate module-level -/// "system" account, so you could have separate "system" accounts for currency -/// exchange, message dispatch and other modules. +/// Type of accounts on the source chain. +pub enum SourceAccount { + /// An account that belongs to Root (priviledged origin). + Root, + /// A non-priviledged account. + /// + /// The embedded account ID may or may not have a private key depending on the "owner" of the + /// account (private key, pallet, proxy, etc.). + Account(T), +} + +/// Derive an account ID from a foreign account ID. +/// +/// This function returns an encoded Blake2 hash. It is the responsibility of the caller to ensure +/// this can be succesfully decoded into an AccountId. +/// +/// The `bridge_id` is used to provide extra entropy when producing account IDs. This helps prevent +/// AccountId collisions between different bridges on a single target chain. /// -/// The account is not supposed to actually exists on the chain, or to have any funds. -/// It is only used to -pub fn bridge_account_id(bridge: InstanceId, module_prefix: &[u8]) -> AccountId +/// Note: If the same `bridge_id` is used across different chains (for example, if one source chain +/// is bridged to multiple target chains), then all the derived accounts would be the same across +/// the different chains. This could negatively impact users' privacy across chains. +pub fn derive_account_id(bridge_id: InstanceId, id: SourceAccount) -> H256 where - AccountId: Decode + Default, + AccountId: Encode, { - let entropy = (module_prefix, bridge).using_encoded(blake2_256); - AccountId::decode(&mut &entropy[..]).unwrap_or_default() + match id { + SourceAccount::Root => ("root", bridge_id).using_encoded(blake2_256), + SourceAccount::Account(id) => ("account", bridge_id, id).using_encoded(blake2_256), + } + .into() +} + +/// Derive the account ID of the shared relayer fund account. +/// +/// This account is used to collect fees for relayers that are passing messages across the bridge. +/// +/// The account ID can be the same across different instances of `message-lane` if the same +/// `bridge_id` is used. +pub fn derive_relayer_fund_account_id(bridge_id: InstanceId) -> H256 { + ("relayer-fund-account", bridge_id).using_encoded(blake2_256).into() } diff --git a/bridges/relays/substrate/src/main.rs b/bridges/relays/substrate/src/main.rs index 7d2f4a6dbfc6..e7812219d174 100644 --- a/bridges/relays/substrate/src/main.rs +++ b/bridges/relays/substrate/src/main.rs @@ -26,6 +26,7 @@ use relay_rialto_client::{Rialto, SigningParams as RialtoSigningParams}; use relay_substrate_client::{ConnectionParams, TransactionSignScheme}; use relay_utils::initialize::initialize_relay; use sp_core::{Bytes, Pair}; +use sp_runtime::traits::IdentifyAccount; /// Millau node client. pub type MillauClient = relay_substrate_client::Client; @@ -277,11 +278,12 @@ async fn run_command(command: cli::Command) -> Result<(), String> { let rialto_call_weight = rialto_call.get_dispatch_info().weight; let millau_sender_public: bp_millau::AccountSigner = millau_sign.signer.public().clone().into(); + let millau_account_id: bp_millau::AccountId = millau_sender_public.into_account(); let rialto_origin_public = rialto_sign.signer.public(); let mut rialto_origin_signature_message = Vec::new(); rialto_call.encode_to(&mut rialto_origin_signature_message); - millau_sender_public.encode_to(&mut rialto_origin_signature_message); + millau_account_id.encode_to(&mut rialto_origin_signature_message); let rialto_origin_signature = rialto_sign.signer.sign(&rialto_origin_signature_message); let millau_call = @@ -290,8 +292,8 @@ async fn run_command(command: cli::Command) -> Result<(), String> { MessagePayload { spec_version: rialto_runtime::VERSION.spec_version, weight: rialto_call_weight, - origin: CallOrigin::RealAccount( - millau_sender_public, + origin: CallOrigin::TargetAccount( + millau_account_id, rialto_origin_public.into(), rialto_origin_signature.into(), ), @@ -391,11 +393,12 @@ async fn run_command(command: cli::Command) -> Result<(), String> { let millau_call_weight = millau_call.get_dispatch_info().weight; let rialto_sender_public: bp_rialto::AccountSigner = rialto_sign.signer.public().clone().into(); + let rialto_account_id: bp_rialto::AccountId = rialto_sender_public.into_account(); let millau_origin_public = millau_sign.signer.public(); let mut millau_origin_signature_message = Vec::new(); millau_call.encode_to(&mut millau_origin_signature_message); - rialto_sender_public.encode_to(&mut millau_origin_signature_message); + rialto_account_id.encode_to(&mut millau_origin_signature_message); let millau_origin_signature = millau_sign.signer.sign(&millau_origin_signature_message); let rialto_call = @@ -404,8 +407,8 @@ async fn run_command(command: cli::Command) -> Result<(), String> { MessagePayload { spec_version: millau_runtime::VERSION.spec_version, weight: millau_call_weight, - origin: CallOrigin::RealAccount( - rialto_sender_public, + origin: CallOrigin::TargetAccount( + rialto_account_id, millau_origin_public.into(), millau_origin_signature.into(), ),