From 6bcfede165846a5cf96166052e4616ee908d4e7c Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Wed, 4 Sep 2024 23:13:22 +0200 Subject: [PATCH] Fix `LaneId` encode/decode for backwards compatibility --- bridges/modules/messages/src/lib.rs | 13 +- .../messages/src/tests/pallet_tests.rs | 7 +- bridges/modules/xcm-bridge-hub/src/lib.rs | 22 +-- bridges/primitives/messages/src/lane.rs | 152 ++++++++++++------ bridges/primitives/messages/src/lib.rs | 6 +- .../primitives/messages/src/storage_keys.rs | 6 +- bridges/primitives/relayers/src/lib.rs | 8 +- 7 files changed, 138 insertions(+), 76 deletions(-) diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs index b7fe1c7dbb19..4ef2818f271c 100644 --- a/bridges/modules/messages/src/lib.rs +++ b/bridges/modules/messages/src/lib.rs @@ -97,7 +97,7 @@ pub const LOG_TARGET: &str = "runtime::bridge-messages"; #[frame_support::pallet] pub mod pallet { use super::*; - use bp_messages::{ReceivedMessages, ReceptionResult}; + use bp_messages::{LaneIdBytes, ReceivedMessages, ReceptionResult}; use bp_runtime::RangeInclusiveExt; use frame_support::pallet_prelude::*; use frame_system::pallet_prelude::*; @@ -387,7 +387,7 @@ pub mod pallet { // emit 'delivered' event let received_range = confirmed_messages.begin..=confirmed_messages.end; Self::deposit_event(Event::MessagesDelivered { - lane_id, + lane_id: lane_id.into(), messages: confirmed_messages, }); @@ -441,7 +441,7 @@ pub mod pallet { /// Message has been accepted and is waiting to be delivered. MessageAccepted { /// Lane, which has accepted the message. - lane_id: LaneId, + lane_id: LaneIdBytes, /// Nonce of accepted message. nonce: MessageNonce, }, @@ -453,7 +453,7 @@ pub mod pallet { /// Messages in the inclusive range have been delivered to the bridged chain. MessagesDelivered { /// Lane for which the delivery has been confirmed. - lane_id: LaneId, + lane_id: LaneIdBytes, /// Delivered messages. messages: DeliveredMessages, }, @@ -703,7 +703,10 @@ where message_len, ); - Pallet::::deposit_event(Event::MessageAccepted { lane_id: args.lane_id, nonce }); + Pallet::::deposit_event(Event::MessageAccepted { + lane_id: args.lane_id.into(), + nonce, + }); SendMessageArtifacts { nonce, enqueued_messages } } diff --git a/bridges/modules/messages/src/tests/pallet_tests.rs b/bridges/modules/messages/src/tests/pallet_tests.rs index ceb1744c0665..d2391cd1775d 100644 --- a/bridges/modules/messages/src/tests/pallet_tests.rs +++ b/bridges/modules/messages/src/tests/pallet_tests.rs @@ -67,7 +67,10 @@ fn send_regular_message(lane_id: LaneId) { System::::events(), vec![EventRecord { phase: Phase::Initialization, - event: TestEvent::Messages(Event::MessageAccepted { lane_id, nonce: message_nonce }), + event: TestEvent::Messages(Event::MessageAccepted { + lane_id: lane_id.into(), + nonce: message_nonce + }), topics: vec![], }], ); @@ -105,7 +108,7 @@ fn receive_messages_delivery_proof() { vec![EventRecord { phase: Phase::Initialization, event: TestEvent::Messages(Event::MessagesDelivered { - lane_id: test_lane_id(), + lane_id: test_lane_id().into(), messages: DeliveredMessages::new(1), }), topics: vec![], diff --git a/bridges/modules/xcm-bridge-hub/src/lib.rs b/bridges/modules/xcm-bridge-hub/src/lib.rs index f28e3e5967c9..02e6753c3755 100644 --- a/bridges/modules/xcm-bridge-hub/src/lib.rs +++ b/bridges/modules/xcm-bridge-hub/src/lib.rs @@ -143,7 +143,7 @@ #![warn(missing_docs)] #![cfg_attr(not(feature = "std"), no_std)] -use bp_messages::{LaneId, LaneState, MessageNonce}; +use bp_messages::{LaneId, LaneIdBytes, LaneState, MessageNonce}; use bp_runtime::{AccountIdOf, BalanceOf, RangeInclusiveExt}; pub use bp_xcm_bridge_hub::{Bridge, BridgeId, BridgeState}; use bp_xcm_bridge_hub::{BridgeLocations, BridgeLocationsError, LocalXcmChannelManager}; @@ -391,7 +391,7 @@ pub mod pallet { // deposit the `ClosingBridge` event Self::deposit_event(Event::::ClosingBridge { bridge_id: *locations.bridge_id(), - lane_id: bridge.lane_id, + lane_id: bridge.lane_id.into(), pruned_messages, enqueued_messages, }); @@ -438,7 +438,7 @@ pub mod pallet { // deposit the `BridgePruned` event Self::deposit_event(Event::::BridgePruned { bridge_id: *locations.bridge_id(), - lane_id: bridge.lane_id, + lane_id: bridge.lane_id.into(), bridge_deposit: released_deposit, pruned_messages, }); @@ -541,7 +541,7 @@ pub mod pallet { remote_endpoint: Box::new( locations.bridge_destination_universal_location().clone(), ), - lane_id, + lane_id: lane_id.into(), }); Ok(()) @@ -805,14 +805,14 @@ pub mod pallet { /// Universal location of remote bridge endpoint. remote_endpoint: Box, /// Lane identifier. - lane_id: LaneId, + lane_id: LaneIdBytes, }, /// Bridge is going to be closed, but not yet fully pruned from the runtime storage. ClosingBridge { /// Bridge identifier. bridge_id: BridgeId, /// Lane identifier. - lane_id: LaneId, + lane_id: LaneIdBytes, /// Number of pruned messages during the close call. pruned_messages: MessageNonce, /// Number of enqueued messages that need to be pruned in follow up calls. @@ -824,7 +824,7 @@ pub mod pallet { /// Bridge identifier. bridge_id: BridgeId, /// Lane identifier. - lane_id: LaneId, + lane_id: LaneIdBytes, /// Amount of deposit released. bridge_deposit: BalanceOf>, /// Number of pruned messages during the close call. @@ -1221,7 +1221,7 @@ mod tests { remote_endpoint: Box::new( locations.bridge_destination_universal_location().clone() ), - lane_id + lane_id: lane_id.into() }), topics: vec![], }), @@ -1364,7 +1364,7 @@ mod tests { phase: Phase::Initialization, event: RuntimeEvent::XcmOverBridge(Event::ClosingBridge { bridge_id: *locations.bridge_id(), - lane_id: bridge.lane_id, + lane_id: bridge.lane_id.into(), pruned_messages: 16, enqueued_messages: 16, }), @@ -1412,7 +1412,7 @@ mod tests { phase: Phase::Initialization, event: RuntimeEvent::XcmOverBridge(Event::ClosingBridge { bridge_id: *locations.bridge_id(), - lane_id: bridge.lane_id, + lane_id: bridge.lane_id.into(), pruned_messages: 8, enqueued_messages: 8, }), @@ -1453,7 +1453,7 @@ mod tests { phase: Phase::Initialization, event: RuntimeEvent::XcmOverBridge(Event::BridgePruned { bridge_id: *locations.bridge_id(), - lane_id: bridge.lane_id, + lane_id: bridge.lane_id.into(), bridge_deposit: expected_deposit, pruned_messages: 8, }), diff --git a/bridges/primitives/messages/src/lane.rs b/bridges/primitives/messages/src/lane.rs index 41e38517cea9..9f3b6d687bfd 100644 --- a/bridges/primitives/messages/src/lane.rs +++ b/bridges/primitives/messages/src/lane.rs @@ -108,53 +108,66 @@ impl core::fmt::Debug for LaneId { } } -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, -)] +#[derive(Clone, Copy, Eq, Ord, PartialOrd, PartialEq, TypeInfo, Serialize, Deserialize)] enum InnerLaneId { - /// Old format (for backwards compatibility). - Array([u8; 4]), /// New format 32-byte hash generated by `blake2_256`. Hash(H256), + /// Old format (for backwards compatibility). + Array([u8; 4]), +} + +// Prefix used to differentiate `LaneId` for backward compatibility. +// Hex value: `48615368` +const INNER_LANE_ID_AS_HASH_PREFIX: &[u8; 4] = b"HaSh"; + +impl MaxEncodedLen for InnerLaneId { + fn max_encoded_len() -> usize { + INNER_LANE_ID_AS_HASH_PREFIX + .encoded_size() + .saturating_add(H256::max_encoded_len()) + } } impl Encode for InnerLaneId { fn encode(&self) -> sp_std::vec::Vec { match self { - InnerLaneId::Array(array) => array.encode(), - InnerLaneId::Hash(hash) => hash.encode(), + InnerLaneId::Hash(hash) => { + // prefix new hash, so we can easily decode + (INNER_LANE_ID_AS_HASH_PREFIX, hash).encode() + }, + InnerLaneId::Array(array) => { + // encode backwards compatible + array.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)) + // read 4 bytes first + let prefix_or_array: [u8; 4] = Decode::decode(input)?; + + // if matches prefix, it is a new format + if prefix_or_array.eq(INNER_LANE_ID_AS_HASH_PREFIX) { + // now read more 32 bytes for hash + return H256::decode(input).map(InnerLaneId::Hash) } - // else check new format - H256::decode(input).map(InnerLaneId::Hash) + // return prefix `[u8; 4]` for backwards compatibly as a best effort + Ok(InnerLaneId::Array(prefix_or_array)) } } 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), + InnerLaneId::Array(array) => write!(f, "InnerLaneId::Array({:?})", array), + InnerLaneId::Hash(hash) => write!(f, "InnerLaneId::Hash({:?})", hash), } } } @@ -168,11 +181,14 @@ impl core::fmt::Debug for InnerLaneId { } } -impl AsRef<[u8]> for InnerLaneId { - fn as_ref(&self) -> &[u8] { - match self { - InnerLaneId::Array(array) => array.as_ref(), - InnerLaneId::Hash(hash) => hash.as_ref(), +/// The representation of `LaneId`'s inner bytes. +#[derive(Clone, Encode, Decode, RuntimeDebug, PartialEq, Eq, TypeInfo)] +pub struct LaneIdBytes(sp_std::vec::Vec); +impl From for LaneIdBytes { + fn from(lane_id: LaneId) -> Self { + match lane_id.inner() { + Either::Left(hash) => Self(hash.as_bytes().to_vec()), + Either::Right(array) => Self(array.to_vec()), } } } @@ -202,6 +218,7 @@ impl LaneState { #[cfg(test)] mod tests { use super::*; + use crate::MessageNonce; #[test] fn lane_id_debug_format_matches_inner_hash_format() { @@ -216,35 +233,74 @@ mod tests { } #[test] - fn lane_id_as_ref_works() { + fn encode_decode_works() { + // simple encode/decode - new format + let lane_id = LaneId(InnerLaneId::Hash(H256::from([1u8; 32]))); + let encoded_lane_id = lane_id.encode(); + let decoded_lane_id = LaneId::decode(&mut &encoded_lane_id[..]).expect("decodable"); + assert_eq!(lane_id, decoded_lane_id); assert_eq!( - "0101010101010101010101010101010101010101010101010101010101010101", - hex::encode(LaneId(InnerLaneId::Hash(H256::from([1u8; 32]))).as_ref()), + "486153680101010101010101010101010101010101010101010101010101010101010101", + hex::encode(encoded_lane_id) ); - assert_eq!("00000001", hex::encode(LaneId(InnerLaneId::Array([0, 0, 0, 1])).as_ref()),); + + // simple encode/decode - old format + let lane_id = LaneId(InnerLaneId::Array([0, 0, 0, 1])); + let encoded_lane_id = lane_id.encode(); + let decoded_lane_id = LaneId::decode(&mut &encoded_lane_id[..]).expect("decodable"); + assert_eq!(lane_id, decoded_lane_id); + assert_eq!("00000001", hex::encode(encoded_lane_id)); + + // decode sample + let bytes = vec![0, 0, 0, 2, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0]; + let (lane, nonce_start, nonce_end): (LaneId, MessageNonce, MessageNonce) = + Decode::decode(&mut &bytes[..]).unwrap(); + assert_eq!(lane, LaneId(InnerLaneId::Array([0, 0, 0, 2]))); + assert_eq!(nonce_start, 1); + assert_eq!(nonce_end, 1); + + // run encode/decode for `LaneId` with different positions + let test_data = vec![ + (LaneId(InnerLaneId::Array([0, 0, 0, 1])), 1088_u64, 9185_u64), + (LaneId(InnerLaneId::Hash(H256::from([1u8; 32]))), 1088_u64, 9185_u64), + ]; + for (expected_lane, expected_nonce_start, expected_nonce_end) in test_data { + // decode: LaneId,Nonce,Nonce + let bytes = (expected_lane, expected_nonce_start, expected_nonce_end).encode(); + let (lane, nonce_start, nonce_end): (LaneId, MessageNonce, MessageNonce) = + Decode::decode(&mut &bytes[..]).unwrap(); + assert_eq!(lane, expected_lane); + assert_eq!(nonce_start, expected_nonce_start); + assert_eq!(nonce_end, expected_nonce_end); + + // decode: Nonce,LaneId,Nonce + let bytes = (expected_nonce_start, expected_lane, expected_nonce_end).encode(); + let (nonce_start, lane, nonce_end): (MessageNonce, LaneId, MessageNonce) = + Decode::decode(&mut &bytes[..]).unwrap(); + assert_eq!(lane, expected_lane); + assert_eq!(nonce_start, expected_nonce_start); + assert_eq!(nonce_end, expected_nonce_end); + + // decode: Nonce,Nonce,LaneId + let bytes = (expected_nonce_start, expected_nonce_end, expected_lane).encode(); + let (nonce_start, nonce_end, lane): (MessageNonce, MessageNonce, LaneId) = + Decode::decode(&mut &bytes[..]).unwrap(); + assert_eq!(lane, expected_lane); + assert_eq!(nonce_start, expected_nonce_start); + assert_eq!(nonce_end, expected_nonce_end); + } } #[test] - fn lane_id_encode_decode_works() { - let test_encode_decode = |expected_hex, lane_id: LaneId| { - let enc = lane_id.encode(); - let decoded_lane_id = LaneId::decode(&mut &enc[..]).expect("decodable"); - assert_eq!(lane_id, decoded_lane_id); - - assert_eq!(expected_hex, hex::encode(lane_id.as_ref()),); - assert_eq!(expected_hex, hex::encode(decoded_lane_id.as_ref()),); - - let hex_bytes = hex::decode(expected_hex).expect("valid hex"); - let hex_decoded_lane_id = LaneId::decode(&mut &hex_bytes[..]).expect("decodable"); - assert_eq!(hex_decoded_lane_id, lane_id); - assert_eq!(hex_decoded_lane_id, decoded_lane_id); - }; - - test_encode_decode( + fn lane_id_bytes_works() { + let lane_id_bytes: LaneIdBytes = LaneId(InnerLaneId::Array([0, 0, 0, 1])).into(); + assert_eq!("00000001", hex::encode(lane_id_bytes.0)); + + let lane_id_bytes: LaneIdBytes = LaneId(InnerLaneId::Hash(H256::from([1u8; 32]))).into(); + assert_eq!( "0101010101010101010101010101010101010101010101010101010101010101", - LaneId(InnerLaneId::Hash(H256::from([1u8; 32]))), + hex::encode(lane_id_bytes.0) ); - test_encode_decode("00000001", LaneId(InnerLaneId::Array([0, 0, 0, 1]))); } #[test] diff --git a/bridges/primitives/messages/src/lib.rs b/bridges/primitives/messages/src/lib.rs index 7eb0c5629395..f8812c6a1952 100644 --- a/bridges/primitives/messages/src/lib.rs +++ b/bridges/primitives/messages/src/lib.rs @@ -38,7 +38,7 @@ pub use call_info::{ BaseMessagesProofInfo, BridgeMessagesCall, BridgeMessagesCallOf, MessagesCallInfo, ReceiveMessagesDeliveryProofInfo, ReceiveMessagesProofInfo, UnrewardedRelayerOccupation, }; -pub use lane::{LaneId, LaneState}; +pub use lane::{LaneId, LaneIdBytes, LaneState}; mod call_info; mod lane; @@ -339,7 +339,7 @@ pub struct UnrewardedRelayer { #[derive(Clone, Encode, Decode, RuntimeDebug, PartialEq, Eq, TypeInfo)] pub struct ReceivedMessages { /// Id of the lane which is receiving messages. - pub lane: LaneId, + pub lane: LaneIdBytes, /// Result of messages which we tried to dispatch pub receive_results: Vec<(MessageNonce, ReceptionResult)>, } @@ -350,7 +350,7 @@ impl ReceivedMessages { lane: LaneId, receive_results: Vec<(MessageNonce, ReceptionResult)>, ) -> Self { - ReceivedMessages { lane, receive_results } + ReceivedMessages { lane: lane.into(), receive_results } } /// Push `result` of the `message` delivery onto `receive_results` vector. diff --git a/bridges/primitives/messages/src/storage_keys.rs b/bridges/primitives/messages/src/storage_keys.rs index ff62dab078e7..77bdb1bbe567 100644 --- a/bridges/primitives/messages/src/storage_keys.rs +++ b/bridges/primitives/messages/src/storage_keys.rs @@ -95,7 +95,7 @@ mod tests { let storage_key = message_key("BridgeMessages", &LaneId::new(1, 2), 42).0; assert_eq!( storage_key, - hex!("dd16c784ebd3390a9bc0357c7511ed018a395e6242c6813b196ca31ed0547ea70e9bdb8f50c68d12f06eabb57759ee5eb1d3dccd8b3c3a012afe265f3e3c4432129b8aee50c9dcf87f9793be208e5ea02a00000000000000").to_vec(), + hex!("dd16c784ebd3390a9bc0357c7511ed018a395e6242c6813b196ca31ed0547ea7d69c6fd1fe3b4dd5f5d67ce7c585813a48615368b1d3dccd8b3c3a012afe265f3e3c4432129b8aee50c9dcf87f9793be208e5ea02a00000000000000").to_vec(), "Unexpected storage key: {}", hex::encode(&storage_key), ); @@ -118,7 +118,7 @@ mod tests { let storage_key = outbound_lane_data_key("BridgeMessages", &LaneId::new(1, 2)).0; assert_eq!( storage_key, - hex!("dd16c784ebd3390a9bc0357c7511ed0196c246acb9b55077390e3ca723a0ca1fd3bef8b00df8ca7b01813b5e2741950db1d3dccd8b3c3a012afe265f3e3c4432129b8aee50c9dcf87f9793be208e5ea0").to_vec(), + hex!("dd16c784ebd3390a9bc0357c7511ed0196c246acb9b55077390e3ca723a0ca1f35a17fa33e857f05a3b8d3b8b7f0eeb548615368b1d3dccd8b3c3a012afe265f3e3c4432129b8aee50c9dcf87f9793be208e5ea0").to_vec(), "Unexpected storage key: {}", hex::encode(&storage_key), ); @@ -142,7 +142,7 @@ mod tests { let storage_key = inbound_lane_data_key("BridgeMessages", &LaneId::new(1, 2)).0; assert_eq!( storage_key, - hex!("dd16c784ebd3390a9bc0357c7511ed01e5f83cf83f2127eb47afdc35d6e43fabd3bef8b00df8ca7b01813b5e2741950db1d3dccd8b3c3a012afe265f3e3c4432129b8aee50c9dcf87f9793be208e5ea0").to_vec(), + hex!("dd16c784ebd3390a9bc0357c7511ed01e5f83cf83f2127eb47afdc35d6e43fab35a17fa33e857f05a3b8d3b8b7f0eeb548615368b1d3dccd8b3c3a012afe265f3e3c4432129b8aee50c9dcf87f9793be208e5ea0").to_vec(), "Unexpected storage key: {}", hex::encode(&storage_key), ); diff --git a/bridges/primitives/relayers/src/lib.rs b/bridges/primitives/relayers/src/lib.rs index 1e63c89ecd70..e23de3074943 100644 --- a/bridges/primitives/relayers/src/lib.rs +++ b/bridges/primitives/relayers/src/lib.rs @@ -174,7 +174,7 @@ mod tests { *b"test", RewardsAccountOwner::ThisChain )), - hex_literal::hex!("627261700074657374b1d3dccd8b3c3a012afe265f3e3c4432129b8aee50c9dc") + hex_literal::hex!("62726170007465737448615368b1d3dccd8b3c3a012afe265f3e3c4432129b8a") .into(), ); @@ -184,7 +184,7 @@ mod tests { *b"test", RewardsAccountOwner::ThisChain )), - hex_literal::hex!("627261700074657374a43e8951aa302c133beb5f85821a21645f07b487270ef3") + hex_literal::hex!("62726170007465737448615368a43e8951aa302c133beb5f85821a21645f07b4") .into(), ); } @@ -197,7 +197,7 @@ mod tests { *b"test", RewardsAccountOwner::ThisChain )), - hex_literal::hex!("627261700074657374b1d3dccd8b3c3a012afe265f3e3c4432129b8aee50c9dc") + hex_literal::hex!("62726170007465737448615368b1d3dccd8b3c3a012afe265f3e3c4432129b8a") .into(), ); @@ -207,7 +207,7 @@ mod tests { *b"test", RewardsAccountOwner::BridgedChain )), - hex_literal::hex!("627261700174657374b1d3dccd8b3c3a012afe265f3e3c4432129b8aee50c9dc") + hex_literal::hex!("62726170017465737448615368b1d3dccd8b3c3a012afe265f3e3c4432129b8a") .into(), ); }