Skip to content

Commit

Permalink
Some error improvements (#1956)
Browse files Browse the repository at this point in the history 
* Use `HeaderChainError` in parachains module

* Use MessageProofError instead of 'static str in some places

* Avoid implementing Into<'static str> for some errors

We avoid deriving `Debug` for the structs that we use in the runtime and
we derive `RuntimeDebug` instead in order to avoid bloating th eruntime
with static strs. But implementing `Into<'static str>` does the same. So
in some places it makes sense to replace `Into<'static str>` with `Debug`.

* Move the messages error definition

Move the messages error definition outside of `mod target`
  • Loading branch information
@serban300
serban300 committed Mar 9, 2023
1 parent 434c5e0 commit ef1b1bc
Show file tree
Hide file tree
Showing 7 changed files with 145 additions and 132 deletions.
170 changes: 75 additions & 95 deletions bin/runtime-common/src/messages.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,29 @@ pub type OriginOf<C> = <C as ThisChainWithMessages>::RuntimeOrigin;
/// Type of call that is used on this chain.
pub type CallOf<C> = <C as ThisChainWithMessages>::RuntimeCall;

/// Error that happens during message verification.
#[derive(Debug, PartialEq, Eq)]
pub enum Error {
/// The message proof is empty.
EmptyMessageProof,
/// Error returned by the bridged header chain.
HeaderChain(HeaderChainError),
/// Error returned while reading/decoding inbound lane data from the storage proof.
InboundLaneStorage(StorageProofError),
/// The declared message weight is incorrect.
InvalidMessageWeight,
/// Declared messages count doesn't match actual value.
MessagesCountMismatch,
/// Error returned while reading/decoding message data from the storage proof.
MessageStorage(StorageProofError),
/// The message is too large.
MessageTooLarge,
/// Error returned while reading/decoding outbound lane data from the storage proof.
OutboundLaneStorage(StorageProofError),
/// Storage proof related error.
StorageProof(StorageProofError),
}

/// Sub-module that is declaring types required for processing This -> Bridged chain messages.
pub mod source {
use super::*;
Expand Down Expand Up @@ -169,8 +192,6 @@ pub mod source {
/// The error message returned from `LaneMessageVerifier` when too many pending messages at the
/// lane.
pub const TOO_MANY_PENDING_MESSAGES: &str = "Too many pending messages at the lane.";
/// The error message returned from `LaneMessageVerifier` when call origin is mismatch.
pub const BAD_ORIGIN: &str = "Unable to match the source origin to expected target origin.";

impl<B> LaneMessageVerifier<OriginOf<ThisChain<B>>, FromThisChainMessagePayload>
for FromThisChainMessageVerifier<B>
Expand Down Expand Up @@ -220,7 +241,7 @@ pub mod source {
impl<B: MessageBridge> TargetHeaderChain<FromThisChainMessagePayload, AccountIdOf<ThisChain<B>>>
for TargetHeaderChainAdapter<B>
{
type Error = &'static str;
type Error = Error;
type MessagesDeliveryProof = FromBridgedChainMessagesDeliveryProof<HashOf<BridgedChain<B>>>;

fn verify_message(payload: &FromThisChainMessagePayload) -> Result<(), Self::Error> {
Expand All @@ -241,9 +262,9 @@ pub mod source {
/// check) that would reject message (see `FromThisChainMessageVerifier`).
pub fn verify_chain_message<B: MessageBridge>(
payload: &FromThisChainMessagePayload,
) -> Result<(), &'static str> {
) -> Result<(), Error> {
if !BridgedChain::<B>::verify_dispatch_weight(payload) {
return Err("Incorrect message weight declared")
return Err(Error::InvalidMessageWeight)
}

// The maximal size of extrinsic at Substrate-based chain depends on the
Expand All @@ -257,7 +278,7 @@ pub mod source {
// transaction also contains signatures and signed extensions. Because of this, we reserve
// 1/3 of the the maximal extrinsic weight for this data.
if payload.len() > maximal_message_size::<B>() as usize {
return Err("The message is too large to be sent over the lane")
return Err(Error::MessageTooLarge)
}

Ok(())
Expand All @@ -269,7 +290,7 @@ pub mod source {
/// parachains, please use the `verify_messages_delivery_proof_from_parachain`.
pub fn verify_messages_delivery_proof<B: MessageBridge>(
proof: FromBridgedChainMessagesDeliveryProof<HashOf<BridgedChain<B>>>,
) -> Result<ParsedMessagesDeliveryProofFromBridgedChain<B>, &'static str> {
) -> Result<ParsedMessagesDeliveryProofFromBridgedChain<B>, Error> {
let FromBridgedChainMessagesDeliveryProof { bridged_header_hash, storage_proof, lane } =
proof;
B::BridgedHeaderChain::parse_finalized_storage_proof(
Expand All @@ -283,22 +304,17 @@ pub mod source {
B::BRIDGED_MESSAGES_PALLET_NAME,
&lane,
);
let raw_inbound_lane_data = storage
.read_value(storage_inbound_lane_data_key.0.as_ref())
.map_err(|_| "Failed to read inbound lane state from storage proof")?
.ok_or("Inbound lane state is missing from the messages proof")?;
let inbound_lane_data = InboundLaneData::decode(&mut &raw_inbound_lane_data[..])
.map_err(|_| "Failed to decode inbound lane state from the proof")?;
let inbound_lane_data = storage
.read_and_decode_mandatory_value(storage_inbound_lane_data_key.0.as_ref())
.map_err(Error::InboundLaneStorage)?;

// check that the storage proof doesn't have any untouched trie nodes
storage
.ensure_no_unused_nodes()
.map_err(|_| "Messages delivery proof has unused trie nodes")?;
storage.ensure_no_unused_nodes().map_err(Error::StorageProof)?;

Ok((lane, inbound_lane_data))
},
)
.map_err(<&'static str>::from)?
.map_err(Error::HeaderChain)?
}

/// XCM bridge.
Expand Down Expand Up @@ -580,14 +596,14 @@ pub mod target {
pub struct SourceHeaderChainAdapter<B>(PhantomData<B>);

impl<B: MessageBridge> SourceHeaderChain for SourceHeaderChainAdapter<B> {
type Error = &'static str;
type Error = Error;
type MessagesProof = FromBridgedChainMessagesProof<HashOf<BridgedChain<B>>>;

fn verify_messages_proof(
proof: Self::MessagesProof,
messages_count: u32,
) -> Result<ProvedMessages<Message>, Self::Error> {
verify_messages_proof::<B>(proof, messages_count).map_err(Into::into)
verify_messages_proof::<B>(proof, messages_count)
}
}

Expand All @@ -602,7 +618,7 @@ pub mod target {
pub fn verify_messages_proof<B: MessageBridge>(
proof: FromBridgedChainMessagesProof<HashOf<BridgedChain<B>>>,
messages_count: u32,
) -> Result<ProvedMessages<Message>, MessageProofError> {
) -> Result<ProvedMessages<Message>, Error> {
let FromBridgedChainMessagesProof {
bridged_header_hash,
storage_proof,
Expand All @@ -625,7 +641,7 @@ pub mod target {
// (this bounds maximal capacity of messages vec below)
let messages_in_the_proof = nonces_difference.saturating_add(1);
if messages_in_the_proof != MessageNonce::from(messages_count) {
return Err(MessageProofError::MessagesCountMismatch)
return Err(Error::MessagesCountMismatch)
}

messages_in_the_proof
Expand All @@ -640,37 +656,26 @@ pub mod target {
let mut messages = Vec::with_capacity(messages_in_the_proof as _);
for nonce in nonces_start..=nonces_end {
let message_key = MessageKey { lane_id: lane, nonce };
let raw_message_data = parser
.read_raw_message(&message_key)
.ok_or(MessageProofError::MissingRequiredMessage)?;
let payload = MessagePayload::decode(&mut &raw_message_data[..])
.map_err(|_| MessageProofError::FailedToDecodeMessage)?;
messages.push(Message { key: message_key, payload });
let message_payload = parser.read_and_decode_message_payload(&message_key)?;
messages.push(Message { key: message_key, payload: message_payload });
}

// Now let's check if proof contains outbound lane state proof. It is optional, so
// we simply ignore `read_value` errors and missing value.
let mut proved_lane_messages = ProvedLaneMessages { lane_state: None, messages };
let raw_outbound_lane_data = parser.read_raw_outbound_lane_data(&lane);
if let Some(raw_outbound_lane_data) = raw_outbound_lane_data {
proved_lane_messages.lane_state = Some(
OutboundLaneData::decode(&mut &raw_outbound_lane_data[..])
.map_err(|_| MessageProofError::FailedToDecodeOutboundLaneState)?,
);
}
let proved_lane_messages = ProvedLaneMessages {
lane_state: parser.read_and_decode_outbound_lane_data(&lane)?,
messages,
};

// Now we may actually check if the proof is empty or not.
if proved_lane_messages.lane_state.is_none() &&
proved_lane_messages.messages.is_empty()
{
return Err(MessageProofError::Empty)
return Err(Error::EmptyMessageProof)
}

// check that the storage proof doesn't have any untouched trie nodes
parser
.storage
.ensure_no_unused_nodes()
.map_err(MessageProofError::StorageProof)?;
parser.storage.ensure_no_unused_nodes().map_err(Error::StorageProof)?;

// We only support single lane messages in this generated_schema
let mut proved_messages = ProvedMessages::new();
Expand All @@ -679,43 +684,7 @@ pub mod target {
Ok(proved_messages)
},
)
.map_err(MessageProofError::HeaderChain)?
}

/// Error that happens during message proof verification.
#[derive(Debug, PartialEq, Eq)]
pub enum MessageProofError {
/// Error returned by the bridged header chain.
HeaderChain(HeaderChainError),
/// The message proof is empty.
Empty,
/// Declared messages count doesn't match actual value.
MessagesCountMismatch,
/// Message is missing from the proof.
MissingRequiredMessage,
/// Failed to decode message from the proof.
FailedToDecodeMessage,
/// Failed to decode outbound lane data from the proof.
FailedToDecodeOutboundLaneState,
/// Storage proof related error.
StorageProof(StorageProofError),
}

impl From<MessageProofError> for &'static str {
fn from(err: MessageProofError) -> &'static str {
match err {
MessageProofError::HeaderChain(err) => err.into(),
MessageProofError::Empty => "Messages proof is empty",
MessageProofError::MessagesCountMismatch =>
"Declared messages count doesn't match actual value",
MessageProofError::MissingRequiredMessage => "Message is missing from the proof",
MessageProofError::FailedToDecodeMessage =>
"Failed to decode message from the proof",
MessageProofError::FailedToDecodeOutboundLaneState =>
"Failed to decode outbound lane data from the proof",
MessageProofError::StorageProof(_) => "Invalid storage proof",
}
}
.map_err(Error::HeaderChain)?
}

struct StorageProofCheckerAdapter<H: Hasher, B> {
Expand All @@ -724,21 +693,32 @@ pub mod target {
}

impl<H: Hasher, B: MessageBridge> StorageProofCheckerAdapter<H, B> {
fn read_raw_outbound_lane_data(&mut self, lane_id: &LaneId) -> Option<Vec<u8>> {
fn read_and_decode_outbound_lane_data(
&mut self,
lane_id: &LaneId,
) -> Result<Option<OutboundLaneData>, Error> {
let storage_outbound_lane_data_key = bp_messages::storage_keys::outbound_lane_data_key(
B::BRIDGED_MESSAGES_PALLET_NAME,
lane_id,
);
self.storage.read_value(storage_outbound_lane_data_key.0.as_ref()).ok()?

self.storage
.read_and_decode_opt_value(storage_outbound_lane_data_key.0.as_ref())
.map_err(Error::OutboundLaneStorage)
}

fn read_raw_message(&mut self, message_key: &MessageKey) -> Option<Vec<u8>> {
fn read_and_decode_message_payload(
&mut self,
message_key: &MessageKey,
) -> Result<MessagePayload, Error> {
let storage_message_key = bp_messages::storage_keys::message_key(
B::BRIDGED_MESSAGES_PALLET_NAME,
&message_key.lane_id,
message_key.nonce,
);
self.storage.read_value(storage_message_key.0.as_ref()).ok()?
self.storage
.read_and_decode_mandatory_value(storage_message_key.0.as_ref())
.map_err(Error::MessageStorage)
}
}
}
Expand Down Expand Up @@ -896,7 +876,7 @@ mod tests {
using_messages_proof(10, None, encode_all_messages, encode_lane_data, |proof| {
target::verify_messages_proof::<OnThisChainBridge>(proof, 5)
}),
Err(target::MessageProofError::MessagesCountMismatch),
Err(Error::MessagesCountMismatch),
);
}

Expand All @@ -906,7 +886,7 @@ mod tests {
using_messages_proof(10, None, encode_all_messages, encode_lane_data, |proof| {
target::verify_messages_proof::<OnThisChainBridge>(proof, 15)
}),
Err(target::MessageProofError::MessagesCountMismatch),
Err(Error::MessagesCountMismatch),
);
}

Expand All @@ -919,7 +899,7 @@ mod tests {
pallet_bridge_grandpa::ImportedHeaders::<TestRuntime>::remove(bridged_header_hash);
target::verify_messages_proof::<OnThisChainBridge>(proof, 10)
}),
Err(target::MessageProofError::HeaderChain(HeaderChainError::UnknownHeader)),
Err(Error::HeaderChain(HeaderChainError::UnknownHeader)),
);
}

Expand All @@ -942,7 +922,7 @@ mod tests {
);
target::verify_messages_proof::<OnThisChainBridge>(proof, 10)
}),
Err(target::MessageProofError::HeaderChain(HeaderChainError::StorageProof(
Err(Error::HeaderChain(HeaderChainError::StorageProof(
StorageProofError::StorageRootMismatch
))),
);
Expand All @@ -957,7 +937,7 @@ mod tests {
proof.storage_proof.push(node);
target::verify_messages_proof::<OnThisChainBridge>(proof, 10)
},),
Err(target::MessageProofError::HeaderChain(HeaderChainError::StorageProof(
Err(Error::HeaderChain(HeaderChainError::StorageProof(
StorageProofError::DuplicateNodesInProof
))),
);
Expand All @@ -970,27 +950,27 @@ mod tests {
proof.storage_proof.push(vec![42]);
target::verify_messages_proof::<OnThisChainBridge>(proof, 10)
},),
Err(target::MessageProofError::StorageProof(StorageProofError::UnusedNodesInTheProof)),
Err(Error::StorageProof(StorageProofError::UnusedNodesInTheProof)),
);
}

#[test]
fn message_proof_is_rejected_if_required_message_is_missing() {
assert_eq!(
matches!(
using_messages_proof(
10,
None,
|n, m| if n != 5 { Some(m.encode()) } else { None },
encode_lane_data,
|proof| target::verify_messages_proof::<OnThisChainBridge>(proof, 10)
),
Err(target::MessageProofError::MissingRequiredMessage),
Err(Error::MessageStorage(StorageProofError::StorageValueEmpty)),
);
}

#[test]
fn message_proof_is_rejected_if_message_decode_fails() {
assert_eq!(
matches!(
using_messages_proof(
10,
None,
Expand All @@ -1004,13 +984,13 @@ mod tests {
encode_lane_data,
|proof| target::verify_messages_proof::<OnThisChainBridge>(proof, 10),
),
Err(target::MessageProofError::FailedToDecodeMessage),
Err(Error::MessageStorage(StorageProofError::StorageValueDecodeFailed(_))),
);
}

#[test]
fn message_proof_is_rejected_if_outbound_lane_state_decode_fails() {
assert_eq!(
matches!(
using_messages_proof(
10,
Some(OutboundLaneData {
Expand All @@ -1026,7 +1006,7 @@ mod tests {
},
|proof| target::verify_messages_proof::<OnThisChainBridge>(proof, 10),
),
Err(target::MessageProofError::FailedToDecodeOutboundLaneState),
Err(Error::OutboundLaneStorage(StorageProofError::StorageValueDecodeFailed(_))),
);
}

Expand All @@ -1036,7 +1016,7 @@ mod tests {
using_messages_proof(0, None, encode_all_messages, encode_lane_data, |proof| {
target::verify_messages_proof::<OnThisChainBridge>(proof, 0)
},),
Err(target::MessageProofError::Empty),
Err(Error::EmptyMessageProof),
);
}

Expand Down Expand Up @@ -1110,7 +1090,7 @@ mod tests {
proof.nonces_end = u64::MAX;
target::verify_messages_proof::<OnThisChainBridge>(proof, u32::MAX)
},),
Err(target::MessageProofError::MessagesCountMismatch),
Err(Error::MessagesCountMismatch),
);
}
}
Loading

0 comments on commit ef1b1bc

Please sign in to comment.