Skip to content

Propagate message receival confirmation errors #2116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 9, 2023
Merged

Propagate message receival confirmation errors #2116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab196d0
Propagate message receival confirmation errors
serban300 May 5, 2023
ef8dab0
spellcheck
serban300 May 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 23 additions & 44 deletions modules/messages/src/lib.rs
View file
Open in desktop
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 @@ -1814,7 +1791,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
Open in desktop
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