Impl review suggestions from #2021 (#2036)

* unrewarded_relayers for ReceiveMessagesProofInfo only

* simplify return

* removed comment

* appends_to_stored_nonce

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<UnrewardedRelayerOccupation>,
+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 {
+/// Info about a `ReceiveMessagesProof` call which tries to update a single lane.
-	/// Returns true if `bundled_range` cannot be directly appended to the `best_stored_nonce`
+#[derive(PartialEq, RuntimeDebug)]
-	/// or if the `bundled_range` is empty (unless we're confirming rewards when unrewarded
+pub struct ReceiveMessagesProofInfo {
-	/// relayers vector is full).
+	/// 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() {
+		if self.base.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));
 			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;
+				self.unrewarded_relayers.free_message_slots == 0;
-			if empty_transactions_allowed {
-				return false
-			}
 
-			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<T: Config<I>, I: 'static> CallHelper<T, I> {
 		match info {
 			CallInfo::ReceiveMessagesProof(info) => {
 				let inbound_lane_data =
-					pallet_bridge_messages::InboundLanes::<T, I>::get(info.0.lane_id);
+					pallet_bridge_messages::InboundLanes::<T, I>::get(info.base.lane_id);
-				if info.0.bundled_range.is_empty() {
+				if info.base.bundled_range.is_empty() {
-					match info.0.unrewarded_relayers {
+					let post_occupation =
-						Some(ref pre_occupation) => {
+						unrewarded_relayers_occupation::<T, I>(&inbound_lane_data);
-							let post_occupation =
+					// we don't care about `free_relayer_slots` here - it is checked in
-								unrewarded_relayers_occupation::<T, I>(&inbound_lane_data);
+					// `is_obsolete` and every relayer has delivered at least one message,
-							// we don't care about `free_relayer_slots` here - it is checked in
+					// so if relayer slots are released, then message slots are also
-							// `is_obsolete` and every relayer has delivered at least one message,
+					// released
-							// so if relayer slots are released, then message slots are also
+					return post_occupation.free_message_slots >
-							// released
+						info.unrewarded_relayers.free_message_slots
-							return post_occupation.free_message_slots >
-								pre_occupation.free_message_slots
-						},
-						None => {
-							// shouldn't happen in practice, given our code
-							return false
-						},
-					}
 				}
 
-				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::<T, I>::get(proof.lane);
 
-			return Some(ReceiveMessagesProofInfo(BaseMessagesProofInfo {
+			return Some(ReceiveMessagesProofInfo {
-				lane_id: proof.lane,
+				base: BaseMessagesProofInfo {
-				// we want all messages in this range to be new for us. Otherwise transaction will
+					lane_id: proof.lane,
-				// be considered obsolete.
+					// we want all messages in this range to be new for us. Otherwise transaction
-				bundled_range: proof.nonces_start..=proof.nonces_end,
+					// will be considered obsolete.
-				best_stored_nonce: inbound_lane_data.last_delivered_nonce(),
+					bundled_range: proof.nonces_start..=proof.nonces_end,
-				unrewarded_relayers: Some(unrewarded_relayers_occupation::<T, I>(
+					best_stored_nonce: inbound_lane_data.last_delivered_nonce(),
-					&inbound_lane_data,
+				},
-				)),
+				unrewarded_relayers: unrewarded_relayers_occupation::<T, I>(&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<CallInfo> {
 		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<T: Config<I>, 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::<TestRuntime, ()>::was_successful(&CallInfo::ReceiveMessagesProof(
-			ReceiveMessagesProofInfo(BaseMessagesProofInfo {
+			ReceiveMessagesProofInfo {
-				lane_id: LaneId([0, 0, 0, 0]),
+				base: BaseMessagesProofInfo {
-				bundled_range,
+					lane_id: LaneId([0, 0, 0, 0]),
-				best_stored_nonce: 0, // doesn't matter for `was_successful`
+					bundled_range,
-				unrewarded_relayers: Some(UnrewardedRelayerOccupation {
+					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,
 			}),
 		))
 	}

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(
+				MessagesCallInfo::ReceiveMessagesProof(ReceiveMessagesProofInfo {
-					BaseMessagesProofInfo {
+					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(
+				MessagesCallInfo::ReceiveMessagesProof(ReceiveMessagesProofInfo {
-					BaseMessagesProofInfo {
+					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 {
+				ReceiveMessagesProofInfo {
-					lane_id: TEST_LANE_ID,
+					base: BaseMessagesProofInfo {
-					bundled_range: 101..=200,
+						lane_id: TEST_LANE_ID,
-					best_stored_nonce: 100,
+						bundled_range: 101..=200,
-					unrewarded_relayers: Some(UnrewardedRelayerOccupation {
+						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,
 				}),
 			)),
 		}