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.

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

CheckBridgedBlockNumber signed extension to reject duplicate header-submit transactions #1352

Merged

Conversation

svyatonik
Copy link
Contributor

@svyatonik svyatonik commented Mar 11, 2022

related to #1162 (we'll need a couple of other extensions to close the issue)

Things to do:

  • actually test it;
  • think about adding BestBlockNumber storage entry (or changing existing BestFinalized to store both block hash and number) to the grandpa pallet. This would allow us to use single storage read instead of two when validating. This needs to be done carefully (in a separate PR), because it may break existing deployments, which will need an upgrade in this case. UPD: extracted to Store best finalized header id (hash + number) instead of just hash in bridge-grandpa pallet #1368;
  • is there any other way than using macro to declare runtime-specific extension? Static type map or something like that? Maybe some existing macro. UPD: why do we need macro at all, instead of introducing multiple extensions - i.e. CheckBridgedBlockNumber<Runtime, WestendGrandpaInstance> and CheckBridgedBlockNumber<Runtime, RialtoGrandpaInstance>? That's because every extension has associated IDENTIFIER const, which has to be unique. The outcome of not being unique, iirc is invalid metadata. We could use adt_const_params rust feature if we were on nightly, but we are not. So the only solution I see now, is to have this single, multi-instance extension that can only be created using macro (as I said before - worth exploring).

Update related to last checkbox - I was thinking that possibly using impl_for_tuples would help. So something like:

trait FilterCall<Call> {
  fn is_valid(call: &Call) -> Option<bool>;
}

// implement `FilterCall` for Pallet<T, I> and for tuples, so we could use something like `(WithWestendGrandpa, WithRialtoGrandpa)`

type SignedExtension = SignedExtension<(WithWestendGrandpa, WithRialtoGrandpa)>;

But I've found no way to match (and deconstruct) runtime call when you're at pallet level. So given runtime call, but no knowledge of runtime, I can't do match inside FilterCall::is_valid. So I see no other option, but to use macro for now.

@svyatonik svyatonik added P-Runtime PR-audit-needed A PR has to be audited before going live. labels Mar 11, 2022
@svyatonik
Copy link
Contributor Author

Merging with deferred review

@serban300
Copy link
Collaborator

Related to:

* [x]  is there any other way than using macro to declare runtime-specific extension? Static type map or something like that? Maybe some existing macro. UPD: why do we need macro at all, instead of introducing multiple extensions - i.e. `CheckBridgedBlockNumber<Runtime, WestendGrandpaInstance>` and `CheckBridgedBlockNumber<Runtime, RialtoGrandpaInstance>`? That's because every extension has associated `IDENTIFIER` const, which has to be unique. The outcome of not being unique, iirc is invalid metadata. We could use `adt_const_params` rust feature if we were on nightly, but we are not. So the only solution I see now, is to have this single, multi-instance extension that can only be created using macro (as I said before - worth exploring).

Update related to last checkbox - I was thinking that possibly using impl_for_tuples would help. So something like:

trait FilterCall<Call> {
  fn is_valid(call: &Call) -> Option<bool>;
}

// implement `FilterCall` for Pallet<T, I> and for tuples, so we could use something like `(WithWestendGrandpa, WithRialtoGrandpa)`

type SignedExtension = SignedExtension<(WithWestendGrandpa, WithRialtoGrandpa)>;

But I've found no way to match (and deconstruct) runtime call when you're at pallet level. So given runtime call, but no knowledge of runtime, I can't do match inside FilterCall::is_valid. So I see no other option, but to use macro for now.

We might be able to match the call with something like:

fn validate<T: Config<I>, I: 'static + Callable<T, Call = crate::Call<T, I>>>(call: T::Call)
where
	T::Call: IsSubType<CallableCallFor<I, T>>,
	<T as pallet_bridge_grandpa::Config<T::BridgesGrandpaPalletInstance>>::BridgedChain:
		bp_runtime::Chain<
			BlockNumber = RelayBlockNumber,
			Hash = RelayBlockHash,
			Hasher = RelayBlockHasher,
		>,
{
	if let Some(crate::Call::<T, I>::submit_parachain_heads {
		ref at_relay_block,
		ref parachains,
		..
	}) = call.is_sub_type()
	{}
}

This example is for parachains but it can be adapted for the grandpa pallet. I will do some experiments these days to see if it works.

@serban300 serban300 mentioned this pull request Jul 20, 2022
@serban300 serban300 self-requested a review July 25, 2022 08:29
Copy link
Collaborator

@serban300 serban300 left a comment

Choose a reason for hiding this comment

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

The PR looks good and the comment related to reducing the number of macros has been addressed as part of #1519 . Marking the PR as reviewed.

wuminzhe pushed a commit to darwinia-network/darwinia-messages-substrate that referenced this pull request Aug 8, 2022
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024
…ubmit transactions (paritytech#1352)

* CheckBridgedBlockNumber signed extension to reject duplicate header submit transactions

* fix depends_on
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
…ubmit transactions (paritytech#1352)

* CheckBridgedBlockNumber signed extension to reject duplicate header submit transactions

* fix depends_on
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-Runtime PR-audit-needed A PR has to be audited before going live.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants