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

Jump to bottom

Fix invalid batch transaction #1957

Merged
merged 3 commits into from
Mar 14, 2023
Merged

Fix invalid batch transaction #1957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1976206
fix invalid batch transaction
svyatonik Mar 9, 2023
efd738c
RaceState is now trait
svyatonik Mar 9, 2023
9ddd174
clippy
svyatonik Mar 9, 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
28 changes: 21 additions & 7 deletions relays/lib-substrate-relay/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,18 +91,21 @@ impl<AccountId> TaggedAccount<AccountId> {
}

/// Batch call builder.
pub trait BatchCallBuilder<Call>: Send {
pub trait BatchCallBuilder<Call>: Clone + Send {
/// Create batch call from given calls vector.
fn build_batch_call(&self, _calls: Vec<Call>) -> Call;
}

/// Batch call builder constructor.
pub trait BatchCallBuilderConstructor<Call> {
pub trait BatchCallBuilderConstructor<Call>: Clone {
/// Call builder, used by this constructor.
type CallBuilder: BatchCallBuilder<Call>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

An alternative to this would be to add a method to BatchCallBuilder:
fn clone_box(&self) -> Box<dyn BatchCallBuilder<Call>>;

And then:

impl<Call> Clone for Box<dyn BatchCallBuilder<Call>> {
	fn clone(&self) -> Box<dyn BatchCallBuilder<Call>> {
		self.clone_box()
	}
}

Also we would need to use #[derive(CloneNoBound)] instead of #[derive(Clone)] for BatchProofTransaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it'd be better? I mean - I'm more in favor of associated types than Box<dyn> :) But if you think it'd be better - I could explore and fix it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure which version is best. The associated type should be safe also since for the () implementation of BatchCallBuilderConstructor, new_builder will always return None. I feel like with the Box<dyn> it would be safest, but it's true that there are cons to this as well.

Anyway, I'm ok with merging it like this (with associated types) and we can revisit the decision later.

/// Create a new instance of a batch call builder.
fn new_builder() -> Option<Box<dyn BatchCallBuilder<Call>>>;
fn new_builder() -> Option<Self::CallBuilder>;
}

/// Batch call builder based on `pallet-utility`.
#[derive(Clone)]
pub struct UtilityPalletBatchCallBuilder<C: Chain>(PhantomData<C>);

impl<C: Chain> BatchCallBuilder<C::Call> for UtilityPalletBatchCallBuilder<C>
Expand All @@ -118,14 +121,25 @@ impl<C: Chain> BatchCallBuilderConstructor<C::Call> for UtilityPalletBatchCallBu
where
C: ChainWithUtilityPallet,
{
fn new_builder() -> Option<Box<dyn BatchCallBuilder<C::Call>>> {
Some(Box::new(Self(Default::default())))
type CallBuilder = Self;

fn new_builder() -> Option<Self::CallBuilder> {
Some(Self(Default::default()))
}
}

/// A `BatchCallBuilderConstructor` that always returns `None`.
// A `BatchCallBuilderConstructor` that always returns `None`.
impl<Call> BatchCallBuilderConstructor<Call> for () {
fn new_builder() -> Option<Box<dyn BatchCallBuilder<Call>>> {
type CallBuilder = ();
fn new_builder() -> Option<Self::CallBuilder> {
None
}
}

// Dummy `BatchCallBuilder` implementation that must never be used outside
// of the `impl BatchCallBuilderConstructor for ()` code.
impl<Call> BatchCallBuilder<Call> for () {
fn build_batch_call(&self, _calls: Vec<Call>) -> Call {
unreachable!("never called, because ()::new_builder() returns None; qed")
}
}
13 changes: 12 additions & 1 deletion relays/lib-substrate-relay/src/messages_lane.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,26 @@ pub struct MessagesRelayParams<P: SubstrateMessageLane> {

/// Batch transaction that brings headers + and messages delivery/receiving confirmations to the
/// source node.
#[derive(Clone)]
pub struct BatchProofTransaction<SC: Chain, TC: Chain, B: BatchCallBuilderConstructor<CallOf<SC>>> {
builder: Box<dyn BatchCallBuilder<CallOf<SC>>>,
builder: B::CallBuilder,
proved_header: HeaderIdOf<TC>,
prove_calls: Vec<CallOf<SC>>,

/// Using `fn() -> B` in order to avoid implementing `Send` for `B`.
_phantom: PhantomData<fn() -> B>,
}

impl<SC: Chain, TC: Chain, B: BatchCallBuilderConstructor<CallOf<SC>>> std::fmt::Debug
for BatchProofTransaction<SC, TC, B>
{
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
fmt.debug_struct("BatchProofTransaction")
.field("proved_header", &self.proved_header)
.finish()
}
}

impl<SC: Chain, TC: Chain, B: BatchCallBuilderConstructor<CallOf<SC>>>
BatchProofTransaction<SC, TC, B>
{
Expand Down
13 changes: 10 additions & 3 deletions relays/messages/src/message_lane_loop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ pub struct NoncesSubmitArtifacts<T> {

/// Batch transaction that already submit some headers and needs to be extended with
/// messages/delivery proof before sending.
pub trait BatchTransaction<HeaderId>: Send {
pub trait BatchTransaction<HeaderId>: Debug + Send {
/// Header that was required in the original call and which is bundled within this
/// batch transaction.
fn required_header_id(&self) -> HeaderId;
Expand All @@ -121,7 +121,7 @@ pub trait BatchTransaction<HeaderId>: Send {
#[async_trait]
pub trait SourceClient<P: MessageLane>: RelayClient {
/// Type of batch transaction that submits finality and message receiving proof.
type BatchTransaction: BatchTransaction<TargetHeaderIdOf<P>>;
type BatchTransaction: BatchTransaction<TargetHeaderIdOf<P>> + Clone;
/// Transaction tracker to track submitted transactions.
type TransactionTracker: TransactionTracker<HeaderId = SourceHeaderIdOf<P>>;

Expand Down Expand Up @@ -186,7 +186,7 @@ pub trait SourceClient<P: MessageLane>: RelayClient {
#[async_trait]
pub trait TargetClient<P: MessageLane>: RelayClient {
/// Type of batch transaction that submits finality and messages proof.
type BatchTransaction: BatchTransaction<SourceHeaderIdOf<P>>;
type BatchTransaction: BatchTransaction<SourceHeaderIdOf<P>> + Clone;
/// Transaction tracker to track submitted transactions.
type TransactionTracker: TransactionTracker<HeaderId = TargetHeaderIdOf<P>>;

Expand Down Expand Up @@ -1212,6 +1212,9 @@ pub(crate) mod tests {
original_data,
Arc::new(|_| {}),
Arc::new(move |data: &mut TestClientData| {
data.source_state.best_self =
HeaderId(data.source_state.best_self.0 + 1, data.source_state.best_self.1 + 1);
data.source_state.best_finalized_self = data.source_state.best_self;
if let Some(target_to_source_header_required) =
data.target_to_source_header_required.take()
{
Expand All @@ -1223,6 +1226,10 @@ pub(crate) mod tests {
}),
Arc::new(|_| {}),
Arc::new(move |data: &mut TestClientData| {
data.target_state.best_self =
HeaderId(data.target_state.best_self.0 + 1, data.target_state.best_self.1 + 1);
data.target_state.best_finalized_self = data.target_state.best_self;

if let Some(source_to_target_header_required) =
data.source_to_target_header_required.take()
{
Expand Down
66 changes: 43 additions & 23 deletions relays/messages/src/message_race_delivery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,13 +322,19 @@ where
self.strategy.is_empty()
}

fn required_source_header_at_target(
fn required_source_header_at_target<RS: RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>>>(
&self,
current_best: &SourceHeaderIdOf<P>,
race_state: RS,
) -> Option<SourceHeaderIdOf<P>> {
// we have already submitted something - let's wait until it is mined
if race_state.nonces_submitted().is_some() {
return None
}

let has_nonces_to_deliver = !self.strategy.is_empty();
let header_required_for_messages_delivery =
self.strategy.required_source_header_at_target(current_best);
self.strategy.required_source_header_at_target(current_best, race_state);
let header_required_for_reward_confirmations_delivery = self
.latest_confirmed_nonces_at_source
.back()
Expand Down Expand Up @@ -381,10 +387,10 @@ where
self.strategy.source_nonces_updated(at_block, nonces)
}

fn best_target_nonces_updated(
fn best_target_nonces_updated<RS: RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>>>(
&mut self,
nonces: TargetClientNonces<DeliveryRaceTargetNoncesData>,
race_state: &mut RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>, P::MessagesProof>,
race_state: &mut RS,
) {
// best target nonces must always be ge than finalized target nonces
let latest_nonce = nonces.latest_nonce;
Expand All @@ -396,13 +402,13 @@ where
)
}

fn finalized_target_nonces_updated(
fn finalized_target_nonces_updated<RS: RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>>>(
&mut self,
nonces: TargetClientNonces<DeliveryRaceTargetNoncesData>,
race_state: &mut RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>, P::MessagesProof>,
race_state: &mut RS,
) {
if let Some(ref best_finalized_source_header_id_at_best_target) =
race_state.best_finalized_source_header_id_at_best_target
race_state.best_finalized_source_header_id_at_best_target()
{
let oldest_header_number_to_keep = best_finalized_source_header_id_at_best_target.0;
while self
Expand All @@ -426,13 +432,13 @@ where
)
}

async fn select_nonces_to_deliver(
async fn select_nonces_to_deliver<RS: RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>>>(
&self,
race_state: RaceState<SourceHeaderIdOf<P>, TargetHeaderIdOf<P>, P::MessagesProof>,
race_state: RS,
) -> Option<(RangeInclusive<MessageNonce>, Self::ProofParameters)> {
let best_target_nonce = self.strategy.best_at_target()?;
let best_finalized_source_header_id_at_best_target =
race_state.best_finalized_source_header_id_at_best_target.clone()?;
race_state.best_finalized_source_header_id_at_best_target()?;
let latest_confirmed_nonce_at_source = self
.latest_confirmed_nonces_at_source
.iter()
Expand Down Expand Up @@ -576,20 +582,29 @@ impl<SourceChainBalance: std::fmt::Debug> NoncesRange for MessageDetailsMap<Sour

#[cfg(test)]
mod tests {
use crate::message_lane_loop::{
tests::{
header_id, TestMessageLane, TestMessagesProof, TestSourceChainBalance,
TestSourceClient, TestSourceHeaderId, TestTargetClient, TestTargetHeaderId,
use crate::{
message_lane_loop::{
tests::{
header_id, TestMessageLane, TestMessagesBatchTransaction, TestMessagesProof,
TestSourceChainBalance, TestSourceClient, TestSourceHeaderId, TestTargetClient,
TestTargetHeaderId,
},
MessageDetails,
},
MessageDetails,
message_race_loop::RaceStateImpl,
};

use super::*;

const DEFAULT_DISPATCH_WEIGHT: Weight = Weight::from_parts(1, 0);
const DEFAULT_SIZE: u32 = 1;

type TestRaceState = RaceState<TestSourceHeaderId, TestTargetHeaderId, TestMessagesProof>;
type TestRaceState = RaceStateImpl<
TestSourceHeaderId,
TestTargetHeaderId,
TestMessagesProof,
TestMessagesBatchTransaction,
>;
type TestStrategy =
MessageDeliveryStrategy<TestMessageLane, TestSourceClient, TestTargetClient>;

Expand Down Expand Up @@ -617,12 +632,13 @@ mod tests {
}

fn prepare_strategy() -> (TestRaceState, TestStrategy) {
let mut race_state = RaceState {
let mut race_state = RaceStateImpl {
best_finalized_source_header_id_at_source: Some(header_id(1)),
best_finalized_source_header_id_at_best_target: Some(header_id(1)),
best_target_header_id: Some(header_id(1)),
best_finalized_target_header_id: Some(header_id(1)),
nonces_to_submit: None,
nonces_to_submit_batch: None,
nonces_submitted: None,
};

Expand Down Expand Up @@ -964,14 +980,17 @@ mod tests {
);
// nothing needs to be delivered now and we don't need any new headers
assert_eq!(strategy.select_nonces_to_deliver(state.clone()).await, None);
assert_eq!(strategy.required_source_header_at_target(&header_id(1)), None);
assert_eq!(strategy.required_source_header_at_target(&header_id(1), state.clone()), None);

// now let's generate two more nonces [24; 25] at the soruce;
strategy.source_nonces_updated(header_id(2), source_nonces(24..=25, 19, 0));
//
// - so now we'll need to relay source block#2 to be able to accept messages [24; 25].
assert_eq!(strategy.select_nonces_to_deliver(state.clone()).await, None);
assert_eq!(strategy.required_source_header_at_target(&header_id(1)), Some(header_id(2)));
assert_eq!(
strategy.required_source_header_at_target(&header_id(1), state.clone()),
Some(header_id(2))
);

// let's relay source block#2
state.best_finalized_source_header_id_at_source = Some(header_id(2));
Expand All @@ -982,18 +1001,18 @@ mod tests {
// and ask strategy again => still nothing to deliver, because parallel confirmations
// race need to be pushed further
assert_eq!(strategy.select_nonces_to_deliver(state.clone()).await, None);
assert_eq!(strategy.required_source_header_at_target(&header_id(2)), None);
assert_eq!(strategy.required_source_header_at_target(&header_id(2), state.clone()), None);

// let's confirm messages [20; 23]
strategy.source_nonces_updated(header_id(2), source_nonces(24..=25, 23, 0));

// and ask strategy again => now we have everything required to deliver remaining
// [24; 25] nonces and proof of [20; 23] confirmation
assert_eq!(
strategy.select_nonces_to_deliver(state).await,
strategy.select_nonces_to_deliver(state.clone()).await,
Some(((24..=25), proof_parameters(true, 2))),
);
assert_eq!(strategy.required_source_header_at_target(&header_id(2)), None);
assert_eq!(strategy.required_source_header_at_target(&header_id(2), state), None);
}

#[async_std::test]
Expand Down Expand Up @@ -1025,6 +1044,7 @@ mod tests {
#[test]
#[allow(clippy::reversed_empty_ranges)]
fn no_source_headers_required_at_target_if_lanes_are_empty() {
let (state, _) = prepare_strategy();
let mut strategy = TestStrategy {
max_unrewarded_relayer_entries_at_target: 4,
max_unconfirmed_nonces_at_target: 4,
Expand Down Expand Up @@ -1053,7 +1073,7 @@ mod tests {
strategy.latest_confirmed_nonces_at_source,
VecDeque::from([(source_header_id, 0)])
);
assert_eq!(strategy.required_source_header_at_target(&source_header_id), None);
assert_eq!(strategy.required_source_header_at_target(&source_header_id, state), None);
}

#[async_std::test]
Expand Down
Loading