From 5aa0d7cf89b6a87ecdd48f15a3382710f0143367 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Sat, 27 Jul 2024 01:29:37 +0200 Subject: [PATCH] Make `LaneId` backwards compatible - part 1 https://github.com/paritytech/parity-bridges-common/issues/2500 https://github.com/paritytech/parity-bridges-common/issues/2794 --- bridges/primitives/messages/Cargo.toml | 1 + bridges/primitives/messages/src/lane.rs | 258 ++++++++++++++++++ bridges/primitives/messages/src/lib.rs | 158 +---------- .../primitives/runtime/src/storage_proof.rs | 2 +- .../relays/lib-substrate-relay/src/cli/mod.rs | 90 +++--- .../src/cli/relay_headers_and_messages/mod.rs | 3 +- .../src/cli/relay_messages.rs | 4 +- 7 files changed, 317 insertions(+), 199 deletions(-) create mode 100644 bridges/primitives/messages/src/lane.rs diff --git a/bridges/primitives/messages/Cargo.toml b/bridges/primitives/messages/Cargo.toml index 59e458a402dd..29514ecccc25 100644 --- a/bridges/primitives/messages/Cargo.toml +++ b/bridges/primitives/messages/Cargo.toml @@ -28,6 +28,7 @@ sp-io = { workspace = true } [dev-dependencies] hex = { workspace = true, default-features = true } hex-literal = { workspace = true, default-features = true } +bp-runtime = { features = ["test-helpers"], workspace = true } [features] default = ["std"] diff --git a/bridges/primitives/messages/src/lane.rs b/bridges/primitives/messages/src/lane.rs new file mode 100644 index 000000000000..cd085096f1b6 --- /dev/null +++ b/bridges/primitives/messages/src/lane.rs @@ -0,0 +1,258 @@ +// Copyright (C) Parity Technologies (UK) Ltd. +// This file is part of Parity Bridges Common. + +// Parity Bridges Common is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Parity Bridges Common is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Parity Bridges Common. If not, see . + +//! Primitives of messages module, that represents lane id. + +use codec::{Decode, Encode, Error as CodecError, Input, MaxEncodedLen}; +use frame_support::sp_runtime::Either; +use scale_info::TypeInfo; +use serde::{Deserialize, Serialize}; +use sp_core::{RuntimeDebug, TypeId, H256}; +use sp_io::hashing::blake2_256; + +/// Bridge lane identifier. +/// +/// Lane connects two endpoints at both sides of the bridge. We assume that every endpoint +/// has its own unique identifier. We want lane identifiers to be the same on the both sides +/// of the bridge (and naturally unique across global consensus if endpoints have unique +/// identifiers). So lane id is the hash (`blake2_256`) of **ordered** encoded locations +/// concatenation (separated by some binary data). I.e.: +/// +/// ```nocompile +/// let endpoint1 = X2(GlobalConsensus(NetworkId::Rococo), Parachain(42)); +/// let endpoint2 = X2(GlobalConsensus(NetworkId::Wococo), Parachain(777)); +/// +/// let final_lane_key = if endpoint1 < endpoint2 { +/// (endpoint1, VALUES_SEPARATOR, endpoint2) +/// } else { +/// (endpoint2, VALUES_SEPARATOR, endpoint1) +/// }.using_encoded(blake2_256); +/// ``` +/// +/// Note: For backwards compatibility reasons, we also handle the older format `[u8; 4]`. +#[derive( + Clone, + Copy, + Decode, + Encode, + Eq, + Ord, + PartialOrd, + PartialEq, + TypeInfo, + MaxEncodedLen, + Serialize, + Deserialize, +)] +pub struct LaneId(InnerLaneId); + +impl LaneId { + /// Create lane identifier from two locations. + pub fn new(endpoint1: T, endpoint2: T) -> Self { + const VALUES_SEPARATOR: [u8; 31] = *b"bridges-lane-id-value-separator"; + + LaneId(InnerLaneId::Hash( + if endpoint1 < endpoint2 { + (endpoint1, VALUES_SEPARATOR, endpoint2) + } else { + (endpoint2, VALUES_SEPARATOR, endpoint1) + } + .using_encoded(blake2_256) + .into(), + )) + } + + /// Create lane identifier from given hash. + /// + /// There's no `From` implementation for the `LaneId`, because using this conversion + /// in a wrong way (i.e. computing hash of endpoints manually) may lead to issues. So we + /// want the call to be explicit. + pub const fn from_inner(inner: Either) -> Self { + LaneId(match inner { + Either::Left(hash) => InnerLaneId::Hash(hash), + Either::Right(array) => InnerLaneId::Array(array), + }) + } +} + +impl core::fmt::Display for LaneId { + fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { + core::fmt::Display::fmt(&self.0, f) + } +} + +impl core::fmt::Debug for LaneId { + fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { + core::fmt::Debug::fmt(&self.0, f) + } +} + +impl AsRef<[u8]> for LaneId { + fn as_ref(&self) -> &[u8] { + self.0.as_ref() + } +} + +impl TypeId for LaneId { + const TYPE_ID: [u8; 4] = *b"blan"; +} + +#[derive( + Clone, Copy, Eq, Ord, PartialOrd, PartialEq, TypeInfo, MaxEncodedLen, Serialize, Deserialize, +)] +enum InnerLaneId { + /// Old format (for backwards compatibility). + Array([u8; 4]), + /// New format 32-byte hash generated by `blake2_256`. + Hash(H256), +} + +impl Encode for InnerLaneId { + fn encode(&self) -> Vec { + match self { + InnerLaneId::Array(array) => array.encode(), + InnerLaneId::Hash(hash) => hash.encode(), + } + } +} + +impl Decode for InnerLaneId { + fn decode(input: &mut I) -> Result { + // check backwards compatibly first + if input.remaining_len() == Ok(Some(4)) { + let array: [u8; 4] = Decode::decode(input)?; + return Ok(InnerLaneId::Array(array)) + } + + // else check new format + H256::decode(input).map(InnerLaneId::Hash) + } +} + +impl core::fmt::Display for InnerLaneId { + fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { + match self { + InnerLaneId::Array(array) => write!(f, "Array({:?})", array), + InnerLaneId::Hash(hash) => write!(f, "Hash({:?})", hash), + } + } +} + +impl core::fmt::Debug for InnerLaneId { + fn fmt(&self, fmt: &mut core::fmt::Formatter) -> core::fmt::Result { + match self { + InnerLaneId::Array(array) => array.fmt(fmt), + InnerLaneId::Hash(hash) => hash.fmt(fmt), + } + } +} + +impl AsRef<[u8]> for InnerLaneId { + fn as_ref(&self) -> &[u8] { + match self { + InnerLaneId::Array(array) => array.as_ref(), + InnerLaneId::Hash(hash) => hash.as_ref(), + } + } +} + +/// Lane state. +#[derive(Clone, Copy, Decode, Encode, Eq, PartialEq, TypeInfo, MaxEncodedLen, RuntimeDebug)] +pub enum LaneState { + /// Lane is opened and messages may be sent/received over it. + Opened, + /// Lane is closed and all attempts to send/receive messages to/from this lane + /// will fail. + /// + /// Keep in mind that the lane has two ends and the state of the same lane at + /// its ends may be different. Those who are controlling/serving the lane + /// and/or sending messages over the lane, have to coordinate their actions on + /// both ends to make sure that lane is operating smoothly on both ends. + Closed, +} + +impl LaneState { + /// Returns true if lane state allows sending/receiving messages. + pub fn is_active(&self) -> bool { + matches!(*self, LaneState::Opened) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn lane_id_debug_format_matches_inner_hash_format() { + assert_eq!( + format!("{:?}", LaneId(InnerLaneId::Hash(H256::from([1u8; 32])))), + format!("{:?}", H256::from([1u8; 32])), + ); + assert_eq!( + format!("{:?}", LaneId(InnerLaneId::Array([0, 0, 0, 1]))), + format!("{:?}", [0, 0, 0, 1]), + ); + } + + #[test] + fn lane_id_as_ref_works() { + assert_eq!( + "0101010101010101010101010101010101010101010101010101010101010101", + hex::encode(LaneId(InnerLaneId::Hash(H256::from([1u8; 32]))).as_ref()), + ); + assert_eq!("00000001", hex::encode(LaneId(InnerLaneId::Array([0, 0, 0, 1])).as_ref()),); + } + + #[test] + fn lane_id_is_generated_using_ordered_endpoints() { + assert_eq!(LaneId::new(1, 2), LaneId::new(2, 1)); + } + + #[test] + fn lane_id_is_different_for_different_endpoints() { + assert_ne!(LaneId::new(1, 2), LaneId::new(1, 3)); + } + + #[test] + fn lane_id_is_different_even_if_arguments_has_partial_matching_encoding() { + /// Some artificial type that generates the same encoding for different values + /// concatenations. I.e. the encoding for `(Either::Two(1, 2), Either::Two(3, 4))` + /// is the same as encoding of `(Either::Three(1, 2, 3), Either::One(4))`. + /// In practice, this type is not useful, because you can't do a proper decoding. + /// But still there may be some collisions even in proper types. + #[derive(Eq, Ord, PartialEq, PartialOrd)] + enum Either { + Three(u64, u64, u64), + Two(u64, u64), + One(u64), + } + + impl codec::Encode for Either { + fn encode(&self) -> Vec { + match *self { + Self::One(a) => a.encode(), + Self::Two(a, b) => (a, b).encode(), + Self::Three(a, b, c) => (a, b, c).encode(), + } + } + } + + assert_ne!( + LaneId::new(Either::Two(1, 2), Either::Two(3, 4)), + LaneId::new(Either::Three(1, 2, 3), Either::One(4)), + ); + } +} diff --git a/bridges/primitives/messages/src/lib.rs b/bridges/primitives/messages/src/lib.rs index 1aa1792a863e..7eb0c5629395 100644 --- a/bridges/primitives/messages/src/lib.rs +++ b/bridges/primitives/messages/src/lib.rs @@ -31,17 +31,17 @@ pub use frame_support::weights::Weight; use scale_info::TypeInfo; use serde::{Deserialize, Serialize}; use source_chain::RelayersRewards; -use sp_core::{RuntimeDebug, TypeId, H256}; -use sp_io::hashing::blake2_256; +use sp_core::RuntimeDebug; use sp_std::{collections::vec_deque::VecDeque, ops::RangeInclusive, prelude::*}; pub use call_info::{ BaseMessagesProofInfo, BridgeMessagesCall, BridgeMessagesCallOf, MessagesCallInfo, ReceiveMessagesDeliveryProofInfo, ReceiveMessagesProofInfo, UnrewardedRelayerOccupation, }; +pub use lane::{LaneId, LaneState}; mod call_info; - +mod lane; pub mod source_chain; pub mod storage_keys; pub mod target_chain; @@ -173,110 +173,6 @@ impl OperatingMode for MessagesOperatingMode { } } -/// Bridge lane identifier. -/// -/// Lane connects two endpoints at both sides of the bridge. We assume that every endpoint -/// has its own unique identifier. We want lane identifiers to be the same on the both sides -/// of the bridge (and naturally unique across global consensus if endpoints have unique -/// identifiers). So lane id is the hash (`blake2_256`) of **ordered** encoded locations -/// concatenation (separated by some binary data). I.e.: -/// -/// ```nocompile -/// let endpoint1 = X2(GlobalConsensus(NetworkId::Rococo), Parachain(42)); -/// let endpoint2 = X2(GlobalConsensus(NetworkId::Wococo), Parachain(777)); -/// -/// let final_lane_key = if endpoint1 < endpoint2 { -/// (endpoint1, VALUES_SEPARATOR, endpoint2) -/// } else { -/// (endpoint2, VALUES_SEPARATOR, endpoint1) -/// }.using_encoded(blake2_256); -/// ``` -#[derive( - Clone, - Copy, - Decode, - Encode, - Eq, - Ord, - PartialOrd, - PartialEq, - TypeInfo, - MaxEncodedLen, - Serialize, - Deserialize, -)] -pub struct LaneId(H256); - -impl LaneId { - /// Create lane identifier from two locations. - pub fn new(endpoint1: T, endpoint2: T) -> Self { - const VALUES_SEPARATOR: [u8; 31] = *b"bridges-lane-id-value-separator"; - - LaneId( - if endpoint1 < endpoint2 { - (endpoint1, VALUES_SEPARATOR, endpoint2) - } else { - (endpoint2, VALUES_SEPARATOR, endpoint1) - } - .using_encoded(blake2_256) - .into(), - ) - } - - /// Create lane identifier from given hash. - /// - /// There's no `From` implementation for the `LaneId`, because using this conversion - /// in a wrong way (i.e. computing hash of endpoints manually) may lead to issues. So we - /// want the call to be explicit. - pub const fn from_inner(hash: H256) -> Self { - LaneId(hash) - } -} - -impl core::fmt::Display for LaneId { - fn fmt(&self, fmt: &mut core::fmt::Formatter) -> core::fmt::Result { - self.0.fmt(fmt) - } -} - -impl core::fmt::Debug for LaneId { - fn fmt(&self, fmt: &mut core::fmt::Formatter) -> core::fmt::Result { - self.0.fmt(fmt) - } -} - -impl AsRef for LaneId { - fn as_ref(&self) -> &H256 { - &self.0 - } -} - -impl TypeId for LaneId { - const TYPE_ID: [u8; 4] = *b"blan"; -} - -/// Lane state. -#[derive(Clone, Copy, Decode, Encode, Eq, PartialEq, TypeInfo, MaxEncodedLen, RuntimeDebug)] -pub enum LaneState { - /// Lane is opened and messages may be sent/received over it. - Opened, - /// Lane is closed and all attempts to send/receive messages to/from this lane - /// will fail. - /// - /// Keep in mind that the lane has two ends and the state of the same lane at - /// its ends may be different. Those who are controlling/serving the lane - /// and/or sending messages over the lane, have to coordinate their actions on - /// both ends to make sure that lane is operating smoothly on both ends. - Closed, -} - -impl LaneState { - /// Returns true if lane state allows sending/receiving messages. - pub fn is_active(&self) -> bool { - matches!(*self, LaneState::Opened) - } -} - /// Message nonce. Valid messages will never have 0 nonce. pub type MessageNonce = u64; @@ -709,52 +605,4 @@ mod tests { assert!(delivered_messages.contains_message(150)); assert!(!delivered_messages.contains_message(151)); } - - #[test] - fn lane_id_debug_format_matches_inner_hash_format() { - assert_eq!( - format!("{:?}", LaneId(H256::from([1u8; 32]))), - format!("{:?}", H256::from([1u8; 32])), - ); - } - - #[test] - fn lane_id_is_generated_using_ordered_endpoints() { - assert_eq!(LaneId::new(1, 2), LaneId::new(2, 1)); - } - - #[test] - fn lane_id_is_different_for_different_endpoints() { - assert_ne!(LaneId::new(1, 2), LaneId::new(1, 3)); - } - - #[test] - fn lane_id_is_different_even_if_arguments_has_partial_matching_encoding() { - /// Some artificial type that generates the same encoding for different values - /// concatenations. I.e. the encoding for `(Either::Two(1, 2), Either::Two(3, 4))` - /// is the same as encoding of `(Either::Three(1, 2, 3), Either::One(4))`. - /// In practice, this type is not useful, because you can't do a proper decoding. - /// But still there may be some collisions even in proper types. - #[derive(Eq, Ord, PartialEq, PartialOrd)] - enum Either { - Three(u64, u64, u64), - Two(u64, u64), - One(u64), - } - - impl codec::Encode for Either { - fn encode(&self) -> Vec { - match *self { - Self::One(a) => a.encode(), - Self::Two(a, b) => (a, b).encode(), - Self::Three(a, b, c) => (a, b, c).encode(), - } - } - } - - assert_ne!( - LaneId::new(Either::Two(1, 2), Either::Two(3, 4)), - LaneId::new(Either::Three(1, 2, 3), Either::One(4)), - ); - } } diff --git a/bridges/primitives/runtime/src/storage_proof.rs b/bridges/primitives/runtime/src/storage_proof.rs index 7bfa0d6fde01..53efd572b593 100644 --- a/bridges/primitives/runtime/src/storage_proof.rs +++ b/bridges/primitives/runtime/src/storage_proof.rs @@ -280,7 +280,7 @@ where /// Return valid storage proof and state root. /// -/// NOTE: This should only be used for **testing**. +/// Note: This should only be used for **testing**. #[cfg(feature = "std")] pub fn craft_valid_storage_proof() -> (sp_core::H256, RawStorageProof) { use sp_state_machine::{backend::Backend, prove_read, InMemoryBackend}; diff --git a/bridges/relays/lib-substrate-relay/src/cli/mod.rs b/bridges/relays/lib-substrate-relay/src/cli/mod.rs index 2882d230f6aa..ef8403ff68ee 100644 --- a/bridges/relays/lib-substrate-relay/src/cli/mod.rs +++ b/bridges/relays/lib-substrate-relay/src/cli/mod.rs @@ -16,14 +16,14 @@ //! Deal with CLI args of substrate-to-substrate relay. -use codec::{Decode, Encode}; +use bp_messages::LaneId; use rbtag::BuildInfo; use sp_core::H256; +use sp_runtime::Either; +use std::str::FromStr; use structopt::StructOpt; use strum::{EnumString, VariantNames}; -use bp_messages::LaneId; - pub mod bridge; pub mod chain_schema; pub mod detect_equivocations; @@ -43,7 +43,7 @@ pub type DefaultClient = relay_substrate_client::RpcWithCachingClient; /// Lane id. #[derive(Debug, Clone, PartialEq, Eq)] -pub struct HexLaneId(pub H256); +pub struct HexLaneId(Either); impl From for LaneId { fn from(lane_id: HexLaneId) -> LaneId { @@ -51,35 +51,28 @@ impl From for LaneId { } } -impl std::str::FromStr for HexLaneId { +impl FromStr for HexLaneId { type Err = rustc_hex::FromHexError; fn from_str(s: &str) -> Result { - Ok(HexLaneId(H256::from_str(s)?)) - } -} - -/// Nicer formatting for raw bytes vectors. -#[derive(Default, Encode, Decode, PartialEq, Eq)] -pub struct HexBytes(pub Vec); - -impl std::str::FromStr for HexBytes { - type Err = hex::FromHexError; - - fn from_str(s: &str) -> Result { - Ok(Self(hex::decode(s)?)) - } -} - -impl std::fmt::Debug for HexBytes { - fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { - write!(fmt, "0x{self}") - } -} - -impl std::fmt::Display for HexBytes { - fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { - write!(fmt, "{}", hex::encode(&self.0)) + // check `H256` variant at first + match H256::from_str(s) { + Ok(hash) => Ok(HexLaneId(Either::Left(hash))), + Err(hash_error) => { + // check backwards compatible + let mut lane_id = [0u8; 4]; + match hex::decode_to_slice(s, &mut lane_id) { + Ok(_) => Ok(HexLaneId(Either::Right(lane_id))), + Err(array_error) => { + log::error!( + target: "bridge", + "Failed to parse `HexLaneId` as hex string: {s:?} - hash_error: {hash_error:?}, array_error: {array_error:?}", + ); + Err(hash_error) + }, + } + }, + } } } @@ -181,15 +174,32 @@ mod tests { use super::*; #[test] - fn hex_bytes_display_matches_from_str_for_clap() { - // given - let hex = HexBytes(vec![1, 2, 3, 4]); - let display = format!("{hex}"); - - // when - let hex2: HexBytes = display.parse().unwrap(); - - // then - assert_eq!(hex.0, hex2.0); + fn hex_lane_id_from_str_works() { + // hash variant + assert!(HexLaneId::from_str( + "101010101010101010101010101010101010101010101010101010101010101" + ) + .is_err()); + assert!(HexLaneId::from_str( + "00101010101010101010101010101010101010101010101010101010101010101" + ) + .is_err()); + assert_eq!( + LaneId::from( + HexLaneId::from_str( + "0101010101010101010101010101010101010101010101010101010101010101" + ) + .unwrap() + ), + LaneId::from_inner(Either::Left(H256::from([1u8; 32]))) + ); + + // array variant + assert!(HexLaneId::from_str("0000001").is_err()); + assert!(HexLaneId::from_str("000000001").is_err()); + assert_eq!( + LaneId::from(HexLaneId::from_str("00000001").unwrap()), + LaneId::from_inner(Either::Right([0, 0, 0, 1])) + ); } } diff --git a/bridges/relays/lib-substrate-relay/src/cli/relay_headers_and_messages/mod.rs b/bridges/relays/lib-substrate-relay/src/cli/relay_headers_and_messages/mod.rs index 8e524fe22dec..3786976bed9b 100644 --- a/bridges/relays/lib-substrate-relay/src/cli/relay_headers_and_messages/mod.rs +++ b/bridges/relays/lib-substrate-relay/src/cli/relay_headers_and_messages/mod.rs @@ -360,6 +360,7 @@ mod tests { use relay_substrate_client::{ChainRuntimeVersion, Parachain, SimpleRuntimeVersion}; use sp_core::H256; + use sp_runtime::Either; #[test] // We need `#[allow(dead_code)]` because some of the methods generated by the macros @@ -433,7 +434,7 @@ mod tests { res, BridgeHubKusamaBridgeHubPolkadotHeadersAndMessages { shared: HeadersAndMessagesSharedParams { - lane: vec![HexLaneId(H256::from([0x00u8; 32]))], + lane: vec![HexLaneId(Either::Left(H256::from([0x00u8; 32])))], only_mandatory_headers: false, only_free_headers: false, prometheus_params: PrometheusParams { diff --git a/bridges/relays/lib-substrate-relay/src/cli/relay_messages.rs b/bridges/relays/lib-substrate-relay/src/cli/relay_messages.rs index 1771155bbcd3..34d5226e90c5 100644 --- a/bridges/relays/lib-substrate-relay/src/cli/relay_messages.rs +++ b/bridges/relays/lib-substrate-relay/src/cli/relay_messages.rs @@ -88,8 +88,8 @@ pub struct RelayMessagesDeliveryConfirmationParams { /// delivery proof. This header must be previously proved to the source chain. #[structopt(long)] at_target_block: u128, - /// Hex-encoded lane id that should be served by the relay. Defaults to `00000000`. - #[structopt(long, default_value = "00000000")] + /// Hex-encoded lane id that should be served by the relay. + #[structopt(long)] lane: HexLaneId, #[structopt(flatten)] source: SourceConnectionParams,