Skip to content

Commit

Permalink
Propagate message receival confirmation errors (paritytech#2116)
Browse files Browse the repository at this point in the history 
* Propagate message receival confirmation errors

* spellcheck
  • Loading branch information
@serban300
serban300 authored May 9, 2023
1 parent 63caabc commit 2e05ce0
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 102 deletions.
67 changes: 23 additions & 44 deletions modules/messages/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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),
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1818,7 +1795,9 @@ mod tests {
))),
UnrewardedRelayersState { last_delivered_nonce: 1, ..Default::default() },
),
Error::<TestRuntime, ()>::TryingToConfirmMoreMessagesThanExpected,
Error::<TestRuntime, ()>::ReceivalConfirmation(
ReceivalConfirmationError::TryingToConfirmMoreMessagesThanExpected
),
);
});
}
Expand Down
106 changes: 48 additions & 58 deletions modules/messages/src/outbound_lane.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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,
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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);
Expand All @@ -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);
});
Expand All @@ -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),
);
}

Expand All @@ -323,7 +313,7 @@ mod tests {
.chain(unrewarded_relayers(3..=3).into_iter())
.collect(),
),
ReceivalConfirmationResult::FailedToConfirmFutureMessages,
Err(ReceivalConfirmationError::FailedToConfirmFutureMessages),
);
}

Expand All @@ -339,7 +329,7 @@ mod tests {
.chain(unrewarded_relayers(2..=3).into_iter())
.collect(),
),
ReceivalConfirmationResult::EmptyUnrewardedRelayerEntry,
Err(ReceivalConfirmationError::EmptyUnrewardedRelayerEntry),
);
}

Expand All @@ -354,7 +344,7 @@ mod tests {
.chain(unrewarded_relayers(2..=2).into_iter())
.collect(),
),
ReceivalConfirmationResult::NonConsecutiveUnrewardedRelayerEntries,
Err(ReceivalConfirmationError::NonConsecutiveUnrewardedRelayerEntries),
);
}

Expand Down Expand Up @@ -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)),
Expand All @@ -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)),
Expand All @@ -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))),
);
});
}
Expand Down

0 comments on commit 2e05ce0

Please sign in to comment.