From 62939a632f6f634b4692091ffd2942e376a568df Mon Sep 17 00:00:00 2001 From: Serban Iorga Date: Tue, 9 May 2023 11:43:15 +0300 Subject: [PATCH] Propagate message receival confirmation errors (#2116) * Propagate message receival confirmation errors * spellcheck --- bridges/modules/messages/src/lib.rs | 67 ++++------- bridges/modules/messages/src/outbound_lane.rs | 106 ++++++++---------- 2 files changed, 71 insertions(+), 102 deletions(-) diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs index 6e27c7f2f7ae..02255e15082f 100644 --- a/bridges/modules/messages/src/lib.rs +++ b/bridges/modules/messages/src/lib.rs @@ -48,7 +48,7 @@ pub use weights_ext::{ use crate::{ inbound_lane::{InboundLane, InboundLaneStorage}, - outbound_lane::{OutboundLane, OutboundLaneStorage, ReceivalConfirmationResult}, + outbound_lane::{OutboundLane, OutboundLaneStorage, ReceivalConfirmationError}, }; use bp_messages::{ @@ -458,36 +458,13 @@ pub mod pallet { // mark messages as delivered let mut lane = outbound_lane::(lane_id); let last_delivered_nonce = lane_data.last_delivered_nonce(); - let confirmed_messages = match lane.confirm_delivery( - relayers_state.total_messages, - last_delivered_nonce, - &lane_data.relayers, - ) { - ReceivalConfirmationResult::ConfirmedMessages(confirmed_messages) => - Some(confirmed_messages), - ReceivalConfirmationResult::NoNewConfirmations => None, - ReceivalConfirmationResult::TryingToConfirmMoreMessagesThanExpected( - to_confirm_messages_count, - ) => { - log::trace!( - target: LOG_TARGET, - "Messages delivery proof contains too many messages to confirm: {} vs declared {}", - to_confirm_messages_count, - relayers_state.total_messages, - ); - - fail!(Error::::TryingToConfirmMoreMessagesThanExpected); - }, - error => { - log::trace!( - target: LOG_TARGET, - "Messages delivery proof contains invalid unrewarded relayers vec: {:?}", - error, - ); - - fail!(Error::::InvalidUnrewardedRelayers); - }, - }; + let confirmed_messages = lane + .confirm_delivery( + relayers_state.total_messages, + last_delivered_nonce, + &lane_data.relayers, + ) + .map_err(Error::::ReceivalConfirmation)?; if let Some(confirmed_messages) = confirmed_messages { // emit 'delivered' event @@ -568,8 +545,6 @@ pub mod pallet { InvalidMessagesProof, /// Invalid messages delivery proof has been submitted. InvalidMessagesDeliveryProof, - /// The bridged chain has invalid `UnrewardedRelayers` in its storage (fatal for the lane). - InvalidUnrewardedRelayers, /// The relayer has declared invalid unrewarded relayers state in the /// `receive_messages_delivery_proof` call. InvalidUnrewardedRelayersState, @@ -578,9 +553,8 @@ pub mod pallet { InsufficientDispatchWeight, /// The message someone is trying to work with (i.e. increase fee) is not yet sent. MessageIsNotYetSent, - /// The number of actually confirmed messages is going to be larger than the number of - /// messages in the proof. This may mean that this or bridged chain storage is corrupted. - TryingToConfirmMoreMessagesThanExpected, + /// Error confirming messages receival. + ReceivalConfirmation(ReceivalConfirmationError), /// Error generated by the `OwnedBridgeModule` trait. BridgeModule(bp_runtime::OwnedBridgeModuleError), } @@ -941,13 +915,16 @@ fn verify_and_decode_messages_proof::TryingToConfirmMoreMessagesThanExpected, + Error::::ReceivalConfirmation( + ReceivalConfirmationError::TryingToConfirmMoreMessagesThanExpected + ), ); }); } diff --git a/bridges/modules/messages/src/outbound_lane.rs b/bridges/modules/messages/src/outbound_lane.rs index 3d0d4de966a5..bc4fdfab9195 100644 --- a/bridges/modules/messages/src/outbound_lane.rs +++ b/bridges/modules/messages/src/outbound_lane.rs @@ -16,16 +16,18 @@ //! Everything about outgoing messages sending. -use crate::Config; +use crate::{Config, LOG_TARGET}; use bp_messages::{ DeliveredMessages, LaneId, MessageNonce, MessagePayload, OutboundLaneData, UnrewardedRelayer, }; +use codec::{Decode, Encode}; use frame_support::{ weights::{RuntimeDbWeight, Weight}, - BoundedVec, RuntimeDebug, + BoundedVec, PalletError, RuntimeDebug, }; use num_traits::Zero; +use scale_info::TypeInfo; use sp_std::collections::vec_deque::VecDeque; /// Outbound lane storage. @@ -49,13 +51,8 @@ pub trait OutboundLaneStorage { pub type StoredMessagePayload = BoundedVec>::MaximalOutboundPayloadSize>; /// Result of messages receival confirmation. -#[derive(RuntimeDebug, PartialEq, Eq)] -pub enum ReceivalConfirmationResult { - /// New messages have been confirmed by the confirmation transaction. - ConfirmedMessages(DeliveredMessages), - /// Confirmation transaction brings no new confirmation. This may be a result of relayer - /// error or several relayers running. - NoNewConfirmations, +#[derive(Encode, Decode, RuntimeDebug, PartialEq, Eq, PalletError, TypeInfo)] +pub enum ReceivalConfirmationError { /// Bridged chain is trying to confirm more messages than we have generated. May be a result /// of invalid bridged chain storage. FailedToConfirmFutureMessages, @@ -66,7 +63,7 @@ pub enum ReceivalConfirmationResult { /// bridged chain storage. NonConsecutiveUnrewardedRelayerEntries, /// The chain has more messages that need to be confirmed than there is in the proof. - TryingToConfirmMoreMessagesThanExpected(MessageNonce), + TryingToConfirmMoreMessagesThanExpected, } /// Outbound messages lane. @@ -105,37 +102,39 @@ impl OutboundLane { max_allowed_messages: MessageNonce, latest_delivered_nonce: MessageNonce, relayers: &VecDeque>, - ) -> ReceivalConfirmationResult { + ) -> Result, ReceivalConfirmationError> { let mut data = self.storage.data(); - if latest_delivered_nonce <= data.latest_received_nonce { - return ReceivalConfirmationResult::NoNewConfirmations + let confirmed_messages = DeliveredMessages { + begin: data.latest_received_nonce.saturating_add(1), + end: latest_delivered_nonce, + }; + if confirmed_messages.total_messages() == 0 { + return Ok(None) } - if latest_delivered_nonce > data.latest_generated_nonce { - return ReceivalConfirmationResult::FailedToConfirmFutureMessages + if confirmed_messages.end > data.latest_generated_nonce { + return Err(ReceivalConfirmationError::FailedToConfirmFutureMessages) } - if latest_delivered_nonce - data.latest_received_nonce > max_allowed_messages { + if confirmed_messages.total_messages() > max_allowed_messages { // that the relayer has declared correct number of messages that the proof contains (it // is checked outside of the function). But it may happen (but only if this/bridged // chain storage is corrupted, though) that the actual number of confirmed messages if // larger than declared. This would mean that 'reward loop' will take more time than the // weight formula accounts, so we can't allow that. - return ReceivalConfirmationResult::TryingToConfirmMoreMessagesThanExpected( - latest_delivered_nonce - data.latest_received_nonce, - ) + log::trace!( + target: LOG_TARGET, + "Messages delivery proof contains too many messages to confirm: {} vs declared {}", + confirmed_messages.total_messages(), + max_allowed_messages, + ); + return Err(ReceivalConfirmationError::TryingToConfirmMoreMessagesThanExpected) } - if let Err(e) = ensure_unrewarded_relayers_are_correct(latest_delivered_nonce, relayers) { - return e - } + ensure_unrewarded_relayers_are_correct(confirmed_messages.end, relayers)?; - let prev_latest_received_nonce = data.latest_received_nonce; - data.latest_received_nonce = latest_delivered_nonce; + data.latest_received_nonce = confirmed_messages.end; self.storage.set_data(data); - ReceivalConfirmationResult::ConfirmedMessages(DeliveredMessages { - begin: prev_latest_received_nonce + 1, - end: latest_delivered_nonce, - }) + Ok(Some(confirmed_messages)) } /// Prune at most `max_messages_to_prune` already received messages. @@ -176,27 +175,24 @@ impl OutboundLane { fn ensure_unrewarded_relayers_are_correct( latest_received_nonce: MessageNonce, relayers: &VecDeque>, -) -> Result<(), ReceivalConfirmationResult> { - let mut last_entry_end: Option = None; +) -> Result<(), ReceivalConfirmationError> { + let mut expected_entry_begin = relayers.front().map(|entry| entry.messages.begin); for entry in relayers { // unrewarded relayer entry must have at least 1 unconfirmed message // (guaranteed by the `InboundLane::receive_message()`) if entry.messages.end < entry.messages.begin { - return Err(ReceivalConfirmationResult::EmptyUnrewardedRelayerEntry) + return Err(ReceivalConfirmationError::EmptyUnrewardedRelayerEntry) } // every entry must confirm range of messages that follows previous entry range // (guaranteed by the `InboundLane::receive_message()`) - if let Some(last_entry_end) = last_entry_end { - let expected_entry_begin = last_entry_end.checked_add(1); - if expected_entry_begin != Some(entry.messages.begin) { - return Err(ReceivalConfirmationResult::NonConsecutiveUnrewardedRelayerEntries) - } + if expected_entry_begin != Some(entry.messages.begin) { + return Err(ReceivalConfirmationError::NonConsecutiveUnrewardedRelayerEntries) } - last_entry_end = Some(entry.messages.end); + expected_entry_begin = entry.messages.end.checked_add(1); // entry can't confirm messages larger than `inbound_lane_data.latest_received_nonce()` // (guaranteed by the `InboundLane::receive_message()`) if entry.messages.end > latest_received_nonce { - return Err(ReceivalConfirmationResult::FailedToConfirmFutureMessages) + return Err(ReceivalConfirmationError::FailedToConfirmFutureMessages) } } @@ -231,7 +227,7 @@ mod tests { fn assert_3_messages_confirmation_fails( latest_received_nonce: MessageNonce, relayers: &VecDeque>, - ) -> ReceivalConfirmationResult { + ) -> Result, ReceivalConfirmationError> { run_test(|| { let mut lane = outbound_lane::(TEST_LANE_ID); lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); @@ -268,7 +264,7 @@ mod tests { assert_eq!(lane.storage.data().latest_received_nonce, 0); assert_eq!( lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)), - ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(1..=3)), + Ok(Some(delivered_messages(1..=3))), ); assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 3); @@ -286,19 +282,13 @@ mod tests { assert_eq!(lane.storage.data().latest_received_nonce, 0); assert_eq!( lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)), - ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(1..=3)), - ); - assert_eq!( - lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)), - ReceivalConfirmationResult::NoNewConfirmations, + Ok(Some(delivered_messages(1..=3))), ); + assert_eq!(lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)), Ok(None),); assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 3); - assert_eq!( - lane.confirm_delivery(1, 2, &unrewarded_relayers(1..=1)), - ReceivalConfirmationResult::NoNewConfirmations, - ); + assert_eq!(lane.confirm_delivery(1, 2, &unrewarded_relayers(1..=1)), Ok(None),); assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 3); }); @@ -308,7 +298,7 @@ mod tests { fn confirm_delivery_rejects_nonce_larger_than_last_generated() { assert_eq!( assert_3_messages_confirmation_fails(10, &unrewarded_relayers(1..=10),), - ReceivalConfirmationResult::FailedToConfirmFutureMessages, + Err(ReceivalConfirmationError::FailedToConfirmFutureMessages), ); } @@ -323,7 +313,7 @@ mod tests { .chain(unrewarded_relayers(3..=3).into_iter()) .collect(), ), - ReceivalConfirmationResult::FailedToConfirmFutureMessages, + Err(ReceivalConfirmationError::FailedToConfirmFutureMessages), ); } @@ -339,7 +329,7 @@ mod tests { .chain(unrewarded_relayers(2..=3).into_iter()) .collect(), ), - ReceivalConfirmationResult::EmptyUnrewardedRelayerEntry, + Err(ReceivalConfirmationError::EmptyUnrewardedRelayerEntry), ); } @@ -354,7 +344,7 @@ mod tests { .chain(unrewarded_relayers(2..=2).into_iter()) .collect(), ), - ReceivalConfirmationResult::NonConsecutiveUnrewardedRelayerEntries, + Err(ReceivalConfirmationError::NonConsecutiveUnrewardedRelayerEntries), ); } @@ -383,7 +373,7 @@ mod tests { // after confirmation, some messages are received assert_eq!( lane.confirm_delivery(2, 2, &unrewarded_relayers(1..=2)), - ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(1..=2)), + Ok(Some(delivered_messages(1..=2))), ); assert_eq!( lane.prune_messages(RocksDbWeight::get(), RocksDbWeight::get().writes(101)), @@ -396,7 +386,7 @@ mod tests { // after last message is confirmed, everything is pruned assert_eq!( lane.confirm_delivery(1, 3, &unrewarded_relayers(3..=3)), - ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(3..=3)), + Ok(Some(delivered_messages(3..=3))), ); assert_eq!( lane.prune_messages(RocksDbWeight::get(), RocksDbWeight::get().writes(101)), @@ -418,15 +408,15 @@ mod tests { lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); assert_eq!( lane.confirm_delivery(0, 3, &unrewarded_relayers(1..=3)), - ReceivalConfirmationResult::TryingToConfirmMoreMessagesThanExpected(3), + Err(ReceivalConfirmationError::TryingToConfirmMoreMessagesThanExpected), ); assert_eq!( lane.confirm_delivery(2, 3, &unrewarded_relayers(1..=3)), - ReceivalConfirmationResult::TryingToConfirmMoreMessagesThanExpected(3), + Err(ReceivalConfirmationError::TryingToConfirmMoreMessagesThanExpected), ); assert_eq!( lane.confirm_delivery(3, 3, &unrewarded_relayers(1..=3)), - ReceivalConfirmationResult::ConfirmedMessages(delivered_messages(1..=3)), + Ok(Some(delivered_messages(1..=3))), ); }); }