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

[Tx ext stage 2: 1/4] Add TransactionSource as argument in TransactionExtension::validate #6323

Merged
merged 8 commits into from
Nov 13, 2024

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Nov 1, 2024

Meta

This PR is part of 4 PR:

Description

One goal of transaction extension is to get rid or unsigned transactions.
But unsigned transaction validation has access to the TransactionSource.

The source is used for unsigned transactions that the node trust and don't want to pay upfront.
Instead of using transaction source we could do: the transaction is valid if it is signed by the block author, conceptually it should work, but it doesn't look so easy.

This PR add TransactionSource to the validate function for transaction extensions

@gui1117 gui1117 changed the title Add TransactionSource as argument in TransactionExtension::validate [1/4] Add TransactionSource as argument in TransactionExtension::validate Nov 1, 2024
@gui1117 gui1117 changed the title [1/4] Add TransactionSource as argument in TransactionExtension::validate [Tx ext stage 2: 1/4] Add TransactionSource as argument in TransactionExtension::validate Nov 1, 2024
@gui1117 gui1117 added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Nov 1, 2024
@gui1117 gui1117 marked this pull request as ready for review November 1, 2024 11:04
@gui1117 gui1117 requested a review from a team as a code owner November 1, 2024 11:04
@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 1, 2024 11:05
@@ -243,6 +245,7 @@ pub trait TransactionExtension<Call: Dispatchable>:
len: usize,
self_implicit: Self::Implicit,
Copy link
Contributor

Choose a reason for hiding this comment

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

outside of this PR: I think we'll also need:

/// Versions for which this Transaction Extension is applicable.
const VERSIONS: &'static [u8]

I could handle this in my metadata update PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we'll do it that way but rather a list of versions supported in the current TxExtension definition, which would end up being an enum like structure for each supported version. But in this format, it would not be on the TransactionExtension trait definition, but on the ExtrinsicMetadata definition, as in one for the big, tuple extension of the runtime.

Copy link
Member

Choose a reason for hiding this comment

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

@lexnv what you mean by this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've created a new PR for frame-metadata to add a few minor changes. The ExtrinsicMetadata looks like:

pub struct ExtrinsicMetadata<T: Form = MetaForm> {
	/// Extrinsic versions.
	pub versions: Vec<u8>,
	/// The type of the address that signs the extrinsic
	pub address_ty: T::Type,
	/// The type of the extrinsic's signature.
	pub signature_ty: T::Type,
	/// A mapping of supported transaction extrinsic versions to their respective transaction extension indexes.
	///
	/// For each supported version number, list the indexes, in order, of the extensions used.
	pub transaction_extensions_by_version: BTreeMap<u8, Vec<u32>>,
	/// The transaction extensions in the order they appear in the extrinsic.
	pub transaction_extensions: Vec<TransactionExtensionMetadata<T>>,
}
  • (1) ExtrinsicMetadata::versions should be [4 (legacy), 5 (new format)]. This can be done by changing the ExtrinsicMetadata trait

  • (2) TransactionExtensionMetadata are grouped by their extension version (currently only version 0u8 is supported). But in the future we might have multiple transaction extension versions which have different tx extensions and possibly different order of encoding

I wanted to get the information from (2) available for collection in metadata 🤔 Just to double check is my understanding of (2) correct? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think (1) is correct to me.

in (2) you mean that a transaction extension like CheckWeight will be valid for a predefined set of version? This is not the correct level. The runtime is defining multiple version of transaction extension set.

So a runtime can have:

  • Transaction extensions version 33: CheckWeight + Payment.
  • Transaction extensions version 34: CheckWeight + Payment + CheckMetadataVersion.
  • Transaction extensions version 35: Payment.

Then an extrinsic can choose one transaction extensions version.

I believe the syntax will be something a bit like: (syntax can be different)

type UncheckedExtrinsic = UncheckedExtrinsic<Address, Signature, ((const 33, TxExtensionV33), (const 34, TxExtensionV34), (const 35, TxExtensionV35))>

type TxExtensionV33 = (CheckWeight, Payment);

//etc...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah 1 and 2 make sense. However, I still don't get the initial comment. Maybe what Guillaume has written above makes it more clear. The version is in the runtime and not in the extension. If we need to change extensions, we need to give them a new name.

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM!

@georgepisaltu georgepisaltu added this pull request to the merge queue Nov 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 12, 2024
@gui1117 gui1117 added this pull request to the merge queue Nov 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 13, 2024
@gui1117 gui1117 added this pull request to the merge queue Nov 13, 2024
@gui1117 gui1117 removed this pull request from the merge queue due to a manual request Nov 13, 2024
@gui1117 gui1117 added this pull request to the merge queue Nov 13, 2024
github-merge-queue bot pushed a commit that referenced this pull request Nov 13, 2024
…tionExtension::validate` (#6323)

## Meta 

This PR is part of 4 PR:
* #6323
* #6324
* #6325
* #6326

## Description

One goal of transaction extension is to get rid or unsigned
transactions.
But unsigned transaction validation has access to the
`TransactionSource`.

The source is used for unsigned transactions that the node trust and
don't want to pay upfront.
Instead of using transaction source we could do: the transaction is
valid if it is signed by the block author, conceptually it should work,
but it doesn't look so easy.

This PR add `TransactionSource` to the validate function for transaction
extensions
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 13, 2024
@alvicsam alvicsam added this pull request to the merge queue Nov 13, 2024
Merged via the queue into master with commit 8e3d929 Nov 13, 2024
193 of 197 checks passed
@alvicsam alvicsam deleted the gui-tx-ext-stage-2-part-1 branch November 13, 2024 10:12
@gui1117 gui1117 added the A4-needs-backport Pull request must be backported to all maintained releases. label Nov 13, 2024
@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2407:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-6323-to-stable2407
git worktree add --checkout .worktree/backport-6323-to-stable2407 backport-6323-to-stable2407
cd .worktree/backport-6323-to-stable2407
git reset --hard HEAD^
git cherry-pick -x 8e3d929623d43398ed3ab8c9ca813aff32588011
git push --force-with-lease

@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2409:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-6323-to-stable2409
git worktree add --checkout .worktree/backport-6323-to-stable2409 backport-6323-to-stable2409
cd .worktree/backport-6323-to-stable2409
git reset --hard HEAD^
git cherry-pick -x 8e3d929623d43398ed3ab8c9ca813aff32588011
git push --force-with-lease

@gui1117 gui1117 restored the gui-tx-ext-stage-2-part-1 branch November 13, 2024 23:59
@gui1117 gui1117 deleted the gui-tx-ext-stage-2-part-1 branch November 14, 2024 00:00
gui1117 added a commit that referenced this pull request Nov 14, 2024
…tionExtension::validate` (#6323)

## Meta

This PR is part of 4 PR:
* #6323
* #6324
* #6325
* #6326

## Description

One goal of transaction extension is to get rid or unsigned
transactions.
But unsigned transaction validation has access to the
`TransactionSource`.

The source is used for unsigned transactions that the node trust and
don't want to pay upfront.
Instead of using transaction source we could do: the transaction is
valid if it is signed by the block author, conceptually it should work,
but it doesn't look so easy.

This PR add `TransactionSource` to the validate function for transaction
extensions

(cherry picked from commit 8e3d929)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A4-needs-backport Pull request must be backported to all maintained releases. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants