Skip to content

Commit

Permalink
Remove dispatch_result field (paritytech#1660)
Browse files Browse the repository at this point in the history
* remove dispatch_result field

* fix benchmarks
  • Loading branch information
svyatonik authored and serban300 committed Apr 10, 2024
1 parent 501d207 commit 056f8dc
Show file tree
Hide file tree
Showing 16 changed files with 41 additions and 180 deletions.
1 change: 0 additions & 1 deletion bridges/bin/millau/runtime/src/rialto_messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,6 @@ mod tests {
let max_incoming_inbound_lane_data_proof_size =
bp_messages::InboundLaneData::<()>::encoded_size_hint_u32(
bp_millau::MAX_UNREWARDED_RELAYERS_IN_CONFIRMATION_TX as _,
bp_millau::MAX_UNCONFIRMED_MESSAGES_IN_CONFIRMATION_TX as _,
);
pallet_bridge_messages::ensure_able_to_receive_confirmation::<Weights>(
bp_millau::Millau::max_extrinsic_size(),
Expand Down
1 change: 0 additions & 1 deletion bridges/bin/millau/runtime/src/xcm_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,6 @@ mod tests {
assert_eq!(
dispatch_result,
MessageDispatchResult {
dispatch_result: true,
unspent_weight: frame_support::weights::Weight::from_ref_time(0),
dispatch_fee_paid_during_dispatch: false,
}
Expand Down
1 change: 0 additions & 1 deletion bridges/bin/rialto-parachain/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -895,7 +895,6 @@ mod tests {
assert_eq!(
dispatch_result,
MessageDispatchResult {
dispatch_result: true,
unspent_weight: frame_support::weights::Weight::from_ref_time(0),
dispatch_fee_paid_during_dispatch: false,
}
Expand Down
1 change: 0 additions & 1 deletion bridges/bin/rialto/runtime/src/millau_messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,6 @@ mod tests {
let max_incoming_inbound_lane_data_proof_size =
bp_messages::InboundLaneData::<()>::encoded_size_hint_u32(
bp_rialto::MAX_UNREWARDED_RELAYERS_IN_CONFIRMATION_TX as _,
bp_rialto::MAX_UNCONFIRMED_MESSAGES_IN_CONFIRMATION_TX as _,
);
pallet_bridge_messages::ensure_able_to_receive_confirmation::<Weights>(
bp_rialto::Rialto::max_extrinsic_size(),
Expand Down
1 change: 0 additions & 1 deletion bridges/bin/rialto/runtime/src/xcm_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,6 @@ mod tests {
assert_eq!(
dispatch_result,
MessageDispatchResult {
dispatch_result: true,
unspent_weight: frame_support::weights::Weight::from_ref_time(0),
dispatch_fee_paid_during_dispatch: false,
}
Expand Down
9 changes: 3 additions & 6 deletions bridges/bin/runtime-common/src/messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ pub mod target {
};

let xcm_outcome = do_dispatch();
let dispatch_result = match xcm_outcome {
match xcm_outcome {
Ok(outcome) => {
log::trace!(
target: "runtime::bridge-dispatch",
Expand All @@ -520,15 +520,14 @@ pub mod target {
outcome,
);
match outcome.ensure_execution() {
Ok(_weight) => true,
Ok(_weight) => (),
Err(e) => {
log::error!(
target: "runtime::bridge-dispatch",
"Incoming message {:?} was not dispatched, error: {:?}",
message_id,
e,
);
false
},
}
},
Expand All @@ -539,12 +538,10 @@ pub mod target {
message_id,
e,
);
false
},
};
}

MessageDispatchResult {
dispatch_result,
unspent_weight: Weight::zero(),
dispatch_fee_paid_during_dispatch: false,
}
Expand Down
1 change: 0 additions & 1 deletion bridges/modules/messages/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ edition = "2021"
license = "GPL-3.0-or-later WITH Classpath-exception-2.0"

[dependencies]
bitvec = { version = "1", default-features = false, features = ["alloc"] }
codec = { package = "parity-scale-codec", version = "3.1.5", default-features = false }
log = { version = "0.4.17", default-features = false }
num-traits = { version = "0.2", default-features = false }
Expand Down
12 changes: 6 additions & 6 deletions bridges/modules/messages/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ benchmarks_instance_pallet! {
inbound_lane_data: InboundLaneData {
relayers: vec![UnrewardedRelayer {
relayer: relayer_id.clone(),
messages: DeliveredMessages::new(1, true),
messages: DeliveredMessages::new(1),
}].into_iter().collect(),
last_confirmed_nonce: 0,
},
Expand Down Expand Up @@ -342,8 +342,8 @@ benchmarks_instance_pallet! {
total_messages: 2,
last_delivered_nonce: 2,
};
let mut delivered_messages = DeliveredMessages::new(1, true);
delivered_messages.note_dispatched_message(true);
let mut delivered_messages = DeliveredMessages::new(1);
delivered_messages.note_dispatched_message();
let proof = T::prepare_message_delivery_proof(MessageDeliveryProofParams {
lane: T::bench_lane_id(),
inbound_lane_data: InboundLaneData {
Expand Down Expand Up @@ -387,11 +387,11 @@ benchmarks_instance_pallet! {
relayers: vec![
UnrewardedRelayer {
relayer: relayer1_id.clone(),
messages: DeliveredMessages::new(1, true),
messages: DeliveredMessages::new(1),
},
UnrewardedRelayer {
relayer: relayer2_id.clone(),
messages: DeliveredMessages::new(2, true),
messages: DeliveredMessages::new(2),
},
].into_iter().collect(),
last_confirmed_nonce: 0,
Expand All @@ -414,7 +414,7 @@ fn receive_messages<T: Config<I>, I: 'static>(nonce: MessageNonce) {
inbound_lane_storage.set_data(InboundLaneData {
relayers: vec![UnrewardedRelayer {
relayer: T::bridged_relayer_id(),
messages: DeliveredMessages::new(nonce, true),
messages: DeliveredMessages::new(nonce),
}]
.into_iter()
.collect(),
Expand Down
9 changes: 2 additions & 7 deletions bridges/modules/messages/src/inbound_lane.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ impl<T: Config<I>, I: 'static> MaxEncodedLen for StoredInboundLaneData<T, I> {
fn max_encoded_len() -> usize {
InboundLaneData::<T::InboundRelayer>::encoded_size_hint(
T::MaxUnrewardedRelayerEntriesAtInboundLane::get() as usize,
T::MaxUnconfirmedMessagesAtInboundLane::get() as usize,
)
.unwrap_or(usize::MAX)
}
Expand Down Expand Up @@ -150,10 +149,6 @@ impl<S: InboundLaneStorage> InboundLane<S> {
// overlap.
match data.relayers.front_mut() {
Some(entry) if entry.messages.begin < new_confirmed_nonce => {
entry.messages.dispatch_results = entry
.messages
.dispatch_results
.split_off((new_confirmed_nonce + 1 - entry.messages.begin) as _);
entry.messages.begin = new_confirmed_nonce + 1;
},
_ => {},
Expand Down Expand Up @@ -200,15 +195,15 @@ impl<S: InboundLaneStorage> InboundLane<S> {
// now let's update inbound lane storage
let push_new = match data.relayers.back_mut() {
Some(entry) if entry.relayer == *relayer_at_bridged_chain => {
entry.messages.note_dispatched_message(dispatch_result.dispatch_result);
entry.messages.note_dispatched_message();
false
},
_ => true,
};
if push_new {
data.relayers.push_back(UnrewardedRelayer {
relayer: (*relayer_at_bridged_chain).clone(),
messages: DeliveredMessages::new(nonce, dispatch_result.dispatch_result),
messages: DeliveredMessages::new(nonce),
});
}
self.storage.set_data(data);
Expand Down
10 changes: 5 additions & 5 deletions bridges/modules/messages/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -987,7 +987,7 @@ mod tests {
last_confirmed_nonce: 1,
relayers: vec![UnrewardedRelayer {
relayer: 0,
messages: DeliveredMessages::new(1, true),
messages: DeliveredMessages::new(1),
}]
.into_iter()
.collect(),
Expand All @@ -1007,7 +1007,7 @@ mod tests {
phase: Phase::Initialization,
event: TestEvent::Messages(Event::MessagesDelivered {
lane_id: TEST_LANE_ID,
messages: DeliveredMessages::new(1, true),
messages: DeliveredMessages::new(1),
}),
topics: vec![],
}],
Expand Down Expand Up @@ -1622,8 +1622,8 @@ mod tests {

// messages 1+2 are confirmed in 1 tx, message 3 in a separate tx
// dispatch of message 2 has failed
let mut delivered_messages_1_and_2 = DeliveredMessages::new(1, true);
delivered_messages_1_and_2.note_dispatched_message(false);
let mut delivered_messages_1_and_2 = DeliveredMessages::new(1);
delivered_messages_1_and_2.note_dispatched_message();
let messages_1_and_2_proof = Ok((
TEST_LANE_ID,
InboundLaneData {
Expand All @@ -1636,7 +1636,7 @@ mod tests {
.collect(),
},
));
let delivered_message_3 = DeliveredMessages::new(3, true);
let delivered_message_3 = DeliveredMessages::new(3);
let messages_3_proof = Ok((
TEST_LANE_ID,
InboundLaneData {
Expand Down
15 changes: 1 addition & 14 deletions bridges/modules/messages/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

use crate::{calc_relayers_rewards, Config};

use bitvec::prelude::*;
use bp_messages::{
source_chain::{LaneMessageVerifier, MessageDeliveryAndDispatchPayment, TargetHeaderChain},
target_chain::{
Expand Down Expand Up @@ -394,7 +393,6 @@ pub const fn message_payload(id: u64, declared_weight: u64) -> TestPayload {
/// Returns message dispatch result with given unspent weight.
pub const fn dispatch_result(unspent_weight: u64) -> MessageDispatchResult {
MessageDispatchResult {
dispatch_result: true,
unspent_weight: Weight::from_ref_time(unspent_weight),
dispatch_fee_paid_during_dispatch: true,
}
Expand All @@ -406,18 +404,7 @@ pub fn unrewarded_relayer(
end: MessageNonce,
relayer: TestRelayer,
) -> UnrewardedRelayer<TestRelayer> {
UnrewardedRelayer {
relayer,
messages: DeliveredMessages {
begin,
end,
dispatch_results: if end >= begin {
bitvec![u8, Msb0; 1; (end - begin + 1) as _]
} else {
Default::default()
},
},
}
UnrewardedRelayer { relayer, messages: DeliveredMessages { begin, end } }
}

/// Run pallet test.
Expand Down
78 changes: 9 additions & 69 deletions bridges/modules/messages/src/outbound_lane.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,8 @@

use crate::Config;

use bitvec::prelude::*;
use bp_messages::{
DeliveredMessages, DispatchResultsBitVec, LaneId, MessageNonce, MessagePayload,
OutboundLaneData, UnrewardedRelayer,
DeliveredMessages, LaneId, MessageNonce, MessagePayload, OutboundLaneData, UnrewardedRelayer,
};
use frame_support::{
weights::{RuntimeDbWeight, Weight},
Expand Down Expand Up @@ -67,9 +65,6 @@ pub enum ReceivalConfirmationResult {
/// The unrewarded relayers vec contains non-consecutive entries. May be a result of invalid
/// bridged chain storage.
NonConsecutiveUnrewardedRelayerEntries,
/// The unrewarded relayers vec contains entry with mismatched number of dispatch results. May
/// be a result of invalid bridged chain storage.
InvalidNumberOfDispatchResults,
/// The chain has more messages that need to be confirmed than there is in the proof.
TryingToConfirmMoreMessagesThanExpected(MessageNonce),
}
Expand Down Expand Up @@ -129,14 +124,9 @@ impl<S: OutboundLaneStorage> OutboundLane<S> {
)
}

let dispatch_results = match extract_dispatch_results(
data.latest_received_nonce,
latest_delivered_nonce,
relayers,
) {
Ok(dispatch_results) => dispatch_results,
Err(extract_error) => return extract_error,
};
if let Err(e) = ensure_unrewarded_relayers_are_correct(latest_delivered_nonce, relayers) {
return e
}

let prev_latest_received_nonce = data.latest_received_nonce;
data.latest_received_nonce = latest_delivered_nonce;
Expand All @@ -145,7 +135,6 @@ impl<S: OutboundLaneStorage> OutboundLane<S> {
ReceivalConfirmationResult::ConfirmedMessages(DeliveredMessages {
begin: prev_latest_received_nonce + 1,
end: latest_delivered_nonce,
dispatch_results,
})
}

Expand Down Expand Up @@ -180,21 +169,14 @@ impl<S: OutboundLaneStorage> OutboundLane<S> {
}
}

/// Extract new dispatch results from the unrewarded relayers vec.
/// Verifies unrewarded relayers vec.
///
/// Returns `Err(_)` if unrewarded relayers vec contains invalid data, meaning that the bridged
/// chain has invalid runtime storage.
fn extract_dispatch_results<RelayerId>(
prev_latest_received_nonce: MessageNonce,
fn ensure_unrewarded_relayers_are_correct<RelayerId>(
latest_received_nonce: MessageNonce,
relayers: &VecDeque<UnrewardedRelayer<RelayerId>>,
) -> Result<DispatchResultsBitVec, ReceivalConfirmationResult> {
// the only caller of this functions checks that the
// prev_latest_received_nonce..=latest_received_nonce is valid, so we're ready to accept
// messages in this range => with_capacity call must succeed here or we'll be unable to receive
// confirmations at all
let mut received_dispatch_result =
BitVec::with_capacity((latest_received_nonce - prev_latest_received_nonce + 1) as _);
) -> Result<(), ReceivalConfirmationResult> {
let mut last_entry_end: Option<MessageNonce> = None;
for entry in relayers {
// unrewarded relayer entry must have at least 1 unconfirmed message
Expand All @@ -219,33 +201,9 @@ fn extract_dispatch_results<RelayerId>(
// this is detected now
return Err(ReceivalConfirmationResult::FailedToConfirmFutureMessages)
}
// entry must have single dispatch result for every message
// (guaranteed by the `InboundLane::receive_message()`)
if entry.messages.dispatch_results.len() as MessageNonce !=
entry.messages.end - entry.messages.begin + 1
{
return Err(ReceivalConfirmationResult::InvalidNumberOfDispatchResults)
}

// now we know that the entry is valid
// => let's check if it brings new confirmations
let new_messages_begin =
sp_std::cmp::max(entry.messages.begin, prev_latest_received_nonce + 1);
let new_messages_end = sp_std::cmp::min(entry.messages.end, latest_received_nonce);
let new_messages_range = new_messages_begin..=new_messages_end;
if new_messages_range.is_empty() {
continue
}

// now we know that entry brings new confirmations
// => let's extract dispatch results
received_dispatch_result.extend_from_bitslice(
&entry.messages.dispatch_results
[(new_messages_begin - entry.messages.begin) as usize..],
);
}

Ok(received_dispatch_result)
Ok(())
}

#[cfg(test)]
Expand All @@ -270,11 +228,7 @@ mod tests {
}

fn delivered_messages(nonces: RangeInclusive<MessageNonce>) -> DeliveredMessages {
DeliveredMessages {
begin: *nonces.start(),
end: *nonces.end(),
dispatch_results: bitvec![u8, Msb0; 1; (nonces.end() - nonces.start() + 1) as _],
}
DeliveredMessages { begin: *nonces.start(), end: *nonces.end() }
}

fn assert_3_messages_confirmation_fails(
Expand Down Expand Up @@ -407,20 +361,6 @@ mod tests {
);
}

#[test]
fn confirm_delivery_fails_if_number_of_dispatch_results_in_entry_is_invalid() {
let mut relayers: VecDeque<_> = unrewarded_relayers(1..=1)
.into_iter()
.chain(unrewarded_relayers(2..=2).into_iter())
.chain(unrewarded_relayers(3..=3).into_iter())
.collect();
relayers[0].messages.dispatch_results.clear();
assert_eq!(
assert_3_messages_confirmation_fails(3, &relayers),
ReceivalConfirmationResult::InvalidNumberOfDispatchResults,
);
}

#[test]
fn prune_messages_works() {
run_test(|| {
Expand Down
2 changes: 0 additions & 2 deletions bridges/primitives/messages/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ edition = "2021"
license = "GPL-3.0-or-later WITH Classpath-exception-2.0"

[dependencies]
bitvec = { version = "1", default-features = false, features = ["alloc"] }
codec = { package = "parity-scale-codec", version = "3.1.5", default-features = false, features = ["derive", "bit-vec"] }
impl-trait-for-tuples = "0.2"
scale-info = { version = "2.1.1", default-features = false, features = ["bit-vec", "derive"] }
Expand All @@ -30,7 +29,6 @@ hex-literal = "0.3"
[features]
default = ["std"]
std = [
"bitvec/std",
"bp-runtime/std",
"codec/std",
"frame-support/std",
Expand Down
Loading

0 comments on commit 056f8dc

Please sign in to comment.