From 17c048dc4e22f960e19d94d28e6b16ab5b8d6a3e Mon Sep 17 00:00:00 2001 From: Serban Iorga Date: Tue, 9 May 2023 18:16:06 +0300 Subject: [PATCH] Cosmetics (#2124) * Cosmetics * Address PR comment --- bridges/modules/messages/src/lib.rs | 31 +++------ bridges/primitives/messages/src/lib.rs | 69 +++++++++---------- bridges/primitives/runtime/src/lib.rs | 14 +++- .../src/messages_target.rs | 23 ++----- 4 files changed, 61 insertions(+), 76 deletions(-) diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs index 307de89d54eca..f04e86f3a8e32 100644 --- a/bridges/modules/messages/src/lib.rs +++ b/bridges/modules/messages/src/lib.rs @@ -59,9 +59,9 @@ use bp_messages::{ DeliveryPayments, DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages, SourceHeaderChain, }, - total_unrewarded_messages, DeliveredMessages, InboundLaneData, InboundMessageDetails, LaneId, - MessageKey, MessageNonce, MessagePayload, MessagesOperatingMode, OutboundLaneData, - OutboundMessageDetails, UnrewardedRelayersState, VerificationError, + DeliveredMessages, InboundLaneData, InboundMessageDetails, LaneId, MessageKey, MessageNonce, + MessagePayload, MessagesOperatingMode, OutboundLaneData, OutboundMessageDetails, + UnrewardedRelayersState, VerificationError, }; use bp_runtime::{BasicOperatingMode, ChainId, OwnedBridgeModule, PreComputedSize, Size}; use codec::{Decode, Encode, MaxEncodedLen}; @@ -437,21 +437,8 @@ pub mod pallet { Error::::InvalidMessagesDeliveryProof })?; - - // verify that the relayer has declared correct `lane_data::relayers` state - // (we only care about total number of entries and messages, because this affects call - // weight) - ensure!( - total_unrewarded_messages(&lane_data.relayers).unwrap_or(MessageNonce::MAX) == - relayers_state.total_messages && - lane_data.relayers.len() as MessageNonce == - relayers_state.unrewarded_relayer_entries, - Error::::InvalidUnrewardedRelayersState - ); - // the `last_delivered_nonce` field may also be used by the signed extension. Even - // though providing wrong value isn't critical, let's also check it here. ensure!( - lane_data.last_delivered_nonce() == relayers_state.last_delivered_nonce, + relayers_state.is_valid(&lane_data), Error::::InvalidUnrewardedRelayersState ); @@ -975,9 +962,9 @@ mod tests { ))), UnrewardedRelayersState { unrewarded_relayer_entries: 1, + messages_in_oldest_entry: 1, total_messages: 1, last_delivered_nonce: 1, - ..Default::default() }, )); @@ -1341,9 +1328,9 @@ mod tests { single_message_delivery_proof, UnrewardedRelayersState { unrewarded_relayer_entries: 1, + messages_in_oldest_entry: 1, total_messages: 1, last_delivered_nonce: 1, - ..Default::default() }, ); assert_ok!(result); @@ -1381,9 +1368,9 @@ mod tests { two_messages_delivery_proof, UnrewardedRelayersState { unrewarded_relayer_entries: 2, + messages_in_oldest_entry: 1, total_messages: 2, last_delivered_nonce: 2, - ..Default::default() }, ); assert_ok!(result); @@ -1746,9 +1733,9 @@ mod tests { TestMessagesDeliveryProof(messages_1_and_2_proof), UnrewardedRelayersState { unrewarded_relayer_entries: 1, + messages_in_oldest_entry: 2, total_messages: 2, last_delivered_nonce: 2, - ..Default::default() }, )); // second tx with message 3 @@ -1757,9 +1744,9 @@ mod tests { TestMessagesDeliveryProof(messages_3_proof), UnrewardedRelayersState { unrewarded_relayer_entries: 1, + messages_in_oldest_entry: 1, total_messages: 1, last_delivered_nonce: 3, - ..Default::default() }, )); }); diff --git a/bridges/primitives/messages/src/lib.rs b/bridges/primitives/messages/src/lib.rs index 10aee6ce97b7b..8f6c946610960 100644 --- a/bridges/primitives/messages/src/lib.rs +++ b/bridges/primitives/messages/src/lib.rs @@ -191,6 +191,17 @@ impl InboundLaneData { .map(|entry| entry.messages.end) .unwrap_or(self.last_confirmed_nonce) } + + /// Returns the total number of messages in the `relayers` vector, + /// saturating in case of underflow or overflow. + pub fn total_unrewarded_messages(&self) -> MessageNonce { + let relayers = &self.relayers; + match (relayers.front(), relayers.back()) { + (Some(front), Some(back)) => + (front.messages.begin..=back.messages.end).saturating_len(), + _ => 0, + } + } } /// Outbound message details, returned by runtime APIs. @@ -287,7 +298,7 @@ impl DeliveredMessages { /// Return total count of delivered messages. pub fn total_messages(&self) -> MessageNonce { - (self.begin..=self.end).checked_len().unwrap_or(0) + (self.begin..=self.end).saturating_len() } /// Note new dispatched message. @@ -318,6 +329,13 @@ pub struct UnrewardedRelayersState { pub last_delivered_nonce: MessageNonce, } +impl UnrewardedRelayersState { + // Verify that the relayers state corresponds with the `InboundLaneData`. + pub fn is_valid(&self, lane_data: &InboundLaneData) -> bool { + self == &lane_data.into() + } +} + impl From<&InboundLaneData> for UnrewardedRelayersState { fn from(lane: &InboundLaneData) -> UnrewardedRelayersState { UnrewardedRelayersState { @@ -325,9 +343,9 @@ impl From<&InboundLaneData> for UnrewardedRelayersState { messages_in_oldest_entry: lane .relayers .front() - .and_then(|entry| (entry.messages.begin..=entry.messages.end).checked_len()) + .map(|entry| entry.messages.total_messages()) .unwrap_or(0), - total_messages: total_unrewarded_messages(&lane.relayers).unwrap_or(MessageNonce::MAX), + total_messages: lane.total_unrewarded_messages(), last_delivered_nonce: lane.last_delivered_nonce(), } } @@ -357,24 +375,6 @@ impl Default for OutboundLaneData { } } -/// Returns total number of messages in the `InboundLaneData::relayers` vector. -/// -/// Returns `None` if there are more messages that `MessageNonce` may fit (i.e. `MessageNonce + 1`). -pub fn total_unrewarded_messages( - relayers: &VecDeque>, -) -> Option { - match (relayers.front(), relayers.back()) { - (Some(front), Some(back)) => { - if let Some(difference) = back.messages.end.checked_sub(front.messages.begin) { - difference.checked_add(1) - } else { - Some(0) - } - }, - _ => Some(0), - } -} - /// Calculate the number of messages that the relayers have delivered. pub fn calc_relayers_rewards( messages_relayers: VecDeque>, @@ -447,20 +447,19 @@ mod tests { #[test] fn total_unrewarded_messages_does_not_overflow() { - assert_eq!( - total_unrewarded_messages( - &vec![ - UnrewardedRelayer { relayer: 1, messages: DeliveredMessages::new(0) }, - UnrewardedRelayer { - relayer: 2, - messages: DeliveredMessages::new(MessageNonce::MAX) - }, - ] - .into_iter() - .collect() - ), - None, - ); + let lane_data = InboundLaneData { + relayers: vec![ + UnrewardedRelayer { relayer: 1, messages: DeliveredMessages::new(0) }, + UnrewardedRelayer { + relayer: 2, + messages: DeliveredMessages::new(MessageNonce::MAX), + }, + ] + .into_iter() + .collect(), + last_confirmed_nonce: 0, + }; + assert_eq!(lane_data.total_unrewarded_messages(), MessageNonce::MAX); } #[test] diff --git a/bridges/primitives/runtime/src/lib.rs b/bridges/primitives/runtime/src/lib.rs index df77745bc02dd..742ffe32ea753 100644 --- a/bridges/primitives/runtime/src/lib.rs +++ b/bridges/primitives/runtime/src/lib.rs @@ -35,7 +35,7 @@ pub use chain::{ UnderlyingChainProvider, }; pub use frame_support::storage::storage_prefix as storage_value_final_key; -use num_traits::{CheckedAdd, CheckedSub, One}; +use num_traits::{CheckedAdd, CheckedSub, One, SaturatingAdd, Zero}; pub use storage_proof::{ record_all_keys as record_all_trie_keys, Error as StorageProofError, ProofSize as StorageProofSize, RawStorageProof, StorageProofChecker, @@ -527,17 +527,27 @@ impl Debug for StrippableError { pub trait RangeInclusiveExt { /// Computes the length of the `RangeInclusive`, checking for underflow and overflow. fn checked_len(&self) -> Option; + /// Computes the length of the `RangeInclusive`, saturating in case of underflow or overflow. + fn saturating_len(&self) -> Idx; } impl RangeInclusiveExt for RangeInclusive where - Idx: CheckedSub + CheckedAdd + One, + Idx: CheckedSub + CheckedAdd + SaturatingAdd + One + Zero, { fn checked_len(&self) -> Option { self.end() .checked_sub(self.start()) .and_then(|len| len.checked_add(&Idx::one())) } + + fn saturating_len(&self) -> Idx { + let len = match self.end().checked_sub(self.start()) { + Some(len) => len, + None => return Idx::zero(), + }; + len.saturating_add(&Idx::one()) + } } #[cfg(test)] diff --git a/bridges/relays/lib-substrate-relay/src/messages_target.rs b/bridges/relays/lib-substrate-relay/src/messages_target.rs index 68b68b1993647..f47b48010ecdd 100644 --- a/bridges/relays/lib-substrate-relay/src/messages_target.rs +++ b/bridges/relays/lib-substrate-relay/src/messages_target.rs @@ -31,8 +31,8 @@ use crate::{ use async_std::sync::Arc; use async_trait::async_trait; use bp_messages::{ - storage_keys::inbound_lane_data_key, total_unrewarded_messages, InboundLaneData, LaneId, - MessageNonce, UnrewardedRelayersState, + storage_keys::inbound_lane_data_key, InboundLaneData, LaneId, MessageNonce, + UnrewardedRelayersState, }; use bridge_runtime_common::messages::source::FromBridgedChainMessagesDeliveryProof; use messages_relay::{ @@ -45,7 +45,7 @@ use relay_substrate_client::{ }; use relay_utils::relay_loop::Client as RelayClient; use sp_core::Pair; -use std::{collections::VecDeque, convert::TryFrom, ops::RangeInclusive}; +use std::{convert::TryFrom, ops::RangeInclusive}; /// Message receiving proof returned by the target Substrate node. pub type SubstrateMessagesDeliveryProof = @@ -197,20 +197,9 @@ where id: TargetHeaderIdOf>, ) -> Result<(TargetHeaderIdOf>, UnrewardedRelayersState), SubstrateError> { - let inbound_lane_data = self.inbound_lane_data(id).await?; - let last_delivered_nonce = - inbound_lane_data.as_ref().map(|data| data.last_delivered_nonce()).unwrap_or(0); - let relayers = inbound_lane_data.map(|data| data.relayers).unwrap_or_else(VecDeque::new); - let unrewarded_relayers_state = bp_messages::UnrewardedRelayersState { - unrewarded_relayer_entries: relayers.len() as _, - messages_in_oldest_entry: relayers - .front() - .map(|entry| 1 + entry.messages.end - entry.messages.begin) - .unwrap_or(0), - total_messages: total_unrewarded_messages(&relayers).unwrap_or(MessageNonce::MAX), - last_delivered_nonce, - }; - Ok((id, unrewarded_relayers_state)) + let inbound_lane_data = + self.inbound_lane_data(id).await?.unwrap_or(InboundLaneData::default()); + Ok((id, (&inbound_lane_data).into())) } async fn prove_messages_receiving(