Skip to content
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

Impl review suggestions from #2021 #2036

Merged
merged 4 commits into from
Apr 13, 2023
Merged

Impl review suggestions from #2021 #2036

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92e41f9
unrewarded_relayers for ReceiveMessagesProofInfo only
svyatonik Apr 12, 2023
45d500e
simplify return
svyatonik Apr 12, 2023
34d9919
removed comment
svyatonik Apr 12, 2023
7bcea5d
appends_to_stored_nonce
svyatonik Apr 13, 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
133 changes: 64 additions & 69 deletions bin/runtime-common/src/messages_call_ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,6 @@ 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>,
}

/// Occupation state of the unrewarded relayers vector.
Expand All @@ -57,45 +54,53 @@ 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
}

return true
self.unrewarded_relayers.free_message_slots == 0;

return !empty_transactions_allowed;
}

// otherwise we require bundled messages to continue stored range
*self.bundled_range.start() != self.best_stored_nonce + 1
*self.base.bundled_range.start() != self.base.best_stored_nonce + 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Maybe we could extract *self.base.bundled_range.start() != self.base.best_stored_nonce + 1 in another method let's say BaseMessagesProofInfo::is_obsolete() since it's used both here and for ReceiveMessagesDeliveryProofInfo. In order to avoid duplication.

}
}

/// 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.bundled_range.start() != self.0.best_stored_nonce + 1
}
}

/// Info about a `ReceiveMessagesProof` or a `ReceiveMessagesDeliveryProof` call
/// which tries to update a single lane.
#[derive(PartialEq, RuntimeDebug)]
Expand All @@ -108,7 +113,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 +141,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 +212,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 +245,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 +266,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 +275,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 +285,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 +310,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 +555,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 +620,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 bin/runtime-common/src/refund_relayer_extension.rs
View file
Open in desktop
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