Skip to content

Commit

Permalink
Impl review suggestions from paritytech#2021 (paritytech#2036)
Browse files Browse the repository at this point in the history 
* unrewarded_relayers for ReceiveMessagesProofInfo only

* simplify return

* removed comment

* appends_to_stored_nonce
  • Loading branch information
@svyatonik @serban300
svyatonik authored and serban300 committed Apr 9, 2024
1 parent 85d8899 commit 6c79e4c
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 92 deletions.
137 changes: 69 additions & 68 deletions bridges/bin/runtime-common/src/messages_call_ext.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)]
Expand All @@ -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,
};

Expand Down Expand Up @@ -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);
if info.0.bundled_range.is_empty() {
match info.0.unrewarded_relayers {
Some(ref pre_occupation) => {
let post_occupation =
unrewarded_relayers_occupation::<T, I>(&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::<T, I>::get(info.base.lane_id);
if info.base.bundled_range.is_empty() {
let post_occupation =
unrewarded_relayers_occupation::<T, I>(&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 =
Expand Down Expand Up @@ -215,16 +218,16 @@ impl<
{
let inbound_lane_data = pallet_bridge_messages::InboundLanes::<T, I>::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::<T, I>(
&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::<T, I>(&inbound_lane_data),
})
}

None
Expand All @@ -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,
}))
}

Expand All @@ -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
Expand All @@ -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: {:?}",
Expand All @@ -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,
Expand All @@ -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);
Expand Down Expand Up @@ -561,19 +561,21 @@ mod tests {
is_empty: bool,
) -> bool {
CallHelper::<TestRuntime, ()>::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()
},
}),
}),
},
},
))
}

Expand Down Expand Up @@ -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,
}),
))
}
Expand Down
47 changes: 23 additions & 24 deletions bridges/bin/runtime-common/src/refund_relayer_extension.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
},
}),
),
}
}
Expand All @@ -747,7 +747,6 @@ mod tests {
lane_id: TEST_LANE_ID,
bundled_range: 101..=200,
best_stored_nonce: 100,
unrewarded_relayers: None,
},
)),
),
Expand All @@ -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(),
},
}),
),
}
}
Expand All @@ -792,7 +791,6 @@ mod tests {
lane_id: TEST_LANE_ID,
bundled_range: 101..=200,
best_stored_nonce: 100,
unrewarded_relayers: None,
},
)),
),
Expand All @@ -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(),
}),
}),
},
},
)),
}
}
Expand All @@ -824,7 +824,6 @@ mod tests {
lane_id: TEST_LANE_ID,
bundled_range: 101..=200,
best_stored_nonce: 100,
unrewarded_relayers: None,
}),
)),
}
Expand Down

0 comments on commit 6c79e4c

Please sign in to comment.