From 201dfddc54167f47cde251ebf4a21ca15417f734 Mon Sep 17 00:00:00 2001
From: Serban Iorga <serban@parity.io>
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 6e27c7f2f7a..02255e15082 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::<T, I>(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::<T, I>::TryingToConfirmMoreMessagesThanExpected);
- },
- error => {
- log::trace!(
- target: LOG_TARGET,
- "Messages delivery proof contains invalid unrewarded relayers vec: {:?}",
- error,
- );
-
- fail!(Error::<T, I>::InvalidUnrewardedRelayers);
- },
- };
+ let confirmed_messages = lane
+ .confirm_delivery(
+ relayers_state.total_messages,
+ last_delivered_nonce,
+ &lane_data.relayers,
+ )
+ .map_err(Error::<T, I>::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<Chain: SourceHeaderChain, DispatchPayload: D
#[cfg(test)]
mod tests {
use super::*;
- use crate::mock::{
- inbound_unrewarded_relayers_state, message, message_payload, run_test, unrewarded_relayer,
- AccountId, DbWeight, RuntimeEvent as TestEvent, RuntimeOrigin,
- TestDeliveryConfirmationPayments, TestDeliveryPayments, TestMessagesDeliveryProof,
- TestMessagesProof, TestRelayer, TestRuntime, TestWeightInfo, MAX_OUTBOUND_PAYLOAD_SIZE,
- PAYLOAD_REJECTED_BY_TARGET_CHAIN, REGULAR_PAYLOAD, TEST_LANE_ID, TEST_LANE_ID_2,
- TEST_LANE_ID_3, TEST_RELAYER_A, TEST_RELAYER_B,
+ use crate::{
+ mock::{
+ inbound_unrewarded_relayers_state, message, message_payload, run_test,
+ unrewarded_relayer, AccountId, DbWeight, RuntimeEvent as TestEvent, RuntimeOrigin,
+ TestDeliveryConfirmationPayments, TestDeliveryPayments, TestMessagesDeliveryProof,
+ TestMessagesProof, TestRelayer, TestRuntime, TestWeightInfo, MAX_OUTBOUND_PAYLOAD_SIZE,
+ PAYLOAD_REJECTED_BY_TARGET_CHAIN, REGULAR_PAYLOAD, TEST_LANE_ID, TEST_LANE_ID_2,
+ TEST_LANE_ID_3, TEST_RELAYER_A, TEST_RELAYER_B,
+ },
+ outbound_lane::ReceivalConfirmationError,
};
use bp_messages::{BridgeMessagesCall, UnrewardedRelayer, UnrewardedRelayersState};
use bp_test_utils::generate_owned_bridge_module_tests;
@@ -1818,7 +1795,9 @@ mod tests {
))),
UnrewardedRelayersState { last_delivered_nonce: 1, ..Default::default() },
),
- Error::<TestRuntime, ()>::TryingToConfirmMoreMessagesThanExpected,
+ Error::<TestRuntime, ()>::ReceivalConfirmation(
+ ReceivalConfirmationError::TryingToConfirmMoreMessagesThanExpected
+ ),
);
});
}
diff --git a/bridges/modules/messages/src/outbound_lane.rs b/bridges/modules/messages/src/outbound_lane.rs
index 3d0d4de966a..bc4fdfab919 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<T, I> = BoundedVec<u8, <T as Config<I>>::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<S: OutboundLaneStorage> OutboundLane<S> {
max_allowed_messages: MessageNonce,
latest_delivered_nonce: MessageNonce,
relayers: &VecDeque<UnrewardedRelayer<RelayerId>>,
- ) -> ReceivalConfirmationResult {
+ ) -> Result<Option<DeliveredMessages>, 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<S: OutboundLaneStorage> OutboundLane<S> {
fn ensure_unrewarded_relayers_are_correct<RelayerId>(
latest_received_nonce: MessageNonce,
relayers: &VecDeque<UnrewardedRelayer<RelayerId>>,
-) -> Result<(), ReceivalConfirmationResult> {
- let mut last_entry_end: Option<MessageNonce> = 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<UnrewardedRelayer<TestRelayer>>,
- ) -> ReceivalConfirmationResult {
+ ) -> Result<Option<DeliveredMessages>, ReceivalConfirmationError> {
run_test(|| {
let mut lane = outbound_lane::<TestRuntime, _>(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))),
);
});
}
--
GitLab