From 0d5cdac50fd747a21500884d94cc2871478aaaf4 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 13 Apr 2023 10:15:30 +0300 Subject: [PATCH] Impl review suggestions from #2021 (#2036) * unrewarded_relayers for ReceiveMessagesProofInfo only * simplify return * removed comment * appends_to_stored_nonce --- .../runtime-common/src/messages_call_ext.rs | 137 +++++++++--------- .../src/refund_relayer_extension.rs | 47 +++--- 2 files changed, 92 insertions(+), 92 deletions(-) diff --git a/bridges/bin/runtime-common/src/messages_call_ext.rs b/bridges/bin/runtime-common/src/messages_call_ext.rs index 8d53c4844c80f..b208a9d02b422 100644 --- a/bridges/bin/runtime-common/src/messages_call_ext.rs +++ b/bridges/bin/runtime-common/src/messages_call_ext.rs @@ -42,9 +42,13 @@ pub struct BaseMessagesProofInfo { /// For delivery transaction, it is the nonce of best delivered message before the call. /// For confirmation transaction, it is the nonce of best confirmed message before the call. pub best_stored_nonce: MessageNonce, - /// For message delivery transactions, the state of unrewarded relayers vector before the - /// call is dispatched. - pub unrewarded_relayers: Option, +} + +impl BaseMessagesProofInfo { + /// Returns true if `bundled_range` continues the `0..=best_stored_nonce` range. + fn appends_to_stored_nonce(&self) -> bool { + *self.bundled_range.start() == self.best_stored_nonce + 1 + } } /// Occupation state of the unrewarded relayers vector. @@ -57,45 +61,52 @@ pub struct UnrewardedRelayerOccupation { pub free_message_slots: MessageNonce, } -impl BaseMessagesProofInfo { - /// Returns true if `bundled_range` cannot be directly appended to the `best_stored_nonce` - /// or if the `bundled_range` is empty (unless we're confirming rewards when unrewarded - /// relayers vector is full). +/// Info about a `ReceiveMessagesProof` call which tries to update a single lane. +#[derive(PartialEq, RuntimeDebug)] +pub struct ReceiveMessagesProofInfo { + /// Base messages proof info + pub base: BaseMessagesProofInfo, + /// State of unrewarded relayers vector. + pub unrewarded_relayers: UnrewardedRelayerOccupation, +} + +impl ReceiveMessagesProofInfo { + /// Returns true if: + /// + /// - either inbound lane is ready to accept bundled messages; + /// + /// - or there are no bundled messages, but the inbound lane is blocked by too many unconfirmed + /// messages and/or unrewarded relayers. fn is_obsolete(&self) -> bool { // transactions with zero bundled nonces are not allowed, unless they're message // delivery transactions, which brings reward confirmations required to unblock // the lane - if self.bundled_range.is_empty() { - let (free_relayer_slots, free_message_slots) = self - .unrewarded_relayers - .as_ref() - .map(|s| (s.free_relayer_slots, s.free_message_slots)) - .unwrap_or((MessageNonce::MAX, MessageNonce::MAX)); + if self.base.bundled_range.is_empty() { let empty_transactions_allowed = // we allow empty transactions when we can't accept delivery from new relayers - free_relayer_slots == 0 || + self.unrewarded_relayers.free_relayer_slots == 0 || // or if we can't accept new messages at all - free_message_slots == 0; - if empty_transactions_allowed { - return false - } + self.unrewarded_relayers.free_message_slots == 0; - return true + return !empty_transactions_allowed } // otherwise we require bundled messages to continue stored range - *self.bundled_range.start() != self.best_stored_nonce + 1 + !self.base.appends_to_stored_nonce() } } -/// Info about a `ReceiveMessagesProof` call which tries to update a single lane. -#[derive(PartialEq, RuntimeDebug)] -pub struct ReceiveMessagesProofInfo(pub BaseMessagesProofInfo); - /// Info about a `ReceiveMessagesDeliveryProof` call which tries to update a single lane. #[derive(PartialEq, RuntimeDebug)] pub struct ReceiveMessagesDeliveryProofInfo(pub BaseMessagesProofInfo); +impl ReceiveMessagesDeliveryProofInfo { + /// Returns true if outbound lane is ready to accept confirmations of bundled messages. + fn is_obsolete(&self) -> bool { + self.0.bundled_range.is_empty() || !self.0.appends_to_stored_nonce() + } +} + /// Info about a `ReceiveMessagesProof` or a `ReceiveMessagesDeliveryProof` call /// which tries to update a single lane. #[derive(PartialEq, RuntimeDebug)] @@ -108,7 +119,7 @@ impl CallInfo { /// Returns number of messages, bundled with this transaction. pub fn bundled_messages(&self) -> MessageNonce { let bundled_range = match *self { - Self::ReceiveMessagesProof(ref info) => &info.0.bundled_range, + Self::ReceiveMessagesProof(ref info) => &info.base.bundled_range, Self::ReceiveMessagesDeliveryProof(ref info) => &info.0.bundled_range, }; @@ -136,27 +147,19 @@ impl, I: 'static> CallHelper { match info { CallInfo::ReceiveMessagesProof(info) => { let inbound_lane_data = - pallet_bridge_messages::InboundLanes::::get(info.0.lane_id); - if info.0.bundled_range.is_empty() { - match info.0.unrewarded_relayers { - Some(ref pre_occupation) => { - let post_occupation = - unrewarded_relayers_occupation::(&inbound_lane_data); - // we don't care about `free_relayer_slots` here - it is checked in - // `is_obsolete` and every relayer has delivered at least one message, - // so if relayer slots are released, then message slots are also - // released - return post_occupation.free_message_slots > - pre_occupation.free_message_slots - }, - None => { - // shouldn't happen in practice, given our code - return false - }, - } + pallet_bridge_messages::InboundLanes::::get(info.base.lane_id); + if info.base.bundled_range.is_empty() { + let post_occupation = + unrewarded_relayers_occupation::(&inbound_lane_data); + // we don't care about `free_relayer_slots` here - it is checked in + // `is_obsolete` and every relayer has delivered at least one message, + // so if relayer slots are released, then message slots are also + // released + return post_occupation.free_message_slots > + info.unrewarded_relayers.free_message_slots } - inbound_lane_data.last_delivered_nonce() == *info.0.bundled_range.end() + inbound_lane_data.last_delivered_nonce() == *info.base.bundled_range.end() }, CallInfo::ReceiveMessagesDeliveryProof(info) => { let outbound_lane_data = @@ -215,16 +218,16 @@ impl< { let inbound_lane_data = pallet_bridge_messages::InboundLanes::::get(proof.lane); - return Some(ReceiveMessagesProofInfo(BaseMessagesProofInfo { - lane_id: proof.lane, - // we want all messages in this range to be new for us. Otherwise transaction will - // be considered obsolete. - bundled_range: proof.nonces_start..=proof.nonces_end, - best_stored_nonce: inbound_lane_data.last_delivered_nonce(), - unrewarded_relayers: Some(unrewarded_relayers_occupation::( - &inbound_lane_data, - )), - })) + return Some(ReceiveMessagesProofInfo { + base: BaseMessagesProofInfo { + lane_id: proof.lane, + // we want all messages in this range to be new for us. Otherwise transaction + // will be considered obsolete. + bundled_range: proof.nonces_start..=proof.nonces_end, + best_stored_nonce: inbound_lane_data.last_delivered_nonce(), + }, + unrewarded_relayers: unrewarded_relayers_occupation::(&inbound_lane_data), + }) } None @@ -248,7 +251,6 @@ impl< bundled_range: outbound_lane_data.latest_received_nonce + 1..= relayers_state.last_delivered_nonce, best_stored_nonce: outbound_lane_data.latest_received_nonce, - unrewarded_relayers: None, })) } @@ -270,7 +272,7 @@ impl< fn call_info_for(&self, lane_id: LaneId) -> Option { self.call_info().filter(|info| { let actual_lane_id = match info { - CallInfo::ReceiveMessagesProof(info) => info.0.lane_id, + CallInfo::ReceiveMessagesProof(info) => info.base.lane_id, CallInfo::ReceiveMessagesDeliveryProof(info) => info.0.lane_id, }; actual_lane_id == lane_id @@ -279,7 +281,7 @@ impl< fn check_obsolete_call(&self) -> TransactionValidity { match self.call_info() { - Some(CallInfo::ReceiveMessagesProof(proof_info)) if proof_info.0.is_obsolete() => { + Some(CallInfo::ReceiveMessagesProof(proof_info)) if proof_info.is_obsolete() => { log::trace!( target: pallet_bridge_messages::LOG_TARGET, "Rejecting obsolete messages delivery transaction: {:?}", @@ -289,7 +291,7 @@ impl< return sp_runtime::transaction_validity::InvalidTransaction::Stale.into() }, Some(CallInfo::ReceiveMessagesDeliveryProof(proof_info)) - if proof_info.0.is_obsolete() => + if proof_info.is_obsolete() => { log::trace!( target: pallet_bridge_messages::LOG_TARGET, @@ -314,8 +316,6 @@ fn unrewarded_relayers_occupation, I: 'static>( free_relayer_slots: T::MaxUnrewardedRelayerEntriesAtInboundLane::get() .saturating_sub(inbound_lane_data.relayers.len() as MessageNonce), free_message_slots: { - // 5 - 0 = 5 ==> 1,2,3,4,5 - // 5 - 3 = 2 ==> 4,5 let unconfirmed_messages = inbound_lane_data .last_delivered_nonce() .saturating_sub(inbound_lane_data.last_confirmed_nonce); @@ -561,19 +561,21 @@ mod tests { is_empty: bool, ) -> bool { CallHelper::::was_successful(&CallInfo::ReceiveMessagesProof( - ReceiveMessagesProofInfo(BaseMessagesProofInfo { - lane_id: LaneId([0, 0, 0, 0]), - bundled_range, - best_stored_nonce: 0, // doesn't matter for `was_successful` - unrewarded_relayers: Some(UnrewardedRelayerOccupation { + ReceiveMessagesProofInfo { + base: BaseMessagesProofInfo { + lane_id: LaneId([0, 0, 0, 0]), + bundled_range, + best_stored_nonce: 0, // doesn't matter for `was_successful` + }, + unrewarded_relayers: UnrewardedRelayerOccupation { free_relayer_slots: 0, // doesn't matter for `was_successful` free_message_slots: if is_empty { 0 } else { MaxUnconfirmedMessagesAtInboundLane::get() }, - }), - }), + }, + }, )) } @@ -624,7 +626,6 @@ mod tests { lane_id: LaneId([0, 0, 0, 0]), bundled_range, best_stored_nonce: 0, // doesn't matter for `was_successful` - unrewarded_relayers: None, }), )) } diff --git a/bridges/bin/runtime-common/src/refund_relayer_extension.rs b/bridges/bin/runtime-common/src/refund_relayer_extension.rs index 83eafa1a36a2d..1aae6a16a3c1d 100644 --- a/bridges/bin/runtime-common/src/refund_relayer_extension.rs +++ b/bridges/bin/runtime-common/src/refund_relayer_extension.rs @@ -713,17 +713,17 @@ mod tests { para_id: ParaId(TestParachain::get()), para_head_hash: [1u8; 32].into(), }, - MessagesCallInfo::ReceiveMessagesProof(ReceiveMessagesProofInfo( - BaseMessagesProofInfo { + MessagesCallInfo::ReceiveMessagesProof(ReceiveMessagesProofInfo { + base: BaseMessagesProofInfo { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, - unrewarded_relayers: Some(UnrewardedRelayerOccupation { - free_relayer_slots: MaxUnrewardedRelayerEntriesAtInboundLane::get(), - free_message_slots: MaxUnconfirmedMessagesAtInboundLane::get(), - }), }, - )), + unrewarded_relayers: UnrewardedRelayerOccupation { + free_relayer_slots: MaxUnrewardedRelayerEntriesAtInboundLane::get(), + free_message_slots: MaxUnconfirmedMessagesAtInboundLane::get(), + }, + }), ), } } @@ -747,7 +747,6 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, - unrewarded_relayers: None, }, )), ), @@ -763,17 +762,17 @@ mod tests { para_id: ParaId(TestParachain::get()), para_head_hash: [1u8; 32].into(), }, - MessagesCallInfo::ReceiveMessagesProof(ReceiveMessagesProofInfo( - BaseMessagesProofInfo { + MessagesCallInfo::ReceiveMessagesProof(ReceiveMessagesProofInfo { + base: BaseMessagesProofInfo { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, - unrewarded_relayers: Some(UnrewardedRelayerOccupation { - free_relayer_slots: MaxUnrewardedRelayerEntriesAtInboundLane::get(), - free_message_slots: MaxUnconfirmedMessagesAtInboundLane::get(), - }), }, - )), + unrewarded_relayers: UnrewardedRelayerOccupation { + free_relayer_slots: MaxUnrewardedRelayerEntriesAtInboundLane::get(), + free_message_slots: MaxUnconfirmedMessagesAtInboundLane::get(), + }, + }), ), } } @@ -792,7 +791,6 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, - unrewarded_relayers: None, }, )), ), @@ -803,15 +801,17 @@ mod tests { PreDispatchData { relayer: relayer_account_at_this_chain(), call_info: CallInfo::Msgs(MessagesCallInfo::ReceiveMessagesProof( - ReceiveMessagesProofInfo(BaseMessagesProofInfo { - lane_id: TEST_LANE_ID, - bundled_range: 101..=200, - best_stored_nonce: 100, - unrewarded_relayers: Some(UnrewardedRelayerOccupation { + ReceiveMessagesProofInfo { + base: BaseMessagesProofInfo { + lane_id: TEST_LANE_ID, + bundled_range: 101..=200, + best_stored_nonce: 100, + }, + unrewarded_relayers: UnrewardedRelayerOccupation { free_relayer_slots: MaxUnrewardedRelayerEntriesAtInboundLane::get(), free_message_slots: MaxUnconfirmedMessagesAtInboundLane::get(), - }), - }), + }, + }, )), } } @@ -824,7 +824,6 @@ mod tests { lane_id: TEST_LANE_ID, bundled_range: 101..=200, best_stored_nonce: 100, - unrewarded_relayers: None, }), )), }